qemu: Don't crash when parsing command line lacking -M

Parse the -M (or -machine) command line option before starting
processing in earnest and have a fallback ready in case it's not
present, so that while parsing other options we can rely on
def->os.machine being initialized.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1379218

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

src/qemu/qemu_parse_command.c

@@ -1884,6 +1884,98 @@ qemuParseCommandLine(virCapsPtr caps,
         }
     }
 
+    /* Detect machine type before processing any other arguments,
+     * because they might depend on it */
+    for (i = 1; progargv[i]; i++) {
+        const char *arg = progargv[i];
+
+        /* Make sure we have a single - for all options to
+           simplify next logic */
+        if (STRPREFIX(arg, "--"))
+            arg++;
+
+        if (STREQ(arg, "-M") ||
+            STREQ(arg, "-machine")) {
+            char *param;
+            size_t j = 0;
+
+            /* -machine [type=]name[,prop[=value][,...]]
+             * Set os.machine only if first parameter lacks '=' or
+             * contains explicit type='...' */
+            WANT_VALUE();
+            if (!(list = virStringSplit(val, ",", 0)))
+                goto error;
+            param = list[0];
+
+            if (STRPREFIX(param, "type="))
+                param += strlen("type=");
+            if (!strchr(param, '=')) {
+                if (VIR_STRDUP(def->os.machine, param) < 0)
+                    goto error;
+                j++;
+            }
+
+            /* handle all remaining "-machine" parameters */
+            while ((param = list[j++])) {
+                if (STRPREFIX(param, "dump-guest-core=")) {
+                    param += strlen("dump-guest-core=");
+                    def->mem.dump_core = virTristateSwitchTypeFromString(param);
+                    if (def->mem.dump_core <= 0)
+                        def->mem.dump_core = VIR_TRISTATE_SWITCH_ABSENT;
+                } else if (STRPREFIX(param, "mem-merge=off")) {
+                    def->mem.nosharepages = true;
+                } else if (STRPREFIX(param, "accel=kvm")) {
+                    def->virtType = VIR_DOMAIN_VIRT_KVM;
+                    def->features[VIR_DOMAIN_FEATURE_PAE] = VIR_TRISTATE_SWITCH_ON;
+                } else if (STRPREFIX(param, "aes-key-wrap=")) {
+                    if (STREQ(arg, "-M")) {
+                        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                                       _("aes-key-wrap is not supported with "
+                                         "this QEMU binary"));
+                        goto error;
+                    }
+                    param += strlen("aes-key-wrap=");
+                    if (!def->keywrap && VIR_ALLOC(def->keywrap) < 0)
+                        goto error;
+                    def->keywrap->aes = virTristateSwitchTypeFromString(param);
+                    if (def->keywrap->aes < 0)
+                        def->keywrap->aes = VIR_TRISTATE_SWITCH_ABSENT;
+                } else if (STRPREFIX(param, "dea-key-wrap=")) {
+                    if (STREQ(arg, "-M")) {
+                        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                                       _("dea-key-wrap is not supported with "
+                                         "this QEMU binary"));
+                        goto error;
+                    }
+                    param += strlen("dea-key-wrap=");
+                    if (!def->keywrap && VIR_ALLOC(def->keywrap) < 0)
+                        goto error;
+                    def->keywrap->dea = virTristateSwitchTypeFromString(param);
+                    if (def->keywrap->dea < 0)
+                        def->keywrap->dea = VIR_TRISTATE_SWITCH_ABSENT;
+                }
+            }
+            virStringListFree(list);
+            list = NULL;
+        }
+    }
+
+    /* If no machine type has been found among the arguments, then figure
+     * out a reasonable value by using capabilities */
+    if (!def->os.machine) {
+        virCapsDomainDataPtr capsdata;
+
+        if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type,
+                def->os.arch, def->virtType, NULL, NULL)))
+            goto error;
+
+        if (VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0) {
+            VIR_FREE(capsdata);
+            goto error;
+        }
+        VIR_FREE(capsdata);
+    }
+
     /* Now the real processing loop */
     for (i = 1; progargv[i]; i++) {
         const char *arg = progargv[i];
@@ -2137,69 +2229,6 @@ qemuParseCommandLine(virCapsPtr caps,
             }
             if (STREQ(def->name, ""))
                 VIR_FREE(def->name);
-        } else if (STREQ(arg, "-M") ||
-                   STREQ(arg, "-machine")) {
-            char *param;
-            size_t j = 0;
-
-            /* -machine [type=]name[,prop[=value][,...]]
-             * Set os.machine only if first parameter lacks '=' or
-             * contains explicit type='...' */
-            WANT_VALUE();
-            if (!(list = virStringSplit(val, ",", 0)))
-                goto error;
-            param = list[0];
-
-            if (STRPREFIX(param, "type="))
-                param += strlen("type=");
-            if (!strchr(param, '=')) {
-                if (VIR_STRDUP(def->os.machine, param) < 0)
-                    goto error;
-                j++;
-            }
-
-            /* handle all remaining "-machine" parameters */
-            while ((param = list[j++])) {
-                if (STRPREFIX(param, "dump-guest-core=")) {
-                    param += strlen("dump-guest-core=");
-                    def->mem.dump_core = virTristateSwitchTypeFromString(param);
-                    if (def->mem.dump_core <= 0)
-                        def->mem.dump_core = VIR_TRISTATE_SWITCH_ABSENT;
-                } else if (STRPREFIX(param, "mem-merge=off")) {
-                    def->mem.nosharepages = true;
-                } else if (STRPREFIX(param, "accel=kvm")) {
-                    def->virtType = VIR_DOMAIN_VIRT_KVM;
-                    def->features[VIR_DOMAIN_FEATURE_PAE] = VIR_TRISTATE_SWITCH_ON;
-                } else if (STRPREFIX(param, "aes-key-wrap=")) {
-                    if (STREQ(arg, "-M")) {
-                        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                                       _("aes-key-wrap is not supported with "
-                                         "this QEMU binary"));
-                        goto error;
-                    }
-                    param += strlen("aes-key-wrap=");
-                    if (!def->keywrap && VIR_ALLOC(def->keywrap) < 0)
-                        goto error;
-                    def->keywrap->aes = virTristateSwitchTypeFromString(param);
-                    if (def->keywrap->aes < 0)
-                        def->keywrap->aes = VIR_TRISTATE_SWITCH_ABSENT;
-                } else if (STRPREFIX(param, "dea-key-wrap=")) {
-                    if (STREQ(arg, "-M")) {
-                        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                                       _("dea-key-wrap is not supported with "
-                                         "this QEMU binary"));
-                        goto error;
-                    }
-                    param += strlen("dea-key-wrap=");
-                    if (!def->keywrap && VIR_ALLOC(def->keywrap) < 0)
-                        goto error;
-                    def->keywrap->dea = virTristateSwitchTypeFromString(param);
-                    if (def->keywrap->dea < 0)
-                        def->keywrap->dea = VIR_TRISTATE_SWITCH_ABSENT;
-                }
-            }
-            virStringListFree(list);
-            list = NULL;
         } else if (STREQ(arg, "-serial")) {
             WANT_VALUE();
             if (STRNEQ(val, "none")) {
@@ -2489,6 +2518,11 @@ qemuParseCommandLine(virCapsPtr caps,
 
                 argRecognized = false;
             }
+        } else if (STREQ(arg, "-M") ||
+                   STREQ(arg, "-machine")) {
+            /* This option has already been processed before entering this
+             * loop, so we just need to skip its argument and move along */
+            WANT_VALUE();
         } else {
             argRecognized = false;
         }
@@ -2571,20 +2605,6 @@ qemuParseCommandLine(virCapsPtr caps,
         }
     }
 
-    if (!def->os.machine) {
-        virCapsDomainDataPtr capsdata;
-
-        if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type,
-                def->os.arch, def->virtType, NULL, NULL)))
-            goto error;
-
-        if (VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0) {
-            VIR_FREE(capsdata);
-            goto error;
-        }
-        VIR_FREE(capsdata);
-    }
-
     if (!nographics && (def->ngraphics == 0 || have_sdl)) {
         virDomainGraphicsDefPtr sdl;
         const char *display = qemuFindEnv(progenv, "DISPLAY");

tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.args

@@ -0,0 +1,11 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-aarch64 \
+-name QEMUGuest1 \
+-m 512 \
+-hda /dev/HostVG/QEMUGuest1 \
+-cdrom /root/boot.iso

tests/qemuargv2xmldata/qemuargv2xml-nomachine-aarch64.xml

@@ -0,0 +1,39 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>524288</memory>
+  <currentMemory unit='KiB'>524288</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='aarch64' machine='virt'>hvm</type>
+  </os>
+  <features>
+    <gic version='2'/>
+  </features>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-aarch64</emulator>
+    <disk type='block' device='disk'>
+      <driver name='qemu' type='raw'/>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='hda' bus='ide'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <disk type='file' device='cdrom'>
+      <driver name='qemu' type='raw'/>
+      <source file='/root/boot.iso'/>
+      <target dev='hdc' bus='ide'/>
+      <readonly/>
+      <address type='drive' controller='0' bus='1' target='0' unit='0'/>
+    </disk>
+    <controller type='ide' index='0'/>
+    <graphics type='sdl'/>
+    <video>
+      <model type='cirrus' vram='16384' heads='1' primary='yes'/>
+    </video>
+    <memballoon model='none'/>
+  </devices>
+</domain>

tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.args

@@ -0,0 +1,11 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-ppc64 \
+-name QEMUGuest1 \
+-m 512 \
+-hda /dev/HostVG/QEMUGuest1 \
+-cdrom /root/boot.iso

tests/qemuargv2xmldata/qemuargv2xml-nomachine-ppc64.xml

@@ -0,0 +1,49 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>524288</memory>
+  <currentMemory unit='KiB'>524288</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='ppc64' machine='pseries'>hvm</type>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-ppc64</emulator>
+    <disk type='block' device='disk'>
+      <driver name='qemu' type='raw'/>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='hda' bus='scsi'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <disk type='file' device='cdrom'>
+      <driver name='qemu' type='raw'/>
+      <source file='/root/boot.iso'/>
+      <target dev='hdc' bus='scsi'/>
+      <readonly/>
+      <address type='drive' controller='0' bus='0' target='0' unit='2'/>
+    </disk>
+    <controller type='usb' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'>
+      <model name='spapr-pci-host-bridge'/>
+      <target index='0'/>
+    </controller>
+    <controller type='scsi' index='0'>
+      <address type='spapr-vio' reg='0x2000'/>
+    </controller>
+    <input type='keyboard' bus='usb'/>
+    <input type='mouse' bus='usb'/>
+    <graphics type='sdl'/>
+    <video>
+      <model type='cirrus' vram='16384' heads='1' primary='yes'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+    </video>
+    <memballoon model='none'/>
+    <panic model='pseries'/>
+  </devices>
+</domain>

tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.args

@@ -0,0 +1,11 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name QEMUGuest1 \
+-m 512 \
+-hda /dev/HostVG/QEMUGuest1 \
+-cdrom /root/boot.iso

tests/qemuargv2xmldata/qemuargv2xml-nomachine-x86_64.xml

@@ -0,0 +1,48 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <memory unit='KiB'>524288</memory>
+  <currentMemory unit='KiB'>524288</currentMemory>
+  <vcpu placement='static'>1</vcpu>
+  <os>
+    <type arch='x86_64' machine='pc-0.11'>hvm</type>
+  </os>
+  <features>
+    <acpi/>
+  </features>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <disk type='block' device='disk'>
+      <driver name='qemu' type='raw'/>
+      <source dev='/dev/HostVG/QEMUGuest1'/>
+      <target dev='hda' bus='ide'/>
+      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+    </disk>
+    <disk type='file' device='cdrom'>
+      <driver name='qemu' type='raw'/>
+      <source file='/root/boot.iso'/>
+      <target dev='hdc' bus='ide'/>
+      <readonly/>
+      <address type='drive' controller='0' bus='1' target='0' unit='0'/>
+    </disk>
+    <controller type='usb' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'/>
+    <controller type='ide' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+    </controller>
+    <input type='mouse' bus='ps2'/>
+    <input type='keyboard' bus='ps2'/>
+    <graphics type='sdl'/>
+    <video>
+      <model type='cirrus' vram='16384' heads='1' primary='yes'/>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+    </video>
+    <memballoon model='none'/>
+  </devices>
+</domain>

tests/qemuargv2xmltest.c

@@ -288,6 +288,10 @@ mymain(void)
     DO_TEST("machine-deakeywrap-off-argv");
     DO_TEST("machine-keywrap-none-argv");
 
+    DO_TEST("nomachine-x86_64");
+    DO_TEST("nomachine-aarch64");
+    DO_TEST("nomachine-ppc64");
+
     qemuTestDriverFree(&driver);
 
     return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;