Re: [Qemu-devel] [PATCH 4/6] vmport_rpc: Add QMP access to vmport_rpc object.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4881 bytes --]

On 01/30/2015 02:06 PM, Don Slutz wrote:
> This adds two new inject commands:
> 
> inject-vmport-reboot
> inject-vmport-halt
> 
> And three guest info commands:
> 
> vmport-guestinfo-set
> vmport-guestinfo-get
> query-vmport-guestinfo
> 
> More details in qmp-commands.hx
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---

> +static int foreach_dynamic_VMPortRpc_device(FindVMPortRpcDeviceFunc *func,
> +                                            void *arg)
> +{
> +    VMPortRpcFind find = {
> +        .func = func,
> +        .arg = arg,
> +    };
> +
> +    /* Loop through all VMPortRpc devices that were spawened outside

s/spawened/spawned/


>  #ifdef VMPORT_RPC_DEBUG
>  /*
>   * Add helper function for traceing.  This routine will convert

Pre-existing as of this patch, but while you are here:
s/traceing/tracing/
unless it occurred earlier in the series, in which case fix it there.


> +static void convert_local_rc(Error **errp, int rc)
> +{
> +    switch (rc) {
> +    case 0:
> +        break;
> +    case VMPORT_DEVICE_NOT_FOUND:
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, TYPE_VMPORT_RPC);
> +        break;
> +    case SEND_NOT_OPEN:
> +        error_set(errp, ERROR_CLASS_GENERIC_ERROR, "VMWare rpc not open");

shorter as:
error_setg(errp, "VMWare rpc not open");

and similar for all your other uses of ERROR_CLASS_GENERIC_ERROR

> +++ b/qapi-schema.json
> @@ -1271,6 +1271,101 @@
>  { 'command': 'inject-nmi' }
>  
>  ##
> +# @inject-vmport-reboot:
> +#
> +# Injects a VMWare Tools reboot to the guest.
> +#
> +# Returns:  If successful, nothing
> +#
> +# Since:  2.3
> +#
> +##
> +{ 'command': 'inject-vmport-reboot' }
> +
> +##
> +# @inject-vmport-halt:
> +#
> +# Injects a VMWare Tools halt to the guest.
> +#
> +# Returns:  If successful, nothing
> +#
> +# Since:  2.3
> +#
> +##
> +{ 'command': 'inject-vmport-halt' }

Why two commands?  Why not just 'inject-vmport-action' with a parameter
that says whether the action is 'reboot', 'halt', or something else?

> +
> +##
> +# @vmport-guestinfo-set:
> +#
> +# Set a VMWare Tools guestinfo key to a value
> +#
> +# @key: the key to set
> +#
> +# @value: The data to set the key to
> +#
> +# @format: #optional value encoding (default 'utf8').
> +#          - base64: value must be base64 encoded text.  Its binary
> +#            decoding gets set.
> +#            Bug: invalid base64 is currently not rejected.

We should fix the bug, rather than document it.

> +#            Whitespace *is* invalid.
> +#          - utf8: value's UTF-8 encoding is written
> +#          - value itself is always Unicode regardless of format, like
> +#            any other string.

This was confusing to read - there are three bullets, so it looks like
you meant to document three valid DataFormat enum values; but there are
only two.  The comment about 'value' being supplied as valid JSON UTF8
and only later decoded according to 'format' might belong better on
'value', if at all.  On the other hand, I see you are just blindly
copy-and-pasting from 'ringbuf-write'.

> +#
> +# Returns: Nothing on success
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'vmport-guestinfo-set',
> +  'data': {'key': 'str', 'value': 'str',
> +           '*format': 'DataFormat'} }
> +
> +##
> +# @vmport-guestinfo-get:
> +#
> +# Get a VMWare Tools guestinfo value for a key
> +#
> +# @key: the key to get
> +#
> +# @format: #optional data encoding (default 'utf8').
> +#          - base64: the value is returned in base64 encoding.
> +#          - utf8: the value is interpreted as UTF-8.
> +#            Bug: can screw up when the buffer contains invalid UTF-8
> +#            sequences, NUL characters.
> +#          - The return value is always Unicode regardless of format,
> +#            like any other string.

Similar comments, but again sourced by copy-and-pasting from an existing
interface.

> +#
> +# Returns: value for the guest info key
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'vmport-guestinfo-get',
> +  'data': {'key': 'str', '*format': 'DataFormat'},
> +  'returns': 'str' }
> +
> +##
> +# @VmportGuestInfo:
> +#
> +# Information about a single VMWare Tools guestinfo
> +#
> +# @key: The known key
> +#
> +# Since: 2.3
> +##
> +{ 'type': 'VmportGuestInfo', 'data': {'key': 'str'} }
> +
> +##
> +# @query-vmport-guestinfo:
> +#
> +# Returns information about VMWare Tools guestinfo
> +#
> +# Returns: a list of @VmportGuestInfo
> +#
> +# Since: 2.3
> +##
> +{ 'command': 'query-vmport-guestinfo', 'returns': ['VmportGuestInfo'] }
> +
> +##

Interface seems more or less okay, since it copies from existing idioms.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]