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

[Georg Chini]

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().

>
>> Also writing unescaped strings in situations where escaping is not necessary
>> saves the overhead of looping over all the characters.
>>
>>>>> core-util already contains pa_unescape() that does the same thing more
>>>>> efficiently (if you drop the single quote thing).
>>>> pa_unescape() currently does not do the same thing. It removes all
>>>> escape characters, while I only want to remove the characters
>>>> I actually introduced (those before { or }).
>>>> I can however modify pa_unescape() to take the same arguments
>>>> as pa_escape().
>>> I don't see the need for being selective when unescaping. Nothing
>>> breaks if all (except escaped) backslashes are stripped.
>> You are right, if previously all backslashes in the original string
>> have been escaped, nothing will break. I was still thinking of the
>> old solution where I did not escape backslashes.
>>
>>
>>>>>> +
>>>>>> +/* 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. And I don't think that a 
boolean
return value is adding significantly to the complexity of a function, 
especially
since passing NULL is allowed if you do not care for the value.