Re: [Qemu-devel] [PATCH] Fix double backslash problem in Windows

[Johannes Schindelin]

Hi,

On Wed, 9 Jan 2008, Laurent Vivier wrote:

> Le mercredi 09 janvier 2008 à 12:27 +0000, Johannes Schindelin a écrit :
> 
> > On Wed, 9 Jan 2008, Laurent Vivier wrote:
> > 
> > > but "-hda" is an alias for "-drive file="%s",index=%d,media=disk".
> > 
> > It appears to me as if "-hda" is implemented suboptimally, then.  In 
> > particular, drive_add() should be able to get a separate "file" 
> > parameter, which can be overridden by the "fmt" parameter.  Of course, 
> > this would mean that the global drives_opt[] array should not have 
> > element type "char", but a struct.
> 
> This introduces complexity and special cases I don't want to manage...

The problem is that you introduced a regression, as you can see by the 
size of this thread.

> > It is a pity that there is DWIMery going on in drive_init(), needing the 
> > other options to be parsed already, otherwise drive_add() could have been 
> > replaced by calls to drive_init().
> 
> I just mimic here already existing behavior of qemu.
> drive_init() already parse parameter to extract options.
> You can't make drive_init() before having parsed all parameters because
> of parameters like "-hda f -hdachs x,y,z"

But it feels wrong to parse one option, unparse it into another option, 
and then reparse it again (with all problems introduced by the then-needed 
escaping).

Besides, it would not be _that_ complicated:

-- snipsnap --
[PATCH] make special escaping for -hda parameters unnecessary

Signed-off-by: Johannes Schindelin <address@hidden>

---

 vl.c |   64 ++++++++++++++++++++++++++++++++++++----------------------------
 1 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/vl.c b/vl.c
index 8e346fe..c9966d1 100644
--- a/vl.c
+++ b/vl.c
@@ -231,7 +231,10 @@ unsigned int nb_prom_envs = 0;
 const char *prom_envs[MAX_PROM_ENVS];
 #endif
 int nb_drives_opt;
-char drives_opt[MAX_DRIVES][1024];
+struct drive_opt {
+       const char *file;
+       char opt[1024];
+} drives_opt[MAX_DRIVES];
 
 static CPUState *cur_cpu;
 static CPUState *next_cpu;
@@ -4859,18 +4862,18 @@ void do_info_network(void)
     }
 }
 
-#define HD_ALIAS "file=\"%s\",index=%d,media=disk"
+#define HD_ALIAS "index=%d,media=disk"
 #ifdef TARGET_PPC
 #define CDROM_ALIAS "index=1,media=cdrom"
 #else
 #define CDROM_ALIAS "index=2,media=cdrom"
 #endif
 #define FD_ALIAS "index=%d,if=floppy"
-#define PFLASH_ALIAS "file=\"%s\",if=pflash"
-#define MTD_ALIAS "file=\"%s\",if=mtd"
+#define PFLASH_ALIAS "if=pflash"
+#define MTD_ALIAS "if=mtd"
 #define SD_ALIAS "index=0,if=sd"
 
-static int drive_add(const char *fmt, ...)
+static int drive_add(const char *file, const char *fmt, ...)
 {
     va_list ap;
 
@@ -4879,8 +4882,9 @@ static int drive_add(const char *fmt, ...)
         exit(1);
     }
 
+    drives_opt[nb_drives_opt].file = file;
     va_start(ap, fmt);
-    vsnprintf(drives_opt[nb_drives_opt], sizeof(drives_opt[0]), fmt, ap);
+    vsnprintf(drives_opt[nb_drives_opt].opt, sizeof(drives_opt[0]), fmt, ap);
     va_end(ap);
 
     return nb_drives_opt++;
@@ -4915,12 +4919,13 @@ int drive_get_max_bus(BlockInterfaceType type)
     return max_bus;
 }
 
-static int drive_init(const char *str, int snapshot, QEMUMachine *machine)
+static int drive_init(struct drive_opt *o, int snapshot, QEMUMachine *machine)
 {
     char buf[128];
     char file[1024];
     char devname[128];
     const char *mediastr = "";
+    const char *str = o->opt;
     BlockInterfaceType type;
     enum { MEDIA_DISK, MEDIA_CDROM } media;
     int bus_id, unit_id;
@@ -4940,7 +4945,11 @@ static int drive_init(const char *str, int snapshot, 
QEMUMachine *machine)
          return -1;
     }
 
-    file[0] = 0;
+    if (o->file) {
+       strncpy(file, o->file, sizeof(file) - 1);
+       file[sizeof(file) - 1] = 0;
+    } else
+        file[0] = 0;
     cyls = heads = secs = 0;
     bus_id = 0;
     unit_id = -1;
@@ -8212,7 +8221,7 @@ int main(int argc, char **argv)
             break;
         r = argv[optind];
         if (r[0] != '-') {
-           hda_index = drive_add(HD_ALIAS, argv[optind++], 0);
+           hda_index = drive_add(argv[optind++], HD_ALIAS, 0);
         } else {
             const QEMUOption *popt;
 
@@ -8273,11 +8282,11 @@ int main(int argc, char **argv)
                 break;
             case QEMU_OPTION_hda:
                 if (cyls == 0)
-                    hda_index = drive_add(HD_ALIAS, optarg, 0);
+                    hda_index = drive_add(optarg, HD_ALIAS, 0);
                 else
-                    hda_index = drive_add(HD_ALIAS
+                    hda_index = drive_add(optarg, HD_ALIAS
                             ",cyls=%d,heads=%d,secs=%d%s",
-                             optarg, 0, cyls, heads, secs,
+                             0, cyls, heads, secs,
                              translation == BIOS_ATA_TRANSLATION_LBA ?
                                  ",trans=lba" :
                              translation == BIOS_ATA_TRANSLATION_NONE ?
@@ -8286,19 +8295,19 @@ int main(int argc, char **argv)
             case QEMU_OPTION_hdb:
             case QEMU_OPTION_hdc:
             case QEMU_OPTION_hdd:
-               drive_add(HD_ALIAS, optarg, popt->index - QEMU_OPTION_hda);
+               drive_add(optarg, HD_ALIAS, popt->index - QEMU_OPTION_hda);
                 break;
             case QEMU_OPTION_drive:
-                drive_add("%s", optarg);
+                drive_add(optarg, "");
                break;
             case QEMU_OPTION_mtdblock:
-               drive_add(MTD_ALIAS, optarg);
+               drive_add(optarg, MTD_ALIAS);
                 break;
             case QEMU_OPTION_sd:
-                drive_add("file=\"%s\"," SD_ALIAS, optarg);
+                drive_add(optarg, SD_ALIAS);
                 break;
             case QEMU_OPTION_pflash:
-               drive_add(PFLASH_ALIAS, optarg);
+               drive_add(optarg, PFLASH_ALIAS);
                 break;
             case QEMU_OPTION_snapshot:
                 snapshot = 1;
@@ -8338,10 +8347,10 @@ int main(int argc, char **argv)
                         exit(1);
                     }
                    if (hda_index != -1)
-                       snprintf(drives_opt[hda_index] +
-                                strlen(drives_opt[hda_index]),
-                                sizeof(drives_opt[0]) -
-                                strlen(drives_opt[hda_index]),
+                       snprintf(drives_opt[hda_index].opt +
+                                strlen(drives_opt[hda_index].opt),
+                                sizeof(drives_opt[0].opt) -
+                                strlen(drives_opt[hda_index].opt),
                                 ",cyls=%d,heads=%d,secs=%d%s",
                                 cyls, heads, secs,
                                 translation == BIOS_ATA_TRANSLATION_LBA ?
@@ -8366,7 +8375,7 @@ int main(int argc, char **argv)
                 kernel_cmdline = optarg;
                 break;
             case QEMU_OPTION_cdrom:
-               drive_add("file=\"%s\"," CDROM_ALIAS, optarg);
+               drive_add(optarg, CDROM_ALIAS);
                 break;
             case QEMU_OPTION_boot:
                 boot_devices = optarg;
@@ -8401,8 +8410,7 @@ int main(int argc, char **argv)
                 break;
             case QEMU_OPTION_fda:
             case QEMU_OPTION_fdb:
-               drive_add("file=\"%s\"," FD_ALIAS, optarg,
-                         popt->index - QEMU_OPTION_fda);
+               drive_add(optarg, FD_ALIAS, popt->index - QEMU_OPTION_fda);
                 break;
 #ifdef TARGET_I386
             case QEMU_OPTION_no_fd_bootchk:
@@ -8871,22 +8879,22 @@ int main(int argc, char **argv)
     /* we always create the cdrom drive, even if no disk is there */
 
     if (nb_drives_opt < MAX_DRIVES)
-        drive_add(CDROM_ALIAS);
+        drive_add(NULL, CDROM_ALIAS);
 
     /* we always create at least one floppy */
 
     if (nb_drives_opt < MAX_DRIVES)
-        drive_add(FD_ALIAS, 0);
+        drive_add(NULL, FD_ALIAS, 0);
 
     /* we always create one sd slot, even if no card is in it */
 
     if (nb_drives_opt < MAX_DRIVES)
-        drive_add(SD_ALIAS);
+        drive_add(NULL, SD_ALIAS);
 
     /* open the virtual block devices */
 
     for(i = 0; i < nb_drives_opt; i++)
-        if (drive_init(drives_opt[i], snapshot, machine) == -1)
+        if (drive_init(&drives_opt[i], snapshot, machine) == -1)
            exit(1);
 
     register_savevm("timer", 0, 2, timer_save, timer_load, NULL);