improve error handling during module load, by changing:
bool module_load(const char *prefix, const char *lib_name);
void module_load_qom(const char *type);
to:
int module_load(const char *prefix, const char *name, Error **errp);
int module_load_qom(const char *type, Error **errp);
where the return value is:
-1 on module load error, and errp is set with the error
0 on module or one of its dependencies are not installed
1 on module load success
2 on module load success (module already loaded or built-in)
module_load_qom_one has been introduced in:
commit 28457744c345 ("module: qom module support"), which built on top of
module_load_one, but discarded the bool return value. Restore it.
Adapt all callers to emit errors, or ignore them, or fail hard,
as appropriate in each context.
Some memory leaks also fixed as part of the module_load changes.
audio: when attempting to load an audio module, report module load errors.
block: when attempting to load a block module, report module load errors.
console: when attempting to load a display module, report module load errors.
qdev: when creating a new qdev Device object (DeviceState), report load errors.
If a module cannot be loaded to create that device, now abort execution.
qom/object.c: when initializing a QOM object, or looking up class_by_name,
report module load errors.
qtest: when processing the "module_load" qtest command, report errors
in the load of the module.
Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
audio/audio.c | 16 ++--
block.c | 21 ++++-
block/dmg.c | 12 ++-
hw/core/qdev.c | 5 +-
include/qemu/module.h | 37 +++++++-
qom/object.c | 18 +++-
softmmu/qtest.c | 8 +-
ui/console.c | 18 +++-
util/module.c | 211 +++++++++++++++++++++++-------------------
9 files changed, 223 insertions(+), 123 deletions(-)
diff --git a/audio/audio.c b/audio/audio.c
index 0a682336a0..ea51793843 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -72,20 +72,24 @@ void audio_driver_register(audio_driver *drv)
audio_driver *audio_driver_lookup(const char *name)
{
struct audio_driver *d;
+ Error *local_err = NULL;
+ int rv;
QLIST_FOREACH(d, &audio_drivers, next) {
if (strcmp(name, d->name) == 0) {
return d;
}
}
-
- audio_module_load(name);
- QLIST_FOREACH(d, &audio_drivers, next) {
- if (strcmp(name, d->name) == 0) {
- return d;
+ rv = audio_module_load(name, &local_err);
+ if (rv > 0) {
+ QLIST_FOREACH(d, &audio_drivers, next) {
+ if (strcmp(name, d->name) == 0) {
+ return d;
+ }
}
+ } else if (rv < 0) {
+ error_report_err(local_err);
}
-
return NULL;
}
diff --git a/block.c b/block.c
index 72c7f6d47d..0390ece056 100644
--- a/block.c
+++ b/block.c
@@ -464,12 +464,18 @@ BlockDriver *bdrv_find_format(const char *format_name)
/* The driver isn't registered, maybe we need to load a module */
for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
if (!strcmp(block_driver_modules[i].format_name, format_name)) {
- block_module_load(block_driver_modules[i].library_name);
+ Error *local_err = NULL;
+ int rv = block_module_load(block_driver_modules[i].library_name,
+ &local_err);
+ if (rv > 0) {
+ return bdrv_do_find_format(format_name);
+ } else if (rv < 0) {
+ error_report_err(local_err);
+ }
break;
}
}
-
- return bdrv_do_find_format(format_name);
+ return NULL;
}
static int bdrv_format_is_whitelisted(const char *format_name, bool read_only)
@@ -976,12 +982,17 @@ BlockDriver *bdrv_find_protocol(const char *filename,
for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
if (block_driver_modules[i].protocol_name &&
!strcmp(block_driver_modules[i].protocol_name, protocol)) {
- block_module_load(block_driver_modules[i].library_name);
+ Error *local_err = NULL;
+ int rv = block_module_load(block_driver_modules[i].library_name, &local_err);
+ if (rv > 0) {
+ drv1 = bdrv_do_find_protocol(protocol);
+ } else if (rv < 0) {
+ error_report_err(local_err);
+ }
break;
}
}
- drv1 = bdrv_do_find_protocol(protocol);
if (!drv1) {
error_setg(errp, "Unknown protocol '%s'", protocol);
}
diff --git a/block/dmg.c b/block/dmg.c
index 007b8d9996..a422cf8d5b 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,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
return -EINVAL;
}
- block_module_load("dmg-bz2");
- block_module_load("dmg-lzfse");
+ if (block_module_load("dmg-bz2", &local_err) < 0) {
+ error_report_err(local_err);
+ return -EINVAL;
+ }
+ local_err = NULL;
+ if (block_module_load("dmg-lzfse", &local_err) < 0) {
+ error_report_err(local_err);
+ return -EINVAL;
+ }
s->n_chunks = 0;
s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 25dfc08468..06a4bce145 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -147,8 +147,9 @@ bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
DeviceState *qdev_new(const char *name)
{
- if (!object_class_by_name(name)) {
- module_load_qom(name);
+ if (!module_object_class_by_name(name)) {
+ error_report("could not find a module for type '%s'", name);
+ abort();
}
return DEVICE(object_new(name));
}
diff --git a/include/qemu/module.h b/include/qemu/module.h
index b7911ce791..c37ce74b16 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -61,16 +61,43 @@ typedef enum {
#define fuzz_target_init(function) module_init(function, \
MODULE_INIT_FUZZ_TARGET)
#define migration_init(function) module_init(function, MODULE_INIT_MIGRATION)
-#define block_module_load(lib) module_load("block-", lib)
-#define ui_module_load(lib) module_load("ui-", lib)
-#define audio_module_load(lib) module_load("audio-", lib)
+#define block_module_load(lib, errp) module_load("block-", lib, errp)
+#define ui_module_load(lib, errp) module_load("ui-", lib, errp)
+#define audio_module_load(lib, errp) module_load("audio-", lib, errp)
void register_module_init(void (*fn)(void), module_init_type type);
void register_dso_module_init(void (*fn)(void), module_init_type type);
void module_call_init(module_init_type type);
-bool module_load(const char *prefix, const char *lib_name);
-void module_load_qom(const char *type);
+
+/*
+ * module_load: attempt to load a module from a set of directories
+ *
+ * directories searched are:
+ * - getenv("QEMU_MODULE_DIR")
+ * - get_relocated_path(CONFIG_QEMU_MODDIR);
+ * - /var/run/qemu/${version_dir}
+ *
+ * prefix: a subsystem prefix, or the empty string ("audio-", ..., "")
+ * name: name of the module
+ * errp: error to set in case the module is found, but load failed.
+ *
+ * Return value: -1 on error (errp set if not NULL).
+ * 0 if module or one of its dependencies are not installed,
+ * 1 if the module is found and loaded,
+ * 2 if the module is already loaded, or module is built-in.
+ */
+int module_load(const char *prefix, const char *name, Error **errp);
+
+/*
+ * module_load_qom: attempt to load a module to provide a QOM type
+ *
+ * type: the type to be provided
+ * errp: error to set.
+ *
+ * Return value: as per module_load.
+ */
+int module_load_qom(const char *type, Error **errp);
void module_load_qom_all(void);
void module_allow_arch(const char *arch);
diff --git a/qom/object.c b/qom/object.c
index 4f834f3bf6..35ced55282 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -526,8 +526,13 @@ void object_initialize(void *data, size_t size, const char *typename)
#ifdef CONFIG_MODULES
if (!type) {
- module_load_qom(typename);
- type = type_get_by_name(typename);
+ Error *local_err = NULL;
+ int rv = module_load_qom(typename, &local_err);
+ if (rv > 0) {
+ type = type_get_by_name(typename);
+ } else if (rv < 0) {
+ error_report_err(local_err);
+ }
}
#endif
if (!type) {
@@ -1033,8 +1038,13 @@ ObjectClass *module_object_class_by_name(const char *typename)
oc = object_class_by_name(typename);
#ifdef CONFIG_MODULES
if (!oc) {
- module_load_qom(typename);
- oc = object_class_by_name(typename);
+ Error *local_err = NULL;
+ int rv = module_load_qom(typename, &local_err);
+ if (rv > 0) {
+ oc = object_class_by_name(typename);
+ } else if (rv < 0) {
+ error_report_err(local_err);
+ }
}
#endif
return oc;
diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index fc5b733c63..36e28609ff 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -753,12 +753,18 @@ static void qtest_process_command(CharBackend *chr, gchar **words)
qtest_sendf(chr, "OK %"PRIi64"\n",
(int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
} else if (strcmp(words[0], "module_load") == 0) {
+ Error *local_err = NULL;
+ int rv;
g_assert(words[1] && words[2]);
qtest_send_prefix(chr);
- if (module_load(words[1], words[2])) {
+ rv = module_load(words[1], words[2], &local_err);
+ if (rv > 0) {
qtest_sendf(chr, "OK\n");
} else {
+ if (rv < 0) {
+ error_report_err(local_err);
+ }
qtest_sendf(chr, "FAIL\n");
}
} else if (qtest_enabled() && strcmp(words[0], "clock_set") == 0) {
diff --git a/ui/console.c b/ui/console.c
index 4913c55684..4e53c3c71b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2632,7 +2632,11 @@ bool qemu_display_find_default(DisplayOptions *opts)
for (i = 0; i < (int)ARRAY_SIZE(prio); i++) {
if (dpys[prio[i]] == NULL) {
- ui_module_load(DisplayType_str(prio[i]));
+ Error *local_err = NULL;
+ int rv = ui_module_load(DisplayType_str(prio[i]), &local_err);
+ if (rv < 0) {
+ error_report_err(local_err);
+ }
}
if (dpys[prio[i]] == NULL) {
continue;
@@ -2650,7 +2654,11 @@ void qemu_display_early_init(DisplayOptions *opts)
return;
}
if (dpys[opts->type] == NULL) {
- ui_module_load(DisplayType_str(opts->type));
+ Error *local_err = NULL;
+ int rv = ui_module_load(DisplayType_str(opts->type), &local_err);
+ if (rv < 0) {
+ error_report_err(local_err);
+ }
}
if (dpys[opts->type] == NULL) {
error_report("Display '%s' is not available.",
@@ -2680,7 +2688,11 @@ void qemu_display_help(void)
printf("none\n");
for (idx = DISPLAY_TYPE_NONE; idx < DISPLAY_TYPE__MAX; idx++) {
if (!dpys[idx]) {
- ui_module_load(DisplayType_str(idx));
+ Error *local_err = NULL;
+ int rv = ui_module_load(DisplayType_str(idx), &local_err);
+ if (rv < 0) {
+ error_report_err(local_err);
+ }
}
if (dpys[idx]) {
printf("%s\n", DisplayType_str(dpys[idx]->type));
diff --git a/util/module.c b/util/module.c
index ad89cd50dc..b67923a986 100644
--- a/util/module.c
+++ b/util/module.c
@@ -21,6 +21,7 @@
#include "qemu/module.h"
#include "qemu/cutils.h"
#include "qemu/config-file.h"
+#include "qapi/error.h"
#ifdef CONFIG_MODULE_UPGRADES
#include "qemu-version.h"
#endif
@@ -144,25 +145,22 @@ static bool module_check_arch(const QemuModinfo *modinfo)
return true;
}
-static int module_load_file(const char *fname, bool export_symbols)
+/*
+ * module_load_dso: attempt to load an existing dso file
+ *
+ * fname: full pathname of the file to load
+ * export_symbols: if true, add the symbols to the global name space
+ * errp: error to set.
+ *
+ * Return value: true on success, false on error, and errp will be set.
+ */
+static bool module_load_dso(const char *fname, bool export_symbols,
+ Error **errp)
{
GModule *g_module;
void (*sym)(void);
- const char *dsosuf = CONFIG_HOST_DSOSUF;
- int len = strlen(fname);
- int suf_len = strlen(dsosuf);
ModuleEntry *e, *next;
- int ret, flags;
-
- if (len <= suf_len || strcmp(&fname[len - suf_len], dsosuf)) {
- /* wrong suffix */
- ret = -EINVAL;
- goto out;
- }
- if (access(fname, F_OK)) {
- ret = -ENOENT;
- goto out;
- }
+ int flags;
assert(QTAILQ_EMPTY(&dso_init_list));
@@ -172,46 +170,38 @@ static int module_load_file(const char *fname, bool export_symbols)
}
g_module = g_module_open(fname, flags);
if (!g_module) {
- fprintf(stderr, "Failed to open module: %s\n",
- g_module_error());
- ret = -EINVAL;
- goto out;
+ error_setg(errp, "failed to open module: %s", g_module_error());
+ return false;
}
if (!g_module_symbol(g_module, DSO_STAMP_FUN_STR, (gpointer *)&sym)) {
- fprintf(stderr, "Failed to initialize module: %s\n",
- fname);
- /* Print some info if this is a QEMU module (but from different build),
- * this will make debugging user problems easier. */
+ error_setg(errp, "failed to initialize module: %s", fname);
+ /*
+ * Print some info if this is a QEMU module (but from different build),
+ * this will make debugging user problems easier.
+ */
if (g_module_symbol(g_module, "qemu_module_dummy", (gpointer *)&sym)) {
- fprintf(stderr,
- "Note: only modules from the same build can be loaded.\n");
+ error_append_hint(errp,
+ "Only modules from the same build can be loaded.\n");
}
g_module_close(g_module);
- ret = -EINVAL;
- } else {
- QTAILQ_FOREACH(e, &dso_init_list, node) {
- e->init();
- register_module_init(e->init, e->type);
- }
- ret = 0;
+ return false;
}
+ QTAILQ_FOREACH(e, &dso_init_list, node) {
+ e->init();
+ register_module_init(e->init, e->type);
+ }
trace_module_load_module(fname);
QTAILQ_FOREACH_SAFE(e, &dso_init_list, node, next) {
QTAILQ_REMOVE(&dso_init_list, e, node);
g_free(e);
}
-out:
- return ret;
+ return true;
}
-#endif
-bool module_load(const char *prefix, const char *lib_name)
+int module_load(const char *prefix, const char *name, Error **errp)
{
- bool success = false;
-
-#ifdef CONFIG_MODULES
- char *fname = NULL;
+ int rv = -1;
#ifdef CONFIG_MODULE_UPGRADES
char *version_dir;
#endif
@@ -219,54 +209,29 @@ bool module_load(const char *prefix, const char *lib_name)
char *dirs[5];
char *module_name;
int i = 0, n_dirs = 0;
- int ret;
bool export_symbols = false;
static GHashTable *loaded_modules;
const QemuModinfo *modinfo;
const char **sl;
if (!g_module_supported()) {
- fprintf(stderr, "Module is not supported by system.\n");
- return false;
+ error_setg(errp, "%s", "this platform does not support GLib modules");
+ return -1;
}
if (!loaded_modules) {
loaded_modules = g_hash_table_new(g_str_hash, g_str_equal);
}
- module_name = g_strdup_printf("%s%s", prefix, lib_name);
+ /* allocate all resources managed by the out: label here */
+ module_name = g_strdup_printf("%s%s", prefix, name);
if (g_hash_table_contains(loaded_modules, module_name)) {
g_free(module_name);
- return true;
+ return 2; /* module already loaded */
}
g_hash_table_add(loaded_modules, module_name);
- for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
- if (modinfo->arch) {
- if (strcmp(modinfo->name, module_name) == 0) {
- if (!module_check_arch(modinfo)) {
- return false;
- }
- }
- }
- if (modinfo->deps) {
- if (strcmp(modinfo->name, module_name) == 0) {
- /* we depend on other module(s) */
- for (sl = modinfo->deps; *sl != NULL; sl++) {
- module_load("", *sl);
- }
- } else {
- for (sl = modinfo->deps; *sl != NULL; sl++) {
- if (strcmp(module_name, *sl) == 0) {
- /* another module depends on us */
- export_symbols = true;
- }
- }
- }
- }
- }
-
search_dir = getenv("QEMU_MODULE_DIR");
if (search_dir != NULL) {
dirs[n_dirs++] = g_strdup_printf("%s", search_dir);
@@ -279,46 +244,87 @@ bool module_load(const char *prefix, const char *lib_name)
'_');
dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
#endif
-
assert(n_dirs <= ARRAY_SIZE(dirs));
- for (i = 0; i < n_dirs; i++) {
- fname = g_strdup_printf("%s/%s%s",
- dirs[i], module_name, CONFIG_HOST_DSOSUF);
- ret = module_load_file(fname, export_symbols);
- g_free(fname);
- fname = NULL;
- /* Try loading until loaded a module file */
- if (!ret) {
- success = true;
- break;
+ /* end of resources managed by the out: label */
+
+ for (modinfo = module_info; modinfo->name != NULL; modinfo++) {
+ if (modinfo->arch) {
+ if (strcmp(modinfo->name, module_name) == 0) {
+ if (!module_check_arch(modinfo)) {
+ error_setg(errp, "module arch does not match: "
+ "expected '%s', got '%s'", module_arch, modinfo->arch);
+ goto out;
+ }
+ }
+ }
+ if (modinfo->deps) {
+ if (strcmp(modinfo->name, module_name) == 0) {
+ /* we depend on other module(s) */
+ for (sl = modinfo->deps; *sl != NULL; sl++) {
+ int subrv = module_load("", *sl, errp);
+ if (subrv <= 0) {
+ rv = subrv;
+ goto out;
+ }
+ }
+ } else {
+ for (sl = modinfo->deps; *sl != NULL; sl++) {
+ if (strcmp(module_name, *sl) == 0) {
+ /* another module depends on us */
+ export_symbols = true;
+ }
+ }
+ }
}
}
- if (!success) {
- g_hash_table_remove(loaded_modules, module_name);
- g_free(module_name);
+ for (i = 0; i < n_dirs; i++) {
+ char *fname = g_strdup_printf("%s/%s%s",
+ dirs[i], module_name, CONFIG_HOST_DSOSUF);
+ int ret = access(fname, F_OK);
+ if (ret != 0 && (errno == ENOENT || errno == ENOTDIR)) {
+ /*
+ * if we don't find the module in this dir, try the next one.
+ * If we don't find it in any dir, that can be fine too: user
+ * did not install the module. We will return 0 in this case
+ * with no error set.
+ */
+ g_free(fname);
+ continue;
+ } else if (ret != 0) {
+ /* most common is EACCES here */
+ error_setg_errno(errp, errno, "error trying to access %s", fname);
+ } else if (module_load_dso(fname, export_symbols, errp)) {
+ rv = 1; /* module successfully loaded */
+ }
+ g_free(fname);
+ goto out;
}
+ rv = 0; /* module not found */
+out:
+ if (rv <= 0) {
+ g_hash_table_remove(loaded_modules, module_name);
+ }
+ g_free(module_name);
for (i = 0; i < n_dirs; i++) {
g_free(dirs[i]);
}
-
-#endif
- return success;
+ return rv;
}
-#ifdef CONFIG_MODULES
-
static bool module_loaded_qom_all;
-void module_load_qom(const char *type)
+int module_load_qom(const char *type, Error **errp)
{
const QemuModinfo *modinfo;
const char **sl;
+ int rv = 0;
if (!type) {
- return;
+ error_setg(errp, "%s", "type is NULL");
+ return -1;
}
trace_module_lookup_object_type(type);
@@ -331,15 +337,24 @@ void module_load_qom(const char *type)
}
for (sl = modinfo->objs; *sl != NULL; sl++) {
if (strcmp(type, *sl) == 0) {
- module_load("", modinfo->name);
+ if (rv > 0) {
+ error_setg(errp, "multiple modules providing '%s'", type);
+ return -1;
+ }
+ rv = module_load("", modinfo->name, errp);
+ if (rv < 0) {
+ return rv;
+ }
}
}
}
+ return rv;
}
void module_load_qom_all(void)
{
const QemuModinfo *modinfo;
+ Error *local_err = NULL;
if (module_loaded_qom_all) {
return;
@@ -352,7 +367,9 @@ void module_load_qom_all(void)
if (!module_check_arch(modinfo)) {
continue;
}
- module_load("", modinfo->name);
+ if (module_load("", modinfo->name, &local_err) < 0) {
+ error_report_err(local_err);
+ }
}
module_loaded_qom_all = true;
}
@@ -368,7 +385,10 @@ void qemu_load_module_for_opts(const char *group)
}
for (sl = modinfo->opts; *sl != NULL; sl++) {
if (strcmp(group, *sl) == 0) {
- module_load("", modinfo->name);
+ Error *local_err = NULL;
+ if (module_load("", modinfo->name, &local_err) < 0) {
+ error_report_err(local_err);
+ }
}
}
}
@@ -378,7 +398,8 @@ void qemu_load_module_for_opts(const char *group)
void module_allow_arch(const char *arch) {}
void qemu_load_module_for_opts(const char *group) {}
-void module_load_qom(const char *type) {}
+int module_load(const char *prefix, const char *name, Error **errp) { return 2; }
+int module_load_qom(const char *type, Error **errp) { return 2; }
void module_load_qom_all(void) {}
#endif
--
2.26.2Am 24.09.2022 um 01:21 hat Claudio Fontana geschrieben:
> improve error handling during module load, by changing:
>
> bool module_load(const char *prefix, const char *lib_name);
> void module_load_qom(const char *type);
>
> to:
>
> int module_load(const char *prefix, const char *name, Error **errp);
> int module_load_qom(const char *type, Error **errp);
>
> where the return value is:
>
> -1 on module load error, and errp is set with the error
> 0 on module or one of its dependencies are not installed
> 1 on module load success
> 2 on module load success (module already loaded or built-in)
>
> module_load_qom_one has been introduced in:
>
> commit 28457744c345 ("module: qom module support"), which built on top of
> module_load_one, but discarded the bool return value. Restore it.
>
> Adapt all callers to emit errors, or ignore them, or fail hard,
> as appropriate in each context.
>
> Some memory leaks also fixed as part of the module_load changes.
>
> audio: when attempting to load an audio module, report module load errors.
> block: when attempting to load a block module, report module load errors.
> console: when attempting to load a display module, report module load errors.
>
> qdev: when creating a new qdev Device object (DeviceState), report load errors.
> If a module cannot be loaded to create that device, now abort execution.
>
> qom/object.c: when initializing a QOM object, or looking up class_by_name,
> report module load errors.
>
> qtest: when processing the "module_load" qtest command, report errors
> in the load of the module.
>
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> diff --git a/block/dmg.c b/block/dmg.c
> index 007b8d9996..a422cf8d5b 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,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> return -EINVAL;
> }
>
> - block_module_load("dmg-bz2");
> - block_module_load("dmg-lzfse");
> + if (block_module_load("dmg-bz2", &local_err) < 0) {
> + error_report_err(local_err);
> + return -EINVAL;
> + }
> + local_err = NULL;
> + if (block_module_load("dmg-lzfse", &local_err) < 0) {
> + error_report_err(local_err);
> + return -EINVAL;
> + }
Why don't we pass the existing errp instead of adding a new local_err
and printing it locally? If we use error_report_err(local_err) and leave
errp untouched, the caller will print another less specific error
message, which is generally not what we want.
Kevin
On 9/26/22 12:38, Kevin Wolf wrote:
> Am 24.09.2022 um 01:21 hat Claudio Fontana geschrieben:
>> improve error handling during module load, by changing:
>>
>> bool module_load(const char *prefix, const char *lib_name);
>> void module_load_qom(const char *type);
>>
>> to:
>>
>> int module_load(const char *prefix, const char *name, Error **errp);
>> int module_load_qom(const char *type, Error **errp);
>>
>> where the return value is:
>>
>> -1 on module load error, and errp is set with the error
>> 0 on module or one of its dependencies are not installed
>> 1 on module load success
>> 2 on module load success (module already loaded or built-in)
>>
>> module_load_qom_one has been introduced in:
>>
>> commit 28457744c345 ("module: qom module support"), which built on top of
>> module_load_one, but discarded the bool return value. Restore it.
>>
>> Adapt all callers to emit errors, or ignore them, or fail hard,
>> as appropriate in each context.
>>
>> Some memory leaks also fixed as part of the module_load changes.
>>
>> audio: when attempting to load an audio module, report module load errors.
>> block: when attempting to load a block module, report module load errors.
>> console: when attempting to load a display module, report module load errors.
>>
>> qdev: when creating a new qdev Device object (DeviceState), report load errors.
>> If a module cannot be loaded to create that device, now abort execution.
>>
>> qom/object.c: when initializing a QOM object, or looking up class_by_name,
>> report module load errors.
>>
>> qtest: when processing the "module_load" qtest command, report errors
>> in the load of the module.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>
>> diff --git a/block/dmg.c b/block/dmg.c
>> index 007b8d9996..a422cf8d5b 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,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>> return -EINVAL;
>> }
>>
>> - block_module_load("dmg-bz2");
>> - block_module_load("dmg-lzfse");
>> + if (block_module_load("dmg-bz2", &local_err) < 0) {
>> + error_report_err(local_err);
>> + return -EINVAL;
>> + }
>> + local_err = NULL;
>> + if (block_module_load("dmg-lzfse", &local_err) < 0) {
>> + error_report_err(local_err);
>> + return -EINVAL;
I am concerned about the resources allocation here though,
is returning EINVAL here right, vs using "goto fail"?
I matched the behavior of the preceding call:
bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_IMAGE, false, errp);
if (!bs->file) {
return -EINVAL;
}
But afterwards the code goes:
.
/* locate the UDIF trailer */
offset = dmg_find_koly_offset(bs->file, errp);
if (offset < 0) {
ret = offset;
goto fail;
}
Should the resources be freed or not in your view?
Thanks,
Claudio
>> + }
>
> Why don't we pass the existing errp instead of adding a new local_err
> and printing it locally? If we use error_report_err(local_err) and leave
> errp untouched, the caller will print another less specific error
> message, which is generally not what we want.
>
> Kevin
>
Claudio Fontana <cfontana@suse.de> writes:
> On 9/26/22 12:38, Kevin Wolf wrote:
>> Am 24.09.2022 um 01:21 hat Claudio Fontana geschrieben:
>>> improve error handling during module load, by changing:
>>>
>>> bool module_load(const char *prefix, const char *lib_name);
>>> void module_load_qom(const char *type);
>>>
>>> to:
>>>
>>> int module_load(const char *prefix, const char *name, Error **errp);
>>> int module_load_qom(const char *type, Error **errp);
>>>
>>> where the return value is:
>>>
>>> -1 on module load error, and errp is set with the error
>>> 0 on module or one of its dependencies are not installed
>>> 1 on module load success
>>> 2 on module load success (module already loaded or built-in)
>>>
>>> module_load_qom_one has been introduced in:
>>>
>>> commit 28457744c345 ("module: qom module support"), which built on top of
>>> module_load_one, but discarded the bool return value. Restore it.
>>>
>>> Adapt all callers to emit errors, or ignore them, or fail hard,
>>> as appropriate in each context.
>>>
>>> Some memory leaks also fixed as part of the module_load changes.
>>>
>>> audio: when attempting to load an audio module, report module load errors.
>>> block: when attempting to load a block module, report module load errors.
>>> console: when attempting to load a display module, report module load errors.
>>>
>>> qdev: when creating a new qdev Device object (DeviceState), report load errors.
>>> If a module cannot be loaded to create that device, now abort execution.
>>>
>>> qom/object.c: when initializing a QOM object, or looking up class_by_name,
>>> report module load errors.
>>>
>>> qtest: when processing the "module_load" qtest command, report errors
>>> in the load of the module.
>>>
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>
>>> diff --git a/block/dmg.c b/block/dmg.c
>>> index 007b8d9996..a422cf8d5b 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,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>>> return -EINVAL;
>>> }
>>>
>>> - block_module_load("dmg-bz2");
>>> - block_module_load("dmg-lzfse");
>>> + if (block_module_load("dmg-bz2", &local_err) < 0) {
>>> + error_report_err(local_err);
>>> + return -EINVAL;
>>> + }
>>> + local_err = NULL;
>>> + if (block_module_load("dmg-lzfse", &local_err) < 0) {
>>> + error_report_err(local_err);
>>> + return -EINVAL;
>
> I am concerned about the resources allocation here though,
> is returning EINVAL here right, vs using "goto fail"?
>
> I matched the behavior of the preceding call:
>
> bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
> BDRV_CHILD_IMAGE, false, errp);
> if (!bs->file) {
> return -EINVAL;
> }
>
> But afterwards the code goes:
> .
> /* locate the UDIF trailer */
> offset = dmg_find_koly_offset(bs->file, errp);
> if (offset < 0) {
> ret = offset;
> goto fail;
> }
>
> Should the resources be freed or not in your view?
Functions should generally fail cleanly, and that means undoing side
effects such as allocations.
Typically, we undo in reverse order, and goto the right spot in that
sequence.
When the undo can be made to work whether the "do" happened or not, we
can use fewer labels for simplicity. For instance, g_free(mumble) works
as long as mumble is initialized to NULL.
In this function:
fail:
g_free(s->types);
g_free(s->offsets);
g_free(s->lengths);
g_free(s->sectors);
g_free(s->sectorcounts);
qemu_vfree(s->compressed_chunk);
qemu_vfree(s->uncompressed_chunk);
return ret;
I figure this undoes side effects hidden in the read functions called.
Potential issue before this patch: I can't see bdrv_open_child() being
undone. Shouldn't we close bs->file? And what about
bdrv_open_child()'s side effect on @options?
[...]
Am 27.09.2022 um 09:54 hat Markus Armbruster geschrieben: > Potential issue before this patch: I can't see bdrv_open_child() being > undone. Shouldn't we close bs->file? And what about > bdrv_open_child()'s side effect on @options? bs->file is handled by the caller, bdrv_open_driver(). options is going to be freed anyway, so I don't think we care about its content. And anyway, doesn't bdrv_open_child() only remove those entries from it that it processed, like in the success case? Kevin
On 9/27/22 09:54, Markus Armbruster wrote:
> Claudio Fontana <cfontana@suse.de> writes:
>
>> On 9/26/22 12:38, Kevin Wolf wrote:
>>> Am 24.09.2022 um 01:21 hat Claudio Fontana geschrieben:
>>>> improve error handling during module load, by changing:
>>>>
>>>> bool module_load(const char *prefix, const char *lib_name);
>>>> void module_load_qom(const char *type);
>>>>
>>>> to:
>>>>
>>>> int module_load(const char *prefix, const char *name, Error **errp);
>>>> int module_load_qom(const char *type, Error **errp);
>>>>
>>>> where the return value is:
>>>>
>>>> -1 on module load error, and errp is set with the error
>>>> 0 on module or one of its dependencies are not installed
>>>> 1 on module load success
>>>> 2 on module load success (module already loaded or built-in)
>>>>
>>>> module_load_qom_one has been introduced in:
>>>>
>>>> commit 28457744c345 ("module: qom module support"), which built on top of
>>>> module_load_one, but discarded the bool return value. Restore it.
>>>>
>>>> Adapt all callers to emit errors, or ignore them, or fail hard,
>>>> as appropriate in each context.
>>>>
>>>> Some memory leaks also fixed as part of the module_load changes.
>>>>
>>>> audio: when attempting to load an audio module, report module load errors.
>>>> block: when attempting to load a block module, report module load errors.
>>>> console: when attempting to load a display module, report module load errors.
>>>>
>>>> qdev: when creating a new qdev Device object (DeviceState), report load errors.
>>>> If a module cannot be loaded to create that device, now abort execution.
>>>>
>>>> qom/object.c: when initializing a QOM object, or looking up class_by_name,
>>>> report module load errors.
>>>>
>>>> qtest: when processing the "module_load" qtest command, report errors
>>>> in the load of the module.
>>>>
>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>
>>>> diff --git a/block/dmg.c b/block/dmg.c
>>>> index 007b8d9996..a422cf8d5b 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,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>>>> return -EINVAL;
>>>> }
>>>>
>>>> - block_module_load("dmg-bz2");
>>>> - block_module_load("dmg-lzfse");
>>>> + if (block_module_load("dmg-bz2", &local_err) < 0) {
>>>> + error_report_err(local_err);
>>>> + return -EINVAL;
>>>> + }
>>>> + local_err = NULL;
>>>> + if (block_module_load("dmg-lzfse", &local_err) < 0) {
>>>> + error_report_err(local_err);
>>>> + return -EINVAL;
>>
>> I am concerned about the resources allocation here though,
>> is returning EINVAL here right, vs using "goto fail"?
>>
>> I matched the behavior of the preceding call:
>>
>> bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
>> BDRV_CHILD_IMAGE, false, errp);
>> if (!bs->file) {
>> return -EINVAL;
>> }
>>
>> But afterwards the code goes:
>> .
>> /* locate the UDIF trailer */
>> offset = dmg_find_koly_offset(bs->file, errp);
>> if (offset < 0) {
>> ret = offset;
>> goto fail;
>> }
>>
>> Should the resources be freed or not in your view?
>
> Functions should generally fail cleanly, and that means undoing side
> effects such as allocations.
This is pretty obvious.
Note that this function does not perform the allocations,
bdrv_open is called with existing allocated resources,
and then frees those resources only in specific code paths.
I am asking here because the code preceding my hunk is doing:
"
ret = bdrv_apply_auto_read_only(bs, NULL, errp);
if (ret < 0) {
return ret;
}
bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_IMAGE, false, errp);
if (!bs->file) {
return -EINVAL;
}
"
So something seems fishy here, looks inconsistent with the successive code that instead frees bs resources on error.
Might be correct, but would need looking deeper into this by block-aware people. Kevin?
Am 27.09.2022 um 11:13 hat Claudio Fontana geschrieben:
> On 9/27/22 09:54, Markus Armbruster wrote:
> > Claudio Fontana <cfontana@suse.de> writes:
> >
> >> On 9/26/22 12:38, Kevin Wolf wrote:
> >>> Am 24.09.2022 um 01:21 hat Claudio Fontana geschrieben:
> >>>> improve error handling during module load, by changing:
> >>>>
> >>>> bool module_load(const char *prefix, const char *lib_name);
> >>>> void module_load_qom(const char *type);
> >>>>
> >>>> to:
> >>>>
> >>>> int module_load(const char *prefix, const char *name, Error **errp);
> >>>> int module_load_qom(const char *type, Error **errp);
> >>>>
> >>>> where the return value is:
> >>>>
> >>>> -1 on module load error, and errp is set with the error
> >>>> 0 on module or one of its dependencies are not installed
> >>>> 1 on module load success
> >>>> 2 on module load success (module already loaded or built-in)
> >>>>
> >>>> module_load_qom_one has been introduced in:
> >>>>
> >>>> commit 28457744c345 ("module: qom module support"), which built on top of
> >>>> module_load_one, but discarded the bool return value. Restore it.
> >>>>
> >>>> Adapt all callers to emit errors, or ignore them, or fail hard,
> >>>> as appropriate in each context.
> >>>>
> >>>> Some memory leaks also fixed as part of the module_load changes.
> >>>>
> >>>> audio: when attempting to load an audio module, report module load errors.
> >>>> block: when attempting to load a block module, report module load errors.
> >>>> console: when attempting to load a display module, report module load errors.
> >>>>
> >>>> qdev: when creating a new qdev Device object (DeviceState), report load errors.
> >>>> If a module cannot be loaded to create that device, now abort execution.
> >>>>
> >>>> qom/object.c: when initializing a QOM object, or looking up class_by_name,
> >>>> report module load errors.
> >>>>
> >>>> qtest: when processing the "module_load" qtest command, report errors
> >>>> in the load of the module.
> >>>>
> >>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> >>>
> >>>> diff --git a/block/dmg.c b/block/dmg.c
> >>>> index 007b8d9996..a422cf8d5b 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,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> >>>> return -EINVAL;
> >>>> }
> >>>>
> >>>> - block_module_load("dmg-bz2");
> >>>> - block_module_load("dmg-lzfse");
> >>>> + if (block_module_load("dmg-bz2", &local_err) < 0) {
> >>>> + error_report_err(local_err);
> >>>> + return -EINVAL;
> >>>> + }
> >>>> + local_err = NULL;
> >>>> + if (block_module_load("dmg-lzfse", &local_err) < 0) {
> >>>> + error_report_err(local_err);
> >>>> + return -EINVAL;
> >>
> >> I am concerned about the resources allocation here though,
> >> is returning EINVAL here right, vs using "goto fail"?
> >>
> >> I matched the behavior of the preceding call:
> >>
> >> bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
> >> BDRV_CHILD_IMAGE, false, errp);
> >> if (!bs->file) {
> >> return -EINVAL;
> >> }
> >>
> >> But afterwards the code goes:
> >> .
> >> /* locate the UDIF trailer */
> >> offset = dmg_find_koly_offset(bs->file, errp);
> >> if (offset < 0) {
> >> ret = offset;
> >> goto fail;
> >> }
> >>
> >> Should the resources be freed or not in your view?
> >
> > Functions should generally fail cleanly, and that means undoing side
> > effects such as allocations.
>
> This is pretty obvious.
>
> Note that this function does not perform the allocations,
> bdrv_open is called with existing allocated resources,
> and then frees those resources only in specific code paths.
These aren't allocated, the caller just allocates the BDRVDMGState and
initialises it with zeros. In other words, at this point, the free
functions don't do anything yet and you may or may not call them, it
doesn't matter.
We have an explicit line like this after the module loading:
s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
It's completely unnecessary because the BDRVDMGState is already
initialised like this, but if you think of this as the point where these
fields come to life, using a direct return before it and 'goto fail'
afterwards makes a little bit of sense at least.
As far as I can tell, only dmg_read_resource_fork() or
dmg_read_plist_xml() actually allocate some memeory that needs to be
undone, so some of the existing 'goto fail' instances could be direct
returns. But they're not wrong either, so no need to change them.
Kevin
On 9/27/22 13:53, Kevin Wolf wrote:
> Am 27.09.2022 um 11:13 hat Claudio Fontana geschrieben:
>> On 9/27/22 09:54, Markus Armbruster wrote:
>>> Claudio Fontana <cfontana@suse.de> writes:
>>>
>>>> On 9/26/22 12:38, Kevin Wolf wrote:
>>>>> Am 24.09.2022 um 01:21 hat Claudio Fontana geschrieben:
>>>>>> improve error handling during module load, by changing:
>>>>>>
>>>>>> bool module_load(const char *prefix, const char *lib_name);
>>>>>> void module_load_qom(const char *type);
>>>>>>
>>>>>> to:
>>>>>>
>>>>>> int module_load(const char *prefix, const char *name, Error **errp);
>>>>>> int module_load_qom(const char *type, Error **errp);
>>>>>>
>>>>>> where the return value is:
>>>>>>
>>>>>> -1 on module load error, and errp is set with the error
>>>>>> 0 on module or one of its dependencies are not installed
>>>>>> 1 on module load success
>>>>>> 2 on module load success (module already loaded or built-in)
>>>>>>
>>>>>> module_load_qom_one has been introduced in:
>>>>>>
>>>>>> commit 28457744c345 ("module: qom module support"), which built on top of
>>>>>> module_load_one, but discarded the bool return value. Restore it.
>>>>>>
>>>>>> Adapt all callers to emit errors, or ignore them, or fail hard,
>>>>>> as appropriate in each context.
>>>>>>
>>>>>> Some memory leaks also fixed as part of the module_load changes.
>>>>>>
>>>>>> audio: when attempting to load an audio module, report module load errors.
>>>>>> block: when attempting to load a block module, report module load errors.
>>>>>> console: when attempting to load a display module, report module load errors.
>>>>>>
>>>>>> qdev: when creating a new qdev Device object (DeviceState), report load errors.
>>>>>> If a module cannot be loaded to create that device, now abort execution.
>>>>>>
>>>>>> qom/object.c: when initializing a QOM object, or looking up class_by_name,
>>>>>> report module load errors.
>>>>>>
>>>>>> qtest: when processing the "module_load" qtest command, report errors
>>>>>> in the load of the module.
>>>>>>
>>>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>>>
>>>>>> diff --git a/block/dmg.c b/block/dmg.c
>>>>>> index 007b8d9996..a422cf8d5b 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,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>>>>>> return -EINVAL;
>>>>>> }
>>>>>>
>>>>>> - block_module_load("dmg-bz2");
>>>>>> - block_module_load("dmg-lzfse");
>>>>>> + if (block_module_load("dmg-bz2", &local_err) < 0) {
>>>>>> + error_report_err(local_err);
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> + local_err = NULL;
>>>>>> + if (block_module_load("dmg-lzfse", &local_err) < 0) {
>>>>>> + error_report_err(local_err);
>>>>>> + return -EINVAL;
>>>>
>>>> I am concerned about the resources allocation here though,
>>>> is returning EINVAL here right, vs using "goto fail"?
>>>>
>>>> I matched the behavior of the preceding call:
>>>>
>>>> bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
>>>> BDRV_CHILD_IMAGE, false, errp);
>>>> if (!bs->file) {
>>>> return -EINVAL;
>>>> }
>>>>
>>>> But afterwards the code goes:
>>>> .
>>>> /* locate the UDIF trailer */
>>>> offset = dmg_find_koly_offset(bs->file, errp);
>>>> if (offset < 0) {
>>>> ret = offset;
>>>> goto fail;
>>>> }
>>>>
>>>> Should the resources be freed or not in your view?
>>>
>>> Functions should generally fail cleanly, and that means undoing side
>>> effects such as allocations.
>>
>> This is pretty obvious.
>>
>> Note that this function does not perform the allocations,
>> bdrv_open is called with existing allocated resources,
>> and then frees those resources only in specific code paths.
>
> These aren't allocated, the caller just allocates the BDRVDMGState and
> initialises it with zeros. In other words, at this point, the free
> functions don't do anything yet and you may or may not call them, it
> doesn't matter.
>
> We have an explicit line like this after the module loading:
>
> s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
>
> It's completely unnecessary because the BDRVDMGState is already
> initialised like this, but if you think of this as the point where these
> fields come to life, using a direct return before it and 'goto fail'
> afterwards makes a little bit of sense at least.
>
> As far as I can tell, only dmg_read_resource_fork() or
> dmg_read_plist_xml() actually allocate some memeory that needs to be
> undone, so some of the existing 'goto fail' instances could be direct
> returns. But they're not wrong either, so no need to change them.
>
> Kevin
>
Thanks Kevin,
given this for my hunk I'll keep returning immediately with EINVAL then.
Claudio
On 9/26/22 12:38, Kevin Wolf wrote:
> Am 24.09.2022 um 01:21 hat Claudio Fontana geschrieben:
>> improve error handling during module load, by changing:
>>
>> bool module_load(const char *prefix, const char *lib_name);
>> void module_load_qom(const char *type);
>>
>> to:
>>
>> int module_load(const char *prefix, const char *name, Error **errp);
>> int module_load_qom(const char *type, Error **errp);
>>
>> where the return value is:
>>
>> -1 on module load error, and errp is set with the error
>> 0 on module or one of its dependencies are not installed
>> 1 on module load success
>> 2 on module load success (module already loaded or built-in)
>>
>> module_load_qom_one has been introduced in:
>>
>> commit 28457744c345 ("module: qom module support"), which built on top of
>> module_load_one, but discarded the bool return value. Restore it.
>>
>> Adapt all callers to emit errors, or ignore them, or fail hard,
>> as appropriate in each context.
>>
>> Some memory leaks also fixed as part of the module_load changes.
>>
>> audio: when attempting to load an audio module, report module load errors.
>> block: when attempting to load a block module, report module load errors.
>> console: when attempting to load a display module, report module load errors.
>>
>> qdev: when creating a new qdev Device object (DeviceState), report load errors.
>> If a module cannot be loaded to create that device, now abort execution.
>>
>> qom/object.c: when initializing a QOM object, or looking up class_by_name,
>> report module load errors.
>>
>> qtest: when processing the "module_load" qtest command, report errors
>> in the load of the module.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>
>> diff --git a/block/dmg.c b/block/dmg.c
>> index 007b8d9996..a422cf8d5b 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,15 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
>> return -EINVAL;
>> }
>>
>> - block_module_load("dmg-bz2");
>> - block_module_load("dmg-lzfse");
>> + if (block_module_load("dmg-bz2", &local_err) < 0) {
>> + error_report_err(local_err);
>> + return -EINVAL;
>> + }
>> + local_err = NULL;
>> + if (block_module_load("dmg-lzfse", &local_err) < 0) {
>> + error_report_err(local_err);
>> + return -EINVAL;
>> + }
>
> Why don't we pass the existing errp instead of adding a new local_err
> and printing it locally? If we use error_report_err(local_err) and leave
> errp untouched, the caller will print another less specific error
> message, which is generally not what we want.
>
> Kevin
>
Ok, you know that code best, will merge the two.
Thanks,
Claudio
© 2016 - 2026 Red Hat, Inc.