[libvirt] [PATCH 00/11] Fixes and improvements to feature handling
This series is the result of shaving many a yak while implementing support for pSeries optional features (first attempt at [1], respin coming right after I send this). Quick summary of the changes: * handle more sanely cases where the GIC capability has been enabled but no GIC version has been provided; * use the switch construct more, so that the compiler can warn us when we forget to handle some feature; * where possible (eg. the value is mandatory and there is only a limited number of options to choose from) avoid storing whether a feature is enabled and its actual value separately. Save for the first one, none of the changes should be visible to the user, but they will hopefully improve things for developers. [1] https://www.redhat.com/archives/libvir-list/2018-January/msg00779.html Andrea Bolognani (11): qemu: Move feature verification from PostParse() to Validate() qemu: Use switch in qemuDomainDefValidateFeatures() qemu: Move GIC checks to qemuDomainDefValidateFeatures() conf: Use switch in virDomainDefFeaturesCheckABIStability() conf: Validate VIR_DOMAIN_FEATURE_CAPABILITIES properly conf: Integrate all features ABI checks in the switch tests: Improve GIC tests qemu: Fix GIC behavior for the default case conf: Improve IOAPIC feature handling conf: Improve HPT feature handling tests: Clean up HPT tests src/conf/domain_conf.c | 174 +++++++++++++-------- src/conf/domain_conf.h | 15 +- src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 16 +- src/qemu/qemu_domain.c | 117 +++++++++----- .../qemuxml2argvdata/aarch64-gic-default-both.args | 1 + .../qemuxml2argvdata/aarch64-gic-default-both.xml | 1 + tests/qemuxml2argvdata/aarch64-gic-default-v2.args | 1 + tests/qemuxml2argvdata/aarch64-gic-default-v2.xml | 1 + tests/qemuxml2argvdata/aarch64-gic-default-v3.args | 1 + tests/qemuxml2argvdata/aarch64-gic-default-v3.xml | 1 + ...hpt-resizing.args => pseries-features-hpt.args} | 1 - .../pseries-features-hpt.xml} | 0 ...ne.xml => pseries-features-invalid-machine.xml} | 2 +- tests/qemuxml2argvdata/pseries-hpt-resizing.xml | 19 --- tests/qemuxml2argvtest.c | 29 ++-- .../aarch64-gic-default-both.xml | 1 + .../qemuxml2xmloutdata/aarch64-gic-default-v2.xml | 1 + .../qemuxml2xmloutdata/aarch64-gic-default-v3.xml | 1 + tests/qemuxml2xmloutdata/pseries-features-hpt.xml | 1 + tests/qemuxml2xmltest.c | 9 +- 21 files changed, 230 insertions(+), 164 deletions(-) create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-both.args create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-both.xml create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v2.args create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v2.xml create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v3.args create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v3.xml rename tests/qemuxml2argvdata/{pseries-hpt-resizing.args => pseries-features-hpt.args} (96%) rename tests/{qemuxml2xmloutdata/pseries-hpt-resizing.xml => qemuxml2argvdata/pseries-features-hpt.xml} (100%) rename tests/qemuxml2argvdata/{pseries-hpt-resizing-invalid-machine.xml => pseries-features-invalid-machine.xml} (86%) delete mode 100644 tests/qemuxml2argvdata/pseries-hpt-resizing.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml create mode 120000 tests/qemuxml2xmloutdata/pseries-features-hpt.xml -- 2.14.3
We want to perform all feature verification in a single spot, but some of it (eg. GIC) is currently being performed at command line generation time, and moving it to PostParse() would cause guests to disappear. Moving verification to Validate() allows us to side-step the issue. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index df433c2f0..27a4751e9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3113,30 +3113,6 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def) } -static int -qemuDomainDefVerifyFeatures(const virDomainDef *def) -{ - if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] == VIR_TRISTATE_SWITCH_ON && - !ARCH_IS_X86(def->os.arch)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("I/O APIC tuning is not supported " - "for '%s' architecture"), - virArchToString(def->os.arch)); - return -1; - } - - if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON && - !qemuDomainIsPSeries(def)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", - _("HPT tuning is only supported for pSeries guests")); - return -1; - } - - return 0; -} - - static int qemuDomainDefPostParseBasic(virDomainDefPtr def, virCapsPtr caps, @@ -3195,9 +3171,6 @@ qemuDomainDefPostParse(virDomainDefPtr def, qemuDomainDefEnableDefaultFeatures(def, qemuCaps); - if (qemuDomainDefVerifyFeatures(def) < 0) - goto cleanup; - if (qemuDomainRecheckInternalPaths(def, cfg, parseFlags) < 0) goto cleanup; @@ -3250,6 +3223,30 @@ qemuDomainDefGetVcpuHotplugGranularity(const virDomainDef *def) #define QEMU_MAX_VCPUS_WITHOUT_EIM 255 +static int +qemuDomainDefValidateFeatures(const virDomainDef *def) +{ + if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] == VIR_TRISTATE_SWITCH_ON && + !ARCH_IS_X86(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("I/O APIC tuning is not supported " + "for '%s' architecture"), + virArchToString(def->os.arch)); + return -1; + } + + if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON && + !qemuDomainIsPSeries(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", + _("HPT tuning is only supported for pSeries guests")); + return -1; + } + + return 0; +} + + static int qemuDomainDefValidate(const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, @@ -3362,6 +3359,9 @@ qemuDomainDefValidate(const virDomainDef *def, } } + if (qemuDomainDefValidateFeatures(def) < 0) + goto cleanup; + ret = 0; cleanup: -- 2.14.3
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
We want to perform all feature verification in a single spot, but some of it (eg. GIC) is currently being performed at command line generation time, and moving it to PostParse() would cause guests to disappear. Moving verification to Validate() allows us to side-step the issue.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 27 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
The compiler can make sure we are handling all features. While reworking the logic, also change error messages to a more consistent style. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_domain.c | 57 ++++++++++++++++++++++++++++++++++++------------ 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0bce0bbfb..17c3b71e0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -345,6 +345,8 @@ virDomainDiskSetDriver; virDomainDiskSetFormat; virDomainDiskSetSource; virDomainDiskSetType; +virDomainFeatureTypeFromString; +virDomainFeatureTypeToString; virDomainFSDefFree; virDomainFSDefNew; virDomainFSIndexByName; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 27a4751e9..dd36b42eb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3226,21 +3226,50 @@ qemuDomainDefGetVcpuHotplugGranularity(const virDomainDef *def) static int qemuDomainDefValidateFeatures(const virDomainDef *def) { - if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] == VIR_TRISTATE_SWITCH_ON && - !ARCH_IS_X86(def->os.arch)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("I/O APIC tuning is not supported " - "for '%s' architecture"), - virArchToString(def->os.arch)); - return -1; - } + size_t i; - if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON && - !qemuDomainIsPSeries(def)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - "%s", - _("HPT tuning is only supported for pSeries guests")); - return -1; + for (i = 0; i < VIR_DOMAIN_FEATURE_LAST; i++) { + const char *featureName = virDomainFeatureTypeToString(i); + + switch ((virDomainFeature) i) { + case VIR_DOMAIN_FEATURE_IOAPIC: + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !ARCH_IS_X86(def->os.arch)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The '%s' feature is only supported for %s guests"), + featureName, "x86"); + return -1; + } + break; + + case VIR_DOMAIN_FEATURE_HPT: + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !qemuDomainIsPSeries(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The '%s' feature is only supported for %s guests"), + featureName, "pSeries"); + return -1; + } + break; + + case VIR_DOMAIN_FEATURE_ACPI: + case VIR_DOMAIN_FEATURE_APIC: + case VIR_DOMAIN_FEATURE_PAE: + case VIR_DOMAIN_FEATURE_HAP: + case VIR_DOMAIN_FEATURE_VIRIDIAN: + case VIR_DOMAIN_FEATURE_PRIVNET: + case VIR_DOMAIN_FEATURE_HYPERV: + case VIR_DOMAIN_FEATURE_KVM: + case VIR_DOMAIN_FEATURE_PVSPINLOCK: + case VIR_DOMAIN_FEATURE_CAPABILITIES: + case VIR_DOMAIN_FEATURE_PMU: + case VIR_DOMAIN_FEATURE_VMPORT: + case VIR_DOMAIN_FEATURE_GIC: + case VIR_DOMAIN_FEATURE_SMM: + case VIR_DOMAIN_FEATURE_VMCOREINFO: + case VIR_DOMAIN_FEATURE_LAST: + break; + } } return 0; -- 2.14.3
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
The compiler can make sure we are handling all features.
While reworking the logic, also change error messages to a more consistent style.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_domain.c | 57 ++++++++++++++++++++++++++++++++++++------------ 2 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0bce0bbfb..17c3b71e0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -345,6 +345,8 @@ virDomainDiskSetDriver; virDomainDiskSetFormat; virDomainDiskSetSource; virDomainDiskSetType;
You'll have a merge conflict right about here... Reviewed-by: John Ferlan <jferlan@redhat.com> John
Keep them along with other arch/machine type checks for features instead of waiting until command line generation time. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 7 ------- src/qemu/qemu_domain.c | 11 ++++++++++- tests/qemuxml2argvtest.c | 12 ++++++------ 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 24b434a45..529079be0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7192,13 +7192,6 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) { if (def->gic_version != VIR_GIC_VERSION_NONE) { - if (!qemuDomainIsVirt(def)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("gic-version option is available " - "only for ARM virt machine")); - goto cleanup; - } - /* The default GIC version (GICv2) should not be specified on * the QEMU commandline for backwards compatibility reasons */ if (def->gic_version != VIR_GIC_VERSION_2) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index dd36b42eb..98cba8789 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3252,6 +3252,16 @@ qemuDomainDefValidateFeatures(const virDomainDef *def) } break; + case VIR_DOMAIN_FEATURE_GIC: + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !qemuDomainIsVirt(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The '%s' feature is only supported for %s guests"), + featureName, "mach-virt"); + return -1; + } + break; + case VIR_DOMAIN_FEATURE_ACPI: case VIR_DOMAIN_FEATURE_APIC: case VIR_DOMAIN_FEATURE_PAE: @@ -3264,7 +3274,6 @@ qemuDomainDefValidateFeatures(const virDomainDef *def) case VIR_DOMAIN_FEATURE_CAPABILITIES: case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_VMPORT: - case VIR_DOMAIN_FEATURE_GIC: case VIR_DOMAIN_FEATURE_SMM: case VIR_DOMAIN_FEATURE_VMCOREINFO: case VIR_DOMAIN_FEATURE_LAST: diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c8739909d..b7afb6980 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2656,12 +2656,12 @@ mymain(void) DO_TEST_PARSE_ERROR("aarch64-gic-invalid", QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACH_VIRT_GIC_VERSION); - DO_TEST_FAILURE("aarch64-gic-not-virt", - QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, - QEMU_CAPS_MACH_VIRT_GIC_VERSION); - DO_TEST_FAILURE("aarch64-gic-not-arm", - QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, - QEMU_CAPS_MACH_VIRT_GIC_VERSION); + DO_TEST_PARSE_ERROR("aarch64-gic-not-virt", + QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, + QEMU_CAPS_MACH_VIRT_GIC_VERSION); + DO_TEST_PARSE_ERROR("aarch64-gic-not-arm", + QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, + QEMU_CAPS_MACH_VIRT_GIC_VERSION); DO_TEST("aarch64-kvm-32-on-64", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_VIRTIO_MMIO, QEMU_CAPS_DEVICE_PL011, -- 2.14.3
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
Keep them along with other arch/machine type checks for features instead of waiting until command line generation time.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_command.c | 7 ------- src/qemu/qemu_domain.c | 11 ++++++++++- tests/qemuxml2argvtest.c | 12 ++++++------ 3 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 24b434a45..529079be0 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7192,13 +7192,6 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) { if (def->gic_version != VIR_GIC_VERSION_NONE) { - if (!qemuDomainIsVirt(def)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("gic-version option is available " - "only for ARM virt machine")); - goto cleanup; - } - /* The default GIC version (GICv2) should not be specified on * the QEMU commandline for backwards compatibility reasons */ if (def->gic_version != VIR_GIC_VERSION_2) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index dd36b42eb..98cba8789 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3252,6 +3252,16 @@ qemuDomainDefValidateFeatures(const virDomainDef *def) } break;
+ case VIR_DOMAIN_FEATURE_GIC: + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !qemuDomainIsVirt(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The '%s' feature is only supported for %s guests"),
s/for %s guests/for '%s' guests/ ??
+ featureName, "mach-virt");
Not that I think it matters greatly, the error message changes from ARM specifically to mach-virt... I guess I'm just used to seeing 'ARM' or 'aarch64' and not 'mach-virt' (although the XML would be AIUI "<type arch='aarch64' machine='virt'>"). Suffice to say it caused me to wonder and I have to imagine it would do the same for anyone getting that message. I don't have a strong opinion one way or other and it's not really easily "decode-able" based on someone just reading the "<os... <type ... machine=''..." in the docs. Still - the change otherwise seems fine. I trust you'll come up with the right magical phrase ;-) Reviewed-by: John Ferlan <jferlan@redhat.com> John
On Mon, 2018-02-12 at 14:59 -0500, John Ferlan wrote:
@@ -3252,6 +3252,16 @@ qemuDomainDefValidateFeatures(const virDomainDef *def) } break;
+ case VIR_DOMAIN_FEATURE_GIC: + if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + !qemuDomainIsVirt(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The '%s' feature is only supported for %s guests"),
s/for %s guests/for '%s' guests/ ??
Sure, why not :)
+ featureName, "mach-virt");
Not that I think it matters greatly, the error message changes from ARM specifically to mach-virt... I guess I'm just used to seeing 'ARM' or 'aarch64' and not 'mach-virt' (although the XML would be AIUI "<type arch='aarch64' machine='virt'>"). Suffice to say it caused me to wonder and I have to imagine it would do the same for anyone getting that message.
I don't have a strong opinion one way or other and it's not really easily "decode-able" based on someone just reading the "<os... <type ... machine=''..." in the docs.
Yeah, we're being quite inconsistent throughout both the code and the test suite. The root of the problem is that, while other architectures use very recognizable or at least distinct names for the machine types we care about, such as pseries or q35, on aarch64 the machine type is just... virt. Which is not really a great name. But we can't just use the architecture name either, because there are a lot of arm and aarch64 machine types out there. Using mach-virt is an attempt to sidestep the problem, and it's used (at least internally) in QEMU. But I agree it can be confusing. Maybe the solution in this case is to use the approach originally used for the <ioapic/> feature and just say The '%s' feature is not supported for architecture '%s' or machine type '%s' though that's slightly less helpful for users... -- Andrea Bolognani / Red Hat / Virtualization
The compiler can make sure we are handling all features. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 34aae82f1..e4d01b869 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21324,14 +21324,39 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, size_t i; for (i = 0; i < VIR_DOMAIN_FEATURE_LAST; i++) { - if (src->features[i] != dst->features[i]) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("State of feature '%s' differs: " - "source: '%s', destination: '%s'"), - virDomainFeatureTypeToString(i), - virTristateSwitchTypeToString(src->features[i]), - virTristateSwitchTypeToString(dst->features[i])); - return false; + const char *featureName = virDomainFeatureTypeToString(i); + + switch ((virDomainFeature) i) { + case VIR_DOMAIN_FEATURE_ACPI: + case VIR_DOMAIN_FEATURE_APIC: + case VIR_DOMAIN_FEATURE_PAE: + case VIR_DOMAIN_FEATURE_HAP: + case VIR_DOMAIN_FEATURE_VIRIDIAN: + case VIR_DOMAIN_FEATURE_PRIVNET: + case VIR_DOMAIN_FEATURE_HYPERV: + case VIR_DOMAIN_FEATURE_KVM: + case VIR_DOMAIN_FEATURE_PVSPINLOCK: + case VIR_DOMAIN_FEATURE_CAPABILITIES: + case VIR_DOMAIN_FEATURE_PMU: + case VIR_DOMAIN_FEATURE_VMPORT: + case VIR_DOMAIN_FEATURE_GIC: + case VIR_DOMAIN_FEATURE_SMM: + case VIR_DOMAIN_FEATURE_IOAPIC: + case VIR_DOMAIN_FEATURE_HPT: + case VIR_DOMAIN_FEATURE_VMCOREINFO: + if (src->features[i] != dst->features[i]) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("State of feature '%s' differs: " + "source: '%s', destination: '%s'"), + featureName, + virTristateSwitchTypeToString(src->features[i]), + virTristateSwitchTypeToString(dst->features[i])); + return false; + } + break; + + case VIR_DOMAIN_FEATURE_LAST: + break; } } -- 2.14.3
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
The compiler can make sure we are handling all features.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Unlike most other features, VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy instead of virTristateSwitch, so we need to handle it separately for the error message to make sense. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e4d01b869..9f019c906 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21336,7 +21336,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, case VIR_DOMAIN_FEATURE_HYPERV: case VIR_DOMAIN_FEATURE_KVM: case VIR_DOMAIN_FEATURE_PVSPINLOCK: - case VIR_DOMAIN_FEATURE_CAPABILITIES: case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_VMPORT: case VIR_DOMAIN_FEATURE_GIC: @@ -21355,6 +21354,18 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, } break; + case VIR_DOMAIN_FEATURE_CAPABILITIES: + if (src->features[i] != dst->features[i]) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("State of feature '%s:%s' differs: " + "source: '%s', destination: '%s'"), + featureName, "policy", + virDomainCapabilitiesPolicyTypeToString(src->features[i]), + virDomainCapabilitiesPolicyTypeToString(dst->features[i])); + return false; + } + break; + case VIR_DOMAIN_FEATURE_LAST: break; } -- 2.14.3
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
Unlike most other features, VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy instead of virTristateSwitch, so we need to handle it separately for the error message to make sense.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e4d01b869..9f019c906 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21336,7 +21336,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, case VIR_DOMAIN_FEATURE_HYPERV: case VIR_DOMAIN_FEATURE_KVM: case VIR_DOMAIN_FEATURE_PVSPINLOCK: - case VIR_DOMAIN_FEATURE_CAPABILITIES: case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_VMPORT: case VIR_DOMAIN_FEATURE_GIC: @@ -21355,6 +21354,18 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, } break;
+ case VIR_DOMAIN_FEATURE_CAPABILITIES: + if (src->features[i] != dst->features[i]) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("State of feature '%s:%s' differs: " + "source: '%s', destination: '%s'"), + featureName, "policy",
OK - based on what I saw Peter ACK for Michal's patches related to his similar usage, fine. Still looks strange especially since we're talking about a named XML attribute which we document as "policy". I just hope there isn't some language variant that alters the meaning. It does look strange to me "State of feature 'capabilities:policy' differs: "... I almost wonder if "State of feature 'capabilities' attribute 'policy' differs: " would help (or really matter). Also, based on the tests/domainschemadata/domain-caps-features.xml: <capabilities policy="deny"> <mknod state="on"/> </capabilities> /me wonders how many more yaks might get shaved if we needed to check differences w/r/t the caps_features array? IOW: If src->features[i] != VIR_DOMAIN_CAPABILITIES_POLICY_DEFAULT, then should we be checking each of the VIR_DOMAIN_CAPS_FEATURE_LAST to ensure that they're not different too. Not something required for this patch, but perhaps a follow-up... Reviewed-by: John Ferlan <jferlan@redhat.com>
+ virDomainCapabilitiesPolicyTypeToString(src->features[i]), + virDomainCapabilitiesPolicyTypeToString(dst->features[i])); + return false; + } + break; + case VIR_DOMAIN_FEATURE_LAST: break; }
On Mon, 2018-02-12 at 15:18 -0500, John Ferlan wrote:
+ case VIR_DOMAIN_FEATURE_CAPABILITIES: + if (src->features[i] != dst->features[i]) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("State of feature '%s:%s' differs: " + "source: '%s', destination: '%s'"), + featureName, "policy",
OK - based on what I saw Peter ACK for Michal's patches related to his similar usage, fine. Still looks strange especially since we're talking about a named XML attribute which we document as "policy". I just hope there isn't some language variant that alters the meaning. It does look strange to me "State of feature 'capabilities:policy' differs: "... I almost wonder if "State of feature 'capabilities' attribute 'policy' differs: " would help (or really matter).
Hm. Maybe it could look like virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of feature '%s' differs: " "source: '%s,%s=%s', destination: '%s,%s=%s'"), featureName, virTristateSwitchTypeToString(src->features[i]), "version", virGICVersionTypeToString(src->gic_version), virTristateSwitchTypeToString(dst->features[i]), "version", virGICVersionTypeToString(dst->gic_version)); so that the state of the feature itself and that of its various attributes, when present, would be displayed without ambiguity.
Also, based on the tests/domainschemadata/domain-caps-features.xml:
<capabilities policy="deny"> <mknod state="on"/> </capabilities>
/me wonders how many more yaks might get shaved if we needed to check differences w/r/t the caps_features array?
IOW: If src->features[i] != VIR_DOMAIN_CAPABILITIES_POLICY_DEFAULT, then should we be checking each of the VIR_DOMAIN_CAPS_FEATURE_LAST to ensure that they're not different too.
Not something required for this patch, but perhaps a follow-up...
For some reason, I thought caps_features were handled later on along with kvm_features and hyperv_features, but I see now that's not the case. I agree that's something that should be addressed. -- Andrea Bolognani / Red Hat / Virtualization
There are a few stray checks which still live outside of the switch in virDomainDefFeaturesCheckABIStability() for no good reason. Move them inside the switch, and update the error messages to be consistent while at it. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 105 ++++++++++++++++++++++++++++--------------------- 1 file changed, 60 insertions(+), 45 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9f019c906..170c56665 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21328,7 +21328,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, switch ((virDomainFeature) i) { case VIR_DOMAIN_FEATURE_ACPI: - case VIR_DOMAIN_FEATURE_APIC: case VIR_DOMAIN_FEATURE_PAE: case VIR_DOMAIN_FEATURE_HAP: case VIR_DOMAIN_FEATURE_VIRIDIAN: @@ -21338,10 +21337,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, case VIR_DOMAIN_FEATURE_PVSPINLOCK: case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_VMPORT: - case VIR_DOMAIN_FEATURE_GIC: case VIR_DOMAIN_FEATURE_SMM: - case VIR_DOMAIN_FEATURE_IOAPIC: - case VIR_DOMAIN_FEATURE_HPT: case VIR_DOMAIN_FEATURE_VMCOREINFO: if (src->features[i] != dst->features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -21366,28 +21362,69 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, } break; - case VIR_DOMAIN_FEATURE_LAST: + case VIR_DOMAIN_FEATURE_GIC: + if (src->features[i] != dst->features[i] || + src->gic_version != dst->gic_version) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("State of feature '%s:%s' differs: " + "source: '%s:%s', destination: '%s:%s'"), + featureName, "version", + virTristateSwitchTypeToString(src->features[i]), + virGICVersionTypeToString(src->gic_version), + virTristateSwitchTypeToString(dst->features[i]), + virGICVersionTypeToString(dst->gic_version)); + return false; + } break; - } - } - /* APIC EOI */ - if (src->apic_eoi != dst->apic_eoi) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("State of APIC EOI differs: " - "source: '%s', destination: '%s'"), - virTristateSwitchTypeToString(src->apic_eoi), - virTristateSwitchTypeToString(dst->apic_eoi)); - return false; - } + case VIR_DOMAIN_FEATURE_HPT: + if (src->features[i] != dst->features[i] || + src->hpt_resizing != dst->hpt_resizing) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("State of feature '%s:%s' differs: " + "source: '%s:%s', destination: '%s:%s'"), + featureName, "resizing", + virTristateSwitchTypeToString(src->features[i]), + virDomainHPTResizingTypeToString(src->hpt_resizing), + virTristateSwitchTypeToString(dst->features[i]), + virDomainHPTResizingTypeToString(dst->hpt_resizing)); + return false; + } + break; - /* GIC version */ - if (src->gic_version != dst->gic_version) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Source GIC version '%s' does not match destination '%s'"), - virGICVersionTypeToString(src->gic_version), - virGICVersionTypeToString(dst->gic_version)); - return false; + case VIR_DOMAIN_FEATURE_APIC: + if (src->features[i] != dst->features[i] || + src->apic_eoi != dst->apic_eoi) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("State of feature '%s:%s' differs: " + "source: '%s:%s', destination: '%s:%s'"), + featureName, "eoi", + virTristateSwitchTypeToString(src->features[i]), + virTristateSwitchTypeToString(src->apic_eoi), + virTristateSwitchTypeToString(dst->features[i]), + virTristateSwitchTypeToString(dst->apic_eoi)); + return false; + } + break; + + case VIR_DOMAIN_FEATURE_IOAPIC: + if (src->features[i] != dst->features[i] || + src->ioapic != dst->ioapic) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("State of feature '%s:%s' differs: " + "source: '%s:%s', destination: '%s:%s'"), + featureName, "driver", + virTristateSwitchTypeToString(src->features[i]), + virDomainIOAPICTypeToString(src->ioapic), + virTristateSwitchTypeToString(dst->features[i]), + virDomainIOAPICTypeToString(dst->ioapic)); + return false; + } + break; + + case VIR_DOMAIN_FEATURE_LAST: + break; + } } /* hyperv */ @@ -21468,28 +21505,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, } } - /* ioapic */ - if (src->ioapic != dst->ioapic) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("State of ioapic differs: " - "source: '%s', destination: '%s'"), - virDomainIOAPICTypeToString(src->ioapic), - virDomainIOAPICTypeToString(dst->ioapic)); - return false; - } - - /* HPT resizing */ - if (src->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON) { - if (src->hpt_resizing != dst->hpt_resizing) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("HPT resizing configuration differs: " - "source: '%s', destination: '%s'"), - virDomainHPTResizingTypeToString(src->hpt_resizing), - virDomainHPTResizingTypeToString(dst->hpt_resizing)); - return false; - } - } - return true; } -- 2.14.3
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
There are a few stray checks which still live outside of the switch in virDomainDefFeaturesCheckABIStability() for no good reason. Move them inside the switch, and update the error messages to be consistent while at it.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 105 ++++++++++++++++++++++++++++--------------------- 1 file changed, 60 insertions(+), 45 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9f019c906..170c56665 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21328,7 +21328,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
switch ((virDomainFeature) i) { case VIR_DOMAIN_FEATURE_ACPI: - case VIR_DOMAIN_FEATURE_APIC: case VIR_DOMAIN_FEATURE_PAE: case VIR_DOMAIN_FEATURE_HAP: case VIR_DOMAIN_FEATURE_VIRIDIAN: @@ -21338,10 +21337,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, case VIR_DOMAIN_FEATURE_PVSPINLOCK: case VIR_DOMAIN_FEATURE_PMU: case VIR_DOMAIN_FEATURE_VMPORT: - case VIR_DOMAIN_FEATURE_GIC: case VIR_DOMAIN_FEATURE_SMM: - case VIR_DOMAIN_FEATURE_IOAPIC: - case VIR_DOMAIN_FEATURE_HPT: case VIR_DOMAIN_FEATURE_VMCOREINFO: if (src->features[i] != dst->features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -21366,28 +21362,69 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, } break;
- case VIR_DOMAIN_FEATURE_LAST: + case VIR_DOMAIN_FEATURE_GIC: + if (src->features[i] != dst->features[i] || + src->gic_version != dst->gic_version) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("State of feature '%s:%s' differs: " + "source: '%s:%s', destination: '%s:%s'"), + featureName, "version", + virTristateSwitchTypeToString(src->features[i]), + virGICVersionTypeToString(src->gic_version), + virTristateSwitchTypeToString(dst->features[i]), + virGICVersionTypeToString(dst->gic_version)); + return false;
Similar to previous, should these become "State of feature '%s' attribute 'version' differs: " likewise for other error messages.... Leave it up to you to decide though. Reviewed-by: John Ferlan <jferlan@redhat.com> John
Account for the fact that the default might change based on what GIC versions are supported by QEMU. That's not the case at the moment, but it will be soon. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2argvdata/aarch64-gic-default-both.args | 1 + tests/qemuxml2argvdata/aarch64-gic-default-both.xml | 1 + tests/qemuxml2argvdata/aarch64-gic-default-v2.args | 1 + tests/qemuxml2argvdata/aarch64-gic-default-v2.xml | 1 + tests/qemuxml2argvdata/aarch64-gic-default-v3.args | 1 + tests/qemuxml2argvdata/aarch64-gic-default-v3.xml | 1 + tests/qemuxml2argvtest.c | 6 +++--- tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml | 1 + tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml | 1 + tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml | 1 + tests/qemuxml2xmltest.c | 6 +++--- 11 files changed, 15 insertions(+), 6 deletions(-) create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-both.args create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-both.xml create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v2.args create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v2.xml create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v3.args create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v3.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-both.args b/tests/qemuxml2argvdata/aarch64-gic-default-both.args new file mode 120000 index 000000000..04ecd4ce7 --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-gic-default-both.args @@ -0,0 +1 @@ +aarch64-gic-v2.args \ No newline at end of file diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-both.xml b/tests/qemuxml2argvdata/aarch64-gic-default-both.xml new file mode 120000 index 000000000..3e2183c92 --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-gic-default-both.xml @@ -0,0 +1 @@ +aarch64-gic-default.xml \ No newline at end of file diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-v2.args b/tests/qemuxml2argvdata/aarch64-gic-default-v2.args new file mode 120000 index 000000000..04ecd4ce7 --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-gic-default-v2.args @@ -0,0 +1 @@ +aarch64-gic-v2.args \ No newline at end of file diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-v2.xml b/tests/qemuxml2argvdata/aarch64-gic-default-v2.xml new file mode 120000 index 000000000..3e2183c92 --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-gic-default-v2.xml @@ -0,0 +1 @@ +aarch64-gic-default.xml \ No newline at end of file diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-v3.args b/tests/qemuxml2argvdata/aarch64-gic-default-v3.args new file mode 120000 index 000000000..04ecd4ce7 --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-gic-default-v3.args @@ -0,0 +1 @@ +aarch64-gic-v2.args \ No newline at end of file diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-v3.xml b/tests/qemuxml2argvdata/aarch64-gic-default-v3.xml new file mode 120000 index 000000000..3e2183c92 --- /dev/null +++ b/tests/qemuxml2argvdata/aarch64-gic-default-v3.xml @@ -0,0 +1 @@ +aarch64-gic-default.xml \ No newline at end of file diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b7afb6980..dd64772e2 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2602,13 +2602,13 @@ mymain(void) DO_TEST_GIC("aarch64-gic-default", GIC_NONE, QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACH_VIRT_GIC_VERSION); - DO_TEST_GIC("aarch64-gic-default", GIC_V2, + DO_TEST_GIC("aarch64-gic-default-v2", GIC_V2, QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACH_VIRT_GIC_VERSION); - DO_TEST_GIC("aarch64-gic-default", GIC_V3, + DO_TEST_GIC("aarch64-gic-default-v3", GIC_V3, QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACH_VIRT_GIC_VERSION); - DO_TEST_GIC("aarch64-gic-default", GIC_BOTH, + DO_TEST_GIC("aarch64-gic-default-both", GIC_BOTH, QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACH_VIRT_GIC_VERSION); DO_TEST_GIC("aarch64-gic-v2", GIC_NONE, diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml b/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml new file mode 120000 index 000000000..ee470fb1f --- /dev/null +++ b/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/aarch64-gic-v2.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml b/tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml new file mode 120000 index 000000000..ee470fb1f --- /dev/null +++ b/tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/aarch64-gic-v2.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml b/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml new file mode 120000 index 000000000..ee470fb1f --- /dev/null +++ b/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/aarch64-gic-v2.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index d3544a1ef..83809574c 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1265,9 +1265,9 @@ mymain(void) DO_TEST_FULL("aarch64-gic-none-both", WHEN_BOTH, GIC_BOTH, NONE); DO_TEST_FULL("aarch64-gic-none-tcg", WHEN_BOTH, GIC_BOTH, NONE); DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_NONE, NONE); - DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_V2, NONE); - DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_V3, NONE); - DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_BOTH, NONE); + DO_TEST_FULL("aarch64-gic-default-v2", WHEN_BOTH, GIC_V2, NONE); + DO_TEST_FULL("aarch64-gic-default-v3", WHEN_BOTH, GIC_V3, NONE); + DO_TEST_FULL("aarch64-gic-default-both", WHEN_BOTH, GIC_BOTH, NONE); DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_NONE, NONE); DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V2, NONE); DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V3, NONE); -- 2.14.3
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
Account for the fact that the default might change based on what GIC versions are supported by QEMU. That's not the case at the moment, but it will be soon.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/qemuxml2argvdata/aarch64-gic-default-both.args | 1 + tests/qemuxml2argvdata/aarch64-gic-default-both.xml | 1 + tests/qemuxml2argvdata/aarch64-gic-default-v2.args | 1 + tests/qemuxml2argvdata/aarch64-gic-default-v2.xml | 1 + tests/qemuxml2argvdata/aarch64-gic-default-v3.args | 1 + tests/qemuxml2argvdata/aarch64-gic-default-v3.xml | 1 + tests/qemuxml2argvtest.c | 6 +++--- tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml | 1 + tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml | 1 + tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml | 1 + tests/qemuxml2xmltest.c | 6 +++--- 11 files changed, 15 insertions(+), 6 deletions(-) create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-both.args create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-both.xml create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v2.args create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v2.xml create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v3.args create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v3.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml
Reviewed-by: John Ferlan <jferlan@redhat.com> John
When no GIC version is specified, we currently default to GIC v2; however, that's not a great default, since guests will fail to start if the hardware only supports GIC v3. Change the behavior so that a sensible default is chosen instead. That basically means using the same algorithm whether the user didn't explicitly enable the GIC feature or they explicitly enabled it but didn't specify any GIC version. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 25 +++++++++++----------- .../qemuxml2argvdata/aarch64-gic-default-both.args | 2 +- tests/qemuxml2argvdata/aarch64-gic-default-v3.args | 2 +- .../aarch64-gic-default-both.xml | 2 +- .../qemuxml2xmloutdata/aarch64-gic-default-v3.xml | 2 +- 5 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 98cba8789..ee720d328 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2838,13 +2838,14 @@ static void qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { - virGICVersion version; - - /* The virt machine type always uses GIC: if the relevant element + /* The virt machine type always uses GIC: if the relevant information * was not included in the domain XML, we need to choose a suitable * GIC version ourselves */ - if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT && - qemuDomainIsVirt(def)) { + if ((def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT && + qemuDomainIsVirt(def)) || + (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON && + def->gic_version == VIR_GIC_VERSION_NONE)) { + virGICVersion version; VIR_DEBUG("Looking for usable GIC version in domain capabilities"); for (version = VIR_GIC_VERSION_LAST - 1; @@ -2878,17 +2879,17 @@ qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def, } } + /* Use the default GIC version (GICv2) as a last-ditch attempt + * if no match could be found above */ + if (def->gic_version == VIR_GIC_VERSION_NONE) { + VIR_DEBUG("Using GIC version 2 (default)"); + def->gic_version = VIR_GIC_VERSION_2; + } + /* Even if we haven't found a usable GIC version in the domain * capabilities, we still want to enable this */ def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON; } - - /* Use the default GIC version (GICv2) if no version was specified */ - if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON && - def->gic_version == VIR_GIC_VERSION_NONE) { - VIR_DEBUG("Using GIC version 2 (default)"); - def->gic_version = VIR_GIC_VERSION_2; - } } diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-both.args b/tests/qemuxml2argvdata/aarch64-gic-default-both.args index 04ecd4ce7..6209eff4b 120000 --- a/tests/qemuxml2argvdata/aarch64-gic-default-both.args +++ b/tests/qemuxml2argvdata/aarch64-gic-default-both.args @@ -1 +1 @@ -aarch64-gic-v2.args \ No newline at end of file +aarch64-gic-v3.args \ No newline at end of file diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-v3.args b/tests/qemuxml2argvdata/aarch64-gic-default-v3.args index 04ecd4ce7..6209eff4b 120000 --- a/tests/qemuxml2argvdata/aarch64-gic-default-v3.args +++ b/tests/qemuxml2argvdata/aarch64-gic-default-v3.args @@ -1 +1 @@ -aarch64-gic-v2.args \ No newline at end of file +aarch64-gic-v3.args \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml b/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml index ee470fb1f..bf9d58c38 120000 --- a/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml +++ b/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml @@ -1 +1 @@ -../qemuxml2argvdata/aarch64-gic-v2.xml \ No newline at end of file +../qemuxml2argvdata/aarch64-gic-v3.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml b/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml index ee470fb1f..bf9d58c38 120000 --- a/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml +++ b/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml @@ -1 +1 @@ -../qemuxml2argvdata/aarch64-gic-v2.xml \ No newline at end of file +../qemuxml2argvdata/aarch64-gic-v3.xml \ No newline at end of file -- 2.14.3
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
When no GIC version is specified, we currently default to GIC v2; however, that's not a great default, since guests will fail to start if the hardware only supports GIC v3.
Change the behavior so that a sensible default is chosen instead. That basically means using the same algorithm whether the user didn't explicitly enable the GIC feature or they explicitly enabled it but didn't specify any GIC version.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 25 +++++++++++----------- .../qemuxml2argvdata/aarch64-gic-default-both.args | 2 +- tests/qemuxml2argvdata/aarch64-gic-default-v3.args | 2 +- .../aarch64-gic-default-both.xml | 2 +- .../qemuxml2xmloutdata/aarch64-gic-default-v3.xml | 2 +- 5 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 98cba8789..ee720d328 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2838,13 +2838,14 @@ static void qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { - virGICVersion version; - - /* The virt machine type always uses GIC: if the relevant element + /* The virt machine type always uses GIC: if the relevant information * was not included in the domain XML, we need to choose a suitable * GIC version ourselves */ - if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT && - qemuDomainIsVirt(def)) { + if ((def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT && + qemuDomainIsVirt(def)) || + (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
And patch3 (e.g. verification) takes care of us if this is "on", but !qemuDomainIsVirt(def) ends up being true... It's a tangled web.
+ def->gic_version == VIR_GIC_VERSION_NONE)) { + virGICVersion version;
VIR_DEBUG("Looking for usable GIC version in domain capabilities"); for (version = VIR_GIC_VERSION_LAST - 1; @@ -2878,17 +2879,17 @@ qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def, } }
FWIW: There's a hunk above here which notes something about bz 1414081... If one goes and reads that bz, one finds it's closed notabug. In any case, there's a lot of verbiage there.. Can any of it be shaved? What's being done seems reasonable, but if we can take the opportunity to revisit the comment as well - that'd be good. For what's here as long as the above comment doesn't cause you to have an oh sh* moment... Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ /* Use the default GIC version (GICv2) as a last-ditch attempt + * if no match could be found above */ + if (def->gic_version == VIR_GIC_VERSION_NONE) { + VIR_DEBUG("Using GIC version 2 (default)"); + def->gic_version = VIR_GIC_VERSION_2; + } + /* Even if we haven't found a usable GIC version in the domain * capabilities, we still want to enable this */ def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON; } - - /* Use the default GIC version (GICv2) if no version was specified */ - if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON && - def->gic_version == VIR_GIC_VERSION_NONE) { - VIR_DEBUG("Using GIC version 2 (default)"); - def->gic_version = VIR_GIC_VERSION_2; - } }
[...]
On Mon, 2018-02-12 at 16:05 -0500, John Ferlan wrote:
qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { - virGICVersion version; - - /* The virt machine type always uses GIC: if the relevant element + /* The virt machine type always uses GIC: if the relevant information * was not included in the domain XML, we need to choose a suitable * GIC version ourselves */ - if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT && - qemuDomainIsVirt(def)) { + if ((def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT && + qemuDomainIsVirt(def)) || + (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
And patch3 (e.g. verification) takes care of us if this is "on", but !qemuDomainIsVirt(def) ends up being true... It's a tangled web.
That's correct.
+ def->gic_version == VIR_GIC_VERSION_NONE)) { + virGICVersion version;
VIR_DEBUG("Looking for usable GIC version in domain capabilities"); for (version = VIR_GIC_VERSION_LAST - 1; @@ -2878,17 +2879,17 @@ qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def, } }
FWIW: There's a hunk above here which notes something about bz 1414081... If one goes and reads that bz, one finds it's closed notabug.
In any case, there's a lot of verbiage there.. Can any of it be shaved?
Not a lot, I don't think... Maybe We'll want to revisit this once MSI support for GICv3 has been implemented in QEMU. but that's about it, everything else still applies. -- Andrea Bolognani / Red Hat / Virtualization
Instead of storing separately whether the feature is enabled or not and what driver should be used, store both of them in a single place. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 29 +++++++++++++---------------- src/conf/domain_conf.h | 11 ++++++----- src/qemu/qemu_command.c | 5 +++-- src/qemu/qemu_domain.c | 2 +- 4 files changed, 23 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 170c56665..19884ec13 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -900,6 +900,7 @@ VIR_ENUM_IMPL(virDomainLoader, VIR_ENUM_IMPL(virDomainIOAPIC, VIR_DOMAIN_IOAPIC_LAST, + "none", "qemu", "kvm") @@ -5913,8 +5914,7 @@ virDomainDefValidateInternal(const virDomainDef *def) if (def->iommu && def->iommu->intremap == VIR_TRISTATE_SWITCH_ON && - (def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_TRISTATE_SWITCH_ON || - def->ioapic != VIR_DOMAIN_IOAPIC_QEMU)) { + def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_QEMU) { virReportError(VIR_ERR_XML_ERROR, "%s", _("IOMMU interrupt remapping requires split I/O APIC " "(ioapic driver='qemu')")); @@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml, tmp = virXMLPropString(nodes[i], "driver"); if (tmp) { int value = virDomainIOAPICTypeFromString(tmp); - if (value < 0) { + if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown driver mode: %s"), tmp); goto error; } - def->ioapic = value; - def->features[val] = VIR_TRISTATE_SWITCH_ON; + def->features[val] = value; VIR_FREE(tmp); } break; @@ -21408,16 +21407,13 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, break; case VIR_DOMAIN_FEATURE_IOAPIC: - if (src->features[i] != dst->features[i] || - src->ioapic != dst->ioapic) { + if (src->features[i] != dst->features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of feature '%s:%s' differs: " - "source: '%s:%s', destination: '%s:%s'"), + "source: '%s', destination: '%s'"), featureName, "driver", - virTristateSwitchTypeToString(src->features[i]), - virDomainIOAPICTypeToString(src->ioapic), - virTristateSwitchTypeToString(dst->features[i]), - virDomainIOAPICTypeToString(dst->ioapic)); + virDomainIOAPICTypeToString(src->features[i]), + virDomainIOAPICTypeToString(dst->features[i])); return false; } break; @@ -26943,10 +26939,11 @@ virDomainDefFormatInternal(virDomainDefPtr def, break; case VIR_DOMAIN_FEATURE_IOAPIC: - if (def->features[i] == VIR_TRISTATE_SWITCH_ON) { - virBufferAsprintf(buf, "<ioapic driver='%s'/>\n", - virDomainIOAPICTypeToString(def->ioapic)); - } + if (def->features[i] == VIR_DOMAIN_IOAPIC_NONE) + break; + + virBufferAsprintf(buf, "<ioapic driver='%s'/>\n", + virDomainIOAPICTypeToString(def->features[i])); break; case VIR_DOMAIN_FEATURE_HPT: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 21e004515..20f0efc36 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1861,7 +1861,8 @@ struct _virDomainLoaderDef { void virDomainLoaderDefFree(virDomainLoaderDefPtr loader); typedef enum { - VIR_DOMAIN_IOAPIC_QEMU = 0, + VIR_DOMAIN_IOAPIC_NONE = 0, + VIR_DOMAIN_IOAPIC_QEMU, VIR_DOMAIN_IOAPIC_KVM, VIR_DOMAIN_IOAPIC_LAST @@ -2352,9 +2353,10 @@ struct _virDomainDef { virDomainOSDef os; char *emulator; - /* These three options are of type virTristateSwitch, - * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type - * virDomainCapabilitiesPolicy */ + /* Most of the values in {kvm_,hyperv_,}features are of type + * virTristateSwitch, but there are exceptions: for example, + * VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy, + * VIR_DOMAIN_FEATURE_IOAPIC is of type virDomainIOAPIC and so on */ int features[VIR_DOMAIN_FEATURE_LAST]; int apic_eoi; int hyperv_features[VIR_DOMAIN_HYPERV_LAST]; @@ -2362,7 +2364,6 @@ struct _virDomainDef { unsigned int hyperv_spinlocks; virGICVersion gic_version; char *hyperv_vendor_id; - virDomainIOAPIC ioapic; virDomainHPTResizing hpt_resizing; /* These options are of type virTristateSwitch: ON = keep, OFF = drop */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 529079be0..3aabdf7a2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7240,14 +7240,14 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, } } - if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] == VIR_TRISTATE_SWITCH_ON) { + if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_NONE) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("I/O APIC tuning is not supported by this " "QEMU binary")); goto cleanup; } - switch (def->ioapic) { + switch ((virDomainIOAPIC) def->features[VIR_DOMAIN_FEATURE_IOAPIC]) { case VIR_DOMAIN_IOAPIC_QEMU: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -7260,6 +7260,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, case VIR_DOMAIN_IOAPIC_KVM: virBufferAddLit(&buf, ",kernel_irqchip=on"); break; + case VIR_DOMAIN_IOAPIC_NONE: case VIR_DOMAIN_IOAPIC_LAST: break; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ee720d328..cfea1f500 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3234,7 +3234,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def) switch ((virDomainFeature) i) { case VIR_DOMAIN_FEATURE_IOAPIC: - if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + if (def->features[i] != VIR_DOMAIN_IOAPIC_NONE && !ARCH_IS_X86(def->os.arch)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("The '%s' feature is only supported for %s guests"), -- 2.14.3
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
Instead of storing separately whether the feature is enabled or not and what driver should be used, store both of them in a single place.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 29 +++++++++++++---------------- src/conf/domain_conf.h | 11 ++++++----- src/qemu/qemu_command.c | 5 +++-- src/qemu/qemu_domain.c | 2 +- 4 files changed, 23 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 170c56665..19884ec13 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -900,6 +900,7 @@ VIR_ENUM_IMPL(virDomainLoader,
VIR_ENUM_IMPL(virDomainIOAPIC, VIR_DOMAIN_IOAPIC_LAST, + "none", "qemu", "kvm")
@@ -5913,8 +5914,7 @@ virDomainDefValidateInternal(const virDomainDef *def)
if (def->iommu && def->iommu->intremap == VIR_TRISTATE_SWITCH_ON && - (def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_TRISTATE_SWITCH_ON || - def->ioapic != VIR_DOMAIN_IOAPIC_QEMU)) { + def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_QEMU) { virReportError(VIR_ERR_XML_ERROR, "%s", _("IOMMU interrupt remapping requires split I/O APIC " "(ioapic driver='qemu')")); @@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml, tmp = virXMLPropString(nodes[i], "driver"); if (tmp) { int value = virDomainIOAPICTypeFromString(tmp); - if (value < 0) { + if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {
"none" wouldn't be legal XML according to "iopic" in domaincommon.rng. I think this is where things get tricky - I'm fine with the changes as is, but that interaction between domain_conf xml parsing and the rng schema gets me wondering about whether that "none" value should be in the rng. Perhaps there's another opinion on this that may want to pipe in now...
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown driver mode: %s"), tmp); goto error; } - def->ioapic = value; - def->features[val] = VIR_TRISTATE_SWITCH_ON; + def->features[val] = value; VIR_FREE(tmp); } break; @@ -21408,16 +21407,13 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, break;
case VIR_DOMAIN_FEATURE_IOAPIC: - if (src->features[i] != dst->features[i] || - src->ioapic != dst->ioapic) { + if (src->features[i] != dst->features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of feature '%s:%s' differs: " - "source: '%s:%s', destination: '%s:%s'"), + "source: '%s', destination: '%s'"), featureName, "driver", - virTristateSwitchTypeToString(src->features[i]), - virDomainIOAPICTypeToString(src->ioapic), - virTristateSwitchTypeToString(dst->features[i]), - virDomainIOAPICTypeToString(dst->ioapic)); + virDomainIOAPICTypeToString(src->features[i]), + virDomainIOAPICTypeToString(dst->features[i])); return false; } break; @@ -26943,10 +26939,11 @@ virDomainDefFormatInternal(virDomainDefPtr def, break;
case VIR_DOMAIN_FEATURE_IOAPIC: - if (def->features[i] == VIR_TRISTATE_SWITCH_ON) { - virBufferAsprintf(buf, "<ioapic driver='%s'/>\n", - virDomainIOAPICTypeToString(def->ioapic)); - } + if (def->features[i] == VIR_DOMAIN_IOAPIC_NONE) + break; + + virBufferAsprintf(buf, "<ioapic driver='%s'/>\n", + virDomainIOAPICTypeToString(def->features[i])); break;
case VIR_DOMAIN_FEATURE_HPT: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 21e004515..20f0efc36 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1861,7 +1861,8 @@ struct _virDomainLoaderDef { void virDomainLoaderDefFree(virDomainLoaderDefPtr loader);
typedef enum { - VIR_DOMAIN_IOAPIC_QEMU = 0, + VIR_DOMAIN_IOAPIC_NONE = 0, + VIR_DOMAIN_IOAPIC_QEMU, VIR_DOMAIN_IOAPIC_KVM,
VIR_DOMAIN_IOAPIC_LAST @@ -2352,9 +2353,10 @@ struct _virDomainDef {
virDomainOSDef os; char *emulator; - /* These three options are of type virTristateSwitch, - * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type - * virDomainCapabilitiesPolicy */ + /* Most of the values in {kvm_,hyperv_,}features are of type
{kvm_|hyperv_|caps_}features
+ * virTristateSwitch, but there are exceptions: for example, + * VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy,
s/,//
+ * VIR_DOMAIN_FEATURE_IOAPIC is of type virDomainIOAPIC and so on */
s/and so on/./ Reviewed-by: John Ferlan <jferlan@redhat.com> John
int features[VIR_DOMAIN_FEATURE_LAST]; int apic_eoi; int hyperv_features[VIR_DOMAIN_HYPERV_LAST]; @@ -2362,7 +2364,6 @@ struct _virDomainDef { unsigned int hyperv_spinlocks; virGICVersion gic_version; char *hyperv_vendor_id; - virDomainIOAPIC ioapic; virDomainHPTResizing hpt_resizing;
/* These options are of type virTristateSwitch: ON = keep, OFF = drop */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 529079be0..3aabdf7a2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7240,14 +7240,14 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, } }
- if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] == VIR_TRISTATE_SWITCH_ON) { + if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_NONE) { if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("I/O APIC tuning is not supported by this " "QEMU binary")); goto cleanup; } - switch (def->ioapic) { + switch ((virDomainIOAPIC) def->features[VIR_DOMAIN_FEATURE_IOAPIC]) { case VIR_DOMAIN_IOAPIC_QEMU: if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -7260,6 +7260,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, case VIR_DOMAIN_IOAPIC_KVM: virBufferAddLit(&buf, ",kernel_irqchip=on"); break; + case VIR_DOMAIN_IOAPIC_NONE: case VIR_DOMAIN_IOAPIC_LAST: break; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ee720d328..cfea1f500 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3234,7 +3234,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
switch ((virDomainFeature) i) { case VIR_DOMAIN_FEATURE_IOAPIC: - if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + if (def->features[i] != VIR_DOMAIN_IOAPIC_NONE && !ARCH_IS_X86(def->os.arch)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("The '%s' feature is only supported for %s guests"),
On Mon, 2018-02-12 at 16:59 -0500, John Ferlan wrote:
@@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml, tmp = virXMLPropString(nodes[i], "driver"); if (tmp) { int value = virDomainIOAPICTypeFromString(tmp); - if (value < 0) { + if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {
"none" wouldn't be legal XML according to "iopic" in domaincommon.rng. I think this is where things get tricky - I'm fine with the changes as is, but that interaction between domain_conf xml parsing and the rng schema gets me wondering about whether that "none" value should be in the rng. Perhaps there's another opinion on this that may want to pipe in now...
"none" should definitely *not* be in the schema, since it's not a valid value. The problem here is that schema compliance is not enforced by libvirt: when using virsh, you are given the option to simply ignore schema validation failures and go ahead with defining/modifying the guest, so the only way to actually make sure invalid values don't make it into the virDomainDef is to do something like the above. I mean, it's not like accepting "none" in the parser would be a big issue - it would just be ignored anyway. But by adding an explicit check we can report a better error in the very unlikely scenario where <ioapic driver="none"/> has been specified in the guest configuration, so I think it's a good idea to have it there given how little effort it requires.
@@ -2352,9 +2353,10 @@ struct _virDomainDef {
virDomainOSDef os; char *emulator; - /* These three options are of type virTristateSwitch, - * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type - * virDomainCapabilitiesPolicy */ + /* Most of the values in {kvm_,hyperv_,}features are of type
{kvm_|hyperv_|caps_}features
+ * virTristateSwitch, but there are exceptions: for example, + * VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy,
s/,//
+ * VIR_DOMAIN_FEATURE_IOAPIC is of type virDomainIOAPIC and so on */
s/and so on/./
I see you ask for the comment to be updated in the next commit when _HPT is changed as well, but it wasn't really my intention to list all types there - we both know how that kind of comment can get out of sync with reality very quickly :) Another approach, which didn't make it to the list for some reason, was to end the comment with [...] and so on. See virDomainDefFeaturesCheckABIStability() for more details. That seems like a better way to handle the ever-changing nature of libvirt than a comment, don't you think? -- Andrea Bolognani / Red Hat / Virtualization
On 02/13/2018 04:40 AM, Andrea Bolognani wrote:
On Mon, 2018-02-12 at 16:59 -0500, John Ferlan wrote:
@@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml, tmp = virXMLPropString(nodes[i], "driver"); if (tmp) { int value = virDomainIOAPICTypeFromString(tmp); - if (value < 0) { + if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {
"none" wouldn't be legal XML according to "iopic" in domaincommon.rng. I think this is where things get tricky - I'm fine with the changes as is, but that interaction between domain_conf xml parsing and the rng schema gets me wondering about whether that "none" value should be in the rng. Perhaps there's another opinion on this that may want to pipe in now...
"none" should definitely *not* be in the schema, since it's not a valid value. The problem here is that schema compliance is not enforced by libvirt: when using virsh, you are given the option to simply ignore schema validation failures and go ahead with defining/modifying the guest, so the only way to actually make sure invalid values don't make it into the virDomainDef is to do something like the above.
I mean, it's not like accepting "none" in the parser would be a big issue - it would just be ignored anyway. But by adding an explicit check we can report a better error in the very unlikely scenario where
<ioapic driver="none"/>
has been specified in the guest configuration, so I think it's a good idea to have it there given how little effort it requires.
@@ -2352,9 +2353,10 @@ struct _virDomainDef {
virDomainOSDef os; char *emulator; - /* These three options are of type virTristateSwitch, - * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type - * virDomainCapabilitiesPolicy */ + /* Most of the values in {kvm_,hyperv_,}features are of type
{kvm_|hyperv_|caps_}features
+ * virTristateSwitch, but there are exceptions: for example, + * VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy,
s/,//
+ * VIR_DOMAIN_FEATURE_IOAPIC is of type virDomainIOAPIC and so on */
s/and so on/./
I see you ask for the comment to be updated in the next commit when _HPT is changed as well, but it wasn't really my intention to list all types there - we both know how that kind of comment can get out of sync with reality very quickly :)
Another approach, which didn't make it to the list for some reason, was to end the comment with
[...] and so on. See virDomainDefFeaturesCheckABIStability() for more details.
That seems like a better way to handle the ever-changing nature of libvirt than a comment, don't you think?
That's fine... I guess since you started listing them I figured adding another in the next patch was "natural". How about this (or something close to it): "Most {hyperv_|kvm_|cpu_}feature options utilize a virTristateSwitch to handle support. A few assign specific data values to the option. See virDomainDefFeaturesCheckABIStability for details." John
On Tue, 2018-02-13 at 07:22 -0500, John Ferlan wrote:
Another approach, which didn't make it to the list for some reason, was to end the comment with
[...] and so on. See virDomainDefFeaturesCheckABIStability() for more details.
That seems like a better way to handle the ever-changing nature of libvirt than a comment, don't you think?
That's fine... I guess since you started listing them I figured adding another in the next patch was "natural".
How about this (or something close to it):
"Most {hyperv_|kvm_|cpu_}feature options utilize a virTristateSwitch to handle support. A few assign specific data values to the option. See virDomainDefFeaturesCheckABIStability for details."
I've changed it to match your suggestion. I've also addressed all other review comments as described in my previous replies. Do you have any objections to those? Should I send out a v2, or should I just go ahead and push the series given the minor nature of the adjustments? -- Andrea Bolognani / Red Hat / Virtualization
On 02/13/2018 08:46 AM, Andrea Bolognani wrote:
On Tue, 2018-02-13 at 07:22 -0500, John Ferlan wrote:
Another approach, which didn't make it to the list for some reason, was to end the comment with
[...] and so on. See virDomainDefFeaturesCheckABIStability() for more details.
That seems like a better way to handle the ever-changing nature of libvirt than a comment, don't you think?
That's fine... I guess since you started listing them I figured adding another in the next patch was "natural".
How about this (or something close to it):
"Most {hyperv_|kvm_|cpu_}feature options utilize a virTristateSwitch to handle support. A few assign specific data values to the option. See virDomainDefFeaturesCheckABIStability for details."
I've changed it to match your suggestion.
I've also addressed all other review comments as described in my previous replies. Do you have any objections to those? Should I send out a v2, or should I just go ahead and push the series given the minor nature of the adjustments?
I see no need for a v2 since the adjustments were essentially minor. Consider some of those comments a replacement for you (or I) being able to walk across the big pond in order to toss random thoughts or ideas that spring to mind when I'm reading patches. Many times it's just what I'm thinking as I'm reading. If it gives you an aha moment to describe or do something different, then great. If it's utter nonesense, then no big deal either ;-). John
On Tue, 2018-02-13 at 09:34 -0500, John Ferlan wrote:
On 02/13/2018 08:46 AM, Andrea Bolognani wrote:
[...] Should I send out a v2, or should I just go ahead and push the series given the minor nature of the adjustments?
I see no need for a v2 since the adjustments were essentially minor. Consider some of those comments a replacement for you (or I) being able to walk across the big pond in order to toss random thoughts or ideas that spring to mind when I'm reading patches. Many times it's just what I'm thinking as I'm reading. If it gives you an aha moment to describe or do something different, then great. If it's utter nonesense, then no big deal either ;-).
Cool, I had assumed as much but wanted to make sure ;) I've now pushed the series, thanks for your review! -- Andrea Bolognani / Red Hat / Virtualization
Instead of storing separately whether the feature is enabled or not and what resizing policy should be used, store both of them in a single place. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 26 ++++++++++++-------------- src/conf/domain_conf.h | 4 ++-- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 2 +- 4 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 19884ec13..c1d549594 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -906,6 +906,7 @@ VIR_ENUM_IMPL(virDomainIOAPIC, VIR_ENUM_IMPL(virDomainHPTResizing, VIR_DOMAIN_HPT_RESIZING_LAST, + "none", "enabled", "disabled", "required", @@ -19239,14 +19240,13 @@ virDomainDefParseXML(xmlDocPtr xml, tmp = virXMLPropString(nodes[i], "resizing"); if (tmp) { int value = virDomainHPTResizingTypeFromString(tmp); - if (value < 0) { + if (value < 0 || value == VIR_DOMAIN_HPT_RESIZING_NONE) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown HPT resizing setting: %s"), tmp); goto error; } - def->hpt_resizing = value; - def->features[val] = VIR_TRISTATE_SWITCH_ON; + def->features[val] = value; VIR_FREE(tmp); } break; @@ -21377,16 +21377,13 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, break; case VIR_DOMAIN_FEATURE_HPT: - if (src->features[i] != dst->features[i] || - src->hpt_resizing != dst->hpt_resizing) { + if (src->features[i] != dst->features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of feature '%s:%s' differs: " - "source: '%s:%s', destination: '%s:%s'"), + "source: '%s', destination: '%s'"), featureName, "resizing", - virTristateSwitchTypeToString(src->features[i]), - virDomainHPTResizingTypeToString(src->hpt_resizing), - virTristateSwitchTypeToString(dst->features[i]), - virDomainHPTResizingTypeToString(dst->hpt_resizing)); + virDomainHPTResizingTypeToString(src->features[i]), + virDomainHPTResizingTypeToString(dst->features[i])); return false; } break; @@ -26947,10 +26944,11 @@ virDomainDefFormatInternal(virDomainDefPtr def, break; case VIR_DOMAIN_FEATURE_HPT: - if (def->features[i] == VIR_TRISTATE_SWITCH_ON) { - virBufferAsprintf(buf, "<hpt resizing='%s'/>\n", - virDomainHPTResizingTypeToString(def->hpt_resizing)); - } + if (def->features[i] == VIR_DOMAIN_HPT_RESIZING_NONE) + break; + + virBufferAsprintf(buf, "<hpt resizing='%s'/>\n", + virDomainHPTResizingTypeToString(def->features[i])); break; /* coverity[dead_error_begin] */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 20f0efc36..4e9044ae6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1871,7 +1871,8 @@ typedef enum { VIR_ENUM_DECL(virDomainIOAPIC); typedef enum { - VIR_DOMAIN_HPT_RESIZING_ENABLED = 0, + VIR_DOMAIN_HPT_RESIZING_NONE = 0, + VIR_DOMAIN_HPT_RESIZING_ENABLED, VIR_DOMAIN_HPT_RESIZING_DISABLED, VIR_DOMAIN_HPT_RESIZING_REQUIRED, @@ -2364,7 +2365,6 @@ struct _virDomainDef { unsigned int hyperv_spinlocks; virGICVersion gic_version; char *hyperv_vendor_id; - virDomainHPTResizing hpt_resizing; /* These options are of type virTristateSwitch: ON = keep, OFF = drop */ int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST]; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3aabdf7a2..faf09a599 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7266,7 +7266,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, } } - if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON) { + if (def->features[VIR_DOMAIN_FEATURE_HPT] != VIR_DOMAIN_HPT_RESIZING_NONE) { const char *str; if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT)) { @@ -7276,7 +7276,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd, goto cleanup; } - str = virDomainHPTResizingTypeToString(def->hpt_resizing); + str = virDomainHPTResizingTypeToString(def->features[VIR_DOMAIN_FEATURE_HPT]); if (!str) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Invalid setting for HPT resizing")); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index cfea1f500..5d21d3703 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3244,7 +3244,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def) break; case VIR_DOMAIN_FEATURE_HPT: - if (def->features[i] == VIR_TRISTATE_SWITCH_ON && + if (def->features[i] != VIR_DOMAIN_HPT_RESIZING_NONE && !qemuDomainIsPSeries(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("The '%s' feature is only supported for %s guests"), -- 2.14.3
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
Instead of storing separately whether the feature is enabled or not and what resizing policy should be used, store both of them in a single place.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_conf.c | 26 ++++++++++++-------------- src/conf/domain_conf.h | 4 ++-- src/qemu/qemu_command.c | 4 ++-- src/qemu/qemu_domain.c | 2 +- 4 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 19884ec13..c1d549594 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -906,6 +906,7 @@ VIR_ENUM_IMPL(virDomainIOAPIC,
VIR_ENUM_IMPL(virDomainHPTResizing, VIR_DOMAIN_HPT_RESIZING_LAST, + "none", "enabled", "disabled", "required", @@ -19239,14 +19240,13 @@ virDomainDefParseXML(xmlDocPtr xml, tmp = virXMLPropString(nodes[i], "resizing"); if (tmp) { int value = virDomainHPTResizingTypeFromString(tmp); - if (value < 0) { + if (value < 0 || value == VIR_DOMAIN_HPT_RESIZING_NONE) {
Similar note as w/ previous and rng schema. Here too I'm fine with this way, but if someone else isn't hopefully they speak up.
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unknown HPT resizing setting: %s"), tmp); goto error; } - def->hpt_resizing = value; - def->features[val] = VIR_TRISTATE_SWITCH_ON; + def->features[val] = value; VIR_FREE(tmp); } break; @@ -21377,16 +21377,13 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, break;
case VIR_DOMAIN_FEATURE_HPT: - if (src->features[i] != dst->features[i] || - src->hpt_resizing != dst->hpt_resizing) { + if (src->features[i] != dst->features[i]) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("State of feature '%s:%s' differs: " - "source: '%s:%s', destination: '%s:%s'"), + "source: '%s', destination: '%s'"), featureName, "resizing", - virTristateSwitchTypeToString(src->features[i]), - virDomainHPTResizingTypeToString(src->hpt_resizing), - virTristateSwitchTypeToString(dst->features[i]), - virDomainHPTResizingTypeToString(dst->hpt_resizing)); + virDomainHPTResizingTypeToString(src->features[i]), + virDomainHPTResizingTypeToString(dst->features[i])); return false; } break; @@ -26947,10 +26944,11 @@ virDomainDefFormatInternal(virDomainDefPtr def, break;
case VIR_DOMAIN_FEATURE_HPT: - if (def->features[i] == VIR_TRISTATE_SWITCH_ON) { - virBufferAsprintf(buf, "<hpt resizing='%s'/>\n", - virDomainHPTResizingTypeToString(def->hpt_resizing)); - } + if (def->features[i] == VIR_DOMAIN_HPT_RESIZING_NONE) + break; + + virBufferAsprintf(buf, "<hpt resizing='%s'/>\n", + virDomainHPTResizingTypeToString(def->features[i])); break;
/* coverity[dead_error_begin] */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 20f0efc36..4e9044ae6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1871,7 +1871,8 @@ typedef enum { VIR_ENUM_DECL(virDomainIOAPIC);
typedef enum { - VIR_DOMAIN_HPT_RESIZING_ENABLED = 0, + VIR_DOMAIN_HPT_RESIZING_NONE = 0, + VIR_DOMAIN_HPT_RESIZING_ENABLED, VIR_DOMAIN_HPT_RESIZING_DISABLED, VIR_DOMAIN_HPT_RESIZING_REQUIRED,
@@ -2364,7 +2365,6 @@ struct _virDomainDef {
The comment a few lines above here should be updated to include: * VIR_DOMAIN_FEATURE_HPT is of type virDomainHPTResizing Reviewed-by: John Ferlan <jferlan@redhat.com> John
unsigned int hyperv_spinlocks; virGICVersion gic_version; char *hyperv_vendor_id; - virDomainHPTResizing hpt_resizing;
/* These options are of type virTristateSwitch: ON = keep, OFF = drop */ int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST];
[...]
Give them better names and remove some redundancy. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...es-hpt-resizing.args => pseries-features-hpt.args} | 1 - .../pseries-features-hpt.xml} | 0 ...chine.xml => pseries-features-invalid-machine.xml} | 2 +- tests/qemuxml2argvdata/pseries-hpt-resizing.xml | 19 ------------------- tests/qemuxml2argvtest.c | 11 +++-------- tests/qemuxml2xmloutdata/pseries-features-hpt.xml | 1 + tests/qemuxml2xmltest.c | 3 +-- 7 files changed, 6 insertions(+), 31 deletions(-) rename tests/qemuxml2argvdata/{pseries-hpt-resizing.args => pseries-features-hpt.args} (96%) rename tests/{qemuxml2xmloutdata/pseries-hpt-resizing.xml => qemuxml2argvdata/pseries-features-hpt.xml} (100%) rename tests/qemuxml2argvdata/{pseries-hpt-resizing-invalid-machine.xml => pseries-features-invalid-machine.xml} (86%) delete mode 100644 tests/qemuxml2argvdata/pseries-hpt-resizing.xml create mode 120000 tests/qemuxml2xmloutdata/pseries-features-hpt.xml diff --git a/tests/qemuxml2argvdata/pseries-hpt-resizing.args b/tests/qemuxml2argvdata/pseries-features-hpt.args similarity index 96% rename from tests/qemuxml2argvdata/pseries-hpt-resizing.args rename to tests/qemuxml2argvdata/pseries-features-hpt.args index 994789a5e..8cdb32965 100644 --- a/tests/qemuxml2argvdata/pseries-hpt-resizing.args +++ b/tests/qemuxml2argvdata/pseries-features-hpt.args @@ -12,7 +12,6 @@ QEMU_AUDIO_DRV=none \ -smp 1,sockets=1,cores=1,threads=1 \ -uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \ -nographic \ --nodefconfig \ -nodefaults \ -chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\ server,nowait \ diff --git a/tests/qemuxml2xmloutdata/pseries-hpt-resizing.xml b/tests/qemuxml2argvdata/pseries-features-hpt.xml similarity index 100% rename from tests/qemuxml2xmloutdata/pseries-hpt-resizing.xml rename to tests/qemuxml2argvdata/pseries-features-hpt.xml diff --git a/tests/qemuxml2argvdata/pseries-hpt-resizing-invalid-machine.xml b/tests/qemuxml2argvdata/pseries-features-invalid-machine.xml similarity index 86% rename from tests/qemuxml2argvdata/pseries-hpt-resizing-invalid-machine.xml rename to tests/qemuxml2argvdata/pseries-features-invalid-machine.xml index 757fcc70e..5a6bb02d5 100644 --- a/tests/qemuxml2argvdata/pseries-hpt-resizing-invalid-machine.xml +++ b/tests/qemuxml2argvdata/pseries-features-invalid-machine.xml @@ -7,7 +7,7 @@ <type arch='x86_64' machine='pc'>hvm</type> </os> <features> - <!-- HPT resizing can't be enabled for non-pSeries guests --> + <!-- pSeries features can't be enabled for non-pSeries guests --> <hpt resizing='enabled'/> </features> <devices> diff --git a/tests/qemuxml2argvdata/pseries-hpt-resizing.xml b/tests/qemuxml2argvdata/pseries-hpt-resizing.xml deleted file mode 100644 index f9dc9cac9..000000000 --- a/tests/qemuxml2argvdata/pseries-hpt-resizing.xml +++ /dev/null @@ -1,19 +0,0 @@ -<domain type='qemu'> - <name>guest</name> - <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> - <memory unit='KiB'>524288</memory> - <vcpu placement='static'>1</vcpu> - <os> - <type arch='ppc64' machine='pseries'>hvm</type> - </os> - <features> - <!-- Explicitly enable HPT resizing. The guest will not start - at all unless HPT resizing can be arranged --> - <hpt resizing='required'/> - </features> - <devices> - <emulator>/usr/bin/qemu-system-ppc64</emulator> - <controller type='usb' model='none'/> - <memballoon model='none'/> - </devices> -</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index dd64772e2..3aa69fcee 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1891,17 +1891,12 @@ mymain(void) QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VFIO_PCI); - DO_TEST("pseries-hpt-resizing", - QEMU_CAPS_NODEFCONFIG, + DO_TEST("pseries-features-hpt", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT); - DO_TEST_FAILURE("pseries-hpt-resizing", - QEMU_CAPS_NODEFCONFIG, + DO_TEST_FAILURE("pseries-features-hpt", QEMU_CAPS_MACHINE_OPT); - DO_TEST_PARSE_ERROR("pseries-hpt-resizing-invalid-machine", - QEMU_CAPS_NODEFCONFIG, - QEMU_CAPS_MACHINE_OPT, - QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT); + DO_TEST_PARSE_ERROR("pseries-features-invalid-machine", NONE); DO_TEST("pseries-serial-native", QEMU_CAPS_NODEFCONFIG, diff --git a/tests/qemuxml2xmloutdata/pseries-features-hpt.xml b/tests/qemuxml2xmloutdata/pseries-features-hpt.xml new file mode 120000 index 000000000..bcaf2e6fe --- /dev/null +++ b/tests/qemuxml2xmloutdata/pseries-features-hpt.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/pseries-features-hpt.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 83809574c..0eb9e6c77 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -763,8 +763,7 @@ mymain(void) QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VFIO_PCI); - DO_TEST("pseries-hpt-resizing", - QEMU_CAPS_NODEFCONFIG, + DO_TEST("pseries-features-hpt", QEMU_CAPS_MACHINE_OPT, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT); -- 2.14.3
On 02/06/2018 11:43 AM, Andrea Bolognani wrote:
Give them better names and remove some redundancy.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ...es-hpt-resizing.args => pseries-features-hpt.args} | 1 - .../pseries-features-hpt.xml} | 0 ...chine.xml => pseries-features-invalid-machine.xml} | 2 +- tests/qemuxml2argvdata/pseries-hpt-resizing.xml | 19 ------------------- tests/qemuxml2argvtest.c | 11 +++-------- tests/qemuxml2xmloutdata/pseries-features-hpt.xml | 1 + tests/qemuxml2xmltest.c | 3 +-- 7 files changed, 6 insertions(+), 31 deletions(-) rename tests/qemuxml2argvdata/{pseries-hpt-resizing.args => pseries-features-hpt.args} (96%) rename tests/{qemuxml2xmloutdata/pseries-hpt-resizing.xml => qemuxml2argvdata/pseries-features-hpt.xml} (100%) rename tests/qemuxml2argvdata/{pseries-hpt-resizing-invalid-machine.xml => pseries-features-invalid-machine.xml} (86%) delete mode 100644 tests/qemuxml2argvdata/pseries-hpt-resizing.xml create mode 120000 tests/qemuxml2xmloutdata/pseries-features-hpt.xml
Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (2)
-
Andrea Bolognani -
John Ferlan