On 06/13/2011 05:06 PM, Daniel P. Berrange wrote:
On Tue, Jun 07, 2011 at 05:11:17PM +0800, Lai Jiangshan wrote:
Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> --- src/qemu/qemu_driver.c | 50 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 27 ++++++++++++++++++++++ src/qemu/qemu_monitor.h | 6 +++++ src/qemu/qemu_monitor_json.c | 15 ++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++++ src/qemu/qemu_monitor_text.c | 47 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_text.h | 5 ++++ 7 files changed, 155 insertions(+), 0 deletions(-)
In addition to Daniel's comments:
+ + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + }
This check should be moved down...
+ + priv = vm->privateData; + + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) + goto cleanup;
...to here, since qemuDomainObjBeginJobWithDriver temporarily drops locks, and therefore vm could die before you get the lock.
+ qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorSendKey(priv->mon, codeset, holdtime, nkeycodes, keycodes);
Yuck. Let's fix the qemuMonitorSendKey parameter order to match the API order of keycodes before nkeycodes.
+int qemuMonitorSendKey(qemuMonitorPtr mon, + unsigned int codeset, + unsigned int holdtime, + unsigned int nkeycodes, + unsigned int *keycodes)
Fix this API...
+{ + int ret; + + VIR_DEBUG("mon=%p, codeset=%u, holdtime=%u, nkeycodes=%u", + mon, codeset, holdtime, nkeycodes); + + if (codeset != LIBVIRT_KEYCODE_XT) { + qemuReportError(VIR_ERR_NO_SUPPORT, + "qemu monitor can not support the codeset: %d", + codeset); + return -1; + }
Hmm, so your proposed virsh command defaults to --codeset linux, but right now you only support --codeset xt. That's kind of mean to the users. It would be nice to get the codeset translations going at the virsh level.
+ + if (mon->json) + ret = qemuMonitorJSONSendKey(mon, holdtime, nkeycodes, keycodes); + else + ret = qemuMonitorTextSendKey(mon, holdtime, nkeycodes, keycodes);
...and these callbacks, to do array before length.
+++ b/src/qemu/qemu_monitor_text.c @@ -2718,6 +2718,53 @@ fail: return -1; }
+int qemuMonitorTextSendKey(qemuMonitorPtr mon, + unsigned int holdtime, + unsigned int nkeycodes, + unsigned int *keycodes) +{ + int i; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *cmd, *reply = NULL; + + if (nkeycodes > 16 || nkeycodes == 0)
Magic number. Use VIR_DOMAIN_SEND_KEY_MAX_KEYS from libvirt.h. Or don't even bother to check - we already guaranteed that libvirt.c filtered out invalid calls, so by the time you get here, nkeycodes has already been validated to be in range.
+ return -1; + + virBufferAddLit(&buf, "sendkey "); + for (i = 0; i < nkeycodes; i++) { + if (keycodes[i] > 255) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("the %dth keycode is invalid: 0x%02X"), + i, keycodes[i]);
Printing %02X with a value that is > 255 will use more than 2 digits, at which point, you could have just used %X instead of %02X. Also, "1th" doesn't read well in English, let alone translate well to other languages. The error message should probably be: _("keycode %d is invalid: 0x%X") -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org