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

From: shuah <shuah@kernel.org>
To: Joe Lawrence <joe.lawrence@redhat.com>,
	live-patching@vger.kernel.org, linux-kselftest@vger.kernel.org,
	shuah <shuah@kernel.org>
Subject: Re: [PATCH] selftests/livepatch: add test skip handling
Date: Sat, 20 Jul 2019 05:26:19 -0600	[thread overview]
Message-ID: <215509dd-27d9-244a-870b-69df69fef38a@kernel.org> (raw)
In-Reply-To: <42feff41-8a92-e252-0e90-5c0335547de1@redhat.com>

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