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>
 <CAGS_qxpCoyr0_xpRVpjMwAALn49iShGAs-MuBrYHwrEJhKcZUA@mail.gmail.com> <CAFd5g46N6kbcknJxAug3nkEqe5Q+UuqYAfh8AhPkfPzeQFX8PA@mail.gmail.com>
In-Reply-To: <CAFd5g46N6kbcknJxAug3nkEqe5Q+UuqYAfh8AhPkfPzeQFX8PA@mail.gmail.com>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Thu, 24 Mar 2022 19:08:33 -0500
Message-ID: <CAGS_qxrHQvYfe0qimYxrqmbszWASwHVokDTS3+4X6Ehqpa1Nhg@mail.gmail.com>
Subject: Re: [PATCH] kunit: split resource API from test.h into new resource.h
To:     Brendan Higgins <brendanhiggins@google.com>
Cc:     David Gow <davidgow@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 23, 2022 at 6:51 PM Brendan Higgins
<brendanhiggins@google.com> wrote:

<snip>

>
> 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) {...}

That's exactly why I didn't bother to try and wrap it, yeah.

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

To clarify, are you saying you're fine w/ the nested header as-is, or
fine with it + a TODO?

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

Ack.
I can work on adding a second patch on to this series that splits out
resource.c?

That causes more churn for the other in-flight patches, but we already
have some since David has some changes in test.h.