1:15 p.m.
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 | 79 +++++++++++++++++++++++++++++++++++++++------
src/util/virportallocator.c | 40 ++++++++++++++++++++++-
src/util/virportallocator.h | 4 +++
5 files changed, 117 insertions(+), 10 deletions(-)
--
1.9.3
1:15 p.m.
New subject: [libvirt] [PATCH 1/2] virtportallocator: new function "virPortAllocatorSetUsed"
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(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virportallocator.c | 40 +++++++++++++++++++++++++++++++++++++++-
src/util/virportallocator.h | 4 ++++
3 files changed, 44 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..e98fc8a 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,41 @@ int virPortAllocatorRelease(virPortAllocatorPtr pa,
virObjectUnlock(pa);
return ret;
}
+
+int virPortAllocatorSetUsed(virPortAllocatorPtr pa,
+ unsigned short port,
+ bool used)
+{
+ int ret = -1;
+
+ virObjectLock(pa);
+
+
+ if (port < pa->start ||
+ port > pa->end) {
+ ret = 0;
+ goto cleanup;
+ }
+
+ if (used) {
+ if (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
1:15 p.m.
New subject: [libvirt] [PATCH 2/2] graphics: remember graphics not auto allocated ports
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]ortAllocated variables keep track of the ports that were
successfully registered (either bound or simply tracked as used) by
the port allocator to allow a clean rollback on errors.
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1081881
Signed-off-by: Giuseppe Scrivano <gscrivan(a)redhat.com>
---
src/conf/domain_conf.h | 3 ++
src/qemu/qemu_process.c | 79 +++++++++++++++++++++++++++++++++++++++++++------
2 files changed, 73 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 6779a41..7617574 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1385,6 +1385,7 @@ struct _virDomainGraphicsDef {
union {
struct {
int port;
+ bool portAllocated;
int websocket;
bool autoport;
char *keymap;
@@ -1410,6 +1411,8 @@ struct _virDomainGraphicsDef {
struct {
int port;
int tlsPort;
+ bool portAllocated;
+ bool tlsPortAllocated;
int mousemode;
char *keymap;
virDomainGraphicsAuthDef auth;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 0b8155b..06f1e54 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3487,6 +3487,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0)
return -1;
graphics->data.vnc.port = port;
+ graphics->data.vnc.portAllocated = true;
}
if (graphics->data.vnc.websocket == -1) {
@@ -3548,6 +3549,7 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
goto error;
graphics->data.spice.port = port;
+ graphics->data.spice.portAllocated = true;
}
if (needTLSPort || graphics->data.spice.tlsPort == -1) {
@@ -3573,6 +3575,7 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
goto error;
graphics->data.spice.tlsPort = tlsPort;
+ graphics->data.spice.tlsPortAllocated = true;
}
}
@@ -3803,6 +3806,36 @@ 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.portAllocated = true;
+
+ } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
+ !graphics->data.spice.autoport) {
+ if (virPortAllocatorSetUsed(driver->remotePorts,
+ graphics->data.spice.port,
+ true) < 0)
+ goto cleanup;
+
+ graphics->data.spice.portAllocated = true;
+
+ if (virPortAllocatorSetUsed(driver->remotePorts,
+ graphics->data.spice.tlsPort,
+ true) < 0)
+ goto cleanup;
+
+ graphics->data.spice.tlsPortAllocated = 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;
@@ -4509,19 +4542,47 @@ void qemuProcessStop(virQEMUDriverPtr driver,
for (i = 0; i < vm->def->ngraphics; ++i) {
virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
- if (graphics->data.vnc.autoport) {
- virPortAllocatorRelease(driver->remotePorts,
- graphics->data.vnc.port);
+ if (graphics->data.vnc.portAllocated) {
+ if (graphics->data.vnc.autoport) {
+ virPortAllocatorRelease(driver->remotePorts,
+ graphics->data.vnc.port);
+ } else {
+ virPortAllocatorSetUsed(driver->remotePorts,
+ graphics->data.vnc.port,
+ false);
+ }
+ graphics->data.vnc.portAllocated = 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) {
+ if (graphics->data.spice.portAllocated) {
+ virPortAllocatorRelease(driver->remotePorts,
+ graphics->data.spice.port);
+ graphics->data.spice.portAllocated = false;
+ }
+ if (graphics->data.spice.tlsPortAllocated) {
+ virPortAllocatorRelease(driver->remotePorts,
+ graphics->data.spice.tlsPort);
+ graphics->data.spice.tlsPortAllocated = false;
+ }
+ } else {
+ if (graphics->data.spice.portAllocated) {
+ virPortAllocatorSetUsed(driver->remotePorts,
+ graphics->data.spice.port,
+ false);
+ graphics->data.spice.portAllocated = false;
+ }
+ if (graphics->data.spice.tlsPortAllocated) {
+ virPortAllocatorSetUsed(driver->remotePorts,
+ graphics->data.spice.tlsPort,
+ false);
+ graphics->data.spice.tlsPortAllocated = false;
+ }
+ }
}
}
--
1.9.3
2:51 a.m.
New subject: [libvirt] [PATCH 2/2] graphics: remember graphics not auto allocated ports
On 06/23/2014 08:15 PM, 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]ortAllocated variables keep track of the ports that were
successfully registered (either bound or simply tracked as used) by
the port allocator to allow a clean rollback on errors.
Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1081881
Signed-off-by: Giuseppe Scrivano <gscrivan(a)redhat.com>
---
src/conf/domain_conf.h | 3 ++
src/qemu/qemu_process.c | 79 +++++++++++++++++++++++++++++++++++++++++++------
2 files changed, 73 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 0b8155b..06f1e54 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3487,6 +3487,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0)
return -1;
graphics->data.vnc.port = port;
+ graphics->data.vnc.portAllocated = true;
}
if (graphics->data.vnc.websocket == -1) {
@@ -3548,6 +3549,7 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
goto error;
graphics->data.spice.port = port;
+ graphics->data.spice.portAllocated = true;
}
if (needTLSPort || graphics->data.spice.tlsPort == -1) {
@@ -3573,6 +3575,7 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
goto error;
graphics->data.spice.tlsPort = tlsPort;
+ graphics->data.spice.tlsPortAllocated = true;
}
}
@@ -3803,6 +3806,36 @@ 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) {
virPortAllocatorSetUsed doesn't error out if the static port is already marked
as used. If it's already used by another domain, this one fails to start and
releases the port in qemuProcessStop.
If you change it to fail on occupied ports, it will also catch duplicit static
ports, but only in the port allocator range. I expect someone will file a bug
about conflicting ports out of that range as well :)
+ goto cleanup;
+ }
+
+ graphics->data.vnc.portAllocated = true;
+
+ } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
+ !graphics->data.spice.autoport) {
If either of the ports is -1, you shouldn't be reserving them. (For VNC, we
set autoport to true if port is -1 when parsing the XML. For SPICE, this only
happens if both ports are -1).
+ if (virPortAllocatorSetUsed(driver->remotePorts,
+ graphics->data.spice.port,
+ true) < 0)
+ goto cleanup;
+
+ graphics->data.spice.portAllocated = true;
+
+ if (virPortAllocatorSetUsed(driver->remotePorts,
+ graphics->data.spice.tlsPort,
+ true) < 0)
+ goto cleanup;
+
+ graphics->data.spice.tlsPortAllocated = 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;
@@ -4509,19 +4542,47 @@ void qemuProcessStop(virQEMUDriverPtr driver,
for (i = 0; i < vm->def->ngraphics; ++i) {
virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
- if (graphics->data.vnc.autoport) {
- virPortAllocatorRelease(driver->remotePorts,
- graphics->data.vnc.port);
+ if (graphics->data.vnc.portAllocated) {
+ if (graphics->data.vnc.autoport) {
+ virPortAllocatorRelease(driver->remotePorts,
+ graphics->data.vnc.port);
+ } else {
+ virPortAllocatorSetUsed(driver->remotePorts,
+ graphics->data.vnc.port,
+ false);
+ }
+ graphics->data.vnc.portAllocated = 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) {
We allocate a port for SPICE even if autoport is no, but either port or
tlsPort is -1. This would fix the bug of not releasing the port.
+ if (graphics->data.spice.portAllocated) {
+ virPortAllocatorRelease(driver->remotePorts,
+ graphics->data.spice.port);
+ graphics->data.spice.portAllocated = false;
+ }
+ if (graphics->data.spice.tlsPortAllocated) {
+ virPortAllocatorRelease(driver->remotePorts,
+ graphics->data.spice.tlsPort);
+ graphics->data.spice.tlsPortAllocated = false;
+ }
If we could make virPortAllocatorRelease silently ignore ports out of range,
the else branch would not be necessary and so would the bool parameter of
'SetUsed'.
+ } else {
+ if (graphics->data.spice.portAllocated) {
+ virPortAllocatorSetUsed(driver->remotePorts,
+ graphics->data.spice.port,
+ false);
+ graphics->data.spice.portAllocated = false;
+ }
+ if (graphics->data.spice.tlsPortAllocated) {
+ virPortAllocatorSetUsed(driver->remotePorts,
+ graphics->data.spice.tlsPort,
+ false);
+ graphics->data.spice.tlsPortAllocated = false;
+ }
+ }
}
}
Jan
3:57 a.m.
New subject: [libvirt] [PATCH 2/2] graphics: remember graphics not auto allocated ports
Ján Tomko <jtomko(a)redhat.com> writes:
On 06/23/2014 08:15 PM, 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]ortAllocated variables keep track of the ports that were
> successfully registered (either bound or simply tracked as used) by
> the port allocator to allow a clean rollback on errors.
>
> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1081881
>
> Signed-off-by: Giuseppe Scrivano <gscrivan(a)redhat.com>
> ---
> src/conf/domain_conf.h | 3 ++
> src/qemu/qemu_process.c | 79 +++++++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 73 insertions(+), 9 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 0b8155b..06f1e54 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3487,6 +3487,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
> if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0)
> return -1;
> graphics->data.vnc.port = port;
> + graphics->data.vnc.portAllocated = true;
> }
>
> if (graphics->data.vnc.websocket == -1) {
> @@ -3548,6 +3549,7 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
> goto error;
>
> graphics->data.spice.port = port;
> + graphics->data.spice.portAllocated = true;
> }
>
> if (needTLSPort || graphics->data.spice.tlsPort == -1) {
> @@ -3573,6 +3575,7 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
> goto error;
>
> graphics->data.spice.tlsPort = tlsPort;
> + graphics->data.spice.tlsPortAllocated = true;
> }
> }
>
> @@ -3803,6 +3806,36 @@ 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) {
virPortAllocatorSetUsed doesn't error out if the static port is already marked
as used. If it's already used by another domain, this one fails to start and
releases the port in qemuProcessStop.
If you change it to fail on occupied ports, it will also catch duplicit static
ports, but only in the port allocator range. I expect someone will file a bug
about conflicting ports out of that range as well :)
sorry, it had to be failing on occupied ports, I'll change it in v2.
> + goto cleanup;
> + }
> +
> + graphics->data.vnc.portAllocated = true;
> +
> + } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
> + !graphics->data.spice.autoport) {
If either of the ports is -1, you shouldn't be reserving them. (For VNC, we
set autoport to true if port is -1 when parsing the XML. For SPICE, this only
happens if both ports are -1).
I will modify it to reserve a port only if !autoport && port != -1 and
fix the release part too.
Thanks,
Giuseppe
3804
days inactive
3805
days old
4 comments
2 participants
participants (2)
-
Giuseppe Scrivano -
Ján Tomko