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

On 02/04/2014 10:10 AM, Ján Tomko wrote: > On 02/04/2014 03:29 PM, John Ferlan wrote: >> >> >> 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. > > I meant that right now virPCIProbeStubDriver is only called from here and if > it did not report an error, we will report one here. > > It seemed cleaner not to report an error here and make virPCIProbeStubDriver > report an error in all cases (not just when the module is blacklisted and/or > on OOM in virPCIDriverDir). > oh - ah... light dawns on marblehead. >> >> 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... > > Only the last reported error gets displayed, but both will get logged. > > Jan > > The "last" message is what was important to someone using virt-install so a "git diff master src/util/virpci.c" then has the following... Basically an adjustment cleanup: label in order to handle both error conditions and removal of the error message from the caller (if you want to see a v4 I'll send it, but hopefully this is OK): diff --git a/src/util/virpci.c b/src/util/virpci.c index e2d222e..c3d211f 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -42,6 +42,7 @@ #include "vircommand.h" #include "virerror.h" #include "virfile.h" +#include "virkmod.h" #include "virstring.h" #include "virutil.h" @@ -990,18 +991,32 @@ recheck: VIR_FREE(drvpath); if (!probed) { - const char *const probecmd[] = { MODPROBE, driver, NULL }; + char *errbuf = NULL; probed = true; - if (virRun(probecmd, NULL) < 0) { - char ebuf[1024]; - VIR_WARN("failed to load driver %s: %s", driver, - virStrerror(errno, ebuf, sizeof(ebuf))); - return -1; + if ((errbuf = virKModLoad(driver, true))) { + VIR_WARN("failed to load driver %s: %s", driver, errbuf);

+ VIR_FREE(errbuf); + goto cleanup; } goto recheck; } +cleanup: + /* If we know failure was because of blacklist, let's report that; + * otherwise, report a more generic failure message + */ + if (virKModIsBlacklisted(driver)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s: " + "administratively prohibited"), + driver); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to load PCI stub module %s"), + driver); + } + return -1; } @@ -1312,12 +1327,8 @@ virPCIDeviceDetach(virPCIDevicePtr dev, virPCIDeviceList *activeDevs, virPCIDeviceList *inactiveDevs) { - if (virPCIProbeStubDriver(dev->stubDriver) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to load PCI stub module %s"), - dev->stubDriver); + if (virPCIProbeStubDriver(dev->stubDriver) < 0) return -1; - } if (activeDevs && virPCIDeviceListFind(activeDevs, dev)) { virReportError(VIR_ERR_INTERNAL_ERROR,