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

[Fam Zheng <famz@redhat.com>]

On Tue, 12/01 21:28, Peter Xu wrote:
> One new QMP event DUMP_COMPLETED is added. When a dump finishes, one
> DUMP_COMPLETED event will occur to notify the user.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  docs/qmp-events.txt | 16 ++++++++++++++++
>  dump.c              | 11 +++++------
>  qapi-schema.json    |  3 ++-
>  qapi/event.json     | 13 +++++++++++++
>  qmp-commands.hx     |  5 +++--
>  util/error.c        |  6 +++++-
>  6 files changed, 44 insertions(+), 10 deletions(-)
> 
> diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
> index d2f1ce4..1f79588 100644
> --- a/docs/qmp-events.txt
> +++ b/docs/qmp-events.txt
> @@ -220,6 +220,22 @@ Data:
>    },
>    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>  
> +DUMP_COMPLETED
> +--------------
> +
> +Emitted when the guest has finished one memory dump.
> +
> +Data:
> +
> +- "error": Error message when dump failed. This is only a
> +  human-readable string provided when dump failed. It should not be
> +  parsed in any way (json-string, optional)
> +
> +Example:
> +
> +{ "event": "DUMP_COMPLETED",
> +  "data": {} }
> +
>  GUEST_PANICKED
>  --------------
>  
> diff --git a/dump.c b/dump.c
> index c86bc2d..5b040b7 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 <zlib.h>
>  #ifdef CONFIG_LZO
> @@ -1612,6 +1613,9 @@ static void dump_process(DumpState *s, Error **errp)
>          s->status = DUMP_STATUS_COMPLETED;
>      }
>  
> +    /* send DUMP_COMPLETED message (unconditionally) */
> +    qapi_event_send_dump_completed(!!(*errp), error_get_pretty(*errp),
> +                                   &error_abort);
>      dump_cleanup(s);
>  }
>  
> @@ -1619,13 +1623,8 @@ static void *dump_thread(void *data)
>  {
>      Error *err = NULL;
>      DumpState *s = (DumpState *)data;
> -
>      dump_process(s, &err);
> -
> -    if (err) {
> -        /* TODO: notify user the error */
> -        error_free(err);
> -    }
> +    error_free(err);
>      return NULL;
>  }
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 691a130..f0d3c4a 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2116,7 +2116,8 @@
>  #               is the fd's name.
>  #
>  # @detach: #optional if true, QMP will return immediately rather than
> -#          waiting for the dump to finish. (since 2.6).
> +#          waiting for the dump to finish. A DUMP_COMPLETED event will
> +#          occur at the end. (since 2.6).
>  #
>  # @begin: #optional if specified, the starting physical address.
>  #
> diff --git a/qapi/event.json b/qapi/event.json
> index f0cef01..9b7f714 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -356,3 +356,16 @@
>  ##
>  { 'event': 'MEM_UNPLUG_ERROR',
>    'data': { 'device': 'str', 'msg': 'str' } }
> +
> +##
> +# @DUMP_COMPLETED
> +#
> +# Emitted when background dump has completed
> +#
> +# @error: #optional human-readable error string that provides
> +#         hint on why dump failed.

Please explicitly mention that successful dump emits DUMP_COMPLETED without
error, and failed dump emits DUMP_COMPLETED that has an error str.

> +#
> +# Since: 2.6
> +##
> +{ 'event': 'DUMP_COMPLETED' ,
> +  'data': { '*error': 'str' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6b51585..7b6f915 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -857,8 +857,9 @@ Arguments:
>  - "paging": do paging to get guest's memory mapping (json-bool)
>  - "protocol": destination file(started with "file:") or destination file
>                descriptor (started with "fd:") (json-string)
> -- "detach": if specified, command will return immediately, without waiting
> -            for the dump to finish (json-bool)
> +- "detach": if specified, command will return immediately rather than waiting
> +            for the dump completion. A DUMP_COMPLETED event will occur at
> +            the end. (json-bool)
>  - "begin": the starting physical address. It's optional, and should be specified
>             with length together (json-int)
>  - "length": the memory size, in bytes. It's optional, and should be specified
> diff --git a/util/error.c b/util/error.c
> index 80c89a2..645b9af 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -197,7 +197,11 @@ ErrorClass error_get_class(const Error *err)
>  
>  const char *error_get_pretty(Error *err)
>  {
> -    return err->msg;
> +    if (err) {
> +        return err->msg;
> +    } else {
> +        return NULL;
> +    }

This change belongs to a separate patch, if any. But personally I don't like
it, because it doesn't work very well when error_get_pretty is used in
printf-like function parameters:

    Error *err = NULL;
    error_report("error: %s", error_get_pretty(err));

will print "error: (null)" which is ugly, in which case the caller need to
check the pointer anyway. And that is the dominant use case for
error_get_pretty in the code base.

IMO the caller can always do this:

    err ? error_get_pretty(err) : NULL

in place of your proposed

    error_get_pretty(err)

So maybe leave this and change dump_process like above? Or if you insist, make
this hunk a separate patch please.

Fam

>  }
>  
>  void error_report_err(Error *err)
> -- 
> 2.4.3
>