[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 04:29 PM, John Ferlan wrote:
> 
> 
> 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);

Can you do virReportError here with the contents of errbuf (and return instead
of jumping to cleanup)?

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

ACK

Jan


Attachment: signature.asc
Description: OpenPGP digital signature


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