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

[Daniel P. Berrange]

On Mon, May 17, 2010 at 12:10:00PM -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;

This should be a field in 'struct qemud_driver' rather than static.

>  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;
> +    }
> +
>      for (i = 5900 ; i < 65535 ; i++) {
>          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;

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|