Messages in this thread | | | Subject | Re: [PATCH v5] printk: Userspace format enumeration support | From | Rasmus Villemoes <> | Date | Wed, 17 Mar 2021 11:03:20 +0100 |
| |
On 17/03/2021 09.40, Petr Mladek wrote: > On Tue 2021-03-16 14:28:12, Chris Down wrote: >> Rasmus Villemoes writes: >>> I think it's pointless renaming the symbol to _printk, with all the >>> churn and reduced readability that involves (especially when reading >>> assembly "why are we calling _printk and not printk here?"). There's >>> nothing wrong with providing a macro wrapper by the same name >>> >>> #define printk(bla bla) ({ do_stuff; printk(bla bla); }) >>> >>> Only two places would need to be updated to surround the word printk in >>> parentheses to suppress macro expansion: The declaration and the >>> definition of printk. I.e. >>> >>> int (printk)(const char *s, ...)
[Of course, one could avoid that on the declaration by making sure the macro wrapper is defined below.]
>> Hmm, I'm indifferent to either. Personally I don't like the ambiguity of >> having both a macro and function share the same name and having to think >> "what's the preprocessor context here?". > > I would prefer to keep _printk. I agree that it creates some churn but > it is easier to see what is going on.
It is? Nobody except the few who happen to remember about the printk_index thing are going to understand why, when looking at disassembly, there's now calls of a _printk function. "Huh, my code just does pr_err(), I'm not calling some internal printk function". But it's not up to me, so I'll stop there.
Anyway, on to the other thing I mentioned on dev_err and friends: I think it would improve readability and make it a lot easier to (probably in a later patch) add support for all those dev_* and net_* and whatever other subsystems have their own wrappers if one created a new header, linux/printk-index.h, doing something like
#ifdef CONFIG_PRINTK_INDEX #define __printk_index_emit(fmt) do {the appropriate magic} while (0) #else #define __printk_index_emit(fmt) do {} while (0) #endif
#define printk_index_wrap(fmt, real_func, ...) ({ \ __printk_index_emit(fmt); \ real_func(__VA_ARGS__); \ })
and then it's a matter of doing
#define printk(fmt, ...) printk_index_wrap(fmt, _printk, fmt, ##__VA_ARGS__)
or
#define _dev_err(dev, fmt, ...) printk_index_wrap(fmt, __dev_err, dev, fmt, ##__VA_ARGS__)
(yeah, _dev_err is the real function, dev_err is already a macro, so doing it for dev_* would involve renaming _dev_err to __dev_err. Or one could fold the printk_index logic into the existing dev_err macro).
That is, avoid defining two different versions of each and every required wrapper macro depending on CONFIG_PRINTK_INDEX.
One could also record the function a format is being used with - without that, the display probably can't show a reasonable <level> for those dev_* function.
Rasmus
| |