Minor cleanups in common/
v2 cover Minor cleanups in common/
This patch series removes some unnecessary sourcing of common/rc and decouples the call to init_rc() from the sourcing of common/rc. This is proposed in [1] and [2]. It also removes direct usage of exit command with a _exit wrapper. The individual patches have the details.
[v1] --> v[2]
1. Added R.B from Darrick in patch 1 of [v1]
2. Kept the init_rc call that was deleted in the v1.
3. Introduced _exit wrapper around exit command. This will help us get correct exit codes ("$?") on failures.
[1] https://lore.kernel.org/all/20250206155251.GA21787@frogsfrogsfrogs/
[v1] https://lore.kernel.org/all/[email protected]/
Nirjhar Roy (IBM) (5):
generic/749: Remove redundant sourcing of common/rc
check: Remove redundant _test_mount in check check,common{rc,preamble}: Decouple init_rc() call from sourcing
common/rc
common/config: Introduce _exit wrapper around exit command
common: exit --> _exit
check | 8 +--- common/btrfs | 6 +-- common/ceph | 2 +- common/config | 15 +++++-- common/ext4 | 2 +- common/populate | 2 +- common/preamble | 3 +- common/punch | 12 +++--- common/rc | 105 ++++++++++++++++++++++------------------------ common/xfs | 8 ++-- tests/generic/749 | 1 - 11 files changed, 81 insertions(+), 83 deletions(-)
--
2.34.1
PATCH v2 1/5 generic/749: Remove redundant sourcing of common/rc
common/rc is already sourced before the test starts running in _begin_fstest() preamble.
--- tests/generic/749 | 1 - 1 file changed, 1 deletion(-)
tests/generic/749 +0 -1
@@ -15,7 +15,6 @@ # boundary and ensures we get a SIGBUS if we write to data beyond the system # page size even if the block size is greater than the system page size. . ./common/preamble -. ./common/rc _begin_fstest auto quick prealloc # Import common functions.
Signature · 1 line
2.34.1
PATCH v2 2/5 check: Remove redundant _test_mount in check
In reply to Nirjhar Roy (IBM)init_rc already does a _test_mount. Hence removing the additional _test_mount call when OLD_TEST_FS_MOUNT_OPTS != TEST_FS_MOUNT_OPTS.
--- check | 6 ------ 1 file changed, 6 deletions(-)
check +0 -6
@@ -792,12 +792,6 @@ function run_section() _prepare_test_list elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then _test_unmount 2> /dev/null - if ! _test_mount - then - echo "check: failed to mount $TEST_DEV on $TEST_DIR" - status=1 - exit - fi fi init_rc
Signature · 1 line
2.34.1
PATCH v2 3/5 check,common{rc,preamble}: Decouple init_rc() call from sourcing common/rc
In reply to Nirjhar Roy (IBM)Silently executing scripts during sourcing common/rc isn't good practice and also causes unnecessary script execution. Decouple init_rc() call and call init_rc() explicitly where required.
--- check | 2 ++ common/preamble | 1 + common/rc | 2 -- 3 files changed, 3 insertions(+), 2 deletions(-)
check +2 -0
@@ -364,6 +364,8 @@ if ! . ./common/rc; then exit 1 fi +init_rc + # If the test config specified a soak test duration, see if there are any # unit suffixes that need converting to an integer seconds count. if [ -n "$SOAK_DURATION" ]; then
common/preamble +1 -0
@@ -50,6 +50,7 @@ _begin_fstest() _register_cleanup _cleanup . ./common/rc + init_rc # remove previous $seqres.full before test rm -f $seqres.full $seqres.hints
common/rc +0 -2
@@ -5817,8 +5817,6 @@ _require_program() { _have_program "$1" || _notrun "$tag required" } -init_rc - ################################################################################ # make sure this script returns success /bin/true
Signature · 1 line
2.34.1
PATCH v2 4/5 common/config: Introduce _exit wrapper around exit command
In reply to Nirjhar Roy (IBM)We should always set the value of status correctly when we are exiting. Else, "$?" might not give us the correct value.
If we see the following trap
handler registration in the check script:
if $OPTIONS_HAVE_SECTIONS; then
trap "_kill_seq; _summary; exit \$status" 0 1 2 3 15
else
trap "_kill_seq; _wrapup; exit \$status" 0 1 2 3 15fi
So, "exit 1" will exit the check script without setting the correct return value. I ran with the following local.config file:
[xfs_4k_valid]
FSTYP=xfs
TEST_DEV=/dev/loop0
TEST_DIR=/mnt1/test
SCRATCH_DEV=/dev/loop1
SCRATCH_MNT=/mnt1/scratch
[xfs_4k_invalid]
FSTYP=xfs
TEST_DEV=/dev/loop0
TEST_DIR=/mnt1/invalid_dir
SCRATCH_DEV=/dev/loop1
SCRATCH_MNT=/mnt1/scratch
This caused the init_rc() to catch the case of invalid _test_mount options. Although the check script correctly failed during the execution of the "xfs_4k_invalid" section, the return value was 0, i.e "echo $?" returned 0. This is because init_rc exits with "exit 1" without correctly setting the value of "status". IMO, the correct behavior should have been that "$?" should have been non-zero.
The next patch will replace exit with _exit.
--- common/config | 8 ++++++++ 1 file changed, 8 insertions(+)
common/config +8 -0
@@ -96,6 +96,14 @@ 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. +_exit() +{ + status="$1" + exit "$status" +} + # Handle mkfs.$fstyp which does (or does not) require -f to overwrite set_mkfs_prog_path_with_opts() {
Signature · 1 line
2.34.1
PATCH v2 5/5 common: exit --> _exit
In reply to Nirjhar Roy (IBM)Replace exit <return-val> with _exit <return-val> which is introduced in the previous patch.
--- 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(-)
common/btrfs +3 -3
@@ -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
common/ceph +1 -1
@@ -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 \
common/config +3 -4
@@ -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
common/ext4 +1 -1
@@ -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 \
common/populate +1 -1
@@ -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
common/preamble +1 -1
@@ -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`
common/punch +6 -6
@@ -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 } # test the different corner cases for zeroing a range:
@@ -276,7 +276,7 @@ _test_generic_punch() u) unwritten_tests= ;; ?) echo Invalid flag - exit 1 + _exit 1 ;; esac done
@@ -552,7 +552,7 @@ _test_block_boundaries() d) sync_cmd= ;; ?) echo Invalid flag - exit 1 + _exit 1 ;; esac done
common/rc +50 -53
402 lines not on this page · raw message
common/xfs +4 -4
@@ -553,7 +553,7 @@ _require_xfs_db_command() { if [ $# -ne 1 ]; then echo "Usage: _require_xfs_db_command command" 1>&2 - exit 1 + _exit 1 fi command=$1
@@ -789,7 +789,7 @@ _check_xfs_filesystem() if [ $# -ne 3 ]; then echo "Usage: _check_xfs_filesystem device <logdev>|none <rtdev>|none" 1>&2 - exit 1 + _exit 1 fi extra_mount_options=""
@@ -1014,7 +1014,7 @@ _check_xfs_filesystem() if [ $ok -eq 0 ]; then status=1 if [ "$iam" != "check" ]; then - exit 1 + _exit 1 fi return 1 fi
@@ -1379,7 +1379,7 @@ _require_xfs_spaceman_command() { if [ -z "$1" ]; then echo "Usage: _require_xfs_spaceman_command command [switch]" 1>&2 - exit 1 + _exit 1 fi local command=$1 shift
Signature · 1 line
2.34.1
Quoted 17 lines · Nirjhar Roy (IBM)
This patch series removes some unnecessary sourcing of common/rc and decouples the call to
init_rc()from the sourcing of common/rc. This is proposed in [1] and [2]. It also removes direct usage of exit command with a_exitwrapper. The individual patches have the details.[v1] --> v[2]
1. Added R.B from Darrick in patch 1 of [v1]
2. Kept theinit_rccall that was deleted in the v1.
3. Introduced_exitwrapper around exit command. This will help us get correct exit codes ("$?") on failures.[1] https://lore.kernel.org/all/20250206155251.GA21787@frogsfrogsfrogs/
Quoted 7 lines
Nirjhar Roy (IBM) (5):
generic/749: Remove redundant sourcing of common/rc
check: Remove redundant_test_mountin check check,common{rc,preamble}: Decoupleinit_rc()call from sourcing
common/rc
common/config: Introduce_exitwrapper around exit command
common: exit -->_exit
Whole series looks fine to me. I've got similar patches in my current check-parallel stack, as well as changing common/config to match the "don't run setup code when sourcing the file" behaviour.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH v2 1/5] generic/749: Remove redundant sourcing of common/rc
In reply to Nirjhar Roy (IBM)Quoted 2 lines · Nirjhar Roy (IBM)
common/rc is already sourced before the test starts running in
_begin_fstest()preamble.
Indeed. The patch looks good to me.
Feel free to add:
Quoted 21 lines
Signed-off-by Nirjhar Roy (IBM) <[email protected]>Reviewed-by "Darrick J. Wong" <[email protected]>--- tests/generic/749 | 1 - 1 file changed, 1 deletion(-)tests/generic/749 +0
-1@@ -15,7 +15,6 @@ # boundary and ensures we get a SIGBUS if we write to data beyond the system # page size even if the block size is greater than the system page size. . ./common/preamble -. ./common/rc _begin_fstest auto quick prealloc # Import common functions.Signature · 1 line
2.34.1
Re: [PATCH v2 2/5] check: Remove redundant _test_mount in check
In reply to Nirjhar Roy (IBM)Quoted 13 lines · Nirjhar Roy (IBM)
init_rcalready does a_test_mount. Hence removing the additional_test_mountcall whenOLD_TEST_FS_MOUNT_OPTS!=TEST_FS_MOUNT_OPTS.Signed-off-by Nirjhar Roy (IBM) <[email protected]>--- check | 6 ------ 1 file changed, 6 deletions(-)check +0
-0
check function run_section() · line 794
Quoted 3 lines
check +0
-0@@ -792,12 +792,6 @@ function run_section() _prepare_test_list elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then _test_unmount 2> /dev/null
Would have been nicer if there was a small comment here like:
elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then # Unmount TEST_DEV to apply the updated mount options. # It will be mounted again by init_rc(), called shortly after. _test_unmount 2> /dev/null
fi
init_rc
But either ways, no strong preference for adding comments here.
Feel free to add -
Quoted 11 lines
check +0
-6- if ! _test_mount - then - echo "check: failed to mount $TEST_DEV on $TEST_DIR" - status=1 - exit - fi fi init_rcSignature · 1 line
2.34.1
Re: [PATCH v2 3/5] check,common{rc,preamble}: Decouple init_rc() call from sourcing common/rc
In reply to Nirjhar Roy (IBM)Quoted 3 lines · Nirjhar Roy (IBM)
Silently executing scripts during sourcing common/rc isn't good practice and also causes unnecessary script execution. Decouple
init_rc()call and callinit_rc()explicitly where required.
This patch looks good to me. Please feel free to add:
While reviewing this patch, I also noticed couple of related cleanups which you might be interested in:
1. common/rc sources common/config which executes a function _canonicalize_devices()
2. tests/generic/367 sources common/config which is not really required since _begin_fstests() will anyways source common/rc and common/config will get sourced automatically.
-ritesh
Quoted 48 lines
Signed-off-by Nirjhar Roy (IBM) <[email protected]>--- check | 2 ++ common/preamble | 1 + common/rc | 2 -- 3 files changed, 3 insertions(+), 2 deletions(-)check +2
-0@@ -364,6 +364,8 @@ if ! . ./common/rc; then exit 1 fi +init_rc + # If the test config specified a soak test duration, see if there are any # unit suffixes that need converting to an integer seconds count. if [ -n "$SOAK_DURATION" ]; thencommon/preamble +1
-0@@ -50,6 +50,7 @@ _begin_fstest() _register_cleanup _cleanup . ./common/rc + init_rc # remove previous $seqres.full before test rm -f $seqres.full $seqres.hintscommon/rc +0
-2@@ -5817,8 +5817,6 @@ _require_program() { _have_program "$1" || _notrun "$tag required" } -init_rc - ################################################################################ # make sure this script returns success /bin/trueSignature · 1 line
2.34.1
Re: [PATCH v2 3/5] check,common{rc,preamble}: Decouple init_rc() call from sourcing common/rc
Quoted 12 lines · Ritesh Harjani (IBM)
Quoted 3 lines · Nirjhar Roy (IBM)
Silently executing scripts during sourcing common/rc isn't good practice and also causes unnecessary script execution. Decouple
init_rc()call and callinit_rc()explicitly where required.This patch looks good to me. Please feel free to add:
Reviewed-by Ritesh Harjani (IBM) <[email protected]>While reviewing this patch, I also noticed couple of related cleanups which you might be interested in:
Quoted 6 lines
1. common/rc sources common/config which executes a function
_canonicalize_devices()2. tests/generic/367 sources common/config which is not really required since
_begin_fstests()will anyways source common/rc and common/config will get sourced automatically.
Thank you for the pointer. I can have follow-up patches for such cleanups.
--NR
Quoted 50 lines
-ritesh
Quoted 47 lines
Signed-off-by Nirjhar Roy (IBM) <[email protected]>--- check | 2 ++ common/preamble | 1 + common/rc | 2 -- 3 files changed, 3 insertions(+), 2 deletions(-)check +2
-0@@ -364,6 +364,8 @@ if ! . ./common/rc; then exit 1 fi +init_rc + # If the test config specified a soak test duration, see if there are any # unit suffixes that need converting to an integer seconds count. if [ -n "$SOAK_DURATION" ]; thencommon/preamble +1
-0@@ -50,6 +50,7 @@ _begin_fstest() _register_cleanup _cleanup . ./common/rc + init_rc # remove previous $seqres.full before test rm -f $seqres.full $seqres.hintscommon/rc +0
-2@@ -5817,8 +5817,6 @@ _require_program() { _have_program "$1" || _notrun "$tag required" } -init_rc - ################################################################################ # make sure this script returns success /bin/trueSignature · 1 line
2.34.1
Signature · 3 lines
Nirjhar Roy Linux Kernel Developer IBM, Bangalore
Re: [PATCH v2 4/5] common/config: Introduce _exit wrapper around exit command
In reply to Nirjhar Roy (IBM)Quoted 28 lines · Nirjhar Roy (IBM)
We should always set the value of status correctly when we are exiting. Else, "$?" might not give us the correct value.
If we see the following trap
handler registration in the check script:if $OPTIONS_HAVE_SECTIONS; then
trap "_kill_seq; _summary; exit \$status" 0 1 2 3 15 else trap "_kill_seq; _wrapup; exit \$status" 0 1 2 3 15fi
So, "exit 1" will exit the check script without setting the correct return value. I ran with the following local.config file:
[
xfs_4k_valid]
FSTYP=xfs
TEST_DEV=/dev/loop0
TEST_DIR=/mnt1/test
SCRATCH_DEV=/dev/loop1
SCRATCH_MNT=/mnt1/scratch[
xfs_4k_invalid]
FSTYP=xfs
TEST_DEV=/dev/loop0
TEST_DIR=/mnt1/invalid_dir
SCRATCH_DEV=/dev/loop1
SCRATCH_MNT=/mnt1/scratch
Quoted 6 lines
This caused the
init_rc()to catch the case of invalid_test_mountoptions. Although the check script correctly failed during the execution of the "xfs_4k_invalid" section, the return value was 0, i.e "echo $?" returned 0. This is becauseinit_rcexits with "exit 1" without correctly setting the value of "status". IMO, the correct behavior should have been that "$?" should have been non-zero.
Nice catch. Feel free to add:
Quoted 29 lines
The next patch will replace exit with
_exit.Signed-off-by Nirjhar Roy (IBM) <[email protected]>--- common/config | 8 ++++++++ 1 file changed, 8 insertions(+)common/config +8
-0@@ -96,6 +96,14 @@ 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. +_exit() +{ + status="$1" + exit "$status" +} + # Handle mkfs.$fstyp which does (or does not) require -f to overwrite set_mkfs_prog_path_with_opts() {Signature · 1 line
2.34.1
Re: [PATCH v2 5/5] common: exit --> _exit
In reply to Nirjhar Roy (IBM)Quoted 156 lines · Nirjhar Roy (IBM)
Replace exit <return-val> with
_exit<return-val> which is introduced in the previous patch.Signed-off-by Nirjhar Roy (IBM) <[email protected]>--- 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(-)common/btrfs +3
-3@@ -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 ficommon/ceph +1
-1@@ -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 \common/config +3
-4@@ -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_DEVcommon/ext4 +1
-1@@ -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 \common/populate +1
-1@@ -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 ]; thencommon/preamble +1
-1@@ -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`common/punch +3
-3@@ -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"; }' |
common/punch _filter_bmap() · line 228
Quoted 5 lines
common/punch +1
-1@@ -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?
Rest looks good to me.
-ritesh
Quoted 25 lines · Dave Chinner
Quoted 17 lines · Nirjhar Roy (IBM)
This patch series removes some unnecessary sourcing of common/rc and decouples the call to
init_rc()from the sourcing of common/rc. This is proposed in [1] and [2]. It also removes direct usage of exit command with a_exitwrapper. The individual patches have the details.[v1] --> v[2]
1. Added R.B from Darrick in patch 1 of [v1]
2. Kept theinit_rccall that was deleted in the v1.
3. Introduced_exitwrapper around exit command. This will help us get correct exit codes ("$?") on failures.[1] https://lore.kernel.org/all/20250206155251.GA21787@frogsfrogsfrogs/
Quoted 7 lines
Nirjhar Roy (IBM) (5):
generic/749: Remove redundant sourcing of common/rc
check: Remove redundant_test_mountin check check,common{rc,preamble}: Decoupleinit_rc()call from sourcing
common/rc
common/config: Introduce_exitwrapper around exit command
common: exit -->_exit
Quoted 5 lines
Whole series looks fine to me. I've got similar patches in my current check-parallel stack, as well as changing common/config to match the "don't run setup code when sourcing the file" behaviour.
Reviewed-by Dave Chinner <[email protected]>
Thank you for the review.
--NR
Quoted 1 line
Signature · 3 lines
Nirjhar Roy Linux Kernel Developer IBM, Bangalore
Re: [PATCH v2 5/5] common: exit --> _exit
In reply to Ritesh Harjani (IBM)Quoted 158 lines · Ritesh Harjani
Quoted 156 lines · Nirjhar Roy (IBM)
Replace exit <return-val> with
_exit<return-val> which is introduced in the previous patch.Signed-off-by Nirjhar Roy (IBM) <[email protected]>--- 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(-)common/btrfs +3
-3@@ -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 ficommon/ceph +1
-1@@ -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 \common/config +3
-4@@ -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_DEVcommon/ext4 +1
-1@@ -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 \common/populate +1
-1@@ -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 ]; thencommon/preamble +1
-1@@ -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`common/punch +3
-3@@ -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()
Quoted 8 lines
common/punch _filter_bmap() · line 228
Quoted 5 lines
common/punch +1
-1die_now() { status=1 - exit + _exitWhy not remove status=1 too and just do
_exit1 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.
Thanks,
Zorro
Quoted 5 lines
Rest looks good to me.
-ritesh
Re: [PATCH v2 5/5] common: exit --> _exit
Quoted 7 lines · Zorro Lang
Quoted 6 lines · Ritesh Harjani
Quoted 4 lines · Nirjhar Roy (IBM)
Replace exit <return-val> with
_exit<return-val> which is introduced in the previous patch.Signed-off-by Nirjhar Roy (IBM) <[email protected]>
<...>
Quoted 42 lines
Quoted 10 lines
common/punch _filter_bmap() · line 228
Quoted 7 lines
---
common/punch +1
-1@@ -225,7 +225,7 @@ _filter_bmap() die_now() { status=1 - exit + _exitWhy not remove status=1 too and just do
_exit1 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:
"
_exit1
"And "
_exit" looks not make sense, due to it gives null to status.Same problem likes below:
common/rc +1
-1@@ -3776,7 +3773,7 @@ _get_os_name() echo 'linux' else echo Unknown operating system: `uname` - exit + _exit
Quoted 2 lines
The "
_exit" without argument looks not make sense.
That's right. _exit called with no argument could make status as null. To prevent such misuse in future, should we add a warning/echo message if the no. of arguments passed to _exit() is not 1?
-ritesh
Re: [PATCH v2 5/5] common: exit --> _exit
In reply to Zorro LangQuoted 197 lines · Zorro Lang
Quoted 157 lines · Ritesh Harjani
Quoted 155 lines · Nirjhar Roy (IBM)
Replace exit <return-val> with
_exit<return-val> which is introduced in the previous patch.Signed-off-by Nirjhar Roy (IBM) <[email protected]>--- 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(-)common/btrfs +3
-3@@ -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 ficommon/ceph +1
-1@@ -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 \common/config +3
-4@@ -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_DEVcommon/ext4 +1
-1@@ -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 \common/populate +1
-1@@ -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 ]; thencommon/preamble +1
-1@@ -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`common/punch +3
-3@@ -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"; }' |Quoted 8 lines
common/punch _filter_bmap() · line 228
Quoted 6 lines
common/punch +1
-1@@ -225,7 +225,7 @@ _filter_bmap() die_now() { status=1 - exit + _exitWhy not remove status=1 too and just do
_exit1 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:
common/rc +1
-1@@ -3776,7 +3773,7 @@ _get_os_name() echo 'linux' else echo Unknown operating system: `uname` - exit + _exit
Quoted 1 line
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/[email protected]/
Quoted 8 lines
Thanks,
ZorroQuoted 4 lines
Rest looks good to me.
-ritesh
Signature · 3 lines
Nirjhar Roy Linux Kernel Developer IBM, Bangalore
Re: [PATCH v2 5/5] common: exit --> _exit
In reply to Ritesh Harjani (IBM)Quoted 1 line · Ritesh Harjani
Zorro Lang <[email protected]> writes:
<snip>
Quoted 31 lines
Quoted 31 lines
Yeah, nice catch! As the defination of
_exit:_exit() { status="$1" exit "$status" }The
"
status=1
exit
"
should be equal to:
"
_exit1
"And "
_exit" looks not make sense, due to it gives null to status.Same problem likes below:
common/rc +1
-1@@ -3776,7 +3773,7 @@ _get_os_name() echo 'linux' else echo Unknown operating system: `uname` - exit + _exit
Quoted 6 lines
Quoted 2 lines
The "
_exit" without argument looks not make sense.That's right.
_exitcalled with no argument could make status as null. To prevent such misuse in future, should we add a warning/echo message if the no. of arguments passed to_exit()is not 1?
Why not set status only if the caller provides an argument?
test -n "$1" && status="$1"
perhaps?
--D
Quoted 1 line
-ritesh
Re: [PATCH v2 5/5] common: exit --> _exit
In reply to Ritesh Harjani (IBM)Quoted 50 lines · Ritesh Harjani (IBM)
Quoted 7 lines · Zorro Lang
Quoted 6 lines · Ritesh Harjani
Quoted 4 lines · Nirjhar Roy (IBM)
Replace exit <return-val> with
_exit<return-val> which is introduced in the previous patch.Signed-off-by Nirjhar Roy (IBM) <[email protected]><...>
Quoted 40 lines
Quoted 9 lines
common/punch _filter_bmap() · line 228
Quoted 7 lines
---
common/punch +1
-1@@ -225,7 +225,7 @@ _filter_bmap() die_now() { status=1 - exit + _exitWhy not remove status=1 too and just do
_exit1 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:
common/rc +1
-1@@ -3776,7 +3773,7 @@ _get_os_name() echo 'linux' else echo Unknown operating system: `uname` - exit + _exit
Quoted 3 lines
Quoted 2 lines
The "
_exit" without argument looks not make sense.That's right.
_exitcalled with no argument could make status as null.
Yes, that is correct.
Quoted 1 line
To prevent such misuse in future, should we add a warning/echo message
Yeah, the other thing that we can do is 'status=${1:-0}'. In that case, for cases where the return value is a success, we simply use "_exit". Which one do you think adds more value and flexibility to the usage?
--NR
Quoted 3 lines
if the no. of arguments passed to
_exit()is not 1?-ritesh
Signature · 3 lines
Nirjhar Roy Linux Kernel Developer IBM, Bangalore
Re: [PATCH v2 5/5] common: exit --> _exit
In reply to Darrick J. WongQuoted 35 lines · Darrick J. Wong
Quoted 1 line · Ritesh Harjani
Zorro Lang <[email protected]> writes:
<snip>
Quoted 31 lines
Quoted 31 lines
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:
common/rc +1
-1@@ -3776,7 +3773,7 @@ _get_os_name() echo 'linux' else echo Unknown operating system: `uname` - exit + _exit
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
The "
_exit" without argument looks not make sense.That's right.
_exitcalled with no argument could make status as null. To prevent such misuse in future, should we add a warning/echo message if the no. of arguments passed to_exit()is not 1?Why not set status only if the caller provides an argument?
test -n "$1" && status="$1"
And if the caller doesn't provide any, should status hold some default value? I have suggested something in [1]
[1] https://lore.kernel.org/all/[email protected]/
--NR
Quoted 6 lines
perhaps?
--D
Quoted 1 line
-ritesh
Signature · 3 lines
Nirjhar Roy Linux Kernel Developer IBM, Bangalore
Re: [PATCH v2 2/5] check: Remove redundant _test_mount in check
In reply to Ritesh Harjani (IBM)Quoted 26 lines · Ritesh Harjani (IBM)
Quoted 13 lines · Nirjhar Roy (IBM)
init_rcalready does a_test_mount. Hence removing the additional_test_mountcall whenOLD_TEST_FS_MOUNT_OPTS!=TEST_FS_MOUNT_OPTS.Signed-off-by Nirjhar Roy (IBM) <[email protected]>--- check | 6 ------ 1 file changed, 6 deletions(-)check +0
-0check function run_section() · line 794
Quoted 3 lines
check +0
-0@@ -792,12 +792,6 @@ function run_section() _prepare_test_list elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then _test_unmount 2> /dev/nullWould have been nicer if there was a small comment here like:
elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then
# UnmountTEST_DEVto apply the updated mount options. # It will be mounted again byinit_rc(), called shortly after._test_unmount2>/dev/null
fi
Quoted 3 lines
init_rcBut either ways, no strong preference for adding comments here.
Added in [v3]
Thank you for pointing out.
--NR
Quoted 16 lines
Feel free to add -
Reviewed-by Ritesh Harjani (IBM) <[email protected]>Quoted 11 lines
check +0
-6- if ! _test_mount - then - echo "check: failed to mount $TEST_DEV on $TEST_DIR" - status=1 - exit - fi fi init_rcSignature · 1 line
2.34.1
Signature · 3 lines
Nirjhar Roy Linux Kernel Developer IBM, Bangalore
Re: [PATCH v2 3/5] check,common{rc,preamble}: Decouple init_rc() call from sourcing common/rc
In reply to Ritesh Harjani (IBM)Quoted 12 lines · Ritesh Harjani (IBM)
Quoted 3 lines · Nirjhar Roy (IBM)
Silently executing scripts during sourcing common/rc isn't good practice and also causes unnecessary script execution. Decouple
init_rc()call and callinit_rc()explicitly where required.This patch looks good to me. Please feel free to add:
Reviewed-by Ritesh Harjani (IBM) <[email protected]>While reviewing this patch, I also noticed couple of related cleanups which you might be interested in:
Quoted 6 lines
1. common/rc sources common/config which executes a function
_canonicalize_devices()2. tests/generic/367 sources common/config which is not really required since
_begin_fstests()will anyways source common/rc and common/config will get sourced automatically.
Addressed 2 in [v3].
Thanks.
--NR
Quoted 50 lines
-ritesh
Quoted 47 lines
Signed-off-by Nirjhar Roy (IBM) <[email protected]>--- check | 2 ++ common/preamble | 1 + common/rc | 2 -- 3 files changed, 3 insertions(+), 2 deletions(-)check +2
-0@@ -364,6 +364,8 @@ if ! . ./common/rc; then exit 1 fi +init_rc + # If the test config specified a soak test duration, see if there are any # unit suffixes that need converting to an integer seconds count. if [ -n "$SOAK_DURATION" ]; thencommon/preamble +1
-0@@ -50,6 +50,7 @@ _begin_fstest() _register_cleanup _cleanup . ./common/rc + init_rc # remove previous $seqres.full before test rm -f $seqres.full $seqres.hintscommon/rc +0
-2@@ -5817,8 +5817,6 @@ _require_program() { _have_program "$1" || _notrun "$tag required" } -init_rc - ################################################################################ # make sure this script returns success /bin/trueSignature · 1 line
2.34.1
Signature · 3 lines
Nirjhar Roy Linux Kernel Developer IBM, Bangalore
Re: [PATCH v2 2/5] check: Remove redundant _test_mount in check
In reply to Ritesh Harjani (IBM)Quoted 26 lines · Ritesh Harjani (IBM)
Quoted 13 lines · Nirjhar Roy (IBM)
init_rcalready does a_test_mount. Hence removing the additional_test_mountcall whenOLD_TEST_FS_MOUNT_OPTS!=TEST_FS_MOUNT_OPTS.Signed-off-by Nirjhar Roy (IBM) <[email protected]>--- check | 6 ------ 1 file changed, 6 deletions(-)check +0
-0check function run_section() · line 794
Quoted 3 lines
check +0
-0@@ -792,12 +792,6 @@ function run_section() _prepare_test_list elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then _test_unmount 2> /dev/nullWould have been nicer if there was a small comment here like:
elif [ "$OLD_TEST_FS_MOUNT_OPTS" != "$TEST_FS_MOUNT_OPTS" ]; then
# UnmountTEST_DEVto apply the updated mount options. # It will be mounted again byinit_rc(), called shortly after._test_unmount2>/dev/null
fi
Quoted 3 lines
init_rcBut either ways, no strong preference for adding comments here.
Addressed in https://lore.kernel.org/all/fa1bfd04d6b592f4d812a90039c643a02d7e1033.1744090313.git.nirjhar.roy.lists@gmail.com/ . Please ignore the previous link. I mistakenly gave the link to patch 2/6.
--NR
Quoted 16 lines
Feel free to add -
Reviewed-by Ritesh Harjani (IBM) <[email protected]>Quoted 11 lines
check +0
-6- if ! _test_mount - then - echo "check: failed to mount $TEST_DEV on $TEST_DIR" - status=1 - exit - fi fi init_rcSignature · 1 line
2.34.1
Signature · 3 lines
Nirjhar Roy Linux Kernel Developer IBM, Bangalore
Re: [PATCH v2 5/5] common: exit --> _exit
In reply to Nirjhar Roy (IBM)Quoted 52 lines · Nirjhar Roy (IBM)
Quoted 50 lines · Ritesh Harjani (IBM)
Quoted 7 lines · Zorro Lang
Quoted 6 lines · Ritesh Harjani
Quoted 4 lines · Nirjhar Roy (IBM)
Replace exit <return-val> with
_exit<return-val> which is introduced in the previous patch.Signed-off-by Nirjhar Roy (IBM) <[email protected]><...>
Quoted 40 lines
Quoted 9 lines
common/punch _filter_bmap() · line 228
Quoted 7 lines
---
common/punch +1
-1@@ -225,7 +225,7 @@ _filter_bmap() die_now() { status=1 - exit + _exitWhy not remove status=1 too and just do
_exit1 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:
common/rc +1
-1@@ -3776,7 +3773,7 @@ _get_os_name() echo 'linux' else echo Unknown operating system: `uname` - exit + _exit
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
The "
_exit" without argument looks not make sense.That's right.
_exitcalled with no argument could make status as null.Yes, that is correct.
Quoted 1 line
To prevent such misuse in future, should we add a warning/echo message
Yeah, the other thing that we can do is 'status=${1:-0}'. In that case, for
^^^^^^^^^^^^^^
That's good to me, I'm just wondering if the default value should be "1", to tell us "hey, there's an unknown exit status" :)
Thanks,
Zorro
Quoted 14 lines
cases where the return value is a success, we simply use "
_exit". Which one do you think adds more value and flexibility to the usage?--NR
Quoted 3 lines
if the no. of arguments passed to
_exit()is not 1?-ritesh
Signature · 3 lines
Nirjhar Roy Linux Kernel Developer IBM, Bangalore
Re: [PATCH v2 5/5] common: exit --> _exit
Quoted 56 lines · Zorro Lang
Quoted 55 lines · Nirjhar Roy (IBM)
Quoted 53 lines · Ritesh Harjani (IBM)
Quoted 7 lines · Zorro Lang
Quoted 6 lines · Ritesh Harjani
Quoted 4 lines · Nirjhar Roy (IBM)
Replace exit <return-val> with
_exit<return-val> which is introduced in the previous patch.Signed-off-by Nirjhar Roy (IBM) <[email protected]><...>
Quoted 40 lines
Quoted 9 lines
common/punch _filter_bmap() · line 228
Quoted 7 lines
---
common/punch +1
-1@@ -225,7 +225,7 @@ _filter_bmap() die_now() { status=1 - exit + _exitWhy not remove status=1 too and just do
_exit1 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:
common/rc +1
-1@@ -3776,7 +3773,7 @@ _get_os_name() echo 'linux' else echo Unknown operating system: `uname` - exit + _exitQuoted 2 lines
The "
_exit" without argument looks not make sense.That's right.
_exitcalled with no argument could make status as null.
Quoted 7 lines
Quoted 4 lines
Yes, that is correct.
Quoted 1 line
To prevent such misuse in future, should we add a warning/echo message
Yeah, the other thing that we can do is 'status=${1:-0}'. In that case, for
^^^^^^^^^^^^^^That's good to me, I'm just wondering if the default value should be "1", to tell us "hey, there's an unknown exit status" :)
I think status=1 usually means failure...
/usr/include/stdlib.h:92:#define EXIT_FAILURE 1 /* Failing exit status. */ /usr/include/stdlib.h:93:#define EXIT_SUCCESS 0 /* Successful exit status. */
--D
Quoted 19 lines
Thanks,
ZorroQuoted 14 lines
cases where the return value is a success, we simply use "
_exit". Which one do you think adds more value and flexibility to the usage?--NR
Quoted 3 lines
if the no. of arguments passed to
_exit()is not 1?-ritesh
Signature · 3 lines
Nirjhar Roy Linux Kernel Developer IBM, Bangalore
Re: [PATCH v2 5/5] common: exit --> _exit
Quoted 58 lines · Darrick J. Wong
Quoted 57 lines · Zorro Lang
Quoted 56 lines · Nirjhar Roy (IBM)
Quoted 50 lines · Ritesh Harjani (IBM)
Quoted 7 lines · Zorro Lang
Quoted 6 lines · Ritesh Harjani
Quoted 4 lines · Nirjhar Roy (IBM)
Replace exit <return-val> with
_exit<return-val> which is introduced in the previous patch.Signed-off-by Nirjhar Roy (IBM) <[email protected]><...>
Quoted 40 lines
Quoted 9 lines
Review _filter_bmap() · line 228
Quoted 7 lines
---
(quoted diff) +1
-1@@ -225,7 +225,7 @@ _filter_bmap() die_now() { status=1 - exit + _exitWhy not remove status=1 too and just do
_exit1 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:
(quoted diff) +1
-1@@ -3776,7 +3773,7 @@ _get_os_name() echo 'linux' else echo Unknown operating system: `uname` - exit + _exitQuoted 3 lines
Quoted 2 lines
The "
_exit" without argument looks not make sense.That's right.
_exitcalled with no argument could make status as null.Yes, that is correct.
Quoted 1 line
To prevent such misuse in future, should we add a warning/echo message
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Yeah, the other thing that we can do is 'status=${1:-0}'. In that case, for
^^^^^^^^^^^^^^That's good to me, I'm just wondering if the default value should be "1", to tell us "hey, there's an unknown exit status" :)
I think status=1 usually means failure...
/usr/include/stdlib.h:92:#define EXIT_FAILURE 1 /* Failing exit status. */ /usr/include/stdlib.h:93:#define EXIT_SUCCESS 0 /* Successful exit status. */
Yeah, right. I like Darrick's suggestion to explicitly set the value of status in _exit() only if it is passed (test -n "$1" && status="$1"). This will preserve any intentional pre-set value of status. I have sent a [v3] for this series but haven't modified the definition of _exit. I will wait for further comments on [v3] and make this change (along with the other changes that will be suggested).
[v3] https://lore.kernel.org/all/[email protected]/
--NR
Quoted 20 lines
--D
Quoted 17 lines
Thanks,
ZorroQuoted 13 lines
cases where the return value is a success, we simply use "
_exit". Which one do you think adds more value and flexibility to the usage?--NR
Quoted 3 lines
if the no. of arguments passed to
_exit()is not 1?-ritesh
Signature · 3 lines
Nirjhar Roy Linux Kernel Developer IBM, Bangalore
Signature · 3 lines
Nirjhar Roy Linux Kernel Developer IBM, Bangalore