[pulseaudio-discuss] Questions regarding messaging API

[Tanu Kaskinen]

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.

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

-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk