On Tue, 05 Nov 2013 13:31:09 +0800 Wenchao Xia <xiaw...@linux.vnet.ibm.com> wrote:
> 于 2013/11/5 10:51, Luiz Capitulino 写道: > > On Tue, 05 Nov 2013 10:17:28 +0800 > > Wenchao Xia <xiaw...@linux.vnet.ibm.com> wrote: > > > >> 于 2013/11/4 21:33, Luiz Capitulino 写道: > >>> On Mon, 04 Nov 2013 09:59:50 +0800 > >>> Wenchao Xia <xiaw...@linux.vnet.ibm.com> wrote: > >>> > >>>> 于 2013/11/1 22:02, Luiz Capitulino 写道: > >>>>> On Mon, 21 Oct 2013 10:16:01 +0800 > >>>>> Wenchao Xia <xiaw...@linux.vnet.ibm.com> wrote: > >>>>> > >>>>>> Signed-off-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com> > >>>>>> --- > >>>>>> 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.