[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
>>>>> the heap are quite heavy operations, and your approach will do a lot of
>>>>> those operations. Even reading a simple integer will, I suppose,
>>>>> involve first allocating a new string. I can't say for sure that the
>>>>> performance penalty makes any practical difference, but my intuition
>>>>> says that we should use the approach that I proposed that requires far
>>>>> fewer heap memory allocations.
>>>> I can accept the argument that my solution unnecessarily allocates
>>>> strings in some situations. Though I do not think it has much practical
>>>> impact, most of it can be avoided by using special functions to retrieve
>>>> numbers or booleans. I am also quite sure that a function similar to the
>>>> *_split_in_place() functions can be implemented to completely avoid
>>>> the allocation of unnecessary strings.
>>> An in-place split function seems feasible indeed.
>>>

> Since your splitting function can be modified to avoid mallocs, I'm not
> so strongly against it any more.

Sounds good, I sent a new patch set.

> It looks like we're anyway going to
> need a bunch of "helper" functions, so there's not that much difference
> between our approaches, mainly list parsing details are affected
> somewhat. How to organize the helper functions is an open question,
> since you have so far opposed my pa_message idea, and haven't provided
> a proper alternative for keeping parameter writing state.

Can you perhaps explain your idea again? Maybe I just opposed
because I did not understand what you proposed. I agree that we
need helper functions to read and write values and if you like to
include the braces around the parameter in the helper functions,
I can live with it as well. What do you mean by "keeping parameter
writing state"?

>
> Here's one more topic for discussion: could we please support null
> values? One thing that has caused some trouble with D-Bus is that it
> doesn't properly support null values. Generally nulls could be encoded
> with "{}", but that doesn't allow distinguishing between an empty
> string or list and null. Maybe that distinction is not needed?

For numbers and booleans an empty element can be treated
as NULL value, for a string I guess it does not matter that much
if it is empty or NULL.

My parsing function supports empty elements and returns an
empty string. A special case is an empty list, but in this case
the function would return "end of string" when you try to
access the first element in the list.

>
> How should reading and writing nulls work? Writing nulls is not
> complicated, we could just have pa_message_write_null() (or since we
> probably will have a "raw string append" function, it's trivial to just
> append "{}" manually). Reading nulls is less obvious.
>
> All reading functions could have an "is_null" parameter:
>
>      double double_var;
>      int is_null;
>      ret = pa_message_read_double(msg, &double_var, &is_null);
>
> If the is_null parameter isn't provided (i.e. is itself NULL), the
> function could fail if it encounters null.

If we define that NULL is represented by the "special element"
{} it is very easy to determine if an element is empty, because
in this case, the length returned by the parsing function will be 0.
So I don't think we need to include an allow_null parameter in
the helper function (unless we treat a sequence of white spaces
like an empty element), because in cases where it is important,
we can check the length of the element before calling the helper.
Currently, parsing a value (including full error check) would look
something like

ret = pa_split_message_parameter_string(list, max_length, &start_pos, 
&length, NULL, &state);
if (ret == 0)
     handle empty element;
if (ret == -1)
     handle parse error;

/* start_pos contains now the first valid character of the element
* after { and length the length of the element without closing
* bracket */

ret = pa_message_read_double(start_pos, length, &double_var)


Strings would not need a (read) helper function because they can
be directly returned by the parsing function.

>
> Alternatively, if it's too ugly to have the is_null parameter in all
> reading functions, there could be separate "_or_null()" functions
> alongside the regular functions, e.g. pa_message_read_double_or_null().
>
> And still one more topic: we might want to support arbitrary binary
> data at some point. This could be supported by escaping non-ascii bytes
> with \xnn (with n being a hexadecimal number). This escaping support
> should exist from the beginning. Do we perhaps want to support \n, \r
> and \t as well?
>
I do not think this must be supported from the start. "Binary data"
would be a different data type than "string" and have special
helper functions that can handle escaping.

This is different from escaping the control characters.