| Message ID | 20210615154952.2744-5-justin.he@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | make '%pD' print full path for file | expand |
On Tue, Jun 15, 2021 at 6:55 PM Jia He <justin.he@arm.com> wrote: > > After the behaviour of specifier '%pD' is changed to print full path > of struct file, the related test cases are also updated. > > Given the string of '%pD' is prepended from the end of the buffer, the > check of "wrote beyond the nul-terminator" should be skipped. > > Signed-off-by: Jia He <justin.he@arm.com> > --- > lib/test_printf.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index d1d2f898ebae..9f851a82b3af 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -16,6 +16,7 @@ > > #include <linux/bitmap.h> > #include <linux/dcache.h> > +#include <linux/fs.h> > #include <linux/socket.h> > #include <linux/in.h> > > @@ -34,6 +35,7 @@ KSTM_MODULE_GLOBALS(); > > static char *test_buffer __initdata; > static char *alloced_buffer __initdata; > +static bool is_prepended_buf __initdata; > > extern bool no_hash_pointers; > > @@ -78,7 +80,7 @@ do_test(int bufsize, const char *expect, int elen, > return 1; > } > > - if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) { > + if (!is_prepended_buf && memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) { Can it be parametrized? I don't like the custom test case being involved here like this. > pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n", > bufsize, fmt); > return 1; > @@ -501,6 +503,27 @@ dentry(void) > test(" bravo/alfa| bravo/alfa", "%12pd2|%*pd2", &test_dentry[2], 12, &test_dentry[2]); > } > > +static struct vfsmount test_vfsmnt __initdata = {}; > + > +static struct file test_file __initdata = { > + .f_path = { .dentry = &test_dentry[2], > + .mnt = &test_vfsmnt, > + }, > +}; > + > +static void __init > +f_d_path(void) > +{ > + test("(null)", "%pD", NULL); > + test("(efault)", "%pD", PTR_INVALID); > + > + is_prepended_buf = true; > + test("/bravo/alfa |/bravo/alfa ", "%-14pD|%*pD", &test_file, -14, &test_file); > + test(" /bravo/alfa| /bravo/alfa", "%14pD|%*pD", &test_file, 14, &test_file); > + test(" /bravo/alfa|/bravo/alfa ", "%14pD|%-14pD", &test_file, &test_file); > + is_prepended_buf = false; > +} > + > static void __init > struct_va_format(void) > { > @@ -784,6 +807,7 @@ test_pointer(void) > ip(); > uuid(); > dentry(); > + f_d_path(); > struct_va_format(); > time_and_date(); > struct_clk(); > -- > 2.17.1 >
On Tue 2021-06-15 23:47:29, Andy Shevchenko wrote: > On Tue, Jun 15, 2021 at 6:55 PM Jia He <justin.he@arm.com> wrote: > > > > After the behaviour of specifier '%pD' is changed to print full path > > of struct file, the related test cases are also updated. > > > > Given the string of '%pD' is prepended from the end of the buffer, the > > check of "wrote beyond the nul-terminator" should be skipped. > > > > Signed-off-by: Jia He <justin.he@arm.com> > > --- > > lib/test_printf.c | 26 +++++++++++++++++++++++++- > > 1 file changed, 25 insertions(+), 1 deletion(-) > > > > diff --git a/lib/test_printf.c b/lib/test_printf.c > > index d1d2f898ebae..9f851a82b3af 100644 > > --- a/lib/test_printf.c > > +++ b/lib/test_printf.c > > @@ -78,7 +80,7 @@ do_test(int bufsize, const char *expect, int elen, > > return 1; > > } > > > > - if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) { > > > + if (!is_prepended_buf && memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) { > > Can it be parametrized? I don't like the custom test case being > involved here like this. Yup, it would be nice. Also it is far from obvious what @is_prepended_buf means if you do not have context of this patchset. I think about a more generic name that comes from the wording used in 3rd patch, e.g. @need_scratch_space or @using_scratch_space or @dirty_buf Anyway, the most easy way to pass this as a parameter would be to add it to __test() and define a wrapper, .e.g: static void __printf(3, 4) __init __test(const char *expect, int elen, bool using_scratch_space, const char *fmt, ...) /* * More relaxed test for non-standard formats that are using the provided buffer * as a scratch space and write beyond the trailing '\0'. */ #define test_using_scratch_space(expect, fmt, ...) \ __test(expect, strlen(expect), true, fmt, ##__VA_ARGS__) Best Regards, Petr
> -----Original Message----- > From: Petr Mladek <pmladek@suse.com> > Sent: Thursday, June 17, 2021 10:53 PM > To: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Justin He <Justin.He@arm.com>; Steven Rostedt <rostedt@goodmis.org>; > Sergey Senozhatsky <senozhatsky@chromium.org>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com>; Rasmus Villemoes > <linux@rasmusvillemoes.dk>; Jonathan Corbet <corbet@lwn.net>; Alexander > Viro <viro@zeniv.linux.org.uk>; Linus Torvalds <torvalds@linux- > foundation.org>; Peter Zijlstra (Intel) <peterz@infradead.org>; Eric > Biggers <ebiggers@google.com>; Ahmed S. Darwish <a.darwish@linutronix.de>; > Linux Documentation List <linux-doc@vger.kernel.org>; Linux Kernel Mailing > List <linux-kernel@vger.kernel.org>; Linux FS Devel <linux- > fsdevel@vger.kernel.org>; Matthew Wilcox <willy@infradead.org> > Subject: Re: [PATCH RFCv4 4/4] lib/test_printf.c: add test cases for '%pD' > > On Tue 2021-06-15 23:47:29, Andy Shevchenko wrote: > > On Tue, Jun 15, 2021 at 6:55 PM Jia He <justin.he@arm.com> wrote: > > > > > > After the behaviour of specifier '%pD' is changed to print full path > > > of struct file, the related test cases are also updated. > > > > > > Given the string of '%pD' is prepended from the end of the buffer, the > > > check of "wrote beyond the nul-terminator" should be skipped. > > > > > > Signed-off-by: Jia He <justin.he@arm.com> > > > --- > > > lib/test_printf.c | 26 +++++++++++++++++++++++++- > > > 1 file changed, 25 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/test_printf.c b/lib/test_printf.c > > > index d1d2f898ebae..9f851a82b3af 100644 > > > --- a/lib/test_printf.c > > > +++ b/lib/test_printf.c > > > @@ -78,7 +80,7 @@ do_test(int bufsize, const char *expect, int elen, > > > return 1; > > > } > > > > > > - if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - > (written + 1))) { > > > > > + if (!is_prepended_buf && memchr_inv(test_buffer + written + 1, > FILL_CHAR, bufsize - (written + 1))) { > > > > Can it be parametrized? I don't like the custom test case being > > involved here like this. > > Yup, it would be nice. > > Also it is far from obvious what @is_prepended_buf means if you do not > have context of this patchset. I think about a more generic name > that comes from the wording used in 3rd patch, e.g. > > @need_scratch_space or @using_scratch_space or @dirty_buf > > Anyway, the most easy way to pass this as a parameter would be to add it > to __test() and define a wrapper, .e.g: > > static void __printf(3, 4) __init > __test(const char *expect, int elen, bool using_scratch_space, > const char *fmt, ...) > > /* > * More relaxed test for non-standard formats that are using the provided > buffer > * as a scratch space and write beyond the trailing '\0'. > */ > #define test_using_scratch_space(expect, fmt, ...) \ > __test(expect, strlen(expect), true, fmt, ##__VA_ARGS__) Okay, thanks for the suggestion. -- Cheers, Justin (Jia He) IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
diff --git a/lib/test_printf.c b/lib/test_printf.c index d1d2f898ebae..9f851a82b3af 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -16,6 +16,7 @@ #include <linux/bitmap.h> #include <linux/dcache.h> +#include <linux/fs.h> #include <linux/socket.h> #include <linux/in.h> @@ -34,6 +35,7 @@ KSTM_MODULE_GLOBALS(); static char *test_buffer __initdata; static char *alloced_buffer __initdata; +static bool is_prepended_buf __initdata; extern bool no_hash_pointers; @@ -78,7 +80,7 @@ do_test(int bufsize, const char *expect, int elen, return 1; } - if (memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) { + if (!is_prepended_buf && memchr_inv(test_buffer + written + 1, FILL_CHAR, bufsize - (written + 1))) { pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n", bufsize, fmt); return 1; @@ -501,6 +503,27 @@ dentry(void) test(" bravo/alfa| bravo/alfa", "%12pd2|%*pd2", &test_dentry[2], 12, &test_dentry[2]); } +static struct vfsmount test_vfsmnt __initdata = {}; + +static struct file test_file __initdata = { + .f_path = { .dentry = &test_dentry[2], + .mnt = &test_vfsmnt, + }, +}; + +static void __init +f_d_path(void) +{ + test("(null)", "%pD", NULL); + test("(efault)", "%pD", PTR_INVALID); + + is_prepended_buf = true; + test("/bravo/alfa |/bravo/alfa ", "%-14pD|%*pD", &test_file, -14, &test_file); + test(" /bravo/alfa| /bravo/alfa", "%14pD|%*pD", &test_file, 14, &test_file); + test(" /bravo/alfa|/bravo/alfa ", "%14pD|%-14pD", &test_file, &test_file); + is_prepended_buf = false; +} + static void __init struct_va_format(void) { @@ -784,6 +807,7 @@ test_pointer(void) ip(); uuid(); dentry(); + f_d_path(); struct_va_format(); time_and_date(); struct_clk();
After the behaviour of specifier '%pD' is changed to print full path of struct file, the related test cases are also updated. Given the string of '%pD' is prepended from the end of the buffer, the check of "wrote beyond the nul-terminator" should be skipped. Signed-off-by: Jia He <justin.he@arm.com> --- lib/test_printf.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)