[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 at 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
> >>
>