[PATCH 1/3] kunit: add parameter generation macro using description from array

[PATCH 1/3] kunit: add parameter generation macro using description from array

[benjamin]

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>
---
 include/kunit/test.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 08d3559dd703..519b90261c72 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -1414,6 +1414,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.39.2

[PATCH 2/3] kunit: add ability to register a simple cleanup function

[benjamin]

From: Benjamin Berg <benjamin.berg@intel.com>

This is useful to e.g. automatically free resources at the end of the
test, without having to deal with kunit resource objects directly.

The whole point of doing this is that the callback is guaranteed to
happen in case the test aborts (due to an assertion). As such, use
assertions internally rather than requiring further error checking by
the API user.

Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
---
 include/kunit/test.h | 20 ++++++++++++++++++++
 lib/kunit/test.c     | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 519b90261c72..ab1dacf1c9f4 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -373,6 +373,26 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp
 	return kunit_kmalloc_array(test, n, size, gfp | __GFP_ZERO);
 }
 
+typedef void (*kunit_cleanup_t)(const void *);
+
+/**
+ * kunit_add_cleanup() - Add post-test cleanup action.
+ * @test: The test case to which the resource belongs.
+ * @cleanup_func: function to call at end of test.
+ * @data: data to pass to @free_func.
+ * @internal_gfp: gfp to use for internal allocations, if unsure, use GFP_KERNEL
+ *
+ * This adds a cleanup action to be executed after the test completes.
+ * Internally this is handled using a *test managed resource*.
+ *
+ * This function will abort the test on failure.
+ *
+ * Note: KUnit needs to allocate memory for a kunit_resource object. You must
+ * specify an @internal_gfp that is compatible with the current context.
+ */
+void kunit_add_cleanup(struct kunit *test, kunit_cleanup_t cleanup_func,
+		       const void *data, gfp_t internal_gfp);
+
 void kunit_cleanup(struct kunit *test);
 
 void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c9e15bb60058..72d956dfc324 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -719,6 +719,43 @@ void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp)
 }
 EXPORT_SYMBOL_GPL(kunit_kmalloc_array);
 
+struct kunit_auto_cleanup {
+	struct kunit_resource resource;
+	kunit_cleanup_t cleanup_func;
+};
+
+static void kunit_auto_cleanup_free(struct kunit_resource *res)
+{
+	struct kunit_auto_cleanup *cleanup;
+
+	cleanup = container_of(res, struct kunit_auto_cleanup, resource);
+
+	cleanup->cleanup_func(cleanup->resource.data);
+}
+
+void kunit_add_cleanup(struct kunit *test, kunit_cleanup_t cleanup_func,
+		       const void *data, gfp_t internal_gfp)
+{
+	struct kunit_auto_cleanup *res;
+
+	KUNIT_ASSERT_NOT_NULL_MSG(test, cleanup_func,
+				  "Cleanup function must not be NULL");
+
+	res = kzalloc(sizeof(*res), internal_gfp);
+	if (!res) {
+		cleanup_func(data);
+		KUNIT_ASSERT_FAILURE(test, "Could not allocate resource for cleanup");
+	}
+
+	res->cleanup_func = cleanup_func;
+	res->resource.should_kfree = true;
+
+	/* Cannot fail as init is NULL */
+	__kunit_add_resource(test, NULL, kunit_auto_cleanup_free,
+			     &res->resource, (void *)data);
+}
+EXPORT_SYMBOL_GPL(kunit_add_cleanup);
+
 static inline bool kunit_kfree_match(struct kunit *test,
 				     struct kunit_resource *res, void *match_data)
 {
-- 
2.39.2

Re: [PATCH 2/3] kunit: add ability to register a simple cleanup function

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 4209 bytes --]

On Thu, 30 Mar 2023 at 01:23, <benjamin@sipsolutions.net> wrote:
>
> From: Benjamin Berg <benjamin.berg@intel.com>
>
> This is useful to e.g. automatically free resources at the end of the
> test, without having to deal with kunit resource objects directly.
>
> The whole point of doing this is that the callback is guaranteed to
> happen in case the test aborts (due to an assertion). As such, use
> assertions internally rather than requiring further error checking by
> the API user.
>
> Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
> ---

Thanks!

I've actually been working on something similar here:
https://lore.kernel.org/linux-kselftest/20230325043104.3761770-2-davidgow@google.com/

Maxime suggested that it might be worth naming this kunit_add_action()
for consistency with devm_add_action(). We also need the cancel/remove
and trigger/release helpers for some of the other device helpers.

Hopefully between these, we can have something which works for everyone.

Cheers,
-- David




>  include/kunit/test.h | 20 ++++++++++++++++++++
>  lib/kunit/test.c     | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 519b90261c72..ab1dacf1c9f4 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -373,6 +373,26 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp
>         return kunit_kmalloc_array(test, n, size, gfp | __GFP_ZERO);
>  }
>
> +typedef void (*kunit_cleanup_t)(const void *);
> +
> +/**
> + * kunit_add_cleanup() - Add post-test cleanup action.
> + * @test: The test case to which the resource belongs.
> + * @cleanup_func: function to call at end of test.
> + * @data: data to pass to @free_func.
> + * @internal_gfp: gfp to use for internal allocations, if unsure, use GFP_KERNEL
> + *
> + * This adds a cleanup action to be executed after the test completes.
> + * Internally this is handled using a *test managed resource*.
> + *
> + * This function will abort the test on failure.
> + *
> + * Note: KUnit needs to allocate memory for a kunit_resource object. You must
> + * specify an @internal_gfp that is compatible with the current context.
> + */
> +void kunit_add_cleanup(struct kunit *test, kunit_cleanup_t cleanup_func,
> +                      const void *data, gfp_t internal_gfp);
> +
>  void kunit_cleanup(struct kunit *test);
>
>  void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index c9e15bb60058..72d956dfc324 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -719,6 +719,43 @@ void *kunit_kmalloc_array(struct kunit *test, size_t n, size_t size, gfp_t gfp)
>  }
>  EXPORT_SYMBOL_GPL(kunit_kmalloc_array);
>
> +struct kunit_auto_cleanup {
> +       struct kunit_resource resource;
> +       kunit_cleanup_t cleanup_func;
> +};
> +
> +static void kunit_auto_cleanup_free(struct kunit_resource *res)
> +{
> +       struct kunit_auto_cleanup *cleanup;
> +
> +       cleanup = container_of(res, struct kunit_auto_cleanup, resource);
> +
> +       cleanup->cleanup_func(cleanup->resource.data);
> +}
> +
> +void kunit_add_cleanup(struct kunit *test, kunit_cleanup_t cleanup_func,
> +                      const void *data, gfp_t internal_gfp)
> +{
> +       struct kunit_auto_cleanup *res;
> +
> +       KUNIT_ASSERT_NOT_NULL_MSG(test, cleanup_func,
> +                                 "Cleanup function must not be NULL");
> +
> +       res = kzalloc(sizeof(*res), internal_gfp);
> +       if (!res) {
> +               cleanup_func(data);
> +               KUNIT_ASSERT_FAILURE(test, "Could not allocate resource for cleanup");
> +       }
> +
> +       res->cleanup_func = cleanup_func;
> +       res->resource.should_kfree = true;
> +
> +       /* Cannot fail as init is NULL */
> +       __kunit_add_resource(test, NULL, kunit_auto_cleanup_free,
> +                            &res->resource, (void *)data);
> +}
> +EXPORT_SYMBOL_GPL(kunit_add_cleanup);
> +
>  static inline bool kunit_kfree_match(struct kunit *test,
>                                      struct kunit_resource *res, void *match_data)
>  {
> --
> 2.39.2
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH 2/3] kunit: add ability to register a simple cleanup function

[Benjamin Berg]

On Thu, 2023-03-30 at 14:17 +0800, David Gow wrote:
> On Thu, 30 Mar 2023 at 01:23, <benjamin@sipsolutions.net> wrote:
> > 
> > From: Benjamin Berg <benjamin.berg@intel.com>
> > 
> > This is useful to e.g. automatically free resources at the end of the
> > test, without having to deal with kunit resource objects directly.
> > 
> > The whole point of doing this is that the callback is guaranteed to
> > happen in case the test aborts (due to an assertion). As such, use
> > assertions internally rather than requiring further error checking by
> > the API user.
> > 
> > Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
> > ---
> 
> Thanks!
> 
> I've actually been working on something similar here:
> https://lore.kernel.org/linux-kselftest/20230325043104.3761770-2-davidgow@google.com/

Oh, neat!

> Maxime suggested that it might be worth naming this kunit_add_action()
> for consistency with devm_add_action(). We also need the cancel/remove
> and trigger/release helpers for some of the other device helpers.

With kunit_add_cleanup I simply chose to copy the python unittest
API[1], which may already be familiar to people and feels quite
descriptive to me.


Something to maybe talk about is whether my approach of simply
asserting instead of returning an error is desirable. In my view, tests
should be fine with being aborted in most situations and the test
itself is never going to succeed after an allocation failure. I had a
quick look at kunit_kzalloc, and doing an assertion right afterwards is
a very common pattern. But, even if it is similar to e.g. userspace
GLib's g_malloc, it would be a new behaviour for the kernel.

Benjamin

[1] https://docs.python.org/3/library/unittest.html#unittest.TestCase.addCleanup

> Hopefully between these, we can have something which works for everyone.
> 
> Cheers,
> -- David
> 
> 
> 
> 
> >  include/kunit/test.h | 20 ++++++++++++++++++++
> >  lib/kunit/test.c     | 37 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 57 insertions(+)
> > 
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 519b90261c72..ab1dacf1c9f4 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -373,6 +373,26 @@ static inline void *kunit_kcalloc(struct kunit
> > *test, size_t n, size_t size, gfp
> >         return kunit_kmalloc_array(test, n, size, gfp |
> > __GFP_ZERO);
> >  }
> > 
> > +typedef void (*kunit_cleanup_t)(const void *);
> > +
> > +/**
> > + * kunit_add_cleanup() - Add post-test cleanup action.
> > + * @test: The test case to which the resource belongs.
> > + * @cleanup_func: function to call at end of test.
> > + * @data: data to pass to @free_func.
> > + * @internal_gfp: gfp to use for internal allocations, if unsure,
> > use GFP_KERNEL
> > + *
> > + * This adds a cleanup action to be executed after the test
> > completes.
> > + * Internally this is handled using a *test managed resource*.
> > + *
> > + * This function will abort the test on failure.
> > + *
> > + * Note: KUnit needs to allocate memory for a kunit_resource
> > object. You must
> > + * specify an @internal_gfp that is compatible with the current
> > context.
> > + */
> > +void kunit_add_cleanup(struct kunit *test, kunit_cleanup_t
> > cleanup_func,
> > +                      const void *data, gfp_t internal_gfp);
> > +
> >  void kunit_cleanup(struct kunit *test);
> > 
> >  void __printf(2, 3) kunit_log_append(char *log, const char *fmt,
> > ...);
> > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > index c9e15bb60058..72d956dfc324 100644
> > --- a/lib/kunit/test.c
> > +++ b/lib/kunit/test.c
> > @@ -719,6 +719,43 @@ void *kunit_kmalloc_array(struct kunit *test,
> > size_t n, size_t size, gfp_t gfp)
> >  }
> >  EXPORT_SYMBOL_GPL(kunit_kmalloc_array);
> > 
> > +struct kunit_auto_cleanup {
> > +       struct kunit_resource resource;
> > +       kunit_cleanup_t cleanup_func;
> > +};
> > +
> > +static void kunit_auto_cleanup_free(struct kunit_resource *res)
> > +{
> > +       struct kunit_auto_cleanup *cleanup;
> > +
> > +       cleanup = container_of(res, struct kunit_auto_cleanup,
> > resource);
> > +
> > +       cleanup->cleanup_func(cleanup->resource.data);
> > +}
> > +
> > +void kunit_add_cleanup(struct kunit *test, kunit_cleanup_t
> > cleanup_func,
> > +                      const void *data, gfp_t internal_gfp)
> > +{
> > +       struct kunit_auto_cleanup *res;
> > +
> > +       KUNIT_ASSERT_NOT_NULL_MSG(test, cleanup_func,
> > +                                 "Cleanup function must not be
> > NULL");
> > +
> > +       res = kzalloc(sizeof(*res), internal_gfp);
> > +       if (!res) {
> > +               cleanup_func(data);
> > +               KUNIT_ASSERT_FAILURE(test, "Could not allocate
> > resource for cleanup");
> > +       }
> > +
> > +       res->cleanup_func = cleanup_func;
> > +       res->resource.should_kfree = true;
> > +
> > +       /* Cannot fail as init is NULL */
> > +       __kunit_add_resource(test, NULL, kunit_auto_cleanup_free,
> > +                            &res->resource, (void *)data);
> > +}
> > +EXPORT_SYMBOL_GPL(kunit_add_cleanup);
> > +
> >  static inline bool kunit_kfree_match(struct kunit *test,
> >                                      struct kunit_resource *res,
> > void *match_data)
> >  {
> > --
> > 2.39.2
> > 

Re: [PATCH 2/3] kunit: add ability to register a simple cleanup function

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on wireless-next/main]
[also build test WARNING on wireless/main linus/master v6.3-rc6 next-20230411]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/benjamin-sipsolutions-net/kunit-add-ability-to-register-a-simple-cleanup-function/20230330-012515
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
patch link:    https://lore.kernel.org/r/20230329172311.71861-2-benjamin%40sipsolutions.net
patch subject: [PATCH 2/3] kunit: add ability to register a simple cleanup function
config: arc-randconfig-m031-20230410 (https://download.01.org/0day-ci/archive/20230411/202304112307.LAyTVYon-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304112307.LAyTVYon-lkp@intel.com/

New smatch warnings:
lib/kunit/test.c:750 kunit_add_cleanup() error: potential null dereference 'res'.  (kzalloc returns null)
lib/kunit/test.c:750 kunit_add_cleanup() error: we previously assumed 'res' could be null (see line 745)

Old smatch warnings:
lib/kunit/test.c:293 kunit_abort() warn: ignoring unreachable code.

vim +/res +750 lib/kunit/test.c

   735	
   736	void kunit_add_cleanup(struct kunit *test, kunit_cleanup_t cleanup_func,
   737			       const void *data, gfp_t internal_gfp)
   738	{
   739		struct kunit_auto_cleanup *res;
   740	
   741		KUNIT_ASSERT_NOT_NULL_MSG(test, cleanup_func,
   742					  "Cleanup function must not be NULL");
   743	
   744		res = kzalloc(sizeof(*res), internal_gfp);
 > 745		if (!res) {
   746			cleanup_func(data);
   747			KUNIT_ASSERT_FAILURE(test, "Could not allocate resource for cleanup");
   748		}
   749	
 > 750		res->cleanup_func = cleanup_func;
   751		res->resource.should_kfree = true;
   752	
   753		/* Cannot fail as init is NULL */
   754		__kunit_add_resource(test, NULL, kunit_auto_cleanup_free,
   755				     &res->resource, (void *)data);
   756	}
   757	EXPORT_SYMBOL_GPL(kunit_add_cleanup);
   758	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

[PATCH 3/3] kunit: add a convenience allocation wrapper for SKBs

[benjamin]

From: Benjamin Berg <benjamin.berg@intel.com>

This simplifies the use of SKBs in tests by avoiding the need for
error checking.

Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
---
 include/kunit/skbuff.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 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..46dcb00af655
--- /dev/null
+++ b/include/kunit/skbuff.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * KUnit struct sk_buff helpers.
+ *
+ * Copyright (C) 2023 Intel Corporation
+ */
+
+#ifndef _KUNIT_SKBUFF_H
+#define _KUNIT_SKBUFF_H
+
+#include <kunit/test.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
+ * @gfp: allocation mask
+ *
+ * Allocate a new struct sk_buff, zero fill the give length and add it as a
+ * resource to the kunit test for automatic cleanup.
+ *
+ * The function will not return in case of an allocation 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);
+
+	KUNIT_ASSERT_NOT_NULL(test, res);
+	KUNIT_ASSERT_EQ(test, skb_pad(res, len), 0);
+
+	kunit_add_cleanup(test, (kunit_cleanup_t) kfree_skb, res, gfp);
+
+	return res;
+}
+
+#endif /* _KUNIT_SKBUFF_H */
-- 
2.39.2