If there's still time, suggest to tweak the subject to
hmp/migration: Fix "migrate" command's documentation
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.
>> > 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>
Thanks!
