[libvirt] [PATCH] conf: more useful error message when pci function is out of range
laine at laine.org
Thu Jul 23 14:54:03 UTC 2015
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'>
>> <address domain='0x0000' bus='0x02' slot='0x00' function='0x9'/>
> 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:
>>> 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.
> libvir-list mailing list
> libvir-list at redhat.com
More information about the libvir-list