Message-ID: <20250425133648.5uygihyqo7vqofi3@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com>
Date: Fri, 25 Apr 2025 21:36:48 +0800
From: Zorro Lang <zlang@...hat.com>
To: "Nirjhar Roy (IBM)" <nirjhar.roy.lists@...il.com>
Cc: fstests@...r.kernel.org, linux-ext4@...r.kernel.org,
	linux-xfs@...r.kernel.org, ritesh.list@...il.com,
	ojaswin@...ux.ibm.com, djwong@...nel.org, zlang@...nel.org,
	david@...morbit.com
Subject: Re: [PATCH v1 1/2] common: Move exit related functions to a
 common/exit

On Fri, Apr 25, 2025 at 05:33:24PM +0530, Nirjhar Roy (IBM) wrote:
> 
> On 4/25/25 16:57, Zorro Lang wrote:
> > On Thu, Apr 24, 2025 at 02:39:39PM +0530, Nirjhar Roy (IBM) wrote:
> > > On 4/23/25 19:48, Zorro Lang wrote:
> > > > On Wed, Apr 23, 2025 at 06:41:34AM +0000, Nirjhar Roy (IBM) wrote:
> > > > > Introduce a new file common/exit that will contain all the exit
> > > > > related functions. This will remove the dependencies these functions
> > > > > have on other non-related helper files and they can be indepedently
> > > > > sourced. This was suggested by Dave Chinner[1].
> > > > > 
> > > > > [1] https://lore.kernel.org/linux-xfs/Z_UJ7XcpmtkPRhTr@dread.disaster.area/
> > > > > Suggested-by: Dave Chinner <david@...morbit.com>
> > > > > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@...il.com>
> > > > > ---
> > > > >    check           |  1 +
> > > > >    common/btrfs    |  2 +-
> > > > >    common/ceph     |  2 ++
> > > > >    common/config   | 17 +----------------
> > > > >    common/dump     |  1 +
> > > > >    common/exit     | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >    common/ext4     |  2 +-
> > > > >    common/populate |  2 +-
> > > > >    common/preamble |  1 +
> > > > >    common/punch    |  6 +-----
> > > > >    common/rc       | 29 +---------------------------
> > > > >    common/repair   |  1 +
> > > > >    common/xfs      |  1 +
> > > > I think if you define exit helpers in common/exit, and import common/exit
> > > > in common/config, then you don't need to source it(common/exit) in other
> > > > common files (.e.g common/xfs, common/rc, etc). Due to when we call the
> > > > helpers in these common files, the process should already imported
> > > > common/rc -> common/config -> common/exit. right?
> > > Oh, right. I can remove the redundant imports from
> > > common/{btrfs,ceph,dump,ext4,populate,preamble,punch,rc,repair,xfs} in v2. I
> > > will keep ". common/exit" only in common/config and check. The reason for me
> > > to keep it in check is that before common/rc is sourced in check, we might
> > > need _exit() (which is present is common/exit). Do you agree?
> > I thought "check" might not need that either. I didn't give it a test, but I found
> > before importing common/rc, there're only command arguments initialization, and
> > "check" calls "exit" directly if the initialization fails (except you want to call
> > _exit, but I didn't see you change that).
> 
> Yes, I have changed the exit() to _exit() in "check" in the next patch [1]
> of this series. Can you please take a look at that patch[1] and suggest
> whether I should have ". common/exit" in "check" or not?
> 
> 
> [1] https://lore.kernel.org/all/7d8587b8342ee2cbe226fb691b372ac7df5fdb71.1745390030.git.nirjhar.roy.lists@gmail.com/

Oh, as "check" has:

  if $OPTIONS_HAVE_SECTIONS; then
          trap "_summary; exit \$status" 0 1 2 3 15
  else
          trap "_wrapup; exit \$status" 0 1 2 3 15
  fi

So I think it makes sense to use _exit() to deal with status variable :)

> 
> --NR
> 
> > 
> > Thanks,
> > Zorro
> > 
> > > --NR
> > > 
> > > > Thanks,
> > > > Zorro
> > > > 
> > > > >    13 files changed, 63 insertions(+), 52 deletions(-)
> > > > >    create mode 100644 common/exit
> > > > > 
> > > > > diff --git a/check b/check
> > > > > index 9451c350..67355c52 100755
> > > > > --- a/check
> > > > > +++ b/check
> > > > > @@ -51,6 +51,7 @@ rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
> > > > >    SRC_GROUPS="generic"
> > > > >    export SRC_DIR="tests"
> > > > > +. common/exit
> > > > >    usage()
> > > > >    {
> > > > > diff --git a/common/btrfs b/common/btrfs
> > > > > index 3725632c..9e91ee71 100644
> > > > > --- a/common/btrfs
> > > > > +++ b/common/btrfs
> > > > > @@ -1,7 +1,7 @@
> > > > >    #
> > > > >    # Common btrfs specific functions
> > > > >    #
> > > > > -
> > > > > +. common/exit
> > > > >    . common/module
> > > > >    # The recommended way to execute simple "btrfs" command.
> > > > > diff --git a/common/ceph b/common/ceph
> > > > > index df7a6814..89e36403 100644
> > > > > --- a/common/ceph
> > > > > +++ b/common/ceph
> > > > > @@ -2,6 +2,8 @@
> > > > >    # CephFS specific common functions.
> > > > >    #
> > > > > +. common/exit
> > > > > +
> > > > >    # _ceph_create_file_layout <filename> <stripe unit> <stripe count> <object size>
> > > > >    # This function creates a new empty file and sets the file layout according to
> > > > >    # parameters.  It will exit if the file already exists.
> > > > > diff --git a/common/config b/common/config
> > > > > index eada3971..6a60d144 100644
> > > > > --- a/common/config
> > > > > +++ b/common/config
> > > > > @@ -38,7 +38,7 @@
> > > > >    # - this script shouldn't make any assertions about filesystem
> > > > >    #   validity or mountedness.
> > > > >    #
> > > > > -
> > > > > +. common/exit
> > > > >    . common/test_names
> > > > >    # all tests should use a common language setting to prevent golden
> > > > > @@ -96,15 +96,6 @@ export LOCAL_CONFIGURE_OPTIONS=${LOCAL_CONFIGURE_OPTIONS:=--enable-readline=yes}
> > > > >    export RECREATE_TEST_DEV=${RECREATE_TEST_DEV:=false}
> > > > > -# This functions sets the exit code to status and then exits. Don't use
> > > > > -# exit directly, as it might not set the value of "$status" correctly, which is
> > > > > -# used as an exit code in the trap handler routine set up by the check script.
> > > > > -_exit()
> > > > > -{
> > > > > -	test -n "$1" && status="$1"
> > > > > -	exit "$status"
> > > > > -}
> > > > > -
> > > > >    # Handle mkfs.$fstyp which does (or does not) require -f to overwrite
> > > > >    set_mkfs_prog_path_with_opts()
> > > > >    {
> > > > > @@ -121,12 +112,6 @@ set_mkfs_prog_path_with_opts()
> > > > >    	fi
> > > > >    }
> > > > > -_fatal()
> > > > > -{
> > > > > -    echo "$*"
> > > > > -    _exit 1
> > > > > -}
> > > > > -
> > > > >    export MKFS_PROG="$(type -P mkfs)"
> > > > >    [ "$MKFS_PROG" = "" ] && _fatal "mkfs not found"
> > > > > diff --git a/common/dump b/common/dump
> > > > > index 09859006..4701a956 100644
> > > > > --- a/common/dump
> > > > > +++ b/common/dump
> > > > > @@ -3,6 +3,7 @@
> > > > >    # Copyright (c) 2000-2002,2005 Silicon Graphics, Inc.  All Rights Reserved.
> > > > >    #
> > > > >    # Functions useful for xfsdump/xfsrestore tests
> > > > > +. common/exit
> > > > >    # --- initializations ---
> > > > >    rm -f $seqres.full
> > > > > diff --git a/common/exit b/common/exit
> > > > > new file mode 100644
> > > > > index 00000000..ad7e7498
> > > > > --- /dev/null
> > > > > +++ b/common/exit
> > > > > @@ -0,0 +1,50 @@
> > > > > +##/bin/bash
> > > > > +
> > > > > +# This functions sets the exit code to status and then exits. Don't use
> > > > > +# exit directly, as it might not set the value of "$status" correctly, which is
> > > > > +# used as an exit code in the trap handler routine set up by the check script.
> > > > > +_exit()
> > > > > +{
> > > > > +	test -n "$1" && status="$1"
> > > > > +	exit "$status"
> > > > > +}
> > > > > +
> > > > > +_fatal()
> > > > > +{
> > > > > +    echo "$*"
> > > > > +    _exit 1
> > > > > +}
> > > > > +
> > > > > +_die()
> > > > > +{
> > > > > +        echo $@
> > > > > +        _exit 1
> > > > > +}
> > > > > +
> > > > > +die_now()
> > > > > +{
> > > > > +	_exit 1
> > > > > +}
> > > > > +
> > > > > +# just plain bail out
> > > > > +#
> > > > > +_fail()
> > > > > +{
> > > > > +    echo "$*" | tee -a $seqres.full
> > > > > +    echo "(see $seqres.full for details)"
> > > > > +    _exit 1
> > > > > +}
> > > > > +
> > > > > +# bail out, setting up .notrun file. Need to kill the filesystem check files
> > > > > +# here, otherwise they are set incorrectly for the next test.
> > > > > +#
> > > > > +_notrun()
> > > > > +{
> > > > > +    echo "$*" > $seqres.notrun
> > > > > +    echo "$seq not run: $*"
> > > > > +    rm -f ${RESULT_DIR}/require_test*
> > > > > +    rm -f ${RESULT_DIR}/require_scratch*
> > > > > +
> > > > > +    _exit 0
> > > > > +}
> > > > > +
> > > > > diff --git a/common/ext4 b/common/ext4
> > > > > index f88fa532..ab566c41 100644
> > > > > --- a/common/ext4
> > > > > +++ b/common/ext4
> > > > > @@ -1,7 +1,7 @@
> > > > >    #
> > > > >    # ext4 specific common functions
> > > > >    #
> > > > > -
> > > > > +. common/exit
> > > > >    __generate_ext4_report_vars() {
> > > > >    	__generate_blockdev_report_vars TEST_LOGDEV
> > > > >    	__generate_blockdev_report_vars SCRATCH_LOGDEV
> > > > > diff --git a/common/populate b/common/populate
> > > > > index 50dc75d3..a17acc9e 100644
> > > > > --- a/common/populate
> > > > > +++ b/common/populate
> > > > > @@ -4,7 +4,7 @@
> > > > >    #
> > > > >    # Routines for populating a scratch fs, and helpers to exercise an FS
> > > > >    # once it's been fuzzed.
> > > > > -
> > > > > +. common/exit
> > > > >    . ./common/quota
> > > > >    _require_populate_commands() {
> > > > > diff --git a/common/preamble b/common/preamble
> > > > > index ba029a34..0f306412 100644
> > > > > --- a/common/preamble
> > > > > +++ b/common/preamble
> > > > > @@ -3,6 +3,7 @@
> > > > >    # Copyright (c) 2021 Oracle.  All Rights Reserved.
> > > > >    # Boilerplate fstests functionality
> > > > > +. common/exit
> > > > >    # Standard cleanup function.  Individual tests can override this.
> > > > >    _cleanup()
> > > > > diff --git a/common/punch b/common/punch
> > > > > index 64d665d8..637f463f 100644
> > > > > --- a/common/punch
> > > > > +++ b/common/punch
> > > > > @@ -3,6 +3,7 @@
> > > > >    # Copyright (c) 2007 Silicon Graphics, Inc.  All Rights Reserved.
> > > > >    #
> > > > >    # common functions for excersizing hole punches with extent size hints etc.
> > > > > +. common/exit
> > > > >    _spawn_test_file() {
> > > > >    	echo "# spawning test file with $*"
> > > > > @@ -222,11 +223,6 @@ _filter_bmap()
> > > > >    	_coalesce_extents
> > > > >    }
> > > > > -die_now()
> > > > > -{
> > > > > -	_exit 1
> > > > > -}
> > > > > -
> > > > >    # test the different corner cases for zeroing a range:
> > > > >    #
> > > > >    #	1. into a hole
> > > > > diff --git a/common/rc b/common/rc
> > > > > index 9bed6dad..945f5134 100644
> > > > > --- a/common/rc
> > > > > +++ b/common/rc
> > > > > @@ -2,6 +2,7 @@
> > > > >    # SPDX-License-Identifier: GPL-2.0+
> > > > >    # Copyright (c) 2000-2006 Silicon Graphics, Inc.  All Rights Reserved.
> > > > > +. common/exit
> > > > >    . common/config
> > > > >    BC="$(type -P bc)" || BC=
> > > > > @@ -1798,28 +1799,6 @@ _do()
> > > > >        return $ret
> > > > >    }
> > > > > -# bail out, setting up .notrun file. Need to kill the filesystem check files
> > > > > -# here, otherwise they are set incorrectly for the next test.
> > > > > -#
> > > > > -_notrun()
> > > > > -{
> > > > > -    echo "$*" > $seqres.notrun
> > > > > -    echo "$seq not run: $*"
> > > > > -    rm -f ${RESULT_DIR}/require_test*
> > > > > -    rm -f ${RESULT_DIR}/require_scratch*
> > > > > -
> > > > > -    _exit 0
> > > > > -}
> > > > > -
> > > > > -# just plain bail out
> > > > > -#
> > > > > -_fail()
> > > > > -{
> > > > > -    echo "$*" | tee -a $seqres.full
> > > > > -    echo "(see $seqres.full for details)"
> > > > > -    _exit 1
> > > > > -}
> > > > > -
> > > > >    #
> > > > >    # Tests whether $FSTYP should be exclude from this test.
> > > > >    #
> > > > > @@ -3835,12 +3814,6 @@ _link_out_file()
> > > > >    	_link_out_file_named $seqfull.out "$features"
> > > > >    }
> > > > > -_die()
> > > > > -{
> > > > > -        echo $@
> > > > > -        _exit 1
> > > > > -}
> > > > > -
> > > > >    # convert urandom incompressible data to compressible text data
> > > > >    _ddt()
> > > > >    {
> > > > > diff --git a/common/repair b/common/repair
> > > > > index fd206f8e..db6a1b5c 100644
> > > > > --- a/common/repair
> > > > > +++ b/common/repair
> > > > > @@ -3,6 +3,7 @@
> > > > >    # Copyright (c) 2000-2002 Silicon Graphics, Inc.  All Rights Reserved.
> > > > >    #
> > > > >    # Functions useful for xfs_repair tests
> > > > > +. common/exit
> > > > >    _zero_position()
> > > > >    {
> > > > > diff --git a/common/xfs b/common/xfs
> > > > > index 96c15f3c..c236146c 100644
> > > > > --- a/common/xfs
> > > > > +++ b/common/xfs
> > > > > @@ -1,6 +1,7 @@
> > > > >    #
> > > > >    # XFS specific common functions.
> > > > >    #
> > > > > +. common/exit
> > > > >    __generate_xfs_report_vars() {
> > > > >    	__generate_blockdev_report_vars TEST_RTDEV
> > > > > -- 
> > > > > 2.34.1
> > > > > 
> > > -- 
> > > Nirjhar Roy
> > > Linux Kernel Developer
> > > IBM, Bangalore
> > > 
> -- 
> Nirjhar Roy
> Linux Kernel Developer
> IBM, Bangalore
> 

Powered by blists - more mailing lists