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

[Stephen Warren]

On 10/31/2012 04:43 AM, Andreas Bießmann wrote:
> Dear Stephen Warren,
> 
> On 22.10.2012 18:43, 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>
>> ---
>> v4:
>> * Use FS_TYPE_* everywhere, rather than separate FS_* and FS_TYPE_*.
>> * Make fs_set_blk_dev() loop over a table of filesystems.
>> v3:
>> * Updated the implementation of the new commands to be suitable for use
>>   as the body of {fat,ext[24]}{ls,load} too.
>> * Enhanced the implementation to make more fsload parameters optional
>>   (load address, filename); they can now come from environment variables
>>   like ext2load supported.
>> * Moved implementation into fs.c.
>> * Removed cmd_ext_common.c.
>> * s/partition/filesystem/ in patch subject.
>> v2:
>> * Build cmd_fs.c when CONFIG_CMD_FS_GENERIC not always.
>> * Use new CONFIG_FS_{FAT,EXT4} rather than CONFIG_CMD_{FAT,EXT2}.
>> * Implemented fs_close() and call it from the tail of fs_ls() and fs_read().
>>   This ensures that any other usage of e.g. the ext4 library between fs_*()
>>   calls, then the two usages won't interfere.
>> * Re-organized fs.c to reduce the number of ifdef blocks.
>> * Added argc checking to do_ls().
>> ---
>>  Makefile                |    3 +-
>>  common/Makefile         |    6 +-
>>  common/cmd_ext2.c       |   13 +--
>>  common/cmd_ext4.c       |   12 +--
>>  common/cmd_ext_common.c |  197 ------------------------------
>>  common/cmd_fat.c        |   76 +-----------
>>  common/cmd_fs.c         |   51 ++++++++
>>  fs/Makefile             |   47 +++++++
>>  fs/fs.c                 |  308 +++++++++++++++++++++++++++++++++++++++++++++++
>>  include/ext_common.h    |    3 -
>>  include/fs.h            |   65 ++++++++++
>>  11 files changed, 483 insertions(+), 298 deletions(-)
>>  delete mode 100644 common/cmd_ext_common.c
>>  create mode 100644 common/cmd_fs.c
>>  create mode 100644 fs/Makefile
>>  create mode 100644 fs/fs.c
>>  create mode 100644 include/fs.h
>>
> 
> <snip>
> 
>> diff --git a/common/cmd_fs.c b/common/cmd_fs.c
>> new file mode 100644
>> index 0000000..296124b
>> --- /dev/null
>> +++ b/common/cmd_fs.c
>> @@ -0,0 +1,51 @@
>> +/*
>> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
>> + *
>> + * Inspired by cmd_ext_common.c, cmd_fat.c.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <common.h>
>> +#include <command.h>
>> +#include <fs.h>
>> +
>> +int do_fsload_wrapper(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> +{
>> +	return do_fsload(cmdtp, flag, argc, argv, FS_TYPE_ANY);
>> +}
>> +
>> +U_BOOT_CMD(
>> +	fsload,	7,	0,	do_fsload_wrapper,
> 
> I just realized that cmd_jffs2.c also defines a U_BOOT_CMD fsload ...
> this may also be a problem for users of the old command!
> 
> However if one defines CONFIG_CMD_JFFS2 _and_ CONFIG_CMD_FS_GENERIC the
> linker will throw following warning:
> 
> ---8<---
> /tmp/build_avr32/common/cmd_jffs2.o:(.u_boot_list.cmd.fsload+0x0):
> multiple definition of `_u_boot_list_cmd_fsload'
> /tmp/build_avr32/common/cmd_fs.o:(.u_boot_list.cmd.fsload+0x0): first
> defined here
> /tmp/build_avr32/common/cmd_jffs2.o:(.u_boot_list.cmd.ls+0x0): multiple
> definition of `_u_boot_list_cmd_ls'
> /tmp/build_avr32/common/cmd_fs.o:(.u_boot_list.cmd.ls+0x0): first
> defined here
> --->8---
> 
> Was this intended?

No, this is certainly a problem. I guess we need to rename the new command.

I had originally thought about calling it plain "load" rather than
"fsload" (just like it's "ls" not "fsls"). However, that seemed far too
generic. However, given this conflict, and the fact that I don't think
there's any existing "load" command, perhaps we have no choice but to
s/fsload/load/, unless anyone has any better ideas? "fileload"?