Re: [Qemu-devel] [PATCH 2/6] qapi: rename MonitorEvent to QEvent

[Luiz Capitulino]

On Tue, 05 Nov 2013 10:17:28 +0800
Wenchao Xia <address@hidden> wrote:

> 于 2013/11/4 21:33, Luiz Capitulino 写道:
> > On Mon, 04 Nov 2013 09:59:50 +0800
> > Wenchao Xia <address@hidden> wrote:
> >
> >> 于 2013/11/1 22:02, Luiz Capitulino 写道:
> >>> On Mon, 21 Oct 2013 10:16:01 +0800
> >>> Wenchao Xia <address@hidden> wrote:
> >>>
> >>>> Signed-off-by: Wenchao Xia <address@hidden>
> >>>> ---
> >>>>    block.c                    |    2 +-
> >>>>    include/block/block_int.h  |    2 +-
> >>>>    include/monitor/monitor.h  |    6 +++---
> >>>>    monitor.c                  |   12 ++++++------
> >>>>    stubs/mon-protocol-event.c |    2 +-
> >>>>    ui/vnc.c                   |    2 +-
> >>>>    6 files changed, 13 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/block.c b/block.c
> >>>> index 2c15e5d..458a4f8 100644
> >>>> --- a/block.c
> >>>> +++ b/block.c
> >>>> @@ -1760,7 +1760,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const 
> >>>> BlockDevOps *ops,
> >>>>    }
> >>>>
> >>>>    void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
> >>>> -                               MonitorEvent ev,
> >>>> +                               QEvent ev,
> >>>>                                   BlockErrorAction action, bool is_read)
> >>>>    {
> >>>>        QObject *data;
> >>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
> >>>> index bcc72e2..bfdaf84 100644
> >>>> --- a/include/block/block_int.h
> >>>> +++ b/include/block/block_int.h
> >>>> @@ -337,7 +337,7 @@ AioContext *bdrv_get_aio_context(BlockDriverState 
> >>>> *bs);
> >>>>    int is_windows_drive(const char *filename);
> >>>>    #endif
> >>>>    void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
> >>>> -                               MonitorEvent ev,
> >>>> +                               QEvent ev,
> >>>>                                   BlockErrorAction action, bool is_read);
> >>>>
> >>>>    /**
> >>>> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> >>>> index 10fa0e3..8b14a6f 100644
> >>>> --- a/include/monitor/monitor.h
> >>>> +++ b/include/monitor/monitor.h
> >>>> @@ -20,7 +20,7 @@ extern Monitor *default_mon;
> >>>>    #define MONITOR_CMD_ASYNC       0x0001
> >>>>
> >>>>    /* QMP events */
> >>>> -typedef enum MonitorEvent {
> >>>> +typedef enum QEvent {
> >>>
> >>> Qt has a QEvent class, so QEvent is not a good name for us if we're
> >>> considering making it public in the schema (which could become an
> >>> external library in the distant future).
> >>>
> >>> I suggest calling it QMPEvent.
> >>>
> >>
> >>     Maybe QMPEventType, since QMPEvent should be used an union?
> >
> > If we add the 'event' type, like:
> >
> > { 'event': 'BLOCK_IO_ERROR', 'data': { ... } }
> >
> > Then we don't need an union.
> >
> 
>    It would bring a little trouble to C code caller, for example the
> event generate function(just like monitor_protocol_event) would be:
> event_generate(QMPEvent *e);

Hmm, no.

Today, events are open-coded and an event's definition lives in the code,
like the DEVICE_TRAY_MOVED event:

static void bdrv_emit_qmp_eject_event(BlockDriverState *bs, bool ejected)
{
    QObject *data;

    data = qobject_from_jsonf("{ 'device': %s, 'tray-open': %i }",
                              bdrv_get_device_name(bs), ejected);
    monitor_protocol_event(QEVENT_DEVICE_TRAY_MOVED, data);

    qobject_decref(data);
}

Having an union for the event's names saves some typing elsewhere, but
it doesn't solve the problem of having the event definition in the code.
Adding event type support to the QAPI does solve this problem.

Taking the DEVICE_TRAY_MOVED event as an example, we would have the
following entry in the qapi-schema.json file:

{ 'event': 'DEVICE_TRAY_MOVED', 'data': { 'device': 'str',
                                          'tray-open': 'bool' } }

Then the QAPI generator would generate this function:

void qmp_send_event_device_tray_moved(const char *device, bool tray_open);

This function uses visitors to build a qobject which is then passed
to monitor_protocol_event(), along with the event name. Also note that
monitor_protocol_event() could take the event name as a string, which
completely eliminates the current enum MonitorEvent.

I believe this is the direction we want to go wrt events.