[libvirt] [PATCH 2/6] qemu: Make switch statements more strict

Andrea Bolognani posted 6 patches 4 years, 3 months ago

[libvirt] [PATCH 2/6] qemu: Make switch statements more strict

Posted by Andrea Bolognani 4 years, 3 months ago
When switching over the values in the virDomainControllerModelPCI
enumeration, make sure the proper cast is in place so that the
compiler can warn us when the coverage is not exaustive.

For the same reason, fold some unstructured checks (performed by
comparing directly against some values in the enumeration) inside
an existing switch statement.
---
 src/qemu/qemu_command.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 552fdcf..f0b938f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2664,19 +2664,13 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
         break;
 
     case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
-        if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
-            def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
-            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                           _("wrong function called for pci-root/pcie-root"));
-            return NULL;
-        }
         if (def->idx == 0) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                            _("index for pci controllers of model '%s' must be > 0"),
                            virDomainControllerModelPCITypeToString(def->model));
             goto error;
         }
-        switch (def->model) {
+        switch ((virDomainControllerModelPCI) def->model) {
         case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
             if (def->opts.pciopts.modelName
                 == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE ||
@@ -2917,6 +2911,12 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
                virBufferAsprintf(&buf, ",numa_node=%d",
                                  def->opts.pciopts.numaNode);
             break;
+        case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
+        case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
+        case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("wrong function called"));
+            goto error;
         }
         break;
 
@@ -6501,7 +6501,7 @@ qemuBuildGlobalControllerCommandLine(virCommandPtr cmd,
             bool cap = false;
             bool machine = false;
 
-            switch (cont->model) {
+            switch ((virDomainControllerModelPCI) cont->model) {
             case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
                 hoststr = "i440FX-pcihost";
                 cap = virQEMUCapsGet(qemuCaps, QEMU_CAPS_I440FX_PCI_HOLE64_SIZE);
-- 
2.7.4

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

Re: [libvirt] [PATCH 2/6] qemu: Make switch statements more strict

Posted by Laine Stump 4 years, 3 months ago
On 02/21/2017 02:57 PM, Andrea Bolognani wrote:
> When switching over the values in the virDomainControllerModelPCI
> enumeration, make sure the proper cast is in place so that the
> compiler can warn us when the coverage is not exaustive.
>
> For the same reason, fold some unstructured checks (performed by
> comparing directly against some values in the enumeration) inside
> an existing switch statement.
> ---
>   src/qemu/qemu_command.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 552fdcf..f0b938f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2664,19 +2664,13 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
>           break;
>   
>       case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> -        if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
> -            def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                           _("wrong function called for pci-root/pcie-root"));
> -            return NULL;
> -        }

It makes sense that the above code would never happen (certainly one of 
the two current callers to qemuBuildControllerDevStr() guarantees that 
it won't happen by skipping the call in that case), but how much do you 
want to trues the caller.
>           if (def->idx == 0) {
>               virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                              _("index for pci controllers of model '%s' must be > 0"),
>                              virDomainControllerModelPCITypeToString(def->model));
>               goto error;
>           }
> -        switch (def->model) {
> +        switch ((virDomainControllerModelPCI) def->model) {
>           case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE:
>               if (def->opts.pciopts.modelName
>                   == VIR_DOMAIN_CONTROLLER_PCI_MODEL_NAME_NONE ||
> @@ -2917,6 +2911,12 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
>                  virBufferAsprintf(&buf, ",numa_node=%d",
>                                    def->opts.pciopts.numaNode);
>               break;
> +        case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> +        case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> +        case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("wrong function called"));

Actually it will probably never get to here, because the code above 
where you removed this check also checks to be sure that def->idx != 0 
(and for root controllers it always does, unless the user has manually 
altered it). So instead of getting a "wrong function called" log, you 
would probably get the incorrect "index for pci controllers of model 
"pci[e]-root" must be > 0".

You can solve this by putting the "if (def->idx == 0)" check down just 
below the "switch (def->model)".

ACK with that change.

> +            goto error;
>           }
>           break;
>   
> @@ -6501,7 +6501,7 @@ qemuBuildGlobalControllerCommandLine(virCommandPtr cmd,
>               bool cap = false;
>               bool machine = false;
>   
> -            switch (cont->model) {
> +            switch ((virDomainControllerModelPCI) cont->model) {
>               case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
>                   hoststr = "i440FX-pcihost";
>                   cap = virQEMUCapsGet(qemuCaps, QEMU_CAPS_I440FX_PCI_HOLE64_SIZE);

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

Re: [libvirt] [PATCH 2/6] qemu: Make switch statements more strict

Posted by Andrea Bolognani 4 years, 3 months ago
On Tue, 2017-02-21 at 15:26 -0500, Laine Stump wrote:
> > @@ -2664,19 +2664,13 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
> >           break;
> >   
> >       case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> > -        if (def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT ||
> > -            def->model == VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) {
> > -            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > -                           _("wrong function called for pci-root/pcie-root"));
> > -            return NULL;
> > -        }
> 
> It makes sense that the above code would never happen (certainly one of 
> the two current callers to qemuBuildControllerDevStr() guarantees that 
> it won't happen by skipping the call in that case), but how much do you 
> want to trues the caller.

Not at all! That's why I didn't drop the check but merely
moved it ;)

> > @@ -2917,6 +2911,12 @@ qemuBuildControllerDevStr(const virDomainDef *domainDef,
> >                  virBufferAsprintf(&buf, ",numa_node=%d",
> >                                    def->opts.pciopts.numaNode);
> >               break;
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT:
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT:
> > +        case VIR_DOMAIN_CONTROLLER_MODEL_PCI_LAST:
> > +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                           _("wrong function called"));
> 
> Actually it will probably never get to here, because the code above 
> where you removed this check also checks to be sure that def->idx != 0 
> (and for root controllers it always does, unless the user has manually 
> altered it). So instead of getting a "wrong function called" log, you 
> would probably get the incorrect "index for pci controllers of model 
> "pci[e]-root" must be > 0".
> 
> You can solve this by putting the "if (def->idx == 0)" check down just 
> below the "switch (def->model)".

Makes sense.

-- 
Andrea Bolognani / Red Hat / Virtualization

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