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

[Claudio Fontana]

On 9/21/22 13:56, Kevin Wolf wrote:
> Am 21.09.2022 um 09:50 hat Claudio Fontana geschrieben:
>> On 9/20/22 18:50, Kevin Wolf wrote:
>>> 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)
> 
> I think I misunderstood the code here actually. dmg_read_mish_block()
> skips chunks of unknown type, so later trying to find them fails and
> dmg_co_preadv() returns -EIO. Which is a reasonable return value for
> this.
> 
>>> , 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.
>>
>> Hmm really? I'd think that if there is an actual error loading the
>> module (module is installed, but the loading itself fails due to
>> broken module, wrong permissions, I/O errors etc) we would want to
>> report that fact as it happens?
> 
> Can we distinguish the two error cases?
> 
> Oooh... Reading the code again carefully, are you returning false
> without setting errp if the module just couldn't be found? This is a
> surprising interface.
> 
> Yes, I guess then your proposed code is fine (modulo moving
> warn_report() somewhere else so that it doesn't complain when the image
> doesn't even contain compressed chunks).
> 
>>> 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
>>>
>>
>> That would mean, moving the
>>
>> warn_report("dmg-bz2 is not present, dmg will skip bz2-compressed chunks")
>>
>> to the uncompression code and change it to a warn_report_once() right?
> 
> Yeah, though I think this doesn't actually work because we never even
> stored the metadata for chunks of unknown type (see above), so we never
> reach the uncompression code.
> 
> What misled me initially is this code in dmg_read_chunk():
> 
>         case UDBZ: /* bzip2 compressed */
>             if (!dmg_uncompress_bz2) {
>                 break;
>             }
> 
> I believe this is dead code, it could actually be an assertion. So
> if I'm not missing anything, adding the warning there would be useless.
> 
> The other option is moving it into dmg_is_known_block_type() or its
> caller dmg_read_mish_block(), then we would detect it during open, which
> is probably nicer anyway.
> 
> Kevin
> 
> 

Hi Kevin, I got a bit lost on whether we have some agreement on where if 
anywhere to move the check/warning about missing decompression submodules.

If that's ok I'd post a V5, and we can rediscuss from the new starting point?

Thanks,

Claudio