Re: [PATCH v6 3/5] module: add Error arguments to module_load and module_load_qom

[Kevin Wolf <kwolf@redhat.com>]

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