On Fri, 2015-12-18 at 11:12 -0500, Laine Stump wrote:
>
> >> + 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()...
Okay, will squash that in before pushing.
> >> + 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 :-)
Yes, he meant compile time :)
> > 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.
Both me and Michal thought the compiler would be smart enough to realize
that the code above doesn't make much sense and emit at the very least a
warning. Of course it's not going to be able to do anything if the value
is read at runtime, but in this case it would be right in the code and
still it would compile just fine.
Maybe other tools like coverity can detect this kind of mistake and
point it out?
Cheers.
--
Andrea Bolognani
Software Engineer - Virtualization Team
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list