[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] qemu: configurable VNC port boundaries for domains



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]