[U-Boot] [PATCH 1/2] ext4fs ls load support

[Mike Frysinger <vapier@gentoo.org>]

On Thursday 15 December 2011 12:39:28 uma.shankar at samsung.com wrote:
> From: Uma Shankar <uma.shankar@samsung.com>

NAK: please add some sort of description here.  you must document all the 
places you're copying code from for example, and retain all the appropriate 
copyrights.  as it stands, this patch is a mess from that perspective.

>  Makefile              |    2 +-
>  common/Makefile       |    1 +
>  common/cmd_ext2.c     |    1 +
>  common/cmd_ext4.c     |  253 ++++++++++++++++++++++
>  fs/Makefile           |    1 +
>  fs/ext2/dev.c         |    1 +
>  fs/ext2/ext2fs.c      |  340 +++---------------------------
>  fs/ext4/Makefile      |   51 +++++
>  fs/ext4/ext4_common.c |  573
> fs/ext4/ext4_common.h |   44 ++++
>  fs/ext4/ext4fs.c      |  215 ++++++++++++++++++
>  include/ext2fs.h      |   16 +-
>  include/ext4fs.h      |  116 ++++++++++
>  include/ext_common.h  |  199 +++++++++++++++++

when moving code around, you should use the -M/-C flags so git can better 
describe what you did

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

static

> +{
> +	char *filename = NULL;

const

> +	switch (argc) {
> +	case 3:
> +		addr_str = getenv("loadaddr");
> +		if (addr_str != NULL)
> +			strict_strtoul(addr_str, 16, &addr);
> +		else
> +			addr = CONFIG_SYS_LOAD_ADDR;
> +
> +		filename = getenv("bootfile");
> +		count = 0;
> +		break;
> +	case 4:
> +		strict_strtoul(argv[3], 16, &addr);
> +		filename = getenv("bootfile");
> +		count = 0;
> +		break;
> +	case 5:
> +		strict_strtoul(argv[3], 16, &addr);
> +		filename = argv[4];
> +		count = 0;
> +		break;
> +	case 6:
> +		strict_strtoul(argv[3], 16, &addr);
> +		filename = argv[4];
> +		strict_strtoul(argv[5], 16, &count);
> +		break;
> +
> +	default:
> +		return cmd_usage(cmdtp);
> +	}

there's duplicated code here.  simpler to write it as:
	count = 0;
	filename = getenv("bootfile");
	addr_str = getenv("loadaddr");
	switch (argc) {
	case 6:
		strict_strtoul(argv[5], 16, &count);
	case 5:
		filename = argv[4];
	case 4:
		addr_str = argv[3];
	case 3:
		break;
	default:
		return cmd_usage(cmdtp);
	}
	if (addr_str)
		strict_strtoul(addr_str, 16, &addr);
	else
		addr = CONFIG_SYS_LOAD_ADDR;


> +		(int)strict_strtoul(++ep, 16, &part);

that cast doesn't make sense

> +		if (strncmp((char *)info.type, BOOT_PART_TYPE,
> +			    sizeof(info.type)) != 0) {

i don't see how this could possibly work.  info.type is "unsigned char", so 
casting that single byte into an address and then reading a single byte !?

> +			printf("** Invalid partition type \"%.32s\""
> +			       " (expect \"" BOOT_PART_TYPE "\")\n", info.type);

use %s instead of embeddeding a constant string

> +int do_ext4_ls(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])

static

> +{
> +	char *filename = "/";

const

> +		(int)strict_strtoul(++ep, 16, &part);

useless cast

> --- /dev/null
> +++ b/fs/ext4/Makefile
>
> +LIB	= $(obj)libext4fs.o
> +
> +AOBJS	=
> +COBJS-$(CONFIG_CMD_EXT4) := ext4fs.o ext4_common.o
> +
> +SRCS	:= $(AOBJS:.o=.S) $(COBJS-y:.o=.c)
> +OBJS	:= $(addprefix $(obj),$(AOBJS) $(COBJS-y))
> +
> +
> +all:	$(LIB) $(AOBJS)

drop the dead $(AOBJS) logic

> --- /dev/null
> +++ b/fs/ext4/ext4_common.c
>
> +#ifndef offsetof
> +#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
> +#endif

NAK: the linux/ headers already provide this

> +void *xmalloc(size_t size)
> +{
> +	void *ptr = malloc(size);
> +	if (ptr == NULL && size != 0)
> +		printf("bb_msg_memory_exhausted\n");
> +	return ptr;
> +}

NAK: this is useless, and clearly you're copying code from busybox.  you 
*must* document that.

> +void *xzalloc(size_t size)
> +{
> +	void *ptr = xmalloc(size);
> +	memset(ptr, 0, size);
> +	return ptr;
> +}

NAK: this doesn't fail (so the "x" doesn't make sense), and the zalloc() 
aspect should be in a common header written as:
	#define zalloc(size) calloc(1, size)

> +int ext4fs_mount(unsigned part_length)
> +{
> ...
> +#ifdef DEBUG
> +	printf("EXT2 rev %d, inode_size %d\n",
> +	       __le32_to_cpu(data->sblock.revision_level), fs->inodesz);
> +#endif

use debug()

> --- /dev/null
> +++ b/fs/ext4/ext4fs.c
>
> +/*
> + *  ext4load - based on code from GRUB2  fs/ext2.c
> +*/

you must retain copyright from the source files in the new files

> +int init_fs(block_dev_desc_t *dev_desc)
> +{
> ...
> +	fs = (struct ext_filesystem *)xzalloc(sizeof(struct ext_filesystem));

useless cast
-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/20111216/067b1576/attachment.pgp>