[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 6/9] vmport_rpc: Add QMP access to vmport_rpc
From: |
Don Slutz |
Subject: |
Re: [Qemu-devel] [PATCH v7 6/9] vmport_rpc: Add QMP access to vmport_rpc object. |
Date: |
Wed, 17 Jun 2015 12:42:10 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
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 <address@hidden>
>> CC: Don Slutz <address@hidden>
>> ---
>
>> +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
[Qemu-devel] [PATCH v7 2/9] vmport: Switch to trace, Don Slutz, 2015/06/12
[Qemu-devel] [BUGFIX][PATCH v7 3/9] vmport: Fix vmport_cmd_ram_size, Don Slutz, 2015/06/12
[Qemu-devel] [PATCH v7 6/9] vmport_rpc: Add QMP access to vmport_rpc object., Don Slutz, 2015/06/12
[Qemu-devel] [PATCH v7 7/9] vmport_rpc: Add migration, Don Slutz, 2015/06/12
[Qemu-devel] [PATCH v7 5/9] vmport_rpc: Add limited support of VMware's hyper-call rpc, Don Slutz, 2015/06/12
[Qemu-devel] [PATCH v7 8/9] vmport: Add VMware all ring hack, Don Slutz, 2015/06/12
[Qemu-devel] [PATCH v7 4/9] vmport_rpc: Add the object vmport_rpc, Don Slutz, 2015/06/12
[Qemu-devel] [PATCH v7 9/9] MAINTAINERS: add VMware port, Don Slutz, 2015/06/12
Re: [Qemu-devel] [PATCH v7 0/9] Add limited support of VMware's hyper-call rpc, Paolo Bonzini, 2015/06/17