Re: [libvirt] [PATCH v2 10/11] qemu: auto-add bridges and allow using them

On 04/17/2013 01:00 PM, Ján Tomko wrote: I wonder if it's worth squashing the indentation change into 7/11, leaving only the s/static// for this patch. Then again, it's the same net number of line changes. If you write this as: if (addr->bus >= addrs->nbuses) { virReportError(VIR_ERR_XML_ERROR, _("Only PCI buses up to %u are available"), then you don't have to split the string. Instead of casting, use %zu (since addrs->nbuses is size_t). I did a double-take reading that; how about: Ensure addr fits in the address set, by expanding it if needed. static int qemuPCIAddressSetGrow(... Do we need an else that fails if we have no pci bridge support, but more than one pci controller is present? Space after comma. I found this use of 'i' a bit confusing - any time you change the iteration conditions inside the loop body, it becomes trickier to follow the logic. I think you got it right, in that basically you want to iterate over 31 slots, instead of 32 (by skipping slot 0), but where the slot you probe starts from addrs->lastaddr and wraps around rather than starting at 1. Would it be any clearer if the inner loop iterated over i=1; i < SLOT_LAST, so that the only increment of i is in the loop header? Also, I think I see why keeping this cache logic is essential for bus 0 for back-compat reasons (we want to avoid hot-plugging a new device into a previously-used but now unplugged slot, at least until we have run out of unique slots to plug into, in order to minimize guest confusion about different devices trying to reuse a slot). But do we really need to extend it to bus 1? That is, if one call fills the last slot of bus 0 and sets addrs->lastaddr to 5, do we really want the next call to start at slot 5 of bus 1, or even though nothing has ever probed slots 1-4 of bus 1? I'm worried that your caching of last-used slot is not quite right; in fact, I wonder if you need to cache both last-used bus and last-used slot on that bus, or even last-used slot on all buses, rather than just a single last-used slot without relation to bus. Again, thinking about last-used slot, it seems like if this filled up all available slots on the bus, wouldn't it be better to set addrs->lastaddr to 1 (to start the next bus correctly)? Does the outer loop (on 'j') need to start iterating from the last-used bus instead of starting at bus 0 and finding all 31 slots full? Or is caching the last-used slot something that we can completely avoid in the case of a multibus support, and only worry about it for older qemu? I didn't spot any other logic questions, but making sure we get next address allocation done correctly is probably still worth a v3. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org