| Message ID | 20230824143129.1957914-6-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7b4481cbe7e6bde275aa5e5f2aaa21455915e07c |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: Add dynamically-extending log | expand |
On Thu, 24 Aug 2023 at 22:31, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > There is no need to use a test-managed alloc in is_literal(). > The function frees the temporary buffer before returning. > > This removes the only use of the test and gfp members of > struct string_stream outside of the string_stream implementation. > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > --- This makes sense to me, particularly given how independent string-stream otherwise is from the KUnit resource management bits. The only possible downside is that the memory won't be cleaned up if strncmp() crashes due to 'text' being somehow invalid. But given this is really only even used with static data (generated by the assert macros), and to fail on the strncmp and not the strlen() would require some horrible race-condition-y madness, I don't think it's ever reasonably possible to hit that case. So, looks good. Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David > lib/kunit/assert.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c > index 05a09652f5a1..dd1d633d0fe2 100644 > --- a/lib/kunit/assert.c > +++ b/lib/kunit/assert.c > @@ -89,8 +89,7 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, > EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format); > > /* Checks if `text` is a literal representing `value`, e.g. "5" and 5 */ > -static bool is_literal(struct kunit *test, const char *text, long long value, > - gfp_t gfp) > +static bool is_literal(const char *text, long long value) > { > char *buffer; > int len; > @@ -100,14 +99,15 @@ static bool is_literal(struct kunit *test, const char *text, long long value, > if (strlen(text) != len) > return false; > > - buffer = kunit_kmalloc(test, len+1, gfp); > + buffer = kmalloc(len+1, GFP_KERNEL); > if (!buffer) > return false; > > snprintf(buffer, len+1, "%lld", value); > ret = strncmp(buffer, text, len) == 0; > > - kunit_kfree(test, buffer); > + kfree(buffer); > + > return ret; > } > > @@ -125,14 +125,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, > binary_assert->text->left_text, > binary_assert->text->operation, > binary_assert->text->right_text); > - if (!is_literal(stream->test, binary_assert->text->left_text, > - binary_assert->left_value, stream->gfp)) > + if (!is_literal(binary_assert->text->left_text, binary_assert->left_value)) > string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)\n", > binary_assert->text->left_text, > binary_assert->left_value, > binary_assert->left_value); > - if (!is_literal(stream->test, binary_assert->text->right_text, > - binary_assert->right_value, stream->gfp)) > + if (!is_literal(binary_assert->text->right_text, binary_assert->right_value)) > string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)", > binary_assert->text->right_text, > binary_assert->right_value, > -- > 2.30.2 >
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c index 05a09652f5a1..dd1d633d0fe2 100644 --- a/lib/kunit/assert.c +++ b/lib/kunit/assert.c @@ -89,8 +89,7 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format); /* Checks if `text` is a literal representing `value`, e.g. "5" and 5 */ -static bool is_literal(struct kunit *test, const char *text, long long value, - gfp_t gfp) +static bool is_literal(const char *text, long long value) { char *buffer; int len; @@ -100,14 +99,15 @@ static bool is_literal(struct kunit *test, const char *text, long long value, if (strlen(text) != len) return false; - buffer = kunit_kmalloc(test, len+1, gfp); + buffer = kmalloc(len+1, GFP_KERNEL); if (!buffer) return false; snprintf(buffer, len+1, "%lld", value); ret = strncmp(buffer, text, len) == 0; - kunit_kfree(test, buffer); + kfree(buffer); + return ret; } @@ -125,14 +125,12 @@ void kunit_binary_assert_format(const struct kunit_assert *assert, binary_assert->text->left_text, binary_assert->text->operation, binary_assert->text->right_text); - if (!is_literal(stream->test, binary_assert->text->left_text, - binary_assert->left_value, stream->gfp)) + if (!is_literal(binary_assert->text->left_text, binary_assert->left_value)) string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)\n", binary_assert->text->left_text, binary_assert->left_value, binary_assert->left_value); - if (!is_literal(stream->test, binary_assert->text->right_text, - binary_assert->right_value, stream->gfp)) + if (!is_literal(binary_assert->text->right_text, binary_assert->right_value)) string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld (0x%llx)", binary_assert->text->right_text, binary_assert->right_value,
There is no need to use a test-managed alloc in is_literal(). The function frees the temporary buffer before returning. This removes the only use of the test and gfp members of struct string_stream outside of the string_stream implementation. Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> --- lib/kunit/assert.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)