Re: [PATCH v2 5/5] common: exit --> _exit

["Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com>]

On 4/7/25 21:49, Zorro Lang wrote:
> On Fri, Apr 04, 2025 at 10:34:47AM +0530, Ritesh Harjani wrote:
>> "Nirjhar Roy (IBM)" <nirjhar.roy.lists@gmail.com> writes:
>>
>>> Replace exit <return-val> with _exit <return-val> which
>>> is introduced in the previous patch.
>>>
>>> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
>>> ---
>>>   common/btrfs    |   6 +--
>>>   common/ceph     |   2 +-
>>>   common/config   |   7 ++--
>>>   common/ext4     |   2 +-
>>>   common/populate |   2 +-
>>>   common/preamble |   2 +-
>>>   common/punch    |  12 +++---
>>>   common/rc       | 103 +++++++++++++++++++++++-------------------------
>>>   common/xfs      |   8 ++--
>>>   9 files changed, 70 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/common/btrfs b/common/btrfs
>>> index a3b9c12f..3725632c 100644
>>> --- a/common/btrfs
>>> +++ b/common/btrfs
>>> @@ -80,7 +80,7 @@ _require_btrfs_mkfs_feature()
>>>   {
>>>   	if [ -z $1 ]; then
>>>   		echo "Missing feature name argument for _require_btrfs_mkfs_feature"
>>> -		exit 1
>>> +		_exit 1
>>>   	fi
>>>   	feat=$1
>>>   	$MKFS_BTRFS_PROG -O list-all 2>&1 | \
>>> @@ -104,7 +104,7 @@ _require_btrfs_fs_feature()
>>>   {
>>>   	if [ -z $1 ]; then
>>>   		echo "Missing feature name argument for _require_btrfs_fs_feature"
>>> -		exit 1
>>> +		_exit 1
>>>   	fi
>>>   	feat=$1
>>>   	modprobe btrfs > /dev/null 2>&1
>>> @@ -214,7 +214,7 @@ _check_btrfs_filesystem()
>>>   	if [ $ok -eq 0 ]; then
>>>   		status=1
>>>   		if [ "$iam" != "check" ]; then
>>> -			exit 1
>>> +			_exit 1
>>>   		fi
>>>   		return 1
>>>   	fi
>>> diff --git a/common/ceph b/common/ceph
>>> index d6f24df1..df7a6814 100644
>>> --- a/common/ceph
>>> +++ b/common/ceph
>>> @@ -14,7 +14,7 @@ _ceph_create_file_layout()
>>>   
>>>   	if [ -e $fname ]; then
>>>   		echo "File $fname already exists."
>>> -		exit 1
>>> +		_exit 1
>>>   	fi
>>>   	touch $fname
>>>   	$SETFATTR_PROG -n ceph.file.layout \
>>> diff --git a/common/config b/common/config
>>> index eb6af35a..4c5435b7 100644
>>> --- a/common/config
>>> +++ b/common/config
>>> @@ -123,8 +123,7 @@ set_mkfs_prog_path_with_opts()
>>>   _fatal()
>>>   {
>>>       echo "$*"
>>> -    status=1
>>> -    exit 1
>>> +    _exit 1
>>>   }
>>>   
>>>   export MKFS_PROG="$(type -P mkfs)"
>>> @@ -868,7 +867,7 @@ get_next_config() {
>>>   		echo "Warning: need to define parameters for host $HOST"
>>>   		echo "       or set variables:"
>>>   		echo "       $MC"
>>> -		exit 1
>>> +		_exit 1
>>>   	fi
>>>   
>>>   	_check_device TEST_DEV required $TEST_DEV
>>> @@ -879,7 +878,7 @@ get_next_config() {
>>>   	if [ ! -z "$SCRATCH_DEV_POOL" ]; then
>>>   		if [ ! -z "$SCRATCH_DEV" ]; then
>>>   			echo "common/config: Error: \$SCRATCH_DEV ($SCRATCH_DEV) should be unset when \$SCRATCH_DEV_POOL ($SCRATCH_DEV_POOL) is set"
>>> -			exit 1
>>> +			_exit 1
>>>   		fi
>>>   		SCRATCH_DEV=`echo $SCRATCH_DEV_POOL | awk '{print $1}'`
>>>   		export SCRATCH_DEV
>>> diff --git a/common/ext4 b/common/ext4
>>> index e1b336d3..f88fa532 100644
>>> --- a/common/ext4
>>> +++ b/common/ext4
>>> @@ -182,7 +182,7 @@ _require_scratch_ext4_feature()
>>>   {
>>>       if [ -z "$1" ]; then
>>>           echo "Usage: _require_scratch_ext4_feature feature"
>>> -        exit 1
>>> +        _exit 1
>>>       fi
>>>       $MKFS_EXT4_PROG -F $MKFS_OPTIONS -O "$1" \
>>>   		    $SCRATCH_DEV 512m >/dev/null 2>&1 \
>>> diff --git a/common/populate b/common/populate
>>> index 7352f598..50dc75d3 100644
>>> --- a/common/populate
>>> +++ b/common/populate
>>> @@ -1003,7 +1003,7 @@ _fill_fs()
>>>   
>>>   	if [ $# -ne 4 ]; then
>>>   		echo "Usage: _fill_fs filesize dir blocksize switch_user"
>>> -		exit 1
>>> +		_exit 1
>>>   	fi
>>>   
>>>   	if [ $switch_user -eq 0 ]; then
>>> diff --git a/common/preamble b/common/preamble
>>> index c92e55bb..ba029a34 100644
>>> --- a/common/preamble
>>> +++ b/common/preamble
>>> @@ -35,7 +35,7 @@ _begin_fstest()
>>>   {
>>>   	if [ -n "$seq" ]; then
>>>   		echo "_begin_fstest can only be called once!"
>>> -		exit 1
>>> +		_exit 1
>>>   	fi
>>>   
>>>   	seq=`basename $0`
>>> diff --git a/common/punch b/common/punch
>>> index 43ccab69..6567b9d1 100644
>>> --- a/common/punch
>>> +++ b/common/punch
>>> @@ -172,16 +172,16 @@ _filter_fiemap_flags()
>>>   	$AWK_PROG -e "$awk_script" | _coalesce_extents
>>>   }
>>>   
>>> -# Filters fiemap output to only print the
>>> +# Filters fiemap output to only print the
>>>   # file offset column and whether or not
>>>   # it is an extent or a hole
>>>   _filter_hole_fiemap()
>>>   {
>>>   	$AWK_PROG '
>>>   		$3 ~ /hole/ {
>>> -			print $1, $2, $3;
>>> +			print $1, $2, $3;
>>>   			next;
>>> -		}
>>> +		}
>>>   		$5 ~ /0x[[:xdigit:]]+/ {
>>>   			print $1, $2, "extent";
>>>   		}' |
>>> @@ -225,7 +225,7 @@ _filter_bmap()
>>>   die_now()
>>>   {
>>>   	status=1
>>> -	exit
>>> +	_exit
>> Why not remove status=1 too and just do _exit 1 here too?
>> Like how we have done at other places?
> Yeah, nice catch! As the defination of _exit:
>
>    _exit()
>    {
>         status="$1"
>         exit "$status"
>    }
>
> The
>    "
>    status=1
>    exit
>    "
> should be equal to:
>    "
>    _exit 1
>    "
>
> And "_exit" looks not make sense, due to it gives null to status.
>
> Same problem likes below:
>
>
> @@ -3776,7 +3773,7 @@ _get_os_name()
>                  echo 'linux'
>          else
>                  echo Unknown operating system: `uname`
> -               exit
> +               _exit
>
>
> The "_exit" without argument looks not make sense.

Yes, thank you Ritesh and Zorro. Yes it should have been "_exit 1". I 
missed it while making the changes. I will make the changes in v3 and 
add RBs from Dave[1] and Ritesh.

[1] https://lore.kernel.org/all/Z-xcne3f5Klvuxcq@dread.disaster.area/

>
> Thanks,
> Zorro
>
>> Rest looks good to me.
>>
>> -ritesh
>>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore