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

[Wenchao Xia <wenchaoqemu@gmail.com>]

于 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.