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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3112 bytes --]

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.

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]