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

[Max Reitz]

Ping

<bait>just the final three patches left to review</bait>


On 22.05.19 19:03, Max Reitz wrote:
> 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 penultimate 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.
> 
> 
> v3:
> - Received no reply to my “Hm, warnings break 'make check', so maybe we
>   should just keep quiet if loosening restrictions fails?” question, so
>   I assume silence means agreement.  Changed patch 7 accordingly.
> 
> - Added a test: The fact how make check kind-of-but-not-really broke
>   showed a nice reproducer: Launching qemu with some file, then deleting
>   that file, then quitting qemu.
> 
> - Rebase “conflict” in patch 6: The forward declaration of
>   bdrv_get_cumulative_perm() is already in qemu thanks to commit
>   481e0eeef4f.
> 
> 
> git-backport-diff against v2:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, 
> respectively
> 
> 001/8:[----] [--] 'file-posix: Update open_flags in raw_set_perm()'
> 002/8:[----] [--] 'block: Add bdrv_child_refresh_perms()'
> 003/8:[----] [--] 'block/mirror: Fix child permissions'
> 004/8:[----] [--] 'block/commit: Drop bdrv_child_try_set_perm()'
> 005/8:[0018] [FC] 'block: Fix order in bdrv_replace_child()'
>        ^^^^ Again confuses my v2 patch with 8aecf1d1bd250a, should be:
>       [----] : patches are identical
> 006/8:[0002] [FC] 'block: Add *tighten_restrictions to *check*_perm()'
> 007/8:[0018] [FC] 'block: Ignore loosening perm restrictions failures'
> 008/8:[down] 'iotests: Test failure to loosen restrictions'
> 
> 
> Max Reitz (8):
>   file-posix: Update open_flags in raw_set_perm()
>   block: Add bdrv_child_refresh_perms()
>   block/mirror: Fix child permissions
>   block/commit: Drop bdrv_child_try_set_perm()
>   block: Fix order in bdrv_replace_child()
>   block: Add *tighten_restrictions to *check*_perm()
>   block: Ignore loosening perm restrictions failures
>   iotests: Test failure to loosen restrictions
> 
>  include/block/block_int.h  |  15 ++++
>  block.c                    | 153 +++++++++++++++++++++++++++++++------
>  block/commit.c             |   2 -
>  block/file-posix.c         |   4 +
>  block/mirror.c             |  32 +++++---
>  tests/qemu-iotests/182     |  21 +++++
>  tests/qemu-iotests/182.out |   6 ++
>  7 files changed, 198 insertions(+), 35 deletions(-)
>

signature.asc
Description: OpenPGP digital signature