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

[Eric Blake]

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)


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


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


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

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

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

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

Another spurious cast.

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

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

Looking better, though.

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

signature.asc
Description: OpenPGP digital signature