[igt-dev] [PATCH i-g-t 1/5] lib/igt_core: Add support for subtest descriptions

[Arkadiusz Hiler]

On Wed, Jul 03, 2019 at 04:08:54PM +0300, Ser, Simon wrote:
> Overall looks good. A few comments inline.
> 
> On Mon, 2019-07-01 at 15:21 +0300, Arkadiusz Hiler wrote:
> > This patch adds igt_description() which attaches a description to the
> > following igt_subtest or igt_subtest_group block.
> > 
> > Descriptions are accessible via './test --describe[=pattern]'
> > 
> > Subtest description is its own igt_describe as well as igt_describes
> > of all the parenting igt_subtest_groups, starting from the outermost
> > scope.
> > 
> > Examples of code and produced outputs are included in
> > lib/test/igt_describe.c and as a documentation comment on igt_describe()
> > macro.
> > 
> > Cc: Petri Latvala <petri.latvala at intel.com>
> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> > Acked-by: Petri Latvala <petri.latvala at intel.com>
> > ---
> >  lib/igt_core.c           | 179 ++++++++++++++++++++++++--
> >  lib/igt_core.h           | 137 ++++++++++++++++++--
> >  lib/tests/igt_describe.c | 266 +++++++++++++++++++++++++++++++++++++++
> >  lib/tests/meson.build    |   1 +
> >  4 files changed, 562 insertions(+), 21 deletions(-)
> >  create mode 100644 lib/tests/igt_describe.c
> > 
> > diff --git a/lib/igt_core.c b/lib/igt_core.c
> > index 6b9f0425..8b1c7b2f 100644
> > --- a/lib/igt_core.c
> > +++ b/lib/igt_core.c
> > @@ -70,6 +70,7 @@
> >  #include "igt_sysfs.h"
> >  #include "igt_sysrq.h"
> >  #include "igt_rc.h"
> > +#include "igt_list.h"
> >  
> >  #define UNW_LOCAL_ONLY
> >  #include <libunwind.h>
> > @@ -259,6 +260,7 @@ const char *igt_interactive_debug;
> >  
> >  /* subtests helpers */
> >  static bool list_subtests = false;
> > +static bool describe_subtests = false;
> >  static char *run_single_subtest = NULL;
> >  static bool run_single_subtest_found = false;
> >  static const char *in_subtest = NULL;
> > @@ -271,6 +273,16 @@ static enum {
> >  	CONT = 0, SKIP, FAIL
> >  } skip_subtests_henceforth = CONT;
> >  
> > +static char __current_description[4096];
> 
> If it was just me, I'd intentionally make this a lot smaller to make
> sure people don't start writing about their personal life in there.
> 
> I mean: if the description is bigger than 512 chars, you're doing
> something wrong. And 512 chars is probably a very generous limit.
> "Descriptions should fit in a Tweet".

fair point, reduced to 512

<SNIP>
> > +#define __INDENT "  "
> 
> I wouldn't use a macro here. Just adding this to the body of the
> function should be enough:
> 
>     static const indent[] = "  ";

It was a leftover from back when this was defined for a couple of
functions. Fixed.

<SNIP>
> > -void __igt_subtest_group_save(int *save)
> > +
> > +
> > +void __igt_subtest_group_save(int *save, int *desc)
> 
> desc can be bool *.

#define igt_subtest_group for (int igt_tokencat(__tmpint,__LINE__) = 0, \
			       igt_tokencat(__save,__LINE__) = 0, \
			       igt_tokencat(__desc,__LINE__) = 0; \
			       igt_tokencat(__tmpint,__LINE__) < 1 && \
			       (__igt_subtest_group_save(& igt_tokencat(__save,__LINE__), \
							 & igt_tokencat(__desc,__LINE__) ), true); \
			       igt_tokencat(__tmpint,__LINE__) ++, \
			       __igt_subtest_group_restore(igt_tokencat(__save,__LINE__), \
							   igt_tokencat(__desc,__LINE__)))

Because of that it's int. We could make the function take bool, but that
just adds some complications with type casting.

<SNIP>
> > +static void read_whole_pipe(int fd, char *buf, size_t buflen)
> > +{
> > +	ssize_t readlen;
> > +	off_t offset;
> > +
> > +	offset = 0;
> > +	while ((readlen = read(fd, buf+offset, buflen-offset))) {
> 
> Doesn't handle EINTR.

if (readlen == -1) {
	if (errno == EINTR) {
		continue;
	} else {
		printf("read filed with %s\n", strerror(errno));
		exit(1);
	}
}

<SNIP>
> > diff --git a/lib/tests/igt_describe.c b/lib/tests/igt_describe.c
> > new file mode 100644
> > index 00000000..a9f857bb
> > --- /dev/null
> > +++ b/lib/tests/igt_describe.c
<SNIP>
> > +static int _wait(pid_t pid, int *status) {
> > +	int ret;
> > +
> > +	do {
> > +		ret = waitpid(pid, status, 0);
> > +	} while (ret == -1 && errno == EINTR);
> > +
> > +	return ret;
> 
> We could check ret == 0 in this function.

If wait() or waitpid() returns because the status of a child process is
available, these functions shall return a value equal to the process ID
of the child process for which status is reported. If wait() or
waitpid() returns due to the delivery of a signal to the calling
process, -1 shall be returned and errno set to [EINTR]. If waitpid() was
invoked with WNOHANG set in options, it has at least one child process
specified by pid for which status is not available, and status is not
available for any process specified by pid, 0 is returned. Otherwise,
(pid_t)-1 shall be returned, and errno set to indicate the error. 


So I am not really sure why we would check for (ret == 0) here.
Can you elaborate?

> Could also check for WIFEXITED, since WEXITSTATUS only works in this
> case.

I'll add internal_assert(WIFEXITED(status));

Thanks for the review!

-- 
Cheers,
Arek