| Message ID | 20181128193636.254378-4-brendanhiggins@google.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | kunit: introduce KUnit, the Linux kernel unit testing framework | expand |
On Wed, Nov 28, 2018 at 11:36:20AM -0800, Brendan Higgins wrote: > A number of test features need to do pretty complicated string printing > where it may not be possible to rely on a single preallocated string > with parameters. > > So provide a library for constructing the string as you go similar to > C++'s std::string. Hrm, what's the potential for such thing actually being eventually generically useful for printk folks, I wonder? Petr? Luis > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com> > --- > include/kunit/string-stream.h | 44 ++++++++++ > kunit/Makefile | 3 +- > kunit/string-stream.c | 149 ++++++++++++++++++++++++++++++++++ > 3 files changed, 195 insertions(+), 1 deletion(-) > create mode 100644 include/kunit/string-stream.h > create mode 100644 kunit/string-stream.c > > diff --git a/include/kunit/string-stream.h b/include/kunit/string-stream.h > new file mode 100644 > index 0000000000000..933ed5740cf07 > --- /dev/null > +++ b/include/kunit/string-stream.h > @@ -0,0 +1,44 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * C++ stream style string builder used in KUnit for building messages. > + * > + * Copyright (C) 2018, 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 <linux/kref.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 kref refcount; > + int (*add)(struct string_stream *this, const char *fmt, ...); > + int (*vadd)(struct string_stream *this, const char *fmt, va_list args); > + char *(*get_string)(struct string_stream *this); > + void (*clear)(struct string_stream *this); > + bool (*is_empty)(struct string_stream *this); > +}; > + > +struct string_stream *new_string_stream(void); > + > +void destroy_string_stream(struct string_stream *stream); > + > +void string_stream_get(struct string_stream *stream); > + > +int string_stream_put(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..1e7efa630cc35 > --- /dev/null > +++ b/kunit/string-stream.c > @@ -0,0 +1,149 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * C++ stream style string builder used in KUnit for building messages. > + * > + * Copyright (C) 2018, Google LLC. > + * Author: Brendan Higgins <brendanhiggins@google.com> > + */ > + > +#include <linux/list.h> > +#include <linux/slab.h> > +#include <kunit/string-stream.h> > + > +static int string_stream_vadd(struct string_stream *this, > + const char *fmt, > + va_list args) > +{ > + struct string_stream_fragment *fragment; > + int len; > + va_list args_for_counting; > + unsigned long flags; > + > + /* 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); > + > + fragment = kmalloc(sizeof(*fragment), GFP_KERNEL); > + if (!fragment) > + return -ENOMEM; > + > + fragment->fragment = kmalloc(len, GFP_KERNEL); > + if (!fragment->fragment) { > + kfree(fragment); > + return -ENOMEM; > + } > + > + len = vsnprintf(fragment->fragment, len, fmt, args); > + spin_lock_irqsave(&this->lock, flags); > + this->length += len; > + list_add_tail(&fragment->node, &this->fragments); > + spin_unlock_irqrestore(&this->lock, flags); > + return 0; > +} > + > +static int string_stream_add(struct string_stream *this, const char *fmt, ...) > +{ > + va_list args; > + int result; > + > + va_start(args, fmt); > + result = string_stream_vadd(this, fmt, args); > + va_end(args); > + return result; > +} > + > +static void string_stream_clear(struct string_stream *this) > +{ > + struct string_stream_fragment *fragment, *fragment_safe; > + unsigned long flags; > + > + spin_lock_irqsave(&this->lock, flags); > + list_for_each_entry_safe(fragment, > + fragment_safe, > + &this->fragments, > + node) { > + list_del(&fragment->node); > + kfree(fragment->fragment); > + kfree(fragment); > + } > + this->length = 0; > + spin_unlock_irqrestore(&this->lock, flags); > +} > + > +static char *string_stream_get_string(struct string_stream *this) > +{ > + struct string_stream_fragment *fragment; > + size_t buf_len = this->length + 1; /* +1 for null byte. */ > + char *buf; > + unsigned long flags; > + > + buf = kzalloc(buf_len, GFP_KERNEL); > + if (!buf) > + return NULL; > + > + spin_lock_irqsave(&this->lock, flags); > + list_for_each_entry(fragment, &this->fragments, node) > + strlcat(buf, fragment->fragment, buf_len); > + spin_unlock_irqrestore(&this->lock, flags); > + > + return buf; > +} > + > +static bool string_stream_is_empty(struct string_stream *this) > +{ > + bool is_empty; > + unsigned long flags; > + > + spin_lock_irqsave(&this->lock, flags); > + is_empty = list_empty(&this->fragments); > + spin_unlock_irqrestore(&this->lock, flags); > + > + return is_empty; > +} > + > +void destroy_string_stream(struct string_stream *stream) > +{ > + stream->clear(stream); > + kfree(stream); > +} > + > +static void string_stream_destroy(struct kref *kref) > +{ > + struct string_stream *stream = container_of(kref, > + struct string_stream, > + refcount); > + destroy_string_stream(stream); > +} > + > +struct string_stream *new_string_stream(void) > +{ > + struct string_stream *stream = kzalloc(sizeof(*stream), GFP_KERNEL); > + > + if (!stream) > + return NULL; > + > + INIT_LIST_HEAD(&stream->fragments); > + spin_lock_init(&stream->lock); > + kref_init(&stream->refcount); > + stream->add = string_stream_add; > + stream->vadd = string_stream_vadd; > + stream->get_string = string_stream_get_string; > + stream->clear = string_stream_clear; > + stream->is_empty = string_stream_is_empty; > + return stream; > +} > + > +void string_stream_get(struct string_stream *stream) > +{ > + kref_get(&stream->refcount); > +} > + > +int string_stream_put(struct string_stream *stream) > +{ > + return kref_put(&stream->refcount, &string_stream_destroy); > +} > + > -- > 2.20.0.rc0.387.gc7a69e6b6c-goog >
On Thu, Nov 29, 2018 at 7:29 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Wed, Nov 28, 2018 at 11:36:20AM -0800, Brendan Higgins wrote: > > A number of test features need to do pretty complicated string printing > > where it may not be possible to rely on a single preallocated string > > with parameters. > > > > So provide a library for constructing the string as you go similar to > > C++'s std::string. > > Hrm, what's the potential for such thing actually being eventually > generically useful for printk folks, I wonder? Petr? Are you saying you think this is applicable for other things? Or are you saying that you are afraid that somebody might try to use this elsewhere? In the former case, this doesn't belong here. In the latter case, it explicitly depends on KUnit, so it is only available when running tests. Cheers
On Fri, Nov 30, 2018 at 06:14:17PM -0800, Brendan Higgins wrote: > On Thu, Nov 29, 2018 at 7:29 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > On Wed, Nov 28, 2018 at 11:36:20AM -0800, Brendan Higgins wrote: > > > A number of test features need to do pretty complicated string printing > > > where it may not be possible to rely on a single preallocated string > > > with parameters. > > > > > > So provide a library for constructing the string as you go similar to > > > C++'s std::string. > > > > Hrm, what's the potential for such thing actually being eventually > > generically useful for printk folks, I wonder? Petr? > > Are you saying you think this is applicable for other things? Yes. > This doesn't belong here. Luis
On Thu 2018-11-29 19:29:24, Luis Chamberlain wrote: > On Wed, Nov 28, 2018 at 11:36:20AM -0800, Brendan Higgins wrote: > > A number of test features need to do pretty complicated string printing > > where it may not be possible to rely on a single preallocated string > > with parameters. > > > > So provide a library for constructing the string as you go similar to > > C++'s std::string. > > Hrm, what's the potential for such thing actually being eventually > generically useful for printk folks, I wonder? Petr? printk() is a bit tricky: + It should work in any context. Any additional lock adds risk of a deadlock. Especially the NMI and scheduler contexts are problematic. There are problems with any other code that might be called from console drivers and calls printk() under a lock. + It should work also when the system is out of memory. Especially atomic context is problematic because we could not wait for memory reclaim or swap. + We also do to the best effort to get the message out on the console. It is important when the system is about to die. Any extra buffering layer might cause delay and avoid seeing the message. From this point of views, this API is not generally usable with printk(). Now, the question is how many of the above fits also for unit testing. At least, you might need to be careful when allocating memory in atomic context. BTW: There are more existing printk APIs: Well, I admit the they are not easily reusable in unit testing: + printk() is old, crappy code, complicated with all the cornercases and consoles. + include/linux/seq_buf.h is simple buffering. It is used primary for sysfs output. It might be usable if you add support for loglevel and use big enough buffer. I quess that you should flush the buffer regularly anyway. + trace_printk() uses lockless per-CPU buffers. It currently does not support loglevels. But it might be pretty interesting choice as well. I do not say that you have to use one of the existing API. But you might consider them if you encouter any problems and maintaining your variant gets complicated. Best Regards, Petr
On Mon, Dec 3, 2018 at 2:55 AM Petr Mladek <pmladek@suse.com> wrote: > > On Thu 2018-11-29 19:29:24, Luis Chamberlain wrote: > > On Wed, Nov 28, 2018 at 11:36:20AM -0800, Brendan Higgins wrote: > > > A number of test features need to do pretty complicated string printing > > > where it may not be possible to rely on a single preallocated string > > > with parameters. > > > > > > So provide a library for constructing the string as you go similar to > > > C++'s std::string. > > > > Hrm, what's the potential for such thing actually being eventually > > generically useful for printk folks, I wonder? Petr? > > printk() is a bit tricky: > > + It should work in any context. Any additional lock adds risk of a > deadlock. Especially the NMI and scheduler contexts are problematic. > There are problems with any other code that might be called > from console drivers and calls printk() under a lock. > > + It should work also when the system is out of memory. Especially > atomic context is problematic because we could not wait for > memory reclaim or swap. > > + We also do to the best effort to get the message out on the > console. It is important when the system is about to die. > Any extra buffering layer might cause delay and avoid seeing the > message. > > From this point of views, this API is not generally usable with printk(). Yeah, that makes sense. I wouldn't really expect this to work well in those cases. > Now, the question is how many of the above fits also for unit testing. > At least, you might need to be careful when allocating memory in > atomic context. True, but this is only supposed to be used for constructing expectation failure messages which should only happen from a non-atomic context. > > BTW: There are more existing printk APIs: Well, I admit the they are > not easily reusable in unit testing: > > + printk() is old, crappy code, complicated with all the > cornercases and consoles. > > + include/linux/seq_buf.h is simple buffering. It is used primary > for sysfs output. It might be usable if you add support for > loglevel and use big enough buffer. I quess that you should > flush the buffer regularly anyway. > > + trace_printk() uses lockless per-CPU buffers. It currently does not > support loglevels. But it might be pretty interesting choice as well. > > > I do not say that you have to use one of the existing API. But you > might consider them if you encouter any problems and maintaining > your variant gets complicated. Alright, I will take a look. Thanks!
diff --git a/include/kunit/string-stream.h b/include/kunit/string-stream.h new file mode 100644 index 0000000000000..933ed5740cf07 --- /dev/null +++ b/include/kunit/string-stream.h @@ -0,0 +1,44 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * C++ stream style string builder used in KUnit for building messages. + * + * Copyright (C) 2018, 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 <linux/kref.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 kref refcount; + int (*add)(struct string_stream *this, const char *fmt, ...); + int (*vadd)(struct string_stream *this, const char *fmt, va_list args); + char *(*get_string)(struct string_stream *this); + void (*clear)(struct string_stream *this); + bool (*is_empty)(struct string_stream *this); +}; + +struct string_stream *new_string_stream(void); + +void destroy_string_stream(struct string_stream *stream); + +void string_stream_get(struct string_stream *stream); + +int string_stream_put(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..1e7efa630cc35 --- /dev/null +++ b/kunit/string-stream.c @@ -0,0 +1,149 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * C++ stream style string builder used in KUnit for building messages. + * + * Copyright (C) 2018, Google LLC. + * Author: Brendan Higgins <brendanhiggins@google.com> + */ + +#include <linux/list.h> +#include <linux/slab.h> +#include <kunit/string-stream.h> + +static int string_stream_vadd(struct string_stream *this, + const char *fmt, + va_list args) +{ + struct string_stream_fragment *fragment; + int len; + va_list args_for_counting; + unsigned long flags; + + /* 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); + + fragment = kmalloc(sizeof(*fragment), GFP_KERNEL); + if (!fragment) + return -ENOMEM; + + fragment->fragment = kmalloc(len, GFP_KERNEL); + if (!fragment->fragment) { + kfree(fragment); + return -ENOMEM; + } + + len = vsnprintf(fragment->fragment, len, fmt, args); + spin_lock_irqsave(&this->lock, flags); + this->length += len; + list_add_tail(&fragment->node, &this->fragments); + spin_unlock_irqrestore(&this->lock, flags); + return 0; +} + +static int string_stream_add(struct string_stream *this, const char *fmt, ...) +{ + va_list args; + int result; + + va_start(args, fmt); + result = string_stream_vadd(this, fmt, args); + va_end(args); + return result; +} + +static void string_stream_clear(struct string_stream *this) +{ + struct string_stream_fragment *fragment, *fragment_safe; + unsigned long flags; + + spin_lock_irqsave(&this->lock, flags); + list_for_each_entry_safe(fragment, + fragment_safe, + &this->fragments, + node) { + list_del(&fragment->node); + kfree(fragment->fragment); + kfree(fragment); + } + this->length = 0; + spin_unlock_irqrestore(&this->lock, flags); +} + +static char *string_stream_get_string(struct string_stream *this) +{ + struct string_stream_fragment *fragment; + size_t buf_len = this->length + 1; /* +1 for null byte. */ + char *buf; + unsigned long flags; + + buf = kzalloc(buf_len, GFP_KERNEL); + if (!buf) + return NULL; + + spin_lock_irqsave(&this->lock, flags); + list_for_each_entry(fragment, &this->fragments, node) + strlcat(buf, fragment->fragment, buf_len); + spin_unlock_irqrestore(&this->lock, flags); + + return buf; +} + +static bool string_stream_is_empty(struct string_stream *this) +{ + bool is_empty; + unsigned long flags; + + spin_lock_irqsave(&this->lock, flags); + is_empty = list_empty(&this->fragments); + spin_unlock_irqrestore(&this->lock, flags); + + return is_empty; +} + +void destroy_string_stream(struct string_stream *stream) +{ + stream->clear(stream); + kfree(stream); +} + +static void string_stream_destroy(struct kref *kref) +{ + struct string_stream *stream = container_of(kref, + struct string_stream, + refcount); + destroy_string_stream(stream); +} + +struct string_stream *new_string_stream(void) +{ + struct string_stream *stream = kzalloc(sizeof(*stream), GFP_KERNEL); + + if (!stream) + return NULL; + + INIT_LIST_HEAD(&stream->fragments); + spin_lock_init(&stream->lock); + kref_init(&stream->refcount); + stream->add = string_stream_add; + stream->vadd = string_stream_vadd; + stream->get_string = string_stream_get_string; + stream->clear = string_stream_clear; + stream->is_empty = string_stream_is_empty; + return stream; +} + +void string_stream_get(struct string_stream *stream) +{ + kref_get(&stream->refcount); +} + +int string_stream_put(struct string_stream *stream) +{ + return kref_put(&stream->refcount, &string_stream_destroy); +} +
A number of test features need to do pretty complicated string printing where it may not be possible to rely on a single preallocated string with parameters. So provide a library for constructing the string as you go similar to C++'s std::string. Signed-off-by: Brendan Higgins <brendanhiggins@google.com> --- include/kunit/string-stream.h | 44 ++++++++++ kunit/Makefile | 3 +- kunit/string-stream.c | 149 ++++++++++++++++++++++++++++++++++ 3 files changed, 195 insertions(+), 1 deletion(-) create mode 100644 include/kunit/string-stream.h create mode 100644 kunit/string-stream.c