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í <mar...@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 <mar...@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 :|
