From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58704) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aTw1w-0008Gi-L3 for qemu-devel@nongnu.org; Thu, 11 Feb 2016 13:31:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aTw1t-0001da-EA for qemu-devel@nongnu.org; Thu, 11 Feb 2016 13:31:52 -0500 Date: Thu, 11 Feb 2016 16:31:45 -0200 From: Eduardo Habkost Message-ID: <20160211183145.GE6239@thinpad.lan.raisama.net> References: <1454932469-27761-1-git-send-email-marcel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1454932469-27761-1-git-send-email-marcel@redhat.com> Subject: Re: [Qemu-devel] [PATCH V4] vl.c: fixed regression in machine error message List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: mst@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, qemu-stable@nongnu.org, pbonzini@redhat.com, lersek@redhat.com On Mon, Feb 08, 2016 at 01:54:29PM +0200, Marcel Apfelbaum wrote: > Commit e1ce0c3cb(vl.c: fix regression when reading machine type from config file) > fixed the error message when the machine type was supplied inside the > config file. However now the option name is not displayed correctly if > the error happens when the machine is specified at command line. > > Running > ./x86_64-softmmu/qemu-system-x86_64 -M q35-1.5 -redir tcp:8022::22 > will result in the error message: > qemu-system-x86_64: -redir tcp:8022::22: unsupported machine type > Use -machine help to list supported machines > > Fixed it by restoring the error location and also extracted the code > dealing with machine options into a separate function. > > Reported-by: Michael S. Tsirkin > Reviewed-by: Laszlo Ersek > Signed-off-by: Marcel Apfelbaum > --- > > v3 -> v4: > - removed double call of loc_push_none (thanks again Laszlo) > > v2 -> v3: > - fixed commit message and called qemu_get_machine_opts only once. (thanks Laszlo) > > v1 -> v2: > - Addressed Laszlo Ersek's comments: > - no need to save the machine options location, is saved in opts > - rename the extracted method to set_machine_options > - added the bug reporter to the CC > > - tested with and without the config file and the error message is now OK: > config file: > - qemu-system-x86_64:machine-bug.conf:3: unsupported machine type > cli: > - qemu-system-x86_64: -M q35-1.5: unsupported machine type > > Thanks, > Marcel > > vl.c | 38 +++++++++++++++++++++++++++----------- > 1 file changed, 27 insertions(+), 11 deletions(-) > > diff --git a/vl.c b/vl.c > index 2c03f54..5e22a35 100644 > --- a/vl.c > +++ b/vl.c > @@ -2748,6 +2748,31 @@ static const QEMUOption *lookup_opt(int argc, char **argv, > return popt; > } > > +static void set_machine_options(MachineClass **machine_class) > +{ > + const char *optarg; > + QemuOpts *opts; > + Location loc; > + > + loc_push_none(&loc); > + > + opts = qemu_get_machine_opts(); > + qemu_opts_loc_restore(opts); > + > + optarg = qemu_opt_get(opts, "type"); > + if (optarg) { > + *machine_class = machine_parse(optarg); > + } > + > + if (*machine_class == NULL) { > + error_report("No machine specified, and there is no default"); > + error_printf("Use -machine help to list supported machines\n"); > + exit(1); > + } > + > + loc_pop(&loc); > +} > + > static int machine_set_property(void *opaque, > const char *name, const char *value, > Error **errp) > @@ -4028,17 +4053,7 @@ int main(int argc, char **argv, char **envp) > > replay_configure(icount_opts); > > - opts = qemu_get_machine_opts(); > - optarg = qemu_opt_get(opts, "type"); > - if (optarg) { > - machine_class = machine_parse(optarg); > - } > - > - if (machine_class == NULL) { > - error_report("No machine specified, and there is no default"); > - error_printf("Use -machine help to list supported machines\n"); > - exit(1); > - } > + set_machine_options(&machine_class); > > set_memory_options(&ram_slots, &maxram_size, machine_class); [Extra context for reference:] > > loc_set_none(); You are fixing the machine error message, but not the root cause: $ ./x86_64-softmmu/qemu-system-x86_64 -m size= -vnc :0 qemu-system-x86_64: -vnc :0: missing 'size' option value $ ./x86_64-softmmu/qemu-system-x86_64 -icount rr=x -vnc :0 qemu-system-x86_64: -vnc :0: Invalid icount rr option: x Moving the loc_set_none() call above replay_configure() should fix the bug in the three cases. Setting location in set_machine_options(), set_memory_options(), and replay_configure() would be nice, too. But I would prefer to apply a one-line fix first, and then consider improving the machine/memory/icount error messages. -- Eduardo