contrib/virtiofsd/passthrough_ll.c | 9 +++++++++ 1 file changed, 9 insertions(+)
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;
}
--
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
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 >
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
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 >
* 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
* 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
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 > . >
© 2016 - 2024 Red Hat, Inc.