Am 29.03.2019 um 10:53 hat Vladimir Sementsov-Ogievskiy geschrieben: > 28.03.2019 21:40, Kevin Wolf wrote: > > Am 28.03.2019 um 08:21 hat Vladimir Sementsov-Ogievskiy geschrieben: > >> bdrv_replace_child() calls bdrv_check_perm() with error_abort on > >> loosening permissions. However file-locking operations may fail even > >> in this case, for example on NFS. And this leads to Qemu crash. > >> > >> Let's ignore such errors, as we do already on permission update commit > >> and abort. > >> > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > > > > I think this would better be fixed in block.c code so that unlock never > > fails for any block driver. > > Hmm. We now only have one .bdrv_check_perm handler - raw_check_perm. And > in this particular case, yes, the only thing we can do is ignoring error > and do not fail on loosening permissions.. > > If we have more drivers with this callback, what should be the common > behavior? > > Do you propose to ignore .bdrv_check_perm errors in common case? > > Isn't it better to require, that .bdrv_check_perm handler do not fail on > loosening permissions, and abort if it fail in this case, like it actually > works after this patch?
Maybe an assertion in the common code is actually better, yes. I do think that the common behaviour we want is to ignore .bdrv_check_perm errors for unlock, but it might be surprising for drivers that .bdrv_set_perm is called when .bdrv_check_perm failed. So we need the drivers to be aware of the problem anyway. Back to your patch: Why do you need to call raw_check_lock_bytes() in the unlock case? We don't acquire any new permissions and hold the locks for everything, so nobody else should have taken a conflicting lock. Kevin
