On Mon, Apr 01, 2019 at 01:50:18PM +0100, fdman...@kernel.org wrote:
> From: Filipe Manana <fdman...@suse.com>
>
> Currently the fsync function can only be performed against regular files.
> Allow it to operate on directories too, to increase test coverage and
> allow for chances of finding bugs in a filesystem's implementation of
> fsync against directories.
>
> Signed-off-by: Filipe Manana <fdman...@suse.com>
> ---
>
> V2: Added helper functions to open and close files or directories.
not exactly what I meant, more below....
>
> ltp/fsstress.c | 42 +++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> index 2223fd7d..1169b840 100644
> --- a/ltp/fsstress.c
> +++ b/ltp/fsstress.c
> @@ -303,6 +303,7 @@ int attr_remove_path(pathname_t *, const char *,
> int);
> int attr_set_path(pathname_t *, const char *, const char *, const int, int);
> void check_cwd(void);
> void cleanup_flist(void);
> +void close_file_or_dir(int, DIR *);
> int creat_path(pathname_t *, mode_t);
> void dcache_enter(int, int);
> void dcache_init(void);
> @@ -324,6 +325,7 @@ void make_freq_table(void);
> int mkdir_path(pathname_t *, mode_t);
> int mknod_path(pathname_t *, mode_t, dev_t);
> void namerandpad(int, char *, int);
> +int open_file_or_dir(pathname_t *, int, DIR **);
> int open_path(pathname_t *, int);
> DIR *opendir_path(pathname_t *);
> void process_freq(char *);
> @@ -852,6 +854,15 @@ cleanup_flist(void)
> }
> }
>
> +void
> +close_file_or_dir(int fd, DIR *dir)
> +{
> + if (dir)
> + closedir(dir);
> + else
> + close(fd);
> +}
> +
> int
> creat_path(pathname_t *name, mode_t mode)
> {
> @@ -1385,6 +1396,30 @@ namerandpad(int id, char *buf, int i)
> }
>
> int
> +open_file_or_dir(pathname_t *name, int flags, DIR **dir)
> +{
> + int fd;
> +
> + fd = open_path(name, flags);
> + if (fd < 0 && errno == EISDIR) {
> + *dir = opendir_path(name);
Why this function, and not just open(O_DIRECTORY)?
None of the code that uses this function actually uses the DIR *
that is returned, just the fd that is extracted from it. So why no
just open() the directory directly and avoid having all the mess
with dir streams that aren't needed?
> + if (*dir) {
> + fd = dirfd(*dir);
> + if (fd < 0) {
> + int e = errno;
> +
> + closedir(*dir);
> + *dir = NULL;
> + errno = e;
> + }
> + }
> + } else {
> + *dir = NULL;
> + }
> + return fd;
Excessive nesting. I think this should be as simple as:
fd = open_path(name, flags);
if (fd < 0 && errno != EISDIR) {
return -1;
return open_path(name, flags | O_DIRECTORY);
And the close_file_or_dir() function is completely unnecessary.
> +
> +int
> open_path(pathname_t *name, int oflag)
> {
> char buf[NAME_MAX + 1];
> @@ -3440,15 +3475,16 @@ fsync_f(int opno, long r)
> pathname_t f;
> int fd;
> int v;
> + DIR *dir;
>
> init_pathname(&f);
> - if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) {
> + if (!get_fname(FT_REGFILE | FT_DIRm, r, &f, NULL, NULL, &v)) {
> if (v)
> printf("%d/%d: fsync - no filename\n", procid, opno);
> free_pathname(&f);
> return;
> }
> - fd = open_path(&f, O_WRONLY);
> + fd = open_file_or_dir(&f, O_WRONLY, &dir);
> e = fd < 0 ? errno : 0;
> check_cwd();
> if (fd < 0) {
This whole hunk - from init_pathname to check_cwd - was what I was
suggesting you factor out, not just the open_path() code.
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
