Al Viro <viro@xxxxxxxxxxxxxxxxxx> writes: > An obvious way to deal with that would be to have filldir_t return > bool; true => stop, false => go on. However, that's going to cause silent > breakage of out-of-tree filesystems. Even though e.g. ext2 had always > treated any non-zero return value of filldir as "stop", we'd grown filesystems > that check for return value < 0 instead. Having it return true (i.e. 1) > will break all of those. Note that generic callbacks currently return > negative values for "stop", so they work with those filesystems right now. > > FWIW, I'm seriously tempted to go for the following variant: > new __bitwise type with two values - READDIR_STOP and READDIR_CONTINUE, > -1 and 0 resp. The type of filldir_t would become filldir_res_t (*)(......), > all existing instances of ->readdir() would keep working and sparse > would catch all mismatches. > > Another variant is to change filldir_t in visible incompatible way > that would have bool as return value and let readdir instances adjust or > refuse to compile; maintainers of out-of-tree code would presumably notice > that, so we would just have to document the calling conventions and say > that checking for < 0 doesn't work anymore. Personally, I'd like latter than would it work. And I hope we don't do this again... In the case of -EOVERFLOW, even current generic filldir() is strange like following, and I saw simular in past. -- OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> The return value of fillonedir/filldir() is for the caller of fillonedir/filldir(). If we want to tell the result to the caller of ->readdir(), we need to set it to buf->result/error. This fixes -EOVERFLOW case, and cleans up related stuff. [BTW: 32bit compat of ia64/powerpc seems to need to update] Signed-off-by: OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> --- fs/readdir.c | 45 ++++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff -puN fs/readdir.c~filldir-errcode-fix fs/readdir.c --- linux-2.6/fs/readdir.c~filldir-errcode-fix 2008-02-02 04:03:55.000000000 +0900 +++ linux-2.6-hirofumi/fs/readdir.c 2008-02-02 04:03:55.000000000 +0900 @@ -46,18 +46,15 @@ out: EXPORT_SYMBOL(vfs_readdir); +#ifdef __ARCH_WANT_OLD_READDIR /* * Traditional linux readdir() handling.. * - * "count=1" is a special case, meaning that the buffer is one + * "result=1" is a special case, meaning that the buffer is one * dirent-structure in size and that the code can't handle more * anyway. Thus the special "fillonedir()" function for that * case (the low-level handlers don't need to care about this). */ -#define NAME_OFFSET(de) ((int) ((de)->d_name - (char __user *) (de))) - -#ifdef __ARCH_WANT_OLD_READDIR - struct old_linux_dirent { unsigned long d_ino; unsigned long d_offset; @@ -80,8 +77,10 @@ static int fillonedir(void * __buf, cons if (buf->result) return -EINVAL; d_ino = ino; - if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) - return -EOVERFLOW; + if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) { + buf->result = -EOVERFLOW; + goto error; + } buf->result++; dirent = buf->dirent; if (!access_ok(VERIFY_WRITE, dirent, @@ -97,7 +96,8 @@ static int fillonedir(void * __buf, cons return 0; efault: buf->result = -EFAULT; - return -EFAULT; +error: + return buf->result; } asmlinkage long old_readdir(unsigned int fd, struct old_linux_dirent __user * dirent, unsigned int count) @@ -122,9 +122,10 @@ asmlinkage long old_readdir(unsigned int out: return error; } - #endif /* __ARCH_WANT_OLD_READDIR */ +#define NAME_OFFSET(de) ((int) ((de)->d_name - (char __user *) (de))) + /* * New, all-improved, singing, dancing, iBCS2-compliant getdents() * interface. @@ -151,12 +152,15 @@ static int filldir(void * __buf, const c unsigned long d_ino; int reclen = ALIGN(NAME_OFFSET(dirent) + namlen + 2, sizeof(long)); - buf->error = -EINVAL; /* only used if we fail.. */ - if (reclen > buf->count) - return -EINVAL; + if (reclen > buf->count) { + buf->error = -EINVAL; + goto error; + } d_ino = ino; - if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) - return -EOVERFLOW; + if (sizeof(d_ino) < sizeof(ino) && d_ino != ino) { + buf->error = -EOVERFLOW; + goto error; + } dirent = buf->previous; if (dirent) { if (__put_user(offset, &dirent->d_off)) @@ -180,7 +184,8 @@ static int filldir(void * __buf, const c return 0; efault: buf->error = -EFAULT; - return -EFAULT; +error: + return buf->error; } asmlinkage long sys_getdents(unsigned int fd, struct linux_dirent __user * dirent, unsigned int count) @@ -236,9 +241,10 @@ static int filldir64(void * __buf, const struct getdents_callback64 * buf = (struct getdents_callback64 *) __buf; int reclen = ALIGN(NAME_OFFSET(dirent) + namlen + 1, sizeof(u64)); - buf->error = -EINVAL; /* only used if we fail.. */ - if (reclen > buf->count) - return -EINVAL; + if (reclen > buf->count) { + buf->error = -EINVAL; + goto error; + } dirent = buf->previous; if (dirent) { if (__put_user(offset, &dirent->d_off)) @@ -264,7 +270,8 @@ static int filldir64(void * __buf, const return 0; efault: buf->error = -EFAULT; - return -EFAULT; +error: + return buf->error; } asmlinkage long sys_getdents64(unsigned int fd, struct linux_dirent64 __user * dirent, unsigned int count) _ -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html