From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59972) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCr90-0006eY-VW for qemu-devel@nongnu.org; Mon, 10 Feb 2014 08:43:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WCr8t-0003QH-KH for qemu-devel@nongnu.org; Mon, 10 Feb 2014 08:43:30 -0500 Received: from cantor2.suse.de ([195.135.220.15]:39722 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCr8t-0003Q8-94 for qemu-devel@nongnu.org; Mon, 10 Feb 2014 08:43:23 -0500 Message-ID: <52F8D770.8060901@suse.de> Date: Mon, 10 Feb 2014 14:43:12 +0100 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1391995682-27028-1-git-send-email-famz@redhat.com> <87wqh3mjzp.fsf@blackfin.pond.sub.org> In-Reply-To: <87wqh3mjzp.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] qtest: Don't segfault with invalid -qtest option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Fam Zheng Cc: qemu-devel@nongnu.org, Anthony Liguori Am 10.02.2014 09:48, schrieb Markus Armbruster: > Fam Zheng writes: >=20 >> 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 >> --- >> 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 >> =20 >> #include "qemu-common.h" >> +#include "qapi/error.h" >> =20 >> extern bool qtest_allowed; >> =20 >> @@ -24,7 +25,7 @@ static inline bool qtest_enabled(void) >> } >> =20 >> 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, Erro= r **errp); >> =20 >> 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; >> } >> =20 >> -void qtest_init(const char *qtest_chrdev, const char *qtest_log) >> +void qtest_init(const char *qtest_chrdev, const char *qtest_log, Erro= r **errp) >> { >> CharDriverState *chr; >> =20 >> chr =3D qemu_chr_new("qtest", qtest_chrdev, NULL); >> =20 >> + if (chr =3D=3D 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_even= t, chr); >> qemu_chr_fe_set_echo(chr, true); >> =20 >> 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(); >> =20 >> if (qtest_chrdev) { >> - qtest_init(qtest_chrdev, qtest_log); >> + Error *local_err =3D 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); >> + } >> } >> =20 >> machine_opts =3D qemu_get_machine_opts(); >=20 > No objections, although I would've gone for simple & stupid instead: > Make qtest_init() return success / failure, and error_report() either i= n > 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 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg