When connecting to a QEMU monitor, we always try to enable migration
events, but this is an invalid operation during migration. Thus
reconnecting to a domain with active migration would fail. Let's check
the state of migration events capability and only try to enable it when
it is disabled.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/qemu/qemu_migration_params.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index 34416f89be..ed2d9ac103 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -1391,13 +1391,16 @@ qemuMigrationCapsCheck(virQEMUDriver *driver,
     g_autoptr(virBitmap) migEvent = NULL;
     g_autoptr(virJSONValue) json = NULL;
     g_auto(GStrv) caps = NULL;
+    g_autoptr(virBitmap) capState = NULL;
+    g_autoptr(virBitmap) enabled = virBitmapNew(QEMU_MIGRATION_CAP_LAST);
     char **capStr;
+    size_t i;
     int rc;
 
     if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
         return -1;
 
-    rc = qemuMonitorGetMigrationCapabilities(priv->mon, &caps, NULL);
+    rc = qemuMonitorGetMigrationCapabilities(priv->mon, &caps, &capState);
 
     qemuDomainObjExitMonitor(vm);
     if (rc < 0)
@@ -1408,7 +1411,7 @@ qemuMigrationCapsCheck(virQEMUDriver *driver,
 
     priv->migrationCaps = virBitmapNew(QEMU_MIGRATION_CAP_LAST);
 
-    for (capStr = caps; *capStr; capStr++) {
+    for (i = 0, capStr = caps; *capStr; capStr++, i++) {
         int cap = qemuMigrationCapabilityTypeFromString(*capStr);
 
         if (cap < 0) {
@@ -1416,10 +1419,20 @@ qemuMigrationCapsCheck(virQEMUDriver *driver,
         } else {
             ignore_value(virBitmapSetBit(priv->migrationCaps, cap));
             VIR_DEBUG("Found migration capability: '%s'", *capStr);
+
+            if (virBitmapIsBitSet(capState, i))
+                ignore_value(virBitmapSetBit(enabled, cap));
         }
     }
 
-    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
+    if (virBitmapIsBitSet(enabled, QEMU_MIGRATION_CAP_EVENTS)) {
+        if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
+            VIR_DEBUG("Migration events already enabled");
+        } else {
+            VIR_DEBUG("Migration events enabled; setting capability");
+            virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
+        }
+    } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
         migEvent = virBitmapNew(QEMU_MIGRATION_CAP_LAST);
 
         ignore_value(virBitmapSetBit(migEvent, QEMU_MIGRATION_CAP_EVENTS));
-- 
2.35.1

On Tue, May 10, 2022 at 17:20:25 +0200, Jiri Denemark wrote:
> When connecting to a QEMU monitor, we always try to enable migration
> events, but this is an invalid operation during migration. Thus
> reconnecting to a domain with active migration would fail. Let's check
> the state of migration events capability and only try to enable it when
> it is disabled.
> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  src/qemu/qemu_migration_params.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

[...]

> @@ -1416,10 +1419,20 @@ qemuMigrationCapsCheck(virQEMUDriver *driver,
>          } else {
>              ignore_value(virBitmapSetBit(priv->migrationCaps, cap));
>              VIR_DEBUG("Found migration capability: '%s'", *capStr);
> +
> +            if (virBitmapIsBitSet(capState, i))
> +                ignore_value(virBitmapSetBit(enabled, cap));
>          }
>      }
>  
> -    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
> +    if (virBitmapIsBitSet(enabled, QEMU_MIGRATION_CAP_EVENTS)) {
> +        if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
> +            VIR_DEBUG("Migration events already enabled");
> +        } else {
> +            VIR_DEBUG("Migration events enabled; setting capability");
> +            virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
> +        }

Is all of the "DEBUG" and checking 'qemuCaps' dance really needed? I
think you can simply unconditionally enable the capability, but is there
any reason it would not be enabled?

> +    } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
>          migEvent = virBitmapNew(QEMU_MIGRATION_CAP_LAST);
>  
>          ignore_value(virBitmapSetBit(migEvent, QEMU_MIGRATION_CAP_EVENTS));

If you provide a good enough explanation:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On Wed, May 11, 2022 at 10:37:25 +0200, Peter Krempa wrote:
> On Tue, May 10, 2022 at 17:20:25 +0200, Jiri Denemark wrote:
> > When connecting to a QEMU monitor, we always try to enable migration
> > events, but this is an invalid operation during migration. Thus
> > reconnecting to a domain with active migration would fail. Let's check
> > the state of migration events capability and only try to enable it when
> > it is disabled.
> > 
> > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > ---
> >  src/qemu/qemu_migration_params.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> [...]
> 
> > @@ -1416,10 +1419,20 @@ qemuMigrationCapsCheck(virQEMUDriver *driver,
> >          } else {
> >              ignore_value(virBitmapSetBit(priv->migrationCaps, cap));
> >              VIR_DEBUG("Found migration capability: '%s'", *capStr);
> > +
> > +            if (virBitmapIsBitSet(capState, i))
> > +                ignore_value(virBitmapSetBit(enabled, cap));
> >          }
> >      }
> >  
> > -    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
> > +    if (virBitmapIsBitSet(enabled, QEMU_MIGRATION_CAP_EVENTS)) {
> > +        if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
> > +            VIR_DEBUG("Migration events already enabled");
> > +        } else {
> > +            VIR_DEBUG("Migration events enabled; setting capability");
> > +            virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
> > +        }
> 
> Is all of the "DEBUG" and checking 'qemuCaps' dance really needed? I
> think you can simply unconditionally enable the capability, but is there
> any reason it would not be enabled?
> 
> > +    } else if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
> >          migEvent = virBitmapNew(QEMU_MIGRATION_CAP_LAST);
> >  
> >          ignore_value(virBitmapSetBit(migEvent, QEMU_MIGRATION_CAP_EVENTS));
> 
> If you provide a good enough explanation:
> 
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>

This patch was replaced with the "qemu: Drop QEMU_CAPS_MIGRATION_EVENT"
series.

Jirka

On Wed, May 11, 2022 at 10:37:25 +0200, Peter Krempa wrote:
> On Tue, May 10, 2022 at 17:20:25 +0200, Jiri Denemark wrote:
> > When connecting to a QEMU monitor, we always try to enable migration
> > events, but this is an invalid operation during migration. Thus
> > reconnecting to a domain with active migration would fail. Let's check
> > the state of migration events capability and only try to enable it when
> > it is disabled.
> > 
> > Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> > ---
> >  src/qemu/qemu_migration_params.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> [...]
> 
> > @@ -1416,10 +1419,20 @@ qemuMigrationCapsCheck(virQEMUDriver *driver,
> >          } else {
> >              ignore_value(virBitmapSetBit(priv->migrationCaps, cap));
> >              VIR_DEBUG("Found migration capability: '%s'", *capStr);
> > +
> > +            if (virBitmapIsBitSet(capState, i))
> > +                ignore_value(virBitmapSetBit(enabled, cap));
> >          }
> >      }
> >  
> > -    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
> > +    if (virBitmapIsBitSet(enabled, QEMU_MIGRATION_CAP_EVENTS)) {
> > +        if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
> > +            VIR_DEBUG("Migration events already enabled");
> > +        } else {
> > +            VIR_DEBUG("Migration events enabled; setting capability");
> > +            virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
> > +        }
> 
> Is all of the "DEBUG" and checking 'qemuCaps' dance really needed? I
> think you can simply unconditionally enable the capability, but is there
> any reason it would not be enabled?

Hmm, it looks like migration events are indeed supported by all QEMU
releases we support so we could drop some parts of this and most likely
even the previous patch :-)

Jirka