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 <[EMAIL PROTECTED]> --- 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);