[libvirt] [PATCH 08/10] qemu: Add ability to abort existing console while creating new one

[Daniel P. Berrange]

On Wed, Oct 12, 2011 at 03:43:18PM +0200, Peter Krempa wrote:
> This patch fixes console corruption, that happens if two concurrent
> sessions are opened for a single console on a domain. Result of this
> corruption was, that each of the console streams did recieve just a part
> of the data written to the pipe so every console rendered unusable.
> 
> This patch adds a check that verifies if an open console exists and
> notifies the user. This patch also adds support for the flag
> VIR_DOMAIN_CONSOLE_FORCE that interrupts the existing session before
> creating a new one. This serves as a fallback if an AFK admin holds the
> console or a connection breaks.
> ---
>  src/qemu/qemu_domain.c |    3 +++
>  src/qemu/qemu_domain.h |    2 ++
>  src/qemu/qemu_driver.c |   23 ++++++++++++++++++++++-
>  3 files changed, 27 insertions(+), 1 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 85bebd6..be316b6 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -233,6 +233,9 @@ static void qemuDomainObjPrivateFree(void *data)
>      VIR_FREE(priv->lockState);
>      VIR_FREE(priv->origname);
> 
> +    if (priv->console)
> +        virStreamFree(priv->console);
> +
>      /* This should never be non-NULL if we get here, but just in case... */
>      if (priv->mon) {
>          VIR_ERROR(_("Unexpected QEMU monitor still active during domain deletion"));
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index d9f323c..e32f4ef 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -127,6 +127,8 @@ struct _qemuDomainObjPrivate {
> 
>      unsigned long migMaxBandwidth;
>      char *origname;
> +
> +    virStreamPtr console;
>  };
> 
>  struct qemuDomainWatchdogEvent
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ec01cd5..3b0cb70 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10394,8 +10394,10 @@ qemuDomainOpenConsole(virDomainPtr dom,
>      int ret = -1;
>      int i;
>      virDomainChrDefPtr chr = NULL;
> +    qemuDomainObjPrivatePtr priv;
> 
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_DOMAIN_CONSOLE_TRY |
> +                  VIR_DOMAIN_CONSOLE_FORCE, -1);
> 
>      qemuDriverLock(driver);
>      virUUIDFormat(dom->uuid, uuidstr);
> @@ -10412,6 +10414,8 @@ qemuDomainOpenConsole(virDomainPtr dom,
>          goto cleanup;
>      }
> 
> +    priv = vm->privateData;
> +
>      if (dev_name) {
>          if (vm->def->console &&
>              STREQ(dev_name, vm->def->console->info.alias))
> @@ -10445,10 +10449,27 @@ qemuDomainOpenConsole(virDomainPtr dom,
>          goto cleanup;
>      }
> 
> +    /* kill open console stream */
> +    if (priv->console) {
> +        if (virFDStreamIsOpen(priv->console) &&
> +            !(flags & VIR_DOMAIN_CONSOLE_FORCE)) {
> +            qemuReportError(VIR_ERR_OPERATION_FAILED,
> +                            _("Active console session exists for this domain"));
> +            goto cleanup;
> +        }
> +
> +        virStreamAbort(priv->console);
> +        virStreamFree(priv->console);
> +        priv->console = NULL;
> +    }
> +
>      if (virFDStreamOpenFile(st, chr->source.data.file.path,
>                              0, 0, O_RDWR) < 0)
>          goto cleanup;
> 
> +    virStreamRef(st);
> +    priv->console = st;
> +


Hmm, I have a feeling this will cause a leak. Currently the virStreamPtr
reference is held by the daemon itself, and when the client disconnects
it free's the stream. With this extra reference held by the QEMU driver,
the virStreamPtr will live forever if a client disconnects, without
closing its stream. In fact I think it'll live forever even if the client
does an explicit finish/abort, since you only seem to release the reference
when the virDomainObjPtr is free'd, or when another console is opened.

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 :|