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.

Quoted 7 lines · Suriyan Ramasami

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

Quoted 1 line

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

Quoted 9 lines · Pavel Machek

Quoted 7 lines · Suriyan Ramasami

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.

Quoted 6 lines

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

Quoted 1 line

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)

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 <[email protected]>

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

arch/sandbox/cpu/os.c +4 -4

index 1c4aa3f..9b052ee 100644
@@ -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)
 {

arch/sandbox/cpu/os.c +0 -0

        if (size != state->ram_size)
                return -ENOSPC;

arch/sandbox/cpu/state.c +3 -3

index 59adad6..e0d119a 100644
@@ -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;
        }

common/board_f.c +3 -3

index e6aa298..b8ec7ac 100644
@@ -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;
        }

fs/ext4/ext4_common.c +6 -7

index 33d69c9..fd9b611 100644
@@ -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);
 

fs/ext4/ext4fs.c +13 -8

index cbdc220..3e4eaaa 100644
@@ -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;
        }

fs/fat/fat.c +13 -13

index 561921f..650ee7e 100644
@@ -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

fs/fat/fat.c +7 -3

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

fs/fat/fat.c +1 -0

 }
 
 long file_fat_read_at(const char *filename, unsigned long pos, void *buffer,
                      unsigned long maxsize)
 {
+       int ret;

fs/fat/fat.c +0 -0

 }
 
 long file_fat_read(const char *filename, void *buffer, unsigned long maxsize)

fs/fs.c +8 -7

index dd680f3..c2d5b5f 100644
@@ -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);

fs/sandbox/sandboxfs.c +16 -7

index ba6402c..a384cc7 100644
@@ -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)

include/ext4fs.h +2 -2

index 6c419f3..1f04973 100644
@@ -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);

include/fat.h +1 -1

index 20ca3f3..56aa3b7 100644
@@ -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);

include/fs.h +0 -0

index 06a45f2..df39609 100644

include/fs.h +1 -1

  */
-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(),

include/os.h +1 -1

index 0230a7f..75d9846 100644
@@ -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

include/sandboxfs.h +1 -2

index e7c3262..8588a29 100644
@@ -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

Hi Suriyan,

Quoted 4 lines · Suriyan Ramasami

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.

Quoted 3 lines

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.

Quoted 17 lines

Signed-off-by Suriyan Ramasami <[email protected]>

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

Quoted 32 lines

arch/sandbox/cpu/os.c +5 -5

index 1c4aa3f..9b052ee 100644
@@ -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;
 

arch/sandbox/cpu/os.c int os_read_ram_buf(const char *fname) · line 434

Quoted 5 lines

arch/sandbox/cpu/os.c +2 -2

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

Quoted 202 lines

arch/sandbox/cpu/os.c +0 -0

        if (size != state->ram_size)
                return -ENOSPC;

arch/sandbox/cpu/state.c +3 -3

index 59adad6..e0d119a 100644
@@ -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;
        }

common/board_f.c +3 -3

index e6aa298..b8ec7ac 100644
@@ -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;
        }

fs/ext4/ext4_common.c +6 -7

index 33d69c9..fd9b611 100644
@@ -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);
 

fs/ext4/ext4fs.c +13 -8

index cbdc220..3e4eaaa 100644
@@ -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;
        }

fs/fat/fat.c +13 -12

index 561921f..650ee7e 100644
@@ -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;
 }
 

fs/fat/fat.c rootdir_done: · line 1175

Quoted 11 lines

fs/fat/fat.c +7 -2

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

Quoted 20 lines

fs/fat/fat.c +7 -3

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

fs/fat/fat.c int file_fat_ls(const char *dir) · line 1261

Quoted 8 lines

fs/fat/fat.c +5 -2

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

Quoted 11 lines

fs/fat/fat.c +4 -1

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

fs/fat/fat.c int file_fat_ls(const char *dir) · line 1276

Quoted 5 lines

fs/fat/fat.c +5 -0

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

Quoted 146 lines

fs/fat/fat.c +0 -0

 }
 
 long file_fat_read(const char *filename, void *buffer, unsigned long maxsize)

fs/fs.c +8 -7

index dd680f3..c2d5b5f 100644
@@ -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);

fs/sandbox/sandboxfs.c +16 -7

index ba6402c..a384cc7 100644
@@ -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)

include/ext4fs.h +2 -2

index 6c419f3..1f04973 100644
@@ -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);

include/fat.h +1 -1

index 20ca3f3..56aa3b7 100644
@@ -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);

include/fs.h +0 -0

index 06a45f2..df39609 100644

include/fs.h int fs_exists(const char *filename); · line 56

Quoted 2 lines

include/fs.h +0 -0

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

Quoted 32 lines

include/fs.h +1 -1

  */
-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(),

include/os.h +1 -1

index 0230a7f..75d9846 100644
@@ -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

include/sandboxfs.h +1 -1

index e7c3262..8588a29 100644
@@ -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);
 

Quoted 3 lines

--
1.9.1

Regards,
Simon

Quoted 11 lines · Simon Glass

Hi Suriyan,

Quoted 7 lines · Suriyan Ramasami

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.

Quoted 2 lines

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 ?

Quoted 20 lines

Quoted 4 lines

Signed-off-by Suriyan Ramasami <[email protected]>

Quoted 15 lines

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

Quoted 4 lines

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.

Quoted 32 lines

Quoted 32 lines

arch/sandbox/cpu/os.c +5 -5

index 1c4aa3f..9b052ee 100644
@@ -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;
 

Quoted 7 lines

arch/sandbox/cpu/os.c int os_read_ram_buf(const char *fname) · line 434

Quoted 5 lines

arch/sandbox/cpu/os.c +2 -2

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

Quoted 202 lines

Quoted 202 lines

arch/sandbox/cpu/os.c +0 -0

        if (size != state->ram_size)
                return -ENOSPC;

arch/sandbox/cpu/state.c +3 -3

index 59adad6..e0d119a 100644
@@ -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;
        }

common/board_f.c +3 -3

index e6aa298..b8ec7ac 100644
@@ -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;
        }

fs/ext4/ext4_common.c +6 -7

index 33d69c9..fd9b611 100644
@@ -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);
 

fs/ext4/ext4fs.c +13 -8

index cbdc220..3e4eaaa 100644
@@ -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;
        }

fs/fat/fat.c +13 -12

index 561921f..650ee7e 100644
@@ -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;
 }
 

Quoted 13 lines

fs/fat/fat.c rootdir_done: · line 1175

Quoted 11 lines

fs/fat/fat.c +7 -2

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

Quoted 20 lines

Quoted 20 lines

fs/fat/fat.c +7 -3

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

Quoted 11 lines

fs/fat/fat.c int file_fat_ls(const char *dir) · line 1261

Quoted 8 lines

fs/fat/fat.c +5 -2

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

Quoted 11 lines

Quoted 11 lines

fs/fat/fat.c +4 -1

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

Quoted 9 lines

fs/fat/fat.c int file_fat_ls(const char *dir) · line 1276

Quoted 5 lines

fs/fat/fat.c +5 -0

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

Quoted 183 lines

Quoted 146 lines

fs/fat/fat.c +0 -0

 }
 
 long file_fat_read(const char *filename, void *buffer, unsigned long maxsize)

fs/fs.c +8 -7

index dd680f3..c2d5b5f 100644
@@ -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);

fs/sandbox/sandboxfs.c +16 -7

index ba6402c..a384cc7 100644
@@ -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)

include/ext4fs.h +2 -2

index 6c419f3..1f04973 100644
@@ -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);

include/fat.h +1 -1

index 20ca3f3..56aa3b7 100644
@@ -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);

include/fs.h +0 -0

index 06a45f2..df39609 100644

include/fs.h int fs_exists(const char *filename); · line 56

Quoted 2 lines

include/fs.h +0 -0

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

Quoted 32 lines

include/fs.h +1 -1

  */
-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(),

include/os.h +1 -1

index 0230a7f..75d9846 100644
@@ -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

include/sandboxfs.h +1 -1

index e7c3262..8588a29 100644
@@ -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);
 

Quoted 4 lines

Quoted 3 lines

--
1.9.1

Thanks for taking a look. Please also let me know your comments wrt the off64_t approach.

+Tom for the question below re return values

Hi,

Quoted 13 lines · Suriyan Ramasami

Quoted 11 lines · Simon Glass

Hi Suriyan,

Quoted 7 lines · Suriyan Ramasami

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 68 lines

Quoted 20 lines

Quoted 4 lines

Signed-off-by Suriyan Ramasami <[email protected]>

Quoted 15 lines

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

Quoted 4 lines

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.

Quoted 32 lines

Quoted 32 lines

arch/sandbox/cpu/os.c +5 -5

index 1c4aa3f..9b052ee 100644
@@ -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;
 

Quoted 7 lines

arch/sandbox/cpu/os.c int os_read_ram_buf(const char *fname) · line 434

Quoted 5 lines

arch/sandbox/cpu/os.c +2 -2

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

Quoted 8 lines

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.

Quoted 468 lines

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?

Quoted 202 lines

Quoted 202 lines

arch/sandbox/cpu/os.c +0 -0

        if (size != state->ram_size)
                return -ENOSPC;

arch/sandbox/cpu/state.c +3 -3

index 59adad6..e0d119a 100644
@@ -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;
        }

common/board_f.c +3 -3

index e6aa298..b8ec7ac 100644
@@ -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;
        }

fs/ext4/ext4_common.c +6 -7

index 33d69c9..fd9b611 100644
@@ -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);
 

fs/ext4/ext4fs.c +13 -8

index cbdc220..3e4eaaa 100644
@@ -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;
        }

fs/fat/fat.c +13 -12

index 561921f..650ee7e 100644
@@ -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;
 }
 

Quoted 13 lines

fs/fat/fat.c rootdir_done: · line 1175

Quoted 11 lines

fs/fat/fat.c +7 -2

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

Quoted 20 lines

Quoted 20 lines

fs/fat/fat.c +7 -3

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

Quoted 11 lines

fs/fat/fat.c int file_fat_ls(const char *dir) · line 1261

Quoted 8 lines

fs/fat/fat.c +5 -2

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

Quoted 11 lines

Quoted 11 lines

fs/fat/fat.c +4 -1

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

Quoted 9 lines

fs/fat/fat.c int file_fat_ls(const char *dir) · line 1276

Quoted 5 lines

fs/fat/fat.c +5 -0

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

Quoted 183 lines

Quoted 146 lines

fs/fat/fat.c +0 -0

 }
 
 long file_fat_read(const char *filename, void *buffer, unsigned long maxsize)

fs/fs.c +8 -7

index dd680f3..c2d5b5f 100644
@@ -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);

fs/sandbox/sandboxfs.c +16 -7

index ba6402c..a384cc7 100644
@@ -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)

include/ext4fs.h +2 -2

index 6c419f3..1f04973 100644
@@ -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);

include/fat.h +1 -1

index 20ca3f3..56aa3b7 100644
@@ -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);

include/fs.h +0 -0

index 06a45f2..df39609 100644

include/fs.h int fs_exists(const char *filename); · line 56

Quoted 2 lines

include/fs.h +0 -0

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

Quoted 32 lines

include/fs.h +1 -1

  */
-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(),

include/os.h +1 -1

index 0230a7f..75d9846 100644
@@ -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

include/sandboxfs.h +1 -1

index e7c3262..8588a29 100644
@@ -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);
 

Quoted 7 lines

Quoted 4 lines

Quoted 3 lines

--
1.9.1

Thanks for taking a look. Please also let me know your comments wrt the off64_t approach.

Regards,
Simon

Hello Simon,

Quoted 26 lines · Simon Glass

+Tom for the question below re return values

Hi,

Quoted 13 lines · Suriyan Ramasami

Quoted 11 lines · Simon Glass

Hi Suriyan,

Quoted 7 lines · Suriyan Ramasami

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.

Quoted 7 lines

Quoted 2 lines

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 ?

Quoted 7 lines

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.