Re: [PATCH v4 2/3] job: introduce dump guest memory job

[Markus Armbruster]

Hogan Wang <hogan.wang@huawei.com> writes:

> There's no way to cancel the current executing dump process, lead to the
> virtual machine manager daemon((e.g. libvirtd) cannot restore the dump
> job after daemon restart.
>
> Introduce dump guest memory job type, and add an optional 'job-id'
> argument for dump-guest-memory QMP to make use of jobs framework.
>
> Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
> ---
>  dump/dump-hmp-cmds.c | 12 ++++++++++--
>  dump/dump.c          |  1 +
>  qapi/dump.json       |  6 +++++-
>  qapi/job.json        |  5 ++++-
>  4 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/dump/dump-hmp-cmds.c b/dump/dump-hmp-cmds.c
> index e5053b04cd..ba28a5e631 100644
> --- a/dump/dump-hmp-cmds.c
> +++ b/dump/dump-hmp-cmds.c
> @@ -24,9 +24,11 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict 
> *qdict)
>      bool has_begin = qdict_haskey(qdict, "begin");
>      bool has_length = qdict_haskey(qdict, "length");
>      bool has_detach = qdict_haskey(qdict, "detach");
> +    bool has_job_id = qdict_haskey(qdict, "job-id");
>      int64_t begin = 0;
>      int64_t length = 0;
>      bool detach = false;
> +    const char *job_id = NULL;
>      enum DumpGuestMemoryFormat dump_format = DUMP_GUEST_MEMORY_FORMAT_ELF;
>      char *prot;
>  
> @@ -62,10 +64,16 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict 
> *qdict)
>          detach = qdict_get_bool(qdict, "detach");
>      }
>  
> +    if (has_job_id) {
> +        job_id = qdict_get_str(qdict, "job-id");
> +    }
> +

Simpler:

       const char *job_id = qdict_get_try_str(qdict, "job-id");

>      prot = g_strconcat("file:", file, NULL);
>  
> -    qmp_dump_guest_memory(paging, prot, true, detach, has_begin, begin,
> -                          has_length, length, true, dump_format, &err);
> +    qmp_dump_guest_memory(paging, prot, has_job_id, job_id,

This becomes

       qmp_dump_guest_memory(paging, prot, !!job_id, job_id,

then.

> +                          true, detach, has_begin, begin,
> +                          has_length, length, true, dump_format,
> +                          &err);
>      hmp_handle_error(mon, err);
>      g_free(prot);
>  }
> diff --git a/dump/dump.c b/dump/dump.c
> index a57c580b12..cec9be30b4 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1895,6 +1895,7 @@ DumpQueryResult *qmp_query_dump(Error **errp)
>  }
>  
>  void qmp_dump_guest_memory(bool paging, const char *file,
> +                           bool has_job_id, const char *job_id,
>                             bool has_detach, bool detach,
>                             bool has_begin, int64_t begin, bool has_length,
>                             int64_t length, bool has_format,
> diff --git a/qapi/dump.json b/qapi/dump.json
> index 90859c5483..d162a9f028 100644
> --- a/qapi/dump.json
> +++ b/qapi/dump.json
> @@ -59,6 +59,9 @@
>  #            2. fd: the protocol starts with "fd:", and the following string
>  #               is the fd's name.
>  #
> +# @job-id: identifier for the newly-created memory dump job. To be compatible
> +#          with legacy dump process, @job-id should omitted. (Since 7.2)
> +#

I think we need to describe things in more detail.

What are the behavioral differences between dumping with and without 
@job-id?

Why would you want to pass @job-id?  I figure it's to gain the ability
to monitor and control dump task with query-job, job-cancel, ...

>  # @detach: if true, QMP will return immediately rather than
>  #          waiting for the dump to finish. The user can track progress
>  #          using "query-dump". (since 2.6).

Hmm, does "detach": false make any sense when "job-id" is present?

Preexisting: @detach's default is undocumented.

> @@ -88,7 +91,8 @@
>  #
>  ##
>  { 'command': 'dump-guest-memory',
> -  'data': { 'paging': 'bool', 'protocol': 'str', '*detach': 'bool',
> +  'data': { 'paging': 'bool', 'protocol': 'str',
> +            '*job-id': 'str', '*detach': 'bool',
>              '*begin': 'int', '*length': 'int',
>              '*format': 'DumpGuestMemoryFormat'} }
>  
> diff --git a/qapi/job.json b/qapi/job.json
> index d5f84e9615..e14d2290a5 100644
> --- a/qapi/job.json
> +++ b/qapi/job.json
> @@ -28,11 +28,14 @@
>  #
>  # @snapshot-delete: snapshot delete job type, see "snapshot-delete" (since 
> 6.0)
>  #
> +# @dump-guest-memory: dump guest memory job type, see "dump-guest-memory" 
> (since 7.2)
> +#
>  # Since: 1.7
>  ##
>  { 'enum': 'JobType',
>    'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend',
> -           'snapshot-load', 'snapshot-save', 'snapshot-delete'] }
> +           'snapshot-load', 'snapshot-save', 'snapshot-delete',
> +           'dump-guest-memory'] }
>  
>  ##
>  # @JobStatus: