From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kim Phillips Date: Thu, 15 Dec 2011 16:20:14 -0600 Subject: [U-Boot] [PATCH 2/2] ext4fs write support In-Reply-To: <1323970793-23858-1-git-send-email-uma.shankar@samsung.com> References: <1323970793-23858-1-git-send-email-uma.shankar@samsung.com> Message-ID: <20111215162014.4f17b163760212e441925cf7@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Thu, 15 Dec 2011 23:09:53 +0530 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