[libvirt] [PATCHv2] 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 which is logged by virDevicePCIAddressParseXML() after virDevicePCIAddressIsValid returns a failure. This patch enhances virDevicePCIAddressIsValid() to optionally report the error itself (since it is the place that decides which part of the address is "invalid"), and uses that feature when calling from virDevicePCIAddressParseXML(), so that the error will be more useful, e.g.: Invalid PCI address function=0x8, must be <= 7 Previously, virDevicePCIAddressIsValid didn't check for the theoretical limits of domain or bus, only for slot or function. While adding log messages, we also correct that ommission. (The RNG for PCI addresses already enforces this limit, which by the way means that we can't add any negative tests for this - as far as I know our domainschematest has no provisions for passing XML that is supposed to fail). Note that virDevicePCIAddressIsValid() can only check against the absolute maximum attribute values for *any* possible PCI controller, not for the actual maximums of the specific controller that this device is attaching to; fortunately there is later more specific validation for guest-side PCI addresses when building the set of assigned PCI addresses. For host-side PCI addresses (e.g. for <hostdev> and for network device pools), we rely on the error that will be logged when it is found that the device doesn't actually exist. This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1004596 --- Change from V1: V1 simply removed the validation from virDevicePCIAddressParseXML() in favor of allowing things later in the parse process to figure out the out-of-bounds conditions, but jtomko pointed out that there are other uses of virDevicePCIAddressParseXML() aside from the guest-side PCI addresses of devices, and that the other uses relied on the one validation check in virDevicePCIAddressParseXML() to make sure the address was sensible. So instead of simply removing the check, I had to make it smarter about what it logged. In the process, I added checks for max domain and bus. src/conf/device_conf.c | 52 +++++++++++++++++++++++++++++++++++++++++--------- src/conf/device_conf.h | 5 +++-- src/conf/domain_conf.c | 2 +- 3 files changed, 47 insertions(+), 12 deletions(-) diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index e7b7957..4e15d38 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 @@ -55,12 +55,49 @@ VIR_ENUM_IMPL(virNetDevFeature, "rdma", "txudptnl") -int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) +int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr, + bool report) { - /* PCI bus has 32 slots and 8 functions per slot */ - if (addr->slot >= 32 || addr->function >= 8) + if (addr->domain > 0xFFFF) { + if (report) + virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address domain='0x%x', " + "must be <= 0xFFFF"), + addr->domain); + return 0; + } + if (addr->bus > 0xFF) { + if (report) + virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address bus='0x%x', " + "must be <= 0xFF"), + addr->bus); return 0; - return addr->domain || addr->bus || addr->slot; + } + if (addr->slot > 0x1F) { + if (report) + virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address slot='0x%x', " + "must be <= 0x1F"), + addr->slot); + return 0; + } + if (addr->function > 7) { + if (report) + virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address function=0x%x, " + "must be <= 7"), + addr->function); + return 0; + } + if (!(addr->domain || addr->bus || addr->slot)) { + if (report) + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid PCI address 0000:00:00, at least " + "one of domain, bus, or slot must be > 0")); + return 0; + } + return 1; } @@ -115,11 +152,8 @@ virDevicePCIAddressParseXML(xmlNodePtr node, goto cleanup; } - if (!virDevicePCIAddressIsValid(addr)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Insufficient specification for PCI address")); + if (!virDevicePCIAddressIsValid(addr, true)) goto cleanup; - } ret = 0; diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 40a2b3d..85ce40f 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -1,7 +1,7 @@ /* * device_conf.h: device XML handling entry points * - * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2012, 2014-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 @@ -81,7 +81,8 @@ typedef enum { VIR_ENUM_DECL(virNetDevFeature) -int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr); +int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr, + bool report); int virDevicePCIAddressParseXML(xmlNodePtr node, virDevicePCIAddressPtr addr); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8a89b9a..5142201 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3133,7 +3133,7 @@ int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, switch (info->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: - return virDevicePCIAddressIsValid(&info->addr.pci); + return virDevicePCIAddressIsValid(&info->addr.pci, false); case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE: return 1; -- 2.1.0
On Thu, Jul 23, 2015 at 04:37:05PM -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
which is logged by virDevicePCIAddressParseXML() after virDevicePCIAddressIsValid returns a failure.
This patch enhances virDevicePCIAddressIsValid() to optionally report the error itself (since it is the place that decides which part of the address is "invalid"), and uses that feature when calling from virDevicePCIAddressParseXML(), so that the error will be more useful, e.g.:
Invalid PCI address function=0x8, must be <= 7
Previously, virDevicePCIAddressIsValid didn't check for the theoretical limits of domain or bus, only for slot or function. While adding log messages, we also correct that ommission.
(The RNG for PCI addresses already enforces this limit, which by the way means that we can't add any negative tests for this - as far as I know our domainschematest has no provisions for passing XML that is supposed to fail).
Any XML file with a name ending in *-invalid.xml is expected to fail validation, see tests/schematestutils.sh
Note that virDevicePCIAddressIsValid() can only check against the absolute maximum attribute values for *any* possible PCI controller, not for the actual maximums of the specific controller that this device is attaching to; fortunately there is later more specific validation for guest-side PCI addresses when building the set of assigned PCI addresses. For host-side PCI addresses (e.g. for <hostdev> and for network device pools), we rely on the error that will be logged when it is found that the device doesn't actually exist.
This resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=1004596 --- Change from V1:
V1 simply removed the validation from virDevicePCIAddressParseXML() in favor of allowing things later in the parse process to figure out the out-of-bounds conditions, but jtomko pointed out that there are other uses of virDevicePCIAddressParseXML() aside from the guest-side PCI addresses of devices, and that the other uses relied on the one validation check in virDevicePCIAddressParseXML() to make sure the address was sensible. So instead of simply removing the check, I had to make it smarter about what it logged. In the process, I added checks for max domain and bus.
src/conf/device_conf.c | 52 +++++++++++++++++++++++++++++++++++++++++--------- src/conf/device_conf.h | 5 +++-- src/conf/domain_conf.c | 2 +- 3 files changed, 47 insertions(+), 12 deletions(-)
diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index e7b7957..4e15d38 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 @@ -55,12 +55,49 @@ VIR_ENUM_IMPL(virNetDevFeature, "rdma", "txudptnl")
-int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) +int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr,
Returning bool would match the usage more closely, but that's pre-existing. ACK Jan
On 07/28/2015 08:53 AM, Ján Tomko wrote:
On Thu, Jul 23, 2015 at 04:37:05PM -0400, Laine Stump wrote:
(The RNG for PCI addresses already enforces this limit, which by the way means that we can't add any negative tests for this - as far as I know our domainschematest has no provisions for passing XML that is supposed to fail).
Any XML file with a name ending in *-invalid.xml is expected to fail validation, see tests/schematestutils.sh
Cool! I added a negative schema and parse test for each of domain, bus, slot, and function before I pushed.
-int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr) +int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr, Returning bool would match the usage more closely, but that's pre-existing.
Yeah, I had thought of changing that, but if I changed the return of this function, I would need to change the return type of virDomainDeviceAddressIsValid(), and then virDomainDeviceCCWAddressIsValid() (since it's used in virDomainDeviceAddressIsValid() identically to virDevicePCIAddressIsValid()). Fortunately it would have stopped there (as all the uses of all three functions essentially behave as if they returned bool anyway), but it seemed like a bit too much change unrelated to fixing the bug. I've finally pushed this patch. Thanks for the review!
participants (2)
-
Ján Tomko -
Laine Stump