Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 7 Oct 2021 23:05:21 +0300
From:   Kari Argillander <kari.argillander@gmail.com>
To:     Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Cc:     ntfs3@lists.linux.dev, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, mark@harmstone.com
Subject: Re: [PATCH] fs/ntfs3: Remove bothcase variable
Message-ID: <20211007200521.fj7aq5wlppxa7xzw@kari-VirtualBox>
References: <45f2af05-9f5c-bf56-aa91-47d8b0055f5b@paragon-software.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <45f2af05-9f5c-bf56-aa91-47d8b0055f5b@paragon-software.com>
Precedence: bulk

On Thu, Oct 07, 2021 at 06:37:55PM +0300, Konstantin Komarov wrote:
> bothcase == true when upcase != NULL
> bothcase == false when upcase == NULL
> We don't need to have second variable, that is a copy of upcase.

Can I suggest this one. This way we have three possible state. 

/* Note that NTFS_CASE can also have state CASE_BOTH */
int ntfs_cmp_names(const __le16 *s1, size_t l1, const __le16 *s2, size_t l2,
		   const u16 *upcase, enum NTFS_CASE case_cmp)
{
	int diff1 = 0;
	int diff2;
	size_t len = min(l1, l2);

	if (case_cmp == CASE_INSENSITIVE)
		goto case_insentive;

	for (; len; s1++, s2++, len--) {
		diff1 = le16_to_cpu(*s1) - le16_to_cpu(*s2);
		if (diff1) {
			if (case_cmp != CASE_SENSITIVE)
				goto case_insentive;

			return diff1;
		}
	}
	return l1 - l2;

case_insentive:
	for (; len; s1++, s2++, len--) {
		diff2 = upcase_unicode_char(upcase, le16_to_cpu(*s1)) -
			upcase_unicode_char(upcase, le16_to_cpu(*s2));
		if (diff2)
			return diff2;
	}

	diff2 = l1 - l2;
	return diff2 ? diff2 : diff1;
}


It might also be possible to add compile time error for condition

	!upcase && case_cmp != CASE_SENSITIVE

You also wrote [1] this  

On 6.10.2021 17.37 Konstantin Komarov wrote:
>
> There is indeed some dubious code with ntfs_cmp_names_cpu / ntfs_cmp_names.
> We must always compare filenames with bothcase == true.

This I do not understand. If we have upcase and bothcase == false then
it will mean we are comparing case insentive. bothcase was dubious I
admit because with two parameters there could be four state and one of
them did not make any sense. But maybe enum will make it more clear. We
really need 3 different stage.

Your patch cannot actually compare case_insentive at all. Your patch
will compare case_sensitive or case_both. I think we need to also way to
compare case_insentive. I also belive that because bothcase was
everywhere true that it was the real bug. More testing is needed from my
part before I can make sure. I will test this in weekend and also review
other patches.

  Argillander

> 
> Suggested-by: Mark Harmstone <mark@harmstone.com>
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  fs/ntfs3/attrlist.c |  8 +++-----
>  fs/ntfs3/frecord.c  |  3 +--
>  fs/ntfs3/index.c    |  6 ++----
>  fs/ntfs3/inode.c    |  2 +-
>  fs/ntfs3/ntfs_fs.h  |  4 ++--
>  fs/ntfs3/record.c   |  2 +-
>  fs/ntfs3/upcase.c   | 18 ++++++------------
>  7 files changed, 16 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/ntfs3/attrlist.c b/fs/ntfs3/attrlist.c
> index bad6d8a849a2..1a31ef4ed92b 100644
> --- a/fs/ntfs3/attrlist.c
> +++ b/fs/ntfs3/attrlist.c
> @@ -194,8 +194,7 @@ struct ATTR_LIST_ENTRY *al_find_ex(struct ntfs_inode *ni,
>  			 * Compare entry names only for entry with vcn == 0.
>  			 */
>  			diff = ntfs_cmp_names(le_name(le), name_len, name,
> -					      name_len, ni->mi.sbi->upcase,
> -					      true);
> +					      name_len, ni->mi.sbi->upcase);
>  			if (diff < 0)
>  				continue;
>  
> @@ -246,8 +245,7 @@ static struct ATTR_LIST_ENTRY *al_find_le_to_insert(struct ntfs_inode *ni,
>  			 * Compare entry names only for entry with vcn == 0.
>  			 */
>  			diff = ntfs_cmp_names(le_name(le), le->name_len, name,
> -					      name_len, ni->mi.sbi->upcase,
> -					      true);
> +					      name_len, ni->mi.sbi->upcase);
>  			if (diff < 0)
>  				continue;
>  
> @@ -393,7 +391,7 @@ bool al_delete_le(struct ntfs_inode *ni, enum ATTR_TYPE type, CLST vcn,
>  	if (le->name_len != name_len)
>  		return false;
>  	if (name_len && ntfs_cmp_names(le_name(le), name_len, name, name_len,
> -				       ni->mi.sbi->upcase, true))
> +				       ni->mi.sbi->upcase))
>  		return false;
>  	if (le64_to_cpu(le->vcn) != vcn)
>  		return false;
> diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
> index 007602badd90..ecf982aca437 100644
> --- a/fs/ntfs3/frecord.c
> +++ b/fs/ntfs3/frecord.c
> @@ -1589,8 +1589,7 @@ struct ATTR_FILE_NAME *ni_fname_name(struct ntfs_inode *ni,
>  	if (uni->len != fname->name_len)
>  		goto next;
>  
> -	if (ntfs_cmp_names_cpu(uni, (struct le_str *)&fname->name_len, NULL,
> -			       false))
> +	if (ntfs_cmp_names_cpu(uni, (struct le_str *)&fname->name_len, NULL))
>  		goto next;
>  
>  	return fname;
> diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
> index 6f81e3a49abf..a12f6fa0537e 100644
> --- a/fs/ntfs3/index.c
> +++ b/fs/ntfs3/index.c
> @@ -38,7 +38,6 @@ static int cmp_fnames(const void *key1, size_t l1, const void *key2, size_t l2,
>  	const struct ntfs_sb_info *sbi = data;
>  	const struct ATTR_FILE_NAME *f1;
>  	u16 fsize2;
> -	bool both_case;
>  
>  	if (l2 <= offsetof(struct ATTR_FILE_NAME, name))
>  		return -1;
> @@ -47,7 +46,6 @@ static int cmp_fnames(const void *key1, size_t l1, const void *key2, size_t l2,
>  	if (l2 < fsize2)
>  		return -1;
>  
> -	both_case = f2->type != FILE_NAME_DOS /*&& !sbi->options.nocase*/;
>  	if (!l1) {
>  		const struct le_str *s2 = (struct le_str *)&f2->name_len;
>  
> @@ -55,12 +53,12 @@ static int cmp_fnames(const void *key1, size_t l1, const void *key2, size_t l2,
>  		 * If names are equal (case insensitive)
>  		 * try to compare it case sensitive.
>  		 */
> -		return ntfs_cmp_names_cpu(key1, s2, sbi->upcase, both_case);
> +		return ntfs_cmp_names_cpu(key1, s2, sbi->upcase);
>  	}
>  
>  	f1 = key1;
>  	return ntfs_cmp_names(f1->name, f1->name_len, f2->name, f2->name_len,
> -			      sbi->upcase, both_case);
> +			      sbi->upcase);
>  }
>  
>  /*
> diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
> index 7dd162f6a7e2..c7014e5f941c 100644
> --- a/fs/ntfs3/inode.c
> +++ b/fs/ntfs3/inode.c
> @@ -179,7 +179,7 @@ static struct inode *ntfs_read_mft(struct inode *inode,
>  		names += 1;
>  		if (name && name->len == fname->name_len &&
>  		    !ntfs_cmp_names_cpu(name, (struct le_str *)&fname->name_len,
> -					NULL, false))
> +					NULL))
>  			is_match = true;
>  
>  		goto next_attr;
> diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
> index 38b7c1a9dc52..859624d0dccb 100644
> --- a/fs/ntfs3/ntfs_fs.h
> +++ b/fs/ntfs3/ntfs_fs.h
> @@ -830,9 +830,9 @@ int ntfs_trim_fs(struct ntfs_sb_info *sbi, struct fstrim_range *range);
>  
>  /* Globals from upcase.c */
>  int ntfs_cmp_names(const __le16 *s1, size_t l1, const __le16 *s2, size_t l2,
> -		   const u16 *upcase, bool bothcase);
> +		   const u16 *upcase);
>  int ntfs_cmp_names_cpu(const struct cpu_str *uni1, const struct le_str *uni2,
> -		       const u16 *upcase, bool bothcase);
> +		       const u16 *upcase);
>  
>  /* globals from xattr.c */
>  #ifdef CONFIG_NTFS3_FS_POSIX_ACL
> diff --git a/fs/ntfs3/record.c b/fs/ntfs3/record.c
> index 861e35791506..6afd3c20b0d3 100644
> --- a/fs/ntfs3/record.c
> +++ b/fs/ntfs3/record.c
> @@ -23,7 +23,7 @@ static inline int compare_attr(const struct ATTRIB *left, enum ATTR_TYPE type,
>  
>  	/* They have the same type code, so we have to compare the names. */
>  	return ntfs_cmp_names(attr_name(left), left->name_len, name, name_len,
> -			      upcase, true);
> +			      upcase);
>  }
>  
>  /*
> diff --git a/fs/ntfs3/upcase.c b/fs/ntfs3/upcase.c
> index b5e8256fd710..c15ae0993839 100644
> --- a/fs/ntfs3/upcase.c
> +++ b/fs/ntfs3/upcase.c
> @@ -24,29 +24,26 @@ static inline u16 upcase_unicode_char(const u16 *upcase, u16 chr)
>  /*
>   * ntfs_cmp_names
>   *
> - * Thanks Kari Argillander <kari.argillander@gmail.com> for idea and implementation 'bothcase'
> + * Thanks Kari Argillander <kari.argillander@gmail.com> for idea and implementation
>   *
>   * Straight way to compare names:
>   * - Case insensitive
> - * - If name equals and 'bothcases' then
> + * - If name equals and 'upcase' then
>   * - Case sensitive
>   * 'Straight way' code scans input names twice in worst case.
>   * Optimized code scans input names only once.
>   */
>  int ntfs_cmp_names(const __le16 *s1, size_t l1, const __le16 *s2, size_t l2,
> -		   const u16 *upcase, bool bothcase)
> +		   const u16 *upcase)
>  {
>  	int diff1 = 0;
>  	int diff2;
>  	size_t len = min(l1, l2);
>  
> -	if (!bothcase && upcase)
> -		goto case_insentive;
> -
>  	for (; len; s1++, s2++, len--) {
>  		diff1 = le16_to_cpu(*s1) - le16_to_cpu(*s2);
>  		if (diff1) {
> -			if (bothcase && upcase)
> +			if (upcase)
>  				goto case_insentive;
>  
>  			return diff1;
> @@ -67,7 +64,7 @@ int ntfs_cmp_names(const __le16 *s1, size_t l1, const __le16 *s2, size_t l2,
>  }
>  
>  int ntfs_cmp_names_cpu(const struct cpu_str *uni1, const struct le_str *uni2,
> -		       const u16 *upcase, bool bothcase)
> +		       const u16 *upcase)
>  {
>  	const u16 *s1 = uni1->name;
>  	const __le16 *s2 = uni2->name;
> @@ -77,13 +74,10 @@ int ntfs_cmp_names_cpu(const struct cpu_str *uni1, const struct le_str *uni2,
>  	int diff1 = 0;
>  	int diff2;
>  
> -	if (!bothcase && upcase)
> -		goto case_insentive;
> -
>  	for (; len; s1++, s2++, len--) {
>  		diff1 = *s1 - le16_to_cpu(*s2);
>  		if (diff1) {
> -			if (bothcase && upcase)
> +			if (upcase)
>  				goto case_insentive;
>  
>  			return diff1;
> -- 
> 2.33.0
>