Re: 2.6.36/2.6.37: broken compatibility with userspace input-utils ?
From: Mauro Carvalho Chehab <hidden>
Date: 2011-01-25 22:01:47
Also in:
linux-media, lkml
Em 25-01-2011 18:54, Dmitry Torokhov escreveu:
On Wed, Jan 26, 2011 at 06:09:45AM +1000, Linus Torvalds wrote:quoted
On Wed, Jan 26, 2011 at 2:48 AM, Dmitry Torokhov [off-list ref] wrote:quoted
We should be able to handle the case where scancode is valid even though it might be unmapped yet. This is regardless of what version of EVIOCGKEYCODE we use, 1 or 2, and whether it is sparse keymap or not. Is it possible to validate the scancode by driver?More appropriately, why not just revert the thing? The version change
Reverting the version increment is a bad thing. I agree with Dmitry that an application that fails just because the API version were incremented is buggy.
Well, then we'll break Ubuntu again as they recompiled their input-utils package (without fixing the check). And the rest of distros do not seem to be using that package...
Reverting it will also break the ir-keytable userspace program that it is meant to be used by the Remote Controller devices, and uses it to adjust its behaviour to support RC's with more than 16 bits of scancodes. I agree that it is bad that the ABI broke, but reverting it will cause even more damage.
quoted
and the buggy EINVAL return both.I believe that -EINVAL thing only affects RC devices that Mauro switched to the new rc-core; input core in itself should be ABI compatible. Thus I'll leave the decision to him whether he wants to revert or fix compatibility issue.
The Remote Controller keycode tables are very sparse. In general, they contain up to 100 entries, and the scan codes typically have 16 bits. Some newer devices have 24 or 32 bits. With version 1, as the table index is the scancode, in order to read all keytables with EVIOCGKEYCODE, the userspace needs to do 2^16 reads (or 2^32 for RC-6 remotes). I don't need to say that this is highly ineffective. So, using V1 doesn't work fine anyway for Remote Controllers. Btw, ir-keycodestool don't work with V1 and more than 16 bits, because it doesn't scale. I didn't actually checked, but based on Dmitry's patch for input-kbd, it is clear to me that the old version only supports 16 bits scancodes: Em 25-01-2011 04:52, Dmitry Torokhov escreveu:
quoted hunk ↗ jump to hunk
From c22c85c0b675422a23e3d853ed06fedc36805774 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov <dmitry.torokhov@gmail.com> Date: Mon, 24 Jan 2011 22:49:59 -0800 Subject: [PATCH] input-kbd - switch to using EVIOCGKEYCODE2 when available Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- input-kbd.c | 118 ++++++++++++++++++++++++++++++++++++++++------------------- 1 files changed, 80 insertions(+), 38 deletions(-)diff --git a/input-kbd.c b/input-kbd.c index e94529d..5d93d54 100644 --- a/input-kbd.c +++ b/input-kbd.c
<snip>
quoted hunk ↗ jump to hunk
@@ -23,7 +41,7 @@ struct kbd_map { /* ------------------------------------------------------------------ */ -static struct kbd_map* kbd_map_read(int fd) +static struct kbd_map* kbd_map_read(int fd, unsigned int version) { struct kbd_entry entry; struct kbd_map *map;@@ -32,16 +50,37 @@ static struct kbd_map* kbd_map_read(int fd) map = malloc(sizeof(*map)); memset(map,0,sizeof(*map)); for (map->size = 0; map->size < 65536; map->size++) {
See, it will only look into the 16-bits scancode space. There are several remote controllers with 24 bits and 32 bits, so the tool is already broken anyway. On the tests I did here with an ir-keytable version made before such change, with a Fedora rawhide kernel (2.6.37), I didn't notice any breakage at EVIOCGKEYCODE. I'll do more tests tomorrow with a vanilla Kernel. I'll compile a vanilla 2.6.37 kernel tomorrow and, if needed, write a patch.
quoted
As Mark said, breaking user space simply isn't acceptable. And since breaking user space isn't acceptable, then incrementing the version is stupid too.It might not have been the best idea to increment, however I maintain that if there exists version is can be changed. Otherwise there is no point in having version at all.
Not arguing in favor of the version numbering, but it is easy to read the version increment at the beginning of the application, and adjust if the code will use EVIOCGKEYCODE or EVIOCGKEYCODE_V2 of the ioctl's, depending on what kernel provides. Ok, we might be just calling the new ioctl and check for -ENOSYS at the beginning, using some fake arguments.
As I said, reverting the version bump will cause yet another wave of breakages so I propose leaving version as is.quoted
The way we add new ioctl's is not by incrementing some "ABI version" crap. It's by adding new ioctl's or system calls or whatever that simply used to return -ENOSYS or other error before, while preserving the old ABI. That way old binaries don't break (for _ANY_ reason), and new binaries can see "oh, this doesn't support the new thing".That has been done as well; we have 2 new ioctls and kept 2 old ioctls.
Regards, Mauro.