[pulseaudio-discuss] Questions regarding messaging API

[Georg Chini]

On 28.01.19 16:48, Tanu Kaskinen wrote:
> On Sat, 2019-01-26 at 23:15 +0100, Georg Chini wrote:
>> Hi Tanu,
>>
>> as already said in a previous mail, I am working again on the messaging API.
>> I hope you remember enough of your review that you can answer a couple
>> of questions:
>>
>> 1) I seem to remember that you did not want a boolean allocate parameter
>> to the read_string() function, but looking through your comments, I cannot
>> find anything like that. Did you object or is the allocate parameter OK?
> I remember objecting to that, but I didn't find my comment either,
> until I ran into it accidentally while answering your other questions.
> Here's my comment:
>
> https://lists.freedesktop.org/archives/pulseaudio-discuss/2018-July/030266.html
>
>      It should be mentioned that if allocate is true, the result should
>      be freed with pa_xfree(). I'm not sure about the net benefit of the
>      allocate parameter, though. It might be better to let the
>      application take care of copying the string when needed. The
>      function becomes simpler, and there's no need to think about the
>      correct freeing function.

OK, I dropped the allocate parameter in the current version because
I remembered that you objected. Personally I still think it would be
useful, especially for reading a string array where it is probable that
you want to use the strings outside the message handler callback.

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

>
>> 3) Concerning the conversion of double to string and back, I still think
>> the way I am doing it is safe and simple. Using the g conversion specifier
>> should ensure, that the only locale dependent character is the decimal
>> separator and this is either dot or comma in any locale. There is a
>> pa_atod() function, but I think it is inconsistent. It uses C locale on
>> platforms that have strtod_l() and else uses the server locale. If I
>> should use this function, it would need to be corrected to always use
>> C locale and an additional function pa_dtoa() would need to be
>> implemented which does the reverse conversion in C locale.
>> However I can't really see the point of messing with the locale if a
>> simple substitution of the decimal separator is sufficient.
> I find it very ugly to have such format variability in the protocol. I
> have never seen a serialization spec that would allow two different
> decimal separators. But maybe it's not worth fighting this. Your
> approach is certainly much nicer in that it doesn't require dealing
> with a global variable in the server and in libpulse, as I suggested
> for caching the C locale.
>
> Could you make sure that pa_message_params_write_double() always uses a
> dot for the separator? Sometimes it may not be possible to use the
> helpers in libpulse to parse the message parameters, for example when
> using "pactl send-message" from a script (or from an application that
> is written in a language without libpulse bindings), and in those
> situations it's nice to have consistent input.
>
Sounds like a good idea, I'll do that.