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@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;
+        }
+
+        if (blockers && blockers[0]) {
+            g_autofree char *reasons = g_strjoinv(", ", blockers);
+            virReportError(VIR_ERR_OPERATION_INVALID,
+                           _("cannot migrate domain: %s"), reasons);
+            return false;
+        }
+    }
 
     /* perform these checks only when migrating to remote hosts */
     if (remote) {
-- 
2.31.1

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@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;

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

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

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@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@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?

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.

Jirka

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

That's right.

Deleting error report then.

Thanks!