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 <pet...@redhat.com>
> > ---
> > 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);
> >
