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

Laine Stump laine at laine.org
Thu Oct 21 02:15:22 UTC 2021


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 at 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
> 




More information about the libvir-list mailing list