Re: [PATCH v2 1/4] test-tool: help verifying BUG() code paths

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]

Hi Duy,

On Wed, 2 May 2018, Duy Nguyen wrote:

> On Wed, May 2, 2018 at 11:38 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > When we call BUG(), we signal via SIGABRT that something bad happened,
> > dumping cores if so configured. In some setups these coredumps are
> > redirected to some central place such as /proc/sys/kernel/core_pattern,
> > which is a good thing.
> >
> > However, when we try to verify in our test suite that bugs are caught in
> > certain code paths, we do *not* want to clutter such a central place
> > with unnecessary coredumps.
> >
> > So let's special-case the test helpers (which we use to verify such code
> > paths) so that the BUG() calls will *not* call abort() but exit with a
> > special-purpose exit code instead.
> >
> > Helped-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/helper/test-tool.c | 2 ++
> >  usage.c              | 5 +++++
> >  2 files changed, 7 insertions(+)
> >
> > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
> > index 87066ced62a..5176f9f20ae 100644
> > --- a/t/helper/test-tool.c
> > +++ b/t/helper/test-tool.c
> > @@ -47,7 +47,9 @@ static struct test_cmd cmds[] = {
> >  int cmd_main(int argc, const char **argv)
> >  {
> >         int i;
> > +       extern int BUG_exit_code;
> >
> > +       BUG_exit_code = 99;
> 
> It may be even better to let individual tests in t1406 control this,
> pretty much like your original patch, except that we tell usage.c to
> not send SIGABRT and just return a special fault code. That way we
> don't accidentally suppress BUG()'s sigabrt behavior  in tests that do
> not anticipate it (even in t1406).

I thought long and hard about this, even slept over it. And I came to the
conclusion that I do not really know whether we want such a special
treatment (you may even want to go crazier and limit *which* BUG() call
you intend to catch, so that others are still reported).

And I came to an important realization: whether or not to handle the bugs
in the way you described is actually *outside* the purpose of this patch
series. This patch series is really only about converting die(BUG:...)
calls to BUG() calls. And it simply leaves the concern you raised for
another patch series.

> But this patch is ok for me too if you don't want another reroll.

I don't ;-) (for the reasons mentioned above: I don't disagree with you, I
just think it should be addressed in another patch series than this here
patch series).

Ciao,
Dscho