On 04/22/2013 10:37 PM, Laine Stump wrote:
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.
I'm squashing this in before pushing: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ab99538..98ac56f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -668,6 +668,8 @@ qemuDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, void *opaque ATTRIBUTE_UNUSED) { + bool addPCIRoot = false; + /* check for emulator and create a default one if needed */ if (!def->emulator && !(def->emulator = virDomainDefGetDefaultEmulator(def, caps))) @@ -688,6 +690,8 @@ qemuDomainDefPostParse(virDomainDefPtr def, !STREQ(def->os.machine, "pc") && !STRPREFIX(def->os.machine, "rhel")) break; + addPCIRoot = true; + break; case VIR_ARCH_ALPHA: case VIR_ARCH_PPC: @@ -695,15 +699,18 @@ qemuDomainDefPostParse(virDomainDefPtr def, case VIR_ARCH_PPCEMB: case VIR_ARCH_SH4: case VIR_ARCH_SH4EB: - if (virDomainDefMaybeAddController( - def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, - VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) - return -1; + addPCIRoot = true; break; default: break; } + if (addPCIRoot && + virDomainDefMaybeAddController( + def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, + VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) < 0) + return -1; + return 0; }