We will rewrite pretty much every single line of this function
over the course of the next several commits, and starting from
a clean slate rather than replacing it bit by bit makes the
resulting diffs unmeasurably easier to read and understand,
and you need fewer of them to boot. Trust me, I tried the other
approach first :)
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/qemu/qemu_domain.c | 199 +------------------------------------------------
1 file changed, 2 insertions(+), 197 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2fbae695a..9dc3d5597 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4243,204 +4243,9 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll
static int
-qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller,
- const virDomainDef *def)
+qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controller ATTRIBUTE_UNUSED,
+ const virDomainDef *def ATTRIBUTE_UNUSED)
{
- virDomainControllerModelPCI model = controller->model;
- const virDomainPCIControllerOpts *pciopts;
- const char *modelName = NULL;
-
- /* skip pcie-root */
- if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT)
- return 0;
-
- /* Skip pci-root, except for pSeries guests (which actually
- * support more than one PCI Host Bridge per guest) */
- if (!qemuDomainIsPSeries(def) &&
- controller->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT)
- return 0;
-
- /* First pass - just check the controller index for the model's
- * that we care to check... */
- switch (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:
- if (controller->idx == 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("index for pci controllers of model '%s' must be > 0"),
- virDomainControllerModelPCITypeToString(model));
- return -1;
- }
- break;
-
- case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
- case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
- case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
- break;
- }
-
- 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) {
- case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
- if (pciopts->chassisNr == -1) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("autogenerated pci-bridge options not set"));
- 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;
- }
-
- break;
-
- case VIR_DOMAIN_CONTROLLER_MODEL_PCI_EXPANDER_BUS:
- if (pciopts->busNr == -1) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("autogenerated pci-expander-bus options not set"));
- 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;
- }
-
- 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;
- }
-
- break;
-
- case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT_PORT:
- if (pciopts->chassis == -1 || pciopts->port == -1) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("autogenerated pcie-root-port options not set"));
- 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;
- }
-
- 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;
- }
-
- break;
-
- case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_DOWNSTREAM_PORT:
- if (pciopts->chassis == -1 || pciopts->port == -1) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("autogenerated pcie-switch-downstream-port "
- "options not set"));
- 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;
- }
-
- break;
-
- case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_EXPANDER_BUS:
- if (pciopts->busNr == -1) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("autogenerated pcie-expander-bus options not set"));
- 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;
- }
-
- break;
-
- case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
- if (pciopts->targetIndex == -1) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("autogenerated pci-root options not set"));
- return -1;
- }
-
- /* Skip the implicit one */
- 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;
- }
-
- break;
-
- case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
- case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
- break;
- }
-
return 0;
}
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Feb 16, 2018 at 17:28:04 +0100, Andrea Bolognani wrote: > We will rewrite pretty much every single line of this function > over the course of the next several commits, and starting from > a clean slate rather than replacing it bit by bit makes the > resulting diffs unmeasurably easier to read and understand, > and you need fewer of them to boot. Trust me, I tried the other > approach first :) Will this remove any checks during the series? If yes, then you probably should at first rename this function and add a almost-empty wrapper then add new code to the wrapper and delete the renamed function at the end. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2018-02-19 at 07:28 +0100, Peter Krempa wrote:
> On Fri, Feb 16, 2018 at 17:28:04 +0100, Andrea Bolognani wrote:
> > We will rewrite pretty much every single line of this function
> > over the course of the next several commits, and starting from
> > a clean slate rather than replacing it bit by bit makes the
> > resulting diffs unmeasurably easier to read and understand,
> > and you need fewer of them to boot. Trust me, I tried the other
> > approach first :)
>
> Will this remove any checks during the series? If yes, then you probably
> should at first rename this function and add a almost-empty wrapper then
> add new code to the wrapper and delete the renamed function at the end.
No, if anything it *adds* a bunch of checks :)
Renaming the function won't work because then the compiler will
complain about it being unused. Unless you meant something like
/* Delete once done */
ValidateControllerPCIOld() {
/* Existing checks here */
}
ValidateControllerPCI() {
/* New checks here */
/* Delete once done */
ValidateControllerPCIOld();
}
which could actually do the trick.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Feb 19, 2018 at 10:22:29 +0100, Andrea Bolognani wrote:
> On Mon, 2018-02-19 at 07:28 +0100, Peter Krempa wrote:
> > On Fri, Feb 16, 2018 at 17:28:04 +0100, Andrea Bolognani wrote:
> > > We will rewrite pretty much every single line of this function
> > > over the course of the next several commits, and starting from
> > > a clean slate rather than replacing it bit by bit makes the
> > > resulting diffs unmeasurably easier to read and understand,
> > > and you need fewer of them to boot. Trust me, I tried the other
> > > approach first :)
> >
> > Will this remove any checks during the series? If yes, then you probably
> > should at first rename this function and add a almost-empty wrapper then
> > add new code to the wrapper and delete the renamed function at the end.
>
> No, if anything it *adds* a bunch of checks :)
Well, I meant that after applying this patch a bunch of checks will
vanish until you add them in the next patches, which I don't think we
should do.
>
> Renaming the function won't work because then the compiler will
> complain about it being unused. Unless you meant something like
>
> /* Delete once done */
> ValidateControllerPCIOld() {
> /* Existing checks here */
> }
>
> ValidateControllerPCI() {
> /* New checks here */
>
> /* Delete once done */
> ValidateControllerPCIOld();
> }
>
> which could actually do the trick.
I meant this flow obviously, so that the checks are kept until
fixed/reimplemented.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2018-02-19 at 10:40 +0100, Peter Krempa wrote:
> > Renaming the function won't work because then the compiler will
> > complain about it being unused. Unless you meant something like
> >
> > /* Delete once done */
> > ValidateControllerPCIOld() {
> > /* Existing checks here */
> > }
> >
> > ValidateControllerPCI() {
> > /* New checks here */
> >
> > /* Delete once done */
> > ValidateControllerPCIOld();
> > }
> >
> > which could actually do the trick.
>
> I meant this flow obviously, so that the checks are kept until
> fixed/reimplemented.
It was not obvious to me when reading the first message :)
But I agree it's a better way to handle the situation, so I will
do it this way in v3.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.