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

[Don Slutz <don.slutz@gmail.com>]

On 07/03/15 13:56, Markus Armbruster wrote:
> Don Slutz <don.slutz@gmail.com> writes:
>
>> On 07/02/15 10:11, Markus Armbruster wrote:
>>> Don Slutz <don.slutz@gmail.com> writes:
>>>
>>>> From: Don Slutz <dslutz@verizon.com>
>>>>
>>>> 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 <dslutz@verizon.com>
>>>> CC: Don Slutz <don.slutz@gmail.com>
>>> 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 <armbru@redhat.com>
>>> 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 <armbru@redhat.com>
>>>       Reviewed-by: Eric Blake <eblake@redhat.com>
>>>       Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>       Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
>>>
>>> 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
> [...]