[Qemu-devel] [PATCH v2 0/2] Dynamic module support for block drivers

[Qemu-devel] [PATCH v2 0/2] Dynamic module support for block drivers

[Marc Marí]

The current module infrastructure has been improved to enable dynamic module
loading.

This reduces the load time for very simple guests. For the following
configuration (very loaded)

./configure --enable-sdl --enable-gtk --enable-vte --enable-curses \
    --enable-vnc --enable-vnc-{jpeg,tls,sasl,png} --enable-virtfs \
    --enable-brlapi --enable-curl --enable-fdt --enable-bluez \
    --enable-kvm --enable-rdma --enable-uuid --enable-vde \
    --enable-linux-aio --enable-cap-ng --enable-attr --enable-vhost-net \
    --enable-vhost-scsi --enable-spice --enable-rbd --enable-libiscsi \
    --enable-smartcard-nss --enable-guest-agent --enable-libusb \
    --enable-usb-redir --enable-lzo --enable-snappy --enable-bzip2 \
    --enable-seccomp --enable-coroutine-pool --enable-glusterfs \
    --enable-tpm --enable-libssh2 --enable-vhdx --enable-numa \
    --enable-tcmalloc --target-list=x86_64-softmmu

With modules disabled, there are 142 libraries loaded at startup. Time is
the following:
 LD time: 0.065 seconds
 QEMU time: 0.02 seconds
 Total time: 0.085 seconds

With this patch series and modules enabled, there are 128 libraries loaded
at startup. Time is the following:
 LD time: 0.02 seconds
 QEMU time: 0.02 seconds
 Total time: 0.04 seconds

Where LD time is the time between the program startup and the jump to main,
and QEMU time is the time between the start of main and the first kvm_entry.

These results are just with a few block drivers, that were already a module.
Adding more modules (block or not block) should be easy, and will reduce
the load time even more.

Marc Marí (2):
  Add dynamic module loading for block drivers
  Add dynamic generation of module_block.h

 .gitignore                      |   1 +
 Makefile                        |  10 ++-
 block.c                         |  70 +++++++++++++++++++++
 configure                       |   2 +-
 include/qemu/module.h           |   3 +
 scripts/modules/module_block.py | 134 ++++++++++++++++++++++++++++++++++++++++
 util/module.c                   |  38 ++++--------
 7 files changed, 227 insertions(+), 31 deletions(-)
 create mode 100755 scripts/modules/module_block.py

-- 
2.4.3

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

[Marc Marí]

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.

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                     | 70 +++++++++++++++++++++++++++++++++++
 configure                   |  2 +-
 include/qemu/module.h       |  3 ++
 include/qemu/module_block.h | 90 +++++++++++++++++++++++++++++++++++++++++++++
 util/module.c               | 38 ++++++-------------
 5 files changed, 175 insertions(+), 28 deletions(-)
 create mode 100644 include/qemu/module_block.h

diff --git a/block.c b/block.c
index 090923c..814429a 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;
+    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;
 }
 
@@ -484,8 +504,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);
@@ -507,6 +534,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 */
 
@@ -533,6 +561,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 +569,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;
 }
@@ -562,8 +608,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);
@@ -2784,6 +2837,7 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
     BlockDriver *drv;
     int count = 0;
     int i;
+    size_t n;
     const char **formats = NULL;
 
     QLIST_FOREACH(drv, &bdrv_drivers, list) {
@@ -2801,6 +2855,22 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
         }
     }
 
+    for (n = 0; n < ARRAY_SIZE(block_driver_modules); ++n) {
+        if (block_driver_modules[n].format_name) {
+            bool found = false;
+            int i = count;
+            while (formats && i && !found) {
+                found = !strcmp(formats[--i],
+                                block_driver_modules[n].format_name);
+            }
+
+            if (!found) {
+                formats = g_renew(const char *, formats, count + 1);
+                formats[count++] = block_driver_modules[n].format_name;
+            }
+        }
+    }
+
     qsort(formats, count, sizeof(formats[0]), qsort_strcmp);
 
     for (i = 0; i < count; i++) {
diff --git a/configure b/configure
index d854936..72ac564 100755
--- a/configure
+++ b/configure
@@ -4984,7 +4984,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..d725db8
--- /dev/null
+++ b/include/qemu/module_block.h
@@ -0,0 +1,90 @@
+/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
+/*
+ * 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.
+ *
+ */
+
+#ifndef QEMU_MODULE_BLOCK_H
+#define QEMU_MODULE_BLOCK_H
+
+#include "qemu-common.h"
+
+static const struct {
+    const char *format_name;
+    const char *protocol_name;
+    const char *library_name;
+    bool has_probe;
+    bool has_probe_device;
+} block_driver_modules[] = {
+	{
+	.library_name = "curl",
+	.format_name = "http",
+	.protocol_name = "http",
+	},
+	{
+	.library_name = "curl",
+	.format_name = "https",
+	.protocol_name = "https",
+	},
+	{
+	.library_name = "curl",
+	.format_name = "ftp",
+	.protocol_name = "ftp",
+	},
+	{
+	.library_name = "curl",
+	.format_name = "ftps",
+	.protocol_name = "ftps",
+	},
+	{
+	.library_name = "curl",
+	.format_name = "tftp",
+	.protocol_name = "tftp",
+	},
+	{
+	.library_name = "rbd",
+	.format_name = "rbd",
+	.protocol_name = "rbd",
+	},
+	{
+	.library_name = "gluster",
+	.format_name = "gluster",
+	.protocol_name = "gluster",
+	},
+	{
+	.library_name = "gluster",
+	.format_name = "gluster",
+	.protocol_name = "gluster+tcp",
+	},
+	{
+	.library_name = "gluster",
+	.format_name = "gluster",
+	.protocol_name = "gluster+unix",
+	},
+	{
+	.library_name = "gluster",
+	.format_name = "gluster",
+	.protocol_name = "gluster+rdma",
+	},
+	{
+	.library_name = "ssh",
+	.format_name = "ssh",
+	.protocol_name = "ssh",
+	},
+	{
+	.library_name = "dmg",
+	.format_name = "dmg",
+	.has_probe = true,
+	},
+};
+
+#endif
+
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) {
@@ -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

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

[Denis V. Lunev]

On 09/08/2015 04:53 PM, 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.
>
> 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                     | 70 +++++++++++++++++++++++++++++++++++
>   configure                   |  2 +-
>   include/qemu/module.h       |  3 ++
>   include/qemu/module_block.h | 90 +++++++++++++++++++++++++++++++++++++++++++++
>   util/module.c               | 38 ++++++-------------
>   5 files changed, 175 insertions(+), 28 deletions(-)
>   create mode 100644 include/qemu/module_block.h
>
> diff --git a/block.c b/block.c
> index 090923c..814429a 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;
> +    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;
>   }
>   
> @@ -484,8 +504,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);
> @@ -507,6 +534,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 */
>   
> @@ -533,6 +561,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 +569,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;
>   }
> @@ -562,8 +608,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);
> @@ -2784,6 +2837,7 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
>       BlockDriver *drv;
>       int count = 0;
>       int i;
> +    size_t n;
>       const char **formats = NULL;
>   
>       QLIST_FOREACH(drv, &bdrv_drivers, list) {
> @@ -2801,6 +2855,22 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
>           }
>       }
>   
> +    for (n = 0; n < ARRAY_SIZE(block_driver_modules); ++n) {
> +        if (block_driver_modules[n].format_name) {
> +            bool found = false;
> +            int i = count;
> +            while (formats && i && !found) {
> +                found = !strcmp(formats[--i],
> +                                block_driver_modules[n].format_name);
> +            }
> +
> +            if (!found) {
> +                formats = g_renew(const char *, formats, count + 1);
> +                formats[count++] = block_driver_modules[n].format_name;
> +            }
> +        }
> +    }
> +
>       qsort(formats, count, sizeof(formats[0]), qsort_strcmp);
>   
>       for (i = 0; i < count; i++) {
> diff --git a/configure b/configure
> index d854936..72ac564 100755
> --- a/configure
> +++ b/configure
> @@ -4984,7 +4984,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..d725db8
> --- /dev/null
> +++ b/include/qemu/module_block.h
> @@ -0,0 +1,90 @@
> +/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +/*
> + * 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.
> + *
> + */
> +
> +#ifndef QEMU_MODULE_BLOCK_H
> +#define QEMU_MODULE_BLOCK_H
> +
> +#include "qemu-common.h"
> +
> +static const struct {
> +    const char *format_name;
> +    const char *protocol_name;
> +    const char *library_name;
> +    bool has_probe;
> +    bool has_probe_device;
> +} block_driver_modules[] = {
> +	{
> +	.library_name = "curl",
> +	.format_name = "http",
> +	.protocol_name = "http",
> +	},
> +	{
> +	.library_name = "curl",
> +	.format_name = "https",
> +	.protocol_name = "https",
> +	},
> +	{
> +	.library_name = "curl",
> +	.format_name = "ftp",
> +	.protocol_name = "ftp",
> +	},
> +	{
> +	.library_name = "curl",
> +	.format_name = "ftps",
> +	.protocol_name = "ftps",
> +	},
> +	{
> +	.library_name = "curl",
> +	.format_name = "tftp",
> +	.protocol_name = "tftp",
> +	},
> +	{
> +	.library_name = "rbd",
> +	.format_name = "rbd",
> +	.protocol_name = "rbd",
> +	},
> +	{
> +	.library_name = "gluster",
> +	.format_name = "gluster",
> +	.protocol_name = "gluster",
> +	},
> +	{
> +	.library_name = "gluster",
> +	.format_name = "gluster",
> +	.protocol_name = "gluster+tcp",
> +	},
> +	{
> +	.library_name = "gluster",
> +	.format_name = "gluster",
> +	.protocol_name = "gluster+unix",
> +	},
> +	{
> +	.library_name = "gluster",
> +	.format_name = "gluster",
> +	.protocol_name = "gluster+rdma",
> +	},
> +	{
> +	.library_name = "ssh",
> +	.format_name = "ssh",
> +	.protocol_name = "ssh",
> +	},
> +	{
> +	.library_name = "dmg",
> +	.format_name = "dmg",
> +	.has_probe = true,
> +	},
> +};
> +
> +#endif
> +
> 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) {
> @@ -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;
>           }
>       }
>   
I think that the API will be more convenient if block_module_load_one
will return just loaded driver. Second loop will not be needed in this
case.

Den

[Qemu-devel] [PATCH v2 2/2] Add dynamic generation of module_block.h

[Marc Marí]

To simplify the addition of new block modules, add a script that generates
include/qemu/module_block.h automatically from the modules' source code.

This script assumes that the QEMU coding style rules are followed.

Signed-off-by: Marc Marí <markmb@redhat.com>
---
 .gitignore                      |   1 +
 Makefile                        |  10 ++-
 include/qemu/module_block.h     |  90 ---------------------------
 scripts/modules/module_block.py | 134 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 142 insertions(+), 93 deletions(-)
 delete mode 100644 include/qemu/module_block.h
 create mode 100755 scripts/modules/module_block.py

diff --git a/.gitignore b/.gitignore
index cb4b8ec..8428dd0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -106,3 +106,4 @@ cscope.*
 tags
 TAGS
 *~
+/include/qemu/module_block.h
diff --git a/Makefile b/Makefile
index 9ce3972..6b23e14 100644
--- a/Makefile
+++ b/Makefile
@@ -73,6 +73,8 @@ GENERATED_HEADERS += trace/generated-ust-provider.h
 GENERATED_SOURCES += trace/generated-ust.c
 endif
 
+GENERATED_HEADERS += include/qemu/module_block.h
+
 # Don't try to regenerate Makefile or configure
 # We don't generate any of them
 Makefile: ;
@@ -220,9 +222,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
@@ -313,6 +312,11 @@ msi:
 	@echo "MSI build not configured or dependency resolution failed (reconfigure with --enable-guest-agent-msi option)"
 endif
 
+include/qemu/module_block.h: $(SRC_PATH)/scripts/modules/module_block.py
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/modules/module_block.py \
+		"./include/qemu/" $(patsubst %.mo,%.c,$(block-obj-m)), \
+		"  GEN   $@")
+
 clean:
 # avoid old build problems by removing potentially incorrect old files
 	rm -f config.mak op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
diff --git a/include/qemu/module_block.h b/include/qemu/module_block.h
deleted file mode 100644
index d725db8..0000000
--- a/include/qemu/module_block.h
+++ /dev/null
@@ -1,90 +0,0 @@
-/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
-/*
- * 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.
- *
- */
-
-#ifndef QEMU_MODULE_BLOCK_H
-#define QEMU_MODULE_BLOCK_H
-
-#include "qemu-common.h"
-
-static const struct {
-    const char *format_name;
-    const char *protocol_name;
-    const char *library_name;
-    bool has_probe;
-    bool has_probe_device;
-} block_driver_modules[] = {
-	{
-	.library_name = "curl",
-	.format_name = "http",
-	.protocol_name = "http",
-	},
-	{
-	.library_name = "curl",
-	.format_name = "https",
-	.protocol_name = "https",
-	},
-	{
-	.library_name = "curl",
-	.format_name = "ftp",
-	.protocol_name = "ftp",
-	},
-	{
-	.library_name = "curl",
-	.format_name = "ftps",
-	.protocol_name = "ftps",
-	},
-	{
-	.library_name = "curl",
-	.format_name = "tftp",
-	.protocol_name = "tftp",
-	},
-	{
-	.library_name = "rbd",
-	.format_name = "rbd",
-	.protocol_name = "rbd",
-	},
-	{
-	.library_name = "gluster",
-	.format_name = "gluster",
-	.protocol_name = "gluster",
-	},
-	{
-	.library_name = "gluster",
-	.format_name = "gluster",
-	.protocol_name = "gluster+tcp",
-	},
-	{
-	.library_name = "gluster",
-	.format_name = "gluster",
-	.protocol_name = "gluster+unix",
-	},
-	{
-	.library_name = "gluster",
-	.format_name = "gluster",
-	.protocol_name = "gluster+rdma",
-	},
-	{
-	.library_name = "ssh",
-	.format_name = "ssh",
-	.protocol_name = "ssh",
-	},
-	{
-	.library_name = "dmg",
-	.format_name = "dmg",
-	.has_probe = true,
-	},
-};
-
-#endif
-
diff --git a/scripts/modules/module_block.py b/scripts/modules/module_block.py
new file mode 100755
index 0000000..0846362
--- /dev/null
+++ b/scripts/modules/module_block.py
@@ -0,0 +1,134 @@
+#!/usr/bin/python
+#
+# Module information generator
+#
+# 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.
+
+from __future__ import print_function
+import sys
+import os
+
+def get_string_struct(line):
+    data = line.split()
+
+    # data[0] -> struct element name
+    # data[1] -> =
+    # data[2] -> value
+
+    return data[2].replace('"', '')[:-1]
+
+def add_module(fhader, library, format_name, protocol_name,
+                probe, probe_device):
+    lines = []
+    lines.append('.library_name = "' + library + '",')
+    if format_name != "":
+        lines.append('.format_name = "' + format_name + '",')
+    if protocol_name != "":
+        lines.append('.protocol_name = "' + protocol_name + '",')
+    if probe:
+        lines.append('.has_probe = true,')
+    if probe_device:
+        lines.append('.has_probe_device = true,')
+
+    text = '\n\t'.join(lines)
+    fheader.write('\n\t{\n\t' + text + '\n\t},')
+
+def process_file(fheader, filename):
+    # This parser assumes the coding style rules are being followed
+    with open(filename, "r") as cfile:
+        found_something = False
+        found_start = False
+        library, _ = os.path.splitext(os.path.basename(filename))
+        for line in cfile:
+            if found_start:
+                line = line.replace('\n', '')
+                if line.find(".format_name") != -1:
+                    format_name = get_string_struct(line)
+                elif line.find(".protocol_name") != -1:
+                    protocol_name = get_string_struct(line)
+                elif line.find(".bdrv_probe") != -1:
+                    probe = True
+                elif line.find(".bdrv_probe_device") != -1:
+                    probe_device = True
+                elif line == "};":
+                    add_module(fheader, library, format_name, protocol_name,
+                                probe, probe_device)
+                    found_start = False
+            elif line.find("static BlockDriver") != -1:
+                found_something = True
+                found_start = True
+                format_name = ""
+                protocol_name = ""
+                probe = False
+                probe_device = False
+
+        if not found_something:
+            print("No BlockDriver struct found in " + filename + ". \
+                    Is this really a module?", file=sys.stderr)
+            sys.exit(1)
+
+def print_top(fheader):
+    fheader.write('''/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
+/*
+ * 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.
+ *
+ */
+
+''')
+
+    fheader.write('''#ifndef QEMU_MODULE_BLOCK_H
+#define QEMU_MODULE_BLOCK_H
+
+#include "qemu-common.h"
+
+static const struct {
+    const char *format_name;
+    const char *protocol_name;
+    const char *library_name;
+    bool has_probe;
+    bool has_probe_device;
+} block_driver_modules[] = {''')
+
+def print_bottom(fheader):
+    fheader.write('''
+};
+
+#endif
+''')
+
+# First argument: output folder
+# All other arguments: modules source files (.c)
+output_dir = sys.argv[1]
+if not os.path.isdir(output_dir):
+    print("Folder " + output_dir + " does not exist", file=sys.stderr)
+    sys.exit(1)
+
+path = output_dir + 'module_block.h'
+
+with open(path, 'w') as fheader:
+    print_top(fheader)
+
+    for filename in sys.argv[2:]:
+        if os.path.isfile(filename):
+            process_file(fheader, filename)
+        else:
+            print("File " + filename + " does not exist.", file=sys.stderr)
+            sys.exit(1)
+
+    print_bottom(fheader)
+
+sys.exit(0)
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 2/2] Add dynamic generation of module_block.h

[Fam Zheng]

On Tue, 09/08 15:53, Marc Marí wrote:
> diff --git a/include/qemu/module_block.h b/include/qemu/module_block.h
> deleted file mode 100644
> index d725db8..0000000
> --- a/include/qemu/module_block.h
> +++ /dev/null
> @@ -1,90 +0,0 @@
> -/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> -/*
> - * 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.
> - *
> - */
> -

Could you reorder the patches so you don't need to add them remove the
generated header?

> diff --git a/scripts/modules/module_block.py b/scripts/modules/module_block.py
> new file mode 100755
> index 0000000..0846362
> --- /dev/null
> +++ b/scripts/modules/module_block.py
> @@ -0,0 +1,134 @@
> +#!/usr/bin/python
> +#
> +# Module information generator
> +#
> +# 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.
> +
> +from __future__ import print_function
> +import sys
> +import os
> +
> +def get_string_struct(line):
> +    data = line.split()
> +
> +    # data[0] -> struct element name
> +    # data[1] -> =
> +    # data[2] -> value
> +
> +    return data[2].replace('"', '')[:-1]
> +
> +def add_module(fhader, library, format_name, protocol_name,
> +                probe, probe_device):
> +    lines = []
> +    lines.append('.library_name = "' + library + '",')
> +    if format_name != "":
> +        lines.append('.format_name = "' + format_name + '",')
> +    if protocol_name != "":
> +        lines.append('.protocol_name = "' + protocol_name + '",')
> +    if probe:
> +        lines.append('.has_probe = true,')
> +    if probe_device:
> +        lines.append('.has_probe_device = true,')
> +
> +    text = '\n\t'.join(lines)
> +    fheader.write('\n\t{\n\t' + text + '\n\t},')
> +
> +def process_file(fheader, filename):
> +    # This parser assumes the coding style rules are being followed
> +    with open(filename, "r") as cfile:
> +        found_something = False
> +        found_start = False
> +        library, _ = os.path.splitext(os.path.basename(filename))
> +        for line in cfile:
> +            if found_start:
> +                line = line.replace('\n', '')
> +                if line.find(".format_name") != -1:
> +                    format_name = get_string_struct(line)
> +                elif line.find(".protocol_name") != -1:
> +                    protocol_name = get_string_struct(line)
> +                elif line.find(".bdrv_probe") != -1:
> +                    probe = True
> +                elif line.find(".bdrv_probe_device") != -1:
> +                    probe_device = True
> +                elif line == "};":
> +                    add_module(fheader, library, format_name, protocol_name,
> +                                probe, probe_device)
> +                    found_start = False
> +            elif line.find("static BlockDriver") != -1:
> +                found_something = True
> +                found_start = True
> +                format_name = ""
> +                protocol_name = ""
> +                probe = False
> +                probe_device = False
> +
> +        if not found_something:
> +            print("No BlockDriver struct found in " + filename + ". \
> +                    Is this really a module?", file=sys.stderr)
> +            sys.exit(1)
> +
> +def print_top(fheader):
> +    fheader.write('''/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +/*
> + * 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.
> + *
> + */
> +
> +''')
> +
> +    fheader.write('''#ifndef QEMU_MODULE_BLOCK_H
> +#define QEMU_MODULE_BLOCK_H
> +
> +#include "qemu-common.h"
> +
> +static const struct {
> +    const char *format_name;
> +    const char *protocol_name;
> +    const char *library_name;
> +    bool has_probe;
> +    bool has_probe_device;
> +} block_driver_modules[] = {''')
> +
> +def print_bottom(fheader):
> +    fheader.write('''
> +};
> +
> +#endif
> +''')
> +
> +# First argument: output folder
> +# All other arguments: modules source files (.c)
> +output_dir = sys.argv[1]
> +if not os.path.isdir(output_dir):
> +    print("Folder " + output_dir + " does not exist", file=sys.stderr)
> +    sys.exit(1)
> +
> +path = output_dir + 'module_block.h'
> +
> +with open(path, 'w') as fheader:
> +    print_top(fheader)
> +
> +    for filename in sys.argv[2:]:
> +        if os.path.isfile(filename):
> +            process_file(fheader, filename)
> +        else:
> +            print("File " + filename + " does not exist.", file=sys.stderr)
> +            sys.exit(1)
> +
> +    print_bottom(fheader)
> +
> +sys.exit(0)

If we decide to parse c sources, I really want the parser as simple as
possible, while keeping the syntax explicit:

in block_int.h:

    /* In order to allow dynamical loading, block modules should specify which
     * protocols and formats are supported with one or more of following
     * macros, which are parsed by code generator to create mappings from
     * format or protocol name to module name.
     */
    #define BLOCK_PROTOCOL_MODULE(protocol, has_probe)
    #define BLOCK_FORMAT_MODULE(format, has_probe)

in curl.c:
    BLOCK_PROTOCOL_MODULE(http, false)
    BLOCK_PROTOCOL_MODULE(https, false)
    BLOCK_PROTOCOL_MODULE(ftp, false)
    BLOCK_PROTOCOL_MODULE(ftps, false)
    BLOCK_PROTOCOL_MODULE(tftp, false)

in dmg.c:
    BLOCK_FORMAT_MODULE(dmg, true)

In generator.py, you can grep for "BLOCK_PROTOCOL_MODULE(*)" and
"BLOCK_FORMAT_MODULE(*)".

Fam

Re: [Qemu-devel] [PATCH v2 2/2] Add dynamic generation of module_block.h

[Marc Marí]

On Wed, 9 Sep 2015 10:27:13 +0800
Fam Zheng <famz@redhat.com> wrote:

> On Tue, 09/08 15:53, Marc Marí wrote:
> > diff --git a/include/qemu/module_block.h
> > b/include/qemu/module_block.h deleted file mode 100644
> > index d725db8..0000000
> > --- a/include/qemu/module_block.h
> > +++ /dev/null
> > @@ -1,90 +0,0 @@
> > -/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> > -/*
> > - * 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.
> > - *
> > - */
> > -
> 
> Could you reorder the patches so you don't need to add them remove the
> generated header?
> 
> > diff --git a/scripts/modules/module_block.py
> > b/scripts/modules/module_block.py new file mode 100755
> > index 0000000..0846362
> > --- /dev/null
> > +++ b/scripts/modules/module_block.py
> > @@ -0,0 +1,134 @@
> > +#!/usr/bin/python
> > +#
> > +# Module information generator
> > +#
> > +# 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.
> > +
> > +from __future__ import print_function
> > +import sys
> > +import os
> > +
> > +def get_string_struct(line):
> > +    data = line.split()
> > +
> > +    # data[0] -> struct element name
> > +    # data[1] -> =
> > +    # data[2] -> value
> > +
> > +    return data[2].replace('"', '')[:-1]
> > +
> > +def add_module(fhader, library, format_name, protocol_name,
> > +                probe, probe_device):
> > +    lines = []
> > +    lines.append('.library_name = "' + library + '",')
> > +    if format_name != "":
> > +        lines.append('.format_name = "' + format_name + '",')
> > +    if protocol_name != "":
> > +        lines.append('.protocol_name = "' + protocol_name + '",')
> > +    if probe:
> > +        lines.append('.has_probe = true,')
> > +    if probe_device:
> > +        lines.append('.has_probe_device = true,')
> > +
> > +    text = '\n\t'.join(lines)
> > +    fheader.write('\n\t{\n\t' + text + '\n\t},')
> > +
> > +def process_file(fheader, filename):
> > +    # This parser assumes the coding style rules are being followed
> > +    with open(filename, "r") as cfile:
> > +        found_something = False
> > +        found_start = False
> > +        library, _ = os.path.splitext(os.path.basename(filename))
> > +        for line in cfile:
> > +            if found_start:
> > +                line = line.replace('\n', '')
> > +                if line.find(".format_name") != -1:
> > +                    format_name = get_string_struct(line)
> > +                elif line.find(".protocol_name") != -1:
> > +                    protocol_name = get_string_struct(line)
> > +                elif line.find(".bdrv_probe") != -1:
> > +                    probe = True
> > +                elif line.find(".bdrv_probe_device") != -1:
> > +                    probe_device = True
> > +                elif line == "};":
> > +                    add_module(fheader, library, format_name,
> > protocol_name,
> > +                                probe, probe_device)
> > +                    found_start = False
> > +            elif line.find("static BlockDriver") != -1:
> > +                found_something = True
> > +                found_start = True
> > +                format_name = ""
> > +                protocol_name = ""
> > +                probe = False
> > +                probe_device = False
> > +
> > +        if not found_something:
> > +            print("No BlockDriver struct found in " + filename +
> > ". \
> > +                    Is this really a module?", file=sys.stderr)
> > +            sys.exit(1)
> > +
> > +def print_top(fheader):
> > +    fheader.write('''/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> > +/*
> > + * 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.
> > + *
> > + */
> > +
> > +''')
> > +
> > +    fheader.write('''#ifndef QEMU_MODULE_BLOCK_H
> > +#define QEMU_MODULE_BLOCK_H
> > +
> > +#include "qemu-common.h"
> > +
> > +static const struct {
> > +    const char *format_name;
> > +    const char *protocol_name;
> > +    const char *library_name;
> > +    bool has_probe;
> > +    bool has_probe_device;
> > +} block_driver_modules[] = {''')
> > +
> > +def print_bottom(fheader):
> > +    fheader.write('''
> > +};
> > +
> > +#endif
> > +''')
> > +
> > +# First argument: output folder
> > +# All other arguments: modules source files (.c)
> > +output_dir = sys.argv[1]
> > +if not os.path.isdir(output_dir):
> > +    print("Folder " + output_dir + " does not exist",
> > file=sys.stderr)
> > +    sys.exit(1)
> > +
> > +path = output_dir + 'module_block.h'
> > +
> > +with open(path, 'w') as fheader:
> > +    print_top(fheader)
> > +
> > +    for filename in sys.argv[2:]:
> > +        if os.path.isfile(filename):
> > +            process_file(fheader, filename)
> > +        else:
> > +            print("File " + filename + " does not exist.",
> > file=sys.stderr)
> > +            sys.exit(1)
> > +
> > +    print_bottom(fheader)
> > +
> > +sys.exit(0)
> 
> If we decide to parse c sources, I really want the parser as simple as
> possible, while keeping the syntax explicit:
> 
> in block_int.h:
> 
>     /* In order to allow dynamical loading, block modules should
> specify which
>      * protocols and formats are supported with one or more of
> following
>      * macros, which are parsed by code generator to create mappings
> from
>      * format or protocol name to module name.
>      */
>     #define BLOCK_PROTOCOL_MODULE(protocol, has_probe)
>     #define BLOCK_FORMAT_MODULE(format, has_probe)
> 
> in curl.c:
>     BLOCK_PROTOCOL_MODULE(http, false)
>     BLOCK_PROTOCOL_MODULE(https, false)
>     BLOCK_PROTOCOL_MODULE(ftp, false)
>     BLOCK_PROTOCOL_MODULE(ftps, false)
>     BLOCK_PROTOCOL_MODULE(tftp, false)
> 
> in dmg.c:
>     BLOCK_FORMAT_MODULE(dmg, true)
> 
> In generator.py, you can grep for "BLOCK_PROTOCOL_MODULE(*)" and
> "BLOCK_FORMAT_MODULE(*)".
> 

Yes, that makes it simpler.

The only drawback I see is that all drivers that want to be converted
into modules will have to be touched, and the information from the
struct duplicated. It is not a big drawback because this will make the
parser really simple.

Thanks
Marc

Re: [Qemu-devel] [PATCH v2 0/2] Dynamic module support for block drivers

[Marc Marí]

Ping!

Anyone has more comments for the next version?

Thanks
Marc

>On Tue,  8 Sep 2015 15:53:20 +0200
>Marc Marí <markmb@redhat.com> wrote:
>
> The current module infrastructure has been improved to enable dynamic
> module loading.
> 
> This reduces the load time for very simple guests. For the following
> configuration (very loaded)
> 
> ./configure --enable-sdl --enable-gtk --enable-vte --enable-curses \
>     --enable-vnc --enable-vnc-{jpeg,tls,sasl,png} --enable-virtfs \
>     --enable-brlapi --enable-curl --enable-fdt --enable-bluez \
>     --enable-kvm --enable-rdma --enable-uuid --enable-vde \
>     --enable-linux-aio --enable-cap-ng --enable-attr
> --enable-vhost-net \ --enable-vhost-scsi --enable-spice --enable-rbd
> --enable-libiscsi \ --enable-smartcard-nss --enable-guest-agent
> --enable-libusb \ --enable-usb-redir --enable-lzo --enable-snappy
> --enable-bzip2 \ --enable-seccomp --enable-coroutine-pool
> --enable-glusterfs \ --enable-tpm --enable-libssh2 --enable-vhdx
> --enable-numa \ --enable-tcmalloc --target-list=x86_64-softmmu
> 
> With modules disabled, there are 142 libraries loaded at startup.
> Time is the following:
>  LD time: 0.065 seconds
>  QEMU time: 0.02 seconds
>  Total time: 0.085 seconds
> 
> With this patch series and modules enabled, there are 128 libraries
> loaded at startup. Time is the following:
>  LD time: 0.02 seconds
>  QEMU time: 0.02 seconds
>  Total time: 0.04 seconds
> 
> Where LD time is the time between the program startup and the jump to
> main, and QEMU time is the time between the start of main and the
> first kvm_entry.
> 
> These results are just with a few block drivers, that were already a
> module. Adding more modules (block or not block) should be easy, and
> will reduce the load time even more.
> 
> Marc Marí (2):
>   Add dynamic module loading for block drivers
>   Add dynamic generation of module_block.h
> 
>  .gitignore                      |   1 +
>  Makefile                        |  10 ++-
>  block.c                         |  70 +++++++++++++++++++++
>  configure                       |   2 +-
>  include/qemu/module.h           |   3 +
>  scripts/modules/module_block.py | 134
> ++++++++++++++++++++++++++++++++++++++++
> util/module.c                   |  38 ++++-------- 7 files changed,
> 227 insertions(+), 31 deletions(-) create mode 100755
> scripts/modules/module_block.py
> 

Re: [Qemu-devel] [PATCH v2 0/2] Dynamic module support for block drivers

[Denis V. Lunev]

On 09/08/2015 04:53 PM, Marc Marí wrote:
> The current module infrastructure has been improved to enable dynamic module
> loading.
>
> This reduces the load time for very simple guests. For the following
> configuration (very loaded)
>
> ./configure --enable-sdl --enable-gtk --enable-vte --enable-curses \
>      --enable-vnc --enable-vnc-{jpeg,tls,sasl,png} --enable-virtfs \
>      --enable-brlapi --enable-curl --enable-fdt --enable-bluez \
>      --enable-kvm --enable-rdma --enable-uuid --enable-vde \
>      --enable-linux-aio --enable-cap-ng --enable-attr --enable-vhost-net \
>      --enable-vhost-scsi --enable-spice --enable-rbd --enable-libiscsi \
>      --enable-smartcard-nss --enable-guest-agent --enable-libusb \
>      --enable-usb-redir --enable-lzo --enable-snappy --enable-bzip2 \
>      --enable-seccomp --enable-coroutine-pool --enable-glusterfs \
>      --enable-tpm --enable-libssh2 --enable-vhdx --enable-numa \
>      --enable-tcmalloc --target-list=x86_64-softmmu
>
> With modules disabled, there are 142 libraries loaded at startup. Time is
> the following:
>   LD time: 0.065 seconds
>   QEMU time: 0.02 seconds
>   Total time: 0.085 seconds
>
> With this patch series and modules enabled, there are 128 libraries loaded
> at startup. Time is the following:
>   LD time: 0.02 seconds
>   QEMU time: 0.02 seconds
>   Total time: 0.04 seconds
>
> Where LD time is the time between the program startup and the jump to main,
> and QEMU time is the time between the start of main and the first kvm_entry.
>
> These results are just with a few block drivers, that were already a module.
> Adding more modules (block or not block) should be easy, and will reduce
> the load time even more.
>
> Marc Marí (2):
>    Add dynamic module loading for block drivers
>    Add dynamic generation of module_block.h
>
>   .gitignore                      |   1 +
>   Makefile                        |  10 ++-
>   block.c                         |  70 +++++++++++++++++++++
>   configure                       |   2 +-
>   include/qemu/module.h           |   3 +
>   scripts/modules/module_block.py | 134 ++++++++++++++++++++++++++++++++++++++++
>   util/module.c                   |  38 ++++--------
>   7 files changed, 227 insertions(+), 31 deletions(-)
>   create mode 100755 scripts/modules/module_block.py
>

 From my point of view the design looks a bit complex.
The approach should be quite similar to one used
in Linux kernel.

If the block driver is configured as a module, block_init
should register proper hooks and create generic lists.
C code parsing does not look like a good approach
to me.

If block_init is a bad name, we could use something like
module_init.

Re: [Qemu-devel] [PATCH v2 0/2] Dynamic module support for block drivers

[Marc Marí]

On Mon, 21 Sep 2015 16:43:27 +0300
"Denis V. Lunev" <den-lists@parallels.com> wrote:

> On 09/08/2015 04:53 PM, Marc Marí wrote:
> > The current module infrastructure has been improved to enable
> > dynamic module loading.
> >
> > This reduces the load time for very simple guests. For the following
> > configuration (very loaded)
> >
> > ./configure --enable-sdl --enable-gtk --enable-vte --enable-curses \
> >      --enable-vnc --enable-vnc-{jpeg,tls,sasl,png} --enable-virtfs \
> >      --enable-brlapi --enable-curl --enable-fdt --enable-bluez \
> >      --enable-kvm --enable-rdma --enable-uuid --enable-vde \
> >      --enable-linux-aio --enable-cap-ng --enable-attr
> > --enable-vhost-net \ --enable-vhost-scsi --enable-spice
> > --enable-rbd --enable-libiscsi \ --enable-smartcard-nss
> > --enable-guest-agent --enable-libusb \ --enable-usb-redir
> > --enable-lzo --enable-snappy --enable-bzip2 \ --enable-seccomp
> > --enable-coroutine-pool --enable-glusterfs \ --enable-tpm
> > --enable-libssh2 --enable-vhdx --enable-numa \ --enable-tcmalloc
> > --target-list=x86_64-softmmu
> >
> > With modules disabled, there are 142 libraries loaded at startup.
> > Time is the following:
> >   LD time: 0.065 seconds
> >   QEMU time: 0.02 seconds
> >   Total time: 0.085 seconds
> >
> > With this patch series and modules enabled, there are 128 libraries
> > loaded at startup. Time is the following:
> >   LD time: 0.02 seconds
> >   QEMU time: 0.02 seconds
> >   Total time: 0.04 seconds
> >
> > Where LD time is the time between the program startup and the jump
> > to main, and QEMU time is the time between the start of main and
> > the first kvm_entry.
> >
> > These results are just with a few block drivers, that were already
> > a module. Adding more modules (block or not block) should be easy,
> > and will reduce the load time even more.
> >
> > Marc Marí (2):
> >    Add dynamic module loading for block drivers
> >    Add dynamic generation of module_block.h
> >
> >   .gitignore                      |   1 +
> >   Makefile                        |  10 ++-
> >   block.c                         |  70 +++++++++++++++++++++
> >   configure                       |   2 +-
> >   include/qemu/module.h           |   3 +
> >   scripts/modules/module_block.py | 134
> > ++++++++++++++++++++++++++++++++++++++++
> > util/module.c                   |  38 ++++-------- 7 files changed,
> > 227 insertions(+), 31 deletions(-) create mode 100755
> > scripts/modules/module_block.py
> >
> 
>  From my point of view the design looks a bit complex.
> The approach should be quite similar to one used
> in Linux kernel.
> 
> If the block driver is configured as a module, block_init
> should register proper hooks and create generic lists.
> C code parsing does not look like a good approach
> to me.
> 
> If block_init is a bad name, we could use something like
> module_init.
> 

I think applying the kind of modules of the Linux kernel here would
mean reworking an important part of the drivers that want to be
converted into modules.

If that's the case, I think it's better to have a less good solution
that is still good enough, and doesn't mean reworking half of the QEMU
codebase. Correct me if I'm wrong.

But of course, this is not the only valid approach.

Thanks
Marc

Re: [Qemu-devel] [PATCH v2 0/2] Dynamic module support for block drivers

[Denis V. Lunev]

On 09/21/2015 05:44 PM, Marc Marí wrote:
> On Mon, 21 Sep 2015 16:43:27 +0300
> "Denis V. Lunev" <den-lists@parallels.com> wrote:
>
>> On 09/08/2015 04:53 PM, Marc Marí wrote:
>>> The current module infrastructure has been improved to enable
>>> dynamic module loading.
>>>
>>> This reduces the load time for very simple guests. For the following
>>> configuration (very loaded)
>>>
>>> ./configure --enable-sdl --enable-gtk --enable-vte --enable-curses \
>>>       --enable-vnc --enable-vnc-{jpeg,tls,sasl,png} --enable-virtfs \
>>>       --enable-brlapi --enable-curl --enable-fdt --enable-bluez \
>>>       --enable-kvm --enable-rdma --enable-uuid --enable-vde \
>>>       --enable-linux-aio --enable-cap-ng --enable-attr
>>> --enable-vhost-net \ --enable-vhost-scsi --enable-spice
>>> --enable-rbd --enable-libiscsi \ --enable-smartcard-nss
>>> --enable-guest-agent --enable-libusb \ --enable-usb-redir
>>> --enable-lzo --enable-snappy --enable-bzip2 \ --enable-seccomp
>>> --enable-coroutine-pool --enable-glusterfs \ --enable-tpm
>>> --enable-libssh2 --enable-vhdx --enable-numa \ --enable-tcmalloc
>>> --target-list=x86_64-softmmu
>>>
>>> With modules disabled, there are 142 libraries loaded at startup.
>>> Time is the following:
>>>    LD time: 0.065 seconds
>>>    QEMU time: 0.02 seconds
>>>    Total time: 0.085 seconds
>>>
>>> With this patch series and modules enabled, there are 128 libraries
>>> loaded at startup. Time is the following:
>>>    LD time: 0.02 seconds
>>>    QEMU time: 0.02 seconds
>>>    Total time: 0.04 seconds
>>>
>>> Where LD time is the time between the program startup and the jump
>>> to main, and QEMU time is the time between the start of main and
>>> the first kvm_entry.
>>>
>>> These results are just with a few block drivers, that were already
>>> a module. Adding more modules (block or not block) should be easy,
>>> and will reduce the load time even more.
>>>
>>> Marc Marí (2):
>>>     Add dynamic module loading for block drivers
>>>     Add dynamic generation of module_block.h
>>>
>>>    .gitignore                      |   1 +
>>>    Makefile                        |  10 ++-
>>>    block.c                         |  70 +++++++++++++++++++++
>>>    configure                       |   2 +-
>>>    include/qemu/module.h           |   3 +
>>>    scripts/modules/module_block.py | 134
>>> ++++++++++++++++++++++++++++++++++++++++
>>> util/module.c                   |  38 ++++-------- 7 files changed,
>>> 227 insertions(+), 31 deletions(-) create mode 100755
>>> scripts/modules/module_block.py
>>>
>>   From my point of view the design looks a bit complex.
>> The approach should be quite similar to one used
>> in Linux kernel.
>>
>> If the block driver is configured as a module, block_init
>> should register proper hooks and create generic lists.
>> C code parsing does not look like a good approach
>> to me.
>>
>> If block_init is a bad name, we could use something like
>> module_init.
>>
> I think applying the kind of modules of the Linux kernel here would
> mean reworking an important part of the drivers that want to be
> converted into modules.
>
> If that's the case, I think it's better to have a less good solution
> that is still good enough, and doesn't mean reworking half of the QEMU
> codebase. Correct me if I'm wrong.
>
> But of course, this is not the only valid approach.
>
> Thanks
> Marc
Frankly speaking this does not look to me like a very big deal
and a huge rework. Build system should create 2 object files
for a modularized driver:
- one with driver stub which has required properties aka has_probe, name etc
   and a bit meaning that the driver is not present
- one with driver code
and from my point of view changes will not be big with this
approach.

Anyway, this is my opinion about this stuff. That is all, I not
against your change, I would like to note that there is a better
way which will allow further improvements.

Den