[U-Boot] [PATCH V2 1/5] fs: fix generic save command implementation

[U-Boot] [PATCH V2 1/5] fs: fix generic save command implementation

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Fix a few issues with the generic "save" shell command, and fs_write()
function.

1) fstypes[].write wasn't filled in for some file-systems, and isn't
   checked when used, which could cause crashes/... if executing save
   on e.g. fat/ext filesystems.

2) fs_write() requires the length argument to be non-zero, since it needs
   to know exactly how many bytes to write. Adjust the comments and code
   according to this.

3) fs_write() wasn't prototyped in <fs.h> like other generic functions;
   other code should be able to call this directly rather than invoking
   the "save" shell command.

Cc: Simon Glass <sjg@chromium.org>
Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 fs/fs.c      |  9 +++------
 include/fs.h | 10 ++++++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/fs/fs.c b/fs/fs.c
index be1855d1291f..9c2ef6b6597c 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -75,6 +75,7 @@ static struct fstype_info fstypes[] = {
 		.close = fat_close,
 		.ls = file_fat_ls,
 		.read = fat_read_file,
+		.write = fs_write_unsupported,
 	},
 #endif
 #ifdef CONFIG_FS_EXT4
@@ -84,6 +85,7 @@ static struct fstype_info fstypes[] = {
 		.close = ext4fs_close,
 		.ls = ext4fs_ls,
 		.read = ext4_read_file,
+		.write = fs_write_unsupported,
 	},
 #endif
 #ifdef CONFIG_SANDBOX
@@ -212,16 +214,11 @@ int fs_write(const char *filename, ulong addr, int offset, int len)
 	void *buf;
 	int ret;
 
-	/*
-	 * We don't actually know how many bytes are being read, since len==0
-	 * means read the whole file.
-	 */
 	buf = map_sysmem(addr, len);
 	ret = info->write(filename, buf, offset, len);
 	unmap_sysmem(buf);
 
-	/* If we requested a specific number of bytes, check we got it */
-	if (ret >= 0 && len && ret != len) {
+	if (ret >= 0 && ret != len) {
 		printf("** Unable to write file %s **\n", filename);
 		ret = -1;
 	}
diff --git a/include/fs.h b/include/fs.h
index 7d9403ed8758..97b0094e954b 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -55,6 +55,16 @@ int fs_ls(const char *dirname);
 int fs_read(const char *filename, ulong addr, int offset, int len);
 
 /*
+ * Write file "filename" to the partition previously set by fs_set_blk_dev(),
+ * from address "addr", starting at byte offset "offset", and writing "len"
+ * bytes. "offset" may be 0 to write to the start of the file. Note that not
+ * all filesystem types support offset!=0.
+ *
+ * Returns number of bytes read on success. Returns <= 0 on error.
+ */
+int fs_write(const char *filename, ulong addr, int offset, int len);
+
+/*
  * Common implementation for various filesystem commands, optionally limited
  * to a specific filesystem type via the fstype parameter.
  */
-- 
1.8.1.5

[U-Boot] [PATCH V2 2/5] fs: implement infra-structure for an 'exists' function

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This could be used in scripts such as:

if exists mmc 0:1 /boot/boot.scr; then
    load mmc 0:1 ${scriptaddr} /boot/boot.scr
    source ${scriptaddr}
fi

rather than:

if load mmc 0:1 ${scriptaddr} /boot/boot.scr; then
    source ${scriptaddr}
fi

This prevents errors being printed by attempts to load non-existent
files, which can be important when checking for a large set of files,
such as /boot/boot.scr.uimg, /boot/boot.scr, /boot/extlinux.conf,
/boot.scr.uimg, /boot.scr, /extlinux.conf.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 common/cmd_fs.c | 14 ++++++++++++++
 fs/fs.c         | 38 ++++++++++++++++++++++++++++++++++++++
 include/fs.h    | 10 ++++++++++
 3 files changed, 62 insertions(+)

diff --git a/common/cmd_fs.c b/common/cmd_fs.c
index 91a205ac1e69..44b00cd125b7 100644
--- a/common/cmd_fs.c
+++ b/common/cmd_fs.c
@@ -49,3 +49,17 @@ U_BOOT_CMD(
 	"    - List files in directory 'directory' of partition 'part' on\n"
 	"      device type 'interface' instance 'dev'."
 );
+
+int do_exists_wrapper(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	return do_exists(cmdtp, flag, argc, argv, FS_TYPE_ANY);
+}
+
+U_BOOT_CMD(
+	exists,	4,	0,	do_exists_wrapper,
+	"determine whether a file exists",
+	"<interface> <dev[:part]> filename\n"
+	"    - Determine whether 'filename' exists in partition 'part' on\n"
+	"      device type 'interface' instance 'dev', and set the command.\n"
+	"      exit status so that 'if exists ...; then' works.\n"
+);
diff --git a/fs/fs.c b/fs/fs.c
index 9c2ef6b6597c..f3d9a1c6bc7d 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -41,6 +41,11 @@ static inline int fs_ls_unsupported(const char *dirname)
 	return -1;
 }
 
+static inline int fs_exists_unsupported(const char *filename)
+{
+	return -1;
+}
+
 static inline int fs_read_unsupported(const char *filename, void *buf,
 				      int offset, int len)
 {
@@ -62,6 +67,7 @@ struct fstype_info {
 	int (*probe)(block_dev_desc_t *fs_dev_desc,
 		     disk_partition_t *fs_partition);
 	int (*ls)(const char *dirname);
+	int (*exists)(const char *filename);
 	int (*read)(const char *filename, void *buf, int offset, int len);
 	int (*write)(const char *filename, void *buf, int offset, int len);
 	void (*close)(void);
@@ -74,6 +80,7 @@ static struct fstype_info fstypes[] = {
 		.probe = fat_set_blk_dev,
 		.close = fat_close,
 		.ls = file_fat_ls,
+		.exists = fs_exists_unsupported,
 		.read = fat_read_file,
 		.write = fs_write_unsupported,
 	},
@@ -84,6 +91,7 @@ static struct fstype_info fstypes[] = {
 		.probe = ext4fs_probe,
 		.close = ext4fs_close,
 		.ls = ext4fs_ls,
+		.exists = fs_exists_unsupported,
 		.read = ext4_read_file,
 		.write = fs_write_unsupported,
 	},
@@ -94,6 +102,7 @@ static struct fstype_info fstypes[] = {
 		.probe = sandbox_fs_set_blk_dev,
 		.close = sandbox_fs_close,
 		.ls = sandbox_fs_ls,
+		.exists = fs_exists_unsupported,
 		.read = fs_read_sandbox,
 		.write = fs_write_sandbox,
 	},
@@ -103,6 +112,7 @@ static struct fstype_info fstypes[] = {
 		.probe = fs_probe_unsupported,
 		.close = fs_close_unsupported,
 		.ls = fs_ls_unsupported,
+		.exists = fs_exists_unsupported,
 		.read = fs_read_unsupported,
 		.write = fs_write_unsupported,
 	},
@@ -184,6 +194,19 @@ int fs_ls(const char *dirname)
 	return ret;
 }
 
+int fs_exists(const char *filename)
+{
+	int ret;
+
+	struct fstype_info *info = fs_get_info(fs_type);
+
+	ret = info->exists(filename);
+
+	fs_close();
+
+	return ret;
+}
+
 int fs_read(const char *filename, ulong addr, int offset, int len)
 {
 	struct fstype_info *info = fs_get_info(fs_type);
@@ -309,6 +332,21 @@ int do_ls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 	return 0;
 }
 
+int do_exists(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
+		int fstype)
+{
+	if (argc != 4)
+		return CMD_RET_USAGE;
+
+	if (fs_set_blk_dev(argv[1], argv[2], fstype))
+		return 1;
+
+	if (fs_exists(argv[3]))
+		return 1;
+
+	return 0;
+}
+
 int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		int fstype)
 {
diff --git a/include/fs.h b/include/fs.h
index 97b0094e954b..b8b7706410ad 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -44,6 +44,14 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype);
 int fs_ls(const char *dirname);
 
 /*
+ * Determine whether a file exists
+ *
+ * Returns 0 if the file exists, non-zero if it doesn't exist.
+ * This encoding was picked to help shell command implementation.
+ */
+int fs_exists(const char *filename);
+
+/*
  * Read file "filename" from the partition previously set by fs_set_blk_dev(),
  * to address "addr", starting at byte offset "offset", and reading "len"
  * bytes. "offset" may be 0 to read from the start of the file. "len" may be
@@ -72,6 +80,8 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		int fstype);
 int do_ls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		int fstype);
+int do_exists(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
+		int fstype);
 int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		int fstype);
 
-- 
1.8.1.5

[U-Boot] [PATCH V2 3/5] sandbox: implement fs_exists() and 'sb exists' shell function

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This hooks into the generic "file exists" support added in the previous
patch, and provides an implementation for the sandbox test environment.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 common/cmd_sandbox.c   | 7 +++++++
 fs/fs.c                | 2 +-
 fs/sandbox/sandboxfs.c | 8 ++++++++
 include/sandboxfs.h    | 1 +
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c
index 00982b164dd3..c8d36b747d6a 100644
--- a/common/cmd_sandbox.c
+++ b/common/cmd_sandbox.c
@@ -22,6 +22,12 @@ static int do_sandbox_ls(cmd_tbl_t *cmdtp, int flag, int argc,
 	return do_ls(cmdtp, flag, argc, argv, FS_TYPE_SANDBOX);
 }
 
+static int do_sandbox_exists(cmd_tbl_t *cmdtp, int flag, int argc,
+			     char * const argv[])
+{
+	return do_exists(cmdtp, flag, argc, argv, FS_TYPE_SANDBOX);
+}
+
 static int do_sandbox_save(cmd_tbl_t *cmdtp, int flag, int argc,
 			   char * const argv[])
 {
@@ -88,6 +94,7 @@ static int do_sandbox_info(cmd_tbl_t *cmdtp, int flag, int argc,
 static cmd_tbl_t cmd_sandbox_sub[] = {
 	U_BOOT_CMD_MKENT(load, 7, 0, do_sandbox_load, "", ""),
 	U_BOOT_CMD_MKENT(ls, 3, 0, do_sandbox_ls, "", ""),
+	U_BOOT_CMD_MKENT(exists, 3, 0, do_sandbox_exists, "", ""),
 	U_BOOT_CMD_MKENT(save, 6, 0, do_sandbox_save, "", ""),
 	U_BOOT_CMD_MKENT(bind, 3, 0, do_sandbox_bind, "", ""),
 	U_BOOT_CMD_MKENT(info, 3, 0, do_sandbox_info, "", ""),
diff --git a/fs/fs.c b/fs/fs.c
index f3d9a1c6bc7d..4f344c6d6d72 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -102,7 +102,7 @@ static struct fstype_info fstypes[] = {
 		.probe = sandbox_fs_set_blk_dev,
 		.close = sandbox_fs_close,
 		.ls = sandbox_fs_ls,
-		.exists = fs_exists_unsupported,
+		.exists = sandbox_fs_exists,
 		.read = fs_read_sandbox,
 		.write = fs_write_sandbox,
 	},
diff --git a/fs/sandbox/sandboxfs.c b/fs/sandbox/sandboxfs.c
index dd028da8e32b..7940c93dc6a3 100644
--- a/fs/sandbox/sandboxfs.c
+++ b/fs/sandbox/sandboxfs.c
@@ -72,6 +72,14 @@ int sandbox_fs_ls(const char *dirname)
 	return 0;
 }
 
+int sandbox_fs_exists(const char *filename)
+{
+	ssize_t sz;
+
+	sz = os_get_filesize(filename);
+	return (sz >= 0) ? 0 : -1;
+}
+
 void sandbox_fs_close(void)
 {
 }
diff --git a/include/sandboxfs.h b/include/sandboxfs.h
index 8ea8cb7e2e62..a51ad13044e1 100644
--- a/include/sandboxfs.h
+++ b/include/sandboxfs.h
@@ -25,6 +25,7 @@ long sandbox_fs_read_at(const char *filename, unsigned long pos,
 
 void sandbox_fs_close(void);
 int sandbox_fs_ls(const char *dirname);
+int sandbox_fs_exists(const char *filename);
 int fs_read_sandbox(const char *filename, void *buf, int offset, int len);
 int fs_write_sandbox(const char *filename, void *buf, int offset, int len);
 
-- 
1.8.1.5

[U-Boot] [PATCH V2 4/5] ext4: implement exists() for ext4fs

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This hooks into the generic "file exists" support added in an earlier
patch, and provides an implementation for the ext4 filesystem.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 fs/ext4/ext4fs.c | 8 ++++++++
 fs/fs.c          | 2 +-
 include/ext4fs.h | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c
index 735b2564175b..71ecdd8e9a31 100644
--- a/fs/ext4/ext4fs.c
+++ b/fs/ext4/ext4fs.c
@@ -174,6 +174,14 @@ int ext4fs_ls(const char *dirname)
 	return 0;
 }
 
+int ext4fs_exists(const char *filename)
+{
+	int file_len;
+
+	file_len = ext4fs_open(filename);
+	return (file_len >= 0) ? 0 : 1;
+}
+
 int ext4fs_read(char *buf, unsigned len)
 {
 	if (ext4fs_root == NULL || ext4fs_file == NULL)
diff --git a/fs/fs.c b/fs/fs.c
index 4f344c6d6d72..3f14d0169b43 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -91,7 +91,7 @@ static struct fstype_info fstypes[] = {
 		.probe = ext4fs_probe,
 		.close = ext4fs_close,
 		.ls = ext4fs_ls,
-		.exists = fs_exists_unsupported,
+		.exists = ext4fs_exists,
 		.read = ext4_read_file,
 		.write = fs_write_unsupported,
 	},
diff --git a/include/ext4fs.h b/include/ext4fs.h
index 242938039662..aacb147de24b 100644
--- a/include/ext4fs.h
+++ b/include/ext4fs.h
@@ -134,6 +134,7 @@ int ext4fs_read(char *buf, unsigned len);
 int ext4fs_mount(unsigned part_length);
 void ext4fs_close(void);
 int ext4fs_ls(const char *dirname);
+int ext4fs_exists(const char *filename);
 void ext4fs_free_node(struct ext2fs_node *node, struct ext2fs_node *currroot);
 int ext4fs_devread(lbaint_t sector, int byte_offset, int byte_len, char *buf);
 void ext4fs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info);
-- 
1.8.1.5

[U-Boot] [PATCH V2 5/5] fat: implement exists() for FAT fs

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This hooks into the generic "file exists" support added in an earlier
patch, and provides an implementation for the ext4 filesystem.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v2: s/ext/fat/ in the commit subject
---
 fs/fat/fat.c  | 18 ++++++++++++++----
 fs/fs.c       |  2 +-
 include/fat.h |  1 +
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index b41d62e3c386..bc06c0a3c688 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -808,7 +808,7 @@ __u8 do_fat_read_at_block[MAX_CLUSTSIZE]
 
 long
 do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
-	       unsigned long maxsize, int dols)
+	       unsigned long maxsize, int dols, int dogetsize)
 {
 	char fnamecopy[2048];
 	boot_sector bs;
@@ -1152,7 +1152,10 @@ rootdir_done:
 			subname = nextname;
 	}
 
-	ret = get_contents(mydata, dentptr, pos, buffer, maxsize);
+	if (dogetsize)
+		ret = FAT2CPU32(dentptr->size);
+	else
+		ret = get_contents(mydata, dentptr, pos, buffer, maxsize);
 	debug("Size: %d, got: %ld\n", FAT2CPU32(dentptr->size), ret);
 
 exit:
@@ -1163,7 +1166,7 @@ exit:
 long
 do_fat_read(const char *filename, void *buffer, unsigned long maxsize, int dols)
 {
-	return do_fat_read_at(filename, 0, buffer, maxsize, dols);
+	return do_fat_read_at(filename, 0, buffer, maxsize, dols, 0);
 }
 
 int file_fat_detectfs(void)
@@ -1233,11 +1236,18 @@ int file_fat_ls(const char *dir)
 	return do_fat_read(dir, NULL, 0, LS_YES);
 }
 
+int fat_exists(const char *filename)
+{
+	int sz;
+	sz = do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1);
+	return (sz >= 0) ? 0 : 1;
+}
+
 long file_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 		      unsigned long maxsize)
 {
 	printf("reading %s\n", filename);
-	return do_fat_read_at(filename, pos, buffer, maxsize, LS_NO);
+	return do_fat_read_at(filename, pos, buffer, maxsize, LS_NO, 0);
 }
 
 long file_fat_read(const char *filename, void *buffer, unsigned long maxsize)
diff --git a/fs/fs.c b/fs/fs.c
index 3f14d0169b43..d2bc8d0f1459 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -80,7 +80,7 @@ static struct fstype_info fstypes[] = {
 		.probe = fat_set_blk_dev,
 		.close = fat_close,
 		.ls = file_fat_ls,
-		.exists = fs_exists_unsupported,
+		.exists = fat_exists,
 		.read = fat_read_file,
 		.write = fs_write_unsupported,
 	},
diff --git a/include/fat.h b/include/fat.h
index 2c951e7d79c6..c8eb7ccd2904 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -188,6 +188,7 @@ file_read_func		file_fat_read;
 int file_cd(const char *path);
 int file_fat_detectfs(void);
 int file_fat_ls(const char *dir);
+int fat_exists(const char *filename);
 long file_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 		      unsigned long maxsize);
 long file_fat_read(const char *filename, void *buffer, unsigned long maxsize);
-- 
1.8.1.5

[U-Boot] [PATCH V2 1/5] fs: fix generic save command implementation

[Simon Glass]

On 23 January 2014 12:56, Stephen Warren <swarren@wwwdotorg.org> wrote:

> From: Stephen Warren <swarren@nvidia.com>
>
> Fix a few issues with the generic "save" shell command, and fs_write()
> function.
>
> 1) fstypes[].write wasn't filled in for some file-systems, and isn't
>    checked when used, which could cause crashes/... if executing save
>    on e.g. fat/ext filesystems.
>
> 2) fs_write() requires the length argument to be non-zero, since it needs
>    to know exactly how many bytes to write. Adjust the comments and code
>    according to this.
>
> 3) fs_write() wasn't prototyped in <fs.h> like other generic functions;
>    other code should be able to call this directly rather than invoking
>    the "save" shell command.
>
> Cc: Simon Glass <sjg@chromium.org>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>

Not sure about the change log for v2, but the patch looks good.

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH V2 2/5] fs: implement infra-structure for an 'exists' function

[Simon Glass]

Hi Stephen,

On 23 January 2014 12:56, Stephen Warren <swarren@wwwdotorg.org> wrote:

> From: Stephen Warren <swarren@nvidia.com>
>
> This could be used in scripts such as:
>
> if exists mmc 0:1 /boot/boot.scr; then
>     load mmc 0:1 ${scriptaddr} /boot/boot.scr
>     source ${scriptaddr}
> fi
>
> rather than:
>
> if load mmc 0:1 ${scriptaddr} /boot/boot.scr; then
>     source ${scriptaddr}
> fi
>
> This prevents errors being printed by attempts to load non-existent
> files, which can be important when checking for a large set of files,
> such as /boot/boot.scr.uimg, /boot/boot.scr, /boot/extlinux.conf,
> /boot.scr.uimg, /boot.scr, /extlinux.conf.
>

Change log?


>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>

Acked-by: Simon Glass <sjg@chromium.org>

Seems useful.

In addition, if it is just the error messages you are worried about (and I
agree they should be eliminated) I wonder if we should consider adding a -e
flag (or similar) to the read command to make it silently fail when the
file does not exist? Arguably your code fragment above could be:

if load -e mmc 0:1 ${scriptaddr} /boot/boot.scr; then
    source ${scriptaddr}
fi

Regards,
Simon

[U-Boot] [PATCH V2 3/5] sandbox: implement fs_exists() and 'sb exists' shell function

[Simon Glass]

On 23 January 2014 12:56, Stephen Warren <swarren@wwwdotorg.org> wrote:

> From: Stephen Warren <swarren@nvidia.com>
>
> This hooks into the generic "file exists" support added in the previous
> patch, and provides an implementation for the sandbox test environment.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH V2 4/5] ext4: implement exists() for ext4fs

[Simon Glass]

On 23 January 2014 12:56, Stephen Warren <swarren@wwwdotorg.org> wrote:

> From: Stephen Warren <swarren@nvidia.com>
>
> This hooks into the generic "file exists" support added in an earlier
> patch, and provides an implementation for the ext4 filesystem.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH V2 5/5] fat: implement exists() for FAT fs

[Simon Glass]

Hi Stephen,

On 23 January 2014 12:57, Stephen Warren <swarren@wwwdotorg.org> wrote:

> From: Stephen Warren <swarren@nvidia.com>
>
> This hooks into the generic "file exists" support added in an earlier
> patch, and provides an implementation for the ext4 filesystem.
>

s/ext4/fat


>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v2: s/ext/fat/ in the commit subject
> ---
>  fs/fat/fat.c  | 18 ++++++++++++++----
>  fs/fs.c       |  2 +-
>  include/fat.h |  1 +
>  3 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index b41d62e3c386..bc06c0a3c688 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -808,7 +808,7 @@ __u8 do_fat_read_at_block[MAX_CLUSTSIZE]
>
>  long
>  do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
> -              unsigned long maxsize, int dols)
> +              unsigned long maxsize, int dols, int dogetsize)
>

I think it would be better to combine the three available operations (read,
ls and getsize) into an enum and pass the required operation explicitly
into this function in a single parameter.


>  {
>         char fnamecopy[2048];
>         boot_sector bs;
> @@ -1152,7 +1152,10 @@ rootdir_done:
>                         subname = nextname;
>         }
>
> -       ret = get_contents(mydata, dentptr, pos, buffer, maxsize);
> +       if (dogetsize)
> +               ret = FAT2CPU32(dentptr->size);
> +       else
> +


Doesn't this mean you are actually reading the contents here into a NULL
buffer? At least I think it needs a comment as to why this works.


> ret = get_contents(mydata, dentptr, pos, buffer, maxsize);
>         debug("Size: %d, got: %ld\n", FAT2CPU32(dentptr->size), ret);
>
>  exit:
> @@ -1163,7 +1166,7 @@ exit:
>  long
>  do_fat_read(const char *filename, void *buffer, unsigned long maxsize,
> int dols)
>  {
> -       return do_fat_read_at(filename, 0, buffer, maxsize, dols);
> +       return do_fat_read_at(filename, 0, buffer, maxsize, dols, 0);
>  }
>
>  int file_fat_detectfs(void)
> @@ -1233,11 +1236,18 @@ int file_fat_ls(const char *dir)
>         return do_fat_read(dir, NULL, 0, LS_YES);
>  }
>
> +int fat_exists(const char *filename)
> +{
> +       int sz;
> +       sz = do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1);
> +       return (sz >= 0) ? 0 : 1;
> +}
> +
>  long file_fat_read_at(const char *filename, unsigned long pos, void
> *buffer,
>                       unsigned long maxsize)
>  {
>         printf("reading %s\n", filename);
> -       return do_fat_read_at(filename, pos, buffer, maxsize, LS_NO);
> +       return do_fat_read_at(filename, pos, buffer, maxsize, LS_NO, 0);
>  }
>
>  long file_fat_read(const char *filename, void *buffer, unsigned long
> maxsize)
> diff --git a/fs/fs.c b/fs/fs.c
> index 3f14d0169b43..d2bc8d0f1459 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -80,7 +80,7 @@ static struct fstype_info fstypes[] = {
>                 .probe = fat_set_blk_dev,
>                 .close = fat_close,
>                 .ls = file_fat_ls,
> -               .exists = fs_exists_unsupported,
> +               .exists = fat_exists,
>                 .read = fat_read_file,
>                 .write = fs_write_unsupported,
>         },
> diff --git a/include/fat.h b/include/fat.h
> index 2c951e7d79c6..c8eb7ccd2904 100644
> --- a/include/fat.h
> +++ b/include/fat.h
> @@ -188,6 +188,7 @@ file_read_func              file_fat_read;
>  int file_cd(const char *path);
>  int file_fat_detectfs(void);
>  int file_fat_ls(const char *dir);
> +int fat_exists(const char *filename);
>  long file_fat_read_at(const char *filename, unsigned long pos, void
> *buffer,
>                       unsigned long maxsize);
>  long file_fat_read(const char *filename, void *buffer, unsigned long
> maxsize);
> --
> 1.8.1.5
>
>
Regards,
Simon

[U-Boot] [PATCH V2 2/5] fs: implement infra-structure for an 'exists' function

[Wolfgang Denk]

Dear Stephen Warren,

In message <1390507020-15766-2-git-send-email-swarren@wwwdotorg.org> you wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> This could be used in scripts such as:
> 
> if exists mmc 0:1 /boot/boot.scr; then
>     load mmc 0:1 ${scriptaddr} /boot/boot.scr
>     source ${scriptaddr}
> fi

I understand and agree with your intentions, but I dislike to invent a
new, totally non-standard command.

In UNIX, we would use the "test" command for such purposes.  Would it
not make sense to implement this feature in a more compatible way?  we
have "test", soo, so we should be able to plug it in there?

What do you think?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Life, loathe it or ignore it, you can't like it."
- Marvin the paranoid android

[U-Boot] [PATCH V2 1/5] fs: fix generic save command implementation

[Stephen Warren]

On 01/26/2014 08:26 AM, Simon Glass wrote:
> On 23 January 2014 12:56, Stephen Warren <swarren@wwwdotorg.org
> <mailto:swarren@wwwdotorg.org>> wrote:
> 
>     From: Stephen Warren <swarren at nvidia.com <mailto:swarren@nvidia.com>>
> 
>     Fix a few issues with the generic "save" shell command, and fs_write()
>     function.
...
> 
> Not sure about the change log for v2, but the patch looks good.

I only put one in for the patch which changed; the others were identical
to v1.

> Acked-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>

Thanks.

[U-Boot] [PATCH V2 2/5] fs: implement infra-structure for an 'exists' function

[Stephen Warren]

On 01/26/2014 08:41 AM, Simon Glass wrote:
> Hi Stephen,
> 
> On 23 January 2014 12:56, Stephen Warren <swarren@wwwdotorg.org
> <mailto:swarren@wwwdotorg.org>> wrote:
> 
>     From: Stephen Warren <swarren at nvidia.com <mailto:swarren@nvidia.com>>
> 
>     This could be used in scripts such as:
> 
>     if exists mmc 0:1 /boot/boot.scr; then
>         load mmc 0:1 ${scriptaddr} /boot/boot.scr
>         source ${scriptaddr}
>     fi
...
> Acked-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
> 
> Seems useful.
> 
> In addition, if it is just the error messages you are worried about (and
> I agree they should be eliminated) I wonder if we should consider adding
> a -e flag (or similar) to the read command to make it silently fail when
> the file does not exist? Arguably your code fragment above could be:
> 
> if load -e mmc 0:1 ${scriptaddr} /boot/boot.scr; then
>     source ${scriptaddr}
> fi

That would certainly work, although I think a direct check of
file-existence is more in line with how a regular Unix shell script
would work (test -e). I could imagine wanting to test for the existence
of a file without caring about its contents (e.g. using a file as a flag
to trigger a firmware upgrade or something)

[U-Boot] [PATCH V2 5/5] fat: implement exists() for FAT fs

[Stephen Warren]

On 01/26/2014 08:52 AM, Simon Glass wrote:
> Hi Stephen,
> 
> On 23 January 2014 12:57, Stephen Warren <swarren@wwwdotorg.org
> <mailto:swarren@wwwdotorg.org>> wrote:
> 
>     From: Stephen Warren <swarren at nvidia.com <mailto:swarren@nvidia.com>>
> 
>     This hooks into the generic "file exists" support added in an earlier
>     patch, and provides an implementation for the ext4 filesystem.

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

>     @@ -808,7 +808,7 @@ __u8 do_fat_read_at_block[MAX_CLUSTSIZE]
> 
>      long
>      do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
>     -              unsigned long maxsize, int dols)
>     +              unsigned long maxsize, int dols, int dogetsize)
> 
> I think it would be better to combine the three available operations
> (read, ls and getsize) into an enum and pass the required operation
> explicitly into this function in a single parameter.

I had originally thought that, but the implementation of the function
changes the value of dols during operation. I suppose it would be
possible to achieve that using bit-mask operations on the variable, but
it seemed a bit cleaner to just make it a separate variable rather than
using fields within a somewhat unrelated variable.

Would you still prefer to combine them into one:?

>     -       ret = get_contents(mydata, dentptr, pos, buffer, maxsize);
>     +       if (dogetsize)
>     +               ret = FAT2CPU32(dentptr->size);
>     +       else
>     +              
> 
> 
> Doesn't this mean you are actually reading the contents here into a NULL
> buffer? At least I think it needs a comment as to why this works.
> 
>     ret = get_contents(mydata, dentptr, pos, buffer, maxsize);
>             debug("Size: %d, got: %ld\n", FAT2CPU32(dentptr->size), ret);

get_contents() is the function that actually reads the file content;
everything before this point is simply parsing the directory entry for
the file. So, no, I don't think the file content is read at all.

That implementation of dols!=0 is somewhat similar, although it does
"goto exit" at a different location in the code.

[U-Boot] [PATCH V2 2/5] fs: implement infra-structure for an 'exists' function

[Simon Glass]

Hi Stephen,

On 27 January 2014 09:03, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 01/26/2014 08:41 AM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On 23 January 2014 12:56, Stephen Warren <swarren@wwwdotorg.org
>> <mailto:swarren@wwwdotorg.org>> wrote:
>>
>>     From: Stephen Warren <swarren at nvidia.com <mailto:swarren@nvidia.com>>
>>
>>     This could be used in scripts such as:
>>
>>     if exists mmc 0:1 /boot/boot.scr; then
>>         load mmc 0:1 ${scriptaddr} /boot/boot.scr
>>         source ${scriptaddr}
>>     fi
> ...
>> Acked-by: Simon Glass <sjg at chromium.org <mailto:sjg@chromium.org>>
>>
>> Seems useful.
>>
>> In addition, if it is just the error messages you are worried about (and
>> I agree they should be eliminated) I wonder if we should consider adding
>> a -e flag (or similar) to the read command to make it silently fail when
>> the file does not exist? Arguably your code fragment above could be:
>>
>> if load -e mmc 0:1 ${scriptaddr} /boot/boot.scr; then
>>     source ${scriptaddr}
>> fi
>
> That would certainly work, although I think a direct check of
> file-existence is more in line with how a regular Unix shell script
> would work (test -e). I could imagine wanting to test for the existence
> of a file without caring about its contents (e.g. using a file as a flag
> to trigger a firmware upgrade or something)

Given all the parameters, it is bit different from Unix, but test -e
<lots of params> sounds like a nice idea.

Regards,
Simon

[U-Boot] [PATCH V2 2/5] fs: implement infra-structure for an 'exists' function

[Stephen Warren]

On 01/26/2014 12:44 PM, Wolfgang Denk wrote:
> Dear Stephen Warren,
> 
> In message <1390507020-15766-2-git-send-email-swarren@wwwdotorg.org> you wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> This could be used in scripts such as:
>>
>> if exists mmc 0:1 /boot/boot.scr; then
>>     load mmc 0:1 ${scriptaddr} /boot/boot.scr
>>     source ${scriptaddr}
>> fi
> 
> I understand and agree with your intentions, but I dislike to invent a
> new, totally non-standard command.
> 
> In UNIX, we would use the "test" command for such purposes.  Would it
> not make sense to implement this feature in a more compatible way?  we
> have "test", soo, so we should be able to plug it in there?
> 
> What do you think?

I thought that "test -e mmc 0:1 /boot/boot.scr" was different enough
from a regular Unix shell's single-argument path that it would look a
little weird. Still, it works out fine, so I've sent V3 that implements
this via test instead.

[U-Boot] [PATCH V2 2/5] fs: implement infra-structure for an 'exists' function

[Wolfgang Denk]

Dear Stephen,

In message <52E6C6CC.7000604@wwwdotorg.org> you wrote:
>
> > I understand and agree with your intentions, but I dislike to invent a
> > new, totally non-standard command.
> > 
> > In UNIX, we would use the "test" command for such purposes.  Would it
> > not make sense to implement this feature in a more compatible way?  we
> > have "test", soo, so we should be able to plug it in there?
> > 
> > What do you think?
> 
> I thought that "test -e mmc 0:1 /boot/boot.scr" was different enough
> from a regular Unix shell's single-argument path that it would look a
> little weird. Still, it works out fine, so I've sent V3 that implements
> this via test instead.

Yes, it looks weird, but unless we have a better device model and
abstraction for files this is probably the best we can get.

Thanks a lot!

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
What is wanted is not the will to believe,  but the will to find out,
which is the exact opposite.
		        -- Bertrand Russell, "Skeptical Essays", 1928