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

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(a)virtuozzo.com&gt; > --- > 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 > } > } > } >