[Qemu-devel] RFC: Small cleanup to legacy drive option parsing

[David Gibson]

Currently, the old-style options for specifying drives (-hd[abcd],
-cdrom, etc.) are equivalent to more longer new-style (-drive)
options.  However, in the code which handles these, the equivalency is
not directly obvious, since it's handled via the drive_add() function
which somewhat awkwardly translates an interface type enum into a
string option, so that drive_def() can change it back again.

This patch cleanups up this handling, by abolishing drive_add() and
replacing it with drive_def_print(), a convenience wrapper around
drive_def() which should make the equivalency between the leagcy
options and longer -drive strings more obvious to C programmers.

This also slighly reduces code size:
 blockdev.c |   22 -------------------
 vl.c       |   68 +++++++++++++++++++++++++++++++++++++------------------------
 2 files changed, 42 insertions(+), 48 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index ecf2252..7b8b8ef 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -92,28 +92,6 @@ QemuOpts *drive_def(const char *optstr)
     return qemu_opts_parse(qemu_find_opts("drive"), optstr, 0);
 }
 
-QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
-                    const char *optstr)
-{
-    QemuOpts *opts;
-    char buf[32];
-
-    opts = drive_def(optstr);
-    if (!opts) {
-        return NULL;
-    }
-    if (type != IF_DEFAULT) {
-        qemu_opt_set(opts, "if", if_name[type]);
-    }
-    if (index >= 0) {
-        snprintf(buf, sizeof(buf), "%d", index);
-        qemu_opt_set(opts, "index", buf);
-    }
-    if (file)
-        qemu_opt_set(opts, "file", file);
-    return opts;
-}
-
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
 {
     DriveInfo *dinfo;
diff --git a/vl.c b/vl.c
index 192a240..8b9dfed 100644
--- a/vl.c
+++ b/vl.c
@@ -21,6 +21,7 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
+#include <stdarg.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <signal.h>
@@ -623,10 +624,10 @@ static int bt_parse(const char *opt)
 
 #define HD_OPTS "media=disk"
 #define CDROM_OPTS "media=cdrom"
-#define FD_OPTS ""
-#define PFLASH_OPTS ""
-#define MTD_OPTS ""
-#define SD_OPTS ""
+#define FD_OPTS "if=floppy"
+#define PFLASH_OPTS "if=pflash"
+#define MTD_OPTS "if=mtd"
+#define SD_OPTS "if=sd"
 
 static int drive_init_func(QemuOpts *opts, void *opaque)
 {
@@ -643,21 +644,37 @@ static int drive_enable_snapshot(QemuOpts *opts, void 
*opaque)
     return 0;
 }
 
+static QemuOpts *drive_def_printf(const char *fmt, ...)
+{
+    QemuOpts *opts;
+    char *optstr;
+    va_list ap;
+
+    va_start(ap, fmt);
+
+    if (vasprintf(&optstr, fmt, ap) < 0) {
+        fprintf(stderr, "Failed to allocate memory: %s\n", strerror(errno));
+        abort();
+    }
+
+    opts = drive_def(optstr);
+
+    free(optstr);
+
+    return opts;
+}
+
 static void default_drive(int enable, int snapshot, int use_scsi,
                           BlockInterfaceType type, int index,
                           const char *optstr)
 {
     QemuOpts *opts;
 
-    if (type == IF_DEFAULT) {
-        type = use_scsi ? IF_SCSI : IF_IDE;
-    }
-
     if (!enable || drive_get_by_index(type, index)) {
         return;
     }
 
-    opts = drive_add(type, index, NULL, optstr);
+    opts = drive_def_printf("%s,index=%d", optstr, index);
     if (snapshot) {
         drive_enable_snapshot(opts, NULL);
     }
@@ -2130,7 +2147,7 @@ int main(int argc, char **argv, char **envp)
         if (optind >= argc)
             break;
         if (argv[optind][0] != '-') {
-           hda_opts = drive_add(IF_DEFAULT, 0, argv[optind++], HD_OPTS);
+           hda_opts = drive_def_printf("%s,file=%s", HD_OPTS, argv[optind++]);
         } else {
             const QEMUOption *popt;
 
@@ -2170,25 +2187,24 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_hda:
                 {
-                    char buf[256];
-                    if (cyls == 0)
-                        snprintf(buf, sizeof(buf), "%s", HD_OPTS);
-                    else
-                        snprintf(buf, sizeof(buf),
-                                 "%s,cyls=%d,heads=%d,secs=%d%s",
-                                 HD_OPTS , cyls, heads, secs,
+                    char chsopts[256] = "";
+                    if (cyls != 0)
+                        snprintf(chsopts, sizeof(chsopts),
+                                 ",cyls=%d,heads=%d,secs=%d%s",
+                                 cyls, heads, secs,
                                  translation == BIOS_ATA_TRANSLATION_LBA ?
                                  ",trans=lba" :
                                  translation == BIOS_ATA_TRANSLATION_NONE ?
                                  ",trans=none" : "");
-                    drive_add(IF_DEFAULT, 0, optarg, buf);
+                    drive_def_printf("%s%s,index=0,file=%s",
+                                     HD_OPTS, chsopts, optarg);
                     break;
                 }
             case QEMU_OPTION_hdb:
             case QEMU_OPTION_hdc:
             case QEMU_OPTION_hdd:
-                drive_add(IF_DEFAULT, popt->index - QEMU_OPTION_hda, optarg,
-                          HD_OPTS);
+                drive_def_printf("%s,index=%d,file=%s", HD_OPTS,
+                                 popt->index - QEMU_OPTION_hda, optarg);
                 break;
             case QEMU_OPTION_drive:
                 drive_def(optarg);
@@ -2202,13 +2218,13 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                break;
             case QEMU_OPTION_mtdblock:
-                drive_add(IF_MTD, -1, optarg, MTD_OPTS);
+                drive_def_printf("%s,file=%s", MTD_OPTS, optarg);
                 break;
             case QEMU_OPTION_sd:
-                drive_add(IF_SD, 0, optarg, SD_OPTS);
+                drive_def_printf("%s,file=%s", SD_OPTS, optarg);
                 break;
             case QEMU_OPTION_pflash:
-                drive_add(IF_PFLASH, -1, optarg, PFLASH_OPTS);
+                drive_def_printf("%s,file=%s", PFLASH_OPTS, optarg);
                 break;
             case QEMU_OPTION_snapshot:
                 snapshot = 1;
@@ -2293,7 +2309,7 @@ int main(int argc, char **argv, char **envp)
                 kernel_cmdline = optarg;
                 break;
             case QEMU_OPTION_cdrom:
-                drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
+                drive_def_printf("%s,index=2,file=%s", CDROM_OPTS, optarg);
                 break;
             case QEMU_OPTION_boot:
                 {
@@ -2346,8 +2362,8 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_fda:
             case QEMU_OPTION_fdb:
-                drive_add(IF_FLOPPY, popt->index - QEMU_OPTION_fda,
-                          optarg, FD_OPTS);
+                drive_def_printf("%s,index=%d,file=%s", FD_OPTS,
+                                 popt->index - QEMU_OPTION_fda, optarg);
                 break;
             case QEMU_OPTION_no_fd_bootchk:
                 fd_bootchk = 0;


-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson