[pulseaudio-discuss] [PATCH 1/4] core: Add generic message interface

[Tanu Kaskinen]

On Fri, 2017-07-21 at 21:25 +0200, Georg Chini wrote:
> This patch adds a new feature to the core which allows to exchange
> messages between objects. An object can register/unregister a message
> handler with pa_core_{register, unregister}_message_handler() while
> any other object can check if a handler is registered for some name
> with pa_core_find_message_handler() and then call the returned handler.

Splitting the message sending into separate "find handler" and "use
handler" steps seems strange. Why not just pa_core_send_message() (or
maybe "handle" or "post" or "receive" would be better verbs, I'm not
sure)? The pa_core_message_handler struct could then be hidden in
core.c.

> The patch is a precondition for the following patches that also allow
> clients to send messages to pulseaudio objects.
> 
> There is no restriction on object names, but 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.
> 
> Objects also can have a short name which can be used in several ways.
> First objects of the same type should have the same short name, so
> that you can either send a message to all instances or to the first
> instance of an object.

The current code doesn't seem to provide an option to send a message to
multiple recipients.

> pa_core_find_message_handler() returns the
> first instance if a short name is specified as search string.
> Second, special objects like the default sink/source can have unique
> short names to make it easy to send messages to them.

I'd rather not add extra functionality if it's not used. Do you have
plans to add code that uses the short names?

I also think it would be better to not tie the short names directly in
the message handlers. In case there's an alias for the default sink,
then the alias is not a property of the sink message handler, because
the alias can be changed to point to a different sink. I'd think it as
a symlink. The short name/alias/symlink should have its own message
handler, which just forwards the message to the current "target"
object.

> ---
>  src/pulsecore/core-util.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/pulsecore/core-util.h | 22 +++++++++++++++
>  src/pulsecore/core.c      |  4 +++
>  src/pulsecore/core.h      |  2 +-
>  4 files changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/src/pulsecore/core-util.c b/src/pulsecore/core-util.c
> index d4cfa20c..2aa0e2e4 100644
> --- a/src/pulsecore/core-util.c
> +++ b/src/pulsecore/core-util.c

This code is better suited for core.c, since it modifies the pa_core
state.

> +/* Register message handler. recipient_name must be a unique name. short_name can be used
> + * to group objects of the same type, see pa_core_find_message_handler(). short_name and
> + * description are optional and may be NULL. */
> +int pa_core_register_message_handler(pa_core *c, const char *recipient_name, const char *short_name, const char *description, pa_core_message_handler_cb_t cb, void *userdata) {

What do you plan to use the description for? It seems potentially
unnecessary.

> +    pa_core_message_handler *handler;
> +
> +    pa_assert(c);
> +    pa_assert(recipient_name);
> +    pa_assert(cb);
> +    pa_assert(userdata);
> +
> +    if (pa_core_find_message_handler(c, recipient_name, &handler) > 0) {
> +        pa_log_warn("Cannot register new message handler, name %s is already registered.", recipient_name);
> +        return -1;
> +    }

Isn't it a bug to try to register a message handler twice? An assertion
would seem appropriate here. Or actually, there's already an assertion
for the pa_hashmap_put() call, so no checking is needed here.

-- 
Tanu

https://www.patreon.com/tanuk