Re: [Qemu-devel] [PATCH v2 6/8] dump-query: implement "status" of "dump-query" command.

[Paolo Bonzini]

On 27/11/2015 03:48, Peter Xu wrote:
> This patch implements the status changes inside dump process. When
> query dump status, three possible results could be:
> 
> 1. never started dump:
> { "status": "NOT_STARTED", "percentage": "N/A" }
> 
> 2. one dump is running in background:
> { "status": "IN_PROGRESS", "percentage": "{0..100}%" }
> 
> 3. no dump is running, and the last dump has finished:
> { "status": "SUCCEEDED|FAILED", "percentage": "N/A" }
> 
> It's mostly done. Except that we might need more accurate
> "percentage" results (which is now 50% all the time!).

As mentioned early, you can use an enum.  QEMU usually prints enums in
lowercase and separates words with "-".  Similar to MigrationStatus you
can use none, active, completed, failed.  In fact you might even reuse
MigrationStatus directly, it's simpler.

The percentage is not necessary as part of the QMP return value.  You
can compute it in hmp_info_dump however and print it to HMP only.

I think you can structure the patches like this:

add basic "detach" support
add qmp event DUMP_COMPLETED
add total_size and written_size to DumpState
add "query-dump" QMP command
add "info dump" HMP command

where the fourth patch already sets all fields correctly.

Thanks,

Paolo

> Signed-off-by: Peter Xu <address@hidden>
> ---
>  dump.c                | 57 
> +++++++++++++++++++++++++++++++++++++++++++--------
>  hmp.c                 |  7 +++++--
>  include/qemu-common.h |  4 ++++
>  include/sysemu/dump.h | 13 ++++++++++--
>  4 files changed, 68 insertions(+), 13 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 446a991..25298b7 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -1594,11 +1594,44 @@ static GlobalDumpState *dump_state_get_global(void)
>      if (unlikely(!init)) {
>          qemu_mutex_init(&global_dump_state.gds_mutex);
>          global_dump_state.gds_cur = NULL;
> +        global_dump_state.gds_result = DUMP_RES_NOT_STARTED;
>          init = true;
>      }
>      return &global_dump_state;
>  }
>  
> +static const char *const dump_result_table[] = {
> +    [DUMP_RES_NOT_STARTED] = "NOT_STARTED",
> +    [DUMP_RES_IN_PROGRESS] = "IN_PROGRESS",
> +    [DUMP_RES_SUCCEEDED]   = "SUCCEEDED",
> +    [DUMP_RES_FAILED]      = "FAILED",
> +};
> +
> +/* Returns DumpStatus. Caller should be responsible to free the
> + * resources using qapi_free_DumpStatus(). */
> +DumpStatus *dump_status_query(void)
> +{
> +    DumpStatus *status = g_malloc0(sizeof(*status));
> +    int percentage = 50;
> +    char buf[64] = {0};
> +
> +    GlobalDumpState *global = dump_state_get_global();
> +    qemu_mutex_lock(&global->gds_mutex);
> +
> +    /* TBD: get correct percentage */
> +    status->status = g_strdup(dump_result_table[global->gds_result]);
> +    if (global->gds_result == DUMP_RES_IN_PROGRESS) {
> +        snprintf(buf, sizeof(buf) - 1, "%d%%", percentage);
> +        status->percentage = g_strdup(buf);
> +    } else {
> +        status->percentage = g_strdup("N/A");
> +    }
> +
> +    qemu_mutex_unlock(&global->gds_mutex);
> +
> +    return status;
> +}
> +
>  /* Returns non-zero if there is existing dump in progress, otherwise
>   * zero is returned. */
>  bool dump_in_progress(void)
> @@ -1620,27 +1653,37 @@ static DumpState *dump_state_create(GlobalDumpState 
> *global)
>      cur = global->gds_cur;
>      if (cur) {
>          /* one dump in progress */
> +        assert(global->gds_result == DUMP_RES_IN_PROGRESS);
>          cur = NULL;
>      } else {
>          /* we are the first! */
> +        assert(global->gds_result != DUMP_RES_IN_PROGRESS);
>          cur = g_malloc0(sizeof(*cur));
>          global->gds_cur = cur;
>      }
> +    global->gds_result = DUMP_RES_IN_PROGRESS;
>  
>      qemu_mutex_unlock(&global->gds_mutex);
>      return cur;
>  }
>  
> -/* release current DumpState structure */
> -static void dump_state_release(GlobalDumpState *global)
> +/* release current DumpState structure. "done" is hint to show
> + * whether the dump is succeeded or not. */
> +static void dump_state_release(GlobalDumpState *global, bool done)
>  {
>      DumpState *cur = NULL;
>      qemu_mutex_lock(&global->gds_mutex);
>  
> +    assert(global->gds_result == DUMP_RES_IN_PROGRESS);
>      cur = global->gds_cur;
>      /* we should never call release before having one */
>      assert(cur);
>      global->gds_cur = NULL;
> +    if (done) {
> +        global->gds_result = DUMP_RES_SUCCEEDED;
> +    } else {
> +        global->gds_result = DUMP_RES_FAILED;
> +    }
>  
>      qemu_mutex_unlock(&global->gds_mutex);
>      dump_cleanup(cur);
> @@ -1664,7 +1707,7 @@ static void *dump_thread(void *data)
>      const char *msg = "Dump completed successfully";
>  
>      dump_process(global->gds_cur, &local_err);
> -    dump_state_release(global);
> +    dump_state_release(global, (local_err == NULL));
>  
>      /* if detach is used, notify user that dump has finished */
>      if (local_err) {
> @@ -1769,7 +1812,7 @@ void qmp_dump_guest_memory(bool paging, const char 
> *file,
>      if (!detach_p) {
>          /* sync dump */
>          dump_process(s, errp);
> -        dump_state_release(global);
> +        dump_state_release(global, (*errp == NULL));
>      } else {
>          qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
>                             global, QEMU_THREAD_DETACHED);
> @@ -1779,11 +1822,7 @@ void qmp_dump_guest_memory(bool paging, const char 
> *file,
>  
>  DumpStatus *qmp_dump_query(Error **errp)
>  {
> -    DumpStatus *status = g_malloc0(sizeof(*status));
> -    /* TBD */
> -    status->status = g_strdup("WORKING");
> -    status->percentage = g_strdup("50%");
> -    return status;
> +    return dump_status_query();
>  }
>  
>  DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error 
> **errp)
> diff --git a/hmp.c b/hmp.c
> index 6d9c127..049ac4b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1578,8 +1578,11 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
>  
>  void hmp_dump_query(Monitor *mon, const QDict *qdict)
>  {
> -    /* TBD */
> -    monitor_printf(mon, "QUERY DUMP STATUS\n");
> +    DumpStatus *status = dump_status_query();
> +    assert(status);
> +    monitor_printf(mon, "STATUS: %s\n", status->status);
> +    monitor_printf(mon, "PERCENTAGE: %s\n", status->percentage);
> +    qapi_free_DumpStatus(status);
>  }
>  
>  void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 7b74961..fbfa852 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -505,4 +505,8 @@ void page_size_init(void);
>   * returned. */
>  bool dump_in_progress(void);
>  
> +/* Returns DumpStatus. Caller should be responsible to free the
> + * resources using qapi_free_DumpStatus(). */
> +DumpStatus *dump_status_query(void);
> +
>  #endif
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index 13c913e..0f0a463 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -189,9 +189,18 @@ typedef struct DumpState {
>      DumpGuestMemoryFormat format; /* valid only if has_format == true */
>  } DumpState;
>  
> +typedef enum DumpResult {
> +    DUMP_RES_NOT_STARTED = 0,
> +    DUMP_RES_IN_PROGRESS,
> +    DUMP_RES_SUCCEEDED,
> +    DUMP_RES_FAILED,
> +    DUMP_RES_MAX,
> +} DumpResult;
> +
>  typedef struct GlobalDumpState {
> -    QemuMutex gds_mutex;  /* protector for GlobalDumpState itself */
> -    DumpState *gds_cur;   /* current DumpState (might be NULL) */
> +    QemuMutex gds_mutex;        /* protector for GlobalDumpState itself */
> +    DumpState *gds_cur;         /* current DumpState (might be NULL) */
> +    DumpResult gds_result;      /* current dump result */
>  } GlobalDumpState;
>  
>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
>