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

On Thu, 15 Dec 2011 23:09:53 +0530 uma.shankar@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