|
| From: | Don Slutz |
| Subject: | Re: [Qemu-devel] [PATCH v8 6/8] vmport_rpc: Add QMP access to vmport_rpc object. |
| Date: | Fri, 03 Jul 2015 17:26:10 -0400 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
On 07/03/15 13:56, Markus Armbruster wrote:
Don Slutz <address@hidden> writes:On 07/02/15 10:11, Markus Armbruster wrote:Don Slutz <address@hidden> writes:From: Don Slutz <address@hidden> This adds one new inject command: inject-vmport-action 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> CC: Don Slutz <address@hidden>Reviewing only the QAPI/QMP interface, plus a bit of drive-by shooting.diff --git a/hw/misc/vmport_rpc.c b/hw/misc/vmport_rpc.c index f157425..917898b 100644 --- a/hw/misc/vmport_rpc.c +++ b/hw/misc/vmport_rpc.c @@ -178,6 +178,19 @@ typedef struct VMPortRpcState { #endif } VMPortRpcState; +typedef int FindVMPortRpcDeviceFunc(VMPortRpcState *s, const void *arg); + +/* + * The input and output argument that find_VMPortRpc_device() uses + * to do its work. + */ +typedef struct VMPortRpcFind { + const void *arg; + FindVMPortRpcDeviceFunc *func; + int found; + int rc; +} VMPortRpcFind; + /* Basic request types */ typedef enum { MESSAGE_TYPE_OPEN, @@ -202,6 +215,56 @@ typedef struct { uint32_t edi; } vregs; +/* + * Run func() for every VMPortRpc device, traverse the tree for + * everything else. + */ +static int find_VMPortRpc_device(Object *obj, void *opaque) +{ + VMPortRpcFind *find = opaque; + Object *dev; + VMPortRpcState *s; + + assert(find); + 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_vmport_rpc_device(FindVMPortRpcDeviceFunc *func, + const void *arg) +{ + VMPortRpcFind find = { + .func = func, + .arg = arg, + }; + + /* Loop through all VMPortRpc devices that were spawned 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 tracing. This routine will convert @@ -329,7 +392,7 @@ static int vmport_rpc_send(VMPortRpcState *s, channel_t *c, return rc; } -static int vmport_rpc_ctrl_send(VMPortRpcState *s, char *msg) +static int vmport_rpc_ctrl_send(VMPortRpcState *s, const char *msg) { int rc = SEND_NOT_OPEN; unsigned int i; @@ -457,6 +520,29 @@ static int get_guestinfo(VMPortRpcState *s, return GUESTINFO_NOTFOUND; } +static int get_qmp_guestinfo(VMPortRpcState *s, + unsigned int a_key_len, const char *a_info_key, + unsigned int *a_value_len, void **a_value_data) +{ + guestinfo_t *gi = NULL; + + if (a_key_len <= MAX_KEY_LEN) { + gpointer key = g_strndup(a_info_key, a_key_len); + + gi = (guestinfo_t *)g_hash_table_lookup(s->guestinfo, key); + g_free(key); + } else { + return GUESTINFO_KEYTOOLONG; + } + if (gi) { + *a_value_len = gi->val_len; + *a_value_data = gi->val_data; + return 0; + } + + return GUESTINFO_NOTFOUND; +} + static int set_guestinfo(VMPortRpcState *s, int a_key_len, unsigned int a_val_len, const char *a_info_key, char *val) @@ -775,7 +861,7 @@ static void vmport_rpc(VMPortRpcState *s , vregs *ur) } if (delta > s->reset_time) { trace_vmport_rpc_ping(delta); - vmport_rpc_ctrl_send(s, (char *)"reset"); + vmport_rpc_ctrl_send(s, "reset"); } vmport_rpc_sweep(s, now_time); do { @@ -848,6 +934,206 @@ static uint32_t vmport_rpc_ioport_read(void *opaque, uint32_t addr) return ur.data[0]; } +static int vmport_rpc_find_send(VMPortRpcState *s, const void *arg) +{ + return vmport_rpc_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;Conflicts with commit 75158ebbe259f0bd8bf435e8f4827a43ec89c877 Author: Markus Armbruster <address@hidden> Date: Mon Mar 16 08:57:47 2015 +0100 qerror: Eliminate QERR_DEVICE_NOT_FOUND Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used in new code. Hiding them in QERR_ macros makes new uses hard to spot. Fortunately, there's just one such macro left. Eliminate it with this coccinelle semantic patch: @@ expression EP, E; @@ -error_set(EP, QERR_DEVICE_NOT_FOUND, E) +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E) Signed-off-by: Markus Armbruster <address@hidden> Reviewed-by: Eric Blake <address@hidden> Reviewed-by: Stefan Hajnoczi <address@hidden> Reviewed-by: Luiz Capitulino <address@hidden> Sorry if you explained this already for previous revisions: what's the justification for ERROR_CLASS_DEVICE_NOT_FOUND? Can you stick to ERROR_CLASS_GENERIC_ERROR?I was only coping code from other places. Happy to convert to error_setg().Please do.
Ok, will do.
+ case SEND_NOT_OPEN: + error_setg(errp, "VMWare rpc not open"); + break; + case SEND_SKIPPED: + error_setg(errp, "VMWare rpc send skipped"); + break; + case SEND_TRUCATED: + error_setg(errp, "VMWare rpc send trucated"); + break; + case SEND_NO_MEMORY: + error_setg(errp, "VMWare rpc send out of memory"); + break; + case GUESTINFO_NOTFOUND: + error_setg(errp, "VMWare guestinfo not found"); + break; + case GUESTINFO_VALTOOLONG: + error_setg(errp, "VMWare guestinfo value too long"); + break; + case GUESTINFO_KEYTOOLONG: + error_setg(errp, "VMWare guestinfo key too long"); + break; + case GUESTINFO_TOOMANYKEYS: + error_setg(errp, "VMWare guestinfo too many keys"); + break; + case GUESTINFO_NO_MEMORY: + error_setg(errp, "VMWare guestinfo out of memory"); + break; + default: + error_setg(errp, "VMWare rpc send rc=%d unknown", rc); + break; + } +} + +void qmp_inject_vmport_action(enum VmportAction action, Error **errp) +{ + int rc; + + switch (action) { + case VMPORT_ACTION_REBOOT: + rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send, + "OS_Reboot"); + break; + case VMPORT_ACTION_HALT: + rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send, + "OS_Halt"); + break; + case VMPORT_ACTION_MAX: + assert(action != VMPORT_ACTION_MAX);If this fails, you likely found a major compiler bug.Nope. That assert() does happen:Uh, I misread != for ==. The assert *always* fails. Sorry!+ abort(); /* Should be impossible to get here. */Why not simply default: abort();Will change to this. abort() was added in v7.Thanks!+ } + convert_local_rc(errp, rc); +} + +typedef struct keyValue { + const char *key_data; + void *value_data; + unsigned int key_len; + unsigned int value_len; +} keyValue; + +static int find_set(VMPortRpcState *s, const void *arg) +{ + const keyValue *key_value = arg; + + return set_guestinfo(s, key_value->key_len, key_value->value_len, + key_value->key_data, key_value->value_data); +} + +static int find_get(VMPortRpcState *s, const void *arg) +{ + keyValue *key_value = (void *)arg; + + return get_qmp_guestinfo(s, key_value->key_len, key_value->key_data, + &key_value->value_len, &key_value->value_data); +} + +void qmp_vmport_guestinfo_set(const char *key, const char *value, + bool has_format, enum DataFormat format, + Error **errp) +{ + int rc; + keyValue key_value; + + if (strncmp(key, "guestinfo.", strlen("guestinfo.")) == 0) { + key_value.key_data = (key + strlen("guestinfo.")); + key_value.key_len = strlen(key) - strlen("guestinfo."); + } else { + key_value.key_data = key; + key_value.key_len = strlen(key); + } + if (has_format && (format == DATA_FORMAT_BASE64)) { + gsize write_count; + + key_value.value_data = g_base64_decode(value, &write_count); + key_value.value_len = write_count; + } else { + key_value.value_data = (void *)value; + key_value.value_len = strlen(value); + } + + rc = foreach_dynamic_vmport_rpc_device(find_set, &key_value); + + if (key_value.value_data != value) { + g_free(key_value.value_data); + } + + if (rc) { + convert_local_rc(errp, rc); + return; + } +} + +VmportGuestInfoValue *qmp_vmport_guestinfo_get(const char *key, + bool has_format, + enum DataFormat format, + Error **errp) +{ + int rc; + keyValue key_value; + VmportGuestInfoValue *ret_value; + + if (strncmp(key, "guestinfo.", strlen("guestinfo.")) == 0) { + key_value.key_data = (key + strlen("guestinfo.")); + key_value.key_len = strlen(key) - strlen("guestinfo."); + } else { + key_value.key_data = key; + key_value.key_len = strlen(key); + } + + rc = foreach_dynamic_vmport_rpc_device(find_get, &key_value); + if (rc) { + convert_local_rc(errp, rc); + return NULL; + } + + ret_value = g_new0(VmportGuestInfoValue, 1); + if (has_format && (format == DATA_FORMAT_BASE64)) { + ret_value->value = g_base64_encode(key_value.value_data, + key_value.value_len); + } else { + /* + * FIXME should check for complete, valid UTF-8 characters. + * Invalid sequences should be replaced by a suitable + * replacement character. Or cause an error to be raised. + */Thanks for copying the FIXME from qmp_ringbuf_read(), I appreciate it.+ ret_value->value = g_malloc(key_value.value_len + 1); + memcpy(ret_value->value, key_value.value_data, key_value.value_len); + ret_value->value[key_value.value_len] = 0; + } + + return ret_value; +} + + +static void vmport_rpc_find_list_one(gpointer key, gpointer value, + gpointer opaque) +{ + VmportGuestInfoKeyList **keys = opaque; + VmportGuestInfoKeyList *info = g_new0(VmportGuestInfoKeyList, 1); + char *ckey = key; + + info->value = g_new0(VmportGuestInfoKey, 1); + info->value->key = g_strdup_printf("guestinfo.%s", ckey); + info->next = *keys; + *keys = info; +} + +static int vmport_rpc_find_list(VMPortRpcState *s, const void *arg) +{ + g_hash_table_foreach(s->guestinfo, vmport_rpc_find_list_one, (gpointer)arg); + return 0; +} + +VmportGuestInfoKeyList *qmp_query_vmport_guestinfo(Error **errp) +{ + VmportGuestInfoKeyList *keys = NULL; + int rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_list, + (void *)&keys); + + if (rc) { + convert_local_rc(errp, rc); + } + + return keys; +} + + static void vmport_rpc_reset(DeviceState *d) { unsigned int i; diff --git a/monitor.c b/monitor.c index aeea2b5..4224314 100644 --- a/monitor.c +++ b/monitor.c @@ -5360,4 +5360,28 @@ void qmp_rtc_reset_reinjection(Error **errp) { error_setg(errp, QERR_FEATURE_DISABLED, "rtc-reset-reinjection"); } +void qmp_inject_vmport_action(enum VmportAction action, Error **errp) +{ + error_set(errp, QERR_FEATURE_DISABLED, "inject-vmport-action"); +} +void qmp_vmport_guestinfo_set(const char *key, const char *value, + bool has_format, enum DataFormat format, + Error **errp) +{ + error_set(errp, QERR_FEATURE_DISABLED, "vmport-guestinfo-set"); +} +VmportGuestInfoValue *qmp_vmport_guestinfo_get(const char *key, + bool has_format, + enum DataFormat format, + Error **errp) +{ + error_set(errp, QERR_FEATURE_DISABLED, "vmport-guestinfo-get"); + return NULL; +} +VmportGuestInfoKeyList *qmp_query_vmport_guestinfo(Error **errp) +{ + error_set(errp, QERR_FEATURE_DISABLED, "query-vmport-guestinfo"); + return NULL; +}I understand you're just following existing practice here, but I can help to wonder whether the practice is smart. The command exists in all compile-time configurations, but it works in only some. To figure out whether it works, you have to try it. If we add the command only when it actually works, you can use query-commands instead. This isn't a demand for you to change anything now. We QMP guys need to decide how we want this done.#endifSince the #ifdef section is now longer than a few lines, -#endif +#endif /* !TARGET_I386 */ would be niceWill add.+No blank line at end of file, please.Will drop.diff --git a/qapi-schema.json b/qapi-schema.json index 106008c..5d9e9e2 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1429,6 +1429,107 @@ { 'command': 'inject-nmi' } ## +# @VmportAction: +# +# An enumeration of actions that can be requested via vmport RPC. +# +# @reboot: Ask the guest via VMware tools to reboot +# +# @halt: Ask the guest via VMware tools to halt +# +# Since: 2.4 +## +{ 'enum': 'VmportAction', + 'data': [ 'reboot', 'halt' ] } + +## +# @inject-vmport-action: +# +# Injects a VMWare Tools action to the guest. +# +# Returns: If successful, nothing +# +# Since: 2.4 +# +## +{ 'command': 'inject-vmport-action', + 'data': {'action': 'VmportAction'} } + +## +# @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 is assumed to be base64 encoded text. Its binary +# decoding gets set.Shouldn't you copy ringbuf-write's bug note? # Bug: invalid base64 is currently not rejected.Nope. From the cover letter: ---------------------------------------------------------------------------------------- Much more on format=base64: 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). ---------------------------------------------------------------------------------------- Based on all this, I did drop it.The QMP user isn't at all interested whether QEMU or GLib or any other library screws this up, only in what the command's specification is, and whatever bugs its implementation may have. We obviously have leeway with the specification. We *can* override RFC 3548 by "explicitly stating otherwise". For ringbuf-write, we deliberately chose not to. Instead we declared the non-rejection of invalid characters a bug. The exact same reasoning applies to your command. If you disagree with the reasoning, it's certainly open to debate.
I feel that in real use (not testing) only valid base64 would be passed in. However I do not want to delay this patch on this issue.
Regardless of what consensus emerges, all uses of base64 DataFormat in QMP should be consistent. Right now they aren't with your patch applied.
Ok, I will copy the "bug comment line" so that it is consistent.
+# - utf8: value's UTF-8 encoding is used to set. +# +# Returns: Nothing on success +# +# Since: 2.4 +## +{ 'command': 'vmport-guestinfo-set', + 'data': {'key': 'str', 'value': 'str', + '*format': 'DataFormat'} } + +## +# @VmportGuestInfoValue: +# +# Information about a single VMWare Tools guestinfo +# +# @value: The value for a key +# +# Since: 2.4 +## +{ 'struct': 'VmportGuestInfoValue', 'data': {'value': 'str'} } + +## +# @vmport-guestinfo-get: +# +# Get a VMWare Tools guestinfo value for a key +# +# @key: the key to getIgnorant question: is the set of keys fixed (e.g. specified by some authority), completely dynamic (i.e. whatever has been set), or something else?completely dynamicThanks.+# +# @format: #optional data encoding (default 'utf8'). +# - base64: the value is returned in base64 encoding. +# - utf8: the value is interpreted as UTF-8. +# +# Returns: value for the guest info keyringbuf-read carries this bug note: # Bug: can screw up when the buffer contains invalid UTF-8 # sequences, NUL characters, after the ring buffer lost # data, and when reading stops because the size limit is # reached. Doesn't it apply here, too, with "the buffer" replaced?Not directly. The best description I have so far to match the FIXME would be: ----------------------------------------------------------------------------------- Bug: format=utf8 can cause QEMU to screw up (may crash) when the value contains invalid UTF-8 sequences. NUL characters will cause loss of data. ---------------------------------------------------------------------------------- I have not spent the time investigating the issues with QMP and invalid utf8 because it looks to be very wide spread and not just restricted to this one routine.The problem with ringbuf-read with format=utf8 is that it attempts to interpret the ring buffer's contents, which could be anything, as UTF-8 by design (not a problem), and the code doing so is insufficiently robust (this is the bug; patch welcome, as usual). As far as I can tell, vmport-guestinfo-get is a similar case: the value associated with the key could again be anything (if written with format=base64), and we attempt to interpret it as UTF-8 by design. What makes you think the problem is very widespread?
I only did a quick look and did not find what I would have expected. I will do more looking and reply with those results.
I am happy to apply either the above Bug comment or an adjusted version of the ringbuf-read one for now (I have been working on getting this accepted for a while now, 1st posted on Fri, 30 Jan 2015 16:06:24 -0500).
-Don Slutz
+# +# Since: 2.4 +## +{ 'command': 'vmport-guestinfo-get', + 'data': {'key': 'str', '*format': 'DataFormat'}, + 'returns': 'VmportGuestInfoValue' }I gather you wrap vmport-guestinfo-get's return value in a struct for extensibility. Can't see why and how we'd want to extend it, but better safe than sorry.+ +## +# @VmportGuestInfoKey: +# +# Information about a single VMWare Tools guestinfo +# +# @key: The known key +# +# Since: 2.4 +## +{ 'struct': 'VmportGuestInfoKey', 'data': {'key': 'str'} } + +## +# @query-vmport-guestinfo: +# +# Returns information about VMWare Tools guestinfo +# +# Returns: a list of @VmportGuestInfoKey +# +# Since: 2.4 +## +{ 'command': 'query-vmport-guestinfo', 'returns': ['VmportGuestInfoKey'] }Similar. Imagining an extension of a string key is even harder. Okay.+ +## # @set_link: # # Sets the link status of a virtual network adapter. diff --git a/qmp-commands.hx b/qmp-commands.hx index a05d25f..a43db8b 100644[...]
| [Prev in Thread] | Current Thread | [Next in Thread] |