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

[Fam Zheng]

On Wed, 06/22 17:35, Colin Lord wrote:
> From: Marc Mari <address@hidden>
> 
> 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.
> 
> All the necessary module information is located in a new structure found in
> include/qemu/module_block.h
> 
> Signed-off-by: Marc Mari <address@hidden>
> Signed-off-by: Colin Lord <address@hidden>
> ---
>  Makefile              |  3 --
>  block.c               | 86 
> +++++++++++++++++++++++++++++++++++++++++++++------
>  include/qemu/module.h |  3 ++
>  util/module.c         | 37 ++++++----------------
>  4 files changed, 90 insertions(+), 39 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 8f8b6a2..461187c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -247,9 +247,6 @@ Makefile: $(version-obj-y) $(version-lobj-y)
>  libqemustub.a: $(stub-obj-y)
>  libqemuutil.a: $(util-obj-y)
>  
> -block-modules = $(foreach o,$(block-obj-m),"$(basename $(subst /,-,$o))",) 
> NULL
> -util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)'
> -
>  ######################################################################
>  
>  qemu-img.o: qemu-img-cmds.h
> diff --git a/block.c b/block.c
> index f54bc25..7a91434 100644
> --- a/block.c
> +++ b/block.c
> @@ -26,6 +26,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/qbool.h"
> @@ -242,11 +243,29 @@ BlockDriverState *bdrv_new(void)
>  BlockDriver *bdrv_find_format(const char *format_name)
>  {
>      BlockDriver *drv1;
> +    size_t i;
> +
>      QLIST_FOREACH(drv1, &bdrv_drivers, list) {
>          if (!strcmp(drv1->format_name, format_name)) {
>              return drv1;
>          }
>      }
> +
> +    for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) {
> +        if (!strcmp(block_driver_modules[i].format_name, format_name)) {
> +            block_module_load_one(block_driver_modules[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 (!strcmp(drv1->format_name, format_name)) {
> +                    return drv1;
> +                }
> +            }
> +        }
> +    }
> +
>      return NULL;
>  }
>  
> @@ -447,8 +466,15 @@ int get_tmp_filename(char *filename, int size)
>  static BlockDriver *find_hdev_driver(const char *filename)
>  {
>      int score_max = 0, score;
> +    size_t i;
>      BlockDriver *drv = NULL, *d;
>  
> +    for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) {
> +        if (block_driver_modules[i].has_probe_device) {
> +            block_module_load_one(block_driver_modules[i].library_name);
> +        }
> +    }
> +
>      QLIST_FOREACH(d, &bdrv_drivers, list) {
>          if (d->bdrv_probe_device) {
>              score = d->bdrv_probe_device(filename);
> @@ -470,6 +496,7 @@ BlockDriver *bdrv_find_protocol(const char *filename,
>      char protocol[128];
>      int len;
>      const char *p;
> +    size_t i;
>  
>      /* TODO Drivers without bdrv_file_open must be specified explicitly */
>  
> @@ -496,6 +523,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)) {
> @@ -503,6 +531,23 @@ BlockDriver *bdrv_find_protocol(const char *filename,
>          }
>      }
>  
> +    for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) {
> +        if (block_driver_modules[i].protocol_name &&
> +            !strcmp(block_driver_modules[i].protocol_name, protocol)) {
> +            block_module_load_one(block_driver_modules[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;
>  }
> @@ -525,8 +570,15 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int 
> buf_size,
>                              const char *filename)
>  {
>      int score_max = 0, score;
> +    size_t i;
>      BlockDriver *drv = NULL, *d;
>  
> +    for (i = 0; i < ARRAY_SIZE(block_driver_modules); ++i) {
> +        if (block_driver_modules[i].has_probe) {
> +            block_module_load_one(block_driver_modules[i].library_name);
> +        }
> +    }
> +
>      QLIST_FOREACH(d, &bdrv_drivers, list) {
>          if (d->bdrv_probe) {
>              score = d->bdrv_probe(buf, buf_size, filename);
> @@ -2738,26 +2790,42 @@ static int qsort_strcmp(const void *a, const void *b)
>      return strcmp(a, b);
>  }
>  
> +static const char **add_format(const char **formats, int *count,
> +                               const char *format_name)
> +{
> +    int i;
> +
> +    for (i = 0; i < *count; i++) {
> +        if (!strcmp(formats[i], format_name)) {
> +            return formats;
> +        }
> +    }
> +
> +    *count += 1;
> +    formats = g_renew(const char *, formats, *count);
> +    formats[*count] = format_name;
> +    return formats;
> +}
> +
>  void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
>                           void *opaque)
>  {
>      BlockDriver *drv;
>      int count = 0;
>      int i;
> +    size_t n;
>      const char **formats = NULL;
>  
>      QLIST_FOREACH(drv, &bdrv_drivers, list) {
>          if (drv->format_name) {
> -            bool found = false;
> -            int i = count;
> -            while (formats && i && !found) {
> -                found = !strcmp(formats[--i], drv->format_name);
> -            }
> +            formats = add_format(formats, &count, drv->format_name);
> +        }
> +    }
>  
> -            if (!found) {
> -                formats = g_renew(const char *, formats, count + 1);
> -                formats[count++] = drv->format_name;
> -            }
> +    for (n = 0; n < ARRAY_SIZE(block_driver_modules); ++n) {
> +        if (block_driver_modules[n].format_name) {
> +            formats = add_format(formats, &count,
> +                                 block_driver_modules[n].format_name);
>          }
>      }
>  
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 2370708..4729858 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -52,9 +52,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);

Superfluous ending semi-colon?

> +
>  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/util/module.c b/util/module.c
> index ce058ae..dbc6e52 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) {
> @@ -163,14 +160,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 +174,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 +182,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.5.5
> 
>