Message-Id: <20190716175021.9CA412173C@mail.kernel.org>
Date:   Tue, 16 Jul 2019 10:50:20 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Brendan Higgins <brendanhiggins@...gle.com>
Cc:     Frank Rowand <frowand.list@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Kees Cook <keescook@...gle.com>,
        Kieran Bingham <kieran.bingham@...asonboard.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Rob Herring <robh@...nel.org>, shuah <shuah@...nel.org>,
        Theodore Ts'o <tytso@....edu>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        devicetree <devicetree@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        kunit-dev@...glegroups.com,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        linux-fsdevel@...r.kernel.org,
        linux-kbuild <linux-kbuild@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <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>,
        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>,
        Logan Gunthorpe <logang@...tatee.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Petr Mladek <pmladek@...e.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Richard Weinberger <richard@....at>,
        David Rientjes <rientjes@...gle.com>,
        Steven Rostedt <rostedt@...dmis.org>, wfg@...ux.intel.com
Subject: Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger

Quoting Brendan Higgins (2019-07-16 01:37:34)
> On Tue, Jul 16, 2019 at 12:57 AM Brendan Higgins
> <brendanhiggins@...gle.com> wrote:
> >
> > A `struct kunit_stream` is usually associated with a message that is
> > being built up over time like maybe an expectation; it is meant to
> > capture the idea that we might want to send some information out to
> > the user pertaining to some thing 'X', but we aren't sure that we
> > actually want to send it until 'X' is complete, but do to the nature
> > of 'X' it is easier to start constructing the message before 'X' is
> > complete.
> >
> > Consider a complicated expectation, there might be multiple conditions
> > that satisfy it and multiple conditions which could make it fail. As
> > we start exploring the input to the expectation we gain information
> > that we might want to share back with the user if the expectation were
> > to fail and we might get that information before we are actually sure
> > that the expectation does indeed fail.
> >
> > When we first step into the expectation we immediately know the
> > function name, file name, and line number where we are called and
> > would want to put that information into any message we would send to
> > the user about this expectation. Next, we might want to check a
> > property of the input, it may or may not be enough information on its
> > own for the expectation to fail, but we want to share the result of
> > the property check with the user regardless, BUT only if the
> > expectation as a whole fails.
> >
> > Hence, we can have multiple `struct kunit_stream`s associated with a
> > `struct kunit` active at any given time.

I'm coming back to this now after reading the rest of the patches that
deal with assertions and expectations. It looks like the string stream
is there to hold a few different pieces of information:

 - Line Number
 - File Name
 - Function Name

The above items could be stored in a structure on the stack that then
gets printed and formatted when the expectation or assertion fails. That
would make the whole string stream structure and code unnecessary.

The only hypothetical case where this can't be done is a complicated
assertion or expectation that does more than one check and can't be
written as a function that dumps out what went wrong. Is this a real
problem? Maybe such an assertion should just open code that logic so we
don't have to build up a string for all the other simple cases.

It seems far simpler to get rid of the string stream API and just have a
struct for this.

	struct kunit_fail_msg {
		const char *line;
		const char *file;
		const char *func;
		const char *msg;
	};

Then you can have the assertion macros create this on the stack (with
another macro?).

	#define DEFINE_KUNIT_FAIL_MSG(name, _msg) \
		struct kunit_fail_msg name = { \
			.line =  __LINE__, \
			.file = __FILE__, \
			.func = __func__, \
			.msg = _msg, \
		}

Note: I don't know if the __LINE__ above will use the macro location, so
this probably needs another wrapper to put the right line number there.

I don't want to derail this whole series on this topic, but it seems
like a bunch of code is there to construct this same set of information
over and over again into a buffer a little bit at a time and then throw
it away when nothing fails just because we may want to support the case
where we have some unstructured data to inform the user about.

Why not build in the structured part into the framework (i.e. the struct
above) so that it's always there and then add the string building part
later when we have to?

Powered by blists - more mailing lists