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.
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
