[PATCH 0/4] VFAT cleanup

[PATCH 0/4] VFAT cleanup

[Rene Scharfe]

Hello,

the following four mails contain patches for a basic cleanup of the
VFAT filename handling code. The intention is to get the (hopefully)
easy changes merged before I post more controversial ones. The patches
are against 2.6.10-rc1-bk18 and they are tested lightly.

Any comments are welcome.

Thanks,
René

[PATCH 1/4] Move check for invalid chars to vfat_valid_longname()

[lsr]

This patch moves the check for invalid characters into
vfat_valid_longname(), i.e. before the filename is converted to Unicode. 
Benefits: It's simpler to check the non-Unicode string and now all
checks are done in one place. Note: we don't need to check for / (slash)
as this is already done by VFS.

--- ./fs/vfat/namei.c.orig	2004-11-08 18:35:52.000000000 +0100
+++ ./fs/vfat/namei.c	2004-11-08 18:37:06.000000000 +0100
@@ -24,6 +24,7 @@
 #include <linux/smp_lock.h>
 #include <linux/buffer_head.h>
 #include <linux/namei.h>
+#include <linux/string.h>
 
 static int vfat_hashi(struct dentry *parent, struct qstr *qstr);
 static int vfat_hash(struct dentry *parent, struct qstr *qstr);
@@ -158,14 +159,6 @@ static int vfat_cmp(struct dentry *dentr
 
 /* Characters that are undesirable in an MS-DOS file name */
 
-static wchar_t bad_chars[] = {
-	/*  `*'     `?'     `<'    `>'      `|'     `"'     `:'     `/' */
-	0x002A, 0x003F, 0x003C, 0x003E, 0x007C, 0x0022, 0x003A, 0x002F,
-	/*  `\' */
-	0x005C, 0,
-};
-#define IS_BADCHAR(uni)	(vfat_unistrchr(bad_chars, (uni)) != NULL)
-
 static wchar_t replace_chars[] = {
 	/*  `['     `]'    `;'     `,'     `+'      `=' */
 	0x005B, 0x005D, 0x003B, 0x002C, 0x002B, 0x003D, 0,
@@ -187,18 +180,10 @@ static inline wchar_t *vfat_unistrchr(co
 	return (wchar_t *) s;
 }
 
-static inline int vfat_is_used_badchars(const wchar_t *s, int len)
-{
-	int i;
-	
-	for (i = 0; i < len; i++)
-		if (s[i] < 0x0020 || IS_BADCHAR(s[i]))
-			return -EINVAL;
-	return 0;
-}
-
 static int vfat_valid_longname(const unsigned char *name, unsigned int len)
 {
+	const unsigned char *p;
+
 	if (len && name[len-1] == ' ')
 		return 0;
 	if (len >= 256)
@@ -221,6 +206,12 @@ static int vfat_valid_longname(const uns
 		}
 	}
 
+	/* check for invalid characters */
+	for (p = name; p < name + len; p++) {
+		if (*p < 0x0020 || strchr("*?<>|\":\\", *p) != NULL)
+			return 0;
+	}
+
 	return 1;
 }
 
@@ -636,10 +627,6 @@ static int vfat_build_slots(struct inode
 	if (res < 0)
 		goto out_free;
 
-	res = vfat_is_used_badchars(uname, ulen);
-	if (res < 0)
-		goto out_free;
-
 	res = vfat_create_shortname(dir, sbi->nls_disk, uname, ulen,
 				    msdos_name, &lcase);
 	if (res < 0)

[PATCH 2/4] Return better error codes from vfat_valid_longname()

[lsr]

Currently vfat returns -EINVAL if one tries to create a file or directory
with an invalid name. This patch changes vfat_valid_longname() to return
a more specific error code.

POSIX doesn't define a nice error code for invalid filenames, so I chose
EACCES -- unlike EINVAL this is a valid error code of mkdir(2). Hope it
sort of fits. (EINVAL did *not* fit; it generally seems to point to
problems not with the filename  but with e.g. the flags value of open(2)
etc.).

As a result file utilities give more meaningful error messages. Example:

	before $ touch nul
	touch: setting times of `nul': No such file or directory

	after $ touch nul
	touch: cannot touch `nul': Permission denied

--- ./fs/vfat/namei.c.orig	2004-11-08 21:33:35.000000000 +0100
+++ ./fs/vfat/namei.c	2004-11-08 21:32:52.000000000 +0100
@@ -184,10 +184,13 @@ static int vfat_valid_longname(const uns
 {
 	const unsigned char *p;
 
-	if (len && name[len-1] == ' ')
-		return 0;
-	if (len >= 256)
-		return 0;
+	if (len == 0)
+		return -ENOENT;
+	if (len > 255)
+		return -ENAMETOOLONG;
+
+	if (name[len-1] == ' ')
+		return -EACCES;
 
 	/* MS-DOS "device special files" */
 	if (len == 3 || (len > 3 && name[3] == '.')) {	/* basename == 3 */
@@ -195,24 +198,24 @@ static int vfat_valid_longname(const uns
 		    !strnicmp(name, "con", 3) ||
 		    !strnicmp(name, "nul", 3) ||
 		    !strnicmp(name, "prn", 3))
-			return 0;
+			return -EACCES;
 	}
 	if (len == 4 || (len > 4 && name[4] == '.')) {	/* basename == 4 */
 		/* "com1", "com2", ... */
 		if ('1' <= name[3] && name[3] <= '9') {
 			if (!strnicmp(name, "com", 3) ||
 			    !strnicmp(name, "lpt", 3))
-				return 0;
+				return -EACCES;
 		}
 	}
 
 	/* check for invalid characters */
 	for (p = name; p < name + len; p++) {
 		if (*p < 0x0020 || strchr("*?<>|\":\\", *p) != NULL)
-			return 0;
+			return -EACCES;
 	}
 
-	return 1;
+	return 0;
 }
 
 static int vfat_find_form(struct inode *dir, unsigned char *name)
@@ -615,8 +618,9 @@ static int vfat_build_slots(struct inode
 	loff_t offset;
 
 	*slots = 0;
-	if (!vfat_valid_longname(name, len))
-		return -EINVAL;
+	res = vfat_valid_longname(name, len);
+	if (res)
+		return res;
 
 	if(!(page = __get_free_page(GFP_KERNEL)))
 		return -ENOMEM;

[PATCH 3/4] Simplify checks for unwanted chars

[lsr]

This patch replaces the macros IS_REPLACECHAR and IS_SKIPCHAR and their
static char arrays with inline functions. IMHO it makes the code
slightly more readable and shrinks both code and compiled text size.

The use of inline functions instead of macros also gives us type safety
without having to resort to casting.

--- ./fs/vfat/namei.c.orig	2004-11-08 21:18:48.000000000 +0100
+++ ./fs/vfat/namei.c	2004-11-08 21:18:19.000000000 +0100
@@ -159,25 +159,20 @@ static int vfat_cmp(struct dentry *dentr
 
 /* Characters that are undesirable in an MS-DOS file name */
 
-static wchar_t replace_chars[] = {
-	/*  `['     `]'    `;'     `,'     `+'      `=' */
-	0x005B, 0x005D, 0x003B, 0x002C, 0x002B, 0x003D, 0,
-};
-#define IS_REPLACECHAR(uni)	(vfat_unistrchr(replace_chars, (uni)) != NULL)
-
-static wchar_t skip_chars[] = {
-	/*  `.'     ` ' */
-	0x002E, 0x0020, 0,
-};
-#define IS_SKIPCHAR(uni) \
-	((wchar_t)(uni) == skip_chars[0] || (wchar_t)(uni) == skip_chars[1])
+static inline int vfat_replace_char(wchar_t w)
+{
+	return (w == 0x005B)	/* [ */
+	    || (w == 0x005D)	/* ] */
+	    || (w == 0x003B)	/* ; */
+	    || (w == 0x002C)	/* , */
+	    || (w == 0x002B)	/* + */
+	    || (w == 0x003D);	/* = */
+}
 
-static inline wchar_t *vfat_unistrchr(const wchar_t *s, const wchar_t c)
+static inline int vfat_skip_char(wchar_t w)
 {
-	for(; *s != c; ++s)
-		if (*s == 0)
-			return NULL;
-	return (wchar_t *) s;
+	return (w == 0x002E)	/* . */
+	    || (w == 0x0020);	/* <space> */
 }
 
 static int vfat_valid_longname(const unsigned char *name, unsigned int len)
@@ -285,11 +280,11 @@ static inline int to_shortname_char(stru
 {
 	int len;
 
-	if (IS_SKIPCHAR(*src)) {
+	if (vfat_skip_char(*src)) {
 		info->valid = 0;
 		return 0;
 	}
-	if (IS_REPLACECHAR(*src)) {
+	if (vfat_replace_char(*src)) {
 		info->valid = 0;
 		buf[0] = '_';
 		return 1;
@@ -369,7 +364,7 @@ static int vfat_create_shortname(struct 
 		 */
 		name_start = &uname[0];
 		while (name_start < ext_start) {
-			if (!IS_SKIPCHAR(*name_start))
+			if (!vfat_skip_char(*name_start))
 				break;
 			name_start++;
 		}

[PATCH 4/4] Manually inline shortname_info_to_lcase()

[lsr]

This patch inlines shortname_info_to_lcase() by hand. At least my
compiler (gcc 3.3.4 from Debian) doesn't go all the way, so the compiled
text size is decreased by this patch. And IMHO the code gets more
readable, too.

The terms (base->valid && ext->valid), (ext->lower || ext->upper) and
(base->lower || base->upper) are trivially found to be true at the
single callsite of shortname_info_to_lcase(). The relevant lines are
included in this patch file.

--- ./fs/vfat/namei.c.orig	2004-11-09 01:40:54.000000000 +0100
+++ ./fs/vfat/namei.c	2004-11-09 01:41:16.000000000 +0100
@@ -258,22 +258,6 @@
 	(x)->valid = 1;				\
 } while (0)
 
-static inline unsigned char
-shortname_info_to_lcase(struct shortname_info *base,
-			struct shortname_info *ext)
-{
-	unsigned char lcase = 0;
-
-	if (base->valid && ext->valid) {
-		if (!base->upper && base->lower && (ext->lower || ext->upper))
-			lcase |= CASE_LOWER_BASE;
-		if (!ext->upper && ext->lower && (base->lower || base->upper))
-			lcase |= CASE_LOWER_EXT;
-	}
-
-	return lcase;
-}
-
 static inline int to_shortname_char(struct nls_table *nls,
 				    unsigned char *buf, int buf_size, wchar_t *src,
 				    struct shortname_info *info)
@@ -447,20 +431,22 @@
 	if (is_shortname && base_info.valid && ext_info.valid) {
 		if (vfat_find_form(dir, name_res) == 0)
 			return -EEXIST;
 
 		if (opt_shortname & VFAT_SFN_CREATE_WIN95) {
 			return (base_info.upper && ext_info.upper);
 		} else if (opt_shortname & VFAT_SFN_CREATE_WINNT) {
 			if ((base_info.upper || base_info.lower)
 			    && (ext_info.upper || ext_info.lower)) {
-				*lcase = shortname_info_to_lcase(&base_info,
-								 &ext_info);
+				if (!base_info.upper && base_info.lower)
+					*lcase |= CASE_LOWER_BASE;
+				if (!ext_info.upper && ext_info.lower)
+					*lcase |= CASE_LOWER_EXT;
 				return 1;
 			}
 			return 0;
 		} else {
 			BUG();
 		}
 	}
 	
 	if (MSDOS_SB(dir->i_sb)->options.numtail == 0)

Re: [PATCH 1/4] Move check for invalid chars to vfat_valid_longname()

[OGAWA Hirofumi]

lsr@neapel230.server4you.de writes:

> +	/* check for invalid characters */
> +	for (p = name; p < name + len; p++) {
> +		if (*p < 0x0020 || strchr("*?<>|\":\\", *p) != NULL)
> +			return 0;
> +	}
> +
>  	return 1;
>  }
>  
> @@ -636,10 +627,6 @@ static int vfat_build_slots(struct inode
>  	if (res < 0)
>  		goto out_free;
>  
> -	res = vfat_is_used_badchars(uname, ulen);
> -	if (res < 0)
> -		goto out_free;
> -
>  	res = vfat_create_shortname(dir, sbi->nls_disk, uname, ulen,
>  				    msdos_name, &lcase);
>  	if (res < 0)

Some encodings is using the area of ascii code as second byte.
So, it can't.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 4/4] Manually inline shortname_info_to_lcase()

[OGAWA Hirofumi]

lsr@neapel230.server4you.de writes:

> @@ -447,20 +431,22 @@
>  	if (is_shortname && base_info.valid && ext_info.valid) {
>  		if (vfat_find_form(dir, name_res) == 0)
>  			return -EEXIST;
>  
>  		if (opt_shortname & VFAT_SFN_CREATE_WIN95) {
>  			return (base_info.upper && ext_info.upper);
>  		} else if (opt_shortname & VFAT_SFN_CREATE_WINNT) {
>  			if ((base_info.upper || base_info.lower)
>  			    && (ext_info.upper || ext_info.lower)) {
> -				*lcase = shortname_info_to_lcase(&base_info,
> -								 &ext_info);
> +				if (!base_info.upper && base_info.lower)
> +					*lcase |= CASE_LOWER_BASE;
> +				if (!ext_info.upper && ext_info.lower)
> +					*lcase |= CASE_LOWER_EXT;

Looks good. Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 2/4] Return better error codes from vfat_valid_longname()

[OGAWA Hirofumi]

lsr@neapel230.server4you.de writes:

> Currently vfat returns -EINVAL if one tries to create a file or directory
> with an invalid name. This patch changes vfat_valid_longname() to return
> a more specific error code.
>
> POSIX doesn't define a nice error code for invalid filenames, so I chose
> EACCES -- unlike EINVAL this is a valid error code of mkdir(2). Hope it
> sort of fits. (EINVAL did *not* fit; it generally seems to point to
> problems not with the filename  but with e.g. the flags value of open(2)
> etc.).

Yes, the error code for this should be consistent on _system_.
Until we do it, this change would not be useful.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 3/4] Simplify checks for unwanted chars

[OGAWA Hirofumi]

lsr@neapel230.server4you.de writes:

> +static inline int vfat_skip_char(wchar_t w)
>  {
> -	for(; *s != c; ++s)
> -		if (*s == 0)
> -			return NULL;
> -	return (wchar_t *) s;
> +	return (w == 0x002E)	/* . */
> +	    || (w == 0x0020);	/* <space> */
>  }

Looks good. However, I can't apply the following patch. Can you also
do it to IS_BADCHARS()?

[PATCH 1/4] Move check for invalid chars to vfat_valid_longname()
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 1/4] Move check for invalid chars to vfat_valid_longname()

[René Scharfe]

OGAWA Hirofumi wrote:
> lsr@neapel230.server4you.de writes:
>>+	/* check for invalid characters */
>>+	for (p = name; p < name + len; p++) {
>>+		if (*p < 0x0020 || strchr("*?<>|\":\\", *p) != NULL)
>>+			return 0;
>>+	}
>>+
>> 	return 1;
>> }
>> 
>>@@ -636,10 +627,6 @@ static int vfat_build_slots(struct inode
>> 	if (res < 0)
>> 		goto out_free;
>> 
>>-	res = vfat_is_used_badchars(uname, ulen);
>>-	if (res < 0)
>>-		goto out_free;
>>-
>> 	res = vfat_create_shortname(dir, sbi->nls_disk, uname, ulen,
>> 				    msdos_name, &lcase);
>> 	if (res < 0)
> 
> 
> Some encodings is using the area of ascii code as second byte.

Yes. But..

> So, it can't.

We want to make sure filenames don't contain '*', '?' etc. It doesn't 
matter whether we check the VFS idea of the filename (a simple C string) 
or some other encoding of it, no?

Right now we check for 0x002A after xlate_to_uni(), after the patch we 
check for 0x2A (ASCII code of '*') before xlate_to_uni() etc. I just 
translated the Unicode chars back to ASCII and moved the check.

Am I missing something?

Thanks,
René

Re: [PATCH 2/4] Return better error codes from vfat_valid_longname()

[Rene Scharfe]

OGAWA Hirofumi wrote:
> lsr@neapel230.server4you.de writes:
> 
> 
>>Currently vfat returns -EINVAL if one tries to create a file or directory
>>with an invalid name. This patch changes vfat_valid_longname() to return
>>a more specific error code.
>>
>>POSIX doesn't define a nice error code for invalid filenames, so I chose
>>EACCES -- unlike EINVAL this is a valid error code of mkdir(2). Hope it
>>sort of fits. (EINVAL did *not* fit; it generally seems to point to
>>problems not with the filename  but with e.g. the flags value of open(2)
>>etc.).
> 
> 
> Yes, the error code for this should be consistent on _system_.
> Until we do it, this change would not be useful.

At least ENAMETOOLONG and ENOENT are properly defined error codes. :)

VFAT returning EACCES when trying to access "forbidden" filenames would
be useful right now because it gives better error messages with current
tools. At least the error msg of touch looks less confusing IMHO
(didn't check much other programs).

And I doubt we'll get an official Linux error code for invalid
filenames soon: according to POSIX there are only two forbidden
characters, '/' and '\0', so anything goes.

Anyway, what do you think about the following patch? I just replaced
EACCES by EINVAL.

Thanks,
René



--- ./fs/vfat/namei.c.orig	2004-11-08 21:33:35.000000000 +0100
+++ ./fs/vfat/namei.c	2004-11-08 21:32:52.000000000 +0100
@@ -184,10 +184,13 @@ static int vfat_valid_longname(const uns
 {
 	const unsigned char *p;
 
-	if (len && name[len-1] == ' ')
-		return 0;
-	if (len >= 256)
-		return 0;
+	if (len == 0)
+		return -ENOENT;
+	if (len > 255)
+		return -ENAMETOOLONG;
+
+	if (name[len-1] == ' ')
+		return -EINVAL;
 
 	/* MS-DOS "device special files" */
 	if (len == 3 || (len > 3 && name[3] == '.')) {	/* basename == 3 */
@@ -195,24 +198,24 @@ static int vfat_valid_longname(const uns
 		    !strnicmp(name, "con", 3) ||
 		    !strnicmp(name, "nul", 3) ||
 		    !strnicmp(name, "prn", 3))
-			return 0;
+			return -EINVAL;
 	}
 	if (len == 4 || (len > 4 && name[4] == '.')) {	/* basename == 4 */
 		/* "com1", "com2", ... */
 		if ('1' <= name[3] && name[3] <= '9') {
 			if (!strnicmp(name, "com", 3) ||
 			    !strnicmp(name, "lpt", 3))
-				return 0;
+				return -EINVAL;
 		}
 	}
 
 	/* check for invalid characters */
 	for (p = name; p < name + len; p++) {
 		if (*p < 0x0020 || strchr("*?<>|\":\\", *p) != NULL)
-			return 0;
+			return -EINVAL;
 	}
 
-	return 1;
+	return 0;
 }
 
 static int vfat_find_form(struct inode *dir, unsigned char *name)
@@ -615,8 +618,9 @@ static int vfat_build_slots(struct inode
 	loff_t offset;
 
 	*slots = 0;
-	if (!vfat_valid_longname(name, len))
-		return -EINVAL;
+	res = vfat_valid_longname(name, len);
+	if (res)
+		return res;
 
 	if(!(page = __get_free_page(GFP_KERNEL)))
 		return -ENOMEM;

Re: [PATCH 1/4] Move check for invalid chars to vfat_valid_longname()

[OGAWA Hirofumi]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> We want to make sure filenames don't contain '*', '?' etc.

No. For example, Shift-JIS has 0x815c. It's contains the '\' (0x5c),
but the 0x815c is a valid char for fatfs.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 2/4] Return better error codes from vfat_valid_longname()

[OGAWA Hirofumi]

Rene Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> At least ENAMETOOLONG and ENOENT are properly defined error codes. :)

Ah, yes. IIRC I already fixed the ENOENT case.
We shouldn't need "len == 0" check, right?

> Anyway, what do you think about the following patch? I just replaced
> EACCES by EINVAL.

Looks good.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 1/4] Move check for invalid chars to vfat_valid_longname()

[Rene Scharfe]

On Wed, Nov 10, 2004 at 02:25:28AM +0900, OGAWA Hirofumi wrote:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
> > We want to make sure filenames don't contain '*', '?' etc.
> 
> No. For example, Shift-JIS has 0x815c. It's contains the '\' (0x5c),
> but the 0x815c is a valid char for fatfs.

Oops -- of course.

But doesn't imply this we can't do any of our checks on the VFS string?
A dot (0x2E) at the end of a filename could be the half of some other
character in some encoding, right? And the same could be said about the
checks in vfat_valid_longname(), no? Do we need to convert them to
Unicode?


The patch you asked for converting IS_BADCHAR to an inline function
follows. I rolled it together with the other conversions from patch 3.
Applies directly on top of 2.6.10-rc1-bk18.

Thanks,
René


--- linux-2.6.10-rc1-bk18/fs/vfat/namei.c.orig	2004-10-18 23:54:37.000000000 +0200
+++ linux-2.6.10-rc1-bk18/fs/vfat/namei.c	2004-11-09 18:56:48.000000000 +0100
@@ -158,33 +158,34 @@ static int vfat_cmp(struct dentry *dentr
 
 /* Characters that are undesirable in an MS-DOS file name */
 
-static wchar_t bad_chars[] = {
-	/*  `*'     `?'     `<'    `>'      `|'     `"'     `:'     `/' */
-	0x002A, 0x003F, 0x003C, 0x003E, 0x007C, 0x0022, 0x003A, 0x002F,
-	/*  `\' */
-	0x005C, 0,
-};
-#define IS_BADCHAR(uni)	(vfat_unistrchr(bad_chars, (uni)) != NULL)
-
-static wchar_t replace_chars[] = {
-	/*  `['     `]'    `;'     `,'     `+'      `=' */
-	0x005B, 0x005D, 0x003B, 0x002C, 0x002B, 0x003D, 0,
-};
-#define IS_REPLACECHAR(uni)	(vfat_unistrchr(replace_chars, (uni)) != NULL)
-
-static wchar_t skip_chars[] = {
-	/*  `.'     ` ' */
-	0x002E, 0x0020, 0,
-};
-#define IS_SKIPCHAR(uni) \
-	((wchar_t)(uni) == skip_chars[0] || (wchar_t)(uni) == skip_chars[1])
+static inline wchar_t vfat_bad_char(wchar_t w)
+{
+	return (w < 0x0020)
+	    || (w == 0x002A)	/* * */
+	    || (w == 0x003F)	/* ? */
+	    || (w == 0x003C)	/* < */
+	    || (w == 0x003E)	/* > */
+	    || (w == 0x007C)	/* | */
+	    || (w == 0x0022)	/* " */
+	    || (w == 0x003A)	/* : */
+	    || (w == 0x002F)	/* / */
+	    || (w == 0x005C);	/* \ */
+}
+
+static inline wchar_t vfat_replace_char(wchar_t w)
+{
+	return (w == 0x005B)	/* [ */
+	    || (w == 0x005D)	/* ] */
+	    || (w == 0x003B)	/* ; */
+	    || (w == 0x002C)	/* , */
+	    || (w == 0x002B)	/* + */
+	    || (w == 0x003D);	/* = */
+}
 
-static inline wchar_t *vfat_unistrchr(const wchar_t *s, const wchar_t c)
+static inline wchar_t vfat_skip_char(wchar_t w)
 {
-	for(; *s != c; ++s)
-		if (*s == 0)
-			return NULL;
-	return (wchar_t *) s;
+	return (w == 0x002E)	/* . */
+	    || (w == 0x0020);	/* <space> */
 }
 
 static inline int vfat_is_used_badchars(const wchar_t *s, int len)
@@ -192,7 +193,7 @@ static inline int vfat_is_used_badchars(
 	int i;
 	
 	for (i = 0; i < len; i++)
-		if (s[i] < 0x0020 || IS_BADCHAR(s[i]))
+		if (vfat_bad_char(s[i]))
 			return -EINVAL;
 	return 0;
 }
@@ -291,11 +292,11 @@ static inline int to_shortname_char(stru
 {
 	int len;
 
-	if (IS_SKIPCHAR(*src)) {
+	if (vfat_skip_char(*src)) {
 		info->valid = 0;
 		return 0;
 	}
-	if (IS_REPLACECHAR(*src)) {
+	if (vfat_replace_char(*src)) {
 		info->valid = 0;
 		buf[0] = '_';
 		return 1;
@@ -375,7 +376,7 @@ static int vfat_create_shortname(struct 
 		 */
 		name_start = &uname[0];
 		while (name_start < ext_start) {
-			if (!IS_SKIPCHAR(*name_start))
+			if (!vfat_skip_char(*name_start))
 				break;
 			name_start++;
 		}

Re: [PATCH 2/4] Return better error codes from vfat_valid_longname()

[Rene Scharfe]

On Wed, Nov 10, 2004 at 02:35:00AM +0900, OGAWA Hirofumi wrote:
> Rene Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
> > At least ENAMETOOLONG and ENOENT are properly defined error codes. :)
> 
> Ah, yes. IIRC I already fixed the ENOENT case.
> We shouldn't need "len == 0" check, right?

Yes. I removed the check and rediffed the patch against the one I sent
a few minutes ago.

René



--- ./fs/vfat/namei.c.orig	2004-11-09 19:32:40.000000000 +0100
+++ ./fs/vfat/namei.c	2004-11-09 19:32:24.000000000 +0100
@@ -200,10 +200,10 @@ static inline int vfat_is_used_badchars(
 
 static int vfat_valid_longname(const unsigned char *name, unsigned int len)
 {
-	if (len && name[len-1] == ' ')
-		return 0;
+	if (name[len-1] == ' ')
+		return -EINVAL;
 	if (len >= 256)
-		return 0;
+		return -ENAMETOOLONG;
 
 	/* MS-DOS "device special files" */
 	if (len == 3 || (len > 3 && name[3] == '.')) {	/* basename == 3 */
@@ -211,18 +211,18 @@ static int vfat_valid_longname(const uns
 		    !strnicmp(name, "con", 3) ||
 		    !strnicmp(name, "nul", 3) ||
 		    !strnicmp(name, "prn", 3))
-			return 0;
+			return -EINVAL;
 	}
 	if (len == 4 || (len > 4 && name[4] == '.')) {	/* basename == 4 */
 		/* "com1", "com2", ... */
 		if ('1' <= name[3] && name[3] <= '9') {
 			if (!strnicmp(name, "com", 3) ||
 			    !strnicmp(name, "lpt", 3))
-				return 0;
+				return -EINVAL;
 		}
 	}
 
-	return 1;
+	return 0;
 }
 
 static int vfat_find_form(struct inode *dir, unsigned char *name)
@@ -625,8 +625,9 @@ static int vfat_build_slots(struct inode
 	loff_t offset;
 
 	*slots = 0;
-	if (!vfat_valid_longname(name, len))
-		return -EINVAL;
+	res = vfat_valid_longname(name, len);
+	if (res)
+		return res;
 
 	if(!(page = __get_free_page(GFP_KERNEL)))
 		return -ENOMEM;

Re: [PATCH 1/4] Move check for invalid chars to vfat_valid_longname()

[OGAWA Hirofumi]

Rene Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> But doesn't imply this we can't do any of our checks on the VFS
> string?

Basically yes.

> A dot (0x2E) at the end of a filename could be the half of some other
> character in some encoding, right?

'.'/' ' is not contained as second byte by any encodings, at least
current nls is supporting encodings.

> And the same could be said about the checks in
> vfat_valid_longname(), no?

These are string, not char. These should be unique.

> The patch you asked for converting IS_BADCHAR to an inline function
> follows. I rolled it together with the other conversions from patch 3.
> Applies directly on top of 2.6.10-rc1-bk18.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH 2/4] Return better error codes from vfat_valid_longname()

[OGAWA Hirofumi]

Rene Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Yes. I removed the check and rediffed the patch against the one I sent
> a few minutes ago.

Thanks. I'll submit after 2.6.10.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>