Re: [libvirt] [PATCH 2/2] Fix race in finding available vnc port

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.