On Fri, Nov 22, 2013 at 15:41:28 +0100, Martin Kletzander wrote:
> 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).
However, we sometimes (during incoming migration) allocate a port in one
thread and release it in another thread... :-)
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list