Re: [Qemu-devel] [PATCH] vl.c: Report unknown machines correctly

[Markus Armbruster]

Palmer Dabbelt <address@hidden> writes:

> I was recently typing in a QEMU command line, and ended up with
> something like
>
>     qemu-system-riscv64 -machine virt ... -M 8G
>
> which is, in retrospect, obviously incorrect: there is no "8G" machine.
> I should have typed something like
>
>     qemu-system-riscv64 -machine virt ... -m 8G
>
> but since QEMU was giving me the excessively unhelpful error message
>
>     qemu-system-riscv64: -machine virt: unsupported machine type
>     Use -machine help to list supported machines
>
> I had to spend a few minutes scratching my head to figure out what was
> going on.  For some reason I felt like I'd done that before, so I
> figured I'd take a whack at fixing the problem this time.  It turns out
> the error reporting for non-existant machines is just wrong: the invalid
> machine is detected after we've lost the argument pointer, so it just
> prints out the first instance of "-machine" instead of the one we're
> actually looking for.
>
> I've fixed this by just printing out "-machine $NAME" directly, but I
> feel like there's a better way to do this.  Specifically, my issue is
> that it always prints out "-machine" instead of "-M" -- that's actually
> a regression for users just passing a single invalid machine via "-M",
> which I assume is the more common case.
>
> I'm not sure how to do this right, though, and my flight is boarding so
> I figured I'd send this out as a way to ask the question.  I didn't have
> time to run the test suite or figure out how to add a test for this, as
> I'm assuming there's a better way to do it.
>
> Signed-off-by: Palmer Dabbelt <address@hidden>
> ---
>  vl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/vl.c b/vl.c
> index 630f5c5e9c..821a5d91c8 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2487,7 +2487,7 @@ static MachineClass *machine_parse(const char *name, 
> GSList *machines)
>  
>      mc = find_machine(name, machines);
>      if (!mc) {
> -        error_report("unsupported machine type");
> +        error_printf("-machine %s: unsupported machine type\n", name);
>          error_printf("Use -machine help to list supported machines\n");
>          exit(1);
>      }

This is an instance of QemuOpts features undermining each other.

error_report() uses the "current location".  A few judiciously placed
loc_FOO() ensure many error_report() just work.  For instance, anything
within main()'s two loops over argv[] has the current location point to
the current option or argument.

= QemuOpts feature #1: capture option location =

A struct QemuOpts stores a complex option for later use.  Since many
such later uses call error_report(), it captures the option's location,
so you can make the current location point to it again.  It's even
automatic with qemu_opts_foreach().

= QemuOpts feature #2: option merging =

You can request multiple options to be merged[*] by putting .merge_lists
= true into the QemuOptsList.  qemu_machine_opts does.  -machine a=b
-machine x=y gets merged into -machine a=b,x=y.

= The two feature don't want to play with each other =

With option merging, we can have any number of *actual* option
locations, but QemuOpts can capture just one, so we arbitrarily capture
the first one.  In your example, that's "-machine virt", which is
misleading, because the offending location is actually "-M 8G".

By the time we realized the features don't play with each other, we were
too deeply invested into both features to say "either feature is fine,
but we can't have both".

To make them play, we'd have to capture locations at the struct QemuOpt
level instead.  Looks like an awful lot of work just to make a few error
messages less confusing.  Makes me sad, because I do value decent error
messages.



[*] Multiple options with the same ID.  A detail that isn't relevant
here, I think.  Can explain if you're curious.