hw/display/xenfb: Simulate auto-repeat key events

PATCH In review Liang Yan 3 participants 6 messages Nov 2, 2017 – Nov 3, 2017

Share & tools

Liang Yan

author

2017-11-02 17:19 UTC #0 lore raw

PATCH v2 1/1 hw/display/xenfb: Simulate auto-repeat key events

New tigervnc changes the way to send long pressed key, from "down up down up ..." to "down down ... up", it only affects xen pv console mode. I send a patch to latest kernel side, but it may have a fix in qemu backend for back compatible becase guest VMs may use very old kernel. This patch inserts an up event after each regular key down event to simulate an auto-repeat key event for xen keyboard frontend driver.

Signed-off-by Liang Yan <[email protected]>

---
v2:
- exclude extended key
- change log comment

 hw/display/xenfb.c | 5 +++++
 1 file changed, 5 insertions(+)

hw/display/xenfb.c +5 -0

index 8e2547ac05..1bc5b41ab7 100644
@@ -292,6 +292,11 @@ static void xenfb_key_event(void *opaque, int scancode)
     }
     trace_xenfb_key_event(opaque, scancode2linux[scancode], down);
     xenfb_send_key(xenfb, down, scancode2linux[scancode]);
+
+    /* insert an up event for regular down key event */
+    if (down && !xenfb->extended) {
+        xenfb_send_key(xenfb, 0, scancode2linux[scancode]);
+    }
 }
 
 /*

Signature · 7 lines

2.14.2


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Peter Maydell

2017-11-02 17:27 UTC #1 lore raw

Quoted 23 lines · Liang Yan

New tigervnc changes the way to send long pressed key, from "down up down up ..." to "down down ... up", it only affects xen pv console mode. I send a patch to latest kernel side, but it may have a fix in qemu backend for back compatible becase guest VMs may use very old kernel. This patch inserts an up event after each regular key down event to simulate an auto-repeat key event for xen keyboard frontend driver.
Signed-off-by Liang Yan <[email protected]>
---
v2:
- exclude extended key
- change log comment
 hw/display/xenfb.c | 5 +++++
 1 file changed, 5 insertions(+)
hw/display/xenfb.c +0 -0
index 8e2547ac05..1bc5b41ab7 100644

hw/display/xenfb.c static void xenfb_key_event(void *opaque, int scancode) · line 300

Quoted 9 lines

hw/display/xenfb.c +5 -0

@@ -292,6 +292,11 @@ static void xenfb_key_event(void *opaque, int scancode)
     }
     trace_xenfb_key_event(opaque, scancode2linux[scancode], down);
     xenfb_send_key(xenfb, down, scancode2linux[scancode]);
+
+    /* insert an up event for regular down key event */
+    if (down && !xenfb->extended) {
+        xenfb_send_key(xenfb, 0, scancode2linux[scancode]);
+    }
 }

This doesn't look to me like the right place to fix this bug. The xenfb key event handler is just one QEMU keyboard backend (in a setup where there are many possible sources of keyboard
events: vnc, gtk, SDL, cocoa UI frontends; and many possible
sinks: xenfb's key handling, ps2 keyboard emulator, etc etc).

We need to be clear in our definition of generic QEMU key events how key repeat is supposed to be handled, and then every consumer and every producer needs to follow that. In the specific case of the vnc UI frontend, we need to also look at what the VNC protocol specifies for key repeat. That then tells us whether the bug to be fixed is in QEMU, or in a particular VNC client.

thanks
-- PMM

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Daniel P. Berrange

2017-11-02 17:40 UTC #2 lore raw

Quoted 40 lines · Peter Maydell

Quoted 23 lines · Liang Yan
New tigervnc changes the way to send long pressed key, from "down up down up ..." to "down down ... up", it only affects xen pv console mode. I send a patch to latest kernel side, but it may have a fix in qemu backend for back compatible becase guest VMs may use very old kernel. This patch inserts an up event after each regular key down event to simulate an auto-repeat key event for xen keyboard frontend driver.
Signed-off-by Liang Yan <[email protected]>
---
v2:
- exclude extended key
- change log comment
 hw/display/xenfb.c | 5 +++++
 1 file changed, 5 insertions(+)
hw/display/xenfb.c +0 -0
index 8e2547ac05..1bc5b41ab7 100644
hw/display/xenfb.c static void xenfb_key_event(void *opaque, int scancode) · line 300
Quoted 9 lines
hw/display/xenfb.c +5 -0
@@ -292,6 +292,11 @@ static void xenfb_key_event(void *opaque, int scancode)
     }
     trace_xenfb_key_event(opaque, scancode2linux[scancode], down);
     xenfb_send_key(xenfb, down, scancode2linux[scancode]);
+
+    /* insert an up event for regular down key event */
+    if (down && !xenfb->extended) {
+        xenfb_send_key(xenfb, 0, scancode2linux[scancode]);
+    }
 }
This doesn't look to me like the right place to fix this bug. The xenfb key event handler is just one QEMU keyboard backend (in a setup where there are many possible sources of keyboard
events: vnc, gtk, SDL, cocoa UI frontends; and many possible
sinks: xenfb's key handling, ps2 keyboard emulator, etc etc).

Quoted 7 lines

We need to be clear in our definition of generic QEMU key events how key repeat is supposed to be handled, and then every consumer and every producer needs to follow that. In the specific case of the vnc UI frontend, we need to also look at what the VNC protocol specifies for key repeat. That then tells us whether the bug to be fixed is in QEMU, or in a particular VNC client.

I'm somewhat inclined to say this is a Tigervnc bug. We fixed this same issue in GTK-VNC ~10 years ago. While X11 would send a sequence of press,release,press,release, GTK would turn this into press,press,press,press,release which broke some VNC servers. So GTK-VNC undoes this optimization from GTK to ensure a full set of press,release,press,release pairs is always sent.

The official RFC for VNC does not specify any auto-repeat behaviour

https://tools.ietf.org/html/rfc6143#section-7.5.4

The unofficial community authored extension to the RFC suggests taking the press,press,press,release approach to allow servers to distinguish auto-repeat from manual repeat, but I'm not really convinced by that justification really

http://vncdotool.readthedocs.io/en/latest/rfbproto.html#keyevent

Regards,
Daniel

Signature · 8 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Liang Yan

author

2017-11-02 18:09 UTC #3 lore raw

Quoted 47 lines · Daniel P. Berrange

Quoted 39 lines · Peter Maydell
Quoted 23 lines · Liang Yan
New tigervnc changes the way to send long pressed key, from "down up down up ..." to "down down ... up", it only affects xen pv console mode. I send a patch to latest kernel side, but it may have a fix in qemu backend for back compatible becase guest VMs may use very old kernel. This patch inserts an up event after each regular key down event to simulate an auto-repeat key event for xen keyboard frontend driver.
Signed-off-by Liang Yan <[email protected]>
---
v2:
- exclude extended key
- change log comment
 hw/display/xenfb.c | 5 +++++
 1 file changed, 5 insertions(+)
hw/display/xenfb.c +0 -0
index 8e2547ac05..1bc5b41ab7 100644
hw/display/xenfb.c static void xenfb_key_event(void *opaque, int scancode) · line 300
Quoted 9 lines
hw/display/xenfb.c +5 -0
@@ -292,6 +292,11 @@ static void xenfb_key_event(void *opaque, int scancode)
     }
     trace_xenfb_key_event(opaque, scancode2linux[scancode], down);
     xenfb_send_key(xenfb, down, scancode2linux[scancode]);
+
+    /* insert an up event for regular down key event */
+    if (down && !xenfb->extended) {
+        xenfb_send_key(xenfb, 0, scancode2linux[scancode]);
+    }
 }
This doesn't look to me like the right place to fix this bug. The xenfb key event handler is just one QEMU keyboard backend (in a setup where there are many possible sources of keyboard
events: vnc, gtk, SDL, cocoa UI frontends; and many possible
sinks: xenfb's key handling, ps2 keyboard emulator, etc etc).
Quoted 7 lines
We need to be clear in our definition of generic QEMU key events how key repeat is supposed to be handled, and then every consumer and every producer needs to follow that. In the specific case of the vnc UI frontend, we need to also look at what the VNC protocol specifies for key repeat. That then tells us whether the bug to be fixed is in QEMU, or in a particular VNC client.

Quoted 6 lines

I'm somewhat inclined to say this is a Tigervnc bug. We fixed this same issue in GTK-VNC ~10 years ago. While X11 would send a sequence of press,release,press,release, GTK would turn this into press,press,press,press,release which broke some VNC servers. So GTK-VNC undoes this optimization from GTK to ensure a full set of press,release,press,release pairs is always sent.

Tigervnc uses "press press press ... release" now, this one is actually because xenkb couldn't handler these repeat events. I sent a fix to front-end side, and this patch here is for old compatibly only, otherwise we need to patch all those guest VMs even we run a newer host.

Thanks,
Liang

Quoted 9 lines

The official RFC for VNC does not specify any auto-repeat behaviour
https://tools.ietf.org/html/rfc6143#section-7.5.4
The unofficial community authored extension to the RFC suggests taking the press,press,press,release approach to allow servers to distinguish auto-repeat from manual repeat, but I'm not really convinced by that justification really

Quoted 4 lines

http://vncdotool.readthedocs.io/en/latest/rfbproto.html#keyevent
Regards,
Daniel

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Liang Yan

author

2017-11-03 01:12 UTC #4 lore raw

In reply to Daniel P. Berrange

This patch doesn't work here, my test environment actually broke, I am so sorry for the mess!
Please just ignore it.

Quoted 31 lines · Daniel P. Berrange

Quoted 30 lines · Peter Maydell
Quoted 29 lines · Liang Yan
New tigervnc changes the way to send long pressed key, from "down up down up ..." to "down down ... up", it only affects xen pv console mode. I send a patch to latest kernel side, but it may have a fix in qemu backend for back compatible becase guest VMs may use very old kernel. This patch inserts an up event after each regular key down event to simulate an auto-repeat key event for xen keyboard frontend driver.
Signed-off-by Liang Yan <[email protected]>
---
v2:
- exclude extended key
- change log comment
 hw/display/xenfb.c | 5 +++++
 1 file changed, 5 insertions(+)
hw/display/xenfb.c +3 -0
index 8e2547ac05..1bc5b41ab7 100644
@@ -292,6 +292,11 @@ static void xenfb_key_event(void *opaque, int scancode)
     }
     trace_xenfb_key_event(opaque, scancode2linux[scancode], down);
     xenfb_send_key(xenfb, down, scancode2linux[scancode]);
+
+    /* insert an up event for regular down key event */
+    if (down && !xenfb->extended) {

Quoted 8 lines

Quoted 8 lines
hw/display/xenfb.c static void xenfb_key_event(void *opaque, int scancode) · line 300
Quoted 3 lines
hw/display/xenfb.c +2 -0
+        xenfb_send_key(xenfb, 0, scancode2linux[scancode]);
+    }
 }
This doesn't look to me like the right place to fix this bug. The xenfb key event handler is just one QEMU keyboard backend (in a setup where there are many possible sources of keyboard
events: vnc, gtk, SDL, cocoa UI frontends; and many possible
sinks: xenfb's key handling, ps2 keyboard emulator, etc etc).

You are right, this is not a good place. Sorry did not think this much at first time.

Quoted 3 lines

Quoted 3 lines
We need to be clear in our definition of generic QEMU key events how key repeat is supposed to be handled, and then every consumer and every producer needs to follow that.

The is good, but I do not think it could be done easily.

Quoted 10 lines

Quoted 4 lines
In the specific case of the vnc UI frontend, we need to also look at what the VNC protocol specifies for key repeat. That then tells us whether the bug to be fixed is in QEMU, or in a particular VNC client.
I'm somewhat inclined to say this is a Tigervnc bug. We fixed this same issue in GTK-VNC ~10 years ago. While X11 would send a sequence of press,release,press,release, GTK would turn this into press,press,press,press,release which broke some VNC servers. So GTK-VNC undoes this optimization from GTK to ensure a full set of press,release,press,release pairs is always sent.

yeah, I saw both here and there. This one looks in a reverse way.

Quoted 6 lines

The official RFC for VNC does not specify any auto-repeat behaviour
https://tools.ietf.org/html/rfc6143#section-7.5.4
The unofficial community authored extension to the RFC suggests taking the press,press,press,release approach to allow servers to

Kernel input subsystem looks use this way now.

Best,
Liang

Quoted 7 lines

distinguish auto-repeat from manual repeat, but I'm not really convinced by that justification really
http://vncdotool.readthedocs.io/en/latest/rfbproto.html#keyevent
Regards,
Daniel

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

Liang Yan

author

2017-11-02 17:56 UTC #5 lore raw

In reply to Peter Maydell

Thanks for the reply.

Quoted 30 lines · Peter Maydell

Quoted 29 lines · Liang Yan
New tigervnc changes the way to send long pressed key, from "down up down up ..." to "down down ... up", it only affects xen pv console mode. I send a patch to latest kernel side, but it may have a fix in qemu backend for back compatible becase guest VMs may use very old kernel. This patch inserts an up event after each regular key down event to simulate an auto-repeat key event for xen keyboard frontend driver.
Signed-off-by Liang Yan <[email protected]>
---
v2:
- exclude extended key
- change log comment
 hw/display/xenfb.c | 5 +++++
 1 file changed, 5 insertions(+)
hw/display/xenfb.c +3 -0
index 8e2547ac05..1bc5b41ab7 100644
@@ -292,6 +292,11 @@ static void xenfb_key_event(void *opaque, int scancode)
     }
     trace_xenfb_key_event(opaque, scancode2linux[scancode], down);
     xenfb_send_key(xenfb, down, scancode2linux[scancode]);
+
+    /* insert an up event for regular down key event */
+    if (down && !xenfb->extended) {

Quoted 8 lines

hw/display/xenfb.c static void xenfb_key_event(void *opaque, int scancode) · line 300
Quoted 3 lines
hw/display/xenfb.c +2 -0
+        xenfb_send_key(xenfb, 0, scancode2linux[scancode]);
+    }
 }
This doesn't look to me like the right place to fix this bug. The xenfb key event handler is just one QEMU keyboard backend (in a setup where there are many possible sources of keyboard
events: vnc, gtk, SDL, cocoa UI frontends; and many possible
sinks: xenfb's key handling, ps2 keyboard emulator, etc etc).

QEMU actually just forwards what it receives(vnc,sdl) to different backend handler, usually those front and back(device) end will work together to handle those events. For this one, it could and should be fixed in front-end driver, but there are so many different guest kernel, especially for those old versions, it would be totally a pain. That is why I came back to backend side. BTW, I saw same logic in other places of qemu too.

Best,
Liang

Quoted 8 lines

We need to be clear in our definition of generic QEMU key events how key repeat is supposed to be handled, and then every consumer and every producer needs to follow that. In the specific case of the vnc UI frontend, we need to also look at what the VNC protocol specifies for key repeat. That then tells us whether the bug to be fixed is in QEMU, or in a particular VNC client.

Quoted 4 lines

thanks
-- PMM

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel