Re: [PATCH] vfprint: validate nargs and argument-based offsets

[Roland McGrath <roland at hack dot frob dot com>]

> +	 tst-vfprintf-nargs

We tend to call them bug-foo when they are regression tests for particular
bugs found in the past, and tst-foo when they are preemptively-written
tests.  But there is no real consistency about that and nobody really cares.

This really warrants a bugzilla report.  Then you should put:
	[BZ #nnn]
at the top of the ChangeLog stanza.  It's probably also worthwhile to
mention the bug reference inside the test case source.

> +int

Make a function static whenever it can be.

> +format_failed(const char *fmt, const char *expected, int wanted)

Space before paren (here and many other places).

> +  /* Since the stack could be extremely wrecked by this test, use
> +     an external supervisor process to catch the signals, since a
> +     signal handler may not be able to recover.
> +   */

*/ goes on the preceding line, after two spaces.

I'll give some more style comments below for future reference.
But this part doesn't actually belong here.  Use the test-skeleton.c
framework instead.

> +  if (pid)

We usually eschew implicit Boolean conversions, so write "if (pid != 0)"
(or > 0, here).

> +      waitpid(pid, &status, 0);

Fails to check the return value.

> +          fprintf(stderr, "Unexpected failure\n");

We generally use fputs rather than fprintf for constant strings.

> +  memset(output, 0, sizeof(output));

We generally use "sizeof variable" rather than "sizeof (variable)".
The parens are only required when it's a type name.

> +  /* Positional arguments are constructed via read_int(), so nargs
> +     can only overflow on 32bit systems. On 64bit systems, it will
> +     attempt to allocate a giant amount of stack memory and crash,
> +     which is the expected situation.  */

Two spaces after every sentence.  Say "64-bit", not "64bit".

> +  if (sizeof(long) == 4)

We eschew implicit "int", i.e. use "long int" instead of "long".

Contrary to what Ryan said, I think it's generally preferable to use if
rather than #if whenever you can.  That avoids compile-time bit rot in the
code path that someone doesn't remember to update and doesn't always test.

> +  if (format_failed(buf, "1 %$d", wanted)) rc = 1;

Never put the consequent on the same line.

> +    /* Check for potential integer overflow. */

Still need two spaces after the sentence here, and below.


Thanks,
Roland