[pulseaudio-discuss] [PATCH] pactl: Added unloading modules by name
Tanu Kaskinen
tanuk at iki.fi
Fri May 18 21:28:36 PDT 2012
On Wed, 2012-05-16 at 22:39 +0200, poljar wrote:
> pactl should allow unloading modules by name.
> pactl and the native protocol were expanded to handle names while
> unloading modules.
Protocol changes should be documented in the PROTOCOL file, and the
protocol version number needs to be incremented. Here's one example of a
proper protocol change commit:
http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=793f46320e98aa10dca16bcc1b3a421a4f2b6b7e
>
> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=48289
> ---
> src/map-file | 3 ++-
> src/pulse/introspect.c | 46 +++++++++++++++++++++++++++++++++++++--
> src/pulse/introspect.h | 8 +++++--
> src/pulsecore/protocol-native.c | 29 ++++++++++++++++++++----
> src/utils/pactl.c | 13 ++++++++---
> 5 files changed, 87 insertions(+), 12 deletions(-)
>
> diff --git a/src/map-file b/src/map-file
> index 69cf25b..82fdd06 100644
> --- a/src/map-file
> +++ b/src/map-file
> @@ -113,7 +113,8 @@ pa_context_suspend_sink_by_index;
> pa_context_suspend_sink_by_name;
> pa_context_suspend_source_by_index;
> pa_context_suspend_source_by_name;
> -pa_context_unload_module;
> +pa_context_unload_module_by_index;
> +pa_context_unload_module_by_name;
> pa_context_unref;
> pa_cvolume_avg;
> pa_cvolume_avg_mask;
> diff --git a/src/pulse/introspect.c b/src/pulse/introspect.c
> index 38a9d1c..332733b 100644
> --- a/src/pulse/introspect.c
> +++ b/src/pulse/introspect.c
> @@ -1864,8 +1864,50 @@ pa_operation* pa_context_load_module(pa_context *c, const char*name, const char
> return o;
> }
>
> -pa_operation* pa_context_unload_module(pa_context *c, uint32_t idx, pa_context_success_cb_t cb, void *userdata) {
You can't remove functions from the client API, because it breaks binary
compatibility for existing applications. I suggest marking
pa_context_unload_module as deprecated (see
pa_context_get_autoload_info_by_name() for an example of how that
marking is done) and change the implementation to just call
pa_context_unload_module_by_index().
> - return command_kill(c, PA_COMMAND_UNLOAD_MODULE, idx, cb, userdata);
> +pa_operation* pa_context_unload_module_by_index(pa_context *c, uint32_t idx, pa_context_success_cb_t cb, void *userdata) {
> + pa_operation *o;
> + pa_tagstruct *t;
> + uint32_t tag;
> +
> + pa_assert(c);
> + pa_assert(PA_REFCNT_VALUE(c) >= 1);
> +
> + PA_CHECK_VALIDITY_RETURN_NULL(c, !pa_detect_fork(), PA_ERR_FORKED);
> + PA_CHECK_VALIDITY_RETURN_NULL(c, c->state == PA_CONTEXT_READY, PA_ERR_BADSTATE);
> + PA_CHECK_VALIDITY_RETURN_NULL(c, idx != PA_INVALID_INDEX, PA_ERR_INVALID);
> +
> + o = pa_operation_new(c, NULL, (pa_operation_cb_t) cb, userdata);
> +
> + t = pa_tagstruct_command(c, PA_COMMAND_UNLOAD_MODULE, &tag);
> + pa_tagstruct_putu32(t, idx);
> + pa_tagstruct_puts(t, NULL);
> + pa_pstream_send_tagstruct(c->pstream, t);
> + pa_pdispatch_register_reply(c->pdispatch, tag, DEFAULT_TIMEOUT, pa_context_simple_ack_callback, pa_operation_ref(o), (pa_free_cb_t) pa_operation_unref);
> +
> + return o;
> +}
You seem to change the PA_COMMAND_UNLOAD_MODULE parameters. That breaks
compatibility with older servers (think network usage). You should check
what protocol version is in use, and set the parameters according to
that. Or, even better (in my opinion), don't change the semantics of
PA_COMMAND_UNLOAD_MODULE at all, just rename it to
PA_COMMAND_UNLOAD_MODULE_BY_INDEX, and create a new command
PA_COMMAND_UNLOAD_MODULE_BY_NAME.
> +
> +pa_operation* pa_context_unload_module_by_name(pa_context *c, const char *name, pa_context_success_cb_t cb, void *userdata) {
> + pa_operation *o;
> + pa_tagstruct *t;
> + uint32_t tag;
> +
> + pa_assert(c);
> + pa_assert(PA_REFCNT_VALUE(c) >= 1);
> +
> + PA_CHECK_VALIDITY_RETURN_NULL(c, !pa_detect_fork(), PA_ERR_FORKED);
> + PA_CHECK_VALIDITY_RETURN_NULL(c, c->state == PA_CONTEXT_READY, PA_ERR_BADSTATE);
> + PA_CHECK_VALIDITY_RETURN_NULL(c, name && *name, PA_ERR_INVALID);
This function should also check the protocol version, and if it's too
old, then return PA_ERR_NOTSUPPORTED.
> +
> + o = pa_operation_new(c, NULL, (pa_operation_cb_t) cb, userdata);
> +
> + t = pa_tagstruct_command(c, PA_COMMAND_UNLOAD_MODULE, &tag);
> + pa_tagstruct_putu32(t, PA_INVALID_INDEX);
> + pa_tagstruct_puts(t, name);
> + pa_pstream_send_tagstruct(c->pstream, t);
> + pa_pdispatch_register_reply(c->pdispatch, tag, DEFAULT_TIMEOUT, pa_context_simple_ack_callback, pa_operation_ref(o), (pa_free_cb_t) pa_operation_unref);
> +
> + return o;
> }
>
> /*** Autoload stuff ***/
> diff --git a/src/pulse/introspect.h b/src/pulse/introspect.h
> index 0072f5d..f80fb86 100644
> --- a/src/pulse/introspect.h
> +++ b/src/pulse/introspect.h
> @@ -409,8 +409,12 @@ typedef void (*pa_context_index_cb_t)(pa_context *c, uint32_t idx, void *userdat
> /** Load a module. */
> pa_operation* pa_context_load_module(pa_context *c, const char*name, const char *argument, pa_context_index_cb_t cb, void *userdata);
>
> -/** Unload a module. */
> -pa_operation* pa_context_unload_module(pa_context *c, uint32_t idx, pa_context_success_cb_t cb, void *userdata);
> +/** Unload a module by its index. */
This comment (and the next one too) should have a "\since 3.0" tag.
> +pa_operation* pa_context_unload_module_by_index(pa_context *c, uint32_t idx, pa_context_success_cb_t cb, void *userdata);
> +
> +
Cosmetic: Only one empty line, please.
> +/** Unload a module by its name. */
> +pa_operation* pa_context_unload_module_by_name(pa_context *c, const char*name, pa_context_success_cb_t cb, void *userdata);
>
> /** @} */
>
> diff --git a/src/pulsecore/protocol-native.c b/src/pulsecore/protocol-native.c
> index 396e143..56722e4 100644
> --- a/src/pulsecore/protocol-native.c
> +++ b/src/pulsecore/protocol-native.c
> @@ -4434,23 +4434,44 @@ static void command_load_module(pa_pdispatch *pd, uint32_t command, uint32_t tag
> static void command_unload_module(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
> pa_native_connection *c = PA_NATIVE_CONNECTION(userdata);
> uint32_t idx;
> + const char *name;
> pa_module *m;
>
> pa_native_connection_assert_ref(c);
> pa_assert(t);
>
> if (pa_tagstruct_getu32(t, &idx) < 0 ||
> + pa_tagstruct_gets(t, &name) < 0 ||
> !pa_tagstruct_eof(t)) {
> protocol_error(c);
> return;
> }
>
> CHECK_VALIDITY(c->pstream, c->authorized, tag, PA_ERR_ACCESS);
> - m = pa_idxset_get_by_index(c->protocol->core->modules, idx);
> - CHECK_VALIDITY(c->pstream, m, tag, PA_ERR_NOENTITY);
>
> - pa_module_unload_request(m, FALSE);
> - pa_pstream_send_simple_ack(c->pstream, tag);
> + if (idx != PA_INVALID_INDEX) {
> + CHECK_VALIDITY(c->pstream, idx != PA_INVALID_INDEX, tag, PA_ERR_INVALID);
> +
> + m = pa_idxset_get_by_index(c->protocol->core->modules, idx);
> +
> + CHECK_VALIDITY(c->pstream, m, tag, PA_ERR_NOENTITY);
> +
> + pa_module_unload_request(m, FALSE);
> + pa_pstream_send_simple_ack(c->pstream, tag);
> + }
> + else {
Cosmetic: Put else on the same line as the closing bracket. You can
leave an empty line before the closing bracket, if you like (I'd like
that, but I don't consider it a strict rule).
> + CHECK_VALIDITY(c->pstream, name && *name && pa_utf8_valid(name) && !strchr(name, '/'), tag, PA_ERR_INVALID);
Is this check copied from somewhere? Especially the slash check seems
odd here. There's a whole bunch of characters that aren't allowed in
module names, so what's so special about slash that it needs to be
checked here?
I'd only check that name is not NULL. Otherwise the the name comparison
later will make sure that invalid module names won't match anything. In
some cases it may be more efficient to fail fast with invalid module
names, but that's optimizing for the uncommon case.
> +
> + /* Traverse the module list and unload all modules with the matching name */
> + for (m = pa_idxset_first(c->protocol->core->modules, &idx); m; m = pa_idxset_next(c->protocol->core->modules, &idx)) {
PA_IDXSET_FOREACH is more convenient (and easier to read).
> + if (strcmp(name, m->name) == 0) {
We have pa_streq() for checking for string equality - use that.
> + CHECK_VALIDITY(c->pstream, m, tag, PA_ERR_NOENTITY);
What's this for? You check that m is not NULL? m is guaranteed to be
non-NULL, since if it's NULL, the loop will terminate. And you have
already dereferenced the pointer in the string comparison.
The PA_ERR_NOENTITY error should be sent after you have checked all
modules and none of them matched the given name.
> +
> + pa_module_unload_request(m, FALSE);
> + pa_pstream_send_simple_ack(c->pstream, tag);
The ack should be sent only once, after all modules have been unloaded.
> + }
> + }
> + }
I'd like to put the code for searching and unloading modules to a new
function called pa_module_unload_request_by_name(). Maybe the biggest
reason why I wanted the unload-by-name logic to be at the server end in
the first place was that the same logic should be used by other
protocols too (D-Bus and HTTP). Having a single function that takes care
of the unload-by-name logic ensures that all protocols behave in the
same way, and there's less copy-pasted code.
> }
>
> static void command_move_stream(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
> diff --git a/src/utils/pactl.c b/src/utils/pactl.c
The pactl changes could be in a separate patch. Don't forget to update
the man page too :)
--
Tanu
More information about the pulseaudio-discuss
mailing list