[PATCH v3 1/2] igt_kmod: add compatibility for KUnit
Export this patch
This adds functions for both executing the tests as well as parsing TAP
kmsg output.
Please, expand your commit message a little bit more. For example, many
people might not know what TAP is. Also, add some examples.
Changes since v1:
- Correct handling of read to deal with partial buffer.
When you send this patch to the upstream, you can drop this part because
this is a new patch for the upstream.
Signed-off-by: Isabella Basso <isabbasso@riseup.net>
---
lib/igt_kmod.c | 188 +++++++++++++++++++++++++++++++++++++++++++++++++
lib/igt_kmod.h | 13 ++++
2 files changed, 201 insertions(+)
diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index cf7a3b22..14f1939d 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -345,6 +345,194 @@ igt_kmod_list_loaded(void)
kmod_module_unref_list(list);
}
+ static void kmsg_parse_tap(int fd)
I know this is a static function, and we usually don't document it.
However, this function does many things that imho deserve a brief
explanation to set up the developer's mind to reading this code.
+ {
+ char record[4096 + 1];
+ const char *num_tests_str, *nok_str, *ok_str, *end_str_success, *end_str_fail;
Let's use better variable names here, something like:
nok_str -> not_ok_str
end_str_success -> end_of_the_test_success
end_str_fail -> end_of_the_test_fail
You don't need to use those suggested names, just use something more
meaningful.
+ bool has_num = false;
+
+ num_tests_str = "..";
+ nok_str = " not ok ";
+ ok_str = " ok ";
Do you know if we have a link from the spec that says these spaces are
part of the TAP specification? If so, let's add the link here.
+ end_str_fail = "not ok ";
+ end_str_success = "ok ";
I thought this part was a bit confusing the first time I read it. We
have the same string set for the substest pass and the complete test
pass. Maybe new variable names can mitigate this issue, and a comment or
two can also help.
+
+ if (fd == -1) {
+ igt_warn("Unable to retrieve kernel log (from /dev/kmsg)\n");
+ return;
+ }
I think it is okay to check this, but since this function returns
nothing, we should expect to validate the fd before calling this
function.
+
+ record[sizeof(record) - 1] = '\0';
+
+ while (true) {
+ const char *end, *result;
+ ssize_t r;
Let's call it ret.
+ bool skip = true;
+
+ if (!skip) {
Please, correct me if I'm wrong, but I think we never enter into this if
condition because you set `skip = true` before. Or did I miss something?
+ end = strchrnul(record, '\n');
+ } else {
+ end = NULL;
+ skip = false;
+ }
+
+ r = read(fd, record,
+ end != NULL ?
+ (int) (end - record) :
+ sizeof(record) - 1);
Can we create a variable that will have the result of `end != NULL ?
(int) (end - record) : sizeof(record) - 1)` and just use it here?
+
+ if (r <= 0) {
+ if (errno == EINTR)
+ continue;
Do we really want to continue in this case?
+
+ if (errno == EPIPE) {
+ igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
+ continue;
+ }
+
+ if (errno != EAGAIN)
+ igt_warn("kmsg truncated: unknown error (%m)\n");
+
+ break;
+ }
+
+ if (!has_num) {
Maybe we can use a variable name that better describe this number?
+ result = strstr(record, num_tests_str);
+ if (result) {
+ result += strlen(num_tests_str);
+ end = strchrnul(result, '\n');
+ /* in case line > 4096 */
+ if (end == NULL) {
You don't need `{` and `}` in this if.
+ igt_info("%s\n", record);
+ }
+ else {
+ igt_info("[IGT] running %.*s tests...\n",
+ (int)(end - result), result);
+ }
+ has_num = true;
+ }
+ continue;
+ }
+
+ /* all results have 'ok' in them */
+ result = strstr(record, "ok");
+
+ if (result) {
+ end = strchrnul(result, '\n');
+ /* in case line > 4096 */
+ if (end == NULL) {
+ igt_info("%s\n", record);
+ continue;
+ }
+
+ /* subtest succeded (i.e. ok_str) */
+ result = strstr(record, ok_str);
+
+ if (result) {
+ result += strlen(ok_str);
+ igt_info("[IGT] SUBTEST %.*s\n",
+ (int)(end - result), result);
+ continue;
+ }
+
+ /* subtest failed (i.e. nok_str) */
How about creating a function for handling the pass and failed cases
instead of keeping these codes here?
+ result = strstr(record, nok_str);
+
+ if (result) {
+ result += strlen(nok_str);
+ igt_warn("[IGT] SUBTEST FAILED %.*s\n",
+ (int)(end - result), result);
+ continue;
+ }
+
+ /* test ended (i.e. end_str_success) */
+ /* we should skip the ; in the beginning of the string */
+ result = strchr(record, ';') + 1;
+
+ if (strncmp(result, end_str_success, strlen(end_str_success)) == 0) {
+ igt_info("[IGT] TEST SUCCEEDED %.*s\n",
+ (int)(end - result), result);
+
+ igt_success();
+ }
+
+ if (strncmp(result, end_str_fail, strlen(end_str_fail)) == 0) {
+ igt_warn("[IGT] TEST FAILED %.*s\n",
+ (int)(end - result), result);
+ igt_fail(IGT_EXIT_FAILURE);
+ }
+ }
+ skip = true;
+ }
+ }
+
+ /**
+ * igt_kunit: tests a kunit module
/:/ -/
+ * @mod_name: the name of the module
+ * @options: options to load the module
+ * @req_mods: array with additional dependencies to run the KUnit test
+ * @req_mod_len: size of req_mods array
+ *
+ * Loads the kunit module, parses its dmesg output, then unloads it
+ */
+ void igt_kunit(const char *mod_name, const char *options,
+ const char **req_mods, int req_mod_len)
+ {
+ int kmsg_fd, cmi;
cmi? What is it?
+ struct igt_kselftest tst;
+ const char *norm_name;
Just for curiosity, what does normalize a name mean?
Thanks
Siqueira
+
+ /* get normalized module name */
+ igt_require(igt_kselftest_init(&tst, mod_name) == 0);
+
+ norm_name = kmod_module_get_name(tst.kmod);
+
+ if (!igt_kmod_is_loaded(norm_name)) {
+ /* The kunit module is required for running any kunit tests */
+ if (!igt_kmod_is_loaded("kunit"))
+ igt_require(igt_kmod_load("kunit", NULL) == 0);
+
+ for (cmi = 0; cmi < req_mod_len; cmi++) {
+ if (strcmp(req_mods[cmi], "kunit") == 0)
+ continue;
+ if (igt_kmod_load(req_mods[cmi], NULL))
+ goto unload;
+ }
+ } else {
+ /* Unload module if loaded (maybe previous run bailed out?) */
+ igt_kmod_unload(mod_name, 0);
+ }
+
+ kmsg_fd = open("/dev/kmsg", O_RDONLY | O_NONBLOCK);
+
+ if (kmsg_fd < 0) {
+ igt_warn("Could not open /dev/kmsg");
+ goto unload;
+ }
+
+ if (lseek(kmsg_fd, 0, SEEK_END)) {
+ igt_warn("Could not seek the end of /dev/kmsg");
+ close(kmsg_fd);
+ goto unload;
+ }
+
+ igt_require(igt_kmod_load(mod_name, options) == 0);
+
+ kmsg_parse_tap(kmsg_fd);
+
+ igt_kmod_unload(mod_name, 0);
+
+ close(kmsg_fd);
+ unload:
+ igt_kmod_unload("kunit", 0);
+
+ for (cmi--; cmi >= 0; cmi--) {
+ if (strcmp(req_mods[cmi], "kunit") == 0)
+ continue;
+ igt_kmod_unload(req_mods[cmi], 0);
+ }
+ }
+
/**
* igt_i915_driver_load:
* @opts: options to pass to i915 driver
diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
index 0898122b..60e7a750 100644
--- a/lib/igt_kmod.h
+++ b/lib/igt_kmod.h
@@ -28,6 +28,16 @@
#include "igt_list.h"
+ #define MAX_MOD_NAME_LEN 40
+
+ /**
+ * ARRAY_SIZE:
+ * @arr: static array
+ *
+ * Macro to compute the size of the static array @arr.
+ */
+ #define ARRAY_SIZE(arr) (sizeof(arr)/sizeof(arr[0]))
+
bool igt_kmod_is_loaded(const char *mod_name);
void igt_kmod_list_loaded(void);
@@ -36,6 +46,9 @@ bool igt_kmod_has_param(const char *mod_name, const char *param);
int igt_kmod_load(const char *mod_name, const char *opts);
int igt_kmod_unload(const char *mod_name, unsigned int flags);
+ void igt_kunit(const char *mod_name, const char *options,
+ const char **req_mods, int req_mod_len);
+
int igt_i915_driver_load(const char *opts);
int igt_i915_driver_unload(void);
int __igt_i915_driver_unload(const char **whom);
--
2.35.1
Hi Isabella,
[PATCH v3 2/2] tests/kms_kunit: add KUnit damage helper test
Export this patch
This adds a converted KUnit test module to IGT.
Could you elaborate more about this patch in the commit message?
Signed-off-by: Isabella Basso <isabbasso@riseup.net>
---
tests/kms_kunit.c | 34 ++++++++++++++++++++++++++++++++++
tests/meson.build | 1 +
2 files changed, 35 insertions(+)
create mode 100644 tests/kms_kunit.c
diff --git a/tests/kms_kunit.c b/tests/kms_kunit.c
new file mode 100644
index 00000000..31566910
--- /dev/null
+++ b/tests/kms_kunit.c
@@ -0,0 +1,34 @@
+ /*
+ * Copyright © 2021 Isabella Basso do Amaral <isabbasso@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.
+ */
+
+ #include <libkmod.h>
+
+ #include "igt.h"
+ #include "igt_kmod.h"
+
+ IGT_TEST_DESCRIPTION("KUnit DRM damage helper module testing");
uppose this file will be expanded to all kunit tests, right? If so, I
think you will need a more generic description.
Thanks
> +
+
+ igt_simple_main
+ {
+ igt_kunit("test-drm_damage_helper", NULL, NULL, 0);
+ }
diff --git a/tests/meson.build b/tests/meson.build
index 3dbba7a1..6d3685c3 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -36,6 +36,7 @@ test_progs = [
'kms_hdmi_inject',
'kms_hdr',
'kms_invalid_mode',
+ 'kms_kunit',
'kms_lease',
'kms_multipipe_modeset',
'kms_panel_fitting',
--
2.35.1
21/2022/
> +