[libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers

Eugenio Pérez posted 5 patches 3 years, 6 months ago
There is a newer version of this series
[libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers
Posted by Eugenio Pérez 3 years, 6 months ago
vDPA devices will be migratable soon. Since they are not migratable
before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
for migration blockers, let it hardcoded in that case.

Otherwise, ask qemu about the explicit blocker.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 src/qemu/qemu_migration.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 4224339f39..2f5c1d8121 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
     int pauseReason;
     size_t i;
     int r;
+    bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
+                                          QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
 
     /* Ask qemu if it have a migration blocker */
-    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
+    if (blockedReasonsCap) {
         g_auto(GStrv) blockers = NULL;
         r = qemuDomainGetMigrationBlockers(driver, vm, &blockers);
         if (r != 0) {
@@ -1579,7 +1581,8 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
             virDomainNetDef *net = vm->def->nets[i];
             qemuSlirp *slirp;
 
-            if (net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
+
+            if (!blockedReasonsCap && net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
                 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                                _("vDPA devices cannot be migrated"));
                 return false;
-- 
2.31.1

Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers
Posted by Jiri Denemark 3 years, 6 months ago
On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> vDPA devices will be migratable soon. Since they are not migratable
> before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> for migration blockers, let it hardcoded in that case.
> 
> Otherwise, ask qemu about the explicit blocker.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  src/qemu/qemu_migration.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 4224339f39..2f5c1d8121 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
>      int pauseReason;
>      size_t i;
>      int r;
> +    bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
> +                                          QEMU_CAPS_MIGRATION_BLOCKED_REASONS);

Wrong indentation of the second line (QEMU... should be aligned with
priv).

>  
>      /* Ask qemu if it have a migration blocker */
> -    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
> +    if (blockedReasonsCap) {
>          g_auto(GStrv) blockers = NULL;
>          r = qemuDomainGetMigrationBlockers(driver, vm, &blockers);
>          if (r != 0) {
> @@ -1579,7 +1581,8 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
>              virDomainNetDef *net = vm->def->nets[i];
>              qemuSlirp *slirp;
>  
> -            if (net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
> +

Extra empty line.

> +            if (!blockedReasonsCap && net->type == VIR_DOMAIN_NET_TYPE_VDPA) {

So possibly we could skip more checks in the function if migration
blockers are supported by QEMU, but this is a good conservative
approach. The other checks (if any) can be taken care of separately.

>                  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>                                 _("vDPA devices cannot be migrated"));
>                  return false;

Jirka
Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers
Posted by Eugenio Perez Martin 3 years, 6 months ago
On Wed, Jul 20, 2022 at 12:36 PM Jiri Denemark <jdenemar@redhat.com> wrote:
>
> On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> > vDPA devices will be migratable soon. Since they are not migratable
> > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > for migration blockers, let it hardcoded in that case.
> >
> > Otherwise, ask qemu about the explicit blocker.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  src/qemu/qemu_migration.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 4224339f39..2f5c1d8121 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> >      int pauseReason;
> >      size_t i;
> >      int r;
> > +    bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
> > +                                          QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
>
> Wrong indentation of the second line (QEMU... should be aligned with
> priv).
>

I'm fine with that indentation, but that will be a line with more than 80 chars.

> >
> >      /* Ask qemu if it have a migration blocker */
> > -    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_BLOCKED_REASONS)) {
> > +    if (blockedReasonsCap) {
> >          g_auto(GStrv) blockers = NULL;
> >          r = qemuDomainGetMigrationBlockers(driver, vm, &blockers);
> >          if (r != 0) {
> > @@ -1579,7 +1581,8 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> >              virDomainNetDef *net = vm->def->nets[i];
> >              qemuSlirp *slirp;
> >
> > -            if (net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
> > +
>
> Extra empty line.
>

I'll delete it in the next version.

> > +            if (!blockedReasonsCap && net->type == VIR_DOMAIN_NET_TYPE_VDPA) {
>
> So possibly we could skip more checks in the function if migration
> blockers are supported by QEMU, but this is a good conservative
> approach. The other checks (if any) can be taken care of separately.
>

I did a fast search but I didn't see something super obvious to me.

Thanks!

> >                  virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> >                                 _("vDPA devices cannot be migrated"));
> >                  return false;
>
> Jirka
>
Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers
Posted by Jiri Denemark 3 years, 6 months ago
On Wed, Jul 20, 2022 at 13:11:57 +0200, Eugenio Perez Martin wrote:
> On Wed, Jul 20, 2022 at 12:36 PM Jiri Denemark <jdenemar@redhat.com> wrote:
> >
> > On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> > > vDPA devices will be migratable soon. Since they are not migratable
> > > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > > for migration blockers, let it hardcoded in that case.
> > >
> > > Otherwise, ask qemu about the explicit blocker.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  src/qemu/qemu_migration.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > index 4224339f39..2f5c1d8121 100644
> > > --- a/src/qemu/qemu_migration.c
> > > +++ b/src/qemu/qemu_migration.c
> > > @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> > >      int pauseReason;
> > >      size_t i;
> > >      int r;
> > > +    bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
> > > +                                          QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
> >
> > Wrong indentation of the second line (QEMU... should be aligned with
> > priv).
> >
> 
> I'm fine with that indentation, but that will be a line with more than 80 chars.

No problem, it's not a hard requirement anymore in cases where it would
make the result look worse.

Jirka
Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers
Posted by Peter Krempa 3 years, 6 months ago
On Wed, Jul 20, 2022 at 13:11:57 +0200, Eugenio Perez Martin wrote:
> On Wed, Jul 20, 2022 at 12:36 PM Jiri Denemark <jdenemar@redhat.com> wrote:
> >
> > On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> > > vDPA devices will be migratable soon. Since they are not migratable
> > > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > > for migration blockers, let it hardcoded in that case.
> > >
> > > Otherwise, ask qemu about the explicit blocker.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  src/qemu/qemu_migration.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > index 4224339f39..2f5c1d8121 100644
> > > --- a/src/qemu/qemu_migration.c
> > > +++ b/src/qemu/qemu_migration.c
> > > @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> > >      int pauseReason;
> > >      size_t i;
> > >      int r;
> > > +    bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
> > > +                                          QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
> >
> > Wrong indentation of the second line (QEMU... should be aligned with
> > priv).
> >
> 
> I'm fine with that indentation, but that will be a line with more than 80 chars.

The max 80 columns limitation is only a soft limit. We prefer way more
to keep alignment and readability of the code rather than stick to the
antiquated 80 columns limit. 
Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers
Posted by Eugenio Perez Martin 3 years, 6 months ago
On Wed, Jul 20, 2022 at 1:13 PM Peter Krempa <pkrempa@redhat.com> wrote:
>
> On Wed, Jul 20, 2022 at 13:11:57 +0200, Eugenio Perez Martin wrote:
> > On Wed, Jul 20, 2022 at 12:36 PM Jiri Denemark <jdenemar@redhat.com> wrote:
> > >
> > > On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> > > > vDPA devices will be migratable soon. Since they are not migratable
> > > > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > > > for migration blockers, let it hardcoded in that case.
> > > >
> > > > Otherwise, ask qemu about the explicit blocker.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > >  src/qemu/qemu_migration.c | 7 +++++--
> > > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > > index 4224339f39..2f5c1d8121 100644
> > > > --- a/src/qemu/qemu_migration.c
> > > > +++ b/src/qemu/qemu_migration.c
> > > > @@ -1454,9 +1454,11 @@ qemuMigrationSrcIsAllowed(virQEMUDriver *driver,
> > > >      int pauseReason;
> > > >      size_t i;
> > > >      int r;
> > > > +    bool blockedReasonsCap = virQEMUCapsGet(priv->qemuCaps,
> > > > +                                          QEMU_CAPS_MIGRATION_BLOCKED_REASONS);
> > >
> > > Wrong indentation of the second line (QEMU... should be aligned with
> > > priv).
> > >
> >
> > I'm fine with that indentation, but that will be a line with more than 80 chars.
>
> The max 80 columns limitation is only a soft limit. We prefer way more
> to keep alignment and readability of the code rather than stick to the
> antiquated 80 columns limit.
>

Got it, I'll indent properly for the next version then.

Thanks!
Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers
Posted by Jiri Denemark 3 years, 6 months ago
On Wed, Jul 20, 2022 at 12:36:16 +0200, Jiri Denemark wrote:
> On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> > vDPA devices will be migratable soon. Since they are not migratable
> > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > for migration blockers, let it hardcoded in that case.
> > 
> > Otherwise, ask qemu about the explicit blocker.
> > 
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  src/qemu/qemu_migration.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)

Oh and I forgot to mention s/qemu_migrate/qemu_migration/ in the
subject.

Jirka
Re: [libvirt PATCH v2 5/5] qemu_migrate: Do not forbif vDPA devices if can query blockers
Posted by Eugenio Perez Martin 3 years, 6 months ago
On Wed, Jul 20, 2022 at 12:37 PM Jiri Denemark <jdenemar@redhat.com> wrote:
>
> On Wed, Jul 20, 2022 at 12:36:16 +0200, Jiri Denemark wrote:
> > On Wed, Jul 20, 2022 at 11:11:54 +0200, Eugenio Pérez wrote:
> > > vDPA devices will be migratable soon. Since they are not migratable
> > > before qemu 6.0, and qemu pre-6.0 didn't have the capability of asking
> > > for migration blockers, let it hardcoded in that case.
> > >
> > > Otherwise, ask qemu about the explicit blocker.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  src/qemu/qemu_migration.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
>
> Oh and I forgot to mention s/qemu_migrate/qemu_migration/ in the
> subject.
>

I'll fix it in the next version.

Thanks!

> Jirka
>