[Qemu-devel] [PATCH v2] qtest: Don't segfault with invalid -qtest option

[Qemu-devel] [PATCH v2] qtest: Don't segfault with invalid -qtest option

[Fam Zheng]

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 <famz@redhat.com>
---
 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();
-- 
1.8.5.4

Re: [Qemu-devel] [PATCH v2] qtest: Don't segfault with invalid -qtest option

[Markus Armbruster]

Fam Zheng <famz@redhat.com> 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 <famz@redhat.com>
> ---
>  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.

Re: [Qemu-devel] [PATCH v2] qtest: Don't segfault with invalid -qtest option

[Andreas Färber]

Am 10.02.2014 09:48, schrieb Markus Armbruster:
> Fam Zheng <famz@redhat.com> 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 <famz@redhat.com>
>> ---
>>  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
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.

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

Re: [Qemu-devel] [PATCH v2] qtest: Don't segfault with invalid -qtest option

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 10.02.2014 09:48, schrieb Markus Armbruster:
>> Fam Zheng <famz@redhat.com> 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 <famz@redhat.com>
>>> ---
>>>  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 :)

Re: [Qemu-devel] [PATCH v2] qtest: Don't segfault with invalid -qtest option

[Andreas Färber]

Am 10.02.2014 15:29, schrieb Markus Armbruster:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 10.02.2014 09:48, schrieb Markus Armbruster:
>>> Fam Zheng <famz@redhat.com> 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 <famz@redhat.com>
>>>> ---
>>>>  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