From: Markus Armbruster <armbru@redhat.com>
To: Peter Xu <peterx@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, 03 May 2024 16:08:45 +0200 [thread overview]
Message-ID: <87h6ffvvgy.fsf@pond.sub.org> (raw)
In-Reply-To: <ZjTsouwxaJ8S5Icf@x1n> (Peter Xu's message of "Fri, 3 May 2024 09:54:42 -0400")
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!
next prev parent reply other threads:[~2024-05-03 14:09 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 [this message]
2024-05-03 14:36 ` Peter Xu
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=87h6ffvvgy.fsf@pond.sub.org \
--to=armbru@redhat.com \
--cc=dave@treblig.org \
--cc=farosas@suse.de \
--cc=peterx@redhat.com \
--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).