| Message ID | 1397220945-11926-2-git-send-email-jlayton@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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!
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.
On Sat, 12 Apr 2014, Carlos O'Donell wrote:
> So please use `#if __USE_GNU'.
No. Please keep what's done with any given macro consistent rather than
confusing things by mixing styles for a single macro. That means
__USE_GNU is tested with #ifdef / #ifndef unless and until all definitions
and users are changed to use 0/1 values instead of undefined/defined.
On Fri, 11 Apr 2014, Carlos O'Donell wrote: > This needs a bug filed since it's a user visible change in features > e.g. Bug XXXX "Add support for file-private locks." My comment in <https://sourceware.org/ml/libc-alpha/2014-03/msg00992.html> applies. I don't think we should be asking for bugs to be filed for new features, only if there was an actual bug that was user-visible in a past release. (It's not wrong to file such a bug, just unnecessary.)
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> + + * sysdeps/unix/sysv/linux/bits/fcntl-linux.h: + (F_GETLKP, F_SETLKP, F_SETLKPW): New macros. + 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. + */ +#ifdef __USE_GNU +# ifndef F_GETLKP +# define F_GETLKP 36 +# define F_SETLKP 37 +# define F_SETLKPW 38 +# endif +#endif + #ifdef __USE_LARGEFILE64 # define O_LARGEFILE __O_LARGEFILE #endif
Signed-off-by: Jeff Layton <jlayton@redhat.com> --- ChangeLog | 5 +++++ sysdeps/unix/sysv/linux/bits/fcntl-linux.h | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+)