Re: [PATCH] 1(3) hurd+glibc: Support for file record locking

[Samuel Thibault]

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