[PATCH] selftests/livepatch: add test skip handling

[PATCH] selftests/livepatch: add test skip handling

[Joe Lawrence]

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

Re: [PATCH] selftests/livepatch: add test skip handling

[Joe Lawrence]

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

Re: [PATCH] selftests/livepatch: add test skip handling

[Miroslav Benes]

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

Re: [PATCH] selftests/livepatch: add test skip handling

[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

Re: [PATCH] selftests/livepatch: add test skip handling

[Joe Lawrence]

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

Re: [PATCH] selftests/livepatch: add test skip handling

[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

Re: [PATCH] selftests/livepatch: add test skip handling

[Kamalesh Babulal]

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

Re: [PATCH] selftests/livepatch: add test skip handling

[Joe Lawrence]

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

Re: [PATCH] selftests/livepatch: add test skip handling

[Petr Mladek]

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