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

[Brendan Higgins]

On Tue, Jul 16, 2019 at 12:57 AM Brendan Higgins
<brendanhiggins@xxxxxxxxxx> wrote:
>
> On Mon, Jul 15, 2019 at 3:15 PM Stephen Boyd <sboyd@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > + */
> > > +
> > > +#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@xxxxxxxxxx>
> > > + */
> > > +
> > > +#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!