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

[Andreas Färber]

Am 08.02.2014 10:28, schrieb 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 <address@hidden>
> ---
>  include/sysemu/qtest.h |  2 +-
>  qtest.c                | 10 +++++++++-
>  vl.c                   |  5 ++++-
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/include/sysemu/qtest.h b/include/sysemu/qtest.h
> index 112a661..fc108c7 100644
> --- a/include/sysemu/qtest.h
> +++ b/include/sysemu/qtest.h
> @@ -24,7 +24,7 @@ static inline bool qtest_enabled(void)
>  }
>  
>  int qtest_init_accel(void);
> -void qtest_init(const char *qtest_chrdev, const char *qtest_log);
> +int qtest_init(const char *qtest_chrdev, const char *qtest_log);
>  
>  static inline int qtest_available(void)
>  {
> diff --git a/qtest.c b/qtest.c
> index dcf1301..a037b3b 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -19,6 +19,7 @@
>  #include "hw/irq.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpus.h"
> +#include "qemu/error-report.h"
>  
>  #define MAX_IRQ 256
>  
> @@ -507,12 +508,18 @@ int qtest_init_accel(void)
>      return 0;
>  }
>  
> -void qtest_init(const char *qtest_chrdev, const char *qtest_log)
> +int qtest_init(const char *qtest_chrdev, const char *qtest_log)
>  {
>      CharDriverState *chr;
>  
>      chr = qemu_chr_new("qtest", qtest_chrdev, NULL);
>  
> +    if (chr == NULL) {
> +        error_report("Failed to initialize device for qtest: \"%s\"",
> +                     qtest_chrdev);
> +        return -1;
> +    }
> +
>      qemu_chr_add_handlers(chr, qtest_can_read, qtest_read, qtest_event, chr);
>      qemu_chr_fe_set_echo(chr, true);
>  
> @@ -527,4 +534,5 @@ void qtest_init(const char *qtest_chrdev, const char 
> *qtest_log)
>      }
>  
>      qtest_chr = chr;
> +    return 0;
>  }
> diff --git a/vl.c b/vl.c
> index 383be1b..97ca823 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4078,7 +4078,10 @@ int main(int argc, char **argv, char **envp)
>      configure_accelerator();
>  
>      if (qtest_chrdev) {
> -        qtest_init(qtest_chrdev, qtest_log);
> +        int ret = qtest_init(qtest_chrdev, qtest_log);
> +        if (ret) {
> +            exit(1);
> +        }
>      }
>  
>      machine_opts = qemu_get_machine_opts();

Why don't you do the exit(1) in qtest_init()? Either that or keep void
and return an Error **errp so that you can print it here where the
exit(1) is.

Otherwise error_report() usage looks good.

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