Re: [libvirt] [PATCH 2/5] qemu: Store supported migration capabilities in a bitmap

[Jiri Denemark]

On Fri, Oct 20, 2017 at 07:08:37 -0400, John Ferlan wrote:
> 
> 
> On 10/20/2017 03:03 AM, Jiri Denemark wrote:
> > On Thu, Oct 19, 2017 at 18:30:53 -0400, John Ferlan wrote:
> >>
> >>
> >> On 10/18/2017 07:29 AM, Jiri Denemark wrote:
> >>> Each time we need to check whether a given migration capability is
> >>> supported by QEMU, we call query-migrate-capabilities QMP command and
> >>> lookup the capability in the returned list. Asking for the list of
> >>> supported capabilities once when we connect to QEMU and storing the
> >>> result in a bitmap is much better and we don't need to enter a monitor
> >>> just to check whether a migration capability is supported.
> >>>
> >>> Signed-off-by: Jiri Denemark 
> >>> ---
> >>>  src/qemu/qemu_domain.c  | 68 
> >>> +
> >>>  src/qemu/qemu_domain.h  |  9 +++
> >>>  src/qemu/qemu_process.c | 13 +-
> >>>  3 files changed, 78 insertions(+), 12 deletions(-)
> >>>
> >>
> >> There's no corresponding adjustment to qemuDomainObjPrivateXMLFormat and
> >> qemuDomainObjPrivateXMLParse in order to handle the restart scenario.
> >>
> >> The rest of this looks OK, but do you need the Format/Parse logic for
> >> the bitmap?
> > 
> > No. The migration capabilities are rechecked every time libvirt connects
> > to QEMU as said in the commit message and in qemu_domain.h:
> > 
> 
> OK, so to be official...
> 
> Reviewed-by: John Ferlan 

I pushed this series. Thanks for the review.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/5] qemu: Store supported migration capabilities in a bitmap

[John Ferlan]

On 10/20/2017 03:03 AM, Jiri Denemark wrote:
> On Thu, Oct 19, 2017 at 18:30:53 -0400, John Ferlan wrote:
>>
>>
>> On 10/18/2017 07:29 AM, Jiri Denemark wrote:
>>> Each time we need to check whether a given migration capability is
>>> supported by QEMU, we call query-migrate-capabilities QMP command and
>>> lookup the capability in the returned list. Asking for the list of
>>> supported capabilities once when we connect to QEMU and storing the
>>> result in a bitmap is much better and we don't need to enter a monitor
>>> just to check whether a migration capability is supported.
>>>
>>> Signed-off-by: Jiri Denemark 
>>> ---
>>>  src/qemu/qemu_domain.c  | 68 
>>> +
>>>  src/qemu/qemu_domain.h  |  9 +++
>>>  src/qemu/qemu_process.c | 13 +-
>>>  3 files changed, 78 insertions(+), 12 deletions(-)
>>>
>>
>> There's no corresponding adjustment to qemuDomainObjPrivateXMLFormat and
>> qemuDomainObjPrivateXMLParse in order to handle the restart scenario.
>>
>> The rest of this looks OK, but do you need the Format/Parse logic for
>> the bitmap?
> 
> No. The migration capabilities are rechecked every time libvirt connects
> to QEMU as said in the commit message and in qemu_domain.h:
> 

OK, so to be official...

Reviewed-by: John Ferlan 

John

>>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>>> index 5201c6a0ac..fb20d8ea63 100644
>>> --- a/src/qemu/qemu_domain.h
>>> +++ b/src/qemu/qemu_domain.h
>>> @@ -323,6 +323,10 @@ struct _qemuDomainObjPrivate {
>>>  
>>>  /* Tracks blockjob state for vm. Valid only while reconnecting to 
>>> qemu. */
>>>  virTristateBool reconnectBlockjobs;
>>> +
>>> +/* Migration capabilities. Rechecked on reconnect, not to be saved in
>>> + * private XML. */
>>> +virBitmapPtr migrationCaps;
>>>  };
> 
> Jirka
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/5] qemu: Store supported migration capabilities in a bitmap

[Jiri Denemark]

On Thu, Oct 19, 2017 at 18:30:53 -0400, John Ferlan wrote:
> 
> 
> On 10/18/2017 07:29 AM, Jiri Denemark wrote:
> > Each time we need to check whether a given migration capability is
> > supported by QEMU, we call query-migrate-capabilities QMP command and
> > lookup the capability in the returned list. Asking for the list of
> > supported capabilities once when we connect to QEMU and storing the
> > result in a bitmap is much better and we don't need to enter a monitor
> > just to check whether a migration capability is supported.
> > 
> > Signed-off-by: Jiri Denemark 
> > ---
> >  src/qemu/qemu_domain.c  | 68 
> > +
> >  src/qemu/qemu_domain.h  |  9 +++
> >  src/qemu/qemu_process.c | 13 +-
> >  3 files changed, 78 insertions(+), 12 deletions(-)
> > 
> 
> There's no corresponding adjustment to qemuDomainObjPrivateXMLFormat and
> qemuDomainObjPrivateXMLParse in order to handle the restart scenario.
> 
> The rest of this looks OK, but do you need the Format/Parse logic for
> the bitmap?

No. The migration capabilities are rechecked every time libvirt connects
to QEMU as said in the commit message and in qemu_domain.h:

> > diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> > index 5201c6a0ac..fb20d8ea63 100644
> > --- a/src/qemu/qemu_domain.h
> > +++ b/src/qemu/qemu_domain.h
> > @@ -323,6 +323,10 @@ struct _qemuDomainObjPrivate {
> >  
> >  /* Tracks blockjob state for vm. Valid only while reconnecting to 
> > qemu. */
> >  virTristateBool reconnectBlockjobs;
> > +
> > +/* Migration capabilities. Rechecked on reconnect, not to be saved in
> > + * private XML. */
> > +virBitmapPtr migrationCaps;
> >  };

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/5] qemu: Store supported migration capabilities in a bitmap

[John Ferlan]

On 10/18/2017 07:29 AM, Jiri Denemark wrote:
> Each time we need to check whether a given migration capability is
> supported by QEMU, we call query-migrate-capabilities QMP command and
> lookup the capability in the returned list. Asking for the list of
> supported capabilities once when we connect to QEMU and storing the
> result in a bitmap is much better and we don't need to enter a monitor
> just to check whether a migration capability is supported.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_domain.c  | 68 
> +
>  src/qemu/qemu_domain.h  |  9 +++
>  src/qemu/qemu_process.c | 13 +-
>  3 files changed, 78 insertions(+), 12 deletions(-)
> 

There's no corresponding adjustment to qemuDomainObjPrivateXMLFormat and
qemuDomainObjPrivateXMLParse in order to handle the restart scenario.

The rest of this looks OK, but do you need the Format/Parse logic for
the bitmap?  It was late in my day and I was too lazy to go chase seeing
as I already did the merging ;-)

John

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 05e8b96aa4..a8cabc5727 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1767,6 +1767,9 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr 
> priv)
>  priv->namespaces = NULL;
>  
>  priv->reconnectBlockjobs = VIR_TRISTATE_BOOL_ABSENT;
> +
> +virBitmapFree(priv->migrationCaps);
> +priv->migrationCaps = NULL;
>  }
>  
>  
> @@ -10122,3 +10125,68 @@ qemuDomainGetMachineName(virDomainObjPtr vm)
>  
>  return ret;
>  }
> +
> +
> +int
> +qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + qemuDomainAsyncJob asyncJob)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +char **caps = NULL;
> +char **capStr;
> +int ret = -1;
> +int rc;
> +
> +if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> +return -1;
> +
> +rc = qemuMonitorGetMigrationCapabilities(priv->mon, &caps);
> +
> +if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
> +goto cleanup;
> +
> +if (!caps) {
> +ret = 0;
> +goto cleanup;
> +}
> +
> +priv->migrationCaps = virBitmapNew(QEMU_MONITOR_MIGRATION_CAPS_LAST);
> +if (!priv->migrationCaps)
> +goto cleanup;
> +
> +for (capStr = caps; *capStr; capStr++) {
> +int cap = qemuMonitorMigrationCapsTypeFromString(*capStr);
> +
> +if (cap < 0) {
> +VIR_DEBUG("Unknown migration capability: '%s'", *capStr);
> +} else {
> +ignore_value(virBitmapSetBit(priv->migrationCaps, cap));
> +VIR_DEBUG("Found migration capability: '%s'", *capStr);
> +}
> +}
> +
> +if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
> +if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> +goto cleanup;
> +
> +rc = qemuMonitorSetMigrationCapability(priv->mon,
> +   
> QEMU_MONITOR_MIGRATION_CAPS_EVENTS,
> +   true);
> +
> +if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +goto cleanup;
> +
> +if (rc < 0) {
> +virResetLastError();
> +VIR_DEBUG("Cannot enable migration events; clearing capability");
> +virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
> +}
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +virStringListFree(caps);
> +return ret;
> +}
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 5201c6a0ac..fb20d8ea63 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -323,6 +323,10 @@ struct _qemuDomainObjPrivate {
>  
>  /* Tracks blockjob state for vm. Valid only while reconnecting to qemu. 
> */
>  virTristateBool reconnectBlockjobs;
> +
> +/* Migration capabilities. Rechecked on reconnect, not to be saved in
> + * private XML. */
> +virBitmapPtr migrationCaps;
>  };
>  
>  # define QEMU_DOMAIN_PRIVATE(vm) \
> @@ -978,4 +982,9 @@ qemuDomainFixupCPUs(virDomainObjPtr vm,
>  char *
>  qemuDomainGetMachineName(virDomainObjPtr vm);
>  
> +int
> +qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + qemuDomainAsyncJob asyncJob);
> +
>  #endif /* __QEMU_DOMAIN_H__ */
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 24675498a2..cea2f90ce1 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1816,18 +1816,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
> virDomainObjPtr vm, int asyncJob,
>  if (qemuProcessInitMonitor(driver, vm, asyncJob) < 0)
>  return -1;
>  
> -if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> -

[libvirt] [PATCH 2/5] qemu: Store supported migration capabilities in a bitmap

[Jiri Denemark]

Each time we need to check whether a given migration capability is
supported by QEMU, we call query-migrate-capabilities QMP command and
lookup the capability in the returned list. Asking for the list of
supported capabilities once when we connect to QEMU and storing the
result in a bitmap is much better and we don't need to enter a monitor
just to check whether a migration capability is supported.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_domain.c  | 68 +
 src/qemu/qemu_domain.h  |  9 +++
 src/qemu/qemu_process.c | 13 +-
 3 files changed, 78 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 05e8b96aa4..a8cabc5727 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1767,6 +1767,9 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr 
priv)
 priv->namespaces = NULL;
 
 priv->reconnectBlockjobs = VIR_TRISTATE_BOOL_ABSENT;
+
+virBitmapFree(priv->migrationCaps);
+priv->migrationCaps = NULL;
 }
 
 
@@ -10122,3 +10125,68 @@ qemuDomainGetMachineName(virDomainObjPtr vm)
 
 return ret;
 }
+
+
+int
+qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuDomainAsyncJob asyncJob)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+char **caps = NULL;
+char **capStr;
+int ret = -1;
+int rc;
+
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+return -1;
+
+rc = qemuMonitorGetMigrationCapabilities(priv->mon, &caps);
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
+goto cleanup;
+
+if (!caps) {
+ret = 0;
+goto cleanup;
+}
+
+priv->migrationCaps = virBitmapNew(QEMU_MONITOR_MIGRATION_CAPS_LAST);
+if (!priv->migrationCaps)
+goto cleanup;
+
+for (capStr = caps; *capStr; capStr++) {
+int cap = qemuMonitorMigrationCapsTypeFromString(*capStr);
+
+if (cap < 0) {
+VIR_DEBUG("Unknown migration capability: '%s'", *capStr);
+} else {
+ignore_value(virBitmapSetBit(priv->migrationCaps, cap));
+VIR_DEBUG("Found migration capability: '%s'", *capStr);
+}
+}
+
+if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT)) {
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+goto cleanup;
+
+rc = qemuMonitorSetMigrationCapability(priv->mon,
+   
QEMU_MONITOR_MIGRATION_CAPS_EVENTS,
+   true);
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+goto cleanup;
+
+if (rc < 0) {
+virResetLastError();
+VIR_DEBUG("Cannot enable migration events; clearing capability");
+virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
+}
+}
+
+ret = 0;
+
+ cleanup:
+virStringListFree(caps);
+return ret;
+}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 5201c6a0ac..fb20d8ea63 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -323,6 +323,10 @@ struct _qemuDomainObjPrivate {
 
 /* Tracks blockjob state for vm. Valid only while reconnecting to qemu. */
 virTristateBool reconnectBlockjobs;
+
+/* Migration capabilities. Rechecked on reconnect, not to be saved in
+ * private XML. */
+virBitmapPtr migrationCaps;
 };
 
 # define QEMU_DOMAIN_PRIVATE(vm)   \
@@ -978,4 +982,9 @@ qemuDomainFixupCPUs(virDomainObjPtr vm,
 char *
 qemuDomainGetMachineName(virDomainObjPtr vm);
 
+int
+qemuDomainCheckMigrationCapabilities(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuDomainAsyncJob asyncJob);
+
 #endif /* __QEMU_DOMAIN_H__ */
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 24675498a2..cea2f90ce1 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1816,18 +1816,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
virDomainObjPtr vm, int asyncJob,
 if (qemuProcessInitMonitor(driver, vm, asyncJob) < 0)
 return -1;
 
-if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
-return -1;
-
-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT) &&
-qemuMonitorSetMigrationCapability(priv->mon,
-  QEMU_MONITOR_MIGRATION_CAPS_EVENTS,
-  true) < 0) {
-VIR_DEBUG("Cannot enable migration events; clearing capability");
-virQEMUCapsClear(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
-}
-
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
+if (qemuDomainCheckMigrationCapabilities(driver, vm, asyncJob) < 0)
 return -1;
 
 return 0;
-- 
2.14.2

--
libvir-list mailing list
libvir-list@redhat.com