Re: [PATCH v2 1/2] fcntl-linux.h: add new definitions for file-private lock cmd values

["Carlos O'Donell" <carlos at redhat dot com>]

On 04/11/2014 08:55 AM, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <jlayton@redhat.com>

Thank you very much for contributing this.

Don't read anything in the thoroughness or harshness of the
review, I think this patch is important, but needs some adjustment.
Some things are just nits, other are questions I have about the
actual changes.

Please see:
https://sourceware.org/glibc/wiki/Contribution%20checklist

This needs a bug filed since it's a user visible change in features
e.g. Bug XXXX "Add support for file-private locks."

This also needs a NEWS entry to tell users it's new and they can use it.

> ---
>  ChangeLog                                  |  5 +++++
>  sysdeps/unix/sysv/linux/bits/fcntl-linux.h | 19 +++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 5708d4eb64c2..55a84e598e46 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-04-11  Jeff Layton  <jlayton@redhat.com>
> +
	[BZ #XXXX]
> +	* sysdeps/unix/sysv/linux/bits/fcntl-linux.h:
> +	  (F_GETLKP, F_SETLKP, F_SETLKPW): New macros.
> +

ChangeLog is generally posted outside of the patch and not part of
the diff so we can just paste it into the top of the ChangeLog.

>  2014-04-11  Stefan Liebler  <stli@linux.vnet.ibm.com>
>  
>  	* sysdeps/s390/s390-32/configure.ac: Unify file with ...
> diff --git a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
> index 915eb3ede560..ae8ec1598a15 100644
> --- a/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
> +++ b/sysdeps/unix/sysv/linux/bits/fcntl-linux.h
> @@ -117,6 +117,25 @@
>  # define F_SETLKW64	14	/* Set record locking info (blocking).	*/
>  #endif
>  
> +/* fd "private" POSIX locks.
> +
> +   Usually POSIX locks held by a process are released on *any* close and are
> +   not inherited across a fork.
> +
> +   These cmd values will set locks that conflict with normal POSIX locks, but
> +   are "owned" by the opened file, not the process.  This means that they are
> +   inherited across fork like BSD (flock) locks, and they are only released
> +   automatically when the last reference to the the open file against which
> +   they were acquired is put.
> + */

Move '*/' up to the end of the line e.g. 'put.  */'.

> +#ifdef __USE_GNU
> +# ifndef F_GETLKP

Why `ifndef F_GETLKP'? Why not just define them?

If this is a header order inclusion conflict it needs to be solved like this:

https://sourceware.org/glibc/wiki/Synchronizing_Headers?highlight=%28header%29

If it's not a header order inclusion conflict, and you're using #ifndef to
allow machines a chance to define the constants themselves, don't, this is
a generic constant that is in upstream UAPI asm-generic and everyone should
be using the new values.

Note: Be aware we've started a typo-safe campaign using -Wundef and are looking
to avoid ifndef/ifdef in favour of just if. This way a typo doesn't lead to
unintended consequences.

> +#  define F_GETLKP	36
> +#  define F_SETLKP	37
> +#  define F_SETLKPW	38
> +# endif
> +#endif
> +
>  #ifdef __USE_LARGEFILE64
>  # define O_LARGEFILE __O_LARGEFILE
>  #endif
> 

Please post a new version with the changes.

Cheers,
Carlos.