[pulseaudio-discuss] [PATCH 4/6] core: add message handler

Wed Oct 4 11:00:59 UTC 2017

On Tue, 2017-10-03 at 16:31 +0200, Georg Chini wrote:
> On 03.10.2017 14:58, Tanu Kaskinen wrote:
> > On Mon, 2017-10-02 at 14:32 +0200, Georg Chini wrote:
> > > On 02.10.2017 11:31, Tanu Kaskinen wrote:
> > > > On Sun, 2017-10-01 at 20:31 +0200, Georg Chini wrote:
> > > > > On 01.10.2017 18:16, Tanu Kaskinen wrote:
> > > > > > On Sat, 2017-08-19 at 17:48 +0200, Georg Chini wrote:
> > > > > > > +/* List handlers */
> > > > > > > +char *pa_core_message_handler_list(pa_core *c);
> > > > > > 
> > > > > > Putting this function to core-messages.h doesn't seem right to me. The
> > > > > > function will never be used outside core.c, so the it should be a
> > > > > > private function in core.c.
> > > > > > 
> > > > > 
> > > > > If I put it in core.c, I would need to expose the pa_core_message_handler
> > > > > structure, which you wanted to keep private.
> > > > 
> > > > Something that didn't occur to me when I reviewed this patch yesterday
> > > > is that the string that the list-handlers returns is tailor-made for
> > > > pactl/pacmd, which is not how the message handlers are expected to
> > > > behave. What if an application wants to not only print a message (of
> > > > which layout is already decided by the server), but to get a list of
> > > > handlers and do something else with that information?
> > > 
> > > I don't understand. Message handlers return a single string, so
> > > parsing the result is the task of the sender. The format returned
> > > by the handler is rather simple, apart from the header line
> > > it only consists of lines of the form
> > > 
> > > handler-path, " ", description
> > > 
> > > This should be easy to parse.
> > 
> > Not as easy as it would be with a standardized serialization format.
> > 
> > If we assume that the information that list-handlers returns is
> > basically a list of (path,description) pairs (which IMO is not a good
> > idea, I'll discuss that later), as an application developer I'd want to
> > be able to do this rather than dealing with the details of string
> > parsing:
> > 
> > const char *state = message_parameters;
> > 
> > if (pa_message_read_start_list(&state) < 0)
> >      fail("Expected a list.");
> > 
> > while (pa_message_read_end_list(&state) < 0) {
> >      char *path;
> >      char *description;
> > 
> >      if (pa_message_read_path(&state, &path) < 0)
> >          fail("Expected a path.");
> >      if (pa_message_read_string(&state, &description) < 0)
> >          fail("Expected a string.")
> > 
> >      /* Do something with path and description. */
> > 
> >      pa_xfree(description);
> >      pa_xfree(path);
> > }
> > 
> > The documentation becomes much simpler too. You don't need to describe
> > the string in detail (what are the separators, how to deal with
> > escaping etc.), this is sufficient to document the exact format of the
> > message:
> > 
> >      list-handlers
> >          parameters: none
> >          response: [Path String]
> > 
> >          The message returns all message handlers as a list. The list
> >          items contain the handler path and the handler description.
> > 
> > It can be explained just once how to read this notation. The
> > "parameters:" section describes the parameters of the message (in this
> > case there are none) and the "response:" section describes type of the
> > response parameters. Square brackets denote a list, and inside the
> > square brackets is the list item type (in this case the items consist
> > of a Path and a String value).
> 
> I still don't understand. The return value of a message is always a
> string, so all results that are passed back to the caller must be part
> of a single string. The message handler is not passing back any
> structures or lists.
> What you propose could only be done with the introspection API.

I don't see why my proposal couldn't be done with strings. The string
formatting just needs to be well defined. Here's an example of what a
list of (path,description) pairs could look like:

[ /core "Core message handler" /core/sink/alsa_output.pci-0000_00_1b.0.analog-stereo/alsa "ALSA message handler for Built-in Audio Analog Stereo" ]

pa_message_read_start_list() would consume "[ " and set the state
variable point to the beginning of /core. pa_message_read_path() would
consume "/core " and set the state variable point to the beginning of
"Core message handler". And so on.

> > Now, on the topic why (path,description) pairs is not a very good idea:
> > choosing that structure assumes that message handlers won't have any
> > other properties in the future. We can't change the response format
> > specification, so it should be flexible enough so that we can add new
> > properties without breaking applications that were written before the
> > new properties existed.
> > 
> > If we use (path,description) pairs now, and for example add the handler
> > type (e.g. sink, loopback, etc) as a new property later, we'll have to
> > introduce a new message to be able to deal with that. Not ideal.
> 
> Yes, not ideal, but it would be the only way to deal with it.
> See below for some reasoning why I still think it should be
> implemented using messages.

That's not the only way to deal with it. list-handlers could return a
value corresponding to the D-Bus signature "{o{sv}}". That can be
serialized into the following string, for example:

{ /core { "description" string:"Core message handler" } /core/sink/alsa_output.pci-0000_00_1b.0.analog-stereo/alsa { "description" string:"ALSA message handler for Built-in Audio Analog Stereo" } }

If the properties are sent as a dictionary with variant values, it's
possible to add new properties without breaking old clients.

In this example the description value is prefixed with "string:",
because it's a variant, and parsing a variant needs a type annotation.

> > > > Also, it's questionable why the list-handlers command is implemented
> > > > with the messaging API in the first place. It's core functionality, so
> > > > why is it not implemented using the normal introspection API?
> > > 
> > > Two reasons:
> > > 
> > > a) It's much simpler than using the introspection API
> > 
> > Ok, but didn't we agree earlier that the messaging API is only meant
> > for modules, and new core features shall be added using the existing
> > API conventions?
> 
> No, we did not. If that was the purpose, my first approach to pass
> parameters to modules would have been fully sufficient. On the
> contrary, you wanted to have something more general, so that
> any object could have a message handler.

I've forgotten the details of the first version. If it used the module
name as the message recipient identifier, then that was not sufficient
(or maybe it could have been made work somehow, but it's certainly
nicer to have identifiers that are not permanently tied to a particular
module).

Also, I'd like to keep the option open that we will replace the whole
introspection API with the messaging system at some point in the
future.

> > If
> > you want pursue that route, I'd like get an ok from Arun as well. This
> > is not only about the list-handlers message, it's about all future core
> > features.
> > 
> > Another possibility is to deprecate the whole introspection API and
> > reimplement it using the messaging API, but you probably don't want to
> > take such a big project.
> 
> I wonder why you are talking black and white here. As said above,
> I think the messaging API could be used for simple tasks, while
> the introspection API is useful if the information exchanged is
> rather complex. I don't think developers are overburdened if they
> have to look in two places for some functionality.

Ok. We value different things, let's agree to disagree.

> > > > And one more thing: if the handler descriptions are visible to clients
> > > > and the descriptions are mutable, clients should be notified about
> > > > description changes.
> > > > 
> > > 
> > > How? Sending a signal?
> > 
> > A signal would be appropriate if the list-handlers operation is
> > implemented only as a message, but if it's implemented using the
> > introspection API, then the subscription system should be used for the
> > notifications.
> > 
> > Another thing that came to my mind is that notifications should also be
> > sent when message handlers are added and removed.
> 
> So you mean adding a PA_SUBSCRIPTION_EVENT_MESSAGE_HANDLER?
> This would be subject of an additional patch I think.

Yes.

-- 
Tanu

https://www.patreon.com/tanuk