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:37 PM, Jeff Layton wrote:
> On Fri, 11 Apr 2014 19:38:13 -0400
> "Carlos O'Donell" <carlos@redhat.com> wrote:
> 
>> 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.
>>
> 
> Not a problem, I appreciate the feedback.
> 
>> 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."
>>
> 
> Ok. I'll go over that doc and file a bug.

Thanks.

>> This also needs a NEWS entry to tell users it's new and they can use it.
>>
> 
> Ok, does that go in with the patch (in contrast to the ChangeLog entry) ?

As the contributor of the patch you just make a writeup for the NEWS
file and the committer adds it there along with the fixed BZ#.

So your contribution just looks like:

NEWS:

* Cool new feature! Blah blah blah!

See the existing NEWS file for examples.

>>> ---
>>>  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.
>>
> 
> Ok. I'll drop that hunk.

Thanks. See other posts for the right (tm) way of posting.

>>>  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.  */'.
>>
> 
> No problem. I think I got confused by another comment block above that
> had it on a newline.

The GNU Coding Style applies.

In general:

https://sourceware.org/glibc/wiki/Style_and_Conventions

>>> +#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.
>>
> 
> 
> I don't think there's any reason that we can't leave that off. They
> shouldn't be defined anywhere else (aside from the uapi headers), and I
> tried to pick values that are not used on any existing arches so that
> we can use the same ones everywhere. I'll respin with that removed.

Thanks for the respin.

> Now, that said...I don't really have a great feel for how to get the
> header file synchronization right so I'd appreciate any guidance here.
> 
> If you have time, could you also take a look at these definitions in
> the kernel tree as well? Is there some way we could wrap these there
> such that we wouldn't need to separately define them in glibc?
> 
> For now I'll assume that that's not feasible unless you tell me
> otherwise.

For now that is not feasible. That is to say that things are not
structured yet to use kernel constants directly without redefining them
in glibc, particularly for fcntl.h. It isn't impossible, but it is a
distinct project with unique requirements that far exceeds the scope of
the work you're trying to accomplish.

>> 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.
>>
> 
> Does that mean I should do this instead?
> 
> #if defined __USE_GNU

Same problem.

The typeo-safe alternative is:

#if __USE_GNU

Where __USE_GNU is defined as 0 or 1.

The goal is to move away from defined vs. undefined,
and towards always being defined with various values.

That avoids typos where you accidentally undefine a
constant or never define it in the first place.

So please use `#if __USE_GNU'.
 
>>> +#  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.
>>
> 
> No problem, will do. It turns out that I have a mistake in patch #2 as
> well, so I'll fix that too and repost both.
> 
> Thanks for the help so far!

You're welcome.

Cheers,
Carlos.