[U-Boot] [PATCH 2/2] ext4fs write support

[Kim Phillips]

On Thu, 15 Dec 2011 23:09:53 +0530
<uma.shankar at samsung.com> wrote:

>  fs/ext4/crc16.c        |   62 +++
>  fs/ext4/crc16.h        |   16 +

there are already three crc16 implementations in u-boot according to
my count.  Please let's not add a fourth - hopefully the only thing
wrong with using lib/crc16.c was that you weren't aware it existed.

> +	/*get the filename */

> +	/*get the address in hexadecimal format (string to int) */

> +	/*get the filesize in base 10 format */

> +	/*set the device as block device */

> +	/*register the device and partition */

> +	/*get the partition information */

> +	/*mount the filesystem */

> +	/*start write */

space after the /*

> +	if (status)
> +		*ptr = *ptr & ~(operand);
> +	return;

> +	}
> +	return crc;

insert blank line before return statements

> +static int check_void_in_dentry(struct ext2_dirent *dir, char *filename)

> +	dentry_length = sizeof(struct ext2_dirent) +
> +	    dir->namelen + padding_factor;

alignment

> +	sizeof_void_space = dir->direntlen - dentry_length;
> +	if (sizeof_void_space == 0) {
> +		return 0;
> +	} else {
> +		padding_factor = 0;

if (sizeof_void_space == 0)
	return 0;

padding_factor = 0;
...

> +		new_entry_byte_reqd = strlen(filename) +
> +		    sizeof(struct ext2_dirent) + padding_factor;

alignment

> +		if (sizeof_void_space >= new_entry_byte_reqd) {
> +			dir->direntlen = dentry_length;
> +			return sizeof_void_space;
> +		} else
> +			return 0;

if (sizeof_void_space >= new_entry_byte_reqd) {
	dir->direntlen = dentry_length;
	return sizeof_void_space;
}

return 0;

> +	/*##START: Update the root directory entry block of root inode ### */
> +	/*since we are populating this file under  root
> +	 *directory take the root inode and its first block
> +	 *currently we are looking only in first block of root
> +	 *inode*/

/*
 * multi-line comments
 * are formatted
 * like this
 */


> +	zero_buffer = xzalloc(fs->blksz);
> +	if (!zero_buffer)
> +		return;

> +	root_first_block_buffer = (char *)xzalloc(fs->blksz);
> +	if (!root_first_block_buffer)
> +		return;

first time reviewing fs code, can this fn be modified to return an
error code?  If not, at least print something.

> +RESTART:

labels in lowercase

> +	status = ext2fs_devread(first_block_no_of_root
> +				* fs->sect_perblk,

operator at end of prior line

> +				0, fs->blksz, root_first_block_buffer);
> +	if (status == 0)
> +		goto fail;
> +	else {
> +		if (log_journal(root_first_block_buffer,

if (...)
	goto fail;

if (log_journal(...

this will save horizontal space for some tightly right-justified
code under it.

> +				last_entry_dirlen = dir->namelen +
> +				    sizeof(struct ext2_dirent) + padding_factor;

alignment

> +				if ((fs->blksz - totalbytes - last_entry_dirlen)
> +				    < new_entry_byte_reqd) {

operator on prior line

> +					printf("1st Block Full:"
> +					"allocating new block\n");

here,

> +					if (direct_blk_idx ==
> +					    INDIRECT_BLOCKS - 1) {
> +						printf("Directory capacity"
> +						"exceeds the limit\n");

here,

> +						goto fail;
> +					}
> +					g_parent_inode->b.blocks.dir_blocks
> +					    [direct_blk_idx] = get_new_blk_no();
> +					if (g_parent_inode->b.blocks.dir_blocks
> +					    [direct_blk_idx] == -1) {
> +						printf("no block"
> +							"left to assign\n");

and here, this prevents users trying to grep for this error text - I believe
it's allowed to exceed 80 columns in this case.

> +	/*END: Update the root directory entry block of root inode */

is some tool you're using supposed to correlate this with the above?:

/*##START: Update the root directory entry block of root inode ### */

If so, we (or certainly I) don't use it, and therefore don't care
for it - please remove it, and any other instances.

> +	/*get the first block of root */
> +	/*read the block no allocated to a file */

fix comment format

> +		status = ext2fs_devread(blknr * fs->sect_perblk,
> +					0, fs->blksz, (char *)block_buffer);
> +		if (status == 0)
> +			goto fail;
> +		else {

again, rm the else to avoid unnecessary indentation

> +			while (dir->direntlen >= 0) {
> +				/*blocksize-totalbytes because last directory
> +				 *length i.e.,*dir->direntlen is free availble
> +				 *space in the block that means
> +				 *it is a last entry of directory entry
> +				 */

fix comment format


> +	/*add root */
> +	arr[i] = xzalloc(strlen("/") + 1);
> +	if (!arr[i])
> +		return -1;

return -ENOMEM?

> +int iget(int inode_no, struct ext2_inode *inode)
> +{
> +	if (ext4fs_read_inode(ext4fs_root, inode_no, inode) == 0)
> +		return -1;
> +	else
> +		return 0;
> +}

rm else

> +fail:
> +	if (depth_dirname)
> +		free(depth_dirname);
> +	if (parse_dirname)
> +		free(parse_dirname);
> +	if (ptr)
> +		free(ptr);
> +	if (parent_inode)
> +		free(parent_inode);
> +	if (first_inode)
> +		free(first_inode);

free(0) is already protected for you

> +	/*single indirect */
> +	unsigned int *SI_buffer = NULL;
> +	long int SI_blockno;
> +	unsigned int *SI_start_addr = NULL;

no caps in variable names

> +	/*allocation of direct blocks */

this

> +	/*allocation of single indirect blocks */
> +	alloc_single_indirect_block(file_inode, &total_remaining_blocks,
> +				    &no_blks_reqd);
> +
> +	/*allocation of double indirect blocks */
> +	alloc_double_indirect_block(file_inode, &total_remaining_blocks,
> +				    &no_blks_reqd);
> +
> +	/*allocation of triple indirect blocks */
> +	alloc_triple_indirect_block(file_inode, &total_remaining_blocks,
> +				    &no_blks_reqd);

and these three comments don't seem to add any real value, given the
names of the functions.

> +	if (inode->b.blocks.tripple_indir_block != 0) {
> +		TIGP_buffer = (unsigned int *)xzalloc(fs->blksz);

s/tripple/triple/g

s/TIGP/tigp/g

also buy horizontal indentation space via

	if (inode->b.blocks.tripple_indir_block == 0)
		goto fail;
	TIGP_buffer = (unsigned int *)xzalloc(fs->blksz);
	...

> +				if (fs->blksz != 1024) {
> +					bg_idx = (*TIP_buffer) / (uint32_t)
> +					    ext4fs_root->sblock.
> +					    blocks_per_group;

also assign ext4fs_root->sblock.blocks_per_group to a function local
variable, to avoid tight right-justification like this.

This is a lot of code with recurring CodingStyle review patterns, so
I'm stopping my review here.  Please detect and fix the other
occurrences and resubmit.

Thank you for your contribution!

Kim