Re: [PATCH v4 06/10] kunit: string-stream: Pass struct kunit to string_stream_get_string()

[David Gow]

On Tue, 22 Aug 2023 at 00:10, Richard Fitzgerald
<rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On 17/08/2023 07:26, David Gow wrote:
> > On Tue, 15 Aug 2023 at 17:57, Richard Fitzgerald
> > <rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >>
> >> On 15/8/23 10:16, David Gow wrote:
> >>> On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
> >>> <rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >>>>
> >>>> Pass a struct kunit* and gfp_t to string_stream_get_string(). Allocate
> >>>> the returned buffer using these instead of using the stream->test and
> >>>> stream->gfp.
> >>>>
> >>>> This is preparation for removing the dependence of string_stream on
> >>>> struct kunit, so that string_stream can be used for the debugfs log.
> >>>>
> >>>> Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>
> >>>> ---
> >>>
> >>> Makes sense to me.
> >>>
> >>> I think that, if we weren't going to remove the struct kunit
> >>> dependency, we'd want to either keep a version of
> >>> string_stream_get_string() which uses it, or, e.g., fall back to it if
> >>> the struct passed in is NULL.
> >>>
> >>
> >> That was my first thought. But I thought that was open to subtle
> >> accidental bugs in calling code - it might return you a managed
> >> allocation, or it might return you an unmanaged allocation that you
> >> must free.
> >>
> >> As there weren't many callers of string_stream_get_string() I decided
> >> to go with changing the API to pass in test and gfp like other managed
> >> allocations. This makes it more generalized, since the returned buffer
> >> is not part of the stream itself, it's a temporary buffer owned by the
> >> caller. It also makes it clearer that what you are getting back is
> >> likely to be a managed allocation.
> >>
> >> If you'd prefer to leave string_stream_get_string() as it was, or make
> >> it return an unmanged buffer, I can send a new version.
> >>
> >>> The other option is to have a version which doesn't manage the string
> >>> at all, so just takes a gfp and requires the caller to free it (or
> >>
> >> I didn't add a companion function to get a raw unmanaged string buffer
> >> because there's nothing that needs it. It could be added later if
> >> it's needed.
> >>
> >
> > Fair enough.
> >
> >>> register an action to do so). If we did that, we could get rid of the
> >>> struct kunit pointer totally (though it may be better to keep it and
> >>> have both versions).
> >>>
> >>
> >> Another option is to make string_stream_get_string() return a raw
> >> buffer and add a kunit_string_stream_geT_string() that returns a
> >> managed buffer. This follows some consistency with the normal mallocs
> >> where kunit_xxxx() is the managed version.
> >>
> >
> > Ooh... I like this best. Let's go with that.
> >
>
> I was busy last week with other things and have only got back to looking
> at this.
>
> I'm trying to decide what to do with string_stream_get_string() and I'd
> value an opinion.
>
> The only use for a test-managed get_string() is the string_stream test.
> So I've thought of 4 options:
>
> 1) Add a kunit_string_stream_get_string() anyway. But only the test code
> uses it.
>
> 2) Change the tests to have a local function to do the same thing, and
> add an explicit test case for the (unmanaged)
> string_stream_get_string() that ensures it's freed.
>
> 3) Change the tests to have a local function to get the string, which
> calls string_stream_get_string() then copies it to a test-managed buffer
> and frees the unmanaged buffer.
>
> 4) Implement a kunit_kfree_on_exit() function that takes an already-
> allocated buffer and kfree()s it when the test exists, so that we can
> do:
>
>     // on success kunit_kfree_on_exit() returns the buffer it was given
>     str = kunit_kfree_on_exit(test, string_stream_get_string(stream));
>     KUNIT_ASSERT_NOT_ERR_OR_NULL(test, str);
>
> I'm leaning towards (2) but open to going with any of the other options.

I'd be happy with any of those options (though 3 is my least favourite).

There is already a kfree_at_end() function in the executor test. It's
not quite as nice as your proposed kunit_kfree_on_exit() function (I
like this idea of passing the pointer through), but it proves the
concept. I think your version of this would be a useful thing to have
as an actual part of the KUnit API, rather than just a per-test hack:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/kunit/executor_test.c#n131

Cheers,
-- David

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature