[Qemu-devel] [PATCH 1/1] vl.c: Since the help says that 'disk_image' is a raw hard disk image, pass format=raw

[Qemu-devel] [PATCH 1/1] vl.c: Since the help says that 'disk_image' is a raw hard disk image, pass format=raw

[Don Slutz]

~/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(-)

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;
 
-- 
1.8.4

Re: [Qemu-devel] [PATCH 1/1] vl.c: Since the help says that 'disk_image' is a raw hard disk image, pass format=raw

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1784 bytes --]

[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;
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/1] vl.c: Since the help says that 'disk_image' is a raw hard disk image, pass format=raw

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/1] vl.c: Since the help says that 'disk_image' is a raw hard disk image, pass format=raw

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2065 bytes --]

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
> 



[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 1/1] vl.c: Since the help says that 'disk_image' is a raw hard disk image, pass format=raw

[Don Slutz]

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
>> 
> 
> 

Re: [Qemu-devel] [PATCH 1/1] vl.c: Since the help says that 'disk_image' is a raw hard disk image, pass format=raw

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 1/1] vl.c: Since the help says that 'disk_image' is a raw hard disk image, pass format=raw

[Markus Armbruster]

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.

[...]

Re: [Qemu-devel] [PATCH 1/1] vl.c: Since the help says that 'disk_image' is a raw hard disk image, pass format=raw

[Kevin Wolf]

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