fs: delete unused Makefile

From: Stephen Warren <[email protected]>

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 <[email protected]>

---

fs/fs.c new file +0 -0

new file mode 100644
index 0000000..23ffa25
@@ -0,0 +1,308 @@

fs/fs.c new file +4 -0

+
+static const struct {
+	int fstype;
+	int (*probe)(void);

fs/fs.c new file +13 -0

+
+int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype)
+{
+	int part, i;
+
+	part = get_device_and_partition(ifname, dev_part_str, &fs_dev_desc,
+					&fs_partition, 1);
+	if (part < 0)
+		return -1;
+
+	for (i = 0; i < ARRAY_SIZE(fstypes); i++) {
+		if ((fstype != FS_TYPE_ANY) && (fstype != fstypes[i].fstype))
+			continue;

Dear all,

Quoted 10 lines · Stephen Warren

From: Stephen Warren <[email protected]>

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.

Quoted 1 line

Signed-off-by Stephen Warren <[email protected]>

this patch (namely 045fa1e1142552799ad3203e9e0bc22a11e866ea) seems to break avr32 on runtime. It seems there is a new array introduced (the fstypes array in fs/fs.c) which do not provide a relocation method (CONFIG_NEEDS_MANUAL_RELOC). This is currently only a weak assumption, but has any other requiring manual relocation m86k, mips, nds32m sparc) also encountered this problem?

Commit 045fa1e1142552799ad3203e9e0bc22a11e866ea introduce an array with filesystem accessors. On arches which need manual reloc this is broken cause the function pointers still pointing to the privious location, fix it.

Quoted 3 lines · Andreas Bie??mann

Commit 045fa1e1142552799ad3203e9e0bc22a11e866ea introduce an array with filesystem accessors. On arches which need manual reloc this is broken cause the function pointers still pointing to the privious location, fix it.

We found the same code to copy:-)

Quoted 1 line

fs/fs.c +0 -0

fs/fs.c static const struct { · line 164

Quoted 4 lines

int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype)

fs/fs.c +2 -0

+#ifdef CONFIG_NEEDS_MANUAL_RELOC
+	static int relocated = 0;

checkpatch bitches about the "= 0" there. I assume BSS initialization isn't also broken when CONFIG_NEEDS_MANUAL_RELOC is enabled?

Quoted 3 lines · Andreas Bie?mann

Commit 045fa1e1142552799ad3203e9e0bc22a11e866ea introduce an array with filesystem accessors. On arches which need manual reloc this is broken cause the function pointers still pointing to the privious location, fix it.

We found the same code to copy:-)

fs/fs.c static const struct { · line 164

Quoted 6 lines

fs/fs.c +2 -0

 
 int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype)
 
+#ifdef CONFIG_NEEDS_MANUAL_RELOC
+	static int relocated = 0;

checkpatch bitches about the "= 0" there. I assume BSS initialization isn't also broken when CONFIG_NEEDS_MANUAL_RELOC is enabled?

From: Stephen Warren <[email protected]>

Without this, fstypes[].probe points at the wrong place, so calling the function results in undefined behaviour.

Signed-off-by Stephen Warren <[email protected]>

---
Note: I have compile-tested this with the relocation code forcibly enabled, but have no way of actually testing the result.
---

Dear Stephen Warren,

Quoted 2 lines · Stephen Warren

From: Stephen Warren <[email protected]>

Quoted 4 lines

Without this, fstypes[].probe points at the wrong place, so calling the function results in undefined behaviour.

Signed-off-by Stephen Warren <[email protected]>

Tested on AVR32 (atstk1002)

Tested-by Andreas Bie?mann <[email protected]>

From: Stephen Warren <[email protected]>

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 <[email protected]>

fs/fs.c new file +4 -0

+		else
+			addr = CONFIG_SYS_LOAD_ADDR;
+	}
+	if (argc >= 5) {

fs/fs.c new file +15 -0

+	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;
+}
+

Hi Stephen,

Quoted 17 lines · Stephen Warren

From: Stephen Warren <[email protected]>

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.

fs/fs.c +1 -0

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

fs/fs.c line 257

Quoted 4 lines

fs/fs.c +4 -0

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

Quoted 19 lines · Beno?t Th?baudeau

Hi Stephen,

Quoted 15 lines · Stephen Warren

From: Stephen Warren <[email protected]>

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.

fs/fs.c +1 -0

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

const

Quoted 7 lines

Quoted 2 lines

fs/fs.c +1 -0

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

Quoted 15 lines · Beno?t Th?baudeau

fs/fs.c line 304

Quoted 12 lines

+int do_ls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],

fs/fs.c new file +6 -0

+	int fstype)
+{
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	if (fs_set_blk_dev(argv[1], (argc >= 3) ? argv[2] : NULL,

fstype))

fs/fs.c new file +3 -0

+		return 1;
+
+	if (fs_ls(argc == 4 ? argv[3] : "/"))

IMHO, it would be better to just ignore the possible extra arguments, like in:

Review

Quoted 1 line

(quoted diff) +1 -0

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

Quoted 19 lines · Stephen Warren

Quoted 15 lines · Beno?t Th?baudeau

fs/fs.c line 304

Quoted 12 lines

+int do_ls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],

fs/fs.c new file +6 -0

+	int fstype)
+{
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	if (fs_set_blk_dev(argv[1], (argc >= 3) ? argv[2] : NULL,

fstype))

fs/fs.c new file +3 -0

+		return 1;
+
+	if (fs_ls(argc == 4 ? argv[3] : "/"))

IMHO, it would be better to just ignore the possible extra arguments, like in:

Review

Quoted 1 line

(quoted diff) +1 -0

+	if (fs_ls(argc >= 4 ? argv[3] : "/"))

Here I don't agree. If the command expects a certain set of

Quoted 8 lines

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.

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.

From: Stephen Warren <[email protected]>

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 <[email protected]>

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

common/cmd_fs.c new file +19 -0

new file mode 100644
index 0000000..296124b
@@ -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>

Dear Stephen Warren,

Quoted 51 lines · Stephen Warren

From: Stephen Warren <[email protected]>

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 <[email protected]>

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

Quoted 25 lines

common/cmd_fs.c new file +19 -0

new file mode 100644
index 0000000..296124b
@@ -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>

common/cmd_fs.c line 29

Quoted 10 lines

common/cmd_fs.c new file +10 -0

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

From: Stephen Warren <[email protected]>

fs/Makefile is unused. The top-level Makefile sets LIBS-y += fs/xxx and hence causes make to directly descend two directory levels into each individual filesystem, and it never descends into fs/ itself.

So, delete this useless file.

Signed-off-by Stephen Warren <[email protected]>

Reviewed-by Beno??t Th??baudeau <[email protected]>

Acked-by Simon Glass <[email protected]>