Re: [libvirt PATCH v3 3/4] qemu_migration: get migration blockers before hardcoded checks

On Wed, Jul 20, 2022 at 16:29:45 +0200, Eugenio Perez Martin wrote: > On Wed, Jul 20, 2022 at 4:01 PM Jiri Denemark <jdenemar(a)redhat.com&gt; wrote: > > > > On Wed, Jul 20, 2022 at 14:15:57 +0200, Eugenio Pérez wrote: > > > since qemu 6.0, if migration is blocked for some reason, 'query-migrate' > > > will return an array of error strings describing the migration blockers. > > > This can be used to check whether there are any devices blocking > > > migration, etc. > > > > > > Enable qemuMigrationSrcIsAllowed to query it. > > > > > > Signed-off-by: Eugenio Pérez <eperezma(a)redhat.com&gt; > > > --- > > > v3: > > > * Report message with a colon. > > > * Report all blockers instead of only the first. > > > --- > > > src/qemu/qemu_migration.c | 34 ++++++++++++++++++++++++++++++++++ > > > 1 file changed, 34 insertions(+) > > > > > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > > > index b12cb518ee..6ac4ef150b 100644 > > > --- a/src/qemu/qemu_migration.c > > > +++ b/src/qemu/qemu_migration.c > > > @@ -1414,6 +1414,20 @@ qemuMigrationSrcIsAllowedHostdev(const virDomainDef *def) > > > return true; > > > } > > > > > > +static int > > > +qemuDomainGetMigrationBlockers(virQEMUDriver *driver, > > > + virDomainObj *vm, > > > + char ***blockers) > > > +{ > > > + qemuDomainObjPrivate *priv = vm->privateData; > > > + int rc; > > > + > > > + qemuDomainObjEnterMonitor(driver, vm); > > > + rc = qemuMonitorGetMigrationBlockers(priv->mon, blockers); > > > + qemuDomainObjExitMonitor(vm); > > > + > > > + return rc; > > > +} > > > > > > /** > > > * qemuMigrationSrcIsAllowed: > > > @@ -1439,6 +1453,26 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver, > > > int nsnapshots; > > > int pauseReason; > > > size_t i; > > > + int r; > > > + > > > + /* Ask qemu if it have a migration blocker */ > > > + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) { > > > + g_auto(GStrv) blockers = NULL; > > > + r = qemuDomainGetMigrationBlockers(driver, vm, &blockers); > > > + if (r != 0) { > > > + virReportError(VIR_ERR_OPERATION_INVALID, > > > + _("cannot migrate domain: %s"), > > > + _("error getting blockers")); > > > + return false; > > > + } > > > > As mentioned in v2 review the virReportError call should be dropped as > > it overwrites the error reported by qemuDomainGetMigrationBlockers. That > > is, you can just > > > > if (qemuDomainGetMigrationBlockers(driver, vm, &blockers) < 0) > > return false; > > > > But there are a few conditions that don't report an error, like a bad > JSON answer. For example, if "blockers" is not an array parsing the > response JSON, libvirt would not print any error, isn't it? Well in qemuDomainGetMigrationBlockers you now have if (!(jblockers = virJSONValueObjectGetArray(data, "blocked-reasons"))) return 0; so you would not get pass r != 0 in such case anyway. If you wanted to distinguish missing blocked-reasons (that is no blockers) and blocked-reasons which is not an array (invalid response), you would need to do something else: if (!(jblockers = virJSONValueObjectGet(data, "blocked-reasons"))) return 0; if (!virJSONValueIsArray(jblockers)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("blocker-reasons is not an array")); return -1; } All this inside qemuDomainGetMigrationBlockers() to make sure the function reports an error when returning -1. The caller would not report any error anyway.