Re: [libvirt] [PATCH v3 4/5] qemu: auto-add pci-root controller for pc machine types

On 04/22/2013 02:43 PM, Ján Tomko wrote: > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index a7aabdf..ab99538 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -673,6 +673,37 @@ qemuDomainDefPostParse(virDomainDefPtr def, > !(def->emulator = virDomainDefGetDefaultEmulator(def, caps))) > return -1; > > + /* Add implicit PCI root controller if the machine has one */ > + switch (def->os.arch) { > + case VIR_ARCH_I686: > + case VIR_ARCH_X86_64: > + if (!def->os.machine) > + break; > + if (STRPREFIX(def->os.machine, "pc-q35") || > + STREQ(def->os.machine, "q35") || > + STREQ(def->os.machine, "isapc")) > + break; > + if (!STRPREFIX(def->os.machine, "pc-0.") && > + !STRPREFIX(def->os.machine, "pc-1.") && > + !STREQ(def->os.machine, "pc") && > + !STRPREFIX(def->os.machine, "rhel")) > + break; If you're going to fall through to a different case like this, you should put a comment in the code something like this: /* FALL THROUGH TO NEXT CASE */ just so people don't have to think too hard :-) However, I think it would be more easily expandable in the future if you had a straight switch statement with all the cases, and just set a "needsPCIRoot" boolean for those cases that need it, then an "if (needsPCIRoot)" at the end. In the future when we want to add other implicit devices, each case can be a mix of the appropriate "needsThis" and "needsThat", with the actual additions at the end. ...

ACK, with the addition of the "FALLTHROUGH" comment, or restructuring it is as I suggested.