Re: [PATCH v1] s390x: add stidp interception test

["Radim Krčmář" <rkrcmar@redhat.com>]

2017-06-19 14:21+0200, David Hildenbrand:
> On 19.06.2017 12:44, Thomas Huth wrote:
> > On 19.06.2017 11:15, David Hildenbrand wrote:
> >> @@ -105,6 +105,32 @@ static void test_stap(void)
> >> +	report("id.version == 0 || id.version == 0xff)",
> > 
> > Superfluous parenthesis -----------------------------^
> 
> Whops, yes, thanks.

This pattern could use a macro ;)

  #define report(x) report(#x, x)

> >> +	       id.version == 0 || id.version == 0xff);
> >> +	report("id.reserved == 0", !id.reserved);
> >
> > I also think you should not use C code in the text output here.
> > Not everybody who's running the kvm-unit-tests might be familiar with
> > the C language syntax. So it'd be nicer to write something like
> > "reserved bits are zero" instead of "id.reserved == 0" ?
> 
> That's also what we have in s390x/selftest.c, suggested by Radim.

The main purpose is to find the place that failed in the code (any
unique string qualifies), but it is desirable to explain what is tested.
Please use a simplification in natural language if you feel that it
provides better information.

(For s390x/selftest.c, I was mainly proposing a different code structure
 and unique messages.  Using C-like code to describe the format of
 arguments just allowed the thing to fit on one line while remaining
 understandable, IMO.)

> If someone cannot read C code, that person for sure will also not be
> able to fix that bug.

Well said.  The audience is expected to understand the test and the
source code of the condition looks ok here.