[libvirt] [PATCH 00/10] Console coruption with two or more clients series
This series fixes anoying console corruption if two clients try to connect at same time to the console. The current state of this is, that two/more of libvirt iohelpers are spawned on the same time that compete for data from the pty. This causes that each of the consoles get scrambled and unusable. Patches fdstream: Emit stream abort callback even if poll() doesnt. virnetclientstream: Propagate stream error messages to callback daemon: Subscribe the stream event callback for error events. add the ability to abort a stream from the daemon side. fdstream: Add internal function to check if a fdstream is open This patch adds a helper function that checks internally if a fdstream is open and working. virsh: fix console stream error reporting Add flags for virDomainOpenConsole virsh: add support for VIR_DOMAIN_CONSOLE_FORCE flag This patches add instrumentation to virsh that supports the new ability to abort streams from the daemon side and adapts the console callback to handle this. These patches also add a new flag set for virDomainOpenConsole API call that allow users to control the if the existing console connection should be left in place or killed in favor of the new one qemu: Add ability to abort existing console while creating new one lxc: Add ability to abort existing console when creating a new one uml: Add ability to abort existing console when creating a new one These patches modify the hypervisor drivers so that they are aware of existing console connections and refuse to create a new one (or kill the old). The xen driver also supports console in a similar way like the previously mentioned drivers, but lacks the means to store a stream reference permanently. I'll look in if it's possible to modify this driver to support this new functionality. For convinience, to review these patches: git checkout -b console_corruption 33b55fd85ae5435bda53c3cfcbe1385074befd01 git pull git://aeon.pipo.sk/libvirt.git console_corruption Peter Peter Krempa (10): fdstream: Emit stream abort callback even if poll() doesnt. virnetclientstream: Propagate stream error messages to callback daemon: Subscribe the stream event callback for error events. fdstream: Add internal function to check if a fdstream is open virsh: fix console stream error reporting Add flags for virDomainOpenConsole virsh: add support for VIR_DOMAIN_CONSOLE_FORCE flag qemu: Add ability to abort existing console while creating new one lxc: Add ability to abort existing console when creating a new one uml: Add ability to abort existing console when creating a new one daemon/stream.c | 2 +- include/libvirt/libvirt.h.in | 12 +++++++- src/fdstream.c | 61 ++++++++++++++++++++++++++++++++++++++--- src/fdstream.h | 2 + src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 28 ++++++++++++++++++- src/qemu/qemu_domain.c | 3 ++ src/qemu/qemu_domain.h | 2 + src/qemu/qemu_driver.c | 23 +++++++++++++++- src/rpc/virnetclientstream.c | 12 +++++--- src/uml/uml_driver.c | 28 ++++++++++++++++++- tools/console.c | 27 ++++++++++++------ tools/console.h | 2 +- tools/virsh.c | 18 +++++++++--- 14 files changed, 192 insertions(+), 29 deletions(-) -- 1.7.3.4
This patch causes the fdstream driver to call the stream event callback if virStreamAbort() is issued on a stream using this driver. This prohibited to abort streams from the daemon, as the daemon remote handler installs a callback to watch for stream errors as the only mean of detecting changes in the stream. --- src/fdstream.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index b60162c..8fb5e78 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -55,6 +55,7 @@ struct virFDStreamData { unsigned long long length; int watch; + int events; /* events the stream callback is subscribed for */ bool cbRemoved; bool dispatching; bool closed; @@ -62,6 +63,9 @@ struct virFDStreamData { void *opaque; virFreeCallback ff; + /* don't call the abort callback more than once */ + bool abortCallbackCalled; + virMutex lock; }; @@ -92,6 +96,7 @@ static int virFDStreamRemoveCallback(virStreamPtr stream) fdst->watch = 0; fdst->ff = NULL; fdst->cb = NULL; + fdst->events = 0; fdst->opaque = NULL; ret = 0; @@ -120,6 +125,7 @@ static int virFDStreamUpdateCallback(virStreamPtr stream, int events) } virEventUpdateHandle(fdst->watch, events); + fdst->events = events; ret = 0; @@ -214,6 +220,8 @@ virFDStreamAddCallback(virStreamPtr st, fdst->cb = cb; fdst->opaque = opaque; fdst->ff = ff; + fdst->events = events; + fdst->abortCallbackCalled = false; virStreamRef(st); ret = 0; @@ -223,18 +231,40 @@ cleanup: return ret; } - static int -virFDStreamClose(virStreamPtr st) +virFDStreamCloseInt(virStreamPtr st, bool streamAbort) { - struct virFDStreamData *fdst = st->privateData; + struct virFDStreamData *fdst; int ret; VIR_DEBUG("st=%p", st); - if (!fdst) + if (!st || !(fdst = st->privateData)) return 0; + /* aborting the stream, ensure the callback is called if it's + * registered for stream error event */ + if (streamAbort && + fdst->cb && + (fdst->events & VIR_STREAM_EVENT_ERROR)) { + /* don't enter this function accidentaly from the callback again */ + virMutexLock(&fdst->lock); + if (fdst->abortCallbackCalled) { + virMutexUnlock(&fdst->lock); + return 0; + } + + fdst->abortCallbackCalled = true; + virMutexUnlock(&fdst->lock); + + /* call failure callback, poll does report nothing on closed fd */ + (fdst->cb)(st, VIR_STREAM_EVENT_ERROR, fdst->opaque); + + /* re-check the pointer, someone could tamper with it in the callback */ + if (!(fdst = st->privateData)) + return 0; + } + virMutexLock(&fdst->lock); ret = VIR_CLOSE(fdst->fd); @@ -282,6 +312,18 @@ virFDStreamClose(virStreamPtr st) return ret; } +static int +virFDStreamClose(virStreamPtr st) +{ + return virFDStreamCloseInt(st, false); +} + +static int +virFDStreamAbort(virStreamPtr st) +{ + return virFDStreamCloseInt(st, true); +} + static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes) { struct virFDStreamData *fdst = st->privateData; @@ -388,7 +430,7 @@ static virStreamDriver virFDStreamDrv = { .streamSend = virFDStreamWrite, .streamRecv = virFDStreamRead, .streamFinish = virFDStreamClose, - .streamAbort = virFDStreamClose, + .streamAbort = virFDStreamAbort, .streamAddCallback = virFDStreamAddCallback, .streamUpdateCallback = virFDStreamUpdateCallback, .streamRemoveCallback = virFDStreamRemoveCallback -- 1.7.3.4
On Wed, Oct 12, 2011 at 03:43:11PM +0200, Peter Krempa wrote:
This patch causes the fdstream driver to call the stream event callback if virStreamAbort() is issued on a stream using this driver. This prohibited to abort streams from the daemon, as the daemon remote handler installs a callback to watch for stream errors as the only mean of detecting changes in the stream. --- src/fdstream.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 47 insertions(+), 5 deletions(-)
diff --git a/src/fdstream.c b/src/fdstream.c index b60162c..8fb5e78 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -55,6 +55,7 @@ struct virFDStreamData { unsigned long long length;
int watch; + int events; /* events the stream callback is subscribed for */ bool cbRemoved; bool dispatching; bool closed; @@ -62,6 +63,9 @@ struct virFDStreamData { void *opaque; virFreeCallback ff;
+ /* don't call the abort callback more than once */ + bool abortCallbackCalled; + virMutex lock; };
@@ -92,6 +96,7 @@ static int virFDStreamRemoveCallback(virStreamPtr stream) fdst->watch = 0; fdst->ff = NULL; fdst->cb = NULL; + fdst->events = 0; fdst->opaque = NULL;
ret = 0; @@ -120,6 +125,7 @@ static int virFDStreamUpdateCallback(virStreamPtr stream, int events) }
virEventUpdateHandle(fdst->watch, events); + fdst->events = events;
ret = 0;
@@ -214,6 +220,8 @@ virFDStreamAddCallback(virStreamPtr st, fdst->cb = cb; fdst->opaque = opaque; fdst->ff = ff; + fdst->events = events; + fdst->abortCallbackCalled = false; virStreamRef(st);
ret = 0; @@ -223,18 +231,40 @@ cleanup: return ret; }
- static int -virFDStreamClose(virStreamPtr st) +virFDStreamCloseInt(virStreamPtr st, bool streamAbort) { - struct virFDStreamData *fdst = st->privateData; + struct virFDStreamData *fdst; int ret;
VIR_DEBUG("st=%p", st);
- if (!fdst) + if (!st || !(fdst = st->privateData)) return 0;
+ /* aborting the stream, ensure the callback is called if it's + * registered for stream error event */ + if (streamAbort && + fdst->cb && + (fdst->events & VIR_STREAM_EVENT_ERROR)) {
You should be checking for 'READABLE | WRITABLE' here
+ /* don't enter this function accidentaly from the callback again */ + virMutexLock(&fdst->lock); + if (fdst->abortCallbackCalled) { + virMutexUnlock(&fdst->lock); + return 0; + } + + fdst->abortCallbackCalled = true; + virMutexUnlock(&fdst->lock); + + /* call failure callback, poll does report nothing on closed fd */ + (fdst->cb)(st, VIR_STREAM_EVENT_ERROR, fdst->opaque);
Regards, 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 :|
If a stream notification message arives from the daemon side, the event dispatcher only sets the error state for the stream but does not emit the stream error event and the corresponding callback is not called. This patch adds the emision of the event in the cause a stream error (abortion) happens and the user may act on this using the event callback. --- src/rpc/virnetclientstream.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c index 7e2d9ae..9049659 100644 --- a/src/rpc/virnetclientstream.c +++ b/src/rpc/virnetclientstream.c @@ -65,7 +65,6 @@ struct _virNetClientStream { int cbDispatch; }; - static void virNetClientStreamEventTimerUpdate(virNetClientStreamPtr st) { @@ -76,7 +75,8 @@ virNetClientStreamEventTimerUpdate(virNetClientStreamPtr st) if (((st->incomingOffset || st->incomingEOF) && (st->cbEvents & VIR_STREAM_EVENT_READABLE)) || - (st->cbEvents & VIR_STREAM_EVENT_WRITABLE)) { + (st->cbEvents & VIR_STREAM_EVENT_WRITABLE) || + (st->cbEvents & VIR_STREAM_EVENT_ERROR && st->err.code != VIR_ERR_OK)) { VIR_DEBUG("Enabling event timer"); virEventUpdateTimeout(st->cbTimer, 0); } else { @@ -85,7 +85,6 @@ virNetClientStreamEventTimerUpdate(virNetClientStreamPtr st) } } - static void virNetClientStreamEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque) { @@ -102,6 +101,10 @@ virNetClientStreamEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque) if (st->cb && (st->cbEvents & VIR_STREAM_EVENT_WRITABLE)) events |= VIR_STREAM_EVENT_WRITABLE; + if (st->cb && + (st->cbEvents & VIR_STREAM_EVENT_ERROR) && + st->err.code != VIR_ERR_OK) + events |= VIR_STREAM_EVENT_ERROR; VIR_DEBUG("Got Timer dispatch %d %d offset=%zu", events, st->cbEvents, st->incomingOffset); if (events) { @@ -121,7 +124,6 @@ virNetClientStreamEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque) virMutexUnlock(&st->lock); } - static void virNetClientStreamEventTimerFree(void *opaque) { @@ -276,6 +278,8 @@ int virNetClientStreamSetError(virNetClientStreamPtr st, cleanup: xdr_free((xdrproc_t)xdr_virNetMessageError, (void*)&err); virMutexUnlock(&st->lock); + virNetClientStreamEventTimerUpdate(st); + return ret; } -- 1.7.3.4
On Wed, Oct 12, 2011 at 03:43:12PM +0200, Peter Krempa wrote:
If a stream notification message arives from the daemon side, the event dispatcher only sets the error state for the stream but does not emit the stream error event and the corresponding callback is not called.
This patch adds the emision of the event in the cause a stream error (abortion) happens and the user may act on this using the event callback. --- src/rpc/virnetclientstream.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c index 7e2d9ae..9049659 100644 --- a/src/rpc/virnetclientstream.c +++ b/src/rpc/virnetclientstream.c @@ -65,7 +65,6 @@ struct _virNetClientStream { int cbDispatch; };
- static void virNetClientStreamEventTimerUpdate(virNetClientStreamPtr st) { @@ -76,7 +75,8 @@ virNetClientStreamEventTimerUpdate(virNetClientStreamPtr st)
if (((st->incomingOffset || st->incomingEOF) && (st->cbEvents & VIR_STREAM_EVENT_READABLE)) || - (st->cbEvents & VIR_STREAM_EVENT_WRITABLE)) { + (st->cbEvents & VIR_STREAM_EVENT_WRITABLE) || + (st->cbEvents & VIR_STREAM_EVENT_ERROR && st->err.code != VIR_ERR_OK)) { VIR_DEBUG("Enabling event timer"); virEventUpdateTimeout(st->cbTimer, 0); } else { @@ -85,7 +85,6 @@ virNetClientStreamEventTimerUpdate(virNetClientStreamPtr st) } }
- static void virNetClientStreamEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque) { @@ -102,6 +101,10 @@ virNetClientStreamEventTimer(int timer ATTRIBUTE_UNUSED, void *opaque) if (st->cb && (st->cbEvents & VIR_STREAM_EVENT_WRITABLE)) events |= VIR_STREAM_EVENT_WRITABLE; + if (st->cb && + (st->cbEvents & VIR_STREAM_EVENT_ERROR) && + st->err.code != VIR_ERR_OK) + events |= VIR_STREAM_EVENT_ERROR;
VIR_DEBUG("Got Timer dispatch %d %d offset=%zu", events, st->cbEvents, st->incomingOffset); if (events) {
You shouldn't be checking 'cbEvents & VIR_STREAM_EVENT_ERROR' (or HANGUP). Both those event types are always enabled, whenever cbEvents is set WRITABLE or READABLE. Regards, 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 :|
This patch subscribes the daemon-side event callback for error and hangup events. The functionality to handle them is already implemented in the callback. --- daemon/stream.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/daemon/stream.c b/daemon/stream.c index 7df9952..5ec899c 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -77,7 +77,7 @@ daemonStreamHandleAbort(virNetServerClientPtr client, static void daemonStreamUpdateEvents(daemonClientStream *stream) { - int newEvents = 0; + int newEvents = VIR_STREAM_EVENT_HANGUP | VIR_STREAM_EVENT_ERROR; if (stream->rx) newEvents |= VIR_STREAM_EVENT_WRITABLE; if (stream->tx && !stream->recvEOF) -- 1.7.3.4
On Wed, Oct 12, 2011 at 03:43:13PM +0200, Peter Krempa wrote:
This patch subscribes the daemon-side event callback for error and hangup events. The functionality to handle them is already implemented in the callback. --- daemon/stream.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/daemon/stream.c b/daemon/stream.c index 7df9952..5ec899c 100644 --- a/daemon/stream.c +++ b/daemon/stream.c @@ -77,7 +77,7 @@ daemonStreamHandleAbort(virNetServerClientPtr client, static void daemonStreamUpdateEvents(daemonClientStream *stream) { - int newEvents = 0; + int newEvents = VIR_STREAM_EVENT_HANGUP | VIR_STREAM_EVENT_ERROR; if (stream->rx) newEvents |= VIR_STREAM_EVENT_WRITABLE; if (stream->tx && !stream->recvEOF)
This shouldn't be required. With the event loop HANGUP/ERROR are always implied, whenever you request WRITABLE/READABLE. Regards, 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 :|
This patch adds a new internal api function to check if a fdstream is open. The check is done as an trivial check of private data structures, as if the stream is closed, those don't exist. --- src/fdstream.c | 9 +++++++++ src/fdstream.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 12 insertions(+), 0 deletions(-) diff --git a/src/fdstream.c b/src/fdstream.c index 8fb5e78..2e2f3af 100644 --- a/src/fdstream.c +++ b/src/fdstream.c @@ -674,3 +674,12 @@ int virFDStreamCreateFile(virStreamPtr st, offset, length, oflags | O_CREAT, mode); } + +/* internal function to check if stream is still open */ +int virFDStreamIsOpen(virStreamPtr st) +{ + if (!st || !st->privateData) + return 0; + + return 1; +} diff --git a/src/fdstream.h b/src/fdstream.h index f9edb23..dbea450 100644 --- a/src/fdstream.h +++ b/src/fdstream.h @@ -45,4 +45,6 @@ int virFDStreamCreateFile(virStreamPtr st, int oflags, mode_t mode); +int virFDStreamIsOpen(virStreamPtr st); + #endif /* __VIR_FDSTREAM_H_ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 11ff705..3fa042c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -543,6 +543,7 @@ virFDStreamOpen; virFDStreamConnectUNIX; virFDStreamOpenFile; virFDStreamCreateFile; +virFDStreamIsOpen; # hash.h -- 1.7.3.4
This patch subscribes the console stream event callback to handle errors (and stream abortion) from the daemon. The functionality was (partly) implemented in the callback, but the error events were not registered. --- tools/console.c | 21 ++++++++++++++------- 1 files changed, 14 insertions(+), 7 deletions(-) diff --git a/tools/console.c b/tools/console.c index 0f85bc7..3913c42 100644 --- a/tools/console.c +++ b/tools/console.c @@ -109,6 +109,12 @@ virConsoleEventOnStream(virStreamPtr st, { virConsolePtr con = opaque; + if (events & VIR_STREAM_EVENT_ERROR || + events & VIR_STREAM_EVENT_HANGUP) { + virConsoleShutdown(con); + return; + } + if (events & VIR_STREAM_EVENT_READABLE) { size_t avail = con->streamToTerminal.length - con->streamToTerminal.offset; @@ -169,12 +175,10 @@ virConsoleEventOnStream(virStreamPtr st, } if (!con->terminalToStream.offset) virStreamEventUpdateCallback(con->st, - VIR_STREAM_EVENT_READABLE); + ( VIR_STREAM_EVENT_READABLE | + VIR_STREAM_EVENT_HANGUP | + VIR_STREAM_EVENT_ERROR )); - if (events & VIR_STREAM_EVENT_ERROR || - events & VIR_STREAM_EVENT_HANGUP) { - virConsoleShutdown(con); - } } static void @@ -346,7 +350,9 @@ int vshRunConsole(virDomainPtr dom, const char *dev_name) NULL); virStreamEventAddCallback(con->st, - VIR_STREAM_EVENT_READABLE, + (VIR_STREAM_EVENT_READABLE | + VIR_STREAM_EVENT_HANGUP | + VIR_STREAM_EVENT_ERROR), virConsoleEventOnStream, con, NULL); @@ -356,7 +362,8 @@ int vshRunConsole(virDomainPtr dom, const char *dev_name) break; } - ret = 0; + if (!virGetLastError()) + ret = 0; cleanup: -- 1.7.3.4
On Wed, Oct 12, 2011 at 03:43:15PM +0200, Peter Krempa wrote:
This patch subscribes the console stream event callback to handle errors (and stream abortion) from the daemon. The functionality was (partly) implemented in the callback, but the error events were not registered. --- tools/console.c | 21 ++++++++++++++------- 1 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/tools/console.c b/tools/console.c index 0f85bc7..3913c42 100644 --- a/tools/console.c +++ b/tools/console.c @@ -109,6 +109,12 @@ virConsoleEventOnStream(virStreamPtr st, { virConsolePtr con = opaque;
+ if (events & VIR_STREAM_EVENT_ERROR || + events & VIR_STREAM_EVENT_HANGUP) { + virConsoleShutdown(con); + return; + }
There is already code for this further down in virConsoleEventOnStream. The check for ERROR/HANGUP should come *last* in the method, because you might also have READABLE set at the same time. We need to make sure we read any final data.
if (events & VIR_STREAM_EVENT_READABLE) { size_t avail = con->streamToTerminal.length - con->streamToTerminal.offset; @@ -169,12 +175,10 @@ virConsoleEventOnStream(virStreamPtr st, } if (!con->terminalToStream.offset) virStreamEventUpdateCallback(con->st, - VIR_STREAM_EVENT_READABLE); + ( VIR_STREAM_EVENT_READABLE | + VIR_STREAM_EVENT_HANGUP | + VIR_STREAM_EVENT_ERROR ));
- if (events & VIR_STREAM_EVENT_ERROR || - events & VIR_STREAM_EVENT_HANGUP) { - virConsoleShutdown(con); - } }
static void @@ -346,7 +350,9 @@ int vshRunConsole(virDomainPtr dom, const char *dev_name) NULL);
virStreamEventAddCallback(con->st, - VIR_STREAM_EVENT_READABLE, + (VIR_STREAM_EVENT_READABLE | + VIR_STREAM_EVENT_HANGUP | + VIR_STREAM_EVENT_ERROR), virConsoleEventOnStream, con, NULL);
As with the other patch, this is not required. HANGUP/ERROR are always enabled, if you have READABLE or WRITABLE set.
@@ -356,7 +362,8 @@ int vshRunConsole(virDomainPtr dom, const char *dev_name) break; }
- ret = 0; + if (!virGetLastError()) + ret = 0;
This looks like the only bit we might need, but I'm not entirely convinced. If the user aborts the console with Ctrl-], then I don't think we need to print the error from virStreamAbort. They know they just aborted it 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 :|
This patch adds a set of flags to be used with the virDomainOpenConsole API call to specify if the user wishes to interrupt an existing console session or just to try open a new one. VIR_DOMAIN_CONSOLE_TRY - specifies that the caller wants to try open a new console session if no open sessions exist VIR_DOMAIN_CONSOLE_FORCE - specifies that the caller wishes to interrupt existing session and force a creation of a new one. --- include/libvirt/libvirt.h.in | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index c991dfc..ae31601 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3093,7 +3093,17 @@ int virNWFilterGetUUIDString (virNWFilterPtr nwfilter, char *buf); char * virNWFilterGetXMLDesc (virNWFilterPtr nwfilter, unsigned int flags); - +/** + * virDomainConsoleFlags + * + * Since 0.9.7 + */ +typedef enum { + VIR_DOMAIN_CONSOLE_TRY = 0, /* try to open the console, don't abort active + connection */ + VIR_DOMAIN_CONSOLE_FORCE /* abort a (possibly) active console connection + to force a new connection*/ +} virDomainConsoleFlags; int virDomainOpenConsole(virDomainPtr dom, const char *devname, -- 1.7.3.4
This patch adds support for the newly introduced VIR_DOMAIN_CONSOLE flag. The console command now has an optional parameter --force that specifies that the user wants to forcibly interrupt an ongoing console session and create a new one. The behaviour to this point was, that the daemon openend two streams to the console, that competed for data from the pipe and the result was, that both of the consoles ended up scrambled. --- tools/console.c | 6 ++++-- tools/console.h | 2 +- tools/virsh.c | 18 +++++++++++++----- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/tools/console.c b/tools/console.c index 3913c42..814eacb 100644 --- a/tools/console.c +++ b/tools/console.c @@ -282,7 +282,9 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, } -int vshRunConsole(virDomainPtr dom, const char *dev_name) +int vshRunConsole(virDomainPtr dom, + const char *dev_name, + unsigned int flags) { int ret = -1; struct termios ttyattr, rawattr; @@ -335,7 +337,7 @@ int vshRunConsole(virDomainPtr dom, const char *dev_name) if (!con->st) goto cleanup; - if (virDomainOpenConsole(dom, dev_name, con->st, 0) < 0) + if (virDomainOpenConsole(dom, dev_name, con->st, flags) < 0) goto cleanup; con->stdinWatch = virEventAddHandle(STDIN_FILENO, diff --git a/tools/console.h b/tools/console.h index 9b05ff1..1b4b158 100644 --- a/tools/console.h +++ b/tools/console.h @@ -25,7 +25,7 @@ # ifndef WIN32 -int vshRunConsole(virDomainPtr dom, const char *dev_name); +int vshRunConsole(virDomainPtr dom, const char *dev_name, unsigned int flags); # endif /* !WIN32 */ diff --git a/tools/virsh.c b/tools/virsh.c index bcf0603..3933b8f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -774,11 +774,14 @@ static const vshCmdInfo info_console[] = { static const vshCmdOptDef opts_console[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"devname", VSH_OT_STRING, 0, N_("character device name")}, + {"force", VSH_OT_BOOL, 0, N_("force console connection (disconnect already connected sessions)")}, {NULL, 0, 0, NULL} }; static bool -cmdRunConsole(vshControl *ctl, virDomainPtr dom, const char *name) +cmdRunConsole(vshControl *ctl, virDomainPtr dom, + const char *name, + unsigned int flags) { bool ret = false; int state; @@ -795,7 +798,7 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom, const char *name) vshPrintExtra(ctl, _("Connected to domain %s\n"), virDomainGetName(dom)); vshPrintExtra(ctl, "%s", _("Escape character is ^]\n")); - if (vshRunConsole(dom, name) == 0) + if (vshRunConsole(dom, name, flags) == 0) ret = true; cleanup: @@ -808,6 +811,8 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom; bool ret = false; + bool force = vshCommandOptBool(cmd, "force"); + unsigned int flags = 0; const char *name = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -821,7 +826,10 @@ cmdConsole(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - ret = cmdRunConsole(ctl, dom, name); + if (force) + flags = VIR_DOMAIN_CONSOLE_FORCE; + + ret = cmdRunConsole(ctl, dom, name, flags); cleanup: virDomainFree(dom); @@ -1838,7 +1846,7 @@ cmdCreate(vshControl *ctl, const vshCmd *cmd) virDomainGetName(dom), from); #ifndef WIN32 if (console) - cmdRunConsole(ctl, dom, NULL); + cmdRunConsole(ctl, dom, NULL, 0); #endif virDomainFree(dom); } else { @@ -2161,7 +2169,7 @@ started: vshPrint(ctl, _("Domain %s started\n"), virDomainGetName(dom)); #ifndef WIN32 - if (console && !cmdRunConsole(ctl, dom, NULL)) + if (console && !cmdRunConsole(ctl, dom, NULL, 0)) goto cleanup; #endif -- 1.7.3.4
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
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 :|
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
This patch is identical to the patch fixing this same issue in the qemu hypervisor. --- src/lxc/lxc_driver.c | 28 +++++++++++++++++++++++++++- 1 files changed, 27 insertions(+), 1 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index c475887..a2ef8a1 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -67,6 +67,8 @@ typedef lxcDomainObjPrivate *lxcDomainObjPrivatePtr; struct _lxcDomainObjPrivate { int monitor; int monitorWatch; + + virStreamPtr console; }; @@ -102,6 +104,9 @@ static void lxcDomainObjPrivateFree(void *data) { lxcDomainObjPrivatePtr priv = data; + if (priv->console) + virStreamFree(priv->console); + VIR_FREE(priv); } @@ -2868,8 +2873,10 @@ lxcDomainOpenConsole(virDomainPtr dom, char uuidstr[VIR_UUID_STRING_BUFLEN]; int ret = -1; virDomainChrDefPtr chr = NULL; + lxcDomainObjPrivatePtr priv; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_CONSOLE_TRY | + VIR_DOMAIN_CONSOLE_FORCE, -1); lxcDriverLock(driver); virUUIDFormat(dom->uuid, uuidstr); @@ -2898,6 +2905,8 @@ lxcDomainOpenConsole(virDomainPtr dom, chr = vm->def->serials[0]; } + priv = vm->privateData; + if (!chr) { lxcError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot find default console device")); @@ -2910,10 +2919,27 @@ lxcDomainOpenConsole(virDomainPtr dom, goto cleanup; } + /* kill open console stream */ + if (priv->console) { + if (virFDStreamIsOpen(priv->console) && + !(flags & VIR_DOMAIN_CONSOLE_FORCE)) { + lxcError(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
This patch is identical to the patch fixing this same issue in the qemu hypervisor. --- src/uml/uml_driver.c | 28 +++++++++++++++++++++++++++- 1 files changed, 27 insertions(+), 1 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 2b7219a..265faf1 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -73,6 +73,8 @@ typedef umlDomainObjPrivate *umlDomainObjPrivatePtr; struct _umlDomainObjPrivate { int monitor; int monitorWatch; + + virStreamPtr console; }; @@ -95,6 +97,9 @@ static void umlDomainObjPrivateFree(void *data) { umlDomainObjPrivatePtr priv = data; + if(priv->console) + virStreamFree(priv->console); + VIR_FREE(priv); } @@ -2244,8 +2249,10 @@ umlDomainOpenConsole(virDomainPtr dom, char uuidstr[VIR_UUID_STRING_BUFLEN]; int ret = -1; virDomainChrDefPtr chr = NULL; + umlDomainObjPrivatePtr priv; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_CONSOLE_TRY | + VIR_DOMAIN_CONSOLE_FORCE, -1); umlDriverLock(driver); virUUIDFormat(dom->uuid, uuidstr); @@ -2274,6 +2281,8 @@ umlDomainOpenConsole(virDomainPtr dom, chr = vm->def->serials[0]; } + priv = vm->privateData; + if (!chr) { umlReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find character device %s"), dev_name); @@ -2286,10 +2295,27 @@ umlDomainOpenConsole(virDomainPtr dom, goto cleanup; } + /* kill open console stream */ + if (priv->console) { + if (virFDStreamIsOpen(priv->console) && + !(flags & VIR_DOMAIN_CONSOLE_FORCE)) { + umlReportError(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
I rebased this series for current head. To fetch this series from my repo conviniently: git checkout -b console_corruption 1afcfbdda0cac112faa61f74ec943e46aa43f2f5 git pull git://aeon.pipo.sk/libvirt.git console_corruption Peter
On Wed, Oct 12, 2011 at 03:43:10PM +0200, Peter Krempa wrote:
This series fixes anoying console corruption if two clients try to connect at same time to the console. The current state of this is, that two/more of libvirt iohelpers are spawned on the same time that compete for data from the pty. This causes that each of the consoles get scrambled and unusable.
Patches fdstream: Emit stream abort callback even if poll() doesnt. virnetclientstream: Propagate stream error messages to callback daemon: Subscribe the stream event callback for error events.
add the ability to abort a stream from the daemon side.
fdstream: Add internal function to check if a fdstream is open This patch adds a helper function that checks internally if a fdstream is open and working.
virsh: fix console stream error reporting Add flags for virDomainOpenConsole virsh: add support for VIR_DOMAIN_CONSOLE_FORCE flag This patches add instrumentation to virsh that supports the new ability to abort streams from the daemon side and adapts the console callback to handle this. These patches also add a new flag set for virDomainOpenConsole API call that allow users to control the if the existing console connection should be left in place or killed in favor of the new one
qemu: Add ability to abort existing console while creating new one lxc: Add ability to abort existing console when creating a new one uml: Add ability to abort existing console when creating a new one These patches modify the hypervisor drivers so that they are aware of existing console connections and refuse to create a new one (or kill the old).
The xen driver also supports console in a similar way like the previously mentioned drivers, but lacks the means to store a stream reference permanently. I'll look in if it's possible to modify this driver to support this new functionality.
The problem with doing the console checks by looking for an in use virStreamPtr is that it only solves it for apps using libvirt. If someone connects using 'xm console' or 'minicom' then we're not protected. The traditional way to protect PTYs from concurrent usage is to place a lock file in /var/lock with a special standardized naming scheme. Should we perhaps be doing that instead ? 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 :|
On 10/18/2011 12:34 PM, Daniel P. Berrange wrote:
On Wed, Oct 12, 2011 at 03:43:10PM +0200, Peter Krempa wrote:
This series fixes anoying console corruption if two clients try to connect at same time to the console. The current state of this is, that two/more of libvirt iohelpers are spawned on the same time that compete for data from the pty. This causes that each of the consoles get scrambled and unusable.
The problem with doing the console checks by looking for an in use virStreamPtr is that it only solves it for apps using libvirt. If someone connects using 'xm console' or 'minicom' then we're not protected. I agree :(.
The traditional way to protect PTYs from concurrent usage is to place a lock file in /var/lock with a special standardized naming scheme. Should we perhaps be doing that instead ? I think, we should be doing this _along_ with alowing libvirt users to break existing connections while using libvirt. My main reason for this is that, if someone would open a console connection and successfully acquire the lock on the pty and then leave for extended period, while his connection is ok (Jiri's patches will handle crashed conections), he would effectively block other users from accessing the console. Same may happen if somebody uses an external tool, to acquire the lock, but in that case, we can't do anything (identifying the process that holds the console pty open and killing it is probably not what we want to do).
IMO, while using libvirt, the users will prefer to use the console provided by libvirt, and not to tinker with minicom/etc, so while it'd be still possible to block potential console users, it would at least handle the most common mistakes. (It will still be possible to break the console,by manually removing the lock file, etc ...). I think that addition of lockfile checking will improve this patchset, but doing it solely that way could be confusing for some and cause problems. Peter
Daniel
On 10/18/2011 12:34 PM, Daniel P. Berrange wrote:
On Wed, Oct 12, 2011 at 03:43:10PM +0200, Peter Krempa wrote:
This series fixes anoying console corruption if two clients try to connect at same time to the console. The current state of this is, that two/more of libvirt iohelpers are spawned on the same time that compete for data from the pty. This causes that each of the consoles get scrambled and unusable.
Patches fdstream: Emit stream abort callback even if poll() doesnt. virnetclientstream: Propagate stream error messages to callback daemon: Subscribe the stream event callback for error events.
add the ability to abort a stream from the daemon side.
fdstream: Add internal function to check if a fdstream is open This patch adds a helper function that checks internally if a fdstream is open and working.
virsh: fix console stream error reporting Add flags for virDomainOpenConsole virsh: add support for VIR_DOMAIN_CONSOLE_FORCE flag This patches add instrumentation to virsh that supports the new ability to abort streams from the daemon side and adapts the console callback to handle this. These patches also add a new flag set for virDomainOpenConsole API call that allow users to control the if the existing console connection should be left in place or killed in favor of the new one
qemu: Add ability to abort existing console while creating new one lxc: Add ability to abort existing console when creating a new one uml: Add ability to abort existing console when creating a new one These patches modify the hypervisor drivers so that they are aware of existing console connections and refuse to create a new one (or kill the old).
The xen driver also supports console in a similar way like the previously mentioned drivers, but lacks the means to store a stream reference permanently. I'll look in if it's possible to modify this driver to support this new functionality.
The problem with doing the console checks by looking for an in use virStreamPtr is that it only solves it for apps using libvirt. If someone connects using 'xm console' or 'minicom' then we're not protected.
The traditional way to protect PTYs from concurrent usage is to place a lock file in /var/lock with a special standardized naming scheme. Should we perhaps be doing that instead ?
Daniel
I've prepared an updated version of this series taking into account recent changes to console code and Daniel's comments on this version. I've got some questions regarding implementation of lock files on PTY's suggested in this thread I'd like to discuss: 1) When using lock files, there's the risk of having them left behind when the daemon crashes/is killed. According to the filesystem hierarchy standard (http://www.pathname.com/fhs/2.2/fhs-5.9.html) lock files should contain PID of process holding the lock. This enables to check if the process holding the lock still exists. I'm not sure if we want to check if the process exists and remove lock files, that were left behind, or just warn the user that a lock exists and refuse to open console. 2) When a non-good-mannered program opens the PTY and does not check for lock files or create them, we will still end up with a corrupted console. Is there an elegant solution for this? I'd appreciate your comments on this topic. Thanks, Peter
On Thu, Dec 01, 2011 at 04:35:10PM +0100, Peter Krempa wrote:
On 10/18/2011 12:34 PM, Daniel P. Berrange wrote:
On Wed, Oct 12, 2011 at 03:43:10PM +0200, Peter Krempa wrote:
This series fixes anoying console corruption if two clients try to connect at same time to the console. The current state of this is, that two/more of libvirt iohelpers are spawned on the same time that compete for data from the pty. This causes that each of the consoles get scrambled and unusable.
Patches fdstream: Emit stream abort callback even if poll() doesnt. virnetclientstream: Propagate stream error messages to callback daemon: Subscribe the stream event callback for error events.
add the ability to abort a stream from the daemon side.
fdstream: Add internal function to check if a fdstream is open This patch adds a helper function that checks internally if a fdstream is open and working.
virsh: fix console stream error reporting Add flags for virDomainOpenConsole virsh: add support for VIR_DOMAIN_CONSOLE_FORCE flag This patches add instrumentation to virsh that supports the new ability to abort streams from the daemon side and adapts the console callback to handle this. These patches also add a new flag set for virDomainOpenConsole API call that allow users to control the if the existing console connection should be left in place or killed in favor of the new one
qemu: Add ability to abort existing console while creating new one lxc: Add ability to abort existing console when creating a new one uml: Add ability to abort existing console when creating a new one These patches modify the hypervisor drivers so that they are aware of existing console connections and refuse to create a new one (or kill the old).
The xen driver also supports console in a similar way like the previously mentioned drivers, but lacks the means to store a stream reference permanently. I'll look in if it's possible to modify this driver to support this new functionality.
The problem with doing the console checks by looking for an in use virStreamPtr is that it only solves it for apps using libvirt. If someone connects using 'xm console' or 'minicom' then we're not protected.
The traditional way to protect PTYs from concurrent usage is to place a lock file in /var/lock with a special standardized naming scheme. Should we perhaps be doing that instead ?
Daniel
I've prepared an updated version of this series taking into account recent changes to console code and Daniel's comments on this version.
I've got some questions regarding implementation of lock files on PTY's suggested in this thread I'd like to discuss:
1) When using lock files, there's the risk of having them left behind when the daemon crashes/is killed. According to the filesystem hierarchy standard (http://www.pathname.com/fhs/2.2/fhs-5.9.html) lock files should contain PID of process holding the lock. This enables to check if the process holding the lock still exists. I'm not sure if we want to check if the process exists and remove lock files, that were left behind, or just warn the user that a lock exists and refuse to open console.
That seems like a reasonable approach to me.
2) When a non-good-mannered program opens the PTY and does not check for lock files or create them, we will still end up with a corrupted console. Is there an elegant solution for this?
I don't see any elegant way to protect against ill-mannered programs ignoring the lock files. Dave
I'd appreciate your comments on this topic.
Thanks, Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Daniel P. Berrange -
Dave Allan -
Peter Krempa