[libvirt] [PATCH 1/2] qemu: remove duplicated code for allocating spice ports

Martin Kletzander mkletzan at redhat.com
Mon Mar 2 16:22:28 UTC 2015


On Fri, Feb 27, 2015 at 03:35:26PM +0100, Pavel Hrdina wrote:
>We have two different places that needs to be updated while touching
>code for allocation spice ports.  Add a bool option to
>'qemuProcessSPICEAllocatePorts' function to switch between true and fake
>allocation so we can use this function also in qemu_driver to generate
>native domain definition.
>
>Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>---
> src/qemu/qemu_driver.c  | 45 ++-------------------------------------------
> src/qemu/qemu_process.c | 42 ++++++++++++++++++++++++++----------------
> src/qemu/qemu_process.h |  5 +++++
> 3 files changed, 33 insertions(+), 59 deletions(-)
>
>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>index e282464..46439cb 100644
>--- a/src/qemu/qemu_driver.c
>+++ b/src/qemu/qemu_driver.c
>@@ -6579,49 +6579,8 @@ static char *qemuConnectDomainXMLToNative(virConnectPtr conn,
>             !graphics->data.vnc.socket && graphics->data.vnc.autoport) {
>             graphics->data.vnc.port = 5900;
>         } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
>-            size_t j;
>-            bool needTLSPort = false;
>-            bool needPort = false;
>-            int defaultMode = graphics->data.spice.defaultMode;
>-
>-            if (graphics->data.spice.autoport) {
>-                /* check if tlsPort or port need allocation */
>-                for (j = 0; j < VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST; j++) {
>-                    switch (graphics->data.spice.channels[j]) {
>-                    case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
>-                        needTLSPort = true;
>-                        break;
>-
>-                    case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE:
>-                        needPort = true;
>-                        break;
>-
>-                    case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY:
>-                        switch (defaultMode) {
>-                        case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
>-                            needTLSPort = true;
>-                            break;
>-
>-                        case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE:
>-                            needPort = true;
>-                            break;
>-
>-                        case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY:
>-                            if (cfg->spiceTLS)
>-                                needTLSPort = true;
>-                            needPort = true;
>-                            break;
>-                        }
>-                        break;
>-                    }
>-                }
>-            }
>-
>-            if (needPort || graphics->data.spice.port == -1)
>-                graphics->data.spice.port = 5901;
>-
>-            if (needTLSPort || graphics->data.spice.tlsPort == -1)
>-                graphics->data.spice.tlsPort = 5902;
>+            ignore_value(qemuProcessSPICEAllocatePorts(driver, cfg,
>+                                                       graphics, false));

Don't ignore any errors (even though currently there won't be any),
just jump to clean-up for return value < 0.

>         }
>     }
>
>diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>index 515402e..aeb479c 100644
>--- a/src/qemu/qemu_process.c
>+++ b/src/qemu/qemu_process.c
>@@ -4019,10 +4019,11 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
>     return 0;
> }
>
>-static int
>+int
> qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
>                               virQEMUDriverConfigPtr cfg,
>-                              virDomainGraphicsDefPtr graphics)
>+                              virDomainGraphicsDefPtr graphics,
>+                              bool allocate)
> {
>     unsigned short port = 0;
>     unsigned short tlsPort;
>@@ -4066,30 +4067,39 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
>     }
>
>     if (needPort || graphics->data.spice.port == -1) {
>-        if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0)
>-            goto error;
>+        if (allocate) {
>+            if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0)
>+                goto error;
>
>-        graphics->data.spice.port = port;
>+            graphics->data.spice.port = port;
>+        } else {
>+            graphics->data.spice.port = 5901;
>+        }
>     }
>
>     if (needTLSPort || graphics->data.spice.tlsPort == -1) {
>-        if (!cfg->spiceTLS) {
>-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>-                           _("Auto allocation of spice TLS port requested "
>-                             "but spice TLS is disabled in qemu.conf"));
>-            goto error;
>-        }
>+        if (allocate) {

I'd rather use:

  if (!allocate) {
      if (needPort || graphics->data.spice.port == -1)
          graphics->data.spice.port = 5901;
      if (needTlsPort || graphics->data.spice.tlsPort == -1)
          graphics->data.spice.tlsPort = 5902;

      return 0;
  }

  /* and here deal with the allocations */

but what you've done is fine, too.

ACK with or without that changed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150302/a5530b48/attachment-0001.sig>


More information about the libvir-list mailing list