+Tom for the question below re return values Hi,
On 8 October 2014 15:54, Suriyan Ramasami <suriya...@gmail.com> wrote: > > On Wed, Oct 8, 2014 at 1:44 PM, Simon Glass <s...@chromium.org> wrote: > > Hi Suriyan, > > > > On 8 October 2014 14:23, Suriyan Ramasami <suriya...@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 <suriya...@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. > > > > >> + else > >> + return 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; > >> + else > >> + return 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot