[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/7] qemu: eliminate almost-duplicate code in qemu_command.c



On Fri, Aug 2, 2013 at 11:55 AM, Laine Stump <laine laine org> wrote:
> * 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.
> ---
>  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);

So even though this patch has been ACK'd and committed. I really would
have liked to see some info about the device printed in the error
message because when you're defining a domain its really not clear
which device is the problem.

> +            } 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);

Same as above, when you're defining the whole domain its really not
clear which device is the problem so provide some details on which one
it is.

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



-- 
Doug Goldstein


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]