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

[Luiz Capitulino]

On Tue, 05 Nov 2013 13:31:09 +0800
Wenchao Xia <address@hidden> wrote:

> 于 2013/11/5 10:51, 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.
> >
> 
>    It seems a different direction: let QAPI generator recognize
> keyword 'event', and generate emit functions.

Yes, the same way we have it for QMP commands.

> My draft is a bit different:
> caller:
> 
>    QMPEvent *e = g_malloc0(sizeof(QMPEvent));
>    e->kind = QMP_EVENT_TYPE_DEVICE_TRAY_MOVED;
>    e->device_tray_moved = g_malloc0(sizeof(QMPEventDeviceTrayMoved));
>    e->device_tray_moved->device = g_strdup("ide0-hd");
>    e->device_tray_moved->tray_open = false;
>    qmp_event_generate(e);
>    qapi_free_QMPEvent(e);
> 
>    Then qmp_event_generate() will use visitors to build qobject and then
> emit it.

Seems a lot more complex to me, we're going to write a lot of code if
we do this.

>    Compared with above, I think qmp_send_event_device_tray_moved()
> method saves malloc and free calls, other things are similar(My draft
> tells the caller how to set value by structure define, while your way
> tells it by function define), but it may require more modification
> for genrator scripts which I need to rework on, so wonder whether the
> easier way is acceptable.

We'll add event support to the code generator only once, then all
a programmer has to do to add a new event is to add an new entry
in the qapi-schema.json and call the generated qmp_send_event_()
function from the appropriate place. That's trivial compared to
what you showed above. Besides, there are other advantages. One of
them is adding support to allow for C code to listen for events,
which we might want to have in the future and builds nicely on
top of having the QAPI event type.