[U-Boot] [PATCH v7 5/6] fs: API changes to enable extra parameter to return size of type loff_t

Suriyan Ramasami suriyan.r at gmail.com
Fri Nov 14 22:04:01 CET 2014


On Tue, Nov 11, 2014 at 4:55 PM, Simon Glass <sjg at chromium.org> wrote:
> On 10 November 2014 13:17, Suriyan Ramasami <suriyan.r at gmail.com> wrote:
>> The sandbox/ext4/fat/generic fs commands do not gracefully deal with file
>> greater than 2GB. Negative values are returned in such cases.
>>
>> To handle this, the fs functions have been modified to take an additional
>> parameter of type "* loff_t" which is then populated. The return value of
>> the fs fucntions are used only for error conditions.
>>
>> Signed-off-by: Suriyan Ramasami <suriyan.r at gmail.com>
>
> A few nits below, but:
>
> Acked-by: Simon Glass <sjg at chromium.org>
>
>>
>> ---
>>
>> Changes in v7:
>> * Simon - API change in separate patch
>>
>>  common/cmd_fs.c        | 17 +++++++++++
>>  fs/ext4/ext4fs.c       | 22 ++++-----------
>>  fs/fat/fat.c           | 20 ++++---------
>>  fs/fs.c                | 77 ++++++++++++++++++++++++++++++--------------------
>>  fs/sandbox/sandboxfs.c | 29 +++++++------------
>>  include/ext4fs.h       |  5 ++--
>>  include/fat.h          |  5 ++--
>>  include/fs.h           | 41 ++++++++++++++++-----------
>>  include/sandboxfs.h    |  8 ++++--
>>  9 files changed, 121 insertions(+), 103 deletions(-)
>>
>> diff --git a/common/cmd_fs.c b/common/cmd_fs.c
>> index 6754340..0d9da11 100644
>> --- a/common/cmd_fs.c
>> +++ b/common/cmd_fs.c
>> @@ -51,6 +51,23 @@ U_BOOT_CMD(
>>         "      If 'pos' is 0 or omitted, the file is read from the start."
>>  )
>>
>> +static int do_save_wrapper(cmd_tbl_t *cmdtp, int flag, int argc,
>> +                               char * const argv[])
>> +{
>> +       return do_save(cmdtp, flag, argc, argv, FS_TYPE_ANY);
>> +}
>> +
>> +U_BOOT_CMD(
>> +       save,   7,      0,      do_save_wrapper,
>> +       "save file to a filesystem",
>> +       "<interface> <dev[:part]> <addr> <filename> bytes [pos]\n"
>> +       "    - Save binary file 'filename' to partition 'part' on device\n"
>> +       "      type 'interface' instance 'dev' from addr 'addr' in memory.\n"
>> +       "      'bytes' gives the size to save in bytes and is mandatory.\n"
>> +       "      'pos' gives the file byte position to start writing to.\n"
>> +       "      If 'pos' is 0 or omitted, the file is written from the start."
>> +)
>> +
>>  static int do_ls_wrapper(cmd_tbl_t *cmdtp, int flag, int argc,
>>                                 char * const argv[])
>>  {
>> diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c
>> index dcb4151..ae1c47d 100644
>> --- a/fs/ext4/ext4fs.c
>> +++ b/fs/ext4/ext4fs.c
>> @@ -184,16 +184,9 @@ int ext4fs_exists(const char *filename)
>>         return ret == 0;
>>  }
>>
>> -int ext4fs_size(const char *filename)
>> +int ext4fs_size(const char *filename, loff_t *size)
>>  {
>> -       loff_t  size;
>> -       int ret;
>> -
>> -       ret = ext4fs_open(filename, &size);
>> -       if (ret)
>> -               return ret;
>> -       else
>> -               return size;
>> +       return ext4fs_open(filename, size);
>>  }
>>
>>  int ext4fs_read(char *buf, loff_t len, loff_t *actread)
>> @@ -217,10 +210,10 @@ int ext4fs_probe(block_dev_desc_t *fs_dev_desc,
>>         return 0;
>>  }
>>
>> -int ext4_read_file(const char *filename, void *buf, int offset, int len)
>> +int ext4_read_file(const char *filename, void *buf, loff_t offset, loff_t len,
>> +                  loff_t *len_read)
>>  {
>>         loff_t file_len;
>> -       loff_t len_read;
>>         int ret;
>>
>>         if (offset != 0) {
>> @@ -237,10 +230,7 @@ int ext4_read_file(const char *filename, void *buf, int offset, int len)
>>         if (len == 0)
>>                 len = file_len;
>>
>> -       ret = ext4fs_read(buf, len, &len_read);
>> +       ret = ext4fs_read(buf, len, len_read);
>>
>> -       if (ret)
>> -               return ret;
>> -       else
>> -               return len_read;
>> +       return ret;
>
> Probably don't need ret here
>

I shall change it to:
return ext4fs_read(buf, len, len_read);
and get rid of ret.

>>  }
>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>> index 29d0825..993b1f2 100644
>> --- a/fs/fat/fat.c
>> +++ b/fs/fat/fat.c
>> @@ -1248,16 +1248,9 @@ int fat_exists(const char *filename)
>>         return ret == 0;
>>  }
>>
>> -int fat_size(const char *filename)
>> +int fat_size(const char *filename, loff_t *size)
>>  {
>> -loff_t size;
>> -int ret;
>> -
>> -       ret = do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1, &size);
>> -       if (ret)
>> -               return ret;
>> -       else
>> -               return size;
>> +       return do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1, size);
>>  }
>>
>>  int file_fat_read_at(const char *filename, loff_t pos, void *buffer,
>> @@ -1280,18 +1273,17 @@ int ret;
>>                 return actread;
>>  }
>>
>> -int fat_read_file(const char *filename, void *buf, int offset, int len)
>> +int fat_read_file(const char *filename, void *buf, loff_t offset, loff_t len,
>> +                 loff_t *actread)
>>  {
>>         int ret;
>> -       loff_t actread;
>>
>> -       ret = file_fat_read_at(filename, offset, buf, len, &actread);
>> +       ret = file_fat_read_at(filename, offset, buf, len, actread);
>>         if (ret) {
>>                 printf("** Unable to read file %s **\n", filename);
>> -               return ret;
>>         }
>
> Remove {} around that
>

OK

>>
>> -       return actread;
>> +       return ret;
>>  }
>>
>>  void fat_close(void)
>> diff --git a/fs/fs.c b/fs/fs.c
>> index dd680f3..0f5a1f4 100644
>> --- a/fs/fs.c
>> +++ b/fs/fs.c
>> @@ -46,19 +46,21 @@ 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, loff_t *size)
>>  {
>>         return -1;
>>  }
>>
>>  static inline int fs_read_unsupported(const char *filename, void *buf,
>> -                                     int offset, int len)
>> +                                     loff_t offset, loff_t len,
>> +                                     loff_t *actread)
>>  {
>>         return -1;
>>  }
>>
>>  static inline int fs_write_unsupported(const char *filename, void *buf,
>> -                                     int offset, int len)
>> +                                     loff_t offset, loff_t len,
>> +                                     loff_t *actwrite)
>>  {
>>         return -1;
>>  }
>> @@ -82,9 +84,11 @@ struct fstype_info {
>>                      disk_partition_t *fs_partition);
>>         int (*ls)(const char *dirname);
>>         int (*exists)(const char *filename);
>> -       int (*size)(const char *filename);
>> -       int (*read)(const char *filename, void *buf, int offset, int len);
>> -       int (*write)(const char *filename, void *buf, int offset, int len);
>> +       int (*size)(const char *filename, loff_t *size);
>> +       int (*read)(const char *filename, void *buf, loff_t offset,
>> +                   loff_t len, loff_t *actread);
>> +       int (*write)(const char *filename, void *buf, loff_t offset,
>> +                    loff_t len, loff_t *actwrite);
>>         void (*close)(void);
>>  };
>>
>> @@ -99,7 +103,11 @@ static struct fstype_info fstypes[] = {
>>                 .exists = fat_exists,
>>                 .size = fat_size,
>>                 .read = fat_read_file,
>> +#ifdef CONFIG_FAT_WRITE
>> +               .write = file_fat_write,
>> +#else
>>                 .write = fs_write_unsupported,
>> +#endif
>>         },
>>  #endif
>>  #ifdef CONFIG_FS_EXT4
>> @@ -112,7 +120,11 @@ static struct fstype_info fstypes[] = {
>>                 .exists = ext4fs_exists,
>>                 .size = ext4fs_size,
>>                 .read = ext4_read_file,
>> +#ifdef CONFIG_CMD_EXT4_WRITE
>> +               .write = ext4_write_file,
>> +#else
>>                 .write = fs_write_unsupported,
>> +#endif
>>         },
>>  #endif
>>  #ifdef CONFIG_SANDBOX
>> @@ -233,20 +245,21 @@ int fs_exists(const char *filename)
>>         return ret;
>>  }
>>
>> -int fs_size(const char *filename)
>> +int fs_size(const char *filename, loff_t *size)
>>  {
>>         int ret;
>>
>>         struct fstype_info *info = fs_get_info(fs_type);
>>
>> -       ret = info->size(filename);
>> +       ret = info->size(filename, size);
>>
>>         fs_close();
>>
>>         return ret;
>>  }
>>
>> -int fs_read(const char *filename, ulong addr, int offset, int len)
>> +int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
>> +           loff_t *actread)
>>  {
>>         struct fstype_info *info = fs_get_info(fs_type);
>>         void *buf;
>> @@ -257,11 +270,11 @@ int fs_read(const char *filename, ulong addr, int offset, int len)
>>          * means read the whole file.
>>          */
>>         buf = map_sysmem(addr, len);
>> -       ret = info->read(filename, buf, offset, len);
>> +       ret = info->read(filename, buf, offset, len, actread);
>>         unmap_sysmem(buf);
>>
>>         /* If we requested a specific number of bytes, check we got it */
>> -       if (ret >= 0 && len && ret != len) {
>> +       if (ret == 0 && len && *actread != len) {
>>                 printf("** Unable to read file %s **\n", filename);
>>                 ret = -1;
>>         }
>> @@ -270,17 +283,18 @@ int fs_read(const char *filename, ulong addr, int offset, int len)
>>         return ret;
>>  }
>>
>> -int fs_write(const char *filename, ulong addr, int offset, int len)
>> +int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len,
>> +            loff_t *actwrite)
>>  {
>>         struct fstype_info *info = fs_get_info(fs_type);
>>         void *buf;
>>         int ret;
>>
>>         buf = map_sysmem(addr, len);
>> -       ret = info->write(filename, buf, offset, len);
>> +       ret = info->write(filename, buf, offset, len, actwrite);
>>         unmap_sysmem(buf);
>>
>> -       if (ret >= 0 && ret != len) {
>> +       if (ret < 0 && len != *actwrite) {
>>                 printf("** Unable to write file %s **\n", filename);
>>                 ret = -1;
>>         }
>> @@ -292,7 +306,7 @@ 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;
>> +       loff_t size;
>>
>>         if (argc != 4)
>>                 return CMD_RET_USAGE;
>> @@ -300,8 +314,7 @@ 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)
>> +       if (fs_size(argv[3], &size) < 0)
>>                 return CMD_RET_FAILURE;
>>
>>         setenv_hex("filesize", size);
>> @@ -315,9 +328,10 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
>>         unsigned long addr;
>>         const char *addr_str;
>>         const char *filename;
>> -       unsigned long bytes;
>> -       unsigned long pos;
>> -       int len_read;
>> +       loff_t bytes;
>> +       loff_t pos;
>> +       loff_t len_read;
>> +       int ret;
>>         unsigned long time;
>>         char *ep;
>>
>> @@ -359,12 +373,12 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
>>                 pos = 0;
>>
>>         time = get_timer(0);
>> -       len_read = fs_read(filename, addr, pos, bytes);
>> +       ret = fs_read(filename, addr, pos, bytes, &len_read);
>>         time = get_timer(time);
>> -       if (len_read <= 0)
>> +       if (ret < 0)
>>                 return 1;
>>
>> -       printf("%d bytes read in %lu ms", len_read, time);
>> +       printf("%llu bytes read in %lu ms", len_read, time);
>>         if (time > 0) {
>>                 puts(" (");
>>                 print_size(len_read / time * 1000, "/s");
>> @@ -408,9 +422,10 @@ int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
>>  {
>>         unsigned long addr;
>>         const char *filename;
>> -       unsigned long bytes;
>> -       unsigned long pos;
>> -       int len;
>> +       loff_t bytes;
>> +       loff_t pos;
>> +       loff_t len;
>> +       int ret;
>>         unsigned long time;
>>
>>         if (argc < 6 || argc > 7)
>> @@ -419,8 +434,8 @@ int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
>>         if (fs_set_blk_dev(argv[1], argv[2], fstype))
>>                 return 1;
>>
>> -       filename = argv[3];
>> -       addr = simple_strtoul(argv[4], NULL, 16);
>> +       addr = simple_strtoul(argv[3], NULL, 16);
>> +       filename = argv[4];
>>         bytes = simple_strtoul(argv[5], NULL, 16);
>>         if (argc >= 7)
>>                 pos = simple_strtoul(argv[6], NULL, 16);
>> @@ -428,12 +443,12 @@ int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
>>                 pos = 0;
>>
>>         time = get_timer(0);
>> -       len = fs_write(filename, addr, pos, bytes);
>> +       ret = fs_write(filename, addr, pos, bytes, &len);
>>         time = get_timer(time);
>> -       if (len <= 0)
>> +       if (ret < 0)
>>                 return 1;
>>
>> -       printf("%d bytes written in %lu ms", len, time);
>> +       printf("%llu bytes written in %lu ms", len, time);
>>         if (time > 0) {
>>                 puts(" (");
>>                 print_size(len / time * 1000, "/s");
>> diff --git a/fs/sandbox/sandboxfs.c b/fs/sandbox/sandboxfs.c
>> index 312caec..ca25104 100644
>> --- a/fs/sandbox/sandboxfs.c
>> +++ b/fs/sandbox/sandboxfs.c
>> @@ -103,46 +103,37 @@ int sandbox_fs_exists(const char *filename)
>>         return ret == 0;
>>  }
>>
>> -int sandbox_fs_size(const char *filename)
>> +int sandbox_fs_size(const char *filename, loff_t *size)
>>  {
>> -       loff_t size;
>> -       int ret;
>> -
>> -       ret = os_get_filesize(filename, &size);
>> -       if (ret)
>> -               return ret;
>> -       else
>> -               return size;
>> +       return os_get_filesize(filename, size);
>>  }
>>
>>  void sandbox_fs_close(void)
>>  {
>>  }
>>
>> -int fs_read_sandbox(const char *filename, void *buf, int offset, int len)
>> +int fs_read_sandbox(const char *filename, void *buf, loff_t offset, loff_t len,
>> +                   loff_t *actread)
>>  {
>>         int ret;
>> -       loff_t actread;
>>
>> -       ret = sandbox_fs_read_at(filename, offset, buf, len, &actread);
>> +       ret = sandbox_fs_read_at(filename, offset, buf, len, actread);
>>         if (ret) {
>>                 printf("** Unable to read file %s **\n", filename);
>> -               return ret;
>>         }
>
> Remove {}
>

OK

>>
>> -       return actread;
>> +       return ret;
>>  }
>>
>> -int fs_write_sandbox(const char *filename, void *buf, int offset, int len)
>> +int fs_write_sandbox(const char *filename, void *buf, loff_t offset,
>> +                    loff_t len, loff_t *actwrite)
>>  {
>>         int ret;
>> -       loff_t actwrite;
>>
>> -       ret = sandbox_fs_write_at(filename, offset, buf, len, &actwrite);
>> +       ret = sandbox_fs_write_at(filename, offset, buf, len, actwrite);
>>         if (ret) {
>>                 printf("** Unable to write file %s **\n", filename);
>> -               return ret;
>>         }
>
> Remove {}
>

OK

>>
>> -       return actwrite;
>> +       return ret;
>>  }
>> diff --git a/include/ext4fs.h b/include/ext4fs.h
>> index 66841e7..7630057 100644
>> --- a/include/ext4fs.h
>> +++ b/include/ext4fs.h
>> @@ -138,13 +138,14 @@ 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, loff_t *size);
>>  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);
>>  long int read_allocated_block(struct ext2_inode *inode, int fileblock);
>>  int ext4fs_probe(block_dev_desc_t *fs_dev_desc,
>>                  disk_partition_t *fs_partition);
>> -int ext4_read_file(const char *filename, void *buf, int offset, int len);
>> +int ext4_read_file(const char *filename, void *buf, loff_t offset, loff_t len,
>> +                  loff_t *actread);
>>  int ext4_read_superblock(char *buffer);
>>  #endif
>> diff --git a/include/fat.h b/include/fat.h
>> index 99c6429..3038bd7 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, loff_t *size);
>>  int file_fat_read_at(const char *filename, loff_t pos, void *buffer,
>>                      loff_t maxsize, loff_t *actread);
>>  int file_fat_read(const char *filename, void *buffer, int maxsize);
>> @@ -208,6 +208,7 @@ int fat_register_device(block_dev_desc_t *dev_desc, int part_no);
>>
>>  int file_fat_write(const char *filename, void *buf, loff_t offset, loff_t len,
>>                    loff_t *actwrite);
>> -int fat_read_file(const char *filename, void *buf, int offset, int len);
>> +int fat_read_file(const char *filename, void *buf, loff_t offset, loff_t len,
>> +                 loff_t *actread);
>>  void fat_close(void);
>>  #endif /* _FAT_H_ */
>> diff --git a/include/fs.h b/include/fs.h
>> index 06a45f2..95966dd 100644
>> --- a/include/fs.h
>> +++ b/include/fs.h
>> @@ -51,32 +51,41 @@ int fs_ls(const char *dirname);
>>  int fs_exists(const char *filename);
>>
>>  /*
>> - * Determine a file's size
>> + * fs_size - Determine a file's size
>>   *
>> - * Returns the file's size in bytes, or a negative value if it doesn't exist.
>> + * @filename: Name of the file
>> + * @size: Size of file
>> + * @return 0 if ok with valid *size, negative on error
>>   */
>> -int fs_size(const char *filename);
>> +int fs_size(const char *filename, loff_t *size);
>>
>>  /*
>> - * Read file "filename" from the partition previously set by fs_set_blk_dev(),
>> - * to address "addr", starting at byte offset "offset", and reading "len"
>> - * bytes. "offset" may be 0 to read from the start of the file. "len" may be
>> - * 0 to read the entire file. Note that not all filesystem types support
>> - * either/both offset!=0 or len!=0.
>> + * fs_read - Read file from the partition previously set by fs_set_blk_dev()
>> + * Note that not all filesystem types support either/both offset!=0 or len!=0.
>>   *
>> - * Returns number of bytes read on success. Returns <= 0 on error.
>> + * @filename: Name of file to read from
>> + * @addr: The address to read into
>> + * @offset: The offset in file to read from
>> + * @len: The number of bytes to read. Maybe 0 to read entire file
>> + * @actread: Returns the actual number of bytes read
>> + * @return 0 if ok with valid *actread, -1 on error conditions
>>   */
>> -int fs_read(const char *filename, ulong addr, int offset, int len);
>> +int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len,
>> +           loff_t *actread);
>>
>>  /*
>> - * Write file "filename" to the partition previously set by fs_set_blk_dev(),
>> - * from address "addr", starting at byte offset "offset", and writing "len"
>> - * bytes. "offset" may be 0 to write to the start of the file. Note that not
>> - * all filesystem types support offset!=0.
>> + * fs_write - Write file to the partition previously set by fs_set_blk_dev()
>> + * Note that not all filesystem types support offset!=0.
>>   *
>> - * Returns number of bytes read on success. Returns <= 0 on error.
>> + * @filename: Name of file to read from
>> + * @addr: The address to read into
>> + * @offset: The offset in file to read from. Maybe 0 to write to start of file
>> + * @len: The number of bytes to write
>> + * @actwrite: Returns the actual number of bytes written
>> + * @return 0 if ok with valid *actwrite, -1 on error conditions
>>   */
>> -int fs_write(const char *filename, ulong addr, int offset, int len);
>> +int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len,
>> +            loff_t *actwrite);
>>
>>  /*
>>   * Common implementation for various filesystem commands, optionally limited
>> diff --git a/include/sandboxfs.h b/include/sandboxfs.h
>> index 8e5da6b..4c7745d 100644
>> --- a/include/sandboxfs.h
>> +++ b/include/sandboxfs.h
>> @@ -28,8 +28,10 @@ int sandbox_fs_write_at(const char *filename, loff_t pos, void *buffer,
>>  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 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);
>> +int sandbox_fs_size(const char *filename, loff_t *size);
>> +int fs_read_sandbox(const char *filename, void *buf, loff_t offset, loff_t len,
>> +                   loff_t *actread);
>> +int fs_write_sandbox(const char *filename, void *buf, loff_t offset,
>> +                    loff_t len, loff_t *actwrite);
>>
>>  #endif
>> --
>> 1.9.1
>>
>
> Regards,
> Simon

Thanks for the review!
- Suriyan


More information about the U-Boot mailing list