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

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

[Colin Lord]

This is a repost of some previous patches written by Marc Marí which
were also reposted by Richard Jones a few months ago. The original
series and reposted series are here:

https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg01995.html
https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg01994.html

I've tried to take some of the previous feedback and integrate it into
this series. There are also a few things I haven't touched that were
previously mentioned though:

1) Denis Lunev suggested having block_module_load_one return the
loaded driver to reduce duplicated for loops in many of the functions
in block.c. I'd be happy to do this but wasn't completely sure how
error handling would happen in that case since currently the return
value is an integer error code. Would I switch to using the
error handling mechanisms provided in util/error.c?

2) In the past some debate was had about the design of the modules.
I haven't made any changes in this regard because it didn't seem a
conclusion had been reached. Some links for background:

Fam Zheng suggested a simpler parser:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg02201.html

Denis Lunev suggested a design more similar to Linux kernel modules:
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05331.html

Richard Jones suggested reasons to keep things as is:
https://lists.nongnu.org/archive/html/qemu-devel/2016-04/msg01994.html

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 +++++++++++++++++++++++---
 include/qemu/module.h           |   3 +
 scripts/modules/module_block.py | 134 ++++++++++++++++++++++++++++++++++++++++
 util/module.c                   |  37 +++--------
 6 files changed, 233 insertions(+), 39 deletions(-)
 create mode 100644 scripts/modules/module_block.py

-- 
2.5.5

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

[Colin Lord]

From: Marc Mari <address@hidden>

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í <address@hidden>
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..005bc49
--- /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 <address@hidden>
+#
+# 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       <address@hidden>
+ *
+ * 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 2/2] blockdev: Add dynamic module loading for block drivers

[Colin Lord]

From: Marc Mari <address@hidden>

Extend the current module interface to allow for block drivers to be loaded
dynamically on request.

The only block drivers that can be converted into modules are the drivers
that don't perform any init operation except for registering themselves. 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í <address@hidden>
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 1/2] blockdev: Add dynamic generation of module_block.h

[Paolo Bonzini]

On 15/06/2016 20:40, Colin Lord wrote:
> +def add_module(fhader, library, format_name, protocol_name,

fhader looks like a typo.

Paolo

> +                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},')

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

[Paolo Bonzini]

On 15/06/2016 20:40, Colin Lord wrote:
> 
> 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.

I don't think it has in this patch :) but you can also move the
iscsi_opts registration to vl.c.

Paolo

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

[Fam Zheng]

On Wed, 06/15 14:40, Colin Lord wrote:
> From: Marc Mari <address@hidden>
> 
> 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í <address@hidden>
> 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..005bc49
> --- /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 <address@hidden>

Address hidden seems like a mistake during copy from web. :)

One more below..

> +#
> +# 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       <address@hidden>

Here!

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

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

[Colin Lord]

On 06/16/2016 12:59 AM, Fam Zheng wrote:
> On Wed, 06/15 14:40, Colin Lord wrote:
>> From: Marc Mari <address@hidden>
>>
>> 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í <address@hidden>
>> 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..005bc49
>> --- /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 <address@hidden>
> 
> Address hidden seems like a mistake during copy from web. :)
> 
> One more below..
> 

Yep, I didn't have the original emails (only the web archives) and I
didn't realize it wasn't supposed to look like that until I sent it out.
I'll fix it for the next version.

>> +#
>> +# 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       <address@hidden>
> 
> Here!
> 
>> + *
>> + * 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
>>
>>
> 
Colin

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

[Colin Lord]

On 06/15/2016 06:50 PM, Paolo Bonzini wrote:
> 
> 
> On 15/06/2016 20:40, Colin Lord wrote:
>>
>> 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.
> 
> I don't think it has in this patch :) but you can also move the
> iscsi_opts registration to vl.c.
> 
> Paolo
> 

Yeah I think Stefan mentioned this point in one of the earlier threads
but he said to do it in a separate commit, which I took to mean I
shouldn't include it here. Should I add it as a third part to this patch
series or leave it for a completely separate commit?

Colin

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

[Paolo Bonzini]

On 16/06/2016 16:00, Colin Lord wrote:
>> >> 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.
> > 
> > I don't think it has in this patch :) but you can also move the
> > iscsi_opts registration to vl.c.
> 
> Yeah I think Stefan mentioned this point in one of the earlier threads
> but he said to do it in a separate commit, which I took to mean I
> shouldn't include it here. Should I add it as a third part to this patch
> series or leave it for a completely separate commit?

The patches in the series are left separate when including them in QEMU.
 Therefore, a separate patch *is* (or will become :)) a separate commit.

Therefore, putting the change before this patch, or alternatively as the
first patch in the series, will be fine.

Thanks,

Paolo

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

[Colin Lord]

On 06/16/2016 10:05 AM, Paolo Bonzini wrote:
> 
> 
> On 16/06/2016 16:00, Colin Lord wrote:
>>>>> 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.
>>>
>>> I don't think it has in this patch :) but you can also move the
>>> iscsi_opts registration to vl.c.
>>
>> Yeah I think Stefan mentioned this point in one of the earlier threads
>> but he said to do it in a separate commit, which I took to mean I
>> shouldn't include it here. Should I add it as a third part to this patch
>> series or leave it for a completely separate commit?
> 
> The patches in the series are left separate when including them in QEMU.
>  Therefore, a separate patch *is* (or will become :)) a separate commit.

Yep, mostly just wasn't sure whether it was considered to be related
enough to include with the other two.
> 
> Therefore, putting the change before this patch, or alternatively as the
> first patch in the series, will be fine.
> 
> Thanks,
> 
> Paolo
> 

Sounds good, I'll work on putting that in the next version then.

Colin

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

[Paolo Bonzini]

On 16/06/2016 16:10, Colin Lord wrote:
> On 06/16/2016 10:05 AM, Paolo Bonzini wrote:
>>
>>
>> On 16/06/2016 16:00, Colin Lord wrote:
>>>>>> 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.
>>>>
>>>> I don't think it has in this patch :) but you can also move the
>>>> iscsi_opts registration to vl.c.
>>>
>>> Yeah I think Stefan mentioned this point in one of the earlier threads
>>> but he said to do it in a separate commit, which I took to mean I
>>> shouldn't include it here. Should I add it as a third part to this patch
>>> series or leave it for a completely separate commit?
>>
>> The patches in the series are left separate when including them in QEMU.
>>  Therefore, a separate patch *is* (or will become :)) a separate commit.
> 
> Yep, mostly just wasn't sure whether it was considered to be related
> enough to include with the other two.

I think it is, since you had to refer to (the lack of) it in a commit
message.

In general, the beauty of patch series is that it's relatively cheap to
add a patch.  One should not overdo it, but a long series of
dependencies benefits neither the author nor the reviewer.

Paolo

>> Therefore, putting the change before this patch, or alternatively as the
>> first patch in the series, will be fine.
>>
>> Thanks,
>>
>> Paolo
>>
> 
> Sounds good, I'll work on putting that in the next version then.
> 
> Colin
> 

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

[Stefan Hajnoczi]

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

On Wed, Jun 15, 2016 at 02:40:53PM -0400, Colin Lord wrote:
> 1) Denis Lunev suggested having block_module_load_one return the
> loaded driver to reduce duplicated for loops in many of the functions
> in block.c. I'd be happy to do this but wasn't completely sure how
> error handling would happen in that case since currently the return
> value is an integer error code. Would I switch to using the
> error handling mechanisms provided in util/error.c?

Yes, change "int foo(...)" to "MyObject *foo(..., Error **errp)".  The
Error object allows functions to provide detailed, human-readable error
messages so it can be a win.

If this change would cause a lot of changes you can stop the refactoring
from snowballing using error_setg_errno() to bridge new Error functions
with old int -errno functions:

  MyObject *foo(..., Error **errp)
  {
      /* I don't want propagate Error to all called functions yet, it
       * would snowball.  So just wrap up the errno:
       */
      ret = legacy_function(...);
      if (ret < 0) {
          error_setg_errno(errp, -ret, "legacy_function failed");
	  return NULL;
      }
  }

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

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

[Stefan Hajnoczi]

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

On Thu, Jun 16, 2016 at 09:57:20AM -0400, Colin Lord wrote:
> On 06/16/2016 12:59 AM, Fam Zheng wrote:
> > On Wed, 06/15 14:40, Colin Lord wrote:
> >> From: Marc Mari <address@hidden>
> >>
> >> 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í <address@hidden>
> >> 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..005bc49
> >> --- /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 <address@hidden>
> > 
> > Address hidden seems like a mistake during copy from web. :)
> > 
> > One more below..
> > 
> 
> Yep, I didn't have the original emails (only the web archives) and I
> didn't realize it wasn't supposed to look like that until I sent it out.
> I'll fix it for the next version.

Mailing list archives are available here in mbox format:

ftp://lists.gnu.org/qemu-devel/

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

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

[Colin Lord]

On 06/17/2016 05:54 AM, Stefan Hajnoczi wrote:
> On Wed, Jun 15, 2016 at 02:40:53PM -0400, Colin Lord wrote:
>> 1) Denis Lunev suggested having block_module_load_one return the
>> loaded driver to reduce duplicated for loops in many of the functions
>> in block.c. I'd be happy to do this but wasn't completely sure how
>> error handling would happen in that case since currently the return
>> value is an integer error code. Would I switch to using the
>> error handling mechanisms provided in util/error.c?
> 
> Yes, change "int foo(...)" to "MyObject *foo(..., Error **errp)".  The
> Error object allows functions to provide detailed, human-readable error
> messages so it can be a win.
> 
> If this change would cause a lot of changes you can stop the refactoring
> from snowballing using error_setg_errno() to bridge new Error functions
> with old int -errno functions:
> 
>   MyObject *foo(..., Error **errp)
>   {
>       /* I don't want propagate Error to all called functions yet, it
>        * would snowball.  So just wrap up the errno:
>        */
>       ret = legacy_function(...);
>       if (ret < 0) {
>           error_setg_errno(errp, -ret, "legacy_function failed");
> 	  return NULL;
>       }
>   }
> 

So I started implementing this part (having block_module_load_one return
the module/driver) last Friday and in the process I realized that it is
not as simple as it seemed to me at first. The duplicated for loops this
was supposed to fix aren't the nicest thing, but I don't think that
returning the block/module directly is any better.

The issue is that a module may contain multiple drivers, so
block_module_load_one would have to know exactly which driver to return,
which seems rather out of scope for that function. The function
registers multiple drivers when the module is loaded, so choosing just
one of them to return seems a little odd.

An alternative way to do this is to return the entire module rather than
just the driver, and let the caller figure out which driver it needs
from the module. However, that would require a loop of some sort anyway
to examine all the drivers in the module, so we're kind of back where we
started. But it is actually a little worse than where we started I think
because as far as I can tell, to actually access the drivers through the
module, you need to know the name of the symbol you want (which I
believe is the name of the BlockDriver structs). I don't see a good way
to know the exact name of the struct that would be robust, so at this
point it seems like it may be better to just leave the duplicated for
loops in place.

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

[Stefan Hajnoczi]

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

On Mon, Jun 20, 2016 at 11:32:38AM -0400, Colin Lord wrote:
> On 06/17/2016 05:54 AM, Stefan Hajnoczi wrote:
> > On Wed, Jun 15, 2016 at 02:40:53PM -0400, Colin Lord wrote:
> >> 1) Denis Lunev suggested having block_module_load_one return the
> >> loaded driver to reduce duplicated for loops in many of the functions
> >> in block.c. I'd be happy to do this but wasn't completely sure how
> >> error handling would happen in that case since currently the return
> >> value is an integer error code. Would I switch to using the
> >> error handling mechanisms provided in util/error.c?
> > 
> > Yes, change "int foo(...)" to "MyObject *foo(..., Error **errp)".  The
> > Error object allows functions to provide detailed, human-readable error
> > messages so it can be a win.
> > 
> > If this change would cause a lot of changes you can stop the refactoring
> > from snowballing using error_setg_errno() to bridge new Error functions
> > with old int -errno functions:
> > 
> >   MyObject *foo(..., Error **errp)
> >   {
> >       /* I don't want propagate Error to all called functions yet, it
> >        * would snowball.  So just wrap up the errno:
> >        */
> >       ret = legacy_function(...);
> >       if (ret < 0) {
> >           error_setg_errno(errp, -ret, "legacy_function failed");
> > 	  return NULL;
> >       }
> >   }
> > 
> 
> So I started implementing this part (having block_module_load_one return
> the module/driver) last Friday and in the process I realized that it is
> not as simple as it seemed to me at first. The duplicated for loops this
> was supposed to fix aren't the nicest thing, but I don't think that
> returning the block/module directly is any better.
> 
> The issue is that a module may contain multiple drivers, so
> block_module_load_one would have to know exactly which driver to return,
> which seems rather out of scope for that function. The function
> registers multiple drivers when the module is loaded, so choosing just
> one of them to return seems a little odd.
> 
> An alternative way to do this is to return the entire module rather than
> just the driver, and let the caller figure out which driver it needs
> from the module. However, that would require a loop of some sort anyway
> to examine all the drivers in the module, so we're kind of back where we
> started. But it is actually a little worse than where we started I think
> because as far as I can tell, to actually access the drivers through the
> module, you need to know the name of the symbol you want (which I
> believe is the name of the BlockDriver structs). I don't see a good way
> to know the exact name of the struct that would be robust, so at this
> point it seems like it may be better to just leave the duplicated for
> loops in place.

I think the issue comes from the fact that you are considering something
like load_block_module(const char *filename) as the API instead of
request_block_driver(const char *driver_name).  In the latter case it's
possible to return a BlockDriver pointer.  In the former it's not.

The request_block_driver() approach requires a mapping from block driver
names to modules.  This can be achieved using a directory layout with
symlinks (hmm...Windows portability?):

  /usr/lib/qemu/block/
    +--- sheepdog.so
    +--- by-protocol/
      +--- sheepdog+unix -> ../sheepdog.so

request_block_driver() would look at
/usr/lib/qemu/block/by-protocol/<protocol> to find the module file.

Stefan

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

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

[Paolo Bonzini]

On 21/06/2016 11:32, Stefan Hajnoczi wrote:
> I think the issue comes from the fact that you are considering something
> like load_block_module(const char *filename) as the API instead of
> request_block_driver(const char *driver_name).  In the latter case it's
> possible to return a BlockDriver pointer.  In the former it's not.
> 
> The request_block_driver() approach requires a mapping from block driver
> names to modules.  This can be achieved using a directory layout with
> symlinks (hmm...Windows portability?):
> 
>   /usr/lib/qemu/block/
>     +--- sheepdog.so
>     +--- by-protocol/
>       +--- sheepdog+unix -> ../sheepdog.so
> 
> request_block_driver() would look at
> /usr/lib/qemu/block/by-protocol/<protocol> to find the module file.

Another possibility is to add a ".loaded" element to the 
block_driver_modules[] array and break the recursion (or infinite loop):


retry:
    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 (!block_driver_modules[i].loaded &&
            !strcmp(block_driver_modules[i].format_name, format_name)) {
            block_driver_modules[i].loaded = true;
	    block_module_load_one(block_driver_modules[i].library_name);
            goto retry;
        }
    }

BTW, please give a name to block_driver_modules[x]'s type, so that you
can assign &block_driver_modules[i] to a pointer and use that as a
shortcut.

Thanks,

Paolo

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

[Colin Lord]

On 06/21/2016 05:32 AM, Stefan Hajnoczi wrote:
> On Mon, Jun 20, 2016 at 11:32:38AM -0400, Colin Lord wrote:
>> On 06/17/2016 05:54 AM, Stefan Hajnoczi wrote:
>>> On Wed, Jun 15, 2016 at 02:40:53PM -0400, Colin Lord wrote:
>>>> 1) Denis Lunev suggested having block_module_load_one return the
>>>> loaded driver to reduce duplicated for loops in many of the functions
>>>> in block.c. I'd be happy to do this but wasn't completely sure how
>>>> error handling would happen in that case since currently the return
>>>> value is an integer error code. Would I switch to using the
>>>> error handling mechanisms provided in util/error.c?
>>>
>>> Yes, change "int foo(...)" to "MyObject *foo(..., Error **errp)".  The
>>> Error object allows functions to provide detailed, human-readable error
>>> messages so it can be a win.
>>>
>>> If this change would cause a lot of changes you can stop the refactoring
>>> from snowballing using error_setg_errno() to bridge new Error functions
>>> with old int -errno functions:
>>>
>>>   MyObject *foo(..., Error **errp)
>>>   {
>>>       /* I don't want propagate Error to all called functions yet, it
>>>        * would snowball.  So just wrap up the errno:
>>>        */
>>>       ret = legacy_function(...);
>>>       if (ret < 0) {
>>>           error_setg_errno(errp, -ret, "legacy_function failed");
>>> 	  return NULL;
>>>       }
>>>   }
>>>
>>
>> So I started implementing this part (having block_module_load_one return
>> the module/driver) last Friday and in the process I realized that it is
>> not as simple as it seemed to me at first. The duplicated for loops this
>> was supposed to fix aren't the nicest thing, but I don't think that
>> returning the block/module directly is any better.
>>
>> The issue is that a module may contain multiple drivers, so
>> block_module_load_one would have to know exactly which driver to return,
>> which seems rather out of scope for that function. The function
>> registers multiple drivers when the module is loaded, so choosing just
>> one of them to return seems a little odd.
>>
>> An alternative way to do this is to return the entire module rather than
>> just the driver, and let the caller figure out which driver it needs
>> from the module. However, that would require a loop of some sort anyway
>> to examine all the drivers in the module, so we're kind of back where we
>> started. But it is actually a little worse than where we started I think
>> because as far as I can tell, to actually access the drivers through the
>> module, you need to know the name of the symbol you want (which I
>> believe is the name of the BlockDriver structs). I don't see a good way
>> to know the exact name of the struct that would be robust, so at this
>> point it seems like it may be better to just leave the duplicated for
>> loops in place.
> 
> I think the issue comes from the fact that you are considering something
> like load_block_module(const char *filename) as the API instead of
> request_block_driver(const char *driver_name).  In the latter case it's
> possible to return a BlockDriver pointer.  In the former it's not.
> 
> The request_block_driver() approach requires a mapping from block driver
> names to modules.  This can be achieved using a directory layout with
> symlinks (hmm...Windows portability?):
> 
>   /usr/lib/qemu/block/
>     +--- sheepdog.so
>     +--- by-protocol/
>       +--- sheepdog+unix -> ../sheepdog.so
> 
> request_block_driver() would look at
> /usr/lib/qemu/block/by-protocol/<protocol> to find the module file.
> 
> Stefan
> 
The question is, even if I was using request_block_driver(const char
*driver_name), I'm still not completely clear in your suggestion how I'm
supposed to get the driver name in the first place. I don't see where
I'd be getting that information from. I'd be happy to hear more about
it, but it also made me think of another possible solution: right now
the block_driver_modules array is being auto-generated by the python
script. Why not just add a "name" field to the struct so that each array
entry would actually know the name of the corresponding BlockDriver struct?

Then request_block_driver would take in the array entry (which is a
struct which currently has no name, which I will probably be fixing as
Paolo asked) and it could load the file using the library name (as
module_load_one does now). It could easily return the BlockDriver then
using the name field. It seems to me that this would work, and would be
a fairly minor change from how things are now (in particular I think
that symlinks wouldn't be necessary with this).

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

[Paolo Bonzini]

On 21/06/2016 17:42, Colin Lord wrote:
> It could easily return the BlockDriver then
> using the name field. It seems to me that this would work, and would be
> a fairly minor change from how things are now (in particular I think
> that symlinks wouldn't be necessary with this).

Yes, I agree.

paolo

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

[Stefan Hajnoczi]

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

On Tue, Jun 21, 2016 at 06:59:58PM +0200, Paolo Bonzini wrote:
> On 21/06/2016 17:42, Colin Lord wrote:
> > It could easily return the BlockDriver then
> > using the name field. It seems to me that this would work, and would be
> > a fairly minor change from how things are now (in particular I think
> > that symlinks wouldn't be necessary with this).
> 
> Yes, I agree.

I like that more than symlinks too.

Regarding where names come from, in general:

1. From the format=, driver=, etc options during opening or creating
   images.  Also from backing filenames inside image files.

2. From probing, see bdrv_find_protocol() and bdrv_find_format().

The simple approach to #2 is to load all modules, but this defeats the
purpose of on-demand loading.

There are solutions for #2 without loading all modules.  You could move
probing functions into the QEMU binary, for example, so that they don't
require loading modules.

Stefan

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