[libvirt] [PATCH 0/6] port allocator: make used port bitmap global etc
This patch set addresses issue described in [1] and the core of changes go to the first patch. The others are cleanups and refactorings. [1] https://www.redhat.com/archives/libvir-list/2017-December/msg00600.html Nikolay Shirokovskiy (6): port allocator: make used port bitmap global port allocator: remove range on manual port reserving port allocator: remove range check in release function port allocator: drop skip flag port allocator: remove release functionality from set used port allocator: make port range constant object src/bhyve/bhyve_command.c | 4 +- src/bhyve/bhyve_driver.c | 4 +- src/bhyve/bhyve_process.c | 7 +- src/bhyve/bhyve_utils.h | 2 +- src/libvirt_private.syms | 3 +- src/libxl/libxl_conf.c | 8 +-- src/libxl/libxl_conf.h | 12 ++-- src/libxl/libxl_domain.c | 3 +- src/libxl/libxl_driver.c | 17 +++-- src/libxl/libxl_migration.c | 4 +- src/qemu/qemu_conf.h | 12 ++-- src/qemu/qemu_driver.c | 27 ++++---- src/qemu/qemu_migration.c | 12 ++-- src/qemu/qemu_process.c | 55 +++++---------- src/util/virportallocator.c | 148 +++++++++++++++++++++++------------------ src/util/virportallocator.h | 24 +++---- tests/bhyvexml2argvtest.c | 5 +- tests/libxlxml2domconfigtest.c | 7 +- tests/virportallocatortest.c | 49 ++++++++------ 19 files changed, 196 insertions(+), 207 deletions(-) -- 1.8.3.1
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 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
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/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;
I wonder if this is the way to go. I mean, this virPortAllocatorInstance is going to be a global variable that will never be freed (even if we wanted to). I mean, if virPortRange had a pointer to virPortAllocator and ref/unref it in virPortRangeNew() and virPortRangeFree() then we're good. To break this down, leave the global variable, have virPortRangeNew() call: virPortRangePtr virPortRangeNew(const char *name, unsigned short start, unsigned short end, unsigned int flags) { virPortAllocatorInitialize(); range->pa = virObjectRef(virPortAllocatorInstance); ... } Also, virPortAllocatorDispose() would set the virPortAllocatorInstance pointer to NULL. This is also the root cause of libxlxml2domconfigtest failing for me. So even though you construct virPortRange fresh new for each testCompareXMLToDomConfig() run, the underlying virPortAllocatorInstance stays through different runs. So a port allocated in one run stays allocated in the second run too. Yeah, virPortAllocatorRelease() is never called from this test. Oh, and one BIG problem although complier doesn't complain: there already is virPortRange in src/util/virsocketaddr.h and it looks slightly different. So you need to rename your type. virPortAllocatorRange maybe? Michal
On Mon, Jan 29, 2018 at 07:09:54 +0100, Michal Privoznik wrote:
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
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/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;
I wonder if this is the way to go. I mean, this virPortAllocatorInstance is going to be a global variable that will never be freed (even if we wanted to). I mean, if virPortRange had a pointer to virPortAllocator and ref/unref it in virPortRangeNew() and virPortRangeFree() then we're good. To break this down, leave the global variable, have virPortRangeNew() call:
virPortRangePtr virPortRangeNew(const char *name, unsigned short start, unsigned short end, unsigned int flags) {
virPortAllocatorInitialize();
range->pa = virObjectRef(virPortAllocatorInstance); ... }
Also, virPortAllocatorDispose() would set the virPortAllocatorInstance pointer to NULL.
This is also the root cause of libxlxml2domconfigtest failing for me. So even though you construct virPortRange fresh new for each testCompareXMLToDomConfig() run, the underlying virPortAllocatorInstance stays through different runs. So a port allocated in one run stays allocated in the second run too. Yeah, virPortAllocatorRelease() is never called from this test.
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 01/29/2018 12:07 PM, Peter Krempa wrote:
On Mon, Jan 29, 2018 at 07:09:54 +0100, Michal Privoznik wrote:
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
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/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;
I wonder if this is the way to go. I mean, this virPortAllocatorInstance is going to be a global variable that will never be freed (even if we wanted to). I mean, if virPortRange had a pointer to virPortAllocator and ref/unref it in virPortRangeNew() and virPortRangeFree() then we're good. To break this down, leave the global variable, have virPortRangeNew() call:
virPortRangePtr virPortRangeNew(const char *name, unsigned short start, unsigned short end, unsigned int flags) {
virPortAllocatorInitialize();
range->pa = virObjectRef(virPortAllocatorInstance); ... }
Also, virPortAllocatorDispose() would set the virPortAllocatorInstance pointer to NULL.
This is also the root cause of libxlxml2domconfigtest failing for me. So even though you construct virPortRange fresh new for each testCompareXMLToDomConfig() run, the underlying virPortAllocatorInstance stays through different runs. So a port allocated in one run stays allocated in the second run too. Yeah, virPortAllocatorRelease() is never called from this test.
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.
Agreed. I was thinking about this, but I'm unsure how far we are from that happening.
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.
Agreed, but that is even more remote than the big split into daemons. For instance, libxl doesn't even have any notion of FD passing. So in the end, we will need a separate virportallocatord I guess. But okay, how about we meet in the middle and turn these patches into a separate daemon? It looks to me like Dan started working on the big split and if we introduce separate daemon for this we will not interfere with his work. Michal
On 29.01.2018 09:09, Michal Privoznik wrote:
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
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/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;
I wonder if this is the way to go. I mean, this virPortAllocatorInstance is going to be a global variable that will never be freed (even if we wanted to). I mean, if virPortRange had a pointer to virPortAllocator
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.
and ref/unref it in virPortRangeNew() and virPortRangeFree() then we're good. To break this down, leave the global variable, have virPortRangeNew() call:
virPortRangePtr virPortRangeNew(const char *name, unsigned short start, unsigned short end, unsigned int flags) {
virPortAllocatorInitialize();
range->pa = virObjectRef(virPortAllocatorInstance); ... }
Also, virPortAllocatorDispose() would set the virPortAllocatorInstance pointer to NULL.
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.
This is also the root cause of libxlxml2domconfigtest failing for me. So even though you construct virPortRange fresh new for each testCompareXMLToDomConfig() run, the underlying virPortAllocatorInstance stays through different runs. So a port allocated in one run stays allocated in the second run too. Yeah, virPortAllocatorRelease() is never called from this test.
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 02/01/2018 08:51 AM, Nikolay Shirokovskiy wrote:
On 29.01.2018 09:09, Michal Privoznik wrote:
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
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/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;
I wonder if this is the way to go. I mean, this virPortAllocatorInstance is going to be a global variable that will never be freed (even if we wanted to). I mean, if virPortRange had a pointer to virPortAllocator
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.
Well, okay. Disregard my suggestion. However, what we still need to discuss is the driver separation work of Daniel's. Dan, how badly will this hit you if I merged these? In another thread I suggested to turn this into a separate deaemon (which might be overkill). Michal
On Mon, Feb 05, 2018 at 01:39:56PM +0100, Michal Privoznik wrote:
On 02/01/2018 08:51 AM, Nikolay Shirokovskiy wrote:
On 29.01.2018 09:09, Michal Privoznik wrote:
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
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/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;
I wonder if this is the way to go. I mean, this virPortAllocatorInstance is going to be a global variable that will never be freed (even if we wanted to). I mean, if virPortRange had a pointer to virPortAllocator
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.
Well, okay. Disregard my suggestion. However, what we still need to discuss is the driver separation work of Daniel's. Dan, how badly will this hit you if I merged these? In another thread I suggested to turn this into a separate deaemon (which might be overkill).
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 12:55:11 +0000, Daniel Berrange wrote:
On Mon, Feb 05, 2018 at 01:39:56PM +0100, Michal Privoznik wrote:
On 02/01/2018 08:51 AM, Nikolay Shirokovskiy wrote:
On 29.01.2018 09:09, Michal Privoznik wrote:
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
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/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;
I wonder if this is the way to go. I mean, this virPortAllocatorInstance is going to be a global variable that will never be freed (even if we wanted to). I mean, if virPortRange had a pointer to virPortAllocator
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.
Well, okay. Disregard my suggestion. However, what we still need to discuss is the driver separation work of Daniel's. Dan, how badly will this hit you if I merged these? In another thread I suggested to turn this into a separate deaemon (which might be overkill).
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.
Well, you get slightly worse odds of having the same kind of race if you have multiple instances of the port allocation approach in multiple processes. Our problem is that when we bind()+listen() we still need to close that port and have qemu open it again. This race window is still present but will be worsened by multiple of these doing the same thing. When qemu will be able to accept the socket via FD passing then this would be strictly an optimization, but until then it worsens the odds of failure.
On Mon, Feb 05, 2018 at 02:11:24PM +0100, Peter Krempa wrote:
On Mon, Feb 05, 2018 at 12:55:11 +0000, Daniel Berrange wrote:
On Mon, Feb 05, 2018 at 01:39:56PM +0100, Michal Privoznik wrote:
On 02/01/2018 08:51 AM, Nikolay Shirokovskiy wrote:
On 29.01.2018 09:09, Michal Privoznik wrote:
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
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/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;
I wonder if this is the way to go. I mean, this virPortAllocatorInstance is going to be a global variable that will never be freed (even if we wanted to). I mean, if virPortRange had a pointer to virPortAllocator
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.
Well, okay. Disregard my suggestion. However, what we still need to discuss is the driver separation work of Daniel's. Dan, how badly will this hit you if I merged these? In another thread I suggested to turn this into a separate deaemon (which might be overkill).
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.
Well, you get slightly worse odds of having the same kind of race if you have multiple instances of the port allocation approach in multiple processes.
Our problem is that when we bind()+listen() we still need to close that port and have qemu open it again. This race window is still present but will be worsened by multiple of these doing the same thing.
When qemu will be able to accept the socket via FD passing then this would be strictly an optimization, but until then it worsens the odds of failure.
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 02/05/2018 02:21 PM, Daniel P. Berrangé wrote:
On Mon, Feb 05, 2018 at 02:11:24PM +0100, Peter Krempa wrote:
On Mon, Feb 05, 2018 at 12:55:11 +0000, Daniel Berrange wrote:
On Mon, Feb 05, 2018 at 01:39:56PM +0100, Michal Privoznik wrote:
On 02/01/2018 08:51 AM, Nikolay Shirokovskiy wrote:
On 29.01.2018 09:09, Michal Privoznik wrote:
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote: > 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/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;
I wonder if this is the way to go. I mean, this virPortAllocatorInstance is going to be a global variable that will never be freed (even if we wanted to). I mean, if virPortRange had a pointer to virPortAllocator
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.
Well, okay. Disregard my suggestion. However, what we still need to discuss is the driver separation work of Daniel's. Dan, how badly will this hit you if I merged these? In another thread I suggested to turn this into a separate deaemon (which might be overkill).
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.
Well, you get slightly worse odds of having the same kind of race if you have multiple instances of the port allocation approach in multiple processes.
Our problem is that when we bind()+listen() we still need to close that port and have qemu open it again. This race window is still present but will be worsened by multiple of these doing the same thing.
When qemu will be able to accept the socket via FD passing then this would be strictly an optimization, but until then it worsens the odds of failure.
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.
Okay, so it looks like this can be merged. I mean, v2 can be merged (which fixes other issues raised like tests failing, build fixes in some drivers, etc.). Nikolay, do you think you can send v2? Michal
On 05.02.2018 16:27, Michal Privoznik wrote:
On 02/05/2018 02:21 PM, Daniel P. Berrangé wrote:
On Mon, Feb 05, 2018 at 02:11:24PM +0100, Peter Krempa wrote:
On Mon, Feb 05, 2018 at 12:55:11 +0000, Daniel Berrange wrote:
On Mon, Feb 05, 2018 at 01:39:56PM +0100, Michal Privoznik wrote:
On 02/01/2018 08:51 AM, Nikolay Shirokovskiy wrote:
On 29.01.2018 09:09, Michal Privoznik wrote: > On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote: >> 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/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; > > I wonder if this is the way to go. I mean, this virPortAllocatorInstance > is going to be a global variable that will never be freed (even if we > wanted to). I mean, if virPortRange had a pointer to virPortAllocator
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.
Well, okay. Disregard my suggestion. However, what we still need to discuss is the driver separation work of Daniel's. Dan, how badly will this hit you if I merged these? In another thread I suggested to turn this into a separate deaemon (which might be overkill).
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.
Well, you get slightly worse odds of having the same kind of race if you have multiple instances of the port allocation approach in multiple processes.
Our problem is that when we bind()+listen() we still need to close that port and have qemu open it again. This race window is still present but will be worsened by multiple of these doing the same thing.
When qemu will be able to accept the socket via FD passing then this would be strictly an optimization, but until then it worsens the odds of failure.
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.
Okay, so it looks like this can be merged. I mean, v2 can be merged (which fixes other issues raised like tests failing, build fixes in some drivers, etc.). Nikolay, do you think you can send v2?
Sure!
Range check in virPortAllocatorSetUsed is not useful anymore when we manage ports for entire unsigned short range values. --- src/bhyve/bhyve_command.c | 4 +--- src/bhyve/bhyve_process.c | 4 +--- src/qemu/qemu_process.c | 41 +++++++++++------------------------------ src/util/virportallocator.c | 9 +-------- src/util/virportallocator.h | 4 +--- 5 files changed, 15 insertions(+), 47 deletions(-) diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 55032ae..9d21788 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -411,9 +411,7 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def, return -1; graphics->data.vnc.port = port; } else { - if (virPortAllocatorSetUsed(driver->remotePorts, - graphics->data.vnc.port, - true) < 0) + if (virPortAllocatorSetUsed(graphics->data.vnc.port, true) < 0) VIR_WARN("Failed to mark VNC port '%d' as used by '%s'", graphics->data.vnc.port, def->name); } diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 5076246..e215f6a 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -425,9 +425,7 @@ virBhyveProcessReconnect(virDomainObjPtr vm, if (vm->def->ngraphics == 1 && vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { int vnc_port = vm->def->graphics[0]->data.vnc.port; - if (virPortAllocatorSetUsed(data->driver->remotePorts, - vnc_port, - true) < 0) { + if (virPortAllocatorSetUsed(vnc_port, true) < 0) { VIR_WARN("Failed to mark VNC port '%d' as used by '%s'", vnc_port, vm->def->name); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a0f430f..b0f8ea8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4153,8 +4153,7 @@ qemuProcessStartHook(virQEMUDriverPtr driver, static int -qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver, - virDomainGraphicsDefPtr graphics, +qemuProcessGraphicsReservePorts(virDomainGraphicsDefPtr graphics, bool reconnect) { virDomainGraphicsListenDefPtr glisten; @@ -4172,16 +4171,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver, case VIR_DOMAIN_GRAPHICS_TYPE_VNC: if (!graphics->data.vnc.autoport || reconnect) { - if (virPortAllocatorSetUsed(driver->remotePorts, - graphics->data.vnc.port, - true) < 0) + if (virPortAllocatorSetUsed(graphics->data.vnc.port, true) < 0) return -1; graphics->data.vnc.portReserved = true; } if (graphics->data.vnc.websocket > 0 && - virPortAllocatorSetUsed(driver->remotePorts, - graphics->data.vnc.websocket, - true) < 0) + virPortAllocatorSetUsed(graphics->data.vnc.websocket, true) < 0) return -1; break; @@ -4190,17 +4185,13 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver, return 0; if (graphics->data.spice.port > 0) { - if (virPortAllocatorSetUsed(driver->remotePorts, - graphics->data.spice.port, - true) < 0) + if (virPortAllocatorSetUsed(graphics->data.spice.port, true) < 0) return -1; graphics->data.spice.portReserved = true; } if (graphics->data.spice.tlsPort > 0) { - if (virPortAllocatorSetUsed(driver->remotePorts, - graphics->data.spice.tlsPort, - true) < 0) + if (virPortAllocatorSetUsed(graphics->data.spice.tlsPort, true) < 0) return -1; graphics->data.spice.tlsPortReserved = true; } @@ -4390,7 +4381,7 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver, for (i = 0; i < vm->def->ngraphics; i++) { graphics = vm->def->graphics[i]; - if (qemuProcessGraphicsReservePorts(driver, graphics, false) < 0) + if (qemuProcessGraphicsReservePorts(graphics, false) < 0) goto cleanup; } } @@ -6551,9 +6542,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, virPortAllocatorRelease(driver->remotePorts, graphics->data.vnc.port); } else if (graphics->data.vnc.portReserved) { - virPortAllocatorSetUsed(driver->remotePorts, - graphics->data.spice.port, - false); + virPortAllocatorSetUsed(graphics->data.spice.port, false); graphics->data.vnc.portReserved = false; } if (graphics->data.vnc.websocketGenerated) { @@ -6562,9 +6551,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, graphics->data.vnc.websocketGenerated = false; graphics->data.vnc.websocket = -1; } else if (graphics->data.vnc.websocket) { - virPortAllocatorSetUsed(driver->remotePorts, - graphics->data.vnc.websocket, - false); + virPortAllocatorSetUsed(graphics->data.vnc.websocket, false); } } if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { @@ -6575,16 +6562,12 @@ void qemuProcessStop(virQEMUDriverPtr driver, graphics->data.spice.tlsPort); } else { if (graphics->data.spice.portReserved) { - virPortAllocatorSetUsed(driver->remotePorts, - graphics->data.spice.port, - false); + virPortAllocatorSetUsed(graphics->data.spice.port, false); graphics->data.spice.portReserved = false; } if (graphics->data.spice.tlsPortReserved) { - virPortAllocatorSetUsed(driver->remotePorts, - graphics->data.spice.tlsPort, - false); + virPortAllocatorSetUsed(graphics->data.spice.tlsPort, false); graphics->data.spice.tlsPortReserved = false; } } @@ -7177,9 +7160,7 @@ qemuProcessReconnect(void *opaque) } for (i = 0; i < obj->def->ngraphics; i++) { - if (qemuProcessGraphicsReservePorts(driver, - obj->def->graphics[i], - true) < 0) + if (qemuProcessGraphicsReservePorts(obj->def->graphics[i], true) < 0) goto error; } diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index cd64356..76346c7 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -293,8 +293,7 @@ int virPortAllocatorRelease(virPortRangePtr range, return ret; } -int virPortAllocatorSetUsed(virPortRangePtr range, - unsigned short port, +int virPortAllocatorSetUsed(unsigned short port, bool value) { int ret = -1; @@ -305,12 +304,6 @@ int virPortAllocatorSetUsed(virPortRangePtr range, virObjectLock(pa); - if (port < range->start || - port > range->end) { - ret = 0; - goto cleanup; - } - if (value) { if (virBitmapIsBitSet(pa->bitmap, port) || virBitmapSetBit(pa->bitmap, port) < 0) { diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h index e9b9038..8511eca 100644 --- a/src/util/virportallocator.h +++ b/src/util/virportallocator.h @@ -45,8 +45,6 @@ int virPortAllocatorAcquire(virPortRangePtr range, int virPortAllocatorRelease(virPortRangePtr range, unsigned short port); -int virPortAllocatorSetUsed(virPortRangePtr range, - unsigned short port, - bool value); +int virPortAllocatorSetUsed(unsigned short port, bool value); #endif /* __VIR_PORT_ALLOCATOR_H__ */ -- 1.8.3.1
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
Range check in virPortAllocatorSetUsed is not useful anymore when we manage ports for entire unsigned short range values. --- src/bhyve/bhyve_command.c | 4 +--- src/bhyve/bhyve_process.c | 4 +--- src/qemu/qemu_process.c | 41 +++++++++++------------------------------ src/util/virportallocator.c | 9 +-------- src/util/virportallocator.h | 4 +--- 5 files changed, 15 insertions(+), 47 deletions(-)
diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index cd64356..76346c7 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -293,8 +293,7 @@ int virPortAllocatorRelease(virPortRangePtr range, return ret; }
-int virPortAllocatorSetUsed(virPortRangePtr range, - unsigned short port, +int virPortAllocatorSetUsed(unsigned short port, bool value) { int ret = -1; @@ -305,12 +304,6 @@ int virPortAllocatorSetUsed(virPortRangePtr range,
virObjectLock(pa);
- if (port < range->start || - port > range->end) { - ret = 0; - goto cleanup; - } -
1: ^^^
if (value) { if (virBitmapIsBitSet(pa->bitmap, port) || virBitmapSetBit(pa->bitmap, port) < 0) { diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h index e9b9038..8511eca 100644 --- a/src/util/virportallocator.h +++ b/src/util/virportallocator.h @@ -45,8 +45,6 @@ int virPortAllocatorAcquire(virPortRangePtr range, int virPortAllocatorRelease(virPortRangePtr range, unsigned short port);
-int virPortAllocatorSetUsed(virPortRangePtr range, - unsigned short port, - bool value); +int virPortAllocatorSetUsed(unsigned short port, bool value);
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
On 29.01.2018 09:09, Michal Privoznik wrote:
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
Range check in virPortAllocatorSetUsed is not useful anymore when we manage ports for entire unsigned short range values. --- src/bhyve/bhyve_command.c | 4 +--- src/bhyve/bhyve_process.c | 4 +--- src/qemu/qemu_process.c | 41 +++++++++++------------------------------ src/util/virportallocator.c | 9 +-------- src/util/virportallocator.h | 4 +--- 5 files changed, 15 insertions(+), 47 deletions(-)
diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index cd64356..76346c7 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -293,8 +293,7 @@ int virPortAllocatorRelease(virPortRangePtr range, return ret; }
-int virPortAllocatorSetUsed(virPortRangePtr range, - unsigned short port, +int virPortAllocatorSetUsed(unsigned short port, bool value) { int ret = -1; @@ -305,12 +304,6 @@ int virPortAllocatorSetUsed(virPortRangePtr range,
virObjectLock(pa);
- if (port < range->start || - port > range->end) { - ret = 0; - goto cleanup; - } -
1: ^^^
if (value) { if (virBitmapIsBitSet(pa->bitmap, port) || virBitmapSetBit(pa->bitmap, port) < 0) { diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h index e9b9038..8511eca 100644 --- a/src/util/virportallocator.h +++ b/src/util/virportallocator.h @@ -45,8 +45,6 @@ int virPortAllocatorAcquire(virPortRangePtr range, int virPortAllocatorRelease(virPortRangePtr range, unsigned short port);
-int virPortAllocatorSetUsed(virPortRangePtr range, - unsigned short port, - bool value); +int virPortAllocatorSetUsed(unsigned short port, bool value);
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
So this depends on how we get virPortAllocator object reference. Let's discuss later after we come to agreement on previous patch. Nikolay
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
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
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(-)
Same comment as in previous patch. Michal
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
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
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(-)
It's not that easy. libxlxml2domconfigtest doesn't link with the mock therefore if say port 5900 is opened at the time the test runs I get different results. Michal
On 29.01.2018 09:09, Michal Privoznik wrote:
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
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(-)
It's not that easy. libxlxml2domconfigtest doesn't link with the mock therefore if say port 5900 is opened at the time the test runs I get different results.
I did't actually check the consequences for libxl and bhyve tests... Well we can overload bind for these tests too given they already overload some functions it won't take much effort. Nikolay
Let's use virPortAllocatorRelease instead of virPortAllocatorSetUsed(false). --- src/bhyve/bhyve_command.c | 2 +- src/bhyve/bhyve_process.c | 2 +- src/qemu/qemu_process.c | 16 ++++++++-------- src/util/virportallocator.c | 22 ++++++---------------- src/util/virportallocator.h | 2 +- 5 files changed, 17 insertions(+), 27 deletions(-) diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 9d21788..d723cc2 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -411,7 +411,7 @@ bhyveBuildGraphicsArgStr(const virDomainDef *def, return -1; graphics->data.vnc.port = port; } else { - if (virPortAllocatorSetUsed(graphics->data.vnc.port, true) < 0) + if (virPortAllocatorSetUsed(graphics->data.vnc.port) < 0) VIR_WARN("Failed to mark VNC port '%d' as used by '%s'", graphics->data.vnc.port, def->name); } diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 5e682fa..4ff6257 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -424,7 +424,7 @@ virBhyveProcessReconnect(virDomainObjPtr vm, if (vm->def->ngraphics == 1 && vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { int vnc_port = vm->def->graphics[0]->data.vnc.port; - if (virPortAllocatorSetUsed(vnc_port, true) < 0) { + if (virPortAllocatorSetUsed(vnc_port) < 0) { VIR_WARN("Failed to mark VNC port '%d' as used by '%s'", vnc_port, vm->def->name); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0777922..6f4ccd9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4171,12 +4171,12 @@ qemuProcessGraphicsReservePorts(virDomainGraphicsDefPtr graphics, case VIR_DOMAIN_GRAPHICS_TYPE_VNC: if (!graphics->data.vnc.autoport || reconnect) { - if (virPortAllocatorSetUsed(graphics->data.vnc.port, true) < 0) + if (virPortAllocatorSetUsed(graphics->data.vnc.port) < 0) return -1; graphics->data.vnc.portReserved = true; } if (graphics->data.vnc.websocket > 0 && - virPortAllocatorSetUsed(graphics->data.vnc.websocket, true) < 0) + virPortAllocatorSetUsed(graphics->data.vnc.websocket) < 0) return -1; break; @@ -4185,13 +4185,13 @@ qemuProcessGraphicsReservePorts(virDomainGraphicsDefPtr graphics, return 0; if (graphics->data.spice.port > 0) { - if (virPortAllocatorSetUsed(graphics->data.spice.port, true) < 0) + if (virPortAllocatorSetUsed(graphics->data.spice.port) < 0) return -1; graphics->data.spice.portReserved = true; } if (graphics->data.spice.tlsPort > 0) { - if (virPortAllocatorSetUsed(graphics->data.spice.tlsPort, true) < 0) + if (virPortAllocatorSetUsed(graphics->data.spice.tlsPort) < 0) return -1; graphics->data.spice.tlsPortReserved = true; } @@ -6541,7 +6541,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, if (graphics->data.vnc.autoport) { virPortAllocatorRelease(graphics->data.vnc.port); } else if (graphics->data.vnc.portReserved) { - virPortAllocatorSetUsed(graphics->data.spice.port, false); + virPortAllocatorRelease(graphics->data.spice.port); graphics->data.vnc.portReserved = false; } if (graphics->data.vnc.websocketGenerated) { @@ -6549,7 +6549,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, graphics->data.vnc.websocketGenerated = false; graphics->data.vnc.websocket = -1; } else if (graphics->data.vnc.websocket) { - virPortAllocatorSetUsed(graphics->data.vnc.websocket, false); + virPortAllocatorRelease(graphics->data.vnc.websocket); } } if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { @@ -6558,12 +6558,12 @@ void qemuProcessStop(virQEMUDriverPtr driver, virPortAllocatorRelease(graphics->data.spice.tlsPort); } else { if (graphics->data.spice.portReserved) { - virPortAllocatorSetUsed(graphics->data.spice.port, false); + virPortAllocatorRelease(graphics->data.spice.port); graphics->data.spice.portReserved = false; } if (graphics->data.spice.tlsPortReserved) { - virPortAllocatorSetUsed(graphics->data.spice.tlsPort, false); + virPortAllocatorRelease(graphics->data.spice.tlsPort); graphics->data.spice.tlsPortReserved = false; } } diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index 76fac49..565715c 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -279,8 +279,7 @@ int virPortAllocatorRelease(unsigned short port) return ret; } -int virPortAllocatorSetUsed(unsigned short port, - bool value) +int virPortAllocatorSetUsed(unsigned short port) { int ret = -1; virPortAllocatorPtr pa = virPortAllocatorGet(); @@ -290,20 +289,11 @@ int virPortAllocatorSetUsed(unsigned short port, virObjectLock(pa); - if (value) { - 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) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to release port %d"), - port); - goto cleanup; - } + if (virBitmapIsBitSet(pa->bitmap, port) || + virBitmapSetBit(pa->bitmap, port) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to reserve port %d"), port); + goto cleanup; } ret = 0; diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h index bddeadd..3bc3ef2 100644 --- a/src/util/virportallocator.h +++ b/src/util/virportallocator.h @@ -39,6 +39,6 @@ int virPortAllocatorAcquire(virPortRangePtr range, int virPortAllocatorRelease(unsigned short port); -int virPortAllocatorSetUsed(unsigned short port, bool value); +int virPortAllocatorSetUsed(unsigned short port); #endif /* __VIR_PORT_ALLOCATOR_H__ */ -- 1.8.3.1
On 12/20/2017 07:35 AM, Nikolay Shirokovskiy wrote:
Let's use virPortAllocatorRelease instead of virPortAllocatorSetUsed(false). --- src/bhyve/bhyve_command.c | 2 +- src/bhyve/bhyve_process.c | 2 +- src/qemu/qemu_process.c | 16 ++++++++-------- src/util/virportallocator.c | 22 ++++++---------------- src/util/virportallocator.h | 2 +- 5 files changed, 17 insertions(+), 27 deletions(-)
ACK Michal
--- src/libxl/libxl_conf.h | 4 ++-- src/qemu/qemu_conf.h | 6 +++--- src/util/virportallocator.c | 2 +- src/util/virportallocator.h | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 3ba8710..457dabd 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -130,10 +130,10 @@ struct _libxlDriverPrivate { /* Immutable pointer, self-locking APIs */ virObjectEventStatePtr domainEventState; - /* Immutable pointer, self-locking APIs */ + /* Immutable pointer, immutable object */ virPortRangePtr reservedGraphicsPorts; - /* Immutable pointer, self-locking APIs */ + /* Immutable pointer, immutable object */ virPortRangePtr migrationPorts; /* Immutable pointer, lockless APIs*/ diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index e8c8bd6..b0854ce 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -263,13 +263,13 @@ struct _virQEMUDriver { /* Immutable pointer. Unsafe APIs. XXX */ virHashTablePtr sharedDevices; - /* Immutable pointer, self-locking APIs */ + /* Immutable pointer, immutable object */ virPortRangePtr remotePorts; - /* Immutable pointer, self-locking APIs */ + /* Immutable pointer, immutable object */ virPortRangePtr webSocketPorts; - /* Immutable pointer, self-locking APIs */ + /* Immutable pointer, immutable object */ virPortRangePtr migrationPorts; /* Immutable pointer, lockless APIs*/ diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c index 565715c..3ca3f44 100644 --- a/src/util/virportallocator.c +++ b/src/util/virportallocator.c @@ -207,7 +207,7 @@ static virPortAllocatorPtr virPortAllocatorGet(void) return virPortAllocatorInstance; } -int virPortAllocatorAcquire(virPortRangePtr range, +int virPortAllocatorAcquire(const virPortRange *range, unsigned short *port) { int ret = -1; diff --git a/src/util/virportallocator.h b/src/util/virportallocator.h index 3bc3ef2..71a924a 100644 --- a/src/util/virportallocator.h +++ b/src/util/virportallocator.h @@ -34,7 +34,7 @@ virPortRangePtr virPortRangeNew(const char *name, void virPortRangeFree(virPortRangePtr range); -int virPortAllocatorAcquire(virPortRangePtr range, +int virPortAllocatorAcquire(const virPortRange *range, unsigned short *port); int virPortAllocatorRelease(unsigned short port); -- 1.8.3.1
ping On 20.12.2017 09:35, Nikolay Shirokovskiy wrote:
This patch set addresses issue described in [1] and the core of changes go to the first patch. The others are cleanups and refactorings.
[1] https://www.redhat.com/archives/libvir-list/2017-December/msg00600.html
Nikolay Shirokovskiy (6): port allocator: make used port bitmap global port allocator: remove range on manual port reserving port allocator: remove range check in release function port allocator: drop skip flag port allocator: remove release functionality from set used port allocator: make port range constant object
src/bhyve/bhyve_command.c | 4 +- src/bhyve/bhyve_driver.c | 4 +- src/bhyve/bhyve_process.c | 7 +- src/bhyve/bhyve_utils.h | 2 +- src/libvirt_private.syms | 3 +- src/libxl/libxl_conf.c | 8 +-- src/libxl/libxl_conf.h | 12 ++-- src/libxl/libxl_domain.c | 3 +- src/libxl/libxl_driver.c | 17 +++-- src/libxl/libxl_migration.c | 4 +- src/qemu/qemu_conf.h | 12 ++-- src/qemu/qemu_driver.c | 27 ++++---- src/qemu/qemu_migration.c | 12 ++-- src/qemu/qemu_process.c | 55 +++++---------- src/util/virportallocator.c | 148 +++++++++++++++++++++++------------------ src/util/virportallocator.h | 24 +++---- tests/bhyvexml2argvtest.c | 5 +- tests/libxlxml2domconfigtest.c | 7 +- tests/virportallocatortest.c | 49 ++++++++------ 19 files changed, 196 insertions(+), 207 deletions(-)
participants (4)
-
Daniel P. Berrangé -
Michal Privoznik -
Nikolay Shirokovskiy -
Peter Krempa