On Tue, May 24, 2022 at 12:41:25PM +0300, Amir Goldstein wrote:
> On Tue, May 24, 2022 at 12:12 PM Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> >
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > When I ctrl-c g/038, it either does nothing or it leaves processes
> > running in the background. It is not cleaning up it's background
> > processes correctly, so add kill vectors into the cleanup. Make sure
> > we only kill in the cleanup trap if the background processes are
> > running.
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> > tests/generic/038 | 16 ++++++++++------
> > 1 file changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/generic/038 b/tests/generic/038
> > index c6cea94e..0462ea13 100755
> > --- a/tests/generic/038
> > +++ b/tests/generic/038
> > @@ -36,6 +36,10 @@ _begin_fstest auto stress trim
> > # Override the default cleanup function.
> > _cleanup()
> > {
> > + [ -n "${create_pids}" ] && kill ${create_pids[@]}
> > + [ -n "${fallocate_pids}" ] && kill ${fallocate_pids[@]}
> > + [ -n "${trim_pids}" ] && kill ${trim_pids[@]}
> > + wait
>
> Following the pattern of recently fixed generic/019,
> Please redirect stderr of kill to /dev/null
No. Redirecting errors to /dev/null to hide them is poor behaviour
- g/019 does not test if the pids variables are still valid even
though they get unset. Hence the normal exit trap
runs an empty `kill` command which outputs a help message. The
redirect to /dev/null hides this broken behaviour - it's poor,
buggy code, and an example how buggy and broken a lot of the cleanup
code actually is.
The code above will not run kill with an empty pid list, hence it
should not fail when it is run. If it does fail then we've got a
problem that needs to be captured and analysed and so redirecting
that error to /dev/null is exactly the wrong thing to be doing.
> and I think we would rather not wait in cleanup callbacks
> which could potentially block forever?
We do that in many, many tests - that's how we stop the test leaving
background processes running after the test exits. And if it hangs
here waiting on a hung process, then as a developer using this to
find bugs in my code, I really do want the test to hang hard so I
notice it and can capture the failure and analyse it.
Leaving a warm corpse to post-morten is the desired behaviour of a
failed test.
> > rm -fr $tmp
>
> I suppose another patch is going to replace that with the proper _cleanup()?
> Patches from vger have been VERY slowly trickling into my mailbox.
Not in this patch series. When I tackle the generic tests I'll
address these bugs. This patch is just fixing broken ctrl-c
behaviour.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
