[U-Boot,v1,1/1] fs: fat/ext4/sandbox: Deal with files > 2GB in ls and size commands

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.

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(-)

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.

>
> 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.

>
> 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)?

>         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?

> +       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?

>  }
>
>  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.

>  }
>
>  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
>

Regards,
Simon

On Wed, Oct 8, 2014 at 1:35 PM, Pavel Machek <pavel@denx.de> wrote:
> On Wed 2014-10-08 13:23:48, Suriyan Ramasami 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.
>
> Would not it be better to simply change return type of affected
> functions to off_t?
>
> (If off_t is unsigned, invent signed off_t. It is still nicer than
> extra parameter.)
>>

off_t is 32 bits in 32 bit architectures. So, signed or unsigned it
shall always have a case where it will be impossible to detect a valid
file size vs an error condition, right?
For example if we return 0xFFFFFFFF, is that an error condition or the
actual file size?
Another approach would be to make the return value off64_t, but then
in sandbox cases we will hit the same issue of differentiating a valid
file size vs an error condition (although we possibly would never have
such a huge file)

Please let me know if I am not following your suggestion.

Thanks
- Suriyan

>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

On Wed 2014-10-08 14:08:45, Suriyan Ramasami wrote:
> On Wed, Oct 8, 2014 at 1:35 PM, Pavel Machek <pavel@denx.de> wrote:
> > On Wed 2014-10-08 13:23:48, Suriyan Ramasami 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.
> >
> > Would not it be better to simply change return type of affected
> > functions to off_t?
> >
> > (If off_t is unsigned, invent signed off_t. It is still nicer than
> > extra parameter.)
> >>
> 
> off_t is 32 bits in 32 bit architectures. So, signed or unsigned it
> shall always have a case where it will be impossible to detect a valid
> file size vs an error condition, right?
> For example if we return 0xFFFFFFFF, is that an error condition or the
> actual file size?
> Another approach would be to make the return value off64_t, but then
> in sandbox cases we will hit the same issue of differentiating a valid
> file size vs an error condition (although we possibly would never have
> such a huge file)

I'd just make it off64_t. Because otherwise you fixed files in
2GB..4GB range, but still have problems for >4GB files.

									Pavel

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
?

>>
>> 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.

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

+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.
>
> >
> >> +       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

Hello Simon,

On Thu, Oct 9, 2014 at 8:42 AM, Simon Glass <sjg@chromium.org> wrote:
> +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.
>

I also do think that having a consistent error return value from all
functions (int) seems more clean.
Hence, I am going forward in working on a patch towards that goal. I
am also introducing automated tests cases in test/ as you have
suggested (in a different thread).
BTW, I shall be using (loff_t *), for returning the actual size in the
extra parameter to the function calls.

Thanks
- Suriyan

>>
>>
>> >>
>> >> 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.
>>
>> >
>> >> +       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

Hi Suriyan,

On 17 October 2014 13:17, Suriyan Ramasami <suriyan.r@gmail.com> wrote:
> Hello Simon,
>
> On Thu, Oct 9, 2014 at 8:42 AM, Simon Glass <sjg@chromium.org> wrote:
>> +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.
>>
>
> I also do think that having a consistent error return value from all
> functions (int) seems more clean.
> Hence, I am going forward in working on a patch towards that goal. I
> am also introducing automated tests cases in test/ as you have
> suggested (in a different thread).
> BTW, I shall be using (loff_t *), for returning the actual size in the
> extra parameter to the function calls.

That sounds good, thanks.

Regards,
Simon