[pulseaudio-discuss] [PATCH v3 04/11] tunnel-manager: New module for managing tunnels to remote servers

[Alexander E. Patrakov]

(I have also looked at previous patches in the series, but have no 
comments about them)

04.12.2014 23:44, Tanu Kaskinen wrote:
> +void pa_tunnel_manager_remote_server_new(pa_tunnel_manager *manager, pa_tunnel_manager_remote_server_config *config) {

Why void? There are some failure conditions (such as bad address).

> +    int r;
> +    pa_parsed_address parsed_address;
> +    pa_tunnel_manager_remote_server *server = NULL;
> +
> +    pa_assert(manager);
> +    pa_assert(config);
> +
> +    if (!config->address) {
> +        pa_log("No address configured for remote server %s.", config->name);
> +        return;
> +    }
> +
> +    r = pa_parse_address(config->address->value, &parsed_address);
> +    if (r < 0) {
> +        pa_log("[%s:%u] Invalid address: \"%s\"", config->address->filename, config->address->lineno, config->address->value);
> +        return;
> +    }
> +
> +    pa_xfree(parsed_address.path_or_host);
> +
> +    server = pa_xnew0(pa_tunnel_manager_remote_server, 1);
> +    server->manager = manager;
> +    server->name = pa_xstrdup(config->name);
> +    server->address = pa_xstrdup(config->address->value);
> +    server->devices = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
> +    server->device_stubs = pa_hashmap_new(NULL, NULL);
> +
> +    pa_assert_se(pa_hashmap_put(manager->remote_servers, server->name, server) >= 0);
> +
> +    pa_log_debug("Created remote server %s.", server->name);
> +    pa_log_debug("    Address: %s", server->address);
> +    pa_log_debug("    Failed: %s", pa_boolean_to_string(server->failed));

How can server->failed become true here? Maybe you meant this debug 
output to be placed after set_up_connection()?

> +
> +    set_up_connection(server);
> +}

<snip>

> +static void set_up_connection(pa_tunnel_manager_remote_server *server) {
> +    pa_assert(server);
> +    pa_assert(!server->context);
> +
> +    server->context = pa_context_new(server->manager->core->mainloop, "PulseAudio");
> +    if (server->context) {
> +        int r;
> +
> +        r = pa_context_connect(server->context, server->address, PA_CONTEXT_NOFLAGS, NULL);
> +        if (r >= 0)
> +            pa_context_set_state_callback(server->context, context_state_cb, server);
> +        else {
> +            pa_log("[%s] pa_context_connect() failed: %s", server->name, pa_strerror(pa_context_errno(server->context)));
> +            pa_tunnel_manager_remote_server_set_failed(server, true);
> +        }
> +    } else {
> +        pa_log("[%s] pa_context_new() failed.", server->name);
> +        pa_tunnel_manager_remote_server_set_failed(server, true);
> +    }
> +}

Here one can reduce the need for "else" statements and thus increase 
readability by handling the error cases first. I.e.: if 
(!server->context) { ... ; return; }

-- 
Alexander E. Patrakov