[U-Boot] [PATCH V2 1/5] fs: fix generic save command implementation
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. */
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);
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
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@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@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)
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@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@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
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
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.
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
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);
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
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);
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
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);
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
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@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.
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.
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.
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.
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
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@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@chromium.org mailto:sjg@chromium.org>
Thanks.
participants (3)
-
Simon Glass -
Stephen Warren -
Wolfgang Denk