From patchwork Tue Apr 7 07:18:44 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Qu Wenruo X-Patchwork-Id: 11477651 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 3E84714DD for ; Tue, 7 Apr 2020 07:19:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1B16C2074F for ; Tue, 7 Apr 2020 07:19:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727528AbgDGHS7 (ORCPT ); Tue, 7 Apr 2020 03:18:59 -0400 Received: from mx2.suse.de ([195.135.220.15]:47136 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726591AbgDGHS7 (ORCPT ); Tue, 7 Apr 2020 03:18:59 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 8C1EFAD39 for ; Tue, 7 Apr 2020 07:18:55 +0000 (UTC) From: Qu Wenruo To: linux-btrfs@vger.kernel.org Subject: [PATCH v3 1/2] btrfs-progs: tests: Filter output for run_check_stdout Date: Tue, 7 Apr 2020 15:18:44 +0800 Message-Id: <20200407071845.29428-2-wqu@suse.com> X-Mailer: git-send-email 2.26.0 In-Reply-To: <20200407071845.29428-1-wqu@suse.com> References: <20200407071845.29428-1-wqu@suse.com> MIME-Version: 1.0 Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org 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 --- 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" From patchwork Tue Apr 7 07:18:45 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Qu Wenruo X-Patchwork-Id: 11477653 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 35EEA14DD for ; Tue, 7 Apr 2020 07:19:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0EAB620747 for ; Tue, 7 Apr 2020 07:19:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726918AbgDGHTB (ORCPT ); Tue, 7 Apr 2020 03:19:01 -0400 Received: from mx2.suse.de ([195.135.220.15]:47228 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727192AbgDGHTA (ORCPT ); Tue, 7 Apr 2020 03:19:00 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 5CFCDAA7C for ; Tue, 7 Apr 2020 07:18:58 +0000 (UTC) From: Qu Wenruo To: linux-btrfs@vger.kernel.org Subject: [PATCH v3 2/2] btrfs-progs: tests: Introduce expand_command() to inject aruguments more accurately Date: Tue, 7 Apr 2020 15:18:45 +0800 Message-Id: <20200407071845.29428-3-wqu@suse.com> X-Mailer: git-send-email 2.26.0 In-Reply-To: <20200407071845.29428-1-wqu@suse.com> References: <20200407071845.29428-1-wqu@suse.com> MIME-Version: 1.0 Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org [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 --- 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"