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

["Daniel P. Berrange" <berrange@redhat.com>]

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í <markmb@redhat.com>
> ---
>  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 ?

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

> +
>      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       <markmb@redhat.com>
> + *
> + * 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 :-)

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

> @@ -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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|