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

[Don Slutz]

On 05/12/15 15:50, Eric Blake wrote:
> 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
>>vmport-guestinfo-get key=foo
>> More details in qmp-commands.hx
>>
>> Signed-off-by: Don Slutz <address@hidden>
>> ---
> 
>> +/*
>> + * 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?

My memory says that someone complained about too many asserts used,
so I just commented it.

I have no issue with adding an assert() and dropping the comment,
and so plan on doing this.

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

See below

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

Not sure.  I still need 2 casts:

-static int vmport_rpc_find_list(VMPortRpcState *s, void *arg)
+static int vmport_rpc_find_list(VMPortRpcState *s, const void *arg)
 {
-    g_hash_table_foreach(s->guestinfo, vmport_rpc_find_list_one, arg);
+    g_hash_table_foreach(s->guestinfo, vmport_rpc_find_list_one,
(gpointer)arg);
     return 0;
 }


and


-static int find_get(VMPortRpcState *s, void *arg)
+static int find_get(VMPortRpcState *s, const void *arg)
 {
-    keyValue *key_value = arg;
+    keyValue *key_value = (void *)arg;

get_qmp_guestinfo() does change parts of key_value.

So are these casts better or worse?


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

Ok, will change to abort.

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

There is no need to.  There is a limit on the max values that can be
used here (because without the max check it is possible for the guest
to request a lot of memory outside of the memory provided to the guest
as ram.

Currently using unsigned char for key_len and unsigned short for
value_len will work, but I went with unsigned int to avoid strange
issues if someone in the future changes the allowed maxes.

However this is not important to me and so if you want them
to be size_t, I am happy to change them.


> 
>> +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 *'?

Not sure.  Best guess is that I was focused on the value being binary
data, and just did the same for the key.  The change to
"const char *key_data;" works, and so I will do this.


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

Sigh, will fix.

> 
>> +-> { "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?
> 

You are following this correctly.  The issue is with "*format" (i.e.
DataFormat).  So the format arg would need to be added, and while I
expect most uses will all be base64, I do not see this as hard requirement.

So if the user of QMP what to use base64 for some keys and utf8 for
others, there is no way to express this in 1 arg.  This is the way
I came up with to handle this case.

   -Don Slutz

signature.asc
Description: OpenPGP digital signature