On 04/03/2013 11:50 AM, Ján Tomko wrote:
Move bus and domain checks from qemuPCIAddressAsString to a separate function and add a check for function and slot so that we can switch from a hash table to an array.
Remove redundant checks in qemuBuildDeviceAddressStr. --- src/qemu/qemu_command.c | 111 +++++++++++++++++++++++++++++------------------- 1 file changed, 68 insertions(+), 43 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8321dcd..a16d5f1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1193,17 +1193,43 @@ struct _qemuDomainPCIAddressSet { };
+/* Check the PCI address + * Returns -1 if the address is unusable + * 0 if it's OK. + */ +static int qemuPCIAddressCheck(qemuDomainPCIAddressSetPtr addrs ATTRIBUTE_UNUSED, + virDevicePCIAddressPtr addr)
How about naming this qemuPCIAddressValidate()? (This is especially good since the verb "Check" is used elsewhere in this file to mean "check to see if this is *in use*")
+{ + if (addr->domain != 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Only PCI domain 0 is available")); + return -1; + } + if (addr->bus != 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Only PCI bus 0 is available")); + return -1; + } + if (addr->function >= QEMU_PCI_ADDRESS_LAST_FUNCTION) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address: function must be < %u"), + QEMU_PCI_ADDRESS_LAST_FUNCTION); + return -1; + } + if (addr->slot >= QEMU_PCI_ADDRESS_LAST_SLOT) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid PCI address: slot must be < %u"), + QEMU_PCI_ADDRESS_LAST_SLOT); + return -1; + } + return 0; +} + + static char *qemuPCIAddressAsString(virDevicePCIAddressPtr addr) { char *str;
- if (addr->domain != 0 || - addr->bus != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Only PCI domain 0 and bus 0 are available")); - return NULL; - } -
Yes, definitely by the time we are converting this to a string it should have already been validated.
if (virAsprintf(&str, "%d:%d:%d.%d", addr->domain, addr->bus, @@ -1222,7 +1248,8 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, void *opaque) { int ret = -1; - char *addr = NULL; + char *str = NULL; + virDevicePCIAddressPtr addr = &info->addr.pci; qemuDomainPCIAddressSetPtr addrs = opaque;
if ((info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) @@ -1235,57 +1262,60 @@ static int qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, return 0; }
- addr = qemuPCIAddressAsString(&info->addr.pci); - if (!addr) + if (qemuPCIAddressCheck(addrs, addr) < 0) + return -1; + + str = qemuPCIAddressAsString(addr); + if (!str) goto cleanup;
I prefer putting the assignment into the if condition: if (!(str = qemuPCIAddressAsString(addr))) goto cleanup;
- if (virHashLookup(addrs->used, addr)) { + if (virHashLookup(addrs->used, str)) { if (info->addr.pci.function != 0) { virReportError(VIR_ERR_XML_ERROR, _("Attempted double use of PCI Address '%s' " "(may need \"multifunction='on'\" for device on function 0)"), - addr); + str); } else { virReportError(VIR_ERR_XML_ERROR, - _("Attempted double use of PCI Address '%s'"), addr); + _("Attempted double use of PCI Address '%s'"), str); } goto cleanup; }
- VIR_DEBUG("Remembering PCI addr %s", addr); - if (virHashAddEntry(addrs->used, addr, addr) < 0) + VIR_DEBUG("Remembering PCI addr %s", str); + if (virHashAddEntry(addrs->used, str, str) < 0) goto cleanup; - addr = NULL; + str = NULL;
if ((info->addr.pci.function == 0) && (info->addr.pci.multi != VIR_DEVICE_ADDRESS_PCI_MULTI_ON)) { /* a function 0 w/o multifunction=on must reserve the entire slot */ - virDevicePCIAddress tmp_addr = info->addr.pci; + virDevicePCIAddress tmp_addr = *addr; unsigned int *func = &tmp_addr.function;
for (*func = 1; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) { - addr = qemuPCIAddressAsString(&tmp_addr); - if (!addr) + str = qemuPCIAddressAsString(&tmp_addr); + if (!str) goto cleanup;
Again, as long as you're modifying the lines, might as well stuff the assignment into the if condition.
- if (virHashLookup(addrs->used, addr)) { + if (virHashLookup(addrs->used, str)) { virReportError(VIR_ERR_XML_ERROR, _("Attempted double use of PCI Address '%s' " "(need \"multifunction='off'\" for device " "on function 0)"), - addr); + str); goto cleanup; }
- VIR_DEBUG("Remembering PCI addr %s (multifunction=off for function 0)", addr); - if (virHashAddEntry(addrs->used, addr, addr)) + VIR_DEBUG("Remembering PCI addr %s (multifunction=off for function 0)", str); + if (virHashAddEntry(addrs->used, str, str)) goto cleanup; - addr = NULL; + str = NULL; } } ret = 0; cleanup: - VIR_FREE(addr); + VIR_FREE(str); return ret; }
@@ -1385,6 +1415,9 @@ static int qemuDomainPCIAddressCheckSlot(qemuDomainPCIAddressSetPtr addrs,
I just noticed that the (existing) comment for this function isn't worded very well. As long as you're modifying things, could you fix that too? (just s/the other/another/g) Hmm, and now that I've suggested changing the name of qemuPCIAddressCheck because of this function using the word "Check" differently, I'm thinking *this* function could be better named as well. How about qemuDomainPCIAddressInUse()? Also, I think it should return true or false, not 0 or -1 (with associated adjustments in callers).
virDevicePCIAddress tmp_addr = *addr; unsigned int *func = &(tmp_addr.function);
+ if (qemuPCIAddressCheck(addrs, addr) < 0) + return -1; +
And as a matter of fact, I think you shouldn't be validating the PCI address here - in two of the 3 callers, a fixed hard-coded pci address is constructed (so you know that it's always valid), and in the 3rd caller, it's being done inside a loop whose index self-limits the PCI address to a valid range. (This is good, because if you left the call to the validation in here, you would have to have a tri-state return value to allow for failure as well as inuse/free).
for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) { str = qemuPCIAddressAsString(&tmp_addr); if (!str) @@ -1406,6 +1439,9 @@ int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs, { char *str;
+ if (qemuPCIAddressCheck(addrs, addr) < 0) + return -1; + str = qemuPCIAddressAsString(addr); if (!str) return -1; @@ -1479,6 +1515,9 @@ int qemuDomainPCIAddressReleaseAddr(qemuDomainPCIAddressSetPtr addrs, char *str; int ret;
+ if (qemuPCIAddressCheck(addrs, addr) < 0) + return -1; + str = qemuPCIAddressAsString(addr); if (!str) return -1; @@ -1498,6 +1537,9 @@ int qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs, virDevicePCIAddress tmp_addr = *addr; unsigned int *func = &tmp_addr.function;
+ if (qemuPCIAddressCheck(addrs, addr) < 0) + return -1; + for (*func = 0; *func < QEMU_PCI_ADDRESS_LAST_FUNCTION; (*func)++) { str = qemuPCIAddressAsString(&tmp_addr); if (!str) @@ -1965,24 +2007,7 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, virQEMUCapsPtr qemuCaps) { if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - if (info->addr.pci.domain != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Only PCI device addresses with domain=0 are supported")); - return -1; - } - if (info->addr.pci.bus != 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Only PCI device addresses with bus=0 are supported")); - return -1; - } - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIFUNCTION)) { - if (info->addr.pci.function > 7) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("The function of PCI device addresses must " - "be less than 8")); - return -1; - } - } else { + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIFUNCTION)) { if (info->addr.pci.function != 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Only PCI device addresses with function=0 "
Looks fine aside from the nits I listed above.