Re: [PATCH] fs/ntfs3: Remove bothcase variable

From: Kari Argillander
Date: Thu Oct 07 2021 - 16:05:30 EST

Next message: Chuck Lever III: "[GIT PULL] more nfsd fixes for 5.15"
Previous message: Bjorn Andersson: "Re: [PATCH] drm/msm/dp: Shorten SETUP timeout"
In reply to: Konstantin Komarov: "[PATCH] fs/ntfs3: Remove bothcase variable"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxx>
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxx> for idea and implementation 'bothcase'
> + * Thanks Kari Argillander <kari.argillander@xxxxxxxxx> 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
>

Next message: Chuck Lever III: "[GIT PULL] more nfsd fixes for 5.15"
Previous message: Bjorn Andersson: "Re: [PATCH] drm/msm/dp: Shorten SETUP timeout"
In reply to: Konstantin Komarov: "[PATCH] fs/ntfs3: Remove bothcase variable"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]