[libvirt] [PATCH v4 2/3] qemu: Add support for S3/S4 state configuration

[Martin Kletzander]

On 08/31/2012 07:49 PM, Eric Blake wrote:
> On 08/31/2012 07:59 AM, Martin Kletzander wrote:
>> This patch adds support for running qemu guests with the required
>> parameters to forcefully enable or disable BIOS advertising of S3 and
>> S4 states.  The support for this is added to capabilities and there is
>> also a qemu command parameter parsing implemented.
>> ---
>>  src/qemu/qemu_capabilities.c |  7 +++++
>>  src/qemu/qemu_capabilities.h |  2 ++
>>  src/qemu/qemu_command.c      | 62 ++++++++++++++++++++++++++++++++++++++++++++
>>  src/qemu/qemu_driver.c       | 17 ++++++++++++
>>  4 files changed, 88 insertions(+)
>>
>> +++ b/src/qemu/qemu_command.c
>> @@ -4782,6 +4782,32 @@ qemuBuildCommandLine(virConnectPtr conn,
>>              virCommandAddArg(cmd, "-no-acpi");
>>      }
>>
>> +    if (def->pm.s3) {
>> +        if (!qemuCapsGet(qemuCaps, QEMU_CAPS_DISABLE_S3)) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                           "%s", _("setting ACPI S3 not supported"));
>> +            goto error;
>> +        }
>> +        virCommandAddArgList(cmd,
>> +                             "-global",
>> +                             (def->pm.s3 == VIR_DOMAIN_PM_STATE_ENABLED ?
>> +                              "PIIX4_PM.disable_s3=0" : "PIIX4_PM.disable_s3=1"),
>> +                             NULL);
> 
> Fine as is, but I probably would have written:
> 
> virCommandAddArg(cmd, "-global");
> virCommandAddArgFormat(cmd, "PIIX4_PM.disable_s3=%d",
>       def->pm.s3 == VIR_DOMAIN_PM_STATE_ENABLED);
> 
> for less typing.
> 
>> +++ b/src/qemu/qemu_driver.c
>> @@ -13722,6 +13722,23 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom,
>>          goto cleanup;
>>      }
>>
>> +    if (vm->def->pm.s3 || vm->def->pm.s4) {
>> +        if (!vm->def->pm.s3 == VIR_DOMAIN_PM_STATE_DISABLED &&
> 
> Logic bug.  (!vm->def->pm.s3) means that you have flattened an enum into
> 0 or 1, before comparing it back to an enum value.  I think you meant to
> drop the ! entirely.
> 
> ACK with that fix.
> 

Yes, I've missed it after one fixing and haven't checked if it works
after that. Fixed, checked and pushed now. Thanks for the review.

Martin