[PATCH] fsck.xfs: fix fsck.xfs run by different shells when fsck.mode=force is set

[PATCH] fsck.xfs: fix fsck.xfs run by different shells when fsck.mode=force is set

[Gerald Yang]

When fsck.mode=force is specified in the kernel command line, fsck.xfs
is executed during the boot process. However, when the default shell is
not bash, $PS1 should be a different value, consider the following script:
cat ps1.sh
echo "$PS1"

run ps1.sh with different shells:
ash ./ps1.sh
$
bash ./ps1.sh

dash ./ps1.sh
$
ksh ./ps1.sh

zsh ./ps1.sh

On systems like Ubuntu, where dash is the default shell during the boot
process to improve startup speed. This results in FORCE being incorrectly
set to false and then xfs_repair is not invoked:
if [ -n "$PS1" -o -t 0 ]; then
        FORCE=false
fi

Other distros may encounter this issue too if the default shell is set
to anoother shell.

Check "-t 0" is enough to determine if we are in interactive mode, and
xfs_repair is invoked as expected regardless of the shell used.

Fixes: 04a2d5dc ("fsck.xfs: allow forced repairs using xfs_repair")
Signed-off-by: Gerald Yang <gerald.yang@canonical.com>
---
 fsck/xfs_fsck.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fsck/xfs_fsck.sh b/fsck/xfs_fsck.sh
index 62a1e0b3..19ada9a7 100755
--- a/fsck/xfs_fsck.sh
+++ b/fsck/xfs_fsck.sh
@@ -55,12 +55,12 @@ fi
 # directly.
 #
 # Use multiple methods to capture most of the cases:
-# The case for *i* and -n "$PS1" are commonly suggested in bash manual
+# The case for *i* is commonly suggested in bash manual
 # and the -t 0 test checks stdin
 case $- in
 	*i*) FORCE=false ;;
 esac
-if [ -n "$PS1" -o -t 0 ]; then
+if [ -t 0 ]; then
 	FORCE=false
 fi
 
-- 
2.43.0

Re: [PATCH] fsck.xfs: fix fsck.xfs run by different shells when fsck.mode=force is set

[Darrick J. Wong]

On Tue, Aug 13, 2024 at 03:25:51PM +0800, Gerald Yang wrote:
> When fsck.mode=force is specified in the kernel command line, fsck.xfs
> is executed during the boot process. However, when the default shell is
> not bash, $PS1 should be a different value, consider the following script:
> cat ps1.sh
> echo "$PS1"
> 
> run ps1.sh with different shells:
> ash ./ps1.sh
> $
> bash ./ps1.sh
> 
> dash ./ps1.sh
> $
> ksh ./ps1.sh
> 
> zsh ./ps1.sh
> 
> On systems like Ubuntu, where dash is the default shell during the boot
> process to improve startup speed. This results in FORCE being incorrectly
> set to false and then xfs_repair is not invoked:
> if [ -n "$PS1" -o -t 0 ]; then
>         FORCE=false
> fi
> 
> Other distros may encounter this issue too if the default shell is set
> to anoother shell.
> 
> Check "-t 0" is enough to determine if we are in interactive mode, and
> xfs_repair is invoked as expected regardless of the shell used.
> 
> Fixes: 04a2d5dc ("fsck.xfs: allow forced repairs using xfs_repair")
> Signed-off-by: Gerald Yang <gerald.yang@canonical.com>
> ---
>  fsck/xfs_fsck.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fsck/xfs_fsck.sh b/fsck/xfs_fsck.sh
> index 62a1e0b3..19ada9a7 100755
> --- a/fsck/xfs_fsck.sh
> +++ b/fsck/xfs_fsck.sh
> @@ -55,12 +55,12 @@ fi
>  # directly.
>  #
>  # Use multiple methods to capture most of the cases:
> -# The case for *i* and -n "$PS1" are commonly suggested in bash manual
> +# The case for *i* is commonly suggested in bash manual
>  # and the -t 0 test checks stdin
>  case $- in
>  	*i*) FORCE=false ;;

I can't remember why we allow any argument with the letter 'i' in it to
derail an xfs_repair -f invocation??

Regardless, the bits you changed look correct so
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  esac
> -if [ -n "$PS1" -o -t 0 ]; then
> +if [ -t 0 ]; then
>  	FORCE=false
>  fi
>  
> -- 
> 2.43.0
> 
> 

Re: [PATCH] fsck.xfs: fix fsck.xfs run by different shells when fsck.mode=force is set

[Gerald Yang]

Thank you Darrick for the review
I just would like to ask if this will be merged into for-next soon?


On Tue, Aug 13, 2024 at 10:53 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Aug 13, 2024 at 03:25:51PM +0800, Gerald Yang wrote:
> > When fsck.mode=force is specified in the kernel command line, fsck.xfs
> > is executed during the boot process. However, when the default shell is
> > not bash, $PS1 should be a different value, consider the following script:
> > cat ps1.sh
> > echo "$PS1"
> >
> > run ps1.sh with different shells:
> > ash ./ps1.sh
> > $
> > bash ./ps1.sh
> >
> > dash ./ps1.sh
> > $
> > ksh ./ps1.sh
> >
> > zsh ./ps1.sh
> >
> > On systems like Ubuntu, where dash is the default shell during the boot
> > process to improve startup speed. This results in FORCE being incorrectly
> > set to false and then xfs_repair is not invoked:
> > if [ -n "$PS1" -o -t 0 ]; then
> >         FORCE=false
> > fi
> >
> > Other distros may encounter this issue too if the default shell is set
> > to anoother shell.
> >
> > Check "-t 0" is enough to determine if we are in interactive mode, and
> > xfs_repair is invoked as expected regardless of the shell used.
> >
> > Fixes: 04a2d5dc ("fsck.xfs: allow forced repairs using xfs_repair")
> > Signed-off-by: Gerald Yang <gerald.yang@canonical.com>
> > ---
> >  fsck/xfs_fsck.sh | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fsck/xfs_fsck.sh b/fsck/xfs_fsck.sh
> > index 62a1e0b3..19ada9a7 100755
> > --- a/fsck/xfs_fsck.sh
> > +++ b/fsck/xfs_fsck.sh
> > @@ -55,12 +55,12 @@ fi
> >  # directly.
> >  #
> >  # Use multiple methods to capture most of the cases:
> > -# The case for *i* and -n "$PS1" are commonly suggested in bash manual
> > +# The case for *i* is commonly suggested in bash manual
> >  # and the -t 0 test checks stdin
> >  case $- in
> >       *i*) FORCE=false ;;
>
> I can't remember why we allow any argument with the letter 'i' in it to
> derail an xfs_repair -f invocation??
>
> Regardless, the bits you changed look correct so
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>
> --D
>
> >  esac
> > -if [ -n "$PS1" -o -t 0 ]; then
> > +if [ -t 0 ]; then
> >       FORCE=false
> >  fi
> >
> > --
> > 2.43.0
> >
> >

Re: [PATCH] fsck.xfs: fix fsck.xfs run by different shells when fsck.mode=force is set

[Carlos Maiolino]

On Fri, Sep 06, 2024 at 11:44:37AM GMT, Gerald Yang wrote:
> Thank you Darrick for the review
> I just would like to ask if this will be merged into for-next soon?

This is already in my queue, but got delayed due the problems with C++ compiler,
hopefully some day next week it should hit for-next.

Carlos

> 
> 
> On Tue, Aug 13, 2024 at 10:53 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Tue, Aug 13, 2024 at 03:25:51PM +0800, Gerald Yang wrote:
> > > When fsck.mode=force is specified in the kernel command line, fsck.xfs
> > > is executed during the boot process. However, when the default shell is
> > > not bash, $PS1 should be a different value, consider the following script:
> > > cat ps1.sh
> > > echo "$PS1"
> > >
> > > run ps1.sh with different shells:
> > > ash ./ps1.sh
> > > $
> > > bash ./ps1.sh
> > >
> > > dash ./ps1.sh
> > > $
> > > ksh ./ps1.sh
> > >
> > > zsh ./ps1.sh
> > >
> > > On systems like Ubuntu, where dash is the default shell during the boot
> > > process to improve startup speed. This results in FORCE being incorrectly
> > > set to false and then xfs_repair is not invoked:
> > > if [ -n "$PS1" -o -t 0 ]; then
> > >         FORCE=false
> > > fi
> > >
> > > Other distros may encounter this issue too if the default shell is set
> > > to anoother shell.
> > >
> > > Check "-t 0" is enough to determine if we are in interactive mode, and
> > > xfs_repair is invoked as expected regardless of the shell used.
> > >
> > > Fixes: 04a2d5dc ("fsck.xfs: allow forced repairs using xfs_repair")
> > > Signed-off-by: Gerald Yang <gerald.yang@canonical.com>
> > > ---
> > >  fsck/xfs_fsck.sh | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fsck/xfs_fsck.sh b/fsck/xfs_fsck.sh
> > > index 62a1e0b3..19ada9a7 100755
> > > --- a/fsck/xfs_fsck.sh
> > > +++ b/fsck/xfs_fsck.sh
> > > @@ -55,12 +55,12 @@ fi
> > >  # directly.
> > >  #
> > >  # Use multiple methods to capture most of the cases:
> > > -# The case for *i* and -n "$PS1" are commonly suggested in bash manual
> > > +# The case for *i* is commonly suggested in bash manual
> > >  # and the -t 0 test checks stdin
> > >  case $- in
> > >       *i*) FORCE=false ;;
> >
> > I can't remember why we allow any argument with the letter 'i' in it to
> > derail an xfs_repair -f invocation??
> >
> > Regardless, the bits you changed look correct so
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> >
> > --D
> >
> > >  esac
> > > -if [ -n "$PS1" -o -t 0 ]; then
> > > +if [ -t 0 ]; then
> > >       FORCE=false
> > >  fi
> > >
> > > --
> > > 2.43.0
> > >
> > >
>