[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 1/4] Add per-guest S3/S4 state configuration



On 09.08.2012 10:26, Martin Kletzander wrote:
> There is a new <pm/> element implemented that can control what ACPI
> sleeping states will be advertised by BIOS and allowed to be switched
> to by libvirt. The default keeps defaults on hypervisor, otherwise
> forces chosen setting.
> ---
>  docs/schemas/domaincommon.rng |   33 ++++++++++++++++++++++++++++++
>  src/conf/domain_conf.c        |   44 +++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h        |   15 ++++++++++++++
>  src/libvirt_private.syms      |    2 +
>  4 files changed, 94 insertions(+), 0 deletions(-)

As I've mentioned in one of previous series - I'd like to see
documentation and XML extension in one patch as it's advised here:

   http://libvirt.org/api_extension.html

But then again, having a separate patch within set is okay too.

> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index c85d763..d0c6d47 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -53,6 +53,9 @@
>          <ref name="features"/>
>          <ref name="termination"/>
>          <optional>
> +          <ref name="pm"/>
> +        </optional>
> +        <optional>
>            <ref name="devices"/>
>          </optional>
>          <optional>
> @@ -2192,6 +2195,36 @@
>      </choice>
>    </define>
>    <!--
> +      Control ACPI sleep states (dis)allowed for the domain
> +      For each of the states the following rules apply:
> +      on: the state will be forcefully enabled
> +      off: the state will be forcefully disabled
> +      not specified: hypervisor will be left to decide its defaults
> +  -->
> +  <define name="pm">
> +    <element name="pm">
> +      <interleave>
> +        <optional>
> +          <attribute name="suspend-to-mem">
> +            <ref name="suspendChoices"/>
> +          </attribute>
> +        </optional>
> +        <optional>
> +          <attribute name="suspend-to-disk">
> +            <ref name="suspendChoices"/>
> +          </attribute>
> +        </optional>
> +      </interleave>
> +      <empty/>
> +    </element>
> +  </define>
> +  <define name="suspendChoices">
> +    <choice>
> +      <value>on</value>
> +      <value>off</value>
> +    </choice>
> +  </define>
> +  <!--

Let's hope we won't need any other configurable knobs for s3/s4;
otherwise we should move from attributes to elements:
  <pm>
     <s3 enabled='yes'/>
     <s4 enabled='no'/>
  </pm>

>        Specific setup for a qemu emulated character device.  Note: this
>        definition doesn't fully specify the constraints on this node.
>      -->
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d8c0969..04e0be5 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -124,6 +124,11 @@ VIR_ENUM_IMPL(virDomainLifecycleCrash, VIR_DOMAIN_LIFECYCLE_CRASH_LAST,
>                "coredump-destroy",
>                "coredump-restart")
> 
> +VIR_ENUM_IMPL(virDomainPMState, VIR_DOMAIN_PM_STATE_LAST,
> +              "default",
> +              "on",
> +              "off")
> +
>  VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST,
>                "none",
>                "disk",
> @@ -8413,6 +8418,19 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>                                     virDomainLifecycleCrashTypeFromString) < 0)
>          goto error;
> 
> +    if (virXPathNode("./pm", ctxt)) {
> +        tmp = virXPathString("string(./pm/@suspend-to-mem)", ctxt);
> +        if (tmp) {
> +            def->pm.s3 = virDomainPMStateTypeFromString(tmp);
> +            VIR_FREE(tmp);
> +        }
> +        tmp = virXPathString("string(./pm/@suspend-to-disk)", ctxt);
> +        if (tmp) {
> +            def->pm.s4 = virDomainPMStateTypeFromString(tmp);
> +            VIR_FREE(tmp);
> +        }
> +    }
> +

vir*TypeFromString() may fail and return < 0 value. We should not
silently ignore not allowed values here. NB please don't use
VIR_ERR_INTERNAL_ERROR as it is not an internal error but
VIR_ERR_CONFIG_UNSUPPORTED.

>      tmp = virXPathString("string(./clock/@offset)", ctxt);
>      if (tmp) {
>          if ((def->clock.offset = virDomainClockOffsetTypeFromString(tmp)) < 0) {
> @@ -13092,6 +13110,32 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>                                      virDomainLifecycleCrashTypeToString) < 0)
>          goto cleanup;
> 
> +    if (def->pm.s3 || def->pm.s4) {
> +        virBufferAddLit(buf, "  <pm");
> +
> +        if (def->pm.s3) {
> +            const char *tmp = virDomainPMStateTypeToString(def->pm.s3);
> +            if (!tmp) {

This is useless. Libvirt must validate its inputs (esp. those from
user). Outputs are believed to be trusted - that is - once we validate
input and set def->pm.s3 to one of allowed values, nobody will hop into
our address space and change it. It he will anyway, it's problem.

> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("unexpected PM state %d"), def->pm.s3);
> +                goto cleanup;
> +            }
> +            virBufferAsprintf(buf, " suspend-to-mem='%s'", tmp);
> +        }
> +
> +        if (def->pm.s4) {
> +            const char *tmp = virDomainPMStateTypeToString(def->pm.s4);
> +            if (!tmp) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("unexpected PM state %d"), def->pm.s4);
> +                goto cleanup;
> +            }
> +            virBufferAsprintf(buf, " suspend-to-disk='%s'", tmp);
> +        }
> +
> +        virBufferAddLit(buf, "/>\n");
> +    }
> +
>      virBufferAddLit(buf, "  <devices>\n");
> 
>      virBufferEscapeString(buf, "    <emulator>%s</emulator>\n",
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 3f25ad2..ecca72f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1373,6 +1373,14 @@ enum virDomainLifecycleCrashAction {
>      VIR_DOMAIN_LIFECYCLE_CRASH_LAST
>  };
> 
> +enum virDomainPMState {
> +    VIR_DOMAIN_PM_STATE_DEFAULT = 0,
> +    VIR_DOMAIN_PM_STATE_ON,
> +    VIR_DOMAIN_PM_STATE_OFF,
> +
> +    VIR_DOMAIN_PM_STATE_LAST,
> +};
> +
>  enum virDomainBIOSUseserial {
>      VIR_DOMAIN_BIOS_USESERIAL_DEFAULT = 0,
>      VIR_DOMAIN_BIOS_USESERIAL_YES,
> @@ -1619,6 +1627,12 @@ struct _virDomainDef {
>      int onPoweroff;
>      int onCrash;
> 
> +    struct {
> +        /* These options are actually type of enum virDomainPMState */
> +        int s3;
> +        int s4;
> +    } pm;
> +
>      virDomainOSDef os;
>      char *emulator;
>      int features;
> @@ -2166,6 +2180,7 @@ VIR_ENUM_DECL(virDomainBoot)
>  VIR_ENUM_DECL(virDomainFeature)
>  VIR_ENUM_DECL(virDomainLifecycle)
>  VIR_ENUM_DECL(virDomainLifecycleCrash)
> +VIR_ENUM_DECL(virDomainPMState)
>  VIR_ENUM_DECL(virDomainDevice)
>  VIR_ENUM_DECL(virDomainDeviceAddress)
>  VIR_ENUM_DECL(virDomainDeviceAddressPciMulti)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 79b4a18..d8f409a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -437,6 +437,8 @@ virDomainPausedReasonTypeFromString;
>  virDomainPausedReasonTypeToString;
>  virDomainPciRombarModeTypeFromString;
>  virDomainPciRombarModeTypeToString;
> +virDomainPMStateTypeFromString;
> +virDomainPMStateTypeToString;
>  virDomainRedirdevBusTypeFromString;
>  virDomainRedirdevBusTypeToString;
>  virDomainRemoveInactive;
> 

Otherwise looking good.

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]