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

Can we detect presence of compressed blocks on open?