Host tcp4/tcp6 ports is a global resource thus we need to make port accounting also global or we have issues described in [1] when port allocator ranges of different instances are overlapped (which is by default for qemu for example). Let's have only one global port allocator object that take care of the entire ports range (0 - 65535) and introduce port range object for clients to specify desired auto allocation band. [1] https://www.redhat.com/archives/libvir-list/2017-December/msg00600.html --- src/bhyve/bhyve_driver.c | 4 +- src/bhyve/bhyve_utils.h | 2 +- src/libvirt_private.syms | 3 +- src/libxl/libxl_conf.c | 8 +-- src/libxl/libxl_conf.h | 8 +-- src/libxl/libxl_driver.c | 18 +++--- src/qemu/qemu_conf.h | 6 +- src/qemu/qemu_driver.c | 30 +++++----- src/util/virportallocator.c | 125 ++++++++++++++++++++++++++++------------- src/util/virportallocator.h | 20 ++++--- tests/bhyvexml2argvtest.c | 6 +- tests/libxlxml2domconfigtest.c | 8 +-- tests/virportallocatortest.c | 48 ++++++++++------ 13 files changed, 175 insertions(+), 111 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index dd6e8ab..43487f5 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1220,7 +1220,7 @@ bhyveStateCleanup(void) virObjectUnref(bhyve_driver->closeCallbacks); virObjectUnref(bhyve_driver->domainEventState); virObjectUnref(bhyve_driver->config); - virObjectUnref(bhyve_driver->remotePorts); + virPortRangeFree(bhyve_driver->remotePorts); virMutexDestroy(&bhyve_driver->lock); VIR_FREE(bhyve_driver); @@ -1267,7 +1267,7 @@ bhyveStateInitialize(bool privileged, if (!(bhyve_driver->domainEventState = virObjectEventStateNew())) goto cleanup; - if (!(bhyve_driver->remotePorts = virPortAllocatorNew(_("display"), 5900, 65535, 0))) + if (!(bhyve_driver->remotePorts = virPortRangeNew(_("display"), 5900, 65535, 0))) goto cleanup; bhyve_driver->hostsysinfo = virSysinfoRead(); diff --git a/src/bhyve/bhyve_utils.h b/src/bhyve/bhyve_utils.h index 8ad2698..9ab590b 100644 --- a/src/bhyve/bhyve_utils.h +++ b/src/bhyve/bhyve_utils.h @@ -59,7 +59,7 @@ struct _bhyveConn { virCloseCallbacksPtr closeCallbacks; - virPortAllocatorPtr remotePorts; + virPortRangePtr remotePorts; unsigned bhyvecaps; unsigned grubcaps; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d5c3b9a..c668ded 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2496,9 +2496,10 @@ virPolkitCheckAuth; # util/virportallocator.h virPortAllocatorAcquire; -virPortAllocatorNew; virPortAllocatorRelease; virPortAllocatorSetUsed; +virPortRangeFree; +virPortRangeNew; # util/virprocess.h diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 970cff2..8e606b2 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1287,7 +1287,7 @@ libxlMakeNicList(virDomainDefPtr def, libxl_domain_config *d_config) } int -libxlMakeVfb(virPortAllocatorPtr graphicsports, +libxlMakeVfb(virPortRangePtr graphicsports, virDomainGraphicsDefPtr l_vfb, libxl_device_vfb *x_vfb) { @@ -1348,7 +1348,7 @@ libxlMakeVfb(virPortAllocatorPtr graphicsports, } static int -libxlMakeVfbList(virPortAllocatorPtr graphicsports, +libxlMakeVfbList(virPortRangePtr graphicsports, virDomainDefPtr def, libxl_domain_config *d_config) { @@ -1397,7 +1397,7 @@ libxlMakeVfbList(virPortAllocatorPtr graphicsports, * populate libxl_domain_config->vfbs. */ static int -libxlMakeBuildInfoVfb(virPortAllocatorPtr graphicsports, +libxlMakeBuildInfoVfb(virPortRangePtr graphicsports, virDomainDefPtr def, libxl_domain_config *d_config) { @@ -2284,7 +2284,7 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver, virNodeInfoPtr info) } int -libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, +libxlBuildDomainConfig(virPortRangePtr graphicsports, virDomainDefPtr def, const char *channelDir LIBXL_ATTR_UNUSED, libxl_ctx *ctx, diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 264df11..3ba8710 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -131,10 +131,10 @@ struct _libxlDriverPrivate { virObjectEventStatePtr domainEventState; /* Immutable pointer, self-locking APIs */ - virPortAllocatorPtr reservedGraphicsPorts; + virPortRangePtr reservedGraphicsPorts; /* Immutable pointer, self-locking APIs */ - virPortAllocatorPtr migrationPorts; + virPortRangePtr migrationPorts; /* Immutable pointer, lockless APIs*/ virSysinfoDefPtr hostsysinfo; @@ -189,7 +189,7 @@ libxlMakeNic(virDomainDefPtr def, libxl_device_nic *x_nic, bool attach); int -libxlMakeVfb(virPortAllocatorPtr graphicsports, +libxlMakeVfb(virPortRangePtr graphicsports, virDomainGraphicsDefPtr l_vfb, libxl_device_vfb *x_vfb); int @@ -213,7 +213,7 @@ libxlCreateXMLConf(void); # define LIBXL_ATTR_UNUSED ATTRIBUTE_UNUSED # endif int -libxlBuildDomainConfig(virPortAllocatorPtr graphicsports, +libxlBuildDomainConfig(virPortRangePtr graphicsports, virDomainDefPtr def, const char *channelDir LIBXL_ATTR_UNUSED, libxl_ctx *ctx, diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 79e29ce..5209ea7 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -477,8 +477,8 @@ libxlStateCleanup(void) virObjectUnref(libxl_driver->config); virObjectUnref(libxl_driver->xmlopt); virObjectUnref(libxl_driver->domains); - virObjectUnref(libxl_driver->reservedGraphicsPorts); - virObjectUnref(libxl_driver->migrationPorts); + virPortRangeFree(libxl_driver->reservedGraphicsPorts); + virPortRangeFree(libxl_driver->migrationPorts); virLockManagerPluginUnref(libxl_driver->lockManager); virObjectUnref(libxl_driver->domainEventState); @@ -657,17 +657,17 @@ libxlStateInitialize(bool privileged, /* Allocate bitmap for vnc port reservation */ if (!(libxl_driver->reservedGraphicsPorts = - virPortAllocatorNew(_("VNC"), - LIBXL_VNC_PORT_MIN, - LIBXL_VNC_PORT_MAX, - 0))) + virPortRangeNew(_("VNC"), + LIBXL_VNC_PORT_MIN, + LIBXL_VNC_PORT_MAX, + 0))) goto error; /* Allocate bitmap for migration port reservation */ if (!(libxl_driver->migrationPorts = - virPortAllocatorNew(_("migration"), - LIBXL_MIGRATION_PORT_MIN, - LIBXL_MIGRATION_PORT_MAX, 0))) + virPortRangeNew(_("migration"), + LIBXL_MIGRATION_PORT_MIN, + LIBXL_MIGRATION_PORT_MAX, 0))) goto error; if (!(libxl_driver->domains = virDomainObjListNew())) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index a553e30..e8c8bd6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -264,13 +264,13 @@ struct _virQEMUDriver { virHashTablePtr sharedDevices; /* Immutable pointer, self-locking APIs */ - virPortAllocatorPtr remotePorts; + virPortRangePtr remotePorts; /* Immutable pointer, self-locking APIs */ - virPortAllocatorPtr webSocketPorts; + virPortRangePtr webSocketPorts; /* Immutable pointer, self-locking APIs */ - virPortAllocatorPtr migrationPorts; + virPortRangePtr migrationPorts; /* Immutable pointer, lockless APIs*/ virSysinfoDefPtr hostsysinfo; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 97b194b..254ccd1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -746,24 +746,24 @@ qemuStateInitialize(bool privileged, * do this before the config is loaded properly, since the port * numbers are configurable now */ if ((qemu_driver->remotePorts = - virPortAllocatorNew(_("display"), - cfg->remotePortMin, - cfg->remotePortMax, - 0)) == NULL) + virPortRangeNew(_("display"), + cfg->remotePortMin, + cfg->remotePortMax, + 0)) == NULL) goto error; if ((qemu_driver->webSocketPorts = - virPortAllocatorNew(_("webSocket"), - cfg->webSocketPortMin, - cfg->webSocketPortMax, - 0)) == NULL) + virPortRangeNew(_("webSocket"), + cfg->webSocketPortMin, + cfg->webSocketPortMax, + 0)) == NULL) goto error; if ((qemu_driver->migrationPorts = - virPortAllocatorNew(_("migration"), - cfg->migrationPortMin, - cfg->migrationPortMax, - 0)) == NULL) + virPortRangeNew(_("migration"), + cfg->migrationPortMin, + cfg->migrationPortMax, + 0)) == NULL) goto error; if (qemuSecurityInit(qemu_driver) < 0) @@ -1106,9 +1106,9 @@ qemuStateCleanup(void) virObjectUnref(qemu_driver->qemuCapsCache); virObjectUnref(qemu_driver->domains); - virObjectUnref(qemu_driver->remotePorts); - virObjectUnref(qemu_driver->webSocketPorts); - virObjectUnref(qemu_driver->migrationPorts); + virPortRangeFree(qemu_driver->remotePorts); + virPortRangeFree(qemu_driver->webSocketPorts); + virPortRangeFree(qemu_driver->migrationPorts); virObjectUnref(qemu_driver->migrationErrors); virObjectUnref(qemu_driver->xmlopt); diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index fcd4f74..cd64356 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -35,10 +35,14 @@ #define VIR_FROM_THIS VIR_FROM_NONE +typedef struct _virPortAllocator virPortAllocator; +typedef virPortAllocator *virPortAllocatorPtr; struct _virPortAllocator { virObjectLockable parent; virBitmapPtr bitmap; +}; +struct _virPortRange { char *name; unsigned short start; @@ -48,6 +52,7 @@ struct _virPortAllocator { }; static virClassPtr virPortAllocatorClass; +static virPortAllocatorPtr virPortAllocatorInstance; static void virPortAllocatorDispose(void *obj) @@ -55,7 +60,22 @@ virPortAllocatorDispose(void *obj) virPortAllocatorPtr pa = obj; virBitmapFree(pa->bitmap); - VIR_FREE(pa->name); +} + +static virPortAllocatorPtr virPortAllocatorNew(void) +{ + virPortAllocatorPtr pa; + + if (!(pa = virObjectLockableNew(virPortAllocatorClass))) + return NULL; + + if (!(pa->bitmap = virBitmapNew(USHRT_MAX))) + goto error; + + return pa; + error: + virObjectUnref(pa); + return NULL; } static int virPortAllocatorOnceInit(void) @@ -66,17 +86,20 @@ static int virPortAllocatorOnceInit(void) virPortAllocatorDispose))) return -1; + if (!(virPortAllocatorInstance = virPortAllocatorNew())) + return -1; + return 0; } VIR_ONCE_GLOBAL_INIT(virPortAllocator) -virPortAllocatorPtr virPortAllocatorNew(const char *name, - unsigned short start, - unsigned short end, - unsigned int flags) +virPortRangePtr virPortRangeNew(const char *name, + unsigned short start, + unsigned short end, + unsigned int flags) { - virPortAllocatorPtr pa; + virPortRangePtr range; if (start >= end) { virReportInvalidArg(start, "start port %d must be less than end port %d", @@ -84,23 +107,31 @@ virPortAllocatorPtr virPortAllocatorNew(const char *name, return NULL; } - if (virPortAllocatorInitialize() < 0) + if (VIR_ALLOC(range) < 0) return NULL; - if (!(pa = virObjectLockableNew(virPortAllocatorClass))) - return NULL; + range->flags = flags; + range->start = start; + range->end = end; - pa->flags = flags; - pa->start = start; - pa->end = end; + if (VIR_STRDUP(range->name, name) < 0) + goto error; - if (!(pa->bitmap = virBitmapNew((end-start)+1)) || - VIR_STRDUP(pa->name, name) < 0) { - virObjectUnref(pa); - return NULL; - } + return range; + + error: + virPortRangeFree(range); + return NULL; +} + +void +virPortRangeFree(virPortRangePtr range) +{ + if (!range) + return; - return pa; + VIR_FREE(range->name); + VIR_FREE(range); } static int virPortAllocatorBindToPort(bool *used, @@ -172,22 +203,35 @@ static int virPortAllocatorBindToPort(bool *used, return ret; } -int virPortAllocatorAcquire(virPortAllocatorPtr pa, +static virPortAllocatorPtr virPortAllocatorGet(void) +{ + if (virPortAllocatorInitialize() < 0) + return NULL; + + return virPortAllocatorInstance; +} + +int virPortAllocatorAcquire(virPortRangePtr range, unsigned short *port) { int ret = -1; size_t i; + virPortAllocatorPtr pa = virPortAllocatorGet(); *port = 0; + + if (!pa) + return -1; + virObjectLock(pa); - for (i = pa->start; i <= pa->end && !*port; i++) { + for (i = range->start; i <= range->end && !*port; i++) { bool used = false, v6used = false; - if (virBitmapIsBitSet(pa->bitmap, i - pa->start)) + if (virBitmapIsBitSet(pa->bitmap, i)) continue; - if (!(pa->flags & VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK)) { + if (!(range->flags & VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK)) { if (virPortAllocatorBindToPort(&v6used, i, AF_INET6) < 0 || virPortAllocatorBindToPort(&used, i, AF_INET) < 0) goto cleanup; @@ -195,8 +239,7 @@ int virPortAllocatorAcquire(virPortAllocatorPtr pa, if (!used && !v6used) { /* Add port to bitmap of reserved ports */ - if (virBitmapSetBit(pa->bitmap, - i - pa->start) < 0) { + if (virBitmapSetBit(pa->bitmap, i) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to reserve port %zu"), i); goto cleanup; @@ -209,32 +252,35 @@ int virPortAllocatorAcquire(virPortAllocatorPtr pa, 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); + range->name, range->start, range->end); } cleanup: virObjectUnlock(pa); return ret; } -int virPortAllocatorRelease(virPortAllocatorPtr pa, +int virPortAllocatorRelease(virPortRangePtr range, unsigned short port) { int ret = -1; + virPortAllocatorPtr pa = virPortAllocatorGet(); + + if (!pa) + return -1; if (!port) return 0; virObjectLock(pa); - if (port < pa->start || - port > pa->end) { + if (port < range->start || + port > range->end) { virReportInvalidArg(port, "port %d must be in range (%d, %d)", - port, pa->start, pa->end); + port, range->start, range->end); goto cleanup; } - if (virBitmapClearBit(pa->bitmap, - port - pa->start) < 0) { + if (virBitmapClearBit(pa->bitmap, port) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to release port %d"), port); @@ -247,30 +293,33 @@ int virPortAllocatorRelease(virPortAllocatorPtr pa, return ret; } -int virPortAllocatorSetUsed(virPortAllocatorPtr pa, +int virPortAllocatorSetUsed(virPortRangePtr range, unsigned short port, bool value) { int ret = -1; + virPortAllocatorPtr pa = virPortAllocatorGet(); + + if (!pa) + return -1; virObjectLock(pa); - if (port < pa->start || - port > pa->end) { + if (port < range->start || + port > range->end) { ret = 0; goto cleanup; } if (value) { - if (virBitmapIsBitSet(pa->bitmap, port - pa->start) || - virBitmapSetBit(pa->bitmap, port - pa->start) < 0) { + if (virBitmapIsBitSet(pa->bitmap, port) || + virBitmapSetBit(pa->bitmap, port) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to reserve port %d"), port); goto cleanup; } } else { - if (virBitmapClearBit(pa->bitmap, - port - pa->start) < 0) { + if (virBitmapClearBit(pa->bitmap, port) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to release port %d"), port); diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h index 14c3b24..e9b9038 100644 --- a/src/util/virportallocator.h +++ b/src/util/virportallocator.h @@ -25,25 +25,27 @@ # include "internal.h" # include "virobject.h" -typedef struct _virPortAllocator virPortAllocator; -typedef virPortAllocator *virPortAllocatorPtr; +typedef struct _virPortRange virPortRange; +typedef virPortRange *virPortRangePtr; typedef enum { VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK = (1 << 0), } virPortAllocatorFlags; -virPortAllocatorPtr virPortAllocatorNew(const char *name, - unsigned short start, - unsigned short end, - unsigned int flags); +virPortRangePtr virPortRangeNew(const char *name, + unsigned short start, + unsigned short end, + unsigned int flags); -int virPortAllocatorAcquire(virPortAllocatorPtr pa, +void virPortRangeFree(virPortRangePtr range); + +int virPortAllocatorAcquire(virPortRangePtr range, unsigned short *port); -int virPortAllocatorRelease(virPortAllocatorPtr pa, +int virPortAllocatorRelease(virPortRangePtr range, unsigned short port); -int virPortAllocatorSetUsed(virPortAllocatorPtr pa, +int virPortAllocatorSetUsed(virPortRangePtr range, unsigned short port, bool value); diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c index 8128589..93c8026 100644 --- a/tests/bhyvexml2argvtest.c +++ b/tests/bhyvexml2argvtest.c @@ -145,8 +145,8 @@ mymain(void) if ((driver.xmlopt = virBhyveDriverCreateXMLConf(&driver)) == NULL) return EXIT_FAILURE; - if (!(driver.remotePorts = virPortAllocatorNew("display", 5900, 65535, - VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK))) + if (!(driver.remotePorts = virPortRangeNew("display", 5900, 65535, + VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK))) return EXIT_FAILURE; @@ -240,7 +240,7 @@ mymain(void) virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); - virObjectUnref(driver.remotePorts); + virPortRangeFree(driver.remotePorts); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index bd4c3af..b3bdcaf 100644 --- a/tests/libxlxml2domconfigtest.c +++ b/tests/libxlxml2domconfigtest.c @@ -58,7 +58,7 @@ testCompareXMLToDomConfig(const char *xmlfile, libxl_domain_config expectconfig; xentoollog_logger *log = NULL; libxl_ctx *ctx = NULL; - virPortAllocatorPtr gports = NULL; + virPortRangePtr gports = NULL; virDomainXMLOptionPtr xmlopt = NULL; virDomainDefPtr vmdef = NULL; char *actualjson = NULL; @@ -74,8 +74,8 @@ testCompareXMLToDomConfig(const char *xmlfile, if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, log) < 0) goto cleanup; - if (!(gports = virPortAllocatorNew("vnc", 5900, 6000, - VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK))) + if (!(gports = virPortRangeNew("vnc", 5900, 6000, + VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK))) goto cleanup; if (!(xmlopt = libxlCreateXMLConf())) @@ -116,7 +116,7 @@ testCompareXMLToDomConfig(const char *xmlfile, VIR_FREE(actualjson); VIR_FREE(tempjson); virDomainDefFree(vmdef); - virObjectUnref(gports); + virPortRangeFree(gports); virObjectUnref(xmlopt); libxl_ctx_free(ctx); libxl_domain_config_dispose(&actualconfig); diff --git a/tests/virportallocatortest.c b/tests/virportallocatortest.c index ed9c4f2..972b579 100644 --- a/tests/virportallocatortest.c +++ b/tests/virportallocatortest.c @@ -42,63 +42,71 @@ VIR_LOG_INIT("tests.portallocatortest"); static int testAllocAll(const void *args ATTRIBUTE_UNUSED) { - virPortAllocatorPtr alloc = virPortAllocatorNew("test", 5900, 5909, 0); + virPortRangePtr ports = virPortRangeNew("test", 5900, 5909, 0); int ret = -1; unsigned short p1, p2, p3, p4, p5, p6, p7; - if (!alloc) + if (!ports) return -1; - if (virPortAllocatorAcquire(alloc, &p1) < 0) + if (virPortAllocatorAcquire(ports, &p1) < 0) goto cleanup; if (p1 != 5901) { VIR_TEST_DEBUG("Expected 5901, got %d", p1); goto cleanup; } - if (virPortAllocatorAcquire(alloc, &p2) < 0) + if (virPortAllocatorAcquire(ports, &p2) < 0) goto cleanup; if (p2 != 5902) { VIR_TEST_DEBUG("Expected 5902, got %d", p2); goto cleanup; } - if (virPortAllocatorAcquire(alloc, &p3) < 0) + if (virPortAllocatorAcquire(ports, &p3) < 0) goto cleanup; if (p3 != 5903) { VIR_TEST_DEBUG("Expected 5903, got %d", p3); goto cleanup; } - if (virPortAllocatorAcquire(alloc, &p4) < 0) + if (virPortAllocatorAcquire(ports, &p4) < 0) goto cleanup; if (p4 != 5907) { VIR_TEST_DEBUG("Expected 5907, got %d", p4); goto cleanup; } - if (virPortAllocatorAcquire(alloc, &p5) < 0) + if (virPortAllocatorAcquire(ports, &p5) < 0) goto cleanup; if (p5 != 5908) { VIR_TEST_DEBUG("Expected 5908, got %d", p5); goto cleanup; } - if (virPortAllocatorAcquire(alloc, &p6) < 0) + if (virPortAllocatorAcquire(ports, &p6) < 0) goto cleanup; if (p6 != 5909) { VIR_TEST_DEBUG("Expected 5909, got %d", p6); goto cleanup; } - if (virPortAllocatorAcquire(alloc, &p7) == 0) { + if (virPortAllocatorAcquire(ports, &p7) == 0) { VIR_TEST_DEBUG("Expected error, got %d", p7); goto cleanup; } + virPortAllocatorRelease(ports, p1); + virPortAllocatorRelease(ports, p2); + virPortAllocatorRelease(ports, p3); + virPortAllocatorRelease(ports, p4); + virPortAllocatorRelease(ports, p5); + virPortAllocatorRelease(ports, p6); + virPortAllocatorRelease(ports, p7); + ret = 0; cleanup: - virObjectUnref(alloc); + virPortRangeFree(ports); return ret; } @@ -106,28 +114,28 @@ static int testAllocAll(const void *args ATTRIBUTE_UNUSED) static int testAllocReuse(const void *args ATTRIBUTE_UNUSED) { - virPortAllocatorPtr alloc = virPortAllocatorNew("test", 5900, 5910, 0); + virPortRangePtr ports = virPortRangeNew("test", 5900, 5910, 0); int ret = -1; unsigned short p1, p2, p3, p4; - if (!alloc) + if (!ports) return -1; - if (virPortAllocatorAcquire(alloc, &p1) < 0) + if (virPortAllocatorAcquire(ports, &p1) < 0) goto cleanup; if (p1 != 5901) { VIR_TEST_DEBUG("Expected 5901, got %d", p1); goto cleanup; } - if (virPortAllocatorAcquire(alloc, &p2) < 0) + if (virPortAllocatorAcquire(ports, &p2) < 0) goto cleanup; if (p2 != 5902) { VIR_TEST_DEBUG("Expected 5902, got %d", p2); goto cleanup; } - if (virPortAllocatorAcquire(alloc, &p3) < 0) + if (virPortAllocatorAcquire(ports, &p3) < 0) goto cleanup; if (p3 != 5903) { VIR_TEST_DEBUG("Expected 5903, got %d", p3); @@ -135,19 +143,23 @@ static int testAllocReuse(const void *args ATTRIBUTE_UNUSED) } - if (virPortAllocatorRelease(alloc, p2) < 0) + if (virPortAllocatorRelease(ports, p2) < 0) goto cleanup; - if (virPortAllocatorAcquire(alloc, &p4) < 0) + if (virPortAllocatorAcquire(ports, &p4) < 0) goto cleanup; if (p4 != 5902) { VIR_TEST_DEBUG("Expected 5902, got %d", p4); goto cleanup; } + virPortAllocatorRelease(ports, p1); + virPortAllocatorRelease(ports, p3); + virPortAllocatorRelease(ports, p4); + ret = 0; cleanup: - virObjectUnref(alloc); + virPortRangeFree(ports); return ret; } -- 1.8.3.1

On Mon, Jan 29, 2018 at 07:09:54 +0100, Michal Privoznik wrote: One additional thought. With the upcomming split of the monilithic libvirtd daemon into sub-services on a per-VM-driver basis, all the work here would be undone and this would be again a per-VM-driver port allocator. While it would be possible to add a port allocator service to our daemon collection (I think we are doing some serious necromancy here.) I'm not sure whether it's worth. It would be far better to fix the hypervisor programs to accept ports via 'fd passing' and thus no races would be possible since we'd get this handled by the kernel.

On 29.01.2018 09:09, Michal Privoznik wrote: Not sure why we need to free it. It is like global variables for classes, we don't need to free them yet. As to libxlxml2domconfigtest it can be fixed just like virportallocatortest by releasing all acquired ports. I think this won't work. virPortAllocatorInitialize will set ref to 1 on behalf of virPortAllocatorInstance. So after you dispose all ranges you still have this 1 ref and dispose will not be called. And I don't know how to fix this approach because on this way we have to use virOnce somehow to initialize virPortAllocatorInstance many times. Also it looks just not natural - after freeing all port ranges you destroy port allocator. Client can use port range as a temporary construct just to acquire port. Sorry, I did not build with libxl driver and thus did not run this test. I can fix it as virportallocatortest - by releasing all acquired ports. Nikolay

On Mon, Feb 05, 2018 at 01:39:56PM +0100, Michal Privoznik wrote: The caching of the used ports in the bitmap is just an optimization, to avoid us having to retry the bind()+listen() on every port we've previously got in use. If we split the daemon, if multiple daemons all need port allocation tracking, they'll get separate virPortAllocator bitmap instances. Since one daemon won't see what other daemon has in use, it will mean that we must try to bind()+listen() on ports that the other daemon has in use. Thereafter we'll have cached that usage the bitmap. The main downside is that if one daemon releases a port, the other daemon won't see that release. This is only a significant problem if the 2 (or more) daemons are using the same port range. This would, however, be exactly the same when we have a per-QEMU instance daemon. The proposed change, however, does not make life worse than it already is in this respect. IOW, we'll probably have some trouble, but that's not a reason to reject this proposal. It is just one of many things we'll need to figure out wrt unique assignment. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Mon, Feb 05, 2018 at 02:11:24PM +0100, Peter Krempa wrote: I have patches to let QEMU accept a preopened FD for chardevs. VNC / SPICE are the other big ones we hit. I should make fixing those a higher priority. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 05.02.2018 16:27, Michal Privoznik wrote: Sure!

On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote: 1: ^^^ I'm not a big fan of this. Since virPortRange (or whatever name you'll give it) is going to have a pointer to virPortAllocator object I rather leave this signature as is. The idea (hunk 1) makes sense though Michal

Range check in virPortAllocatorSetUsed is not useful anymore when we manage ports for entire unsigned short range values. --- src/bhyve/bhyve_process.c | 3 +-- src/libxl/libxl_domain.c | 3 +-- src/libxl/libxl_migration.c | 4 ++-- src/qemu/qemu_migration.c | 12 ++++++------ src/qemu/qemu_process.c | 14 +++++--------- src/util/virportallocator.c | 10 +--------- src/util/virportallocator.h | 3 +-- tests/virportallocatortest.c | 23 +++++++++++------------ 8 files changed, 28 insertions(+), 44 deletions(-) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index e215f6a..5e682fa 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -296,8 +296,7 @@ virBhyveProcessStop(bhyveConnPtr driver, /* VNC autoport cleanup */ if ((vm->def->ngraphics == 1) && vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { - if (virPortAllocatorRelease(driver->remotePorts, - vm->def->graphics[0]->data.vnc.port) < 0) { + if (virPortAllocatorRelease(vm->def->graphics[0]->data.vnc.port) < 0) { VIR_WARN("Failed to release VNC port for '%s'", vm->def->name); } diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 395c8a9..f69d60a 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -778,8 +778,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, vm->def->graphics[0]->data.vnc.autoport) { vnc_port = vm->def->graphics[0]->data.vnc.port; if (vnc_port >= LIBXL_VNC_PORT_MIN) { - if (virPortAllocatorRelease(driver->reservedGraphicsPorts, - vnc_port) < 0) + if (virPortAllocatorRelease(vnc_port) < 0) VIR_DEBUG("Could not mark port %d as unused", vnc_port); } } diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 228c850..9c03488 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -805,7 +805,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, } VIR_FREE(socks); virObjectUnref(args); - virPortAllocatorRelease(driver->migrationPorts, priv->migrationPort); + virPortAllocatorRelease(priv->migrationPort); priv->migrationPort = 0; /* Remove virDomainObj from domain list */ @@ -1256,7 +1256,7 @@ libxlDomainMigrationFinish(virConnectPtr dconn, virObjectEventPtr event = NULL; virDomainPtr dom = NULL; - virPortAllocatorRelease(driver->migrationPorts, priv->migrationPort); + virPortAllocatorRelease(priv->migrationPort); priv->migrationPort = 0; if (cancelled) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index c1ceb16..a19abd0 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -552,7 +552,7 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver, cleanup: VIR_FREE(diskAlias); if (ret < 0 && nbdPort == 0) - virPortAllocatorRelease(driver->migrationPorts, port); + virPortAllocatorRelease(port); return ret; exit_monitor: @@ -580,7 +580,7 @@ qemuMigrationStopNBDServer(virQEMUDriverPtr driver, if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; - virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort); + virPortAllocatorRelease(priv->nbdPort); priv->nbdPort = 0; return 0; } @@ -2227,7 +2227,7 @@ qemuMigrationPrepareCleanup(virQEMUDriverPtr driver, qemuDomainJobTypeToString(priv->job.active), qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); - virPortAllocatorRelease(driver->migrationPorts, priv->migrationPort); + virPortAllocatorRelease(priv->migrationPort); priv->migrationPort = 0; if (!qemuMigrationJobIsActive(vm, QEMU_ASYNC_JOB_MIGRATION_IN)) @@ -2907,7 +2907,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, * it is given in parameters */ if (nbdPort == 0) - virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort); + virPortAllocatorRelease(priv->nbdPort); priv->nbdPort = 0; virDomainObjRemoveTransientDef(vm); qemuDomainRemoveInactiveJob(driver, vm); @@ -3150,7 +3150,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, if (ret != 0) { VIR_FREE(*uri_out); if (autoPort) - virPortAllocatorRelease(driver->migrationPorts, port); + virPortAllocatorRelease(port); } return ret; } @@ -5500,7 +5500,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver, cleanup: VIR_FREE(jobInfo); - virPortAllocatorRelease(driver->migrationPorts, port); + virPortAllocatorRelease(port); if (priv->mon) qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL); VIR_FREE(priv->origname); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b0f8ea8..0777922 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6384,7 +6384,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, } } - virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort); + virPortAllocatorRelease(priv->nbdPort); priv->nbdPort = 0; if (priv->agent) { @@ -6539,15 +6539,13 @@ void qemuProcessStop(virQEMUDriverPtr driver, 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); + virPortAllocatorRelease(graphics->data.vnc.port); } else if (graphics->data.vnc.portReserved) { virPortAllocatorSetUsed(graphics->data.spice.port, false); graphics->data.vnc.portReserved = false; } if (graphics->data.vnc.websocketGenerated) { - virPortAllocatorRelease(driver->webSocketPorts, - graphics->data.vnc.websocket); + virPortAllocatorRelease(graphics->data.vnc.websocket); graphics->data.vnc.websocketGenerated = false; graphics->data.vnc.websocket = -1; } else if (graphics->data.vnc.websocket) { @@ -6556,10 +6554,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, } if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { if (graphics->data.spice.autoport) { - virPortAllocatorRelease(driver->remotePorts, - graphics->data.spice.port); - virPortAllocatorRelease(driver->remotePorts, - graphics->data.spice.tlsPort); + virPortAllocatorRelease(graphics->data.spice.port); + virPortAllocatorRelease(graphics->data.spice.tlsPort); } else { if (graphics->data.spice.portReserved) { virPortAllocatorSetUsed(graphics->data.spice.port, false); diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index 76346c7..a154806 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -259,8 +259,7 @@ int virPortAllocatorAcquire(virPortRangePtr range, return ret; } -int virPortAllocatorRelease(virPortRangePtr range, - unsigned short port) +int virPortAllocatorRelease(unsigned short port) { int ret = -1; virPortAllocatorPtr pa = virPortAllocatorGet(); @@ -273,13 +272,6 @@ int virPortAllocatorRelease(virPortRangePtr range, virObjectLock(pa); - if (port < range->start || - port > range->end) { - virReportInvalidArg(port, "port %d must be in range (%d, %d)", - port, range->start, range->end); - goto cleanup; - } - if (virBitmapClearBit(pa->bitmap, port) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to release port %d"), diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h index 8511eca..ae00edc 100644 --- a/src/util/virportallocator.h +++ b/src/util/virportallocator.h @@ -42,8 +42,7 @@ void virPortRangeFree(virPortRangePtr range); int virPortAllocatorAcquire(virPortRangePtr range, unsigned short *port); -int virPortAllocatorRelease(virPortRangePtr range, - unsigned short port); +int virPortAllocatorRelease(unsigned short port); int virPortAllocatorSetUsed(unsigned short port, bool value); diff --git a/tests/virportallocatortest.c b/tests/virportallocatortest.c index 972b579..cd792ad 100644 --- a/tests/virportallocatortest.c +++ b/tests/virportallocatortest.c @@ -96,13 +96,13 @@ static int testAllocAll(const void *args ATTRIBUTE_UNUSED) goto cleanup; } - virPortAllocatorRelease(ports, p1); - virPortAllocatorRelease(ports, p2); - virPortAllocatorRelease(ports, p3); - virPortAllocatorRelease(ports, p4); - virPortAllocatorRelease(ports, p5); - virPortAllocatorRelease(ports, p6); - virPortAllocatorRelease(ports, p7); + virPortAllocatorRelease(p1); + virPortAllocatorRelease(p2); + virPortAllocatorRelease(p3); + virPortAllocatorRelease(p4); + virPortAllocatorRelease(p5); + virPortAllocatorRelease(p6); + virPortAllocatorRelease(p7); ret = 0; cleanup: @@ -142,8 +142,7 @@ static int testAllocReuse(const void *args ATTRIBUTE_UNUSED) goto cleanup; } - - if (virPortAllocatorRelease(ports, p2) < 0) + if (virPortAllocatorRelease(p2) < 0) goto cleanup; if (virPortAllocatorAcquire(ports, &p4) < 0) @@ -153,9 +152,9 @@ static int testAllocReuse(const void *args ATTRIBUTE_UNUSED) goto cleanup; } - virPortAllocatorRelease(ports, p1); - virPortAllocatorRelease(ports, p3); - virPortAllocatorRelease(ports, p4); + virPortAllocatorRelease(p1); + virPortAllocatorRelease(p3); + virPortAllocatorRelease(p4); ret = 0; cleanup: -- 1.8.3.1

This flag is only used for tests and tests overload socket and bind functions using virportallocatormock.c already in a suitable fashion so we don't need this flag at all. --- src/bhyve/bhyve_driver.c | 2 +- src/libxl/libxl_driver.c | 5 ++--- src/qemu/qemu_driver.c | 9 +++------ src/util/virportallocator.c | 14 ++++---------- src/util/virportallocator.h | 7 +------ tests/bhyvexml2argvtest.c | 3 +-- tests/libxlxml2domconfigtest.c | 3 +-- tests/virportallocatortest.c | 4 ++-- 8 files changed, 15 insertions(+), 32 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 43487f5..19c86e2 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1267,7 +1267,7 @@ bhyveStateInitialize(bool privileged, if (!(bhyve_driver->domainEventState = virObjectEventStateNew())) goto cleanup; - if (!(bhyve_driver->remotePorts = virPortRangeNew(_("display"), 5900, 65535, 0))) + if (!(bhyve_driver->remotePorts = virPortRangeNew(_("display"), 5900, 65535))) goto cleanup; bhyve_driver->hostsysinfo = virSysinfoRead(); diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5209ea7..18126e4 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -659,15 +659,14 @@ libxlStateInitialize(bool privileged, if (!(libxl_driver->reservedGraphicsPorts = virPortRangeNew(_("VNC"), LIBXL_VNC_PORT_MIN, - LIBXL_VNC_PORT_MAX, - 0))) + LIBXL_VNC_PORT_MAX))) goto error; /* Allocate bitmap for migration port reservation */ if (!(libxl_driver->migrationPorts = virPortRangeNew(_("migration"), LIBXL_MIGRATION_PORT_MIN, - LIBXL_MIGRATION_PORT_MAX, 0))) + LIBXL_MIGRATION_PORT_MAX))) goto error; if (!(libxl_driver->domains = virDomainObjListNew())) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 254ccd1..edde4e2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -748,22 +748,19 @@ qemuStateInitialize(bool privileged, if ((qemu_driver->remotePorts = virPortRangeNew(_("display"), cfg->remotePortMin, - cfg->remotePortMax, - 0)) == NULL) + cfg->remotePortMax)) == NULL) goto error; if ((qemu_driver->webSocketPorts = virPortRangeNew(_("webSocket"), cfg->webSocketPortMin, - cfg->webSocketPortMax, - 0)) == NULL) + cfg->webSocketPortMax)) == NULL) goto error; if ((qemu_driver->migrationPorts = virPortRangeNew(_("migration"), cfg->migrationPortMin, - cfg->migrationPortMax, - 0)) == NULL) + cfg->migrationPortMax)) == NULL) goto error; if (qemuSecurityInit(qemu_driver) < 0) diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index a154806..76fac49 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -47,8 +47,6 @@ struct _virPortRange { unsigned short start; unsigned short end; - - unsigned int flags; }; static virClassPtr virPortAllocatorClass; @@ -96,8 +94,7 @@ VIR_ONCE_GLOBAL_INIT(virPortAllocator) virPortRangePtr virPortRangeNew(const char *name, unsigned short start, - unsigned short end, - unsigned int flags) + unsigned short end) { virPortRangePtr range; @@ -110,7 +107,6 @@ virPortRangePtr virPortRangeNew(const char *name, if (VIR_ALLOC(range) < 0) return NULL; - range->flags = flags; range->start = start; range->end = end; @@ -231,11 +227,9 @@ int virPortAllocatorAcquire(virPortRangePtr range, if (virBitmapIsBitSet(pa->bitmap, i)) continue; - if (!(range->flags & VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK)) { - if (virPortAllocatorBindToPort(&v6used, i, AF_INET6) < 0 || - virPortAllocatorBindToPort(&used, i, AF_INET) < 0) - goto cleanup; - } + if (virPortAllocatorBindToPort(&v6used, i, AF_INET6) < 0 || + virPortAllocatorBindToPort(&used, i, AF_INET) < 0) + goto cleanup; if (!used && !v6used) { /* Add port to bitmap of reserved ports */ diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h index ae00edc..bddeadd 100644 --- a/src/util/virportallocator.h +++ b/src/util/virportallocator.h @@ -28,14 +28,9 @@ typedef struct _virPortRange virPortRange; typedef virPortRange *virPortRangePtr; -typedef enum { - VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK = (1 << 0), -} virPortAllocatorFlags; - virPortRangePtr virPortRangeNew(const char *name, unsigned short start, - unsigned short end, - unsigned int flags); + unsigned short end); void virPortRangeFree(virPortRangePtr range); diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c index 93c8026..4edf921 100644 --- a/tests/bhyvexml2argvtest.c +++ b/tests/bhyvexml2argvtest.c @@ -145,8 +145,7 @@ mymain(void) if ((driver.xmlopt = virBhyveDriverCreateXMLConf(&driver)) == NULL) return EXIT_FAILURE; - if (!(driver.remotePorts = virPortRangeNew("display", 5900, 65535, - VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK))) + if (!(driver.remotePorts = virPortRangeNew("display", 5900, 65535))) return EXIT_FAILURE; diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c index b3bdcaf..375fac3 100644 --- a/tests/libxlxml2domconfigtest.c +++ b/tests/libxlxml2domconfigtest.c @@ -74,8 +74,7 @@ testCompareXMLToDomConfig(const char *xmlfile, if (libxl_ctx_alloc(&ctx, LIBXL_VERSION, 0, log) < 0) goto cleanup; - if (!(gports = virPortRangeNew("vnc", 5900, 6000, - VIR_PORT_ALLOCATOR_SKIP_BIND_CHECK))) + if (!(gports = virPortRangeNew("vnc", 5900, 6000))) goto cleanup; if (!(xmlopt = libxlCreateXMLConf())) diff --git a/tests/virportallocatortest.c b/tests/virportallocatortest.c index cd792ad..25641d1 100644 --- a/tests/virportallocatortest.c +++ b/tests/virportallocatortest.c @@ -42,7 +42,7 @@ VIR_LOG_INIT("tests.portallocatortest"); static int testAllocAll(const void *args ATTRIBUTE_UNUSED) { - virPortRangePtr ports = virPortRangeNew("test", 5900, 5909, 0); + virPortRangePtr ports = virPortRangeNew("test", 5900, 5909); int ret = -1; unsigned short p1, p2, p3, p4, p5, p6, p7; @@ -114,7 +114,7 @@ static int testAllocAll(const void *args ATTRIBUTE_UNUSED) static int testAllocReuse(const void *args ATTRIBUTE_UNUSED) { - virPortRangePtr ports = virPortRangeNew("test", 5900, 5910, 0); + virPortRangePtr ports = virPortRangeNew("test", 5900, 5910); int ret = -1; unsigned short p1, p2, p3, p4; -- 1.8.3.1