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