MIME-Version: 1.0
References: <0b6c06487234b0fb52b7a2fbd2237af42f9d11a6.1639560869.git.geert+renesas@glider.be>
 <CANn89iKdorp0Ki0KFf6LAdjtKOm2np=vYY_YtkmJCoGfet1q-g@mail.gmail.com>
 <CAMuHMdWQZA_fS-pr+4wVYtZ6h9Bx4PJ_92qpDNZ2kdjpzj+DHQ@mail.gmail.com> <CANn89iJ-uGzpbAhNjT=fGfDYTjpxo335yhKbqUKwSUPOwPZqWw@mail.gmail.com>
In-Reply-To: <CANn89iJ-uGzpbAhNjT=fGfDYTjpxo335yhKbqUKwSUPOwPZqWw@mail.gmail.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Wed, 15 Dec 2021 11:41:36 +0100
Message-ID: <CAMuHMdUepDEgf9xD6+6qLqKtQH-ptvUf-fP1M=gt5nemitQBsw@mail.gmail.com>
Subject: Re: [PATCH -next] lib: TEST_REF_TRACKER should depend on REF_TRACKER
 instead of selecting it
To:     Eric Dumazet <edumazet@google.com>
Cc:     Jakub Kicinski <kuba@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        netdev <netdev@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Eric,

On Wed, Dec 15, 2021 at 11:24 AM Eric Dumazet <edumazet@google.com> wrote:
> On Wed, Dec 15, 2021 at 2:10 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Dec 15, 2021 at 10:51 AM Eric Dumazet <edumazet@google.com> wrote:
> > > On Wed, Dec 15, 2021 at 1:36 AM Geert Uytterhoeven
> > > <geert+renesas@glider.be> wrote:
> > > > TEST_REF_TRACKER selects REF_TRACKER, thus enabling an optional feature
> > > > the user may not want to have enabled.  Fix this by making the test
> > > > depend on REF_TRACKER instead.
> > >
> > > I do not understand this.
> >
> > The issue is that merely enabling tests should not enable optional
> > features, to prevent unwanted features sneaking into a product.
>
> if you do not want the feature, just say no ?
>
> # CONFIG_TEST_REF_TRACKER is not set
> # CONFIG_NET_DEV_REFCNT_TRACKER is not set
> # CONFIG_NET_NS_REFCNT_TRACKER is not set
>
> > If tests depend on features, all tests for features that are enabled can
> > still be enabled (e.g. made modular, so they can be loaded when needed).
> >
> > > How can I test this infra alone, without any ref_tracker being selected ?
> > >
> > > I have in my configs
> > >
> > > CONFIG_TEST_REF_TRACKER=m
> > > # CONFIG_NET_DEV_REFCNT_TRACKER is not set
> > > # CONFIG_NET_NS_REFCNT_TRACKER is not set
> > >
> > > This should work.
> >
> > So you want to test the reference tracker, without having any actual
> > users of the reference tracker enabled?
>
> Yes, I fail to see the problem you have with this.
>
> lib/ref_tracker.c is not adding intrusive features like KASAN.

How can I be sure of that? ;-)

> > Perhaps REF_TRACKER should become visible, cfr. CRC32 and
> > the related CRC32_SELFTEST?
>
> I can not speak for CRC32.
>
> My point is that I sent a series, I wanted this series to be bisectable.
>
> When the test was added, I wanted to be able to use it right away.
> (like compile it, and run it)

Then you indeed need a way to force-enable the feature. For other
library-like features, that is done by making the feature visible,
cfr. CRC32.

My point is that a user should be able to easily enable all available
tests for all features he has wilfully enabled in his kernel config,
without running into the risk of accidentally enabling more features.
Hence a test should depend on the feature under test, not blindly
enable the feature.

An example of this is commit 302fdadeafe4be53 ("ext: EXT4_KUNIT_TESTS
should depend on EXT4_FS instead of selecting it"). Before that commit,
enabling KUNIT_ALL_TESTS=m enabled EXT4_FS, even on diskless system.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds