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

Re: [libvirt] [PATCHv3 2/2] qemu: improve error reporting during PCI address validation



On Mon, Aug 5, 2013 at 8:13 PM, Laine Stump <laine laine org> wrote:
> This patch addresses two concerns with the error reporting when an
> incompatible PCI address is specified for a device:
>
> 1) It wasn't always apparent which device had the problem. With this
> patch applied, any error about an incompatible address will always
> contain the full address as given in the config, so it will be easier
> to determine which device's config aused the problem.
>
> 2) In some cases when the problem came from bad config, the error
> message was erroneously classified as VIR_ERR_INTERNAL_ERROR. With
> this patch applied, the same error message will be changed to indicate
> either "internal" or "xml" error depending on whether the address came
> from the config, or was automatically generated by libvirt.
>
> Note that in the case of "internal" (due to bad auto-generation)
> errors, the PCI address won't be of much use in finding the location
> in config to change (because it was automatically generated). Of
> course that makes perfect sense, but still the address could provide a
> clue about a bug in libvirt attempting to use a type of pci bus that
> doesn't have its flags set correctly (or something similar). In other
> words, it's not perfect, but it is definitely better.
> ---
>  src/qemu/qemu_command.c | 224 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 138 insertions(+), 86 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 3d670f9..6060bb0 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1445,10 +1445,15 @@ struct _qemuDomainPCIAddressSet {
>
>  static bool
>  qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr,
> +                                    char *addrStr,

Slight nitpick. const char maybe?

>                                      qemuDomainPCIConnectFlags busFlags,
>                                      qemuDomainPCIConnectFlags devFlags,
> -                                    bool reportError)
> +                                    bool reportError,
> +                                    bool fromConfig)
>  {
> +    virErrorNumber errType = (fromConfig
> +                              ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR);
> +
>      /* 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.
> @@ -1456,24 +1461,24 @@ qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr,
>      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);
> +                virReportError(errType,
> +                               _("PCI bus is not compatible with the device "
> +                                 "at %s. Device requires a standard PCI slot, "
> +                                 "which is not provided by bus %.4x:%.2x"),
> +                               addrStr, addr->domain, addr->bus);
>              } else if (devFlags & QEMU_PCI_CONNECT_TYPE_PCIE) {
> -                virReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("PCI bus %.4x:%.2x is not compatible with the "
> -                                 "device. Device requires a PCI Express slot, "
> -                                 "which is not provided by this bus"),
> -                               addr->domain, addr->bus);
> +                virReportError(errType,
> +                               _("PCI bus is not compatible with the device "
> +                                 "at %s. Device requires a PCI Express slot, "
> +                                 "which is not provided by bus %.4x:%.2x"),
> +                               addrStr, 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"));
> +                virReportError(errType,
> +                           _("The device information for %s has no PCI "
> +                             "connection types listed"), addrStr);
>              }
>          }
>          return false;
> @@ -1481,11 +1486,11 @@ qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr,
>      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);
> +            virReportError(errType,
> +                           _("PCI bus is not compatible with the device "
> +                             "at %s. Device requires hot-plug capability, "
> +                             "which is not provided by bus %.4x:%.2x"),
> +                           addrStr, addr->domain, addr->bus);
>          }
>          return false;
>      }
> @@ -1500,23 +1505,30 @@ qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr,
>  static bool
>  qemuDomainPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs,
>                               virDevicePCIAddressPtr addr,
> -                             qemuDomainPCIConnectFlags flags)
> +                             char *addrStr,

Same here? const char?

> +                             qemuDomainPCIConnectFlags flags,
> +                             bool fromConfig)
>  {
>      qemuDomainPCIAddressBusPtr bus;
> +    virErrorNumber errType = (fromConfig
> +                              ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR);
>
>      if (addrs->nbuses == 0) {
> -        virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available"));
> +        virReportError(errType, "%s", _("No PCI buses available"));
>          return false;
>      }
>      if (addr->domain != 0) {
> -        virReportError(VIR_ERR_XML_ERROR, "%s",
> -                       _("Only PCI domain 0 is available"));
> +        virReportError(errType,
> +                       _("Invalid PCI address %s. "
> +                         "Only PCI domain 0 is available"),
> +                       addrStr);
>          return false;
>      }
>      if (addr->bus >= addrs->nbuses) {
> -        virReportError(VIR_ERR_XML_ERROR,
> -                       _("Only PCI buses up to %zu are available"),
> -                       addrs->nbuses - 1);
> +        virReportError(errType,
> +                       _("Invalid PCI address %s. "
> +                         "Only PCI buses up to %zu are available"),
> +                       addrStr, addrs->nbuses - 1);
>          return false;
>      }
>
> @@ -1525,26 +1537,27 @@ qemuDomainPCIAddressValidate(qemuDomainPCIAddressSetPtr addrs,
>      /* assure that at least one of the requested connection types is
>       * provided by this bus
>       */
> -    if (!qemuDomainPCIAddressFlagsCompatible(addr, bus->flags, flags, true))
> +    if (!qemuDomainPCIAddressFlagsCompatible(addr, addrStr, bus->flags,
> +                                             flags, true, fromConfig))
>          return false;
>
>      /* some "buses" are really just a single port */
>      if (bus->minSlot && addr->slot < bus->minSlot) {
> -        virReportError(VIR_ERR_XML_ERROR,
> -                       _("Invalid PCI address: slot must be >= %zu"),
> -                       bus->minSlot);
> +        virReportError(errType,
> +                       _("Invalid PCI address %s. slot must be >= %zu"),
> +                       addrStr, bus->minSlot);
>          return false;
>      }
>      if (addr->slot > bus->maxSlot) {
> -        virReportError(VIR_ERR_XML_ERROR,
> -                       _("Invalid PCI address: slot must be <= %zu"),
> -                       bus->maxSlot);
> +        virReportError(errType,
> +                       _("Invalid PCI address %s. slot must be <= %zu"),
> +                       addrStr, bus->maxSlot);
>          return false;
>      }
>      if (addr->function > QEMU_PCI_ADDRESS_FUNCTION_LAST) {
> -        virReportError(VIR_ERR_XML_ERROR,
> -                       _("Invalid PCI address: function must be <= %u"),
> -                       QEMU_PCI_ADDRESS_FUNCTION_LAST);
> +        virReportError(errType,
> +                       _("Invalid PCI address %s. function must be <= %u"),
> +                       addrStr, QEMU_PCI_ADDRESS_FUNCTION_LAST);
>          return false;
>      }
>      return true;
> @@ -1953,12 +1966,12 @@ qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
>                                  bool fromConfig)
>  {
>      int ret = -1;
> -    char *str = NULL;
> +    char *addrStr = NULL;
>      qemuDomainPCIAddressBusPtr bus;
>      virErrorNumber errType = (fromConfig
>                                ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR);
>
> -    if (!(str = qemuDomainPCIAddressAsString(addr)))
> +    if (!(addrStr = qemuDomainPCIAddressAsString(addr)))
>          goto cleanup;
>
>      /* Add an extra bus if necessary */
> @@ -1967,7 +1980,7 @@ qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
>      /* Check that the requested bus exists, is the correct type, and we
>       * are asking for a valid slot
>       */
> -    if (!qemuDomainPCIAddressValidate(addrs, addr, flags))
> +    if (!qemuDomainPCIAddressValidate(addrs, addr, addrStr, flags, fromConfig))
>          goto cleanup;
>
>      bus = &addrs->buses[addr->bus];
> @@ -1977,31 +1990,32 @@ qemuDomainPCIAddressReserveAddr(qemuDomainPCIAddressSetPtr addrs,
>              virReportError(errType,
>                             _("Attempted double use of PCI slot %s "
>                               "(may need \"multifunction='on'\" for "
> -                             "device on function 0)"), str);
> +                             "device on function 0)"), addrStr);
>              goto cleanup;
>          }
>          bus->slots[addr->slot] = 0xFF; /* reserve all functions of slot */
> -        VIR_DEBUG("Reserving PCI slot %s (multifunction='off')", str);
> +        VIR_DEBUG("Reserving PCI slot %s (multifunction='off')", addrStr);
>      } else {
>          if (bus->slots[addr->slot] & (1 << addr->function)) {
>              if (addr->function == 0) {
>                  virReportError(errType,
> -                               _("Attempted double use of PCI Address %s"), str);
> +                               _("Attempted double use of PCI Address %s"),
> +                               addrStr);
>              } else {
>                  virReportError(errType,
>                                 _("Attempted double use of PCI Address %s "
>                                   "(may need \"multifunction='on'\" "
> -                                 "for device on function 0)"), str);
> +                                 "for device on function 0)"), addrStr);
>              }
>              goto cleanup;
>          }
>          bus->slots[addr->slot] |= (1 << addr->function);
> -        VIR_DEBUG("Reserving PCI address %s", str);
> +        VIR_DEBUG("Reserving PCI address %s", addrStr);
>      }
>
>      ret = 0;
>  cleanup:
> -    VIR_FREE(str);
> +    VIR_FREE(addrStr);
>      return ret;
>  }
>
> @@ -2017,7 +2031,8 @@ qemuDomainPCIAddressReserveSlot(qemuDomainPCIAddressSetPtr addrs,
>  int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
>                                     virDomainDeviceInfoPtr dev)
>  {
> -    int ret = 0;
> +    int ret = -1;
> +    char *addrStr = NULL;
>      /* Flags should be set according to the particular device,
>       * but only the caller knows the type of device. Currently this
>       * function is only used for hot-plug, though, and hot-plug is
> @@ -2026,6 +2041,9 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
>      qemuDomainPCIConnectFlags flags = (QEMU_PCI_CONNECT_HOTPLUGGABLE |
>                                         QEMU_PCI_CONNECT_TYPE_PCI);
>
> +    if (!(addrStr = qemuDomainPCIAddressAsString(&dev->addr.pci)))
> +        goto cleanup;
> +
>      if (dev->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
>          /* We do not support hotplug multi-function PCI device now, so we should
>           * reserve the whole slot. The function of the PCI device must be 0.
> @@ -2034,16 +2052,20 @@ int qemuDomainPCIAddressEnsureAddr(qemuDomainPCIAddressSetPtr addrs,
>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                             _("Only PCI device addresses with function=0"
>                               " are supported"));
> -            return -1;
> +            goto cleanup;
>          }
>
> -        if (!qemuDomainPCIAddressValidate(addrs, &dev->addr.pci, flags))
> -            return -1;
> +        if (!qemuDomainPCIAddressValidate(addrs, &dev->addr.pci,
> +                                          addrStr, flags, true))
> +            goto cleanup;
>
>          ret = qemuDomainPCIAddressReserveSlot(addrs, &dev->addr.pci, flags);
>      } else {
>          ret = qemuDomainPCIAddressReserveNextSlot(addrs, dev, flags);
>      }
> +
> +cleanup:
> +    VIR_FREE(addrStr);
>      return ret;
>  }
>
> @@ -2063,12 +2085,20 @@ qemuDomainPCIAddressReleaseSlot(qemuDomainPCIAddressSetPtr addrs,
>       * already had it, and are giving it back.
>       */
>      qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPES_MASK;
> +    int ret = -1;
> +    char *addrStr = NULL;
>
> -    if (!qemuDomainPCIAddressValidate(addrs, addr, flags))
> -        return -1;
> +    if (!(addrStr = qemuDomainPCIAddressAsString(addr)))
> +        goto cleanup;
> +
> +    if (!qemuDomainPCIAddressValidate(addrs, addr, addrStr, flags, false))
> +        goto cleanup;
>
>      addrs->buses[addr->bus].slots[addr->slot] = 0;
> -    return 0;
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(addrStr);
> +    return ret;
>  }
>
>  void qemuDomainPCIAddressSetFree(qemuDomainPCIAddressSetPtr addrs)
> @@ -2090,6 +2120,7 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
>       * 0000:00:00.0
>       */
>      virDevicePCIAddress a = { 0, 0, 0, 0, false };
> +    char *addrStr = NULL;
>
>      /* except if this search is for the exact same type of device as
>       * last time, continue the search from the previous match
> @@ -2099,13 +2130,17 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
>
>      if (addrs->nbuses == 0) {
>          virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available"));
> -        return -1;
> +        goto error;
>      }
>
>      /* Start the search at the last used bus and slot */
>      for (a.slot++; a.bus < addrs->nbuses; a.bus++) {
> -        if (!qemuDomainPCIAddressFlagsCompatible(&a, addrs->buses[a.bus].flags,
> -                                                 flags, false)) {
> +        addrStr = NULL;
> +        if (!(addrStr = qemuDomainPCIAddressAsString(&a)))
> +            goto error;
> +        if (!qemuDomainPCIAddressFlagsCompatible(&a, addrStr,
> +                                                 addrs->buses[a.bus].flags,
> +                                                 flags, false, false)) {
>              VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device",
>                        a.domain, a.bus);
>              continue;
> @@ -2124,13 +2159,17 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
>      if (addrs->dryRun) {
>          /* a is already set to the first new bus and slot 1 */
>          if (qemuDomainPCIAddressSetGrow(addrs, &a, flags) < 0)
> -            return -1;
> +            goto error;
>          goto success;
>      } else if (flags == addrs->lastFlags) {
>          /* Check the buses from 0 up to the last used one */
>          for (a.bus = 0; a.bus <= addrs->lastaddr.bus; a.bus++) {
> -            if (!qemuDomainPCIAddressFlagsCompatible(&a, addrs->buses[a.bus].flags,
> -                                                     flags, false)) {
> +            addrStr = NULL;
> +            if (!(addrStr = qemuDomainPCIAddressAsString(&a)))
> +                goto error;
> +            if (!qemuDomainPCIAddressFlagsCompatible(&a, addrStr,
> +                                                     addrs->buses[a.bus].flags,
> +                                                     flags, false, false)) {
>                  VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device",
>                            a.domain, a.bus);
>                  continue;
> @@ -2147,12 +2186,15 @@ qemuDomainPCIAddressGetNextSlot(qemuDomainPCIAddressSetPtr addrs,
>
>      virReportError(VIR_ERR_INTERNAL_ERROR,
>                     "%s", _("No more available PCI slots"));
> +error:
> +    VIR_FREE(addrStr);
>      return -1;
>
>  success:
>      VIR_DEBUG("Found free PCI slot %.4x:%.2x:%.2x",
>                a.domain, a.bus, a.slot);
>      *next_addr = a;
> +    VIR_FREE(addrStr);
>      return 0;
>  }
>
> @@ -2217,10 +2259,12 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
>                                  virQEMUCapsPtr qemuCaps,
>                                  qemuDomainPCIAddressSetPtr addrs)
>  {
> +    int ret = -1;
>      size_t i;
>      virDevicePCIAddress tmp_addr;
>      bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
>      virDevicePCIAddressPtr addrptr;
> +    char *addrStr = NULL;
>      qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_HOTPLUGGABLE | QEMU_PCI_CONNECT_TYPE_PCI;
>
>      /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */
> @@ -2235,7 +2279,7 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
>                      def->controllers[i]->info.addr.pci.function != 1) {
>                      virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                                     _("Primary IDE controller must have PCI address 0:0:1.1"));
> -                    goto error;
> +                    goto cleanup;
>                  }
>              } else {
>                  def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> @@ -2255,7 +2299,7 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
>                      def->controllers[i]->info.addr.pci.function != 2) {
>                      virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                                     _("PIIX3 USB controller must have PCI address 0:0:1.2"));
> -                    goto error;
> +                    goto cleanup;
>                  }
>              } else {
>                  def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> @@ -2274,7 +2318,7 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
>          memset(&tmp_addr, 0, sizeof(tmp_addr));
>          tmp_addr.slot = 1;
>          if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0)
> -            goto error;
> +            goto cleanup;
>      }
>
>      if (def->nvideos > 0) {
> @@ -2293,8 +2337,11 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
>              primaryVideo->info.addr.pci.function = 0;
>              addrptr = &primaryVideo->info.addr.pci;
>
> -            if (!qemuDomainPCIAddressValidate(addrs, addrptr, flags))
> -                goto error;
> +            if (!(addrStr = qemuDomainPCIAddressAsString(addrptr)))
> +                goto cleanup;
> +            if (!qemuDomainPCIAddressValidate(addrs, addrptr,
> +                                              addrStr, flags, false))
> +                goto cleanup;
>
>              if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) {
>                  if (qemuDeviceVideoUsable) {
> @@ -2302,15 +2349,15 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
>                      if (qemuDomainPCIAddressReserveNextSlot(addrs,
>                                                              &primaryVideo->info,
>                                                              flags) < 0)
> -                        goto error;
> +                        goto cleanup;
>                  } else {
>                      virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                                     _("PCI address 0:0:2.0 is in use, "
>                                       "QEMU needs it for primary video"));
> -                    goto error;
> +                    goto cleanup;
>                  }
>              } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) {
> -                goto error;
> +                goto cleanup;
>              }
>          } else if (!qemuDeviceVideoUsable) {
>              if (primaryVideo->info.addr.pci.domain != 0 ||
> @@ -2319,7 +2366,7 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
>                  primaryVideo->info.addr.pci.function != 0) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                                 _("Primary video card must have PCI address 0:0:2.0"));
> -                goto error;
> +                goto cleanup;
>              }
>              /* If TYPE==PCI, then qemuCollectPCIAddress() function
>               * has already reserved the address, so we must skip */
> @@ -2334,13 +2381,13 @@ qemuValidateDevicePCISlotsPIIX3(virDomainDefPtr def,
>                        " intervention");
>              virResetLastError();
>          } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) {
> -            goto error;
> +            goto cleanup;
>          }
>      }
> -    return 0;
> -
> -error:
> -    return -1;
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(addrStr);
> +    return ret;
>  }
>
>
> @@ -2357,10 +2404,12 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
>                                      virQEMUCapsPtr qemuCaps,
>                                      qemuDomainPCIAddressSetPtr addrs)
>  {
> +    int ret = -1;
>      size_t i;
>      virDevicePCIAddress tmp_addr;
>      bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
>      virDevicePCIAddressPtr addrptr;
> +    char *addrStr = NULL;
>      qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPE_PCIE;
>
>      /* Verify that the first SATA controller is at 00:1F.2 */
> @@ -2375,7 +2424,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
>                      def->controllers[i]->info.addr.pci.function != 2) {
>                      virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                                     _("Primary SATA controller must have PCI address 0:0:1f.2"));
> -                    goto error;
> +                    goto cleanup;
>                  }
>              } else {
>                  def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
> @@ -2399,12 +2448,12 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
>          tmp_addr.multi = 1;
>          if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags,
>                                              false, false) < 0)
> -           goto error;
> +           goto cleanup;
>          tmp_addr.function = 3;
>          tmp_addr.multi = 0;
>          if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags,
>                                              false, false) < 0)
> -           goto error;
> +           goto cleanup;
>      }
>
>      if (def->nvideos > 0) {
> @@ -2423,8 +2472,11 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
>              primaryVideo->info.addr.pci.function = 0;
>              addrptr = &primaryVideo->info.addr.pci;
>
> -            if (!qemuDomainPCIAddressValidate(addrs, addrptr, flags))
> -                goto error;
> +            if (!(addrStr = qemuDomainPCIAddressAsString(addrptr)))
> +                goto cleanup;
> +            if (!qemuDomainPCIAddressValidate(addrs, addrptr,
> +                                              addrStr, flags, false))
> +                goto cleanup;
>
>              if (qemuDomainPCIAddressSlotInUse(addrs, addrptr)) {
>                  if (qemuDeviceVideoUsable) {
> @@ -2432,15 +2484,15 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
>                      if (qemuDomainPCIAddressReserveNextSlot(addrs,
>                                                              &primaryVideo->info,
>                                                              flags) < 0)
> -                        goto error;
> +                        goto cleanup;
>                  } else {
>                      virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                                     _("PCI address 0:0:1.0 is in use, "
>                                       "QEMU needs it for primary video"));
> -                    goto error;
> +                    goto cleanup;
>                  }
>              } else if (qemuDomainPCIAddressReserveSlot(addrs, addrptr, flags) < 0) {
> -                goto error;
> +                goto cleanup;
>              }
>          } else if (!qemuDeviceVideoUsable) {
>              if (primaryVideo->info.addr.pci.domain != 0 ||
> @@ -2449,7 +2501,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
>                  primaryVideo->info.addr.pci.function != 0) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                                 _("Primary video card must have PCI address 0:0:1.0"));
> -                goto error;
> +                goto cleanup;
>              }
>              /* If TYPE==PCI, then qemuCollectPCIAddress() function
>               * has already reserved the address, so we must skip */
> @@ -2464,13 +2516,13 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def,
>                        " intervention");
>              virResetLastError();
>          } else if (qemuDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) {
> -            goto error;
> +            goto cleanup;
>          }
>      }
> -    return 0;
> -
> -error:
> -    return -1;
> +    ret = 0;
> +cleanup:
> +    VIR_FREE(addrStr);
> +    return ret;
>  }
>
>
> --
> 1.7.11.7
>

Other than my slight nitpick, visually this looks good. I'll give it a
whirl with some of my bad configs tonight.

-- 
Doug Goldstein


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