[libvirt] [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently

[Wangyufei (A)]

Yes, I get your point.

            this_port = QEMUD_MIGRATION_FIRST_PORT + port++;
            if (port == QEMUD_MIGRATION_NUM_PORTS)
                port = 0;

the operations above should be atomic, otherwise, port may be bigger than QEMUD_MIGRATION_NUM_PORTS and out of control at last. Right?

Using virPortAllocator is much more complicated, I need to think about it. I guess qemu driver lock removed has sent some devils out, we'll see.

Thanks for your reply.

> -----Original Message-----
> From: jdenemar at redhat.com [mailto:jdenemar at redhat.com]
> Sent: Friday, September 27, 2013 5:06 PM
> To: Wangyufei (A)
> Cc: libvir-list at redhat.com; Michal Privoznik; Wangrui (K)
> Subject: Re: [libvirt] [PATCH] qemu_migrate: Fix assign the same port when
> migrating concurrently
> 
> On Fri, Sep 27, 2013 at 06:28:50 +0000, Wangyufei (A) wrote:
> > From: WangYufei <james.wangyufei at huawei.com>
> > Date: Fri, 27 Sep 2013 11:53:57 +0800
> > Subject: [PATCH] qemu_migrate: Fix assign the same port when migrating
> concurrently
> >
> > When we migrate vms concurrently, there's a chance that libvirtd on
> destination assign the same port for different migrations, which will lead to
> migration failed during migration prepare phase on destination. So we
> change the port increase to atomic operation to solve the problem.
> 
> Oops, this was apparently latent until the big qemu driver lock was
> removed.
> 
> >
> > Signed-off-by: WangYufei <james.wangyufei at huawei.com>
> > ---
> >  src/qemu/qemu_migration.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 3a1aab7..0f496f4 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -57,6 +57,7 @@
> >  #include "virhook.h"
> >  #include "virstring.h"
> >  #include "virtypedparam.h"
> > +#include "viratomic.h"
> >
> >  #define VIR_FROM_THIS VIR_FROM_QEMU
> >
> > @@ -2521,7 +2522,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr
> driver,
> >       * to be a correct hostname which refers to the target machine).
> >       */
> >      if (uri_in == NULL) {
> > -        this_port = QEMUD_MIGRATION_FIRST_PORT + port++;
> > +        this_port = QEMUD_MIGRATION_FIRST_PORT +
> virAtomicIntInc(&port);
> >          if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0;
> >
> >          /* Get hostname */
> > @@ -2578,7 +2579,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr
> driver,
> >
> >          if (uri->port == 0) {
> >              /* Generate a port */
> > -            this_port = QEMUD_MIGRATION_FIRST_PORT + port++;
> > +            this_port = QEMUD_MIGRATION_FIRST_PORT +
> virAtomicIntInc(&port);
> >              if (port == QEMUD_MIGRATION_NUM_PORTS)
> >                  port = 0;
> >
> 
> Unfortunately, this patch is incomplete. The increments are now atomic
> but the wrapping at QEMUD_MIGRATION_NUM_PORTS is not. I think this
> should use virPortAllocator instead.
> 
> Jirka