From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33443) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZB8TR-0003dZ-I5 for qemu-devel@nongnu.org; Fri, 03 Jul 2015 17:26:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZB8TN-0007MW-Ue for qemu-devel@nongnu.org; Fri, 03 Jul 2015 17:26:17 -0400 Received: from mail-yk0-x22b.google.com ([2607:f8b0:4002:c07::22b]:35398) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZB8TN-0007MO-Mg for qemu-devel@nongnu.org; Fri, 03 Jul 2015 17:26:13 -0400 Received: by ykdy1 with SMTP id y1so104714813ykd.2 for ; Fri, 03 Jul 2015 14:26:13 -0700 (PDT) From: Don Slutz Message-ID: <5596FDF2.1060702@Gmail.com> Date: Fri, 03 Jul 2015 17:26:10 -0400 MIME-Version: 1.0 References: <1435102773-3498-1-git-send-email-Don.Slutz@Gmail.com> <1435102773-3498-7-git-send-email-Don.Slutz@Gmail.com> <87a8vevec7.fsf@blackfin.pond.sub.org> <5596BDFB.4050603@Gmail.com> <87k2uhyvkb.fsf@blackfin.pond.sub.org> In-Reply-To: <87k2uhyvkb.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 6/8] vmport_rpc: Add QMP access to vmport_rpc object. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: "Michael S. Tsirkin" , qemu-devel@nongnu.org, Don Slutz , Luiz Capitulino , Anthony Liguori , Paolo Bonzini , =?windows-1252?Q?Andreas_F=E4rber?= , Richard Henderson On 07/03/15 13:56, Markus Armbruster wrote: > Don Slutz writes: > >> On 07/02/15 10:11, Markus Armbruster wrote: >>> Don Slutz writes: >>> >>>> From: Don Slutz >>>> >>>> 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 >>>> CC: Don Slutz >>> 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 >>> 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 >>> Reviewed-by: Eric Blake >>> Reviewed-by: Stefan Hajnoczi >>> Reviewed-by: Luiz Capitulino >>> >>> 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. >>> >>>> #endif >>> Since the #ifdef section is now longer than a few lines, >>> >>> -#endif >>> +#endif /* !TARGET_I386 */ >>> >>> would be nice >> Will 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 get >>> Ignorant 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 dynamic > Thanks. > >>>> +# >>>> +# @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 key >>> ringbuf-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 > [...]