| Message ID | e50a063f65dc29381ce3301d74a61d6b1de0f5bc.1376324862.git.minovotn@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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) {
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) {
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) {
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.
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
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
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) {
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(+)