[pulseaudio-discuss] [PATCH 3/6] pactl, pacmd, cli-command: Add send-message command

[Tanu Kaskinen]

On Sat, 2017-08-19 at 17:48 +0200, Georg Chini wrote:
> ---
>  man/pactl.1.xml.in               |  6 +++++
>  man/pulse-cli-syntax.5.xml.in    |  6 +++++
>  shell-completion/bash/pulseaudio |  5 +++--
>  shell-completion/zsh/_pulseaudio |  2 ++
>  src/pulsecore/cli-command.c      | 47 ++++++++++++++++++++++++++++++++++++++++
>  src/utils/pacmd.c                |  1 +
>  src/utils/pactl.c                | 39 ++++++++++++++++++++++++++++++++-
>  7 files changed, 103 insertions(+), 3 deletions(-)
> 
> diff --git a/man/pactl.1.xml.in b/man/pactl.1.xml.in
> index 39569b6b..e868babc 100644
> --- a/man/pactl.1.xml.in
> +++ b/man/pactl.1.xml.in
> @@ -246,6 +246,12 @@ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
>        </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.</p></optdesc>
> +    </option>

Are the message parameters expected to be just one string? I think it
would be more user-frienly to allow the parameters to be split in
multiple command line arguments.

There should be a pointer to the place where all the messages are
documented.

We haven't agreed where to put the documentation. I think it would be
highly desirable to have the documentation in the same repository with
the code, so the wiki is probably not the best place. A plain text file
would be the simplest way forward. The pactl man page could then point
to the text file in cgit.freedesktop.org.

I think it would be a good idea to use something like Sphinx for
generating nice html pages from plain text with some light markup, not
only for the messaging API documentation but for other documentation as
well. That requires some work to learn and configure the system,
however, and while I expect it to be pretty simple, I'm probably not
going to do that myself anytime soon. Would you be interested in
working on that? Even if not, what do you think about using something
like Sphinx for documentation? Would it be better than the wiki? Maybe
I will some day find the time and motivation to set it up myself.

> diff --git a/man/pulse-cli-syntax.5.xml.in b/man/pulse-cli-syntax.5.xml.in
> index 0a0fabaf..adf33f58 100644
> --- a/man/pulse-cli-syntax.5.xml.in
> +++ b/man/pulse-cli-syntax.5.xml.in
> @@ -298,6 +298,12 @@ License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
>      </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.</p></optdesc>
> +    </option>

Pointer to the API reference here as well.

> +static int pa_cli_command_send_message_to_object(pa_core *c, pa_tokenizer *t, pa_strbuf *buf, bool *fail) {
> +     const char *recipient_name, *message, *message_parameters;

The indentation is a bit off until midway through the function.

> +     char *response = NULL;
> +     int ret;
> +
> +     pa_core_assert_ref(c);
> +     pa_assert(t);
> +     pa_assert(buf);
> +     pa_assert(fail);
> +
> +
> +     if (!(recipient_name = pa_tokenizer_get(t, 1))) {
> +         pa_strbuf_puts(buf, "You need to specify a recipient for the message.\n");
> +            return -1;
> +     }
> +
> +     if (!(message = pa_tokenizer_get(t, 2))) {
> +         pa_strbuf_puts(buf, "You need to specify a message string.\n");
> +         return -1;
> +     }
> +
> +     /* parameters may be NULL */
> +     message_parameters = pa_tokenizer_get(t, 3);
> +
> +    ret = pa_core_send_message(c, recipient_name, message, message_parameters, &response);
> +
> +    if (ret == PA_CORE_SEND_NO_RECIPIENT) {
> +        pa_strbuf_puts(buf, "Invalid recipient name.\n");
> +        ret = -1;
> +
> +    } else if (ret == PA_CORE_SEND_FAILURE) {
> +        pa_strbuf_puts(buf, "Send message failed.\n");
> +        ret = -1;
> +
> +    } else {
> +        pa_strbuf_puts(buf, (const char *)response);

Why is the cast there?

> +static void string_callback(pa_context *c, const char *response, void *userdata) {
> +    if (!response) {
> +        pa_log(_("Failure: %s"), pa_strerror(pa_context_errno(c)));

The callback is not called on failures, and there's nothing preventing
the message handler from returning a null string as the response, so
either you shouldn't treat a NULL response as a failure, or you should
add documentation and some safeguards that prevent NULL responses from
successful operations.

> @@ -2015,6 +2036,19 @@ 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 a recipient and a message"));
> +                goto quit;
> +            }
> +
> +            recipient = pa_xstrdup(argv[optind + 1]);
> +            message = pa_xstrdup(argv[optind + 2]);
> +            if (argc == optind+4)
> +                message_args = pa_xstrdup(argv[optind + 3]);

message_args doesn't get set if argc > optiond + 4.

-- 
Tanu

https://www.patreon.com/tanuk