[libvirt] [RFC v4] Export KVM Host Power Management capabilities

[Eric Blake]

On 08/09/2011 09:29 AM, Srivatsa S. Bhat wrote:
> +    if(caps->host.powerMgmt_valid) {
> +        /* The PM query was successful. */
> +        if(caps->host.powerMgmt) {
> +            /* The host supports some PM features. */
> +            unsigned int pm = caps->host.powerMgmt;
> +            virBufferAddLit(&xml, "<power_management>\n");
> +            while(pm) {

Our style is to use space after keywords and before opening '(' (that 
is, you should be using 'if (', 'while ('); we only omit space on 
function calls (your use of 'virBufferAddLit(' is correct).

> +++ b/src/qemu/qemu_capabilities.c
> @@ -794,6 +794,8 @@ virCapsPtr qemuCapsInit(virCapsPtr old_caps)
>       struct utsname utsname;
>       virCapsPtr caps;
>       int i;
> +    int status = -1;
> +    unsigned int pmbitmask = 0;

You don't need this temporary.  Instead,...

>       char *xenner = NULL;
>
>       /* Really, this never fails - look at the man-page. */
> @@ -824,6 +826,22 @@ virCapsPtr qemuCapsInit(virCapsPtr old_caps)
>           old_caps->host.cpu = NULL;
>       }
>
> +    /* Add the power management features of the host */
> +
> +    status = virGetPMCapabilities(&pmbitmask);
> +    if(status<  0) {
> +        caps->host.powerMgmt_valid = false;

VIR_ALLOC guaranteed that false was already the default.

> +        VIR_WARN("Failed to get host power management capabilities");
> +    } else {
> +        /* The PM query succeeded. */

...you can just do the assignment in place:

if (virGetPMCapabilities(&caps->host.powerMgmt) < 0)
     VIR_WARN("Failed to get host power management capabilities");
else
     caps->host.powerMgmt_valid = true;

> +int
> +virGetPMCapabilities(unsigned int * bitmask)

Our style does not use space after '*' for pointers:

unsigned int *bitmask

> +    if((path = virFindFileInPath("pm-is-supported")) == NULL) {
> +        virUtilError(VIR_ERR_INTERNAL_ERROR,
> +                     "%s", _("Failed to get the path of pm-is-supported"));
> +        return -1;
> +    }

Overkill - virCommand already does this check on your behalf.

> +
> +    /* Check support for Suspend-to-RAM (S3) */
> +    cmd = virCommandNew(path);
> +    virCommandAddArg(cmd, "--suspend");
> +    if(virCommandRun(cmd,&status)<  0) {
> +        virUtilError(VIR_ERR_INTERNAL_ERROR,
> +                     "%s", _("Failed to run command"
> +                             "'pm-is-supported --suspend'"));

Overkill - virCommand already issues a (much better) error message on 
your behalf.

I'd use:

int ret = -1;
int status;

cmd = virCommandNewArgList("pm-is-supported", "--suspend", NULL);
if (virCommandRun(cmd, &status) < 0)
     goto cleanup;
if (status == 0)
     *bitmask |= 1U << VIR_HOST_PM_S3;
virCommandFree(cmd);
cmd = virCommandNewArgList("pm-is-supported", "--hibernate", NULL);
if (virCommandRun(cmd, &status) < 0)
     goto cleanup;
if (status == 0)
     *bitmask |= 1U << VIR_HOST_PM_S4;
ret = 0;

cleanup:
virCommandFree(cmd);
return ret;

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org