| Message ID | 1430418196-8691-1-git-send-email-dslutz@verizon.com |
|---|---|
| State | New |
| Headers | show |
[adding qemu-block] On 04/30/2015 12:23 PM, Don Slutz wrote: > ~/qemu/out/master/x86_64-softmmu/qemu-system-x86_64 -h | head > QEMU emulator version 2.3.50, Copyright (c) 2003-2008 Fabrice Bellard > usage: qemu-system-x86_64 [options] [disk_image] > > 'disk_image' is a raw hard disk image for IDE hard disk 0 > > Standard options: > ... > > Signed-off-by: Don Slutz <dslutz@verizon.com> > --- > vl.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Eric Blake <eblake@redhat.com> Without this, qemu will try to probe formats. It is arguably is more convenient when using the shorthand of supplying a disk image to let qemu pick the format; but as the --help text says the image is raw, it's better to be explicit and avoid probing. Besides, we can always use longhand to specify actual format and/or probing if we don't like the shorthand. > > diff --git a/vl.c b/vl.c > index 74c2681..93e674f 100644 > --- a/vl.c > +++ b/vl.c > @@ -1084,6 +1084,7 @@ static int cleanup_add_fd(QemuOpts *opts, void *opaque) > /* QEMU Block devices */ > > #define HD_OPTS "media=disk" > +#define HD_OPTS_RAW "media=disk,format=raw" > #define CDROM_OPTS "media=cdrom" > #define FD_OPTS "" > #define PFLASH_OPTS "" > @@ -2862,7 +2863,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_add(IF_DEFAULT, 0, argv[optind++], HD_OPTS_RAW); > } else { > const QEMUOption *popt; > >
On 04/30/2015 01:15 PM, Eric Blake wrote: > [adding qemu-block] > > On 04/30/2015 12:23 PM, Don Slutz wrote: >> ~/qemu/out/master/x86_64-softmmu/qemu-system-x86_64 -h | head >> QEMU emulator version 2.3.50, Copyright (c) 2003-2008 Fabrice Bellard >> usage: qemu-system-x86_64 [options] [disk_image] >> >> 'disk_image' is a raw hard disk image for IDE hard disk 0 Oh, and subject line is long. I might have done: vl.c: Force 'disk_image' to be raw to match help text
Am 30.04.2015 um 21:15 hat Eric Blake geschrieben: > [adding qemu-block] > > On 04/30/2015 12:23 PM, Don Slutz wrote: > > ~/qemu/out/master/x86_64-softmmu/qemu-system-x86_64 -h | head > > QEMU emulator version 2.3.50, Copyright (c) 2003-2008 Fabrice Bellard > > usage: qemu-system-x86_64 [options] [disk_image] > > > > 'disk_image' is a raw hard disk image for IDE hard disk 0 > > > > Standard options: > > ... > > > > Signed-off-by: Don Slutz <dslutz@verizon.com> > > --- > > vl.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Without this, qemu will try to probe formats. It is arguably is more > convenient when using the shorthand of supplying a disk image to let > qemu pick the format; but as the --help text says the image is raw, it's > better to be explicit and avoid probing. Besides, we can always use > longhand to specify actual format and/or probing if we don't like the > shorthand. I don't think you can take an outdated help text as a justification for breaking backwards compatibility. Kevin > > > > diff --git a/vl.c b/vl.c > > index 74c2681..93e674f 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -1084,6 +1084,7 @@ static int cleanup_add_fd(QemuOpts *opts, void *opaque) > > /* QEMU Block devices */ > > > > #define HD_OPTS "media=disk" > > +#define HD_OPTS_RAW "media=disk,format=raw" > > #define CDROM_OPTS "media=cdrom" > > #define FD_OPTS "" > > #define PFLASH_OPTS "" > > @@ -2862,7 +2863,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_add(IF_DEFAULT, 0, argv[optind++], HD_OPTS_RAW); > > } else { > > const QEMUOption *popt; > > > > > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On 04/30/15 16:15, Kevin Wolf wrote: > Am 30.04.2015 um 21:15 hat Eric Blake geschrieben: >> [adding qemu-block] >> >> On 04/30/2015 12:23 PM, Don Slutz wrote: >>> ~/qemu/out/master/x86_64-softmmu/qemu-system-x86_64 -h | head >>> QEMU emulator version 2.3.50, Copyright (c) 2003-2008 Fabrice >>> Bellard usage: qemu-system-x86_64 [options] [disk_image] >>> >>> 'disk_image' is a raw hard disk image for IDE hard disk 0 >>> >>> Standard options: ... >>> >>> Signed-off-by: Don Slutz <dslutz@verizon.com> --- vl.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> >> Without this, qemu will try to probe formats. It is arguably is >> more convenient when using the shorthand of supplying a disk >> image to let qemu pick the format; but as the --help text says >> the image is raw, it's better to be explicit and avoid probing. >> Besides, we can always use longhand to specify actual format >> and/or probing if we don't like the shorthand. > > I don't think you can take an outdated help text as a justification > for breaking backwards compatibility. > Is this a hard no, soft no, or TBD? The following is what prompted this: WARNING: Image format was not specified for '/dev/etherd/e500.1' and probing guessed raw. Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted. Specify the 'raw' format explicitly to remove the restrictions. which I would say also breaks backwards compatibility. However I do agree that this is the "right" change to have done. However there currently is no way to specify that the 'disk_image' is raw. Also the old options (-hda, -hdb, -hdc, and -hdd) all have similar issues. So do you want a more complex patch that allows the format to be specified? Only for 'disk_image'? Include -hd* ? Change the help message also? -Don Slutz > Kevin > >>> >>> diff --git a/vl.c b/vl.c index 74c2681..93e674f 100644 --- >>> a/vl.c +++ b/vl.c @@ -1084,6 +1084,7 @@ static int >>> cleanup_add_fd(QemuOpts *opts, void *opaque) /* QEMU Block >>> devices */ >>> >>> #define HD_OPTS "media=disk" +#define HD_OPTS_RAW >>> "media=disk,format=raw" #define CDROM_OPTS "media=cdrom" >>> #define FD_OPTS "" #define PFLASH_OPTS "" @@ -2862,7 +2863,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_add(IF_DEFAULT, 0, argv[optind++], >>> HD_OPTS_RAW); } else { const QEMUOption *popt; >>> >>> >> >> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt >> virtualization library http://libvirt.org >> > >
Am 01.05.2015 um 01:28 hat Don Slutz geschrieben: > On 04/30/15 16:15, Kevin Wolf wrote: > > Am 30.04.2015 um 21:15 hat Eric Blake geschrieben: > >> [adding qemu-block] > >> > >> On 04/30/2015 12:23 PM, Don Slutz wrote: > >>> ~/qemu/out/master/x86_64-softmmu/qemu-system-x86_64 -h | head > >>> QEMU emulator version 2.3.50, Copyright (c) 2003-2008 Fabrice > >>> Bellard usage: qemu-system-x86_64 [options] [disk_image] > >>> > >>> 'disk_image' is a raw hard disk image for IDE hard disk 0 > >>> > >>> Standard options: ... > >>> > >>> Signed-off-by: Don Slutz <dslutz@verizon.com> --- vl.c | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> Reviewed-by: Eric Blake <eblake@redhat.com> > >> > >> Without this, qemu will try to probe formats. It is arguably is > >> more convenient when using the shorthand of supplying a disk > >> image to let qemu pick the format; but as the --help text says > >> the image is raw, it's better to be explicit and avoid probing. > >> Besides, we can always use longhand to specify actual format > >> and/or probing if we don't like the shorthand. > > > > I don't think you can take an outdated help text as a justification > > for breaking backwards compatibility. > > > > Is this a hard no, soft no, or TBD? > > The following is what prompted this: > > WARNING: Image format was not specified for '/dev/etherd/e500.1' and > probing guessed raw. > Automatically detecting the format is dangerous for raw > images, write operations on block 0 will be restricted. > Specify the 'raw' format explicitly to remove the restrictions. > > > which I would say also breaks backwards compatibility. It doesn't, except if you consider an additional warning as breakage. Only some corner cases that you probably wouldn't ever hit intentionally changed their failure mode from 'image is opened in the wrong format when restarting qemu' to 'write to sector 0 failed'. If you don't hit the restrictions and you never specify the format explicitly, you can just ignore the warning. > However I do > agree that this is the "right" change to have done. However there > currently is no way to specify that the 'disk_image' is raw. > > Also the old options (-hda, -hdb, -hdc, and -hdd) all have similar issues. Yes, you need to use -drive for that currently. > So do you want a more complex patch that allows the format to be > specified? > > Only for 'disk_image'? > > Include -hd* ? I'm afraid that there is no nice way to improve the plain 'disk_image' case. You would have to add another option, and then it's not any better than -hda and friends any more. -hda in turn could be extended to take additional options (i.e. it becomes something like -drive with implied index=0), but then there's little reason not to use -drive. We might actually want to extent -hd* at some point, but we would probably use this as a chance to clean up the interface and remove legacy options that -drive has to maintain for compatibility. > Change the help message also? Yes, this definitely. If the help message states that it's a raw image, the help text is wrong. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 01.05.2015 um 01:28 hat Don Slutz geschrieben: [...] >> So do you want a more complex patch that allows the format to be >> specified? >> >> Only for 'disk_image'? >> >> Include -hd* ? > > I'm afraid that there is no nice way to improve the plain 'disk_image' > case. You would have to add another option, and then it's not any better > than -hda and friends any more. > > -hda in turn could be extended to take additional options (i.e. it > becomes something like -drive with implied index=0), but then there's > little reason not to use -drive. Would likely break images with ',' in their name. [...]
Am 04.05.2015 um 15:39 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 01.05.2015 um 01:28 hat Don Slutz geschrieben: > [...] > >> So do you want a more complex patch that allows the format to be > >> specified? > >> > >> Only for 'disk_image'? > >> > >> Include -hd* ? > > > > I'm afraid that there is no nice way to improve the plain 'disk_image' > > case. You would have to add another option, and then it's not any better > > than -hda and friends any more. > > > > -hda in turn could be extended to take additional options (i.e. it > > becomes something like -drive with implied index=0), but then there's > > little reason not to use -drive. > > Would likely break images with ',' in their name. A possible justification would be that -hda is a convenience option that, similar to HMP, is meant only for human users and if you use it in scripts and it changes, you get to keep both pieces. In addition, while some use cases would be affected, most of them wouldn't, even in scripts. Whether or not this justification is valid might be controversial. Kevin
diff --git a/vl.c b/vl.c index 74c2681..93e674f 100644 --- a/vl.c +++ b/vl.c @@ -1084,6 +1084,7 @@ static int cleanup_add_fd(QemuOpts *opts, void *opaque) /* QEMU Block devices */ #define HD_OPTS "media=disk" +#define HD_OPTS_RAW "media=disk,format=raw" #define CDROM_OPTS "media=cdrom" #define FD_OPTS "" #define PFLASH_OPTS "" @@ -2862,7 +2863,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_add(IF_DEFAULT, 0, argv[optind++], HD_OPTS_RAW); } else { const QEMUOption *popt;
~/qemu/out/master/x86_64-softmmu/qemu-system-x86_64 -h | head QEMU emulator version 2.3.50, Copyright (c) 2003-2008 Fabrice Bellard usage: qemu-system-x86_64 [options] [disk_image] 'disk_image' is a raw hard disk image for IDE hard disk 0 Standard options: ... Signed-off-by: Don Slutz <dslutz@verizon.com> --- vl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)