Skip to contentSkip to conversation

- Check interface and functionality

PATCH New [email protected] 2 participants 4 messages Aug 22, 2017 – Aug 24, 2017

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 - Check interface and functionality

From: Veronika Kabatova <[email protected]>

Add a new test to verify enabling and disabling traces with supported flags works and doesn't cause any issues.

Dynamic debug was added in kernel 2.6.30, the first version only supported "p" flag and used "-" as indicator for no flags set. In kernel 3.2, additional flags "flmt" were added. Finally, in kernel 3.4, the indicator for no flags was changed to "=_".

The test takes into account different kernel versions and interface changes listed above.

Signed-off-by Veronika Kabatova <[email protected]>
---
 runtest/tracing                                    |   1 +
 testcases/kernel/tracing/dynamic_debug/Makefile    |  27 ++++
 .../tracing/dynamic_debug/dynamic_debug01.sh       | 174 +++++++++++++++++++++
 3 files changed, 202 insertions(+)
 create mode 100644 testcases/kernel/tracing/dynamic_debug/Makefile
 create mode 100755 testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh
runtest/tracing +1 -0
index 9f8d5c86a..8285eeb43 100644
@@ -2,3 +2,4 @@
 ftrace_regression01	ftrace_regression01.sh
 ftrace_regression02	ftrace_regression02.sh
 ftrace-stress-test	ftrace_stress_test.sh 90
+dynamic_debug01		dynamic_debug01.sh
testcases/kernel/tracing/dynamic_debug/Makefile new file +27 -0
new file mode 100644
index 000000000..2a6e53a11
@@ -0,0 +1,27 @@
+#
+#    tracing/dynamic_debug test suite Makefile.
+#
+#    Copyright (C) 2017 Red Hat, Inc.
+#
+#    This program is free software; you can redistribute it and/or modify
+#    it under the terms of the GNU General Public License as published by
+#    the Free Software Foundation; either version 2 of the License, or
+#    (at your option) any later version.
+#
+#    This program is distributed in the hope that it will be useful,
+#    but WITHOUT ANY WARRANTY; without even the implied warranty of
+#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+#    General Public License for more details.
+#
+#    You should have received a copy of the GNU General Public License along
+#    with this program; if not, see <http://www.gnu.org/licenses/>.
+#
+
+top_srcdir		?= ../../..
+
+include $(top_srcdir)/include/mk/env_pre.mk
+
+INSTALL_TARGETS		:= *.sh
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
+
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +174 -0

174 lines not on this page · raw message

Signature · 1 line
2.13.3
#1 lore raw

PATCH 1/1 - Check interface and functionality

Hi!

Quoted 24 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +24 -0
+# Test description: Test functionality of dynamic debug feature by enabling
+#                   and disabling traces with available flags. Check that these
+#                   settings don't cause issues by searching dmesg.
+#
+#                   This test handles changes of dynamic debug interface from
+#                   commits 5ca7d2a6 (dynamic_debug: describe_flags with
+#                   '=[pmflt_]*') and 8ba6ebf5 (Dynamic debug: Add more flags)
+#
+# Usage
+# ./dynamic_debug01.sh
+
+TST_TESTFUNC=ddebug_test
+TST_NEEDS_CMDS="awk"
+TST_NEEDS_TMPDIR=1
+TST_NEEDS_ROOT=1
+TST_SETUP=setup
+TST_CLEANUP=cleanup
+
+. tst_test.sh
+
+
+DEBUGFS_WAS_MOUNTED=0
+DEBUGFS_PATH=""
+DEBUGFS_CONTROL=""

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 52

Quoted 10 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +10 -0
+DYNDEBUG_STATEMENTS="./debug_statements"
+EMPTY_FLAG=""
+NEW_INTERFACE=0
+
+
+mount_debugfs()
+{
+	if grep -q debugfs /proc/mounts ; then
+		DEBUGFS_WAS_MOUNTED=1
+		DEBUGFS_PATH=$(grep debugfs /proc/mounts | awk '{print $2}')

                                    ^
				  $(awk /debugfs/ '{print $2}' /proc/mounts)

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 58

Quoted 6 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +6 -0
+		tst_res TINFO "debugfs already mounted at $DEBUGFS_PATH"
+	else
+		if ! grep -q debugfs /proc/filesystems ; then
+			tst_res TCONF "debugfs not supported"
+		fi
+		DEBUGFS_PATH="$(pwd)/tst_debug"

                                  ^
				$PWD/tst_debug

Or we may as well use relative path i.e. just ./tst_debugfs/

Quoted 8 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +8 -0
+		mkdir "$DEBUGFS_PATH"
+		if mount -t debugfs xxx "$DEBUGFS_PATH" ; then
+			tst_res TINFO "debugfs mounted at $DEBUGFS_PATH"
+		else
+			tst_res TFAIL "Unable to mount debugfs"
+		fi
+	fi
+}

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 76

Quoted 10 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +10 -0
+
+setup()
+{
+	if tst_kvcmp -lt 2.6.30 ; then
+		tst_brk TCONF "Dynamic debug is available since version 2.6.30"
+	fi
+
+	mount_debugfs
+	if [ ! -d "$DEBUGFS_PATH/dynamic_debug" ] ; then
+		tst_res TBROK "Unable to find $DEBUGFS_PATH/dynamic_debug"

                ^
		tst_brk?

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 80

Quoted 4 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +4 -0
+	fi
+	DEBUGFS_CONTROL="$DEBUGFS_PATH/dynamic_debug/control"
+	if [ ! -e "$DEBUGFS_CONTROL" ] ; then
+		tst_res TBROK "Unable to find $DEBUGFS_CONTROL"

^
here as well?

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 88

Quoted 8 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +8 -0
+	fi
+
+	# Both patches with changes were backported to RHEL6 kernel 2.6.32-547
+	if tst_kvcmp -ge '3.4 RHEL6:2.6.32-547' ; then
+		NEW_INTERFACE=1
+	fi
+
+	EMPTY_FLAG=$([ $NEW_INTERFACE -eq 1 ] && echo "=_" || echo "-")

Now, why don't we set empty flag to the old value on the top and set it to the new value in the if that sets the NEW_INTERFACE variable.

We could even use the EMPTY_FLAG content in the conditions below, but I guess that having NEW_INTERFACE variable is more readable.

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 96

Quoted 8 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +8 -0
+	grep -v "^#" "$DEBUGFS_CONTROL" > "$DYNDEBUG_STATEMENTS"
+}
+
+do_flag()
+{
+	FLAG="$1"
+	OPTION_TO_SET="$2"
+	OPTION_VALUE="$3"

It would be a sligtly better to declare these variables as local.

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 102

Quoted 5 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +5 -0
+	if ! echo "$OPTION_TO_SET $OPTION_VALUE $FLAG" > \
+		"$DEBUGFS_CONTROL" ; then
+		tst_res TFAIL "Setting '$OPTION_TO_SET $OPTION_VALUE " \
+			"$FLAG' failed with $?!"
+	fi

We do have EXPECT_PASS and EXPECT_FAIL functions in the test library:

https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#expect_pass-and-expect_fail

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 110

Quoted 8 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +8 -0
+}
+
+do_all_flags()
+{
+	OPTION="$1"
+	ALL_INPUTS="$2"
+
+	echo "$ALL_INPUTS" | while read -r INPUT_LINE ; do

You can do just for input_line in $ALL_INPUTS; do here instead, the newlines were converted to spaces once the output from the awk/sort/uniq pipeline was assigned into a variable.

Quoted 52 lines
(quoted diff) +52 -0
+		do_flag "+p" "$OPTION" "$INPUT_LINE"
+		if tst_kvcmp -ge 3.2 || [ $NEW_INTERFACE -eq 1 ] ; then
+			do_flag "+flmt" "$OPTION" "$INPUT_LINE"
+			do_flag "-flmt" "$OPTION" "$INPUT_LINE"
+		fi
+		do_flag "-p" "$OPTION" "$INPUT_LINE"
+	done
+
+	if awk -v emp="$EMPTY_FLAG" '$3 != emp' "$DEBUGFS_CONTROL" \
+		| grep -v -q "^#" ; then
+		tst_res TFAIL "Failed to remove all set flags"
+	fi
+
+}
+
+ddebug_test()
+{
+	dmesg > ./dmesg.old
+
+	DD_FUNCS=$(awk -F " |]" '{print $3}' "$DYNDEBUG_STATEMENTS" \
+		| sort | uniq)
+	DD_FILES=$(awk -F " |:" '{print $1}' "$DYNDEBUG_STATEMENTS" \
+		| sort | uniq)
+	DD_LINES=$(awk -F " |:" '{print $2}' "$DYNDEBUG_STATEMENTS" \
+		| sort | uniq)
+	DD_MODULES=$(awk -F [][] '{print $2}' "$DYNDEBUG_STATEMENTS" \
+		| sort | uniq)
+
+	do_all_flags "func" "$DD_FUNCS"
+	do_all_flags "file" "$DD_FILES"
+	do_all_flags "line" "$DD_LINES"
+	do_all_flags "module" "$DD_MODULES"
+
+	dmesg > ./dmesg.new
+	sed -i -e 1,`wc -l < ./dmesg.old`d ./dmesg.new
+	if grep -q -e "Kernel panic" -e "Oops" -e "general protection fault" \
+		-e "general protection handler: wrong gs" -e "\(XEN\) Panic" \
+		-e "fault" -e "warn" -e "BUG" ./dmesg.new ; then
+		tst_res TFAIL "Issues found in dmesg!"
+	else
+		tst_res TPASS "Dynamic debug OK"
+	fi
+}
+
+cleanup()
+{
+	if [ $DEBUGFS_WAS_MOUNTED -eq 0 ] ; then
+		tst_umount "$DEBUGFS_PATH"
+	else
+		FLAGS_SET=$(awk -v emp="$EMPTY_FLAG" '$3 != emp' \
+			$DYNDEBUG_STATEMENTS)
+		if [ "$FLAGS_SET" ] ; then

Review

Quoted 10 lines
(quoted diff) +10 -0
+			FLAG_PREFIX=$([ $NEW_INTERFACE -eq 1 ] \
+				&& echo "" || echo "+")
+			echo "$FLAGS_SET" | while read -r FLAG_LINE ; do
+				echo -n "$FLAG_LINE" \
+					| awk -v prf="$FLAG_PREFIX" -F " |:" \
+					'{print "file "$1" line "$2" "prf $4}' \
+					> "$DEBUGFS_CONTROL"
+			done
+		fi
+	fi

I wonder if we should attempt to restore the values even if debugfs wasn't mounted and then umount it if it wasn't mounted once we are done.

On my machine I have 45 values set to =p by default, although debugfs is mounted there...

Quoted 3 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +3 -0
+}
+
+tst_run
Signature · 2 lines
Cyril Hrubis
[email protected]
author
#2 lore raw

PATCH 1/1 - Check interface and functionality

----- Original Message -----

Quoted 7 lines

From: "Cyril Hrubis" <[email protected]>
To: [email protected]
Cc: [email protected]
Sent: Tuesday, August 22, 2017 3:09:53 PM
Subject: Re: [LTP] [PATCH] [dynamic debug] - Check interface and functionality

Hi!

Hi, thanks for the feedback! Most of it already applied.

Quoted 51 lines
Quoted 28 lines

+# Test description: Test functionality of dynamic debug feature by enabling
+# and disabling traces with available flags. Check that
these

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +3 -0
+#                   settings don't cause issues by searching dmesg.
+#
+#                   This test handles changes of dynamic debug interface

from

(quoted diff) +2 -0
+#                   commits 5ca7d2a6 (dynamic_debug: describe_flags with
+#                   '=[pmflt_]*') and 8ba6ebf5 (Dynamic debug: Add more

flags)

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +17 -0
+#
+# Usage
+# ./dynamic_debug01.sh
+
+TST_TESTFUNC=ddebug_test
+TST_NEEDS_CMDS="awk"
+TST_NEEDS_TMPDIR=1
+TST_NEEDS_ROOT=1
+TST_SETUP=setup
+TST_CLEANUP=cleanup
+
+. tst_test.sh
+
+
+DEBUGFS_WAS_MOUNTED=0
+DEBUGFS_PATH=""
+DEBUGFS_CONTROL=""

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 52

Quoted 10 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +10 -0
+DYNDEBUG_STATEMENTS="./debug_statements"
+EMPTY_FLAG=""
+NEW_INTERFACE=0
+
+
+mount_debugfs()
+{
+	if grep -q debugfs /proc/mounts ; then
+		DEBUGFS_WAS_MOUNTED=1
+		DEBUGFS_PATH=$(grep debugfs /proc/mounts | awk '{print $2}')

                                    ^
				  $(awk /debugfs/ '{print $2}' /proc/mounts)

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 58

Quoted 6 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +6 -0
+		tst_res TINFO "debugfs already mounted at $DEBUGFS_PATH"
+	else
+		if ! grep -q debugfs /proc/filesystems ; then
+			tst_res TCONF "debugfs not supported"
+		fi
+		DEBUGFS_PATH="$(pwd)/tst_debug"

                                  ^
				$PWD/tst_debug
Quoted 2 lines

Or we may as well use relative path i.e. just ./tst_debugfs/

+1 for relative path.

Quoted 60 lines
Quoted 8 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +8 -0
+		mkdir "$DEBUGFS_PATH"
+		if mount -t debugfs xxx "$DEBUGFS_PATH" ; then
+			tst_res TINFO "debugfs mounted at $DEBUGFS_PATH"
+		else
+			tst_res TFAIL "Unable to mount debugfs"
+		fi
+	fi
+}

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 76

Quoted 10 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +10 -0
+
+setup()
+{
+	if tst_kvcmp -lt 2.6.30 ; then
+		tst_brk TCONF "Dynamic debug is available since version 2.6.30"
+	fi
+
+	mount_debugfs
+	if [ ! -d "$DEBUGFS_PATH/dynamic_debug" ] ; then
+		tst_res TBROK "Unable to find $DEBUGFS_PATH/dynamic_debug"

                ^
		tst_brk?

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 80

Quoted 4 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +4 -0
+	fi
+	DEBUGFS_CONTROL="$DEBUGFS_PATH/dynamic_debug/control"
+	if [ ! -e "$DEBUGFS_CONTROL" ] ; then
+		tst_res TBROK "Unable to find $DEBUGFS_CONTROL"

^
here as well?

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 88

Quoted 8 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +8 -0
+	fi
+
+	# Both patches with changes were backported to RHEL6 kernel 2.6.32-547
+	if tst_kvcmp -ge '3.4 RHEL6:2.6.32-547' ; then
+		NEW_INTERFACE=1
+	fi
+
+	EMPTY_FLAG=$([ $NEW_INTERFACE -eq 1 ] && echo "=_" || echo "-")

Now, why don't we set empty flag to the old value on the top and set it to the new value in the if that sets the NEW_INTERFACE variable.

We could even use the EMPTY_FLAG content in the conditions below, but I guess that having NEW_INTERFACE variable is more readable.

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 96

Quoted 8 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +8 -0
+	grep -v "^#" "$DEBUGFS_CONTROL" > "$DYNDEBUG_STATEMENTS"
+}
+
+do_flag()
+{
+	FLAG="$1"
+	OPTION_TO_SET="$2"
+	OPTION_VALUE="$3"

It would be a sligtly better to declare these variables as local.

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 102

Quoted 5 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +5 -0
+	if ! echo "$OPTION_TO_SET $OPTION_VALUE $FLAG" > \
+		"$DEBUGFS_CONTROL" ; then
+		tst_res TFAIL "Setting '$OPTION_TO_SET $OPTION_VALUE " \
+			"$FLAG' failed with $?!"
+	fi

We do have EXPECT_PASS and EXPECT_FAIL functions in the test library:

Quoted 2 lines

https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#expect_pass-and-expect_fail

I'm not convinced using EXPECT_* would be the same since I only want to report failures and these functions report both TPASS and TFAIL. On my machine, this setup produces additional 16k of not that useful lines (if something fails, you are still notified about it and know every line before it passed). Of course if you can think of a use case where having reports for every passed line would make sense I can do this change.

Quoted 70 lines

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 110

Quoted 8 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +8 -0
+}
+
+do_all_flags()
+{
+	OPTION="$1"
+	ALL_INPUTS="$2"
+
+	echo "$ALL_INPUTS" | while read -r INPUT_LINE ; do

You can do just for input_line in $ALL_INPUTS; do here instead, the newlines were converted to spaces once the output from the awk/sort/uniq pipeline was assigned into a variable.

Quoted 57 lines
(quoted diff) +57 -0
+		do_flag "+p" "$OPTION" "$INPUT_LINE"
+		if tst_kvcmp -ge 3.2 || [ $NEW_INTERFACE -eq 1 ] ; then
+			do_flag "+flmt" "$OPTION" "$INPUT_LINE"
+			do_flag "-flmt" "$OPTION" "$INPUT_LINE"
+		fi
+		do_flag "-p" "$OPTION" "$INPUT_LINE"
+	done
+
+	if awk -v emp="$EMPTY_FLAG" '$3 != emp' "$DEBUGFS_CONTROL" \
+		| grep -v -q "^#" ; then
+		tst_res TFAIL "Failed to remove all set flags"
+	fi
+
+}
+
+ddebug_test()
+{
+	dmesg > ./dmesg.old
+
+	DD_FUNCS=$(awk -F " |]" '{print $3}' "$DYNDEBUG_STATEMENTS" \
+		| sort | uniq)
+	DD_FILES=$(awk -F " |:" '{print $1}' "$DYNDEBUG_STATEMENTS" \
+		| sort | uniq)
+	DD_LINES=$(awk -F " |:" '{print $2}' "$DYNDEBUG_STATEMENTS" \
+		| sort | uniq)
+	DD_MODULES=$(awk -F [][] '{print $2}' "$DYNDEBUG_STATEMENTS" \
+		| sort | uniq)
+
+	do_all_flags "func" "$DD_FUNCS"
+	do_all_flags "file" "$DD_FILES"
+	do_all_flags "line" "$DD_LINES"
+	do_all_flags "module" "$DD_MODULES"
+
+	dmesg > ./dmesg.new
+	sed -i -e 1,`wc -l < ./dmesg.old`d ./dmesg.new
+	if grep -q -e "Kernel panic" -e "Oops" -e "general protection fault" \
+		-e "general protection handler: wrong gs" -e "\(XEN\) Panic" \
+		-e "fault" -e "warn" -e "BUG" ./dmesg.new ; then
+		tst_res TFAIL "Issues found in dmesg!"
+	else
+		tst_res TPASS "Dynamic debug OK"
+	fi
+}
+
+cleanup()
+{
+	if [ $DEBUGFS_WAS_MOUNTED -eq 0 ] ; then
+		tst_umount "$DEBUGFS_PATH"
+	else
+		FLAGS_SET=$(awk -v emp="$EMPTY_FLAG" '$3 != emp' \
+			$DYNDEBUG_STATEMENTS)
+		if [ "$FLAGS_SET" ] ; then
+			FLAG_PREFIX=$([ $NEW_INTERFACE -eq 1 ] \
+				&& echo "" || echo "+")
+			echo "$FLAGS_SET" | while read -r FLAG_LINE ; do
+				echo -n "$FLAG_LINE" \
+					| awk -v prf="$FLAG_PREFIX" -F " |:" \
Quoted 9 lines

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 171

Quoted 5 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +5 -0
+					'{print "file "$1" line "$2" "prf $4}' \
+					> "$DEBUGFS_CONTROL"
+			done
+		fi
+	fi

I wonder if we should attempt to restore the values even if debugfs wasn't mounted and then umount it if it wasn't mounted once we are done.

Sounds good. While it was only meant to restore original setup, this might serve as additional check.

Quoted 11 lines

On my machine I have 45 values set to =p by default, although debugfs is mounted there...

testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh line 174

Quoted 3 lines
testcases/kernel/tracing/dynamic_debug/dynamic_debug01.sh new file +3 -0
+}
+
+tst_run

--
Cyril Hrubis
[email protected]

Thanks,

Veronika Kabatova

#3 lore raw

PATCH 1/1 - Check interface and functionality

Hi!

Quoted 7 lines

I'm not convinced using EXPECT_* would be the same since I only want to report failures and these functions report both TPASS and TFAIL. On my machine, this setup produces additional 16k of not that useful lines (if something fails, you are still notified about it and know every line before it passed). Of course if you can think of a use case where having reports for every passed line would make sense I can do this change.

Right, haven't thought of that, yes adding 16k of useless output is not something we want to do.

We may as well add EXPECT_PASS_SILENT or keep the code as it is...

Signature · 2 lines
Cyril Hrubis
[email protected]