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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
