On 9/27/22 09:54, Markus Armbruster wrote: > Claudio Fontana <cfont...@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 <cfont...@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?