Re: [patch] lockf() fix

[Ben Collins <bcollins at debian dot org>]

On Sun, Oct 29, 2000 at 09:24:14AM +0100, Andreas Jaeger wrote:
> >>>>> Ben Collins writes:
> 
>  > This seems to only affect sparc, sparc64 and alpha, since they are the
>  > only ones that define F_RDLCK to something other than 0 (1, in each case).
> 
>  > Thanks to Anton Blanchard for helping me track this down.
> 
>  > --- ./sysdeps/generic/lockf.c~	Fri Oct 27 18:43:40 2000
>  > +++ ./sysdeps/generic/lockf.c	Fri Oct 27 18:34:25 2000
>  > @@ -32,6 +32,7 @@
>  >    memset ((char *) &fl, '\0', sizeof (fl));
>  >  
>  >    /* lockf is always relative to the current file position.  */
>  > +  fl.l_type = F_RDLCK;
>  >    fl.l_whence = SEEK_CUR;
>  >    fl.l_start = 0;
>  >    fl.l_len = len;
> 
> I don't understand why this is needed at all.  Looking at the code
> each of the switch cases sets l_type explicitly.  Is l_type needed for
> the fcntl (fd, F_GETLK) ?

F_TEST does not set the l_type explicitly. Perhaps, this needs to be
inside the F_TEST case block.

> If your patch would be right, the same patch would be needed for
> lockf64.

Sorry, didn't look, but it does.

> Please explain a bit more why this is needed - or send a simple test
> program that fails on those architectures.

In the case of F_TEST, lockf() wants to send an l_type of F_RDLCK.
Normally this is ok not to set explicitly, since on most architectures,
F_RDLCK == 0 (and the memset above sets this implicitly). However, on
alpha, sparc and sparc64, these types are defined as:

/* for posix fcntl() and lockf() */
#define F_RDLCK         1
#define F_WRLCK         2
#define F_UNLCK         3

So you can see, that sending 0 as l_type is just undefined, not to
mention, plain wrong and thus the kernel returns -1 and sets EINVAL.

Here's a new patch that puts this in the right place, and includes
lockf64.c.

Thanks for looking at the patch,
  Ben

-- 
 -----------=======-=-======-=========-----------=====------------=-=------
/  Ben Collins  --  ...on that fantastic voyage...  --  Debian GNU/Linux   \
`  bcollins@debian.org  --  bcollins@openldap.org  --  bcollins@linux.com  '
 `---=========------=======-------------=-=-----=-===-======-------=--=---'
--- ChangeLog~	Sun Oct 29 07:38:00 2000
+++ ChangeLog	Sun Oct 29 08:07:30 2000
@@ -1,0 +1,6 @@
+2000-10-29  Ben Collins  <bcollins@debian.org>
+
+	* sysdeps/generic/lockf.c: In the case of F_TEST, set l_type to
+	  F_RDLCK explicitly.
+	* sysdeps/unix/sysv/linux/i386/lockf64.c: Likewise.
+
--- sysdeps/generic/lockf.c~	Sun Oct 29 07:28:58 2000
+++ sysdeps/generic/lockf.c	Sun Oct 29 07:37:53 2000
@@ -41,6 +41,7 @@
     case F_TEST:
       /* Test the lock: return 0 if FD is unlocked or locked by this process;
 	 return -1, set errno to EACCES, if another process holds the lock.  */
+      fl.l_type = F_RDLCK;
       if (__fcntl (fd, F_GETLK, &fl) < 0)
 	return -1;
       if (fl.l_type == F_UNLCK || fl.l_pid == __getpid ())
--- sysdeps/unix/sysv/linux/i386/lockf64.c~	Sun Oct 29 07:34:47 2000
+++ sysdeps/unix/sysv/linux/i386/lockf64.c	Sun Oct 29 07:37:28 2000
@@ -85,6 +85,7 @@
       /* Test the lock: return 0 if FD is unlocked or locked by this process;
 	 return -1, set errno to EACCES, if another process holds the lock.  */
 #if __ASSUME_FCNTL64 > 0
+      fl64.l_type = F_RDLCK;
       if (INLINE_SYSCALL (fcntl64, 3, fd, F_GETLK64, &fl64) < 0)
         return -1;
       if (fl64.l_type == F_UNLCK || fl64.l_pid == __getpid ())
@@ -93,6 +94,7 @@
       return -1;
 #else
 # ifdef __NR_fcntl64
+      fl64.l_type = F_RDLCK;
       if (!__have_no_fcntl64)
 	{
 	  int res = INLINE_SYSCALL (fcntl64, 3, fd, F_GETLK64, &fl64);
@@ -120,6 +122,7 @@
 	    }
 	}
 # endif
+      fl.l_type = F_RDLCK;
       if (__fcntl (fd, F_GETLK, &fl) < 0)
 	return -1;
       if (fl.l_type == F_UNLCK || fl.l_pid == __getpid ())