[pulseaudio-discuss] Questions regarding messaging API

[Georg Chini]

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