On 16/9/22 11:27, Markus Armbruster wrote:
Claudio Fontana <cfontana@suse.de> writes:
improve error handling during module load, by changing:
bool module_load_one(const char *prefix, const char *lib_name);
void module_load_qom_one(const char *type);
to:
bool module_load_one(const char *prefix, const char *name, Error **errp);
bool module_load_qom_one(const char *type, Error **errp);
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.
How exactly does behavior change? The commit message is mum on the
behavior before the patch, and vague on the behavior afterwards.
Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
audio/audio.c | 9 ++-
block.c | 15 ++++-
block/dmg.c | 18 +++++-
hw/core/qdev.c | 10 ++-
include/qemu/module.h | 38 ++++++++++--
qom/object.c | 18 +++++-
softmmu/qtest.c | 6 +-
ui/console.c | 18 +++++-
util/module.c | 140 ++++++++++++++++++++++++------------------
9 files changed, 194 insertions(+), 78 deletions(-)
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 8c012bbe03..78d4c4de96 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -61,16 +61,44 @@ typedef enum {
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);
+
+/*
+ * module_load_one: 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: true on success (found and loaded);
+ * if module if found, but load failed, errp will be set.
+ * if module is not found, errp will not be set.
I understand you need to distingush two failure modes "found, but load
failed" and "not found".
Functions that set an error on some failures only tend to be awkward: in
addition to checking the return value for failure, you have to check
@errp for special failures. This is particularly cumbersome when it
requires a @local_err and an error_propagate() just for that. I
generally prefer to return an error code and always set an error.
I notice the same issue, therefore would suggest this alternative
prototype:
bool module_load_one(const char *prefix, const char *name,
bool ignore_if_missing, Error **errp);
which always sets *errp when returning false:
* Return value: if ignore_if_missing is true:
* true on success (found or missing), false on
* load failure.
* if ignore_if_missing is false:
* true on success (found and loaded); false if
* not found or load failed.
* errp will be set if the returned value is false.
*/