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

[Richard Fitzgerald]

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.