Re: [PATCH hurd 0/6] Add file record locking support
On Tue, 2015-01-20 at 01:37 +0100, Samuel Thibault wrote: > Hello, > > Thanks for reviving this, it'll definitely be useful! > > Svante Signell, le Thu 08 Jan 2015 12:28:05 +0100, a écrit : > > 1) l_pid of struct flock is not set. According to the manpage (and > > posix) the PID of the process blocking the lock should be reported. Can > > maybe be solved by a shared memory solution in glibc or as indicated in > > task 1022. > > We can not trust a shared memory solution, we need an io_reidentify > round through the proc server. Can you give more information about how to implement this? > > 2) fork(2) does not create a process without the locks set for the > > child. > > > (This is probably the main cause tdbtorture in tdb does not work well > > with the current file record locking patches.) > > I don't think tdbtorture forks with locks held: AIUI it just starts > several processes and let them play at will with locks, without creating > or terminating them. Looking at the code of tdbtorture.c it forks a lot of processes (and assumes the locks are not kept) while the current implementation does not release them. With num_procs=1 tdbtorture works fine. Is the correct solution to add a hook in fork.c code to release these locks? -- To UNSUBSCRIBE, email to debian-hurd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/1421749161.10796.14.ca...@g3620.my.own.domain
Re: [PATCH hurd 0/6] Add file record locking support
Svante Signell, le Tue 20 Jan 2015 11:19:21 +0100, a écrit : > On Tue, 2015-01-20 at 01:37 +0100, Samuel Thibault wrote: > > Hello, > > > > Thanks for reviving this, it'll definitely be useful! > > > > Svante Signell, le Thu 08 Jan 2015 12:28:05 +0100, a écrit : > > > 1) l_pid of struct flock is not set. According to the manpage (and > > > posix) the PID of the process blocking the lock should be reported. Can > > > maybe be solved by a shared memory solution in glibc or as indicated in > > > task 1022. > > > > We can not trust a shared memory solution, we need an io_reidentify > > round through the proc server. > > Can you give more information about how to implement this? AIUI, it is the same principle as io_reauthenticate, see for instance in reauth_dtable for the d->port.port: In the application process glibc creates a rendez-vous port, sends it to the file server through io_reauthenticate, and to the auth server through auth_user_authenticate. The file server passes it to the auth server through auth_server_authenticate. The auth server notices the two auth_user_authenticate and auth_server_authenticate requests, notices that the rendez-vous port is the same, and then returns to the file server what it knows about the application process (uid, gid, etc.) Here it would be almost the same: In the application process glibc creates a rendez-vous port, sends it to the file server through io_reidentify, and to the proc server through proc_user_identify. The file server passes it to the proc server through proc_server_identify. The proc server notices the two proc_user_identify and proc_server_identify requests, notices that the rendez-vous port is the same, and then returns to the file server what it knows about the application process (pid). Samuel -- To UNSUBSCRIBE, email to debian-hurd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/20150120104018.gb18...@type.bordeaux.inria.fr
Re: [PATCH glibc] Add file record locking support
On Tue, 2015-01-20 at 01:06 +0100, Samuel Thibault wrote:
> Hello,
>
> Svante Signell, le Thu 08 Jan 2015 19:54:12 +0100, a écrit :
> > 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!
> > + 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.
> > + default:
> > + errno = EINVAL;
> > return -1;
> > + break;
>
> Do not add a break after a return, it is completely useless. Same below.
Done.
> Apart from that it seems good.
tks!
New patch attached (glibc is currently building)
sysdeps/mach/hurd/Changelog
2014-08-21 Svante Signell
* fcntl.c: Add support for file-record-lock RPC fixing posix
file locking using the flock64 version of struct flock.
* bits/fcntl.h: Since MIG cannot mix 32 bit and 64 bit
integers define unique numbers for F_GETLK64, F_SETLK64 and
F_SETLKW64 to prepare for a flock64 implementation of file
record locking in hurd.
Index: glibc-2.19/sysdeps/mach/hurd/bits/fcntl.h
===
--- glibc-2.19.orig/sysdeps/mach/hurd/bits/fcntl.h
+++ glibc-2.19/sysdeps/mach/hurd/bits/fcntl.h
@@ -163,9 +163,18 @@
# define F_GETOWN 5 /* Get owner (receiver of SIGIO). */
# define F_SETOWN 6 /* Set owner (receiver of SIGIO). */
#endif
+#ifdef __USE_FILE_OFFSET64
+# define F_GETLK F_GETLK64
+# define F_SETLK F_SETLK64
+# define F_SETLKW F_SETLKW64
+#else
#define F_GETLK 7 /* Get record locking info. */
#define F_SETLK 8 /* Set record locking info (non-blocking). */
#define F_SETLKW 9 /* Set record locking info (blocking). */
+#endif
+#define F_GETLK64 10 /* Get record locking info. */
+#define F_SETLK64 11 /* Set record locking info (non-blocking). */
+#define F_SETLKW64 12 /* Set record locking info (blocking). */
#ifdef __USE_XOPEN2K8
# define F_DUPFD_CLOEXEC 1030 /* Duplicate, set FD_CLOEXEC on new one. */
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
@@ -20,7 +20,6 @@
#include
#include
#include
-#include /* XXX for LOCK_* */
/* Perform file control operations on FD. */
int
@@ -128,31 +127,27 @@ __libc_fcntl (int fd, int cmd, ...)
case F_SETLK:
case F_SETLKW:
{
- /* XXX
- We need new RPCs to support POSIX.1 fcntl file locking!!
- For the time being we support the whole-file case only,
- with all kinds of WRONG WRONG WRONG semantics,
- by using flock. This is definitely the Wrong Thing,
- but it might be better than nothing (?). */
struct flock *fl = va_arg (ap, struct flock *);
- va_end (ap);
+ struct flock64 fl64;
+
switch (cmd)
{
case F_GETLK:
- errno = ENOSYS;
- return -1;
+ cmd = F_GETLK64;
+ break;
case F_SETLK:
- cmd = LOCK_NB;
+ cmd = F_SETLK64;
break;
- default:
- cmd = 0;
+ case F_SETLKW:
+ cmd = F_SETLKW64;
break;
}
switch (fl->l_type)
{
- case F_RDLCK: cmd |= LOCK_SH | __LOCK_ATOMIC; break;
- case F_WRLCK: cmd |= LOCK_EX | __LOCK_ATOMIC; break;
- case F_UNLCK: cmd |= LOCK_UN; break;
+ case F_RDLCK:
+ case F_WRLCK:
+ case F_UNLCK:
+ break;
default:
errno = EINVAL;
return -1;
@@ -160,24 +155,59 @@ __libc_fcntl (int fd, int cmd, ...)
switch (fl->l_whence)
{
case SEEK_SET:
- if (fl->l_start == 0 && fl->l_len == 0) /* Whole file request. */
- break;
- /* It seems to be common for applications to lock the first
- byte of the file when they are really doing whole-file locking.
- So, since it's so wrong already, might as well do that too. */
- if (fl->l_start == 0 && fl->l_len == 1)
- break;
- /* FALLTHROUGH */
case SEEK_CUR:
case SEEK_END:
- errno = ENOTSUP;
+ break;
+ default:
+ errno = EINVAL;
return -1;
+ }
+
+ 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;
+ result = err ? __hurd_dfail (fd, err) : 0;
+ break;
+ }
+
+case F_GETLK64:
+case F_SETLK64:
+case F_SETLKW64:
+ {
+ struct flock64 *fl = va_arg (ap, struct flock64 *);
+
+ switch (fl->l_type)
+ {
+ case F_RDLCK:
+ case F_WRLCK:
+ case F_UNLCK:
+ break;
+ default:
+ errno = EINVAL;
+ return -1;
+ }
+ switch (fl->l_whence)
+
Re: [PATCH hurd 0/6] Add file record locking support
Svante Signell, le Tue 20 Jan 2015 11:19:21 +0100, a écrit : > > I don't think tdbtorture forks with locks held: AIUI it just starts > > several processes and let them play at will with locks, without creating > > or terminating them. > > Looking at the code of tdbtorture.c it forks a lot of processes (and > assumes the locks are not kept) Ah, it seems the test tries to kill and restart children randomly indeed. Samuel -- To UNSUBSCRIBE, email to debian-hurd-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/20150120104440.ga21...@type.bordeaux.inria.fr
