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

[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é <berrange at redhat.com>
> ---
>  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(&buf, ",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 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.

>              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...


Reviewed-by: John Ferlan <jferlan at redhat.com>

John