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@xxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list