Re: [libvirt] [PATCH 3/3] qemu: Improve qemuMonitorJSONSendKey function

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(a)redhat.com&gt; > --- > 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. 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.