Re: [PATCH,Hurd] Fix F_*LK* fcntl with __USE_FILE_OFFSET64

[Roland McGrath]

> 2014-08-31  Samuel Thibault  <samuel.thibault@ens-lyon.org>
> 
>         struct flock64 uses 64bit values.  This introduces other values for
>         F_GETLK, F_SETLK, F_SETLKW to distinguish between both.
> 
>         * sysdeps/mach/hurd/bits/fcntl.h (F_GETLK64, F_SETLK64, F_SETLKW64): 
> New

No blank line between the (optional) overview description of the paragraph
and the first (required) file line.  Use tabs rather than spaces for the
indentation of the log entry.

>       * sysdeps/mach/hurd/flockconv.c: New file.
>         * sysdeps/mach/hurd/fcntl.c (__libc_fcntl): Include "flockconv.c",
>         handle F_GETLK64, F_SETLK64, F_SETLKW64 cases.

More bad indentation.

> +#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" when inside "#ifdef".

> +       case F_GETLK64:
> +         result = fcntl (fd, F_GETLK, &fl);

If you're actually going to have it call itself recursively, then call
__libc_fcntl, not the alias name.  But that's not the right thing to do
anyway.  Instead, break out the locking cases into a subroutine that you
can call for both versions.

> +  if (sizeof *buf == sizeof *buf64
> +      && sizeof buf->l_start == sizeof buf64->l_start
> +      && sizeof buf->l_len == sizeof buf64->l_len)
> +    {
> +      *buf = *(struct flock *) buf64;
> +      return 0;
> +    }

For paranoia's sake, test offsetof matches for each field too.

Why is this a separate file #include'd if it has only the one use?
Just put it directly into fcntl.c instead.

But given the actual implementation in fcntl, I don't think this generic
conversion layer is actually the useful thing to use.  You can just make
the new subroutine take the few individual fields that are used as
individual parameters instead of using any struct.  You can just do the
EOVERFLOW checks directly before calling the subroutine.  Since we don't
actually support F_GETLK, there is no conversion to do in the opposite
direction.


Thanks,
Roland