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

[Georg Chini]

On 24.01.2018 01:40, Tanu Kaskinen wrote:
> On Mon, 2018-01-22 at 16:19 +0100, Georg Chini wrote:
>> On 21.01.2018 01:03, Tanu Kaskinen wrote:
>>> 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"?
> I mean the state that pa_strbuf keeps. Apparently you want to make
> pa_strbuf available in libpulse and add helper functions in a different
> namespace that nevertheless operates on pa_strbuf. You never explained
> that explicitly, so it was not clear to me that you wanted that.
>
> I'm ok with that plan. I would prefer pa_message_params as the helper
> function namespace rather than just pa_message.
What do you mean by "make strbuf available in libpulse"? Do I
need to move the strbuf code to libpulse? Currently the functions
are already used in libpulse by format.c, json.c and proplist.c.
I was thinking of hiding the use of strbuf behind some wrappers,
so that that you call for example pa_message_params_new()
instead of pa_strbuf_new() to avoid having a mixed name space.
Not sure if this is a good idea, because some of the functions
(like _new() or _write_string()) would really not be more than
simple wrappers.

Do you think we should have a new file (pulse/message-helper.c/.h)
for the helper functions or should they go into pulse/util.c?

Which functions do we need for a start?

Reading:

pa_message_params_split_list() - splits the list, see below
pa_message_params_read_string() - reads a string
pa_message_params_read_double() - reads a float value
pa_message_params_read_int() - reads a long integer
pa_message_params_read_unsigned() - reads an unsigned long
pa_message_param_read_bool() - reads a boolean

Anything else?

Writing:

pa_message_params_new() - returns a new strbuf
pa_message_params_to_string() - converts the strbuf to a string
pa_message_params_write_string() - writes a string
pa_message_params_write_escaped_string() - writes an escaped string
pa_message_params_write_double() - writes a float value
pa_message_params_write_int() - writes a long integer
pa_message_params_write_unsigned() - writes an unsigned long
pa_message_param_write_bool() - writes a boolean

Anything else? Should _write_escaped_string() and _write_string()
be two different functions or should we pass a boolean to
_write_string() to indicate if we want escaping or not?

Concerning the problem of writing/reading floats: Do you think
it is OK if we search for "." or "," in the string and replace the
character with the decimal point character of the current locale
before conversion? This would work as long as the float is written
without thousand separator and we can ensure that in the
_write_float() function.


Any further ideas/suggestions/restrictions that need to be taken
into account?

>
>>> 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.
> So you're proposing that reading a double should take two function
> calls. I think the two steps should be merged into one
> pa_message_params_read_double() function. Then error handling needs to
> be done only once, rather than twice per value. With this approach,
> however, the question arises how to deal with nulls.
>
You are right, it should be folded into one function. I guess the
easiest way to handle NULL's is to have a special return value,
let's say 1 if parsing was OK, 0 if NULL was found and -1 on
parse error, similar to the split function.

Actually I noticed, that the code above is not correct. After
pa_split_message_parameter_string() three checks need to
be performed:

1) ret = 0: This does NOT mean that the element is empty,
it means that the element is completely missing (end of list).
It can still be treated like an empty element.
2) ret= -1: Parse error
3) length = 0: This means the element is empty.

I'll rename the split function to pa_message_params_split_list()
to match the name space. Or do you have a better idea?