diff mbox series

[v2,1/2] lib: Add support for test tags

Message ID 20191022140757.29713-2-chrubis@suse.cz
State Accepted
Headers show
Series [v2,1/2] lib: Add support for test tags | expand

Commit Message

Cyril Hrubis Oct. 22, 2019, 2:07 p.m. UTC 
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:

--------------------------------------------------------------------------
snd_timer01.c:134: FAIL: kernel seems vulnerable

HINT: You _MAY_ be missing kernel fixes, see:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d11662f4f798
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ba3021b2c79b

HINT: You _MAY_ be vunerable to CVE(s), see:

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

Summary:
passed   0
failed   1
skipped  0
warnings 0
--------------------------------------------------------------------------

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
Reviewed-by: Petr Vorel <pvorel@suse.cz>

---
 include/tst_test.h | 10 +++++
 lib/tst_test.c     | 91 ++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 94 insertions(+), 7 deletions(-)

Comments

Petr Vorel Oct. 22, 2019, 2:19 p.m. UTC | #1 
Hi Cyril,

> +	fprintf(stderr, "Options\n");
> +	fprintf(stderr, "-------\n\n");
I still would remove this extra new line:
	fprintf(stderr, "-------\n");
> +
>  	for (i = 0; i < ARRAY_SIZE(options); i++)
>  		fprintf(stderr, "%s\n", options[i].help);

> @@ -424,6 +430,28 @@ static void print_help(void)
>  		fprintf(stderr, "%s\n", tst_test->options[i].help);
>  }

> +static void print_test_tags(void)
> +{
> +	unsigned int i;
> +	const struct tst_tag *tags = tst_test->tags;
> +
> +	printf("\nTags\n");
> +	printf("----\n\n");
And here:
	printf("----\n");

Otherwise LGTM.

Kind regards,
Petr
Li Wang Oct. 23, 2019, 7:39 a.m. UTC | #2 
Cyril Hrubis <chrubis@suse.cz> wrote:

...
> +static void print_failure_hints(void)
> +{
> +       unsigned int i;
> +       const struct tst_tag *tags = tst_test->tags;
> +
> +       if (!tags)
> +               return;
> +
> +       int hint_printed = 0;
> +       for (i = 0; tags[i].name; i++) {
> +               if (!strcmp(tags[i].name, "linux-git")) {
> +                       if (!hint_printed) {
> +                               hint_printed = 1;
> +                               printf("\n");
> +                               print_colored("HINT: ");
> +                               printf("You _MAY_ be missing kernel fixes,
> see:\n\n");
> +                       }
> +
> +                       printf(LINUX_GIT_URL "%s\n", tags[i].value);
> +               }
> +
> +       }
> +
> +       hint_printed = 0;
> +       for (i = 0; tags[i].name; i++) {
> +               if (!strcmp(tags[i].name, "CVE")) {
>

Maybe we can merge this string compare in the same for loops?

e.g: http://pastebin.test.redhat.com/808028
Jan Stancek Oct. 23, 2019, 7:58 a.m. UTC | #3 
----- Original Message -----
> Maybe we can merge this string compare in the same for loops?
> 
> e.g: http://pastebin.test.redhat.com/808028

Li, I don't think this is public server.
Li Wang Oct. 23, 2019, 8:06 a.m. UTC | #4 
On Wed, Oct 23, 2019 at 3:58 PM Jan Stancek <jstancek@redhat.com> wrote:

>
> ----- Original Message -----
> > Maybe we can merge this string compare in the same for loops?
> >
> > e.g: http://pastebin.test.redhat.com/808028
>
> Li, I don't think this is public server.
>

Sorry, thanks for the reminder.
[I copied the below code to gmail, it always lost the indent]

static void print_failure_hints(void)
{
        unsigned int i;
        int hint_printed = 0;
        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")) {
                        if (hint_printed != 1) {
                                hint_printed = 1;
                                print_colored("\nHINT: ");
                                printf("You _MAY_ be missing kernel fixes,
see:\n\n");
                        }

                        printf(LINUX_GIT_URL "%s\n", tags[i].value);
                } else if (!strcmp(tags[i].name, "CVE")) {
                         if (hint_printed != 2) {
                                 hint_printed = 2;
                                 print_colored("\nHINT: ");
                                 printf("You _MAY_ be vunerable to CVE(s),
see:\n\n");
                          }

                        printf(CVE_DB_URL "%s\n", tags[i].value);
                } else {
                        print_colored("\nERROR: ");
                        printf("tags[%d].name should be linux-git or
CVE\n", i);
                        return;
                }
        }
}
Cyril Hrubis Oct. 23, 2019, 4:06 p.m. UTC | #5 
Hi!
> Sorry, thanks for the reminder.
> [I copied the below code to gmail, it always lost the indent]
> 
> static void print_failure_hints(void)
> {
>         unsigned int i;
>         int hint_printed = 0;
>         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")) {
>                         if (hint_printed != 1) {
>                                 hint_printed = 1;
>                                 print_colored("\nHINT: ");
>                                 printf("You _MAY_ be missing kernel fixes,
> see:\n\n");
>                         }
> 
>                         printf(LINUX_GIT_URL "%s\n", tags[i].value);
>                 } else if (!strcmp(tags[i].name, "CVE")) {
>                          if (hint_printed != 2) {
>                                  hint_printed = 2;
>                                  print_colored("\nHINT: ");
>                                  printf("You _MAY_ be vunerable to CVE(s),
> see:\n\n");
>                           }
> 
>                         printf(CVE_DB_URL "%s\n", tags[i].value);

This would produce intermixed CVE and linux-git lines unless the tags
are sorted correctly in the source code, I do not want to depend on the
order hence the two loops.

>                 } else {
>                         print_colored("\nERROR: ");
>                         printf("tags[%d].name should be linux-git or
> CVE\n", i);
>                         return;

I've tried to explain to pvorel already that this is a wrong place to
assert the tag names. If nothing else this piece of code will be rarely
triggered and the error would end up ignored.

I plan to assert the tag names in the docparse tool that will build the
test metadata during the LTP build, so that wrong metadata will actually
fail the LTP build.

>                 }
>         }
> }
Li Wang Oct. 24, 2019, 2:45 a.m. UTC | #6 
On Thu, Oct 24, 2019 at 12:06 AM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > Sorry, thanks for the reminder.
> > [I copied the below code to gmail, it always lost the indent]
> >
> > static void print_failure_hints(void)
> > {
> >         unsigned int i;
> >         int hint_printed = 0;
> >         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")) {
> >                         if (hint_printed != 1) {
> >                                 hint_printed = 1;
> >                                 print_colored("\nHINT: ");
> >                                 printf("You _MAY_ be missing kernel
> fixes,
> > see:\n\n");
> >                         }
> >
> >                         printf(LINUX_GIT_URL "%s\n", tags[i].value);
> >                 } else if (!strcmp(tags[i].name, "CVE")) {
> >                          if (hint_printed != 2) {
> >                                  hint_printed = 2;
> >                                  print_colored("\nHINT: ");
> >                                  printf("You _MAY_ be vunerable to
> CVE(s),
> > see:\n\n");
> >                           }
> >
> >                         printf(CVE_DB_URL "%s\n", tags[i].value);
>
> This would produce intermixed CVE and linux-git lines unless the tags
> are sorted correctly in the source code, I do not want to depend on the
> order hence the two loops.
>

Yes. But personally I suggest the tags sorted in order. I'm OK with two
loops, it depends on you.


>
> >                 } else {
> >                         print_colored("\nERROR: ");
> >                         printf("tags[%d].name should be linux-git or
> > CVE\n", i);
> >                         return;
>
> I've tried to explain to pvorel already that this is a wrong place to
> assert the tag names. If nothing else this piece of code will be rarely
> triggered and the error would end up ignored.
>

Agree, it seems like too late to see this error in a test fails.


>
> I plan to assert the tag names in the docparse tool that will build the
> test metadata during the LTP build, so that wrong metadata will actually
> fail the LTP build.
>

Cool~ That sounds better.

Reviewed-by: Li Wang <liwang@redhat.com>
Cyril Hrubis Nov. 6, 2019, 3:26 p.m. UTC | #7 
Hi!
> > +	fprintf(stderr, "Options\n");
> > +	fprintf(stderr, "-------\n\n");
> I still would remove this extra new line:
> 	fprintf(stderr, "-------\n");
> > +
> >  	for (i = 0; i < ARRAY_SIZE(options); i++)
> >  		fprintf(stderr, "%s\n", options[i].help);
> 
> > @@ -424,6 +430,28 @@ static void print_help(void)
> >  		fprintf(stderr, "%s\n", tst_test->options[i].help);
> >  }
> 
> > +static void print_test_tags(void)
> > +{
> > +	unsigned int i;
> > +	const struct tst_tag *tags = tst_test->tags;
> > +
> > +	printf("\nTags\n");
> > +	printf("----\n\n");
> And here:
> 	printf("----\n");

Changed as you suggested and pushed whole patchset, many thanks for
reviews.
diff mbox series

Patch

diff --git a/include/tst_test.h b/include/tst_test.h
index 84acf2c59..4a51b6d16 100644
--- a/include/tst_test.h
+++ b/include/tst_test.h
@@ -108,6 +108,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;
+};
+
 extern unsigned int tst_variant;
 
 struct tst_test {
@@ -212,6 +217,11 @@  struct tst_test {
 	 * NULL-terminated array of capability settings
 	 */
 	struct tst_cap *caps;
+
+	/*
+	 * {NULL, NULL} terminated array of tags.
+	 */
+	const struct tst_tag *tags;
 };
 
 /*
diff --git a/lib/tst_test.c b/lib/tst_test.c
index 6239acf89..639939ca0 100644
--- a/lib/tst_test.c
+++ b/lib/tst_test.c
@@ -31,6 +31,9 @@ 
 #include "old_device.h"
 #include "old_tmpdir.h"
 
+#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-"
+
 struct tst_test *tst_test;
 
 static const char *tid;
@@ -414,6 +417,9 @@  static void print_help(void)
 {
 	unsigned int i;
 
+	fprintf(stderr, "Options\n");
+	fprintf(stderr, "-------\n\n");
+
 	for (i = 0; i < ARRAY_SIZE(options); i++)
 		fprintf(stderr, "%s\n", options[i].help);
 
@@ -424,6 +430,28 @@  static void print_help(void)
 		fprintf(stderr, "%s\n", tst_test->options[i].help);
 }
 
+static void print_test_tags(void)
+{
+	unsigned int i;
+	const struct tst_tag *tags = tst_test->tags;
+
+	printf("\nTags\n");
+	printf("----\n\n");
+
+	if (tags) {
+		for (i = 0; tags[i].name; i++) {
+			if (!strcmp(tags[i].name, "CVE"))
+				printf(CVE_DB_URL "%s\n", tags[i].value);
+			else if (!strcmp(tags[i].name, "linux-git"))
+				printf(LINUX_GIT_URL "%s\n", tags[i].value);
+			else
+				printf("%s: %s\n", tags[i].name, tags[i].value);
+		}
+	}
+
+	printf("\n");
+}
+
 static void check_option_collision(void)
 {
 	unsigned int i, j;
@@ -499,6 +527,7 @@  static void parse_opts(int argc, char *argv[])
 		break;
 		case 'h':
 			print_help();
+			print_test_tags();
 			exit(0);
 		case 'i':
 			iterations = atoi(optarg);
@@ -584,26 +613,74 @@  int tst_parse_float(const char *str, float *val, float min, float max)
 	return 0;
 }
 
+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;
+
+	int hint_printed = 0;
+	for (i = 0; tags[i].name; i++) {
+		if (!strcmp(tags[i].name, "linux-git")) {
+			if (!hint_printed) {
+				hint_printed = 1;
+				printf("\n");
+				print_colored("HINT: ");
+				printf("You _MAY_ be missing kernel fixes, see:\n\n");
+			}
+
+			printf(LINUX_GIT_URL "%s\n", tags[i].value);
+		}
+
+	}
+
+	hint_printed = 0;
+	for (i = 0; tags[i].name; i++) {
+		if (!strcmp(tags[i].name, "CVE")) {
+			if (!hint_printed) {
+				hint_printed = 1;
+				printf("\n");
+				print_colored("HINT: ");
+				printf("You _MAY_ be vunerable to CVE(s), see:\n\n");
+			}
+
+			printf(CVE_DB_URL "%s\n", 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();