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

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.