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

[Markus Armbruster <armbru@redhat.com>]

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 |  7 ++++---
>  dump/dump.c          |  1 +
>  qapi/dump.json       | 11 +++++++++--
>  qapi/job.json        |  5 ++++-
>  4 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/dump/dump-hmp-cmds.c b/dump/dump-hmp-cmds.c
> index e5053b04cd..a77f31fd15 100644
> --- a/dump/dump-hmp-cmds.c
> +++ b/dump/dump-hmp-cmds.c
> @@ -21,6 +21,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>      bool lzo = qdict_get_try_bool(qdict, "lzo", false);
>      bool snappy = qdict_get_try_bool(qdict, "snappy", false);
>      const char *file = qdict_get_str(qdict, "filename");
> +    const char *job_id = qdict_get_str(qdict, "job-id");
>      bool has_begin = qdict_haskey(qdict, "begin");
>      bool has_length = qdict_haskey(qdict, "length");
>      bool has_detach = qdict_haskey(qdict, "detach");
> @@ -63,9 +64,9 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
>      }
>  
>      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, !!job_id, job_id, 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..ca3bd720c6 100644
> --- a/qapi/dump.json
> +++ b/qapi/dump.json
> @@ -59,9 +59,15 @@
>  #            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. If you want to
> +#          compatible with legacy dumping process, @job-id should omitted.
> +#          If @job-id specified, gain the ability to monitor and control dump
> +#          task with query-job, job-cancel, etc.(Since 7.2).

Space before "(Since 7.2)", please.



> +#
>  # @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).
> +#          using "query-dump". (since 2.6). If @job-id specified, @detach
> +#          argument value will be ignored (Since 7.2).

I think @detach should be rejected then.

>  #
>  # @begin: if specified, the starting physical address.
>  #
> @@ -88,7 +94,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:

I'd like to hear Kevin's opinion on the alternatives I sketched
yesterday in reply to his question regarding v1.