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@xxxxxxxxxx> >>> --- >>> 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
