[libvirt] [PATCH 0/5] qemu: misc graphics fixes
Nikolay Shirokovskiy (5): qemu: fix typo in vnc port releasing qemu: simplify graphics port releasing qemu: vnc: mark websocket as used on reconnect qemu: mark graphics ports as used on migration qemu: keep websocketGenerated on libvirtd restarts src/conf/domain_conf.c | 9 +++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_migration.c | 6 ++++++ src/qemu/qemu_process.c | 51 ++++++++++++++++++++++------------------------- src/qemu/qemu_process.h | 3 +++ 5 files changed, 43 insertions(+), 27 deletions(-) -- 1.8.3.1
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 40d35cb..da5656d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6977,7 +6977,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, if (graphics->data.vnc.autoport) { virPortAllocatorRelease(graphics->data.vnc.port); } else if (graphics->data.vnc.portReserved) { - virPortAllocatorRelease(graphics->data.spice.port); + virPortAllocatorRelease(graphics->data.vnc.port); graphics->data.vnc.portReserved = false; } if (graphics->data.vnc.websocketGenerated) { -- 1.8.3.1
On 07/04/2018 07:03 AM, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Copy-Pasta from commit id 1a065caa7 it seems (it's only 4 years old, just a young bug). Reviewed-by: John Ferlan <jferlan@redhat.com> John
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. 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 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(-) 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; } if (graphics->data.vnc.websocket == -1) { if (virPortAllocatorAcquire(driver->webSocketPorts, &port) < 0) return -1; graphics->data.vnc.websocket = port; + graphics->data.vnc.websocketReserved = true; graphics->data.vnc.websocketGenerated = true; } @@ -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; } 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; } 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; + } 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; } } } -- 1.8.3.1
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
} } }
On 17.07.2018 23:15, John Ferlan wrote:
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.
I mean look at src/util/virportallocator.c before 5dbda5e972. We use virPortAllocatorRelease to release auto generated ports and virPortAllocatorSetUsed($value, false) to release manually specified ports.
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.
Hmm, somehow I just lost a part of text in message. I thought we can have issues with port releasing in current code because we use portReserved a bit innacurately. For example we set it for manual ports on domain start so we know we need to release port on domain stop. But on reconnect we set portReserved for all ports and do not clear this flag for auto ports on domain stop. Fortunately for autoports this flags is never checked and on domain config update if we change autoport to manual port we reset this flag as it is not part of user defined configuration. So using portReserved is inaccurate but we don't have any issues yet. One can just clear portReserved unconditionally on domain stop to make it more accurate. But I think we can instead make releasing ports on domain stop more starightforward. Instead of make distinction auto/manual let's use portReserved for both auto and manual ports. We need this flag for manual ports why not use it for autoports too? In short, let's set portReserved for any port reserving via virPortAllocatorSetUsed or virPortAllocatorAcquire so we know we need to release port later with virPortAllocatorRelease. Nikolay
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
} } }
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index de2e84b..09e0327 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4326,7 +4326,8 @@ qemuProcessGraphicsReservePorts(virDomainGraphicsDefPtr graphics, return -1; graphics->data.vnc.portReserved = true; } - if (graphics->data.vnc.websocket > 0) { + if (graphics->data.vnc.websocket > 0 || + reconnect) { if (virPortAllocatorSetUsed(graphics->data.vnc.websocket) < 0) return -1; graphics->data.vnc.websocketReserved = true; -- 1.8.3.1
On 07/04/2018 07:03 AM, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
So your contention is that commit id 84067696 missed a condition? I agree, but would like you to reference that commit id in your commit message before pushing - especially since AIUI websocket > 0 means it's a port we've specifically wanted reserved. Reviewed-by: John Ferlan <jferlan@redhat.com> John
On 17.07.2018 23:16, John Ferlan wrote:
On 07/04/2018 07:03 AM, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
So your contention is that commit id 84067696 missed a condition?
I agree, but would like you to reference that commit id in your commit message before pushing - especially since AIUI websocket > 0 means it's a port we've specifically wanted reserved.
Honestly I don't undestand why I wrote this patch. On reconnection whether websocket is auto generated or not it will be marked as used in qemuProcessGraphicsReservePorts because it is > 0. Nikolay
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_migration.c | 6 ++++++ src/qemu/qemu_process.c | 2 +- src/qemu/qemu_process.h | 3 +++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 435cd17..7e12ff6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4928,6 +4928,7 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver, qemuDomainJobInfoPtr jobInfo = NULL; bool inPostCopy = false; bool doKill = true; + size_t i; VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=0x%lx, retcode=%d", @@ -5000,6 +5001,11 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver, if (qemuConnectAgent(driver, vm) < 0) goto endjob; + for (i = 0; i < vm->def->ngraphics; i++) { + if (qemuProcessGraphicsReservePorts(vm->def->graphics[i], true) < 0) + goto endjob; + } + if (flags & VIR_MIGRATE_PERSIST_DEST) { if (qemuMigrationDstPersist(driver, vm, mig, !v3proto) < 0) { /* Hmpf. Migration was successful, but making it persistent diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 09e0327..9f41313 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4303,7 +4303,7 @@ qemuProcessStartHook(virQEMUDriverPtr driver, } -static int +int qemuProcessGraphicsReservePorts(virDomainGraphicsDefPtr graphics, bool reconnect) { diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 07ce3a9..5d234f0 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -214,4 +214,7 @@ int qemuProcessStartManagedPRDaemon(virDomainObjPtr vm); void qemuProcessKillManagedPRDaemon(virDomainObjPtr vm); +int qemuProcessGraphicsReservePorts(virDomainGraphicsDefPtr graphics, + bool reconnect); + #endif /* __QEMU_PROCESS_H__ */ -- 1.8.3.1
Please disregard, this one is definetly a wrong one. On 04.07.2018 14:03, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_migration.c | 6 ++++++ src/qemu/qemu_process.c | 2 +- src/qemu/qemu_process.h | 3 +++ 3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 435cd17..7e12ff6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4928,6 +4928,7 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver, qemuDomainJobInfoPtr jobInfo = NULL; bool inPostCopy = false; bool doKill = true; + size_t i;
VIR_DEBUG("driver=%p, dconn=%p, vm=%p, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=0x%lx, retcode=%d", @@ -5000,6 +5001,11 @@ qemuMigrationDstFinish(virQEMUDriverPtr driver, if (qemuConnectAgent(driver, vm) < 0) goto endjob;
+ for (i = 0; i < vm->def->ngraphics; i++) { + if (qemuProcessGraphicsReservePorts(vm->def->graphics[i], true) < 0) + goto endjob; + } + if (flags & VIR_MIGRATE_PERSIST_DEST) { if (qemuMigrationDstPersist(driver, vm, mig, !v3proto) < 0) { /* Hmpf. Migration was successful, but making it persistent diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 09e0327..9f41313 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4303,7 +4303,7 @@ qemuProcessStartHook(virQEMUDriverPtr driver, }
-static int +int qemuProcessGraphicsReservePorts(virDomainGraphicsDefPtr graphics, bool reconnect) { diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 07ce3a9..5d234f0 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -214,4 +214,7 @@ int qemuProcessStartManagedPRDaemon(virDomainObjPtr vm);
void qemuProcessKillManagedPRDaemon(virDomainObjPtr vm);
+int qemuProcessGraphicsReservePorts(virDomainGraphicsDefPtr graphics, + bool reconnect); + #endif /* __QEMU_PROCESS_H__ */
Otherwise after libvirtd restart we come back to issues fixed by introducing this flag in [1]. [1] 61a0026a : qemu: Fix xml dump of autogenerated websocket Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f4e59f6..e27a125 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13574,6 +13574,7 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, { char *port = virXMLPropString(node, "port"); char *websocket = virXMLPropString(node, "websocket"); + char *websocketGenerated = virXMLPropString(node, "websocketGenerated"); char *sharePolicy = virXMLPropString(node, "sharePolicy"); char *autoport = virXMLPropString(node, "autoport"); int ret = -1; @@ -13618,6 +13619,9 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, } } + if (websocketGenerated && STREQ(websocketGenerated, "yes")) + def->data.vnc.websocketGenerated = true; + if (sharePolicy) { int policy = virDomainGraphicsVNCSharePolicyTypeFromString(sharePolicy); @@ -13643,6 +13647,7 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def, VIR_FREE(port); VIR_FREE(autoport); VIR_FREE(websocket); + VIR_FREE(websocketGenerated); VIR_FREE(sharePolicy); return ret; } @@ -26245,6 +26250,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, else if (def->data.vnc.websocket) virBufferAsprintf(buf, " websocket='%d'", def->data.vnc.websocket); + if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS) + virBufferAsprintf(buf, " websocketGenerated='%s'", + def->data.vnc.websocketGenerated ? "yes" : "no"); + virDomainGraphicsListenDefFormatAddr(buf, glisten, flags); break; case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NONE: -- 1.8.3.1
On 07/04/2018 07:03 AM, Nikolay Shirokovskiy wrote:
Otherwise after libvirtd restart we come back to issues fixed by introducing this flag in [1].
[1] 61a0026a : qemu: Fix xml dump of autogenerated websocket
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/domain_conf.c | 9 +++++++++ 1 file changed, 9 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
ACKed Patches 1 and 5 are pushed, thanx! Sorry it took so long after review, I hoped to get further response. Nikolay On 04.07.2018 14:03, Nikolay Shirokovskiy wrote:
Nikolay Shirokovskiy (5): qemu: fix typo in vnc port releasing qemu: simplify graphics port releasing qemu: vnc: mark websocket as used on reconnect qemu: mark graphics ports as used on migration qemu: keep websocketGenerated on libvirtd restarts
src/conf/domain_conf.c | 9 +++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_migration.c | 6 ++++++ src/qemu/qemu_process.c | 51 ++++++++++++++++++++++------------------------- src/qemu/qemu_process.h | 3 +++ 5 files changed, 43 insertions(+), 27 deletions(-)
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy