| Message ID | 20210412091824.707855-1-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm | expand |
On 12/04/2021 11.18, Stefan Hajnoczi wrote: > Some downstreams rename the QEMU binary to "qemu-kvm". This breaks > qtest_get_arch(), which attempts to parse the target architecture from > the QTEST_QEMU_BINARY environment variable. > > Print an error instead of returning the architecture "kvm". Things fail > in weird ways when the architecture string is bogus. > > Arguably qtests should always be run in a build directory instead of > against an installed QEMU. In any case, printing a clear error when this > happens is helpful. > > Reported-by: Qin Wang <qinwang@rehdat.com> > Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > tests/qtest/libqtest.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c > index 71e359efcd..7caf20f56b 100644 > --- a/tests/qtest/libqtest.c > +++ b/tests/qtest/libqtest.c > @@ -910,6 +910,16 @@ const char *qtest_get_arch(void) > abort(); > } > > + if (!strstr(qemu, "-system-")) { > + fprintf(stderr, "QTEST_QEMU_BINARY must end with *-system-<arch> where " > + "'arch' is the target architecture (x86_64, aarch64, " > + "etc). If you are using qemu-kvm or another custom " > + "name, please create a symlink like ln -s " > + "path/to/qemu-kvm qemu-system-x86_64 and use that " > + "instead.\n"); The text is very long ... maybe add some \n to wrap it after 80 columns? (also not sure whether we really need the second part about the symlink... but I also don't mind leaving it in) > + abort(); Since this can be triggered by the user, I'd rather use exit(1) instead, what do you think? Thomas > + } > + > return end + 1; > } > >
On Mon, 12 Apr 2021 at 10:35, Thomas Huth <thuth@redhat.com> wrote: > > On 12/04/2021 11.18, Stefan Hajnoczi wrote: > > Some downstreams rename the QEMU binary to "qemu-kvm". This breaks > > qtest_get_arch(), which attempts to parse the target architecture from > > the QTEST_QEMU_BINARY environment variable. > > > > Print an error instead of returning the architecture "kvm". Things fail > > in weird ways when the architecture string is bogus. > > > > Arguably qtests should always be run in a build directory instead of > > against an installed QEMU. In any case, printing a clear error when this > > happens is helpful. > > > > Reported-by: Qin Wang <qinwang@rehdat.com> > > Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > tests/qtest/libqtest.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c > > index 71e359efcd..7caf20f56b 100644 > > --- a/tests/qtest/libqtest.c > > +++ b/tests/qtest/libqtest.c > > @@ -910,6 +910,16 @@ const char *qtest_get_arch(void) > > abort(); > > } > > > > + if (!strstr(qemu, "-system-")) { > > + fprintf(stderr, "QTEST_QEMU_BINARY must end with *-system-<arch> where " > > + "'arch' is the target architecture (x86_64, aarch64, " > > + "etc). If you are using qemu-kvm or another custom " > > + "name, please create a symlink like ln -s " > > + "path/to/qemu-kvm qemu-system-x86_64 and use that " > > + "instead.\n"); > > The text is very long ... maybe add some \n to wrap it after 80 columns? > (also not sure whether we really need the second part about the symlink... > but I also don't mind leaving it in) Yeah, anybody who runs into this is doing something weird and can be assumed to be able to figure out how to do what they want with a name of the right form, I think. You'll never see it if you're just running 'make check'. thanks -- PMM
On Mon, Apr 12, 2021 at 11:35:40AM +0200, Thomas Huth wrote: > On 12/04/2021 11.18, Stefan Hajnoczi wrote: > > Some downstreams rename the QEMU binary to "qemu-kvm". This breaks > > qtest_get_arch(), which attempts to parse the target architecture from > > the QTEST_QEMU_BINARY environment variable. > > > > Print an error instead of returning the architecture "kvm". Things fail > > in weird ways when the architecture string is bogus. > > > > Arguably qtests should always be run in a build directory instead of > > against an installed QEMU. In any case, printing a clear error when this > > happens is helpful. > > > > Reported-by: Qin Wang <qinwang@rehdat.com> > > Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > tests/qtest/libqtest.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c > > index 71e359efcd..7caf20f56b 100644 > > --- a/tests/qtest/libqtest.c > > +++ b/tests/qtest/libqtest.c > > @@ -910,6 +910,16 @@ const char *qtest_get_arch(void) > > abort(); > > } > > + if (!strstr(qemu, "-system-")) { > > + fprintf(stderr, "QTEST_QEMU_BINARY must end with *-system-<arch> where " > > + "'arch' is the target architecture (x86_64, aarch64, " > > + "etc). If you are using qemu-kvm or another custom " > > + "name, please create a symlink like ln -s " > > + "path/to/qemu-kvm qemu-system-x86_64 and use that " > > + "instead.\n"); > > The text is very long ... maybe add some \n to wrap it after 80 columns? > (also not sure whether we really need the second part about the symlink... > but I also don't mind leaving it in) > > > + abort(); > > Since this can be triggered by the user, I'd rather use exit(1) instead, > what do you think? Sure, but in that case I guess the abort() call above also needs to be changed? It is triggered when the QTEST_QEMU_BINARY path does not contain a hyphen ('-') and it currently aborts. Stefan
On 12/04/2021 16.21, Stefan Hajnoczi wrote: > On Mon, Apr 12, 2021 at 11:35:40AM +0200, Thomas Huth wrote: >> On 12/04/2021 11.18, Stefan Hajnoczi wrote: >>> Some downstreams rename the QEMU binary to "qemu-kvm". This breaks >>> qtest_get_arch(), which attempts to parse the target architecture from >>> the QTEST_QEMU_BINARY environment variable. >>> >>> Print an error instead of returning the architecture "kvm". Things fail >>> in weird ways when the architecture string is bogus. >>> >>> Arguably qtests should always be run in a build directory instead of >>> against an installed QEMU. In any case, printing a clear error when this >>> happens is helpful. >>> >>> Reported-by: Qin Wang <qinwang@rehdat.com> >>> Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com> >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> tests/qtest/libqtest.c | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c >>> index 71e359efcd..7caf20f56b 100644 >>> --- a/tests/qtest/libqtest.c >>> +++ b/tests/qtest/libqtest.c >>> @@ -910,6 +910,16 @@ const char *qtest_get_arch(void) >>> abort(); >>> } >>> + if (!strstr(qemu, "-system-")) { >>> + fprintf(stderr, "QTEST_QEMU_BINARY must end with *-system-<arch> where " >>> + "'arch' is the target architecture (x86_64, aarch64, " >>> + "etc). If you are using qemu-kvm or another custom " >>> + "name, please create a symlink like ln -s " >>> + "path/to/qemu-kvm qemu-system-x86_64 and use that " >>> + "instead.\n"); >> >> The text is very long ... maybe add some \n to wrap it after 80 columns? >> (also not sure whether we really need the second part about the symlink... >> but I also don't mind leaving it in) >> >>> + abort(); >> >> Since this can be triggered by the user, I'd rather use exit(1) instead, >> what do you think? > > Sure, but in that case I guess the abort() call above also needs to be > changed? It is triggered when the QTEST_QEMU_BINARY path does not > contain a hyphen ('-') and it currently aborts. Drat, you're right, and it was even me who added that :-/ ... if you've got some spare minutes, could you send a patch for that, too, please? (Otherwise I'll do it later) Thomas
On 12/04/2021 16.35, Thomas Huth wrote: > On 12/04/2021 16.21, Stefan Hajnoczi wrote: >> On Mon, Apr 12, 2021 at 11:35:40AM +0200, Thomas Huth wrote: >>> On 12/04/2021 11.18, Stefan Hajnoczi wrote: >>>> Some downstreams rename the QEMU binary to "qemu-kvm". This breaks >>>> qtest_get_arch(), which attempts to parse the target architecture from >>>> the QTEST_QEMU_BINARY environment variable. >>>> >>>> Print an error instead of returning the architecture "kvm". Things fail >>>> in weird ways when the architecture string is bogus. >>>> >>>> Arguably qtests should always be run in a build directory instead of >>>> against an installed QEMU. In any case, printing a clear error when this >>>> happens is helpful. >>>> >>>> Reported-by: Qin Wang <qinwang@rehdat.com> >>>> Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com> >>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>>> --- >>>> tests/qtest/libqtest.c | 10 ++++++++++ >>>> 1 file changed, 10 insertions(+) >>>> >>>> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c >>>> index 71e359efcd..7caf20f56b 100644 >>>> --- a/tests/qtest/libqtest.c >>>> +++ b/tests/qtest/libqtest.c >>>> @@ -910,6 +910,16 @@ const char *qtest_get_arch(void) >>>> abort(); >>>> } >>>> + if (!strstr(qemu, "-system-")) { >>>> + fprintf(stderr, "QTEST_QEMU_BINARY must end with >>>> *-system-<arch> where " >>>> + "'arch' is the target architecture (x86_64, >>>> aarch64, " >>>> + "etc). If you are using qemu-kvm or another >>>> custom " >>>> + "name, please create a symlink like ln -s " >>>> + "path/to/qemu-kvm qemu-system-x86_64 and use >>>> that " >>>> + "instead.\n"); >>> >>> The text is very long ... maybe add some \n to wrap it after 80 columns? >>> (also not sure whether we really need the second part about the symlink... >>> but I also don't mind leaving it in) >>> >>>> + abort(); >>> >>> Since this can be triggered by the user, I'd rather use exit(1) instead, >>> what do you think? >> >> Sure, but in that case I guess the abort() call above also needs to be >> changed? It is triggered when the QTEST_QEMU_BINARY path does not >> contain a hyphen ('-') and it currently aborts. > > Drat, you're right, and it was even me who added that :-/ ... if you've got > some spare minutes, could you send a patch for that, too, please? (Otherwise > I'll do it later) Never mind, I just saw that you've fixed it in v3 already :-) Thomas
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index 71e359efcd..7caf20f56b 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -910,6 +910,16 @@ const char *qtest_get_arch(void) abort(); } + if (!strstr(qemu, "-system-")) { + fprintf(stderr, "QTEST_QEMU_BINARY must end with *-system-<arch> where " + "'arch' is the target architecture (x86_64, aarch64, " + "etc). If you are using qemu-kvm or another custom " + "name, please create a symlink like ln -s " + "path/to/qemu-kvm qemu-system-x86_64 and use that " + "instead.\n"); + abort(); + } + return end + 1; }
Some downstreams rename the QEMU binary to "qemu-kvm". This breaks qtest_get_arch(), which attempts to parse the target architecture from the QTEST_QEMU_BINARY environment variable. Print an error instead of returning the architecture "kvm". Things fail in weird ways when the architecture string is bogus. Arguably qtests should always be run in a build directory instead of against an installed QEMU. In any case, printing a clear error when this happens is helpful. Reported-by: Qin Wang <qinwang@rehdat.com> Cc: Emanuele Giuseppe Esposito <eesposit@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- tests/qtest/libqtest.c | 10 ++++++++++ 1 file changed, 10 insertions(+)