[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]