[pulseaudio-discuss] Questions regarding messaging API

[Georg Chini]

On 28.01.19 18:37, Georg Chini wrote:
> On 28.01.19 18:05, Tanu Kaskinen wrote:
>> On Mon, 2019-01-28 at 17:50 +0100, Georg Chini wrote:
>>>>> 2) For read_string() and read_raw() it is difficult to retain a 
>>>>> default
>>>>> value.
>>>>> The string returned by the functions is a char, but it must not be 
>>>>> freed
>>>>> because it is part of the larger parameter string. So the string 
>>>>> passed
>>>>> as default would need to be a const char, which makes some type 
>>>>> casting
>>>>> necessary. Therefore I would like to keep it as is for the string 
>>>>> functions.
>>>> Is this about not changing the pass-by-reference arguments to 
>>>> functions
>>>> until success? If so, that seems like a separate issue from whether 
>>>> the
>>>> argument should be const or not. Just use an internal temporary
>>>> variable in the functions and only assign to the user variable at the
>>>> end of the function.
>>>>
>>>> Regarding constness, I think the function parameter can be kept non-
>>>> const even if it's not supposed to be freed (just make sure to 
>>>> document
>>>> that). It's ok to modify the string contents, nothing will break.
>>> As said on IRC I think you did not get the point. Consider the
>>> following code:
>>>
>>> char *result;
>>>
>>> /* To demonstrate the point, I fill result with a newly allocated 
>>> string */
>>> result = pa_xstrdup("A string");
>>>
>>> /* Now I call read_string() */
>>> err = pa_message_params_read_string(parameter_list, &result, &state);
>>>
>>> Now, if the default value is returned, the string in result must be 
>>> freed.
>>> If the value returned is from the parameter list, the string in 
>>> result must
>>> NOT be freed. You could work around it by using typecasts or by 
>>> deciding
>>> if the string needs to be freed based on the err value, but I think
>>> this is very ugly and there is nothing that prevents a user doing it 
>>> the
>>> wrong way. Therefore I would suggest not to use default values for the
>>> string functions. For read_raw() a default seems pointless anyway,
>>> so only the functions read_string() and read_string_array() would be
>>> affected. For the numeric/boolean functions there is no such issue.
>> It's an error to pass a string that needs freeing to
>> pa_message_params_read_string(), just like it's an error to pass a
>> string that needs freeing to pa_tagstruct_gets().
>>
>> Your original patch didn't free the parameter either, so I don't know
>> why you started to worry about this. Maybe you misunderstood this
>> sentence:
>>
>> "It should be mentioned that if allocate is true, the result should be
>> freed with pa_xfree()."
>>
>> Taken out of context (as was the case when I quoted it), it could be
>> understood so that pa_message_params_read_string() should free the
>> function parameter before assigning a new value to it, but I didn't
>> mean that. I meant that the function documentation should tell the
>> caller to use pa_xfree() (rather than plain free() or whatever else)
>> when freeing the returned string.
>>
> OK, I was worrying because unlike pa_tagstruct_gets() the
> function parameter is currently char ** and not const char **
> But after taking a second look, I see that I can make it const
> now, so problem solved. Are you OK with leaving read_raw()
> without default? Here the parameter must remain char**
> because the returned string may be a list that can be
> processed again by the parsing functions (and in my opinion
> a default in this case does not make sense anyway).
>
>
And one more point I stumbled over: When reading numeric
arrays, how should an empty element ({}) be treated? Should
it be an error or treated as 0? My current approach is to read
it as 0.