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

[Kevin Wolf <kwolf@redhat.com>]

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