| Message ID | 20210419162732.766055-1-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-6.0] migration: Drop redundant query-migrate result @blocked | expand |
On Mon, 19 Apr 2021 at 17:27, Markus Armbruster <armbru@redhat.com> wrote: > > Result @blocked is true when and only when result @blocked-reasons is > present. It's always non-empty when present. @blocked is redundant; > drop. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> "for-6.0" needs to be accompanied by a justification of why it's important to go in the release at this point... thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On Mon, 19 Apr 2021 at 17:27, Markus Armbruster <armbru@redhat.com> wrote: > > > > Result @blocked is true when and only when result @blocked-reasons is > > present. It's always non-empty when present. @blocked is redundant; > > drop. > > > > Signed-off-by: Markus Armbruster <armbru@redhat.com> So I'm OK with it in principal and I think the code is OK, so Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > "for-6.0" needs to be accompanied by a justification of why it's > important to go in the release at this point... I guess the argument is that when we hit 6.0 it becomes API and removing the 'blocked' becomes a matter of deprecation which is a pain. Hmm; I agree it's the right change, but I'm not sure I can justify it this late in the release. Dave > thanks > -- PMM >
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Peter Maydell (peter.maydell@linaro.org) wrote: >> On Mon, 19 Apr 2021 at 17:27, Markus Armbruster <armbru@redhat.com> wrote: >> > >> > Result @blocked is true when and only when result @blocked-reasons is >> > present. It's always non-empty when present. @blocked is redundant; >> > drop. >> > >> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > > So I'm OK with it in principal and I think the code is OK, so > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >> "for-6.0" needs to be accompanied by a justification of why it's >> important to go in the release at this point... You're right. My bad. > I guess the argument is that when we hit 6.0 it becomes API and removing > the 'blocked' becomes a matter of deprecation which is a pain. Correct. > Hmm; I agree it's the right change, but I'm not sure I can justify it > this late in the release. If we decide taking it out is too late, we should at least deprecate it in 6.0. I'll post the patch, so you guys can pick the one you like better.
diff --git a/qapi/migration.json b/qapi/migration.json index 9bf0bc4d25..7a5bdf9a0d 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -224,9 +224,9 @@ # only returned if VFIO device is present, migration is supported by all # VFIO devices and status is 'active' or 'completed' (since 5.2) # -# @blocked: True if outgoing migration is blocked (since 6.0) -# -# @blocked-reasons: A list of reasons an outgoing migration is blocked (since 6.0) +# @blocked-reasons: A list of reasons an outgoing migration is blocked. +# Present and non-empty when migration is blocked. +# (since 6.0) # # Since: 0.14 ## @@ -241,7 +241,6 @@ '*setup-time': 'int', '*cpu-throttle-percentage': 'int', '*error-desc': 'str', - 'blocked': 'bool', '*blocked-reasons': ['str'], '*postcopy-blocktime' : 'uint32', '*postcopy-vcpu-blocktime': ['uint32'], diff --git a/migration/migration.c b/migration/migration.c index 8ca034136b..fdadee290e 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1073,27 +1073,24 @@ static void populate_vfio_info(MigrationInfo *info) static void fill_source_migration_info(MigrationInfo *info) { MigrationState *s = migrate_get_current(); + GSList *cur_blocker = migration_blockers; - info->blocked = migration_is_blocked(NULL); - info->has_blocked_reasons = info->blocked; info->blocked_reasons = NULL; - if (info->blocked) { - GSList *cur_blocker = migration_blockers; - /* - * There are two types of reasons a migration might be blocked; - * a) devices marked in VMState as non-migratable, and - * b) Explicit migration blockers - * We need to add both of them here. - */ - qemu_savevm_non_migratable_list(&info->blocked_reasons); + /* + * There are two types of reasons a migration might be blocked; + * a) devices marked in VMState as non-migratable, and + * b) Explicit migration blockers + * We need to add both of them here. + */ + qemu_savevm_non_migratable_list(&info->blocked_reasons); - while (cur_blocker) { - QAPI_LIST_PREPEND(info->blocked_reasons, - g_strdup(error_get_pretty(cur_blocker->data))); - cur_blocker = g_slist_next(cur_blocker); - } + while (cur_blocker) { + QAPI_LIST_PREPEND(info->blocked_reasons, + g_strdup(error_get_pretty(cur_blocker->data))); + cur_blocker = g_slist_next(cur_blocker); } + info->has_blocked_reasons = info->blocked_reasons != NULL; switch (s->state) { case MIGRATION_STATUS_NONE: diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 0ad5b77477..d9bef63373 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -224,7 +224,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict) migration_global_dump(mon); - if (info->blocked) { + if (info->blocked_reasons) { strList *reasons = info->blocked_reasons; monitor_printf(mon, "Outgoing migration blocked:\n"); while (reasons) {
Result @blocked is true when and only when result @blocked-reasons is present. It's always non-empty when present. @blocked is redundant; drop. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qapi/migration.json | 7 +++---- migration/migration.c | 29 +++++++++++++---------------- monitor/hmp-cmds.c | 2 +- 3 files changed, 17 insertions(+), 21 deletions(-)