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

[Peter Xu]

On Fri, Nov 27, 2015 at 11:22:48AM +0100, Paolo Bonzini wrote:
> 
> 
> 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.

I think it might be a little big confusing if I use MigrationStatus
directly. I can define a enum. 

> 
> 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.

Ok, Then I will drop percentage in QMP query.

> 
> 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.

Ok. Will reorder the patches and make sure there are no fake values
any more.

Thanks!
Peter

> 
> 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);
> >