[libvirt] [PATCH 2/5] qemu: simplify graphics port releasing
Nikolay Shirokovskiy
nshirokovskiy at virtuozzo.com
Wed Jul 18 07:21:25 UTC 2018
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 at 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
>
>> }
>> }
>> }
>>
More information about the libvir-list
mailing list