Re: [PATCH] apr_dir_read doesn't return requested information
William A. Rowe, Jr. wrote: At 11:48 AM 12/18/2002, William A. Rowe, Jr. wrote: At 08:14 AM 12/18/2002, Philip Martin wrote: This is for dir.c version 1.71 with the patch reverted. The Subversion code is svn_io_get_dirents in subversion/libsvn_subr/io.c, it passes APR_FINFO_TYPE | APR_FINFO_NAME to apr_dir_read. The first two calls to apr_dir_read return . and .. and the Subversion code skips them, the following gdb information is for the third call ,,, never mind my earlier questions. Committed a patch to ignore the results of d_type when it's DT_UNKNOWN (or a code we don't grok) and ignore the results of d_fileno/d_ino when the value is 0 or -1. Yes, I'd figured on something like that being the correct fix. But I'm not sure what to use as an invalid inode number; -1 almost certainly, but I have a horrible suspicion that 0 might be a valid inode. -- Brane ibej [EMAIL PROTECTED] http://www.xbc.nu/brane/
Re: [PATCH] apr_dir_read doesn't return requested information
As much as I agree 0 might be a valid inode... I strongly suspect that 0 would be reserved for the boot sector or other filesystem tables. I'm not too worried that 0 is a valid file of anything other than '/' Bill At 07:19 PM 12/18/2002, =?UTF-8?B?QnJhbmtvIMSMaWJlag==?= wrote: William A. Rowe, Jr. wrote: At 11:48 AM 12/18/2002, William A. Rowe, Jr. wrote: At 08:14 AM 12/18/2002, Philip Martin wrote: This is for dir.c version 1.71 with the patch reverted. The Subversion code is svn_io_get_dirents in subversion/libsvn_subr/io.c, it passes APR_FINFO_TYPE | APR_FINFO_NAME to apr_dir_read. The first two calls to apr_dir_read return . and .. and the Subversion code skips them, the following gdb information is for the third call ,,, never mind my earlier questions. Committed a patch to ignore the results of d_type when it's DT_UNKNOWN (or a code we don't grok) and ignore the results of d_fileno/d_ino when the value is 0 or -1. Yes, I'd figured on something like that being the correct fix. But I'm not sure what to use as an invalid inode number; -1 almost certainly, but I have a horrible suspicion that 0 might be a valid inode. -- Brane Čibej [EMAIL PROTECTED] http://www.xbc.nu/brane/
Re: [PATCH] apr_dir_read doesn't return requested information
Philip Martin wrote: Philip Martin [EMAIL PROTECTED] writes: Eeek! I've just upgraded to apache/apr/apr-util to HEAD and now I can reproduce this. $ svnadmin create repo $ svn mkdir file://`pwd`/repo/foo $ svn co file://`pwd`/repo wc $ svn up wc ../svn/subversion/libsvn_wc/adm_crawler.c:315: (apr_err=155000, src_err=0) svn: Obstructed update svn: The entry 'bar' is no longer a directory, which prevents proper updates. Please remove this entry and try updating again. Looks like a recent apr change causes apr_dir_read to fail to return all the requested information. I don't know if this is complete from an apr point of view, but it's sufficient to get Subversion working on my glibc 2.2.5 Linux machine. Index: apr/file_io/unix/dir.c === RCS file: /home/cvspublic/apr/file_io/unix/dir.c,v retrieving revision 1.69 diff -u -r1.69 dir.c --- apr/file_io/unix/dir.c 15 Dec 2002 05:17:51 - 1.69 +++ apr/file_io/unix/dir.c 17 Dec 2002 00:49:35 - @@ -218,10 +218,10 @@ return ret; } -#ifdef DIRENT_INODE +#ifndef DIRENT_INODE wanted = ~APR_FINFO_INODE; #endif -#ifdef DIRENT_TYPE +#ifndef DIRENT_TYPE wanted = ~APR_FINFO_TYPE; #endif Yup, your patch fixes my problem, too. Committed in version 1.70. Thanks! -- Brane ibej [EMAIL PROTECTED] http://www.xbc.nu/brane/
Re: [PATCH] apr_dir_read doesn't return requested information
I'm sorry... this patch dir not come through to [EMAIL PROTECTED] for me today (although I watched for it...) but it's simply WRONG. At 07:04 PM 12/17/2002, =?UTF-8?B?QnJhbmtvIMSMaWJlag==?= wrote: --- apr/file_io/unix/dir.c 15 Dec 2002 05:17:51 - 1.69 +++ apr/file_io/unix/dir.c 17 Dec 2002 00:49:35 - @@ -218,10 +218,10 @@ return ret; } -#ifdef DIRENT_INODE +#ifndef DIRENT_INODE wanted = ~APR_FINFO_INODE; #endif Old logic; if we have an INODE from dirent, we don't care that we want an INODE from stat() because we already have the INODE. New Logic: if we don't have an INODE, we won't ask for an INODE from stat(). I'm sorry, but that's just broken. ' Please revert and (re)post the original description of the problem. If you pass APR_FINFO_TYPE | APR_FINFO_INDOE | APR_FINFO_NAME that is *ALL* you are promised... we do NOT stat for info you don't ask for. Bill
Re: [PATCH] apr_dir_read doesn't return requested information
On Tue, 17 Dec 2002, William A. Rowe, Jr. wrote: I'm sorry... this patch dir not come through to [EMAIL PROTECTED] for me today (although I watched for it...) but it's simply WRONG. At 07:04 PM 12/17/2002, =?UTF-8?B?QnJhbmtvIMSMaWJlag==?= wrote: --- apr/file_io/unix/dir.c 15 Dec 2002 05:17:51 - 1.69 +++ apr/file_io/unix/dir.c 17 Dec 2002 00:49:35 - @@ -218,10 +218,10 @@ return ret; } -#ifdef DIRENT_INODE +#ifndef DIRENT_INODE wanted = ~APR_FINFO_INODE; #endif Old logic; if we have an INODE from dirent, we don't care that we want an INODE from stat() because we already have the INODE. New Logic: if we don't have an INODE, we won't ask for an INODE from stat(). I'm sorry, but that's just broken. ' Please revert and (re)post the original description of the problem. If you pass APR_FINFO_TYPE | APR_FINFO_INDOE | APR_FINFO_NAME that is *ALL* you are promised... we do NOT stat for info you don't ask for. Am I the only person who believes that our stat API is incredibly complex and over-engineered? If I am, I will drop it, but if I'm not can we take the time to fix it? Ryan
Re: [PATCH] apr_dir_read doesn't return requested information
William A. Rowe, Jr. wrote: I'm sorry... this patch dir not come through to [EMAIL PROTECTED] for me today (although I watched for it...) but it's simply WRONG. At 07:04 PM 12/17/2002, =?UTF-8?B?QnJhbmtvIMSMaWJlag==?= wrote: --- apr/file_io/unix/dir.c 15 Dec 2002 05:17:51 - 1.69 +++ apr/file_io/unix/dir.c 17 Dec 2002 00:49:35 - @@ -218,10 +218,10 @@ return ret; } -#ifdef DIRENT_INODE +#ifndef DIRENT_INODE wanted = ~APR_FINFO_INODE; #endif Old logic; if we have an INODE from dirent, we don't care that we want an INODE from stat() because we already have the INODE. New Logic: if we don't have an INODE, we won't ask for an INODE from stat(). I'm sorry, but that's just broken. ' Please revert and (re)post the original description of the problem. If you pass APR_FINFO_TYPE | APR_FINFO_INDOE | APR_FINFO_NAME that is *ALL* you are promised... we do NOT stat for info you don't ask for. Bill Here's the original report: Philip Martin [EMAIL PROTECTED] writes: Eeek! I've just upgraded to apache/apr/apr-util to HEAD and now I can reproduce this. $ svnadmin create repo $ svn mkdir file://`pwd`/repo/foo $ svn co file://`pwd`/repo wc $ svn up wc ../svn/subversion/libsvn_wc/adm_crawler.c:315: (apr_err=155000, src_err=0) svn: Obstructed update svn: The entry 'bar' is no longer a directory, which prevents proper updates. Please remove this entry and try updating again. Looks like a recent apr change causes apr_dir_read to fail to return all the requested information. I don't know if this is complete from an apr point of view, but it's sufficient to get Subversion working on my glibc 2.2.5 Linux machine. Index: apr/file_io/unix/dir.c === RCS file: /home/cvspublic/apr/file_io/unix/dir.c,v retrieving revision 1.69 diff -u -r1.69 dir.c --- apr/file_io/unix/dir.c 15 Dec 2002 05:17:51 - 1.69 +++ apr/file_io/unix/dir.c 17 Dec 2002 00:49:35 - @@ -218,10 +218,10 @@ return ret; } -#ifdef DIRENT_INODE +#ifndef DIRENT_INODE wanted = ~APR_FINFO_INODE; #endif -#ifdef DIRENT_TYPE +#ifndef DIRENT_TYPE wanted = ~APR_FINFO_TYPE; #endif -- Philip Martin Obviously, the type at least did not make it into the fle info. Looking at this code again, the patch may indeed be wrong; but I find it really, really hard to follow that code. In fact, I can't understand it at all. If you can enlighten me about what's happening there, I may be able to come up with a better patch. I reverted my change, but be aware that apr_dir_read is currently broken. It simply does not work on Linux with a redent glibc, it also doesn't work in Solaris 7 (at least for me), etc. etc. It must be fixed. -- Brane ibej [EMAIL PROTECTED] http://www.xbc.nu/brane/
Re: [PATCH] apr_dir_read doesn't return requested information
At 10:42 PM 12/17/2002, =?UTF-8?B?QnJhbmtvIMSMaWJlag==?= wrote: Obviously, the type at least did not make it into the fle info. Looking at this code again, the patch may indeed be wrong; but I find it really, really hard to follow that code. In fact, I can't understand it at all. If you can enlighten me about what's happening there, I may be able to come up with a better patch. What I would like to know (if you can track it...) Is it possible to dump the finfo structure within gdb at the point this request fails? I'd pay especially close attention to the .valid bits, since those are the identifiers that will help us determine if stat() was also called later. First; the definition of all the apr_file_info...() fn's is to return *ALL* of the available information. If more information is available (without extra effort) than the user requests, that is always OK. If some 'extra step' must be performed on a given platfrom (e.g. win32 can't just get an inode/dev without drilling with an open file handle), then we *must* perform that extra step. I'm very interested in what is returned in .filetype. The definition of d_type is to spell out what info is available about the file directory entry. This is an lstat() style info (that is, the file doesn't reside in a directory, the link to the file does.) I reverted my change, but be aware that apr_dir_read is currently broken. It simply does not work on Linux with a redent glibc, it also doesn't work in Solaris 7 (at least for me), etc. etc. It must be fixed. Thanks... If you can spell out how we called apr_dir_read it might help me a ton. I'm actively interested in helping debug the issue with the right fix. A.F.A rbb's complaint; if this code is difficult to read, by all means we should refactor for clarity. However, there is no reason *not* to deal with ALL of the information we can recover from struct dirent, if it proves reliable. I strongly suspect we are looking for some unrelated datum that we 1) didn't request, or 2) recovered an APR_INCOMPLETE because some bit was requested that can't be recovered on the platform in question. Or 3) there is some very obvious, fat fingered mistake of mine in the code. Bill
Re: [PATCH] apr_dir_read doesn't return requested information
William A. Rowe, Jr. [EMAIL PROTECTED] writes:
What I would like to know (if you can track it...)
Is it possible to dump the finfo structure within gdb at the point this
request fails? I'd pay especially close attention to the .valid bits, since
those are the identifiers that will help us determine if stat() was also
called later.
This is for dir.c version 1.71 with the patch reverted. The
Subversion code is svn_io_get_dirents in subversion/libsvn_subr/io.c,
it passes APR_FINFO_TYPE | APR_FINFO_NAME to apr_dir_read. The first
two calls to apr_dir_read return . and .. and the Subversion code
skips them, the following gdb information is for the third call
(gdb) s
apr_dir_read (finfo=0xb660, wanted=33587200, thedir=0x809a878) at dir.c:174
174 apr_status_t ret = 0;
(gdb) n
179 ret = readdir_r(thedir-dirstruct, thedir-entry, retent);
(gdb)
184 if(!ret thedir-entry != retent)
(gdb) p ret
$1 = 0
(gdb) p thedir-entry[0]
$2 = {d_ino = 186434, d_off = 13512064, d_reclen = 16, d_type = 0 '\0',
d_name = .., '\0' repeats 253 times}
(gdb) n
194 if (ret == EINVAL) {
(gdb)
214 finfo-fname = NULL;
(gdb)
216 if (ret) {
(gdb)
222 wanted = ~APR_FINFO_INODE;
(gdb) p/x wanted
$3 = 0x2008000
(gdb) n
225 wanted = ~APR_FINFO_TYPE;
(gdb)
228 wanted = ~APR_FINFO_NAME;
(gdb)
230 if (wanted)
(gdb) p wanted
$4 = 0
(gdb) n
244 if (wanted (ret == APR_SUCCESS || ret == APR_INCOMPLETE)) {
(gdb)
251 finfo-pool = thedir-pool;
(gdb)
252 finfo-valid = 0;
(gdb)
254 finfo-filetype =
filetype_from_dirent_type(thedir-entry-DIRENT_TYPE);
(gdb)
255 finfo-valid |= APR_FINFO_TYPE;
(gdb) p finfo-filetype
$5 = APR_UNKFILE
(gdb) n
258 finfo-inode = thedir-entry-DIRENT_INODE;
(gdb)
259 finfo-valid |= APR_FINFO_INODE;
(gdb)
263 finfo-name = apr_pstrdup(thedir-pool, thedir-entry-d_name);
(gdb)
264 finfo-valid |= APR_FINFO_NAME;
(gdb)
266 if (wanted)
(gdb)
269 return APR_SUCCESS;
(gdb)
Subversion explicitly requests APR_FINFO_TYPE but then the
apr_dir_read code clears that bit and so doesn't call apr_lstat.
APR_UNKFILE appears to be correct for d_type of 0, but is not a very
useful thing for APR to return in response to APR_FINFO_TYPE. It's a
change in APR's behaviour and it breaks Subversion.
--
Philip Martin
Re: [PATCH] apr_dir_read doesn't return requested information
At 08:14 AM 12/18/2002, Philip Martin wrote:
William A. Rowe, Jr. [EMAIL PROTECTED] writes:
What I would like to know (if you can track it...)
Is it possible to dump the finfo structure within gdb at the point this
request fails? I'd pay especially close attention to the .valid bits, since
those are the identifiers that will help us determine if stat() was also
called later.
This is for dir.c version 1.71 with the patch reverted. The
Subversion code is svn_io_get_dirents in subversion/libsvn_subr/io.c,
it passes APR_FINFO_TYPE | APR_FINFO_NAME to apr_dir_read. The first
two calls to apr_dir_read return . and .. and the Subversion code
skips them, the following gdb information is for the third call
(gdb) s
apr_dir_read (finfo=0xb660, wanted=33587200, thedir=0x809a878) at dir.c:174
174 apr_status_t ret = 0;
(gdb) n
179 ret = readdir_r(thedir-dirstruct, thedir-entry, retent);
(gdb)
184 if(!ret thedir-entry != retent)
(gdb) p ret
$1 = 0
(gdb) p thedir-entry[0]
$2 = {d_ino = 186434, d_off = 13512064, d_reclen = 16, d_type = 0 '\0',
d_name = .., '\0' repeats 253 times}
Philip... thanks.
Now for the oddball question, looking at dirent.h or it's associate sys/
includes, what symbol DT_xxx (DT_REG, etc) do you find for value 0?
Also, what values do you have for DIRENT_TYPE, DIRENT_INODE from
apr/include/arch/unix/apr_private.h?
Thanks again,
Bill
Re: [PATCH] apr_dir_read doesn't return requested information
William A. Rowe, Jr. [EMAIL PROTECTED] writes:
Philip... thanks.
Now for the oddball question, looking at dirent.h or it's associate sys/
includes, what symbol DT_xxx (DT_REG, etc) do you find for value 0?
/usr/include/dirent.h
/* File types for `d_type'. */
enum
{
DT_UNKNOWN = 0,
# define DT_UNKNOWN DT_UNKNOWN
DT_FIFO = 1,
# define DT_FIFODT_FIFO
DT_CHR = 2,
# define DT_CHR DT_CHR
DT_DIR = 4,
# define DT_DIR DT_DIR
DT_BLK = 6,
# define DT_BLK DT_BLK
DT_REG = 8,
# define DT_REG DT_REG
DT_LNK = 10,
# define DT_LNK DT_LNK
DT_SOCK = 12,
# define DT_SOCKDT_SOCK
DT_WHT = 14
# define DT_WHT DT_WHT
};
Also, what values do you have for DIRENT_TYPE, DIRENT_INODE from
apr/include/arch/unix/apr_private.h?
#define DIRENT_INODE d_fileno
#define DIRENT_TYPE d_type
From the libc info files:
`unsigned char d_type'
This is the type of the file, possibly unknown. The
following constants are defined for its value:
`DT_UNKNOWN'
The type is unknown. On some systems this is the only
value returned.
A test program:
$ cat z.c
#include dirent.h
#include sys/types.h
int main() {
DIR *d = opendir(.);
struct dirent e, *r;
int v = readdir_r(d, e, r);
while (! v r) {
printf(%s %d\n, r-d_name, r-d_type);
v = readdir_r(d, e, r);
}
return 0;
}
$ gcc -o z z.c
$ ls -l
total 13
drwxr-sr-x2 pm pm 48 Dec 18 18:21 foo
-rwxr-xr-x1 pm pm 5147 Dec 18 18:22 z
-rw-r--r--1 pm pm262 Dec 18 18:22 z.c
$ ./z
. 0
.. 0
z 0
foo 0
z.c 0
--
Philip Martin
Re: [PATCH] apr_dir_read doesn't return requested information
At 11:48 AM 12/18/2002, William A. Rowe, Jr. wrote: At 08:14 AM 12/18/2002, Philip Martin wrote: This is for dir.c version 1.71 with the patch reverted. The Subversion code is svn_io_get_dirents in subversion/libsvn_subr/io.c, it passes APR_FINFO_TYPE | APR_FINFO_NAME to apr_dir_read. The first two calls to apr_dir_read return . and .. and the Subversion code skips them, the following gdb information is for the third call ,,, never mind my earlier questions. Committed a patch to ignore the results of d_type when it's DT_UNKNOWN (or a code we don't grok) and ignore the results of d_fileno/d_ino when the value is 0 or -1. Bill
