[libvirt] [PATCH 0/3] qemu: bugfixes for websocket port in graphics
Patch 1 is a preparation for patch 2. Patch 3 particularly fixes next 2 cases: == A. Can not restore domain with autoconfigured websocket. domain 1 and 2 have autoconfigured websocket. 1. domain 1 is started then, saved 2. domain 2 is started 3. domain 1 restoration is failed: error: internal error: qemu unexpectedly closed the monitor: 2016-11-21T10:23:11.356687Z qemu-kvm: -vnc 0.0.0.0:2,websocket=5700: Failed to start VNC server on `(null)': Failed to bind socket: Address already in use == B. Can not migrate domain with autoconfigured websocket. domain 1 on host A, domain 2 on host B, both have autoconfigured websocket 1. domain 1 started, domain 2 started 2. domain 1 migration to host B is failed with the above error. Nikolay Shirokovskiy (3): qemu: refactor: use switch for enum in qemuProcessGraphicsReservePorts qemu: mark user defined websocket as used qemu: fix xml dump of autogenerated websocket src/conf/domain_conf.c | 5 ++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 45 ++++++++++++++++++++++++++++++++++++--------- 3 files changed, 41 insertions(+), 10 deletions(-) -- 1.8.3.1
--- src/qemu/qemu_process.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3552a31..6aaaa10 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4056,16 +4056,21 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver, glisten->type != VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK) return 0; - if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && - !graphics->data.vnc.autoport) { - if (virPortAllocatorSetUsed(driver->remotePorts, - graphics->data.vnc.port, - true) < 0) - return -1; - graphics->data.vnc.portReserved = true; + switch (graphics->type) { + case VIR_DOMAIN_GRAPHICS_TYPE_VNC: + if (!graphics->data.vnc.autoport) { + if (virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.vnc.port, + true) < 0) + return -1; + graphics->data.vnc.portReserved = true; + } + break; + + case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: + if (graphics->data.spice.autoport) + return 0; - } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE && - !graphics->data.spice.autoport) { if (graphics->data.spice.port > 0) { if (virPortAllocatorSetUsed(driver->remotePorts, graphics->data.spice.port, @@ -4081,6 +4086,13 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver, return -1; graphics->data.spice.tlsPortReserved = true; } + break; + + case VIR_DOMAIN_GRAPHICS_TYPE_SDL: + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: + case VIR_DOMAIN_GRAPHICS_TYPE_LAST: + break; } return 0; -- 1.8.3.1
On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
--- src/qemu/qemu_process.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)
ACK - although I'll reword commit a bit: qemu: Refactor qemuProcessGraphicsReservePorts Use switch for enums rather than if/else conditions. John FWIW: After reading patch 2, why not alter the code in qemuProcessStop in order to have a switch too? In fact that code could be split out into a qemuProcessGraphicsUnreservePorts function. If you want to make a v2 to do then, then let me know.
On 08.12.2016 16:21, John Ferlan wrote:
On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
--- src/qemu/qemu_process.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)
ACK - although I'll reword commit a bit:
qemu: Refactor qemuProcessGraphicsReservePorts
Use switch for enums rather than if/else conditions.
Ok.
FWIW: After reading patch 2, why not alter the code in qemuProcessStop in order to have a switch too? In fact that code could be split out into a qemuProcessGraphicsUnreservePorts function. If you want to make a v2 to do then, then let me know.
I'd better offload the change you suggest to a different series/patch. This patch on the other hand is correlated with next one - you can not add extra vnc logic to qemuProcessGraphicsReservePorts without changing its overall control flow. So does this patch. Nikolay
We need extra state variable to distinguish between autogenerated and user defined cases after auto generation is done. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 541b600..9bc4522 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1468,6 +1468,7 @@ struct _virDomainGraphicsDef { int port; bool portReserved; int websocket; + bool websocketGenerated; bool autoport; char *keymap; virDomainGraphicsAuthDef auth; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6aaaa10..1799f33 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3574,6 +3574,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver, if (virPortAllocatorAcquire(driver->webSocketPorts, &port) < 0) return -1; graphics->data.vnc.websocket = port; + graphics->data.vnc.websocketGenerated = true; } return 0; @@ -4065,6 +4066,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver, return -1; graphics->data.vnc.portReserved = true; } + if (graphics->data.vnc.websocket != -1 && /* auto websocket */ + graphics->data.vnc.websocket && /* no websocket */ + virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.vnc.websocket, + true) < 0) + return -1; break; case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: @@ -6189,8 +6196,16 @@ void qemuProcessStop(virQEMUDriverPtr driver, false); graphics->data.vnc.portReserved = false; } - virPortAllocatorRelease(driver->webSocketPorts, - graphics->data.vnc.websocket); + if (graphics->data.vnc.websocketGenerated) { + virPortAllocatorRelease(driver->webSocketPorts, + graphics->data.vnc.websocket); + graphics->data.vnc.websocketGenerated = false; + graphics->data.vnc.websocket = -1; + } else if (graphics->data.vnc.websocket) { + virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.vnc.websocket, + false); + } } if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { if (graphics->data.spice.autoport) { -- 1.8.3.1
On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
We need extra state variable to distinguish between autogenerated and user defined cases after auto generation is done. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 541b600..9bc4522 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1468,6 +1468,7 @@ struct _virDomainGraphicsDef { int port; bool portReserved; int websocket; + bool websocketGenerated; bool autoport; char *keymap; virDomainGraphicsAuthDef auth; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6aaaa10..1799f33 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3574,6 +3574,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver, if (virPortAllocatorAcquire(driver->webSocketPorts, &port) < 0) return -1; graphics->data.vnc.websocket = port; + graphics->data.vnc.websocketGenerated = true; }
return 0; @@ -4065,6 +4066,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver, return -1; graphics->data.vnc.portReserved = true; } + if (graphics->data.vnc.websocket != -1 && /* auto websocket */ + graphics->data.vnc.websocket && /* no websocket */
Some would prefer no comments because the logic should be self explanatory. IDC, but would rather see the comment before rather than within the "if" statement. In any case, why isn't this just: if (graphics->data.vnc.websocket > 0) { note: no comments. IOW: If a user defined a specific port, set that in the remotePorts. ACK in general - I can adjust the check before pushing based on your feedback. I could also wait for a v2 if you want to create an Unreserve helper with switch/case too. John
+ virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.vnc.websocket, + true) < 0) + return -1; break;
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: @@ -6189,8 +6196,16 @@ void qemuProcessStop(virQEMUDriverPtr driver, false); graphics->data.vnc.portReserved = false; } - virPortAllocatorRelease(driver->webSocketPorts, - graphics->data.vnc.websocket); + if (graphics->data.vnc.websocketGenerated) { + virPortAllocatorRelease(driver->webSocketPorts, + graphics->data.vnc.websocket); + graphics->data.vnc.websocketGenerated = false; + graphics->data.vnc.websocket = -1; + } else if (graphics->data.vnc.websocket) { + virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.vnc.websocket, + false); + } } if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { if (graphics->data.spice.autoport) {
On 08.12.2016 16:21, John Ferlan wrote:
On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
We need extra state variable to distinguish between autogenerated and user defined cases after auto generation is done. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 541b600..9bc4522 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1468,6 +1468,7 @@ struct _virDomainGraphicsDef { int port; bool portReserved; int websocket; + bool websocketGenerated; bool autoport; char *keymap; virDomainGraphicsAuthDef auth; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6aaaa10..1799f33 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3574,6 +3574,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver, if (virPortAllocatorAcquire(driver->webSocketPorts, &port) < 0) return -1; graphics->data.vnc.websocket = port; + graphics->data.vnc.websocketGenerated = true; }
return 0; @@ -4065,6 +4066,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver, return -1; graphics->data.vnc.portReserved = true; } + if (graphics->data.vnc.websocket != -1 && /* auto websocket */ + graphics->data.vnc.websocket && /* no websocket */
Some would prefer no comments because the logic should be self explanatory. IDC, but would rather see the comment before rather than within the "if" statement.
In any case, why isn't this just:
if (graphics->data.vnc.websocket > 0) {
This is better of course )
note: no comments.
IOW: If a user defined a specific port, set that in the remotePorts.
ACK in general - I can adjust the check before pushing based on your feedback. I could also wait for a v2 if you want to create an Unreserve helper with switch/case too.
So I'm on with you change. Nikolay
On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
We need extra state variable to distinguish between autogenerated and user defined cases after auto generation is done. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 541b600..9bc4522 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1468,6 +1468,7 @@ struct _virDomainGraphicsDef { int port; bool portReserved; int websocket; + bool websocketGenerated; bool autoport; char *keymap; virDomainGraphicsAuthDef auth; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6aaaa10..1799f33 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3574,6 +3574,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver, if (virPortAllocatorAcquire(driver->webSocketPorts, &port) < 0) return -1; graphics->data.vnc.websocket = port; + graphics->data.vnc.websocketGenerated = true; }
return 0; @@ -4065,6 +4066,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver, return -1; graphics->data.vnc.portReserved = true; } + if (graphics->data.vnc.websocket != -1 && /* auto websocket */ + graphics->data.vnc.websocket && /* no websocket */ + virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.vnc.websocket, + true) < 0) + return -1; break;
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: @@ -6189,8 +6196,16 @@ void qemuProcessStop(virQEMUDriverPtr driver, false); graphics->data.vnc.portReserved = false; } - virPortAllocatorRelease(driver->webSocketPorts, - graphics->data.vnc.websocket); + if (graphics->data.vnc.websocketGenerated) { + virPortAllocatorRelease(driver->webSocketPorts, + graphics->data.vnc.websocket); + graphics->data.vnc.websocketGenerated = false; + graphics->data.vnc.websocket = -1;
One more question... Should this be 0 instead of -1? We set to -1 during Reserve and set the Generated flag indicating that the user didn't set to -1, but we did. So when we Release the autogenerated port that we created because -1 was set, shouldn't we set it back to 0 just like it would have been before we decided to set to -1 and set the Generated flag? Avoids other code that then may *print* the websocket as -1... John
+ } else if (graphics->data.vnc.websocket) { + virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.vnc.websocket, + false); + } } if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { if (graphics->data.spice.autoport) {
On 08.12.2016 23:07, John Ferlan wrote:
On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
We need extra state variable to distinguish between autogenerated and user defined cases after auto generation is done. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 541b600..9bc4522 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1468,6 +1468,7 @@ struct _virDomainGraphicsDef { int port; bool portReserved; int websocket; + bool websocketGenerated; bool autoport; char *keymap; virDomainGraphicsAuthDef auth; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6aaaa10..1799f33 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3574,6 +3574,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver, if (virPortAllocatorAcquire(driver->webSocketPorts, &port) < 0) return -1; graphics->data.vnc.websocket = port; + graphics->data.vnc.websocketGenerated = true; }
return 0; @@ -4065,6 +4066,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver, return -1; graphics->data.vnc.portReserved = true; } + if (graphics->data.vnc.websocket != -1 && /* auto websocket */ + graphics->data.vnc.websocket && /* no websocket */ + virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.vnc.websocket, + true) < 0) + return -1; break;
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: @@ -6189,8 +6196,16 @@ void qemuProcessStop(virQEMUDriverPtr driver, false); graphics->data.vnc.portReserved = false; } - virPortAllocatorRelease(driver->webSocketPorts, - graphics->data.vnc.websocket); + if (graphics->data.vnc.websocketGenerated) { + virPortAllocatorRelease(driver->webSocketPorts, + graphics->data.vnc.websocket); + graphics->data.vnc.websocketGenerated = false; + graphics->data.vnc.websocket = -1;
One more question... Should this be 0 instead of -1?
We set to -1 during Reserve and set the Generated flag indicating that the user didn't set to -1, but we did.
Not quite. -1 is valid user input. Reserve does not change websocket value.
So when we Release the autogenerated port that we created because -1 was set, shouldn't we set it back to 0 just like it would have been before we decided to set to -1 and set the Generated flag?
We autogenerate only because initial value is -1 which means 'autogenerate' by convention. So if flag is set we should revert back that is set websocket to -1.
Avoids other code that then may *print* the websocket as -1...
websocket -1 is valid xml telling websocket should be autogenerated. Nikolay
On 12/09/2016 01:51 AM, Nikolay Shirokovskiy wrote:
On 08.12.2016 23:07, John Ferlan wrote:
On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
We need extra state variable to distinguish between autogenerated and user defined cases after auto generation is done. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 541b600..9bc4522 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1468,6 +1468,7 @@ struct _virDomainGraphicsDef { int port; bool portReserved; int websocket; + bool websocketGenerated; bool autoport; char *keymap; virDomainGraphicsAuthDef auth; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6aaaa10..1799f33 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3574,6 +3574,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver, if (virPortAllocatorAcquire(driver->webSocketPorts, &port) < 0) return -1; graphics->data.vnc.websocket = port; + graphics->data.vnc.websocketGenerated = true; }
return 0; @@ -4065,6 +4066,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver, return -1; graphics->data.vnc.portReserved = true; } + if (graphics->data.vnc.websocket != -1 && /* auto websocket */ + graphics->data.vnc.websocket && /* no websocket */ + virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.vnc.websocket, + true) < 0) + return -1; break;
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: @@ -6189,8 +6196,16 @@ void qemuProcessStop(virQEMUDriverPtr driver, false); graphics->data.vnc.portReserved = false; } - virPortAllocatorRelease(driver->webSocketPorts, - graphics->data.vnc.websocket); + if (graphics->data.vnc.websocketGenerated) { + virPortAllocatorRelease(driver->webSocketPorts, + graphics->data.vnc.websocket); + graphics->data.vnc.websocketGenerated = false; + graphics->data.vnc.websocket = -1;
One more question... Should this be 0 instead of -1?
We set to -1 during Reserve and set the Generated flag indicating that the user didn't set to -1, but we did.
Not quite. -1 is valid user input. Reserve does not change websocket value.
Oh right - I think I was crossing thoughts and thinking setting to -1 was internally done... The series is now pushed Tks, John
So when we Release the autogenerated port that we created because -1 was set, shouldn't we set it back to 0 just like it would have been before we decided to set to -1 and set the Generated flag?
We autogenerate only because initial value is -1 which means 'autogenerate' by convention. So if flag is set we should revert back that is set websocket to -1.
Avoids other code that then may *print* the websocket as -1...
websocket -1 is valid xml telling websocket should be autogenerated.
Nikolay
On 09.12.2016 15:57, John Ferlan wrote:
On 12/09/2016 01:51 AM, Nikolay Shirokovskiy wrote:
On 08.12.2016 23:07, John Ferlan wrote:
On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
We need extra state variable to distinguish between autogenerated and user defined cases after auto generation is done. --- src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 541b600..9bc4522 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1468,6 +1468,7 @@ struct _virDomainGraphicsDef { int port; bool portReserved; int websocket; + bool websocketGenerated; bool autoport; char *keymap; virDomainGraphicsAuthDef auth; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6aaaa10..1799f33 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3574,6 +3574,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver, if (virPortAllocatorAcquire(driver->webSocketPorts, &port) < 0) return -1; graphics->data.vnc.websocket = port; + graphics->data.vnc.websocketGenerated = true; }
return 0; @@ -4065,6 +4066,12 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver, return -1; graphics->data.vnc.portReserved = true; } + if (graphics->data.vnc.websocket != -1 && /* auto websocket */ + graphics->data.vnc.websocket && /* no websocket */ + virPortAllocatorSetUsed(driver->remotePorts, + graphics->data.vnc.websocket, + true) < 0) + return -1; break;
case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: @@ -6189,8 +6196,16 @@ void qemuProcessStop(virQEMUDriverPtr driver, false); graphics->data.vnc.portReserved = false; } - virPortAllocatorRelease(driver->webSocketPorts, - graphics->data.vnc.websocket); + if (graphics->data.vnc.websocketGenerated) { + virPortAllocatorRelease(driver->webSocketPorts, + graphics->data.vnc.websocket); + graphics->data.vnc.websocketGenerated = false; + graphics->data.vnc.websocket = -1;
One more question... Should this be 0 instead of -1?
We set to -1 during Reserve and set the Generated flag indicating that the user didn't set to -1, but we did.
Not quite. -1 is valid user input. Reserve does not change websocket value.
Oh right - I think I was crossing thoughts and thinking setting to -1 was internally done...
The series is now pushed
Tks,
John
Thankx!
--- src/conf/domain_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e008e2..fb6ff0b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22534,7 +22534,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " autoport='%s'", def->data.vnc.autoport ? "yes" : "no"); - if (def->data.vnc.websocket) + if (def->data.vnc.websocketGenerated && + (flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) + virBufferAddLit(buf, " websocket='-1'"); + else if (def->data.vnc.websocket) virBufferAsprintf(buf, " websocket='%d'", def->data.vnc.websocket); virDomainGraphicsListenDefFormatAddr(buf, glisten, flags); -- 1.8.3.1
Perhaps a commit message would answer my question below... On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
--- src/conf/domain_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e008e2..fb6ff0b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22534,7 +22534,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " autoport='%s'", def->data.vnc.autoport ? "yes" : "no");
- if (def->data.vnc.websocket) + if (def->data.vnc.websocketGenerated &&
Wouldn't websocketGenerated imply an active domain? And a change of the websocket in the active xml to be some value? So is the purpose of this because if you have an active domain you don't want to display the websocket that was generated? And why is that? IOW: What are you trying to ensure with this patch? John
+ (flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) + virBufferAddLit(buf, " websocket='-1'"); + else if (def->data.vnc.websocket) virBufferAsprintf(buf, " websocket='%d'", def->data.vnc.websocket);
virDomainGraphicsListenDefFormatAddr(buf, glisten, flags);
On 08.12.2016 16:21, John Ferlan wrote:
Perhaps a commit message would answer my question below...
On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
--- src/conf/domain_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e008e2..fb6ff0b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22534,7 +22534,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " autoport='%s'", def->data.vnc.autoport ? "yes" : "no");
- if (def->data.vnc.websocket) + if (def->data.vnc.websocketGenerated &&
Wouldn't websocketGenerated imply an active domain? And a change of the websocket in the active xml to be some value?
So is the purpose of this because if you have an active domain you don't want to display the websocket that was generated?
And why is that?
IOW: What are you trying to ensure with this patch?
AFAIU this combination - active domain with inactive dump flag is used to serialize config in situations described in cover letter - migration or saving of a domain. So instead of saving port we save the fact it is autogenerated and regenerate on migration destination/restore. One can infer this from socket port logic in near by code. Nikolay
On 12/08/2016 09:00 AM, Nikolay Shirokovskiy wrote:
On 08.12.2016 16:21, John Ferlan wrote:
Perhaps a commit message would answer my question below...
On 11/22/2016 06:09 AM, Nikolay Shirokovskiy wrote:
--- src/conf/domain_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e008e2..fb6ff0b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22534,7 +22534,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " autoport='%s'", def->data.vnc.autoport ? "yes" : "no");
- if (def->data.vnc.websocket) + if (def->data.vnc.websocketGenerated &&
Wouldn't websocketGenerated imply an active domain? And a change of the websocket in the active xml to be some value?
So is the purpose of this because if you have an active domain you don't want to display the websocket that was generated?
And why is that?
IOW: What are you trying to ensure with this patch?
AFAIU this combination - active domain with inactive dump flag is used to serialize config in situations described in cover letter - migration or saving of a domain. So instead of saving port we save the fact it is autogenerated and regenerate on migration destination/restore. One can infer this from socket port logic in near by code.
Oh - didn't read the cover.... I can move that explanation in here before pushing. Although I'll wait for a response to my recent question in 2/3 before doing so. John
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy