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

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

[Colin Lord]

One more minor revision from v6, no big changes.

v7:
- Add ifdef around qemu_iscsi_opts in vl.c (first patch)

v6:
- Fix bug so that users can specify a modularized driver on the cli
  without qemu exiting
- Remove extra lines from Makefile
- Add patch to modularize NFS

v5:
- No format drivers are modularized, therefore the probe functions are
  all being left completely untouched.
- Remove dmg from block-obj-m since it is not a target of the
  modularization effort.
- Modify module_block.py to only include the library name and protocol
  name fields in the generated struct. The other fields are no longer
  necessary for the drivers that are being modularized.

v4:
- Fix indentation of the generated header file module_block.h
- Drivers and probe functions are now all located in the block/
  directory, rather than being split between block/ and block/probe/. In
  addition the header files for each probe/driver pair are in the block/
  directory, not the include/block/driver/ directory (which no longer
  exists).
- Since the probe files are in block/ now, they follow the naming
  pattern of format-probe.c
- Renamed crypto probe file to be crypto-probe.c, luks is no longer in
  the filename
- Fixed formatting of parallels_probe() function header
- Enforced consistent naming convention for the probe functions. They
  now follow the pattern bdrv_format_probe().

Colin Lord (2):
  blockdev: prepare iSCSI block driver for dynamic loading
  blockdev: Modularize nfs block driver

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

 Makefile                        |  10 ++--
 block.c                         |  62 ++++++++++++++++++++---
 block/Makefile.objs             |   4 +-
 block/iscsi.c                   |  36 --------------
 configure                       |   4 +-
 include/qemu/module.h           |   3 ++
 scripts/modules/module_block.py | 108 ++++++++++++++++++++++++++++++++++++++++
 util/module.c                   |  38 ++++----------
 vl.c                            |  40 +++++++++++++++
 9 files changed, 228 insertions(+), 77 deletions(-)
 create mode 100644 scripts/modules/module_block.py

-- 
2.5.5

[Qemu-devel] [PATCH v7 1/4] 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 the iscsi module to be
dynamically loaded.

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

diff --git a/block/iscsi.c b/block/iscsi.c
index 95ce9e1..c4a0937 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2010,45 +2010,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 c4eeaff..8b562d3 100644
--- a/vl.c
+++ b/vl.c
@@ -506,6 +506,43 @@ static QemuOptsList qemu_fw_cfg_opts = {
     },
 };
 
+#ifdef CONFIG_LIBISCSI
+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 */ }
+    },
+};
+#endif
+
 /**
  * Get machine options
  *
@@ -3001,6 +3038,9 @@ 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);
+#ifdef CONFIG_LIBISCSI
+    qemu_add_opts(&qemu_iscsi_opts);
+#endif
     module_call_init(MODULE_INIT_OPTS);
 
     runstate_init();
-- 
2.5.5

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

[Stefan Hajnoczi]

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

On Mon, Aug 08, 2016 at 02:07:17PM -0400, Colin Lord wrote:
> This commit moves the initialization of the QemuOptsList qemu_iscsi_opts
> struct out of block/iscsi.c in order to allow the iscsi module to be
> dynamically loaded.
> 
> Signed-off-by: Colin Lord <clord@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
>  block/iscsi.c | 36 ------------------------------------
>  vl.c          | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+), 36 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[Qemu-devel] [PATCH v7 2/4] 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
module_block.h automatically from the modules' source code.

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

Signed-off-by: Marc Marí <markmb@redhat.com>
Signed-off-by: Colin Lord <clord@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 Makefile                        |   7 +++
 scripts/modules/module_block.py | 108 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+)
 create mode 100644 scripts/modules/module_block.py

diff --git a/Makefile b/Makefile
index 0d7647f..4b9384e 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 += module_block.h
+
 # Don't try to regenerate Makefile or configure
 # We don't generate any of them
 Makefile: ;
@@ -352,6 +354,11 @@ ivshmem-client$(EXESUF): $(ivshmem-client-obj-y) libqemuutil.a libqemustub.a
 ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) libqemuutil.a libqemustub.a
 	$(call LINK, $^)
 
+module_block.h: $(SRC_PATH)/scripts/modules/module_block.py config-host.mak
+	$(call quiet-command,$(PYTHON) $< $@ \
+	$(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..db4fb54
--- /dev/null
+++ b/scripts/modules/module_block.py
@@ -0,0 +1,108 @@
+#!/usr/bin/python
+#
+# Module information generator
+#
+# Copyright Red Hat, Inc. 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):
+    lines = []
+    lines.append('.library_name = "' + library + '",')
+    if format_name != "":
+        lines.append('.format_name = "' + format_name + '",')
+    if protocol_name != "":
+        lines.append('.protocol_name = "' + protocol_name + '",')
+
+    text = '\n        '.join(lines)
+    fheader.write('\n    {\n        ' + text + '\n    },')
+
+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 == "};":
+                    add_module(fheader, library, format_name, protocol_name)
+                    found_start = False
+            elif line.find("static BlockDriver") != -1:
+                found_something = True
+                found_start = True
+                format_name = ""
+                protocol_name = ""
+
+        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
+ *
+ * Authors:
+ *  Marc Mari       <markmb@redhat.com>
+ */
+
+''')
+
+    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;
+} block_driver_modules[] = {''')
+
+def print_bottom(fheader):
+    fheader.write('''
+};
+
+#endif
+''')
+
+# First argument: output file
+# All other arguments: modules source files (.c)
+output_file = sys.argv[1]
+with open(output_file, '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 v7 2/4] blockdev: Add dynamic generation of module_block.h

[Max Reitz]

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

On 08.08.2016 20:07, Colin Lord wrote:
> From: Marc Mari <markmb@redhat.com>
> 
> To simplify the addition of new block modules, add a script that generates
> module_block.h automatically from the modules' source code.
> 
> This script assumes that the QEMU coding style rules are followed.
> 
> Signed-off-by: Marc Marí <markmb@redhat.com>
> Signed-off-by: Colin Lord <clord@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  Makefile                        |   7 +++
>  scripts/modules/module_block.py | 108 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 115 insertions(+)
>  create mode 100644 scripts/modules/module_block.py
> 

[...]

> diff --git a/scripts/modules/module_block.py b/scripts/modules/module_block.py
> new file mode 100644
> index 0000000..db4fb54
> --- /dev/null
> +++ b/scripts/modules/module_block.py
> @@ -0,0 +1,108 @@
> +#!/usr/bin/python
> +#
> +# Module information generator
> +#
> +# Copyright Red Hat, Inc. 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):
> +    lines = []
> +    lines.append('.library_name = "' + library + '",')
> +    if format_name != "":
> +        lines.append('.format_name = "' + format_name + '",')

Hm, why did you reintroduce some of the format driver handling?

Max

> +    if protocol_name != "":
> +        lines.append('.protocol_name = "' + protocol_name + '",')
> +
> +    text = '\n        '.join(lines)
> +    fheader.write('\n    {\n        ' + text + '\n    },')


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

[Qemu-devel] [PATCH v7 3/4] 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.

In addition, only the protocol drivers are being modularized, as they
are the only ones which see significant performance benefits. The format
drivers do not generally link to external libraries, so modularizing
them is of no benefit from a performance perspective.

All the necessary module information is located in a new structure found
in module_block.h

Signed-off-by: Marc Marí <markmb@redhat.com>
Signed-off-by: Colin Lord <clord@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 Makefile              |  3 ---
 block.c               | 62 +++++++++++++++++++++++++++++++++++++++++++++------
 block/Makefile.objs   |  3 +--
 include/qemu/module.h |  3 +++
 util/module.c         | 38 +++++++++----------------------
 5 files changed, 70 insertions(+), 39 deletions(-)

diff --git a/Makefile b/Makefile
index 4b9384e..c7aa8cd 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 30d64e6..6c5e249 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 "module_block.h"
 #include "qemu/module.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qbool.h"
@@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void)
     return bs;
 }
 
-BlockDriver *bdrv_find_format(const char *format_name)
+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 *drv1;
+    size_t i;
+
+    drv1 = bdrv_do_find_format(format_name);
+    if (drv1) {
+        return drv1;
+    }
+
+    /* 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);
+}
+
 static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
 {
     static const char *whitelist_rw[] = {
@@ -460,6 +484,19 @@ static BlockDriver *find_hdev_driver(const char *filename)
     return drv;
 }
 
+static BlockDriver *bdrv_do_find_protocol(const char *protocol)
+{
+    BlockDriver *drv1;
+
+    QLIST_FOREACH(drv1, &bdrv_drivers, list) {
+        if (drv1->protocol_name && !strcmp(drv1->protocol_name, protocol)) {
+            return drv1;
+        }
+    }
+
+    return NULL;
+}
+
 BlockDriver *bdrv_find_protocol(const char *filename,
                                 bool allow_protocol_prefix,
                                 Error **errp)
@@ -468,6 +505,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 */
 
@@ -494,15 +532,25 @@ 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)) {
-            return drv1;
+
+    drv1 = bdrv_do_find_protocol(protocol);
+    if (drv1) {
+        return drv1;
+    }
+
+    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);
+            break;
         }
     }
 
-    error_setg(errp, "Unknown protocol '%s'", protocol);
-    return NULL;
+    drv1 = bdrv_do_find_protocol(protocol);
+    if (!drv1) {
+        error_setg(errp, "Unknown protocol '%s'", protocol);
+    }
+    return drv1;
 }
 
 /*
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 2593a2f..595f366 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -1,4 +1,4 @@
-block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o
+block-obj-y += raw_bsd.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o dmg.o
 block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
 block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
@@ -39,7 +39,6 @@ gluster.o-libs     := $(GLUSTERFS_LIBS)
 ssh.o-cflags       := $(LIBSSH2_CFLAGS)
 ssh.o-libs         := $(LIBSSH2_LIBS)
 archipelago.o-libs := $(ARCHIPELAGO_LIBS)
-block-obj-m        += dmg.o
 dmg.o-libs         := $(BZIP2_LIBS)
 qcow.o-libs        := -lz
 linux-aio.o-libs   := -laio
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 2370708..dc2c9d4 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 86e3f7a..a5f7fbd 100644
--- a/util/module.c
+++ b/util/module.c
@@ -87,14 +87,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) {
@@ -145,6 +142,7 @@ static int module_load_file(const char *fname)
         ret = -EINVAL;
     } else {
         QTAILQ_FOREACH(e, &dso_init_list, node) {
+            e->init();
             register_module_init(e->init, e->type);
         }
         ret = 0;
@@ -159,14 +157,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;
@@ -177,15 +171,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 ? : "");
@@ -194,16 +179,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 v7 3/4] blockdev: Add dynamic module loading for block drivers

[Max Reitz]

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

On 08.08.2016 20:07, 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.
> 
> In addition, only the protocol drivers are being modularized, as they
> are the only ones which see significant performance benefits. The format
> drivers do not generally link to external libraries, so modularizing
> them is of no benefit from a performance perspective.
> 
> All the necessary module information is located in a new structure found
> in module_block.h
> 
> Signed-off-by: Marc Marí <markmb@redhat.com>
> Signed-off-by: Colin Lord <clord@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  Makefile              |  3 ---
>  block.c               | 62 +++++++++++++++++++++++++++++++++++++++++++++------
>  block/Makefile.objs   |  3 +--
>  include/qemu/module.h |  3 +++
>  util/module.c         | 38 +++++++++----------------------
>  5 files changed, 70 insertions(+), 39 deletions(-)
> 

[...]

> diff --git a/block.c b/block.c
> index 30d64e6..6c5e249 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 "module_block.h"
>  #include "qemu/module.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/qbool.h"
> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void)
>      return bs;
>  }
>  
> -BlockDriver *bdrv_find_format(const char *format_name)
> +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 *drv1;
> +    size_t i;
> +
> +    drv1 = bdrv_do_find_format(format_name);
> +    if (drv1) {
> +        return drv1;
> +    }
> +
> +    /* 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);
> +}
> +

Did you reintroduce this function for dmg? I thought Fam is taking care
of that? I'm confused as to how Fam's patch for dmg and this series are
supposed to interact; the fact that the script added in patch 2 breaks
down with Fam's patch isn't exactly helping...

Hm, so is this series now supposed to be applied without Fam's patch
with the idea of sorting dmg out later on?

Max

>  static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
>  {
>      static const char *whitelist_rw[] = {


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Colin Lord]

On 08/10/2016 02:37 PM, Max Reitz wrote:
> On 08.08.2016 20:07, 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.
>>
>> In addition, only the protocol drivers are being modularized, as they
>> are the only ones which see significant performance benefits. The format
>> drivers do not generally link to external libraries, so modularizing
>> them is of no benefit from a performance perspective.
>>
>> All the necessary module information is located in a new structure found
>> in module_block.h
>>
>> Signed-off-by: Marc Marí <markmb@redhat.com>
>> Signed-off-by: Colin Lord <clord@redhat.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  Makefile              |  3 ---
>>  block.c               | 62 +++++++++++++++++++++++++++++++++++++++++++++------
>>  block/Makefile.objs   |  3 +--
>>  include/qemu/module.h |  3 +++
>>  util/module.c         | 38 +++++++++----------------------
>>  5 files changed, 70 insertions(+), 39 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/block.c b/block.c
>> index 30d64e6..6c5e249 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 "module_block.h"
>>  #include "qemu/module.h"
>>  #include "qapi/qmp/qerror.h"
>>  #include "qapi/qmp/qbool.h"
>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void)
>>      return bs;
>>  }
>>  
>> -BlockDriver *bdrv_find_format(const char *format_name)
>> +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 *drv1;
>> +    size_t i;
>> +
>> +    drv1 = bdrv_do_find_format(format_name);
>> +    if (drv1) {
>> +        return drv1;
>> +    }
>> +
>> +    /* 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);
>> +}
>> +
> 
> Did you reintroduce this function for dmg? I thought Fam is taking care
> of that? I'm confused as to how Fam's patch for dmg and this series are
> supposed to interact; the fact that the script added in patch 2 breaks
> down with Fam's patch isn't exactly helping...
> 
> Hm, so is this series now supposed to be applied without Fam's patch
> with the idea of sorting dmg out later on?
> 
> Max
> 
>>  static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
>>  {
>>      static const char *whitelist_rw[] = {
> 
I'm not completely sure how Fam's patch is supposed to interact with
this series actually. I'm kind of hoping it can be done on top of my
patches at some future point, but in either case this revision was not
done with the dmg patch in mind. The change in find_format was actually
due to a bug I discovered in my patch series (I fixed it in v6, but you
may have missed that).

Essentially, if a user specifies the driver explicitly as part of their
call to qemu, eg driver=gluster, there was a bug in v5 where if the
driver was modularized, it would not be found/loaded. So since gluster
was modularized, if you said driver=gluster on the command line, the
gluster module would not be found. The modules could be found by probing
perfectly fine, this only happened when the driver was specified
manually. The reason is because the drivers get searched based on the
format field if they're specified manually, which means bdrv_find_format
gets called when the driver is specified on the command line. This makes
it necessary for bdrv_find_format to take into account modularized
drivers even though the format drivers are not being modularized. That's
also why the format field was added to the module_block header file again.

Colin

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

[Max Reitz]

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

On 10.08.2016 21:04, Colin Lord wrote:
> On 08/10/2016 02:37 PM, Max Reitz wrote:
>> On 08.08.2016 20:07, 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.
>>>
>>> In addition, only the protocol drivers are being modularized, as they
>>> are the only ones which see significant performance benefits. The format
>>> drivers do not generally link to external libraries, so modularizing
>>> them is of no benefit from a performance perspective.
>>>
>>> All the necessary module information is located in a new structure found
>>> in module_block.h
>>>
>>> Signed-off-by: Marc Marí <markmb@redhat.com>
>>> Signed-off-by: Colin Lord <clord@redhat.com>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  Makefile              |  3 ---
>>>  block.c               | 62 +++++++++++++++++++++++++++++++++++++++++++++------
>>>  block/Makefile.objs   |  3 +--
>>>  include/qemu/module.h |  3 +++
>>>  util/module.c         | 38 +++++++++----------------------
>>>  5 files changed, 70 insertions(+), 39 deletions(-)
>>>
>>
>> [...]
>>
>>> diff --git a/block.c b/block.c
>>> index 30d64e6..6c5e249 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 "module_block.h"
>>>  #include "qemu/module.h"
>>>  #include "qapi/qmp/qerror.h"
>>>  #include "qapi/qmp/qbool.h"
>>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void)
>>>      return bs;
>>>  }
>>>  
>>> -BlockDriver *bdrv_find_format(const char *format_name)
>>> +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 *drv1;
>>> +    size_t i;
>>> +
>>> +    drv1 = bdrv_do_find_format(format_name);
>>> +    if (drv1) {
>>> +        return drv1;
>>> +    }
>>> +
>>> +    /* 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);
>>> +}
>>> +
>>
>> Did you reintroduce this function for dmg? I thought Fam is taking care
>> of that? I'm confused as to how Fam's patch for dmg and this series are
>> supposed to interact; the fact that the script added in patch 2 breaks
>> down with Fam's patch isn't exactly helping...
>>
>> Hm, so is this series now supposed to be applied without Fam's patch
>> with the idea of sorting dmg out later on?
>>
>> Max
>>
>>>  static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
>>>  {
>>>      static const char *whitelist_rw[] = {
>>
> I'm not completely sure how Fam's patch is supposed to interact with
> this series actually. I'm kind of hoping it can be done on top of my
> patches at some future point, but in either case this revision was not
> done with the dmg patch in mind. The change in find_format was actually
> due to a bug I discovered in my patch series (I fixed it in v6, but you
> may have missed that).
> 
> Essentially, if a user specifies the driver explicitly as part of their
> call to qemu, eg driver=gluster, there was a bug in v5 where if the
> driver was modularized, it would not be found/loaded. So since gluster
> was modularized, if you said driver=gluster on the command line, the
> gluster module would not be found. The modules could be found by probing
> perfectly fine, this only happened when the driver was specified
> manually. The reason is because the drivers get searched based on the
> format field if they're specified manually, which means bdrv_find_format
> gets called when the driver is specified on the command line. This makes
> it necessary for bdrv_find_format to take into account modularized
> drivers even though the format drivers are not being modularized. That's
> also why the format field was added to the module_block header file again.

Ah, that makes sense, thanks for explaining.

Patches 1-3:

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Fam Zheng]

On Wed, 08/10 21:06, Max Reitz wrote:
> On 10.08.2016 21:04, Colin Lord wrote:
> > On 08/10/2016 02:37 PM, Max Reitz wrote:
> >> On 08.08.2016 20:07, 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.
> >>>
> >>> In addition, only the protocol drivers are being modularized, as they
> >>> are the only ones which see significant performance benefits. The format
> >>> drivers do not generally link to external libraries, so modularizing
> >>> them is of no benefit from a performance perspective.
> >>>
> >>> All the necessary module information is located in a new structure found
> >>> in module_block.h
> >>>
> >>> Signed-off-by: Marc Marí <markmb@redhat.com>
> >>> Signed-off-by: Colin Lord <clord@redhat.com>
> >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>> ---
> >>>  Makefile              |  3 ---
> >>>  block.c               | 62 +++++++++++++++++++++++++++++++++++++++++++++------
> >>>  block/Makefile.objs   |  3 +--
> >>>  include/qemu/module.h |  3 +++
> >>>  util/module.c         | 38 +++++++++----------------------
> >>>  5 files changed, 70 insertions(+), 39 deletions(-)
> >>>
> >>
> >> [...]
> >>
> >>> diff --git a/block.c b/block.c
> >>> index 30d64e6..6c5e249 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 "module_block.h"
> >>>  #include "qemu/module.h"
> >>>  #include "qapi/qmp/qerror.h"
> >>>  #include "qapi/qmp/qbool.h"
> >>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void)
> >>>      return bs;
> >>>  }
> >>>  
> >>> -BlockDriver *bdrv_find_format(const char *format_name)
> >>> +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 *drv1;
> >>> +    size_t i;
> >>> +
> >>> +    drv1 = bdrv_do_find_format(format_name);
> >>> +    if (drv1) {
> >>> +        return drv1;
> >>> +    }
> >>> +
> >>> +    /* 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);
> >>> +}
> >>> +
> >>
> >> Did you reintroduce this function for dmg? I thought Fam is taking care
> >> of that? I'm confused as to how Fam's patch for dmg and this series are
> >> supposed to interact; the fact that the script added in patch 2 breaks
> >> down with Fam's patch isn't exactly helping...
> >>
> >> Hm, so is this series now supposed to be applied without Fam's patch
> >> with the idea of sorting dmg out later on?
> >>
> >> Max
> >>
> >>>  static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
> >>>  {
> >>>      static const char *whitelist_rw[] = {
> >>
> > I'm not completely sure how Fam's patch is supposed to interact with
> > this series actually. I'm kind of hoping it can be done on top of my
> > patches at some future point, but in either case this revision was not
> > done with the dmg patch in mind. The change in find_format was actually
> > due to a bug I discovered in my patch series (I fixed it in v6, but you
> > may have missed that).
> > 
> > Essentially, if a user specifies the driver explicitly as part of their
> > call to qemu, eg driver=gluster, there was a bug in v5 where if the
> > driver was modularized, it would not be found/loaded. So since gluster
> > was modularized, if you said driver=gluster on the command line, the
> > gluster module would not be found. The modules could be found by probing
> > perfectly fine, this only happened when the driver was specified
> > manually. The reason is because the drivers get searched based on the
> > format field if they're specified manually, which means bdrv_find_format
> > gets called when the driver is specified on the command line. This makes
> > it necessary for bdrv_find_format to take into account modularized
> > drivers even though the format drivers are not being modularized. That's
> > also why the format field was added to the module_block header file again.
> 
> Ah, that makes sense, thanks for explaining.
> 
> Patches 1-3:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 

If we apply this series first, there will be an intermediate state that the
main QEMU binary is linked to libbz2. It doesn't hurt us, but it is better to
make it clear, in case downstream backportings want to keep the library
dependency intact.

So, let's add a word to this commit message, at least.

Fam

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

[Colin Lord]

On 08/10/2016 11:23 PM, Fam Zheng wrote:
> On Wed, 08/10 21:06, Max Reitz wrote:
>> On 10.08.2016 21:04, Colin Lord wrote:
>>> On 08/10/2016 02:37 PM, Max Reitz wrote:
>>>> On 08.08.2016 20:07, 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.
>>>>>
>>>>> In addition, only the protocol drivers are being modularized, as they
>>>>> are the only ones which see significant performance benefits. The format
>>>>> drivers do not generally link to external libraries, so modularizing
>>>>> them is of no benefit from a performance perspective.
>>>>>
>>>>> All the necessary module information is located in a new structure found
>>>>> in module_block.h
>>>>>
>>>>> Signed-off-by: Marc Marí <markmb@redhat.com>
>>>>> Signed-off-by: Colin Lord <clord@redhat.com>
>>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>> ---
>>>>>  Makefile              |  3 ---
>>>>>  block.c               | 62 +++++++++++++++++++++++++++++++++++++++++++++------
>>>>>  block/Makefile.objs   |  3 +--
>>>>>  include/qemu/module.h |  3 +++
>>>>>  util/module.c         | 38 +++++++++----------------------
>>>>>  5 files changed, 70 insertions(+), 39 deletions(-)
>>>>>
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/block.c b/block.c
>>>>> index 30d64e6..6c5e249 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 "module_block.h"
>>>>>  #include "qemu/module.h"
>>>>>  #include "qapi/qmp/qerror.h"
>>>>>  #include "qapi/qmp/qbool.h"
>>>>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void)
>>>>>      return bs;
>>>>>  }
>>>>>  
>>>>> -BlockDriver *bdrv_find_format(const char *format_name)
>>>>> +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 *drv1;
>>>>> +    size_t i;
>>>>> +
>>>>> +    drv1 = bdrv_do_find_format(format_name);
>>>>> +    if (drv1) {
>>>>> +        return drv1;
>>>>> +    }
>>>>> +
>>>>> +    /* 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);
>>>>> +}
>>>>> +
>>>>
>>>> Did you reintroduce this function for dmg? I thought Fam is taking care
>>>> of that? I'm confused as to how Fam's patch for dmg and this series are
>>>> supposed to interact; the fact that the script added in patch 2 breaks
>>>> down with Fam's patch isn't exactly helping...
>>>>
>>>> Hm, so is this series now supposed to be applied without Fam's patch
>>>> with the idea of sorting dmg out later on?
>>>>
>>>> Max
>>>>
>>>>>  static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
>>>>>  {
>>>>>      static const char *whitelist_rw[] = {
>>>>
>>> I'm not completely sure how Fam's patch is supposed to interact with
>>> this series actually. I'm kind of hoping it can be done on top of my
>>> patches at some future point, but in either case this revision was not
>>> done with the dmg patch in mind. The change in find_format was actually
>>> due to a bug I discovered in my patch series (I fixed it in v6, but you
>>> may have missed that).
>>>
>>> Essentially, if a user specifies the driver explicitly as part of their
>>> call to qemu, eg driver=gluster, there was a bug in v5 where if the
>>> driver was modularized, it would not be found/loaded. So since gluster
>>> was modularized, if you said driver=gluster on the command line, the
>>> gluster module would not be found. The modules could be found by probing
>>> perfectly fine, this only happened when the driver was specified
>>> manually. The reason is because the drivers get searched based on the
>>> format field if they're specified manually, which means bdrv_find_format
>>> gets called when the driver is specified on the command line. This makes
>>> it necessary for bdrv_find_format to take into account modularized
>>> drivers even though the format drivers are not being modularized. That's
>>> also why the format field was added to the module_block header file again.
>>
>> Ah, that makes sense, thanks for explaining.
>>
>> Patches 1-3:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
> 
> If we apply this series first, there will be an intermediate state that the
> main QEMU binary is linked to libbz2. It doesn't hurt us, but it is better to
> make it clear, in case downstream backportings want to keep the library
> dependency intact.
> 
> So, let's add a word to this commit message, at least.
> 
> Fam
> 
> 
> 
I guess I'm a little confused about the issue. It seems to me the only
difference between now and before is that if libbz2 is enabled, it will
be linked to the main binary rather than a module. But before, the
modules were always loaded unconditionally at startup anyway, so I'm not
seeing how there is a difference. Either way libbz2 would be loaded. I'm
just not really sure what sort of message I should be adding to the
commit message to indicate this.

Also, would you like me to try and port your patch for dmg to work on
top of this series? I'd still prefer if this series was applied first
because 1) if the dmg patch is done first, this series will have to
change parts of that patch anyway since the block module objects aren't
being loaded unconditionally anymore. That means the bz2 parts would
have to be converted to being dynamically linked at runtime, so it makes
sense to me to just do it that way the first time rather than going back
to change it. And 2) I am leaving soon and may or may not have time to
make this series work on top of the dmg patch. But I'm happy to try and
make your patch to work on top of this series in the little time I have
before I leave.

thanks,
Colin

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

[Fam Zheng]

On Thu, 08/11 12:03, Colin Lord wrote:
> On 08/10/2016 11:23 PM, Fam Zheng wrote:
> > On Wed, 08/10 21:06, Max Reitz wrote:
> >> On 10.08.2016 21:04, Colin Lord wrote:
> >>> On 08/10/2016 02:37 PM, Max Reitz wrote:
> >>>> On 08.08.2016 20:07, 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.
> >>>>>
> >>>>> In addition, only the protocol drivers are being modularized, as they
> >>>>> are the only ones which see significant performance benefits. The format
> >>>>> drivers do not generally link to external libraries, so modularizing
> >>>>> them is of no benefit from a performance perspective.
> >>>>>
> >>>>> All the necessary module information is located in a new structure found
> >>>>> in module_block.h
> >>>>>
> >>>>> Signed-off-by: Marc Marí <markmb@redhat.com>
> >>>>> Signed-off-by: Colin Lord <clord@redhat.com>
> >>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>>>> ---
> >>>>>  Makefile              |  3 ---
> >>>>>  block.c               | 62 +++++++++++++++++++++++++++++++++++++++++++++------
> >>>>>  block/Makefile.objs   |  3 +--
> >>>>>  include/qemu/module.h |  3 +++
> >>>>>  util/module.c         | 38 +++++++++----------------------
> >>>>>  5 files changed, 70 insertions(+), 39 deletions(-)
> >>>>>
> >>>>
> >>>> [...]
> >>>>
> >>>>> diff --git a/block.c b/block.c
> >>>>> index 30d64e6..6c5e249 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 "module_block.h"
> >>>>>  #include "qemu/module.h"
> >>>>>  #include "qapi/qmp/qerror.h"
> >>>>>  #include "qapi/qmp/qbool.h"
> >>>>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void)
> >>>>>      return bs;
> >>>>>  }
> >>>>>  
> >>>>> -BlockDriver *bdrv_find_format(const char *format_name)
> >>>>> +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 *drv1;
> >>>>> +    size_t i;
> >>>>> +
> >>>>> +    drv1 = bdrv_do_find_format(format_name);
> >>>>> +    if (drv1) {
> >>>>> +        return drv1;
> >>>>> +    }
> >>>>> +
> >>>>> +    /* 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);
> >>>>> +}
> >>>>> +
> >>>>
> >>>> Did you reintroduce this function for dmg? I thought Fam is taking care
> >>>> of that? I'm confused as to how Fam's patch for dmg and this series are
> >>>> supposed to interact; the fact that the script added in patch 2 breaks
> >>>> down with Fam's patch isn't exactly helping...
> >>>>
> >>>> Hm, so is this series now supposed to be applied without Fam's patch
> >>>> with the idea of sorting dmg out later on?
> >>>>
> >>>> Max
> >>>>
> >>>>>  static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
> >>>>>  {
> >>>>>      static const char *whitelist_rw[] = {
> >>>>
> >>> I'm not completely sure how Fam's patch is supposed to interact with
> >>> this series actually. I'm kind of hoping it can be done on top of my
> >>> patches at some future point, but in either case this revision was not
> >>> done with the dmg patch in mind. The change in find_format was actually
> >>> due to a bug I discovered in my patch series (I fixed it in v6, but you
> >>> may have missed that).
> >>>
> >>> Essentially, if a user specifies the driver explicitly as part of their
> >>> call to qemu, eg driver=gluster, there was a bug in v5 where if the
> >>> driver was modularized, it would not be found/loaded. So since gluster
> >>> was modularized, if you said driver=gluster on the command line, the
> >>> gluster module would not be found. The modules could be found by probing
> >>> perfectly fine, this only happened when the driver was specified
> >>> manually. The reason is because the drivers get searched based on the
> >>> format field if they're specified manually, which means bdrv_find_format
> >>> gets called when the driver is specified on the command line. This makes
> >>> it necessary for bdrv_find_format to take into account modularized
> >>> drivers even though the format drivers are not being modularized. That's
> >>> also why the format field was added to the module_block header file again.
> >>
> >> Ah, that makes sense, thanks for explaining.
> >>
> >> Patches 1-3:
> >>
> >> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >>
> > 
> > If we apply this series first, there will be an intermediate state that the
> > main QEMU binary is linked to libbz2. It doesn't hurt us, but it is better to
> > make it clear, in case downstream backportings want to keep the library
> > dependency intact.
> > 
> > So, let's add a word to this commit message, at least.
> > 
> > Fam
> > 
> > 
> > 
> I guess I'm a little confused about the issue. It seems to me the only
> difference between now and before is that if libbz2 is enabled, it will
> be linked to the main binary rather than a module. But before, the
> modules were always loaded unconditionally at startup anyway, so I'm not
> seeing how there is a difference. Either way libbz2 would be loaded. I'm
> just not really sure what sort of message I should be adding to the
> commit message to indicate this.

What I propose to be added in the commit message is this:

--- 8< ---

This spoils the purpose of 5505e8b76f (block/dmg: make it modular).

Before this patch, if module build is enabled, block-dmg.so is linked to
libbz2, whereas the main binary is not. In downstream, theoretically, it means
only the qemu-block-extra package depends on libbz2, while the main QEMU
package needn't to.  With this patch, we (temporarily) change the case so that
the main QEMU depends on libbz2 again.

--- >8 ---

> 
> Also, would you like me to try and port your patch for dmg to work on
> top of this series? I'd still prefer if this series was applied first
> because 1) if the dmg patch is done first, this series will have to
> change parts of that patch anyway since the block module objects aren't
> being loaded unconditionally anymore. That means the bz2 parts would
> have to be converted to being dynamically linked at runtime, so it makes
> sense to me to just do it that way the first time rather than going back
> to change it. And 2) I am leaving soon and may or may not have time to
> make this series work on top of the dmg patch. But I'm happy to try and
> make your patch to work on top of this series in the little time I have
> before I leave.

I think it is fine for me to rebase on top of yours because: 1) practically
it probably has no impact - Debian and ArchLinux both put block-dmg.so in the
main QEMU package; 2) it's not a bug to introduce an extra dependency (the only
special thing is, though, in this case it is not absolutely necessary, but it
makes the development easier); 3) we will fix it within the same release.

Fam

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

[Colin Lord]

On 08/11/2016 09:29 PM, Fam Zheng wrote:
> On Thu, 08/11 12:03, Colin Lord wrote:
>> On 08/10/2016 11:23 PM, Fam Zheng wrote:
>>> On Wed, 08/10 21:06, Max Reitz wrote:
>>>> On 10.08.2016 21:04, Colin Lord wrote:
>>>>> On 08/10/2016 02:37 PM, Max Reitz wrote:
>>>>>> On 08.08.2016 20:07, 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.
>>>>>>>
>>>>>>> In addition, only the protocol drivers are being modularized, as they
>>>>>>> are the only ones which see significant performance benefits. The format
>>>>>>> drivers do not generally link to external libraries, so modularizing
>>>>>>> them is of no benefit from a performance perspective.
>>>>>>>
>>>>>>> All the necessary module information is located in a new structure found
>>>>>>> in module_block.h
>>>>>>>
>>>>>>> Signed-off-by: Marc Marí <markmb@redhat.com>
>>>>>>> Signed-off-by: Colin Lord <clord@redhat.com>
>>>>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>>>>> ---
>>>>>>>  Makefile              |  3 ---
>>>>>>>  block.c               | 62 +++++++++++++++++++++++++++++++++++++++++++++------
>>>>>>>  block/Makefile.objs   |  3 +--
>>>>>>>  include/qemu/module.h |  3 +++
>>>>>>>  util/module.c         | 38 +++++++++----------------------
>>>>>>>  5 files changed, 70 insertions(+), 39 deletions(-)
>>>>>>>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> diff --git a/block.c b/block.c
>>>>>>> index 30d64e6..6c5e249 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 "module_block.h"
>>>>>>>  #include "qemu/module.h"
>>>>>>>  #include "qapi/qmp/qerror.h"
>>>>>>>  #include "qapi/qmp/qbool.h"
>>>>>>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void)
>>>>>>>      return bs;
>>>>>>>  }
>>>>>>>  
>>>>>>> -BlockDriver *bdrv_find_format(const char *format_name)
>>>>>>> +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 *drv1;
>>>>>>> +    size_t i;
>>>>>>> +
>>>>>>> +    drv1 = bdrv_do_find_format(format_name);
>>>>>>> +    if (drv1) {
>>>>>>> +        return drv1;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    /* 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);
>>>>>>> +}
>>>>>>> +
>>>>>>
>>>>>> Did you reintroduce this function for dmg? I thought Fam is taking care
>>>>>> of that? I'm confused as to how Fam's patch for dmg and this series are
>>>>>> supposed to interact; the fact that the script added in patch 2 breaks
>>>>>> down with Fam's patch isn't exactly helping...
>>>>>>
>>>>>> Hm, so is this series now supposed to be applied without Fam's patch
>>>>>> with the idea of sorting dmg out later on?
>>>>>>
>>>>>> Max
>>>>>>
>>>>>>>  static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
>>>>>>>  {
>>>>>>>      static const char *whitelist_rw[] = {
>>>>>>
>>>>> I'm not completely sure how Fam's patch is supposed to interact with
>>>>> this series actually. I'm kind of hoping it can be done on top of my
>>>>> patches at some future point, but in either case this revision was not
>>>>> done with the dmg patch in mind. The change in find_format was actually
>>>>> due to a bug I discovered in my patch series (I fixed it in v6, but you
>>>>> may have missed that).
>>>>>
>>>>> Essentially, if a user specifies the driver explicitly as part of their
>>>>> call to qemu, eg driver=gluster, there was a bug in v5 where if the
>>>>> driver was modularized, it would not be found/loaded. So since gluster
>>>>> was modularized, if you said driver=gluster on the command line, the
>>>>> gluster module would not be found. The modules could be found by probing
>>>>> perfectly fine, this only happened when the driver was specified
>>>>> manually. The reason is because the drivers get searched based on the
>>>>> format field if they're specified manually, which means bdrv_find_format
>>>>> gets called when the driver is specified on the command line. This makes
>>>>> it necessary for bdrv_find_format to take into account modularized
>>>>> drivers even though the format drivers are not being modularized. That's
>>>>> also why the format field was added to the module_block header file again.
>>>>
>>>> Ah, that makes sense, thanks for explaining.
>>>>
>>>> Patches 1-3:
>>>>
>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>>
>>>
>>> If we apply this series first, there will be an intermediate state that the
>>> main QEMU binary is linked to libbz2. It doesn't hurt us, but it is better to
>>> make it clear, in case downstream backportings want to keep the library
>>> dependency intact.
>>>
>>> So, let's add a word to this commit message, at least.
>>>
>>> Fam
>>>
>>>
>>>
>> I guess I'm a little confused about the issue. It seems to me the only
>> difference between now and before is that if libbz2 is enabled, it will
>> be linked to the main binary rather than a module. But before, the
>> modules were always loaded unconditionally at startup anyway, so I'm not
>> seeing how there is a difference. Either way libbz2 would be loaded. I'm
>> just not really sure what sort of message I should be adding to the
>> commit message to indicate this.
> 
> What I propose to be added in the commit message is this:
> 
> --- 8< ---
> 
> This spoils the purpose of 5505e8b76f (block/dmg: make it modular).
> 
> Before this patch, if module build is enabled, block-dmg.so is linked to
> libbz2, whereas the main binary is not. In downstream, theoretically, it means
> only the qemu-block-extra package depends on libbz2, while the main QEMU
> package needn't to.  With this patch, we (temporarily) change the case so that
> the main QEMU depends on libbz2 again.
> 
> --- >8 ---
> 
>>
>> Also, would you like me to try and port your patch for dmg to work on
>> top of this series? I'd still prefer if this series was applied first
>> because 1) if the dmg patch is done first, this series will have to
>> change parts of that patch anyway since the block module objects aren't
>> being loaded unconditionally anymore. That means the bz2 parts would
>> have to be converted to being dynamically linked at runtime, so it makes
>> sense to me to just do it that way the first time rather than going back
>> to change it. And 2) I am leaving soon and may or may not have time to
>> make this series work on top of the dmg patch. But I'm happy to try and
>> make your patch to work on top of this series in the little time I have
>> before I leave.
> 
> I think it is fine for me to rebase on top of yours because: 1) practically
> it probably has no impact - Debian and ArchLinux both put block-dmg.so in the
> main QEMU package; 2) it's not a bug to introduce an extra dependency (the only
> special thing is, though, in this case it is not absolutely necessary, but it
> makes the development easier); 3) we will fix it within the same release.
> 
> Fam
> 
Okay sounds good, I'll add the comment and resend to the mailing list.

thanks,
Colin

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

[Colin Lord]

On 08/10/2016 03:04 PM, Colin Lord wrote:
> On 08/10/2016 02:37 PM, Max Reitz wrote:
>> On 08.08.2016 20:07, 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.
>>>
>>> In addition, only the protocol drivers are being modularized, as they
>>> are the only ones which see significant performance benefits. The format
>>> drivers do not generally link to external libraries, so modularizing
>>> them is of no benefit from a performance perspective.
>>>
>>> All the necessary module information is located in a new structure found
>>> in module_block.h
>>>
>>> Signed-off-by: Marc Marí <markmb@redhat.com>
>>> Signed-off-by: Colin Lord <clord@redhat.com>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  Makefile              |  3 ---
>>>  block.c               | 62 +++++++++++++++++++++++++++++++++++++++++++++------
>>>  block/Makefile.objs   |  3 +--
>>>  include/qemu/module.h |  3 +++
>>>  util/module.c         | 38 +++++++++----------------------
>>>  5 files changed, 70 insertions(+), 39 deletions(-)
>>>
>>
>> [...]
>>
>>> diff --git a/block.c b/block.c
>>> index 30d64e6..6c5e249 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 "module_block.h"
>>>  #include "qemu/module.h"
>>>  #include "qapi/qmp/qerror.h"
>>>  #include "qapi/qmp/qbool.h"
>>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void)
>>>      return bs;
>>>  }
>>>  
>>> -BlockDriver *bdrv_find_format(const char *format_name)
>>> +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 *drv1;
>>> +    size_t i;
>>> +
>>> +    drv1 = bdrv_do_find_format(format_name);
>>> +    if (drv1) {
>>> +        return drv1;
>>> +    }
>>> +
>>> +    /* 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);
>>> +}
>>> +
>>
>> Did you reintroduce this function for dmg? I thought Fam is taking care
>> of that? I'm confused as to how Fam's patch for dmg and this series are
>> supposed to interact; the fact that the script added in patch 2 breaks
>> down with Fam's patch isn't exactly helping...
>>
>> Hm, so is this series now supposed to be applied without Fam's patch
>> with the idea of sorting dmg out later on?
>>
>> Max
>>
>>>  static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
>>>  {
>>>      static const char *whitelist_rw[] = {
>>
> I'm not completely sure how Fam's patch is supposed to interact with
> this series actually. I'm kind of hoping it can be done on top of my
> patches at some future point, but in either case this revision was not
> done with the dmg patch in mind. The change in find_format was actually
> due to a bug I discovered in my patch series (I fixed it in v6, but you
> may have missed that).
> 
> Essentially, if a user specifies the driver explicitly as part of their
> call to qemu, eg driver=gluster, there was a bug in v5 where if the
> driver was modularized, it would not be found/loaded. So since gluster
> was modularized, if you said driver=gluster on the command line, the
> gluster module would not be found. The modules could be found by probing
> perfectly fine, this only happened when the driver was specified
> manually. The reason is because the drivers get searched based on the
> format field if they're specified manually, which means bdrv_find_format
> gets called when the driver is specified on the command line. This makes
> it necessary for bdrv_find_format to take into account modularized
> drivers even though the format drivers are not being modularized. That's
> also why the format field was added to the module_block header file again.
> 
> Colin
> 
Oops, this was meant to be a reply to your response of patch 4/4, in
case anyone gets confused.

Colin

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

[Colin Lord]

On 08/10/2016 03:24 PM, Colin Lord wrote:
> On 08/10/2016 03:04 PM, Colin Lord wrote:
>> On 08/10/2016 02:37 PM, Max Reitz wrote:
>>> On 08.08.2016 20:07, 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.
>>>>
>>>> In addition, only the protocol drivers are being modularized, as they
>>>> are the only ones which see significant performance benefits. The format
>>>> drivers do not generally link to external libraries, so modularizing
>>>> them is of no benefit from a performance perspective.
>>>>
>>>> All the necessary module information is located in a new structure found
>>>> in module_block.h
>>>>
>>>> Signed-off-by: Marc Marí <markmb@redhat.com>
>>>> Signed-off-by: Colin Lord <clord@redhat.com>
>>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> ---
>>>>  Makefile              |  3 ---
>>>>  block.c               | 62 +++++++++++++++++++++++++++++++++++++++++++++------
>>>>  block/Makefile.objs   |  3 +--
>>>>  include/qemu/module.h |  3 +++
>>>>  util/module.c         | 38 +++++++++----------------------
>>>>  5 files changed, 70 insertions(+), 39 deletions(-)
>>>>
>>>
>>> [...]
>>>
>>>> diff --git a/block.c b/block.c
>>>> index 30d64e6..6c5e249 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 "module_block.h"
>>>>  #include "qemu/module.h"
>>>>  #include "qapi/qmp/qerror.h"
>>>>  #include "qapi/qmp/qbool.h"
>>>> @@ -241,17 +242,40 @@ BlockDriverState *bdrv_new(void)
>>>>      return bs;
>>>>  }
>>>>  
>>>> -BlockDriver *bdrv_find_format(const char *format_name)
>>>> +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 *drv1;
>>>> +    size_t i;
>>>> +
>>>> +    drv1 = bdrv_do_find_format(format_name);
>>>> +    if (drv1) {
>>>> +        return drv1;
>>>> +    }
>>>> +
>>>> +    /* 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);
>>>> +}
>>>> +
>>>
>>> Did you reintroduce this function for dmg? I thought Fam is taking care
>>> of that? I'm confused as to how Fam's patch for dmg and this series are
>>> supposed to interact; the fact that the script added in patch 2 breaks
>>> down with Fam's patch isn't exactly helping...
>>>
>>> Hm, so is this series now supposed to be applied without Fam's patch
>>> with the idea of sorting dmg out later on?
>>>
>>> Max
>>>
>>>>  static int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
>>>>  {
>>>>      static const char *whitelist_rw[] = {
>>>
>> I'm not completely sure how Fam's patch is supposed to interact with
>> this series actually. I'm kind of hoping it can be done on top of my
>> patches at some future point, but in either case this revision was not
>> done with the dmg patch in mind. The change in find_format was actually
>> due to a bug I discovered in my patch series (I fixed it in v6, but you
>> may have missed that).
>>
>> Essentially, if a user specifies the driver explicitly as part of their
>> call to qemu, eg driver=gluster, there was a bug in v5 where if the
>> driver was modularized, it would not be found/loaded. So since gluster
>> was modularized, if you said driver=gluster on the command line, the
>> gluster module would not be found. The modules could be found by probing
>> perfectly fine, this only happened when the driver was specified
>> manually. The reason is because the drivers get searched based on the
>> format field if they're specified manually, which means bdrv_find_format
>> gets called when the driver is specified on the command line. This makes
>> it necessary for bdrv_find_format to take into account modularized
>> drivers even though the format drivers are not being modularized. That's
>> also why the format field was added to the module_block header file again.
>>
>> Colin
>>
> Oops, this was meant to be a reply to your response of patch 4/4, in
> case anyone gets confused.
> 
> Colin
> 
Nevermind, looks like I was confused. Seems like my eyes are playing
tricks on me, sorry for the spam.

[Qemu-devel] [PATCH v7 4/4] blockdev: Modularize nfs block driver

[Colin Lord]

Modularizes the nfs block driver so that it gets dynamically loaded.

Signed-off-by: Colin Lord <clord@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/Makefile.objs | 1 +
 configure           | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 595f366..fa4d8b8 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -28,6 +28,7 @@ block-obj-y += crypto.o
 common-obj-y += stream.o
 common-obj-y += backup.o
 
+nfs.o-libs         := $(LIBNFS_LIBS)
 iscsi.o-cflags     := $(LIBISCSI_CFLAGS)
 iscsi.o-libs       := $(LIBISCSI_LIBS)
 curl.o-cflags      := $(CURL_CFLAGS)
diff --git a/configure b/configure
index f57fcc6..f1e7d14 100755
--- a/configure
+++ b/configure
@@ -4561,7 +4561,6 @@ if test "$libnfs" != "no" ; then
   if $pkg_config --atleast-version=1.9.3 libnfs; then
     libnfs="yes"
     libnfs_libs=$($pkg_config --libs libnfs)
-    LIBS="$LIBS $libnfs_libs"
   else
     if test "$libnfs" = "yes" ; then
       feature_not_found "libnfs" "Install libnfs devel >= 1.9.3"
@@ -5320,7 +5319,8 @@ if test "$libiscsi" = "yes" ; then
 fi
 
 if test "$libnfs" = "yes" ; then
-  echo "CONFIG_LIBNFS=y" >> $config_host_mak
+  echo "CONFIG_LIBNFS=m" >> $config_host_mak
+  echo "LIBNFS_LIBS=$libnfs_libs" >> $config_host_mak
 fi
 
 if test "$seccomp" = "yes"; then
-- 
2.5.5

Re: [Qemu-devel] [PATCH v7 4/4] blockdev: Modularize nfs block driver

[Max Reitz]

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

On 08.08.2016 20:07, Colin Lord wrote:
> Modularizes the nfs block driver so that it gets dynamically loaded.
> 
> Signed-off-by: Colin Lord <clord@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/Makefile.objs | 1 +
>  configure           | 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)

I'm not quite sure what this achieves. From what I can see, the NFS
block driver is still linked hard into qemu and it is unconditionally
registered at qemu startup.

(The output from a printf() in nfs_block_init() is visible even when
just starting qemu-img or qemu-io without any arguments; most notably
without bdrv_find_protocol() having been invoked at all.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v7 4/4] blockdev: Modularize nfs block driver

[Colin Lord]

On 08/10/2016 03:04 PM, Max Reitz wrote:
> On 08.08.2016 20:07, Colin Lord wrote:
>> Modularizes the nfs block driver so that it gets dynamically loaded.
>>
>> Signed-off-by: Colin Lord <clord@redhat.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  block/Makefile.objs | 1 +
>>  configure           | 4 ++--
>>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> I'm not quite sure what this achieves. From what I can see, the NFS
> block driver is still linked hard into qemu and it is unconditionally
> registered at qemu startup.
> 
ldd seems to be telling me that libnfs is not linked to the main binary
after this patch.
> (The output from a printf() in nfs_block_init() is visible even when
> just starting qemu-img or qemu-io without any arguments; most notably
> without bdrv_find_protocol() having been invoked at all.)
> 
> Max
> 
I can't seem to reproduce this. Is it possible you applied this patch
without applying the first 3 before it? Or maybe didn't have modules
enabled in the configuration? As far as I can tell NFS seems to building
as a module and doesn't seem to be hard linked, so I'm not really sure
what's going on.

Colin

Re: [Qemu-devel] [PATCH v7 4/4] blockdev: Modularize nfs block driver

[Max Reitz]

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

On 10.08.2016 21:22, Colin Lord wrote:
> On 08/10/2016 03:04 PM, Max Reitz wrote:
>> On 08.08.2016 20:07, Colin Lord wrote:
>>> Modularizes the nfs block driver so that it gets dynamically loaded.
>>>
>>> Signed-off-by: Colin Lord <clord@redhat.com>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  block/Makefile.objs | 1 +
>>>  configure           | 4 ++--
>>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> I'm not quite sure what this achieves. From what I can see, the NFS
>> block driver is still linked hard into qemu and it is unconditionally
>> registered at qemu startup.
>>
> ldd seems to be telling me that libnfs is not linked to the main binary
> after this patch.
>> (The output from a printf() in nfs_block_init() is visible even when
>> just starting qemu-img or qemu-io without any arguments; most notably
>> without bdrv_find_protocol() having been invoked at all.)
>>
>> Max
>>
> I can't seem to reproduce this. Is it possible you applied this patch
> without applying the first 3 before it? Or maybe didn't have modules
> enabled in the configuration?

I'm so stupid. Yep, that's it. Works great now. :D

Reviewed-by: Max Reitz <mreitz@redhat.com>

>                               As far as I can tell NFS seems to building
> as a module and doesn't seem to be hard linked, so I'm not really sure
> what's going on.
> 
> Colin
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Max Reitz]

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

On 08.08.2016 20:07, Colin Lord wrote:
> One more minor revision from v6, no big changes.
> 
> v7:
> - Add ifdef around qemu_iscsi_opts in vl.c (first patch)
> 
> v6:
> - Fix bug so that users can specify a modularized driver on the cli
>   without qemu exiting
> - Remove extra lines from Makefile
> - Add patch to modularize NFS
> 
> v5:
> - No format drivers are modularized, therefore the probe functions are
>   all being left completely untouched.
> - Remove dmg from block-obj-m since it is not a target of the
>   modularization effort.
> - Modify module_block.py to only include the library name and protocol
>   name fields in the generated struct. The other fields are no longer
>   necessary for the drivers that are being modularized.
> 
> v4:
> - Fix indentation of the generated header file module_block.h
> - Drivers and probe functions are now all located in the block/
>   directory, rather than being split between block/ and block/probe/. In
>   addition the header files for each probe/driver pair are in the block/
>   directory, not the include/block/driver/ directory (which no longer
>   exists).
> - Since the probe files are in block/ now, they follow the naming
>   pattern of format-probe.c
> - Renamed crypto probe file to be crypto-probe.c, luks is no longer in
>   the filename
> - Fixed formatting of parallels_probe() function header
> - Enforced consistent naming convention for the probe functions. They
>   now follow the pattern bdrv_format_probe().
> 
> Colin Lord (2):
>   blockdev: prepare iSCSI block driver for dynamic loading
>   blockdev: Modularize nfs block driver
> 
> Marc Mari (2):
>   blockdev: Add dynamic generation of module_block.h
>   blockdev: Add dynamic module loading for block drivers
> 
>  Makefile                        |  10 ++--
>  block.c                         |  62 ++++++++++++++++++++---
>  block/Makefile.objs             |   4 +-
>  block/iscsi.c                   |  36 --------------
>  configure                       |   4 +-
>  include/qemu/module.h           |   3 ++
>  scripts/modules/module_block.py | 108 ++++++++++++++++++++++++++++++++++++++++
>  util/module.c                   |  38 ++++----------
>  vl.c                            |  40 +++++++++++++++
>  9 files changed, 228 insertions(+), 77 deletions(-)
>  create mode 100644 scripts/modules/module_block.py

Assuming that this series is supposed to be applied before Fam's patch
for dmg, the first three patches look good. I'm afraid I don't really
understand patch 4, though.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

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

[Max Reitz]

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

On 08.08.2016 20:07, Colin Lord wrote:
> One more minor revision from v6, no big changes.
> 
> v7:
> - Add ifdef around qemu_iscsi_opts in vl.c (first patch)
> 
> v6:
> - Fix bug so that users can specify a modularized driver on the cli
>   without qemu exiting
> - Remove extra lines from Makefile
> - Add patch to modularize NFS
> 
> v5:
> - No format drivers are modularized, therefore the probe functions are
>   all being left completely untouched.
> - Remove dmg from block-obj-m since it is not a target of the
>   modularization effort.
> - Modify module_block.py to only include the library name and protocol
>   name fields in the generated struct. The other fields are no longer
>   necessary for the drivers that are being modularized.
> 
> v4:
> - Fix indentation of the generated header file module_block.h
> - Drivers and probe functions are now all located in the block/
>   directory, rather than being split between block/ and block/probe/. In
>   addition the header files for each probe/driver pair are in the block/
>   directory, not the include/block/driver/ directory (which no longer
>   exists).
> - Since the probe files are in block/ now, they follow the naming
>   pattern of format-probe.c
> - Renamed crypto probe file to be crypto-probe.c, luks is no longer in
>   the filename
> - Fixed formatting of parallels_probe() function header
> - Enforced consistent naming convention for the probe functions. They
>   now follow the pattern bdrv_format_probe().
> 
> Colin Lord (2):
>   blockdev: prepare iSCSI block driver for dynamic loading
>   blockdev: Modularize nfs block driver
> 
> Marc Mari (2):
>   blockdev: Add dynamic generation of module_block.h
>   blockdev: Add dynamic module loading for block drivers
> 
>  Makefile                        |  10 ++--
>  block.c                         |  62 ++++++++++++++++++++---
>  block/Makefile.objs             |   4 +-
>  block/iscsi.c                   |  36 --------------
>  configure                       |   4 +-
>  include/qemu/module.h           |   3 ++
>  scripts/modules/module_block.py | 108 ++++++++++++++++++++++++++++++++++++++++
>  util/module.c                   |  38 ++++----------
>  vl.c                            |  40 +++++++++++++++
>  9 files changed, 228 insertions(+), 77 deletions(-)
>  create mode 100644 scripts/modules/module_block.py

I'd be happy to apply the series as-is, but I'll still give you some
time to decide whether you want to extend patch 3's commit message by
what Fam has proposed.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]