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

2015-02-02 Thread 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 dsl...@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/
 

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 

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

2015-01-30 Thread Eric Blake
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 dsl...@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



signature.asc
Description: OpenPGP digital signature


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

2015-01-30 Thread Don Slutz
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 dsl...@verizon.com
---
 hw/misc/vmport_rpc.c | 275 +++
 qapi-schema.json |  95 ++
 qmp-commands.hx  | 141 ++
 3 files changed, 511 insertions(+)

diff --git a/hw/misc/vmport_rpc.c b/hw/misc/vmport_rpc.c
index e12f912..74cc95e 100644
--- a/hw/misc/vmport_rpc.c
+++ b/hw/misc/vmport_rpc.c
@@ -218,6 +218,56 @@ typedef struct {
 uint32_t edi;
 } vregs;
 
+/*
+ * Run func() for every VMPortRpc device, traverse the tree for
+ * everything else.  Note: This routine expects that opaque is a
+ * VMPortRpcFind pointer and not NULL.
+ */
+static int find_VMPortRpc_device(Object *obj, void *opaque)
+{
+VMPortRpcFind *find = opaque;
+Object *dev;
+VMPortRpcState *s;
+
+if (find-found) {
+return 0;
+}
+dev = object_dynamic_cast(obj, TYPE_VMPORT_RPC);
+s = (VMPortRpcState *)dev;
+
+if (!s) {
+/* Container, traverse it for children */
+return object_child_foreach(obj, find_VMPortRpc_device, opaque);
+}
+
+find-found++;
+find-rc = find-func(s, find-arg);
+
+return 0;
+}
+
+/*
+ * Loop through all dynamically created VMPortRpc devices and call
+ * func() for each instance.
+ */
+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
+ * the machine */
+find_VMPortRpc_device(qdev_get_machine(), find);
+if (find.found) {
+return find.rc;
+} else {
+return VMPORT_DEVICE_NOT_FOUND;
+}
+}
+
 #ifdef VMPORT_RPC_DEBUG
 /*
  * Add helper function for traceing.  This routine will convert
@@ -465,6 +515,26 @@ static int get_guestinfo(VMPortRpcState *s,
 return GUESTINFO_NOTFOUND;
 }
 
+static int get_qmp_guestinfo(VMPortRpcState *s,
+ unsigned int a_key_len, char *a_info_key,
+ unsigned int *a_value_len, void **a_value_data)
+{
+unsigned int i;
+
+for (i = 0; i  s-used_guestinfo; i++) {
+if (s-guestinfo[i] 
+(s-guestinfo[i]-key_len == a_key_len) 
+(memcmp(a_info_key, s-guestinfo[i]-key_data,
+s-guestinfo[i]-key_len) == 0)) {
+*a_value_len = s-guestinfo[i]-val_len;
+*a_value_data = s-guestinfo[i]-val_data;
+return 0;
+}
+}
+
+return GUESTINFO_NOTFOUND;
+}
+
 static int set_guestinfo(VMPortRpcState *s, int a_key_len,
  unsigned int a_val_len, char *a_info_key, char *val)
 {
@@ -864,6 +934,210 @@ static uint32_t vmport_rpc_ioport_read(void *opaque, 
uint32_t addr)
 return ur.data[0];
 }
 
+static int find_send(VMPortRpcState *s, void *arg)
+{
+return ctrl_send(s, arg);
+}
+
+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);
+break;
+case SEND_SKIPPED:
+error_set(errp, ERROR_CLASS_GENERIC_ERROR, VMWare rpc send skipped);
+break;
+case SEND_TRUCATED:
+error_set(errp, ERROR_CLASS_GENERIC_ERROR, VMWare rpc send trucated);
+break;
+case SEND_NO_MEMORY:
+error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+  VMWare rpc send out of memory);
+break;
+case GUESTINFO_NOTFOUND:
+error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+  VMWare guestinfo not found);
+break;
+case GUESTINFO_VALTOOLONG:
+error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+  VMWare guestinfo value too long);
+break;
+case GUESTINFO_KEYTOOLONG:
+error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+  VMWare guestinfo key too long);
+break;
+case GUESTINFO_TOOMANYKEYS:
+error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+  VMWare guestinfo too many keys);
+break;
+case GUESTINFO_NO_MEMORY:
+error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+  VMWare guestinfo out of memory);
+break;
+default:
+error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+  VMWare rpc send rc=%d unknown, rc);
+break;
+}
+}
+
+void qmp_inject_vmport_reboot(Error **errp)
+{
+int rc = foreach_dynamic_VMPortRpc_device(find_send, (void *)OS_Reboot);
+
+convert_local_rc(errp, rc);
+}
+
+void qmp_inject_vmport_halt(Error **errp)
+{
+