| Message ID | 1427911244-22565-1-git-send-email-emaste@freebsd.org |
|---|---|
| State | New |
| Headers | show |
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>
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
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
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
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
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
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-");
Signed-off-by: Ed Maste <emaste@freebsd.org> --- tests/libqtest.c | 1 + 1 file changed, 1 insertion(+)