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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> 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.

True for immortals.  Since we're not, it actually depends on
probabilities.

> 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.

Fair point.

>> 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.

In most cases, "open fails because some blocks are known to be
unreadable" is much better UX than "everything goes swimmingly until you
try to read one of the known-unreadable blocks".

Even when your software manages not to eat your data, surprise bad
blocks are still likely to result in a bad day.

> 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.

Makes sense.