[PATCH] common/rc: filter out extra mount error

[PATCH] common/rc: filter out extra mount error

[Zorro Lang]

The lastest mount command (from util-linux) merged below commit:
  79534c0d7e0f ("mount: add hint about dmesg(8) to error messages")
which brought in a new error output when mount fails.

That cause some cases (e.g. xfs/005) fail as:
  mount: Structure needs cleaning
         dmesg(1) may have more information after failed mount system call

More failed cases like generic/050, ext4/002, xfs/154, etc ...

Signed-off-by: Zorro Lang <zlang@kernel.org>
---

Due to _try_scratch_mount is call in many places, so I'm not sure if it's good
to filter this message in _try_scratch_mount. Maybe better to filter it in each
failed cases? Or write another function (e.g. _try_scratch_mount_fail) ?
Welcome review points.

Thanks,
Zorro

 common/rc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 70a15f9c..67342760 100644
--- a/common/rc
+++ b/common/rc
@@ -335,7 +335,8 @@ _try_scratch_mount()
 		_overlay_scratch_mount $*
 		return $?
 	fi
-	_mount -t $FSTYP `_scratch_mount_options $*`
+	_mount -t $FSTYP `_scratch_mount_options $*`  2> \
+		>(grep -v "dmesg(1) may have more information after failed mount" >&2)
 	mount_ret=$?
 	[ $mount_ret -ne 0 ] && return $mount_ret
 	_idmapped_mount $SCRATCH_DEV $SCRATCH_MNT
-- 
2.31.1

Re: [PATCH] common/rc: filter out extra mount error

[Dave Chinner]

On Thu, May 26, 2022 at 05:57:04PM +0800, Zorro Lang wrote:
> The lastest mount command (from util-linux) merged below commit:
>   79534c0d7e0f ("mount: add hint about dmesg(8) to error messages")
> which brought in a new error output when mount fails.
> 
> That cause some cases (e.g. xfs/005) fail as:
>   mount: Structure needs cleaning
>          dmesg(1) may have more information after failed mount system call
> 
> More failed cases like generic/050, ext4/002, xfs/154, etc ...
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
> 
> Due to _try_scratch_mount is call in many places, so I'm not sure if it's good
> to filter this message in _try_scratch_mount. Maybe better to filter it in each
> failed cases? Or write another function (e.g. _try_scratch_mount_fail) ?
> Welcome review points.
> 
> Thanks,
> Zorro
> 
>  common/rc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/common/rc b/common/rc
> index 70a15f9c..67342760 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -335,7 +335,8 @@ _try_scratch_mount()
>  		_overlay_scratch_mount $*
>  		return $?
>  	fi
> -	_mount -t $FSTYP `_scratch_mount_options $*`
> +	_mount -t $FSTYP `_scratch_mount_options $*`  2> \
> +		>(grep -v "dmesg(1) may have more information after failed mount" >&2)

_filter_error_mount()?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] common/rc: filter out extra mount error

[Zorro Lang]

On Fri, May 27, 2022 at 09:45:06AM +1000, Dave Chinner wrote:
> On Thu, May 26, 2022 at 05:57:04PM +0800, Zorro Lang wrote:
> > The lastest mount command (from util-linux) merged below commit:
> >   79534c0d7e0f ("mount: add hint about dmesg(8) to error messages")
> > which brought in a new error output when mount fails.
> > 
> > That cause some cases (e.g. xfs/005) fail as:
> >   mount: Structure needs cleaning
> >          dmesg(1) may have more information after failed mount system call
> > 
> > More failed cases like generic/050, ext4/002, xfs/154, etc ...
> > 
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> > 
> > Due to _try_scratch_mount is call in many places, so I'm not sure if it's good
> > to filter this message in _try_scratch_mount. Maybe better to filter it in each
> > failed cases? Or write another function (e.g. _try_scratch_mount_fail) ?
> > Welcome review points.
> > 
> > Thanks,
> > Zorro
> > 
> >  common/rc | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 70a15f9c..67342760 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -335,7 +335,8 @@ _try_scratch_mount()
> >  		_overlay_scratch_mount $*
> >  		return $?
> >  	fi
> > -	_mount -t $FSTYP `_scratch_mount_options $*`
> > +	_mount -t $FSTYP `_scratch_mount_options $*`  2> \
> > +		>(grep -v "dmesg(1) may have more information after failed mount" >&2)
> 
> _filter_error_mount()?

OK, how about this change [1] ?

Do you think doing this "filter out" in _try_scratch_mount is better, or in
each case which expects mount failures in .out file (e.g. xfs/005) ? Or a new
_try_scratch_mount_fail() ?

Thanks,
Zorro

[1]
diff --git a/common/filter b/common/filter
index a6a42b7a..54fb8d68 100644
--- a/common/filter
+++ b/common/filter
@@ -655,5 +655,10 @@ _filter_blkzone_report()
        sed -e 's/len/cap/2'
 }
 
+_filter_mount_error()
+{
+       grep -v "dmesg(1) may have more information after failed mount"
+}
+
 # make sure this script returns success
 /bin/true
diff --git a/common/rc b/common/rc
index 2f31ca46..c16bd405 100644
--- a/common/rc
+++ b/common/rc
@@ -288,7 +288,8 @@ _try_scratch_mount()
                _overlay_scratch_mount $*
                return $?
        fi
-       _mount -t $FSTYP `_scratch_mount_options $*`
+       _mount -t $FSTYP `_scratch_mount_options $*` 2> \
+               >(_filter_mount_error >&2)
        mount_ret=$?
        [ $mount_ret -ne 0 ] && return $mount_ret
        _idmapped_mount $SCRATCH_DEV $SCRATCH_MNT

Thanks,
Zorro

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH] common/rc: filter out extra mount error

[Dave Chinner]

On Fri, May 27, 2022 at 12:21:29PM +0800, Zorro Lang wrote:
> On Fri, May 27, 2022 at 09:45:06AM +1000, Dave Chinner wrote:
> > On Thu, May 26, 2022 at 05:57:04PM +0800, Zorro Lang wrote:
> > > The lastest mount command (from util-linux) merged below commit:
> > >   79534c0d7e0f ("mount: add hint about dmesg(8) to error messages")
> > > which brought in a new error output when mount fails.
> > > 
> > > That cause some cases (e.g. xfs/005) fail as:
> > >   mount: Structure needs cleaning
> > >          dmesg(1) may have more information after failed mount system call
> > > 
> > > More failed cases like generic/050, ext4/002, xfs/154, etc ...
> > > 
> > > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > > ---
> > > 
> > > Due to _try_scratch_mount is call in many places, so I'm not sure if it's good
> > > to filter this message in _try_scratch_mount. Maybe better to filter it in each
> > > failed cases? Or write another function (e.g. _try_scratch_mount_fail) ?
> > > Welcome review points.
> > > 
> > > Thanks,
> > > Zorro
> > > 
> > >  common/rc | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index 70a15f9c..67342760 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -335,7 +335,8 @@ _try_scratch_mount()
> > >  		_overlay_scratch_mount $*
> > >  		return $?
> > >  	fi
> > > -	_mount -t $FSTYP `_scratch_mount_options $*`
> > > +	_mount -t $FSTYP `_scratch_mount_options $*`  2> \
> > > +		>(grep -v "dmesg(1) may have more information after failed mount" >&2)
> > 
> > _filter_error_mount()?
> 
> OK, how about this change [1] ?
> 
> Do you think doing this "filter out" in _try_scratch_mount is better, or in
> each case which expects mount failures in .out file (e.g. xfs/005) ? Or a new
> _try_scratch_mount_fail() ?

I think you missed my point entirely. We already have a filter for this in
common/filter because util-linux has changed it's failure message for
EFSCORRUPTED several times:

# Filter a failed mount output due to EUCLEAN and USTALE, util-linux changed
# the message several times.
#
# prior to v2.21:
# mount: Structure needs cleaning
# v2.21 to v2.29:
# mount: mount <device> on <mountpoint> failed: Structure needs cleaning
# v2.30 and later:
# mount: <mountpoint>: mount(2) system call failed: Structure needs cleaning.
#
# This is also true for ESTALE error. So let's remove all the changing parts
# and keep the 'prior to v2.21' format:
# mount: Structure needs cleaning
# mount: Stale file handle
_filter_error_mount()
{
        sed -e "s/mount:\(.*failed:\)/mount:/" | _filter_ending_dot
}


So, capture and supress the new output via _filter_error_mount() and
use that in the specific tests that are trying to mount a corrupt
filesystem and capture that specific error message in their golden
output....

Cheers,

Dave.


-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] common/rc: filter out extra mount error

[Zorro Lang]

On Fri, May 27, 2022 at 07:13:30PM +1000, Dave Chinner wrote:
> On Fri, May 27, 2022 at 12:21:29PM +0800, Zorro Lang wrote:
> > On Fri, May 27, 2022 at 09:45:06AM +1000, Dave Chinner wrote:
> > > On Thu, May 26, 2022 at 05:57:04PM +0800, Zorro Lang wrote:
> > > > The lastest mount command (from util-linux) merged below commit:
> > > >   79534c0d7e0f ("mount: add hint about dmesg(8) to error messages")
> > > > which brought in a new error output when mount fails.
> > > > 
> > > > That cause some cases (e.g. xfs/005) fail as:
> > > >   mount: Structure needs cleaning
> > > >          dmesg(1) may have more information after failed mount system call
> > > > 
> > > > More failed cases like generic/050, ext4/002, xfs/154, etc ...
> > > > 
> > > > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > > > ---
> > > > 
> > > > Due to _try_scratch_mount is call in many places, so I'm not sure if it's good
> > > > to filter this message in _try_scratch_mount. Maybe better to filter it in each
> > > > failed cases? Or write another function (e.g. _try_scratch_mount_fail) ?
> > > > Welcome review points.
> > > > 
> > > > Thanks,
> > > > Zorro
> > > > 
> > > >  common/rc | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/common/rc b/common/rc
> > > > index 70a15f9c..67342760 100644
> > > > --- a/common/rc
> > > > +++ b/common/rc
> > > > @@ -335,7 +335,8 @@ _try_scratch_mount()
> > > >  		_overlay_scratch_mount $*
> > > >  		return $?
> > > >  	fi
> > > > -	_mount -t $FSTYP `_scratch_mount_options $*`
> > > > +	_mount -t $FSTYP `_scratch_mount_options $*`  2> \
> > > > +		>(grep -v "dmesg(1) may have more information after failed mount" >&2)
> > > 
> > > _filter_error_mount()?
> > 
> > OK, how about this change [1] ?
> > 
> > Do you think doing this "filter out" in _try_scratch_mount is better, or in
> > each case which expects mount failures in .out file (e.g. xfs/005) ? Or a new
> > _try_scratch_mount_fail() ?
> 
> I think you missed my point entirely. We already have a filter for this in
> common/filter because util-linux has changed it's failure message for
> EFSCORRUPTED several times:

Oh, sorry, I didn't notice that there's a _filter_error_mount. I thought you
hope to have one. Thanks, I'll change that function and try to find out all
test cases which use mount errors as golden image.

Thanks,
Zorro

> 
> # Filter a failed mount output due to EUCLEAN and USTALE, util-linux changed
> # the message several times.
> #
> # prior to v2.21:
> # mount: Structure needs cleaning
> # v2.21 to v2.29:
> # mount: mount <device> on <mountpoint> failed: Structure needs cleaning
> # v2.30 and later:
> # mount: <mountpoint>: mount(2) system call failed: Structure needs cleaning.
> #
> # This is also true for ESTALE error. So let's remove all the changing parts
> # and keep the 'prior to v2.21' format:
> # mount: Structure needs cleaning
> # mount: Stale file handle
> _filter_error_mount()
> {
>         sed -e "s/mount:\(.*failed:\)/mount:/" | _filter_ending_dot
> }
> 
> 
> So, capture and supress the new output via _filter_error_mount() and
> use that in the specific tests that are trying to mount a corrupt
> filesystem and capture that specific error message in their golden
> output....
> 
> Cheers,
> 
> Dave.
> 
> 
> -- 
> Dave Chinner
> david@fromorbit.com
>