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

piaojun posted 1 patch 4 years, 8 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
contrib/virtiofsd/passthrough_ll.c | 9 +++++++++
1 file changed, 9 insertions(+)
[Qemu-devel] [PATCH] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined
Posted by piaojun 4 years, 8 months ago
Use F_GETLK for fcntl when F_OFD_GETLK not defined.

Signed-off-by: Jun Piao <piaojun@huawei.com>
---
 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
 	if (ret == -1)
 		saverr = errno;
 	pthread_mutex_unlock(&inode->plock_mutex);
@@ -1668,7 +1672,12 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino,

 	/* TODO: Is it alright to modify flock? */
 	lock->l_pid = 0;
+
+#ifdef F_OFD_GETLK
 	ret = fcntl(plock->fd, F_OFD_SETLK, lock);
+#else
+	ret = fcntl(plock->fd, F_SETLK, lock);
+#endif
 	if (ret == -1) {
 		saverr = errno;
 	}
-- 

Re: [Qemu-devel] [PATCH] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined
Posted by Liam Merwick 4 years, 8 months ago
On 30/07/2019 01:27, piaojun wrote:
> Use F_GETLK for fcntl when F_OFD_GETLK not defined.


Use F_GETLK/F_SETLK for fcntl when F_OFD_GETLK/F_OFD_SETLK not defined.

> 
> Signed-off-by: Jun Piao <piaojun@huawei.com>
> ---
>   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
>   	if (ret == -1)
>   		saverr = errno;
>   	pthread_mutex_unlock(&inode->plock_mutex);
> @@ -1668,7 +1672,12 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino,
> 
>   	/* TODO: Is it alright to modify flock? */
>   	lock->l_pid = 0;
> +
> +#ifdef F_OFD_GETLK


This checks for F_OFD_GETLK but uses F_OFD_SETLK (albeit unlikely one 
being defined without the other)


>   	ret = fcntl(plock->fd, F_OFD_SETLK, lock);
> +#else
> +	ret = fcntl(plock->fd, F_SETLK, lock);
> +#endif
>   	if (ret == -1) {
>   		saverr = errno;
>   	}
>
Might qemu_lock_fd()/qemu_unlock_fd() in util/osdep.c be useful to hide 
this instead?

Regards,
Liam

Re: [Qemu-devel] [PATCH] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined
Posted by piaojun 4 years, 8 months ago
Hi Liam,

On 2019/7/30 20:22, Liam Merwick wrote:
> On 30/07/2019 01:27, piaojun wrote:
>> Use F_GETLK for fcntl when F_OFD_GETLK not defined.
> 
> 
> Use F_GETLK/F_SETLK for fcntl when F_OFD_GETLK/F_OFD_SETLK not defined.
> 
>>
>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>> ---
>>   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
>>       if (ret == -1)
>>           saverr = errno;
>>       pthread_mutex_unlock(&inode->plock_mutex);
>> @@ -1668,7 +1672,12 @@ static void lo_setlk(fuse_req_t req, fuse_ino_t ino,
>>
>>       /* TODO: Is it alright to modify flock? */
>>       lock->l_pid = 0;
>> +
>> +#ifdef F_OFD_GETLK
> 
> 
> This checks for F_OFD_GETLK but uses F_OFD_SETLK (albeit unlikely one being defined without the other)

Right, F_OFD_SETLK looks better.

> 
> 
>>       ret = fcntl(plock->fd, F_OFD_SETLK, lock);
>> +#else
>> +    ret = fcntl(plock->fd, F_SETLK, lock);
>> +#endif
>>       if (ret == -1) {
>>           saverr = errno;
>>       }
>>
> Might qemu_lock_fd()/qemu_unlock_fd() in util/osdep.c be useful to hide this instead?

Yes, I'm trying re-write do_getlk/setlk with qemu_lock_fd, and thanks for
your suggestion.

Thanks,
Jun

> 
> Regards,
> Liam
> 

Re: [Qemu-devel] [PATCH] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined
Posted by Eric Blake 4 years, 8 months ago
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 <piaojun@huawei.com>
> ---
>  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

Re: [Qemu-devel] [PATCH] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined
Posted by piaojun 4 years, 8 months ago
Hi Eric,

On 2019/7/30 21:28, Eric Blake wrote:
> 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?

virtiofsd compile error due to missed 'F_OFD_GETLK'. My kernel version is
3.10.

linux-PUALcm:/home/code/virtiofs/qemu # make -j 8 virtiofsd

make[1]: Entering directory `/home/code/virtiofs/qemu/slirp'
make[1]: Nothing to be done for `all'.
make[1]: Leaving directory `/home/code/virtiofs/qemu/slirp'
	CHK version_gen.h
  CC      contrib/virtiofsd/passthrough_ll.o
  CC      qapi/qapi-clone-visitor.o
  CC      qapi/qmp-event.o
  CC      qapi/qapi-util.o
  CC      qapi/qapi-builtin-types.o
  CC      qapi/qapi-types-audio.o
  CC      qapi/qapi-types-authz.o
  CC      qapi/qapi-types-block-core.o
contrib/virtiofsd/passthrough_ll.c: In function ‘lo_getlk’:
contrib/virtiofsd/passthrough_ll.c:1622:25: error: ‘F_OFD_GETLK’ undeclared (first use in this function)
  ret = fcntl(plock->fd, F_OFD_GETLK, lock);
                         ^
contrib/virtiofsd/passthrough_ll.c:1622:25: note: each undeclared identifier is reported only once for each function it appears in
contrib/virtiofsd/passthrough_ll.c: In function ‘lo_setlk’:
contrib/virtiofsd/passthrough_ll.c:1671:25: error: ‘F_OFD_SETLK’ undeclared (first use in this function)

> 
>>
>> Signed-off-by: Jun Piao <piaojun@huawei.com>
>> ---
>>  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.

Perhaps reusing qemu_lock_fd() looks a better way to solve this.

Thanks,
Jun

> 


Re: [Qemu-devel] [Virtio-fs] [PATCH] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined
Posted by Dr. David Alan Gilbert 4 years, 7 months ago
* Eric Blake (eblake@redhat.com) wrote:
> 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 <piaojun@huawei.com>
> > ---
> >  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.

It is, but I guess the answer here is someone wanted to build on RHEL7.

Dave

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




> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [Virtio-fs] [PATCH] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined
Posted by Dr. David Alan Gilbert 4 years, 7 months ago
* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * Eric Blake (eblake@redhat.com) wrote:
> > 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 <piaojun@huawei.com>
> > > ---
> > >  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.
> 
> It is, but I guess the answer here is someone wanted to build on RHEL7.

although looking at the tools it went in 7.6

Dave

> Dave
> 
> > 
> > -- 
> > Eric Blake, Principal Software Engineer
> > Red Hat, Inc.           +1-919-301-3226
> > Virtualization:  qemu.org | libvirt.org
> > 
> 
> 
> 
> 
> > _______________________________________________
> > Virtio-fs mailing list
> > Virtio-fs@redhat.com
> > https://www.redhat.com/mailman/listinfo/virtio-fs
> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [Virtio-fs] [PATCH] virtiofsd: fix compile error if 'F_OFD_GETLK' not defined
Posted by piaojun 4 years, 7 months ago
Hi Dave and Eric,

On 2019/8/1 22:26, Dr. David Alan Gilbert wrote:
> * Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
>> * Eric Blake (eblake@redhat.com) wrote:
>>> 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 <piaojun@huawei.com>
>>>> ---
>>>>  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.
>>
>> It is, but I guess the answer here is someone wanted to build on RHEL7.
> 
> although looking at the tools it went in 7.6
> 
> Dave
> 

Yes, the compile error comes from kernel 3.10, and it seems necessary
to solve this. I try to reuse qemu_lock_fd() to compat F_GETLK/F_SETLK,
but its semantics differs from fcntl, so I think using #ifdef will be
easier.

We could delete F_GETLK/F_SETLK compat when virtiofsd is limited to be
built in newer kernel. And I'm glad to hear from other developers.

Thanks,
Jun

>> Dave
>>
>>>
>>> -- 
>>> Eric Blake, Principal Software Engineer
>>> Red Hat, Inc.           +1-919-301-3226
>>> Virtualization:  qemu.org | libvirt.org
>>>
>>
>>
>>
>>
>>> _______________________________________________
>>> Virtio-fs mailing list
>>> Virtio-fs@redhat.com
>>> https://www.redhat.com/mailman/listinfo/virtio-fs
>>
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>
>> _______________________________________________
>> Virtio-fs mailing list
>> Virtio-fs@redhat.com
>> https://www.redhat.com/mailman/listinfo/virtio-fs
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://www.redhat.com/mailman/listinfo/virtio-fs
> .
>