[PATCH v7 0/5] improve error handling for module load

[PATCH v7 0/5] improve error handling for module load

[Claudio Fontana]

CHANGELOG:

v6 -> v7:

* changed instances of abort() to exit(1), for the CONFIG_MODULES case (Philippe).

* dmg: do not use a separate local error, use the existing errp (Kevin)

* block: do not use a separate local error, use the existing errp for
  bdrv_find_protocol (Markus)

v5 -> v6:

* added a patch by Kevin to handle the dmg warning about missing
  decompression submodules. (Kevin)

* added more verbose comments about all the affected callers of module_load
  and module_load_qom (Markus)

(OPEN ISSUE): change abort() to exit() when type not present even after loading module?

(Philippe)

v4 -> v5:

* added a patch to rename module_load_one and friends to module_load

* qdev_new: just reuse module_object_class_by_name, to avoid duplicating code

* changed return value of module_load to an int:
  -1 error (Error **errp set).
   0 module or dependencies not installed,
   1 loaded
   2 already loaded (or built-in)

   Adapted all callers.

* module_load: fixed some pre-existing memory leaks, used an out: label
  to do the cleanup.

v3 -> v4: (Richard)

* module_object_class_by_name: return NULL immediately on load error.
* audio_driver_lookup: same.
* bdrv_find_format: same.

* dmg_open: handle optional compression submodules better: f.e.,
  if "dmg-bz2" is not present, continue but offer a warning.
  If "dmg-bz2" load fails with error, error out and return.

* module_load_dso: add newline to error_append_hint.

v2 -> v3:

* take the file existence check outside of module_load_file,
  rename module_load_file to module_load_dso, will be called only on
  an existing file. This will simplify the return value. (Richard)

* move exported function documentation into header files (Richard)

v1 -> v2:

* do not treat the display help text any differently and do report
  module load _errors_. If the module does not exist (ENOENT, ENOTDIR),
  no error will be produced.

DESCRIPTION:

while investigating a permission issue in accel, where accel-tcg-x86_64.so
was not accessible, I noticed that no errors were produced regarding the
module load failure.

This series attempts to improve module_load_one and module_load_qom_one
to handle the error cases better and produce some errors.

Patch 1 is already reviewed and is about removing an unused existing
argument "mayfail" from the call stack.

Patch 2 is the real meat, and that one I would say is RFC.
Will follow up with comments on the specific questions I have.

Patch 3 finally adds a simple check in accel/, aborting if a module
is not found, but relying on the existing error report from
module_load_qom_one.

Claudio Fontana (4):
  module: removed unused function argument "mayfail"
  module: rename module_load_one to module_load
  module: add Error arguments to module_load and module_load_qom
  accel: abort if we fail to load the accelerator plugin

Kevin Wolf (1):
  dmg: warn when opening dmg images containing blocks of unknown type

 accel/accel-softmmu.c |   8 +-
 audio/audio.c         |  16 ++--
 block.c               |  20 +++-
 block/dmg.c           |  33 ++++++-
 hw/core/qdev.c        |  17 +++-
 include/qemu/module.h |  37 +++++++-
 qom/object.c          |  18 +++-
 softmmu/qtest.c       |   8 +-
 ui/console.c          |  18 +++-
 util/module.c         | 213 +++++++++++++++++++++++-------------------
 10 files changed, 261 insertions(+), 127 deletions(-)

-- 
2.26.2

[PATCH v7 1/5] module: removed unused function argument "mayfail"

[Claudio Fontana]

mayfail is always passed as false for every invocation throughout the program.
It controls whether to printf or not to printf an error on
g_module_open failure.

Remove this unused argument.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qemu/module.h |  8 ++++----
 softmmu/qtest.c       |  2 +-
 util/module.c         | 20 +++++++++-----------
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/include/qemu/module.h b/include/qemu/module.h
index bd73607104..8c012bbe03 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -61,15 +61,15 @@ typedef enum {
 #define fuzz_target_init(function) module_init(function, \
                                                MODULE_INIT_FUZZ_TARGET)
 #define migration_init(function) module_init(function, MODULE_INIT_MIGRATION)
-#define block_module_load_one(lib) module_load_one("block-", lib, false)
-#define ui_module_load_one(lib) module_load_one("ui-", lib, false)
-#define audio_module_load_one(lib) module_load_one("audio-", lib, false)
+#define block_module_load_one(lib) module_load_one("block-", lib)
+#define ui_module_load_one(lib) module_load_one("ui-", lib)
+#define audio_module_load_one(lib) module_load_one("audio-", 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);
-bool module_load_one(const char *prefix, const char *lib_name, bool mayfail);
+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);
 void module_allow_arch(const char *arch);
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index f8acef2628..76eb7bac56 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -756,7 +756,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         g_assert(words[1] && words[2]);
 
         qtest_send_prefix(chr);
-        if (module_load_one(words[1], words[2], false)) {
+        if (module_load_one(words[1], words[2])) {
             qtest_sendf(chr, "OK\n");
         } else {
             qtest_sendf(chr, "FAIL\n");
diff --git a/util/module.c b/util/module.c
index 8ddb0e18f5..8563edd626 100644
--- a/util/module.c
+++ b/util/module.c
@@ -144,7 +144,7 @@ static bool module_check_arch(const QemuModinfo *modinfo)
     return true;
 }
 
-static int module_load_file(const char *fname, bool mayfail, bool export_symbols)
+static int module_load_file(const char *fname, bool export_symbols)
 {
     GModule *g_module;
     void (*sym)(void);
@@ -172,10 +172,8 @@ static int module_load_file(const char *fname, bool mayfail, bool export_symbols
     }
     g_module = g_module_open(fname, flags);
     if (!g_module) {
-        if (!mayfail) {
-            fprintf(stderr, "Failed to open module: %s\n",
-                    g_module_error());
-        }
+        fprintf(stderr, "Failed to open module: %s\n",
+                g_module_error());
         ret = -EINVAL;
         goto out;
     }
@@ -208,7 +206,7 @@ out:
 }
 #endif
 
-bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
+bool module_load_one(const char *prefix, const char *lib_name)
 {
     bool success = false;
 
@@ -256,7 +254,7 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
             if (strcmp(modinfo->name, module_name) == 0) {
                 /* we depend on other module(s) */
                 for (sl = modinfo->deps; *sl != NULL; sl++) {
-                    module_load_one("", *sl, false);
+                    module_load_one("", *sl);
                 }
             } else {
                 for (sl = modinfo->deps; *sl != NULL; sl++) {
@@ -287,7 +285,7 @@ bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
     for (i = 0; i < n_dirs; i++) {
         fname = g_strdup_printf("%s/%s%s",
                 dirs[i], module_name, CONFIG_HOST_DSOSUF);
-        ret = module_load_file(fname, mayfail, export_symbols);
+        ret = module_load_file(fname, export_symbols);
         g_free(fname);
         fname = NULL;
         /* Try loading until loaded a module file */
@@ -333,7 +331,7 @@ void module_load_qom_one(const char *type)
         }
         for (sl = modinfo->objs; *sl != NULL; sl++) {
             if (strcmp(type, *sl) == 0) {
-                module_load_one("", modinfo->name, false);
+                module_load_one("", modinfo->name);
             }
         }
     }
@@ -354,7 +352,7 @@ void module_load_qom_all(void)
         if (!module_check_arch(modinfo)) {
             continue;
         }
-        module_load_one("", modinfo->name, false);
+        module_load_one("", modinfo->name);
     }
     module_loaded_qom_all = true;
 }
@@ -370,7 +368,7 @@ void qemu_load_module_for_opts(const char *group)
         }
         for (sl = modinfo->opts; *sl != NULL; sl++) {
             if (strcmp(group, *sl) == 0) {
-                module_load_one("", modinfo->name, false);
+                module_load_one("", modinfo->name);
             }
         }
     }
-- 
2.26.2

[PATCH v7 2/5] module: rename module_load_one to module_load

[Claudio Fontana]

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 audio/audio.c         |  2 +-
 block.c               |  4 ++--
 block/dmg.c           |  4 ++--
 hw/core/qdev.c        |  2 +-
 include/qemu/module.h | 10 +++++-----
 qom/object.c          |  4 ++--
 softmmu/qtest.c       |  2 +-
 ui/console.c          |  6 +++---
 util/module.c         | 14 +++++++-------
 9 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 76b8735b44..0a682336a0 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -79,7 +79,7 @@ audio_driver *audio_driver_lookup(const char *name)
         }
     }
 
-    audio_module_load_one(name);
+    audio_module_load(name);
     QLIST_FOREACH(d, &audio_drivers, next) {
         if (strcmp(name, d->name) == 0) {
             return d;
diff --git a/block.c b/block.c
index bc85f46eed..72c7f6d47d 100644
--- a/block.c
+++ b/block.c
@@ -464,7 +464,7 @@ BlockDriver *bdrv_find_format(const char *format_name)
     /* The driver isn't registered, maybe we need to load a module */
     for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
         if (!strcmp(block_driver_modules[i].format_name, format_name)) {
-            block_module_load_one(block_driver_modules[i].library_name);
+            block_module_load(block_driver_modules[i].library_name);
             break;
         }
     }
@@ -976,7 +976,7 @@ BlockDriver *bdrv_find_protocol(const char *filename,
     for (i = 0; i < (int)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);
+            block_module_load(block_driver_modules[i].library_name);
             break;
         }
     }
diff --git a/block/dmg.c b/block/dmg.c
index 98db18d82a..007b8d9996 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -446,8 +446,8 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
         return -EINVAL;
     }
 
-    block_module_load_one("dmg-bz2");
-    block_module_load_one("dmg-lzfse");
+    block_module_load("dmg-bz2");
+    block_module_load("dmg-lzfse");
 
     s->n_chunks = 0;
     s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 0806d8fcaa..25dfc08468 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -148,7 +148,7 @@ bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
 DeviceState *qdev_new(const char *name)
 {
     if (!object_class_by_name(name)) {
-        module_load_qom_one(name);
+        module_load_qom(name);
     }
     return DEVICE(object_new(name));
 }
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 8c012bbe03..b7911ce791 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -61,16 +61,16 @@ typedef enum {
 #define fuzz_target_init(function) module_init(function, \
                                                MODULE_INIT_FUZZ_TARGET)
 #define migration_init(function) module_init(function, MODULE_INIT_MIGRATION)
-#define block_module_load_one(lib) module_load_one("block-", lib)
-#define ui_module_load_one(lib) module_load_one("ui-", lib)
-#define audio_module_load_one(lib) module_load_one("audio-", lib)
+#define block_module_load(lib) module_load("block-", lib)
+#define ui_module_load(lib) module_load("ui-", lib)
+#define audio_module_load(lib) module_load("audio-", 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);
-bool module_load_one(const char *prefix, const char *lib_name);
-void module_load_qom_one(const char *type);
+bool module_load(const char *prefix, const char *lib_name);
+void module_load_qom(const char *type);
 void module_load_qom_all(void);
 void module_allow_arch(const char *arch);
 
diff --git a/qom/object.c b/qom/object.c
index d34608558e..4f834f3bf6 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -526,7 +526,7 @@ void object_initialize(void *data, size_t size, const char *typename)
 
 #ifdef CONFIG_MODULES
     if (!type) {
-        module_load_qom_one(typename);
+        module_load_qom(typename);
         type = type_get_by_name(typename);
     }
 #endif
@@ -1033,7 +1033,7 @@ ObjectClass *module_object_class_by_name(const char *typename)
     oc = object_class_by_name(typename);
 #ifdef CONFIG_MODULES
     if (!oc) {
-        module_load_qom_one(typename);
+        module_load_qom(typename);
         oc = object_class_by_name(typename);
     }
 #endif
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index 76eb7bac56..fc5b733c63 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -756,7 +756,7 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         g_assert(words[1] && words[2]);
 
         qtest_send_prefix(chr);
-        if (module_load_one(words[1], words[2])) {
+        if (module_load(words[1], words[2])) {
             qtest_sendf(chr, "OK\n");
         } else {
             qtest_sendf(chr, "FAIL\n");
diff --git a/ui/console.c b/ui/console.c
index 765892f84f..4913c55684 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2632,7 +2632,7 @@ bool qemu_display_find_default(DisplayOptions *opts)
 
     for (i = 0; i < (int)ARRAY_SIZE(prio); i++) {
         if (dpys[prio[i]] == NULL) {
-            ui_module_load_one(DisplayType_str(prio[i]));
+            ui_module_load(DisplayType_str(prio[i]));
         }
         if (dpys[prio[i]] == NULL) {
             continue;
@@ -2650,7 +2650,7 @@ void qemu_display_early_init(DisplayOptions *opts)
         return;
     }
     if (dpys[opts->type] == NULL) {
-        ui_module_load_one(DisplayType_str(opts->type));
+        ui_module_load(DisplayType_str(opts->type));
     }
     if (dpys[opts->type] == NULL) {
         error_report("Display '%s' is not available.",
@@ -2680,7 +2680,7 @@ void qemu_display_help(void)
     printf("none\n");
     for (idx = DISPLAY_TYPE_NONE; idx < DISPLAY_TYPE__MAX; idx++) {
         if (!dpys[idx]) {
-            ui_module_load_one(DisplayType_str(idx));
+            ui_module_load(DisplayType_str(idx));
         }
         if (dpys[idx]) {
             printf("%s\n",  DisplayType_str(dpys[idx]->type));
diff --git a/util/module.c b/util/module.c
index 8563edd626..ad89cd50dc 100644
--- a/util/module.c
+++ b/util/module.c
@@ -206,7 +206,7 @@ out:
 }
 #endif
 
-bool module_load_one(const char *prefix, const char *lib_name)
+bool module_load(const char *prefix, const char *lib_name)
 {
     bool success = false;
 
@@ -254,7 +254,7 @@ bool module_load_one(const char *prefix, const char *lib_name)
             if (strcmp(modinfo->name, module_name) == 0) {
                 /* we depend on other module(s) */
                 for (sl = modinfo->deps; *sl != NULL; sl++) {
-                    module_load_one("", *sl);
+                    module_load("", *sl);
                 }
             } else {
                 for (sl = modinfo->deps; *sl != NULL; sl++) {
@@ -312,7 +312,7 @@ bool module_load_one(const char *prefix, const char *lib_name)
 
 static bool module_loaded_qom_all;
 
-void module_load_qom_one(const char *type)
+void module_load_qom(const char *type)
 {
     const QemuModinfo *modinfo;
     const char **sl;
@@ -331,7 +331,7 @@ void module_load_qom_one(const char *type)
         }
         for (sl = modinfo->objs; *sl != NULL; sl++) {
             if (strcmp(type, *sl) == 0) {
-                module_load_one("", modinfo->name);
+                module_load("", modinfo->name);
             }
         }
     }
@@ -352,7 +352,7 @@ void module_load_qom_all(void)
         if (!module_check_arch(modinfo)) {
             continue;
         }
-        module_load_one("", modinfo->name);
+        module_load("", modinfo->name);
     }
     module_loaded_qom_all = true;
 }
@@ -368,7 +368,7 @@ void qemu_load_module_for_opts(const char *group)
         }
         for (sl = modinfo->opts; *sl != NULL; sl++) {
             if (strcmp(group, *sl) == 0) {
-                module_load_one("", modinfo->name);
+                module_load("", modinfo->name);
             }
         }
     }
@@ -378,7 +378,7 @@ void qemu_load_module_for_opts(const char *group)
 
 void module_allow_arch(const char *arch) {}
 void qemu_load_module_for_opts(const char *group) {}
-void module_load_qom_one(const char *type) {}
+void module_load_qom(const char *type) {}
 void module_load_qom_all(void) {}
 
 #endif
-- 
2.26.2

[PATCH v7 3/5] module: add Error arguments to module_load and module_load_qom

[Claudio Fontana]

improve error handling during module load, by changing:

bool module_load(const char *prefix, const char *lib_name);
void module_load_qom(const char *type);

to:

int module_load(const char *prefix, const char *name, Error **errp);
int module_load_qom(const char *type, Error **errp);

where the return value is:

 -1 on module load error, and errp is set with the error
  0 on module or one of its dependencies are not installed
  1 on module load success
  2 on module load success (module already loaded or built-in)

module_load_qom_one has been introduced in:

commit 28457744c345 ("module: qom module support"), which built on top of
module_load_one, but discarded the bool return value. Restore it.

Adapt all callers to emit errors, or ignore them, or fail hard,
as appropriate in each context.

Some memory leaks also fixed as part of the module_load changes.

audio: when attempting to load an audio module, report module load errors.
block: when attempting to load a block module, report module load errors.
console: when attempting to load a display module, report module load errors.

qdev: when creating a new qdev Device object (DeviceState), report load errors.
      If a module cannot be loaded to create that device, now abort execution.

qom/object.c: when initializing a QOM object, or looking up class_by_name,
              report module load errors.

qtest: when processing the "module_load" qtest command, report errors
       in the load of the module.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 audio/audio.c         |  16 ++--
 block.c               |  20 +++-
 block/dmg.c           |  14 ++-
 hw/core/qdev.c        |  17 +++-
 include/qemu/module.h |  37 +++++++-
 qom/object.c          |  18 +++-
 softmmu/qtest.c       |   8 +-
 ui/console.c          |  18 +++-
 util/module.c         | 211 +++++++++++++++++++++++-------------------
 9 files changed, 235 insertions(+), 124 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 0a682336a0..ea51793843 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -72,20 +72,24 @@ void audio_driver_register(audio_driver *drv)
 audio_driver *audio_driver_lookup(const char *name)
 {
     struct audio_driver *d;
+    Error *local_err = NULL;
+    int rv;
 
     QLIST_FOREACH(d, &audio_drivers, next) {
         if (strcmp(name, d->name) == 0) {
             return d;
         }
     }
-
-    audio_module_load(name);
-    QLIST_FOREACH(d, &audio_drivers, next) {
-        if (strcmp(name, d->name) == 0) {
-            return d;
+    rv = audio_module_load(name, &local_err);
+    if (rv > 0) {
+        QLIST_FOREACH(d, &audio_drivers, next) {
+            if (strcmp(name, d->name) == 0) {
+                return d;
+            }
         }
+    } else if (rv < 0) {
+        error_report_err(local_err);
     }
-
     return NULL;
 }
 
diff --git a/block.c b/block.c
index 72c7f6d47d..7a94739aed 100644
--- a/block.c
+++ b/block.c
@@ -464,12 +464,18 @@ BlockDriver *bdrv_find_format(const char *format_name)
     /* The driver isn't registered, maybe we need to load a module */
     for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
         if (!strcmp(block_driver_modules[i].format_name, format_name)) {
-            block_module_load(block_driver_modules[i].library_name);
+            Error *local_err = NULL;
+            int rv = block_module_load(block_driver_modules[i].library_name,
+                                       &local_err);
+            if (rv > 0) {
+                return bdrv_do_find_format(format_name);
+            } else if (rv < 0) {
+                error_report_err(local_err);
+            }
             break;
         }
     }
-
-    return bdrv_do_find_format(format_name);
+    return NULL;
 }
 
 static int bdrv_format_is_whitelisted(const char *format_name, bool read_only)
@@ -976,12 +982,16 @@ BlockDriver *bdrv_find_protocol(const char *filename,
     for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
         if (block_driver_modules[i].protocol_name &&
             !strcmp(block_driver_modules[i].protocol_name, protocol)) {
-            block_module_load(block_driver_modules[i].library_name);
+            int rv = block_module_load(block_driver_modules[i].library_name, errp);
+            if (rv > 0) {
+                drv1 = bdrv_do_find_protocol(protocol);
+            } else if (rv < 0) {
+                return NULL;
+            }
             break;
         }
     }
 
-    drv1 = bdrv_do_find_protocol(protocol);
     if (!drv1) {
         error_setg(errp, "Unknown protocol '%s'", protocol);
     }
diff --git a/block/dmg.c b/block/dmg.c
index 007b8d9996..837f18aa20 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -445,9 +445,17 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
     if (!bs->file) {
         return -EINVAL;
     }
-
-    block_module_load("dmg-bz2");
-    block_module_load("dmg-lzfse");
+    /*
+     * NB: if uncompress submodules are absent,
+     * ie block_module_load return value == 0, the function pointers
+     * dmg_uncompress_bz2 and dmg_uncompress_lzfse will be NULL.
+     */
+    if (block_module_load("dmg-bz2", errp) < 0) {
+        return -EINVAL;
+    }
+    if (block_module_load("dmg-lzfse", errp) < 0) {
+        return -EINVAL;
+    }
 
     s->n_chunks = 0;
     s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 25dfc08468..0145501904 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -147,8 +147,21 @@ bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
 
 DeviceState *qdev_new(const char *name)
 {
-    if (!object_class_by_name(name)) {
-        module_load_qom(name);
+    ObjectClass *oc = object_class_by_name(name);
+#ifdef CONFIG_MODULES
+    if (!oc) {
+        int rv = module_load_qom(name, &error_fatal);
+        if (rv > 0) {
+            oc = object_class_by_name(name);
+        } else {
+            error_report("could not find a module for type '%s'", name);
+            exit(1);
+        }
+    }
+#endif
+    if (!oc) {
+        error_report("unknown type '%s'", name);
+        abort();
     }
     return DEVICE(object_new(name));
 }
diff --git a/include/qemu/module.h b/include/qemu/module.h
index b7911ce791..c37ce74b16 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -61,16 +61,43 @@ typedef enum {
 #define fuzz_target_init(function) module_init(function, \
                                                MODULE_INIT_FUZZ_TARGET)
 #define migration_init(function) module_init(function, MODULE_INIT_MIGRATION)
-#define block_module_load(lib) module_load("block-", lib)
-#define ui_module_load(lib) module_load("ui-", lib)
-#define audio_module_load(lib) module_load("audio-", lib)
+#define block_module_load(lib, errp) module_load("block-", lib, errp)
+#define ui_module_load(lib, errp) module_load("ui-", lib, errp)
+#define audio_module_load(lib, errp) module_load("audio-", lib, errp)
 
 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);
-bool module_load(const char *prefix, const char *lib_name);
-void module_load_qom(const char *type);
+
+/*
+ * module_load: attempt to load a module from a set of directories
+ *
+ * directories searched are:
+ * - getenv("QEMU_MODULE_DIR")
+ * - get_relocated_path(CONFIG_QEMU_MODDIR);
+ * - /var/run/qemu/${version_dir}
+ *
+ * prefix:         a subsystem prefix, or the empty string ("audio-", ..., "")
+ * name:           name of the module
+ * errp:           error to set in case the module is found, but load failed.
+ *
+ * Return value:   -1 on error (errp set if not NULL).
+ *                 0 if module or one of its dependencies are not installed,
+ *                 1 if the module is found and loaded,
+ *                 2 if the module is already loaded, or module is built-in.
+ */
+int module_load(const char *prefix, const char *name, Error **errp);
+
+/*
+ * module_load_qom: attempt to load a module to provide a QOM type
+ *
+ * type:           the type to be provided
+ * errp:           error to set.
+ *
+ * Return value:   as per module_load.
+ */
+int module_load_qom(const char *type, Error **errp);
 void module_load_qom_all(void);
 void module_allow_arch(const char *arch);
 
diff --git a/qom/object.c b/qom/object.c
index 4f834f3bf6..45da07980a 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -526,8 +526,13 @@ void object_initialize(void *data, size_t size, const char *typename)
 
 #ifdef CONFIG_MODULES
     if (!type) {
-        module_load_qom(typename);
-        type = type_get_by_name(typename);
+        int rv = module_load_qom(typename, &error_fatal);
+        if (rv > 0) {
+            type = type_get_by_name(typename);
+        } else {
+            error_report("missing object type '%s'", typename);
+            exit(1);
+        }
     }
 #endif
     if (!type) {
@@ -1033,8 +1038,13 @@ ObjectClass *module_object_class_by_name(const char *typename)
     oc = object_class_by_name(typename);
 #ifdef CONFIG_MODULES
     if (!oc) {
-        module_load_qom(typename);
-        oc = object_class_by_name(typename);
+        Error *local_err = NULL;
+        int rv = module_load_qom(typename, &local_err);
+        if (rv > 0) {
+            oc = object_class_by_name(typename);
+        } else if (rv < 0) {
+            error_report_err(local_err);
+        }
     }
 #endif
     return oc;
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index fc5b733c63..36e28609ff 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -753,12 +753,18 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
         qtest_sendf(chr, "OK %"PRIi64"\n",
                     (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
     } else if (strcmp(words[0], "module_load") == 0) {
+        Error *local_err = NULL;
+        int rv;
         g_assert(words[1] && words[2]);
 
         qtest_send_prefix(chr);
-        if (module_load(words[1], words[2])) {
+        rv = module_load(words[1], words[2], &local_err);
+        if (rv > 0) {
             qtest_sendf(chr, "OK\n");
         } else {
+            if (rv < 0) {
+                error_report_err(local_err);
+            }
             qtest_sendf(chr, "FAIL\n");
         }
     } else if (qtest_enabled() && strcmp(words[0], "clock_set") == 0) {
diff --git a/ui/console.c b/ui/console.c
index 4913c55684..4e53c3c71b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2632,7 +2632,11 @@ bool qemu_display_find_default(DisplayOptions *opts)
 
     for (i = 0; i < (int)ARRAY_SIZE(prio); i++) {
         if (dpys[prio[i]] == NULL) {
-            ui_module_load(DisplayType_str(prio[i]));
+            Error *local_err = NULL;
+            int rv = ui_module_load(DisplayType_str(prio[i]), &local_err);
+            if (rv < 0) {
+                error_report_err(local_err);
+            }
         }
         if (dpys[prio[i]] == NULL) {
             continue;
@@ -2650,7 +2654,11 @@ void qemu_display_early_init(DisplayOptions *opts)
         return;
     }
     if (dpys[opts->type] == NULL) {
-        ui_module_load(DisplayType_str(opts->type));
+        Error *local_err = NULL;
+        int rv = ui_module_load(DisplayType_str(opts->type), &local_err);
+        if (rv < 0) {
+            error_report_err(local_err);
+        }
     }
     if (dpys[opts->type] == NULL) {
         error_report("Display '%s' is not available.",
@@ -2680,7 +2688,11 @@ void qemu_display_help(void)
     printf("none\n");
     for (idx = DISPLAY_TYPE_NONE; idx < DISPLAY_TYPE__MAX; idx++) {
         if (!dpys[idx]) {
-            ui_module_load(DisplayType_str(idx));
+            Error *local_err = NULL;
+            int rv = ui_module_load(DisplayType_str(idx), &local_err);
+            if (rv < 0) {
+                error_report_err(local_err);
+            }
         }
         if (dpys[idx]) {
             printf("%s\n",  DisplayType_str(dpys[idx]->type));
diff --git a/util/module.c b/util/module.c
index ad89cd50dc..b67923a986 100644
--- a/util/module.c
+++ b/util/module.c
@@ -21,6 +21,7 @@
 #include "qemu/module.h"
 #include "qemu/cutils.h"
 #include "qemu/config-file.h"
+#include "qapi/error.h"
 #ifdef CONFIG_MODULE_UPGRADES
 #include "qemu-version.h"
 #endif
@@ -144,25 +145,22 @@ static bool module_check_arch(const QemuModinfo *modinfo)
     return true;
 }
 
-static int module_load_file(const char *fname, bool export_symbols)
+/*
+ * module_load_dso: attempt to load an existing dso file
+ *
+ * fname:          full pathname of the file to load
+ * export_symbols: if true, add the symbols to the global name space
+ * errp:           error to set.
+ *
+ * Return value:   true on success, false on error, and errp will be set.
+ */
+static bool module_load_dso(const char *fname, bool export_symbols,
+                            Error **errp)
 {
     GModule *g_module;
     void (*sym)(void);
-    const char *dsosuf = CONFIG_HOST_DSOSUF;
-    int len = strlen(fname);
-    int suf_len = strlen(dsosuf);
     ModuleEntry *e, *next;
-    int ret, flags;
-
-    if (len <= suf_len || strcmp(&fname[len - suf_len], dsosuf)) {
-        /* wrong suffix */
-        ret = -EINVAL;
-        goto out;
-    }
-    if (access(fname, F_OK)) {
-        ret = -ENOENT;
-        goto out;
-    }
+    int flags;
 
     assert(QTAILQ_EMPTY(&dso_init_list));
 
@@ -172,46 +170,38 @@ static int module_load_file(const char *fname, bool export_symbols)
     }
     g_module = g_module_open(fname, flags);
     if (!g_module) {
-        fprintf(stderr, "Failed to open module: %s\n",
-                g_module_error());
-        ret = -EINVAL;
-        goto out;
+        error_setg(errp, "failed to open module: %s", g_module_error());
+        return false;
     }
     if (!g_module_symbol(g_module, DSO_STAMP_FUN_STR, (gpointer *)&sym)) {
-        fprintf(stderr, "Failed to initialize module: %s\n",
-                fname);
-        /* Print some info if this is a QEMU module (but from different build),
-         * this will make debugging user problems easier. */
+        error_setg(errp, "failed to initialize module: %s", fname);
+        /*
+         * Print some info if this is a QEMU module (but from different build),
+         * this will make debugging user problems easier.
+         */
         if (g_module_symbol(g_module, "qemu_module_dummy", (gpointer *)&sym)) {
-            fprintf(stderr,
-                    "Note: only modules from the same build can be loaded.\n");
+            error_append_hint(errp,
+                "Only modules from the same build can be loaded.\n");
         }
         g_module_close(g_module);
-        ret = -EINVAL;
-    } else {
-        QTAILQ_FOREACH(e, &dso_init_list, node) {
-            e->init();
-            register_module_init(e->init, e->type);
-        }
-        ret = 0;
+        return false;
     }
 
+    QTAILQ_FOREACH(e, &dso_init_list, node) {
+        e->init();
+        register_module_init(e->init, e->type);
+    }
     trace_module_load_module(fname);
     QTAILQ_FOREACH_SAFE(e, &dso_init_list, node, next) {
         QTAILQ_REMOVE(&dso_init_list, e, node);
         g_free(e);
     }
-out:
-    return ret;
+    return true;
 }
-#endif
 
-bool module_load(const char *prefix, const char *lib_name)
+int module_load(const char *prefix, const char *name, Error **errp)
 {
-    bool success = false;
-
-#ifdef CONFIG_MODULES
-    char *fname = NULL;
+    int rv = -1;
 #ifdef CONFIG_MODULE_UPGRADES
     char *version_dir;
 #endif
@@ -219,54 +209,29 @@ bool module_load(const char *prefix, const char *lib_name)
     char *dirs[5];
     char *module_name;
     int i = 0, n_dirs = 0;
-    int ret;
     bool export_symbols = false;
     static GHashTable *loaded_modules;
     const QemuModinfo *modinfo;
     const char **sl;
 
     if (!g_module_supported()) {
-        fprintf(stderr, "Module is not supported by system.\n");
-        return false;
+        error_setg(errp, "%s", "this platform does not support GLib modules");
+        return -1;
     }
 
     if (!loaded_modules) {
         loaded_modules = g_hash_table_new(g_str_hash, g_str_equal);
     }
 
-    module_name = g_strdup_printf("%s%s", prefix, lib_name);
+    /* allocate all resources managed by the out: label here */
+    module_name = g_strdup_printf("%s%s", prefix, name);
 
     if (g_hash_table_contains(loaded_modules, module_name)) {
         g_free(module_name);
-        return true;
+        return 2; /* module already loaded */
     }
     g_hash_table_add(loaded_modules, module_name);
 
-    for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
-        if (modinfo->arch) {
-            if (strcmp(modinfo->name, module_name) == 0) {
-                if (!module_check_arch(modinfo)) {
-                    return false;
-                }
-            }
-        }
-        if (modinfo->deps) {
-            if (strcmp(modinfo->name, module_name) == 0) {
-                /* we depend on other module(s) */
-                for (sl = modinfo->deps; *sl != NULL; sl++) {
-                    module_load("", *sl);
-                }
-            } else {
-                for (sl = modinfo->deps; *sl != NULL; sl++) {
-                    if (strcmp(module_name, *sl) == 0) {
-                        /* another module depends on us */
-                        export_symbols = true;
-                    }
-                }
-            }
-        }
-    }
-
     search_dir = getenv("QEMU_MODULE_DIR");
     if (search_dir != NULL) {
         dirs[n_dirs++] = g_strdup_printf("%s", search_dir);
@@ -279,46 +244,87 @@ bool module_load(const char *prefix, const char *lib_name)
                              '_');
     dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
 #endif
-
     assert(n_dirs <= ARRAY_SIZE(dirs));
 
-    for (i = 0; i < n_dirs; i++) {
-        fname = g_strdup_printf("%s/%s%s",
-                dirs[i], module_name, CONFIG_HOST_DSOSUF);
-        ret = module_load_file(fname, export_symbols);
-        g_free(fname);
-        fname = NULL;
-        /* Try loading until loaded a module file */
-        if (!ret) {
-            success = true;
-            break;
+    /* end of resources managed by the out: label */
+
+    for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
+        if (modinfo->arch) {
+            if (strcmp(modinfo->name, module_name) == 0) {
+                if (!module_check_arch(modinfo)) {
+                    error_setg(errp, "module arch does not match: "
+                        "expected '%s', got '%s'", module_arch, modinfo->arch);
+                    goto out;
+                }
+            }
+        }
+        if (modinfo->deps) {
+            if (strcmp(modinfo->name, module_name) == 0) {
+                /* we depend on other module(s) */
+                for (sl = modinfo->deps; *sl != NULL; sl++) {
+                    int subrv = module_load("", *sl, errp);
+                    if (subrv <= 0) {
+                        rv = subrv;
+                        goto out;
+                    }
+                }
+            } else {
+                for (sl = modinfo->deps; *sl != NULL; sl++) {
+                    if (strcmp(module_name, *sl) == 0) {
+                        /* another module depends on us */
+                        export_symbols = true;
+                    }
+                }
+            }
         }
     }
 
-    if (!success) {
-        g_hash_table_remove(loaded_modules, module_name);
-        g_free(module_name);
+    for (i = 0; i < n_dirs; i++) {
+        char *fname = g_strdup_printf("%s/%s%s",
+                                      dirs[i], module_name, CONFIG_HOST_DSOSUF);
+        int ret = access(fname, F_OK);
+        if (ret != 0 && (errno == ENOENT || errno == ENOTDIR)) {
+            /*
+             * if we don't find the module in this dir, try the next one.
+             * If we don't find it in any dir, that can be fine too: user
+             * did not install the module. We will return 0 in this case
+             * with no error set.
+             */
+            g_free(fname);
+            continue;
+        } else if (ret != 0) {
+            /* most common is EACCES here */
+            error_setg_errno(errp, errno, "error trying to access %s", fname);
+        } else if (module_load_dso(fname, export_symbols, errp)) {
+            rv = 1; /* module successfully loaded */
+        }
+        g_free(fname);
+        goto out;
     }
+    rv = 0; /* module not found */
 
+out:
+    if (rv <= 0) {
+        g_hash_table_remove(loaded_modules, module_name);
+    }
+    g_free(module_name);
     for (i = 0; i < n_dirs; i++) {
         g_free(dirs[i]);
     }
-
-#endif
-    return success;
+    return rv;
 }
 
-#ifdef CONFIG_MODULES
-
 static bool module_loaded_qom_all;
 
-void module_load_qom(const char *type)
+int module_load_qom(const char *type, Error **errp)
 {
     const QemuModinfo *modinfo;
     const char **sl;
+    int rv = 0;
 
     if (!type) {
-        return;
+        error_setg(errp, "%s", "type is NULL");
+        return -1;
     }
 
     trace_module_lookup_object_type(type);
@@ -331,15 +337,24 @@ void module_load_qom(const char *type)
         }
         for (sl = modinfo->objs; *sl != NULL; sl++) {
             if (strcmp(type, *sl) == 0) {
-                module_load("", modinfo->name);
+                if (rv > 0) {
+                    error_setg(errp, "multiple modules providing '%s'", type);
+                    return -1;
+                }
+                rv = module_load("", modinfo->name, errp);
+                if (rv < 0) {
+                    return rv;
+                }
             }
         }
     }
+    return rv;
 }
 
 void module_load_qom_all(void)
 {
     const QemuModinfo *modinfo;
+    Error *local_err = NULL;
 
     if (module_loaded_qom_all) {
         return;
@@ -352,7 +367,9 @@ void module_load_qom_all(void)
         if (!module_check_arch(modinfo)) {
             continue;
         }
-        module_load("", modinfo->name);
+        if (module_load("", modinfo->name, &local_err) < 0) {
+            error_report_err(local_err);
+        }
     }
     module_loaded_qom_all = true;
 }
@@ -368,7 +385,10 @@ void qemu_load_module_for_opts(const char *group)
         }
         for (sl = modinfo->opts; *sl != NULL; sl++) {
             if (strcmp(group, *sl) == 0) {
-                module_load("", modinfo->name);
+                Error *local_err = NULL;
+                if (module_load("", modinfo->name, &local_err) < 0) {
+                    error_report_err(local_err);
+                }
             }
         }
     }
@@ -378,7 +398,8 @@ void qemu_load_module_for_opts(const char *group)
 
 void module_allow_arch(const char *arch) {}
 void qemu_load_module_for_opts(const char *group) {}
-void module_load_qom(const char *type) {}
+int module_load(const char *prefix, const char *name, Error **errp) { return 2; }
+int module_load_qom(const char *type, Error **errp) { return 2; }
 void module_load_qom_all(void) {}
 
 #endif
-- 
2.26.2

[PATCH v7 4/5] dmg: warn when opening dmg images containing blocks of unknown type

[Claudio Fontana]

From: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 block/dmg.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/block/dmg.c b/block/dmg.c
index 837f18aa20..96f8c2d14f 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -254,6 +254,25 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds,
     for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) {
         s->types[i] = buff_read_uint32(buffer, offset);
         if (!dmg_is_known_block_type(s->types[i])) {
+            switch (s->types[i]) {
+            case UDBZ:
+                warn_report_once("dmg-bzip2 module is missing, accessing bzip2 "
+                                 "compressed blocks will result in I/O errors");
+                break;
+            case ULFO:
+                warn_report_once("dmg-lzfse module is missing, accessing lzfse "
+                                 "compressed blocks will result in I/O errors");
+                break;
+            case UDCM:
+            case UDLE:
+                /* Comments and last entry can be ignored without problems */
+                break;
+            default:
+                warn_report_once("Image contains chunks of unknown type %x, "
+                                 "accessing them will result in I/O errors",
+                                 s->types[i]);
+                break;
+            }
             chunk_count--;
             i--;
             offset += 40;
-- 
2.26.2

[PATCH v7 5/5] accel: abort if we fail to load the accelerator plugin

[Claudio Fontana]

if QEMU is configured with modules enabled, it is possible that the
load of an accelerator module will fail.
Abort in this case, relying on module_object_class_by_name to report
the specific load error if any.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

[claudio: changed abort() to exit(1)]
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 accel/accel-softmmu.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
index 67276e4f52..f9cdafb148 100644
--- a/accel/accel-softmmu.c
+++ b/accel/accel-softmmu.c
@@ -66,6 +66,7 @@ void accel_init_ops_interfaces(AccelClass *ac)
 {
     const char *ac_name;
     char *ops_name;
+    ObjectClass *oc;
     AccelOpsClass *ops;
 
     ac_name = object_class_get_name(OBJECT_CLASS(ac));
@@ -73,8 +74,13 @@ void accel_init_ops_interfaces(AccelClass *ac)
 
     ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name);
     ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));
+    oc = module_object_class_by_name(ops_name);
+    if (!oc) {
+        error_report("fatal: could not load module for type '%s'", ops_name);
+        exit(1);
+    }
     g_free(ops_name);
-
+    ops = ACCEL_OPS_CLASS(oc);
     /*
      * all accelerators need to define ops, providing at least a mandatory
      * non-NULL create_vcpu_thread operation.
-- 
2.26.2

Re: [PATCH v7 4/5] dmg: warn when opening dmg images containing blocks of unknown type

[Markus Armbruster]

The patch looks like a useful improvement on its own.  But I wonder
whether users would appreciate a configuration knob to fail open right
away instead of risking I/O errors later.

Re: [PATCH v7 5/5] accel: abort if we fail to load the accelerator plugin

[Markus Armbruster]

Claudio Fontana <cfontana@suse.de> writes:

> if QEMU is configured with modules enabled, it is possible that the
> load of an accelerator module will fail.
> Abort in this case, relying on module_object_class_by_name to report
> the specific load error if any.
>
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> [claudio: changed abort() to exit(1)]
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  accel/accel-softmmu.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
> index 67276e4f52..f9cdafb148 100644
> --- a/accel/accel-softmmu.c
> +++ b/accel/accel-softmmu.c
> @@ -66,6 +66,7 @@ void accel_init_ops_interfaces(AccelClass *ac)
>  {
>      const char *ac_name;
>      char *ops_name;
> +    ObjectClass *oc;
>      AccelOpsClass *ops;
>  
>      ac_name = object_class_get_name(OBJECT_CLASS(ac));
> @@ -73,8 +74,13 @@ void accel_init_ops_interfaces(AccelClass *ac)
>  
>      ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name);
>      ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));
> +    oc = module_object_class_by_name(ops_name);
> +    if (!oc) {
> +        error_report("fatal: could not load module for type '%s'", ops_name);

I'm not sure the additional error message is worth it.

> +        exit(1);

Commit message claims we abort, but we don't, we terminate
unsuccessfully.  Easy enough to fix :)

> +    }
>      g_free(ops_name);
> -
> +    ops = ACCEL_OPS_CLASS(oc);
>      /*
>       * all accelerators need to define ops, providing at least a mandatory
>       * non-NULL create_vcpu_thread operation.

With the commit message corrected:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v7 3/5] module: add Error arguments to module_load and module_load_qom

[Markus Armbruster]

Claudio Fontana <cfontana@suse.de> writes:

> improve error handling during module load, by changing:
>
> bool module_load(const char *prefix, const char *lib_name);
> void module_load_qom(const char *type);
>
> to:
>
> int module_load(const char *prefix, const char *name, Error **errp);
> int module_load_qom(const char *type, Error **errp);
>
> where the return value is:
>
>  -1 on module load error, and errp is set with the error
>   0 on module or one of its dependencies are not installed
>   1 on module load success
>   2 on module load success (module already loaded or built-in)

Two changes, if I understand things correctly:

1. Convert to Error API from fprintf(stderr, ...)

2. Return a more useful value

Right?

Do you add any new errors here that weren't reported before?  Just
trying to calibrate my expectations before I dig into the actual patch.

> module_load_qom_one has been introduced in:
>
> commit 28457744c345 ("module: qom module support"), which built on top of
> module_load_one, but discarded the bool return value. Restore it.
>
> Adapt all callers to emit errors, or ignore them, or fail hard,
> as appropriate in each context.
>
> Some memory leaks also fixed as part of the module_load changes.

Where?

> audio: when attempting to load an audio module, report module load errors.
> block: when attempting to load a block module, report module load errors.
> console: when attempting to load a display module, report module load errors.
>
> qdev: when creating a new qdev Device object (DeviceState), report load errors.
>       If a module cannot be loaded to create that device, now abort execution.
>
> qom/object.c: when initializing a QOM object, or looking up class_by_name,
>               report module load errors.
>
> qtest: when processing the "module_load" qtest command, report errors
>        in the load of the module.

This looks like a list of behavioral changes.  Appreciated!  It's a bit
terse, though.  I might come back to this and suggest improvement.  But
first, I need to understand the patch.

>
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  audio/audio.c         |  16 ++--
>  block.c               |  20 +++-
>  block/dmg.c           |  14 ++-
>  hw/core/qdev.c        |  17 +++-
>  include/qemu/module.h |  37 +++++++-
>  qom/object.c          |  18 +++-
>  softmmu/qtest.c       |   8 +-
>  ui/console.c          |  18 +++-
>  util/module.c         | 211 +++++++++++++++++++++++-------------------
>  9 files changed, 235 insertions(+), 124 deletions(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 0a682336a0..ea51793843 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -72,20 +72,24 @@ void audio_driver_register(audio_driver *drv)
>  audio_driver *audio_driver_lookup(const char *name)
>  {
>      struct audio_driver *d;
> +    Error *local_err = NULL;
> +    int rv;
>  
>      QLIST_FOREACH(d, &audio_drivers, next) {
>          if (strcmp(name, d->name) == 0) {
>              return d;
>          }
>      }
> -
> -    audio_module_load(name);
> -    QLIST_FOREACH(d, &audio_drivers, next) {
> -        if (strcmp(name, d->name) == 0) {
> -            return d;
> +    rv = audio_module_load(name, &local_err);
> +    if (rv > 0) {
> +        QLIST_FOREACH(d, &audio_drivers, next) {
> +            if (strcmp(name, d->name) == 0) {
> +                return d;
> +            }
>          }
> +    } else if (rv < 0) {
> +        error_report_err(local_err);
>      }
> -
>      return NULL;
>  }
>  

Before: audio_module_load() reports to stderr, but the caller can't
know.  So, error or no error, search the driver registry for the one we
want.  Return it if found, else fail.

After: if audio_module_load() fails, report to stderr or current
monitor, and fail.  If it could find no module or loaded one, search the
driver registry.  Return it if found, else fail.

What if audio_module_load() fails, but a search for the driver succeeds?
Before the patch, we succeed.  Afterwards, we fail.  Can this happen?
Yes: module_load() starts with

       if (!g_module_supported()) {
           error_setg(errp, "%s", "this platform does not support GLib modules");
           return -1;
       }

Regression.

One way to fix: ensure module_load() cannot fail when the search will
succeed.

Another: search first, and module_load() only if not found, then search
again.

Now let's review your use of the Error API.  Error objects should be
passed up the call chain to the place that handles then.  The call
chains are:

* audio_init(), which is called by

  - AUD_register_card(), which is called by a bunch of device creation
    helpers called within machine initialization functions (I think),
    which are called from qemu_init() via qemu_init_board() and
    machine_run_board_init()

  - AUD_add_capture(), which is called by

    · wav_start_capture(), which is called by hmp_wavcapture on behalf
      of HMP command wavcapture

    · audio_add(), which is called by protocol_client_msg() in respone
      of a VNC_MSG_CLIENT_QEMU_AUDIO_ENABLE message from a VNC client (I
      think).  Since audio_add() does return anything, its callers
      remain oblivious of failure.

  - audio_init_audiodevs(), which is called by
    qemu_create_early_backends(), which is called by qemu_init()

* audio_help(), which is called by

  - audio_parse_option(), which is called by qemu_init()

  - qemu_init()

* audio_handle_legacy_opts()

  - audio_init(); see above

  - audio_legacy_help(), which is called by qemu_init()

For the call chains that end in qemu_init(), error_report_err() is okay.
Just beware of error cascades, i.e. one issue triggering multiple
reports.  That's bad UX.  The extra reporting should predate this patch,
which means it's no reason to reject this patch.

Likewise for call chains that end in an HMP command, because
error_report_err() does the right thing then: it reports to the current
monitor.  Bug fix: before the patch, it reports to stderr.  Commit
message should mention the fix.

The problematic case is audio_add().  Reporting an error and then
ignoring it feels wrong.  But it's how audio_add() works even before
this patch.  Not this patch's problem to solve.

> diff --git a/block.c b/block.c
> index 72c7f6d47d..7a94739aed 100644
> --- a/block.c
> +++ b/block.c
> @@ -464,12 +464,18 @@ BlockDriver *bdrv_find_format(const char *format_name)
>      /* The driver isn't registered, maybe we need to load a module */
>      for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
>          if (!strcmp(block_driver_modules[i].format_name, format_name)) {
> -            block_module_load(block_driver_modules[i].library_name);
> +            Error *local_err = NULL;
> +            int rv = block_module_load(block_driver_modules[i].library_name,
> +                                       &local_err);
> +            if (rv > 0) {
> +                return bdrv_do_find_format(format_name);
> +            } else if (rv < 0) {
> +                error_report_err(local_err);
> +            }
>              break;
>          }
>      }
> -
> -    return bdrv_do_find_format(format_name);
> +    return NULL;
>  }
>  

Same regression, I think.

Callers:

* bdrv_open_common()

* bdrv_fill_options()

* bdrv_open_inherit()

* bdrv_insert_node()

* bdrv_img_create()

* qmp_x_blockdev_amend()

* qmp_blockdev_create()

* qcow_co_create_opts()

* enable_write_target()

These all use the Error API.  Thus, we have instances of the
"error_report() or similar from within a user of th Error API"
anti-pattern.

Let's look more closely at just one of them: qmp_blockdev_create().
Since we're in QMP monitor context, bdrv_find_format()'s
error_report_err() will report a specific error to stderr, and then
qmp_blockdev_create() will report a less specific one to the QMP client.
This is wrong.

The obvious fix is to convert bdrv_find_format() to the Error API.

If you can't do that now, please note the issue in the commit message.

>  static int bdrv_format_is_whitelisted(const char *format_name, bool read_only)
> @@ -976,12 +982,16 @@ BlockDriver *bdrv_find_protocol(const char *filename,
>      for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
>          if (block_driver_modules[i].protocol_name &&
>              !strcmp(block_driver_modules[i].protocol_name, protocol)) {
> -            block_module_load(block_driver_modules[i].library_name);
> +            int rv = block_module_load(block_driver_modules[i].library_name, errp);
> +            if (rv > 0) {
> +                drv1 = bdrv_do_find_protocol(protocol);
> +            } else if (rv < 0) {
> +                return NULL;
> +            }
>              break;
>          }
>      }
>  
> -    drv1 = bdrv_do_find_protocol(protocol);
>      if (!drv1) {
>          error_setg(errp, "Unknown protocol '%s'", protocol);
>      }

Same regression, I think.

bdrv_find_protocol() already uses the Error API, and your patch passes
the Error up the chain.  Good.

[...]

Out of steam for today, intend to continue later.  If you want to respin
meanwhile, that's okay.  If you prefer to wait for me to finish
reviewing this one, that's okay, too.

Re: [PATCH v7 3/5] module: add Error arguments to module_load and module_load_qom

[Claudio Fontana]

On 9/28/22 13:31, Markus Armbruster wrote:
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> improve error handling during module load, by changing:
>>
>> bool module_load(const char *prefix, const char *lib_name);
>> void module_load_qom(const char *type);
>>
>> to:
>>
>> int module_load(const char *prefix, const char *name, Error **errp);
>> int module_load_qom(const char *type, Error **errp);
>>
>> where the return value is:
>>
>>  -1 on module load error, and errp is set with the error
>>   0 on module or one of its dependencies are not installed
>>   1 on module load success
>>   2 on module load success (module already loaded or built-in)
> 
> Two changes, if I understand things correctly:
> 
> 1. Convert to Error API from fprintf(stderr, ...)
> 
> 2. Return a more useful value
> 
> Right?

Yes.

> 
> Do you add any new errors here that weren't reported before?  Just

Yes.

> trying to calibrate my expectations before I dig into the actual patch.
> 
>> module_load_qom_one has been introduced in:
>>
>> commit 28457744c345 ("module: qom module support"), which built on top of
>> module_load_one, but discarded the bool return value. Restore it.
>>
>> Adapt all callers to emit errors, or ignore them, or fail hard,
>> as appropriate in each context.
>>
>> Some memory leaks also fixed as part of the module_load changes.
> 
> Where?

I misinterpreted the use of g_hash_table_add, so there is a fix for me here too (module_name should not be freed as the g_hash_table_add takes ownership).
The g_* API is a bit messy here, as the ownership and whether the key is freed depends on how the table was created (supplying a free function or not).

However, the code in the loop following the g_hash_table_add may "return false;" from the function directly,
skipping the cleanup code:

g_hash_table_add(loaded_modules, module_name);

-    for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
-        if (modinfo->arch) {
-            if (strcmp(modinfo->name, module_name) == 0) {
-                if (!module_check_arch(modinfo)) {
-                    return false;
-                }
-            }
-        }
     }

So there is no chance to free at:

-    if (!success) {
-        g_hash_table_remove(loaded_modules, module_name);
-        g_free(module_name);
      }

But, I should not have taken the g_free(...) out of the test, will fix.



> 
>> audio: when attempting to load an audio module, report module load errors.
>> block: when attempting to load a block module, report module load errors.
>> console: when attempting to load a display module, report module load errors.
>>
>> qdev: when creating a new qdev Device object (DeviceState), report load errors.
>>       If a module cannot be loaded to create that device, now abort execution.
>>
>> qom/object.c: when initializing a QOM object, or looking up class_by_name,
>>               report module load errors.
>>
>> qtest: when processing the "module_load" qtest command, report errors
>>        in the load of the module.
> 
> This looks like a list of behavioral changes.  Appreciated!  It's a bit
> terse, though.  I might come back to this and suggest improvement.  But
> first, I need to understand the patch.
> 
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  audio/audio.c         |  16 ++--
>>  block.c               |  20 +++-
>>  block/dmg.c           |  14 ++-
>>  hw/core/qdev.c        |  17 +++-
>>  include/qemu/module.h |  37 +++++++-
>>  qom/object.c          |  18 +++-
>>  softmmu/qtest.c       |   8 +-
>>  ui/console.c          |  18 +++-
>>  util/module.c         | 211 +++++++++++++++++++++++-------------------
>>  9 files changed, 235 insertions(+), 124 deletions(-)
>>
>> diff --git a/audio/audio.c b/audio/audio.c
>> index 0a682336a0..ea51793843 100644
>> --- a/audio/audio.c
>> +++ b/audio/audio.c
>> @@ -72,20 +72,24 @@ void audio_driver_register(audio_driver *drv)
>>  audio_driver *audio_driver_lookup(const char *name)
>>  {
>>      struct audio_driver *d;
>> +    Error *local_err = NULL;
>> +    int rv;
>>  
>>      QLIST_FOREACH(d, &audio_drivers, next) {
>>          if (strcmp(name, d->name) == 0) {
>>              return d;
>>          }
>>      }
>> -
>> -    audio_module_load(name);
>> -    QLIST_FOREACH(d, &audio_drivers, next) {
>> -        if (strcmp(name, d->name) == 0) {
>> -            return d;
>> +    rv = audio_module_load(name, &local_err);
>> +    if (rv > 0) {
>> +        QLIST_FOREACH(d, &audio_drivers, next) {
>> +            if (strcmp(name, d->name) == 0) {
>> +                return d;
>> +            }
>>          }
>> +    } else if (rv < 0) {
>> +        error_report_err(local_err);
>>      }
>> -
>>      return NULL;
>>  }
>>  
> 
> Before: audio_module_load() reports to stderr, but the caller can't

before: reports _some_ errors to stderr

> know.  So, error or no error, search the driver registry for the one we
> want.  Return it if found, else fail.
> 
> After: if audio_module_load() fails, report to stderr or current
> monitor, and fail.  If it could find no module or loaded one, search the
> driver registry.  Return it if found, else fail.
> 
> What if audio_module_load() fails, but a search for the driver succeeds?
> Before the patch, we succeed.  

audio_module_load() is the only way that audio_drivers can be updated and the search would return a different result.
The previous code was just lazily running the same code twice to make it simpler for the programmer in those conditions.

Afterwards, we fail.  Can this happen?

No.

> Yes: module_load() starts with
> 
>        if (!g_module_supported()) {
>            error_setg(errp, "%s", "this platform does not support GLib modules");
>            return -1;
>        }
> 
> Regression.
> 
> One way to fix: ensure module_load() cannot fail when the search will
> succeed.
> 
> Another: search first, and module_load() only if not found, then search
> again.
> 

Does not apply.

> Now let's review your use of the Error API.  Error objects should be
> passed up the call chain to the place that handles then.  The call
> chains are:
> 
> * audio_init(), which is called by
> 
>   - AUD_register_card(), which is called by a bunch of device creation
>     helpers called within machine initialization functions (I think),
>     which are called from qemu_init() via qemu_init_board() and
>     machine_run_board_init()
> 
>   - AUD_add_capture(), which is called by
> 
>     · wav_start_capture(), which is called by hmp_wavcapture on behalf
>       of HMP command wavcapture
> 
>     · audio_add(), which is called by protocol_client_msg() in respone
>       of a VNC_MSG_CLIENT_QEMU_AUDIO_ENABLE message from a VNC client (I
>       think).  Since audio_add() does return anything, its callers
>       remain oblivious of failure.
> 
>   - audio_init_audiodevs(), which is called by
>     qemu_create_early_backends(), which is called by qemu_init()
> 
> * audio_help(), which is called by
> 
>   - audio_parse_option(), which is called by qemu_init()
> 
>   - qemu_init()
> 
> * audio_handle_legacy_opts()
> 
>   - audio_init(); see above
> 
>   - audio_legacy_help(), which is called by qemu_init()
> 
> For the call chains that end in qemu_init(), error_report_err() is okay.
> Just beware of error cascades, i.e. one issue triggering multiple
> reports.  That's bad UX.  The extra reporting should predate this patch,
> which means it's no reason to reject this patch.
> 
> Likewise for call chains that end in an HMP command, because
> error_report_err() does the right thing then: it reports to the current
> monitor.  Bug fix: before the patch, it reports to stderr.  Commit
> message should mention the fix.
> 
> The problematic case is audio_add().  Reporting an error and then
> ignoring it feels wrong.  But it's how audio_add() works even before
> this patch.  Not this patch's problem to solve.
> 
>> diff --git a/block.c b/block.c
>> index 72c7f6d47d..7a94739aed 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -464,12 +464,18 @@ BlockDriver *bdrv_find_format(const char *format_name)
>>      /* The driver isn't registered, maybe we need to load a module */
>>      for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
>>          if (!strcmp(block_driver_modules[i].format_name, format_name)) {
>> -            block_module_load(block_driver_modules[i].library_name);
>> +            Error *local_err = NULL;
>> +            int rv = block_module_load(block_driver_modules[i].library_name,
>> +                                       &local_err);
>> +            if (rv > 0) {
>> +                return bdrv_do_find_format(format_name);
>> +            } else if (rv < 0) {
>> +                error_report_err(local_err);
>> +            }
>>              break;
>>          }
>>      }
>> -
>> -    return bdrv_do_find_format(format_name);
>> +    return NULL;
>>  }
>>  
> 
> Same regression, I think.

There is no problem here either.

> 
> Callers:
> 
> * bdrv_open_common()
> 
> * bdrv_fill_options()
> 
> * bdrv_open_inherit()
> 
> * bdrv_insert_node()
> 
> * bdrv_img_create()
> 
> * qmp_x_blockdev_amend()
> 
> * qmp_blockdev_create()
> 
> * qcow_co_create_opts()
> 
> * enable_write_target()
> 
> These all use the Error API.  Thus, we have instances of the
> "error_report() or similar from within a user of th Error API"
> anti-pattern.
> 
> Let's look more closely at just one of them: qmp_blockdev_create().
> Since we're in QMP monitor context, bdrv_find_format()'s
> error_report_err() will report a specific error to stderr, and then
> qmp_blockdev_create() will report a less specific one to the QMP client.
> This is wrong.
> 
> The obvious fix is to convert bdrv_find_format() to the Error API.
> 
> If you can't do that now, please note the issue in the commit message.
> 
>>  static int bdrv_format_is_whitelisted(const char *format_name, bool read_only)
>> @@ -976,12 +982,16 @@ BlockDriver *bdrv_find_protocol(const char *filename,
>>      for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
>>          if (block_driver_modules[i].protocol_name &&
>>              !strcmp(block_driver_modules[i].protocol_name, protocol)) {
>> -            block_module_load(block_driver_modules[i].library_name);
>> +            int rv = block_module_load(block_driver_modules[i].library_name, errp);
>> +            if (rv > 0) {
>> +                drv1 = bdrv_do_find_protocol(protocol);
>> +            } else if (rv < 0) {
>> +                return NULL;
>> +            }
>>              break;
>>          }
>>      }
>>  
>> -    drv1 = bdrv_do_find_protocol(protocol);
>>      if (!drv1) {
>>          error_setg(errp, "Unknown protocol '%s'", protocol);
>>      }
> 
> Same regression, I think.
> 
> bdrv_find_protocol() already uses the Error API, and your patch passes
> the Error up the chain.  Good.

This can be done here.

> 
> [...]
> 
> Out of steam for today, intend to continue later.  If you want to respin
> meanwhile, that's okay.  If you prefer to wait for me to finish
> reviewing this one, that's okay, too.
> 

I will be in vacation for two weeks, my concern is that this issue will be lost to the noise.
Will respin with the actual fixes, and hope this can be put into qemu and then move from there.

Thanks,

Claudio

Re: [PATCH v7 3/5] module: add Error arguments to module_load and module_load_qom

[Markus Armbruster]

Claudio Fontana <cfontana@suse.de> writes:

> On 9/28/22 13:31, Markus Armbruster wrote:
>> Claudio Fontana <cfontana@suse.de> writes:
>> 
>>> improve error handling during module load, by changing:
>>>
>>> bool module_load(const char *prefix, const char *lib_name);
>>> void module_load_qom(const char *type);
>>>
>>> to:
>>>
>>> int module_load(const char *prefix, const char *name, Error **errp);
>>> int module_load_qom(const char *type, Error **errp);
>>>
>>> where the return value is:
>>>
>>>  -1 on module load error, and errp is set with the error
>>>   0 on module or one of its dependencies are not installed
>>>   1 on module load success
>>>   2 on module load success (module already loaded or built-in)
>> 
>> Two changes, if I understand things correctly:
>> 
>> 1. Convert to Error API from fprintf(stderr, ...)
>> 
>> 2. Return a more useful value
>> 
>> Right?
>
> Yes.
>
>> 
>> Do you add any new errors here that weren't reported before?  Just
>
> Yes.

Thanks!

>> trying to calibrate my expectations before I dig into the actual patch.
>> 
>>> module_load_qom_one has been introduced in:
>>>
>>> commit 28457744c345 ("module: qom module support"), which built on top of
>>> module_load_one, but discarded the bool return value. Restore it.
>>>
>>> Adapt all callers to emit errors, or ignore them, or fail hard,
>>> as appropriate in each context.
>>>
>>> Some memory leaks also fixed as part of the module_load changes.

[...]

>>> audio: when attempting to load an audio module, report module load errors.
>>> block: when attempting to load a block module, report module load errors.
>>> console: when attempting to load a display module, report module load errors.
>>>
>>> qdev: when creating a new qdev Device object (DeviceState), report load errors.
>>>       If a module cannot be loaded to create that device, now abort execution.
>>>
>>> qom/object.c: when initializing a QOM object, or looking up class_by_name,
>>>               report module load errors.
>>>
>>> qtest: when processing the "module_load" qtest command, report errors
>>>        in the load of the module.
>> 
>> This looks like a list of behavioral changes.  Appreciated!  It's a bit
>> terse, though.  I might come back to this and suggest improvement.  But
>> first, I need to understand the patch.
>> 
>>>
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> ---
>>>  audio/audio.c         |  16 ++--
>>>  block.c               |  20 +++-
>>>  block/dmg.c           |  14 ++-
>>>  hw/core/qdev.c        |  17 +++-
>>>  include/qemu/module.h |  37 +++++++-
>>>  qom/object.c          |  18 +++-
>>>  softmmu/qtest.c       |   8 +-
>>>  ui/console.c          |  18 +++-
>>>  util/module.c         | 211 +++++++++++++++++++++++-------------------
>>>  9 files changed, 235 insertions(+), 124 deletions(-)
>>>
>>> diff --git a/audio/audio.c b/audio/audio.c
>>> index 0a682336a0..ea51793843 100644
>>> --- a/audio/audio.c
>>> +++ b/audio/audio.c
>>> @@ -72,20 +72,24 @@ void audio_driver_register(audio_driver *drv)
>>>  audio_driver *audio_driver_lookup(const char *name)
>>>  {
>>>      struct audio_driver *d;
>>> +    Error *local_err = NULL;
>>> +    int rv;
>>>  
>>>      QLIST_FOREACH(d, &audio_drivers, next) {
>>>          if (strcmp(name, d->name) == 0) {
>>>              return d;
>>>          }
>>>      }
>>> -
>>> -    audio_module_load(name);
>>> -    QLIST_FOREACH(d, &audio_drivers, next) {
>>> -        if (strcmp(name, d->name) == 0) {
>>> -            return d;
>>> +    rv = audio_module_load(name, &local_err);
>>> +    if (rv > 0) {
>>> +        QLIST_FOREACH(d, &audio_drivers, next) {
>>> +            if (strcmp(name, d->name) == 0) {
>>> +                return d;
>>> +            }
>>>          }
>>> +    } else if (rv < 0) {
>>> +        error_report_err(local_err);
>>>      }
>>> -
>>>      return NULL;
>>>  }
>>>  
>> 
>> Before: audio_module_load() reports to stderr, but the caller can't
>
> before: reports _some_ errors to stderr

Thanks for the reminder.

>> know.  So, error or no error, search the driver registry for the one we
>> want.  Return it if found, else fail.
>> 
>> After: if audio_module_load() fails, report to stderr or current
>> monitor, and fail.  If it could find no module or loaded one, search the
>> driver registry.  Return it if found, else fail.
>> 
>> What if audio_module_load() fails, but a search for the driver succeeds?
>> Before the patch, we succeed.  
>
> audio_module_load() is the only way that audio_drivers can be updated and the search would return a different result.

Not true.

@audio_driver gets built with audio_driver_register().  Audio drivers
call it via type_init().  For instance:

    static void register_audio_none(void)
    {
        audio_driver_register(&no_audio_driver);
    }
    type_init(register_audio_none);

My build of qemu-system-x86_64 puts "none", "wav", "alsa", "oss", "pa",
"sdl", and "spice" into @audio_driver without entering module_load().

> The previous code was just lazily running the same code twice to make it simpler for the programmer in those conditions.
>
> Afterwards, we fail.  Can this happen?
>
> No.

It can't, but not for the reason you claimed.  The error in my argument
was a misreading of the patch: we *still* only call module_load() when
the driver is not in @audio_driver.

I wish I was better at reading patches.  And, if I may be so frank, I
wish you were better at identifying my errors.  Telling me I'm wrong
when I actually am wrong is helpful (thanks!), but the way you told me
this time made me waste time chasing phantoms.

[...]

Re: [PATCH v7 3/5] module: add Error arguments to module_load and module_load_qom

[Claudio Fontana]

On 9/30/22 13:50, Markus Armbruster wrote:
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> On 9/28/22 13:31, Markus Armbruster wrote:
>>> Claudio Fontana <cfontana@suse.de> writes:
>>>
>>>> improve error handling during module load, by changing:
>>>>
>>>> bool module_load(const char *prefix, const char *lib_name);
>>>> void module_load_qom(const char *type);
>>>>
>>>> to:
>>>>
>>>> int module_load(const char *prefix, const char *name, Error **errp);
>>>> int module_load_qom(const char *type, Error **errp);
>>>>
>>>> where the return value is:
>>>>
>>>>  -1 on module load error, and errp is set with the error
>>>>   0 on module or one of its dependencies are not installed
>>>>   1 on module load success
>>>>   2 on module load success (module already loaded or built-in)
>>>
>>> Two changes, if I understand things correctly:
>>>
>>> 1. Convert to Error API from fprintf(stderr, ...)
>>>
>>> 2. Return a more useful value
>>>
>>> Right?
>>
>> Yes.
>>
>>>
>>> Do you add any new errors here that weren't reported before?  Just
>>
>> Yes.
> 
> Thanks!
> 
>>> trying to calibrate my expectations before I dig into the actual patch.
>>>
>>>> module_load_qom_one has been introduced in:
>>>>
>>>> commit 28457744c345 ("module: qom module support"), which built on top of
>>>> module_load_one, but discarded the bool return value. Restore it.
>>>>
>>>> Adapt all callers to emit errors, or ignore them, or fail hard,
>>>> as appropriate in each context.
>>>>
>>>> Some memory leaks also fixed as part of the module_load changes.
> 
> [...]
> 
>>>> audio: when attempting to load an audio module, report module load errors.
>>>> block: when attempting to load a block module, report module load errors.
>>>> console: when attempting to load a display module, report module load errors.
>>>>
>>>> qdev: when creating a new qdev Device object (DeviceState), report load errors.
>>>>       If a module cannot be loaded to create that device, now abort execution.
>>>>
>>>> qom/object.c: when initializing a QOM object, or looking up class_by_name,
>>>>               report module load errors.
>>>>
>>>> qtest: when processing the "module_load" qtest command, report errors
>>>>        in the load of the module.
>>>
>>> This looks like a list of behavioral changes.  Appreciated!  It's a bit
>>> terse, though.  I might come back to this and suggest improvement.  But
>>> first, I need to understand the patch.
>>>
>>>>
>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>> ---
>>>>  audio/audio.c         |  16 ++--
>>>>  block.c               |  20 +++-
>>>>  block/dmg.c           |  14 ++-
>>>>  hw/core/qdev.c        |  17 +++-
>>>>  include/qemu/module.h |  37 +++++++-
>>>>  qom/object.c          |  18 +++-
>>>>  softmmu/qtest.c       |   8 +-
>>>>  ui/console.c          |  18 +++-
>>>>  util/module.c         | 211 +++++++++++++++++++++++-------------------
>>>>  9 files changed, 235 insertions(+), 124 deletions(-)
>>>>
>>>> diff --git a/audio/audio.c b/audio/audio.c
>>>> index 0a682336a0..ea51793843 100644
>>>> --- a/audio/audio.c
>>>> +++ b/audio/audio.c
>>>> @@ -72,20 +72,24 @@ void audio_driver_register(audio_driver *drv)
>>>>  audio_driver *audio_driver_lookup(const char *name)
>>>>  {
>>>>      struct audio_driver *d;
>>>> +    Error *local_err = NULL;
>>>> +    int rv;
>>>>  
>>>>      QLIST_FOREACH(d, &audio_drivers, next) {
>>>>          if (strcmp(name, d->name) == 0) {
>>>>              return d;
>>>>          }
>>>>      }
>>>> -
>>>> -    audio_module_load(name);
>>>> -    QLIST_FOREACH(d, &audio_drivers, next) {
>>>> -        if (strcmp(name, d->name) == 0) {
>>>> -            return d;
>>>> +    rv = audio_module_load(name, &local_err);
>>>> +    if (rv > 0) {
>>>> +        QLIST_FOREACH(d, &audio_drivers, next) {
>>>> +            if (strcmp(name, d->name) == 0) {
>>>> +                return d;
>>>> +            }
>>>>          }
>>>> +    } else if (rv < 0) {
>>>> +        error_report_err(local_err);
>>>>      }
>>>> -
>>>>      return NULL;
>>>>  }
>>>>  
>>>
>>> Before: audio_module_load() reports to stderr, but the caller can't
>>
>> before: reports _some_ errors to stderr
> 
> Thanks for the reminder.
> 
>>> know.  So, error or no error, search the driver registry for the one we
>>> want.  Return it if found, else fail.
>>>
>>> After: if audio_module_load() fails, report to stderr or current
>>> monitor, and fail.  If it could find no module or loaded one, search the
>>> driver registry.  Return it if found, else fail.
>>>
>>> What if audio_module_load() fails, but a search for the driver succeeds?
>>> Before the patch, we succeed.  
>>
>> audio_module_load() is the only way that audio_drivers can be updated and the search would return a different result.
> 
> Not true.
> 
> @audio_driver gets built with audio_driver_register().  Audio drivers
> call it via type_init().  For instance:
> 
>     static void register_audio_none(void)
>     {
>         audio_driver_register(&no_audio_driver);
>     }
>     type_init(register_audio_none);
> 
> My build of qemu-system-x86_64 puts "none", "wav", "alsa", "oss", "pa",
> "sdl", and "spice" into @audio_driver without entering module_load().
> 
>> The previous code was just lazily running the same code twice to make it simpler for the programmer in those conditions.
>>
>> Afterwards, we fail.  Can this happen?
>>
>> No.
> 
> It can't, but not for the reason you claimed.  The error in my argument
> was a misreading of the patch: we *still* only call module_load() when
> the driver is not in @audio_driver.
> 
> I wish I was better at reading patches.  And, if I may be so frank, I
> wish you were better at identifying my errors.  Telling me I'm wrong
> when I actually am wrong is helpful (thanks!), but the way you told me
> this time made me waste time chasing phantoms.
> 
> [...]
> 

Hi Markus,

sorry I missed this email from you, I am just back from sick leave.

The latest is v9 at:

https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg05092.html

And it's fully reviewed from my perspective, anything else that needs to be done to make this series proceed?

thanks,

Claudio