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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

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

   OK, it is a good enough reason, I'll write qapi-event.py to get
qapi-event.h and qapi-event.c.

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