Stefan Berger wrote:
> 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.
>
I originally had it there but decided it was a waste of 64k when no
domains use vnc :-). I'll move it back.
> 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.
>
Right. Probably another patch that defines VNC_PORT_{MIN,MAX} and use
those in the existing for loop, followed by this patch.
>
>> 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.
>
Nod. I'll respin these patches.
Jim
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list