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

[Wangjing(Hogan)]

> 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(-)
> >
> > @@ -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.
> 
Accept the improvements.

> > --- 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, ...
> 
Thanks for your review comments, I will write the detailed comment for @job-id 
in patch set for the next version.

> >  # @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?
> 
I had tested in my environment,  "detach": false haven't got any sense when 
"job-id" is present,cann't cancel and query the job.
I will delete the code condition branch for non-detach dump job.

> Preexisting: @detach's default is undocumented.

Hogan Wang