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

[Claudio Fontana <cfontana@suse.de>]

On 9/23/22 16:42, Kevin Wolf wrote:
> Am 23.09.2022 um 16:10 hat Claudio Fontana geschrieben:
>> 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?
> 
> Sure, feel free, though I don't think the code will otherwise change for
> dmg, so we could as well continue here.
> 
> My conclusion was that only dmg_read_mish_block() or something called by
> it can know whether compressed blocks exist in the image when the
> modules aren't present. So if we want to make the warning conditional on
> that (and my understanding is correct), this is where a
> warn_report_once() would have to live.
> 
> Kevin
> 

I took a look, but I feel a bit too ignorant of the code there, maybe you could move the warning as a patch to the right place after the series?

Or give me the extra commit needed to move into the right place?

Ciao,

Claudio