Re: [PATCH 2/2] Add support to RENAME_EXCHANGE flag

[Eric Sandeen <sandeen@sandeen.net>]

On 11/10/14 11:41 AM, Carlos Maiolino wrote:
> Adds a new function named xfs_cross_rename(), responsible to handle requests
> from sys_renameat2() using RENAME_EXCHANGE flag.
> 
> Changelog:
> 
> 	V2: refactor xfs_cross_rename() to not duplicate code from xfs_rename()

Which tree is this against?

> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 315 +++++++++++++++++++++++++++++++++++------------------
>  fs/xfs/xfs_inode.h |   8 +-
>  fs/xfs/xfs_iops.c  |   4 +-
>  3 files changed, 220 insertions(+), 107 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index fea3c92..bf09bfc 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2674,12 +2674,14 @@ xfs_rename(
>  	xfs_inode_t	*src_ip,
>  	xfs_inode_t	*target_dp,
>  	struct xfs_name	*target_name,
> -	xfs_inode_t	*target_ip)
> +	xfs_inode_t	*target_ip,
> +	unsigned int	flags)
>  {
>  	xfs_trans_t	*tp = NULL;
>  	xfs_mount_t	*mp = src_dp->i_mount;
>  	int		new_parent;		/* moving to a new dir */
>  	int		src_is_directory;	/* src_name is a directory */
> +	int		tgt_is_directory;
>  	int		error;
>  	xfs_bmap_free_t free_list;
>  	xfs_fsblock_t   first_block;
> @@ -2752,141 +2754,158 @@ xfs_rename(
>  	}
>  
>  	/*
> -	 * Set up the target.
> -	 */
> -	if (target_ip == NULL) {
> +	 * Handle RENAME_EXCHANGE flags
> +	*/
> +	if (flags & RENAME_EXCHANGE) {
>  		/*
> -		 * If there's no space reservation, check the entry will
> -		 * fit before actually inserting it.
> +		 * target_ip will always exist if RENAME_EXCHANGE flag is set
>  		 */
> -		error = xfs_dir_canenter(tp, target_dp, target_name, spaceres);
> +		tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
> +
> +		error = xfs_cross_rename(src_dp, src_name, src_ip, target_dp, target_name, target_ip,
> +					 new_parent, src_is_directory, tgt_is_directory,
> +					 &free_list, &first_block, tp, spaceres);

Ok, just some style drive-by comments, first -

EEK ;)  13 args and ~100 chars wide ;)

You could at least move some of those back to local vars in the
cross_rename function so you don't have to pass them on the stack.
It's also customary to pass the transaction pointer as the first arg,
i.e.:

+		error = xfs_cross_rename(tp, src_dp, src_name, src_ip,
+					 target_dp, target_name, target_ip,
+					 &free_list, &first_block, spaceres);

and:

xfs_cross_rename()
{
	...
	new_parent = (src_dp != target_dp);
	src_is_directory = S_ISDIR(src_ip->i_d.di_mode);
	tgt_is_directory = S_ISDIR(target_ip->i_d.di_mode);
	...

}

There are a few other places where you go well beyond 80 chars, would rather
not do that if at all possible.

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs