| Message ID | 20191014112522.24548-2-chrubis@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for a test tags | expand |
On Mon, Oct 14, 2019 at 7:25 PM Cyril Hrubis <chrubis@suse.cz> wrote: > 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. > Nice to see this feature patchset achieved, it looks quite clear&good to me. Just have some tiny comments below. > > 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 > -------------------------------------------------------------------------- > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > --- > include/tst_test.h | 10 ++++++ > lib/tst_test.c | 77 +++++++++++++++++++++++++++++++++++++++++----- > 2 files changed, 80 insertions(+), 7 deletions(-) > > 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..2129f38cb 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,27 @@ static void print_help(void) > fprintf(stderr, "%s\n", tst_test->options[i].help); > } > > +static void print_test_info(void) > print_test_info sounds like general information for the test, maybe print_tags() is a better/precise name? > +{ > + 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 +526,7 @@ static void parse_opts(int argc, char *argv[]) > break; > case 'h': > print_help(); > + print_test_info(); > exit(0); > case 'i': > iterations = atoi(optarg); > @@ -584,26 +612,61 @@ 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; > + > + 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); > This sentence 'HINT: This is a ...' will be printed many times if there are many commits in tags, I prefer to see only once in front of these linux-kernel links. > + } > + > + 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); > Here as well.
Hello, Li Wang <liwang@redhat.com> writes: >> } >> >> +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" It may be better just to print "Associated Linux kernel commit: ..." and "Associated bug ID: CVE-..." in the loop. Then we can avoid any discussions about what type of test it is. Also, a test description tag could be added if necessary. Maybe it would also be a good idea to link back to the test source code on github? Possibly this info could be injected at build time? >> + LINUX_GIT_URL "%s\n", tags[i].value); >> > > This sentence 'HINT: This is a ...' will be printed many times if there are > many commits in tags, I prefer to see only once in front of these > linux-kernel links. > > >> + } >> + >> + 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); >> > > Here as well. > > -- > Regards, > Li Wang
Hi! > > +static void print_test_info(void) > > > > print_test_info sounds like general information for the test, maybe > print_tags() is a better/precise name? I named it as such in a case that we will add anything else there, but I do not have a strong opinion, I can change that if you insist. > > +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); > > > > This sentence 'HINT: This is a ...' will be printed many times if there are > many commits in tags, I prefer to see only once in front of these > linux-kernel links. Good catch, I will do something about this.
Hi! > It may be better just to print "Associated Linux kernel commit: ..." and > "Associated bug ID: CVE-..." in the loop. Then we can avoid any > discussions about what type of test it is. I will try that. > Also, a test description tag could be added if necessary. My plan is actually to keep the description in the top level comment but parse that by the docparse tool, then we will have a browseable test documentation that could be put to the web and we can point to that here. > Maybe it would also be a good idea to link back to the test source code > on github? Possibly this info could be injected at build time? I tried to inject things at a build time and it wasn't pretty, so I would like to avoid that.
Hi, > + 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")) Not sure if it's worth of defining some enums instead of "CVE", "linux-git" (would catch typos). > + printf(LINUX_GIT_URL "%s\n", tags[i].value); > + else > + printf("%s: %s\n", tags[i].name, tags[i].value); > + printf("\n"); Kind regards, Petr
Hi! > > + 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")) > Not sure if it's worth of defining some enums instead of "CVE", "linux-git" > (would catch typos). I wanted to avoid enums because they are not flexible enough. The plan here is that docparse would do sanity checks on test metadata and fail the compilation if it founds typos there. With the checks in the docparse tool we can easily check for tags that looks like typos, i.e. check the levenshtein distance from all known tags and print a nice looking error message...
Hi Cyril, > > > + 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")) > > Not sure if it's worth of defining some enums instead of "CVE", "linux-git" > > (would catch typos). > I wanted to avoid enums because they are not flexible enough. The plan > here is that docparse would do sanity checks on test metadata and fail > the compilation if it founds typos there. With the checks in the > docparse tool we can easily check for tags that looks like typos, i.e. > check the levenshtein distance from all known tags and print a nice > looking error message... OK, sounds good :). Kind regards, Petr
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..2129f38cb 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,27 @@ 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; + + 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 +526,7 @@ static void parse_opts(int argc, char *argv[]) break; case 'h': print_help(); + print_test_info(); exit(0); case 'i': iterations = atoi(optarg); @@ -584,26 +612,61 @@ 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; + + 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();
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 -------------------------------------------------------------------------- Signed-off-by: Cyril Hrubis <chrubis@suse.cz> --- include/tst_test.h | 10 ++++++ lib/tst_test.c | 77 +++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 80 insertions(+), 7 deletions(-)