From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44016) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vdtl5-0004SV-0g for qemu-devel@nongnu.org; Tue, 05 Nov 2013 22:26:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vdtkw-0007Lg-L6 for qemu-devel@nongnu.org; Tue, 05 Nov 2013 22:26:18 -0500 Received: from e23smtp08.au.ibm.com ([202.81.31.141]:33443) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vdtkv-0007Kl-Va for qemu-devel@nongnu.org; Tue, 05 Nov 2013 22:26:10 -0500 Received: from /spool/local by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 6 Nov 2013 13:25:57 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id DE5EC2BB0055 for ; Wed, 6 Nov 2013 14:25:54 +1100 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id rA638Asj3670384 for ; Wed, 6 Nov 2013 14:08:16 +1100 Received: from d23av04.au.ibm.com (localhost [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id rA63PmKh011054 for ; Wed, 6 Nov 2013 14:25:48 +1100 Message-ID: <5279B6BE.9050809@linux.vnet.ibm.com> Date: Wed, 06 Nov 2013 11:25:50 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1382321765-29052-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1382321765-29052-3-git-send-email-xiawenc@linux.vnet.ibm.com> <20131101100231.5c2aa5a5@redhat.com> <5276FF96.20702@linux.vnet.ibm.com> <20131104083339.79a190ce@redhat.com> <52785538.8080103@linux.vnet.ibm.com> <20131104215118.32b17eb1@redhat.com> <5278829D.90905@linux.vnet.ibm.com> <20131105090612.010a6e46@redhat.com> In-Reply-To: <20131105090612.010a6e46@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH 2/6] qapi: rename MonitorEvent to QEvent List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com 于 2013/11/5 22:06, Luiz Capitulino 写道: > On Tue, 05 Nov 2013 13:31:09 +0800 > Wenchao Xia wrote: > >> 于 2013/11/5 10:51, Luiz Capitulino 写道: >>> On Tue, 05 Nov 2013 10:17:28 +0800 >>> Wenchao Xia wrote: >>> >>>> 于 2013/11/4 21:33, Luiz Capitulino 写道: >>>>> On Mon, 04 Nov 2013 09:59:50 +0800 >>>>> Wenchao Xia wrote: >>>>> >>>>>> 于 2013/11/1 22:02, Luiz Capitulino 写道: >>>>>>> On Mon, 21 Oct 2013 10:16:01 +0800 >>>>>>> Wenchao Xia wrote: >>>>>>> >>>>>>>> Signed-off-by: Wenchao Xia >>>>>>>> --- >>>>>>>> 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. >