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

[Martin Kletzander]

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).

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131122/8390dcbc/attachment-0001.sig>