Live-Patching Archive on lore.kernel.org
 help / color / Atom feed
From: Joe Lawrence <joe.lawrence@redhat.com>
To: Petr Mladek <pmladek@suse.com>
Cc: live-patching@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH 1/3] selftests/livepatch: Don't clear dmesg when running tests
Date: Sun, 14 Jun 2020 11:19:07 -0400
Message-ID: <5dc57181-383f-666a-8d3e-17d36ba6aa8c@redhat.com> (raw)
In-Reply-To: <20200612094903.GE4311@linux-b0ei>

On 6/12/20 5:49 AM, Petr Mladek wrote:
> On Wed 2020-06-10 13:20:59, Joe Lawrence wrote:
>> [ ... snip ... ]
>>   
>> +function start_test {
>> +	local test="$1"
>> +
>> +	# Save existing dmesg so we can detect new content below
>> +	SAVED_DMESG=$(mktemp --tmpdir -t klp-dmesg-XXXXXX)
> 
> There is a nice trick how to remove the temporary files even
> when the script fails from other reasons. The following should
> do the job:
> 
> function cleanup {
> 	rm -f "$SAVED_DMESG"
> }
> 
> trap cleanup EXIT
> 

Right, we currently trap EXIT INT TERM HUP to pop the dynamic debug and 
kernel.ftrace_enabled sysctl settings.  I can add a cleanup() function 
to take care of both the settings and the tmp file.

>> +	dmesg > "$SAVED_DMESG"
>> +
>> +	echo -n "TEST: $test ... "
>> +}
>> +
>>   # check_result() - verify dmesg output
>>   #	TODO - better filter, out of order msgs, etc?
>>   function check_result {
>>   	local expect="$*"
>>   	local result
>>   
>> -	result=$(dmesg | grep -v 'tainting' | grep -e 'livepatch:' -e 'test_klp' | sed 's/^\[[ 0-9.]*\] //')
>> +	result=$(dmesg | diff --changed-group-format='%>' --unchanged-group-format='' "$SAVED_DMESG" - | \
>> +		 grep -v 'tainting' | grep -e 'livepatch:' -e 'test_klp' | \
>> +		 sed 's/^\[[ 0-9.]*\] //')
>>   
>>   	if [[ "$expect" == "$result" ]] ; then
>>   		echo "ok"
>> @@ -257,4 +269,6 @@ function check_result {
>>   		echo -e "not ok\n\n$(diff -upr --label expected --label result <(echo "$expect") <(echo "$result"))\n"
>>   		die "livepatch kselftest(s) failed"
>>   	fi
>> +
>> +	rm -f "$SAVED_DMESG"
> 
> This change will not be necessary with the above trap handler.

Well start_test() and check_result() are called multiple times per 
script.  That means that with the original v1 change, we're creating new 
$SAVED_DMESG files per start_test() since check_result() is comparing 
only the new entries from test to test.

So I think this particular hunk in the modification would be necessary 
even with the trap handler.  The trap handler would still be nice to 
have in case the script is interrupted though.

> 
> Otherwise, I really like the change. I was always a bit worried that these
> tests were clearing all other messages.
> 

Yup, we ran into an instance internally where another test was failing 
because were blowing away the logs :(


-- Joe


  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-10 17:20 [PATCH 0/3] selftests/livepatch: small script cleanups Joe Lawrence
2020-06-10 17:20 ` [PATCH 1/3] selftests/livepatch: Don't clear dmesg when running tests Joe Lawrence
2020-06-11  7:38   ` Miroslav Benes
2020-06-11 13:01     ` Joe Lawrence
2020-06-11 13:06       ` Miroslav Benes
2020-06-12  9:49   ` Petr Mladek
2020-06-14 15:19     ` Joe Lawrence [this message]
2020-06-12 10:11   ` Petr Mladek
2020-06-10 17:21 ` [PATCH 2/3] selftests/livepatch: use $(dmesg --notime) instead of manually filtering Joe Lawrence
2020-06-12 10:12   ` Petr Mladek
2020-06-10 17:21 ` [PATCH 3/3] selftests/livepatch: filter 'taints' from dmesg comparison Joe Lawrence
2020-06-11  7:39   ` Miroslav Benes
2020-06-11 13:10     ` Joe Lawrence
2020-06-12 11:47       ` Petr Mladek
2020-06-12 12:57         ` Kamalesh Babulal
2020-06-14 14:45           ` Joe Lawrence
2020-06-15  7:55             ` Miroslav Benes
2020-06-15  9:19               ` Petr Mladek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5dc57181-383f-666a-8d3e-17d36ba6aa8c@redhat.com \
    --to=joe.lawrence@redhat.com \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=pmladek@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Live-Patching Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/live-patching/0 live-patching/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 live-patching live-patching/ https://lore.kernel.org/live-patching \
		live-patching@vger.kernel.org
	public-inbox-index live-patching

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.live-patching


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git