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

2011-10-18 Thread 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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2011-10-18 Thread Peter Krempa 

On 10/18/2011 12:23 PM, Daniel P. Berrange wrote:



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

Yes, memory is being held allocated indefinitly after the first console
connection is made. I would not describe it as a leak, as if a new console
is opened or the domain object is freed, the old reference is free'd.
So no buildup of this memory is possible after the first connection is 
made.

A possible solution to this (if allocating some extra bytes of memory is a
big issue), would be to internally add another callback to the fdstream,
and register a handler to it, that would free the reference to the stream
object and reset the pointer needed to detect a live session.
Otherwise I don't see any other possibility how to detect if the client
has disconnected and thus removing the stream object.

Peter

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2011-10-12 Thread Peter Krempa 
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;
+
 ret = 0;
 cleanup:
 if (vm)
-- 
1.7.3.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list