On Mon, 2018-04-09 at 19:35 +0200, Georg Chini wrote:
> See doc/messaging_api.txt for the added functions. All read functions
> return 1 on success, 0 if end of string is found and -1 on parse error.
> Additionally, for the numeric/boolean read functions, 2 is returned if
> the list element is empty.
> ---
> doc/messaging_api.txt | 15 +++-
> src/map-file | 9 +++
> src/pulse/message-params.c | 185 +++++++++++++++++++++++++++++++++++++++++++--
> src/pulse/message-params.h | 35 +++++++++
> 4 files changed, 236 insertions(+), 8 deletions(-)
>
> diff --git a/doc/messaging_api.txt b/doc/messaging_api.txt
> index 0e6be53f..d080b783 100644
> --- a/doc/messaging_api.txt
> +++ b/doc/messaging_api.txt
> @@ -27,6 +27,11 @@ the structure
> pa_message_param_begin_list() - starts a list
> pa_message_param_end_list() - ends a list
> pa_message_param_write_string() - writes a string to a pa_message_param structure
> +pa_message_param_write_double() - writes a double to a pa_message_param structure
> +pa_message_param_write_int64() - writes an integer to a pa_message_param structure
> +pa_message_param_write_uint64() - writes an unsigned to a pa_message_param structure
> +pa_message_param_write_bool() - writes a boolean to a pa_message_param structure
> +pa_message_param_write_raw() - writes raw a string to a pa_message_param structure
>
> For string parameters that contain curly braces, those braces must be escaped
> by adding a "\" before them. This however means that a trailing backslash would
> @@ -44,9 +49,15 @@ Other strings can be passed without modification.
> For reading, the following functions are available:
> pa_message_param_split_list() - parse message parameter string
> pa_message_param_read_string() - read a string from a parameter list
> +pa_message_param_read_double() - read a double from a parameter list
> +pa_message_param_read_int64() - read an integer from a parameter list
> +pa_message_param_read_uint64() - read an unsigned from a parameter list
> +pa_message_param_read_bool() - read a boolean from a parameter list
>
> -pa_message_param_read_string() also reverts the changes that
> -pa_message_param_write_string() might have introduced.
> +All read functions return 1 on success, 0 if end of string is found and -1 on
> +parse error. Additionally, for the numeric/boolean read functions, 2 is returned
> +if the list element is empty. Also pa_message_param_read_string() reverts the
> +changes that pa_message_param_write_string() might have introduced.
>
> Reference:
>
> diff --git a/src/map-file b/src/map-file
> index 372d190d..ab8c21c6 100644
> --- a/src/map-file
> +++ b/src/map-file
> @@ -228,10 +228,19 @@ pa_mainloop_wakeup;
> pa_message_param_begin_list;
> pa_message_param_end_list;
> pa_message_param_new;
> +pa_message_param_read_bool;
> +pa_message_param_read_double;
> +pa_message_param_read_int64;
> pa_message_param_read_string;
> +pa_message_param_read_uint64;
> pa_message_param_split_list;
> pa_message_param_to_string;
> +pa_message_param_write_bool;
> +pa_message_param_write_double;
> +pa_message_param_write_int64;
> +pa_message_param_write_raw;
> pa_message_param_write_string;
> +pa_message_param_write_uint64;
> pa_msleep;
> pa_operation_cancel;
> pa_operation_get_state;
> diff --git a/src/pulse/message-params.c b/src/pulse/message-params.c
> index d68ec59d..93972399 100644
> --- a/src/pulse/message-params.c
> +++ b/src/pulse/message-params.c
> @@ -23,6 +23,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <locale.h>
> #include <sys/types.h>
>
> #include <pulse/xmalloc.h>
> @@ -84,7 +85,7 @@ int pa_message_param_split_list(char *c, char **result, bool *is_unpacked, void
>
> /* Empty or no string */
> if (!current || *current == 0)
> - return 0;
> + return PA_PARAM_LIST_END;
>
> /* Find opening brace */
> while (*current != 0) {
> @@ -101,7 +102,7 @@ int pa_message_param_split_list(char *c, char **result, bool *is_unpacked, void
>
> /* unexpected closing brace, parse error */
> if (*current == '}' && !found_backslash)
> - return -1;
> + return PA_PARAM_PARSE_ERROR;
>
> found_backslash = false;
> current++;
> @@ -109,7 +110,7 @@ int pa_message_param_split_list(char *c, char **result, bool *is_unpacked, void
>
> /* No opening brace found, end of string */
> if (*current == 0)
> - return 0;
> + return PA_PARAM_LIST_END;
>
> if (is_unpacked)
> *is_unpacked = true;
> @@ -140,7 +141,7 @@ int pa_message_param_split_list(char *c, char **result, bool *is_unpacked, void
> /* Parse error, closing brace missing */
> if (open_braces != 0) {
> *result = NULL;
> - return -1;
> + return PA_PARAM_PARSE_ERROR;
> }
>
> /* Replace } with 0 */
> @@ -148,7 +149,7 @@ int pa_message_param_split_list(char *c, char **result, bool *is_unpacked, void
>
> *state = current + 1;
>
> - return 1;
> + return PA_PARAM_OK;
> }
>
> /* Read a string from the parameter list. The state pointer is
> @@ -165,7 +166,7 @@ int pa_message_param_read_string(char *c, char **result, bool allocate, void **s
>
> *result = NULL;
>
> - if ((err = pa_message_param_split_list(c, &start_pos, &is_unpacked, state)) != 1)
> + if ((err = pa_message_param_split_list(c, &start_pos, &is_unpacked, state)) != PA_PARAM_OK)
> return err;
>
> *result = start_pos;
> @@ -179,6 +180,126 @@ int pa_message_param_read_string(char *c, char **result, bool allocate, void **s
> return err;
> }
>
> +/* Read a double from the parameter list. The state pointer is
> + * advanced to the next element of the list. */
> +int pa_message_param_read_double(char *c, double *result, void **state) {
> + char *start_pos, *end_pos, *s;
> + int err;
> + struct lconv *locale;
> +
> + pa_assert(result);
> +
> + *result = 0;
We should modify the result only after the parsing has been successful.
If you wonder why - it's a general pattern that may not be so important
here, but in other cases the caller may have set a default value in the
variable that shouldn't be overwritten. It's not impossible to imagine
that such default value would be used in case the message parameter is
null.
> +
> + if ((err = pa_message_param_split_list(c, &start_pos, NULL, state)) != PA_PARAM_OK)
> + return err;
> +
> + /* Empty element */
> + if (strlen(start_pos) == 0)
!*start_pos would be more efficient.
> + return PA_PARAM_IS_NULL;
> +
> + locale = localeconv();
> +
> + /* Replace decimal point with the correct character for the
> + * current locale. This assumes that no thousand separator
> + * is used. */
> + for (s = start_pos; *s; s++) {
> + if (*s == '.' || *s == ',')
> + *s = *locale->decimal_point;
> + }
> +
> + /* Convert to double */
> + errno = 0;
> + *result = strtod(start_pos, &end_pos);
> +
> + /* Conversion error or string contains invalid characters. If the
> + * whole string was used for conversion, end_pos should point to
> + * the end of the string. */
> + if (errno != 0 || *end_pos != 0)
> + return PA_PARAM_PARSE_ERROR;
> +
> + return PA_PARAM_OK;
> +}
> +
> +/* Read an integer from the parameter list. The state pointer is
> + * advanced to the next element of the list. */
> +int pa_message_param_read_int64(char *c, int64_t *result, void **state) {
> + char *start_pos, *end_pos;
> + int err;
> +
> + pa_assert(result);
> +
> + *result = 0;
> +
> + if ((err = pa_message_param_split_list(c, &start_pos, NULL, state)) != PA_PARAM_OK)
> + return err;
> +
> + /* Empty element */
> + if (strlen(start_pos) == 0)
> + return PA_PARAM_IS_NULL;
> +
> + /* Convert to int64 */
> + errno = 0;
> + *result = strtol(start_pos, &end_pos, 0);
We should add a helper function in core-util.c for parsing 64-bit
numbers like we have for 32-bit numbers.
> +
> + /* Conversion error or string contains invalid characters. If the
> + * whole string was used for conversion, end_pos should point to
> + * the end of the string. */
> + if (errno != 0 || *end_pos != 0)
> + return PA_PARAM_PARSE_ERROR;
> +
> + return PA_PARAM_OK;
> +}
> +
> +/* Read an unsigned integer from the parameter list. The state pointer is
> + * advanced to the next element of the list. */
> +int pa_message_param_read_uint64(char *c, uint64_t *result, void **state) {
> + char *start_pos, *end_pos;
> + int err;
> +
> + pa_assert(result);
> +
> + *result = 0;
> +
> + if ((err = pa_message_param_split_list(c, &start_pos, NULL, state)) != PA_PARAM_OK)
> + return err;
> +
> + /* Empty element */
> + if (strlen(start_pos) == 0)
> + return PA_PARAM_IS_NULL;
> +
> + /* Convert to uint64 */
> + errno = 0;
> + *result = strtoul(start_pos, &end_pos, 0);
> +
> + /* Conversion error or string contains invalid characters. If the
> + * whole string was used for conversion, end_pos should point to
> + * the end of the string. */
> + if (errno != 0 || *end_pos != 0)
> + return PA_PARAM_PARSE_ERROR;
> +
> + return PA_PARAM_OK;
> +}
> +
> +/* Read a boolean from the parameter list. The state pointer is
> + * advanced to the next element of the list. */
> +int pa_message_param_read_bool(char *c, bool *result, void **state) {
> + int err;
> + uint64_t value;
> +
> + pa_assert(result);
> +
> + *result = false;
> +
> + if ((err = pa_message_param_read_uint64(c, &value, state)) != PA_PARAM_OK)
> + return err;
> +
> + if (value)
> + *result = true;
> +
> + return PA_PARAM_OK;
> +}
> +
> /* Write functions. The functions are wrapper functions around pa_strbuf,
> * so that the client does not need to use pa_strbuf directly. */
>
> @@ -277,3 +398,55 @@ void pa_message_param_write_string(pa_message_param *param, const char *value, b
> pa_strbuf_puts(param->buffer, output);
> pa_xfree(output);
> }
> +
> +/* Writes a raw string to the pa_message_param structure. This is needed
> + * if a string cannot be written in one step. */
> +void pa_message_param_write_raw(pa_message_param *param, const char *value) {
> + pa_assert(param);
> +
> + /* Null value is not written */
> + if (!value)
> + return;
An assertion would make more sense to me.
> +
> + pa_strbuf_puts(param->buffer, value);
> +}
> +
> +/* Writes a double to a message_param structure, adding curly braces.
> + * precision gives the number of significant digits, not digits after
> + * the decimal point. */
> +void pa_message_param_write_double(pa_message_param *param, double value, int precision) {
> +
> + pa_assert(param);
> +
> + /* We do not care about locale because we do not know which locale is
> + * used on the server side. The decimal point character is converted
> + * to the server locale by the read function. */
> + pa_strbuf_printf(param->buffer, "{%.*g}", precision, value);
(I think we've discussed the floating point formatting before, but I
don't remember what the conclusion was. I find it unlikely that I would
have consented to using any random locale when converting double to
string.)
I think we should ensure that the value is converted to string using
the C locale. That would be much cleaner: no need to document in the
protocol spec that the doubles are allowed to be written in any locale
and hope that our parsing code catches all weird locale cases. Then
read_double() could just use pa_atod() (although that also needs to be
modified to ensure that it always uses the C locale - currently it
depends on whether strtod_l is available).
I suggest you modify pa_strbuf_printf() so that it sets LC_NUMERIC to C
before calling vsnprintf(). Something like this:
tmp_locale = duplocale(LC_GLOBAL_LOCALE);
pa_assert(tmp_locale != (locale_t) 0);
tmp_locale = newlocale(LC_NUMERIC_MASK, "C", tmp_locale);
old_locale = uselocale(tmp_locale);
r = vsnprintf(CHUNK_TO_TEXT(c), size, format, ap);
uselocale(old_locale);
freelocale(tmp_locale);
To avoid allocating a new locale object every time, we could add a
pa_get_c_numeric_locale() function that returns a static locale
variable. That could be used by pa_atod() too (and maybe there are also
other places that are allocating C locales that would benefit from that
function).
> +}
> +
> +/* Writes an integer to a message_param structure, adding curly braces. */
> +void pa_message_param_write_int64(pa_message_param *param, int64_t value) {
> +
> + pa_assert(param);
> +
> + pa_strbuf_printf(param->buffer, "{%li}", value);
long int is 32 bits on 32-bit systems. Use the PRId64 macro, or if that
looks too ugly, I think %lli will work reliably too. There's no
consistent convention about this in PulseAudio. I think using the PRI
macros would be more correct, but it just looks horrible.
> +}
> +
> +/* Writes an unsigned integer to a message_param structure, adding curly braces. */
> +void pa_message_param_write_uint64(pa_message_param *param, uint64_t value) {
> +
> + pa_assert(param);
> +
> + pa_strbuf_printf(param->buffer, "{%lu}", value);
PRIu64 or %llu.
> +}
> +
> +/* Writes a boolean to a message_param structure, adding curly braces. */
> +void pa_message_param_write_bool(pa_message_param *param, bool value) {
> +
> + pa_assert(param);
> +
> + if (value)
> + pa_strbuf_puts(param->buffer, "{1}");
> + else
> + pa_strbuf_puts(param->buffer, "{0}");
> +}
> diff --git a/src/pulse/message-params.h b/src/pulse/message-params.h
> index c0675c6e..f865e236 100644
> --- a/src/pulse/message-params.h
> +++ b/src/pulse/message-params.h
> @@ -32,6 +32,14 @@ PA_C_DECL_BEGIN
>
> typedef struct pa_message_param pa_message_param;
>
> +/* Read function return values */
> +enum {
> + PA_PARAM_PARSE_ERROR = -1,
> + PA_PARAM_LIST_END = 0,
> + PA_PARAM_OK = 1,
> + PA_PARAM_IS_NULL = 2,
> +};
I think these values should have proper documentation. Probably the
enum should have a name too so that it can be referenced from the
function documentation.
> +
> /** Read functions */
>
> /** Split message parameter string into list elements */
> @@ -40,6 +48,18 @@ int pa_message_param_split_list(char *c, char **result, bool * is_unpacked, void
> /** Read a string from the parameter list. */
> int pa_message_param_read_string(char *c, char **result, bool allocate, void **state);
>
> +/** Read a double from the parameter list. */
> +int pa_message_param_read_double(char *c, double *result, void **state);
> +
> +/** Read an integer from the parameter list. */
> +int pa_message_param_read_int64(char *c, int64_t *result, void **state);
> +
> +/** Read an unsigned integer from the parameter list. */
> +int pa_message_param_read_uint64(char *c, uint64_t *result, void **state);
> +
> +/** Read a boolean from the parameter list. */
> +int pa_message_param_read_bool(char *c, bool *result, void **state);
The return values need to be documented. \since tags are missing.
> +
> /** Write functions */
>
> /** Create a new pa_message_param structure */
> @@ -57,6 +77,21 @@ void pa_message_param_end_list(pa_message_param *param);
> /** Append string to parameter list */
> void pa_message_param_write_string(pa_message_param *param, const char *value, bool do_escape);
>
> +/** Append a double to parameter list */
> +void pa_message_param_write_double(pa_message_param *param, double value, int precision);
> +
> +/** Append an integer to parameter list */
> +void pa_message_param_write_int64(pa_message_param *param, int64_t value);
> +
> +/** Append an unsigned integer to parameter list */
> +void pa_message_param_write_uint64(pa_message_param *param, uint64_t value);
> +
> +/** Append a boolean to parameter list */
> +void pa_message_param_write_bool(pa_message_param *param, bool value);
> +
> +/** Append a raw string to parameter list */
> +void pa_message_param_write_raw(pa_message_param *param, const char *value);
\since tags are missing.
The pa_message_param_write_raw() documentation should be more clear
about that there's no error checking and no escaping or other
modifications are done to the value.
> +
> PA_C_DECL_END
>
> #endif
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
