Re: [libvirt] [PATCH v3 3/3] Honor blacklist for modprobe command

On 02/04/2014 06:06 AM, Ján Tomko wrote: > On 01/30/2014 06:50 PM, John Ferlan wrote: >> + VIR_FREE(errbuf); >> + goto cleanup; >> } >> >> goto recheck; >> } >> >> + /* If we know failure was because of blacklist, let's report that */ >> + if (virKModIsBlacklisted(driver)) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("Failed to load PCI stub module %s: " >> + "administratively prohibited"), >> + driver); >> + } >> + >> +cleanup: >> return -1; >> } >> >> @@ -1313,9 +1322,10 @@ virPCIDeviceDetach(virPCIDevicePtr dev, >> virPCIDeviceList *inactiveDevs) >> { >> if (virPCIProbeStubDriver(dev->stubDriver) < 0) { >> - virReportError(VIR_ERR_INTERNAL_ERROR, >> - _("Failed to load PCI stub module %s"), >> - dev->stubDriver); >> + if (virGetLastError() == NULL) > > This seems to be the only caller of virPCIProbeStubDriver. > You could just report the error unconditionally inside it. > Attempting to make the differentiation between load failed load failed because of administratively prohibited means an additional check or two in the caller.

Furthermore if something that virPCIProbeStubDriver() called provided some other error wouldn't it be better to not overwrite the message? If the virAsprintf() called by virPCIDriverDir() failed because of memory allocation, then which error message would be displayed without the virGetLastError() check? I guess I'm not 100% clear in my mind which error message would get displayed...