On Mon, Nov 02, 2009 at 11:11:55PM -0800, Dmitry Torokhov wrote: > Evdev is way too paranoid when checking whether it deals with the same > device when switching consoles which causes people lose keyboards if > they happen to adjust keymap after X starts. PLease consider applying > the patch below.
Thanks for the patch Dmitry. I certainly agree with the principle but I do have a few minor comments. > From: Dmitry Torokhov <[email protected]> > Subject: [PATCH] evdev - relax checks when reopening devices > > When checking whether we are dealing with the same device as before > when we try to reopen it evdev should not require exact match of > entire keymap. Users should be allowed to adjust keymaps to better > match their hardware even after X starts. However we don't expect > changes in [BTN_MISC, KEY_OK) range since these codes are reserved for > mice, joysticks, tablets and so forth, so we will limit the check to > this range. > > The same goes for absinfo - limits can change and it should not result > in device being disabled. > > Also check the length of the data returned by ioctl and don't try to > compare more than we were given. > > Signed-off-by: Dmitry Torokhov <[email protected]> > --- > src/evdev.c | 126 > +++++++++++++++++++++++++++++++++-------------------------- > 1 files changed, 71 insertions(+), 55 deletions(-) > > diff --git a/src/evdev.c b/src/evdev.c > index 0dff271..747f3b1 100644 > --- a/src/evdev.c > +++ b/src/evdev.c > @@ -1671,6 +1671,7 @@ static int > EvdevCacheCompare(InputInfoPtr pInfo, BOOL compare) > { > EvdevPtr pEvdev = pInfo->private; > + size_t len; > int i; > > char name[1024] = {0}; > @@ -1679,107 +1680,122 @@ EvdevCacheCompare(InputInfoPtr pInfo, BOOL compare) > unsigned long rel_bitmask[NLONGS(REL_CNT)] = {0}; > unsigned long abs_bitmask[NLONGS(ABS_CNT)] = {0}; > unsigned long led_bitmask[NLONGS(LED_CNT)] = {0}; > - struct input_absinfo absinfo[ABS_CNT]; > > - if (ioctl(pInfo->fd, > - EVIOCGNAME(sizeof(name) - 1), name) < 0) { > + if (ioctl(pInfo->fd, EVIOCGNAME(sizeof(name) - 1), name) < 0) { this line is white-space only, no? > xf86Msg(X_ERROR, "ioctl EVIOCGNAME failed: %s\n", strerror(errno)); > goto error; > } > [...] > - if (ioctl(pInfo->fd, > - EVIOCGBIT(EV_KEY, sizeof(key_bitmask)), key_bitmask) < 0) { > - xf86Msg(X_ERROR, "%s: ioctl EVIOCGBIT failed: %s\n", pInfo->name, > strerror(errno)); > + len = ioctl(pInfo->fd, EVIOCGBIT(EV_KEY, sizeof(key_bitmask)), > key_bitmask); > + if (len < 0) { > + xf86Msg(X_ERROR, "%s: ioctl EVIOCGBIT failed: %s\n", > + pInfo->name, strerror(errno)); > goto error; > } > > - if (compare && memcmp(pEvdev->key_bitmask, key_bitmask, > sizeof(key_bitmask))) { > - xf86Msg(X_ERROR, "%s: device key_bitmask has changed\n", > pInfo->name); > - goto error; > + if (compare) { > + /* > + * Keys are special as user can adjust keymap at any time (on > + * devices that support EVIOCSKEYCODE. However we do not expect > + * buttons reserved fir mice/tablets/digitizers and so on to typo: "for" > + * appear/disappear so we will check only those in > + * [BTN_MISC, KEY_OK) range. > + */ > + size_t start_word = BTN_MISC / LONG_BITS; > + size_t start_byte = start_word * sizeof(unsigned long); > + size_t end_word = KEY_OK / LONG_BITS; > + size_t end_byte = end_word * sizeof(unsigned long); > + > + if (len >= start_byte && > + memcmp(&pEvdev->key_bitmask[start_word], > &key_bitmask[start_word], > + min(len, end_byte) - start_byte + 1)) { > + xf86Msg(X_ERROR, "%s: device key_bitmask has changed\n", > pInfo->name); > + goto error; > + } > } > > - if (ioctl(pInfo->fd, > - EVIOCGBIT(EV_LED, sizeof(led_bitmask)), led_bitmask) < 0) { > - xf86Msg(X_ERROR, "%s: ioctl EVIOCGBIT failed: %s\n", pInfo->name, > strerror(errno)); > + /* Copy the data so we have reasonably up-to-date info */ > + memcpy(pEvdev->key_bitmask, key_bitmask, len); I think we should move this whole keyinfo bit below the absinfo stuff before. Otherwise there is a narrow case where the key info is updated but the compare still then still fail because something else has changed. Other than that, I'm happy with the patch. Eric - have you managed to test this patch yet? Cheers, Peter > + > + len = ioctl(pInfo->fd, EVIOCGBIT(EV_LED, sizeof(led_bitmask)), > led_bitmask); > + if (len < 0) { > + xf86Msg(X_ERROR, "%s: ioctl EVIOCGBIT failed: %s\n", > + pInfo->name, strerror(errno)); > goto error; > } > > - if (compare && memcmp(pEvdev->led_bitmask, led_bitmask, > sizeof(led_bitmask))) { > + if (!compare) { > + memcpy(pEvdev->led_bitmask, led_bitmask, len); > + } else if (memcmp(pEvdev->led_bitmask, led_bitmask, len)) { > xf86Msg(X_ERROR, "%s: device led_bitmask has changed\n", > pInfo->name); > goto error; > } > - memset(absinfo, 0, sizeof(absinfo)); > - > - for (i = ABS_X; i <= ABS_MAX; i++) > - { > - if (TestBit(i, abs_bitmask)) > - { > - if (ioctl(pInfo->fd, EVIOCGABS(i), &absinfo[i]) < 0) { > - xf86Msg(X_ERROR, "%s: ioctl EVIOCGABS failed: %s\n", > pInfo->name, strerror(errno)); > + /* > + * Do not try to validate absinfo data since it is not expected > + * to be static, always refresh it in evdev structure. > + */ > + for (i = ABS_X; i <= ABS_MAX; i++) { > + if (TestBit(i, abs_bitmask)) { > + len = ioctl(pInfo->fd, EVIOCGABS(i), &pEvdev->absinfo[i]); > + if (len < 0) { > + xf86Msg(X_ERROR, "%s: ioctl EVIOCGABSi(%d) failed: %s\n", > + pInfo->name, i, strerror(errno)); > goto error; > } > - /* ignore current position (value) in comparison (bug #19819) */ > - absinfo[i].value = pEvdev->absinfo[i].value; > } > } > > - if (compare && memcmp(pEvdev->absinfo, absinfo, sizeof(absinfo))) { > - xf86Msg(X_ERROR, "%s: device absinfo has changed\n", pInfo->name); > - goto error; > - } > - > - /* cache info */ > - if (!compare) > - { > - strcpy(pEvdev->name, name); > - memcpy(pEvdev->bitmask, bitmask, sizeof(bitmask)); > - memcpy(pEvdev->key_bitmask, key_bitmask, sizeof(key_bitmask)); > - memcpy(pEvdev->rel_bitmask, rel_bitmask, sizeof(rel_bitmask)); > - memcpy(pEvdev->abs_bitmask, abs_bitmask, sizeof(abs_bitmask)); > - memcpy(pEvdev->led_bitmask, led_bitmask, sizeof(led_bitmask)); > - memcpy(pEvdev->absinfo, absinfo, sizeof(absinfo)); > - } > - > return Success; > > error: _______________________________________________ xorg mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/xorg
