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

Simon Glass sjg at chromium.org
Thu Oct 9 17:42:07 CEST 2014


+Tom for the question below re return values

Hi,

On 8 October 2014 15:54, Suriyan Ramasami <suriyan.r at gmail.com> wrote:
>
> On Wed, Oct 8, 2014 at 1:44 PM, Simon Glass <sjg at chromium.org> wrote:
> > Hi Suriyan,
> >
> > On 8 October 2014 14:23, Suriyan Ramasami <suriyan.r at 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 at 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


More information about the U-Boot mailing list