Date:   Wed, 23 Mar 2022 19:51:23 -0400
From:   Brendan Higgins <brendanhiggins@...gle.com>
To:     Daniel Latypov <dlatypov@...gle.com>
Cc:     David Gow <davidgow@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH] kunit: split resource API from test.h into new resource.h

On Wed, Mar 16, 2022 at 12:19 PM 'Daniel Latypov' via KUnit
Development <kunit-dev@...glegroups.com> wrote:
>
> On Wed, Mar 16, 2022 at 12:41 AM David Gow <davidgow@...gle.com> wrote:
> >
> > On Wed, Mar 16, 2022 at 10:44 AM Daniel Latypov <dlatypov@...gle.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).

Another alternative workaround is to split even more stuff out into
other header files.

Personally I would prefer not to wrap the lock/unlock functions; I
like seeing the kind of locking that's happening. Plus, such a helper
would be pretty gross:

void kunit_lock(struct kunit *test, unsigned long* flags) {...}

It wouldn't actually clean up the call site, just facilitate breaking
out code into a header.

> > 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?

I am fine with this.

> 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@...gle.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@...gle.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?

I personally don't think test.c is so huge that it is a problem to
understand, but I can see it getting there.

If it's going to happen, sooner is probably better.

> >
> > 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...>

Powered by blists - more mailing lists