| Message ID | 20180720025723.6736-12-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fs: fat: extend FAT write operations | expand |
On 07/20/2018 04:57 AM, AKASHI Takahiro wrote: > "mkdir" interface is added to file operations. > This is a preparatory change as mkdir support for FAT file system > will be added in next patch. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > fs/fs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > include/fs.h | 10 ++++++++++ > 2 files changed, 55 insertions(+) > > diff --git a/fs/fs.c b/fs/fs.c > index 33808d549e..3cb6b21fe9 100644 > --- a/fs/fs.c > +++ b/fs/fs.c > @@ -105,6 +105,11 @@ static inline int fs_opendir_unsupported(const char *filename, > return -EACCES; > } > > +static inline int fs_mkdir_unsupported(const char *dirname) > +{ > + return -1; > +} > + > struct fstype_info { > int fstype; > char *name; > @@ -142,6 +147,7 @@ struct fstype_info { > int (*readdir)(struct fs_dir_stream *dirs, struct fs_dirent **dentp); > /* see fs_closedir() */ > void (*closedir)(struct fs_dir_stream *dirs); > + int (*mkdir)(const char *dirname); > }; > > static struct fstype_info fstypes[] = { > @@ -165,6 +171,7 @@ static struct fstype_info fstypes[] = { > .opendir = fat_opendir, > .readdir = fat_readdir, > .closedir = fat_closedir, > + .mkdir = fs_mkdir_unsupported, Why should we create a dummy function? Just use NULL as an indicator that the interface method is not implemented. See the use of structures for the driver model. @Simon The array fstypes[] should be replaced by a linker list to match the driver model. > }, > #endif > #ifdef CONFIG_FS_EXT4 > @@ -185,6 +192,7 @@ static struct fstype_info fstypes[] = { > #endif > .uuid = ext4fs_uuid, > .opendir = fs_opendir_unsupported, > + .mkdir = fs_mkdir_unsupported, > }, > #endif > #ifdef CONFIG_SANDBOX > @@ -201,6 +209,7 @@ static struct fstype_info fstypes[] = { > .write = fs_write_sandbox, > .uuid = fs_uuid_unsupported, > .opendir = fs_opendir_unsupported, > + .mkdir = fs_mkdir_unsupported, > }, > #endif > #ifdef CONFIG_CMD_UBIFS > @@ -217,6 +226,7 @@ static struct fstype_info fstypes[] = { > .write = fs_write_unsupported, > .uuid = fs_uuid_unsupported, > .opendir = fs_opendir_unsupported, > + .mkdir = fs_mkdir_unsupported, > }, > #endif > #ifdef CONFIG_FS_BTRFS > @@ -233,6 +243,7 @@ static struct fstype_info fstypes[] = { > .write = fs_write_unsupported, > .uuid = btrfs_uuid, > .opendir = fs_opendir_unsupported, > + .mkdir = fs_mkdir_unsupported, > }, > #endif > { > @@ -248,6 +259,7 @@ static struct fstype_info fstypes[] = { > .write = fs_write_unsupported, > .uuid = fs_uuid_unsupported, > .opendir = fs_opendir_unsupported, > + .mkdir = fs_mkdir_unsupported, > }, > }; > > @@ -498,6 +510,20 @@ void fs_closedir(struct fs_dir_stream *dirs) > } > > > +int fs_mkdir(const char *dirname) > +{ > + int ret; > + > + struct fstype_info *info = fs_get_info(fs_type); > + > + ret = info->mkdir(dirname); Please, check if mkdir is NULL before calling. > + > + fs_type = FS_TYPE_ANY; > + fs_close(); What do you want to close here if mkdir() is not implemented by the file system? Best regards Heinrich > + > + return ret; > +} > + > int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], > int fstype) > { > @@ -700,3 +726,22 @@ int do_fs_type(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > return CMD_RET_SUCCESS; > } > > +int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], > + int fstype) > +{ > + int ret; > + > + if (argc != 4) > + return CMD_RET_USAGE; > + > + if (fs_set_blk_dev(argv[1], argv[2], fstype)) > + return 1; > + > + ret = fs_mkdir(argv[3]); > + if (ret) { > + printf("** Unable to create a directory \"%s\" **\n", argv[3]); > + return 1; > + } > + > + return 0; > +} > diff --git a/include/fs.h b/include/fs.h > index 163da103b4..fbaee154dd 100644 > --- a/include/fs.h > +++ b/include/fs.h > @@ -155,6 +155,14 @@ struct fs_dirent *fs_readdir(struct fs_dir_stream *dirs); > */ > void fs_closedir(struct fs_dir_stream *dirs); > > +/* > + * fs_mkdir - Create a directory > + * > + * @filename: Name of directory to create > + * @return 0 on success, -1 on error conditions > + */ > +int fs_mkdir(const char *filename); > + > /* > * Common implementation for various filesystem commands, optionally limited > * to a specific filesystem type via the fstype parameter. > @@ -169,6 +177,8 @@ int file_exists(const char *dev_type, const char *dev_part, const char *file, > int fstype); > int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], > int fstype); > +int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], > + int fstype); > > /* > * Determine the UUID of the specified filesystem and print it. Optionally it is >
Hi, On 20 July 2018 at 11:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 07/20/2018 04:57 AM, AKASHI Takahiro wrote: >> "mkdir" interface is added to file operations. >> This is a preparatory change as mkdir support for FAT file system >> will be added in next patch. >> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> --- >> fs/fs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ >> include/fs.h | 10 ++++++++++ >> 2 files changed, 55 insertions(+) >> We need to get a proper fs test in place before we add any more stuff. Does someone waent to try converting fs-test.sh to pytest in test/py/tests ? Regards, Simon
On Mon, Jul 23, 2018 at 05:48:13PM -0600, Simon Glass wrote: > Hi, > > On 20 July 2018 at 11:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 07/20/2018 04:57 AM, AKASHI Takahiro wrote: > >> "mkdir" interface is added to file operations. > >> This is a preparatory change as mkdir support for FAT file system > >> will be added in next patch. > >> > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >> --- > >> fs/fs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > >> include/fs.h | 10 ++++++++++ > >> 2 files changed, 55 insertions(+) > >> > > We need to get a proper fs test in place before we add any more stuff. Agreed that we need more tests as part of this series. > Does someone waent to try converting fs-test.sh to pytest in test/py/tests ? I thought there was at least a first pass at that? -- Tom
On Fri, Jul 20, 2018 at 07:35:18PM +0200, Heinrich Schuchardt wrote: > On 07/20/2018 04:57 AM, AKASHI Takahiro wrote: > > "mkdir" interface is added to file operations. > > This is a preparatory change as mkdir support for FAT file system > > will be added in next patch. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > fs/fs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > > include/fs.h | 10 ++++++++++ > > 2 files changed, 55 insertions(+) > > > > diff --git a/fs/fs.c b/fs/fs.c > > index 33808d549e..3cb6b21fe9 100644 > > --- a/fs/fs.c > > +++ b/fs/fs.c > > @@ -105,6 +105,11 @@ static inline int fs_opendir_unsupported(const char *filename, > > return -EACCES; > > } > > > > +static inline int fs_mkdir_unsupported(const char *dirname) > > +{ > > + return -1; > > +} > > + > > struct fstype_info { > > int fstype; > > char *name; > > @@ -142,6 +147,7 @@ struct fstype_info { > > int (*readdir)(struct fs_dir_stream *dirs, struct fs_dirent **dentp); > > /* see fs_closedir() */ > > void (*closedir)(struct fs_dir_stream *dirs); > > + int (*mkdir)(const char *dirname); > > }; > > > > static struct fstype_info fstypes[] = { > > @@ -165,6 +171,7 @@ static struct fstype_info fstypes[] = { > > .opendir = fat_opendir, > > .readdir = fat_readdir, > > .closedir = fat_closedir, > > + .mkdir = fs_mkdir_unsupported, > > Why should we create a dummy function? Just use NULL as an indicator because I followed coding style of the existing code here. > that the interface method is not implemented. See the use of structures > for the driver model. Can any file system ever be based on driver model? > @Simon > The array fstypes[] should be replaced by a linker list to match the > driver model. > > > }, > > #endif > > #ifdef CONFIG_FS_EXT4 > > @@ -185,6 +192,7 @@ static struct fstype_info fstypes[] = { > > #endif > > .uuid = ext4fs_uuid, > > .opendir = fs_opendir_unsupported, > > + .mkdir = fs_mkdir_unsupported, > > }, > > #endif > > #ifdef CONFIG_SANDBOX > > @@ -201,6 +209,7 @@ static struct fstype_info fstypes[] = { > > .write = fs_write_sandbox, > > .uuid = fs_uuid_unsupported, > > .opendir = fs_opendir_unsupported, > > + .mkdir = fs_mkdir_unsupported, > > }, > > #endif > > #ifdef CONFIG_CMD_UBIFS > > @@ -217,6 +226,7 @@ static struct fstype_info fstypes[] = { > > .write = fs_write_unsupported, > > .uuid = fs_uuid_unsupported, > > .opendir = fs_opendir_unsupported, > > + .mkdir = fs_mkdir_unsupported, > > }, > > #endif > > #ifdef CONFIG_FS_BTRFS > > @@ -233,6 +243,7 @@ static struct fstype_info fstypes[] = { > > .write = fs_write_unsupported, > > .uuid = btrfs_uuid, > > .opendir = fs_opendir_unsupported, > > + .mkdir = fs_mkdir_unsupported, > > }, > > #endif > > { > > @@ -248,6 +259,7 @@ static struct fstype_info fstypes[] = { > > .write = fs_write_unsupported, > > .uuid = fs_uuid_unsupported, > > .opendir = fs_opendir_unsupported, > > + .mkdir = fs_mkdir_unsupported, > > }, > > }; > > > > @@ -498,6 +510,20 @@ void fs_closedir(struct fs_dir_stream *dirs) > > } > > > > > > +int fs_mkdir(const char *dirname) > > +{ > > + int ret; > > + > > + struct fstype_info *info = fs_get_info(fs_type); > > + > > + ret = info->mkdir(dirname); > > Please, check if mkdir is NULL before calling. No other place checks for NULL. Is this really required? > > + > > + fs_type = FS_TYPE_ANY; > > + fs_close(); > > What do you want to close here if mkdir() is not implemented by the file > system? I'm not quite confident here but ->close() be paired with ->probe(), which is set to be called in fs_set_blk_dev(). Thanks, -Takahiro AKASHI > Best regards > > Heinrich > > > + > > + return ret; > > +} > > + > > int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], > > int fstype) > > { > > @@ -700,3 +726,22 @@ int do_fs_type(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > return CMD_RET_SUCCESS; > > } > > > > +int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], > > + int fstype) > > +{ > > + int ret; > > + > > + if (argc != 4) > > + return CMD_RET_USAGE; > > + > > + if (fs_set_blk_dev(argv[1], argv[2], fstype)) > > + return 1; > > + > > + ret = fs_mkdir(argv[3]); > > + if (ret) { > > + printf("** Unable to create a directory \"%s\" **\n", argv[3]); > > + return 1; > > + } > > + > > + return 0; > > +} > > diff --git a/include/fs.h b/include/fs.h > > index 163da103b4..fbaee154dd 100644 > > --- a/include/fs.h > > +++ b/include/fs.h > > @@ -155,6 +155,14 @@ struct fs_dirent *fs_readdir(struct fs_dir_stream *dirs); > > */ > > void fs_closedir(struct fs_dir_stream *dirs); > > > > +/* > > + * fs_mkdir - Create a directory > > + * > > + * @filename: Name of directory to create > > + * @return 0 on success, -1 on error conditions > > + */ > > +int fs_mkdir(const char *filename); > > + > > /* > > * Common implementation for various filesystem commands, optionally limited > > * to a specific filesystem type via the fstype parameter. > > @@ -169,6 +177,8 @@ int file_exists(const char *dev_type, const char *dev_part, const char *file, > > int fstype); > > int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], > > int fstype); > > +int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], > > + int fstype); > > > > /* > > * Determine the UUID of the specified filesystem and print it. Optionally it is > > >
On Mon, Jul 23, 2018 at 07:56:58PM -0400, Tom Rini wrote: > On Mon, Jul 23, 2018 at 05:48:13PM -0600, Simon Glass wrote: > > Hi, > > > > On 20 July 2018 at 11:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > On 07/20/2018 04:57 AM, AKASHI Takahiro wrote: > > >> "mkdir" interface is added to file operations. > > >> This is a preparatory change as mkdir support for FAT file system > > >> will be added in next patch. > > >> > > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > >> --- > > >> fs/fs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > > >> include/fs.h | 10 ++++++++++ > > >> 2 files changed, 55 insertions(+) > > >> > > > > We need to get a proper fs test in place before we add any more stuff. > > Agreed that we need more tests as part of this series. Is this a new standard rule for new features? Just kidding, but testing was definitely one of my concerns partly because fs-test.sh is anyhow in an old fashion and partly because I can't find any criteria for test coverage: - white-box test or black-box test - coverage for corner (or edge) cases - some sort of stress test, etc. > > Does someone waent to try converting fs-test.sh to pytest in test/py/tests ? > > I thought there was at least a first pass at that? Well, I don't see any file system related scripts under test/py/tests. Please advise me. Thanks, -Takahiro AKASHI > -- > Tom
Hi, On 23 July 2018 at 19:45, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > On Mon, Jul 23, 2018 at 07:56:58PM -0400, Tom Rini wrote: >> On Mon, Jul 23, 2018 at 05:48:13PM -0600, Simon Glass wrote: >> > Hi, >> > >> > On 20 July 2018 at 11:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> > > On 07/20/2018 04:57 AM, AKASHI Takahiro wrote: >> > >> "mkdir" interface is added to file operations. >> > >> This is a preparatory change as mkdir support for FAT file system >> > >> will be added in next patch. >> > >> >> > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> > >> --- >> > >> fs/fs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ >> > >> include/fs.h | 10 ++++++++++ >> > >> 2 files changed, 55 insertions(+) >> > >> >> > >> > We need to get a proper fs test in place before we add any more stuff. >> >> Agreed that we need more tests as part of this series. > > Is this a new standard rule for new features? > Just kidding, but testing was definitely one of my concerns partly > because fs-test.sh is anyhow in an old fashion and partly because > I can't find any criteria for test coverage: > - white-box test or black-box test > - coverage for corner (or edge) cases > - some sort of stress test, etc. > >> > Does someone waent to try converting fs-test.sh to pytest in test/py/tests ? >> >> I thought there was at least a first pass at that? > > Well, I don't see any file system related scripts under test/py/tests. > > Please advise me. It should be possible to translate it from shell to Python without too much trouble if you able able to give it a crack. See for example: 8729d58259 test: Convert the vboot test to test/py Re testing in general, my view is: - Unit tests / smaller tests are better than large ones - We don't need stress tests for functionality (e.g. the current FS test uses filesystems that are quite large, and I think they could just be very very small and still get good test coverage and run faster) - Corner cases are an important part of tests (and easy to test with unit tests) - Use the pytest framework One day we'll enable code coverage for U-Boot sandbox and get a feel for the missing test coverage. Regards, Simon
diff --git a/fs/fs.c b/fs/fs.c index 33808d549e..3cb6b21fe9 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -105,6 +105,11 @@ static inline int fs_opendir_unsupported(const char *filename, return -EACCES; } +static inline int fs_mkdir_unsupported(const char *dirname) +{ + return -1; +} + struct fstype_info { int fstype; char *name; @@ -142,6 +147,7 @@ struct fstype_info { int (*readdir)(struct fs_dir_stream *dirs, struct fs_dirent **dentp); /* see fs_closedir() */ void (*closedir)(struct fs_dir_stream *dirs); + int (*mkdir)(const char *dirname); }; static struct fstype_info fstypes[] = { @@ -165,6 +171,7 @@ static struct fstype_info fstypes[] = { .opendir = fat_opendir, .readdir = fat_readdir, .closedir = fat_closedir, + .mkdir = fs_mkdir_unsupported, }, #endif #ifdef CONFIG_FS_EXT4 @@ -185,6 +192,7 @@ static struct fstype_info fstypes[] = { #endif .uuid = ext4fs_uuid, .opendir = fs_opendir_unsupported, + .mkdir = fs_mkdir_unsupported, }, #endif #ifdef CONFIG_SANDBOX @@ -201,6 +209,7 @@ static struct fstype_info fstypes[] = { .write = fs_write_sandbox, .uuid = fs_uuid_unsupported, .opendir = fs_opendir_unsupported, + .mkdir = fs_mkdir_unsupported, }, #endif #ifdef CONFIG_CMD_UBIFS @@ -217,6 +226,7 @@ static struct fstype_info fstypes[] = { .write = fs_write_unsupported, .uuid = fs_uuid_unsupported, .opendir = fs_opendir_unsupported, + .mkdir = fs_mkdir_unsupported, }, #endif #ifdef CONFIG_FS_BTRFS @@ -233,6 +243,7 @@ static struct fstype_info fstypes[] = { .write = fs_write_unsupported, .uuid = btrfs_uuid, .opendir = fs_opendir_unsupported, + .mkdir = fs_mkdir_unsupported, }, #endif { @@ -248,6 +259,7 @@ static struct fstype_info fstypes[] = { .write = fs_write_unsupported, .uuid = fs_uuid_unsupported, .opendir = fs_opendir_unsupported, + .mkdir = fs_mkdir_unsupported, }, }; @@ -498,6 +510,20 @@ void fs_closedir(struct fs_dir_stream *dirs) } +int fs_mkdir(const char *dirname) +{ + int ret; + + struct fstype_info *info = fs_get_info(fs_type); + + ret = info->mkdir(dirname); + + fs_type = FS_TYPE_ANY; + fs_close(); + + return ret; +} + int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], int fstype) { @@ -700,3 +726,22 @@ int do_fs_type(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return CMD_RET_SUCCESS; } +int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], + int fstype) +{ + int ret; + + if (argc != 4) + return CMD_RET_USAGE; + + if (fs_set_blk_dev(argv[1], argv[2], fstype)) + return 1; + + ret = fs_mkdir(argv[3]); + if (ret) { + printf("** Unable to create a directory \"%s\" **\n", argv[3]); + return 1; + } + + return 0; +} diff --git a/include/fs.h b/include/fs.h index 163da103b4..fbaee154dd 100644 --- a/include/fs.h +++ b/include/fs.h @@ -155,6 +155,14 @@ struct fs_dirent *fs_readdir(struct fs_dir_stream *dirs); */ void fs_closedir(struct fs_dir_stream *dirs); +/* + * fs_mkdir - Create a directory + * + * @filename: Name of directory to create + * @return 0 on success, -1 on error conditions + */ +int fs_mkdir(const char *filename); + /* * Common implementation for various filesystem commands, optionally limited * to a specific filesystem type via the fstype parameter. @@ -169,6 +177,8 @@ int file_exists(const char *dev_type, const char *dev_part, const char *file, int fstype); int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], int fstype); +int do_mkdir(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], + int fstype); /* * Determine the UUID of the specified filesystem and print it. Optionally it is
"mkdir" interface is added to file operations. This is a preparatory change as mkdir support for FAT file system will be added in next patch. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- fs/fs.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ include/fs.h | 10 ++++++++++ 2 files changed, 55 insertions(+)