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

[Mike Frysinger <vapier@gentoo.org>]

On Tuesday 27 December 2011 21:23:27 uma.shankar at samsung.com wrote:
> From: Uma Shankar <uma.shankar@samsung.com>
> 
> Signed-off-by: Uma Shankar <uma.shankar@samsung.com>
> Signed-off-by: Manjunatha C Achar <a.manjunatha@samsung.com>
> Signed-off-by: Iqbal Shareef <iqbal.ams@samsung.com>
> Signed-off-by: Hakgoo Lee <goodguy.lee@samsung.com>

you need to note the sources of these files in the changelog

> --- a/common/cmd_ext4.c
> +++ b/common/cmd_ext4.c
>  
> +int do_ext4_write(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])

static

> +	char *filename = "/";

const

> +	int dev = 0;

considering you set dev first below, no point in setting this to 0

> +	/*get the filename */

many of the comments here are missing a space after the "/*"

> --- /dev/null
> +++ b/fs/ext4/README

docs belong in docs/

> --- /dev/null
> +++ b/fs/ext4/crc16.c
>
> +/*
> + *      crc16.c
> + *
> + * This source code is licensed under the GNU General Public License,
> + * Version 2. See the file COPYING for more details.
> + */

why can't you use the existing lib/crc16.c code ?

> --- a/fs/ext4/ext4_common.c
> +++ b/fs/ext4/ext4_common.c
> 
> +#if defined(CONFIG_CMD_EXT4_WRITE)
> +#include "ext4_journal.h"
> +#include "crc16.h"
> +#else
>  #include "ext4_common.h"
> +#endif

conditional header includes are a bad idea

> +/* To convert big endian journal superblock entries to little endian */
> +unsigned int be_le(unsigned int num)
> ...
> +/* 4-byte number */
> +unsigned int le_be(unsigned int num)

we already have swap functions in include/ you should be able to use instead 
of duplicating them

> +void reset_inode_bmap(int inode_no, unsigned char *buffer, int index)
> ...
> +
> +	return;
> +}

useless return -> delete it

> --- a/fs/ext4/ext4_common.h
> +++ b/fs/ext4/ext4_common.h
> 
> +int get_parent_inode_num(char *dirname, char *dname, int flags);
> +void update_parent_dentry(char *filename, int *p_ino, int file_type);
> +long int get_new_blk_no(void);
> +int get_new_inode_no(void);
> +void reset_block_bmap(long int blockno, unsigned char *buffer, int index);
> +int set_block_bmap(long int blockno, unsigned char *buffer, int index);
> +int set_inode_bmap(int inode_no, unsigned char *buffer, int index);
> +void reset_inode_bmap(int inode_no, unsigned char *buffer, int index);
> +int iget(int inode_no, struct ext2_inode *inode);
> +void allocate_blocks(struct ext2_inode *file_inode,
> +		     unsigned int total_remaining_blocks,
> +		     unsigned int *total_no_of_block);
> +void put_ext4(uint64_t off, void *buf, uint32_t size);
>
> --- /dev/null
> +++ b/fs/ext4/ext4_journal.h
> 
> +int init_journal(void);
> +void deinit_journal(void);
> +int log_gdt(char *gd_table);
> +void update_journal(void);
> +int check_journal_state(int recovery_flag);
> +int log_journal(char *journal_buffer, long int blknr);
> +int put_metadata(char *metadata_buffer, long int blknr);
> +void dump_metadata(void);
> +void free_journal(void);
> +void free_revoke_blks(void);
> +void push_revoke_blk(char *buffer);

these are a lot of bad names.  these need to be properly namespaced if you're 
going to export them.

> --- /dev/null
> +++ b/fs/ext4/ext4_journal.c
>
> +		journal_ptr[i] = (struct journal_log *)zalloc
> +		    (sizeof(struct journal_log));
> ...
> +		dirty_block_ptr[i] = (struct dirty_blocks *)
> +		    zalloc(sizeof(struct dirty_blocks));
> ...
> +		journal_ptr[gindex]->buf = (char *)zalloc(fs->blksz);
> ...
> +		journal_ptr[gindex]->buf = (char *)zalloc(fs->blksz);

useless casts.  zalloc() returns void *.  there's a bunch more throughout this 
patch.

> +void free_journal(void)
> +{
> ...
> +
> +	return;
> +}

useless return

> +void print_revoke_blks(char *revk_blk)
> +{
> ...
> +
> +	return;
> +}

useless return

> +static struct revoke_blk_list *_get_node(void)
> +{
> ...
> +
> +	return;
> +}

useless return

> +void free_revoke_blks()

needs (void)

> +	return;
> +}

useless return

> +void print_jrnl_status(int recovery_flag)
> +{
> ...
> +
> +	return;
> +}

useless return

> --- a/fs/ext4/ext4fs.c
> +++ b/fs/ext4/ext4fs.c
> 
> +static void delete_single_indirect_block(struct ext2_inode *inode)
> +{
> ...
> +
> +	return;
> +
> +}

useless return

> +static void delete_double_indirect_block(struct ext2_inode *inode)
> +{
> ...
> +
> +	return;
> +}

useless return

> +static void delete_triple_indirect_block(struct ext2_inode *inode)
> +{
> ...
> +
> +	return;
> +}

useless return

> +void ext4fs_deinit(void)
> +{
> ...
> +
> +	return;
> +}

useless return
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120107/11d75ffc/attachment.pgp>