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

[Gustavo Sousa <gustavo.sousa@intel.com>]

Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> writes:

> On Mon, Mar 30, 2026 at 11:20:46AM -0300, Gustavo Sousa wrote:
>> Zbigniew Kempczyński <zbigniew.kempczynski@intel.com> writes:
>> 
>> > 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 finally 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)
>> > ---
>> >  lib/igt_hook.c    |  4 ++++
>> >  runner/executor.c | 44 +++++++++++++++++++++++++++++++++++++++++---
>> >  runner/executor.h |  2 ++
>> >  3 files changed, 47 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/lib/igt_hook.c b/lib/igt_hook.c
>> > index f86ed56f72..57817bdc12 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\
>> 
>> Nitpick: s/igt_runner environment/igt_runner, environment/
>
> Ack.
>
>> 
>> > +is passed additionally to the hook script. It contains directory where\n\
>> > +script may write additional attachments like guc logs, etc.\n\
>> 
>> Looking at how this is implemented, the attachments directory is not a
>> hook-specific thing, so I believe documentation for it would be better
>> placed in igt_runner's help/docs.
>
> That's true, I placed it here because --help-hook is first place where
> I would look for environment vars I can use. What do you think to place
> information about this variable in both places?

Well, we could have a section listing environment variables associated
with igt_runner in its help output and refer to that section here.

>
>> 
>> >  ", option_name);
>> >  }
>> > diff --git a/runner/executor.c b/runner/executor.c
>> > index 1485b59d1f..bc421f7dbb 100644
>> > --- a/runner/executor.c
>> > +++ b/runner/executor.c
>> > @@ -1779,17 +1779,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 +1875,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 */
>> > @@ -1950,7 +1956,10 @@ static int remove_file(int dirfd, const char *name)
>> >  
>> >  static bool clear_test_result_directory(int dirfd)
>> >  {
>> > -	int i;
>> > +	DIR *dir;
>> > +	struct dirent *entry;
>> > +	int i, adirfd;
>> > +	int ret = false;
>> >  
>> >  	for (i = 0; i < _F_LAST; i++) {
>> >  		if (remove_file(dirfd, filenames[i])) {
>> > @@ -1960,7 +1969,33 @@ static bool clear_test_result_directory(int dirfd)
>> >  		}
>> >  	}
>> >  
>> > -	return true;
>> > +	if ((adirfd = openat(dirfd, DIR_ATTACHMENTS, O_DIRECTORY | O_RDONLY | O_CLOEXEC)) < 0) {
>> > +		errf("Cannot open attachments directory\n");
>> > +		return false;
>> > +	}
>> > +
>> > +	if ((dir = fdopendir(adirfd)) == NULL) {
>> > +		errf("Cannot fdopen attachments directory\n");
>> > +		goto out1;
>> > +	}
>> > +
>> > +	while ((entry = readdir(dir)) != NULL) {
>> > +		if (!strcmp(entry->d_name, ".") || !strcmp(entry->d_name, ".."))
>> > +			continue;
>> > +
>> > +		if (unlinkat(adirfd, entry->d_name, 0))  {
>> > +			errf("Error removing %s\n", entry->d_name);
>> > +			goto out2;
>> > +		}
>> > +	}
>> 
>> Since we can't predict that the attachments directory will remain flat,
>> I think it would be safer to do a recursive cleanup here.  I guess we
>> could use nftw() in a post-order fashion here?
>
> I wondered about recursive removal, what prevented me from doing this
> was if there'll bug here I can remove too much. At the moment I guess
> guc logs will be only user so I would try to stay in flatten structure.

I don't think we should assume that's going to be the only user.  The
end user can basically run "anything" as a hook and could as well create
subdirectories.

The alternative is to consider the attachments dir something not to be
messed by the user and consider only "official" hook scripts (i.e. those
in IGT's source tree) as "authorized" to handle it.  But I believe
that's not the original intent, right?

--
Gustavo Sousa

>
> --
> Zbigniew
>
>> 
>> --
>> Gustavo Sousa
>> 
>> > +
>> > +	ret = true;
>> > +out2:
>> > +	closedir(dir);
>> > +out1:
>> > +	close(adirfd);
>> > +
>> > +	return ret;
>> >  }
>> >  
>> >  static bool clear_old_results(char *path)
>> > @@ -2002,6 +2037,9 @@ static bool clear_old_results(char *path)
>> >  			close(dirfd);
>> >  			return false;
>> >  		}
>> > +		if (unlinkat(resdirfd, DIR_ATTACHMENTS, AT_REMOVEDIR))
>> > +			errf("Warning: Cannot remove attachments directory\n");
>> > +
>> >  		close(resdirfd);
>> >  		if (unlinkat(dirfd, name, AT_REMOVEDIR)) {
>> >  			errf("Warning: Result directory %s contains extra files\n",
>> > 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