[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 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,


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