[libvirt] [PATCH] conf: more useful error message when pci function is out of range
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 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. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -115,11 +115,6 @@ virDevicePCIAddressParseXML(xmlNodePtr node, goto cleanup; } - if (!virDevicePCIAddressIsValid(addr)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Insufficient specification for PCI address")); - goto cleanup; - } ret = 0; -- 2.1.0
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> A vague error message (and refusing to continue with invalid data) is better than no error, I think we should keep the error here.
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
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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Ján Tomko -
Laine Stump