[PATCH 0/4] fs: fat: code clean up
With the series paths with trailing periods like '/foo/a...' do no lead to creating files and directories with invalid names.
Duplicate code is carved out into a new function.
Patch 1/4 is resent to indicate import sequence.
Heinrich Schuchardt (4): fs: fat: usage basename in file_fat_write_at, fat_mkdir fs: fat: must not write directory '.' and '..' fs: fat: carve out fat_create_dir_entry() fs: fat: remove trailing periods from long name
fs/fat/fat_write.c | 131 ++++++++++++++++++++------------------------- 1 file changed, 57 insertions(+), 74 deletions(-)
-- 2.29.2
This patch involves no functional change. It is just about code readability.
Both in file_fat_write_at() and fat_mkdir() the incoming file or directory path are split into two parts: the parent directory and the base name.
In file_fat_write_at() the value of the variable basename is assigned to the filename parameter and afterwards the variable filename is used instead of basename. It is more readable to use the variable basename and leave filename unchanged.
In fat_mkdir() the base name variable is called directory. This is confusing. Call it basename like in file_fat_write_at(). This allows to rename parameter new_directory to directory in the implementation of fat_mkdir() to match the function declaration.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- fs/fat/fat_write.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index b43a27b205..a9b9fa5d68 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1299,9 +1299,8 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer, goto exit; }
- filename = basename; - if (normalize_longname(l_filename, filename)) { - printf("FAT: illegal filename (%s)\n", filename); + if (normalize_longname(l_filename, basename)) { + printf("FAT: illegal filename (%s)\n", basename); ret = -EINVAL; goto exit; } @@ -1365,7 +1364,7 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer, }
/* Check if long name is needed */ - ndent = set_name(itr, filename, shortname); + ndent = set_name(itr, basename, shortname); if (ndent < 0) { ret = ndent; goto exit; @@ -1375,7 +1374,7 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer, goto exit; if (ndent > 1) { /* Set long name entries */ - ret = fill_dir_slot(itr, filename, shortname); + ret = fill_dir_slot(itr, basename, shortname); if (ret) goto exit; } @@ -1611,31 +1610,31 @@ exit: return ret; }
-int fat_mkdir(const char *new_dirname) +int fat_mkdir(const char *dirname) { dir_entry *retdent; fsdata datablock = { .fatbuf = NULL, }; fsdata *mydata = &datablock; fat_itr *itr = NULL; - char *dirname_copy, *parent, *dirname; + char *dirname_copy, *parent, *basename; char l_dirname[VFAT_MAXLEN_BYTES]; int ret = -1; loff_t actwrite; unsigned int bytesperclust; dir_entry *dotdent = NULL;
- dirname_copy = strdup(new_dirname); + dirname_copy = strdup(dirname); if (!dirname_copy) goto exit;
- split_filename(dirname_copy, &parent, &dirname); - if (!strlen(dirname)) { + split_filename(dirname_copy, &parent, &basename); + if (!strlen(basename)) { ret = -EINVAL; goto exit; }
- if (normalize_longname(l_dirname, dirname)) { - printf("FAT: illegal filename (%s)\n", dirname); + if (normalize_longname(l_dirname, basename)) { + printf("FAT: illegal filename (%s)\n", basename); ret = -EINVAL; goto exit; } @@ -1678,7 +1677,7 @@ int fat_mkdir(const char *new_dirname) }
/* Check if long name is needed */ - ndent = set_name(itr, dirname, shortname); + ndent = set_name(itr, basename, shortname); if (ndent < 0) { ret = ndent; goto exit; @@ -1688,7 +1687,7 @@ int fat_mkdir(const char *new_dirname) goto exit; if (ndent > 1) { /* Set long name entries */ - ret = fill_dir_slot(itr, dirname, shortname); + ret = fill_dir_slot(itr, basename, shortname); if (ret) goto exit; } -- 2.29.2
Directories or files called '.' or '..' cannot be created or written to in any directory. Move the test to normalize_longname() to check this early.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- fs/fat/fat_write.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index a9b9fa5d68..7bd7463464 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1260,6 +1260,9 @@ static int normalize_longname(char *l_filename, const char *filename) { const char *p, illegal[] = "<>:"/\|?*";
+ if (!strcmp(filename, ".") || !strcmp(filename, "..")) + return -1; + if (strlen(filename) >= VFAT_MAXLEN_BYTES) return -1;
@@ -1348,15 +1351,6 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer, char shortname[SHORT_NAME_SIZE]; int ndent;
- if (itr->is_root) { - /* root dir cannot have "." or ".." */ - if (!strcmp(l_filename, ".") || - !strcmp(l_filename, "..")) { - ret = -EINVAL; - goto exit; - } - } - if (pos) { /* No hole allowed */ ret = -EINVAL; -- 2.29.2
fat_mkdir() and file_fat_write_at() use identical code to create a new directory entry. Carve out a new function fat_create_dir_entry() to avoid this code duplication.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- fs/fat/fat_write.c | 93 ++++++++++++++++++++-------------------------- 1 file changed, 40 insertions(+), 53 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 7bd7463464..0f4786ef0f 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1279,6 +1279,43 @@ static int normalize_longname(char *l_filename, const char *filename) return 0; }
+/** + * fat_create_dir_entry() - create directory entry + * + * @itr: directory iterator + * @basename: name of file or directory to be created + * @size: file size + * @attr: file or directory attributes + * Return: 0 for success, -EIO on error + */ +static int fat_create_dir_entry(fat_itr *itr, const char *basename, + loff_t size, u8 attr) +{ + /* Create a new file */ + char shortname[SHORT_NAME_SIZE]; + int ndent; + int ret; + + /* Check if long name is needed */ + ndent = set_name(itr, basename, shortname); + if (ndent < 0) + return ndent; + ret = fat_find_empty_dentries(itr, ndent); + if (ret) + return ret; + if (ndent > 1) { + /* Set long name entries */ + ret = fill_dir_slot(itr, basename, shortname); + if (ret) + return ret; + } + + /* Set short name entry */ + fill_dentry(itr->fsdata, itr->dent, shortname, 0, size, attr); + + return 0; +} + int file_fat_write_at(const char *filename, loff_t pos, void *buffer, loff_t size, loff_t *actwrite) { @@ -1348,8 +1385,6 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer, retdent->size = cpu_to_le32(pos + size); } else { /* Create a new file */ - char shortname[SHORT_NAME_SIZE]; - int ndent;
if (pos) { /* No hole allowed */ @@ -1357,25 +1392,7 @@ int file_fat_write_at(const char *filename, loff_t pos, void *buffer, goto exit; }
- /* Check if long name is needed */ - ndent = set_name(itr, basename, shortname); - if (ndent < 0) { - ret = ndent; - goto exit; - } - ret = fat_find_empty_dentries(itr, ndent); - if (ret) - goto exit; - if (ndent > 1) { - /* Set long name entries */ - ret = fill_dir_slot(itr, basename, shortname); - if (ret) - goto exit; - } - - /* Set short name entry */ - fill_dentry(itr->fsdata, itr->dent, shortname, 0, size, - ATTR_ARCH); + ret = fat_create_dir_entry(itr, basename, size, ATTR_ARCH);
retdent = itr->dent; } @@ -1658,38 +1675,8 @@ int fat_mkdir(const char *dirname) ret = -EEXIST; goto exit; } else { - char shortname[SHORT_NAME_SIZE]; - int ndent; - - if (itr->is_root) { - /* root dir cannot have "." or ".." */ - if (!strcmp(l_dirname, ".") || - !strcmp(l_dirname, "..")) { - ret = -EINVAL; - goto exit; - } - } - - /* Check if long name is needed */ - ndent = set_name(itr, basename, shortname); - if (ndent < 0) { - ret = ndent; - goto exit; - } - ret = fat_find_empty_dentries(itr, ndent); - if (ret) - goto exit; - if (ndent > 1) { - /* Set long name entries */ - ret = fill_dir_slot(itr, basename, shortname); - if (ret) - goto exit; - } - - /* Set attribute as archive for regular file */ - fill_dentry(itr->fsdata, itr->dent, shortname, 0, 0, - ATTR_DIR | ATTR_ARCH); - + ret = fat_create_dir_entry(itr, basename, 0, + ATTR_DIR | ATTR_ARCH); retdent = itr->dent; }
-- 2.29.2
The FAT32 File System Specification [1] requires leading and trailing spaces as well as trailing periods of long names to be ignored.
This renders a test for '.' and '..' as file name superfluous.
But we must check that the resulting name has at least one character.
[1] Microsoft Extensible Firmware Initiative FAT32 File System Specification Version 1.03, December 6, 2000 Microsoft Corporation https://www.win.tue.nl/~aeb/linux/fs/fat/fatgen103.pdf
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- fs/fat/fat_write.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 0f4786ef0f..1b0a0eda09 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1237,12 +1237,32 @@ again: }
*last_slash_cont = '\0'; - *basename = last_slash_cont + 1; + filename = last_slash_cont + 1; } else { *dirname = "/"; /* root by default */ - *basename = filename; }
+ /* + * The FAT32 File System Specification v1.03 requires leading and + * trailing spaces as well as trailing periods to be ignored. + */ + for (; *filename == ' '; ++filename) + ; + /* Remove trailing periods and spaces */ + for (p = filename + strlen(filename) - 1; p >= filename; --p) { + switch (*p) { + case ' ': + case '.': + *p = 0; + break; + default: + goto done; + } + } + +done: + *basename = filename; + return 0; }
@@ -1260,10 +1280,7 @@ static int normalize_longname(char *l_filename, const char *filename) { const char *p, illegal[] = "<>:"/\|?*";
- if (!strcmp(filename, ".") || !strcmp(filename, "..")) - return -1; - - if (strlen(filename) >= VFAT_MAXLEN_BYTES) + if (!*filename || strlen(filename) >= VFAT_MAXLEN_BYTES) return -1;
for (p = filename; *p; ++p) { -- 2.29.2
On Sun, Jan 31, 2021 at 12:09:53AM +0100, Heinrich Schuchardt wrote:
The FAT32 File System Specification [1] requires leading and trailing spaces as well as trailing periods of long names to be ignored.
This renders a test for '.' and '..' as file name superfluous.
But we must check that the resulting name has at least one character.
[1] Microsoft Extensible Firmware Initiative FAT32 File System Specification Version 1.03, December 6, 2000 Microsoft Corporation https://www.win.tue.nl/~aeb/linux/fs/fat/fatgen103.pdf
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
fs/fat/fat_write.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 0f4786ef0f..1b0a0eda09 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1237,12 +1237,32 @@ again: }
*last_slash_cont = '\0';
*basename = last_slash_cont + 1;
filename = last_slash_cont + 1;
*basename = filename;
- /*
* The FAT32 File System Specification v1.03 requires leading and* trailing spaces as well as trailing periods to be ignored.*/- for (; *filename == ' '; ++filename)
;- /* Remove trailing periods and spaces */
- for (p = filename + strlen(filename) - 1; p >= filename; --p) {
switch (*p) {case ' ':case '.':*p = 0;break;default:goto done;}- }
Given the semantics of the functions, split_filename() and normalize_longname(), the code you added above should be moved to normalize_longname().
+done:
- *basename = filename;
- return 0;
}
@@ -1260,10 +1280,7 @@ static int normalize_longname(char *l_filename, const char *filename) { const char *p, illegal[] = "<>:"/\|?*";
- if (!strcmp(filename, ".") || !strcmp(filename, ".."))
return -1;
It would be better for the check above to remain here as "." and ".." are legal directory names. (I even think it would be best to move it to the caller, file_fat_write_at() or fat_mkdir().)
I think that the suggested sequence would be more intuitive for better understanding of what Windows requirements say.
-Takahiro Akashi
- if (strlen(filename) >= VFAT_MAXLEN_BYTES)
if (!*filename || strlen(filename) >= VFAT_MAXLEN_BYTES) return -1;
for (p = filename; *p; ++p) {
-- 2.29.2
On 01.02.21 09:18, AKASHI Takahiro wrote:
On Sun, Jan 31, 2021 at 12:09:53AM +0100, Heinrich Schuchardt wrote:
The FAT32 File System Specification [1] requires leading and trailing spaces as well as trailing periods of long names to be ignored.
This renders a test for '.' and '..' as file name superfluous.
But we must check that the resulting name has at least one character.
[1] Microsoft Extensible Firmware Initiative FAT32 File System Specification Version 1.03, December 6, 2000 Microsoft Corporation https://www.win.tue.nl/~aeb/linux/fs/fat/fatgen103.pdf
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
fs/fat/fat_write.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 0f4786ef0f..1b0a0eda09 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1237,12 +1237,32 @@ again: }
*last_slash_cont = '\0';
*basename = last_slash_cont + 1;
filename = last_slash_cont + 1;
*basename = filename;
- /*
* The FAT32 File System Specification v1.03 requires leading and* trailing spaces as well as trailing periods to be ignored.*/- for (; *filename == ' '; ++filename)
;- /* Remove trailing periods and spaces */
- for (p = filename + strlen(filename) - 1; p >= filename; --p) {
switch (*p) {case ' ':case '.':*p = 0;break;default:goto done;}- }
Given the semantics of the functions, split_filename() and normalize_longname(), the code you added above should be moved to normalize_longname().
normalize_longname(l_filename, filename) converts the argument filename to a lowercase string l_filename. The parameter filename remains unchanged. But it is the value of filename that is used to create the new directory entry in file_fat_write_at() and fat_mkdir().
So moving the change to normalize_longname() will not lead to the intended behavior.
Removing leading and trailing blanks fits well into the task of split_filename to identify the actual file name.
Best regards
Heinrich
+done:
- *basename = filename;
- return 0;
}
@@ -1260,10 +1280,7 @@ static int normalize_longname(char *l_filename, const char *filename) { const char *p, illegal[] = "<>:"/\|?*";
- if (!strcmp(filename, ".") || !strcmp(filename, ".."))
return -1;It would be better for the check above to remain here as "." and ".." are legal directory names. (I even think it would be best to move it to the caller, file_fat_write_at() or fat_mkdir().)
I think that the suggested sequence would be more intuitive for better understanding of what Windows requirements say.
-Takahiro Akashi
- if (strlen(filename) >= VFAT_MAXLEN_BYTES)
if (!*filename || strlen(filename) >= VFAT_MAXLEN_BYTES) return -1;
for (p = filename; *p; ++p) {
-- 2.29.2
On Mon, Feb 01, 2021 at 01:34:59PM +0100, Heinrich Schuchardt wrote:
On 01.02.21 09:18, AKASHI Takahiro wrote:
On Sun, Jan 31, 2021 at 12:09:53AM +0100, Heinrich Schuchardt wrote:
The FAT32 File System Specification [1] requires leading and trailing spaces as well as trailing periods of long names to be ignored.
This renders a test for '.' and '..' as file name superfluous.
But we must check that the resulting name has at least one character.
[1] Microsoft Extensible Firmware Initiative FAT32 File System Specification Version 1.03, December 6, 2000 Microsoft Corporation https://www.win.tue.nl/~aeb/linux/fs/fat/fatgen103.pdf
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
fs/fat/fat_write.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 0f4786ef0f..1b0a0eda09 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1237,12 +1237,32 @@ again: }
*last_slash_cont = '\0';
*basename = last_slash_cont + 1;
filename = last_slash_cont + 1;
*basename = filename;
- /*
* The FAT32 File System Specification v1.03 requires leading and* trailing spaces as well as trailing periods to be ignored.*/- for (; *filename == ' '; ++filename)
;- /* Remove trailing periods and spaces */
- for (p = filename + strlen(filename) - 1; p >= filename; --p) {
switch (*p) {case ' ':case '.':*p = 0;break;default:goto done;}- }
Given the semantics of the functions, split_filename() and normalize_longname(), the code you added above should be moved to normalize_longname().
normalize_longname(l_filename, filename) converts the argument filename to a lowercase string l_filename. The parameter filename remains unchanged. But it is the value of filename that is used to create the new directory entry in file_fat_write_at() and fat_mkdir().
That is why I also suggested, "I even think it would be best to move it to the caller, file_fat_write_at() or fat_mkdir()."
So moving the change to normalize_longname() will not lead to the intended behavior.
Removing leading and trailing blanks fits well into the task of split_filename to identify the actual file name.
Again, "." and ".." are legal directory names. To reject a request of creating such names is a caller's job, not split_filename()'s as its name suggests.
-Takahiro Akashi
Best regards
Heinrich
+done:
- *basename = filename;
- return 0;
}
@@ -1260,10 +1280,7 @@ static int normalize_longname(char *l_filename, const char *filename) { const char *p, illegal[] = "<>:"/\|?*";
- if (!strcmp(filename, ".") || !strcmp(filename, ".."))
return -1;It would be better for the check above to remain here as "." and ".." are legal directory names. (I even think it would be best to move it to the caller, file_fat_write_at() or fat_mkdir().)
I think that the suggested sequence would be more intuitive for better understanding of what Windows requirements say.
-Takahiro Akashi
- if (strlen(filename) >= VFAT_MAXLEN_BYTES)
if (!*filename || strlen(filename) >= VFAT_MAXLEN_BYTES) return -1;
for (p = filename; *p; ++p) {
-- 2.29.2
Am 2. Februar 2021 00:54:58 MEZ schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Mon, Feb 01, 2021 at 01:34:59PM +0100, Heinrich Schuchardt wrote:
On 01.02.21 09:18, AKASHI Takahiro wrote:
On Sun, Jan 31, 2021 at 12:09:53AM +0100, Heinrich Schuchardt
wrote:
The FAT32 File System Specification [1] requires leading and
trailing
spaces as well as trailing periods of long names to be ignored.
This renders a test for '.' and '..' as file name superfluous.
But we must check that the resulting name has at least one
character.
[1] Microsoft Extensible Firmware Initiative FAT32 File System Specification Version 1.03, December 6, 2000 Microsoft Corporation https://www.win.tue.nl/~aeb/linux/fs/fat/fatgen103.pdf
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
fs/fat/fat_write.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 0f4786ef0f..1b0a0eda09 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1237,12 +1237,32 @@ again: }
*last_slash_cont = '\0';
*basename = last_slash_cont + 1;
filename = last_slash_cont + 1;
*basename = filename;
- /*
* The FAT32 File System Specification v1.03 requires leadingand
* trailing spaces as well as trailing periods to be ignored.*/- for (; *filename == ' '; ++filename)
;- /* Remove trailing periods and spaces */
- for (p = filename + strlen(filename) - 1; p >= filename; --p) {
switch (*p) {case ' ':case '.':*p = 0;break;default:goto done;}- }
Given the semantics of the functions, split_filename() and
normalize_longname(),
the code you added above should be moved to normalize_longname().
normalize_longname(l_filename, filename) converts the argument
filename
to a lowercase string l_filename. The parameter filename remains unchanged. But it is the value of filename that is used to create the new directory entry in file_fat_write_at() and fat_mkdir().
That is why I also suggested, "I even think it would be best to move it to the caller, file_fat_write_at() or fat_mkdir()."
So moving the change to normalize_longname() will not lead to the intended behavior.
Removing leading and trailing blanks fits well into the task of split_filename to identify the actual file name.
Again, "." and ".." are legal directory names. To reject a request of creating such names is a caller's job, not split_filename()'s as its name suggests.
These filenames are illegal for all callers.
We should not duplicate code in each caller.
Best regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
+done:
- *basename = filename;
- return 0;
}
@@ -1260,10 +1280,7 @@ static int normalize_longname(char
*l_filename, const char *filename)
{ const char *p, illegal[] = "<>:"/\|?*";
- if (!strcmp(filename, ".") || !strcmp(filename, ".."))
return -1;It would be better for the check above to remain here as "." and
".." are
legal directory names. (I even think it would be best to move it to the caller, file_fat_write_at() or fat_mkdir().)
I think that the suggested sequence would be more intuitive for better understanding of what Windows requirements say.
-Takahiro Akashi
- if (strlen(filename) >= VFAT_MAXLEN_BYTES)
if (!*filename || strlen(filename) >= VFAT_MAXLEN_BYTES) return -1;
for (p = filename; *p; ++p) {
-- 2.29.2
On Tue, Feb 02, 2021 at 07:05:53AM +0100, Heinrich Schuchardt wrote:
Am 2. Februar 2021 00:54:58 MEZ schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Mon, Feb 01, 2021 at 01:34:59PM +0100, Heinrich Schuchardt wrote:
On 01.02.21 09:18, AKASHI Takahiro wrote:
On Sun, Jan 31, 2021 at 12:09:53AM +0100, Heinrich Schuchardt
wrote:
The FAT32 File System Specification [1] requires leading and
trailing
spaces as well as trailing periods of long names to be ignored.
This renders a test for '.' and '..' as file name superfluous.
But we must check that the resulting name has at least one
character.
[1] Microsoft Extensible Firmware Initiative FAT32 File System Specification Version 1.03, December 6, 2000 Microsoft Corporation https://www.win.tue.nl/~aeb/linux/fs/fat/fatgen103.pdf
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
fs/fat/fat_write.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 0f4786ef0f..1b0a0eda09 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1237,12 +1237,32 @@ again: }
*last_slash_cont = '\0';
*basename = last_slash_cont + 1;
filename = last_slash_cont + 1;
*basename = filename;
- /*
* The FAT32 File System Specification v1.03 requires leadingand
* trailing spaces as well as trailing periods to be ignored.*/- for (; *filename == ' '; ++filename)
;- /* Remove trailing periods and spaces */
- for (p = filename + strlen(filename) - 1; p >= filename; --p) {
switch (*p) {case ' ':case '.':*p = 0;break;default:goto done;}- }
Given the semantics of the functions, split_filename() and
normalize_longname(),
the code you added above should be moved to normalize_longname().
normalize_longname(l_filename, filename) converts the argument
filename
to a lowercase string l_filename. The parameter filename remains unchanged. But it is the value of filename that is used to create the new directory entry in file_fat_write_at() and fat_mkdir().
That is why I also suggested, "I even think it would be best to move it to the caller, file_fat_write_at() or fat_mkdir()."
So moving the change to normalize_longname() will not lead to the intended behavior.
Removing leading and trailing blanks fits well into the task of split_filename to identify the actual file name.
Again, "." and ".." are legal directory names. To reject a request of creating such names is a caller's job, not split_filename()'s as its name suggests.
These filenames are illegal for all callers.
The purpose of split_filename() is to separate a file name from its directory path. As I said, excluding "." or ".." is out of this function's scope, but the caller's semantics.
We should not duplicate code in each caller.
I don't think so. handling "." or ".." entry and removing leading/trailing spaces are totally different.
-Takahiro Akashi
Best regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
+done:
- *basename = filename;
- return 0;
}
@@ -1260,10 +1280,7 @@ static int normalize_longname(char
*l_filename, const char *filename)
{ const char *p, illegal[] = "<>:"/\|?*";
- if (!strcmp(filename, ".") || !strcmp(filename, ".."))
return -1;It would be better for the check above to remain here as "." and
".." are
legal directory names. (I even think it would be best to move it to the caller, file_fat_write_at() or fat_mkdir().)
I think that the suggested sequence would be more intuitive for better understanding of what Windows requirements say.
-Takahiro Akashi
- if (strlen(filename) >= VFAT_MAXLEN_BYTES)
if (!*filename || strlen(filename) >= VFAT_MAXLEN_BYTES) return -1;
for (p = filename; *p; ++p) {
-- 2.29.2
Am 2. Februar 2021 07:39:34 MEZ schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Tue, Feb 02, 2021 at 07:05:53AM +0100, Heinrich Schuchardt wrote:
Am 2. Februar 2021 00:54:58 MEZ schrieb AKASHI Takahiro
On Mon, Feb 01, 2021 at 01:34:59PM +0100, Heinrich Schuchardt wrote:
On 01.02.21 09:18, AKASHI Takahiro wrote:
On Sun, Jan 31, 2021 at 12:09:53AM +0100, Heinrich Schuchardt
wrote:
The FAT32 File System Specification [1] requires leading and
trailing
spaces as well as trailing periods of long names to be ignored.
This renders a test for '.' and '..' as file name superfluous.
But we must check that the resulting name has at least one
character.
[1] Microsoft Extensible Firmware Initiative FAT32 File System Specification Version 1.03, December 6, 2000 Microsoft Corporation https://www.win.tue.nl/~aeb/linux/fs/fat/fatgen103.pdf
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
fs/fat/fat_write.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c index 0f4786ef0f..1b0a0eda09 100644 --- a/fs/fat/fat_write.c +++ b/fs/fat/fat_write.c @@ -1237,12 +1237,32 @@ again: }
*last_slash_cont = '\0';
*basename = last_slash_cont + 1;
filename = last_slash_cont + 1;
*basename = filename;
- /*
* The FAT32 File System Specification v1.03 requires leadingand
* trailing spaces as well as trailing periods to be ignored.*/- for (; *filename == ' '; ++filename)
;- /* Remove trailing periods and spaces */
- for (p = filename + strlen(filename) - 1; p >= filename; --p)
{
switch (*p) {case ' ':case '.':*p = 0;break;default:goto done;}- }
Given the semantics of the functions, split_filename() and
normalize_longname(),
the code you added above should be moved to
normalize_longname().
normalize_longname(l_filename, filename) converts the argument
filename
to a lowercase string l_filename. The parameter filename remains unchanged. But it is the value of filename that is used to create
the
new directory entry in file_fat_write_at() and fat_mkdir().
That is why I also suggested, "I even think it would be best to move
it
to the caller, file_fat_write_at() or fat_mkdir()."
So moving the change to normalize_longname() will not lead to the intended behavior.
Removing leading and trailing blanks fits well into the task of split_filename to identify the actual file name.
Again, "." and ".." are legal directory names. To reject a request of creating such names is a caller's job, not split_filename()'s as its name suggests.
These filenames are illegal for all callers.
The purpose of split_filename() is to separate a file name from its directory path. As I said, excluding "." or ".." is out of this function's scope, but the caller's semantics.
We should not duplicate code in each caller.
I don't think so. handling "." or ".." entry and removing leading/trailing spaces are totally different.
Trailing periods have to be removed here too.
Putting special handling for '.' and '..' here *and* in each caller will increase code size without any gain in functionality.
Best regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
+done:
- *basename = filename;
- return 0;
}
@@ -1260,10 +1280,7 @@ static int normalize_longname(char
*l_filename, const char *filename)
{ const char *p, illegal[] = "<>:"/\|?*";
- if (!strcmp(filename, ".") || !strcmp(filename, ".."))
return -1;It would be better for the check above to remain here as "." and
".." are
legal directory names. (I even think it would be best to move it
to
the caller, file_fat_write_at() or fat_mkdir().)
I think that the suggested sequence would be more intuitive for better understanding of what Windows requirements say.
-Takahiro Akashi
- if (strlen(filename) >= VFAT_MAXLEN_BYTES)
if (!*filename || strlen(filename) >= VFAT_MAXLEN_BYTES) return -1;
for (p = filename; *p; ++p) {
-- 2.29.2
participants (2)
-
AKASHI Takahiro -
Heinrich Schuchardt