From 3155b1327ffc178fea9ef96987dfbeb296862d58 Mon Sep 17 00:00:00 2001

Message-Id: <3155b1327ffc178fea9ef96987dfbeb296862d58.1377873636.git.jdenemar@redhat.com>

From: Laine Stump <laine@laine.org>

Date: Tue, 6 Aug 2013 13:23:20 -0600

Subject: [PATCH] qemu: eliminate almost-duplicate code in qemu_command.c

This patch is part of the resolution to:

   https://bugzilla.redhat.com/show_bug.cgi?id=819968

* The functions qemuDomainPCIAddressReserveAddr and

qemuDomainPCIAddressReserveSlot were very similar (and should have

been more similar) and were about to get more code added to them which

would create even more duplicated code, so this patch gives

qemuDomainPCIAddressReserveAddr a "reserveEntireSlot" arg, then

replaces the body of qemuDomainPCIAddressReserveSlot with a call to

qemuDomainPCIAddressReserveAddr.

You will notice that addrs->lastaddr was previously set in

qemuDomainPCIAddressReserveAddr (but *not* set in

qemuDomainPCIAddressReserveSlot). For consistency and cleanliness of

code, that bit was removed and put into the one caller of

qemuDomainPCIAddressReserveAddr (there is a similar place where the

caller of qemuDomainPCIAddressReserveSlot sets lastaddr). This does

guarantee identical functionality to pre-patch code, but in practice

isn't really critical, because lastaddr is just keeping track of where

to start when looking for a free slot - if it isn't updated, we will

just start looking on a slot that's already occupied, then skip up to

one that isn't.

* qemuCollectPCIAddress was essentially doing the same thing as

qemuDomainPCIAddressReserveAddr, but with some extra special case

checking at the beginning. The duplicate code has been replaced with

a call to qemuDomainPCIAddressReserveAddr. This required adding a

"fromConfig" boolean, which is only used to change the log error

code from VIR_ERR_INTERNAL_ERROR (when the address was

auto-generated by libvirt) to VIR_ERR_XML_ERROR (when the address is

coming from the config); without this differentiation, it would be

difficult to tell if an error was caused by something wrong in

libvirt's auto-allocate code or just bad config.

* the bit of code in qemuDomainPCIAddressValidate that checks the

connect type flags is going to be used in a couple more places where

we don't need to also check the slot limits (because we're generating

the slot number ourselves), so that has been pulled out into a

separate qemuDomainPCIAddressFlagsCompatible function.

(cherry picked from commit 3bb0125766a301a9d99884e7cdb002ba5fa5df01)

---

 src/qemu/qemu_command.c | 232 ++++++++++++++++++++++++------------------------

 src/qemu/qemu_command.h |   4 +-

 2 files changed, 119 insertions(+), 117 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c

index 4345456..abc973a 100644

--- a/src/qemu/qemu_command.c

+++ b/src/qemu/qemu_command.c

@@ -1434,9 +1434,53 @@ struct _qemuDomainPCIAddressSet {

 };

-/*

- * Check that the PCI address is valid for use

- * with the specified PCI address set.

+static bool

+qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr,

+                                    qemuDomainPCIConnectFlags busFlags,

+                                    qemuDomainPCIConnectFlags devFlags,

+                                    bool reportError)

+{

+    /* If this bus doesn't allow the type of connection (PCI

+     * vs. PCIe) required by the device, or if the device requires

+     * hot-plug and this bus doesn't have it, return false.

+     */

+    if (!(devFlags & busFlags & QEMU_PCI_CONNECT_TYPES_MASK)) {

+        if (reportError) {

+            if (devFlags & QEMU_PCI_CONNECT_TYPE_PCI) {

+                virReportError(VIR_ERR_INTERNAL_ERROR,

+                               _("PCI bus %.4x:%.2x is not compatible with the "

+                                 "device. Device requires a standard PCI slot, "

+                                 "which is not provided by this bus"),

+                               addr->domain, addr->bus);

+            } else {

+                /* this should never happen. If it does, there is a

+                 * bug in the code that sets the flag bits for devices.

+                 */

+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

+                           _("The device information has no PCI "

+                             "connection types listed"));

+            }

+        }

+        return false;

+    }

+    if ((devFlags & QEMU_PCI_CONNECT_HOTPLUGGABLE) &&

+        !(busFlags & QEMU_PCI_CONNECT_HOTPLUGGABLE)) {

+        if (reportError) {

+            virReportError(VIR_ERR_INTERNAL_ERROR,

+                           _("PCI bus %.4x:%.2x is not compatible with the "

+                             "device. Device requires hot-plug capability, "

+                             "which is not provided by the bus"),

+                           addr->domain, addr->bus);

+        }

+        return false;

+    }

+    return true;

+}

+

+

+/* Verify that the address is in bounds for the chosen bus, and

+ * that the bus is of the correct type for the device (via

+ * comparing the flags).

  */

 static bool

 qemuDomainPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs,

@@ -1466,24 +1510,9 @@ qemuDomainPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs,

     /* assure that at least one of the requested connection types is

      * provided by this bus

      */

-    if (!(flags & bus->flags & QEMU_PCI_CONNECT_TYPES_MASK)) {

-        virReportError(VIR_ERR_XML_ERROR,

-                       _("Invalid PCI address: The PCI controller "

-                         "providing bus %04x doesn't support "

-                         "connections appropriate for the device "

-                         "(%x required vs. %x provided by bus)"),

-                       addr->bus, flags & QEMU_PCI_CONNECT_TYPES_MASK,

-                       bus->flags & QEMU_PCI_CONNECT_TYPES_MASK);

-        return false;

-    }

-    /* make sure this bus allows hot-plug if the caller demands it */

-    if ((flags & QEMU_PCI_CONNECT_HOTPLUGGABLE) &&

-        !(bus->flags &  QEMU_PCI_CONNECT_HOTPLUGGABLE)) {

-        virReportError(VIR_ERR_XML_ERROR,

-                       _("Invalid PCI address: hot-pluggable slot requested, "

-                         "but bus %04x doesn't support hot-plug"), addr->bus);

+    if (!qemuDomainPCIAddressFlagsCompatible(addr, bus->flags, flags, true))

         return false;

-    }

+

     /* some "buses" are really just a single port */

     if (bus->minSlot && addr->slot < bus->minSlot) {

         virReportError(VIR_ERR_XML_ERROR,

@@ -1597,10 +1626,10 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,

                       virDomainDeviceInfoPtr info,

                       void *opaque)

 {

+    qemuDomainPCIAddressSetPtr addrs = opaque;

     int ret = -1;

-    char *str = NULL;

     virDevicePCIAddressPtr addr = &info->addr.pci;

-    qemuDomainPCIAddressSetPtr addrs = opaque;

+    bool entireSlot;

     /* FIXME: flags should be set according to the requirements of @device */

     qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE |

                                        QEMU_PCI_CONNECT_TYPE_PCI);

@@ -1639,56 +1668,15 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,

             return 0;

     }

-    /* add an additional bus of the correct type if needed */

-    if (addrs->dryRun &&

-        qemuDomainPCIAddressSetGrow(addrs, addr, flags) < 0)

-        return -1;

-

-    /* verify that the address is in bounds for the chosen bus, and

-     * that the bus is of the correct type for the device (via

-     * comparing the flags).

-     */

-    if (!qemuDomainPCIAddressValidate(addrs, addr, flags))

-        return -1;

-

-    if (!(str = qemuDomainPCIAddressAsString(addr)))

-        goto cleanup;

+    entireSlot = (addr->function == 0 &&

+                  addr->multi != VIR_DEVICE_ADDRESS_PCI_MULTI_ON);

-    /* check if already in use */

-    if (addrs->buses[addr->bus].slots[addr->slot] & (1 << addr->function)) {

-        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)"),

-                           str);

-        } else {

-            virReportError(VIR_ERR_XML_ERROR,

-                           _("Attempted double use of PCI Address '%s'"), str);

-        }

+    if (qemuDomainPCIAddressReserveAddr(addrs, addr, flags,

+                                        entireSlot, true) < 0)

         goto cleanup;

-    }

-    /* mark as in use */

-    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 */

-        if (addrs->buses[addr->bus].slots[addr->slot]) {

-            virReportError(VIR_ERR_XML_ERROR,

-                           _("Attempted double use of PCI Address on slot '%s' "

-                             "(need \"multifunction='off'\" for device "

-                             "on function 0)"),

-                           str);

-            goto cleanup;

-        }

-        addrs->buses[addr->bus].slots[addr->slot] = 0xFF;

-        VIR_DEBUG("Remembering PCI slot: %s (multifunction=off)", str);

-    } else {

-        VIR_DEBUG("Remembering PCI addr: %s", str);

-        addrs->buses[addr->bus].slots[addr->slot] |= 1 << addr->function;

-    }

     ret = 0;

 cleanup:

-    VIR_FREE(str);

     return ret;

 }

@@ -1871,46 +1859,73 @@ static bool qemuDomainPCIAddressSlotInUse(qemuDomainPCIAddressSetPtr addrs,

 }

+/*

+ * Reserve a slot (or just one function) for a device. If

+ * reserveEntireSlot is true, all functions for the slot are reserved,

+ * otherwise only one. If fromConfig is true, the address being

+ * requested came directly from the config and errors should be worded

+ * appropriately. If fromConfig is false, the address was

+ * automatically created by libvirt, so it is an internal error (not

+ * XML).

+ */

 int

 qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,

                                 virDevicePCIAddressPtr addr,

-                                qemuDomainPCIConnectFlags flags)

+                                qemuDomainPCIConnectFlags flags,

+                                bool reserveEntireSlot,

+                                bool fromConfig)

 {

-    char *str;

+    int ret = -1;

+    char *str = NULL;

     qemuDomainPCIAddressBusPtr bus;

-

-    if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr, flags) < 0)

-        return -1;

+    virErrorNumber errType = (fromConfig

+                              ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR);

     if (!(str = qemuDomainPCIAddressAsString(addr)))

-        return -1;

+        goto cleanup;

-    VIR_DEBUG("Reserving PCI addr %s", str);

+    /* Add an extra bus if necessary */

+    if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr, flags) < 0)

+        goto cleanup;

+    /* Check that the requested bus exists, is the correct type, and we

+     * are asking for a valid slot

+     */

+    if (!qemuDomainPCIAddressValidate(addrs, addr, flags))

+        goto cleanup;

     bus = &addrs->buses[addr->bus];

-    if ((bus->minSlot && addr->slot < bus->minSlot) ||

-        addr->slot > bus->maxSlot) {

-        virReportError(VIR_ERR_INTERNAL_ERROR,

-                       _("Unable to reserve PCI address %s. "

-                         "Slot %02x can't be used on bus %04x, "

-                         "only slots %02zx - %02zx are permitted on this bus."),

-                       str, addr->slot, addr->bus, bus->minSlot, bus->maxSlot);

-    }

-    if (bus->slots[addr->slot] & (1 << addr->function)) {

-        virReportError(VIR_ERR_INTERNAL_ERROR,

-                       _("unable to reserve PCI address %s already in use"), str);

-        VIR_FREE(str);

-        return -1;

+    if (reserveEntireSlot) {

+        if (bus->slots[addr->slot]) {

+            virReportError(errType,

+                           _("Attempted double use of PCI slot %s "

+                             "(may need \"multifunction='on'\" for "

+                             "device on function 0)"), str);

+            goto cleanup;

+        }

+        bus->slots[addr->slot] = 0xFF; /* reserve all functions of slot */

+        VIR_DEBUG("Reserving PCI slot %s (multifunction='off')", str);

+    } else {

+        if (bus->slots[addr->slot] & (1 << addr->function)) {

+            if (addr->function == 0) {

+                virReportError(errType,

+                               _("Attempted double use of PCI Address %s"), str);

+            } else {

+                virReportError(errType,

+                               _("Attempted double use of PCI Address %s "

+                                 "(may need \"multifunction='on'\" "

+                                 "for device on function 0)"), str);

+            }

+            goto cleanup;

+        }

+        bus->slots[addr->slot] |= (1 << addr->function);

+        VIR_DEBUG("Reserving PCI address %s", str);

     }

+    ret = 0;

+cleanup:

     VIR_FREE(str);

-

-    addrs->lastaddr = *addr;

-    addrs->lastaddr.function = 0;

-    addrs->lastaddr.multi = 0;

-    bus->slots[addr->slot] |= 1 << addr->function;

-    return 0;

+    return ret;

 }

@@ -1919,26 +1934,7 @@ qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,

                                 virDevicePCIAddressPtr addr,

                                 qemuDomainPCIConnectFlags flags)

 {

-    char *str;

-

-    if (addrs->dryRun && qemuDomainPCIAddressSetGrow(addrs, addr, flags) < 0)

-        return -1;

-

-    if (!(str = qemuDomainPCIAddressAsString(addr)))

-        return -1;

-

-    VIR_DEBUG("Reserving PCI slot %s", str);

-

-    if (addrs->buses[addr->bus].slots[addr->slot]) {

-        virReportError(VIR_ERR_INTERNAL_ERROR,

-                       _("unable to reserve PCI slot %s"), str);

-        VIR_FREE(str);

-        return -1;

-    }

-

-    VIR_FREE(str);

-    addrs->buses[addr->bus].slots[addr->slot] = 0xFF;

-    return 0;

+    return qemuDomainPCIAddressReserveAddr(addrs, addr, flags, true, false);

 }

 int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,

@@ -2044,12 +2040,11 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,

                 VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use",

                           a.domain, a.bus, a.slot);

             }

-

         }

     }

     virReportError(VIR_ERR_INTERNAL_ERROR,

-                   "%s", _("No more available PCI addresses"));

+                   "%s", _("No more available PCI slots"));

     return -1;

 success:

@@ -2409,9 +2404,14 @@ qemuAssignDevicePCISlots(virDomainDefPtr def,

                 addr.bus = tmp_addr.bus;

                 addr.slot = tmp_addr.slot;

+

+                addrs->lastaddr = addr;

+                addrs->lastaddr.function = 0;

+                addrs->lastaddr.multi = 0;

             }

             /* Finally we can reserve the slot+function */

-            if (qemuDomainPCIAddressReserveAddr(addrs, &addr, flags) < 0)

+            if (qemuDomainPCIAddressReserveAddr(addrs, &addr, flags,

+                                                false, false) < 0)

                 goto error;

             def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;

diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h

index c9f1600..bf4953a 100644

--- a/src/qemu/qemu_command.h

+++ b/src/qemu/qemu_command.h

@@ -253,7 +253,9 @@ int qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,

                                     qemuDomainPCIConnectFlags flags);

 int qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,

                                     virDevicePCIAddressPtr addr,

-                                    qemuDomainPCIConnectFlags flags);

+                                    qemuDomainPCIConnectFlags flags,

+                                    bool reserveEntireSlot,

+                                    bool fromConfig);

 int qemuDomainPCIAddressReserveNextSlot(qemuDomainPCIAddressSetPtr addrs,

                                         virDomainDeviceInfoPtr dev,

                                         qemuDomainPCIConnectFlags flags);

-- 

1.8.3.2