Re: [PATCH] common/config: Always create RESULT_BASE

[Eryu Guan <guaneryu@gmail.com>]

On Fri, Jan 04, 2019 at 08:56:09AM +1100, Dave Chinner wrote:
> On Thu, Jan 03, 2019 at 02:29:08PM +0200, Nikolay Borisov wrote:
> > 
> > 
> > On 3.01.19 г. 12:01 ч., Nikolay Borisov wrote:
> > > Commit 7fc034868d5d ("common/config: create $RESULT_BASE before dumping kmemleak leaks")
> > > inadvertently broke $RESULT_BASE dir creation since it changed the logic
> > > to only create the directory only if this variable is not explicitly set
> > > by the user. Fix this by ensuring RESULT_BASE is always created.
> > > 
> > > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > 
> > Eryu,
> > 
> > I think Johannes' commit should actually be reverted. Currently
> > get_next_config is called at the beginning of the section code _AFTER_
> > _init_kmemleak so it's not really fixing the problem that Johannes
> > described. So care to revert his commit ?
> 
> Yes, the original commit should be reverted. Using RESULT_BASE in
> the way that init_kmemleak is using it is fundamentally broken.
> Using RESULTS_BASE is incorrect - harness run state is supposed to
> be stored in the section-aware RESULTS_DIR (e.g. see
> _require_scratch()) which is constant and always exists for each
> section that is run.
> 
> The code was architectected this way because section definitions can
> change RESULT_BASE, and that means the RESULT_BASE defined when
> init_kmemleak() is called may not point to the same location as when
> check_memleak() is called. Hence check_memleak will never run if a
> section definition changes RESULT_BASE.
> 
> IOWs, the init_kmemleak() call needs to be done inside the
> section iteration loop, after the section config has been parsed and
> we've determined if the the section will be run. This is where the
> original code created RESULT_BASE if it didn't exist. Further, all
> the section run state is then stored in RESULTS_DIR, which is
> created from the current RESULT_BASE (e.g. see _require_scratch,
> check_dmesg, etc).

This makes more sense, thanks for the suggestion!

> 
> Hence init_kmemleak() and check_kmemleak() should have a scope
> inside a valid RESULT_DIR path. And once this is done, you can then
> add a USE_KMEMLEAK section variable to turn kmemleak detection on
> and off via the config file on a per-section basis.....
> 
> So, IMO, the correct thing to do here is revert the original broken
> change and fix the kmemleak infrastructure to be properly aware of
> config sections.

I think we can take Nikolay's patch for now so we don't leave
user-specified RESULT_BASE not created for any longer, and revert it and
7fc034868d5d when we rework kmemleak infrastructure.

Thanks,
Eryu