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

[Jiri Denemark]

On Wed, Jul 20, 2022 at 13:25:45 +0200, Eugenio Perez Martin wrote:
> On Wed, Jul 20, 2022 at 12:31 PM Jiri Denemark <jdenemar at redhat.com> wrote:
> >
> > 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 at redhat.com>
> > > ---
> > >  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.
> >
> 
> Got it.
> 
> > > +            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().
> >
> 
> So the resulting message should be:
> "cannot migrate domain: <error1>, <error2>, ..., <errorN>", right?

Yeah, something like that... or with "; " as a separator to avoid
confusion in case any migration blocker contains ','.

Jirka