[PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm

[PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm

[Stefan Hajnoczi]

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

Re: [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm

[Thomas Huth]

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;
>   }
>   
> 

Re: [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm

[Peter Maydell]

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

Re: [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm

[Stefan Hajnoczi]

[-- 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 --]

Re: [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm

[Thomas Huth]

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

Re: [PATCH v2] libqtest: refuse QTEST_QEMU_BINARY=qemu-kvm

[Thomas Huth]

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