From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35036) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCuhw-0002i6-AQ for qemu-devel@nongnu.org; Mon, 10 Feb 2014 12:31:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WCuhr-0003qS-2A for qemu-devel@nongnu.org; Mon, 10 Feb 2014 12:31:48 -0500 Received: from cantor2.suse.de ([195.135.220.15]:45985 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCuhq-0003qD-OT for qemu-devel@nongnu.org; Mon, 10 Feb 2014 12:31:43 -0500 Message-ID: <52F90CFB.6020101@suse.de> Date: Mon, 10 Feb 2014 18:31:39 +0100 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1391995682-27028-1-git-send-email-famz@redhat.com> <87wqh3mjzp.fsf@blackfin.pond.sub.org> <52F8D770.8060901@suse.de> <87txc72g9d.fsf@blackfin.pond.sub.org> In-Reply-To: <87txc72g9d.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=UTF-8 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 15:29, schrieb Markus Armbruster: > Andreas F=C3=A4rber writes: >=20 >> Am 10.02.2014 09:48, schrieb Markus Armbruster: >>> Fam Zheng 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 >>>> --- >>>> 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, Er= ror **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, Er= ror **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_ev= ent, 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(); >>> >>> 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 objec= t. >> >> error_report() had been in the function in v1 and I suggested to eithe= r >> move the exit() there too (avoids signature changes and keeps them >> together, avoiding multiple error messages for the same failure) or to >=20 > Yes, that's even simpler. >=20 >> do it via Error** here. >> >> Since he decided for this route, I would propose to apply v2 to qom-ne= xt >> with error_is_set(&local_err) replaced with just local_err, honoring >> your cleanup patch. >=20 > Again, no objections :) Thanks, applied to qom-next with the following change: diff --git a/vl.c b/vl.c index 1bcd083..0f7d31f 100644 --- a/vl.c +++ b/vl.c @@ -4080,7 +4080,7 @@ int main(int argc, char **argv, char **envp) if (qtest_chrdev) { Error *local_err =3D NULL; qtest_init(qtest_chrdev, qtest_log, &local_err); - if (error_is_set(&local_err)) { + if (local_err) { error_report("%s", error_get_pretty(local_err)); error_free(local_err); exit(1); https://github.com/afaerber/qemu-cpu/commits/qom-next Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg