29.03.2019 14:08, Kevin Wolf wrote: > Am 29.03.2019 um 11:55 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 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. > > Not exactly the same, but a subset of the old ones. > >> 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. > > Okay, so the plan for v3 is to drop the raw_check_lock_bytes() call > here, and add an assertion in block.c, right?
but where to assert? No I think - nowhere. We'll abort on error_abort anyway. And what to assert? We can't do such check in common bdrv_check_perm, as we don't keep previous cumulative permissions.. So I think, my v3 is OK. -- Best regards, Vladimir
