On Mon, 2010-05-17 at 12:10 -0600, Jim Fehlig wrote:
The qemu driver contains a subtle race in the logic to find next available vnc port. Currently it iterates through all available ports and returns the first for which bind(2) succeeds. However it is possible that a previously issued port has not yet been bound by qemu, resulting in the same port used for a subsequent domain.
This patch addresses the race by using a simple bitmap to "reserve" the ports allocated by libvirt. --- src/qemu/qemu_driver.c | 22 ++++++++++++++++++++++ 1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 582fdee..329859e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2629,13 +2629,24 @@ qemuInitPCIAddresses(struct qemud_driver *driver, return ret; }
+static virBitmapPtr vnc_used_port_bitmap; + static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) { int i;
+ if (vnc_used_port_bitmap == NULL) { + if ((vnc_used_port_bitmap = virBitmapAlloc(59635)) == NULL) + return -1; + } + I think you could move the above into qmudStartup.
Maybe replace the 59635 with (VNC_PORT_MAX - VNC_PORT_MIN) so one can see the math ...
for (i = 5900 ; i < 65535 ; i++) {
... and replace 5900 here with VNC_PORT_MIN and 65535 with VNC_PORT_MAX.
int fd; int reuse = 1; struct sockaddr_in addr; + + if (virBitmapGetBit(vnc_used_port_bitmap, i - 5900)) + continue; + addr.sin_family = AF_INET; addr.sin_port = htons(i); addr.sin_addr.s_addr = htonl(INADDR_ANY); @@ -2651,6 +2662,8 @@ static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) { if (bind(fd, (struct sockaddr*)&addr, sizeof(addr)) == 0) { /* Not in use, lets grab it */ close(fd); + /* Add port to bitmap of reserved ports */ + virBitmapSetBit(vnc_used_port_bitmap, i - 5900); return i; } close(fd); @@ -3717,6 +3730,15 @@ retry:
qemudRemoveDomainStatus(driver, vm);
+ /* Remove VNC port from vnc_used_port_bitmap, but only if it was reserved + by the driver (autoport=yes) + */ + if ((vm->def->ngraphics == 1) && + vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && + vm->def->graphics[0]->data.vnc.autoport) + virBitmapClearBit(vnc_used_port_bitmap, + vm->def->graphics[0]->data.vnc.port - 5900); + vm->pid = -1; vm->def->id = -1; vm->state = VIR_DOMAIN_SHUTOFF;
Afaict, you also need to add cleanup to the 'cleanup:' part of qemudStartVMDaemon() after checking that the port has been allocated. Stefan