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

[Ján Tomko]

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;
 }