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

[Georg Chini]

On 24.01.2018 02:10, Tanu Kaskinen wrote:
> On Mon, 2018-01-22 at 21:43 +0100, Georg Chini wrote:
>> On 21.01.2018 01:03, Tanu Kaskinen wrote:
>>> On Fri, 2018-01-19 at 14:23 +0100, Georg Chini wrote:
>>>> 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.
> Are you claiming that if the "list message handlers" call returns an
> implicit list, i.e. no braces around the outermost list, then it's
> still possible to later add a new parameter after the list? I'm saying
> that's impossible, and that's the problem with the implicit list. How
> can the parsing code detect when the list ends and the new parameter
> begins, if originally "end of string" was the only indication that the
> list ended?

No, I am not saying that we could later add a parameter in that
case. I am just saying, that this is not a design issue, because any
message handler that expects later extension can add those outer
braces. I am also saying, that we always have some outer list and
that it does not matter, if the outer list is enclosed in "start of
string" and "end of string" or in brackets.
Take the list message handlers message. If we enforce outer braces,
the response looks like

{{{p1} {d1}} {{p2} {d2}}}

Now, if we want to add a new element after the list, we cannot do

{{{p1} {d1}} {{p2} {d2}}} {new}

because we have no implicit outer list. We have to write it this way:

{{{{p1} {d1}} {{p2} {d2}}} {new}}

This however will break old clients, because they don't expect the
extra braces. So the initial reply would need to look like

{{{{p1} {d1}} {{p2} {d2}}}}

so that we can do

{{{{p1} {d1}} {{p2} {d2}}} {new}}

And this is equivalent to what we have to do without the
outer brackets.

As already said, I don't mind adding the braces to the response of
the list-handlers message if you think it will ever be extended.

>
>>>>>> 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.
> How does validating the brace correctness prevent some values being
> used before all values have been parsed? You're only enforcing brace
> correctness, but if you have a list of doubles, any of the double
> values can be invalid. Nothing is preventing the application from using
> the first values of the list before noticing that a value later in the
> list is invalid.
>
> I didn't claim that my approach was any better in this regard, you were
> just making impossible-sounding claims about the benefits of your
> approach.

I can only repeat myself: My example above shows how this
helps detecting errors before any values are used at all. The
point is, that the consistency check is done before the values
are parsed. This means when pa_split_message_response()
returns a list, the whole list is already checked for consistency
(with the exception of the outer, implicit list). This does neither
prevent later lists from having issues nor does it help against
wrong list values, but at least you should never hit a parse
error within a list. I think it is a big benefit to know in advance
that a list is at least consistent.
It also means, that if we add braces around the whole
parameter string, the complete string will be checked for
consistency before any value is parsed.

>
>>>>> 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.
> I introduced the pa_message structure as a solution to the problem that
> we need something like pa_strbuf, but pa_strbuf isn't fully adequate as
> it is.
>
> Now it's starting to be clear that you want to use pa_strbuf plus some
> separate helper functions. That's fine. The small benefit of pa_message
> would be that the structure would contain both the necessary parts of
> pa_strbuf and the helper functions all in the same namespace.
>
> Another benefit would be that if pa_context_send_message() took a
> pa_message rather than a string for the parameters, then applications
> wouldn't need to first allocate a pa_strbuf, then convert that into a
> string and then free the string. Using pa_message would have one less
> step. The same benefit could be achieved if pa_context_send_message()
> took a pa_strbuf instead of a string for the parameters, but I'm not
> sure if we want to do that.
>
I can't see much difference if you pass a strbuf (or similar) explicitly
or if you hide it within a structure. Anyway, because you are OK
with the current plan, I'll leave it as is. See my reply  to your other
mail (tomorrow) for my thoughts about the helper functions.