[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1045124
>>
>> When loading modules, libvirt does not honor the modprobe blacklist.
>> Use the new virKModLoad() API in order to attempt load with blacklist check.
>> Use the new virKModIsBlacklisted() API to check if the failure to load
>> was due to the blacklist
>>
>> Signed-off-by: John Ferlan <jferlan redhat com>
>> ---
>>  src/util/virpci.c | 28 +++++++++++++++++++---------
>>  1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/util/virpci.c b/src/util/virpci.c
>> index e2d222e..41735eb 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,26 @@ 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;
>>      }
>>  
>> +    /* 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...

John


>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Failed to load PCI stub module %s"),
>> +                           dev->stubDriver);
>>          return -1;
>>      }
>>  
>>
> 
> Jan
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]