On Mon, 2012-05-21 at 15:37 +0200, poljarinho at gmail.com wrote:
> > > + 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.
> >
> Yes, this is copied from the module load command, I thought that this
> check is mandatory for handling modules by names.
Ok. The check in the module loading command is probably done because the
name will be used when accessing the file system. With module unloading
there's no need to worry about that.
> > > +
> > > + /* 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).
> This is also used in command_extension() so I copied it from there. I
> will make a cosmetic patch for that too.
>
> >
> > > + if (strcmp(name, m->name) == 0) {
> >
> Same thing as the for().
>
> I will work on this, and send a better patch.
Thanks!
--
Tanu
