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

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(a)redhat.com&gt; > > --- > > 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).