| Message ID | 20190812182421.141150-4-brendanhiggins@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | kunit: introduce KUnit, the Linux kernel unit testing framework | expand |
Quoting Brendan Higgins (2019-08-12 11:24:06) > +void string_stream_clear(struct string_stream *stream) > +{ > + struct string_stream_fragment *frag_container, *frag_container_safe; > + > + spin_lock(&stream->lock); > + list_for_each_entry_safe(frag_container, > + frag_container_safe, > + &stream->fragments, > + node) { > + list_del(&frag_container->node); Shouldn't we free the allocation here? Otherwise, if some test is going to add, add, clear, add, it's going to leak until the test is over? > + } > + stream->length = 0; > + spin_unlock(&stream->lock); > +} > +
On Mon, Aug 12, 2019 at 03:55:19PM -0700, Stephen Boyd wrote: > Quoting Brendan Higgins (2019-08-12 11:24:06) > > +void string_stream_clear(struct string_stream *stream) > > +{ > > + struct string_stream_fragment *frag_container, *frag_container_safe; > > + > > + spin_lock(&stream->lock); > > + list_for_each_entry_safe(frag_container, > > + frag_container_safe, > > + &stream->fragments, > > + node) { > > + list_del(&frag_container->node); > > Shouldn't we free the allocation here? Otherwise, if some test is going > to add, add, clear, add, it's going to leak until the test is over? So basically this means I should add a kunit_kfree and kunit_resource_destroy (respective equivalents to devm_kfree, and devres_destroy) and use kunit_kfree here? > > + } > > + stream->length = 0; > > + spin_unlock(&stream->lock); > > +} > > +
Quoting Brendan Higgins (2019-08-12 16:33:36) > On Mon, Aug 12, 2019 at 03:55:19PM -0700, Stephen Boyd wrote: > > Quoting Brendan Higgins (2019-08-12 11:24:06) > > > +void string_stream_clear(struct string_stream *stream) > > > +{ > > > + struct string_stream_fragment *frag_container, *frag_container_safe; > > > + > > > + spin_lock(&stream->lock); > > > + list_for_each_entry_safe(frag_container, > > > + frag_container_safe, > > > + &stream->fragments, > > > + node) { > > > + list_del(&frag_container->node); > > > > Shouldn't we free the allocation here? Otherwise, if some test is going > > to add, add, clear, add, it's going to leak until the test is over? > > So basically this means I should add a kunit_kfree and > kunit_resource_destroy (respective equivalents to devm_kfree, and > devres_destroy) and use kunit_kfree here? > Yes, or drop the API entirely? Does anything need this functionality?
On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Brendan Higgins (2019-08-12 16:33:36) > > On Mon, Aug 12, 2019 at 03:55:19PM -0700, Stephen Boyd wrote: > > > Quoting Brendan Higgins (2019-08-12 11:24:06) > > > > +void string_stream_clear(struct string_stream *stream) > > > > +{ > > > > + struct string_stream_fragment *frag_container, *frag_container_safe; > > > > + > > > > + spin_lock(&stream->lock); > > > > + list_for_each_entry_safe(frag_container, > > > > + frag_container_safe, > > > > + &stream->fragments, > > > > + node) { > > > > + list_del(&frag_container->node); > > > > > > Shouldn't we free the allocation here? Otherwise, if some test is going > > > to add, add, clear, add, it's going to leak until the test is over? > > > > So basically this means I should add a kunit_kfree and > > kunit_resource_destroy (respective equivalents to devm_kfree, and > > devres_destroy) and use kunit_kfree here? > > > > Yes, or drop the API entirely? Does anything need this functionality? Drop the kunit_resource API? I would strongly prefer not to. string_stream uses it; the expectation stuff uses it via string stream; some of the tests in this patchset allocate memory as part of the test setup that uses it. The intention is that we would provide a kunit_res_* version of many (hopefully eventually most) common resources required by tests and it would be used in the same way that the devm_* stuff is. Nevertheless, I am fine adding the kunit_resource_destroy, etc. I just wanted to make sure I understood what you were asking.
Quoting Brendan Higgins (2019-08-12 17:41:05) > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and > > > devres_destroy) and use kunit_kfree here? > > > > > > > Yes, or drop the API entirely? Does anything need this functionality? > > Drop the kunit_resource API? I would strongly prefer not to. No. I mean this API, string_stream_clear(). Does anything use it?
On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Brendan Higgins (2019-08-12 17:41:05) > > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and > > > > devres_destroy) and use kunit_kfree here? > > > > > > > > > > Yes, or drop the API entirely? Does anything need this functionality? > > > > Drop the kunit_resource API? I would strongly prefer not to. > > No. I mean this API, string_stream_clear(). Does anything use it? Oh, right. No. However, now that I added the kunit_resource_destroy, I thought it might be good to free the string_stream after I use it in each call to kunit_assert->format(...) in which case I will be using this logic. So I think the right thing to do is to expose string_stream_destroy so kunit_do_assert can clean up when it's done, and then demote string_stream_clear to static. Sound good?
Quoting Brendan Higgins (2019-08-12 22:02:59) > On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Brendan Higgins (2019-08-12 17:41:05) > > > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and > > > > > devres_destroy) and use kunit_kfree here? > > > > > > > > > > > > > Yes, or drop the API entirely? Does anything need this functionality? > > > > > > Drop the kunit_resource API? I would strongly prefer not to. > > > > No. I mean this API, string_stream_clear(). Does anything use it? > > Oh, right. No. > > However, now that I added the kunit_resource_destroy, I thought it > might be good to free the string_stream after I use it in each call to > kunit_assert->format(...) in which case I will be using this logic. > > So I think the right thing to do is to expose string_stream_destroy so > kunit_do_assert can clean up when it's done, and then demote > string_stream_clear to static. Sound good? Ok, sure. I don't really see how clearing it explicitly when the assertion prints vs. never allocating it to begin with is really any different. Maybe I've missed something though.
On Mon, Aug 12, 2019 at 10:30 PM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Brendan Higgins (2019-08-12 22:02:59) > > On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > Quoting Brendan Higgins (2019-08-12 17:41:05) > > > > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > > > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and > > > > > > devres_destroy) and use kunit_kfree here? > > > > > > > > > > > > > > > > Yes, or drop the API entirely? Does anything need this functionality? > > > > > > > > Drop the kunit_resource API? I would strongly prefer not to. > > > > > > No. I mean this API, string_stream_clear(). Does anything use it? > > > > Oh, right. No. > > > > However, now that I added the kunit_resource_destroy, I thought it > > might be good to free the string_stream after I use it in each call to > > kunit_assert->format(...) in which case I will be using this logic. > > > > So I think the right thing to do is to expose string_stream_destroy so > > kunit_do_assert can clean up when it's done, and then demote > > string_stream_clear to static. Sound good? > > Ok, sure. I don't really see how clearing it explicitly when the > assertion prints vs. never allocating it to begin with is really any > different. Maybe I've missed something though. It's for the case that we *do* print something out. Once we are doing printing, we don't want the fragments anymore.
On Tue, Aug 13, 2019 at 2:04 AM Brendan Higgins <brendanhiggins@google.com> wrote: > > On Mon, Aug 12, 2019 at 10:30 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Brendan Higgins (2019-08-12 22:02:59) > > > On Mon, Aug 12, 2019 at 9:56 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > > > Quoting Brendan Higgins (2019-08-12 17:41:05) > > > > > On Mon, Aug 12, 2019 at 4:59 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > > > > > > > > kunit_resource_destroy (respective equivalents to devm_kfree, and > > > > > > > devres_destroy) and use kunit_kfree here? > > > > > > > > > > > > > > > > > > > Yes, or drop the API entirely? Does anything need this functionality? > > > > > > > > > > Drop the kunit_resource API? I would strongly prefer not to. > > > > > > > > No. I mean this API, string_stream_clear(). Does anything use it? > > > > > > Oh, right. No. > > > > > > However, now that I added the kunit_resource_destroy, I thought it > > > might be good to free the string_stream after I use it in each call to > > > kunit_assert->format(...) in which case I will be using this logic. > > > > > > So I think the right thing to do is to expose string_stream_destroy so > > > kunit_do_assert can clean up when it's done, and then demote > > > string_stream_clear to static. Sound good? > > > > Ok, sure. I don't really see how clearing it explicitly when the > > assertion prints vs. never allocating it to begin with is really any > > different. Maybe I've missed something though. > > It's for the case that we *do* print something out. Once we are doing > printing, we don't want the fragments anymore. Oops, sorry fat fingered: s/doing/done
Quoting Brendan Higgins (2019-08-13 02:12:54) > On Tue, Aug 13, 2019 at 2:04 AM Brendan Higgins > <brendanhiggins@google.com> wrote: > > > > On Mon, Aug 12, 2019 at 10:30 PM Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > Quoting Brendan Higgins (2019-08-12 22:02:59) > > > > However, now that I added the kunit_resource_destroy, I thought it > > > > might be good to free the string_stream after I use it in each call to > > > > kunit_assert->format(...) in which case I will be using this logic. > > > > > > > > So I think the right thing to do is to expose string_stream_destroy so > > > > kunit_do_assert can clean up when it's done, and then demote > > > > string_stream_clear to static. Sound good? > > > > > > Ok, sure. I don't really see how clearing it explicitly when the > > > assertion prints vs. never allocating it to begin with is really any > > > different. Maybe I've missed something though. > > > > It's for the case that we *do* print something out. Once we are doing > > printing, we don't want the fragments anymore. > > Oops, sorry fat fingered: s/doing/done Yes, but when we print something out we've run into some sort of problem and then the test is over. So freeing the memory when it fails vs. when the test is over seems like a minor difference. Or is it also used to print other informational messages while the test is running? I'm not particularly worried here, just trying to see if less code is possible.
diff --git a/include/kunit/string-stream.h b/include/kunit/string-stream.h new file mode 100644 index 0000000000000..4fa107e38deb5 --- /dev/null +++ b/include/kunit/string-stream.h @@ -0,0 +1,49 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * C++ stream style string builder used in KUnit for building messages. + * + * Copyright (C) 2019, Google LLC. + * Author: Brendan Higgins <brendanhiggins@google.com> + */ + +#ifndef _KUNIT_STRING_STREAM_H +#define _KUNIT_STRING_STREAM_H + +#include <linux/types.h> +#include <linux/spinlock.h> +#include <stdarg.h> + +struct string_stream_fragment { + struct list_head node; + char *fragment; +}; + +struct string_stream { + size_t length; + struct list_head fragments; + /* length and fragments are protected by this lock */ + spinlock_t lock; + struct kunit *test; + gfp_t gfp; +}; + +struct kunit; + +struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp); + +int string_stream_add(struct string_stream *stream, const char *fmt, ...); + +int string_stream_vadd(struct string_stream *stream, + const char *fmt, + va_list args); + +char *string_stream_get_string(struct string_stream *stream); + +int string_stream_append(struct string_stream *stream, + struct string_stream *other); + +void string_stream_clear(struct string_stream *stream); + +bool string_stream_is_empty(struct string_stream *stream); + +#endif /* _KUNIT_STRING_STREAM_H */ diff --git a/kunit/Makefile b/kunit/Makefile index 5efdc4dea2c08..275b565a0e81f 100644 --- a/kunit/Makefile +++ b/kunit/Makefile @@ -1 +1,2 @@ -obj-$(CONFIG_KUNIT) += test.o +obj-$(CONFIG_KUNIT) += test.o \ + string-stream.o diff --git a/kunit/string-stream.c b/kunit/string-stream.c new file mode 100644 index 0000000000000..bcd56d6755544 --- /dev/null +++ b/kunit/string-stream.c @@ -0,0 +1,152 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * C++ stream style string builder used in KUnit for building messages. + * + * Copyright (C) 2019, Google LLC. + * Author: Brendan Higgins <brendanhiggins@google.com> + */ + +#include <linux/list.h> +#include <linux/slab.h> +#include <kunit/string-stream.h> +#include <kunit/test.h> + +int string_stream_vadd(struct string_stream *stream, + const char *fmt, + va_list args) +{ + struct string_stream_fragment *frag_container; + int len; + va_list args_for_counting; + + /* Make a copy because `vsnprintf` could change it */ + va_copy(args_for_counting, args); + + /* Need space for null byte. */ + len = vsnprintf(NULL, 0, fmt, args_for_counting) + 1; + + va_end(args_for_counting); + + frag_container = kunit_kmalloc(stream->test, sizeof(*frag_container), + stream->gfp); + if (!frag_container) + return -ENOMEM; + + frag_container->fragment = kunit_kmalloc(stream->test, len, + stream->gfp); + if (!frag_container->fragment) + return -ENOMEM; + + len = vsnprintf(frag_container->fragment, len, fmt, args); + spin_lock(&stream->lock); + stream->length += len; + list_add_tail(&frag_container->node, &stream->fragments); + spin_unlock(&stream->lock); + + return 0; +} + +int string_stream_add(struct string_stream *stream, const char *fmt, ...) +{ + va_list args; + int result; + + va_start(args, fmt); + result = string_stream_vadd(stream, fmt, args); + va_end(args); + + return result; +} + +void string_stream_clear(struct string_stream *stream) +{ + struct string_stream_fragment *frag_container, *frag_container_safe; + + spin_lock(&stream->lock); + list_for_each_entry_safe(frag_container, + frag_container_safe, + &stream->fragments, + node) { + list_del(&frag_container->node); + } + stream->length = 0; + spin_unlock(&stream->lock); +} + +char *string_stream_get_string(struct string_stream *stream) +{ + struct string_stream_fragment *frag_container; + size_t buf_len = stream->length + 1; /* +1 for null byte. */ + char *buf; + + buf = kunit_kzalloc(stream->test, buf_len, stream->gfp); + if (!buf) + return NULL; + + spin_lock(&stream->lock); + list_for_each_entry(frag_container, &stream->fragments, node) + strlcat(buf, frag_container->fragment, buf_len); + spin_unlock(&stream->lock); + + return buf; +} + +int string_stream_append(struct string_stream *stream, + struct string_stream *other) +{ + const char *other_content; + + other_content = string_stream_get_string(other); + + if (!other_content) + return -ENOMEM; + + return string_stream_add(stream, other_content); +} + +bool string_stream_is_empty(struct string_stream *stream) +{ + return list_empty(&stream->fragments); +} + +struct string_stream_alloc_context { + struct kunit *test; + gfp_t gfp; +}; + +static int string_stream_init(struct kunit_resource *res, void *context) +{ + struct string_stream *stream; + struct string_stream_alloc_context *ctx = context; + + stream = kunit_kzalloc(ctx->test, sizeof(*stream), ctx->gfp); + if (!stream) + return -ENOMEM; + + res->allocation = stream; + stream->gfp = ctx->gfp; + stream->test = ctx->test; + INIT_LIST_HEAD(&stream->fragments); + spin_lock_init(&stream->lock); + + return 0; +} + +static void string_stream_free(struct kunit_resource *res) +{ + /* Nothing to do since everything is already a KUnit managed resource */ +} + +struct string_stream *alloc_string_stream(struct kunit *test, gfp_t gfp) +{ + struct string_stream_alloc_context context = { + .test = test, + .gfp = gfp + }; + + return kunit_alloc_resource(test, + string_stream_init, + string_stream_free, + gfp, + &context); +}