Re: [libvirt] [PATCH v2 01/42] conf: add enum constants for default controller models

2018-02-20 Thread Daniel P . Berrangé
On Tue, Feb 20, 2018 at 10:38:41AM +0100, Ján Tomko wrote:
> On Thu, Feb 15, 2018 at 04:43:06PM +, Daniel P. Berrangé wrote:
> > 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 fallthough from the
> 
> s/fallthough/fallthrough/
> 
> > 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é 
> > ---
> > src/conf/domain_addr.c |  5 +
> > src/conf/domain_conf.c |  1 +
> > src/conf/domain_conf.h |  4 
> > src/qemu/qemu_command.c| 17 ++---
> > src/qemu/qemu_domain.c | 10 +-
> > src/qemu/qemu_domain_address.c | 22 ++
> > src/vbox/vbox_common.c | 13 +
> > 7 files changed, 64 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> > index 6422682391..df19c6be1a 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,9 @@ 
> > virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus,
> > bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST;
> > break;
> > 
> > +case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
> > +break;
> > +
> 
> Unlike '-usb' for USB controllers, there is no -pci for pci controllers
> - we need to know the model at the time of building the command line.
> 
> Not having one set here is either an error in the user input or the
> part of our code that should have filled the model in. If we want to be
> robust, this should be grouped with the next case.

Yes, I've checked test and latter it fallthrough to _LAST works too

> 
> > case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> >"%s", _("PCI controller model was not set 
> > correctly"));
> > @@ -1746,6 +1750,7 @@ 
> > virDomainUSBAddressControllerModelToPorts(virDomainControllerDefPtr cont)
> > return cont->opts.usbopts.ports;
> > return 8;
> > 
> > +case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT:
> > case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
> > case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
> > break;
> 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 6c73cd7bfe..2a75a169c2 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 178ec24ae7..e114f5dfcf 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -4098,11 +4098,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;
> 
> [...]
> 
> > diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> > index e28119e4b1..de565dbf74 100644
> > --- a/src/qemu/qemu_domain_address.c
> > +++ b/src/qemu/qemu_domain_address.c
> > @@ -513,6 +513,16 @@ 
> > qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
> > 
> > case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> > switch ((virDomainControllerModelUSB) cont->model) {
> > +case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT:
> > +/* XXX it isn't clear that this is the right
> > + * thing to do here. We probably ought to
> > +

Re: [libvirt] [PATCH v2 01/42] conf: add enum constants for default controller models

2018-02-20 Thread Daniel P . Berrangé
On Mon, Feb 19, 2018 at 01:33:23PM -0500, John Ferlan wrote:
> 
> 
> On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> > 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 fallthough 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é 
> > ---
> >  src/conf/domain_addr.c |  5 +
> >  src/conf/domain_conf.c |  1 +
> >  src/conf/domain_conf.h |  4 
> >  src/qemu/qemu_command.c| 17 ++---
> >  src/qemu/qemu_domain.c | 10 +-
> >  src/qemu/qemu_domain_address.c | 22 ++
> >  src/vbox/vbox_common.c | 13 +
> >  7 files changed, 64 insertions(+), 8 deletions(-)
> > 
> 
> There's a few places where in the code where model is compared against
> -1 that could perhaps use the VIR_DOMAIN_CONTROLLER_MODEL_*_DEFAULT (I
> used 'model.*=.*-1' in the Find this egrep pattern in cscope).
> 
> At least one of them (below) caused a Coverity complaint.
> 
> So should any of those be altered or should we just live with the fact
> that using/knowing -1 is the 'default' model?

Probably best to use the constant rather than literal -1.

> > diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> > index 6422682391..df19c6be1a 100644
> > --- a/src/conf/domain_addr.c
> > +++ b/src/conf/domain_addr.c
> 
> [...]
> 
> > @@ -1746,6 +1750,7 @@ 
> > virDomainUSBAddressControllerModelToPorts(virDomainControllerDefPtr cont)
> >  return cont->opts.usbopts.ports;
> >  return 8;
> >  
> > +case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT:
> 
> Coverity points out that because at the top of this function, we have:
> 
> if (model == -1)
> model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
> 
> this particular condition cannot be met - IOW: DEADCODE
> 
> Since the code isn't changing cont->model - we could just remove the
> @model local and move the DEFAULT into the return 2 leaving some sort of
> comment (perhaps) that DEFAULT == PIIX3_UHCI?

I'll just move the DEFAULT: case to be a fallthrough to the PIIX3_UHCI
case instead, and delete that if (...).

> > @@ -540,6 +550,16 @@ 
> > qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
> >  
> >  case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> >  switch ((virDomainControllerModelSCSI) cont->model) {
> > +case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT:
> > +/* XXX it isn't clear that this is the right
> > + * thing to do here. We probably ought to
> > + * return 0, but that breaks test suite right
> > + * now because we call this method before we
> > + * have turned the _DEFAULT case into a
> > + * specific choice
> > + */
> > +return pciFlags;
> > +
> 
> I think it was Laine that asked at one time about re-ordering things -
> /me taking a brief look caused my head to spin ;-) and I think this is a
> fine alternative (at least for now) until someone gets the gumption to
> attempt to fix it.

Laine suggested changing code so that it assigned USB addresses before
assigning PCI addresses, but that does not work. We unfortunately don't
assign default controller models until much later.

> 
> >  case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
> >  return virtioFlags;
> 
> [...]
> 
> For what's here w/ the issue Coverity noted handled...  Adding setting
> the model to *_DEFAULT is an add on if it's done...

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 01/42] conf: add enum constants for default controller models

2018-02-20 Thread Ján Tomko

On Thu, Feb 15, 2018 at 04:43:06PM +, Daniel P. Berrangé wrote:

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 fallthough from the


s/fallthough/fallthrough/


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é 
---
src/conf/domain_addr.c |  5 +
src/conf/domain_conf.c |  1 +
src/conf/domain_conf.h |  4 
src/qemu/qemu_command.c| 17 ++---
src/qemu/qemu_domain.c | 10 +-
src/qemu/qemu_domain_address.c | 22 ++
src/vbox/vbox_common.c | 13 +
7 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 6422682391..df19c6be1a 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,9 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr 
bus,
bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST;
break;

+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
+break;
+


Unlike '-usb' for USB controllers, there is no -pci for pci controllers
- we need to know the model at the time of building the command line.

Not having one set here is either an error in the user input or the
part of our code that should have filled the model in. If we want to be
robust, this should be grouped with the next case.


case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR,
   "%s", _("PCI controller model was not set correctly"));
@@ -1746,6 +1750,7 @@ 
virDomainUSBAddressControllerModelToPorts(virDomainControllerDefPtr cont)
return cont->opts.usbopts.ports;
return 8;

+case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT:
case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
break;



diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6c73cd7bfe..2a75a169c2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -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(, ",id=%s", def->info.alias);
break;
@@ -2771,9 +2777,14 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
virBufferAsprintf(, ",numa_node=%d", pciopts->numaNode);
break;
case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Unsupported PCI-E 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;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 178ec24ae7..e114f5dfcf 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4098,11 +4098,16 @@ qemuDomainCheckSCSIControllerModel(virQEMUCapsPtr 
qemuCaps,
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
case 

Re: [libvirt] [PATCH v2 01/42] conf: add enum constants for default controller models

2018-02-19 Thread John Ferlan


On 02/15/2018 11:43 AM, Daniel P. Berrangé wrote:
> 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 fallthough 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é 
> ---
>  src/conf/domain_addr.c |  5 +
>  src/conf/domain_conf.c |  1 +
>  src/conf/domain_conf.h |  4 
>  src/qemu/qemu_command.c| 17 ++---
>  src/qemu/qemu_domain.c | 10 +-
>  src/qemu/qemu_domain_address.c | 22 ++
>  src/vbox/vbox_common.c | 13 +
>  7 files changed, 64 insertions(+), 8 deletions(-)
> 

There's a few places where in the code where model is compared against
-1 that could perhaps use the VIR_DOMAIN_CONTROLLER_MODEL_*_DEFAULT (I
used 'model.*=.*-1' in the Find this egrep pattern in cscope).

At least one of them (below) caused a Coverity complaint.

So should any of those be altered or should we just live with the fact
that using/knowing -1 is the 'default' model?

> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 6422682391..df19c6be1a 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c

[...]

> @@ -1746,6 +1750,7 @@ 
> virDomainUSBAddressControllerModelToPorts(virDomainControllerDefPtr cont)
>  return cont->opts.usbopts.ports;
>  return 8;
>  
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT:

Coverity points out that because at the top of this function, we have:

if (model == -1)
model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;

this particular condition cannot be met - IOW: DEADCODE

Since the code isn't changing cont->model - we could just remove the
@model local and move the DEFAULT into the return 2 leaving some sort of
comment (perhaps) that DEFAULT == PIIX3_UHCI?

>  case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
>  case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
>  break;

[...]

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6c73cd7bfe..2a75a169c2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c

[...]

> @@ -2771,9 +2777,14 @@ qemuBuildControllerDevStr(const virDomainDef 
> *domainDef,
>  virBufferAsprintf(, ",numa_node=%d", pciopts->numaNode);
>  break;
>  case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("Unsupported PCI-E root controller"));

PCI-Express

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

[...]

> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index e28119e4b1..de565dbf74 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -513,6 +513,16 @@ 
> qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>  
>  case VIR_DOMAIN_CONTROLLER_TYPE_USB:
>  switch ((virDomainControllerModelUSB) cont->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT:
> +/* XXX it isn't clear that this is the right
> + * thing to do here. We probably ought to
> + * return 0, but that breaks test suite right
> + * now because we call this method before we
> + * have turned the _DEFAULT case into a
> + * specific choice
> + */
> +return pciFlags;
> +
>  case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
>  case VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI:
>  return pcieFlags;
> @@ -540,6 +550,16 @@ 
> qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
>  
>  case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
>  switch ((virDomainControllerModelSCSI) cont->model) {
> +case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_DEFAULT:
> +/* XXX it isn't clear that this is the right
> + * thing to do here. We probably 

[libvirt] [PATCH v2 01/42] conf: add enum constants for default controller models

2018-02-15 Thread Daniel P . Berrangé
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 fallthough 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é 
---
 src/conf/domain_addr.c |  5 +
 src/conf/domain_conf.c |  1 +
 src/conf/domain_conf.h |  4 
 src/qemu/qemu_command.c| 17 ++---
 src/qemu/qemu_domain.c | 10 +-
 src/qemu/qemu_domain_address.c | 22 ++
 src/vbox/vbox_common.c | 13 +
 7 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 6422682391..df19c6be1a 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,9 @@ virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr 
bus,
 bus->maxSlot = VIR_PCI_ADDRESS_SLOT_LAST;
 break;
 
+case VIR_DOMAIN_CONTROLLER_MODEL_PCI_DEFAULT:
+break;
+
 case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
 virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("PCI controller model was not set correctly"));
@@ -1746,6 +1750,7 @@ 
virDomainUSBAddressControllerModelToPorts(virDomainControllerDefPtr cont)
 return cont->opts.usbopts.ports;
 return 8;
 
+case VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT:
 case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
 case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
 break;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fb732a0c2a..abc3332377 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10196,6 +10196,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 6cd81ef2de..78b57aa6d4 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/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6c73cd7bfe..2a75a169c2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -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:
+