Re: [Qemu-devel] [PATCH v10 5/8] module: implement module loading

[Fam Zheng <famz@redhat.com>]

On Mon, 09/16 09:59, Daniel P. Berrange wrote:
> On Mon, Sep 16, 2013 at 02:50:24PM +0800, Fam Zheng wrote:
> > Added three types of modules:
> > 
> >     typedef enum {
> >         MODULE_LOAD_BLOCK = 0,
> >         MODULE_LOAD_UI,
> >         MODULE_LOAD_NET,
> >         MODULE_LOAD_MAX,
> >     } module_load_type;
> > 
> > and their loading function:
> > 
> >     void module_load(module_load_type).
> > 
> > which loads whitelisted ".so" files of the given type under ${MODDIR}.
> > 
> > Modules of each type should be loaded in respective subsystem
> > initialization code.
> > 
> > The init function of dynamic module is no longer with
> > __attribute__((constructor)) as static linked version, and need to be
> > explicitly called once loaded. The function name is mangled with per
> > configure fingerprint as:
> > 
> >     init_$(date +%s$$$RANDOM)
> > 
> > Which is known to module_load function, and the loading fails if this
> > symbol is not there. With this, modules built from a different
> > tree/version/configure will not be loaded.
> > 
> > The module loading code requires gmodule-2.0.
> > 
> > Configure option "--enable-modules=L" can be used to restrict qemu to
> > only build/load some whitelisted modules.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  Makefile              |  3 ++
> >  block.c               |  1 +
> >  configure             | 44 +++++++++++++++++++++-------
> >  include/qemu/module.h | 23 +++++++++++++++
> >  rules.mak             |  9 ++++--
> >  scripts/create_config | 22 ++++++++++++++
> >  util/module.c         | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  vl.c                  |  2 ++
> >  8 files changed, 172 insertions(+), 13 deletions(-)
> > 
> > +void module_load(module_load_type type)
> > +{
> > +#ifdef CONFIG_MODULES
> > +    const char *prefix;
> > +    char *fname = NULL;
> > +    const char **mp;
> > +    static const char *module_whitelist[] = {
> > +        CONFIG_MODULE_WHITELIST
> > +    };
> > +
> > +    if (!g_module_supported()) {
> > +        return;
> > +    }
> > +
> > +    switch (type) {
> > +    case MODULE_LOAD_BLOCK:
> > +        prefix = "block-";
> > +        break;
> > +    case MODULE_LOAD_UI:
> > +        prefix = "ui-";
> > +        break;
> > +    case MODULE_LOAD_NET:
> > +        prefix = "ui-";
> 
> Wrong prefix.
> 
> > +        break;
> > +    default:
> > +        return;
> > +    }
> > +
> > +    for (mp = &module_whitelist[0]; *mp; mp++) {
> > +        if (strncmp(prefix, *mp, strlen(prefix))) {
> > +            continue;
> > +        }
> > +        fname = g_strdup_printf("%s/%s%s", CONFIG_MODDIR, *mp, HOST_DSOSUF);
> > +        module_load_file(fname);
> > +        g_free(fname);
> > +    }
> > +
> > +#endif
> > +}
> 
> IMHO this method design is really crazy. If you want to have a
> situation where you call module_load() multiple times, then
> have separate whitelists for block/net/ui, instead of having
> one global whitelist which you then have to load a subset of
> each time.  Alternatively just call module_load() once and
> give rid of these enums for loading block/net/ui separately.
> 

It's pretty clear that we have different subsets of modules to load for target
emulator and qemu-img, so not having enums is not working.

What's the disadvantage of this, and why are separate lists better?

Thanks,
Fam