[PATCH 0/4] typesafe callbacks

[PATCH 0/4] typesafe callbacks

[Rusty Russell]

It's possible to portably make callbacks typesafe, as well as taking the 
existing void * arg.  This makes the callers more readable and more typesafe.

Done in a way that doesn't break non-gcc: they just don't get a warning.

Been in linux-next for about two full cycles now, and I really want to start 
using this in my own code.

Thanks,
Rusty.

[PATCH 1/4] typesafe: cast_if_type: allow macros functions which take more than one type.

[Rusty Russell]

To create functions which can take two types, but still warn on any
other types, we need a way of casting one type and no others.

To make things more complex, it should correctly handle function args,
NULL, and be usable in initializers.  __builtin_choose_expr was
introduced in gcc 3.1 (kernel needs >= 3.2 anyway).

(includes Harvey Harrison <harvey.harrison@gmail.com>'s sparse fix)

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: Harvey Harrison <harvey.harrison@gmail.com>
---
 include/linux/compiler-gcc.h   |   18 ++++++++++++++++++
 include/linux/compiler-intel.h |    2 ++
 2 files changed, 20 insertions(+)

diff -r ecbc022961d8 include/linux/compiler-gcc.h
--- a/include/linux/compiler-gcc.h	Mon Jul 14 12:30:35 2008 +1000
+++ b/include/linux/compiler-gcc.h	Mon Jul 14 20:46:23 2008 +1000
@@ -61,3 +61,21 @@
 #define  noinline			__attribute__((noinline))
 #define __attribute_const__		__attribute__((__const__))
 #define __maybe_unused			__attribute__((unused))
+
+/**
+ * cast_if_type - allow an alternate type
+ * @expr: the expression to optionally cast
+ * @oktype: the type to allow.
+ * @desttype: the type to cast to.
+ *
+ * This is used to accept a particular alternate type for an expression:
+ * because any other types will not be cast, they will cause a warning as
+ * normal.
+ *
+ * Note that the unnecessary trinary forces functions to devolve into
+ * function pointers as users expect, but means @expr must be a pointer or
+ * integer.
+ */
+#define cast_if_type(expr, oktype, desttype)	__builtin_choose_expr(	\
+	__builtin_types_compatible_p(typeof(1?(expr):(expr)), oktype),	\
+	(desttype)(expr), (expr))
diff -r ecbc022961d8 include/linux/compiler-intel.h
--- a/include/linux/compiler-intel.h	Mon Jul 14 12:30:35 2008 +1000
+++ b/include/linux/compiler-intel.h	Mon Jul 14 20:46:23 2008 +1000
@@ -29,3 +29,5 @@
 #endif
 
 #define uninitialized_var(x) x
+
+#define cast_if_type(expr, oktype, desttype) ((desttype)(expr))

[PATCH 2/4] typesafe: typesafe_cb: wrappers for typesafe callbacks.

[Rusty Russell]

cast_if_type is clever but annoying to use, so we make some slightly
less annoying wrappers for callback registration.

These allow const pointer-taking functions as well as the normal
non-const ones (volatile variant omitted for simplicity).  We also
have a typesafe_cb_preargs() for callbacks which don't just take one
argument.

Here's an example of use:

BEFORE:
	void foo_set_callback(int (*callback)(void *), void *arg);

AFTER:
	#define foo_set_callback(cb, arg) \
		__foo_set_callback(typesafe_cb(int, (cb), (arg)), (arg))
	void __foo_set_callback(int (*callback)(void *), void *arg);

o If cb is of form "int cb(typeof(arg))" or "int cb(const typeof(arg))" it
  will be cast to form "int cb(void *)".
o Otherwise, if cb is not already of form "int cb(void *)" it will cause a
  warning when passed to __foo_set_callback().
o If arg is not a pointer, then handing it to __foo_set_callback() will cause
  a warning.

Note: there are at least two possible additions we don't yet need.  We
don't cover callbacks which take a volatile pointer, even though that
would be fine, and we don't have a typesafe_cb_postargs().

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/kernel.h |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff -r 907c55be656f include/linux/kernel.h
--- a/include/linux/kernel.h	Mon Apr 21 06:58:00 2008 +1000
+++ b/include/linux/kernel.h	Mon Apr 21 07:04:02 2008 +1000
@@ -436,4 +436,39 @@ struct sysinfo {
 #define NUMA_BUILD 0
 #endif
 
+/* If fn is of type ok1 or ok2, cast to desttype */
+#define __typesafe_cb(desttype, fn, ok1, ok2) \
+	cast_if_type(cast_if_type((fn), ok1, desttype), ok2, desttype)
+
+/**
+ * typesafe_cb - cast a callback function if it matches the arg
+ * @rettype: the return type of the callback function
+ * @fn: the callback function to cast
+ * @arg: the (pointer) argument to hand to the callback function.
+ *
+ * If a callback function takes a single argument, this macro does
+ * appropriate casts to a function which takes a single void * argument if the
+ * callback provided matches the @arg (or a const or volatile version).
+ *
+ * It is assumed that @arg is of pointer type: usually @arg is passed
+ * or assigned to a void * elsewhere anyway.
+ */
+#define typesafe_cb(rettype, fn, arg)					\
+	__typesafe_cb(rettype (*)(void *), (fn),			\
+		      rettype (*)(const typeof(arg)),			\
+		      rettype (*)(typeof(arg)))
+
+/**
+ * typesafe_cb_preargs - cast a callback function if it matches the arg
+ * @rettype: the return type of the callback function
+ * @fn: the callback function to cast
+ * @arg: the (pointer) argument to hand to the callback function.
+ *
+ * This is a version of typesafe_cb() for callbacks that take other arguments
+ * before the @arg.
+ */
+#define typesafe_cb_preargs(rettype, fn, arg, ...)			\
+	__typesafe_cb(rettype (*)(__VA_ARGS__, void *), (fn),		\
+		      rettype (*)(__VA_ARGS__, const typeof(arg)),	\
+		      rettype (*)(__VA_ARGS__, typeof(arg)))
 #endif

[PATCH 3/4] typesafe: kthread_create and kthread_run

[Rusty Russell]

(Assumes patch to add printf attr to kthread_create is applied, but fixup trivial)

Straight-forward conversion to allow typesafe callbacks.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/kthread.h |   28 +++++++++++++++++++++++++---
 kernel/kthread.c        |   29 +++++------------------------
 2 files changed, 30 insertions(+), 27 deletions(-)

diff -r f5a7f7f9683c include/linux/kthread.h
--- a/include/linux/kthread.h	Mon Apr 07 18:36:20 2008 +1000
+++ b/include/linux/kthread.h	Mon Apr 07 19:12:04 2008 +1000
@@ -4,9 +4,31 @@
 #include <linux/err.h>
 #include <linux/sched.h>
 
-struct task_struct *kthread_create(int (*threadfn)(void *data),
-				   void *data,
-				   const char namefmt[], ...)
+/**
+ * kthread_create - create a kthread.
+ * @threadfn: the function to run until signal_pending(current).
+ * @data: data ptr for @threadfn.
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: This helper function creates and names a kernel
+ * thread.  The thread will be stopped: use wake_up_process() to start
+ * it.  See also kthread_run(), kthread_create_on_cpu().
+ *
+ * When woken, the thread will run @threadfn() with @data as its
+ * argument. @threadfn() can either call do_exit() directly if it is a
+ * standalone thread for which noone will call kthread_stop(), or
+ * return when 'kthread_should_stop()' is true (which means
+ * kthread_stop() has been called).  The return value should be zero
+ * or a negative error number; it will be passed to kthread_stop().
+ *
+ * Returns a task_struct or ERR_PTR(-ENOMEM).
+ */
+#define kthread_create(threadfn, data, namefmt...)			\
+	__kthread_create(typesafe_cb(int,(threadfn),(data)), (data), namefmt)
+
+struct task_struct *__kthread_create(int (*threadfn)(void *data),
+				     void *data,
+				     const char namefmt[], ...)
 	__attribute__((format(printf, 3, 4)));
 
 /**
diff -r f5a7f7f9683c kernel/kthread.c
--- a/kernel/kthread.c	Mon Apr 07 18:36:20 2008 +1000
+++ b/kernel/kthread.c	Mon Apr 07 19:12:04 2008 +1000
@@ -112,29 +112,10 @@ static void create_kthread(struct kthrea
 	complete(&create->done);
 }
 
-/**
- * kthread_create - create a kthread.
- * @threadfn: the function to run until signal_pending(current).
- * @data: data ptr for @threadfn.
- * @namefmt: printf-style name for the thread.
- *
- * Description: This helper function creates and names a kernel
- * thread.  The thread will be stopped: use wake_up_process() to start
- * it.  See also kthread_run(), kthread_create_on_cpu().
- *
- * When woken, the thread will run @threadfn() with @data as its
- * argument. @threadfn() can either call do_exit() directly if it is a
- * standalone thread for which noone will call kthread_stop(), or
- * return when 'kthread_should_stop()' is true (which means
- * kthread_stop() has been called).  The return value should be zero
- * or a negative error number; it will be passed to kthread_stop().
- *
- * Returns a task_struct or ERR_PTR(-ENOMEM).
- */
-struct task_struct *kthread_create(int (*threadfn)(void *data),
-				   void *data,
-				   const char namefmt[],
-				   ...)
+struct task_struct *__kthread_create(int (*threadfn)(void *data),
+				     void *data,
+				     const char namefmt[],
+				     ...)
 {
 	struct kthread_create_info create;
 
@@ -159,7 +140,7 @@ struct task_struct *kthread_create(int (
 	}
 	return create.result;
 }
-EXPORT_SYMBOL(kthread_create);
+EXPORT_SYMBOL(__kthread_create);
 
 /**
  * kthread_bind - bind a just-created kthread to a cpu.

[PATCH 4/4] typesafe: TIMER_INITIALIZER and setup_timer

[Rusty Russell]

This patch lets timer callback functions have their natural type
(ie. exactly match the data pointer type); it allows the old "unsigned
long data" type as well.

Downside: if you use the old "unsigned long" callback type, you won't
get a warning if your data is not an unsigned long, due to the cast.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/timer.h |   32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff -r 138795de70da include/linux/timer.h
--- a/include/linux/timer.h	Thu May 01 21:03:51 2008 +1000
+++ b/include/linux/timer.h	Thu May 01 21:05:13 2008 +1000
@@ -25,12 +25,22 @@ struct timer_list {
 
 extern struct tvec_base boot_tvec_bases;
 
-#define TIMER_INITIALIZER(_function, _expires, _data) {		\
-		.entry = { .prev = TIMER_ENTRY_STATIC },	\
-		.function = (_function),			\
-		.expires = (_expires),				\
-		.data = (_data),				\
-		.base = &boot_tvec_bases,			\
+/*
+ * For historic reasons the timer function takes an unsigned long, so
+ * we use this variant of typesafe_cb.  data is converted to an unsigned long
+ * if it is another integer type, by adding 0UL.
+ */
+#define typesafe_timerfn(fn, data)				\
+	__typesafe_cb(void (*)(unsigned long), (fn),		\
+		      void (*)(const typeof((data)+0UL)),	\
+		      void (*)(typeof((data)+0UL)))
+
+#define TIMER_INITIALIZER(_function, _expires, _data) {			\
+		.entry = { .prev = TIMER_ENTRY_STATIC },		\
+		.function = typesafe_timerfn((_function), (_data)),	\
+		.expires = (_expires),					\
+		.data = (unsigned long)(_data),				\
+		.base = &boot_tvec_bases,				\
 	}
 
 #define DEFINE_TIMER(_name, _function, _expires, _data)		\
@@ -51,9 +61,13 @@ static inline void init_timer_on_stack(s
 }
 #endif
 
-static inline void setup_timer(struct timer_list * timer,
-				void (*function)(unsigned long),
-				unsigned long data)
+#define setup_timer(timer, function, data)				\
+	__setup_timer((timer), typesafe_timerfn((function), (data)),	\
+		      (unsigned long)(data))
+
+static inline void __setup_timer(struct timer_list *timer,
+				 void (*function)(unsigned long),
+				 unsigned long data)
 {
 	timer->function = function;
 	timer->data = data;