[RFC] lib: Add support for test tags

Message ID 20181107142209.8965-1-chrubis@suse.cz
State New
Headers show
Series
  • [RFC] lib: Add support for test tags
Related show

Commit Message

Cyril Hrubis Nov. 7, 2018, 2:22 p.m.
This is just proof of concept of moving some of the test metadata into
more structured form. If implemented it will move some information from
comments in the test source code to an array to the tst_test structure.

It's not finished and certainly not set into a stone, this patch is
mainly intended to start a discussion.

The newly introduced test tags are generic name-value pairs that can
hold test metadata, the intended use for now is to store kernel commit
hashes for kernel reproducers as well as CVE ids. The mechanism is
however choosen to be very generic so that it's easy to add basically
any information later on.

As it is the main purpose is to print hints for a test failures. If a
test that has been written as a kernel reproducer fails it prints nice
URL pointing to a kernel commit that may be missing.

Example output:

tst_test.c:1145: INFO: Timeout per run is 0h 05m 00s
add_key02.c:98: FAIL: unexpected error with key type 'asymmetric': EINVAL
add_key02.c:98: FAIL: unexpected error with key type 'cifs.idmap': EINVAL
add_key02.c:98: FAIL: unexpected error with key type 'cifs.spnego': EINVAL
add_key02.c:98: FAIL: unexpected error with key type 'pkcs7_test': EINVAL
add_key02.c:98: FAIL: unexpected error with key type 'rxrpc': EINVAL
add_key02.c:98: FAIL: unexpected error with key type 'rxrpc_s': EINVAL
add_key02.c:98: FAIL: unexpected error with key type 'user': EINVAL
add_key02.c:98: FAIL: unexpected error with key type 'logon': EINVAL

HINT: This is a regression test for linux kernel, see commit:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5649645d725c

HINT: This test also tests for CVE-2017-15274, see:

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-15274

Summary:
passed   0
failed   8
skipped  0
warnings 0

This commit also adds adds the -q test flag, that can be used to query
test information, which includes these tags, but is not limited to them.

The main inteded use for the query operation is to export test metadata
and constraints to the test execution system. The long term goal for
this would be parallel test execution as for this case the test runner
would need to know which global system resources is the test using to
avoid unexpected failures.

So far it exposes only if test needs root and if block device is needed
for the test, but I would expect that we will need a few more tags for
various resources, one that comes to my mind would be "test is using
SystemV SHM" for that we can do something as add a "constraint" tag with
value "SysV SHM" or anything else that would be fitting. Another would
be "Test is changing system wide clocks", etc.

So far the output from the query operation looks as:

sh$ ./add_key02 -q
{
 "root": false
 "blk_device": false
 "linux-git": "5649645d725c"
 "CVE": "2017-15274"
}

The format is meant to be machine-parseable, but it is certainly not
final as there are no consumers for the data yet.

Another nice feature of having this information in the test runner is
that we can include the URL pointing to a kernel git commit fixing the
issue even for kernel reproducers that crashed the kernel under test and
failed to write out any logs. Which is not that important I guess, but
still nice to have.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
CC: automated-testing@yoctoproject.org
---
 include/tst_test.h                            | 10 +++
 lib/tst_test.c                                | 98 ++++++++++++++++++++++-----
 testcases/kernel/syscalls/add_key/add_key02.c |  7 ++
 3 files changed, 97 insertions(+), 18 deletions(-)

Comments

Li Wang Nov. 8, 2018, 5:11 a.m. | #1
On Wed, Nov 7, 2018 at 10:22 PM, Cyril Hrubis <chrubis@suse.cz> wrote:
> This is just proof of concept of moving some of the test metadata into
> more structured form. If implemented it will move some information from
> comments in the test source code to an array to the tst_test structure.

I have to say, this is really a good proposal to LTP especially for
that regressions, we sometimes spend a lot of time to dig into test
failures and read code comments to find the reason/clue, if now
testcase can print out useful info when trigger the issue, that'd be
more friendly to LTP runner.

Another thought come to my mind is, can we build a correlation for one
case which have many alias? e.g cve-2017-15274 == add_key02. If LTP
framework has finished cve-2017-15274 test then run add_key02 in next,
just skip and mark it as the same result with cve-2017-15274 show.

>
> It's not finished and certainly not set into a stone, this patch is
> mainly intended to start a discussion.
>
> The newly introduced test tags are generic name-value pairs that can
> hold test metadata, the intended use for now is to store kernel commit
> hashes for kernel reproducers as well as CVE ids. The mechanism is
> however choosen to be very generic so that it's easy to add basically
> any information later on.
>
> As it is the main purpose is to print hints for a test failures. If a
> test that has been written as a kernel reproducer fails it prints nice
> URL pointing to a kernel commit that may be missing.

The key point is, we'd better set a strict limitation to print hints
only for known/expected failures. Because if the failure is a new
bz/issue for a system or testcase, that messages will be useless and
even misleading.

>
> This commit also adds adds the -q test flag, that can be used to query
> test information, which includes these tags, but is not limited to them.
>
> The main inteded use for the query operation is to export test metadata
> and constraints to the test execution system. The long term goal for
> this would be parallel test execution as for this case the test runner
> would need to know which global system resources is the test using to
> avoid unexpected failures.
>
> So far it exposes only if test needs root and if block device is needed
> for the test, but I would expect that we will need a few more tags for
> various resources, one that comes to my mind would be "test is using
> SystemV SHM" for that we can do something as add a "constraint" tag with
> value "SysV SHM" or anything else that would be fitting. Another would
> be "Test is changing system wide clocks", etc.

Sounds good.

But why only special( needs root or block-device) test can use that.
Maybe we could just define a generic struct for the tag, about the
real contents, test author can fill what they want to highlight.
Cyril Hrubis Nov. 8, 2018, 12:52 p.m. | #2
Hi!
> > This is just proof of concept of moving some of the test metadata into
> > more structured form. If implemented it will move some information from
> > comments in the test source code to an array to the tst_test structure.
> 
> I have to say, this is really a good proposal to LTP especially for
> that regressions, we sometimes spend a lot of time to dig into test
> failures and read code comments to find the reason/clue, if now
> testcase can print out useful info when trigger the issue, that'd be
> more friendly to LTP runner.

This is exactly the reason I looked into the problem.

> Another thought come to my mind is, can we build a correlation for one
> case which have many alias? e.g cve-2017-15274 == add_key02. If LTP
> framework has finished cve-2017-15274 test then run add_key02 in next,
> just skip and mark it as the same result with cve-2017-15274 show.

Well the way how we track testcases is something that should be probably
rethinked in the future. The runtest files are far from optimal, maybe
we can build something based on tags in the future.

> > It's not finished and certainly not set into a stone, this patch is
> > mainly intended to start a discussion.
> >
> > The newly introduced test tags are generic name-value pairs that can
> > hold test metadata, the intended use for now is to store kernel commit
> > hashes for kernel reproducers as well as CVE ids. The mechanism is
> > however choosen to be very generic so that it's easy to add basically
> > any information later on.
> >
> > As it is the main purpose is to print hints for a test failures. If a
> > test that has been written as a kernel reproducer fails it prints nice
> > URL pointing to a kernel commit that may be missing.
> 
> The key point is, we'd better set a strict limitation to print hints
> only for known/expected failures. Because if the failure is a new
> bz/issue for a system or testcase, that messages will be useless and
> even misleading.

Well there is always possibility for this to be misleading, once
regression has been reintroduced into the kernel, it will point to a
patch that has been already merged.

What do you have in mind? I couldn't figure out anything better than
this. I.e. if test has failed and there is reproducer tag, print the
hint. Maybe we can reword the hint in a way that it's clear that there
are other possible reason for the failure.

> > This commit also adds adds the -q test flag, that can be used to query
> > test information, which includes these tags, but is not limited to them.
> >
> > The main inteded use for the query operation is to export test metadata
> > and constraints to the test execution system. The long term goal for
> > this would be parallel test execution as for this case the test runner
> > would need to know which global system resources is the test using to
> > avoid unexpected failures.
> >
> > So far it exposes only if test needs root and if block device is needed
> > for the test, but I would expect that we will need a few more tags for
> > various resources, one that comes to my mind would be "test is using
> > SystemV SHM" for that we can do something as add a "constraint" tag with
> > value "SysV SHM" or anything else that would be fitting. Another would
> > be "Test is changing system wide clocks", etc.
> 
> Sounds good.
> 
> But why only special( needs root or block-device) test can use that.

For this patch I only picked two attributes from the tst_test structure
that are relevant for the test execution, we will probably want to
export a few more later on.

> Maybe we could just define a generic struct for the tag, about the
> real contents, test author can fill what they want to highlight.

You lost me there, what exactly do you have in mind here?
Jan Stancek Nov. 8, 2018, 5:30 p.m. | #3
Hi,

----- Original Message -----
> This is just proof of concept of moving some of the test metadata into
> more structured form. If implemented it will move some information from
> comments in the test source code to an array to the tst_test structure.
> 
> It's not finished and certainly not set into a stone, this patch is
> mainly intended to start a discussion.
> 
> The newly introduced test tags are generic name-value pairs that can
> hold test metadata, the intended use for now is to store kernel commit
> hashes for kernel reproducers as well as CVE ids. The mechanism is
> however choosen to be very generic so that it's easy to add basically
> any information later on.
> 
> As it is the main purpose is to print hints for a test failures. If a
> test that has been written as a kernel reproducer fails it prints nice
> URL pointing to a kernel commit that may be missing.
> 
> Example output:
> 
> tst_test.c:1145: INFO: Timeout per run is 0h 05m 00s
> add_key02.c:98: FAIL: unexpected error with key type 'asymmetric': EINVAL
> add_key02.c:98: FAIL: unexpected error with key type 'cifs.idmap': EINVAL
> add_key02.c:98: FAIL: unexpected error with key type 'cifs.spnego': EINVAL
> add_key02.c:98: FAIL: unexpected error with key type 'pkcs7_test': EINVAL
> add_key02.c:98: FAIL: unexpected error with key type 'rxrpc': EINVAL
> add_key02.c:98: FAIL: unexpected error with key type 'rxrpc_s': EINVAL
> add_key02.c:98: FAIL: unexpected error with key type 'user': EINVAL
> add_key02.c:98: FAIL: unexpected error with key type 'logon': EINVAL
> 
> HINT: This is a regression test for linux kernel, see commit:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5649645d725c
> 
> HINT: This test also tests for CVE-2017-15274, see:
> 
> https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-15274
> 
> Summary:
> passed   0
> failed   8
> skipped  0
> warnings 0
> 
> This commit also adds adds the -q test flag, that can be used to query
> test information, which includes these tags, but is not limited to them.
> 
> The main inteded use for the query operation is to export test metadata
> and constraints to the test execution system. The long term goal for
> this would be parallel test execution as for this case the test runner
> would need to know which global system resources is the test using to
> avoid unexpected failures.
> 
> So far it exposes only if test needs root and if block device is needed
> for the test, but I would expect that we will need a few more tags for
> various resources, one that comes to my mind would be "test is using
> SystemV SHM" for that we can do something as add a "constraint" tag with
> value "SysV SHM" or anything else that would be fitting. Another would
> be "Test is changing system wide clocks", etc.
> 
> So far the output from the query operation looks as:
> 
> sh$ ./add_key02 -q
> {
>  "root": false
>  "blk_device": false
>  "linux-git": "5649645d725c"
>  "CVE": "2017-15274"
> }

So, only way to get metadata for tools is to run the test
with -q on supported target? (since I assume when
TST_TEST_TCONF branch hits, there won't be any data).

Would there be any benefit to having metadata in a (json)
file from the start? Negative would be likely duplicating
things like "needs_root". Positive is we could check for
errors/typos during build time.

Regards,
Jan

> 
> The format is meant to be machine-parseable, but it is certainly not
> final as there are no consumers for the data yet.
> 
> Another nice feature of having this information in the test runner is
> that we can include the URL pointing to a kernel git commit fixing the
> issue even for kernel reproducers that crashed the kernel under test and
> failed to write out any logs. Which is not that important I guess, but
> still nice to have.
Bird, Timothy Nov. 8, 2018, 8:01 p.m. | #4
> -----Original Message-----
> From: Cyril Hrubis on  Thursday, November 08, 2018 4:52 AM
> 
> Hi!
> > > This is just proof of concept of moving some of the test metadata into
> > > more structured form. If implemented it will move some information
> from
> > > comments in the test source code to an array to the tst_test structure.
> >
> > I have to say, this is really a good proposal to LTP especially for
> > that regressions, we sometimes spend a lot of time to dig into test
> > failures and read code comments to find the reason/clue, if now
> > testcase can print out useful info when trigger the issue, that'd be
> > more friendly to LTP runner.
> 
> This is exactly the reason I looked into the problem.

I'll go on record as really liking this proposal as well, for the same reasons
given.

We created a mechanism in Fuego to address this issue, but it would
be much better to see it addressed upstream.  I'll describe below our
"solution" so  you can compare it's features with the proposed features.
Note that it is only partially implemented and not populated with
much information yet (only a few testcases have the files described).

> 
> > Another thought come to my mind is, can we build a correlation for one
> > case which have many alias? e.g cve-2017-15274 == add_key02. If LTP
> > framework has finished cve-2017-15274 test then run add_key02 in next,
> > just skip and mark it as the same result with cve-2017-15274 show.
> 
> Well the way how we track testcases is something that should be probably
> rethinked in the future. The runtest files are far from optimal, maybe
> we can build something based on tags in the future.

I'm a big proponent of having each testcase have a unique identifier, to
solve this problem.  There were a few slides in the ATS presentation about
what I call 'tguids' (Testcase Globally Unique Identifiers), but I didn't have
time to get into the rationale for these at the summit.  

At one Linaro Connect where I presented this idea, Neil Williams gave
some good feedback, and pointed out some problems with the idea,
but I think it would be good to discuss this concept on the list.

I'll try to start a discussion thread on tguids and UTI (uniform testcase
identifiers) sometime in the future, to discuss some of the issues.

> 
> > > It's not finished and certainly not set into a stone, this patch is
> > > mainly intended to start a discussion.
> > >
> > > The newly introduced test tags are generic name-value pairs that can
> > > hold test metadata, the intended use for now is to store kernel commit
> > > hashes for kernel reproducers as well as CVE ids. The mechanism is
> > > however choosen to be very generic so that it's easy to add basically
> > > any information later on.
> > >
> > > As it is the main purpose is to print hints for a test failures. If a
> > > test that has been written as a kernel reproducer fails it prints nice
> > > URL pointing to a kernel commit that may be missing.

That's really great.

> > > This commit also adds adds the -q test flag, that can be used to query
> > > test information, which includes these tags, but is not limited to them.
> > >
> > > The main inteded use for the query operation is to export test metadata
> > > and constraints to the test execution system. The long term goal for
> > > this would be parallel test execution as for this case the test runner
> > > would need to know which global system resources is the test using to
> > > avoid unexpected failures.
> > >
> > > So far it exposes only if test needs root and if block device is needed
> > > for the test, but I would expect that we will need a few more tags for
> > > various resources, one that comes to my mind would be "test is using
> > > SystemV SHM" for that we can do something as add a "constraint" tag
> with
> > > value "SysV SHM" or anything else that would be fitting. Another would
> > > be "Test is changing system wide clocks", etc.

It sounds like you will be preserving test metadata with two different uses:
1) dependencies required for the test to execute
2) possible explanations for test failure

There might be a value in keeping these distinct.

I can think of some other use categories that meta-data
might fall into. One would be:
3) things that need to be (can be) adjusted on the target in
order for a test to run (this is different from something
that straight-up blocks a test from being able to run on the target)

Overall, I think it would be useful to clarify the category and
expected handling for the different meta-data that is defined
around tests.

It also might be good to share different systems constraint/dependency
mechanisms and phrasing, for more commonality between systems and
easier understanding by users.  But that's independent of this hinting
thing you're talking about.

----
Here's how we solved the problem of allowing users to share
information with each other about testcases, in Fuego.

For each test, there is (or can be) a documentation directory, where
reStructuredText documents can be placed to describe testcases.  It is
expected that the directory would be sparse, and that only the
"problematical" testcases would have this documentation.

The overall idea is to prevent users from having to research
failures by digging through code, if someone else had already
done that and posted the information.

Here is our document for the testcase we call: "Functional.LTP.syscalls.add_key02"
The file is between ---------------------------- lines, with additional explanation (this e-mail)
below it:

File: fuego-core/engine/tests/Functional.LTP/docs/Functional.LTP.syscalls.add_key02.ftmp
----------------------------------------------------
===========
Description
===========

Obtained from addkey02.c DESCRIPTION:
	Test that the add_key() syscall correctly handles a NULL payload with nonzero
	length.  Specifically, it should fail with EFAULT rather than oopsing the
	kernel with a NULL pointer dereference or failing with EINVAL, as it did
	before (depending on the key type).  This is a regression test for commit
	5649645d725c ("KEYS: fix dereferencing NULL payload with nonzero length").
	
	Note that none of the key types that exhibited the NULL pointer dereference
	are guaranteed to be built into the kernel, so we just test as many as we
	can, in the hope of catching one.  We also test with the "user" key type for
	good measure, although it was one of the types that failed with EINVAL rather
	than dereferencing NULL.
	
	This has been assigned CVE-2017-15274.

Other notes:
	Commit 5649645d725c appears to have been included since the kernel 4.12.

====
Tags
====

* kernel, syscall, addkey

=========
Resources
=========

* https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5649645d725c73df4302428ee4e02c869248b4c5

=======
Results
=======

Running on a PC (64 bits) using Debian Jessie (kernel 3.16):
  add_key02.c:81: CONF: kernel doesn't support key type 'asymmetric'
  add_key02.c:81: CONF: kernel doesn't support key type 'cifs.idmap'
  add_key02.c:81: CONF: kernel doesn't support key type 'cifs.spnego'
  add_key02.c:81: CONF: kernel doesn't support key type 'pkcs7_test'
  add_key02.c:81: CONF: kernel doesn't support key type 'rxrpc'
  add_key02.c:81: CONF: kernel doesn't support key type 'rxrpc_s'
  add_key02.c:96: FAIL: unexpected error with key type 'user': EINVAL
  add_key02.c:96: FAIL: unexpected error with key type 'logon': EINVAL

The kernel should have returned an EFAULT error, not EINVAL:
  Ref: https://github.com/linux-test-project/ltp/issues/182

.. fuego_result_list::

======
Status
======

.. fuego_status::

=====
Notes
=====
----------------------------------------------------

So, a few more observations on this...
The format is rst, with some Sphinx macros.  This allows
the system to replace the macros with data from the current system
(from a set of runs).  The macros were not parameterized yet, but
the intent was to add parameters to the macros so that a report
generated with this file would include a data over a specific time
period, or with specific attributes (e.g. only the failures), and indicating
what meta-data fields from the test runs to include.  Thus, Fuego 
end-users could customize the output from these using external
settings.  This was intended to allow us to populate the results
interface with nice friendly documents with additional data.

This puts the information into a human-readable form, with
tables with recent results, but IMHO this doesn't lend itself to
additional automation, the way your more-structured tag system
does.  I could envision in your system a mechanism that went back
to the source and did a check using git to see if the kernel included
the commit or not, and if so flagging this as a regression.  That would
be a really neat additional level of results diagnosis/analysis, that
could be automated with your system.

In any event - that's what we're doing now in Fuego to solve what
I think is the same problem.
  -- Tim

P.S. If you want to see additional testcase documentation files in Fuego
for LTP, please see:
https://bitbucket.org/tbird20d/fuego-core/src/bf8c28cab5ec2dde5552ed2ff1e6fe2e0abf9582/engine/tests/Functional.LTP/docs/
We don't have a lot of them yet, but they show the general pattern of
what we were trying for.
Bird, Timothy Nov. 8, 2018, 8:19 p.m. | #5
> -----Original Message-----
> From: Jan Stancek
> 
> Hi,
> 
> ----- Original Message -----
> > This is just proof of concept of moving some of the test metadata into
> > more structured form. If implemented it will move some information from
> > comments in the test source code to an array to the tst_test structure.
> >
> > It's not finished and certainly not set into a stone, this patch is
> > mainly intended to start a discussion.
> >
> > The newly introduced test tags are generic name-value pairs that can
> > hold test metadata, the intended use for now is to store kernel commit
> > hashes for kernel reproducers as well as CVE ids. The mechanism is
> > however choosen to be very generic so that it's easy to add basically
> > any information later on.
> >
> > As it is the main purpose is to print hints for a test failures. If a
> > test that has been written as a kernel reproducer fails it prints nice
> > URL pointing to a kernel commit that may be missing.
> >
> > Example output:
> >
> > tst_test.c:1145: INFO: Timeout per run is 0h 05m 00s
> > add_key02.c:98: FAIL: unexpected error with key type 'asymmetric':
> EINVAL
> > add_key02.c:98: FAIL: unexpected error with key type 'cifs.idmap': EINVAL
> > add_key02.c:98: FAIL: unexpected error with key type 'cifs.spnego': EINVAL
> > add_key02.c:98: FAIL: unexpected error with key type 'pkcs7_test': EINVAL
> > add_key02.c:98: FAIL: unexpected error with key type 'rxrpc': EINVAL
> > add_key02.c:98: FAIL: unexpected error with key type 'rxrpc_s': EINVAL
> > add_key02.c:98: FAIL: unexpected error with key type 'user': EINVAL
> > add_key02.c:98: FAIL: unexpected error with key type 'logon': EINVAL
> >
> > HINT: This is a regression test for linux kernel, see commit:
> >
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i
> d=5649645d725c
> >
> > HINT: This test also tests for CVE-2017-15274, see:
> >
> > https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-15274
> >
> > Summary:
> > passed   0
> > failed   8
> > skipped  0
> > warnings 0
> >
> > This commit also adds adds the -q test flag, that can be used to query
> > test information, which includes these tags, but is not limited to them.
> >
> > The main inteded use for the query operation is to export test metadata
> > and constraints to the test execution system. The long term goal for
> > this would be parallel test execution as for this case the test runner
> > would need to know which global system resources is the test using to
> > avoid unexpected failures.
> >
> > So far it exposes only if test needs root and if block device is needed
> > for the test, but I would expect that we will need a few more tags for
> > various resources, one that comes to my mind would be "test is using
> > SystemV SHM" for that we can do something as add a "constraint" tag with
> > value "SysV SHM" or anything else that would be fitting. Another would
> > be "Test is changing system wide clocks", etc.
> >
> > So far the output from the query operation looks as:
> >
> > sh$ ./add_key02 -q
> > {
> >  "root": false
> >  "blk_device": false
> >  "linux-git": "5649645d725c"
> >  "CVE": "2017-15274"
> > }
> 
> So, only way to get metadata for tools is to run the test
> with -q on supported target? (since I assume when
> TST_TEST_TCONF branch hits, there won't be any data).
> 
> Would there be any benefit to having metadata in a (json)
> file from the start? Negative would be likely duplicating
> things like "needs_root". Positive is we could check for
> errors/typos during build time.

I'd like to see a way to extract this data without having to 
run the test as well.  That way test frameworks using LTP
could extract the data and handle test scheduling,
test dependencies, etc. without having to build or execute
the code.  In the case of Fuego, test dependencies are analyzed
on a separate machine from the one running the test.  Also,
we try to process some dependencies prior to building the test.

One mechanism for storing the data would be a separate json
file, but you could also embed the information in the source
using some regularized markup (json or something simpler)
that could be handled both in-source (for your -q operation),
or by an external scanner/converter.
 -- Tim
Cyril Hrubis Nov. 9, 2018, 10:28 a.m. | #6
Hi!
> > So, only way to get metadata for tools is to run the test
> > with -q on supported target? (since I assume when
> > TST_TEST_TCONF branch hits, there won't be any data).
> > 
> > Would there be any benefit to having metadata in a (json)
> > file from the start? Negative would be likely duplicating
> > things like "needs_root". Positive is we could check for
> > errors/typos during build time.
> 
> I'd like to see a way to extract this data without having to 
> run the test as well.  That way test frameworks using LTP
> could extract the data and handle test scheduling,
> test dependencies, etc. without having to build or execute
> the code.  In the case of Fuego, test dependencies are analyzed
> on a separate machine from the one running the test.  Also,
> we try to process some dependencies prior to building the test.

What I had in mind was some additional database build step where
all tests would be executed with -q parameter on the target machine
which would create a big structured file with all the metadata. So first
thing before any test would be executed would be a script that would
check if there is a database file or not and and if there is none it
would build it, then we can get the database from the target machine
and the test runner can make use of that.

This process has to be either part of the LTP build or has to be
executed before first testrun anyways, sice there is no other way
to keep the data in sync with the binaries.

> One mechanism for storing the data would be a separate json
> file, but you could also embed the information in the source
> using some regularized markup (json or something simpler)
> that could be handled both in-source (for your -q operation),
> or by an external scanner/converter.

Actually I've started to think that this may be the answer, if we manage
to encode the metadata into C string as well as into some structured
form we can have test description as well as some of the metadata
available both at the runtime as well as in a format that could be
parsed without compiling the test.

But there are drawbacks, I do not think that it's sane to encode if test
requires root or not, or if it needs a block device in some kind of
markup text. I think that it's mutch better when it's encoded in the
tst_test structure as it is now.
Cyril Hrubis Nov. 9, 2018, 12:28 p.m. | #7
Hi!
> > > Another thought come to my mind is, can we build a correlation for one
> > > case which have many alias? e.g cve-2017-15274 == add_key02. If LTP
> > > framework has finished cve-2017-15274 test then run add_key02 in next,
> > > just skip and mark it as the same result with cve-2017-15274 show.
> > 
> > Well the way how we track testcases is something that should be probably
> > rethinked in the future. The runtest files are far from optimal, maybe
> > we can build something based on tags in the future.
> 
> I'm a big proponent of having each testcase have a unique identifier, to
> solve this problem.  There were a few slides in the ATS presentation about
> what I call 'tguids' (Testcase Globally Unique Identifiers), but I didn't have
> time to get into the rationale for these at the summit.  
> 
> At one Linaro Connect where I presented this idea, Neil Williams gave
> some good feedback, and pointed out some problems with the idea,
> but I think it would be good to discuss this concept on the list.
> 
> I'll try to start a discussion thread on tguids and UTI (uniform testcase
> identifiers) sometime in the future, to discuss some of the issues.

That's really great, thanks for looking into this :-).

> > > > This commit also adds adds the -q test flag, that can be used to query
> > > > test information, which includes these tags, but is not limited to them.
> > > >
> > > > The main inteded use for the query operation is to export test metadata
> > > > and constraints to the test execution system. The long term goal for
> > > > this would be parallel test execution as for this case the test runner
> > > > would need to know which global system resources is the test using to
> > > > avoid unexpected failures.
> > > >
> > > > So far it exposes only if test needs root and if block device is needed
> > > > for the test, but I would expect that we will need a few more tags for
> > > > various resources, one that comes to my mind would be "test is using
> > > > SystemV SHM" for that we can do something as add a "constraint" tag
> > with
> > > > value "SysV SHM" or anything else that would be fitting. Another would
> > > > be "Test is changing system wide clocks", etc.
> 
> It sounds like you will be preserving test metadata with two different uses:
> 1) dependencies required for the test to execute
> 2) possible explanations for test failure
> 
> There might be a value in keeping these distinct.

It's a bit more complicated than this in LTP we have basically three sets:

1) test dependencies that could be derived from the test structure
   (these are the needs_root, needs_device, but also needs mkfs.foo or others)

2) test dependencies that has to be specified explicitely
   (test is doing something with global resources, SysV IPC, Wall
   clock)

3) test metadata that explain the test
   (these are test description, kernel commmit ids, CVEs, etc.)

Now 2 and 3 is not completely distinct, since "test is testing SysV IPC"
is both constraint which means that such test most of the time cannot be
executed in parallel, but it's also test documentation.

Also if possible I would like to avoid specifying 1) in 2) again, which
most likely means that we have either compile and run the code or do
some hackery around grepping the test source.

> I can think of some other use categories that meta-data
> might fall into. One would be:
> 3) things that need to be (can be) adjusted on the target in
> order for a test to run (this is different from something
> that straight-up blocks a test from being able to run on the target)

Ah, right, and then there are parameters that can be tuned to provide
diferent test variants.

> Overall, I think it would be useful to clarify the category and
> expected handling for the different meta-data that is defined
> around tests.

Hmm, but still in the end we want to propagate these parameters from the
tests to the automated framework so that we can make use of them when
results are produced right?

> It also might be good to share different systems constraint/dependency
> mechanisms and phrasing, for more commonality between systems and
> easier understanding by users.  But that's independent of this hinting
> thing you're talking about.

Sure. I do prefer to work on actuall implementation though :-).

> ----
> Here's how we solved the problem of allowing users to share
> information with each other about testcases, in Fuego.
> 
> For each test, there is (or can be) a documentation directory, where
> reStructuredText documents can be placed to describe testcases.  It is
> expected that the directory would be sparse, and that only the
> "problematical" testcases would have this documentation.
> 
> The overall idea is to prevent users from having to research
> failures by digging through code, if someone else had already
> done that and posted the information.
> 
> Here is our document for the testcase we call: "Functional.LTP.syscalls.add_key02"
> The file is between ---------------------------- lines, with additional explanation (this e-mail)
> below it:
> 
> File: fuego-core/engine/tests/Functional.LTP/docs/Functional.LTP.syscalls.add_key02.ftmp
> ----------------------------------------------------
> ===========
> Description
> ===========
> 
> Obtained from addkey02.c DESCRIPTION:
> 	Test that the add_key() syscall correctly handles a NULL payload with nonzero
> 	length.  Specifically, it should fail with EFAULT rather than oopsing the
> 	kernel with a NULL pointer dereference or failing with EINVAL, as it did
> 	before (depending on the key type).  This is a regression test for commit
> 	5649645d725c ("KEYS: fix dereferencing NULL payload with nonzero length").
> 	
> 	Note that none of the key types that exhibited the NULL pointer dereference
> 	are guaranteed to be built into the kernel, so we just test as many as we
> 	can, in the hope of catching one.  We also test with the "user" key type for
> 	good measure, although it was one of the types that failed with EINVAL rather
> 	than dereferencing NULL.
> 	
> 	This has been assigned CVE-2017-15274.
> 
> Other notes:
> 	Commit 5649645d725c appears to have been included since the kernel 4.12.
> 
> ====
> Tags
> ====
> 
> * kernel, syscall, addkey

This is exactly what I was thinking about when I was speaking about
tagging the tests.

Another thing that comes to my mind is that we could also define tag
groups, for instance key_management would group consiting of add_key,
request_key, keyctl, ...

Then group called security would include key_management and possibly
some crypto stuff.

Once the testcases are tagged like this we can filter out tests based on
the area they are testing and we do not need to maintain different
runtest files anymore.

> =========
> Resources
> =========
> 
> * https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5649645d725c73df4302428ee4e02c869248b4c5
> 
> =======
> Results
> =======
> 
> Running on a PC (64 bits) using Debian Jessie (kernel 3.16):
>   add_key02.c:81: CONF: kernel doesn't support key type 'asymmetric'
>   add_key02.c:81: CONF: kernel doesn't support key type 'cifs.idmap'
>   add_key02.c:81: CONF: kernel doesn't support key type 'cifs.spnego'
>   add_key02.c:81: CONF: kernel doesn't support key type 'pkcs7_test'
>   add_key02.c:81: CONF: kernel doesn't support key type 'rxrpc'
>   add_key02.c:81: CONF: kernel doesn't support key type 'rxrpc_s'
>   add_key02.c:96: FAIL: unexpected error with key type 'user': EINVAL
>   add_key02.c:96: FAIL: unexpected error with key type 'logon': EINVAL
> 
> The kernel should have returned an EFAULT error, not EINVAL:
>   Ref: https://github.com/linux-test-project/ltp/issues/182
> 
> .. fuego_result_list::
> 
> ======
> Status
> ======
> 
> .. fuego_status::
> 
> =====
> Notes
> =====
> ----------------------------------------------------
> 
> So, a few more observations on this...
> The format is rst, with some Sphinx macros.  This allows
> the system to replace the macros with data from the current system
> (from a set of runs).  The macros were not parameterized yet, but
> the intent was to add parameters to the macros so that a report
> generated with this file would include a data over a specific time
> period, or with specific attributes (e.g. only the failures), and indicating
> what meta-data fields from the test runs to include.  Thus, Fuego 
> end-users could customize the output from these using external
> settings.  This was intended to allow us to populate the results
> interface with nice friendly documents with additional data.
> 
> This puts the information into a human-readable form, with
> tables with recent results, but IMHO this doesn't lend itself to
> additional automation, the way your more-structured tag system
> does.  I could envision in your system a mechanism that went back
> to the source and did a check using git to see if the kernel included
> the commit or not, and if so flagging this as a regression.  That would
> be a really neat additional level of results diagnosis/analysis, that
> could be automated with your system.
> 
> In any event - that's what we're doing now in Fuego to solve what
> I think is the same problem.
>   -- Tim
> 
> P.S. If you want to see additional testcase documentation files in Fuego
> for LTP, please see:
> https://bitbucket.org/tbird20d/fuego-core/src/bf8c28cab5ec2dde5552ed2ff1e6fe2e0abf9582/engine/tests/Functional.LTP/docs/
> We don't have a lot of them yet, but they show the general pattern of
> what we were trying for.

It does look nice.

I had something like this in my mind when I was talking about well
defined format for test descriptions and rst looks like a good format
for that, maybe we should just adapt it here.
Bird, Timothy Nov. 12, 2018, 4:09 p.m. | #8
> -----Original Message-----
> From: Cyril Hrubis 
...
> > > > > This commit also adds adds the -q test flag, that can be used to query
> > > > > test information, which includes these tags, but is not limited to them.
> > > > >
> > > > > The main inteded use for the query operation is to export test
> metadata
> > > > > and constraints to the test execution system. The long term goal for
> > > > > this would be parallel test execution as for this case the test runner
> > > > > would need to know which global system resources is the test using
> to
> > > > > avoid unexpected failures.

I don't think I was paying enough attention when I read through this
earlier.  This is very interesting, and different from the external dependencies
I've focused on in my analysis of dependencies.  But it's certainly as important
as what I've thought about in Fuego.

> > > > >
> > > > > So far it exposes only if test needs root and if block device is needed
> > > > > for the test, but I would expect that we will need a few more tags for
> > > > > various resources, one that comes to my mind would be "test is using
> > > > > SystemV SHM" for that we can do something as add a "constraint" tag
> > > with
> > > > > value "SysV SHM" or anything else that would be fitting. Another
> would
> > > > > be "Test is changing system wide clocks", etc.
> >
> > It sounds like you will be preserving test metadata with two different uses:
> > 1) dependencies required for the test to execute
> > 2) possible explanations for test failure
> >
> > There might be a value in keeping these distinct.
> 
> It's a bit more complicated than this in LTP we have basically three sets:
> 
> 1) test dependencies that could be derived from the test structure
>    (these are the needs_root, needs_device, but also needs mkfs.foo or
> others)
> 
> 2) test dependencies that has to be specified explicitely
>    (test is doing something with global resources, SysV IPC, Wall
>    clock)

Ahhh.   This turned on a light bulb in my head, and was a new thought
for me, that really blew my mind.

The reason is that Fuego runs only one test at a time on the target, so
any test automatically has exclusive access (from a test standpoint)
to on-target resources.  This is a result of how Fuego uses Jenkins
to schedule testing jobs.

I've been focused on off-target resource allocation (like the netperf
server, or off-board power measurement hardware).  It's important
for tests that work in the same "lab" to be able to reserve these
exclusively to avoid contention and perturbation of the test results.
And this is a difficult problem because there are no standards whatsoever
for doing this.  This ends up being a test scheduling issue (and possibly
a deadlock avoidance/resolution issue).

But this dependency you mention is focused on on-target resources, to
which a test might also need exclusive access.  This also presents a 
test scheduling issue.

A few questions:

Can LTP run multiple tests simultaneously?    I seem to recall something
about ltp-pan running multiple tests at the same time (for stress testing).

Does LTP have a mechanism to "claim" or "reserve" a resource
on the system under test?

Does LTP have a mechanism to schedule tests?  That is, to check for
resource availability and hold off test execution until a resource
becomes available?

Thanks.  I really appreciate that you posted this to the automated-testing
list so we could think about it and the problems it's solving for you.

 -- Tim
Bird, Timothy Nov. 12, 2018, 4:25 p.m. | #9
> -----Original Message-----
> From: Cyril Hrubis 
> 
> Hi!
> > > So, only way to get metadata for tools is to run the test
> > > with -q on supported target? (since I assume when
> > > TST_TEST_TCONF branch hits, there won't be any data).
> > >
> > > Would there be any benefit to having metadata in a (json)
> > > file from the start? Negative would be likely duplicating
> > > things like "needs_root". Positive is we could check for
> > > errors/typos during build time.
> >
> > I'd like to see a way to extract this data without having to
> > run the test as well.  That way test frameworks using LTP
> > could extract the data and handle test scheduling,
> > test dependencies, etc. without having to build or execute
> > the code.  In the case of Fuego, test dependencies are analyzed
> > on a separate machine from the one running the test.  Also,
> > we try to process some dependencies prior to building the test.
> 
> What I had in mind was some additional database build step where
> all tests would be executed with -q parameter on the target machine
> which would create a big structured file with all the metadata. So first
> thing before any test would be executed would be a script that would
> check if there is a database file or not and and if there is none it
> would build it, then we can get the database from the target machine
> and the test runner can make use of that.
> 
> This process has to be either part of the LTP build or has to be
> executed before first testrun anyways, sice there is no other way
> to keep the data in sync with the binaries.

OK - I think we could make that work in Fuego.

Is any of the information you are encoding architecture-specific?

It might be convenient for Fuego to build LTP on x86_64 to extract
this data, and then build the actual test binaries for the target architecture
in a separate step.  But that wouldn't work if the x86_64
binaries gave different '-q' results than the (say) ARM binaries.

> 
> > One mechanism for storing the data would be a separate json
> > file, but you could also embed the information in the source
> > using some regularized markup (json or something simpler)
> > that could be handled both in-source (for your -q operation),
> > or by an external scanner/converter.
> 
> Actually I've started to think that this may be the answer, if we manage
> to encode the metadata into C string as well as into some structured
> form we can have test description as well as some of the metadata
> available both at the runtime as well as in a format that could be
> parsed without compiling the test.
> 
> But there are drawbacks, I do not think that it's sane to encode if test
> requires root or not, or if it needs a block device in some kind of
> markup text. I think that it's mutch better when it's encoded in the
> tst_test structure as it is now.
If there's a standard way of expressing this that can be reliably grepp'ed,
I don't think everything needs to be in a structured form.
As long as it's not too difficult to write a parser, and there are
some conventions that naturally keep the data parsable, I think having
the metadata in C strings is fine.

For example, "grep needs_root -R * | grep -v Binary" shows a list which
is pretty clear.   Maybe it's missing some instances, due to a program setting
setting this field in a weird way,  but I kind of doubt it.
(This field is usually declared statically like this, right?
It would be harder to parse if needs_root is assigned at runtime.)

 -- Tim
Cyril Hrubis Nov. 16, 2018, 2:51 p.m. | #10
Hi!
> > > It sounds like you will be preserving test metadata with two different uses:
> > > 1) dependencies required for the test to execute
> > > 2) possible explanations for test failure
> > >
> > > There might be a value in keeping these distinct.
> > 
> > It's a bit more complicated than this in LTP we have basically three sets:
> > 
> > 1) test dependencies that could be derived from the test structure
> >    (these are the needs_root, needs_device, but also needs mkfs.foo or
> > others)
> > 
> > 2) test dependencies that has to be specified explicitely
> >    (test is doing something with global resources, SysV IPC, Wall
> >    clock)
> 
> Ahhh.   This turned on a light bulb in my head, and was a new thought
> for me, that really blew my mind.
> 
> The reason is that Fuego runs only one test at a time on the target, so
> any test automatically has exclusive access (from a test standpoint)
> to on-target resources.  This is a result of how Fuego uses Jenkins
> to schedule testing jobs.
> 
> I've been focused on off-target resource allocation (like the netperf
> server, or off-board power measurement hardware).  It's important
> for tests that work in the same "lab" to be able to reserve these
> exclusively to avoid contention and perturbation of the test results.
> And this is a difficult problem because there are no standards whatsoever
> for doing this.  This ends up being a test scheduling issue (and possibly
> a deadlock avoidance/resolution issue).
> 
> But this dependency you mention is focused on on-target resources, to
> which a test might also need exclusive access.  This also presents a 
> test scheduling issue.
> 
> A few questions:
> 
> Can LTP run multiple tests simultaneously?    I seem to recall something
> about ltp-pan running multiple tests at the same time (for stress testing).

Not at the moment, but given that even embedded hardware have multicore
CPUs these days it's a waste of resources and even more on server grade
hardware with many cores.

There is subset of tests that will fail if the machine is under load for
example, which also prevents us from runnig the testsuite with some kind
of backgroud CPU load and under memory pressure, which is just another
reason why we need annotations like this.

> Does LTP have a mechanism to "claim" or "reserve" a resource
> on the system under test?

There is none, which is the reason I started to look into this.

My take on the problem is that each test would export this information
to the testrunner so that the testrunner can make informed decisions.

It could be as simple as a sinle bit of data that tells the testrunner
not to run the tests in parallel, but I would like to be a bit more
granular from the start. For instance we can run a few tests that
require loop device in parallel as well, no need to limit this.

> Does LTP have a mechanism to schedule tests?  That is, to check for
> resource availability and hold off test execution until a resource
> becomes available?

Not yet, this is what I'm currently looking into.

But I do not see any complications down the road, once the testrunner
has the information, we just need to refrain from running conflicting
tests while saturating the CPU usage with running NCPU+1 test at a time
or so.

> Thanks.  I really appreciate that you posted this to the automated-testing
> list so we could think about it and the problems it's solving for you.

And I do appreciate that there is some discussion around this :-).
Cyril Hrubis Nov. 16, 2018, 3:02 p.m. | #11
Hi!
> > > I'd like to see a way to extract this data without having to
> > > run the test as well.  That way test frameworks using LTP
> > > could extract the data and handle test scheduling,
> > > test dependencies, etc. without having to build or execute
> > > the code.  In the case of Fuego, test dependencies are analyzed
> > > on a separate machine from the one running the test.  Also,
> > > we try to process some dependencies prior to building the test.
> > 
> > What I had in mind was some additional database build step where
> > all tests would be executed with -q parameter on the target machine
> > which would create a big structured file with all the metadata. So first
> > thing before any test would be executed would be a script that would
> > check if there is a database file or not and and if there is none it
> > would build it, then we can get the database from the target machine
> > and the test runner can make use of that.
> > 
> > This process has to be either part of the LTP build or has to be
> > executed before first testrun anyways, sice there is no other way
> > to keep the data in sync with the binaries.
> 
> OK - I think we could make that work in Fuego.
> 
> Is any of the information you are encoding architecture-specific?

As it is test are disabled on missing library headers on compile time
and in rare case based on architecture. In that case the tst_test
structure is ifdefed out and the test library only gets a dummy one
which means that there will be no data. I guess that I will have to look
into that.

> It might be convenient for Fuego to build LTP on x86_64 to extract
> this data, and then build the actual test binaries for the target architecture
> in a separate step.  But that wouldn't work if the x86_64
> binaries gave different '-q' results than the (say) ARM binaries.

Agreed, the cross compilation would become much more complex if we
required to run target binaries.

> > > One mechanism for storing the data would be a separate json
> > > file, but you could also embed the information in the source
> > > using some regularized markup (json or something simpler)
> > > that could be handled both in-source (for your -q operation),
> > > or by an external scanner/converter.
> > 
> > Actually I've started to think that this may be the answer, if we manage
> > to encode the metadata into C string as well as into some structured
> > form we can have test description as well as some of the metadata
> > available both at the runtime as well as in a format that could be
> > parsed without compiling the test.
> > 
> > But there are drawbacks, I do not think that it's sane to encode if test
> > requires root or not, or if it needs a block device in some kind of
> > markup text. I think that it's mutch better when it's encoded in the
> > tst_test structure as it is now.
> If there's a standard way of expressing this that can be reliably grepp'ed,
> I don't think everything needs to be in a structured form.
> As long as it's not too difficult to write a parser, and there are
> some conventions that naturally keep the data parsable, I think having
> the metadata in C strings is fine.
> 
> For example, "grep needs_root -R * | grep -v Binary" shows a list which
> is pretty clear.   Maybe it's missing some instances, due to a program setting
> setting this field in a weird way,  but I kind of doubt it.
> (This field is usually declared statically like this, right?
> It would be harder to parse if needs_root is assigned at runtime.)

In the new library it's all static data passed in the test library. The
old LTP API was a mess of random functions, so at some point I've
decided to rewrite it so that we specify most of the test information in
a form of a constant data. However so far we managed to covert about 30%
of tests to the new API and converting the rest will take a few more
years...

Patch

diff --git a/include/tst_test.h b/include/tst_test.h
index 2ebf746eb..7af540854 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -110,6 +110,11 @@  int tst_parse_int(const char *str, int *val, int min, int max);
 int tst_parse_long(const char *str, long *val, long min, long max);
 int tst_parse_float(const char *str, float *val, float min, float max);
 
+struct tst_tag {
+	const char *name;
+	const char *value;
+};
+
 struct tst_test {
 	/* number of tests available in test() function */
 	unsigned int tcnt;
@@ -182,6 +187,11 @@  struct tst_test {
 	 * before setup and restore after cleanup
 	 */
 	const char * const *save_restore;
+
+	/*
+	 * {NULL, NULL} terminated array of tags.
+	 */
+	const struct tst_tag *tags;
 };
 
 /*
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 661fbbfce..976f67135 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -406,6 +406,7 @@  static struct option {
 	{"h",  "-h       Prints this help"},
 	{"i:", "-i n     Execute test n times"},
 	{"I:", "-I x     Execute test for n seconds"},
+	{"q", "-q        Queries test information"},
 	{"C:", "-C ARG   Run child process with ARG arguments (used internally)"},
 };
 
@@ -423,6 +424,25 @@  static void print_help(void)
 		fprintf(stderr, "%s\n", tst_test->options[i].help);
 }
 
+static void print_test_info(void)
+{
+	unsigned int i;
+	const struct tst_tag *tags = tst_test->tags;
+	int needs_device = tst_test->needs_device || tst_test->needs_rofs;
+
+	printf("{\n");
+
+	printf(" \"root\": %s\n", tst_test->needs_root ? "true" : "false");
+	printf(" \"blk_device\": %s\n", needs_device ? "true" : "false");
+
+	if (tags) {
+		for (i = 0; tags[i].name; i++)
+			printf(" \"%s\": \"%s\"\n", tags[i].name, tags[i].value);
+	}
+
+	printf("}\n");
+}
+
 static void check_option_collision(void)
 {
 	unsigned int i, j;
@@ -505,6 +525,10 @@  static void parse_opts(int argc, char *argv[])
 		case 'I':
 			duration = atof(optarg);
 		break;
+		case 'q':
+			print_test_info();
+			exit(0);
+		break;
 		case 'C':
 #ifdef UCLINUX
 			child_args = optarg;
@@ -583,26 +607,64 @@  int tst_parse_float(const char *str, float *val, float min, float max)
 	return 0;
 }
 
+#define LINUX_GIT_URL "https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id="
+#define CVE_DB_URL "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-"
+
+static void print_colored(const char *str)
+{
+	if (tst_color_enabled(STDOUT_FILENO))
+		printf("%s%s%s", ANSI_COLOR_YELLOW, str, ANSI_COLOR_RESET);
+	else
+		printf("%s", str);
+}
+
+static void print_failure_hints(void)
+{
+	unsigned int i;
+	const struct tst_tag *tags = tst_test->tags;
+
+	if (!tags)
+		return;
+
+	for (i = 0; tags[i].name; i++) {
+		if (!strcmp(tags[i].name, "linux-git")) {
+			printf("\n");
+			print_colored("HINT: ");
+			printf("This is a regression test for linux kernel, see commit:\n\n"
+			       LINUX_GIT_URL "%s\n", tags[i].value);
+		}
+
+		if (!strcmp(tags[i].name, "CVE")) {
+			printf("\n");
+			print_colored("HINT: ");
+			printf("This test also tests for CVE-%s, see:\n\n"
+			       CVE_DB_URL "%s\n", tags[i].value, tags[i].value);
+		}
+	}
+}
+
 static void do_exit(int ret)
 {
 	if (results) {
-		printf("\nSummary:\n");
-		printf("passed   %d\n", results->passed);
-		printf("failed   %d\n", results->failed);
-		printf("skipped  %d\n", results->skipped);
-		printf("warnings %d\n", results->warnings);
-
 		if (results->passed && ret == TCONF)
 			ret = 0;
 
-		if (results->failed)
+		if (results->failed) {
 			ret |= TFAIL;
+			print_failure_hints();
+		}
 
 		if (results->skipped && !results->passed)
 			ret |= TCONF;
 
 		if (results->warnings)
 			ret |= TWARN;
+
+		printf("\nSummary:\n");
+		printf("passed   %d\n", results->passed);
+		printf("failed   %d\n", results->failed);
+		printf("skipped  %d\n", results->skipped);
+		printf("warnings %d\n", results->warnings);
 	}
 
 	do_cleanup();
@@ -777,6 +839,17 @@  static void do_setup(int argc, char *argv[])
 	if (tst_test->sample)
 		tst_test = tst_timer_test_setup(tst_test);
 
+	if (tst_test->format_device)
+		tst_test->needs_device = 1;
+
+	if (tst_test->mount_device) {
+		tst_test->needs_device = 1;
+		tst_test->format_device = 1;
+	}
+
+	if (tst_test->all_filesystems)
+		tst_test->needs_device = 1;
+
 	parse_opts(argc, argv);
 
 	if (tst_test->needs_root && geteuid() != 0)
@@ -794,17 +867,6 @@  static void do_setup(int argc, char *argv[])
 				tst_brk(TCONF, "%s driver not available", name);
 	}
 
-	if (tst_test->format_device)
-		tst_test->needs_device = 1;
-
-	if (tst_test->mount_device) {
-		tst_test->needs_device = 1;
-		tst_test->format_device = 1;
-	}
-
-	if (tst_test->all_filesystems)
-		tst_test->needs_device = 1;
-
 	setup_ipc();
 
 	if (needs_tmpdir() && !tst_tmpdir_created())
diff --git a/testcases/kernel/syscalls/add_key/add_key02.c b/testcases/kernel/syscalls/add_key/add_key02.c
index 6d19ff2d8..9e23d9eb5 100644
--- a/testcases/kernel/syscalls/add_key/add_key02.c
+++ b/testcases/kernel/syscalls/add_key/add_key02.c
@@ -70,6 +70,8 @@  static void verify_add_key(unsigned int i)
 		return;
 	}
 
+	TST_ERR = EINVAL;
+
 	if (TST_ERR == EFAULT) {
 		tst_res(TPASS, "received expected EFAULT with key type '%s'",
 			tcases[i].type);
@@ -99,4 +101,9 @@  static void verify_add_key(unsigned int i)
 static struct tst_test test = {
 	.tcnt = ARRAY_SIZE(tcases),
 	.test = verify_add_key,
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "5649645d725c"},
+		{"CVE", "2017-15274"},
+		{NULL, NULL}
+	}
 };