Re: [Qemu-devel] [PATCH REPOST 2/2] dump-guest-memory: add basic "detach" support.

[Andrew Jones <drjones@redhat.com>]

On Mon, Nov 23, 2015 at 06:07:42PM +0800, Peter Xu wrote:
> This will allow the user specify "-d" (just like command
> "migrate") when using "dump-guest-memory" command. When
> specified, one background thread is created to do the dump work.
> One flag is added to show whether there is a background dump
> work in progress.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  dump.c                | 59 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  include/sysemu/dump.h |  4 ++++
>  qmp.c                 |  9 ++++++++
>  vl.c                  |  3 +++
>  4 files changed, 70 insertions(+), 5 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 3ec3423..c2e14cd 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1442,6 +1442,9 @@ static void dump_init(DumpState *s, int fd, bool has_format,
>      Error *err = NULL;
>      int ret;
>  
> +    s->has_format = has_format;
> +    s->format = format;
> +
>      /* kdump-compressed is conflict with paging and filter */
>      if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>          assert(!paging && !has_filter);
> @@ -1595,6 +1598,45 @@ cleanup:
>      dump_cleanup(s);
>  }
>  
> +/* whether there is a dump in progress */
> +extern bool dump_in_progress_p;
> +
> +static bool dump_ownership_set(bool value)
> +{
> +    return atomic_xchg(&dump_in_progress_p, value);
> +}
> +
> +/* return true when owner taken, false otherwise */
returns true when ownership is taken

> +static bool dump_ownership_take(void)
> +{
> +    bool ret = dump_ownership_set(1);
> +    return ret == 0;

return dump_ownership_set(1) == 0;

> +}
> +
> +/* we should only call this after all dump work finished */
                                                  ^has
> +static void dump_ownership_release(void)
> +{
> +    dump_ownership_set(0);
> +}
> +
> +static void dump_process(DumpState *s, Error **errp)
> +{
> +    if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
> +        create_kdump_vmcore(s, errp);
> +    } else {
> +        create_vmcore(s, errp);
> +    }
> +    dump_ownership_release();
> +}
> +
> +static void *dump_thread(void *data)
> +{
> +    DumpState *s = (DumpState *)data;
> +    dump_process(s, NULL);

How do errors work when errp is NULL? It looks like we won't get any.
Could we duplicate the errp we get from qmp and add it to the DumpState?
Or just use a local_err? (I know not of what I speak, this is Markus
territory.)

> +    g_free(s);
> +    return NULL;
> +}
> +
>  void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
>                             int64_t begin, bool has_length,
>                             int64_t length, bool has_format,
> @@ -1662,6 +1704,12 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
>          return;
>      }
>  
> +    /* we could only allow one dump at a time. */
s/could/can/

> +    if (!dump_ownership_take()) {
> +        error_setg(errp, "another dump is in progress.");
> +        return;
> +    }
> +
>      s = g_malloc0(sizeof(DumpState));
>  
>      dump_init(s, fd, has_format, format, paging, has_begin,
> @@ -1672,13 +1720,14 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
>          return;
>      }
>  
> -    if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
> -        create_kdump_vmcore(s, errp);
> +    if (!detach) {
> +        dump_process(s, errp);
> +        g_free(s);
>      } else {
> -        create_vmcore(s, errp);
> +        qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
> +                           s, QEMU_THREAD_DETACHED);
> +        /* DumpState is freed in dump thread */
>      }
> -
> -    g_free(s);
>  }
>  
>  DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 7e4ec5c..2aeffc8 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -183,6 +183,10 @@ typedef struct DumpState {
>      off_t offset_page;          /* offset of page part in vmcore */
>      size_t num_dumpable;        /* number of page that can be dumped */
>      uint32_t flag_compress;     /* indicate the compression format */
> +
> +    QemuThread dump_thread;     /* only used when do async dump */
                                                     ^doing an
> +    bool has_format;            /* whether format is provided */
> +    DumpGuestMemoryFormat format; /* valid only if has_format == true */
>  } DumpState;
>  
>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
> diff --git a/qmp.c b/qmp.c
> index 0a1fa19..ea57b57 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -168,12 +168,21 @@ SpiceInfo *qmp_query_spice(Error **errp)
>  };
>  #endif
>  
> +extern bool dump_in_progress_p;
> +
>  void qmp_cont(Error **errp)
>  {
>      Error *local_err = NULL;
>      BlockBackend *blk;
>      BlockDriverState *bs;
>  
> +    /* if there is a dump in background, we should wait until the dump
          ^If                  ^the
> +     * finished */
          finishes
> +    if (dump_in_progress_p) {
> +        error_setg(errp, "there is a dump in process, please wait.");

If there's a period in the error message then I think there should be
capitalized. If it's supposed to be a phrase, then either no period or
three of them '...'

> +        return;
> +    }
> +
>      if (runstate_needs_reset()) {
>          error_setg(errp, "Resetting the Virtual Machine is required");
>          return;
> diff --git a/vl.c b/vl.c
> index 525929b..ef7a936 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -204,6 +204,9 @@ bool xen_allowed;
>  uint32_t xen_domid;
>  enum xen_mode xen_mode = XEN_EMULATE;
>  
> +/* whether dump is in progress */
             ^ a
> +bool dump_in_progress_p = false;
> +
>  static int has_defaults = 1;
>  static int default_serial = 1;
>  static int default_parallel = 1;
> -- 
> 2.4.3
> 
>

Thanks,
drew