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