[v9,04/18] kunit: test: add kunit_stream a std::stream like logger
diff mbox series

Message ID 20190712081744.87097-5-brendanhiggins@google.com
State New
Headers show
Series
  • kunit: introduce KUnit, the Linux kernel unit testing framework
Related show

Commit Message

Brendan Higgins July 12, 2019, 8:17 a.m. UTC
A lot of the expectation and assertion infrastructure prints out fairly
complicated test failure messages, so add a C++ style log library for
for logging test results.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
---
 include/kunit/kunit-stream.h |  81 +++++++++++++++++++++++
 include/kunit/test.h         |   3 +
 kunit/Makefile               |   3 +-
 kunit/kunit-stream.c         | 123 +++++++++++++++++++++++++++++++++++
 kunit/test.c                 |   6 ++
 5 files changed, 215 insertions(+), 1 deletion(-)
 create mode 100644 include/kunit/kunit-stream.h
 create mode 100644 kunit/kunit-stream.c

Comments

Stephen Boyd July 15, 2019, 10:15 p.m. UTC | #1
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
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * C++ stream style string formatter and printer used in KUnit for outputting
> + * KUnit messages.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#ifndef _KUNIT_KUNIT_STREAM_H
> +#define _KUNIT_KUNIT_STREAM_H
> +
> +#include <linux/types.h>
> +#include <kunit/string-stream.h>
> +
> +struct kunit;
> +
> +/**
> + * 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.

> +       struct string_stream *internal_stream;
> +};
> diff --git a/kunit/kunit-stream.c b/kunit/kunit-stream.c
> new file mode 100644
> index 0000000000000..8bea1f22eafb5
> --- /dev/null
> +++ b/kunit/kunit-stream.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * C++ stream style string formatter and printer used in KUnit for outputting
> + * KUnit messages.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#include <kunit/test.h>
> +#include <kunit/kunit-stream.h>
> +#include <kunit/string-stream.h>
> +
> +void kunit_stream_add(struct kunit_stream *kstream, const char *fmt, ...)
> +{
> +       va_list args;
> +       struct string_stream *stream = kstream->internal_stream;
> +
> +       va_start(args, fmt);
> +
> +       if (string_stream_vadd(stream, fmt, args) < 0)
> +               kunit_err(kstream->test,
> +                         "Failed to allocate fragment: %s\n",
> +                         fmt);
> +
> +       va_end(args);
> +}
> +
> +void kunit_stream_append(struct kunit_stream *kstream,
> +                               struct kunit_stream *other)
> +{
> +       struct string_stream *other_stream = other->internal_stream;
> +       const char *other_content;
> +
> +       other_content = string_stream_get_string(other_stream);
> +
> +       if (!other_content) {
> +               kunit_err(kstream->test,
> +                         "Failed to get string from second argument for appending\n");
> +               return;
> +       }
> +
> +       kunit_stream_add(kstream, other_content);
> +}

Why can't this function be implemented in the string_stream API? Seems
valid to want to append one stream to another and that isn't
kunit_stream specific.

> +
> +void kunit_stream_clear(struct kunit_stream *kstream)
> +{
> +       string_stream_clear(kstream->internal_stream);
> +}
> +
> +void kunit_stream_commit(struct kunit_stream *kstream)
> +{
> +       struct string_stream *stream = kstream->internal_stream;
> +       struct string_stream_fragment *fragment;
> +       struct kunit *test = kstream->test;
> +       char *buf;
> +
> +       buf = string_stream_get_string(stream);
> +       if (!buf) {
> +               kunit_err(test,
> +                         "Could not allocate buffer, dumping stream:\n");
> +               list_for_each_entry(fragment, &stream->fragments, node) {
> +                       kunit_err(test, fragment->fragment);
> +               }
> +               kunit_err(test, "\n");
> +               goto cleanup;
> +       }
> +
> +       kunit_printk(kstream->level, test, buf);
> +       kfree(buf);
> +
> +cleanup:

Drop the goto and use an 'else' please.

> +       kunit_stream_clear(kstream);
> +}
> +
> +static int kunit_stream_init(struct kunit_resource *res, void *context)
> +{
> +       struct kunit *test = context;
> +       struct kunit_stream *stream;
> +
> +       stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> +       if (!stream)
> +               return -ENOMEM;
> +
> +       res->allocation = stream;
> +       stream->test = test;
> +       stream->internal_stream = alloc_string_stream(test);
> +
> +       if (!stream->internal_stream)
> +               return -ENOMEM;
> +
> +       return 0;
> +}
> +
> +static void kunit_stream_free(struct kunit_resource *res)
> +{
> +       struct kunit_stream *stream = res->allocation;
> +
> +       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.

> diff --git a/kunit/test.c b/kunit/test.c
> index f165c9d8e10b0..29edf34a89a37 100644
> --- a/kunit/test.c
> +++ b/kunit/test.c
> @@ -120,6 +120,12 @@ static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
>                               test_case->name);
>  }
>  
> +void kunit_fail(struct kunit *test, struct kunit_stream *stream)

Why doesn't 'struct kunit' have a 'struct kunit_stream' inside of it? It
seems that the two are highly related, to the point that it might just
make sense to have

	struct kunit {
		struct kunit_stream stream;
		...
	};

> +{
> +       kunit_set_failure(test);
> +       kunit_stream_commit(stream);

And then this function can just take a test and the stream can be
associated with the test directly. Use container_of() to get to the test
when the only pointer in hand is for the stream too.

> +}
> +
>  void kunit_init_test(struct kunit *test, const char *name)
>  {
>         mutex_init(&test->lock);
Brendan Higgins July 16, 2019, 7:57 a.m. UTC | #2
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
> > @@ -0,0 +1,81 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * C++ stream style string formatter and printer used in KUnit for outputting
> > + * KUnit messages.
> > + *
> > + * Copyright (C) 2019, Google LLC.
> > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > + */
> > +
> > +#ifndef _KUNIT_KUNIT_STREAM_H
> > +#define _KUNIT_KUNIT_STREAM_H
> > +
> > +#include <linux/types.h>
> > +#include <kunit/string-stream.h>
> > +
> > +struct kunit;
> > +
> > +/**
> > + * 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.

> > +       struct string_stream *internal_stream;
> > +};
> > diff --git a/kunit/kunit-stream.c b/kunit/kunit-stream.c
> > new file mode 100644
> > index 0000000000000..8bea1f22eafb5
> > --- /dev/null
> > +++ b/kunit/kunit-stream.c
> > @@ -0,0 +1,123 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * C++ stream style string formatter and printer used in KUnit for outputting
> > + * KUnit messages.
> > + *
> > + * Copyright (C) 2019, Google LLC.
> > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > + */
> > +
> > +#include <kunit/test.h>
> > +#include <kunit/kunit-stream.h>
> > +#include <kunit/string-stream.h>
> > +
> > +void kunit_stream_add(struct kunit_stream *kstream, const char *fmt, ...)
> > +{
> > +       va_list args;
> > +       struct string_stream *stream = kstream->internal_stream;
> > +
> > +       va_start(args, fmt);
> > +
> > +       if (string_stream_vadd(stream, fmt, args) < 0)
> > +               kunit_err(kstream->test,
> > +                         "Failed to allocate fragment: %s\n",
> > +                         fmt);
> > +
> > +       va_end(args);
> > +}
> > +
> > +void kunit_stream_append(struct kunit_stream *kstream,
> > +                               struct kunit_stream *other)
> > +{
> > +       struct string_stream *other_stream = other->internal_stream;
> > +       const char *other_content;
> > +
> > +       other_content = string_stream_get_string(other_stream);
> > +
> > +       if (!other_content) {
> > +               kunit_err(kstream->test,
> > +                         "Failed to get string from second argument for appending\n");
> > +               return;
> > +       }
> > +
> > +       kunit_stream_add(kstream, other_content);
> > +}
>
> Why can't this function be implemented in the string_stream API? Seems
> valid to want to append one stream to another and that isn't
> kunit_stream specific.

Fair point. Will do.

> > +
> > +void kunit_stream_clear(struct kunit_stream *kstream)
> > +{
> > +       string_stream_clear(kstream->internal_stream);
> > +}
> > +
> > +void kunit_stream_commit(struct kunit_stream *kstream)
> > +{
> > +       struct string_stream *stream = kstream->internal_stream;
> > +       struct string_stream_fragment *fragment;
> > +       struct kunit *test = kstream->test;
> > +       char *buf;
> > +
> > +       buf = string_stream_get_string(stream);
> > +       if (!buf) {
> > +               kunit_err(test,
> > +                         "Could not allocate buffer, dumping stream:\n");
> > +               list_for_each_entry(fragment, &stream->fragments, node) {
> > +                       kunit_err(test, fragment->fragment);
> > +               }
> > +               kunit_err(test, "\n");
> > +               goto cleanup;
> > +       }
> > +
> > +       kunit_printk(kstream->level, test, buf);
> > +       kfree(buf);
> > +
> > +cleanup:
>
> Drop the goto and use an 'else' please.

Will do.

> > +       kunit_stream_clear(kstream);
> > +}
> > +
> > +static int kunit_stream_init(struct kunit_resource *res, void *context)
> > +{
> > +       struct kunit *test = context;
> > +       struct kunit_stream *stream;
> > +
> > +       stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> > +       if (!stream)
> > +               return -ENOMEM;
> > +
> > +       res->allocation = stream;
> > +       stream->test = test;
> > +       stream->internal_stream = alloc_string_stream(test);
> > +
> > +       if (!stream->internal_stream)
> > +               return -ENOMEM;
> > +
> > +       return 0;
> > +}
> > +
> > +static void kunit_stream_free(struct kunit_resource *res)
> > +{
> > +       struct kunit_stream *stream = res->allocation;
> > +
> > +       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.

> > diff --git a/kunit/test.c b/kunit/test.c
> > index f165c9d8e10b0..29edf34a89a37 100644
> > --- a/kunit/test.c
> > +++ b/kunit/test.c
> > @@ -120,6 +120,12 @@ static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
> >                               test_case->name);
> >  }
> >
> > +void kunit_fail(struct kunit *test, struct kunit_stream *stream)
>
> Why doesn't 'struct kunit' have a 'struct kunit_stream' inside of it? It
> seems that the two are highly related, to the point that it might just
> make sense to have

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.

>         struct kunit {
>                 struct kunit_stream stream;
>                 ...
>         };
>
> > +{
> > +       kunit_set_failure(test);
> > +       kunit_stream_commit(stream);
>
> And then this function can just take a test and the stream can be
> associated with the test directly. Use container_of() to get to the test
> when the only pointer in hand is for the stream too.

Unfortunately that wouldn't work. See my above explanation.

> > +}
> > +
> >  void kunit_init_test(struct kunit *test, const char *name)
> >  {
> >         mutex_init(&test->lock);

Thanks!
Brendan Higgins July 16, 2019, 8:37 a.m. UTC | #3
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
> > > @@ -0,0 +1,81 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * C++ stream style string formatter and printer used in KUnit for outputting
> > > + * KUnit messages.
> > > + *
> > > + * Copyright (C) 2019, Google LLC.
> > > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > > + */
> > > +
> > > +#ifndef _KUNIT_KUNIT_STREAM_H
> > > +#define _KUNIT_KUNIT_STREAM_H
> > > +
> > > +#include <linux/types.h>
> > > +#include <kunit/string-stream.h>
> > > +
> > > +struct kunit;
> > > +
> > > +/**
> > > + * 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.
>
> > > +       struct string_stream *internal_stream;
> > > +};
> > > diff --git a/kunit/kunit-stream.c b/kunit/kunit-stream.c
> > > new file mode 100644
> > > index 0000000000000..8bea1f22eafb5
> > > --- /dev/null
> > > +++ b/kunit/kunit-stream.c
> > > @@ -0,0 +1,123 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * C++ stream style string formatter and printer used in KUnit for outputting
> > > + * KUnit messages.
> > > + *
> > > + * Copyright (C) 2019, Google LLC.
> > > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > > + */
> > > +
> > > +#include <kunit/test.h>
> > > +#include <kunit/kunit-stream.h>
> > > +#include <kunit/string-stream.h>
> > > +
> > > +void kunit_stream_add(struct kunit_stream *kstream, const char *fmt, ...)
> > > +{
> > > +       va_list args;
> > > +       struct string_stream *stream = kstream->internal_stream;
> > > +
> > > +       va_start(args, fmt);
> > > +
> > > +       if (string_stream_vadd(stream, fmt, args) < 0)
> > > +               kunit_err(kstream->test,
> > > +                         "Failed to allocate fragment: %s\n",
> > > +                         fmt);
> > > +
> > > +       va_end(args);
> > > +}
> > > +
> > > +void kunit_stream_append(struct kunit_stream *kstream,
> > > +                               struct kunit_stream *other)
> > > +{
> > > +       struct string_stream *other_stream = other->internal_stream;
> > > +       const char *other_content;
> > > +
> > > +       other_content = string_stream_get_string(other_stream);
> > > +
> > > +       if (!other_content) {
> > > +               kunit_err(kstream->test,
> > > +                         "Failed to get string from second argument for appending\n");
> > > +               return;
> > > +       }
> > > +
> > > +       kunit_stream_add(kstream, other_content);
> > > +}
> >
> > Why can't this function be implemented in the string_stream API? Seems
> > valid to want to append one stream to another and that isn't
> > kunit_stream specific.
>
> Fair point. Will do.
>
> > > +
> > > +void kunit_stream_clear(struct kunit_stream *kstream)
> > > +{
> > > +       string_stream_clear(kstream->internal_stream);
> > > +}
> > > +
> > > +void kunit_stream_commit(struct kunit_stream *kstream)
> > > +{
> > > +       struct string_stream *stream = kstream->internal_stream;
> > > +       struct string_stream_fragment *fragment;
> > > +       struct kunit *test = kstream->test;
> > > +       char *buf;
> > > +
> > > +       buf = string_stream_get_string(stream);
> > > +       if (!buf) {
> > > +               kunit_err(test,
> > > +                         "Could not allocate buffer, dumping stream:\n");
> > > +               list_for_each_entry(fragment, &stream->fragments, node) {
> > > +                       kunit_err(test, fragment->fragment);
> > > +               }
> > > +               kunit_err(test, "\n");
> > > +               goto cleanup;
> > > +       }
> > > +
> > > +       kunit_printk(kstream->level, test, buf);
> > > +       kfree(buf);
> > > +
> > > +cleanup:
> >
> > Drop the goto and use an 'else' please.
>
> Will do.
>
> > > +       kunit_stream_clear(kstream);
> > > +}
> > > +
> > > +static int kunit_stream_init(struct kunit_resource *res, void *context)
> > > +{
> > > +       struct kunit *test = context;
> > > +       struct kunit_stream *stream;
> > > +
> > > +       stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> > > +       if (!stream)
> > > +               return -ENOMEM;
> > > +
> > > +       res->allocation = stream;
> > > +       stream->test = test;
> > > +       stream->internal_stream = alloc_string_stream(test);
> > > +
> > > +       if (!stream->internal_stream)
> > > +               return -ENOMEM;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static void kunit_stream_free(struct kunit_resource *res)
> > > +{
> > > +       struct kunit_stream *stream = res->allocation;
> > > +
> > > +       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.

> > > diff --git a/kunit/test.c b/kunit/test.c
> > > index f165c9d8e10b0..29edf34a89a37 100644
> > > --- a/kunit/test.c
> > > +++ b/kunit/test.c
> > > @@ -120,6 +120,12 @@ static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
> > >                               test_case->name);
> > >  }
> > >
> > > +void kunit_fail(struct kunit *test, struct kunit_stream *stream)
> >
> > Why doesn't 'struct kunit' have a 'struct kunit_stream' inside of it? It
> > seems that the two are highly related, to the point that it might just
> > make sense to have
>
> 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.
>
> >         struct kunit {
> >                 struct kunit_stream stream;
> >                 ...
> >         };
> >
> > > +{
> > > +       kunit_set_failure(test);
> > > +       kunit_stream_commit(stream);
> >
> > And then this function can just take a test and the stream can be
> > associated with the test directly. Use container_of() to get to the test
> > when the only pointer in hand is for the stream too.
>
> Unfortunately that wouldn't work. See my above explanation.
>
> > > +}
> > > +
> > >  void kunit_init_test(struct kunit *test, const char *name)
> > >  {
> > >         mutex_init(&test->lock);
>
> Thanks!
Stephen Boyd July 16, 2019, 3:30 p.m. UTC | #4
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.
Stephen Boyd July 16, 2019, 5:50 p.m. UTC | #5
Quoting Brendan Higgins (2019-07-16 01:37:34)
> On Tue, Jul 16, 2019 at 12:57 AM Brendan Higgins
> <brendanhiggins@google.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?
Brendan Higgins July 16, 2019, 5:51 p.m. UTC | #6
On Tue, Jul 16, 2019 at 8:30 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> 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.

Will do.

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

Sounds good. Will do.

Thanks!
Brendan Higgins July 16, 2019, 6:52 p.m. UTC | #7
On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-07-16 01:37:34)
> > On Tue, Jul 16, 2019 at 12:57 AM Brendan Higgins
> > <brendanhiggins@google.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.

Most of the expectations and assertions in this patchset are fairly
simple, and what you are describing would probably work. However, I
have some expectations I plan on adding in later patchsets that make
much more complicated checks.

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

I have some expectations in follow up patchsets for which I created a
set of composable matchers for matching structures and function calls
that by their nature cannot be written as a single function. The
matcher thing is a bit speculative, I know, but for any kind of
function call matching, you need to store a record of functions you
are expecting to have called and then each one needs to have a set of
expectations defined by the user; I don't think there is a way to do
that that doesn't involve having multiple separate functions each
having some information useful to constructing the message.

I know the code in question isn't in this patchset; the function
matching code was in one of the earlier versions of the RFC, but I
dropped it to make this patchset smaller and more manageable. So I get
it if you would like me to drop it and add it back in when I try to
get the function and structure matching stuff in, but I would really
prefer to keep it as is if you don't care too much.

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

No, that should work. It picks up where the macro ends up being
finally evaluated.

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

Yeah, that's fair. I think there are a number of improvements to be
made with how the expectations are defined other than that, but I was
hoping I could do that after this patchset is merged. I just figured
with the kinds of things I would like to do, it would lead to a whole
new round of discussion.

In either case, I think I would still like to use the `struct
kunit_stream` as part of the interface to share the failure message
with the test case runner code in test.c, at least eventually, so that
I only have to have one way to receive data from expectations, but I
think I can do that and still do what you suggest by just constructing
the kunit_stream at the end of expectations where it is feasible.

All in all I agree with what you are saying, but I would rather do it
as a follow up possibly once we have some more code on the table. I
could just see this opening up a whole new can of worms where we
debate about exactly how expectations and assertions work for another
several months, only to rip it all out shortly there after. I know
that's how these things go, but that's my preference.

I can do what you suggest if you feel strongly about it, but I would
prefer to hold off until later. It's your call.

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

See above comments.
Stephen Boyd July 18, 2019, 5:50 p.m. UTC | #8
Quoting Brendan Higgins (2019-07-16 11:52:01)
> On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> 
> > 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.
> 
> I have some expectations in follow up patchsets for which I created a
> set of composable matchers for matching structures and function calls
> that by their nature cannot be written as a single function. The
> matcher thing is a bit speculative, I know, but for any kind of
> function call matching, you need to store a record of functions you
> are expecting to have called and then each one needs to have a set of
> expectations defined by the user; I don't think there is a way to do
> that that doesn't involve having multiple separate functions each
> having some information useful to constructing the message.
> 
> I know the code in question isn't in this patchset; the function
> matching code was in one of the earlier versions of the RFC, but I
> dropped it to make this patchset smaller and more manageable. So I get
> it if you would like me to drop it and add it back in when I try to
> get the function and structure matching stuff in, but I would really
> prefer to keep it as is if you don't care too much.

Do you have a link to those earlier patches?

> 
> > 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, \
> >                 }
> >
> > 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.
> 
> Yeah, that's fair. I think there are a number of improvements to be
> made with how the expectations are defined other than that, but I was
> hoping I could do that after this patchset is merged. I just figured
> with the kinds of things I would like to do, it would lead to a whole
> new round of discussion.
> 
> In either case, I think I would still like to use the `struct
> kunit_stream` as part of the interface to share the failure message
> with the test case runner code in test.c, at least eventually, so that
> I only have to have one way to receive data from expectations, but I
> think I can do that and still do what you suggest by just constructing
> the kunit_stream at the end of expectations where it is feasible.
> 
> All in all I agree with what you are saying, but I would rather do it
> as a follow up possibly once we have some more code on the table. I
> could just see this opening up a whole new can of worms where we
> debate about exactly how expectations and assertions work for another
> several months, only to rip it all out shortly there after. I know
> that's how these things go, but that's my preference.
> 
> I can do what you suggest if you feel strongly about it, but I would
> prefer to hold off until later. It's your call.
> 

The crux of my complaint is that the string stream API is too loosely
defined to be usable. It allows tests to build up a string of
unstructured information, but with certain calling constraints so we
have to tread carefully. If there was more structure to the data that's
being recorded then the test case runner could operate on the data
without having to do string/stream operations, allocations, etc. This
would make the assertion logic much more concrete and specific to kunit,
instead of this small kunit wrapper that's been placed on top of string
stream.

TL;DR: If we can get rid of the string stream API I'd view that as an
improvement because building arbitrary strings in the kernel is complex,
error prone and has calling context concerns.

Is the intention that other code besides unit tests will use this string
stream API to build up strings? Any targets in mind? This would be a
good way to get the API merged upstream given that its 2019 and we
haven't had such an API in the kernel so far.

An "object oriented" (strong quotes!) approach where kunit_fail_msg is
the innermost struct in some assertion specific structure might work
nicely and allow the test runner to call a generic 'format' function to
print out the message based on the type of assertion/expectation it is.
It probably would mean less code size too because the strings that are
common will be in the common printing function instead of created twice,
in the macros/code and then copied to the heap for the string stream.

	struct kunit_assert {
		const char *line;
		const char *file;
		const char *func;
		void (*format)(struct kunit_assert *assert);
	};

	struct kunit_comparison_assert {
		enum operator operator;
		const char *left;
		const char *right;
		struct kunit_assert assert;
	};

	struct kunit_bool_assert {
		const char *truth;
		const char *statement;
		struct kunit_assert assert;
	};

	void kunit_format_comparison(struct kunit_assert *assert)
	{
		struct kunit_comparison_assert *comp = container_of(assert, ...)

		kunit_printk(...)
	}

Maybe other people have opinions here on if you should do it now or
later. Future coding is not a great argument because it's hard to
predict the future. On the other hand, this patchset is in good shape to
merge and I'd like to use it to write unit tests for code I maintain so
I don't want to see this stall out. Sorry if I'm opening the can of
worms you're talking about.
Brendan Higgins July 18, 2019, 7:22 p.m. UTC | #9
On Thu, Jul 18, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-07-16 11:52:01)
> > On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> >
> > > 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.
> >
> > I have some expectations in follow up patchsets for which I created a
> > set of composable matchers for matching structures and function calls
> > that by their nature cannot be written as a single function. The
> > matcher thing is a bit speculative, I know, but for any kind of
> > function call matching, you need to store a record of functions you
> > are expecting to have called and then each one needs to have a set of
> > expectations defined by the user; I don't think there is a way to do
> > that that doesn't involve having multiple separate functions each
> > having some information useful to constructing the message.
> >
> > I know the code in question isn't in this patchset; the function
> > matching code was in one of the earlier versions of the RFC, but I
> > dropped it to make this patchset smaller and more manageable. So I get
> > it if you would like me to drop it and add it back in when I try to
> > get the function and structure matching stuff in, but I would really
> > prefer to keep it as is if you don't care too much.
>
> Do you have a link to those earlier patches?

This is the first patchset:

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788057.html

In particular you can see the code for matching functions here:

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788073.html

And parameter matching code here:

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788072.html

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788086.html

My apologies in advance, but the code at this early stage had not
adopted the kunit_* prefix and was still using the test_* and mock_*
prefix. (Hence, struct kunit_stream was known as struct test_stream).

> > > 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, \
> > >                 }
> > >
> > > 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.
> >
> > Yeah, that's fair. I think there are a number of improvements to be
> > made with how the expectations are defined other than that, but I was
> > hoping I could do that after this patchset is merged. I just figured
> > with the kinds of things I would like to do, it would lead to a whole
> > new round of discussion.
> >
> > In either case, I think I would still like to use the `struct
> > kunit_stream` as part of the interface to share the failure message
> > with the test case runner code in test.c, at least eventually, so that
> > I only have to have one way to receive data from expectations, but I
> > think I can do that and still do what you suggest by just constructing
> > the kunit_stream at the end of expectations where it is feasible.
> >
> > All in all I agree with what you are saying, but I would rather do it
> > as a follow up possibly once we have some more code on the table. I
> > could just see this opening up a whole new can of worms where we
> > debate about exactly how expectations and assertions work for another
> > several months, only to rip it all out shortly there after. I know
> > that's how these things go, but that's my preference.
> >
> > I can do what you suggest if you feel strongly about it, but I would
> > prefer to hold off until later. It's your call.
> >
>
> The crux of my complaint is that the string stream API is too loosely
> defined to be usable. It allows tests to build up a string of
> unstructured information, but with certain calling constraints so we
> have to tread carefully. If there was more structure to the data that's
> being recorded then the test case runner could operate on the data
> without having to do string/stream operations, allocations, etc. This
> would make the assertion logic much more concrete and specific to kunit,
> instead of this small kunit wrapper that's been placed on top of string
> stream.

Yeah, I can see the point of wanting something that provides more
structure than the raw `struct kunit_stream` interface. In fact, it is
something I had already started working on, when I had determined it
would be a large effort to capture all the variations. I was further
put off from the idea when I had been asked to convert the KUnit
intermediate format from what I was using to TAP, because, as it is,
the current data printed out by KUnit doesn't contain all the data I
would like to put in it in a way that best takes advantage of the TAP
specification. One problematic area in particular: TAP already
provides a way to present a lot of the data I would like to export,
but it involves JSON serialization which was an idea that some of the
other reviewers understandably weren't too keen on. TAP also wants to
report data some time after it is available, which is generally not a
good idea for test debug information; you want to make it available as
soon as you can or you risk crashing with the data still inside.

Hence, I decided we could probably spend a good long while debating
how I present the information. So the idea of having a loose
definition seemed attractive to me in its own right since it would
likely conform to whatever we ended up deciding in the long run. Also,
all the better that it was what I already had and no one seemed to
mind too much.

The only constant I expect is that `struct kunit` will likely need to
take an abstract object with a `commit` method, or a `format` method
or whatever so it could control when data was going to be printed out
to the user. We will probably also use a string builder in there
somewhere.

> TL;DR: If we can get rid of the string stream API I'd view that as an
> improvement because building arbitrary strings in the kernel is complex,
> error prone and has calling context concerns.

True. No argument there.

> Is the intention that other code besides unit tests will use this string
> stream API to build up strings? Any targets in mind? This would be a
> good way to get the API merged upstream given that its 2019 and we
> haven't had such an API in the kernel so far.

Someone, (was it you?) asked about code sharing with a string builder
thingy that was used for creating structured human readable files, but
that seemed like a pretty massive undertaking.

Aside from that, no. I would kind of prefered that nobody used it for
anything else because I the issues you described.

Nevertheless, I think the debate over the usefulness of the
string_stream and kunit_stream are separate topics. Even if we made
kunit_stream more structured, I am pretty sure I would want to use
string_stream or some variation for constructing the message.

> An "object oriented" (strong quotes!) approach where kunit_fail_msg is
> the innermost struct in some assertion specific structure might work
> nicely and allow the test runner to call a generic 'format' function to
> print out the message based on the type of assertion/expectation it is.
> It probably would mean less code size too because the strings that are
> common will be in the common printing function instead of created twice,
> in the macros/code and then copied to the heap for the string stream.
>
>         struct kunit_assert {
>                 const char *line;
>                 const char *file;
>                 const char *func;
>                 void (*format)(struct kunit_assert *assert);
>         };
>
>         struct kunit_comparison_assert {
>                 enum operator operator;
>                 const char *left;
>                 const char *right;
>                 struct kunit_assert assert;
>         };
>
>         struct kunit_bool_assert {
>                 const char *truth;
>                 const char *statement;
>                 struct kunit_assert assert;
>         };
>
>         void kunit_format_comparison(struct kunit_assert *assert)
>         {
>                 struct kunit_comparison_assert *comp = container_of(assert, ...)
>
>                 kunit_printk(...)
>         }

I started working on something similarish, but by the time I ended up
coming up with a parent object whose definition was loose enough to
satisfy all the properties required by the child classes it ended up
basically being the same as what I have now just with a more complex
hierarchy of message manipulation logic.

On the other hand, I didn't have the idea of doing the parent object
quite the way you did and that would clean up a lot of the duplicated
first line logic.

I would like to give it a try, but I am afraid I am going to get
sucked down a really deep rabbit hole.

> Maybe other people have opinions here on if you should do it now or
> later. Future coding is not a great argument because it's hard to
> predict the future. On the other hand, this patchset is in good shape to

Yeah, that's kind of why I am afraid to go down this road when I have
something that works now and I know works with the mocking stuff I
want to do.

I would like to try your suggestion, but I want to try to make it work
with my mocking patches before I commit to it because otherwise I am
just going to have to back it out in a follow up patchset.

> merge and I'd like to use it to write unit tests for code I maintain so
> I don't want to see this stall out. Sorry if I'm opening the can of
> worms you're talking about.

Don't be sorry. I agree with you that the kunit_stream stuff is not very pretty.

Shuah, have we missed the merge window for 5.3?

I saw you only sent one PR out so far for this release, and there
wasn't much in it; I imagine you are going to send at least one more?

I figure, if we still got time to try out your suggestion, Stephen, no
harm in trying.

Also if we missed it, then I have another couple months to play around with it.

What do you think?
Brendan Higgins July 19, 2019, 12:08 a.m. UTC | #10
On Thu, Jul 18, 2019 at 12:22:33PM -0700, Brendan Higgins wrote:
> On Thu, Jul 18, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-07-16 11:52:01)
> > > On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
[...]
> > Do you have a link to those earlier patches?
> 
> This is the first patchset:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788057.html
> 
> In particular you can see the code for matching functions here:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788073.html
> 
> And parameter matching code here:
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788072.html
> 
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788086.html
> 
> My apologies in advance, but the code at this early stage had not
> adopted the kunit_* prefix and was still using the test_* and mock_*
> prefix. (Hence, struct kunit_stream was known as struct test_stream).
[...]
> > The crux of my complaint is that the string stream API is too loosely
> > defined to be usable. It allows tests to build up a string of
> > unstructured information, but with certain calling constraints so we
> > have to tread carefully. If there was more structure to the data that's
> > being recorded then the test case runner could operate on the data
> > without having to do string/stream operations, allocations, etc. This
> > would make the assertion logic much more concrete and specific to kunit,
> > instead of this small kunit wrapper that's been placed on top of string
> > stream.
> 
> Yeah, I can see the point of wanting something that provides more
> structure than the raw `struct kunit_stream` interface. In fact, it is
> something I had already started working on, when I had determined it
> would be a large effort to capture all the variations. I was further
> put off from the idea when I had been asked to convert the KUnit
> intermediate format from what I was using to TAP, because, as it is,
> the current data printed out by KUnit doesn't contain all the data I
> would like to put in it in a way that best takes advantage of the TAP
> specification. One problematic area in particular: TAP already
> provides a way to present a lot of the data I would like to export,
> but it involves JSON serialization which was an idea that some of the
> other reviewers understandably weren't too keen on. TAP also wants to
> report data some time after it is available, which is generally not a
> good idea for test debug information; you want to make it available as
> soon as you can or you risk crashing with the data still inside.
> 
> Hence, I decided we could probably spend a good long while debating
> how I present the information. So the idea of having a loose
> definition seemed attractive to me in its own right since it would
> likely conform to whatever we ended up deciding in the long run. Also,
> all the better that it was what I already had and no one seemed to
> mind too much.
> 
> The only constant I expect is that `struct kunit` will likely need to
> take an abstract object with a `commit` method, or a `format` method
> or whatever so it could control when data was going to be printed out
> to the user. We will probably also use a string builder in there
> somewhere.
> 
> > TL;DR: If we can get rid of the string stream API I'd view that as an
> > improvement because building arbitrary strings in the kernel is complex,
> > error prone and has calling context concerns.
> 
> True. No argument there.
> 
> > Is the intention that other code besides unit tests will use this string
> > stream API to build up strings? Any targets in mind? This would be a
> > good way to get the API merged upstream given that its 2019 and we
> > haven't had such an API in the kernel so far.
> 
> Someone, (was it you?) asked about code sharing with a string builder
> thingy that was used for creating structured human readable files, but
> that seemed like a pretty massive undertaking.
> 
> Aside from that, no. I would kind of prefered that nobody used it for
> anything else because I the issues you described.
> 
> Nevertheless, I think the debate over the usefulness of the
> string_stream and kunit_stream are separate topics. Even if we made
> kunit_stream more structured, I am pretty sure I would want to use
> string_stream or some variation for constructing the message.
> 
> > An "object oriented" (strong quotes!) approach where kunit_fail_msg is
> > the innermost struct in some assertion specific structure might work
> > nicely and allow the test runner to call a generic 'format' function to
> > print out the message based on the type of assertion/expectation it is.
> > It probably would mean less code size too because the strings that are
> > common will be in the common printing function instead of created twice,
> > in the macros/code and then copied to the heap for the string stream.
> >
> >         struct kunit_assert {
> >                 const char *line;
> >                 const char *file;
> >                 const char *func;
> >                 void (*format)(struct kunit_assert *assert);
> >         };
> >
> >         struct kunit_comparison_assert {
> >                 enum operator operator;
> >                 const char *left;
> >                 const char *right;
> >                 struct kunit_assert assert;
> >         };
> >
> >         struct kunit_bool_assert {
> >                 const char *truth;
> >                 const char *statement;
> >                 struct kunit_assert assert;
> >         };
> >
> >         void kunit_format_comparison(struct kunit_assert *assert)
> >         {
> >                 struct kunit_comparison_assert *comp = container_of(assert, ...)
> >
> >                 kunit_printk(...)
> >         }

I started poking around with your suggestion while we are waiting. A
couple early observations:

1) It is actually easier to do than I previously thought and will probably
   help with getting more of the planned TAP output stuff working.

   That being said, this is still a pretty substantial undertaking and
   will likely take *at least* a week to implement and properly review.
   Assuming everything goes extremely well (no unexpected issues on my
   end, very responsive reviewers, etc).

2) It *will* eliminate the need for kunit_stream.

3) ...but, it *will not* eliminate the need for string_stream.

Based on my early observations, I do think it is worth doing, but I
don't think it is worth trying to make it in this patchset (unless I
have already missed the window, or it is going to be open for a while):
I do think it will make things much cleaner, but I don't think it will
achieve your desired goal of getting rid of an unstructured
{kunit|string}_stream style interface; it just adds a layer on top of it
that makes it harder to misuse.

I attached a patch of what I have so far at the end of this email so you
can see what I am talking about. And of course, if you agree with my
assessment, so we can start working on it as a future patch.

A couple things in regard to the patch I attached:

1) I wrote it pretty quickly so there are almost definitely mistakes.
   You should consider it RFC. I did verify it compiles though.

2) Also, I did use kunit_stream in writing it: all occurences should be
   pretty easy to replace with string_stream; nevertheless, the reason
   for this is just to make it easier to play with the current APIs. I
   wanted to have something working before I went through a big tedious
   refactoring. So sorry if it causes any confusion.

3) I also based the patch on all the KUnit patches I have queued up
   (includes things like mocking and such) since I want to see how this
   serialization thing will work with mocks and matchers and things like
   that.

> I started working on something similarish, but by the time I ended up
> coming up with a parent object whose definition was loose enough to
> satisfy all the properties required by the child classes it ended up
> basically being the same as what I have now just with a more complex
> hierarchy of message manipulation logic.
> 
> On the other hand, I didn't have the idea of doing the parent object
> quite the way you did and that would clean up a lot of the duplicated
> first line logic.
> 
> I would like to give it a try, but I am afraid I am going to get
> sucked down a really deep rabbit hole.
> 
> > Maybe other people have opinions here on if you should do it now or
> > later. Future coding is not a great argument because it's hard to
> > predict the future. On the other hand, this patchset is in good shape to
> 
> Yeah, that's kind of why I am afraid to go down this road when I have
> something that works now and I know works with the mocking stuff I
> want to do.
> 
> I would like to try your suggestion, but I want to try to make it work
> with my mocking patches before I commit to it because otherwise I am
> just going to have to back it out in a follow up patchset.
> 
> > merge and I'd like to use it to write unit tests for code I maintain so
> > I don't want to see this stall out. Sorry if I'm opening the can of
> > worms you're talking about.
> 
> Don't be sorry. I agree with you that the kunit_stream stuff is not very pretty.
> 
> Shuah, have we missed the merge window for 5.3?
> 
> I saw you only sent one PR out so far for this release, and there
> wasn't much in it; I imagine you are going to send at least one more?
> 
> I figure, if we still got time to try out your suggestion, Stephen, no
> harm in trying.
> 
> Also if we missed it, then I have another couple months to play around with it.
> 
> What do you think?

I attached the patch mentioned above below. Let me know what you think!

Cheers!

From 53d475d3d56afcf92b452c6d347dbedfa1a17d34 Mon Sep 17 00:00:00 2001
From: Brendan Higgins <brendanhiggins@google.com>
Date: Thu, 18 Jul 2019 16:08:52 -0700
Subject: [PATCH v1] DO NOT MERGE: started playing around with the
 serialization api

---
 include/kunit/assert.h | 130 ++++++++++++++++++++++++++++++
 include/kunit/mock.h   |   4 +
 kunit/Makefile         |   3 +-
 kunit/assert.c         | 179 +++++++++++++++++++++++++++++++++++++++++
 kunit/mock.c           |   6 +-
 5 files changed, 318 insertions(+), 4 deletions(-)
 create mode 100644 include/kunit/assert.h
 create mode 100644 kunit/assert.c

diff --git a/include/kunit/assert.h b/include/kunit/assert.h
new file mode 100644
index 0000000000000..e054fdff4642f
--- /dev/null
+++ b/include/kunit/assert.h
@@ -0,0 +1,130 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Assertion and expectation serialization API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_ASSERT_H
+#define _KUNIT_ASSERT_H
+
+#include <kunit/test.h>
+#include <kunit/mock.h>
+
+enum kunit_assert_type {
+	KUNIT_ASSERTION,
+	KUNIT_EXPECTATION,
+};
+
+struct kunit_assert {
+	enum kunit_assert_type type;
+	const char *line;
+	const char *file;
+	struct va_format message;
+	void (*format)(struct kunit_assert *assert,
+		       struct kunit_stream *stream);
+};
+
+void kunit_base_assert_format(struct kunit_assert *assert,
+			      struct kunit_stream *stream);
+
+void kunit_assert_print_msg(struct kunit_assert *assert,
+			    struct kunit_stream *stream);
+
+struct kunit_unary_assert {
+	struct kunit_assert assert;
+	const char *condition;
+	bool expected_true;
+};
+
+void kunit_unary_assert_format(struct kunit_assert *assert,
+			       struct kunit_stream *stream);
+
+struct kunit_ptr_not_err_assert {
+	struct kunit_assert assert;
+	const char *text;
+	void *value;
+};
+
+void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
+				     struct kunit_stream *stream);
+
+struct kunit_binary_assert {
+	struct kunit_assert assert;
+	const char *operation;
+	const char *left_text;
+	long long left_value;
+	const char *right_text;
+	long long right_value;
+};
+
+void kunit_binary_assert_format(struct kunit_assert *assert,
+				struct kunit_stream *stream);
+
+struct kunit_binary_ptr_assert {
+	struct kunit_assert assert;
+	const char *operation;
+	const char *left_text;
+	void *left_value;
+	const char *right_text;
+	void *right_value;
+};
+
+void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
+				    struct kunit_stream *stream);
+
+struct kunit_binary_str_assert {
+	struct kunit_assert assert;
+	const char *operation;
+	const char *left_text;
+	const char *left_value;
+	const char *right_text;
+	const char *right_value;
+};
+
+void kunit_binary_str_assert_format(struct kunit_assert *assert,
+				    struct kunit_stream *stream);
+
+struct kunit_mock_assert {
+	struct kunit_assert assert;
+};
+
+struct kunit_mock_no_expectations {
+	struct kunit_mock_assert assert;
+};
+
+struct kunit_mock_declaration {
+	const char *function_name;
+	const char **type_names;
+	const void **params;
+	int len;
+};
+
+void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
+				   struct kunit_stream *stream);
+
+struct kunit_matcher_result {
+	struct kunit_assert assert;
+};
+
+struct kunit_mock_failed_match {
+	struct list_head node;
+	const char *expectation_text;
+	struct kunit_matcher_result *matcher_list;
+	size_t matcher_list_len;
+};
+
+void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
+				    struct kunit_stream *stream);
+
+struct kunit_mock_no_match {
+	struct kunit_mock_assert assert;
+	struct kunit_mock_declaration declaration;
+	struct list_head failed_match_list;
+};
+
+void kunit_mock_no_match_format(struct kunit_assert *assert,
+				struct kunit_stream *stream);
+
+#endif /*  _KUNIT_ASSERT_H */
diff --git a/include/kunit/mock.h b/include/kunit/mock.h
index 001b96af62f1e..52c9e427c831b 100644
--- a/include/kunit/mock.h
+++ b/include/kunit/mock.h
@@ -144,6 +144,10 @@ void mock_register_formatter(struct mock_param_formatter *formatter);
 
 void mock_unregister_formatter(struct mock_param_formatter *formatter);
 
+void mock_format_param(struct kunit_stream *stream,
+		       const char *type_name,
+		       const void *param);
+
 struct mock *mock_get_global_mock(void);
 
 #define MOCK(name) name##_mock
diff --git a/kunit/Makefile b/kunit/Makefile
index bbf43fcfb93a9..149d856a30f04 100644
--- a/kunit/Makefile
+++ b/kunit/Makefile
@@ -3,7 +3,8 @@ obj-$(CONFIG_KUNIT) +=			test.o \
 					common-mocks.o \
 					string-stream.o \
 					kunit-stream.o \
-					try-catch.o
+					try-catch.o \
+					assert.o
 
 obj-$(CONFIG_KUNIT_TEST) +=		test-test.o \
 					test-mock.o \
diff --git a/kunit/assert.c b/kunit/assert.c
new file mode 100644
index 0000000000000..75bb6922a994e
--- /dev/null
+++ b/kunit/assert.c
@@ -0,0 +1,179 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Assertion and expectation serialization API.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+#include <kunit/assert.h>
+
+void kunit_base_assert_format(struct kunit_assert *assert,
+			      struct kunit_stream *stream)
+{
+	const char *expect_or_assert;
+
+	if (assert->type == KUNIT_EXPECTATION)
+		expect_or_assert = "EXPECTATION";
+	else
+		expect_or_assert = "ASSERTION";
+
+	kunit_stream_add(stream, "%s FAILED at %s:%s\n",
+			 expect_or_assert, assert->file, assert->line);
+}
+
+void kunit_assert_print_msg(struct kunit_assert *assert,
+			    struct kunit_stream *stream)
+{
+	if (assert->message.fmt)
+		kunit_stream_add(stream, "\n%pV", &assert->message);
+}
+
+void kunit_unary_assert_format(struct kunit_assert *assert,
+			       struct kunit_stream *stream)
+{
+	struct kunit_unary_assert *unary_assert = container_of(
+			assert, struct kunit_unary_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	if (unary_assert->expected_true)
+		kunit_stream_add(stream,
+				 "\tExpected %s to be true, but is false\n",
+				 unary_assert->condition);
+	else
+		kunit_stream_add(stream,
+				 "\tExpected %s to be false, but is true\n",
+				 unary_assert->condition);
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
+				     struct kunit_stream *stream)
+{
+	struct kunit_ptr_not_err_assert *ptr_assert = container_of(
+			assert, struct kunit_ptr_not_err_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	if (!ptr_assert->value) {
+		kunit_stream_add(stream,
+				 "\tExpected %s is not null, but is\n",
+				 ptr_assert->text);
+	} else if (IS_ERR(ptr_assert->value)) {
+		kunit_stream_add(stream,
+				 "\tExpected %s is not error, but is: %ld\n",
+				 ptr_assert->text,
+				 PTR_ERR(ptr_assert->value));
+	}
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_binary_assert_format(struct kunit_assert *assert,
+				struct kunit_stream *stream)
+{
+	struct kunit_binary_assert *binary_assert = container_of(
+			assert, struct kunit_binary_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	kunit_stream_add(stream,
+			 "\tExpected %s %s %s, but\n",
+			 binary_assert->left_text,
+			 binary_assert->operation,
+			 binary_assert->right_text);
+	kunit_stream_add(stream, "\t\t%s == %lld\n",
+			 binary_assert->left_text,
+			 binary_assert->left_value);
+	kunit_stream_add(stream, "\t\t%s == %lld",
+			 binary_assert->right_text,
+			 binary_assert->right_value);
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
+				    struct kunit_stream *stream)
+{
+	struct kunit_binary_ptr_assert *binary_assert = container_of(
+			assert, struct kunit_binary_ptr_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	kunit_stream_add(stream,
+			 "\tExpected %s %s %s, but\n",
+			 binary_assert->left_text,
+			 binary_assert->operation,
+			 binary_assert->right_text);
+	kunit_stream_add(stream, "\t\t%s == %pK\n",
+			 binary_assert->left_text,
+			 binary_assert->left_value);
+	kunit_stream_add(stream, "\t\t%s == %pK",
+			 binary_assert->right_text,
+			 binary_assert->right_value);
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_binary_str_assert_format(struct kunit_assert *assert,
+				    struct kunit_stream *stream)
+{
+	struct kunit_binary_str_assert *binary_assert = container_of(
+			assert, struct kunit_binary_str_assert, assert);
+
+	kunit_base_assert_format(assert, stream);
+	kunit_stream_add(stream,
+			 "\tExpected %s %s %s, but\n",
+			 binary_assert->left_text,
+			 binary_assert->operation,
+			 binary_assert->right_text);
+	kunit_stream_add(stream, "\t\t%s == %s\n",
+			 binary_assert->left_text,
+			 binary_assert->left_value);
+	kunit_stream_add(stream, "\t\t%s == %s",
+			 binary_assert->right_text,
+			 binary_assert->right_value);
+	kunit_assert_print_msg(assert, stream);
+}
+
+void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
+				   struct kunit_stream *stream)
+{
+	int i;
+
+	kunit_stream_add(stream, "%s(", declaration->function_name);
+	for (i = 0; i < declaration->len; i++) {
+		mock_format_param(stream,
+				  declaration->type_names[i],
+				  declaration->params[i]);
+		if (i < declaration->len - 1)
+			kunit_stream_add(stream, ", ");
+	}
+	kunit_stream_add(stream, ")\n");
+}
+
+void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
+				    struct kunit_stream *stream)
+{
+	struct kunit_matcher_result *result;
+	size_t i;
+
+	kunit_stream_add(stream,
+			 "Tried expectation: %s, but\n",
+			 match->expectation_text);
+	for (i = 0; i < match->matcher_list_len; i++) {
+		result = &match->matcher_list[i];
+		kunit_stream_add(stream, "\t");
+		result->assert.format(&result->assert, stream);
+		kunit_stream_add(stream, "\n");
+	}
+}
+
+void kunit_mock_no_match_format(struct kunit_assert *assert,
+				struct kunit_stream *stream)
+{
+	struct kunit_mock_assert *mock_assert = container_of(
+			assert, struct kunit_mock_assert, assert);
+	struct kunit_mock_no_match *no_match = container_of(
+			mock_assert, struct kunit_mock_no_match, assert);
+	struct kunit_mock_failed_match *expectation;
+
+	kunit_base_assert_format(assert, stream);
+	kunit_mock_declaration_format(&no_match->declaration, stream);
+
+	list_for_each_entry(expectation, &no_match->failed_match_list, node)
+		kunit_mock_failed_match_format(expectation, stream);
+}
diff --git a/kunit/mock.c b/kunit/mock.c
index ccb0abe111402..ab441a58a918c 100644
--- a/kunit/mock.c
+++ b/kunit/mock.c
@@ -269,9 +269,9 @@ struct mock_param_formatter *mock_find_formatter(const char *type_name)
 	return NULL;
 }
 
-static void mock_format_param(struct kunit_stream *stream,
-			      const char *type_name,
-			      const void *param)
+void mock_format_param(struct kunit_stream *stream,
+		       const char *type_name,
+		       const void *param)
 {
 	struct mock_param_formatter *formatter;
Brendan Higgins July 22, 2019, 6:10 p.m. UTC | #11
On Thu, Jul 18, 2019 at 5:08 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Thu, Jul 18, 2019 at 12:22:33PM -0700, Brendan Higgins wrote:
> > On Thu, Jul 18, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Brendan Higgins (2019-07-16 11:52:01)
> > > > On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@kernel.org> wrote:
> [...]
> > > Do you have a link to those earlier patches?
> >
> > This is the first patchset:
> >
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788057.html
> >
> > In particular you can see the code for matching functions here:
> >
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788073.html
> >
> > And parameter matching code here:
> >
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788072.html
> >
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1788086.html
> >
> > My apologies in advance, but the code at this early stage had not
> > adopted the kunit_* prefix and was still using the test_* and mock_*
> > prefix. (Hence, struct kunit_stream was known as struct test_stream).
> [...]
> > > The crux of my complaint is that the string stream API is too loosely
> > > defined to be usable. It allows tests to build up a string of
> > > unstructured information, but with certain calling constraints so we
> > > have to tread carefully. If there was more structure to the data that's
> > > being recorded then the test case runner could operate on the data
> > > without having to do string/stream operations, allocations, etc. This
> > > would make the assertion logic much more concrete and specific to kunit,
> > > instead of this small kunit wrapper that's been placed on top of string
> > > stream.
> >
> > Yeah, I can see the point of wanting something that provides more
> > structure than the raw `struct kunit_stream` interface. In fact, it is
> > something I had already started working on, when I had determined it
> > would be a large effort to capture all the variations. I was further
> > put off from the idea when I had been asked to convert the KUnit
> > intermediate format from what I was using to TAP, because, as it is,
> > the current data printed out by KUnit doesn't contain all the data I
> > would like to put in it in a way that best takes advantage of the TAP
> > specification. One problematic area in particular: TAP already
> > provides a way to present a lot of the data I would like to export,
> > but it involves JSON serialization which was an idea that some of the
> > other reviewers understandably weren't too keen on. TAP also wants to
> > report data some time after it is available, which is generally not a
> > good idea for test debug information; you want to make it available as
> > soon as you can or you risk crashing with the data still inside.
> >
> > Hence, I decided we could probably spend a good long while debating
> > how I present the information. So the idea of having a loose
> > definition seemed attractive to me in its own right since it would
> > likely conform to whatever we ended up deciding in the long run. Also,
> > all the better that it was what I already had and no one seemed to
> > mind too much.
> >
> > The only constant I expect is that `struct kunit` will likely need to
> > take an abstract object with a `commit` method, or a `format` method
> > or whatever so it could control when data was going to be printed out
> > to the user. We will probably also use a string builder in there
> > somewhere.
> >
> > > TL;DR: If we can get rid of the string stream API I'd view that as an
> > > improvement because building arbitrary strings in the kernel is complex,
> > > error prone and has calling context concerns.
> >
> > True. No argument there.
> >
> > > Is the intention that other code besides unit tests will use this string
> > > stream API to build up strings? Any targets in mind? This would be a
> > > good way to get the API merged upstream given that its 2019 and we
> > > haven't had such an API in the kernel so far.
> >
> > Someone, (was it you?) asked about code sharing with a string builder
> > thingy that was used for creating structured human readable files, but
> > that seemed like a pretty massive undertaking.
> >
> > Aside from that, no. I would kind of prefered that nobody used it for
> > anything else because I the issues you described.
> >
> > Nevertheless, I think the debate over the usefulness of the
> > string_stream and kunit_stream are separate topics. Even if we made
> > kunit_stream more structured, I am pretty sure I would want to use
> > string_stream or some variation for constructing the message.
> >
> > > An "object oriented" (strong quotes!) approach where kunit_fail_msg is
> > > the innermost struct in some assertion specific structure might work
> > > nicely and allow the test runner to call a generic 'format' function to
> > > print out the message based on the type of assertion/expectation it is.
> > > It probably would mean less code size too because the strings that are
> > > common will be in the common printing function instead of created twice,
> > > in the macros/code and then copied to the heap for the string stream.
> > >
> > >         struct kunit_assert {
> > >                 const char *line;
> > >                 const char *file;
> > >                 const char *func;
> > >                 void (*format)(struct kunit_assert *assert);
> > >         };
> > >
> > >         struct kunit_comparison_assert {
> > >                 enum operator operator;
> > >                 const char *left;
> > >                 const char *right;
> > >                 struct kunit_assert assert;
> > >         };
> > >
> > >         struct kunit_bool_assert {
> > >                 const char *truth;
> > >                 const char *statement;
> > >                 struct kunit_assert assert;
> > >         };
> > >
> > >         void kunit_format_comparison(struct kunit_assert *assert)
> > >         {
> > >                 struct kunit_comparison_assert *comp = container_of(assert, ...)
> > >
> > >                 kunit_printk(...)
> > >         }
>
> I started poking around with your suggestion while we are waiting. A
> couple early observations:
>
> 1) It is actually easier to do than I previously thought and will probably
>    help with getting more of the planned TAP output stuff working.
>
>    That being said, this is still a pretty substantial undertaking and
>    will likely take *at least* a week to implement and properly review.
>    Assuming everything goes extremely well (no unexpected issues on my
>    end, very responsive reviewers, etc).
>
> 2) It *will* eliminate the need for kunit_stream.
>
> 3) ...but, it *will not* eliminate the need for string_stream.
>
> Based on my early observations, I do think it is worth doing, but I
> don't think it is worth trying to make it in this patchset (unless I
> have already missed the window, or it is going to be open for a while):
> I do think it will make things much cleaner, but I don't think it will
> achieve your desired goal of getting rid of an unstructured
> {kunit|string}_stream style interface; it just adds a layer on top of it
> that makes it harder to misuse.
>
> I attached a patch of what I have so far at the end of this email so you
> can see what I am talking about. And of course, if you agree with my
> assessment, so we can start working on it as a future patch.
>
> A couple things in regard to the patch I attached:
>
> 1) I wrote it pretty quickly so there are almost definitely mistakes.
>    You should consider it RFC. I did verify it compiles though.
>
> 2) Also, I did use kunit_stream in writing it: all occurences should be
>    pretty easy to replace with string_stream; nevertheless, the reason
>    for this is just to make it easier to play with the current APIs. I
>    wanted to have something working before I went through a big tedious
>    refactoring. So sorry if it causes any confusion.
>
> 3) I also based the patch on all the KUnit patches I have queued up
>    (includes things like mocking and such) since I want to see how this
>    serialization thing will work with mocks and matchers and things like
>    that.
>
> > I started working on something similarish, but by the time I ended up
> > coming up with a parent object whose definition was loose enough to
> > satisfy all the properties required by the child classes it ended up
> > basically being the same as what I have now just with a more complex
> > hierarchy of message manipulation logic.
> >
> > On the other hand, I didn't have the idea of doing the parent object
> > quite the way you did and that would clean up a lot of the duplicated
> > first line logic.
> >
> > I would like to give it a try, but I am afraid I am going to get
> > sucked down a really deep rabbit hole.
> >
> > > Maybe other people have opinions here on if you should do it now or
> > > later. Future coding is not a great argument because it's hard to
> > > predict the future. On the other hand, this patchset is in good shape to
> >
> > Yeah, that's kind of why I am afraid to go down this road when I have
> > something that works now and I know works with the mocking stuff I
> > want to do.
> >
> > I would like to try your suggestion, but I want to try to make it work
> > with my mocking patches before I commit to it because otherwise I am
> > just going to have to back it out in a follow up patchset.
> >
> > > merge and I'd like to use it to write unit tests for code I maintain so
> > > I don't want to see this stall out. Sorry if I'm opening the can of
> > > worms you're talking about.
> >
> > Don't be sorry. I agree with you that the kunit_stream stuff is not very pretty.
> >
> > Shuah, have we missed the merge window for 5.3?
> >
> > I saw you only sent one PR out so far for this release, and there
> > wasn't much in it; I imagine you are going to send at least one more?
> >
> > I figure, if we still got time to try out your suggestion, Stephen, no
> > harm in trying.
> >
> > Also if we missed it, then I have another couple months to play around with it.
> >
> > What do you think?

I talked to Shuah off thread, she would like us to resolve this
discussion before accepting the patchset.

She also said that this is probably going to have to wait until v5.4.

Nevertheless, Stephen, would you mind taking a look at the patch I
posted below? I would like to get your thoughts on the sum of all the
changes I am going to have to make before I try to integrate them into
the existing patches.

Sorry for being lazy, but I suspect you won't like the first pass of
how I am doing it, and I think it will probably be easier for you to
give early feedback on it as its own change anyway.

> I attached the patch mentioned above below. Let me know what you think!
>
> Cheers!
>
> From 53d475d3d56afcf92b452c6d347dbedfa1a17d34 Mon Sep 17 00:00:00 2001
> From: Brendan Higgins <brendanhiggins@google.com>
> Date: Thu, 18 Jul 2019 16:08:52 -0700
> Subject: [PATCH v1] DO NOT MERGE: started playing around with the
>  serialization api
>
> ---
>  include/kunit/assert.h | 130 ++++++++++++++++++++++++++++++
>  include/kunit/mock.h   |   4 +
>  kunit/Makefile         |   3 +-
>  kunit/assert.c         | 179 +++++++++++++++++++++++++++++++++++++++++
>  kunit/mock.c           |   6 +-
>  5 files changed, 318 insertions(+), 4 deletions(-)
>  create mode 100644 include/kunit/assert.h
>  create mode 100644 kunit/assert.c
>
> diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> new file mode 100644
> index 0000000000000..e054fdff4642f
> --- /dev/null
> +++ b/include/kunit/assert.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Assertion and expectation serialization API.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#ifndef _KUNIT_ASSERT_H
> +#define _KUNIT_ASSERT_H
> +
> +#include <kunit/test.h>
> +#include <kunit/mock.h>
> +
> +enum kunit_assert_type {
> +       KUNIT_ASSERTION,
> +       KUNIT_EXPECTATION,
> +};
> +
> +struct kunit_assert {
> +       enum kunit_assert_type type;
> +       const char *line;
> +       const char *file;
> +       struct va_format message;
> +       void (*format)(struct kunit_assert *assert,
> +                      struct kunit_stream *stream);
> +};
> +
> +void kunit_base_assert_format(struct kunit_assert *assert,
> +                             struct kunit_stream *stream);
> +
> +void kunit_assert_print_msg(struct kunit_assert *assert,
> +                           struct kunit_stream *stream);
> +
> +struct kunit_unary_assert {
> +       struct kunit_assert assert;
> +       const char *condition;
> +       bool expected_true;
> +};
> +
> +void kunit_unary_assert_format(struct kunit_assert *assert,
> +                              struct kunit_stream *stream);
> +
> +struct kunit_ptr_not_err_assert {
> +       struct kunit_assert assert;
> +       const char *text;
> +       void *value;
> +};
> +
> +void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
> +                                    struct kunit_stream *stream);
> +
> +struct kunit_binary_assert {
> +       struct kunit_assert assert;
> +       const char *operation;
> +       const char *left_text;
> +       long long left_value;
> +       const char *right_text;
> +       long long right_value;
> +};
> +
> +void kunit_binary_assert_format(struct kunit_assert *assert,
> +                               struct kunit_stream *stream);
> +
> +struct kunit_binary_ptr_assert {
> +       struct kunit_assert assert;
> +       const char *operation;
> +       const char *left_text;
> +       void *left_value;
> +       const char *right_text;
> +       void *right_value;
> +};
> +
> +void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
> +                                   struct kunit_stream *stream);
> +
> +struct kunit_binary_str_assert {
> +       struct kunit_assert assert;
> +       const char *operation;
> +       const char *left_text;
> +       const char *left_value;
> +       const char *right_text;
> +       const char *right_value;
> +};
> +
> +void kunit_binary_str_assert_format(struct kunit_assert *assert,
> +                                   struct kunit_stream *stream);
> +
> +struct kunit_mock_assert {
> +       struct kunit_assert assert;
> +};
> +
> +struct kunit_mock_no_expectations {
> +       struct kunit_mock_assert assert;
> +};
> +
> +struct kunit_mock_declaration {
> +       const char *function_name;
> +       const char **type_names;
> +       const void **params;
> +       int len;
> +};
> +
> +void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
> +                                  struct kunit_stream *stream);
> +
> +struct kunit_matcher_result {
> +       struct kunit_assert assert;
> +};
> +
> +struct kunit_mock_failed_match {
> +       struct list_head node;
> +       const char *expectation_text;
> +       struct kunit_matcher_result *matcher_list;
> +       size_t matcher_list_len;
> +};
> +
> +void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
> +                                   struct kunit_stream *stream);
> +
> +struct kunit_mock_no_match {
> +       struct kunit_mock_assert assert;
> +       struct kunit_mock_declaration declaration;
> +       struct list_head failed_match_list;
> +};
> +
> +void kunit_mock_no_match_format(struct kunit_assert *assert,
> +                               struct kunit_stream *stream);
> +
> +#endif /*  _KUNIT_ASSERT_H */
> diff --git a/include/kunit/mock.h b/include/kunit/mock.h
> index 001b96af62f1e..52c9e427c831b 100644
> --- a/include/kunit/mock.h
> +++ b/include/kunit/mock.h
> @@ -144,6 +144,10 @@ void mock_register_formatter(struct mock_param_formatter *formatter);
>
>  void mock_unregister_formatter(struct mock_param_formatter *formatter);
>
> +void mock_format_param(struct kunit_stream *stream,
> +                      const char *type_name,
> +                      const void *param);
> +
>  struct mock *mock_get_global_mock(void);
>
>  #define MOCK(name) name##_mock
> diff --git a/kunit/Makefile b/kunit/Makefile
> index bbf43fcfb93a9..149d856a30f04 100644
> --- a/kunit/Makefile
> +++ b/kunit/Makefile
> @@ -3,7 +3,8 @@ obj-$(CONFIG_KUNIT) +=                  test.o \
>                                         common-mocks.o \
>                                         string-stream.o \
>                                         kunit-stream.o \
> -                                       try-catch.o
> +                                       try-catch.o \
> +                                       assert.o
>
>  obj-$(CONFIG_KUNIT_TEST) +=            test-test.o \
>                                         test-mock.o \
> diff --git a/kunit/assert.c b/kunit/assert.c
> new file mode 100644
> index 0000000000000..75bb6922a994e
> --- /dev/null
> +++ b/kunit/assert.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Assertion and expectation serialization API.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +#include <kunit/assert.h>
> +
> +void kunit_base_assert_format(struct kunit_assert *assert,
> +                             struct kunit_stream *stream)
> +{
> +       const char *expect_or_assert;
> +
> +       if (assert->type == KUNIT_EXPECTATION)
> +               expect_or_assert = "EXPECTATION";
> +       else
> +               expect_or_assert = "ASSERTION";
> +
> +       kunit_stream_add(stream, "%s FAILED at %s:%s\n",
> +                        expect_or_assert, assert->file, assert->line);
> +}
> +
> +void kunit_assert_print_msg(struct kunit_assert *assert,
> +                           struct kunit_stream *stream)
> +{
> +       if (assert->message.fmt)
> +               kunit_stream_add(stream, "\n%pV", &assert->message);
> +}
> +
> +void kunit_unary_assert_format(struct kunit_assert *assert,
> +                              struct kunit_stream *stream)
> +{
> +       struct kunit_unary_assert *unary_assert = container_of(
> +                       assert, struct kunit_unary_assert, assert);
> +
> +       kunit_base_assert_format(assert, stream);
> +       if (unary_assert->expected_true)
> +               kunit_stream_add(stream,
> +                                "\tExpected %s to be true, but is false\n",
> +                                unary_assert->condition);
> +       else
> +               kunit_stream_add(stream,
> +                                "\tExpected %s to be false, but is true\n",
> +                                unary_assert->condition);
> +       kunit_assert_print_msg(assert, stream);
> +}
> +
> +void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
> +                                    struct kunit_stream *stream)
> +{
> +       struct kunit_ptr_not_err_assert *ptr_assert = container_of(
> +                       assert, struct kunit_ptr_not_err_assert, assert);
> +
> +       kunit_base_assert_format(assert, stream);
> +       if (!ptr_assert->value) {
> +               kunit_stream_add(stream,
> +                                "\tExpected %s is not null, but is\n",
> +                                ptr_assert->text);
> +       } else if (IS_ERR(ptr_assert->value)) {
> +               kunit_stream_add(stream,
> +                                "\tExpected %s is not error, but is: %ld\n",
> +                                ptr_assert->text,
> +                                PTR_ERR(ptr_assert->value));
> +       }
> +       kunit_assert_print_msg(assert, stream);
> +}
> +
> +void kunit_binary_assert_format(struct kunit_assert *assert,
> +                               struct kunit_stream *stream)
> +{
> +       struct kunit_binary_assert *binary_assert = container_of(
> +                       assert, struct kunit_binary_assert, assert);
> +
> +       kunit_base_assert_format(assert, stream);
> +       kunit_stream_add(stream,
> +                        "\tExpected %s %s %s, but\n",
> +                        binary_assert->left_text,
> +                        binary_assert->operation,
> +                        binary_assert->right_text);
> +       kunit_stream_add(stream, "\t\t%s == %lld\n",
> +                        binary_assert->left_text,
> +                        binary_assert->left_value);
> +       kunit_stream_add(stream, "\t\t%s == %lld",
> +                        binary_assert->right_text,
> +                        binary_assert->right_value);
> +       kunit_assert_print_msg(assert, stream);
> +}
> +

I could probably reduce some of the code duplication here by using a
variable type struct for left_value and right_value, but that would
actually increase the usage of {kunit|string}_stream; it is probably
the right thing to do, but I wanted to get your thoughts on it first.

> +void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
> +                                   struct kunit_stream *stream)
> +{
> +       struct kunit_binary_ptr_assert *binary_assert = container_of(
> +                       assert, struct kunit_binary_ptr_assert, assert);
> +
> +       kunit_base_assert_format(assert, stream);
> +       kunit_stream_add(stream,
> +                        "\tExpected %s %s %s, but\n",
> +                        binary_assert->left_text,
> +                        binary_assert->operation,
> +                        binary_assert->right_text);
> +       kunit_stream_add(stream, "\t\t%s == %pK\n",
> +                        binary_assert->left_text,
> +                        binary_assert->left_value);
> +       kunit_stream_add(stream, "\t\t%s == %pK",
> +                        binary_assert->right_text,
> +                        binary_assert->right_value);
> +       kunit_assert_print_msg(assert, stream);
> +}
> +
> +void kunit_binary_str_assert_format(struct kunit_assert *assert,
> +                                   struct kunit_stream *stream)
> +{
> +       struct kunit_binary_str_assert *binary_assert = container_of(
> +                       assert, struct kunit_binary_str_assert, assert);
> +
> +       kunit_base_assert_format(assert, stream);
> +       kunit_stream_add(stream,
> +                        "\tExpected %s %s %s, but\n",
> +                        binary_assert->left_text,
> +                        binary_assert->operation,
> +                        binary_assert->right_text);
> +       kunit_stream_add(stream, "\t\t%s == %s\n",
> +                        binary_assert->left_text,
> +                        binary_assert->left_value);
> +       kunit_stream_add(stream, "\t\t%s == %s",
> +                        binary_assert->right_text,
> +                        binary_assert->right_value);
> +       kunit_assert_print_msg(assert, stream);
> +}
> +
> +void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
> +                                  struct kunit_stream *stream)
> +{
> +       int i;
> +
> +       kunit_stream_add(stream, "%s(", declaration->function_name);
> +       for (i = 0; i < declaration->len; i++) {
> +               mock_format_param(stream,
> +                                 declaration->type_names[i],
> +                                 declaration->params[i]);
> +               if (i < declaration->len - 1)
> +                       kunit_stream_add(stream, ", ");
> +       }
> +       kunit_stream_add(stream, ")\n");
> +}
> +
> +void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
> +                                   struct kunit_stream *stream)
> +{
> +       struct kunit_matcher_result *result;
> +       size_t i;
> +
> +       kunit_stream_add(stream,
> +                        "Tried expectation: %s, but\n",
> +                        match->expectation_text);
> +       for (i = 0; i < match->matcher_list_len; i++) {
> +               result = &match->matcher_list[i];
> +               kunit_stream_add(stream, "\t");
> +               result->assert.format(&result->assert, stream);
> +               kunit_stream_add(stream, "\n");
> +       }
> +}
> +
> +void kunit_mock_no_match_format(struct kunit_assert *assert,
> +                               struct kunit_stream *stream)
> +{
> +       struct kunit_mock_assert *mock_assert = container_of(
> +                       assert, struct kunit_mock_assert, assert);
> +       struct kunit_mock_no_match *no_match = container_of(
> +                       mock_assert, struct kunit_mock_no_match, assert);
> +       struct kunit_mock_failed_match *expectation;
> +
> +       kunit_base_assert_format(assert, stream);
> +       kunit_mock_declaration_format(&no_match->declaration, stream);
> +
> +       list_for_each_entry(expectation, &no_match->failed_match_list, node)
> +               kunit_mock_failed_match_format(expectation, stream);
> +}
> diff --git a/kunit/mock.c b/kunit/mock.c
> index ccb0abe111402..ab441a58a918c 100644
> --- a/kunit/mock.c
> +++ b/kunit/mock.c
> @@ -269,9 +269,9 @@ struct mock_param_formatter *mock_find_formatter(const char *type_name)
>         return NULL;
>  }
>
> -static void mock_format_param(struct kunit_stream *stream,
> -                             const char *type_name,
> -                             const void *param)
> +void mock_format_param(struct kunit_stream *stream,
> +                      const char *type_name,
> +                      const void *param)
>  {
>         struct mock_param_formatter *formatter;
>
> --
> 2.22.0.657.g960e92d24f-goog
>
Stephen Boyd July 22, 2019, 8:03 p.m. UTC | #12
Quoting Brendan Higgins (2019-07-18 17:08:34)
> On Thu, Jul 18, 2019 at 12:22:33PM -0700, Brendan Higgins wrote:
>
> I started poking around with your suggestion while we are waiting. A
> couple early observations:
> 
> 1) It is actually easier to do than I previously thought and will probably
>    help with getting more of the planned TAP output stuff working.
> 
>    That being said, this is still a pretty substantial undertaking and
>    will likely take *at least* a week to implement and properly review.
>    Assuming everything goes extremely well (no unexpected issues on my
>    end, very responsive reviewers, etc).
> 
> 2) It *will* eliminate the need for kunit_stream.
> 
> 3) ...but, it *will not* eliminate the need for string_stream.
> 
> Based on my early observations, I do think it is worth doing, but I
> don't think it is worth trying to make it in this patchset (unless I
> have already missed the window, or it is going to be open for a while):

The merge window is over. Typically code needs to be settled a few weeks
before it opens (i.e. around -rc4 or -rc5) for most maintainers to pick
up patches for the next merge window.

> I do think it will make things much cleaner, but I don't think it will
> achieve your desired goal of getting rid of an unstructured
> {kunit|string}_stream style interface; it just adds a layer on top of it
> that makes it harder to misuse.

Ok.

> 
> I attached a patch of what I have so far at the end of this email so you
> can see what I am talking about. And of course, if you agree with my
> assessment, so we can start working on it as a future patch.
> 
> A couple things in regard to the patch I attached:
> 
> 1) I wrote it pretty quickly so there are almost definitely mistakes.
>    You should consider it RFC. I did verify it compiles though.
> 
> 2) Also, I did use kunit_stream in writing it: all occurences should be
>    pretty easy to replace with string_stream; nevertheless, the reason
>    for this is just to make it easier to play with the current APIs. I
>    wanted to have something working before I went through a big tedious
>    refactoring. So sorry if it causes any confusion.
> 
> 3) I also based the patch on all the KUnit patches I have queued up
>    (includes things like mocking and such) since I want to see how this
>    serialization thing will work with mocks and matchers and things like
>    that.

Great!

> 
> From 53d475d3d56afcf92b452c6d347dbedfa1a17d34 Mon Sep 17 00:00:00 2001
> From: Brendan Higgins <brendanhiggins@google.com>
> Date: Thu, 18 Jul 2019 16:08:52 -0700
> Subject: [PATCH v1] DO NOT MERGE: started playing around with the
>  serialization api
> 
> ---
>  include/kunit/assert.h | 130 ++++++++++++++++++++++++++++++
>  include/kunit/mock.h   |   4 +
>  kunit/Makefile         |   3 +-
>  kunit/assert.c         | 179 +++++++++++++++++++++++++++++++++++++++++
>  kunit/mock.c           |   6 +-
>  5 files changed, 318 insertions(+), 4 deletions(-)
>  create mode 100644 include/kunit/assert.h
>  create mode 100644 kunit/assert.c
> 
> diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> new file mode 100644
> index 0000000000000..e054fdff4642f
> --- /dev/null
> +++ b/include/kunit/assert.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Assertion and expectation serialization API.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +
> +#ifndef _KUNIT_ASSERT_H
> +#define _KUNIT_ASSERT_H
> +
> +#include <kunit/test.h>
> +#include <kunit/mock.h>
> +
> +enum kunit_assert_type {
> +       KUNIT_ASSERTION,
> +       KUNIT_EXPECTATION,
> +};
> +
> +struct kunit_assert {
> +       enum kunit_assert_type type;
> +       const char *line;
> +       const char *file;
> +       struct va_format message;
> +       void (*format)(struct kunit_assert *assert,
> +                      struct kunit_stream *stream);

Would passing in the test help too?

> +};
> +
> +void kunit_base_assert_format(struct kunit_assert *assert,
> +                             struct kunit_stream *stream);
> +
> +void kunit_assert_print_msg(struct kunit_assert *assert,
> +                           struct kunit_stream *stream);
> +
> +struct kunit_unary_assert {
> +       struct kunit_assert assert;
> +       const char *condition;
> +       bool expected_true;
> +};
> +
> +void kunit_unary_assert_format(struct kunit_assert *assert,
> +                              struct kunit_stream *stream);
> +
> +struct kunit_ptr_not_err_assert {
> +       struct kunit_assert assert;
> +       const char *text;
> +       void *value;
> +};
> +
> +void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
> +                                    struct kunit_stream *stream);
> +
> +struct kunit_binary_assert {
> +       struct kunit_assert assert;
> +       const char *operation;
> +       const char *left_text;
> +       long long left_value;
> +       const char *right_text;
> +       long long right_value;
> +};
> +
> +void kunit_binary_assert_format(struct kunit_assert *assert,
> +                               struct kunit_stream *stream);
> +
> +struct kunit_binary_ptr_assert {
> +       struct kunit_assert assert;
> +       const char *operation;
> +       const char *left_text;
> +       void *left_value;
> +       const char *right_text;
> +       void *right_value;
> +};
> +
> +void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
> +                                   struct kunit_stream *stream);
> +
> +struct kunit_binary_str_assert {
> +       struct kunit_assert assert;
> +       const char *operation;
> +       const char *left_text;
> +       const char *left_value;
> +       const char *right_text;
> +       const char *right_value;
> +};
> +
> +void kunit_binary_str_assert_format(struct kunit_assert *assert,
> +                                   struct kunit_stream *stream);
> +
> +struct kunit_mock_assert {
> +       struct kunit_assert assert;
> +};
> +
> +struct kunit_mock_no_expectations {
> +       struct kunit_mock_assert assert;
> +};

What's the purpose of making a wrapper struct with no other members?
Just to make a different struct for some sort of type checking? I guess
it's OK but I don't think it will be very useful in practice.

> +
> +struct kunit_mock_declaration {
> +       const char *function_name;
> +       const char **type_names;
> +       const void **params;
> +       int len;
> +};
> +
> +void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
> +                                  struct kunit_stream *stream);
> +
> +struct kunit_matcher_result {
> +       struct kunit_assert assert;
> +};
> +
> +struct kunit_mock_failed_match {
> +       struct list_head node;
> +       const char *expectation_text;
> +       struct kunit_matcher_result *matcher_list;

Minor nitpick: this code could use some const sprinkling.

> +       size_t matcher_list_len;
> +};
> +
> +void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
> +                                   struct kunit_stream *stream);
> +
> +struct kunit_mock_no_match {
> +       struct kunit_mock_assert assert;
> +       struct kunit_mock_declaration declaration;
> +       struct list_head failed_match_list;
> +};
> +
> +void kunit_mock_no_match_format(struct kunit_assert *assert,
> +                               struct kunit_stream *stream);
> +
> +#endif /*  _KUNIT_ASSERT_H */
> diff --git a/kunit/assert.c b/kunit/assert.c
> new file mode 100644
> index 0000000000000..75bb6922a994e
> --- /dev/null
> +++ b/kunit/assert.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Assertion and expectation serialization API.
> + *
> + * Copyright (C) 2019, Google LLC.
> + * Author: Brendan Higgins <brendanhiggins@google.com>
> + */
> +#include <kunit/assert.h>
> +
> +void kunit_base_assert_format(struct kunit_assert *assert,
> +                             struct kunit_stream *stream)
> +{
> +       const char *expect_or_assert;
> +
> +       if (assert->type == KUNIT_EXPECTATION)
> +               expect_or_assert = "EXPECTATION";
> +       else
> +               expect_or_assert = "ASSERTION";

Make this is a switch statement so we can have the compiler complain if
an enum is missing.

> +
> +       kunit_stream_add(stream, "%s FAILED at %s:%s\n",
> +                        expect_or_assert, assert->file, assert->line);
> +}
> +
> +void kunit_assert_print_msg(struct kunit_assert *assert,
> +                           struct kunit_stream *stream)
> +{
> +       if (assert->message.fmt)
> +               kunit_stream_add(stream, "\n%pV", &assert->message);
> +}
> +
[...]
> +
> +void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
> +                                   struct kunit_stream *stream)
> +{
> +       struct kunit_matcher_result *result;
> +       size_t i;
> +
> +       kunit_stream_add(stream,
> +                        "Tried expectation: %s, but\n",
> +                        match->expectation_text);
> +       for (i = 0; i < match->matcher_list_len; i++) {
> +               result = &match->matcher_list[i];
> +               kunit_stream_add(stream, "\t");
> +               result->assert.format(&result->assert, stream);
> +               kunit_stream_add(stream, "\n");
> +       }

What's the calling context of the assertions and expectations? I still
don't like the fact that string stream needs to allocate buffers and
throw them into a list somewhere because the calling context matters
there. I'd prefer we just wrote directly to the console/log via printk
instead. That way things are simple because we use the existing
buffering path of printk, but maybe there's some benefit to the string
stream that I don't see? Right now it looks like it builds a string and
then dumps it to printk so I'm sort of lost what the benefit is over
just writing directly with printk.

Maybe it's this part that you wrote up above?

> > Nevertheless, I think the debate over the usefulness of the
> > string_stream and kunit_stream are separate topics. Even if we made
> > kunit_stream more structured, I am pretty sure I would want to use
> > string_stream or some variation for constructing the message.

Why do we need string_stream to construct the message? Can't we just
print it as we process it?
Brendan Higgins July 22, 2019, 10:30 p.m. UTC | #13
On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-07-18 17:08:34)
> > On Thu, Jul 18, 2019 at 12:22:33PM -0700, Brendan Higgins wrote:
> >
> > I started poking around with your suggestion while we are waiting. A
> > couple early observations:
> >
> > 1) It is actually easier to do than I previously thought and will probably
> >    help with getting more of the planned TAP output stuff working.
> >
> >    That being said, this is still a pretty substantial undertaking and
> >    will likely take *at least* a week to implement and properly review.
> >    Assuming everything goes extremely well (no unexpected issues on my
> >    end, very responsive reviewers, etc).
> >
> > 2) It *will* eliminate the need for kunit_stream.
> >
> > 3) ...but, it *will not* eliminate the need for string_stream.
> >
> > Based on my early observations, I do think it is worth doing, but I
> > don't think it is worth trying to make it in this patchset (unless I
> > have already missed the window, or it is going to be open for a while):
>
> The merge window is over. Typically code needs to be settled a few weeks
> before it opens (i.e. around -rc4 or -rc5) for most maintainers to pick
> up patches for the next merge window.

Yeah, it closed on Sunday, right?

I thought we might be able to squeak in since it was *mostly* settled,
and Shuah sent me an email two weeks ago which I interpreted to mean
she was still willing to take it.

In any case, it doesn't matter now.

> > I do think it will make things much cleaner, but I don't think it will
> > achieve your desired goal of getting rid of an unstructured
> > {kunit|string}_stream style interface; it just adds a layer on top of it
> > that makes it harder to misuse.
>
> Ok.
>
> >
> > I attached a patch of what I have so far at the end of this email so you
> > can see what I am talking about. And of course, if you agree with my
> > assessment, so we can start working on it as a future patch.
> >
> > A couple things in regard to the patch I attached:
> >
> > 1) I wrote it pretty quickly so there are almost definitely mistakes.
> >    You should consider it RFC. I did verify it compiles though.
> >
> > 2) Also, I did use kunit_stream in writing it: all occurences should be
> >    pretty easy to replace with string_stream; nevertheless, the reason
> >    for this is just to make it easier to play with the current APIs. I
> >    wanted to have something working before I went through a big tedious
> >    refactoring. So sorry if it causes any confusion.
> >
> > 3) I also based the patch on all the KUnit patches I have queued up
> >    (includes things like mocking and such) since I want to see how this
> >    serialization thing will work with mocks and matchers and things like
> >    that.
>
> Great!
>
> >
> > From 53d475d3d56afcf92b452c6d347dbedfa1a17d34 Mon Sep 17 00:00:00 2001
> > From: Brendan Higgins <brendanhiggins@google.com>
> > Date: Thu, 18 Jul 2019 16:08:52 -0700
> > Subject: [PATCH v1] DO NOT MERGE: started playing around with the
> >  serialization api
> >
> > ---
> >  include/kunit/assert.h | 130 ++++++++++++++++++++++++++++++
> >  include/kunit/mock.h   |   4 +
> >  kunit/Makefile         |   3 +-
> >  kunit/assert.c         | 179 +++++++++++++++++++++++++++++++++++++++++
> >  kunit/mock.c           |   6 +-
> >  5 files changed, 318 insertions(+), 4 deletions(-)
> >  create mode 100644 include/kunit/assert.h
> >  create mode 100644 kunit/assert.c
> >
> > diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> > new file mode 100644
> > index 0000000000000..e054fdff4642f
> > --- /dev/null
> > +++ b/include/kunit/assert.h
> > @@ -0,0 +1,130 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Assertion and expectation serialization API.
> > + *
> > + * Copyright (C) 2019, Google LLC.
> > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > + */
> > +
> > +#ifndef _KUNIT_ASSERT_H
> > +#define _KUNIT_ASSERT_H
> > +
> > +#include <kunit/test.h>
> > +#include <kunit/mock.h>
> > +
> > +enum kunit_assert_type {
> > +       KUNIT_ASSERTION,
> > +       KUNIT_EXPECTATION,
> > +};
> > +
> > +struct kunit_assert {
> > +       enum kunit_assert_type type;
> > +       const char *line;
> > +       const char *file;
> > +       struct va_format message;
> > +       void (*format)(struct kunit_assert *assert,
> > +                      struct kunit_stream *stream);
>
> Would passing in the test help too?

Yeah, it would probably be good to put one in `struct kunit_assert`.

> > +};
> > +
> > +void kunit_base_assert_format(struct kunit_assert *assert,
> > +                             struct kunit_stream *stream);
> > +
> > +void kunit_assert_print_msg(struct kunit_assert *assert,
> > +                           struct kunit_stream *stream);
> > +
> > +struct kunit_unary_assert {
> > +       struct kunit_assert assert;
> > +       const char *condition;
> > +       bool expected_true;
> > +};
> > +
> > +void kunit_unary_assert_format(struct kunit_assert *assert,
> > +                              struct kunit_stream *stream);
> > +
> > +struct kunit_ptr_not_err_assert {
> > +       struct kunit_assert assert;
> > +       const char *text;
> > +       void *value;
> > +};
> > +
> > +void kunit_ptr_not_err_assert_format(struct kunit_assert *assert,
> > +                                    struct kunit_stream *stream);
> > +
> > +struct kunit_binary_assert {
> > +       struct kunit_assert assert;
> > +       const char *operation;
> > +       const char *left_text;
> > +       long long left_value;
> > +       const char *right_text;
> > +       long long right_value;
> > +};
> > +
> > +void kunit_binary_assert_format(struct kunit_assert *assert,
> > +                               struct kunit_stream *stream);
> > +
> > +struct kunit_binary_ptr_assert {
> > +       struct kunit_assert assert;
> > +       const char *operation;
> > +       const char *left_text;
> > +       void *left_value;
> > +       const char *right_text;
> > +       void *right_value;
> > +};
> > +
> > +void kunit_binary_ptr_assert_format(struct kunit_assert *assert,
> > +                                   struct kunit_stream *stream);
> > +
> > +struct kunit_binary_str_assert {
> > +       struct kunit_assert assert;
> > +       const char *operation;
> > +       const char *left_text;
> > +       const char *left_value;
> > +       const char *right_text;
> > +       const char *right_value;
> > +};
> > +
> > +void kunit_binary_str_assert_format(struct kunit_assert *assert,
> > +                                   struct kunit_stream *stream);
> > +
> > +struct kunit_mock_assert {
> > +       struct kunit_assert assert;
> > +};
> > +
> > +struct kunit_mock_no_expectations {
> > +       struct kunit_mock_assert assert;
> > +};
>
> What's the purpose of making a wrapper struct with no other members?
> Just to make a different struct for some sort of type checking? I guess
> it's OK but I don't think it will be very useful in practice.

Yeah, just for typing purposes. I don't mind integrating this into the
current patchset and then deciding if we want it or not.

> > +
> > +struct kunit_mock_declaration {
> > +       const char *function_name;
> > +       const char **type_names;
> > +       const void **params;
> > +       int len;
> > +};
> > +
> > +void kunit_mock_declaration_format(struct kunit_mock_declaration *declaration,
> > +                                  struct kunit_stream *stream);
> > +
> > +struct kunit_matcher_result {
> > +       struct kunit_assert assert;
> > +};
> > +
> > +struct kunit_mock_failed_match {
> > +       struct list_head node;
> > +       const char *expectation_text;
> > +       struct kunit_matcher_result *matcher_list;
>
> Minor nitpick: this code could use some const sprinkling.

Will do.

> > +       size_t matcher_list_len;
> > +};
> > +
> > +void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
> > +                                   struct kunit_stream *stream);
> > +
> > +struct kunit_mock_no_match {
> > +       struct kunit_mock_assert assert;
> > +       struct kunit_mock_declaration declaration;
> > +       struct list_head failed_match_list;
> > +};
> > +
> > +void kunit_mock_no_match_format(struct kunit_assert *assert,
> > +                               struct kunit_stream *stream);
> > +
> > +#endif /*  _KUNIT_ASSERT_H */
> > diff --git a/kunit/assert.c b/kunit/assert.c
> > new file mode 100644
> > index 0000000000000..75bb6922a994e
> > --- /dev/null
> > +++ b/kunit/assert.c
> > @@ -0,0 +1,179 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Assertion and expectation serialization API.
> > + *
> > + * Copyright (C) 2019, Google LLC.
> > + * Author: Brendan Higgins <brendanhiggins@google.com>
> > + */
> > +#include <kunit/assert.h>
> > +
> > +void kunit_base_assert_format(struct kunit_assert *assert,
> > +                             struct kunit_stream *stream)
> > +{
> > +       const char *expect_or_assert;
> > +
> > +       if (assert->type == KUNIT_EXPECTATION)
> > +               expect_or_assert = "EXPECTATION";
> > +       else
> > +               expect_or_assert = "ASSERTION";
>
> Make this is a switch statement so we can have the compiler complain if
> an enum is missing.

Nice call! I didn't know the compiler warned about that. Will fix.

> > +
> > +       kunit_stream_add(stream, "%s FAILED at %s:%s\n",
> > +                        expect_or_assert, assert->file, assert->line);
> > +}
> > +
> > +void kunit_assert_print_msg(struct kunit_assert *assert,
> > +                           struct kunit_stream *stream)
> > +{
> > +       if (assert->message.fmt)
> > +               kunit_stream_add(stream, "\n%pV", &assert->message);
> > +}
> > +
> [...]
> > +
> > +void kunit_mock_failed_match_format(struct kunit_mock_failed_match *match,
> > +                                   struct kunit_stream *stream)
> > +{
> > +       struct kunit_matcher_result *result;
> > +       size_t i;
> > +
> > +       kunit_stream_add(stream,
> > +                        "Tried expectation: %s, but\n",
> > +                        match->expectation_text);
> > +       for (i = 0; i < match->matcher_list_len; i++) {
> > +               result = &match->matcher_list[i];
> > +               kunit_stream_add(stream, "\t");
> > +               result->assert.format(&result->assert, stream);
> > +               kunit_stream_add(stream, "\n");
> > +       }
>
> What's the calling context of the assertions and expectations? I still
> don't like the fact that string stream needs to allocate buffers and
> throw them into a list somewhere because the calling context matters
> there.

The calling context is the same as before, which is anywhere.

> I'd prefer we just wrote directly to the console/log via printk
> instead. That way things are simple because we use the existing
> buffering path of printk, but maybe there's some benefit to the string
> stream that I don't see? Right now it looks like it builds a string and
> then dumps it to printk so I'm sort of lost what the benefit is over
> just writing directly with printk.

It's just buffering it so the whole string gets printed uninterrupted.
If we were to print out piecemeal to printk, couldn't we have another
call to printk come in causing it to garble the KUnit message we are
in the middle of printing?

> Maybe it's this part that you wrote up above?
>
> > > Nevertheless, I think the debate over the usefulness of the
> > > string_stream and kunit_stream are separate topics. Even if we made
> > > kunit_stream more structured, I am pretty sure I would want to use
> > > string_stream or some variation for constructing the message.
>
> Why do we need string_stream to construct the message? Can't we just
> print it as we process it?

See preceding comment.
Stephen Boyd July 22, 2019, 11:54 p.m. UTC | #14
Quoting Brendan Higgins (2019-07-22 15:30:49)
> On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> >
> > What's the calling context of the assertions and expectations? I still
> > don't like the fact that string stream needs to allocate buffers and
> > throw them into a list somewhere because the calling context matters
> > there.
> 
> The calling context is the same as before, which is anywhere.

Ok. That's concerning then.

> 
> > I'd prefer we just wrote directly to the console/log via printk
> > instead. That way things are simple because we use the existing
> > buffering path of printk, but maybe there's some benefit to the string
> > stream that I don't see? Right now it looks like it builds a string and
> > then dumps it to printk so I'm sort of lost what the benefit is over
> > just writing directly with printk.
> 
> It's just buffering it so the whole string gets printed uninterrupted.
> If we were to print out piecemeal to printk, couldn't we have another
> call to printk come in causing it to garble the KUnit message we are
> in the middle of printing?

Yes, printing piecemeal by calling printk many times could lead to
interleaving of messages if something else comes in such as an interrupt
printing something. Printk has some support to hold "records" but I'm
not sure how that would work here because KERN_CONT talks about only
being used early on in boot code. I haven't looked at printk in detail
though so maybe I'm all wrong and KERN_CONT just works?

Can printk be called once with whatever is in the struct? Otherwise if
this is about making printk into a structured log then maybe printk
isn't the proper solution anyway. Maybe a dev interface should be used
instead that can handle starting and stopping tests (via ioctl) in
addition to reading test results, records, etc. with read() and a
clearing of the records. Then the seqfile API works naturally. All of
this is a bit premature, but it looks like you're going down the path of
making something akin to ftrace that stores binary formatted
assertion/expectation records in a lockless ring buffer that then
formats those records when the user asks for them.

I can imagine someone wanting to write unit tests that check conditions
from a simulated hardirq context via irq works (a driver mock
framework?), so this doesn't seem far off.
Brendan Higgins July 23, 2019, 12:32 a.m. UTC | #15
On Mon, Jul 22, 2019 at 4:54 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-07-22 15:30:49)
> > On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > >
> > > What's the calling context of the assertions and expectations? I still
> > > don't like the fact that string stream needs to allocate buffers and
> > > throw them into a list somewhere because the calling context matters
> > > there.
> >
> > The calling context is the same as before, which is anywhere.
>
> Ok. That's concerning then.

Yeah. Luis suggested just not supporting the IRQ context until later.
See my later comment.

> > > I'd prefer we just wrote directly to the console/log via printk
> > > instead. That way things are simple because we use the existing
> > > buffering path of printk, but maybe there's some benefit to the string
> > > stream that I don't see? Right now it looks like it builds a string and
> > > then dumps it to printk so I'm sort of lost what the benefit is over
> > > just writing directly with printk.
> >
> > It's just buffering it so the whole string gets printed uninterrupted.
> > If we were to print out piecemeal to printk, couldn't we have another
> > call to printk come in causing it to garble the KUnit message we are
> > in the middle of printing?
>
> Yes, printing piecemeal by calling printk many times could lead to
> interleaving of messages if something else comes in such as an interrupt
> printing something. Printk has some support to hold "records" but I'm
> not sure how that would work here because KERN_CONT talks about only
> being used early on in boot code. I haven't looked at printk in detail
> though so maybe I'm all wrong and KERN_CONT just works?

It says KERN_CONT is not SMP safe, and it isn't supposed to contain
newlines, so it doesn't sound like it does any buffering for you. I
looked at it a while ago and those comments agreed with my
understanding of the code, but I could be wrong.

> Can printk be called once with whatever is in the struct?

Unfortunately, no. That is part of what I was trying to illustrate
with this patch. Most of the messages are deterministic, but
hardcoding all the possible message types would lead to a massive
number of hard coded strings. However, even this would break down for
the mocking formatters. All the different ways a function can be
called are just too complex to encode into a finite set of hard coded
fmt strings.

> Otherwise if
> this is about making printk into a structured log then maybe printk
> isn't the proper solution anyway. Maybe a dev interface should be used
> instead that can handle starting and stopping tests (via ioctl) in
> addition to reading test results, records, etc. with read() and a
> clearing of the records. Then the seqfile API works naturally. All of

Ehhh...I wouldn't mind providing such an interface, but I would really
like to be able to provide the results without having to depend on a
userland doing something to get test results. That has always been a
pretty important goal for me.

> this is a bit premature, but it looks like you're going down the path of
> making something akin to ftrace that stores binary formatted
> assertion/expectation records in a lockless ring buffer that then
> formats those records when the user asks for them.

Like you said, I think it is a bit premature to go that far.

In anycase, I don't see a way to get rid of string_stream, without
significantly sacrificing usability.

> I can imagine someone wanting to write unit tests that check conditions
> from a simulated hardirq context via irq works (a driver mock
> framework?), so this doesn't seem far off.

Yep, I actually presented the first pieces of that in the RFC v1 that
I linked to you earlier in this discussion. I have a more fleshed out
example here:

https://kunit.googlesource.com/linux/+/e10484ad2f9fc7926412ec84739fe105981b4771/drivers/i2c/busses/i2c-aspeed-test.c

I actually already have some people at Google playing around with it.
So yeah, not far off at all! However, in these cases we are not
actually running in the IRQ context (despite the fact that we are
testing IRQ code) because we provide a fake IRQ chip, or some other
fake mechanism that triggers the IRQ. Still, I could see someone
wanting to do it in a non-fake-IRQ context.

Luis' suggestion was just to hold off on the IRQ safe stuff at the
outset, since that is going to require a lot more effort to review. I
know that's kind of the future coding argument again, but maybe the
answer will be to just restrict what features an IRQ user has access
to (maybe just really simple expectations, for example). I mean, we
will probably have to restrict what they are allowed to use anyway.

Luis, do you have any ideas?

Cheers
Petr Mladek July 24, 2019, 7:31 a.m. UTC | #16
On Mon 2019-07-22 16:54:10, Stephen Boyd wrote:
> Quoting Brendan Higgins (2019-07-22 15:30:49)
> > On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > >
> > > What's the calling context of the assertions and expectations? I still
> > > don't like the fact that string stream needs to allocate buffers and
> > > throw them into a list somewhere because the calling context matters
> > > there.
> > 
> > The calling context is the same as before, which is anywhere.
> 
> Ok. That's concerning then.
> 
> > 
> > > I'd prefer we just wrote directly to the console/log via printk
> > > instead. That way things are simple because we use the existing
> > > buffering path of printk, but maybe there's some benefit to the string
> > > stream that I don't see? Right now it looks like it builds a string and
> > > then dumps it to printk so I'm sort of lost what the benefit is over
> > > just writing directly with printk.
> > 
> > It's just buffering it so the whole string gets printed uninterrupted.
> > If we were to print out piecemeal to printk, couldn't we have another
> > call to printk come in causing it to garble the KUnit message we are
> > in the middle of printing?
> 
> Yes, printing piecemeal by calling printk many times could lead to
> interleaving of messages if something else comes in such as an interrupt
> printing something. Printk has some support to hold "records" but I'm
> not sure how that would work here because KERN_CONT talks about only
> being used early on in boot code. I haven't looked at printk in detail
> though so maybe I'm all wrong and KERN_CONT just works?

KERN_CONT does not guarantee that the message will get printed
together. The pieces get interleaved with messages printed in
parallel.

Note that KERN_CONT was originally really meant to be used only during
boot. It was later used more widely and ended in the best effort category.

There were several attempts to make it more reliable. But it was
always either too complicated or error prone or both.

You need to use your own buffering if you rely want perfect output.
The question is if it is really worth the complexity. Also note that
any buffering reduces the chance that the messages will reach
the console.

BTW: There is a work in progress on a lockless printk ring buffer.
It will make printk() more secure regarding deadlocks. But it might
make transparent handling of continuous lines even more tricky.

I guess that local buffering, before calling printk(), will be
even more important then. Well, it might really force us to create
an API for it.


> Can printk be called once with whatever is in the struct? Otherwise if
> this is about making printk into a structured log then maybe printk
> isn't the proper solution anyway. Maybe a dev interface should be used
> instead that can handle starting and stopping tests (via ioctl) in
> addition to reading test results, records, etc. with read() and a
> clearing of the records. Then the seqfile API works naturally. All of
> this is a bit premature, but it looks like you're going down the path of
> making something akin to ftrace that stores binary formatted
> assertion/expectation records in a lockless ring buffer that then
> formats those records when the user asks for them.

IMHO, ftrace postpones the text formatting primary because it does not
not want to slow down the traced code more than necessary. It is yet
another layer and there should be some strong reason for it.

> I can imagine someone wanting to write unit tests that check conditions
> from a simulated hardirq context via irq works (a driver mock
> framework?), so this doesn't seem far off.

Note that stroring the messages into the printk log is basically safe in any
context. It uses temporary per-CPU buffers for recursive messages and
in NMI. The only problem is panic() when some CPU gets stuck with the
lock taken. This will get solved by the lockless ringbuffer. Also
the temporary buffers will not be necessary any longer.

Much bigger problems are with consoles. There are many of them. It
means a lot of code and more locks involved, including scheduler
locks. Note that console lock is a semaphore.

Best Regards,
Petr
Brendan Higgins July 25, 2019, 8:21 p.m. UTC | #17
On Wed, Jul 24, 2019 at 12:31 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Mon 2019-07-22 16:54:10, Stephen Boyd wrote:
> > Quoting Brendan Higgins (2019-07-22 15:30:49)
> > > On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > >
> > > > What's the calling context of the assertions and expectations? I still
> > > > don't like the fact that string stream needs to allocate buffers and
> > > > throw them into a list somewhere because the calling context matters
> > > > there.
> > >
> > > The calling context is the same as before, which is anywhere.
> >
> > Ok. That's concerning then.
> >
> > >
> > > > I'd prefer we just wrote directly to the console/log via printk
> > > > instead. That way things are simple because we use the existing
> > > > buffering path of printk, but maybe there's some benefit to the string
> > > > stream that I don't see? Right now it looks like it builds a string and
> > > > then dumps it to printk so I'm sort of lost what the benefit is over
> > > > just writing directly with printk.
> > >
> > > It's just buffering it so the whole string gets printed uninterrupted.
> > > If we were to print out piecemeal to printk, couldn't we have another
> > > call to printk come in causing it to garble the KUnit message we are
> > > in the middle of printing?
> >
> > Yes, printing piecemeal by calling printk many times could lead to
> > interleaving of messages if something else comes in such as an interrupt
> > printing something. Printk has some support to hold "records" but I'm
> > not sure how that would work here because KERN_CONT talks about only
> > being used early on in boot code. I haven't looked at printk in detail
> > though so maybe I'm all wrong and KERN_CONT just works?
>
> KERN_CONT does not guarantee that the message will get printed
> together. The pieces get interleaved with messages printed in
> parallel.
>
> Note that KERN_CONT was originally really meant to be used only during
> boot. It was later used more widely and ended in the best effort category.
>
> There were several attempts to make it more reliable. But it was
> always either too complicated or error prone or both.
>
> You need to use your own buffering if you rely want perfect output.
> The question is if it is really worth the complexity. Also note that
> any buffering reduces the chance that the messages will reach
> the console.

Seems like that settles it then. Thanks!

> BTW: There is a work in progress on a lockless printk ring buffer.
> It will make printk() more secure regarding deadlocks. But it might
> make transparent handling of continuous lines even more tricky.
>
> I guess that local buffering, before calling printk(), will be
> even more important then. Well, it might really force us to create
> an API for it.

Cool! Can you CC me on that discussion?

> > Can printk be called once with whatever is in the struct? Otherwise if
> > this is about making printk into a structured log then maybe printk
> > isn't the proper solution anyway. Maybe a dev interface should be used
> > instead that can handle starting and stopping tests (via ioctl) in
> > addition to reading test results, records, etc. with read() and a
> > clearing of the records. Then the seqfile API works naturally. All of
> > this is a bit premature, but it looks like you're going down the path of
> > making something akin to ftrace that stores binary formatted
> > assertion/expectation records in a lockless ring buffer that then
> > formats those records when the user asks for them.
>
> IMHO, ftrace postpones the text formatting primary because it does not
> not want to slow down the traced code more than necessary. It is yet
> another layer and there should be some strong reason for it.

Noted. Yeah, I would prefer avoiding printing out the info at a separate time.

> > I can imagine someone wanting to write unit tests that check conditions
> > from a simulated hardirq context via irq works (a driver mock
> > framework?), so this doesn't seem far off.
>
> Note that stroring the messages into the printk log is basically safe in any
> context. It uses temporary per-CPU buffers for recursive messages and
> in NMI. The only problem is panic() when some CPU gets stuck with the
> lock taken. This will get solved by the lockless ringbuffer. Also
> the temporary buffers will not be necessary any longer.

Sure, I think Stephen's concern is all the supporting code that is
involved. Not printk specifically. It just means a lot more of KUnit
has to be IRQ safe.

> Much bigger problems are with consoles. There are many of them. It
> means a lot of code and more locks involved, including scheduler
> locks. Note that console lock is a semaphore.

That shouldn't affect us though, right? As long as we continue to use
the printk interface?
Petr Mladek July 26, 2019, 8:31 a.m. UTC | #18
On Thu 2019-07-25 13:21:12, Brendan Higgins wrote:
> On Wed, Jul 24, 2019 at 12:31 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Mon 2019-07-22 16:54:10, Stephen Boyd wrote:
> > > Quoting Brendan Higgins (2019-07-22 15:30:49)
> > > > On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > > >
> > > > >
> > > > > What's the calling context of the assertions and expectations? I still
> > > > > don't like the fact that string stream needs to allocate buffers and
> > > > > throw them into a list somewhere because the calling context matters
> > > > > there.
> > > >
> > > > The calling context is the same as before, which is anywhere.
> > >
> > > Ok. That's concerning then.
> > >
> > > >
> > > > > I'd prefer we just wrote directly to the console/log via printk
> > > > > instead. That way things are simple because we use the existing
> > > > > buffering path of printk, but maybe there's some benefit to the string
> > > > > stream that I don't see? Right now it looks like it builds a string and
> > > > > then dumps it to printk so I'm sort of lost what the benefit is over
> > > > > just writing directly with printk.
> > > >
> > > > It's just buffering it so the whole string gets printed uninterrupted.
> > > > If we were to print out piecemeal to printk, couldn't we have another
> > > > call to printk come in causing it to garble the KUnit message we are
> > > > in the middle of printing?
> > >
> > > Yes, printing piecemeal by calling printk many times could lead to
> > > interleaving of messages if something else comes in such as an interrupt
> > > printing something. Printk has some support to hold "records" but I'm
> > > not sure how that would work here because KERN_CONT talks about only
> > > being used early on in boot code. I haven't looked at printk in detail
> > > though so maybe I'm all wrong and KERN_CONT just works?
> >
> > KERN_CONT does not guarantee that the message will get printed
> > together. The pieces get interleaved with messages printed in
> > parallel.
> >
> > Note that KERN_CONT was originally really meant to be used only during
> > boot. It was later used more widely and ended in the best effort category.
> >
> > There were several attempts to make it more reliable. But it was
> > always either too complicated or error prone or both.
> >
> > You need to use your own buffering if you rely want perfect output.
> > The question is if it is really worth the complexity. Also note that
> > any buffering reduces the chance that the messages will reach
> > the console.
> 
> Seems like that settles it then. Thanks!
> 
> > BTW: There is a work in progress on a lockless printk ring buffer.
> > It will make printk() more secure regarding deadlocks. But it might
> > make transparent handling of continuous lines even more tricky.
> >
> > I guess that local buffering, before calling printk(), will be
> > even more important then. Well, it might really force us to create
> > an API for it.
> 
> Cool! Can you CC me on that discussion?

Adding John Oggness into CC.

John, please CC Brendan Higgins on the patchsets eventually switching
printk() into the lockless buffer. The test framework is going to
do its own buffering to keep the related messages together.

The lockless ringbuffer might make handling of related (partial)
lines worse or better. It might justify KUnit's extra buffering
or it might allow to get rid of it.

> > Note that stroring the messages into the printk log is basically safe in any
> > context. It uses temporary per-CPU buffers for recursive messages and
> > in NMI. The only problem is panic() when some CPU gets stuck with the
> > lock taken. This will get solved by the lockless ringbuffer. Also
> > the temporary buffers will not be necessary any longer.
> 
> Sure, I think Stephen's concern is all the supporting code that is
> involved. Not printk specifically. It just means a lot more of KUnit
> has to be IRQ safe.

I see.

BTW: I wonder if KUnit could reuse the existing seq_buf implementation
for buffering messages.

I am sorry if it has already been proposed and rejected for some
reason. I might have missed it. Feel free to just point me to
same older mail.

> > Much bigger problems are with consoles. There are many of them. It
> > means a lot of code and more locks involved, including scheduler
> > locks. Note that console lock is a semaphore.
> 
> That shouldn't affect us though, right? As long as we continue to use
> the printk interface?

I guess that it should not affect KUnit.

The only problem might be if the testing framework calls printk()
inside scheduler or console code. And only when the tested code
uses the same locks that will be used by the called printk().

To be honest I do not fully understand KUnit design. I am not
completely sure how the tested code is isolated from the running
system. Namely, I do not know if the tested code shares
the same locks with the system running the test.

Best Regards,
Petr
Brendan Higgins Aug. 1, 2019, 6:55 p.m. UTC | #19
On Fri, Jul 26, 2019 at 1:31 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Thu 2019-07-25 13:21:12, Brendan Higgins wrote:
> > On Wed, Jul 24, 2019 at 12:31 AM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > On Mon 2019-07-22 16:54:10, Stephen Boyd wrote:
> > > > Quoting Brendan Higgins (2019-07-22 15:30:49)
> > > > > On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > > > >
> > > > > >
> > > > > > What's the calling context of the assertions and expectations? I still
> > > > > > don't like the fact that string stream needs to allocate buffers and
> > > > > > throw them into a list somewhere because the calling context matters
> > > > > > there.
> > > > >
> > > > > The calling context is the same as before, which is anywhere.
> > > >
> > > > Ok. That's concerning then.
> > > >
> > > > >
> > > > > > I'd prefer we just wrote directly to the console/log via printk
> > > > > > instead. That way things are simple because we use the existing
> > > > > > buffering path of printk, but maybe there's some benefit to the string
> > > > > > stream that I don't see? Right now it looks like it builds a string and
> > > > > > then dumps it to printk so I'm sort of lost what the benefit is over
> > > > > > just writing directly with printk.
> > > > >
> > > > > It's just buffering it so the whole string gets printed uninterrupted.
> > > > > If we were to print out piecemeal to printk, couldn't we have another
> > > > > call to printk come in causing it to garble the KUnit message we are
> > > > > in the middle of printing?
> > > >
> > > > Yes, printing piecemeal by calling printk many times could lead to
> > > > interleaving of messages if something else comes in such as an interrupt
> > > > printing something. Printk has some support to hold "records" but I'm
> > > > not sure how that would work here because KERN_CONT talks about only
> > > > being used early on in boot code. I haven't looked at printk in detail
> > > > though so maybe I'm all wrong and KERN_CONT just works?
> > >
> > > KERN_CONT does not guarantee that the message will get printed
> > > together. The pieces get interleaved with messages printed in
> > > parallel.
> > >
> > > Note that KERN_CONT was originally really meant to be used only during
> > > boot. It was later used more widely and ended in the best effort category.
> > >
> > > There were several attempts to make it more reliable. But it was
> > > always either too complicated or error prone or both.
> > >
> > > You need to use your own buffering if you rely want perfect output.
> > > The question is if it is really worth the complexity. Also note that
> > > any buffering reduces the chance that the messages will reach
> > > the console.
> >
> > Seems like that settles it then. Thanks!
> >
> > > BTW: There is a work in progress on a lockless printk ring buffer.
> > > It will make printk() more secure regarding deadlocks. But it might
> > > make transparent handling of continuous lines even more tricky.
> > >
> > > I guess that local buffering, before calling printk(), will be
> > > even more important then. Well, it might really force us to create
> > > an API for it.
> >
> > Cool! Can you CC me on that discussion?
>
> Adding John Oggness into CC.
>
> John, please CC Brendan Higgins on the patchsets eventually switching
> printk() into the lockless buffer. The test framework is going to
> do its own buffering to keep the related messages together.
>
> The lockless ringbuffer might make handling of related (partial)
> lines worse or better. It might justify KUnit's extra buffering
> or it might allow to get rid of it.

Thanks for CC'ing me on the printk ringbuffer thread. It looks like it
actually probably won't affect my needs for KUnit logging. The biggest
reason I need some sort of buffering system is to be able to compose
messages piece meal into a single message that will be printed out to
the user as a single message with no messages from other printk
callers printed out in the middle of mine.

The prb does look interesting; however, it appears that to get the
semantics that I need, I would have to put my entire message in a
single data block and would consequently need to know the size of my
message a priori, which is problematic. Consequently, it seems as
though I will probably need to compose my entire message using my own
buffering system.

> > > Note that stroring the messages into the printk log is basically safe in any
> > > context. It uses temporary per-CPU buffers for recursive messages and
> > > in NMI. The only problem is panic() when some CPU gets stuck with the
> > > lock taken. This will get solved by the lockless ringbuffer. Also
> > > the temporary buffers will not be necessary any longer.
> >
> > Sure, I think Stephen's concern is all the supporting code that is
> > involved. Not printk specifically. It just means a lot more of KUnit
> > has to be IRQ safe.
>
> I see.
>
> BTW: I wonder if KUnit could reuse the existing seq_buf implementation
> for buffering messages.
>
> I am sorry if it has already been proposed and rejected for some
> reason. I might have missed it. Feel free to just point me to
> same older mail.

Yeah, we discussed it briefly here:

https://lkml.org/lkml/2019/5/17/497

Looks like I forgot to include my reasoning in the commit text, sorry
about that.

> > > Much bigger problems are with consoles. There are many of them. It
> > > means a lot of code and more locks involved, including scheduler
> > > locks. Note that console lock is a semaphore.
> >
> > That shouldn't affect us though, right? As long as we continue to use
> > the printk interface?
>
> I guess that it should not affect KUnit.
>
> The only problem might be if the testing framework calls printk()
> inside scheduler or console code. And only when the tested code
> uses the same locks that will be used by the called printk().

Yeah, well printk will not be our only problem in those instances.

> To be honest I do not fully understand KUnit design. I am not
> completely sure how the tested code is isolated from the running
> system. Namely, I do not know if the tested code shares
> the same locks with the system running the test.

No worries, I don't expect printk to be the hang up in those cases. It
sounds like KUnit has a long way to evolve before printk is going to
be a limitation.

Thanks!
Brendan Higgins Aug. 1, 2019, 6:59 p.m. UTC | #20
On Thu, Aug 1, 2019 at 11:55 AM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Fri, Jul 26, 2019 at 1:31 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > On Thu 2019-07-25 13:21:12, Brendan Higgins wrote:
> > > On Wed, Jul 24, 2019 at 12:31 AM Petr Mladek <pmladek@suse.com> wrote:
> > > >
> > > > On Mon 2019-07-22 16:54:10, Stephen Boyd wrote:
> > > > > Quoting Brendan Higgins (2019-07-22 15:30:49)
> > > > > > On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
> > > > > > >
> > > > > > >
> > > > > > > What's the calling context of the assertions and expectations? I still
> > > > > > > don't like the fact that string stream needs to allocate buffers and
> > > > > > > throw them into a list somewhere because the calling context matters
> > > > > > > there.
> > > > > >
> > > > > > The calling context is the same as before, which is anywhere.
> > > > >
> > > > > Ok. That's concerning then.
> > > > >
> > > > > >
> > > > > > > I'd prefer we just wrote directly to the console/log via printk
> > > > > > > instead. That way things are simple because we use the existing
> > > > > > > buffering path of printk, but maybe there's some benefit to the string
> > > > > > > stream that I don't see? Right now it looks like it builds a string and
> > > > > > > then dumps it to printk so I'm sort of lost what the benefit is over
> > > > > > > just writing directly with printk.
> > > > > >
> > > > > > It's just buffering it so the whole string gets printed uninterrupted.
> > > > > > If we were to print out piecemeal to printk, couldn't we have another
> > > > > > call to printk come in causing it to garble the KUnit message we are
> > > > > > in the middle of printing?
> > > > >
> > > > > Yes, printing piecemeal by calling printk many times could lead to
> > > > > interleaving of messages if something else comes in such as an interrupt
> > > > > printing something. Printk has some support to hold "records" but I'm
> > > > > not sure how that would work here because KERN_CONT talks about only
> > > > > being used early on in boot code. I haven't looked at printk in detail
> > > > > though so maybe I'm all wrong and KERN_CONT just works?
> > > >
> > > > KERN_CONT does not guarantee that the message will get printed
> > > > together. The pieces get interleaved with messages printed in
> > > > parallel.
> > > >
> > > > Note that KERN_CONT was originally really meant to be used only during
> > > > boot. It was later used more widely and ended in the best effort category.
> > > >
> > > > There were several attempts to make it more reliable. But it was
> > > > always either too complicated or error prone or both.
> > > >
> > > > You need to use your own buffering if you rely want perfect output.
> > > > The question is if it is really worth the complexity. Also note that
> > > > any buffering reduces the chance that the messages will reach
> > > > the console.
> > >
> > > Seems like that settles it then. Thanks!
> > >
> > > > BTW: There is a work in progress on a lockless printk ring buffer.
> > > > It will make printk() more secure regarding deadlocks. But it might
> > > > make transparent handling of continuous lines even more tricky.
> > > >
> > > > I guess that local buffering, before calling printk(), will be
> > > > even more important then. Well, it might really force us to create
> > > > an API for it.
> > >
> > > Cool! Can you CC me on that discussion?
> >
> > Adding John Oggness into CC.
> >
> > John, please CC Brendan Higgins on the patchsets eventually switching
> > printk() into the lockless buffer. The test framework is going to
> > do its own buffering to keep the related messages together.
> >
> > The lockless ringbuffer might make handling of related (partial)
> > lines worse or better. It might justify KUnit's extra buffering
> > or it might allow to get rid of it.
>
> Thanks for CC'ing me on the printk ringbuffer thread. It looks like it
> actually probably won't affect my needs for KUnit logging. The biggest
> reason I need some sort of buffering system is to be able to compose
> messages piece meal into a single message that will be printed out to
> the user as a single message with no messages from other printk
> callers printed out in the middle of mine.
>
> The prb does look interesting; however, it appears that to get the
> semantics that I need, I would have to put my entire message in a
> single data block and would consequently need to know the size of my
> message a priori, which is problematic. Consequently, it seems as
> though I will probably need to compose my entire message using my own
> buffering system.
>
> > > > Note that stroring the messages into the printk log is basically safe in any
> > > > context. It uses temporary per-CPU buffers for recursive messages and
> > > > in NMI. The only problem is panic() when some CPU gets stuck with the
> > > > lock taken. This will get solved by the lockless ringbuffer. Also
> > > > the temporary buffers will not be necessary any longer.
> > >
> > > Sure, I think Stephen's concern is all the supporting code that is
> > > involved. Not printk specifically. It just means a lot more of KUnit
> > > has to be IRQ safe.
> >
> > I see.
> >
> > BTW: I wonder if KUnit could reuse the existing seq_buf implementation
> > for buffering messages.
> >
> > I am sorry if it has already been proposed and rejected for some
> > reason. I might have missed it. Feel free to just point me to
> > same older mail.
>
> Yeah, we discussed it briefly here:
>
> https://lkml.org/lkml/2019/5/17/497
>
> Looks like I forgot to include my reasoning in the commit text, sorry
> about that.
>
> > > > Much bigger problems are with consoles. There are many of them. It
> > > > means a lot of code and more locks involved, including scheduler
> > > > locks. Note that console lock is a semaphore.
> > >
> > > That shouldn't affect us though, right? As long as we continue to use
> > > the printk interface?
> >
> > I guess that it should not affect KUnit.
> >
> > The only problem might be if the testing framework calls printk()
> > inside scheduler or console code. And only when the tested code
> > uses the same locks that will be used by the called printk().
>
> Yeah, well printk will not be our only problem in those instances.
>
> > To be honest I do not fully understand KUnit design. I am not
> > completely sure how the tested code is isolated from the running
> > system. Namely, I do not know if the tested code shares
> > the same locks with the system running the test.
>
> No worries, I don't expect printk to be the hang up in those cases. It
> sounds like KUnit has a long way to evolve before printk is going to
> be a limitation.

So Stephen, what do you think?

Do you want me to go forward with the new kunit_assert API wrapping
the string_stream as I have it now? Would you prefer to punt this to a
later patch? Or would you prefer something else?

Cheers
Stephen Boyd Aug. 1, 2019, 9:14 p.m. UTC | #21
Quoting Brendan Higgins (2019-08-01 11:59:57)
> On Thu, Aug 1, 2019 at 11:55 AM Brendan Higgins
> <brendanhiggins@google.com> wrote:
> >
> > On Fri, Jul 26, 2019 at 1:31 AM Petr Mladek <pmladek@suse.com> wrote:
> >
> > > To be honest I do not fully understand KUnit design. I am not
> > > completely sure how the tested code is isolated from the running
> > > system. Namely, I do not know if the tested code shares
> > > the same locks with the system running the test.
> >
> > No worries, I don't expect printk to be the hang up in those cases. It
> > sounds like KUnit has a long way to evolve before printk is going to
> > be a limitation.
> 
> So Stephen, what do you think?
> 
> Do you want me to go forward with the new kunit_assert API wrapping
> the string_stream as I have it now? Would you prefer to punt this to a
> later patch? Or would you prefer something else?
> 

I like the struct based approach. If anything, it can be adjusted to
make the code throw some records into a spinlock later on and delay the
formatting of the assertion if need be. Can you resend with that
approach? I don't think I'll have any more comments after that.
Brendan Higgins Aug. 1, 2019, 9:43 p.m. UTC | #22
On Thu, Aug 1, 2019 at 2:14 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2019-08-01 11:59:57)
> > On Thu, Aug 1, 2019 at 11:55 AM Brendan Higgins
> > <brendanhiggins@google.com> wrote:
> > >
> > > On Fri, Jul 26, 2019 at 1:31 AM Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > > To be honest I do not fully understand KUnit design. I am not
> > > > completely sure how the tested code is isolated from the running
> > > > system. Namely, I do not know if the tested code shares
> > > > the same locks with the system running the test.
> > >
> > > No worries, I don't expect printk to be the hang up in those cases. It
> > > sounds like KUnit has a long way to evolve before printk is going to
> > > be a limitation.
> >
> > So Stephen, what do you think?
> >
> > Do you want me to go forward with the new kunit_assert API wrapping
> > the string_stream as I have it now? Would you prefer to punt this to a
> > later patch? Or would you prefer something else?
> >
>
> I like the struct based approach. If anything, it can be adjusted to
> make the code throw some records into a spinlock later on and delay the
> formatting of the assertion if need be.

That's a fair point.

> Can you resend with that
> approach? I don't think I'll have any more comments after that.

Cool, will do.

Thanks!
John Ogness Aug. 2, 2019, 7:37 a.m. UTC | #23
On 2019-08-01, Brendan Higgins <brendanhiggins@google.com> wrote:
> On Fri, Jul 26, 2019 at 1:31 AM Petr Mladek <pmladek@suse.com> wrote:
>> On Thu 2019-07-25 13:21:12, Brendan Higgins wrote:
>>> On Wed, Jul 24, 2019 at 12:31 AM Petr Mladek <pmladek@suse.com> wrote:
>>>> On Mon 2019-07-22 16:54:10, Stephen Boyd wrote:
>>>>> Quoting Brendan Higgins (2019-07-22 15:30:49)
>>>>>> On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
>>>>>>> What's the calling context of the assertions and expectations? I
>>>>>>> still don't like the fact that string stream needs to allocate
>>>>>>> buffers and throw them into a list somewhere because the calling
>>>>>>> context matters there.
>>>>>>
>>>>>> The calling context is the same as before, which is anywhere.
>>>>>
>>>>> Ok. That's concerning then.
>>>>>
>>>>>>> I'd prefer we just wrote directly to the console/log via printk
>>>>>>> instead. That way things are simple because we use the existing
>>>>>>> buffering path of printk, but maybe there's some benefit to the
>>>>>>> string stream that I don't see? Right now it looks like it
>>>>>>> builds a string and then dumps it to printk so I'm sort of lost
>>>>>>> what the benefit is over just writing directly with printk.
>>>>>>
>>>>>> It's just buffering it so the whole string gets printed
>>>>>> uninterrupted.  If we were to print out piecemeal to printk,
>>>>>> couldn't we have another call to printk come in causing it to
>>>>>> garble the KUnit message we are in the middle of printing?
>>>>>
>>>>> Yes, printing piecemeal by calling printk many times could lead to
>>>>> interleaving of messages if something else comes in such as an
>>>>> interrupt printing something. Printk has some support to hold
>>>>> "records" but I'm not sure how that would work here because
>>>>> KERN_CONT talks about only being used early on in boot code. I
>>>>> haven't looked at printk in detail though so maybe I'm all wrong
>>>>> and KERN_CONT just works?
>>>>
>>>> KERN_CONT does not guarantee that the message will get printed
>>>> together. The pieces get interleaved with messages printed in
>>>> parallel.
>>>>
>>>> Note that KERN_CONT was originally really meant to be used only
>>>> during boot. It was later used more widely and ended in the best
>>>> effort category.
>>>>
>>>> There were several attempts to make it more reliable. But it was
>>>> always either too complicated or error prone or both.
>>>>
>>>> You need to use your own buffering if you rely want perfect output.
>>>> The question is if it is really worth the complexity. Also note
>>>> that any buffering reduces the chance that the messages will reach
>>>> the console.
>>>
>>> Seems like that settles it then. Thanks!
>>>
>>>> BTW: There is a work in progress on a lockless printk ring buffer.
>>>> It will make printk() more secure regarding deadlocks. But it might
>>>> make transparent handling of continuous lines even more tricky.
>>>>
>>>> I guess that local buffering, before calling printk(), will be
>>>> even more important then. Well, it might really force us to create
>>>> an API for it.
>>>
>>> Cool! Can you CC me on that discussion?
>>
>> Adding John Oggness into CC.
>>
>> John, please CC Brendan Higgins on the patchsets eventually switching
>> printk() into the lockless buffer. The test framework is going to
>> do its own buffering to keep the related messages together.
>>
>> The lockless ringbuffer might make handling of related (partial)
>> lines worse or better. It might justify KUnit's extra buffering
>> or it might allow to get rid of it.
>
> Thanks for CC'ing me on the printk ringbuffer thread. It looks like it
> actually probably won't affect my needs for KUnit logging. The biggest
> reason I need some sort of buffering system is to be able to compose
> messages piece meal into a single message that will be printed out to
> the user as a single message with no messages from other printk
> callers printed out in the middle of mine.

printk has this same requirement for its CONT messages. You can read
about how I propose to implement that here[0], using a separate prb
ringbuffer for buffered storage until all the pieces are available.

It is not my goal that multiple subsystems start making use of the prb
ringbuffer. However, its features can be attractive if you don't want to
worry about multiple writers/readers or context (including NMI). Before
writing "yet another ringbuffer" [1] it might be worth the effort to at
least see if one of the existing implementations can work (or be
extended to work) for you.

John Ogness

[0] https://lkml.kernel.org/r/87imt2bl0k.fsf@linutronix.de
[1] https://lwn.net/Articles/789603/

> The prb does look interesting; however, it appears that to get the
> semantics that I need, I would have to put my entire message in a
> single data block and would consequently need to know the size of my
> message a priori, which is problematic. Consequently, it seems as
> though I will probably need to compose my entire message using my own
> buffering system.
>
>>>> Note that stroring the messages into the printk log is basically
>>>> safe in any context. It uses temporary per-CPU buffers for
>>>> recursive messages and in NMI. The only problem is panic() when
>>>> some CPU gets stuck with the lock taken. This will get solved by
>>>> the lockless ringbuffer. Also the temporary buffers will not be
>>>> necessary any longer.
>>>
>>> Sure, I think Stephen's concern is all the supporting code that is
>>> involved. Not printk specifically. It just means a lot more of KUnit
>>> has to be IRQ safe.
>>
>> I see.
>>
>> BTW: I wonder if KUnit could reuse the existing seq_buf
>> implementation for buffering messages.
>>
>> I am sorry if it has already been proposed and rejected for some
>> reason. I might have missed it. Feel free to just point me to
>> same older mail.
>
> Yeah, we discussed it briefly here:
>
> https://lkml.org/lkml/2019/5/17/497
>
> Looks like I forgot to include my reasoning in the commit text, sorry
> about that.
>
>>>> Much bigger problems are with consoles. There are many of them. It
>>>> means a lot of code and more locks involved, including scheduler
>>>> locks. Note that console lock is a semaphore.
>>>
>>> That shouldn't affect us though, right? As long as we continue to
>>> use the printk interface?
>>
>> I guess that it should not affect KUnit.
>>
>> The only problem might be if the testing framework calls printk()
>> inside scheduler or console code. And only when the tested code
>> uses the same locks that will be used by the called printk().
>
> Yeah, well printk will not be our only problem in those instances.
>
>> To be honest I do not fully understand KUnit design. I am not
>> completely sure how the tested code is isolated from the running
>> system. Namely, I do not know if the tested code shares
>> the same locks with the system running the test.
>
> No worries, I don't expect printk to be the hang up in those cases. It
> sounds like KUnit has a long way to evolve before printk is going to
> be a limitation.
>
> Thanks!
Brendan Higgins Aug. 12, 2019, 8:41 p.m. UTC | #24
On Thu, Aug 1, 2019 at 2:43 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Thu, Aug 1, 2019 at 2:14 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-08-01 11:59:57)
> > > On Thu, Aug 1, 2019 at 11:55 AM Brendan Higgins
> > > <brendanhiggins@google.com> wrote:
> > > >
> > > > On Fri, Jul 26, 2019 at 1:31 AM Petr Mladek <pmladek@suse.com> wrote:
> > > >
> > > > > To be honest I do not fully understand KUnit design. I am not
> > > > > completely sure how the tested code is isolated from the running
> > > > > system. Namely, I do not know if the tested code shares
> > > > > the same locks with the system running the test.
> > > >
> > > > No worries, I don't expect printk to be the hang up in those cases. It
> > > > sounds like KUnit has a long way to evolve before printk is going to
> > > > be a limitation.
> > >
> > > So Stephen, what do you think?
> > >
> > > Do you want me to go forward with the new kunit_assert API wrapping
> > > the string_stream as I have it now? Would you prefer to punt this to a
> > > later patch? Or would you prefer something else?
> > >
> >
> > I like the struct based approach. If anything, it can be adjusted to
> > make the code throw some records into a spinlock later on and delay the
> > formatting of the assertion if need be.
>
> That's a fair point.
>
> > Can you resend with that
> > approach? I don't think I'll have any more comments after that.

I sent a new revision, v12, that incorporates the kunit_assert stuff.

Let me know what you think!
Brendan Higgins Aug. 12, 2019, 9:12 p.m. UTC | #25
On Fri, Aug 02, 2019 at 09:37:53AM +0200, John Ogness wrote:
> On 2019-08-01, Brendan Higgins <brendanhiggins@google.com> wrote:
> > On Fri, Jul 26, 2019 at 1:31 AM Petr Mladek <pmladek@suse.com> wrote:
> >> On Thu 2019-07-25 13:21:12, Brendan Higgins wrote:
> >>> On Wed, Jul 24, 2019 at 12:31 AM Petr Mladek <pmladek@suse.com> wrote:
> >>>> On Mon 2019-07-22 16:54:10, Stephen Boyd wrote:
> >>>>> Quoting Brendan Higgins (2019-07-22 15:30:49)
> >>>>>> On Mon, Jul 22, 2019 at 1:03 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >>>>>>> What's the calling context of the assertions and expectations? I
> >>>>>>> still don't like the fact that string stream needs to allocate
> >>>>>>> buffers and throw them into a list somewhere because the calling
> >>>>>>> context matters there.
> >>>>>>
> >>>>>> The calling context is the same as before, which is anywhere.
> >>>>>
> >>>>> Ok. That's concerning then.
> >>>>>
> >>>>>>> I'd prefer we just wrote directly to the console/log via printk
> >>>>>>> instead. That way things are simple because we use the existing
> >>>>>>> buffering path of printk, but maybe there's some benefit to the
> >>>>>>> string stream that I don't see? Right now it looks like it
> >>>>>>> builds a string and then dumps it to printk so I'm sort of lost
> >>>>>>> what the benefit is over just writing directly with printk.
> >>>>>>
> >>>>>> It's just buffering it so the whole string gets printed
> >>>>>> uninterrupted.  If we were to print out piecemeal to printk,
> >>>>>> couldn't we have another call to printk come in causing it to
> >>>>>> garble the KUnit message we are in the middle of printing?
> >>>>>
> >>>>> Yes, printing piecemeal by calling printk many times could lead to
> >>>>> interleaving of messages if something else comes in such as an
> >>>>> interrupt printing something. Printk has some support to hold
> >>>>> "records" but I'm not sure how that would work here because
> >>>>> KERN_CONT talks about only being used early on in boot code. I
> >>>>> haven't looked at printk in detail though so maybe I'm all wrong
> >>>>> and KERN_CONT just works?
> >>>>
> >>>> KERN_CONT does not guarantee that the message will get printed
> >>>> together. The pieces get interleaved with messages printed in
> >>>> parallel.
> >>>>
> >>>> Note that KERN_CONT was originally really meant to be used only
> >>>> during boot. It was later used more widely and ended in the best
> >>>> effort category.
> >>>>
> >>>> There were several attempts to make it more reliable. But it was
> >>>> always either too complicated or error prone or both.
> >>>>
> >>>> You need to use your own buffering if you rely want perfect output.
> >>>> The question is if it is really worth the complexity. Also note
> >>>> that any buffering reduces the chance that the messages will reach
> >>>> the console.
> >>>
> >>> Seems like that settles it then. Thanks!
> >>>
> >>>> BTW: There is a work in progress on a lockless printk ring buffer.
> >>>> It will make printk() more secure regarding deadlocks. But it might
> >>>> make transparent handling of continuous lines even more tricky.
> >>>>
> >>>> I guess that local buffering, before calling printk(), will be
> >>>> even more important then. Well, it might really force us to create
> >>>> an API for it.
> >>>
> >>> Cool! Can you CC me on that discussion?
> >>
> >> Adding John Oggness into CC.
> >>
> >> John, please CC Brendan Higgins on the patchsets eventually switching
> >> printk() into the lockless buffer. The test framework is going to
> >> do its own buffering to keep the related messages together.
> >>
> >> The lockless ringbuffer might make handling of related (partial)
> >> lines worse or better. It might justify KUnit's extra buffering
> >> or it might allow to get rid of it.
> >
> > Thanks for CC'ing me on the printk ringbuffer thread. It looks like it
> > actually probably won't affect my needs for KUnit logging. The biggest
> > reason I need some sort of buffering system is to be able to compose
> > messages piece meal into a single message that will be printed out to
> > the user as a single message with no messages from other printk
> > callers printed out in the middle of mine.
> 
> printk has this same requirement for its CONT messages. You can read
> about how I propose to implement that here[0], using a separate prb
> ringbuffer for buffered storage until all the pieces are available.
> 
> It is not my goal that multiple subsystems start making use of the prb
> ringbuffer. However, its features can be attractive if you don't want to
> worry about multiple writers/readers or context (including NMI). Before

That sounds like it might be useful down the road, but not to replace
the string_stream.

> writing "yet another ringbuffer" [1] it might be worth the effort to at
> least see if one of the existing implementations can work (or be
> extended to work) for you.

In regards to the conversation here about string_stream/kunit_stream, I
think Petr already answered that question. As I said previously:
> [I]t appears that to get the
> semantics that I need, I would have to put my entire message in a
> single data block and would consequently need to know the size of my
> message a priori, which is problematic. Consequently, it seems as
> though I will probably need to compose my entire message using my own
> buffering system.

I could potentially use my own set of prbs for that buffering; however,
I think this use case is probably closer to seq_buf than your prb.
Really, I just want some kind of string builder, not a message queue.

The place where I think your prb is relevant here is once I have
composed the message and I just want to print it, having a way to print
it without worrying about context is nice, but I think that is a
separate discussion from the main topic here which was just figuring out
the right way to compose that message in the first place.

Cheers

> John Ogness
> 
> [0] https://lkml.kernel.org/r/87imt2bl0k.fsf@linutronix.de
> [1] https://lwn.net/Articles/789603/
> 
> > The prb does look interesting; however, it appears that to get the
> > semantics that I need, I would have to put my entire message in a
> > single data block and would consequently need to know the size of my
> > message a priori, which is problematic. Consequently, it seems as
> > though I will probably need to compose my entire message using my own
> > buffering system.
> >
> >>>> Note that stroring the messages into the printk log is basically
> >>>> safe in any context. It uses temporary per-CPU buffers for
> >>>> recursive messages and in NMI. The only problem is panic() when
> >>>> some CPU gets stuck with the lock taken. This will get solved by
> >>>> the lockless ringbuffer. Also the temporary buffers will not be
> >>>> necessary any longer.
> >>>
> >>> Sure, I think Stephen's concern is all the supporting code that is
> >>> involved. Not printk specifically. It just means a lot more of KUnit
> >>> has to be IRQ safe.
> >>
> >> I see.
> >>
> >> BTW: I wonder if KUnit could reuse the existing seq_buf
> >> implementation for buffering messages.
> >>
> >> I am sorry if it has already been proposed and rejected for some
> >> reason. I might have missed it. Feel free to just point me to
> >> same older mail.
> >
> > Yeah, we discussed it briefly here:
> >
> > https://lkml.org/lkml/2019/5/17/497
> >
> > Looks like I forgot to include my reasoning in the commit text, sorry
> > about that.
> >
> >>>> Much bigger problems are with consoles. There are many of them. It
> >>>> means a lot of code and more locks involved, including scheduler
> >>>> locks. Note that console lock is a semaphore.
> >>>
> >>> That shouldn't affect us though, right? As long as we continue to
> >>> use the printk interface?
> >>
> >> I guess that it should not affect KUnit.
> >>
> >> The only problem might be if the testing framework calls printk()
> >> inside scheduler or console code. And only when the tested code
> >> uses the same locks that will be used by the called printk().
> >
> > Yeah, well printk will not be our only problem in those instances.
> >
> >> To be honest I do not fully understand KUnit design. I am not
> >> completely sure how the tested code is isolated from the running
> >> system. Namely, I do not know if the tested code shares
> >> the same locks with the system running the test.
> >
> > No worries, I don't expect printk to be the hang up in those cases. It
> > sounds like KUnit has a long way to evolve before printk is going to
> > be a limitation.
> >
> > Thanks!

Patch
diff mbox series

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
@@ -0,0 +1,81 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * C++ stream style string formatter and printer used in KUnit for outputting
+ * KUnit messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#ifndef _KUNIT_KUNIT_STREAM_H
+#define _KUNIT_KUNIT_STREAM_H
+
+#include <linux/types.h>
+#include <kunit/string-stream.h>
+
+struct kunit;
+
+/**
+ * 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;
+	struct string_stream *internal_stream;
+};
+
+/**
+ * alloc_kunit_stream() - constructs a new &struct kunit_stream.
+ * @test: The test context object.
+ * @level: The log level at which to print out the message.
+ *
+ * Constructs a new test managed &struct kunit_stream.
+ */
+struct kunit_stream *alloc_kunit_stream(struct kunit *test, const char *level);
+
+/**
+ * kunit_stream_add(): adds the formatted input to the internal buffer.
+ * @kstream: the stream being operated on.
+ * @fmt: printf style format string to append to stream.
+ *
+ * Appends the formatted string, @fmt, to the internal buffer.
+ */
+void __printf(2, 3) kunit_stream_add(struct kunit_stream *kstream,
+				     const char *fmt, ...);
+
+/**
+ * kunit_stream_append(): appends the contents of @other to @kstream.
+ * @kstream: the stream to which @other is appended.
+ * @other: the stream whose contents are appended to @kstream.
+ *
+ * Appends the contents of @other to @kstream.
+ */
+void kunit_stream_append(struct kunit_stream *kstream,
+			 struct kunit_stream *other);
+
+/**
+ * kunit_stream_commit(): prints out the internal buffer to the user.
+ * @kstream: the stream being operated on.
+ *
+ * Outputs the contents of the internal buffer as a kunit_printk formatted
+ * output. KUNIT_STREAM ONLY OUTPUTS ITS BUFFER TO THE USER IF COMMIT IS
+ * CALLED!!! The reason for this is that it allows us to construct a message
+ * before we know whether we want to print it out; this can be extremely handy
+ * if there is information you might need for a failure message that is easiest
+ * to collect in the steps leading up to the actual check.
+ */
+void kunit_stream_commit(struct kunit_stream *kstream);
+
+/**
+ * kunit_stream_clear(): clears the internal buffer.
+ * @kstream: the stream being operated on.
+ *
+ * Clears the contents of the internal buffer.
+ */
+void kunit_stream_clear(struct kunit_stream *kstream);
+
+#endif /* _KUNIT_KUNIT_STREAM_H */
diff --git a/include/kunit/test.h b/include/kunit/test.h
index bdf41d31c343c..bc7dbdcf8abab 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -11,6 +11,7 @@ 
 
 #include <linux/types.h>
 #include <linux/slab.h>
+#include <kunit/kunit-stream.h>
 
 struct kunit_resource;
 
@@ -184,6 +185,8 @@  struct kunit {
 
 void kunit_init_test(struct kunit *test, const char *name);
 
+void kunit_fail(struct kunit *test, struct kunit_stream *stream);
+
 int kunit_run_tests(struct kunit_suite *suite);
 
 /**
diff --git a/kunit/Makefile b/kunit/Makefile
index 275b565a0e81f..6ddc622ee6b1c 100644
--- a/kunit/Makefile
+++ b/kunit/Makefile
@@ -1,2 +1,3 @@ 
 obj-$(CONFIG_KUNIT) +=			test.o \
-					string-stream.o
+					string-stream.o \
+					kunit-stream.o
diff --git a/kunit/kunit-stream.c b/kunit/kunit-stream.c
new file mode 100644
index 0000000000000..8bea1f22eafb5
--- /dev/null
+++ b/kunit/kunit-stream.c
@@ -0,0 +1,123 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * C++ stream style string formatter and printer used in KUnit for outputting
+ * KUnit messages.
+ *
+ * Copyright (C) 2019, Google LLC.
+ * Author: Brendan Higgins <brendanhiggins@google.com>
+ */
+
+#include <kunit/test.h>
+#include <kunit/kunit-stream.h>
+#include <kunit/string-stream.h>
+
+void kunit_stream_add(struct kunit_stream *kstream, const char *fmt, ...)
+{
+	va_list args;
+	struct string_stream *stream = kstream->internal_stream;
+
+	va_start(args, fmt);
+
+	if (string_stream_vadd(stream, fmt, args) < 0)
+		kunit_err(kstream->test,
+			  "Failed to allocate fragment: %s\n",
+			  fmt);
+
+	va_end(args);
+}
+
+void kunit_stream_append(struct kunit_stream *kstream,
+				struct kunit_stream *other)
+{
+	struct string_stream *other_stream = other->internal_stream;
+	const char *other_content;
+
+	other_content = string_stream_get_string(other_stream);
+
+	if (!other_content) {
+		kunit_err(kstream->test,
+			  "Failed to get string from second argument for appending\n");
+		return;
+	}
+
+	kunit_stream_add(kstream, other_content);
+}
+
+void kunit_stream_clear(struct kunit_stream *kstream)
+{
+	string_stream_clear(kstream->internal_stream);
+}
+
+void kunit_stream_commit(struct kunit_stream *kstream)
+{
+	struct string_stream *stream = kstream->internal_stream;
+	struct string_stream_fragment *fragment;
+	struct kunit *test = kstream->test;
+	char *buf;
+
+	buf = string_stream_get_string(stream);
+	if (!buf) {
+		kunit_err(test,
+			  "Could not allocate buffer, dumping stream:\n");
+		list_for_each_entry(fragment, &stream->fragments, node) {
+			kunit_err(test, fragment->fragment);
+		}
+		kunit_err(test, "\n");
+		goto cleanup;
+	}
+
+	kunit_printk(kstream->level, test, buf);
+	kfree(buf);
+
+cleanup:
+	kunit_stream_clear(kstream);
+}
+
+static int kunit_stream_init(struct kunit_resource *res, void *context)
+{
+	struct kunit *test = context;
+	struct kunit_stream *stream;
+
+	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+	if (!stream)
+		return -ENOMEM;
+
+	res->allocation = stream;
+	stream->test = test;
+	stream->internal_stream = alloc_string_stream(test);
+
+	if (!stream->internal_stream)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void kunit_stream_free(struct kunit_resource *res)
+{
+	struct kunit_stream *stream = res->allocation;
+
+	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);
+	}
+}
+
+struct kunit_stream *alloc_kunit_stream(struct kunit *test, const char *level)
+{
+	struct kunit_stream *kstream;
+	struct kunit_resource *res;
+
+	res = kunit_alloc_resource(test,
+				   kunit_stream_init,
+				   kunit_stream_free,
+				   test);
+
+	if (!res)
+		return NULL;
+
+	kstream = res->allocation;
+	kstream->level = level;
+
+	return kstream;
+}
diff --git a/kunit/test.c b/kunit/test.c
index f165c9d8e10b0..29edf34a89a37 100644
--- a/kunit/test.c
+++ b/kunit/test.c
@@ -120,6 +120,12 @@  static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
 			      test_case->name);
 }
 
+void kunit_fail(struct kunit *test, struct kunit_stream *stream)
+{
+	kunit_set_failure(test);
+	kunit_stream_commit(stream);
+}
+
 void kunit_init_test(struct kunit *test, const char *name)
 {
 	mutex_init(&test->lock);