29.03.2019 13:55, Vladimir Sementsov-Ogievskiy wrote: > 29.03.2019 13:12, Kevin Wolf wrote: >> 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. >> > > Hmm.. it check not same arguments, so I didn't drop it as > raw_apply_lock_bytes. > > On the other hand, the only meaning of this raw_check_lock_bytes, is that > we'll > print error if it come (when it should not). > > Seems OK for me to drop it too and just return 0 immediately. > >
sent corresponding v3, renamed a bit: [PATCH v3] block/file-posix: do not fail on unlock bytes -- Best regards, Vladimir