On Fri, Sep 23, 2022 at 04:51:30PM +0200, Claudio Fontana wrote:
> improve error handling during module load, by changing:
>
> bool module_load(const char *prefix, const char *lib_name);
> void module_load_qom(const char *type);
>
> to:
>
> int module_load(const char *prefix, const char *name, Error **errp);
> int module_load_qom(const char *type, Error **errp);
>
> where the return value is:
>
> -1 on module load error, and errp is set with the error
> 0 on module or one of its dependencies are not installed
> 1 on module load success
> 2 on module load success (module already loaded or built-in)
> diff --git a/audio/audio.c b/audio/audio.c
> index 0a682336a0..ea51793843 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -72,20 +72,24 @@ void audio_driver_register(audio_driver *drv)
> audio_driver *audio_driver_lookup(const char *name)
> {
> struct audio_driver *d;
> + Error *local_err = NULL;
> + int rv;
>
> QLIST_FOREACH(d, &audio_drivers, next) {
> if (strcmp(name, d->name) == 0) {
> return d;
> }
> }
> -
> - audio_module_load(name);
> - QLIST_FOREACH(d, &audio_drivers, next) {
> - if (strcmp(name, d->name) == 0) {
> - return d;
> + rv = audio_module_load(name, &local_err);
> + if (rv > 0) {
> + QLIST_FOREACH(d, &audio_drivers, next) {
> + if (strcmp(name, d->name) == 0) {
> + return d;
> + }
> }
> + } else if (rv < 0) {
> + error_report_err(local_err);
> }
The rv == 0 case could be treated the same as rv > 0
meaning the diff merely needs to be
audio_module_load(name, &local_err)
if (rv < 0) {
error_report_err(local_err);
return NULL;.
}
> return NULL;
> }
>
> diff --git a/block.c b/block.c
> index 72c7f6d47d..0390ece056 100644
> --- a/block.c
> +++ b/block.c
> @@ -464,12 +464,18 @@ BlockDriver *bdrv_find_format(const char *format_name)
> /* The driver isn't registered, maybe we need to load a module */
> for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
> if (!strcmp(block_driver_modules[i].format_name, format_name)) {
> - block_module_load(block_driver_modules[i].library_name);
> + Error *local_err = NULL;
> + int rv = block_module_load(block_driver_modules[i].library_name,
> + &local_err);
> + if (rv > 0) {
> + return bdrv_do_find_format(format_name);
> + } else if (rv < 0) {
> + error_report_err(local_err);
> + }
Again, rv ==0 can be handled the same as rv > 0
> @@ -976,12 +982,17 @@ BlockDriver *bdrv_find_protocol(const char *filename,
> for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); ++i) {
> if (block_driver_modules[i].protocol_name &&
> !strcmp(block_driver_modules[i].protocol_name, protocol)) {
> - block_module_load(block_driver_modules[i].library_name);
> + Error *local_err = NULL;
> + int rv = block_module_load(block_driver_modules[i].library_name,
> &local_err);
> + if (rv > 0) {
> + drv1 = bdrv_do_find_protocol(protocol);
> + } else if (rv < 0) {
> + error_report_err(local_err);
> + }
Likewise rv == 0 vs rv > 0
> diff --git a/block/dmg.c b/block/dmg.c
> index 007b8d9996..e84a7a44a3 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -434,6 +434,8 @@ static int dmg_open(BlockDriverState *bs, QDict *options,
> int flags,
> uint64_t plist_xml_offset, plist_xml_length;
> int64_t offset;
> int ret;
> + int module_rv;
> + Error *local_err = NULL;
>
> ret = bdrv_apply_auto_read_only(bs, NULL, errp);
> if (ret < 0) {
> @@ -446,8 +448,21 @@ static int dmg_open(BlockDriverState *bs, QDict
> *options, int flags,
> return -EINVAL;
> }
>
> - block_module_load("dmg-bz2");
> - block_module_load("dmg-lzfse");
> + module_rv = block_module_load("dmg-bz2", &local_err);
> + if (module_rv < 0) {
> + error_report_err(local_err);
> + return -EINVAL;
> + } else if (module_rv == 0) {
> + warn_report("dmg-bz2 module not present, bz2 decomp unavailable");
> + }
> + local_err = NULL;
> + module_rv = block_module_load("dmg-lzfse", &local_err);
> + if (module_rv < 0) {
> + error_report_err(local_err);
> + return -EINVAL;
> + } else if (module_rv == 0) {
> + warn_report("dmg-lzfse module not present, lzfse decomp
> unavailable");
> + }
THis is the wrong place for these warnings, it'll spam
stdout, even if the file opened doesn't use bz2/lzfse.
The real problem is the later code which appears to
silently ignore data blocks if the lzfse/bz2 modules
are not loaded, instead of using error_report.
> diff --git a/qom/object.c b/qom/object.c
> index 4f834f3bf6..35ced55282 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -526,8 +526,13 @@ void object_initialize(void *data, size_t size, const
> char *typename)
>
> #ifdef CONFIG_MODULES
> if (!type) {
> - module_load_qom(typename);
> - type = type_get_by_name(typename);
> + Error *local_err = NULL;
> + int rv = module_load_qom(typename, &local_err);
> + if (rv > 0) {
> + type = type_get_by_name(typename);
> + } else if (rv < 0) {
> + error_report_err(local_err);
> + }
Again no need to distinguish rv == 0 from rv > 0
> @@ -1033,8 +1038,13 @@ ObjectClass *module_object_class_by_name(const char
> *typename)
> oc = object_class_by_name(typename);
> #ifdef CONFIG_MODULES
> if (!oc) {
> - module_load_qom(typename);
> - oc = object_class_by_name(typename);
> + Error *local_err = NULL;
> + int rv = module_load_qom(typename, &local_err);
> + if (rv > 0) {
> + oc = object_class_by_name(typename);
> + } else if (rv < 0) {
> + error_report_err(local_err);
> + }
Same comment
> diff --git a/softmmu/qtest.c b/softmmu/qtest.c
> index fc5b733c63..36e28609ff 100644
> --- a/softmmu/qtest.c
> +++ b/softmmu/qtest.c
> @@ -753,12 +753,18 @@ static void qtest_process_command(CharBackend *chr,
> gchar **words)
> qtest_sendf(chr, "OK %"PRIi64"\n",
> (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> } else if (strcmp(words[0], "module_load") == 0) {
> + Error *local_err = NULL;
> + int rv;
> g_assert(words[1] && words[2]);
>
> qtest_send_prefix(chr);
> - if (module_load(words[1], words[2])) {
> + rv = module_load(words[1], words[2], &local_err);
> + if (rv > 0) {
> qtest_sendf(chr, "OK\n");
> } else {
> + if (rv < 0) {
> + error_report_err(local_err);
> + }
> qtest_sendf(chr, "FAIL\n");
> }
This change means the 'module_load' command is totally silent
if 'rv == 0', but the code appears to try to read a response
line which will now never arrive AFAICT.
In the context of 'modules-test.c' I think it is fine to treat
rv == 0 the same as rv > 0 and thus print 'OK'.
Perhaps I've overlooked something, but I'm not seeing a reason
we need module_load() to return 4 different return values. It
looks like every caller would work with a boolean success/fail
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
