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

[Max Reitz <mreitz@redhat.com>]

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(-)

-- 
2.21.0