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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> 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.

Attachment: pgpPcwSiSaqpt.pgp
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list