Re: [PATCH] acpi/hotplug: Fix error message format to conform to spec

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 <ani(a)anisinha.ca&gt; 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.

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 >