[libvirt] [PATCH v3 0/4] qemu: configurable port boundaries for remote displays

This series introduces a possibility to change default minimal and maximal port numbers that are used to specify a remote display port for both VNC and SPICE. Because the code was a bit messy, PATCH 1/4 cleans up few things needed to make a clean run of PATCH 2/4, that does the main change in the code. I also noticed two things that could be changed and it made sense for me to do that, but these are nowhere near any importance, so feel free to reject them if your heart feels that way. PATCH 3/4 [optional] rewords three messages and applying it would mean that they are not translated. Even though I think it makes more sense this way, I'm not a good English speaker, so that's more like an RFC. PATCH 4/4 [optional] makes more flexible port searching available, but now it is used just on one place and it's not necessary to have it. -- v3: - rebase on current HEAD v2: - apply the change to both VNC and SPICE sessions Martin Kletzander (4): qemu: Unify port-wise SPICE and VNC behavior qemu: configurable remote display port boundaries qemu: modify 3 error messages qemu: allow searching for all open ports src/conf/domain_conf.c | 2 +- src/qemu/libvirtd_qemu.aug | 4 ++ src/qemu/qemu.conf | 14 ++++++ src/qemu/qemu_command.h | 11 ++++- src/qemu/qemu_conf.c | 48 +++++++++++++++++++++- src/qemu/qemu_conf.h | 4 +- src/qemu/qemu_driver.c | 19 ++++----- src/qemu/qemu_process.c | 80 +++++++++++++++++++++-------------- src/qemu/test_libvirtd_qemu.aug.in | 2 + 9 files changed, 136 insertions(+), 48 deletions(-) -- 1.7.8.6

Port allocations for SPICE and VNC behave almost the same (with default ports), but there is some mess in the code. This patch clears these inconsistencies and makes sure the same behavior will be used when ports for remote displays are changed. Changes: - hard-coded number 5900 removed (handled elsewhere like with VNC) - reservedVNCPorts renamed to reservedRemotePorts (it's not just for VNC anymore) - QEMU_VNC_PORT_{MIN,MAX} renamed to QEMU_REMOTE_PORT_{MIN,MAX} - port allocation unified for VNC and SPICE --- src/conf/domain_conf.c | 2 +- src/qemu/qemu_command.h | 4 +- src/qemu/qemu_conf.h | 2 +- src/qemu/qemu_driver.c | 11 ++----- src/qemu/qemu_process.c | 66 +++++++++++++++++++++++++--------------------- 5 files changed, 43 insertions(+), 42 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d8c0969..24e4860 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6244,7 +6244,7 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, } VIR_FREE(port); } else { - def->data.spice.port = 5900; + def->data.spice.port = 0; } tlsPort = virXMLPropString(node, "tlsPort"); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index e999bc7..14afd9b 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -37,8 +37,8 @@ # define QEMU_VIRTIO_SERIAL_PREFIX "virtio-serial" # define QEMU_FSDEV_HOST_PREFIX "fsdev-" -# define QEMU_VNC_PORT_MIN 5900 -# define QEMU_VNC_PORT_MAX 65535 +# define QEMU_REMOTE_PORT_MIN 5900 +# define QEMU_REMOTE_PORT_MAX 65535 virCommandPtr qemuBuildCommandLine(virConnectPtr conn, diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 92e4968..3895889 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -135,7 +135,7 @@ struct qemud_driver { /* The devices which is are not in use by the host or any guest. */ pciDeviceList *inactivePciHostdevs; - virBitmapPtr reservedVNCPorts; + virBitmapPtr reservedRemotePorts; virSysinfoDefPtr hostsysinfo; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9bf89bb..0d886c1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -521,8 +521,8 @@ qemudStartup(int privileged) { goto error; /* Allocate bitmap for vnc port reservation */ - if ((qemu_driver->reservedVNCPorts = - virBitmapAlloc(QEMU_VNC_PORT_MAX - QEMU_VNC_PORT_MIN)) == NULL) + if ((qemu_driver->reservedRemotePorts = + virBitmapAlloc(QEMU_REMOTE_PORT_MAX - QEMU_REMOTE_PORT_MIN)) == NULL) goto out_of_memory; /* read the host sysinfo */ @@ -880,7 +880,7 @@ qemudShutdown(void) { virCapabilitiesFree(qemu_driver->caps); virDomainObjListDeinit(&qemu_driver->domains); - virBitmapFree(qemu_driver->reservedVNCPorts); + virBitmapFree(qemu_driver->reservedRemotePorts); virSysinfoDefFree(qemu_driver->hostsysinfo); @@ -4785,11 +4785,6 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, } net->info.bootIndex = bootIndex; } - 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; - } if (qemuCapsExtractVersionInfo(def->emulator, def->os.arch, false, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index df4a016..4c40b49 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2454,15 +2454,15 @@ static int qemuProcessNextFreePort(struct qemud_driver *driver, { int i; - for (i = startPort ; i < QEMU_VNC_PORT_MAX; i++) { + for (i = startPort ; i < QEMU_REMOTE_PORT_MAX; 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); + if (virBitmapGetBit(driver->reservedRemotePorts, + i - QEMU_REMOTE_PORT_MIN, &used) < 0) + VIR_DEBUG("virBitmapGetBit failed on bit %d", i - QEMU_REMOTE_PORT_MIN); if (used) continue; @@ -2483,10 +2483,10 @@ static int qemuProcessNextFreePort(struct qemud_driver *driver, /* Not in use, lets grab it */ VIR_FORCE_CLOSE(fd); /* Add port to bitmap of reserved ports */ - if (virBitmapSetBit(driver->reservedVNCPorts, - i - QEMU_VNC_PORT_MIN) < 0) { + if (virBitmapSetBit(driver->reservedRemotePorts, + i - QEMU_REMOTE_PORT_MIN) < 0) { VIR_DEBUG("virBitmapSetBit failed on bit %d", - i - QEMU_VNC_PORT_MIN); + i - QEMU_REMOTE_PORT_MIN); } return i; } @@ -2507,11 +2507,11 @@ static void qemuProcessReturnPort(struct qemud_driver *driver, int port) { - if (port < QEMU_VNC_PORT_MIN) + if (port < QEMU_REMOTE_PORT_MIN) return; - if (virBitmapClearBit(driver->reservedVNCPorts, - port - QEMU_VNC_PORT_MIN) < 0) + if (virBitmapClearBit(driver->reservedRemotePorts, + port - QEMU_REMOTE_PORT_MIN) < 0) VIR_DEBUG("Could not mark port %d as unused", port); } @@ -3417,41 +3417,47 @@ 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; + if (vm->def->graphics[0]->data.vnc.port > 0) + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.vnc.port); + else + port = qemuProcessNextFreePort(driver, QEMU_REMOTE_PORT_MIN); + if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find an unused VNC port")); goto cleanup; } vm->def->graphics[0]->data.vnc.port = port; - } else if (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { - 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); - if (port < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to find an unused SPICE port")); - goto cleanup; - } + } else if (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE && + vm->def->graphics[0]->data.spice.autoport) { + int port; + if (vm->def->graphics[0]->data.spice.port > 0) + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.port); + else + port = qemuProcessNextFreePort(driver, QEMU_REMOTE_PORT_MIN); - vm->def->graphics[0]->data.spice.port = port; + if (port < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Unable to find an unused SPICE port")); + goto cleanup; } + vm->def->graphics[0]->data.spice.port = port; - if (driver->spiceTLS && - (vm->def->graphics[0]->data.spice.autoport || - vm->def->graphics[0]->data.spice.tlsPort == -1)) { - int tlsPort = qemuProcessNextFreePort(driver, - vm->def->graphics[0]->data.spice.port + 1); - if (tlsPort < 0) { + if (driver->spiceTLS) { + if (vm->def->graphics[0]->data.spice.tlsPort > 0) + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.tlsPort); + else + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.port + 1); + if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to find an unused SPICE TLS port")); + "%s", _("Unable to find an unused SPICE TLS port")); qemuProcessReturnPort(driver, port); goto cleanup; } - vm->def->graphics[0]->data.spice.tlsPort = tlsPort; + vm->def->graphics[0]->data.spice.tlsPort = port; } } -- 1.7.8.6

On Mon, Aug 13, 2012 at 03:21:22PM +0200, Martin Kletzander wrote:
Port allocations for SPICE and VNC behave almost the same (with default ports), but there is some mess in the code. This patch clears these inconsistencies and makes sure the same behavior will be used when ports for remote displays are changed.
Changes: - hard-coded number 5900 removed (handled elsewhere like with VNC) - reservedVNCPorts renamed to reservedRemotePorts (it's not just for VNC anymore) - QEMU_VNC_PORT_{MIN,MAX} renamed to QEMU_REMOTE_PORT_{MIN,MAX} - port allocation unified for VNC and SPICE --- @@ -3417,41 +3417,47 @@ 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; + if (vm->def->graphics[0]->data.vnc.port > 0) + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.vnc.port); + else + port = qemuProcessNextFreePort(driver, QEMU_REMOTE_PORT_MIN); +
If 'autoport' is set, then 'vnc.port' should be completely ignored. This is changing that behaviour.
+ } else if (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE && + vm->def->graphics[0]->data.spice.autoport) { + int port; + if (vm->def->graphics[0]->data.spice.port > 0) + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.port); + else + port = qemuProcessNextFreePort(driver, QEMU_REMOTE_PORT_MIN);
And again.
+ if (driver->spiceTLS) { + if (vm->def->graphics[0]->data.spice.tlsPort > 0) + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.tlsPort); + else + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.port + 1);
And again. 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 08/14/2012 11:42 AM, Daniel P. Berrange wrote:
On Mon, Aug 13, 2012 at 03:21:22PM +0200, Martin Kletzander wrote:
Port allocations for SPICE and VNC behave almost the same (with default ports), but there is some mess in the code. This patch clears these inconsistencies and makes sure the same behavior will be used when ports for remote displays are changed.
Changes: - hard-coded number 5900 removed (handled elsewhere like with VNC) - reservedVNCPorts renamed to reservedRemotePorts (it's not just for VNC anymore) - QEMU_VNC_PORT_{MIN,MAX} renamed to QEMU_REMOTE_PORT_{MIN,MAX} - port allocation unified for VNC and SPICE --- @@ -3417,41 +3417,47 @@ 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; + if (vm->def->graphics[0]->data.vnc.port > 0) + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.vnc.port); + else + port = qemuProcessNextFreePort(driver, QEMU_REMOTE_PORT_MIN); +
If 'autoport' is set, then 'vnc.port' should be completely ignored. This is changing that behaviour.
Since this wasn't mentioned anywhere, I figured this might be helpful to someone, but I've got no problem keeping it that way. Will post v2 then along with the fixes in PATCH 4/4. Martin

Port allocations for SPICE and VNC behave almost the same (with default ports), but there is some mess in the code. This patch clears these inconsistencies and makes sure the same behavior will be used when ports for remote displays are changed. Changes: - hard-coded number 5900 removed (handled elsewhere like with VNC) - reservedVNCPorts renamed to reservedRemotePorts (it's not just for VNC anymore) - QEMU_VNC_PORT_{MIN,MAX} renamed to QEMU_REMOTE_PORT_{MIN,MAX} - port allocation unified for VNC and SPICE --- src/conf/domain_conf.c | 2 +- src/qemu/qemu_command.h | 4 ++-- src/qemu/qemu_conf.h | 2 +- src/qemu/qemu_driver.c | 11 +++-------- src/qemu/qemu_process.c | 27 +++++++++++++-------------- 5 files changed, 20 insertions(+), 26 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f60a953..3d11e1c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6243,7 +6243,7 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, } VIR_FREE(port); } else { - def->data.spice.port = 5900; + def->data.spice.port = 0; } tlsPort = virXMLPropString(node, "tlsPort"); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index e999bc7..14afd9b 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -37,8 +37,8 @@ # define QEMU_VIRTIO_SERIAL_PREFIX "virtio-serial" # define QEMU_FSDEV_HOST_PREFIX "fsdev-" -# define QEMU_VNC_PORT_MIN 5900 -# define QEMU_VNC_PORT_MAX 65535 +# define QEMU_REMOTE_PORT_MIN 5900 +# define QEMU_REMOTE_PORT_MAX 65535 virCommandPtr qemuBuildCommandLine(virConnectPtr conn, diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 92e4968..3895889 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -135,7 +135,7 @@ struct qemud_driver { /* The devices which is are not in use by the host or any guest. */ pciDeviceList *inactivePciHostdevs; - virBitmapPtr reservedVNCPorts; + virBitmapPtr reservedRemotePorts; virSysinfoDefPtr hostsysinfo; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8b12fcf..cc37c64 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -520,8 +520,8 @@ qemudStartup(int privileged) { goto error; /* Allocate bitmap for vnc port reservation */ - if ((qemu_driver->reservedVNCPorts = - virBitmapAlloc(QEMU_VNC_PORT_MAX - QEMU_VNC_PORT_MIN)) == NULL) + if ((qemu_driver->reservedRemotePorts = + virBitmapAlloc(QEMU_REMOTE_PORT_MAX - QEMU_REMOTE_PORT_MIN)) == NULL) goto out_of_memory; /* read the host sysinfo */ @@ -879,7 +879,7 @@ qemudShutdown(void) { virCapabilitiesFree(qemu_driver->caps); virDomainObjListDeinit(&qemu_driver->domains); - virBitmapFree(qemu_driver->reservedVNCPorts); + virBitmapFree(qemu_driver->reservedRemotePorts); virSysinfoDefFree(qemu_driver->hostsysinfo); @@ -4784,11 +4784,6 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, } net->info.bootIndex = bootIndex; } - 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; - } if (qemuCapsExtractVersionInfo(def->emulator, def->os.arch, false, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index df4a016..39365af 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2454,15 +2454,15 @@ static int qemuProcessNextFreePort(struct qemud_driver *driver, { int i; - for (i = startPort ; i < QEMU_VNC_PORT_MAX; i++) { + for (i = startPort ; i < QEMU_REMOTE_PORT_MAX; 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); + if (virBitmapGetBit(driver->reservedRemotePorts, + i - QEMU_REMOTE_PORT_MIN, &used) < 0) + VIR_DEBUG("virBitmapGetBit failed on bit %d", i - QEMU_REMOTE_PORT_MIN); if (used) continue; @@ -2483,10 +2483,10 @@ static int qemuProcessNextFreePort(struct qemud_driver *driver, /* Not in use, lets grab it */ VIR_FORCE_CLOSE(fd); /* Add port to bitmap of reserved ports */ - if (virBitmapSetBit(driver->reservedVNCPorts, - i - QEMU_VNC_PORT_MIN) < 0) { + if (virBitmapSetBit(driver->reservedRemotePorts, + i - QEMU_REMOTE_PORT_MIN) < 0) { VIR_DEBUG("virBitmapSetBit failed on bit %d", - i - QEMU_VNC_PORT_MIN); + i - QEMU_REMOTE_PORT_MIN); } return i; } @@ -2507,11 +2507,11 @@ static void qemuProcessReturnPort(struct qemud_driver *driver, int port) { - if (port < QEMU_VNC_PORT_MIN) + if (port < QEMU_REMOTE_PORT_MIN) return; - if (virBitmapClearBit(driver->reservedVNCPorts, - port - QEMU_VNC_PORT_MIN) < 0) + if (virBitmapClearBit(driver->reservedRemotePorts, + port - QEMU_REMOTE_PORT_MIN) < 0) VIR_DEBUG("Could not mark port %d as unused", port); } @@ -3417,7 +3417,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, QEMU_REMOTE_PORT_MIN); if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find an unused VNC port")); @@ -3428,7 +3428,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, QEMU_REMOTE_PORT_MIN); if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -3438,7 +3438,6 @@ int qemuProcessStart(virConnectPtr conn, vm->def->graphics[0]->data.spice.port = port; } - if (driver->spiceTLS && (vm->def->graphics[0]->data.spice.autoport || vm->def->graphics[0]->data.spice.tlsPort == -1)) { @@ -3451,7 +3450,7 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; } - vm->def->graphics[0]->data.spice.tlsPort = tlsPort; + vm->def->graphics[0]->data.spice.tlsPort = port; } } -- 1.7.8.6

On Tue, Aug 14, 2012 at 02:52:19PM +0200, Martin Kletzander wrote:
Port allocations for SPICE and VNC behave almost the same (with default ports), but there is some mess in the code. This patch clears these inconsistencies and makes sure the same behavior will be used when ports for remote displays are changed.
Changes: - hard-coded number 5900 removed (handled elsewhere like with VNC) - reservedVNCPorts renamed to reservedRemotePorts (it's not just for VNC anymore) - QEMU_VNC_PORT_{MIN,MAX} renamed to QEMU_REMOTE_PORT_{MIN,MAX} - port allocation unified for VNC and SPICE --- src/conf/domain_conf.c | 2 +- src/qemu/qemu_command.h | 4 ++-- src/qemu/qemu_conf.h | 2 +- src/qemu/qemu_driver.c | 11 +++-------- src/qemu/qemu_process.c | 27 +++++++++++++-------------- 5 files changed, 20 insertions(+), 26 deletions(-)
ACK 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 08/21/2012 11:36 AM, Daniel P. Berrange wrote:
On Tue, Aug 14, 2012 at 02:52:19PM +0200, Martin Kletzander wrote:
Port allocations for SPICE and VNC behave almost the same (with default ports), but there is some mess in the code. This patch clears these inconsistencies and makes sure the same behavior will be used when ports for remote displays are changed.
Changes: - hard-coded number 5900 removed (handled elsewhere like with VNC) - reservedVNCPorts renamed to reservedRemotePorts (it's not just for VNC anymore) - QEMU_VNC_PORT_{MIN,MAX} renamed to QEMU_REMOTE_PORT_{MIN,MAX} - port allocation unified for VNC and SPICE --- src/conf/domain_conf.c | 2 +- src/qemu/qemu_command.h | 4 ++-- src/qemu/qemu_conf.h | 2 +- src/qemu/qemu_driver.c | 11 +++-------- src/qemu/qemu_process.c | 27 +++++++++++++-------------- 5 files changed, 20 insertions(+), 26 deletions(-)
ACK
Daniel
Thanks, pushed the series without the optional 4/4 patch. Martin

The defines QEMU_REMOTE_PORT_MIN and QEMU_REMOTE_PORT_MAX were used to find free port when starting domains. As this was hard-coded 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 (mostly for completeness) in qemu config file. Support for two new config options in qemu.conf is added: - remote_port_min (defaults to QEMU_REMOTE_PORT_MIN and must be >= than this value) - remote_port_max (defaults to QEMU_REMOTE_PORT_MAX and must be <= than this value) --- src/qemu/libvirtd_qemu.aug | 4 +++ src/qemu/qemu.conf | 14 ++++++++++ src/qemu/qemu_command.h | 7 +++++ src/qemu/qemu_conf.c | 48 +++++++++++++++++++++++++++++++++++- src/qemu/qemu_conf.h | 2 + src/qemu/qemu_driver.c | 12 +++++--- src/qemu/qemu_process.c | 14 +++++----- src/qemu/test_libvirtd_qemu.aug.in | 2 + 8 files changed, 90 insertions(+), 13 deletions(-) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 683aadb..b95d751 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -39,6 +39,9 @@ module Libvirtd_qemu = | str_entry "spice_tls_x509_cert_dir" | str_entry "spice_password" + let remote_display_entry = int_entry "remote_display_port_min" + | int_entry "remote_display_port_max" + let security_entry = str_entry "security_driver" | bool_entry "security_default_confined" | bool_entry "security_require_confined" @@ -72,6 +75,7 @@ module Libvirtd_qemu = (* Each enty in the config is one of the following three ... *) let entry = vnc_entry | spice_entry + | remote_display_entry | security_entry | save_entry | process_entry diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index ed4683c..bfdf351 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -140,6 +140,20 @@ #spice_password = "XYZ12345" +# Override the port for creating both VNC and SPICE sessions (min). +# This defaults to 5900 and increases for consecutive sessions +# or when ports are occupied, until it hits the maximum. +# +# Minimum must be greater than or equal to 5900 as lower number would +# result into negative vnc display number. +# +# Maximum must be less than 65536, because higher numbers do not make +# sense as a port number. +# +#remote_display_port_min = 5900 +#remote_display_port_max = 65535 + + # The default security driver is SELinux. If SELinux is disabled # on the host, then the security driver will automatically disable # itself. If you wish to disable QEMU SELinux security driver while diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 14afd9b..7c5e8dd 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -37,6 +37,13 @@ # 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). + * + * This limitation is mentioned in qemu.conf, so bear in mind that the + * configuration file should reflect any changes made to these values. + */ # define QEMU_REMOTE_PORT_MIN 5900 # define QEMU_REMOTE_PORT_MAX 65535 diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b7db277..e1528af 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->remotePortMin = QEMU_REMOTE_PORT_MIN; + driver->remotePortMax = QEMU_REMOTE_PORT_MAX; + if (!(driver->vncTLSx509certdir = strdup(SYSCONFDIR "/pki/libvirt-vnc"))) { virReportOOMError(); return -1; @@ -263,6 +268,47 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, } } + p = virConfGetValue (conf, "remote_display_port_min"); + CHECK_TYPE ("remote_display_port_min", VIR_CONF_LONG); + if (p) { + if (p->l < QEMU_REMOTE_PORT_MIN) { + /* if the port is too low, we can't get the display name + * to tell to vnc (usually subtract 5900, e.g. localhost:1 + * for port 5901) */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_display_port_min: port must be greater than or equal to %d"), + filename, QEMU_REMOTE_PORT_MIN); + virConfFree(conf); + return -1; + } + driver->remotePortMin = p->l; + } + + p = virConfGetValue (conf, "remote_display_port_max"); + CHECK_TYPE ("remote_display_port_max", VIR_CONF_LONG); + if (p) { + if (p->l > QEMU_REMOTE_PORT_MAX || + p->l < driver->remotePortMin) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_display_port_max: port must be between the minimal port and %d"), + filename, QEMU_REMOTE_PORT_MAX); + virConfFree(conf); + return -1; + } + /* increasing the value by 1 makes all the loops going through + the bitmap (i = remotePortMin; i < remotePortMax; i++), work as + expected. */ + driver->remotePortMax = p->l + 1; + } + + if (driver->remotePortMin > driver->remotePortMax) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_display_port_min: min port must not be greater than max port"), + filename); + virConfFree(conf); + return -1; + } + p = virConfGetValue (conf, "user"); CHECK_TYPE ("user", VIR_CONF_STRING); if (!(user = strdup(p && p->str ? p->str : QEMU_USER))) { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 3895889..ea81b35 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -95,6 +95,8 @@ struct qemud_driver { char *spiceTLSx509certdir; char *spiceListen; char *spicePassword; + int remotePortMin; + int remotePortMax; char *hugetlbfs_mount; char *hugepage_path; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0d886c1..5a82e76 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -520,11 +520,6 @@ qemudStartup(int privileged) { if (!qemu_driver->domainEventState) goto error; - /* Allocate bitmap for vnc port reservation */ - if ((qemu_driver->reservedRemotePorts = - virBitmapAlloc(QEMU_REMOTE_PORT_MAX - QEMU_REMOTE_PORT_MIN)) == NULL) - goto out_of_memory; - /* read the host sysinfo */ if (privileged) qemu_driver->hostsysinfo = virSysinfoRead(); @@ -650,6 +645,13 @@ qemudStartup(int privileged) { } VIR_FREE(driverConf); + /* Allocate bitmap for remote display port reservations. We cannot + * do this before the config is loaded properly, since the port + * numbers are configurable now */ + if ((qemu_driver->reservedRemotePorts = + virBitmapAlloc(qemu_driver->remotePortMax - qemu_driver->remotePortMin)) == NULL) + goto out_of_memory; + /* We should always at least have the 'nop' manager, so * NULLs here are a fatal error */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 4c40b49..05b2f18 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2454,15 +2454,15 @@ static int qemuProcessNextFreePort(struct qemud_driver *driver, { int i; - for (i = startPort ; i < QEMU_REMOTE_PORT_MAX; i++) { + for (i = startPort ; i < driver->remotePortMax; i++) { int fd; int reuse = 1; struct sockaddr_in addr; bool used = false; if (virBitmapGetBit(driver->reservedRemotePorts, - i - QEMU_REMOTE_PORT_MIN, &used) < 0) - VIR_DEBUG("virBitmapGetBit failed on bit %d", i - QEMU_REMOTE_PORT_MIN); + i - driver->remotePortMin, &used) < 0) + VIR_DEBUG("virBitmapGetBit failed on bit %d", i - driver->remotePortMin); if (used) continue; @@ -2484,9 +2484,9 @@ static int qemuProcessNextFreePort(struct qemud_driver *driver, VIR_FORCE_CLOSE(fd); /* Add port to bitmap of reserved ports */ if (virBitmapSetBit(driver->reservedRemotePorts, - i - QEMU_REMOTE_PORT_MIN) < 0) { + i - driver->remotePortMin) < 0) { VIR_DEBUG("virBitmapSetBit failed on bit %d", - i - QEMU_REMOTE_PORT_MIN); + i - driver->remotePortMin); } return i; } @@ -2507,11 +2507,11 @@ static void qemuProcessReturnPort(struct qemud_driver *driver, int port) { - if (port < QEMU_REMOTE_PORT_MIN) + if (port < driver->remotePortMin) return; if (virBitmapClearBit(driver->reservedRemotePorts, - port - QEMU_REMOTE_PORT_MIN) < 0) + port - driver->remotePortMin) < 0) VIR_DEBUG("Could not mark port %d as unused", port); } diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 959f250..eac5882 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -15,6 +15,8 @@ module Test_libvirtd_qemu = { "spice_tls" = "1" } { "spice_tls_x509_cert_dir" = "/etc/pki/libvirt-spice" } { "spice_password" = "XYZ12345" } +{ "remote_display_port_min" = "5900" } +{ "remote_display_port_max" = "65535" } { "security_driver" = "selinux" } { "security_default_confined" = "1" } { "security_require_confined" = "1" } -- 1.7.8.6

On Mon, Aug 13, 2012 at 03:21:23PM +0200, Martin Kletzander wrote:
The defines QEMU_REMOTE_PORT_MIN and QEMU_REMOTE_PORT_MAX were used to find free port when starting domains. As this was hard-coded 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 (mostly for completeness) in qemu config file.
Support for two new config options in qemu.conf is added: - remote_port_min (defaults to QEMU_REMOTE_PORT_MIN and must be >= than this value) - remote_port_max (defaults to QEMU_REMOTE_PORT_MAX and must be <= than this value) --- src/qemu/libvirtd_qemu.aug | 4 +++ src/qemu/qemu.conf | 14 ++++++++++ src/qemu/qemu_command.h | 7 +++++ src/qemu/qemu_conf.c | 48 +++++++++++++++++++++++++++++++++++- src/qemu/qemu_conf.h | 2 + src/qemu/qemu_driver.c | 12 +++++--- src/qemu/qemu_process.c | 14 +++++----- src/qemu/test_libvirtd_qemu.aug.in | 2 + 8 files changed, 90 insertions(+), 13 deletions(-)
ACK 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 :|

After the cleanup of remote display port allocation, I noticed some messages that didn't make a lot of sense the way they were written. So I rephrased them. --- src/qemu/qemu_process.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 05b2f18..654cb51 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3425,7 +3425,7 @@ int qemuProcessStart(virConnectPtr conn, if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to find an unused VNC port")); + "%s", _("Unable to find an unused port for VNC")); goto cleanup; } vm->def->graphics[0]->data.vnc.port = port; @@ -3440,7 +3440,7 @@ int qemuProcessStart(virConnectPtr conn, if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to find an unused SPICE port")); + "%s", _("Unable to find an unused port for SPICE")); goto cleanup; } vm->def->graphics[0]->data.spice.port = port; @@ -3452,7 +3452,7 @@ int qemuProcessStart(virConnectPtr conn, port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.port + 1); if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to find an unused SPICE TLS port")); + "%s", _("Unable to find an unused port for SPICE TLS")); qemuProcessReturnPort(driver, port); goto cleanup; } -- 1.7.8.6

On Mon, Aug 13, 2012 at 03:21:24PM +0200, Martin Kletzander wrote:
After the cleanup of remote display port allocation, I noticed some messages that didn't make a lot of sense the way they were written. So I rephrased them. --- src/qemu/qemu_process.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 05b2f18..654cb51 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3425,7 +3425,7 @@ int qemuProcessStart(virConnectPtr conn,
if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to find an unused VNC port")); + "%s", _("Unable to find an unused port for VNC")); goto cleanup; } vm->def->graphics[0]->data.vnc.port = port; @@ -3440,7 +3440,7 @@ int qemuProcessStart(virConnectPtr conn,
if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to find an unused SPICE port")); + "%s", _("Unable to find an unused port for SPICE")); goto cleanup; } vm->def->graphics[0]->data.spice.port = port; @@ -3452,7 +3452,7 @@ int qemuProcessStart(virConnectPtr conn, port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.port + 1); if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Unable to find an unused SPICE TLS port")); + "%s", _("Unable to find an unused port for SPICE TLS")); qemuProcessReturnPort(driver, port); goto cleanup; }
ACK 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 :|

This patch allows to specify a flag 'rotate' saying that the search for next port should not be limited from the 'startPort' upwards. This is subsequently used in the search for open SPICE TLS port when none is specified and TLS is enabled. --- src/qemu/qemu_process.c | 24 +++++++++++++++++------- 1 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 654cb51..097d73a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2450,10 +2450,12 @@ qemuProcessInitPCIAddresses(struct qemud_driver *driver, static int qemuProcessNextFreePort(struct qemud_driver *driver, - int startPort) + int startPort, + bool rotate) { int i; + retry: for (i = startPort ; i < driver->remotePortMax; i++) { int fd; int reuse = 1; @@ -2499,6 +2501,14 @@ static int qemuProcessNextFreePort(struct qemud_driver *driver, /* Some other bad failure, get out.. */ break; } + + /* In case we want to search for any open port and the startPort + * parameter is just a preference */ + if (rotate && startPort != driver->remotePortMin) { + startPort = driver->remotePortMin; + goto retry; + } + return -1; } @@ -3419,9 +3429,9 @@ int qemuProcessStart(virConnectPtr conn, vm->def->graphics[0]->data.vnc.autoport) { int port; if (vm->def->graphics[0]->data.vnc.port > 0) - port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.vnc.port); + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.vnc.port, false); else - port = qemuProcessNextFreePort(driver, QEMU_REMOTE_PORT_MIN); + port = qemuProcessNextFreePort(driver, QEMU_REMOTE_PORT_MIN, false); if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -3434,9 +3444,9 @@ int qemuProcessStart(virConnectPtr conn, vm->def->graphics[0]->data.spice.autoport) { int port; if (vm->def->graphics[0]->data.spice.port > 0) - port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.port); + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.port, false); else - port = qemuProcessNextFreePort(driver, QEMU_REMOTE_PORT_MIN); + port = qemuProcessNextFreePort(driver, QEMU_REMOTE_PORT_MIN, false); if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -3447,9 +3457,9 @@ int qemuProcessStart(virConnectPtr conn, if (driver->spiceTLS) { if (vm->def->graphics[0]->data.spice.tlsPort > 0) - port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.tlsPort); + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.tlsPort, false); else - port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.port + 1); + port = qemuProcessNextFreePort(driver, vm->def->graphics[0]->data.spice.port + 1, true); if (port < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find an unused port for SPICE TLS")); -- 1.7.8.6

On Mon, Aug 13, 2012 at 03:21:25PM +0200, Martin Kletzander wrote:
This patch allows to specify a flag 'rotate' saying that the search for next port should not be limited from the 'startPort' upwards.
This is subsequently used in the search for open SPICE TLS port when none is specified and TLS is enabled. --- src/qemu/qemu_process.c | 24 +++++++++++++++++------- 1 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 654cb51..097d73a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2450,10 +2450,12 @@ qemuProcessInitPCIAddresses(struct qemud_driver *driver,
static int qemuProcessNextFreePort(struct qemud_driver *driver, - int startPort) + int startPort, + bool rotate)
I think this is bogus - we should simply not pass startPort or rotate in as parameters at all.
{ int i;
+ retry: for (i = startPort ; i < driver->remotePortMax; i++) {
This should just loop from driver->remotePortMin always 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 (2)
-
Daniel P. Berrange
-
Martin Kletzander