[libvirt] [PATCH 2/5] qemu: simplify graphics port releasing

[John Ferlan]

On 07/04/2018 07:03 AM, Nikolay Shirokovskiy wrote:
> Originally port allocator have 2 functions to release ports: one
> for for manually reserved ports and one for autoallocated ports. Thus
> a bit complicated code of port releasing. Now we have only one releasing
> function.

But there's a reason for the difference between manually allocated and
automatically allocated. This patch I believe will blur those lines.

Also "technically" it's not 2 functions it's different conditions during
qemuProcessStop which must be handled.

> 
> Let's use *reserved flag whenever we manually/automatically allocate
> port so that we can use it later for releasing.
> 
> Actually we set *reserved flag on autoallocation already on reconnection,
> which lead to uncleared flag on stop. So this step looks natural.
> 
> qemuProcessGraphicsReservePorts is called on reconnect. As a result
> portReserved is set not only for manual ports but autoports too. Now
> due to the way port releasing is written in qemuProcessStop portReserved
> stays set for autoports after domain stop. Now imagine one redefine
> 

^^^ This whole commit message is really difficult to read and you seem
to have lost your thought in the last sentence of the last paragraph.

> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> ---
>  src/conf/domain_conf.h  |  1 +
>  src/qemu/qemu_process.c | 48 ++++++++++++++++++++++--------------------------
>  2 files changed, 23 insertions(+), 26 deletions(-)
> 
NACK without a better explanation as to why this is important.

> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 41d2748..1da6b8f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1610,6 +1610,7 @@ struct _virDomainGraphicsDef {
>              int port;
>              bool portReserved;
>              int websocket;
> +            bool websocketReserved;
>              bool websocketGenerated;
>              bool autoport;
>              char *keymap;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index da5656d..de2e84b 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3628,12 +3628,14 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
>          if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0)
>              return -1;
>          graphics->data.vnc.port = port;
> +        graphics->data.vnc.portReserved = true;

Why?  "autoport" says acquire some/any port not a specific port.  So on
restart we can use anything and it's not a specifically reserved port.
So on reconnect IIUC we don't need to "block" usage of specific ports.
Keeping track of what port number got autoallocated is a different issue
I would think and if that's what you're after, then that would be a
different set of patches, wouldn't it?

>      }
>  
>      if (graphics->data.vnc.websocket == -1) {

Recall from:

struct _virDomainGraphicsDef {
    /* Port value discipline:
     * Value -1 is legacy syntax indicating that it should be
auto-allocated.
     * Value 0 means port wasn't specified in XML at all.
     * Positive value is actual port number given in XML.
     */

>          if (virPortAllocatorAcquire(driver->webSocketPorts, &port) < 0)
>              return -1;
>          graphics->data.vnc.websocket = port;
> +        graphics->data.vnc.websocketReserved = true;
>          graphics->data.vnc.websocketGenerated = true;

So again, why? Not sure I agree - the value was Generated, but not
Reserved ad infinatum.

>      }
>  
> @@ -3705,9 +3707,7 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
>              goto cleanup;
>  
>          graphics->data.spice.port = port;
> -
> -        if (!graphics->data.spice.autoport)
> -            graphics->data.spice.portReserved = true;
> +        graphics->data.spice.portReserved = true;

Same as before - autoport != reserved

>      }
>  
>      if (needTLSPort || graphics->data.spice.tlsPort == -1) {
> @@ -3722,9 +3722,7 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
>              goto cleanup;
>  
>          graphics->data.spice.tlsPort = tlsPort;
> -
> -        if (!graphics->data.spice.autoport)
> -            graphics->data.spice.tlsPortReserved = true;
> +        graphics->data.spice.tlsPortReserved = true;

ditto

>      }
>  
>      ret = 0;
> @@ -4328,9 +4326,11 @@ qemuProcessGraphicsReservePorts(virDomainGraphicsDefPtr graphics,
>                  return -1;
>              graphics->data.vnc.portReserved = true;
>          }
> -        if (graphics->data.vnc.websocket > 0 &&
> -            virPortAllocatorSetUsed(graphics->data.vnc.websocket) < 0)
> -            return -1;
> +        if (graphics->data.vnc.websocket > 0) {
> +            if (virPortAllocatorSetUsed(graphics->data.vnc.websocket) < 0)
> +                return -1;
> +            graphics->data.vnc.websocketReserved = true;

This may have some merit, although for websocket, the
!websocketGenerated means the port was reserved so is there a real need?
 It's just reverse logic from autoport and portReserved.

> +        }
>          break;
>  
>      case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
> @@ -6974,34 +6974,30 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>      for (i = 0; i < vm->def->ngraphics; ++i) {
>          virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
>          if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> -            if (graphics->data.vnc.autoport) {
> -                virPortAllocatorRelease(graphics->data.vnc.port);
> -            } else if (graphics->data.vnc.portReserved) {
> +            if (graphics->data.vnc.portReserved) {
>                  virPortAllocatorRelease(graphics->data.vnc.port);
>                  graphics->data.vnc.portReserved = false;
>              }
> +
> +            if (graphics->data.vnc.websocketReserved) {
> +                virPortAllocatorRelease(graphics->data.vnc.websocket);
> +                graphics->data.vnc.websocketReserved = false;
> +            }
> +
>              if (graphics->data.vnc.websocketGenerated) {
> -                virPortAllocatorRelease(graphics->data.vnc.websocket);
>                  graphics->data.vnc.websocketGenerated = false;
>                  graphics->data.vnc.websocket = -1;
> -            } else if (graphics->data.vnc.websocket) {
> -                virPortAllocatorRelease(graphics->data.vnc.websocket);
>              }
>          }
>          if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) {
> -            if (graphics->data.spice.autoport) {
> +            if (graphics->data.spice.portReserved) {
>                  virPortAllocatorRelease(graphics->data.spice.port);
> +                graphics->data.spice.portReserved = false;
> +            }
> +
> +            if (graphics->data.spice.tlsPortReserved) {
>                  virPortAllocatorRelease(graphics->data.spice.tlsPort);
> -            } else {
> -                if (graphics->data.spice.portReserved) {
> -                    virPortAllocatorRelease(graphics->data.spice.port);
> -                    graphics->data.spice.portReserved = false;
> -                }
> -
> -                if (graphics->data.spice.tlsPortReserved) {
> -                    virPortAllocatorRelease(graphics->data.spice.tlsPort);
> -                    graphics->data.spice.tlsPortReserved = false;
> -                }
> +                graphics->data.spice.tlsPortReserved = false;

I think the existing code is fine until proven otherwise.

John

>              }
>          }
>      }
>