Re: [PATCH v2 2/2] manual/llio.texi: update manual to document file-private locks

[Jeff Layton <jlayton at redhat dot com>]

On Wed, 16 Apr 2014 15:13:45 +0200
"Michael Kerrisk (man-pages)" <mtk.lists@gmail.com> wrote:

> On 04/11/2014 02:55 PM, Jeff Layton wrote:
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> 
> [...]
> 

You may want to look at the v3 patch I sent yesterday. It has some
changes, but I think your comments below still apply to it.


> > @@ -3817,6 +3834,168 @@ Remember that file locks are only a @emph{voluntary} protocol for
> 
> s/voluntary/advisory/
> 

Ok...

> >  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
> > +locks are associated with an open file table entry rather than a
> > +process.  File-private locks set on an open file descriptor will never
> 
> s/File-private locks/New file-private locks/
> 

Carlos suggested avoiding adjectives like "new" or "classic" because in
a year or two, these will no longer be "new". I've moved to using
"process-associated" instead of "classic" in the latest rev and
dropping the "new" moniker.

> > +conflict with existing file-private locks set on that file descriptor.
> 
> Perhaps add a sentence here that a lock conversion may be involved?
> 
> [...]
> 

Sure, sounds reasonable.

> > +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.  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
> 
> s/If no lock applies/If no conflicting lock exists/
> 
> > +update the @code{l_type} to a value of @code{F_UNLCK}.
> 
> Slightly clumsy wording
> 
> s/update the @code{l_type} to a value of/
>   update the @code{l_type} field to the value/
> 
> s/update the @code{l_type} to a value of/
>   update @code{l_type} to the value/
> 
> 
> > +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.  
> 
> This doesn't sound correct. At the glibc level isn't the return value 
> either 0 for success or -1 on error?
> 
> [...]
> 

Good catch.  I copied that verbatim from the older File lock section.

That may be the case on non-linux kernels, but on linux, you'll get
back 0 on success. I think we'd want to mandate that for other OS' that
implement this as well. Will fix.

> > +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}.
> 
> See comment above.
> 

Thanks, will fix.

> [...]
> 
> > +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.
> 
> I think this could be explained more clearly. How about:
> 
>     They can also be used to synchronize file access between threads
>     within the same process by giving having each thread perform its
>     own @code{open} of the file, to obtain its own open file instance.
> 

Much better. I'll use that.

> > +Because they are only released automatically when the last reference to
> > +an open file is destroyed, file-private locks allow more assurance that
> 
> Suggest s/destroyed/closed/ (that gives the reader a clue what the 
> user-space operation is).
> 
> > +the locks will not be released due to a library routine opening and
> > +closing a file without the application being aware.
> 
> I suggest rewording as:
> 
>     ..., file-private locks avoid the possibility that locks are
>     released due to a library routine opening and closing a 
>     file without the application being aware.
> 

Much better. I'll use that too.

> > +
> > +As with classic record locks, file-private locks are also voluntary.
> 
> s/voluntary/advisory/
> 
> Cheers,
> 
> Michael

Thanks for the review. I'll plan to post v4 later today that
incorporates your suggestions once I give Carlos and others a chance to
comment. Once we get the manual sorted out here, I'll resubmit the fcntl
manpage patch.

-- 
Jeff Layton <jlayton@redhat.com>