[igt-dev] [PATCH i-g-t 3/4] lib/igt_kmod: add compatibility for KUnit
Janusz Krzysztofik
janusz.krzysztofik at linux.intel.com
Fri Mar 3 14:51:56 UTC 2023
On Friday, 3 March 2023 13:23:35 CET Petri Latvala wrote:
> On Fri, Mar 03, 2023 at 12:07:14PM +0100, Dominik Karol Piatkowski wrote:
> > From: Isabella Basso <isabbasso at riseup.net>
> >
> > 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
> >
> > v1 -> v2:
> > - refactor igt_kunit function and ktap parser so that we have only one
> > parser that we call only once (code size is now less than half the
> > size as v1)
> > - add lookup_value helper
> > - fix parsing problems
> > v2 -> v3:
> > - move ktap parsing functions to own file
> > - rename to ktap_parser
> > - get rid of unneeded pointers in igt_kunit
> > - change return values to allow for subsequent call to igt_kselftests if
> > needed
> > - add docs to parsing functions and helpers
> > - switch to line buffering
> > - add line buffering logging helper
> > - fix kunit module handling
> > - fix parsing of version lines
> > - use igt_subtest blocks to improve output handling on the CI
> > - fix output handling during crashes
> >
> > Signed-off-by: Isabella Basso <isabbasso at riseup.net>
> >
> > v3 -> v4:
> > - handle igt_ktap_parser fail with IGT_EXIT_ABORT code
> >
> > Co-authored-by: Dominik Karol Piątkowski <dominik.karol.piatkowski at intel.com>
> > ---
> > lib/igt_kmod.c | 84 ++++++++++++
> > lib/igt_kmod.h | 2 +
> > lib/igt_ktap.c | 334 ++++++++++++++++++++++++++++++++++++++++++++++++
> > lib/igt_ktap.h | 31 +++++
> > lib/meson.build | 1 +
> > 5 files changed, 452 insertions(+)
> > create mode 100644 lib/igt_ktap.c
> > create mode 100644 lib/igt_ktap.h
> >
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index fbda1aa6..a156e43d 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -29,6 +29,7 @@
> > #include "igt_aux.h"
> > #include "igt_core.h"
> > #include "igt_kmod.h"
> > +#include "igt_ktap.h"
> > #include "igt_sysfs.h"
> > #include "igt_taints.h"
> >
> > @@ -744,6 +745,89 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
> > kmod_module_info_free_list(pre);
> > }
> >
> > +/**
> > + * igt_kunit:
> > + * @module_name: the name of the module
> > + * @opts: options to load the module
> > + *
> > + * Loads the test module, parses its (k)tap dmesg output, then unloads it
> > + *
> > + * Returns: IGT default codes
> > + */
> > +int igt_kunit(const char *module_name, const char *opts)
> > +{
> > + struct igt_ktest tst;
> > + struct kmod_module *kunit_kmod;
> > + char record[BUF_LEN + 1];
> > + FILE *f;
> > + bool is_builtin;
> > + int ret;
> > +
> > + ret = IGT_EXIT_INVALID;
> > +
> > + /* get normalized module name */
> > + if (igt_ktest_init(&tst, module_name) != 0) {
> > + igt_warn("Unable to initialize ktest for %s\n", module_name);
> > + return ret;
> > + }
> > +
> > + if (igt_ktest_begin(&tst) != 0) {
> > + igt_warn("Unable to begin ktest for %s\n", module_name);
> > +
> > + igt_ktest_fini(&tst);
> > + return ret;
> > + }
> > +
> > + 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;
> > + }
> > +
> > + f = fdopen(tst.kmsg, "r");
> > +
> > + if (f == NULL) {
> > + igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer");
> > + goto unload;
> > + }
> > +
> > + if (setvbuf(f, record, _IOLBF, BUF_LEN)) {
> > + igt_warn("Could not set line buffering on /dev/kmsg");
> > + goto unload;
> > + }
>
> Make sure all your igt_warn()s end with a newline.
>
> There seems to be some kind of misunderstanding with how setvbuf
> works. Here a buffer is given to setvbuf() so that fgets() in the
> parser can read one line at a time, but then that's also reading into
> the same buffer. According to setvbuf() docs you're not supposed to
> touch the buffer yourself.
>
> I vaguely remember someone suggesting line-buffered IO for reading
> /dev/kmsg but can't find that message now. What was the reason for it?
> Using read() on /dev/kmsg already gives only one log record at a time.
That was me, in reaction to previous implementation, based on read(), but not
looking good to me, see my comments to
https://patchwork.freedesktop.org/patch/499945/?series=105294&rev=2
I had no idea that read() on /dev/kmsg behaves so friendly.
...
> > +
> > +/**
> > + * igt_ktap_parser:
> > + * @fp: FILE pointer
> > + * @record: buffer used to read fp
> > + * @is_builtin: whether the KUnit module is built-in or not
> > + *
> > + * This function parses the output of a ktap script and prints the test results,
> > + * as well as any other output to stdout.
> > + *
> > + * Returns: IGT default codes
> > + */
> > +int igt_ktap_parser(FILE *fp, char *record, bool is_builtin)
> > +{
> > + char test_name[BUF_LEN + 1];
> > + bool failed_tests, found_tests;
> > + int sublevel = 0;
> > +
> > + test_name[0] = '\0';
> > + test_name[BUF_LEN] = '\0';
> > +
> > + failed_tests = false;
> > + found_tests = false;
> > +
> > + while (sublevel >= 0) {
> > + if (fgets(record, BUF_LEN, fp) == NULL) {
> > + if (!found_tests)
> > + igt_warn("kmsg truncated: unknown error (%m)\n");
> > + break;
> > + }
> > +
> > + switch (find_next_tap_subtest(fp, record, is_builtin)) {
> > + case -2:
> > + /* no more data to read */
> > + return IGT_EXIT_FAILURE;
> > + case -1:
> > + /* no test found, so we keep parsing */
> > + break;
> > + case 0:
> > + /*
> > + * tests found, but they're missing info, so we might
> > + * have read into test output
> > + */
> > + found_tests = true;
> > + sublevel++;
> > + break;
> > + default:
> > + if (fgets(record, BUF_LEN, fp) == NULL) {
> > + igt_warn("kmsg truncated: unknown error (%m)\n");
> > + return -2;
> > + }
> > + found_tests = true;
> > + sublevel++;
> > + break;
> > + }
> > +
> > + switch (parse_kmsg_for_tap(fp, record, test_name)) {
> > + case -2:
> > + return IGT_EXIT_FAILURE;
> > + case -1:
> > + sublevel--;
> > + failed_tests = true;
> > + igt_subtest(test_name)
> > + igt_fail(IGT_EXIT_FAILURE);
> > + test_name[0] = '\0';
> > + break;
> > + case 0: /* fallthrough */
> > + igt_subtest(test_name)
> > + igt_success();
> > + test_name[0] = '\0';
> > + default:
> > + break;
> > + }
> > + }
> > +
> > + if (failed_tests || !found_tests)
> > + return IGT_EXIT_FAILURE;
> > +
> > + return IGT_EXIT_SUCCESS;
I think that instead of returning just one total result, the ktap parser could
better be a background process providing us with results of every subtest it
sees being executed, which we then read and handle on the fly like if those
were IGT dynamic sub-subtests we execute, and their results.
Thanks,
Janusz
> > +}
> > diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
> > new file mode 100644
> > index 00000000..b2f69df2
> > --- /dev/null
> > +++ b/lib/igt_ktap.h
> > @@ -0,0 +1,31 @@
> > +/*
> > + * Copyright © 2022 Isabella Basso do Amaral <isabbasso at riseup.net>
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef IGT_KTAP_H
> > +#define IGT_KTAP_H
> > +
> > +#define BUF_LEN 4096
> > +
> > +int igt_ktap_parser(FILE *fp, char *record, bool is_builtin);
> > +
> > +#endif /* IGT_KTAP_H */
> > diff --git a/lib/meson.build b/lib/meson.build
> > index c5131d9a..58cd6f44 100644
> > --- a/lib/meson.build
> > +++ b/lib/meson.build
> > @@ -87,6 +87,7 @@ lib_sources = [
> > 'igt_store.c',
> > 'uwildmat/uwildmat.c',
> > 'igt_kmod.c',
> > + 'igt_ktap.c',
> > 'igt_panfrost.c',
> > 'igt_v3d.c',
> > 'igt_vc4.c',
>
More information about the igt-dev
mailing list