On 10/10/2017 02:30 PM, Denis V. Lunev wrote:
> On 10/10/2017 09:50 PM, Eric Blake wrote:
>> On 10/10/2017 08:42 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> We do not reopen lock_fd on bdrv_reopen which leads to problems on
>>> reopen image RO. So, lets make lock_fd be always RO.
>>> This is correct, because qemu_lock_fd always called with exclusive=false
>>> on lock_fd.
>> How is that correct?  file-posix.c calls:
>>             ret = qemu_lock_fd_test(s->lock_fd, off, 1, true);
>> where exclusive being true in turn sets:
>>         .l_type   = exclusive ? F_WRLCK : F_RDLCK,
>>
>> and F_WRLCK requests fail on a RO fd with EBADF.
> this works fine for us as here we do not get the lock but just
> query it.
> 

>         ftruncate(fd, 1024);
>         fcntl(fd, F_SETLK, &fl_rd);
>         fcntl(fd, F_GETLK, &fl_wr);

Trying with F_OFD_SETLK (which is more important, because it matches
what we prefer to use) gives the same results as your strace (remember
to #define _GNU_SOURCE 1 before compilation, to get F_OFD_SETLK into scope).

> 
> The key is that in test cod we do not _set_ the lock, but query it! This
> is allowed even on
> RDONLY file descriptor.

Hmm, you're right: file-posix only uses qemu_lock_fd() to set locks
(with exclusive == false), and qemu_lock_fd_test() to query locks
(there, exclusive == true means to probe if a write lock can be grabbed,
but does not actually grab one so it doesn't care whether the fd is RW.
We ).

We'd need a lot more explanation in the commit message (for example, you
still haven't stated an actual backtrace or error message you got when
the lock file is left RW), but you may have just convinced me that the
conversion to always use a RO lock fd is correct, at least for
file-posix' use of fcntl locking.

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