Re: [PATCH] conf: more useful error message when pci function is out of range

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

 



On 07/23/2015 10:21 AM, Laine Stump wrote:
> On 07/23/2015 05:04 AM, Ján Tomko wrote:
>> On Wed, Jul 22, 2015 at 12:04:41PM -0400, Laine Stump wrote:
>>> If a pci address had a function number out of range, the error message
>>> would be:
>>>
>>>   Insufficient specification for PCI address
>>>
>>> This was due to an unnecessary call to virDevicePCIAddressIsValid()
>>> during parse of the pci address - we will anyway check for validity of
>>> the PCI address later on.
>>>
>>> With that extra check removed, the error message is the much more useful:
>>>
>>>   Invalid PCI address 0000:02:06.8. function must be <= 7
>>>
>> That error is reported by virDomainPCIAddressValidate, called when we
>> validate and assign guest PCI addresses.
>>
>> Other code paths do not have that protection. After this patch, we
>> happily parse:
>> <hostdev mode='subsystem' type='pci' managed='yes'>
>>   <source>
>>     <address domain='0x0000' bus='0x02' slot='0x00' function='0x9'/>
>>   </source>
>> </hostdev>
> That is bad. It means that qemuCollectPCIAddress() (which calls
> virDomainPCIAddressReserveAddr(), which in turn calls
> virDomainPCIAddressValidate()) isn't being called for that slot, so it's
> not being added to the set of in-use addresses.
>
> I'll look into why that happens.

Derp. Of course it happens because it is the PCI adress on the *host*
side and not the guest side. I guess a full 1L french press of high
octane isn't enough to wake me up in the morning; I'd better go make
another (and in the meantime see about putting in better validation for
the source PCI address without messing it up for the target PCI address).

>
>> A vague error message (and refusing to continue with invalid data)
>> is better than no error, I think we should keep the error here.
> I still think this error should be removed, since
> virDevicePCIAddressIsValid() isn't used to determine an error condition
> in any other of its uses, and only does an approximation as to whether
> or not the address is valid (it can't do any better with the information
> it has, because it doesn't know anything about the type of bus it is
> plugging into and that info is unknown at the time the individual device
> is parsed).
>
> Instead, we should assure that the proper function
> (virDomainPCIAddressValidate()) is called at the appropriate time for
> *all* devices.

Sigh. Since this is the source PCI address, we *never* have the
information about the bus it's plugged into, so the rudimentary
validation like that in virDevicePCIAddressIsVali() is the best we can
hope for (although we still can do a better job of reporting the error
than just "this is bad").

>
>>> This resolves:
>>>
>>>   https://bugzilla.redhat.com/show_bug.cgi?id=1004596
>>> ---
>>>  src/conf/device_conf.c | 7 +------
>>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
>>> index e7b7957..09a7019 100644
>>> --- a/src/conf/device_conf.c
>>> +++ b/src/conf/device_conf.c
>>> @@ -1,7 +1,7 @@
>>>  /*
>>>   * device_conf.c: device XML handling
>>>   *
>>> - * Copyright (C) 2006-2012 Red Hat, Inc.
>>> + * Copyright (C) 2006-2015 Red Hat, Inc.
>> Oh no, this file was unprotected for almost three years.
>>
>> Jan
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]