On 10/18/21 12:31 AM, Ani Sinha wrote:
Error messages must conform to spec as specified here:
https://www.libvirt.org/coding-style.html#error-message-format
This change encloses format specifiers in quotes and unbreaks error messages.
Fixes: 8eadf82fb5 ("conf: introduce option to enable/disable pci hotplug on pci-root
controller")
Fixes: 7300ccc9b3 ("conf: introduce support for acpi-bridge-hotplug feature")
Signed-off-by: Ani Sinha <a...@anisinha.ca>
Hi Ani,
Thanks for following up on your contributions even after they've been
pushed :-)
The parts of this patch that are relevent to the pci-root hotplug look
fine and can be pushed.
But the QEMU people have discovered problems with the
"acpi-pci-hotplug-with-bridge-support" switch used by libvirt's
<acpi-bridge-hotplug> switch that is forcing them to rethink not just
their implementation/design.
Basically the way that the QEMU switch works (setting it off will enable
native and disable ACPI hotplug, while setting it on will disable native
and enable ACPI), while making logical sense from the outside, is
"confusing" some guests - that's as much detail as I have, but it means
that most likely that QEMU switch will be scrapped and one or more new
(and possibly different) switches will be added in their place; it all
seems to be up in the air right now. Anyway, since we don't want to have
code in libvirt for a QEMU switch that didn't work correctly in the
beginning and was then replaced, and since it's highly likely that the
new QEMU hotplug-selection method will work differently (and anyway
isn't known now), our best course of action seems to be reverting all
the acpi-bridge-hotplug patches for now - this is still possible since
there hasn't been an official release since they were added.
I've already made the revert patches (for the original 4 of the feature,
plus 6 patches from pkrempa that fixed up test cases and such) and will
be posting them in the morning with a (hopefully) slightly better
explanation.
I hope you don't find this too discouraging - just keep in mind that the
reason for reverting isn't your code, but rather is because the QEMU
feature your code is using turns out to not work as advertised, and if
the libvirt code that is using it makes it into a release, then we will
have to support it essentially forever.
(NB: it's also completely possible (but looks to be very unlikely) that
QEMU will figure out a way to solve their problems without modifying
this switch, and we'll be able to simply re-push the reverted commits;
we can't be sure of that right now though).
More in the morning...
---
src/conf/domain_conf.c | 6 ++----
src/qemu/qemu_validate.c | 6 +++---
.../pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err | 2 +-
.../pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err | 2 +-
4 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6fcf86ba58..d5de07d13d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17557,8 +17557,7 @@ virDomainFeaturesPCIDefParse(virDomainDef *def,
feature = virDomainPCITypeFromString((const char *)node->name);
if (feature < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unsupported PCI feature: %s"),
- node->name);
+ _("unsupported PCI feature: '%s'"), node->name);
return -1;
}
@@ -21833,8 +21832,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDef *src,
case VIR_DOMAIN_PCI_ACPI_BRIDGE_HOTPLUG:
if (src->pci_features[i] != dst->pci_features[i]) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("State of PCI feature '%s' differs: "
- "source: '%s', destination: '%s'"),
+ _("State of PCI feature '%s' differs: source:
'%s', destination: '%s'"),
virDomainPCITypeToString(i),
virTristateSwitchTypeToString(src->pci_features[i]),
virTristateSwitchTypeToString(dst->pci_features[i]));
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 3045e4b64b..f93b697265 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3947,7 +3947,7 @@ qemuValidateDomainDeviceDefControllerPCI(const
virDomainControllerDef *cont,
case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
if (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_PIIX4_ACPI_ROOT_PCI_HOTPLUG)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("setting the %s property on a '%s' device is not
supported by this QEMU binary"),
+ _("setting the '%s' property on a '%s' device is not
supported by this QEMU binary"),
"hotplug", "pci-root");
return -1;
}
@@ -3956,8 +3956,8 @@ qemuValidateDomainDeviceDefControllerPCI(const
virDomainControllerDef *cont,
case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("setting the hotplug property on a '%s' device is
not supported by this QEMU binary"),
- modelName);
+ _("setting the '%s' property on a '%s' device is not
supported by this QEMU binary"),
+ "hotplug", modelName);
return -1;
}
break;
diff --git
a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err
b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err
index b507f1f8bc..55ec41c476 100644
---
a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err
+++
b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.x86_64-5.1.0.err
@@ -1 +1 @@
-unsupported configuration: setting the hotplug property on a 'pci-root' device
is not supported by this QEMU binary
+unsupported configuration: setting the 'hotplug' property on a 'pci-root'
device is not supported by this QEMU binary
diff --git
a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err
b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err
index b507f1f8bc..55ec41c476 100644
--- a/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err
+++ b/tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.x86_64-5.1.0.err
@@ -1 +1 @@
-unsupported configuration: setting the hotplug property on a 'pci-root' device
is not supported by this QEMU binary
+unsupported configuration: setting the 'hotplug' property on a 'pci-root'
device is not supported by this QEMU binary
