On 01/30/2014 07:05 AM, Daniel P. Berrange wrote: > On Wed, Jan 29, 2014 at 07:52:45PM -0500, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1045124 >> >> When loading modules, libvirt does not honor the modprobe blacklist. >> By adding a "-b" to the modprobe command libvirt will fail to load a >> module if it's on the blacklist >> >> Check if the failure to load a driver was due to it being on the blacklist >> using the output of a "modprobe -c" searching for "blacklist <driver_Name>" >> where driver_Name is possibly a modified string of the input driver changing >> all '-' into '_' since that's what modprobe does. >> >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> >> --- >> src/util/virpci.c | 49 ++++++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 40 insertions(+), 9 deletions(-) >> >> diff --git a/src/util/virpci.c b/src/util/virpci.c >> index e2d222e..5d4168a 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" >> >> @@ -979,6 +980,9 @@ virPCIProbeStubDriver(const char *driver) >> { >> char *drvpath = NULL; >> bool probed = false; >> + size_t i; >> + char *drvblklst = NULL; >> + char *outbuf = NULL; >> >> recheck: >> if (virPCIDriverDir(&drvpath, driver) == 0 && virFileExists(drvpath)) { >> @@ -990,18 +994,44 @@ 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 = virModprobeUseBlacklist(driver))) { >> + VIR_WARN("failed to load driver %s: %s", driver, errbuf); >> + VIR_FREE(errbuf); >> + goto cleanup; >> } >> >> goto recheck; >> } >> >> + /* All error path code - purpose is to determine whether the failure >> + * occurs because device is on blacklist in order to add an error >> + * message to help detect why load failed >> + */ >> + if (virAsprintfQuiet(&drvblklst, "blacklist %s", driver) < 0) >> + goto cleanup; >> + >> + /* modprobe will convert all '-' into '_', so we need to as well */ >> + for (i = 0; i < drvblklst[i]; i++) >> + if (drvblklst[i] == '-') >> + drvblklst[i] = '_'; >> + >> + outbuf = virModprobeConfig(); >> + if (!outbuf) >> + goto cleanup; >> + >> + /* Find driver on blacklist? */ >> + if (strstr(outbuf, drvblklst)) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("Failed to load PCI stub module %s: " >> + "administratively prohibited"), >> + driver); >> + } > > So when I suggested creation of virkmod.{c,h} I was really meaning > that all this code would go in there. eg here you'd just invoke > > if (virKModIsBlacklisted(driver)) { > ....report error... > } > > so all the kmod config parsing would be isolated from this pci > code. > So here's the rub (since Jan asked and probably figured it out) - 'modprobe -b' returns 0 and no error when it finds the module on the blacklist. I guess I had "brain lock" over removing the VIR_WARN() and already having gone through a round of testing by the bz submitter in a "real" environment with the original set of changes. This was so much simpler when all that was originally asked for was a "-b" to be added to the MODPROBE command :-)... A v3 will come soon. John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list