[PATCH v5 00/10] build some devices as modules.

[PATCH v5 00/10] build some devices as modules.

[Gerd Hoffmann]

Specifically devices which depend on shared libraries,
to reduce the runtime dependencies of core qemu.

v2:
 - better commit messages.
 - add some more devices.
 - general tidy up.

v3:
 - rebase, solve stubs conflict.
 - fix -vga $name
 - fix -device $name,help

v4:
 - rebase to latest master
 - adapt to armbru's device init changes

v5:
 - move from qdev (aka device) to qom (aka object) level.
 - add braille chardev.

Gerd Hoffmann (10):
  module: qom module support
  object: qom module support
  qdev: device module support
  build: fix device module builds
  ccid: build smartcard as module
  usb: build usb-redir as module
  vga: build qxl as module
  vga: build virtio-gpu only once
  vga: build virtio-gpu as module
  chardev: enable modules, use for braille

 Makefile.objs            |  2 ++
 Makefile.target          |  7 +++++
 include/qemu/module.h    |  2 ++
 include/qom/object.h     | 12 +++++++
 chardev/char.c           |  2 +-
 hw/core/qdev.c           |  6 ++--
 qdev-monitor.c           |  5 +--
 qom/object.c             | 14 +++++++++
 qom/qom-qmp-cmds.c       |  3 +-
 softmmu/vl.c             |  4 +--
 util/module.c            | 67 ++++++++++++++++++++++++++++++++++++++++
 chardev/Makefile.objs    |  5 ++-
 hw/Makefile.objs         |  2 ++
 hw/display/Makefile.objs | 28 ++++++++++-------
 hw/usb/Makefile.objs     | 13 +++++---
 15 files changed, 148 insertions(+), 24 deletions(-)

-- 
2.18.4

[PATCH v5 01/10] module: qom module support

[Gerd Hoffmann]

Add support for qom types provided by modules.  For starters use a
manually maintained list which maps qom type to module and prefix.

Two load functions are added:  One to load the module for a specific
type, and one to load all modules (needed for object/device lists as
printed by -- for example -- qemu -device help).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/qemu/module.h |  2 ++
 util/module.c         | 55 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index 011ae1ae7605..9121a475c1b6 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -70,5 +70,7 @@ void register_dso_module_init(void (*fn)(void), module_init_type type);
 
 void module_call_init(module_init_type type);
 bool module_load_one(const char *prefix, const char *lib_name);
+void module_load_qom_one(const char *type);
+void module_load_qom_all(void);
 
 #endif
diff --git a/util/module.c b/util/module.c
index e48d9aacc05a..ee560a4b4269 100644
--- a/util/module.c
+++ b/util/module.c
@@ -245,3 +245,58 @@ bool module_load_one(const char *prefix, const char *lib_name)
 #endif
     return success;
 }
+
+/*
+ * Building devices and other qom objects modular is mostly useful in
+ * case they have dependencies to external shared libraries, so we can
+ * cut down the core qemu library dependencies.  Which is the case for
+ * only a very few devices & objects.
+ *
+ * So with the expectation that this will be rather the exception than
+ * to rule and the list will not gain that many entries go with a
+ * simple manually maintained list for now.
+ */
+static struct {
+    const char *type;
+    const char *prefix;
+    const char *module;
+} const qom_modules[] = {
+};
+
+static bool module_loaded_qom_all;
+
+void module_load_qom_one(const char *type)
+{
+    int i;
+
+    if (module_loaded_qom_all) {
+        return;
+    }
+    for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
+        if (strcmp(qom_modules[i].type, type) == 0) {
+            module_load_one(qom_modules[i].prefix,
+                            qom_modules[i].module);
+            return;
+        }
+    }
+}
+
+void module_load_qom_all(void)
+{
+    int i;
+
+    if (module_loaded_qom_all) {
+        return;
+    }
+    for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
+        if (i > 0 && (strcmp(qom_modules[i - 1].module,
+                             qom_modules[i].module) == 0 &&
+                      strcmp(qom_modules[i - 1].prefix,
+                             qom_modules[i].prefix) == 0)) {
+            /* one module implementing multiple types -> load only once */
+            continue;
+        }
+        module_load_one(qom_modules[i].prefix, qom_modules[i].module);
+    }
+    module_loaded_qom_all = true;
+}
-- 
2.18.4

[PATCH v5 02/10] object: qom module support

[Gerd Hoffmann]

Little helper function to load modules on demand.  In most cases adding
module loading support for devices and other objects is just
s/object_class_by_name/module_object_class_by_name/ in the right spot.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/qom/object.h | 12 ++++++++++++
 qom/object.c         | 14 ++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index 94a61ccc3fe8..51f188137f1f 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -994,6 +994,18 @@ bool object_class_is_abstract(ObjectClass *klass);
  */
 ObjectClass *object_class_by_name(const char *typename);
 
+/**
+ * module_object_class_by_name:
+ * @typename: The QOM typename to obtain the class for.
+ *
+ * For objects which might be provided by a module.  Behaves like
+ * object_class_by_name, but additionally tries to load the module
+ * needed in case the class is not available.
+ *
+ * Returns: The class for @typename or %NULL if not found.
+ */
+ObjectClass *module_object_class_by_name(const char *typename);
+
 void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
                           const char *implements_type, bool include_abstract,
                           void *opaque);
diff --git a/qom/object.c b/qom/object.c
index 6ece96bc2bfc..34daaf1280f5 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -985,6 +985,20 @@ ObjectClass *object_class_by_name(const char *typename)
     return type->class;
 }
 
+ObjectClass *module_object_class_by_name(const char *typename)
+{
+    ObjectClass *oc;
+
+    oc = object_class_by_name(typename);
+#ifdef CONFIG_MODULES
+    if (!oc) {
+        module_load_qom_one(typename);
+        oc = object_class_by_name(typename);
+    }
+#endif
+    return oc;
+}
+
 ObjectClass *object_class_get_parent(ObjectClass *class)
 {
     TypeImpl *type = type_get_parent(class->type);
-- 
2.18.4

[PATCH v5 03/10] qdev: device module support

[Gerd Hoffmann]

Hook module loading into the places where we
need it when building devices as modules.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/core/qdev.c     | 6 ++++--
 qdev-monitor.c     | 5 +++--
 qom/qom-qmp-cmds.c | 3 ++-
 softmmu/vl.c       | 4 ++--
 4 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 2131c7f951dd..9de16eae05b7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -137,6 +137,9 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
  */
 DeviceState *qdev_new(const char *name)
 {
+    if (!object_class_by_name(name)) {
+        module_load_qom_one(name);
+    }
     return DEVICE(object_new(name));
 }
 
@@ -147,10 +150,9 @@ DeviceState *qdev_new(const char *name)
  */
 DeviceState *qdev_try_new(const char *name)
 {
-    if (!object_class_by_name(name)) {
+    if (!module_object_class_by_name(name)) {
         return NULL;
     }
-
     return DEVICE(object_new(name));
 }
 
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 22da107484c5..8e7a7f7bbdbc 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -147,6 +147,7 @@ static void qdev_print_devinfos(bool show_no_user)
     int i;
     bool cat_printed;
 
+    module_load_qom_all();
     list = object_class_get_list_sorted(TYPE_DEVICE, false);
 
     for (i = 0; i <= DEVICE_CATEGORY_MAX; i++) {
@@ -215,13 +216,13 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
     DeviceClass *dc;
     const char *original_name = *driver;
 
-    oc = object_class_by_name(*driver);
+    oc = module_object_class_by_name(*driver);
     if (!oc) {
         const char *typename = find_typename_by_alias(*driver);
 
         if (typename) {
             *driver = typename;
-            oc = object_class_by_name(*driver);
+            oc = module_object_class_by_name(*driver);
         }
     }
 
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index c5249e44d020..5e2c8cbf333f 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -116,6 +116,7 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
 {
     ObjectTypeInfoList *ret = NULL;
 
+    module_load_qom_all();
     object_class_foreach(qom_list_types_tramp, implements, abstract, &ret);
 
     return ret;
@@ -130,7 +131,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
     ObjectPropertyIterator iter;
     ObjectPropertyInfoList *prop_list = NULL;
 
-    klass = object_class_by_name(typename);
+    klass = module_object_class_by_name(typename);
     if (klass == NULL) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", typename);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index f669c06ede4a..1297815a5f5b 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1772,8 +1772,8 @@ static bool vga_interface_available(VGAInterfaceType t)
 
     assert(t < VGA_TYPE_MAX);
     return !ti->class_names[0] ||
-           object_class_by_name(ti->class_names[0]) ||
-           object_class_by_name(ti->class_names[1]);
+           module_object_class_by_name(ti->class_names[0]) ||
+           module_object_class_by_name(ti->class_names[1]);
 }
 
 static const char *
-- 
2.18.4

[PATCH v5 04/10] build: fix device module builds

[Gerd Hoffmann]

See comment.  Feels quite hackish.  Better ideas anyone?

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile.target | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Makefile.target b/Makefile.target
index 8ed1eba95b9c..c70325df5796 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -179,6 +179,13 @@ endif # CONFIG_SOFTMMU
 dummy := $(call unnest-vars,,obj-y)
 all-obj-y := $(obj-y)
 
+#
+# common-obj-m has some crap here, probably as side effect from
+# filling obj-y.  Clear it.  Fixes suspious dependency errors when
+# building devices as modules.
+#
+common-obj-m :=
+
 include $(SRC_PATH)/Makefile.objs
 dummy := $(call unnest-vars,.., \
                authz-obj-y \
-- 
2.18.4

[PATCH v5 05/10] ccid: build smartcard as module

[Gerd Hoffmann]

Drops libcacard.so dependency from core qemu.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile.objs        | 1 +
 util/module.c        | 2 ++
 hw/Makefile.objs     | 1 +
 hw/usb/Makefile.objs | 4 +++-
 4 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Makefile.objs b/Makefile.objs
index 7ce2588b89a3..ca555ede0710 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -59,6 +59,7 @@ common-obj-y += migration/
 common-obj-y += audio/
 common-obj-m += audio/
 common-obj-y += hw/
+common-obj-m += hw/
 
 common-obj-y += replay/
 
diff --git a/util/module.c b/util/module.c
index ee560a4b4269..89da9a3cce05 100644
--- a/util/module.c
+++ b/util/module.c
@@ -261,6 +261,8 @@ static struct {
     const char *prefix;
     const char *module;
 } const qom_modules[] = {
+    { "ccid-card-passthru",    "hw-", "usb-smartcard"         },
+    { "ccid-card-emulated",    "hw-", "usb-smartcard"         },
 };
 
 static bool module_loaded_qom_all;
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 4cbe5e4e57d6..af8fd9a510ed 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -43,4 +43,5 @@ devices-dirs-y += smbios/
 endif
 
 common-obj-y += $(devices-dirs-y)
+common-obj-m += usb/
 obj-y += $(devices-dirs-y)
diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
index fa5c3fa1b877..3c5b3d4fadd3 100644
--- a/hw/usb/Makefile.objs
+++ b/hw/usb/Makefile.objs
@@ -29,11 +29,13 @@ common-obj-$(CONFIG_USB_NETWORK)      += dev-network.o
 
 ifeq ($(CONFIG_USB_SMARTCARD),y)
 common-obj-y                          += dev-smartcard-reader.o
-common-obj-$(CONFIG_SMARTCARD)        += smartcard.mo
+ifeq ($(CONFIG_SMARTCARD),y)
+common-obj-m                          += smartcard.mo
 smartcard.mo-objs := ccid-card-passthru.o ccid-card-emulated.o
 smartcard.mo-cflags := $(SMARTCARD_CFLAGS)
 smartcard.mo-libs := $(SMARTCARD_LIBS)
 endif
+endif
 
 ifeq ($(CONFIG_POSIX),y)
 common-obj-$(CONFIG_USB_STORAGE_MTP)  += dev-mtp.o
-- 
2.18.4

[PATCH v5 06/10] usb: build usb-redir as module

[Gerd Hoffmann]

Drops libusbredirparser.so dependency from core qemu.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/module.c        | 1 +
 hw/usb/Makefile.objs | 9 ++++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/util/module.c b/util/module.c
index 89da9a3cce05..e3226165e91c 100644
--- a/util/module.c
+++ b/util/module.c
@@ -263,6 +263,7 @@ static struct {
 } const qom_modules[] = {
     { "ccid-card-passthru",    "hw-", "usb-smartcard"         },
     { "ccid-card-emulated",    "hw-", "usb-smartcard"         },
+    { "usb-redir",             "hw-", "usb-redirect"          },
 };
 
 static bool module_loaded_qom_all;
diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
index 3c5b3d4fadd3..e342ff59fab0 100644
--- a/hw/usb/Makefile.objs
+++ b/hw/usb/Makefile.objs
@@ -43,9 +43,12 @@ endif
 
 # usb redirection
 ifeq ($(CONFIG_USB),y)
-common-obj-$(CONFIG_USB_REDIR) += redirect.o quirks.o
-redirect.o-cflags = $(USB_REDIR_CFLAGS)
-redirect.o-libs = $(USB_REDIR_LIBS)
+ifeq ($(CONFIG_USB_REDIR),y)
+common-obj-m += redirect.mo
+redirect.mo-objs = redirect.o quirks.o
+redirect.mo-cflags = $(USB_REDIR_CFLAGS)
+redirect.mo-libs = $(USB_REDIR_LIBS)
+endif
 endif
 
 # usb pass-through
-- 
2.18.4

[PATCH v5 07/10] vga: build qxl as module

[Gerd Hoffmann]

First step in making spice support modular.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/module.c            | 2 ++
 hw/Makefile.objs         | 1 +
 hw/display/Makefile.objs | 5 ++++-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/util/module.c b/util/module.c
index e3226165e91c..7c76d2a84b94 100644
--- a/util/module.c
+++ b/util/module.c
@@ -264,6 +264,8 @@ static struct {
     { "ccid-card-passthru",    "hw-", "usb-smartcard"         },
     { "ccid-card-emulated",    "hw-", "usb-smartcard"         },
     { "usb-redir",             "hw-", "usb-redirect"          },
+    { "qxl-vga",               "hw-", "display-qxl"           },
+    { "qxl",                   "hw-", "display-qxl"           },
 };
 
 static bool module_loaded_qom_all;
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index af8fd9a510ed..14b7ea4eb62e 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -43,5 +43,6 @@ devices-dirs-y += smbios/
 endif
 
 common-obj-y += $(devices-dirs-y)
+common-obj-m += display/
 common-obj-m += usb/
 obj-y += $(devices-dirs-y)
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index 77a7d622bd2d..76b3571e4902 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -44,7 +44,10 @@ common-obj-$(CONFIG_ARTIST) += artist.o
 
 obj-$(CONFIG_VGA) += vga.o
 
-common-obj-$(CONFIG_QXL) += qxl.o qxl-logger.o qxl-render.o
+ifeq ($(CONFIG_QXL),y)
+common-obj-m += qxl.mo
+qxl.mo-objs = qxl.o qxl-logger.o qxl-render.o
+endif
 
 obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
 obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
-- 
2.18.4

[PATCH v5 08/10] vga: build virtio-gpu only once

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/display/Makefile.objs | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index 76b3571e4902..d619594ad4d3 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -49,12 +49,12 @@ common-obj-m += qxl.mo
 qxl.mo-objs = qxl.o qxl-logger.o qxl-render.o
 endif
 
-obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
-obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
-obj-$(call land,$(CONFIG_VIRTIO_GPU),$(CONFIG_VIRTIO_PCI)) += virtio-gpu-pci.o
-obj-$(call land,$(CONFIG_VHOST_USER_GPU),$(CONFIG_VIRTIO_PCI)) += vhost-user-gpu-pci.o
-obj-$(CONFIG_VIRTIO_VGA) += virtio-vga.o
-obj-$(CONFIG_VHOST_USER_VGA) += vhost-user-vga.o
+common-obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
+common-obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
+common-obj-$(call land,$(CONFIG_VIRTIO_GPU),$(CONFIG_VIRTIO_PCI)) += virtio-gpu-pci.o
+common-obj-$(call land,$(CONFIG_VHOST_USER_GPU),$(CONFIG_VIRTIO_PCI)) += vhost-user-gpu-pci.o
+common-obj-$(CONFIG_VIRTIO_VGA) += virtio-vga.o
+common-obj-$(CONFIG_VHOST_USER_VGA) += vhost-user-vga.o
 virtio-gpu.o-cflags := $(VIRGL_CFLAGS)
 virtio-gpu.o-libs += $(VIRGL_LIBS)
 virtio-gpu-3d.o-cflags := $(VIRGL_CFLAGS)
-- 
2.18.4

[PATCH v5 09/10] vga: build virtio-gpu as module

[Gerd Hoffmann]

Drops libvirglrenderer.so dependency from core qemu.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 util/module.c            |  6 ++++++
 hw/display/Makefile.objs | 23 +++++++++++++----------
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/util/module.c b/util/module.c
index 7c76d2a84b94..a74214eac052 100644
--- a/util/module.c
+++ b/util/module.c
@@ -266,6 +266,12 @@ static struct {
     { "usb-redir",             "hw-", "usb-redirect"          },
     { "qxl-vga",               "hw-", "display-qxl"           },
     { "qxl",                   "hw-", "display-qxl"           },
+    { "virtio-gpu-device",     "hw-", "display-virtio-gpu"    },
+    { "virtio-gpu-pci",        "hw-", "display-virtio-gpu"    },
+    { "virtio-vga",            "hw-", "display-virtio-gpu"    },
+    { "vhost-user-gpu-device", "hw-", "display-virtio-gpu"    },
+    { "vhost-user-gpu-pci",    "hw-", "display-virtio-gpu"    },
+    { "vhost-user-vga",        "hw-", "display-virtio-gpu"    },
 };
 
 static bool module_loaded_qom_all;
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index d619594ad4d3..e907f3182b0c 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -49,16 +49,19 @@ common-obj-m += qxl.mo
 qxl.mo-objs = qxl.o qxl-logger.o qxl-render.o
 endif
 
-common-obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
-common-obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
-common-obj-$(call land,$(CONFIG_VIRTIO_GPU),$(CONFIG_VIRTIO_PCI)) += virtio-gpu-pci.o
-common-obj-$(call land,$(CONFIG_VHOST_USER_GPU),$(CONFIG_VIRTIO_PCI)) += vhost-user-gpu-pci.o
-common-obj-$(CONFIG_VIRTIO_VGA) += virtio-vga.o
-common-obj-$(CONFIG_VHOST_USER_VGA) += vhost-user-vga.o
-virtio-gpu.o-cflags := $(VIRGL_CFLAGS)
-virtio-gpu.o-libs += $(VIRGL_LIBS)
-virtio-gpu-3d.o-cflags := $(VIRGL_CFLAGS)
-virtio-gpu-3d.o-libs += $(VIRGL_LIBS)
+ifeq ($(CONFIG_VIRTIO_GPU),y)
+common-obj-m += virtio-gpu.mo
+virtio-gpu-obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
+virtio-gpu-obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
+virtio-gpu-obj-$(call land,$(CONFIG_VIRTIO_GPU),$(CONFIG_VIRTIO_PCI)) += virtio-gpu-pci.o
+virtio-gpu-obj-$(call land,$(CONFIG_VHOST_USER_GPU),$(CONFIG_VIRTIO_PCI)) += vhost-user-gpu-pci.o
+virtio-gpu-obj-$(CONFIG_VIRTIO_VGA) += virtio-vga.o
+virtio-gpu-obj-$(CONFIG_VHOST_USER_VGA) += vhost-user-vga.o
+virtio-gpu.mo-objs := $(virtio-gpu-obj-y)
+virtio-gpu.mo-cflags := $(VIRGL_CFLAGS)
+virtio-gpu.mo-libs := $(VIRGL_LIBS)
+endif
+
 common-obj-$(CONFIG_DPCD) += dpcd.o
 common-obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx_dp.o
 
-- 
2.18.4

[PATCH v5 10/10] chardev: enable modules, use for braille

[Gerd Hoffmann]

Removes brlapi library dependency from core qemu.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile.objs         | 1 +
 chardev/char.c        | 2 +-
 util/module.c         | 1 +
 chardev/Makefile.objs | 5 ++++-
 4 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index ca555ede0710..2dfcd19713f8 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -71,6 +71,7 @@ common-obj-$(CONFIG_TPM) += tpm.o
 
 common-obj-y += backends/
 common-obj-y += chardev/
+common-obj-m += chardev/
 
 common-obj-$(CONFIG_SECCOMP) += qemu-seccomp.o
 qemu-seccomp.o-cflags := $(SECCOMP_CFLAGS)
diff --git a/chardev/char.c b/chardev/char.c
index e3051295ac37..df697f3ce9e0 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -527,7 +527,7 @@ static const ChardevClass *char_get_class(const char *driver, Error **errp)
     const ChardevClass *cc;
     char *typename = g_strdup_printf("chardev-%s", driver);
 
-    oc = object_class_by_name(typename);
+    oc = module_object_class_by_name(typename);
     g_free(typename);
 
     if (!object_class_dynamic_cast(oc, TYPE_CHARDEV)) {
diff --git a/util/module.c b/util/module.c
index a74214eac052..32b0547b8266 100644
--- a/util/module.c
+++ b/util/module.c
@@ -272,6 +272,7 @@ static struct {
     { "vhost-user-gpu-device", "hw-", "display-virtio-gpu"    },
     { "vhost-user-gpu-pci",    "hw-", "display-virtio-gpu"    },
     { "vhost-user-vga",        "hw-", "display-virtio-gpu"    },
+    { "chardev-braille",       "chardev-", "baum"             },
 };
 
 static bool module_loaded_qom_all;
diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
index d68e1347f9af..3a58c9d329d6 100644
--- a/chardev/Makefile.objs
+++ b/chardev/Makefile.objs
@@ -18,8 +18,11 @@ chardev-obj-$(CONFIG_WIN32) += char-win.o
 chardev-obj-$(CONFIG_WIN32) += char-win-stdio.o
 
 common-obj-y += msmouse.o wctablet.o testdev.o
-common-obj-$(CONFIG_BRLAPI) += baum.o
+
+ifeq ($(CONFIG_BRLAPI),y)
+common-obj-m += baum.o
 baum.o-cflags := $(SDL_CFLAGS)
 baum.o-libs := $(BRLAPI_LIBS)
+endif
 
 common-obj-$(CONFIG_SPICE) += spice.o
-- 
2.18.4

Re: [PATCH v5 04/10] build: fix device module builds

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> See comment.  Feels quite hackish.  Better ideas anyone?
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  Makefile.target | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Makefile.target b/Makefile.target
> index 8ed1eba95b9c..c70325df5796 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -179,6 +179,13 @@ endif # CONFIG_SOFTMMU
>  dummy := $(call unnest-vars,,obj-y)
>  all-obj-y := $(obj-y)
>
> +#
> +# common-obj-m has some crap here, probably as side effect from
> +# filling obj-y.  Clear it.  Fixes suspious dependency errors when

Typo: suspicious

> +# building devices as modules.
> +#

(As an aside: I'm also not filled with confidence by this comment ;-)

> +common-obj-m :=
> +
>  include $(SRC_PATH)/Makefile.objs
>  dummy := $(call unnest-vars,.., \
>                 authz-obj-y \


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 01/10] module: qom module support

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Add support for qom types provided by modules.  For starters use a
> manually maintained list which maps qom type to module and prefix.
>
> Two load functions are added:  One to load the module for a specific
> type, and one to load all modules (needed for object/device lists as
> printed by -- for example -- qemu -device help).
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/qemu/module.h |  2 ++
>  util/module.c         | 55 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
>
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 011ae1ae7605..9121a475c1b6 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -70,5 +70,7 @@ void register_dso_module_init(void (*fn)(void), module_init_type type);
>
>  void module_call_init(module_init_type type);
>  bool module_load_one(const char *prefix, const char *lib_name);
> +void module_load_qom_one(const char *type);
> +void module_load_qom_all(void);
>
>  #endif
> diff --git a/util/module.c b/util/module.c
> index e48d9aacc05a..ee560a4b4269 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -245,3 +245,58 @@ bool module_load_one(const char *prefix, const char *lib_name)
>  #endif
>      return success;
>  }
> +
> +/*
> + * Building devices and other qom objects modular is mostly useful in
> + * case they have dependencies to external shared libraries, so we can
> + * cut down the core qemu library dependencies.  Which is the case for
> + * only a very few devices & objects.
> + *
> + * So with the expectation that this will be rather the exception than
> + * to rule and the list will not gain that many entries go with a

Nit: Add a comma after "entries"

I would also indicate that this list needs to be sorted by prefix/module

> + * simple manually maintained list for now.
> + */
> +static struct {
> +    const char *type;
> +    const char *prefix;
> +    const char *module;

Because of the sorting rule, it is more intuitive to put the module first
and the type last, for cases where you have

    mod1 prefix1 type1
    mod1 prefix1 type2
    mod1 prefix1 type3
    mod2 prefix2 type1

Visually, I expect the constants to come first.


> +} const qom_modules[] = {
> +};
> +
> +static bool module_loaded_qom_all;
> +
> +void module_load_qom_one(const char *type)
> +{
> +    int i;
> +
> +    if (module_loaded_qom_all) {
> +        return;
> +    }
> +    for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
> +        if (strcmp(qom_modules[i].type, type) == 0) {
> +            module_load_one(qom_modules[i].prefix,
> +                            qom_modules[i].module);
> +            return;
> +        }
> +    }
> +}
> +
> +void module_load_qom_all(void)
> +{
> +    int i;
> +
> +    if (module_loaded_qom_all) {
> +        return;
> +    }
> +    for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
> +        if (i > 0 && (strcmp(qom_modules[i - 1].module,
> +                             qom_modules[i].module) == 0 &&
> +                      strcmp(qom_modules[i - 1].prefix,
> +                             qom_modules[i].prefix) == 0)) {
> +            /* one module implementing multiple types -> load only once */

This is the sorting requirement I'm talking about

> +            continue;
> +        }
> +        module_load_one(qom_modules[i].prefix, qom_modules[i].module);
> +    }
> +    module_loaded_qom_all = true;
> +}


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 02/10] object: qom module support

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Little helper function to load modules on demand.  In most cases adding
> module loading support for devices and other objects is just
> s/object_class_by_name/module_object_class_by_name/ in the right spot.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/qom/object.h | 12 ++++++++++++
>  qom/object.c         | 14 ++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 94a61ccc3fe8..51f188137f1f 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -994,6 +994,18 @@ bool object_class_is_abstract(ObjectClass *klass);
>   */
>  ObjectClass *object_class_by_name(const char *typename);
>
> +/**
> + * module_object_class_by_name:
> + * @typename: The QOM typename to obtain the class for.
> + *
> + * For objects which might be provided by a module.  Behaves like
> + * object_class_by_name, but additionally tries to load the module
> + * needed in case the class is not available.
> + *
> + * Returns: The class for @typename or %NULL if not found.
> + */
> +ObjectClass *module_object_class_by_name(const char *typename);
> +
>  void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
>                            const char *implements_type, bool include_abstract,
>                            void *opaque);
> diff --git a/qom/object.c b/qom/object.c
> index 6ece96bc2bfc..34daaf1280f5 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -985,6 +985,20 @@ ObjectClass *object_class_by_name(const char *typename)
>      return type->class;
>  }
>
> +ObjectClass *module_object_class_by_name(const char *typename)
> +{
> +    ObjectClass *oc;
> +
> +    oc = object_class_by_name(typename);
> +#ifdef CONFIG_MODULES
> +    if (!oc) {
> +        module_load_qom_one(typename);
> +        oc = object_class_by_name(typename);
> +    }
> +#endif

I'm wondering if there is any reason to only trigger the module load when
you don't find the object class. You could simply call module_load_qom_one
under #ifdef CONFIG_MODULES.

Performance wise, I don't think this makes much of a difference, and it
simplifies the logical flow IMO.

> +    return oc;
> +}
> +
>  ObjectClass *object_class_get_parent(ObjectClass *class)
>  {
>      TypeImpl *type = type_get_parent(class->type);


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 03/10] qdev: device module support

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Hook module loading into the places where we
> need it when building devices as modules.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/core/qdev.c     | 6 ++++--
>  qdev-monitor.c     | 5 +++--
>  qom/qom-qmp-cmds.c | 3 ++-
>  softmmu/vl.c       | 4 ++--
>  4 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 2131c7f951dd..9de16eae05b7 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -137,6 +137,9 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
>   */
>  DeviceState *qdev_new(const char *name)
>  {
> +    if (!object_class_by_name(name)) {
> +        module_load_qom_one(name);
> +    }

Curious why you don't you call module_object_class_by_name here?


>      return DEVICE(object_new(name));
>  }
>
> @@ -147,10 +150,9 @@ DeviceState *qdev_new(const char *name)
>   */
>  DeviceState *qdev_try_new(const char *name)
>  {
> -    if (!object_class_by_name(name)) {
> +    if (!module_object_class_by_name(name)) {
>          return NULL;
>      }
> -
>      return DEVICE(object_new(name));
>  }
>
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 22da107484c5..8e7a7f7bbdbc 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -147,6 +147,7 @@ static void qdev_print_devinfos(bool show_no_user)
>      int i;
>      bool cat_printed;
>
> +    module_load_qom_all();
>      list = object_class_get_list_sorted(TYPE_DEVICE, false);
>
>      for (i = 0; i <= DEVICE_CATEGORY_MAX; i++) {
> @@ -215,13 +216,13 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
>      DeviceClass *dc;
>      const char *original_name = *driver;
>
> -    oc = object_class_by_name(*driver);
> +    oc = module_object_class_by_name(*driver);
>      if (!oc) {
>          const char *typename = find_typename_by_alias(*driver);
>
>          if (typename) {
>              *driver = typename;
> -            oc = object_class_by_name(*driver);
> +            oc = module_object_class_by_name(*driver);
>          }
>      }
>
> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
> index c5249e44d020..5e2c8cbf333f 100644
> --- a/qom/qom-qmp-cmds.c
> +++ b/qom/qom-qmp-cmds.c
> @@ -116,6 +116,7 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
>  {
>      ObjectTypeInfoList *ret = NULL;
>
> +    module_load_qom_all();
>      object_class_foreach(qom_list_types_tramp, implements, abstract, &ret);
>
>      return ret;
> @@ -130,7 +131,7 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
>      ObjectPropertyIterator iter;
>      ObjectPropertyInfoList *prop_list = NULL;
>
> -    klass = object_class_by_name(typename);
> +    klass = module_object_class_by_name(typename);
>      if (klass == NULL) {
>          error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>                    "Device '%s' not found", typename);
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index f669c06ede4a..1297815a5f5b 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1772,8 +1772,8 @@ static bool vga_interface_available(VGAInterfaceType t)
>
>      assert(t < VGA_TYPE_MAX);
>      return !ti->class_names[0] ||
> -           object_class_by_name(ti->class_names[0]) ||
> -           object_class_by_name(ti->class_names[1]);
> +           module_object_class_by_name(ti->class_names[0]) ||
> +           module_object_class_by_name(ti->class_names[1]);
>  }
>
>  static const char *


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 05/10] ccid: build smartcard as module

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Drops libcacard.so dependency from core qemu.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  Makefile.objs        | 1 +
>  util/module.c        | 2 ++
>  hw/Makefile.objs     | 1 +
>  hw/usb/Makefile.objs | 4 +++-
>  4 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 7ce2588b89a3..ca555ede0710 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -59,6 +59,7 @@ common-obj-y += migration/
>  common-obj-y += audio/
>  common-obj-m += audio/
>  common-obj-y += hw/
> +common-obj-m += hw/
>
>  common-obj-y += replay/
>
> diff --git a/util/module.c b/util/module.c
> index ee560a4b4269..89da9a3cce05 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -261,6 +261,8 @@ static struct {
>      const char *prefix;
>      const char *module;
>  } const qom_modules[] = {
> +    { "ccid-card-passthru",    "hw-", "usb-smartcard"         },
> +    { "ccid-card-emulated",    "hw-", "usb-smartcard"         },
>  };
>
>  static bool module_loaded_qom_all;
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 4cbe5e4e57d6..af8fd9a510ed 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -43,4 +43,5 @@ devices-dirs-y += smbios/
>  endif
>
>  common-obj-y += $(devices-dirs-y)
> +common-obj-m += usb/
>  obj-y += $(devices-dirs-y)
> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
> index fa5c3fa1b877..3c5b3d4fadd3 100644
> --- a/hw/usb/Makefile.objs
> +++ b/hw/usb/Makefile.objs
> @@ -29,11 +29,13 @@ common-obj-$(CONFIG_USB_NETWORK)      += dev-network.o
>
>  ifeq ($(CONFIG_USB_SMARTCARD),y)
>  common-obj-y                          += dev-smartcard-reader.o

I'm curious why you don't use something like:

common-obj-$(CONFIG_USB_SMARTCARD)

Do we want to be able to configure individual elements as modules?
Or is the intent to force as module things that are marked as 'y'?

> -common-obj-$(CONFIG_SMARTCARD)        += smartcard.mo
> +ifeq ($(CONFIG_SMARTCARD),y)
> +common-obj-m                          += smartcard.mo
>  smartcard.mo-objs := ccid-card-passthru.o ccid-card-emulated.o
>  smartcard.mo-cflags := $(SMARTCARD_CFLAGS)
>  smartcard.mo-libs := $(SMARTCARD_LIBS)
>  endif
> +endif
>
>  ifeq ($(CONFIG_POSIX),y)
>  common-obj-$(CONFIG_USB_STORAGE_MTP)  += dev-mtp.o


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 06/10] usb: build usb-redir as module

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Drops libusbredirparser.so dependency from core qemu.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  util/module.c        | 1 +
>  hw/usb/Makefile.objs | 9 ++++++---
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/util/module.c b/util/module.c
> index 89da9a3cce05..e3226165e91c 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -263,6 +263,7 @@ static struct {
>  } const qom_modules[] = {
>      { "ccid-card-passthru",    "hw-", "usb-smartcard"         },
>      { "ccid-card-emulated",    "hw-", "usb-smartcard"         },
> +    { "usb-redir",             "hw-", "usb-redirect"          },
>  };
>
>  static bool module_loaded_qom_all;
> diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs
> index 3c5b3d4fadd3..e342ff59fab0 100644
> --- a/hw/usb/Makefile.objs
> +++ b/hw/usb/Makefile.objs
> @@ -43,9 +43,12 @@ endif
>
>  # usb redirection
>  ifeq ($(CONFIG_USB),y)
> -common-obj-$(CONFIG_USB_REDIR) += redirect.o quirks.o
> -redirect.o-cflags = $(USB_REDIR_CFLAGS)
> -redirect.o-libs = $(USB_REDIR_LIBS)
> +ifeq ($(CONFIG_USB_REDIR),y)
> +common-obj-m += redirect.mo
> +redirect.mo-objs = redirect.o quirks.o
> +redirect.mo-cflags = $(USB_REDIR_CFLAGS)
> +redirect.mo-libs = $(USB_REDIR_LIBS)
> +endif
>  endif
>
>  # usb pass-through

With the same questions as for earlier patches

Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 07/10] vga: build qxl as module

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> First step in making spice support modular.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  util/module.c            | 2 ++
>  hw/Makefile.objs         | 1 +
>  hw/display/Makefile.objs | 5 ++++-
>  3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/util/module.c b/util/module.c
> index e3226165e91c..7c76d2a84b94 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -264,6 +264,8 @@ static struct {
>      { "ccid-card-passthru",    "hw-", "usb-smartcard"         },
>      { "ccid-card-emulated",    "hw-", "usb-smartcard"         },
>      { "usb-redir",             "hw-", "usb-redirect"          },
> +    { "qxl-vga",               "hw-", "display-qxl"           },
> +    { "qxl",                   "hw-", "display-qxl"           },
>  };
>
>  static bool module_loaded_qom_all;
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index af8fd9a510ed..14b7ea4eb62e 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -43,5 +43,6 @@ devices-dirs-y += smbios/
>  endif
>
>  common-obj-y += $(devices-dirs-y)
> +common-obj-m += display/
>  common-obj-m += usb/
>  obj-y += $(devices-dirs-y)
> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> index 77a7d622bd2d..76b3571e4902 100644
> --- a/hw/display/Makefile.objs
> +++ b/hw/display/Makefile.objs
> @@ -44,7 +44,10 @@ common-obj-$(CONFIG_ARTIST) += artist.o
>
>  obj-$(CONFIG_VGA) += vga.o
>
> -common-obj-$(CONFIG_QXL) += qxl.o qxl-logger.o qxl-render.o
> +ifeq ($(CONFIG_QXL),y)
> +common-obj-m += qxl.mo
> +qxl.mo-objs = qxl.o qxl-logger.o qxl-render.o
> +endif
>
>  obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
>  obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o

Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>

--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 09/10] vga: build virtio-gpu as module

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Drops libvirglrenderer.so dependency from core qemu.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  util/module.c            |  6 ++++++
>  hw/display/Makefile.objs | 23 +++++++++++++----------
>  2 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/util/module.c b/util/module.c
> index 7c76d2a84b94..a74214eac052 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -266,6 +266,12 @@ static struct {
>      { "usb-redir",             "hw-", "usb-redirect"          },
>      { "qxl-vga",               "hw-", "display-qxl"           },
>      { "qxl",                   "hw-", "display-qxl"           },
> +    { "virtio-gpu-device",     "hw-", "display-virtio-gpu"    },
> +    { "virtio-gpu-pci",        "hw-", "display-virtio-gpu"    },
> +    { "virtio-vga",            "hw-", "display-virtio-gpu"    },
> +    { "vhost-user-gpu-device", "hw-", "display-virtio-gpu"    },
> +    { "vhost-user-gpu-pci",    "hw-", "display-virtio-gpu"    },
> +    { "vhost-user-vga",        "hw-", "display-virtio-gpu"    },
>  };
>
>  static bool module_loaded_qom_all;
> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
> index d619594ad4d3..e907f3182b0c 100644
> --- a/hw/display/Makefile.objs
> +++ b/hw/display/Makefile.objs
> @@ -49,16 +49,19 @@ common-obj-m += qxl.mo
>  qxl.mo-objs = qxl.o qxl-logger.o qxl-render.o
>  endif
>
> -common-obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
> -common-obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
> -common-obj-$(call land,$(CONFIG_VIRTIO_GPU),$(CONFIG_VIRTIO_PCI)) += virtio-gpu-pci.o
> -common-obj-$(call land,$(CONFIG_VHOST_USER_GPU),$(CONFIG_VIRTIO_PCI)) += vhost-user-gpu-pci.o
> -common-obj-$(CONFIG_VIRTIO_VGA) += virtio-vga.o
> -common-obj-$(CONFIG_VHOST_USER_VGA) += vhost-user-vga.o
> -virtio-gpu.o-cflags := $(VIRGL_CFLAGS)
> -virtio-gpu.o-libs += $(VIRGL_LIBS)
> -virtio-gpu-3d.o-cflags := $(VIRGL_CFLAGS)
> -virtio-gpu-3d.o-libs += $(VIRGL_LIBS)
> +ifeq ($(CONFIG_VIRTIO_GPU),y)
> +common-obj-m += virtio-gpu.mo
> +virtio-gpu-obj-$(CONFIG_VIRTIO_GPU) += virtio-gpu-base.o virtio-gpu.o virtio-gpu-3d.o
> +virtio-gpu-obj-$(CONFIG_VHOST_USER_GPU) += vhost-user-gpu.o
> +virtio-gpu-obj-$(call land,$(CONFIG_VIRTIO_GPU),$(CONFIG_VIRTIO_PCI)) += virtio-gpu-pci.o
> +virtio-gpu-obj-$(call land,$(CONFIG_VHOST_USER_GPU),$(CONFIG_VIRTIO_PCI)) += vhost-user-gpu-pci.o
> +virtio-gpu-obj-$(CONFIG_VIRTIO_VGA) += virtio-vga.o
> +virtio-gpu-obj-$(CONFIG_VHOST_USER_VGA) += vhost-user-vga.o
> +virtio-gpu.mo-objs := $(virtio-gpu-obj-y)
> +virtio-gpu.mo-cflags := $(VIRGL_CFLAGS)
> +virtio-gpu.mo-libs := $(VIRGL_LIBS)
> +endif
> +
>  common-obj-$(CONFIG_DPCD) += dpcd.o
>  common-obj-$(CONFIG_XLNX_ZYNQMP_ARM) += xlnx_dp.o

Reviewed-by: Christophe de Dinechin <dinechin@redhat.com>

--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 10/10] chardev: enable modules, use for braille

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Removes brlapi library dependency from core qemu.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  Makefile.objs         | 1 +
>  chardev/char.c        | 2 +-
>  util/module.c         | 1 +
>  chardev/Makefile.objs | 5 ++++-
>  4 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index ca555ede0710..2dfcd19713f8 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -71,6 +71,7 @@ common-obj-$(CONFIG_TPM) += tpm.o
>
>  common-obj-y += backends/
>  common-obj-y += chardev/
> +common-obj-m += chardev/
>
>  common-obj-$(CONFIG_SECCOMP) += qemu-seccomp.o
>  qemu-seccomp.o-cflags := $(SECCOMP_CFLAGS)
> diff --git a/chardev/char.c b/chardev/char.c
> index e3051295ac37..df697f3ce9e0 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -527,7 +527,7 @@ static const ChardevClass *char_get_class(const char *driver, Error **errp)
>      const ChardevClass *cc;
>      char *typename = g_strdup_printf("chardev-%s", driver);
>
> -    oc = object_class_by_name(typename);
> +    oc = module_object_class_by_name(typename);
>      g_free(typename);
>
>      if (!object_class_dynamic_cast(oc, TYPE_CHARDEV)) {
> diff --git a/util/module.c b/util/module.c
> index a74214eac052..32b0547b8266 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -272,6 +272,7 @@ static struct {
>      { "vhost-user-gpu-device", "hw-", "display-virtio-gpu"    },
>      { "vhost-user-gpu-pci",    "hw-", "display-virtio-gpu"    },
>      { "vhost-user-vga",        "hw-", "display-virtio-gpu"    },
> +    { "chardev-braille",       "chardev-", "baum"             },
>  };
>
>  static bool module_loaded_qom_all;
> diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
> index d68e1347f9af..3a58c9d329d6 100644
> --- a/chardev/Makefile.objs
> +++ b/chardev/Makefile.objs
> @@ -18,8 +18,11 @@ chardev-obj-$(CONFIG_WIN32) += char-win.o
>  chardev-obj-$(CONFIG_WIN32) += char-win-stdio.o
>
>  common-obj-y += msmouse.o wctablet.o testdev.o
> -common-obj-$(CONFIG_BRLAPI) += baum.o
> +
> +ifeq ($(CONFIG_BRLAPI),y)
> +common-obj-m += baum.o

Shouldn't that be a .mo?

>  baum.o-cflags := $(SDL_CFLAGS)
>  baum.o-libs := $(BRLAPI_LIBS)
> +endif
>
>  common-obj-$(CONFIG_SPICE) += spice.o


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 01/10] module: qom module support

[Gerd Hoffmann]

> > +/*
> > + * Building devices and other qom objects modular is mostly useful in
> > + * case they have dependencies to external shared libraries, so we can
> > + * cut down the core qemu library dependencies.  Which is the case for
> > + * only a very few devices & objects.
> > + *
> > + * So with the expectation that this will be rather the exception than
> > + * to rule and the list will not gain that many entries go with a
> 
> Nit: Add a comma after "entries"
> 
> I would also indicate that this list needs to be sorted by prefix/module

Done (incremental patch, series is merged already).

> > +    const char *type;
> > +    const char *prefix;
> > +    const char *module;
> 
> Because of the sorting rule, it is more intuitive to put the module first
> and the type last, for cases where you have
> 
>     mod1 prefix1 type1
>     mod1 prefix1 type2
>     mod1 prefix1 type3
>     mod2 prefix2 type1
> 
> Visually, I expect the constants to come first.

I see it more as a object-type -> module lookup table, thats why the
type comes first ...

take care,
  Gerd

Re: [PATCH v5 02/10] object: qom module support

[Gerd Hoffmann]

  Hi,

> > +ObjectClass *module_object_class_by_name(const char *typename)
> > +{
> > +    ObjectClass *oc;
> > +
> > +    oc = object_class_by_name(typename);
> > +#ifdef CONFIG_MODULES
> > +    if (!oc) {
> > +        module_load_qom_one(typename);
> > +        oc = object_class_by_name(typename);
> > +    }
> > +#endif
> 
> I'm wondering if there is any reason to only trigger the module load when
> you don't find the object class. You could simply call module_load_qom_one
> under #ifdef CONFIG_MODULES.
> 
> Performance wise, I don't think this makes much of a difference, and it
> simplifies the logical flow IMO.

I expect the common case is that the object class is found and there is
rarely a need to actually load a module.

take care,
  Gerd

Re: [PATCH v5 03/10] qdev: device module support

[Gerd Hoffmann]

  Hi,

> >  DeviceState *qdev_new(const char *name)
> >  {
> > +    if (!object_class_by_name(name)) {
> > +        module_load_qom_one(name);
> > +    }
> 
> Curious why you don't you call module_object_class_by_name here?

Because object_new() wants a name not an ObjectClass ...

> >      return DEVICE(object_new(name));
> >  }

take care,
  Gerd

Re: [PATCH v5 04/10] build: fix device module builds

[Gerd Hoffmann]

  Hi,

> > +# filling obj-y.  Clear it.  Fixes suspious dependency errors when
> 
> Typo: suspicious

Fixed.

thanks,
  Gerd

Re: [PATCH v5 05/10] ccid: build smartcard as module

[Gerd Hoffmann]

> >  ifeq ($(CONFIG_USB_SMARTCARD),y)
> >  common-obj-y                          += dev-smartcard-reader.o
> 
> I'm curious why you don't use something like:
> 
> common-obj-$(CONFIG_USB_SMARTCARD)
> 
> Do we want to be able to configure individual elements as modules?
> Or is the intent to force as module things that are marked as 'y'?

qemu kconfig miniconf handles bool only, not tristate.

So, yes, for now we can do only "all modules" or "no modules" but
nothing inbetween.

take care,
  Gerd

Re: [PATCH v5 02/10] object: qom module support

[Daniel P. Berrangé]

On Mon, Jul 20, 2020 at 04:20:55PM +0200, Christophe de Dinechin wrote:
> 
> On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> > Little helper function to load modules on demand.  In most cases adding
> > module loading support for devices and other objects is just
> > s/object_class_by_name/module_object_class_by_name/ in the right spot.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  include/qom/object.h | 12 ++++++++++++
> >  qom/object.c         | 14 ++++++++++++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index 94a61ccc3fe8..51f188137f1f 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -994,6 +994,18 @@ bool object_class_is_abstract(ObjectClass *klass);
> >   */
> >  ObjectClass *object_class_by_name(const char *typename);
> >
> > +/**
> > + * module_object_class_by_name:
> > + * @typename: The QOM typename to obtain the class for.
> > + *
> > + * For objects which might be provided by a module.  Behaves like
> > + * object_class_by_name, but additionally tries to load the module
> > + * needed in case the class is not available.
> > + *
> > + * Returns: The class for @typename or %NULL if not found.
> > + */
> > +ObjectClass *module_object_class_by_name(const char *typename);
> > +
> >  void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
> >                            const char *implements_type, bool include_abstract,
> >                            void *opaque);
> > diff --git a/qom/object.c b/qom/object.c
> > index 6ece96bc2bfc..34daaf1280f5 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -985,6 +985,20 @@ ObjectClass *object_class_by_name(const char *typename)
> >      return type->class;
> >  }
> >
> > +ObjectClass *module_object_class_by_name(const char *typename)
> > +{
> > +    ObjectClass *oc;
> > +
> > +    oc = object_class_by_name(typename);
> > +#ifdef CONFIG_MODULES
> > +    if (!oc) {
> > +        module_load_qom_one(typename);
> > +        oc = object_class_by_name(typename);
> > +    }
> > +#endif
> 
> I'm wondering if there is any reason to only trigger the module load when
> you don't find the object class. You could simply call module_load_qom_one
> under #ifdef CONFIG_MODULES.
> 
> Performance wise, I don't think this makes much of a difference, and it
> simplifies the logical flow IMO.

I wouldn't make that assumption about performance - module_load_qom_one()
does alot of work, and there are places where object / class creation is
performance critical. We might not happen to trigger them now with the
current set of modules, but we can easily do so in future.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v5 10/10] chardev: enable modules, use for braille

[Gerd Hoffmann]

> >  common-obj-y += msmouse.o wctablet.o testdev.o
> > -common-obj-$(CONFIG_BRLAPI) += baum.o
> > +
> > +ifeq ($(CONFIG_BRLAPI),y)
> > +common-obj-m += baum.o
> 
> Shouldn't that be a .mo?

This variant works too (but can only be used for modules built from a
single object file).

take care,
  Gerd

Re: [PATCH v5 03/10] qdev: device module support

[Christophe de Dinechin]

On 2020-07-21 at 16:27 CEST, Gerd Hoffmann wrote...
>   Hi,
>
>> >  DeviceState *qdev_new(const char *name)
>> >  {
>> > +    if (!object_class_by_name(name)) {
>> > +        module_load_qom_one(name);
>> > +    }
>>
>> Curious why you don't you call module_object_class_by_name here?
>
> Because object_new() wants a name not an ObjectClass ...

I'm talking about the two lines above.

    if (!object_class_by_name(name)) {
        module_load_qom_one(name);
    }

Thi9s code looks very similar to the code below:

    ObjectClass *module_object_class_by_name(const char *typename)
    {
        ObjectClass *oc;

        oc = object_class_by_name(typename);
    #ifdef CONFIG_MODULES
        if (!oc) {
            module_load_qom_one(typename);
            oc = object_class_by_name(typename);
        }
    #endif
        return oc;
    }

Both call module_load_qom_one and object_class_by_name using the name as
input, so I don't see the difference (except for the order).

Am I reading this wrong?

>
>> >      return DEVICE(object_new(name));
>> >  }
>
> take care,
>   Gerd


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 02/10] object: qom module support

[Christophe de Dinechin]

On 2020-06-24 at 15:10 CEST, Gerd Hoffmann wrote...
> Little helper function to load modules on demand.  In most cases adding
> module loading support for devices and other objects is just
> s/object_class_by_name/module_object_class_by_name/ in the right spot.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/qom/object.h | 12 ++++++++++++
>  qom/object.c         | 14 ++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 94a61ccc3fe8..51f188137f1f 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -994,6 +994,18 @@ bool object_class_is_abstract(ObjectClass *klass);
>   */
>  ObjectClass *object_class_by_name(const char *typename);
>
> +/**
> + * module_object_class_by_name:
> + * @typename: The QOM typename to obtain the class for.
> + *
> + * For objects which might be provided by a module.  Behaves like
> + * object_class_by_name, but additionally tries to load the module
> + * needed in case the class is not available.
> + *
> + * Returns: The class for @typename or %NULL if not found.
> + */
> +ObjectClass *module_object_class_by_name(const char *typename);
> +
>  void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
>                            const char *implements_type, bool include_abstract,
>                            void *opaque);
> diff --git a/qom/object.c b/qom/object.c
> index 6ece96bc2bfc..34daaf1280f5 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -985,6 +985,20 @@ ObjectClass *object_class_by_name(const char *typename)
>      return type->class;
>  }
>
> +ObjectClass *module_object_class_by_name(const char *typename)

Re-reading this, it occurred to me that typename is a keyword in C++.

I don't know if we want to make it a rule to not use C++ keywords just in
case we would some day compile that code with a C++ compiler.

> +{
> +    ObjectClass *oc;
> +
> +    oc = object_class_by_name(typename);
> +#ifdef CONFIG_MODULES
> +    if (!oc) {
> +        module_load_qom_one(typename);
> +        oc = object_class_by_name(typename);
> +    }
> +#endif
> +    return oc;
> +}
> +
>  ObjectClass *object_class_get_parent(ObjectClass *class)
>  {
>      TypeImpl *type = type_get_parent(class->type);


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 05/10] ccid: build smartcard as module

[Christophe de Dinechin]

On 2020-07-21 at 16:33 CEST, Gerd Hoffmann wrote...
>> >  ifeq ($(CONFIG_USB_SMARTCARD),y)
>> >  common-obj-y                          += dev-smartcard-reader.o
>>
>> I'm curious why you don't use something like:
>>
>> common-obj-$(CONFIG_USB_SMARTCARD)
>>
>> Do we want to be able to configure individual elements as modules?
>> Or is the intent to force as module things that are marked as 'y'?
>
> qemu kconfig miniconf handles bool only, not tristate.

Ah, I guess that right, I had a small "fix" for part of that in a recent
RFC, but you don't have it. OK.

>
> So, yes, for now we can do only "all modules" or "no modules" but
> nothing inbetween.
>
> take care,
>   Gerd


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 03/10] qdev: device module support

[Gerd Hoffmann]

On Wed, Jul 22, 2020 at 10:05:51AM +0200, Christophe de Dinechin wrote:
> 
> On 2020-07-21 at 16:27 CEST, Gerd Hoffmann wrote...
> >   Hi,
> >
> >> >  DeviceState *qdev_new(const char *name)
> >> >  {
> >> > +    if (!object_class_by_name(name)) {
> >> > +        module_load_qom_one(name);
> >> > +    }
> >>
> >> Curious why you don't you call module_object_class_by_name here?
> >
> > Because object_new() wants a name not an ObjectClass ...
> 
> I'm talking about the two lines above.
> 
>     if (!object_class_by_name(name)) {
>         module_load_qom_one(name);
>     }
> 
> Thi9s code looks very similar to the code below:
> 
>     ObjectClass *module_object_class_by_name(const char *typename)
>     {
>         ObjectClass *oc;
> 
>         oc = object_class_by_name(typename);
>     #ifdef CONFIG_MODULES
>         if (!oc) {
>             module_load_qom_one(typename);
>             oc = object_class_by_name(typename);
>         }
>     #endif
>         return oc;
>     }
> 
> Both call module_load_qom_one and object_class_by_name using the name as
> input, so I don't see the difference (except for the order).

Yes, calling module_object_class_by_name then throw away the result
would work too.  I don't like the idea to hide the module loading
though.

take care,
  Gerd

Re: [PATCH v5 02/10] object: qom module support

[Gerd Hoffmann]

> > +ObjectClass *module_object_class_by_name(const char *typename)
> 
> Re-reading this, it occurred to me that typename is a keyword in C++.
> 
> I don't know if we want to make it a rule to not use C++ keywords just in
> case we would some day compile that code with a C++ compiler.

If we want do this we have to touch lots of places anyway, using
typename as variable is pretty common in qom code ...

take care,
  Gerd

Re: [PATCH v5 03/10] qdev: device module support

[Christophe de Dinechin]

On 2020-07-22 at 13:05 CEST, Gerd Hoffmann wrote...
> On Wed, Jul 22, 2020 at 10:05:51AM +0200, Christophe de Dinechin wrote:
>>
>> On 2020-07-21 at 16:27 CEST, Gerd Hoffmann wrote...
>> >   Hi,
>> >
>> >> >  DeviceState *qdev_new(const char *name)
>> >> >  {
>> >> > +    if (!object_class_by_name(name)) {
>> >> > +        module_load_qom_one(name);
>> >> > +    }
>> >>
>> >> Curious why you don't you call module_object_class_by_name here?
>> >
>> > Because object_new() wants a name not an ObjectClass ...
>>
>> I'm talking about the two lines above.
>>
>>     if (!object_class_by_name(name)) {
>>         module_load_qom_one(name);
>>     }
>>
>> Thi9s code looks very similar to the code below:
>>
>>     ObjectClass *module_object_class_by_name(const char *typename)
>>     {
>>         ObjectClass *oc;
>>
>>         oc = object_class_by_name(typename);
>>     #ifdef CONFIG_MODULES
>>         if (!oc) {
>>             module_load_qom_one(typename);
>>             oc = object_class_by_name(typename);
>>         }
>>     #endif
>>         return oc;
>>     }
>>
>> Both call module_load_qom_one and object_class_by_name using the name as
>> input, so I don't see the difference (except for the order).
>
> Yes, calling module_object_class_by_name then throw away the result
> would work too.  I don't like the idea to hide the module loading
> though.

Why do you consider calling a function called "module_object_class_by_name"
as hiding the module loading?

More importantly, why is it better to have two ways to do the same thing
that are slightly different? The reason for the slight difference is really
unclear to me. If we later do a change to module_object_class_by_name, are
there cases where we won't need the same change in qdev_new?

>
> take care,
>   Gerd


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v5 01/10] module: qom module support

[Claudio Fontana]

Hello Gerd, Paolo,

this commit from 2020, together with others in the same series, contains a flaw that now comes back to bite.

From commit 28457744c345ca4ccb58c984c9552e9c5955a9de ("module: qom module support") :

On 6/24/20 15:10, Gerd Hoffmann wrote:
> Add support for qom types provided by modules.  For starters use a
> manually maintained list which maps qom type to module and prefix.
> 
> Two load functions are added:  One to load the module for a specific
> type, and one to load all modules (needed for object/device lists as
> printed by -- for example -- qemu -device help).
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  include/qemu/module.h |  2 ++
>  util/module.c         | 55 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 011ae1ae7605..9121a475c1b6 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -70,5 +70,7 @@ void register_dso_module_init(void (*fn)(void), module_init_type type);
>  
>  void module_call_init(module_init_type type);
>  bool module_load_one(const char *prefix, const char *lib_name);
> +void module_load_qom_one(const char *type);
> +void module_load_qom_all(void);


See the "+void" just below the "bool". We have now hidden all the return values of module_load_one,
so now we cannot figure out if a load was successful or not, and the fact that module_load_one returns bool is now pointless, at least for qom.

As a pattern, I think this is rarely a good idea.


>  
>  #endif
> diff --git a/util/module.c b/util/module.c
> index e48d9aacc05a..ee560a4b4269 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -245,3 +245,58 @@ bool module_load_one(const char *prefix, const char *lib_name)
>  #endif
>      return success;
>  }
> +
> +/*
> + * Building devices and other qom objects modular is mostly useful in
> + * case they have dependencies to external shared libraries, so we can
> + * cut down the core qemu library dependencies.  Which is the case for
> + * only a very few devices & objects.
> + *
> + * So with the expectation that this will be rather the exception than
> + * to rule and the list will not gain that many entries go with a
> + * simple manually maintained list for now.
> + */
> +static struct {
> +    const char *type;
> +    const char *prefix;
> +    const char *module;
> +} const qom_modules[] = {
> +};
> +
> +static bool module_loaded_qom_all;
> +
> +void module_load_qom_one(const char *type)
> +{
> +    int i;
> +
> +    if (module_loaded_qom_all) {
> +        return;
> +    }
> +    for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
> +        if (strcmp(qom_modules[i].type, type) == 0) {
> +            module_load_one(qom_modules[i].prefix,
> +                            qom_modules[i].module);
> +            return;

return value lost.

Should we not at least trace something, warn something?

Maybe we want to do it in module_load_one?


> +        }
> +    }
> +}
> +
> +void module_load_qom_all(void)
> +{
> +    int i;
> +
> +    if (module_loaded_qom_all) {
> +        return;
> +    }
> +    for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
> +        if (i > 0 && (strcmp(qom_modules[i - 1].module,
> +                             qom_modules[i].module) == 0 &&
> +                      strcmp(qom_modules[i - 1].prefix,
> +                             qom_modules[i].prefix) == 0)) {
> +            /* one module implementing multiple types -> load only once */
> +            continue;
> +        }
> +        module_load_one(qom_modules[i].prefix, qom_modules[i].module);

return value lost.

> +    }
> +    module_loaded_qom_all = true;
> +}

Pair this with the next commit,

commit 0f8198f1b2f3c33df2381c412ad8d8fd219b90b2 ("object: qom module support") :

which goes:

> commit 0f8198f1b2f3c33df2381c412ad8d8fd219b90b2
> Author: Gerd Hoffmann <kraxel@redhat.com>
> Date:   Wed Jun 24 15:10:37 2020 +0200
> 
>     object: qom module support
>     
>     Little helper function to load modules on demand.  In most cases adding
>     module loading support for devices and other objects is just
>     s/object_class_by_name/module_object_class_by_name/ in the right spot.
>     
>     Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>     Message-id: 20200624131045.14512-3-kraxel@redhat.com
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 94a61ccc3f..51f188137f 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -994,6 +994,18 @@ bool object_class_is_abstract(ObjectClass *klass);
>   */
>  ObjectClass *object_class_by_name(const char *typename);
>  
> +/**
> + * module_object_class_by_name:
> + * @typename: The QOM typename to obtain the class for.
> + *
> + * For objects which might be provided by a module.  Behaves like
> + * object_class_by_name, but additionally tries to load the module

Note the use of the word "tries" in the comment.

> + * needed in case the class is not available.
> + *
> + * Returns: The class for @typename or %NULL if not found.
> + */
> +ObjectClass *module_object_class_by_name(const char *typename);
> +
>  void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
>                            const char *implements_type, bool include_abstract,
>                            void *opaque);
> diff --git a/qom/object.c b/qom/object.c
> index 6ece96bc2b..34daaf1280 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -985,6 +985,20 @@ ObjectClass *object_class_by_name(const char *typename)
>      return type->class;
>  }
>  
> +ObjectClass *module_object_class_by_name(const char *typename)
> +{
> +    ObjectClass *oc;
> +
> +    oc = object_class_by_name(typename);
> +#ifdef CONFIG_MODULES
> +    if (!oc) {
> +        module_load_qom_one(typename);
> +        oc = object_class_by_name(typename);

now here we fail during object_class_by_name() with oc that is NULL. So I guess we can evince that module_load_qom_one failed from here.


> +    }
> +#endif
> +    return oc;
> +}
> +
>  ObjectClass *object_class_get_parent(ObjectClass *class)
>  {
>      TypeImpl *type = type_get_parent(class->type);


We should be able to tell if a load is successful or not I think, but before that,

I think we are currently inconsistent, and do not report enough in terms of tracing and errors to the user
as to the reason that a module load fails.

Either we add tracing to module_load_one, and make it so that module_load_one also returns void,
or we change module_load_qom_one to also return bool.

Note, qdev also makes use of this facility.

Thoughts?

Claudio

Re: [PATCH v5 01/10] module: qom module support

[Philippe Mathieu-Daudé via]

On 5/9/22 11:17, Claudio Fontana wrote:
> Hello Gerd, Paolo,
> 
> this commit from 2020, together with others in the same series, contains a flaw that now comes back to bite.
> 
>  From commit 28457744c345ca4ccb58c984c9552e9c5955a9de ("module: qom module support") :
> 
> On 6/24/20 15:10, Gerd Hoffmann wrote:
>> Add support for qom types provided by modules.  For starters use a
>> manually maintained list which maps qom type to module and prefix.
>>
>> Two load functions are added:  One to load the module for a specific
>> type, and one to load all modules (needed for object/device lists as
>> printed by -- for example -- qemu -device help).
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>   include/qemu/module.h |  2 ++
>>   util/module.c         | 55 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 57 insertions(+)
>>
>> diff --git a/include/qemu/module.h b/include/qemu/module.h
>> index 011ae1ae7605..9121a475c1b6 100644
>> --- a/include/qemu/module.h
>> +++ b/include/qemu/module.h
>> @@ -70,5 +70,7 @@ void register_dso_module_init(void (*fn)(void), module_init_type type);
>>   
>>   void module_call_init(module_init_type type);
>>   bool module_load_one(const char *prefix, const char *lib_name);
>> +void module_load_qom_one(const char *type);
>> +void module_load_qom_all(void);
> 
> 
> See the "+void" just below the "bool". We have now hidden all the return values of module_load_one,
> so now we cannot figure out if a load was successful or not, and the fact that module_load_one returns bool is now pointless, at least for qom.
> 
> As a pattern, I think this is rarely a good idea.
> 
> 
>>   
>>   #endif
>> diff --git a/util/module.c b/util/module.c
>> index e48d9aacc05a..ee560a4b4269 100644
>> --- a/util/module.c
>> +++ b/util/module.c
>> @@ -245,3 +245,58 @@ bool module_load_one(const char *prefix, const char *lib_name)
>>   #endif
>>       return success;
>>   }
>> +
>> +/*
>> + * Building devices and other qom objects modular is mostly useful in
>> + * case they have dependencies to external shared libraries, so we can
>> + * cut down the core qemu library dependencies.  Which is the case for
>> + * only a very few devices & objects.
>> + *
>> + * So with the expectation that this will be rather the exception than
>> + * to rule and the list will not gain that many entries go with a
>> + * simple manually maintained list for now.
>> + */
>> +static struct {
>> +    const char *type;
>> +    const char *prefix;
>> +    const char *module;
>> +} const qom_modules[] = {
>> +};
>> +
>> +static bool module_loaded_qom_all;
>> +
>> +void module_load_qom_one(const char *type)
>> +{
>> +    int i;
>> +
>> +    if (module_loaded_qom_all) {
>> +        return;
>> +    }
>> +    for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
>> +        if (strcmp(qom_modules[i].type, type) == 0) {
>> +            module_load_one(qom_modules[i].prefix,
>> +                            qom_modules[i].module);
>> +            return;
> 
> return value lost.
> 
> Should we not at least trace something, warn something?
> 
> Maybe we want to do it in module_load_one?
> 
> 
>> +        }
>> +    }
>> +}
>> +
>> +void module_load_qom_all(void)
>> +{
>> +    int i;
>> +
>> +    if (module_loaded_qom_all) {
>> +        return;
>> +    }
>> +    for (i = 0; i < ARRAY_SIZE(qom_modules); i++) {
>> +        if (i > 0 && (strcmp(qom_modules[i - 1].module,
>> +                             qom_modules[i].module) == 0 &&
>> +                      strcmp(qom_modules[i - 1].prefix,
>> +                             qom_modules[i].prefix) == 0)) {
>> +            /* one module implementing multiple types -> load only once */
>> +            continue;
>> +        }
>> +        module_load_one(qom_modules[i].prefix, qom_modules[i].module);
> 
> return value lost.
> 
>> +    }
>> +    module_loaded_qom_all = true;
>> +}
> 
> Pair this with the next commit,
> 
> commit 0f8198f1b2f3c33df2381c412ad8d8fd219b90b2 ("object: qom module support") :
> 
> which goes:
> 
>> commit 0f8198f1b2f3c33df2381c412ad8d8fd219b90b2
>> Author: Gerd Hoffmann <kraxel@redhat.com>
>> Date:   Wed Jun 24 15:10:37 2020 +0200
>>
>>      object: qom module support
>>      
>>      Little helper function to load modules on demand.  In most cases adding
>>      module loading support for devices and other objects is just
>>      s/object_class_by_name/module_object_class_by_name/ in the right spot.
>>      
>>      Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>      Message-id: 20200624131045.14512-3-kraxel@redhat.com
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 94a61ccc3f..51f188137f 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -994,6 +994,18 @@ bool object_class_is_abstract(ObjectClass *klass);
>>    */
>>   ObjectClass *object_class_by_name(const char *typename);
>>   
>> +/**
>> + * module_object_class_by_name:
>> + * @typename: The QOM typename to obtain the class for.
>> + *
>> + * For objects which might be provided by a module.  Behaves like
>> + * object_class_by_name, but additionally tries to load the module
> 
> Note the use of the word "tries" in the comment.
> 
>> + * needed in case the class is not available.
>> + *
>> + * Returns: The class for @typename or %NULL if not found.
>> + */
>> +ObjectClass *module_object_class_by_name(const char *typename);
>> +
>>   void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
>>                             const char *implements_type, bool include_abstract,
>>                             void *opaque);
>> diff --git a/qom/object.c b/qom/object.c
>> index 6ece96bc2b..34daaf1280 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -985,6 +985,20 @@ ObjectClass *object_class_by_name(const char *typename)
>>       return type->class;
>>   }
>>   
>> +ObjectClass *module_object_class_by_name(const char *typename)
>> +{
>> +    ObjectClass *oc;
>> +
>> +    oc = object_class_by_name(typename);
>> +#ifdef CONFIG_MODULES
>> +    if (!oc) {
>> +        module_load_qom_one(typename);
>> +        oc = object_class_by_name(typename);
> 
> now here we fail during object_class_by_name() with oc that is NULL. So I guess we can evince that module_load_qom_one failed from here.
> 
> 
>> +    }
>> +#endif
>> +    return oc;
>> +}
>> +
>>   ObjectClass *object_class_get_parent(ObjectClass *class)
>>   {
>>       TypeImpl *type = type_get_parent(class->type);
> 
> 
> We should be able to tell if a load is successful or not I think, but before that,
> 
> I think we are currently inconsistent, and do not report enough in terms of tracing and errors to the user
> as to the reason that a module load fails.
> 
> Either we add tracing to module_load_one, and make it so that module_load_one also returns void,
> or we change module_load_qom_one to also return bool.
> 
> Note, qdev also makes use of this facility.
> 
> Thoughts?

You are correct, module_load_qom_*() should return a boolean and ideally 
take an Error* parameter.