From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37523) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm3SO-0000Y8-LI for qemu-devel@nongnu.org; Tue, 13 Oct 2015 13:33:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zm3SK-0000mE-E0 for qemu-devel@nongnu.org; Tue, 13 Oct 2015 13:33:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58612) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zm3SK-0000m8-6C for qemu-devel@nongnu.org; Tue, 13 Oct 2015 13:33:44 -0400 References: <20151013171020.21325.27626.stgit@localhost> <20151013171038.21325.97389.stgit@localhost> From: Eric Blake Message-ID: <561D4072.1080207@redhat.com> Date: Tue, 13 Oct 2015 11:33:38 -0600 MIME-Version: 1.0 In-Reply-To: <20151013171038.21325.97389.stgit@localhost> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ORVE0at26AR5gWGLa9NWfBwNsjMgSwMu4" Subject: Re: [Qemu-devel] [PATCHv13/8] trace: [tcg] Identify events with the 'vcpu' property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Llu=c3=ads_Vilanova?= , qemu-devel@nongnu.org Cc: Stefan Hajnoczi , Markus Armbruster , Stefan Hajnoczi This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --ORVE0at26AR5gWGLa9NWfBwNsjMgSwMu4 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/13/2015 11:10 AM, Llu=C3=ADs Vilanova wrote: > Signed-off-by: Llu=C3=ADs Vilanova If you'd send with 'qemu format-patch/send-email -v1', then your subject line would be formatted [PATCH v1 3/8] instead of the confusing results you got by omitting spaces [PATCHv13/8] (this is v13? out of 8?). Also, no need to include v1 (it's fairly obvious that an unversioned patch is the first), you really only need the designation for -v2 and beyond. The one-line commit subject correctly explains the 'what', but there is no commit body explaining the 'why'. While a commit body is not mandatory, it usually helps. > +++ b/qapi/trace.json > @@ -1,6 +1,6 @@ > # -*- mode: python -*- > # > -# Copyright (C) 2011-2014 Llu=C3=ADs Vilanova > +# Copyright (C) 2011-2015 Llu=C3=ADs Vilanova > # > # This work is licensed under the terms of the GNU GPL, version 2 or l= ater. > # See the COPYING file in the top-level directory. > @@ -29,11 +29,12 @@ > # > # @name: Event name. > # @state: Tracing state. > +# @vcpu: Whether this is a per-vCPU event. > # Missing a '(since 2.5)' comment on the @vcpu line. > # Since 2.2 > ## > { 'struct': 'TraceEventInfo', > - 'data': {'name': 'str', 'state': 'TraceEventState'} } > + 'data': {'name': 'str', 'state': 'TraceEventState', 'vcpu': 'bool'} = } This is okay from the qapi interface perspective (events are output, so unconditional addition of new fields won't break existing clients). I did not closely review the rest, however, I did spot this: > +++ b/trace/qmp.c > @@ -22,6 +22,8 @@ TraceEventInfoList *qmp_trace_event_get_state(const c= har *name, Error **errp) > while ((ev =3D trace_event_pattern(name, ev)) !=3D NULL) { > TraceEventInfoList *elem =3D g_new(TraceEventInfoList, 1); > elem->value =3D g_new(TraceEventInfo, 1); > + elem->value->vcpu =3D > + trace_event_get_cpu_id(ev) =3D=3D TRACE_CPU_EVENT_COUNT ? = false : true; I'm not a fan of the over-verbose 'cond ? false : true'. It can almost always be written '!cond' with just as much clarity. In your case: elem->value->vcpu =3D trace_event_get_cpu_id(ev) !=3D TRACE_CPU_EVENT_COU= NT; --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --ORVE0at26AR5gWGLa9NWfBwNsjMgSwMu4 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWHUByAAoJEKeha0olJ0NqYusH/2pzFxlw2x9iU84c20XTFRrL ir4CNITiW8XL+rILPJPICcykEd++TQ3UiShbiZWuL0Nw7iuk1KBD8+eirFFi0nkT VxUS6YDsGhqFcsCJjX0kkGANvgqpF9ZAvuG8btZNWGGyX7tEpqq8XoWsO5N7/tC+ fZ9ABoPw2o0ognPXwOGvc9sM5EFj5JCIpfE1FTo7X4tKdQnxBfy1xL+wA3Xi9bOH nGhgAs8R08TB5MFKmI/7bOk3YJaOZ2VGGDaeqseYL++MQC8f0pVUaRMQFOKE7xm5 zR/O+pR66/hO9NkmOPSQqTyNJjTsh3SdQe57cgnucm8ASwLsyI/YIP8kvGyxwFo= =MxtH -----END PGP SIGNATURE----- --ORVE0at26AR5gWGLa9NWfBwNsjMgSwMu4--