From: Markus Armbruster <armbru@redhat.com>
To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: qemu-devel@nongnu.org, quintela@redhat.com
Subject: Re: [PATCH 2/6] migration: Fix migrate-set-parameters argument validation
Date: Fri, 13 Nov 2020 14:24:54 +0100 [thread overview]
Message-ID: <87tuttfkyh.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20201113114926.GF3251@work-vm> (David Alan Gilbert's message of "Fri, 13 Nov 2020 11:49:26 +0000")
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> Commit 741d4086c8 "migration: Use proper types in json" (v2.12.0)
>> switched MigrationParameters to narrower integer types, and removed
>> the simplified qmp_migrate_set_parameters()'s argument checking
>> accordingly.
>>
>> Good idea, except qmp_migrate_set_parameters() takes
>> MigrateSetParameters, not MigrationParameters. Its job is updating
>> migrate_get_current()->parameters (which *is* of type
>> MigrationParameters) according to its argument. The integers now get
>> truncated silently. Reproducer:
>>
>> ---> {'execute': 'query-migrate-parameters'}
>> <--- {"return": {[...] "compress-threads": 8, [...]}}
>> ---> {"execute": "migrate-set-parameters", "arguments": {"compress-threads": 257}}
>> <--- {"return": {}}
>> ---> {'execute': 'query-migrate-parameters'}
>> <--- {"return": {[...] "compress-threads": 1, [...]}}
>>
>> Fix by resynchronizing MigrateSetParameters with MigrationParameters.
>
> Having those two separate types is a pain!
It is!
MigrateSetParameters is the argument of migrate-set-parameters,
MigrationParameters is the result of query-migrate-parameters and part
of the internal migration state.
Differences:
(1) Optional members
For migrate-set-parameters, we need *all* members to be optional.
For migration state and query-migrate-parameters, we want only some
members to be optional (currently only @tls-authz, I think).
(2) Special values
migrate-set-parameters has a "reset to default, whatever that may
be" feature for some members (currently only @tls-creds,
@tls-hostname, @tls-authz, I think). Doing that cleanly requires an
additonal value.
The first attempt to fuse the two types (commit de63ab6124 "migrate:
Share common MigrationParameters struct", 2016-10-13) took care of (1).
Introspection of query-migrate-parameters became mildly misleading (it
claims members are optional that aren't), and C code dealing with
migration state had to take care to set the has_FOO = true. Tolerable.
I had to revert it to address (2) cleanly and in time: commit 01fa559826
"migration: Use JSON null instead of "" to reset parameter to default",
2017-07-24.
A second try needs to take care of (2) as well. Messes up
query-migrate-parameters some more: introspection claims a few members
can be null that can't.
Is the "reset" feature is worth all that trouble? Is overloading
migrate-set-parameters a good idea?
>> Fixes: 741d4086c856320807a2575389d7c0505578270b
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Thanks!
next prev parent reply other threads:[~2020-11-13 13:26 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-13 6:52 [PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters Markus Armbruster
2020-11-13 6:52 ` [PATCH 1/6] migration: Fix and clean up around @tls-authz Markus Armbruster
2020-11-13 11:56 ` Dr. David Alan Gilbert
2020-12-10 17:35 ` Dr. David Alan Gilbert
2020-12-10 18:10 ` Daniel P. Berrangé
2020-12-14 10:14 ` Markus Armbruster
2020-12-16 10:55 ` Daniel P. Berrangé
2020-12-17 13:07 ` Markus Armbruster
2020-12-17 14:04 ` Daniel P. Berrangé
2021-01-27 16:01 ` Markus Armbruster
2020-11-13 6:52 ` [PATCH 2/6] migration: Fix migrate-set-parameters argument validation Markus Armbruster
2020-11-13 11:49 ` Dr. David Alan Gilbert
2020-11-13 13:24 ` Markus Armbruster [this message]
2020-11-13 6:52 ` [PATCH 3/6] migration: Clean up signed vs. unsigned XBZRLE cache-size Markus Armbruster
2020-11-13 10:40 ` Dr. David Alan Gilbert
2020-11-13 6:52 ` [PATCH 4/6] migration: Check xbzrle-cache-size more carefully Markus Armbruster
2020-11-13 10:59 ` Dr. David Alan Gilbert
2020-11-13 13:35 ` Markus Armbruster
2020-11-13 16:39 ` Dr. David Alan Gilbert
2020-11-16 7:00 ` Markus Armbruster
2020-11-13 6:52 ` [PATCH 5/6] migration: Fix cache_init()'s "Failed to allocate" error messages Markus Armbruster
2020-11-13 11:01 ` Dr. David Alan Gilbert
2020-11-13 6:52 ` [PATCH 6/6] migration: Fix a few absurdly defective " Markus Armbruster
2020-11-13 11:27 ` Dr. David Alan Gilbert
2020-11-13 11:56 ` [PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters Dr. David Alan Gilbert
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=87tuttfkyh.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=dgilbert@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=quintela@redhat.com \
/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 NNTP newsgroup(s).