Am 13.03.2025 um 12:53 hat Markus Armbruster geschrieben: > Kevin Wolf <kw...@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 <arm...@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
