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

[Stephen Warren]

On 10/30/2012 03:59 PM, Benoît Thébaudeau wrote:
> On Tuesday, October 30, 2012 10:18:03 PM, Stephen Warren wrote:
>> On 10/30/2012 02:23 PM, Benoît Thébaudeau wrote:
>>>> +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.
> 
> My comment was misleading. Actually, with the current code, do_ls() can not be
> called (except directly) if there are more than 4 arguments, because of the way
> the ls command is declared through U_BOOT_CMD(). Hence, if ">=" is used,
> arguments can be added later without changing existing lines.

Ah OK, that makes sense.

> And if we consider a direct call to do_ls() skipping the command system, then
> this function should return CMD_RET_USAGE if argc > 4.

True.