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

[Markus Armbruster]

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?

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

> +        abort(); /* Should be impossible to get here. */

Why not simply

       default:
           abort();

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

> +

No blank line at end of file, please.

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


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

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

> +#
> +# 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
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -490,6 +490,125 @@ Note: inject-nmi fails when the guest doesn't support 
> injecting.
>  EQMP
>  
>      {
> +        .name       = "inject-vmport-action",
> +        .args_type  = "action:s",
> +        .mhandler.cmd_new = qmp_marshal_input_inject_vmport_action,
> +    },
> +
> +SQMP
> +inject-vmport-action
> +--------------------
> +
> +Inject a VMWare Tools action to the guest.
> +
> +Arguments:
> +
> +- "action": vmport action (json-string)
> +          - Possible values: "reboot", "halt"
> +
> +Example:
> +
> +-> { "execute": "inject-vmport-action",
> +                "arguments": { "action": "reboot" } }
> +<- { "return": {} }
> +
> +Note: inject-vmport-action fails when the guest doesn't support injecting.
> +
> +EQMP
> +
> +    {
> +        .name       = "vmport-guestinfo-set",
> +        .args_type  = "key:s,value:s,format:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_vmport_guestinfo_set,
> +    },
> +
> +SQMP
> +vmport-guestinfo-set
> +--------------------
> +
> +Set a VMWare Tools guestinfo key to a value
> +
> +Arguments:
> +
> +- "key": the key to set (json-string)
> +- "value": data to write (json-string)
> +- "format": data format (json-string, optional)
> +          - Possible values: "utf8" (default), "base64"

Shouldn't you copy ringbuf-write's bug note?

               Bug: invalid base64 is currently not rejected.
               Whitespace *is* invalid.

> +
> +Example:
> +
> +-> { "execute": "vmport-guestinfo-set",
> +                "arguments": { "key": "guestinfo.foo",
> +                               "value": "abcdefgh",
> +                               "format": "utf8" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
> +        .name       = "vmport-guestinfo-get",
> +        .args_type  = "key:s,format:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_vmport_guestinfo_get,
> +    },
> +
> +SQMP
> +vmport-guestinfo-get
> +--------------------
> +
> +Get a VMWare Tools guestinfo value for a key
> +
> +Arguments:
> +
> +- "key": the key to get (json-string)
> +- "format": data format (json-string, optional)
> +          - Possible values: "utf8" (default), "base64"
> +          - Naturally, format "utf8" works only when the value
> +            contains valid UTF-8 text.  Bug: replacement doesn't work.
> +         Bug: can screw up on encountering NUL characters.

You adapted ringbuf-read's bug note.  Good.

> +
> +Example:
> +
> +-> { "execute": "vmport-guestinfo-get",
> +                "arguments": { "key": "guestinfo.foo",
> +                               "format": "utf8" } }
> +<- {"return": {"value": "abcdefgh"}}
> +
> +
> +EQMP
> +
> +    {
> +        .name       = "query-vmport-guestinfo",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_vmport_guestinfo,
> +    },
> +
> +SQMP
> +query-vmport-guestinfo
> +----------------------
> +
> +Returns information about VMWare Tools guestinfo.  The returned value is a 
> json-array

Long line, please wrap.

> +of all keys.
> +
> +Example:
> +
> +-> { "execute": "query-vmport-guestinfo" }
> +<- {
> +      "return": [
> +         {
> +            "key": "guestinfo.ip"
> +         },
> +         {
> +            "key": "guestinfo.foo"
> +         },
> +         {
> +            "key": "guestinfo.long"
> +         }
> +      ]
> +   }
> +
> +EQMP
> +
> +    {
>          .name       = "ringbuf-write",
>          .args_type  = "device:s,data:s,format:s?",
>          .mhandler.cmd_new = qmp_marshal_input_ringbuf_write,

I guess I'd split this into inject-vmport-action and the guestinfo
stuff.  Matter of taste.