Re: [Qemu-devel] [Qemu-block] [PATCH] block/file-posix: ignore fail on unlock bytes

[Eric Blake]

On 3/27/19 3:33 PM, John Snow wrote:
> 
> 
> On 3/27/19 8:49 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 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 <address@hidden>
>> ---
>>  block/file-posix.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index db4cccbe51..403e67fe90 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -815,6 +815,20 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
>>  
>>      switch (op) {
>>      case RAW_PL_PREPARE:
>> +        if ((s->perm | new_perm) == s->perm &&

This says if new_perm does not add any bits beyond what s->perm had.

>> +            (~s->shared_perm | ~new_perm) == ~s->shared_perm)
> 
> Little strange to read, but ultimately "If we aren't changing anything"
> based on the call below.

'(~a | ~b)' is equivalent to '~(a & b)'.

'~(a & b) == ~a' is equivalent to '(a & b) == a'

That expression is much easier to read, as new_perm does not remove any
bits beyond what s->shared_perm already had.

But rewriting it in an easier form would indeed make the patch easier to
swallow.

> 
>> +        {
>> +            /*
>> +             * We are going to unlock bytes, it should not fail. If fail,
>> +             * just report it and ignore, like we do for ABORT and COMMIT
>> +             * anyway.
>> +             */
>> +            ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, 
>> &local_err);
>> +            if (local_err) {
>> +                error_report_err(local_err);
>> +            }
>> +            return 0;
>> +        }
>>          ret = raw_apply_lock_bytes(s, s->fd, s->perm | new_perm,
>>                                     ~s->shared_perm | ~new_shared,
>>                                     false, errp);
>>
> 
> I thiiiink this makes sense, but hopefully someone else can give it the
> once-over too.
> 
> Reviewed-by: John Snow <address@hidden>
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature