[libvirt] [PATCH 0/3] qemu: Improve send-key command using input-send-event
QEMU added a qmp command input-send-event that is more precise about what the outcome for the guest is; it can control particular events as press/release of a key, button, move of a pointer, etc. This series is still waiting for two patches [1] from Amos Kong to be applied before it can be used and will not be pushed until that change is in upstream QEMU. Martin [1] https://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01187.html Martin Kletzander (3): qemu: Add capability probing for 'imput-send-event' qmp command util: Add virKeycodeValueIsModifier function qemu: Improve qemuMonitorJSONSendKey function src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 3 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_monitor.c | 6 +- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 142 ++++++++++++++++++++++++++++++++++++------- src/qemu/qemu_monitor_json.h | 3 +- src/util/virkeycode.c | 21 +++++++ src/util/virkeycode.h | 2 + tests/qemumonitorjsontest.c | 72 ++++++++++++++++++++-- 11 files changed, 226 insertions(+), 32 deletions(-) -- 2.1.3
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 74a3b24..9ada568 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -272,6 +272,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "migrate-rdma", "ivshmem", "drive-iotune-max", + + "input-send-event", /* 180 */ ); @@ -1436,6 +1438,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "nbd-server-start", QEMU_CAPS_NBD_SERVER }, { "change-backing-file", QEMU_CAPS_CHANGE_BACKING_FILE }, { "rtc-reset-reinjection", QEMU_CAPS_RTC_RESET_REINJECTION }, + { "input-send-event", QEMU_CAPS_INPUT_EVENT }, }; struct virQEMUCapsStringFlags virQEMUCapsMigration[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index ffe3494..b4c5390 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -219,6 +219,7 @@ typedef enum { QEMU_CAPS_MIGRATE_RDMA = 177, /* have rdma migration */ QEMU_CAPS_DEVICE_IVSHMEM = 178, /* -device ivshmem */ QEMU_CAPS_DRIVE_IOTUNE_MAX = 179, /* -drive bps_max= and friends */ + QEMU_CAPS_INPUT_EVENT = 180, /* input-send-event qmp command */ QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; -- 2.1.3
On 11/11/14 15:54, Martin Kletzander wrote:
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 4 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 74a3b24..9ada568 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -272,6 +272,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "migrate-rdma", "ivshmem", "drive-iotune-max", + + "input-send-event", /* 180 */ );
@@ -1436,6 +1438,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "nbd-server-start", QEMU_CAPS_NBD_SERVER }, { "change-backing-file", QEMU_CAPS_CHANGE_BACKING_FILE }, { "rtc-reset-reinjection", QEMU_CAPS_RTC_RESET_REINJECTION }, + { "input-send-event", QEMU_CAPS_INPUT_EVENT }, };
struct virQEMUCapsStringFlags virQEMUCapsMigration[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index ffe3494..b4c5390 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -219,6 +219,7 @@ typedef enum { QEMU_CAPS_MIGRATE_RDMA = 177, /* have rdma migration */ QEMU_CAPS_DEVICE_IVSHMEM = 178, /* -device ivshmem */ QEMU_CAPS_DRIVE_IOTUNE_MAX = 179, /* -drive bps_max= and friends */ + QEMU_CAPS_INPUT_EVENT = 180, /* input-send-event qmp command */
<bikeshedding> I'd add the word send here too, as this might clash with a possible future addition of an event. As the enum value name is internal it can be changed in the future though and the stable name string of the capability is okay. </bikeshedding>
QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags;
ACK, Peter
s/imput/input/ Jan
This function returns true if the value supplied is a modifier (Ctrl, Shift, Alt or Meta). Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virkeycode.c | 21 +++++++++++++++++++++ src/util/virkeycode.h | 2 ++ 3 files changed, 24 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b8f35e8..5c3de01 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1530,6 +1530,7 @@ virJSONValueToString; virKeycodeSetTypeFromString; virKeycodeSetTypeToString; virKeycodeValueFromString; +virKeycodeValueIsModifier; virKeycodeValueTranslate; diff --git a/src/util/virkeycode.c b/src/util/virkeycode.c index 7880a0a..7705ffd 100644 --- a/src/util/virkeycode.c +++ b/src/util/virkeycode.c @@ -1,4 +1,5 @@ /* + * Copyright (C) 2014 Red Hat, Inc. * Copyright (c) 2011 Lai Jiangshan * * This library is free software; you can redistribute it and/or @@ -124,3 +125,23 @@ int virKeycodeValueTranslate(virKeycodeSet from_codeset, return -1; } + + +bool +virKeycodeValueIsModifier(unsigned int key_value) +{ + switch (key_value) { + case 29: /* Left Control */ + case 157: /* Right Control */ + case 42: /* Left Shift */ + case 54: /* Right Shift */ + case 56: /* Left Alt */ + case 184: /* Right Alt */ + case 219: /* Left Meta */ + case 220: /* Right Meta */ + return true; + + default: + return false; + } +} diff --git a/src/util/virkeycode.h b/src/util/virkeycode.h index 6947cfe..d04a2a4 100644 --- a/src/util/virkeycode.h +++ b/src/util/virkeycode.h @@ -1,6 +1,7 @@ /* * virkeycode.h: keycodes definitions and declarations * + * Copyright (C) 2014 Red Hat, Inc. * Copyright (c) 2011 Lai Jiangshan * * This library is free software; you can redistribute it and/or @@ -29,5 +30,6 @@ int virKeycodeValueFromString(virKeycodeSet codeset, const char *keyname); int virKeycodeValueTranslate(virKeycodeSet from_codeset, virKeycodeSet to_offset, int key_value); +bool virKeycodeValueIsModifier(unsigned int key_value); #endif -- 2.1.3
On 11/11/14 15:54, Martin Kletzander wrote:
This function returns true if the value supplied is a modifier (Ctrl, Shift, Alt or Meta).
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virkeycode.c | 21 +++++++++++++++++++++ src/util/virkeycode.h | 2 ++ 3 files changed, 24 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b8f35e8..5c3de01 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1530,6 +1530,7 @@ virJSONValueToString; virKeycodeSetTypeFromString; virKeycodeSetTypeToString; virKeycodeValueFromString; +virKeycodeValueIsModifier; virKeycodeValueTranslate;
diff --git a/src/util/virkeycode.c b/src/util/virkeycode.c index 7880a0a..7705ffd 100644 --- a/src/util/virkeycode.c +++ b/src/util/virkeycode.c @@ -1,4 +1,5 @@ /* + * Copyright (C) 2014 Red Hat, Inc. * Copyright (c) 2011 Lai Jiangshan * * This library is free software; you can redistribute it and/or @@ -124,3 +125,23 @@ int virKeycodeValueTranslate(virKeycodeSet from_codeset,
return -1; } + + +bool +virKeycodeValueIsModifier(unsigned int key_value) +{ + switch (key_value) { + case 29: /* Left Control */ + case 157: /* Right Control */ + case 42: /* Left Shift */ + case 54: /* Right Shift */ + case 56: /* Left Alt */ + case 184: /* Right Alt */ + case 219: /* Left Meta */ + case 220: /* Right Meta */ + return true;
I've checked those with a random keymap that I found on the internet :)
+ + default: + return false; + } +} diff --git a/src/util/virkeycode.h b/src/util/virkeycode.h index 6947cfe..d04a2a4 100644 --- a/src/util/virkeycode.h +++ b/src/util/virkeycode.h @@ -1,6 +1,7 @@ /* * virkeycode.h: keycodes definitions and declarations * + * Copyright (C) 2014 Red Hat, Inc. * Copyright (c) 2011 Lai Jiangshan * * This library is free software; you can redistribute it and/or @@ -29,5 +30,6 @@ int virKeycodeValueFromString(virKeycodeSet codeset, const char *keyname); int virKeycodeValueTranslate(virKeycodeSet from_codeset, virKeycodeSet to_offset, int key_value); +bool virKeycodeValueIsModifier(unsigned int key_value);
#endif
ACK, Peter
When using modifiers with send-key, then we cannot know with what keys those modifiers should be pressed down. QEMU changed the order of the release events few times and that caused few send-key command to work differently than expected. We already state in virsh man page that the keys sent will be sent to the guest simultaneously and can be received in random order. This patch just tries improving user experience and keeping old behaviour. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_monitor.c | 6 +- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 142 ++++++++++++++++++++++++++++++++++++------- src/qemu/qemu_monitor_json.h | 3 +- tests/qemumonitorjsontest.c | 72 ++++++++++++++++++++-- 6 files changed, 198 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 56e8430..bdf99eb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2589,7 +2589,9 @@ static int qemuDomainSendKey(virDomainPtr domain, } qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorSendKey(priv->mon, holdtime, keycodes, nkeycodes); + ret = qemuMonitorSendKey(priv->mon, holdtime, keycodes, nkeycodes, + virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_INPUT_EVENT)); qemuDomainObjExitMonitor(driver, vm); endjob: diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 276649d..4f128a1 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3431,7 +3431,8 @@ int qemuMonitorInjectNMI(qemuMonitorPtr mon) int qemuMonitorSendKey(qemuMonitorPtr mon, unsigned int holdtime, unsigned int *keycodes, - unsigned int nkeycodes) + unsigned int nkeycodes, + bool events) { int ret; @@ -3439,7 +3440,8 @@ int qemuMonitorSendKey(qemuMonitorPtr mon, mon, holdtime, nkeycodes); if (mon->json) - ret = qemuMonitorJSONSendKey(mon, holdtime, keycodes, nkeycodes); + ret = qemuMonitorJSONSendKey(mon, holdtime, keycodes, nkeycodes, + events); else ret = qemuMonitorTextSendKey(mon, holdtime, keycodes, nkeycodes); return ret; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 76c91a3..14c6347 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -740,7 +740,8 @@ int qemuMonitorScreendump(qemuMonitorPtr mon, int qemuMonitorSendKey(qemuMonitorPtr mon, unsigned int holdtime, unsigned int *keycodes, - unsigned int nkeycodes); + unsigned int nkeycodes, + bool events); typedef enum { BLOCK_JOB_ABORT, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 91a7aba..cd8a38b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -44,6 +44,7 @@ #include "virprobe.h" #include "virstring.h" #include "cpu/cpu_x86.h" +#include "virkeycode.h" #ifdef WITH_DTRACE_PROBES # include "libvirt_qemu_probes.h" @@ -3938,52 +3939,151 @@ int qemuMonitorJSONInjectNMI(qemuMonitorPtr mon) return ret; } +static int +qemuMonitorJSONAppendKey(virJSONValuePtr list, + unsigned int keycode, + bool use_events, + bool down) +{ + virJSONValuePtr data = NULL; + virJSONValuePtr event = NULL; + virJSONValuePtr key = NULL; + + if (!(key = virJSONValueNewObject())) + goto cleanup; + + /* Union KeyValue has two types, use the generic one */ + if (virJSONValueObjectAppendString(key, "type", "number") < 0) + goto cleanup; + + /* with the keycode */ + if (virJSONValueObjectAppendNumberInt(key, "data", keycode) < 0) + goto cleanup; + + if (!use_events) { + if (virJSONValueArrayAppend(list, key) < 0) + goto cleanup; + + /* That's all if we're not using 'input-send-event'. */ + return 0; + } + + if (!(event = virJSONValueNewObject()) || + !(data = virJSONValueNewObject())) + goto cleanup; + + if (virJSONValueObjectAppendBoolean(data, "down", down) < 0) + goto cleanup; + + if (virJSONValueObjectAppend(data, "key", key) < 0) + goto cleanup; + + key = NULL; + + if (virJSONValueObjectAppendString(event, "type", "key") < 0) + goto cleanup; + + if (virJSONValueObjectAppend(event, "data", data) < 0) + goto cleanup; + + data = NULL; + + if (virJSONValueArrayAppend(list, event) < 0) + goto cleanup; + + event = NULL; + + return 0; + + cleanup: + virJSONValueFree(data); + virJSONValueFree(event); + virJSONValueFree(key); + return -1; +} + int qemuMonitorJSONSendKey(qemuMonitorPtr mon, unsigned int holdtime, unsigned int *keycodes, - unsigned int nkeycodes) + unsigned int nkeycodes, + bool events) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; virJSONValuePtr keys = NULL; - virJSONValuePtr key = NULL; - size_t i; + ssize_t i; + size_t nreverts = 0; + unsigned int *reverts = NULL; + + + /* + * We are trying to use input-send-event to control press/release + * events more precisely, but that API doesn't support the + * 'holdtime' parameter (obviously). Due to backward + * compatibility, we need to keep the 'holdtime' parameter working + * as that is the thing that wasn't broken (yet), unlike the order + * of press/release events 'send-key' sends to the guest. + * + * And we cannot combine 'input-send-event' and 'send-key' because + * if there's any error between sending press and release events, + * some modifiers might be left pressed. + */ + if (events && holdtime) { + events = false; + VIR_WARN("Using only send-key even though " + "this QEMU binary supports input-send-event"); + } /* create the key data array */ if (!(keys = virJSONValueNewArray())) goto cleanup; for (i = 0; i < nkeycodes; i++) { + bool modifier = virKeycodeValueIsModifier(keycodes[i]); + if (keycodes[i] > 0xffff) { virReportError(VIR_ERR_OPERATION_FAILED, _("keycode %zu is invalid: 0x%X"), i, keycodes[i]); goto cleanup; } - /* create single key object */ - if (!(key = virJSONValueNewObject())) + if (qemuMonitorJSONAppendKey(keys, keycodes[i], + events, true) < 0) goto cleanup; - /* Union KeyValue has two types, use the generic one */ - if (virJSONValueObjectAppendString(key, "type", "number") < 0) - goto cleanup; - - /* with the keycode */ - if (virJSONValueObjectAppendNumberInt(key, "data", keycodes[i]) < 0) - goto cleanup; + if (events) { + if (modifier) { + if (VIR_APPEND_ELEMENT_COPY(reverts, nreverts, keycodes[i]) < 0) + goto cleanup; + } else { + if (qemuMonitorJSONAppendKey(keys, keycodes[i], + events, false) < 0) + goto cleanup; + } + } + } - if (virJSONValueArrayAppend(keys, key) < 0) + /* + * At the end, release all modifiers in reversed order. + */ + for (i = nreverts; i > 0; i--) { + if (qemuMonitorJSONAppendKey(keys, keycodes[i - 1], + events, false) < 0) goto cleanup; + } - key = NULL; - + if (events) { + cmd = qemuMonitorJSONMakeCommand("input-send-event", + "a:events", keys, + NULL); + } else { + cmd = qemuMonitorJSONMakeCommand("send-key", + "a:keys", keys, + "p:hold-time", holdtime, + NULL); } - cmd = qemuMonitorJSONMakeCommand("send-key", - "a:keys", keys, - "p:hold-time", holdtime, - NULL); if (!cmd) goto cleanup; @@ -3994,17 +4094,17 @@ int qemuMonitorJSONSendKey(qemuMonitorPtr mon, goto cleanup; if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - VIR_DEBUG("send-key command not found, trying HMP"); + VIR_DEBUG("send-key (input-send-event) command not found, trying HMP"); ret = qemuMonitorTextSendKey(mon, holdtime, keycodes, nkeycodes); } else { ret = qemuMonitorJSONCheckError(cmd, reply); } cleanup: + VIR_FREE(reverts); virJSONValueFree(cmd); virJSONValueFree(reply); virJSONValueFree(keys); - virJSONValueFree(key); return ret; } diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 3b6159a..1ddbc65 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -294,7 +294,8 @@ int qemuMonitorJSONInjectNMI(qemuMonitorPtr mon); int qemuMonitorJSONSendKey(qemuMonitorPtr mon, unsigned int holdtime, unsigned int *keycodes, - unsigned int nkeycodes); + unsigned int nkeycodes, + bool events); int qemuMonitorJSONScreendump(qemuMonitorPtr mon, const char *file); diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 1b202fb..fc40d67 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1964,7 +1964,8 @@ testQemuMonitorJSONqemuMonitorJSONSendKey(const void *data) goto cleanup; if (qemuMonitorJSONSendKey(qemuMonitorTestGetMonitor(test), - 0, keycodes, ARRAY_CARDINALITY(keycodes)) < 0) + 0, keycodes, ARRAY_CARDINALITY(keycodes), + false) < 0) goto cleanup; ret = 0; @@ -1980,6 +1981,10 @@ testQemuMonitorJSONqemuMonitorJSONSendKeyHoldtime(const void *data) qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); int ret = -1; unsigned int keycodes[] = {43, 26, 46, 32}; + const char *keys = "[{\"type\":\"number\",\"data\":43}," + "{\"type\":\"number\",\"data\":26}," + "{\"type\":\"number\",\"data\":46}," + "{\"type\":\"number\",\"data\":32}]"; if (!test) return -1; @@ -1987,16 +1992,70 @@ testQemuMonitorJSONqemuMonitorJSONSendKeyHoldtime(const void *data) if (qemuMonitorTestAddItemParams(test, "send-key", "{\"return\":{}}", "hold-time", "31337", - "keys", "[{\"type\":\"number\",\"data\":43}," - "{\"type\":\"number\",\"data\":26}," - "{\"type\":\"number\",\"data\":46}," - "{\"type\":\"number\",\"data\":32}]", + "keys", keys, NULL, NULL) < 0) goto cleanup; + if (qemuMonitorTestAddItemParams(test, "send-key", + "{\"return\":{}}", + "hold-time", "31337", + "keys", keys, + NULL, NULL) < 0) + goto cleanup; + + if (qemuMonitorJSONSendKey(qemuMonitorTestGetMonitor(test), + 31337, keycodes, + ARRAY_CARDINALITY(keycodes), false) < 0) + goto cleanup; + if (qemuMonitorJSONSendKey(qemuMonitorTestGetMonitor(test), 31337, keycodes, - ARRAY_CARDINALITY(keycodes)) < 0) + ARRAY_CARDINALITY(keycodes), true) < 0) + goto cleanup; + + ret = 0; + cleanup: + qemuMonitorTestFree(test); + return ret; +} + +static int +testQemuMonitorJSONqemuMonitorJSONSendKeyWithEvents(const void *data) +{ + virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; + qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); + int ret = -1; + unsigned int keycodes[] = {157, 70, 70}; + + if (!test) + return -1; + + if (qemuMonitorTestAddItemParams(test, "input-send-event", + "{\"return\":{}}", + "events", "[" + "{\"type\":\"key\",\"data\":{\"down\":true" + ",\"key\":{\"type\":\"number\"," + "\"data\":157}}}," + "{\"type\":\"key\",\"data\":{\"down\":true" + ",\"key\":{\"type\":\"number\"," + "\"data\":70}}}," + "{\"type\":\"key\",\"data\":{\"down\":false" + ",\"key\":{\"type\":\"number\"," + "\"data\":70}}}," + "{\"type\":\"key\",\"data\":{\"down\":true" + ",\"key\":{\"type\":\"number\"," + "\"data\":70}}}," + "{\"type\":\"key\",\"data\":{\"down\":false" + ",\"key\":{\"type\":\"number\"," + "\"data\":70}}}," + "{\"type\":\"key\",\"data\":{\"down\":false" + ",\"key\":{\"type\":\"number\"," + "\"data\":157}}}]", + NULL, NULL) < 0) + goto cleanup; + + if (qemuMonitorJSONSendKey(qemuMonitorTestGetMonitor(test), 0, keycodes, + ARRAY_CARDINALITY(keycodes), true) < 0) goto cleanup; ret = 0; @@ -2393,6 +2452,7 @@ mymain(void) DO_TEST(qemuMonitorJSONGetCPUInfo); DO_TEST(qemuMonitorJSONGetVirtType); DO_TEST(qemuMonitorJSONSendKey); + DO_TEST(qemuMonitorJSONSendKeyWithEvents); DO_TEST(qemuMonitorJSONGetDumpGuestMemoryCapability); DO_TEST(qemuMonitorJSONSendKeyHoldtime); DO_TEST(qemuMonitorSupportsActiveCommit); -- 2.1.3
On 11/11/14 15:54, Martin Kletzander wrote:
When using modifiers with send-key, then we cannot know with what keys those modifiers should be pressed down. QEMU changed the order of the release events few times and that caused few send-key command to work differently than expected.
We already state in virsh man page that the keys sent will be sent to the guest simultaneously and can be received in random order. This patch just tries improving user experience and keeping old behaviour.
Although we state this in the virsh man page, the API reference section on the SendKey function is pretty poor. We should improve and state a few things there I'll point out later.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_monitor.c | 6 +- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 142 ++++++++++++++++++++++++++++++++++++------- src/qemu/qemu_monitor_json.h | 3 +- tests/qemumonitorjsontest.c | 72 ++++++++++++++++++++-- 6 files changed, 198 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 91a7aba..cd8a38b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c
@@ -3938,52 +3939,151 @@ int qemuMonitorJSONInjectNMI(qemuMonitorPtr mon) return ret; }
+static int +qemuMonitorJSONAppendKey(virJSONValuePtr list, + unsigned int keycode, + bool use_events, + bool down) +{ + virJSONValuePtr data = NULL; + virJSONValuePtr event = NULL; + virJSONValuePtr key = NULL; + + if (!(key = virJSONValueNewObject())) + goto cleanup; + + /* Union KeyValue has two types, use the generic one */ + if (virJSONValueObjectAppendString(key, "type", "number") < 0) + goto cleanup; + + /* with the keycode */ + if (virJSONValueObjectAppendNumberInt(key, "data", keycode) < 0) + goto cleanup; + + if (!use_events) { + if (virJSONValueArrayAppend(list, key) < 0) + goto cleanup; + + /* That's all if we're not using 'input-send-event'. */ + return 0; + } + + if (!(event = virJSONValueNewObject()) || + !(data = virJSONValueNewObject())) + goto cleanup; + + if (virJSONValueObjectAppendBoolean(data, "down", down) < 0) + goto cleanup; + + if (virJSONValueObjectAppend(data, "key", key) < 0) + goto cleanup; + + key = NULL; + + if (virJSONValueObjectAppendString(event, "type", "key") < 0) + goto cleanup; + + if (virJSONValueObjectAppend(event, "data", data) < 0) + goto cleanup; + + data = NULL; + + if (virJSONValueArrayAppend(list, event) < 0) + goto cleanup; + + event = NULL; + + return 0; + + cleanup: + virJSONValueFree(data); + virJSONValueFree(event); + virJSONValueFree(key); + return -1; +} + int qemuMonitorJSONSendKey(qemuMonitorPtr mon, unsigned int holdtime, unsigned int *keycodes, - unsigned int nkeycodes) + unsigned int nkeycodes, + bool events) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; virJSONValuePtr keys = NULL; - virJSONValuePtr key = NULL; - size_t i; + ssize_t i; + size_t nreverts = 0; + unsigned int *reverts = NULL; + + + /* + * We are trying to use input-send-event to control press/release + * events more precisely, but that API doesn't support the + * 'holdtime' parameter (obviously). Due to backward + * compatibility, we need to keep the 'holdtime' parameter working + * as that is the thing that wasn't broken (yet), unlike the order + * of press/release events 'send-key' sends to the guest. + * + * And we cannot combine 'input-send-event' and 'send-key' because + * if there's any error between sending press and release events, + * some modifiers might be left pressed. + */ + if (events && holdtime) { + events = false; + VIR_WARN("Using only send-key even though " + "this QEMU binary supports input-send-event");
Using the API extensively may result in log pollution. How about adding a bool into qemuMonitorPtr to suppress the repeated warnings for a certain instance of a VM?
+ }
/* create the key data array */ if (!(keys = virJSONValueNewArray())) goto cleanup;
for (i = 0; i < nkeycodes; i++) { + bool modifier = virKeycodeValueIsModifier(keycodes[i]);
So here you check if a specific key sent by the user is a modifier in order as the keys were specified. Thus sending [ctrl] s [shift] s is added as: ctrl down, 's' down, 's' up, shift down, 's' down, 's' up, shift up, ctrl up. resulting into a ^s^S The documentation should state that the modifier keys are pressed in the order they were specified along with other keys in the input array. Unless you need to also release some keys as part of the key combo this should be fine for the default use case and releasing a modifier is possible by calling the command multiple times and re-specifying the correct modifiers. This should be documented. One thing that can't be expressed is if a certain combination would require "holding" down one modifier key, while a second one would need to be toggled, but I don't know of any of those :) Second issue that can happen is that a user specifies a modifier multiple times which would generate multiple down events followed by multiple up events. I think we shouldn't allow those. As an additional thought: With a special flag we could specify that specifying the modifier more that once toggles it's state. (and the state is reverted to up regardless of the number of times it was specified) This is a future thought though. As said, taking parts of the virsh docs and using them in the API docs would be an great improvement in case of this API.
+ if (keycodes[i] > 0xffff) { virReportError(VIR_ERR_OPERATION_FAILED, _("keycode %zu is invalid: 0x%X"), i, keycodes[i]); goto cleanup; }
- /* create single key object */ - if (!(key = virJSONValueNewObject())) + if (qemuMonitorJSONAppendKey(keys, keycodes[i], + events, true) < 0) goto cleanup;
- /* Union KeyValue has two types, use the generic one */ - if (virJSONValueObjectAppendString(key, "type", "number") < 0) - goto cleanup; - - /* with the keycode */ - if (virJSONValueObjectAppendNumberInt(key, "data", keycodes[i]) < 0) - goto cleanup; + if (events) { + if (modifier) { + if (VIR_APPEND_ELEMENT_COPY(reverts, nreverts, keycodes[i]) < 0) + goto cleanup; + } else { + if (qemuMonitorJSONAppendKey(keys, keycodes[i], + events, false) < 0) + goto cleanup; + } + } + }
- if (virJSONValueArrayAppend(keys, key) < 0) + /* + * At the end, release all modifiers in reversed order. + */ + for (i = nreverts; i > 0; i--) { + if (qemuMonitorJSONAppendKey(keys, keycodes[i - 1], + events, false) < 0) goto cleanup; + }
- key = NULL; - + if (events) { + cmd = qemuMonitorJSONMakeCommand("input-send-event", + "a:events", keys, + NULL); + } else { + cmd = qemuMonitorJSONMakeCommand("send-key", + "a:keys", keys, + "p:hold-time", holdtime, + NULL); }
- cmd = qemuMonitorJSONMakeCommand("send-key", - "a:keys", keys, - "p:hold-time", holdtime, - NULL); if (!cmd) goto cleanup;
@@ -3994,17 +4094,17 @@ int qemuMonitorJSONSendKey(qemuMonitorPtr mon, goto cleanup;
if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - VIR_DEBUG("send-key command not found, trying HMP"); + VIR_DEBUG("send-key (input-send-event) command not found, trying HMP"); ret = qemuMonitorTextSendKey(mon, holdtime, keycodes, nkeycodes); } else { ret = qemuMonitorJSONCheckError(cmd, reply); }
cleanup: + VIR_FREE(reverts); virJSONValueFree(cmd); virJSONValueFree(reply); virJSONValueFree(keys); - virJSONValueFree(key); return ret; }
Overall looks good. I'd be glad if you could document few of the nuances of this API as a part of this patch and forbid the use of repeated multipliers in case the event is used. Peter
On Wed, Nov 12, 2014 at 11:50:18AM +0100, Peter Krempa wrote:
On 11/11/14 15:54, Martin Kletzander wrote:
When using modifiers with send-key, then we cannot know with what keys those modifiers should be pressed down. QEMU changed the order of the release events few times and that caused few send-key command to work differently than expected.
We already state in virsh man page that the keys sent will be sent to the guest simultaneously and can be received in random order. This patch just tries improving user experience and keeping old behaviour.
Although we state this in the virsh man page, the API reference section on the SendKey function is pretty poor. We should improve and state a few things there I'll point out later.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 4 +- src/qemu/qemu_monitor.c | 6 +- src/qemu/qemu_monitor.h | 3 +- src/qemu/qemu_monitor_json.c | 142 ++++++++++++++++++++++++++++++++++++------- src/qemu/qemu_monitor_json.h | 3 +- tests/qemumonitorjsontest.c | 72 ++++++++++++++++++++-- 6 files changed, 198 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 91a7aba..cd8a38b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c
@@ -3938,52 +3939,151 @@ int qemuMonitorJSONInjectNMI(qemuMonitorPtr mon) return ret; }
+static int +qemuMonitorJSONAppendKey(virJSONValuePtr list, + unsigned int keycode, + bool use_events, + bool down) +{ + virJSONValuePtr data = NULL; + virJSONValuePtr event = NULL; + virJSONValuePtr key = NULL; + + if (!(key = virJSONValueNewObject())) + goto cleanup; + + /* Union KeyValue has two types, use the generic one */ + if (virJSONValueObjectAppendString(key, "type", "number") < 0) + goto cleanup; + + /* with the keycode */ + if (virJSONValueObjectAppendNumberInt(key, "data", keycode) < 0) + goto cleanup; + + if (!use_events) { + if (virJSONValueArrayAppend(list, key) < 0) + goto cleanup; + + /* That's all if we're not using 'input-send-event'. */ + return 0; + } + + if (!(event = virJSONValueNewObject()) || + !(data = virJSONValueNewObject())) + goto cleanup; + + if (virJSONValueObjectAppendBoolean(data, "down", down) < 0) + goto cleanup; + + if (virJSONValueObjectAppend(data, "key", key) < 0) + goto cleanup; + + key = NULL; + + if (virJSONValueObjectAppendString(event, "type", "key") < 0) + goto cleanup; + + if (virJSONValueObjectAppend(event, "data", data) < 0) + goto cleanup; + + data = NULL; + + if (virJSONValueArrayAppend(list, event) < 0) + goto cleanup; + + event = NULL; + + return 0; + + cleanup: + virJSONValueFree(data); + virJSONValueFree(event); + virJSONValueFree(key); + return -1; +} + int qemuMonitorJSONSendKey(qemuMonitorPtr mon, unsigned int holdtime, unsigned int *keycodes, - unsigned int nkeycodes) + unsigned int nkeycodes, + bool events) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; virJSONValuePtr keys = NULL; - virJSONValuePtr key = NULL; - size_t i; + ssize_t i; + size_t nreverts = 0; + unsigned int *reverts = NULL; + + + /* + * We are trying to use input-send-event to control press/release + * events more precisely, but that API doesn't support the + * 'holdtime' parameter (obviously). Due to backward + * compatibility, we need to keep the 'holdtime' parameter working + * as that is the thing that wasn't broken (yet), unlike the order + * of press/release events 'send-key' sends to the guest. + * + * And we cannot combine 'input-send-event' and 'send-key' because + * if there's any error between sending press and release events, + * some modifiers might be left pressed. + */ + if (events && holdtime) { + events = false; + VIR_WARN("Using only send-key even though " + "this QEMU binary supports input-send-event");
Using the API extensively may result in log pollution. How about adding a bool into qemuMonitorPtr to suppress the repeated warnings for a certain instance of a VM?
+ }
/* create the key data array */ if (!(keys = virJSONValueNewArray())) goto cleanup;
for (i = 0; i < nkeycodes; i++) { + bool modifier = virKeycodeValueIsModifier(keycodes[i]);
So here you check if a specific key sent by the user is a modifier in order as the keys were specified.
Thus sending [ctrl] s [shift] s is added as:
ctrl down, 's' down, 's' up, shift down, 's' down, 's' up, shift up, ctrl up.
resulting into a ^s^S
Yes, we state that if you want '^sS' you should use multiple SendKey calls.
The documentation should state that the modifier keys are pressed in the order they were specified along with other keys in the input array.
Unless you need to also release some keys as part of the key combo this should be fine for the default use case and releasing a modifier is possible by calling the command multiple times and re-specifying the correct modifiers. This should be documented.
It is, only in virsh man page, though. I agree that it could be part of the API doc.
One thing that can't be expressed is if a certain combination would require "holding" down one modifier key, while a second one would need to be toggled, but I don't know of any of those :)
Me neither, but anyway, this wasn't possible before using send-key and it still doesn't forbid you to do anything like that when connected to the domain's console.
Second issue that can happen is that a user specifies a modifier multiple times which would generate multiple down events followed by multiple up events. I think we shouldn't allow those.
But that worked before, and also that's what qemu does anyway with send-key; and we can't guess what the user meant by that.
As an additional thought: With a special flag we could specify that specifying the modifier more that once toggles it's state. (and the state is reverted to up regardless of the number of times it was specified) This is a future thought though.
It looks like an API abuse, new API would make more sense for this kind of stuff, and it could also support other event types (e.g. relative/absolute mouse movement, mouse buttons).
As said, taking parts of the virsh docs and using them in the API docs would be an great improvement in case of this API.
Overall looks good. I'd be glad if you could document few of the nuances of this API as a part of this patch and forbid the use of repeated multipliers in case the event is used.
I'd love to, *but*... There would be many ifs that would just confuse users and developers. Imagine a documentation that starts: This API does not guarantee any of the following features unless you are using QEMU/KVM and the binary used supports 'input-send-event' API. There's also no guarantee if you are using 'holdtime' parameter. <bunch of super cool features follows> Not mentioning that after first change in this code it would not be true any more (again). I was trying to just keep a bit more of backward compatibility. What you suggests can be exposed as a new API, which I'd be fine with, but was out of scope of this series. Adding features you mentioned to SendKey API doesn't make sense for me due to the specificity of those features. And I guess send-key is just used for stuff like Ctrl-Alt-Del and similar when you don't have any console connection, anyway. Update: I tried to do this in the SendKey API, but just realized a while back, that it's not needed for the use-case I wanted it to. I'll add it as an API later on unless if there are no objections. Martin
participants (3)
-
Ján Tomko -
Martin Kletzander -
Peter Krempa