Re: [Qemu-devel] [PATCH 1/2] Add dynamic module loading for block drivers

[Marc Marí]

On Thu, 27 Aug 2015 10:19:35 +0100
"Daniel P. Berrange" <address@hidden> wrote:

> On Mon, Aug 17, 2015 at 10:09:34AM +0200, Marc Marí wrote:
> > Extend the current module interface to allow for block drivers to
> > be loaded dynamically on request.
> > 
> > The only block drivers that can be converted into modules are the
> > drivers that don't perform any init operation except for
> > registering themselves. This is why libiscsi has been disabled as a
> > module.
> 
> Seems like we would just need to split the iscsi opts registration out
> into a separate file that is permanently linked.
> 
> > All the necessary module information is located in a new structure
> > found in include/qemu/module_block.h
> > 
> > Signed-off-by: Marc Marí <address@hidden>
> > ---
> >  block.c                     | 73
> > +++++++++++++++++++++++++++++++++++-- configure
> > |  2 +- include/qemu/module.h       |  3 ++
> >  include/qemu/module_block.h | 89
> > +++++++++++++++++++++++++++++++++++++++++++++
> > util/module.c               | 38 ++++++------------- 5 files
> > changed, 174 insertions(+), 31 deletions(-) create mode 100644
> > include/qemu/module_block.h
> > 
> > diff --git a/block.c b/block.c
> > index d088ee0..f24a624 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -27,6 +27,7 @@
> >  #include "block/block_int.h"
> >  #include "block/blockjob.h"
> >  #include "qemu/error-report.h"
> > +#include "qemu/module_block.h"
> >  #include "qemu/module.h"
> >  #include "qapi/qmp/qerror.h"
> >  #include "qapi/qmp/qjson.h"
> > @@ -277,11 +278,30 @@ void bdrv_add_close_notifier(BlockDriverState
> > *bs, Notifier *notify) BlockDriver *bdrv_find_format(const char
> > *format_name) {
> >      BlockDriver *drv1;
> > +    int i;
> 
> Nit-pick  'size_t' is a better type for loop iterators, especially
> when combined with a sizeof() comparison. Some comment in later
> functions too.
> 
> > +
> >      QLIST_FOREACH(drv1, &bdrv_drivers, list) {
> >          if (!strcmp(drv1->format_name, format_name)) {
> >              return drv1;
> >          }
> >      }
> > +
> > +    for (i = 0; i < ARRAY_SIZE(block_driver_module); ++i) {
> > +        if (!strcmp(block_driver_module[i].format_name,
> > format_name)) {
> > +
> > block_module_load_one(block_driver_module[i].library_name);
> > +            /* Copying code is not nice, but this way the current
> > discovery is
> > +             * not modified. Calling recursively could fail if the
> > library
> > +             * has been deleted.
> > +             */
> 
> Can you explaiin what you mean here about "if the library has been
> deleted" ?
> 
> Are you referring to possibilty of dlclose()ing the previously loaded
> library, or about possibility of the module on disk having been
> deleted or something else ?

I was thinking of relaunching the search by calling recursively the
function. But this would loop infinitely if somebody, manually, deleted
the library file.

> > @@ -483,9 +503,15 @@ int get_tmp_filename(char *filename, int size)
> >   */
> >  static BlockDriver *find_hdev_driver(const char *filename)
> >  {
> > -    int score_max = 0, score;
> > +    int score_max = 0, score, i;
> >      BlockDriver *drv = NULL, *d;
> >  
> > +    for (i = 0; i < ARRAY_SIZE(block_driver_module); ++i) {
> > +        if (block_driver_module[i].has_probe_device) {
> > +
> > block_module_load_one(block_driver_module[i].library_name);
> > +        }
> > +    }
> 
> If we have multiuple disks of the same type given to QEMU, it
> seems like we might end up calling block_module_load_one()
> multiple times for the same module & end up loading the same
> .so multiple times as a result. Should module_load() keep a
> record of everything it has loaded and short-circuit itself
> to a no-op, so that callers of module_load() don't have to
> worry about avoiding multiple calls.
 
I avoided that because glib already has it. It just increments the
reference count. Which is not important unless we want to dlclose it.

https://developer.gnome.org/glib/stable/glib-Dynamic-Loading-of-Modules.html#g-module-open

> > +
> >      QLIST_FOREACH(d, &bdrv_drivers, list) {
> >          if (d->bdrv_probe_device) {
> >              score = d->bdrv_probe_device(filename);
> > @@ -507,6 +533,7 @@ BlockDriver *bdrv_find_protocol(const char
> > *filename, char protocol[128];
> >      int len;
> >      const char *p;
> > +    int i;
> >  
> >      /* TODO Drivers without bdrv_file_open must be specified
> > explicitly */ 
> > @@ -533,6 +560,7 @@ BlockDriver *bdrv_find_protocol(const char
> > *filename, len = sizeof(protocol) - 1;
> >      memcpy(protocol, filename, len);
> >      protocol[len] = '\0';
> > +
> >      QLIST_FOREACH(drv1, &bdrv_drivers, list) {
> >          if (drv1->protocol_name &&
> >              !strcmp(drv1->protocol_name, protocol)) {
> > @@ -540,6 +568,23 @@ BlockDriver *bdrv_find_protocol(const char
> > *filename, }
> >      }
> >  
> > +    for (i = 0; i < ARRAY_SIZE(block_driver_module); ++i) {
> > +        if (block_driver_module[i].protocol_name &&
> > +            !strcmp(block_driver_module[i].protocol_name,
> > protocol)) {
> > +
> > block_module_load_one(block_driver_module[i].library_name);
> > +            /* Copying code is not nice, but this way the current
> > discovery is
> > +             * not modified. Calling recursively could fail if the
> > library
> > +             * has been deleted.
> > +             */
> > +            QLIST_FOREACH(drv1, &bdrv_drivers, list) {
> > +                if (drv1->protocol_name &&
> > +                    !strcmp(drv1->protocol_name, protocol)) {
> > +                    return drv1;
> > +                }
> > +            }
> > +        }
> > +    }
> > +
> >      error_setg(errp, "Unknown protocol '%s'", protocol);
> >      return NULL;
> >  }
> > @@ -561,9 +606,15 @@ BlockDriver *bdrv_find_protocol(const char
> > *filename, BlockDriver *bdrv_probe_all(const uint8_t *buf, int
> > buf_size, const char *filename)
> >  {
> > -    int score_max = 0, score;
> > +    int score_max = 0, score, i;
> >      BlockDriver *drv = NULL, *d;
> >  
> > +    for (i = 0; i < ARRAY_SIZE(block_driver_module); ++i) {
> > +        if (block_driver_module[i].has_probe) {
> > +
> > block_module_load_one(block_driver_module[i].library_name);
> > +        }
> > +    }
> > +
> >      QLIST_FOREACH(d, &bdrv_drivers, list) {
> >          if (d->bdrv_probe) {
> >              score = d->bdrv_probe(buf, buf_size, filename);
> > @@ -2783,7 +2834,7 @@ void bdrv_iterate_format(void (*it)(void
> > *opaque, const char *name), {
> >      BlockDriver *drv;
> >      int count = 0;
> > -    int i;
> > +    int i, n;
> >      const char **formats = NULL;
> >  
> >      QLIST_FOREACH(drv, &bdrv_drivers, list) {
> > @@ -2801,6 +2852,22 @@ void bdrv_iterate_format(void (*it)(void
> > *opaque, const char *name), }
> >      }
> >  
> > +    for (n = 0; n < ARRAY_SIZE(block_driver_module); ++n) {
> > +        if (block_driver_module[n].format_name) {
> > +            bool found = false;
> > +            int i = count;
> > +            while (formats && i && !found) {
> > +                found = !strcmp(formats[--i],
> > +
> > block_driver_module[n].format_name);
> > +            }
> > +
> > +            if (!found) {
> > +                formats = g_renew(const char *, formats, count +
> > 1);
> > +                formats[count++] =
> > block_driver_module[n].format_name;
> > +            }
> > +        }
> > +    }
> > +
> >      qsort(formats, count, sizeof(formats[0]), qsort_strcmp);
> >  
> >      for (i = 0; i < count; i++) {
> > diff --git a/configure b/configure
> > index cd219d8..9fca9ee 100755
> > --- a/configure
> > +++ b/configure
> > @@ -4971,7 +4971,7 @@ if test "$bzip2" = "yes" ; then
> >  fi
> >  
> >  if test "$libiscsi" = "yes" ; then
> > -  echo "CONFIG_LIBISCSI=m" >> $config_host_mak
> > +  echo "CONFIG_LIBISCSI=y" >> $config_host_mak
> >    echo "LIBISCSI_CFLAGS=$libiscsi_cflags" >> $config_host_mak
> >    echo "LIBISCSI_LIBS=$libiscsi_libs" >> $config_host_mak
> >  fi
> > diff --git a/include/qemu/module.h b/include/qemu/module.h
> > index 72d9498..0ad4bb7 100644
> > --- a/include/qemu/module.h
> > +++ b/include/qemu/module.h
> > @@ -53,9 +53,12 @@ typedef enum {
> >  #define qapi_init(function) module_init(function, MODULE_INIT_QAPI)
> >  #define type_init(function) module_init(function, MODULE_INIT_QOM)
> >  
> > +#define block_module_load_one(lib) module_load_one("block-", lib);
> > +
> >  void register_module_init(void (*fn)(void), module_init_type type);
> >  void register_dso_module_init(void (*fn)(void), module_init_type
> > type); 
> >  void module_call_init(module_init_type type);
> > +void module_load_one(const char *prefix, const char *lib_name);
> >  
> >  #endif
> > diff --git a/include/qemu/module_block.h
> > b/include/qemu/module_block.h new file mode 100644
> > index 0000000..f1d389c
> > --- /dev/null
> > +++ b/include/qemu/module_block.h
> > @@ -0,0 +1,88 @@
> > +/*
> > + * QEMU Block Module Infrastructure
> > + *
> > + * Copyright Red Hat, Inc. 2015
> > + *
> > + * Authors:
> > + *  Marc Mari       <address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version
> > 2.  See
> > + * the COPYING file in the top-level directory.
> > + *
> > + */
> 
> [snip]
> 
> > +
> > +#endif
> > \ No newline at end of file
> 
> Missing newline at end of file :-)

Oops :). Anyway, this is deleted in the other patch :)

> > diff --git a/util/module.c b/util/module.c
> > index 4bd4a94..992d317 100644
> > --- a/util/module.c
> > +++ b/util/module.c
> > @@ -91,14 +91,11 @@ void register_dso_module_init(void (*fn)(void),
> > module_init_type type) QTAILQ_INSERT_TAIL(&dso_init_list, e, node);
> >  }
> >  
> > -static void module_load(module_init_type type);
> > -
> >  void module_call_init(module_init_type type)
> >  {
> >      ModuleTypeList *l;
> >      ModuleEntry *e;
> >  
> > -    module_load(type);
> >      l = find_type(type);
> >  
> >      QTAILQ_FOREACH(e, l, node) {
> 
> Isn't the entire of module_call_init() a no-op now that you removed
> the module_load() call. IIUC, the find_type() method should return
> an empty list at that point, so the rest of the method doesn't
> nothing. IOW, I would think module_call_init() and its supporting
> functions can be deleted now. 

The module interface is no just for dynamic library loading but for
loading all the devices into the system. Devices that are inside the
monolithic QEMU still use this interface. So, it cannot be removed.

Thanks
Marc

> > @@ -149,6 +146,7 @@ static int module_load_file(const char *fname)
> >          ret = -EINVAL;
> >      } else {
> >          QTAILQ_FOREACH(e, &dso_init_list, node) {
> > +            e->init();
> >              register_module_init(e->init, e->type);
> >          }
> >          ret = 0;
> > @@ -163,14 +161,10 @@ out:
> >  }
> >  #endif
> >  
> > -static void module_load(module_init_type type)
> > +void module_load_one(const char *prefix, const char *lib_name)
> >  {
> >  #ifdef CONFIG_MODULES
> >      char *fname = NULL;
> > -    const char **mp;
> > -    static const char *block_modules[] = {
> > -        CONFIG_BLOCK_MODULES
> > -    };
> >      char *exec_dir;
> >      char *dirs[3];
> >      int i = 0;
> > @@ -181,15 +175,6 @@ static void module_load(module_init_type type)
> >          return;
> >      }
> >  
> > -    switch (type) {
> > -    case MODULE_INIT_BLOCK:
> > -        mp = block_modules;
> > -        break;
> > -    default:
> > -        /* no other types have dynamic modules for now*/
> > -        return;
> > -    }
> > -
> >      exec_dir = qemu_get_exec_dir();
> >      dirs[i++] = g_strdup_printf("%s", CONFIG_QEMU_MODDIR);
> >      dirs[i++] = g_strdup_printf("%s/..", exec_dir ? : "");
> > @@ -198,16 +183,15 @@ static void module_load(module_init_type type)
> >      g_free(exec_dir);
> >      exec_dir = NULL;
> >  
> > -    for ( ; *mp; mp++) {
> > -        for (i = 0; i < ARRAY_SIZE(dirs); i++) {
> > -            fname = g_strdup_printf("%s/%s%s", dirs[i], *mp,
> > HOST_DSOSUF);
> > -            ret = module_load_file(fname);
> > -            g_free(fname);
> > -            fname = NULL;
> > -            /* Try loading until loaded a module file */
> > -            if (!ret) {
> > -                break;
> > -            }
> > +    for (i = 0; i < ARRAY_SIZE(dirs); i++) {
> > +        fname = g_strdup_printf("%s/%s%s%s",
> > +                dirs[i], prefix, lib_name, HOST_DSOSUF);
> > +        ret = module_load_file(fname);
> > +        g_free(fname);
> > +        fname = NULL;
> > +        /* Try loading until loaded a module file */
> > +        if (!ret) {
> > +            break;
> >          }
> >      }
> >  
> > -- 
> > 2.4.3
> > 
> > 
> 
> Regards,
> Daniel