[pulseaudio-discuss] Questions regarding messaging API

[Georg Chini]

On 31.01.19 19:33, Tanu Kaskinen wrote:
> On Mon, 2019-01-28 at 18:43 +0100, Georg Chini wrote:
>> 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).
> By "leaving without default", do you mean that you want to assign to
> the result variable early, when the function might still fail? I don't
> see why you'd want to do that, except maybe for avoiding one local
> variable.
>
> It's just a general pattern to not touch return variables on failure.
> It probably won't cause any trouble in this case if you go against that
> pattern, because the caller is unlikely to assign a default value that
> would get used in case the function fails, but I really don't see any
> good reason to deliberately go against the generally-useful pattern.
> Whether the string is const or not doesn't matter here at all, as far
> as I can see.

I want to return NULL in case that the function fails.

First, the function read_raw() differs from all other read
functions in so far that the result of the function is expected
to be a string that must be further processed with the other
read functions. So a default does not make any sense and
returning NULL when nothing is read seems sensible.

Second I still think it matters that the function argument is
char and not const char. For the caller, it means that it has
to provide a char that needs not be freed if a default is
wanted. This is ugly and very likely to cause memory leaks.

I do not think we need to implement something just to
follow a pattern if it is no use at all and even likely to cause
errors, but if you insist, I will change it in the next version.


>
>> 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.
> I think it should be an error. When reading single numeric values, we
> distinguish between 0 and null, and I don't think we should change the
> rules when reading numeric values in arrays.
>
OK, will change it in the next version.


Meanwhile I created a merge request for the patch set. Please
take a look at it. Patches 2 - 5 are (apart from one single wording
change you requested) unchanged, so I guess they do not need a
thorough review again.