On Fri, 2018-01-12 at 21:47 +0100, Georg Chini wrote: > 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. Normal clients can't use libpulsecommon, because its interface isn't stable. pactl can use libpulsecommon, because pactl and libpulsecommon come from the same source package, so they are always in sync. > > 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. We should have specialized parsing functions also for the simple cases, like reading a single int or float. We need to specify exactly how e.g. floats are serialized, and we shouldn't burden every client with the responsibility of implementing our float serialization rules. Not only is it more work for them, the likelihood of not following our spec exactly is very high. Returning to the topic of mallocs: allocating and freeing memory from the heap are quite heavy operations, and your approach will do a lot of those operations. Even reading a simple integer will, I suppose, involve first allocating a new string. I can't say for sure that the performance penalty makes any practical difference, but my intuition says that we should use the approach that I proposed that requires far fewer heap memory allocations. > > 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. Can you elaborate, what risk of confusion do you see? At this point I'm not convinced by your reasoning, and I feel pretty strongly about the parsing design. If you want to continue with the "single split function" approach, getting Arun on your side is likely going to be needed for me to give up on this. > > > + 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. We're talking about client processes here. We should never crash in libpulse, no matter what crazy stuff the server does. > 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? I would prefer returning an error code. > > > > > > + > > > + *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. Again, in libpulse we can't trust the server to act correctly. -- Tanu https://liberapay.com/tanuk https://www.patreon.com/tanuk
