Date:   Fri, 25 Aug 2023 14:58:12 +0800
From:   David Gow <davidgow@...gle.com>
To:     Richard Fitzgerald <rf@...nsource.cirrus.com>
Cc:     brendan.higgins@...ux.dev, rmoar@...gle.com,
        linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
        linux-kernel@...r.kernel.org, patches@...nsource.cirrus.com
Subject: Re: [PATCH v5 00/10] kunit: Add dynamically-extending log

On Thu, 24 Aug 2023 at 22:32, 'Richard Fitzgerald' via KUnit
Development <kunit-dev@...glegroups.com> wrote:
>
> This patch chain changes the logging implementation to use string_stream
> so that the log will grow dynamically.
>
> The first 8 patches add test code for string_stream, and make some
> changes to string_stream needed to be able to use it for the log.
>
> The final patch adds a performance report of string_stream.
>
> CHANGES SINCE V4:
> - Re-ordered the first 3 patches from V4 to squash the first two sets
>   of string_stream tests into a single patch.
> - Changed is_literal() so it doesn't need a struct kunit.
> - Split out the new resource-managed alloc and free functions into
>   a pre-patch to reduce the amount of code churn when the string_stream
>   is decoupled from kunit.
> - Wrapped the call to string_stream_geT_string() in string-stream-test
>   in a local function to reduce the amount of code churn when the
>   string_stream is decoupled from kunit.
> - Some minor changes to test implementations.
> - string_stream is now completely separated from kunit and the 'test'
>   member of struct string_stream has been eliminated.
>
> Richard Fitzgerald (10):
>   kunit: string-stream: Don't create a fragment for empty strings
>   kunit: string-stream: Improve testing of string_stream
>   kunit: string-stream: Add option to make all lines end with newline
>   kunit: string-stream: Add cases for string_stream newline appending
>   kunit: Don't use a managed alloc in is_literal()
>   kunit: string-stream: Add kunit_alloc_string_stream()
>   kunit: string-stream: Decouple string_stream from kunit
>   kunit: string-stream: Add tests for freeing resource-managed
>     string_stream
>   kunit: Use string_stream for test log
>   kunit: string-stream: Test performance of string_stream
>

Thanks a lot for sticking with this. I think we're in pretty good
shape now. There are a few minor comments, only one of which really
concerns me. That's the freeing of string streams in the
resource-managed string stream tests. I can't quite see how the actual
stream is freed after being "fake freed" by the stub. Is there
something I'm missing?

Otherwise, this seems good enough to go. I fear we're probably past
the point where it can make it into 6.6 (pull requests are already
being sent out, and I'd really rather have the final version of this
soak in linux-next for a while before sending it to Linus. But we'll
make it the first thing to go into 6.7, I think.

Cheers,
-- David


>  include/kunit/test.h           |  14 +-
>  lib/kunit/assert.c             |  14 +-
>  lib/kunit/debugfs.c            |  36 ++-
>  lib/kunit/kunit-test.c         |  46 ++-
>  lib/kunit/string-stream-test.c | 508 +++++++++++++++++++++++++++++++--
>  lib/kunit/string-stream.c      | 100 +++++--
>  lib/kunit/string-stream.h      |  16 +-
>  lib/kunit/test.c               |  50 +---
>  8 files changed, 662 insertions(+), 122 deletions(-)
>
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230824143129.1957914-1-rf%40opensource.cirrus.com.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)

Powered by blists - more mailing lists