Re: [PATCH 1/8] fs/9p: Deletion of unnecessary checks before the function call "p9_client_clunk"

Julia Lawall
2014-12-28T21:01:28Z

> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 9ee5343..a787d4c 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -700,11 +700,9 @@ v9fs_create(struct v9fs_session_info *v9ses, struct inode *dir,
> }
> return ofid;
> error:
> - if (ofid)
> - p9_client_clunk(ofid);
> + p9_client_clunk(ofid);
>
> - if (fid)
> - p9_client_clunk(fid);
> + p9_client_clunk(fid);
>
> return ERR_PTR(err);
> }

This code seems rather sloppy. ofid could never be NULL at the point
where it was tested. There is a useless definition of ofid to NULL at the
top of the function. The error handling code could be rearranged with an
extra error label so that p9_client_clunk is never called on fid when it
it is not defined. The the two assignments of fid to NULL could also be
removed. There is also a useless initialization of err to 0 at the
beginning of the function. And the first two calls to ERR_PTR are not
needed, as the types of dfid and ofid are the same as the return type of
the function.

> @@ -768,8 +766,7 @@ static int v9fs_vfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode
> v9fs_invalidate_inode_attr(dir);
> }
>
> - if (fid)
> - p9_client_clunk(fid);
> + p9_client_clunk(fid);
>
> return err;
> }

This function is also unnecessarily complex. The if above this one, that
detects the error could just return PTR_ERR(fid);. The rest of the
function could call inc_nlink, v9fs_invalidate_inode_attr, and
p9_client_clunk, and the returnvalue could be 0, removing the need for the
err variable.

> @@ -916,8 +913,7 @@ out:
> return err;
>
> error:
> - if (fid)
> - p9_client_clunk(fid);
> + p9_client_clunk(fid);
> goto out;
> }

In the case where fid is set to NULL, it could just goto out directly,
instead of calling p9_client_clunk, testing fid, coming back, and going to
out anyway.

julia

> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index 6054c16b..3611b0f 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -363,11 +363,9 @@ out:
> return err;
>
> error:
> - if (fid)
> - p9_client_clunk(fid);
> + p9_client_clunk(fid);
> err_clunk_old_fid:
> - if (ofid)
> - p9_client_clunk(ofid);
> + p9_client_clunk(ofid);
> goto out;
> }
>
> @@ -464,8 +462,7 @@ static int v9fs_vfs_mkdir_dotl(struct inode *dir,
> inc_nlink(dir);
> v9fs_invalidate_inode_attr(dir);
> error:
> - if (fid)
> - p9_client_clunk(fid);
> + p9_client_clunk(fid);
> v9fs_put_acl(dacl, pacl);
> return err;
> }
> @@ -744,8 +741,7 @@ v9fs_vfs_symlink_dotl(struct inode *dir, struct dentry *dentry,
> }
>
> error:
> - if (fid)
> - p9_client_clunk(fid);
> + p9_client_clunk(fid);
>
> return err;
> }
> @@ -896,8 +892,7 @@ v9fs_vfs_mknod_dotl(struct inode *dir, struct dentry *dentry, umode_t omode,
> d_instantiate(dentry, inode);
> }
> error:
> - if (fid)
> - p9_client_clunk(fid);
> + p9_client_clunk(fid);
> v9fs_put_acl(dacl, pacl);
> return err;
> }
> diff --git a/fs/9p/xattr.c b/fs/9p/xattr.c
> index f95e01e..8e72269 100644
> --- a/fs/9p/xattr.c
> +++ b/fs/9p/xattr.c
> @@ -65,8 +65,7 @@ ssize_t v9fs_fid_xattr_get(struct p9_fid *fid, const char *name,
> /* Total read xattr bytes */
> retval = offset;
> error:
> - if (attr_fid)
> - p9_client_clunk(attr_fid);
> + p9_client_clunk(attr_fid);
> return retval;
>
> }
> --
> 2.2.1
>
>