On Tue, 22 Aug 2023 at 00:10, Richard Fitzgerald rf@opensource.cirrus.com wrote:
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:
- Add a kunit_string_stream_get_string() anyway. But only the test code
uses it.
- 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.
- 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.
- 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/...
Cheers, -- David