Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220316024432.1454366-1-dlatypov@google.com> <CABVgOSkvmuiHfG11VRsQhigOLw_-5HYWjJrQrA4Vy18MvUpW9w@mail.gmail.com>
In-Reply-To: <CABVgOSkvmuiHfG11VRsQhigOLw_-5HYWjJrQrA4Vy18MvUpW9w@mail.gmail.com>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Wed, 16 Mar 2022 11:19:02 -0500
Message-ID: <CAGS_qxpCoyr0_xpRVpjMwAALn49iShGAs-MuBrYHwrEJhKcZUA@mail.gmail.com>
Subject: Re: [PATCH] kunit: split resource API from test.h into new resource.h
To:     David Gow <davidgow@google.com>
Cc:     Brendan Higgins <brendanhiggins@google.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        KUnit Development <kunit-dev@googlegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Mar 16, 2022 at 12:41 AM David Gow <davidgow@google.com> wrote:
>
> On Wed, Mar 16, 2022 at 10:44 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > Background:
> > Currently, a reader looking at kunit/test.h will find the file is quite
> > long, and the first meaty comment is a doc comment about struct
> > kunit_resource.
> >
> > Most users will not ever use the KUnit resource API directly.
> > They'll use kunit_kmalloc() and friends, or decide it's simpler to do
> > cleanups via labels (it often can be) instead of figuring out how to use
> > the API.
> >
>
> A depressing (but probably not untrue) thought. I think that, even if

I'm not sure it's that depressing.
Without some compiler support (e.g. GCC's `cleanup`), I can see there
being a number of one-off things that don't warrant formalizing into a
resource.

More detail:
It works OK when there's one pointer parameter, e.g. [1], but I feel
like you'd normally need to capture at least one more local variable.
So then you need to define a new struct to hold all the values, which
is where I'd draw the line personally.

[1] https://elixir.bootlin.com/linux/v5.17-rc8/source/lib/kunit/executor_test.c#L182

> most people were to use the resource API, having it in test.h makes it
> harder, as having the resource functions separate makes it easier to
> understand as well.
>
> > It's also logically separate from everything else in test.h.
> > Removing it from the file doesn't cause any compilation errors (since
> > struct kunit has `struct list_head resources` to store them).
> >
> > This commit:
> > Let's move it into a kunit/resource.h file and give it a separate page
> > in the docs, kunit/api/resource.rst.
>
> Yay! This makes a lot of sense to me, as I've wasted a lot of time
> scrolling through test.h.
>
> >
> > We include resource.h at the bottom of test.h since
> > * don't want to force existing users to add a new include if they use the API
> > * it accesses `lock` inside `struct kunit` in a inline func
> >   * so we can't just forward declare, and the alternatives require
> >     uninlining the func, adding hepers to lock/unlock, or other more
> >     invasive changes.
>
> I don't like this, but still think it's an improvement on what we have
> now. Ultimately, I think adding helpers to lock/unlock or similar and

Yes, I can see us maybe needing this in the future.
Right now, outside of test.c, there's only one callsite for each (in
resource.h).

> making users include this separately is probably the right thing to
> do, as nesting the headers like this is a bit ugly, but I won't lose
> sleep over leaving it till later.

Ack, I can add a TODO to indicate we want to clean this up?
It's a bit annoying right now, but it'll only get more annoying in the future.

>
> >
> > Now the first big comment in test.h is about kunit_case, which is a lot
> > more relevant to what a new user wants to know.
> >
> > A side effect of this is git blame won't properly track history by
> > default, users need to run
> > $ git blame -L ,1 -C17 include/kunit/resource.h
>
> This is a pain, but is probably worth it. Thanks for including the
> command in the commit message, which should mitigate it slightly.
>
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> This was starting to annoy me, too, as it was a pain to read through
> everything in test.h. It'll be a bit of short-term pain,
> merge-conflict wise if we have other changes to the resource system
> (which I fear is likely), but is worth it.
>
> Reviewed-by: David Gow <davidgow@google.com>
>
> -- David
>
> >
> > NOTE: this file doesn't split out code from test.c to a new resource.c
> > file.
> > I'm primarily concerned with users trying to read the headers, so I
> > didn't think messing up git blame (w/ default settings) was worth it.
> > But I can make that change if it feels appropriate (it might also be
> > messier).
>
> Personally, I think it's probably worth splitting this out as well.
> And the sooner we do it, the less history we'll obscure. :-)

Yeah, that was my thought.
But if you think this would help users, then I think we have a case to
make this change.

Should I send a v2 with resource.c split out?
Brendan (and any others who have an opinion), what's your preference?
>
> But I agree, it's less of an issue as it only directly affects people
> working on KUnit itself. Though making it easier for users to find and
> read the implementation of these functions could help them understand
> API "gotchas", so I think it's worthwhile.
>
> >
> > ---
> >  Documentation/dev-tools/kunit/api/index.rst   |   5 +
> >  .../dev-tools/kunit/api/resource.rst          |  13 +
> >  include/kunit/resource.h                      | 319 ++++++++++++++++++
> >  include/kunit/test.h                          | 301 +----------------
> >  4 files changed, 339 insertions(+), 299 deletions(-)
> >  create mode 100644 Documentation/dev-tools/kunit/api/resource.rst
> >  create mode 100644 include/kunit/resource.h
> >
> <...snip...>