[libvirt] [PATCH v2 0/2] 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 Ján Tomko (2): 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.1.5
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
On Thu, Oct 31, 2013 at 01:07:26PM +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(-)
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;
Probably need to mark these for translation. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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 Thu, Oct 31, 2013 at 01:07:27PM +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 | 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; - }
Opps, you're loosing the release of 'port' when tlsPort allocation fails. In fact there was already a broken codepath in this respect. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/13/2013 05:39 PM, Daniel P. Berrange wrote:
On Thu, Oct 31, 2013 at 01:07:27PM +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 | 12 ------------ src/util/virportallocator.c | 7 ++++++- tests/virportallocatortest.c | 6 ++---- 5 files changed, 10 insertions(+), 36 deletions(-)
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; - }
Opps, you're loosing the release of 'port' when tlsPort allocation fails. In fact there was already a broken codepath in this respect.
'port' is released on the error: label. Should I move the double-release removal into a separate commit to make it more obvious? Jan
On Wed, Nov 13, 2013 at 05:48:28PM +0100, Ján Tomko wrote:
On 11/13/2013 05:39 PM, Daniel P. Berrange wrote:
On Thu, Oct 31, 2013 at 01:07:27PM +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 | 12 ------------ src/util/virportallocator.c | 7 ++++++- tests/virportallocatortest.c | 6 ++---- 5 files changed, 10 insertions(+), 36 deletions(-)
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; - }
Opps, you're loosing the release of 'port' when tlsPort allocation fails. In fact there was already a broken codepath in this respect.
'port' is released on the error: label. Should I move the double-release removal into a separate commit to make it more obvious?
Sure, sounds good. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/31/2013 01:07 PM, Ján Tomko wrote:
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
Ján Tomko (2): 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(-)
Ping?
participants (2)
-
Daniel P. Berrange -
Ján Tomko