* [PATCH v5 0/4] improve error handling for module load
@ 2022-09-23 14:51 Claudio Fontana
2022-09-23 14:51 ` [PATCH v5 1/4] module: removed unused function argument "mayfail" Claudio Fontana
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Claudio Fontana @ 2022-09-23 14:51 UTC (permalink / raw)
To: Paolo Bonzini, Richard Henderson, Markus Armbruster, Kevin Wolf
Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau,
Daniel P . Berrangé, Philippe Mathieu-Daudé,
Claudio Fontana
CHANGELOG:
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
accel/accel-softmmu.c | 8 +-
audio/audio.c | 16 ++--
block.c | 21 ++++-
block/dmg.c | 19 +++-
hw/core/qdev.c | 5 +-
include/qemu/module.h | 37 +++++++-
qom/object.c | 18 +++-
softmmu/qtest.c | 8 +-
ui/console.c | 18 +++-
util/module.c | 213 +++++++++++++++++++++++-------------------
10 files changed, 237 insertions(+), 126 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH v5 1/4] module: removed unused function argument "mayfail" 2022-09-23 14:51 [PATCH v5 0/4] improve error handling for module load Claudio Fontana @ 2022-09-23 14:51 ` Claudio Fontana 2022-09-23 14:51 ` [PATCH v5 2/4] module: rename module_load_one to module_load Claudio Fontana ` (2 subsequent siblings) 3 siblings, 0 replies; 12+ messages in thread From: Claudio Fontana @ 2022-09-23 14:51 UTC (permalink / raw) To: Paolo Bonzini, Richard Henderson, Markus Armbruster, Kevin Wolf Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v5 2/4] module: rename module_load_one to module_load 2022-09-23 14:51 [PATCH v5 0/4] improve error handling for module load Claudio Fontana 2022-09-23 14:51 ` [PATCH v5 1/4] module: removed unused function argument "mayfail" Claudio Fontana @ 2022-09-23 14:51 ` Claudio Fontana 2022-09-23 15:19 ` Philippe Mathieu-Daudé via 2022-09-23 14:51 ` [PATCH v5 3/4] module: add Error arguments to module_load and module_load_qom Claudio Fontana 2022-09-23 14:51 ` [PATCH v5 4/4] accel: abort if we fail to load the accelerator plugin Claudio Fontana 3 siblings, 1 reply; 12+ messages in thread From: Claudio Fontana @ 2022-09-23 14:51 UTC (permalink / raw) To: Paolo Bonzini, Richard Henderson, Markus Armbruster, Kevin Wolf Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé, Claudio Fontana Signed-off-by: Claudio Fontana <cfontana@suse.de> --- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v5 2/4] module: rename module_load_one to module_load 2022-09-23 14:51 ` [PATCH v5 2/4] module: rename module_load_one to module_load Claudio Fontana @ 2022-09-23 15:19 ` Philippe Mathieu-Daudé via 0 siblings, 0 replies; 12+ messages in thread From: Philippe Mathieu-Daudé via @ 2022-09-23 15:19 UTC (permalink / raw) To: Claudio Fontana, Paolo Bonzini, Richard Henderson, Markus Armbruster, Kevin Wolf Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé On 23/9/22 16:51, Claudio Fontana wrote: > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > 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(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v5 3/4] module: add Error arguments to module_load and module_load_qom 2022-09-23 14:51 [PATCH v5 0/4] improve error handling for module load Claudio Fontana 2022-09-23 14:51 ` [PATCH v5 1/4] module: removed unused function argument "mayfail" Claudio Fontana 2022-09-23 14:51 ` [PATCH v5 2/4] module: rename module_load_one to module_load Claudio Fontana @ 2022-09-23 14:51 ` Claudio Fontana 2022-09-23 15:28 ` Philippe Mathieu-Daudé via 2022-09-23 15:51 ` Daniel P. Berrangé 2022-09-23 14:51 ` [PATCH v5 4/4] accel: abort if we fail to load the accelerator plugin Claudio Fontana 3 siblings, 2 replies; 12+ messages in thread From: Claudio Fontana @ 2022-09-23 14:51 UTC (permalink / raw) To: Paolo Bonzini, Richard Henderson, Markus Armbruster, Kevin Wolf Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé, 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. Signed-off-by: Claudio Fontana <cfontana@suse.de> --- audio/audio.c | 16 ++-- block.c | 21 ++++- block/dmg.c | 19 +++- hw/core/qdev.c | 5 +- include/qemu/module.h | 37 +++++++- qom/object.c | 18 +++- softmmu/qtest.c | 8 +- ui/console.c | 18 +++- util/module.c | 211 +++++++++++++++++++++++------------------- 9 files changed, 230 insertions(+), 123 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..0390ece056 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,17 @@ 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); + Error *local_err = NULL; + int rv = block_module_load(block_driver_modules[i].library_name, &local_err); + if (rv > 0) { + drv1 = bdrv_do_find_protocol(protocol); + } else if (rv < 0) { + error_report_err(local_err); + } 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..e84a7a44a3 100644 --- a/block/dmg.c +++ b/block/dmg.c @@ -434,6 +434,8 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, uint64_t plist_xml_offset, plist_xml_length; int64_t offset; int ret; + int module_rv; + Error *local_err = NULL; ret = bdrv_apply_auto_read_only(bs, NULL, errp); if (ret < 0) { @@ -446,8 +448,21 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, return -EINVAL; } - block_module_load("dmg-bz2"); - block_module_load("dmg-lzfse"); + module_rv = block_module_load("dmg-bz2", &local_err); + if (module_rv < 0) { + error_report_err(local_err); + return -EINVAL; + } else if (module_rv == 0) { + warn_report("dmg-bz2 module not present, bz2 decomp unavailable"); + } + local_err = NULL; + module_rv = block_module_load("dmg-lzfse", &local_err); + if (module_rv < 0) { + error_report_err(local_err); + return -EINVAL; + } else if (module_rv == 0) { + warn_report("dmg-lzfse module not present, lzfse decomp unavailable"); + } 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..06a4bce145 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -147,8 +147,9 @@ 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); + if (!module_object_class_by_name(name)) { + error_report("could not find a module for 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..35ced55282 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); + Error *local_err = NULL; + int rv = module_load_qom(typename, &local_err); + if (rv > 0) { + type = type_get_by_name(typename); + } else if (rv < 0) { + error_report_err(local_err); + } } #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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/4] module: add Error arguments to module_load and module_load_qom 2022-09-23 14:51 ` [PATCH v5 3/4] module: add Error arguments to module_load and module_load_qom Claudio Fontana @ 2022-09-23 15:28 ` Philippe Mathieu-Daudé via 2022-09-23 22:41 ` Claudio Fontana 2022-09-23 15:51 ` Daniel P. Berrangé 1 sibling, 1 reply; 12+ messages in thread From: Philippe Mathieu-Daudé via @ 2022-09-23 15:28 UTC (permalink / raw) To: Claudio Fontana, Paolo Bonzini, Richard Henderson, Markus Armbruster, Kevin Wolf Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé On 23/9/22 16:51, Claudio Fontana wrote: > 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. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > audio/audio.c | 16 ++-- > block.c | 21 ++++- > block/dmg.c | 19 +++- > hw/core/qdev.c | 5 +- > include/qemu/module.h | 37 +++++++- > qom/object.c | 18 +++- > softmmu/qtest.c | 8 +- > ui/console.c | 18 +++- > util/module.c | 211 +++++++++++++++++++++++------------------- > 9 files changed, 230 insertions(+), 123 deletions(-) > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 25dfc08468..06a4bce145 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -147,8 +147,9 @@ 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); > + if (!module_object_class_by_name(name)) { > + error_report("could not find a module for type '%s'", name); > + abort(); No need to abort: exit(1); > } > 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..35ced55282 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); > + Error *local_err = NULL; > + int rv = module_load_qom(typename, &local_err); > + if (rv > 0) { > + type = type_get_by_name(typename); > + } else if (rv < 0) { > + error_report_err(local_err); > + } > } > #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)); Up to here: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> I need some sugar to review the rest, bbl. > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/4] module: add Error arguments to module_load and module_load_qom 2022-09-23 15:28 ` Philippe Mathieu-Daudé via @ 2022-09-23 22:41 ` Claudio Fontana 0 siblings, 0 replies; 12+ messages in thread From: Claudio Fontana @ 2022-09-23 22:41 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Paolo Bonzini, Richard Henderson, Markus Armbruster, Kevin Wolf Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé On 9/23/22 17:28, Philippe Mathieu-Daudé wrote: > On 23/9/22 16:51, Claudio Fontana wrote: >> 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. >> >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> --- >> audio/audio.c | 16 ++-- >> block.c | 21 ++++- >> block/dmg.c | 19 +++- >> hw/core/qdev.c | 5 +- >> include/qemu/module.h | 37 +++++++- >> qom/object.c | 18 +++- >> softmmu/qtest.c | 8 +- >> ui/console.c | 18 +++- >> util/module.c | 211 +++++++++++++++++++++++------------------- >> 9 files changed, 230 insertions(+), 123 deletions(-) > >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c >> index 25dfc08468..06a4bce145 100644 >> --- a/hw/core/qdev.c >> +++ b/hw/core/qdev.c >> @@ -147,8 +147,9 @@ 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); >> + if (!module_object_class_by_name(name)) { >> + error_report("could not find a module for type '%s'", name); >> + abort(); > > No need to abort: > > exit(1); Hi Philippe, I just noticed that all the rest of the code in similar situations in object.c abort()s in these case, never exit()s. Do you think this case is different than the others? The most emblematic example is in qom/object.c: void object_initialize(void *data, size_t size, const char *typename) { TypeImpl *type = type_get_by_name(typename); #ifdef CONFIG_MODULES if (!type) { Error *local_err = NULL; int rv = module_load_qom(typename, &local_err); if (rv > 0) { type = type_get_by_name(typename); } else if (rv < 0) { error_report_err(local_err); } } #endif if (!type) { error_report("missing object type '%s'", typename); abort(); } object_initialize_with_type(data, size, type); } Do you think we should always exit and not abort when we don't find a type? Just trying to avoid introducing inconsistent handling of the same situation. Thanks, Claudio > >> } >> 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..35ced55282 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); >> + Error *local_err = NULL; >> + int rv = module_load_qom(typename, &local_err); >> + if (rv > 0) { >> + type = type_get_by_name(typename); >> + } else if (rv < 0) { >> + error_report_err(local_err); >> + } >> } >> #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)); > > Up to here: > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > I need some sugar to review the rest, bbl. > >> 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/4] module: add Error arguments to module_load and module_load_qom 2022-09-23 14:51 ` [PATCH v5 3/4] module: add Error arguments to module_load and module_load_qom Claudio Fontana 2022-09-23 15:28 ` Philippe Mathieu-Daudé via @ 2022-09-23 15:51 ` Daniel P. Berrangé 2022-09-23 16:11 ` Claudio Fontana 1 sibling, 1 reply; 12+ messages in thread From: Daniel P. Berrangé @ 2022-09-23 15:51 UTC (permalink / raw) To: Claudio Fontana Cc: Paolo Bonzini, Richard Henderson, Markus Armbruster, Kevin Wolf, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Philippe Mathieu-Daudé On Fri, Sep 23, 2022 at 04:51:30PM +0200, Claudio Fontana wrote: > 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) > 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); > } The rv == 0 case could be treated the same as rv > 0 meaning the diff merely needs to be audio_module_load(name, &local_err) if (rv < 0) { error_report_err(local_err); return NULL;. } > return NULL; > } > > diff --git a/block.c b/block.c > index 72c7f6d47d..0390ece056 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); > + } Again, rv ==0 can be handled the same as rv > 0 > @@ -976,12 +982,17 @@ 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); > + Error *local_err = NULL; > + int rv = block_module_load(block_driver_modules[i].library_name, &local_err); > + if (rv > 0) { > + drv1 = bdrv_do_find_protocol(protocol); > + } else if (rv < 0) { > + error_report_err(local_err); > + } Likewise rv == 0 vs rv > 0 > diff --git a/block/dmg.c b/block/dmg.c > index 007b8d9996..e84a7a44a3 100644 > --- a/block/dmg.c > +++ b/block/dmg.c > @@ -434,6 +434,8 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, > uint64_t plist_xml_offset, plist_xml_length; > int64_t offset; > int ret; > + int module_rv; > + Error *local_err = NULL; > > ret = bdrv_apply_auto_read_only(bs, NULL, errp); > if (ret < 0) { > @@ -446,8 +448,21 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, > return -EINVAL; > } > > - block_module_load("dmg-bz2"); > - block_module_load("dmg-lzfse"); > + module_rv = block_module_load("dmg-bz2", &local_err); > + if (module_rv < 0) { > + error_report_err(local_err); > + return -EINVAL; > + } else if (module_rv == 0) { > + warn_report("dmg-bz2 module not present, bz2 decomp unavailable"); > + } > + local_err = NULL; > + module_rv = block_module_load("dmg-lzfse", &local_err); > + if (module_rv < 0) { > + error_report_err(local_err); > + return -EINVAL; > + } else if (module_rv == 0) { > + warn_report("dmg-lzfse module not present, lzfse decomp unavailable"); > + } THis is the wrong place for these warnings, it'll spam stdout, even if the file opened doesn't use bz2/lzfse. The real problem is the later code which appears to silently ignore data blocks if the lzfse/bz2 modules are not loaded, instead of using error_report. > diff --git a/qom/object.c b/qom/object.c > index 4f834f3bf6..35ced55282 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); > + Error *local_err = NULL; > + int rv = module_load_qom(typename, &local_err); > + if (rv > 0) { > + type = type_get_by_name(typename); > + } else if (rv < 0) { > + error_report_err(local_err); > + } Again no need to distinguish rv == 0 from rv > 0 > @@ -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); > + } Same comment > 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"); > } This change means the 'module_load' command is totally silent if 'rv == 0', but the code appears to try to read a response line which will now never arrive AFAICT. In the context of 'modules-test.c' I think it is fine to treat rv == 0 the same as rv > 0 and thus print 'OK'. Perhaps I've overlooked something, but I'm not seeing a reason we need module_load() to return 4 different return values. It looks like every caller would work with a boolean success/fail With 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 :| ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 3/4] module: add Error arguments to module_load and module_load_qom 2022-09-23 15:51 ` Daniel P. Berrangé @ 2022-09-23 16:11 ` Claudio Fontana 0 siblings, 0 replies; 12+ messages in thread From: Claudio Fontana @ 2022-09-23 16:11 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Paolo Bonzini, Richard Henderson, Markus Armbruster, Kevin Wolf, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Philippe Mathieu-Daudé On 9/23/22 17:51, Daniel P. Berrangé wrote: > On Fri, Sep 23, 2022 at 04:51:30PM +0200, Claudio Fontana wrote: >> 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) > > > > >> 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); >> } > > The rv == 0 case could be treated the same as rv > 0 Not really, there is no need to do the loop again if rv == 0. I mean it doesn't hurt, but Richard explicitly asked to remove redundant execution in these cases. > meaning the diff merely needs to be > > audio_module_load(name, &local_err) > if (rv < 0) { > error_report_err(local_err); > return NULL;. > } > >> return NULL; >> } >> >> diff --git a/block.c b/block.c >> index 72c7f6d47d..0390ece056 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); >> + } > > Again, rv ==0 can be handled the same as rv > 0 > > >> @@ -976,12 +982,17 @@ 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); >> + Error *local_err = NULL; >> + int rv = block_module_load(block_driver_modules[i].library_name, &local_err); >> + if (rv > 0) { >> + drv1 = bdrv_do_find_protocol(protocol); >> + } else if (rv < 0) { >> + error_report_err(local_err); >> + } > > Likewise rv == 0 vs rv > 0 same here, this approach was there around v2, but I changed this, to avoid calling bdrv_do_find_protocol() again if we did not load the module successfully, as we already know it will fail. > > > >> diff --git a/block/dmg.c b/block/dmg.c >> index 007b8d9996..e84a7a44a3 100644 >> --- a/block/dmg.c >> +++ b/block/dmg.c >> @@ -434,6 +434,8 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, >> uint64_t plist_xml_offset, plist_xml_length; >> int64_t offset; >> int ret; >> + int module_rv; >> + Error *local_err = NULL; >> >> ret = bdrv_apply_auto_read_only(bs, NULL, errp); >> if (ret < 0) { >> @@ -446,8 +448,21 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, >> return -EINVAL; >> } >> >> - block_module_load("dmg-bz2"); >> - block_module_load("dmg-lzfse"); >> + module_rv = block_module_load("dmg-bz2", &local_err); >> + if (module_rv < 0) { >> + error_report_err(local_err); >> + return -EINVAL; >> + } else if (module_rv == 0) { >> + warn_report("dmg-bz2 module not present, bz2 decomp unavailable"); >> + } >> + local_err = NULL; >> + module_rv = block_module_load("dmg-lzfse", &local_err); >> + if (module_rv < 0) { >> + error_report_err(local_err); >> + return -EINVAL; >> + } else if (module_rv == 0) { >> + warn_report("dmg-lzfse module not present, lzfse decomp unavailable"); >> + } > > THis is the wrong place for these warnings, it'll spam > stdout, even if the file opened doesn't use bz2/lzfse. > > The real problem is the later code which appears to > silently ignore data blocks if the lzfse/bz2 modules > are not loaded, instead of using error_report. Yes, the right place for the warning is a bit unknown to me though in the block code as I mentioned elsewhere in the thread, Kevin seems to understand what the right handling should be, maybe he can do an after-patch I could attach or merge in the series.. > > >> diff --git a/qom/object.c b/qom/object.c >> index 4f834f3bf6..35ced55282 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); >> + Error *local_err = NULL; >> + int rv = module_load_qom(typename, &local_err); >> + if (rv > 0) { >> + type = type_get_by_name(typename); >> + } else if (rv < 0) { >> + error_report_err(local_err); >> + } > > Again no need to distinguish rv == 0 from rv > 0 same answer here. I don't feel strongly, just saying, I changed this as a result of the review process. > >> @@ -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); >> + } > > Same comment same > >> 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"); >> } > > This change means the 'module_load' command is totally silent If rv == 0, qtest will do qtest_sendf(chr, "FAIL\n"); but will not report an error message. Should we? > if 'rv == 0', but the code appears to try to read a response > line which will now never arrive AFAICT. > > In the context of 'modules-test.c' I think it is fine to treat > rv == 0 the same as rv > 0 and thus print 'OK'. > > > > Perhaps I've overlooked something, but I'm not seeing a reason > we need module_load() to return 4 different return values. It > looks like every caller would work with a boolean success/fail > > > With regards, > Daniel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v5 4/4] accel: abort if we fail to load the accelerator plugin 2022-09-23 14:51 [PATCH v5 0/4] improve error handling for module load Claudio Fontana ` (2 preceding siblings ...) 2022-09-23 14:51 ` [PATCH v5 3/4] module: add Error arguments to module_load and module_load_qom Claudio Fontana @ 2022-09-23 14:51 ` Claudio Fontana 2022-09-23 15:28 ` Philippe Mathieu-Daudé via 3 siblings, 1 reply; 12+ messages in thread From: Claudio Fontana @ 2022-09-23 14:51 UTC (permalink / raw) To: Paolo Bonzini, Richard Henderson, Markus Armbruster, Kevin Wolf Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé, 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> --- 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..9fa4849f2c 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); + abort(); + } 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v5 4/4] accel: abort if we fail to load the accelerator plugin 2022-09-23 14:51 ` [PATCH v5 4/4] accel: abort if we fail to load the accelerator plugin Claudio Fontana @ 2022-09-23 15:28 ` Philippe Mathieu-Daudé via 2022-09-23 15:50 ` Claudio Fontana 0 siblings, 1 reply; 12+ messages in thread From: Philippe Mathieu-Daudé via @ 2022-09-23 15:28 UTC (permalink / raw) To: Claudio Fontana, Paolo Bonzini, Richard Henderson, Markus Armbruster, Kevin Wolf Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé On 23/9/22 16:51, Claudio Fontana wrote: > 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> > --- > 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..9fa4849f2c 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); > + abort(); I'm not sure aborting is helpful here: QEMU process state is alright, we only miss a module... Aren't we good with a simple '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. Preferably calling exit(): Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 4/4] accel: abort if we fail to load the accelerator plugin 2022-09-23 15:28 ` Philippe Mathieu-Daudé via @ 2022-09-23 15:50 ` Claudio Fontana 0 siblings, 0 replies; 12+ messages in thread From: Claudio Fontana @ 2022-09-23 15:50 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Paolo Bonzini, Richard Henderson, Markus Armbruster, Kevin Wolf Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé On 9/23/22 17:28, Philippe Mathieu-Daudé via wrote: > On 23/9/22 16:51, Claudio Fontana wrote: >> 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> >> --- >> 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..9fa4849f2c 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); >> + abort(); > > I'm not sure aborting is helpful here: QEMU process state is alright, > we only miss a module... > > Aren't we good with a simple 'exit(1);'? we are I think... Thanks, C > >> + } >> 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. > > Preferably calling exit(): > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-09-23 22:43 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-23 14:51 [PATCH v5 0/4] improve error handling for module load Claudio Fontana 2022-09-23 14:51 ` [PATCH v5 1/4] module: removed unused function argument "mayfail" Claudio Fontana 2022-09-23 14:51 ` [PATCH v5 2/4] module: rename module_load_one to module_load Claudio Fontana 2022-09-23 15:19 ` Philippe Mathieu-Daudé via 2022-09-23 14:51 ` [PATCH v5 3/4] module: add Error arguments to module_load and module_load_qom Claudio Fontana 2022-09-23 15:28 ` Philippe Mathieu-Daudé via 2022-09-23 22:41 ` Claudio Fontana 2022-09-23 15:51 ` Daniel P. Berrangé 2022-09-23 16:11 ` Claudio Fontana 2022-09-23 14:51 ` [PATCH v5 4/4] accel: abort if we fail to load the accelerator plugin Claudio Fontana 2022-09-23 15:28 ` Philippe Mathieu-Daudé via 2022-09-23 15:50 ` Claudio Fontana
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).