2004-11-09 01:33:25

by René Scharfe

[permalink] [raw]
Subject: [PATCH 0/4] VFAT cleanup

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?


2004-11-09 01:38:48

by lsr

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

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)

2004-11-09 01:44:09

by lsr

[permalink] [raw]
Subject: [PATCH 2/4] Return better error codes from vfat_valid_longname()

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;

2004-11-09 01:50:09

by lsr

[permalink] [raw]
Subject: [PATCH 4/4] Manually inline shortname_info_to_lcase()

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)

2004-11-09 01:54:43

by lsr

[permalink] [raw]
Subject: [PATCH 3/4] Simplify checks for unwanted chars

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++;
}

2004-11-09 15:04:14

by OGAWA Hirofumi

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

[email protected] 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 <[email protected]>

2004-11-09 16:00:26

by OGAWA Hirofumi

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

[email protected] 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 <[email protected]>

2004-11-09 16:00:41

by OGAWA Hirofumi

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

[email protected] 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 <[email protected]>

2004-11-09 16:03:17

by OGAWA Hirofumi

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

[email protected] 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 <[email protected]>

2004-11-09 16:24:21

by René Scharfe

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

OGAWA Hirofumi wrote:
> [email protected] 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?

2004-11-09 16:49:22

by René Scharfe

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

OGAWA Hirofumi wrote:
> [email protected] 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;

2004-11-09 17:25:58

by OGAWA Hirofumi

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

Ren? Scharfe <[email protected]> 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 <[email protected]>

2004-11-09 17:35:11

by OGAWA Hirofumi

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

Rene Scharfe <[email protected]> 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 <[email protected]>

2004-11-09 18:22:45

by René Scharfe

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

On Wed, Nov 10, 2004 at 02:25:28AM +0900, OGAWA Hirofumi wrote:
> Ren? Scharfe <[email protected]> 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++;
}

2004-11-09 18:36:14

by René Scharfe

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

On Wed, Nov 10, 2004 at 02:35:00AM +0900, OGAWA Hirofumi wrote:
> Rene Scharfe <[email protected]> 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;

2004-11-09 18:42:13

by OGAWA Hirofumi

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

Rene Scharfe <[email protected]> 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 <[email protected]>

2004-11-09 19:40:16

by OGAWA Hirofumi

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

Rene Scharfe <[email protected]> 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 <[email protected]>