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

[Tanu Kaskinen]

On Thu, 2018-01-11 at 19:06 +0100, Georg Chini wrote:
> On 11.01.2018 16:28, Tanu Kaskinen wrote:
> > On Sun, 2017-10-29 at 20:51 +0100, Georg Chini wrote:
> > > This patch adds a small message handler to the core which enables
> > > clients to list available handlers via the list-handlers message.
> > > Command: pacmd send-message /core list-handlers
> > > pactl can be used with the same parameters.
> > > 
> > > The patch also introduces a convention for the return string.
> > > It consists of a list of elements where curly braces are used
> > > to separate elements. Each element can itself contain further
> > > elements. For example consider a message that returns multiple
> > > elements which each contain an integer and an array of float.
> > > A response string would look like that:
> > > {{Integer} {{1st float} {2nd float} ...}}{...}
> > > ---
> > >   doc/messaging_api.txt           | 20 +++++++++++++------
> > >   src/pulsecore/core.c            | 43 +++++++++++++++++++++++++++++++++++++++++
> > >   src/pulsecore/message-handler.c | 24 +++++++++++++++++++++++
> > >   3 files changed, 81 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/doc/messaging_api.txt b/doc/messaging_api.txt
> > > index 11835cda..85a56d84 100644
> > > --- a/doc/messaging_api.txt
> > > +++ b/doc/messaging_api.txt
> > > @@ -7,10 +7,18 @@ and a message command, both specified as strings. Additional parameters can
> > >   be specified using a single string, but are not mandatory. The message handler
> > >   returns an error number as defined in def.h and may also return a string in
> > >   the "response" variable. If "response" is NULL, this should be treated like
> > > -an empty string. The following reference lists available messages, their
> > > -parameters and return values.
> > > +an empty string. It it is not NULL, it consists of a list of elements. Curly
> > > +braces are used to separate elements. Each element can itself contain further
> > > +elements. For example consider a message that returns multiple elements which
> > > +each contain an integer and an array of float. A response string would look
> > > +like that:
> > > +{{Integer} {{1st float} {2nd float} ...}}{...}
> > 
> > As discussed earlier, escaping curly braces in strings should be
> > defined and supported already in the first version.
> 
> I have a (still unsubmitted) patch which applies on top of the
> messaging/signal patches which introduces escaping. Is that
> OK for you? I would submit it immediately when the other
> patches are pushed. Or do you want it in this patch?

You can send it separately, although I think it would make sense to
have it in this patch.

> > 
> > The use of whitespace needs to be defined in detail too. What
> > characters are considered whitespace? Is the use of whitespace
> > mandatory, and are there rules about the amount of whitespace?
> 
> Basically everything which is not enclosed in {} will be ignored.
> I'll add that to the description.
> (This could - if necessary - in the future be used to add a name
> and a type to a field like {name.type{...} name.type{...} ...}
> without breaking older parsing that does not expect such
> information)

Ok, accepting any random stuff is unusual, but I can't think of any
significant issues with it. It probably makes parsing slightly easier
too.

> > 
> > > +/* Check if a string does not contain control characters. Currently these are
> > > + * only "{" and "}". */
> > > +static bool string_is_valid(const char *test_string) {
> > > +    uint32_t i;
> > > +
> > > +    for (i = 0; i < strlen(test_string); i++) {
> > 
> > This potentially calls strlen() once per character. Check
> > test_string[i] instead.
> > 
> > This function is probably not needed anyway once you add escaping
> > support, though.
> > 
> 
> It's still needed to check the handler name.

The handler name can be escaped too. But if you want to limit the
allowed character set for handler names, I'm fine with that. In that
case I would suggest changing the function name to
handler_name_is_valid, and rejecting any characters outside the
printable subset of ascii characters, and whitespace should be rejected
too.

What would you think about changing the terminology a bit? It annoys me
that sometimes the identifier is called a "handler name", sometimes a
"recipient name" and with signals probably something different. Could
we borrow the "object path" term from D-Bus and use that consistently?

-- 
Tanu

https://liberapay.com/tanuk
https://www.patreon.com/tanuk