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.
>
> 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 = 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).
>
> Signed-off-by: Wenchao Xia <wenchaoq...@gmail.com>
> ---
> monitor.c | 128
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> trace-events | 4 ++
> 2 files changed, 131 insertions(+), 1 deletions(-)
>
>
> +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[] = {
> QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
>
> 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 >= 0?
> + QAPIEvent event = (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 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
...[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 = opaque;
> + int64_t now = 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)
...[1] this function is documented in milliseconds, not nanoseconds, and
you are scaling internally.
> +{
> + MonitorQAPIEventState *evstate;
> + assert(event < QAPI_EVENT_MAX);
> +
> + evstate = &(monitor_qapi_event_state[event]);
> +
> + trace_monitor_qapi_event_throttle(event, rate);
> + evstate->event = event;
> + evstate->rate = rate * SCALE_MS;
This value can overflow if a user passes in an insanely large rate. Do
we care?
> + evstate->last = 0;
> + evstate->data = NULL;
> + evstate->timer = timer_new(QEMU_CLOCK_REALTIME,
> + SCALE_MS,
> + monitor_qapi_event_handler,
> + evstate);
> @@ -507,7 +633,6 @@ monitor_protocol_event_emit(MonitorEvent event,
> }
> }
>
> -
> /*
Why the spurious line deletion?
> +++ b/trace-events
> @@ -932,6 +932,10 @@ monitor_protocol_event_handler(uint32_t event, void
> *data, uint64_t last, uint64
> monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
> monitor_protocol_event_queue(uint32_t event, void *data, uint64_t rate,
> uint64_t last, uint64_t now) "event=%d data=%p rate=%" PRId64 " last=%"
> PRId64 " now=%" PRId64
> monitor_protocol_event_throttle(uint32_t event, uint64_t rate) "event=%d
> rate=%" PRId64
> +monitor_qapi_event_emit(uint32_t event, void *data) "event=%d data=%p"
> +monitor_qapi_event_queue(uint32_t event, void *data, uint64_t rate, uint64_t
> last, uint64_t now) "event=%d data=%p rate=%" PRId64 " last=%" PRId64 "
> now=%" PRId64
> +monitor_qapi_event_handler(uint32_t event, void *data, uint64_t last,
> uint64_t now) "event=%d data=%p last=%" PRId64 " now=%" PRId64
> +monitor_qapi_event_throttle(uint32_t event, uint64_t rate) "event=%d rate=%"
> 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
