[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3 0/8] block: Ignore loosening perm restriction
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v3 0/8] block: Ignore loosening perm restrictions failures |
Date: |
Thu, 13 Jun 2019 21:44:19 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
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
- Re: [Qemu-block] [PATCH v3 0/8] block: Ignore loosening perm restrictions failures,
Max Reitz <=