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

[Georg Chini]

On 23.09.2017 13:59, Tanu Kaskinen wrote:
> On Sat, 2017-08-19 at 17:48 +0200, Georg Chini wrote:
>> This patch adds a new feature to the core which allows to send
>> messages to objects. An object can register/unregister a message
>> handler with pa_core_message_handler_{register, unregister}() while
> I would prefer pa_core_register_message_handler(), but if you don't
> like that, you can keep it as is.

Will change it.

>> a message can be sent to the handler using the pa_core_send_message()
>> function. A message has 4 arguments (apart from passing the core):
>>
>> recipient: The name of the message handler that will receive the message
>> message: message command
>> message_parameters: A string containing additional parameters
>> response: Pointer to a response string that will be filled by the
>>            message handler. The caller is responsible to free the string.
>>
>> The patch is a precondition for the following patches that allow clients
>> to send messages to pulseaudio objects.
>>
>> There is no restriction on object names, except that a handler name
>> always starts with a "/". The intention is to use a path-like syntax,
>> for example /core/sink_1 for a sink or /name/instances/index for modules.
>> The exact naming convention still needs to be agreed.
>> ---
>>   src/Makefile.am               |   1 +
>>   src/pulsecore/core-messages.c | 118 ++++++++++++++++++++++++++++++++++++++++++
>>   src/pulsecore/core-messages.h |  51 ++++++++++++++++++
> I would prefer to have the functions in core.[ch], but if you don't
> like that, you can keep it as it is.

I would like to keep it in a separate file. I think it is cleaner. Also,
the pa_signal_post() function that is added in one of the later
patches seemed to fit neither into core.[ch] nor into
core_subscribe.[ch].

>>   src/pulsecore/core.c          |   4 ++
>>   src/pulsecore/core.h          |   2 +-
>>   5 files changed, 175 insertions(+), 1 deletion(-)
>>   create mode 100644 src/pulsecore/core-messages.c
>>   create mode 100644 src/pulsecore/core-messages.h
>>
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 3ff1139f..7355cf03 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -957,6 +957,7 @@ libpulsecore_ at PA_MAJORMINOR@_la_SOURCES = \
>>   		pulsecore/core-scache.c pulsecore/core-scache.h \
>>   		pulsecore/core-subscribe.c pulsecore/core-subscribe.h \
>>   		pulsecore/core.c pulsecore/core.h \
>> +		pulsecore/core-messages.c pulsecore/core-messages.h \
>>   		pulsecore/hook-list.c pulsecore/hook-list.h \
>>   		pulsecore/ltdl-helper.c pulsecore/ltdl-helper.h \
>>   		pulsecore/modargs.c pulsecore/modargs.h \
>> diff --git a/src/pulsecore/core-messages.c b/src/pulsecore/core-messages.c
>> new file mode 100644
>> index 00000000..69055786
>> --- /dev/null
>> +++ b/src/pulsecore/core-messages.c
>> @@ -0,0 +1,118 @@
>> +/***
>> +  This file is part of PulseAudio.
>> +
>> +  Copyright 2004-2006 Lennart Poettering
>> +  Copyright 2006 Pierre Ossman <ossman at cendio.se> for Cendio AB
> Lennart and Pierre didn't write this code. You can put your own name
> here. Or alternatively you can leave this part out altogether. To me
> the "Copyright 20XX Some Name Here" convention seems pretty useless,
> because the information becomes out of date as soon as someone else
> modifies the code.

OK, I'll leave out the Copyright. Always wondered why it is there.

>
>> +
>> +  PulseAudio is free software; you can redistribute it and/or modify
>> +  it under the terms of the GNU Lesser General Public License as published
>> +  by the Free Software Foundation; either version 2.1 of the License,
>> +  or (at your option) any later version.
>> +
>> +  PulseAudio is distributed in the hope that it will be useful, but
>> +  WITHOUT ANY WARRANTY; without even the implied warranty of
>> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> +  General Public License for more details.
>> +
>> +  You should have received a copy of the GNU Lesser General Public License
>> +  along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
>> +***/
>> +
>> +#ifdef HAVE_CONFIG_H
>> +#include <config.h>
>> +#endif
>> +
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +
>> +#include <pulse/xmalloc.h>
>> +
>> +#include <pulsecore/core.h>
>> +#include <pulsecore/core-util.h>
>> +#include <pulsecore/log.h>
>> +#include <pulsecore/macro.h>
>> +
>> +#include "core-messages.h"
>> +
>> +struct pa_core_message_handler {
>> +    char *recipient;
>> +    char *description;
>> +    pa_core_message_handler_cb_t callback;
>> +    void *userdata;
>> +};
>> +
>> +/* Message handler functions */
>> +
>> +/* Register message handler. recipient_name must be a unique name starting with "/". */
>> +void pa_core_message_handler_register(pa_core *c, const char *recipient_name, const char *description, pa_core_message_handler_cb_t cb, void *userdata) {
>> +    struct pa_core_message_handler *handler;
>> +
>> +    pa_assert(c);
>> +    pa_assert(recipient_name);
>> +    pa_assert(cb);
>> +    pa_assert(userdata);
>> +
>> +    /* 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?

>
>> +
>> +    /* Name must not be in use. */
>> +    pa_assert(!pa_hashmap_get(c->message_handlers, recipient_name));
> This is redundant, because the assertion at the end of the function
> does the same thing.
>
>> +
>> +    handler = pa_xnew0(struct pa_core_message_handler, 1);
>> +    handler->userdata = userdata;
>> +    handler->callback = cb;
>> +    handler->recipient = pa_xstrdup(recipient_name);
>> +    handler->description = pa_xstrdup(description);
>> +
>> +    pa_assert_se(pa_hashmap_put(c->message_handlers, handler->recipient, (void *) handler) == 0);
>> +}
>> +
>> +/* Unregister a message handler */
>> +void pa_core_message_handler_unregister(pa_core *c, const char *recipient_name) {
>> +    struct pa_core_message_handler *handler;
>> +
>> +    pa_assert(c);
>> +    pa_assert(recipient_name);
>> +
>> +    pa_assert_se(handler = pa_hashmap_remove(c->message_handlers, recipient_name));
>> +
>> +    pa_xfree(handler->recipient);
>> +    pa_xfree(handler->description);
>> +    pa_xfree(handler);
>> +}
>> +
>> +/* 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.

> So, I propose that you drop the PA_CORE_SEND_* enum and use the regular
> error codes instead.
>
>> +
>> +    return PA_CORE_SEND_OK;
>> +}
>> +
>> +/* 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.

>
>> diff --git a/src/pulsecore/core-messages.h b/src/pulsecore/core-messages.h
>> new file mode 100644
>> index 00000000..d34e304e
>> --- /dev/null
>> +++ b/src/pulsecore/core-messages.h
>> @@ -0,0 +1,51 @@
>> +#ifndef foocoremessageshfoo
>> +#define foocoremessageshfoo
>> +
>> +/***
>> +  This file is part of PulseAudio.
>> +
>> +  Copyright 2004-2006 Lennart Poettering
> See the earlier comment about copyrights.
>