Re: [PATCH] hmp/migration: Fix documents for "migrate" command

[Markus Armbruster <armbru@redhat.com>]

If there's still time, suggest to tweak the subject to

    hmp/migration: Fix "migrate" command's documentation

Peter Xu <peterx@redhat.com> writes:

> On Fri, May 03, 2024 at 08:58:09AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@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 <dave@treblig.org>
>> > Cc: Fabiano Rosas <farosas@suse.de>
>> > Cc: Markus Armbruster <armbru@redhat.com>
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >
>> > Based-on: <20240430142737.29066-1-farosas@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 <armbru@redhat.com>

Thanks!