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

[Markus Armbruster]

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?

[...]