linux-kernel - Re: [PATCH 3/4] exfat: Expand exfat_err() and co directly to pr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <871qu8tiys.wl-tiwai@suse.de>
Date:   Tue, 26 Jul 2022 09:46:03 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Namjae Jeon <linkinjeon@...nel.org>
Cc:     Takashi Iwai <tiwai@...e.de>, Joe Perches <joe@...ches.com>,
        linux-fsdevel@...r.kernel.org,
        Sungjong Seo <sj1557.seo@...sung.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] exfat: Expand exfat_err() and co directly to pr_*() macro

On Tue, 26 Jul 2022 09:02:40 +0200,
Namjae Jeon wrote:
> 
> 2022-07-23 17:04 GMT+09:00, Takashi Iwai <tiwai@...e.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