Re: [Qemu-devel] [PATCH v3 08/12] dump-guest-memory: add qmp event DUMP_COMPLETED

2015-11-30 Thread Eric Blake 
On 11/30/2015 04:32 AM, Peter Xu wrote:
> One new QMP event DUMP_COMPLETED is added. It is used when user
> specified "detach" in dump, and triggered when the dump finishes
> (either succeeded or failed). If failed, one "err" data will be
> passed with specific error message.
> 
> Signed-off-by: Peter Xu 
> ---
>  docs/qmp-events.txt | 14 ++
>  dump.c  |  6 +-
>  qapi/event.json | 10 ++
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
> index d2f1ce4..7b9f835 100644
> --- a/docs/qmp-events.txt
> +++ b/docs/qmp-events.txt
> @@ -674,3 +674,17 @@ 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:
> +
> +- "error": Error message when dump failed (json-string, optional)
> +
> +Example:
> +
> +{ "event": "DUMP_COMPLETED",
> +  "data": {} }

Please keep this file sorted.  The insertion should be between
DEVICE_TRAY_MOVED and GUEST_PANICKED.

> diff --git a/dump.c b/dump.c
> index 14fd41f..43f565d 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -25,6 +25,7 @@
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qmp-commands.h"
> +#include "qapi-event.h"
>  
>  #include 
>  #ifdef CONFIG_LZO
> @@ -1633,8 +1634,11 @@ static void *dump_thread(void *data)
>  dump_process(s, );
>  
>  if (err) {
> -/* TODO: notify user the error */
> +qapi_event_send_dump_completed(true, error_get_pretty(err),
> +   _abort);
>  error_free(err);
> +} else {
> +qapi_event_send_dump_completed(false, NULL, _abort);
>  }

Hmmm. I wonder if error_get_pretty() should be improved to return NULL
when there is no error.  Then we could write:

qapi_event_send_dump_completed(!!err, error_get_pretty(err),
   _abort);
error_free(err);

But that doesn't affect the correctness of your patch.

>  return NULL;
>  }
> diff --git a/qapi/event.json b/qapi/event.json
> index f0cef01..c46214b 100644
> --- a/qapi/event.json
> +++ 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
> +#
> +# Since: 2.6

Missing documentation of 'error'.

> +##
> +{ 'event': 'DUMP_COMPLETED' ,
> +  'data': { '*error': 'str' } }
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 08/12] dump-guest-memory: add qmp event DUMP_COMPLETED

2015-11-30 Thread Peter Xu 
On Mon, Nov 30, 2015 at 03:12:31PM -0700, Eric Blake wrote:
> On 11/30/2015 04:32 AM, Peter Xu wrote:
> > +Example:
> > +
> > +{ "event": "DUMP_COMPLETED",
> > +  "data": {} }
> 
> Please keep this file sorted.  The insertion should be between
> DEVICE_TRAY_MOVED and GUEST_PANICKED.

Sorry for the incaution to miss that. Will fix in v4.

> 
> > diff --git a/dump.c b/dump.c
> > index 14fd41f..43f565d 100644
> > --- a/dump.c
> > +++ b/dump.c
> > @@ -25,6 +25,7 @@
> >  #include "qapi/error.h"
> >  #include "qapi/qmp/qerror.h"
> >  #include "qmp-commands.h"
> > +#include "qapi-event.h"
> >  
> >  #include 
> >  #ifdef CONFIG_LZO
> > @@ -1633,8 +1634,11 @@ static void *dump_thread(void *data)
> >  dump_process(s, );
> >  
> >  if (err) {
> > -/* TODO: notify user the error */
> > +qapi_event_send_dump_completed(true, error_get_pretty(err),
> > +   _abort);
> >  error_free(err);
> > +} else {
> > +qapi_event_send_dump_completed(false, NULL, _abort);
> >  }
> 
> Hmmm. I wonder if error_get_pretty() should be improved to return NULL
> when there is no error.  Then we could write:
> 
> qapi_event_send_dump_completed(!!err, error_get_pretty(err),
>_abort);
> error_free(err);
> 
> But that doesn't affect the correctness of your patch.

After seeing that improving error_get_pretty() is simple (and also
will not break the other callers), I'd like to take your advice,
which is clean enough. Thanks.

> 
> >  return NULL;
> >  }
> > diff --git a/qapi/event.json b/qapi/event.json
> > index f0cef01..c46214b 100644
> > --- a/qapi/event.json
> > +++ 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
> > +#
> > +# Since: 2.6
> 
> Missing documentation of 'error'.

Added.

Thanks!
Peter

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

[Qemu-devel] [PATCH v3 08/12] dump-guest-memory: add qmp event DUMP_COMPLETED

2015-11-30 Thread Peter Xu 
One new QMP event DUMP_COMPLETED is added. It is used when user
specified "detach" in dump, and triggered when the dump finishes
(either succeeded or failed). If failed, one "err" data will be
passed with specific error message.

Signed-off-by: Peter Xu 
---
 docs/qmp-events.txt | 14 ++
 dump.c  |  6 +-
 qapi/event.json | 10 ++
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index d2f1ce4..7b9f835 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -674,3 +674,17 @@ 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:
+
+- "error": Error message when dump failed (json-string, optional)
+
+Example:
+
+{ "event": "DUMP_COMPLETED",
+  "data": {} }
diff --git a/dump.c b/dump.c
index 14fd41f..43f565d 100644
--- a/dump.c
+++ b/dump.c
@@ -25,6 +25,7 @@
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
 #include "qmp-commands.h"
+#include "qapi-event.h"
 
 #include 
 #ifdef CONFIG_LZO
@@ -1633,8 +1634,11 @@ static void *dump_thread(void *data)
 dump_process(s, );
 
 if (err) {
-/* TODO: notify user the error */
+qapi_event_send_dump_completed(true, error_get_pretty(err),
+   _abort);
 error_free(err);
+} else {
+qapi_event_send_dump_completed(false, NULL, _abort);
 }
 return NULL;
 }
diff --git a/qapi/event.json b/qapi/event.json
index f0cef01..c46214b 100644
--- a/qapi/event.json
+++ 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
+#
+# Since: 2.6
+##
+{ 'event': 'DUMP_COMPLETED' ,
+  'data': { '*error': 'str' } }
-- 
2.4.3