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
