Re: [PATCH v3 2/3] module: add Error arguments to module_load_one and module_load_qom_one

[Kevin Wolf <kwolf@redhat.com>]

Am 08.09.2022 um 19:36 hat Claudio Fontana geschrieben:
> On 9/8/22 19:10, Claudio Fontana wrote:
> > On 9/8/22 18:03, Richard Henderson wrote:
> >> On 9/8/22 15:53, Claudio Fontana wrote:
> >>> @@ -446,8 +447,13 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
> >>>           return -EINVAL;
> >>>       }
> >>>   
> >>> -    block_module_load_one("dmg-bz2");
> >>> -    block_module_load_one("dmg-lzfse");
> >>> +    if (!block_module_load_one("dmg-bz2", &local_err) && local_err) {
> >>> +        error_report_err(local_err);
> >>> +    }
> >>> +    local_err = NULL;
> >>> +    if (!block_module_load_one("dmg-lzfse", &local_err) && local_err) {
> >>> +        error_report_err(local_err);
> >>> +    }
> >>>   
> >>>       s->n_chunks = 0;
> >>>       s->offsets = s->lengths = s->sectors = s->sectorcounts = NULL;
> >>
> >> I wonder if these shouldn't fail hard if the modules don't exist?
> >> Or at least pass back the error.
> >>
> >> Kevin?
> 
> is "dmg-bz" _required_ for dmg open to work? I suspect if the dmg
> image is not compressed, "dmg" can function even if the extra dmg-bz
> module is not loaded right?

Indeed. The code seems to consider that the modules may not be present.
The behaviour in these cases is questionable (it seems to silently leave
the buffers as they are and return success), but the modules are clearly
optional.

> I'd suspect we should then do:
> 
> if (!block_module_load_one("dmg-bz2", &local_err)) {
>   if (local_err) {
>      error_report_err(local_err);
>      return -EINVAL;
>   }
>   warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks */
> }
> 
> and same for dmg-lzfse...?

Actually, I think during initialisation, we should just pass NULL as
errp and ignore any errors.

When a request would access a block that can't be uncompressed because
of the missing module, that's where we can have a warn_report_once() and
arguably should fail the I/O request.

Kevin