Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger

From: Stephen Boyd <sboyd@kernel.org>
To: Brendan Higgins <brendanhiggins@google.com>
Cc: Petr Mladek <pmladek@suse.com>,
	"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Amir Goldstein <amir73il@gmail.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Sasha Levin <Alexander.Levin@microsoft.com>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Michael Ellerman <mpe@ellerman.id.au>,
	"open list:KERNEL SELFTEST FRAMEWORK"
	<linux-kselftest@vger.kernel.org>, shuah <shuah@kernel.org>,
	Rob Herring <robh@kernel.org>,
	linux-nvdimm <linux-nvdimm@lists.01.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Knut Omang <knut.omang@oracle.com>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	wfg@linux.intel.com, Joel Stanley <joel@jms.id.au>,
	David Rientjes <rientjes@google.com>,
	Jeff Dike <jdike@addtoit.com>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	devicetree <devicetree@vger.kernel.org>,
	linux-kbuild <linux-kbuild@vger.kernel.org>,
	"Bird, Timothy  <Tim.Bird@sony.com>,
	linux-um@lists.infradead.org,
	Steven Rostedt" <rostedt@goodmis.org>,
	Julia Lawall <julia.lawall@lip6.fr>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	kunit-dev@googlegroups.com, Theodore Ts'o <tytso@mit.edu>,
	Richard Weinberger <richard@nod.at>,
	Greg KH <gregkh@linuxfoundation.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Daniel Vetter <daniel@ffwll.ch>, Kees Cook <keescook@google.com>,
	linux-fsdevel@vger.kernel.org,
	Kevin Hilman <khilman@baylibre.com>
Subject: Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
Date: Tue, 16 Jul 2019 08:30:01 -0700	[thread overview]
Message-ID: <20190716153002.49E292054F@mail.kernel.org> (raw)
In-Reply-To: <CAFd5g44_axVHNMBzxSURQB_-R+Rif7cZcg7PyZ_SS+5hcy5jZA@mail.gmail.com>

Quoting Brendan Higgins (2019-07-16 01:37:34)
> On Tue, Jul 16, 2019 at 12:57 AM Brendan Higgins
> <brendanhiggins@google.com> wrote:
> >
> > On Mon, Jul 15, 2019 at 3:15 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Brendan Higgins (2019-07-12 01:17:30)
> > > > diff --git a/include/kunit/kunit-stream.h b/include/kunit/kunit-stream.h
> > > > new file mode 100644
> > > > index 0000000000000..a7b53eabf6be4
> > > > --- /dev/null
> > > > +++ b/include/kunit/kunit-stream.h
> > > > +/**
> > > > + * struct kunit_stream - a std::stream style string builder.
> > > > + *
> > > > + * A std::stream style string builder. Allows messages to be built up and
> > > > + * printed all at once.
> > > > + */
> > > > +struct kunit_stream {
> > > > +       /* private: internal use only. */
> > > > +       struct kunit *test;
> > > > +       const char *level;
> > >
> > > Is the level changed? See my comment below, but I wonder if this whole
> > > struct can go away and the wrappers can just operate on 'struct
> > > string_stream' instead.
> >
> > I was inclined to agree with you when I first read your comment, but
> > then I thought about the case that someone wants to add in a debug
> > message (of which I currently have none). I think under most
> > circumstances a user of kunit_stream would likely want to pick a
> > default verbosity that maybe I should provide, but may still want
> > different verbosity levels.
> >
> > The main reason I want to keep the types separate, string_stream vs.
> > kunit_stream, is that they are intended to be used differently.
> > string_stream is just a generic string builder. If you are using that,
> > you are expecting to see someone building the string at some point and
> > then doing something interesting with it. kunit_stream really tells
> > you specifically that KUnit is putting together a message to
> > communicate something to a user of KUnit. It is really used in a very
> > specific way, and I wouldn't want to generalize its usage beyond how
> > it is currently used. I think in order to preserve the author's
> > intention it adds clarity to keep the types separate regardless of how
> > similar they might be in reality.

You may want to add some of these reasons to the commit text.

> > > > +
> > > > +       if (!string_stream_is_empty(stream->internal_stream)) {
> > > > +               kunit_err(stream->test,
> > > > +                         "End of test case reached with uncommitted stream entries\n");
> > > > +               kunit_stream_commit(stream);
> > > > +       }
> > > > +}
> > > > +
> > >
> > > Nitpick: Drop this extra newline.
> >
> > Oops, nice catch.
> 
> Not super important, but I don't want you to think that I am ignoring
> you. I think you must have unintentionally deleted the last function
> in this file, or maybe you are referring to something that I am just
> not seeing, but I don't see the extra newline here.

No worries. Sorry for the noise.

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

Makes sense. I wasn't sure if there were more than one stream associated
with a test. Sounds like there are many to one so it can't just be a
member of the test. This could be documented somewhere so this question
doesn't come up again.

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm