| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20190715221554.8417320665@mail.kernel.org>
Date: Mon, 15 Jul 2019 15:15:53 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Brendan Higgins <brendanhiggins@...gle.com>,
frowand.list@...il.com, gregkh@...uxfoundation.org,
jpoimboe@...hat.com, keescook@...gle.com,
kieran.bingham@...asonboard.com, mcgrof@...nel.org,
peterz@...radead.org, robh@...nel.org, shuah@...nel.org,
tytso@....edu, yamada.masahiro@...ionext.com
Cc: devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org,
kunit-dev@...glegroups.com, linux-doc@...r.kernel.org,
linux-fsdevel@...r.kernel.org, linux-kbuild@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
linux-nvdimm@...ts.01.org, linux-um@...ts.infradead.org,
Alexander.Levin@...rosoft.com, Tim.Bird@...y.com,
amir73il@...il.com, dan.carpenter@...cle.com, daniel@...ll.ch,
jdike@...toit.com, joel@....id.au, julia.lawall@...6.fr,
khilman@...libre.com, knut.omang@...cle.com, logang@...tatee.com,
mpe@...erman.id.au, pmladek@...e.com, rdunlap@...radead.org,
richard@....at, rientjes@...gle.com, rostedt@...dmis.org,
wfg@...ux.intel.com, Brendan Higgins <brendanhiggins@...gle.com>
Subject: Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream like logger
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@...gle.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@...gle.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);
Powered by blists - more mailing lists