Re: [libvirt PATCH 04/80] qemu: Enable migration events only when disabled

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(a)redhat.com&gt; > --- > 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(a)redhat.com&gt;