[PATCH v2 0/3] fix btrfs-corrupt-block options value and offset

[PATCH v2 0/3] fix btrfs-corrupt-block options value and offset

[Anand Jain]

The btrfs-corrupt-block options -v and -o have been replaced with
--value and --offset, so this patchset makes fixes accordingly.

v1:
  https://lore.kernel.org/all/eb2493499d2f30f43afa09e980589bb4f15e9789.1710984595.git.anand.jain@oracle.com/

Anand Jain (3):
  common/btrfs: refactor _require_btrfs_corrupt_block to check option
  btrfs/290: fix btrfs_corrupt_block options
  common/verity: fix btrfs-corrupt-block -v option

 common/btrfs    | 15 +++++++++++++++
 common/verity   |  5 +++--
 tests/btrfs/290 | 24 +++++++++++++++---------
 3 files changed, 33 insertions(+), 11 deletions(-)

-- 
2.39.3

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

[Anand Jain]

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 "
+	ret=$?
+
+	if [ $ret != 0 ]; then
+		_notrun "Require $BTRFS_CORRUPT_BLOCK_PROG option --$opt"
+	fi
 }
 
 _require_btrfs_send_version()
-- 
2.39.3

[PATCH v2 2/3] btrfs/290: fix btrfs_corrupt_block options

[Anand Jain]

Checks if the running btrfs-corrupt-block also has the options value and
offset.

Remove btrfs-corrupt-block command's STDOUT and STDERR output redirection
to /dev/null. Without this, debugging wasn't possible. I also noticed that
command is quiet when successfull, so no redirect to $seqres.full is required.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tests/btrfs/290 | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/tests/btrfs/290 b/tests/btrfs/290
index 61e741faeb45..281333b200f9 100755
--- a/tests/btrfs/290
+++ b/tests/btrfs/290
@@ -31,7 +31,8 @@ _require_odirect
 _require_xfs_io_command "falloc"
 _require_xfs_io_command "pread"
 _require_xfs_io_command "pwrite"
-_require_btrfs_corrupt_block
+_require_btrfs_corrupt_block "value"
+_require_btrfs_corrupt_block "offset"
 _disable_fsverity_signatures
 
 get_ino() {
@@ -58,7 +59,7 @@ corrupt_inline() {
 	_scratch_unmount
 	# inline data starts at disk_bytenr
 	# overwrite the first u64 with random bogus junk
-	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f disk_bytenr $SCRATCH_DEV > /dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f disk_bytenr $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
@@ -72,7 +73,7 @@ corrupt_prealloc_to_reg() {
 	_scratch_unmount
 	# ensure non-zero at the pre-allocated region on disk
 	# set extent type from prealloc (2) to reg (1)
-	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type -v 1 $SCRATCH_DEV >/dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type --value 1 $SCRATCH_DEV
 	_scratch_mount
 	# now that it's a regular file, reading actually looks at the previously
 	# preallocated region, so ensure that has non-zero contents.
@@ -88,7 +89,7 @@ corrupt_reg_to_prealloc() {
 	_fsv_enable $f
 	_scratch_unmount
 	# set type from reg (1) to prealloc (2)
-	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type -v 2 $SCRATCH_DEV >/dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type --value 2 $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
@@ -104,7 +105,8 @@ corrupt_punch_hole() {
 	_fsv_enable $f
 	_scratch_unmount
 	# change disk_bytenr to 0, representing a hole
-	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr -v 0 $SCRATCH_DEV > /dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr --value 0 \
+								    $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
@@ -118,7 +120,8 @@ corrupt_plug_hole() {
 	_fsv_enable $f
 	_scratch_unmount
 	# change disk_bytenr to some value, plugging the hole
-	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr -v 13639680 $SCRATCH_DEV > /dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr \
+						   --value 13639680 $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
@@ -132,7 +135,8 @@ corrupt_verity_descriptor() {
 	_scratch_unmount
 	# key for the descriptor item is <inode, BTRFS_VERITY_DESC_ITEM_KEY, 1>,
 	# 88 is X. So we write 5 Xs to the start of the descriptor
-	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 -v 88 -o 0 -b 5 $SCRATCH_DEV > /dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 --value 88 --offset 0 -b 5 \
+								    $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
@@ -144,7 +148,8 @@ corrupt_root_hash() {
 	local ino=$(get_ino $f)
 	_fsv_enable $f
 	_scratch_unmount
-	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 -v 88 -o 16 -b 1 $SCRATCH_DEV > /dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 --value 88 --offset 16 -b 1 \
+								    $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
@@ -159,7 +164,8 @@ corrupt_merkle_tree() {
 	# key for the descriptor item is <inode, BTRFS_VERITY_MERKLE_ITEM_KEY, 0>,
 	# 88 is X. So we write 5 Xs to somewhere in the middle of the first
 	# merkle item
-	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v 88 -o 100 -b 5 $SCRATCH_DEV > /dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 --value 88 --offset 100 \
+							       -b 5 $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
-- 
2.39.3

[PATCH v2 3/3] common/verity: fix btrfs-corrupt-block -v option

[Anand Jain]

The btrfs-corrupt-block -v has been replaced with --value so fix it.

_fsv_scratch_corrupt_merkle_tree() uses the btrfs-corrupt-block
--value option, so add the "value" prerequisite in the function
_require_fsverity_corruption.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 common/verity | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/verity b/common/verity
index 03d175ce1b7a..59b67e12010a 100644
--- a/common/verity
+++ b/common/verity
@@ -191,7 +191,7 @@ _require_fsverity_corruption()
 {
 	_require_xfs_io_command "fiemap"
 	if [ $FSTYP == "btrfs" ]; then
-		_require_btrfs_corrupt_block
+		_require_btrfs_corrupt_block "value"
 	fi
 }
 
@@ -402,7 +402,8 @@ _fsv_scratch_corrupt_merkle_tree()
 			# in the default filesystem tree (-r 5) and corrupt one byte (-b 1) at
 			# $offset (-o $offset) with the ascii representation of the byte we read
 			# (-v $ascii)
-			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v $ascii -o $offset -b 1 $SCRATCH_DEV
+			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 \
+			       --value $ascii --offset $offset -b 1 $SCRATCH_DEV
 			(( offset += 1 ))
 		done
 		_scratch_mount
-- 
2.39.3

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

[Zorro Lang]

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

Re: [PATCH v2 2/3] btrfs/290: fix btrfs_corrupt_block options

[Zorro Lang]

On Fri, Mar 22, 2024 at 04:46:40PM +0530, Anand Jain wrote:
> Checks if the running btrfs-corrupt-block also has the options value and
> offset.
> 
> Remove btrfs-corrupt-block command's STDOUT and STDERR output redirection
> to /dev/null. Without this, debugging wasn't possible. I also noticed that
> command is quiet when successfull, so no redirect to $seqres.full is required.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---

Good to me,

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

>  tests/btrfs/290 | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/btrfs/290 b/tests/btrfs/290
> index 61e741faeb45..281333b200f9 100755
> --- a/tests/btrfs/290
> +++ b/tests/btrfs/290
> @@ -31,7 +31,8 @@ _require_odirect
>  _require_xfs_io_command "falloc"
>  _require_xfs_io_command "pread"
>  _require_xfs_io_command "pwrite"
> -_require_btrfs_corrupt_block
> +_require_btrfs_corrupt_block "value"
> +_require_btrfs_corrupt_block "offset"
>  _disable_fsverity_signatures
>  
>  get_ino() {
> @@ -58,7 +59,7 @@ corrupt_inline() {
>  	_scratch_unmount
>  	# inline data starts at disk_bytenr
>  	# overwrite the first u64 with random bogus junk
> -	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f disk_bytenr $SCRATCH_DEV > /dev/null 2>&1
> +	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f disk_bytenr $SCRATCH_DEV
>  	_scratch_mount
>  	validate $f
>  }
> @@ -72,7 +73,7 @@ corrupt_prealloc_to_reg() {
>  	_scratch_unmount
>  	# ensure non-zero at the pre-allocated region on disk
>  	# set extent type from prealloc (2) to reg (1)
> -	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type -v 1 $SCRATCH_DEV >/dev/null 2>&1
> +	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type --value 1 $SCRATCH_DEV
>  	_scratch_mount
>  	# now that it's a regular file, reading actually looks at the previously
>  	# preallocated region, so ensure that has non-zero contents.
> @@ -88,7 +89,7 @@ corrupt_reg_to_prealloc() {
>  	_fsv_enable $f
>  	_scratch_unmount
>  	# set type from reg (1) to prealloc (2)
> -	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type -v 2 $SCRATCH_DEV >/dev/null 2>&1
> +	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type --value 2 $SCRATCH_DEV
>  	_scratch_mount
>  	validate $f
>  }
> @@ -104,7 +105,8 @@ corrupt_punch_hole() {
>  	_fsv_enable $f
>  	_scratch_unmount
>  	# change disk_bytenr to 0, representing a hole
> -	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr -v 0 $SCRATCH_DEV > /dev/null 2>&1
> +	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr --value 0 \
> +								    $SCRATCH_DEV
>  	_scratch_mount
>  	validate $f
>  }
> @@ -118,7 +120,8 @@ corrupt_plug_hole() {
>  	_fsv_enable $f
>  	_scratch_unmount
>  	# change disk_bytenr to some value, plugging the hole
> -	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr -v 13639680 $SCRATCH_DEV > /dev/null 2>&1
> +	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr \
> +						   --value 13639680 $SCRATCH_DEV
>  	_scratch_mount
>  	validate $f
>  }
> @@ -132,7 +135,8 @@ corrupt_verity_descriptor() {
>  	_scratch_unmount
>  	# key for the descriptor item is <inode, BTRFS_VERITY_DESC_ITEM_KEY, 1>,
>  	# 88 is X. So we write 5 Xs to the start of the descriptor
> -	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 -v 88 -o 0 -b 5 $SCRATCH_DEV > /dev/null 2>&1
> +	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 --value 88 --offset 0 -b 5 \
> +								    $SCRATCH_DEV
>  	_scratch_mount
>  	validate $f
>  }
> @@ -144,7 +148,8 @@ corrupt_root_hash() {
>  	local ino=$(get_ino $f)
>  	_fsv_enable $f
>  	_scratch_unmount
> -	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 -v 88 -o 16 -b 1 $SCRATCH_DEV > /dev/null 2>&1
> +	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 --value 88 --offset 16 -b 1 \
> +								    $SCRATCH_DEV
>  	_scratch_mount
>  	validate $f
>  }
> @@ -159,7 +164,8 @@ corrupt_merkle_tree() {
>  	# key for the descriptor item is <inode, BTRFS_VERITY_MERKLE_ITEM_KEY, 0>,
>  	# 88 is X. So we write 5 Xs to somewhere in the middle of the first
>  	# merkle item
> -	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v 88 -o 100 -b 5 $SCRATCH_DEV > /dev/null 2>&1
> +	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 --value 88 --offset 100 \
> +							       -b 5 $SCRATCH_DEV
>  	_scratch_mount
>  	validate $f
>  }
> -- 
> 2.39.3
> 
> 

Re: [PATCH v2 3/3] common/verity: fix btrfs-corrupt-block -v option

[Zorro Lang]

On Fri, Mar 22, 2024 at 04:46:41PM +0530, Anand Jain wrote:
> The btrfs-corrupt-block -v has been replaced with --value so fix it.
> 
> _fsv_scratch_corrupt_merkle_tree() uses the btrfs-corrupt-block
> --value option, so add the "value" prerequisite in the function
> _require_fsverity_corruption.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---

It makes sense to me, as an option is changed. But I think it'll
cause _notrun on old btrfs-corrupt-block command/version. If btrfs
list feels good for that (no objection), then that's fine.

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

>  common/verity | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/common/verity b/common/verity
> index 03d175ce1b7a..59b67e12010a 100644
> --- a/common/verity
> +++ b/common/verity
> @@ -191,7 +191,7 @@ _require_fsverity_corruption()
>  {
>  	_require_xfs_io_command "fiemap"
>  	if [ $FSTYP == "btrfs" ]; then
> -		_require_btrfs_corrupt_block
> +		_require_btrfs_corrupt_block "value"
>  	fi
>  }
>  
> @@ -402,7 +402,8 @@ _fsv_scratch_corrupt_merkle_tree()
>  			# in the default filesystem tree (-r 5) and corrupt one byte (-b 1) at
>  			# $offset (-o $offset) with the ascii representation of the byte we read
>  			# (-v $ascii)
> -			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v $ascii -o $offset -b 1 $SCRATCH_DEV
> +			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 \
> +			       --value $ascii --offset $offset -b 1 $SCRATCH_DEV
>  			(( offset += 1 ))
>  		done
>  		_scratch_mount
> -- 
> 2.39.3
> 
>