Re: [PATCH v3 01/15] VFS: note error returns in documentation for various lookup functions

[NeilBrown]

On Tue, 03 Mar 2026, Jeff Layton wrote:
> On Wed, 2026-02-25 at 09:16 +1100, NeilBrown wrote:
> > From: NeilBrown <neil@xxxxxxxxxx>
> > 
> > Darrick recently noted that try_lookup_noperm() is documented as
> > "Look up a dentry by name in the dcache, returning NULL if it does not
> > currently exist." but it can in fact return an error.
> > 
> > So update the documentation for that and related functions.
> > 
> > Link: https://lore.kernel.org/all/20260218234917.GA6490@frogsfrogsfrogs/
> > Cc: "Darrick J. Wong" <djwong@xxxxxxxxxx>
> > Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> > ---
> >  fs/namei.c | 29 ++++++++++++++++++++++++++++-
> >  1 file changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 58f715f7657e..6f595f58acfe 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -3124,7 +3124,8 @@ static int lookup_one_common(struct mnt_idmap *idmap,
> >   * @base:	base directory to lookup from
> >   *
> >   * Look up a dentry by name in the dcache, returning NULL if it does not
> > - * currently exist.  The function does not try to create a dentry and if one
> > + * currently exist or an error if there is a problem with the name.
> > + * The function does not try to create a dentry and if one
> >   * is found it doesn't try to revalidate it.
> >   *
> >   * Note that this routine is purely a helper for filesystem usage and should
> > @@ -3132,6 +3133,11 @@ static int lookup_one_common(struct mnt_idmap *idmap,
> >   *
> >   * No locks need be held - only a counted reference to @base is needed.
> >   *
> > + * Returns:
> > + *   - ref-counted dentry on success, or
> > + *   - %NULL if name could not be found, or
> > + *   - ERR_PTR(-EACCES) if name is dot or dotdot or contains a slash or nul, or
> > + *   - ERR_PTR() if fs provide ->d_hash, and this returned an error.
> >   */
> >  struct dentry *try_lookup_noperm(struct qstr *name, struct dentry *base)
> >  {
> > @@ -3208,6 +3214,11 @@ EXPORT_SYMBOL(lookup_one);
> >   *
> >   * Unlike lookup_one, it should be called without the parent
> >   * i_rwsem held, and will take the i_rwsem itself if necessary.
> > + *
> > + * Returns: - A dentry, possibly negative, or
> > + *	    - same errors as try_lookup_noperm() or
> > + *	    - ERR_PTR(-ENOENT) if parent has been removed, or
> > + *	    - ERR_PTR(-EACCES) if parent directory is not searchable.
> >   */
> >  struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap, struct qstr *name,
> >  				   struct dentry *base)
> > @@ -3244,6 +3255,10 @@ EXPORT_SYMBOL(lookup_one_unlocked);
> >   * It should be called without the parent i_rwsem held, and will take
> >   * the i_rwsem itself if necessary.  If a fatal signal is pending or
> >   * delivered, it will return %-EINTR if the lock is needed.
> > + *
> > + * Returns: A dentry, possibly negative, or
> > + *	   - same errors as lookup_one_unlocked() or
> > + *	   - ERR_PTR(-EINTR) if a fatal signal is pending.
> >   */
> >  struct dentry *lookup_one_positive_killable(struct mnt_idmap *idmap,
> >  					    struct qstr *name,
> 
> Claude says:
> 
>   lookup_one_positive_killable() documentation says "A dentry, possibly negative" but the function
>   explicitly converts negative dentries to ERR_PTR(-ENOENT). It should say "A positive dentry" like
>   the companion functions lookup_one_positive_unlocked() and lookup_noperm_positive_unlocked().

Thanks.  The top patch in my next batch contains a bunch of similar
documentation cleanups.  I'll add this to that patch.

> 
> ...but that seems to be the only "regression" it found. 
> 

Good to know!

Thanks,
NeilBrown

> 
> Aside from that nit, this looks fine to me.
> 
> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
>