Re: [PATCH RFC 1/3] selftests/nolibc: add custom test harness

[Thomas Weißschuh]

On 2023-11-16 08:16:22+0100, Willy Tarreau wrote:
> Hi Thomas,
> 
> On Wed, Nov 15, 2023 at 10:08:19PM +0100, Thomas Weißschuh wrote:
> > The harness provides a framework to write unit tests for nolibc itself
> > and kernel selftests using nolibc.
> > 
> > Advantages over the current harness:
> > * Makes it possible to emit KTAP for integration into kselftests.
> > * Provides familiarity with the kselftest harness and google test.
> > * It is nicer to write testcases that are longer than one line.
> > 
> > Design goals:
> > * Compatibility with nolibc. kselftest-harness requires setjmp() and
> >   signals which are not supported on nolibc.
> > * Provide the same output as the existing unittests.
> > * Provide a way to emit KTAP.
> > 
> > Notes:
> > * This differs from kselftest-harness in its support for test suites,
> >   the same as google test.
> >
> > Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> 
> Nice intro to present the benefits, but you forgot to explain what
> the patch itself does among these points, the decisions you took,
> tradeoffs if any etc. All of these are particularly important so as
> to figure what to expect from the patch itself, because, tob be
> honest, for me it's a bit difficult to estimate the suitability of
> the code for a given purpose, thus for now I'll mostly focus on
> general code.

Good points. I'll expand more in v2 after we are through this round.

> A few comments below:
> 
> > +static void putcharn(char c, size_t n)
> > +{
> > +	char buf[64];
> > +
> > +	memset(buf, c, n);
> > +	buf[n] = '\0';
> > +	fputs(buf, stdout);
> > +}
> 
> You should really check that n < 64 here, not only because it's test
> code that will trigger about any possible bug around, but also because
> you want others to easily contribute and not get trapped by calling
> this with a larger value without figuring it will do whatever. And
> that way you can remove the tests from the callers which don't need
> to hard-code this limit.

Ack.

> 
> > +#define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
> > +#define is_pointer_type(var)	(__builtin_classify_type(var) == 5)
> 
> The hard-coded "5" above should either be replaced with pointer_type_class
> (if available here) or left as-is with a comment at the end of the line
> saying e.g. "// pointer_type_class" so that the value can be looked up
> more easily if needed.

Ack.