Re: [PATCH v1] 9p: Fix mkdir to return NULL on success

[Hongling Zeng]

  Thank you for the review. You're right about the unnecessary err variable
  and NULL check - v9fs_create() never returns NULL and p9_fid_put() 
handles
  both NULL and IS_ERR cases internally.

  I'll prepare a v2 patch with the simplified implementation as suggested.

  Regards,
  Hongling Zeng

在 2026年05月20日 01:35, David Laight 写道:
> On Mon, 11 May 2026 14:18:30 +0800
> Hongling Zeng <zenghongling@xxxxxxxxxx> wrote:
>
> > When mkdir succeeds, v9fs_vfs_mkdir_dotl() and v9fs_vfs_mkdir() return
> > ERR_PTR(0) which is incorrect. They should return NULL instead for
> > success and ERR_PTR() only with negative error codes for failure.
> >
> > Return NULL instead of passing to ERR_PTR while err is zero
> > Fixes smatch warnings:
> >    fs/9p/vfs_inode_dotl.c:420 v9fs_vfs_mkdir_dotl() warn: passing zero to 'ERR_PTR'
> >    fs/9p/vfs_inode.c:695 v9fs_vfs_mkdir() warn: passing zero to 'ERR_PTR'
> >
> > This change does not alter the runtime behavior since ERR_PTR(0) and NULL
> > are equivalent. However, it improves code readability and silences static
> > analyzer warnings.
> >
> > Fixes: 88d5baf69082 ("Change inode_operations.mkdir to return struct dentry *")
> > Acked-by: Christian Schoenebeck <linux_oss@xxxxxxxxxxxxx>
> > Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
> >
> > ---
> > change inv1:
> > - Add clarification in commit message that this is not a functional
> >    change, as suggested by Christian Schoenebeck.
> > - Add acked-by.
> > ---
> > ---
> >   fs/9p/vfs_inode.c      | 5 ++---
> >   fs/9p/vfs_inode_dotl.c | 4 ++--
> >   2 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> > index d1508b1fe109..d2d18c796b58 100644
> > --- a/fs/9p/vfs_inode.c
> > +++ b/fs/9p/vfs_inode.c
> > @@ -672,13 +672,12 @@ v9fs_vfs_create(struct mnt_idmap *idmap, struct inode *dir,
> >   static struct dentry *v9fs_vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> >   				     struct dentry *dentry, umode_t mode)
> >   {
> > -	int err;
> > +	int err = 0;
> >   	u32 perm;
> >   	struct p9_fid *fid;
> >   	struct v9fs_session_info *v9ses;
> >   
> >   	p9_debug(P9_DEBUG_VFS, "name %pd\n", dentry);
> > -	err = 0;
> >   	v9ses = v9fs_inode2v9ses(dir);
> >   	perm = unixmode2p9mode(v9ses, mode | S_IFDIR);
> >   	fid = v9fs_create(v9ses, dir, dentry, NULL, perm, P9_OREAD);
> > @@ -692,7 +691,7 @@ static struct dentry *v9fs_vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
> >   
> >   	if (fid)
> >   		p9_fid_put(fid);
> > -	return ERR_PTR(err);
> > +	return err ? ERR_PTR(err) : NULL;
> >   }
> That function is odd, it ends:
> 	fid = v9fs_create(v9ses, dir, dentry, NULL, perm, P9_OREAD);
> 	if (IS_ERR(fid)) {
> 		err = PTR_ERR(fid);
> 		fid = NULL;
>
> A 'return' here would solve the mess later on.
>
> 	} else {
> 		inc_nlink(dir);
> 		v9fs_invalidate_inode_attr(dir);
> 	}
>
> Can v9fs_create() even return NULL?
> In any case p9_fid_put() already contains an (inline) check for both
> NULL and IS_ERR() so the 'if (fid)' and 'fid == NULL' (above) are pointless.
>
> 	if (fid)
> 		p9_fid_put(fid);
> 	return ERR_PTR(err);
>
> -- David
>
> >   
> >   /**
> > diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> > index 71796a89bcf4..83a52a85ce08 100644
> > --- a/fs/9p/vfs_inode_dotl.c
> > +++ b/fs/9p/vfs_inode_dotl.c
> > @@ -349,7 +349,7 @@ static struct dentry *v9fs_vfs_mkdir_dotl(struct mnt_idmap *idmap,
> >   					  struct inode *dir, struct dentry *dentry,
> >   					  umode_t omode)
> >   {
> > -	int err;
> > +	int err = 0;
> >   	struct v9fs_session_info *v9ses;
> >   	struct p9_fid *fid = NULL, *dfid = NULL;
> >   	kgid_t gid;
> > @@ -412,7 +412,7 @@ static struct dentry *v9fs_vfs_mkdir_dotl(struct mnt_idmap *idmap,
> >   	p9_fid_put(fid);
> >   	v9fs_put_acl(dacl, pacl);
> >   	p9_fid_put(dfid);
> > -	return ERR_PTR(err);
> > +	return err ? ERR_PTR(err) : NULL;
> >   }
> >   
> >   static int