On 12.01.2018 16:40, Tanu Kaskinen wrote: > On Sun, 2017-10-29 at 20:51 +0100, 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 function pa_split_message_response() for easy >> parsing of the message response string. >> --- >> man/pactl.1.xml.in | 2 +- >> shell-completion/bash/pulseaudio | 2 +- >> shell-completion/zsh/_pulseaudio | 1 + >> src/pulsecore/core-util.c | 34 +++++++++++++++++++++++++++++++++ >> src/pulsecore/core-util.h | 1 + >> src/utils/pactl.c | 41 ++++++++++++++++++++++++++++++++++++++-- >> 6 files changed, 77 insertions(+), 4 deletions(-) >> >> diff --git a/man/pactl.1.xml.in b/man/pactl.1.xml.in >> index 9669aca9..e444f973 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/pulsecore/core-util.c b/src/pulsecore/core-util.c >> index d4cfa20c..e7587f38 100644 >> --- a/src/pulsecore/core-util.c >> +++ b/src/pulsecore/core-util.c >> @@ -1140,6 +1140,40 @@ const char *pa_split_spaces_in_place(const char *c, int *n, const char **state) >> return current; >> } >> >> +/* Split the specified string into elements. An element is defined as >> + * a sub-string between curly braces. The function is needed to parse >> + * the response of messages. Each time it is called returns a newly >> + * allocated string with pa_xmalloc(). The variable state points to, >> + * should be initialized to NULL before the first call. */ >> +char *pa_split_message_response(const char *c, const char **state) { > There are three cases where we need to parse this data format: message > parameters, message responses and signal parameters. Therefore the > parsing function(s) should be named in some generic manner. OK, I'll think of a name. > > Clients are going to need the parsing function(s), so this should go to > libpulse. Isn't pactl a "normal"Â client? Just asking because the function can be used by pactl. > > I would also prefer more fine-grained functions than just one single > split function. When parsing lists, this split function will > unnecessarily malloc the list before mallocing the individual list > elements. The top level is an implicit list (see my other mail), so a new string will only be allocated if a list is passed as an element of another list. I agree with you that there should be functions for more complex structures, but in this special case it does not make sense to me because parsing is simple. I also think that the low level function does exactly what it should do. It allows you to recursively resolve the string and the additional few bytes of memory should not be an issue. Consider a more complex case. A message returning some top level variables, a simple list and a list of structures which themselves contain simple lists like that: {top_var1} {top_var2} {{l1}{l2} ...} {{{struct1_var1}{struct1_var2} {{struct1_l1}{struct1_l2} ...} {struct1_var3}}{{struct2_var1}{struct2_var2} {{struct2_l1}{struct2_l2} ...} {struct2_var3}} ...} {top_var3} The first pass would return the following strings: top_var1 top_var2 {l1}{l2} ... {{struct1_var1}{struct1_var2}{{struct1_l1}{struct1_l2} ...} {struct1_var3}} {{struct2_var1}{struct2_var2}{{struct2_l1}{struct2_l2} ...} {struct2_var3}} ... top_var3 So top level is resolved. The simple list can be resolved in the 2nd pass. The field of structures will be split in single structures during the 2nd pass like that: {struct1_var1}{struct1_var2}{{struct1_l1}{struct1_l2} ...} {struct1_var3} {struct2_var1}{struct2_var2}{{struct2_l1}{struct2_l2} ...} {struct2_var3} ... Now we can have a function which parses the struct and uses the low level function again twice to do so. I think we should introduce those high level parsing functions whenever they are first needed. For me, functions like string_to_array_of_{int|float|...}() or string_to_struct_xyz() would make sense. > > I already provided an example about how I'd like the parsing to work. > Apparently you didn't like that for some reason? I'll copy the example > here again: > > const char *state = message_parameters; > > if (pa_message_read_start_list(&state) < 0) > fail("Expected a list."); > > while (pa_message_read_end_list(&state) < 0) { > char *path; > char *description; > > if (pa_message_read_path(&state, &path) < 0) > fail("Expected a path."); > if (pa_message_read_string(&state, &description) < 0) > fail("Expected a string.") > > /* Do something with path and description. */ > > pa_xfree(description); > pa_xfree(path); > } As said above, I would prefer the single parsing function until the need for more complex functions arises. We should not be too fine grained, this will lead to confusion which function must be used when. >> + const char *current = *state ? *state : c; >> + const char *start_pos; >> + uint32_t open_braces = 1; >> + >> + if (!*current || *c == 0) >> + return NULL; >> + >> + current = strchr(current, '{'); >> + if (!current) >> + return NULL; >> + >> + start_pos = current + 1; >> + > I think it would be slightly easier to follow the logic if open_braces > was initialized here, just before the while block. > >> + while (open_braces != 0 && *current != 0) { >> + current++; >> + if (*current == '{') >> + open_braces++; >> + if (*current == '}') >> + open_braces--; >> + } > This is going to have to handle escaping. Another reason why I would like to add the escaping patch on top of the other patches. > >> + >> + pa_assert(open_braces == 0); > Invalid input shouldn't cause crashing. This will crash if the input > has too few closing brackets. Yes, that was the intention. But probably it is the wrong place for the assertion. I think if a message handler returns garbage this is a reason to crash. But you are right, if parameters are not correct, it shouldn't crash. I will just return NULL in that case or maybe let the function return an error code and have the string in the parameter list. What would you prefer? > >> + >> + *state = current + 1; >> + >> + return pa_xstrndup(start_pos, current - start_pos); >> +} >> + >> PA_STATIC_TLS_DECLARE(signame, pa_xfree); >> >> /* Return the name of an UNIX signal. Similar to Solaris sig2str() */ >> diff --git a/src/pulsecore/core-util.h b/src/pulsecore/core-util.h >> index e28b6aa7..947dfc13 100644 >> --- a/src/pulsecore/core-util.h >> +++ b/src/pulsecore/core-util.h >> @@ -113,6 +113,7 @@ char *pa_split(const char *c, const char *delimiters, const char **state); >> const char *pa_split_in_place(const char *c, const char *delimiters, int *n, const char **state); >> char *pa_split_spaces(const char *c, const char **state); >> const char *pa_split_spaces_in_place(const char *c, int *n, const char **state); >> +char *pa_split_message_response(const char *c, const char **state); >> >> char *pa_strip_nl(char *s); >> char *pa_strip(char *s); >> diff --git a/src/utils/pactl.c b/src/utils/pactl.c >> index 2e15b189..c7dc57ac 100644 >> --- a/src/utils/pactl.c >> +++ b/src/utils/pactl.c >> @@ -854,6 +854,34 @@ static void string_callback(pa_context *c, int success, const char *response, vo >> complete_action(); >> } >> >> +static void list_handlers_callback(pa_context *c, int success, const char *response, void *userdata) { >> + const char *state = NULL; >> + char *element; >> + char *handler_name; >> + char *description; >> + >> + if (success < 0) { > >> + >> + while ((element = pa_split_message_response(response, &state))) { >> + const char *state2 = NULL; >> + handler_name = pa_split_message_response(element, &state2); > Error handling is missing. Is it really necessary? The handler returns well defined data and there should be no case where the parsing fails. If you still think it is necessary, I will add it.
