Re: [PATCH] block: Fix bdrv_activate() not to fail without medium

[Kevin Wolf <kwolf@redhat.com>]

Am 13.03.2025 um 12:53 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 12.03.2025 um 15:37 hat Markus Armbruster geschrieben:
> >> bdrv_activate() returns failure without setting an error when
> >> !bs->drv.  This is suspicious.  Turns out it used to succeed then,
> >> until commit 5416645fcf82 changed it to return -ENOMEDIUM.
> >> 
> >> Return zero instead.
> >> 
> >> Fixes: 5416645fcf82 (block: return error-code from bdrv_invalidate_cache)
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > The commit message sounds more theoretical. Did you find this only by
> > code inspection? Do we know what the effect on real-world cases is, so
> > we could add a sentence about it to the commit message? Maybe we could
> > even have a qemu-iotests case to show the effect?
> >
> > I absolutely agree that returning -ENOMEDIUM while not setting errp is
> > wrong. But without an example of what is affected, it's not obvious to
> > me which part of it needs to be fixed.
> 
> Code inspection.
> 
> Here's my somewhat extended rationale for my fix.
> [...]

> Not failing the function on !bs->drv is clearly intentional.
> 
> Behavior stayed this way for more than six years.  Then commit
> 5416645fcf8 (block: return error-code from bdrv_invalidate_cache)
> changed the function to return zero on success, a negative errno on
> failure.  According to the commit message, the patch is mere cleanup,
> and not supposed to change behavior.
> 
> Since the first return was a success before the patch (no error set),
> the correct value to return was zero.  The patch used -ENOMEDIUM
> instead.  This is a clear regression.
> 
> My patch restores previous behavior.

I understand your rationale and don't disagree with your patch.
But I would still like the commit message to explain the practical
consequences of the bug and if possible a test case.

If you tried to find the practical consequences and couldn't find any
way to trigger a bug as a user, that is worth documenting, too.

Kevin