[libvirt] [PATCH v2 0/2] virtportallocator: remember not auto allocated graphics ports
Changes in v2: * virPortAllocatorSetUsed returns an error if the port is already used. * Changed *[pP]ortAllocated to *[pP]ortReserved that keep track only of ports that were marked used but not bound. * Handle separately port and tlsPort for SPICE. Fix a conflict when both autoport graphics and statically configured ports are used, like: <graphics type='spice' autoport='yes'/> <graphics type='vnc' port='5900' autoport='no'/> More details in 2/2 Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1081881 Giuseppe Scrivano (2): virtportallocator: new function "virPortAllocatorSetUsed" graphics: remember graphics not auto allocated ports src/conf/domain_conf.h | 3 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 68 +++++++++++++++++++++++++++++++++++++++++---- src/util/virportallocator.c | 44 ++++++++++++++++++++++++++++- src/util/virportallocator.h | 4 +++ 5 files changed, 113 insertions(+), 7 deletions(-) -- 1.9.3
virPortAllocatorSetUsed permits to set a port as already used and prevent the port allocator to use it without any attempt to bind it. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virportallocator.c | 44 +++++++++++++++++++++++++++++++++++++++++++- src/util/virportallocator.h | 4 ++++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2a2b9c0..95a45aa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1770,6 +1770,7 @@ virPidFileWritePath; virPortAllocatorAcquire; virPortAllocatorNew; virPortAllocatorRelease; +virPortAllocatorSetUsed; # util/virprocess.h diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index b68133a..226f386 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -1,7 +1,7 @@ /* * virportallocator.c: Allocate & track TCP port allocations * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013, 2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -250,3 +250,45 @@ int virPortAllocatorRelease(virPortAllocatorPtr pa, virObjectUnlock(pa); return ret; } + +int virPortAllocatorSetUsed(virPortAllocatorPtr pa, + unsigned short port, + bool value) +{ + int ret = -1; + + virObjectLock(pa); + + if (port < pa->start || + port > pa->end) { + ret = 0; + goto cleanup; + } + + if (value) { + bool used = false; + if (virBitmapGetBit(pa->bitmap, port - pa->start, &used) < 0) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to query port %d"), port); + + if (used || virBitmapSetBit(pa->bitmap, port - pa->start) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to reserve port %d"), port); + goto cleanup; + } + } + else { + if (virBitmapClearBit(pa->bitmap, + port - pa->start) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to release port %d"), + port); + goto cleanup; + } + } + + ret = 0; + cleanup: + virObjectUnlock(pa); + return ret; +} diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h index c8aa6de..e5ee56d 100644 --- a/src/util/virportallocator.h +++ b/src/util/virportallocator.h @@ -38,4 +38,8 @@ int virPortAllocatorAcquire(virPortAllocatorPtr pa, int virPortAllocatorRelease(virPortAllocatorPtr pa, unsigned short port); +int virPortAllocatorSetUsed(virPortAllocatorPtr pa, + unsigned short port, + bool value); + #endif /* __VIR_PORT_ALLOCATOR_H__ */ -- 1.9.3
On 24.06.2014 13:34, Giuseppe Scrivano wrote:
virPortAllocatorSetUsed permits to set a port as already used and prevent the port allocator to use it without any attempt to bind it.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virportallocator.c | 44 +++++++++++++++++++++++++++++++++++++++++++- src/util/virportallocator.h | 4 ++++ 3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2a2b9c0..95a45aa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1770,6 +1770,7 @@ virPidFileWritePath; virPortAllocatorAcquire; virPortAllocatorNew; virPortAllocatorRelease; +virPortAllocatorSetUsed;
# util/virprocess.h diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index b68133a..226f386 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -1,7 +1,7 @@ /* * virportallocator.c: Allocate & track TCP port allocations * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013, 2014 Red Hat, Inc.
How about 2013-2014?
* * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -250,3 +250,45 @@ int virPortAllocatorRelease(virPortAllocatorPtr pa, virObjectUnlock(pa); return ret; } + +int virPortAllocatorSetUsed(virPortAllocatorPtr pa, + unsigned short port, + bool value) +{ + int ret = -1; + + virObjectLock(pa); + + if (port < pa->start || + port > pa->end) { + ret = 0; + goto cleanup; + } + + if (value) { + bool used = false; + if (virBitmapGetBit(pa->bitmap, port - pa->start, &used) < 0) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to query port %d"), port); + + if (used || virBitmapSetBit(pa->bitmap, port - pa->start) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to reserve port %d"), port); + goto cleanup; + } + } + else {
move the 'else' at the same line with the closing bracket
+ if (virBitmapClearBit(pa->bitmap, + port - pa->start) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to release port %d"), + port); + goto cleanup; + } + } + + ret = 0; + cleanup: + virObjectUnlock(pa); + return ret; +} diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h index c8aa6de..e5ee56d 100644 --- a/src/util/virportallocator.h +++ b/src/util/virportallocator.h @@ -38,4 +38,8 @@ int virPortAllocatorAcquire(virPortAllocatorPtr pa, int virPortAllocatorRelease(virPortAllocatorPtr pa, unsigned short port);
+int virPortAllocatorSetUsed(virPortAllocatorPtr pa, + unsigned short port, + bool value); + #endif /* __VIR_PORT_ALLOCATOR_H__ */
ACK Michal
Michal Privoznik <mprivozn@redhat.com> writes:
On 24.06.2014 13:34, Giuseppe Scrivano wrote:
virPortAllocatorSetUsed permits to set a port as already used and prevent the port allocator to use it without any attempt to bind it.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virportallocator.c | 44 +++++++++++++++++++++++++++++++++++++++++++- src/util/virportallocator.h | 4 ++++ 3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2a2b9c0..95a45aa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1770,6 +1770,7 @@ virPidFileWritePath; virPortAllocatorAcquire; virPortAllocatorNew; virPortAllocatorRelease; +virPortAllocatorSetUsed;
# util/virprocess.h diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index b68133a..226f386 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -1,7 +1,7 @@ /* * virportallocator.c: Allocate & track TCP port allocations * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013, 2014 Red Hat, Inc.
How about 2013-2014?
+ else {
I've no write access to the repo. Could these fixes be amended before pushing the patches or shall I go for v3? Thanks, Giuseppe
On 24.06.2014 14:04, Giuseppe Scrivano wrote:
Michal Privoznik <mprivozn@redhat.com> writes:
On 24.06.2014 13:34, Giuseppe Scrivano wrote:
virPortAllocatorSetUsed permits to set a port as already used and prevent the port allocator to use it without any attempt to bind it.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virportallocator.c | 44 +++++++++++++++++++++++++++++++++++++++++++- src/util/virportallocator.h | 4 ++++ 3 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2a2b9c0..95a45aa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1770,6 +1770,7 @@ virPidFileWritePath; virPortAllocatorAcquire; virPortAllocatorNew; virPortAllocatorRelease; +virPortAllocatorSetUsed;
# util/virprocess.h diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index b68133a..226f386 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -1,7 +1,7 @@ /* * virportallocator.c: Allocate & track TCP port allocations * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013, 2014 Red Hat, Inc.
How about 2013-2014?
+ else {
I've no write access to the repo. Could these fixes be amended before pushing the patches or shall I go for v3?
Thanks, Giuseppe
No, I've ammended the changes and pushed. Cheers. Michal
When looking for a port to allocate, the port allocator didn't take in consideration ports that are statically set by the user. Defining these two graphics elements in the XML would cause an error, as the port allocator would try to use the same port for the spice graphics element: <graphics type='spice' autoport='yes'/> <graphics type='vnc' port='5900' autoport='no'/> The new *[pP]ortReserved variables keep track of the ports that were successfully tracked as used by the port allocator but that weren't bound. Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1081881 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/conf/domain_conf.h | 3 +++ src/qemu/qemu_process.c | 68 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6779a41..1122eb2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1385,6 +1385,7 @@ struct _virDomainGraphicsDef { union { struct { int port; + bool portReserved; int websocket; bool autoport; char *keymap; @@ -1410,6 +1411,8 @@ struct _virDomainGraphicsDef { struct { int port; int tlsPort; + bool portReserved; + bool tlsPortReserved; int mousemode; char *keymap; virDomainGraphicsAuthDef auth; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0b8155b..561e37e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3803,6 +3803,41 @@ int qemuProcessStart(virConnectPtr conn, for (i = 0; i < vm->def->ngraphics; ++i) { virDomainGraphicsDefPtr graphics = vm->def->graphics[i]; + if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && + !graphics->data.vnc.autoport) { + if (virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.vnc.port, + true) < 0) { + goto cleanup; + } + + graphics->data.vnc.portReserved = true; + + } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE && + !graphics->data.spice.autoport) { + + if (graphics->data.spice.port > 0) { + if (virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.spice.port, + true) < 0) + goto cleanup; + + graphics->data.spice.portReserved = true; + } + + if (graphics->data.spice.tlsPort > 0) { + if (virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.spice.tlsPort, + true) < 0) + goto cleanup; + + graphics->data.spice.tlsPortReserved = true; + } + } + } + + for (i = 0; i < vm->def->ngraphics; ++i) { + virDomainGraphicsDefPtr graphics = vm->def->graphics[i]; if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { if (qemuProcessVNCAllocatePorts(driver, graphics) < 0) goto cleanup; @@ -4513,15 +4548,36 @@ void qemuProcessStop(virQEMUDriverPtr driver, virPortAllocatorRelease(driver->remotePorts, graphics->data.vnc.port); } + else if (graphics->data.vnc.portReserved) { + virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.spice.port, + false); + graphics->data.vnc.portReserved = false; + } virPortAllocatorRelease(driver->webSocketPorts, graphics->data.vnc.websocket); } - if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE && - graphics->data.spice.autoport) { - virPortAllocatorRelease(driver->remotePorts, - graphics->data.spice.port); - virPortAllocatorRelease(driver->remotePorts, - graphics->data.spice.tlsPort); + if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { + if (graphics->data.spice.autoport) { + virPortAllocatorRelease(driver->remotePorts, + graphics->data.spice.port); + virPortAllocatorRelease(driver->remotePorts, + graphics->data.spice.tlsPort); + } else { + if (graphics->data.spice.portReserved) { + virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.spice.port, + false); + graphics->data.spice.portReserved = false; + } + + if (graphics->data.spice.tlsPortReserved) { + virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.spice.tlsPort, + false); + graphics->data.spice.tlsPortReserved = false; + } + } } } -- 1.9.3
On 24.06.2014 13:34, Giuseppe Scrivano wrote:
When looking for a port to allocate, the port allocator didn't take in consideration ports that are statically set by the user. Defining these two graphics elements in the XML would cause an error, as the port allocator would try to use the same port for the spice graphics element:
<graphics type='spice' autoport='yes'/> <graphics type='vnc' port='5900' autoport='no'/>
The new *[pP]ortReserved variables keep track of the ports that were successfully tracked as used by the port allocator but that weren't bound.
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1081881
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/conf/domain_conf.h | 3 +++ src/qemu/qemu_process.c | 68 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 65 insertions(+), 6 deletions(-)
ACK Michal
participants (2)
-
Giuseppe Scrivano -
Michal Privoznik