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

[Tanu Kaskinen]

On Thu, 2017-07-27 at 22:36 +0200, Georg Chini wrote:
> 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.

Yes, the commit message needs some fine tuning.

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

I don't have a need for that. Do you?

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

That's what I meant.

> Which object would install the handler for the default sink?

pa_core_update_default_sink() would set the alias. I'd expect the
messaging infrastructure to provide a simple way to set up aliases:

pa_core_set_message_recipient_alias(core, "default_sink", "/sinks/sink_42")
pa_core_remove_message_recipient_alias(core, "default_sink")

Setting the alias would create a message handler that forwards the
message handler to the named recipient.

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

What if there's need for multiple aliases for one object? In this
hypothetical situation where "default_sink" is an alias for a sink,
your solution doesn't allow any other aliases to be used for sinks. If
you also want an alias for accessing the first sink (although I don't
see the need), you can't do that for sinks, because the "default_sink"
alias is already occupying the only "slot" available for sinks.

(This is all very theoretical, though. AFAIK, you don't plan to
implement the code for a default sink alias in any case, and I don't
see other uses for the alias feature getting implemented either, so
currently it seems like the alias feature can just be dropped.)

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

Ok, that seems like a nice feature.

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

I'd prefer an assertion until there's need for something else. It's
allows pa_core_register_message_handler() to return void, which avoids
the need for error handling code in the callers.

-- 
Tanu

https://www.patreon.com/tanuk