| Message ID | 29458.1490711932@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
David Howells <dhowells@redhat.com> wrote: > Here are my current changes to Eric's statx interface patch. I've made it > analoguous to the stat command and so it only does statx-of-fd. > > I've made the "-m" flag able to take the words "basic" and "all" in > preference to a number and able to take an octal or hex number as an > alternative too. > > I've dropped the -A and -L flags since it no longer passes a path over. > > Finally, I've added a -c flag that causes an fstat() to be done as well and > the buffers compared as a consistency check. Note that the intention is to put the testing of the syscall parameter handling into LTP, along with testing of the symlink following and dirfd usage since xfstests seems unsuitable for this. David -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I am (literally) in the woods this week so can't really comment at length, but generally: xfs_io is useful to facilitate testing at times, but usually doesn't have tests built into the tool itself. And I think you are right that it is a poor fit for some of the testing you'd like to do. There is a src/ dir in xfstests for specialized C tests which get called by the test harness; that might also be a reasonable option. Thanks, Eric > On Mar 28, 2017, at 9:41 AM, David Howells <dhowells@redhat.com> wrote: > > Note that the intention is to put the testing of the syscall parameter > handling into LTP, along with testing of the symlink following and dirfd usage > since xfstests seems unsuitable for this. > > David -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 28, 2017 at 12:42 PM, Eric Sandeen <sandeen@sandeen.net> wrote: > I am (literally) in the woods this week so can't really comment at length, but generally: > > xfs_io is useful to facilitate testing at times, but usually doesn't have tests built into the tool itself. > David, Eric being the maintainer, so he gets to decide, but he is being somewhat subtle. There is no precedence AFAIK to tests within xfs_io. Instead of comparing to stat with -c flag, you can make sure that output of xfs_io -c 'statx -c' is fully compatible to output of xfs_io -c 'stat -v' and do the test in scripts. > And I think you are right that it is a poor fit for some of the testing you'd like to do. > There is a src/ dir in xfstests for specialized C tests which get called by the test harness; that might also be a reasonable option. > That's actually the shortest path for you and there are quite a few tests in xfstests that took this path. It should be easy for you to copy & paste one of them. That's not instead of adding xfs_io statx - just for tests that are not natural to do with xfs_io. > Thanks, > Eric > >> On Mar 28, 2017, at 9:41 AM, David Howells <dhowells@redhat.com> wrote: >> >> Note that the intention is to put the testing of the syscall parameter >> handling into LTP, along with testing of the symlink following and dirfd usage >> since xfstests seems unsuitable for this. >> >> David > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Amir Goldstein <amir73il@gmail.com> wrote: > That's actually the shortest path for you and there are quite a few > tests in xfstests that took > this path. It should be easy for you to copy & paste one of them. Don't forget that syscall testing for this has to be added to LTP anyway... David -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 28, 2017 at 1:56 PM, David Howells <dhowells@redhat.com> wrote: > Amir Goldstein <amir73il@gmail.com> wrote: > >> That's actually the shortest path for you and there are quite a few >> tests in xfstests that took >> this path. It should be easy for you to copy & paste one of them. > > Don't forget that syscall testing for this has to be added to LTP anyway... > Sure, LTP is good for that and many testers run LTP, but I have no idea how wide is the file system diversity of their setups. statx may have bugs in syscall/vfs and it may have bugs in specific fs, so having some C tests in LTP and some C tests in xfstest can't hurt. It's not difficult to harness a C test to xfstest. See recent example of C only (mostly) test in 2161614 generic: concurrent non-overlapping direct I/O on the same extents -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mar 28, 2017, at 8:38 AM, David Howells <dhowells@redhat.com> wrote: > > Here are my current changes to Eric's statx interface patch. I've made it > analoguous to the stat command and so it only does statx-of-fd. > > I've made the "-m" flag able to take the words "basic" and "all" in > preference to a number and able to take an octal or hex number as an > alternative too. > > I've dropped the -A and -L flags since it no longer passes a path over. > > Finally, I've added a -c flag that causes an fstat() to be done as well and > the buffers compared as a consistency check. It probably makes sense to use "-C" to avoid confusion with xfs_io's "-c" option, and also the "stat -c" option to dump only specific fields (which may be useful to add at some point in the future as well). Cheers, Andreas > > David > --- > diff --git a/io/stat.c b/io/stat.c > index a7aebcd..961b6d1 100644 > --- a/io/stat.c > +++ b/io/stat.c > @@ -189,12 +189,10 @@ 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" > -" -A -- Suppress terminal automount traversal\n" > +" -c -- Compare against fstat/fstatat on the same file/fd\n" > +" -m mask -- Specify the field mask for the statx call (can also be 'basic' or 'all'; default STATX_ALL)\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 +225,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 +296,23 @@ statx_f( > { > int c; > struct statx stx; > - int atflag = AT_SYMLINK_NOFOLLOW; > - unsigned int m_mask = 0; /* mask requested with -m */ > - int Oflag = 0, mflag = 0; /* -O or -m was used */ > + struct stat st; > + int atflag = 0; > unsigned int mask = STATX_ALL; > + int compare = 0; > > - while ((c = getopt(argc, argv, "m:FDLOA")) != EOF) { > + while ((c = getopt(argc, argv, "cm:FD")) != EOF) { > switch (c) { > + case 'c': > + compare = 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; > @@ -258,38 +322,28 @@ statx_f( > atflag &= ~AT_STATX_SYNC_TYPE; > atflag |= AT_STATX_DONT_SYNC; > break; > - case 'L': > - atflag &= ~AT_SYMLINK_NOFOLLOW; > - break; > - case 'O': > - mask = STATX_BASIC_STATS; > - Oflag = 1; > - break; > - case 'A': > - atflag |= AT_NO_AUTOMOUNT; > - break; > default: > return command_usage(&statx_cmd); > } > } > > - if (Oflag && mflag) { > - printf("Cannot specify both -m mask and -O\n"); > - return 0; > - } > - > - /* -m overrides any other mask options */ > - if (mflag) > - mask = m_mask; > - > memset(&stx, 0xbf, sizeof(stx)); > - if (statx(AT_FDCWD, file->name, atflag, mask, &stx) < 0) { > + > + if (statx(file->fd, NULL, atflag, mask, &stx) < 0) { > perror("statx"); > return 0; > } > > - dump_statx(&stx); > + if (compare) { > + if (fstat(file->fd, &st) < 0) { > + perror("fstat"); > + return 0; > + } > > + cmp_statx(&stx, &st); > + } > + > + dump_statx(&stx); > return 0; > } > > diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8 > index 77ba760..d82f882 100644 > --- a/man/man8/xfs_io.8 > +++ b/man/man8/xfs_io.8 > @@ -884,16 +884,23 @@ and the XFS_IOC_GETXATTR system call on the current file. If the > option is specified, the atime (last access), mtime > (last modify), and ctime (last change) timestamps are also displayed. > .TP > -.BR statx " [ " \-O " | " "\-m mask" " ][ \-" FDLA " ]" > +.BR statx " [ " "\-m mask" " ][ \-" cFD " ]" > Extended information from the statx syscall. > .RS 1.0i > .PD 0 > .TP 0.4i > .B \-m mask > -Specify the field mask for the statx call (default STATX_ALL) > +Specify the field mask for the statx call as an decimal, hex or octal integer > +or > +.RI \" basic "\" or \"" all \" > +to specify the basic stats that > +.IR stat () > +returns or all the stats known by the header file. All is the default. > .TP > -.B \-O > -Add only basic stats (STATX_BASIC_STATS) to default mask > +.B \-c > +Do an > +.IR fstat () > +call as well and compare the buffers. > .TP > .B \-F > Force the attributes to be sync'd with the server > @@ -901,12 +908,6 @@ Force the attributes to be sync'd with the server > .B \-D > Don't sync attributes with the server > .TP > -.B \-L > -Follow symlinks (statx link target) > -.TP > -.B \-A > -Suppress terminal automount traversal > -.TP > .RE > .IP > .TP Cheers, Andreas
On Tue, Mar 28, 2017 at 2:40 PM, Andreas Dilger <adilger@dilger.ca> wrote: > On Mar 28, 2017, at 8:38 AM, David Howells <dhowells@redhat.com> wrote: >> >> Here are my current changes to Eric's statx interface patch. I've made it >> analoguous to the stat command and so it only does statx-of-fd. >> >> I've made the "-m" flag able to take the words "basic" and "all" in >> preference to a number and able to take an octal or hex number as an >> alternative too. >> >> I've dropped the -A and -L flags since it no longer passes a path over. >> >> Finally, I've added a -c flag that causes an fstat() to be done as well and >> the buffers compared as a consistency check. > > It probably makes sense to use "-C" to avoid confusion with xfs_io's "-c" > option, and also the "stat -c" option to dump only specific fields (which > may be useful to add at some point in the future as well). > I'm afraid its not better for avoiding confusion. -C already taken as well for new "oneshot commands"... -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/28/17 12:35 PM, Amir Goldstein wrote: > On Tue, Mar 28, 2017 at 12:42 PM, Eric Sandeen <sandeen@sandeen.net> wrote: >> I am (literally) in the woods this week so can't really comment at length, but generally: >> >> xfs_io is useful to facilitate testing at times, but usually doesn't have tests built into the tool itself. >> > > David, > > Eric being the maintainer, so he gets to decide, but he is being > somewhat subtle. Sorry about that. More like: terse, with only a phone to communicate, and a wife who thinks we're on vacation. ;) > There is no precedence AFAIK to tests within xfs_io. > Instead of comparing to stat with -c flag, you can make sure that > output of xfs_io -c 'statx -c' > is fully compatible to output of xfs_io -c 'stat -v' and do the test in scripts. *nod* >> And I think you are right that it is a poor fit for some of the testing you'd like to do. >> There is a src/ dir in xfstests for specialized C tests which get called by the test harness; that might also be a reasonable option. >> > > That's actually the shortest path for you and there are quite a few > tests in xfstests that took > this path. It should be easy for you to copy & paste one of them. > > That's not instead of adding xfs_io statx - just for tests that are > not natural to do with xfs_io. Agreed. I think there's value in having a statx command in xfs_io, but it won't be able to facilitate testing /everything/ that needs to be tested. Thanks, -Eric >> Thanks, >> Eric >> >>> On Mar 28, 2017, at 9:41 AM, David Howells <dhowells@redhat.com> wrote: >>> >>> Note that the intention is to put the testing of the syscall parameter >>> handling into LTP, along with testing of the symlink following and dirfd usage >>> since xfstests seems unsuitable for this. >>> >>> David >> > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/28/17 2:07 PM, Amir Goldstein wrote: > On Tue, Mar 28, 2017 at 2:40 PM, Andreas Dilger <adilger@dilger.ca> wrote: >> On Mar 28, 2017, at 8:38 AM, David Howells <dhowells@redhat.com> wrote: >>> >>> Here are my current changes to Eric's statx interface patch. I've made it >>> analoguous to the stat command and so it only does statx-of-fd. >>> >>> I've made the "-m" flag able to take the words "basic" and "all" in >>> preference to a number and able to take an octal or hex number as an >>> alternative too. >>> >>> I've dropped the -A and -L flags since it no longer passes a path over. >>> >>> Finally, I've added a -c flag that causes an fstat() to be done as well and >>> the buffers compared as a consistency check. >> >> It probably makes sense to use "-C" to avoid confusion with xfs_io's "-c" >> option, and also the "stat -c" option to dump only specific fields (which >> may be useful to add at some point in the future as well). >> > > I'm afraid its not better for avoiding confusion. > -C already taken as well for new "oneshot commands"... There's no conflict here AFAICT; the "-C" for oneshot is for xfs_io invocation; individual commands are free to re-use such options for their own purposes. i.e.: # xfs_io -C "command -C" file is fine; the two different -C args have different contexts. Adding a -C to the statx command poses no problem. And with that public service announcement let me go read this thread and catch up... ;) -Eric > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 28, 2017 at 03:38:52PM +0100, David Howells wrote: > Here are my current changes to Eric's statx interface patch. I've made it > analoguous to the stat command and so it only does statx-of-fd. > > I've made the "-m" flag able to take the words "basic" and "all" in > preference to a number and able to take an octal or hex number as an > alternative too. > > I've dropped the -A and -L flags since it no longer passes a path over. > > Finally, I've added a -c flag that causes an fstat() to be done as well and > the buffers compared as a consistency check. > > David > --- > diff --git a/io/stat.c b/io/stat.c > index a7aebcd..961b6d1 100644 > --- a/io/stat.c > +++ b/io/stat.c > @@ -189,12 +189,10 @@ 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" > -" -A -- Suppress terminal automount traversal\n" > +" -c -- Compare against fstat/fstatat on the same file/fd\n" Running "compare" tests like this is not a function that xfs_io should perform. Have it run and output the statx information, and use xfstests to compare that against a golden output and/or the output of the xfs_io stat command. i.e. xfs_io is a utility for executing commands, whilst xfstests is used to test that the executed functionality has the correct output. Cheers, Dave.
Dave Chinner <david@fromorbit.com> wrote: > Running "compare" tests like this is not a function that xfs_io > should perform. Have it run and output the statx information, > and use xfstests to compare that against a golden output and/or > the output of the xfs_io stat command. A golden output that covers timestamps isn't possible. Also comparing the output of Eric's statx command with the output of the stat command as it stands isn't practical. I guess I can add another parameter to the stat command to produce something that looks the same. I'm tempted to write C-based python3 module that gives raw access to the system calls (something I want for the unionmount testsuite) and then script this in python. David -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/io/stat.c b/io/stat.c index a7aebcd..961b6d1 100644 --- a/io/stat.c +++ b/io/stat.c @@ -189,12 +189,10 @@ 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" -" -A -- Suppress terminal automount traversal\n" +" -c -- Compare against fstat/fstatat on the same file/fd\n" +" -m mask -- Specify the field mask for the statx call (can also be 'basic' or 'all'; default STATX_ALL)\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 +225,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 +296,23 @@ statx_f( { int c; struct statx stx; - int atflag = AT_SYMLINK_NOFOLLOW; - unsigned int m_mask = 0; /* mask requested with -m */ - int Oflag = 0, mflag = 0; /* -O or -m was used */ + struct stat st; + int atflag = 0; unsigned int mask = STATX_ALL; + int compare = 0; - while ((c = getopt(argc, argv, "m:FDLOA")) != EOF) { + while ((c = getopt(argc, argv, "cm:FD")) != EOF) { switch (c) { + case 'c': + compare = 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; @@ -258,38 +322,28 @@ statx_f( atflag &= ~AT_STATX_SYNC_TYPE; atflag |= AT_STATX_DONT_SYNC; break; - case 'L': - atflag &= ~AT_SYMLINK_NOFOLLOW; - break; - case 'O': - mask = STATX_BASIC_STATS; - Oflag = 1; - break; - case 'A': - atflag |= AT_NO_AUTOMOUNT; - break; default: return command_usage(&statx_cmd); } } - if (Oflag && mflag) { - printf("Cannot specify both -m mask and -O\n"); - return 0; - } - - /* -m overrides any other mask options */ - if (mflag) - mask = m_mask; - memset(&stx, 0xbf, sizeof(stx)); - if (statx(AT_FDCWD, file->name, atflag, mask, &stx) < 0) { + + if (statx(file->fd, NULL, atflag, mask, &stx) < 0) { perror("statx"); return 0; } - dump_statx(&stx); + if (compare) { + if (fstat(file->fd, &st) < 0) { + perror("fstat"); + return 0; + } + cmp_statx(&stx, &st); + } + + dump_statx(&stx); return 0; } diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8 index 77ba760..d82f882 100644 --- a/man/man8/xfs_io.8 +++ b/man/man8/xfs_io.8 @@ -884,16 +884,23 @@ and the XFS_IOC_GETXATTR system call on the current file. If the option is specified, the atime (last access), mtime (last modify), and ctime (last change) timestamps are also displayed. .TP -.BR statx " [ " \-O " | " "\-m mask" " ][ \-" FDLA " ]" +.BR statx " [ " "\-m mask" " ][ \-" cFD " ]" Extended information from the statx syscall. .RS 1.0i .PD 0 .TP 0.4i .B \-m mask -Specify the field mask for the statx call (default STATX_ALL) +Specify the field mask for the statx call as an decimal, hex or octal integer +or +.RI \" basic "\" or \"" all \" +to specify the basic stats that +.IR stat () +returns or all the stats known by the header file. All is the default. .TP -.B \-O -Add only basic stats (STATX_BASIC_STATS) to default mask +.B \-c +Do an +.IR fstat () +call as well and compare the buffers. .TP .B \-F Force the attributes to be sync'd with the server @@ -901,12 +908,6 @@ Force the attributes to be sync'd with the server .B \-D Don't sync attributes with the server .TP -.B \-L -Follow symlinks (statx link target) -.TP -.B \-A -Suppress terminal automount traversal -.TP .RE .IP .TP