[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);+ }--- 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/
> +