linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] btrfs-progs: tests: Enahance INSTRUMENT coverage
@ 2020-04-06  6:11 Qu Wenruo
  2020-04-06  6:11 ` [PATCH v2 1/2] btrfs-progs: tests: Don't use run_check_stdout without filtering Qu Wenruo
  2020-04-06  6:11 ` [PATCH v2 2/2] btrfs-progs: tests: Introduce expand_command() to inject aruguments more accurately Qu Wenruo
  0 siblings, 2 replies; 7+ messages in thread
From: Qu Wenruo @ 2020-04-06  6:11 UTC (permalink / raw)
  To: linux-btrfs

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

Qu Wenruo (2):
  btrfs-progs: tests: Don't use run_check_stdout without filtering
  btrfs-progs: tests: Introduce expand_command() to inject aruguments
    more accurately

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

-- 
2.26.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/2] btrfs-progs: tests: Don't use run_check_stdout without filtering
  2020-04-06  6:11 [PATCH v2 0/2] btrfs-progs: tests: Enahance INSTRUMENT coverage Qu Wenruo
@ 2020-04-06  6:11 ` Qu Wenruo
  2020-04-06 16:56   ` David Sterba
  2020-04-06  6:11 ` [PATCH v2 2/2] btrfs-progs: tests: Introduce expand_command() to inject aruguments more accurately Qu Wenruo
  1 sibling, 1 reply; 7+ messages in thread
From: Qu Wenruo @ 2020-04-06  6:11 UTC (permalink / raw)
  To: linux-btrfs

Since run_check_stdout() can insert INSTRUMENT, which could easily
pollute the stdout, any caller of run_check_stdout() should filter its
output before using it.

There are some callers which just grab the output without filtering it,
most of them are simple inspect-dump commands.

To avoid false alert with INSTRUMENT set, just don't utilize
run_check_stdout() for those call sites.
Since those call sites are pretty simple, shouldn't cause too many holes
in the coverage.

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

diff --git a/tests/common b/tests/common
index 71661e950db0..a040a1b803c4 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
diff --git a/tests/misc-tests/004-shrink-fs/test.sh b/tests/misc-tests/004-shrink-fs/test.sh
index 741500634edb..8ae6f8a50cb2 100755
--- a/tests/misc-tests/004-shrink-fs/test.sh
+++ b/tests/misc-tests/004-shrink-fs/test.sh
@@ -14,7 +14,7 @@ setup_root_helper
 # 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=$($SUDO_HELPER "$TOP/btrfs" inspect-internal min-dev-size ${1:+--id "$1"}  "$TEST_MNT")
 	min_size=$(echo "$min_size" | cut -d ' ' -f 1)
 	_log "min size = ${min_size}"
 	if [ -z "$min_size" ]; then
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..5c8eb03d91a2 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=`$SUDO_HELPER "$TOP/btrfs" inspect-internal rootid 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..be457773a899 100755
--- a/tests/misc-tests/024-inspect-internal-rootid/test.sh
+++ b/tests/misc-tests/024-inspect-internal-rootid/test.sh
@@ -21,19 +21,19 @@ run_check touch file1
 run_check touch dir/file2
 run_check touch sub/file3
 
-id1=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid .) \
+id1=$("$TOP/btrfs" inspect-internal rootid .) \
 	|| { echo $id1; exit 1; }
-id2=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid sub) \
+id2=$("$TOP/btrfs" inspect-internal rootid sub) \
 	|| { echo $id2; exit 1; }
-id3=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid sub/subsub) \
+id3=$("$TOP/btrfs" inspect-internal rootid sub/subsub) \
 	|| { echo $id3; exit 1; }
-id4=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid dir) \
+id4=$("$TOP/btrfs" inspect-internal rootid dir) \
 	|| { echo $id4; exit 1; }
-id5=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid file1) \
+id5=$("$TOP/btrfs" inspect-internal rootid file1) \
 	|| { echo $id5; exit 1; }
-id6=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid dir/file2) \
+id6=$("$TOP/btrfs" inspect-internal rootid dir/file2) \
 	|| { echo $id6; exit 1; }
-id7=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid sub/file3) \
+id7=$("$TOP/btrfs" inspect-internal rootid sub/file3) \
 	|| { echo $id7; exit 1; }
 
 if ! ([ "$id1" -ne "$id2" ] && [ "$id1" -ne "$id3" ] && [ "$id2" -ne "$id3" ]); then
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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/2] btrfs-progs: tests: Introduce expand_command() to inject aruguments more accurately
  2020-04-06  6:11 [PATCH v2 0/2] btrfs-progs: tests: Enahance INSTRUMENT coverage Qu Wenruo
  2020-04-06  6:11 ` [PATCH v2 1/2] btrfs-progs: tests: Don't use run_check_stdout without filtering Qu Wenruo
@ 2020-04-06  6:11 ` Qu Wenruo
  1 sibling, 0 replies; 7+ messages in thread
From: Qu Wenruo @ 2020-04-06  6:11 UTC (permalink / raw)
  To: linux-btrfs

[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 a040a1b803c4..ae8e1a231c4c 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] btrfs-progs: tests: Don't use run_check_stdout without filtering
  2020-04-06  6:11 ` [PATCH v2 1/2] btrfs-progs: tests: Don't use run_check_stdout without filtering Qu Wenruo
@ 2020-04-06 16:56   ` David Sterba
  2020-04-06 22:44     ` Qu Wenruo
  0 siblings, 1 reply; 7+ messages in thread
From: David Sterba @ 2020-04-06 16:56 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Mon, Apr 06, 2020 at 02:11:05PM +0800, Qu Wenruo wrote:
> Since run_check_stdout() can insert INSTRUMENT, which could easily
> pollute the stdout, any caller of run_check_stdout() should filter its
> output before using it.
> 
> There are some callers which just grab the output without filtering it,
> most of them are simple inspect-dump commands.
> 
> To avoid false alert with INSTRUMENT set, just don't utilize
> run_check_stdout() for those call sites.
> Since those call sites are pretty simple, shouldn't cause too many holes
> in the coverage.

I don't like this, it removes a lot of the coverage. The instrument
tools should provide a way to avoid stdout/stderr pollution, eg.
valgrind has --log-fd or --log-file. We can add a shortcut that will
provide some defaults so we can conveniently use 'INSTRUMENT=valgrind'.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] btrfs-progs: tests: Don't use run_check_stdout without filtering
  2020-04-06 16:56   ` David Sterba
@ 2020-04-06 22:44     ` Qu Wenruo
  2020-04-09 15:41       ` David Sterba
  0 siblings, 1 reply; 7+ messages in thread
From: Qu Wenruo @ 2020-04-06 22:44 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, linux-btrfs


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



On 2020/4/7 上午12:56, David Sterba wrote:
> On Mon, Apr 06, 2020 at 02:11:05PM +0800, Qu Wenruo wrote:
>> Since run_check_stdout() can insert INSTRUMENT, which could easily
>> pollute the stdout, any caller of run_check_stdout() should filter its
>> output before using it.
>>
>> There are some callers which just grab the output without filtering it,
>> most of them are simple inspect-dump commands.
>>
>> To avoid false alert with INSTRUMENT set, just don't utilize
>> run_check_stdout() for those call sites.
>> Since those call sites are pretty simple, shouldn't cause too many holes
>> in the coverage.
> 
> I don't like this, it removes a lot of the coverage. The instrument
> tools should provide a way to avoid stdout/stderr pollution, eg.
> valgrind has --log-fd or --log-file. We can add a shortcut that will
> provide some defaults so we can conveniently use 'INSTRUMENT=valgrind'.
> 
Then that's too INSTURMENT dependent.

What about adding filtering for the mentioned callers?

Thanks,
Qu


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] btrfs-progs: tests: Don't use run_check_stdout without filtering
  2020-04-06 22:44     ` Qu Wenruo
@ 2020-04-09 15:41       ` David Sterba
  2020-04-09 15:45         ` David Sterba
  0 siblings, 1 reply; 7+ messages in thread
From: David Sterba @ 2020-04-09 15:41 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: dsterba, Qu Wenruo, linux-btrfs

On Tue, Apr 07, 2020 at 06:44:37AM +0800, Qu Wenruo wrote:
> On 2020/4/7 上午12:56, David Sterba wrote:
> > On Mon, Apr 06, 2020 at 02:11:05PM +0800, Qu Wenruo wrote:
> >> Since run_check_stdout() can insert INSTRUMENT, which could easily
> >> pollute the stdout, any caller of run_check_stdout() should filter its
> >> output before using it.
> >>
> >> There are some callers which just grab the output without filtering it,
> >> most of them are simple inspect-dump commands.
> >>
> >> To avoid false alert with INSTRUMENT set, just don't utilize
> >> run_check_stdout() for those call sites.
> >> Since those call sites are pretty simple, shouldn't cause too many holes
> >> in the coverage.
> > 
> > I don't like this, it removes a lot of the coverage. The instrument
> > tools should provide a way to avoid stdout/stderr pollution, eg.
> > valgrind has --log-fd or --log-file. We can add a shortcut that will
> > provide some defaults so we can conveniently use 'INSTRUMENT=valgrind'.
> > 
> Then that's too INSTURMENT dependent.

Yes, but how many do we use besides valgrind. I have tried gdb with some
init script or batch mode to catch crashes, but I can't think of other
tools.

> What about adding filtering for the mentioned callers?

I'm not sure what exactly do you mean, can you send an example?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] btrfs-progs: tests: Don't use run_check_stdout without filtering
  2020-04-09 15:41       ` David Sterba
@ 2020-04-09 15:45         ` David Sterba
  0 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2020-04-09 15:45 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, Qu Wenruo, linux-btrfs

On Thu, Apr 09, 2020 at 05:41:35PM +0200, David Sterba wrote:
> On Tue, Apr 07, 2020 at 06:44:37AM +0800, Qu Wenruo wrote:
> > On 2020/4/7 上午12:56, David Sterba wrote:
> > What about adding filtering for the mentioned callers?
> 
> I'm not sure what exactly do you mean, can you send an example?

Never mind, it's in the v3 patches.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-04-09 15:45 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-06  6:11 [PATCH v2 0/2] btrfs-progs: tests: Enahance INSTRUMENT coverage Qu Wenruo
2020-04-06  6:11 ` [PATCH v2 1/2] btrfs-progs: tests: Don't use run_check_stdout without filtering Qu Wenruo
2020-04-06 16:56   ` David Sterba
2020-04-06 22:44     ` Qu Wenruo
2020-04-09 15:41       ` David Sterba
2020-04-09 15:45         ` David Sterba
2020-04-06  6:11 ` [PATCH v2 2/2] btrfs-progs: tests: Introduce expand_command() to inject aruguments more accurately Qu Wenruo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).