Am 27.03.2014 14:09, schrieb Markus Armbruster:
> Reply after commit, sorry.
>
> Stefan Hajnoczi <stefa...@redhat.com> writes:
>
>> If an assertion fails during qtest_init() the SIGABRT handler is
>> invoked. This is the correct behavior since we need to kill the QEMU
>> process to avoid leaking it when the test dies.
>>
>> The global_qtest pointer used by the SIGABRT handler is currently only
>> assigned after qtest_init() returns. This results in a segfault if an
>> assertion failure occurs during qtest_init().
>>
>> Move global_qtest assignment inside qtest_init(). Not pretty but let's
>> face it - the signal handler dependeds on global state.
>>
>> Reported-by: Marcel Apfelbaum <marce...@redhat.com>
>> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
>> ---
>> tests/libqtest.c | 3 ++-
>> tests/libqtest.h | 4 +---
>> 2 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index c9e78aa..f387662 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -120,7 +120,7 @@ QTestState *qtest_init(const char *extra_args)
>> qemu_binary = getenv("QTEST_QEMU_BINARY");
>> g_assert(qemu_binary != NULL);
>>
>> - s = g_malloc(sizeof(*s));
>> + global_qtest = s = g_malloc(sizeof(*s));
>>
>> socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
>> qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
>> @@ -181,6 +181,7 @@ QTestState *qtest_init(const char *extra_args)
>> void qtest_quit(QTestState *s)
>> {
>> sigaction(SIGABRT, &s->sigact_old, NULL);
>> + global_qtest = NULL;
>>
>> kill_qemu(s);
>> close(s->fd);
>> diff --git a/tests/libqtest.h b/tests/libqtest.h
>> index 9deebdc..7e23a4e 100644
>> --- a/tests/libqtest.h
>> +++ b/tests/libqtest.h
>> @@ -335,8 +335,7 @@ void qtest_add_func(const char *str, void (*fn));
>> */
>> static inline QTestState *qtest_start(const char *args)
>> {
>> - global_qtest = qtest_init(args);
>> - return global_qtest;
>> + return qtest_init(args);
>> }
>>
>> /**
>> @@ -347,7 +346,6 @@ static inline QTestState *qtest_start(const char *args)
>> static inline void qtest_end(void)
>> {
>> qtest_quit(global_qtest);
>> - global_qtest = NULL;
>> }
>>
>> /**
>
> Before this patch, the libqtest API could theoretically support multiple
> simultaneous instances of QTestState. This patch kills that option,
> doesn't it?
Ouch, I thought I had looked out for that...
>
> If yes: fine with me, we don't need it anyway.
We do. Migration and ivshmem are examples that need two machines - might
explain why my ivshmem-test was behaving unexpectedly.
Apart from reverting, what are our options?
Regards,
Andreas
> But shouldn't we clean
> up the API then? It typically provides a function taking a QTestState
> parameter, and a wrapper that passes global_qtest. If global_qtest is
> the only QTestState we can have, having the former function is
> pointless.
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
