Hi
On Wed, Sep 16, 2015 at 6:40 PM, Markus Armbruster <arm...@redhat.com> wrote:
> marcandre.lur...@redhat.com writes:
>
>> From: Marc-André Lureau <marcandre.lur...@redhat.com>
>>
>> Use a hash table to lookup the pending event corresponding to the "id"
>> field. The hash table may grow without limit here, the following patch
>> will add some cleaning.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> Reviewed-by: Eric Blake <ebl...@redhat.com>
>> ---
>> monitor.c | 104
>> ++++++++++++++++++++++++++++++++++++++++++++++----------------
>> 1 file changed, 78 insertions(+), 26 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 0a6a16a..7f5c80f 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -462,10 +462,10 @@ static void monitor_qapi_event_emit(QAPIEvent event,
>> QObject *data)
>> }
>>
>> static bool
>> -monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
>> +monitor_qapi_event_pending_update(MonitorQAPIEventState *evstate,
>> + MonitorQAPIEventPending *p, QDict *data)
>
> Function name gets even longer, yet I miss a function comment as much as
> before :)
It's longer simply because the struct name is longer.
The function "update" MonitorQAPIEventPending.
>
> Since this is the common part of monitor_qapi_event_delay() and
> monitor_qapi_event_id_delay(), what about calling it
> monitor_qapi_event_do_delay()?
>
That's confusing, not only it doesn't follow the struct prefix, but
it's also very close to monitor_qapi_event_delay() (the simple
throttle/delay handler)
>> {
>> int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>> - MonitorQAPIEventPending *p = evstate->data;
>> int64_t delta = now - p->last;
>>
>> /* Rate limit of 0 indicates no throttling */
>> @@ -494,31 +494,13 @@ monitor_qapi_event_delay(MonitorQAPIEventState
>> *evstate, QDict *data)
>> return false;
>> }
>>
>> -/*
>> - * Queue a new event for emission to Monitor instances,
>> - * applying any rate limiting if required.
>> - */
>> -static void
>> -monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
>> -{
>> - MonitorQAPIEventState *evstate;
>> - assert(event < QAPI_EVENT_MAX);
>> - int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>> -
>> - evstate = &(monitor_qapi_event_state[event]);
>> - trace_monitor_protocol_event_queue(event,
>> - data,
>> - evstate->rate,
>> - now);
>>
>> - qemu_mutex_lock(&monitor_lock);
>> -
>> - if (!evstate->delay ||
>> - !evstate->delay(evstate, data)) {
>> - monitor_qapi_event_emit(event, QOBJECT(data));
>> - }
>
> No change, just diff being insufficiently smart.
>
>> +static bool
>> +monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
>> +{
>> + MonitorQAPIEventPending *p = evstate->data;
>>
>> - qemu_mutex_unlock(&monitor_lock);
>> + return monitor_qapi_event_pending_update(evstate, p, data);
>> }
>
> Why not simply
>
> return monitor_qapi_event_pending_update(evstate, evstate->data, data);
ok
>
>>
>> /*
>> @@ -558,6 +540,58 @@ monitor_qapi_event_pending_new(QAPIEvent event)
>> return p;
>> }
>>
>> +static bool
>> +monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *data)
>> +{
>> + GHashTable *ht = evstate->data;
>> + QDict *s = qdict_get_qdict(data, "data");
>
> Why is this called s?
>
> Here are less confusing names:
>
> * The parameter should not be called data, because it's the complete QMP
> event object, and *not* its data member. Either match
> QMPEventFuncEmit and call it qdict, or call it event.
>
It was called "data" before, I think because "event" is for QAPIEvent
in general, and the "data" of QDict is the actual event data.
So I guess qdict is a better name, although it's also very generic to me.
> * The variable s *is* the data member, and should therefore be called data.
ok
>
>> + const char *id = qdict_get_str(s, "id");
>
> This assumes s has a member "id" of type QString. Therefore, the
> function can only be used for events where that is always the case.
> Unobvious requirements like that should be spelled out in a function
> comment.
It spelled in the function name, but I added a comment on top.
>> + MonitorQAPIEventPending *p = g_hash_table_lookup(ht, id);
>> + QAPIEvent event = evstate - monitor_qapi_event_state;
>
> Another unobvious requirement: evstate must point into
> monitor_qapi_event_state.
It's an element in the array. I'll add a comment and an assert.
>
>> +
>> + if (!p) {
>> + p = monitor_qapi_event_pending_new(event);
>> + g_hash_table_insert(ht, g_strdup(id), p);
>
> Note for later: both key and value are dynamically allocated.
value has a destroy handler being called when removing the value
The key should also be destroyed, let's fix that leak. thanks
>
>> + }
>> +
>> + return monitor_qapi_event_pending_update(evstate, p, data);
>> +}
>> +
>> +/*
>> + * Queue a new event for emission to Monitor instances,
>> + * applying any rate limiting if required.
>> + */
>> +static void
>> +monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
>> +{
>> + MonitorQAPIEventState *evstate;
>> + assert(event < QAPI_EVENT_MAX);
>> + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>> +
>> + evstate = &(monitor_qapi_event_state[event]);
>> + trace_monitor_protocol_event_queue(event,
>> + data,
>> + evstate->rate,
>> + now);
>> +
>> + qemu_mutex_lock(&monitor_lock);
>> +
>> + if (!evstate->delay ||
>> + !evstate->delay(evstate, data)) {
>> + monitor_qapi_event_emit(event, QOBJECT(data));
>> + }
>> +
>> + qemu_mutex_unlock(&monitor_lock);
>> +}
>> +
>> +static void
>> +monitor_qapi_event_pending_free(MonitorQAPIEventPending *p)
>> +{
>> + timer_del(p->timer);
>> + timer_free(p->timer);
>> + g_free(p);
>
> Ignorant question: who owns p->data?
>
MonitorQAPIEventPending, but the event is freed when emitted. Let's
add a cleanup here too.
>> +}
>> +
>> /*
>> * @event: the event ID to be limited
>> * @rate: the rate limit in milliseconds
>> @@ -582,6 +616,24 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t
>> rate)
>> evstate->data = monitor_qapi_event_pending_new(event);
>> }
>>
>> +static void
>> +monitor_qapi_event_id_throttle(QAPIEvent event, int64_t rate)
>> +{
>> + MonitorQAPIEventState *evstate;
>> + assert(event < QAPI_EVENT_MAX);
>> +
>> + evstate = &(monitor_qapi_event_state[event]);
>
> Superfluous parenthesis.
>
was there before, copy-pasta. removing
>> +
>> + trace_monitor_protocol_event_throttle(event, rate);
>> + assert(rate * SCALE_MS <= INT64_MAX);
>> + evstate->rate = rate * SCALE_MS;
>> +
>> + evstate->delay = monitor_qapi_event_id_delay;
>> + evstate->data =
>> + g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
>> + (GDestroyNotify)monitor_qapi_event_pending_free);
>
> Both key and value are dynamically allocated. But you define only the
> value's free callback. Why?
At first I thought the key would remain in the hashtable, it's a leftover. Fixed
>
>> +}
>> +
>> static void monitor_qapi_event_init(void)
>> {
>> /* Limit guest-triggerable events to 1 per second */
>> @@ -590,7 +642,7 @@ static void monitor_qapi_event_init(void)
>> monitor_qapi_event_throttle(QAPI_EVENT_BALLOON_CHANGE, 1000);
>> monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000);
>> monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000);
>> - monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
>> + monitor_qapi_event_id_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
>>
>> qmp_event_set_func_emit(monitor_qapi_event_queue);
>> }
>
--
Marc-André Lureau
