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:

> On Tue, Apr 14, 2026 at 06:39:28PM -0300, Gustavo Sousa wrote:
>> 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.
>
> What for? Imo attachments array for test/subtest which contains
> relative paths from test/subtest is enough. I see no value by adding
> recursive arrays apart of increasing complexity to the code. Be aware CI has
> to present these attachments as well, so more we complicate results.json
> more work will be on CI side to alter javascript.
>
>> 
>> > +		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.
>
> CI folks want minimize changes in results.json, so according to above
> I would keep everything in flat attachments array.
>
>> 
>> 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?
>
> And Android if I'm not wrong.

That's Linux as well, no?  Perhaps just need to have a sanity check to
see if those functions are also available there.

>
>> 
>> Thinking back, I guess it could fts(3) could even be used for the
>> recursive removal.
>
> Question - do we really need this? I can live with ugly static
> json at cost of simplicity especially this is only result generator
> which likely won't be touched by next months/years...

I would say: if the right tool is available for the job, why not using
it?

Not going to block on this though...

>
> For attachments case imo allowing recursive creation from hooks scripts
> is an overkill.

The hook mechanism is not something created for exclusive use by CI. It
is a tool for developers to use are they see fit.  That was my original
intention when I first developed the hooks infrastructure.

I don't like the fact that we are making assumptions of how the hooks
will behave here.

If CI wants a flat structure, that's fine: let CI provide hooks that
generate files in a flat hierarchy.  I just think that it is not right
to assume that all hooks will behave like that.

--
Gustavo Sousa

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