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

[Richard Fitzgerald <rf@opensource.cirrus.com>]

On 17/08/2023 07:26, David Gow wrote:
> On Tue, 15 Aug 2023 at 17:57, Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
>>
>> On 15/8/23 10:16, David Gow wrote:
>>> On Mon, 14 Aug 2023 at 21:23, Richard Fitzgerald
>>> <rf@opensource.cirrus.com> 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@opensource.cirrus.com>
>>>> ---
>>>
>>> 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.