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.
> >
> > I think it actually makes sense to make the struct public. It could be
> > private if it didn't have the description field, but since the handler
> > objects contain information that clients can query, they're similar to
> > all other core objects. I think it even makes sense to move the struct
> > to a separate message-handler.h header (but if you don't like that
> > idea, I'm fine with keeping it in core-messaging.h or moving it to
> > core.h).
>
> You want a header file just for this struct? I would keep it in
> core-messages.h but if you prefer, I don't mind using a
> separate file.
Indeed, creating a separate header just for the struct definition
doesn't make much sense. After thinking this a bit more, I still like
the idea of having message-handler.[ch] for the message handler object,
though. The message-handler.h header could include not only the struct
definition, but also the following functions:
pa_message_handler_new()
(renamed from pa_core_message_handler_register())
pa_message_handler_free()
(renamed from pa_core_message_handler_unregister)
pa_message_handler_set_description()
(renamed from pa_core_message_handler_set_description())
The only thing left in core-messaging.h would be
pa_core_send_message(), and I'd move that to core.h.
But do as you like, I don't care about this that much.
> > 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).
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.
> > 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? 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. 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.
> 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.
> > 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.
--
Tanu
https://www.patreon.com/tanuk
