On Mon, 2018-04-09 at 19:35 +0200, Georg Chini wrote: > 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; Let's use "pa_message_params" as the prefix to be consistent with the file names (and plural makes more sense anyway). > 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); 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. > > /** 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". > + * 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. > +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. > +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. > + > + 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. > + > +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. > + 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). > + 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, ¶m_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. > + > + } > + > + 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). > 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
