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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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!
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.