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

[Georg Chini]

On 25.01.2018 00:26, Tanu Kaskinen wrote:
> On Wed, 2018-01-24 at 22:01 +0100, Georg Chini wrote:
>> 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.
> By "add", do you mean adding after the initial implementation, or
> having the outer braces from the beginning? If the latter, then we are
> on the same page at least to some extent.

I mean the latter and will add the additional braces in the next
version.

>
> My original point was that I'd like *all* messages to support later
> extension.
>
>> 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.
> You lost me here. Why can't we do this? I don't understand what you're
> trying to say with "because we have no implicit outer list".
>
> Hmm... Maybe you're thinking that I want all message parameter lists to
> be enclosed in an extra pair of braces.

Yes, that's what I thought.

>>>>>>>> 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.
> I just don't see the big benefit. You have to do error checking for the
> individual values in any case. The benefit of having the list
> formatting check early is that reading the individual values can't fail
> due to list formatting errors, but since there are other error cases,
> the code complexity for reading a value is exactly the same as without
> any early list formatting checks (assuming that the application doesn't
> care about the exact failure reason).
>
Strange that you don't see the benefit. In my example, if the y1
y2 are very high but still acceptable volume values, not catching
the parse error in advance could even lead to damage of
hardware or ears.

But anyway, it does not impact implementation, therefore it is
no issue if we disagree on that point.