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

[Florian Weimer <fweimer at redhat dot com>]

On 12/28/2016 02:04 PM, Adhemerval Zanella wrote:
> > +/* 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?

You are correct.

> > +};
> > +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)?

We might want to add implicit delayed error reporting to FAIL_EXIT1 
eventually, so that it works from subprocess as well.  In that case, 
this would introduce a circular dependency.

> > +  /* 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.

The phase number is just there so that it easier to find the exact place 
where a test failure occurred.  I could perhaps replace it with 
__LINE__, or maybe with a global variable.  Adding an enum isn't really 
helpful because the values have no intrinsic meaning.  I agree that the 
collection of int argument is a bit unfortunate.

Thanks,
Florian