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