[PATCH] rfkill: prep for rfkill API changes

[PATCH] rfkill: prep for rfkill API changes

[Johannes Berg]

We've designed the /dev/rfkill API in a way that we
can increase the event struct by adding members at
the end, should it become necessary. To validate the
events, userspace and the kernel need to have the
proper event size to check for -- when reading from
the other end they need to verify that it's at least
version 1 of the event API, with the current struct
size, so define a constant for that and make the
code a little more 'future proof'.

Not that I expect that we'll have to change the event
size any time soon, but it's better to write the code
in a way that lends itself to extending.

Due to the current size of the event struct, the code
is currently equivalent, but should the event struct
ever need to be increased the new code might not need
changing.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 include/linux/rfkill.h |   14 ++++++++++++++
 net/rfkill/core.c      |   10 ++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

--- wireless-testing.orig/include/linux/rfkill.h	2009-07-05 14:39:09.000000000 +0200
+++ wireless-testing/include/linux/rfkill.h	2009-07-05 14:41:58.000000000 +0200
@@ -82,6 +82,20 @@ struct rfkill_event {
 	__u8  soft, hard;
 } __packed;
 
+/*
+ * We are planning to be backward and forward compatible with changes
+ * to the event struct, by adding new, optional, members at the end.
+ * When reading an event (whether the kernel from userspace or vice
+ * versa) we need to accept anything that's at least as large as the
+ * version 1 event size, but might be able to accept other sizes in
+ * the future.
+ *
+ * One exception is the kernel -- we already have two event sizes in
+ * that we've made the 'hard' member optional since our only option
+ * is to ignore it anyway.
+ */
+#define RFKILL_EVENT_SIZE_V1	8
+
 /* ioctl for turning off rfkill-input (if present) */
 #define RFKILL_IOC_MAGIC	'R'
 #define RFKILL_IOC_NOINPUT	1
--- wireless-testing.orig/net/rfkill/core.c	2009-07-05 14:40:13.000000000 +0200
+++ wireless-testing/net/rfkill/core.c	2009-07-05 14:49:41.000000000 +0200
@@ -1076,10 +1076,16 @@ static ssize_t rfkill_fop_write(struct f
 	struct rfkill_event ev;
 
 	/* we don't need the 'hard' variable but accept it */
-	if (count < sizeof(ev) - 1)
+	if (count < RFKILL_EVENT_SIZE_V1 - 1)
 		return -EINVAL;
 
-	if (copy_from_user(&ev, buf, sizeof(ev) - 1))
+	/*
+	 * Copy as much data as we can accept into our 'ev' buffer,
+	 * but tell userspace how much we've copied so it can determine
+	 * our API version even in a write() call, if it cares.
+	 */
+	count = min(count, sizeof(ev));
+	if (copy_from_user(&ev, buf, count))
 		return -EFAULT;
 
 	if (ev.op != RFKILL_OP_CHANGE && ev.op != RFKILL_OP_CHANGE_ALL)

Re: [PATCH] rfkill: prep for rfkill API changes

[Henrique de Moraes Holschuh]

On Sun, 05 Jul 2009, Johannes Berg wrote:
> We've designed the /dev/rfkill API in a way that we
> can increase the event struct by adding members at
> the end, should it become necessary. To validate the
> events, userspace and the kernel need to have the
> proper event size to check for -- when reading from
> the other end they need to verify that it's at least
> version 1 of the event API, with the current struct
> size, so define a constant for that and make the
> code a little more 'future proof'.
> 
> Not that I expect that we'll have to change the event
> size any time soon, but it's better to write the code
> in a way that lends itself to extending.
> 
> Due to the current size of the event struct, the code
> is currently equivalent, but should the event struct
> ever need to be increased the new code might not need
> changing.

This is good, but not complete.  Changes to the API that don't change the
size (or those which reduces it) are a problem: they are impossible with a
schema that detects API version by the size alone.

Maybe a new field with a API serial number should be added right _now_ while
it is still not too painful to do it?  This would be V2 of the API
(detectable by the size change), but there aren't many users yet, so the
effort to support it would not be daunting.

You can also publish the API version through a read-only sysfs attribute or
a separate IOCTL... it doesn't really need to be in-band (although in-band
is a lot better).

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] rfkill: prep for rfkill API changes

[Johannes Berg]

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

On Sun, 2009-07-05 at 10:24 -0300, Henrique de Moraes Holschuh wrote:

> This is good, but not complete.  Changes to the API that don't change the
> size (or those which reduces it) are a problem: they are impossible with a
> schema that detects API version by the size alone.

Umm, no, we can also add operations as required already.

> Maybe a new field with a API serial number should be added right _now_ while
> it is still not too painful to do it?  This would be V2 of the API
> (detectable by the size change), but there aren't many users yet, so the
> effort to support it would not be daunting.
> 
> You can also publish the API version through a read-only sysfs attribute or
> a separate IOCTL... it doesn't really need to be in-band (although in-band
> is a lot better).

No.

This patch doesn't change anything at all. It just reduces the
_subtlety_ involved in actually doing a size change, nothing more. A
real event version is _not_ necessary at all. In fact, I would argue
that basing _anything_ on the "version" rather than the "feature set" is
useless.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] rfkill: prep for rfkill API changes

[Henrique de Moraes Holschuh]

On Sun, 05 Jul 2009, Johannes Berg wrote:
> On Sun, 2009-07-05 at 10:24 -0300, Henrique de Moraes Holschuh wrote:
> > This is good, but not complete.  Changes to the API that don't change the
> > size (or those which reduces it) are a problem: they are impossible with a
> > schema that detects API version by the size alone.
> 
> Umm, no, we can also add operations as required already.

Yeah, I know.  What we can't do is to change the current ones, or drop them.

> _subtlety_ involved in actually doing a size change, nothing more. A
> real event version is _not_ necessary at all. In fact, I would argue
> that basing _anything_ on the "version" rather than the "feature set" is
> useless.

IMO version can tell you the supported feature set, but one could use a
feature set bitmap as well if one wanted.  Anyway, if you guys deem the
current way good enough, I am fine with it.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] rfkill: prep for rfkill API changes

[Marcel Holtmann]

Hi Johannes,

> We've designed the /dev/rfkill API in a way that we
> can increase the event struct by adding members at
> the end, should it become necessary. To validate the
> events, userspace and the kernel need to have the
> proper event size to check for -- when reading from
> the other end they need to verify that it's at least
> version 1 of the event API, with the current struct
> size, so define a constant for that and make the
> code a little more 'future proof'.
> 
> Not that I expect that we'll have to change the event
> size any time soon, but it's better to write the code
> in a way that lends itself to extending.
> 
> Due to the current size of the event struct, the code
> is currently equivalent, but should the event struct
> ever need to be increased the new code might not need
> changing.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

patch is fine as it is. We have discussed future extensions in this area
and I don't see any problems.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel