| Message ID | 539B4B23.60807@redhat.com |
|---|---|
| State | New |
| Headers | show |
δΊ 2014/6/14 3:04, Eric Blake ει: > 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: > The QAPI_EVENT_MAX = 0 cause a warning for code: monitor_qapi_event_throttle(QAPIEvent event, int64_t rate) { .... assert(event < QAPI_EVENT_MAX); } which is always false now. Perhaps change it as assert((int)event < QAPI_EVENT_MAX); ? > 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). > Thanks for tipping, it is a good way to go. >> >> Signed-off-by: Wenchao Xia <wenchaoqemu@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? Will fix. > >> +} 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. Fixing it. > >> +{ >> + 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? > Build waring will be raised for QAPI_EVENT_MAX = 0, same as above, any better way to solve? >> + 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 > Since QAPIEvent is generated by qapi-event.py, and they are different in qemu code and test code, so there will be conflict in test code which include qmp-event.h: in qemu: qmp-event.h include qapi-event.h in test: qmp-event.h include test-qapi-event.h For simple I just used int type instead of QAPIEvent type. To use QAPIEvent in declaration, I think there would be some tricks in test, and doc that using qapi-event.h define in qmp-event.c may break test code, the encapsulation is not very goood. >> + 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? > Copied code, will fix. >> + >> +/* >> + * @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? > There is not much existing caller, maybe we can change it as nonoseconds. >> + 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? > Will remove. >> +++ 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. > The old trace functions are still in use of old event code, so I added fource new ones. The old ones should be removed in cleanup patch.
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;