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

[Daniel P. Berrange]

On Wed, Nov 13, 2013 at 05:48:28PM +0100, Ján Tomko wrote:
> 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?

Sure, sounds good.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|