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 <f...@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
