Re: [Qemu-devel] [PATCH V6 06/29] monitor: add an implemention as qapi event emit method

[Wenchao Xia]

于 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 <address@hidden>
> > ---
> >   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.