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

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(a)redhat.com&gt; >> --- >> 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.