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
?
>>
>> 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.
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
- Suriyan
> Regards,
> Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
