Re: [Qemu-devel] [PATCH V6 08/29] qapi event: convert SHUTDOWN

[Wenchao Xia <wenchaoqemu@gmail.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 <wenchaoqemu@gmail.com>
>> ---
>>   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.