[libvirt] [PATCH] util: Make port allocator data more up-to-date
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@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); } /* 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
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@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. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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@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
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@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
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Martin Kletzander