On Fri, Nov 22, 2013 at 10:24:17AM +0000, Daniel P. Berrange wrote:
> On Fri, Nov 22, 2013 at 06:01:44AM +0100, Martin Kletzander wrote:
> > There are few places where we know about port getting opened even
> > though it was not allocated on any virPortAllocator (or we know it was
> > reserved like this). We cannot notify the virPortAllocator that the
> > port is open because of that (since the port may be out of range) and
> > in that case, we are stuck with old data.
> >
> > This also fixes the issue that if you have autoport='no' and
> > port='-1', the port never gets returned (introduced by 7b4a630484);
> > the case when we don't know whether to release the port or not.
> >
> > There may be also other places where the port is being released even
> > though it might have been set statically out of range by the user. In
> > that case we report an error, but that shouldn't stop APIs releasing
> > ports (e.g. destroy). Moreover, the return value is not checked
> > sometimes and we're stuck with an error in the logs and also set as a
> > lastError.
> >
> > To make this behave the best I can imagine, we'd have to have a single
> > virPortAllocator with configurable ranges (ports shared between
> > allocators), but that's an idea for another day. In this patch, I'm
> > modifyping the virPortAllocatorRelease() function to be safe for all
> > kinds of parameters, making it a no-op in case of out-of-range ports
> > and warning in case virBitmapClearBit() returns an error (which is
> > just a sanity code in combination with the range check).
> >
> > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> > ---
> > src/libxl/libxl_driver.c | 13 ++++---------
> > src/qemu/qemu_process.c | 32 +++++++++++++++++++++-----------
> > src/util/virportallocator.c | 33 +++++++++------------------------
> > src/util/virportallocator.h | 4 ++--
> > tests/virportallocatortest.c | 5 +----
> > 5 files changed, 37 insertions(+), 50 deletions(-)
> >
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> > index 1b42f14..26e2d3a 100644
> > --- a/src/libxl/libxl_driver.c
> > +++ b/src/libxl/libxl_driver.c
> > @@ -279,15 +279,10 @@ libxlVmCleanup(libxlDriverPrivatePtr driver,
> > if (virAtomicIntDecAndTest(&driver->nactive) && driver->inhibitCallback)
> > driver->inhibitCallback(false, driver->inhibitOpaque);
> >
> > - if ((vm->def->ngraphics == 1) &&
> > - vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
> > - vm->def->graphics[0]->data.vnc.autoport) {
> > - vnc_port = vm->def->graphics[0]->data.vnc.port;
> > - if (vnc_port >= LIBXL_VNC_PORT_MIN) {
> > - if (virPortAllocatorRelease(driver->reservedVNCPorts,
> > - vnc_port) < 0)
> > - VIR_DEBUG("Could not mark port %d as unused", vnc_port);
> > - }
> > + if (vm->def->ngraphics == 1 &&
> > + vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) {
> > + virPortAllocatorRelease(driver->reservedVNCPorts,
> > + vm->def->graphics[0]->data.vnc.port);
> > }
>
> I don't think this change or the others that follow are safe.
>
> Consider we have guests, A & B.
>
> Start A which is using autoport and it gets given port 5900
> and this is recorded in the port allocator database.
>
> Now start B which has static port, also 5900, and this is
> *not* recorded in the port allocator database.
>
> Now B tries to start up and fails, due to clashing port, so now
> we run the cleanup code. Your change means we release port 5900,
> which is the port belonging to A, not B.
>
Oh, this is a case I haven't considered, right. So I think I'll need
to come up with the shared port allocator and it will have a hint to
who it currently using the code (in order to know whether to release
it or not).
Martin
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list