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

On Wed, Oct 8, 2014 at 1:44 PM, Simon Glass sjg@chromium.org wrote:

Hi Suriyan,

On 8 October 2014 14:23, Suriyan Ramasami suriyan.r@gmail.com wrote:

...

The commands fatls/ext4ls give -ve values when dealing with files > 2GB. The commands fatsize/ext4size do not update the variable filesize for these files.

To deal with this, the functions *_size have been modified to take a second parameter of type "* off_t" which is then populated. The return value of the *_size function is then only used to determine error conditions.

That seems like a sensible idea to me.

Hello Simon, I got the reply from Pavel as I was writing this. So, what do you think of just changing the return value of these functions to off64_t ?

...

Signed-off-by: Suriyan Ramasami suriyan.r@gmail.com

arch/sandbox/cpu/os.c | 14 +++++------ arch/sandbox/cpu/state.c | 6 ++--- common/board_f.c | 6 ++--- fs/ext4/ext4_common.c | 13 +++++------ fs/ext4/ext4fs.c | 21 ++++++++++------- fs/fat/fat.c | 61 ++++++++++++++++++++++++++++++++---------------- fs/fs.c | 15 ++++++------ fs/sandbox/sandboxfs.c | 23 ++++++++++++------ include/ext4fs.h | 4 ++-- include/fat.h | 2 +- include/fs.h | 2 +- include/os.h | 2 +- include/sandboxfs.h | 2 +- 13 files changed, 103 insertions(+), 68 deletions(-)

Thanks for doing this. Do you think you could also add a test for this (perhaps in test/fs). It could create a temporary ext2 filesystem, put a large file in it and then check a few operations.

Thanks for pointing out the test directory. I was unaware of its existence. I shall indeed add some test cases.

...

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index 1c4aa3f..9b052ee 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -385,15 +385,15 @@ const char *os_dirent_get_typename(enum os_dirent_t type) return os_dirent_typename[OS_FILET_UNKNOWN]; }

-ssize_t os_get_filesize(const char *fname) +int os_get_filesize(const char *fname, off_t *size) { struct stat buf; int ret;

    ret = stat(fname, &buf);

•   if (ret)
  

•           return ret;
  

•   return buf.st_size;
  

•   if (ret == 0)
  

•           *size = buf.st_size;
  

•   return ret;
  

}

void os_putc(int ch) @@ -427,10 +427,10 @@ int os_read_ram_buf(const char *fname) { struct sandbox_state *state = state_get_current(); int fd, ret;

•   int size;
  

•   off_t size;
  
  

•   size = os_get_filesize(fname);
  

•   if (size < 0)
  

•   ret = os_get_filesize(fname, &size);
  

•   if (ret < 0)
            return -ENOENT;
  

Should you return ret instead here (and in other cases below)?

I wanted to keep the return values as consistent with the original, so as to preserve same behavior before this code change. os_get_filesize() calls the stat() function in sandbox, and that has quite a many failure return values. I did not check if they are handled by the callers of os_read_ram_buf(). I am OK to change it, but then have to follow up on all the callers of os_read_ram_buf() to see if they are indeed gracefully handling the error conditions.

This is also the reasoning (potentially flawed) in general that I followed when the *_size() functions are being used by calls which do the actual read from the file. In those cases, I have reverted to previous behavior where I return an int as before, assuming that a call to read more than 2G of data might not be issued.

I am assuming, to be clean, more effort in consistently using size_t would be apt all across the code. Right?

...

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

diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c index 59adad6..e0d119a 100644 --- a/arch/sandbox/cpu/state.c +++ b/arch/sandbox/cpu/state.c @@ -49,12 +49,12 @@ static int state_ensure_space(int extra_size)

static int state_read_file(struct sandbox_state *state, const char *fname) {

•   int size;
  

•   off_t size;
    int ret;
    int fd;
  
  

•   size = os_get_filesize(fname);
  

•   if (size < 0) {
  

•   ret = os_get_filesize(fname, &size);
  

•   if (ret < 0) {
            printf("Cannot find sandbox state file '%s'\n", fname);
            return -ENOENT;
    }
  

diff --git a/common/board_f.c b/common/board_f.c index e6aa298..b8ec7ac 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -291,7 +291,7 @@ static int read_fdt_from_file(void) struct sandbox_state *state = state_get_current(); const char *fname = state->fdt_fname; void *blob;

•   ssize_t size;
  

•   off_t size;
    int err;
    int fd;
  
  

@@ -304,8 +304,8 @@ static int read_fdt_from_file(void) return -EINVAL; }

•   size = os_get_filesize(fname);
  

•   if (size < 0) {
  

•   err = os_get_filesize(fname, &size);
  

•   if (err < 0) {
            printf("Failed to file FDT file '%s'\n", fname);
            return -ENOENT;
    }
  

diff --git a/fs/ext4/ext4_common.c b/fs/ext4/ext4_common.c index 33d69c9..fd9b611 100644 --- a/fs/ext4/ext4_common.c +++ b/fs/ext4/ext4_common.c @@ -2003,9 +2003,9 @@ int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name, printf("< ? > "); break; }

•                           printf("%10d %s\n",
  

•                                   __le32_to_cpu(fdiro->inode.size),
  

•                                   filename);
  

•                           printf("%10u %s\n",
  

•                                  __le32_to_cpu(fdiro->inode.size),
  

•                                  filename);
                    }
                    free(fdiro);
            }
  

@@ -2169,11 +2169,10 @@ int ext4fs_find_file(const char *path, struct ext2fs_node *rootnode, return 1; }

-int ext4fs_open(const char *filename) +int ext4fs_open(const char *filename, off_t *len) { struct ext2fs_node *fdiro = NULL; int status;

•   int len;
  
    if (ext4fs_root == NULL)
            return -1;
  

@@ -2190,10 +2189,10 @@ int ext4fs_open(const char *filename) if (status == 0) goto fail; }

•   len = __le32_to_cpu(fdiro->inode.size);
  

•   *len = __le32_to_cpu(fdiro->inode.size);
    ext4fs_file = fdiro;
  
  

•   return len;
  

•   return 0;
  

fail: ext4fs_free_node(fdiro, &ext4fs_root->diropen);

diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c index cbdc220..3e4eaaa 100644 --- a/fs/ext4/ext4fs.c +++ b/fs/ext4/ext4fs.c @@ -176,15 +176,19 @@ int ext4fs_ls(const char *dirname)

int ext4fs_exists(const char *filename) {

•   int file_len;
  

•   off_t file_len;
  

•   int ret;
  
  

•   file_len = ext4fs_open(filename);
  

•   return file_len >= 0;
  

•   ret = ext4fs_open(filename, &file_len);
  

•   return ret == 0;
  

}

-int ext4fs_size(const char *filename) +int ext4fs_size(const char *filename, off_t *file_size) {

•   return ext4fs_open(filename);
  

•   int ret;
  

•   ret =  ext4fs_open(filename, file_size);
  

•   return ret == 0;
  

}

int ext4fs_read(char *buf, unsigned len) @@ -210,7 +214,8 @@ int ext4fs_probe(block_dev_desc_t *fs_dev_desc,

int ext4_read_file(const char *filename, void *buf, int offset, int len) {

•   int file_len;
  

•   off_t file_len;
  

•   int ret;
    int len_read;
  
    if (offset != 0) {
  

@@ -218,8 +223,8 @@ int ext4_read_file(const char *filename, void *buf, int offset, int len) return -1; }

•   file_len = ext4fs_open(filename);
  

•   if (file_len < 0) {
  

•   ret = ext4fs_open(filename, &file_len);
  

•   if (ret != 0) {
            printf("** File not found %s **\n", filename);
            return -1;
    }
  

diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 561921f..650ee7e 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -806,9 +806,9 @@ exit: __u8 do_fat_read_at_block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN);

-long +int do_fat_read_at(const char *filename, unsigned long pos, void *buffer,

•          unsigned long maxsize, int dols, int dogetsize)
  

•          unsigned long maxsize, int dols, int dogetsize, off_t *size)
  

{ char fnamecopy[2048]; boot_sector bs; @@ -974,10 +974,10 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, } if (doit) { if (dirc == ' ') {

•                                                           printf(" %8ld   %s%c\n",
  

•                                                                   (long)FAT2CPU32(dentptr->size),
  

•                                                                   l_name,
  

•                                                                   dirc);
  

•                                                           printf(" %8u   %s%c\n",
  

•                                                                  FAT2CPU32(dentptr->size),
  

•                                                                  l_name,
  

•                                                                  dirc);
                                                    } else {
                                                            printf("            %s%c\n",
                                                                    l_name,
  

@@ -1032,9 +1032,9 @@ do_fat_read_at(const char *filename, unsigned long pos, void *buffer, } if (doit) { if (dirc == ' ') {

•                                           printf(" %8ld   %s%c\n",
  

•                                                   (long)FAT2CPU32(dentptr->size),
  

•                                                   s_name, dirc);
  

•                                           printf(" %8u   %s%c\n",
  

•                                                  FAT2CPU32(dentptr->size),
  

•                                                  s_name, dirc);
                                    } else {
                                            printf("            %s%c\n",
                                                    s_name, dirc);
  

@@ -1153,20 +1153,28 @@ rootdir_done: }

    if (dogetsize)

•           ret = FAT2CPU32(dentptr->size);
  

•           *size = FAT2CPU32(dentptr->size);
    else
  

•           ret = get_contents(mydata, dentptr, pos, buffer, maxsize);
  

•   debug("Size: %d, got: %ld\n", FAT2CPU32(dentptr->size), ret);
  

•           *size = get_contents(mydata, dentptr, pos, buffer, maxsize);
  

•   debug("Size: %u, got: %u\n", FAT2CPU32(dentptr->size),
  

•         (unsigned) *size);
  
  

exit: free(mydata->fatbuf); return ret; }

-long +int do_fat_read(const char *filename, void *buffer, unsigned long maxsize, int dols) {

•   return do_fat_read_at(filename, 0, buffer, maxsize, dols, 0);
  

•   int ret;
  

•   off_t size;
  

•   ret = do_fat_read_at(filename, 0, buffer, maxsize, dols, 0, &size);
  

•   if (ret == 0)
  

•           return size;
  

Will this cause problems if size is >2GB?

Yes it will, but my reasoning was as I have mentioned above.

...

•   else
  

•           return ret;
  

}

int file_fat_detectfs(void) @@ -1238,21 +1246,34 @@ int file_fat_ls(const char *dir)

int fat_exists(const char *filename) {

•   int sz;
  

•   sz = do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1);
  

•   return sz >= 0;
  

•   int ret;
  

•   off_t sz;
  

•   ret = do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1, &sz);
  

•   return ret == 0;
  

}

-int fat_size(const char *filename) +int fat_size(const char *filename, off_t *sz) {

•   return do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1);
  

•   int ret;
  

•   ret =  do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1, sz);
  

•   return ret == 0;
  

Should this return an error?

I shall correct this. Missed this one :-)

...

}

long file_fat_read_at(const char *filename, unsigned long pos, void *buffer, unsigned long maxsize) {

•   int ret;
  

•   off_t sz;
  

•   printf("reading %s\n", filename);
  

•   return do_fat_read_at(filename, pos, buffer, maxsize, LS_NO, 0);
  

•   ret = do_fat_read_at(filename, pos, buffer, maxsize, LS_NO, 0, &sz);
  

•   if (ret == 0)
  

•           return sz;
  

•   else
  

•           return ret;
  

This is fine for now. It seems like in the future we should change the fs interface to return an error code for every method.

OK.

...

}

long file_fat_read(const char *filename, void *buffer, unsigned long maxsize) diff --git a/fs/fs.c b/fs/fs.c index dd680f3..c2d5b5f 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -46,7 +46,7 @@ static inline int fs_exists_unsupported(const char *filename) return 0; }

-static inline int fs_size_unsupported(const char *filename) +static inline int fs_size_unsupported(const char *filename, off_t *sz) { return -1; } @@ -82,7 +82,7 @@ struct fstype_info { disk_partition_t *fs_partition); int (*ls)(const char *dirname); int (*exists)(const char *filename);

•   int (*size)(const char *filename);
  

•   int (*size)(const char *filename, off_t *size);
    int (*read)(const char *filename, void *buf, int offset, int len);
    int (*write)(const char *filename, void *buf, int offset, int len);
    void (*close)(void);
  

@@ -233,13 +233,13 @@ int fs_exists(const char *filename) return ret; }

-int fs_size(const char *filename) +int fs_size(const char *filename, off_t *size) { int ret;

    struct fstype_info *info = fs_get_info(fs_type);

•   ret = info->size(filename);
  

•   ret = info->size(filename, size);
  
    fs_close();
  
  

@@ -292,7 +292,8 @@ int fs_write(const char *filename, ulong addr, int offset, int len) int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], int fstype) {

•   int size;
  

•   off_t size;
  

•   int ret;
  
    if (argc != 4)
            return CMD_RET_USAGE;
  

@@ -300,8 +301,8 @@ int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], if (fs_set_blk_dev(argv[1], argv[2], fstype)) return 1;

•   size = fs_size(argv[3]);
  

•   if (size < 0)
  

•   ret = fs_size(argv[3], &size);
  

•   if (ret < 0)
            return CMD_RET_FAILURE;
  
    setenv_hex("filesize", size);
  

diff --git a/fs/sandbox/sandboxfs.c b/fs/sandbox/sandboxfs.c index ba6402c..a384cc7 100644 --- a/fs/sandbox/sandboxfs.c +++ b/fs/sandbox/sandboxfs.c @@ -27,8 +27,13 @@ long sandbox_fs_read_at(const char *filename, unsigned long pos, os_close(fd); return ret; }

•   if (!maxsize)
  

•           maxsize = os_get_filesize(filename);
  

•   if (!maxsize) {
  

•           ret = os_get_filesize(filename, (off_t *)&maxsize);
  

•           if (ret < 0) {
  

•                   os_close(fd);
  

•                   return ret;
  

•           }
  

•   }
    size = os_read(fd, buffer, maxsize);
    os_close(fd);
  
  

@@ -74,15 +79,19 @@ int sandbox_fs_ls(const char *dirname)

int sandbox_fs_exists(const char *filename) {

•   ssize_t sz;
  

•   off_t sz;
  

•   int ret;
  
  

•   sz = os_get_filesize(filename);
  

•   return sz >= 0;
  

•   ret = os_get_filesize(filename, &sz);
  

•   return ret == 0;
  

}

-int sandbox_fs_size(const char *filename) +int sandbox_fs_size(const char *filename, off_t *size) {

•   return os_get_filesize(filename);
  

•   int ret;
  

•   ret = os_get_filesize(filename, size);
  

•   return ret == 0;
  

}

void sandbox_fs_close(void) diff --git a/include/ext4fs.h b/include/ext4fs.h index 6c419f3..1f04973 100644 --- a/include/ext4fs.h +++ b/include/ext4fs.h @@ -129,14 +129,14 @@ int ext4fs_write(const char *fname, unsigned char *buffer, #endif

struct ext_filesystem *get_fs(void); -int ext4fs_open(const char *filename); +int ext4fs_open(const char *filename, off_t *size); int ext4fs_read(char *buf, unsigned len); int ext4fs_mount(unsigned part_length); void ext4fs_close(void); void ext4fs_reinit_global(void); int ext4fs_ls(const char *dirname); int ext4fs_exists(const char *filename); -int ext4fs_size(const char *filename); +int ext4fs_size(const char *filename, off_t *len); void ext4fs_free_node(struct ext2fs_node *node, struct ext2fs_node *currroot); int ext4fs_devread(lbaint_t sector, int byte_offset, int byte_len, char *buf); void ext4fs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info); diff --git a/include/fat.h b/include/fat.h index 20ca3f3..56aa3b7 100644 --- a/include/fat.h +++ b/include/fat.h @@ -198,7 +198,7 @@ int file_cd(const char *path); int file_fat_detectfs(void); int file_fat_ls(const char *dir); int fat_exists(const char *filename); -int fat_size(const char *filename); +int fat_size(const char *filename, off_t *len); long file_fat_read_at(const char *filename, unsigned long pos, void *buffer, unsigned long maxsize); long file_fat_read(const char *filename, void *buffer, unsigned long maxsize); diff --git a/include/fs.h b/include/fs.h index 06a45f2..df39609 100644 --- a/include/fs.h +++ b/include/fs.h @@ -55,7 +55,7 @@ int fs_exists(const char *filename);

• Returns the file's size in bytes, or a negative value if it doesn't exist.

As mentioned above I'm not sure your size is consistent.

...

*/ -int fs_size(const char *filename); +int fs_size(const char *filename, off_t *len);

/*

• Read file "filename" from the partition previously set by fs_set_blk_dev(),

diff --git a/include/os.h b/include/os.h index 0230a7f..75d9846 100644 --- a/include/os.h +++ b/include/os.h @@ -219,7 +219,7 @@ const char *os_dirent_get_typename(enum os_dirent_t type);

• @param fname Filename to check
• @return size of file, or -1 if an error ocurred

*/ -ssize_t os_get_filesize(const char *fname); +int os_get_filesize(const char *fname, off_t *size);

/**

• Write a character to the controlling OS terminal

diff --git a/include/sandboxfs.h b/include/sandboxfs.h index e7c3262..8588a29 100644 --- a/include/sandboxfs.h +++ b/include/sandboxfs.h @@ -26,7 +26,7 @@ long sandbox_fs_read_at(const char *filename, unsigned long pos, void sandbox_fs_close(void); int sandbox_fs_ls(const char *dirname); int sandbox_fs_exists(const char *filename); -int sandbox_fs_size(const char *filename); +int sandbox_fs_size(const char *filename, off_t *size); int fs_read_sandbox(const char *filename, void *buf, int offset, int len); int fs_write_sandbox(const char *filename, void *buf, int offset, int len);

-- 1.9.1

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

Regards - Suriyan

Regards, Simon