linux-ext4 - Re: [PATCH 11/12] ext4: simplify parameters of add_new

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: <0A64161B-0BC0-4F64-86EB-50FBBCD7A7AF@dilger.ca>
Date:	Mon, 18 Jul 2011 00:46:02 -0600
From:	Andreas Dilger <adilger@...ger.ca>
To:	Yongqiang Yang <xiaoqiangnk@...il.com>
Cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH 11/12] ext4: simplify parameters of add_new_gdb()

On 2011-07-17, at 8:52 PM, Yongqiang Yang wrote:
> add_new_gdb() only needs the no. of a group, there is no need to pass
> a pointer to struct ext4_new_group_data to add_new_gdb().
> 
> static int add_new_gdb(handle_t *handle, struct inode *inode,
> -		       struct ext4_new_group_data *input,
> -		       struct buffer_head **primary)
> +		       ext4_group_t group)

This patch is changing the code in a subtle, but not incorrect way.
At a minimum, the commit comment should also mention why "primary"
can be removed as an argument.  That is because add_new_gdb() is
storing the bh pointer for the new group descriptor block into
s_group_desc[] internally:

> @@ -513,7 +514,7 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
> 	o_group_desc = EXT4_SB(sb)->s_group_desc;
> 	memcpy(n_group_desc, o_group_desc,
> 	       EXT4_SB(sb)->s_gdb_count * sizeof(struct buffer_head *));
> -	n_group_desc[gdb_num] = *primary;
> +	n_group_desc[gdb_num] = gdb_bh;

Here...

> @@ -843,8 +844,12 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input)
> 		if (reserved_gdb && ext4_bg_num_gdb(sb, input->group) &&
> 		    (err = reserve_backup_gdb(handle, inode, input)))
> 			goto exit_journal;
> -	} else if ((err = add_new_gdb(handle, inode, input, &primary)))
> -		goto exit_journal;
> +	} else {
> +		err = add_new_gdb(handle, inode, input->group);
> +		if (err)
> +			goto exit_journal;
> +		primary = sbi->s_group_desc[gdb_num];

and the caller can safely access this after the function returns.  It
would be incorrect to "optimize" this code to consolidate the setting
of "primary" from sbi->s_group_desc[gdb_num] before the if/else clause,
so this should get a comment that this array element is only valid after
add_new_gdb() returns.

Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html