Message-ID: <20250423141808.2qdmacsyxu3rtrwh@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com>
Date: Wed, 23 Apr 2025 22:18:08 +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 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?

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
> 

Powered by blists - more mailing lists