11:01 a.m.
v1: Properly check the return value of virPortAllocatorAcquire
https://www.redhat.com/archives/libvir-list/2013-October/msg01239.html
v2: Moves the error reporting inside virPortAllocatorAcquire
https://www.redhat.com/archives/libvir-list/2013-October/msg01289.html
v3: Make the names translatable. Split out double-release of port
on error path when allocating SPICE TLS ports.
Ján Tomko (3):
Don't release spice port twice when no TLS port is available
Add a name to virPortAllocator
Return -1 in virPortAllocatorAcquire if all ports are used
src/libxl/libxl_conf.c | 5 -----
src/libxl/libxl_driver.c | 3 ++-
src/qemu/qemu_driver.c | 9 ++++++---
src/qemu/qemu_migration.c | 16 ++--------------
src/qemu/qemu_process.c | 12 ------------
src/util/virportallocator.c | 16 +++++++++++++---
src/util/virportallocator.h | 3 ++-
tests/virportallocatortest.c | 10 ++++------
8 files changed, 29 insertions(+), 45 deletions(-)
--
1.8.3.2
11:01 a.m.
New subject: [libvirt] [PATCHv3 1/3] Don't release spice port twice when no TLS port is available
Introduced by 7b4a630.
---
src/qemu/qemu_process.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e34f542..1365b59 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3431,7 +3431,6 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
if (tlsPort == 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Unable to find an unused port for SPICE
TLS"));
- virPortAllocatorRelease(driver->remotePorts, port);
goto error;
}
graphics->data.spice.tlsPort = tlsPort;
--
1.8.3.2
7:12 a.m.
New subject: [libvirt] [PATCHv3 1/3] Don't release spice port twice when no TLS port is available
On Wed, Nov 13, 2013 at 06:01:40PM +0100, Ján Tomko wrote:
Introduced by 7b4a630.
---
src/qemu/qemu_process.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e34f542..1365b59 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3431,7 +3431,6 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
if (tlsPort == 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Unable to find an unused port for SPICE
TLS"));
- virPortAllocatorRelease(driver->remotePorts, port);
goto error;
}
graphics->data.spice.tlsPort = tlsPort;
--
1.8.3.2
ACK,
Martin
11:01 a.m.
New subject: [libvirt] [PATCHv3 2/3] Add a name to virPortAllocator
This allows its error messages to be more specific.
---
src/libxl/libxl_driver.c | 3 ++-
src/qemu/qemu_driver.c | 9 ++++++---
src/util/virportallocator.c | 9 +++++++--
src/util/virportallocator.h | 3 ++-
tests/virportallocatortest.c | 4 ++--
5 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index f1e500f..7a75a04 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -844,7 +844,8 @@ libxlStateInitialize(bool privileged,
/* Allocate bitmap for vnc port reservation */
if (!(libxl_driver->reservedVNCPorts =
- virPortAllocatorNew(LIBXL_VNC_PORT_MIN,
+ virPortAllocatorNew(_("VNC"),
+ LIBXL_VNC_PORT_MIN,
LIBXL_VNC_PORT_MAX)))
goto error;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ef1359c..378b542 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -678,17 +678,20 @@ qemuStateInitialize(bool privileged,
* do this before the config is loaded properly, since the port
* numbers are configurable now */
if ((qemu_driver->remotePorts =
- virPortAllocatorNew(cfg->remotePortMin,
+ virPortAllocatorNew(_("display"),
+ cfg->remotePortMin,
cfg->remotePortMax)) == NULL)
goto error;
if ((qemu_driver->webSocketPorts =
- virPortAllocatorNew(cfg->webSocketPortMin,
+ virPortAllocatorNew(_("webSocket"),
+ cfg->webSocketPortMin,
cfg->webSocketPortMax)) == NULL)
goto error;
if ((qemu_driver->migrationPorts =
- virPortAllocatorNew(cfg->migrationPortMin,
+ virPortAllocatorNew(_("migration"),
+ cfg->migrationPortMin,
cfg->migrationPortMax)) == NULL)
goto error;
diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c
index 5b7ad41..0497978 100644
--- a/src/util/virportallocator.c
+++ b/src/util/virportallocator.c
@@ -31,6 +31,7 @@
#include "virthread.h"
#include "virerror.h"
#include "virfile.h"
+#include "virstring.h"
#define VIR_FROM_THIS VIR_FROM_NONE
@@ -38,6 +39,8 @@ struct _virPortAllocator {
virObjectLockable parent;
virBitmapPtr bitmap;
+ char *name;
+
unsigned short start;
unsigned short end;
};
@@ -65,7 +68,8 @@ static int virPortAllocatorOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virPortAllocator)
-virPortAllocatorPtr virPortAllocatorNew(unsigned short start,
+virPortAllocatorPtr virPortAllocatorNew(const char *name,
+ unsigned short start,
unsigned short end)
{
virPortAllocatorPtr pa;
@@ -85,7 +89,8 @@ virPortAllocatorPtr virPortAllocatorNew(unsigned short start,
pa->start = start;
pa->end = end;
- if (!(pa->bitmap = virBitmapNew((end-start)+1))) {
+ if (!(pa->bitmap = virBitmapNew((end-start)+1)) ||
+ VIR_STRDUP(pa->name, name) < 0) {
virObjectUnref(pa);
return NULL;
}
diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h
index a5e68f7..c8aa6de 100644
--- a/src/util/virportallocator.h
+++ b/src/util/virportallocator.h
@@ -28,7 +28,8 @@
typedef struct _virPortAllocator virPortAllocator;
typedef virPortAllocator *virPortAllocatorPtr;
-virPortAllocatorPtr virPortAllocatorNew(unsigned short start,
+virPortAllocatorPtr virPortAllocatorNew(const char *name,
+ unsigned short start,
unsigned short end);
int virPortAllocatorAcquire(virPortAllocatorPtr pa,
diff --git a/tests/virportallocatortest.c b/tests/virportallocatortest.c
index 4d0518a..33de782 100644
--- a/tests/virportallocatortest.c
+++ b/tests/virportallocatortest.c
@@ -63,7 +63,7 @@ int bind(int sockfd ATTRIBUTE_UNUSED,
static int testAllocAll(const void *args ATTRIBUTE_UNUSED)
{
- virPortAllocatorPtr alloc = virPortAllocatorNew(5900, 5909);
+ virPortAllocatorPtr alloc = virPortAllocatorNew("test", 5900, 5909);
int ret = -1;
unsigned short p1, p2, p3, p4, p5, p6, p7;
@@ -136,7 +136,7 @@ cleanup:
static int testAllocReuse(const void *args ATTRIBUTE_UNUSED)
{
- virPortAllocatorPtr alloc = virPortAllocatorNew(5900, 5910);
+ virPortAllocatorPtr alloc = virPortAllocatorNew("test", 5900, 5910);
int ret = -1;
unsigned short p1, p2, p3, p4;
--
1.8.3.2
7:13 a.m.
New subject: [libvirt] [PATCHv3 2/3] Add a name to virPortAllocator
On Wed, Nov 13, 2013 at 06:01:41PM +0100, Ján Tomko wrote:
This allows its error messages to be more specific.
---
src/libxl/libxl_driver.c | 3 ++-
src/qemu/qemu_driver.c | 9 ++++++---
src/util/virportallocator.c | 9 +++++++--
src/util/virportallocator.h | 3 ++-
tests/virportallocatortest.c | 4 ++--
5 files changed, 19 insertions(+), 9 deletions(-)
You leak the *name. ACK if you add VIR_FREE(pa->name) into
virPortAllocatorDispose().
Martin
5:39 a.m.
New subject: [libvirt] [PATCHv3 2/3] Add a name to virPortAllocator
On 11/14/2013 02:13 PM, Martin Kletzander wrote:
On Wed, Nov 13, 2013 at 06:01:41PM +0100, Ján Tomko wrote:
> This allows its error messages to be more specific.
> ---
> src/libxl/libxl_driver.c | 3 ++-
> src/qemu/qemu_driver.c | 9 ++++++---
> src/util/virportallocator.c | 9 +++++++--
> src/util/virportallocator.h | 3 ++-
> tests/virportallocatortest.c | 4 ++--
> 5 files changed, 19 insertions(+), 9 deletions(-)
>
You leak the *name. ACK if you add VIR_FREE(pa->name) into
virPortAllocatorDispose().
Martin
I have fixed the leak and pushed the series, thank you for the reviews!
Jan
11:01 a.m.
New subject: [libvirt] [PATCHv3 3/3] Return -1 in virPortAllocatorAcquire if all ports are used
Report the error in virPortAllocatorAcquire instead
of doing it in every caller.
The error contains the port range name instead of the intended
use for the port, e.g.:
Unable to find an unused port in range 'display' (65534-65535)
instead of:
Unable to find an unused port for SPICE
This also adds error reporting when the QEMU driver could not
find an unused port for VNC, VNC WebSockets or NBD migration.
---
src/libxl/libxl_conf.c | 5 -----
src/qemu/qemu_migration.c | 16 ++--------------
src/qemu/qemu_process.c | 11 -----------
src/util/virportallocator.c | 7 ++++++-
tests/virportallocatortest.c | 6 ++----
5 files changed, 10 insertions(+), 35 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 79cf2b6..aaeb00e 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -950,11 +950,6 @@ libxlMakeVfb(libxlDriverPrivatePtr driver,
if (virPortAllocatorAcquire(driver->reservedVNCPorts, &port) <
0)
return -1;
- if (port == 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("Unable to find an unused VNC
port"));
- return -1;
- }
l_vfb->data.vnc.port = port;
}
x_vfb->vnc.display = l_vfb->data.vnc.port - LIBXL_VNC_PORT_MIN;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index a3d986f..9d9f2b6 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2557,14 +2557,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
* to be a correct hostname which refers to the target machine).
*/
if (uri_in == NULL) {
- if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) {
+ if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0)
goto cleanup;
- } else if (!port) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("No migration port available within the "
- "configured range"));
- goto cleanup;
- }
if ((hostname = virGetHostname()) == NULL)
goto cleanup;
@@ -2619,14 +2613,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
}
if (uri->port == 0) {
- if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) {
+ if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0)
goto cleanup;
- } else if (!port) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("No migration port available within the "
- "configured range"));
- goto cleanup;
- }
if (well_formed_uri) {
uri->port = port;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1365b59..c73ca13 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3397,12 +3397,6 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0)
goto error;
- if (port == 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to find an unused port for SPICE"));
- goto error;
- }
-
graphics->data.spice.port = port;
}
@@ -3428,11 +3422,6 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
if (virPortAllocatorAcquire(driver->remotePorts, &tlsPort) < 0)
goto error;
- if (tlsPort == 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to find an unused port for SPICE
TLS"));
- goto error;
- }
graphics->data.spice.tlsPort = tlsPort;
}
}
diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c
index 0497978..42e1f28 100644
--- a/src/util/virportallocator.c
+++ b/src/util/virportallocator.c
@@ -156,10 +156,15 @@ int virPortAllocatorAcquire(virPortAllocatorPtr pa,
goto cleanup;
}
*port = i;
+ ret = 0;
}
}
- ret = 0;
+ if (*port == 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to find an unused port in range '%s'
(%d-%d)"),
+ pa->name, pa->start, pa->end);
+ }
cleanup:
virObjectUnlock(pa);
VIR_FORCE_CLOSE(fd);
diff --git a/tests/virportallocatortest.c b/tests/virportallocatortest.c
index 33de782..721356e 100644
--- a/tests/virportallocatortest.c
+++ b/tests/virportallocatortest.c
@@ -118,11 +118,9 @@ static int testAllocAll(const void *args ATTRIBUTE_UNUSED)
goto cleanup;
}
- if (virPortAllocatorAcquire(alloc, &p7) < 0)
- goto cleanup;
- if (p7 != 0) {
+ if (virPortAllocatorAcquire(alloc, &p7) == 0) {
if (virTestGetDebug())
- fprintf(stderr, "Expected 0, got %d", p7);
+ fprintf(stderr, "Expected error, got %d", p7);
goto cleanup;
}
--
1.8.3.2
9:22 a.m.
New subject: [libvirt] [PATCHv3 3/3] Return -1 in virPortAllocatorAcquire if all ports are used
On Wed, Nov 13, 2013 at 06:01:42PM +0100, Ján Tomko wrote:
Report the error in virPortAllocatorAcquire instead
of doing it in every caller.
The error contains the port range name instead of the intended
use for the port, e.g.:
Unable to find an unused port in range 'display' (65534-65535)
instead of:
Unable to find an unused port for SPICE
This also adds error reporting when the QEMU driver could not
find an unused port for VNC, VNC WebSockets or NBD migration.
---
src/libxl/libxl_conf.c | 5 -----
src/qemu/qemu_migration.c | 16 ++--------------
src/qemu/qemu_process.c | 11 -----------
src/util/virportallocator.c | 7 ++++++-
tests/virportallocatortest.c | 6 ++----
5 files changed, 10 insertions(+), 35 deletions(-)
ACK,
Martin
4053
days inactive
4058
days old
7 comments
2 participants
participants (2)
-
Ján Tomko -
Martin Kletzander