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.

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...

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

--- 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);

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.