* [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm
@ 2021-04-12 9:18 Stefan Hajnoczi
2021-04-12 9:35 ` Thomas Huth
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2021-04-12 9:18 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Peter Maydell, Thomas Huth,
Emanuele Giuseppe Esposito, Qin Wang, Stefan Hajnoczi,
Paolo Bonzini
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");
+ abort();
+ }
+
return end + 1;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm
2021-04-12 9:18 [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm Stefan Hajnoczi
@ 2021-04-12 9:35 ` Thomas Huth
2021-04-12 9:50 ` Peter Maydell
2021-04-12 14:21 ` Stefan Hajnoczi
0 siblings, 2 replies; 6+ messages in thread
From: Thomas Huth @ 2021-04-12 9:35 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Laurent Vivier, Paolo Bonzini, Emanuele Giuseppe Esposito,
Qin Wang, Peter Maydell
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;
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm
2021-04-12 9:35 ` Thomas Huth
@ 2021-04-12 9:50 ` Peter Maydell
2021-04-12 14:21 ` Stefan Hajnoczi
1 sibling, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2021-04-12 9:50 UTC (permalink / raw)
To: Thomas Huth
Cc: Laurent Vivier, Emanuele Giuseppe Esposito, Qin Wang,
QEMU Developers, Stefan Hajnoczi, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm
2021-04-12 9:35 ` Thomas Huth
2021-04-12 9:50 ` Peter Maydell
@ 2021-04-12 14:21 ` Stefan Hajnoczi
2021-04-12 14:35 ` Thomas Huth
1 sibling, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2021-04-12 14:21 UTC (permalink / raw)
To: Thomas Huth
Cc: Laurent Vivier, Emanuele Giuseppe Esposito, Peter Maydell,
Qin Wang, qemu-devel, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 2198 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm
2021-04-12 14:21 ` Stefan Hajnoczi
@ 2021-04-12 14:35 ` Thomas Huth
2021-04-12 14:37 ` Thomas Huth
0 siblings, 1 reply; 6+ messages in thread
From: Thomas Huth @ 2021-04-12 14:35 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Laurent Vivier, Emanuele Giuseppe Esposito, Peter Maydell,
Qin Wang, qemu-devel, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm
2021-04-12 14:35 ` Thomas Huth
@ 2021-04-12 14:37 ` Thomas Huth
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Huth @ 2021-04-12 14:37 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Laurent Vivier, Emanuele Giuseppe Esposito, Qin Wang,
Peter Maydell, qemu-devel, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-12 14:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-12 9:18 [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm Stefan Hajnoczi
2021-04-12 9:35 ` Thomas Huth
2021-04-12 9:50 ` Peter Maydell
2021-04-12 14:21 ` Stefan Hajnoczi
2021-04-12 14:35 ` Thomas Huth
2021-04-12 14:37 ` Thomas Huth
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).