Re: [libvirt PATCH v2 4/5] qemu_migration: get migration blockers before hardcoded checks

On Wed, Jul 20, 2022 at 11:11:53 +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; > --- > src/qemu/qemu_migration.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c > index b12cb518ee..4224339f39 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,25 @@ 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")); If qemuDomainGetMigrationBlockers returned -1 a better error message should already be set and you would overwrite it here. Also with your current patch you would report this error even in case there was no migration blocker reported by QEMU. But this part would be fixed by letting qemuDomainGetMigrationBlockers return 0 when the blocked-reasons field is missing.

> + return false; > + } > + > + if (blockers[0]) { blockers && blockers[0] > + virReportError(VIR_ERR_OPERATION_INVALID, > + _("cannot migrate domain, %s"), blockers[0]); I would prefer a colon there: "cannot migrate domain: %s". And we got a list of blockers from QEMU, but only use the first one? Please, join them with g_strjoinv().

> + return false; > + } > + } > > /* perform these checks only when migrating to remote hosts */ > if (remote) { Jirka