diff mbox series

[RFC,kunit-next,2/2] kunit: add support for named resources

Message ID 1583251361-12748-3-git-send-email-alan.maguire@oracle.com
State New
Headers show
Series kunit: extend kunit resources API | expand

Commit Message

Alan Maguire March 3, 2020, 4:02 p.m. UTC
The kunit resources API allows for custom initialization and
cleanup code (init/fini); here we use the init code to set the
new "struct kunit_resource" "name" field, and call an additional
init function if needed.  Having a simple way to name resources
is useful in cases such as multithreaded tests where a set of
resources are shared among threads; a pointer to the
"struct kunit *" test state then is all that is needed to
retrieve and use named resources.  Support is provided to add,
find and destroy named resources; the latter two are simply
wrappers that use a "match-by-name" callback.

If an attempt to add a resource with a name that already exists
is made kunit_add_named_resource() will return NULL.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 include/kunit/test.h   | 40 ++++++++++++++++++++++++++++++++++++++-
 lib/kunit/kunit-test.c | 37 ++++++++++++++++++++++++++++++++++++
 lib/kunit/test.c       | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 1 deletion(-)

Comments

Brendan Higgins March 13, 2020, 12:09 a.m. UTC | #1
On Tue, Mar 3, 2020 at 8:03 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> The kunit resources API allows for custom initialization and
> cleanup code (init/fini); here we use the init code to set the
> new "struct kunit_resource" "name" field, and call an additional
> init function if needed.  Having a simple way to name resources
> is useful in cases such as multithreaded tests where a set of
> resources are shared among threads; a pointer to the
> "struct kunit *" test state then is all that is needed to
> retrieve and use named resources.  Support is provided to add,
> find and destroy named resources; the latter two are simply
> wrappers that use a "match-by-name" callback.
>
> If an attempt to add a resource with a name that already exists
> is made kunit_add_named_resource() will return NULL.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>

Overall, I think this seems reasonable. I think it needs a use case to
be justified, so long as Patricia ends up using it.

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

> ---
>  include/kunit/test.h   | 40 ++++++++++++++++++++++++++++++++++++++-
>  lib/kunit/kunit-test.c | 37 ++++++++++++++++++++++++++++++++++++
>  lib/kunit/test.c       | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 127 insertions(+), 1 deletion(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 11c80f5..70ee581 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -73,9 +73,14 @@
>   *                     kunit_kmalloc_free, &params);
>   *     }
>   *
> + * Resources can also be named, with lookup/removal done on a name
> + * basis also.  kunit_add_named_resource(), kunit_find_named_resource()
> + * and kunit_destroy_named_resource() below.  Resource names must be
> + * unique within the test instance.
>   */
>  struct kunit_resource {
>         void *data;
> +       const char *name;       /* optional name */
>         kunit_resource_init_t init;
>         kunit_resource_free_t free;
>
> @@ -275,12 +280,27 @@ struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
>   * @init: a user-supplied function to initialize the result (if needed).  If
>   *        none is supplied, the resource data value is simply set to @data.
>   *       If an init function is supplied, @data is passed to it instead.
> - * @free: a user-supplied function to free the resource (if needed).
> + * @free: a user-supplied function to free the resource data (if needed).
>   * @data: value to pass to init function or set in resource data field.
>   */
>  int kunit_add_resource(struct kunit *test, kunit_resource_init_t init,
>                        kunit_resource_free_t free, struct kunit_resource *res,
>                        void *data);
> +
> +/**
> + * kunit_add_named_resource() - Add a named *test managed resource*.
> + * @test: The test context object.
> + * @init: a user-supplied function to initialize the resource data, if needed.
> + * @free: a user-supplied function to free the resource data, if needed.
> + * @name_data: name and data to be set for resource.
> + */
> +int kunit_add_named_resource(struct kunit *test,
> +                            kunit_resource_init_t init,
> +                            kunit_resource_free_t free,
> +                            struct kunit_resource *res,
> +                            const char *name,
> +                            void *data);
> +
>  /**
>   * kunit_alloc_resource() - Allocates a *test managed resource*.
>   * @test: The test context object.
> @@ -336,6 +356,19 @@ static inline bool kunit_resource_instance_match(struct kunit *test,
>  }
>
>  /**
> + * kunit_resource_name_match() - Match a resource with the same name.
> + * @test: Test case to which the resource belongs.
> + * @res: The resource.
> + * @match_name: The name to match against.
> + */
> +static inline bool kunit_resource_name_match(struct kunit *test,
> +                                            struct kunit_resource *res,
> +                                            void *match_name)
> +{
> +       return res->name && strcmp(res->name, match_name) == 0;
> +}
> +
> +/**
>   * kunit_find_resource() - Find a resource using match function/data.
>   * @test: Test case to which the resource belongs.
>   * @match: match function to be applied to resources/match data.
> @@ -345,6 +378,9 @@ struct kunit_resource *kunit_find_resource(struct kunit *test,
>                                            kunit_resource_match_t match,
>                                            void *match_data);
>
> +#define kunit_find_named_resource(test, name)                  \
> +       kunit_find_resource(test, kunit_resource_name_match, (void *)name)
> +
>  /**
>   * kunit_destroy_resource() - Find a kunit_resource and destroy it.
>   * @test: Test case to which the resource belongs.
> @@ -358,6 +394,8 @@ int kunit_destroy_resource(struct kunit *test,
>                            kunit_resource_match_t match,
>                            void *match_data);
>
> +#define kunit_destroy_named_resource(test, name)               \
> +       kunit_destroy_resource(test, kunit_resource_name_match, name)

nit: I would prefer a static inline function here instead of a macro.

>  /**
>   * kunit_remove_resource: remove resource from resource list associated with
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index b8bf36d..079c558 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -317,6 +317,42 @@ static void kunit_resource_test_static(struct kunit *test)
>         KUNIT_EXPECT_TRUE(test, list_empty(&test->resources));
>  }
>
> +static void kunit_resource_test_named(struct kunit *test)
> +{
> +       struct kunit_resource res1, res2, *found = NULL;
> +       struct kunit_test_resource_context ctx;
> +
> +       KUNIT_EXPECT_EQ(test,
> +                       kunit_add_named_resource(test, NULL, NULL, &res1,
> +                                                "resource_1", &ctx),
> +                       0);
> +       KUNIT_EXPECT_PTR_EQ(test, res1.data, (void *)&ctx);
> +
> +       KUNIT_EXPECT_EQ(test,
> +                       kunit_add_named_resource(test, NULL, NULL, &res1,
> +                                                "resource_1", &ctx),
> +                       -EEXIST);
> +
> +       KUNIT_EXPECT_EQ(test,
> +                       kunit_add_named_resource(test, NULL, NULL, &res2,
> +                                                "resource_2", &ctx),
> +                       0);
> +
> +       found = kunit_find_named_resource(test, "resource_1");
> +
> +       KUNIT_EXPECT_PTR_EQ(test, found, &res1);
> +
> +       if (found)
> +               kunit_put_resource(&res1);
> +
> +       KUNIT_EXPECT_EQ(test, kunit_destroy_named_resource(test, "resource_2"),
> +                       0);
> +
> +       kunit_cleanup(test);
> +
> +       KUNIT_EXPECT_TRUE(test, list_empty(&test->resources));
> +}
> +
>  static int kunit_resource_test_init(struct kunit *test)
>  {
>         struct kunit_test_resource_context *ctx =
> @@ -346,6 +382,7 @@ static void kunit_resource_test_exit(struct kunit *test)
>         KUNIT_CASE(kunit_resource_test_cleanup_resources),
>         KUNIT_CASE(kunit_resource_test_proper_free_ordering),
>         KUNIT_CASE(kunit_resource_test_static),
> +       KUNIT_CASE(kunit_resource_test_named),
>         {}
>  };
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 132e9bf..86a4d9c 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -380,6 +380,57 @@ int kunit_add_resource(struct kunit *test, kunit_resource_init_t init,
>  }
>  EXPORT_SYMBOL_GPL(kunit_add_resource);
>
> +/* Used to initialize named resource. */
> +struct kunit_name_data {
> +       const char *name;
> +       kunit_resource_init_t init;
> +       void *data;
> +};
> +
> +static int kunit_init_named_resource(struct kunit_resource *res, void *data)
> +{
> +       struct kunit_name_data *name_data = data;
> +
> +       res->name = name_data->name;
> +       res->data = name_data->data;
> +       res->init = name_data->init;
> +
> +       if (res->init)
> +               return res->init(res, name_data->data);
> +
> +       res->data = name_data->data;
> +
> +       return 0;
> +}
> +
> +int kunit_add_named_resource(struct kunit *test,
> +                            kunit_resource_init_t init,
> +                            kunit_resource_free_t free,
> +                            struct kunit_resource *res,
> +                            const char *name,
> +                            void *data)
> +{
> +       struct kunit_name_data name_data;
> +       struct kunit_resource *existing;
> +
> +       if (!name)
> +               return -EINVAL;
> +
> +       existing = kunit_find_named_resource(test, name);
> +       if (existing) {
> +               kunit_put_resource(existing);
> +               return -EEXIST;
> +       }
> +
> +       name_data.name = name;
> +       name_data.init = init;
> +       name_data.data = data;
> +
> +       return kunit_add_resource(test, kunit_init_named_resource, free, res,
> +                                 &name_data);
> +}
> +EXPORT_SYMBOL_GPL(kunit_add_named_resource);

Could we maybe combine the above with the non-named variants? Seems
like there might be some unnecessary code duplication.

>  struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
>                                                     kunit_resource_init_t init,
>                                                     kunit_resource_free_t free,

Once again, thank you for all your hard work!
diff mbox series

Patch

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 11c80f5..70ee581 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -73,9 +73,14 @@ 
  *			kunit_kmalloc_free, &params);
  *	}
  *
+ * Resources can also be named, with lookup/removal done on a name
+ * basis also.  kunit_add_named_resource(), kunit_find_named_resource()
+ * and kunit_destroy_named_resource() below.  Resource names must be
+ * unique within the test instance.
  */
 struct kunit_resource {
 	void *data;
+	const char *name;	/* optional name */
 	kunit_resource_init_t init;
 	kunit_resource_free_t free;
 
@@ -275,12 +280,27 @@  struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
  * @init: a user-supplied function to initialize the result (if needed).  If
  *        none is supplied, the resource data value is simply set to @data.
  *	  If an init function is supplied, @data is passed to it instead.
- * @free: a user-supplied function to free the resource (if needed).
+ * @free: a user-supplied function to free the resource data (if needed).
  * @data: value to pass to init function or set in resource data field.
  */
 int kunit_add_resource(struct kunit *test, kunit_resource_init_t init,
 		       kunit_resource_free_t free, struct kunit_resource *res,
 		       void *data);
+
+/**
+ * kunit_add_named_resource() - Add a named *test managed resource*.
+ * @test: The test context object.
+ * @init: a user-supplied function to initialize the resource data, if needed.
+ * @free: a user-supplied function to free the resource data, if needed.
+ * @name_data: name and data to be set for resource.
+ */
+int kunit_add_named_resource(struct kunit *test,
+			     kunit_resource_init_t init,
+			     kunit_resource_free_t free,
+			     struct kunit_resource *res,
+			     const char *name,
+			     void *data);
+
 /**
  * kunit_alloc_resource() - Allocates a *test managed resource*.
  * @test: The test context object.
@@ -336,6 +356,19 @@  static inline bool kunit_resource_instance_match(struct kunit *test,
 }
 
 /**
+ * kunit_resource_name_match() - Match a resource with the same name.
+ * @test: Test case to which the resource belongs.
+ * @res: The resource.
+ * @match_name: The name to match against.
+ */
+static inline bool kunit_resource_name_match(struct kunit *test,
+					     struct kunit_resource *res,
+					     void *match_name)
+{
+	return res->name && strcmp(res->name, match_name) == 0;
+}
+
+/**
  * kunit_find_resource() - Find a resource using match function/data.
  * @test: Test case to which the resource belongs.
  * @match: match function to be applied to resources/match data.
@@ -345,6 +378,9 @@  struct kunit_resource *kunit_find_resource(struct kunit *test,
 					   kunit_resource_match_t match,
 					   void *match_data);
 
+#define kunit_find_named_resource(test, name)			\
+	kunit_find_resource(test, kunit_resource_name_match, (void *)name)
+
 /**
  * kunit_destroy_resource() - Find a kunit_resource and destroy it.
  * @test: Test case to which the resource belongs.
@@ -358,6 +394,8 @@  int kunit_destroy_resource(struct kunit *test,
 			   kunit_resource_match_t match,
 			   void *match_data);
 
+#define kunit_destroy_named_resource(test, name)		\
+	kunit_destroy_resource(test, kunit_resource_name_match, name)
 
 /**
  * kunit_remove_resource: remove resource from resource list associated with
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index b8bf36d..079c558 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -317,6 +317,42 @@  static void kunit_resource_test_static(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, list_empty(&test->resources));
 }
 
+static void kunit_resource_test_named(struct kunit *test)
+{
+	struct kunit_resource res1, res2, *found = NULL;
+	struct kunit_test_resource_context ctx;
+
+	KUNIT_EXPECT_EQ(test,
+			kunit_add_named_resource(test, NULL, NULL, &res1,
+						 "resource_1", &ctx),
+			0);
+	KUNIT_EXPECT_PTR_EQ(test, res1.data, (void *)&ctx);
+
+	KUNIT_EXPECT_EQ(test,
+			kunit_add_named_resource(test, NULL, NULL, &res1,
+						 "resource_1", &ctx),
+			-EEXIST);
+
+	KUNIT_EXPECT_EQ(test,
+			kunit_add_named_resource(test, NULL, NULL, &res2,
+						 "resource_2", &ctx),
+			0);
+
+	found = kunit_find_named_resource(test, "resource_1");
+
+	KUNIT_EXPECT_PTR_EQ(test, found, &res1);
+
+	if (found)
+		kunit_put_resource(&res1);
+
+	KUNIT_EXPECT_EQ(test, kunit_destroy_named_resource(test, "resource_2"),
+			0);
+
+	kunit_cleanup(test);
+
+	KUNIT_EXPECT_TRUE(test, list_empty(&test->resources));
+}
+
 static int kunit_resource_test_init(struct kunit *test)
 {
 	struct kunit_test_resource_context *ctx =
@@ -346,6 +382,7 @@  static void kunit_resource_test_exit(struct kunit *test)
 	KUNIT_CASE(kunit_resource_test_cleanup_resources),
 	KUNIT_CASE(kunit_resource_test_proper_free_ordering),
 	KUNIT_CASE(kunit_resource_test_static),
+	KUNIT_CASE(kunit_resource_test_named),
 	{}
 };
 
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 132e9bf..86a4d9c 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -380,6 +380,57 @@  int kunit_add_resource(struct kunit *test, kunit_resource_init_t init,
 }
 EXPORT_SYMBOL_GPL(kunit_add_resource);
 
+/* Used to initialize named resource. */
+struct kunit_name_data {
+	const char *name;
+	kunit_resource_init_t init;
+	void *data;
+};
+
+static int kunit_init_named_resource(struct kunit_resource *res, void *data)
+{
+	struct kunit_name_data *name_data = data;
+
+	res->name = name_data->name;
+	res->data = name_data->data;
+	res->init = name_data->init;
+
+	if (res->init)
+		return res->init(res, name_data->data);
+
+	res->data = name_data->data;
+
+	return 0;
+}
+
+int kunit_add_named_resource(struct kunit *test,
+			     kunit_resource_init_t init,
+			     kunit_resource_free_t free,
+			     struct kunit_resource *res,
+			     const char *name,
+			     void *data)
+{
+	struct kunit_name_data name_data;
+	struct kunit_resource *existing;
+
+	if (!name)
+		return -EINVAL;
+
+	existing = kunit_find_named_resource(test, name);
+	if (existing) {
+		kunit_put_resource(existing);
+		return -EEXIST;
+	}
+
+	name_data.name = name;
+	name_data.init = init;
+	name_data.data = data;
+
+	return kunit_add_resource(test, kunit_init_named_resource, free, res,
+				  &name_data);
+}
+EXPORT_SYMBOL_GPL(kunit_add_named_resource);
+
 struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
 						    kunit_resource_init_t init,
 						    kunit_resource_free_t free,