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,
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
