Re: [libvirt] [PATCHv4 5/5] qemu: auto-add bridges and allow using them

On 04/23/2013 06:47 AM, Ján Tomko wrote: > @@ -1326,15 +1368,53 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, > qemuDomainObjPrivatePtr priv = NULL; > > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { > + int max_idx = -1; So let's trace what happens if I have XML with no <controller> but I do use 33 PCI devices and have a capable qemu: max_idx starts at -1, > int nbuses = 0; > int i; > + int rv; > > for (i = 0; i < def->ncontrollers; i++) { > - if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) > - nbuses++; > + if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { > + if (def->controllers[i]->idx > max_idx) > + max_idx = def->controllers[i]->idx; > + } > + } If no controllers were specified, it is still at -1, > + > + nbuses = max_idx + 1; so nbuses is now 0, > + > + if (nbuses > 0 && > + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PCI_BRIDGE)) { therefore we skip this if, > + virDomainDeviceInfo info; > + /* 1st pass to figure out how many PCI bridges we need */ > + if (!(addrs = qemuDomainPCIAddressSetCreate(def, nbuses, true))) > + goto cleanup; > + if (qemuAssignDevicePCISlots(def, qemuCaps, addrs) < 0) > + goto cleanup; > + /* Reserve 1 extra slot for a (potential) bridge */ > + if (qemuDomainPCIAddressSetNextAddr(addrs, &info) < 0) > + goto cleanup; > + > + for (i = 1; i < addrs->nbuses; i++) { > + if ((rv = virDomainDefMaybeAddController( > + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, > + i, VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE)) < 0) > + goto cleanup; > + /* If we added a new bridge, we will need one more address */ > + if (rv > 0 && qemuDomainPCIAddressSetNextAddr(addrs, &info) < 0) > + goto cleanup; > + } > + nbuses = addrs->nbuses; > + qemuDomainPCIAddressSetFree(addrs); > + addrs = NULL; > + > + } else if (max_idx > 0) { we don't error out, > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("PCI bridges are not supported " > + "by this QEMU binary")); > + goto cleanup; > } but we also didn't auto-instantiate any bridges, even if the capability is supported. Is that a problem?