[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: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [PATCH v2] qtest: Don't segfault with invalid -qtest option |
Date: |
Mon, 10 Feb 2014 18:31:39 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 |
Am 10.02.2014 15:29, schrieb Markus Armbruster:
> 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 :)
Thanks, applied to qom-next with the following change:
diff --git a/vl.c b/vl.c
index 1bcd083..0f7d31f 100644
--- a/vl.c
+++ b/vl.c
@@ -4080,7 +4080,7 @@ int main(int argc, char **argv, char **envp)
if (qtest_chrdev) {
Error *local_err = NULL;
qtest_init(qtest_chrdev, qtest_log, &local_err);
- if (error_is_set(&local_err)) {
+ if (local_err) {
error_report("%s", error_get_pretty(local_err));
error_free(local_err);
exit(1);
https://github.com/afaerber/qemu-cpu/commits/qom-next
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg