diff mbox series

[8/8] xfs/178: fix mkfs success test

Message ID 162078494495.3302755.13327851823592717788.stgit@magnolia (mailing list archive)
State New, archived
Headers show
Series fstests: miscellaneous fixes | expand

Commit Message

Darrick J. Wong May 12, 2021, 2:02 a.m. UTC
From: Darrick J. Wong <djwong@kernel.org>

Fix the obviously incorrect code here that wants to fail the test if
mkfs doesn't succeed.  The return value ("$?") is always the status of
the /last/ command in the pipe.  Change the checker to _notrun so that
we don't leave the scratch check files around.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/178 |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Eryu Guan May 16, 2021, 3:54 p.m. UTC | #1
On Tue, May 11, 2021 at 07:02:24PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Fix the obviously incorrect code here that wants to fail the test if
> mkfs doesn't succeed.  The return value ("$?") is always the status of
> the /last/ command in the pipe.  Change the checker to _notrun so that
> we don't leave the scratch check files around.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/xfs/178 |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/tests/xfs/178 b/tests/xfs/178
> index a24ef50c..bf72e640 100755
> --- a/tests/xfs/178
> +++ b/tests/xfs/178
> @@ -57,8 +57,8 @@ _supported_fs xfs
>  #             fix filesystem, new mkfs.xfs will be fine.
>  
>  _require_scratch
> -_scratch_mkfs_xfs | _filter_mkfs 2>$tmp.mkfs \
> -        || _fail "mkfs failed!"
> +_scratch_mkfs_xfs | _filter_mkfs 2>$tmp.mkfs
> +test "${PIPESTATUS[0]}" -eq 0 || _notrun "mkfs failed!"

I still don't understand why changing this to _notrun, shouldn't creating a
default filesystem should always pass? and fail the test if mkfs failed?

Thanks,
Eryu

>  
>  # By executing the followint tmp file, will get on the mkfs options stored in
>  # variables
>
Darrick J. Wong May 19, 2021, 11:20 p.m. UTC | #2
On Sun, May 16, 2021 at 11:54:30PM +0800, Eryu Guan wrote:
> On Tue, May 11, 2021 at 07:02:24PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Fix the obviously incorrect code here that wants to fail the test if
> > mkfs doesn't succeed.  The return value ("$?") is always the status of
> > the /last/ command in the pipe.  Change the checker to _notrun so that
> > we don't leave the scratch check files around.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  tests/xfs/178 |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/tests/xfs/178 b/tests/xfs/178
> > index a24ef50c..bf72e640 100755
> > --- a/tests/xfs/178
> > +++ b/tests/xfs/178
> > @@ -57,8 +57,8 @@ _supported_fs xfs
> >  #             fix filesystem, new mkfs.xfs will be fine.
> >  
> >  _require_scratch
> > -_scratch_mkfs_xfs | _filter_mkfs 2>$tmp.mkfs \
> > -        || _fail "mkfs failed!"
> > +_scratch_mkfs_xfs | _filter_mkfs 2>$tmp.mkfs
> > +test "${PIPESTATUS[0]}" -eq 0 || _notrun "mkfs failed!"
> 
> I still don't understand why changing this to _notrun, shouldn't creating a
> default filesystem should always pass?

It will, unless you've injected a malicious mkfs.xfs to make sure
constructions like these actually work.

> and fail the test if mkfs failed?

Yeah, you're right, it /should/ fail.  I'll leave it alone then.

--D

> 
> Thanks,
> Eryu
> 
> >  
> >  # By executing the followint tmp file, will get on the mkfs options stored in
> >  # variables
> >
diff mbox series

Patch

diff --git a/tests/xfs/178 b/tests/xfs/178
index a24ef50c..bf72e640 100755
--- a/tests/xfs/178
+++ b/tests/xfs/178
@@ -57,8 +57,8 @@  _supported_fs xfs
 #             fix filesystem, new mkfs.xfs will be fine.
 
 _require_scratch
-_scratch_mkfs_xfs | _filter_mkfs 2>$tmp.mkfs \
-        || _fail "mkfs failed!"
+_scratch_mkfs_xfs | _filter_mkfs 2>$tmp.mkfs
+test "${PIPESTATUS[0]}" -eq 0 || _notrun "mkfs failed!"
 
 # By executing the followint tmp file, will get on the mkfs options stored in
 # variables