[PATCH 0/5] typesafe callbacks

[PATCH 0/5] typesafe callbacks

[Rusty Russell]

Hi all,

   More typesafety is usually good, and these patches apply that to some 
common callbacks, using a new conditional cast.

(I'm nominally on holiday for all of Feb, so my replies will be sporadic.  
Please sort this out amongst yourselves).

Cheers!
Rusty.

[PATCH 1/5] 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 (we need >= 3.2 anyway).

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/compiler-gcc.h   |   17 +++++++++++++++++
 include/linux/compiler-intel.h |    2 ++
 2 files changed, 19 insertions(+)

diff -r e6626cc7bdc2 include/linux/compiler-gcc.h
--- a/include/linux/compiler-gcc.h	Sun Jan 20 18:51:51 2008 +1100
+++ b/include/linux/compiler-gcc.h	Sun Jan 20 18:57:14 2008 +1100
@@ -53,3 +53,20 @@
 #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. */
+#define cast_if_type(expr, oktype, desttype)				\
+  __builtin_choose_expr(__builtin_types_compatible_p(typeof(1?(expr):NULL), \
+						     oktype),		\
+			(desttype)(expr), (expr))
diff -r e6626cc7bdc2 include/linux/compiler-intel.h
--- a/include/linux/compiler-intel.h	Sun Jan 20 18:51:51 2008 +1100
+++ b/include/linux/compiler-intel.h	Sun Jan 20 18:57:14 2008 +1100
@@ -29,3 +29,5 @@
 #endif
 
 #define uninitialized_var(x) x
+
+#define cast_if_type(expr, oktype, desttype) ((desttype)(expr))

[PATCH 2/5] typesafe: Convert stop_machine

[Rusty Russell]

Using cast_if_type() we can have a callback funciton either of the
exactly correct type to take "data", or to take a void *.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r e279190b7b43 include/linux/stop_machine.h
--- a/include/linux/stop_machine.h	Mon Jan 21 14:42:54 2008 +1100
+++ b/include/linux/stop_machine.h	Mon Jan 21 15:04:00 2008 +1100
@@ -5,9 +5,9 @@
    (and more).  So the "read" side to such a lock is anything which
    diables preeempt. */
 #include <linux/cpu.h>
+#include <linux/compiler.h>
 #include <asm/system.h>
 
-#if defined(CONFIG_STOP_MACHINE) && defined(CONFIG_SMP)
 /**
  * stop_machine_run: freeze the machine on all CPUs and run this function
  * @fn: the function to run
@@ -21,7 +21,12 @@
  *
  * This can be thought of as a very heavy write lock, equivalent to
  * grabbing every spinlock in the kernel. */
-int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu);
+#define stop_machine_run(fn, data, cpu)					\
+	stop_machine_run_notype(cast_if_type((fn), int(*)(typeof(data)), \
+					     int(*)(void *)), (data), (cpu))
+
+#if defined(CONFIG_STOP_MACHINE) && defined(CONFIG_SMP)
+int stop_machine_run_notype(int (*fn)(void *), void *data, unsigned int cpu);
 
 /**
  * __stop_machine_run: freeze the machine on all CPUs and run this function
@@ -38,8 +46,8 @@ struct task_struct *__stop_machine_run(i
 
 #else
 
-static inline int stop_machine_run(int (*fn)(void *), void *data,
-				   unsigned int cpu)
+static inline int stop_machine_run_notype(int (*fn)(void *), void *data,
+					  unsigned int cpu)
 {
 	int ret;
 	local_irq_disable();
diff -r e279190b7b43 kernel/stop_machine.c
--- a/kernel/stop_machine.c	Mon Jan 21 14:42:54 2008 +1100
+++ b/kernel/stop_machine.c	Mon Jan 21 15:04:00 2008 +1100
@@ -197,7 +197,7 @@ struct task_struct *__stop_machine_run(i
 	return p;
 }
 
-int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
+int stop_machine_run_notype(int (*fn)(void *), void *data, unsigned int cpu)
 {
 	struct task_struct *p;
 	int ret;

[PATCH 3/5] typesafe: kthread_create and kthread_run

[Rusty Russell]

Using cast_if_type() we can have a callback funciton either of the
exactly correct type to take "data", or to take a void *.

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

diff -r 5748be68d6ee include/linux/kthread.h
--- a/include/linux/kthread.h	Wed Jan 23 12:08:28 2008 +1100
+++ b/include/linux/kthread.h	Wed Jan 23 12:14:00 2008 +1100
@@ -4,9 +4,33 @@
 #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(cast_if_type((threadfn), int(*)(typeof(data)), \
+				      int(*)(void *)),			\
+			 (data), namefmt)
+
+struct task_struct *__kthread_create(int (*threadfn)(void *data),
+				     void *data,
+				     const char namefmt[], ...);
 
 /**
  * kthread_run - create and wake a thread.
diff -r 5748be68d6ee kernel/kthread.c
--- a/kernel/kthread.c	Wed Jan 23 12:08:28 2008 +1100
+++ b/kernel/kthread.c	Wed Jan 23 12:14:00 2008 +1100
@@ -102,29 +102,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;
 
@@ -149,7 +130,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/5] typesafe: request_irq and devm_request_irq

[Rusty Russell]

This patch lets interrupt handler functions have their natural type
(ie. exactly match the data pointer type); it allows the old irq_handler_t
type as well.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/interrupt.h |   17 +++++++++++++++--
 kernel/irq/devres.c       |   10 +++++-----
 kernel/irq/manage.c       |    6 +++---
 3 files changed, 23 insertions(+), 10 deletions(-)

diff -r 0fe1a980708b include/linux/interrupt.h
--- a/include/linux/interrupt.h	Thu Jan 17 14:48:56 2008 +1100
+++ b/include/linux/interrupt.h	Thu Jan 17 15:42:01 2008 +1100
@@ -69,13 +69,26 @@ struct irqaction {
 };
 
 extern irqreturn_t no_action(int cpl, void *dev_id);
-extern int __must_check request_irq(unsigned int, irq_handler_t handler,
+
+/* Typesafe version of request_irq.  Also takes old-style void * handlers. */
+#define request_irq(irq, handler, flags, name, dev_id)			\
+	__request_irq((irq),						\
+		      cast_if_type((handler), int (*)(int, typeof(dev_id)), \
+				   irq_handler_t),			\
+		      (flags), (name), (dev_id))
+
+extern int __must_check __request_irq(unsigned int, irq_handler_t handler,
 		       unsigned long, const char *, void *);
 extern void free_irq(unsigned int, void *);
 
 struct device;
 
-extern int __must_check devm_request_irq(struct device *dev, unsigned int irq,
+#define devm_request_irq(dev, irq, handler, flags, name, dev_id)	\
+	__devm_request_irq((dev), (irq),				\
+		      cast_if_type((handler), int (*)(int, typeof(dev_id)), \
+				   irq_handler_t),			\
+		      (flags), (name), (dev_id))
+extern int __must_check __devm_request_irq(struct device *dev, unsigned int irq,
 			    irq_handler_t handler, unsigned long irqflags,
 			    const char *devname, void *dev_id);
 extern void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id);
diff -r 0fe1a980708b kernel/irq/devres.c
--- a/kernel/irq/devres.c	Thu Jan 17 14:48:56 2008 +1100
+++ b/kernel/irq/devres.c	Thu Jan 17 15:42:01 2008 +1100
@@ -41,9 +41,9 @@ static int devm_irq_match(struct device 
  *	If an IRQ allocated with this function needs to be freed
  *	separately, dev_free_irq() must be used.
  */
-int devm_request_irq(struct device *dev, unsigned int irq,
-		     irq_handler_t handler, unsigned long irqflags,
-		     const char *devname, void *dev_id)
+int __devm_request_irq(struct device *dev, unsigned int irq,
+		       irq_handler_t handler, unsigned long irqflags,
+		       const char *devname, void *dev_id)
 {
 	struct irq_devres *dr;
 	int rc;
@@ -53,7 +53,7 @@ int devm_request_irq(struct device *dev,
 	if (!dr)
 		return -ENOMEM;
 
-	rc = request_irq(irq, handler, irqflags, devname, dev_id);
+	rc = __request_irq(irq, handler, irqflags, devname, dev_id);
 	if (rc) {
 		devres_free(dr);
 		return rc;
@@ -65,7 +65,7 @@ int devm_request_irq(struct device *dev,
 
 	return 0;
 }
-EXPORT_SYMBOL(devm_request_irq);
+EXPORT_SYMBOL(__devm_request_irq);
 
 /**
  *	devm_free_irq - free an interrupt
diff -r 0fe1a980708b kernel/irq/manage.c
--- a/kernel/irq/manage.c	Thu Jan 17 14:48:56 2008 +1100
+++ b/kernel/irq/manage.c	Thu Jan 17 15:42:01 2008 +1100
@@ -514,8 +514,8 @@ EXPORT_SYMBOL(free_irq);
  *	IRQF_SAMPLE_RANDOM	The interrupt can be used for entropy
  *
  */
-int request_irq(unsigned int irq, irq_handler_t handler,
-		unsigned long irqflags, const char *devname, void *dev_id)
+int __request_irq(unsigned int irq, irq_handler_t handler,
+		  unsigned long irqflags, const char *devname, void *dev_id)
 {
 	struct irqaction *action;
 	int retval;
@@ -576,4 +576,4 @@ int request_irq(unsigned int irq, irq_ha
 
 	return retval;
 }
-EXPORT_SYMBOL(request_irq);
+EXPORT_SYMBOL(__request_irq);

[PATCH 5/5] 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 |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff -r c53eb02af893 include/linux/timer.h
--- a/include/linux/timer.h	Thu Jan 31 17:27:22 2008 +1100
+++ b/include/linux/timer.h	Thu Jan 31 17:27:39 2008 +1100
@@ -24,11 +24,13 @@ struct timer_list {
 
 extern struct tvec_base boot_tvec_bases;
 
-#define TIMER_INITIALIZER(_function, _expires, _data) {		\
-		.function = (_function),			\
-		.expires = (_expires),				\
-		.data = (_data),				\
-		.base = &boot_tvec_bases,			\
+
+#define TIMER_INITIALIZER(_function, _expires, _data) {			\
+		.function = cast_if_type(_function, void (*)(typeof(_data)), \
+					 void (*)(unsigned long)),	\
+		.expires = (_expires),					\
+		.data = (unsigned long)(_data),				\
+		.base = &boot_tvec_bases,				\
 	}
 
 #define DEFINE_TIMER(_name, _function, _expires, _data)		\
@@ -38,9 +40,15 @@ void fastcall init_timer(struct timer_li
 void fastcall init_timer(struct timer_list * timer);
 void fastcall init_timer_deferrable(struct timer_list *timer);
 
-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),						\
+		      cast_if_type(function, void (*)(typeof(data)),	\
+				   void (*)(unsigned long)),		\
+		      (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;

Re: [PATCH 5/5] typesafe: TIMER_INITIALIZER and setup_timer

[Al Viro]

On Mon, Feb 04, 2008 at 11:19:44PM +1100, Rusty Russell wrote:
> 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.

No.  There's much saner way to do that and it does not involve any gccisms
at all.  I'd posted such patches quite a while ago; normal C constructs
are quite sufficient, TYVM.

We don't _need_ that 'allow the old "unsigned long data"' junk.  There's
not enough users of setup_timer() to excuse it; other instances can and
should be converted to setup_timer() one by one, getting their types switched
to sanity as we go.

Let's not mix type safety with that ugliness, please.

Re: [PATCH 5/5] typesafe: TIMER_INITIALIZER and setup_timer

[Rusty Russell]

On Tuesday 05 February 2008 01:57:37 Al Viro wrote:
> On Mon, Feb 04, 2008 at 11:19:44PM +1100, Rusty Russell wrote:
> > 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.
>
> No.  There's much saner way to do that and it does not involve any gccisms
> at all.  I'd posted such patches quite a while ago; normal C constructs
> are quite sufficient, TYVM.

If you're referring to your 1 Dec 2006 posting, it was enlightening, but I was 
unable to find any patches.

I'd be interested in seeing what you ended up with; I agree it'd be nice to 
kill that unsigned long.

Thanks,
Rusty.

Re: [PATCH 5/5] typesafe: TIMER_INITIALIZER and setup_timer

[Rusty Russell]

On Tuesday 05 February 2008 14:41:53 Rusty Russell wrote:
> On Tuesday 05 February 2008 01:57:37 Al Viro wrote:
> > On Mon, Feb 04, 2008 at 11:19:44PM +1100, Rusty Russell wrote:
> > > 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.
> >
> > No.  There's much saner way to do that and it does not involve any
> > gccisms at all.  I'd posted such patches quite a while ago; normal C
> > constructs are quite sufficient, TYVM.
>
> If you're referring to your 1 Dec 2006 posting, it was enlightening, but I
> was unable to find any patches.

No reply?  I'll try again now.

> > No.  There's much saner way to do that and it does not involve any
> > gccisms at all.

Except typeof, you mean?

> > I'd posted such patches quite a while ago

AFAICT you posted an untested code fragment which won't actually compile 
because the callback in this case returns void.

> > ; normal C constructs are quite sufficient, TYVM.

Your code didn't check the return type, except that it's a pointer or 
int-compatible.  We can do better, as this patch showed.

If you have a "saner way" of doing this in general which is just as effective, 
I look forward to it.  Otherwise you're just managed to delay these 
improvements for another release.

Rusty.

Re: [PATCH 5/5] typesafe: TIMER_INITIALIZER and setup_timer

[Al Viro]

On Wed, Mar 05, 2008 at 01:55:36PM +1100, Rusty Russell wrote:

> AFAICT you posted an untested code fragment which won't actually compile 
> because the callback in this case returns void.

Callbacks for timers *do* return void.  FWIW,

extern void decay_to_pointer(void const volatile *);
#define check_callback_type(f,x) (sizeof((0 ? decay_to_pointer(x),(f)(x) : (void)0), 0))
#define __setup_timer(timer, func, arg) \
do {    \
	struct timer_list *__timer = (timer);   \
	__timer->function = (void (*)(unsigned long))(func); \
	__timer->data = (unsigned long)(arg);   \
	(void)check_callback_type(func, arg); \
} while(0)

#define setup_timer(timer, func, arg) \
do {    \
	struct timer_list *__timer = (timer);   \
	__timer->function = (void (*)(unsigned long))(func); \
	__timer->data = (unsigned long)(arg);   \
	init_timer(__timer);    \
	(void)check_callback_type(func, arg); \
} while(0)

#define DECLARE_TIMER(_name, _function, _data) \
        struct timer_list _name = { \
                .base = &boot_tvec_bases, \
                .function = (void (*)(unsigned long))(_function), \
                .data = (unsigned long)(_data) + \
                        0 * check_callback_type(_function, _data) \
        }

is what the timer series ended up using.  Note that this is _after_
conversion of setup_timer() users to natural argument types.  And yes,
it went timer-by-timer.
 
> > > ; normal C constructs are quite sufficient, TYVM.
> 
> Your code didn't check the return type, except that it's a pointer or 
> int-compatible.

Huh?  It certainly does _not_ accept return of int or pointer - ? : with
(void) 0 in the other branch is there to give a warning on those.  Note
that any timer callback that does anything of that kind is simply bogus -
there is nothing the caller could do about whatever return values we might
have, anyway.

> We can do better, as this patch showed.

> If you have a "saner way" of doing this in general which is just as effective, 
> I look forward to it.  Otherwise you're just managed to delay these 
> improvements for another release.

See above.  The point is, we don't bloody need to convert them all at once
and we can eliminate void (unsigned long) ones as we go.

Let me put it that way: if you actually look at these suckers, you'll see
that there's less than a dozen instances that actually want an integer.
Out of several hundreds.  And out of those only two, IIRC, were not trivially
converted by "pass pointer to array element instead of index in array".
Something in arch/sparc and arch/sparc64 - I can dig the details out if
you wish, but (a) it'll obviously need search from scratch to verify that
new pathology didn't show up and (b) it's extremely unlikely that the total
amount had become considerable.

So for timer callbacks the right answer is "just convert them to pointers,
these two cases are not worth the trouble".  And we _can_ do that in bisectable
way, TYVM - it's not that we can't do conversion stepwise.

A _very_ long series (we've a lot of these suckers) and I had two of its
incarnations bitrot on me by now, but it's not particulary tricky.

FWIW, I've finally rescued another bitrot victim (bdev API conversion and
related cleanups and fixes; sent to Jens for review last week, might be
actually mergable next cycle), so the timer one is near the top of the pile
right now; I can certainly bump it up and post the infrastructure + several
hundred instances converted by the next week.  Finishing vfs-2.6 tree should
take a couple of days (there are several _old_ pending mini-series that might
go there; currently the tree on kernel.org still has only immediate fixes +
ro-bind, all under vfs-fixes.b1 / ro-bind.b1), then I'm free for that...

PS: apologies for missing your previous mail; ETOOBIGMBOX ;-/

Re: [PATCH 5/5] typesafe: TIMER_INITIALIZER and setup_timer

[Rusty Russell]

On Thursday 06 March 2008 21:40:35 Al Viro wrote:
> extern void decay_to_pointer(void const volatile *);
> #define check_callback_type(f,x) (sizeof((0 ? decay_to_pointer(x),(f)(x) :
> (void)0), 0)) #define __setup_timer(timer, func, arg) \
> do {    \
> 	struct timer_list *__timer = (timer);   \
> 	__timer->function = (void (*)(unsigned long))(func); \
> 	__timer->data = (unsigned long)(arg);   \
> 	(void)check_callback_type(func, arg); \
> } while(0)

   Thanks for that snippet; it's much more complete than the last one I found.  
I especially like the handling of const- and volatile-taking callback fns.  
My version is uglier, pasted below.

   There are three problems I see here.  First, the lack of warning for "void 
intfn(int); DECLARE_TIMER(t, intfn, 0);", but that's relatively minor.  
Second, the change of all the users is just churn, with cast_if_type() it can 
be done over the next couple of years, if ever.

   Worst, I can't see a way to apply your technique in general, for 
non-void-returning functions (eg. interrupt handlers).

Here's the typesafe_cb () helper function from ccan (there are also 
typesafe_cb_preargs and postargs for callbacks with extra args):

/* Doesn't handle const volatile, but can be added. */
#define typesafe_cb(rettype, fn, arg)					\
	cast_if_type(cast_if_type(cast_if_type((fn),			\
					       rettype (*)(const typeof(arg)), \
					       rettype (*)(void *)),	\
				  rettype (*)(volatile typeof(arg)),	\
				  rettype (*)(void *)),			\
		     rettype (*)(typeof(arg)),				\
		     rettype (*)(void *))

> PS: apologies for missing your previous mail; ETOOBIGMBOX ;-/

That's fine, happens to all of us.  I was a little disappointed to return from 
holiday to discover that either your work or mine hadn't been merged tho :)

Cheers,
Rusty.

Re: [PATCH 5/5] typesafe: TIMER_INITIALIZER and setup_timer

[Al Viro]

On Mon, Mar 10, 2008 at 12:07:19PM +1100, Rusty Russell wrote:

>    Worst, I can't see a way to apply your technique in general, for 
> non-void-returning functions (eg. interrupt handlers).

Interrupt handlers are easy.  Just switch irqreturn_t from into to
struct {int x;}, adjust the rest of irqreturn.h accordingly, add
extern void want_irqreturn_t(irqreturn_t);
then replace 0 ? (f)(x) : (void)0 with want_irqreturn_t((f)(x)) in the
expression under sizeof and we are all set (assuming Jeff's elimination
of irq number in place by then or done in parallel).

Re: [PATCH 5/5] typesafe: TIMER_INITIALIZER and setup_timer

[Rusty Russell]

On Monday 10 March 2008 13:03:14 Al Viro wrote:
> On Mon, Mar 10, 2008 at 12:07:19PM +1100, Rusty Russell wrote:
> >    Worst, I can't see a way to apply your technique in general, for
> > non-void-returning functions (eg. interrupt handlers).
>
> Interrupt handlers are easy.  Just switch irqreturn_t from into to
> struct {int x;}, adjust the rest of irqreturn.h accordingly, add
> extern void want_irqreturn_t(irqreturn_t);
> then replace 0 ? (f)(x) : (void)0 with want_irqreturn_t((f)(x)) in the
> expression under sizeof and we are all set (assuming Jeff's elimination
> of irq number in place by then or done in parallel).

Hmm, I was hoping we'd kill irqreturn_t some day: I don't think 2.4 
compatibility is worth much any more.

That means doing similar things for stop_machine and kthread_create (just from 
looking at this patch series).

The benefits of natural-typed callback args drops if the return type pays the 
price.

Cheers,
Rusty.