Re: [Qemu-devel] [PATCH RFC] file-posix: make lock_fd read-only

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2344 bytes --]

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.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> Hi all!
> 
> We've faced the following problem with our shared-storage migration
> scheme. We make an external snapshot and need base image to be reopened
> RO. However, bdrv_reopen reopens only .fd of BDRVRawState but not
> .lock_fd. So, .lock_fd is left opened RW and this breaks the whole
> thing.

What exactly breaks?  Any specific error message, or a backtrace, or
something with more details?

> 
> The simple fix is here: let's just open lock_fd as RO always. This
> looks fine for current code, as we never try to set write locks
> (qemu_lock_fd always called with exclusive=false).

I just argued that we DO try to set write locks in file-posix, and
therefore, we need more details of what the real problem is, because
this does not feel like the right fix.

> 
> However it will not work if we are going to use write locks.
> 
> 
>  block/file-posix.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 36ee89e940..5c52df9174 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -514,7 +514,8 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>  
>      s->lock_fd = -1;
>      if (s->use_lock) {
> -        fd = qemu_open(filename, s->open_flags);
> +        /* open it read-only, as we do not reopen it on bdrv_reopen */
> +        fd = qemu_open(filename, (s->open_flags & ~BDRV_O_RDWR));
>          if (fd < 0) {
>              ret = -errno;
>              error_setg_errno(errp, errno, "Could not open '%s' for locking",
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]