[PATCH glibc] Add file record locking support

[Svante Signell]

Hi,

Attached is the patch adding support for file record locking in glibc,
as implemented in Hurd by Neal Walfield and others. This patch should be
applied after the corresponding hurd patch series in case glibc and hurd
are not built simultaneously. (Maybe the conversion functions as written
by Samuel should be used to verify struct sizes:
http://lists.gnu.org/archive/html/bug-hurd/2014-08/msg00082.html)

Thanks!

[Guillem Jover]

Index: glibc-2.19/sysdeps/mach/hurd/fcntl.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

--- glibc-2.19.orig/sysdeps/mach/hurd/fcntl.c
+++ glibc-2.19/sysdeps/mach/hurd/fcntl.c
@@ -128,56 +127,87 @@ __libc_fcntl (int fd, int cmd, ...)
{

[=E2=80=A6]

struct flock *fl =3D va_arg (ap, struct flock *);

[=E2=80=A6]

+ struct flock64 *fl64 =3D malloc (sizeof (struct flock64));

You are not checking if malloc failed, but in any case there's no need
at all to malloc the struct, just use =C2=ABstruct flock64 fl64=C2=BB.

+ switch (fl->l_whence)
+ {
+ break;
+ errno =3D EINVAL;
+ return -1;
+ break;
+ }

The indentation here and in previous places seems messed up, check for
space vs tab and similar.

Thanks,
Guillem

[Svante Signell]

Post by Guillem Jover

Index: glibc-2.19/sysdeps/mach/hurd/fcntl.c
===================================================================
--- glibc-2.19.orig/sysdeps/mach/hurd/fcntl.c
+++ glibc-2.19/sysdeps/mach/hurd/fcntl.c
@@ -128,56 +127,87 @@ __libc_fcntl (int fd, int cmd, ...)
{

[
]

struct flock *fl = va_arg (ap, struct flock *);

[
]

+ struct flock64 *fl64 = malloc (sizeof (struct flock64));

You are not checking if malloc failed, but in any case there's no need
at all to malloc the struct, just use «struct flock64 fl64».

Yes you are right, no checks are made. I removed the malloc part. What
about freeing fl64 later on?

Post by Guillem Jover

+ switch (fl->l_whence)
+ {
+ break;
+ errno = EINVAL;
+ return -1;
+ break;
+ }

The indentation here and in previous places seems messed up, check for
space vs tab and similar.

Fixed! (hopefully)

Updated patch attached.

[Guillem Jover]

Post by Svante Signell

Post by Guillem Jover

Index: glibc-2.19/sysdeps/mach/hurd/fcntl.c
===================================================================
--- glibc-2.19.orig/sysdeps/mach/hurd/fcntl.c
+++ glibc-2.19/sysdeps/mach/hurd/fcntl.c
@@ -128,56 +127,87 @@ __libc_fcntl (int fd, int cmd, ...)
{

[…]

struct flock *fl = va_arg (ap, struct flock *);

[…]

+ struct flock64 *fl64 = malloc (sizeof (struct flock64));

You are not checking if malloc failed, but in any case there's no need
at all to malloc the struct, just use «struct flock64 fl64».

Yes you are right, no checks are made. I removed the malloc part. What
about freeing fl64 later on?

You cannot free() memory from the stack, no. It gets released
automatically when it gets out of scope (but this is basic C).

Post by Svante Signell
Index: glibc-2.19/sysdeps/mach/hurd/fcntl.c
===================================================================
--- glibc-2.19.orig/sysdeps/mach/hurd/fcntl.c
+++ glibc-2.19/sysdeps/mach/hurd/fcntl.c
@@ -128,56 +127,87 @@ __libc_fcntl (int fd, int cmd, ...)
+ struct flock64 fl64;
+ fl64->l_type = fl->l_type;
+ fl64->l_whence = fl->l_whence;
+ fl64->l_start = fl->l_start;
+ fl64->l_len = fl->l_len;
+ fl64->l_pid = fl->l_pid;
+ err = HURD_FD_PORT_USE (d, __file_record_lock (port, cmd, fl64));
+ fl->l_type = fl64->l_type;
+ fl->l_whence = fl64->l_whence;
+ fl->l_start = fl64->l_start;
+ fl->l_len = fl64->l_len;
+ fl->l_pid = fl64->l_pid;
+ free (fl64);
+ result = err ? __hurd_dfail (fd, err) : 0;
+ break;
+ }

I'm assuming you didn't build this. It should be fl64.<member>, and
__file_record_lock(..., &fl64), and the free() would also have given
you an error there, please build-test it.

Post by Svante Signell
+ {
+ struct flock64 *fl = va_arg (ap, struct flock64 *);
+
+ switch (fl->l_type)
+ {

Still space vs tab here.

Post by Svante Signell
+ break;
+ errno = EINVAL;
+ return -1;
+ break;
}

Thanks,
Guillem

[Svante Signell]

Post by Guillem Jover

Post by Svante Signell
Yes you are right, no checks are made. I removed the malloc part. What
about freeing fl64 later on?

You cannot free() memory from the stack, no. It gets released
automatically when it gets out of scope (but this is basic C).

Of course.

Post by Guillem Jover
I'm assuming you didn't build this. It should be fl64.<member>, and
__file_record_lock(..., &fl64), and the free() would also have given
you an error there, please build-test it.

You made me confused, so all changes were not made. I think the malloc
version was better for symmetry reasons. Changed anyway.

Post by Guillem Jover

Post by Svante Signell
+ {
+ struct flock64 *fl = va_arg (ap, struct flock64 *);
+
+ switch (fl->l_type)
+ {

Still space vs tab here.

Yes, the editor adds spaces up till 7 blanks, and eight spaces are
replaced by a tab, and the adds spces until two tabs, etc.

The other code does also contain these constructs. What to do, untabify
the whole file?

Updated patch, building in progress.

[Samuel Thibault]

Hello,

Post by Svante Signell
You made me confused, so all changes were not made. I think the malloc
version was better for symmetry reasons. Changed anyway.

Which symmetry? You mean fl->foo vs fl.foo? It is not worth spending a
malloc call for this.

Post by Svante Signell
+ if (cmd == F_GETLK)
+ cmd = F_GETLK64;
+ if (cmd == F_SETLK)
+ cmd = F_SETLK64;
+ if (cmd == F_SETLKW)
+ cmd = F_SETLKW64;

Better use a switch () statement for that.

Post by Svante Signell
+ errno = EINVAL;
return -1;
+ break;

Do not add a break after a return, it is completely useless. Same below.

Apart from that it seems good.

Samuel

[Svante Signell]

Post by Samuel Thibault
Hello,

Post by Svante Signell
You made me confused, so all changes were not made. I think the malloc
version was better for symmetry reasons. Changed anyway.

Which symmetry? You mean fl->foo vs fl.foo? It is not worth spending a
malloc call for this.

Ok!

Post by Samuel Thibault

Post by Svante Signell
+ if (cmd == F_GETLK)
+ cmd = F_GETLK64;
+ if (cmd == F_SETLK)
+ cmd = F_SETLK64;
+ if (cmd == F_SETLKW)
+ cmd = F_SETLKW64;

Better use a switch () statement for that.

Done.

Post by Samuel Thibault

Post by Svante Signell
+ errno = EINVAL;
return -1;
+ break;

Do not add a break after a return, it is completely useless. Same below.

Done.

Post by Samuel Thibault
Apart from that it seems good.

tks!

New patch attached (glibc is currently building)

[Svante Signell]

Post by Svante Signell
Hi,
Attached is the patch adding support for file record locking in glibc,
as implemented in Hurd by Neal Walfield and others. This patch should be
applied after the corresponding hurd patch series in case glibc and hurd
are not built simultaneously.

With the new version of glibc (2.28) the new file f-setlk.c (and f-setlk.h) is
introduced. This makes the file-record-locking patch for glibc very cumbersome.
Stuff already in fcntl.c now has to be redefined in that file, in order to add
the file record locking part.

Old patch:
fcntl.diff.old

New patch:
fcntl.diff.new

Please revert the patch introducing that file.

[Samuel Thibault]

Post by Svante Signell

Post by Svante Signell
Attached is the patch adding support for file record locking in glibc,
as implemented in Hurd by Neal Walfield and others. This patch should be
applied after the corresponding hurd patch series in case glibc and hurd
are not built simultaneously.

With the new version of glibc (2.28) the new file f-setlk.c (and f-setlk.h) is
introduced. This makes the file-record-locking patch for glibc very cumbersome.

[...]

Post by Svante Signell
+ err = HURD_FD_PORT_USE (d, __file_record_lock (port, cmd, fl64));

Ah, the introduced file_record_lock RPC takes the flock64, I thought it
was taking the parameters one by one, sorry about that.

Then I'd say that in your patch you can indeed just drop the f_setlk.c
file (which was just meant to be compatibility between fcntl locking and
flock locking), and in fcntl.c, in the 64bit case just call the RPC,
and in the non-64bit case, fill a struct flock64 with the fields of the
struct flock parameter content, and call the RPC, something like:

case F_GETLK:
case F_SETLK:
case F_SETLKW:
{
struct flock *fl = va_arg (ap, struct flock *);
struct flock64 fl64 = {
.l_type = fl->l_type,
.l_whence = fl->l_whence,
etc.
};

err = HURD_FD_PORT_USE (d, __file_record_lock (port, cmd, &fl64));
result = err ? __hurd_dfail(fd, err) : 0;

break;
}

case F_GETLK64:
case F_SETLK64:
case F_SETLKW64:
{
struct flock64 *fl64 = va_arg (ap, struct flock64 *);
err = HURD_FD_PORT_USE (d, __file_record_lock (port, cmd, fl64));
result = err ? __hurd_dfail(fd, err) : 0;
break;
}

Samuel

[Samuel Thibault]

{
struct flock *fl = va_arg (ap, struct flock *);
struct flock64 fl64 = {
.l_type = fl->l_type,
.l_whence = fl->l_whence,
etc.
};
err = HURD_FD_PORT_USE (d, __file_record_lock (port, cmd, &fl64));
result = err ? __hurd_dfail(fd, err) : 0;

When successful, the content of fl64.l_* should be copied into fl->l_*.
Additionally, for l_start and l_len, after copying, compare e.g.
fl64.l_start vs fl->l_start to check that it wasn't truncated. If that
fails, set the EOVERFLOW error. See sysdeps/mach/hurd/xstatconv.c for an
instance.

break;
}

Samuel

[Svante Signell]

Is this really needed? From git-fcntl64.diff:
Index: glibc-2.28/sysdeps/mach/hurd/fcntl.c
===================================================================
--- glibc-2.28.orig/sysdeps/mach/hurd/fcntl.c
+++ glibc-2.28/sysdeps/mach/hurd/fcntl.c
...
@@ -215,3 +210,4 @@ strong_alias (__libc_fcntl, __libc_fcntl
libc_hidden_def (__libc_fcntl64)
weak_alias (__libc_fcntl64, __fcntl64)
libc_hidden_weak (__fcntl64)
+weak_alias (__fcntl64, fcntl64)

Index: glibc-2.28/sysdeps/mach/hurd/fcntl64.c
===================================================================
--- /dev/null
+++ glibc-2.28/sysdeps/mach/hurd/fcntl64.c
@@ -0,0 +1 @@
+/* fcntl64 is defined in fcntl.c as an alias. */

Additionally, sysdeps/mach/hurd/flock.c calls __file_lock() and the file-record-
locking patches need to patch lib{diskfs,netfs,trivfs} file-lock.c files too.

It would be much simpler to use sysdeps/posix/flock.c that calls fcntl()
directly, and scratch sysdeps/mach/hurd/flock.c as well as
libdiskfs/file-lock.c
libnetfs/file-lock.c
libtrivfs/file-lock.c
in due time.

What about libtreefs/s-file.c:treefs_S_file_lock() and it's hooks?
What it the main usage application of libtreefs?

[Samuel Thibault]

Post by Svante Signell
Is this really needed?

Please quote a bit more than just that, otherwise we don't have any
actual context.

But actually, are you really talking about the mail you are quoting?
(which is about the GETLK part only, not 64bit vs 32bit questions.)

What is "this" in your sentence, exactly? The lines below? Why quoting
an unrelated sentence then?

Post by Svante Signell
Index: glibc-2.28/sysdeps/mach/hurd/fcntl.c
===================================================================
--- glibc-2.28.orig/sysdeps/mach/hurd/fcntl.c
+++ glibc-2.28/sysdeps/mach/hurd/fcntl.c
...
@@ -215,3 +210,4 @@ strong_alias (__libc_fcntl, __libc_fcntl
libc_hidden_def (__libc_fcntl64)
weak_alias (__libc_fcntl64, __fcntl64)
libc_hidden_weak (__fcntl64)
+weak_alias (__fcntl64, fcntl64)
Index: glibc-2.28/sysdeps/mach/hurd/fcntl64.c
===================================================================
--- /dev/null
+++ glibc-2.28/sysdeps/mach/hurd/fcntl64.c
@@ -0,0 +1 @@
+/* fcntl64 is defined in fcntl.c as an alias. */

Well, yes? Like on Linux, fcntl and fcntl64 are just made the same, and
it's the cmd parameter which decides between 32bit vs 64bit.

Or are you saying that we do not need the fcntl64 alias? Well, we do,
since it's declared and used by upstream commit
06ab719d30b01da401150068054d3b8ea93dd12f

Post by Svante Signell
Additionally, sysdeps/mach/hurd/flock.c calls __file_lock() and the file-record-
locking patches need to patch lib{diskfs,netfs,trivfs} file-lock.c files too.
It would be much simpler to use sysdeps/posix/flock.c that calls fcntl()
directly, and scratch sysdeps/mach/hurd/flock.c as well as
libdiskfs/file-lock.c
libnetfs/file-lock.c
libtrivfs/file-lock.c
in due time.

It's not so simple: see my mail from 5th march 2015

http://lists.gnu.org/archive/html/bug-hurd/2015-03/msg00032.html

flock is supposed to be per opened file, thus inherited through forks,
while fcntl/lockf to be per process, thus not inherited through forks.

In my mail I mentioned that we'd use the rendezvous port to distinguish
between both, but for now we'll defer that part and just pass NULL.

In other words, the steps would be:

- just keep flock as it is for now, make use fcntl use the RPC with
rendezvous==NULL, and still make it per-process. tdb will be happy.
- add rendezvous support in the RPC. Then we can make the
rendezvous!=NULL mean per-process, and make the rendezvous==NULL mean
per-open.
- Then we can make flock.c use it instead of file_lock. Notice however
that we can't just call fcntl like sysdeps/posix/flock.c does, since
flock wants per-open lock, not per-process.
- then we can scratch file-lock.c (after we are fairly convinced that
people have migrated to a libc that supports using the new locking
RPC).

Post by Svante Signell
What about libtreefs/s-file.c:treefs_S_file_lock() and it's hooks?

I don't see any users of this hook, and google doesn't either, so I'd
say we will be fine with scratching it at the same time as file-lock.C

Post by Svante Signell
What it the main usage application of libtreefs?

I don't know.

Samuel

[Svante Signell]

Post by Samuel Thibault

Post by Svante Signell
Is this really needed?

Please quote a bit more than just that, otherwise we don't have any
actual context.
But actually, are you really talking about the mail you are quoting?
(which is about the GETLK part only, not 64bit vs 32bit questions.)
What is "this" in your sentence, exactly? The lines below? Why quoting
an unrelated sentence then?

I commented out the following three debian patches and updated my patch to
fcntl.c.
#hurd-i386/git-fcntl64.diff
#hurd-i386/git-lockf-0.diff
#hurd-i386/tg-WRLCK-upgrade.diff
hurd-i386/fcntl.diff

And in the first patch we have the stuff below (that I did not have in the patch
for fcntl.c). Added now.

Post by Samuel Thibault

Post by Svante Signell
Index: glibc-2.28/sysdeps/mach/hurd/fcntl.c
===================================================================
--- glibc-2.28.orig/sysdeps/mach/hurd/fcntl.c
+++ glibc-2.28/sysdeps/mach/hurd/fcntl.c
...
@@ -215,3 +210,4 @@ strong_alias (__libc_fcntl, __libc_fcntl
libc_hidden_def (__libc_fcntl64)
weak_alias (__libc_fcntl64, __fcntl64)
libc_hidden_weak (__fcntl64)
+weak_alias (__fcntl64, fcntl64)
Index: glibc-2.28/sysdeps/mach/hurd/fcntl64.c
===================================================================
--- /dev/null
+++ glibc-2.28/sysdeps/mach/hurd/fcntl64.c
@@ -0,0 +1 @@
+/* fcntl64 is defined in fcntl.c as an alias. */

Well, yes? Like on Linux, fcntl and fcntl64 are just made the same, and
it's the cmd parameter which decides between 32bit vs 64bit.
Or are you saying that we do not need the fcntl64 alias? Well, we do,
since it's declared and used by upstream commit
06ab719d30b01da401150068054d3b8ea93dd12f

Post by Svante Signell
Additionally, sysdeps/mach/hurd/flock.c calls __file_lock() and the file-
record-locking patches need to patch lib{diskfs,netfs,trivfs} file-lock.c
files too.

...

Post by Samuel Thibault
It's not so simple: see my mail from 5th march 2015
http://lists.gnu.org/archive/html/bug-hurd/2015-03/msg00032.html
flock is supposed to be per opened file, thus inherited through forks,
while fcntl/lockf to be per process, thus not inherited through forks.
In my mail I mentioned that we'd use the rendezvous port to distinguish
between both, but for now we'll defer that part and just pass NULL.
- just keep flock as it is for now, make use fcntl use the RPC with
rendezvous==NULL, and still make it per-process. tdb will be happy.

Unfortunately tdb will not be happy, it does use fork a lot and fork should not
inherit locks, see
http://lists.gnu.org/archive/html/bug-hurd/2015-01/msg00092.html
http://lists.gnu.org/archive/html/bug-hurd/2015-01/msg00095.html

Never mind for now.

Post by Samuel Thibault
- add rendezvous support in the RPC. Then we can make the
rendezvous!=NULL mean per-process, and make the rendezvous==NULL mean
per-open.
- Then we can make flock.c use it instead of file_lock. Notice however
that we can't just call fcntl like sysdeps/posix/flock.c does, since
flock wants per-open lock, not per-process.
- then we can scratch file-lock.c (after we are fairly convinced that
people have migrated to a libc that supports using the new locking
RPC).

Post by Svante Signell
What about libtreefs/s-file.c:treefs_S_file_lock() and it's hooks?

I don't see any users of this hook, and google doesn't either, so I'd
say we will be fine with scratching it at the same time as file-lock.C

Post by Svante Signell
What it the main usage application of libtreefs?

I don't know.

OK, good roadmap idea!

Thanks!

[Samuel Thibault]

Post by Svante Signell
I commented out the following three debian patches and updated my patch to
fcntl.c.
#hurd-i386/git-fcntl64.diff
#hurd-i386/git-lockf-0.diff
#hurd-i386/tg-WRLCK-upgrade.diff
hurd-i386/fcntl.diff

Ok, then please say that, instead of quoting a mail talking about
another part of the changes, thus only bringing confusion.

Post by Svante Signell
And in the first patch we have the stuff below (that I did not have in the patch
for fcntl.c). Added now.

Ok.

Post by Svante Signell

Post by Samuel Thibault
- just keep flock as it is for now, make use fcntl use the RPC with
rendezvous==NULL, and still make it per-process. tdb will be happy.

Unfortunately tdb will not be happy, it does use fork a lot and fork should not
inherit locks, see
http://lists.gnu.org/archive/html/bug-hurd/2015-01/msg00092.html
http://lists.gnu.org/archive/html/bug-hurd/2015-01/msg00095.html
Never mind for now.

Ok, that will indeed do for a first commit, which does not actually
bring regression compared to what we currently have (only full-file
per-open locking).

Samuel