Re: [PATCH v5 1/4] fs: introduce helper d_path_unsafe()

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Jia 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@arm.com
Subject: Re: [PATCH v5 1/4] fs: introduce helper d_path_unsafe()
Date: Tue, 22 Jun 2021 17:36:39 +0300	[thread overview]
Message-ID: <YNH1d0aAu1WRiua1@smile.fi.intel.com> (raw)
In-Reply-To: <20210622140634.2436-2-justin.he@arm.com>

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.

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

...

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

>   *
>   * 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?

...

> +/**
> + * 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.

> + */

...

> +	struct path root;
> +	struct mount *mnt = real_mount(path->mnt);
> +	DECLARE_BUFFER(b, buf, buflen);

Can wee keep this in reversed xmas tree order?

-- 
With Best Regards,
Andy Shevchenko