[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] qtest: Don't segfault with invalid -qtest op
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2] qtest: Don't segfault with invalid -qtest option |
Date: |
Mon, 10 Feb 2014 15:29:02 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
Andreas Färber <address@hidden> writes:
> Am 10.02.2014 09:48, schrieb Markus Armbruster:
>> Fam Zheng <address@hidden> writes:
>>
>>> This prints an error message, instead of core dump, when "-qtest"
>>> option value is invalid, e.g.:
>>>
>>> $ ./x86_64-softmmu/qemu-system-x86_64 -qtest unknown
>>> qemu-system-x86_64: Failed to initialize device for qtest:
>>> "unknown"
>>>
>>> Signed-off-by: Fam Zheng <address@hidden>
>>> ---
>>> include/sysemu/qtest.h | 3 ++-
>>> qtest.c | 8 +++++++-
>>> vl.c | 8 +++++++-
>>> 3 files changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/sysemu/qtest.h b/include/sysemu/qtest.h
>>> index 112a661..2de61c6 100644
>>> --- a/include/sysemu/qtest.h
>>> +++ b/include/sysemu/qtest.h
>>> @@ -15,6 +15,7 @@
>>> #define QTEST_H
>>>
>>> #include "qemu-common.h"
>>> +#include "qapi/error.h"
>>>
>>> extern bool qtest_allowed;
>>>
>>> @@ -24,7 +25,7 @@ static inline bool qtest_enabled(void)
>>> }
>>>
>>> int qtest_init_accel(void);
>>> -void qtest_init(const char *qtest_chrdev, const char *qtest_log);
>>> +void qtest_init(const char *qtest_chrdev, const char *qtest_log, Error
>>> **errp);
>>>
>>> static inline int qtest_available(void)
>>> {
>>> diff --git a/qtest.c b/qtest.c
>>> index dcf1301..a4ad407 100644
>>> --- a/qtest.c
>>> +++ b/qtest.c
>>> @@ -507,12 +507,18 @@ int qtest_init_accel(void)
>>> return 0;
>>> }
>>>
>>> -void qtest_init(const char *qtest_chrdev, const char *qtest_log)
>>> +void qtest_init(const char *qtest_chrdev, const char *qtest_log, Error
>>> **errp)
>>> {
>>> CharDriverState *chr;
>>>
>>> chr = qemu_chr_new("qtest", qtest_chrdev, NULL);
>>>
>>> + if (chr == NULL) {
>>> + error_setg(errp, "Failed to initialize device for qtest: \"%s\"",
>>> + qtest_chrdev);
>>> + return;
>>> + }
>>> +
>>> qemu_chr_add_handlers(chr, qtest_can_read, qtest_read, qtest_event,
>>> chr);
>>> qemu_chr_fe_set_echo(chr, true);
>>>
>>> diff --git a/vl.c b/vl.c
>>> index e2e576c..bee455d 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -4079,7 +4079,13 @@ int main(int argc, char **argv, char **envp)
>>> configure_accelerator();
>>>
>>> if (qtest_chrdev) {
>>> - qtest_init(qtest_chrdev, qtest_log);
>>> + Error *local_err = NULL;
>>> + qtest_init(qtest_chrdev, qtest_log, &local_err);
>>> + if (error_is_set(&local_err)) {
>>> + error_report("%s", error_get_pretty(local_err));
>>> + error_free(local_err);
>>> + exit(1);
>>> + }
>>> }
>>>
>>> machine_opts = qemu_get_machine_opts();
>>
>> No objections, although I would've gone for simple & stupid instead:
>> Make qtest_init() return success / failure, and error_report() either in
>> qtest_init() or its caller, without the detour through an Error object.
>
> error_report() had been in the function in v1 and I suggested to either
> move the exit() there too (avoids signature changes and keeps them
> together, avoiding multiple error messages for the same failure) or to
Yes, that's even simpler.
> do it via Error** here.
>
> Since he decided for this route, I would propose to apply v2 to qom-next
> with error_is_set(&local_err) replaced with just local_err, honoring
> your cleanup patch.
Again, no objections :)