On Tue, Nov 11, 2014 at 4:55 PM, Simon Glass <s...@chromium.org> wrote: > On 10 November 2014 13:17, Suriyan Ramasami <suriya...@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 <suriya...@gmail.com> > > A few nits below, but: > > Acked-by: Simon Glass <s...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot