Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 28 Mar 2022 19:58:14 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Gabriel Krisman Bertazi <krisman@collabora.com>
Cc:     tytso@mit.edu, jaegeuk@kernel.org, linux-ext4@vger.kernel.org,
        kernel@collabora.com
Subject: Re: [PATCH 1/5] ext4: Match the f2fs ci_compare implementation
Message-ID: <YkJ1xmB1wXQ1WCJ7@sol.localdomain>
References: <20220322030004.148560-1-krisman@collabora.com>
 <20220322030004.148560-2-krisman@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220322030004.148560-2-krisman@collabora.com>
Precedence: bulk

On Mon, Mar 21, 2022 at 11:00:00PM -0400, Gabriel Krisman Bertazi wrote:
> ext4_ci_compare originally follows utf8_*_strcmp, which means return
> zero on match.  This means that every usage of that in ext4 negates
> the return.
> 
> Turn it into a predicate function, let it follow the kernel convention
> and return true on match, which means it's now the same as its f2fs
> counterpart and can be extracted into generic code.
> 
> This change also makes it more obvious that we are ignoring error
> handling in ext4_match, which can occur since casefolding support (bad
> utf8 name due to disk corruption on strict mode causes -EINVAL) and
> casefold+encryption (-ENOMEM).  For now, keep the behavior.  It is
> handled by the following patches.
> 
> While we are there, change the comment to the kernel-doc style.
> 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
> ---
>  fs/ext4/namei.c | 62 +++++++++++++++++++++++++++++++++----------------
>  1 file changed, 42 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 8cf0a924a49b..24ea3bb446d0 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1318,13 +1318,20 @@ static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block)
>  }
>  
>  #if IS_ENABLED(CONFIG_UNICODE)
> -/*
> +/**
> + * ext4_ci_compare() - Match (case-insensitive) a name with a dirent.
> + * @parent: Inode of the parent of the dentry.
> + * @name: name under lookup.
> + * @de_name: Dirent name.
> + * @de_name_len: dirent name length.
> + * @quick: whether @name is already casefolded.
> + *
>   * Test whether a case-insensitive directory entry matches the filename
> - * being searched for.  If quick is set, assume the name being looked up
> - * is already in the casefolded form.
> + * being searched.  If quick is set, the @name being looked up is
> + * already in the casefolded form.
>   *
> - * Returns: 0 if the directory entry matches, more than 0 if it
> - * doesn't match or less than zero on error.
> + * Return: > 0 if the directory entry matches, 0 if it doesn't match, or
> + * < 0 on error.
>   */
>  static int ext4_ci_compare(const struct inode *parent, const struct qstr *name,
>  			   u8 *de_name, size_t de_name_len, bool quick)

Shouldn't this be renamed to ext4_match_ci() as well?  The f2fs equivalent is
called f2fs_match_ci_name(), and this is called from ext4_match().
ext4_match_ci() would better fit the "return 1 on match" behavior, I think.

- Eric