[pulseaudio-discuss] [PATCH 5/5] pactl: Implement list message-handlers

[Georg Chini]

On 21.01.2018 01:03, Tanu Kaskinen wrote:
> On Fri, 2018-01-19 at 14:23 +0100, Georg Chini wrote:
>> On 18.01.2018 23:20, Tanu Kaskinen wrote:
>>> On Wed, 2018-01-17 at 16:12 +0100, Georg Chini wrote:
>>>> On 14.01.2018 21:47, Tanu Kaskinen wrote:ry from
>>>>
>>>>
>>>>
>>>> Here's an updated example that should work with the current list-
>>>> handlers (with an "implicit" top-level list):
>>>>
>>>> while (pa_message_read_list_start(&state) > 0) {
>>>>       read path
>>>>       read description
>>>>       pa_message_skip_rest_of_list(&state) /* Consumes the list closing brace and everything preceding it. */
>>>> }
>>>>
>>>> if (!pa_message_eof(&state)) {
>>>>       /* This can happen if an unexpected closing brace is encountered. */
>>>>       fail("Malformed data");
>>>> }
>>>>
>>>> As you can see, the updated example is exactly the same length as your
>>>> proposal, except for the pa_message_eof() check that you didn't have,
>>>> but you should have a similar check, because otherwise you don't know
>>>> if the parsing loop ended because of end-of-string or malformed input.
> I didn't think this through properly earlier, but the functions should
> return different values for end-of-string and malformed input. The
> pa_message_eof() call should then be replaced with checking the
> pa_message_read_list_start() return value (not that it makes much
> difference in practice, but checking for errors indirectly via eof is
> less clear than checking for errors directly).
>
>> Also you need to handle the case where the end of a list does not
>> match the end of the string because there are further elements
>> behind it.
> Why? There can't be any further elements behind the implicit list. As I
> said earlier, the drawback with the implicit list is that the message
> parameters aren't extensible. If the list is explicit, then it's
> possible to detect when the list ends and other parameters begin.

This is what I mean. Only the outer list is implicit. So it is not
enough to look for end of string but you need also test if you
hit an "end of list". If you have nested lists, it may be possible
that there are elements behind the end of the list you are
currently working on. Concerning the "implicit outer list":
Adding additional braces around the whole parameter list is
up to the message handler. If you prefer, I can do this for the
core message handler, though I believe it is not necessary in
that case. The implicit list is not a design problem, it does not
change anything if you think it through. It's just that "start of
string" and "end of string" are used instead of braces.

>>>
>>>> 2) You are forced to process the elements in the order they are delivered,
>>>> taking care of all the nesting. This has two disadvantages:
>>>>
>>>> a) Because you do not know if the nesting brackets are closed correctly, you
>>>> may continue processing elements after a missing bracket. Consider the
>>>> following example: {{x1} {x2} {x3}} {y1} {y2}. If you forget to close the
>>>> list like {{x1} {x2} {x3} {y1} {y2}, your approach would continue processing
>>>> y1 and y2 as part of the list before the error is encountered.
>>>>
>>>> split_message_response() however would complain before any of the elements
>>>> is processed because it searches for the closing bracket and thereby does a
>>>> consistency check of each element that is returned.
>>> You're right, but I don't see why this is such a bad thing. The error
>>> will be caught eventually.
>> If you only parse the data it is not an issue but if you are already
>> using the
>> values in the parsing loop this might cause problems. Let me make a (not
>> very realistic) example: We have a 5.1 output and the x1 to x3 are used
>> to change the volume of the first three channels. Now you would also
>> change the volume of the 4th and 5th channel to some random value
>> before you hit an error.
> Your approach doesn't actually help here. pa_split_message_response()
> can't do much more than check that the braces match. It doesn't know
> the types of the contained values, so the values can still be invalid
> even if pa_split_message_response() thinks everything is fine.

Well, the values themselves cannot be checked, but that can't
be done for both solutions. But my way avoids that values are
used for something unintended if a parse error occurs. The
point in the case above is that my function will find the parse
error before the values are used, while your solution finds
the parse error only after the values have been used.

>
>>>
> It would certainly be useful to be able to skip uninteresting values.
> There should be a function for this.

You can simply use the parsing function to skip a value.

>
>
> The idea behind read_path() is that if object paths have constraints on
> what is a valid value, then read_path() can do validation that plain
> read_string() can't.
>> That's exactly what I want to avoid and where I see the risk of
>> confusion. I do not want to have multiple functions with different
>> error checking for the same data type. I think the error checking
>> of the values does not belong into the parsing functions.
> I find this point of view strange. The more the parsing functions do
> error checking, the less the application needs to do error checking.
> And in any case, the application is free to parse object paths as
> strings if it doesn't care about strict path formatting.

I am not saying there should be no error checking.
I think however, that error checks in the conversion functions
should be restricted to ensuring that the data is converted
correctly. All "high level" error checks should be done by the
caller. On the other hand I would not reject a function that
does some special error checks if this is needed in multiple
places.

>
>
> There's another topic we should discuss: how should the writing API
> look like? This can have some effect on the parsing API as well. When
> writing data to the parameter string, the whole string can't be easily
> allocated all in one go, so we should have something like pa_strbuf
> available for clients. I don't think we should add pa_strbuf at least
> directly to the client API, because the message parameter writing has
> some specific conventions (i.e. wrapping everything in curly braces),
> and while it would be possible to add the specialized functions to the
> pa_strbuf API, I think that would be somewhat ugly. So I propose we
> introduce a new API similar to pa_strbuf.
>> I took the simple approach and added a function prepare_message_string()
>> which returns an escaped string. So whenever there is a risk that the
>> string contains any characters that need to be escaped, one can use
>> prepare_message_string(string) instead of the string itself. I think it does
>> not make sense to pass every value through the escaping, we only need
>> it for strings and only for those strings that have a risk of containing
>> unwanted characters. My pa_split_message_response() function then
>> removes the escaping when it reaches the innermost braces.
>>
>> Adding the braces need not be done in a special function. On the contrary,
>> the caller should take care and keep track of the braces. It must do so
>> anyway if it is passing lists, so the additional overhead of adding one more
>> pair of brackets should be acceptable.
> I think there should be convenience functions for writing strings,
> floats and ints and whatever other data types we support. Manually
> adding braces around the written values takes extra effort that can
> easily be avoided (and writing floats correctly is not trivial anyway).
>
> If you're not against such convenience functions, what namespace do you
> suggest, if pa_message_write_string() etc. isn't good?

I'm OK with the functions and the name space. As said in my
previous mail I don't mind if those functions add the braces.
In my patch I called the string writing function differently
and it does not add braces, but both can easily be amended.

>
>>> My suggestion would be to introduce a new pa_message struct that has
>>> both reading and writing functions (e.g. pa_message_read_string(),
>>> pa_message_write_string()). It would be analogous to DBusMessage, so it
>>> would do a bit more than just string manipulation (it would at least
>>> carry the recipient/sender and message name information). I'm not sure
>>> you like that, because I've got the impression that you want to call
>>> only the "method calls" messages, and in that case replies and signals
>>> can not be called messages. Nevertheless, my preference is to call all
>>> of those three things messages, and in that case it makes sense to have
>>> pa_message that represents all three things, so that's what I'm going
>>> to propose here.
>> I agree that all three can be called messages so I am fine with
>> your terminology, but I do not see the need for the structure you
>> propose. I have been trying to keep the interface as simple and
>> transparent as possible and I don't think adding layers of abstraction
>> helps achieving that goal.
> Well, I think we need a bunch of functions for reading and writing
> message parameters, and the functions (especially the writing
> functions) are going to need some structure to keep state. pa_message
> is my suggestion, but I'm open to other ideas.

I don't understand what state should be kept. Keeping track
of the braces from within some writing function does not seem
to make much sense to me. How would you enforce, that opened
braces will be closed again? For me, a set of functions
pa_message_write_*() and pa_message_read_*() for the different
data types appears fully sufficient. The read functions operate on
strings while the write functions take a strbuf.
The calling function has to ensure that nesting is correct when a
parameter string is written. Adding functions like
pa_message_write_{start|end}_list() seems pointless, they would
only write a single brace.

>
>>> pa_context_send_message() would take a pa_message as an argument. The
>>> reply callback would get a pa_message as an argument. Signal callbacks
>>> would get a pa_message as an argument. So things would work very
>>> similarly to how D-Bus works. One major difference to D-Bus would be
>>> that errors wouldn't be messages, at least for now (earlier there was
>>> some talk about making errors more informative, and at that point using
>>> pa_message for representing errors as well might make sense).
>> Did we not agree that the client should only receive a string as a reply?
>> If we can pass structures, we don't need all the overhead of transforming
>> the variables into strings.
> Even if we use a pa_message struct, it's still good to have the
> protocol string-based. For example, that makes "pactl send-message"
> possible (or at least much easier to use than with a binary protocol).

I tried to understand but I don't get what the advantage
of such a structure is.

>
>> Concerning messages and signals, they already receive the same set
>> of parameters, so again no need for some structure.
>>
>>
>> Let me add one more complication: What about passing floats
>> when server and client are using different locales? We probably
>> have to provide some float_to_string() function to produce results
>> independent on  the locale. I have been thinking about the "," or "."
>> problem but could not come up with a good solution so far.
>> Also which precision should be used? Full possible number of digits?
> We should have a function for writing a float to the message
> parameters. I don't know how exactly it should do the conversion (I
> know it should use "." as the decimal point), probably we can borrow
> from some existing standard... Whatever it does, it must never produce
> a string that our float parsing function fails to parse.
>
Yes, that was the point I wanted to make. But since the current
patch set only deals with strings, we have some time to think
about a good solution.