[U-Boot] [PATCH V4 3/3] fs: add filesystem switch libary, implement ls and fsload commands

[Benoît Thébaudeau]

Hi Stephen,

On Monday, October 22, 2012 6:43:51 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren at nvidia.com>
> 
> Implement "ls" and "fsload" commands that act like
> {fat,ext2}{ls,load},
> and transparently handle either file-system. This scheme could easily
> be
> extended to other filesystem types; I only didn't do it for zfs
> because
> I don't have any filesystems of that type to test with.
> 
> Replace the implementation of {fat,ext[24]}{ls,load} with this new
> code
> too.
> 
> Signed-off-by: Stephen Warren <swarren at nvidia.com>

Sorry to come after this has been applied. I've been ultra busy lately.

[--snip--]
> diff --git a/fs/fs.c b/fs/fs.c
> new file mode 100644
> index 0000000..23ffa25
> --- /dev/null
> +++ b/fs/fs.c
> @@ -0,0 +1,308 @@
[--snip--]
> +int do_fsload(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> argv[],
> +		int fstype)
> +{
> +	unsigned long addr;
> +	const char *addr_str;
> +	const char *filename;
> +	unsigned long bytes;
> +	unsigned long pos;
> +	int len_read;
> +	char buf[12];
> +
> +	if (argc < 5)

With the arguments now made optional, this should rather be:
+	if (argc < 2)

> +		return CMD_RET_USAGE;
> +
> +	if (fs_set_blk_dev(argv[1], argv[2], fstype))

With <dev[:part]> being optional, this should rather be:
+	if (fs_set_blk_dev(argv[1], (argc >= 3) ? argv[2] : NULL, fstype))

> +		return 1;
> +
> +	if (argc >= 4) {
> +		addr = simple_strtoul(argv[3], NULL, 0);

0 is just natural here. However, this raises the issue of the users of the
legacy fat and ext commands, which used 16 here. So should we use 0 because it
is cleaner, or 16 in order not to break compatibility for existing users?

> +	} else {
> +		addr_str = getenv("loadaddr");
> +		if (addr_str != NULL)
> +			addr = simple_strtoul(addr_str, NULL, 16);

Ditto.

> +		else
> +			addr = CONFIG_SYS_LOAD_ADDR;
> +	}
> +	if (argc >= 5) {
> +		filename = argv[4];
> +	} else {
> +		filename = getenv("bootfile");
> +		if (!filename) {
> +			puts("** No boot file defined **\n");
> +			return 1;
> +		}
> +	}
> +	if (argc >= 6)
> +		bytes = simple_strtoul(argv[5], NULL, 0);

Ditto.

> +	else
> +		bytes = 0;
> +	if (argc >= 7)
> +		pos = simple_strtoul(argv[6], NULL, 0);

Ditto.

> +	else
> +		pos = 0;
> +
> +	len_read = fs_read(filename, addr, pos, bytes);
> +	if (len_read <= 0)
> +		return 1;
> +
> +	printf("%d bytes read\n", len_read);
> +
> +	sprintf(buf, "0x%x", len_read);
> +	setenv("filesize", buf);
> +
> +	return 0;
> +}
> +
> +int do_ls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
> +	int fstype)
> +{
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	if (fs_set_blk_dev(argv[1], (argc >= 3) ? argv[2] : NULL, fstype))
> +		return 1;
> +
> +	if (fs_ls(argc == 4 ? argv[3] : "/"))

IMHO, it would be better to just ignore the possible extra arguments, like in:
+	if (fs_ls(argc >= 4 ? argv[3] : "/"))

> +		return 1;
> +
> +	return 0;
> +}
[--snip--]

Best regards,
Benoît