qemu: Cleanup boot parameter building

This patch cleans up building the "-boot" parameter and while on that
fixes one inconsistency by modifying these things:

 - I completed the unfinished virDomainBootMenu enum by specifying
   LAST, declaring it and also declaring the TypeFromString and
   TypeToString parameters.
 - Previously mentioned TypeFromString and TypeToString are used when
   parsing the XML.
 - Last, but not least, visible change is that the "-boot" parameter
   is built and parsed properly:
    - The "order=" prefix is used only when additional parameters are
      used (menu, etc.).
    - It's rewritten in a way that other parameters can be added
      easily in the future (used in following patch).
    - The "order=" parameter is properly parsed regardless to where it
      is placed in the string (e.g. "menu=on,order=nc").
    - The "menu=" parameter (and others in the future) are created
      when they should be (i.e. even when bootindex is supported and
      used, but not when bootloader is selected).

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 15b360a76bd6951198f24461186f0d2900977cfb..35814fb021b907b1066b7a82c75010586b74d3a9 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -102,6 +102,11 @@ VIR_ENUM_IMPL(virDomainBoot, VIR_DOMAIN_BOOT_LAST,
               "hd",
               "network")
 
+VIR_ENUM_IMPL(virDomainBootMenu, VIR_DOMAIN_BOOT_MENU_LAST,
+              "default",
+              "yes",
+              "no")
+
 VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST,
               "acpi",
               "apic",
@@ -8181,10 +8186,15 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt,
 
     bootstr = virXPathString("string(./os/bootmenu[1]/@enable)", ctxt);
     if (bootstr) {
-        if (STREQ(bootstr, "yes"))
-            def->os.bootmenu = VIR_DOMAIN_BOOT_MENU_ENABLED;
-        else
+        def->os.bootmenu = virDomainBootMenuTypeFromString(bootstr);
+        if (def->os.bootmenu <= 0) {
+            /* In order not to break misconfigured machines, this
+             * should not emit an error, but rather set the bootmenu
+             * to disabled */
+            VIR_WARN("disabling bootmenu due to unknown option '%s'",
+                     bootstr);
             def->os.bootmenu = VIR_DOMAIN_BOOT_MENU_DISABLED;
+        }
         VIR_FREE(bootstr);
     }
 

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f0dea48bf3e23ca527044cdfbb3e5bb29b45652b..4801c27be77a8342a029a4cdd4ee754cd4222f2f 100644 (file)
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1359,6 +1359,8 @@ enum virDomainBootMenu {
     VIR_DOMAIN_BOOT_MENU_DEFAULT = 0,
     VIR_DOMAIN_BOOT_MENU_ENABLED,
     VIR_DOMAIN_BOOT_MENU_DISABLED,
+
+    VIR_DOMAIN_BOOT_MENU_LAST
 };
 
 enum virDomainFeature {
@@ -1429,6 +1431,7 @@ struct _virDomainOSDef {
     char *machine;
     int nBootDevs;
     int bootDevs[VIR_DOMAIN_BOOT_LAST];
+    /* enum virDomainBootMenu */
     int bootmenu;
     char *init;
     char **initargv;
@@ -1440,6 +1443,7 @@ struct _virDomainOSDef {
     char *bootloader;
     char *bootloaderArgs;
     int smbios_mode;
+
     virDomainBIOSDef bios;
 };
 
@@ -2129,6 +2133,7 @@ VIR_ENUM_DECL(virDomainTaint)
 
 VIR_ENUM_DECL(virDomainVirt)
 VIR_ENUM_DECL(virDomainBoot)
+VIR_ENUM_DECL(virDomainBootMenu)
 VIR_ENUM_DECL(virDomainFeature)
 VIR_ENUM_DECL(virDomainApicEoi)
 VIR_ENUM_DECL(virDomainLifecycle)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e75e489cc8488a1ef516566446bb0d645e853847..0b538950e94abb8949251be21826932b6f94ff7d 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -280,6 +280,8 @@ virDomainApicEoiTypeToString;
 virDomainAssignDef;
 virDomainBlockedReasonTypeFromString;
 virDomainBlockedReasonTypeToString;
+virDomainBootMenuTypeFromString;
+virDomainBootMenuTypeToString;
 virDomainChrConsoleTargetTypeFromString;
 virDomainChrConsoleTargetTypeToString;
 virDomainChrDefForeach;

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index cbf4aeec7a8a6bdaf5a4d4d7243ce3938dfcb33d..f8012ec16bc09cd818e049ce6906d8882df80967 100644 (file)
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4879,6 +4879,8 @@ qemuBuildCommandLine(virConnectPtr conn,
     }
 
     if (!def->os.bootloader) {
+        int boot_nparams = 0;
+        virBuffer boot_buf = VIR_BUFFER_INITIALIZER;
         /*
          * We prefer using explicit bootindex=N parameters for predictable
          * results even though domain XML doesn't use per device boot elements.
@@ -4901,7 +4903,6 @@ qemuBuildCommandLine(virConnectPtr conn,
         }
 
         if (!emitBootindex) {
-            virBuffer boot_buf = VIR_BUFFER_INITIALIZER;
             char boot[VIR_DOMAIN_BOOT_LAST+1];
 
             for (i = 0 ; i < def->os.nBootDevs ; i++) {
@@ -4925,19 +4926,38 @@ qemuBuildCommandLine(virConnectPtr conn,
             }
             boot[def->os.nBootDevs] = '\0';
 
-            virCommandAddArg(cmd, "-boot");
+            virBufferAsprintf(&boot_buf, "%s", boot);
+            boot_nparams++;
+        }
+
+        if (def->os.bootmenu) {
+            if (qemuCapsGet(caps, QEMU_CAPS_BOOT_MENU)) {
+                if (boot_nparams++)
+                    virBufferAddChar(&boot_buf, ',');
 
-            if (qemuCapsGet(caps, QEMU_CAPS_BOOT_MENU) &&
-                def->os.bootmenu != VIR_DOMAIN_BOOT_MENU_DEFAULT) {
                 if (def->os.bootmenu == VIR_DOMAIN_BOOT_MENU_ENABLED)
-                    virBufferAsprintf(&boot_buf, "order=%s,menu=on", boot);
-                else if (def->os.bootmenu == VIR_DOMAIN_BOOT_MENU_DISABLED)
-                    virBufferAsprintf(&boot_buf, "order=%s,menu=off", boot);
+                    virBufferAsprintf(&boot_buf, "menu=on");
+                else
+                    virBufferAsprintf(&boot_buf, "menu=off");
             } else {
-                virBufferAdd(&boot_buf, boot, -1);
+                /* We cannot emit an error when bootmenu is enabled but
+                 * unsupported because of backward compatibility */
+                VIR_WARN("bootmenu is enabled but not "
+                         "supported by this QEMU binary");
             }
 
-            virCommandAddArgBuffer(cmd, &boot_buf);
+        }
+
+        if (boot_nparams > 0) {
+            virCommandAddArg(cmd, "-boot");
+
+            if (boot_nparams < 2 || emitBootindex) {
+                virCommandAddArgBuffer(cmd, &boot_buf);
+            } else {
+                virCommandAddArgFormat(cmd,
+                                       "order=%s",
+                                       virBufferContentAndReset(&boot_buf));
+            }
         }
 
         if (def->os.kernel)
@@ -7861,6 +7881,26 @@ error:
 }
 
 
+static void
+qemuParseCommandLineBootDevs(virDomainDefPtr def, const char *str) {
+    int n, b = 0;
+
+    for (n = 0 ; str[n] && b < VIR_DOMAIN_BOOT_LAST ; n++) {
+        if (str[n] == 'a')
+            def->os.bootDevs[b++] = VIR_DOMAIN_BOOT_FLOPPY;
+        else if (str[n] == 'c')
+            def->os.bootDevs[b++] = VIR_DOMAIN_BOOT_DISK;
+        else if (str[n] == 'd')
+            def->os.bootDevs[b++] = VIR_DOMAIN_BOOT_CDROM;
+        else if (str[n] == 'n')
+            def->os.bootDevs[b++] = VIR_DOMAIN_BOOT_NET;
+        else if (str[n] == ',')
+            break;
+    }
+    def->os.nBootDevs = b;
+}
+
+
 /*
  * Analyse the env and argv settings and reconstruct a
  * virDomainDefPtr representing these settings as closely
@@ -8218,24 +8258,27 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps,
             if (!(def->os.cmdline = strdup(val)))
                 goto no_memory;
         } else if (STREQ(arg, "-boot")) {
-            int n, b = 0;
+            const char *token = NULL;
             WANT_VALUE();
-            for (n = 0 ; val[n] && b < VIR_DOMAIN_BOOT_LAST ; n++) {
-                if (val[n] == 'a')
-                    def->os.bootDevs[b++] = VIR_DOMAIN_BOOT_FLOPPY;
-                else if (val[n] == 'c')
-                    def->os.bootDevs[b++] = VIR_DOMAIN_BOOT_DISK;
-                else if (val[n] == 'd')
-                    def->os.bootDevs[b++] = VIR_DOMAIN_BOOT_CDROM;
-                else if (val[n] == 'n')
-                    def->os.bootDevs[b++] = VIR_DOMAIN_BOOT_NET;
-                else if (val[n] == ',')
-                    break;
-            }
-            def->os.nBootDevs = b;
 
-            if (strstr(val, "menu=on"))
-                def->os.bootmenu = 1;
+            if (!strchr(val, ','))
+                qemuParseCommandLineBootDevs(def, val);
+            else {
+                token = val;
+                while (token && *token) {
+                    if (STRPREFIX(token, "order=")) {
+                        token += strlen("order=");
+                        qemuParseCommandLineBootDevs(def, token);
+                    } else if (STRPREFIX(token, "menu=on")) {
+                        def->os.bootmenu = 1;
+                    }
+                    token = strchr(token, ',');
+                    /* This incrementation has to be done here in order to make it
+                     * possible to pass the token pointer properly into the loop */
+                    if (token)
+                        token++;
+                }
+            }
         } else if (STREQ(arg, "-name")) {
             char *process;
             WANT_VALUE();

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-boot-menu-disable-drive-bootindex.args b/tests/qemuxml2argvdata/qemuxml2argv-boot-menu-disable-drive-bootindex.args
index 5074e32bd82da50a9555d446ab080ab02cf24d85..75b6b4cb868a3265db7bb3674863dd633842eb2f 100644 (file)
--- a/tests/qemuxml2argvdata/qemuxml2argv-boot-menu-disable-drive-bootindex.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-boot-menu-disable-drive-bootindex.args
@@ -7,6 +7,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu \
 -nodefaults \
 -monitor unix:/tmp/test-monitor,server,nowait \
 -no-acpi \
+-boot menu=off \
 -drive file=/dev/cdrom,if=none,media=cdrom,id=drive-ide0-1-0 \
 -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0,bootindex=1 \
 -usb \