[Qemu-devel] [QEMU-1.6 PATCH] vl.c: Output error on invalid machine type provided

[Qemu-devel] [QEMU-1.6 PATCH] vl.c: Output error on invalid machine type provided

[Michal Novotny]

Output error message using qemu's error_report() function when user
provides the invalid machine type on the command line. This also saves
time to find what issue is when you downgrade from one version of qemu
to another that doesn't support required machine type yet (the version
user downgraded to have to have this patch applied too, of course).

Signed-off-by: Michal Novotny <minovotn@redhat.com>
---
 vl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/vl.c b/vl.c
index f422a1c..9b4a3f9 100644
--- a/vl.c
+++ b/vl.c
@@ -2671,6 +2671,11 @@ static QEMUMachine *machine_parse(const char *name)
     if (machine) {
         return machine;
     }
+
+    if (name && !is_help_option(name)) {
+        error_report("Unsupported machine type");
+    }
+
     printf("Supported machines are:\n");
     for (m = first_machine; m != NULL; m = m->next) {
         if (m->alias) {
-- 
1.7.11.7

Re: [Qemu-devel] [QEMU-1.6 PATCH] vl.c: Output error on invalid machine type provided

[Eric Blake]

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

On 08/12/2013 10:28 AM, Michal Novotny wrote:
> Output error message using qemu's error_report() function when user
> provides the invalid machine type on the command line. This also saves
> time to find what issue is when you downgrade from one version of qemu
> to another that doesn't support required machine type yet (the version
> user downgraded to have to have this patch applied too, of course).

s/have to have/has to have/

> 
> Signed-off-by: Michal Novotny <minovotn@redhat.com>
> ---
>  vl.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

(It helps to add a v6 or repost nomenclature in the subject line, when
pointing out that this is a repost of a v5 [1]; also, when the patch is
unchanged from an already-reviewed earlier version, it is recommended
that you amend the message to include Reviewed-by lines carried forward,
rather than waiting for reviewers to repeat their comments [2])

[1] https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg01589.html
[2] http://wiki.qemu.org/Contribute/SubmitAPatch

-- 
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: 621 bytes --]

Re: [Qemu-devel] [QEMU-1.6 PATCH] vl.c: Output error on invalid machine type provided

[Michal Novotny]

On 08/12/2013 06:47 PM, Eric Blake wrote:
> On 08/12/2013 10:28 AM, Michal Novotny wrote:
>> Output error message using qemu's error_report() function when user
>> provides the invalid machine type on the command line. This also saves
>> time to find what issue is when you downgrade from one version of qemu
>> to another that doesn't support required machine type yet (the version
>> user downgraded to have to have this patch applied too, of course).
> s/have to have/has to have/
>
>> Signed-off-by: Michal Novotny <minovotn@redhat.com>
>> ---
>>  vl.c | 5 +++++
>>  1 file changed, 5 insertions(+)
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> (It helps to add a v6 or repost nomenclature in the subject line, when
> pointing out that this is a repost of a v5 [1]; also, when the patch is
> unchanged from an already-reviewed earlier version, it is recommended
> that you amend the message to include Reviewed-by lines carried forward,
> rather than waiting for reviewers to repeat their comments [2])
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2013-08/msg01589.html
> [2] http://wiki.qemu.org/Contribute/SubmitAPatch

Ok, thanks for your feedback and advice Eric :-)

Michal
>

-- 
Michal Novotny <minovotn@redhat.com>, RHCE, Red Hat
Virtualization | libvirt-php bindings | php-virt-control.org

Re: [Qemu-devel] [QEMU-1.6 PATCH] vl.c: Output error on invalid machine type provided

[Marcel Apfelbaum]

On Mon, 2013-08-12 at 18:28 +0200, Michal Novotny wrote:
> Output error message using qemu's error_report() function when user
> provides the invalid machine type on the command line. This also saves
> time to find what issue is when you downgrade from one version of qemu
> to another that doesn't support required machine type yet (the version
> user downgraded to have to have this patch applied too, of course).
> 
> Signed-off-by: Michal Novotny <minovotn@redhat.com>
> ---
>  vl.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>

> diff --git a/vl.c b/vl.c
> index f422a1c..9b4a3f9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2671,6 +2671,11 @@ static QEMUMachine *machine_parse(const char *name)
>      if (machine) {
>          return machine;
>      }
> +
> +    if (name && !is_help_option(name)) {
> +        error_report("Unsupported machine type");
> +    }
> +
>      printf("Supported machines are:\n");
>      for (m = first_machine; m != NULL; m = m->next) {
>          if (m->alias) {

Re: [Qemu-devel] [QEMU-1.6 PATCH] vl.c: Output error on invalid machine type provided

[Michal Novotny]

Ping? There are reviews already? Anybody to apply it?

Michal

On 08/12/2013 06:34 PM, Michal Novotny wrote:
> Output error message using qemu's error_report() function when user
> provides the invalid machine type on the command line. This also saves
> time to find what issue is when you downgrade from one version of qemu
> to another that doesn't support required machine type yet (the version
> user downgraded to have to have this patch applied too, of course).
>
> Signed-off-by: Michal Novotny <minovotn@redhat.com>
> ---
>  vl.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/vl.c b/vl.c
> index f422a1c..9b4a3f9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2671,6 +2671,11 @@ static QEMUMachine *machine_parse(const char *name)
>      if (machine) {
>          return machine;
>      }
> +
> +    if (name && !is_help_option(name)) {
> +        error_report("Unsupported machine type");
> +    }
> +
>      printf("Supported machines are:\n");
>      for (m = first_machine; m != NULL; m = m->next) {
>          if (m->alias) {

-- 
Michal Novotny <minovotn@redhat.com>, RHCE, Red Hat
Virtualization | libvirt-php bindings | php-virt-control.org

Re: [Qemu-devel] [QEMU-1.6 PATCH] vl.c: Output error on invalid machine type provided

[Andreas Färber]

Am 23.08.2013 17:52, schrieb Michal Novotny:
> Ping? There are reviews already? Anybody to apply it?

There is no submaintainer for vl.c, so it must go through Anthony.
Anthony uses the patches tool for such patches and there is an
unresolved review comment from Eric, so please respin.

Following Eric's remarks it should be [PATCH v6] then (this one
should've been [PATCH for-1.6 v5]).

Additionally...

> On 08/12/2013 06:34 PM, Michal Novotny wrote:
>> Output error message using qemu's error_report() function when user

"QEMU's" (or shorter: "using error_report() when")

>> provides the invalid machine type on the command line. This also saves
>> time to find what issue is when you downgrade from one version of qemu

"what the issue is", "QEMU"

>> to another that doesn't support required machine type yet (the version
>> user downgraded to have to have this patch applied too, of course).

If you want to have this patch backported to 1.6 and (with
error_report() replaced) earlier versions then you need to add a line
"Cc: qemu-stable@nongnu.org" to the commit message.

>>
>> Signed-off-by: Michal Novotny <minovotn@redhat.com>
>> ---
>>  vl.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/vl.c b/vl.c
>> index f422a1c..9b4a3f9 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2671,6 +2671,11 @@ static QEMUMachine *machine_parse(const char *name)
>>      if (machine) {
>>          return machine;
>>      }
>> +
>> +    if (name && !is_help_option(name)) {
>> +        error_report("Unsupported machine type");

Not seeing a change log below --- nor remembering it, was name
intentionally not incorporated into the error message via '%s'? I'd
consider that handy when the person getting the error is not the one
typing the command, such as libvirt. Either way this is an improvement,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Regards,
Andreas

>> +    }
>> +
>>      printf("Supported machines are:\n");
>>      for (m = first_machine; m != NULL; m = m->next) {
>>          if (m->alias) {

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [QEMU-1.6 PATCH] vl.c: Output error on invalid machine type provided

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 23.08.2013 17:52, schrieb Michal Novotny:
>> Ping? There are reviews already? Anybody to apply it?
>
> There is no submaintainer for vl.c, so it must go through Anthony.
> Anthony uses the patches tool for such patches and there is an
> unresolved review comment from Eric, so please respin.
>
> Following Eric's remarks it should be [PATCH v6] then (this one
> should've been [PATCH for-1.6 v5]).
>
> Additionally...
>
>> On 08/12/2013 06:34 PM, Michal Novotny wrote:
>>> Output error message using qemu's error_report() function when user
>
> "QEMU's" (or shorter: "using error_report() when")
>
>>> provides the invalid machine type on the command line. This also saves
>>> time to find what issue is when you downgrade from one version of qemu
>
> "what the issue is", "QEMU"
>
>>> to another that doesn't support required machine type yet (the version
>>> user downgraded to have to have this patch applied too, of course).
>
> If you want to have this patch backported to 1.6 and (with
> error_report() replaced) earlier versions then you need to add a line
> "Cc: qemu-stable@nongnu.org" to the commit message.
>
>>>
>>> Signed-off-by: Michal Novotny <minovotn@redhat.com>
>>> ---
>>>  vl.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index f422a1c..9b4a3f9 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -2671,6 +2671,11 @@ static QEMUMachine *machine_parse(const char *name)
>>>      if (machine) {
>>>          return machine;
>>>      }
>>> +
>>> +    if (name && !is_help_option(name)) {
>>> +        error_report("Unsupported machine type");
>
> Not seeing a change log below --- nor remembering it, was name
> intentionally not incorporated into the error message via '%s'? I'd
> consider that handy when the person getting the error is not the one
> typing the command, such as libvirt. Either way this is an improvement,

Message looks like this:

qemu-system-x86_64: -M HAL-9000: Unsupported machine type

> Reviewed-by: Andreas Färber <afaerber@suse.de>
>
> Regards,
> Andreas
>
>>> +    }
>>> +
>>>      printf("Supported machines are:\n");
>>>      for (m = first_machine; m != NULL; m = m->next) {
>>>          if (m->alias) {

The list of supported machines can scroll the error message right off
the screen.  I'd print just "Use '-M help' to list supported machines"
after the error.  Can be done on top, of course.

Re: [Qemu-devel] [QEMU-1.6 & QEMU-Upstream PATCH] vl.c: Output error on invalid machine type provided

[Daniel P. Berrange]

On Thu, Sep 05, 2013 at 01:36:09PM +0200, Michal Novotny wrote:
> Output error message using qemu's error_report() function when user
> provides the invalid machine type on the command line. This also saves
> time to find what issue is when you downgrade from one version of qemu
> to another that doesn't support required machine type yet (the version
> user downgraded to have to have this patch applied too, of course).
> 
> (This has been posted a while ago and reviewed however not applied yet
>  so this is basically just a reminder e-mail to ask for pushing it.
>  It also applies cleanly to QEMU-1.6 so I'm sending to qemu-stable as
>  well.)
> 
> Signed-off-by: Michal Novotny <minovotn@redhat.com>
> ---
>  vl.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/vl.c b/vl.c
> index f422a1c..9b4a3f9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2671,6 +2671,11 @@ static QEMUMachine *machine_parse(const char *name)
>      if (machine) {
>          return machine;
>      }
> +
> +    if (name && !is_help_option(name)) {
> +        error_report("Unsupported machine type");
> +    }

It would be nicer to do

     error_report("Unsupported machine type '%s'", name);


So when people report error messages in bugs / mailing lists, without
providing their CLI args, it'll be obvious what they requested.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [QEMU-1.6 & QEMU-Upstream PATCH] vl.c: Output error on invalid machine type provided

[Michal Novotny]

On 09/05/2013 01:39 PM, Daniel P. Berrange wrote:
> On Thu, Sep 05, 2013 at 01:36:09PM +0200, Michal Novotny wrote:
>> Output error message using qemu's error_report() function when user
>> provides the invalid machine type on the command line. This also saves
>> time to find what issue is when you downgrade from one version of qemu
>> to another that doesn't support required machine type yet (the version
>> user downgraded to have to have this patch applied too, of course).
>>
>> (This has been posted a while ago and reviewed however not applied yet
>>  so this is basically just a reminder e-mail to ask for pushing it.
>>  It also applies cleanly to QEMU-1.6 so I'm sending to qemu-stable as
>>  well.)
>>
>> Signed-off-by: Michal Novotny <minovotn@redhat.com>
>> ---
>>  vl.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/vl.c b/vl.c
>> index f422a1c..9b4a3f9 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2671,6 +2671,11 @@ static QEMUMachine *machine_parse(const char *name)
>>      if (machine) {
>>          return machine;
>>      }
>> +
>> +    if (name && !is_help_option(name)) {
>> +        error_report("Unsupported machine type");
>> +    }
> It would be nicer to do
>
>      error_report("Unsupported machine type '%s'", name);

No need Daniel, as the output is having this already (as mentioned by
Markus in the previous thread):

$ ./x86_64-softmmu/qemu-system-x86_64 -M bogus
-M bogus: Unsupported machine type
$

So the CLI arguments are in the beginning of the line already. If we
consider long CLI args, like:

$ ./x86_64-softmmu/qemu-system-x86_64 -S -M bogus -cpu
core2duo,+lahf_lm,+rdtscp,+avx,+osxsave,+xsave,+aes,+tsc-deadline,+popcnt,+x2apic,+sse4.2,+sse4.1,+pdcm,+xtpr,+cx16,+tm2,+est,+smx,+vmx,+ds_cpl,+dtes64,+pclmuldq,+pbe,+tm,+ht,+ss,+acpi,+ds
-enable-kvm -m 1024 -smp 1,sockets=1,cores=1,threads=1 -name debian
-uuid 374677db-d9b1-3326-a097-5f2b79d3fca0 -nodefconfig -nodefaults
-chardev
socket,id=charmonitor,path=/var/lib/libvirt/qemu/debian.monitor,server,nowait
-mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc
-no-shutdown -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2
-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 -device
lsi,id=scsi0,bus=pci.0,addr=0x7 -drive
if=none,media=cdrom,id=drive-ide0-1-0,readonly=on,format=raw -device
ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 -drive
file=/home/mig/images/kvm/debian.qcow2,if=none,id=drive-virtio-disk0,format=qcow2
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
-netdev tap,fd=20,id=hostnet0 -device
rtl8139,netdev=hostnet0,id=net0,mac=52:54:00:4d:d9:c9,bus=pci.0,addr=0x3
-chardev pty,id=charserial0 -device
isa-serial,chardev=charserial0,id=serial0 -chardev
spicevmc,id=charchannel0,name=vdagent -device
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0
-device usb-tablet,id=input0 -spice
port=5900,addr=127.0.0.1,disable-ticketing -vga qxl -global
qxl-vga.vram_size=67108864 -device
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6

then it can make sense, however to change it from:

-S -M bogus -cpu
core2duo,+lahf_lm,+rdtscp,+avx,+osxsave,+xsave,+aes,+tsc-deadline,+popcnt,+x2apic,+sse4.2,+sse4.1,+pdcm,+xtpr,+cx16,+tm2,+est,+smx,+vmx,+ds_cpl,+dtes64,+pclmuldq,+pbe,+tm,+ht,+ss,+acpi,+ds
-enable-kvm -m 1024 -smp 1,sockets=1,cores=1,threads=1 -name debian
-uuid 374677db-d9b1-3326-a097-5f2b79d3fca0 -nodefconfig -nodefaults
-chardev
socket,id=charmonitor,path=/var/lib/libvirt/qemu/debian.monitor,server,nowait
-mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc
-no-shutdown -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2
-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 -device
lsi,id=scsi0,bus=pci.0,addr=0x7 -drive
if=none,media=cdrom,id=drive-ide0-1-0,readonly=on,format=raw -device
ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 -drive
file=/home/mig/images/kvm/debian.qcow2,if=none,id=drive-virtio-disk0,format=qcow2
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
-netdev tap,fd=20,id=hostnet0 -device
rtl8139,netdev=hostnet0,id=net0,mac=52:54:00:4d:d9:c9,bus=pci.0,addr=0x3
-chardev pty,id=charserial0 -device
isa-serial,chardev=charserial0,id=serial0 -chardev
spicevmc,id=charchannel0,name=vdagent -device
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0
-device usb-tablet,id=input0 -spice
port=5900,addr=127.0.0.1,disable-ticketing -vga qxl -global
qxl-vga.vram_size=67108864 -device
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6: Unsupported machine type

to:

-S -M bogus -cpu
core2duo,+lahf_lm,+rdtscp,+avx,+osxsave,+xsave,+aes,+tsc-deadline,+popcnt,+x2apic,+sse4.2,+sse4.1,+pdcm,+xtpr,+cx16,+tm2,+est,+smx,+vmx,+ds_cpl,+dtes64,+pclmuldq,+pbe,+tm,+ht,+ss,+acpi,+ds
-enable-kvm -m 1024 -smp 1,sockets=1,cores=1,threads=1 -name debian
-uuid 374677db-d9b1-3326-a097-5f2b79d3fca0 -nodefconfig -nodefaults
-chardev
socket,id=charmonitor,path=/var/lib/libvirt/qemu/debian.monitor,server,nowait
-mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc
-no-shutdown -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2
-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x4 -device
lsi,id=scsi0,bus=pci.0,addr=0x7 -drive
if=none,media=cdrom,id=drive-ide0-1-0,readonly=on,format=raw -device
ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 -drive
file=/home/mig/images/kvm/debian.qcow2,if=none,id=drive-virtio-disk0,format=qcow2
-device
virtio-blk-pci,scsi=off,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
-netdev tap,fd=20,id=hostnet0 -device
rtl8139,netdev=hostnet0,id=net0,mac=52:54:00:4d:d9:c9,bus=pci.0,addr=0x3
-chardev pty,id=charserial0 -device
isa-serial,chardev=charserial0,id=serial0 -chardev
spicevmc,id=charchannel0,name=vdagent -device
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0
-device usb-tablet,id=input0 -spice
port=5900,addr=127.0.0.1,disable-ticketing -vga qxl -global
qxl-vga.vram_size=67108864 -device
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x6: Unsupported machine
type 'bogus'

In this case, however, I would like to prefer not to show all the
arguments and to stick with standalone printf() printing just:

-M bogus: Unsupported machine type

using:

fprintf(stderr, "-M %s: Unsupported machine type\n", type);

What do you think?

Michal
>
>
> So when people report error messages in bugs / mailing lists, without
> providing their CLI args, it'll be obvious what they requested.
>
> Daniel

-- 
Michal Novotny <minovotn@redhat.com>, RHCE, Red Hat
Virtualization | libvirt-php bindings | php-virt-control.org