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

[Eugenio Perez Martin]

On Wed, Jul 20, 2022 at 4:01 PM Jiri Denemark <jdenemar at redhat.com> 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 at redhat.com>
> > ---
> > 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?

> > +
> > +        if (blockers && blockers[0]) {
> > +            g_autofree char *reasons = g_strjoinv(", ", blockers);
>
> In the following patch you change ", " to "; ". I don't mind that much
> either way, but it should be done in this patch :-)
>
> > +            virReportError(VIR_ERR_OPERATION_INVALID,
> > +                           _("cannot migrate domain: %s"), reasons);
> > +            return false;
> > +        }
> > +    }
> >
> >      /* perform these checks only when migrating to remote hosts */
> >      if (remote) {
>
> Hmm, easy but not trivial changes so I guess v4 would be better.
>
> Jirka
>