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@xxxxxxxxxx> > --- > 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 :) > + 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). > + if (virPortAllocatorSetUsed(driver->remotePorts, > + graphics->data.spice.port, > + true) < 0) > + goto cleanup; > + > + graphics->data.spice.portAllocated = true; > + > + if (virPortAllocatorSetUsed(driver->remotePorts, > + graphics->data.spice.tlsPort, > + true) < 0) > + goto cleanup; > + > + graphics->data.spice.tlsPortAllocated = true; > + } > + } > + > + for (i = 0; i < vm->def->ngraphics; ++i) { > + virDomainGraphicsDefPtr graphics = vm->def->graphics[i]; > if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { > if (qemuProcessVNCAllocatePorts(driver, graphics) < 0) > goto cleanup; > @@ -4509,19 +4542,47 @@ 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(driver->remotePorts, > - graphics->data.vnc.port); > + if (graphics->data.vnc.portAllocated) { > + if (graphics->data.vnc.autoport) { > + virPortAllocatorRelease(driver->remotePorts, > + graphics->data.vnc.port); > + } else { > + virPortAllocatorSetUsed(driver->remotePorts, > + graphics->data.vnc.port, > + false); > + } > + graphics->data.vnc.portAllocated = false; > } > + > virPortAllocatorRelease(driver->webSocketPorts, > graphics->data.vnc.websocket); > } > - if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE && > - graphics->data.spice.autoport) { > - virPortAllocatorRelease(driver->remotePorts, > - graphics->data.spice.port); > - virPortAllocatorRelease(driver->remotePorts, > - graphics->data.spice.tlsPort); > + if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { > + if (graphics->data.spice.autoport) { We allocate a port for SPICE even if autoport is no, but either port or tlsPort is -1. This would fix the bug of not releasing the port. > + if (graphics->data.spice.portAllocated) { > + virPortAllocatorRelease(driver->remotePorts, > + graphics->data.spice.port); > + graphics->data.spice.portAllocated = false; > + } > + if (graphics->data.spice.tlsPortAllocated) { > + virPortAllocatorRelease(driver->remotePorts, > + graphics->data.spice.tlsPort); > + graphics->data.spice.tlsPortAllocated = false; > + } If we could make virPortAllocatorRelease silently ignore ports out of range, the else branch would not be necessary and so would the bool parameter of 'SetUsed'. > + } else { > + if (graphics->data.spice.portAllocated) { > + virPortAllocatorSetUsed(driver->remotePorts, > + graphics->data.spice.port, > + false); > + graphics->data.spice.portAllocated = false; > + } > + if (graphics->data.spice.tlsPortAllocated) { > + virPortAllocatorSetUsed(driver->remotePorts, > + graphics->data.spice.tlsPort, > + false); > + graphics->data.spice.tlsPortAllocated = false; > + } > + } > } > } > > Jan
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list