[libvirt] [PATCH] util: Make port allocator data more up-to-date

[Jiri Denemark]

On Fri, Nov 22, 2013 at 15:41:28 +0100, Martin Kletzander wrote:
> On Fri, Nov 22, 2013 at 10:24:17AM +0000, Daniel P. Berrange wrote:
> > On Fri, Nov 22, 2013 at 06:01:44AM +0100, Martin Kletzander wrote:
> > > There are few places where we know about port getting opened even
> > > though it was not allocated on any virPortAllocator (or we know it was
> > > reserved like this).  We cannot notify the virPortAllocator that the
> > > port is open because of that (since the port may be out of range) and
> > > in that case, we are stuck with old data.
> > > 
> > > This also fixes the issue that if you have autoport='no' and
> > > port='-1', the port never gets returned (introduced by 7b4a630484);
> > > the case when we don't know whether to release the port or not.
> > > 
> > > There may be also other places where the port is being released even
> > > though it might have been set statically out of range by the user.  In
> > > that case we report an error, but that shouldn't stop APIs releasing
> > > ports (e.g. destroy).  Moreover, the return value is not checked
> > > sometimes and we're stuck with an error in the logs and also set as a
> > > lastError.
> > > 
> > > To make this behave the best I can imagine, we'd have to have a single
> > > virPortAllocator with configurable ranges (ports shared between
> > > allocators), but that's an idea for another day.  In this patch, I'm
> > > modifyping the virPortAllocatorRelease() function to be safe for all
> > > kinds of parameters, making it a no-op in case of out-of-range ports
> > > and warning in case virBitmapClearBit() returns an error (which is
> > > just a sanity code in combination with the range check).
> > > 
> > > Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> > > ---
> > >  src/libxl/libxl_driver.c     | 13 ++++---------
> > >  src/qemu/qemu_process.c      | 32 +++++++++++++++++++++-----------
> > >  src/util/virportallocator.c  | 33 +++++++++------------------------
> > >  src/util/virportallocator.h  |  4 ++--
> > >  tests/virportallocatortest.c |  5 +----
> > >  5 files changed, 37 insertions(+), 50 deletions(-)
> > > 
> > > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> > > index 1b42f14..26e2d3a 100644
> > > --- a/src/libxl/libxl_driver.c
> > > +++ b/src/libxl/libxl_driver.c
> > > @@ -279,15 +279,10 @@ libxlVmCleanup(libxlDriverPrivatePtr driver,
> > >      if (virAtomicIntDecAndTest(&driver->nactive) && driver->inhibitCallback)
> > >          driver->inhibitCallback(false, driver->inhibitOpaque);
> > > 
> > > -    if ((vm->def->ngraphics == 1) &&
> > > -        vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
> > > -        vm->def->graphics[0]->data.vnc.autoport) {
> > > -        vnc_port = vm->def->graphics[0]->data.vnc.port;
> > > -        if (vnc_port >= LIBXL_VNC_PORT_MIN) {
> > > -            if (virPortAllocatorRelease(driver->reservedVNCPorts,
> > > -                                        vnc_port) < 0)
> > > -                VIR_DEBUG("Could not mark port %d as unused", vnc_port);
> > > -        }
> > > +    if (vm->def->ngraphics == 1 &&
> > > +        vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> > > +        virPortAllocatorRelease(driver->reservedVNCPorts,
> > > +                                vm->def->graphics[0]->data.vnc.port);
> > >      }
> > 
> > I don't think this change or the others that follow are safe.
> > 
> > Consider we have guests, A & B.
> > 
> > Start A which is using autoport and it gets given port 5900
> > and this is recorded in the port allocator database.
> > 
> > Now start B which has static port, also 5900, and this is
> > *not* recorded in the port allocator database.
> > 
> > Now B tries to start up and fails, due to clashing port, so now
> > we run the cleanup code. Your change means we release port 5900,
> > which is the port belonging to A, not B.
> > 
> 
> Oh, this is a case I haven't considered, right.  So I think I'll need
> to come up with the shared port allocator and it will have a hint to
> who it currently using the code (in order to know whether to release
> it or not).

However, we sometimes (during incoming migration) allocate a port in one
thread and release it in another thread... :-)

Jirka