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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4273 bytes --]

On 05/12/2015 01:03 PM, Don Slutz wrote:
> 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>
> ---

> +/*
> + * 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) {

Why not assert(find) instead of leaving it to the comment?

> +/*
> + * Loop through all dynamically created VMPortRpc devices and call
> + * func() for each instance.
> + */
> +static int foreach_dynamic_vmport_rpc_device(FindVMPortRpcDeviceFunc *func,
> +                                             void *arg)
> +{
> +    VMPortRpcFind find = {
> +        .func = func,
> +        .arg = arg,

Is it worth marking arg const here and in the VMPortRpcFind struct...


> +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,
> +                                               (void *)"OS_Reboot");

...so that you don't have to cast away const here?

> +        break;
> +    case VMPORT_ACTION_HALT:
> +        rc = foreach_dynamic_vmport_rpc_device(vmport_rpc_find_send,
> +                                               (void *)"OS_Halt");
> +        break;
> +    case VMPORT_ACTION_MAX:
> +        assert(action != VMPORT_ACTION_MAX);
> +        rc = 0; /* Should be impossible to get here. */

I'd rather abort() if someone compiled with -NDEBUG.

> +        break;
> +    }
> +    convert_local_rc(errp, rc);
> +}
> +
> +typedef struct keyValue {
> +    void *key_data;
> +    void *value_data;
> +    unsigned int key_len;
> +    unsigned int value_len;

Should these be size_t?

> +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 = (void *)(key + strlen("guestinfo."));
> +        key_value.key_len = strlen(key) - strlen("guestinfo.");
> +    } else {
> +        key_value.key_data = (void *)key;

Casting to (void*) looks awkward; should key_data should be typed 'const
void *' to avoid the need for a cast?  For that matter, why is it void*,
why not 'const char *'?


> +++ b/qmp-commands.hx
> @@ -490,6 +490,126 @@ Note: inject-nmi fails when the guest doesn't support injecting.
>  EQMP

> +SQMP
> +vmport-guestinfo-set
> +----------

Still mismatches on ---- line length (several sites).

> +-> { "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
> +of all keys.
> +
> +Example:
> +
> +-> { "execute": "query-vmport-guestinfo" }
> +<- {
> +      "return": [
> +         {
> +            "key": "guestinfo.ip"
> +         },

So if I'm following this correctly, a user has to call
query-vmport-guestinfo to learn the key names, and then once per key
call vmport-guetsinfo-get to learn the key values.  Why not just return
key names and values all at once, to save the multiple call overhead?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]