[libvirt] [PATCHv3 0/3] Report an error in virPortAllocatorAcquire if all ports are used
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
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
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
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
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
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
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
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
participants (2)
-
Ján Tomko -
Martin Kletzander