[PATCH i-g-t v4 0/4] RFC: Add attachments support

[PATCH i-g-t v4 0/4] RFC: Add attachments support

[Zbigniew Kempczyński]

This series introduces support for hook/test written attachments.
It is somehow limited and stiff but allows to write anything test/hook
wants in attachments directory. File names which occured in attachments
directory are added to results.json.

Series targets our need to include guc logs for failed tests especially
in the point of time when it occurs.

v2: - added clearing of attachments dir in overwrite mode
    - guc-log hook script is now installed to igt datadir/hooks dir
    - added hook-exec-allowlist to selectively execute hook scripts
v3: - drop hook-exec-allowlist in runner, now scripts are filtering
      allowlist on their own
    - add support for attachments in recursive directories
v4: - change clear results to use path (used then in nftw())
    - introduce hooks-wrapper.sh, common helper for hooks scripts
    - replace if/else to switch/case

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Ryszard Knop <ryszard.knop@intel.com>
Cc: Gustavo Sousa <gustavo.sousa@intel.com>
Cc: Krzysztof Karas <krzysztof.karas@intel.com>

Zbigniew Kempczyński (4):
  runner: Rename dirfd to avoid clash with dirfd()
  runner: Create attachments directory to use by hooks
  intel-ci/hooks: Add hooks-wrapper and aux scripts/allowlists
  runner/resultgen: Insert attachments list into results.json

 lib/igt_hook.c                                |  4 +
 runner/executor.c                             | 89 +++++++++++++++----
 runner/executor.h                             |  2 +
 runner/resultgen.c                            | 79 ++++++++++++++++
 tests/intel-ci/hooks/common.helper            | 47 ++++++++++
 .../hooks/copy_guc_log_on_fail.allowlist      |  2 +
 tests/intel-ci/hooks/copy_guc_log_on_fail.sh  | 24 +++++
 tests/intel-ci/hooks/hook-example.allowlist   |  2 +
 tests/intel-ci/hooks/hook-example.sh          | 17 ++++
 tests/intel-ci/hooks/hooks-wrapper.sh         | 12 +++
 tests/intel-ci/meson.build                    | 11 +++
 11 files changed, 271 insertions(+), 18 deletions(-)
 create mode 100755 tests/intel-ci/hooks/common.helper
 create mode 100644 tests/intel-ci/hooks/copy_guc_log_on_fail.allowlist
 create mode 100755 tests/intel-ci/hooks/copy_guc_log_on_fail.sh
 create mode 100644 tests/intel-ci/hooks/hook-example.allowlist
 create mode 100755 tests/intel-ci/hooks/hook-example.sh
 create mode 100755 tests/intel-ci/hooks/hooks-wrapper.sh

-- 
2.43.0

[PATCH i-g-t v4 1/4] runner: Rename dirfd to avoid clash with dirfd()

[Zbigniew Kempczyński]

Upcoming change within execute_next_entry() will call dirfd() so
rename local dirfd variable is necessary.

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Ryszard Knop <ryszard.knop@intel.com>
Reviewed-by: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com>
---
 runner/executor.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/runner/executor.c b/runner/executor.c
index 847abe481a..1485b59d1f 100644
--- a/runner/executor.c
+++ b/runner/executor.c
@@ -1772,7 +1772,7 @@ static int execute_next_entry(struct execute_state *state,
 			      char **abortreason,
 			      bool *abort_already_written)
 {
-	int dirfd;
+	int idirfd;
 	int outputs[_F_LAST];
 	int kmsgfd;
 	int outpipe[2] = { -1, -1 };
@@ -1786,19 +1786,19 @@ static int execute_next_entry(struct execute_state *state,
 
 	snprintf(name, sizeof(name), "%zd", idx);
 	mkdirat(resdirfd, name, 0777);
-	if ((dirfd = openat(resdirfd, name, O_DIRECTORY | O_RDONLY | O_CLOEXEC)) < 0) {
+	if ((idirfd = openat(resdirfd, name, O_DIRECTORY | O_RDONLY | O_CLOEXEC)) < 0) {
 		errf("Error accessing individual test result directory\n");
 		return -1;
 	}
 
-	if (!open_output_files(dirfd, outputs, true)) {
+	if (!open_output_files(idirfd, outputs, true)) {
 		errf("Error opening output files\n");
 		result = -1;
 		goto out_dirfd;
 	}
 
 	if (settings->sync) {
-		fsync(dirfd);
+		fsync(idirfd);
 		fsync(resdirfd);
 	}
 
@@ -1900,8 +1900,8 @@ out_pipe:
 	close_outputs(outputs);
 out_dirfd:
 	if (settings->sync)
-		fsync(dirfd);
-	close(dirfd);
+		fsync(idirfd);
+	close(idirfd);
 	if (settings->sync)
 		fsync(resdirfd);
 
-- 
2.43.0

[PATCH i-g-t v4 2/4] runner: Create attachments directory to use by hooks

[Zbigniew Kempczyński]

Results parsing is currently limited to few predefined files.

Create "attachments" directory and export full path of created directory
to be used within hooks. From now on environment variable
IGT_RUNNER_ATTACHMENTS_DIR become visible when igt_runner is used
to execute tests. This env contains directory where subtests/dynsubtests
may write auxiliary files which filenames will be included in results.json.

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Ryszard Knop <ryszard.knop@intel.com>
Cc: Krzysztof Karas <krzysztof.karas@intel.com>
---
v2: - simplify attachment dirname concatenation (Kamil)
    - remove files in attachments dir when overwrite is set (Kamil)
v3: - unify 'attdir*' variables (Krzysztof)
    - do recursive removal in attachments directories (Gustavo)
v4: - remove noise newline leftover (Krzysztof)
    - pass path instead of fd to clear function and simplify
      the code (compact attachments function within main clear) (Gustavo)
---
 lib/igt_hook.c    |  4 +++
 runner/executor.c | 77 +++++++++++++++++++++++++++++++++++++++--------
 runner/executor.h |  2 ++
 3 files changed, 71 insertions(+), 12 deletions(-)

diff --git a/lib/igt_hook.c b/lib/igt_hook.c
index f86ed56f72..b8f25b6c7a 100644
--- a/lib/igt_hook.c
+++ b/lib/igt_hook.c
@@ -518,5 +518,9 @@ available to the command:\n\
 \n\
 Note that %s can be passed multiple times. Each descriptor is evaluated in turn\n\
 when matching events and running hook commands.\n\
+\n\
+When executed by the igt_runner, environment IGT_RUNNER_ATTACHMENTS_DIR\n\
+is passed additionally to the hook script. It contains directory where\n\
+script may write additional attachments like guc logs, etc.\n\
 ", option_name);
 }
diff --git a/runner/executor.c b/runner/executor.c
index 1485b59d1f..496ca1a4ea 100644
--- a/runner/executor.c
+++ b/runner/executor.c
@@ -1,6 +1,7 @@
 #include <ctype.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <ftw.h>
 #ifdef ANDROID
 #include "android/glib.h"
 #else
@@ -1779,17 +1780,22 @@ static int execute_next_entry(struct execute_state *state,
 	int errpipe[2] = { -1, -1 };
 	int socket[2] = { -1, -1 };
 	int outfd, errfd, socketfd;
-	char name[32];
+	char name[32], attname[32];
+	char attdirname[PATH_MAX];
 	pid_t child;
 	int result;
 	size_t idx = state->next;
 
 	snprintf(name, sizeof(name), "%zd", idx);
+	snprintf(attname, sizeof(attname), "%zd/%s", idx, DIR_ATTACHMENTS);
 	mkdirat(resdirfd, name, 0777);
+	mkdirat(resdirfd, attname, 0777);
 	if ((idirfd = openat(resdirfd, name, O_DIRECTORY | O_RDONLY | O_CLOEXEC)) < 0) {
 		errf("Error accessing individual test result directory\n");
 		return -1;
 	}
+	snprintf(attdirname, sizeof(attdirname), "%s/%s",
+		 settings->results_path, attname);
 
 	if (!open_output_files(idirfd, outputs, true)) {
 		errf("Error opening output files\n");
@@ -1870,6 +1876,7 @@ static int execute_next_entry(struct execute_state *state,
 			setenv("IGT_RUNNER_SOCKET_FD", envstring, 1);
 		}
 		setenv("IGT_SENTINEL_ON_STDERR", "1", 1);
+		setenv("IGT_RUNNER_ATTACHMENTS_DIR", attdirname, 1);
 
 		execute_test_process(outfd, errfd, socketfd, settings, entry);
 		/* unreachable */
@@ -1948,19 +1955,66 @@ static int remove_file(int dirfd, const char *name)
 	return unlinkat(dirfd, name, 0) && errno != ENOENT;
 }
 
-static bool clear_test_result_directory(int dirfd)
+static int ftw_attachment_removal(const char *fpath, const struct stat *sb,
+				  int typeflag, struct FTW *ftwbuf)
 {
-	int i;
+	(void)sb;
+	(void)ftwbuf;
+
+	switch (typeflag) {
+	case FTW_F:
+	case FTW_SL:
+		if (unlink(fpath)) {
+			errf("Error removing file %s: %s\n",
+			     fpath, strerror(errno));
+			return -1;
+		}
+		break;
+	case FTW_DP:
+		if (rmdir(fpath)) {
+			errf("Error removing directory %s: %s\n",
+			     fpath, strerror(errno));
+			return -1;
+		}
+		break;
+	default:
+		errf("Cannot remove %s (unsupported file type)\n", fpath);
+		return -1;
+	}
+
+	return 0;
+}
+
+static bool clear_test_result_directory(const char *name)
+{
+	char *attdirname;
+	int i, resdirfd;
+	int ret;
+
+	if ((resdirfd = open(name, O_DIRECTORY | O_RDONLY)) < 0)
+		return false;
 
 	for (i = 0; i < _F_LAST; i++) {
-		if (remove_file(dirfd, filenames[i])) {
+		if (remove_file(resdirfd, filenames[i])) {
 			errf("Error deleting %s from test result directory: %m\n",
 			     filenames[i]);
+			close(resdirfd);
 			return false;
 		}
 	}
 
-	return true;
+	close(resdirfd);
+
+	if (strlen(name) + strlen(DIR_ATTACHMENTS) + 1 >= PATH_MAX) {
+		errf("Attachments directory length exceeds PATH_MAX\n");
+		return false;
+	}
+
+	asprintf(&attdirname, "%s/%s", name, DIR_ATTACHMENTS);
+	ret = nftw(attdirname, ftw_attachment_removal, 4, FTW_PHYS | FTW_DEPTH);
+	free(attdirname);
+
+	return ret ? false : true;
 }
 
 static bool clear_old_results(char *path)
@@ -1990,20 +2044,19 @@ static bool clear_old_results(char *path)
 		return false;
 	}
 
+
 	for (i = 0; true; i++) {
-		int resdirfd;
+		struct stat st;
 
-		snprintf(name, sizeof(name), "%zd", i);
-		if ((resdirfd = openat(dirfd, name, O_DIRECTORY | O_RDONLY)) < 0)
+		snprintf(name, sizeof(name), "%s/%zd", path, i);
+		if (stat(name, &st))
 			break;
 
-		if (!clear_test_result_directory(resdirfd)) {
-			close(resdirfd);
+		if (!clear_test_result_directory(name)) {
 			close(dirfd);
 			return false;
 		}
-		close(resdirfd);
-		if (unlinkat(dirfd, name, AT_REMOVEDIR)) {
+		if (rmdir(name)) {
 			errf("Warning: Result directory %s contains extra files\n",
 			     name);
 		}
diff --git a/runner/executor.h b/runner/executor.h
index 3b1cabcf55..bc6ac80dc4 100644
--- a/runner/executor.h
+++ b/runner/executor.h
@@ -25,6 +25,8 @@ enum {
 	_F_LAST,
 };
 
+#define DIR_ATTACHMENTS "attachments"
+
 bool open_output_files(int dirfd, int *fds, bool write);
 bool open_output_files_rdonly(int dirfd, int *fds);
 void close_outputs(int *fds);
-- 
2.43.0

Re: [PATCH i-g-t v4 2/4] runner: Create attachments directory to use by hooks

[Krzysztof Karas]

Hi Zbigniew,

[...]

>  
>  static bool clear_old_results(char *path)
> @@ -1990,20 +2044,19 @@ static bool clear_old_results(char *path)
>  		return false;
>  	}
>  
> +
You added a newline here, I think it is redundant.
Apart from that, I like the patch:

Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com>

>  	for (i = 0; true; i++) {
> -		int resdirfd;
> +		struct stat st;
>  
> -		snprintf(name, sizeof(name), "%zd", i);
> -		if ((resdirfd = openat(dirfd, name, O_DIRECTORY | O_RDONLY)) < 0)
> +		snprintf(name, sizeof(name), "%s/%zd", path, i);
> +		if (stat(name, &st))
>  			break;
>  
> -		if (!clear_test_result_directory(resdirfd)) {
> -			close(resdirfd);
> +		if (!clear_test_result_directory(name)) {
>  			close(dirfd);
>  			return false;
>  		}
> -		close(resdirfd);
> -		if (unlinkat(dirfd, name, AT_REMOVEDIR)) {
> +		if (rmdir(name)) {
>  			errf("Warning: Result directory %s contains extra files\n",
>  			     name);
>  		}
> diff --git a/runner/executor.h b/runner/executor.h
> index 3b1cabcf55..bc6ac80dc4 100644
> --- a/runner/executor.h
> +++ b/runner/executor.h
> @@ -25,6 +25,8 @@ enum {
>  	_F_LAST,
>  };
>  
> +#define DIR_ATTACHMENTS "attachments"
> +
>  bool open_output_files(int dirfd, int *fds, bool write);
>  bool open_output_files_rdonly(int dirfd, int *fds);
>  void close_outputs(int *fds);
> -- 
> 2.43.0
> 

-- 
Best Regards,
Krzysztof

[PATCH i-g-t v4 3/4] intel-ci/hooks: Add hooks-wrapper and aux scripts/allowlists

[Zbigniew Kempczyński]

CI currently lacks useful GuC logs when some tests are failing. For some
failed executions (not all as copying guc-log is time consuming) using
hook script which will copy it to attachments directory seems to be
simplest solution.

Add copy_guc_log_on_fail.sh script which copies GuC log when test/subtest
is failing. It requires an allowlist to specify for which tests hook
will copy GuC log. It is time consuming process so doing this for all
failed test would extend CI execution time too much.

Apart of this introduce hooks-wrapper.sh which might be main script for
executing hooks. Instead of adding --hook for each script separately it
may be used as a central point of hooks execution. It assumes it will
be executed after test/subtest completion - called hook scripts depends
on the IGT_HOOK_RESULT env var.

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Ryszard Knop <ryszard.knop@intel.com>
Cc: Krzysztof Karas <krzysztof.karas@intel.com>
---
v2: install hook to datadir/hooks
v3: migrate allowlist from runner to script
v4: adding hooks-wrapper.sh, helpers, etc
---
 tests/intel-ci/hooks/common.helper            | 47 +++++++++++++++++++
 .../hooks/copy_guc_log_on_fail.allowlist      |  2 +
 tests/intel-ci/hooks/copy_guc_log_on_fail.sh  | 24 ++++++++++
 tests/intel-ci/hooks/hook-example.allowlist   |  2 +
 tests/intel-ci/hooks/hook-example.sh          | 17 +++++++
 tests/intel-ci/hooks/hooks-wrapper.sh         | 12 +++++
 tests/intel-ci/meson.build                    | 11 +++++
 7 files changed, 115 insertions(+)
 create mode 100755 tests/intel-ci/hooks/common.helper
 create mode 100644 tests/intel-ci/hooks/copy_guc_log_on_fail.allowlist
 create mode 100755 tests/intel-ci/hooks/copy_guc_log_on_fail.sh
 create mode 100644 tests/intel-ci/hooks/hook-example.allowlist
 create mode 100755 tests/intel-ci/hooks/hook-example.sh
 create mode 100755 tests/intel-ci/hooks/hooks-wrapper.sh

diff --git a/tests/intel-ci/hooks/common.helper b/tests/intel-ci/hooks/common.helper
new file mode 100755
index 0000000000..a3f913f8e8
--- /dev/null
+++ b/tests/intel-ci/hooks/common.helper
@@ -0,0 +1,47 @@
+# Common helper for hooks executed from igt_runner
+#
+# Provide ALLOWLIST="..." as a default, it might be overwritten by --allowlist
+
+for arg in "$@"; do
+    case $arg in
+    --help)
+        echo "Usage: $0 [--help] [--allowlist <name.allowlist>]"
+        echo
+        echo "If name.allowlist is not passed default '${ALLOWLIST}' is used."
+        exit 0
+        ;;
+
+    --allowlist)
+        ALLOWLIST=$2
+        shift 2
+        ;;
+
+        *)
+        shift
+        ;;
+    esac
+done
+
+if [ -z "${IGT_RUNNER_ATTACHMENTS_DIR}" ]; then
+        echo "Missing IGT_RUNNER_ATTACHMENTS_DIR env"
+        exit 0
+fi
+
+# Look for allowlist in following places:
+# 1. Try in IGT_HOOK_ALLOWLIST_DIR if this environment exists
+# 2. Try in <SCRIPTDIR>
+
+if [ ! -z "${IGT_HOOK_ALLOWLIST_DIR}" ]; then
+        ALLOWLIST_PATH="${IGT_HOOK_ALLOWLIST_DIR}/${ALLOWLIST}"
+else
+        ALLOWLIST_PATH="${SCRIPTDIR}/${ALLOWLIST}"
+fi
+
+if [ ! -e "${ALLOWLIST_PATH}" ]; then
+        echo "Missing ${ALLOWLIST_PATH}"
+        exit 0
+fi
+
+if ! echo "${IGT_HOOK_TEST_FULLNAME}" | grep -q -f "$ALLOWLIST_PATH"; then
+        exit 0
+fi
diff --git a/tests/intel-ci/hooks/copy_guc_log_on_fail.allowlist b/tests/intel-ci/hooks/copy_guc_log_on_fail.allowlist
new file mode 100644
index 0000000000..04cb3f8298
--- /dev/null
+++ b/tests/intel-ci/hooks/copy_guc_log_on_fail.allowlist
@@ -0,0 +1,2 @@
+igt@xe_compute@compute-square
+igt@xe_exec_basic@once-basic@ccs0
diff --git a/tests/intel-ci/hooks/copy_guc_log_on_fail.sh b/tests/intel-ci/hooks/copy_guc_log_on_fail.sh
new file mode 100755
index 0000000000..c44c9a5004
--- /dev/null
+++ b/tests/intel-ci/hooks/copy_guc_log_on_fail.sh
@@ -0,0 +1,24 @@
+#!/bin/bash
+
+# Hook script for copying guc.log.
+# Suggested usage with:
+# --hook 'post-subtest:tests/intel-ci/hooks/copy_guc_log_on_fail.sh' \
+# --hook 'post-dyn-subtest:tests/intel-ci/hooks/copy_guc_log_on_fail.sh'
+# or within hooks-wrapper.sh
+
+# Copy only for failed subtests as this is time-consuming
+if [ "${IGT_HOOK_RESULT:-FAIL}" != "FAIL" ]; then
+	exit 0
+fi
+
+ALLOWLIST="copy_guc_log_on_fail.allowlist"
+SCRIPTDIR=$(dirname "$(realpath "$0")")
+. "${SCRIPTDIR}/common.helper"
+
+cd "${IGT_RUNNER_ATTACHMENTS_DIR}"
+
+for log in $(find /sys/kernel/debug/dri -iname 'guc_log'); do
+	attout=$(echo ${log:23} | sed -e 's/\//_/g')
+	mkdir -p "${IGT_HOOK_TEST_FULLNAME}"
+	cp "$log" "${IGT_HOOK_TEST_FULLNAME}/${attout}"
+done
diff --git a/tests/intel-ci/hooks/hook-example.allowlist b/tests/intel-ci/hooks/hook-example.allowlist
new file mode 100644
index 0000000000..54cbc0a1d4
--- /dev/null
+++ b/tests/intel-ci/hooks/hook-example.allowlist
@@ -0,0 +1,2 @@
+igt@xe_exec_basic@once-basic@rcs0
+igt@xe_exec_basic@once-basic@ccs0
diff --git a/tests/intel-ci/hooks/hook-example.sh b/tests/intel-ci/hooks/hook-example.sh
new file mode 100755
index 0000000000..512e123567
--- /dev/null
+++ b/tests/intel-ci/hooks/hook-example.sh
@@ -0,0 +1,17 @@
+#!/bin/bash
+
+# Hook example, touch <testname>/current-date.txt in attachments dir
+#
+# Suggested usage with:
+# --hook 'post-subtest:tests/intel-ci/hooks/hook-example.sh' \
+# --hook 'post-dyn-subtest:tests/intel-ci/hooks/hook-example.sh'
+# or within hooks-wrapper.sh
+
+ALLOWLIST="hook-example.allowlist"
+SCRIPTDIR=$(dirname "$(realpath "$0")")
+. "${SCRIPTDIR}/common.helper"
+
+cd "${IGT_RUNNER_ATTACHMENTS_DIR}"
+
+mkdir -p "${IGT_HOOK_TEST_FULLNAME}"
+date > "${IGT_HOOK_TEST_FULLNAME}/current-date.txt"
diff --git a/tests/intel-ci/hooks/hooks-wrapper.sh b/tests/intel-ci/hooks/hooks-wrapper.sh
new file mode 100755
index 0000000000..263b81a8b7
--- /dev/null
+++ b/tests/intel-ci/hooks/hooks-wrapper.sh
@@ -0,0 +1,12 @@
+#!/bin/bash
+
+# Hooks wrapper
+# Suggested usage with:
+# --hook 'post-subtest:tests/intel-ci/hooks/hooks-wrapper.sh' \
+# --hook 'post-dyn-subtest:tests/intel-ci/hooks/hooks-wrapper.sh'
+
+SCRIPTDIR=$(dirname "$(realpath "$0")")
+
+cd "${SCRIPTDIR}"
+./hook-example.sh
+./copy_guc_log_on_fail.sh
diff --git a/tests/intel-ci/meson.build b/tests/intel-ci/meson.build
index 2b3266bbd9..4a04b6aef8 100644
--- a/tests/intel-ci/meson.build
+++ b/tests/intel-ci/meson.build
@@ -34,8 +34,19 @@ intelci_files = [
   'xe.wcl.core.blocklist.txt',
 ]
 
+hooks_files = [
+  'hooks/common.helper',
+  'hooks/copy_guc_log_on_fail.sh',
+  'hooks/copy_guc_log_on_fail.allowlist',
+  'hooks/hook-example.sh',
+  'hooks/hook-example.allowlist',
+  'hooks/hooks-wrapper.sh',
+]
+
 if meson.version().version_compare('<1.3.0')
   install_data(sources : intelci_files, install_dir : datadir)
+  install_data(sources : hooks_files, install_dir : datadir + '/hooks')
 else
   install_data(sources : intelci_files, install_dir : datadir, follow_symlinks : false)
+  install_data(sources : hooks_files, install_dir : datadir + '/hooks', follow_symlinks : false)
 endif
-- 
2.43.0

Re: [PATCH i-g-t v4 3/4] intel-ci/hooks: Add hooks-wrapper and aux scripts/allowlists

[Krzysztof Karas]

Hi Zbigniew,
 
[...]

> diff --git a/tests/intel-ci/hooks/common.helper b/tests/intel-ci/hooks/common.helper
> new file mode 100755
> index 0000000000..a3f913f8e8
> --- /dev/null
> +++ b/tests/intel-ci/hooks/common.helper
> @@ -0,0 +1,47 @@
> +# Common helper for hooks executed from igt_runner
> +#
> +# Provide ALLOWLIST="..." as a default, it might be overwritten by --allowlist
Maybe instead of "Provide", which to me looks like you expect
an argument, "Set" would be better suited here?
It is just aesthetics, so I'd not block over this.

> +
> +for log in $(find /sys/kernel/debug/dri -iname 'guc_log'); do
> +	attout=$(echo ${log:23} | sed -e 's/\//_/g')
> +	mkdir -p "${IGT_HOOK_TEST_FULLNAME}"
> +	cp "$log" "${IGT_HOOK_TEST_FULLNAME}/${attout}"
> +done
> diff --git a/tests/intel-ci/hooks/hook-example.allowlist b/tests/intel-ci/hooks/hook-example.allowlist
> new file mode 100644
> index 0000000000..54cbc0a1d4
> --- /dev/null
> +++ b/tests/intel-ci/hooks/hook-example.allowlist
> @@ -0,0 +1,2 @@
> +igt@xe_exec_basic@once-basic@rcs0
> +igt@xe_exec_basic@once-basic@ccs0
> diff --git a/tests/intel-ci/hooks/hook-example.sh b/tests/intel-ci/hooks/hook-example.sh
> new file mode 100755
> index 0000000000..512e123567
> --- /dev/null
> +++ b/tests/intel-ci/hooks/hook-example.sh
> @@ -0,0 +1,17 @@
> +#!/bin/bash
> +
> +# Hook example, touch <testname>/current-date.txt in attachments dir
> +#
> +# Suggested usage with:
> +# --hook 'post-subtest:tests/intel-ci/hooks/hook-example.sh' \
> +# --hook 'post-dyn-subtest:tests/intel-ci/hooks/hook-example.sh'
> +# or within hooks-wrapper.sh
> +
> +ALLOWLIST="hook-example.allowlist"
> +SCRIPTDIR=$(dirname "$(realpath "$0")")
> +. "${SCRIPTDIR}/common.helper"
> +
> +cd "${IGT_RUNNER_ATTACHMENTS_DIR}"
> +
> +mkdir -p "${IGT_HOOK_TEST_FULLNAME}"
> +date > "${IGT_HOOK_TEST_FULLNAME}/current-date.txt"
> diff --git a/tests/intel-ci/hooks/hooks-wrapper.sh b/tests/intel-ci/hooks/hooks-wrapper.sh
> new file mode 100755
> index 0000000000..263b81a8b7
> --- /dev/null
> +++ b/tests/intel-ci/hooks/hooks-wrapper.sh
> @@ -0,0 +1,12 @@
> +#!/bin/bash
> +
> +# Hooks wrapper
> +# Suggested usage with:
> +# --hook 'post-subtest:tests/intel-ci/hooks/hooks-wrapper.sh' \
> +# --hook 'post-dyn-subtest:tests/intel-ci/hooks/hooks-wrapper.sh'
> +
> +SCRIPTDIR=$(dirname "$(realpath "$0")")
> +
> +cd "${SCRIPTDIR}"
> +./hook-example.sh
Is hook-example.sh necessary here? If it is supposed to serve as
an example, then there is no need to call it each time wrapper
executes. If it is supposed to be a sanity check, then a more
suited name would be in order (hook-sanity-check.sh or
hook-date.sh).

-- 
Best Regards,
Krzysztof

Re: [PATCH i-g-t v4 3/4] intel-ci/hooks: Add hooks-wrapper and aux scripts/allowlists

[Zbigniew Kempczyński]

On Tue, Apr 21, 2026 at 08:15:48AM +0000, Krzysztof Karas wrote:
> Hi Zbigniew,
>  
> [...]
> 
> > diff --git a/tests/intel-ci/hooks/common.helper b/tests/intel-ci/hooks/common.helper
> > new file mode 100755
> > index 0000000000..a3f913f8e8
> > --- /dev/null
> > +++ b/tests/intel-ci/hooks/common.helper
> > @@ -0,0 +1,47 @@
> > +# Common helper for hooks executed from igt_runner
> > +#
> > +# Provide ALLOWLIST="..." as a default, it might be overwritten by --allowlist
> Maybe instead of "Provide", which to me looks like you expect
> an argument, "Set" would be better suited here?
> It is just aesthetics, so I'd not block over this.

Ok, I'll rephrase a bit as I've just realized that if ALLOWLIST wasn't
provided and --allowlist wasn't set we won't have ALLOWLIST at all,
so helper should exit in this case.

> 
> > +
> > +for log in $(find /sys/kernel/debug/dri -iname 'guc_log'); do
> > +	attout=$(echo ${log:23} | sed -e 's/\//_/g')
> > +	mkdir -p "${IGT_HOOK_TEST_FULLNAME}"
> > +	cp "$log" "${IGT_HOOK_TEST_FULLNAME}/${attout}"
> > +done
> > diff --git a/tests/intel-ci/hooks/hook-example.allowlist b/tests/intel-ci/hooks/hook-example.allowlist
> > new file mode 100644
> > index 0000000000..54cbc0a1d4
> > --- /dev/null
> > +++ b/tests/intel-ci/hooks/hook-example.allowlist
> > @@ -0,0 +1,2 @@
> > +igt@xe_exec_basic@once-basic@rcs0
> > +igt@xe_exec_basic@once-basic@ccs0
> > diff --git a/tests/intel-ci/hooks/hook-example.sh b/tests/intel-ci/hooks/hook-example.sh
> > new file mode 100755
> > index 0000000000..512e123567
> > --- /dev/null
> > +++ b/tests/intel-ci/hooks/hook-example.sh
> > @@ -0,0 +1,17 @@
> > +#!/bin/bash
> > +
> > +# Hook example, touch <testname>/current-date.txt in attachments dir
> > +#
> > +# Suggested usage with:
> > +# --hook 'post-subtest:tests/intel-ci/hooks/hook-example.sh' \
> > +# --hook 'post-dyn-subtest:tests/intel-ci/hooks/hook-example.sh'
> > +# or within hooks-wrapper.sh
> > +
> > +ALLOWLIST="hook-example.allowlist"
> > +SCRIPTDIR=$(dirname "$(realpath "$0")")
> > +. "${SCRIPTDIR}/common.helper"
> > +
> > +cd "${IGT_RUNNER_ATTACHMENTS_DIR}"
> > +
> > +mkdir -p "${IGT_HOOK_TEST_FULLNAME}"
> > +date > "${IGT_HOOK_TEST_FULLNAME}/current-date.txt"
> > diff --git a/tests/intel-ci/hooks/hooks-wrapper.sh b/tests/intel-ci/hooks/hooks-wrapper.sh
> > new file mode 100755
> > index 0000000000..263b81a8b7
> > --- /dev/null
> > +++ b/tests/intel-ci/hooks/hooks-wrapper.sh
> > @@ -0,0 +1,12 @@
> > +#!/bin/bash
> > +
> > +# Hooks wrapper
> > +# Suggested usage with:
> > +# --hook 'post-subtest:tests/intel-ci/hooks/hooks-wrapper.sh' \
> > +# --hook 'post-dyn-subtest:tests/intel-ci/hooks/hooks-wrapper.sh'
> > +
> > +SCRIPTDIR=$(dirname "$(realpath "$0")")
> > +
> > +cd "${SCRIPTDIR}"
> > +./hook-example.sh
> Is hook-example.sh necessary here? If it is supposed to serve as
> an example, then there is no need to call it each time wrapper
> executes. If it is supposed to be a sanity check, then a more
> suited name would be in order (hook-sanity-check.sh or
> hook-date.sh).

If rename to hook-date.sh then commenting it out in hooks-wrapper.sh
would be ok? I would like to show an example how scripts can use
common.helper to process allowlists.

Thanks for the review.

--
Zbigniew

> 
> -- 
> Best Regards,
> Krzysztof

[PATCH i-g-t v4 4/4] runner/resultgen: Insert attachments list into results.json

[Zbigniew Kempczyński]

Add list of filenames which were created by hooks in attachments directory
(like GuC log copy) to results.json for allow presenting it in CI.

Due to lack of userdata pointer in nftw() implementation main json
object is passed via temporary static variable. It shouldn't be the
problem because results.json is created in single thread. This change
is not too elegant but allows to minimize the code change and is
much easier to read comparing to recursive readdir().

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Ryszard Knop <ryszard.knop@intel.com>
Cc: Krzysztof Karas <krzysztof.karas@intel.com>
---
v4: use switch/case (Gustavo)
---
 runner/resultgen.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/runner/resultgen.c b/runner/resultgen.c
index 29fc5cbb15..34253bc735 100644
--- a/runner/resultgen.c
+++ b/runner/resultgen.c
@@ -1,11 +1,14 @@
 #include <assert.h>
 #include <ctype.h>
+#include <dirent.h>
 #include <fcntl.h>
+#include <ftw.h>
 #include <inttypes.h>
 #include <stdio.h>
 #include <string.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
+#include <sys/types.h>
 #include <unistd.h>
 
 #include <jansson.h>
@@ -271,6 +274,19 @@ static struct json_t *get_or_create_json_object(struct json_t *base, const char
 	return ret;
 }
 
+static struct json_t *get_or_create_json_array(struct json_t *base, const char *key)
+{
+	struct json_t *ret = json_object_get(base, key);
+
+	if (ret)
+		return ret;
+
+	ret = json_array();
+	json_object_set_new(base, key, ret);
+
+	return ret;
+}
+
 static void set_result(struct json_t *obj, const char *result)
 {
 	if (!result)
@@ -1142,6 +1158,66 @@ static bool fill_from_dmesg(int fd,
 	return true;
 }
 
+struct json_t *tmp_tests;
+static int ftw_attachments_list(const char *fpath, const struct stat *sb,
+				int typeflag, struct FTW *ftwbuf)
+{
+	struct json_t *obj = NULL, *attobj = NULL;
+
+	(void)sb;
+	(void)ftwbuf;
+
+	switch (typeflag) {
+	case FTW_F:
+		char *p, *currpath = (char *) fpath + 2;
+
+		p = strstr(currpath, "/");
+		if (!p)
+			return -1;
+		*p = '\0'; /* temporary for acquiring the piglit name */
+		obj = get_or_create_json_object(tmp_tests, currpath);
+		attobj = get_or_create_json_array(obj, "attachments");
+		*p = '/'; /* bring '/' back */
+		json_array_append_new(attobj, escaped_json_stringn(currpath, strlen(currpath)));
+		break;
+	case FTW_DP:
+		break;
+	default:
+		return -1;
+	}
+
+	return 0;
+}
+
+static bool fill_from_attachments(int idirfd, struct json_t *tests)
+{
+	char attname[32];
+	int attdirfd;
+	DIR *currdir;
+	int ret;
+
+	snprintf(attname, sizeof(attname), "%s", DIR_ATTACHMENTS);
+	if ((attdirfd = openat(idirfd, attname, O_DIRECTORY | O_RDONLY | O_CLOEXEC)) < 0) {
+		fprintf(stderr, "Error opening '%s' dir\n", DIR_ATTACHMENTS);
+		return false;
+	}
+
+	currdir = opendir(".");
+	fchdir(attdirfd);
+
+	/*
+	 * ftw doesn't support passing user data so *tests has to be
+	 * set to some global for being visible in callback function.
+	 * As results.json is not processed in multiple threads it is
+	 * not a big problem.
+	 */
+	tmp_tests = tests;
+	ret = nftw(".", ftw_attachments_list, 4, FTW_PHYS | FTW_DEPTH);
+	fchdir(dirfd(currdir));
+
+	return ret ? false : true;
+}
+
 static const char *result_from_exitcode(int exitcode)
 {
 	switch (exitcode) {
@@ -2230,6 +2306,9 @@ static bool parse_test_directory(int dirfd,
 		fprintf(stderr, "Error parsing output files (dmesg.txt)\n");
 	}
 
+	if (!fill_from_attachments(dirfd, results->tests))
+		fprintf(stderr, "Error parsing attachments directory\n");
+
 	override_results(entry->binary, &subtests, results->tests);
 	prune_subtests(settings, entry, &subtests, results->tests);
 
-- 
2.43.0

Re: [PATCH i-g-t v4 4/4] runner/resultgen: Insert attachments list into results.json

[Krzysztof Karas]

Hi Zbigniew,

On 2026-04-20 at 10:56:58 +0200, Zbigniew Kempczyński wrote:
> Add list of filenames which were created by hooks in attachments directory
> (like GuC log copy) to results.json for allow presenting it in CI.
> 
> Due to lack of userdata pointer in nftw() implementation main json
> object is passed via temporary static variable. It shouldn't be the
> problem because results.json is created in single thread. This change
> is not too elegant but allows to minimize the code change and is
> much easier to read comparing to recursive readdir().
> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Ryszard Knop <ryszard.knop@intel.com>
> Cc: Krzysztof Karas <krzysztof.karas@intel.com>
> ---

Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com>

-- 
Best Regards,
Krzysztof