[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