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

[Stephen Warren]

On 10/30/2012 02:23 PM, Benoît Thébaudeau wrote:
> 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.

>> diff --git a/fs/fs.c b/fs/fs.c

>> +int do_fsload(cmd_tbl_t *cmdtp, int flag, int argc, char * const

>> +		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?

Oh yes, that is a problem.

I think we should use 0 here for the new commands at least; it has
always annoyed me that U-Boot assumes that clearly non-hex values (since
there is no 0x) are actually hex. I often have to manually convert
values because of this (e.g. host-based ls decimal output to U-Boot
command input expecting hex without leading 0x.)

However, we do need to fix this for the existing legacy commands. I
suggest we either:

a) Add a new parameter to do_fsload() indicating which base to use.

or:

b) Assume base 0 if fstype==FS_TYPE_ANY, otherwise use 16.

(a) is probably cleaner.

For the environment variables, I suppose we need to continue to
explicitly request base 16 conversion, since those variables could be
used with either the old or the new commands.

>> +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] : "/"))

Here I don't agree. If the command expects a certain set of arguments,
we should validate that the user provided exactly that set, and no more.
If we allow arbitrary cruft, then if we need to add new arguments later,
we won't be able to guarantee that handling those extra arguments won't
break some existing broken usage of the command.