[pulseaudio-discuss] [PATCH 6/8] message-params: Allow parameter strings to contain escaped curly braces

[Georg Chini]

On 27.07.2018 10:08, Tanu Kaskinen wrote:
> On Thu, 2018-07-26 at 18:02 +0200, Georg Chini wrote:
>> On 26.07.2018 12:37, Tanu Kaskinen wrote:
>>> On Sun, 2018-07-22 at 21:11 +0200, Georg Chini wrote:
>>>> On 22.07.2018 17:48, Tanu Kaskinen wrote:
>>>>> On Sun, 2018-07-22 at 16:02 +0200, Georg Chini wrote:
>>>>>> On 21.07.2018 20:17, Tanu Kaskinen wrote:
>>>>>>> On Mon, 2018-04-09 at 19:35 +0200, Georg Chini wrote:
>>>>>>>> The patch adds the possibility to escape curly braces within parameter strings
>>>>>>>> and introduces several new functions that can be used for writing parameters.
>>>>>>>>
>>>>>>>> For writing, the structure pa_message_param, which is a wrapper for pa_strbuf
>>>>>>>> has been created. Following new write functions are available:
>>>>>>>>
>>>>>>>> pa_message_param_new() - creates a new pa_message_param structure
>>>>>>>> pa_message_param_to_string() - converts a pa_message_param to string and frees
>>>>>>>> the structure
>>>>>>>> The function pa_message_param_write_string()
>>>>>>>> has a parameter do_escape.
>>>>>>> Why not do escaping always?
>>>>>> Because what you are writing as a string may be a list that you have
>>>>>> prepared
>>>>>> previously. Then you will not want escaping. You may for example create
>>>>>> a list
>>>>>> from an array and then insert this list as one string into the final
>>>>>> parameter list
>>>>>> or have a function that converts a certain structure to a parameter
>>>>>> string and
>>>>>> then write the result of this function as one element of the final list.
>>>>> My mental model is that parameters have types, list type is different
>>>>> than string type, and write_string() is only meant for writing values
>>>>> of the string type.
>>>>>
>>>>> Can you add a write_raw() function?
>>>> Yes, this is done in patch 7. But the raw write function differs from what
>>>> write_string() is doing. write_string() writes one element of a list,
>>>> that is
>>>> it encloses the string in braces. The raw write function is intended for
>>>> situations where you can't write a complete element with one write, so
>>>> it does not add any braces. I am still of the opinion, that a structure
>>>> or array converted to a parameter string is a string, so writing something
>>>> like this should be done with write_string().
>>> They are different kinds of strings, different abstraction levels. When
>>> you're writing an array "as a string", in that context it's just a C
>>> string. write_string() with escaping deals with strings in the "message
>>> params type system". I don't know if this makes any sense to you.
>>> Probably not... In any case, the do_escape flag seems unnecessary
>>> complexity to me.
>> The alternative would be a function to write an unescaped string in
>> addition to the write_raw() function. If you don't like the flag, would
>> you be OK with a write_unescaped_string() function? I think it is just
>> more comfortable than using write_raw().
> Thanks for the concession, I was afraid we'll get stuck on this point.
>
> By comfortable, do you refer to that write_raw() doesn't add braces
> around the value? How about adding an add_braces flag to write_raw()?

Yes, that's a good idea. I'll do that and remove the
do_escape flag from write_string().

>
>>>>>>>> +
>>>>>>>> +/* Read functions */
>>>>>>>> +
>>>>>>>>      /* Split the specified string into elements. An element is defined as
>>>>>>>>       * a sub-string between curly braces. The function is needed to parse
>>>>>>>>       * the parameters of messages. Each time it is called returns the position
>>>>>>>>       * of the current element in result and the state pointer is advanced to
>>>>>>>> - * the next list element.
>>>>>>>> + * the next list element. On return, the parameter *is_unpacked indicates
>>>>>>>> + * if the string is plain text or contains a sub-list. is_unpacked may
>>>>>>>> + * be NULL.
>>>>>>> is_unpacked looks like unnecessary complexity.
>>>>>>> pa_message_params_read_string() should always unescape the value.
>>>>>> It may be possible, that the string you read is a list. Consider the
>>>>>> following
>>>>>> parameter list: {string1}{some nested structure}{string2}. You can now
>>>>>> read this list as three strings and then continue to read the elements of
>>>>>> the nested structure from the second string. You might even create a
>>>>>> function
>>>>>> that takes a string and outputs a structure. So you are not forced to go
>>>>>> to the full depth of nesting on the first pass. This makes it much easier
>>>>>> to handle deeply nested parameter lists. For me this behavior is an
>>>>>> important
>>>>>> feature and I do not want to drop it. See also my comment on why I do
>>>>>> not always want escaping.
>>>>> Doesn't split_list() already allow this, why do you want to use
>>>>> read_string() to do the same thing as split_list()?
>>>> read_string() and split_list() are very similar and we could live
>>>> without read_string(). It is provided as a counterpart to write_string()
>>>> and for convenience additionally does the unescaping if necessary
>>>> like write_string does the escaping.
>>>> I don't see why this is a problem. It depends on the context which
>>>> is the better function to use.
>>> Again, in my mind a structure is not a string, they are different
>>> types, and I think read_string() should only deal with the string type.
>>> is_unpacked makes the API more complicated, so I'd like to get rid of
>>> it.
>>>
>> I don't see your point. is_unpacked is not part of the read_string()
>> arguments
>> or return value. In split_list() it is definitively needed to indicate
>> if the returned
>> string (in the C sense) contains another list. I can imagine many
>> situations where
>> you might either pass an array or just a single value or even NULL.
>> is_unpacked
>> allows to differentiate between the situations.
> Can you give an example? You say is_unpacked is definitely needed, but
> so far the only use case has been read_string(), which you wanted to be
> used for reading not only string values but everything else too. If
> read_string() is changed to only read string values, then it doesn't
> need is_unpacked from split_list().
>
> The parameter types are known beforehand, so i don't see the need for
> looking at the data to figure out the type. If introspection support is
> desired, then that's a separate project (the is_unpacked flag isn't
> sufficient for proper introspection).
>
This is not about parameter type, it is just to distinguish between
a list and a simple value. One example comes to my mind immediately:
Consider a parameter list that consists of strings and a couple of
arrays. Now you can read this list as an array of strings (patch 8
provides a function for that) and then examine those strings that
contain arrays separately. Having the flag (and using it in read_string())
provides a more flexible and convenient way to parse a parameter list.

The flag may not be strictly necessary at the moment, but I would still
like to keep it.