From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49498) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvyNE-0005X6-Jg for qemu-devel@nongnu.org; Sat, 14 Jun 2014 20:32:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WvyN5-0001dN-FS for qemu-devel@nongnu.org; Sat, 14 Jun 2014 20:32:40 -0400 Received: from mail-pb0-x232.google.com ([2607:f8b0:400e:c01::232]:47402) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvyN5-0001dG-8i for qemu-devel@nongnu.org; Sat, 14 Jun 2014 20:32:31 -0400 Received: by mail-pb0-f50.google.com with SMTP id rp16so3296574pbb.9 for ; Sat, 14 Jun 2014 17:32:30 -0700 (PDT) Message-ID: <539CE997.9010602@gmail.com> Date: Sun, 15 Jun 2014 08:32:23 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1401970944-18735-1-git-send-email-wenchaoqemu@gmail.com> <1401970944-18735-9-git-send-email-wenchaoqemu@gmail.com> <539B57AB.9050000@redhat.com> In-Reply-To: <539B57AB.9050000@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V6 08/29] qapi event: convert SHUTDOWN List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: mdroth@linux.vnet.ibm.com, armbru@redhat.com, lcapitulino@redhat.com 于 2014/6/14 3:57, Eric Blake 写道: > On 06/05/2014 06:22 AM, Wenchao Xia wrote: >> This patch also eliminates build time warning caused by >> QAPI_EVENT_MAX = 0. > > I still don't know why I wasn't seeing a warning for that, but agree > this cleans it up (or whichever event gets converted first, as there > aren't really any dependency restrictions in the order in which you do > conversions). If you do follow my suggestion of adding a workaround in > 6/29 to avoid build warnings, then don't undo it here, but save it until > 29/29; that way, no one individual conversion is doing more work than > any other. > >> >> Signed-off-by: Wenchao Xia >> --- >> docs/qmp/qmp-events.txt | 15 --------------- >> qapi-event.json | 12 ++++++++++++ >> vl.c | 3 ++- >> 3 files changed, 14 insertions(+), 16 deletions(-) > > Yay - I finally have enough to see what code gets generated in 3/29. > The qapi-event.h header now has: > > void qapi_event_send_shutdown(Error **errp); > > extern const char *QAPIEvent_lookup[]; > typedef enum QAPIEvent > { > QAPI_EVENT_SHUTDOWN = 0, > QAPI_EVENT_MAX = 1, > } QAPIEvent; > > > and the .c file has: > > void qapi_event_send_shutdown(Error **errp) > { > QDict *qmp; > Error *local_err = NULL; > QMPEventFuncEmit emit; > emit = qmp_event_get_func_emit(); > if (!emit) { > return; > } > > qmp = qmp_event_build_dict("SHUTDOWN"); > > emit(QAPI_EVENT_SHUTDOWN, qmp, &local_err); > > error_propagate(errp, local_err); > QDECREF(qmp); > } > > > Looks good, for a data-free event (I guess I'll have to wait till later > in the series to see what gets generated for an event with data). Hmm, > I wonder if patch 3/29 should have also modified docs/qapi-code-gen.txt > to show a sample generated function. > Will add doc part. >> >> - >> -Emitted when the Virtual Machine is powered down. > >> +++ b/qapi-event.json >> @@ -0,0 +1,12 @@ >> +## >> +# @SHUTDOWN >> +# >> +# Emitted when the virtual machine shutdown, qemu terminated the emulation and >> +# is about to exit. > > Different wording than the text it replaces, and the grammar is a bit > unusual. Maybe: > > Emitted when the virtual machine has shut down, indicating that qemu is > about to exit. > >> if (qemu_shutdown_requested()) { >> qemu_kill_report(); >> - monitor_protocol_event(QEVENT_SHUTDOWN, NULL); >> + qapi_event_send_shutdown(NULL); > > Note that the two NULLs serve different purposes. In the old code, NULL > meant there was no additional data. In the new code, NULL indicates > that we are ignoring the possibility for error. I wonder if it would be > better to pass &error_abort instead of NULL? For that matter, I just > re-read 6/29 - the one implementation we have of an emit function > (monitor_qapi_event_queue) never sets errp. Is it better to just > consider events as best-effort, and completely ditch the errp parameter > from the emit callback typedef in 2/29, since it looks like you intend > to pass NULL for all clients of the callback? > It make things simple, I will remove &errp in next version.