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

[Tanu Kaskinen]

On Fri, 2014-12-05 at 00:37 +0500, Alexander E. Patrakov wrote:
> (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).

Because the only caller of the function doesn't care about whether the
function fails or not. But now that you made me think about this again,
it's better to always report failures, because making the function
signature look like it can't fail can cause bugs later, if another call
site is added where failures matter. I'll fix this.

> > +    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()?

It can't become true, but the code works as intended nevertheless.

The rationale behind this is that I think it's a good idea to log the
initial state of new objects. That way you'll have full history of the
variable state in the log.

I don't want to put this message after set_up_connection() either,
because the server may fail in set_up_connection(), which will cause
this message to be logged: "[%s] Failed changed from false to true." I
don't want any such "changed" messages in the log before the initial
object creation message. I hope this makes sense, but I can understand
if people have different taste regarding log messages.

> > +
> > +    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; }

Thanks, I'll do that. My excuse is that the code was probably at some
point in a context where an early return would have broken things.

-- 
Tanu