[pulseaudio-discuss] [PATCH 1/3] core: add generic message interface

Wed Aug 16 14:56:36 UTC 2017

On Sun, 2017-08-06 at 15:29 +0200, Georg Chini wrote:
> On 06.08.2017 07:26, Tanu Kaskinen wrote:
> > On Sat, 2017-08-05 at 21:32 +0200, Georg Chini wrote:
> > > On 05.08.2017 13:37, Tanu Kaskinen wrote:
> > > > On Fri, 2017-08-04 at 15:37 +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_message_handler_{register, unregister}() while
> > > > > any other object can send a message to the handler using the
> > > > > pa_core_send_message() function. A message has 5 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
> > > > > message_data: void pointer to some parameter structure, can be used
> > > > >                 as alternative to message_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 also allow
> > > > > clients to send messages to pulseaudio objects.
> > > > > 
> > > > > Because not every message handler should be visible to clients, a flag
> > > > > was added to the handler structure which allows to mark a handler as
> > > > > public or private.
> > > > > 
> > > > > 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.
> > > > > 
> > > > > Message groups are also implemented, so that a handler can subscribe
> > > > > to a message group using pa_core_message_handler_group_[un]subscribe()
> > > > > to receive messages sent to the group. To distinguish group and handler
> > > > > names, group names lack the leading "/". Some of the code used to
> > > > > implement the message groups was adapted from hook-list.c. Message
> > > > > groups are created/deleted implicitely on subscription/unsubscription.
> > > > > 
> > > > > The messaging interface can serve as a full replacement for the current
> > > > > hook system with several advantages:
> > > > > - no need to change header files when a new handler/group is implemented
> > > > > - slightly simpler registration interface
> > > > > - multi-purpose message handlers that can handle multiple events
> > > > > - mesage handlers may also be accessible from the client side
> > > > 
> > > > We agree that it's good to allow clients to send messages to modules.
> > > > Unfortunately, in this patch you're assuming that we'll also replace
> > > > hooks with the same system. Can we please keep things simple and do one
> > > > change at a time? I'm not enthusiastic about replacing hooks, and I'd
> > > > rather move on with the client message passing before getting consensus
> > > > on the hook stuff.
> > > > 
> > > > For reference, here's a list of unnecessary (from pure client message
> > > > passing point of view) things I can gather from the commit message:
> > > > 
> > > > - void pointer argument in message handlers
> > > > - public/private flag
> > > > - message groups (signals would seem like a better fit for the purpose
> > > > anyway)
> > > > 
> > > 
> > > Possibly I am only using the wrong words. Let me put it like that:
> > > My intention is not to replace the hooks but to incorporate them
> > > into a more general concept. I even used a lot of the code and
> > > put it in the new message context. The functionality of the hooks
> > > is a subset of the messages concept and is fully preserved.
> > > So by replacing hooks with messages, nothing would be lost
> > > (at least not intentionally), only the interface would change.
> > > Take a look at the second patch of the series for an example.
> > > 
> > > The only disadvantage I can think of is that there is one more
> > > lookup step required compared to the hooks when finding
> > > the right handler.
> > 
> > For starters, the conversion has to be implemented, reviewed, and the
> > new interface has to be learned by everyone. Even if I thought that the
> > message system didn't have any API design problems, it's not clear that
> > the conversion would be worth the effort. I would probably be ok with
> > some new API that allows using one callback for multiple hooks, but I
> > think you'll need to get an ok from Arun too. I won't elaborate on the
> > API design issues now, because I don't think it's good to block the
> > client message passing feature.
> 
> Why would it be urgent to get the feature implemented? Nobody is
> pressing us and we are designing a new API.

It's not urgent, that's true, but it would be nice to make some
progress instead of having things stuck in long discussions because you
bundle stuff I don't like with the stuff we already agreed on earlier.

> In my opinion, most
> of the thinking should go into the design phase to get it right from
> the start. It's not a good idea to come up with something quick,
> only to have more work patching it up later.

Small incremental changes are generally nicer to work with than big
changes. The things under discussion don't affect the client side API,
so there's no need to get "everything right" in the sense that the API
needs to support everything that might possibly be nice some day. The
core API can be freely amended when the need comes.

> I think you are reacting too strong (nearly only) to my statement, that
> it would be possible to replace the hooks. All our discussion has been
> centered around that topic up to now, while this is not at all the main
> intention of the patch. It is just mentioned as one possibility in the
> commit message and I can leave out this part if you prefer. You are
> right, for the hooks themselves there is no obvious reason why they
> should be replaced. The mechanism is working, long established and
> seems sufficient for what we are doing with it. I also have no objections
> to it. So let us assume that there will be no replacement and come back
> to what the patch actually does.
> 
> Our main disagreement seems to be that you want a very simple and
> client-only way to pass messages, while my goal is to design a versatile
> message interface that can be used internally and externally. If it was
> only about clients sending messages to modules, my first approach
> would have been fully sufficient. It can even be done with the extension
> interface today.
> 
> I wonder if you are at all willing to accept something more general or
> are completely focused on the client-only approach. I hoped, that the
> features and the two use cases (internal: implementation of signals,
> external: message handler to list handlers) could convince you that
> the new messaging system is a definite win for pulseaudio even if it
> partly duplicates existing functionality.

Basing signals on the other message passing API doesn't seem like a
good fit to me. With signals, the destination isn't interesting for the
signal sender. If I understood right, you're using the destination to
identify the signal, but the signal name already does that. Also, the
message API doesn't natively provide the signal sender identification,
so I assume the sender object path has to be always included in the
message parameters. Contrast this with D-Bus: every message has the
"member" attribute, which doesn't need to be parsed from the message
parameters. With method calls the member identifies the destination,
with signals the member identifies the sender.

> My current implementation might have drawbacks and problems
> that I do not see, but I am willing to put more work into it until those
> potential issues are solved. I already pointed out multiple advantages
> over existing mechanisms and your categoric denial currently seems
> only based on the hook discussion and therefore a bit unfounded to
> me.

I hope the previous paragraph above explains why I don't think this
message API should be used for signals. With that use case and the hook
use case left out, there aren't any immediate use cases left, only
speculation about things that might be nice some day.

> As a side note: Message groups would also make sense from a client
> perspective. If a client could create a group (not implemented, but could
> be easily done), multiple instances of the same object could be controlled
> by one group. Consider an application implementing something like a
> "precision" combine sink by using loopbacks from a null-sink to multiple
> real sinks. Then the loopbacks could be added to some my-loopbacks
> group and controlled via that group.

This can be added when someone actually needs this. (Some food for
thought: if there's just one message from the client, but multiple
recipients, how to report errors when one recipient successfully
performs the client request and another fails?)

> If you are willing to accept some "versatile message interface", I would
> re-work it again. I would then appreciate your additional input regarding
> design.
> 
> Otherwise I will reluctantly strip it down to what you want because further
> discussion seems pointless.

A stripped-down version of the patches would be lovely, thanks.

-- 
Tanu

https://www.patreon.com/tanuk