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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6774 bytes --]

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 <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?

> +} 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]