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

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

[Colin Lord]

Here's v5 of the modularization series. Since it seems the concensus is
that modularizing the format drivers is unnecessary, this series no
longer modularizes those and is thus much shorter than before.

v5:
- No format drivers are modularized, therefore the probe functions are
  all being left completely untouched. The bdrv_find_format function is
  also left untouched as a result.
- 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 (1):
  blockdev: prepare iSCSI block driver for dynamic loading

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

 Makefile                        |   7 +++
 block.c                         |  37 ++++++++++++---
 block/Makefile.objs             |   3 +-
 block/iscsi.c                   |  36 --------------
 include/qemu/module.h           |   3 ++
 scripts/modules/module_block.py | 102 ++++++++++++++++++++++++++++++++++++++++
 util/module.c                   |  38 +++++----------
 vl.c                            |  38 +++++++++++++++
 8 files changed, 193 insertions(+), 71 deletions(-)
 create mode 100644 scripts/modules/module_block.py

-- 
2.5.5

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

[Colin Lord]

This commit moves the initialization of the QemuOptsList qemu_iscsi_opts
struct out of block/iscsi.c in order to allow 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          | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 36 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 129c3af..afaf5ee 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2018,45 +2018,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 a455947..1f2d9c8 100644
--- a/vl.c
+++ b/vl.c
@@ -506,6 +506,41 @@ static QemuOptsList qemu_fw_cfg_opts = {
     },
 };
 
+static QemuOptsList qemu_iscsi_opts = {
+    .name = "iscsi",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_iscsi_opts.head),
+    .desc = {
+        {
+            .name = "user",
+            .type = QEMU_OPT_STRING,
+            .help = "username for CHAP authentication to target",
+        },{
+            .name = "password",
+            .type = QEMU_OPT_STRING,
+            .help = "password for CHAP authentication to target",
+        },{
+            .name = "password-secret",
+            .type = QEMU_OPT_STRING,
+            .help = "ID of the secret providing password for CHAP "
+                    "authentication to target",
+        },{
+            .name = "header-digest",
+            .type = QEMU_OPT_STRING,
+            .help = "HeaderDigest setting. "
+                    "{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
+        },{
+            .name = "initiator-name",
+            .type = QEMU_OPT_STRING,
+            .help = "Initiator iqn name to use when connecting",
+        },{
+            .name = "timeout",
+            .type = QEMU_OPT_NUMBER,
+            .help = "Request timeout in seconds (default 0 = no timeout)",
+        },
+        { /* end of list */ }
+    },
+};
+
 /**
  * Get machine options
  *
@@ -3000,6 +3035,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

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

[Colin Lord]

From: Marc Mari <markmb@redhat.com>

To simplify the addition of new block modules, add a script that generates
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>
---
 Makefile                        |   7 +++
 scripts/modules/module_block.py | 102 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 109 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..8b3fcb9
--- /dev/null
+++ b/scripts/modules/module_block.py
@@ -0,0 +1,102 @@
+#!/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, protocol_name):
+    lines = []
+    lines.append('.library_name = "' + library + '",')
+    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(".protocol_name") != -1:
+                    protocol_name = get_string_struct(line)
+                elif line == "};":
+                    add_module(fheader, library, protocol_name)
+                    found_start = False
+            elif line.find("static BlockDriver") != -1:
+                found_something = True
+                found_start = True
+                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 *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

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

[Colin Lord]

From: Marc Mari <markmb@redhat.com>

Extend the current module interface to allow for block drivers to be
loaded dynamically on request. The only block drivers that can be
converted into modules are the drivers that don't perform any init
operation except for registering themselves.

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>
---
 block.c               | 37 +++++++++++++++++++++++++++++++------
 block/Makefile.objs   |  3 +--
 include/qemu/module.h |  3 +++
 util/module.c         | 38 +++++++++++---------------------------
 4 files changed, 46 insertions(+), 35 deletions(-)

diff --git a/block.c b/block.c
index d2dac3d..fbcd3cb 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"
@@ -460,6 +461,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 +482,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 +509,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 v5 0/3] Dynamic module loading for block drivers

[Max Reitz]

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

On 20.07.2016 16:30, Colin Lord wrote:
> Here's v5 of the modularization series. Since it seems the concensus is
> that modularizing the format drivers is unnecessary, this series no
> longer modularizes those and is thus much shorter than before.
> 
> v5:
> - No format drivers are modularized, therefore the probe functions are
>   all being left completely untouched. The bdrv_find_format function is
>   also left untouched as a result.

You also left the (host) device probing functions untouched in this
revision. However, those are actually only used by protocol drivers
(raw-posix and raw-win32, to be specific).

Probably fine since I think it's impossible to build raw-posix or
raw-win32 as a module anyway (because bdrv_file is used as a "static"
reference in block.c).

> - Remove dmg from block-obj-m since it is not a target of the
>   modularization effort.

Hm, I'm afraid I don't quite understand the reasoning behind this.
Intuitively, I'd say "Doesn't matter, it was already modular, so what
prevents it from staying that way?"

Is it because the changes to util/module.c in patch 3 break how the dmg
module worked, e.g. that it was always implicitly fully loaded on qemu
startup if it was available, but now modules are only loaded on request?

Max

> - 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 (1):
>   blockdev: prepare iSCSI block driver for dynamic loading
> 
> Marc Mari (2):
>   blockdev: Add dynamic generation of module_block.h
>   blockdev: Add dynamic module loading for block drivers
> 
>  Makefile                        |   7 +++
>  block.c                         |  37 ++++++++++++---
>  block/Makefile.objs             |   3 +-
>  block/iscsi.c                   |  36 --------------
>  include/qemu/module.h           |   3 ++
>  scripts/modules/module_block.py | 102 ++++++++++++++++++++++++++++++++++++++++
>  util/module.c                   |  38 +++++----------
>  vl.c                            |  38 +++++++++++++++
>  8 files changed, 193 insertions(+), 71 deletions(-)
>  create mode 100644 scripts/modules/module_block.py
> 



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

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

[Colin Lord]

On 07/23/2016 02:21 PM, Max Reitz wrote:
> On 20.07.2016 16:30, Colin Lord wrote:
>> Here's v5 of the modularization series. Since it seems the concensus is
>> that modularizing the format drivers is unnecessary, this series no
>> longer modularizes those and is thus much shorter than before.
>>
>> v5:
>> - No format drivers are modularized, therefore the probe functions are
>>   all being left completely untouched. The bdrv_find_format function is
>>   also left untouched as a result.
> 
> You also left the (host) device probing functions untouched in this
> revision. However, those are actually only used by protocol drivers
> (raw-posix and raw-win32, to be specific).
> 
> Probably fine since I think it's impossible to build raw-posix or
> raw-win32 as a module anyway (because bdrv_file is used as a "static"
> reference in block.c).
> 
>> - Remove dmg from block-obj-m since it is not a target of the
>>   modularization effort.
> 
> Hm, I'm afraid I don't quite understand the reasoning behind this.
> Intuitively, I'd say "Doesn't matter, it was already modular, so what
> prevents it from staying that way?"
> 
> Is it because the changes to util/module.c in patch 3 break how the dmg
> module worked, e.g. that it was always implicitly fully loaded on qemu
> startup if it was available, but now modules are only loaded on request?
> 
Yes, that's pretty much it. Previously all the modules would get loaded
during initialization, but since the third patch adds dynamic loading
that no longer happens. As we aren't loading format drivers on demand,
dmg.o should be added to block-obj-y instead of block-obj-m so that it
doesn't get modularized.

Also, I should mention that the third patch of this series is not quite
right. I was looking at some stuff with John on Friday and he found a
couple lines I somehow didn't delete from qemu/Makefile (around line 250):

block-modules = $(foreach o,$(block-obj-m),"$(basename $(subst
/,-,$o))",) NULL
util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)'

I was pretty sure I had taken care of these but I guess not. It doesn't
actually affect anything as all it's doing is setting
CONFIG_BLOCK_MODULES (which is no longer used anywhere once patch 3 gets
applied), but it's obviously not good to have unused code sitting
around. Should I submit another version with this fixed?

Colin

> Max
> 
>> - 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 (1):
>>   blockdev: prepare iSCSI block driver for dynamic loading
>>
>> Marc Mari (2):
>>   blockdev: Add dynamic generation of module_block.h
>>   blockdev: Add dynamic module loading for block drivers
>>
>>  Makefile                        |   7 +++
>>  block.c                         |  37 ++++++++++++---
>>  block/Makefile.objs             |   3 +-
>>  block/iscsi.c                   |  36 --------------
>>  include/qemu/module.h           |   3 ++
>>  scripts/modules/module_block.py | 102 ++++++++++++++++++++++++++++++++++++++++
>>  util/module.c                   |  38 +++++----------
>>  vl.c                            |  38 +++++++++++++++
>>  8 files changed, 193 insertions(+), 71 deletions(-)
>>  create mode 100644 scripts/modules/module_block.py
>>
> 
> 

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

[Max Reitz]

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

On 25.07.2016 15:56, Colin Lord wrote:
> On 07/23/2016 02:21 PM, Max Reitz wrote:
>> On 20.07.2016 16:30, Colin Lord wrote:
>>> Here's v5 of the modularization series. Since it seems the concensus is
>>> that modularizing the format drivers is unnecessary, this series no
>>> longer modularizes those and is thus much shorter than before.
>>>
>>> v5:
>>> - No format drivers are modularized, therefore the probe functions are
>>>   all being left completely untouched. The bdrv_find_format function is
>>>   also left untouched as a result.
>>
>> You also left the (host) device probing functions untouched in this
>> revision. However, those are actually only used by protocol drivers
>> (raw-posix and raw-win32, to be specific).
>>
>> Probably fine since I think it's impossible to build raw-posix or
>> raw-win32 as a module anyway (because bdrv_file is used as a "static"
>> reference in block.c).
>>
>>> - Remove dmg from block-obj-m since it is not a target of the
>>>   modularization effort.
>>
>> Hm, I'm afraid I don't quite understand the reasoning behind this.
>> Intuitively, I'd say "Doesn't matter, it was already modular, so what
>> prevents it from staying that way?"
>>
>> Is it because the changes to util/module.c in patch 3 break how the dmg
>> module worked, e.g. that it was always implicitly fully loaded on qemu
>> startup if it was available, but now modules are only loaded on request?
>>
> Yes, that's pretty much it. Previously all the modules would get loaded
> during initialization, but since the third patch adds dynamic loading
> that no longer happens. As we aren't loading format drivers on demand,
> dmg.o should be added to block-obj-y instead of block-obj-m so that it
> doesn't get modularized.

OK, then, for the series:

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

> Also, I should mention that the third patch of this series is not quite
> right. I was looking at some stuff with John on Friday and he found a
> couple lines I somehow didn't delete from qemu/Makefile (around line 250):
> 
> block-modules = $(foreach o,$(block-obj-m),"$(basename $(subst
> /,-,$o))",) NULL
> util/module.o-cflags = -D'CONFIG_BLOCK_MODULES=$(block-modules)'
> 
> I was pretty sure I had taken care of these but I guess not. It doesn't
> actually affect anything as all it's doing is setting
> CONFIG_BLOCK_MODULES (which is no longer used anywhere once patch 3 gets
> applied), but it's obviously not good to have unused code sitting
> around. Should I submit another version with this fixed?

While these lines should eventually of course be removed, this can
simply be done in a follow-up patch just as well.

Max

> 
> Colin
> 
>> Max
>>
>>> - 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 (1):
>>>   blockdev: prepare iSCSI block driver for dynamic loading
>>>
>>> Marc Mari (2):
>>>   blockdev: Add dynamic generation of module_block.h
>>>   blockdev: Add dynamic module loading for block drivers
>>>
>>>  Makefile                        |   7 +++
>>>  block.c                         |  37 ++++++++++++---
>>>  block/Makefile.objs             |   3 +-
>>>  block/iscsi.c                   |  36 --------------
>>>  include/qemu/module.h           |   3 ++
>>>  scripts/modules/module_block.py | 102 ++++++++++++++++++++++++++++++++++++++++
>>>  util/module.c                   |  38 +++++----------
>>>  vl.c                            |  38 +++++++++++++++
>>>  8 files changed, 193 insertions(+), 71 deletions(-)
>>>  create mode 100644 scripts/modules/module_block.py
>>>
>>
>>
> 



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

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

[Fam Zheng]

On Mon, 07/25 09:56, Colin Lord wrote:
> >> - Remove dmg from block-obj-m since it is not a target of the
> >>   modularization effort.
> > 
> > Hm, I'm afraid I don't quite understand the reasoning behind this.
> > Intuitively, I'd say "Doesn't matter, it was already modular, so what
> > prevents it from staying that way?"
> > 
> > Is it because the changes to util/module.c in patch 3 break how the dmg
> > module worked, e.g. that it was always implicitly fully loaded on qemu
> > startup if it was available, but now modules are only loaded on request?
> > 
> Yes, that's pretty much it. Previously all the modules would get loaded
> during initialization, but since the third patch adds dynamic loading
> that no longer happens. As we aren't loading format drivers on demand,
> dmg.o should be added to block-obj-y instead of block-obj-m so that it
> doesn't get modularized.

The question should be raised about the extra libbz2 dependency that could be
pulled in to qemu main package again, by this series.  dmg.o was added to
block-obj-m in 5505e8b76 to make it a separate module (and therefore a
downstream package), so that its reference to libbz2, since 6b383c08c, doesn't
add an extra library to the main executable.

Michael can correct me if I'm wrong about that.

For the constructive part.  To prevent the hard dependency, we can probably
extract the BZ2_bzDecompress* part into block/dmg-bz2.so, and use the same
"register" trick as in the SDL series [1] to hook up into dmg_read_chunk().

[1]: https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg06085.html

Fam