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

[Peter Xu <peterx@redhat.com>]

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..

> 
> > 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

-- 
Peter Xu