On Wed, Mar 16, 2022 at 10:44 AM Daniel Latypov <[email protected]> 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
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
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.
>
> 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 <[email protected]>
> ---
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 <[email protected]>
-- 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. :-)
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...>
On Wed, Mar 16, 2022 at 12:41 AM David Gow <[email protected]> wrote:
>
> On Wed, Mar 16, 2022 at 10:44 AM Daniel Latypov <[email protected]> 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 <[email protected]>
> > ---
>
> 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 <[email protected]>
>
> -- 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...>
On Thu, Mar 17, 2022 at 12:19 AM Daniel Latypov <[email protected]> wrote:
>
> On Wed, Mar 16, 2022 at 12:41 AM David Gow <[email protected]> wrote:
> >
> > On Wed, Mar 16, 2022 at 10:44 AM Daniel Latypov <[email protected]> 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.
True, though I do think that the resources API could use a bit of
polish to reduce the friction involved in figuring out how to use the
API.
(And this patch is a good start!)
> 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.
Yeah, let's get this in largely as-is first, then we can start adding
direct includes of "resource.h" where necessary before making it
required.
> >
> > >
> > > 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 <[email protected]>
> > > ---
> >
> > 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 <[email protected]>
> >
> > -- 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 think it's a separate enough thing that this patch could go in
as-is, and resource.c could be split in a separate one if you
preferred. But doing it in a v2 is fine as well.
> >
> > 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...>
On Wed, Mar 16, 2022 at 12:19 PM 'Daniel Latypov' via KUnit
Development <[email protected]> wrote:
>
> On Wed, Mar 16, 2022 at 12:41 AM David Gow <[email protected]> wrote:
> >
> > On Wed, Mar 16, 2022 at 10:44 AM Daniel Latypov <[email protected]> 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 <[email protected]>
> > > ---
> >
> > 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 <[email protected]>
> >
> > -- 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...>