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

[Claudio Fontana]

On 9/27/22 13:53, Kevin Wolf wrote:
> Am 27.09.2022 um 11:13 hat Claudio Fontana geschrieben:
>> 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.
> 
> These aren't allocated, the caller just allocates the BDRVDMGState and
> initialises it with zeros. In other words, at this point, the free
> functions don't do anything yet and you may or may not call them, it
> doesn't matter.
> 
> We have an explicit line like this after the module loading:
> 
>     s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> 
> It's completely unnecessary because the BDRVDMGState is already
> initialised like this, but if you think of this as the point where these
> fields come to life, using a direct return before it and 'goto fail'
> afterwards makes a little bit of sense at least.
> 
> As far as I can tell, only dmg_read_resource_fork() or
> dmg_read_plist_xml() actually allocate some memeory that needs to be
> undone, so some of the existing 'goto fail' instances could be direct
> returns. But they're not wrong either, so no need to change them.
> 
> Kevin
> 

Thanks Kevin,

given this for my hunk I'll keep returning immediately with EINVAL then.

Claudio