* [PATCH v3 0/3] improve error handling for module load
@ 2022-09-08 14:53 Claudio Fontana
2022-09-08 14:53 ` [PATCH v3 1/3] module: removed unused function argument "mayfail" Claudio Fontana
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Claudio Fontana @ 2022-09-08 14:53 UTC (permalink / raw)
To: Paolo Bonzini, Richard Henderson, Markus Armbruster
Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau,
Daniel P . Berrangé, Philippe Mathieu-Daudé,
Claudio Fontana
CHANGELOG:
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)
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 (3):
module: removed unused function argument "mayfail"
module: add Error arguments to module_load_one and module_load_qom_one
accel: abort if we fail to load the accelerator plugin
accel/accel-softmmu.c | 8 ++-
audio/audio.c | 6 +-
block.c | 12 +++-
block/dmg.c | 10 ++-
hw/core/qdev.c | 10 ++-
include/qemu/module.h | 27 ++++++--
qom/object.c | 15 ++++-
softmmu/qtest.c | 6 +-
ui/console.c | 18 ++++-
util/module.c | 152 +++++++++++++++++++++++++-----------------
10 files changed, 183 insertions(+), 81 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v3 1/3] module: removed unused function argument "mayfail" 2022-09-08 14:53 [PATCH v3 0/3] improve error handling for module load Claudio Fontana @ 2022-09-08 14:53 ` Claudio Fontana 2022-09-08 14:53 ` [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one Claudio Fontana 2022-09-08 14:53 ` [PATCH v3 3/3] accel: abort if we fail to load the accelerator plugin Claudio Fontana 2 siblings, 0 replies; 22+ messages in thread From: Claudio Fontana @ 2022-09-08 14:53 UTC (permalink / raw) To: Paolo Bonzini, Richard Henderson, Markus Armbruster Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé, Claudio Fontana, Philippe Mathieu-Daudé 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] 22+ messages in thread
* [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-08 14:53 [PATCH v3 0/3] improve error handling for module load Claudio Fontana 2022-09-08 14:53 ` [PATCH v3 1/3] module: removed unused function argument "mayfail" Claudio Fontana @ 2022-09-08 14:53 ` Claudio Fontana 2022-09-08 16:03 ` Richard Henderson 2022-09-08 14:53 ` [PATCH v3 3/3] accel: abort if we fail to load the accelerator plugin Claudio Fontana 2 siblings, 1 reply; 22+ messages in thread From: Claudio Fontana @ 2022-09-08 14:53 UTC (permalink / raw) To: Paolo Bonzini, Richard Henderson, Markus Armbruster 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_one(const char *prefix, const char *lib_name); void module_load_qom_one(const char *type); to: bool module_load_one(const char *prefix, const char *name, Error **errp); bool module_load_qom_one(const char *type, Error **errp); module_load_qom_one has been introduced in: commit 28457744c345 ("module: qom module support"), which built on top of module_load_one, but discarded the bool return value. Restore it. Adapt all callers to emit errors, or ignore them, or fail hard, as appropriate in each context. Signed-off-by: Claudio Fontana <cfontana@suse.de> --- audio/audio.c | 6 +- block.c | 12 +++- block/dmg.c | 10 ++- hw/core/qdev.c | 10 ++- include/qemu/module.h | 27 ++++++-- qom/object.c | 15 ++++- softmmu/qtest.c | 6 +- ui/console.c | 18 ++++- util/module.c | 150 +++++++++++++++++++++++++----------------- 9 files changed, 176 insertions(+), 78 deletions(-) diff --git a/audio/audio.c b/audio/audio.c index 76b8735b44..4f4bb10cce 100644 --- a/audio/audio.c +++ b/audio/audio.c @@ -72,6 +72,7 @@ void audio_driver_register(audio_driver *drv) audio_driver *audio_driver_lookup(const char *name) { struct audio_driver *d; + Error *local_err = NULL; QLIST_FOREACH(d, &audio_drivers, next) { if (strcmp(name, d->name) == 0) { @@ -79,7 +80,10 @@ audio_driver *audio_driver_lookup(const char *name) } } - audio_module_load_one(name); + if (!audio_module_load_one(name, &local_err) && local_err) { + error_report_err(local_err); + } + QLIST_FOREACH(d, &audio_drivers, next) { if (strcmp(name, d->name) == 0) { return d; diff --git a/block.c b/block.c index bc85f46eed..85c3742d7a 100644 --- a/block.c +++ b/block.c @@ -464,7 +464,11 @@ 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); + Error *local_err = NULL; + if (!block_module_load_one(block_driver_modules[i].library_name, + &local_err) && local_err) { + error_report_err(local_err); + } break; } } @@ -976,7 +980,11 @@ 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); + Error *local_err = NULL; + if (!block_module_load_one(block_driver_modules[i].library_name, + &local_err) && local_err) { + error_report_err(local_err); + } break; } } diff --git a/block/dmg.c b/block/dmg.c index 98db18d82a..349b05d20b 100644 --- a/block/dmg.c +++ b/block/dmg.c @@ -434,6 +434,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, uint64_t plist_xml_offset, plist_xml_length; int64_t offset; int ret; + Error *local_err = NULL; ret = bdrv_apply_auto_read_only(bs, NULL, errp); if (ret < 0) { @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, return -EINVAL; } - block_module_load_one("dmg-bz2"); - block_module_load_one("dmg-lzfse"); + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { + error_report_err(local_err); + } + local_err = NULL; + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { + error_report_err(local_err); + } 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..5902c59c94 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -148,7 +148,15 @@ 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); + Error *local_err = NULL; + if (!module_load_qom_one(name, &local_err)) { + if (local_err) { + error_report_err(local_err); + } else { + 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 8c012bbe03..e1fd4a6d7b 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -61,16 +61,33 @@ 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_one(lib, errp) module_load_one("block-", lib, errp) +#define ui_module_load_one(lib, errp) module_load_one("ui-", lib, errp) +#define audio_module_load_one(lib, errp) module_load_one("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_one(const char *prefix, const char *lib_name); -void module_load_qom_one(const char *type); + +/* + * module_load_one: attempt to load a module from a set of directories + * + * directories searched are: + * - getenv("QEMU_MODULE_DIR") + * - get_relocated_path(CONFIG_QEMU_MODDIR); + * - /var/run/qemu/${version_dir} + * + * prefix: a subsystem prefix, or the empty string ("audio-", ..., "") + * name: name of the module + * errp: error to set in case the module is found, but load failed. + * + * Return value: true on success (found and loaded); + * if module if found, but load failed, errp will be set. + * if module is not found, errp will not be set. + */ +bool module_load_one(const char *prefix, const char *name, Error **errp); +bool module_load_qom_one(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 d34608558e..6a74e6a478 100644 --- a/qom/object.c +++ b/qom/object.c @@ -526,8 +526,14 @@ void object_initialize(void *data, size_t size, const char *typename) #ifdef CONFIG_MODULES if (!type) { - module_load_qom_one(typename); - type = type_get_by_name(typename); + Error *local_err = NULL; + if (!module_load_qom_one(typename, &local_err)) { + if (local_err) { + error_report_err(local_err); + } + } else { + type = type_get_by_name(typename); + } } #endif if (!type) { @@ -1033,7 +1039,10 @@ 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); + Error *local_err = NULL; + if (!module_load_qom_one(typename, &local_err) && local_err) { + error_report_err(local_err); + } oc = object_class_by_name(typename); } #endif diff --git a/softmmu/qtest.c b/softmmu/qtest.c index 76eb7bac56..bb83c7aae9 100644 --- a/softmmu/qtest.c +++ b/softmmu/qtest.c @@ -753,12 +753,16 @@ 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; g_assert(words[1] && words[2]); qtest_send_prefix(chr); - if (module_load_one(words[1], words[2])) { + if (module_load_one(words[1], words[2], &local_err)) { qtest_sendf(chr, "OK\n"); } else { + if (local_err) { + 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 765892f84f..34dd206167 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_one(DisplayType_str(prio[i])); + Error *local_err = NULL; + if (!ui_module_load_one(DisplayType_str(prio[i]), &local_err) + && local_err) { + 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_one(DisplayType_str(opts->type)); + Error *local_err = NULL; + if (!ui_module_load_one(DisplayType_str(opts->type), &local_err) + && local_err) { + 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_one(DisplayType_str(idx)); + Error *local_err = NULL; + if (!ui_module_load_one(DisplayType_str(idx), &local_err) + && local_err) { + 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 8563edd626..14e18e59c6 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"); } 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_one(const char *prefix, const char *lib_name) +bool module_load_one(const char *prefix, const char *name, Error **errp) { bool success = false; - -#ifdef CONFIG_MODULES - char *fname = NULL; #ifdef CONFIG_MODULE_UPGRADES char *version_dir; #endif @@ -219,14 +209,13 @@ bool module_load_one(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"); + error_setg(errp, "%s", "this platform does not support GLib modules"); return false; } @@ -234,7 +223,7 @@ bool module_load_one(const char *prefix, const char *lib_name) loaded_modules = g_hash_table_new(g_str_hash, g_str_equal); } - module_name = g_strdup_printf("%s%s", prefix, lib_name); + module_name = g_strdup_printf("%s%s", prefix, name); if (g_hash_table_contains(loaded_modules, module_name)) { g_free(module_name); @@ -246,6 +235,8 @@ bool module_load_one(const char *prefix, const char *lib_name) 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); return false; } } @@ -254,7 +245,9 @@ 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); + if (!(module_load_one("", *sl, errp))) { + return false; + } } } else { for (sl = modinfo->deps; *sl != NULL; sl++) { @@ -283,16 +276,26 @@ bool module_load_one(const char *prefix, const char *lib_name) 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; + char *fname = g_strdup_printf("%s/%s%s", + dirs[i], module_name, CONFIG_HOST_DSOSUF); + int ret = access(fname, F_OK); + if (ret != 0) { + if (errno == ENOENT || errno == ENOTDIR) { + /* + * if we just 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 or + * distro did not install the module. We will return false in this + * case, with no error set. + */ + continue; + } + /* most common is EACCES here */ + error_setg_errno(errp, errno, "error trying to access %s", fname); break; } + success = module_load_dso(fname, export_symbols, errp); + g_free(fname); + break; } if (!success) { @@ -304,21 +307,30 @@ bool module_load_one(const char *prefix, const char *lib_name) g_free(dirs[i]); } -#endif return success; } -#ifdef CONFIG_MODULES - static bool module_loaded_qom_all; -void module_load_qom_one(const char *type) +/* + * module_load_qom_one: attempt to load a module to provide a QOM type + * + * type: the type to be provided + * errp: error to set. + * + * Return value: true on success (found and loaded), false on failure. + * If a module is simply not found for the type, + * errp will not be set. + */ +bool module_load_qom_one(const char *type, Error **errp) { + bool found = false; const QemuModinfo *modinfo; const char **sl; if (!type) { - return; + error_setg(errp, "%s", "type is NULL"); + return false; } trace_module_lookup_object_type(type); @@ -331,15 +343,26 @@ 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); + if (found) { + error_setg(errp, "multiple modules providing '%s'", type); + found = false; + break; + } + found = module_load_one("", modinfo->name, errp); + if (!found) { + /* errp optionally set in module_load_one */ + break; + } } } } + return found; } void module_load_qom_all(void) { const QemuModinfo *modinfo; + Error *local_err = NULL; if (module_loaded_qom_all) { return; @@ -352,7 +375,9 @@ void module_load_qom_all(void) if (!module_check_arch(modinfo)) { continue; } - module_load_one("", modinfo->name); + if (!module_load_one("", modinfo->name, &local_err) && local_err) { + error_report_err(local_err); + } } module_loaded_qom_all = true; } @@ -368,7 +393,11 @@ 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); + Error *local_err = NULL; + if (!module_load_one("", modinfo->name, &local_err) + && local_err) { + error_report_err(local_err); + } } } } @@ -378,7 +407,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_one(const char *type) {} +bool module_load_one(const char *prefix, const char *name, Error **errp) { return true; } +bool module_load_qom_one(const char *type, Error **errp) { return true; } void module_load_qom_all(void) {} #endif -- 2.26.2 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-08 14:53 ` [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one Claudio Fontana @ 2022-09-08 16:03 ` Richard Henderson 2022-09-08 17:10 ` Claudio Fontana 0 siblings, 1 reply; 22+ messages in thread From: Richard Henderson @ 2022-09-08 16:03 UTC (permalink / raw) To: Claudio Fontana, Paolo Bonzini, Markus Armbruster Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé, Kevin Wolf On 9/8/22 15:53, Claudio Fontana wrote: > @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, > return -EINVAL; > } > > - block_module_load_one("dmg-bz2"); > - block_module_load_one("dmg-lzfse"); > + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { > + error_report_err(local_err); > + } > + local_err = NULL; > + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { > + error_report_err(local_err); > + } > > s->n_chunks = 0; > s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; I wonder if these shouldn't fail hard if the modules don't exist? Or at least pass back the error. Kevin? > @@ -1033,7 +1039,10 @@ 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); > + Error *local_err = NULL; > + if (!module_load_qom_one(typename, &local_err) && local_err) { > + error_report_err(local_err); > + } You can return NULL from this path, we know it failed. > @@ -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"); With error_append_hint, you add the newline. The rest of the util/module.c reorg looks good. r~ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-08 16:03 ` Richard Henderson @ 2022-09-08 17:10 ` Claudio Fontana 2022-09-08 17:36 ` Claudio Fontana 0 siblings, 1 reply; 22+ messages in thread From: Claudio Fontana @ 2022-09-08 17:10 UTC (permalink / raw) To: Richard Henderson, Paolo Bonzini, Markus Armbruster Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé, Kevin Wolf On 9/8/22 18:03, Richard Henderson wrote: > On 9/8/22 15:53, Claudio Fontana wrote: >> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, >> return -EINVAL; >> } >> >> - block_module_load_one("dmg-bz2"); >> - block_module_load_one("dmg-lzfse"); >> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { >> + error_report_err(local_err); >> + } >> + local_err = NULL; >> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { >> + error_report_err(local_err); >> + } >> >> s->n_chunks = 0; >> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; > > I wonder if these shouldn't fail hard if the modules don't exist? > Or at least pass back the error. > > Kevin? > >> @@ -1033,7 +1039,10 @@ 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); >> + Error *local_err = NULL; >> + if (!module_load_qom_one(typename, &local_err) && local_err) { >> + error_report_err(local_err); >> + } > > You can return NULL from this path, we know it failed. I am tempted then to change also other similar instances in the code. > >> @@ -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"); > > With error_append_hint, you add the newline. > > The rest of the util/module.c reorg looks good. > > > r~ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-08 17:10 ` Claudio Fontana @ 2022-09-08 17:36 ` Claudio Fontana 2022-09-20 16:50 ` Kevin Wolf 0 siblings, 1 reply; 22+ messages in thread From: Claudio Fontana @ 2022-09-08 17:36 UTC (permalink / raw) To: Richard Henderson, Paolo Bonzini, Markus Armbruster, Kevin Wolf Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé On 9/8/22 19:10, Claudio Fontana wrote: > On 9/8/22 18:03, Richard Henderson wrote: >> On 9/8/22 15:53, Claudio Fontana wrote: >>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, >>> return -EINVAL; >>> } >>> >>> - block_module_load_one("dmg-bz2"); >>> - block_module_load_one("dmg-lzfse"); >>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { >>> + error_report_err(local_err); >>> + } >>> + local_err = NULL; >>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { >>> + error_report_err(local_err); >>> + } >>> >>> s->n_chunks = 0; >>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; >> >> I wonder if these shouldn't fail hard if the modules don't exist? >> Or at least pass back the error. >> >> Kevin? is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg image is not compressed, "dmg" can function even if the extra dmg-bz module is not loaded right? I'd suspect we should then do: if (!block_module_load_one("dmg-bz2", &local_err)) { if (local_err) { error_report_err(local_err); return -EINVAL; } warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ } and same for dmg-lzfse...? Ciao, C >>> + error_report_err(local_err); >>> + } >> >>> @@ -1033,7 +1039,10 @@ 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); >>> + Error *local_err = NULL; >>> + if (!module_load_qom_one(typename, &local_err) && local_err) { >>> + error_report_err(local_err); >>> + } >> >> You can return NULL from this path, we know it failed. > > > I am tempted then to change also other similar instances in the code. > >> >>> @@ -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"); >> >> With error_append_hint, you add the newline. >> >> The rest of the util/module.c reorg looks good. >> >> >> r~ > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-08 17:36 ` Claudio Fontana @ 2022-09-20 16:50 ` Kevin Wolf 2022-09-21 4:45 ` Markus Armbruster 2022-09-21 7:50 ` Claudio Fontana 0 siblings, 2 replies; 22+ messages in thread From: Kevin Wolf @ 2022-09-20 16:50 UTC (permalink / raw) To: Claudio Fontana Cc: Richard Henderson, Paolo Bonzini, Markus Armbruster, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: > On 9/8/22 19:10, Claudio Fontana wrote: > > On 9/8/22 18:03, Richard Henderson wrote: > >> On 9/8/22 15:53, Claudio Fontana wrote: > >>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, > >>> return -EINVAL; > >>> } > >>> > >>> - block_module_load_one("dmg-bz2"); > >>> - block_module_load_one("dmg-lzfse"); > >>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { > >>> + error_report_err(local_err); > >>> + } > >>> + local_err = NULL; > >>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { > >>> + error_report_err(local_err); > >>> + } > >>> > >>> s->n_chunks = 0; > >>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; > >> > >> I wonder if these shouldn't fail hard if the modules don't exist? > >> Or at least pass back the error. > >> > >> Kevin? > > is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg > image is not compressed, "dmg" can function even if the extra dmg-bz > module is not loaded right? Indeed. The code seems to consider that the modules may not be present. The behaviour in these cases is questionable (it seems to silently leave the buffers as they are and return success), but the modules are clearly optional. > I'd suspect we should then do: > > if (!block_module_load_one("dmg-bz2", &local_err)) { > if (local_err) { > error_report_err(local_err); > return -EINVAL; > } > warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ > } > > and same for dmg-lzfse...? Actually, I think during initialisation, we should just pass NULL as errp and ignore any errors. When a request would access a block that can't be uncompressed because of the missing module, that's where we can have a warn_report_once() and arguably should fail the I/O request. Kevin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-20 16:50 ` Kevin Wolf @ 2022-09-21 4:45 ` Markus Armbruster 2022-09-21 11:43 ` Kevin Wolf 2022-09-21 7:50 ` Claudio Fontana 1 sibling, 1 reply; 22+ messages in thread From: Markus Armbruster @ 2022-09-21 4:45 UTC (permalink / raw) To: Kevin Wolf Cc: Claudio Fontana, Richard Henderson, Paolo Bonzini, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé Kevin Wolf <kwolf@redhat.com> writes: > Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: >> On 9/8/22 19:10, Claudio Fontana wrote: >> > On 9/8/22 18:03, Richard Henderson wrote: >> >> On 9/8/22 15:53, Claudio Fontana wrote: >> >>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, >> >>> return -EINVAL; >> >>> } >> >>> >> >>> - block_module_load_one("dmg-bz2"); >> >>> - block_module_load_one("dmg-lzfse"); >> >>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { >> >>> + error_report_err(local_err); >> >>> + } >> >>> + local_err = NULL; >> >>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { >> >>> + error_report_err(local_err); >> >>> + } >> >>> >> >>> s->n_chunks = 0; >> >>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; >> >> >> >> I wonder if these shouldn't fail hard if the modules don't exist? >> >> Or at least pass back the error. >> >> >> >> Kevin? >> >> is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg >> image is not compressed, "dmg" can function even if the extra dmg-bz >> module is not loaded right? > > Indeed. The code seems to consider that the modules may not be present. > The behaviour in these cases is questionable (it seems to silently leave > the buffers as they are and return success), but the modules are clearly > optional. > >> I'd suspect we should then do: >> >> if (!block_module_load_one("dmg-bz2", &local_err)) { >> if (local_err) { >> error_report_err(local_err); >> return -EINVAL; >> } >> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ >> } >> >> and same for dmg-lzfse...? > > Actually, I think during initialisation, we should just pass NULL as > errp and ignore any errors. > > When a request would access a block that can't be uncompressed because > of the missing module, that's where we can have a warn_report_once() and > arguably should fail the I/O request. Seems like asking for data corruption. To avoid it, the complete stack needs to handle I/O errors correctly. Can we detect presence of compressed blocks on open? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-21 4:45 ` Markus Armbruster @ 2022-09-21 11:43 ` Kevin Wolf 2022-09-21 12:08 ` Markus Armbruster 0 siblings, 1 reply; 22+ messages in thread From: Kevin Wolf @ 2022-09-21 11:43 UTC (permalink / raw) To: Markus Armbruster Cc: Claudio Fontana, Richard Henderson, Paolo Bonzini, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé Am 21.09.2022 um 06:45 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: > >> On 9/8/22 19:10, Claudio Fontana wrote: > >> > On 9/8/22 18:03, Richard Henderson wrote: > >> >> On 9/8/22 15:53, Claudio Fontana wrote: > >> >>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, > >> >>> return -EINVAL; > >> >>> } > >> >>> > >> >>> - block_module_load_one("dmg-bz2"); > >> >>> - block_module_load_one("dmg-lzfse"); > >> >>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { > >> >>> + error_report_err(local_err); > >> >>> + } > >> >>> + local_err = NULL; > >> >>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { > >> >>> + error_report_err(local_err); > >> >>> + } > >> >>> > >> >>> s->n_chunks = 0; > >> >>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; > >> >> > >> >> I wonder if these shouldn't fail hard if the modules don't exist? > >> >> Or at least pass back the error. > >> >> > >> >> Kevin? > >> > >> is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg > >> image is not compressed, "dmg" can function even if the extra dmg-bz > >> module is not loaded right? > > > > Indeed. The code seems to consider that the modules may not be present. > > The behaviour in these cases is questionable (it seems to silently leave > > the buffers as they are and return success), but the modules are clearly > > optional. > > > >> I'd suspect we should then do: > >> > >> if (!block_module_load_one("dmg-bz2", &local_err)) { > >> if (local_err) { > >> error_report_err(local_err); > >> return -EINVAL; > >> } > >> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ > >> } > >> > >> and same for dmg-lzfse...? > > > > Actually, I think during initialisation, we should just pass NULL as > > errp and ignore any errors. > > > > When a request would access a block that can't be uncompressed because > > of the missing module, that's where we can have a warn_report_once() and > > arguably should fail the I/O request. > > Seems like asking for data corruption. To avoid it, the complete stack > needs to handle I/O errors correctly. If you have any component that doesn't handle I/O errors correctly, keep it far away from your data because it _will_ cause corruption eventually. The earlier it fails, the better for you. I don't think we should put great effort into making fundamentally broken software a little bit less broken in the corner case that you're least likely to hit. > Can we detect presence of compressed blocks on open? We seem to read in the full metadata of the image in dmg_open(). So I think it would be possible to detect it there. dmg_read_mish_block() is what fills in s->types. However, it never fills in types that it doesn't know (and it pretends it doesn't know the types of compressed blocks whose module is not loaded). So instead of checking it in dmg_open() after dmg_read_mish_block() has completed, you would have to catch the situation already in dmg_read_mish_block() while parsing the image file, which should be entirely doable if you want. This is a change in dmg's behaviour, though, which is not the goal of the proposed patch. So if we want to do that, it should be a separate patch. Kevin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-21 11:43 ` Kevin Wolf @ 2022-09-21 12:08 ` Markus Armbruster 2022-09-22 14:33 ` Kevin Wolf 0 siblings, 1 reply; 22+ messages in thread From: Markus Armbruster @ 2022-09-21 12:08 UTC (permalink / raw) To: Kevin Wolf Cc: Claudio Fontana, Richard Henderson, Paolo Bonzini, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé Kevin Wolf <kwolf@redhat.com> writes: > Am 21.09.2022 um 06:45 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: >> >> On 9/8/22 19:10, Claudio Fontana wrote: >> >> > On 9/8/22 18:03, Richard Henderson wrote: >> >> >> On 9/8/22 15:53, Claudio Fontana wrote: >> >> >>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, >> >> >>> return -EINVAL; >> >> >>> } >> >> >>> >> >> >>> - block_module_load_one("dmg-bz2"); >> >> >>> - block_module_load_one("dmg-lzfse"); >> >> >>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { >> >> >>> + error_report_err(local_err); >> >> >>> + } >> >> >>> + local_err = NULL; >> >> >>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { >> >> >>> + error_report_err(local_err); >> >> >>> + } >> >> >>> >> >> >>> s->n_chunks = 0; >> >> >>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; >> >> >> >> >> >> I wonder if these shouldn't fail hard if the modules don't exist? >> >> >> Or at least pass back the error. >> >> >> >> >> >> Kevin? >> >> >> >> is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg >> >> image is not compressed, "dmg" can function even if the extra dmg-bz >> >> module is not loaded right? >> > >> > Indeed. The code seems to consider that the modules may not be present. >> > The behaviour in these cases is questionable (it seems to silently leave >> > the buffers as they are and return success), but the modules are clearly >> > optional. >> > >> >> I'd suspect we should then do: >> >> >> >> if (!block_module_load_one("dmg-bz2", &local_err)) { >> >> if (local_err) { >> >> error_report_err(local_err); >> >> return -EINVAL; >> >> } >> >> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ >> >> } >> >> >> >> and same for dmg-lzfse...? >> > >> > Actually, I think during initialisation, we should just pass NULL as >> > errp and ignore any errors. >> > >> > When a request would access a block that can't be uncompressed because >> > of the missing module, that's where we can have a warn_report_once() and >> > arguably should fail the I/O request. >> >> Seems like asking for data corruption. To avoid it, the complete stack >> needs to handle I/O errors correctly. > > If you have any component that doesn't handle I/O errors correctly, keep > it far away from your data because it _will_ cause corruption eventually. > The earlier it fails, the better for you. True for immortals. Since we're not, it actually depends on probabilities. > I don't think we should put great effort into making fundamentally > broken software a little bit less broken in the corner case that you're > least likely to hit. Fair point. >> Can we detect presence of compressed blocks on open? > > We seem to read in the full metadata of the image in dmg_open(). So I > think it would be possible to detect it there. > > dmg_read_mish_block() is what fills in s->types. However, it never fills > in types that it doesn't know (and it pretends it doesn't know the types > of compressed blocks whose module is not loaded). So instead of checking > it in dmg_open() after dmg_read_mish_block() has completed, you would > have to catch the situation already in dmg_read_mish_block() while > parsing the image file, which should be entirely doable if you want. In most cases, "open fails because some blocks are known to be unreadable" is much better UX than "everything goes swimmingly until you try to read one of the known-unreadable blocks". Even when your software manages not to eat your data, surprise bad blocks are still likely to result in a bad day. > This is a change in dmg's behaviour, though, which is not the goal of > the proposed patch. So if we want to do that, it should be a separate > patch. Makes sense. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-21 12:08 ` Markus Armbruster @ 2022-09-22 14:33 ` Kevin Wolf 2022-09-22 15:09 ` Markus Armbruster 0 siblings, 1 reply; 22+ messages in thread From: Kevin Wolf @ 2022-09-22 14:33 UTC (permalink / raw) To: Markus Armbruster Cc: Claudio Fontana, Richard Henderson, Paolo Bonzini, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé Am 21.09.2022 um 14:08 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 21.09.2022 um 06:45 hat Markus Armbruster geschrieben: > >> Can we detect presence of compressed blocks on open? > > > > We seem to read in the full metadata of the image in dmg_open(). So I > > think it would be possible to detect it there. > > > > dmg_read_mish_block() is what fills in s->types. However, it never fills > > in types that it doesn't know (and it pretends it doesn't know the types > > of compressed blocks whose module is not loaded). So instead of checking > > it in dmg_open() after dmg_read_mish_block() has completed, you would > > have to catch the situation already in dmg_read_mish_block() while > > parsing the image file, which should be entirely doable if you want. > > In most cases, "open fails because some blocks are known to be > unreadable" is much better UX than "everything goes swimmingly until you > try to read one of the known-unreadable blocks". > > Even when your software manages not to eat your data, surprise bad > blocks are still likely to result in a bad day. That's fair. On the other hand, not allowing the user to read the part of data that is perfectly readable would be bad, too. Maybe the right solution would be to have a driver option like "unknown-block-types=io-error|fail-open" (probably with better names), and then having "fail-open" as the new default would be reasonable enough. Kevin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-22 14:33 ` Kevin Wolf @ 2022-09-22 15:09 ` Markus Armbruster 0 siblings, 0 replies; 22+ messages in thread From: Markus Armbruster @ 2022-09-22 15:09 UTC (permalink / raw) To: Kevin Wolf Cc: Claudio Fontana, Richard Henderson, Paolo Bonzini, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé Kevin Wolf <kwolf@redhat.com> writes: > Am 21.09.2022 um 14:08 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@redhat.com> writes: >> >> > Am 21.09.2022 um 06:45 hat Markus Armbruster geschrieben: >> >> Can we detect presence of compressed blocks on open? >> > >> > We seem to read in the full metadata of the image in dmg_open(). So I >> > think it would be possible to detect it there. >> > >> > dmg_read_mish_block() is what fills in s->types. However, it never fills >> > in types that it doesn't know (and it pretends it doesn't know the types >> > of compressed blocks whose module is not loaded). So instead of checking >> > it in dmg_open() after dmg_read_mish_block() has completed, you would >> > have to catch the situation already in dmg_read_mish_block() while >> > parsing the image file, which should be entirely doable if you want. >> >> In most cases, "open fails because some blocks are known to be >> unreadable" is much better UX than "everything goes swimmingly until you >> try to read one of the known-unreadable blocks". >> >> Even when your software manages not to eat your data, surprise bad >> blocks are still likely to result in a bad day. > > That's fair. On the other hand, not allowing the user to read the part > of data that is perfectly readable would be bad, too. > > Maybe the right solution would be to have a driver option like > "unknown-block-types=io-error|fail-open" (probably with better names), > and then having "fail-open" as the new default would be reasonable > enough. Makes sense. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-20 16:50 ` Kevin Wolf 2022-09-21 4:45 ` Markus Armbruster @ 2022-09-21 7:50 ` Claudio Fontana 2022-09-21 11:56 ` Kevin Wolf 1 sibling, 1 reply; 22+ messages in thread From: Claudio Fontana @ 2022-09-21 7:50 UTC (permalink / raw) To: Kevin Wolf Cc: Richard Henderson, Paolo Bonzini, Markus Armbruster, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P. Berrangé, Philippe Mathieu-Daudé On 9/20/22 18:50, Kevin Wolf wrote: > Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: >> On 9/8/22 19:10, Claudio Fontana wrote: >>> On 9/8/22 18:03, Richard Henderson wrote: >>>> On 9/8/22 15:53, Claudio Fontana wrote: >>>>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, >>>>> return -EINVAL; >>>>> } >>>>> >>>>> - block_module_load_one("dmg-bz2"); >>>>> - block_module_load_one("dmg-lzfse"); >>>>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { >>>>> + error_report_err(local_err); >>>>> + } >>>>> + local_err = NULL; >>>>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { >>>>> + error_report_err(local_err); >>>>> + } >>>>> >>>>> s->n_chunks = 0; >>>>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; >>>> >>>> I wonder if these shouldn't fail hard if the modules don't exist? >>>> Or at least pass back the error. >>>> >>>> Kevin? >> >> is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg >> image is not compressed, "dmg" can function even if the extra dmg-bz >> module is not loaded right? > > Indeed. The code seems to consider that the modules may not be present. > The behaviour in these cases is questionable (it seems to silently leave > the buffers as they are and return success), but the modules are clearly > optional. > >> I'd suspect we should then do: >> >> if (!block_module_load_one("dmg-bz2", &local_err)) { >> if (local_err) { >> error_report_err(local_err); >> return -EINVAL; >> } >> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ >> } >> >> and same for dmg-lzfse...? > > Actually, I think during initialisation, we should just pass NULL as > errp and ignore any errors. Hmm really? I'd think that if there is an actual error loading the module (module is installed, but the loading itself fails due to broken module, wrong permissions, I/O errors etc) we would want to report that fact as it happens? > > When a request would access a block that can't be uncompressed because > of the missing module, that's where we can have a warn_report_once() and > arguably should fail the I/O request. > > Kevin > That would mean, moving the warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") to the uncompression code and change it to a warn_report_once() right? Thanks, Claudio ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-21 7:50 ` Claudio Fontana @ 2022-09-21 11:56 ` Kevin Wolf 2022-09-23 14:10 ` Claudio Fontana 0 siblings, 1 reply; 22+ messages in thread From: Kevin Wolf @ 2022-09-21 11:56 UTC (permalink / raw) To: Claudio Fontana Cc: Richard Henderson, Paolo Bonzini, Markus Armbruster, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P. Berrangé, Philippe Mathieu-Daudé Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: > On 9/20/22 18:50, Kevin Wolf wrote: > > Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: > >> On 9/8/22 19:10, Claudio Fontana wrote: > >>> On 9/8/22 18:03, Richard Henderson wrote: > >>>> On 9/8/22 15:53, Claudio Fontana wrote: > >>>>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, > >>>>> return -EINVAL; > >>>>> } > >>>>> > >>>>> - block_module_load_one("dmg-bz2"); > >>>>> - block_module_load_one("dmg-lzfse"); > >>>>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { > >>>>> + error_report_err(local_err); > >>>>> + } > >>>>> + local_err = NULL; > >>>>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { > >>>>> + error_report_err(local_err); > >>>>> + } > >>>>> > >>>>> s->n_chunks = 0; > >>>>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; > >>>> > >>>> I wonder if these shouldn't fail hard if the modules don't exist? > >>>> Or at least pass back the error. > >>>> > >>>> Kevin? > >> > >> is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg > >> image is not compressed, "dmg" can function even if the extra dmg-bz > >> module is not loaded right? > > > > Indeed. The code seems to consider that the modules may not be present. > > The behaviour in these cases is questionable (it seems to silently leave > > the buffers as they are and return success) I think I misunderstood the code here actually. dmg_read_mish_block() skips chunks of unknown type, so later trying to find them fails and dmg_co_preadv() returns -EIO. Which is a reasonable return value for this. > > , but the modules are clearly > > optional. > > > >> I'd suspect we should then do: > >> > >> if (!block_module_load_one("dmg-bz2", &local_err)) { > >> if (local_err) { > >> error_report_err(local_err); > >> return -EINVAL; > >> } > >> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ > >> } > >> > >> and same for dmg-lzfse...? > > > > Actually, I think during initialisation, we should just pass NULL as > > errp and ignore any errors. > > Hmm really? I'd think that if there is an actual error loading the > module (module is installed, but the loading itself fails due to > broken module, wrong permissions, I/O errors etc) we would want to > report that fact as it happens? Can we distinguish the two error cases? Oooh... Reading the code again carefully, are you returning false without setting errp if the module just couldn't be found? This is a surprising interface. Yes, I guess then your proposed code is fine (modulo moving warn_report() somewhere else so that it doesn't complain when the image doesn't even contain compressed chunks). > > When a request would access a block that can't be uncompressed because > > of the missing module, that's where we can have a warn_report_once() and > > arguably should fail the I/O request. > > > > Kevin > > > > That would mean, moving the > > warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") > > to the uncompression code and change it to a warn_report_once() right? Yeah, though I think this doesn't actually work because we never even stored the metadata for chunks of unknown type (see above), so we never reach the uncompression code. What misled me initially is this code in dmg_read_chunk(): case UDBZ: /* bzip2 compressed */ if (!dmg_uncompress_bz2) { break; } I believe this is dead code, it could actually be an assertion. So if I'm not missing anything, adding the warning there would be useless. The other option is moving it into dmg_is_known_block_type() or its caller dmg_read_mish_block(), then we would detect it during open, which is probably nicer anyway. Kevin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-21 11:56 ` Kevin Wolf @ 2022-09-23 14:10 ` Claudio Fontana 2022-09-23 14:42 ` Kevin Wolf 0 siblings, 1 reply; 22+ messages in thread From: Claudio Fontana @ 2022-09-23 14:10 UTC (permalink / raw) To: Kevin Wolf Cc: Richard Henderson, Paolo Bonzini, Markus Armbruster, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P. Berrangé, Philippe Mathieu-Daudé On 9/21/22 13:56, Kevin Wolf wrote: > Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: >> On 9/20/22 18:50, Kevin Wolf wrote: >>> Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: >>>> On 9/8/22 19:10, Claudio Fontana wrote: >>>>> On 9/8/22 18:03, Richard Henderson wrote: >>>>>> On 9/8/22 15:53, Claudio Fontana wrote: >>>>>>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, >>>>>>> return -EINVAL; >>>>>>> } >>>>>>> >>>>>>> - block_module_load_one("dmg-bz2"); >>>>>>> - block_module_load_one("dmg-lzfse"); >>>>>>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { >>>>>>> + error_report_err(local_err); >>>>>>> + } >>>>>>> + local_err = NULL; >>>>>>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { >>>>>>> + error_report_err(local_err); >>>>>>> + } >>>>>>> >>>>>>> s->n_chunks = 0; >>>>>>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; >>>>>> >>>>>> I wonder if these shouldn't fail hard if the modules don't exist? >>>>>> Or at least pass back the error. >>>>>> >>>>>> Kevin? >>>> >>>> is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg >>>> image is not compressed, "dmg" can function even if the extra dmg-bz >>>> module is not loaded right? >>> >>> Indeed. The code seems to consider that the modules may not be present. >>> The behaviour in these cases is questionable (it seems to silently leave >>> the buffers as they are and return success) > > I think I misunderstood the code here actually. dmg_read_mish_block() > skips chunks of unknown type, so later trying to find them fails and > dmg_co_preadv() returns -EIO. Which is a reasonable return value for > this. > >>> , but the modules are clearly >>> optional. >>> >>>> I'd suspect we should then do: >>>> >>>> if (!block_module_load_one("dmg-bz2", &local_err)) { >>>> if (local_err) { >>>> error_report_err(local_err); >>>> return -EINVAL; >>>> } >>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ >>>> } >>>> >>>> and same for dmg-lzfse...? >>> >>> Actually, I think during initialisation, we should just pass NULL as >>> errp and ignore any errors. >> >> Hmm really? I'd think that if there is an actual error loading the >> module (module is installed, but the loading itself fails due to >> broken module, wrong permissions, I/O errors etc) we would want to >> report that fact as it happens? > > Can we distinguish the two error cases? > > Oooh... Reading the code again carefully, are you returning false > without setting errp if the module just couldn't be found? This is a > surprising interface. > > Yes, I guess then your proposed code is fine (modulo moving > warn_report() somewhere else so that it doesn't complain when the image > doesn't even contain compressed chunks). > >>> When a request would access a block that can't be uncompressed because >>> of the missing module, that's where we can have a warn_report_once() and >>> arguably should fail the I/O request. >>> >>> Kevin >>> >> >> That would mean, moving the >> >> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") >> >> to the uncompression code and change it to a warn_report_once() right? > > Yeah, though I think this doesn't actually work because we never even > stored the metadata for chunks of unknown type (see above), so we never > reach the uncompression code. > > What misled me initially is this code in dmg_read_chunk(): > > case UDBZ: /* bzip2 compressed */ > if (!dmg_uncompress_bz2) { > break; > } > > I believe this is dead code, it could actually be an assertion. So > if I'm not missing anything, adding the warning there would be useless. > > The other option is moving it into dmg_is_known_block_type() or its > caller dmg_read_mish_block(), then we would detect it during open, which > is probably nicer anyway. > > Kevin > > Hi Kevin, I got a bit lost on whether we have some agreement on where if anywhere to move the check/warning about missing decompression submodules. If that's ok I'd post a V5, and we can rediscuss from the new starting point? Thanks, Claudio ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-23 14:10 ` Claudio Fontana @ 2022-09-23 14:42 ` Kevin Wolf 2022-09-23 14:46 ` Claudio Fontana 0 siblings, 1 reply; 22+ messages in thread From: Kevin Wolf @ 2022-09-23 14:42 UTC (permalink / raw) To: Claudio Fontana Cc: Richard Henderson, Paolo Bonzini, Markus Armbruster, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P. Berrangé, Philippe Mathieu-Daudé Am 23.09.2022 um 16:10 hat Claudio Fontana geschrieben: > On 9/21/22 13:56, Kevin Wolf wrote: > > Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: > >> On 9/20/22 18:50, Kevin Wolf wrote: > >>> Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: > >>>> On 9/8/22 19:10, Claudio Fontana wrote: > >>>>> On 9/8/22 18:03, Richard Henderson wrote: > >>>>>> On 9/8/22 15:53, Claudio Fontana wrote: > >>>>>>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, > >>>>>>> return -EINVAL; > >>>>>>> } > >>>>>>> > >>>>>>> - block_module_load_one("dmg-bz2"); > >>>>>>> - block_module_load_one("dmg-lzfse"); > >>>>>>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { > >>>>>>> + error_report_err(local_err); > >>>>>>> + } > >>>>>>> + local_err = NULL; > >>>>>>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { > >>>>>>> + error_report_err(local_err); > >>>>>>> + } > >>>>>>> > >>>>>>> s->n_chunks = 0; > >>>>>>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; > >>>>>> > >>>>>> I wonder if these shouldn't fail hard if the modules don't exist? > >>>>>> Or at least pass back the error. > >>>>>> > >>>>>> Kevin? > >>>> > >>>> is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg > >>>> image is not compressed, "dmg" can function even if the extra dmg-bz > >>>> module is not loaded right? > >>> > >>> Indeed. The code seems to consider that the modules may not be present. > >>> The behaviour in these cases is questionable (it seems to silently leave > >>> the buffers as they are and return success) > > > > I think I misunderstood the code here actually. dmg_read_mish_block() > > skips chunks of unknown type, so later trying to find them fails and > > dmg_co_preadv() returns -EIO. Which is a reasonable return value for > > this. > > > >>> , but the modules are clearly > >>> optional. > >>> > >>>> I'd suspect we should then do: > >>>> > >>>> if (!block_module_load_one("dmg-bz2", &local_err)) { > >>>> if (local_err) { > >>>> error_report_err(local_err); > >>>> return -EINVAL; > >>>> } > >>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ > >>>> } > >>>> > >>>> and same for dmg-lzfse...? > >>> > >>> Actually, I think during initialisation, we should just pass NULL as > >>> errp and ignore any errors. > >> > >> Hmm really? I'd think that if there is an actual error loading the > >> module (module is installed, but the loading itself fails due to > >> broken module, wrong permissions, I/O errors etc) we would want to > >> report that fact as it happens? > > > > Can we distinguish the two error cases? > > > > Oooh... Reading the code again carefully, are you returning false > > without setting errp if the module just couldn't be found? This is a > > surprising interface. > > > > Yes, I guess then your proposed code is fine (modulo moving > > warn_report() somewhere else so that it doesn't complain when the image > > doesn't even contain compressed chunks). > > > >>> When a request would access a block that can't be uncompressed because > >>> of the missing module, that's where we can have a warn_report_once() and > >>> arguably should fail the I/O request. > >>> > >>> Kevin > >>> > >> > >> That would mean, moving the > >> > >> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") > >> > >> to the uncompression code and change it to a warn_report_once() right? > > > > Yeah, though I think this doesn't actually work because we never even > > stored the metadata for chunks of unknown type (see above), so we never > > reach the uncompression code. > > > > What misled me initially is this code in dmg_read_chunk(): > > > > case UDBZ: /* bzip2 compressed */ > > if (!dmg_uncompress_bz2) { > > break; > > } > > > > I believe this is dead code, it could actually be an assertion. So > > if I'm not missing anything, adding the warning there would be useless. > > > > The other option is moving it into dmg_is_known_block_type() or its > > caller dmg_read_mish_block(), then we would detect it during open, which > > is probably nicer anyway. > > > > Kevin > > > > > > Hi Kevin, I got a bit lost on whether we have some agreement on where > if anywhere to move the check/warning about missing decompression > submodules. > > If that's ok I'd post a V5, and we can rediscuss from the new starting > point? Sure, feel free, though I don't think the code will otherwise change for dmg, so we could as well continue here. My conclusion was that only dmg_read_mish_block() or something called by it can know whether compressed blocks exist in the image when the modules aren't present. So if we want to make the warning conditional on that (and my understanding is correct), this is where a warn_report_once() would have to live. Kevin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-23 14:42 ` Kevin Wolf @ 2022-09-23 14:46 ` Claudio Fontana 2022-09-23 16:29 ` Kevin Wolf 0 siblings, 1 reply; 22+ messages in thread From: Claudio Fontana @ 2022-09-23 14:46 UTC (permalink / raw) To: Kevin Wolf Cc: Richard Henderson, Paolo Bonzini, Markus Armbruster, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P. Berrangé, Philippe Mathieu-Daudé On 9/23/22 16:42, Kevin Wolf wrote: > Am 23.09.2022 um 16:10 hat Claudio Fontana geschrieben: >> On 9/21/22 13:56, Kevin Wolf wrote: >>> Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: >>>> On 9/20/22 18:50, Kevin Wolf wrote: >>>>> Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: >>>>>> On 9/8/22 19:10, Claudio Fontana wrote: >>>>>>> On 9/8/22 18:03, Richard Henderson wrote: >>>>>>>> On 9/8/22 15:53, Claudio Fontana wrote: >>>>>>>>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, >>>>>>>>> return -EINVAL; >>>>>>>>> } >>>>>>>>> >>>>>>>>> - block_module_load_one("dmg-bz2"); >>>>>>>>> - block_module_load_one("dmg-lzfse"); >>>>>>>>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { >>>>>>>>> + error_report_err(local_err); >>>>>>>>> + } >>>>>>>>> + local_err = NULL; >>>>>>>>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { >>>>>>>>> + error_report_err(local_err); >>>>>>>>> + } >>>>>>>>> >>>>>>>>> s->n_chunks = 0; >>>>>>>>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; >>>>>>>> >>>>>>>> I wonder if these shouldn't fail hard if the modules don't exist? >>>>>>>> Or at least pass back the error. >>>>>>>> >>>>>>>> Kevin? >>>>>> >>>>>> is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg >>>>>> image is not compressed, "dmg" can function even if the extra dmg-bz >>>>>> module is not loaded right? >>>>> >>>>> Indeed. The code seems to consider that the modules may not be present. >>>>> The behaviour in these cases is questionable (it seems to silently leave >>>>> the buffers as they are and return success) >>> >>> I think I misunderstood the code here actually. dmg_read_mish_block() >>> skips chunks of unknown type, so later trying to find them fails and >>> dmg_co_preadv() returns -EIO. Which is a reasonable return value for >>> this. >>> >>>>> , but the modules are clearly >>>>> optional. >>>>> >>>>>> I'd suspect we should then do: >>>>>> >>>>>> if (!block_module_load_one("dmg-bz2", &local_err)) { >>>>>> if (local_err) { >>>>>> error_report_err(local_err); >>>>>> return -EINVAL; >>>>>> } >>>>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ >>>>>> } >>>>>> >>>>>> and same for dmg-lzfse...? >>>>> >>>>> Actually, I think during initialisation, we should just pass NULL as >>>>> errp and ignore any errors. >>>> >>>> Hmm really? I'd think that if there is an actual error loading the >>>> module (module is installed, but the loading itself fails due to >>>> broken module, wrong permissions, I/O errors etc) we would want to >>>> report that fact as it happens? >>> >>> Can we distinguish the two error cases? >>> >>> Oooh... Reading the code again carefully, are you returning false >>> without setting errp if the module just couldn't be found? This is a >>> surprising interface. >>> >>> Yes, I guess then your proposed code is fine (modulo moving >>> warn_report() somewhere else so that it doesn't complain when the image >>> doesn't even contain compressed chunks). >>> >>>>> When a request would access a block that can't be uncompressed because >>>>> of the missing module, that's where we can have a warn_report_once() and >>>>> arguably should fail the I/O request. >>>>> >>>>> Kevin >>>>> >>>> >>>> That would mean, moving the >>>> >>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") >>>> >>>> to the uncompression code and change it to a warn_report_once() right? >>> >>> Yeah, though I think this doesn't actually work because we never even >>> stored the metadata for chunks of unknown type (see above), so we never >>> reach the uncompression code. >>> >>> What misled me initially is this code in dmg_read_chunk(): >>> >>> case UDBZ: /* bzip2 compressed */ >>> if (!dmg_uncompress_bz2) { >>> break; >>> } >>> >>> I believe this is dead code, it could actually be an assertion. So >>> if I'm not missing anything, adding the warning there would be useless. >>> >>> The other option is moving it into dmg_is_known_block_type() or its >>> caller dmg_read_mish_block(), then we would detect it during open, which >>> is probably nicer anyway. >>> >>> Kevin >>> >>> >> >> Hi Kevin, I got a bit lost on whether we have some agreement on where >> if anywhere to move the check/warning about missing decompression >> submodules. >> >> If that's ok I'd post a V5, and we can rediscuss from the new starting >> point? > > Sure, feel free, though I don't think the code will otherwise change for > dmg, so we could as well continue here. > > My conclusion was that only dmg_read_mish_block() or something called by > it can know whether compressed blocks exist in the image when the > modules aren't present. So if we want to make the warning conditional on > that (and my understanding is correct), this is where a > warn_report_once() would have to live. > > Kevin > I took a look, but I feel a bit too ignorant of the code there, maybe you could move the warning as a patch to the right place after the series? Or give me the extra commit needed to move into the right place? Ciao, Claudio ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-23 14:46 ` Claudio Fontana @ 2022-09-23 16:29 ` Kevin Wolf 2022-09-23 22:23 ` Claudio Fontana 0 siblings, 1 reply; 22+ messages in thread From: Kevin Wolf @ 2022-09-23 16:29 UTC (permalink / raw) To: Claudio Fontana Cc: Richard Henderson, Paolo Bonzini, Markus Armbruster, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P. Berrangé, Philippe Mathieu-Daudé Am 23.09.2022 um 16:46 hat Claudio Fontana geschrieben: > On 9/23/22 16:42, Kevin Wolf wrote: > > Am 23.09.2022 um 16:10 hat Claudio Fontana geschrieben: > >> On 9/21/22 13:56, Kevin Wolf wrote: > >>> Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: > >>>> On 9/20/22 18:50, Kevin Wolf wrote: > >>>>> Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: > >>>>>> On 9/8/22 19:10, Claudio Fontana wrote: > >>>>>>> On 9/8/22 18:03, Richard Henderson wrote: > >>>>>>>> On 9/8/22 15:53, Claudio Fontana wrote: > >>>>>>>>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, > >>>>>>>>> return -EINVAL; > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> - block_module_load_one("dmg-bz2"); > >>>>>>>>> - block_module_load_one("dmg-lzfse"); > >>>>>>>>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { > >>>>>>>>> + error_report_err(local_err); > >>>>>>>>> + } > >>>>>>>>> + local_err = NULL; > >>>>>>>>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { > >>>>>>>>> + error_report_err(local_err); > >>>>>>>>> + } > >>>>>>>>> > >>>>>>>>> s->n_chunks = 0; > >>>>>>>>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; > >>>>>>>> > >>>>>>>> I wonder if these shouldn't fail hard if the modules don't exist? > >>>>>>>> Or at least pass back the error. > >>>>>>>> > >>>>>>>> Kevin? > >>>>>> > >>>>>> is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg > >>>>>> image is not compressed, "dmg" can function even if the extra dmg-bz > >>>>>> module is not loaded right? > >>>>> > >>>>> Indeed. The code seems to consider that the modules may not be present. > >>>>> The behaviour in these cases is questionable (it seems to silently leave > >>>>> the buffers as they are and return success) > >>> > >>> I think I misunderstood the code here actually. dmg_read_mish_block() > >>> skips chunks of unknown type, so later trying to find them fails and > >>> dmg_co_preadv() returns -EIO. Which is a reasonable return value for > >>> this. > >>> > >>>>> , but the modules are clearly > >>>>> optional. > >>>>> > >>>>>> I'd suspect we should then do: > >>>>>> > >>>>>> if (!block_module_load_one("dmg-bz2", &local_err)) { > >>>>>> if (local_err) { > >>>>>> error_report_err(local_err); > >>>>>> return -EINVAL; > >>>>>> } > >>>>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ > >>>>>> } > >>>>>> > >>>>>> and same for dmg-lzfse...? > >>>>> > >>>>> Actually, I think during initialisation, we should just pass NULL as > >>>>> errp and ignore any errors. > >>>> > >>>> Hmm really? I'd think that if there is an actual error loading the > >>>> module (module is installed, but the loading itself fails due to > >>>> broken module, wrong permissions, I/O errors etc) we would want to > >>>> report that fact as it happens? > >>> > >>> Can we distinguish the two error cases? > >>> > >>> Oooh... Reading the code again carefully, are you returning false > >>> without setting errp if the module just couldn't be found? This is a > >>> surprising interface. > >>> > >>> Yes, I guess then your proposed code is fine (modulo moving > >>> warn_report() somewhere else so that it doesn't complain when the image > >>> doesn't even contain compressed chunks). > >>> > >>>>> When a request would access a block that can't be uncompressed because > >>>>> of the missing module, that's where we can have a warn_report_once() and > >>>>> arguably should fail the I/O request. > >>>>> > >>>>> Kevin > >>>>> > >>>> > >>>> That would mean, moving the > >>>> > >>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") > >>>> > >>>> to the uncompression code and change it to a warn_report_once() right? > >>> > >>> Yeah, though I think this doesn't actually work because we never even > >>> stored the metadata for chunks of unknown type (see above), so we never > >>> reach the uncompression code. > >>> > >>> What misled me initially is this code in dmg_read_chunk(): > >>> > >>> case UDBZ: /* bzip2 compressed */ > >>> if (!dmg_uncompress_bz2) { > >>> break; > >>> } > >>> > >>> I believe this is dead code, it could actually be an assertion. So > >>> if I'm not missing anything, adding the warning there would be useless. > >>> > >>> The other option is moving it into dmg_is_known_block_type() or its > >>> caller dmg_read_mish_block(), then we would detect it during open, which > >>> is probably nicer anyway. > >>> > >>> Kevin > >>> > >>> > >> > >> Hi Kevin, I got a bit lost on whether we have some agreement on where > >> if anywhere to move the check/warning about missing decompression > >> submodules. > >> > >> If that's ok I'd post a V5, and we can rediscuss from the new starting > >> point? > > > > Sure, feel free, though I don't think the code will otherwise change for > > dmg, so we could as well continue here. > > > > My conclusion was that only dmg_read_mish_block() or something called by > > it can know whether compressed blocks exist in the image when the > > modules aren't present. So if we want to make the warning conditional on > > that (and my understanding is correct), this is where a > > warn_report_once() would have to live. > > > > Kevin > > > > I took a look, but I feel a bit too ignorant of the code there, maybe you could move the warning as a patch to the right place after the series? > > Or give me the extra commit needed to move into the right place? Only built and tested with an uncompressed image, so this could use testing with an actual compressed dmg image and the module present or missing, but something like the following should do the trick. Kevin diff --git a/block/dmg.c b/block/dmg.c index 98db18d82a..630cde3416 100644 --- a/block/dmg.c +++ b/block/dmg.c @@ -254,6 +254,25 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds, for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) { s->types[i] = buff_read_uint32(buffer, offset); if (!dmg_is_known_block_type(s->types[i])) { + switch (s->types[i]) { + case UDBZ: + warn_report_once("dmg-bzip2 module is missing, accessing bzip2 " + "compressed blocks will result in I/O errors"); + break; + case ULFO: + warn_report_once("dmg-lzfse module is missing, accessing lzfse " + "compressed blocks will result in I/O errors"); + break; + case UDCM: + case UDLE: + /* Comments and last entry can be ignored without problems */ + break; + default: + warn_report_once("Image contains chunks of unknown type %x, " + "accessing them will result in I/O errors", + s->types[i]); + break; + } chunk_count--; i--; offset += 40; ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-23 16:29 ` Kevin Wolf @ 2022-09-23 22:23 ` Claudio Fontana 0 siblings, 0 replies; 22+ messages in thread From: Claudio Fontana @ 2022-09-23 22:23 UTC (permalink / raw) To: Kevin Wolf Cc: Richard Henderson, Paolo Bonzini, Markus Armbruster, qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P. Berrangé, Philippe Mathieu-Daudé On 9/23/22 18:29, Kevin Wolf wrote: > Am 23.09.2022 um 16:46 hat Claudio Fontana geschrieben: >> On 9/23/22 16:42, Kevin Wolf wrote: >>> Am 23.09.2022 um 16:10 hat Claudio Fontana geschrieben: >>>> On 9/21/22 13:56, Kevin Wolf wrote: >>>>> Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben: >>>>>> On 9/20/22 18:50, Kevin Wolf wrote: >>>>>>> Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben: >>>>>>>> On 9/8/22 19:10, Claudio Fontana wrote: >>>>>>>>> On 9/8/22 18:03, Richard Henderson wrote: >>>>>>>>>> On 9/8/22 15:53, Claudio Fontana wrote: >>>>>>>>>>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, >>>>>>>>>>> return -EINVAL; >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> - block_module_load_one("dmg-bz2"); >>>>>>>>>>> - block_module_load_one("dmg-lzfse"); >>>>>>>>>>> + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { >>>>>>>>>>> + error_report_err(local_err); >>>>>>>>>>> + } >>>>>>>>>>> + local_err = NULL; >>>>>>>>>>> + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { >>>>>>>>>>> + error_report_err(local_err); >>>>>>>>>>> + } >>>>>>>>>>> >>>>>>>>>>> s->n_chunks = 0; >>>>>>>>>>> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL; >>>>>>>>>> >>>>>>>>>> I wonder if these shouldn't fail hard if the modules don't exist? >>>>>>>>>> Or at least pass back the error. >>>>>>>>>> >>>>>>>>>> Kevin? >>>>>>>> >>>>>>>> is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg >>>>>>>> image is not compressed, "dmg" can function even if the extra dmg-bz >>>>>>>> module is not loaded right? >>>>>>> >>>>>>> Indeed. The code seems to consider that the modules may not be present. >>>>>>> The behaviour in these cases is questionable (it seems to silently leave >>>>>>> the buffers as they are and return success) >>>>> >>>>> I think I misunderstood the code here actually. dmg_read_mish_block() >>>>> skips chunks of unknown type, so later trying to find them fails and >>>>> dmg_co_preadv() returns -EIO. Which is a reasonable return value for >>>>> this. >>>>> >>>>>>> , but the modules are clearly >>>>>>> optional. >>>>>>> >>>>>>>> I'd suspect we should then do: >>>>>>>> >>>>>>>> if (!block_module_load_one("dmg-bz2", &local_err)) { >>>>>>>> if (local_err) { >>>>>>>> error_report_err(local_err); >>>>>>>> return -EINVAL; >>>>>>>> } >>>>>>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */ >>>>>>>> } >>>>>>>> >>>>>>>> and same for dmg-lzfse...? >>>>>>> >>>>>>> Actually, I think during initialisation, we should just pass NULL as >>>>>>> errp and ignore any errors. >>>>>> >>>>>> Hmm really? I'd think that if there is an actual error loading the >>>>>> module (module is installed, but the loading itself fails due to >>>>>> broken module, wrong permissions, I/O errors etc) we would want to >>>>>> report that fact as it happens? >>>>> >>>>> Can we distinguish the two error cases? >>>>> >>>>> Oooh... Reading the code again carefully, are you returning false >>>>> without setting errp if the module just couldn't be found? This is a >>>>> surprising interface. >>>>> >>>>> Yes, I guess then your proposed code is fine (modulo moving >>>>> warn_report() somewhere else so that it doesn't complain when the image >>>>> doesn't even contain compressed chunks). >>>>> >>>>>>> When a request would access a block that can't be uncompressed because >>>>>>> of the missing module, that's where we can have a warn_report_once() and >>>>>>> arguably should fail the I/O request. >>>>>>> >>>>>>> Kevin >>>>>>> >>>>>> >>>>>> That would mean, moving the >>>>>> >>>>>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks") >>>>>> >>>>>> to the uncompression code and change it to a warn_report_once() right? >>>>> >>>>> Yeah, though I think this doesn't actually work because we never even >>>>> stored the metadata for chunks of unknown type (see above), so we never >>>>> reach the uncompression code. >>>>> >>>>> What misled me initially is this code in dmg_read_chunk(): >>>>> >>>>> case UDBZ: /* bzip2 compressed */ >>>>> if (!dmg_uncompress_bz2) { >>>>> break; >>>>> } >>>>> >>>>> I believe this is dead code, it could actually be an assertion. So >>>>> if I'm not missing anything, adding the warning there would be useless. >>>>> >>>>> The other option is moving it into dmg_is_known_block_type() or its >>>>> caller dmg_read_mish_block(), then we would detect it during open, which >>>>> is probably nicer anyway. >>>>> >>>>> Kevin >>>>> >>>>> >>>> >>>> Hi Kevin, I got a bit lost on whether we have some agreement on where >>>> if anywhere to move the check/warning about missing decompression >>>> submodules. >>>> >>>> If that's ok I'd post a V5, and we can rediscuss from the new starting >>>> point? >>> >>> Sure, feel free, though I don't think the code will otherwise change for >>> dmg, so we could as well continue here. >>> >>> My conclusion was that only dmg_read_mish_block() or something called by >>> it can know whether compressed blocks exist in the image when the >>> modules aren't present. So if we want to make the warning conditional on >>> that (and my understanding is correct), this is where a >>> warn_report_once() would have to live. >>> >>> Kevin >>> >> >> I took a look, but I feel a bit too ignorant of the code there, maybe you could move the warning as a patch to the right place after the series? >> >> Or give me the extra commit needed to move into the right place? > > Only built and tested with an uncompressed image, so this could use > testing with an actual compressed dmg image and the module present or > missing, but something like the following should do the trick. > > Kevin > > > diff --git a/block/dmg.c b/block/dmg.c > index 98db18d82a..630cde3416 100644 > --- a/block/dmg.c > +++ b/block/dmg.c > @@ -254,6 +254,25 @@ static int dmg_read_mish_block(BDRVDMGState *s, DmgHeaderState *ds, > for (i = s->n_chunks; i < s->n_chunks + chunk_count; i++) { > s->types[i] = buff_read_uint32(buffer, offset); > if (!dmg_is_known_block_type(s->types[i])) { > + switch (s->types[i]) { > + case UDBZ: > + warn_report_once("dmg-bzip2 module is missing, accessing bzip2 " > + "compressed blocks will result in I/O errors"); > + break; > + case ULFO: > + warn_report_once("dmg-lzfse module is missing, accessing lzfse " > + "compressed blocks will result in I/O errors"); > + break; > + case UDCM: > + case UDLE: > + /* Comments and last entry can be ignored without problems */ > + break; > + default: > + warn_report_once("Image contains chunks of unknown type %x, " > + "accessing them will result in I/O errors", > + s->types[i]); > + break; > + } > chunk_count--; > i--; > offset += 40; > Awesome thanks! Claudio ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 3/3] accel: abort if we fail to load the accelerator plugin 2022-09-08 14:53 [PATCH v3 0/3] improve error handling for module load Claudio Fontana 2022-09-08 14:53 ` [PATCH v3 1/3] module: removed unused function argument "mayfail" Claudio Fontana 2022-09-08 14:53 ` [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one Claudio Fontana @ 2022-09-08 14:53 ` Claudio Fontana 2022-09-08 16:03 ` Richard Henderson 2 siblings, 1 reply; 22+ messages in thread From: Claudio Fontana @ 2022-09-08 14:53 UTC (permalink / raw) To: Paolo Bonzini, Richard Henderson, Markus Armbruster 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> --- 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] 22+ messages in thread
* Re: [PATCH v3 3/3] accel: abort if we fail to load the accelerator plugin 2022-09-08 14:53 ` [PATCH v3 3/3] accel: abort if we fail to load the accelerator plugin Claudio Fontana @ 2022-09-08 16:03 ` Richard Henderson 0 siblings, 0 replies; 22+ messages in thread From: Richard Henderson @ 2022-09-08 16:03 UTC (permalink / raw) To: Claudio Fontana, Paolo Bonzini, Markus Armbruster Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé On 9/8/22 15:53, 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> > --- > accel/accel-softmmu.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~ ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH RESEND v3 0/3] improve error handling for module load @ 2022-09-08 15:00 Claudio Fontana 2022-09-08 15:00 ` [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one Claudio Fontana 0 siblings, 1 reply; 22+ messages in thread From: Claudio Fontana @ 2022-09-08 15:00 UTC (permalink / raw) To: Paolo Bonzini, Richard Henderson, Markus Armbruster Cc: qemu-devel, dinechin, Gerd Hoffmann, Marc-André Lureau, Daniel P . Berrangé, Philippe Mathieu-Daudé, Claudio Fontana CHANGELOG: 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 (3): module: removed unused function argument "mayfail" module: add Error arguments to module_load_one and module_load_qom_one accel: abort if we fail to load the accelerator plugin accel/accel-softmmu.c | 8 ++- audio/audio.c | 6 +- block.c | 12 +++- block/dmg.c | 10 ++- hw/core/qdev.c | 10 ++- include/qemu/module.h | 38 +++++++++-- qom/object.c | 15 ++++- softmmu/qtest.c | 6 +- ui/console.c | 18 +++++- util/module.c | 142 ++++++++++++++++++++++++------------------ 10 files changed, 184 insertions(+), 81 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one 2022-09-08 15:00 [PATCH RESEND v3 0/3] improve error handling for module load Claudio Fontana @ 2022-09-08 15:00 ` Claudio Fontana 0 siblings, 0 replies; 22+ messages in thread From: Claudio Fontana @ 2022-09-08 15:00 UTC (permalink / raw) To: Paolo Bonzini, Richard Henderson, Markus Armbruster 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_one(const char *prefix, const char *lib_name); void module_load_qom_one(const char *type); to: bool module_load_one(const char *prefix, const char *name, Error **errp); bool module_load_qom_one(const char *type, Error **errp); module_load_qom_one has been introduced in: commit 28457744c345 ("module: qom module support"), which built on top of module_load_one, but discarded the bool return value. Restore it. Adapt all callers to emit errors, or ignore them, or fail hard, as appropriate in each context. Signed-off-by: Claudio Fontana <cfontana@suse.de> --- audio/audio.c | 6 +- block.c | 12 +++- block/dmg.c | 10 ++- hw/core/qdev.c | 10 ++- include/qemu/module.h | 38 ++++++++++-- qom/object.c | 15 ++++- softmmu/qtest.c | 6 +- ui/console.c | 18 +++++- util/module.c | 140 ++++++++++++++++++++++++------------------ 9 files changed, 177 insertions(+), 78 deletions(-) diff --git a/audio/audio.c b/audio/audio.c index 76b8735b44..4f4bb10cce 100644 --- a/audio/audio.c +++ b/audio/audio.c @@ -72,6 +72,7 @@ void audio_driver_register(audio_driver *drv) audio_driver *audio_driver_lookup(const char *name) { struct audio_driver *d; + Error *local_err = NULL; QLIST_FOREACH(d, &audio_drivers, next) { if (strcmp(name, d->name) == 0) { @@ -79,7 +80,10 @@ audio_driver *audio_driver_lookup(const char *name) } } - audio_module_load_one(name); + if (!audio_module_load_one(name, &local_err) && local_err) { + error_report_err(local_err); + } + QLIST_FOREACH(d, &audio_drivers, next) { if (strcmp(name, d->name) == 0) { return d; diff --git a/block.c b/block.c index bc85f46eed..85c3742d7a 100644 --- a/block.c +++ b/block.c @@ -464,7 +464,11 @@ 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); + Error *local_err = NULL; + if (!block_module_load_one(block_driver_modules[i].library_name, + &local_err) && local_err) { + error_report_err(local_err); + } break; } } @@ -976,7 +980,11 @@ 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); + Error *local_err = NULL; + if (!block_module_load_one(block_driver_modules[i].library_name, + &local_err) && local_err) { + error_report_err(local_err); + } break; } } diff --git a/block/dmg.c b/block/dmg.c index 98db18d82a..349b05d20b 100644 --- a/block/dmg.c +++ b/block/dmg.c @@ -434,6 +434,7 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, uint64_t plist_xml_offset, plist_xml_length; int64_t offset; int ret; + Error *local_err = NULL; ret = bdrv_apply_auto_read_only(bs, NULL, errp); if (ret < 0) { @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags, return -EINVAL; } - block_module_load_one("dmg-bz2"); - block_module_load_one("dmg-lzfse"); + if (!block_module_load_one("dmg-bz2", &local_err) && local_err) { + error_report_err(local_err); + } + local_err = NULL; + if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) { + error_report_err(local_err); + } 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..5902c59c94 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -148,7 +148,15 @@ 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); + Error *local_err = NULL; + if (!module_load_qom_one(name, &local_err)) { + if (local_err) { + error_report_err(local_err); + } else { + 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 8c012bbe03..78d4c4de96 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -61,16 +61,44 @@ 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_one(lib, errp) module_load_one("block-", lib, errp) +#define ui_module_load_one(lib, errp) module_load_one("ui-", lib, errp) +#define audio_module_load_one(lib, errp) module_load_one("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_one(const char *prefix, const char *lib_name); -void module_load_qom_one(const char *type); + +/* + * module_load_one: attempt to load a module from a set of directories + * + * directories searched are: + * - getenv("QEMU_MODULE_DIR") + * - get_relocated_path(CONFIG_QEMU_MODDIR); + * - /var/run/qemu/${version_dir} + * + * prefix: a subsystem prefix, or the empty string ("audio-", ..., "") + * name: name of the module + * errp: error to set in case the module is found, but load failed. + * + * Return value: true on success (found and loaded); + * if module if found, but load failed, errp will be set. + * if module is not found, errp will not be set. + */ +bool module_load_one(const char *prefix, const char *name, Error **errp); + +/* + * module_load_qom_one: attempt to load a module to provide a QOM type + * + * type: the type to be provided + * errp: error to set. + * + * Return value: true on success (found and loaded), false on failure. + * If a module is simply not found for the type, + * errp will not be set. + */ +bool module_load_qom_one(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 d34608558e..6a74e6a478 100644 --- a/qom/object.c +++ b/qom/object.c @@ -526,8 +526,14 @@ void object_initialize(void *data, size_t size, const char *typename) #ifdef CONFIG_MODULES if (!type) { - module_load_qom_one(typename); - type = type_get_by_name(typename); + Error *local_err = NULL; + if (!module_load_qom_one(typename, &local_err)) { + if (local_err) { + error_report_err(local_err); + } + } else { + type = type_get_by_name(typename); + } } #endif if (!type) { @@ -1033,7 +1039,10 @@ 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); + Error *local_err = NULL; + if (!module_load_qom_one(typename, &local_err) && local_err) { + error_report_err(local_err); + } oc = object_class_by_name(typename); } #endif diff --git a/softmmu/qtest.c b/softmmu/qtest.c index 76eb7bac56..bb83c7aae9 100644 --- a/softmmu/qtest.c +++ b/softmmu/qtest.c @@ -753,12 +753,16 @@ 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; g_assert(words[1] && words[2]); qtest_send_prefix(chr); - if (module_load_one(words[1], words[2])) { + if (module_load_one(words[1], words[2], &local_err)) { qtest_sendf(chr, "OK\n"); } else { + if (local_err) { + 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 765892f84f..34dd206167 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_one(DisplayType_str(prio[i])); + Error *local_err = NULL; + if (!ui_module_load_one(DisplayType_str(prio[i]), &local_err) + && local_err) { + 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_one(DisplayType_str(opts->type)); + Error *local_err = NULL; + if (!ui_module_load_one(DisplayType_str(opts->type), &local_err) + && local_err) { + 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_one(DisplayType_str(idx)); + Error *local_err = NULL; + if (!ui_module_load_one(DisplayType_str(idx), &local_err) + && local_err) { + 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 8563edd626..102884ae22 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"); } 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_one(const char *prefix, const char *lib_name) +bool module_load_one(const char *prefix, const char *name, Error **errp) { bool success = false; - -#ifdef CONFIG_MODULES - char *fname = NULL; #ifdef CONFIG_MODULE_UPGRADES char *version_dir; #endif @@ -219,14 +209,13 @@ bool module_load_one(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"); + error_setg(errp, "%s", "this platform does not support GLib modules"); return false; } @@ -234,7 +223,7 @@ bool module_load_one(const char *prefix, const char *lib_name) loaded_modules = g_hash_table_new(g_str_hash, g_str_equal); } - module_name = g_strdup_printf("%s%s", prefix, lib_name); + module_name = g_strdup_printf("%s%s", prefix, name); if (g_hash_table_contains(loaded_modules, module_name)) { g_free(module_name); @@ -246,6 +235,8 @@ bool module_load_one(const char *prefix, const char *lib_name) 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); return false; } } @@ -254,7 +245,9 @@ 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); + if (!(module_load_one("", *sl, errp))) { + return false; + } } } else { for (sl = modinfo->deps; *sl != NULL; sl++) { @@ -283,16 +276,26 @@ bool module_load_one(const char *prefix, const char *lib_name) 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; + char *fname = g_strdup_printf("%s/%s%s", + dirs[i], module_name, CONFIG_HOST_DSOSUF); + int ret = access(fname, F_OK); + if (ret != 0) { + if (errno == ENOENT || errno == ENOTDIR) { + /* + * if we just 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 or + * distro did not install the module. We will return false in this + * case, with no error set. + */ + continue; + } + /* most common is EACCES here */ + error_setg_errno(errp, errno, "error trying to access %s", fname); break; } + success = module_load_dso(fname, export_symbols, errp); + g_free(fname); + break; } if (!success) { @@ -304,21 +307,20 @@ bool module_load_one(const char *prefix, const char *lib_name) g_free(dirs[i]); } -#endif return success; } -#ifdef CONFIG_MODULES - static bool module_loaded_qom_all; -void module_load_qom_one(const char *type) +bool module_load_qom_one(const char *type, Error **errp) { + bool found = false; const QemuModinfo *modinfo; const char **sl; if (!type) { - return; + error_setg(errp, "%s", "type is NULL"); + return false; } trace_module_lookup_object_type(type); @@ -331,15 +333,26 @@ 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); + if (found) { + error_setg(errp, "multiple modules providing '%s'", type); + found = false; + break; + } + found = module_load_one("", modinfo->name, errp); + if (!found) { + /* errp optionally set in module_load_one */ + break; + } } } } + return found; } void module_load_qom_all(void) { const QemuModinfo *modinfo; + Error *local_err = NULL; if (module_loaded_qom_all) { return; @@ -352,7 +365,9 @@ void module_load_qom_all(void) if (!module_check_arch(modinfo)) { continue; } - module_load_one("", modinfo->name); + if (!module_load_one("", modinfo->name, &local_err) && local_err) { + error_report_err(local_err); + } } module_loaded_qom_all = true; } @@ -368,7 +383,11 @@ 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); + Error *local_err = NULL; + if (!module_load_one("", modinfo->name, &local_err) + && local_err) { + error_report_err(local_err); + } } } } @@ -378,7 +397,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_one(const char *type) {} +bool module_load_one(const char *prefix, const char *name, Error **errp) { return true; } +bool module_load_qom_one(const char *type, Error **errp) { return true; } void module_load_qom_all(void) {} #endif -- 2.26.2 ^ permalink raw reply related [flat|nested] 22+ messages in thread
end of thread, other threads:[~2022-09-23 22:24 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-08 14:53 [PATCH v3 0/3] improve error handling for module load Claudio Fontana 2022-09-08 14:53 ` [PATCH v3 1/3] module: removed unused function argument "mayfail" Claudio Fontana 2022-09-08 14:53 ` [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one Claudio Fontana 2022-09-08 16:03 ` Richard Henderson 2022-09-08 17:10 ` Claudio Fontana 2022-09-08 17:36 ` Claudio Fontana 2022-09-20 16:50 ` Kevin Wolf 2022-09-21 4:45 ` Markus Armbruster 2022-09-21 11:43 ` Kevin Wolf 2022-09-21 12:08 ` Markus Armbruster 2022-09-22 14:33 ` Kevin Wolf 2022-09-22 15:09 ` Markus Armbruster 2022-09-21 7:50 ` Claudio Fontana 2022-09-21 11:56 ` Kevin Wolf 2022-09-23 14:10 ` Claudio Fontana 2022-09-23 14:42 ` Kevin Wolf 2022-09-23 14:46 ` Claudio Fontana 2022-09-23 16:29 ` Kevin Wolf 2022-09-23 22:23 ` Claudio Fontana 2022-09-08 14:53 ` [PATCH v3 3/3] accel: abort if we fail to load the accelerator plugin Claudio Fontana 2022-09-08 16:03 ` Richard Henderson 2022-09-08 15:00 [PATCH RESEND v3 0/3] improve error handling for module load Claudio Fontana 2022-09-08 15:00 ` [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one Claudio Fontana
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.