Re: [PATCH glibc] Add file record locking support

[Guillem Jover]

On Thu, 2015-01-08 at 18:03:31 +0100, Svante Signell wrote:
> On Thu, 2015-01-08 at 16:56 +0100, Guillem Jover wrote:
> > On Thu, 2015-01-08 at 12:40:12 +0100, Svante Signell wrote:
> > > 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, ...)
> > >      case F_SETLK:
> > >      case F_SETLKW:
> > >        {
> > […]
> > >   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).

> 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.

> +    case F_GETLK64:
> +    case F_SETLK64:
> +    case F_SETLKW64:
> +      {
> +     struct flock64 *fl = va_arg (ap, struct flock64 *);
> +
> +        switch (fl->l_type)
> +          {

Still space vs tab here.

> +       case F_RDLCK:
> +       case F_WRLCK:
> +       case F_UNLCK:
> +         break;
> +       default:
> +         errno = EINVAL;
> +         return -1;
> +         break;
>         }

Thanks,
Guillem