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

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

On 06/15/15 11:53, Eric Blake wrote:
> On 06/12/2015 08:05 AM, 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>
>> CC: Don Slutz <don.slutz@gmail.com>
>> ---
> 
>> +typedef int FindVMPortRpcDeviceFunc(VMPortRpcState *s, const void *arg);
>> +
>> +/*
>> + * The input and output argument that find_VMPortRpc_device() uses
>> + * to do it's work.
> 
> s/it's/its/ (the only time "it's" is correct is if "it is" would also be
> correct)
> 

Will fix.

> 
>>  
>> +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)
> 
> No space after **.
> 

Will fix.

> 
>> +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);
>> +        abort(); /* Should be impossible to get here. */
>> +        break;
> 
> Don't know if any static checkers will complain about the break being
> dead code.
> 
> 

I do not know.  However I can just delete it, and so will do so.

>> +
>> +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."));
> 
> Cast here...
> 
>> +        key_value.key_len = strlen(key) - strlen("guestinfo.");
>> +    } else {
>> +        key_value.key_data = (void *)key;
> 
> ...and here...
> 
>> +        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;
> 
> ...and here is only needed to get rid of const. Would it help if struct
> keyValue declared 'const char *key_data' instead of 'void *key_data'?

This is value_data not key_data. So this cast is needed with 'const char
*key_data', the other 2 do go away.


> 
>> +        key_value.value_len = strlen(value);
>> +    }
>> +
>> +    rc = foreach_dynamic_vmport_rpc_device(find_set, (void *)&key_value);
> 
> This cast is spurious (it does not get rid of const, and any non-const
> pointer in C can already be passed to void* without a cast).
> 

Yup fixed.

>> +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 = (void *)(key + strlen("guestinfo."));
>> +        key_value.key_len = strlen(key) - strlen("guestinfo.");
>> +    } else {
>> +        key_value.key_data = (void *)key;
> 
> More casts that might be prevented with correct const in the callback
> struct.
> 

these 2 do get fixed.

>> +        key_value.key_len = strlen(key);
>> +    }
>> +
>> +    rc = foreach_dynamic_vmport_rpc_device(find_get, (void *)&key_value);
> 
> Another spurious cast.
> 

Fixed.

>> +    if (rc) {
>> +        convert_local_rc(errp, rc);
>> +        return NULL;
>> +    }
>> +
>> +    ret_value = g_malloc(sizeof(VmportGuestInfoKey));
> 
> Do you want g_malloc0, or even the additional type-safety (and less
> typing) of g_new0(VmportGuestInfoKey, 1)?  Otherwise, if you later add
> fields, but forget to initialize them, you'll have random garbage.
> 

Switched to g_new0.

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

Added that to the comment.


> 
> Looking better, though.
> 


   -Don Slutz