[libvirt] [PATCH] qemu: configurable VNC port boundaries for domains
The defines QEMU_VNC_PORT_MIN and QEMU_VNC_PORT_MAX were used to find free port when starting domains. As this was hardcoded to the same ports as default VNC servers, there were races with these other programs. This patch includes the possibility to change the default starting port as well as the maximum port in qemu config file. Support for two new config options in qemu.conf is added: * vnc_port_min (defaults to QEMU_VNC_PORT_MIN and must be >= than this value) * vnc_port_max (defaults to QEMU_VNC_PORT_MAX and must be <= than this value) --- src/qemu/qemu.conf | 7 +++++++ src/qemu/qemu_command.h | 3 +++ src/qemu/qemu_conf.c | 37 ++++++++++++++++++++++++++++++++++++- src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_driver.c | 14 ++++++++------ src/qemu/qemu_process.c | 20 ++++++++++---------- 6 files changed, 66 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index cb87728..e2f9011 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -11,6 +11,13 @@ # # vnc_listen = "0.0.0.0" +# Override the port for creating VNC sessions (min) +# This defaults to 5900 and increases for consecutive sessions +# or when ports are occupied, until it hits the maximum. +# +#vnc_port_min = 5900 +#vnc_port_max = 65535 + # Enable this option to have VNC served over an automatically created # unix socket. This prevents unprivileged access from users on the # host machine, though most VNC clients do not support it. diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1eafeb3..f7d2884 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -37,6 +37,9 @@ # define QEMU_VIRTIO_SERIAL_PREFIX "virtio-serial" # define QEMU_FSDEV_HOST_PREFIX "fsdev-" +/* These are only defaults, they can be changed now in qemu.conf and + * explicitely specified port is checked against these two (makes + * sense to limit the values) */ # define QEMU_VNC_PORT_MIN 5900 # define QEMU_VNC_PORT_MAX 65535 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 88a04bc..354d75d 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1,7 +1,7 @@ /* * qemu_conf.c: QEMU configuration management * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -38,6 +38,7 @@ #include "virterror_internal.h" #include "qemu_conf.h" +#include "qemu_command.h" #include "qemu_capabilities.h" #include "qemu_bridge_filter.h" #include "uuid.h" @@ -89,6 +90,10 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, virReportOOMError(); return -1; } + + driver->vncPortMin = QEMU_VNC_PORT_MIN; + driver->vncPortMax = QEMU_VNC_PORT_MAX; + if (!(driver->vncTLSx509certdir = strdup(SYSCONFDIR "/pki/libvirt-vnc"))) { virReportOOMError(); return -1; @@ -181,6 +186,36 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } } + p = virConfGetValue (conf, "vnc_port_min"); + CHECK_TYPE ("vnc_port_min", VIR_CONF_LONG); + if (p) { + if (p->l < QEMU_VNC_PORT_MIN) { + /* if the port is too low, we can't get the display name + * tell to vnc (usually subtract 5900, e.g. localhost:1 + * for port 5901) */ + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: vnc_port_min: port must be greater than or equal to %d"), + filename, QEMU_VNC_PORT_MIN); + virConfFree(conf); + return -1; + } + driver->vncPortMin = p->l; + } + + p = virConfGetValue (conf, "vnc_port_max"); + CHECK_TYPE ("vnc_port_max", VIR_CONF_LONG); + if (p) { + if (p->l > QEMU_VNC_PORT_MAX || + p->l <= driver->vncPortMin) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: vnc_port_max: port must be between the minimal port and %d"), + filename, QEMU_VNC_PORT_MAX); + virConfFree(conf); + return -1; + } + driver->vncPortMax = p->l; + } + p = virConfGetValue (conf, "vnc_password"); CHECK_TYPE ("vnc_password", VIR_CONF_STRING); if (p && p->str) { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 482e6d3..4081887 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -89,6 +89,8 @@ struct qemud_driver { unsigned int vncSASL : 1; char *vncTLSx509certdir; char *vncListen; + long vncPortMin; + long vncPortMax; char *vncPassword; char *vncSASLdir; unsigned int spiceTLS : 1; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 39b27b1..97ec9b9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -485,11 +485,6 @@ qemudStartup(int privileged) { if (!qemu_driver->domainEventState) goto error; - /* Allocate bitmap for vnc port reservation */ - if ((qemu_driver->reservedVNCPorts = - virBitmapAlloc(QEMU_VNC_PORT_MAX - QEMU_VNC_PORT_MIN)) == NULL) - goto out_of_memory; - /* read the host sysinfo */ if (privileged) qemu_driver->hostsysinfo = virSysinfoRead(); @@ -616,6 +611,13 @@ qemudStartup(int privileged) { } VIR_FREE(driverConf); + /* Allocate bitmap for vnc port reservation. We cannot do this + * before the config is loaded properly, since the port numbers + * are configurable now */ + if ((qemu_driver->reservedVNCPorts = + virBitmapAlloc(qemu_driver->vncPortMax - qemu_driver->vncPortMin)) == NULL) + goto out_of_memory; + /* We should always at least have the 'nop' manager, so * NULLs here are a fatal error */ @@ -4692,7 +4694,7 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, for (i = 0 ; i < def->ngraphics ; i++) { if (def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && def->graphics[i]->data.vnc.autoport) - def->graphics[i]->data.vnc.port = QEMU_VNC_PORT_MIN; + def->graphics[i]->data.vnc.port = driver->vncPortMin; } if (qemuCapsExtractVersionInfo(def->emulator, def->os.arch, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 58ba5bf..712ade1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2417,15 +2417,15 @@ static int qemuProcessNextFreePort(struct qemud_driver *driver, { int i; - for (i = startPort ; i < QEMU_VNC_PORT_MAX; i++) { + for (i = startPort ; i < driver->vncPortMax; i++) { int fd; int reuse = 1; struct sockaddr_in addr; bool used = false; if (virBitmapGetBit(driver->reservedVNCPorts, - i - QEMU_VNC_PORT_MIN, &used) < 0) - VIR_DEBUG("virBitmapGetBit failed on bit %d", i - QEMU_VNC_PORT_MIN); + i - driver->vncPortMin, &used) < 0) + VIR_DEBUG("virBitmapGetBit failed on bit %ld", i - driver->vncPortMin); if (used) continue; @@ -2447,9 +2447,9 @@ static int qemuProcessNextFreePort(struct qemud_driver *driver, VIR_FORCE_CLOSE(fd); /* Add port to bitmap of reserved ports */ if (virBitmapSetBit(driver->reservedVNCPorts, - i - QEMU_VNC_PORT_MIN) < 0) { - VIR_DEBUG("virBitmapSetBit failed on bit %d", - i - QEMU_VNC_PORT_MIN); + i - driver->vncPortMin) < 0) { + VIR_DEBUG("virBitmapSetBit failed on bit %ld", + i - driver->vncPortMin); } return i; } @@ -2470,11 +2470,11 @@ static void qemuProcessReturnPort(struct qemud_driver *driver, int port) { - if (port < QEMU_VNC_PORT_MIN) + if (port < driver->vncPortMin) return; if (virBitmapClearBit(driver->reservedVNCPorts, - port - QEMU_VNC_PORT_MIN) < 0) + port - driver->vncPortMin) < 0) VIR_DEBUG("Could not mark port %d as unused", port); } @@ -3349,7 +3349,7 @@ int qemuProcessStart(virConnectPtr conn, if (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && !vm->def->graphics[0]->data.vnc.socket && vm->def->graphics[0]->data.vnc.autoport) { - int port = qemuProcessNextFreePort(driver, QEMU_VNC_PORT_MIN); + int port = qemuProcessNextFreePort(driver, driver->vncPortMin); if (port < 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find an unused VNC port")); @@ -3360,7 +3360,7 @@ int qemuProcessStart(virConnectPtr conn, int port = -1; if (vm->def->graphics[0]->data.spice.autoport || vm->def->graphics[0]->data.spice.port == -1) { - port = qemuProcessNextFreePort(driver, QEMU_VNC_PORT_MIN); + port = qemuProcessNextFreePort(driver, driver->vncPortMin); if (port < 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, -- 1.7.8.6
On Tue, May 22, 2012 at 04:10:03PM +0200, Martin Kletzander wrote:
The defines QEMU_VNC_PORT_MIN and QEMU_VNC_PORT_MAX were used to find free port when starting domains. As this was hardcoded to the same ports as default VNC servers, there were races with these other programs. This patch includes the possibility to change the default starting port as well as the maximum port in qemu config file.
Hi Martin, Two design comments: 1) https://bugzilla.redhat.com/show_bug.cgi?id=782814 requests that the default port be changed to avoid conflicts, which seems reasonable to me. 2) I agree with the config option since most applications on the system will want the system defaults. However, IMO in this case an application writer should be given the option in the XML to override the system default. Dave
Support for two new config options in qemu.conf is added: * vnc_port_min (defaults to QEMU_VNC_PORT_MIN and must be >= than this value) * vnc_port_max (defaults to QEMU_VNC_PORT_MAX and must be <= than this value) --- src/qemu/qemu.conf | 7 +++++++ src/qemu/qemu_command.h | 3 +++ src/qemu/qemu_conf.c | 37 ++++++++++++++++++++++++++++++++++++- src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_driver.c | 14 ++++++++------ src/qemu/qemu_process.c | 20 ++++++++++---------- 6 files changed, 66 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index cb87728..e2f9011 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -11,6 +11,13 @@ # # vnc_listen = "0.0.0.0"
+# Override the port for creating VNC sessions (min) +# This defaults to 5900 and increases for consecutive sessions +# or when ports are occupied, until it hits the maximum. +# +#vnc_port_min = 5900 +#vnc_port_max = 65535 + # Enable this option to have VNC served over an automatically created # unix socket. This prevents unprivileged access from users on the # host machine, though most VNC clients do not support it. diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1eafeb3..f7d2884 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -37,6 +37,9 @@ # define QEMU_VIRTIO_SERIAL_PREFIX "virtio-serial" # define QEMU_FSDEV_HOST_PREFIX "fsdev-"
+/* These are only defaults, they can be changed now in qemu.conf and + * explicitely specified port is checked against these two (makes + * sense to limit the values) */ # define QEMU_VNC_PORT_MIN 5900 # define QEMU_VNC_PORT_MAX 65535
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 88a04bc..354d75d 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1,7 +1,7 @@ /* * qemu_conf.c: QEMU configuration management * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -38,6 +38,7 @@
#include "virterror_internal.h" #include "qemu_conf.h" +#include "qemu_command.h" #include "qemu_capabilities.h" #include "qemu_bridge_filter.h" #include "uuid.h" @@ -89,6 +90,10 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, virReportOOMError(); return -1; } + + driver->vncPortMin = QEMU_VNC_PORT_MIN; + driver->vncPortMax = QEMU_VNC_PORT_MAX; + if (!(driver->vncTLSx509certdir = strdup(SYSCONFDIR "/pki/libvirt-vnc"))) { virReportOOMError(); return -1; @@ -181,6 +186,36 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } }
+ p = virConfGetValue (conf, "vnc_port_min"); + CHECK_TYPE ("vnc_port_min", VIR_CONF_LONG); + if (p) { + if (p->l < QEMU_VNC_PORT_MIN) { + /* if the port is too low, we can't get the display name + * tell to vnc (usually subtract 5900, e.g. localhost:1 + * for port 5901) */ + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: vnc_port_min: port must be greater than or equal to %d"), + filename, QEMU_VNC_PORT_MIN); + virConfFree(conf); + return -1; + } + driver->vncPortMin = p->l; + } + + p = virConfGetValue (conf, "vnc_port_max"); + CHECK_TYPE ("vnc_port_max", VIR_CONF_LONG); + if (p) { + if (p->l > QEMU_VNC_PORT_MAX || + p->l <= driver->vncPortMin) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: vnc_port_max: port must be between the minimal port and %d"), + filename, QEMU_VNC_PORT_MAX); + virConfFree(conf); + return -1; + } + driver->vncPortMax = p->l; + } + p = virConfGetValue (conf, "vnc_password"); CHECK_TYPE ("vnc_password", VIR_CONF_STRING); if (p && p->str) { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 482e6d3..4081887 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -89,6 +89,8 @@ struct qemud_driver { unsigned int vncSASL : 1; char *vncTLSx509certdir; char *vncListen; + long vncPortMin; + long vncPortMax; char *vncPassword; char *vncSASLdir; unsigned int spiceTLS : 1; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 39b27b1..97ec9b9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -485,11 +485,6 @@ qemudStartup(int privileged) { if (!qemu_driver->domainEventState) goto error;
- /* Allocate bitmap for vnc port reservation */ - if ((qemu_driver->reservedVNCPorts = - virBitmapAlloc(QEMU_VNC_PORT_MAX - QEMU_VNC_PORT_MIN)) == NULL) - goto out_of_memory; - /* read the host sysinfo */ if (privileged) qemu_driver->hostsysinfo = virSysinfoRead(); @@ -616,6 +611,13 @@ qemudStartup(int privileged) { } VIR_FREE(driverConf);
+ /* Allocate bitmap for vnc port reservation. We cannot do this + * before the config is loaded properly, since the port numbers + * are configurable now */ + if ((qemu_driver->reservedVNCPorts = + virBitmapAlloc(qemu_driver->vncPortMax - qemu_driver->vncPortMin)) == NULL) + goto out_of_memory; + /* We should always at least have the 'nop' manager, so * NULLs here are a fatal error */ @@ -4692,7 +4694,7 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, for (i = 0 ; i < def->ngraphics ; i++) { if (def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && def->graphics[i]->data.vnc.autoport) - def->graphics[i]->data.vnc.port = QEMU_VNC_PORT_MIN; + def->graphics[i]->data.vnc.port = driver->vncPortMin; }
if (qemuCapsExtractVersionInfo(def->emulator, def->os.arch, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 58ba5bf..712ade1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2417,15 +2417,15 @@ static int qemuProcessNextFreePort(struct qemud_driver *driver, { int i;
- for (i = startPort ; i < QEMU_VNC_PORT_MAX; i++) { + for (i = startPort ; i < driver->vncPortMax; i++) { int fd; int reuse = 1; struct sockaddr_in addr; bool used = false;
if (virBitmapGetBit(driver->reservedVNCPorts, - i - QEMU_VNC_PORT_MIN, &used) < 0) - VIR_DEBUG("virBitmapGetBit failed on bit %d", i - QEMU_VNC_PORT_MIN); + i - driver->vncPortMin, &used) < 0) + VIR_DEBUG("virBitmapGetBit failed on bit %ld", i - driver->vncPortMin);
if (used) continue; @@ -2447,9 +2447,9 @@ static int qemuProcessNextFreePort(struct qemud_driver *driver, VIR_FORCE_CLOSE(fd); /* Add port to bitmap of reserved ports */ if (virBitmapSetBit(driver->reservedVNCPorts, - i - QEMU_VNC_PORT_MIN) < 0) { - VIR_DEBUG("virBitmapSetBit failed on bit %d", - i - QEMU_VNC_PORT_MIN); + i - driver->vncPortMin) < 0) { + VIR_DEBUG("virBitmapSetBit failed on bit %ld", + i - driver->vncPortMin); } return i; } @@ -2470,11 +2470,11 @@ static void qemuProcessReturnPort(struct qemud_driver *driver, int port) { - if (port < QEMU_VNC_PORT_MIN) + if (port < driver->vncPortMin) return;
if (virBitmapClearBit(driver->reservedVNCPorts, - port - QEMU_VNC_PORT_MIN) < 0) + port - driver->vncPortMin) < 0) VIR_DEBUG("Could not mark port %d as unused", port); }
@@ -3349,7 +3349,7 @@ int qemuProcessStart(virConnectPtr conn, if (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && !vm->def->graphics[0]->data.vnc.socket && vm->def->graphics[0]->data.vnc.autoport) { - int port = qemuProcessNextFreePort(driver, QEMU_VNC_PORT_MIN); + int port = qemuProcessNextFreePort(driver, driver->vncPortMin); if (port < 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find an unused VNC port")); @@ -3360,7 +3360,7 @@ int qemuProcessStart(virConnectPtr conn, int port = -1; if (vm->def->graphics[0]->data.spice.autoport || vm->def->graphics[0]->data.spice.port == -1) { - port = qemuProcessNextFreePort(driver, QEMU_VNC_PORT_MIN); + port = qemuProcessNextFreePort(driver, driver->vncPortMin);
if (port < 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, -- 1.7.8.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/22/2012 09:00 AM, Dave Allan wrote:
On Tue, May 22, 2012 at 04:10:03PM +0200, Martin Kletzander wrote:
The defines QEMU_VNC_PORT_MIN and QEMU_VNC_PORT_MAX were used to find free port when starting domains. As this was hardcoded to the same ports as default VNC servers, there were races with these other programs. This patch includes the possibility to change the default starting port as well as the maximum port in qemu config file.
Hi Martin,
Two design comments:
1) https://bugzilla.redhat.com/show_bug.cgi?id=782814 requests that the default port be changed to avoid conflicts, which seems reasonable to me.
If we choose better defaults for new installations, we still need to worry about preserving existing ranges when upgrading old installations. This may need some coordination with the spec file doing some %post magic to add in vnc_port_min with a value other than 5900 to qemu.conf on new installations, but leaving it unspecified when doing an upgrade.
2) I agree with the config option since most applications on the system will want the system defaults. However, IMO in this case an application writer should be given the option in the XML to override the system default.
Agreed - I think we need both solutions - qemu.conf to specify the default range, and per-domain XML to specify an override (does the XML need to specify a range, or just a single port?).
+++ b/src/qemu/qemu_command.h @@ -37,6 +37,9 @@ # define QEMU_VIRTIO_SERIAL_PREFIX "virtio-serial" # define QEMU_FSDEV_HOST_PREFIX "fsdev-"
+/* These are only defaults, they can be changed now in qemu.conf and + * explicitely specified port is checked against these two (makes
s/explicitely/explicitly/
+ p = virConfGetValue (conf, "vnc_port_min"); + CHECK_TYPE ("vnc_port_min", VIR_CONF_LONG); + if (p) { + if (p->l < QEMU_VNC_PORT_MIN) { + /* if the port is too low, we can't get the display name + * tell to vnc (usually subtract 5900, e.g. localhost:1 + * for port 5901) */ + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: vnc_port_min: port must be greater than or equal to %d"), + filename, QEMU_VNC_PORT_MIN);
You ought to mention this restriction in qemu.conf.
+ p = virConfGetValue (conf, "vnc_port_max"); + CHECK_TYPE ("vnc_port_max", VIR_CONF_LONG); + if (p) { + if (p->l > QEMU_VNC_PORT_MAX || + p->l <= driver->vncPortMin) {
Off-by-one. You want to allow min==max for specifying a range of exactly 1 port, useful if you are only running one guest and want to guarantee its port. You need p->l < driver->vncPortMin, not p->l <= driver->vncPortMin.
+++ b/src/qemu/qemu_conf.h @@ -89,6 +89,8 @@ struct qemud_driver { unsigned int vncSASL : 1; char *vncTLSx509certdir; char *vncListen; + long vncPortMin; + long vncPortMax;
long? We know it's only 16 bits; short would do, int might be easier to work with, but long is overkill. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, May 22, 2012 at 09:09:55AM -0600, Eric Blake wrote:
On 05/22/2012 09:00 AM, Dave Allan wrote:
On Tue, May 22, 2012 at 04:10:03PM +0200, Martin Kletzander wrote:
The defines QEMU_VNC_PORT_MIN and QEMU_VNC_PORT_MAX were used to find free port when starting domains. As this was hardcoded to the same ports as default VNC servers, there were races with these other programs. This patch includes the possibility to change the default starting port as well as the maximum port in qemu config file.
Hi Martin,
Two design comments:
1) https://bugzilla.redhat.com/show_bug.cgi?id=782814 requests that the default port be changed to avoid conflicts, which seems reasonable to me.
If we choose better defaults for new installations, we still need to worry about preserving existing ranges when upgrading old installations. This may need some coordination with the spec file doing some %post magic to add in vnc_port_min with a value other than 5900 to qemu.conf on new installations, but leaving it unspecified when doing an upgrade.
That's a good point, we certainly don't want to break things on upgrade. At least one case that would is where people have opened the old range in a firewall, and now instead of ports, say, 5900-n, now people will be getting some other range.
2) I agree with the config option since most applications on the system will want the system defaults. However, IMO in this case an application writer should be given the option in the XML to override the system default.
Agreed - I think we need both solutions - qemu.conf to specify the default range, and per-domain XML to specify an override (does the XML need to specify a range, or just a single port?).
I think a range, like the config option.
+++ b/src/qemu/qemu_command.h @@ -37,6 +37,9 @@ # define QEMU_VIRTIO_SERIAL_PREFIX "virtio-serial" # define QEMU_FSDEV_HOST_PREFIX "fsdev-"
+/* These are only defaults, they can be changed now in qemu.conf and + * explicitely specified port is checked against these two (makes
s/explicitely/explicitly/
+ p = virConfGetValue (conf, "vnc_port_min"); + CHECK_TYPE ("vnc_port_min", VIR_CONF_LONG); + if (p) { + if (p->l < QEMU_VNC_PORT_MIN) { + /* if the port is too low, we can't get the display name + * tell to vnc (usually subtract 5900, e.g. localhost:1 + * for port 5901) */ + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: vnc_port_min: port must be greater than or equal to %d"), + filename, QEMU_VNC_PORT_MIN);
You ought to mention this restriction in qemu.conf.
+ p = virConfGetValue (conf, "vnc_port_max"); + CHECK_TYPE ("vnc_port_max", VIR_CONF_LONG); + if (p) { + if (p->l > QEMU_VNC_PORT_MAX || + p->l <= driver->vncPortMin) {
Off-by-one. You want to allow min==max for specifying a range of exactly 1 port, useful if you are only running one guest and want to guarantee its port. You need p->l < driver->vncPortMin, not p->l <= driver->vncPortMin.
+++ b/src/qemu/qemu_conf.h @@ -89,6 +89,8 @@ struct qemud_driver { unsigned int vncSASL : 1; char *vncTLSx509certdir; char *vncListen; + long vncPortMin; + long vncPortMax;
long? We know it's only 16 bits; short would do, int might be easier to work with, but long is overkill.
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/22/2012 07:22 PM, Dave Allan wrote:
On Tue, May 22, 2012 at 09:09:55AM -0600, Eric Blake wrote:
On 05/22/2012 09:00 AM, Dave Allan wrote:
On Tue, May 22, 2012 at 04:10:03PM +0200, Martin Kletzander wrote:
The defines QEMU_VNC_PORT_MIN and QEMU_VNC_PORT_MAX were used to find free port when starting domains. As this was hardcoded to the same ports as default VNC servers, there were races with these other programs. This patch includes the possibility to change the default starting port as well as the maximum port in qemu config file.
Hi Martin,
Two design comments:
1) https://bugzilla.redhat.com/show_bug.cgi?id=782814 requests that the default port be changed to avoid conflicts, which seems reasonable to me.
If we choose better defaults for new installations, we still need to worry about preserving existing ranges when upgrading old installations. This may need some coordination with the spec file doing some %post magic to add in vnc_port_min with a value other than 5900 to qemu.conf on new installations, but leaving it unspecified when doing an upgrade.
That's a good point, we certainly don't want to break things on upgrade. At least one case that would is where people have opened the old range in a firewall, and now instead of ports, say, 5900-n, now people will be getting some other range.
Yes, I agree, that's why I kept the defaults unchanged. But wouldn't it be confusing for lot of users if we changed the defaults for new installations? Because from user point of view, I think it would cause misunderstanding "why on _some_ installations it uses different ports?". Maybe I misunderstood what they meant in the BZ, but I though of the default port as "the port the we start with when domain has specified port -1", so they don't have to change it for all of the domains, but just in one config file.
2) I agree with the config option since most applications on the system will want the system defaults. However, IMO in this case an application writer should be given the option in the XML to override the system default.
Agreed - I think we need both solutions - qemu.conf to specify the default range, and per-domain XML to specify an override (does the XML need to specify a range, or just a single port?).
I think a range, like the config option.
This is maybe another misunderstanding on my side, but I believed that when user specifies a port in the domain XML (e.g. port='12345'), he can still configure autoport='yes', thus keeping the searching mechanism working the same way as with the port='-1' option, just changing the starting port. But I'm not sure about that, I have to look into this now. However, I see two possibilities that seem reasonable: 1) if autoport doesn't work as I thought, I can make it work as explained before, I think it's pretty intuitive to make it like that 2) independently on how the autoport works, I can make the range configurable in the <listen> subelement or as an another option in the <graphics> element (say: port_max, for example), as Dave mentioned.
+++ b/src/qemu/qemu_command.h @@ -37,6 +37,9 @@ # define QEMU_VIRTIO_SERIAL_PREFIX "virtio-serial" # define QEMU_FSDEV_HOST_PREFIX "fsdev-"
+/* These are only defaults, they can be changed now in qemu.conf and + * explicitely specified port is checked against these two (makes
s/explicitely/explicitly/
+ p = virConfGetValue (conf, "vnc_port_min"); + CHECK_TYPE ("vnc_port_min", VIR_CONF_LONG); + if (p) { + if (p->l < QEMU_VNC_PORT_MIN) { + /* if the port is too low, we can't get the display name + * tell to vnc (usually subtract 5900, e.g. localhost:1 + * for port 5901) */ + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: vnc_port_min: port must be greater than or equal to %d"), + filename, QEMU_VNC_PORT_MIN);
You ought to mention this restriction in qemu.conf.
Yes, I'll do that, to explain there as well.
+ p = virConfGetValue (conf, "vnc_port_max"); + CHECK_TYPE ("vnc_port_max", VIR_CONF_LONG); + if (p) { + if (p->l > QEMU_VNC_PORT_MAX || + p->l <= driver->vncPortMin) {
Off-by-one. You want to allow min==max for specifying a range of exactly 1 port, useful if you are only running one guest and want to guarantee its port. You need p->l < driver->vncPortMin, not p->l <= driver->vncPortMin.
Actually, in case we specify min==max, there are few places where it will fail, because we use it as (i = min; i < max; i++), but I'll change that, because your version makes way more sense.
+++ b/src/qemu/qemu_conf.h @@ -89,6 +89,8 @@ struct qemud_driver { unsigned int vncSASL : 1; char *vncTLSx509certdir; char *vncListen; + long vncPortMin; + long vncPortMax;
long? We know it's only 16 bits; short would do, int might be easier to work with, but long is overkill.
OK, will switch that, I think I just wanted to keep it consistent with the CHECK_TYPE, but that's unnecessary (or I don't know why I did that). Martin
On Tue, May 22, 2012 at 01:22:16PM -0400, Dave Allan wrote:
On Tue, May 22, 2012 at 09:09:55AM -0600, Eric Blake wrote:
On 05/22/2012 09:00 AM, Dave Allan wrote:
On Tue, May 22, 2012 at 04:10:03PM +0200, Martin Kletzander wrote:
The defines QEMU_VNC_PORT_MIN and QEMU_VNC_PORT_MAX were used to find free port when starting domains. As this was hardcoded to the same ports as default VNC servers, there were races with these other programs. This patch includes the possibility to change the default starting port as well as the maximum port in qemu config file.
Hi Martin,
Two design comments:
1) https://bugzilla.redhat.com/show_bug.cgi?id=782814 requests that the default port be changed to avoid conflicts, which seems reasonable to me.
If we choose better defaults for new installations, we still need to worry about preserving existing ranges when upgrading old installations. This may need some coordination with the spec file doing some %post magic to add in vnc_port_min with a value other than 5900 to qemu.conf on new installations, but leaving it unspecified when doing an upgrade.
That's a good point, we certainly don't want to break things on upgrade. At least one case that would is where people have opened the old range in a firewall, and now instead of ports, say, 5900-n, now people will be getting some other range.
2) I agree with the config option since most applications on the system will want the system defaults. However, IMO in this case an application writer should be given the option in the XML to override the system default.
Agreed - I think we need both solutions - qemu.conf to specify the default range, and per-domain XML to specify an override (does the XML need to specify a range, or just a single port?).
I think a range, like the config option.
I think this is unneccessary configurability. A qemu config option is sufficient. I just don't see any application wanting to set different default ranges per guest. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, May 22, 2012 at 11:00:16AM -0400, Dave Allan wrote:
On Tue, May 22, 2012 at 04:10:03PM +0200, Martin Kletzander wrote:
The defines QEMU_VNC_PORT_MIN and QEMU_VNC_PORT_MAX were used to find free port when starting domains. As this was hardcoded to the same ports as default VNC servers, there were races with these other programs. This patch includes the possibility to change the default starting port as well as the maximum port in qemu config file.
Hi Martin,
Two design comments:
1) https://bugzilla.redhat.com/show_bug.cgi?id=782814 requests that the default port be changed to avoid conflicts, which seems reasonable to me.
I disagree - for as long as KVM and Xen have existed, all guests have had ports starting at 5900 & we should not be changing that. libvirt should be checking for a clash before starting a guest, so there should not be any functional problem here. Moving to another port range is merely a nicety that we don't need todo by default, merely give admins the ability.
2) I agree with the config option since most applications on the system will want the system defaults. However, IMO in this case an application writer should be given the option in the XML to override the system default.
I don't really think this is worth while either. I don't really think that apps need / want the ability to override the system ranges here on a per guest basis Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, May 23, 2012 at 10:31:12AM +0100, Daniel P. Berrange wrote:
On Tue, May 22, 2012 at 11:00:16AM -0400, Dave Allan wrote:
On Tue, May 22, 2012 at 04:10:03PM +0200, Martin Kletzander wrote:
The defines QEMU_VNC_PORT_MIN and QEMU_VNC_PORT_MAX were used to find free port when starting domains. As this was hardcoded to the same ports as default VNC servers, there were races with these other programs. This patch includes the possibility to change the default starting port as well as the maximum port in qemu config file.
Hi Martin,
Two design comments:
1) https://bugzilla.redhat.com/show_bug.cgi?id=782814 requests that the default port be changed to avoid conflicts, which seems reasonable to me.
I disagree - for as long as KVM and Xen have existed, all guests have had ports starting at 5900 & we should not be changing that. libvirt should be checking for a clash before starting a guest, so there should not be any functional problem here.
I'm not sure how libvirt does this today, but passing the port in the command line is inherently raceful. Either pass the open socket to qemu, or let qemu choose the port by itself.
Moving to another port range is merely a nicety that we don't need todo by default, merely give admins the ability.
2) I agree with the config option since most applications on the system will want the system defaults. However, IMO in this case an application writer should be given the option in the XML to override the system default.
I don't really think this is worth while either. I don't really think that apps need / want the ability to override the system ranges here on a per guest basis
Yep, that's true for vdsm too (within ovirt). I can tell why host-wide config is useful, but not why the range should be VM-specific. Regards, Dan.
On Mon, May 28, 2012 at 03:18:33PM +0300, Dan Kenigsberg wrote:
On Wed, May 23, 2012 at 10:31:12AM +0100, Daniel P. Berrange wrote:
On Tue, May 22, 2012 at 11:00:16AM -0400, Dave Allan wrote:
On Tue, May 22, 2012 at 04:10:03PM +0200, Martin Kletzander wrote:
The defines QEMU_VNC_PORT_MIN and QEMU_VNC_PORT_MAX were used to find free port when starting domains. As this was hardcoded to the same ports as default VNC servers, there were races with these other programs. This patch includes the possibility to change the default starting port as well as the maximum port in qemu config file.
Hi Martin,
Two design comments:
1) https://bugzilla.redhat.com/show_bug.cgi?id=782814 requests that the default port be changed to avoid conflicts, which seems reasonable to me.
I disagree - for as long as KVM and Xen have existed, all guests have had ports starting at 5900 & we should not be changing that. libvirt should be checking for a clash before starting a guest, so there should not be any functional problem here.
I'm not sure how libvirt does this today, but passing the port in the command line is inherently raceful. Either pass the open socket to qemu, or let qemu choose the port by itself.
There is no race wrt starting multiple VMs, since between the time libvirt allocates the VNC port, and starts QEMU, it holds the global driver lock. This prevents any other VM start attempts racing for port numbers. You do have a tiny race wrt starting VMs vs end user starting plain VNC servers, but this is really tiny & for a RHEV-H style VDSM deployment non-existant. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, May 22, 2012 at 04:10:03PM +0200, Martin Kletzander wrote:
The defines QEMU_VNC_PORT_MIN and QEMU_VNC_PORT_MAX were used to find free port when starting domains. As this was hardcoded to the same ports as default VNC servers, there were races with these other programs. This patch includes the possibility to change the default starting port as well as the maximum port in qemu config file.
Support for two new config options in qemu.conf is added: * vnc_port_min (defaults to QEMU_VNC_PORT_MIN and must be >= than this value) * vnc_port_max (defaults to QEMU_VNC_PORT_MAX and must be <= than this value) --- src/qemu/qemu.conf | 7 +++++++ src/qemu/qemu_command.h | 3 +++ src/qemu/qemu_conf.c | 37 ++++++++++++++++++++++++++++++++++++- src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_driver.c | 14 ++++++++------ src/qemu/qemu_process.c | 20 ++++++++++---------- 6 files changed, 66 insertions(+), 17 deletions(-)
NB, this is incomplete - you need to add the same configuration support for SPICE port numbers. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (5)
-
Dan Kenigsberg -
Daniel P. Berrange -
Dave Allan -
Eric Blake -
Martin Kletzander