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

["Andreas Färber" <afaerber@suse.de>]

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