Re: [libvirt] [PATCH v5 03/12] qemu: Validate PCI controller options (modelName)

Friday, 2 March 2018

On 03/02/2018 10:13 AM, Andrea Bolognani wrote:
...
 https://bugzilla.redhat.com/show_bug.cgi?id=1483816

 Signed-off-by: Andrea Bolognani <abologna(a)redhat.com&gt;
 ---
  src/qemu/qemu_domain.c | 229 +++++++++++++++++++++++++++++++------------------
  1 file changed, 145 insertions(+), 84 deletions(-)

 diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
 index a45a676520..13c2b557fb 100644
 --- a/src/qemu/qemu_domain.c
 +++ b/src/qemu/qemu_domain.c
 @@ -4274,7 +4274,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const
virDomainControllerDef *contro
  {
      virDomainControllerModelPCI model = controller->model;
      const virDomainPCIControllerOpts *pciopts;
 -    const char *modelName = NULL;

      /* skip pcie-root */
      if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
 @@ -4312,24 +4311,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const
virDomainControllerDef *contro
      }

      pciopts = &controller->opts.pciopts;
 -    if (controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT &&
 -        controller->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST) {
 -        if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
 -            virReportError(VIR_ERR_INTERNAL_ERROR,
 -                           _("autogenerated %s options not set"),
 -                          
virDomainControllerModelPCITypeToString(controller->model));
 -            return -1;
 -        }
 -
 -        modelName = virDomainControllerPCIModelNameTypeToString(pciopts->modelName);
 -        if (!modelName) {
 -            virReportError(VIR_ERR_INTERNAL_ERROR,
 -                           _("unknown %s modelName value %d"),
 -                          
virDomainControllerModelPCITypeToString(controller->model),
 -                           pciopts->modelName);
 -            return -1;
 -        }
 -    }

      /* Second pass - now the model specific checks */
      switch (model) {
 @@ -4340,14 +4321,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const
virDomainControllerDef *contro
              return -1;
          }

 -        if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) {
 -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 -                           _("PCI controller model name '%s' is not valid
"
 -                             "for a pci-bridge"),
 -                           modelName);
 -            return -1;
 -        }
 -
          if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) {
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                             _("the pci-bridge controller is not supported "
 @@ -4364,14 +4337,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const
virDomainControllerDef *contro
              return -1;
          }

 -        if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) {
 -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 -                           _("PCI controller model name '%s' is not valid
"
 -                             "for a pci-expander-bus"),
 -                           modelName);
 -            return -1;
 -        }
 -
          if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB)) {
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                             _("the pxb controller is not supported in this "
 @@ -4382,14 +4347,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const
virDomainControllerDef *contro
          break;

      case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
 -        if (pciopts->modelName !=
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) {
 -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 -                           _("PCI controller model name '%s' is not valid
"
 -                             "for a dmi-to-pci-bridge"),
 -                           modelName);
 -            return -1;
 -        }
 -
          if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE)) {
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                             _("the dmi-to-pci-bridge (i82801b11-bridge) "
 @@ -4406,15 +4363,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const
virDomainControllerDef *contro
              return -1;
          }

 -        if ((pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420)
&&
 -            (pciopts->modelName !=
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT)) {
 -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 -                           _("PCI controller model name '%s' is not valid
"
 -                             "for a pcie-root-port"),
 -                           modelName);
 -            return -1;
 -        }
 -
          if ((pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420)
&&
              !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IOH3420)) {
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
 @@ -4434,14 +4382,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const
virDomainControllerDef *contro
          break;

      case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
 -        if (pciopts->modelName !=
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM) {
 -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 -                           _("PCI controller model name '%s' is not valid
"
 -                             "for a pcie-switch-upstream-port"),
 -                           modelName);
 -            return -1;
 -        }
 -
          if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_X3130_UPSTREAM)) {
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                             _("the pcie-switch-upstream-port (x3130-upstream)
"
 @@ -4459,14 +4399,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const
virDomainControllerDef *contro
              return -1;
          }

 -        if (pciopts->modelName !=
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) {
 -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 -                           _("PCI controller model name '%s' is not valid
"
 -                             "for a pcie-switch-downstream-port"),
 -                           modelName);
 -            return -1;
 -        }
 -
          if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_XIO3130_DOWNSTREAM)) {
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                             _("The pcie-switch-downstream-port "
 @@ -4484,14 +4416,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const
virDomainControllerDef *contro
              return -1;
          }

 -        if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) {
 -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 -                           _("PCI controller model name '%s' is not valid
"
 -                             "for a pcie-expander-bus"),
 -                            modelName);
 -             return -1;
 -        }
 -
          if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PXB_PCIE)) {
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                             _("the pxb-pcie controller is not supported "
 @@ -4512,14 +4436,6 @@ qemuDomainDeviceDefValidateControllerPCIOld(const
virDomainControllerDef *contro
          if (pciopts->targetIndex == 0)
              return 0;

 -        if (pciopts->modelName !=
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
 -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 -                           _("PCI controller model name '%s' is not valid
"
 -                             "for a pci-root"),
 -                           modelName);
 -            return -1;
 -        }
 -
          if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE)) {
              virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                             _("the spapr-pci-host-bridge controller is not "
 @@ -4566,6 +4482,151 @@ qemuDomainDeviceDefValidateControllerPCI(const
virDomainControllerDef *cont,
          return -1;
      }

 +    /* modelName */
 +    switch ((virDomainControllerModelPCI) cont->model) {
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
 +    case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
 +        /* modelName should have been set automatically */
 +        if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
 +            virReportError(VIR_ERR_INTERNAL_ERROR,
 +                           _("Option '%s' not set for '%s'
controller"),
 +                           "modelName", model); 
I'm undecided if it's useful to have a special function for this (since
it's going to be used so many times), or if  it's just as efficient to
spell it out every time (since the compiler will combine all the uses of
the same literal string into a single instance of the string in memory /
in the translations).

maybe virReportControllerMissingOption(model, option) - less characters
to type, and simpler to change the wording for all occurrences if we
decide to, but maybe less informative when reading the code.).

As an example of how we might decide to change the exact message - I
think it might be better if it was "Required option '%s' not set for
'%s' controller".

Hmm. Although, I see from at least one example that there will need to
be variations of the message - in the case of targetIndex, for example,
it is only required if the pci-root controller is an
spapr-pci-host-bridge - the error in that case really should have that
info (or maybe the index of the controller would be even better).

...
 +            return -1;
 +        }
 +        break;
 +
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
 +        /* modelName must be set for pSeries guests, but it's an error
 +         * for it to be set for any other guest */
 +        if (pciopts->modelName == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE
&&
 +            qemuDomainIsPSeries(def)) {
 +            virReportError(VIR_ERR_INTERNAL_ERROR,
 +                           _("Option '%s' not set for '%s'
controller"),
 +                           "modelName", model);
 +            return -1;
 +        }
 +        if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE
&&
 +            !qemuDomainIsPSeries(def)) { 
micro-optimization - you could structure this as:

          if (qemuDomainIsPSeries(def) {
               if (pciopts->modelName == .....NONE) {
                     BigFatGreekError();
                     return -1;
               }
          } else {
               if (pciopts->modelName != ....NONE) {
                      BigFatGreekError();
                      return -1;
               }
          }

so that qemuDomainIsPSeries() was only called once.

...
 +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +                           _("Option '%s' is not valid for '%s'
controller"),
 +                           "modelName", model); 
Maybe a helper function for this error too.
(virReportInvalidControllerModelName(model, modelName)?) Again, I'm
undecided if having a separate function (actually a #defined macro so
that function/line number info would be correct in the log) would be
useful for maintenance, or just extra work for nothing.

> +            return -1;
> +        }
> +        break;
> +
> +    case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +        if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
...
 +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +                           _("Option '%s' is not valid for '%s'
controller"),
 +                           "modelName", model); > +            return
-1;
> +        }
> +        break;
> +
> +    case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> +    case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> +    default:
> +        virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
> +        return -1;
> +    }
> +
> +    /* modelName (cont'd) */
> +    switch ((virDomainControllerModelPCI) cont->model) {
> +    case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +        if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE
&&
> +            pciopts->modelName !=
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("Model name '%s' is not valid for
'%s' controller"),
> +                           modelName, "pci-root");

Why hardcode the model instead of just using the string that has already
been set with that value?

...
 +            return -1;
 +        }
 +        break;
 +
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
 +        if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCI_BRIDGE) {
 +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +                           _("Model name '%s' is not valid for '%s'
controller"),
 +                           modelName, "pci-bridge"); 
same. etc etc.

...
 +            return -1;
 +        }
 +        break;
 +
 +    case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE:
 +        if (pciopts->modelName !=
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_I82801B11_BRIDGE) {
 +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +                           _("Model name '%s' is not valid for '%s'
controller"),
 +                           modelName, "dmi-to-pci-bridge");
 +            return -1;
 +        }
 +        break;
 +
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
 +        if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_IOH3420
&&
 +            pciopts->modelName !=
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PCIE_ROOT_PORT) {
 +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +                           _("Model name '%s' is not valid for '%s'
controller"),
 +                           modelName, "pcie-root-port");
 +            return -1;
 +        }
 +        break;
 +
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT:
 +        if (pciopts->modelName !=
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_X3130_UPSTREAM) {
 +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +                           _("Model name '%s' is not valid for '%s'
controller"),
 +                           modelName, "pcie-switch-upstream-port");
 +            return -1;
 +        }
 +        break;
 +
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
 +        if (pciopts->modelName !=
VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_XIO3130_DOWNSTREAM) {
 +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +                           _("Model name '%s' is not valid for '%s'
controller"),
 +                           modelName, "pci-switch-downstream-port");
 +            return -1;
 +        }
 +        break;
 +
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
 +        if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB) {
 +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +                           _("Model name '%s' is not valid for '%s'
controller"),
 +                           modelName, "pci-expander-bus");
 +            return -1;
 +        }
 +        break;
 +
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
 +        if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_PXB_PCIE) {
 +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +                           _("Model name '%s' is not valid for '%s'
controller"),
 +                           modelName, "pcie-expander-bus");
 +             return -1;
 +        }
 +        break;
 +
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
 +        if (pciopts->modelName != VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE) {
 +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +                           _("Model name '%s' is not valid for '%s'
controller"),
 +                           modelName, "pcie-root");
 +             return -1;
 +        }
 +        break;
 +
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
 +    case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
 +    default:
 +        virReportEnumRangeError(virDomainControllerModelPCI, cont->model);
 +        return -1;
 +    }
 +
      return qemuDomainDeviceDefValidateControllerPCIOld(cont, def, qemuCaps);
  }

I'm not sure I see the advantage of grouping the validation for a single
attribute on several different controller models together, rather than
the current code's grouping validation for all attributes on a single
controller together. Potato-potahto. It does mean that if you're hand
tracing the code you need to go through the entire function rather than
just one section. On the other hand, I guess it does mean that many
instances of "attribute X isn't valid for Y controller" can be combined.

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

Re: [libvirt] [PATCH v5 03/12] qemu: Validate PCI controller options (modelName)