| Message ID | 20180511030559.26502-1-guaneryu@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
On Fri, May 11, 2018 at 11:05:59AM +0800, Eryu Guan wrote: > One of _scratch_mkfs_xfs_opts()'s job is to remove any metadata > related mkfs options if mkfs.xfs binary doesn't support them, so > that tests that pass metadata mkfs options to _scratch_mkfs, e.g. > "_scratch_mkfs -m crc=0" won't break with old mkfs.xfs. > > But commit 596a068bf130 ("fstests: teach _scratch_mkfs to handle > mkfs option conflicts") broke this, because it didn't pass any mkfs > options to _scratch_mkfs_xfs_opts(). So fix it. > > Also declare a variable as 'local' and fix a typo. > > Reported-by: Dave Chinner <dchinner@redhat.com> > Signed-off-by: Eryu Guan <guaneryu@gmail.com> Ping on this patch, Zorro's new test would depend on this fix, it's great if it could get some reviews. Thanks, Eryu > --- > common/xfs | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/common/xfs b/common/xfs > index e0bc3f43d3c5..498f45e64b6d 100644 > --- a/common/xfs > +++ b/common/xfs > @@ -46,9 +46,9 @@ _setup_large_xfs_fs() > > _scratch_mkfs_xfs_opts() > { > - mkfs_opts=$* > + local mkfs_opts=$* > > - # remove metadata related mkfs options if mkfs.xfs doesn't them > + # remove metadata related mkfs options if mkfs.xfs doesn't support them > if [ -n "$XFS_MKFS_HAS_NO_META_SUPPORT" ]; then > mkfs_opts=`echo $mkfs_opts | sed "s/-m\s\+\S\+//g"` > fi > @@ -79,14 +79,14 @@ _scratch_mkfs_xfs_supported() > > _scratch_mkfs_xfs() > { > - local mkfs_cmd="`_scratch_mkfs_xfs_opts`" > + local mkfs_cmd="`_scratch_mkfs_xfs_opts $*`" > local mkfs_filter="sed -e '/less than device physical sector/d' \ > -e '/switching to logical sector/d' \ > -e '/Default configuration/d'" > local tmp=`mktemp -u` > local mkfs_status > > - _scratch_do_mkfs "$mkfs_cmd" "$mkfs_filter" $* 2>$tmp.mkfserr 1>$tmp.mkfsstd > + _scratch_do_mkfs "$mkfs_cmd" "$mkfs_filter" 2>$tmp.mkfserr 1>$tmp.mkfsstd > mkfs_status=$? > > > -- > 2.17.0 > -- 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 Fri, May 11, 2018 at 11:05:59AM +0800, Eryu Guan wrote: > One of _scratch_mkfs_xfs_opts()'s job is to remove any metadata > related mkfs options if mkfs.xfs binary doesn't support them, so > that tests that pass metadata mkfs options to _scratch_mkfs, e.g. > "_scratch_mkfs -m crc=0" won't break with old mkfs.xfs. > > But commit 596a068bf130 ("fstests: teach _scratch_mkfs to handle > mkfs option conflicts") broke this, because it didn't pass any mkfs > options to _scratch_mkfs_xfs_opts(). So fix it. > > Also declare a variable as 'local' and fix a typo. > > Reported-by: Dave Chinner <dchinner@redhat.com> > Signed-off-by: Eryu Guan <guaneryu@gmail.com> Looks fine. Reviewed-by: Dave Chinner <dchinner@redhat.com> (sorry for missing this when you opsted it first time) -Dave.
On Tue, May 22, 2018 at 08:32:52AM +1000, Dave Chinner wrote: > On Fri, May 11, 2018 at 11:05:59AM +0800, Eryu Guan wrote: > > One of _scratch_mkfs_xfs_opts()'s job is to remove any metadata > > related mkfs options if mkfs.xfs binary doesn't support them, so > > that tests that pass metadata mkfs options to _scratch_mkfs, e.g. > > "_scratch_mkfs -m crc=0" won't break with old mkfs.xfs. > > > > But commit 596a068bf130 ("fstests: teach _scratch_mkfs to handle > > mkfs option conflicts") broke this, because it didn't pass any mkfs > > options to _scratch_mkfs_xfs_opts(). So fix it. > > > > Also declare a variable as 'local' and fix a typo. > > > > Reported-by: Dave Chinner <dchinner@redhat.com> > > Signed-off-by: Eryu Guan <guaneryu@gmail.com> > > Looks fine. > > Reviewed-by: Dave Chinner <dchinner@redhat.com> > > (sorry for missing this when you opsted it first time) Thanks for the review! Eryu -- 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 Wed, May 23, 2018 at 10:01:29AM +0800, Eryu Guan wrote: > On Tue, May 22, 2018 at 08:32:52AM +1000, Dave Chinner wrote: > > On Fri, May 11, 2018 at 11:05:59AM +0800, Eryu Guan wrote: > > > One of _scratch_mkfs_xfs_opts()'s job is to remove any metadata > > > related mkfs options if mkfs.xfs binary doesn't support them, so > > > that tests that pass metadata mkfs options to _scratch_mkfs, e.g. > > > "_scratch_mkfs -m crc=0" won't break with old mkfs.xfs. > > > > > > But commit 596a068bf130 ("fstests: teach _scratch_mkfs to handle > > > mkfs option conflicts") broke this, because it didn't pass any mkfs > > > options to _scratch_mkfs_xfs_opts(). So fix it. > > > > > > Also declare a variable as 'local' and fix a typo. > > > > > > Reported-by: Dave Chinner <dchinner@redhat.com> > > > Signed-off-by: Eryu Guan <guaneryu@gmail.com> > > > > Looks fine. > > > > Reviewed-by: Dave Chinner <dchinner@redhat.com> > > > > (sorry for missing this when you opsted it first time) > > Thanks for the review! > > Eryu This turns out to be broken when specifying "-m crc=0/1" in MKFS_OPTIONS explicitly and test has its own extra "-m crc=0/1" option (e.g. xfs/001, mkfs fails and test uses SCRATCH_DEV created from previous test), as we have "-m" option specified multiple times, and _scratch_do_mkfs() won't try mkfs for a second time because $extra_mkfs_options is empty, which is because I didn't pass "extra_mkfs_options" as extra options but "mkfs_cmd". I wrote a v2 patch and will send it out soon. Thanks, Eryu -- 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/common/xfs b/common/xfs index e0bc3f43d3c5..498f45e64b6d 100644 --- a/common/xfs +++ b/common/xfs @@ -46,9 +46,9 @@ _setup_large_xfs_fs() _scratch_mkfs_xfs_opts() { - mkfs_opts=$* + local mkfs_opts=$* - # remove metadata related mkfs options if mkfs.xfs doesn't them + # remove metadata related mkfs options if mkfs.xfs doesn't support them if [ -n "$XFS_MKFS_HAS_NO_META_SUPPORT" ]; then mkfs_opts=`echo $mkfs_opts | sed "s/-m\s\+\S\+//g"` fi @@ -79,14 +79,14 @@ _scratch_mkfs_xfs_supported() _scratch_mkfs_xfs() { - local mkfs_cmd="`_scratch_mkfs_xfs_opts`" + local mkfs_cmd="`_scratch_mkfs_xfs_opts $*`" local mkfs_filter="sed -e '/less than device physical sector/d' \ -e '/switching to logical sector/d' \ -e '/Default configuration/d'" local tmp=`mktemp -u` local mkfs_status - _scratch_do_mkfs "$mkfs_cmd" "$mkfs_filter" $* 2>$tmp.mkfserr 1>$tmp.mkfsstd + _scratch_do_mkfs "$mkfs_cmd" "$mkfs_filter" 2>$tmp.mkfserr 1>$tmp.mkfsstd mkfs_status=$?
One of _scratch_mkfs_xfs_opts()'s job is to remove any metadata related mkfs options if mkfs.xfs binary doesn't support them, so that tests that pass metadata mkfs options to _scratch_mkfs, e.g. "_scratch_mkfs -m crc=0" won't break with old mkfs.xfs. But commit 596a068bf130 ("fstests: teach _scratch_mkfs to handle mkfs option conflicts") broke this, because it didn't pass any mkfs options to _scratch_mkfs_xfs_opts(). So fix it. Also declare a variable as 'local' and fix a typo. Reported-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Eryu Guan <guaneryu@gmail.com> --- common/xfs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)