On 08.05.19 20:25, Max Reitz wrote: > We generally assume that loosening permission restrictions can never > fail. We have seen in the past that this assumption is wrong. This has > led to crashes because we generally pass &error_abort when loosening > permissions. > > However, a failure in such a case should actually be handled in quite > the opposite way: It is very much not fatal, so qemu may report it, but > still consider the operation successful. The only realistic problem is > that qemu may then retain permissions and thus locks on images it > actually does not require. But again, that is not fatal. > > To implement this behavior, we make all functions that change > permissions and that pass &error_abort to the initiating function > (bdrv_check_perm() or bdrv_child_check_perm()) evaluate the > @loosen_restrictions value introduced in the previous patch. If it is > true and an error did occur, we abort the permission update, report > the error, and report success to the caller.
Hm, I just noticed that while make check passes, it emits two of these warnings: TEST: tests/i440fx-test... (pid=23021) qemu-system-x86_64: warning: Failed to loosen restrictions: Could not reopen file: No such file or directory qemu-system-x86_64: warning: Failed to loosen restrictions: Could not reopen file: No such file or directory PASS: tests/i440fx-test This is because the test creates temporary files which it unlinks after qemu has opened them. The bdrv_close_all() then attempts to drop the WRITE permission, which requires reopening the file, which fails. (Reproducer: $ touch /tmp/foo; x86_64-softmmu/qemu-system-x86_64 -hda /tmp/foo &; \ sleep 1; rm /tmp/foo; kill %1 [1] 23236 WARNING: Image format was not specified for '/tmp/foo' and probing guessed raw. Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted. Specify the 'raw' format explicitly to remove the restrictions. qemu-system-x86_64: terminating on signal 15 from pid 7922 (-zsh) qemu-system-x86_64: warning: Failed to loosen restrictions: Could not reopen file: No such file or directory qemu-system-x86_64: warning: Failed to loosen restrictions: Could not reopen file: No such file or directory [1] + 23236 done x86_64-softmmu/qemu-system-x86_64 -hda /tmp/foo ) Should I just drop the warning? Max > bdrv_child_try_set_perm() itself does not pass &error_abort, but it is > the only public function to change permissions. As such, callers may > pass &error_abort to it, expecting dropping permission restrictions to > never fail. > > Signed-off-by: Max Reitz <mre...@redhat.com> > --- > block.c | 40 ++++++++++++++++++++++++++++++++++------ > 1 file changed, 34 insertions(+), 6 deletions(-) > > diff --git a/block.c b/block.c > index 8f517be5b4..a5a03906d0 100644 > --- a/block.c > +++ b/block.c > @@ -2088,11 +2088,20 @@ static void bdrv_child_abort_perm_update(BdrvChild *c) > int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared, > Error **errp) > { > + Error *local_err = NULL; > int ret; > + bool tighten_restrictions; > > - ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, NULL, errp); > + ret = bdrv_child_check_perm(c, NULL, perm, shared, NULL, > + &tighten_restrictions, &local_err); > if (ret < 0) { > bdrv_child_abort_perm_update(c); > + if (tighten_restrictions) { > + error_propagate(errp, local_err); > + } else { > + warn_reportf_err(local_err, "Failed to loosen restrictions: "); > + ret = 0; > + } > return ret; > } > > @@ -2275,10 +2284,20 @@ static void bdrv_replace_child(BdrvChild *child, > BlockDriverState *new_bs) > /* Update permissions for old node. This is guaranteed to succeed > * because we're just taking a parent away, so we're loosening > * restrictions. */ > + bool tighten_restrictions; > + Error *local_err = NULL; > + int ret; > + > bdrv_get_cumulative_perm(old_bs, &perm, &shared_perm); > - bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL, > - NULL, &error_abort); > - bdrv_set_perm(old_bs, perm, shared_perm); > + ret = bdrv_check_perm(old_bs, NULL, perm, shared_perm, NULL, > + &tighten_restrictions, &local_err); > + assert(tighten_restrictions == false); > + if (ret < 0) { > + warn_reportf_err(local_err, "Failed to loosen restrictions: "); > + bdrv_abort_perm_update(old_bs); > + } else { > + bdrv_set_perm(old_bs, perm, shared_perm); > + } > } > } > > @@ -5322,7 +5341,9 @@ static bool bdrv_has_bds_parent(BlockDriverState *bs, > bool only_active) > static int bdrv_inactivate_recurse(BlockDriverState *bs) > { > BdrvChild *child, *parent; > + bool tighten_restrictions; > uint64_t perm, shared_perm; > + Error *local_err = NULL; > int ret; > > if (!bs->drv) { > @@ -5358,8 +5379,15 @@ static int bdrv_inactivate_recurse(BlockDriverState > *bs) > > /* Update permissions, they may differ for inactive nodes */ > bdrv_get_cumulative_perm(bs, &perm, &shared_perm); > - bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, &error_abort); > - bdrv_set_perm(bs, perm, shared_perm); > + ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, > + &tighten_restrictions, &local_err); > + assert(tighten_restrictions == false); > + if (ret < 0) { > + warn_reportf_err(local_err, "Failed to loosen restrictions: "); > + bdrv_abort_perm_update(bs); > + } else { > + bdrv_set_perm(bs, perm, shared_perm); > + } > > > /* Recursively inactivate children */ >
signature.asc
Description: OpenPGP digital signature