On Wed, 20 Oct 2021, Laine Stump wrote: > 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@xxxxxxxxxxx> > > 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. > I will send out a separate patch. > 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. Hmm. sadly I am not part of the discussion and I have no visibility either. I guess I need to find a job with redhat :-) 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. Sad. That was lot of effort seems to have gone waste ... > > 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 > > > >
