[Qemu-devel] [PATCH 0/3] Dynamic module loading for block drivers

[Qemu-devel] [PATCH 0/3] Dynamic module loading for block drivers

[Colin Lord]

This is v2 of the series I sent out last week. These are the changes I
made based on the feedback I got:
- Fixed typo and Marc's email address in the python script
- Moved registration of iscsi_opts into vl.c

What I didn't do:
- Remove copy-pasted loops

There was a bit of discussion about how to remove the need for the copy-
paste loops that are in block.c. I attempted to solve it by using
g_module_sym to load the BlockDriver struct directly at the time the
module gets loaded and returning it so that the loops were not
necessary. I accomplished this by adding a field to the struct which,
for a given format/protocol configuration, had the name of the
corresponding BlockDriver struct. Having the name allowed me to load the
symbol right out of the loaded module. However, it turns out that, at
least as far as I can tell, g_module_sym can't load the BlockDriver
structs in this way because they are declared static.

I tested the attempt to remove the copy-pasted loops by using the
qemu-iotests on it with ssh (which is modularized). The errors I got
were along the lines of:

can't open device ssh://[address removed]: Unknown protocol 'ssh'
Failed to find driver in module

To test my theory (I haven't had much luck finding reliable
documentation about this) that it was because they were static, I
changed the definition of the bdrv_ssh BlockDriver to not be static.
Unfortunately I still got errors, but I believe the drivers got loaded.
The errors were not the same, rather these ones were complaining about
the host key not matching the one in known_hosts. I've had this issue
while trying to set up ssh with qemu in the past, so I'm not quite as
worried about it (although I'd love to hear a fix), and more importantly
there aren't any messages about the driver not being found.

That hopefully explains some of the issues, and why I'm submitting this
with the duplicated loops still intact. If there are other ideas for
dealing with this my ears are open, but this is what I have for now.

Colin Lord (1):
  blockdev: prepare iSCSI block driver for dynamic loading

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

 .gitignore                      |   1 +
 Makefile                        |  11 +++-
 block.c                         |  86 +++++++++++++++++++++++---
 block/iscsi.c                   |  36 -----------
 include/qemu/module.h           |   3 +
 scripts/modules/module_block.py | 134 ++++++++++++++++++++++++++++++++++++++++
 util/module.c                   |  37 +++--------
 vl.c                            |  36 +++++++++++
 8 files changed, 269 insertions(+), 75 deletions(-)
 create mode 100644 scripts/modules/module_block.py

-- 
2.5.5

[Qemu-devel] [PATCH 1/3] blockdev: prepare iSCSI block driver for dynamic loading

[Colin Lord]

This commit moves the initialization of the QemuOptsList qemu_iscsi_opts
struct out of block/iscsi.c in order to allow it to be dynamically
loaded. Drivers that perform init operations other than registering
themselves can't be modularized, so this moves the initialization of
this struct into the main binary.

Signed-off-by: Colin Lord <clord@redhat.com>
---
 block/iscsi.c | 36 ------------------------------------
 vl.c          | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 7e78ade..6193499 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1879,45 +1879,9 @@ static BlockDriver bdrv_iscsi = {
     .bdrv_attach_aio_context = iscsi_attach_aio_context,
 };
 
-static QemuOptsList qemu_iscsi_opts = {
-    .name = "iscsi",
-    .head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
-    .desc = {
-        {
-            .name = "user",
-            .type = QEMU_OPT_STRING,
-            .help = "username for CHAP authentication to target",
-        },{
-            .name = "password",
-            .type = QEMU_OPT_STRING,
-            .help = "password for CHAP authentication to target",
-        },{
-            .name = "password-secret",
-            .type = QEMU_OPT_STRING,
-            .help = "ID of the secret providing password for CHAP "
-                    "authentication to target",
-        },{
-            .name = "header-digest",
-            .type = QEMU_OPT_STRING,
-            .help = "HeaderDigest setting. "
-                    "{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
-        },{
-            .name = "initiator-name",
-            .type = QEMU_OPT_STRING,
-            .help = "Initiator iqn name to use when connecting",
-        },{
-            .name = "timeout",
-            .type = QEMU_OPT_NUMBER,
-            .help = "Request timeout in seconds (default 0 = no timeout)",
-        },
-        { /* end of list */ }
-    },
-};
-
 static void iscsi_block_init(void)
 {
     bdrv_register(&bdrv_iscsi);
-    qemu_add_opts(&qemu_iscsi_opts);
 }
 
 block_init(iscsi_block_init);
diff --git a/vl.c b/vl.c
index 45eff56..4f04daa 100644
--- a/vl.c
+++ b/vl.c
@@ -526,6 +526,41 @@ static QemuOptsList qemu_fw_cfg_opts = {
     },
 };
 
+static QemuOptsList qemu_iscsi_opts = {
+    .name = "iscsi",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
+    .desc = {
+        {
+            .name = "user",
+            .type = QEMU_OPT_STRING,
+            .help = "username for CHAP authentication to target",
+        },{
+            .name = "password",
+            .type = QEMU_OPT_STRING,
+            .help = "password for CHAP authentication to target",
+        },{
+            .name = "password-secret",
+            .type = QEMU_OPT_STRING,
+            .help = "ID of the secret providing password for CHAP "
+                    "authentication to target",
+        },{
+            .name = "header-digest",
+            .type = QEMU_OPT_STRING,
+            .help = "HeaderDigest setting. "
+                    "{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
+        },{
+            .name = "initiator-name",
+            .type = QEMU_OPT_STRING,
+            .help = "Initiator iqn name to use when connecting",
+        },{
+            .name = "timeout",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Request timeout in seconds (default 0 = no timeout)",
+        },
+        { /* end of list */ }
+    },
+};
+
 /**
  * Get machine options
  *
@@ -3006,6 +3041,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_icount_opts);
     qemu_add_opts(&qemu_semihosting_config_opts);
     qemu_add_opts(&qemu_fw_cfg_opts);
+    qemu_add_opts(&qemu_iscsi_opts);
     module_call_init(MODULE_INIT_OPTS);
 
     runstate_init();
-- 
2.5.5

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

[Colin Lord]

From: Marc Mari <markmb@redhat.com>

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 Mari <markmb@redhat.com>
Signed-off-by: Colin Lord <clord@redhat.com>
---
 .gitignore                      |   1 +
 Makefile                        |   8 +++
 scripts/modules/module_block.py | 134 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 143 insertions(+)
 create mode 100644 scripts/modules/module_block.py

diff --git a/.gitignore b/.gitignore
index 38ee1c5..06aa064 100644
--- a/.gitignore
+++ b/.gitignore
@@ -110,3 +110,4 @@ tags
 TAGS
 docker-src.*
 *~
+/include/qemu/module_block.h
diff --git a/Makefile b/Makefile
index ed4032a..8f8b6a2 100644
--- a/Makefile
+++ b/Makefile
@@ -76,6 +76,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: ;
@@ -352,6 +354,12 @@ ivshmem-client$(EXESUF): $(ivshmem-client-obj-y) libqemuutil.a libqemustub.a
 ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) libqemuutil.a libqemustub.a
 	$(call LINK, $^)
 
+include/qemu/module_block.h: $(SRC_PATH)/scripts/modules/module_block.py config-host.mak
+	$(call quiet-command,$(PYTHON) \
+$(SRC_PATH)/scripts/modules/module_block.py \
+	$(SRC_PATH)/"./include/qemu/" $(addprefix $(SRC_PATH)/,$(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/scripts/modules/module_block.py b/scripts/modules/module_block.py
new file mode 100644
index 0000000..2b5d24c
--- /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(fheader, 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.5.5

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

[Colin Lord]

From: Marc Mari <markmb@redhat.com>

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 <markmb@redhat.com>
Signed-off-by: Colin Lord <clord@redhat.com>
---
 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);
+
 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

Re: [Qemu-devel] [PATCH 0/3] Dynamic module loading for block drivers

[Colin Lord]

On 06/22/2016 05:35 PM, Colin Lord wrote:
> This is v2 of the series I sent out last week. These are the changes I
> made based on the feedback I got:
> - Fixed typo and Marc's email address in the python script
> - Moved registration of iscsi_opts into vl.c
> 
> What I didn't do:
> - Remove copy-pasted loops
> 
> There was a bit of discussion about how to remove the need for the copy-
> paste loops that are in block.c. I attempted to solve it by using
> g_module_sym to load the BlockDriver struct directly at the time the
> module gets loaded and returning it so that the loops were not
> necessary. I accomplished this by adding a field to the struct which,
> for a given format/protocol configuration, had the name of the
> corresponding BlockDriver struct. Having the name allowed me to load the
> symbol right out of the loaded module. However, it turns out that, at
> least as far as I can tell, g_module_sym can't load the BlockDriver
> structs in this way because they are declared static.
> 
> I tested the attempt to remove the copy-pasted loops by using the
> qemu-iotests on it with ssh (which is modularized). The errors I got
> were along the lines of:
> 
> can't open device ssh://[address removed]: Unknown protocol 'ssh'
> Failed to find driver in module
> 
> To test my theory (I haven't had much luck finding reliable
> documentation about this) that it was because they were static, I
> changed the definition of the bdrv_ssh BlockDriver to not be static.
> Unfortunately I still got errors, but I believe the drivers got loaded.
> The errors were not the same, rather these ones were complaining about
> the host key not matching the one in known_hosts. I've had this issue
> while trying to set up ssh with qemu in the past, so I'm not quite as
> worried about it (although I'd love to hear a fix), and more importantly
> there aren't any messages about the driver not being found.
> 
> That hopefully explains some of the issues, and why I'm submitting this
> with the duplicated loops still intact. If there are other ideas for
> dealing with this my ears are open, but this is what I have for now.
> 
> Colin Lord (1):
>   blockdev: prepare iSCSI block driver for dynamic loading
> 
> Marc Mari (2):
>   blockdev: Add dynamic generation of module_block.h
>   blockdev: Add dynamic module loading for block drivers
> 
>  .gitignore                      |   1 +
>  Makefile                        |  11 +++-
>  block.c                         |  86 +++++++++++++++++++++++---
>  block/iscsi.c                   |  36 -----------
>  include/qemu/module.h           |   3 +
>  scripts/modules/module_block.py | 134 ++++++++++++++++++++++++++++++++++++++++
>  util/module.c                   |  37 +++--------
>  vl.c                            |  36 +++++++++++
>  8 files changed, 269 insertions(+), 75 deletions(-)
>  create mode 100644 scripts/modules/module_block.py
> 
Hmm, apologies for not labeling this series as v2 in the subject line.
Still trying to get this process down properly. Also let me know if I
need to rebase with master again. Of course I'd forget these things and
remember only right after mailing the patches in.

Re: [Qemu-devel] [PATCH 1/3] blockdev: prepare iSCSI block driver for dynamic loading

[Fam Zheng]

On Wed, 06/22 17:35, Colin Lord wrote:
> This commit moves the initialization of the QemuOptsList qemu_iscsi_opts
> struct out of block/iscsi.c in order to allow it to be dynamically
> loaded. Drivers that perform init operations other than registering
> themselves can't be modularized, so this moves the initialization of
> this struct into the main binary.
> 
> Signed-off-by: Colin Lord <clord@redhat.com>
> ---
>  block/iscsi.c | 36 ------------------------------------
>  vl.c          | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 7e78ade..6193499 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -1879,45 +1879,9 @@ static BlockDriver bdrv_iscsi = {
>      .bdrv_attach_aio_context = iscsi_attach_aio_context,
>  };
>  
> -static QemuOptsList qemu_iscsi_opts = {
> -    .name = "iscsi",
> -    .head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
> -    .desc = {
> -        {
> -            .name = "user",
> -            .type = QEMU_OPT_STRING,
> -            .help = "username for CHAP authentication to target",
> -        },{
> -            .name = "password",
> -            .type = QEMU_OPT_STRING,
> -            .help = "password for CHAP authentication to target",
> -        },{
> -            .name = "password-secret",
> -            .type = QEMU_OPT_STRING,
> -            .help = "ID of the secret providing password for CHAP "
> -                    "authentication to target",
> -        },{
> -            .name = "header-digest",
> -            .type = QEMU_OPT_STRING,
> -            .help = "HeaderDigest setting. "
> -                    "{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
> -        },{
> -            .name = "initiator-name",
> -            .type = QEMU_OPT_STRING,
> -            .help = "Initiator iqn name to use when connecting",
> -        },{
> -            .name = "timeout",
> -            .type = QEMU_OPT_NUMBER,
> -            .help = "Request timeout in seconds (default 0 = no timeout)",
> -        },
> -        { /* end of list */ }
> -    },
> -};
> -
>  static void iscsi_block_init(void)
>  {
>      bdrv_register(&bdrv_iscsi);
> -    qemu_add_opts(&qemu_iscsi_opts);
>  }
>  
>  block_init(iscsi_block_init);
> diff --git a/vl.c b/vl.c
> index 45eff56..4f04daa 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -526,6 +526,41 @@ static QemuOptsList qemu_fw_cfg_opts = {
>      },
>  };
>  
> +static QemuOptsList qemu_iscsi_opts = {
> +    .name = "iscsi",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
> +    .desc = {
> +        {
> +            .name = "user",
> +            .type = QEMU_OPT_STRING,
> +            .help = "username for CHAP authentication to target",
> +        },{
> +            .name = "password",
> +            .type = QEMU_OPT_STRING,
> +            .help = "password for CHAP authentication to target",
> +        },{
> +            .name = "password-secret",
> +            .type = QEMU_OPT_STRING,
> +            .help = "ID of the secret providing password for CHAP "
> +                    "authentication to target",
> +        },{
> +            .name = "header-digest",
> +            .type = QEMU_OPT_STRING,
> +            .help = "HeaderDigest setting. "
> +                    "{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
> +        },{
> +            .name = "initiator-name",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Initiator iqn name to use when connecting",
> +        },{
> +            .name = "timeout",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "Request timeout in seconds (default 0 = no timeout)",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>  /**
>   * Get machine options
>   *
> @@ -3006,6 +3041,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_icount_opts);
>      qemu_add_opts(&qemu_semihosting_config_opts);
>      qemu_add_opts(&qemu_fw_cfg_opts);
> +    qemu_add_opts(&qemu_iscsi_opts);

Should the new code still be conditional on CONFIG_LIBISCSI?  Because
previously it was.

Fam

>      module_call_init(MODULE_INIT_OPTS);
>  
>      runstate_init();
> -- 
> 2.5.5
> 
> 

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

[Fam Zheng]

On Wed, 06/22 17:35, Colin Lord wrote:
> From: Marc Mari <markmb@redhat.com>
> 
> 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 Mari <markmb@redhat.com>
> Signed-off-by: Colin Lord <clord@redhat.com>
> ---
>  .gitignore                      |   1 +
>  Makefile                        |   8 +++
>  scripts/modules/module_block.py | 134 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 143 insertions(+)
>  create mode 100644 scripts/modules/module_block.py
> 
> diff --git a/.gitignore b/.gitignore
> index 38ee1c5..06aa064 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -110,3 +110,4 @@ tags
>  TAGS
>  docker-src.*
>  *~
> +/include/qemu/module_block.h
> diff --git a/Makefile b/Makefile
> index ed4032a..8f8b6a2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -76,6 +76,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: ;
> @@ -352,6 +354,12 @@ ivshmem-client$(EXESUF): $(ivshmem-client-obj-y) libqemuutil.a libqemustub.a
>  ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) libqemuutil.a libqemustub.a
>  	$(call LINK, $^)
>  
> +include/qemu/module_block.h: $(SRC_PATH)/scripts/modules/module_block.py config-host.mak
> +	$(call quiet-command,$(PYTHON) \
> +$(SRC_PATH)/scripts/modules/module_block.py \
> +	$(SRC_PATH)/"./include/qemu/" $(addprefix $(SRC_PATH)/,$(patsubst %.mo,%.c,$(block-obj-m))), \

Please don't generate the header under $(SRC_PATH), instead generate in the
build dir. And the rule body should use $@ instead of repeating the target
name.

> +	"  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/scripts/modules/module_block.py b/scripts/modules/module_block.py
> new file mode 100644
> index 0000000..2b5d24c
> --- /dev/null
> +++ b/scripts/modules/module_block.py
> @@ -0,0 +1,134 @@
> +#!/usr/bin/python
> +#
> +# Module information generator
> +#
> +# Copyright Red Hat, Inc. 2015

* 2015 - 2016

> +#
> +# 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(fheader, 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

* 2015 - 2016

> + *
> + * 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)

Please just treat sys.argv[1] as the full path of output file, no need to
hardcode "module_block.h".

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

Fam

Re: [Qemu-devel] [PATCH 0/3] Dynamic module loading for block drivers

[Fam Zheng]

On Wed, 06/22 17:35, Colin Lord wrote:
> This is v2 of the series I sent out last week. These are the changes I
> made based on the feedback I got:
> - Fixed typo and Marc's email address in the python script
> - Moved registration of iscsi_opts into vl.c
> 
> What I didn't do:
> - Remove copy-pasted loops
> 
> There was a bit of discussion about how to remove the need for the copy-
> paste loops that are in block.c. I attempted to solve it by using
> g_module_sym to load the BlockDriver struct directly at the time the
> module gets loaded and returning it so that the loops were not
> necessary. I accomplished this by adding a field to the struct which,
> for a given format/protocol configuration, had the name of the
> corresponding BlockDriver struct. Having the name allowed me to load the
> symbol right out of the loaded module. However, it turns out that, at
> least as far as I can tell, g_module_sym can't load the BlockDriver
> structs in this way because they are declared static.
> 
> I tested the attempt to remove the copy-pasted loops by using the
> qemu-iotests on it with ssh (which is modularized). The errors I got
> were along the lines of:
> 
> can't open device ssh://[address removed]: Unknown protocol 'ssh'
> Failed to find driver in module
> 
> To test my theory (I haven't had much luck finding reliable
> documentation about this) that it was because they were static, I
> changed the definition of the bdrv_ssh BlockDriver to not be static.
> Unfortunately I still got errors, but I believe the drivers got loaded.
> The errors were not the same, rather these ones were complaining about
> the host key not matching the one in known_hosts. I've had this issue
> while trying to set up ssh with qemu in the past, so I'm not quite as
> worried about it (although I'd love to hear a fix), and more importantly
> there aren't any messages about the driver not being found.

Maybe it's not a big deal to export the symbols.

For testing you can also test with iscsi.

Fam

> 
> That hopefully explains some of the issues, and why I'm submitting this
> with the duplicated loops still intact. If there are other ideas for
> dealing with this my ears are open, but this is what I have for now.
> 
> Colin Lord (1):
>   blockdev: prepare iSCSI block driver for dynamic loading
> 
> Marc Mari (2):
>   blockdev: Add dynamic generation of module_block.h
>   blockdev: Add dynamic module loading for block drivers
> 
>  .gitignore                      |   1 +
>  Makefile                        |  11 +++-
>  block.c                         |  86 +++++++++++++++++++++++---
>  block/iscsi.c                   |  36 -----------
>  include/qemu/module.h           |   3 +
>  scripts/modules/module_block.py | 134 ++++++++++++++++++++++++++++++++++++++++
>  util/module.c                   |  37 +++--------
>  vl.c                            |  36 +++++++++++
>  8 files changed, 269 insertions(+), 75 deletions(-)
>  create mode 100644 scripts/modules/module_block.py
> 
> -- 
> 2.5.5
> 
> 

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 <markmb@redhat.com>
> 
> 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 <markmb@redhat.com>
> Signed-off-by: Colin Lord <clord@redhat.com>
> ---
>  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
> 
> 

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 <markmb@redhat.com>
> 
> 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 <markmb@redhat.com>
> Signed-off-by: Colin Lord <clord@redhat.com>
> ---
>  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);
> +        }
> +    }
> +

One idea about probing: we can avoid loading unnecessary modules if we move
all .bdrv_probe functions from BlockDriver structs into a new block/probe.c,
and let them return a driver name - all probing are just testing first bytes in
the header, therefore quite independent from the rest of the drivers.

Then we maintain a list of probing functions there. in block/probe.c:

typedef const char *BdrvProbeFunc(const uint8_t *buf, int buf_size, const char *filename,
                                  int *score);

const char *qcow2_probe(const uint8_t *buf, int buf_size, const char *filename,
                        int *score)
{
    ...
}

static BdrvProbeFunc format_probes[] = {
    qcow2_probe,
    dmg_probe,
    vmdk_probe,
    ...
};

static BdrvProbeFunc device_probes[] = {
    hdev_probe_device,
};

Then bdrv_probe_all can call each probe function in the list and find the
highest score, and load the corresponding block module.

The downside is each driver code is now split into two, but on the other hand
it is more compact than, for example, separate qcow2-probe.c, dmg-probe.c,
vmdk-probe.c that are linked to the main executable.

Fam

>      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);
> +
>  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
> 
> 

Re: [Qemu-devel] [PATCH 1/3] blockdev: prepare iSCSI block driver for dynamic loading

[Colin Lord]

On 06/22/2016 09:22 PM, Fam Zheng wrote:
> On Wed, 06/22 17:35, Colin Lord wrote:
>> This commit moves the initialization of the QemuOptsList qemu_iscsi_opts
>> struct out of block/iscsi.c in order to allow it to be dynamically
>> loaded. Drivers that perform init operations other than registering
>> themselves can't be modularized, so this moves the initialization of
>> this struct into the main binary.
>>
>> Signed-off-by: Colin Lord <clord@redhat.com>
>> ---
>>  block/iscsi.c | 36 ------------------------------------
>>  vl.c          | 36 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 36 insertions(+), 36 deletions(-)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c
>> index 7e78ade..6193499 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -1879,45 +1879,9 @@ static BlockDriver bdrv_iscsi = {
>>      .bdrv_attach_aio_context = iscsi_attach_aio_context,
>>  };
>>  
>> -static QemuOptsList qemu_iscsi_opts = {
>> -    .name = "iscsi",
>> -    .head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
>> -    .desc = {
>> -        {
>> -            .name = "user",
>> -            .type = QEMU_OPT_STRING,
>> -            .help = "username for CHAP authentication to target",
>> -        },{
>> -            .name = "password",
>> -            .type = QEMU_OPT_STRING,
>> -            .help = "password for CHAP authentication to target",
>> -        },{
>> -            .name = "password-secret",
>> -            .type = QEMU_OPT_STRING,
>> -            .help = "ID of the secret providing password for CHAP "
>> -                    "authentication to target",
>> -        },{
>> -            .name = "header-digest",
>> -            .type = QEMU_OPT_STRING,
>> -            .help = "HeaderDigest setting. "
>> -                    "{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
>> -        },{
>> -            .name = "initiator-name",
>> -            .type = QEMU_OPT_STRING,
>> -            .help = "Initiator iqn name to use when connecting",
>> -        },{
>> -            .name = "timeout",
>> -            .type = QEMU_OPT_NUMBER,
>> -            .help = "Request timeout in seconds (default 0 = no timeout)",
>> -        },
>> -        { /* end of list */ }
>> -    },
>> -};
>> -
>>  static void iscsi_block_init(void)
>>  {
>>      bdrv_register(&bdrv_iscsi);
>> -    qemu_add_opts(&qemu_iscsi_opts);
>>  }
>>  
>>  block_init(iscsi_block_init);
>> diff --git a/vl.c b/vl.c
>> index 45eff56..4f04daa 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -526,6 +526,41 @@ static QemuOptsList qemu_fw_cfg_opts = {
>>      },
>>  };
>>  
>> +static QemuOptsList qemu_iscsi_opts = {
>> +    .name = "iscsi",
>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
>> +    .desc = {
>> +        {
>> +            .name = "user",
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "username for CHAP authentication to target",
>> +        },{
>> +            .name = "password",
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "password for CHAP authentication to target",
>> +        },{
>> +            .name = "password-secret",
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "ID of the secret providing password for CHAP "
>> +                    "authentication to target",
>> +        },{
>> +            .name = "header-digest",
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "HeaderDigest setting. "
>> +                    "{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
>> +        },{
>> +            .name = "initiator-name",
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "Initiator iqn name to use when connecting",
>> +        },{
>> +            .name = "timeout",
>> +            .type = QEMU_OPT_NUMBER,
>> +            .help = "Request timeout in seconds (default 0 = no timeout)",
>> +        },
>> +        { /* end of list */ }
>> +    },
>> +};
>> +
>>  /**
>>   * Get machine options
>>   *
>> @@ -3006,6 +3041,7 @@ int main(int argc, char **argv, char **envp)
>>      qemu_add_opts(&qemu_icount_opts);
>>      qemu_add_opts(&qemu_semihosting_config_opts);
>>      qemu_add_opts(&qemu_fw_cfg_opts);
>> +    qemu_add_opts(&qemu_iscsi_opts);
> 
> Should the new code still be conditional on CONFIG_LIBISCSI?  Because
> previously it was.

Yeah, I think that should still be the case. Thanks for catching that.
> 
> Fam
> 
>>      module_call_init(MODULE_INIT_OPTS);
>>  
>>      runstate_init();
>> -- 
>> 2.5.5
>>
>>

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

On Wed, Jun 22, 2016 at 05:35:53PM -0400, Colin Lord wrote:
> +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.
> + *
> + */

The copyright and license doesn't make sense.  qapi.py and tracetool.py
also do not emit a copyright header for generated code.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]

On Wed, Jun 22, 2016 at 05:35:54PM -0400, Colin Lord wrote:
> +    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;
>  }

No recursion:

static BlockDriver *bdrv_do_find_format(const char *format_name)
{
    BlockDriver *drv1;

    QLIST_FOREACH(drv1, &bdrv_drivers, list) {
        if (!strcmp(drv1->format_name, format_name)) {
            return drv1;
        }
    }

    return NULL;
}

BlockDriver *bdrv_find_format(const char *format_name)
{
    BlockDriver *drv;
    size_t i;

    drv = bdrv_do_find_format(format_name);
    if (drv) {
        return drv;
    }

    /* The driver isn't registered, maybe we need to load a module */
    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);
	    break;
        }
    }
    return bdrv_do_find_format(format_name);
}

>  
> @@ -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);
> +        }
> +    }

This patch series needs to solve probing so that we don't end up loading
all block drivers.  Fam's suggestion for a built-in probe.c sounds good
to me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Daniel P. Berrange]

On Fri, Jun 24, 2016 at 11:04:43AM +0100, Stefan Hajnoczi wrote:
> On Wed, Jun 22, 2016 at 05:35:54PM -0400, Colin Lord wrote:
> 
> >  
> > @@ -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);
> > +        }
> > +    }
> 
> This patch series needs to solve probing so that we don't end up loading
> all block drivers.  Fam's suggestion for a built-in probe.c sounds good
> to me.

Do we really care if probing loads all drivers ? Last time we discussed
this I thought we decided that because probing almost always leads to
security vulnerabilities, no one should use it by default and so we
don't really need to worry about optimizing it.

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

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

[Fam Zheng]

On Fri, 06/24 11:37, Daniel P. Berrange wrote:
> On Fri, Jun 24, 2016 at 11:04:43AM +0100, Stefan Hajnoczi wrote:
> > On Wed, Jun 22, 2016 at 05:35:54PM -0400, Colin Lord wrote:
> > 
> > >  
> > > @@ -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);
> > > +        }
> > > +    }
> > 
> > This patch series needs to solve probing so that we don't end up loading
> > all block drivers.  Fam's suggestion for a built-in probe.c sounds good
> > to me.
> 
> Do we really care if probing loads all drivers ? Last time we discussed
> this I thought we decided that because probing almost always leads to
> security vulnerabilities, no one should use it by default and so we
> don't really need to worry about optimizing it.

Does this mean we can drop "has_probe" and "has_probe_device" from the
generated header and load all modules for probe?  If so, I'd like to again
stress my preference for a "simplified" code "parser":

All of current block_init() calls except quorum can be converted with this:

    #define BLOCK_DRIVER_EXPORT(fmt_name, prot_name, drv) \
        static void bdrv_ ## fmt_name ## _ ## prot_name(void) \
        { \
            if (strlen(#fmt_name)) { \
                drv->format_name = #fmt_name; \
            } \
            if (strlen(#prot_name)) { \
                drv->protocol_name = #prot_name; \
            } \
            bdrv_register(&(drv)); \
        } \
        block_init(bdrv_ ## fmt_name ## prot_name)

curl.c:

    BLOCK_DRIVER_EXPORT(http, http, bdrv_http);
    BLOCK_DRIVER_EXPORT(https, https, bdrv_https);
    BLOCK_DRIVER_EXPORT(ftp, ftp, bdrv_ftp);
    ...

iscsi.c (on top of patch 1):

    BLOCK_DRIVER_EXPORT(iscsi, iscsi, bdrv_iscsi);

vmdk.c:

    BLOCK_DRIVER_EXPORT(vmdk,, bdrv_vmdk);

Then the python generator greps for BLOCK_DRIVER_EXPORT, instead of 'static
BlockDriver', which is much more reliable (in the sense of whitespace subtlety,
field name changing, etc).

Fam

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

On Fri, Jun 24, 2016 at 11:37:56AM +0100, Daniel P. Berrange wrote:
> On Fri, Jun 24, 2016 at 11:04:43AM +0100, Stefan Hajnoczi wrote:
> > On Wed, Jun 22, 2016 at 05:35:54PM -0400, Colin Lord wrote:
> > 
> > >  
> > > @@ -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);
> > > +        }
> > > +    }
> > 
> > This patch series needs to solve probing so that we don't end up loading
> > all block drivers.  Fam's suggestion for a built-in probe.c sounds good
> > to me.
> 
> Do we really care if probing loads all drivers ? Last time we discussed
> this I thought we decided that because probing almost always leads to
> security vulnerabilities, no one should use it by default and so we
> don't really need to worry about optimizing it.

If the code to handle probing is simple then doing it is nice.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]