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

[Georg Chini]

On 27.07.2017 07:17, Tanu Kaskinen wrote:
> 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.

I have to think about it, but you are probably right. Although it might
still make sense to have a is_handler_installed() boolean function, so
that the sender can check if handlers for certain objects are present.

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

Yes, I have not added a "find_all_handlers" function because
I have no current use case, but it could easily be implemented if it
should for example be necessary to send a message to all sinks.
Maybe I should make it more clear that there is no special code
for passing messages to multiple recipients.
If you want to send messages to multiple recipients, you have to iterate
over the objects. Here again, the find_handler function comes in handy,
but it could be solved differently.

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

This is not really extra functionality if you want to be able to easily
address the first instance of an object. Consider an object, that has
multiple handlers installed (maybe plugins).
So you might have multiple names that start with /object_prefix. If
you don't have a short name, you need to iterate through the list
of installed handlers to find the best match. With the short name,
you just pick the first handler with a matching short name.
Think of the short name as a category identifier. It seemed too
difficult to find rules that reliably map object name prefixes to object
category, because there is (at least up to now) no rule for  the object
names themselves.

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

I don't understand. There is no alias for the default sink. It's the other
way round, default_sink is an alias for a real object which has a handler.
Which object would install the handler for the default sink? If the alias
is stored with the handler and the default sink changes, you just have
to move the alias name to the handler of the new default sink. That does
not involve setting up any "dummy" handlers.
Using this feature does however not prevent your approach if it is
necessary in some special situation.

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

OK.

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

You are right, it is potentially unnecessary. But I thought it would be
a good idea to give the user the ability to list the objects that can
receive messages. The last patch of the series implements a small
message handler which allows to list installed handlers. Consider you
have a couple of loopback modules and want to address one of them
using pactl or pacmd.
The names will probably look like /loopback/instances/module_index,
so they do not provide a lot of information. The description field could
indicate source and sink.


>
>> +    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.
>
There might be a reason for a message handler to try different names if
one name is not available. I did not want to exclude this possibility, so I
return -1 and leave it to the caller to handle the error properly.  Also, if
for example a module fails to register a handler it would be better if just
the module-load fails instead of a server crash.

I can change it to an assertion if you think it's better.