Discussion:
[pulseaudio-discuss] [PATCH 0/8] core: Add message sending/receiving
Georg Chini
2018-04-09 17:35:17 UTC
Permalink
This is a re-base of the messaging patches. It does not contain
changes. The first two patches have already been reviewed, I
nevertheless included them for completeness.

Georg Chini (8):
core: add simple message interface
protocol-native: add message sending capability
pactl, pacmd, cli-command: Add send-message command
core: add message handler
pactl: Implement list message-handlers
message-params: Allow parameter strings to contain escaped curly
braces
message-params: Add read/write functions for various simple data types
message-params: Add read functions for arrays

PROTOCOL | 14 +
configure.ac | 2 +-
doc/messaging_api.txt | 72 +++++
man/pactl.1.xml.in | 9 +-
man/pulse-cli-syntax.5.xml.in | 7 +
shell-completion/bash/pulseaudio | 7 +-
shell-completion/zsh/_pulseaudio | 3 +
src/Makefile.am | 3 +
src/map-file | 21 ++
src/pulse/introspect.c | 71 +++++
src/pulse/introspect.h | 17 +
src/pulse/message-params.c | 654 +++++++++++++++++++++++++++++++++++++++
src/pulse/message-params.h | 109 +++++++
src/pulsecore/cli-command.c | 44 +++
src/pulsecore/core.c | 49 +++
src/pulsecore/core.h | 2 +-
src/pulsecore/message-handler.c | 148 +++++++++
src/pulsecore/message-handler.h | 50 +++
src/pulsecore/native-common.h | 3 +
src/pulsecore/pdispatch.c | 3 +
src/pulsecore/protocol-native.c | 52 ++++
src/utils/pacmd.c | 1 +
src/utils/pactl.c | 106 ++++++-
23 files changed, 1438 insertions(+), 9 deletions(-)
create mode 100644 doc/messaging_api.txt
create mode 100644 src/pulse/message-params.c
create mode 100644 src/pulse/message-params.h
create mode 100644 src/pulsecore/message-handler.c
create mode 100644 src/pulsecore/message-handler.h
--
2.14.1
Georg Chini
2018-04-09 17:35:18 UTC
Permalink
This patch adds a new feature to the core which allows to send messages
to objects. An object can register/unregister a message handler with
pa_message_handler_{register, unregister}() while a message can be sent
to the handler using the pa_message_handler_send_message() function.
A message has 4 arguments (apart from passing the core):

object_path: The path identifying the object that will receive the message
message: message command
message_parameters: A string containing additional parameters
response: Pointer to a response string that will be filled by the
message handler. The caller is responsible to free the string.

The patch is a precondition for the following patches that allow clients
to send messages to pulseaudio objects.

There is no restriction on object names, except that an object path
always starts with a "/". The intention is to use a path-like syntax,
for example /core/sink_1 for a sink or /name/instances/index for modules.
The exact naming convention still needs to be agreed.
---
src/Makefile.am | 1 +
src/pulsecore/core.c | 4 ++
src/pulsecore/core.h | 2 +-
src/pulsecore/message-handler.c | 104 ++++++++++++++++++++++++++++++++++++++++
src/pulsecore/message-handler.h | 50 +++++++++++++++++++
5 files changed, 160 insertions(+), 1 deletion(-)
create mode 100644 src/pulsecore/message-handler.c
create mode 100644 src/pulsecore/message-handler.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 59b703db..27e5f875 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -964,6 +964,7 @@ ***@PA_MAJORMINOR@_la_SOURCES = \
pulsecore/core-scache.c pulsecore/core-scache.h \
pulsecore/core-subscribe.c pulsecore/core-subscribe.h \
pulsecore/core.c pulsecore/core.h \
+ pulsecore/message-handler.c pulsecore/message-handler.h \
pulsecore/hook-list.c pulsecore/hook-list.h \
pulsecore/ltdl-helper.c pulsecore/ltdl-helper.h \
pulsecore/modargs.c pulsecore/modargs.h \
diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
index da42a13e..f4723728 100644
--- a/src/pulsecore/core.c
+++ b/src/pulsecore/core.c
@@ -103,6 +103,7 @@ pa_core* pa_core_new(pa_mainloop_api *m, bool shared, bool enable_memfd, size_t

c->namereg = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
c->shared = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
+ c->message_handlers = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);

c->default_source = NULL;
c->default_sink = NULL;
@@ -204,6 +205,9 @@ static void core_free(pa_object *o) {
pa_assert(pa_hashmap_isempty(c->shared));
pa_hashmap_free(c->shared);

+ pa_assert(pa_hashmap_isempty(c->message_handlers));
+ pa_hashmap_free(c->message_handlers);
+
pa_assert(pa_hashmap_isempty(c->modules_pending_unload));
pa_hashmap_free(c->modules_pending_unload);

diff --git a/src/pulsecore/core.h b/src/pulsecore/core.h
index 38622f61..d03897c4 100644
--- a/src/pulsecore/core.h
+++ b/src/pulsecore/core.h
@@ -164,7 +164,7 @@ struct pa_core {
pa_idxset *clients, *cards, *sinks, *sources, *sink_inputs, *source_outputs, *modules, *scache;

/* Some hashmaps for all sorts of entities */
- pa_hashmap *namereg, *shared;
+ pa_hashmap *namereg, *shared, *message_handlers;

/* The default sink/source as configured by the user. If the user hasn't
* explicitly configured anything, these are set to NULL. These are strings
diff --git a/src/pulsecore/message-handler.c b/src/pulsecore/message-handler.c
new file mode 100644
index 00000000..7555a18f
--- /dev/null
+++ b/src/pulsecore/message-handler.c
@@ -0,0 +1,104 @@
+/***
+ This file is part of PulseAudio.
+
+ PulseAudio is free software; you can redistribute it and/or modify
+ it under the terms of the GNU Lesser General Public License as published
+ by the Free Software Foundation; either version 2.1 of the License,
+ or (at your option) any later version.
+
+ PulseAudio is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
+***/
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#include <pulse/xmalloc.h>
+
+#include <pulsecore/core.h>
+#include <pulsecore/core-util.h>
+#include <pulsecore/log.h>
+#include <pulsecore/macro.h>
+
+#include "message-handler.h"
+
+/* Message handler functions */
+
+/* Register message handler for the specified object. object_path must be a unique name starting with "/". */
+void pa_message_handler_register(pa_core *c, const char *object_path, const char *description, pa_message_handler_cb_t cb, void *userdata) {
+ struct pa_message_handler *handler;
+
+ pa_assert(c);
+ pa_assert(object_path);
+ pa_assert(cb);
+ pa_assert(userdata);
+
+ /* Ensure that the object path is not empty and starts with "/". */
+ pa_assert(object_path[0] == '/');
+
+ handler = pa_xnew0(struct pa_message_handler, 1);
+ handler->userdata = userdata;
+ handler->callback = cb;
+ handler->object_path = pa_xstrdup(object_path);
+ handler->description = pa_xstrdup(description);
+
+ pa_assert_se(pa_hashmap_put(c->message_handlers, handler->object_path, handler) == 0);
+}
+
+/* Unregister a message handler */
+void pa_message_handler_unregister(pa_core *c, const char *object_path) {
+ struct pa_message_handler *handler;
+
+ pa_assert(c);
+ pa_assert(object_path);
+
+ pa_assert_se(handler = pa_hashmap_remove(c->message_handlers, object_path));
+
+ pa_xfree(handler->object_path);
+ pa_xfree(handler->description);
+ pa_xfree(handler);
+}
+
+/* Send a message to an object identified by object_path */
+int pa_message_handler_send_message(pa_core *c, const char *object_path, const char *message, const char *message_parameters, char **response) {
+ struct pa_message_handler *handler;
+
+ pa_assert(c);
+ pa_assert(object_path);
+ pa_assert(message);
+ pa_assert(response);
+
+ *response = NULL;
+
+ if (!(handler = pa_hashmap_get(c->message_handlers, object_path)))
+ return -PA_ERR_NOENTITY;
+
+ /* The handler is expected to return an error code and may also
+ return an error string in response */
+ return handler->callback(handler->object_path, message, message_parameters, response, handler->userdata);
+}
+
+/* Set handler description */
+int pa_message_handler_set_description(pa_core *c, const char *object_path, const char *description) {
+ struct pa_message_handler *handler;
+
+ pa_assert(c);
+ pa_assert(object_path);
+
+ if (!(handler = pa_hashmap_get(c->message_handlers, object_path)))
+ return -PA_ERR_NOENTITY;
+
+ pa_xfree(handler->description);
+ handler->description = pa_xstrdup(description);
+
+ return PA_OK;
+}
diff --git a/src/pulsecore/message-handler.h b/src/pulsecore/message-handler.h
new file mode 100644
index 00000000..be94510f
--- /dev/null
+++ b/src/pulsecore/message-handler.h
@@ -0,0 +1,50 @@
+#ifndef foocoremessageshfoo
+#define foocoremessageshfoo
+
+/***
+ This file is part of PulseAudio.
+
+ PulseAudio is free software; you can redistribute it and/or modify
+ it under the terms of the GNU Lesser General Public License as published
+ by the Free Software Foundation; either version 2.1 of the License,
+ or (at your option) any later version.
+
+ PulseAudio is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <pulsecore/core.h>
+
+/* Message handler types and functions */
+
+/* Prototype for message callback */
+typedef int (*pa_message_handler_cb_t)(
+ const char *object_path,
+ const char *message,
+ const char *message_parameters,
+ char **response,
+ void *userdata);
+
+/* Message handler object */
+struct pa_message_handler {
+ char *object_path;
+ char *description;
+ pa_message_handler_cb_t callback;
+ void *userdata;
+};
+
+/* Handler registration */
+void pa_message_handler_register(pa_core *c, const char *object_path, const char *description, pa_message_handler_cb_t cb, void *userdata);
+void pa_message_handler_unregister(pa_core *c, const char *object_path);
+
+/* Send message to the specified object path */
+int pa_message_handler_send_message(pa_core *c, const char *object_path, const char *message, const char *message_parameters, char **response);
+
+/* Set handler description */
+int pa_message_handler_set_description(pa_core *c, const char *object_path, const char *description);
+#endif
--
2.14.1
Tanu Kaskinen
2018-07-04 12:36:17 UTC
Permalink
Post by Georg Chini
This patch adds a new feature to the core which allows to send messages
to objects. An object can register/unregister a message handler with
pa_message_handler_{register, unregister}() while a message can be sent
to the handler using the pa_message_handler_send_message() function.
object_path: The path identifying the object that will receive the message
message: message command
message_parameters: A string containing additional parameters
response: Pointer to a response string that will be filled by the
message handler. The caller is responsible to free the string.
The patch is a precondition for the following patches that allow clients
to send messages to pulseaudio objects.
There is no restriction on object names, except that an object path
always starts with a "/". The intention is to use a path-like syntax,
for example /core/sink_1 for a sink or /name/instances/index for modules.
The exact naming convention still needs to be agreed.
---
src/Makefile.am | 1 +
src/pulsecore/core.c | 4 ++
src/pulsecore/core.h | 2 +-
src/pulsecore/message-handler.c | 104 ++++++++++++++++++++++++++++++++++++++++
src/pulsecore/message-handler.h | 50 +++++++++++++++++++
5 files changed, 160 insertions(+), 1 deletion(-)
create mode 100644 src/pulsecore/message-handler.c
create mode 100644 src/pulsecore/message-handler.h
Looks good to me.
--
Tanu

https://liberapay.com/tanuk
https://www.patreon.com/tanuk
Georg Chini
2018-04-09 17:35:20 UTC
Permalink
The patch also adds some API documentation.
---
doc/messaging_api.txt | 15 ++++++++++++++
man/pactl.1.xml.in | 7 +++++++
man/pulse-cli-syntax.5.xml.in | 7 +++++++
shell-completion/bash/pulseaudio | 5 +++--
shell-completion/zsh/_pulseaudio | 2 ++
src/pulse/introspect.h | 3 ++-
src/pulsecore/cli-command.c | 44 ++++++++++++++++++++++++++++++++++++++++
src/utils/pacmd.c | 1 +
src/utils/pactl.c | 43 ++++++++++++++++++++++++++++++++++++++-
9 files changed, 123 insertions(+), 4 deletions(-)
create mode 100644 doc/messaging_api.txt

diff --git a/doc/messaging_api.txt b/doc/messaging_api.txt
new file mode 100644
index 00000000..cbc06e75
--- /dev/null
+++ b/doc/messaging_api.txt
@@ -0,0 +1,15 @@
+Message API reference
+
+The message API allows any object within pulseaudio to register a message
+handler. A message handler is a function that can be called by clients using
+PA_COMMAND_SEND_OBJECT_MESSAGE. A message consists at least of an object path
+and a message command, both specified as strings. Additional parameters can
+be specified using a single string, but are not mandatory. The message handler
+returns an error number as defined in def.h and also returns a string in
+the "response" variable. The following reference lists available messages,
+their parameters and return values.
+
+Recipient:
+Message:
+Parameters:
+Return value:
diff --git a/man/pactl.1.xml.in b/man/pactl.1.xml.in
index 39569b6b..4052fae3 100644
--- a/man/pactl.1.xml.in
+++ b/man/pactl.1.xml.in
@@ -245,6 +245,13 @@ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
'ac3-iec61937, format.rate = "[ 32000, 44100, 48000 ]"').
</p></optdesc> </option>

+ <option>
+ <p><opt>send-message</opt> <arg>RECIPIENT</arg> <arg>MESSAGE</arg> <arg>MESSAGE_PARAMETERS</arg></p>
+ <optdesc><p>Send a message string to the specified recipient object. If applicable an additional string containing
+ message parameters can be specified. A string is returned as a response to the message. For available message
+ commands see https://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/doc/messaging_api.txt.</p></optdesc>
+ </option>
+
<option>
<p><opt>subscribe</opt></p>
<optdesc><p>Subscribe to events, pactl does not exit by itself, but keeps waiting for new events.</p></optdesc>
diff --git a/man/pulse-cli-syntax.5.xml.in b/man/pulse-cli-syntax.5.xml.in
index 0a0fabaf..83f55d6b 100644
--- a/man/pulse-cli-syntax.5.xml.in
+++ b/man/pulse-cli-syntax.5.xml.in
@@ -297,6 +297,13 @@ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
<optdesc><p>Debug: Show shared properties.</p></optdesc>
</option>

+ <option>
+ <p><opt>send-message</opt> <arg>recipient</arg> <arg>message</arg> <arg>message_parameters</arg></p>
+ <optdesc><p>Send a message string to the specified recipient object. If applicable an additional string containing
+ message parameters can be specified. A string is returned as a response to the message. For available message
+ commands see https://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/doc/messaging_api.txt.</p></optdesc>
+ </option>
+
<option>
<p><opt>exit</opt></p>
<optdesc><p>Terminate the daemon. If you want to terminate a CLI
diff --git a/shell-completion/bash/pulseaudio b/shell-completion/bash/pulseaudio
index e473b9c2..797ec067 100644
--- a/shell-completion/bash/pulseaudio
+++ b/shell-completion/bash/pulseaudio
@@ -120,7 +120,8 @@ _pactl() {
set-source-port set-sink-volume set-source-volume
set-sink-input-volume set-source-output-volume set-sink-mute
set-source-mute set-sink-input-mute set-source-output-mute
- set-sink-formats set-port-latency-offset subscribe help)
+ set-sink-formats set-port-latency-offset subscribe send-message
+ help)

_init_completion -n = || return
preprev=${words[$cword-2]}
@@ -270,7 +271,7 @@ _pacmd() {
move-sink-input move-source-output suspend-sink suspend-source
suspend set-card-profile set-sink-port set-source-port
set-port-latency-offset set-log-target set-log-level set-log-meta
- set-log-time set-log-backtrace)
+ set-log-time set-log-backtrace send-message)
_init_completion -n = || return
preprev=${words[$cword-2]}

diff --git a/shell-completion/zsh/_pulseaudio b/shell-completion/zsh/_pulseaudio
index 0e9e89bd..a2817ebb 100644
--- a/shell-completion/zsh/_pulseaudio
+++ b/shell-completion/zsh/_pulseaudio
@@ -263,6 +263,7 @@ _pactl_completion() {
'set-sink-input-mute: mute a stream'
'set-source-output-mute: mute a recording stream'
'set-sink-formats: set supported formats of a sink'
+ 'send-message: send a message to a pulseaudio object'
'subscribe: subscribe to events'
)

@@ -561,6 +562,7 @@ _pacmd_completion() {
'dump: show daemon configuration'
'dump-volumes: show the state of all volumes'
'shared: show shared properties'
+ 'send-message: send a message to a pulseaudio object'
'exit: ask the PulseAudio daemon to exit'
)
_describe 'pacmd commands' _pacmd_commands
diff --git a/src/pulse/introspect.h b/src/pulse/introspect.h
index 7e47e740..fbe5b668 100644
--- a/src/pulse/introspect.h
+++ b/src/pulse/introspect.h
@@ -451,7 +451,8 @@ pa_operation* pa_context_unload_module(pa_context *c, uint32_t idx, pa_context_s
/** Callback prototype for pa_context_send_message_to_object() */
typedef void (*pa_context_string_cb_t)(pa_context *c, int success, const char *response, void *userdata);

-/** Send a message to an object that registered a message handler. */
+/** Send a message to an object that registered a message handler. For more information
+ * see https://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/doc/messaging_api.txt. */
pa_operation* pa_context_send_message_to_object(pa_context *c, const char *recipient_name, const char *message, const char *message_parameters, pa_context_string_cb_t cb, void *userdata);

/** @} */
diff --git a/src/pulsecore/cli-command.c b/src/pulsecore/cli-command.c
index 44795b0d..f0cd617f 100644
--- a/src/pulsecore/cli-command.c
+++ b/src/pulsecore/cli-command.c
@@ -53,6 +53,7 @@
#include <pulsecore/sound-file-stream.h>
#include <pulsecore/shared.h>
#include <pulsecore/core-util.h>
+#include <pulsecore/message-handler.h>
#include <pulsecore/core-error.h>
#include <pulsecore/modinfo.h>
#include <pulsecore/dynarray.h>
@@ -135,6 +136,7 @@ static int pa_cli_command_sink_port(pa_core *c, pa_tokenizer *t, pa_strbuf *buf,
static int pa_cli_command_source_port(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, bool *fail);
static int pa_cli_command_port_offset(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, bool *fail);
static int pa_cli_command_dump_volumes(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, bool *fail);
+static int pa_cli_command_send_message_to_object(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, bool *fail);

/* A method table for all available commands */

@@ -191,6 +193,7 @@ static const struct command commands[] = {
{ "set-log-meta", pa_cli_command_log_meta, "Show source code location in log messages (args: bool)", 2},
{ "set-log-time", pa_cli_command_log_time, "Show timestamps in log messages (args: bool)", 2},
{ "set-log-backtrace", pa_cli_command_log_backtrace, "Show backtrace in log messages (args: frames)", 2},
+ { "send-message", pa_cli_command_send_message_to_object, "Send a message to an object (args: recipient, message, message_parameters)", 4},
{ "play-file", pa_cli_command_play_file, "Play a sound file (args: filename, sink|index)", 3},
{ "dump", pa_cli_command_dump, "Dump daemon configuration", 1},
{ "dump-volumes", pa_cli_command_dump_volumes, "Debug: Show the state of all volumes", 1 },
@@ -1779,6 +1782,47 @@ static int pa_cli_command_port_offset(pa_core *c, pa_tokenizer *t, pa_strbuf *bu
return 0;
}

+static int pa_cli_command_send_message_to_object(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, bool *fail) {
+ const char *object_path, *message, *message_parameters;
+ char *response = NULL;
+ int ret;
+
+ pa_core_assert_ref(c);
+ pa_assert(t);
+ pa_assert(buf);
+ pa_assert(fail);
+
+
+ if (!(object_path = pa_tokenizer_get(t, 1))) {
+ pa_strbuf_puts(buf, "You need to specify an object path as recipient for the message.\n");
+ return -1;
+ }
+
+ if (!(message = pa_tokenizer_get(t, 2))) {
+ pa_strbuf_puts(buf, "You need to specify a message name.\n");
+ return -1;
+ }
+
+ /* parameters may be NULL */
+ message_parameters = pa_tokenizer_get(t, 3);
+
+ ret = pa_message_handler_send_message(c, object_path, message, message_parameters, &response);
+
+ if (ret < 0) {
+ pa_strbuf_printf(buf, "Send message failed: %s\n", pa_strerror(ret));
+ ret = -1;
+
+ } else {
+ if (response)
+ pa_strbuf_puts(buf, response);
+ pa_strbuf_puts(buf, "\n");
+ ret = 0;
+ }
+
+ pa_xfree(response);
+ return ret;
+}
+
static int pa_cli_command_dump(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, bool *fail) {
pa_module *m;
pa_sink *sink;
diff --git a/src/utils/pacmd.c b/src/utils/pacmd.c
index 616573cc..6eae6c42 100644
--- a/src/utils/pacmd.c
+++ b/src/utils/pacmd.c
@@ -77,6 +77,7 @@ static void help(const char *argv0) {
printf("%s %s %s\n", argv0, "set-log-meta", _("1|0"));
printf("%s %s %s\n", argv0, "set-log-time", _("1|0"));
printf("%s %s %s\n", argv0, "set-log-backtrace", _("FRAMES"));
+ printf("%s %s %s\n", argv0, "send-message", _("RECIPIENT MESSAGE [MESSAGE_PARAMETERS]"));

printf(_("\n"
" -h, --help Show this help\n"
diff --git a/src/utils/pactl.c b/src/utils/pactl.c
index e9bf005b..66c0f251 100644
--- a/src/utils/pactl.c
+++ b/src/utils/pactl.c
@@ -56,7 +56,10 @@ static char
*card_name = NULL,
*profile_name = NULL,
*port_name = NULL,
- *formats = NULL;
+ *formats = NULL,
+ *object_path = NULL,
+ *message = NULL,
+ *message_args = NULL;

static uint32_t
sink_input_idx = PA_INVALID_INDEX,
@@ -130,6 +133,7 @@ static enum {
SET_SOURCE_OUTPUT_MUTE,
SET_SINK_FORMATS,
SET_PORT_LATENCY_OFFSET,
+ SEND_OBJECT_MESSAGE,
SUBSCRIBE
} action = NONE;

@@ -834,6 +838,19 @@ static void index_callback(pa_context *c, uint32_t idx, void *userdata) {
complete_action();
}

+static void send_message_callback(pa_context *c, int success, const char *response, void *userdata) {
+
+ if (!success) {
+ pa_log(_("Send message failed: %s"), pa_strerror(pa_context_errno(c)));
+ quit(1);
+ return;
+ }
+
+ printf("%s\n", response);
+
+ complete_action();
+}
+
static void volume_relative_adjust(pa_cvolume *cv) {
pa_assert(volume_flags & VOL_RELATIVE);

@@ -1404,6 +1421,10 @@ static void context_state_callback(pa_context *c, void *userdata) {
o = pa_context_set_port_latency_offset(c, card_name, port_name, latency_offset, simple_callback, NULL);
break;

+ case SEND_OBJECT_MESSAGE:
+ o = pa_context_send_message_to_object(c, object_path, message, message_args, send_message_callback, NULL);
+ break;
+
case SUBSCRIBE:
pa_context_set_subscribe_callback(c, context_subscribe_callback, NULL);

@@ -1580,6 +1601,7 @@ static void help(const char *argv0) {
printf("%s %s %s %s\n", argv0, _("[options]"), "set-(sink-input|source-output)-mute", _("#N 1|0|toggle"));
printf("%s %s %s %s\n", argv0, _("[options]"), "set-sink-formats", _("#N FORMATS"));
printf("%s %s %s %s\n", argv0, _("[options]"), "set-port-latency-offset", _("CARD-NAME|CARD-#N PORT OFFSET"));
+ printf("%s %s %s %s\n", argv0, _("[options]"), "send-message", _("RECIPIENT MESSAGE [MESSAGE_PARAMETERS]"));
printf("%s %s %s\n", argv0, _("[options]"), "subscribe");
printf(_("\nThe special names @DEFAULT_SINK@, @DEFAULT_SOURCE@ and @DEFAULT_MONITOR@\n"
"can be used to specify the default sink, source and monitor.\n"));
@@ -2015,6 +2037,22 @@ int main(int argc, char *argv[]) {
goto quit;
}

+ } else if (pa_streq(argv[optind], "send-message")) {
+ action = SEND_OBJECT_MESSAGE;
+
+ if (argc < optind+3) {
+ pa_log(_("You have to specify at least an object path and a message name"));
+ goto quit;
+ }
+
+ object_path = pa_xstrdup(argv[optind + 1]);
+ message = pa_xstrdup(argv[optind + 2]);
+ if (argc >= optind+4)
+ message_args = pa_xstrdup(argv[optind + 3]);
+
+ if (argc > optind+4)
+ pa_log(_("Excess arguments given, they will be ignored. Note that all message parameters must be given as a single string."));
+
} else if (pa_streq(argv[optind], "subscribe"))

action = SUBSCRIBE;
@@ -2108,6 +2146,9 @@ quit:
pa_xfree(profile_name);
pa_xfree(port_name);
pa_xfree(formats);
+ pa_xfree(object_path);
+ pa_xfree(message);
+ pa_xfree(message_args);

if (sndfile)
sf_close(sndfile);
--
2.14.1
Tanu Kaskinen
2018-07-09 14:24:03 UTC
Permalink
Post by Georg Chini
The patch also adds some API documentation.
---
doc/messaging_api.txt | 15 ++++++++++++++
man/pactl.1.xml.in | 7 +++++++
man/pulse-cli-syntax.5.xml.in | 7 +++++++
shell-completion/bash/pulseaudio | 5 +++--
shell-completion/zsh/_pulseaudio | 2 ++
src/pulse/introspect.h | 3 ++-
src/pulsecore/cli-command.c | 44 ++++++++++++++++++++++++++++++++++++++++
src/utils/pacmd.c | 1 +
src/utils/pactl.c | 43 ++++++++++++++++++++++++++++++++++++++-
9 files changed, 123 insertions(+), 4 deletions(-)
create mode 100644 doc/messaging_api.txt
diff --git a/doc/messaging_api.txt b/doc/messaging_api.txt
new file mode 100644
index 00000000..cbc06e75
--- /dev/null
+++ b/doc/messaging_api.txt
@@ -0,0 +1,15 @@
+Message API reference
+
+The message API allows any object within pulseaudio to register a message
+handler. A message handler is a function that can be called by clients using
+PA_COMMAND_SEND_OBJECT_MESSAGE. A message consists at least of an object path
+and a message command, both specified as strings. Additional parameters can
+be specified using a single string, but are not mandatory. The message handler
+returns an error number as defined in def.h and also returns a string in
+the "response" variable. The following reference lists available messages,
+their parameters and return values.
+
diff --git a/man/pactl.1.xml.in b/man/pactl.1.xml.in
index 39569b6b..4052fae3 100644
--- a/man/pactl.1.xml.in
+++ b/man/pactl.1.xml.in
@@ -245,6 +245,13 @@ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
'ac3-iec61937, format.rate = "[ 32000, 44100, 48000 ]"').
</p></optdesc> </option>
+ <option>
+ <p><opt>send-message</opt> <arg>RECIPIENT</arg> <arg>MESSAGE</arg> <arg>MESSAGE_PARAMETERS</arg></p>
+ <optdesc><p>Send a message string to the specified recipient object. If applicable an additional string containing
I think "a message" is better than "a message string".
Post by Georg Chini
+ message parameters can be specified. A string is returned as a response to the message. For available message
+ commands see https://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/doc/messaging_api.txt.</p></optdesc>;
I think "available messages" is better than "available message
commands".
Post by Georg Chini
+ </option>
+
<option>
<p><opt>subscribe</opt></p>
<optdesc><p>Subscribe to events, pactl does not exit by itself, but keeps waiting for new events.</p></optdesc>
diff --git a/man/pulse-cli-syntax.5.xml.in b/man/pulse-cli-syntax.5.xml.in
index 0a0fabaf..83f55d6b 100644
--- a/man/pulse-cli-syntax.5.xml.in
+++ b/man/pulse-cli-syntax.5.xml.in
@@ -297,6 +297,13 @@ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
<optdesc><p>Debug: Show shared properties.</p></optdesc>
</option>
+ <option>
+ <p><opt>send-message</opt> <arg>recipient</arg> <arg>message</arg> <arg>message_parameters</arg></p>
+ <optdesc><p>Send a message string to the specified recipient object. If applicable an additional string containing
+ message parameters can be specified. A string is returned as a response to the message. For available message
+ commands see https://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/doc/messaging_api.txt.</p></optdesc>;
+ </option>
Same as above.
Post by Georg Chini
diff --git a/src/pulse/introspect.h b/src/pulse/introspect.h
index 7e47e740..fbe5b668 100644
--- a/src/pulse/introspect.h
+++ b/src/pulse/introspect.h
@@ -451,7 +451,8 @@ pa_operation* pa_context_unload_module(pa_context *c, uint32_t idx, pa_context_s
/** Callback prototype for pa_context_send_message_to_object() */
typedef void (*pa_context_string_cb_t)(pa_context *c, int success, const char *response, void *userdata);
-/** Send a message to an object that registered a message handler. */
+/** Send a message to an object that registered a message handler. For more information
+ * see https://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/doc/messaging_api.txt. */
pa_operation* pa_context_send_message_to_object(pa_context *c, const char *recipient_name, const char *message, const char *message_parameters, pa_context_string_cb_t cb, void *userdata);
Apparently I didn't notice when reviewing the previous patch that the
documentation is lacking a \since tag.
--
Tanu

https://liberapay.com/tanuk
https://www.patreon.com/tanuk
Georg Chini
2018-04-09 17:35:21 UTC
Permalink
This patch adds a small message handler to the core which enables
clients to list available handlers via the list-handlers message.
Command: pacmd send-message /core list-handlers
pactl can be used with the same parameters.

The patch also introduces a convention for the return string.
It consists of a list of elements where curly braces are used
to separate elements. Each element can itself contain further
elements. For example consider a message that returns multiple
elements which each contain an integer and an array of float.
A response string would look like that:
{{Integer} {{1st float} {2nd float} ...}}{...}
---
doc/messaging_api.txt | 21 +++++++++++++------
src/pulsecore/core.c | 45 +++++++++++++++++++++++++++++++++++++++++
src/pulsecore/message-handler.c | 24 ++++++++++++++++++++++
3 files changed, 84 insertions(+), 6 deletions(-)

diff --git a/doc/messaging_api.txt b/doc/messaging_api.txt
index cbc06e75..431a5df2 100644
--- a/doc/messaging_api.txt
+++ b/doc/messaging_api.txt
@@ -6,10 +6,19 @@ PA_COMMAND_SEND_OBJECT_MESSAGE. A message consists at least of an object path
and a message command, both specified as strings. Additional parameters can
be specified using a single string, but are not mandatory. The message handler
returns an error number as defined in def.h and also returns a string in
-the "response" variable. The following reference lists available messages,
-their parameters and return values.
+the "response" variable. If the string is not empty it consists of elements.
+Curly braces are used to separate elements. Each element can itself contain
+further elements. For example consider a message that returns multiple elements
+which each contain an integer and an array of float. A response string would
+look like that:
+{{Integer} {{1st float} {2nd float} ...}}{...}
+Any characters that are not enclosed in curly braces are ignored (all characters
+between { and {, between } and } and between } and {). The same syntax is used
+to specify message parameters. The following reference lists available messages,
+their parameters and return values. If a return value is enclosed in {}, this
+means that multiple elements of the same type may be returned.

-Recipient:
-Message:
-Parameters:
-Return value:
+Object path: /core
+Message: list-handlers
+Parameters: None
+Return value: {{{Handler name} {Description}} ...}
diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
index f4723728..10cd6f2f 100644
--- a/src/pulsecore/core.c
+++ b/src/pulsecore/core.c
@@ -33,11 +33,13 @@
#include <pulsecore/module.h>
#include <pulsecore/core-rtclock.h>
#include <pulsecore/core-util.h>
+#include <pulsecore/message-handler.h>
#include <pulsecore/core-scache.h>
#include <pulsecore/core-subscribe.h>
#include <pulsecore/random.h>
#include <pulsecore/log.h>
#include <pulsecore/macro.h>
+#include <pulsecore/strbuf.h>

#include "core.h"

@@ -61,6 +63,45 @@ static int core_process_msg(pa_msgobject *o, int code, void *userdata, int64_t o

static void core_free(pa_object *o);

+/* Returns a list of handlers. */
+static char *message_handler_list(pa_core *c) {
+ pa_strbuf *buf;
+ void *state = NULL;
+ struct pa_message_handler *handler;
+
+ buf = pa_strbuf_new();
+
+ pa_strbuf_putc(buf, '{');
+ PA_HASHMAP_FOREACH(handler, c->message_handlers, state) {
+ pa_strbuf_putc(buf, '{');
+
+ pa_strbuf_printf(buf, "{%s} {", handler->object_path);
+ if (handler->description)
+ pa_strbuf_puts(buf, handler->description);
+
+ pa_strbuf_puts(buf, "}}");
+ }
+ pa_strbuf_putc(buf, '}');
+
+ return pa_strbuf_to_string_free(buf);
+}
+
+static int core_message_handler(const char *object_path, const char *message, const char *message_parameters, char **response, void *userdata) {
+ pa_core *c;
+
+ pa_assert(c = (pa_core *) userdata);
+ pa_assert(message);
+ pa_assert(response);
+ pa_assert(pa_safe_streq(object_path, "/core"));
+
+ if (pa_streq(message, "list-handlers")) {
+ *response = message_handler_list(c);
+ return PA_OK;
+ }
+
+ return -PA_ERR_NOTIMPLEMENTED;
+}
+
pa_core* pa_core_new(pa_mainloop_api *m, bool shared, bool enable_memfd, size_t shm_size) {
pa_core* c;
pa_mempool *pool;
@@ -105,6 +146,8 @@ pa_core* pa_core_new(pa_mainloop_api *m, bool shared, bool enable_memfd, size_t
c->shared = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
c->message_handlers = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);

+ pa_message_handler_register(c, "/core", "Core message handler", core_message_handler, (void *) c);
+
c->default_source = NULL;
c->default_sink = NULL;

@@ -205,6 +248,8 @@ static void core_free(pa_object *o) {
pa_assert(pa_hashmap_isempty(c->shared));
pa_hashmap_free(c->shared);

+ pa_message_handler_unregister(c, "/core");
+
pa_assert(pa_hashmap_isempty(c->message_handlers));
pa_hashmap_free(c->message_handlers);

diff --git a/src/pulsecore/message-handler.c b/src/pulsecore/message-handler.c
index 7555a18f..18c62fc2 100644
--- a/src/pulsecore/message-handler.c
+++ b/src/pulsecore/message-handler.c
@@ -31,6 +31,20 @@

#include "message-handler.h"

+/* Check if a string does not contain control characters. Currently these are
+ * only "{" and "}". */
+static bool string_is_valid(const char *test_string) {
+ uint32_t i;
+
+ for (i = 0; test_string[i]; i++) {
+ if (test_string[i] == '{' ||
+ test_string[i] == '}')
+ return false;
+ }
+
+ return true;
+}
+
/* Message handler functions */

/* Register message handler for the specified object. object_path must be a unique name starting with "/". */
@@ -45,6 +59,11 @@ void pa_message_handler_register(pa_core *c, const char *object_path, const char
/* Ensure that the object path is not empty and starts with "/". */
pa_assert(object_path[0] == '/');

+ /* Ensure that object path and description are valid strings */
+ pa_assert(string_is_valid(object_path));
+ if (description)
+ pa_assert(string_is_valid(description));
+
handler = pa_xnew0(struct pa_message_handler, 1);
handler->userdata = userdata;
handler->callback = cb;
@@ -97,6 +116,11 @@ int pa_message_handler_set_description(pa_core *c, const char *object_path, cons
if (!(handler = pa_hashmap_get(c->message_handlers, object_path)))
return -PA_ERR_NOENTITY;

+ if (description) {
+ if (!string_is_valid(description))
+ return -PA_ERR_INVALID;
+ }
+
pa_xfree(handler->description);
handler->description = pa_xstrdup(description);
--
2.14.1
Tanu Kaskinen
2018-07-15 08:26:57 UTC
Permalink
Post by Georg Chini
This patch adds a small message handler to the core which enables
clients to list available handlers via the list-handlers message.
Command: pacmd send-message /core list-handlers
pactl can be used with the same parameters.
The patch also introduces a convention for the return string.
It consists of a list of elements where curly braces are used
to separate elements. Each element can itself contain further
elements. For example consider a message that returns multiple
elements which each contain an integer and an array of float.
{{Integer} {{1st float} {2nd float} ...}}{...}
---
doc/messaging_api.txt | 21 +++++++++++++------
src/pulsecore/core.c | 45 +++++++++++++++++++++++++++++++++++++++++
src/pulsecore/message-handler.c | 24 ++++++++++++++++++++++
3 files changed, 84 insertions(+), 6 deletions(-)
Looks good to me.
--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
Georg Chini
2018-04-09 17:35:19 UTC
Permalink
This patch adds the PA_COMMAND_SEND_OBJECT_MESSAGE command to protocol-native
so that clients can use the messaging feature introduced in the previous patch.

Sending messages can in effect replace the extension system for modules. The
approach is more flexible than the extension interface because a generic string
format is used to exchange information. Furthermore the messaging system can be
used for any object, not only for modules, and is easier to implement than
extensions.
---
PROTOCOL | 14 +++++++++
configure.ac | 2 +-
src/map-file | 1 +
src/pulse/introspect.c | 64 +++++++++++++++++++++++++++++++++++++++++
src/pulse/introspect.h | 16 +++++++++++
src/pulsecore/native-common.h | 3 ++
src/pulsecore/pdispatch.c | 3 ++
src/pulsecore/protocol-native.c | 52 +++++++++++++++++++++++++++++++++
8 files changed, 154 insertions(+), 1 deletion(-)

diff --git a/PROTOCOL b/PROTOCOL
index 546998b7..f693cd3d 100644
--- a/PROTOCOL
+++ b/PROTOCOL
@@ -420,6 +420,20 @@ memfd support only to 10.0+ clients.

Check commit 451d1d676237c81 for further details.

+## v33, implemented by >= 13.0
+
+Added new command for communication with objects.
+
+PA_COMMAND_SEND_OBJECT_MESSAGE:
+sends a message to an object identified by an object path
+
+parameters:
+ string object_path - unique path identifying the object
+ string message - message command
+ string message_parameters - additional parameters if required
+
+The command returns a string.
+
#### If you just changed the protocol, read this
## module-tunnel depends on the sink/source/sink-input/source-input protocol
## internals, so if you changed these, you might have broken module-tunnel.
diff --git a/configure.ac b/configure.ac
index 57cb3a92..5537d921 100644
--- a/configure.ac
+++ b/configure.ac
@@ -40,7 +40,7 @@ AC_SUBST(PA_MINOR, pa_minor)
AC_SUBST(PA_MAJORMINOR, pa_major.pa_minor)

AC_SUBST(PA_API_VERSION, 12)
-AC_SUBST(PA_PROTOCOL_VERSION, 32)
+AC_SUBST(PA_PROTOCOL_VERSION, 33)

# The stable ABI for client applications, for the version info x:y:z
# always will hold y=z
diff --git a/src/map-file b/src/map-file
index 9b6cba22..4d747f19 100644
--- a/src/map-file
+++ b/src/map-file
@@ -87,6 +87,7 @@ pa_context_remove_autoload_by_name;
pa_context_remove_sample;
pa_context_rttime_new;
pa_context_rttime_restart;
+pa_context_send_message_to_object;
pa_context_set_card_profile_by_index;
pa_context_set_card_profile_by_name;
pa_context_set_default_sink;
diff --git a/src/pulse/introspect.c b/src/pulse/introspect.c
index 510d784a..76bfee41 100644
--- a/src/pulse/introspect.c
+++ b/src/pulse/introspect.c
@@ -2184,3 +2184,67 @@ pa_operation* pa_context_suspend_source_by_index(pa_context *c, uint32_t idx, in

return o;
}
+
+/** Object response string processing **/
+
+static void context_string_callback(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
+ pa_operation *o = userdata;
+ const char *response;
+ int success = 1;
+
+ pa_assert(pd);
+ pa_assert(o);
+ pa_assert(PA_REFCNT_VALUE(o) >= 1);
+
+ if (!o->context)
+ goto finish;
+
+ if (command != PA_COMMAND_REPLY) {
+ if (pa_context_handle_error(o->context, command, t, false) < 0)
+ goto finish;
+
+ success = 0;
+ response = "";
+ } else if (pa_tagstruct_gets(t, &response) < 0 ||
+ !pa_tagstruct_eof(t)) {
+ pa_context_fail(o->context, PA_ERR_PROTOCOL);
+ goto finish;
+ }
+
+ if (!response)
+ response = "";
+
+ if (o->callback) {
+ pa_context_string_cb_t cb = (pa_context_string_cb_t) o->callback;
+ cb(o->context, success, response, o->userdata);
+ }
+
+finish:
+ pa_operation_done(o);
+ pa_operation_unref(o);
+}
+
+pa_operation* pa_context_send_message_to_object(pa_context *c, const char *object_path, const char *message, const char *message_parameters, pa_context_string_cb_t cb, void *userdata) {
+ pa_operation *o;
+ pa_tagstruct *t;
+ uint32_t tag;
+
+ pa_assert(c);
+ pa_assert(PA_REFCNT_VALUE(c) >= 1);
+
+ PA_CHECK_VALIDITY_RETURN_NULL(c, !pa_detect_fork(), PA_ERR_FORKED);
+ PA_CHECK_VALIDITY_RETURN_NULL(c, c->state == PA_CONTEXT_READY, PA_ERR_BADSTATE);
+
+ o = pa_operation_new(c, NULL, (pa_operation_cb_t) cb, userdata);
+
+ t = pa_tagstruct_command(c, PA_COMMAND_SEND_OBJECT_MESSAGE, &tag);
+
+ pa_tagstruct_puts(t, object_path);
+ pa_tagstruct_puts(t, message);
+ pa_tagstruct_puts(t, message_parameters);
+
+ pa_pstream_send_tagstruct(c->pstream, t);
+ pa_pdispatch_register_reply(c->pdispatch, tag, DEFAULT_TIMEOUT, context_string_callback, pa_operation_ref(o), (pa_free_cb_t) pa_operation_unref);
+
+ return o;
+}
diff --git a/src/pulse/introspect.h b/src/pulse/introspect.h
index 43389b73..7e47e740 100644
--- a/src/pulse/introspect.h
+++ b/src/pulse/introspect.h
@@ -204,6 +204,12 @@
* Server modules can be remotely loaded and unloaded using
* pa_context_load_module() and pa_context_unload_module().
*
+ * \subsection message_subsec Messages
+ *
+ * Server objects like sinks, sink inputs or modules can register a message
+ * handler to communicate with clients. A message can be sent to a named
+ * message handler using pa_context_send_message_to_object().
+ *
* \subsection client_subsec Clients
*
* The only operation supported on clients is the possibility of kicking
@@ -440,6 +446,16 @@ pa_operation* pa_context_unload_module(pa_context *c, uint32_t idx, pa_context_s

/** @} */

+/** @{ \name Messages */
+
+/** Callback prototype for pa_context_send_message_to_object() */
+typedef void (*pa_context_string_cb_t)(pa_context *c, int success, const char *response, void *userdata);
+
+/** Send a message to an object that registered a message handler. */
+pa_operation* pa_context_send_message_to_object(pa_context *c, const char *recipient_name, const char *message, const char *message_parameters, pa_context_string_cb_t cb, void *userdata);
+
+/** @} */
+
/** @{ \name Clients */

/** Stores information about clients. Please note that this structure
diff --git a/src/pulsecore/native-common.h b/src/pulsecore/native-common.h
index 70338b9f..561746d7 100644
--- a/src/pulsecore/native-common.h
+++ b/src/pulsecore/native-common.h
@@ -187,6 +187,9 @@ enum {
* BOTH DIRECTIONS */
PA_COMMAND_REGISTER_MEMFD_SHMID,

+ /* Supported since protocol v33 (13.0) */
+ PA_COMMAND_SEND_OBJECT_MESSAGE,
+
PA_COMMAND_MAX
};

diff --git a/src/pulsecore/pdispatch.c b/src/pulsecore/pdispatch.c
index ab632a5a..9f9dc981 100644
--- a/src/pulsecore/pdispatch.c
+++ b/src/pulsecore/pdispatch.c
@@ -199,6 +199,9 @@ static const char *command_names[PA_COMMAND_MAX] = {
/* Supported since protocol v31 (9.0) */
/* BOTH DIRECTIONS */
[PA_COMMAND_REGISTER_MEMFD_SHMID] = "REGISTER_MEMFD_SHMID",
+
+ /* Supported since protocol v33 (13.0) */
+ [PA_COMMAND_SEND_OBJECT_MESSAGE] = "SEND_OBJECT_MESSAGE",
};

#endif
diff --git a/src/pulsecore/protocol-native.c b/src/pulsecore/protocol-native.c
index 2d45a4cb..4e35d406 100644
--- a/src/pulsecore/protocol-native.c
+++ b/src/pulsecore/protocol-native.c
@@ -47,6 +47,7 @@
#include <pulsecore/namereg.h>
#include <pulsecore/core-scache.h>
#include <pulsecore/core-subscribe.h>
+#include <pulsecore/message-handler.h>
#include <pulsecore/log.h>
#include <pulsecore/mem.h>
#include <pulsecore/strlist.h>
@@ -4685,6 +4686,55 @@ static void command_extension(pa_pdispatch *pd, uint32_t command, uint32_t tag,
protocol_error(c);
}

+/* Send message to an object which registered a handler. Result must be returned as string. */
+static void command_send_object_message(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
+ pa_native_connection *c = PA_NATIVE_CONNECTION(userdata);
+ const char *object_path = NULL;
+ const char *message = NULL;
+ const char *message_parameters = NULL;
+ const char *client_name;
+ char *response = NULL;
+ int ret;
+ pa_tagstruct *reply;
+
+ pa_native_connection_assert_ref(c);
+ pa_assert(t);
+
+ if (pa_tagstruct_gets(t, &object_path) < 0 ||
+ pa_tagstruct_gets(t, &message) < 0 ||
+ pa_tagstruct_gets(t, &message_parameters) < 0 ||
+ !pa_tagstruct_eof(t)) {
+ protocol_error(c);
+ return;
+ }
+
+ CHECK_VALIDITY(c->pstream, c->authorized, tag, PA_ERR_ACCESS);
+ CHECK_VALIDITY(c->pstream, object_path != NULL, tag, PA_ERR_INVALID);
+ CHECK_VALIDITY(c->pstream, pa_utf8_valid(object_path), tag, PA_ERR_INVALID);
+ CHECK_VALIDITY(c->pstream, message != NULL, tag, PA_ERR_INVALID);
+ CHECK_VALIDITY(c->pstream, pa_utf8_valid(message), tag, PA_ERR_INVALID);
+ if (message_parameters)
+ CHECK_VALIDITY(c->pstream, pa_utf8_valid(message_parameters), tag, PA_ERR_INVALID);
+
+ client_name = pa_strnull(pa_proplist_gets(c->client->proplist, PA_PROP_APPLICATION_PROCESS_BINARY));
+ pa_log_debug("Client %s sent message %s to path %s", client_name, message, object_path);
+ if (message_parameters)
+ pa_log_debug("Message parameters: %s", message_parameters);
+
+ ret = pa_message_handler_send_message(c->protocol->core, object_path, message, message_parameters, &response);
+
+ if (ret < 0) {
+ pa_pstream_send_error(c->pstream, tag, -ret);
+ return;
+ }
+
+ reply = reply_new(tag);
+ pa_tagstruct_puts(reply, response);
+ pa_xfree(response);
+
+ pa_pstream_send_tagstruct(c->pstream, reply);
+}
+
static void command_set_card_profile(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
pa_native_connection *c = PA_NATIVE_CONNECTION(userdata);
uint32_t idx = PA_INVALID_INDEX;
@@ -4936,6 +4986,8 @@ static const pa_pdispatch_cb_t command_table[PA_COMMAND_MAX] = {

[PA_COMMAND_REGISTER_MEMFD_SHMID] = command_register_memfd_shmid,

+ [PA_COMMAND_SEND_OBJECT_MESSAGE] = command_send_object_message,
+
[PA_COMMAND_EXTENSION] = command_extension
};
--
2.14.1
Tanu Kaskinen
2018-07-04 12:51:31 UTC
Permalink
Post by Georg Chini
This patch adds the PA_COMMAND_SEND_OBJECT_MESSAGE command to protocol-native
so that clients can use the messaging feature introduced in the previous patch.
Sending messages can in effect replace the extension system for modules. The
approach is more flexible than the extension interface because a generic string
format is used to exchange information. Furthermore the messaging system can be
used for any object, not only for modules, and is easier to implement than
extensions.
---
PROTOCOL | 14 +++++++++
configure.ac | 2 +-
src/map-file | 1 +
src/pulse/introspect.c | 64 +++++++++++++++++++++++++++++++++++++++++
src/pulse/introspect.h | 16 +++++++++++
src/pulsecore/native-common.h | 3 ++
src/pulsecore/pdispatch.c | 3 ++
src/pulsecore/protocol-native.c | 52 +++++++++++++++++++++++++++++++++
8 files changed, 154 insertions(+), 1 deletion(-)
Looks good to me.
--
Tanu

https://liberapay.com/tanuk
https://www.patreon.com/tanuk
Georg Chini
2018-04-09 17:35:22 UTC
Permalink
For better readability, "pactl list message-handlers" is introduced which
prints a formatted output of "pactl send-message /core list-handlers".

The patch also adds the functions pa_message_param_split_list() and
pa_message_param_read_string() for easy parsing of the message response
string. Because the function needs to modify the parameter string,
the message handler and the pa_context_string_callback function now
receive a char* instead of a const char* as parameter argument.
---
man/pactl.1.xml.in | 2 +-
shell-completion/bash/pulseaudio | 2 +-
shell-completion/zsh/_pulseaudio | 1 +
src/Makefile.am | 1 +
src/map-file | 2 +
src/pulse/introspect.c | 11 +++-
src/pulse/introspect.h | 2 +-
src/pulse/message-params.c | 116 +++++++++++++++++++++++++++++++++++++++
src/pulse/message-params.h | 41 ++++++++++++++
src/pulsecore/core.c | 2 +-
src/pulsecore/message-handler.c | 9 ++-
src/pulsecore/message-handler.h | 2 +-
src/utils/pactl.c | 65 +++++++++++++++++++++-
13 files changed, 245 insertions(+), 11 deletions(-)
create mode 100644 src/pulse/message-params.c
create mode 100644 src/pulse/message-params.h

diff --git a/man/pactl.1.xml.in b/man/pactl.1.xml.in
index 4052fae3..66c0bda9 100644
--- a/man/pactl.1.xml.in
+++ b/man/pactl.1.xml.in
@@ -76,7 +76,7 @@ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
<option>
<p><opt>list</opt> [<arg>short</arg>] [<arg>TYPE</arg>]</p>
<optdesc><p>Dump all currently loaded modules, available sinks, sources, streams, etc. <arg>TYPE</arg> must be one of:
- modules, sinks, sources, sink-inputs, source-outputs, clients, samples, cards. If not specified, all info is listed. If
+ modules, sinks, sources, sink-inputs, source-outputs, clients, samples, cards, message-handlers. If not specified, all info is listed. If
short is given, output is in a tabular format, for easy parsing by scripts.</p></optdesc>
</option>

diff --git a/shell-completion/bash/pulseaudio b/shell-completion/bash/pulseaudio
index 797ec067..24d2aa1c 100644
--- a/shell-completion/bash/pulseaudio
+++ b/shell-completion/bash/pulseaudio
@@ -113,7 +113,7 @@ _pactl() {
local comps
local flags='-h --help --version -s --server= --client-name='
local list_types='short sinks sources sink-inputs source-outputs cards
- modules samples clients'
+ modules samples clients message-handlers'
local commands=(stat info list exit upload-sample play-sample remove-sample
load-module unload-module move-sink-input move-source-output
suspend-sink suspend-source set-card-profile set-sink-port
diff --git a/shell-completion/zsh/_pulseaudio b/shell-completion/zsh/_pulseaudio
index a2817ebb..c24d0387 100644
--- a/shell-completion/zsh/_pulseaudio
+++ b/shell-completion/zsh/_pulseaudio
@@ -285,6 +285,7 @@ _pactl_completion() {
'clients: list connected clients'
'samples: list samples'
'cards: list available cards'
+ 'message-handlers: list available message-handlers'
)

if ((CURRENT == 2)); then
diff --git a/src/Makefile.am b/src/Makefile.am
index 27e5f875..ccdad8ff 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -896,6 +896,7 @@ libpulse_la_SOURCES = \
pulse/timeval.c pulse/timeval.h \
pulse/utf8.c pulse/utf8.h \
pulse/util.c pulse/util.h \
+ pulse/message-params.c pulse/message-params.h \
pulse/volume.c pulse/volume.h \
pulse/xmalloc.c pulse/xmalloc.h

diff --git a/src/map-file b/src/map-file
index 4d747f19..385731dc 100644
--- a/src/map-file
+++ b/src/map-file
@@ -225,6 +225,8 @@ pa_mainloop_quit;
pa_mainloop_run;
pa_mainloop_set_poll_func;
pa_mainloop_wakeup;
+pa_message_param_read_string;
+pa_message_param_split_list;
pa_msleep;
pa_operation_cancel;
pa_operation_get_state;
diff --git a/src/pulse/introspect.c b/src/pulse/introspect.c
index 76bfee41..23901eb3 100644
--- a/src/pulse/introspect.c
+++ b/src/pulse/introspect.c
@@ -2215,8 +2215,15 @@ static void context_string_callback(pa_pdispatch *pd, uint32_t command, uint32_t
response = "";

if (o->callback) {
- pa_context_string_cb_t cb = (pa_context_string_cb_t) o->callback;
- cb(o->context, success, response, o->userdata);
+ char *response_copy;
+ pa_context_string_cb_t cb;
+
+ response_copy = pa_xstrdup(response);
+
+ cb = (pa_context_string_cb_t) o->callback;
+ cb(o->context, success, response_copy, o->userdata);
+
+ pa_xfree(response_copy);
}

finish:
diff --git a/src/pulse/introspect.h b/src/pulse/introspect.h
index fbe5b668..bcec48da 100644
--- a/src/pulse/introspect.h
+++ b/src/pulse/introspect.h
@@ -449,7 +449,7 @@ pa_operation* pa_context_unload_module(pa_context *c, uint32_t idx, pa_context_s
/** @{ \name Messages */

/** Callback prototype for pa_context_send_message_to_object() */
-typedef void (*pa_context_string_cb_t)(pa_context *c, int success, const char *response, void *userdata);
+typedef void (*pa_context_string_cb_t)(pa_context *c, int success, char *response, void *userdata);

/** Send a message to an object that registered a message handler. For more information
* see https://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/doc/messaging_api.txt. */
diff --git a/src/pulse/message-params.c b/src/pulse/message-params.c
new file mode 100644
index 00000000..964e29d0
--- /dev/null
+++ b/src/pulse/message-params.c
@@ -0,0 +1,116 @@
+/***
+ This file is part of PulseAudio.
+
+ PulseAudio is free software; you can redistribute it and/or modify
+ it under the terms of the GNU Lesser General Public License as
+ published by the Free Software Foundation; either version 2.1 of the
+ License, or (at your option) any later version.
+
+ PulseAudio is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
+***/
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/types.h>
+
+#include <pulse/xmalloc.h>
+
+#include <pulsecore/macro.h>
+
+#include "message-params.h"
+
+/* Split the specified string into elements. An element is defined as
+ * a sub-string between curly braces. The function is needed to parse
+ * the parameters of messages. Each time it is called returns the position
+ * of the current element in result and the state pointer is advanced to
+ * the next list element.
+ * The variable state points to, should be initialized to NULL before
+ * the first call. The function returns 1 on success, 0 if end of string
+ * is encountered and -1 on parse error. */
+int pa_message_param_split_list(char *c, char **result, void **state) {
+ char *current = *state ? *state : c;
+ uint32_t open_braces;
+
+ pa_assert(result);
+
+ *result = NULL;
+
+ /* Empty or no string */
+ if (!current || *current == 0)
+ return 0;
+
+ /* Find opening brace */
+ while (*current != 0) {
+
+ if (*current == '{')
+ break;
+
+ /* unexpected closing brace, parse error */
+ if (*current == '}')
+ return -1;
+
+ current++;
+ }
+
+ /* No opening brace found, end of string */
+ if (*current == 0)
+ return 0;
+
+ *result = current + 1;
+ open_braces = 1;
+
+ while (open_braces != 0 && *current != 0) {
+ current++;
+ if (*current == '{')
+ open_braces++;
+ if (*current == '}')
+ open_braces--;
+ }
+
+ /* Parse error, closing brace missing */
+ if (open_braces != 0) {
+ *result = NULL;
+ return -1;
+ }
+
+ /* Replace } with 0 */
+ *current = 0;
+
+ *state = current + 1;
+
+ return 1;
+}
+
+/* Read a string from the parameter list. The state pointer is
+ * advanced to the next element of the list. If allocate is
+ * true, a newly allocated string will be returned, else a
+ * pointer to a sub-string within c. */
+int pa_message_param_read_string(char *c, char **result, bool allocate, void **state) {
+ char *start_pos;
+ int err;
+
+ pa_assert(result);
+
+ *result = NULL;
+
+ if ((err = pa_message_param_split_list(c, &start_pos, state)) != 1)
+ return err;
+
+ *result = start_pos;
+ if (allocate)
+ *result = pa_xstrdup(*result);
+
+ return err;
+}
diff --git a/src/pulse/message-params.h b/src/pulse/message-params.h
new file mode 100644
index 00000000..02e08363
--- /dev/null
+++ b/src/pulse/message-params.h
@@ -0,0 +1,41 @@
+#ifndef foomessagehelperhfoo
+#define foomessagehelperhfoo
+
+/***
+ This file is part of PulseAudio.
+
+ PulseAudio is free software; you can redistribute it and/or modify
+ it under the terms of the GNU Lesser General Public License as
+ published by the Free Software Foundation; either version 2.1 of the
+ License, or (at your option) any later version.
+
+ PulseAudio is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <stddef.h>
+#include <stdbool.h>
+#include <inttypes.h>
+
+#include <pulse/cdecl.h>
+#include <pulse/version.h>
+
+/** \file
+ * Utility functions for reading and writing message parameters */
+
+PA_C_DECL_BEGIN
+
+/** Split message parameter string into list elements */
+int pa_message_param_split_list(char *c, char **result, void **state);
+
+/** Read a string from the parameter list. */
+int pa_message_param_read_string(char *c, char **result, bool allocate, void **state);
+
+PA_C_DECL_END
+
+#endif
diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
index 10cd6f2f..e95657f0 100644
--- a/src/pulsecore/core.c
+++ b/src/pulsecore/core.c
@@ -86,7 +86,7 @@ static char *message_handler_list(pa_core *c) {
return pa_strbuf_to_string_free(buf);
}

-static int core_message_handler(const char *object_path, const char *message, const char *message_parameters, char **response, void *userdata) {
+static int core_message_handler(const char *object_path, const char *message, char *message_parameters, char **response, void *userdata) {
pa_core *c;

pa_assert(c = (pa_core *) userdata);
diff --git a/src/pulsecore/message-handler.c b/src/pulsecore/message-handler.c
index 18c62fc2..427186db 100644
--- a/src/pulsecore/message-handler.c
+++ b/src/pulsecore/message-handler.c
@@ -90,6 +90,8 @@ void pa_message_handler_unregister(pa_core *c, const char *object_path) {
/* Send a message to an object identified by object_path */
int pa_message_handler_send_message(pa_core *c, const char *object_path, const char *message, const char *message_parameters, char **response) {
struct pa_message_handler *handler;
+ int ret;
+ char *parameter_copy;

pa_assert(c);
pa_assert(object_path);
@@ -101,9 +103,14 @@ int pa_message_handler_send_message(pa_core *c, const char *object_path, const c
if (!(handler = pa_hashmap_get(c->message_handlers, object_path)))
return -PA_ERR_NOENTITY;

+ parameter_copy = pa_xstrdup(message_parameters);
+
/* The handler is expected to return an error code and may also
return an error string in response */
- return handler->callback(handler->object_path, message, message_parameters, response, handler->userdata);
+ ret = handler->callback(handler->object_path, message, parameter_copy, response, handler->userdata);
+
+ pa_xfree(parameter_copy);
+ return ret;
}

/* Set handler description */
diff --git a/src/pulsecore/message-handler.h b/src/pulsecore/message-handler.h
index be94510f..38b24e1b 100644
--- a/src/pulsecore/message-handler.h
+++ b/src/pulsecore/message-handler.h
@@ -26,7 +26,7 @@
typedef int (*pa_message_handler_cb_t)(
const char *object_path,
const char *message,
- const char *message_parameters,
+ char *message_parameters,
char **response,
void *userdata);

diff --git a/src/utils/pactl.c b/src/utils/pactl.c
index 66c0f251..fdafe57b 100644
--- a/src/utils/pactl.c
+++ b/src/utils/pactl.c
@@ -35,6 +35,7 @@
#include <sndfile.h>

#include <pulse/pulseaudio.h>
+#include <pulse/message-params.h>
#include <pulse/ext-device-restore.h>

#include <pulsecore/i18n.h>
@@ -838,7 +839,7 @@ static void index_callback(pa_context *c, uint32_t idx, void *userdata) {
complete_action();
}

-static void send_message_callback(pa_context *c, int success, const char *response, void *userdata) {
+static void send_message_callback(pa_context *c, int success, char *response, void *userdata) {

if (!success) {
pa_log(_("Send message failed: %s"), pa_strerror(pa_context_errno(c)));
@@ -851,6 +852,55 @@ static void send_message_callback(pa_context *c, int success, const char *respon
complete_action();
}

+static void list_handlers_callback(pa_context *c, int success, char *response, void *userdata) {
+ void *state = NULL;
+ char *param_list;
+ char *start_pos;
+ int err;
+
+ if (!success) {
+ pa_log(_("list-handlers message failed: %s"), pa_strerror(pa_context_errno(c)));
+ quit(1);
+ return;
+ }
+
+ if (pa_message_param_split_list(response, &param_list, &state) <= 0) {
+ pa_log(_("list-handlers message response could not be parsed correctly"));
+ quit(1);
+ return;
+ }
+
+ state = NULL;
+ while ((err = pa_message_param_split_list(param_list, &start_pos, &state)) > 0) {
+ void *state2 = NULL;
+ char *path;
+ char *description;
+
+ if (pa_message_param_read_string(start_pos, &path, false, &state2) <= 0) {
+ err = -1;
+ break;
+ }
+ if (pa_message_param_read_string(start_pos, &description, false, &state2) <= 0) {
+ err = -1;
+ break;
+ }
+
+ if (short_list_format)
+ printf("%s\n", path);
+ else
+ printf("Message Handler %s\n Description: %s\n", path, description);
+
+ }
+
+ if (err < 0) {
+ pa_log(_("list-handlers message response could not be parsed correctly"));
+ quit(1);
+ return;
+ }
+
+ complete_action();
+}
+
static void volume_relative_adjust(pa_cvolume *cv) {
pa_assert(volume_flags & VOL_RELATIVE);

@@ -1262,6 +1312,8 @@ static void context_state_callback(pa_context *c, void *userdata) {
o = pa_context_get_sample_info_list(c, get_sample_info_callback, NULL);
else if (pa_streq(list_type, "cards"))
o = pa_context_get_card_info_list(c, get_card_info_callback, NULL);
+ else if (pa_streq(list_type, "message-handlers"))
+ o = pa_context_send_message_to_object(c, "/core", "list-handlers", NULL, list_handlers_callback, NULL);
else
pa_assert_not_reached();
} else {
@@ -1312,6 +1364,12 @@ static void context_state_callback(pa_context *c, void *userdata) {
actions++;
}

+ o = pa_context_send_message_to_object(c, "/core", "list-handlers", NULL, list_handlers_callback, NULL);
+ if (o) {
+ pa_operation_unref(o);
+ actions++;
+ }
+
o = NULL;
}
break;
@@ -1698,12 +1756,13 @@ int main(int argc, char *argv[]) {
if (pa_streq(argv[i], "modules") || pa_streq(argv[i], "clients") ||
pa_streq(argv[i], "sinks") || pa_streq(argv[i], "sink-inputs") ||
pa_streq(argv[i], "sources") || pa_streq(argv[i], "source-outputs") ||
- pa_streq(argv[i], "samples") || pa_streq(argv[i], "cards")) {
+ pa_streq(argv[i], "samples") || pa_streq(argv[i], "cards") ||
+ pa_streq(argv[i], "message-handlers")) {
list_type = pa_xstrdup(argv[i]);
} else if (pa_streq(argv[i], "short")) {
short_list_format = true;
} else {
- pa_log(_("Specify nothing, or one of: %s"), "modules, sinks, sources, sink-inputs, source-outputs, clients, samples, cards");
+ pa_log(_("Specify nothing, or one of: %s"), "modules, sinks, sources, sink-inputs, source-outputs, clients, samples, cards, message-handlers");
goto quit;
}
}
--
2.14.1
Tanu Kaskinen
2018-07-15 11:56:54 UTC
Permalink
Post by Georg Chini
For better readability, "pactl list message-handlers" is introduced which
prints a formatted output of "pactl send-message /core list-handlers".
The patch also adds the functions pa_message_param_split_list() and
pa_message_param_read_string() for easy parsing of the message response
string. Because the function needs to modify the parameter string,
the message handler and the pa_context_string_callback function now
receive a char* instead of a const char* as parameter argument.
---
man/pactl.1.xml.in | 2 +-
shell-completion/bash/pulseaudio | 2 +-
shell-completion/zsh/_pulseaudio | 1 +
src/Makefile.am | 1 +
src/map-file | 2 +
src/pulse/introspect.c | 11 +++-
src/pulse/introspect.h | 2 +-
src/pulse/message-params.c | 116 +++++++++++++++++++++++++++++++++++++++
src/pulse/message-params.h | 41 ++++++++++++++
src/pulsecore/core.c | 2 +-
src/pulsecore/message-handler.c | 9 ++-
src/pulsecore/message-handler.h | 2 +-
src/utils/pactl.c | 65 +++++++++++++++++++++-
13 files changed, 245 insertions(+), 11 deletions(-)
create mode 100644 src/pulse/message-params.c
create mode 100644 src/pulse/message-params.h
diff --git a/man/pactl.1.xml.in b/man/pactl.1.xml.in
index 4052fae3..66c0bda9 100644
--- a/man/pactl.1.xml.in
+++ b/man/pactl.1.xml.in
@@ -76,7 +76,7 @@ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
<option>
<p><opt>list</opt> [<arg>short</arg>] [<arg>TYPE</arg>]</p>
- modules, sinks, sources, sink-inputs, source-outputs, clients, samples, cards. If not specified, all info is listed. If
+ modules, sinks, sources, sink-inputs, source-outputs, clients, samples, cards, message-handlers. If not specified, all info is listed. If
short is given, output is in a tabular format, for easy parsing by scripts.</p></optdesc>
</option>
diff --git a/shell-completion/bash/pulseaudio b/shell-completion/bash/pulseaudio
index 797ec067..24d2aa1c 100644
--- a/shell-completion/bash/pulseaudio
+++ b/shell-completion/bash/pulseaudio
@@ -113,7 +113,7 @@ _pactl() {
local comps
local flags='-h --help --version -s --server= --client-name='
local list_types='short sinks sources sink-inputs source-outputs cards
- modules samples clients'
+ modules samples clients message-handlers'
local commands=(stat info list exit upload-sample play-sample remove-sample
load-module unload-module move-sink-input move-source-output
suspend-sink suspend-source set-card-profile set-sink-port
diff --git a/shell-completion/zsh/_pulseaudio b/shell-completion/zsh/_pulseaudio
index a2817ebb..c24d0387 100644
--- a/shell-completion/zsh/_pulseaudio
+++ b/shell-completion/zsh/_pulseaudio
@@ -285,6 +285,7 @@ _pactl_completion() {
'clients: list connected clients'
'samples: list samples'
'cards: list available cards'
+ 'message-handlers: list available message-handlers'
)
if ((CURRENT == 2)); then
diff --git a/src/Makefile.am b/src/Makefile.am
index 27e5f875..ccdad8ff 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -896,6 +896,7 @@ libpulse_la_SOURCES = \
pulse/timeval.c pulse/timeval.h \
pulse/utf8.c pulse/utf8.h \
pulse/util.c pulse/util.h \
+ pulse/message-params.c pulse/message-params.h \
pulse/volume.c pulse/volume.h \
pulse/xmalloc.c pulse/xmalloc.h
diff --git a/src/map-file b/src/map-file
index 4d747f19..385731dc 100644
--- a/src/map-file
+++ b/src/map-file
@@ -225,6 +225,8 @@ pa_mainloop_quit;
pa_mainloop_run;
pa_mainloop_set_poll_func;
pa_mainloop_wakeup;
+pa_message_param_read_string;
+pa_message_param_split_list;
Let's use "pa_message_params" as the prefix to be consistent with the
file names (and plural makes more sense anyway).
Post by Georg Chini
pa_msleep;
pa_operation_cancel;
pa_operation_get_state;
diff --git a/src/pulse/introspect.c b/src/pulse/introspect.c
index 76bfee41..23901eb3 100644
--- a/src/pulse/introspect.c
+++ b/src/pulse/introspect.c
@@ -2215,8 +2215,15 @@ static void context_string_callback(pa_pdispatch *pd, uint32_t command, uint32_t
response = "";
if (o->callback) {
- pa_context_string_cb_t cb = (pa_context_string_cb_t) o->callback;
- cb(o->context, success, response, o->userdata);
+ char *response_copy;
+ pa_context_string_cb_t cb;
+
+ response_copy = pa_xstrdup(response);
+
+ cb = (pa_context_string_cb_t) o->callback;
+ cb(o->context, success, response_copy, o->userdata);
+
+ pa_xfree(response_copy);
}
diff --git a/src/pulse/introspect.h b/src/pulse/introspect.h
index fbe5b668..bcec48da 100644
--- a/src/pulse/introspect.h
+++ b/src/pulse/introspect.h
@@ -449,7 +449,7 @@ pa_operation* pa_context_unload_module(pa_context *c, uint32_t idx, pa_context_s
/** Callback prototype for pa_context_send_message_to_object() */
-typedef void (*pa_context_string_cb_t)(pa_context *c, int success, const char *response, void *userdata);
+typedef void (*pa_context_string_cb_t)(pa_context *c, int success, char *response, void *userdata);
It would be good to document that it's ok and even expected that the
callback modifies the response string with the pa_message_param_*
functions without copying the string first.
Post by Georg Chini
/** Send a message to an object that registered a message handler. For more information
* see https://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/doc/messaging_api.txt. */
diff --git a/src/pulse/message-params.c b/src/pulse/message-params.c
new file mode 100644
index 00000000..964e29d0
--- /dev/null
+++ b/src/pulse/message-params.c
@@ -0,0 +1,116 @@
+/***
+ This file is part of PulseAudio.
+
+ PulseAudio is free software; you can redistribute it and/or modify
+ it under the terms of the GNU Lesser General Public License as
+ published by the Free Software Foundation; either version 2.1 of the
+ License, or (at your option) any later version.
+
+ PulseAudio is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
+***/
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/types.h>
+
+#include <pulse/xmalloc.h>
+
+#include <pulsecore/macro.h>
+
+#include "message-params.h"
+
+/* Split the specified string into elements. An element is defined as
+ * a sub-string between curly braces. The function is needed to parse
+ * the parameters of messages. Each time it is called returns the position
The word "it" is missing before "returns".
Post by Georg Chini
+ * of the current element in result and the state pointer is advanced to
+ * the next list element.
+ * The variable state points to, should be initialized to NULL before
+ * the first call. The function returns 1 on success, 0 if end of string
+ * is encountered and -1 on parse error. */
This documentation belongs to the header file.

It should be mentioned that result is set to NULL on error and end of
string.
Post by Georg Chini
+int pa_message_param_split_list(char *c, char **result, void **state) {
+ char *current = *state ? *state : c;
+ uint32_t open_braces;
+
+ pa_assert(result);
+
+ *result = NULL;
+
+ /* Empty or no string */
+ if (!current || *current == 0)
+ return 0;
+
+ /* Find opening brace */
+ while (*current != 0) {
+
+ if (*current == '{')
+ break;
+
+ /* unexpected closing brace, parse error */
+ if (*current == '}')
+ return -1;
+
+ current++;
+ }
+
+ /* No opening brace found, end of string */
+ if (*current == 0)
+ return 0;
+
+ *result = current + 1;
+ open_braces = 1;
+
+ while (open_braces != 0 && *current != 0) {
+ current++;
+ if (*current == '{')
+ open_braces++;
+ if (*current == '}')
+ open_braces--;
+ }
+
+ /* Parse error, closing brace missing */
+ if (open_braces != 0) {
+ *result = NULL;
+ return -1;
+ }
+
+ /* Replace } with 0 */
+ *current = 0;
+
+ *state = current + 1;
+
+ return 1;
+}
+
+/* Read a string from the parameter list. The state pointer is
+ * advanced to the next element of the list. If allocate is
+ * true, a newly allocated string will be returned, else a
+ * pointer to a sub-string within c. */
The documentation belongs to the header.

Each of the reading functions should explain how the state variable is
to be used. If that's too much repetition, then explain the state
variable in a separate section and link to it from the function
documentation.

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.
Post by Georg Chini
+int pa_message_param_read_string(char *c, char **result, bool allocate, void **state) {
+ char *start_pos;
+ int err;
I'd use "r" or "ret" instead of "err", since two out of three possible
values are not errors.
Post by Georg Chini
+
+ pa_assert(result);
+
+ *result = NULL;
+
+ if ((err = pa_message_param_split_list(c, &start_pos, state)) != 1)
+ return err;
+
+ *result = start_pos;
+ if (allocate)
+ *result = pa_xstrdup(*result);
+
+ return err;
+}
diff --git a/src/pulse/message-params.h b/src/pulse/message-params.h
new file mode 100644
index 00000000..02e08363
--- /dev/null
+++ b/src/pulse/message-params.h
@@ -0,0 +1,41 @@
+#ifndef foomessagehelperhfoo
+#define foomessagehelperhfoo
+
+/***
+ This file is part of PulseAudio.
+
+ PulseAudio is free software; you can redistribute it and/or modify
+ it under the terms of the GNU Lesser General Public License as
+ published by the Free Software Foundation; either version 2.1 of the
+ License, or (at your option) any later version.
+
+ PulseAudio is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
+***/
+
+#include <stddef.h>
+#include <stdbool.h>
+#include <inttypes.h>
+
+#include <pulse/cdecl.h>
+#include <pulse/version.h>
+
+/** \file
+ * Utility functions for reading and writing message parameters */
+
+PA_C_DECL_BEGIN
+
+/** Split message parameter string into list elements */
+int pa_message_param_split_list(char *c, char **result, void **state);
+
+/** Read a string from the parameter list. */
+int pa_message_param_read_string(char *c, char **result, bool allocate, void **state);
\since tags are missing.
Post by Georg Chini
+
+PA_C_DECL_END
+
+#endif
diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
index 10cd6f2f..e95657f0 100644
--- a/src/pulsecore/core.c
+++ b/src/pulsecore/core.c
@@ -86,7 +86,7 @@ static char *message_handler_list(pa_core *c) {
return pa_strbuf_to_string_free(buf);
}
-static int core_message_handler(const char *object_path, const char *message, const char *message_parameters, char **response, void *userdata) {
+static int core_message_handler(const char *object_path, const char *message, char *message_parameters, char **response, void *userdata) {
pa_core *c;
pa_assert(c = (pa_core *) userdata);
diff --git a/src/pulsecore/message-handler.c b/src/pulsecore/message-handler.c
index 18c62fc2..427186db 100644
--- a/src/pulsecore/message-handler.c
+++ b/src/pulsecore/message-handler.c
@@ -90,6 +90,8 @@ void pa_message_handler_unregister(pa_core *c, const char *object_path) {
/* Send a message to an object identified by object_path */
int pa_message_handler_send_message(pa_core *c, const char *object_path, const char *message, const char *message_parameters, char **response) {
struct pa_message_handler *handler;
+ int ret;
+ char *parameter_copy;
pa_assert(c);
pa_assert(object_path);
@@ -101,9 +103,14 @@ int pa_message_handler_send_message(pa_core *c, const char *object_path, const c
if (!(handler = pa_hashmap_get(c->message_handlers, object_path)))
return -PA_ERR_NOENTITY;
+ parameter_copy = pa_xstrdup(message_parameters);
+
/* The handler is expected to return an error code and may also
return an error string in response */
- return handler->callback(handler->object_path, message, message_parameters, response, handler->userdata);
+ ret = handler->callback(handler->object_path, message, parameter_copy, response, handler->userdata);
+
+ pa_xfree(parameter_copy);
+ return ret;
}
/* Set handler description */
diff --git a/src/pulsecore/message-handler.h b/src/pulsecore/message-handler.h
index be94510f..38b24e1b 100644
--- a/src/pulsecore/message-handler.h
+++ b/src/pulsecore/message-handler.h
@@ -26,7 +26,7 @@
typedef int (*pa_message_handler_cb_t)(
const char *object_path,
const char *message,
- const char *message_parameters,
+ char *message_parameters,
char **response,
void *userdata);
diff --git a/src/utils/pactl.c b/src/utils/pactl.c
index 66c0f251..fdafe57b 100644
--- a/src/utils/pactl.c
+++ b/src/utils/pactl.c
@@ -35,6 +35,7 @@
#include <sndfile.h>
#include <pulse/pulseaudio.h>
+#include <pulse/message-params.h>
#include <pulse/ext-device-restore.h>
#include <pulsecore/i18n.h>
@@ -838,7 +839,7 @@ static void index_callback(pa_context *c, uint32_t idx, void *userdata) {
complete_action();
}
-static void send_message_callback(pa_context *c, int success, const char *response, void *userdata) {
+static void send_message_callback(pa_context *c, int success, char *response, void *userdata) {
if (!success) {
pa_log(_("Send message failed: %s"), pa_strerror(pa_context_errno(c)));
@@ -851,6 +852,55 @@ static void send_message_callback(pa_context *c, int success, const char *respon
complete_action();
}
+static void list_handlers_callback(pa_context *c, int success, char *response, void *userdata) {
+ void *state = NULL;
+ char *param_list;
I think this should be named "handler_list" or just "list".
"param_list" associates with the same thing as the response variable in
my mind.
Post by Georg Chini
+ char *start_pos;
I think "handler_start" would be a better name, because "start_pos" is
rather unclear about which start it's referring to (there is the
parameter list, the handler list and the list of handler attributes).
Post by Georg Chini
+ int err;
+
+ if (!success) {
+ pa_log(_("list-handlers message failed: %s"), pa_strerror(pa_context_errno(c)));
+ quit(1);
+ return;
+ }
+
+ if (pa_message_param_split_list(response, &param_list, &state) <= 0) {
+ pa_log(_("list-handlers message response could not be parsed correctly"));
+ quit(1);
+ return;
+ }
+
+ state = NULL;
+ while ((err = pa_message_param_split_list(param_list, &start_pos, &state)) > 0) {
+ void *state2 = NULL;
+ char *path;
+ char *description;
+
+ if (pa_message_param_read_string(start_pos, &path, false, &state2) <= 0) {
+ err = -1;
+ break;
+ }
+ if (pa_message_param_read_string(start_pos, &description, false, &state2) <= 0) {
+ err = -1;
+ break;
+ }
+
+ if (short_list_format)
+ printf("%s\n", path);
+ else
+ printf("Message Handler %s\n Description: %s\n", path, description);
There should be an empty line between each handler (when not using the
short format). See how the nl variable is used for this.
Post by Georg Chini
+
+ }
+
+ if (err < 0) {
+ pa_log(_("list-handlers message response could not be parsed correctly"));
+ quit(1);
+ return;
+ }
+
+ complete_action();
+}
+
static void volume_relative_adjust(pa_cvolume *cv) {
pa_assert(volume_flags & VOL_RELATIVE);
@@ -1262,6 +1312,8 @@ static void context_state_callback(pa_context *c, void *userdata) {
o = pa_context_get_sample_info_list(c, get_sample_info_callback, NULL);
else if (pa_streq(list_type, "cards"))
o = pa_context_get_card_info_list(c, get_card_info_callback, NULL);
+ else if (pa_streq(list_type, "message-handlers"))
+ o = pa_context_send_message_to_object(c, "/core", "list-handlers", NULL, list_handlers_callback, NULL);
else
pa_assert_not_reached();
} else {
@@ -1312,6 +1364,12 @@ static void context_state_callback(pa_context *c, void *userdata) {
actions++;
}
+ o = pa_context_send_message_to_object(c, "/core", "list-handlers", NULL, list_handlers_callback, NULL);
+ if (o) {
+ pa_operation_unref(o);
+ actions++;
+ }
+
I wonder if it's really a good idea to list the handlers in "pactl
list", since the handlers are a lower-level thing than all the other
objects, and there may be quite many of them in the future. (I have a
vague recollection of me complaining in the first version that you
didn't list the handlers, in which case I'm sorry for being
inconsistent.) We can always add the handler listing later if there
seems to be need for that, but we probably can't do the opposite, i.e.
remove the listing once it's added.

I'm ok with listing the handlers here, I just wanted to point out this
argument against the listing (and to be clear, when the listing is
explicitly requested with "pactl list message-handlers", that's
definitely good).
Post by Georg Chini
o = NULL;
}
break;
@@ -1698,12 +1756,13 @@ int main(int argc, char *argv[]) {
if (pa_streq(argv[i], "modules") || pa_streq(argv[i], "clients") ||
pa_streq(argv[i], "sinks") || pa_streq(argv[i], "sink-inputs") ||
pa_streq(argv[i], "sources") || pa_streq(argv[i], "source-outputs") ||
- pa_streq(argv[i], "samples") || pa_streq(argv[i], "cards")) {
+ pa_streq(argv[i], "samples") || pa_streq(argv[i], "cards") ||
+ pa_streq(argv[i], "message-handlers")) {
list_type = pa_xstrdup(argv[i]);
} else if (pa_streq(argv[i], "short")) {
short_list_format = true;
} else {
- pa_log(_("Specify nothing, or one of: %s"), "modules, sinks, sources, sink-inputs, source-outputs, clients, samples, cards");
+ pa_log(_("Specify nothing, or one of: %s"), "modules, sinks, sources, sink-inputs, source-outputs, clients, samples, cards, message-handlers");
goto quit;
}
}
--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
Georg Chini
2018-04-09 17:35:25 UTC
Permalink
---
doc/messaging_api.txt | 5 ++
src/map-file | 4 +
src/pulse/message-params.c | 202 +++++++++++++++++++++++++++++++++++++++++++++
src/pulse/message-params.h | 12 +++
4 files changed, 223 insertions(+)

diff --git a/doc/messaging_api.txt b/doc/messaging_api.txt
index d080b783..57a92f58 100644
--- a/doc/messaging_api.txt
+++ b/doc/messaging_api.txt
@@ -53,6 +53,11 @@ 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_double_array() - read an array of double from list
+pa_message_param_read_int64_array() - read an array of int64 from list
+pa_message_param_read_uint64_array() - read an array of uint64 from list
+pa_message_param_read_string_array() - read an array of strings from a list
+

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
diff --git a/src/map-file b/src/map-file
index ab8c21c6..88d892ef 100644
--- a/src/map-file
+++ b/src/map-file
@@ -230,9 +230,13 @@ pa_message_param_end_list;
pa_message_param_new;
pa_message_param_read_bool;
pa_message_param_read_double;
+pa_message_param_read_double_array;
pa_message_param_read_int64;
+pa_message_param_read_int64_array;
pa_message_param_read_string;
+pa_message_param_read_string_array;
pa_message_param_read_uint64;
+pa_message_param_read_uint64_array;
pa_message_param_split_list;
pa_message_param_to_string;
pa_message_param_write_bool;
diff --git a/src/pulse/message-params.c b/src/pulse/message-params.c
index 93972399..b9846863 100644
--- a/src/pulse/message-params.c
+++ b/src/pulse/message-params.c
@@ -38,6 +38,8 @@ struct pa_message_param {
pa_strbuf *buffer;
};

+/* Helper functions */
+
/* Remove escaping from a string */
static char *unescape(char *value) {
char *tmp;
@@ -62,6 +64,59 @@ static char *unescape(char *value) {
return value;
}

+/* Count number of top level elements in parameter list */
+static int count_elements(const char *c) {
+ const char *s;
+ uint32_t element_count;
+ bool found_element, found_backslash;
+ int open_braces;
+
+ if (!c || *c == 0)
+ return PA_PARAM_LIST_END;
+
+ element_count = 0;
+ open_braces = 0;
+ found_element = false;
+ found_backslash = false;
+ s = c;
+
+ /* Count elements in list */
+ while (*s != 0) {
+
+ /* Skip escaped curly braces. */
+ if (*s == '\\') {
+ found_backslash = true;
+ s++;
+ continue;
+ }
+
+ if (*s == '{' && !found_backslash) {
+ found_element = true;
+ open_braces++;
+ }
+ if (*s == '}' && !found_backslash)
+ open_braces--;
+
+ /* unexpected closing brace, parse error */
+ if (open_braces < 0)
+ return PA_PARAM_PARSE_ERROR;
+
+ if (open_braces == 0 && found_element) {
+ element_count++;
+ found_element = false;
+ }
+
+ found_backslash = false;
+ s++;
+ }
+
+ /* missing closing brace, parse error */
+ if (open_braces > 0)
+ return PA_PARAM_PARSE_ERROR;
+
+ return element_count;
+}
+
/* Read functions */

/* Split the specified string into elements. An element is defined as
@@ -300,6 +355,153 @@ int pa_message_param_read_bool(char *c, bool *result, void **state) {
return PA_PARAM_OK;
}

+/* Converts a parameter list to a string array. Escaping is removed
+ * from a string if the string does not contain a list. If allocate
+ * is true, new strings will be allocated, otherwise pointers to
+ * sub-strings within c will be returned. */
+int pa_message_param_read_string_array(char *c, char ***parameter_list, bool allocate) {
+ char *start_pos;
+ void *state = NULL;
+ uint32_t element_count, i;
+ bool is_unpacked;
+ int err;
+
+ pa_assert(parameter_list);
+
+ *parameter_list = NULL;
+
+ /* Count elements, return if no element was found or parse error. */
+ if ((element_count = count_elements(c)) <= 0)
+ return element_count;
+
+ /* Allocate array */
+ *parameter_list = pa_xmalloc0(element_count * sizeof(char *));
+
+ i = 0;
+ while ((err = pa_message_param_split_list(c, &start_pos, &is_unpacked, &state)) > 0) {
+ (*parameter_list)[i] = start_pos;
+ if (is_unpacked)
+ (*parameter_list)[i] = unescape(start_pos);
+
+ /* If new strings are allocated, they must be freed by the caller */
+ if (allocate)
+ (*parameter_list)[i] = pa_xstrdup((*parameter_list)[i]);
+
+ i++;
+ }
+
+ if (err < 0) {
+ if (allocate) {
+ for (i = 0; i < element_count; i++)
+ pa_xfree((*parameter_list)[i]);
+ }
+ pa_xfree(*parameter_list);
+ *parameter_list = NULL;
+ return PA_PARAM_PARSE_ERROR;
+ }
+
+ return element_count;
+}
+
+/* Converts a parameter list to a double array. */
+int pa_message_param_read_double_array(char *c, double **parameter_list) {
+ double value;
+ void *state = NULL;
+ uint32_t element_count, i;
+ int err;
+
+ pa_assert(parameter_list);
+
+ *parameter_list = NULL;
+
+ /* Count elements, return if no element was found or parse error. */
+ if ((element_count = count_elements(c)) <= 0)
+ return element_count;
+
+ /* Allocate array */
+ *parameter_list = pa_xmalloc(element_count * sizeof(double));
+
+ i = 0;
+ while ((err = pa_message_param_read_double(c, &value, &state)) > 0) {
+ (*parameter_list)[i] = value;
+ i++;
+ }
+
+ if (err < 0) {
+ pa_xfree(*parameter_list);
+ *parameter_list = NULL;
+ return PA_PARAM_PARSE_ERROR;
+ }
+
+ return element_count;
+}
+
+/* Converts a parameter list to an int64 array. */
+int pa_message_param_read_int64_array(char *c, int64_t **parameter_list) {
+ int64_t value;
+ void *state = NULL;
+ uint32_t element_count, i;
+ int err;
+
+ pa_assert(parameter_list);
+
+ *parameter_list = NULL;
+
+ /* Count elements, return if no element was found or parse error. */
+ if ((element_count = count_elements(c)) <= 0)
+ return element_count;
+
+ /* Allocate array */
+ *parameter_list = pa_xmalloc(element_count * sizeof(int64_t));
+
+ i = 0;
+ while ((err = pa_message_param_read_int64(c, &value, &state)) > 0) {
+ (*parameter_list)[i] = value;
+ i++;
+ }
+
+ if (err < 0) {
+ pa_xfree(*parameter_list);
+ *parameter_list = NULL;
+ return PA_PARAM_PARSE_ERROR;
+ }
+
+ return element_count;
+}
+
+/* Converts a parameter list to an uint64 array. */
+int pa_message_param_read_uint64_array(char *c, uint64_t **parameter_list) {
+ uint64_t value;
+ void *state = NULL;
+ uint32_t element_count, i;
+ int err;
+
+ pa_assert(parameter_list);
+
+ *parameter_list = NULL;
+
+ /* Count elements, return if no element was found or parse error. */
+ if ((element_count = count_elements(c)) <= 0)
+ return element_count;
+
+ /* Allocate array */
+ *parameter_list = pa_xmalloc(element_count * sizeof(uint64_t));
+
+ i = 0;
+ while ((err = pa_message_param_read_uint64(c, &value, &state)) > 0) {
+ (*parameter_list)[i] = value;
+ i++;
+ }
+
+ if (err < 0) {
+ pa_xfree(*parameter_list);
+ *parameter_list = NULL;
+ return PA_PARAM_PARSE_ERROR;
+ }
+
+ return element_count;
+}
+
/* Write functions. The functions are wrapper functions around pa_strbuf,
* so that the client does not need to use pa_strbuf directly. */

diff --git a/src/pulse/message-params.h b/src/pulse/message-params.h
index f865e236..3ab1e418 100644
--- a/src/pulse/message-params.h
+++ b/src/pulse/message-params.h
@@ -60,6 +60,18 @@ 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);

+/** Convert message parameter list to string array */
+int pa_message_param_read_string_array(char *c, char ***parameter_list, bool allocate);
+
+/** Convert message parameter list to double array */
+int pa_message_param_read_double_array(char *c, double **parameter_list);
+
+/** Convert message parameter list to int64 array */
+int pa_message_param_read_int64_array(char *c, int64_t **parameter_list);
+
+/** Convert message parameter list to uint64 array */
+int pa_message_param_read_uint64_array(char *c, uint64_t **parameter_list);
+
/** Write functions */

/** Create a new pa_message_param structure */
--
2.14.1
Tanu Kaskinen
2018-08-11 09:37:54 UTC
Permalink
Post by Georg Chini
---
doc/messaging_api.txt | 5 ++
src/map-file | 4 +
src/pulse/message-params.c | 202 +++++++++++++++++++++++++++++++++++++++++++++
src/pulse/message-params.h | 12 +++
4 files changed, 223 insertions(+)
diff --git a/doc/messaging_api.txt b/doc/messaging_api.txt
index d080b783..57a92f58 100644
--- a/doc/messaging_api.txt
+++ b/doc/messaging_api.txt
@@ -53,6 +53,11 @@ 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_double_array() - read an array of double from list
+pa_message_param_read_int64_array() - read an array of int64 from list
+pa_message_param_read_uint64_array() - read an array of uint64 from list
+pa_message_param_read_string_array() - read an array of strings from a list
+
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
diff --git a/src/map-file b/src/map-file
index ab8c21c6..88d892ef 100644
--- a/src/map-file
+++ b/src/map-file
@@ -230,9 +230,13 @@ pa_message_param_end_list;
pa_message_param_new;
pa_message_param_read_bool;
pa_message_param_read_double;
+pa_message_param_read_double_array;
pa_message_param_read_int64;
+pa_message_param_read_int64_array;
pa_message_param_read_string;
+pa_message_param_read_string_array;
pa_message_param_read_uint64;
+pa_message_param_read_uint64_array;
pa_message_param_split_list;
pa_message_param_to_string;
pa_message_param_write_bool;
diff --git a/src/pulse/message-params.c b/src/pulse/message-params.c
index 93972399..b9846863 100644
--- a/src/pulse/message-params.c
+++ b/src/pulse/message-params.c
@@ -38,6 +38,8 @@ struct pa_message_param {
pa_strbuf *buffer;
};
+/* Helper functions */
+
/* Remove escaping from a string */
static char *unescape(char *value) {
char *tmp;
@@ -62,6 +64,59 @@ static char *unescape(char *value) {
return value;
}
+/* Count number of top level elements in parameter list */
+static int count_elements(const char *c) {
+ const char *s;
+ uint32_t element_count;
+ bool found_element, found_backslash;
+ int open_braces;
+
+ if (!c || *c == 0)
+ return PA_PARAM_LIST_END;
+
+ element_count = 0;
+ open_braces = 0;
+ found_element = false;
+ found_backslash = false;
+ s = c;
+
+ /* Count elements in list */
+ while (*s != 0) {
+
+ /* Skip escaped curly braces. */
+ if (*s == '\\') {
+ found_backslash = true;
+ s++;
+ continue;
+ }
+
+ if (*s == '{' && !found_backslash) {
+ found_element = true;
+ open_braces++;
+ }
+ if (*s == '}' && !found_backslash)
+ open_braces--;
+
+ /* unexpected closing brace, parse error */
+ if (open_braces < 0)
+ return PA_PARAM_PARSE_ERROR;
+
+ if (open_braces == 0 && found_element) {
+ element_count++;
+ found_element = false;
+ }
+
+ found_backslash = false;
+ s++;
+ }
+
+ /* missing closing brace, parse error */
+ if (open_braces > 0)
+ return PA_PARAM_PARSE_ERROR;
+
+ return element_count;
+}
+
/* Read functions */
/* Split the specified string into elements. An element is defined as
@@ -300,6 +355,153 @@ int pa_message_param_read_bool(char *c, bool *result, void **state) {
return PA_PARAM_OK;
}
+/* Converts a parameter list to a string array. Escaping is removed
+ * from a string if the string does not contain a list. If allocate
+ * is true, new strings will be allocated, otherwise pointers to
+ * sub-strings within c will be returned. */
+int pa_message_param_read_string_array(char *c, char ***parameter_list, bool allocate) {
+ char *start_pos;
+ void *state = NULL;
+ uint32_t element_count, i;
+ bool is_unpacked;
+ int err;
+
+ pa_assert(parameter_list);
+
+ *parameter_list = NULL;
The return value should be set only on success (see the patch 7 review
for more elaborate explanation). This comment applies to the other
functions as well.
Post by Georg Chini
+
+ /* Count elements, return if no element was found or parse error. */
+ if ((element_count = count_elements(c)) <= 0)
+ return element_count;
+
+ /* Allocate array */
+ *parameter_list = pa_xmalloc0(element_count * sizeof(char *));
+
+ i = 0;
+ while ((err = pa_message_param_split_list(c, &start_pos, &is_unpacked, &state)) > 0) {
+ (*parameter_list)[i] = start_pos;
+ if (is_unpacked)
+ (*parameter_list)[i] = unescape(start_pos);
+
+ /* If new strings are allocated, they must be freed by the caller */
+ if (allocate)
+ (*parameter_list)[i] = pa_xstrdup((*parameter_list)[i]);
+
+ i++;
+ }
I believe pa_message_params_read_string() can be used, and that should
result in simpler code.
Post by Georg Chini
+
+ if (err < 0) {
+ if (allocate) {
+ for (i = 0; i < element_count; i++)
+ pa_xfree((*parameter_list)[i]);
+ }
+ pa_xfree(*parameter_list);
+ *parameter_list = NULL;
+ return PA_PARAM_PARSE_ERROR;
+ }
+
+ return element_count;
+}
+
+/* Converts a parameter list to a double array. */
+int pa_message_param_read_double_array(char *c, double **parameter_list) {
+ double value;
+ void *state = NULL;
+ uint32_t element_count, i;
+ int err;
+
+ pa_assert(parameter_list);
+
+ *parameter_list = NULL;
+
+ /* Count elements, return if no element was found or parse error. */
+ if ((element_count = count_elements(c)) <= 0)
+ return element_count;
+
+ /* Allocate array */
+ *parameter_list = pa_xmalloc(element_count * sizeof(double));
+
+ i = 0;
+ while ((err = pa_message_param_read_double(c, &value, &state)) > 0) {
+ (*parameter_list)[i] = value;
+ i++;
+ }
The value variable is not needed, and maybe a for loop would be a
better fit. Maybe you did things this way because you think this is
more readable? I don't mind very much, so keep it as it is if you
prefer.

The loop could be rewritten like this:

for (i = 0; (err = pa_message_param_read_double(c, &values[i], &state)) > 0; i++)
;

I replaced &(*parameter_list)[i] with &values[i], because we should
modify parameter_list only on success, so here values is a local double
array variable.

This comment applies to the other functions as well.
Post by Georg Chini
+
+ if (err < 0) {
+ pa_xfree(*parameter_list);
+ *parameter_list = NULL;
+ return PA_PARAM_PARSE_ERROR;
+ }
+
+ return element_count;
+}
+
+/* Converts a parameter list to an int64 array. */
+int pa_message_param_read_int64_array(char *c, int64_t **parameter_list) {
+ int64_t value;
+ void *state = NULL;
+ uint32_t element_count, i;
+ int err;
+
+ pa_assert(parameter_list);
+
+ *parameter_list = NULL;
+
+ /* Count elements, return if no element was found or parse error. */
+ if ((element_count = count_elements(c)) <= 0)
+ return element_count;
+
+ /* Allocate array */
+ *parameter_list = pa_xmalloc(element_count * sizeof(int64_t));
+
+ i = 0;
+ while ((err = pa_message_param_read_int64(c, &value, &state)) > 0) {
+ (*parameter_list)[i] = value;
+ i++;
+ }
+
+ if (err < 0) {
+ pa_xfree(*parameter_list);
+ *parameter_list = NULL;
+ return PA_PARAM_PARSE_ERROR;
+ }
+
+ return element_count;
+}
+
+/* Converts a parameter list to an uint64 array. */
+int pa_message_param_read_uint64_array(char *c, uint64_t **parameter_list) {
+ uint64_t value;
+ void *state = NULL;
+ uint32_t element_count, i;
+ int err;
+
+ pa_assert(parameter_list);
+
+ *parameter_list = NULL;
+
+ /* Count elements, return if no element was found or parse error. */
+ if ((element_count = count_elements(c)) <= 0)
+ return element_count;
+
+ /* Allocate array */
+ *parameter_list = pa_xmalloc(element_count * sizeof(uint64_t));
+
+ i = 0;
+ while ((err = pa_message_param_read_uint64(c, &value, &state)) > 0) {
+ (*parameter_list)[i] = value;
+ i++;
+ }
+
+ if (err < 0) {
+ pa_xfree(*parameter_list);
+ *parameter_list = NULL;
+ return PA_PARAM_PARSE_ERROR;
+ }
+
+ return element_count;
+}
+
/* Write functions. The functions are wrapper functions around pa_strbuf,
* so that the client does not need to use pa_strbuf directly. */
diff --git a/src/pulse/message-params.h b/src/pulse/message-params.h
index f865e236..3ab1e418 100644
--- a/src/pulse/message-params.h
+++ b/src/pulse/message-params.h
@@ -60,6 +60,18 @@ 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);
+/** Convert message parameter list to string array */
+int pa_message_param_read_string_array(char *c, char ***parameter_list, bool allocate);
+
+/** Convert message parameter list to double array */
+int pa_message_param_read_double_array(char *c, double **parameter_list);
+
+/** Convert message parameter list to int64 array */
+int pa_message_param_read_int64_array(char *c, int64_t **parameter_list);
+
+/** Convert message parameter list to uint64 array */
+int pa_message_param_read_uint64_array(char *c, uint64_t **parameter_list);
+
/** Write functions */
/** Create a new pa_message_param structure */
--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
Georg Chini
2018-08-15 19:31:24 UTC
Permalink
Post by Tanu Kaskinen
Post by Georg Chini
---
doc/messaging_api.txt | 5 ++
src/map-file | 4 +
src/pulse/message-params.c | 202 +++++++++++++++++++++++++++++++++++++++++++++
src/pulse/message-params.h | 12 +++
4 files changed, 223 insertions(+)
diff --git a/doc/messaging_api.txt b/doc/messaging_api.txt
index d080b783..57a92f58 100644
--- a/doc/messaging_api.txt
+++ b/doc/messaging_api.txt
@@ -53,6 +53,11 @@ 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_double_array() - read an array of double from list
+pa_message_param_read_int64_array() - read an array of int64 from list
+pa_message_param_read_uint64_array() - read an array of uint64 from list
+pa_message_param_read_string_array() - read an array of strings from a list
+
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
diff --git a/src/map-file b/src/map-file
index ab8c21c6..88d892ef 100644
--- a/src/map-file
+++ b/src/map-file
@@ -230,9 +230,13 @@ pa_message_param_end_list;
pa_message_param_new;
pa_message_param_read_bool;
pa_message_param_read_double;
+pa_message_param_read_double_array;
pa_message_param_read_int64;
+pa_message_param_read_int64_array;
pa_message_param_read_string;
+pa_message_param_read_string_array;
pa_message_param_read_uint64;
+pa_message_param_read_uint64_array;
pa_message_param_split_list;
pa_message_param_to_string;
pa_message_param_write_bool;
diff --git a/src/pulse/message-params.c b/src/pulse/message-params.c
index 93972399..b9846863 100644
--- a/src/pulse/message-params.c
+++ b/src/pulse/message-params.c
@@ -38,6 +38,8 @@ struct pa_message_param {
pa_strbuf *buffer;
};
+/* Helper functions */
+
/* Remove escaping from a string */
static char *unescape(char *value) {
char *tmp;
@@ -62,6 +64,59 @@ static char *unescape(char *value) {
return value;
}
+/* Count number of top level elements in parameter list */
+static int count_elements(const char *c) {
+ const char *s;
+ uint32_t element_count;
+ bool found_element, found_backslash;
+ int open_braces;
+
+ if (!c || *c == 0)
+ return PA_PARAM_LIST_END;
+
+ element_count = 0;
+ open_braces = 0;
+ found_element = false;
+ found_backslash = false;
+ s = c;
+
+ /* Count elements in list */
+ while (*s != 0) {
+
+ /* Skip escaped curly braces. */
+ if (*s == '\\') {
+ found_backslash = true;
+ s++;
+ continue;
+ }
+
+ if (*s == '{' && !found_backslash) {
+ found_element = true;
+ open_braces++;
+ }
+ if (*s == '}' && !found_backslash)
+ open_braces--;
+
+ /* unexpected closing brace, parse error */
+ if (open_braces < 0)
+ return PA_PARAM_PARSE_ERROR;
+
+ if (open_braces == 0 && found_element) {
+ element_count++;
+ found_element = false;
+ }
+
+ found_backslash = false;
+ s++;
+ }
+
+ /* missing closing brace, parse error */
+ if (open_braces > 0)
+ return PA_PARAM_PARSE_ERROR;
+
+ return element_count;
+}
+
/* Read functions */
/* Split the specified string into elements. An element is defined as
@@ -300,6 +355,153 @@ int pa_message_param_read_bool(char *c, bool *result, void **state) {
return PA_PARAM_OK;
}
+/* Converts a parameter list to a string array. Escaping is removed
+ * from a string if the string does not contain a list. If allocate
+ * is true, new strings will be allocated, otherwise pointers to
+ * sub-strings within c will be returned. */
+int pa_message_param_read_string_array(char *c, char ***parameter_list, bool allocate) {
+ char *start_pos;
+ void *state = NULL;
+ uint32_t element_count, i;
+ bool is_unpacked;
+ int err;
+
+ pa_assert(parameter_list);
+
+ *parameter_list = NULL;
The return value should be set only on success (see the patch 7 review
for more elaborate explanation). This comment applies to the other
functions as well.
Post by Georg Chini
+
+ /* Count elements, return if no element was found or parse error. */
+ if ((element_count = count_elements(c)) <= 0)
+ return element_count;
+
+ /* Allocate array */
+ *parameter_list = pa_xmalloc0(element_count * sizeof(char *));
+
+ i = 0;
+ while ((err = pa_message_param_split_list(c, &start_pos, &is_unpacked, &state)) > 0) {
+ (*parameter_list)[i] = start_pos;
+ if (is_unpacked)
+ (*parameter_list)[i] = unescape(start_pos);
+
+ /* If new strings are allocated, they must be freed by the caller */
+ if (allocate)
+ (*parameter_list)[i] = pa_xstrdup((*parameter_list)[i]);
+
+ i++;
+ }
I believe pa_message_params_read_string() can be used, and that should
result in simpler code.
Post by Georg Chini
+
+ if (err < 0) {
+ if (allocate) {
+ for (i = 0; i < element_count; i++)
+ pa_xfree((*parameter_list)[i]);
+ }
+ pa_xfree(*parameter_list);
+ *parameter_list = NULL;
+ return PA_PARAM_PARSE_ERROR;
+ }
+
+ return element_count;
+}
+
+/* Converts a parameter list to a double array. */
+int pa_message_param_read_double_array(char *c, double **parameter_list) {
+ double value;
+ void *state = NULL;
+ uint32_t element_count, i;
+ int err;
+
+ pa_assert(parameter_list);
+
+ *parameter_list = NULL;
+
+ /* Count elements, return if no element was found or parse error. */
+ if ((element_count = count_elements(c)) <= 0)
+ return element_count;
+
+ /* Allocate array */
+ *parameter_list = pa_xmalloc(element_count * sizeof(double));
+
+ i = 0;
+ while ((err = pa_message_param_read_double(c, &value, &state)) > 0) {
+ (*parameter_list)[i] = value;
+ i++;
+ }
The value variable is not needed, and maybe a for loop would be a
better fit. Maybe you did things this way because you think this is
more readable? I don't mind very much, so keep it as it is if you
prefer.
for (i = 0; (err = pa_message_param_read_double(c, &values[i], &state)) > 0; i++)
;
I replaced &(*parameter_list)[i] with &values[i], because we should
modify parameter_list only on success, so here values is a local double
array variable.
This comment applies to the other functions as well.
Post by Georg Chini
+
+ if (err < 0) {
+ pa_xfree(*parameter_list);
+ *parameter_list = NULL;
+ return PA_PARAM_PARSE_ERROR;
+ }
+
+ return element_count;
+}
+
+/* Converts a parameter list to an int64 array. */
+int pa_message_param_read_int64_array(char *c, int64_t **parameter_list) {
+ int64_t value;
+ void *state = NULL;
+ uint32_t element_count, i;
+ int err;
+
+ pa_assert(parameter_list);
+
+ *parameter_list = NULL;
+
+ /* Count elements, return if no element was found or parse error. */
+ if ((element_count = count_elements(c)) <= 0)
+ return element_count;
+
+ /* Allocate array */
+ *parameter_list = pa_xmalloc(element_count * sizeof(int64_t));
+
+ i = 0;
+ while ((err = pa_message_param_read_int64(c, &value, &state)) > 0) {
+ (*parameter_list)[i] = value;
+ i++;
+ }
+
+ if (err < 0) {
+ pa_xfree(*parameter_list);
+ *parameter_list = NULL;
+ return PA_PARAM_PARSE_ERROR;
+ }
+
+ return element_count;
+}
+
+/* Converts a parameter list to an uint64 array. */
+int pa_message_param_read_uint64_array(char *c, uint64_t **parameter_list) {
+ uint64_t value;
+ void *state = NULL;
+ uint32_t element_count, i;
+ int err;
+
+ pa_assert(parameter_list);
+
+ *parameter_list = NULL;
+
+ /* Count elements, return if no element was found or parse error. */
+ if ((element_count = count_elements(c)) <= 0)
+ return element_count;
+
+ /* Allocate array */
+ *parameter_list = pa_xmalloc(element_count * sizeof(uint64_t));
+
+ i = 0;
+ while ((err = pa_message_param_read_uint64(c, &value, &state)) > 0) {
+ (*parameter_list)[i] = value;
+ i++;
+ }
+
+ if (err < 0) {
+ pa_xfree(*parameter_list);
+ *parameter_list = NULL;
+ return PA_PARAM_PARSE_ERROR;
+ }
+
+ return element_count;
+}
+
/* Write functions. The functions are wrapper functions around pa_strbuf,
* so that the client does not need to use pa_strbuf directly. */
diff --git a/src/pulse/message-params.h b/src/pulse/message-params.h
index f865e236..3ab1e418 100644
--- a/src/pulse/message-params.h
+++ b/src/pulse/message-params.h
@@ -60,6 +60,18 @@ 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);
+/** Convert message parameter list to string array */
+int pa_message_param_read_string_array(char *c, char ***parameter_list, bool allocate);
+
+/** Convert message parameter list to double array */
+int pa_message_param_read_double_array(char *c, double **parameter_list);
+
+/** Convert message parameter list to int64 array */
+int pa_message_param_read_int64_array(char *c, int64_t **parameter_list);
+
+/** Convert message parameter list to uint64 array */
+int pa_message_param_read_uint64_array(char *c, uint64_t **parameter_list);
+
/** Write functions */
/** Create a new pa_message_param structure */
Thank you for your reviews. It will probably take until October before I can
send a new series.
Tanu Kaskinen
2018-08-16 17:50:52 UTC
Permalink
Post by Georg Chini
Thank you for your reviews. It will probably take until October before I can
send a new series.
OK, no problem!
--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
Georg Chini
2018-04-09 17:35:24 UTC
Permalink
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;
+
+ 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;
+
+ 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);
+
+ /* 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;
+
+ 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);
+}
+
+/* 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);
+}
+
+/* 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);
+}
+
+/* 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,
+};
+
/** 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);
+
/** 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);
+
PA_C_DECL_END

#endif
--
2.14.1
Tanu Kaskinen
2018-08-06 14:23:09 UTC
Permalink
Post by Georg Chini
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.
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.
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.
Post by Georg Chini
+
+ 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.
Post by Georg Chini
+ 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.
Post by Georg Chini
+
+ /* 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.
Post by Georg Chini
+
+ 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).
Post by Georg Chini
+}
+
+/* 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.
Post by Georg Chini
+}
+
+/* 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.
Post by Georg Chini
+}
+
+/* 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.
Post by Georg Chini
+
/** 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.
Post by Georg Chini
+
/** 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.
Post by Georg Chini
+
PA_C_DECL_END
#endif
--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
Georg Chini
2018-04-09 17:35:23 UTC
Permalink
The patch adds the possibility to escape curly braces within parameter strings
and introduces several new functions that can be used for writing parameters.

For writing, the structure pa_message_param, which is a wrapper for pa_strbuf
has been created. Following new write functions are available:

pa_message_param_new() - creates a new pa_message_param structure
pa_message_param_to_string() - converts a pa_message_param to string and frees
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

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
falsely escape the closing bracket. To avoid this, single quotes must be added
at start and end of the string. The function pa_message_param_write_string()
has a parameter do_escape. If true, the necessary escaping is added. Escaping
is only needed if a string might fulfill one of the following conditions:

- It contains curly braces
- It contains a trailing "\"
- It is enclosed in single quotes

Other strings can be passed without modification.

For reading, pa_message_param_read_string() reverts the changes that
pa_message_param_write_string() might have introduced.

The patch also adds more restrictions on the object path name. Now only
alphanumeric characters and one of "_", ".", "-" and "/" are allowed.
The path name may not end with a /. If the user specifies a trailing / when
sending a message, it will be removed.
---
doc/messaging_api.txt | 34 +++++++-
src/Makefile.am | 3 +-
src/map-file | 5 ++
src/pulse/message-params.c | 181 ++++++++++++++++++++++++++++++++++++++--
src/pulse/message-params.h | 23 ++++-
src/pulsecore/core.c | 20 ++---
src/pulsecore/message-handler.c | 53 +++++++-----
src/utils/pactl.c | 4 +-
8 files changed, 279 insertions(+), 44 deletions(-)

diff --git a/doc/messaging_api.txt b/doc/messaging_api.txt
index 431a5df2..0e6be53f 100644
--- a/doc/messaging_api.txt
+++ b/doc/messaging_api.txt
@@ -14,10 +14,42 @@ look like that:
{{Integer} {{1st float} {2nd float} ...}}{...}
Any characters that are not enclosed in curly braces are ignored (all characters
between { and {, between } and } and between } and {). The same syntax is used
-to specify message parameters. The following reference lists available messages,
+to specify message parameters. The reference further down lists available messages,
their parameters and return values. If a return value is enclosed in {}, this
means that multiple elements of the same type may be returned.

+There are several functions that simplify reading and writing message parameter
+strings. For writing, the structure pa_message_param can be used. Following
+functions are available:
+pa_message_param_new() - creates a new pa_message_param structure
+pa_message_param_to_string() - converts a pa_message_param to string and frees
+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
+
+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
+falsely escape the closing bracket. To avoid this, single quotes must be added
+at start and end of the string. The function pa_message_param_write_string()
+has a parameter do_escape. If true, the necessary escaping is added. Escaping
+is only needed if a string might fulfill one of the following conditions:
+
+- It contains curly braces
+- It contains a trailing "\"
+- It is enclosed in single quotes
+
+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_string() also reverts the changes that
+pa_message_param_write_string() might have introduced.
+
+Reference:
+
Object path: /core
Message: list-handlers
Parameters: None
diff --git a/src/Makefile.am b/src/Makefile.am
index ccdad8ff..72d8cf22 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -675,6 +675,7 @@ ***@PA_MAJORMINOR@_la_SOURCES = \
pulse/timeval.c pulse/timeval.h \
pulse/rtclock.c pulse/rtclock.h \
pulse/volume.c pulse/volume.h \
+ pulse/message-params.c pulse/message-params.h \
pulsecore/atomic.h \
pulsecore/authkey.c pulsecore/authkey.h \
pulsecore/conf-parser.c pulsecore/conf-parser.h \
@@ -884,6 +885,7 @@ libpulse_la_SOURCES = \
pulse/mainloop-api.c pulse/mainloop-api.h \
pulse/mainloop-signal.c pulse/mainloop-signal.h \
pulse/mainloop.c pulse/mainloop.h \
+ pulse/message-params.c pulse/message-params.h \
pulse/operation.c pulse/operation.h \
pulse/proplist.c pulse/proplist.h \
pulse/pulseaudio.h \
@@ -896,7 +898,6 @@ libpulse_la_SOURCES = \
pulse/timeval.c pulse/timeval.h \
pulse/utf8.c pulse/utf8.h \
pulse/util.c pulse/util.h \
- pulse/message-params.c pulse/message-params.h \
pulse/volume.c pulse/volume.h \
pulse/xmalloc.c pulse/xmalloc.h

diff --git a/src/map-file b/src/map-file
index 385731dc..372d190d 100644
--- a/src/map-file
+++ b/src/map-file
@@ -225,8 +225,13 @@ pa_mainloop_quit;
pa_mainloop_run;
pa_mainloop_set_poll_func;
pa_mainloop_wakeup;
+pa_message_param_begin_list;
+pa_message_param_end_list;
+pa_message_param_new;
pa_message_param_read_string;
pa_message_param_split_list;
+pa_message_param_to_string;
+pa_message_param_write_string;
pa_msleep;
pa_operation_cancel;
pa_operation_get_state;
diff --git a/src/pulse/message-params.c b/src/pulse/message-params.c
index 964e29d0..d68ec59d 100644
--- a/src/pulse/message-params.c
+++ b/src/pulse/message-params.c
@@ -28,20 +28,55 @@
#include <pulse/xmalloc.h>

#include <pulsecore/macro.h>
+#include <pulsecore/strbuf.h>

#include "message-params.h"

+/* Message parameter structure, a wrapper for pa_strbuf */
+struct pa_message_param {
+ pa_strbuf *buffer;
+};
+
+/* Remove escaping from a string */
+static char *unescape(char *value) {
+ char *tmp;
+
+ if (!value)
+ return NULL;
+
+ /* If the string is enclosed in single quotes, remove them */
+ if (*value == '\'' && value[strlen(value) - 1] == '\'') {
+
+ memmove(value, value + 1, strlen(value));
+ value[strlen(value) - 1] = 0;
+ }
+
+ /* Remove escape character from curly braces if present. */
+ while ((tmp = strstr(value, "\\{")))
+ memmove(tmp, tmp + 1, strlen(value) - (size_t)(tmp - value));
+ while ((tmp = strstr(value, "\\}")))
+ memmove(tmp, tmp + 1, strlen(value) - (size_t)(tmp - value));
+
+ /* Return the pointer that was passed in. */
+ return value;
+}
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
* The variable state points to, should be initialized to NULL before
* the first call. The function returns 1 on success, 0 if end of string
* is encountered and -1 on parse error. */
-int pa_message_param_split_list(char *c, char **result, void **state) {
+int pa_message_param_split_list(char *c, char **result, bool *is_unpacked, void **state) {
char *current = *state ? *state : c;
uint32_t open_braces;
+ bool found_backslash = false;

pa_assert(result);

@@ -54,29 +89,52 @@ int pa_message_param_split_list(char *c, char **result, void **state) {
/* Find opening brace */
while (*current != 0) {

- if (*current == '{')
+ /* Skip escaped curly braces. */
+ if (*current == '\\') {
+ found_backslash = true;
+ current++;
+ continue;
+ }
+
+ if (*current == '{' && !found_backslash)
break;

/* unexpected closing brace, parse error */
- if (*current == '}')
+ if (*current == '}' && !found_backslash)
return -1;

+ found_backslash = false;
current++;
}

/* No opening brace found, end of string */
if (*current == 0)
- return 0;
+ return 0;

+ if (is_unpacked)
+ *is_unpacked = true;
*result = current + 1;
+ found_backslash = false;
open_braces = 1;

while (open_braces != 0 && *current != 0) {
current++;
- if (*current == '{')
+
+ /* Skip escaped curly braces. */
+ if (*current == '\\') {
+ found_backslash = true;
+ continue;
+ }
+
+ if (*current == '{' && !found_backslash) {
open_braces++;
- if (*current == '}')
+ if (is_unpacked)
+ *is_unpacked = false;
+ }
+ if (*current == '}' && !found_backslash)
open_braces--;
+
+ found_backslash = false;
}

/* Parse error, closing brace missing */
@@ -96,21 +154,126 @@ int pa_message_param_split_list(char *c, char **result, void **state) {
/* Read a string from the parameter list. The state pointer is
* advanced to the next element of the list. If allocate is
* true, a newly allocated string will be returned, else a
- * pointer to a sub-string within c. */
+ * pointer to a sub-string within c. Escaping will be removed
+ * if the string does not contain another list. */
int pa_message_param_read_string(char *c, char **result, bool allocate, void **state) {
char *start_pos;
int err;
+ bool is_unpacked;

pa_assert(result);

*result = NULL;

- if ((err = pa_message_param_split_list(c, &start_pos, state)) != 1)
+ if ((err = pa_message_param_split_list(c, &start_pos, &is_unpacked, state)) != 1)
return err;

*result = start_pos;
+
+ if (is_unpacked)
+ *result = unescape(*result);
+
if (allocate)
*result = pa_xstrdup(*result);

return err;
}
+
+/* Write functions. The functions are wrapper functions around pa_strbuf,
+ * so that the client does not need to use pa_strbuf directly. */
+
+/* Creates a new pa_message_param structure */
+pa_message_param *pa_message_param_new(void) {
+ pa_message_param *param;
+
+ param = pa_xnew(pa_message_param, 1);
+ param->buffer = pa_strbuf_new();
+
+ return param;
+}
+
+/* Converts a pa_message_param structure to string and frees the structure */
+char *pa_message_param_to_string(pa_message_param *param) {
+ char *result;
+
+ pa_assert(param);
+
+ result = pa_strbuf_to_string_free(param->buffer);
+
+ pa_xfree(param);
+ return result;
+}
+
+/* Writes an opening curly brace */
+void pa_message_param_begin_list(pa_message_param *param) {
+
+ pa_assert(param);
+
+ pa_strbuf_putc(param->buffer, '{');
+}
+
+/* Writes a closing curly brace */
+void pa_message_param_end_list(pa_message_param *param) {
+
+ pa_assert(param);
+
+ pa_strbuf_putc(param->buffer, '}');
+}
+
+/* Writes a string to a message_param structure, adding curly braces
+ * around the string. If do_escape is true, leading and trailing single
+ * quotes and also a "\" before curly braces are added to the input string.
+ * Escaping only needs to be used if the original string might fulfill any
+ * of the following conditions:
+ * - It contains curly braces
+ * - It is enclosed in single quotes
+ * - It ends with a "\" */
+void pa_message_param_write_string(pa_message_param *param, const char *value, bool do_escape) {
+ const char *s;
+ char *output, *out_char;
+ size_t brace_count;
+
+ pa_assert(param);
+
+ /* Null value is written as empty element */
+ if (!value)
+ value = "";
+
+ if (!do_escape) {
+ pa_strbuf_printf(param->buffer, "{%s}", value);
+ return;
+ }
+
+ /* Using pa_strbuf_putc() to write to the strbuf while iterating over
+ * the input string would cause the allocation of a linked list element
+ * for each character of the input string. Therefore the output string
+ * is constructed locally before writing it to the buffer */
+
+ /* Count number of characters to escape */
+ brace_count = 0;
+ for (s = value; *s; ++s) {
+ if (*s == '{' || *s == '}')
+ brace_count++;
+ }
+
+ /* allocate output string */
+ output = pa_xmalloc(strlen(value) + brace_count + 5);
+ out_char = output;
+
+ *out_char++ = '{';
+ *out_char++ = '\'';
+
+ for (s = value; *s; ++s) {
+ if (*s == '{' || *s == '}')
+ *out_char++ = '\\';
+
+ *out_char++ = *s;
+ }
+
+ *out_char++ = '\'';
+ *out_char++ = '}';
+ *out_char = 0;
+
+ pa_strbuf_puts(param->buffer, output);
+ pa_xfree(output);
+}
diff --git a/src/pulse/message-params.h b/src/pulse/message-params.h
index 02e08363..c0675c6e 100644
--- a/src/pulse/message-params.h
+++ b/src/pulse/message-params.h
@@ -30,12 +30,33 @@

PA_C_DECL_BEGIN

+typedef struct pa_message_param pa_message_param;
+
+/** Read functions */
+
/** Split message parameter string into list elements */
-int pa_message_param_split_list(char *c, char **result, void **state);
+int pa_message_param_split_list(char *c, char **result, bool * is_unpacked, void **state);

/** Read a string from the parameter list. */
int pa_message_param_read_string(char *c, char **result, bool allocate, void **state);

+/** Write functions */
+
+/** Create a new pa_message_param structure */
+pa_message_param *pa_message_param_new(void);
+
+/** Convert pa_message_param to string, free pa_message_param structure */
+char *pa_message_param_to_string(pa_message_param *param);
+
+/** Write an opening brace */
+void pa_message_param_begin_list(pa_message_param *param);
+
+/** Write a closing brace */
+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);
+
PA_C_DECL_END

#endif
diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
index e95657f0..acd47cbb 100644
--- a/src/pulsecore/core.c
+++ b/src/pulsecore/core.c
@@ -29,6 +29,7 @@
#include <pulse/rtclock.h>
#include <pulse/timeval.h>
#include <pulse/xmalloc.h>
+#include <pulse/message-params.h>

#include <pulsecore/module.h>
#include <pulsecore/core-rtclock.h>
@@ -65,25 +66,24 @@ static void core_free(pa_object *o);

/* Returns a list of handlers. */
static char *message_handler_list(pa_core *c) {
- pa_strbuf *buf;
+ pa_message_param *param;
void *state = NULL;
struct pa_message_handler *handler;

- buf = pa_strbuf_new();
+ param = pa_message_param_new();

- pa_strbuf_putc(buf, '{');
+ pa_message_param_begin_list(param);
PA_HASHMAP_FOREACH(handler, c->message_handlers, state) {
- pa_strbuf_putc(buf, '{');
+ pa_message_param_begin_list(param);

- pa_strbuf_printf(buf, "{%s} {", handler->object_path);
- if (handler->description)
- pa_strbuf_puts(buf, handler->description);
+ pa_message_param_write_string(param, handler->object_path, false);
+ pa_message_param_write_string(param, handler->description, true);

- pa_strbuf_puts(buf, "}}");
+ pa_message_param_end_list(param);
}
- pa_strbuf_putc(buf, '}');
+ pa_message_param_end_list(param);

- return pa_strbuf_to_string_free(buf);
+ return pa_message_param_to_string(param);
}

static int core_message_handler(const char *object_path, const char *message, char *message_parameters, char **response, void *userdata) {
diff --git a/src/pulsecore/message-handler.c b/src/pulsecore/message-handler.c
index 427186db..860f3b8e 100644
--- a/src/pulsecore/message-handler.c
+++ b/src/pulsecore/message-handler.c
@@ -31,15 +31,29 @@

#include "message-handler.h"

-/* Check if a string does not contain control characters. Currently these are
- * only "{" and "}". */
-static bool string_is_valid(const char *test_string) {
+/* Check if a path string starts with a / and only contains valid characters */
+static bool object_path_is_valid(const char *test_string) {
uint32_t i;

+ if (!test_string)
+ return false;
+
+ /* Make sure the string starts with / and does not end with a / */
+ if (test_string[0] != '/' || test_string[strlen(test_string) - 1] == '/')
+ return false;
+
for (i = 0; test_string[i]; i++) {
- if (test_string[i] == '{' ||
- test_string[i] == '}')
- return false;
+
+ if ((test_string[i] >= 'a' && test_string[i] <= 'z') ||
+ (test_string[i] >= 'A' && test_string[i] <= 'Z') ||
+ (test_string[i] >= '0' && test_string[i] <= '9') ||
+ test_string[i] == '.' ||
+ test_string[i] == '_' ||
+ test_string[i] == '-' ||
+ test_string[i] == '/')
+ continue;
+
+ return false;
}

return true;
@@ -56,13 +70,8 @@ void pa_message_handler_register(pa_core *c, const char *object_path, const char
pa_assert(cb);
pa_assert(userdata);

- /* Ensure that the object path is not empty and starts with "/". */
- pa_assert(object_path[0] == '/');
-
- /* Ensure that object path and description are valid strings */
- pa_assert(string_is_valid(object_path));
- if (description)
- pa_assert(string_is_valid(description));
+ /* Ensure that object path is valid */
+ pa_assert(object_path_is_valid(object_path));

handler = pa_xnew0(struct pa_message_handler, 1);
handler->userdata = userdata;
@@ -91,7 +100,7 @@ void pa_message_handler_unregister(pa_core *c, const char *object_path) {
int pa_message_handler_send_message(pa_core *c, const char *object_path, const char *message, const char *message_parameters, char **response) {
struct pa_message_handler *handler;
int ret;
- char *parameter_copy;
+ char *parameter_copy, *path_copy;

pa_assert(c);
pa_assert(object_path);
@@ -100,8 +109,16 @@ int pa_message_handler_send_message(pa_core *c, const char *object_path, const c

*response = NULL;

- if (!(handler = pa_hashmap_get(c->message_handlers, object_path)))
+ path_copy = pa_xstrdup(object_path);
+
+ /* Remove trailing / from path name if present */
+ if (path_copy[strlen(path_copy) - 1] == '/')
+ path_copy[strlen(path_copy) - 1] = 0;
+
+ if (!(handler = pa_hashmap_get(c->message_handlers, path_copy))) {
+ pa_xfree(path_copy);
return -PA_ERR_NOENTITY;
+ }

parameter_copy = pa_xstrdup(message_parameters);

@@ -110,6 +127,7 @@ int pa_message_handler_send_message(pa_core *c, const char *object_path, const c
ret = handler->callback(handler->object_path, message, parameter_copy, response, handler->userdata);

pa_xfree(parameter_copy);
+ pa_xfree(path_copy);
return ret;
}

@@ -123,11 +141,6 @@ int pa_message_handler_set_description(pa_core *c, const char *object_path, cons
if (!(handler = pa_hashmap_get(c->message_handlers, object_path)))
return -PA_ERR_NOENTITY;

- if (description) {
- if (!string_is_valid(description))
- return -PA_ERR_INVALID;
- }
-
pa_xfree(handler->description);
handler->description = pa_xstrdup(description);

diff --git a/src/utils/pactl.c b/src/utils/pactl.c
index fdafe57b..1f13e60c 100644
--- a/src/utils/pactl.c
+++ b/src/utils/pactl.c
@@ -864,14 +864,14 @@ static void list_handlers_callback(pa_context *c, int success, char *response, v
return;
}

- if (pa_message_param_split_list(response, &param_list, &state) <= 0) {
+ if (pa_message_param_split_list(response, &param_list, NULL, &state) <= 0) {
pa_log(_("list-handlers message response could not be parsed correctly"));
quit(1);
return;
}

state = NULL;
- while ((err = pa_message_param_split_list(param_list, &start_pos, &state)) > 0) {
+ while ((err = pa_message_param_split_list(param_list, &start_pos, NULL, &state)) > 0) {
void *state2 = NULL;
char *path;
char *description;
--
2.14.1
Tanu Kaskinen
2018-07-21 18:17:39 UTC
Permalink
Post by Georg Chini
The patch adds the possibility to escape curly braces within parameter strings
and introduces several new functions that can be used for writing parameters.
For writing, the structure pa_message_param, which is a wrapper for pa_strbuf
pa_message_param_new() - creates a new pa_message_param structure
pa_message_param_to_string() - converts a pa_message_param to string and frees
the structure
I'd like to have the _free suffix in the function name, because for the
uninitiated the current name looks like just another "to_string"
function with no surprising side effects.

I think we need pa_message_params_free() as well in case an application
runs into an error while constructing the parameters and it wants to
just get rid of the pa_message_params struct without converting it to a
string first.
Post by Georg Chini
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
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
falsely escape the closing bracket. To avoid this, single quotes must be added
at start and end of the string.
Why not solve this by the usual method: require escaping of "\" in
input with "\\" in output?
Post by Georg Chini
The function pa_message_param_write_string()
has a parameter do_escape.
Why not do escaping always?
Post by Georg Chini
If true, the necessary escaping is added. Escaping
- It contains curly braces
- It contains a trailing "\"
- It is enclosed in single quotes
Other strings can be passed without modification.
For reading, pa_message_param_read_string() reverts the changes that
pa_message_param_write_string() might have introduced.
The patch also adds more restrictions on the object path name. Now only
alphanumeric characters and one of "_", ".", "-" and "/" are allowed.
The path name may not end with a /. If the user specifies a trailing / when
sending a message, it will be removed.
---
doc/messaging_api.txt | 34 +++++++-
src/Makefile.am | 3 +-
src/map-file | 5 ++
src/pulse/message-params.c | 181 ++++++++++++++++++++++++++++++++++++++--
src/pulse/message-params.h | 23 ++++-
src/pulsecore/core.c | 20 ++---
src/pulsecore/message-handler.c | 53 +++++++-----
src/utils/pactl.c | 4 +-
8 files changed, 279 insertions(+), 44 deletions(-)
diff --git a/doc/messaging_api.txt b/doc/messaging_api.txt
index 431a5df2..0e6be53f 100644
--- a/doc/messaging_api.txt
+++ b/doc/messaging_api.txt
{{Integer} {{1st float} {2nd float} ...}}{...}
Any characters that are not enclosed in curly braces are ignored (all characters
between { and {, between } and } and between } and {). The same syntax is used
-to specify message parameters. The following reference lists available messages,
+to specify message parameters. The reference further down lists available messages,
their parameters and return values. If a return value is enclosed in {}, this
means that multiple elements of the same type may be returned.
+There are several functions that simplify reading and writing message parameter
+strings. For writing, the structure pa_message_param can be used. Following
+pa_message_param_new() - creates a new pa_message_param structure
+pa_message_param_to_string() - converts a pa_message_param to string and frees
+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
+
+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
+falsely escape the closing bracket. To avoid this, single quotes must be added
+at start and end of the string. The function pa_message_param_write_string()
+has a parameter do_escape. If true, the necessary escaping is added. Escaping
+
+- It contains curly braces
+- It contains a trailing "\"
+- It is enclosed in single quotes
+
+Other strings can be passed without modification.
+
+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_string() also reverts the changes that
+pa_message_param_write_string() might have introduced.
I think the doxygen documentation is better suited for the details of
the C API. messaging_api.txt should contain information that is common
to both libpulse and pactl users.
Post by Georg Chini
+
+
Object path: /core
Message: list-handlers
Parameters: None
diff --git a/src/Makefile.am b/src/Makefile.am
index ccdad8ff..72d8cf22 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -675,6 +675,7 @@ ***@PA_MAJORMINOR@_la_SOURCES = \
pulse/timeval.c pulse/timeval.h \
pulse/rtclock.c pulse/rtclock.h \
pulse/volume.c pulse/volume.h \
+ pulse/message-params.c pulse/message-params.h \
pulsecore/atomic.h \
pulsecore/authkey.c pulsecore/authkey.h \
pulsecore/conf-parser.c pulsecore/conf-parser.h \
@@ -884,6 +885,7 @@ libpulse_la_SOURCES = \
pulse/mainloop-api.c pulse/mainloop-api.h \
pulse/mainloop-signal.c pulse/mainloop-signal.h \
pulse/mainloop.c pulse/mainloop.h \
+ pulse/message-params.c pulse/message-params.h \
pulse/operation.c pulse/operation.h \
pulse/proplist.c pulse/proplist.h \
pulse/pulseaudio.h \
@@ -896,7 +898,6 @@ libpulse_la_SOURCES = \
pulse/timeval.c pulse/timeval.h \
pulse/utf8.c pulse/utf8.h \
pulse/util.c pulse/util.h \
- pulse/message-params.c pulse/message-params.h \
pulse/volume.c pulse/volume.h \
pulse/xmalloc.c pulse/xmalloc.h
diff --git a/src/map-file b/src/map-file
index 385731dc..372d190d 100644
--- a/src/map-file
+++ b/src/map-file
@@ -225,8 +225,13 @@ pa_mainloop_quit;
pa_mainloop_run;
pa_mainloop_set_poll_func;
pa_mainloop_wakeup;
+pa_message_param_begin_list;
+pa_message_param_end_list;
+pa_message_param_new;
pa_message_param_read_string;
pa_message_param_split_list;
+pa_message_param_to_string;
+pa_message_param_write_string;
pa_msleep;
pa_operation_cancel;
pa_operation_get_state;
diff --git a/src/pulse/message-params.c b/src/pulse/message-params.c
index 964e29d0..d68ec59d 100644
--- a/src/pulse/message-params.c
+++ b/src/pulse/message-params.c
@@ -28,20 +28,55 @@
#include <pulse/xmalloc.h>
#include <pulsecore/macro.h>
+#include <pulsecore/strbuf.h>
#include "message-params.h"
+/* Message parameter structure, a wrapper for pa_strbuf */
+struct pa_message_param {
+ pa_strbuf *buffer;
+};
+
+/* Remove escaping from a string */
+static char *unescape(char *value) {
+ char *tmp;
+
+ if (!value)
+ return NULL;
+
+ /* If the string is enclosed in single quotes, remove them */
+ if (*value == '\'' && value[strlen(value) - 1] == '\'') {
+
+ memmove(value, value + 1, strlen(value));
+ value[strlen(value) - 1] = 0;
+ }
+
+ /* Remove escape character from curly braces if present. */
+ while ((tmp = strstr(value, "\\{")))
+ memmove(tmp, tmp + 1, strlen(value) - (size_t)(tmp - value));
+ while ((tmp = strstr(value, "\\}")))
+ memmove(tmp, tmp + 1, strlen(value) - (size_t)(tmp - value));
+
+ /* Return the pointer that was passed in. */
+ return value;
+}
core-util already contains pa_unescape() that does the same thing more
efficiently (if you drop the single quote thing).
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
Post by Georg Chini
* The variable state points to, should be initialized to NULL before
* the first call. The function returns 1 on success, 0 if end of string
* is encountered and -1 on parse error. */
-int pa_message_param_split_list(char *c, char **result, void **state) {
+int pa_message_param_split_list(char *c, char **result, bool *is_unpacked, void **state) {
char *current = *state ? *state : c;
uint32_t open_braces;
+ bool found_backslash = false;
pa_assert(result);
@@ -54,29 +89,52 @@ int pa_message_param_split_list(char *c, char **result, void **state) {
/* Find opening brace */
while (*current != 0) {
- if (*current == '{')
+ /* Skip escaped curly braces. */
+ if (*current == '\\') {
With my proposed escaping rules, this should be

if (*current == '\\' && !found_backslash)

because if we have two backslashes in a row, the next character is not
escaped.
Post by Georg Chini
+ found_backslash = true;
+ current++;
+ continue;
+ }
+
+ if (*current == '{' && !found_backslash)
break;
/* unexpected closing brace, parse error */
- if (*current == '}')
+ if (*current == '}' && !found_backslash)
return -1;
+ found_backslash = false;
current++;
}
/* No opening brace found, end of string */
if (*current == 0)
- return 0;
+ return 0;
+ if (is_unpacked)
+ *is_unpacked = true;
*result = current + 1;
+ found_backslash = false;
open_braces = 1;
while (open_braces != 0 && *current != 0) {
current++;
- if (*current == '{')
+
+ /* Skip escaped curly braces. */
+ if (*current == '\\') {
Same as above, with my proposed unescaping rules this should be

if (*current == '\\' && !found_backslash)
Post by Georg Chini
+ found_backslash = true;
+ continue;
+ }
+
+ if (*current == '{' && !found_backslash) {
open_braces++;
- if (*current == '}')
+ if (is_unpacked)
+ *is_unpacked = false;
+ }
+ if (*current == '}' && !found_backslash)
open_braces--;
+
+ found_backslash = false;
}
/* Parse error, closing brace missing */
@@ -96,21 +154,126 @@ int pa_message_param_split_list(char *c, char **result, void **state) {
/* Read a string from the parameter list. The state pointer is
* advanced to the next element of the list. If allocate is
* true, a newly allocated string will be returned, else a
- * pointer to a sub-string within c. */
+ * pointer to a sub-string within c. Escaping will be removed
+ * if the string does not contain another list. */
A string is a string, it's not a list. I think this function should
always do unescaping. If a string value is read that is missing
escaping for curly braces, that could be reported as an error (I'm not
saying "should", but I think it would be good to do the validation in
this function).
Post by Georg Chini
int pa_message_param_read_string(char *c, char **result, bool allocate, void **state) {
char *start_pos;
int err;
+ bool is_unpacked;
pa_assert(result);
*result = NULL;
- if ((err = pa_message_param_split_list(c, &start_pos, state)) != 1)
+ if ((err = pa_message_param_split_list(c, &start_pos, &is_unpacked, state)) != 1)
return err;
*result = start_pos;
+
+ if (is_unpacked)
+ *result = unescape(*result);
+
if (allocate)
*result = pa_xstrdup(*result);
return err;
}
+
+/* Write functions. The functions are wrapper functions around pa_strbuf,
+ * so that the client does not need to use pa_strbuf directly. */
+
+/* Creates a new pa_message_param structure */
+pa_message_param *pa_message_param_new(void) {
+ pa_message_param *param;
+
+ param = pa_xnew(pa_message_param, 1);
+ param->buffer = pa_strbuf_new();
+
+ return param;
+}
+
+/* Converts a pa_message_param structure to string and frees the structure */
+char *pa_message_param_to_string(pa_message_param *param) {
+ char *result;
+
+ pa_assert(param);
+
+ result = pa_strbuf_to_string_free(param->buffer);
+
+ pa_xfree(param);
+ return result;
+}
+
+/* Writes an opening curly brace */
+void pa_message_param_begin_list(pa_message_param *param) {
+
+ pa_assert(param);
+
+ pa_strbuf_putc(param->buffer, '{');
+}
+
+/* Writes a closing curly brace */
+void pa_message_param_end_list(pa_message_param *param) {
+
+ pa_assert(param);
+
+ pa_strbuf_putc(param->buffer, '}');
+}
+
+/* Writes a string to a message_param structure, adding curly braces
+ * around the string. If do_escape is true, leading and trailing single
+ * quotes and also a "\" before curly braces are added to the input string.
+ * Escaping only needs to be used if the original string might fulfill any
+ * - It contains curly braces
+ * - It is enclosed in single quotes
+ * - It ends with a "\" */
+void pa_message_param_write_string(pa_message_param *param, const char *value, bool do_escape) {
I would drop the do_escape flag, and if there's need for appending
unescaped raw data to pa_message_params, there could be a separate
function for that.
Post by Georg Chini
+ const char *s;
+ char *output, *out_char;
+ size_t brace_count;
+
+ pa_assert(param);
+
+ /* Null value is written as empty element */
+ if (!value)
+ value = "";
+
+ if (!do_escape) {
+ pa_strbuf_printf(param->buffer, "{%s}", value);
+ return;
+ }
+
+ /* Using pa_strbuf_putc() to write to the strbuf while iterating over
+ * the input string would cause the allocation of a linked list element
+ * for each character of the input string. Therefore the output string
+ * is constructed locally before writing it to the buffer */
This is an interesting point. I think you should improve pa_escape(),
which currently uses the naive pa_strbuf_putc() method. With my
proposed escaping rules, you could then replace this code with a call
to pa_escape().
Post by Georg Chini
+
+ /* Count number of characters to escape */
+ brace_count = 0;
+ for (s = value; *s; ++s) {
+ if (*s == '{' || *s == '}')
+ brace_count++;
+ }
You could at the same time count all characters to save the strlen()
call.
Post by Georg Chini
+
+ /* allocate output string */
+ output = pa_xmalloc(strlen(value) + brace_count + 5);
It would be good to have a comment for that magic value 5.
Post by Georg Chini
+ out_char = output;
+
+ *out_char++ = '{';
+ *out_char++ = '\'';
+
+ for (s = value; *s; ++s) {
+ if (*s == '{' || *s == '}')
+ *out_char++ = '\\';
+
+ *out_char++ = *s;
+ }
+
+ *out_char++ = '\'';
+ *out_char++ = '}';
+ *out_char = 0;
+
+ pa_strbuf_puts(param->buffer, output);
+ pa_xfree(output);
+}
diff --git a/src/pulse/message-params.h b/src/pulse/message-params.h
index 02e08363..c0675c6e 100644
--- a/src/pulse/message-params.h
+++ b/src/pulse/message-params.h
@@ -30,12 +30,33 @@
PA_C_DECL_BEGIN
+typedef struct pa_message_param pa_message_param;
+
+/** Read functions */
While wondering how this kind of lone comments show up in doxygen, I
noticed that message-params.h is not listed in the INPUT list in
doxygen/doxygen.conf.in, so no documentation is generated.
Post by Georg Chini
+
/** Split message parameter string into list elements */
-int pa_message_param_split_list(char *c, char **result, void **state);
+int pa_message_param_split_list(char *c, char **result, bool * is_unpacked, void **state);
/** Read a string from the parameter list. */
int pa_message_param_read_string(char *c, char **result, bool allocate, void **state);
It's worth noting in the documentation that the function unescapes the
string.
Post by Georg Chini
+/** Write functions */
+
+/** Create a new pa_message_param structure */
+pa_message_param *pa_message_param_new(void);
+
+/** Convert pa_message_param to string, free pa_message_param structure */
+char *pa_message_param_to_string(pa_message_param *param);
It should be documented that the returned string should be freed with
pa_xfree(). (Also remember the \since tags.)
Post by Georg Chini
+
+/** Write an opening brace */
+void pa_message_param_begin_list(pa_message_param *param);
+
+/** Write a closing brace */
+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);
The documentation should mention that the function does escaping,
escpecially if you drop the do_escape flag as I hope, but even if the
flag stays, it should be described.
Post by Georg Chini
+
PA_C_DECL_END
#endif
diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
index e95657f0..acd47cbb 100644
--- a/src/pulsecore/core.c
+++ b/src/pulsecore/core.c
@@ -29,6 +29,7 @@
#include <pulse/rtclock.h>
#include <pulse/timeval.h>
#include <pulse/xmalloc.h>
+#include <pulse/message-params.h>
#include <pulsecore/module.h>
#include <pulsecore/core-rtclock.h>
@@ -65,25 +66,24 @@ static void core_free(pa_object *o);
/* Returns a list of handlers. */
static char *message_handler_list(pa_core *c) {
- pa_strbuf *buf;
+ pa_message_param *param;
void *state = NULL;
struct pa_message_handler *handler;
- buf = pa_strbuf_new();
+ param = pa_message_param_new();
- pa_strbuf_putc(buf, '{');
+ pa_message_param_begin_list(param);
PA_HASHMAP_FOREACH(handler, c->message_handlers, state) {
- pa_strbuf_putc(buf, '{');
+ pa_message_param_begin_list(param);
- pa_strbuf_printf(buf, "{%s} {", handler->object_path);
- if (handler->description)
- pa_strbuf_puts(buf, handler->description);
+ pa_message_param_write_string(param, handler->object_path, false);
+ pa_message_param_write_string(param, handler->description, true);
- pa_strbuf_puts(buf, "}}");
+ pa_message_param_end_list(param);
}
- pa_strbuf_putc(buf, '}');
+ pa_message_param_end_list(param);
- return pa_strbuf_to_string_free(buf);
+ return pa_message_param_to_string(param);
}
static int core_message_handler(const char *object_path, const char *message, char *message_parameters, char **response, void *userdata) {
diff --git a/src/pulsecore/message-handler.c b/src/pulsecore/message-handler.c
index 427186db..860f3b8e 100644
--- a/src/pulsecore/message-handler.c
+++ b/src/pulsecore/message-handler.c
@@ -31,15 +31,29 @@
#include "message-handler.h"
-/* Check if a string does not contain control characters. Currently these are
- * only "{" and "}". */
-static bool string_is_valid(const char *test_string) {
+/* Check if a path string starts with a / and only contains valid characters */
+static bool object_path_is_valid(const char *test_string) {
uint32_t i;
+ if (!test_string)
+ return false;
+
+ /* Make sure the string starts with / and does not end with a / */
+ if (test_string[0] != '/' || test_string[strlen(test_string) - 1] == '/')
+ return false;
+
for (i = 0; test_string[i]; i++) {
- if (test_string[i] == '{' ||
- test_string[i] == '}')
- return false;
+
+ if ((test_string[i] >= 'a' && test_string[i] <= 'z') ||
+ (test_string[i] >= 'A' && test_string[i] <= 'Z') ||
+ (test_string[i] >= '0' && test_string[i] <= '9') ||
+ test_string[i] == '.' ||
+ test_string[i] == '_' ||
+ test_string[i] == '-' ||
+ test_string[i] == '/')
+ continue;
+
+ return false;
}
You could save the strlen() call if you checked the trailing slash
after this for loop when you're at the end of the string.

By the way, do you perhaps want to disallow two subsequent slashes in
object paths? (I don't care myself, because I doubt that invalid paths
will ever be encountered anyway when registering message handlers.)
Post by Georg Chini
return true;
@@ -56,13 +70,8 @@ void pa_message_handler_register(pa_core *c, const char *object_path, const char
pa_assert(cb);
pa_assert(userdata);
- /* Ensure that the object path is not empty and starts with "/". */
- pa_assert(object_path[0] == '/');
-
- /* Ensure that object path and description are valid strings */
- pa_assert(string_is_valid(object_path));
- if (description)
- pa_assert(string_is_valid(description));
+ /* Ensure that object path is valid */
+ pa_assert(object_path_is_valid(object_path));
handler = pa_xnew0(struct pa_message_handler, 1);
handler->userdata = userdata;
@@ -91,7 +100,7 @@ void pa_message_handler_unregister(pa_core *c, const char *object_path) {
int pa_message_handler_send_message(pa_core *c, const char *object_path, const char *message, const char *message_parameters, char **response) {
struct pa_message_handler *handler;
int ret;
- char *parameter_copy;
+ char *parameter_copy, *path_copy;
pa_assert(c);
pa_assert(object_path);
@@ -100,8 +109,16 @@ int pa_message_handler_send_message(pa_core *c, const char *object_path, const c
*response = NULL;
- if (!(handler = pa_hashmap_get(c->message_handlers, object_path)))
+ path_copy = pa_xstrdup(object_path);
+
+ /* Remove trailing / from path name if present */
+ if (path_copy[strlen(path_copy) - 1] == '/')
+ path_copy[strlen(path_copy) - 1] = 0;
If you want to allow some slack for applications, would it be better to
do this extra work in libpulse rather than in the daemon?

I would personally not bother with this at all, but I don't mind it
that much (even if it's done in the daemon).
--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
Georg Chini
2018-07-22 14:02:54 UTC
Permalink
Post by Tanu Kaskinen
Post by Georg Chini
The patch adds the possibility to escape curly braces within parameter strings
and introduces several new functions that can be used for writing parameters.
For writing, the structure pa_message_param, which is a wrapper for pa_strbuf
pa_message_param_new() - creates a new pa_message_param structure
pa_message_param_to_string() - converts a pa_message_param to string and frees
the structure
I'd like to have the _free suffix in the function name, because for the
uninitiated the current name looks like just another "to_string"
function with no surprising side effects.
I think we need pa_message_params_free() as well in case an application
runs into an error while constructing the parameters and it wants to
just get rid of the pa_message_params struct without converting it to a
string first.
Post by Georg Chini
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
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
falsely escape the closing bracket. To avoid this, single quotes must be added
at start and end of the string.
Why not solve this by the usual method: require escaping of "\" in
input with "\\" in output?
You are right, if I modify pa_unescape() as mentioned further below and then
use pa_escape()/pa_unescape() the quotes won't be necessary.
Post by Tanu Kaskinen
Post by Georg Chini
The function pa_message_param_write_string()
has a parameter do_escape.
Why not do escaping always?
Because what you are writing as a string may be a list that you have
prepared
previously. Then you will not want escaping. You may for example create
a list
from an array and then insert this list as one string into the final
parameter list
or have a function that converts a certain structure to a parameter
string and
then write the result of this function as one element of the final list.
Post by Tanu Kaskinen
Post by Georg Chini
If true, the necessary escaping is added. Escaping
- It contains curly braces
- It contains a trailing "\"
- It is enclosed in single quotes
Other strings can be passed without modification.
For reading, pa_message_param_read_string() reverts the changes that
pa_message_param_write_string() might have introduced.
The patch also adds more restrictions on the object path name. Now only
alphanumeric characters and one of "_", ".", "-" and "/" are allowed.
The path name may not end with a /. If the user specifies a trailing / when
sending a message, it will be removed.
---
doc/messaging_api.txt | 34 +++++++-
src/Makefile.am | 3 +-
src/map-file | 5 ++
src/pulse/message-params.c | 181 ++++++++++++++++++++++++++++++++++++++--
src/pulse/message-params.h | 23 ++++-
src/pulsecore/core.c | 20 ++---
src/pulsecore/message-handler.c | 53 +++++++-----
src/utils/pactl.c | 4 +-
8 files changed, 279 insertions(+), 44 deletions(-)
diff --git a/doc/messaging_api.txt b/doc/messaging_api.txt
index 431a5df2..0e6be53f 100644
--- a/doc/messaging_api.txt
+++ b/doc/messaging_api.txt
{{Integer} {{1st float} {2nd float} ...}}{...}
Any characters that are not enclosed in curly braces are ignored (all characters
between { and {, between } and } and between } and {). The same syntax is used
-to specify message parameters. The following reference lists available messages,
+to specify message parameters. The reference further down lists available messages,
their parameters and return values. If a return value is enclosed in {}, this
means that multiple elements of the same type may be returned.
+There are several functions that simplify reading and writing message parameter
+strings. For writing, the structure pa_message_param can be used. Following
+pa_message_param_new() - creates a new pa_message_param structure
+pa_message_param_to_string() - converts a pa_message_param to string and frees
+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
+
+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
+falsely escape the closing bracket. To avoid this, single quotes must be added
+at start and end of the string. The function pa_message_param_write_string()
+has a parameter do_escape. If true, the necessary escaping is added. Escaping
+
+- It contains curly braces
+- It contains a trailing "\"
+- It is enclosed in single quotes
+
+Other strings can be passed without modification.
+
+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_string() also reverts the changes that
+pa_message_param_write_string() might have introduced.
I think the doxygen documentation is better suited for the details of
the C API. messaging_api.txt should contain information that is common
to both libpulse and pactl users.
Somehow it is not really clear to me, which documentation you want
to have in messaging_api.txt.
Post by Tanu Kaskinen
Post by Georg Chini
+/* Remove escaping from a string */
+static char *unescape(char *value) {
+ char *tmp;
+
+ if (!value)
+ return NULL;
+
+ /* If the string is enclosed in single quotes, remove them */
+ if (*value == '\'' && value[strlen(value) - 1] == '\'') {
+
+ memmove(value, value + 1, strlen(value));
+ value[strlen(value) - 1] = 0;
+ }
+
+ /* Remove escape character from curly braces if present. */
+ while ((tmp = strstr(value, "\\{")))
+ memmove(tmp, tmp + 1, strlen(value) - (size_t)(tmp - value));
+ while ((tmp = strstr(value, "\\}")))
+ memmove(tmp, tmp + 1, strlen(value) - (size_t)(tmp - value));
+
+ /* Return the pointer that was passed in. */
+ return value;
+}
core-util already contains pa_unescape() that does the same thing more
efficiently (if you drop the single quote thing).
pa_unescape() currently does not do the same thing. It removes all
escape characters, while I only want to remove the characters
I actually introduced (those before { or }).
I can however modify pa_unescape() to take the same arguments
as pa_escape().
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
It may be possible, that the string you read is a list. Consider the
following
parameter list: {string1}{some nested structure}{string2}. You can now
read this list as three strings and then continue to read the elements of
the nested structure from the second string. You might even create a
function
that takes a string and outputs a structure. So you are not forced to go
to the full depth of nesting on the first pass. This makes it much easier
to handle deeply nested parameter lists. For me this behavior is an
important
feature and I do not want to drop it. See also my comment on why I do
not always want escaping.
Post by Tanu Kaskinen
Post by Georg Chini
/* Parse error, closing brace missing */
@@ -96,21 +154,126 @@ int pa_message_param_split_list(char *c, char **result, void **state) {
/* Read a string from the parameter list. The state pointer is
* advanced to the next element of the list. If allocate is
* true, a newly allocated string will be returned, else a
- * pointer to a sub-string within c. */
+ * pointer to a sub-string within c. Escaping will be removed
+ * if the string does not contain another list. */
A string is a string, it's not a list. I think this function should
always do unescaping. If a string value is read that is missing
escaping for curly braces, that could be reported as an error (I'm not
saying "should", but I think it would be good to do the validation in
this function).
See my comments above why a string may be a list.
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Writes a string to a message_param structure, adding curly braces
+ * around the string. If do_escape is true, leading and trailing single
+ * quotes and also a "\" before curly braces are added to the input string.
+ * Escaping only needs to be used if the original string might fulfill any
+ * - It contains curly braces
+ * - It is enclosed in single quotes
+ * - It ends with a "\" */
+void pa_message_param_write_string(pa_message_param *param, const char *value, bool do_escape) {
I would drop the do_escape flag, and if there's need for appending
unescaped raw data to pa_message_params, there could be a separate
function for that.
See above. A function for writing raw data should nevertheless be provided
for the case where a string can only be written as several parts.
Post by Tanu Kaskinen
Post by Georg Chini
+ const char *s;
+ char *output, *out_char;
+ size_t brace_count;
+
+ pa_assert(param);
+
+ /* Null value is written as empty element */
+ if (!value)
+ value = "";
+
+ if (!do_escape) {
+ pa_strbuf_printf(param->buffer, "{%s}", value);
+ return;
+ }
+
+ /* Using pa_strbuf_putc() to write to the strbuf while iterating over
+ * the input string would cause the allocation of a linked list element
+ * for each character of the input string. Therefore the output string
+ * is constructed locally before writing it to the buffer */
This is an interesting point. I think you should improve pa_escape(),
which currently uses the naive pa_strbuf_putc() method. With my
proposed escaping rules, you could then replace this code with a call
to pa_escape().
OK, I'll do that and send a patch for tweaking pa_escape()/pa_unescape()
to my needs separately.
Post by Tanu Kaskinen
Post by Georg Chini
+
+ /* Count number of characters to escape */
+ brace_count = 0;
+ for (s = value; *s; ++s) {
+ if (*s == '{' || *s == '}')
+ brace_count++;
+ }
You could at the same time count all characters to save the strlen()
call.
Mh, do I really save something? If the string is x characters
long, I would increment a variable this often. Is that less work
than a strlen() call? But probably strlen() does something similar ...
Post by Tanu Kaskinen
Post by Georg Chini
+/* Check if a path string starts with a / and only contains valid characters */
+static bool object_path_is_valid(const char *test_string) {
uint32_t i;
+ if (!test_string)
+ return false;
+
+ /* Make sure the string starts with / and does not end with a / */
+ if (test_string[0] != '/' || test_string[strlen(test_string) - 1] == '/')
+ return false;
+
for (i = 0; test_string[i]; i++) {
- if (test_string[i] == '{' ||
- test_string[i] == '}')
- return false;
+
+ if ((test_string[i] >= 'a' && test_string[i] <= 'z') ||
+ (test_string[i] >= 'A' && test_string[i] <= 'Z') ||
+ (test_string[i] >= '0' && test_string[i] <= '9') ||
+ test_string[i] == '.' ||
+ test_string[i] == '_' ||
+ test_string[i] == '-' ||
+ test_string[i] == '/')
+ continue;
+
+ return false;
}
You could save the strlen() call if you checked the trailing slash
after this for loop when you're at the end of the string.
By the way, do you perhaps want to disallow two subsequent slashes in
object paths? (I don't care myself, because I doubt that invalid paths
will ever be encountered anyway when registering message handlers.)
If I don't disallow it, then a double slash in the path will be valid.
At least up to now we do not care about the individual components
that the path is constructed from.
So should I disallow it or not?
Post by Tanu Kaskinen
Post by Georg Chini
return true;
@@ -56,13 +70,8 @@ void pa_message_handler_register(pa_core *c, const char *object_path, const char
pa_assert(cb);
pa_assert(userdata);
- /* Ensure that the object path is not empty and starts with "/". */
- pa_assert(object_path[0] == '/');
-
- /* Ensure that object path and description are valid strings */
- pa_assert(string_is_valid(object_path));
- if (description)
- pa_assert(string_is_valid(description));
+ /* Ensure that object path is valid */
+ pa_assert(object_path_is_valid(object_path));
handler = pa_xnew0(struct pa_message_handler, 1);
handler->userdata = userdata;
@@ -91,7 +100,7 @@ void pa_message_handler_unregister(pa_core *c, const char *object_path) {
int pa_message_handler_send_message(pa_core *c, const char *object_path, const char *message, const char *message_parameters, char **response) {
struct pa_message_handler *handler;
int ret;
- char *parameter_copy;
+ char *parameter_copy, *path_copy;
pa_assert(c);
pa_assert(object_path);
@@ -100,8 +109,16 @@ int pa_message_handler_send_message(pa_core *c, const char *object_path, const c
*response = NULL;
- if (!(handler = pa_hashmap_get(c->message_handlers, object_path)))
+ path_copy = pa_xstrdup(object_path);
+
+ /* Remove trailing / from path name if present */
+ if (path_copy[strlen(path_copy) - 1] == '/')
+ path_copy[strlen(path_copy) - 1] = 0;
If you want to allow some slack for applications, would it be better to
do this extra work in libpulse rather than in the daemon?
I would personally not bother with this at all, but I don't mind it
that much (even if it's done in the daemon).
If you don't mind, I would leave it as it is.


Unfortunately I am still heavily loaded, so it may take a while before
I can send new patches.
Tanu Kaskinen
2018-07-22 15:48:41 UTC
Permalink
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
The patch adds the possibility to escape curly braces within parameter strings
and introduces several new functions that can be used for writing parameters.
For writing, the structure pa_message_param, which is a wrapper for pa_strbuf
pa_message_param_new() - creates a new pa_message_param structure
pa_message_param_to_string() - converts a pa_message_param to string and frees
the structure
I'd like to have the _free suffix in the function name, because for the
uninitiated the current name looks like just another "to_string"
function with no surprising side effects.
I think we need pa_message_params_free() as well in case an application
runs into an error while constructing the parameters and it wants to
just get rid of the pa_message_params struct without converting it to a
string first.
Post by Georg Chini
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
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
falsely escape the closing bracket. To avoid this, single quotes must be added
at start and end of the string.
Why not solve this by the usual method: require escaping of "\" in
input with "\\" in output?
You are right, if I modify pa_unescape() as mentioned further below and then
use pa_escape()/pa_unescape() the quotes won't be necessary.
Post by Tanu Kaskinen
Post by Georg Chini
The function pa_message_param_write_string()
has a parameter do_escape.
Why not do escaping always?
Because what you are writing as a string may be a list that you have
prepared
previously. Then you will not want escaping. You may for example create
a list
from an array and then insert this list as one string into the final
parameter list
or have a function that converts a certain structure to a parameter
string and
then write the result of this function as one element of the final list.
My mental model is that parameters have types, list type is different
than string type, and write_string() is only meant for writing values
of the string type.

Can you add a write_raw() function?
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
If true, the necessary escaping is added. Escaping
- It contains curly braces
- It contains a trailing "\"
- It is enclosed in single quotes
Other strings can be passed without modification.
For reading, pa_message_param_read_string() reverts the changes that
pa_message_param_write_string() might have introduced.
The patch also adds more restrictions on the object path name. Now only
alphanumeric characters and one of "_", ".", "-" and "/" are allowed.
The path name may not end with a /. If the user specifies a trailing / when
sending a message, it will be removed.
---
doc/messaging_api.txt | 34 +++++++-
src/Makefile.am | 3 +-
src/map-file | 5 ++
src/pulse/message-params.c | 181 ++++++++++++++++++++++++++++++++++++++--
src/pulse/message-params.h | 23 ++++-
src/pulsecore/core.c | 20 ++---
src/pulsecore/message-handler.c | 53 +++++++-----
src/utils/pactl.c | 4 +-
8 files changed, 279 insertions(+), 44 deletions(-)
diff --git a/doc/messaging_api.txt b/doc/messaging_api.txt
index 431a5df2..0e6be53f 100644
--- a/doc/messaging_api.txt
+++ b/doc/messaging_api.txt
{{Integer} {{1st float} {2nd float} ...}}{...}
Any characters that are not enclosed in curly braces are ignored (all characters
between { and {, between } and } and between } and {). The same syntax is used
-to specify message parameters. The following reference lists available messages,
+to specify message parameters. The reference further down lists available messages,
their parameters and return values. If a return value is enclosed in {}, this
means that multiple elements of the same type may be returned.
+There are several functions that simplify reading and writing message parameter
+strings. For writing, the structure pa_message_param can be used. Following
+pa_message_param_new() - creates a new pa_message_param structure
+pa_message_param_to_string() - converts a pa_message_param to string and frees
+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
+
+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
+falsely escape the closing bracket. To avoid this, single quotes must be added
+at start and end of the string. The function pa_message_param_write_string()
+has a parameter do_escape. If true, the necessary escaping is added. Escaping
+
+- It contains curly braces
+- It contains a trailing "\"
+- It is enclosed in single quotes
+
+Other strings can be passed without modification.
+
+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_string() also reverts the changes that
+pa_message_param_write_string() might have introduced.
I think the doxygen documentation is better suited for the details of
the C API. messaging_api.txt should contain information that is common
to both libpulse and pactl users.
Somehow it is not really clear to me, which documentation you want
to have in messaging_api.txt.
I want documentation for two things: the serialization format
specification and the documentation for the supported messages (plus a
short introduction explaining why the messaging API exists). When
writing C programs, the doxygen documentation is the reference for the
C API details.
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+/* Remove escaping from a string */
+static char *unescape(char *value) {
+ char *tmp;
+
+ if (!value)
+ return NULL;
+
+ /* If the string is enclosed in single quotes, remove them */
+ if (*value == '\'' && value[strlen(value) - 1] == '\'') {
+
+ memmove(value, value + 1, strlen(value));
+ value[strlen(value) - 1] = 0;
+ }
+
+ /* Remove escape character from curly braces if present. */
+ while ((tmp = strstr(value, "\\{")))
+ memmove(tmp, tmp + 1, strlen(value) - (size_t)(tmp - value));
+ while ((tmp = strstr(value, "\\}")))
+ memmove(tmp, tmp + 1, strlen(value) - (size_t)(tmp - value));
+
+ /* Return the pointer that was passed in. */
+ return value;
+}
core-util already contains pa_unescape() that does the same thing more
efficiently (if you drop the single quote thing).
pa_unescape() currently does not do the same thing. It removes all
escape characters, while I only want to remove the characters
I actually introduced (those before { or }).
I can however modify pa_unescape() to take the same arguments
as pa_escape().
I don't see the need for being selective when unescaping. Nothing
breaks if all (except escaped) backslashes are stripped.
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
It may be possible, that the string you read is a list. Consider the
following
parameter list: {string1}{some nested structure}{string2}. You can now
read this list as three strings and then continue to read the elements of
the nested structure from the second string. You might even create a
function
that takes a string and outputs a structure. So you are not forced to go
to the full depth of nesting on the first pass. This makes it much easier
to handle deeply nested parameter lists. For me this behavior is an
important
feature and I do not want to drop it. See also my comment on why I do
not always want escaping.
Doesn't split_list() already allow this, why do you want to use
read_string() to do the same thing as split_list()?
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
/* Parse error, closing brace missing */
@@ -96,21 +154,126 @@ int pa_message_param_split_list(char *c, char **result, void **state) {
/* Read a string from the parameter list. The state pointer is
* advanced to the next element of the list. If allocate is
* true, a newly allocated string will be returned, else a
- * pointer to a sub-string within c. */
+ * pointer to a sub-string within c. Escaping will be removed
+ * if the string does not contain another list. */
A string is a string, it's not a list. I think this function should
always do unescaping. If a string value is read that is missing
escaping for curly braces, that could be reported as an error (I'm not
saying "should", but I think it would be good to do the validation in
this function).
See my comments above why a string may be a list.
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Writes a string to a message_param structure, adding curly braces
+ * around the string. If do_escape is true, leading and trailing single
+ * quotes and also a "\" before curly braces are added to the input string.
+ * Escaping only needs to be used if the original string might fulfill any
+ * - It contains curly braces
+ * - It is enclosed in single quotes
+ * - It ends with a "\" */
+void pa_message_param_write_string(pa_message_param *param, const char *value, bool do_escape) {
I would drop the do_escape flag, and if there's need for appending
unescaped raw data to pa_message_params, there could be a separate
function for that.
See above. A function for writing raw data should nevertheless be provided
for the case where a string can only be written as several parts.
Post by Tanu Kaskinen
Post by Georg Chini
+ const char *s;
+ char *output, *out_char;
+ size_t brace_count;
+
+ pa_assert(param);
+
+ /* Null value is written as empty element */
+ if (!value)
+ value = "";
+
+ if (!do_escape) {
+ pa_strbuf_printf(param->buffer, "{%s}", value);
+ return;
+ }
+
+ /* Using pa_strbuf_putc() to write to the strbuf while iterating over
+ * the input string would cause the allocation of a linked list element
+ * for each character of the input string. Therefore the output string
+ * is constructed locally before writing it to the buffer */
This is an interesting point. I think you should improve pa_escape(),
which currently uses the naive pa_strbuf_putc() method. With my
proposed escaping rules, you could then replace this code with a call
to pa_escape().
OK, I'll do that and send a patch for tweaking pa_escape()/pa_unescape()
to my needs separately.
Post by Tanu Kaskinen
Post by Georg Chini
+
+ /* Count number of characters to escape */
+ brace_count = 0;
+ for (s = value; *s; ++s) {
+ if (*s == '{' || *s == '}')
+ brace_count++;
+ }
You could at the same time count all characters to save the strlen()
call.
Mh, do I really save something? If the string is x characters
long, I would increment a variable this often. Is that less work
than a strlen() call? But probably strlen() does something similar ...
strlen() loops over all characters. One loop is better than two. Not
that this is likely to cause any noticeable slowdown, unnecessary
strlen() calls just tend to catch my attention...
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+/* Check if a path string starts with a / and only contains valid characters */
+static bool object_path_is_valid(const char *test_string) {
uint32_t i;
+ if (!test_string)
+ return false;
+
+ /* Make sure the string starts with / and does not end with a / */
+ if (test_string[0] != '/' || test_string[strlen(test_string) - 1] == '/')
+ return false;
+
for (i = 0; test_string[i]; i++) {
- if (test_string[i] == '{' ||
- test_string[i] == '}')
- return false;
+
+ if ((test_string[i] >= 'a' && test_string[i] <= 'z') ||
+ (test_string[i] >= 'A' && test_string[i] <= 'Z') ||
+ (test_string[i] >= '0' && test_string[i] <= '9') ||
+ test_string[i] == '.' ||
+ test_string[i] == '_' ||
+ test_string[i] == '-' ||
+ test_string[i] == '/')
+ continue;
+
+ return false;
}
You could save the strlen() call if you checked the trailing slash
after this for loop when you're at the end of the string.
By the way, do you perhaps want to disallow two subsequent slashes in
object paths? (I don't care myself, because I doubt that invalid paths
will ever be encountered anyway when registering message handlers.)
If I don't disallow it, then a double slash in the path will be valid.
At least up to now we do not care about the individual components
that the path is constructed from.
So should I disallow it or not?
As I said, I don't care. Clients don't register message handlers, so
we're dealing only with server code, and I find it unlikely that code
that generates weird paths would get past review.

If I have to have an opinion, then this is it: if we're anyway
validating the paths, disallowing double slashes would be logical,
becacuse I can't imagine such paths ever getting created on purpose.
--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
Georg Chini
2018-07-22 19:11:17 UTC
Permalink
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
The patch adds the possibility to escape curly braces within parameter strings
and introduces several new functions that can be used for writing parameters.
For writing, the structure pa_message_param, which is a wrapper for pa_strbuf
pa_message_param_new() - creates a new pa_message_param structure
pa_message_param_to_string() - converts a pa_message_param to string and frees
the structure
The function pa_message_param_write_string()
has a parameter do_escape.
Why not do escaping always?
Because what you are writing as a string may be a list that you have
prepared
previously. Then you will not want escaping. You may for example create
a list
from an array and then insert this list as one string into the final
parameter list
or have a function that converts a certain structure to a parameter
string and
then write the result of this function as one element of the final list.
My mental model is that parameters have types, list type is different
than string type, and write_string() is only meant for writing values
of the string type.
Can you add a write_raw() function?
Yes, this is done in patch 7. But the raw write function differs from what
write_string() is doing. write_string() writes one element of a list,
that is
it encloses the string in braces. The raw write function is intended for
situations where you can't write a complete element with one write, so
it does not add any braces. I am still of the opinion, that a structure
or array converted to a parameter string is a string, so writing something
like this should be done with write_string().
Also writing unescaped strings in situations where escaping is not necessary
saves the overhead of looping over all the characters.
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
core-util already contains pa_unescape() that does the same thing more
efficiently (if you drop the single quote thing).
pa_unescape() currently does not do the same thing. It removes all
escape characters, while I only want to remove the characters
I actually introduced (those before { or }).
I can however modify pa_unescape() to take the same arguments
as pa_escape().
I don't see the need for being selective when unescaping. Nothing
breaks if all (except escaped) backslashes are stripped.
You are right, if previously all backslashes in the original string
have been escaped, nothing will break. I was still thinking of the
old solution where I did not escape backslashes.
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
It may be possible, that the string you read is a list. Consider the
following
parameter list: {string1}{some nested structure}{string2}. You can now
read this list as three strings and then continue to read the elements of
the nested structure from the second string. You might even create a
function
that takes a string and outputs a structure. So you are not forced to go
to the full depth of nesting on the first pass. This makes it much easier
to handle deeply nested parameter lists. For me this behavior is an
important
feature and I do not want to drop it. See also my comment on why I do
not always want escaping.
Doesn't split_list() already allow this, why do you want to use
read_string() to do the same thing as split_list()?
read_string() and split_list() are very similar and we could live
without read_string(). It is provided as a counterpart to write_string()
and for convenience additionally does the unescaping if necessary
like write_string does the escaping.
I don't see why this is a problem. It depends on the context which
is the better function to use.
Tanu Kaskinen
2018-07-26 10:37:53 UTC
Permalink
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
The patch adds the possibility to escape curly braces within parameter strings
and introduces several new functions that can be used for writing parameters.
For writing, the structure pa_message_param, which is a wrapper for pa_strbuf
pa_message_param_new() - creates a new pa_message_param structure
pa_message_param_to_string() - converts a pa_message_param to string and frees
the structure
The function pa_message_param_write_string()
has a parameter do_escape.
Why not do escaping always?
Because what you are writing as a string may be a list that you have
prepared
previously. Then you will not want escaping. You may for example create
a list
from an array and then insert this list as one string into the final
parameter list
or have a function that converts a certain structure to a parameter
string and
then write the result of this function as one element of the final list.
My mental model is that parameters have types, list type is different
than string type, and write_string() is only meant for writing values
of the string type.
Can you add a write_raw() function?
Yes, this is done in patch 7. But the raw write function differs from what
write_string() is doing. write_string() writes one element of a list,
that is
it encloses the string in braces. The raw write function is intended for
situations where you can't write a complete element with one write, so
it does not add any braces. I am still of the opinion, that a structure
or array converted to a parameter string is a string, so writing something
like this should be done with write_string().
They are different kinds of strings, different abstraction levels. When
you're writing an array "as a string", in that context it's just a C
string. write_string() with escaping deals with strings in the "message
params type system". I don't know if this makes any sense to you.
Probably not... In any case, the do_escape flag seems unnecessary
complexity to me.
Post by Georg Chini
Also writing unescaped strings in situations where escaping is not necessary
saves the overhead of looping over all the characters.
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
core-util already contains pa_unescape() that does the same thing more
efficiently (if you drop the single quote thing).
pa_unescape() currently does not do the same thing. It removes all
escape characters, while I only want to remove the characters
I actually introduced (those before { or }).
I can however modify pa_unescape() to take the same arguments
as pa_escape().
I don't see the need for being selective when unescaping. Nothing
breaks if all (except escaped) backslashes are stripped.
You are right, if previously all backslashes in the original string
have been escaped, nothing will break. I was still thinking of the
old solution where I did not escape backslashes.
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
It may be possible, that the string you read is a list. Consider the
following
parameter list: {string1}{some nested structure}{string2}. You can now
read this list as three strings and then continue to read the elements of
the nested structure from the second string. You might even create a
function
that takes a string and outputs a structure. So you are not forced to go
to the full depth of nesting on the first pass. This makes it much easier
to handle deeply nested parameter lists. For me this behavior is an
important
feature and I do not want to drop it. See also my comment on why I do
not always want escaping.
Doesn't split_list() already allow this, why do you want to use
read_string() to do the same thing as split_list()?
read_string() and split_list() are very similar and we could live
without read_string(). It is provided as a counterpart to write_string()
and for convenience additionally does the unescaping if necessary
like write_string does the escaping.
I don't see why this is a problem. It depends on the context which
is the better function to use.
Again, in my mind a structure is not a string, they are different
types, and I think read_string() should only deal with the string type.
is_unpacked makes the API more complicated, so I'd like to get rid of
it.
--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
Georg Chini
2018-07-26 16:02:11 UTC
Permalink
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
The patch adds the possibility to escape curly braces within parameter strings
and introduces several new functions that can be used for writing parameters.
For writing, the structure pa_message_param, which is a wrapper for pa_strbuf
pa_message_param_new() - creates a new pa_message_param structure
pa_message_param_to_string() - converts a pa_message_param to string and frees
the structure
The function pa_message_param_write_string()
has a parameter do_escape.
Why not do escaping always?
Because what you are writing as a string may be a list that you have
prepared
previously. Then you will not want escaping. You may for example create
a list
from an array and then insert this list as one string into the final
parameter list
or have a function that converts a certain structure to a parameter
string and
then write the result of this function as one element of the final list.
My mental model is that parameters have types, list type is different
than string type, and write_string() is only meant for writing values
of the string type.
Can you add a write_raw() function?
Yes, this is done in patch 7. But the raw write function differs from what
write_string() is doing. write_string() writes one element of a list,
that is
it encloses the string in braces. The raw write function is intended for
situations where you can't write a complete element with one write, so
it does not add any braces. I am still of the opinion, that a structure
or array converted to a parameter string is a string, so writing something
like this should be done with write_string().
They are different kinds of strings, different abstraction levels. When
you're writing an array "as a string", in that context it's just a C
string. write_string() with escaping deals with strings in the "message
params type system". I don't know if this makes any sense to you.
Probably not... In any case, the do_escape flag seems unnecessary
complexity to me.
The alternative would be a function to write an unescaped string in
addition to the write_raw() function. If you don't like the flag, would
you be OK with a write_unescaped_string() function? I think it is just
more comfortable than using write_raw().
Post by Tanu Kaskinen
Post by Georg Chini
Also writing unescaped strings in situations where escaping is not necessary
saves the overhead of looping over all the characters.
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
core-util already contains pa_unescape() that does the same thing more
efficiently (if you drop the single quote thing).
pa_unescape() currently does not do the same thing. It removes all
escape characters, while I only want to remove the characters
I actually introduced (those before { or }).
I can however modify pa_unescape() to take the same arguments
as pa_escape().
I don't see the need for being selective when unescaping. Nothing
breaks if all (except escaped) backslashes are stripped.
You are right, if previously all backslashes in the original string
have been escaped, nothing will break. I was still thinking of the
old solution where I did not escape backslashes.
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
It may be possible, that the string you read is a list. Consider the
following
parameter list: {string1}{some nested structure}{string2}. You can now
read this list as three strings and then continue to read the elements of
the nested structure from the second string. You might even create a
function
that takes a string and outputs a structure. So you are not forced to go
to the full depth of nesting on the first pass. This makes it much easier
to handle deeply nested parameter lists. For me this behavior is an
important
feature and I do not want to drop it. See also my comment on why I do
not always want escaping.
Doesn't split_list() already allow this, why do you want to use
read_string() to do the same thing as split_list()?
read_string() and split_list() are very similar and we could live
without read_string(). It is provided as a counterpart to write_string()
and for convenience additionally does the unescaping if necessary
like write_string does the escaping.
I don't see why this is a problem. It depends on the context which
is the better function to use.
Again, in my mind a structure is not a string, they are different
types, and I think read_string() should only deal with the string type.
is_unpacked makes the API more complicated, so I'd like to get rid of
it.
I don't see your point. is_unpacked is not part of the read_string()
arguments
or return value. In split_list() it is definitively needed to indicate
if the returned
string (in the C sense) contains another list. I can imagine many
situations where
you might either pass an array or just a single value or even NULL.
is_unpacked
allows to differentiate between the situations. And I don't think that a
boolean
return value is adding significantly to the complexity of a function,
especially
since passing NULL is allowed if you do not care for the value.
Tanu Kaskinen
2018-07-27 08:08:25 UTC
Permalink
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
The patch adds the possibility to escape curly braces within parameter strings
and introduces several new functions that can be used for writing parameters.
For writing, the structure pa_message_param, which is a wrapper for pa_strbuf
pa_message_param_new() - creates a new pa_message_param structure
pa_message_param_to_string() - converts a pa_message_param to string and frees
the structure
The function pa_message_param_write_string()
has a parameter do_escape.
Why not do escaping always?
Because what you are writing as a string may be a list that you have
prepared
previously. Then you will not want escaping. You may for example create
a list
from an array and then insert this list as one string into the final
parameter list
or have a function that converts a certain structure to a parameter
string and
then write the result of this function as one element of the final list.
My mental model is that parameters have types, list type is different
than string type, and write_string() is only meant for writing values
of the string type.
Can you add a write_raw() function?
Yes, this is done in patch 7. But the raw write function differs from what
write_string() is doing. write_string() writes one element of a list,
that is
it encloses the string in braces. The raw write function is intended for
situations where you can't write a complete element with one write, so
it does not add any braces. I am still of the opinion, that a structure
or array converted to a parameter string is a string, so writing something
like this should be done with write_string().
They are different kinds of strings, different abstraction levels. When
you're writing an array "as a string", in that context it's just a C
string. write_string() with escaping deals with strings in the "message
params type system". I don't know if this makes any sense to you.
Probably not... In any case, the do_escape flag seems unnecessary
complexity to me.
The alternative would be a function to write an unescaped string in
addition to the write_raw() function. If you don't like the flag, would
you be OK with a write_unescaped_string() function? I think it is just
more comfortable than using write_raw().
Thanks for the concession, I was afraid we'll get stuck on this point.

By comfortable, do you refer to that write_raw() doesn't add braces
around the value? How about adding an add_braces flag to write_raw()?
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
It may be possible, that the string you read is a list. Consider the
following
parameter list: {string1}{some nested structure}{string2}. You can now
read this list as three strings and then continue to read the elements of
the nested structure from the second string. You might even create a
function
that takes a string and outputs a structure. So you are not forced to go
to the full depth of nesting on the first pass. This makes it much easier
to handle deeply nested parameter lists. For me this behavior is an
important
feature and I do not want to drop it. See also my comment on why I do
not always want escaping.
Doesn't split_list() already allow this, why do you want to use
read_string() to do the same thing as split_list()?
read_string() and split_list() are very similar and we could live
without read_string(). It is provided as a counterpart to write_string()
and for convenience additionally does the unescaping if necessary
like write_string does the escaping.
I don't see why this is a problem. It depends on the context which
is the better function to use.
Again, in my mind a structure is not a string, they are different
types, and I think read_string() should only deal with the string type.
is_unpacked makes the API more complicated, so I'd like to get rid of
it.
I don't see your point. is_unpacked is not part of the read_string()
arguments
or return value. In split_list() it is definitively needed to indicate
if the returned
string (in the C sense) contains another list. I can imagine many
situations where
you might either pass an array or just a single value or even NULL.
is_unpacked
allows to differentiate between the situations.
Can you give an example? You say is_unpacked is definitely needed, but
so far the only use case has been read_string(), which you wanted to be
used for reading not only string values but everything else too. If
read_string() is changed to only read string values, then it doesn't
need is_unpacked from split_list().

The parameter types are known beforehand, so i don't see the need for
looking at the data to figure out the type. If introspection support is
desired, then that's a separate project (the is_unpacked flag isn't
sufficient for proper introspection).
--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
Georg Chini
2018-07-27 08:51:21 UTC
Permalink
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
The patch adds the possibility to escape curly braces within parameter strings
and introduces several new functions that can be used for writing parameters.
For writing, the structure pa_message_param, which is a wrapper for pa_strbuf
pa_message_param_new() - creates a new pa_message_param structure
pa_message_param_to_string() - converts a pa_message_param to string and frees
the structure
The function pa_message_param_write_string()
has a parameter do_escape.
Why not do escaping always?
Because what you are writing as a string may be a list that you have
prepared
previously. Then you will not want escaping. You may for example create
a list
from an array and then insert this list as one string into the final
parameter list
or have a function that converts a certain structure to a parameter
string and
then write the result of this function as one element of the final list.
My mental model is that parameters have types, list type is different
than string type, and write_string() is only meant for writing values
of the string type.
Can you add a write_raw() function?
Yes, this is done in patch 7. But the raw write function differs from what
write_string() is doing. write_string() writes one element of a list,
that is
it encloses the string in braces. The raw write function is intended for
situations where you can't write a complete element with one write, so
it does not add any braces. I am still of the opinion, that a structure
or array converted to a parameter string is a string, so writing something
like this should be done with write_string().
They are different kinds of strings, different abstraction levels. When
you're writing an array "as a string", in that context it's just a C
string. write_string() with escaping deals with strings in the "message
params type system". I don't know if this makes any sense to you.
Probably not... In any case, the do_escape flag seems unnecessary
complexity to me.
The alternative would be a function to write an unescaped string in
addition to the write_raw() function. If you don't like the flag, would
you be OK with a write_unescaped_string() function? I think it is just
more comfortable than using write_raw().
Thanks for the concession, I was afraid we'll get stuck on this point.
By comfortable, do you refer to that write_raw() doesn't add braces
around the value? How about adding an add_braces flag to write_raw()?
Yes, that's a good idea. I'll do that and remove the
do_escape flag from write_string().
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
It may be possible, that the string you read is a list. Consider the
following
parameter list: {string1}{some nested structure}{string2}. You can now
read this list as three strings and then continue to read the elements of
the nested structure from the second string. You might even create a
function
that takes a string and outputs a structure. So you are not forced to go
to the full depth of nesting on the first pass. This makes it much easier
to handle deeply nested parameter lists. For me this behavior is an
important
feature and I do not want to drop it. See also my comment on why I do
not always want escaping.
Doesn't split_list() already allow this, why do you want to use
read_string() to do the same thing as split_list()?
read_string() and split_list() are very similar and we could live
without read_string(). It is provided as a counterpart to write_string()
and for convenience additionally does the unescaping if necessary
like write_string does the escaping.
I don't see why this is a problem. It depends on the context which
is the better function to use.
Again, in my mind a structure is not a string, they are different
types, and I think read_string() should only deal with the string type.
is_unpacked makes the API more complicated, so I'd like to get rid of
it.
I don't see your point. is_unpacked is not part of the read_string()
arguments
or return value. In split_list() it is definitively needed to indicate
if the returned
string (in the C sense) contains another list. I can imagine many
situations where
you might either pass an array or just a single value or even NULL.
is_unpacked
allows to differentiate between the situations.
Can you give an example? You say is_unpacked is definitely needed, but
so far the only use case has been read_string(), which you wanted to be
used for reading not only string values but everything else too. If
read_string() is changed to only read string values, then it doesn't
need is_unpacked from split_list().
The parameter types are known beforehand, so i don't see the need for
looking at the data to figure out the type. If introspection support is
desired, then that's a separate project (the is_unpacked flag isn't
sufficient for proper introspection).
This is not about parameter type, it is just to distinguish between
a list and a simple value. One example comes to my mind immediately:
Consider a parameter list that consists of strings and a couple of
arrays. Now you can read this list as an array of strings (patch 8
provides a function for that) and then examine those strings that
contain arrays separately. Having the flag (and using it in read_string())
provides a more flexible and convenient way to parse a parameter list.

The flag may not be strictly necessary at the moment, but I would still
like to keep it.
Tanu Kaskinen
2018-07-29 19:47:46 UTC
Permalink
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
It may be possible, that the string you read is a list. Consider the
following
parameter list: {string1}{some nested structure}{string2}. You can now
read this list as three strings and then continue to read the elements of
the nested structure from the second string. You might even create a
function
that takes a string and outputs a structure. So you are not forced to go
to the full depth of nesting on the first pass. This makes it much easier
to handle deeply nested parameter lists. For me this behavior is an
important
feature and I do not want to drop it. See also my comment on why I do
not always want escaping.
Doesn't split_list() already allow this, why do you want to use
read_string() to do the same thing as split_list()?
read_string() and split_list() are very similar and we could live
without read_string(). It is provided as a counterpart to write_string()
and for convenience additionally does the unescaping if necessary
like write_string does the escaping.
I don't see why this is a problem. It depends on the context which
is the better function to use.
Again, in my mind a structure is not a string, they are different
types, and I think read_string() should only deal with the string type.
is_unpacked makes the API more complicated, so I'd like to get rid of
it.
I don't see your point. is_unpacked is not part of the read_string()
arguments
or return value. In split_list() it is definitively needed to indicate
if the returned
string (in the C sense) contains another list. I can imagine many
situations where
you might either pass an array or just a single value or even NULL.
is_unpacked
allows to differentiate between the situations.
Can you give an example? You say is_unpacked is definitely needed, but
so far the only use case has been read_string(), which you wanted to be
used for reading not only string values but everything else too. If
read_string() is changed to only read string values, then it doesn't
need is_unpacked from split_list().
The parameter types are known beforehand, so i don't see the need for
looking at the data to figure out the type. If introspection support is
desired, then that's a separate project (the is_unpacked flag isn't
sufficient for proper introspection).
This is not about parameter type, it is just to distinguish between
Consider a parameter list that consists of strings and a couple of
arrays. Now you can read this list as an array of strings (patch 8
provides a function for that) and then examine those strings that
contain arrays separately. Having the flag (and using it in read_string())
provides a more flexible and convenient way to parse a parameter list.
The flag may not be strictly necessary at the moment, but I would still
like to keep it.
To continue a familiar theme of my review: if there's a
read_string_array() function, I want that to be used only for string
arrays, not a mishmash of random types. There could be a separate
function split_list_into_array() that does something similar to what you wanted to do with read_string_array(). split_list_into_array() wouldn't do special string handling, though, so unescaping would be left to the application. I find that only logical, since other basic types don't get special handling either (i.e. floats aren't converted to C floats).

Your use case could be served with a vararg function that instead of
producing a string array would read into separate variables, like this:

pa_message_params_read(c, state,
PA_TYPE_STRING, &param1,
PA_TYPE_FLOAT, &param2,
PA_TYPE_RAW, &param3,
0);

PA_TYPE_RAW would be useful for reading a list (or anything else) into
a C string without unescaping or other processing. There could be also
PA_TYPE_IGNORE for uninteresting variables, and PA_TYPE_*_ARRAY for
arrays of basic types.

(Now it occurred to me that the 'c' argument name that is used in the
parsing functions is a bit weird and unhelpful. Maybe "param_list"
would be good?)
--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
Georg Chini
2018-07-29 20:36:56 UTC
Permalink
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
It may be possible, that the string you read is a list. Consider the
following
parameter list: {string1}{some nested structure}{string2}. You can now
read this list as three strings and then continue to read the elements of
the nested structure from the second string. You might even create a
function
that takes a string and outputs a structure. So you are not forced to go
to the full depth of nesting on the first pass. This makes it much easier
to handle deeply nested parameter lists. For me this behavior is an
important
feature and I do not want to drop it. See also my comment on why I do
not always want escaping.
Doesn't split_list() already allow this, why do you want to use
read_string() to do the same thing as split_list()?
read_string() and split_list() are very similar and we could live
without read_string(). It is provided as a counterpart to write_string()
and for convenience additionally does the unescaping if necessary
like write_string does the escaping.
I don't see why this is a problem. It depends on the context which
is the better function to use.
Again, in my mind a structure is not a string, they are different
types, and I think read_string() should only deal with the string type.
is_unpacked makes the API more complicated, so I'd like to get rid of
it.
I don't see your point. is_unpacked is not part of the read_string()
arguments
or return value. In split_list() it is definitively needed to indicate
if the returned
string (in the C sense) contains another list. I can imagine many
situations where
you might either pass an array or just a single value or even NULL.
is_unpacked
allows to differentiate between the situations.
Can you give an example? You say is_unpacked is definitely needed, but
so far the only use case has been read_string(), which you wanted to be
used for reading not only string values but everything else too. If
read_string() is changed to only read string values, then it doesn't
need is_unpacked from split_list().
The parameter types are known beforehand, so i don't see the need for
looking at the data to figure out the type. If introspection support is
desired, then that's a separate project (the is_unpacked flag isn't
sufficient for proper introspection).
This is not about parameter type, it is just to distinguish between
Consider a parameter list that consists of strings and a couple of
arrays. Now you can read this list as an array of strings (patch 8
provides a function for that) and then examine those strings that
contain arrays separately. Having the flag (and using it in read_string())
provides a more flexible and convenient way to parse a parameter list.
The flag may not be strictly necessary at the moment, but I would still
like to keep it.
To continue a familiar theme of my review: if there's a
read_string_array() function, I want that to be used only for string
arrays, not a mishmash of random types. There could be a separate
function split_list_into_array() that does something similar to what you wanted to do with read_string_array(). split_list_into_array() wouldn't do special string handling, though, so unescaping would be left to the application. I find that only logical, since other basic types don't get special handling either (i.e. floats aren't converted to C floats).
Your use case could be served with a vararg function that instead of
pa_message_params_read(c, state,
PA_TYPE_STRING, &param1,
PA_TYPE_FLOAT, &param2,
PA_TYPE_RAW, &param3,
0);
PA_TYPE_RAW would be useful for reading a list (or anything else) into
a C string without unescaping or other processing. There could be also
PA_TYPE_IGNORE for uninteresting variables, and PA_TYPE_*_ARRAY for
arrays of basic types.
(Now it occurred to me that the 'c' argument name that is used in the
parsing functions is a bit weird and unhelpful. Maybe "param_list"
would be good?)
I still don't see your point. Again, the use of is_unpacked is
transparent for the
user of read_string(), so the user just reads a string without having to
care about
unescaping. The flag does not complicate the API, it simplifies it
because the
unescaping is done automatically. Your approach seems unnecessary
complicated to me. A string is a string, even if it contains
sub-structures. Your
split_list_into_array() function would basically return an array of
strings, so what
would be the advantage? It would only make parsing more cumbersome, because
the task of unescaping is given to the user instead of doing it
automatically where
necessary.
Also there is no "mishmash of random types", they are all strings It is
only the
difference between a "simple" string which needs no further processing and a
"complex" string which needs further parsing.

I am still not willing to drop this - for me it simplifies parsing. How
do we proceed?
Tanu Kaskinen
2018-07-30 09:18:08 UTC
Permalink
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
It may be possible, that the string you read is a list. Consider the
following
parameter list: {string1}{some nested structure}{string2}. You can now
read this list as three strings and then continue to read the elements of
the nested structure from the second string. You might even create a
function
that takes a string and outputs a structure. So you are not forced to go
to the full depth of nesting on the first pass. This makes it much easier
to handle deeply nested parameter lists. For me this behavior is an
important
feature and I do not want to drop it. See also my comment on why I do
not always want escaping.
Doesn't split_list() already allow this, why do you want to use
read_string() to do the same thing as split_list()?
read_string() and split_list() are very similar and we could live
without read_string(). It is provided as a counterpart to write_string()
and for convenience additionally does the unescaping if necessary
like write_string does the escaping.
I don't see why this is a problem. It depends on the context which
is the better function to use.
Again, in my mind a structure is not a string, they are different
types, and I think read_string() should only deal with the string type.
is_unpacked makes the API more complicated, so I'd like to get rid of
it.
I don't see your point. is_unpacked is not part of the read_string()
arguments
or return value. In split_list() it is definitively needed to indicate
if the returned
string (in the C sense) contains another list. I can imagine many
situations where
you might either pass an array or just a single value or even NULL.
is_unpacked
allows to differentiate between the situations.
Can you give an example? You say is_unpacked is definitely needed, but
so far the only use case has been read_string(), which you wanted to be
used for reading not only string values but everything else too. If
read_string() is changed to only read string values, then it doesn't
need is_unpacked from split_list().
The parameter types are known beforehand, so i don't see the need for
looking at the data to figure out the type. If introspection support is
desired, then that's a separate project (the is_unpacked flag isn't
sufficient for proper introspection).
This is not about parameter type, it is just to distinguish between
Consider a parameter list that consists of strings and a couple of
arrays. Now you can read this list as an array of strings (patch 8
provides a function for that) and then examine those strings that
contain arrays separately. Having the flag (and using it in read_string())
provides a more flexible and convenient way to parse a parameter list.
The flag may not be strictly necessary at the moment, but I would still
like to keep it.
To continue a familiar theme of my review: if there's a
read_string_array() function, I want that to be used only for string
arrays, not a mishmash of random types. There could be a separate
function split_list_into_array() that does something similar to
what you wanted to do with read_string_array().
split_list_into_array() wouldn't do special string handling,
though, so unescaping would be left to the application. I find that
only logical, since other basic types don't get special handling
either (i.e. floats aren't converted to C floats).
Your use case could be served with a vararg function that instead of
pa_message_params_read(c, state,
PA_TYPE_STRING, &param1,
PA_TYPE_FLOAT, &param2,
PA_TYPE_RAW, &param3,
0);
PA_TYPE_RAW would be useful for reading a list (or anything else) into
a C string without unescaping or other processing. There could be also
PA_TYPE_IGNORE for uninteresting variables, and PA_TYPE_*_ARRAY for
arrays of basic types.
(Now it occurred to me that the 'c' argument name that is used in the
parsing functions is a bit weird and unhelpful. Maybe "param_list"
would be good?)
I still don't see your point. Again, the use of is_unpacked is
transparent for the
user of read_string(), so the user just reads a string without having to
care about
unescaping. The flag does not complicate the API, it simplifies it
because the
unescaping is done automatically.
The flag complicates the split_list() function parameters, but also the
read_string() semantics: you need to explain to the user that unlike
all the other read_foo() functions, read_string() can read any value
and unescaping becomes conditional because of this.
Post by Georg Chini
Your approach seems unnecessary
complicated to me. A string is a string, even if it contains
sub-structures. Your
split_list_into_array() function would basically return an array of
strings, so what
would be the advantage? It would only make parsing more cumbersome, because
the task of unescaping is given to the user instead of doing it
automatically where
necessary.
That's why I came up with the vararg function. I agree that
split_list_into_array() is unlikely to be very useful.
Post by Georg Chini
Also there is no "mishmash of random types", they are all strings It is
only the
difference between a "simple" string which needs no further processing and a
"complex" string which needs further parsing.
The API defines types for the parameters. There are no separate
"simple" and "complex" strings in the type system. The string type is
different than the list type. What you call complex strings are in my
mind just the raw unprocessed serialized data, which is a different
abstraction level than the values that the various read_foo() functions
return. It feels just very wrong to have a function that returns values
in both domains: unparsed raw data and parsed values. Especially when
the function is a family of read_foo() functions where all other
functions only operate on their designated types.

Compare to D-Bus: if you say that "they are all strings", then in D-Bus
the same would be "they are all byte arrays", since that's what they
are in the message buffer. I doubt you would suggest that the
dbus_message_iter_read_byte_array() function (if such thing existed)
should be usable on arbitrarily typed values rather than just byte
array values, and should return the raw serialized data regardless of
its type. Here you're suggesting that read_string() should be able to
return the raw serialized data regardless of its type.

Do you understand what I'm getting at? You may disagree that having
strict separation of concerns between raw serialized data and parsed
values is beneficial for the user-friendliness of the API, but if you
don't even understand what I mean by that separation, then discussing
this is very hard.
--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
Georg Chini
2018-07-30 11:23:55 UTC
Permalink
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
This is not about parameter type, it is just to distinguish between
Consider a parameter list that consists of strings and a couple of
arrays. Now you can read this list as an array of strings (patch 8
provides a function for that) and then examine those strings that
contain arrays separately. Having the flag (and using it in read_string())
provides a more flexible and convenient way to parse a parameter list.
The flag may not be strictly necessary at the moment, but I would still
like to keep it.
To continue a familiar theme of my review: if there's a
read_string_array() function, I want that to be used only for string
arrays, not a mishmash of random types. There could be a separate
function split_list_into_array() that does something similar to
what you wanted to do with read_string_array().
split_list_into_array() wouldn't do special string handling,
though, so unescaping would be left to the application. I find that
only logical, since other basic types don't get special handling
either (i.e. floats aren't converted to C floats).
Your use case could be served with a vararg function that instead of
pa_message_params_read(c, state,
PA_TYPE_STRING, &param1,
PA_TYPE_FLOAT, &param2,
PA_TYPE_RAW, &param3,
0);
PA_TYPE_RAW would be useful for reading a list (or anything else) into
a C string without unescaping or other processing. There could be also
PA_TYPE_IGNORE for uninteresting variables, and PA_TYPE_*_ARRAY for
arrays of basic types.
(Now it occurred to me that the 'c' argument name that is used in the
parsing functions is a bit weird and unhelpful. Maybe "param_list"
would be good?)
I still don't see your point. Again, the use of is_unpacked is
transparent for the
user of read_string(), so the user just reads a string without having to
care about
unescaping. The flag does not complicate the API, it simplifies it
because the
unescaping is done automatically.
The flag complicates the split_list() function parameters, but also the
read_string() semantics: you need to explain to the user that unlike
all the other read_foo() functions, read_string() can read any value
and unescaping becomes conditional because of this.
read_string() is not supposed to read any value, it is only supposed
to read strings. Actually, the user does not need to know anything
about escaping, because read_string() and write_string() do the
necessary escaping/unescaping completely transparent for the user.

The is_unpacked flag is at least useful to check if something returned
by split_list() is really a simple type and not a structure or array. I am
currently not using it in the read_foo() functions, but I think I should.
Post by Tanu Kaskinen
Post by Georg Chini
Your approach seems unnecessary
complicated to me. A string is a string, even if it contains
sub-structures. Your
split_list_into_array() function would basically return an array of
strings, so what
would be the advantage? It would only make parsing more cumbersome, because
the task of unescaping is given to the user instead of doing it
automatically where
necessary.
That's why I came up with the vararg function. I agree that
split_list_into_array() is unlikely to be very useful.
Post by Georg Chini
Also there is no "mishmash of random types", they are all strings It is
only the
difference between a "simple" string which needs no further processing and a
"complex" string which needs further parsing.
The API defines types for the parameters. There are no separate
"simple" and "complex" strings in the type system. The string type is
different than the list type. What you call complex strings are in my
mind just the raw unprocessed serialized data, which is a different
abstraction level than the values that the various read_foo() functions
return. It feels just very wrong to have a function that returns values
in both domains: unparsed raw data and parsed values. Especially when
the function is a family of read_foo() functions where all other
functions only operate on their designated types.
Yes, that is some additional functionality that the read_string()
function provides on top of reading what I called "simple strings"
above. But that does not hinder the function to work exactly
like expected when reading a plain string. Maybe a better name
for the function would be read_string_element(), indicating that
it can read either a plain string or an element of the parameter
list as a string.
I could split this into two functions, read_string() which always
does unescaping and read_element() which would be a wrapper
around split_list() and would never do unescaping. In both functions,
the is_unpacked flag can be useful to check if the input matches
the type "plain string" or "serialized data".
Post by Tanu Kaskinen
Compare to D-Bus: if you say that "they are all strings", then in D-Bus
the same would be "they are all byte arrays", since that's what they
are in the message buffer. I doubt you would suggest that the
dbus_message_iter_read_byte_array() function (if such thing existed)
should be usable on arbitrarily typed values rather than just byte
array values, and should return the raw serialized data regardless of
its type. Here you're suggesting that read_string() should be able to
return the raw serialized data regardless of its type.
Do you understand what I'm getting at? You may disagree that having
strict separation of concerns between raw serialized data and parsed
values is beneficial for the user-friendliness of the API, but if you
don't even understand what I mean by that separation, then discussing
this is very hard.
I do understand your reasoning and technically we surely could drop
the flag. I just don't want it because for me using read_string() is a
more convenient way of parsing a parameter list than using split_list()
directly. It's just a simple way to say "let's skip this array or structure
for the moment and store it somewhere for later inspection". So I can
do something like

x = read_float()
y = read_int()
my_structure = read_string()
z = read_bool()
a = read_string()

and then examine my_structure at a later time. Using split_list() instead
of read_string() is somehow breaking the flow for me. We could do the
same with the read_element() function I suggested above (and then
maybe make split_list() a purely internal function?)
Tanu Kaskinen
2018-08-01 11:13:49 UTC
Permalink
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
This is not about parameter type, it is just to distinguish between
Consider a parameter list that consists of strings and a couple of
arrays. Now you can read this list as an array of strings (patch 8
provides a function for that) and then examine those strings that
contain arrays separately. Having the flag (and using it in read_string())
provides a more flexible and convenient way to parse a parameter list.
The flag may not be strictly necessary at the moment, but I would still
like to keep it.
To continue a familiar theme of my review: if there's a
read_string_array() function, I want that to be used only for string
arrays, not a mishmash of random types. There could be a separate
function split_list_into_array() that does something similar to
what you wanted to do with read_string_array().
split_list_into_array() wouldn't do special string handling,
though, so unescaping would be left to the application. I find that
only logical, since other basic types don't get special handling
either (i.e. floats aren't converted to C floats).
Your use case could be served with a vararg function that instead of
pa_message_params_read(c, state,
PA_TYPE_STRING, &param1,
PA_TYPE_FLOAT, &param2,
PA_TYPE_RAW, &param3,
0);
PA_TYPE_RAW would be useful for reading a list (or anything else) into
a C string without unescaping or other processing. There could be also
PA_TYPE_IGNORE for uninteresting variables, and PA_TYPE_*_ARRAY for
arrays of basic types.
(Now it occurred to me that the 'c' argument name that is used in the
parsing functions is a bit weird and unhelpful. Maybe "param_list"
would be good?)
I still don't see your point. Again, the use of is_unpacked is
transparent for the
user of read_string(), so the user just reads a string without having to
care about
unescaping. The flag does not complicate the API, it simplifies it
because the
unescaping is done automatically.
The flag complicates the split_list() function parameters, but also the
read_string() semantics: you need to explain to the user that unlike
all the other read_foo() functions, read_string() can read any value
and unescaping becomes conditional because of this.
read_string() is not supposed to read any value, it is only supposed
to read strings. Actually, the user does not need to know anything
about escaping, because read_string() and write_string() do the
necessary escaping/unescaping completely transparent for the user.
The is_unpacked flag is at least useful to check if something returned
by split_list() is really a simple type and not a structure or array. I am
currently not using it in the read_foo() functions, but I think I should.
I guess you'd use is_unpacked for catching errors in the read_foo()
functions? That seems reasonable, but I'd like that to be done in an
internal function. You said that maybe split_list() could be made an
internal function, and that seems like a good idea. An alternative
would be to have an internal function for which split_list() would be a
simplified wrapper.
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Your approach seems unnecessary
complicated to me. A string is a string, even if it contains
sub-structures. Your
split_list_into_array() function would basically return an array of
strings, so what
would be the advantage? It would only make parsing more cumbersome, because
the task of unescaping is given to the user instead of doing it
automatically where
necessary.
That's why I came up with the vararg function. I agree that
split_list_into_array() is unlikely to be very useful.
Post by Georg Chini
Also there is no "mishmash of random types", they are all strings It is
only the
difference between a "simple" string which needs no further processing and a
"complex" string which needs further parsing.
The API defines types for the parameters. There are no separate
"simple" and "complex" strings in the type system. The string type is
different than the list type. What you call complex strings are in my
mind just the raw unprocessed serialized data, which is a different
abstraction level than the values that the various read_foo() functions
return. It feels just very wrong to have a function that returns values
in both domains: unparsed raw data and parsed values. Especially when
the function is a family of read_foo() functions where all other
functions only operate on their designated types.
Yes, that is some additional functionality that the read_string()
function provides on top of reading what I called "simple strings"
above. But that does not hinder the function to work exactly
like expected when reading a plain string. Maybe a better name
for the function would be read_string_element(), indicating that
it can read either a plain string or an element of the parameter
list as a string.
I could split this into two functions, read_string() which always
does unescaping and read_element() which would be a wrapper
around split_list() and would never do unescaping. In both functions,
the is_unpacked flag can be useful to check if the input matches
the type "plain string" or "serialized data".
A separate function for reading the raw data sounds good to me. Are you
attached to the read_element() name, or could it be read_raw() (or
perhaps read_raw_value() or read_raw_element())? Somehow read_raw()
seems better to me, but I can't make strong arguments for why it should
be called that.
--
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk
Georg Chini
2018-08-01 11:40:27 UTC
Permalink
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
+
+/* Read functions */
+
/* Split the specified string into elements. An element is defined as
* a sub-string between curly braces. The function is needed to parse
* the parameters of messages. Each time it is called returns the position
* of the current element in result and the state pointer is advanced to
- * the next list element.
+ * the next list element. On return, the parameter *is_unpacked indicates
+ * if the string is plain text or contains a sub-list. is_unpacked may
+ * be NULL.
is_unpacked looks like unnecessary complexity.
pa_message_params_read_string() should always unescape the value.
This is not about parameter type, it is just to distinguish between
Consider a parameter list that consists of strings and a couple of
arrays. Now you can read this list as an array of strings (patch 8
provides a function for that) and then examine those strings that
contain arrays separately. Having the flag (and using it in read_string())
provides a more flexible and convenient way to parse a parameter list.
The flag may not be strictly necessary at the moment, but I would still
like to keep it.
To continue a familiar theme of my review: if there's a
read_string_array() function, I want that to be used only for string
arrays, not a mishmash of random types. There could be a separate
function split_list_into_array() that does something similar to
what you wanted to do with read_string_array().
split_list_into_array() wouldn't do special string handling,
though, so unescaping would be left to the application. I find that
only logical, since other basic types don't get special handling
either (i.e. floats aren't converted to C floats).
Your use case could be served with a vararg function that instead of
pa_message_params_read(c, state,
PA_TYPE_STRING, &param1,
PA_TYPE_FLOAT, &param2,
PA_TYPE_RAW, &param3,
0);
PA_TYPE_RAW would be useful for reading a list (or anything else) into
a C string without unescaping or other processing. There could be also
PA_TYPE_IGNORE for uninteresting variables, and PA_TYPE_*_ARRAY for
arrays of basic types.
(Now it occurred to me that the 'c' argument name that is used in the
parsing functions is a bit weird and unhelpful. Maybe "param_list"
would be good?)
I still don't see your point. Again, the use of is_unpacked is
transparent for the
user of read_string(), so the user just reads a string without having to
care about
unescaping. The flag does not complicate the API, it simplifies it
because the
unescaping is done automatically.
The flag complicates the split_list() function parameters, but also the
read_string() semantics: you need to explain to the user that unlike
all the other read_foo() functions, read_string() can read any value
and unescaping becomes conditional because of this.
read_string() is not supposed to read any value, it is only supposed
to read strings. Actually, the user does not need to know anything
about escaping, because read_string() and write_string() do the
necessary escaping/unescaping completely transparent for the user.
The is_unpacked flag is at least useful to check if something returned
by split_list() is really a simple type and not a structure or array. I am
currently not using it in the read_foo() functions, but I think I should.
I guess you'd use is_unpacked for catching errors in the read_foo()
functions? That seems reasonable, but I'd like that to be done in an
internal function. You said that maybe split_list() could be made an
internal function, and that seems like a good idea. An alternative
would be to have an internal function for which split_list() would be a
simplified wrapper.
I guess I can make split_list() internal.
Post by Tanu Kaskinen
Post by Georg Chini
Post by Tanu Kaskinen
Post by Georg Chini
Your approach seems unnecessary
complicated to me. A string is a string, even if it contains
sub-structures. Your
split_list_into_array() function would basically return an array of
strings, so what
would be the advantage? It would only make parsing more cumbersome, because
the task of unescaping is given to the user instead of doing it
automatically where
necessary.
That's why I came up with the vararg function. I agree that
split_list_into_array() is unlikely to be very useful.
Post by Georg Chini
Also there is no "mishmash of random types", they are all strings It is
only the
difference between a "simple" string which needs no further processing and a
"complex" string which needs further parsing.
The API defines types for the parameters. There are no separate
"simple" and "complex" strings in the type system. The string type is
different than the list type. What you call complex strings are in my
mind just the raw unprocessed serialized data, which is a different
abstraction level than the values that the various read_foo() functions
return. It feels just very wrong to have a function that returns values
in both domains: unparsed raw data and parsed values. Especially when
the function is a family of read_foo() functions where all other
functions only operate on their designated types.
Yes, that is some additional functionality that the read_string()
function provides on top of reading what I called "simple strings"
above. But that does not hinder the function to work exactly
like expected when reading a plain string. Maybe a better name
for the function would be read_string_element(), indicating that
it can read either a plain string or an element of the parameter
list as a string.
I could split this into two functions, read_string() which always
does unescaping and read_element() which would be a wrapper
around split_list() and would never do unescaping. In both functions,
the is_unpacked flag can be useful to check if the input matches
the type "plain string" or "serialized data".
A separate function for reading the raw data sounds good to me. Are you
attached to the read_element() name, or could it be read_raw() (or
perhaps read_raw_value() or read_raw_element())? Somehow read_raw()
seems better to me, but I can't make strong arguments for why it should
be called that.
read_raw() seems OK as a counterpart to write_raw().
Loading...