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@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
} } }