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
