On Fri, May 03, 2024 at 04:08:45PM +0200, Markus Armbruster wrote:
> If there's still time, suggest to tweak the subject to
>
> hmp/migration: Fix "migrate" command's documentation
Yes there is. :)
>
> Peter Xu <pet...@redhat.com> writes:
>
> > 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..
>
> Right. But Fixes: is also for downstreams, who may want to make their
> own decisions.
>
> I think I'd always add Fixes:. When I think there's a need to steer
> stable away from it, I'd say so in the commit message. I doubt needed
> here, as the subject states it's just a doc fix for HMP.
OK, I'll attach a Fixes.
>
> >> > 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
>
> Reviewed-by: Markus Armbruster <arm...@redhat.com>
Thank you!
--
Peter Xu
