Re: [Qemu-devel] [RFC 1/3] monitor: split MonitorQAPIEventState

[Eric Blake <eblake@redhat.com>]

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

On 08/12/2015 01:46 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Create a seperate pending event structure MonitorQAPIEventPending.

s/seperate/separate/

> Use a MonitorQAPIEventDelay callback to handle the delaying. This
> allows other implementations of throttling.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c    | 124 +++++++++++++++++++++++++++++++++++++----------------------
>  trace-events |   2 +-
>  2 files changed, 79 insertions(+), 47 deletions(-)
> 

>  
> +typedef struct MonitorQAPIEventPending {
> +    QAPIEvent event;    /* Event being tracked */
> +    int64_t last;       /* QEMU_CLOCK_REALTIME value at last emission */
> +    QEMUTimer *timer;   /* Timer for handling delayed events */
> +    QObject *data;      /* Event pending delayed dispatch */
> +} MonitorQAPIEventPending;
> +
> +typedef struct MonitorQAPIEventState MonitorQAPIEventState;

Some places combine the typedef with the struct definition; I'm not sure
there's any hard and fast rule, though. (HACKING mentions that we want
the typedef, but doesn't give guidelines on how it must be provided).

> +struct MonitorQAPIEventState {
>      int64_t rate;       /* Minimum time (in ns) between two events */
> -    int64_t last;       /* QEMU_CLOCK_REALTIME value at last emission */
> -    QEMUTimer *timer;   /* Timer for handling delayed events */
> -    QObject *data;      /* Event pending delayed dispatch */
> -} MonitorQAPIEventState;
> +    MonitorQAPIEventDelay delay;
> +    gpointer data;

Do we really need 'gpointer', or is 'void *' sufficient?

>  
> +static bool
> +monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
> +{
> +    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 */
> +    if (!evstate->rate) {
> +        p->last = now;
> +        return FALSE;

s/FALSE/false/ (we want to directly use the C99 'bool' type here, not
the glib macros that expand to who-knows-what-type followed by implicit
conversion back to bool).

> +        p->data = QOBJECT(data);
> +        qobject_incref(p->data);
> +        return TRUE;
> +    }
> +
> +    p->last = now;
> +    return FALSE;

two more ALL_CAPS to convert to the lower bool counterpart.

Otherwise looks like a sane split.

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