On Tue, May 24, 2022 at 3:10 PM Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>
> 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
Yes, I've seen that.
> 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.
Makes sense to me, but I think the practice for cleanup should be
(like in fsstress_cleanup) to send SIGKILL and wait in case
the processes are ignoring or already handling SIGTERM.
>
> 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.
>
Great.
So w.r.t this patch there are only minor comments of kill -9
and move unset of create_pids.
Take it or leave it, either way you may add:
Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
Thanks,
Amir.
