[libvirt] [PATCH v3 2/3] qemu: Implement the HTM pSeries feature

Andrea Bolognani posted 3 patches 7 years, 7 months ago
[libvirt] [PATCH v3 2/3] qemu: Implement the HTM pSeries feature
Posted by Andrea Bolognani 7 years, 7 months ago
https://bugzilla.redhat.com/show_bug.cgi?id=1525599
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 docs/formatdomain.html.in                     |  8 ++++++++
 docs/schemas/domaincommon.rng                 |  5 +++++
 src/conf/domain_conf.c                        | 19 ++++++++++++++++++
 src/conf/domain_conf.h                        |  1 +
 src/qemu/qemu_command.c                       | 20 +++++++++++++++++++
 src/qemu/qemu_domain.c                        | 13 ++++++++++++
 tests/qemuxml2argvdata/pseries-features.args  |  2 +-
 tests/qemuxml2argvdata/pseries-features.xml   |  1 +
 tests/qemuxml2argvtest.c                      |  1 +
 tests/qemuxml2xmloutdata/pseries-features.xml |  1 +
 tests/qemuxml2xmltest.c                       |  1 +
 11 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 0d68596991..73db315c4f 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1929,6 +1929,7 @@
   &lt;smm state='on'&gt;
     &lt;tseg unit='MiB'&gt;48&lt;/tseg&gt;
   &lt;/smm&gt;
+  &lt;htm state='on'/&gt;
 &lt;/features&gt;
 ...</pre>
 
@@ -2162,6 +2163,13 @@
       <dd>Enable QEMU vmcoreinfo device to let the guest kernel save debug
           details. <span class="since">Since 4.4.0</span> (QEMU only)
       </dd>
+      <dt><code>htm</code></dt>
+      <dd>Configure HTM (Hardware Transational Memory) availability for
+          pSeries guests. Possible values for the <code>state</code> attribute
+          are <code>on</code> and <code>off</code>. If the attribute is not
+          defined, the hypervisor default will be used.
+          <span class="since">Since 4.5.0</span> (QEMU/KVM only)
+      </dd>
     </dl>
 
     <h3><a id="elementsTime">Time keeping</a></h3>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 450e8ac77b..3dd1bd7974 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4906,6 +4906,11 @@
           <optional>
             <ref name="vmcoreinfo"/>
           </optional>
+          <optional>
+            <element name="htm">
+              <ref name="featurestate"/>
+            </element>
+          </optional>
         </interleave>
       </element>
     </optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d8cb7f37f3..ea01d08a3c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -152,6 +152,7 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST,
               "ioapic",
               "hpt",
               "vmcoreinfo",
+              "htm",
 );
 
 VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST,
@@ -19827,6 +19828,22 @@ virDomainDefParseXML(xmlDocPtr xml,
             }
             break;
 
+        case VIR_DOMAIN_FEATURE_HTM:
+            if (!(tmp = virXMLPropString(nodes[i], "state"))) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("missing state attribute '%s' of feature '%s'"),
+                               tmp, virDomainFeatureTypeToString(val));
+                goto error;
+            }
+            if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) < 0) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("unknown state attribute '%s' of feature '%s'"),
+                               tmp, virDomainFeatureTypeToString(val));
+                goto error;
+            }
+            VIR_FREE(tmp);
+            break;
+
         /* coverity[dead_error_begin] */
         case VIR_DOMAIN_FEATURE_LAST:
             break;
@@ -21961,6 +21978,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
         case VIR_DOMAIN_FEATURE_VMPORT:
         case VIR_DOMAIN_FEATURE_SMM:
         case VIR_DOMAIN_FEATURE_VMCOREINFO:
+        case VIR_DOMAIN_FEATURE_HTM:
             if (src->features[i] != dst->features[i]) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                                _("State of feature '%s' differs: "
@@ -27626,6 +27644,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
             case VIR_DOMAIN_FEATURE_PMU:
             case VIR_DOMAIN_FEATURE_PVSPINLOCK:
             case VIR_DOMAIN_FEATURE_VMPORT:
+            case VIR_DOMAIN_FEATURE_HTM:
                 switch ((virTristateSwitch) def->features[i]) {
                 case VIR_TRISTATE_SWITCH_LAST:
                 case VIR_TRISTATE_SWITCH_ABSENT:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0924fc4f3c..a41431ccf9 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1771,6 +1771,7 @@ typedef enum {
     VIR_DOMAIN_FEATURE_IOAPIC,
     VIR_DOMAIN_FEATURE_HPT,
     VIR_DOMAIN_FEATURE_VMCOREINFO,
+    VIR_DOMAIN_FEATURE_HTM,
 
     VIR_DOMAIN_FEATURE_LAST
 } virDomainFeature;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a357d2199c..5342c08f19 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7291,6 +7291,26 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
         }
     }
 
+    if (def->features[VIR_DOMAIN_FEATURE_HTM] != VIR_TRISTATE_SWITCH_ABSENT) {
+        const char *str;
+
+        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HTM)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("HTM configuration is not supported by this "
+                             "QEMU binary"));
+            goto cleanup;
+        }
+
+        str = virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM]);
+        if (!str) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("Invalid setting for HTM state"));
+            goto cleanup;
+        }
+
+        virBufferAsprintf(&buf, ",cap-htm=%s", str);
+    }
+
     if (cpu && cpu->model &&
         cpu->mode == VIR_CPU_MODE_HOST_MODEL &&
         qemuDomainIsPSeries(def) &&
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6d203e1f2e..baf11b4532 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3846,6 +3846,19 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
             }
             break;
 
+        case VIR_DOMAIN_FEATURE_HTM:
+            if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
+                !qemuDomainIsPSeries(def)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                               _("The '%s' feature is not supported for "
+                                 "architecture '%s' or machine type '%s'"),
+                               featureName,
+                               virArchToString(def->os.arch),
+                               def->os.machine);
+                return -1;
+            }
+            break;
+
         case VIR_DOMAIN_FEATURE_ACPI:
         case VIR_DOMAIN_FEATURE_APIC:
         case VIR_DOMAIN_FEATURE_PAE:
diff --git a/tests/qemuxml2argvdata/pseries-features.args b/tests/qemuxml2argvdata/pseries-features.args
index 12c14715c6..226d43df44 100644
--- a/tests/qemuxml2argvdata/pseries-features.args
+++ b/tests/qemuxml2argvdata/pseries-features.args
@@ -8,7 +8,7 @@ QEMU_AUDIO_DRV=none \
 -name guest \
 -S \
 -machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required,\
-cap-hpt-max-page-size=1048576k \
+cap-hpt-max-page-size=1048576k,cap-htm=on \
 -m 512 \
 -smp 1,sockets=1,cores=1,threads=1 \
 -uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \
diff --git a/tests/qemuxml2argvdata/pseries-features.xml b/tests/qemuxml2argvdata/pseries-features.xml
index 30cee5b81c..5c842fe87b 100644
--- a/tests/qemuxml2argvdata/pseries-features.xml
+++ b/tests/qemuxml2argvdata/pseries-features.xml
@@ -10,6 +10,7 @@
     <hpt resizing='required'>
       <maxpagesize unit='GiB'>1</maxpagesize>
     </hpt>
+    <htm state='on'/>
   </features>
   <devices>
     <emulator>/usr/bin/qemu-system-ppc64</emulator>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index ac9593acbe..e64813e430 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1850,6 +1850,7 @@ mymain(void)
     DO_TEST("pseries-features",
             QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE,
             QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE,
+            QEMU_CAPS_MACHINE_PSERIES_CAP_HTM,
             QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT);
     DO_TEST_FAILURE("pseries-features",
                     QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE);
diff --git a/tests/qemuxml2xmloutdata/pseries-features.xml b/tests/qemuxml2xmloutdata/pseries-features.xml
index f36705f011..55a44c75a0 100644
--- a/tests/qemuxml2xmloutdata/pseries-features.xml
+++ b/tests/qemuxml2xmloutdata/pseries-features.xml
@@ -12,6 +12,7 @@
     <hpt resizing='required'>
       <maxpagesize unit='KiB'>1048576</maxpagesize>
     </hpt>
+    <htm state='on'/>
   </features>
   <clock offset='utc'/>
   <on_poweroff>destroy</on_poweroff>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index eac6d5b073..bbb995656e 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -620,6 +620,7 @@ mymain(void)
     DO_TEST("pseries-features",
             QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE,
             QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE,
+            QEMU_CAPS_MACHINE_PSERIES_CAP_HTM,
             QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT);
 
     DO_TEST("pseries-serial-native",
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/3] qemu: Implement the HTM pSeries feature
Posted by John Ferlan 7 years, 7 months ago

On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1525599
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  docs/formatdomain.html.in                     |  8 ++++++++
>  docs/schemas/domaincommon.rng                 |  5 +++++
>  src/conf/domain_conf.c                        | 19 ++++++++++++++++++
>  src/conf/domain_conf.h                        |  1 +
>  src/qemu/qemu_command.c                       | 20 +++++++++++++++++++
>  src/qemu/qemu_domain.c                        | 13 ++++++++++++
>  tests/qemuxml2argvdata/pseries-features.args  |  2 +-
>  tests/qemuxml2argvdata/pseries-features.xml   |  1 +
>  tests/qemuxml2argvtest.c                      |  1 +
>  tests/qemuxml2xmloutdata/pseries-features.xml |  1 +
>  tests/qemuxml2xmltest.c                       |  1 +
>  11 files changed, 71 insertions(+), 1 deletion(-)
> 

Even though it's QEMU/KVM only - the conf/docs/xml2xml should be
separated from the qemu/xml2argv changes.

[...]


> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 0d68596991..73db315c4f 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1929,6 +1929,7 @@
>    &lt;smm state='on'&gt;
>      &lt;tseg unit='MiB'&gt;48&lt;/tseg&gt;
>    &lt;/smm&gt;
> +  &lt;htm state='on'/&gt;
>  &lt;/features&gt;
>  ...</pre>
>  
> @@ -2162,6 +2163,13 @@
>        <dd>Enable QEMU vmcoreinfo device to let the guest kernel save debug
>            details. <span class="since">Since 4.4.0</span> (QEMU only)
>        </dd>
> +      <dt><code>htm</code></dt>
> +      <dd>Configure HTM (Hardware Transational Memory) availability for
> +          pSeries guests. Possible values for the <code>state</code> attribute
> +          are <code>on</code> and <code>off</code>. If the attribute is not
> +          defined, the hypervisor default will be used.
> +          <span class="since">Since 4.5.0</span> (QEMU/KVM only)

This'll miss 4.5, so change to 4.6 before pushing

> +      </dd>
>      </dl>
>  
>      <h3><a id="elementsTime">Time keeping</a></h3>

[...]

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d8cb7f37f3..ea01d08a3c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -152,6 +152,7 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST,
>                "ioapic",
>                "hpt",
>                "vmcoreinfo",
> +              "htm",
>  );
>  
>  VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST,
> @@ -19827,6 +19828,22 @@ virDomainDefParseXML(xmlDocPtr xml,
>              }
>              break;
>  
> +        case VIR_DOMAIN_FEATURE_HTM:
> +            if (!(tmp = virXMLPropString(nodes[i], "state"))) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("missing state attribute '%s' of feature '%s'"),
> +                               tmp, virDomainFeatureTypeToString(val));
> +                goto error;
> +            }
> +            if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) < 0) {

[1] checked here, so that...

> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("unknown state attribute '%s' of feature '%s'"),
> +                               tmp, virDomainFeatureTypeToString(val));
> +                goto error;
> +            }
> +            VIR_FREE(tmp);
> +            break;
> +

[...]

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a357d2199c..5342c08f19 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7291,6 +7291,26 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>          }
>      }
>  
> +    if (def->features[VIR_DOMAIN_FEATURE_HTM] != VIR_TRISTATE_SWITCH_ABSENT) {
> +        const char *str;
> +
> +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HTM)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("HTM configuration is not supported by this "
> +                             "QEMU binary"));
> +            goto cleanup;
> +        }
> +
> +        str = virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM]);
> +        if (!str) {


[1] ... this is unnecessary due to virDomainDefParseXML check.


> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("Invalid setting for HTM state"));
> +            goto cleanup;
> +        }
> +
> +        virBufferAsprintf(&buf, ",cap-htm=%s", str);

Just remove @str, and directly call/use:

virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM])

> +    }
> +
>      if (cpu && cpu->model &&
>          cpu->mode == VIR_CPU_MODE_HOST_MODEL &&
>          qemuDomainIsPSeries(def) &&

[...]

with the adjustments and splitting into two patches (I trust you have
that capability),

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/3] qemu: Implement the HTM pSeries feature
Posted by Andrea Bolognani 7 years, 7 months ago
On Sat, 2018-06-30 at 07:23 -0400, John Ferlan wrote:
> On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1525599
> > Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> > ---
> >  docs/formatdomain.html.in                     |  8 ++++++++
> >  docs/schemas/domaincommon.rng                 |  5 +++++
> >  src/conf/domain_conf.c                        | 19 ++++++++++++++++++
> >  src/conf/domain_conf.h                        |  1 +
> >  src/qemu/qemu_command.c                       | 20 +++++++++++++++++++
> >  src/qemu/qemu_domain.c                        | 13 ++++++++++++
> >  tests/qemuxml2argvdata/pseries-features.args  |  2 +-
> >  tests/qemuxml2argvdata/pseries-features.xml   |  1 +
> >  tests/qemuxml2argvtest.c                      |  1 +
> >  tests/qemuxml2xmloutdata/pseries-features.xml |  1 +
> >  tests/qemuxml2xmltest.c                       |  1 +
> >  11 files changed, 71 insertions(+), 1 deletion(-)
> 
> Even though it's QEMU/KVM only - the conf/docs/xml2xml should be
> separated from the qemu/xml2argv changes.

Can do.

> > @@ -2162,6 +2163,13 @@
> >        <dd>Enable QEMU vmcoreinfo device to let the guest kernel save debug
> >            details. <span class="since">Since 4.4.0</span> (QEMU only)
> >        </dd>
> > +      <dt><code>htm</code></dt>
> > +      <dd>Configure HTM (Hardware Transational Memory) availability for
> > +          pSeries guests. Possible values for the <code>state</code> attribute
> > +          are <code>on</code> and <code>off</code>. If the attribute is not
> > +          defined, the hypervisor default will be used.
> > +          <span class="since">Since 4.5.0</span> (QEMU/KVM only)
> 
> This'll miss 4.5, so change to 4.6 before pushing

Sure.

> > +        case VIR_DOMAIN_FEATURE_HTM:
> > +            if (!(tmp = virXMLPropString(nodes[i], "state"))) {
> > +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                               _("missing state attribute '%s' of feature '%s'"),
> > +                               tmp, virDomainFeatureTypeToString(val));
> > +                goto error;
> > +            }
> > +            if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) < 0) {
> 
> [1] checked here, so that...
> 
> > +    if (def->features[VIR_DOMAIN_FEATURE_HTM] != VIR_TRISTATE_SWITCH_ABSENT) {
> > +        const char *str;
> > +
> > +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HTM)) {
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("HTM configuration is not supported by this "
> > +                             "QEMU binary"));
> > +            goto cleanup;
> > +        }
> > +
> > +        str = virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM]);
> > +        if (!str) {
> 
> [1] ... this is unnecessary due to virDomainDefParseXML check.

I strongly dislike the practice of skipping checks in a function
with the rationale that they're already performed somewhere else in
the code base: libvirt is in a state of constant flux, and as stuff
gets moved around you might very well find yourself no longer as
shielded from invalid values as you thought you were. Local sanity
checks should IMHO always be performed locally.

tl;dr I'd really rather keep this in.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 2/3] qemu: Implement the HTM pSeries feature
Posted by John Ferlan 7 years, 7 months ago

On 07/02/2018 04:19 AM, Andrea Bolognani wrote:
> On Sat, 2018-06-30 at 07:23 -0400, John Ferlan wrote:
>> On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1525599
>>> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>>> ---
>>>  docs/formatdomain.html.in                     |  8 ++++++++
>>>  docs/schemas/domaincommon.rng                 |  5 +++++
>>>  src/conf/domain_conf.c                        | 19 ++++++++++++++++++
>>>  src/conf/domain_conf.h                        |  1 +
>>>  src/qemu/qemu_command.c                       | 20 +++++++++++++++++++
>>>  src/qemu/qemu_domain.c                        | 13 ++++++++++++
>>>  tests/qemuxml2argvdata/pseries-features.args  |  2 +-
>>>  tests/qemuxml2argvdata/pseries-features.xml   |  1 +
>>>  tests/qemuxml2argvtest.c                      |  1 +
>>>  tests/qemuxml2xmloutdata/pseries-features.xml |  1 +
>>>  tests/qemuxml2xmltest.c                       |  1 +
>>>  11 files changed, 71 insertions(+), 1 deletion(-)
>>
>> Even though it's QEMU/KVM only - the conf/docs/xml2xml should be
>> separated from the qemu/xml2argv changes.
> 
> Can do.
> 
>>> @@ -2162,6 +2163,13 @@
>>>        <dd>Enable QEMU vmcoreinfo device to let the guest kernel save debug
>>>            details. <span class="since">Since 4.4.0</span> (QEMU only)
>>>        </dd>
>>> +      <dt><code>htm</code></dt>
>>> +      <dd>Configure HTM (Hardware Transational Memory) availability for
>>> +          pSeries guests. Possible values for the <code>state</code> attribute
>>> +          are <code>on</code> and <code>off</code>. If the attribute is not
>>> +          defined, the hypervisor default will be used.
>>> +          <span class="since">Since 4.5.0</span> (QEMU/KVM only)
>>
>> This'll miss 4.5, so change to 4.6 before pushing
> 
> Sure.
> 
>>> +        case VIR_DOMAIN_FEATURE_HTM:
>>> +            if (!(tmp = virXMLPropString(nodes[i], "state"))) {
>>> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> +                               _("missing state attribute '%s' of feature '%s'"),
>>> +                               tmp, virDomainFeatureTypeToString(val));
>>> +                goto error;
>>> +            }
>>> +            if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) < 0) {
>>
>> [1] checked here, so that...
>>
>>> +    if (def->features[VIR_DOMAIN_FEATURE_HTM] != VIR_TRISTATE_SWITCH_ABSENT) {
>>> +        const char *str;
>>> +
>>> +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HTM)) {
>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +                           _("HTM configuration is not supported by this "
>>> +                             "QEMU binary"));
>>> +            goto cleanup;
>>> +        }
>>> +
>>> +        str = virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM]);
>>> +        if (!str) {
>>
>> [1] ... this is unnecessary due to virDomainDefParseXML check.
> 
> I strongly dislike the practice of skipping checks in a function
> with the rationale that they're already performed somewhere else in
> the code base: libvirt is in a state of constant flux, and as stuff
> gets moved around you might very well find yourself no longer as
> shielded from invalid values as you thought you were. Local sanity
> checks should IMHO always be performed locally.
> 
> tl;dr I'd really rather keep this in.
> 

That's fine - I don't mind the check, just pointing out it should be
unnecessary. You can leave it in and the R-by still stands.

John

htm vs. hpt, yep the tla's and seeing pseries with memory related
functionality for both certainly led me down that path...

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list