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:

> 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/

> +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.

>  ", 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?

--
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