From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60637) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvWlr-0004n6-BY for qemu-devel@nongnu.org; Fri, 13 Jun 2014 15:04:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WvWlk-00084Q-Gz for qemu-devel@nongnu.org; Fri, 13 Jun 2014 15:04:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9215) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvWlk-00084K-7y for qemu-devel@nongnu.org; Fri, 13 Jun 2014 15:04:08 -0400 Message-ID: <539B4B23.60807@redhat.com> Date: Fri, 13 Jun 2014 13:04:03 -0600 From: Eric Blake MIME-Version: 1.0 References: <1401970944-18735-1-git-send-email-wenchaoqemu@gmail.com> <1401970944-18735-7-git-send-email-wenchaoqemu@gmail.com> In-Reply-To: <1401970944-18735-7-git-send-email-wenchaoqemu@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="wJcUdirwqXWd8uSL3iWE4OjnwK77ODmLG" Subject: Re: [Qemu-devel] [PATCH V6 06/29] monitor: add an implemention as qapi event emit method List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia , qemu-devel@nongnu.org Cc: mdroth@linux.vnet.ibm.com, armbru@redhat.com, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --wJcUdirwqXWd8uSL3iWE4OjnwK77ODmLG Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 06/05/2014 06:22 AM, Wenchao Xia wrote: In the subject: s/as/of/ > Now monitor has been hooked on the new event mechanism, so the patches s/Now/The/ > later can convert event callers one by one. Most code are copied from s/the patches later/that later patches/ s/are/is/ > old monitor_protocol_* functions with some modification. >=20 > Note that two build time warnings will be raised after this patch. One = is > caused by no caller of monitor_qapi_event_throttle(), the other one is > caused by QAPI_EVENT_MAX =3D 0. They will be fixed automatically after > full event conversion later. I only got one of those two warnings, about the unused function. What compiler and options are you using to get a warning about QAPI_EVENT_MAX?. Furthermore, since the default 'configure' turns warnings into errors, this would be a build-breaker that hurts 'git bisect'. I think it's easy enough to avoid, if you would please squash this in: diff --git i/monitor.c w/monitor.c index 952e1cb..a593d17 100644 --- i/monitor.c +++ w/monitor.c @@ -594,6 +594,7 @@ static void monitor_qapi_event_handler(void *opaque) * more than 1 event will be emitted within @rate * milliseconds */ +__attribute__((unused)) /* FIXME remove once in use */ static void monitor_qapi_event_throttle(QAPIEvent event, int64_t rate) { MonitorQAPIEventState *evstate; You can then remove that attribute later in 29/29 when you delete everything else about the older implementation. At this point, I think we're racking up enough fixes to be worth a v7 respin (particularly since avoiding 'git bisect' breakage cannot be done as a followup patch, but has to be done in-place in the series). >=20 > Signed-off-by: Wenchao Xia > --- > monitor.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++++++= +++++++- > trace-events | 4 ++ > 2 files changed, 131 insertions(+), 1 deletions(-) >=20 > =20 > +typedef struct MonitorQAPIEventState { > + QAPIEvent event; /* Event being tracked */ > + int64_t rate; /* Period over which to throttle. 0 to disable= */ > + int64_t last; /* Time at which event was last emitted */ in what unit? [1]... > + QEMUTimer *timer; /* Timer for handling delayed events */ > + QObject *data; /* Event pending delayed dispatch */ Any reason the comments aren't aligned? > +} MonitorQAPIEventState; > + > struct Monitor { > CharDriverState *chr; > int mux_out; > @@ -489,6 +499,122 @@ static const char *monitor_event_names[] =3D { > QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) !=3D QEVENT_MAX) > =20 > static MonitorEventState monitor_event_state[QEVENT_MAX]; > +static MonitorQAPIEventState monitor_qapi_event_state[QAPI_EVENT_MAX];= If you are still seeing a compiler warning about allocating an array of size 0, you could likewise try this hack: /* FIXME Hack a minimum array size until we have events */ static MonitorQAPIEventState monitor_qapi_event_state[QAPI_EVENT_MAX + !QAPI_EVENT_MAX]; and likewise clean that up in 29/29 > +static void > +monitor_qapi_event_queue(int event_kind, QDict *data, Error **errp) This is not the usual formatting in qemu. > +{ > + MonitorQAPIEventState *evstate; > + assert(event_kind < QAPI_EVENT_MAX); This doesn't filter out negative values for event_kind. Is it worth the extra paranoia to either make event_kind unsigned, or to assert that event_kind >=3D 0? > + QAPIEvent event =3D (QAPIEvent)event_kind; Why are we casting here? Would it not make more sense to change the signature in patch 2/29 to use the enum type from the get-go? typedef void (*QMPEventFuncEmit)(QAPIEvent event_kind, QDict *dict, Error **errp); and adjust the signature of this function to match > + int64_t now =3D qemu_clock_get_ns(QEMU_CLOCK_REALTIME); =2E..[1] okay, it looks like 'rate' and 'last' are specified in nanoseconds. Worth documenting in their declaration above. Particularly since [1]... > +static void monitor_qapi_event_handler(void *opaque) > +{ > + MonitorQAPIEventState *evstate =3D opaque; > + int64_t now =3D qemu_clock_get_ns(QEMU_CLOCK_REALTIME); > + > + > + trace_monitor_qapi_event_handler(evstate->event, Why the double blank line? > + > +/* > + * @event: the event ID to be limited > + * @rate: the rate limit in milliseconds > + * > + * Sets a rate limit on a particular event, so no > + * more than 1 event will be emitted within @rate > + * milliseconds > + */ > +static void monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)= =2E..[1] this function is documented in milliseconds, not nanoseconds, an= d you are scaling internally. > +{ > + MonitorQAPIEventState *evstate; > + assert(event < QAPI_EVENT_MAX); > + > + evstate =3D &(monitor_qapi_event_state[event]); > + > + trace_monitor_qapi_event_throttle(event, rate); > + evstate->event =3D event; > + evstate->rate =3D rate * SCALE_MS; This value can overflow if a user passes in an insanely large rate. Do we care? > + evstate->last =3D 0; > + evstate->data =3D NULL; > + evstate->timer =3D timer_new(QEMU_CLOCK_REALTIME, > + SCALE_MS, > + monitor_qapi_event_handler, > + evstate); > @@ -507,7 +633,6 @@ monitor_protocol_event_emit(MonitorEvent event, > } > } > =20 > - > /* Why the spurious line deletion? > +++ b/trace-events > @@ -932,6 +932,10 @@ monitor_protocol_event_handler(uint32_t event, voi= d *data, uint64_t last, uint64 > monitor_protocol_event_emit(uint32_t event, void *data) "event=3D%d da= ta=3D%p" > monitor_protocol_event_queue(uint32_t event, void *data, uint64_t rate= , uint64_t last, uint64_t now) "event=3D%d data=3D%p rate=3D%" PRId64 " l= ast=3D%" PRId64 " now=3D%" PRId64 > monitor_protocol_event_throttle(uint32_t event, uint64_t rate) "event=3D= %d rate=3D%" PRId64 > +monitor_qapi_event_emit(uint32_t event, void *data) "event=3D%d data=3D= %p" > +monitor_qapi_event_queue(uint32_t event, void *data, uint64_t rate, ui= nt64_t last, uint64_t now) "event=3D%d data=3D%p rate=3D%" PRId64 " last=3D= %" PRId64 " now=3D%" PRId64 > +monitor_qapi_event_handler(uint32_t event, void *data, uint64_t last, = uint64_t now) "event=3D%d data=3D%p last=3D%" PRId64 " now=3D%" PRId64 > +monitor_qapi_event_throttle(uint32_t event, uint64_t rate) "event=3D%d= rate=3D%" PRId64 Any reason you wanted to invent four new trace names, even though the existing 4 traces have the same signatures? I'm wondering whether it would have just been better to use the old trace names. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --wJcUdirwqXWd8uSL3iWE4OjnwK77ODmLG Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTm0sjAAoJEKeha0olJ0Nqf4IH/2KrzSApwES9ePi/lZc1m/hT poAv8626JEEDJb6J2vJltk0c3h9ZGtHlzbBMuJqIXnf86Fts0i0pPPUsIDToE0g3 DZPYWsSL45vM9nZ3NHV/7mLF+1XUMPRIlswmfBr3hjvYXXMuywwBDiQgAG4aTEI8 W1ltrsB/OyoSZUnJOSOhcmyU62UyMog/xMjXaH0X3G0YboDl+kXmv5nyVX/37Bol nPAbqrA1MkmogxOnvEnClSxWOWcNfd0COBnRsroSLHKk7Nz9jZVWCbNKvVpaKbnB vY3sDtfT/PXbTpVXZgw81DNhaTirmndt5MaRisD60zJFiif0BRyj9G3neNF/IyU= =O0+s -----END PGP SIGNATURE----- --wJcUdirwqXWd8uSL3iWE4OjnwK77ODmLG--