Date:   Thu, 2 May 2019 23:48:30 -0700
From:   Brendan Higgins <brendanhiggins@...gle.com>
To:     Logan Gunthorpe <logang@...tatee.com>
Cc:     Frank Rowand <frowand.list@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Kees Cook <keescook@...gle.com>,
        Kieran Bingham <kieran.bingham@...asonboard.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Rob Herring <robh@...nel.org>, Stephen Boyd <sboyd@...nel.org>,
        shuah@...nel.org, devicetree <devicetree@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        kunit-dev@...glegroups.com, linux-doc@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-kbuild@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-kselftest@...r.kernel.org,
        linux-nvdimm <linux-nvdimm@...ts.01.org>,
        linux-um@...ts.infradead.org,
        Sasha Levin <Alexander.Levin@...rosoft.com>,
        "Bird, Timothy" <Tim.Bird@...y.com>,
        Amir Goldstein <amir73il@...il.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Daniel Vetter <daniel@...ll.ch>, Jeff Dike <jdike@...toit.com>,
        Joel Stanley <joel@....id.au>,
        Julia Lawall <julia.lawall@...6.fr>,
        Kevin Hilman <khilman@...libre.com>,
        Knut Omang <knut.omang@...cle.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Petr Mladek <pmladek@...e.com>,
        Richard Weinberger <richard@....at>,
        David Rientjes <rientjes@...gle.com>,
        Steven Rostedt <rostedt@...dmis.org>, wfg@...ux.intel.com
Subject: Re: [PATCH v2 08/17] kunit: test: add support for test abort

On Thu, May 2, 2019 at 8:15 PM Logan Gunthorpe <logang@...tatee.com> wrote:
>
>
>
> On 2019-05-01 5:01 p.m., Brendan Higgins wrote:
> > +/*
> > + * struct kunit_try_catch - provides a generic way to run code which might fail.
> > + * @context: used to pass user data to the try and catch functions.
> > + *
> > + * kunit_try_catch provides a generic, architecture independent way to execute
> > + * an arbitrary function of type kunit_try_catch_func_t which may bail out by
> > + * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is called, @try
> > + * is stopped at the site of invocation and @catch is catch is called.
>
> I found some of the C++ comparisons in this series a bit distasteful but
> wasn't going to say anything until I saw the try catch.... But looking
> into the implementation it's just a thread that can exit early which
> seems fine to me. Just a poor choice of name I guess...

Guilty as charged (I have a long history with C++, sorry). Would you
prefer I changed the name? I just figured that try-catch is a commonly
understood pattern that describes exactly what I am doing.

>
> [snip]
>
> > +static void __noreturn kunit_abort(struct kunit *test)
> > +{
> > +     kunit_set_death_test(test, true);
> > +
> > +     kunit_try_catch_throw(&test->try_catch);
> > +
> > +     /*
> > +      * Throw could not abort from test.
> > +      *
> > +      * XXX: we should never reach this line! As kunit_try_catch_throw is
> > +      * marked __noreturn.
> > +      */
> > +     WARN_ONCE(true, "Throw could not abort from test!\n");
> > +}
> > +
> >  int kunit_init_test(struct kunit *test, const char *name)
> >  {
> >       spin_lock_init(&test->lock);
> > @@ -77,6 +103,7 @@ int kunit_init_test(struct kunit *test, const char *name)
> >       test->name = name;
> >       test->vprintk = kunit_vprintk;
> >       test->fail = kunit_fail;
> > +     test->abort = kunit_abort;
>
> There are a number of these function pointers which seem to be pointless
> to me as you only ever set them to one function. Just call the function
> directly. As it is, it is an unnecessary indirection for someone reading
> the code. If and when you have multiple implementations of the function
> then add the pointer. Don't assume you're going to need it later on and
> add all this maintenance burden if you never use it..

Ah, yes, Frank (and probably others) previously asked me to remove
unnecessary method pointers; I removed all the totally unused ones. As
for these, I don't use them in this patchset, but I use them in my
patchsets that will follow up this one. These in particular are
present so that they can be mocked out for testing.

>
> [snip]
>
> > +void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch)
> > +{
> > +     try_catch->run = kunit_generic_run_try_catch;
> > +     try_catch->throw = kunit_generic_throw;
> > +}
>
> Same here. There's only one implementation of try_catch and I can't
> really see any sensible justification for another implementation. Even
> if there is, add the indirection when the second implementation is
> added. This isn't C++ and we don't need to make everything a "method".

These methods are for a UML specific implementation in a follow up
patchset, which is needed for some features like crash recovery, death
tests, and removes dependence on kthreads.

I know this probably sounds like premature complexity. Arguably it is
in hindsight, but I wrote those features before I pulled out these
interfaces (they were actually both originally in this patchset, but I
dropped them to make this patchset easier to review). I can remove
these methods and add them back in when I actually use them in the
follow up patchsets if you prefer.

Thanks!

Powered by blists - more mailing lists