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,