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@xxxxxxx>:
> 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
>