On Fri, May 03, 2024 at 08:58:09AM +0200, Markus Armbruster wrote:
> Peter Xu <pet...@redhat.com> writes:
>
> > Peter missed the Sphinx HMP document for the "resume/-r" flag in commit
> > 7a4da28b26 ("qmp: hmp: add migrate "resume" option"). Add it. Avoid
> > adding a Fixes to make life easier for the stable maintainer.
>
> I'm curious: how does not adding Fixes: make life easier?
Because if I attach Fixes then IIUC Michael will read it through and judge
whether it should apply to stable, where I want to skip that for him
because I think this doesn't apply to stable. Reasons:
- This is a document update, IIUC we normally only keep the latest
document uptodate, not all the stable versions (especiailly for HMP,
which isn't a stable ABI)? I assume it applies the same when a qtest
case got a slight fixup.
- This patch is even more special as it will need explicit backport due
to the removal of block migration, and I really don't think any of us
should spend time on that..
>
> > When at it, slightly cleanup the lines, move "detach/-d" to a separate
> > section rather than appending it at the end of the command description.
> >
> > Cc: Dr. David Alan Gilbert <d...@treblig.org>
> > Cc: Fabiano Rosas <faro...@suse.de>
> > Cc: Markus Armbruster <arm...@redhat.com>
> > Signed-off-by: Peter Xu <pet...@redhat.com>
> > ---
> >
> > Based-on: <20240430142737.29066-1-faro...@suse.de>
> > ("[PATCH v3 0/6] migration removals & deprecations")
> > ---
> > hmp-commands.hx | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index ebca2cdced..484a8a1c3a 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -918,8 +918,13 @@ ERST
> {
> .name = "migrate",
> .args_type = "detach:-d,blk:-b,inc:-i,resume:-r,uri:s",
> .params = "[-d] [-b] [-i] [-r] uri",
> .help = "migrate to URI (using -d to not wait for
> completion)"
> "\n\t\t\t -b for migration without shared storage
> with"
> " full copy of disk\n\t\t\t -i for migration without
> "
> "shared storage with incremental copy of disk "
> "(base image shared between src and destination)"
> "\n\t\t\t -r to resume a paused migration",
> .cmd = hmp_migrate,
> },
> >
> >
> > SRST
> > -``migrate [-d]`` *uri*
> > - Migrate to *uri* (using -d to not wait for completion).
> > +``migrate [-d] [-r]`` *uri*
> > + Migrate the current VM to *uri*.
>
> Could there be any other VM than the current one? Scratch "current"?
I didn't have "current" until I generated the doc and read, then I see
right below "migrate_cancel" has it:
SRST
``migrate_cancel``
Cancel the current VM migration.
ERST
But maybe it means "current migration", not "current VM".. So yeah I can
drop it.
>
> > +
> > + ``-d``
> > + Run this command asynchronously, so that the command doesn't wait for
> > completion.
>
> What is run asynchronously, and what isn't waiting? These are two
> different entities, aren't they? Calling them "this command" and "the
> command" is confusing :)
>
> Perhaps
>
> Start the migration process, but do not wait for its completion.
>
> Maybe add a hint on how to wait or poll for completion?
Yes this reads better; I will add the hint too.
>
> > + ``-r``
> > + Resume a paused postcopy migration.
>
> .help doesn't have "postcopy". Should it?
It should.
This is the fixup I'll squash when sending v2, let me know if there's other
early comments, thanks.
===8<===
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 484a8a1c3a..06746f0afc 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -912,17 +912,18 @@ ERST
.args_type = "detach:-d,resume:-r,uri:s",
.params = "[-d] [-r] uri",
.help = "migrate to URI (using -d to not wait for completion)"
- "\n\t\t\t -r to resume a paused migration",
+ "\n\t\t\t -r to resume a paused postcopy migration",
.cmd = hmp_migrate,
},
SRST
``migrate [-d] [-r]`` *uri*
- Migrate the current VM to *uri*.
+ Migrate the VM to *uri*.
``-d``
- Run this command asynchronously, so that the command doesn't wait for
completion.
+ Start the migration process, but do not wait for its completion. To
+ query an ongoing migration process, use "info migrate".
``-r``
Resume a paused postcopy migration.
ERST
--
Peter Xu
