Message-ID: <054fa772-360e-4f90-bc4d-ea7ef954d5a2@gmail.com>
Date: Thu, 24 Apr 2025 14:39:39 +0530
From: "Nirjhar Roy (IBM)" <nirjhar.roy.lists@...il.com>
To: Zorro Lang <zlang@...hat.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 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?

--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

Powered by blists - more mailing lists