Re: [libvirt] [PATCH 10/10 V2] qemu:send-key: Implement the driver methods
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)