[PATCH] Convert Samba to OFD locking - version 2

[Richard Sharpe]

On Thu, May 19, 2016 at 2:12 PM, Jeremy Allison <jra at samba.org> wrote:
> OK, here is version 2 with Uri and Simo's changes
> incorporated. Compile-time checks now run the test
> code rather than just compiling, and there's a
> tuneable "smbd:force process locks" that will force
> the old-style process specific locks. Advice to
> use it gets printed in the log if fcntl returns
> EINVAL.
>
> Please review and push if happy !

First few comment, perhaps a bit picky:

Since we always expose OFD locks to clients, if available, why do we
need to still reference count them?

>diff --git a/source3/lib/util.c b/source3/lib/util.c
>index f64f6b5..36a2750 100644
>--- a/source3/lib/util.c
>+++ b/source3/lib/util.c
...
>@@ -1164,7 +1164,7 @@ bool fcntl_getlock(int fd, off_t *poffset, off_t *pcount, int *ptype, pid_t *ppi
>        lock.l_len = *pcount;
>        lock.l_pid = 0;
>
>-       ret = sys_fcntl_ptr(fd,F_GETLK,&lock);
>+       ret = sys_fcntl_ptr(fd,op,&lock);
>
>        if (ret == -1) {
>                int sav = errno;

Since you are touching the line, can we get spaces after the commas?
There is another example in that same file change.

>diff --git a/source3/locking/posix.c b/source3/locking/posix.c
>index 0c729ad..f3a89fd 100644
>--- a/source3/locking/posix.c
>+++ b/source3/locking/posix.c
>@@ -591,12 +591,13 @@ int fd_close_posix(const struct files_struct *fsp)
>        size_t count, i;
>
>        if (!lp_locking(fsp->conn->params) ||
>-           !lp_posix_locking(fsp->conn->params))
>+           !lp_posix_locking(fsp->conn->params) ||
>+           fsp->use_ofd_locks)
>        {
>                /*
>-                * No locking or POSIX to worry about or we want POSIX semantics
>-                * which will lose all locks on all fd's open on this dev/inode,
>-                * just close.
>+                * No locking or POSIX to worry about or we are using POSIX
>+                * open file description lock semantics which only removes
>+                * locks on the file descriptor we're closing. Just close.
>                 */
>                return close(fsp->fh->fd);
>        }
>--
>2.8.0.rc3.226.g39d4020

According to https://www.gnu.org/software/libc/manual/html_node/Open-File-Description-Locks.html
OFD locks on an file description (pointer?) are only freed when the
last file descriptor that refers to the file description (pointer?) is
closed.

If that is true, the comment seems a little misleading. At that point
do we know we only have one FD for that file? If we never dup the the
FD, I guess we do but it would be nice to see that mentioned for the
newbies looking at that code.


-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)