Re: [libvirt] [PATCH] conf: more useful error message when pci function is out of range
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.
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.
> 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