Subject: Re: lib/24324: telldir issues
To: None <gnats-bugs@netbsd.org, lib-bug-people@netbsd.org,>
From: Christos Zoulas <christos@zoulas.com>
List: netbsd-bugs
Date: 05/17/2006 11:41:30
On May 13,  5:45pm, david@l8s.co.uk (David Laight) wrote:
-- Subject: Re: lib/24324: telldir issues

|  Thinking further, replacing the 'long dd_rewind' with a pointer to
|  library private data would be even better.  This would let the
|  library to hide the gory details from applications.
|  Since this woule be allocated by opendir(), telldir() could keep
|  it's 'const DIR *' prototype.
|  
|  For complete binary compatibility, the value in the (old) dd_rewind
|  field could be accepted as a token for 'start of directory'.

How about this?

christos

Index: include/dirent.h
===================================================================
RCS file: /cvsroot/src/include/dirent.h,v
retrieving revision 1.28
diff -u -r1.28 dirent.h
--- include/dirent.h	26 Mar 2006 18:22:40 -0000	1.28
+++ include/dirent.h	17 May 2006 15:38:41 -0000
@@ -62,7 +62,7 @@
 	char	*dd_buf;	/* data buffer */
 	int	dd_len;		/* size of data buffer */
 	off_t	dd_seek;	/* magic cookie returned by getdents */
-	long	dd_rewind;	/* magic cookie for rewinding */
+	void	*dd_internal;	/* state for seekdir/telldir */
 	int	dd_flags;	/* flags for readdir */
 	void	*dd_lock;	/* lock for concurrent access */
 };
@@ -94,7 +94,7 @@
 #endif
 #if defined(_XOPEN_SOURCE) || defined(_NETBSD_SOURCE)
 void seekdir(DIR *, long);
-long telldir(const DIR *);
+long telldir(DIR *);
 #endif /* defined(_NETBSD_SOURCE) || defined(_XOPEN_SOURCE) */
 #if defined(_NETBSD_SOURCE)
 #ifndef __LIBC12_SOURCE__
Index: lib/libc/gen/closedir.c
===================================================================
RCS file: /cvsroot/src/lib/libc/gen/closedir.c,v
retrieving revision 1.14
diff -u -r1.14 closedir.c
--- lib/libc/gen/closedir.c	24 Jan 2006 19:33:10 -0000	1.14
+++ lib/libc/gen/closedir.c	17 May 2006 15:38:41 -0000
@@ -49,6 +49,8 @@
 #include <stdlib.h>
 #include <unistd.h>
 
+#include "dirent_private.h"
+
 #ifdef __weak_alias
 __weak_alias(closedir,_closedir)
 #endif
@@ -57,10 +59,10 @@
  * close a directory.
  */
 int
-closedir(dirp)
-	DIR *dirp;
+closedir(DIR *dirp)
 {
 	int fd;
+	struct dirpos *poslist;
 
 	_DIAGASSERT(dirp != NULL);
 
@@ -68,11 +70,18 @@
 	if (__isthreaded)
 		mutex_lock((mutex_t *)dirp->dd_lock);
 #endif
-	_seekdir_unlocked(dirp, dirp->dd_rewind);/* free seekdir storage */
 	fd = dirp->dd_fd;
 	dirp->dd_fd = -1;
 	dirp->dd_loc = 0;
-	free((void *)dirp->dd_buf);
+	free(dirp->dd_buf);
+
+	/* free seekdir/telldir storage */
+	for (poslist = dirp->dd_internal; poslist; ) {
+		struct dirpos *nextpos = poslist->dp_next;
+		free(poslist);
+		poslist = nextpos;
+	}
+
 #ifdef _REENTRANT
 	if (__isthreaded) {
 		mutex_unlock((mutex_t *)dirp->dd_lock);
Index: lib/libc/gen/directory.3
===================================================================
RCS file: /cvsroot/src/lib/libc/gen/directory.3,v
retrieving revision 1.24
diff -u -r1.24 directory.3
--- lib/libc/gen/directory.3	7 Aug 2003 16:42:47 -0000	1.24
+++ lib/libc/gen/directory.3	17 May 2006 15:38:41 -0000
@@ -29,7 +29,7 @@
 .\"
 .\"     @(#)directory.3	8.1 (Berkeley) 6/4/93
 .\"
-.Dd May 28, 2003
+.Dd May 16, 2006
 .Dt DIRECTORY 3
 .Os
 .Sh NAME
@@ -53,7 +53,7 @@
 .Ft int
 .Fn readdir_r "DIR * restrict dirp" "struct dirent * restrict entry" "struct dirent ** restrict result"
 .Ft long
-.Fn telldir "const DIR *dirp"
+.Fn telldir "DIR *dirp"
 .Ft void
 .Fn seekdir "DIR *dirp" "long  loc"
 .Ft void
Index: lib/libc/gen/opendir.c
===================================================================
RCS file: /cvsroot/src/lib/libc/gen/opendir.c,v
retrieving revision 1.30
diff -u -r1.30 opendir.c
--- lib/libc/gen/opendir.c	24 Jan 2006 19:33:10 -0000	1.30
+++ lib/libc/gen/opendir.c	17 May 2006 15:38:41 -0000
@@ -53,12 +53,13 @@
 #include <string.h>
 #include <unistd.h>
 
+#include "dirent_private.h"
+
 /*
  * Open a directory.
  */
 DIR *
-opendir(name)
-	const char *name;
+opendir(const char *name)
 {
 
 	_DIAGASSERT(name != NULL);
@@ -67,9 +68,7 @@
 }
 
 DIR *
-__opendir2(name, flags)
-	const char *name;
-	int flags;
+__opendir2(const char *name, int flags)
 {
 	DIR *dirp = NULL;
 	int fd;
@@ -304,7 +303,8 @@
 		mutex_init((mutex_t *)dirp->dd_lock, NULL);
 	}
 #endif
-	dirp->dd_rewind = _telldir_unlocked(dirp);
+	dirp->dd_internal = NULL;
+	(void)_telldir_unlocked(dirp);
 	return (dirp);
 error:
 	serrno = errno;
Index: lib/libc/gen/readdir.c
===================================================================
RCS file: /cvsroot/src/lib/libc/gen/readdir.c,v
retrieving revision 1.22
diff -u -r1.22 readdir.c
--- lib/libc/gen/readdir.c	24 Jan 2006 19:33:10 -0000	1.22
+++ lib/libc/gen/readdir.c	17 May 2006 15:38:42 -0000
@@ -48,6 +48,8 @@
 #include <string.h>
 #include <unistd.h>
 
+#include "dirent_private.h"
+
 /*
  * get next entry in a directory.
  */
Index: lib/libc/gen/rewinddir.c
===================================================================
RCS file: /cvsroot/src/lib/libc/gen/rewinddir.c,v
retrieving revision 1.11
diff -u -r1.11 rewinddir.c
--- lib/libc/gen/rewinddir.c	24 Jan 2006 19:33:10 -0000	1.11
+++ lib/libc/gen/rewinddir.c	17 May 2006 15:38:42 -0000
@@ -45,25 +45,27 @@
 
 #include <dirent.h>
 
+#include "dirent_private.h"
+
 #ifdef __weak_alias
 __weak_alias(rewinddir,_rewinddir)
 #endif
 
 void
-rewinddir(dirp)
-	DIR *dirp;
+rewinddir(DIR *dirp)
 {
+	struct dirpos *dp = dirp->dd_internal;
+
+	while (dp->dp_next)
+		dp = dp->dp_next;
 
 #ifdef _REENTRANT
 	if (__isthreaded) {
 		mutex_lock((mutex_t *)dirp->dd_lock);
-		_seekdir_unlocked(dirp, dirp->dd_rewind);
-		dirp->dd_rewind = _telldir_unlocked(dirp);
+		_seekdir_unlocked(dirp, (long)(intptr_t)dp);
 		mutex_unlock((mutex_t *)dirp->dd_lock);
 		return;
 	}
 #endif
-	_seekdir_unlocked(dirp, dirp->dd_rewind);
-	dirp->dd_rewind = _telldir_unlocked(dirp);
-
+	_seekdir_unlocked(dirp, (long)(intptr_t)dp);
 }
Index: lib/libc/gen/seekdir.c
===================================================================
RCS file: /cvsroot/src/lib/libc/gen/seekdir.c,v
retrieving revision 1.13
diff -u -r1.13 seekdir.c
--- lib/libc/gen/seekdir.c	24 Jan 2006 19:33:10 -0000	1.13
+++ lib/libc/gen/seekdir.c	17 May 2006 15:38:42 -0000
@@ -45,6 +45,8 @@
 
 #include <dirent.h>
 
+#include "dirent_private.h"
+
 #ifdef __weak_alias
 __weak_alias(seekdir,_seekdir)
 #endif
@@ -54,9 +56,7 @@
  * _seekdir_unlocked is in telldir.c so that it can share opaque data structures.
  */
 void
-seekdir(dirp, loc)
-	DIR *dirp;
-	long loc;
+seekdir(DIR *dirp, long loc)
 {
 
 #ifdef _REENTRANT
Index: lib/libc/gen/telldir.c
===================================================================
RCS file: /cvsroot/src/lib/libc/gen/telldir.c,v
retrieving revision 1.17
diff -u -r1.17 telldir.c
--- lib/libc/gen/telldir.c	24 Jan 2006 19:33:10 -0000	1.17
+++ lib/libc/gen/telldir.c	17 May 2006 15:38:42 -0000
@@ -48,48 +48,24 @@
 #include <stdlib.h>
 #include <unistd.h>
 
+#include "dirent_private.h"
+
 #ifdef __weak_alias
 __weak_alias(telldir,_telldir)
 #endif
 
-/*
- * The option SINGLEUSE may be defined to say that a telldir
- * cookie may be used only once before it is freed. This option
- * is used to avoid having memory usage grow without bound.
- */
-#define SINGLEUSE
-
-/*
- * One of these structures is malloced to describe the current directory
- * position each time telldir is called. It records the current magic 
- * cookie returned by getdirentries and the offset within the buffer
- * associated with that return value.
- */
-struct ddloc {
-	struct	ddloc *loc_next;/* next structure in list */
-	long	loc_index;	/* key associated with structure */
-	off_t	loc_seek;	/* magic cookie returned by getdirentries */
-	long	loc_loc;	/* offset of entry in buffer */
-};
-
-#define	NDIRHASH	32	/* Num of hash lists, must be a power of 2 */
-#define	LOCHASH(i)	(((int)i)&(NDIRHASH-1))
-
-static long	dd_loccnt;	/* Index of entry for sequential readdir's */
-static struct	ddloc *dd_hash[NDIRHASH];   /* Hash list heads for ddlocs */
-
 long
-telldir(const DIR *dirp)
+telldir(DIR *dirp)
 {
 	long rv;
 #ifdef _REENTRANT
 	if (__isthreaded) {
 		mutex_lock((mutex_t *)dirp->dd_lock);
-		rv = _telldir_unlocked(dirp);
+		rv = (intptr_t)_telldir_unlocked(dirp);
 		mutex_unlock((mutex_t *)dirp->dd_lock);
 	} else
 #endif
-		rv = _telldir_unlocked(dirp);
+		rv = (intptr_t)_telldir_unlocked(dirp);
 	return rv;
 }
 
@@ -97,20 +73,24 @@
  * return a pointer into a directory
  */
 long
-_telldir_unlocked(const DIR *dirp)
+_telldir_unlocked(DIR *dirp)
 {
-	long idx;
-	struct ddloc *lp;
+	struct dirpos *lp;
 
-	if ((lp = (struct ddloc *)malloc(sizeof(struct ddloc))) == NULL)
+	for (lp = dirp->dd_internal; lp; lp = lp->dp_next)
+		if (lp->dp_seek == dirp->dd_seek &&
+		    lp->dp_loc == dirp->dd_loc)
+			return (intptr_t)lp;
+
+	if ((lp = malloc(sizeof(*lp))) == NULL)
 		return (-1);
-	idx = dd_loccnt++;
-	lp->loc_index = idx;
-	lp->loc_seek = dirp->dd_seek;
-	lp->loc_loc = dirp->dd_loc;
-	lp->loc_next = dd_hash[LOCHASH(idx)];
-	dd_hash[LOCHASH(idx)] = lp;
-	return (idx);
+
+	lp->dp_seek = dirp->dd_seek;
+	lp->dp_loc = dirp->dd_loc;
+	lp->dp_next = dirp->dd_internal;
+	dirp->dd_internal = lp;
+
+	return (intptr_t)lp;
 }
 
 /*
@@ -120,35 +100,23 @@
 void
 _seekdir_unlocked(DIR *dirp, long loc)
 {
-	struct ddloc *lp;
-	struct ddloc **prevlp;
-	struct dirent *dp;
+	struct dirpos *lp;
 
 	_DIAGASSERT(dirp != NULL);
 
-	prevlp = &dd_hash[LOCHASH(loc)];
-	lp = *prevlp;
-	while (lp != NULL) {
-		if (lp->loc_index == loc)
+	for (lp = dirp->dd_internal; lp; lp = lp->dp_next)
+		if ((intptr_t)lp == loc)
 			break;
-		prevlp = &lp->loc_next;
-		lp = lp->loc_next;
-	}
+
 	if (lp == NULL)
 		return;
-	if (lp->loc_loc == dirp->dd_loc && lp->loc_seek == dirp->dd_seek)
-		goto found;
-	(void) lseek(dirp->dd_fd, (off_t)lp->loc_seek, SEEK_SET);
-	dirp->dd_seek = lp->loc_seek;
+
+	if (lp->dp_loc == dirp->dd_loc && lp->dp_seek == dirp->dd_seek)
+		return;
+
+	dirp->dd_seek = lseek(dirp->dd_fd, lp->dp_seek, SEEK_SET);
 	dirp->dd_loc = 0;
-	while (dirp->dd_loc < lp->loc_loc) {
-		dp = _readdir_unlocked(dirp);
-		if (dp == NULL)
+	while (dirp->dd_loc < lp->dp_loc)
+		if (_readdir_unlocked(dirp) == NULL)
 			break;
-	}
-found:
-#ifdef SINGLEUSE
-	*prevlp = lp->loc_next;
-	free(lp);
-#endif
 }
Index: lib/libc/include/extern.h
===================================================================
RCS file: /cvsroot/src/lib/libc/include/extern.h,v
retrieving revision 1.12
diff -u -r1.12 extern.h
--- lib/libc/include/extern.h	24 Feb 2006 19:33:09 -0000	1.12
+++ lib/libc/include/extern.h	17 May 2006 15:38:42 -0000
@@ -46,14 +46,6 @@
 int __sigaction_sigtramp __P((int, const struct sigaction *,
     struct sigaction *, const void *, int));
 
-struct _dirdesc;
-void _seekdir_unlocked(struct _dirdesc *, long);
-long _telldir_unlocked(const struct _dirdesc *);
-#ifndef __LIBC12_SOURCE__
-struct dirent;
-struct dirent *_readdir_unlocked(struct _dirdesc *) __RENAME(___readdir_unlocked30);
-#endif
-
 #ifdef WIDE_DOUBLE
 char *__hdtoa(double, const char *, int, int *, int *, char **);
 char *__hldtoa(long double, const char *, int, int *, int *,  char **);
--- /dev/null	2006-05-17 11:38:40.000000000 -0400
+++ lib/libc/gen/dirent_private.h	2006-05-16 22:31:32.000000000 -0400
@@ -0,0 +1,22 @@
+/*	$NetBSD: pw_private.h,v 1.2 2003/07/26 19:24:43 salo Exp $	*/
+
+/*
+ * One struct _dirpos is malloced to describe the current directory
+ * position each time telldir is called. It records the current magic 
+ * cookie returned by getdirentries and the offset within the buffer
+ * associated with that return value.
+ */
+struct dirpos {
+	struct dirpos *dp_next;	/* next structure in list */
+	off_t	dp_seek;	/* magic cookie returned by getdirentries */
+	long	dp_loc;		/* offset of entry in buffer */
+};
+
+struct _dirdesc;
+void _seekdir_unlocked(struct _dirdesc *, long);
+long _telldir_unlocked(struct _dirdesc *);
+#ifndef __LIBC12_SOURCE__
+struct dirent;
+struct dirent *_readdir_unlocked(struct _dirdesc *)
+    __RENAME(___readdir_unlocked30);
+#endif