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

[Don Slutz]

On 01/30/15 17:32, Eric Blake wrote:
> 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 <address@hidden>
>> ---
> 
>> +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/
> 

Fixed.

> 
>>  #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.
> 

Fixed.

> 
>> +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
> 

Fixed.


>> +++ 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?
> 

This is a corner case.  I will convert it to inject-vmport-action with
a parameter.


>> +
>> +##
>> +# @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.
> 

Well, this turns out to be complex.  If there is a bug it is in GLIB.
However the Glib reference manual refers to RFC 1421 and RFC 2045 and
MIME encoding.  Based on all that (which seems to match:

http://en.wikipedia.org/wiki/Base64

) MIME states that all characters outside the (base64) alphabet are
to be ignored.  Testing shows that g_base64_decode() does this.

The confusion is that most non-MIME uses reject a base64 string
that contain characters outside the alphabet.  I was just following
the other uses of base64 in this file.

DataFormat refers to RFC 3548, which has the info:

"  Implementations MUST reject the encoding if it contains characters
   outside the base alphabet when interpreting base encoded data, unless
   the specification referring to this document explicitly states
   otherwise.  Such specifications may, as MIME does, instead state that
   characters outside the base encoding alphabet should simply be
   ignored when interpreting data ("be liberal in what you accept")."

So with GLIB going the MIME way, I do not think this is a QEMU bug
(you could consider this a GLIB bug, but the document I found says
that GLIB goes the MIME way and so does not reject anything).

Having now looked closer at this, I plan to drop this "bug" section
since I am happy with what GLIB is doing.


>> +#            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'.
>

Yup.  Will just drop the "- value..."

> 
>> +#
>> +# 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.
> 

Will adjust as above.

>> +#
>> +# 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.
> 
> 

Thanks,

   -Don Slutz