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

[Georg Chini]

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.

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

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

> The rationale, if you've forgotten it, was that it's
> annoying for application developers if the core API uses inconsistent
> conventions and introspection functionality needs to be searched from
> two different places.
>
> I might accept it if you put the client-facing bits in the
> introspection API, but use the messaging system behind the scenes.

I think it does not really make sense to use the messaging API
at all in that case. It's either via messaging or introspection API
and I have to admit that adding to the introspection API for such
a simple information request is exactly what I wanted to avoid.
The introspection API has massive overhead. Take a look how
many places you have to touch to add some tiny functionality.

The messaging system provides a simple way to get or send
information whenever this information can be expressed as
a string. This is where the messaging system can add real
value and getting the handlers and description is a typical
example for that.
The point is, that functionality can be added by simply writing
a message handler without having to deal with the complexity
of the introspection API.

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

>
>> b) We should at least have one example in the code
>> which shows how a message handler i implemented.
> It's certainly good to have an example now so that we can discuss how
> the message parameter serialization is supposed to work, but I don't
> think we need to apply that example to master. I hope we'll have some
> other example by the time 12.0 is released (IIRC, you plan to add a
> message handler to module-loopback), but it's not a big deal if there
> are no examples.

Yes, I will add a message handler to module-loopback, but there
are still a lot of outstanding patches for it and first tests showed
that adding a message handler to change the latency on the fly
has non-trivial implications. So not sure if I will have it for 12.0.

The original motivation for writing the patches was an equalizer
module developed by somebody in Italy which looked very
promising to me. But the equalizer would need a GUI to change
parameters to be really useful and I was looking for an easy
way to do the parameter exchange between the module and
the GUI. So maybe this might go into 12.0.

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