Messages in this thread | | | From | Justin He <> | Subject | RE: [PATCH v5 1/4] fs: introduce helper d_path_unsafe() | Date | Wed, 23 Jun 2021 02:02:45 +0000 |
| |
Hi Andy
> -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Tuesday, June 22, 2021 10:37 PM > To: Justin He <Justin.He@arm.com> > Cc: Petr Mladek <pmladek@suse.com>; Steven Rostedt <rostedt@goodmis.org>; > Sergey Senozhatsky <senozhatsky@chromium.org>; 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-doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > fsdevel@vger.kernel.org; Matthew Wilcox <willy@infradead.org>; Christoph > Hellwig <hch@infradead.org>; nd <nd@arm.com> > Subject: Re: [PATCH v5 1/4] fs: introduce helper d_path_unsafe() > > On Tue, Jun 22, 2021 at 10:06:31PM +0800, Jia He wrote: > > This helper is similar to d_path() except that it doesn't take any > > seqlock/spinlock. It is typical for debugging purposes. Besides, > > an additional return value *prenpend_len* is used to get the full > > path length of the dentry, ingoring the tail '\0'. > > the full path length = end - buf - prepend_length - 1 > > Missed period at the end of sentence. >
Okay > > Previously it will skip the prepend_name() loop at once in > > __prepen_path() when the buffer length is not enough or even negative. > > prepend_name_with_len() will get the full length of dentry name > > together with the parent recursively regardless of the buffer length. > > > If someone invokes snprintf() with small but positive space, > > prepend_name_with_len() moves and copies the string partially. > > > > More than that, kasprintf() will pass NULL _buf_ and _end_ as the > > parameters. Hence return at the very beginning with false in this case. > > These two paragraphs are talking about printf() interface, while patch has > nothing to do with it. Please, rephrase in a way that it doesn't refer to > the > particular callers. Better to mention them in the corresponding printf() > patch(es). >
Okay > ... > > > * prepend_name - prepend a pathname in front of current buffer pointer > > - * @buffer: buffer pointer > > - * @buflen: allocated length of the buffer > > + * @p: prepend buffer which contains buffer pointer and allocated length > > > * @name: name string and length qstr structure > > Indentation issue btw, can be fixed in the same patch.
Okay > > > * > > * With RCU path tracing, it may race with d_move(). Use READ_ONCE() to > > Shouldn't this be a separate change with corresponding Fixes tag?
Sorry, I don't quite understand here. What do you want to fix?
> > ... > > > +/** > > + * d_path_unsafe - return the full path of a dentry without taking > > + * any seqlock/spinlock. This helper is typical for debugging purposes. > > Seems you ignored my comment, or forget to test, or compile test with > kernel > doc validator enabled doesn't show any issues. If it's the latter, we have > to > fix kernel doc validator. > > TL;DR: describe parameters as well. >
My bad. Apologize for that. > > + */ > > ... > > > + struct path root; > > + struct mount *mnt = real_mount(path->mnt); > > + DECLARE_BUFFER(b, buf, buflen); > > Can wee keep this in reversed xmas tree order?
Sure 😊
-- Cheers, Justin (Jia He)
| |