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

["Ser, Simon" <simon.ser@intel.com>]

On Thu, 2019-07-04 at 14:22 +0300, Arkadiusz Hiler wrote:
> 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@intel.com>
> > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > > Acked-by: Petri Latvala <petri.latvala@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.

I see, fine by me then

> <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);
> 	}
> }

Looks good minus the typo.

(Also the else branch is unnecessary, but this is not important.)

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

Sorry, I meant check for ret >= 0.

> > Could also check for WIFEXITED, since WEXITSTATUS only works in this
> > case.
> 
> I'll add internal_assert(WIFEXITED(status));
> 
> Thanks for the review!
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev