On Mon, Aug 5, 2013 at 8:13 PM, Laine Stump <laine(a)laine.org&gt; wrote: > q35 machines have an implicit ahci (sata) controller at 00:1F.2 which > has no "id" associated with it. For this reason, we can't refer to it > as "ahci0". Instead, we don't give an id on the commandline, which > qemu interprets as "use the first ahci controller". We then need to > specify the unit with "unit=%d" rather than adding it onto the bus > arg. > --- > src/qemu/qemu_command.c | 23 ++++++++++++++++++++--- > tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 ++ > tests/qemuxml2argvdata/qemuxml2argv-q35.xml | 5 +++++ > tests/qemuxml2argvtest.c | 2 +- > tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml | 5 +++++ > 5 files changed, 33 insertions(+), 4 deletions(-) > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 22ffa79..3d670f9 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -4211,9 +4211,26 @@ qemuBuildDriveDevStr(virDomainDefPtr def, > virBufferAddLit(&opt, "ide-drive"); > } > > - virBufferAsprintf(&opt, ",bus=ahci%d.%d", > - disk->info.addr.drive.controller, > - disk->info.addr.drive.unit); > + if (qemuDomainMachineIsQ35(def) && > + disk->info.addr.drive.controller == 0) { > + /* Q35 machines have an implicit ahci (sata) controller at > + * 00:1F.2 which has no "id" associated with it. For this > + * reason, we can't refer to it as "ahci0". Instead, we > + * don't give an id, which qemu interprets as "use the > + * first ahci controller". We then need to specify the > + * unit with "unit=%d" rather than adding it onto the bus > + * arg. > + */ > + virBufferAsprintf(&opt, ",unit=%d", disk->info.addr.drive.unit); > + } else { > + /* All other ahci controllers have been created by > + * libvirt, so they *do* have an id, and we can identify > + * them that way. > + */ > + virBufferAsprintf(&opt, ",bus=ahci%d.%d", > + disk->info.addr.drive.controller, > + disk->info.addr.drive.unit); > + } > break; > case VIR_DOMAIN_DISK_BUS_VIRTIO: > if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args > index a0ec66e..6b30b4d 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args > +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args > @@ -3,4 +3,6 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test \ > -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ > -device pci-bridge,chassis_nr=1,id=pci.1,bus=pcie.0,addr=0x2 \ > -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ > +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ > +-device ide-drive,unit=0,drive=drive-sata0-0-0,id=sata0-0-0 \ > -vga qxl -global qxl.ram_size=67108864 -global qxl.vram_size=18874368 > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml > index 3541b14..edaf6cb 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.xml > +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.xml > @@ -14,6 +14,11 @@ > <on_crash>destroy</on_crash> > <devices> > <emulator>/usr/libexec/qemu-kvm</emulator> > + <disk type='block' device='disk'> > + <source dev='/dev/HostVG/QEMUGuest1'/> > + <target dev='sda' bus='sata'/> > + <address type='drive' controller='0' bus='0' target='0' unit='0'/> > + </disk> > <controller type='pci' index='0' model='pcie-root'/> > <controller type='pci' index='1' model='dmi-to-pci-bridge'/> > <controller type='pci' index='2' model='pci-bridge'/> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index 0068d27..679124e 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -1001,7 +1001,7 @@ mymain(void) > DO_TEST("q35", > QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE, > QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, > - QEMU_CAPS_ICH9_AHCI, > + QEMU_CAPS_DRIVE, QEMU_CAPS_ICH9_AHCI, > QEMU_CAPS_VGA, QEMU_CAPS_DEVICE_VIDEO_PRIMARY, > QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL); > > diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml > index 2a86e61..96f8eaf 100644 > --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml > +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-q35.xml > @@ -14,6 +14,11 @@ > <on_crash>destroy</on_crash> > <devices> > <emulator>/usr/libexec/qemu-kvm</emulator> > + <disk type='block' device='disk'> > + <source dev='/dev/HostVG/QEMUGuest1'/> > + <target dev='sda' bus='sata'/> > + <address type='drive' controller='0' bus='0' target='0' unit='0'/> > + </disk> > <controller type='pci' index='0' model='pcie-root'/> > <controller type='pci' index='1' model='dmi-to-pci-bridge'/> > <controller type='pci' index='2' model='pci-bridge'/> > -- > 1.7.11.7 > After discussing this online, Laine informed me that its a stable characteristic of QEMU to treat "no id" as "first X" so this will always be stable. The names I had found when digging in the code aren't actually documented so they aren't guaranteed to stay that way. So long story short.... ACK.

On Mon, Aug 5, 2013 at 8:13 PM, Laine Stump <laine(a)laine.org&gt; 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