[libvirt] [PATCH 1/2] Fix keymap used to talk with QEMU
From: "Daniel P. Berrange" <berrange@redhat.com> The QEMU 'sendkey' command expects keys to be encoded in the same way as the RFB extended keycode set. Specfically it wants extended keys to have the high bit of the first byte set, while the Linux XT KBD driver codeset uses the low bit of the second byte. To deal with this we introduce a new keymap 'RFB' and use that in the QEMU driver * include/libvirt/libvirt.h.in: Add VIR_KEYCODE_SET_RFB * src/qemu/qemu_driver.c: Use RFB keycode set instead of XT KBD * src/util/virkeycode-mapgen.py: Auto-generate the RFB keycode set from the XT KBD set * src/util/virkeycode.c: Add RFB keycode entry to table. Add a verify check on cardinality of the codeOffset table --- include/libvirt/libvirt.h.in | 1 + src/qemu/qemu_driver.c | 8 ++++---- src/util/virkeycode-mapgen.py | 12 ++++++++++++ src/util/virkeycode.c | 5 +++++ 4 files changed, 22 insertions(+), 4 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index aa29fb6..53a2f7d 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1875,6 +1875,7 @@ typedef enum { VIR_KEYCODE_SET_XT_KBD = 6, VIR_KEYCODE_SET_USB = 7, VIR_KEYCODE_SET_WIN32 = 8, + VIR_KEYCODE_SET_RFB = 9, VIR_KEYCODE_SET_LAST, } virKeycodeSet; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 57ad3d1..4ede142 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1883,17 +1883,17 @@ static int qemuDomainSendKey(virDomainPtr domain, virCheckFlags(0, -1); - /* translate the keycode to XT_KBD for qemu driver */ - if (codeset != VIR_KEYCODE_SET_XT_KBD) { + /* translate the keycode to RFB for qemu driver */ + if (codeset != VIR_KEYCODE_SET_RFB) { int i; int keycode; for (i = 0; i < nkeycodes; i++) { - keycode = virKeycodeValueTranslate(codeset, VIR_KEYCODE_SET_XT_KBD, + keycode = virKeycodeValueTranslate(codeset, VIR_KEYCODE_SET_RFB, keycodes[i]); if (keycode < 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot translate keycode %u of %s codeset to xt_kbd keycode"), + _("cannot translate keycode %u of %s codeset to rfb keycode"), keycodes[i], virKeycodeSetTypeToString(codeset)); return -1; diff --git a/src/util/virkeycode-mapgen.py b/src/util/virkeycode-mapgen.py index acf7364..d3d2aae 100755 --- a/src/util/virkeycode-mapgen.py +++ b/src/util/virkeycode-mapgen.py @@ -14,6 +14,7 @@ import sys import re namecolums = (0,2,10) +xtkbdkey_index = 8 def quotestring(str): if str[0] != '"': @@ -35,10 +36,21 @@ sys.stdin.readline() # eat the fist line. for line in sys.stdin.xreadlines(): a = re.match("([^,]*)," * 13 + "([^,]*)$", line[0:-1]).groups() b = "" + rfbkey = 0 for i in namecolums: b = b + (a[i] and quotestring(a[i]) or 'NULL') + ',' for i in [ x for x in range(12) if not x in namecolums ]: b = b + (a[i] or '0') + ',' + if i == xtkbdkey_index: + # RFB keycodes are XT kbd keycodes with a slightly + # different encoding of 0xe0 scan codes. RFB uses + # the high bit of the first byte, instead of the low + # bit of the second byte. + rfbkey = int(a[i] or '0') + rfbkey = (rfbkey & 0x100) >> 1 | (rfbkey & 0x7f) + + # Append RFB keycode as the last column + b = b + str(rfbkey) print " { " + b + "}," print '};' diff --git a/src/util/virkeycode.c b/src/util/virkeycode.c index 0d42767..de7aa81 100644 --- a/src/util/virkeycode.c +++ b/src/util/virkeycode.c @@ -38,6 +38,7 @@ struct keycode { unsigned short xt_kbd; unsigned short usb; unsigned short win32; + unsigned short rfb; }; #define VIRT_KEY_INTERNAL @@ -62,7 +63,10 @@ static unsigned int codeOffset[] = { offsetof(struct keycode, usb), [VIR_KEYCODE_SET_WIN32] = offsetof(struct keycode, win32), + [VIR_KEYCODE_SET_RFB] = + offsetof(struct keycode, rfb), }; +extern int (*codeOffsetVerify(void)) [verify_true (ARRAY_CARDINALITY(codeOffset) == VIR_KEYCODE_SET_LAST)]; \ VIR_ENUM_IMPL(virKeycodeSet, VIR_KEYCODE_SET_LAST, "linux", @@ -74,6 +78,7 @@ VIR_ENUM_IMPL(virKeycodeSet, VIR_KEYCODE_SET_LAST, "xt_kbd", "usb", "win32", + "rfb", ); static int __virKeycodeValueFromString(unsigned int name_offset, -- 1.7.6
From: "Daniel P. Berrange" <berrange@redhat.com> On success, the 'sendkey' command does not return any data, so any data in the reply should be considered to be an error message * src/qemu/qemu_monitor_text.c: Treat non-"" reply data as an error message for 'sendkey' command --- src/qemu/qemu_monitor_text.c | 15 ++++++++++++--- 1 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 54541ce..4bd761d 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2848,6 +2848,7 @@ int qemuMonitorTextSendKey(qemuMonitorPtr mon, int i; virBuffer buf = VIR_BUFFER_INITIALIZER; char *cmd, *reply = NULL; + int ret = -1; if (nkeycodes > VIR_DOMAIN_SEND_KEY_MAX_KEYS || nkeycodes == 0) return -1; @@ -2880,13 +2881,21 @@ int qemuMonitorTextSendKey(qemuMonitorPtr mon, qemuReportError(VIR_ERR_OPERATION_FAILED, _("failed to send key using command '%s'"), cmd); - VIR_FREE(cmd); - return -1; + goto cleanup; + } + + if (STRNEQ(reply, "")) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("failed to send key '%s'"), reply); + goto cleanup; } + ret = 0; + +cleanup: VIR_FREE(cmd); VIR_FREE(reply); - return 0; + return ret; } /* Returns -1 on error, -2 if not supported */ -- 1.7.6
On 08/25/2011 10:49 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
On success, the 'sendkey' command does not return any data, so any data in the reply should be considered to be an error message
* src/qemu/qemu_monitor_text.c: Treat non-"" reply data as an error message for 'sendkey' command --- src/qemu/qemu_monitor_text.c | 15 ++++++++++++--- 1 files changed, 12 insertions(+), 3 deletions(-)
ACK.
@@ -2880,13 +2881,21 @@ int qemuMonitorTextSendKey(qemuMonitorPtr mon, qemuReportError(VIR_ERR_OPERATION_FAILED, _("failed to send key using command '%s'"), cmd); - VIR_FREE(cmd); - return -1; + goto cleanup; + } + + if (STRNEQ(reply, "")) {
I would have written this as: if (*reply) { but I think the compiler is sufficiently smart to optimize strcmp(expr,"") into a test for the first byte of expr (and if not, that would be a missed optimization in gcc). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/25/2011 10:49 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
The QEMU 'sendkey' command expects keys to be encoded in the same way as the RFB extended keycode set. Specfically it wants extended
s/Specfically/Specifically/
keys to have the high bit of the first byte set, while the Linux XT KBD driver codeset uses the low bit of the second byte. To deal with this we introduce a new keymap 'RFB' and use that in the QEMU driver
* include/libvirt/libvirt.h.in: Add VIR_KEYCODE_SET_RFB * src/qemu/qemu_driver.c: Use RFB keycode set instead of XT KBD * src/util/virkeycode-mapgen.py: Auto-generate the RFB keycode set from the XT KBD set * src/util/virkeycode.c: Add RFB keycode entry to table. Add a verify check on cardinality of the codeOffset table --- include/libvirt/libvirt.h.in | 1 + src/qemu/qemu_driver.c | 8 ++++---- src/util/virkeycode-mapgen.py | 12 ++++++++++++ src/util/virkeycode.c | 5 +++++ 4 files changed, 22 insertions(+), 4 deletions(-)
@@ -62,7 +63,10 @@ static unsigned int codeOffset[] = { offsetof(struct keycode, usb), [VIR_KEYCODE_SET_WIN32] = offsetof(struct keycode, win32), + [VIR_KEYCODE_SET_RFB] = + offsetof(struct keycode, rfb), }; +extern int (*codeOffsetVerify(void)) [verify_true (ARRAY_CARDINALITY(codeOffset) == VIR_KEYCODE_SET_LAST)]; \
Why the trailing backslash? Also, open-coding this verify is dangerous; gcc warnings have changed over time, rendering certain verification styles that were once safe into something that trips up -Werror. It is safer (and shorter!) to let gnulib worry about gcc quirks, by changing this line to just be: verify(ARRAY_CARDINALITY(codeOffset) == VIR_KEYCODE_SET_LAST); ACK with the verify line fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Aug 25, 2011 at 11:20:18AM -0600, Eric Blake wrote:
On 08/25/2011 10:49 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
The QEMU 'sendkey' command expects keys to be encoded in the same way as the RFB extended keycode set. Specfically it wants extended
s/Specfically/Specifically/
keys to have the high bit of the first byte set, while the Linux XT KBD driver codeset uses the low bit of the second byte. To deal with this we introduce a new keymap 'RFB' and use that in the QEMU driver
* include/libvirt/libvirt.h.in: Add VIR_KEYCODE_SET_RFB * src/qemu/qemu_driver.c: Use RFB keycode set instead of XT KBD * src/util/virkeycode-mapgen.py: Auto-generate the RFB keycode set from the XT KBD set * src/util/virkeycode.c: Add RFB keycode entry to table. Add a verify check on cardinality of the codeOffset table --- include/libvirt/libvirt.h.in | 1 + src/qemu/qemu_driver.c | 8 ++++---- src/util/virkeycode-mapgen.py | 12 ++++++++++++ src/util/virkeycode.c | 5 +++++ 4 files changed, 22 insertions(+), 4 deletions(-)
@@ -62,7 +63,10 @@ static unsigned int codeOffset[] = { offsetof(struct keycode, usb), [VIR_KEYCODE_SET_WIN32] = offsetof(struct keycode, win32), + [VIR_KEYCODE_SET_RFB] = + offsetof(struct keycode, rfb), }; +extern int (*codeOffsetVerify(void)) [verify_true (ARRAY_CARDINALITY(codeOffset) == VIR_KEYCODE_SET_LAST)]; \
Why the trailing backslash?
Also, open-coding this verify is dangerous; gcc warnings have changed over time, rendering certain verification styles that were once safe into something that trips up -Werror. It is safer (and shorter!) to let gnulib worry about gcc quirks, by changing this line to just be:
verify(ARRAY_CARDINALITY(codeOffset) == VIR_KEYCODE_SET_LAST);
Yeah it was a stupid cut+paste from src/util/util.h where we had to open-code it for the enums, because the gnulib macro resulted in clashing symbols when used more than one. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 08/26/2011 04:41 AM, Daniel P. Berrange wrote:
+extern int (*codeOffsetVerify(void)) [verify_true (ARRAY_CARDINALITY(codeOffset) == VIR_KEYCODE_SET_LAST)]; \
Why the trailing backslash?
Also, open-coding this verify is dangerous; gcc warnings have changed over time, rendering certain verification styles that were once safe into something that trips up -Werror. It is safer (and shorter!) to let gnulib worry about gcc quirks, by changing this line to just be:
verify(ARRAY_CARDINALITY(codeOffset) == VIR_KEYCODE_SET_LAST);
Yeah it was a stupid cut+paste from src/util/util.h where we had to open-code it for the enums, because the gnulib macro resulted in clashing symbols when used more than one.
Wow. That line dates from 2008, prior to my gnulib fixes to the verify module to make multiple verify() in one file work without triggering gcc complaints (by use of gcc's __COUNTER__ preprocessor magic). Patch coming up soon. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake