From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54160) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKojT-0003KO-DC for qemu-devel@nongnu.org; Fri, 22 Aug 2014 09:18:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XKojL-0000Ss-Qg for qemu-devel@nongnu.org; Fri, 22 Aug 2014 09:18:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49519) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XKojL-0000Si-JJ for qemu-devel@nongnu.org; Fri, 22 Aug 2014 09:18:11 -0400 Date: Fri, 22 Aug 2014 14:18:02 +0100 From: Stefan Hajnoczi Message-ID: <20140822131802.GA14126@stefanha-thinkpad.redhat.com> References: <20140821175235.12061.22630.stgit@fimbulvetr.bsc.es> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="G4iJoqBmSsgzjUCe" Content-Disposition: inline In-Reply-To: <20140821175235.12061.22630.stgit@fimbulvetr.bsc.es> Subject: Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Llu=EDs?= Vilanova Cc: Michael Roth , Markus Armbruster , qemu-devel@nongnu.org, Luiz Capitulino --G4iJoqBmSsgzjUCe Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Aug 21, 2014 at 07:52:37PM +0200, Llu=EDs Vilanova wrote: > Also removes old "trace-event", "trace-file" and "info trace-events" HMP > commands. Is this commit description correct? I think we don't want to remove HMP commands. It is "legacy" but users may still rely on HMP. It's certainly used for ad-hoc debugging. > diff --git a/monitor.c b/monitor.c > index cdbaa60..0f605f5 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -887,19 +887,8 @@ static void do_trace_event_set_state(Monitor *mon, c= onst QDict *qdict) > const char *tp_name =3D qdict_get_str(qdict, "name"); > bool new_state =3D qdict_get_bool(qdict, "option"); > =20 > - bool found =3D false; > - TraceEvent *ev =3D NULL; > - while ((ev =3D trace_event_pattern(tp_name, ev)) !=3D NULL) { > - found =3D true; > - if (!trace_event_get_state_static(ev)) { > - monitor_printf(mon, "event \"%s\" is not traceable\n", tp_na= me); > - } else { > - trace_event_set_state_dynamic(ev, new_state); > - } > - } > - if (!trace_event_is_pattern(tp_name) && !found) { > - monitor_printf(mon, "unknown event name \"%s\"\n", tp_name); > - } > + /* TODO: should propagate QMP errors to HMP */ > + qmp_trace_event_set_state(tp_name, new_state, true, true, NULL); The TODO can be resolved with: if (errp) { monitor_printf(mon, "%s", error_get_pretty(errp)); error_free(errp); } > +TraceEventStateList *qmp_trace_event_get_state(const char *name, Error *= *errp) > +{ > + TraceEventStateList dummy =3D {}; > + TraceEventStateList *prev =3D &dummy; > + > + bool found =3D false; > + TraceEvent *ev =3D NULL; > + while ((ev =3D trace_event_pattern(name, ev)) !=3D NULL) { > + found =3D true; > + TraceEventStateList *elem =3D g_malloc0(sizeof(*elem)); > + elem->value =3D g_malloc0(sizeof(*elem->value)); > + elem->value->name =3D g_strdup(trace_event_get_name(ev)); > + elem->value->sstatic =3D trace_event_get_state_static(ev); > + elem->value->sdynamic =3D trace_event_get_state_dynamic(ev); > + prev->next =3D elem; > + prev =3D elem; > + } > + if (!found && !trace_event_is_pattern(name)) { > + error_setg(errp, "unknown event \"%s\"\n", name); \n is not needed in error_setg() message. Please remove it. There are more instances below. > + } > + > + return dummy.next; > +} > + > +void qmp_trace_event_set_state(const char *name, bool state, bool has_ke= epgoing, > + bool keepgoing, Error **errp) > +{ > + bool error =3D false; > + bool found =3D false; > + TraceEvent *ev =3D NULL; > + > + /* Check all selected events are dynamic */ > + while ((ev =3D trace_event_pattern(name, ev)) !=3D NULL) { > + found =3D true; > + if (!trace_event_get_state_static(ev)) { > + error_setg(errp, "cannot set dynamic tracing state for \"%s\= "\n", > + trace_event_get_name(ev)); error_setg() can only be called once. Calling it with non-NULL errp produces an assertion failure. Maybe this approach can be used instead: while ((ev =3D trace_event_pattern(name, ev)) !=3D NULL) { found =3D true; if (!(has_keepgoing && keepgoing) && !trace_event_get_state_static(ev)) { error_setg(errp, "cannot set dynamic tracing state for \"%s\"", trace_event_get_name(ev)); return; } } The bool error variable can be dropped. > + if (!(has_keepgoing && keepgoing)) { > + error =3D true; > + } > + break; > + } > + } > + if (error) { > + return; > + } > + if (!found && !trace_event_is_pattern(name)) { > + error_setg(errp, "unknown event \"%s\"\n", name); > + return; > + } > + > + if (error) { > + return; > + } This condition has already been checked above. This if statement can be dropped. --G4iJoqBmSsgzjUCe Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJT90MKAAoJEJykq7OBq3PIQA4H/ifaXNMPWB8yQxW3vc6G9o8x M5StHufgbUhxSYyACgbqcViFXzdYtSx5cYIGJQkorpPK9Y8yu8H/YUmgPwULYrbM bsVk3poH+ABYEn3kXTF9cyeOctRTAQCrAwLeqVIFcjwXwry4/QlJOl8JUmDgmhaW KBLP0yoA5Km8v9ZqK96Fsl0SGkWE0+6ETVjcsu8sTLnkNXPHiUnQc68/8aKnUmDh 8QuliwB+nV1FyJ229o7uS6tN4Hl2qqnrx9VTBIqTsk2TnghktIHe3aiaYt4JIMnJ Hr4U7MkMFAYzrpdBolxjAxyrtspLlE5gZVzRI2R1CX+RJZ0sJKoHxMabsjFFdVk= =NKQd -----END PGP SIGNATURE----- --G4iJoqBmSsgzjUCe--