(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
