From: Benjamin Berg benjamin.berg@intel.com
The existing KUNIT_ARRAY_PARAM macro requires a separate function to get the description. However, in a lot of cases the description can just be copied directly from the array. Add a second macro that avoids having to write a static function just for a single strscpy.
Signed-off-by: Benjamin Berg benjamin.berg@intel.com --- Documentation/dev-tools/kunit/usage.rst | 7 ++++--- include/kunit/test.h | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index c27e1646ecd9..fe8c28d66dfe 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -571,8 +571,9 @@ By reusing the same ``cases`` array from above, we can write the test as a { strcpy(desc, t->str); } - // Creates `sha1_gen_params()` to iterate over `cases`. - KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc); + // Creates `sha1_gen_params()` to iterate over `cases` while using + // the struct member `str` for the case description. + KUNIT_ARRAY_PARAM_DESC(sha1, cases, str);
// Looks no different from a normal test. static void sha1_test(struct kunit *test) @@ -588,7 +589,7 @@ By reusing the same ``cases`` array from above, we can write the test as a }
// Instead of KUNIT_CASE, we use KUNIT_CASE_PARAM and pass in the - // function declared by KUNIT_ARRAY_PARAM. + // function declared by KUNIT_ARRAY_PARAM or KUNIT_ARRAY_PARAM_DESC. static struct kunit_case sha1_test_cases[] = { KUNIT_CASE_PARAM(sha1_test, sha1_gen_params), {} diff --git a/include/kunit/test.h b/include/kunit/test.h index 68ff01aee244..f60d11e41855 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -1516,6 +1516,25 @@ do { \ return NULL; \ }
+/** + * KUNIT_ARRAY_PARAM_DESC() - Define test parameter generator from an array. + * @name: prefix for the test parameter generator function. + * @array: array of test parameters. + * @desc_member: structure member from array element to use as description + * + * Define function @name_gen_params which uses @array to generate parameters. + */ +#define KUNIT_ARRAY_PARAM_DESC(name, array, desc_member) \ + static const void *name##_gen_params(const void *prev, char *desc) \ + { \ + typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array); \ + if (__next - (array) < ARRAY_SIZE((array))) { \ + strscpy(desc, __next->desc_member, KUNIT_PARAM_DESC_SIZE); \ + return __next; \ + } \ + return NULL; \ + } + // TODO(dlatypov@google.com): consider eventually migrating users to explicitly // include resource.h themselves if they need it. #include <kunit/resource.h>
From: Benjamin Berg benjamin.berg@intel.com
Add a simple convenience helper to allocate and zero fill an SKB for the use by a kunit test. Also provide a way to free it again in case that may be desirable.
This simply mirrors the kunit_kmalloc API.
Signed-off-by: Benjamin Berg benjamin.berg@intel.com --- include/kunit/skbuff.h | 51 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 include/kunit/skbuff.h
diff --git a/include/kunit/skbuff.h b/include/kunit/skbuff.h new file mode 100644 index 000000000000..947fc8b5b48f --- /dev/null +++ b/include/kunit/skbuff.h @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Base unit test (KUnit) API. + * + * Copyright (C) 2023 Intel Corporation + */ + +#ifndef _KUNIT_SKBUFF_H +#define _KUNIT_SKBUFF_H + +#include <kunit/resource.h> +#include <linux/skbuff.h> + +/** + * kunit_zalloc_skb() - Allocate and initialize a resource managed skb. + * @test: The test case to which the skb belongs + * @len: size to allocate + * + * Allocate a new struct sk_buff with GFP_KERNEL, zero fill the give length + * and add it as a resource to the kunit test for automatic cleanup. + * + * Returns: newly allocated SKB, or %NULL on error + */ +static inline struct sk_buff *kunit_zalloc_skb(struct kunit *test, int len, + gfp_t gfp) +{ + struct sk_buff *res = alloc_skb(len, GFP_KERNEL); + + if (!res || skb_pad(res, len)) + return NULL; + + if (kunit_add_action_or_reset(test, (kunit_action_t *)kfree_skb, res)) + return NULL; + + return res; +} + +/** + * kunit_kfree_skb() - Like kfree_skb except for allocations managed by KUnit. + * @test: The test case to which the resource belongs. + * @skb: The SKB to free. + */ +static inline void kunit_kfree_skb(struct kunit *test, struct sk_buff *skb) +{ + if (!skb) + return; + + kunit_release_action(test, (kunit_action_t *)kfree_skb, (void *)skb); +} + +#endif /* _KUNIT_SKBUFF_H */
On Mon, 4 Sept 2023 at 21:22, benjamin@sipsolutions.net wrote:
From: Benjamin Berg benjamin.berg@intel.com
Add a simple convenience helper to allocate and zero fill an SKB for the use by a kunit test. Also provide a way to free it again in case that may be desirable.
This simply mirrors the kunit_kmalloc API.
Signed-off-by: Benjamin Berg benjamin.berg@intel.com
This _looks_ pretty good to me, but again, I'd like to see something use it, be it a simple test for these helpers, or a real-world test which takes advantage of them. Particularly since I've not had to use sk_buffs before, personally, so I'd love to see it actually working.
Otherwise, this seems okay to me. I'll hold off a final judgement until I've had a chance to actually run it, but I've left a few minor notes (mostly to myself) below.
Cheers, -- David
include/kunit/skbuff.h | 51 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 include/kunit/skbuff.h
diff --git a/include/kunit/skbuff.h b/include/kunit/skbuff.h new file mode 100644 index 000000000000..947fc8b5b48f --- /dev/null +++ b/include/kunit/skbuff.h @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Base unit test (KUnit) API.
- Copyright (C) 2023 Intel Corporation
- */
+#ifndef _KUNIT_SKBUFF_H +#define _KUNIT_SKBUFF_H
Is it worth us hiding this behind #if IS_ENABLED(CONFIG_KUNIT)? I suspect not (we haven't bothered for resource.h, and only really do this for hooks/stubs).
+#include <kunit/resource.h> +#include <linux/skbuff.h>
+/**
- kunit_zalloc_skb() - Allocate and initialize a resource managed skb.
- @test: The test case to which the skb belongs
- @len: size to allocate
- Allocate a new struct sk_buff with GFP_KERNEL, zero fill the give length
- and add it as a resource to the kunit test for automatic cleanup.
- Returns: newly allocated SKB, or %NULL on error
- */
+static inline struct sk_buff *kunit_zalloc_skb(struct kunit *test, int len,
gfp_t gfp)+{
struct sk_buff *res = alloc_skb(len, GFP_KERNEL);if (!res || skb_pad(res, len))return NULL;
From a quick look, skb_pad() will free 'res' if it fails? If so, this is fine, if not we may need to move the add_action call below to prevent a leak.
if (kunit_add_action_or_reset(test, (kunit_action_t *)kfree_skb, res))return NULL;
Just be warned that, while casting to kunit_action_t* is fine by me, some versions of clang are warning on any function pointer casts. So you can expect some whinge-y automatic emails from the kernel test robot and similar.
return res;+}
+/**
- kunit_kfree_skb() - Like kfree_skb except for allocations managed by KUnit.
- @test: The test case to which the resource belongs.
- @skb: The SKB to free.
- */
+static inline void kunit_kfree_skb(struct kunit *test, struct sk_buff *skb) +{
if (!skb)return;kunit_release_action(test, (kunit_action_t *)kfree_skb, (void *)skb);
As above, note that the kunit_action_t cast may cause warnings on some versions of clang. I'm personally okay with it, but if you want to write a wrapper to avoid it, that's fine by me, too.
+}
+#endif /* _KUNIT_SKBUFF_H */
2.41.0
-- You received this message because you are subscribed to the Google Groups "KUnit Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230904132139.103140-2-benjamin....
On Mon, 4 Sept 2023 at 21:22, benjamin@sipsolutions.net wrote:
From: Benjamin Berg benjamin.berg@intel.com
The existing KUNIT_ARRAY_PARAM macro requires a separate function to get the description. However, in a lot of cases the description can just be copied directly from the array. Add a second macro that avoids having to write a static function just for a single strscpy.
Signed-off-by: Benjamin Berg benjamin.berg@intel.com
Looks good to me: this will be much more convenient. The actual implementation looks spot on, just a small comment about the documentation change.
It may make sense to write some tests and/or some follow-up patches to existing tests to use this macro, too. I'm just a little wary of introducing something totally unused. (I'm happy to do these myself if you don't have time, though.)
Regardless, with the documentation fix, this is: Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
Documentation/dev-tools/kunit/usage.rst | 7 ++++--- include/kunit/test.h | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index c27e1646ecd9..fe8c28d66dfe 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -571,8 +571,9 @@ By reusing the same ``cases`` array from above, we can write the test as a { strcpy(desc, t->str); }
// Creates `sha1_gen_params()` to iterate over `cases`.KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc);
// Creates `sha1_gen_params()` to iterate over `cases` while using// the struct member `str` for the case description.KUNIT_ARRAY_PARAM_DESC(sha1, cases, str);
I'd suggest either getting rid of the case_to_desc function totally here, or show both the manual KUNIT_ARRAY_PARAM() example, and then point out explicitly that KUNIT_ARRAY_PARAM_DESC() can replace it.
Otherwise we end up with a vestigial function which doesn't do anything and is confusing.
// Looks no different from a normal test. static void sha1_test(struct kunit *test)@@ -588,7 +589,7 @@ By reusing the same ``cases`` array from above, we can write the test as a }
// Instead of KUNIT_CASE, we use KUNIT_CASE_PARAM and pass in the
// function declared by KUNIT_ARRAY_PARAM.
// function declared by KUNIT_ARRAY_PARAM or KUNIT_ARRAY_PARAM_DESC. static struct kunit_case sha1_test_cases[] = { KUNIT_CASE_PARAM(sha1_test, sha1_gen_params), {}diff --git a/include/kunit/test.h b/include/kunit/test.h index 68ff01aee244..f60d11e41855 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -1516,6 +1516,25 @@ do { \ return NULL; \ }
+/**
- KUNIT_ARRAY_PARAM_DESC() - Define test parameter generator from an array.
- @name: prefix for the test parameter generator function.
- @array: array of test parameters.
- @desc_member: structure member from array element to use as description
- Define function @name_gen_params which uses @array to generate parameters.
- */
+#define KUNIT_ARRAY_PARAM_DESC(name, array, desc_member) \
static const void *name##_gen_params(const void *prev, char *desc) \{ \typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array); \if (__next - (array) < ARRAY_SIZE((array))) { \strscpy(desc, __next->desc_member, KUNIT_PARAM_DESC_SIZE); \return __next; \} \return NULL; \}// TODO(dlatypov@google.com): consider eventually migrating users to explicitly // include resource.h themselves if they need it.
#include <kunit/resource.h>
2.41.0
-- You received this message because you are subscribed to the Google Groups "KUnit Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230904132139.103140-1-benjamin....
Hi,
On Wed, 2023-09-06 at 14:45 +0800, David Gow wrote:
On Mon, 4 Sept 2023 at 21:22, benjamin@sipsolutions.net wrote:
From: Benjamin Berg benjamin.berg@intel.com
The existing KUNIT_ARRAY_PARAM macro requires a separate function to get the description. However, in a lot of cases the description can just be copied directly from the array. Add a second macro that avoids having to write a static function just for a single strscpy.
Signed-off-by: Benjamin Berg benjamin.berg@intel.com
Looks good to me: this will be much more convenient. The actual implementation looks spot on, just a small comment about the documentation change.
It may make sense to write some tests and/or some follow-up patches to existing tests to use this macro, too. I'm just a little wary of introducing something totally unused. (I'm happy to do these myself if you don't have time, though.)
I agree. I am happy to submit one or more patches to change the existing users. The question would be how we pull such a change in. Should it be submitted separately for each subtree or can we pull them all in at the same time here?
Benjamin
Regardless, with the documentation fix, this is: Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
Documentation/dev-tools/kunit/usage.rst | 7 ++++--- include/kunit/test.h | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index c27e1646ecd9..fe8c28d66dfe 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -571,8 +571,9 @@ By reusing the same ``cases`` array from above, we can write the test as a { strcpy(desc, t->str); } - // Creates `sha1_gen_params()` to iterate over `cases`. - KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc); + // Creates `sha1_gen_params()` to iterate over `cases` while using + // the struct member `str` for the case description. + KUNIT_ARRAY_PARAM_DESC(sha1, cases, str);
I'd suggest either getting rid of the case_to_desc function totally here, or show both the manual KUNIT_ARRAY_PARAM() example, and then point out explicitly that KUNIT_ARRAY_PARAM_DESC() can replace it.
Otherwise we end up with a vestigial function which doesn't do anything and is confusing.
// Looks no different from a normal test. static void sha1_test(struct kunit *test) @@ -588,7 +589,7 @@ By reusing the same ``cases`` array from above, we can write the test as a }
// Instead of KUNIT_CASE, we use KUNIT_CASE_PARAM and pass in the - // function declared by KUNIT_ARRAY_PARAM. + // function declared by KUNIT_ARRAY_PARAM or KUNIT_ARRAY_PARAM_DESC. static struct kunit_case sha1_test_cases[] = { KUNIT_CASE_PARAM(sha1_test, sha1_gen_params), {} diff --git a/include/kunit/test.h b/include/kunit/test.h index 68ff01aee244..f60d11e41855 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -1516,6 +1516,25 @@ do { \ return NULL; \ }
+/**
- KUNIT_ARRAY_PARAM_DESC() - Define test parameter generator from
an array.
- @name: prefix for the test parameter generator function.
- @array: array of test parameters.
- @desc_member: structure member from array element to use as
description
- Define function @name_gen_params which uses @array to generate
parameters.
- */
+#define KUNIT_ARRAY_PARAM_DESC(name, array, desc_member) \ + static const void *name##_gen_params(const void *prev, char *desc) \ + { \ + typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array); \ + if (__next - (array) < ARRAY_SIZE((array))) { \ + strscpy(desc, __next->desc_member, KUNIT_PARAM_DESC_SIZE); \ + return __next; \ + } \ + return NULL; \ + }
// TODO(dlatypov@google.com): consider eventually migrating users to explicitly // include resource.h themselves if they need it. #include <kunit/resource.h> -- 2.41.0
-- You received this message because you are subscribed to the Google Groups "KUnit Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230904132139.103140-1-benjamin... .
Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de http://www.intel.de Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928
On Wed, 6 Sept 2023 at 15:07, Berg, Benjamin benjamin.berg@intel.com wrote:
Hi,
On Wed, 2023-09-06 at 14:45 +0800, David Gow wrote:
On Mon, 4 Sept 2023 at 21:22, benjamin@sipsolutions.net wrote:
From: Benjamin Berg benjamin.berg@intel.com
The existing KUNIT_ARRAY_PARAM macro requires a separate function to get the description. However, in a lot of cases the description can just be copied directly from the array. Add a second macro that avoids having to write a static function just for a single strscpy.
Signed-off-by: Benjamin Berg benjamin.berg@intel.com
Looks good to me: this will be much more convenient. The actual implementation looks spot on, just a small comment about the documentation change.
It may make sense to write some tests and/or some follow-up patches to existing tests to use this macro, too. I'm just a little wary of introducing something totally unused. (I'm happy to do these myself if you don't have time, though.)
I agree. I am happy to submit one or more patches to change the existing users. The question would be how we pull such a change in. Should it be submitted separately for each subtree or can we pull them all in at the same time here?
Benjamin
It depends a little bit on the test being modified: if it rarely sees conflicting changes, we can pull it in via KUnit, if it's being actively modified a lot, it's best to send it through separately.
If you're not sure, just include them all in a series here, CC the test maintainers, and ask what tree they'd prefer it to go in via.
It all usually works out in the end, and worst-case, if we miss one or two tests, we can update them separately.
Cheers, -- David
Regardless, with the documentation fix, this is: Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
Documentation/dev-tools/kunit/usage.rst | 7 ++++--- include/kunit/test.h | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index c27e1646ecd9..fe8c28d66dfe 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -571,8 +571,9 @@ By reusing the same ``cases`` array from above, we can write the test as a { strcpy(desc, t->str); }
// Creates `sha1_gen_params()` to iterate over `cases`.KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc);
// Creates `sha1_gen_params()` to iterate over `cases`while using
// the struct member `str` for the case description.KUNIT_ARRAY_PARAM_DESC(sha1, cases, str);I'd suggest either getting rid of the case_to_desc function totally here, or show both the manual KUNIT_ARRAY_PARAM() example, and then point out explicitly that KUNIT_ARRAY_PARAM_DESC() can replace it.
Otherwise we end up with a vestigial function which doesn't do anything and is confusing.
// Looks no different from a normal test. static void sha1_test(struct kunit *test)@@ -588,7 +589,7 @@ By reusing the same ``cases`` array from above, we can write the test as a }
// Instead of KUNIT_CASE, we use KUNIT_CASE_PARAM and passin the
// function declared by KUNIT_ARRAY_PARAM.
// function declared by KUNIT_ARRAY_PARAM orKUNIT_ARRAY_PARAM_DESC. static struct kunit_case sha1_test_cases[] = { KUNIT_CASE_PARAM(sha1_test, sha1_gen_params), {} diff --git a/include/kunit/test.h b/include/kunit/test.h index 68ff01aee244..f60d11e41855 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -1516,6 +1516,25 @@ do { \ return NULL; \ }
+/**
- KUNIT_ARRAY_PARAM_DESC() - Define test parameter generator from
an array.
- @name: prefix for the test parameter generator function.
- @array: array of test parameters.
- @desc_member: structure member from array element to use as
description
- Define function @name_gen_params which uses @array to generate
parameters.
- */
+#define KUNIT_ARRAY_PARAM_DESC(name, array, desc_member) \
static const void *name##_gen_params(const void *prev, char*desc) \
{ \
typeof((array)[0]) *__next = prev ?((typeof(__next)) prev) + 1 : (array); \
if (__next - (array) < ARRAY_SIZE((array))){ \
strscpy(desc, __next->desc_member,KUNIT_PARAM_DESC_SIZE); \
return__next; \
} \
returnNULL; \
}// TODO(dlatypov@google.com): consider eventually migrating users to explicitly // include resource.h themselves if they need it.
#include <kunit/resource.h>
2.41.0
-- You received this message because you are subscribed to the Google Groups "KUnit Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230904132139.103140-1-benjamin... .
Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de http://www.intel.de Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928
linux-kselftest-mirror@lists.linaro.org
-
benjamin＠sipsolutions.net -
Berg, Benjamin -
David Gow