| Message ID | 22058.1490696551@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 28, 2017 at 6:22 AM, David Howells <dhowells@redhat.com> wrote: > Eric Sandeen <sandeen@sandeen.net> wrote: > >> Wire up the statx syscall to xfs_io. >> >> xfs_io> help statx >> statx [-O | -m mask][-FDLA] -- extended information about the currently open file >> ... > > I would like to make the attached changes, to make it more capable, except > that xfs_io seems to precheck the "-m mask" argument somewhere: > > [root@andromeda ~]# xfs_io -c statx -m all /dev/null > non-numeric mode -- all > > and interprets "-c" for itself on the command line: > > [root@andromeda ~]# xfs_io -c statx -c /dev/null > command "/dev/null" not found xfs_io -c "statx -c /dev/null" is what you want > > though both these seem to work when used from xfs_io's command prompt. I > guess I should switch -c to -C and also -m to -M since it's not a file mode as > I think the core is expecting. > > Also, how do you get xfs_io to tell you what fd it has opened from its own > command prompt? I would need to pass that to the -d flag as a dir fd. > See the output of 'file' command or the global var filetable. > David > --- > commit 03e4ca1cdc59aaa362fdd51f079493a1f0da254c > Author: David Howells <dhowells@redhat.com> > Date: Tue Mar 28 10:42:23 2017 +0100 > > changes > > diff --git a/io/stat.c b/io/stat.c > index a7aebcd..0b05ec9 100644 > --- a/io/stat.c > +++ b/io/stat.c > @@ -189,12 +189,14 @@ statx_help(void) > " Display extended file status.\n" > "\n" > " Options:\n" > -" -m mask -- Specify the field mask for the statx call (default STATX_ALL)\n" > +" -c -- Compare against fstat/fstatat on the same file/fd\n" > +" -d dirfd -- Use a specific directory fd\n" > +" -f -- Do fstat equivalent and operate on fd\n" > +" -m mask -- Specify the field mask for the statx call (can also be 'basic' or 'all'; default STATX_ALL)\n" > " -A -- Suppress terminal automount traversal\n" > " -D -- Don't sync attributes with the server\n" > " -F -- Force the attributes to be sync'd with the server\n" > " -L -- Follow symlinks (statx link target)\n" > -" -O -- Add only basic stats (STATX_BASIC_STATS) to default mask\n" > "\n")); > } > > @@ -227,6 +229,65 @@ dump_statx(struct statx *stx) > } > > /* > + * Compare the contents of a statx struct with that of a stat struct and check > + * that they're the same. > + */ > +static int > +cmp_statx(const struct statx *stx, const struct stat *st) > +{ > + const char *what = NULL; > + > +#define cmp(x) \ > + do { \ > + what = #x; \ > + if (stx->stx_##x != st->st_##x) \ > + goto mismatch; \ > + } while (0) > + > + cmp(blksize); > + cmp(nlink); > + cmp(uid); > + cmp(gid); > + cmp(mode); > + cmp(ino); > + cmp(size); > + cmp(blocks); > + > +#define devcmp(x) \ > + do { \ > + what = #x".major"; \ > + if (stx->stx_##x##_major != major(st->st_##x)) \ > + goto mismatch; \ > + what = #x".minor"; \ > + if (stx->stx_##x##_minor != minor(st->st_##x)) \ > + goto mismatch; \ > + } while (0) > + > + devcmp(dev); > + devcmp(rdev); > + > +#define timecmp(x) \ > + do { \ > + what = #x".tv_sec"; \ > + if (stx->stx_##x##time.tv_sec != st->st_##x##tim.tv_sec) \ > + goto mismatch; \ > + what = #x".tv_nsec"; \ > + if (stx->stx_##x##time.tv_nsec != st->st_##x##tim.tv_nsec) \ > + goto mismatch; \ > + } while (0) > + > + timecmp(a); > + timecmp(c); > + timecmp(m); > + > + return 0; > + > +mismatch: > + fprintf(stderr, "Mismatch between stat and statx output (%s)\n", what); > + return -1; > +} > + > +/* > * options: > * - input flags - query type > * - output style for flags (and all else?) (chars vs. hex?) > @@ -239,16 +300,31 @@ statx_f( > { > int c; > struct statx stx; > + struct stat st; > int atflag = AT_SYMLINK_NOFOLLOW; > - unsigned int m_mask = 0; /* mask requested with -m */ > - int Oflag = 0, mflag = 0; /* -O or -m was used */ > unsigned int mask = STATX_ALL; > + int use_fd = 0; > + int dirfd = AT_FDCWD; > + int compare = 0; > > - while ((c = getopt(argc, argv, "m:FDLOA")) != EOF) { > + while ((c = getopt(argc, argv, "d:cfm:FDLA")) != EOF) { > switch (c) { > + case 'c': > + compare = 1; > + break; > + case 'd': > + dirfd = strtoul(optarg, NULL, 0); > + break; > + case 'f': > + use_fd = 1; > + break; > case 'm': > - m_mask = atoi(optarg); > - mflag = 1; > + if (strcmp(optarg, "basic") == 0) > + mask = STATX_BASIC_STATS; > + else if (strcmp(optarg, "all") == 0) > + mask = STATX_ALL; > + else > + mask = strtoul(optarg, NULL, 0); > break; > case 'F': > atflag &= ~AT_STATX_SYNC_TYPE; > @@ -261,10 +337,6 @@ statx_f( > case 'L': > atflag &= ~AT_SYMLINK_NOFOLLOW; > break; > - case 'O': > - mask = STATX_BASIC_STATS; > - Oflag = 1; > - break; > case 'A': > atflag |= AT_NO_AUTOMOUNT; > break; > @@ -273,23 +345,38 @@ statx_f( > } > } > > - if (Oflag && mflag) { > - printf("Cannot specify both -m mask and -O\n"); > - return 0; > + memset(&stx, 0xbf, sizeof(stx)); > + > + if (use_fd) { > + if (statx(file->fd, NULL, atflag, mask, &stx) < 0) { > + perror("statx"); > + return 0; > + } > + } else { > + if (statx(dirfd, file->name, atflag, mask, &stx) < 0) { > + perror("statx"); > + return 0; > + } > } > > - /* -m overrides any other mask options */ > - if (mflag) > - mask = m_mask; > + if (compare) { > + if (use_fd) { > + if (fstat(file->fd, &st) < 0) { > + perror("fstat"); > + return 0; > + } > + } else { > + if (fstatat(dirfd, file->name, &st, > + atflag & ~AT_STATX_SYNC_TYPE) < 0) { > + perror("fstatat"); > + return 0; > + } > + } > > - memset(&stx, 0xbf, sizeof(stx)); > - if (statx(AT_FDCWD, file->name, atflag, mask, &stx) < 0) { > - perror("statx"); > - return 0; > + cmp_statx(&stx, &st); > } > - > + > dump_statx(&stx); > - > return 0; > } >
Amir Goldstein <amir73il@gmail.com> wrote: > xfs_io -c "statx -c /dev/null" > > is what you want That doesn't produce any output. Did you mean: xfs_io -c "statx -c" /dev/null Further, this should work: warthog>xfs_io -c "statx -c" /dev/sda /dev/sda: Permission denied but doesn't. > See the output of 'file' command or the global var filetable. So I would have to make "-d dirfd" take an entry in the global var filetable rather than an actual fd number? David
On Tue, Mar 28, 2017 at 8:31 AM, David Howells <dhowells@redhat.com> wrote: > Amir Goldstein <amir73il@gmail.com> wrote: > >> xfs_io -c "statx -c /dev/null" >> >> is what you want > > That doesn't produce any output. Did you mean: > > xfs_io -c "statx -c" /dev/null Yes. > > Further, this should work: > > warthog>xfs_io -c "statx -c" /dev/sda > /dev/sda: Permission denied > This opens /dev/sda for read/write and feeds the fd to the commands executed. You may want to try xfs_io -r -c "statx -c" /dev/sda > but doesn't. > >> See the output of 'file' command or the global var filetable. > > So I would have to make "-d dirfd" take an entry in the global var filetable > rather than an actual fd number? > I guess if you want to test statx without -f/-d then you'd need to provide filename as one of the args to the statx command (and not as args to xfs_io command line). -f is the natural operation you can get from xfs_io and you can use -d on current fd as well. The only command I see that uses the non current fd is sendfile -f N Mostly commands operate on the "current" fd and current fd is iterated from filetable. if you use -d N from command line, you must use -C instead of -c so statx is not performed on all fds in filetable, e.g: xfs_io -r -C "statx -c -d 0" /a/ /a/foo But you see that this makes little sense if "foo" is not an argument of statx. In my (hopefully unbiased) opinion, there is room for the syscall sanity tests that you posted to LTP and there is room for file system functional tests with xfstests, which xfs_io can be used for. The main advantage you get from writing xfstests is that many of us run them on a diversity of file systems, so it is good for catching wrong implementation of statx by specific file systems. By the time statx() gets to fs specific code it does not matter if you called it with -d/-f or like stat() does it? Amir.
Amir Goldstein <amir73il@gmail.com> wrote: > > Further, this should work: > > > > warthog>xfs_io -c "statx -c" /dev/sda > > /dev/sda: Permission denied > > > > This opens /dev/sda for read/write and feeds the fd to the commands > executed. > You may want to try > xfs_io -r -c "statx -c" /dev/sda I've added -P to open to supply O_PATH, but I'm having trouble testing the dirfd usage - I think probably because file->name is an absolute path: xfs_io> open /dev Opened 0 xfs_io> open sda sda: No such file or directory xfs_io> open /dev/sda Opened 1 Possibly open should be able to take a base dir and call openat(). (I also made it print the file table index after a successful open, though I perhaps only want to do this in interactive mode). > By the time statx() gets to fs specific code it does not matter if you > called it with -d/-f or like stat() does it? No. But all the calling options still have to be tested, as does stuffing bad values into the syscall args - something xfstests seems to be very poor at. > In my (hopefully unbiased) opinion, there is room for the syscall sanity > tests that you posted to LTP and there is room for file system > functional tests with xfstests, which xfs_io can be used for. Yes. I agree. Christoph may be of the opinion that LTP is a trainwreck, but it can do some things much more easily than can xfstests, primarily because its tests are written in C. Any suggestions on how to do timestamp comparisons? I really want to be able to do things like asking if mtime > btime or btime < wall clock time. I guess I could add another command for that.
On Tue, Mar 28, 2017 at 10:04 AM, David Howells <dhowells@redhat.com> wrote: > Amir Goldstein <amir73il@gmail.com> wrote: > >> > Further, this should work: >> > >> > warthog>xfs_io -c "statx -c" /dev/sda >> > /dev/sda: Permission denied >> > >> >> This opens /dev/sda for read/write and feeds the fd to the commands >> executed. >> You may want to try >> xfs_io -r -c "statx -c" /dev/sda > > I've added -P to open to supply O_PATH, but I'm having trouble testing the > dirfd usage - I think probably because file->name is an absolute path: > > xfs_io> open /dev > Opened 0 > xfs_io> open sda > sda: No such file or directory > xfs_io> open /dev/sda > Opened 1 > > Possibly open should be able to take a base dir and call openat(). Yeh, I told you statx -d does not make much sense for xfs_io. I don't think you wanna go there... > > (I also made it print the file table index after a successful open, though I > perhaps only want to do this in interactive mode). > >> By the time statx() gets to fs specific code it does not matter if you >> called it with -d/-f or like stat() does it? > > No. But all the calling options still have to be tested, as does stuffing bad > values into the syscall args - something xfstests seems to be very poor at. > >> In my (hopefully unbiased) opinion, there is room for the syscall sanity >> tests that you posted to LTP and there is room for file system >> functional tests with xfstests, which xfs_io can be used for. > > Yes. I agree. Christoph may be of the opinion that LTP is a trainwreck, but > it can do some things much more easily than can xfstests, primarily because > its tests are written in C. > Adding C tests under src/ dir is a viable option. > Any suggestions on how to do timestamp comparisons? I really want to be able > to do things like asking if mtime > btime or btime < wall clock time. I guess > I could add another command for that. Not in xfs_io, don't go there. You can look at tests/generic/003 (it hurts the eyes a bit) Perhaps it would be better to write a small C helper (e.g. t_compare_statx_times) and then tests could be easily and flexibly extended. Amir.
diff --git a/io/stat.c b/io/stat.c index a7aebcd..0b05ec9 100644 --- a/io/stat.c +++ b/io/stat.c @@ -189,12 +189,14 @@ statx_help(void) " Display extended file status.\n" "\n" " Options:\n" -" -m mask -- Specify the field mask for the statx call (default STATX_ALL)\n" +" -c -- Compare against fstat/fstatat on the same file/fd\n" +" -d dirfd -- Use a specific directory fd\n" +" -f -- Do fstat equivalent and operate on fd\n" +" -m mask -- Specify the field mask for the statx call (can also be 'basic' or 'all'; default STATX_ALL)\n" " -A -- Suppress terminal automount traversal\n" " -D -- Don't sync attributes with the server\n" " -F -- Force the attributes to be sync'd with the server\n" " -L -- Follow symlinks (statx link target)\n" -" -O -- Add only basic stats (STATX_BASIC_STATS) to default mask\n" "\n")); } @@ -227,6 +229,65 @@ dump_statx(struct statx *stx) } /* + * Compare the contents of a statx struct with that of a stat struct and check + * that they're the same. + */ +static int +cmp_statx(const struct statx *stx, const struct stat *st) +{ + const char *what = NULL; + +#define cmp(x) \ + do { \ + what = #x; \ + if (stx->stx_##x != st->st_##x) \ + goto mismatch; \ + } while (0) + + cmp(blksize); + cmp(nlink); + cmp(uid); + cmp(gid); + cmp(mode); + cmp(ino); + cmp(size); + cmp(blocks); + +#define devcmp(x) \ + do { \ + what = #x".major"; \ + if (stx->stx_##x##_major != major(st->st_##x)) \ + goto mismatch; \ + what = #x".minor"; \ + if (stx->stx_##x##_minor != minor(st->st_##x)) \ + goto mismatch; \ + } while (0) + + devcmp(dev); + devcmp(rdev); + +#define timecmp(x) \ + do { \ + what = #x".tv_sec"; \ + if (stx->stx_##x##time.tv_sec != st->st_##x##tim.tv_sec) \ + goto mismatch; \ + what = #x".tv_nsec"; \ + if (stx->stx_##x##time.tv_nsec != st->st_##x##tim.tv_nsec) \ + goto mismatch; \ + } while (0) + + timecmp(a); + timecmp(c); + timecmp(m); + + return 0; + +mismatch: + fprintf(stderr, "Mismatch between stat and statx output (%s)\n", what); + return -1; +} + +/* * options: * - input flags - query type * - output style for flags (and all else?) (chars vs. hex?) @@ -239,16 +300,31 @@ statx_f( { int c; struct statx stx; + struct stat st; int atflag = AT_SYMLINK_NOFOLLOW; - unsigned int m_mask = 0; /* mask requested with -m */ - int Oflag = 0, mflag = 0; /* -O or -m was used */ unsigned int mask = STATX_ALL; + int use_fd = 0; + int dirfd = AT_FDCWD; + int compare = 0; - while ((c = getopt(argc, argv, "m:FDLOA")) != EOF) { + while ((c = getopt(argc, argv, "d:cfm:FDLA")) != EOF) { switch (c) { + case 'c': + compare = 1; + break; + case 'd': + dirfd = strtoul(optarg, NULL, 0); + break; + case 'f': + use_fd = 1; + break; case 'm': - m_mask = atoi(optarg); - mflag = 1; + if (strcmp(optarg, "basic") == 0) + mask = STATX_BASIC_STATS; + else if (strcmp(optarg, "all") == 0) + mask = STATX_ALL; + else + mask = strtoul(optarg, NULL, 0); break; case 'F': atflag &= ~AT_STATX_SYNC_TYPE; @@ -261,10 +337,6 @@ statx_f( case 'L': atflag &= ~AT_SYMLINK_NOFOLLOW; break; - case 'O': - mask = STATX_BASIC_STATS; - Oflag = 1; - break; case 'A': atflag |= AT_NO_AUTOMOUNT; break; @@ -273,23 +345,38 @@ statx_f( } } - if (Oflag && mflag) { - printf("Cannot specify both -m mask and -O\n"); - return 0; + memset(&stx, 0xbf, sizeof(stx)); + + if (use_fd) { + if (statx(file->fd, NULL, atflag, mask, &stx) < 0) { + perror("statx"); + return 0; + } + } else { + if (statx(dirfd, file->name, atflag, mask, &stx) < 0) { + perror("statx"); + return 0; + } } - /* -m overrides any other mask options */ - if (mflag) - mask = m_mask; + if (compare) { + if (use_fd) { + if (fstat(file->fd, &st) < 0) { + perror("fstat"); + return 0; + } + } else { + if (fstatat(dirfd, file->name, &st, + atflag & ~AT_STATX_SYNC_TYPE) < 0) { + perror("fstatat"); + return 0; + } + } - memset(&stx, 0xbf, sizeof(stx)); - if (statx(AT_FDCWD, file->name, atflag, mask, &stx) < 0) { - perror("statx"); - return 0; + cmp_statx(&stx, &st); } - + dump_statx(&stx); - return 0; }