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