12:55 p.m.
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.