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

[Claudio Fontana]

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?