11:04 a.m.
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
4:04 a.m.
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
9:21 a.m.
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
9:54 a.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3412
days inactive
3413
days old
3 comments
2 participants
participants (2)
-
Ján Tomko -
Laine Stump