Re: [PATCH] support: Add support for delayed test failure reporting

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

> +/* This structure keeps track of test failures.  The counter is
> +   incremented on each failure.  The failed member is set to true if a
> +   failure is detected, so that even if the counter wraps around to
> +   zero, the failure of a test can be detected.
> +
> +   The init constructor function below puts *state on a shared
> +   annonymous mapping, so that failure reports from subprocesses
> +   propagate to the parent process.  */
> +struct test_failures
> +{
> +  unsigned counter;
> +  unsigned failed;

Shouldn't these use 'unsigned int' based on code style?

> +};
> +static struct test_failures *state;
> +
> +static __attribute__ ((constructor)) void
> +init (void)
> +{
> +  void *ptr = mmap (NULL, sizeof (*state), PROT_READ | PROT_WRITE,
> +                    MAP_ANONYMOUS | MAP_SHARED, -1, 0);
> +  if (ptr == MAP_FAILED)
> +    {
> +      printf ("error: could not map %zu bytes: %m\n", sizeof (*state));
> +      exit (1);
> +    }

Why not use FAIL_EXIT1 (also for other failures cases as well)?

> +static int
> +do_test (void)
> +{
> +  if (exit_status_with_failure >= 0)
> +    {
> +      /* External invocation with requested error status.  Used by
> +         tst-support_report_failure-2.sh.  */
> +      support_record_failure ();
> +      return exit_status_with_failure;
> +    }
> +  TEST_VERIFY (true);
> +  TEST_VERIFY_EXIT (true);
> +  if (test_verify)
> +    {
> +      TEST_VERIFY (false);
> +      return 2; /* Expected exit status.  */
> +    }
> +  if (test_verify_exit)
> +    {
> +      TEST_VERIFY_EXIT (false);
> +      return 3; /* Not reached.  Expected exit status is 1.  */
> +    }
> +
> +  printf ("info: This test tests the test framework.\n"
> +          "info: It reports some expected errors on stdout.\n");
> +
> +  /* Check that the status is passed through unchanged.  */
> +  check_failure_reporting (1, 0, EXIT_UNSUPPORTED);

I am not very found of plain number to indicate the arguments (it tends to make
code harder to read), so I would suggest for a future clean up to add either some 
enum or defined to indicate the phase (like PHASE{1,...}) and same for the zero
argument.