[Qemu-trivial] [PATCH] qtest: Add assertion that required environment variable is set

[Qemu-trivial] [PATCH] qtest: Add assertion that required environment variable is set

[Ed Maste]

Signed-off-by: Ed Maste <emaste@freebsd.org>
---
 tests/libqtest.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 12d65bd..54550a8 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -453,6 +453,7 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
 const char *qtest_get_arch(void)
 {
     const char *qemu = getenv("QTEST_QEMU_BINARY");
+    g_assert(qemu != NULL);
     const char *end = strrchr(qemu, '/');
 
     return end + strlen("/qemu-system-");
-- 
2.2.2

[Qemu-devel] [PATCH] qtest: Add assertion that required environment variable is set

[Ed Maste]

Signed-off-by: Ed Maste <emaste@freebsd.org>
---
 tests/libqtest.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 12d65bd..54550a8 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -453,6 +453,7 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
 const char *qtest_get_arch(void)
 {
     const char *qemu = getenv("QTEST_QEMU_BINARY");
+    g_assert(qemu != NULL);
     const char *end = strrchr(qemu, '/');
 
     return end + strlen("/qemu-system-");
-- 
2.2.2

Re: [Qemu-trivial] [Qemu-devel] [PATCH] qtest: Add assertion that required environment variable is set

[John Snow]

On 04/01/2015 02:00 PM, Ed Maste wrote:
> Signed-off-by: Ed Maste <emaste@freebsd.org>
> ---
>   tests/libqtest.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 12d65bd..54550a8 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -453,6 +453,7 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
>   const char *qtest_get_arch(void)
>   {
>       const char *qemu = getenv("QTEST_QEMU_BINARY");
> +    g_assert(qemu != NULL);
>       const char *end = strrchr(qemu, '/');
>
>       return end + strlen("/qemu-system-");
>

This one has annoyed me in the past, too.

I wonder if it would be even nicer to add an fprintf to give the user a 
nice message explaining exactly what went wrong, though -- since this 
particular error is only going to happen when a user is invoking the 
test manually.

Maybe:

if (qemu == NULL) {
   fprintf(stderr, "...");
   g_assert_not_reached();
}

Though that does read a little strangely. ("Here's a nice error message 
for something we are asserting will never happen.")



Well, either way, it's better than segfaulting, so:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH] qtest: Add assertion that required environment variable is set

[John Snow]

On 04/01/2015 02:00 PM, Ed Maste wrote:
> Signed-off-by: Ed Maste <emaste@freebsd.org>
> ---
>   tests/libqtest.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 12d65bd..54550a8 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -453,6 +453,7 @@ void qtest_qmp_discard_response(QTestState *s, const char *fmt, ...)
>   const char *qtest_get_arch(void)
>   {
>       const char *qemu = getenv("QTEST_QEMU_BINARY");
> +    g_assert(qemu != NULL);
>       const char *end = strrchr(qemu, '/');
>
>       return end + strlen("/qemu-system-");
>

This one has annoyed me in the past, too.

I wonder if it would be even nicer to add an fprintf to give the user a 
nice message explaining exactly what went wrong, though -- since this 
particular error is only going to happen when a user is invoking the 
test manually.

Maybe:

if (qemu == NULL) {
   fprintf(stderr, "...");
   g_assert_not_reached();
}

Though that does read a little strangely. ("Here's a nice error message 
for something we are asserting will never happen.")



Well, either way, it's better than segfaulting, so:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-trivial] [PATCH] qtest: Add assertion that required environment variable is set

[Paolo Bonzini]

On 01/04/2015 23:06, John Snow wrote:
> 
> if (qemu == NULL) {
>   fprintf(stderr, "...");
>   g_assert_not_reached();
> }
> 
> Though that does read a little strangely. ("Here's a nice error message
> for something we are asserting will never happen.")

Just "exit(1);" then.  :)

Good idea, this is annoying.

Paolo

Re: [Qemu-devel] [PATCH] qtest: Add assertion that required environment variable is set

[Paolo Bonzini]

On 01/04/2015 23:06, John Snow wrote:
> 
> if (qemu == NULL) {
>   fprintf(stderr, "...");
>   g_assert_not_reached();
> }
> 
> Though that does read a little strangely. ("Here's a nice error message
> for something we are asserting will never happen.")

Just "exit(1);" then.  :)

Good idea, this is annoying.

Paolo

Re: [Qemu-trivial] [Qemu-devel] [PATCH] qtest: Add assertion that required environment variable is set

[Peter Maydell]

On 1 April 2015 at 22:14, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 01/04/2015 23:06, John Snow wrote:
>>
>> if (qemu == NULL) {
>>   fprintf(stderr, "...");
>>   g_assert_not_reached();
>> }
>>
>> Though that does read a little strangely. ("Here's a nice error message
>> for something we are asserting will never happen.")
>
> Just "exit(1);" then.  :)
>
> Good idea, this is annoying.

Also irritating is the way it silently requires
the binary to have a name in the shape it was
expecting, which can catch you out if you were
trying to set it to a wrapper shell script that
invokes valgrind or something...

-- PMM

Re: [Qemu-devel] [PATCH] qtest: Add assertion that required environment variable is set

[Peter Maydell]

On 1 April 2015 at 22:14, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 01/04/2015 23:06, John Snow wrote:
>>
>> if (qemu == NULL) {
>>   fprintf(stderr, "...");
>>   g_assert_not_reached();
>> }
>>
>> Though that does read a little strangely. ("Here's a nice error message
>> for something we are asserting will never happen.")
>
> Just "exit(1);" then.  :)
>
> Good idea, this is annoying.

Also irritating is the way it silently requires
the binary to have a name in the shape it was
expecting, which can catch you out if you were
trying to set it to a wrapper shell script that
invokes valgrind or something...

-- PMM

Re: [Qemu-trivial] [Qemu-devel] [PATCH] qtest: Add assertion that required environment variable is set

[Ed Maste]

On 1 April 2015 at 18:45, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 April 2015 at 22:14, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 01/04/2015 23:06, John Snow wrote:
>>>
>>> if (qemu == NULL) {
>>>   fprintf(stderr, "...");
>>>   g_assert_not_reached();
>>> }
>>>
>>> Though that does read a little strangely. ("Here's a nice error message
>>> for something we are asserting will never happen.")
>>
>> Just "exit(1);" then.  :)
>>
>> Good idea, this is annoying.
>
> Also irritating is the way it silently requires
> the binary to have a name in the shape it was
> expecting, which can catch you out if you were
> trying to set it to a wrapper shell script that
> invokes valgrind or something...

I don't really have enough context to propose a good user-facing
message with a tip for manually executing this, so hopefully someone
else can provide one. I just noticed one other instance that already
had an assertion on getenv("QTEST_QEMU_BINARY") being non-null.

-Ed

Re: [Qemu-devel] [PATCH] qtest: Add assertion that required environment variable is set

[Ed Maste]

On 1 April 2015 at 18:45, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 April 2015 at 22:14, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 01/04/2015 23:06, John Snow wrote:
>>>
>>> if (qemu == NULL) {
>>>   fprintf(stderr, "...");
>>>   g_assert_not_reached();
>>> }
>>>
>>> Though that does read a little strangely. ("Here's a nice error message
>>> for something we are asserting will never happen.")
>>
>> Just "exit(1);" then.  :)
>>
>> Good idea, this is annoying.
>
> Also irritating is the way it silently requires
> the binary to have a name in the shape it was
> expecting, which can catch you out if you were
> trying to set it to a wrapper shell script that
> invokes valgrind or something...

I don't really have enough context to propose a good user-facing
message with a tip for manually executing this, so hopefully someone
else can provide one. I just noticed one other instance that already
had an assertion on getenv("QTEST_QEMU_BINARY") being non-null.

-Ed

Re: [Qemu-trivial] [Qemu-devel] [PATCH] qtest: Add assertion that required environment variable is set

[Peter Maydell]

On 2 April 2015 at 20:31, Ed Maste <emaste@freebsd.org> wrote:
> On 1 April 2015 at 18:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Also irritating is the way it silently requires
>> the binary to have a name in the shape it was
>> expecting, which can catch you out if you were
>> trying to set it to a wrapper shell script that
>> invokes valgrind or something...
>
> I don't really have enough context to propose a good user-facing
> message with a tip for manually executing this, so hopefully someone
> else can provide one. I just noticed one other instance that already
> had an assertion on getenv("QTEST_QEMU_BINARY") being non-null.

Yes, that was just me venting about something that caught me
out in the past rather than review comment on this patch :-)
Sorry for any confusion.

-- PMM

Re: [Qemu-devel] [PATCH] qtest: Add assertion that required environment variable is set

[Peter Maydell]

On 2 April 2015 at 20:31, Ed Maste <emaste@freebsd.org> wrote:
> On 1 April 2015 at 18:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Also irritating is the way it silently requires
>> the binary to have a name in the shape it was
>> expecting, which can catch you out if you were
>> trying to set it to a wrapper shell script that
>> invokes valgrind or something...
>
> I don't really have enough context to propose a good user-facing
> message with a tip for manually executing this, so hopefully someone
> else can provide one. I just noticed one other instance that already
> had an assertion on getenv("QTEST_QEMU_BINARY") being non-null.

Yes, that was just me venting about something that caught me
out in the past rather than review comment on this patch :-)
Sorry for any confusion.

-- PMM

Re: [Qemu-trivial] [Qemu-devel] [PATCH] qtest: Add assertion that required environment variable is set

[John Snow]

On 04/03/2015 07:18 AM, Peter Maydell wrote:
> On 2 April 2015 at 20:31, Ed Maste <emaste@freebsd.org> wrote:
>> On 1 April 2015 at 18:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Also irritating is the way it silently requires
>>> the binary to have a name in the shape it was
>>> expecting, which can catch you out if you were
>>> trying to set it to a wrapper shell script that
>>> invokes valgrind or something...
>>
>> I don't really have enough context to propose a good user-facing
>> message with a tip for manually executing this, so hopefully someone
>> else can provide one. I just noticed one other instance that already
>> had an assertion on getenv("QTEST_QEMU_BINARY") being non-null.
>
> Yes, that was just me venting about something that caught me
> out in the past rather than review comment on this patch :-)
> Sorry for any confusion.
>
> -- PMM
>

I'll pull this into ide-next for 2.4 -- I have some ahci-test things to 
submit anyway.

I'll touch up the user facing error messages in a later patch and hit a 
few of the startup assertions all at once.

Thanks.
--js

Re: [Qemu-devel] [PATCH] qtest: Add assertion that required environment variable is set

[John Snow]

On 04/03/2015 07:18 AM, Peter Maydell wrote:
> On 2 April 2015 at 20:31, Ed Maste <emaste@freebsd.org> wrote:
>> On 1 April 2015 at 18:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Also irritating is the way it silently requires
>>> the binary to have a name in the shape it was
>>> expecting, which can catch you out if you were
>>> trying to set it to a wrapper shell script that
>>> invokes valgrind or something...
>>
>> I don't really have enough context to propose a good user-facing
>> message with a tip for manually executing this, so hopefully someone
>> else can provide one. I just noticed one other instance that already
>> had an assertion on getenv("QTEST_QEMU_BINARY") being non-null.
>
> Yes, that was just me venting about something that caught me
> out in the past rather than review comment on this patch :-)
> Sorry for any confusion.
>
> -- PMM
>

I'll pull this into ide-next for 2.4 -- I have some ahci-test things to 
submit anyway.

I'll touch up the user facing error messages in a later patch and hit a 
few of the startup assertions all at once.

Thanks.
--js