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

Ser, Simon
2019-07-04T11:33:47Z

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