linux-kernel - Re: [PATCH 08/11] ext4: rename: create ext4

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131126080436.GA1384@quack.suse.cz>
Date:	Tue, 26 Nov 2013 09:04:36 +0100
From:	Jan Kara <jack@...e.cz>
To:	Miklos Szeredi <miklos@...redi.hu>
Cc:	viro@...IV.linux.org.uk, torvalds@...ux-foundation.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	hch@...radead.org, akpm@...ux-foundation.org, dhowells@...hat.com,
	zab@...hat.com, jack@...e.cz, luto@...capital.net, mszeredi@...e.cz
Subject: Re: [PATCH 08/11] ext4: rename: create ext4_renament structure for
 local vars

On Wed 20-11-13 14:01:49, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@...e.cz>
> 
> Need to split up ext4_rename() into helpers but there are two many local
                                                            ^^ too

> variables involved, so create a new structure.  This also, apparently,
> makes the generated code size slightly smaller.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@...e.cz>
> ---
>  fs/ext4/namei.c | 211 ++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 114 insertions(+), 97 deletions(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index e0129b6e74cf..c9e38bd30e8b 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3001,6 +3001,22 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle,
>  	return ext4_get_first_inline_block(inode, parent_de, retval);
>  }
>  
> +struct ext4_renament {
> +	struct inode *dir;
> +	struct dentry *dentry;
> +	struct inode *inode;
> +
> +	/* entry for "dentry" */
> +	struct buffer_head *bh;
> +	struct ext4_dir_entry_2 *de;
> +	int inlined;
> +
> +	/* entry for parent block of inode if it's a directory */
  Maybe "entry for '..' in inode if it is a directory"? It looks more
comprehensible to me.

> +	struct buffer_head *dir_bh;
> +	struct ext4_dir_entry_2 *parent_de;
> +	int dir_inlined;
> +};
> +
>  /*
>   * Anybody can rename anything with this: the permission checks are left to the
>   * higher-level routines.
> @@ -3014,196 +3030,197 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry,
>  		       unsigned int flags)
>  {
>  	handle_t *handle = NULL;
> -	struct inode *old_inode, *new_inode;
> -	struct buffer_head *old_bh, *new_bh, *dir_bh;
> -	struct ext4_dir_entry_2 *old_de, *new_de;
> +	struct ext4_renament old = {
> +		.dir = old_dir,
> +		.dentry = old_dentry,
> +	};
> +	struct ext4_renament new = {
> +		.dir = new_dir,
> +		.dentry = new_dentry,
> +	};
  The code would look slightly more logical if we initialized .inode as
well. Not sure if it is worth an extra patch or if you just fold it here...

  Otherwise the patch looks good. You can add:
Reviewed-by: Jan Kara <jack@...e.cz>

								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/