The controller model is slightly unusual in that the default value is -1, not 0. As a result the default value is not covered by any of the existing enum cases. This in turn means that any switch() statements that think they have covered all cases, will in fact not match the default value at all. In the qemuDomainDeviceCalculatePCIConnectFlags() method this has caused a serious mistake where we fallthrough from the SCSI controller case, to the VirtioSerial controller case, and from the USB controller case to the IDE controller case. By adding explicit enum constant starting at -1, we can ensure switches remember to handle the default case. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/conf/domain_addr.c | 6 +++--- src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 4 ++++ src/libxl/libxl_conf.c | 2 +- src/qemu/qemu_command.c | 21 ++++++++++++++++----- src/qemu/qemu_domain.c | 14 +++++++++++--- src/qemu/qemu_domain_address.c | 19 +++++++++++++++++-- src/qemu/qemu_hotplug.c | 2 +- src/vbox/vbox_common.c | 13 +++++++++---- src/vmx/vmx.c | 2 +- 10 files changed, 65 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index 6422682391..4f969adc00 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -39,6 +39,7 @@ virDomainPCIControllerModelToConnectType(virDomainControllerModelPCI model) * the equivalent VIR_PCI_CONNECT_TYPE_*. */ switch (model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: @@ -344,6 +345,7 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST; break; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("PCI controller model was not set correctly")); @@ -1710,10 +1712,8 @@ virDomainUSBAddressControllerModelToPorts(virDomainControllerDefPtr cont) { int model = cont->model; - if (model == -1) - model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI; - switch ((virDomainControllerModelUSB) model) { + case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT: case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI: case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI: case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI: diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 970efbb13c..28e4f6f681 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4999,7 +4999,7 @@ virDomainDefPostParseCommon(virDomainDefPtr def, * been added in AddImplicitDevices, after we've done the per-device * post-parse. */ for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->model == -1 && + if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT && def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { virDomainDeviceDef device = { .type = VIR_DOMAIN_DEVICE_CONTROLLER, @@ -10197,6 +10197,7 @@ virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, 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: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: /* Other controller models don't require extra checks */ break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 117efb1d1d..2350926e5b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -685,6 +685,7 @@ typedef enum { typedef enum { + VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT = -1, VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE, @@ -714,6 +715,7 @@ typedef enum { } virDomainControllerPCIModelName; typedef enum { + VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT = -1, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC, @@ -727,6 +729,7 @@ typedef enum { } virDomainControllerModelSCSI; typedef enum { + VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT = -1, VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI, VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI, VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI, @@ -746,6 +749,7 @@ typedef enum { } virDomainControllerModelUSB; typedef enum { + VIR_DOMAIN_CONTROLLER_MODEL_IDE_DEFAULT = -1, VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3, VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4, VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6, diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 970cff205b..0c5de344de 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1894,7 +1894,7 @@ libxlMakeUSBController(virDomainControllerDefPtr controller, if (controller->type != VIR_DOMAIN_CONTROLLER_TYPE_USB) return -1; - if (controller->model == -1) { + if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT) { usbctrl->version = 2; usbctrl->type = LIBXL_USBCTRL_TYPE_QUSB; } else { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0c6a6cbd15..f8526a6454 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2555,7 +2555,7 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, model = def->model; - if (model == -1) { + if (model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("no model provided for USB controller")); return -1; @@ -2671,10 +2671,16 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI: - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported controller model: %s"), virDomainControllerModelSCSITypeToString(def->model)); + goto error; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected SCSI controller model %d"), + def->model); + goto error; } virBufferAsprintf(&buf, ",id=%s", def->info.alias); break; @@ -2771,9 +2777,14 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef, virBufferAsprintf(&buf, ",numa_node=%d", pciopts->numaNode); break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Unsupported PCI Express root controller")); + goto error; + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("wrong function called")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected PCI controller model %d"), + def->model); goto error; } break; @@ -2905,7 +2916,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd, } if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && - cont->model == -1 && + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && !qemuDomainIsQ35(def) && !qemuDomainIsVirt(def)) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5d0a18343e..593b9d0fb5 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4139,11 +4139,16 @@ qemuDomainCheckSCSIControllerModel(virQEMUCapsPtr qemuCaps, case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI: - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unsupported controller model: %s"), virDomainControllerModelSCSITypeToString(model)); return false; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected SCSI controller model %d"), + model); + return false; } return true; @@ -4230,6 +4235,7 @@ qemuDomainDeviceDefValidateControllerSCSI(const virDomainControllerDef *controll case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: break; } @@ -4277,6 +4283,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: break; } @@ -4509,6 +4516,7 @@ qemuDomainDeviceDefValidateControllerPCI(const virDomainControllerDef *controlle case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: break; } @@ -4861,7 +4869,7 @@ qemuDomainControllerDefPostParse(virDomainControllerDefPtr cont, break; case VIR_DOMAIN_CONTROLLER_TYPE_USB: - if (cont->model == -1 && qemuCaps) { + if (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT && qemuCaps) { /* Pick a suitable default model for the USB controller if none * has been selected by the user and we have the qemuCaps for * figuring out which contollers are supported. @@ -5879,7 +5887,7 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, */ if (ARCH_IS_X86(def->os.arch) && qemuDomainIsI440FX(def) && usb && usb->idx == 0 && - (usb->model == -1 || + (usb->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT || usb->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI)) { VIR_DEBUG("Removing default USB controller from domain '%s'" " for migration compatibility", def->name); diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index e28119e4b1..dd3e703ca5 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -513,6 +513,15 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, case VIR_DOMAIN_CONTROLLER_TYPE_USB: switch ((virDomainControllerModelUSB) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT: + /* qemuDomainControllerDefPostParse should have + * changed 'model' to an explicit USB model in + * most cases. Since we're still on the default + * though, we must be going to use "-usb", which + * is assumed to be a PCI default + */ + return pciFlags; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI: case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI: return pcieFlags; @@ -540,6 +549,9 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: switch ((virDomainControllerModelSCSI) cont->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT: + return 0; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: return virtioFlags; @@ -1275,7 +1287,8 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, addr->function == 1) || (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->idx == 0 && (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI || - cont->model == -1) && addr->function == 2)) { + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT) && + addr->function == 2)) { /* Note the check for nbuses > 0 - if there are no PCI * buses, we skip this check. This is a quirk required for * some machinetypes such as s390, which pretend to have a @@ -1435,7 +1448,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->idx == 0 && (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI || - cont->model == -1)) { + cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT)) { if (virDeviceInfoPCIAddressPresent(&cont->info)) { if (cont->info.addr.pci.domain != 0 || cont->info.addr.pci.bus != 0 || @@ -2165,6 +2178,7 @@ qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont, *modelName = VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_SPAPR_PCI_HOST_BRIDGE; break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: break; } @@ -2552,6 +2566,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_SWITCH_UPSTREAM_PORT: case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT: case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST: break; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 3c87d73745..04f4f689a9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -612,7 +612,7 @@ qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver, return NULL; cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; cont->idx = controller; - if (model == -1) + if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT) cont->model = qemuDomainGetSCSIControllerModel(vm->def, cont, priv->qemuCaps); else cont->model = model; diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index aed14f02d7..07f4308784 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -410,12 +410,17 @@ vboxSetStorageController(virDomainControllerDefPtr controller, case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: - case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("The vbox driver does not support %s SCSI " "controller model"), virDomainControllerModelSCSITypeToString(controller->model)); goto cleanup; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected SCSI controller model %d"), + controller->model); + goto cleanup; } /* libvirt ide model => vbox ide model */ } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { @@ -433,10 +438,10 @@ vboxSetStorageController(virDomainControllerDefPtr controller, break; case VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST: + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_DEFAULT: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("The vbox driver does not support %s IDE " - "controller model"), - virDomainControllerModelIDETypeToString(controller->model)); + _("Unexpected IDE controller model %d"), + controller->model); goto cleanup; } } diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 7a749f93ab..1d13ab03db 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -1169,7 +1169,7 @@ virVMXHandleLegacySCSIDiskDriverName(virDomainDefPtr def, return -1; } - if (controller->model == -1) { + if (controller->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT) { controller->model = model; } else if (controller->model != model) { virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.14.3