| Message ID | 20190915202011.30459-1-palmer@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vl.c: Report unknown machines correctly | expand |
CCing Mr command line... Paolo On 15/09/19 22:20, Palmer Dabbelt wrote: > 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 <palmer@sifive.com> > --- > 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); > } >
On 9/15/19 10:20 PM, Palmer Dabbelt wrote: > 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 <palmer@sifive.com> > --- > 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); > } > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Palmer Dabbelt <palmer@sifive.com> 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 <palmer@sifive.com> > --- > 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.
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); }
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 <palmer@sifive.com> --- vl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)