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); } /* Remove any cputune settings */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a26c079..dfaf95f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4331,25 +4331,35 @@ retry: qemuProcessRemoveDomainStatus(driver, vm); - /* Remove VNC and Spice ports from port reservation bitmap, but only if - they were reserved by the driver (autoport=yes) - */ + /* Remove remote ports from all port allocators, even when + * autoport isn't set, because they might have been auto-allocated + * anyway. It also helps keeping the allocator data a tad more + * updated. + */ for (i = 0; i < vm->def->ngraphics; ++i) { virDomainGraphicsDefPtr graphics = vm->def->graphics[i]; - if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { - if (graphics->data.vnc.autoport) { - virPortAllocatorRelease(driver->remotePorts, - graphics->data.vnc.port); - } + switch ((enum virDomainGraphicsType) graphics->type) { + case VIR_DOMAIN_GRAPHICS_TYPE_VNC: + virPortAllocatorRelease(driver->remotePorts, + graphics->data.vnc.port); virPortAllocatorRelease(driver->webSocketPorts, graphics->data.vnc.websocket); - } - if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE && - graphics->data.spice.autoport) { + break; + + case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: virPortAllocatorRelease(driver->remotePorts, graphics->data.spice.port); virPortAllocatorRelease(driver->remotePorts, graphics->data.spice.tlsPort); + break; + + case VIR_DOMAIN_GRAPHICS_TYPE_SDL: + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: + /* Nothing has been allocated */ + + case VIR_DOMAIN_GRAPHICS_TYPE_LAST: + break; } } diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index 694f191..f7f3755 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -27,11 +27,12 @@ #include "viralloc.h" #include "virbitmap.h" -#include "virportallocator.h" -#include "virthread.h" #include "virerror.h" #include "virfile.h" +#include "virlog.h" +#include "virportallocator.h" #include "virstring.h" +#include "virthread.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -172,33 +173,17 @@ cleanup: return ret; } -int virPortAllocatorRelease(virPortAllocatorPtr pa, - unsigned short port) +void virPortAllocatorRelease(virPortAllocatorPtr pa, + unsigned short port) { - int ret = -1; - if (!port) - return 0; + return; virObjectLock(pa); - if (port < pa->start || - port > pa->end) { - virReportInvalidArg(port, "port %d must be in range (%d, %d)", - port, pa->start, pa->end); - goto cleanup; - } - - if (virBitmapClearBit(pa->bitmap, - port - pa->start) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to release port %d"), - port); - goto cleanup; - } + if (port >= pa->start && port <= pa->end && + virBitmapClearBit(pa->bitmap, port - pa->start) < 0) + VIR_WARN("Problem releasing port %d from range '%s'", port, pa->name); - ret = 0; -cleanup: virObjectUnlock(pa); - return ret; } diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h index c8aa6de..ba7c504 100644 --- a/src/util/virportallocator.h +++ b/src/util/virportallocator.h @@ -35,7 +35,7 @@ virPortAllocatorPtr virPortAllocatorNew(const char *name, int virPortAllocatorAcquire(virPortAllocatorPtr pa, unsigned short *port); -int virPortAllocatorRelease(virPortAllocatorPtr pa, - unsigned short port); +void virPortAllocatorRelease(virPortAllocatorPtr pa, + unsigned short port); #endif /* __VIR_PORT_ALLOCATOR_H__ */ diff --git a/tests/virportallocatortest.c b/tests/virportallocatortest.c index 721356e..0d41c98 100644 --- a/tests/virportallocatortest.c +++ b/tests/virportallocatortest.c @@ -165,10 +165,7 @@ static int testAllocReuse(const void *args ATTRIBUTE_UNUSED) goto cleanup; } - - if (virPortAllocatorRelease(alloc, p2) < 0) - goto cleanup; - + virPortAllocatorRelease(alloc, p2); if (virPortAllocatorAcquire(alloc, &p4) < 0) goto cleanup; if (p4 != 5902) { -- 1.8.4.3