Message-ID: <46355124-c8a1-49ee-84fa-883774a11a03@gmail.com>
Date: Wed, 30 Apr 2025 11:43:35 +0530
From: "Nirjhar Roy (IBM)" <nirjhar.roy.lists@...il.com>
To: Dave Chinner <david@...morbit.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
Subject: Re: [PATCH v2 1/2] common: Move exit related functions to a
 common/exit


On 4/30/25 05:21, Dave Chinner wrote:
> On Tue, Apr 29, 2025 at 06:52:53AM +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>
>> ---
>>   common/config   | 17 +----------------
>>   common/exit     | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   common/preamble |  1 +
>>   common/punch    |  5 -----
>>   common/rc       | 28 ---------------------------
>>   5 files changed, 52 insertions(+), 49 deletions(-)
>>   create mode 100644 common/exit
> Neither of my replys to v1 made it to the list [1], so I'll have to
> repeat what I said here.

I am really sorry. I didn't get any of your feedback in [v1]. I will 
address them here.

[v1] 
https://lore.kernel.org/all/cover.1745390030.git.nirjhar.roy.lists@gmail.com/

>
> I did point out that I had already sent this out here:
>
> https://lore.kernel.org/fstests/20250417031208.1852171-4-david@fromorbit.com/
>
> but now this version is mostly the same (except for things noted
> below) so I'm good with this.
>
>> 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
> This isn't needed. Include it in check and other high level scripts
> (which need to include this, anyway) before including common/config.
Yeah, right.
>
>> 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
>> +}
> This should be removed and replaced with _fatal
Okay.
>
>> +die_now()
>> +{
>> +	_exit 1
>> +}
> And this should be removed as well.
>
> i.e. These two functions are only used by common/punch, so change
> them to use _fatal and _exit rather than duplicating the wrappers.
Okay.
>
>> diff --git a/common/preamble b/common/preamble
>> index ba029a34..9b6b4b26 100644
>> --- a/common/preamble
>> +++ b/common/preamble
>> @@ -33,6 +33,7 @@ _register_cleanup()
>>   # explicitly as a member of the 'all' group.
>>   _begin_fstest()
>>   {
>> +	. common/exit
>>   	if [ -n "$seq" ]; then
>>   		echo "_begin_fstest can only be called once!"
>>   		_exit 1
> Please leave a blank line between includes and unrelated code.

Sure.

--NR

>
> -Dave.
>
> [1] Thanks Google, for removing mail auth methods without any
> warning and not reporting permanent delivery failure on attempts
> to send mail an unsupported auth method.
>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Powered by blists - more mailing lists