| Message ID | 1463502981-15593-1-git-send-email-zlang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 18, 2016 at 12:36:20AM +0800, Zorro Lang wrote: > 1. This function try to run $XFS_IO_PROG -c "$command help", that's > wrong. It should be "help $command". I think there was more to it that than: $ sudo xfs_io -c "pwrite help" $ sudo xfs_io -c "help pwrite" pwrite [-i infile [-d] [-s skip]] [-b bs] [-S seed] [-wW] [-FBR [-Z N]] [-V N] off len -- writes a number of bytes at a specified offset writes a range of bytes (in block size increments) from the given offset [....] $ sudo xfs_io -c "foo help" command "foo" not found i.e. that check simply tells us whether the command exists or not with an error message that is captured and then tested. If we actually run the help command, we got lots of output for commands that exist and that may confuse the error string checking we then do. Yes, it's a bit of a hack, and we may not even need the "help" parameter anymore, but ISTR older versions of xfs_io needed it to parse the CLI successfully for all the different commands.... > 2. The $param can't be used for all command's options, for example > "help pwrite" include: > > -Z N -- zeed the random number generator (used when writing randomly) > (heh, zorry, the -s/-S arguments were already in use in pwrite) This bit is fine. In general, you should put separate changes into separate patches. If you find yourself iterating a list of things a patch fixes in the description, then that's a good sign it should be split into multiple patches. Cheers, Dave.
----- ???? ----- > ???: "Dave Chinner" <david@fromorbit.com> > ???: "Zorro Lang" <zlang@redhat.com> > ??: fstests@vger.kernel.org > ????: ???, 2016? 5 ? 19? ?? 7:55:31 > ??: Re: [PATCH 1/2] common/rc: modify _require_xfs_io_command function > > On Wed, May 18, 2016 at 12:36:20AM +0800, Zorro Lang wrote: > > 1. This function try to run $XFS_IO_PROG -c "$command help", that's > > wrong. It should be "help $command". > > I think there was more to it that than: > > $ sudo xfs_io -c "pwrite help" > $ sudo xfs_io -c "help pwrite" > pwrite [-i infile [-d] [-s skip]] [-b bs] [-S seed] [-wW] [-FBR [-Z N]] [-V > N] off len -- writes a number of bytes at a specified offset > > writes a range of bytes (in block size increments) from the given offset > [....] > $ sudo xfs_io -c "foo help" > command "foo" not found > > i.e. that check simply tells us whether the command exists or not > with an error message that is captured and then tested. If we > actually run the help command, we got lots of output for commands > that exist and that may confuse the error string checking we then > do. > > Yes, it's a bit of a hack, and we may not even need the "help" > parameter anymore, but ISTR older versions of xfs_io needed it to > parse the CLI successfully for all the different commands.... Hmm, that's a smart way. So I don't need to change that before someone new xfsprogs break this "hack rule":) > > > 2. The $param can't be used for all command's options, for example > > "help pwrite" include: > > > > -Z N -- zeed the random number generator (used when writing randomly) > > (heh, zorry, the -s/-S arguments were already in use in pwrite) > > This bit is fine. > > In general, you should put separate changes into separate patches. > If you find yourself iterating a list of things a patch fixes in the > description, then that's a good sign it should be split into > multiple patches. Thanks for point that, I'll send a V2 patch only contain this change. Thanks, Zorro > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > -- > To unsubscribe from this list: send the line "unsubscribe fstests" 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 fstests" 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/common/rc b/common/rc index 51092a0..3b87d7e 100644 --- a/common/rc +++ b/common/rc @@ -1874,7 +1874,8 @@ _require_xfs_io_command() exit 1 fi command=$1 - param=$2 + shift + param="$*" testfile=$TEST_DIR/$$.xfs_io case $command in @@ -1896,7 +1897,7 @@ _require_xfs_io_command() _notrun "xfs_io $command support is missing" ;; *) - testio=`$XFS_IO_PROG -c "$command help" 2>&1` + testio=`$XFS_IO_PROG -c "help $command" 2>&1` esac rm -f $testfile 2>&1 > /dev/null
1. This function try to run $XFS_IO_PROG -c "$command help", that's wrong. It should be "help $command". 2. The $param can't be used for all command's options, for example "help pwrite" include: -Z N -- zeed the random number generator (used when writing randomly) (heh, zorry, the -s/-S arguments were already in use in pwrite) We should make param="-Z N", not only "-Z". After this patch, we can run this function as: _require_xfs_io_command pwrite -Z N Signed-off-by: Zorro Lang <zlang@redhat.com> --- common/rc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)