linux-ext4 - Re: [PATCH v2 1/2] common: Move exit related functions to a common/exit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aBFl-eKBBwG-QxCm@dread.disaster.area>
Date: Wed, 30 Apr 2025 09:51:21 +1000
From: Dave Chinner <david@...morbit.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
Subject: Re: [PATCH v2 1/2] common: Move exit related functions to a
 common/exit

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

> 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

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

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

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

-- 
Dave Chinner
david@...morbit.com