Re: [libvirt] [PATCH v2 3/6] pci: Introduce virPCIStubDriver enumeration

  >> +    if (!(drvname = virPCIStubDriverTypeToString(driver)) || >> +        driver == VIR_PCI_STUB_DRIVER_NONE) {   You could save a small bit of time by comparing to  VIR_PCI_STUB_DRIVER_NONE first, and then calling ...TypeToString()...

>> +        virReportError(VIR_ERR_INTERNAL_ERROR, >> +                       "%s", >> +                       _("Attempting to use unknown stub driver")); >> +        return -1; >> +    } > Hm. Interesting, I thought that checking for TypeToString(driver) would > be useless, since we are passing an enum into the function. But > apparently I was wrong since the following code does not throw any error > whatsoever: > > virPCIProbeStubDriver(20);   You mean no error at compile time I guess (I was a bit confused at  first, trying to see how virPCIStubDriverTypeToString could possibly not  return NULL for an out of range enum value :-)

> So I guess it's a good idea to have it there. Also, this brings up an > interesting question - should we check for other TypeToString() return > values too? e.g. like we do in virCPUDefFormatBuf().   There is inconsistency in this throughout the code. I think older  *Format functions tend to not check the return, as they are assuming  that the data being formatted was previously parsed (and the parse would  only have put a proper enum in there), but newer code is more likely to  be paranoid and check the return value. I admit to having done both,  depending on just how paranoid I'm feeling at the time - not checking  the return makes the code cleaner and there is a *very* low probability  that the data will have come from somewhere other than the output of the  parser; on the other hand, this is not always the case, and adding in  the checks for NULL assures that we catch a bug in the code sooner  rather than later. Definitely if you're calling it with data that didn't  come directly out of a parser, then you really really should be checking  the return for NULL.