| Message ID | 1420478019-18877-2-git-send-email-sjoerd.simons@collabora.co.uk |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
On 01/05/2015 10:13 AM, Sjoerd Simons wrote: > New command to determine the filesystem type of a given partition. > Optionally stores the filesystem type in a environment variable. > diff --git a/common/cmd_fs.c b/common/cmd_fs.c > +U_BOOT_CMD( > + fstype, 4, 1, do_fstype_wrapper, > + "Look up a filesystem type", > + "<interface> <dev>:<part>\n" Should this line ... > + "- print filesystem type\n" > + "fstype <interface> <dev>:<part> <varname>\n" ... be consistent with this one - namely either both or neither include "fstype" at the start? > diff --git a/fs/fs.c b/fs/fs.c > +int do_fs_type(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > +{ > + struct fstype_info *info; > + > + if (argc < 3 || argc > 4) > + return CMD_RET_USAGE; > + > + if (fs_set_blk_dev(argv[1], argv[2], FS_TYPE_ANY)) > + return 1; > + > + info = fs_get_info(fs_type); > + > + if (argc == 4) > + setenv(argv[3], info->name); > + else > + printf("%s\n", info->name); > + > + return CMD_RET_SUCCESS; > +} That function has both the cmdline interface and implementation logic in one place. Many of the other features (read, write, ls, ...) separate them out into two functions so that U-Boot code can call the implementation, and shell code can call the cmdline parsing wrapper. Should we do the same here? Admittedly, the implementation would be pretty simple, but perhaps it's still useful? I don't feel that strongly though, and we can easily refactor that later if required.
On Mon, 2015-01-05 at 13:18 -0700, Stephen Warren wrote: > On 01/05/2015 10:13 AM, Sjoerd Simons wrote: > > New command to determine the filesystem type of a given partition. > > Optionally stores the filesystem type in a environment variable. > > > diff --git a/common/cmd_fs.c b/common/cmd_fs.c > > > +U_BOOT_CMD( > > + fstype, 4, 1, do_fstype_wrapper, > > + "Look up a filesystem type", > > + "<interface> <dev>:<part>\n" > > Should this line ... > > > + "- print filesystem type\n" > > + "fstype <interface> <dev>:<part> <varname>\n" > > ... be consistent with this one - namely either both or neither include > "fstype" at the start? Nope, the cmd_usage implementation does (summarized): printf("Usage:\n%s ", cmdtp->name); puts(cmdtp->help); putc('\n'); So the "fstype" at the start of the first line gets added by that code, hence the declaration needs to be inconsistent to have a consistent output for the user :) > > diff --git a/fs/fs.c b/fs/fs.c > > > +int do_fs_type(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > +{ > > + struct fstype_info *info; > > + > > + if (argc < 3 || argc > 4) > > + return CMD_RET_USAGE; > > + > > + if (fs_set_blk_dev(argv[1], argv[2], FS_TYPE_ANY)) > > + return 1; > > + > > + info = fs_get_info(fs_type); > > + > > + if (argc == 4) > > + setenv(argv[3], info->name); > > + else > > + printf("%s\n", info->name); > > + > > + return CMD_RET_SUCCESS; > > +} > > That function has both the cmdline interface and implementation logic in > one place. Many of the other features (read, write, ls, ...) separate > them out into two functions so that U-Boot code can call the > implementation, and shell code can call the cmdline parsing wrapper. > Should we do the same here? Admittedly, the implementation would be > pretty simple, but perhaps it's still useful? Ah i did wonder why the splitup was there in some functions, that explains it :). I would expect that u-boot code which would like to use it would be more interested in getting the fstype struct rather then necessarily the name as a string (or printed out).. But i could well be wrong. > I don't feel that strongly though, and we can easily refactor that later > if required. Same here, although i'd slightly prefer refactoring if-needed rather then pre-emptively :)
On 01/06/2015 09:40 AM, Sjoerd Simons wrote: > On Mon, 2015-01-05 at 13:18 -0700, Stephen Warren wrote: >> On 01/05/2015 10:13 AM, Sjoerd Simons wrote: >>> New command to determine the filesystem type of a given partition. >>> Optionally stores the filesystem type in a environment variable. >> >>> diff --git a/common/cmd_fs.c b/common/cmd_fs.c >> >>> +U_BOOT_CMD( >>> + fstype, 4, 1, do_fstype_wrapper, >>> + "Look up a filesystem type", >>> + "<interface> <dev>:<part>\n" >> >> Should this line ... >> >>> + "- print filesystem type\n" >>> + "fstype <interface> <dev>:<part> <varname>\n" >> >> ... be consistent with this one - namely either both or neither include >> "fstype" at the start? > > Nope, the cmd_usage implementation does (summarized): > > printf("Usage:\n%s ", cmdtp->name); > puts(cmdtp->help); > putc('\n'); > > So the "fstype" at the start of the first line gets added by that code, > hence the declaration needs to be inconsistent to have a consistent > output for the user :) Ah right. In that case, Reviewed-by: Stephen Warren <swarren@nvidia.com>
On Mon, Jan 05, 2015 at 06:13:36PM +0100, Sjoerd Simons wrote: > New command to determine the filesystem type of a given partition. > Optionally stores the filesystem type in a environment variable. > > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk> > Reviewed-by: Stephen Warren <swarren@nvidia.com> Applied to u-boot/master, thanks!
diff --git a/common/cmd_fs.c b/common/cmd_fs.c index 0d9da11..e146254 100644 --- a/common/cmd_fs.c +++ b/common/cmd_fs.c @@ -81,3 +81,18 @@ U_BOOT_CMD( " - List files in directory 'directory' of partition 'part' on\n" " device type 'interface' instance 'dev'." ) + +static int do_fstype_wrapper(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + return do_fs_type(cmdtp, flag, argc, argv); +} + +U_BOOT_CMD( + fstype, 4, 1, do_fstype_wrapper, + "Look up a filesystem type", + "<interface> <dev>:<part>\n" + "- print filesystem type\n" + "fstype <interface> <dev>:<part> <varname>\n" + "- set environment variable to filesystem type\n" +); diff --git a/fs/fs.c b/fs/fs.c index ddd751c..483273f 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -79,6 +79,7 @@ static inline int fs_uuid_unsupported(char *uuid_str) struct fstype_info { int fstype; + char *name; /* * Is it legal to pass NULL as .probe()'s fs_dev_desc parameter? This * should be false in most cases. For "virtual" filesystems which @@ -105,6 +106,7 @@ static struct fstype_info fstypes[] = { #ifdef CONFIG_FS_FAT { .fstype = FS_TYPE_FAT, + .name = "fat", .null_dev_desc_ok = false, .probe = fat_set_blk_dev, .close = fat_close, @@ -123,6 +125,7 @@ static struct fstype_info fstypes[] = { #ifdef CONFIG_FS_EXT4 { .fstype = FS_TYPE_EXT, + .name = "ext4", .null_dev_desc_ok = false, .probe = ext4fs_probe, .close = ext4fs_close, @@ -141,6 +144,7 @@ static struct fstype_info fstypes[] = { #ifdef CONFIG_SANDBOX { .fstype = FS_TYPE_SANDBOX, + .name = "sandbox", .null_dev_desc_ok = true, .probe = sandbox_fs_set_blk_dev, .close = sandbox_fs_close, @@ -154,6 +158,7 @@ static struct fstype_info fstypes[] = { #endif { .fstype = FS_TYPE_ANY, + .name = "unsupported", .null_dev_desc_ok = true, .probe = fs_probe_unsupported, .close = fs_close_unsupported, @@ -190,6 +195,7 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype) if (!relocated) { for (i = 0, info = fstypes; i < ARRAY_SIZE(fstypes); i++, info++) { + info->name += gd->reloc_off; info->probe += gd->reloc_off; info->close += gd->reloc_off; info->ls += gd->reloc_off; @@ -503,3 +509,24 @@ int do_fs_uuid(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], return CMD_RET_SUCCESS; } + +int do_fs_type(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + struct fstype_info *info; + + if (argc < 3 || argc > 4) + return CMD_RET_USAGE; + + if (fs_set_blk_dev(argv[1], argv[2], FS_TYPE_ANY)) + return 1; + + info = fs_get_info(fs_type); + + if (argc == 4) + setenv(argv[3], info->name); + else + printf("%s\n", info->name); + + return CMD_RET_SUCCESS; +} + diff --git a/include/fs.h b/include/fs.h index ffb6ce7..fd1e4ab 100644 --- a/include/fs.h +++ b/include/fs.h @@ -109,4 +109,10 @@ int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], int do_fs_uuid(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], int fstype); +/* + * Determine the type of the specified filesystem and print it. Optionally it is + * possible to store the type directly in env. + */ +int do_fs_type(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]); + #endif /* _FS_H */
New command to determine the filesystem type of a given partition. Optionally stores the filesystem type in a environment variable. Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk> --- common/cmd_fs.c | 15 +++++++++++++++ fs/fs.c | 27 +++++++++++++++++++++++++++ include/fs.h | 6 ++++++ 3 files changed, 48 insertions(+)