Re: [Qemu-devel] [PATCH 0/7] block: Ignore loosening perm restrictions failures

[Kevin Wolf]

Am 06.05.2019 um 21:47 hat Max Reitz geschrieben:
> Hi,
> 
> This series is mainly a fix for
> https://bugzilla.redhat.com/show_bug.cgi?id=1703793.  The problem
> described there is that mirroring to a gluster volume, then switching
> off the volume makes qemu crash.  There are two problems here:
> 
> (1) file-posix reopens the FD all the time because it thinks the FD it
>     has is RDONLY.  It actually isn’t after the first reopen, we just
>     forgot to change the internal flags.  That’s what patch 1 is for.
> 
> (2) Even then, when mirror completes, it drops its write permission on
>     the FD.  This requires a reopen, which will fail if the volume is
>     down.  Mirror doesn’t expect that.  Nobody ever expects that
>     dropping permissions can fail, and rightfully so because that’s what
>     I think we have generally agreed on.
>     Therefore, the block layer should hide this error.  This is what the
>     last two patches are for.
> 
> The last patch adds two assertions: bdrv_replace_child() (for the old
> BDS) and bdrv_inactivate_recurse() assume they only ever drop
> assertions.  This is now substantiated by these new assertions.
> It turns out that this assumption was just plain wrong.  Patches 3 to 5
> make it right.

There are some places in this series that were a bit confusing (just
because the whole permission mechanism is rather confusing). I suggested
improvements for two patches, and deleted the half-written mails for the
rest. After all, it does look correct to me, so whether you want to
address my comments or not:

Reviewed-by: Kevin Wolf <address@hidden>