Re: [Qemu-devel] [PATCH] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined

[Eric Blake]

On 7/29/19 7:27 PM, piaojun wrote:
> Use F_GETLK for fcntl when F_OFD_GETLK not defined.

Which system are you hitting this problem on?

The problem with F_GETLK is that it is NOT as safe as F_OFD_GETLK.

We already have fcntl_op_getlk and qemu_probe_lock_ops() in util/osdep.c
to not only determine which form to use, but also to emit a warning to
the end user if we had to fall back to the unsafe F_GETLK. Why is your
code not reusing that logic?

> 
> Signed-off-by: Jun Piao <address@hidden>
> ---
>  contrib/virtiofsd/passthrough_ll.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/contrib/virtiofsd/passthrough_ll.c 
> b/contrib/virtiofsd/passthrough_ll.c
> index 9ae1381..757785b 100644
> --- a/contrib/virtiofsd/passthrough_ll.c
> +++ b/contrib/virtiofsd/passthrough_ll.c
> @@ -1619,7 +1619,11 @@ static void lo_getlk(fuse_req_t req, fuse_ino_t ino,
>               return;
>       }
> 
> +#ifdef F_OFD_GETLK
>       ret = fcntl(plock->fd, F_OFD_GETLK, lock);
> +#else
> +     ret = fcntl(plock->fd, F_GETLK, lock);
> +#endif

Hmm. Since this is in contrib, you are trying to compile something that
is independent of util/osdep.c (at least, I assume that's the case, as
contrib/virtiofsd/ is not even part of qemu.git master yet - in which
case, why is this not being squashed in to the patch introducing that
file, rather than sent standalone).  On the other hand, that raises the
question - who is trying to use virtiofsd on a kernel that is too old to
provid F_OFD_GETLK?  Isn't the whole point of virtiofsd to be speeding
up modern usage, at which point an old kernel is just gumming up the
works?  It seems like you are better off letting compilation fail on a
system that is too old to support decent F_OFD_GETLK, rather than
silently falling back to something that is unsafe.

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

signature.asc
Description: OpenPGP digital signature