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@xxxxxxxxxxxxx>
> ---
> 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
> }
> }
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
