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 4928695..a166689 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -834,7 +834,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 9c3daad..9dc887f 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.1.5

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 | 12 ------------ src/util/virportallocator.c | 7 ++++++- tests/virportallocatortest.c | 6 ++---- 5 files changed, 10 insertions(+), 36 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index d4226b8..e4bb75f 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -947,11 +947,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 cb59620..8fb85a8 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 bdffdf8..daf081d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3406,12 +3406,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; } @@ -3437,12 +3431,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")); - virPortAllocatorRelease(driver->remotePorts, port); - 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.1.5

On 11/13/2013 05:39 PM, Daniel P. Berrange wrote: 'port' is released on the error: label. Should I move the double-release removal into a separate commit to make it more obvious? Jan