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 2/2] manual/llio.texi: update manual to document file-private locks


On Fri, 11 Apr 2014 20:29:32 -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>
> > ---
> >  ChangeLog        |   1 +
> >  manual/llio.texi | 183 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 182 insertions(+), 2 deletions(-)
> 
> You are a great developer, submitting documentation along
> with new interfaces! You have all of my attention :-)
> 
> Same comments about ChangeLog and BZ apply.
> 
> The ChangeLog here should be:
> 
> 2014-04-11  Jeff Layton  <jlayton@redhat.com>
> 
> 	* manual/llio.texi: add section about file-private locks
> 
> > diff --git a/ChangeLog b/ChangeLog
> > index 55a84e598e46..622315629a44 100644
> > --- a/ChangeLog
> > +++ b/ChangeLog
> > @@ -2,6 +2,7 @@
> >  
> >  	* sysdeps/unix/sysv/linux/bits/fcntl-linux.h:
> >  	  (F_GETLKP, F_SETLKP, F_SETLKPW): New macros.
> > +	* manual/llio.texi: add section about file-private locks
> 
> Other way around.
> 
> If you are batching your ChangeLog's you'd write:
> 
> 2014-04-11  Jeff Layton  <jlayton@redhat.com>
> 
> 	* manual/llio.texi: add section about file-private locks
> 
> 	* sysdeps/unix/sysv/linux/bits/fcntl-linux.h:
> 	(F_GETLKP, F_SETLKP, F_SETLKPW): New macros.
> 
> With a space between them to indicate they were distinct
> commits but on the same date.
>   
> >  2014-04-11  Stefan Liebler  <stli@linux.vnet.ibm.com>
> >  
> > diff --git a/manual/llio.texi b/manual/llio.texi
> > index 6f8adfc607d7..bcaf0e419a90 100644
> > --- a/manual/llio.texi
> > +++ b/manual/llio.texi
> > @@ -57,6 +57,8 @@ directly.)
> >                                           flags associated with open files.
> >  * File Locks::                          Fcntl commands for implementing
> >                                           file locking.
> > +* File-private Locks::                  Fcntl commands for implementing
> > +                                         file-private locking.
> 
> OK.
> 
> >  * Interrupt Input::                     Getting an asynchronous signal when
> >                                           input arrives.
> >  * IOCTLs::                              Generic I/O Control operations.
> > @@ -2890,7 +2892,7 @@ Get flags associated with the open file.  @xref{File Status Flags}.
> >  Set flags associated with the open file.  @xref{File Status Flags}.
> >  
> >  @item F_GETLK
> > -Get a file lock.  @xref{File Locks}.
> > +Get (test) a file lock.  @xref{File Locks}.
> 
> Why the parenthetical comment? Please write out what you'd like to say.
>   

Ok.

> >  @item F_SETLK
> >  Set or clear a file lock.  @xref{File Locks}.
> > @@ -2898,6 +2900,15 @@ Set or clear a file lock.  @xref{File Locks}.
> >  @item F_SETLKW
> >  Like @code{F_SETLK}, but wait for completion.  @xref{File Locks}.
> >  
> > +@item F_GETLKP
> > +Get (test) a file-private lock.  @xref{File Locks}.
> > +
> > +@item F_SETLKP
> > +Set or clear a file lock.  @xref{File Locks}.
> > +
> > +@item F_SETLKPW
> > +Like @code{F_SETLKP}, but wait for completion.  @xref{File Locks}.
> 
> If these three are Linux specific please say so at the end of each
> e.g. `Specific to Linux.'
> 
> We have kFreeBSD and Hurd using glibc and I would like to know which
> constants apply to which OS. Although kFreeBSD is not upstream we
> still have Hurd.
> 

These are currently linux-specific, but eventually I'd like to see
other OS' adopt this as well. I'm attempting to get this incorporated
as part of the POSIX spec, but that will probably take a while.

> > +
> >  @item F_GETOWN
> >  Get process or process group ID to receive @code{SIGIO} signals.
> >  @xref{Interrupt Input}.
> > @@ -3576,6 +3587,10 @@ set_nonblock_flag (int desc, int value)
> >  
> >  @cindex file locks
> >  @cindex record locking
> > +This section describes "classic" record locks.  There is also a newer type
> > +of record lock that is associated with the open file table entry instead
> 
> You should make explicit mention of the distinction between file descriptor
> and file table entry here since it impacts the semantics of file-private
> locks on dup (as you note below).
> 

I'll disagree here. This section is about process-associated locks, so
I don't think it's really appropriate to make that distinction here.
The descriptor/file-table entry distinction doesn't have any bearing on
how process-associated locks work. I think we *do* need to make that
distinction in the file-private lock section.

> > +of the process.  @xref{File-private Locks}
> 
> The distinction has nothing to do with classical versus new, it has to do
> with the association of the lock and how it behaves. Please reword this
> to avoid quote "classic" distinction and instead talk about where and how
> the locks are applied.
> 
> Also note that documentation exists forever, and in 5 years the file table
> entry locks won't be "newer type", instead they will just be another type
> of lock. Thus avoid saying things like "newer type" and just say "open
> file table entry locks."
> 
> e.g.
> 
> This section describe record locks whose locks are associated with the
> process. There is also support for record locks associated with open file
> table entries instead of the process.  @xref{File-private Locks}.
> 
> Note the period after the @xref is expected style.
> 

Ok.

> > +
> >  The remaining @code{fcntl} commands are used to support @dfn{record
> >  locking}, which permits multiple cooperating programs to prevent each
> >  other from simultaneously accessing parts of a file in error-prone
> > @@ -3641,7 +3656,9 @@ the file.
> >  @item pid_t l_pid
> >  This field is the process ID (@pxref{Process Creation Concepts}) of the
> >  process holding the lock.  It is filled in by calling @code{fcntl} with
> > -the @code{F_GETLK} command, but is ignored when making a lock.
> > +the @code{F_GETLK} command, but is ignored when making a lock.  If the
> > +conflicting lock is a File-private lock (@pxref{File-private Locks}),
> > +then this field will be set to @math{-1}.
> >  @end table
> >  @end deftp
> >  
> > @@ -3817,6 +3834,168 @@ Remember that file locks are only a @emph{voluntary} protocol for
> >  controlling access to a file.  There is still potential for access to
> >  the file by programs that don't use the lock protocol.
> >  
> > +@node File-private Locks
> > +@section File-private Locks
> > +
> > +In contrast to classic record locks (@pxref{File Locks}), file-private
> 
> See comments above about "classic" or "new type."
> 
> Please reword like "In contrast to process-associated record locks..."
> 
> > +locks are associated with an open file table entry rather than a
> > +process.  File-private locks set on an open file descriptor will never
> > +conflict with existing file-private locks set on that file descriptor.
> > +
> > +File-private locks are also inherited by child processes across
> > +@code{fork} (@pxref{Creating a Process}), along with the file
> 
> What about clone with/without CLONE_FILES?
> 

Ok, I'll add that in as well.

> > +descriptor.  For this reason, the @code{l_pid} field in @code{struct
> > +flock} is meaningless for file private locks.  For the @code{F_GETLK} and
> 
> It can't be meaningless, it has to have some meaning. Setting it to -1 means
> "the locks is associated with an open file descriptor and not a process."
> Please reword e.g. For this reason, the @code{l_pid} field in @code {struct
> flock} is set to @math{-1} for file private locks to indicate that the lock
> is associated with an open file table entry instead of a process... 
> 

Hmm ok...

> > +@code{F_GETLKP} commands, the @code{l_pid} value is always set to
> > +@math{-1} if the blocking lock is a file-private one.
> 
> OK.
> 
> > +
> > +Note that using @code{dup} (@pxref{Duplicating Descriptors}) to clone a
> 
> s/Note that using/Using/g
> 
> s/to clone/to copy/g -- We are creating a "copy" and that's the language
> we use throughout for dup, not "clone".
> 
> > +file descriptor does not give you a new instance of the open file, but
> > +instead just clones a reference to an existing open file.  Thus,
> 
> Suggest:
> ... but instead copies the reference to the existing file table entry
> for the open file.
> 

Ok.

> > +file-private locks set on a file descriptor cloned by @code{dup} will
> > +never conflict with file-private locks set on the original descriptor.
> 
> OK. The dup'd fd acts as if it was the original fd since it never came
> from a distinct call to open.
> 

Right.

> > +
> > +File-private locks always conflict with classic record locks, even if
> > +acquired by the same process or on the same open file descriptor.
> 
> OK.
> 
> > +
> > +File-private locks use the same @code{struct flock} as classic POSIX
> > +locks as an argument (@pxref{File Locks}) and the macros for the
> > +cmd values are also declared in the header file @file{fcntl.h}. To
> > +use them, the macro @code{_GNU_SOURCE} must be defined prior to
> > +including @file{fcntl.h}.
> 
> OK.
> 
> > +
> > +In contrast to the @code{F_SETLK} or @code{F_SETLKW} commands, any
> > +@code{struct flock} used as an argument to the @code{F_SETLKP} or
> > +@code{F_SETLKPW} commands must have the @code{l_pid} value set to
> > +@math{0}.
> 
> Why not -1? This means that between F_GETLK and F_SETLK I need
> to explicitly zero l_pid otherwise the F_SETLK fails?
> 

This change was suggested by Neil Brown in the comments on the LWN
article. My intent is to leave as little undefined behavior as
possible. If we ensure that this is set to something distinct then we
have the possibility to extend this interface in the future by overloading
the l_pid field.

As to why zero and not -1...I don't think it really matters. You'd need
to reset that value anyway since there's no guarantee that you'd get -1
back in that field on a F_GETLK or F_GETLKP call. In the event that
you're not reusing a struct flock, using 0 here makes it easier to use
a C99 initializer to set it up.

> > +
> > +@pindex fcntl.h.
> > +
> > +@deftypevr Macro int F_GETLKP
> > +This macro is used as the @var{command} argument to @code{fcntl}, to
> > +specify that it should get information about a lock.  This command
> > +requires a third argument of type @w{@code{struct flock *}} to be passed
> > +to @code{fcntl}, so that the form of the call is:
> 
> Missing sentence?
> 

Oops, good catch.

> > +
> > +If there is a lock already in place that would block the lock described
> > +by the @var{lockp} argument, information about that lock overwrites
> > +@code{*@var{lockp}}.  Existing locks are not reported if they are
> > +compatible with making a new lock as specified.  Thus, you should
> > +specify a lock type of @code{F_WRLCK} if you want to find out about both
> > +read and write locks, or @code{F_RDLCK} if you want to find out about
> > +write locks only.
> 
> OK.
> 
> > +
> > +There might be more than one lock affecting the region specified by the
> > +@var{lockp} argument, but @code{fcntl} only returns information about
> > +one of them. 
> 
> Suggest adding more weasel words:
> 
> Which lock information is returned when multiple locks are present is
> unspecified.
> 
> Feel free to merge that with the sentence above into something more
> compact. I only want to say that you should be explicit that it is
> unspecified which lock information is returned if there are multiple
> locks overlapping the region of interest.
> 

Ok. Note that I copied a lot of this verbiage from the original File
Locking manual entry. It may be appropriate to update it with similar
weasel-wordage.

> > ... The @code{l_whence} member of the @var{lockp} structure is
> > +set to @code{SEEK_SET} and the @code{l_start} and @code{l_len} fields
> > +set to identify the locked region.
> > +
> > +If no lock applies, the only change to the @var{lockp} structure is to
> > +update the @code{l_type} to a value of @code{F_UNLCK}.
> > +
> > +The normal return value from @code{fcntl} with this command is an
> > +unspecified value other than @math{-1}, which is reserved to indicate an
> > +error.  The following @code{errno} error conditions are defined for
> > +this command:
> > +
> > +@table @code
> > +@item EBADF
> > +The @var{filedes} argument is invalid.
> > +
> > +@item EINVAL
> > +Either the @var{lockp} argument doesn't specify valid lock information,
> > +or the file associated with @var{filedes} doesn't support locks.
> > +@end table
> > +@end deftypevr
> 
> OK.
> 
> > +
> > +@comment fcntl.h
> > +@comment POSIX.1
> > +@deftypevr Macro int F_SETLKP
> > +This macro is used as the @var{command} argument to @code{fcntl}, to
> > +specify that it should set or clear a lock.  This command requires a
> > +third argument of type @w{@code{struct flock *}} to be passed to
> > +@code{fcntl}, so that the form of the call is:
> > +
> > +@smallexample
> > +fcntl (@var{filedes}, F_SETLKP, @var{lockp})
> > +@end smallexample
> > +
> > +If the opened file already has a lock on any part of the
> > +region, the old lock on that part is replaced with the new lock.  You
> > +can remove a lock by specifying a lock type of @code{F_UNLCK}.
> > +
> > +If the lock cannot be set, @code{fcntl} returns immediately with a value
> > +of @math{-1}.  This function does not block waiting for other tasks
> > +to release locks.  If @code{fcntl} succeeds, it returns a value other
> > +than @math{-1}.
> > +
> > +The following @code{errno} error conditions are defined for this
> > +function:
> > +
> > +@table @code
> > +@item EAGAIN
> > +@itemx EACCES
> > +The lock cannot be set because it is blocked by an existing lock on the
> > +file.  Some systems use @code{EAGAIN} in this case, and other systems
> > +use @code{EACCES}; your program should treat them alike, after
> > +@code{F_SETLKP}.  (@gnulinuxhurdsystems{} always use @code{EAGAIN}.)
> > +
> > +@item EBADF
> > +Either: the @var{filedes} argument is invalid; you requested a read lock
> > +but the @var{filedes} is not open for read access; or, you requested a
> > +write lock but the @var{filedes} is not open for write access.
> > +
> > +@item EINVAL
> > +Either the @var{lockp} argument doesn't specify valid lock information,
> > +or the file associated with @var{filedes} doesn't support locks.
> > +
> > +@item ENOLCK
> > +The system has run out of file lock resources; there are already too
> > +many file locks in place.
> > +
> > +Well-designed file systems never report this error, because they have no
> > +limitation on the number of locks.  However, you must still take account
> > +of the possibility of this error, as it could result from network access
> > +to a file system on another machine.
> > +@end table
> > +@end deftypevr
> 
> OK.
> 
> > +
> > +@comment fcntl.h
> > +@comment POSIX.1
> > +@deftypevr Macro int F_SETLKPW
> > +This macro is used as the @var{command} argument to @code{fcntl}, to
> > +specify that it should set or clear a lock.  It is just like the
> > +@code{F_SETLKP} command, but causes the process to block (or wait)
> 
> s/block (or wait)/wait/g
> 
> > +until the request can be specified.
> 
> s/specified/completed/g
> 

Ok.

> > +
> > +This command requires a third argument of type @code{struct flock *}, as
> > +for the @code{F_SETLKP} command.
> > +
> > +The @code{fcntl} return values and errors are the same as for the
> > +@code{F_SETLKP} command, but these additional @code{errno} error conditions
> > +are defined for this command:
> > +
> > +@table @code
> > +@item EINTR
> > +The function was interrupted by a signal while it was waiting.
> > +@xref{Interrupted Primitives}.
> > +
> > +@end table
> > +@end deftypevr
> 
> OK.
> 
> > +
> > +File-private locks are useful in the same sorts of situations as classic
> > +record locks.  They can also be used to synchronize file access between
> > +threads within the same process by giving each thread its own open file
> > +instance.
> > +
> > +Because they are only released automatically when the last reference to
> > +an open file is destroyed, file-private locks allow more assurance that
> > +the locks will not be released due to a library routine opening and
> > +closing a file without the application being aware.
> > +
> > +As with classic record locks, file-private locks are also voluntary.
> 
> Is it too much to ask for an example?
> 
> Like the threaded example in https://lwn.net/Articles/586904/ ? :-)
> 

Sure, sounds like a good idea.

> > +
> >  @node Interrupt Input
> >  @section Interrupt-Driven Input
>   
> Please repost a new v2 when you get a chance.
> 

Will do. Thanks for the review!

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