Re: [PATCH 3/3] dump: support cancel dump process

[Kevin Wolf]

Am 28.07.2022 um 14:37 hat Marc-André Lureau geschrieben:
> Hi
> 
> On Wed, Jul 27, 2022 at 6:02 PM Hogan Wang via <qemu-devel@nongnu.org>
> wrote:
> 
> > Break saving pages or dump iterate when dump job in cancel state,
> > make sure dump process exits as soon as possible.
> >
> > Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
> >
> 
> Overall, the series looks good to me. Please send it with a top cover
> letter, so it can be processed by patchew too.
> 
> I am not familiar with the job infrastructure, it would be nice if Kevin
> could check the usage or give some advice.

There is one big point I see with this series, which is that it could be
considered an incompatible change to 'dump-guest-memory'. Clients are
expected to manage the job now. Though in practice with the default
settings, maybe it actually just results in clients receiving additional
QMP events. (Technically, it is still incompatible because the command
will now fail if you have another job called 'memory-guest-dump' - no
good reason to have that, but it's a scenario that worked and breaks
after this series.)

Markus, do you have an opinion on whether job creation must be
explicitly enabled with a new option or if we can just switch existing
callers?

The implementation of a very basic job looks mostly okay to me, though
of course it doesn't support a few things like pausing the job and
proper progress monitoring. But these things are optional, so it's not a
blocker.

The one thing I would strongly recommend to include is an auto-dismiss
option like every other job has. It is required so that management tools
can query the final job status before it goes away. Most of the
information is in QMP events, too, but events can be lost. auto-finalize
isn't required for this job because it doesn't actually do anything in
the finalize phase.

I'm not sure how safe the whole thing is when it runs in the background
and you can send additional QMP commands while it's running, but that is
preexisting with detach=true.

Kevin