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

[Jiri Denemark]

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.

> +            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