[libvirt] [PATCH 10/10 V2] qemu:send-key: Implement the driver methods

[Eric Blake]

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 at 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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110614/612465d6/attachment-0001.sig>