From: Peter Xu <peterx@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org,
"Dr . David Alan Gilbert" <dave@treblig.org>,
Fabiano Rosas <farosas@suse.de>
Subject: Re: [PATCH] hmp/migration: Fix documents for "migrate" command
Date: Fri, 3 May 2024 10:36:06 -0400 [thread overview]
Message-ID: <ZjT2Vp5PoUUmHc8A@x1n> (raw)
In-Reply-To: <87h6ffvvgy.fsf@pond.sub.org>
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
prev parent reply other threads:[~2024-05-03 14:36 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-02 15:47 [PATCH] hmp/migration: Fix documents for "migrate" command Peter Xu
2024-05-02 17:34 ` Fabiano Rosas
2024-05-03 6:58 ` Markus Armbruster
2024-05-03 13:54 ` Peter Xu
2024-05-03 14:08 ` Markus Armbruster
2024-05-03 14:36 ` Peter Xu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjT2Vp5PoUUmHc8A@x1n \
--to=peterx@redhat.com \
--cc=armbru@redhat.com \
--cc=dave@treblig.org \
--cc=farosas@suse.de \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).