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

[Tanu Kaskinen]

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.

> 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.

>  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.

> +
> +  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.

> +
> +    /* 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.

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.)

> 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.

-- 
Tanu

https://www.patreon.com/tanuk