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

[Kevin Wolf]

Am 21.09.2022 um 06:45 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > 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.
> 
> Seems like asking for data corruption.  To avoid it, the complete stack
> needs to handle I/O errors correctly.

If you have any component that doesn't handle I/O errors correctly, keep
it far away from your data because it _will_ cause corruption eventually.
The earlier it fails, the better for you.

I don't think we should put great effort into making fundamentally
broken software a little bit less broken in the corner case that you're
least likely to hit.

> Can we detect presence of compressed blocks on open?

We seem to read in the full metadata of the image in dmg_open(). So I
think it would be possible to detect it there.

dmg_read_mish_block() is what fills in s->types. However, it never fills
in types that it doesn't know (and it pretends it doesn't know the types
of compressed blocks whose module is not loaded). So instead of checking
it in dmg_open() after dmg_read_mish_block() has completed, you would
have to catch the situation already in dmg_read_mish_block() while
parsing the image file, which should be entirely doable if you want.

This is a change in dmg's behaviour, though, which is not the goal of
the proposed patch. So if we want to do that, it should be a separate
patch.

Kevin