Re: [Qemu-devel] [PATCH v2 4/8] dump-guest-memory: add qmp event DUMP_COMPLETED

[Peter Xu <peterx@redhat.com>]

On Mon, Nov 30, 2015 at 11:18:48AM -0700, Eric Blake wrote:
> On 11/26/2015 07:48 PM, Peter Xu wrote:
> > To get aligned with QMP interface, one new QMP event DUMP_COMPLETED
> > is added. It is used when user specified "detach" in dump, and
> > triggered when the dump finishes. Error message will be appended to
> > this event if the dump has failed.
> 
> Why not emit the new event unconditionally, instead of only when detach
> was specified?

I was just trying to make sure that dump will work exactly as it is
when detach is not used. Yes, I think I could emit it
unconditionally if libvirt does not complain about it. :)

> 
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  docs/qmp-events.txt | 12 ++++++++++++
> >  dump.c              | 12 +++++++++++-
> >  qapi/event.json     | 10 ++++++++++
> >  3 files changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
> > index d2f1ce4..fe494f9 100644
> > --- a/docs/qmp-events.txt
> > +++ b/docs/qmp-events.txt
> > @@ -674,3 +674,15 @@ Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
> >  followed respectively by the RESET, SHUTDOWN, or STOP events.
> >  
> >  Note: this event is rate-limited.
> > +
> > +DUMP_COMPLETED
> > +--------------
> > +
> > +Emitted when the guest has finished one memory dump.
> > +
> > +Data: None.
> 
> Wrong - you have 'msg' as data.  Except that Paolo is right, 'msg'
> should be optional, and only present on error.  You should also document
> that the contents of 'msg' are for human consumption and should not be
> machine-parsed (basically, only the presence of absence of 'msg' is
> useful for machines).

I will make sure v4 contains these suggestions. Thanks. 

> 
> > +
> > +Example:
> > +
> > +{ "event": "DUMP_COMPLETED",
> > +  "data": { "msg": "Dump completed successfully" } }
> > diff --git a/dump.c b/dump.c
> 
> > +++ b/qapi/event.json
> > @@ -356,3 +356,13 @@
> >  ##
> >  { 'event': 'MEM_UNPLUG_ERROR',
> >    'data': { 'device': 'str', 'msg': 'str' } }
> > +
> > +##
> > +# @DUMP_COMPLETED
> > +#
> > +# Emitted when background dump has completed
> > +#
> 
> Missing documentation of 'msg', which should be optional.

Sorry I missed that. Thanks to point out!

Peter

> 
> > +# Since: 2.6
> > +##
> > +{ 'event': 'DUMP_COMPLETED' ,
> > +  'data': { 'msg': 'str' } }
> > 
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>