[PATCH v3 0/2] btrfs-progs: tests: Enahance INSTRUMENT coverage

[PATCH v3 0/2] btrfs-progs: tests: Enahance INSTRUMENT coverage

[Qu Wenruo]

This patchset will enhance INSTRUMENT coverage for all btrfs related
commands.

Since now INSTRUMENT would also cover a lot of btrfs inspect-dump
comands, fix some test cases where INSTRUMENT output could easily
pollute them.


Now fsck/convert/misc/mkfs all pass with
INSTRUMENT="valgrind --leak-check=full".

Changelog:
v2:
- Pass INSTRUMENT as space split command

- Unset previous cmmd_array in expand_command()
  Instead of unsetting it.

- Use local variable @i in expand_command()
  To fix misc/004

- Add extra commands for INSTRUMENT coverage
  This incldues btrfstune, btrfs-corrupt-block, btrfs-image,
  btrfs-select-super, btrfs-find-root.

- Fix test cases which uses run_check_stdout without filtering

v3:
- Adding proper filtering for run_check_stdout() callers
  Instead of removing run_check_stdout(), to keep the coverage the same.

Qu Wenruo (2):
  btrfs-progs: tests: Filter output for run_check_stdout
  btrfs-progs: tests: Introduce expand_command() to inject aruguments
    more accurately

 tests/common                                  | 189 +++++++++---------
 tests/misc-tests/004-shrink-fs/test.sh        |  11 +-
 .../009-subvolume-sync-must-wait/test.sh      |   2 +-
 .../013-subvolume-sync-crash/test.sh          |   2 +-
 .../024-inspect-internal-rootid/test.sh       |  21 +-
 .../031-qgroup-parent-child-relation/test.sh  |   4 +-
 6 files changed, 120 insertions(+), 109 deletions(-)

-- 
2.26.0

[PATCH v3 1/2] btrfs-progs: tests: Filter output for run_check_stdout

[Qu Wenruo]

Since run_check_stdout() can insert INSTRUMENT for all btrfs related
programs, which could easily pollute the stdout, any caller of
run_check_stdout() should do proper filter.

The following callers are affected:
- misc/004
  Filter the output of "btrfs ins min-dev-size"

- misc/009
- misc/013
- misc/024
  They are all calling "btrfs ins rootid", so introduce get_subvolid()
  function to grab the subvolid properly.

- misc/031
  Loose the filter for "btrfs qgroup show". No need for "tail -n 1".

So we still have the same coverage, but now these tests won't cause
false alert if we insert INSTRUMENT for all btrfs commands.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/common                                  | 13 ++++++++++++
 tests/misc-tests/004-shrink-fs/test.sh        | 11 ++++++++--
 .../009-subvolume-sync-must-wait/test.sh      |  2 +-
 .../013-subvolume-sync-crash/test.sh          |  2 +-
 .../024-inspect-internal-rootid/test.sh       | 21 +++++++------------
 .../031-qgroup-parent-child-relation/test.sh  |  4 ++--
 6 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/tests/common b/tests/common
index 71661e950db0..f8fc3cfd8b4f 100644
--- a/tests/common
+++ b/tests/common
@@ -169,6 +169,9 @@ run_check()
 
 # same as run_check but the stderr+stdout output is duplicated on stdout and
 # can be processed further
+#
+# NOTE: If this function is called on btrfs related command, caller should
+#	filter the output, as INSTRUMENT can easily pollute the output.
 run_check_stdout()
 {
 	local spec
@@ -636,6 +639,16 @@ check_min_kernel_version()
 	return 0
 }
 
+# Get subvolume id for specified path
+get_subvolid()
+{
+	# run_check_stdout may have INSTRUMENT pollating the output,
+	# need to filter the output.
+	run_check_stdout "$TOP/btrfs" inspect-internal rootid "$1" | \
+		grep -e "^[[:digit:]]\+$"
+
+}
+
 # compare running kernel version to the given parameter, return success
 # if running is newer than requested (let caller decide if to fail or skip)
 # $1: minimum version of running kernel in major.minor format (eg. 4.19)
diff --git a/tests/misc-tests/004-shrink-fs/test.sh b/tests/misc-tests/004-shrink-fs/test.sh
index 741500634edb..7410f847a43e 100755
--- a/tests/misc-tests/004-shrink-fs/test.sh
+++ b/tests/misc-tests/004-shrink-fs/test.sh
@@ -11,11 +11,18 @@ check_prereq btrfs
 
 setup_root_helper
 
+get_min_dev_size()
+{
+	size=$(run_check_stdout $SUDO_HELPER "$TOP/btrfs" inspect-internal \
+		min-dev-size ${1:+--id "$1"} "$TEST_MNT" | \
+		grep -e "^[[:digit:]]\+.*)$" | cut -d ' ' -f 1)
+	echo "$size"
+}
+
 # Optionally take id of the device to shrink
 shrink_test()
 {
-	min_size=$(run_check_stdout $SUDO_HELPER "$TOP/btrfs" inspect-internal min-dev-size ${1:+--id "$1"}  "$TEST_MNT")
-	min_size=$(echo "$min_size" | cut -d ' ' -f 1)
+	min_size=$(get_min_dev_size $1)
 	_log "min size = ${min_size}"
 	if [ -z "$min_size" ]; then
 		_fail "Failed to parse minimum size"
diff --git a/tests/misc-tests/009-subvolume-sync-must-wait/test.sh b/tests/misc-tests/009-subvolume-sync-must-wait/test.sh
index 91dcebbbebf9..9f4b49a92080 100755
--- a/tests/misc-tests/009-subvolume-sync-must-wait/test.sh
+++ b/tests/misc-tests/009-subvolume-sync-must-wait/test.sh
@@ -31,7 +31,7 @@ done
 run_check $SUDO_HELPER "$TOP/btrfs" subvolume list .
 run_check $SUDO_HELPER "$TOP/btrfs" subvolume list -d .
 
-idtodel=`run_check_stdout $SUDO_HELPER "$TOP/btrfs" inspect-internal rootid snap3`
+idtodel=$(get_subvolid snap3)
 
 # delete, sync after some time
 run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete -c snap3
diff --git a/tests/misc-tests/013-subvolume-sync-crash/test.sh b/tests/misc-tests/013-subvolume-sync-crash/test.sh
index 64ba99598462..8d4e76032db5 100755
--- a/tests/misc-tests/013-subvolume-sync-crash/test.sh
+++ b/tests/misc-tests/013-subvolume-sync-crash/test.sh
@@ -32,7 +32,7 @@ done
 run_check $SUDO_HELPER "$TOP/btrfs" subvolume list .
 run_check $SUDO_HELPER "$TOP/btrfs" subvolume list -d .
 
-idtodel=`run_check_stdout $SUDO_HELPER "$TOP/btrfs" inspect-internal rootid snap3`
+idtodel=`$SUDO_HELPER "$TOP/btrfs" inspect-internal rootid snap3`
 
 # delete, sync after some time
 run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete -c snap*
diff --git a/tests/misc-tests/024-inspect-internal-rootid/test.sh b/tests/misc-tests/024-inspect-internal-rootid/test.sh
index b1c804d9aedd..cb3fbc6db02c 100755
--- a/tests/misc-tests/024-inspect-internal-rootid/test.sh
+++ b/tests/misc-tests/024-inspect-internal-rootid/test.sh
@@ -21,20 +21,13 @@ run_check touch file1
 run_check touch dir/file2
 run_check touch sub/file3
 
-id1=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid .) \
-	|| { echo $id1; exit 1; }
-id2=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid sub) \
-	|| { echo $id2; exit 1; }
-id3=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid sub/subsub) \
-	|| { echo $id3; exit 1; }
-id4=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid dir) \
-	|| { echo $id4; exit 1; }
-id5=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid file1) \
-	|| { echo $id5; exit 1; }
-id6=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid dir/file2) \
-	|| { echo $id6; exit 1; }
-id7=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid sub/file3) \
-	|| { echo $id7; exit 1; }
+id1=$(get_subvolid .) || { echo $id1; exit 1; }
+id2=$(get_subvolid sub) || { echo $id2; exit 1; }
+id3=$(get_subvolid sub/subsub) || { echo $id3; exit 1; }
+id4=$(get_subvolid dir) || { echo $id4; exit 1; }
+id5=$(get_subvolid file1) || { echo $id5; exit 1; }
+id6=$(get_subvolid dir/file2) || { echo $id6; exit 1; }
+id7=$(get_subvolid sub/file3) || { echo $id7; exit 1; }
 
 if ! ([ "$id1" -ne "$id2" ] && [ "$id1" -ne "$id3" ] && [ "$id2" -ne "$id3" ]); then
 	_fail "inspect-internal rootid: each subvolume must have different id"
diff --git a/tests/misc-tests/031-qgroup-parent-child-relation/test.sh b/tests/misc-tests/031-qgroup-parent-child-relation/test.sh
index f85290584742..53bc953e21d6 100755
--- a/tests/misc-tests/031-qgroup-parent-child-relation/test.sh
+++ b/tests/misc-tests/031-qgroup-parent-child-relation/test.sh
@@ -18,10 +18,10 @@ run_check $SUDO_HELPER "$TOP/btrfs" qgroup assign 0/5 1/0 "$TEST_MNT"
 run_check $SUDO_HELPER "$TOP/btrfs" quota rescan -w "$TEST_MNT"
 
 run_check_stdout $SUDO_HELPER "$TOP/btrfs" qgroup show --sort=-qgroupid \
-	-p "$TEST_MNT" | tail -n 1 | grep -q "1/0" \
+	-p "$TEST_MNT" | grep -q "1/0" \
 	|| _fail "parent qgroup check failed, please check the log"
 run_check_stdout $SUDO_HELPER "$TOP/btrfs" qgroup show --sort=qgroupid \
-	-c "$TEST_MNT" | tail -n 1 | grep -q "0/5" \
+	-c "$TEST_MNT" | grep -q "0/5" \
 	|| _fail "child qgroup check failed, please check the log"
 
 run_check_umount_test_dev "$TEST_MNT"
-- 
2.26.0

[PATCH v3 2/2] btrfs-progs: tests: Introduce expand_command() to inject aruguments more accurately

[Qu Wenruo]

[PROBLEM]
We want to inject $INSTRUMENT (mostly valgrind) before btrfs command but
after root_helper.

Currently we won't inject $INSTRUMENT at all if we are using
root_helper.
This means the coverage is not good enough.

[FIX]
This patch introduce a new function, expand_command(), to handle all
parameter/argument injection, including existing 'btrfs check' inject.

This function will:
- Detect where to inject $INSTRUMENT
  If we have root_helper and the command is target command
  (btrfs/mkfs.btrfs/btrfs-convert), then we inject $INSTRUMENT after
  root_helper.
  If we don't have root_helper, and the command is target command,
  we inject $INSTRUMENT before the command.
  Or we don't inject $INSTRUMENT (it's not the target command).

- Use existing spec facility to inject extra arguments

- Use an array to restore to result
  To avoid bash interpret the IFS inside path/commands.

Now we can make sure no matter if we use root_helper, $INSTRUMENT is
always injected corrected.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/common | 176 +++++++++++++++++++++++++--------------------------
 1 file changed, 87 insertions(+), 89 deletions(-)

diff --git a/tests/common b/tests/common
index f8fc3cfd8b4f..c2c1cb9d5993 100644
--- a/tests/common
+++ b/tests/common
@@ -3,6 +3,9 @@
 # Common routines for all tests
 #
 
+# Temporary command array for building real command
+declare -a cmd_array
+
 # assert that argument is not empty and is an existing path (file or directory)
 _assert_path()
 {
@@ -135,6 +138,60 @@ _cmd_spec()
 	fi
 }
 
+_is_target_command()
+{
+	[[ $1 =~ /btrfs$ ]] && return 0
+	[[ $1 =~ /mkfs.btrfs$ ]] && return 0
+	[[ $1 =~ /btrfs-convert$ ]] && return 0
+	[[ $1 =~ /btrfs-corrupt-block$ ]] && return 0
+	[[ $1 =~ /btrfs-image$ ]] && return 0
+	[[ $1 =~ /btrfs-select-super$ ]] && return 0
+	[[ $1 =~ /btrfs-find-root$ ]] && return 0
+	[[ $1 =~ /btrfstune$ ]] && return 0
+	return 1
+}
+
+# Expanding extra commands/options for current command string
+# This function is responsible for inserting the following things:
+# - @INSTRUMENT before btrfs commands
+#   To ensure that @INSTRUMENT is not executed for things like sudo/mount
+#   which normally has setuid/setgid which will not be traced.
+#
+# - Add extra arguments for 'btrfs check'/'mkfs.btrfs'/'btrfs-convert'
+#   This allows us to test certain function like lowmem mode for btrfs check
+expand_command()
+{
+	local command_index
+	local ins
+	local spec
+	local i
+
+	ins=$(_get_spec_ins "$@")
+	spec=$(($ins-1))
+	spec=$(_cmd_spec "${@:$spec}")
+	cmd_array=()
+
+	if [ "$1" = 'root_helper' ] && _is_target_command "$2" ; then
+		command_index=2
+	elif _is_target_command "$1"  ; then
+		command_index=1
+	else
+		# Since we iterate from offset 1, this never get hit,
+		# thus we won't insert $INSTRUMENT
+		command_index=0
+	fi
+
+	for ((i = 1; i <= $#; i++ )); do
+		if [ $i -eq $command_index -a ! -z "$INSTRUMENT" ]; then
+			cmd_array+=($INSTRUMENT)
+		fi
+		if [ $i -eq $ins -a ! -z "$spec" ]; then
+			cmd_array+=("$spec")
+		fi
+		cmd_array+=("${!i}")
+	done
+}
+
 # Argument passing magic:
 # the command passed to run_* helpers is inspected, if there's 'btrfs command'
 # found and there are defined additional arguments, they're inserted just after
@@ -145,26 +202,11 @@ _cmd_spec()
 
 run_check()
 {
-	local spec
-	local ins
+	expand_command "$@"
+	echo "====== RUN CHECK ${cmd_array[@]}" >> "$RESULTS" 2>&1
+	if [[ $TEST_LOG =~ tty ]]; then echo "CMD: ${cmd_array[@]}" > /dev/tty; fi
 
-	ins=$(_get_spec_ins "$@")
-	spec=$(($ins-1))
-	spec=$(_cmd_spec "${@:$spec}")
-	set -- "${@:1:$(($ins-1))}" $spec "${@: $ins}"
-	echo "====== RUN CHECK $@" >> "$RESULTS" 2>&1
-	if [[ $TEST_LOG =~ tty ]]; then echo "CMD: $@" > /dev/tty; fi
-
-	# If the command is `root_helper` or mount/umount, don't call INSTRUMENT
-	# as most profiling tool like valgrind can't handle setuid/setgid/setcap
-	# which mount normally has.
-	# And since mount/umount is only called with run_check(), we don't need
-	# to do the same check on other run_*() functions.
-	if [ "$1" = 'root_helper' -o "$1" = 'mount' -o "$1" = 'umount' ]; then
-		"$@" >> "$RESULTS" 2>&1 || _fail "failed: $@"
-	else
-		$INSTRUMENT "$@" >> "$RESULTS" 2>&1 || _fail "failed: $@"
-	fi
+	"${cmd_array[@]}" >> "$RESULTS" 2>&1 || _fail "failed: ${cmd_array[@]}"
 }
 
 # same as run_check but the stderr+stdout output is duplicated on stdout and
@@ -174,20 +216,11 @@ run_check()
 #	filter the output, as INSTRUMENT can easily pollute the output.
 run_check_stdout()
 {
-	local spec
-	local ins
-
-	ins=$(_get_spec_ins "$@")
-	spec=$(($ins-1))
-	spec=$(_cmd_spec "${@:$spec}")
-	set -- "${@:1:$(($ins-1))}" $spec "${@: $ins}"
-	echo "====== RUN CHECK $@" >> "$RESULTS" 2>&1
-	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(stdout): $@" > /dev/tty; fi
-	if [ "$1" = 'root_helper' ]; then
-		"$@" 2>&1 | tee -a "$RESULTS"
-	else
-		$INSTRUMENT "$@" 2>&1 | tee -a "$RESULTS"
-	fi
+	expand_command "$@"
+	echo "====== RUN CHECK ${cmd_array[@]}" >> "$RESULTS" 2>&1
+	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(stdout): ${cmd_array[@]}" \
+		> /dev/tty; fi
+	"${cmd_array[@]}" 2>&1 | tee -a "$RESULTS"
 	if [ ${PIPESTATUS[0]} -ne 0 ]; then
 		_fail "failed: $@"
 	fi
@@ -198,21 +231,11 @@ run_check_stdout()
 # output is logged
 run_mayfail()
 {
-	local spec
-	local ins
-	local ret
-
-	ins=$(_get_spec_ins "$@")
-	spec=$(($ins-1))
-	spec=$(_cmd_spec "${@:$spec}")
-	set -- "${@:1:$(($ins-1))}" $spec "${@: $ins}"
-	echo "====== RUN MAYFAIL $@" >> "$RESULTS" 2>&1
-	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mayfail): $@" > /dev/tty; fi
-	if [ "$1" = 'root_helper' ]; then
-		"$@" >> "$RESULTS" 2>&1
-	else
-		$INSTRUMENT "$@" >> "$RESULTS" 2>&1
-	fi
+	expand_command "$@"
+	echo "====== RUN MAYFAIL ${cmd_array[@]}" >> "$RESULTS" 2>&1
+	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mayfail): ${cmd_array[@]}" \
+		> /dev/tty; fi
+	"${cmd_array[@]}" >> "$RESULTS" 2>&1
 	ret=$?
 	if [ $ret != 0 ]; then
 		echo "failed (ignored, ret=$ret): $@" >> "$RESULTS"
@@ -234,23 +257,13 @@ run_mayfail()
 # store the output to a variable for further processing.
 run_mayfail_stdout()
 {
-	local spec
-	local ins
-	local ret
-
 	tmp_output=$(mktemp --tmpdir btrfs-progs-test--mayfail-stdtout.XXXXXX)
 
-	ins=$(_get_spec_ins "$@")
-	spec=$(($ins-1))
-	spec=$(_cmd_spec "${@:$spec}")
-	set -- "${@:1:$(($ins-1))}" $spec "${@: $ins}"
-	echo "====== RUN MAYFAIL $@" >> "$RESULTS" 2>&1
-	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mayfail): $@" > /dev/tty; fi
-	if [ "$1" = 'root_helper' ]; then
-		"$@" 2>&1 > "$tmp_output"
-	else
-		$INSTRUMENT "$@" 2>&1 > "$tmp_output"
-	fi
+	expand_command "$@"
+	echo "====== RUN MAYFAIL ${cmd_array[@]}" >> "$RESULTS" 2>&1
+	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mayfail): ${cmd_array[@]}" \
+		> /dev/tty; fi
+	"${cmd_array[@]}" 2>&1 > "$tmp_output"
 	ret=$?
 
 	cat "$tmp_output" >> "$RESULTS"
@@ -275,8 +288,6 @@ run_mayfail_stdout()
 # same as run_check but expects the command to fail, output is logged
 run_mustfail()
 {
-	local spec
-	local ins
 	local msg
 
 	msg="$1"
@@ -287,17 +298,12 @@ run_mustfail()
 		exit 1
 	fi
 
-	ins=$(_get_spec_ins "$@")
-	spec=$(($ins-1))
-	spec=$(_cmd_spec "${@:$spec}")
-	set -- "${@:1:$(($ins-1))}" $spec "${@: $ins}"
-	echo "====== RUN MUSTFAIL $@" >> "$RESULTS" 2>&1
-	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mustfail): $@" > /dev/tty; fi
-	if [ "$1" = 'root_helper' ]; then
-		"$@" >> "$RESULTS" 2>&1
-	else
-		$INSTRUMENT "$@" >> "$RESULTS" 2>&1
-	fi
+
+	expand_command "$@"
+	echo "====== RUN MUSTFAIL ${cmd_array[@]}" >> "$RESULTS" 2>&1
+	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mustfail): ${cmd_array[@]}" \
+		> /dev/tty; fi
+	"${cmd_array[@]}" >> "$RESULTS" 2>&1
 	if [ $? != 0 ]; then
 		echo "failed (expected): $@" >> "$RESULTS"
 		return 0
@@ -315,8 +321,6 @@ run_mustfail()
 # So it doesn't support pipeline in the @cmd
 run_mustfail_stdout()
 {
-	local spec
-	local ins
 	local msg
 	local ret
 	local tmp_output
@@ -331,17 +335,11 @@ run_mustfail_stdout()
 		exit 1
 	fi
 
-	ins=$(_get_spec_ins "$@")
-	spec=$(($ins-1))
-	spec=$(_cmd_spec "${@:$spec}")
-	set -- "${@:1:$(($ins-1))}" $spec "${@: $ins}"
-	echo "====== RUN MUSTFAIL $@" >> "$RESULTS" 2>&1
-	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mustfail): $@" > /dev/tty; fi
-	if [ "$1" = 'root_helper' ]; then
-		"$@" 2>&1 > "$tmp_output"
-	else
-		$INSTRUMENT "$@" 2>&1 > "$tmp_output"
-	fi
+	expand_command "$@"
+	echo "====== RUN MUSTFAIL ${cmd_array[@]}" >> "$RESULTS" 2>&1
+	if [[ $TEST_LOG =~ tty ]]; then echo "CMD(mustfail): ${cmd_array[@]}" \
+		> /dev/tty; fi
+	"${cmd_array[@]}" 2>&1 > "$tmp_output"
 	ret=$?
 
 	cat "$tmp_output" >> "$RESULTS"
-- 
2.26.0

Re: [PATCH v3 1/2] btrfs-progs: tests: Filter output for run_check_stdout

[David Sterba]

On Tue, Apr 07, 2020 at 03:18:44PM +0800, Qu Wenruo wrote:
> Since run_check_stdout() can insert INSTRUMENT for all btrfs related
> programs, which could easily pollute the stdout, any caller of
> run_check_stdout() should do proper filter.
> 
> The following callers are affected:
> - misc/004
>   Filter the output of "btrfs ins min-dev-size"
> 
> - misc/009
> - misc/013
> - misc/024
>   They are all calling "btrfs ins rootid", so introduce get_subvolid()
>   function to grab the subvolid properly.
> 
> - misc/031
>   Loose the filter for "btrfs qgroup show". No need for "tail -n 1".
> 
> So we still have the same coverage, but now these tests won't cause
> false alert if we insert INSTRUMENT for all btrfs commands.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  tests/common                                  | 13 ++++++++++++
>  tests/misc-tests/004-shrink-fs/test.sh        | 11 ++++++++--
>  .../009-subvolume-sync-must-wait/test.sh      |  2 +-
>  .../013-subvolume-sync-crash/test.sh          |  2 +-
>  .../024-inspect-internal-rootid/test.sh       | 21 +++++++------------
>  .../031-qgroup-parent-child-relation/test.sh  |  4 ++--
>  6 files changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/tests/common b/tests/common
> index 71661e950db0..f8fc3cfd8b4f 100644
> --- a/tests/common
> +++ b/tests/common
> @@ -169,6 +169,9 @@ run_check()
>  
>  # same as run_check but the stderr+stdout output is duplicated on stdout and
>  # can be processed further
> +#
> +# NOTE: If this function is called on btrfs related command, caller should
> +#	filter the output, as INSTRUMENT can easily pollute the output.
>  run_check_stdout()
>  {
>  	local spec
> @@ -636,6 +639,16 @@ check_min_kernel_version()
>  	return 0
>  }
>  
> +# Get subvolume id for specified path
> +get_subvolid()
> +{
> +	# run_check_stdout may have INSTRUMENT pollating the output,
> +	# need to filter the output.
> +	run_check_stdout "$TOP/btrfs" inspect-internal rootid "$1" | \
> +		grep -e "^[[:digit:]]\+$"

This does not seem much better, now it's specific to the commands and
calling the commands directly in new tests will make it fail.

If we find another command where the extra output must be filtered,
another helper. The instrument-tool specific filtering is IMHO fixed in
one place and future proof.

I want to avoid adding yet another test coding style exception like "for
inspect-internal you must use this helper", we have already enough like
new tests not using the mount/umount helpers or opencoding other
existing helpers.

My idea is to let people write tests in a natural way and adapt the
instrumentation tools as we know what problems they could cause.

Re: [PATCH v3 1/2] btrfs-progs: tests: Filter output for run_check_stdout

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3838 bytes --]



On 2020/4/9 下午11:52, David Sterba wrote:
> On Tue, Apr 07, 2020 at 03:18:44PM +0800, Qu Wenruo wrote:
>> Since run_check_stdout() can insert INSTRUMENT for all btrfs related
>> programs, which could easily pollute the stdout, any caller of
>> run_check_stdout() should do proper filter.
>>
>> The following callers are affected:
>> - misc/004
>>   Filter the output of "btrfs ins min-dev-size"
>>
>> - misc/009
>> - misc/013
>> - misc/024
>>   They are all calling "btrfs ins rootid", so introduce get_subvolid()
>>   function to grab the subvolid properly.
>>
>> - misc/031
>>   Loose the filter for "btrfs qgroup show". No need for "tail -n 1".
>>
>> So we still have the same coverage, but now these tests won't cause
>> false alert if we insert INSTRUMENT for all btrfs commands.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  tests/common                                  | 13 ++++++++++++
>>  tests/misc-tests/004-shrink-fs/test.sh        | 11 ++++++++--
>>  .../009-subvolume-sync-must-wait/test.sh      |  2 +-
>>  .../013-subvolume-sync-crash/test.sh          |  2 +-
>>  .../024-inspect-internal-rootid/test.sh       | 21 +++++++------------
>>  .../031-qgroup-parent-child-relation/test.sh  |  4 ++--
>>  6 files changed, 33 insertions(+), 20 deletions(-)
>>
>> diff --git a/tests/common b/tests/common
>> index 71661e950db0..f8fc3cfd8b4f 100644
>> --- a/tests/common
>> +++ b/tests/common
>> @@ -169,6 +169,9 @@ run_check()
>>  
>>  # same as run_check but the stderr+stdout output is duplicated on stdout and
>>  # can be processed further
>> +#
>> +# NOTE: If this function is called on btrfs related command, caller should
>> +#	filter the output, as INSTRUMENT can easily pollute the output.
>>  run_check_stdout()
>>  {
>>  	local spec
>> @@ -636,6 +639,16 @@ check_min_kernel_version()
>>  	return 0
>>  }
>>  
>> +# Get subvolume id for specified path
>> +get_subvolid()
>> +{
>> +	# run_check_stdout may have INSTRUMENT pollating the output,
>> +	# need to filter the output.
>> +	run_check_stdout "$TOP/btrfs" inspect-internal rootid "$1" | \
>> +		grep -e "^[[:digit:]]\+$"
> 
> This does not seem much better, now it's specific to the commands and
> calling the commands directly in new tests will make it fail.
> 
> If we find another command where the extra output must be filtered,
> another helper. The instrument-tool specific filtering is IMHO fixed in
> one place and future proof.

I get your point and concern, and kinda support your point.

> 
> I want to avoid adding yet another test coding style exception like "for
> inspect-internal you must use this helper", we have already enough like
> new tests not using the mount/umount helpers or opencoding other
> existing helpers.

However the problem only happens for command with one line output.
Like the min-dev-size and rootid of inspect group.

All other common tools will need filtering anyway, like qgroup show or
dump-tree.

So just several exception would still be acceptable.
> 
> My idea is to let people write tests in a natural way and adapt the
> instrumentation tools as we know what problems they could cause.
> 
I used to support valgrind specific options to solve the problem, until
knowing you're using a simple wrapper to test.

Yes, we're only using valgrind anyway, but I'm not so sure for other
guys, maybe one day some new tool developer would use their own fancy
tool to check btrfs-progs.

And if they find that we're using valgrind specific option just to make
all tests pass, and their tool fails due to that reason, they may just
exclude btrfs-progs and express their disappointment against btrfs.

Considering the exception is really not that many, the trade at least
looks good enough to me.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] btrfs-progs: tests: Filter output for run_check_stdout

[David Sterba]

On Fri, Apr 10, 2020 at 08:25:28AM +0800, Qu Wenruo wrote:
> 
> 
> On 2020/4/9 下午11:52, David Sterba wrote:
> > On Tue, Apr 07, 2020 at 03:18:44PM +0800, Qu Wenruo wrote:
> >> Since run_check_stdout() can insert INSTRUMENT for all btrfs related
> >> programs, which could easily pollute the stdout, any caller of
> >> run_check_stdout() should do proper filter.
> >>
> >> The following callers are affected:
> >> - misc/004
> >>   Filter the output of "btrfs ins min-dev-size"
> >>
> >> - misc/009
> >> - misc/013
> >> - misc/024
> >>   They are all calling "btrfs ins rootid", so introduce get_subvolid()
> >>   function to grab the subvolid properly.
> >>
> >> - misc/031
> >>   Loose the filter for "btrfs qgroup show". No need for "tail -n 1".
> >>
> >> So we still have the same coverage, but now these tests won't cause
> >> false alert if we insert INSTRUMENT for all btrfs commands.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >>  tests/common                                  | 13 ++++++++++++
> >>  tests/misc-tests/004-shrink-fs/test.sh        | 11 ++++++++--
> >>  .../009-subvolume-sync-must-wait/test.sh      |  2 +-
> >>  .../013-subvolume-sync-crash/test.sh          |  2 +-
> >>  .../024-inspect-internal-rootid/test.sh       | 21 +++++++------------
> >>  .../031-qgroup-parent-child-relation/test.sh  |  4 ++--
> >>  6 files changed, 33 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/tests/common b/tests/common
> >> index 71661e950db0..f8fc3cfd8b4f 100644
> >> --- a/tests/common
> >> +++ b/tests/common
> >> @@ -169,6 +169,9 @@ run_check()
> >>  
> >>  # same as run_check but the stderr+stdout output is duplicated on stdout and
> >>  # can be processed further
> >> +#
> >> +# NOTE: If this function is called on btrfs related command, caller should
> >> +#	filter the output, as INSTRUMENT can easily pollute the output.
> >>  run_check_stdout()
> >>  {
> >>  	local spec
> >> @@ -636,6 +639,16 @@ check_min_kernel_version()
> >>  	return 0
> >>  }
> >>  
> >> +# Get subvolume id for specified path
> >> +get_subvolid()
> >> +{
> >> +	# run_check_stdout may have INSTRUMENT pollating the output,
> >> +	# need to filter the output.
> >> +	run_check_stdout "$TOP/btrfs" inspect-internal rootid "$1" | \
> >> +		grep -e "^[[:digit:]]\+$"
> > 
> > This does not seem much better, now it's specific to the commands and
> > calling the commands directly in new tests will make it fail.
> > 
> > If we find another command where the extra output must be filtered,
> > another helper. The instrument-tool specific filtering is IMHO fixed in
> > one place and future proof.
> 
> I get your point and concern, and kinda support your point.
> 
> > 
> > I want to avoid adding yet another test coding style exception like "for
> > inspect-internal you must use this helper", we have already enough like
> > new tests not using the mount/umount helpers or opencoding other
> > existing helpers.
> 
> However the problem only happens for command with one line output.
> Like the min-dev-size and rootid of inspect group.
> 
> All other common tools will need filtering anyway, like qgroup show or
> dump-tree.
> 
> So just several exception would still be acceptable.
> > 
> > My idea is to let people write tests in a natural way and adapt the
> > instrumentation tools as we know what problems they could cause.
> > 
> I used to support valgrind specific options to solve the problem, until
> knowing you're using a simple wrapper to test.
> 
> Yes, we're only using valgrind anyway, but I'm not so sure for other
> guys, maybe one day some new tool developer would use their own fancy
> tool to check btrfs-progs.
> 
> And if they find that we're using valgrind specific option just to make
> all tests pass, and their tool fails due to that reason, they may just
> exclude btrfs-progs and express their disappointment against btrfs.
> 
> Considering the exception is really not that many, the trade at least
> looks good enough to me.

Ok, I understand, let's do it your way.