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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux