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