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.
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
