[libvirt] [PATCH 2/2] graphics: remember graphics not auto allocated ports

[Giuseppe Scrivano]

Ján Tomko <jtomko at redhat.com> writes:

> On 06/23/2014 08:15 PM, Giuseppe Scrivano wrote:
>> When looking for a port to allocate, the port allocator didn't take in
>> consideration ports that are statically set by the user.  Defining
>> these two graphics elements in the XML would cause an error, as the
>> port allocator would try to use the same port for the spice graphics
>> element:
>> 
>>     <graphics type='spice' autoport='yes'/>
>>     <graphics type='vnc' port='5900' autoport='no'/>
>> 
>> The new *[pP]ortAllocated variables keep track of the ports that were
>> successfully registered (either bound or simply tracked as used) by
>> the port allocator to allow a clean rollback on errors.
>> 
>> Closes: https://bugzilla.redhat.com/show_bug.cgi?id=1081881
>> 
>> Signed-off-by: Giuseppe Scrivano <gscrivan at redhat.com>
>> ---
>>  src/conf/domain_conf.h  |  3 ++
>>  src/qemu/qemu_process.c | 79 +++++++++++++++++++++++++++++++++++++++++++------
>>  2 files changed, 73 insertions(+), 9 deletions(-)
>> 
>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 0b8155b..06f1e54 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -3487,6 +3487,7 @@ qemuProcessVNCAllocatePorts(virQEMUDriverPtr driver,
>>          if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0)
>>              return -1;
>>          graphics->data.vnc.port = port;
>> +        graphics->data.vnc.portAllocated = true;
>>      }
>>  
>>      if (graphics->data.vnc.websocket == -1) {
>> @@ -3548,6 +3549,7 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
>>              goto error;
>>  
>>          graphics->data.spice.port = port;
>> +        graphics->data.spice.portAllocated = true;
>>      }
>>  
>>      if (needTLSPort || graphics->data.spice.tlsPort == -1) {
>> @@ -3573,6 +3575,7 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
>>                  goto error;
>>  
>>              graphics->data.spice.tlsPort = tlsPort;
>> +            graphics->data.spice.tlsPortAllocated = true;
>>          }
>>      }
>>  
>> @@ -3803,6 +3806,36 @@ int qemuProcessStart(virConnectPtr conn,
>>  
>>      for (i = 0; i < vm->def->ngraphics; ++i) {
>>          virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
>> +        if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
>> +            !graphics->data.vnc.autoport) {
>> +            if (virPortAllocatorSetUsed(driver->remotePorts,
>> +                                        graphics->data.vnc.port,
>> +                                        true) < 0) {
>
> virPortAllocatorSetUsed doesn't error out if the static port is already marked
> as used. If it's already used by another domain, this one fails to start and
> releases the port in qemuProcessStop.
>
> If you change it to fail on occupied ports, it will also catch duplicit static
> ports, but only in the port allocator range. I expect someone will file a bug
> about conflicting ports out of that range as well :)

sorry, it had to be failing on occupied ports, I'll change it in v2.

>
>> +                goto cleanup;
>> +            }
>> +
>> +            graphics->data.vnc.portAllocated = true;
>> +
>> +        } else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
>> +                   !graphics->data.spice.autoport) {
>
> If either of the ports is -1, you shouldn't be reserving them. (For VNC, we
> set autoport to true if port is -1 when parsing the XML. For SPICE, this only
> happens if both ports are -1).

I will modify it to reserve a port only if !autoport && port != -1 and
fix the release part too.

Thanks,
Giuseppe