[pulseaudio-discuss] [PATCH 1/6] core: add simple message interface

[Tanu Kaskinen]

On Sat, 2017-09-23 at 14:45 +0200, Georg Chini wrote:
> On 23.09.2017 13:59, Tanu Kaskinen wrote:
> > On Sat, 2017-08-19 at 17:48 +0200, Georg Chini wrote:
> > > +    /* Ensure that the recipient name is not empty and starts with "/". */
> > > +    pa_assert(recipient_name[0] == '/');
> > 
> > There could be more checks for ensuring that the name doesn't contain
> > any weird characters like quotes. Not that it's very important, though,
> > since it's unlikely that anyone would ever try to register that kind of
> > names.
> 
> Is there some appropriate PA function that does some sanity checks?

I don't think there is. pa_namereg_is_valid_name() could otherwise be
used, but it doesn't like slashes.

> > > +/* Send a message to a recipient or recipient group */
> > > +int pa_core_send_message(pa_core *c, const char *recipient_name, const char *message, const char *message_parameters, char **response) {
> > > +    struct pa_core_message_handler *handler;
> > > +
> > > +    pa_assert(c);
> > > +    pa_assert(recipient_name);
> > > +    pa_assert(message);
> > > +    pa_assert(response);
> > > +
> > > +    *response = NULL;
> > > +
> > > +    if (!(handler = pa_hashmap_get(c->message_handlers, recipient_name)))
> > > +        return PA_CORE_SEND_NO_RECIPIENT;
> > 
> > I think it would be better to return -PA_ERR_NOENTITY here...
> > 
> > > +
> > > +    if (handler->callback(handler->recipient, message, message_parameters, response, handler->userdata) < 0)
> > > +        return PA_CORE_SEND_FAILURE;
> > 
> > ...and let the message handler choose what error code shall be returned
> > to the caller. Now all errors (except the "handler not found" error)
> > appear as "internal errors" to the client, which is not nice.
> 
> The idea was to have an error string in response instead of using
> multiple error codes, but we could do both if you prefer.

An error string would be great, but to me it seems that you haven't
implemented a mechanism for returning such string. In any case, even if
a human-friendly error string is sent, we should also send a machine-
friendly code for identifying the kind of error, either numeric or a
string.

> > > +/* Set handler description */
> > > +int pa_core_message_handler_set_description(pa_core *c, const char *recipient_name, const char *description) {
> > > +    struct pa_core_message_handler *handler;
> > > +
> > > +    pa_assert(c);
> > > +    pa_assert(recipient_name);
> > > +
> > > +    if (!(handler = pa_hashmap_get(c->message_handlers, recipient_name)))
> > > +        return -1;
> > > +
> > > +    pa_xfree(handler->description);
> > > +    handler->description = pa_xstrdup(description);
> > > +
> > > +    return 0;
> > > +}
> > 
> > This function doesn't seem to be used in any of your patches. Do you
> > plan to use this later? (I don't count "maybe some day" as a plan.)
> 
> In a way it is a "maybe some day" plan. I think the description needs to
> be changeable. Consider my original example with different loopbacks.
> If the description contains something like source and sink, then, if the
> loopback is moved, that must be reflected in the description. And there
> are probably many other situations you can come up with where it
> makes sense to change the description on the fly.
> Nevertheless, if you prefer I can leave out the function.

That's a good use case, and there will likely be similar cases. You can
keep the function or drop it (and add it later), I don't mind.

-- 
Tanu

https://www.patreon.com/tanuk