On Sat, 2017-08-19 at 17:48 +0200, Georg Chini wrote:
> This patch adds the PA_COMMAND_SEND_OBJECT_MESSAGE command to protocol-native
> so that clients can use the messaging feature introduced in the previous patch.
>
> Sending messages can in effect replace the extension system for modules. The
> approach is more flexible than the extension interface because a generic string
> format is used to exchange information. Furthermore the messaging system can be
> used for any object, not only for modules, and is easier to implement than
> extensions.
> ---
> PROTOCOL | 7 +++++
> configure.ac | 2 +-
> src/map-file | 1 +
> src/pulse/introspect.c | 59 +++++++++++++++++++++++++++++++++++++++++
> src/pulse/introspect.h | 7 +++++
> src/pulsecore/native-common.h | 2 ++
> src/pulsecore/pdispatch.c | 2 ++
> src/pulsecore/protocol-native.c | 46 ++++++++++++++++++++++++++++++++
> 8 files changed, 125 insertions(+), 1 deletion(-)
>
> diff --git a/PROTOCOL b/PROTOCOL
> index 546998b7..ccd3cd2c 100644
> --- a/PROTOCOL
> +++ b/PROTOCOL
> @@ -420,6 +420,13 @@ memfd support only to 10.0+ clients.
>
> Check commit 451d1d676237c81 for further details.
>
> +## v33, implemented by > 11.0
>= 12.0
> +
> +Added new command for communication with objects.
> +
> +PA_COMMAND_SEND_OBJECT_MESSAGE:
> +sends a message to an object that registered a named message handler
It would be good to document what parameters the command takes, and
what parameters the reply has.
> +
> #### If you just changed the protocol, read this
> ## module-tunnel depends on the sink/source/sink-input/source-input protocol
> ## internals, so if you changed these, you might have broken module-tunnel.
> diff --git a/configure.ac b/configure.ac
> index 77b5ff5d..e28755b0 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -40,7 +40,7 @@ AC_SUBST(PA_MINOR, pa_minor)
> AC_SUBST(PA_MAJORMINOR, pa_major.pa_minor)
>
> AC_SUBST(PA_API_VERSION, 12)
> -AC_SUBST(PA_PROTOCOL_VERSION, 32)
> +AC_SUBST(PA_PROTOCOL_VERSION, 33)
>
> # The stable ABI for client applications, for the version info x:y:z
> # always will hold y=z
> diff --git a/src/map-file b/src/map-file
> index 93a62b86..4e78c1f9 100644
> --- a/src/map-file
> +++ b/src/map-file
> @@ -118,6 +118,7 @@ pa_context_suspend_sink_by_name;
> pa_context_suspend_source_by_index;
> pa_context_suspend_source_by_name;
> pa_context_unload_module;
> +pa_context_send_message_to_object;
Keep the alphabetical order.
> pa_context_unref;
> pa_cvolume_avg;
> pa_cvolume_avg_mask;
> diff --git a/src/pulse/introspect.c b/src/pulse/introspect.c
> index 510d784a..f23ff932 100644
> --- a/src/pulse/introspect.c
> +++ b/src/pulse/introspect.c
> @@ -2184,3 +2184,62 @@ pa_operation* pa_context_suspend_source_by_index(pa_context *c, uint32_t idx, in
>
> return o;
> }
> +
> +/** Object response string **/
> +
> +static void context_string_callback(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
> + pa_operation *o = userdata;
> + const char *response;
> +
> + pa_assert(pd);
> + pa_assert(o);
> + pa_assert(PA_REFCNT_VALUE(o) >= 1);
> +
> + if (!o->context)
> + goto finish;
> +
> + if (command != PA_COMMAND_REPLY) {
> + if (pa_context_handle_error(o->context, command, t, false) < 0)
> + goto finish;
> +
> + response = NULL;
> + } else if (pa_tagstruct_gets(t, &response) ||
The convention is to check errors with "< 0".
> diff --git a/src/pulse/introspect.h b/src/pulse/introspect.h
> index 43389b73..e78f6665 100644
> --- a/src/pulse/introspect.h
> +++ b/src/pulse/introspect.h
> @@ -423,6 +423,9 @@ typedef struct pa_module_info {
> /** Callback prototype for pa_context_get_module_info() and friends */
> typedef void (*pa_module_info_cb_t) (pa_context *c, const pa_module_info*i, int eol, void *userdata);
>
> +/** Callback prototype for pa_context_send_message_to_object() */
> +typedef void (*pa_context_string_cb_t)(pa_context *c, const char *response, void *userdata);
> +
I would create a new section for the messaging stuff. I don't think it
belongs in the modules section.
> diff --git a/src/pulsecore/native-common.h b/src/pulsecore/native-common.h
> index 70338b9f..c24dd666 100644
> --- a/src/pulsecore/native-common.h
> +++ b/src/pulsecore/native-common.h
> @@ -187,6 +187,8 @@ enum {
> * BOTH DIRECTIONS */
> PA_COMMAND_REGISTER_MEMFD_SHMID,
>
> + PA_COMMAND_SEND_OBJECT_MESSAGE,
All other commands document in which version they were introduced.
> diff --git a/src/pulsecore/pdispatch.c b/src/pulsecore/pdispatch.c
> index ab632a5a..52970f58 100644
> --- a/src/pulsecore/pdispatch.c
> +++ b/src/pulsecore/pdispatch.c
> @@ -199,6 +199,8 @@ static const char *command_names[PA_COMMAND_MAX] = {
> /* Supported since protocol v31 (9.0) */
> /* BOTH DIRECTIONS */
> [PA_COMMAND_REGISTER_MEMFD_SHMID] = "REGISTER_MEMFD_SHMID",
> +
> + [PA_COMMAND_SEND_OBJECT_MESSAGE] = "SEND_OBJECT_MESSAGE",
All other commands document in which version they were introduced. I
can understand that repeating the same comments in two places can feel
weird, and I'm not sure either that it's a good idea, but let's be
consistent.
> +/* Send message to an object which registered a handler. Result must be returned as string. */
> +static void command_send_object_message(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
> + pa_native_connection *c = PA_NATIVE_CONNECTION(userdata);
> + const char *recipient_name = NULL;
> + const char *message = NULL;
> + const char *message_parameters = NULL;
> + char *response = NULL;
> + int ret;
> + pa_tagstruct *reply;
> +
> + pa_native_connection_assert_ref(c);
> + pa_assert(t);
> +
> + if (pa_tagstruct_gets(t, &recipient_name) < 0 ||
> + pa_tagstruct_gets(t, &message) < 0 ||
> + pa_tagstruct_gets(t, &message_parameters) < 0 ||
> + !pa_tagstruct_eof(t)) {
> + protocol_error(c);
> + return;
> + }
> +
> + CHECK_VALIDITY(c->pstream, c->authorized, tag, PA_ERR_ACCESS);
> + CHECK_VALIDITY(c->pstream, pa_utf8_valid(recipient_name), tag, PA_ERR_INVALID);
> + CHECK_VALIDITY(c->pstream, message != NULL, tag, PA_ERR_INVALID);
I think we should check message and message_parameters for utf8-
validity too.
I'd like to have a debug message that says what client sent what
message. Maybe with message parameters too, or maybe not.
Since clients don't have names, and client indexes are so hard to map
to applications, the message should print something else.
command_set_volume() uses the process name, which you can copy, but
some clients set the application name too, so if you want to be fancy,
you can include the application name in the message.
> +
> + ret = pa_core_send_message(c->protocol->core, recipient_name, message, message_parameters, &response);
> +
> + if (ret == PA_CORE_SEND_NO_RECIPIENT) {
> + pa_pstream_send_error(c->pstream, tag, PA_ERR_NOENTITY);
> + return;
> +
> + } else if (ret == PA_CORE_SEND_FAILURE) {
> + pa_pstream_send_error(c->pstream, tag, PA_ERR_INTERNAL);
> + return;
> + }
See the previous patch for my comments about error handling.
> +
> + reply = reply_new(tag);
> + pa_tagstruct_puts(reply, (const char *)response);
Why is the cast there?
--
Tanu
https://www.patreon.com/tanuk
