diff mbox series

[PATCHv2,2/3] ext4: Cleanup function defs from ext4.h into crypto.c

Message ID 4120e61a1f68c225eb7a27a7a529fd0847270010.1652539361.git.ritesh.list@gmail.com
State Superseded
Headers show
Series ext4/crypto: Move out crypto related ops to crypto.c | expand

Commit Message

Ritesh Harjani (IBM) May 14, 2022, 5:22 p.m. UTC 
Some of these functions when CONFIG_FS_ENCRYPTION is enabled are not
really inline (let compiler be the best judge of it).
Remove inline and move them into crypto.c where they should be present.

Signed-off-by: Ritesh Harjani <ritesh.list@gmail.com>
---
 fs/ext4/crypto.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/ext4.h   | 70 +++++-------------------------------------------
 2 files changed, 71 insertions(+), 64 deletions(-)

Comments

Eric Biggers May 15, 2022, 3:40 a.m. UTC | #1 
On Sat, May 14, 2022 at 10:52:47PM +0530, Ritesh Harjani wrote:
> diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
[...]
> +int ext4_fname_setup_filename(struct inode *dir, const struct qstr *iname,
> +			      int lookup, struct ext4_filename *fname)
> +{
[...]
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
[...]
> +int ext4_fname_setup_filename(struct inode *dir,
> +			      const struct qstr *iname, int lookup,
> +			      struct ext4_filename *fname);

Very minor nit: the above declaration can be formatted on 2 lines, the same as
the definition.

Otherwise this patch looks fine.  I think that filename handling in ext4 in
general is still greatly in need of some cleanups, considering that ext4 now has
to support all combinations of encryption and casefolding.  f2fs does it in a
somewhat cleaner way, IMO.  And it's possible that would lead us down a slightly
different path.  But this is an improvement for now.

Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric
Ritesh Harjani (IBM) May 15, 2022, 4:49 a.m. UTC | #2 
On 22/05/14 08:40PM, Eric Biggers wrote:
> On Sat, May 14, 2022 at 10:52:47PM +0530, Ritesh Harjani wrote:
> > diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
> [...]
> > +int ext4_fname_setup_filename(struct inode *dir, const struct qstr *iname,
> > +			      int lookup, struct ext4_filename *fname)
> > +{
> [...]
> > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> [...]
> > +int ext4_fname_setup_filename(struct inode *dir,
> > +			      const struct qstr *iname, int lookup,
> > +			      struct ext4_filename *fname);
>
> Very minor nit: the above declaration can be formatted on 2 lines, the same as
> the definition.

Thanks for spotting. I will make the change.

>
> Otherwise this patch looks fine.  I think that filename handling in ext4 in
> general is still greatly in need of some cleanups, considering that ext4 now has
> to support all combinations of encryption and casefolding.  f2fs does it in a
> somewhat cleaner way, IMO.  And it's possible that would lead us down a slightly
> different path.  But this is an improvement for now.

Some examples please which you posibly have in mind which should help in
cleanup ext4's filename handling code. I can get back to it after completing
some other items in my todo list.

>
> Reviewed-by: Eric Biggers <ebiggers@google.com>

Thanks!!

-ritesh
diff mbox series

Patch

diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
index e5413c0970ee..f8333927f0f6 100644
--- a/fs/ext4/crypto.c
+++ b/fs/ext4/crypto.c
@@ -6,6 +6,71 @@ 
 #include "xattr.h"
 #include "ext4_jbd2.h"
 
+static void ext4_fname_from_fscrypt_name(struct ext4_filename *dst,
+					 const struct fscrypt_name *src)
+{
+	memset(dst, 0, sizeof(*dst));
+
+	dst->usr_fname = src->usr_fname;
+	dst->disk_name = src->disk_name;
+	dst->hinfo.hash = src->hash;
+	dst->hinfo.minor_hash = src->minor_hash;
+	dst->crypto_buf = src->crypto_buf;
+}
+
+int ext4_fname_setup_filename(struct inode *dir, const struct qstr *iname,
+			      int lookup, struct ext4_filename *fname)
+{
+	struct fscrypt_name name;
+	int err;
+
+	err = fscrypt_setup_filename(dir, iname, lookup, &name);
+	if (err)
+		return err;
+
+	ext4_fname_from_fscrypt_name(fname, &name);
+
+#if IS_ENABLED(CONFIG_UNICODE)
+	err = ext4_fname_setup_ci_filename(dir, iname, fname);
+#endif
+	return err;
+}
+
+int ext4_fname_prepare_lookup(struct inode *dir, struct dentry *dentry,
+			      struct ext4_filename *fname)
+{
+	struct fscrypt_name name;
+	int err;
+
+	err = fscrypt_prepare_lookup(dir, dentry, &name);
+	if (err)
+		return err;
+
+	ext4_fname_from_fscrypt_name(fname, &name);
+
+#if IS_ENABLED(CONFIG_UNICODE)
+	err = ext4_fname_setup_ci_filename(dir, &dentry->d_name, fname);
+#endif
+	return err;
+}
+
+void ext4_fname_free_filename(struct ext4_filename *fname)
+{
+	struct fscrypt_name name;
+
+	name.crypto_buf = fname->crypto_buf;
+	fscrypt_free_filename(&name);
+
+	fname->crypto_buf.name = NULL;
+	fname->usr_fname = NULL;
+	fname->disk_name.name = NULL;
+
+#if IS_ENABLED(CONFIG_UNICODE)
+	kfree(fname->cf_name.name);
+	fname->cf_name.name = NULL;
+#endif
+}
+
 static int ext4_get_context(struct inode *inode, void *ctx, size_t len)
 {
 	return ext4_xattr_get(inode, EXT4_XATTR_INDEX_ENCRYPTION,
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 9100f0ba4a52..11bb0d2ee2eb 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2735,73 +2735,15 @@  extern int ext4_fname_setup_ci_filename(struct inode *dir,
 extern const struct fscrypt_operations ext4_cryptops;
 
 #ifdef CONFIG_FS_ENCRYPTION
-static inline void ext4_fname_from_fscrypt_name(struct ext4_filename *dst,
-						const struct fscrypt_name *src)
-{
-	memset(dst, 0, sizeof(*dst));
-
-	dst->usr_fname = src->usr_fname;
-	dst->disk_name = src->disk_name;
-	dst->hinfo.hash = src->hash;
-	dst->hinfo.minor_hash = src->minor_hash;
-	dst->crypto_buf = src->crypto_buf;
-}
-
-static inline int ext4_fname_setup_filename(struct inode *dir,
-					    const struct qstr *iname,
-					    int lookup,
-					    struct ext4_filename *fname)
-{
-	struct fscrypt_name name;
-	int err;
+int ext4_fname_setup_filename(struct inode *dir,
+			      const struct qstr *iname, int lookup,
+			      struct ext4_filename *fname);
 
-	err = fscrypt_setup_filename(dir, iname, lookup, &name);
-	if (err)
-		return err;
+int ext4_fname_prepare_lookup(struct inode *dir, struct dentry *dentry,
+			      struct ext4_filename *fname);
 
-	ext4_fname_from_fscrypt_name(fname, &name);
+void ext4_fname_free_filename(struct ext4_filename *fname);
 
-#if IS_ENABLED(CONFIG_UNICODE)
-	err = ext4_fname_setup_ci_filename(dir, iname, fname);
-#endif
-	return err;
-}
-
-static inline int ext4_fname_prepare_lookup(struct inode *dir,
-					    struct dentry *dentry,
-					    struct ext4_filename *fname)
-{
-	struct fscrypt_name name;
-	int err;
-
-	err = fscrypt_prepare_lookup(dir, dentry, &name);
-	if (err)
-		return err;
-
-	ext4_fname_from_fscrypt_name(fname, &name);
-
-#if IS_ENABLED(CONFIG_UNICODE)
-	err = ext4_fname_setup_ci_filename(dir, &dentry->d_name, fname);
-#endif
-	return err;
-}
-
-static inline void ext4_fname_free_filename(struct ext4_filename *fname)
-{
-	struct fscrypt_name name;
-
-	name.crypto_buf = fname->crypto_buf;
-	fscrypt_free_filename(&name);
-
-	fname->crypto_buf.name = NULL;
-	fname->usr_fname = NULL;
-	fname->disk_name.name = NULL;
-
-#if IS_ENABLED(CONFIG_UNICODE)
-	kfree(fname->cf_name.name);
-	fname->cf_name.name = NULL;
-#endif
-}
 #else /* !CONFIG_FS_ENCRYPTION */
 static inline int ext4_fname_setup_filename(struct inode *dir,
 					    const struct qstr *iname,