[PATCH 1/2] erofs: using macro instead of definition of log functions

[PATCH 1/2] erofs: using macro instead of definition of log functions

[Gou Hao]

No functional change intended.

Signed-off-by: Gou Hao <gouhao@uniontech.com>
---
 fs/erofs/super.c | 51 ++++++++++++++++++------------------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index 666873f745da..b04f888c8123 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -18,39 +18,26 @@
 
 static struct kmem_cache *erofs_inode_cachep __read_mostly;
 
-void _erofs_err(struct super_block *sb, const char *func, const char *fmt, ...)
-{
-	struct va_format vaf;
-	va_list args;
-
-	va_start(args, fmt);
-
-	vaf.fmt = fmt;
-	vaf.va = &args;
-
-	if (sb)
-		pr_err("(device %s): %s: %pV", sb->s_id, func, &vaf);
-	else
-		pr_err("%s: %pV", func, &vaf);
-	va_end(args);
-}
-
-void _erofs_info(struct super_block *sb, const char *func, const char *fmt, ...)
-{
-	struct va_format vaf;
-	va_list args;
-
-	va_start(args, fmt);
-
-	vaf.fmt = fmt;
-	vaf.va = &args;
+#define _erofs_log_def(name) \
+	void _erofs_##name(struct super_block *sb, const char *func, const char *fmt, ...) \
+	{ \
+		struct va_format vaf; \
+		va_list args; \
+		\
+		va_start(args, (fmt)); \
+		\
+		vaf.fmt = (fmt); \
+		vaf.va = &args; \
+		\
+		if ((sb)) \
+			pr_##name("(device %s): %s: %pV", (sb)->s_id, (func), &vaf); \
+		else \
+			pr_##name("%s: %pV", (func), &vaf); \
+		va_end(args); \
+	}
 
-	if (sb)
-		pr_info("(device %s): %pV", sb->s_id, &vaf);
-	else
-		pr_info("%pV", &vaf);
-	va_end(args);
-}
+_erofs_log_def(err);
+_erofs_log_def(info);
 
 static int erofs_superblock_csum_verify(struct super_block *sb, void *sbdata)
 {
-- 
2.20.1

[PATCH 2/2] erofs: simplify declaration of the log functions

[Gou Hao]

remove the macro of the log declarations.

Signed-off-by: Gou Hao <gouhao@uniontech.com>
---
 fs/erofs/internal.h | 13 +++++--------
 fs/erofs/super.c    | 12 ++++++------
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 4efd578d7c62..0c3d6b9f85b5 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -24,14 +24,11 @@
 #undef pr_fmt
 #define pr_fmt(fmt) "erofs: " fmt
 
-__printf(3, 4) void _erofs_err(struct super_block *sb,
-			       const char *function, const char *fmt, ...);
-#define erofs_err(sb, fmt, ...)	\
-	_erofs_err(sb, __func__, fmt "\n", ##__VA_ARGS__)
-__printf(3, 4) void _erofs_info(struct super_block *sb,
-			       const char *function, const char *fmt, ...);
-#define erofs_info(sb, fmt, ...) \
-	_erofs_info(sb, __func__, fmt "\n", ##__VA_ARGS__)
+#define erofs_log_declare(name) \
+	__printf(2, 3) void erofs_##name(struct super_block *sb, const char *fmt, ...)
+erofs_log_declare(err);
+erofs_log_declare(info);
+
 #ifdef CONFIG_EROFS_FS_DEBUG
 #define DBG_BUGON               BUG_ON
 #else
diff --git a/fs/erofs/super.c b/fs/erofs/super.c
index b04f888c8123..587a56e390ff 100644
--- a/fs/erofs/super.c
+++ b/fs/erofs/super.c
@@ -18,8 +18,8 @@
 
 static struct kmem_cache *erofs_inode_cachep __read_mostly;
 
-#define _erofs_log_def(name) \
-	void _erofs_##name(struct super_block *sb, const char *func, const char *fmt, ...) \
+#define erofs_log_def(name) \
+	__printf(2, 3) void erofs_##name(struct super_block *sb, const char *fmt, ...) \
 	{ \
 		struct va_format vaf; \
 		va_list args; \
@@ -30,14 +30,14 @@ static struct kmem_cache *erofs_inode_cachep __read_mostly;
 		vaf.va = &args; \
 		\
 		if ((sb)) \
-			pr_##name("(device %s): %s: %pV", (sb)->s_id, (func), &vaf); \
+			pr_##name("(device %s): %s: %pV\n", (sb)->s_id, __func__, &vaf); \
 		else \
-			pr_##name("%s: %pV", (func), &vaf); \
+			pr_##name("%s: %pV\n", __func__, &vaf); \
 		va_end(args); \
 	}
 
-_erofs_log_def(err);
-_erofs_log_def(info);
+erofs_log_def(err);
+erofs_log_def(info);
 
 static int erofs_superblock_csum_verify(struct super_block *sb, void *sbdata)
 {
-- 
2.20.1

Re: [PATCH 1/2] erofs: using macro instead of definition of log functions

[Gao Xiang]

Hi Hao,

On 2024/10/16 23:24, Gou Hao wrote:
> No functional change intended.
> 
> Signed-off-by: Gou Hao <gouhao@uniontech.com>
> ---
>   fs/erofs/super.c | 51 ++++++++++++++++++------------------------------
>   1 file changed, 19 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
> index 666873f745da..b04f888c8123 100644
> --- a/fs/erofs/super.c
> +++ b/fs/erofs/super.c
> @@ -18,39 +18,26 @@
>   
>   static struct kmem_cache *erofs_inode_cachep __read_mostly;
>   
> -void _erofs_err(struct super_block *sb, const char *func, const char *fmt, ...)
> -{
> -	struct va_format vaf;
> -	va_list args;
> -
> -	va_start(args, fmt);
> -
> -	vaf.fmt = fmt;
> -	vaf.va = &args;
> -
> -	if (sb)
> -		pr_err("(device %s): %s: %pV", sb->s_id, func, &vaf);
> -	else
> -		pr_err("%s: %pV", func, &vaf);
> -	va_end(args);
> -}
> -
> -void _erofs_info(struct super_block *sb, const char *func, const char *fmt, ...)
> -{
> -	struct va_format vaf;
> -	va_list args;
> -
> -	va_start(args, fmt);
> -
> -	vaf.fmt = fmt;
> -	vaf.va = &args;
> +#define _erofs_log_def(name) \
> +	void _erofs_##name(struct super_block *sb, const char *func, const char *fmt, ...) \
> +	{ \
> +		struct va_format vaf; \
> +		va_list args; \
> +		\
> +		va_start(args, (fmt)); \
> +		\
> +		vaf.fmt = (fmt); \
> +		vaf.va = &args; \
> +		\
> +		if ((sb)) \
> +			pr_##name("(device %s): %s: %pV", (sb)->s_id, (func), &vaf); \
> +		else \
> +			pr_##name("%s: %pV", (func), &vaf); \
> +		va_end(args); \
> +	}

Thanks for the patch!

Although code simplicity is quite important for EROFS, but
I'm not sure introducing unnecessary macro definitions (which
can be avoided) is better for code readability.

I wonder if we can put this into another way, like the current
_btrfs_printk() and _f2fs_printk() if we really need to work
on this.

Thanks,
Gao Xiang

Re: [PATCH 2/2] erofs: simplify declaration of the log functions

[Gao Xiang]

On 2024/10/16 23:24, Gou Hao wrote:
> remove the macro of the log declarations.
> 
> Signed-off-by: Gou Hao <gouhao@uniontech.com>
> ---
>   fs/erofs/internal.h | 13 +++++--------
>   fs/erofs/super.c    | 12 ++++++------
>   2 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 4efd578d7c62..0c3d6b9f85b5 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -24,14 +24,11 @@
>   #undef pr_fmt
>   #define pr_fmt(fmt) "erofs: " fmt
>   
> -__printf(3, 4) void _erofs_err(struct super_block *sb,
> -			       const char *function, const char *fmt, ...);
> -#define erofs_err(sb, fmt, ...)	\
> -	_erofs_err(sb, __func__, fmt "\n", ##__VA_ARGS__)
> -__printf(3, 4) void _erofs_info(struct super_block *sb,
> -			       const char *function, const char *fmt, ...);
> -#define erofs_info(sb, fmt, ...) \
> -	_erofs_info(sb, __func__, fmt "\n", ##__VA_ARGS__)
> +#define erofs_log_declare(name) \
> +	__printf(2, 3) void erofs_##name(struct super_block *sb, const char *fmt, ...)
> +erofs_log_declare(err);
> +erofs_log_declare(info);

I guess it will make the code harder to read IMHO..

Thanks,
Gao Xiang