[pulseaudio-discuss] [PATCH 5/6] protocol-native: Add signal receiving capability

[Tanu Kaskinen]

On Sat, 2017-08-19 at 17:48 +0200, Georg Chini wrote:
> This patch extends the client subscription API, so that signals sent from
> PulseAudio can be processed. Within PulseAudio, a signal can be emitted
> using pa_signal_post(). The interface can be used to notify the client of
> events that are not covered by the subscription API (for example a button
> press event on a bluetooth headset).
> 
> Setting up signal notification is very similar to using subscriptions.
> First the client needs to subscribe with pa_context_subscribe_signals()
> and then sets up a signal handler using pa_context_set_signal_callback().
> 
> The signal handler will receive three arguments in addition to the usual
> context and userdata:
> sender - string that specifies the origin of the signal
> signal - string that specifies the type of the signal
> signal_parameters - optional string for additional information
> ---
>  PROTOCOL                        |  3 ++
>  src/map-file                    |  2 ++
>  src/pulse/def.h                 |  4 +++
>  src/pulse/internal.h            |  2 ++
>  src/pulse/subscribe.c           | 66 +++++++++++++++++++++++++++++++++++++----
>  src/pulse/subscribe.h           |  9 ++++++
>  src/pulsecore/core-messages.c   | 23 ++++++++++++++
>  src/pulsecore/core-messages.h   | 11 +++++++
>  src/pulsecore/core.h            |  1 +
>  src/pulsecore/native-common.h   |  1 +
>  src/pulsecore/pdispatch.c       |  1 +
>  src/pulsecore/protocol-native.c | 57 +++++++++++++++++++++++++++++++++++
>  12 files changed, 174 insertions(+), 6 deletions(-)
> 
> diff --git a/PROTOCOL b/PROTOCOL
> index ccd3cd2c..e9337e27 100644
> --- a/PROTOCOL
> +++ b/PROTOCOL
> @@ -427,6 +427,9 @@ Added new command for communication with objects.
>  PA_COMMAND_SEND_OBJECT_MESSAGE:
>  sends a message to an object that registered a named message handler
>  
> +Added signal subscription functions. Clients can now subscribe to signals
> +that PulseAudio sends.

The PROTOCOL file is supposed to document the protocol changes in
detail (or at least that's my impression), but this description is a
very high-level note. Also, talking about "functions" is off-topic
since the protocol doesn't define any functions.

> +
>  #### 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/src/map-file b/src/map-file
> index 4e78c1f9..83301eba 100644
> --- a/src/map-file
> +++ b/src/map-file
> @@ -111,8 +111,10 @@ pa_context_set_source_volume_by_index;
>  pa_context_set_source_volume_by_name;
>  pa_context_set_state_callback;
>  pa_context_set_subscribe_callback;
> +pa_context_set_signal_callback;

The entries should be in alphabetical order.

> --- a/src/pulse/def.h
> +++ b/src/pulse/def.h
> @@ -599,6 +599,9 @@ typedef enum pa_subscription_event_type {
>      PA_SUBSCRIPTION_EVENT_REMOVE = 0x0020U,
>      /**< An object was removed */
>  
> +    PA_SUBSCRIPTION_EVENT_SIGNAL = 0x0040U,
> +    /** A signal was issued */

This is only used internally, so it should be omitted from the
documentation with \cond fulldocs.

> --- a/src/pulse/subscribe.h
> +++ b/src/pulse/subscribe.h
> @@ -72,12 +72,21 @@ PA_C_DECL_BEGIN
>  /** Subscription event callback prototype */
>  typedef void (*pa_context_subscribe_cb_t)(pa_context *c, pa_subscription_event_type_t t, uint32_t idx, void *userdata);
>  
> +/** Signal event callback prototype */

This \since tag is missing from this and the other API additions.

> +typedef void (*pa_context_signal_cb_t)(pa_context *c, const char *sender, const char *signal, const char *signal_info, void *userdata);
> +
>  /** Enable event notification */
>  pa_operation* pa_context_subscribe(pa_context *c, pa_subscription_mask_t m, pa_context_success_cb_t cb, void *userdata);
>  
> +/** Enable signal notification */
> +pa_operation* pa_context_subscribe_signals(pa_context *c, uint64_t signal_mask, pa_context_success_cb_t cb, void *userdata);
> +
>  /** Set the context specific call back function that is called whenever the state of the daemon changes */
>  void pa_context_set_subscribe_callback(pa_context *c, pa_context_subscribe_cb_t cb, void *userdata);
>  
> +/** Set the context specific call back function that is called whenever pulseaudio sends a signal */
> +void pa_context_set_signal_callback(pa_context *c, pa_context_signal_cb_t cb, void *userdata);

> +/* Send a signal */
> +void pa_signal_post(pa_core *c, const char *sender, uint64_t facility, const char *signal, const char *signal_parameters) {
> +    pa_signal_descriptor *sd;
> +
> +    pa_assert(sender);
> +    pa_assert(facility);
> +    pa_assert(signal);
> +
> +    sd = pa_xnew(pa_signal_descriptor, 1);

You can allocate the struct on the stack, since it's not used after the
function returns.

> +    sd->sender = pa_xstrdup(sender);
> +    sd->facility = facility;
> +    sd->signal = pa_xstrdup(signal);
> +    sd->parameters = pa_xstrdup(signal_parameters);

There's no need to copy the strings.

> +
> +    if (pa_hook_fire(&c->hooks[PA_CORE_HOOK_SEND_SIGNAL], sd) != PA_HOOK_OK)
> +        pa_log_warn("Sending signal %s from %s failed", signal, sender);

Not a big deal, but I don't see the need to handle failures here. The
operation is expected to always succeed.

> +/* Structure to pass signal information */
> +typedef struct pa_signal_descriptor {
> +    char *sender;
> +    char *signal;
> +    char *parameters;

These can be const.

-- 
Tanu

https://www.patreon.com/tanuk