Andreas Färber <afaer...@suse.de> writes: > Am 10.02.2014 09:48, schrieb Markus Armbruster: >> Fam Zheng <f...@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 <f...@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 :)