| Message ID | 20180713130916.4153-22-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 13, 2018 at 03:09:08PM +0200, Marc-André Lureau wrote: > According to Daniel Berrange, fcntl() locks have better portable > semantics than lockf(). Specifically I was referring to this from 'man lockf': On Linux, lockf() is just an interface on top of fcntl(2) locking. Many other systems implement lockf() in this way, but note that POSIX.1 leaves the relationship between lockf() and fcntl(2) locks unspecified. A portable application should probably avoid mixing calls to these interfaces. IOW, if its just a shim around fcntl() on many systems, it is clearer if we just use fcntl() directly, as we then know how fcntl() locks will behave if they're on a network filesystem like NFS. > Use an exclusive lock on the first byte with fcntl(). > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > util/oslib-posix.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index da1d4a3201..26b11490b9 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -92,6 +92,11 @@ bool qemu_write_pidfile(const char *pidfile, Error **errp) > { > int pidfd; > char pidstr[32]; > + struct flock lock = { > + .l_type = F_WRLCK, > + .l_whence = SEEK_SET, > + .l_len = 1, > + }; For the same semantics as lockf we should use len == 0 (ie infinity) Regards, Daniel
Hi On Tue, Aug 28, 2018 at 6:01 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Fri, Jul 13, 2018 at 03:09:08PM +0200, Marc-André Lureau wrote: > > According to Daniel Berrange, fcntl() locks have better portable > > semantics than lockf(). > > Specifically I was referring to this from 'man lockf': > > On Linux, lockf() is just an interface on top of fcntl(2) locking. > Many other systems implement lockf() in this way, but note that POSIX.1 > leaves the relationship between lockf() and fcntl(2) locks unspecified. > A portable application should probably avoid mixing calls to these > interfaces. > > IOW, if its just a shim around fcntl() on many systems, it is clearer > if we just use fcntl() directly, as we then know how fcntl() locks will > behave if they're on a network filesystem like NFS. > > > Use an exclusive lock on the first byte with fcntl(). > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > util/oslib-posix.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > > index da1d4a3201..26b11490b9 100644 > > --- a/util/oslib-posix.c > > +++ b/util/oslib-posix.c > > @@ -92,6 +92,11 @@ bool qemu_write_pidfile(const char *pidfile, Error **errp) > > { > > int pidfd; > > char pidstr[32]; > > + struct flock lock = { > > + .l_type = F_WRLCK, > > + .l_whence = SEEK_SET, > > + .l_len = 1, > > + }; > > For the same semantics as lockf we should use len == 0 (ie infinity) > ok > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
Hi On Tue, Aug 28, 2018 at 6:01 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Fri, Jul 13, 2018 at 03:09:08PM +0200, Marc-André Lureau wrote: > > According to Daniel Berrange, fcntl() locks have better portable > > semantics than lockf(). > > Specifically I was referring to this from 'man lockf': > > On Linux, lockf() is just an interface on top of fcntl(2) locking. > Many other systems implement lockf() in this way, but note that POSIX.1 > leaves the relationship between lockf() and fcntl(2) locks unspecified. > A portable application should probably avoid mixing calls to these > interfaces. > > IOW, if its just a shim around fcntl() on many systems, it is clearer > if we just use fcntl() directly, as we then know how fcntl() locks will > behave if they're on a network filesystem like NFS. > > > Use an exclusive lock on the first byte with fcntl(). > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > util/oslib-posix.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > > index da1d4a3201..26b11490b9 100644 > > --- a/util/oslib-posix.c > > +++ b/util/oslib-posix.c > > @@ -92,6 +92,11 @@ bool qemu_write_pidfile(const char *pidfile, Error **errp) > > { > > int pidfd; > > char pidstr[32]; > > + struct flock lock = { > > + .l_type = F_WRLCK, > > + .l_whence = SEEK_SET, > > + .l_len = 1, > > + }; > > For the same semantics as lockf we should use len == 0 (ie infinity) I went to look at how libvirt implements it. Interestingly, it uses start=0, len=1, and you added it in: https://libvirt.org/git/?p=libvirt.git;a=blobdiff;f=src/util/virpidfile.c;h=38cc7e28c619defa81d55a47f596eeb6f9832cfd;hp=c0b12867db09d138f0382d76db098c78722be7c4;hb=e1da464d88cd39794809ec0d82dcdfbede744ad6;hpb=b7e5ca48f8c753a85ae77be3430fef83629d2178
On Wed, Aug 29, 2018 at 01:41:14AM +0200, Marc-André Lureau wrote: > Hi > > On Tue, Aug 28, 2018 at 6:01 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Fri, Jul 13, 2018 at 03:09:08PM +0200, Marc-André Lureau wrote: > > > According to Daniel Berrange, fcntl() locks have better portable > > > semantics than lockf(). > > > > Specifically I was referring to this from 'man lockf': > > > > On Linux, lockf() is just an interface on top of fcntl(2) locking. > > Many other systems implement lockf() in this way, but note that POSIX.1 > > leaves the relationship between lockf() and fcntl(2) locks unspecified. > > A portable application should probably avoid mixing calls to these > > interfaces. > > > > IOW, if its just a shim around fcntl() on many systems, it is clearer > > if we just use fcntl() directly, as we then know how fcntl() locks will > > behave if they're on a network filesystem like NFS. > > > > > Use an exclusive lock on the first byte with fcntl(). > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > --- > > > util/oslib-posix.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > > > index da1d4a3201..26b11490b9 100644 > > > --- a/util/oslib-posix.c > > > +++ b/util/oslib-posix.c > > > @@ -92,6 +92,11 @@ bool qemu_write_pidfile(const char *pidfile, Error **errp) > > > { > > > int pidfd; > > > char pidstr[32]; > > > + struct flock lock = { > > > + .l_type = F_WRLCK, > > > + .l_whence = SEEK_SET, > > > + .l_len = 1, > > > + }; > > > > For the same semantics as lockf we should use len == 0 (ie infinity) > > I went to look at how libvirt implements it. Interestingly, it uses > start=0, len=1, and you added it in: libvirt never used lockf though. Both are valid, I was just saying that to be identical to what lockf did, it would use len == 0. Regards, Daniel
diff --git a/util/oslib-posix.c b/util/oslib-posix.c index da1d4a3201..26b11490b9 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -92,6 +92,11 @@ bool qemu_write_pidfile(const char *pidfile, Error **errp) { int pidfd; char pidstr[32]; + struct flock lock = { + .l_type = F_WRLCK, + .l_whence = SEEK_SET, + .l_len = 1, + }; pidfd = qemu_open(pidfile, O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR); if (pidfd == -1) { @@ -99,10 +104,11 @@ bool qemu_write_pidfile(const char *pidfile, Error **errp) return false; } - if (lockf(pidfd, F_TLOCK, 0)) { + if (fcntl(pidfd, F_SETLK, &lock)) { error_setg_errno(errp, errno, "Cannot lock pid file"); goto fail; } + if (ftruncate(pidfd, 0)) { error_setg_errno(errp, errno, "Failed to truncate pid file"); goto fail;
According to Daniel Berrange, fcntl() locks have better portable semantics than lockf(). Use an exclusive lock on the first byte with fcntl(). Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- util/oslib-posix.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)