Add some more cfg80211 and mac80211 kunit tests

From: Benjamin Berg <[email protected]>

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 <[email protected]>

---

 Documentation/dev-tools/kunit/usage.rst | 12 ++++--------
 include/kunit/test.h                    | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+), 8 deletions(-)

Documentation/dev-tools/kunit/usage.rst +4 -8

index c27e1646ecd9..b959e5befcbe 100644
@@ -566,13 +566,9 @@ By reusing the same ``cases`` array from above, we can write the test as a
                },
        };
 
-       // Need a helper function to generate a name for each test case.
-       static void case_to_desc(const struct sha1_test_case *t, char *desc)
-       {
-               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 +584,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),
                {}

include/kunit/test.h +2 -0

index 20ed9f9275c9..2dfa851e1f88 100644
@@ -1514,6 +1514,25 @@ do {                                                                            \
                return NULL;                                                                    \
        }
 
+/**
+ * KUNIT_ARRAY_PARAM_DESC() - Define test parameter generator from an array.

include/kunit/test.h +7 -1

+               if (__next - (array) < ARRAY_SIZE((array))) {                                   \
+                       strscpy(desc, __next->desc_member, KUNIT_PARAM_DESC_SIZE);              \
+                       return __next;                                                          \
+               }                                                                               \
+               return NULL;                                                                    \
+       }
+
 // TODO([email protected]): consider eventually migrating users to explicitly
 // include resource.h themselves if they need it.
 #include <kunit/resource.h>
--

2.43.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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20231220151952.415232-2-benjamin%40sipsolutions.net.

Quoted 11 lines · On Wed, 20 Dec 2023 at 23:20, <[email protected]> wrote:

From: Benjamin Berg <[email protected]>

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 <[email protected]>

---

I'm generally pretty happy with this, though note the checkpatch warning below.

There was some discussion at plumbers about expanding the parameterised test APIs, so we may need to adjust the implementation of this down the line, but I don't think that'll happen for a while, so don't worry.

With the warnings fixed, this is:

Reviewed-by David Gow <[email protected]>

I'm okay with this going in via the wireless tree if that's easier; certainly there are some conflicts with the later patches in this series and the kunit one.

Cheers,
-- David

Quoted 66 lines

 Documentation/dev-tools/kunit/usage.rst | 12 ++++-------- include/kunit/test.h                    | 19 +++++++++++++++++++

 2 files changed, 23 insertions(+), 8 deletions(-)

Documentation/dev-tools/kunit/usage.rst +0 -0

index c27e1646ecd9..b959e5befcbe 100644
@@ -566,13 +566,9 @@ By reusing the same ``cases`` array from

above, we can write the test as a
                },
        };

-       // Need a helper function to generate a name for each test case.
-       static void case_to_desc(const struct sha1_test_case *t, char *desc)
-       {
-               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)

Documentation/dev-tools/kunit/usage.rst +0 -0

@@ -588,7 +584,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), {}

include/kunit/test.h +0 -0

index 20ed9f9275c9..2dfa851e1f88 100644
@@ -1514,6 +1514,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,

Quoted 8 lines

desc_member)                                       \
+       static const void *name##_gen_params(const void *prev, char *desc)                      \
+
{
                     \
+               typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array);      \

checkpatch is complaining here:
ERROR: need consistent spacing around '*' (ctx:WxV) #71: FILE: include/kunit/test.h:1528:

+               typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array);      \

Quoted 29 lines

+               if (__next - (array) < ARRAY_SIZE((array))) {                                   \
+                       strscpy(desc, __next->desc_member, KUNIT_PARAM_DESC_SIZE);              \
+                       return
__next;                                                          \
+
}
             \
+               return
NULL;
      \
+       }
+
 // TODO([email protected]): consider eventually migrating users to explicitly
 // include resource.h themselves if they need it. #include <kunit/resource.h>
--
2.43.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 [email protected]. To view this discussion on the web visit
https://groups.google.com/d/msgid/kunit-dev/20231220151952.415232-2-benjamin%40sipsolutions.net
.

Quoted 21 lines · David Gow

Quoted 11 lines · On Wed, 20 Dec 2023 at 23:20, <[email protected]> wrote:

From: Benjamin Berg <[email protected]>

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 <[email protected]>

---

I'm generally pretty happy with this, though note the checkpatch warning below.

There was some discussion at plumbers about expanding the parameterised test APIs, so we may need to adjust the implementation of this down the line, but I don't think that'll happen for a while, so don't worry.

Quoted 1 line

With the warnings fixed, this is:

I think the checkpatch warning is a false positive. It seems to confuse the * as a multiplication due to typeof() looking like a function call rather than a variable declaration.

Benjamin

From: Benjamin Berg <[email protected]>

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 <[email protected]>

---

include/kunit/skbuff.h new file +53 -1

+ *
+ * Copyright (C) 2023 Intel Corporation
+ */
+
+#ifndef _KUNIT_SKBUFF_H
+#define _KUNIT_SKBUFF_H
+
+#include <kunit/resource.h>
+#include <linux/skbuff.h>
+
+static void kunit_action_kfree_skb(void *p)
+{
+       kfree_skb((struct sk_buff *)p);
+}
+
+/**
+ * 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_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_kfree_skb, (void *)skb);
+}
+
+#endif /* _KUNIT_SKBUFF_H */
--

2.43.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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20231220151952.415232-3-benjamin%40sipsolutions.net.

This patchset adds a couple of helpers for kunit as well as tests for cfg80211 and mac80211 that use them.

kunit: add parameter generation macro using description from array
kunit: add a convenience allocation wrapper for SKBs

Quoted 3 lines

This patchset adds a couple of helpers for kunit as well as tests for cfg80211 and mac80211 that use them.

I can take this through the wireless tree, but then I'd like to have ACKs from kunit folks for the kunit patches:

Quoted 3 lines

kunit: add parameter generation macro using description from array
kunit: add a convenience allocation wrapper for SKBs

Quoted 4 lines · Johannes Berg

Quoted 3 lines

This patchset adds a couple of helpers for kunit as well as tests for cfg80211 and mac80211 that use them.

Quoted 3 lines

I can take this through the wireless tree, but then I'd like to have ACKs from kunit folks for the kunit patches:

We have run into conflicts in the past with the kunit tree. I take the kunit patches through linux-kselftest tree. I do want to make sure there are no conflicts. I don't mind taking these through my tree.

Quoted 5 lines · Shuah Khan

Quoted 4 lines · Johannes Berg

Quoted 3 lines

This patchset adds a couple of helpers for kunit as well as tests for cfg80211 and mac80211 that use them.

Quoted 7 lines

Quoted 3 lines

I can take this through the wireless tree, but then I'd like to have ACKs from kunit folks for the kunit patches:

We have run into conflicts in the past with the kunit tree. I take the kunit patches through linux-kselftest tree. I do want to make sure there are no conflicts. I don't mind taking these through my tree.

OK, fair enough.

If you can still put it into 6.8, then I think you can also take the wireless tests, assuming they pass (I haven't run them in the posted version). I don't think we'll have conflicts there, we don't have much work in wireless that's likely to land for 6.8.

Quoted 14 lines · Johannes Berg

Quoted 5 lines · Shuah Khan

Quoted 4 lines · Johannes Berg

Quoted 3 lines

This patchset adds a couple of helpers for kunit as well as tests for cfg80211 and mac80211 that use them.

Quoted 7 lines

Quoted 3 lines

I can take this through the wireless tree, but then I'd like to have ACKs from kunit folks for the kunit patches:

We have run into conflicts in the past with the kunit tree. I take the kunit patches through linux-kselftest tree. I do want to make sure there are no conflicts. I don't mind taking these through my tree.

Quoted 7 lines

OK, fair enough.

If you can still put it into 6.8, then I think you can also take the wireless tests, assuming they pass (I haven't run them in the posted version). I don't think we'll have conflicts there, we don't have much work in wireless that's likely to land for 6.8.

Sounds good.

David, will you be able to look at these patches and let me know if I can apply for Linux 6.8-rc1.

The two initial KUnit patches look fine, modulo a couple of minor docs issues and checkpatch warnings.

They apply cleanly, and I doubt
there's much chance of there being a merge conflict for 6.8 -- there are no other changes to the parameterised test macros, and the skb stuff is in its own file.

The remaining patches don't apply on top of the kunit branch as-is.

I
haven't had a chance to review them properly yet; the initial glance I had didn't show any serious issues (though I think checkpatch suggested some things to 'check').

So (once those small issues are finished), I'm okay with the first two patches going in via either tree. The remaining ones are probably best done via the wireless tree, as they seem to depend on some existing patches there, so maybe it makes sense to push everything via wireless.

Hi,

Thanks for taking a look!

Quoted 2 lines · David Gow

The two initial KUnit patches look fine, modulo a couple of minor docs issues and checkpatch warnings.

I can run checkpatch (even if I can't always take it seriously), but do you want to comment more specifically wrt. the docs?

Quoted 4 lines

They apply cleanly, and I doubt
there's much chance of there being a merge conflict for 6.8 -- there are no other changes to the parameterised test macros, and the skb stuff is in its own file.

Right.

Quoted 1 line

The remaining patches don't apply on top of the kunit branch as-is.

Oh, OK. That makes some sense though, we've had a number of changes in the stack this cycle before. I somehow thought the tests were likely standalone, but apparently not.

Quoted 4 lines

I
haven't had a chance to review them properly yet; the initial glance I had didn't show any serious issues (though I think checkpatch suggested some things to 'check').

I can check.

Quoted 6 lines

So (once those small issues are finished), I'm okay with the first two patches going in via either tree. The remaining ones are probably best done via the wireless tree, as they seem to depend on some existing patches there, so maybe it makes sense to push everything via wireless.

If not through wireless I doubt we'll get it synchronized for 6.8, though of course it's also not needed for 6.8 to have the extra unit tests :)

I'll let Shuah decide.

Hi,

Thanks for taking a look!

Quoted 2 lines · David Gow

The two initial KUnit patches look fine, modulo a couple of minor docs issues and checkpatch warnings.

I can run checkpatch (even if I can't always take it seriously), but do you want to comment more specifically wrt. the docs?

Quoted 4 lines

They apply cleanly, and I doubt
there's much chance of there being a merge conflict for 6.8 -- there are no other changes to the parameterised test macros, and the skb stuff is in its own file.

Right.

Quoted 1 line

The remaining patches don't apply on top of the kunit branch as-is.

Oh, OK. That makes some sense though, we've had a number of changes in the stack this cycle before. I somehow thought the tests were likely standalone, but apparently not.

Quoted 4 lines

I
haven't had a chance to review them properly yet; the initial glance I had didn't show any serious issues (though I think checkpatch suggested some things to 'check').

I can check.

Quoted 5 lines

So (once those small issues are finished), I'm okay with the first two patches going in via either tree. The remaining ones are probably best done via the wireless tree, as they seem to depend on some existing patches there, so maybe it makes sense to push everything via wireless.

If not through wireless I doubt we'll get it synchronized for 6.8, though of course it's also not needed for 6.8 to have the extra unit tests :)

I'll let Shuah decide.