Re: [PATCH i-g-t v3 2/5] 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)
> v3: unify 'attdir*' variables (Krzysztof)
>     do recursive removal in attachments directories (Gustavo)
> ---
>  lib/igt_hook.c    |  4 +++
>  runner/executor.c | 75 +++++++++++++++++++++++++++++++++++++++++++++--
>  runner/executor.h |  2 ++
>  3 files changed, 78 insertions(+), 3 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..af39b12aea 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,9 +1955,61 @@ static int remove_file(int dirfd, const char *name)
>  	return unlinkat(dirfd, name, 0) && errno != ENOENT;
>  }
>  
> +static int ftw_attachment_removal(const char *fpath, const struct stat *sb,
> +				  int typeflag, struct FTW *ftwbuf)
> +{
> +	(void)sb;
> +	(void)ftwbuf;

I was able to compile without warnings after dropping this.  Is there a
reason to keep them here?

> +
> +	if (typeflag == FTW_F) {
> +		if (unlink(fpath)) {
> +			errf("Cannot remove file %s\n", fpath);

Maybe append strerror(errno) to the message here?

> +			return -1;
> +		}
> +	} else if (typeflag == FTW_DP) {
> +		if (rmdir(fpath)) {
> +			errf("Cannot remove directory %s\n", fpath);

Ditto.

> +			return -1;
> +		}
> +	} else {
> +		errf("Cannot remove %s (unsupported file type)\n", fpath);
> +		return -1;

I think it would be sensible to drop symbolic links as well (I think it
could be handled in the same brace for FTW_F.

> +	}

I think this would look nicer if it was handled with a "switch"
statement.

> +
> +	return 0;
> +}
> +
> +static bool clear_attachments_dir_recursively(int attdirfd)
> +{
> +	char attdirname[PATH_MAX];
> +	DIR *currdir;
> +	int ret;
> +
> +	currdir = opendir(".");

Why do we need this call?

> +	fchdir(attdirfd);
> +	getcwd(attdirname, sizeof(attdirname));
> +
> +	/* Sanity check we're in attachments directory structure */
> +	if (!strstr(attdirname, DIR_ATTACHMENTS)) {
> +		errf("Cowardly refusing removing in directory which is not "
> +		     "attachments directory [currdir: %s]!\n", attdirname);
> +		ret = -1;
> +		goto out;
> +	}

This function is static and only called for DIR_ATTACHMENTS.  I believe
this check is unnecessary.

> +
> +	ret = nftw(attdirname, ftw_attachment_removal, 4, FTW_PHYS | FTW_DEPTH);
> +
> +out:
> +	fchdir(dirfd(currdir));
> +	closedir(currdir);
> +
> +	return ret ? false : true;
> +}
> +
>  static bool clear_test_result_directory(int dirfd)

I think we need to have the path to the test result dir passed as
argument here so that we can build attdirname without having the change
dir + get cwd dance.

>  {
> -	int i;
> +	int i, attdirfd;
> +	int ret = true;

I guess we don't need this ret variable. If the removal of the
attachments directory fails, we can just print an error message and
return false.

>  
>  	for (i = 0; i < _F_LAST; i++) {
>  		if (remove_file(dirfd, filenames[i])) {
> @@ -1960,7 +2019,16 @@ static bool clear_test_result_directory(int dirfd)
>  		}
>  	}
>  
> -	return true;
> +	if ((attdirfd = openat(dirfd, DIR_ATTACHMENTS, O_DIRECTORY | O_RDONLY | O_CLOEXEC)) < 0) {
> +		errf("Cannot open attachments directory\n");
> +		return false;
> +	}
> +
> +	ret = clear_attachments_dir_recursively(attdirfd);
> +	if (!ret)
> +		errf("Cannot clear attachments dir recursively\n");

Why not simply call nftw() directly from here? I don't see much value in
having the function clear_attachments_dir_recursively().

--
Gustavo Sousa

> +
> +	return ret;
>  }
>  
>  static bool clear_old_results(char *path)
> @@ -2002,6 +2070,7 @@ static bool clear_old_results(char *path)
>  			close(dirfd);
>  			return false;
>  		}
> +
>  		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