Hi,
Andrew Fresh wrote on Wed, Oct 30, 2013 at 01:50:56PM -0700:
> Post vBSDcon I have most existing patches working with 5.18.1, the only
> failing so far is that with threads enabled
If i understand correctly, we do not currently have threads enabled
in our -current base perl 5.16, so that is not a regression.
So, if that is all that is broken, you are basically claiming
that perl 5.18.1 is ready for commit? ;-)
> t/op/threads-dirh.t fails.
> (mv patches/{APPLIES,GOOD}/use_threads.patch)
> This test appears to point to a failure that needs fixing but I am not
> skilled enough to know how.
That problem is neither related to threads nor to 5.18 nor even to
Perl at all! The problem is that the seekdir(3) in our C library
is badly broken: The readdir(3) function saves dirents in a buffer,
so the lseek(2) done by seekdir(3) only takes effect after that
buffer becomes exhausted. Otto's nice regression test doesn't catch
that breakage because it only seeks far, far away, never in the
neighbourhood.
The following patch to the C library fixes the issue and lets
the perl 5.18.1 test succeed, even with threads enabled.
Some remarks to explain the patch:
* To minimally fix the bug, just adding
dirp->dd_loc = dirp->dd_size;
to __seekdir() would be sufficient.
But that would mean that each and every call to seekdir()
would invalidate the buffer, so some programs might trigger
getdents(2) after each seekdir(3) and re-read the buffer
from kernel space over and over again.
That's inefficient, we should really use the data which is
already available in user space.
* dd_loc = 0 is a bad choice to invalidate the buffer;
it means that one cannot search back to the first entry
in the buffer without invalidating the whole buffer.
* While here, i'm garbage collecting some unused headers
and even some unused functions (__seekdir, _telldir_unlocked),
and i'm moving back seekdir(3) into the file where it belongs.
The comment saying it must be in telldir.c to use some opaque
data structures appears to be a untrue.
OK?
Ingo
Index: opendir.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/opendir.c,v
retrieving revision 1.25
diff -u -p -r1.25 opendir.c
--- opendir.c 6 Oct 2013 17:57:11 -0000 1.25
+++ opendir.c 2 Nov 2013 03:51:44 -0000
@@ -111,7 +111,7 @@ __fdopendir(int fd)
return (NULL);
}
- dirp->dd_loc = 0;
+ dirp->dd_loc = -1;
dirp->dd_fd = fd;
dirp->dd_lock = NULL;
dirp->dd_curpos = 0;
Index: readdir.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/readdir.c,v
retrieving revision 1.19
diff -u -p -r1.19 readdir.c
--- readdir.c 6 Oct 2013 17:57:54 -0000 1.19
+++ readdir.c 2 Nov 2013 03:51:44 -0000
@@ -44,14 +44,15 @@ _readdir_unlocked(DIR *dirp, struct dire
*result = NULL;
for (;;) {
if (dirp->dd_loc >= dirp->dd_size)
- dirp->dd_loc = 0;
- if (dirp->dd_loc == 0) {
+ dirp->dd_loc = -1;
+ if (dirp->dd_loc == -1) {
dirp->dd_size = getdents(dirp->dd_fd, dirp->dd_buf,
dirp->dd_len);
if (dirp->dd_size == 0)
return (0);
if (dirp->dd_size < 0)
return (-1);
+ dirp->dd_loc = 0;
}
dp = (struct dirent *)(dirp->dd_buf + dirp->dd_loc);
if ((long)dp & 03 || /* bogus pointer check */
Index: rewinddir.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/rewinddir.c,v
retrieving revision 1.10
diff -u -p -r1.10 rewinddir.c
--- rewinddir.c 13 Aug 2013 05:52:12 -0000 1.10
+++ rewinddir.c 2 Nov 2013 03:51:44 -0000
@@ -1,5 +1,5 @@
/* $OpenBSD: rewinddir.c,v 1.10 2013/08/13 05:52:12 guenther Exp $ */
-/*-
+/*
* Copyright (c) 1990, 1993
* The Regents of the University of California. All rights reserved.
*
@@ -28,16 +28,12 @@
* SUCH DAMAGE.
*/
-#include <sys/types.h>
#include <dirent.h>
-
#include "thread_private.h"
#include "telldir.h"
void
rewinddir(DIR *dirp)
{
- _MUTEX_LOCK(&dirp->dd_lock);
- __seekdir(dirp, 0);
- _MUTEX_UNLOCK(&dirp->dd_lock);
+ seekdir(dirp, 0);
}
Index: seekdir.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/seekdir.c,v
retrieving revision 1.9
diff -u -p -r1.9 seekdir.c
--- seekdir.c 5 Jun 2007 18:11:48 -0000 1.9
+++ seekdir.c 2 Nov 2013 03:51:45 -0000
@@ -1,46 +1,69 @@
/* $OpenBSD: seekdir.c,v 1.9 2007/06/05 18:11:48 kurt Exp $ */
/*
- * Copyright (c) 1983, 1993
- * The Regents of the University of California. All rights reserved.
+ * Copyright (c) 2103 Ingo Schwarze <schwa...@openbsd.org>
*
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions
- * are met:
- * 1. Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- * 2. Redistributions in binary form must reproduce the above copyright
- * notice, this list of conditions and the following disclaimer in the
- * documentation and/or other materials provided with the distribution.
- * 3. Neither the name of the University nor the names of its contributors
- * may be used to endorse or promote products derived from this software
- * without specific prior written permission.
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
*
- * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND
- * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
- * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
- * ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
- * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
- * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
- * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
- * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
- * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
- * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
- * SUCH DAMAGE.
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/
-#include <sys/param.h>
#include <dirent.h>
+#include <unistd.h>
+
#include "thread_private.h"
#include "telldir.h"
/*
* Seek to an entry in a directory.
- * __seekdir is in telldir.c so that it can share opaque data structures.
+ * Only values returned by "telldir" should be passed to seekdir.
*/
void
seekdir(DIR *dirp, long loc)
{
+ struct dirent *dp;
+
+ /*
+ * First check whether the directory entry to seek for
+ * is still buffered in the directory structure in memory.
+ */
+
_MUTEX_LOCK(&dirp->dd_lock);
- __seekdir(dirp, loc);
+ dp = (struct dirent *)dirp->dd_buf;
+ if (dp->d_off <= loc) {
+ for (dirp->dd_loc = 0;
+ dirp->dd_loc < dirp->dd_size;
+ dirp->dd_loc += dp->d_reclen) {
+ dp = (struct dirent *)(dirp->dd_buf + dirp->dd_loc);
+ if (dp->d_off < loc)
+ continue;
+
+ /*
+ * Entry found in the buffer, use it.
+ * If readdir(3) follows, this will
+ * save us a getdents(2) syscall.
+ * Note that dd_loc is already set.
+ */
+
+ dirp->dd_curpos = loc;
+ _MUTEX_UNLOCK(&dirp->dd_lock);
+ return;
+ }
+ }
+
+ /*
+ * The entry is not in the buffer, prepare a call to getdents(2).
+ * In particular, invalidate dd_loc.
+ */
+
+ dirp->dd_loc = -1;
+ dirp->dd_curpos = lseek(dirp->dd_fd, loc, SEEK_SET);
_MUTEX_UNLOCK(&dirp->dd_lock);
}
Index: telldir.c
===================================================================
RCS file: /cvs/src/lib/libc/gen/telldir.c,v
retrieving revision 1.15
diff -u -p -r1.15 telldir.c
--- telldir.c 16 Aug 2013 05:27:39 -0000 1.15
+++ telldir.c 2 Nov 2013 03:51:45 -0000
@@ -28,44 +28,21 @@
* SUCH DAMAGE.
*/
-#include <sys/param.h>
-#include <sys/queue.h>
#include <dirent.h>
-#include <stdlib.h>
-#include <unistd.h>
-
#include "thread_private.h"
#include "telldir.h"
-int _readdir_unlocked(DIR *, struct dirent **, int);
-
/*
* return a pointer into a directory
*/
long
-_telldir_unlocked(DIR *dirp)
-{
- return (dirp->dd_curpos);
-}
-
-long
telldir(DIR *dirp)
{
long i;
_MUTEX_LOCK(&dirp->dd_lock);
- i = _telldir_unlocked(dirp);
+ i = dirp->dd_curpos;
_MUTEX_UNLOCK(&dirp->dd_lock);
return (i);
-}
-
-/*
- * seek to an entry in a directory.
- * Only values returned by "telldir" should be passed to seekdir.
- */
-void
-__seekdir(DIR *dirp, long loc)
-{
- dirp->dd_curpos = lseek(dirp->dd_fd, loc, SEEK_SET);
}
Index: telldir.h
===================================================================
RCS file: /cvs/src/lib/libc/gen/telldir.h,v
retrieving revision 1.6
diff -u -p -r1.6 telldir.h
--- telldir.h 13 Aug 2013 05:52:13 -0000 1.6
+++ telldir.h 2 Nov 2013 03:51:45 -0000
@@ -47,7 +47,4 @@ struct _dirdesc {
void *dd_lock; /* mutex to protect struct */
};
-long _telldir_unlocked(DIR *);
-void __seekdir(DIR *, long);
-
#endif
