Live-Patching Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] selftests/livepatch: add test skip handling
@ 2019-07-14 14:28 Joe Lawrence
  2019-07-14 14:33 ` Joe Lawrence
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Joe Lawrence @ 2019-07-14 14:28 UTC (permalink / raw)
  To: live-patching, linux-kselftest; +Cc: shuah

Before running a livpeatch self-test, first verify that we've built and
installed the livepatch self-test kernel modules by running a 'modprobe
--dry-run'.  This should catch a few environment issues, including
!CONFIG_LIVEPATCH and !CONFIG_TEST_LIVEPATCH.  In these cases, exit
gracefully with test-skip status rather than test-fail status.

Reported-by: Jiri Benc <jbenc@redhat.com>
Suggested-by: Shuah Khan <shuah@kernel.org>
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 tools/testing/selftests/livepatch/functions.sh | 18 ++++++++++++++++++
 .../selftests/livepatch/test-callbacks.sh      |  5 +++++
 .../selftests/livepatch/test-livepatch.sh      |  3 +++
 .../selftests/livepatch/test-shadow-vars.sh    |  2 ++
 4 files changed, 28 insertions(+)

diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
index 30195449c63c..92d6cfb49365 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -13,6 +13,14 @@ function log() {
 	echo "$1" > /dev/kmsg
 }
 
+# skip(msg) - testing can't proceed
+#	msg - explanation
+function skip() {
+	log "SKIP: $1"
+	echo "SKIP: $1" >&2
+	exit 4
+}
+
 # die(msg) - game over, man
 #	msg - dying words
 function die() {
@@ -43,6 +51,16 @@ function loop_until() {
 	done
 }
 
+function assert_mod() {
+	local mod="$1"
+
+	if ! modprobe --dry-run "$mod" &>/dev/null ; then
+		skip "Failed modprobe --dry-run of module: $mod"
+	fi
+
+	return 1
+}
+
 function is_livepatch_mod() {
 	local mod="$1"
 
diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh
index e97a9dcb73c7..87a407cee7fd 100755
--- a/tools/testing/selftests/livepatch/test-callbacks.sh
+++ b/tools/testing/selftests/livepatch/test-callbacks.sh
@@ -9,6 +9,11 @@ MOD_LIVEPATCH2=test_klp_callbacks_demo2
 MOD_TARGET=test_klp_callbacks_mod
 MOD_TARGET_BUSY=test_klp_callbacks_busy
 
+assert_mod $MOD_LIVEPATCH
+assert_mod $MOD_LIVEPATCH2
+assert_mod $MOD_TARGET
+assert_mod $MOD_TARGET_BUSY
+
 set_dynamic_debug
 
 
diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh
index f05268aea859..8d3b75ceeeff 100755
--- a/tools/testing/selftests/livepatch/test-livepatch.sh
+++ b/tools/testing/selftests/livepatch/test-livepatch.sh
@@ -7,6 +7,9 @@
 MOD_LIVEPATCH=test_klp_livepatch
 MOD_REPLACE=test_klp_atomic_replace
 
+assert_mod $MOD_LIVEPATCH
+assert_mod $MOD_REPLACE
+
 set_dynamic_debug
 
 
diff --git a/tools/testing/selftests/livepatch/test-shadow-vars.sh b/tools/testing/selftests/livepatch/test-shadow-vars.sh
index 04a37831e204..1ab09bc50363 100755
--- a/tools/testing/selftests/livepatch/test-shadow-vars.sh
+++ b/tools/testing/selftests/livepatch/test-shadow-vars.sh
@@ -6,6 +6,8 @@
 
 MOD_TEST=test_klp_shadow_vars
 
+assert_mod $MOD_TEST
+
 set_dynamic_debug
 
 
-- 
2.21.0


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

* Re: [PATCH] selftests/livepatch: add test skip handling
  2019-07-14 14:28 [PATCH] selftests/livepatch: add test skip handling Joe Lawrence
@ 2019-07-14 14:33 ` Joe Lawrence
  2019-07-15 11:17   ` Miroslav Benes
  2019-07-19 22:11   ` shuah
  2019-07-15  6:50 ` Kamalesh Babulal
  2019-07-16 13:13 ` Petr Mladek
  2 siblings, 2 replies; 9+ messages in thread
From: Joe Lawrence @ 2019-07-14 14:33 UTC (permalink / raw)
  To: live-patching, linux-kselftest; +Cc: shuah

On Sun, Jul 14, 2019 at 10:28:29AM -0400, Joe Lawrence wrote:
> Before running a livpeatch self-test, first verify that we've built and
> installed the livepatch self-test kernel modules by running a 'modprobe
> --dry-run'.  This should catch a few environment issues, including
> !CONFIG_LIVEPATCH and !CONFIG_TEST_LIVEPATCH.  In these cases, exit
> gracefully with test-skip status rather than test-fail status.
> 
> Reported-by: Jiri Benc <jbenc@redhat.com>
> Suggested-by: Shuah Khan <shuah@kernel.org>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  tools/testing/selftests/livepatch/functions.sh | 18 ++++++++++++++++++
>  .../selftests/livepatch/test-callbacks.sh      |  5 +++++
>  .../selftests/livepatch/test-livepatch.sh      |  3 +++
>  .../selftests/livepatch/test-shadow-vars.sh    |  2 ++
>  4 files changed, 28 insertions(+)
> 
> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
> index 30195449c63c..92d6cfb49365 100644
> --- a/tools/testing/selftests/livepatch/functions.sh
> +++ b/tools/testing/selftests/livepatch/functions.sh
> @@ -13,6 +13,14 @@ function log() {
>  	echo "$1" > /dev/kmsg
>  }
>  
> +# skip(msg) - testing can't proceed
> +#	msg - explanation
> +function skip() {
> +	log "SKIP: $1"
> +	echo "SKIP: $1" >&2
> +	exit 4
> +}
> +
>  # die(msg) - game over, man
>  #	msg - dying words
>  function die() {
> @@ -43,6 +51,16 @@ function loop_until() {
>  	done
>  }
>  
> +function assert_mod() {
> +	local mod="$1"
> +
> +	if ! modprobe --dry-run "$mod" &>/dev/null ; then
> +		skip "Failed modprobe --dry-run of module: $mod"
> +	fi
> +
> +	return 1
> +}
> +
>  function is_livepatch_mod() {
>  	local mod="$1"
>  
> diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh
> index e97a9dcb73c7..87a407cee7fd 100755
> --- a/tools/testing/selftests/livepatch/test-callbacks.sh
> +++ b/tools/testing/selftests/livepatch/test-callbacks.sh
> @@ -9,6 +9,11 @@ MOD_LIVEPATCH2=test_klp_callbacks_demo2
>  MOD_TARGET=test_klp_callbacks_mod
>  MOD_TARGET_BUSY=test_klp_callbacks_busy
>  
> +assert_mod $MOD_LIVEPATCH
> +assert_mod $MOD_LIVEPATCH2
> +assert_mod $MOD_TARGET
> +assert_mod $MOD_TARGET_BUSY
> +
>  set_dynamic_debug
>  
>  
> diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh
> index f05268aea859..8d3b75ceeeff 100755
> --- a/tools/testing/selftests/livepatch/test-livepatch.sh
> +++ b/tools/testing/selftests/livepatch/test-livepatch.sh
> @@ -7,6 +7,9 @@
>  MOD_LIVEPATCH=test_klp_livepatch
>  MOD_REPLACE=test_klp_atomic_replace
>  
> +assert_mod $MOD_LIVEPATCH
> +assert_mod $MOD_REPLACE
> +
>  set_dynamic_debug
>  
>  
> diff --git a/tools/testing/selftests/livepatch/test-shadow-vars.sh b/tools/testing/selftests/livepatch/test-shadow-vars.sh
> index 04a37831e204..1ab09bc50363 100755
> --- a/tools/testing/selftests/livepatch/test-shadow-vars.sh
> +++ b/tools/testing/selftests/livepatch/test-shadow-vars.sh
> @@ -6,6 +6,8 @@
>  
>  MOD_TEST=test_klp_shadow_vars
>  
> +assert_mod $MOD_TEST
> +
>  set_dynamic_debug
>  
>  
> -- 
> 2.21.0
> 

Testing:

Here's the output if modprobe --dry-run doesn't like the modules (not
built, etc.):

  TAP version 13
  selftests: livepatch: test-livepatch.sh
  ========================================
  SKIP: Failed modprobe --dry-run of module: test_klp_livepatch
  not ok 1..1 selftests: livepatch: test-livepatch.sh [SKIP]
  selftests: livepatch: test-callbacks.sh
  ========================================
  SKIP: Failed modprobe --dry-run of module: test_klp_callbacks_demo
  not ok 1..2 selftests: livepatch: test-callbacks.sh [SKIP]
  selftests: livepatch: test-shadow-vars.sh
  ========================================
  SKIP: Failed modprobe --dry-run of module: test_klp_shadow_vars
  not ok 1..3 selftests: livepatch: test-shadow-vars.sh [SKIP]

We could fold assert_mod() into __load_mod() if folks perfer.  I
don't have strong opinion either way.

-- Joe

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

* Re: [PATCH] selftests/livepatch: add test skip handling
  2019-07-14 14:28 [PATCH] selftests/livepatch: add test skip handling Joe Lawrence
  2019-07-14 14:33 ` Joe Lawrence
@ 2019-07-15  6:50 ` Kamalesh Babulal
  2019-07-15 14:09   ` Joe Lawrence
  2019-07-16 13:13 ` Petr Mladek
  2 siblings, 1 reply; 9+ messages in thread
From: Kamalesh Babulal @ 2019-07-15  6:50 UTC (permalink / raw)
  To: Joe Lawrence, live-patching, linux-kselftest; +Cc: shuah

On 7/14/19 7:58 PM, Joe Lawrence wrote:
> Before running a livpeatch self-test, first verify that we've built and
> installed the livepatch self-test kernel modules by running a 'modprobe
> --dry-run'.  This should catch a few environment issues, including
> !CONFIG_LIVEPATCH and !CONFIG_TEST_LIVEPATCH.  In these cases, exit
> gracefully with test-skip status rather than test-fail status.
> 
> Reported-by: Jiri Benc <jbenc@redhat.com>
> Suggested-by: Shuah Khan <shuah@kernel.org>
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>

Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

[...]
> 
> +function assert_mod() {
> +	local mod="$1"
> +
> +	if ! modprobe --dry-run "$mod" &>/dev/null ; then

Just a preference comment, shorter version 'modprobe -q -n'
can be used here.

Thanks,
Kamalesh




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

* Re: [PATCH] selftests/livepatch: add test skip handling
  2019-07-14 14:33 ` Joe Lawrence
@ 2019-07-15 11:17   ` Miroslav Benes
  2019-07-19 22:11   ` shuah
  1 sibling, 0 replies; 9+ messages in thread
From: Miroslav Benes @ 2019-07-15 11:17 UTC (permalink / raw)
  To: Joe Lawrence; +Cc: live-patching, linux-kselftest, shuah

On Sun, 14 Jul 2019, Joe Lawrence wrote:

> On Sun, Jul 14, 2019 at 10:28:29AM -0400, Joe Lawrence wrote:
> > Before running a livpeatch self-test, first verify that we've built and
> > installed the livepatch self-test kernel modules by running a 'modprobe
> > --dry-run'.  This should catch a few environment issues, including
> > !CONFIG_LIVEPATCH and !CONFIG_TEST_LIVEPATCH.  In these cases, exit
> > gracefully with test-skip status rather than test-fail status.
> > 
> > Reported-by: Jiri Benc <jbenc@redhat.com>
> > Suggested-by: Shuah Khan <shuah@kernel.org>
> > Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> > ---
> >  tools/testing/selftests/livepatch/functions.sh | 18 ++++++++++++++++++
> >  .../selftests/livepatch/test-callbacks.sh      |  5 +++++
> >  .../selftests/livepatch/test-livepatch.sh      |  3 +++
> >  .../selftests/livepatch/test-shadow-vars.sh    |  2 ++
> >  4 files changed, 28 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
> > index 30195449c63c..92d6cfb49365 100644
> > --- a/tools/testing/selftests/livepatch/functions.sh
> > +++ b/tools/testing/selftests/livepatch/functions.sh
> > @@ -13,6 +13,14 @@ function log() {
> >  	echo "$1" > /dev/kmsg
> >  }
> >  
> > +# skip(msg) - testing can't proceed
> > +#	msg - explanation
> > +function skip() {
> > +	log "SKIP: $1"
> > +	echo "SKIP: $1" >&2
> > +	exit 4
> > +}
> > +
> >  # die(msg) - game over, man
> >  #	msg - dying words
> >  function die() {
> > @@ -43,6 +51,16 @@ function loop_until() {
> >  	done
> >  }
> >  
> > +function assert_mod() {
> > +	local mod="$1"
> > +
> > +	if ! modprobe --dry-run "$mod" &>/dev/null ; then
> > +		skip "Failed modprobe --dry-run of module: $mod"
> > +	fi
> > +
> > +	return 1
> > +}
> > +
> >  function is_livepatch_mod() {
> >  	local mod="$1"
> >  
> > diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh
> > index e97a9dcb73c7..87a407cee7fd 100755
> > --- a/tools/testing/selftests/livepatch/test-callbacks.sh
> > +++ b/tools/testing/selftests/livepatch/test-callbacks.sh
> > @@ -9,6 +9,11 @@ MOD_LIVEPATCH2=test_klp_callbacks_demo2
> >  MOD_TARGET=test_klp_callbacks_mod
> >  MOD_TARGET_BUSY=test_klp_callbacks_busy
> >  
> > +assert_mod $MOD_LIVEPATCH
> > +assert_mod $MOD_LIVEPATCH2
> > +assert_mod $MOD_TARGET
> > +assert_mod $MOD_TARGET_BUSY
> > +
> >  set_dynamic_debug
> >  
> >  
> > diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh
> > index f05268aea859..8d3b75ceeeff 100755
> > --- a/tools/testing/selftests/livepatch/test-livepatch.sh
> > +++ b/tools/testing/selftests/livepatch/test-livepatch.sh
> > @@ -7,6 +7,9 @@
> >  MOD_LIVEPATCH=test_klp_livepatch
> >  MOD_REPLACE=test_klp_atomic_replace
> >  
> > +assert_mod $MOD_LIVEPATCH
> > +assert_mod $MOD_REPLACE
> > +
> >  set_dynamic_debug
> >  
> >  
> > diff --git a/tools/testing/selftests/livepatch/test-shadow-vars.sh b/tools/testing/selftests/livepatch/test-shadow-vars.sh
> > index 04a37831e204..1ab09bc50363 100755
> > --- a/tools/testing/selftests/livepatch/test-shadow-vars.sh
> > +++ b/tools/testing/selftests/livepatch/test-shadow-vars.sh
> > @@ -6,6 +6,8 @@
> >  
> >  MOD_TEST=test_klp_shadow_vars
> >  
> > +assert_mod $MOD_TEST
> > +
> >  set_dynamic_debug
> >  
> >  
> > -- 
> > 2.21.0
> > 
> 
> Testing:
> 
> Here's the output if modprobe --dry-run doesn't like the modules (not
> built, etc.):
> 
>   TAP version 13
>   selftests: livepatch: test-livepatch.sh
>   ========================================
>   SKIP: Failed modprobe --dry-run of module: test_klp_livepatch
>   not ok 1..1 selftests: livepatch: test-livepatch.sh [SKIP]
>   selftests: livepatch: test-callbacks.sh
>   ========================================
>   SKIP: Failed modprobe --dry-run of module: test_klp_callbacks_demo
>   not ok 1..2 selftests: livepatch: test-callbacks.sh [SKIP]
>   selftests: livepatch: test-shadow-vars.sh
>   ========================================
>   SKIP: Failed modprobe --dry-run of module: test_klp_shadow_vars
>   not ok 1..3 selftests: livepatch: test-shadow-vars.sh [SKIP]
> 
> We could fold assert_mod() into __load_mod() if folks perfer.  I
> don't have strong opinion either way.

I think it would be better to move it there. Otherwise, we might forget to 
add assert_module call for new modules in the future.

Miroslav

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

* Re: [PATCH] selftests/livepatch: add test skip handling
  2019-07-15  6:50 ` Kamalesh Babulal
@ 2019-07-15 14:09   ` Joe Lawrence
  0 siblings, 0 replies; 9+ messages in thread
From: Joe Lawrence @ 2019-07-15 14:09 UTC (permalink / raw)
  To: Kamalesh Babulal, live-patching, linux-kselftest; +Cc: shuah

On 7/15/19 2:50 AM, Kamalesh Babulal wrote:
> On 7/14/19 7:58 PM, Joe Lawrence wrote:
>> Before running a livpeatch self-test, first verify that we've built and
>> installed the livepatch self-test kernel modules by running a 'modprobe
>> --dry-run'.  This should catch a few environment issues, including
>> !CONFIG_LIVEPATCH and !CONFIG_TEST_LIVEPATCH.  In these cases, exit
>> gracefully with test-skip status rather than test-fail status.
>>
>> Reported-by: Jiri Benc <jbenc@redhat.com>
>> Suggested-by: Shuah Khan <shuah@kernel.org>
>> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> 
> Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
> 
> [...]
>>
>> +function assert_mod() {
>> +	local mod="$1"
>> +
>> +	if ! modprobe --dry-run "$mod" &>/dev/null ; then
> 
> Just a preference comment, shorter version 'modprobe -q -n'
> can be used here.
> 

Hi Kamalesh,

Re: command line options: my preference has been to use the long form 
command switches inside scripts as they are more likely to be self 
documenting than their short counterparts.  e.g. I could have guessed 
that -q is --quiet, but not that -n is --dry-run.

Re: --quiet vs. command redirection: Another detail I don't have a 
strong opinion about.  I guess I very slightly prefer the redirect so I 
don't have to research various modprobe versions to determine if --quiet 
is universally supported (it probably is).

In both cases, I'll defer to whatever reviewers think is more 
readable/conventional for the self-tests.

-- Joe

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

* Re: [PATCH] selftests/livepatch: add test skip handling
  2019-07-14 14:28 [PATCH] selftests/livepatch: add test skip handling Joe Lawrence
  2019-07-14 14:33 ` Joe Lawrence
  2019-07-15  6:50 ` Kamalesh Babulal
@ 2019-07-16 13:13 ` Petr Mladek
  2 siblings, 0 replies; 9+ messages in thread
From: Petr Mladek @ 2019-07-16 13:13 UTC (permalink / raw)
  To: Joe Lawrence; +Cc: linux-kselftest, live-patching, shuah

On Sun 2019-07-14 10:28:29, Joe Lawrence wrote:
> Before running a livpeatch self-test, first verify that we've built and
> installed the livepatch self-test kernel modules by running a 'modprobe
> --dry-run'.  This should catch a few environment issues, including
> !CONFIG_LIVEPATCH and !CONFIG_TEST_LIVEPATCH.  In these cases, exit
> gracefully with test-skip status rather than test-fail status.
> 
> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
> index 30195449c63c..92d6cfb49365 100644
> --- a/tools/testing/selftests/livepatch/functions.sh
> +++ b/tools/testing/selftests/livepatch/functions.sh
> @@ -13,6 +13,14 @@ function log() {
>  	echo "$1" > /dev/kmsg
>  }
>  
> +# skip(msg) - testing can't proceed
> +#	msg - explanation
> +function skip() {
> +	log "SKIP: $1"
> +	echo "SKIP: $1" >&2
> +	exit 4
> +}
> +
>  # die(msg) - game over, man
>  #	msg - dying words
>  function die() {
> @@ -43,6 +51,16 @@ function loop_until() {
>  	done
>  }
>  
> +function assert_mod() {
> +	local mod="$1"
> +
> +	if ! modprobe --dry-run "$mod" &>/dev/null ; then

JFYI, I do not have strong opinion but the long option and redirection
looks fine to me.

> +		skip "Failed modprobe --dry-run of module: $mod"
> +	fi
> +
> +	return 1

return 0 or nothing?

> +}
> +
>  function is_livepatch_mod() {
>  	local mod="$1"
>  
> diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh
> index e97a9dcb73c7..87a407cee7fd 100755
> --- a/tools/testing/selftests/livepatch/test-callbacks.sh
> +++ b/tools/testing/selftests/livepatch/test-callbacks.sh
> @@ -9,6 +9,11 @@ MOD_LIVEPATCH2=test_klp_callbacks_demo2
>  MOD_TARGET=test_klp_callbacks_mod
>  MOD_TARGET_BUSY=test_klp_callbacks_busy
>  
> +assert_mod $MOD_LIVEPATCH
> +assert_mod $MOD_LIVEPATCH2
> +assert_mod $MOD_TARGET
> +assert_mod $MOD_TARGET_BUSY

I agree that moving this into __load_mod() is less error prone.

Best Regards,
Petr

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

* Re: [PATCH] selftests/livepatch: add test skip handling
  2019-07-14 14:33 ` Joe Lawrence
  2019-07-15 11:17   ` Miroslav Benes
@ 2019-07-19 22:11   ` shuah
  2019-07-20  2:51     ` Joe Lawrence
  1 sibling, 1 reply; 9+ messages in thread
From: shuah @ 2019-07-19 22:11 UTC (permalink / raw)
  To: Joe Lawrence, live-patching, linux-kselftest, shuah

On 7/14/19 8:33 AM, Joe Lawrence wrote:
> On Sun, Jul 14, 2019 at 10:28:29AM -0400, Joe Lawrence wrote:
>> Before running a livpeatch self-test, first verify that we've built and
>> installed the livepatch self-test kernel modules by running a 'modprobe
>> --dry-run'.  This should catch a few environment issues, including
>> !CONFIG_LIVEPATCH and !CONFIG_TEST_LIVEPATCH.  In these cases, exit
>> gracefully with test-skip status rather than test-fail status.
>>
>> Reported-by: Jiri Benc <jbenc@redhat.com>
>> Suggested-by: Shuah Khan <shuah@kernel.org>
>> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
>> ---
>>   tools/testing/selftests/livepatch/functions.sh | 18 ++++++++++++++++++
>>   .../selftests/livepatch/test-callbacks.sh      |  5 +++++
>>   .../selftests/livepatch/test-livepatch.sh      |  3 +++
>>   .../selftests/livepatch/test-shadow-vars.sh    |  2 ++
>>   4 files changed, 28 insertions(+)
>>
>> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
>> index 30195449c63c..92d6cfb49365 100644
>> --- a/tools/testing/selftests/livepatch/functions.sh
>> +++ b/tools/testing/selftests/livepatch/functions.sh
>> @@ -13,6 +13,14 @@ function log() {
>>   	echo "$1" > /dev/kmsg
>>   }
>>   
>> +# skip(msg) - testing can't proceed
>> +#	msg - explanation
>> +function skip() {
>> +	log "SKIP: $1"
>> +	echo "SKIP: $1" >&2
>> +	exit 4
>> +}
>> +
>>   # die(msg) - game over, man
>>   #	msg - dying words
>>   function die() {
>> @@ -43,6 +51,16 @@ function loop_until() {
>>   	done
>>   }
>>   
>> +function assert_mod() {
>> +	local mod="$1"
>> +
>> +	if ! modprobe --dry-run "$mod" &>/dev/null ; then
>> +		skip "Failed modprobe --dry-run of module: $mod"
>> +	fi
>> +
>> +	return 1
>> +}
>> +
>>   function is_livepatch_mod() {
>>   	local mod="$1"
>>   
>> diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh
>> index e97a9dcb73c7..87a407cee7fd 100755
>> --- a/tools/testing/selftests/livepatch/test-callbacks.sh
>> +++ b/tools/testing/selftests/livepatch/test-callbacks.sh
>> @@ -9,6 +9,11 @@ MOD_LIVEPATCH2=test_klp_callbacks_demo2
>>   MOD_TARGET=test_klp_callbacks_mod
>>   MOD_TARGET_BUSY=test_klp_callbacks_busy
>>   
>> +assert_mod $MOD_LIVEPATCH
>> +assert_mod $MOD_LIVEPATCH2
>> +assert_mod $MOD_TARGET
>> +assert_mod $MOD_TARGET_BUSY
>> +
>>   set_dynamic_debug
>>   
>>   
>> diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh
>> index f05268aea859..8d3b75ceeeff 100755
>> --- a/tools/testing/selftests/livepatch/test-livepatch.sh
>> +++ b/tools/testing/selftests/livepatch/test-livepatch.sh
>> @@ -7,6 +7,9 @@
>>   MOD_LIVEPATCH=test_klp_livepatch
>>   MOD_REPLACE=test_klp_atomic_replace
>>   
>> +assert_mod $MOD_LIVEPATCH
>> +assert_mod $MOD_REPLACE
>> +
>>   set_dynamic_debug
>>   
>>   
>> diff --git a/tools/testing/selftests/livepatch/test-shadow-vars.sh b/tools/testing/selftests/livepatch/test-shadow-vars.sh
>> index 04a37831e204..1ab09bc50363 100755
>> --- a/tools/testing/selftests/livepatch/test-shadow-vars.sh
>> +++ b/tools/testing/selftests/livepatch/test-shadow-vars.sh
>> @@ -6,6 +6,8 @@
>>   
>>   MOD_TEST=test_klp_shadow_vars
>>   
>> +assert_mod $MOD_TEST
>> +
>>   set_dynamic_debug
>>   
>>   
>> -- 
>> 2.21.0
>>
> 
> Testing:
> 
> Here's the output if modprobe --dry-run doesn't like the modules (not
> built, etc.):
> 
>    TAP version 13
>    selftests: livepatch: test-livepatch.sh
>    ========================================
>    SKIP: Failed modprobe --dry-run of module: test_klp_livepatch
>    not ok 1..1 selftests: livepatch: test-livepatch.sh [SKIP]
>    selftests: livepatch: test-callbacks.sh
>    ========================================
>    SKIP: Failed modprobe --dry-run of module: test_klp_callbacks_demo
>    not ok 1..2 selftests: livepatch: test-callbacks.sh [SKIP]
>    selftests: livepatch: test-shadow-vars.sh
>    ========================================
>    SKIP: Failed modprobe --dry-run of module: test_klp_shadow_vars
>    not ok 1..3 selftests: livepatch: test-shadow-vars.sh [SKIP]
> 
> We could fold assert_mod() into __load_mod() if folks perfer.  I
> don't have strong opinion either way.
> 

Please refine these messages to say what users should do. In addition
to what failed, also add what is missing - enable config option etc.

thanks,
-- Shuah


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

* Re: [PATCH] selftests/livepatch: add test skip handling
  2019-07-19 22:11   ` shuah
@ 2019-07-20  2:51     ` Joe Lawrence
  2019-07-20 11:26       ` shuah
  0 siblings, 1 reply; 9+ messages in thread
From: Joe Lawrence @ 2019-07-20  2:51 UTC (permalink / raw)
  To: shuah, live-patching, linux-kselftest

On 7/19/19 6:11 PM, shuah wrote:
> On 7/14/19 8:33 AM, Joe Lawrence wrote:
>> On Sun, Jul 14, 2019 at 10:28:29AM -0400, Joe Lawrence wrote:
>> 
 >> [ ... snip ... ]
>>
>> Testing:
>>
>> Here's the output if modprobe --dry-run doesn't like the modules (not
>> built, etc.):
>>
>>     TAP version 13
>>     selftests: livepatch: test-livepatch.sh
>>     ========================================
>>     SKIP: Failed modprobe --dry-run of module: test_klp_livepatch
>>     not ok 1..1 selftests: livepatch: test-livepatch.sh [SKIP]
>>     selftests: livepatch: test-callbacks.sh
>>     ========================================
>>     SKIP: Failed modprobe --dry-run of module: test_klp_callbacks_demo
>>     not ok 1..2 selftests: livepatch: test-callbacks.sh [SKIP]
>>     selftests: livepatch: test-shadow-vars.sh
>>     ========================================
>>     SKIP: Failed modprobe --dry-run of module: test_klp_shadow_vars
>>     not ok 1..3 selftests: livepatch: test-shadow-vars.sh [SKIP]
>> 
> 
> Please refine these messages to say what users should do. In addition
> to what failed, also add what is missing - enable config option etc.
> 

Hi Shuah,

Note that v2 was posted [1], but the output remains basically the same, 
so your comments still apply.

Off the top of my head, modprobe can fail for many reasons: unprivileged 
user, missing .ko files, missing modules.dep entry, kernel vermagic, 
interface versions, etc.

What would you think about modifying our skip() function to provide a 
catch-all list of CONFIG, environment, etc. requirements?  Something 
like, "Please ensure that the kernel was build with CONFIG_XYZ and that 
the tests are run with foo privileges"?  That would let us avoid trying 
to figure out exactly why the modprobe failed, but still help out the user.

Alternatively, are there existing modprobe callers in the selftests that 
already do a better job?

[1] 
https://lore.kernel.org/linux-kselftest/eac825ab-04c2-77cf-671e-1a2a576109b0@linux.vnet.ibm.com/T/#mc2f70095215738cb6a539e2c2e6a415c78a8add6

Thanks,

-- Joe

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

* Re: [PATCH] selftests/livepatch: add test skip handling
  2019-07-20  2:51     ` Joe Lawrence
@ 2019-07-20 11:26       ` shuah
  0 siblings, 0 replies; 9+ messages in thread
From: shuah @ 2019-07-20 11:26 UTC (permalink / raw)
  To: Joe Lawrence, live-patching, linux-kselftest, shuah

On 7/19/19 8:51 PM, Joe Lawrence wrote:
> On 7/19/19 6:11 PM, shuah wrote:
>> On 7/14/19 8:33 AM, Joe Lawrence wrote:
>>> On Sun, Jul 14, 2019 at 10:28:29AM -0400, Joe Lawrence wrote:
>>>
>  >> [ ... snip ... ]
>>>
>>> Testing:
>>>
>>> Here's the output if modprobe --dry-run doesn't like the modules (not
>>> built, etc.):
>>>
>>>     TAP version 13
>>>     selftests: livepatch: test-livepatch.sh
>>>     ========================================
>>>     SKIP: Failed modprobe --dry-run of module: test_klp_livepatch
>>>     not ok 1..1 selftests: livepatch: test-livepatch.sh [SKIP]
>>>     selftests: livepatch: test-callbacks.sh
>>>     ========================================
>>>     SKIP: Failed modprobe --dry-run of module: test_klp_callbacks_demo
>>>     not ok 1..2 selftests: livepatch: test-callbacks.sh [SKIP]
>>>     selftests: livepatch: test-shadow-vars.sh
>>>     ========================================
>>>     SKIP: Failed modprobe --dry-run of module: test_klp_shadow_vars
>>>     not ok 1..3 selftests: livepatch: test-shadow-vars.sh [SKIP]
>>>
>>
>> Please refine these messages to say what users should do. In addition
>> to what failed, also add what is missing - enable config option etc.
>>
> 
> Hi Shuah,
> 
> Note that v2 was posted [1], but the output remains basically the same, 
> so your comments still apply.
> 
> Off the top of my head, modprobe can fail for many reasons: unprivileged 
> user, missing .ko files, missing modules.dep entry, kernel vermagic, 
> interface versions, etc.
> 


> What would you think about modifying our skip() function to provide a 
> catch-all list of CONFIG, environment, etc. requirements?  Something 
> like, "Please ensure that the kernel was build with CONFIG_XYZ and that 
> the tests are run with foo privileges"?  That would let us avoid trying 
> to figure out exactly why the modprobe failed, but still help out the user.
> 

I understand. I am not suggesting that you have to figure out why. I am
suggesting that instead of "Failed modprobe --dry-run of module", say
"Unable to load test_klp_shadow_vars module. Check if config option is
enabled"  which is user friendly compared to the current message.

thanks,
-- Shuah



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

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-14 14:28 [PATCH] selftests/livepatch: add test skip handling Joe Lawrence
2019-07-14 14:33 ` Joe Lawrence
2019-07-15 11:17   ` Miroslav Benes
2019-07-19 22:11   ` shuah
2019-07-20  2:51     ` Joe Lawrence
2019-07-20 11:26       ` shuah
2019-07-15  6:50 ` Kamalesh Babulal
2019-07-15 14:09   ` Joe Lawrence
2019-07-16 13:13 ` Petr Mladek

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 live-patching@archiver.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