linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Kriish Sharma <kriish.sharma2006@gmail.com>
Cc: brauner@kernel.org, jack@suse.cz, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, david.hunter.linux@gmail.com,
	skhan@linuxfoundation.org
Subject: Re: [PATCH] fs: doc: describe 'pinned' parameter in do_lock_mount()
Date: Wed, 24 Sep 2025 22:22:07 +0100	[thread overview]
Message-ID: <20250924212207.GV39973@ZenIV> (raw)
In-Reply-To: <20250924205730.GU39973@ZenIV>

On Wed, Sep 24, 2025 at 09:57:30PM +0100, Al Viro wrote:
> On Wed, Sep 24, 2025 at 07:36:11PM +0000, Kriish Sharma wrote:
> > The kernel-doc comment for do_lock_mount() was missing a description
> > for the 'pinned' parameter:
> > 
> >   Warning: fs/namespace.c:2772 function parameter 'pinned' not described
> >   in 'do_lock_mount'
> > 
> > This patch adds a short description:
> > 
> >   @pinned: receives the pinned mountpoint
> > 
> > to fix the warning and improve documentation clarity.
> 
> Sigh...  There we go again...
> 	1.  It does not improve documentation clarity - it adds
> a misleading line to an opaque chunk of text that does not match
> what the function *does*.
> 	2.  In -next both the calling conventions and the comment
> are both changed, hopefully making it more readable.
> 	3.  Essentially the same patch has already been posted and
> discussed.
> 
> NAKed-by: Al Viro <viro@zeniv.linux.org.uk>

PS: as for the clarity, I'd like to point out that with your patch
applied nothing in the comments explains what is *done* to that
argument - it's not even mentioned there.  Incidentally, "receives"
normally implies an IN argument; this is an OUT one (function set the
environment for mounting something at given location and stores the
resulting context in caller-supplied structure).

Comment quality needs to be improved and these warnings actually
do catch some of the stale (and generally incomprehensible) ones.
Unfortunately, it's easy to fool the heuristics without doing
anything about the underlying problem, in effect hiding it.

Folks, please don't do that - this is not an improvement.  If you
spot something of that sort and want to take a pass at fixing it,
more power to you, but the main criteria here would be "does that
text make it easier to understand the function in question and/or
the rules for using it".  If it's genuinely hard to figure out,
don't hesitate to ask.

      reply	other threads:[~2025-09-24 21:22 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-09-24 19:36 [PATCH] fs: doc: describe 'pinned' parameter in do_lock_mount() Kriish Sharma
2025-09-24 20:57 ` Al Viro
2025-09-24 21:22   ` Al Viro [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250924212207.GV39973@ZenIV \
    --to=viro@zeniv.linux.org.uk \
    --cc=brauner@kernel.org \
    --cc=david.hunter.linux@gmail.com \
    --cc=jack@suse.cz \
    --cc=kriish.sharma2006@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=skhan@linuxfoundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).