Re: [PATCH v2 1/3] common/btrfs: refactor _require_btrfs_corrupt_block to check option

From: Zorro Lang <zlang@redhat.com>
To: Anand Jain <anand.jain@oracle.com>
Cc: fstests@vger.kernel.org, linux-btrfs@vger.kernel.org,
	fdmanana@kernel.org
Subject: Re: [PATCH v2 1/3] common/btrfs: refactor _require_btrfs_corrupt_block to check option
Date: Wed, 24 Apr 2024 22:37:54 +0800	[thread overview]
Message-ID: <20240424143754.ag7kc24yysudoigp@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com> (raw)
In-Reply-To: <206f5635cffc0c923afd1a297058fa406bd8e43a.1711097698.git.anand.jain@oracle.com>

On Fri, Mar 22, 2024 at 04:46:39PM +0530, Anand Jain wrote:
> The -v and -o short options in btrfs-corrupt-block were introduced and
> replaced with the long options --value and --offset in the same
> btrfs-progs release 5.19 by the following commits:
> 
>   b2ada0594116 ("btrfs-progs: corrupt-block: corrupt generic item data")
>   22ffee3c6cf2 ("btrfs-progs: corrupt-block: use only long options for value and offset")
> 
> We hope that if these commits are backported, they are both backported at
> the same time.
> 
> Use only the long options of btrfs-corrupt-block in the test cases. Also,
> check if btrfs-corrupt-block has the options --value and --offset.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  common/btrfs | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/common/btrfs b/common/btrfs
> index ae13fb55cbc6..a8340fdd4748 100644
> --- a/common/btrfs
> +++ b/common/btrfs
> @@ -659,7 +659,22 @@ _btrfs_buffered_read_on_mirror()
>  
>  _require_btrfs_corrupt_block()
>  {
> +	# An optional arg1 argument to also check the option.
> +	local opt=$1
> +	local ret
> +
>  	_require_command "$BTRFS_CORRUPT_BLOCK_PROG" btrfs-corrupt-block
> +
> +	if [ -z "$opt" ]; then
> +		return
> +	fi
> +
> +	$BTRFS_CORRUPT_BLOCK_PROG -h 2>&1 | grep -q -- " --$opt "

I'm wondering if we can use the "-w" option to replace the two
blanks around the --$opt (due to I don't know if there always be
two blanks), likes:

  grep -qw -- "--$opt"

?

> +	ret=$?
> +
> +	if [ $ret != 0 ]; then

The "ret" looks like useless at here. If there's not other place uses
this return value, we can use "$?" directly.

If no objection from you, I can help to change that when I merge. Others
looks good to me.

Reviewed-by: Zorro Lang <zlang@redhat.com>

> +		_notrun "Require $BTRFS_CORRUPT_BLOCK_PROG option --$opt"
> +	fi
>  }
>  
>  _require_btrfs_send_version()
> -- 
> 2.39.3
> 
> 