+Tom for the question below re return values
Hi,
On 8 October 2014 15:54, Suriyan Ramasami suriyan.r@gmail.com wrote:
On Wed, Oct 8, 2014 at 1:44 PM, Simon Glass sjg@chromium.org wrote:
Hi Suriyan,
On 8 October 2014 14:23, Suriyan Ramasami suriyan.r@gmail.com wrote:
The commands fatls/ext4ls give -ve values when dealing with files > 2GB. The commands fatsize/ext4size do not update the variable filesize for these files.
To deal with this, the functions *_size have been modified to take a second parameter of type "* off_t" which is then populated. The return value of the *_size function is then only used to determine error conditions.
That seems like a sensible idea to me.
Hello Simon, I got the reply from Pavel as I was writing this. So, what do you think of just changing the return value of these functions to off64_t ?
I don't have strong views on this but I believe it is slightly better to use a consistent error return value from all functions (int) as you have done and put loff_t or whatever as a parameter. Even for the size functions this seems better once we move to handling >2GB or >4GB.
But yes a 64-bit value seems prudent despite the overhead.
Signed-off-by: Suriyan Ramasami suriyan.r@gmail.com
arch/sandbox/cpu/os.c | 14 +++++------ arch/sandbox/cpu/state.c | 6 ++--- common/board_f.c | 6 ++--- fs/ext4/ext4_common.c | 13 +++++------ fs/ext4/ext4fs.c | 21 ++++++++++------- fs/fat/fat.c | 61 ++++++++++++++++++++++++++++++++---------------- fs/fs.c | 15 ++++++------ fs/sandbox/sandboxfs.c | 23 ++++++++++++------ include/ext4fs.h | 4 ++-- include/fat.h | 2 +- include/fs.h | 2 +- include/os.h | 2 +- include/sandboxfs.h | 2 +- 13 files changed, 103 insertions(+), 68 deletions(-)
Thanks for doing this. Do you think you could also add a test for this (perhaps in test/fs). It could create a temporary ext2 filesystem, put a large file in it and then check a few operations.
Thanks for pointing out the test directory. I was unaware of its existence. I shall indeed add some test cases.
diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index 1c4aa3f..9b052ee 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -385,15 +385,15 @@ const char *os_dirent_get_typename(enum os_dirent_t type) return os_dirent_typename[OS_FILET_UNKNOWN]; }
-ssize_t os_get_filesize(const char *fname) +int os_get_filesize(const char *fname, off_t *size) { struct stat buf; int ret;
ret = stat(fname, &buf);
if (ret)return ret;return buf.st_size;
if (ret == 0)*size = buf.st_size;return ret;}
void os_putc(int ch) @@ -427,10 +427,10 @@ int os_read_ram_buf(const char *fname) { struct sandbox_state *state = state_get_current(); int fd, ret;
int size;
off_t size;
size = os_get_filesize(fname);if (size < 0)
ret = os_get_filesize(fname, &size);if (ret < 0) return -ENOENT;Should you return ret instead here (and in other cases below)?
I wanted to keep the return values as consistent with the original, so as to preserve same behavior before this code change. os_get_filesize() calls the stat() function in sandbox, and that has quite a many failure return values. I did not check if they are handled by the callers of os_read_ram_buf(). I am OK to change it, but then have to follow up on all the callers of os_read_ram_buf() to see if they are indeed gracefully handling the error conditions.
Yes you can change it, there is only one caller and it doesn't check the specific error.
This is also the reasoning (potentially flawed) in general that I followed when the *_size() functions are being used by calls which do the actual read from the file. In those cases, I have reverted to previous behavior where I return an int as before, assuming that a call to read more than 2G of data might not be issued.
I am assuming, to be clean, more effort in consistently using size_t would be apt all across the code. Right?
if (size != state->ram_size) return -ENOSPC;diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c index 59adad6..e0d119a 100644 --- a/arch/sandbox/cpu/state.c +++ b/arch/sandbox/cpu/state.c @@ -49,12 +49,12 @@ static int state_ensure_space(int extra_size)
static int state_read_file(struct sandbox_state *state, const char *fname) {
int size;
off_t size; int ret; int fd;
size = os_get_filesize(fname);if (size < 0) {
ret = os_get_filesize(fname, &size);if (ret < 0) { printf("Cannot find sandbox state file '%s'\n", fname); return -ENOENT; }diff --git a/common/board_f.c b/common/board_f.c index e6aa298..b8ec7ac 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -291,7 +291,7 @@ static int read_fdt_from_file(void) struct sandbox_state *state = state_get_current(); const char *fname = state->fdt_fname; void *blob;
ssize_t size;
off_t size; int err; int fd;@@ -304,8 +304,8 @@ static int read_fdt_from_file(void) return -EINVAL; }
size = os_get_filesize(fname);if (size < 0) {
err = os_get_filesize(fname, &size);if (err < 0) { printf("Failed to file FDT file '%s'\n", fname); return -ENOENT; }diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 33d69c9..fd9b611 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -2003,9 +2003,9 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, printf("< ? > "); break; }
printf("%10d %s\n",__le32_to_cpu(fdiro->inode.size),filename);
printf("%10u %s\n",__le32_to_cpu(fdiro->inode.size),filename); } free(fdiro); }@@ -2169,11 +2169,10 @@ int ext4fs_find_file(const char *path, struct ext2fs_node *rootnode, return 1; }
-int ext4fs_open(const char *filename) +int ext4fs_open(const char *filename, off_t *len) { struct ext2fs_node *fdiro = NULL; int status;
int len; if (ext4fs_root == NULL) return -1;@@ -2190,10 +2189,10 @@ int ext4fs_open(const char *filename) if (status == 0) goto fail; }
len = __le32_to_cpu(fdiro->inode.size);
*len = __le32_to_cpu(fdiro->inode.size); ext4fs_file = fdiro;
return len;
return 0;fail: ext4fs_free_node(fdiro, &ext4fs_root->diropen);
diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c index cbdc220..3e4eaaa 100644 --- a/fs/ext4/ext4fs.c +++ b/fs/ext4/ext4fs.c @@ -176,15 +176,19 @@ int ext4fs_ls(const char *dirname)
int ext4fs_exists(const char *filename) {
int file_len;
off_t file_len;int ret;
file_len = ext4fs_open(filename);return file_len >= 0;
ret = ext4fs_open(filename, &file_len);return ret == 0;}
-int ext4fs_size(const char *filename) +int ext4fs_size(const char *filename, off_t *file_size) {
return ext4fs_open(filename);
int ret;ret = ext4fs_open(filename, file_size);return ret == 0;}
int ext4fs_read(char *buf, unsigned len) @@ -210,7 +214,8 @@ int ext4fs_probe(block_dev_desc_t *fs_dev_desc,
int ext4_read_file(const char *filename, void *buf, int offset, int len) {
int file_len;
off_t file_len;int ret; int len_read; if (offset != 0) {@@ -218,8 +223,8 @@ int ext4_read_file(const char *filename, void *buf, int offset, int len) return -1; }
file_len = ext4fs_open(filename);if (file_len < 0) {
ret = ext4fs_open(filename, &file_len);if (ret != 0) { printf("** File not found %s **\n", filename); return -1; }diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 561921f..650ee7e 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -806,9 +806,9 @@ exit: __u8 do_fat_read_at_block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);
-long +int do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
unsigned long maxsize, int dols, int dogetsize)
unsigned long maxsize, int dols, int dogetsize, off_t *size){ char fnamecopy[2048]; boot_sector bs; @@ -974,10 +974,10 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, } if (doit) { if (dirc == ' ') {
printf(" %8ld %s%c\n",(long)FAT2CPU32(dentptr->size),l_name,dirc);
printf(" %8u %s%c\n",FAT2CPU32(dentptr->size),l_name,dirc); } else { printf(" %s%c\n", l_name,@@ -1032,9 +1032,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, } if (doit) { if (dirc == ' ') {
printf(" %8ld %s%c\n",(long)FAT2CPU32(dentptr->size),s_name, dirc);
printf(" %8u %s%c\n",FAT2CPU32(dentptr->size),s_name, dirc); } else { printf(" %s%c\n", s_name, dirc);@@ -1153,20 +1153,28 @@ rootdir_done: }
if (dogetsize)
ret = FAT2CPU32(dentptr->size);
*size = FAT2CPU32(dentptr->size); else
ret = get_contents(mydata, dentptr, pos, buffer, maxsize);debug("Size: %d, got: %ld\n", FAT2CPU32(dentptr->size), ret);
*size = get_contents(mydata, dentptr, pos, buffer, maxsize);debug("Size: %u, got: %u\n", FAT2CPU32(dentptr->size),(unsigned) *size);exit: free(mydata->fatbuf); return ret; }
-long +int do_fat_read(const char *filename, void *buffer, unsigned long maxsize, int dols) {
return do_fat_read_at(filename, 0, buffer, maxsize, dols, 0);
int ret;off_t size;ret = do_fat_read_at(filename, 0, buffer, maxsize, dols, 0, &size);if (ret == 0)return size;Will this cause problems if size is >2GB?
Yes it will, but my reasoning was as I have mentioned above.
elsereturn ret;}
int file_fat_detectfs(void) @@ -1238,21 +1246,34 @@ int file_fat_ls(const char *dir)
int fat_exists(const char *filename) {
int sz;sz = do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1);return sz >= 0;
int ret;off_t sz;ret = do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1, &sz);return ret == 0;}
-int fat_size(const char *filename) +int fat_size(const char *filename, off_t *sz) {
return do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1);
int ret;ret = do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1, sz);return ret == 0;Should this return an error?
I shall correct this. Missed this one :-)
}
long file_fat_read_at(const char *filename, unsigned long pos, void *buffer, unsigned long maxsize) {
int ret;off_t sz;printf("reading %s\n", filename);
return do_fat_read_at(filename, pos, buffer, maxsize, LS_NO, 0);
ret = do_fat_read_at(filename, pos, buffer, maxsize, LS_NO, 0, &sz);if (ret == 0)return sz;elsereturn ret;This is fine for now. It seems like in the future we should change the fs interface to return an error code for every method.
OK.
}
long file_fat_read(const char *filename, void *buffer, unsigned long maxsize) diff --git a/fs/fs.c b/fs/fs.c index dd680f3..c2d5b5f 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -46,7 +46,7 @@ static inline int fs_exists_unsupported(const char *filename) return 0; }
-static inline int fs_size_unsupported(const char *filename) +static inline int fs_size_unsupported(const char *filename, off_t *sz) { return -1; } @@ -82,7 +82,7 @@ struct fstype_info { disk_partition_t *fs_partition); int (*ls)(const char *dirname); int (*exists)(const char *filename);
int (*size)(const char *filename);
int (*size)(const char *filename, off_t *size); 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);@@ -233,13 +233,13 @@ int fs_exists(const char *filename) return ret; }
-int fs_size(const char *filename) +int fs_size(const char *filename, off_t *size) { int ret;
struct fstype_info *info = fs_get_info(fs_type);
ret = info->size(filename);
ret = info->size(filename, size); fs_close();@@ -292,7 +292,8 @@ int fs_write(const char *filename, ulong addr, int offset, int len) int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], int fstype) {
int size;
off_t size;int ret; if (argc != 4) return CMD_RET_USAGE;@@ -300,8 +301,8 @@ int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], if (fs_set_blk_dev(argv[1], argv[2], fstype)) return 1;
size = fs_size(argv[3]);if (size < 0)
ret = fs_size(argv[3], &size);if (ret < 0) return CMD_RET_FAILURE; setenv_hex("filesize", size);diff --git a/fs/sandbox/sandboxfs.c b/fs/sandbox/sandboxfs.c index ba6402c..a384cc7 100644 --- a/fs/sandbox/sandboxfs.c +++ b/fs/sandbox/sandboxfs.c @@ -27,8 +27,13 @@ long sandbox_fs_read_at(const char *filename, unsigned long pos, os_close(fd); return ret; }
if (!maxsize)maxsize = os_get_filesize(filename);
if (!maxsize) {ret = os_get_filesize(filename, (off_t *)&maxsize);if (ret < 0) {os_close(fd);return ret;}} size = os_read(fd, buffer, maxsize); os_close(fd);@@ -74,15 +79,19 @@ int sandbox_fs_ls(const char *dirname)
int sandbox_fs_exists(const char *filename) {
ssize_t sz;
off_t sz;int ret;
sz = os_get_filesize(filename);return sz >= 0;
ret = os_get_filesize(filename, &sz);return ret == 0;}
-int sandbox_fs_size(const char *filename) +int sandbox_fs_size(const char *filename, off_t *size) {
return os_get_filesize(filename);
int ret;ret = os_get_filesize(filename, size);return ret == 0;}
void sandbox_fs_close(void) diff --git a/include/ext4fs.h b/include/ext4fs.h index 6c419f3..1f04973 100644 --- a/include/ext4fs.h +++ b/include/ext4fs.h @@ -129,14 +129,14 @@ int ext4fs_write(const char *fname, unsigned char *buffer, #endif
struct ext_filesystem *get_fs(void); -int ext4fs_open(const char *filename); +int ext4fs_open(const char *filename, off_t *size); int ext4fs_read(char *buf, unsigned len); int ext4fs_mount(unsigned part_length); void ext4fs_close(void); void ext4fs_reinit_global(void); int ext4fs_ls(const char *dirname); int ext4fs_exists(const char *filename); -int ext4fs_size(const char *filename); +int ext4fs_size(const char *filename, off_t *len); 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); diff --git a/include/fat.h b/include/fat.h index 20ca3f3..56aa3b7 100644 --- a/include/fat.h +++ b/include/fat.h @@ -198,7 +198,7 @@ int file_cd(const char *path); int file_fat_detectfs(void); int file_fat_ls(const char *dir); int fat_exists(const char *filename); -int fat_size(const char *filename); +int fat_size(const char *filename, off_t *len); 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); diff --git a/include/fs.h b/include/fs.h index 06a45f2..df39609 100644 --- a/include/fs.h +++ b/include/fs.h @@ -55,7 +55,7 @@ int fs_exists(const char *filename);
- Returns the file's size in bytes, or a negative value if it doesn't exist.
As mentioned above I'm not sure your size is consistent.
*/ -int fs_size(const char *filename); +int fs_size(const char *filename, off_t *len);
/*
- Read file "filename" from the partition previously set by fs_set_blk_dev(),
diff --git a/include/os.h b/include/os.h index 0230a7f..75d9846 100644 --- a/include/os.h +++ b/include/os.h @@ -219,7 +219,7 @@ const char *os_dirent_get_typename(enum os_dirent_t type);
- @param fname Filename to check
- @return size of file, or -1 if an error ocurred
*/ -ssize_t os_get_filesize(const char *fname); +int os_get_filesize(const char *fname, off_t *size);
/**
- Write a character to the controlling OS terminal
diff --git a/include/sandboxfs.h b/include/sandboxfs.h index e7c3262..8588a29 100644 --- a/include/sandboxfs.h +++ b/include/sandboxfs.h @@ -26,7 +26,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 sandbox_fs_size(const char *filename); +int sandbox_fs_size(const char *filename, off_t *size); 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.9.1
Thanks for taking a look. Please also let me know your comments wrt the off64_t approach.
Regards, Simon