From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D8D42C433EF for ; Tue, 24 May 2022 12:28:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233592AbiEXM2z (ORCPT ); Tue, 24 May 2022 08:28:55 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43378 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234097AbiEXM2y (ORCPT ); Tue, 24 May 2022 08:28:54 -0400 Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 8933793991 for ; Tue, 24 May 2022 05:28:52 -0700 (PDT) Received: from dread.disaster.area (pa49-181-2-147.pa.nsw.optusnet.com.au [49.181.2.147]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id 730275345CA; Tue, 24 May 2022 22:28:51 +1000 (AEST) Received: from dave by dread.disaster.area with local (Exim 4.92.3) (envelope-from ) id 1ntTec-00FpYt-Gx; Tue, 24 May 2022 22:28:50 +1000 Date: Tue, 24 May 2022 22:28:50 +1000 From: Dave Chinner To: Amir Goldstein Cc: fstests Subject: Re: [RFC PATCH 0/8] fstests: _cleanup() overrides are a mess Message-ID: <20220524122850.GM2306852@dread.disaster.area> References: <20220524073411.1943480-1-david@fromorbit.com> <20220524095755.GH2306852@dread.disaster.area> <20220524101320.GJ2306852@dread.disaster.area> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.4 cv=e9dl9Yl/ c=1 sm=1 tr=0 ts=628ccf83 a=ivVLWpVy4j68lT4lJFbQgw==:117 a=ivVLWpVy4j68lT4lJFbQgw==:17 a=kj9zAlcOel0A:10 a=oZkIemNP1mAA:10 a=7-415B0cAAAA:8 a=ZBbNHa84x776pvyHGYwA:9 a=CjuIK1q_8ugA:10 a=biEYGPWJfzWAr4FL6Ov7:22 Precedence: bulk List-ID: X-Mailing-List: fstests@vger.kernel.org On Tue, May 24, 2022 at 03:14:39PM +0300, Amir Goldstein wrote: > On Tue, May 24, 2022 at 1:13 PM Dave Chinner wrote: > > > > On Tue, May 24, 2022 at 01:01:57PM +0300, Amir Goldstein wrote: > > > On Tue, May 24, 2022 at 12:57 PM Dave Chinner wrote: > > > > > > > > On Tue, May 24, 2022 at 11:29:17AM +0300, Amir Goldstein wrote: > > > > > On Tue, May 24, 2022 at 11:01 AM Dave Chinner wrote: > > > > > > > > > > > > Hi folks, > > > > > > > > > > > > I pulled on a string a couple of days ago, and it got out of > > > > > > control. It all started when I went to kill a test with ctrl-c and > > > > > > it, once again, left background processes running that I had to hunt > > > > > > down and kill manually. > > > > > > > > > > > > I then started looking a why this keeps happening, and realised that > > > > > > the way we clean up on test completion is messy, inconsistent and > > > > > > frequently buggy. So I started cleaning it all up, starting with the > > > > > > tests/xfs directory because I saw a lot of low hanging fruit there. > > > > > > > > > > > > Essentially, we use _cleanup() functions as a way of overriding the > > > > > > default trap handler we install in _begin_fstest(). Rather than > > > > > > register a new handler, we just redefine the common cleanup function > > > > > > and re-implement it (poorly) in every test that does an override. > > > > > > Often these overrides are completely unnecessary - I think I reduced > > > > > > the total number of overrides in tests/xfs by ~30% (~190 -> ~125), > > > > > > and I reudced the number of *unique overrides by a lot more than > > > > > > that. > > > > > > > > > > > > > > > > That looks like an awesome improvement! > > > > > > > > > > > The method for overriding changes to be "stacked cleanups" rather > > > > > > than "duplicated cleanups". That is, tests no longer open code: > > > > > > > > > > > > cd / > > > > > > rm -rf $tmp.* > > > > > > > > > > > > THis is what common/preamble::_cleanup() does. We should call that > > > > > > function to do this. Hence if we have a local cleanup that we need > > > > > > to do, it becomes: > > > > > > > > > > > > local_cleanup() > > > > > > { > > > > > > rm -f $testfile > > > > > > _cleanup > > > > > > } > > > > > > _register_cleanup local_cleanup > > > > > > > > > > While removing boilerplate code, we had better not create another boilerplate. > > > > > Instead of expecting test writers to always call _cleanup > > > > > if we always want _cleanup to be called we can always implicitly > > > > > chain it in _register_cleanup(): > > > > > > > > > > --- a/common/preamble > > > > > +++ b/common/preamble > > > > > @@ -20,7 +20,7 @@ _register_cleanup() > > > > > shift > > > > > > > > > > test -n "$cleanup" && cleanup="${cleanup}; " > > > > > - trap "${cleanup}exit \$status" EXIT HUP INT QUIT TERM $* > > > > > + trap "${cleanup}_cleanup; exit \$status" EXIT HUP INT QUIT TERM $* > > > > > } > > > > > > > > I considered that, but then I found the _no_cleanup cases. IOWs, > > > > this doesn't work for the cases where we want to prevent the generic > > > > _cleanup function from being run on failure/test exit. Hence the > > > > cleanup function stacking behaviour rather than unconditional > > > > calling of _cleanup as per above. > > > > > > > > > > I didn't know about those. > > > Since you went to all this trouble to find them, can you provide a reference. > > > I wonder, what could ever be the reason not to want to rm $tmp.*? > > > > [PATCH 6/8] fstests: consolidate no cleanup test setup > > > > Ah I see. > It might have been better to explicitly opt-out of cleanup > only for those tests via _register_no_cleanup or _unregister_cleanup Premature optimisation. -Dave. -- Dave Chinner david@fromorbit.com