| Message ID | 20241213054610.55843-2-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vsprintf: Add %pTN to print Task Name | expand |
On Fri 2024-12-13 13:46:04, Yafang Shao wrote: > Since the task->comm is guaranteed to be NUL-ternimated, we can print it > directly. Add a new vsnprintf format specifier "%pTN" to print task comm, > where 'p' represents the task Pointer, 'T' stands for Task, and 'N' denots > Name. With this abstraction, the user no longer needs to care about > retrieving task name. What is the advantage, please? Honestly, I believe that the meaning of printk("%s\n", task->comm); is much more clear than using a cryptic %pXYZ modifier: printk("%pTN\n", task); The %pXYZ modifiers makes sense only when the formatting of the printed information needs some processing. But this is a plain string. IMHO, it is not worth it. In fact, I believe that it is a counter productive. Best Regards, Petr
Petr Mladek <pmladek@suse.com> writes: > On Fri 2024-12-13 13:46:04, Yafang Shao wrote: >> Since the task->comm is guaranteed to be NUL-ternimated, we can print it >> directly. Add a new vsnprintf format specifier "%pTN" to print task comm, >> where 'p' represents the task Pointer, 'T' stands for Task, and 'N' denots >> Name. With this abstraction, the user no longer needs to care about >> retrieving task name. > > What is the advantage, please? > > Honestly, I believe that the meaning of > > printk("%s\n", task->comm); > > is much more clear than using a cryptic %pXYZ modifier: > > printk("%pTN\n", task); > > > The %pXYZ modifiers makes sense only when the formatting of the printed > information needs some processing. But this is a plain string. > IMHO, it is not worth it. In fact, I believe that it is a > counter productive. I agree, it makes the code harder to read for someone who is not familiar with all the %p magic we have (like me).
On Fri, Dec 13, 2024 at 4:05 PM Petr Mladek <pmladek@suse.com> wrote: > > On Fri 2024-12-13 13:46:04, Yafang Shao wrote: > > Since the task->comm is guaranteed to be NUL-ternimated, we can print it > > directly. Add a new vsnprintf format specifier "%pTN" to print task comm, > > where 'p' represents the task Pointer, 'T' stands for Task, and 'N' denots > > Name. With this abstraction, the user no longer needs to care about > > retrieving task name. > > What is the advantage, please? The advantage is that it provides the flexibility to modify the comm representation to meet future requirements. For instance, we could rename it to "task_name" and increase its size. > > Honestly, I believe that the meaning of > > printk("%s\n", task->comm); > > is much more clear than using a cryptic %pXYZ modifier: > > printk("%pTN\n", task); > > > The %pXYZ modifiers makes sense only when the formatting of the printed > information needs some processing. But this is a plain string. That makes sense to me. > IMHO, it is not worth it. In fact, I believe that it is a > counter productive. Linus, what are your thoughts? -- Regards Yafang
On Fri, Dec 13, 2024 at 10:35:03AM +0200, Kalle Valo wrote: > I agree, it makes the code harder to read for someone who is not > familiar with all the %p magic we have (like me). +1
On Fri, Dec 13, 2024 at 02:27:09PM +0100, Borislav Petkov wrote: > On Fri, Dec 13, 2024 at 10:35:03AM +0200, Kalle Valo wrote: > > I agree, it makes the code harder to read for someone who is not > > familiar with all the %p magic we have (like me). > +1 And me too. In case one thinks of unprintable characters, %pE is for that.
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 6ac02bbb7df1..bb1018d79655 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -2273,6 +2273,17 @@ char *resource_or_range(const char *fmt, char *buf, char *end, void *ptr, return resource_string(buf, end, ptr, spec, fmt); } +static noinline_for_stack +char *task_name_string(char *buf, char *end, struct task_struct *p, + struct printf_spec spec) +{ + if (check_pointer(&buf, end, p, spec)) + return buf; + + buf = string(buf, end, p->comm, spec); + return buf; +} + int __init no_hash_pointers_enable(char *str) { if (no_hash_pointers) @@ -2525,6 +2536,13 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, default: return error_string(buf, end, "(einval)", spec); } + case 'T': + switch (fmt[1]) { + case 'N': + return task_name_string(buf, end, ptr, spec); + default: + return error_string(buf, end, "(einval)", spec); + } default: return default_pointer(buf, end, ptr, spec); } diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 9eed3683ad76..fe0d80f55ce8 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -6908,11 +6908,13 @@ sub process { $specifier = $1; $extension = $2; $qualifier = $3; - if ($extension !~ /[4SsBKRraEehMmIiUDdgVCbGNOxtf]/ || + if ($extension !~ /[4SsBKRraEehMmIiUDdgVCbGNOxtfT]/ || ($extension eq "f" && defined $qualifier && $qualifier !~ /^w/) || ($extension eq "4" && - defined $qualifier && $qualifier !~ /^cc/)) { + defined $qualifier && $qualifier !~ /^cc/) || + ($extension eq "T" && + defined $qualifier && $qualifier ne "N")) { $bad_specifier = $specifier; last; }
Since the task->comm is guaranteed to be NUL-ternimated, we can print it directly. Add a new vsnprintf format specifier "%pTN" to print task comm, where 'p' represents the task Pointer, 'T' stands for Task, and 'N' denots Name. With this abstraction, the user no longer needs to care about retrieving task name. checkpatch.pl is updated accordingly. Link: https://lore.kernel.org/bpf/CAHk-=wgqrwFXK-CO8-V4fwUh5ymnUZ=wJnFyufV1dM9rC1t3Lg@mail.gmail.com Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Petr Mladek <pmladek@suse.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> Cc: Sergey Senozhatsky <senozhatsky@chromium.org> Cc: Andy Whitcroft <apw@canonical.com> Cc: Joe Perches <joe@perches.com> Cc: Dwaipayan Ray <dwaipayanray1@gmail.com> Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com> --- lib/vsprintf.c | 18 ++++++++++++++++++ scripts/checkpatch.pl | 6 ++++-- 2 files changed, 22 insertions(+), 2 deletions(-)