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

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

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

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

On Wed, Oct 12, 2011 at 03:43:15PM +0200, Peter Krempa wrote: 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. As with the other patch, this is not required. HANGUP/ERROR are always enabled, if you have READABLE or WRITABLE set. 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 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

On Wed, Oct 12, 2011 at 03:43:18PM +0200, Peter Krempa 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 -- |: 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 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