[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] 1(3) hurd+glibc: Support for file record locking
From: |
Samuel Thibault |
Subject: |
Re: [PATCH] 1(3) hurd+glibc: Support for file record locking |
Date: |
Sun, 30 Dec 2018 21:21:18 +0100 |
User-agent: |
NeoMutt/20170113 (1.7.2) |
Hello,
Svante Signell, le sam. 22 déc. 2018 13:07:37 +0100, a ecrit:
> diskfs_S_file_lock (struct protid *cred, int flags)
> {
...
> + /* XXX: Fix for flock(2) calling fcntl(2) */
> + if ((openstat & O_RDONLY) && !(openstat & O_WRONLY)) openstat |= O_WRONLY;
> + if (!(openstat & O_RDONLY) && (openstat & O_WRONLY)) openstat |= O_RDONLY;
I don't understand this: in which application case is this needed?
> +error_t
> +diskfs_S_file_record_lock (struct protid *cred,
> + int cmd,
> + struct flock64 *lock,
> + mach_port_t rendezvous)
> +{
> + struct node *node;
> + error_t err;
> +
> + if (! cred)
> + return EOPNOTSUPP;
> +
> + /* XXX: Call with non-null for per process locking*/
> + rendezvous = MACH_PORT_NULL;
Why setting it to MACH_PORT_NULL? AIUI, we can just pass the rendezvous
parameter as it is to fshelp_rlock_tweak (and currently it'll already be
MACH_PORT_NULL), and when we'll implement proper support it'll be in
fshelp_rlock_tweak, we won't have to do anything here.
And similarly in other translator libs.
> + node = cred->po->np;
> + pthread_mutex_lock (&node->lock);
> + err = fshelp_rlock_tweak (&node->userlock, &node->lock,
> + &cred->po->lock_status, cred->po->openstat,
> + node->dn_stat.st_size, cred->po->filepointer,
> + cmd, lock, rendezvous);
> + pthread_mutex_unlock (&node->lock);
> + return err;
> +}
> libfshelp/ChangeLog
> 2018-12-07 Svante Signell <svante.signell@gmail.com>
>
> * Update copyright years.
>
> * rlock-tweak.c(fshelp-rlock_tweak):
Please take care of typos, it's important for being able to grep etc.
> Add new argument:
> mach_port_t rendezvous. Calls using MACH_PORT_NULL indicates
> per opened file locking. Calls with !MACH_PORT_NULL indicates
> per process locking. l_pid is set to -1 when a conflicting
> lock is taken by another process, like BSD does. This and per
> process locking will be fixed by new proc RPCs implementing
> proc_{server,user}_identify.
Such information does not belong to the ChangeLog, but to the .h file.
> +# Define the 64 bit versions of the second argument to fcntl()
> +# Can safely be removed when glibc is updated
> +EXTRA_CPP_FLAGS= -DF_GETLK64=10 -DF_SETLK64=11 -DF_SETLKW64=12
> +rlock-tweak-CPPFLAGS += $(EXTRA_CPP_FLAGS)
Avoid such hooks in your patches. Yes, when adding an RPC the bootstrap
process needs some tweaks, but that's not what we will commit in the
end.
> Index: hurd-0.9.git20181030-3.3/libfshelp/rlock.h
> ===================================================================
> --- /dev/null
> +++ hurd-0.9.git20181030-3.3/libfshelp/rlock.h
> @@ -0,0 +1,111 @@
...
> +
> +/*
> +libthreads/cthreads.h:
> +typedef struct condition {
> + spin_lock_t lock;
> + struct cthread_queue queue;
> + const char *name; <-- differs
> + struct cond_imp *implications;
> +} *condition_t;
> +rlock.h:
> + struct condition wait;
> +here replaced by
> + struct pthread_cond_t wait;
> +/usr/include/pthread/pthreadtypes.h:typedef struct __pthread_cond
> pthread_cond_t;
> +/usr/include/i386-gnu/bits/condition.h:struct __pthread_cond
> +struct __pthread_cond
> +{
> + __pthread_spinlock_t __lock;
> + struct __pthread *__queue;
> + struct __pthread_condattr *__attr; <-- differs
> + struct __pthread_condimpl *__impl;
> + void *__data; <-- differs
> +};
> + */
Drop this as well, since we won't commit it.
> Index: hurd-0.9.git20181030-3.3/pflocal/fs.c
> ===================================================================
> --- hurd-0.9.git20181030-3.3.orig/pflocal/fs.c
> +++ hurd-0.9.git20181030-3.3/pflocal/fs.c
> @@ -39,3 +37,11 @@ S_file_check_access (struct sock_user *c
>
> return 0;
> }
> +
> +error_t
> +S_file_record_lock (struct sock_user *cred, int cmd,
> +//S_file_record_lock (file_t file, int cmd,
> + flock_t *flock64, mach_port_t rendezvous)
Again, do not keep such commented code which we won't commit.
> Index: hurd-0.9.git20181030-3.3/libnetfs/make-peropen.c
> ===================================================================
> --- hurd-0.9.git20181030-3.3.orig/libnetfs/make-peropen.c
> +++ hurd-0.9.git20181030-3.3/libnetfs/make-peropen.c
> struct peropen *
> netfs_make_peropen (struct node *np, int flags, struct peropen *context)
> {
> + error_t err;
> struct peropen *po = malloc (sizeof (struct peropen));
>
> if (!po)
> return NULL;
>
> po->filepointer = 0;
> - po->lock_status = LOCK_UN;
> + err = fshelp_rlock_po_init (&po->lock_status);
> + if (err)
> + return NULL;
> + /* XXX: refcount_init() cannot be used since reference counting
> + starts at 0 not 1 as for libdiskfs */
> refcount_init (&po->refcnt, 1);
I don't understand the XXX: how is refcount related to rlocks?
Svante Signell, le sam. 22 déc. 2018 13:09:26 +0100, a ecrit:
> Note that the following glibc patches have been commented out from the
> series file:
> #hurd-i386/git-fcntl64.diff
> #hurd-i386/git-lockf-0.diff
> #hurd-i386/tg-WRLCK-upgrade.diff
And thus I'll have to do the rebase myself? That'll mean I'll have to
defer applying the patches to when I get the time to do such rebase...
Unless you do the rebase so I have patches that I can just apply.
Really, remember that the more you prepare the patches, the earlier I
will find the time to apply them. If they can't be applied as such and
I have to do some work on them, it means I'll have to defer to when I
get the time to do it.
> Index: glibc-2.28-3.1/sysdeps/mach/hurd/fcntl.c
> ===================================================================
> --- glibc-2.28-3.1.orig/sysdeps/mach/hurd/fcntl.c
> +++ glibc-2.28-3.1/sysdeps/mach/hurd/fcntl.c
> @@ -28,6 +27,7 @@ __libc_fcntl (int fd, int cmd, ...)
> {
> va_list ap;
> struct hurd_fd *d;
> + mach_port_t rendezvous = MACH_PORT_NULL;
> int result;
>
> d = _hurd_fd_get (fd);
Avoid introducing the variable for the whole function when you actually
need it only for a couple of cases.
Thanks
Samuel