On Thursday 15 December 2011 12:39:28 uma.shankar@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);elseaddr = 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