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

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

[Peter Xu]

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.

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
 
 
 SRST
-``migrate [-d]`` *uri*
-  Migrate to *uri* (using -d to not wait for completion).
+``migrate [-d] [-r]`` *uri*
+  Migrate the current VM to *uri*.
+
+  ``-d``
+    Run this command asynchronously, so that the command doesn't wait for completion.
+  ``-r``
+    Resume a paused postcopy migration.
 ERST
 
     {
-- 
2.44.0

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

[Fabiano Rosas]

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

Reviewed-by: Fabiano Rosas <farosas@suse.de>

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

[Markus Armbruster]

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?

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

> +
> +  ``-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?

> +  ``-r``
> +    Resume a paused postcopy migration.

.help doesn't have "postcopy".  Should it?

>  ERST
>  
>      {

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

[Peter Xu]

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

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

[Markus Armbruster]

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!

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

[Peter Xu]

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

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

Thank you!

-- 
Peter Xu