[PATCH i-g-t 1/3] igt_hook: Add feature

Gustavo Sousa gustavo.sousa at intel.com
Wed Jun 19 21:12:53 UTC 2024


Quoting Lucas De Marchi (2024-05-21 16:40:56-03:00)
>On Thu, May 09, 2024 at 12:24:29PM GMT, Gustavo Sousa wrote:
>>For development purposes, sometimes it is useful to have a way of
>>running custom scripts at certain points of test executions. A
>>real-world example I bumped into recently is to collect information from
>>sysfs before and after running each entry of a testlist.
>>
>>While it is possible for the user to handcraft a script that calls each
>>test with the correct actions before and after execution, we can provide
>>a better experience by adding built-in support for running hooks during
>>test execution.
>>
>>That would be even better when adding the same kind of support for
>>igt_runner (which is done in an upcoming change), since the user can
>>also nicely resume with igt_resume with the hook already setup in case a
>>crash happens during execution of the test list.
>>
>>As such provide implement support for hooks, integrate it into
>>igt_core and expose the functionality via --hook CLI option on test
>>executables.
>>
>>Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
>>---
>> .../igt-gpu-tools/igt-gpu-tools-docs.xml      |   1 +
>> lib/igt_core.c                                | 116 +++-
>> lib/igt_hook.c                                | 499 ++++++++++++++++++
>> lib/igt_hook.h                                |  86 +++
>> lib/meson.build                               |   1 +
>> lib/tests/igt_hook.c                          | 187 +++++++
>> lib/tests/igt_hook_integration.c              | 297 +++++++++++
>> lib/tests/meson.build                         |   2 +
>> 8 files changed, 1180 insertions(+), 9 deletions(-)
>> create mode 100644 lib/igt_hook.c
>> create mode 100644 lib/igt_hook.h
>> create mode 100644 lib/tests/igt_hook.c
>> create mode 100644 lib/tests/igt_hook_integration.c
>>
>>diff --git a/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml b/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml
>>index 9085eb924e85..11458c68124b 100644
>>--- a/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml
>>+++ b/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml
>>@@ -32,6 +32,7 @@
>>     <xi:include href="xml/igt_fb.xml"/>
>>     <xi:include href="xml/igt_frame.xml"/>
>>     <xi:include href="xml/igt_gt.xml"/>
>>+    <xi:include href="xml/igt_hook.xml"/>
>>     <xi:include href="xml/igt_io.xml"/>
>>     <xi:include href="xml/igt_kmod.xml"/>
>>     <xi:include href="xml/igt_kms.xml"/>
>>diff --git a/lib/igt_core.c b/lib/igt_core.c
>>index 3ff3e0392316..291d891cf884 100644
>>--- a/lib/igt_core.c
>>+++ b/lib/igt_core.c
>>@@ -70,6 +70,7 @@
>>
>> #include "igt_core.h"
>> #include "igt_aux.h"
>>+#include "igt_hook.h"
>> #include "igt_sysfs.h"
>> #include "igt_sysrq.h"
>> #include "igt_rc.h"
>>@@ -241,6 +242,9 @@
>>  * - '*,!basic*' match any subtest not starting basic
>>  * - 'basic*,!basic-render*' match any subtest starting basic but not starting basic-render
>>  *
>>+ * It is possible to run a shell script at certain points of test execution with
>>+ * "--hook". See the usage description with "--help-hook" for details.
>>+ *
>>  * # Configuration
>>  *
>>  * Some of IGT's behavior can be configured through a configuration file.
>>@@ -273,6 +277,8 @@ static unsigned int exit_handler_count;
>> const char *igt_interactive_debug;
>> bool igt_skip_crc_compare;
>>
>>+static struct igt_hook *igt_hook = NULL;
>>+
>> /* subtests helpers */
>> static bool show_testlist = false;
>> static bool list_subtests = false;
>>@@ -338,6 +344,8 @@ enum {
>>         OPT_INTERACTIVE_DEBUG,
>>         OPT_SKIP_CRC,
>>         OPT_TRACE_OOPS,
>>+        OPT_HOOK,
>>+        OPT_HELP_HOOK,
>>         OPT_DEVICE,
>>         OPT_VERSION,
>>         OPT_HELP = 'h'
>>@@ -810,6 +818,8 @@ static void common_exit_handler(int sig)
>>                 bind_fbcon(true);
>>         }
>>
>>+        igt_hook_free(igt_hook);
>>+
>>         /* When not killed by a signal check that igt_exit() has been properly
>>          * called. */
>>         assert(sig != 0 || igt_exit_called || igt_is_aborting);
>>@@ -907,6 +917,8 @@ static void print_usage(const char *help_str, bool output_on_stderr)
>>                    "  --interactive-debug[=domain]\n"
>>                    "  --skip-crc-compare\n"
>>                    "  --trace-on-oops\n"
>>+                   "  --hook [<events>:]<cmd>\n"
>>+                   "  --help-hook\n"
>>                    "  --help-description\n"
>>                    "  --describe\n"
>>                    "  --device filters\n"
>>@@ -1090,6 +1102,8 @@ static int common_init(int *argc, char **argv,
>>                 {"interactive-debug", optional_argument, NULL, OPT_INTERACTIVE_DEBUG},
>>                 {"skip-crc-compare",  no_argument,       NULL, OPT_SKIP_CRC},
>>                 {"trace-on-oops",     no_argument,       NULL, OPT_TRACE_OOPS},
>>+                {"hook",              required_argument, NULL, OPT_HOOK},
>>+                {"help-hook",         no_argument,       NULL, OPT_HELP_HOOK},
>>                 {"device",            required_argument, NULL, OPT_DEVICE},
>>                 {"version",           no_argument,       NULL, OPT_VERSION},
>>                 {"help",              no_argument,       NULL, OPT_HELP},
>>@@ -1225,6 +1239,24 @@ static int common_init(int *argc, char **argv,
>>                 case OPT_TRACE_OOPS:
>>                         show_ftrace = true;
>>                         break;
>>+                case OPT_HOOK:
>>+                        assert(optarg);
>>+                        if (igt_hook) {
>>+                                igt_warn("Overriding previous hook descriptor\n");
>>+                                igt_hook_free(igt_hook);
>>+                        }
>>+                        igt_hook = igt_hook_init(optarg, &ret);
>
>this is a pair with igt_hook_free(). I think it's more common to
>call it either _new() or _create() rather than _init().

Okay. Decided to go with igt_hook_create().

>
>
>>+                        if (!igt_hook) {
>>+                                igt_critical("Failed to initialize hook data: %s\n",
>>+                                             igt_hook_error_str(ret));
>>+                                ret = ret > 0 ? -2 : -3;
>>+                                goto out;
>>+                        }
>>+                        break;
>>+                case OPT_HELP_HOOK:
>>+                        igt_hook_print_help(stdout, "--hook");
>>+                        ret = -1;
>>+                        goto out;
>>                 case OPT_DEVICE:
>>                         assert(optarg);
>>                         /* if set by env IGT_DEVICE we need to free it */
>>@@ -1274,9 +1306,24 @@ out:
>>                         exit(IGT_EXIT_INVALID);
>>         }
>>
>>-        if (ret < 0)
>>-                /* exit with no error for -h/--help */
>>-                exit(ret == -1 ? 0 : IGT_EXIT_INVALID);
>>+        if (ret < 0) {
>>+                free(igt_hook);
>>+                igt_hook = NULL;
>>+
>>+                switch (ret) {
>>+                case -1: /* exit with no error for -h/--help */
>>+                        exit(0);
>>+                        break;
>>+                case -2:
>>+                        exit(IGT_EXIT_INVALID);
>>+                        break;
>>+                case -3:
>>+                        exit(IGT_EXIT_ABORT);
>>+                        break;
>>+                default:
>>+                        assert(0);
>>+                }
>>+        }
>>
>>         if (!igt_only_list_subtests()) {
>>                 bind_fbcon(false);
>>@@ -1284,6 +1331,15 @@ out:
>>                 print_version();
>>                 igt_srandom();
>>
>>+                if (igt_hook) {
>>+                        struct igt_hook_evt hook_evt = {
>>+                                .evt_type = IGT_HOOK_PRE_TEST,
>>+                                .target_name = command_str,
>>+                        };
>>+
>>+                        igt_hook_push_evt(igt_hook, &hook_evt);
>>+                }
>>+
>>                 sync();
>>                 oom_adjust_for_doom();
>>                 ftrace_dump_on_oops(show_ftrace);
>>@@ -1487,6 +1543,16 @@ bool __igt_run_subtest(const char *subtest_name, const char *file, const int lin
>>         igt_thread_clear_fail_state();
>>
>>         igt_gettime(&subtest_time);
>>+
>>+        if (igt_hook) {
>>+                struct igt_hook_evt hook_evt = {
>>+                        .evt_type = IGT_HOOK_PRE_SUBTEST,
>>+                        .target_name = subtest_name,
>>+                };
>>+
>>+                igt_hook_push_evt(igt_hook, &hook_evt);
>>+        }
>>+
>>         return (in_subtest = subtest_name);
>> }
>>
>>@@ -1517,6 +1583,16 @@ bool __igt_run_dynamic_subtest(const char *dynamic_subtest_name)
>>         _igt_dynamic_tests_executed++;
>>
>>         igt_gettime(&dynamic_subtest_time);
>>+
>>+        if (igt_hook) {
>>+                struct igt_hook_evt hook_evt = {
>>+                        .evt_type = IGT_HOOK_PRE_DYN_SUBTEST,
>>+                        .target_name = dynamic_subtest_name,
>>+                };
>>+
>>+                igt_hook_push_evt(igt_hook, &hook_evt);
>>+        }
>>+
>>         return (in_dynamic_subtest = dynamic_subtest_name);
>> }
>>
>>@@ -1602,6 +1678,17 @@ __noreturn static void exit_subtest(const char *result)
>>         struct timespec *thentime = in_dynamic_subtest ? &dynamic_subtest_time : &subtest_time;
>>         jmp_buf *jmptarget = in_dynamic_subtest ? &igt_dynamic_jmpbuf : &igt_subtest_jmpbuf;
>>
>>+        if (igt_hook) {
>>+                struct igt_hook_evt hook_evt = {
>>+                        .evt_type = (in_dynamic_subtest
>>+                                        ? IGT_HOOK_POST_DYN_SUBTEST
>>+                                        : IGT_HOOK_POST_SUBTEST),
>>+                        .result = result,
>>+                };
>>+
>>+                igt_hook_push_evt(igt_hook, &hook_evt);
>>+        }
>>+
>>         if (!igt_thread_is_main()) {
>>                 igt_thread_fail();
>>                 pthread_exit(NULL);
>>@@ -2274,6 +2361,7 @@ void __igt_abort(const char *domain, const char *file, const int line,
>> void igt_exit(void)
>> {
>>         int tmp;
>>+        const char *result;
>>
>>         if (!test_with_subtests)
>>                 igt_thread_assert_no_failures();
>>@@ -2318,12 +2406,7 @@ void igt_exit(void)
>>
>>         assert(waitpid(-1, &tmp, WNOHANG) == -1 && errno == ECHILD);
>>
>>-        if (!test_with_subtests) {
>>-                struct timespec now;
>>-                const char *result;
>>-
>>-                igt_gettime(&now);
>>-
>>+        if (!test_with_subtests || igt_hook) {
>>                 switch (igt_exitcode) {
>>                         case IGT_EXIT_SUCCESS:
>>                                 result = "SUCCESS";
>>@@ -2334,6 +2417,12 @@ void igt_exit(void)
>>                         default:
>>                                 result = "FAIL";
>>                 }
>>+        }
>>+
>>+        if (!test_with_subtests) {
>>+                struct timespec now;
>>+
>>+                igt_gettime(&now);
>>
>>                 if (test_multi_fork_child) /* parent will do the yelling */
>>                         _log_line_fprintf(stdout, "dyn_child pid:%d (%.3fs) ends with err=%d\n",
>>@@ -2344,6 +2433,15 @@ void igt_exit(void)
>>                                           result, igt_time_elapsed(&subtest_time, &now));
>>         }
>>
>>+        if (igt_hook) {
>>+                struct igt_hook_evt hook_evt = {
>>+                        .evt_type = IGT_HOOK_POST_TEST,
>>+                        .result = result,
>>+                };
>>+
>>+                igt_hook_push_evt(igt_hook, &hook_evt);
>>+        }
>>+
>>         exit(igt_exitcode);
>> }
>>
>>diff --git a/lib/igt_hook.c b/lib/igt_hook.c
>>new file mode 100644
>>index 000000000000..8a39e19e3e5f
>>--- /dev/null
>>+++ b/lib/igt_hook.c
>>@@ -0,0 +1,499 @@
>>+/*
>>+ * Copyright © 2024 Intel Corporation
>>+ *
>>+ * 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.
>
>
>I think this should follow kernel and some of our igt and just use SPDX.

Done. Thanks.

>
>>+ */
>>+#include <assert.h>
>>+#include <errno.h>
>>+#include <limits.h>
>>+#include <stdbool.h>
>>+#include <stddef.h>
>>+#include <stdint.h>
>>+#include <stdio.h>
>>+#include <stdlib.h>
>>+#include <string.h>
>>+
>>+#include "igt_hook.h"
>>+
>>+/**
>>+ * SECTION:igt_hook
>>+ * @short_description: Support for running a hook script on test execution
>>+ * @title: Hook support
>>+ *
>>+ * IGT provides support for running a hook script when executing tests. This
>>+ * support is provided to users via CLI option `--hook` available in test
>>+ * binaries. Users should use `--help-hook` for detailed usaged description of
>>+ * the feature.
>>+ *
>>+ * The sole user of the exposed API is `igt_core`, which calls @igt_hook_init()
>>+ * when initializing a test case, then calls @igt_hook_push_evt() for each event
>>+ * that occurs during that test's execution and finally calls @igt_hook_free()
>>+ * to clean up at the end.
>>+ */
>>+
>>+#define TEST_NAME_INITIAL_SIZE 16
>>+
>>+typedef uint16_t evt_mask_t;
>>+
>>+struct igt_hook {
>>+        evt_mask_t evt_mask;
>>+        char *cmd;
>>+        char *test_name;
>>+        size_t test_name_size;
>>+        char *subtest_name;
>>+        size_t subtest_name_size;
>>+        char *dyn_subtest_name;
>>+        size_t dyn_subtest_name_size;
>>+        char *test_fullname;
>>+};
>>+
>>+enum igt_hook_error {
>>+        IGT_HOOK_EVT_EMPTY_NAME = 1,
>>+        IGT_HOOK_EVT_NO_MATCH,
>>+};
>>+
>>+static_assert(IGT_HOOK_NUM_EVENTS <= sizeof(evt_mask_t) * CHAR_BIT,
>>+              "Number of event types does not fit event type mask");
>>+
>>+static const char *igt_hook_evt_type_to_name(enum igt_hook_evt_type evt_type)
>>+{
>>+        switch (evt_type) {
>>+        case IGT_HOOK_PRE_TEST:
>>+                return "pre-test";
>>+        case IGT_HOOK_PRE_SUBTEST:
>>+                return "pre-subtest";
>>+        case IGT_HOOK_PRE_DYN_SUBTEST:
>>+                return "pre-dyn-subtest";
>>+        case IGT_HOOK_POST_DYN_SUBTEST:
>>+                return "post-dyn-subtest";
>>+        case IGT_HOOK_POST_SUBTEST:
>>+                return "post-subtest";
>>+        case IGT_HOOK_POST_TEST:
>>+                return "post-test";
>>+        case IGT_HOOK_NUM_EVENTS:
>>+                break;
>>+        /* No "default:" case, to force a warning from -Wswitch in case we miss
>>+         * any new event type. */
>
>yeah, I like -Wswitch for this purpose, but I don't think we need a
>comment. Instead return something here.
>
>>+        }
>>+        return "?";
>
>and return nothing here 

Not sure I understand your suggestion. Since we do not have a default
case, we would need this return at the end of the function, no? Without
this return the build fails because of -Werror=return-type:

   error: control reaches end of non-void function [-Werror=return-type]

Also note that the -Wswitch is not being treated as error with the
current build config.

>
>>+}
>>+
>>+static int igt_hook_parse_hook_str(const char *hook_str, evt_mask_t *evt_mask, const char **cmd)
>>+{
>>+        const char *s;
>>+
>>+        if (!strchr(hook_str, ':')) {
>>+                *evt_mask = ~0;
>>+                *cmd = hook_str;
>>+                return 0;
>>+        }
>>+
>>+        s = hook_str;
>>+        *evt_mask = 0;
>>+
>>+        while (1) {
>>+                const char *evt_name;
>>+                bool has_match;
>>+                bool is_star;
>>+                enum igt_hook_evt_type evt_type;
>>+
>>+                evt_name = s;
>>+
>>+                while (*s != ':' && *s != ',')
>>+                        s++;
>>+
>>+                if (evt_name == s)
>>+                        return IGT_HOOK_EVT_EMPTY_NAME;
>>+
>>+                has_match = false;
>>+                is_star = *evt_name == '*' && evt_name + 1 == s;
>>+
>>+                for (evt_type = IGT_HOOK_PRE_TEST; evt_type < IGT_HOOK_NUM_EVENTS; evt_type++) {
>>+                        if (!is_star) {
>>+                                const char *this_event_name = igt_hook_evt_type_to_name(evt_type);
>>+                                size_t len = s - evt_name;
>>+
>>+                                if (len != strlen(this_event_name))
>>+                                        continue;
>>+
>>+                                if (strncmp(evt_name, this_event_name, len))
>>+                                        continue;
>>+                        }
>>+
>>+                        *evt_mask |= 1 << evt_type;
>
>pre-test,pre-test,pre-test:echo foo
>
>^ shouldn't this cause a warning?

I don't think it shouldn't. There is no harm in passing the same event
type multiple times and I don't see strong reasons to complain about it.

>
>but see below for alternative interface
>
>>+                        has_match = true;
>>+
>>+                        if (!is_star)
>>+                                break;
>>+                }
>>+
>>+                if (!has_match)
>>+                        return IGT_HOOK_EVT_NO_MATCH;
>>+
>>+                if (*s++ == ':')
>>+                        break;
>>+        }
>>+
>>+        *cmd = s;
>>+
>>+        return 0;
>>+}
>>+
>>+static size_t igt_hook_calc_test_fullname_size(struct igt_hook *igt_hook) {
>>+        /* The maximum size of test_fullname will be the maximum length of
>>+         * "igt@<test_name>@<subtest_name>@<dyn_subtest_name>" plus 1 for the
>>+         * null byte. */
>>+        return (igt_hook->test_name_size +
>>+                igt_hook->subtest_name_size +
>>+                igt_hook->dyn_subtest_name_size) + 4;
>>+}
>>+
>>+static void igt_hook_update_test_fullname(struct igt_hook *igt_hook)
>>+{
>>+        int i;
>>+        char *s;
>>+        const char *values[3] = {
>>+                igt_hook->test_name,
>>+                igt_hook->subtest_name,
>>+                igt_hook->dyn_subtest_name,
>>+        };
>>+
>>+        if (igt_hook->test_name[0] == '\0') {
>>+                igt_hook->test_fullname[0] = '\0';
>>+                return;
>>+        }
>>+
>>+        s = stpcpy(igt_hook->test_fullname, "igt");
>>+        for (i = 0; i < 3 && values[i][0] != '\0'; i++) {
>
>                        ^ ARRAY_SIZE()

Decided to fix this with a sentinel value (NULL).

>
>>+                *s++ = '@';
>>+                s = stpcpy(s, values[i]);
>>+        }
>>+}
>>+
>>+/**
>>+ * igt_hook_init:
>>+ * @hook_str: Hook descriptor string.
>>+ * @error: Pointer to error number.
>>+ *
>>+ * Allocate and initialize an #igt_hook structure.
>>+ *
>>+ * This function parses the hook descriptor @hook_str and initializes the struct
>>+ * to be returned.
>>+ *
>>+ * The hook descriptor comes from the argument to `--hook` of the test
>>+ * executable being run.
>>+ *
>>+ * If not #NULL, @error is used to store a non-zero error number if an error
>>+ * happens. A human-readable string for that error number can be obtained with
>>+ * @igt_hook_error_str().
>>+ *
>>+ * Returns: The pointer to the #igt_hook structure on success or #NULL on error.
>>+ */
>>+struct igt_hook *igt_hook_init(const char *hook_str, int *error)
>>+{
>>+        int err;
>>+        evt_mask_t evt_mask;
>>+        const char *cmd;
>>+        struct igt_hook *igt_hook = NULL;
>>+
>>+
>>+        err = igt_hook_parse_hook_str(hook_str, &evt_mask, &cmd);
>>+        if (err)
>>+                goto out;
>>+
>>+        igt_hook = calloc(1, sizeof(*igt_hook));
>>+        igt_hook->evt_mask = evt_mask;
>>+
>>+        igt_hook->cmd = strdup(cmd);
>>+        if (!igt_hook->cmd) {
>>+                err = -errno;
>>+                goto out;
>>+        }
>>+
>>+        igt_hook->test_name = malloc(TEST_NAME_INITIAL_SIZE);
>>+        igt_hook->test_name_size = TEST_NAME_INITIAL_SIZE;
>>+        igt_hook->subtest_name = malloc(TEST_NAME_INITIAL_SIZE);
>>+        igt_hook->subtest_name_size = TEST_NAME_INITIAL_SIZE;
>>+        igt_hook->dyn_subtest_name = malloc(TEST_NAME_INITIAL_SIZE);
>>+        igt_hook->dyn_subtest_name_size = TEST_NAME_INITIAL_SIZE;
>
>up to you, but in this kind of things I tend to prefer the strings in
>the outer struct so we have fewer separate malloc()'s and thus fewer
>chances of leaking them

What would be the "outer struct" in this case?

>
>just calculate the size ahead and then update igt_hook->test_name,
>igt_hook->subtest_name and igt_hook->dyn_subtest_name to point to the right part.

Hm... Do you mean to have a single memory allocation to keep those three
strings? One issue I see with this approach is the complication when new
names would cause overlap, which would require us to shift stuff when
doing realloc().

>
>>+        igt_hook->test_fullname = malloc(igt_hook_calc_test_fullname_size(igt_hook));
>>+
>>+        igt_hook->test_name[0] = '\0';
>>+        igt_hook->subtest_name[0] = '\0';
>>+        igt_hook->dyn_subtest_name[0] = '\0';
>>+        igt_hook->test_fullname[0] = '\0';
>
>with the benefit that then these are covered by the calloc.
>
>up to you.

I'll send the v2 with this part as is for now. We can discuss more if
you'd like.

>
>>+
>>+out:
>>+        if (err) {
>>+                if (error)
>>+                        *error = err;
>>+
>>+                igt_hook_free(igt_hook);
>>+
>>+                return NULL;
>>+        }
>>+
>>+        return igt_hook;
>>+}
>>+
>>+/**
>>+ * igt_hook_free:
>>+ * @igt_hook: The igt_hook struct.
>>+ *
>>+ * De-initialize an igt_hook struct returned by @igt_hook_init().
>>+ *
>>+ * This is a no-op if @igt_hook is #NULL.
>>+ */
>>+void igt_hook_free(struct igt_hook *igt_hook)
>>+{
>>+        if (!igt_hook)
>>+                return;
>>+
>>+        free(igt_hook->cmd);
>>+        free(igt_hook->test_name);
>>+        free(igt_hook->subtest_name);
>>+        free(igt_hook->dyn_subtest_name);
>>+        free(igt_hook);
>>+}
>>+
>>+static void igt_hook_update_test_name_pre_call(struct igt_hook *igt_hook, struct igt_hook_evt *evt)
>>+{
>>+        char **name_ptr;
>>+        size_t *size_ptr;
>>+        size_t len;
>>+
>>+        switch (evt->evt_type) {
>>+        case IGT_HOOK_PRE_TEST:
>>+                name_ptr = &igt_hook->test_name;
>>+                size_ptr = &igt_hook->test_name_size;
>>+                break;
>>+        case IGT_HOOK_PRE_SUBTEST:
>>+                name_ptr = &igt_hook->subtest_name;
>>+                size_ptr = &igt_hook->subtest_name_size;
>>+                break;
>>+        case IGT_HOOK_PRE_DYN_SUBTEST:
>>+                name_ptr = &igt_hook->dyn_subtest_name;
>>+                size_ptr = &igt_hook->dyn_subtest_name_size;
>>+                break;
>>+        default:
>>+                return;
>>+        }
>>+
>>+        len = strlen(evt->target_name);
>>+        if (len + 1 > *size_ptr) {
>>+                size_t fullname_size;
>>+
>>+                *size_ptr *= 2;
>>+                *name_ptr = realloc(*name_ptr, *size_ptr);
>>+
>>+                fullname_size = igt_hook_calc_test_fullname_size(igt_hook);
>>+                igt_hook->test_fullname = realloc(igt_hook->test_fullname, fullname_size);
>>+        }
>>+
>>+        strcpy(*name_ptr, evt->target_name);
>>+        igt_hook_update_test_fullname(igt_hook);
>>+}
>>+
>>+static void igt_hook_update_test_name_post_call(struct igt_hook *igt_hook, struct igt_hook_evt *evt)
>>+{
>>+        switch (evt->evt_type) {
>>+        case IGT_HOOK_POST_TEST:
>>+                igt_hook->test_name[0] = '\0';
>>+                break;
>>+        case IGT_HOOK_POST_SUBTEST:
>>+                igt_hook->subtest_name[0] = '\0';
>>+                break;
>>+        case IGT_HOOK_POST_DYN_SUBTEST:
>>+                igt_hook->dyn_subtest_name[0] = '\0';
>>+                break;
>>+        default:
>>+                return;
>>+        }
>>+
>>+        igt_hook_update_test_fullname(igt_hook);
>>+}
>>+
>>+static void igt_hook_update_env_vars(struct igt_hook *igt_hook, struct igt_hook_evt *evt)
>>+{
>>+        setenv("IGT_HOOK_EVENT", igt_hook_evt_type_to_name(evt->evt_type), 1);
>>+        setenv("IGT_HOOK_TEST_FULLNAME", igt_hook->test_fullname, 1);
>>+        setenv("IGT_HOOK_TEST", igt_hook->test_name, 1);
>>+        setenv("IGT_HOOK_SUBTEST", igt_hook->subtest_name, 1);
>>+        setenv("IGT_HOOK_DYN_SUBTEST", igt_hook->dyn_subtest_name, 1);
>>+        setenv("IGT_HOOK_RESULT", evt->result ?: "", 1);
>>+}
>>+
>>+/**
>>+ * igt_hook_push_evt:
>>+ * @igt_hook: The igt_hook structure.
>>+ * @evt: The event to be pushed.
>>+ *
>>+ * Push a new igt_hook event.
>>+ *
>>+ * This function must be used to register a new igt_hook event. Calling it will
>>+ * cause execution of the hook script if the event type matches the filters
>>+ * provided during initialization of @igt_hook.
>>+ */
>>+void igt_hook_push_evt(struct igt_hook *igt_hook, struct igt_hook_evt *evt)
>>+{
>>+        typeof(igt_hook->evt_mask) evt_bit = (1 << evt->evt_type);
>>+
>>+        igt_hook_update_test_name_pre_call(igt_hook, evt);
>>+
>>+        if ((evt_bit & igt_hook->evt_mask)) {
>>+                igt_hook_update_env_vars(igt_hook, evt);
>>+                system(igt_hook->cmd);
>
>this polutes the env for the current process just so the child process
>has those env vars. We could either duplicate the environ on startup and
>use execle(), or to be simpler, just make sure to zap our env just after
>running the command.  This avoids undesirable behavior because we didn't
>clean up the environ.

Yeah, that makes sense. Decided to go with:

		struct igt_helper_process proc = {};

		igt_fork_helper(&proc) {
			igt_hook_update_env_vars(igt_hook, evt);
			system(igt_hook->cmd);
		}
		igt_wait_helper(&proc);

This does end up creating a "grandchild" process because we are still
using system(), but it keeps the code simple.

>
>
>>+        }
>>+
>>+        igt_hook_update_test_name_post_call(igt_hook, evt);
>>+}
>>+
>>+/**
>>+ * igt_hook_error_str:
>>+ * @error: Non-zero error number.
>>+ *
>>+ * Return a human-readable string containing a description of an error number
>>+ * generated by one of the `igt_hook_*` functions.
>>+ *
>>+ * The string will be the result of strerror() for errors from the C standard
>>+ * library or a custom description specific to igt_hook.
>>+ */
>>+const char *igt_hook_error_str(int error)
>>+{
>>+        if (!error)
>>+                return "No error";
>>+
>>+        if (error > 0) {
>>+                enum igt_hook_error hook_error = error;
>>+
>>+                switch (hook_error) {
>>+                case IGT_HOOK_EVT_EMPTY_NAME:
>>+                        return "Empty name in event descriptor";
>>+                case IGT_HOOK_EVT_NO_MATCH:
>>+                        return "Event name in event descriptor does not match any event type";
>>+                default:
>>+                        return "Unknown error";
>>+                }
>>+        } else {
>>+                return strerror(-error);
>>+        }
>>+}
>>+
>>+/**
>>+ * igt_hook_print_help:
>>+ * @f: File pointer where to write the output.
>>+ * @option_name: Name of the CLI option that accepts the hook descriptor.
>>+ *
>>+ * Print a detailed user help text on hook usage.
>>+ */
>>+void igt_hook_print_help(FILE *f, const char *option_name)
>>+{
>>+        fprintf(f, "\
>>+The option %1$s receives as argument a \"hook descriptor\" and allows the\n\
>>+execution of a shell command at different points during execution of tests. Each\n\
>>+such a point is called a \"hook event\".\n\
>>+\n\
>>+Examples:\n\
>>+\n\
>>+  # Prints hook-specic env vars for every event.\n\
>>+  %1$s 'printenv | grep ^IGT_HOOK_'\n\
>>+\n\
>>+  # Equivalent to the above. Useful if command contains ':'.\n\
>>+  %1$s '*:printenv | grep ^IGT_HOOK_'\n\
>>+\n\
>>+  # Adds a line to out.txt containing the result of each test case.\n\
>>+  %1$s 'post-test:echo $IGT_HOOK_TEST_FULLNAME $IGT_HOOK_RESULT >> out.txt'\n\
>>+\n\
>>+The accepted format for a hook descriptor is `[<events>:]<cmd>`, where:\n\
>>+\n\
>>+  - <events> is a comma-separated list of event descriptors, which defines the\n\
>>+    set of events be tracked. If omitted, all events are tracked.\n\
>>+\n\
>
>
>I think the interface git-rebase is more natural for this kind of
>thing. You have 2 concepts: event and hook with n:1 relationship
>because of the interface chosen.
>
>Why not simply use 1:1 and then allow the program to have multiple
>hooks?

Yep, I thought about doing it, but was a bit lazy and decided to have a
single hook string instead of an array :-P

>
>
>Just in git-rebase we do  `git rebase -x "echo foo" -x "echo bar"` and
>the commands are appended, we could have:
>
>igt_runner -x 'pre-test:echo foo' -x 'post-test:echo bar'. Then you only
>warn when you user incorrectly used the same hook type.  I think it

I like this.

I personally do not see a problem with having multiple hook commands for
the same event. We would just end up running each of them in turn.

I guess we could basically extend the current code to accept multiple
`--hook` options, in the same format as with the current implementation
here. That would allow something like

  my_test --hook pre-test,pre-subtest:some-command-common-to-both \
          --hook post-test:some-command-only-for-post-test

I'll work on this.

>would be more natural to eventually embed the support on a test list
>(with the exception we'd not warn anymore), with something like:
>
>hook at pre-test@echo foo
>hook at post-test@echo bar
>igt at ...
>igt at ...
>igt at ...
>hook at pre-test@ ....
>igt at ...
>

Yeah. Maybe in a future series :-)

Thanks for all the feedback!

--
Gustavo Sousa

>
>
>
>
>>+  - <cmd> is a shell command to be executed on the occurrence each tracked\n\
>>+    event. If the command contains ':', then passing <events> is required,\n\
>>+    otherwise part of the command would be treated as an event descriptor.\n\
>>+\n\
>>+", option_name);
>>+
>>+        fprintf(f, "\
>>+An \"event descriptor\" is either the name of an event or the string '*'. The\n\
>>+latter matches all event names. The list of possible event names is provided\n\
>>+below:\n\
>>+\n\
>>+");
>>+
>>+        for (enum igt_hook_evt_type et = 0; et < IGT_HOOK_NUM_EVENTS; et++) {
>>+                const char *desc;
>>+
>>+                switch (et) {
>>+                case IGT_HOOK_PRE_TEST:
>>+                        desc = "Occurs before a test case starts.";
>>+                        break;
>>+                case IGT_HOOK_PRE_SUBTEST:
>>+                        desc = "Occurs before the execution of a subtest.";
>>+                        break;
>>+                case IGT_HOOK_PRE_DYN_SUBTEST:
>>+                        desc = "Occurs before the execution of a dynamic subtest.";
>>+                        break;
>>+                case IGT_HOOK_POST_DYN_SUBTEST:
>>+                        desc = "Occurs after the execution of a dynamic subtest.";
>>+                        break;
>>+                case IGT_HOOK_POST_SUBTEST:
>>+                        desc = "Occurs after the execution of a subtest.";
>>+                        break;
>>+                case IGT_HOOK_POST_TEST:
>>+                        desc = "Occurs after a test case has finished.";
>>+                        break;
>>+                default:
>>+                        desc = "MISSING DESCRIPTION";
>>+                }
>>+
>>+                fprintf(f, "  %s\n  %s\n\n", igt_hook_evt_type_to_name(et), desc);
>>+        }
>>+
>>+        fprintf(f, "\
>>+For each event matched by <events>, <cmd> is executed as a shell command. The\n\
>>+exit status of the command is ignored. The following environment variables are\n\
>>+available to the command:\n\
>>+\n\
>>+  IGT_HOOK_EVENT\n\
>>+  Name of the current event.\n\
>>+\n\
>>+  IGT_HOOK_TEST_FULLNAME\n\
>>+  Full name of the test in the format `igt@<test>[@<subtest>[@<dyn_subtest>]]`.\n\
>>+\n\
>>+  IGT_HOOK_TEST                   \n\
>>+  Name of the current test.\n\
>>+\n\
>>+  IGT_HOOK_SUBTEST\n\
>>+  Name of the current subtest. Will be the empty string if not running a\n\
>>+  subtest.\n\
>>+\n\
>>+  IGT_HOOK_DYN_SUBTEST\n\
>>+  Name of the current dynamic subtest. Will be the empty string if not running a\n\
>>+  dynamic subtest.\n\
>>+\n\
>>+  IGT_HOOK_RESULT\n\
>>+  String representing the result of the test/subtest/dynamic subtest. Possible\n\
>>+  values are: SUCCESS, SKIP or FAIL. This is only applicable on \"post-*\"\n\
>>+  events and will be the empty string for other types of events.\n\
>>+\n\
>>+");
>>+}
>>diff --git a/lib/igt_hook.h b/lib/igt_hook.h
>>new file mode 100644
>>index 000000000000..6fef7254a317
>>--- /dev/null
>>+++ b/lib/igt_hook.h
>>@@ -0,0 +1,86 @@
>>+/*
>>+ * Copyright © 2024 Intel Corporation
>>+ *
>>+ * 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.
>>+ */
>
>ditto on SPDX
>
>
>Lucas De Marchi


More information about the igt-dev mailing list