| 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
| ||
|
Date: Thu, 18 Jul 2019 17:08:34 -0700
From: Brendan Higgins <brendanhiggins@...gle.com>
To: Stephen Boyd <sboyd@...nel.org>
Cc: Frank Rowand <frowand.list@...il.com>,
Greg KH <gregkh@...uxfoundation.org>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Kees Cook <keescook@...gle.com>,
Kieran Bingham <kieran.bingham@...asonboard.com>,
Luis Chamberlain <mcgrof@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Rob Herring <robh@...nel.org>, shuah <shuah@...nel.org>,
Theodore Ts'o <tytso@....edu>,
Masahiro Yamada <yamada.masahiro@...ionext.com>,
devicetree <devicetree@...r.kernel.org>,
dri-devel <dri-devel@...ts.freedesktop.org>,
kunit-dev@...glegroups.com,
"open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
linux-fsdevel@...r.kernel.org,
linux-kbuild <linux-kbuild@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@...r.kernel.org>,
linux-nvdimm <linux-nvdimm@...ts.01.org>,
linux-um@...ts.infradead.org,
Sasha Levin <Alexander.Levin@...rosoft.com>,
"Bird, Timothy" <Tim.Bird@...y.com>,
Amir Goldstein <amir73il@...il.com>,
Dan Carpenter <dan.carpenter@...cle.com>,
Daniel Vetter <daniel@...ll.ch>, Jeff Dike <jdike@...toit.com>,
Joel Stanley <joel@....id.au>,
Julia Lawall <julia.lawall@...6.fr>,
Kevin Hilman <khilman@...libre.com>,
Knut Omang <knut.omang@...cle.com>,
Logan Gunthorpe <logang@...tatee.com>,
Michael Ellerman <mpe@...erman.id.au>,
Petr Mladek <pmladek@...e.com>,
Randy Dunlap <rdunlap@...radead.org>,
Richard Weinberger <richard@....at>,
David Rientjes <rientjes@...gle.com>,
Steven Rostedt <rostedt@...dmis.org>, wfg@...ux.intel.com
Subject: Re: [PATCH v9 04/18] kunit: test: add kunit_stream a std::stream
like logger
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@...nel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-07-16 11:52:01)
> > > On Tue, Jul 16, 2019 at 10:50 AM Stephen Boyd <sboyd@...nel.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@...gle.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@...gle.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@...gle.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;
--
2.22.0.657.g960e92d24f-goog
Powered by blists - more mailing lists