[Linuxwacom-devel] [PATCH] xsetwacom: use XkbGetMap instead of XkbGetKeyboard

Discussion:

Sebastian Reuße

2017-03-08 18:25:53 UTC

When running on Wayland or when using the Xorg libinput driver (which is the
default on some distributions), a call to XkbGetKeyboard always returns NULL
(causing xsetwacom to segfault) and is apparently not supported anymore (cf.
[1,2] and others). Instead, we are supposed to use XkbGetMap now.

[1] <https://bugs.freedesktop.org/show_bug.cgi?id=89240>
[2] <https://github.com/glfw/glfw/issues/389>

Signed-off-by: Sebastian Reuße <***@wirrsal.net>
---
tools/xsetwacom.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/xsetwacom.c b/tools/xsetwacom.c
index bbc92f1..b0f21e4 100644
--- a/tools/xsetwacom.c
+++ b/tools/xsetwacom.c
@@ -1173,9 +1173,10 @@ static int keysym_to_keycode(Display *dpy, KeySym sym)
XkbStateRec state;
int kc = 0;

-
- if (!xkb)
- xkb = XkbGetKeyboard(dpy, XkbAllComponentsMask, XkbUseCoreKbd);
+ if (!xkb && !(xkb = XkbGetMap(dpy, XkbAllComponentsMask, XkbUseCoreKbd))) {
+ fprintf(stderr, "Warning: failed to query keyboard map\n");
+ goto out;
+ }
XkbGetState(dpy, XkbUseCoreKbd, &state);

for (kc = xkb->min_key_code; kc <= xkb->max_key_code; kc++)

--
2.12.0

Peter Hutterer

2017-03-09 05:38:23 UTC

Permalink

Post by Sebastian ReuÃe
When running on Wayland or when using the Xorg libinput driver (which is the
default on some distributions), a call to XkbGetKeyboard always returns NULL
(causing xsetwacom to segfault) and is apparently not supported anymore (cf.
[1,2] and others). Instead, we are supposed to use XkbGetMap now.

patch looks good for what it does, but the commit message is confusing.
xsetwacom is a tool specific to the xorg wacom input driver. It won't work
on wayland because xwayland tablets work completely different and it won't
work on the xorg libinput driver because all the properties are different.
Did you actually see a crash here, and if so what was the command and setup
to trigger that crash?

Cheers,
Peter

Post by Sebastian ReuÃe
[1] <https://bugs.freedesktop.org/show_bug.cgi?id=89240>
[2] <https://github.com/glfw/glfw/issues/389>
---
tools/xsetwacom.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/xsetwacom.c b/tools/xsetwacom.c
index bbc92f1..b0f21e4 100644
--- a/tools/xsetwacom.c
+++ b/tools/xsetwacom.c
@@ -1173,9 +1173,10 @@ static int keysym_to_keycode(Display *dpy, KeySym sym)
XkbStateRec state;
int kc = 0;
-
- if (!xkb)
- xkb = XkbGetKeyboard(dpy, XkbAllComponentsMask, XkbUseCoreKbd);
+ if (!xkb && !(xkb = XkbGetMap(dpy, XkbAllComponentsMask, XkbUseCoreKbd))) {
+ fprintf(stderr, "Warning: failed to query keyboard map\n");
+ goto out;
+ }
XkbGetState(dpy, XkbUseCoreKbd, &state);
for (kc = xkb->min_key_code; kc <= xkb->max_key_code; kc++)
--
2.12.0
------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
_______________________________________________
Linuxwacom-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel

Sebastian Reuße

2017-03-09 06:37:39 UTC

Permalink

Hello Peter,

Post by Peter Hutterer

Yes, I saw a segfault when running «xsetwacom --set $id Button 1 "key
$anykey"», which gdb revealed was due to XkbGetKeyboard always returning
NULL. The only references that came up in a search mentioned Wayland, so
my theory was that Archlinux now using libinput as a default was the
cause (I thought libinput somehow backs all the Xorg input, but I
realize now that I was mistaken). Using «XkbGetMap» instead fixed the
issue for me.

However I just realized something entirely different may be the cause.
I’m using the AdNW [1] keymap (sort of a German dvorak) as installed by
running the script in [2]. When I load a standard keymap such as
«setxkbmap de», I can no longer produce the segfault using the unpatched
version of xsetwacom.

I’m not really familiar with Xorg internals. Are you able to make sense
of this by any chance?

[1] <http://adnw.de>
[2] <https://pb.gehidore.net/83Pu>

Kind regards,

Sebastian

Peter Hutterer

2017-03-10 01:53:36 UTC

Permalink

Post by Sebastian ReuÃe
Hello Peter,

Post by Peter Hutterer

yeah, two different problems, xsetwacom cannot work under wayland or with
xorg-libinput. and I checked the code, it does the the property checking
correctly.

Post by Sebastian ReuÃe
However I just realized something entirely different may be the cause.
I’m using the AdNW [1] keymap (sort of a German dvorak) as installed by
running the script in [2]. When I load a standard keymap such as
«setxkbmap de», I can no longer produce the segfault using the unpatched
version of xsetwacom.
I’m not really familiar with Xorg internals. Are you able to make sense
of this by any chance?

yeah, my guess is because you're loading the keymap directly instead of
going through the RMLVO you're missing the keymap names and things go boom.
A more detailed analysis requires me swapping too much of XKB again, so I'd
rather go with the oneliner patch :)

Cheers,
Peter

Post by Sebastian ReuÃe
[1] <http://adnw.de>
[2] <https://pb.gehidore.net/83Pu>
Kind regards,
Sebastian

Jason Gerecke

2017-03-24 18:52:22 UTC

Permalink

Post by Peter Hutterer

Post by Sebastian ReuÃe
Hello Peter,

Post by Peter Hutterer

yeah, two different problems, xsetwacom cannot work under wayland or with
xorg-libinput. and I checked the code, it does the the property checking
correctly.

Just making sure this doesn't get lost. Does this only need an update
to the commit description to make it more accurate?

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....

Peter Hutterer

2017-03-24 21:06:19 UTC

Permalink

Post by Jason Gerecke

Post by Peter Hutterer

Post by Sebastian ReuÃe
Hello Peter,

Post by Peter Hutterer

yeah, two different problems, xsetwacom cannot work under wayland or with
xorg-libinput. and I checked the code, it does the the property checking
correctly.

Just making sure this doesn't get lost. Does this only need an update
to the commit description to make it more accurate?

yes, that's all that's needed here.

Cheers,
Peter

Jason Gerecke

2017-05-10 17:43:22 UTC

Permalink

Post by Peter Hutterer

Post by Jason Gerecke

Post by Peter Hutterer

Post by Sebastian ReuÃe
Hello Peter,

Post by Peter Hutterer

yeah, two different problems, xsetwacom cannot work under wayland or with
xorg-libinput. and I checked the code, it does the the property checking
correctly.

Just making sure this doesn't get lost. Does this only need an update
to the commit description to make it more accurate?

yes, that's all that's needed here.
Cheers,
Peter

Poking Sebastian :)

--
Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....

Sebastian Reuße

2017-05-10 18:05:32 UTC

Permalink

XkbGetMap is more robust in cases where certain keyboard description components
are missing. XkbGetKeyboard will fail when any component cannot be resolved;
since all XkbAllComponentsMask is requested, any missing component will result
in the call returning NULL. Since we don’t necessarily need all
components (e.g., keyboard geometry, keymap names), we use XkbGetMap instead.

Signed-off-by: Sebastian Reuße <***@wirrsal.net>
---
tools/xsetwacom.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/xsetwacom.c b/tools/xsetwacom.c
index bbc92f1..b0f21e4 100644
--- a/tools/xsetwacom.c
+++ b/tools/xsetwacom.c
@@ -1173,9 +1173,10 @@ static int keysym_to_keycode(Display *dpy, KeySym sym)
XkbStateRec state;
int kc = 0;

-
- if (!xkb)
- xkb = XkbGetKeyboard(dpy, XkbAllComponentsMask, XkbUseCoreKbd);
+ if (!xkb && !(xkb = XkbGetMap(dpy, XkbAllComponentsMask, XkbUseCoreKbd))) {
+ fprintf(stderr, "Warning: failed to query keyboard map\n");
+ goto out;
+ }
XkbGetState(dpy, XkbUseCoreKbd, &state);

for (kc = xkb->min_key_code; kc <= xkb->max_key_code; kc++)

--
2.12.2

Sebastian Reuße

2017-05-10 18:16:07 UTC

Permalink

XkbGetMap is more robust in cases where certain keyboard description components
are missing. XkbGetKeyboard will fail when any component cannot be resolved;
since XkbAllComponentsMask is requested, any missing component will result in
the call returning NULL. Since we don’t necessarily need all components (e.g.,
keyboard geometry, keymap names), we use XkbGetMap instead.

Signed-off-by: Sebastian Reuße <***@wirrsal.net>
---
tools/xsetwacom.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/xsetwacom.c b/tools/xsetwacom.c
index bbc92f1..b0f21e4 100644
--- a/tools/xsetwacom.c
+++ b/tools/xsetwacom.c
@@ -1173,9 +1173,10 @@ static int keysym_to_keycode(Display *dpy, KeySym sym)
XkbStateRec state;
int kc = 0;

-
- if (!xkb)
- xkb = XkbGetKeyboard(dpy, XkbAllComponentsMask, XkbUseCoreKbd);
+ if (!xkb && !(xkb = XkbGetMap(dpy, XkbAllComponentsMask, XkbUseCoreKbd))) {
+ fprintf(stderr, "Warning: failed to query keyboard map\n");
+ goto out;
+ }
XkbGetState(dpy, XkbUseCoreKbd, &state);

for (kc = xkb->min_key_code; kc <= xkb->max_key_code; kc++)

--
2.12.2

Sebastian Reuße

2017-05-10 18:16:46 UTC

Permalink

Fixed a typo.

--
Insane cobra split the wood
Trader of the lowland breed
Call a jittney, drive away
In the slipstream we will stay

Peter Hutterer

2017-05-10 23:30:36 UTC

Permalink

Post by Sebastian ReuÃe
XkbGetMap is more robust in cases where certain keyboard description components
are missing. XkbGetKeyboard will fail when any component cannot be resolved;
since XkbAllComponentsMask is requested, any missing component will result in
the call returning NULL. Since we don’t necessarily need all components (e.g.,
keyboard geometry, keymap names), we use XkbGetMap instead.
---
tools/xsetwacom.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/xsetwacom.c b/tools/xsetwacom.c
index bbc92f1..b0f21e4 100644
--- a/tools/xsetwacom.c
+++ b/tools/xsetwacom.c
@@ -1173,9 +1173,10 @@ static int keysym_to_keycode(Display *dpy, KeySym sym)
XkbStateRec state;
int kc = 0;
-
- if (!xkb)
- xkb = XkbGetKeyboard(dpy, XkbAllComponentsMask, XkbUseCoreKbd);
+ if (!xkb && !(xkb = XkbGetMap(dpy, XkbAllComponentsMask, XkbUseCoreKbd))) {
+ fprintf(stderr, "Warning: failed to query keyboard map\n");
+ goto out;
+ }
XkbGetState(dpy, XkbUseCoreKbd, &state);
for (kc = xkb->min_key_code; kc <= xkb->max_key_code; kc++)
--
2.12.2

pushed, thanks. I made a minor modification for readability before pushing.
I'm not a fan of if (!foo && !(foo = bar())) because it's too easy to read
over it. Slightly better is an explicit NULL comparison:

if (!foo && (foo = bar()) == NULL)

but my favourite here is a simple:

if (!foo)
foo = bar();
if (!foo)
boom

then we just hope the compiler gods shine their light and optimise this ;)

Cheers,
Peter

Sebastian Reuße

2017-05-10 18:14:29 UTC

Permalink

--
Insane cobra split the wood
Trader of the lowland breed
Call a jittney, drive away
In the slipstream we will stay

Jason Gerecke

2017-05-10 21:22:34 UTC

Permalink

Post by Sebastian ReuÃe

Alternatively, one might try whether a less encompassing components mask
will do. XkbGetMap appears to just NULL those components of the struct
which are unavailable, while XkbGetKeyboard will return NULL wholesale.
Best regards,
SR
--
Insane cobra split the wood
Trader of the lowland breed
Call a jittney, drive away
In the slipstream we will stay

Thanks for the updated patch :) I'll wait for Peter's response, since
XKB is a dark corner of the xserver that I've (thankfully) never had
to shine my flashlight too intensely on.

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....

Sebastian Reuße

2017-05-10 18:11:05 UTC

Permalink

Hi Jason!

Thanks for the long-range poke!

Post by Peter Hutterer

Post by Jason Gerecke
Just making sure this doesn't get lost. Does this only need an update
to the commit description to make it more accurate?

yes, that's all that's needed here.

So, I tried to conjure up an alternative explanation. Since I’m not very
Xorg savvy, it might not be true though. However, since Peter brought up
this theory, it just might. ;)

Does the commit message make more sense like this? Otherwise, if you
have a more thorough explanation, feel free to reword the patch.

Kind regards,

Sebastian

--
Insane cobra split the wood
Trader of the lowland breed
Call a jittney, drive away
In the slipstream we will stay