Hi, Janusz,
> Am 09/09/2022 um 12:18 PM schrieb Janusz Krzysztofik
> <janusz.krzyszto...@linux.intel.com>:
>
> Hi Isabella,
>
> On Monday, 29 August 2022 02:09:19 CEST Isabella Basso wrote:
>> This adds functions for both executing the tests as well as parsing (K)TAP
>> kmsg output, as per the KTAP spec [1].
>>
>> [1] https://www.kernel.org/doc/html/latest/dev-tools/ktap.html
>>
>> Signed-off-by: Isabella Basso <isabba...@riseup.net>
>> ---
>> lib/igt_kmod.c | 290 +++++++++++++++++++++++++++++++++++++++++++++++++
>> lib/igt_kmod.h | 2 +
>> 2 files changed, 292 insertions(+)
>>
>> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
>> index 97cac7f5..93cdfcc5 100644
>> --- a/lib/igt_kmod.c
>> +++ b/lib/igt_kmod.c
>> @@ -25,6 +25,7 @@
>> #include <signal.h>
>> #include <errno.h>
>> #include <sys/utsname.h>
>> +#include <limits.h>
>>
>> #include "igt_aux.h"
>> #include "igt_core.h"
>> @@ -32,6 +33,8 @@
>> #include "igt_sysfs.h"
>> #include "igt_taints.h"
>>
>> +#define BUF_LEN 4096
>> +
>> /**
>> * SECTION:igt_kmod
>> * @short_description: Wrappers around libkmod for module loading/unloading
>> @@ -713,6 +716,293 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
>> kmod_module_info_free_list(pre);
>> }
>>
>> +/**
>> + * lookup_value:
>> + * @haystack: the string to search in
>> + * @needle: the string to search for
>> + *
>> + * Returns: the value of the needle in the haystack, or -1 if not found.
>> + */
>> +static long lookup_value(const char *haystack, const char *needle)
>> +{
>> + const char *needle_rptr;
>> + char *needle_end;
>> + long num;
>> +
>> + needle_rptr = strcasestr(haystack, needle);
>> +
>> + if (needle_rptr == NULL)
>> + return -1;
>> +
>> + /* skip search string and whitespaces after it */
>> + needle_rptr += strlen(needle);
>> +
>> + num = strtol(needle_rptr, &needle_end, 10);
>> +
>> + if (needle_rptr == needle_end)
>> + return -1;
>> +
>> + if (num == LONG_MIN || num == LONG_MAX)
>> + return 0;
>> +
>> + return num > 0 ? num : 0;
>> +}
>> +
>> +static int find_next_tap_subtest(char *record, char *test_name,
>> + bool is_subtest)
>> +{
>> + const char *name_lookup_str,
>> + *lend, *version_rptr, *name_rptr;
>> + long test_count;
>> +
>> + name_lookup_str = "test: ";
>> +
>> + version_rptr = strcasestr(record, "TAP version ");
>> + name_rptr = strcasestr(record, name_lookup_str);
>> +
>> + /*
>> + * total test count will almost always appear as 0..N at the beginning
>> + * of a run, so we use it as indication of a run
>> + */
>> + test_count = lookup_value(record, "..");
>> +
>> + /* no count found, so this is probably not starting a (sub)test */
>> + if (test_count < 0) {
>> + if (name_rptr != NULL) {
>> + if (test_name[0] == '\0')
>> + strncpy(test_name,
>> + name_rptr + strlen(name_lookup_str),
>> + BUF_LEN);
>> + else if (strcmp(test_name, name_rptr +
>> strlen(name_lookup_str)) == 0)
>> + return 0;
>> + else
>> + test_name[0] = '\0';
>> +
>> + }
>> + return -1;
>> + }
>> +
>> + /*
>> + * "(K)TAP version XX" should be the first line on all (sub)tests as per
>> + *
>> https://www.kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
>> + * but actually isn't, as it currently depends on whoever writes the
>> + * test to print this info
>> + */
>> + if (version_rptr == NULL)
>> + igt_info("Missing test version string\n");
>> +
>> + if (name_rptr == NULL) {
>> + /* we have to keep track of the name string, as it might be
>> + * contained in a line read previously */
>> + if (test_name[0] == '\0') {
>> + igt_info("Missing test name string\n");
>> +
>> + if (is_subtest)
>> + igt_info("Running %ld subtests...\n",
>> test_count);
>> + else
>> + igt_info("Running %ld tests...\n", test_count);
>> + } else {
>> + lend = strchrnul(test_name, '\n');
>> +
>> + if (*lend == '\0') {
>> + if (is_subtest)
>> + igt_info("Executing %ld subtests in:
>> %s\n",
>> + test_count, test_name);
>> + else
>> + igt_info("Executing %ld tests in: %s\n",
>> + test_count, test_name);
>> + return test_count;
>> + }
>> +
>> + if (is_subtest)
>> + igt_info("Executing %ld subtests in: %.*s\n",
>> + test_count, (int)(lend - test_name),
>> + test_name);
>> + else
>> + igt_info("Executing %ld tests in: %.*s\n",
>> + test_count, (int)(lend - test_name),
>> + test_name);
>> + test_name[0] = '\0';
>> + }
>> + } else {
>> + name_rptr += strlen(name_lookup_str);
>> + lend = strchrnul(name_rptr, '\n');
>> + /*
>> + * as the test count comes after the test name we need not check
>> + * for a long line again
>> + */
>> + if (is_subtest)
>> + igt_info("Executing %ld subtests in: %.*s\n",
>> + test_count, (int)(lend - name_rptr),
>> + name_rptr);
>> + else
>> + igt_info("Executing %ld tests in: %.*s\n",
>> + test_count, (int)(lend - name_rptr),
>> + name_rptr);
>> + }
>> +
>> + return test_count;
>> +}
>> +
>> +static void parse_kmsg_for_tap(const char *lstart, char *lend,
>> + int *sublevel, bool *failed_tests)
>> +{
>> + const char *nok_rptr, *comment_start, *value_parse_start;
>> +
>> + nok_rptr = strstr(lstart, "not ok ");
>> + if (nok_rptr != NULL) {
>> + igt_warn("kmsg> %.*s\n",
>> + (int)(lend - lstart), lstart);
>> + *failed_tests = true;
>> + return;
>> + }
>> +
>> + comment_start = strchrnul(lstart, '#');
>> +
>> + /* check if we're still in a subtest */
>> + if (*comment_start != '\0') {
>> + comment_start++;
>> + value_parse_start = comment_start;
>> +
>> + if (lookup_value(value_parse_start, "fail: ") > 0) {
>> + igt_warn("kmsg> %.*s\n",
>> + (int)(lend - comment_start), comment_start);
>> + *failed_tests = true;
>> + (*sublevel)--;
>> + return;
>> + }
>> + }
>> +
>> + igt_info("kmsg> %.*s\n",
>> + (int)(lend - lstart), lstart);
>> +}
>> +
>> +static void igt_kunit_subtests(int fd, char *record,
>> + int *sublevel, bool *failed_tests)
>
> This function looks like nothing but a KTAP parser. It doesn't perform any
> operations required for execution of kunit tests. Shouldn't we better name
> it
> like igt_ktap_parser or something like that? Besides, I would move that
> parser code to a separate source file.
>
>> +{
>> + char test_name[BUF_LEN + 1], *lend;
>> +
>> + lend = NULL;
>> + test_name[0] = '\0';
>> + test_name[BUF_LEN] = '\0';
>> +
>> + while (*sublevel >= 0) {
>> + const char *lstart;
>> + ssize_t r;
>> +
>> + if (lend != NULL && *lend != '\0')
>> + lseek(fd, (int) (lend - record), SEEK_CUR);
>> +
>> + r = read(fd, record, BUF_LEN);
>> +
>> + if (r <= 0) {
>> + switch (errno) {
>> + case EINTR:
>> + continue;
>> + case EPIPE:
>> + igt_warn("kmsg truncated: too many messages. \
>> + You may want to increase log_buf_len \
>> + in your boot options\n");
>> + continue;
>> + case !EAGAIN:
>> + igt_warn("kmsg truncated: unknown error
>> (%m)\n");something
>
> If the intention here is to display this warning only in other cases but
> EAGAIN error then that won't work as intended.
>
>> + *sublevel = -1;
>
> Shouldn't *sublevel be also set to -1 in all cases but EINTR and EPIPE,
> whether EAGAIN or anything else?
>
> Other than that, please use /* fallthrough */ marking if you intentionally
> omit break;
>
>> + default:
>> + break;
>> + }
>> + break;
>> + }
>> +
>> + lend = strchrnul(record, '\n');
>> +
>> + /* in case line > 4096 */
>> + if (*lend == '\0')
>> + continue;
>
> That means we are free to ignore initial fragments of lines exceeding 4096
> characters, but still will be looking at trailing fragments of those lines.
> This approach seems sub-optimal to me. Wouldn't it be more convenient if we
> used line buffered I/O instead of read()?
>
>> +
>> + if (find_next_tap_subtest(record, test_name, *sublevel > 0) !=
>> -1)
>> + (*sublevel)++;
>> +
>> + if (*sublevel > 0) {
>> + lstart = strchrnul(record, ';');
>> +
>> + if (*lstart == '\0') {
>> + igt_warn("kmsg truncated: output malformed
>> (%m)\n");
>> + igt_fail(IGT_EXIT_FAILURE);
>> + }
>> +
>> + lstart++;
>> + while (isspace(*lstart))
>> + lstart++;
>> +
>> + parse_kmsg_for_tap(lstart, lend, sublevel,
>> failed_tests);
>> + }
>> + }
>> +
>> + if (*failed_tests || *sublevel < 0)
>> + igt_fail(IGT_EXIT_FAILURE);
>> + else
>> + igt_success();
>> +}
>> +
>> +/**
>> + * igt_kunit:
>> + * @module_name: the name of the module
>> + * @opts: options to load the module
>> + *
>> + * Loads the kunit module, parses its dmesg output, then unloads it
>> + */
>> +void igt_kunit(const char *module_name, const char *opts)
>> +{
>> + struct igt_ktest tst;
>> + char record[BUF_LEN + 1];
>> + bool failed_tests = false;
>> + int sublevel = 0;
>> +
>> + record[BUF_LEN] = '\0';
>> +
>> + /* get normalized module name */
>> + if (igt_ktest_init(&tst, module_name) != 0) {
>> + igt_warn("Unable to initialize ktest for %s\n", module_name);
>> + return;
>> + }
>> +
>> + if (igt_ktest_begin(&tst) != 0) {
>
> Since igt_ktest_begin() as is calls igt_require() then it may be used only
> from inside an igt_fixture or igt_subtest block. If the intention is to call
> igt_kunit() from an igt_subtest block, unlike igt_kselftest() which has
> igt_fixture and igt_subtest blocks placed in its body, please document that.
>
>> + igt_warn("Unable to begin ktest for %s\n", module_name);
>> +
>> + igt_ktest_fini(&tst);
>> + return;
>> + }
>> +
>> + if (tst.kmsg < 0) {
>> + igt_warn("Could not open /dev/kmsg");
>> + goto unload;
>> + }
>> +
>> + if (lseek(tst.kmsg, 0, SEEK_END)) {
>> + igt_warn("Could not seek the end of /dev/kmsg");
>> + goto unload;
>> + }
>> +
>> + /* The kunit module is required for running any kunit tests */
>> + if (igt_kmod_load("kunit", NULL) != 0) {
>> + igt_warn("Unable to load kunit module\n");
>> + goto unload;
>> + }
>> +
>> + if (igt_kmod_load(module_name, opts) != 0) {
>> + igt_warn("Unable to load %s module\n", module_name);
>> + goto unload;
>> + }
>> +
>> + igt_kunit_subtests(tst.kmsg, record, &sublevel, &failed_tests);
>
> What's the point of passing that many temporary variable pointers to
> igt_kunit_subtests() if we are not interested here in content of any of those
> variables after the function returns? That function could perfectly use its
> own local variables for storing that data.
>
> Besides, my comment about not using igt_require() outside of igt_fixture or
> igt_subtest blocks also applies to igt_fail() and igt_success() called from
> igt_kunit_subtests().
>
> Anyway, related to my comment about naming that function a parser, I think
> the
> best approach would be for that parser to return a generic set of results
> from
> kunit execution, then we could feed that data into an IGT specific handler
> that would convert them to IGT results (SUCCESS, FAIL, or SKIP) as if
> returned
> by a set of IGT dynamic subtests.
That sounds like a good idea to me, I might take some extra time before v3 to
do that, though. I’ll also look into your other suggestions carefully.
Thanks a lot for the review!
Cheers,
--
Isabella Basso
> Thanks,
> Janusz
>
>> +unload:
>> + igt_kmod_unload("kunit", 0);
>> +
>> + igt_ktest_end(&tst);
>> +
>> + igt_ktest_fini(&tst);
>> +}
>> +
>> static int open_parameters(const char *module_name)
>> {
>> char path[256];
>> diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
>> index ceb10cd0..737143c1 100644
>> --- a/lib/igt_kmod.h
>> +++ b/lib/igt_kmod.h
>> @@ -45,6 +45,8 @@ int __igt_i915_driver_unload(char **whom);
>> int igt_amdgpu_driver_load(const char *opts);
>> int igt_amdgpu_driver_unload(void);
>>
>> +void igt_kunit(const char *module_name, const char *opts);
>> +
>> void igt_kselftests(const char *module_name,
>> const char *module_options,
>> const char *result_option,
>>
