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

[Claudio Fontana]

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
>