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.
