[PATCH v5 2/4] conf: introduce option to enable/disable pci hotplug on pci-root controller

[Ani Sinha]

On Mon, 27 Sep 2021, Laine Stump wrote:

> On 9/27/21 4:05 AM, Ján Tomko wrote:
> > On a Monday in 2021, Ani Sinha wrote:
> > > This change introduces libvirt xml support to enable/disable hotplug on
> > > the
> > > pci-root controller. It adds a 'target' subelement for the pci-root
> > > controller
> > > with a 'hotplug' property. This property can be used to enable or disable
> > > hotplug for the pci-root controller. For example, in order to disable
> > > hotplug
> > > on the pci-root controller, one has to use set '<target hotplug='off'>' as
> > > shown below:
> > >
> > > <controller type='pci' model='pci-root'>
> > >  <target hotplug='off'/>
> > > </controller>
> > >
> > > '<target hotplug='on'>' option would enable hotplug for pci-root
> > > controller.
> > > This is also the default value. This option is only available for pc
> > > machine
> > > types and is applicable for qemu only/kvm accelerator onlt.This feature
> > > was
> >
> > s/onlt./only. /
> >
> > > introduced from qemu version 5.2 with the following change in qemu
> > > repository:
> > >
> > > 3d7e78aa7777f ("Introduce a new flag for i440fx to disable PCI hotplug on
> > > the root bus")
> > >
> > > The above qemu commit describes some reasons why users might to disable
> > > hotplug
> > > on PCI root buses.
> > >
> > > Related unit tests to exercise the new conf option has also been added.
> > >
> > > Signed-off-by: Ani Sinha <ani at anisinha.ca>
> > > ---
> > > docs/formatdomain.rst                         | 12 ++++----
> > > src/qemu/qemu_validate.c                      |  9 +++++-
> > > .../pc-i440fx-acpi-root-hotplug-disable.xml   | 30 +++++++++++++++++++
> > > .../pc-i440fx-acpi-root-hotplug-enable.xml    | 30 +++++++++++++++++++
> > > .../pc-i440fx-acpi-root-hotplug-disable.xml   |  1 +
> > > .../pc-i440fx-acpi-root-hotplug-enable.xml    |  1 +
> > > tests/qemuxml2xmltest.c                       |  4 +++
> > > 7 files changed, 81 insertions(+), 6 deletions(-)
> > > create mode 100644
> > > tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-disable.xml
> > > create mode 100644
> > > tests/qemuxml2argvdata/pc-i440fx-acpi-root-hotplug-enable.xml
> > > create mode 120000
> > > tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-disable.xml
> > > create mode 120000
> > > tests/qemuxml2xmloutdata/pc-i440fx-acpi-root-hotplug-enable.xml
> > >
> > > @@ -3879,6 +3879,14 @@ qemuValidateDomainDeviceDefControllerPCI(const
> > > virDomainControllerDef *cont,
> > >     /* hotplug */
> > >     if (pciopts->hotplug != VIR_TRISTATE_SWITCH_ABSENT) {
> > >         switch ((virDomainControllerModelPCI) cont->model) {
> > > +        case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> > > +            if (!virQEMUCapsGet(qemuCaps,
> > > QEMU_CAPS_PIIX_ACPI_ROOT_PCI_HOTPLUG)) {
> > > +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > > +                               _("setting the %s property on a pci-root
> > > device is not supported by this QEMU binary"),
> > > +                               "hotplug");
> >
> > No need to create a new translatable string, the one used by the case
> > below can be reused:
> >
> >                  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> >                                 _("setting the hotplug property on a '%s'
> > device is not supported by this QEMU binary"),
>
> Wouldn't it be better if "hotplug" was also put in the arg list rather than
> inline in the function? That would avoid some overzealous translator
> attempting to translate that word. (Or maybe just use the function
> virReportControllerInvalidOption() for both of these error cases, thus
> removing both strings)

virReportControllerInvalidOption() would be confusing/incorrect. "option
foo is not valid for PCI controller ..." etc is confusing. Better to
be more explicit and say that the option is not supported by Qemu binary
that is being used (maybe an upgrade will help?).