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

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

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

> 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>
> ---
>  runner/resultgen.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/runner/resultgen.c b/runner/resultgen.c
> index f8459385c3..d37f01a433 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>
> @@ -1156,6 +1159,63 @@ 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;
> +
> +	if (typeflag == FTW_F) {
> +		char *p;
> +
> +		p = strstr(fpath + 2, "/");
> +		if (!p)
> +			return -1;
> +		*p = '\0'; /* temporary for acquiring the piglit name */
> +		obj = get_or_create_json_object(tmp_tests, fpath + 2);

For this to be trully recursive, I think we should create the JSON
object in the FTW_DP case.

> +		attobj = get_or_create_json_array(obj, "attachments");
> +		*p = '/'; /* bring '/' back */
> +		json_array_append_new(attobj, escaped_json_stringn(fpath + 2, strlen(fpath + 2)));
> +	} else if (typeflag == FTW_DP) {
> +		;
> +	} else {
> +		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);

I think another option is to use fts(3)? Since it is a streaming API,
you can keep the state (i.e. the current json object) as a local
variable and update as you walk the tree.

It seems it is not POSIX though.  Not sure if that's a real issue for
IGT, since stuff in IGT are supposed to run on Linux, right?

Thinking back, I guess it could fts(3) could even be used for the
recursive removal.

--
Gustavo Sousa

> +	fchdir(dirfd(currdir));
> +
> +	return ret ? false : true;
> +}
> +
>  static const char *result_from_exitcode(int exitcode)
>  {
>  	switch (exitcode) {
> @@ -2244,6 +2304,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