From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Frysinger Date: Fri, 16 Dec 2011 11:30:12 -0500 Subject: [U-Boot] [PATCH 1/2] ext4fs ls load support In-Reply-To: <1323970768-23834-1-git-send-email-uma.shankar@samsung.com> References: <1323970768-23834-1-git-send-email-uma.shankar@samsung.com> Message-ID: <201112161130.15094.vapier@gentoo.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Thursday 15 December 2011 12:39:28 uma.shankar at samsung.com wrote: > From: Uma Shankar 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: