Hi Petr

-----Original Message-----
From: Petr Mladek <pmladek@suse.com>
Sent: Monday, May 10, 2021 9:05 PM
To: Justin He <redacted>
Cc: Steven Rostedt <rostedt@goodmis.org>; Sergey Senozhatsky
[off-list ref]; Andy Shevchenko
[off-list ref]; Rasmus Villemoes
[off-list ref]; Jonathan Corbet [off-list ref]; Alexander
Viro [off-list ref]; Linus Torvalds <torvalds@linux-
foundation.org>; Al Viro [off-list ref]; Heiko Carstens
[off-list ref]; Vasily Gorbik [off-list ref]; Christian
Borntraeger [off-list ref]; Eric W . Biederman
[off-list ref]; Darrick J. Wong [off-list ref]; Peter
Zijlstra (Intel) [off-list ref]; Ira Weiny [off-list ref];
Eric Biggers [off-list ref]; Ahmed S. Darwish
[off-list ref]; linux-doc@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-s390@vger.kernel.org; linux-
fsdevel@vger.kernel.org
Subject: Re: [PATCH RFC 2/3] lib/vsprintf.c: make %pD print full path for
file

On Sat 2021-05-08 20:25:29, Jia He wrote:

quoted

We have '%pD' for printing a filename. It may not be perfect (by
default it only prints one component.)

As suggested by Linus at [1]:
A dentry has a parent, but at the same time, a dentry really does
inherently have "one name" (and given just the dentry pointers, you
can't show mount-related parenthood, so in many ways the "show just
one name" makes sense for "%pd" in ways it doesn't necessarily for
"%pD"). But while a dentry arguably has that "one primary component",
a _file_ is certainly not exclusively about that last component.

Hence "file_dentry_name()" simply shouldn't use "dentry_name()" at all.
Despite that shared code origin, and despite that similar letter
choice (lower-vs-upper case), a dentry and a file really are very
different from a name standpoint.

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f0c35d9b65bf..8220ab1411c5 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c

@@ -27,6 +27,7 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/kernel.h>
+#include <linux/dcache.h>
 #include <linux/kallsyms.h>
 #include <linux/math64.h>
 #include <linux/uaccess.h>

@@ -923,10 +924,17 @@ static noinline_for_stack
 char *file_dentry_name(char *buf, char *end, const struct file *f,
                    struct printf_spec spec, const char *fmt)
 {
+   const struct path *path = &f->f_path;

This dereferences @f before it is checked by check_pointer().

Okay

quoted

+   char *p;
+   char tmp[128];
+
    if (check_pointer(&buf, end, f, spec))
            return buf;

-   return dentry_name(buf, end, f->f_path.dentry, spec, fmt);
+   p = d_path_fast(path, (char *)tmp, 128);
+   buf = string(buf, end, p, spec);

Is 128 a limit of the path or just a compromise, please?

It is just a compromise.

d_path_fast() limits the size of the buffer so we could use @buf
directly. We basically need to imitate what string_nocheck() does:

     + the length is limited by min(spec.precision, end-buf);
     + the string need to get shifted by widen_string()

We already do similar thing in dentry_name(). It might look like:

char *file_dentry_name(char *buf, char *end, const struct file *f,
                      struct printf_spec spec, const char *fmt)
{
      const struct path *path;
      int lim, len;
      char *p;

      if (check_pointer(&buf, end, f, spec))
              return buf;

      path = &f->f_path;
      if (check_pointer(&buf, end, path, spec))
              return buf;

      lim = min(spec.precision, end - buf);
      p = d_path_fast(path, buf, lim);
      if (IS_ERR(p))
              return err_ptr(buf, end, p, spec);

      len = strlen(buf);
      return widen_string(buf + len, len, end, spec);
}

Note that the code is _not_ even compile tested. It might include
some ugly mistake.

Okay, let me try it together with Linus's prepend_entries().
Thanks for the suggestion.

quoted

+
+   return buf;
 }
 #ifdef CONFIG_BLOCK
 static noinline_for_stack

@@ -2296,7 +2304,7 @@ early_param("no_hash_pointers",

no_hash_pointers_enable);

quoted

  * - 'a[pd]' For address types [p] phys_addr_t, [d] dma_addr_t and

derivatives

quoted

  *           (default assumed to be phys_addr_t, passed by reference)
  * - 'd[234]' For a dentry name (optionally 2-4 last components)
- * - 'D[234]' Same as 'd' but for a struct file
+ * - 'D' Same as 'd' but for a struct file

It is not really the same. We should make it clear that it prints
the full path:

Okay


--
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.