Re: [libvirt] [PATCH 2/2] Return -1 in virPortAllocatorAcquire if all ports are used

On 11/13/2013 05:39 PM, Daniel P. Berrange wrote: > On Thu, Oct 31, 2013 at 01:07:27PM +0100, Ján Tomko wrote: >> Report the error in virPortAllocatorAcquire instead >> of doing it in every caller. >> >> The error contains the port range name instead of the intended >> use for the port, e.g.: >> Unable to find an unused port in range 'display' (65534-65535) >> instead of: >> Unable to find an unused port for SPICE >> >> This also adds error reporting when the QEMU driver could not >> find an unused port for VNC, VNC WebSockets or NBD migration. >> --- >> src/libxl/libxl_conf.c | 5 ----- >> src/qemu/qemu_migration.c | 16 ++-------------- >> src/qemu/qemu_process.c | 12 ------------ >> src/util/virportallocator.c | 7 ++++++- >> tests/virportallocatortest.c | 6 ++---- >> 5 files changed, 10 insertions(+), 36 deletions(-) >> >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >> index bdffdf8..daf081d 100644 >> --- a/src/qemu/qemu_process.c >> +++ b/src/qemu/qemu_process.c >> @@ -3406,12 +3406,6 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, >> if (virPortAllocatorAcquire(driver->remotePorts, &port) < 0) >> goto error; >> >> - if (port == 0) { >> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >> - _("Unable to find an unused port for SPICE")); >> - goto error; >> - } >> - >> graphics->data.spice.port = port; >> } >> >> @@ -3437,12 +3431,6 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver, >> if (virPortAllocatorAcquire(driver->remotePorts, &tlsPort) < 0) >> goto error; >> >> - if (tlsPort == 0) { >> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", >> - _("Unable to find an unused port for SPICE TLS")); >> - virPortAllocatorRelease(driver->remotePorts, port); >> - goto error; >> - } > > Opps, you're loosing the release of 'port' when tlsPort allocation fails. > In fact there was already a broken codepath in this respect. > 'port' is released on the error: label. Should I move the double-release removal into a separate commit to make it more obvious?