[PATCH 0/4] exfat: Fixes for ENAMETOOLONG error handling

[PATCH 0/4] exfat: Fixes for ENAMETOOLONG error handling

[Takashi Iwai]

Hi,

this is a series for fixing the error code of rename syscall as well
as cleanup / suppress the superfluous error messages.

As an LTP test case reported, exfat returns the inconsistent error
code for the case of renaming oversized file names:
  https://bugzilla.suse.com/show_bug.cgi?id=1201725
The first patch fixes this inconsistency.

The second patch is just for correcting the definitions as bit flags,
and the remaining two patches are for suppressing the error message
that can be triggered too easily to debug messages.


thanks,

Takashi

===

Takashi Iwai (4):
  exfat: Return ENAMETOOLONG consistently for oversized paths
  exfat: Define NLS_NAME_* as bit flags explicitly
  exfat: Expand exfat_err() and co directly to pr_*() macro
  exfat: Downgrade ENAMETOOLONG error message to debug messages

 fs/exfat/exfat_fs.h | 21 +++++++++++++--------
 fs/exfat/misc.c     | 17 -----------------
 fs/exfat/namei.c    |  2 +-
 fs/exfat/nls.c      |  2 +-
 4 files changed, 15 insertions(+), 27 deletions(-)

-- 
2.35.3

[PATCH 1/4] exfat: Return ENAMETOOLONG consistently for oversized paths

[Takashi Iwai]

LTP has a test for oversized file path renames and it expects the
return value to be ENAMETOOLONG.  However, exfat returns EINVAL
unexpectedly in some cases, hence LTP test fails.  The further
investigation indicated that the problem happens only when iocharset
isn't set to utf8.

The difference comes from that, in the case of utf8,
exfat_utf8_to_utf16() returns the error -ENAMETOOLONG directly and
it's treated as the final error code.  Meanwhile, on other iocharsets,
exfat_nls_to_ucs2() returns the max path size but it sets
NLS_NAME_OVERLEN to lossy flag instead; the caller side checks only
whether lossy flag is set or not, resulting in always -EINVAL
unconditionally.

This patch aligns the return code for both cases by checking the lossy
flag bit and returning ENAMETOOLONG when NLS_NAME_OVERLEN bit is set.

BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1201725
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 fs/exfat/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c
index c6eaf7e9ea74..bcb6445eb3b3 100644
--- a/fs/exfat/namei.c
+++ b/fs/exfat/namei.c
@@ -462,7 +462,7 @@ static int __exfat_resolve_path(struct inode *inode, const unsigned char *path,
 		return namelen; /* return error value */
 
 	if ((lossy && !lookup) || !namelen)
-		return -EINVAL;
+		return (lossy & NLS_NAME_OVERLEN) ? -ENAMETOOLONG : -EINVAL;
 
 	exfat_chain_set(p_dir, ei->start_clu,
 		EXFAT_B_TO_CLU(i_size_read(inode), sbi), ei->flags);
-- 
2.35.3

[PATCH 2/4] exfat: Define NLS_NAME_* as bit flags explicitly

[Takashi Iwai]

NLS_NAME_* are bit flags although they are currently defined as enum;
it's casually working so far (from 0 to 2), but it's error-prone and
may bring a problem when we want to add more flag.

This patch changes the definitions of NLS_NAME_* explicitly being bit
flags.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 fs/exfat/exfat_fs.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index 4a7a2308eb72..f431327af459 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -27,9 +27,9 @@ enum exfat_error_mode {
  * exfat nls lossy flag
  */
 enum {
-	NLS_NAME_NO_LOSSY,	/* no lossy */
-	NLS_NAME_LOSSY,		/* just detected incorrect filename(s) */
-	NLS_NAME_OVERLEN,	/* the length is over than its limit */
+	NLS_NAME_NO_LOSSY =	0,	/* no lossy */
+	NLS_NAME_LOSSY =	1 << 0,	/* just detected incorrect filename(s) */
+	NLS_NAME_OVERLEN =	1 << 1,	/* the length is over than its limit */
 };
 
 #define EXFAT_HASH_BITS		8
-- 
2.35.3

[PATCH 3/4] exfat: Expand exfat_err() and co directly to pr_*() macro

[Takashi Iwai]

Currently the error and info messages handled by exfat_err() and co
are tossed to exfat_msg() function that does nothing but passes the
strings with printk() invocation.  Not only that this is more overhead
by the indirect calls, but also this makes harder to extend for the
debug print usage; because of the direct printk() call, you cannot
make it for dynamic debug or without debug like the standard helpers
such as pr_debug() or dev_dbg().

For addressing the problem, this patch replaces exfat_msg() function
with a macro to expand to pr_*() directly.  This allows us to create
exfat_debug() macro that is expanded to pr_debug() (which output can
gracefully suppressed via dyndbg).

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 fs/exfat/exfat_fs.h | 15 ++++++++++-----
 fs/exfat/misc.c     | 17 -----------------
 2 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
index f431327af459..a5bc0fc11f79 100644
--- a/fs/exfat/exfat_fs.h
+++ b/fs/exfat/exfat_fs.h
@@ -508,14 +508,19 @@ void __exfat_fs_error(struct super_block *sb, int report, const char *fmt, ...)
 #define exfat_fs_error_ratelimit(sb, fmt, args...) \
 		__exfat_fs_error(sb, __ratelimit(&EXFAT_SB(sb)->ratelimit), \
 		fmt, ## args)
-void exfat_msg(struct super_block *sb, const char *lv, const char *fmt, ...)
-		__printf(3, 4) __cold;
+
+/* expand to pr_xxx() with prefix */
+#define exfat_msg(sb, lv, fmt, ...) \
+	pr_##lv("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
+
 #define exfat_err(sb, fmt, ...)						\
-	exfat_msg(sb, KERN_ERR, fmt, ##__VA_ARGS__)
+	exfat_msg(sb, err, fmt, ##__VA_ARGS__)
 #define exfat_warn(sb, fmt, ...)					\
-	exfat_msg(sb, KERN_WARNING, fmt, ##__VA_ARGS__)
+	exfat_msg(sb, warn, fmt, ##__VA_ARGS__)
 #define exfat_info(sb, fmt, ...)					\
-	exfat_msg(sb, KERN_INFO, fmt, ##__VA_ARGS__)
+	exfat_msg(sb, info, fmt, ##__VA_ARGS__)
+#define exfat_debug(sb, fmt, ...)					\
+	exfat_msg(sb, debug, fmt, ##__VA_ARGS__)
 
 void exfat_get_entry_time(struct exfat_sb_info *sbi, struct timespec64 *ts,
 		u8 tz, __le16 time, __le16 date, u8 time_cs);
diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c
index 9380e0188b55..2e1a1a6b1021 100644
--- a/fs/exfat/misc.c
+++ b/fs/exfat/misc.c
@@ -46,23 +46,6 @@ void __exfat_fs_error(struct super_block *sb, int report, const char *fmt, ...)
 	}
 }
 
-/*
- * exfat_msg() - print preformated EXFAT specific messages.
- * All logs except what uses exfat_fs_error() should be written by exfat_msg()
- */
-void exfat_msg(struct super_block *sb, const char *level, const char *fmt, ...)
-{
-	struct va_format vaf;
-	va_list args;
-
-	va_start(args, fmt);
-	vaf.fmt = fmt;
-	vaf.va = &args;
-	/* level means KERN_ pacility level */
-	printk("%sexFAT-fs (%s): %pV\n", level, sb->s_id, &vaf);
-	va_end(args);
-}
-
 #define SECS_PER_MIN    (60)
 #define TIMEZONE_SEC(x)	((x) * 15 * SECS_PER_MIN)
 
-- 
2.35.3

[PATCH 4/4] exfat: Downgrade ENAMETOOLONG error message to debug messages

[Takashi Iwai]

The ENAMETOOLONG error message is printed at each time when user tries
to operate with a too long name, and this can flood the kernel logs
easily, as every user can trigger this.  Let's downgrade this error
message level to a debug message for suppressing the superfluous
logs.

BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1201725
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 fs/exfat/nls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c
index ef115e673406..617aa1272265 100644
--- a/fs/exfat/nls.c
+++ b/fs/exfat/nls.c
@@ -509,7 +509,7 @@ static int exfat_utf8_to_utf16(struct super_block *sb,
 	}
 
 	if (unilen > MAX_NAME_LENGTH) {
-		exfat_err(sb, "failed to %s (estr:ENAMETOOLONG) nls len : %d, unilen : %d > %d",
+		exfat_debug(sb, "failed to %s (estr:ENAMETOOLONG) nls len : %d, unilen : %d > %d",
 			  __func__, len, unilen, MAX_NAME_LENGTH);
 		return -ENAMETOOLONG;
 	}
-- 
2.35.3

Re: [PATCH 3/4] exfat: Expand exfat_err() and co directly to pr_*() macro

[Joe Perches]

On Fri, 2022-07-22 at 16:29 +0200, Takashi Iwai wrote:
> Currently the error and info messages handled by exfat_err() and co
> are tossed to exfat_msg() function that does nothing but passes the
> strings with printk() invocation.  Not only that this is more overhead
> by the indirect calls, but also this makes harder to extend for the
> debug print usage; because of the direct printk() call, you cannot
> make it for dynamic debug or without debug like the standard helpers
> such as pr_debug() or dev_dbg().
> 
> For addressing the problem, this patch replaces exfat_msg() function
> with a macro to expand to pr_*() directly.  This allows us to create
> exfat_debug() macro that is expanded to pr_debug() (which output can
> gracefully suppressed via dyndbg).
[]
> diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
[]
> @@ -508,14 +508,19 @@ void __exfat_fs_error(struct super_block *sb, int report, const char *fmt, ...)
>  #define exfat_fs_error_ratelimit(sb, fmt, args...) \
>  		__exfat_fs_error(sb, __ratelimit(&EXFAT_SB(sb)->ratelimit), \
>  		fmt, ## args)
> -void exfat_msg(struct super_block *sb, const char *lv, const char *fmt, ...)
> -		__printf(3, 4) __cold;
> +
> +/* expand to pr_xxx() with prefix */
> +#define exfat_msg(sb, lv, fmt, ...) \
> +	pr_##lv("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
> +
>  #define exfat_err(sb, fmt, ...)						\
> -	exfat_msg(sb, KERN_ERR, fmt, ##__VA_ARGS__)
> +	exfat_msg(sb, err, fmt, ##__VA_ARGS__)
>  #define exfat_warn(sb, fmt, ...)					\
> -	exfat_msg(sb, KERN_WARNING, fmt, ##__VA_ARGS__)
> +	exfat_msg(sb, warn, fmt, ##__VA_ARGS__)
>  #define exfat_info(sb, fmt, ...)					\
> -	exfat_msg(sb, KERN_INFO, fmt, ##__VA_ARGS__)
> +	exfat_msg(sb, info, fmt, ##__VA_ARGS__)
> +#define exfat_debug(sb, fmt, ...)					\
> +	exfat_msg(sb, debug, fmt, ##__VA_ARGS__)

I think this would be clearer using pr_<level> directly instead
of an indirecting macro that uses concatenation of <level> that
obscures the actual use of pr_<level>

Either: (and this first option would be my preference)

#define exfat_err(sb, fmt, ...) \
	pr_err("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
#define exfat_warn(sb, fmt, ...) \
	pr_warn("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
etc...

or using an indirecting macro:

#define exfat_printk(pr_level, sb, fmt, ...)	\
	pr_level("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)

#define exfat_err(sb, fmt, ...)
	exfat_printk(pr_err, sb, fmt, ##__VA_ARGS)
#define exfat_warn(sb, fmt, ...)
	exfat_printk(pr_warn, sb, fmt, ##__VA_ARGS)
etc...

and btw, there are multiple uses of exfat_<level> output with a
unnecessary and duplicated '\n' that the macro already adds that
should be removed:

$ git grep -P -n '\bexfat_(err|warn|info).*\\n' fs/exfat/
fs/exfat/fatent.c:334:                  exfat_err(sb, "sbi->clu_srch_ptr is invalid (%u)\n",
fs/exfat/nls.c:674:                     exfat_err(sb, "failed to read sector(0x%llx)\n",
fs/exfat/super.c:467:           exfat_err(sb, "bogus sector size bits : %u\n",
fs/exfat/super.c:476:           exfat_err(sb, "bogus sectors bits per cluster : %u\n",

Re: [PATCH 3/4] exfat: Expand exfat_err() and co directly to pr_*() macro

[Takashi Iwai]

On Sat, 23 Jul 2022 09:42:12 +0200,
Joe Perches wrote:
> 
> On Fri, 2022-07-22 at 16:29 +0200, Takashi Iwai wrote:
> > Currently the error and info messages handled by exfat_err() and co
> > are tossed to exfat_msg() function that does nothing but passes the
> > strings with printk() invocation.  Not only that this is more overhead
> > by the indirect calls, but also this makes harder to extend for the
> > debug print usage; because of the direct printk() call, you cannot
> > make it for dynamic debug or without debug like the standard helpers
> > such as pr_debug() or dev_dbg().
> > 
> > For addressing the problem, this patch replaces exfat_msg() function
> > with a macro to expand to pr_*() directly.  This allows us to create
> > exfat_debug() macro that is expanded to pr_debug() (which output can
> > gracefully suppressed via dyndbg).
> []
> > diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> []
> > @@ -508,14 +508,19 @@ void __exfat_fs_error(struct super_block *sb, int report, const char *fmt, ...)
> >  #define exfat_fs_error_ratelimit(sb, fmt, args...) \
> >  		__exfat_fs_error(sb, __ratelimit(&EXFAT_SB(sb)->ratelimit), \
> >  		fmt, ## args)
> > -void exfat_msg(struct super_block *sb, const char *lv, const char *fmt, ...)
> > -		__printf(3, 4) __cold;
> > +
> > +/* expand to pr_xxx() with prefix */
> > +#define exfat_msg(sb, lv, fmt, ...) \
> > +	pr_##lv("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
> > +
> >  #define exfat_err(sb, fmt, ...)						\
> > -	exfat_msg(sb, KERN_ERR, fmt, ##__VA_ARGS__)
> > +	exfat_msg(sb, err, fmt, ##__VA_ARGS__)
> >  #define exfat_warn(sb, fmt, ...)					\
> > -	exfat_msg(sb, KERN_WARNING, fmt, ##__VA_ARGS__)
> > +	exfat_msg(sb, warn, fmt, ##__VA_ARGS__)
> >  #define exfat_info(sb, fmt, ...)					\
> > -	exfat_msg(sb, KERN_INFO, fmt, ##__VA_ARGS__)
> > +	exfat_msg(sb, info, fmt, ##__VA_ARGS__)
> > +#define exfat_debug(sb, fmt, ...)					\
> > +	exfat_msg(sb, debug, fmt, ##__VA_ARGS__)
> 
> I think this would be clearer using pr_<level> directly instead
> of an indirecting macro that uses concatenation of <level> that
> obscures the actual use of pr_<level>
> 
> Either: (and this first option would be my preference)
> 
> #define exfat_err(sb, fmt, ...) \
> 	pr_err("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
> #define exfat_warn(sb, fmt, ...) \
> 	pr_warn("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
> etc...

IMO, it's a matter of taste, and I don't mind either way.
Just let me know.

> or using an indirecting macro:
> 
> #define exfat_printk(pr_level, sb, fmt, ...)	\
> 	pr_level("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)

Is pr_level() defined anywhere...?

> 
> and btw, there are multiple uses of exfat_<level> output with a
> unnecessary and duplicated '\n' that the macro already adds that
> should be removed:
> 
> $ git grep -P -n '\bexfat_(err|warn|info).*\\n' fs/exfat/
> fs/exfat/fatent.c:334:                  exfat_err(sb, "sbi->clu_srch_ptr is invalid (%u)\n",
> fs/exfat/nls.c:674:                     exfat_err(sb, "failed to read sector(0x%llx)\n",
> fs/exfat/super.c:467:           exfat_err(sb, "bogus sector size bits : %u\n",
> fs/exfat/super.c:476:           exfat_err(sb, "bogus sectors bits per cluster : %u\n",

Right, that should be addressed in another patch.


thanks,

Takashi

Re: [PATCH 3/4] exfat: Expand exfat_err() and co directly to pr_*() macro

[Joe Perches]

On Sat, 2022-07-23 at 10:04 +0200, Takashi Iwai wrote:
> On Sat, 23 Jul 2022 09:42:12 +0200, Joe Perches wrote:
> > On Fri, 2022-07-22 at 16:29 +0200, Takashi Iwai wrote:
> > > Currently the error and info messages handled by exfat_err() and co
> > > are tossed to exfat_msg() function that does nothing but passes the
> > > strings with printk() invocation.  Not only that this is more overhead
> > > by the indirect calls, but also this makes harder to extend for the
> > > debug print usage; because of the direct printk() call, you cannot
> > > make it for dynamic debug or without debug like the standard helpers
> > > such as pr_debug() or dev_dbg().
> > > 
> > > For addressing the problem, this patch replaces exfat_msg() function
> > > with a macro to expand to pr_*() directly.  This allows us to create
> > > exfat_debug() macro that is expanded to pr_debug() (which output can
> > > gracefully suppressed via dyndbg).
> > []
> > > diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> > []
> > > @@ -508,14 +508,19 @@ void __exfat_fs_error(struct super_block *sb, int report, const char *fmt, ...)
> > >  #define exfat_fs_error_ratelimit(sb, fmt, args...) \
> > >  		__exfat_fs_error(sb, __ratelimit(&EXFAT_SB(sb)->ratelimit), \
> > >  		fmt, ## args)
> > > -void exfat_msg(struct super_block *sb, const char *lv, const char *fmt, ...)
> > > -		__printf(3, 4) __cold;
> > > +
> > > +/* expand to pr_xxx() with prefix */
> > > +#define exfat_msg(sb, lv, fmt, ...) \
> > > +	pr_##lv("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
> > > +
> > >  #define exfat_err(sb, fmt, ...)						\
> > > -	exfat_msg(sb, KERN_ERR, fmt, ##__VA_ARGS__)
> > > +	exfat_msg(sb, err, fmt, ##__VA_ARGS__)
> > >  #define exfat_warn(sb, fmt, ...)					\
> > > -	exfat_msg(sb, KERN_WARNING, fmt, ##__VA_ARGS__)
> > > +	exfat_msg(sb, warn, fmt, ##__VA_ARGS__)
> > >  #define exfat_info(sb, fmt, ...)					\
> > > -	exfat_msg(sb, KERN_INFO, fmt, ##__VA_ARGS__)
> > > +	exfat_msg(sb, info, fmt, ##__VA_ARGS__)
> > > +#define exfat_debug(sb, fmt, ...)					\
> > > +	exfat_msg(sb, debug, fmt, ##__VA_ARGS__)
> > 
> > I think this would be clearer using pr_<level> directly instead
> > of an indirecting macro that uses concatenation of <level> that
> > obscures the actual use of pr_<level>
> > 
> > Either: (and this first option would be my preference)
> > 
> > #define exfat_err(sb, fmt, ...) \
> > 	pr_err("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
> > #define exfat_warn(sb, fmt, ...) \
> > 	pr_warn("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
> > etc...
> 
> IMO, it's a matter of taste, and I don't mind either way.
> Just let me know.
> 
> > or using an indirecting macro:
> > 
> > #define exfat_printk(pr_level, sb, fmt, ...)	\
> > 	pr_level("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
> 
> Is pr_level() defined anywhere...?

no

$ git grep -w pr_level
$ 

Re: [PATCH 3/4] exfat: Expand exfat_err() and co directly to pr_*() macro

[Petr Vorel]

Hi all,

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

Re: [PATCH 3/4] exfat: Expand exfat_err() and co directly to pr_*() macro

[Namjae Jeon]

2022-07-23 17:04 GMT+09:00, Takashi Iwai <tiwai@suse.de>:
> On Sat, 23 Jul 2022 09:42:12 +0200,
> Joe Perches wrote:
>>
>> On Fri, 2022-07-22 at 16:29 +0200, Takashi Iwai wrote:
>> > Currently the error and info messages handled by exfat_err() and co
>> > are tossed to exfat_msg() function that does nothing but passes the
>> > strings with printk() invocation.  Not only that this is more overhead
>> > by the indirect calls, but also this makes harder to extend for the
>> > debug print usage; because of the direct printk() call, you cannot
>> > make it for dynamic debug or without debug like the standard helpers
>> > such as pr_debug() or dev_dbg().
>> >
>> > For addressing the problem, this patch replaces exfat_msg() function
>> > with a macro to expand to pr_*() directly.  This allows us to create
>> > exfat_debug() macro that is expanded to pr_debug() (which output can
>> > gracefully suppressed via dyndbg).
>> []
>> > diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
>> []
>> > @@ -508,14 +508,19 @@ void __exfat_fs_error(struct super_block *sb, int
>> > report, const char *fmt, ...)
>> >  #define exfat_fs_error_ratelimit(sb, fmt, args...) \
>> >  		__exfat_fs_error(sb, __ratelimit(&EXFAT_SB(sb)->ratelimit), \
>> >  		fmt, ## args)
>> > -void exfat_msg(struct super_block *sb, const char *lv, const char *fmt,
>> > ...)
>> > -		__printf(3, 4) __cold;
>> > +
>> > +/* expand to pr_xxx() with prefix */
>> > +#define exfat_msg(sb, lv, fmt, ...) \
>> > +	pr_##lv("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
>> > +
>> >  #define exfat_err(sb, fmt, ...)						\
>> > -	exfat_msg(sb, KERN_ERR, fmt, ##__VA_ARGS__)
>> > +	exfat_msg(sb, err, fmt, ##__VA_ARGS__)
>> >  #define exfat_warn(sb, fmt, ...)					\
>> > -	exfat_msg(sb, KERN_WARNING, fmt, ##__VA_ARGS__)
>> > +	exfat_msg(sb, warn, fmt, ##__VA_ARGS__)
>> >  #define exfat_info(sb, fmt, ...)					\
>> > -	exfat_msg(sb, KERN_INFO, fmt, ##__VA_ARGS__)
>> > +	exfat_msg(sb, info, fmt, ##__VA_ARGS__)
>> > +#define exfat_debug(sb, fmt, ...)					\
>> > +	exfat_msg(sb, debug, fmt, ##__VA_ARGS__)
>>
>> I think this would be clearer using pr_<level> directly instead
>> of an indirecting macro that uses concatenation of <level> that
>> obscures the actual use of pr_<level>
>>
>> Either: (and this first option would be my preference)
>>
>> #define exfat_err(sb, fmt, ...) \
>> 	pr_err("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
>> #define exfat_warn(sb, fmt, ...) \
>> 	pr_warn("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
>> etc...
>
> IMO, it's a matter of taste, and I don't mind either way.
> Just let me know.
Joe has already said that he prefers the first.
Will you send v2 patch-set ?

Thanks!
>
>> or using an indirecting macro:
>>
>> #define exfat_printk(pr_level, sb, fmt, ...)	\
>> 	pr_level("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
>
> Is pr_level() defined anywhere...?
>
>>
>> and btw, there are multiple uses of exfat_<level> output with a
>> unnecessary and duplicated '\n' that the macro already adds that
>> should be removed:
>>
>> $ git grep -P -n '\bexfat_(err|warn|info).*\\n' fs/exfat/
>> fs/exfat/fatent.c:334:                  exfat_err(sb, "sbi->clu_srch_ptr
>> is invalid (%u)\n",
>> fs/exfat/nls.c:674:                     exfat_err(sb, "failed to read
>> sector(0x%llx)\n",
>> fs/exfat/super.c:467:           exfat_err(sb, "bogus sector size bits :
>> %u\n",
>> fs/exfat/super.c:476:           exfat_err(sb, "bogus sectors bits per
>> cluster : %u\n",
>
> Right, that should be addressed in another patch.
>
>
> thanks,
>
> Takashi
>

Re: [PATCH 3/4] exfat: Expand exfat_err() and co directly to pr_*() macro

[Takashi Iwai]

On Tue, 26 Jul 2022 09:02:40 +0200,
Namjae Jeon wrote:
> 
> 2022-07-23 17:04 GMT+09:00, Takashi Iwai <tiwai@suse.de>:
> > On Sat, 23 Jul 2022 09:42:12 +0200,
> > Joe Perches wrote:
> >>
> >> On Fri, 2022-07-22 at 16:29 +0200, Takashi Iwai wrote:
> >> > Currently the error and info messages handled by exfat_err() and co
> >> > are tossed to exfat_msg() function that does nothing but passes the
> >> > strings with printk() invocation.  Not only that this is more overhead
> >> > by the indirect calls, but also this makes harder to extend for the
> >> > debug print usage; because of the direct printk() call, you cannot
> >> > make it for dynamic debug or without debug like the standard helpers
> >> > such as pr_debug() or dev_dbg().
> >> >
> >> > For addressing the problem, this patch replaces exfat_msg() function
> >> > with a macro to expand to pr_*() directly.  This allows us to create
> >> > exfat_debug() macro that is expanded to pr_debug() (which output can
> >> > gracefully suppressed via dyndbg).
> >> []
> >> > diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
> >> []
> >> > @@ -508,14 +508,19 @@ void __exfat_fs_error(struct super_block *sb, int
> >> > report, const char *fmt, ...)
> >> >  #define exfat_fs_error_ratelimit(sb, fmt, args...) \
> >> >  		__exfat_fs_error(sb, __ratelimit(&EXFAT_SB(sb)->ratelimit), \
> >> >  		fmt, ## args)
> >> > -void exfat_msg(struct super_block *sb, const char *lv, const char *fmt,
> >> > ...)
> >> > -		__printf(3, 4) __cold;
> >> > +
> >> > +/* expand to pr_xxx() with prefix */
> >> > +#define exfat_msg(sb, lv, fmt, ...) \
> >> > +	pr_##lv("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
> >> > +
> >> >  #define exfat_err(sb, fmt, ...)						\
> >> > -	exfat_msg(sb, KERN_ERR, fmt, ##__VA_ARGS__)
> >> > +	exfat_msg(sb, err, fmt, ##__VA_ARGS__)
> >> >  #define exfat_warn(sb, fmt, ...)					\
> >> > -	exfat_msg(sb, KERN_WARNING, fmt, ##__VA_ARGS__)
> >> > +	exfat_msg(sb, warn, fmt, ##__VA_ARGS__)
> >> >  #define exfat_info(sb, fmt, ...)					\
> >> > -	exfat_msg(sb, KERN_INFO, fmt, ##__VA_ARGS__)
> >> > +	exfat_msg(sb, info, fmt, ##__VA_ARGS__)
> >> > +#define exfat_debug(sb, fmt, ...)					\
> >> > +	exfat_msg(sb, debug, fmt, ##__VA_ARGS__)
> >>
> >> I think this would be clearer using pr_<level> directly instead
> >> of an indirecting macro that uses concatenation of <level> that
> >> obscures the actual use of pr_<level>
> >>
> >> Either: (and this first option would be my preference)
> >>
> >> #define exfat_err(sb, fmt, ...) \
> >> 	pr_err("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
> >> #define exfat_warn(sb, fmt, ...) \
> >> 	pr_warn("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
> >> etc...
> >
> > IMO, it's a matter of taste, and I don't mind either way.
> > Just let me know.
> Joe has already said that he prefers the first.

My question was about the preference of the exfat maintainers :)

> Will you send v2 patch-set ?

Sure.


Takashi

Re: [PATCH 3/4] exfat: Expand exfat_err() and co directly to pr_*() macro

[Namjae Jeon]

2022-07-26 16:46 GMT+09:00, Takashi Iwai <tiwai@suse.de>:
> On Tue, 26 Jul 2022 09:02:40 +0200,
> Namjae Jeon wrote:
>>
>> 2022-07-23 17:04 GMT+09:00, Takashi Iwai <tiwai@suse.de>:
>> > On Sat, 23 Jul 2022 09:42:12 +0200,
>> > Joe Perches wrote:
>> >>
>> >> On Fri, 2022-07-22 at 16:29 +0200, Takashi Iwai wrote:
>> >> > Currently the error and info messages handled by exfat_err() and co
>> >> > are tossed to exfat_msg() function that does nothing but passes the
>> >> > strings with printk() invocation.  Not only that this is more
>> >> > overhead
>> >> > by the indirect calls, but also this makes harder to extend for the
>> >> > debug print usage; because of the direct printk() call, you cannot
>> >> > make it for dynamic debug or without debug like the standard helpers
>> >> > such as pr_debug() or dev_dbg().
>> >> >
>> >> > For addressing the problem, this patch replaces exfat_msg() function
>> >> > with a macro to expand to pr_*() directly.  This allows us to create
>> >> > exfat_debug() macro that is expanded to pr_debug() (which output can
>> >> > gracefully suppressed via dyndbg).
>> >> []
>> >> > diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h
>> >> []
>> >> > @@ -508,14 +508,19 @@ void __exfat_fs_error(struct super_block *sb,
>> >> > int
>> >> > report, const char *fmt, ...)
>> >> >  #define exfat_fs_error_ratelimit(sb, fmt, args...) \
>> >> >  		__exfat_fs_error(sb, __ratelimit(&EXFAT_SB(sb)->ratelimit), \
>> >> >  		fmt, ## args)
>> >> > -void exfat_msg(struct super_block *sb, const char *lv, const char
>> >> > *fmt,
>> >> > ...)
>> >> > -		__printf(3, 4) __cold;
>> >> > +
>> >> > +/* expand to pr_xxx() with prefix */
>> >> > +#define exfat_msg(sb, lv, fmt, ...) \
>> >> > +	pr_##lv("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
>> >> > +
>> >> >  #define exfat_err(sb, fmt, ...)						\
>> >> > -	exfat_msg(sb, KERN_ERR, fmt, ##__VA_ARGS__)
>> >> > +	exfat_msg(sb, err, fmt, ##__VA_ARGS__)
>> >> >  #define exfat_warn(sb, fmt, ...)					\
>> >> > -	exfat_msg(sb, KERN_WARNING, fmt, ##__VA_ARGS__)
>> >> > +	exfat_msg(sb, warn, fmt, ##__VA_ARGS__)
>> >> >  #define exfat_info(sb, fmt, ...)					\
>> >> > -	exfat_msg(sb, KERN_INFO, fmt, ##__VA_ARGS__)
>> >> > +	exfat_msg(sb, info, fmt, ##__VA_ARGS__)
>> >> > +#define exfat_debug(sb, fmt, ...)					\
>> >> > +	exfat_msg(sb, debug, fmt, ##__VA_ARGS__)
>> >>
>> >> I think this would be clearer using pr_<level> directly instead
>> >> of an indirecting macro that uses concatenation of <level> that
>> >> obscures the actual use of pr_<level>
>> >>
>> >> Either: (and this first option would be my preference)
>> >>
>> >> #define exfat_err(sb, fmt, ...) \
>> >> 	pr_err("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
>> >> #define exfat_warn(sb, fmt, ...) \
>> >> 	pr_warn("exFAT-fs (%s): " fmt "\n", (sb)->s_id, ##__VA_ARGS__)
>> >> etc...
>> >
>> > IMO, it's a matter of taste, and I don't mind either way.
>> > Just let me know.
>> Joe has already said that he prefers the first.
>
> My question was about the preference of the exfat maintainers :)
I also agree with his opinion.
>
>> Will you send v2 patch-set ?
>
> Sure.
Thanks a lot!
>
>
> Takashi
>