This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

> 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) ?

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

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

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

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.

> 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


> > +#  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!
-- 
Jeff Layton <jlayton@redhat.com>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]