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

[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.