[PATCH 0/4] fs: fat: code clean up

[PATCH 0/4] fs: fat: code clean up

[Heinrich Schuchardt]

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

[PATCH 1/4] fs: fat: usage basename in file_fat_write_at, fat_mkdir

[Heinrich Schuchardt]

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

[PATCH 2/4] fs: fat: must not write directory '.' and '..'

[Heinrich Schuchardt]

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

[PATCH 3/4] fs: fat: carve out fat_create_dir_entry()

[Heinrich Schuchardt]

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

[PATCH 4/4] fs: fat: remove trailing periods from long name

[Heinrich Schuchardt]

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

[PATCH 4/4] fs: fat: remove trailing periods from long name

[AKASHI Takahiro]

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

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
> 

[PATCH 4/4] fs: fat: remove trailing periods from long name

[Heinrich Schuchardt]

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

[PATCH 4/4] fs: fat: remove trailing periods from long name

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

[PATCH 4/4] fs: fat: remove trailing periods from long name

[Heinrich Schuchardt]

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

[PATCH 4/4] fs: fat: remove trailing periods from long name

[AKASHI Takahiro]

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

[PATCH 4/4] fs: fat: remove trailing periods from long name

[Heinrich Schuchardt]

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