[libvirt] [PATCH v2 0/4] tools: console: a fix and improvment
Jonh, I split the 1st patch the other way to avoid changing same lines of code in subsequent patches. Diff to v1[1] - split 1st patch into 3 - change wordings of commit messages/error messages - add minor changes to code as suggested by John [1] https://www.redhat.com/archives/libvir-list/2019-February/msg00920.html Nikolay Shirokovskiy (4): tools: console: cleanup console on errors in main thread tools: console: add missing locks in callbacks tools: console: check if console was shutdown in callbacks tools: console: pass stream/fd errors to user tools/virsh-console.c | 224 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 171 insertions(+), 53 deletions(-) -- 1.8.3.1
We only check now for virObjectWait failures in virshRunConsole but we'd better check and for other failures too. Anyway if failure happened we need to shutdown console to stop delivering events from event loop thread or we are in trouble as console is freed on virshRunConsole exit. We need to turn console into virObject object because stream/fd callbacks can be called from a event loop thread after shutdown/freeing console in main thread. It is convinient to turn into virLockableObject as we have mutex in console object. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- tools/virsh-console.c | 122 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 87 insertions(+), 35 deletions(-) diff --git a/tools/virsh-console.c b/tools/virsh-console.c index 045a636..9289221 100644 --- a/tools/virsh-console.c +++ b/tools/virsh-console.c @@ -60,9 +60,10 @@ struct virConsoleBuffer { typedef struct virConsole virConsole; typedef virConsole *virConsolePtr; struct virConsole { + virObjectLockable parent; + virStreamPtr st; bool quit; - virMutex lock; virCond cond; int stdinWatch; @@ -74,6 +75,19 @@ struct virConsole { char escapeChar; }; +static virClassPtr virConsoleClass; +static void virConsoleDispose(void *obj); + +static int +virConsoleOnceInit(void) +{ + if (!VIR_CLASS_NEW(virConsole, virClassForObjectLockable())) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virConsole); static void virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED) @@ -98,22 +112,22 @@ virConsoleShutdown(virConsolePtr con) virEventRemoveHandle(con->stdoutWatch); con->stdinWatch = -1; con->stdoutWatch = -1; - con->quit = true; - virCondSignal(&con->cond); + if (!con->quit) { + con->quit = true; + virCondSignal(&con->cond); + } } static void -virConsoleFree(virConsolePtr con) +virConsoleDispose(void *obj) { - if (!con) - return; + virConsolePtr con = obj; if (con->st) virStreamFree(con->st); - virMutexDestroy(&con->lock); + virCondDestroy(&con->cond); - VIR_FREE(con); } @@ -288,6 +302,35 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, } +static virConsolePtr +virConsoleNew(void) +{ + virConsolePtr con; + + if (virConsoleInitialize() < 0) + return NULL; + + if (!(con = virObjectNew(virConsoleClass))) + return NULL; + + if (virCondInit(&con->cond) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot initialize console condition")); + + goto error; + } + + con->stdinWatch = -1; + con->stdoutWatch = -1; + + return con; + + error: + virObjectUnref(con); + return NULL; +} + + static char virshGetEscapeChar(const char *s) { @@ -324,6 +367,11 @@ virshRunConsole(vshControl *ctl, if (vshTTYMakeRaw(ctl, true) < 0) goto resettty; + if (!(con = virConsoleNew())) + goto resettty; + + virObjectLock(con); + /* Trap all common signals so that we can safely restore the original * terminal settings on STDIN before the process exits - people don't like * being left with a messed up terminal ! */ @@ -333,9 +381,6 @@ virshRunConsole(vshControl *ctl, sigaction(SIGHUP, &sighandler, &old_sighup); sigaction(SIGPIPE, &sighandler, &old_sigpipe); - if (VIR_ALLOC(con) < 0) - goto cleanup; - con->escapeChar = virshGetEscapeChar(priv->escapeChar); con->st = virStreamNew(virDomainGetConnect(dom), VIR_STREAM_NONBLOCK); @@ -345,42 +390,49 @@ virshRunConsole(vshControl *ctl, if (virDomainOpenConsole(dom, dev_name, con->st, flags) < 0) goto cleanup; - if (virCondInit(&con->cond) < 0 || virMutexInit(&con->lock) < 0) + virObjectRef(con); + if ((con->stdinWatch = virEventAddHandle(STDIN_FILENO, + VIR_EVENT_HANDLE_READABLE, + virConsoleEventOnStdin, + con, + virObjectFreeCallback)) < 0) { + virObjectUnref(con); + goto cleanup; + } + + virObjectRef(con); + if ((con->stdoutWatch = virEventAddHandle(STDOUT_FILENO, + 0, + virConsoleEventOnStdout, + con, + virObjectFreeCallback)) < 0) { + virObjectUnref(con); goto cleanup; + } - virMutexLock(&con->lock); - - con->stdinWatch = virEventAddHandle(STDIN_FILENO, - VIR_EVENT_HANDLE_READABLE, - virConsoleEventOnStdin, - con, - NULL); - con->stdoutWatch = virEventAddHandle(STDOUT_FILENO, - 0, - virConsoleEventOnStdout, - con, - NULL); - - virStreamEventAddCallback(con->st, - VIR_STREAM_EVENT_READABLE, - virConsoleEventOnStream, - con, - NULL); + virObjectRef(con); + if (virStreamEventAddCallback(con->st, + VIR_STREAM_EVENT_READABLE, + virConsoleEventOnStream, + con, + virObjectFreeCallback) < 0) { + virObjectUnref(con); + goto cleanup; + } while (!con->quit) { - if (virCondWait(&con->cond, &con->lock) < 0) { - virMutexUnlock(&con->lock); + if (virCondWait(&con->cond, &con->parent.lock) < 0) { VIR_ERROR(_("unable to wait on console condition")); goto cleanup; } } - virMutexUnlock(&con->lock); - ret = 0; cleanup: - virConsoleFree(con); + virConsoleShutdown(con); + virObjectUnlock(con); + virObjectUnref(con); /* Restore original signal handlers */ sigaction(SIGQUIT, &old_sigquit, NULL); -- 1.8.3.1
On Wed, Mar 13, 2019 at 10:39:46AM +0300, Nikolay Shirokovskiy wrote:
We only check now for virObjectWait failures in virshRunConsole but we'd better check and for other failures too. Anyway if failure happened we need to shutdown console to stop delivering events from event loop thread or we are in trouble as console is freed on virshRunConsole exit.
We need to turn console into virObject object because stream/fd callbacks can be called from a event loop thread after shutdown/freeing console in main thread. It is convinient to turn into virLockableObject as we have mutex in console object.
Could you do this in two separate patches - one which refactors into a virObject, and the second which does the actual error handling fixes. The changes look ok, but distinguishing the actual fix from the refactoring is hard. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Stream/fd callbacks accessing console object are called from the event loop thread and the console object is also accessed from the main thread so we are better add locking to handlers. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- tools/virsh-console.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/tools/virsh-console.c b/tools/virsh-console.c index 9289221..3dae707 100644 --- a/tools/virsh-console.c +++ b/tools/virsh-console.c @@ -137,6 +137,8 @@ virConsoleEventOnStream(virStreamPtr st, { virConsolePtr con = opaque; + virObjectLock(con); + if (events & VIR_STREAM_EVENT_READABLE) { size_t avail = con->streamToTerminal.length - con->streamToTerminal.offset; @@ -146,7 +148,7 @@ virConsoleEventOnStream(virStreamPtr st, if (VIR_REALLOC_N(con->streamToTerminal.data, con->streamToTerminal.length + 1024) < 0) { virConsoleShutdown(con); - return; + goto cleanup; } con->streamToTerminal.length += 1024; avail += 1024; @@ -157,10 +159,10 @@ virConsoleEventOnStream(virStreamPtr st, con->streamToTerminal.offset, avail); if (got == -2) - return; /* blocking */ + goto cleanup; /* blocking */ if (got <= 0) { virConsoleShutdown(con); - return; + goto cleanup; } con->streamToTerminal.offset += got; if (con->streamToTerminal.offset) @@ -176,10 +178,10 @@ virConsoleEventOnStream(virStreamPtr st, con->terminalToStream.data, con->terminalToStream.offset); if (done == -2) - return; /* blocking */ + goto cleanup; /* blocking */ if (done < 0) { virConsoleShutdown(con); - return; + goto cleanup; } memmove(con->terminalToStream.data, con->terminalToStream.data + done, @@ -201,6 +203,9 @@ virConsoleEventOnStream(virStreamPtr st, events & VIR_STREAM_EVENT_HANGUP) { virConsoleShutdown(con); } + + cleanup: + virObjectUnlock(con); } @@ -212,6 +217,8 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, { virConsolePtr con = opaque; + virObjectLock(con); + if (events & VIR_EVENT_HANDLE_READABLE) { size_t avail = con->terminalToStream.length - con->terminalToStream.offset; @@ -221,7 +228,7 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, if (VIR_REALLOC_N(con->terminalToStream.data, con->terminalToStream.length + 1024) < 0) { virConsoleShutdown(con); - return; + goto cleanup; } con->terminalToStream.length += 1024; avail += 1024; @@ -234,15 +241,15 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, if (got < 0) { if (errno != EAGAIN) virConsoleShutdown(con); - return; + goto cleanup; } if (got == 0) { virConsoleShutdown(con); - return; + goto cleanup; } if (con->terminalToStream.data[con->terminalToStream.offset] == con->escapeChar) { virConsoleShutdown(con); - return; + goto cleanup; } con->terminalToStream.offset += got; @@ -256,6 +263,9 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, events & VIR_EVENT_HANDLE_HANGUP) { virConsoleShutdown(con); } + + cleanup: + virObjectUnlock(con); } @@ -267,6 +277,8 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, { virConsolePtr con = opaque; + virObjectLock(con); + if (events & VIR_EVENT_HANDLE_WRITABLE && con->streamToTerminal.offset) { ssize_t done; @@ -277,7 +289,7 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, if (done < 0) { if (errno != EAGAIN) virConsoleShutdown(con); - return; + goto cleanup; } memmove(con->streamToTerminal.data, con->streamToTerminal.data + done, @@ -299,6 +311,9 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, events & VIR_EVENT_HANDLE_HANGUP) { virConsoleShutdown(con); } + + cleanup: + virObjectUnlock(con); } -- 1.8.3.1
On error in main thread virConsoleShutdown is called which deletes fd watches/stream callback and yet callbacks can be called after. Thus we can incorrectly allocate terminalToStream.data memory and get memory leak for example. Let's check if console was shutdown in the very beginning of callbacks. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- tools/virsh-console.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tools/virsh-console.c b/tools/virsh-console.c index 3dae707..d109734 100644 --- a/tools/virsh-console.c +++ b/tools/virsh-console.c @@ -139,6 +139,10 @@ virConsoleEventOnStream(virStreamPtr st, virObjectLock(con); + /* we got late event after console was shutdown */ + if (!con->st) + goto cleanup; + if (events & VIR_STREAM_EVENT_READABLE) { size_t avail = con->streamToTerminal.length - con->streamToTerminal.offset; @@ -219,6 +223,10 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, virObjectLock(con); + /* we got late event after console was shutdown */ + if (!con->st) + goto cleanup; + if (events & VIR_EVENT_HANDLE_READABLE) { size_t avail = con->terminalToStream.length - con->terminalToStream.offset; @@ -279,6 +287,10 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, virObjectLock(con); + /* we got late event after console was shutdown */ + if (!con->st) + goto cleanup; + if (events & VIR_EVENT_HANDLE_WRITABLE && con->streamToTerminal.offset) { ssize_t done; -- 1.8.3.1
If the console was disconnected due to a connection problem or a problem on the server side it is convinient to provide the cause to the user. If the error come from the API then the error is saved in a virsh global variable. However, since success is returned from virshRunConsole after we reach the waiting stage, then the error is never reported. Let's track the error in the event loop. Next after failure we do a cleanup and this cleanup can overwrite root cause. Thus let's save root cause immediately and then set it to virsh error after all cleanup is done. Since we'll be sending the error to the consumer, each failure path from the event handlers needs to be augmented to provide what error generated the failure. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- tools/virsh-console.c | 55 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 8 deletions(-) diff --git a/tools/virsh-console.c b/tools/virsh-console.c index d109734..236933a 100644 --- a/tools/virsh-console.c +++ b/tools/virsh-console.c @@ -73,6 +73,7 @@ struct virConsole { struct virConsoleBuffer terminalToStream; char escapeChar; + virError error; }; static virClassPtr virConsoleClass; @@ -98,6 +99,11 @@ virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED) static void virConsoleShutdown(virConsolePtr con) { + virErrorPtr err = virGetLastError(); + + if (con->error.code == VIR_ERR_OK && err && err->code != VIR_ERR_OK) + virCopyLastError(&con->error); + if (con->st) { virStreamEventRemoveCallback(con->st); virStreamAbort(con->st); @@ -128,6 +134,7 @@ virConsoleDispose(void *obj) virStreamFree(con->st); virCondDestroy(&con->cond); + virResetError(&con->error); } @@ -165,6 +172,10 @@ virConsoleEventOnStream(virStreamPtr st, if (got == -2) goto cleanup; /* blocking */ if (got <= 0) { + if (got == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("console stream EOF")); + } virConsoleShutdown(con); goto cleanup; } @@ -247,11 +258,14 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, con->terminalToStream.offset, avail); if (got < 0) { - if (errno != EAGAIN) + if (errno != EAGAIN) { + virReportSystemError(errno, "%s", _("cannot read from stdin")); virConsoleShutdown(con); + } goto cleanup; } if (got == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin")); virConsoleShutdown(con); goto cleanup; } @@ -267,9 +281,16 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, VIR_STREAM_EVENT_WRITABLE); } - if (events & VIR_EVENT_HANDLE_ERROR || - events & VIR_EVENT_HANDLE_HANGUP) { + if (events & VIR_EVENT_HANDLE_ERROR) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IO error on stdin")); virConsoleShutdown(con); + goto cleanup; + } + + if (events & VIR_EVENT_HANDLE_HANGUP) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin")); + virConsoleShutdown(con); + goto cleanup; } cleanup: @@ -299,8 +320,10 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, con->streamToTerminal.data, con->streamToTerminal.offset); if (done < 0) { - if (errno != EAGAIN) + if (errno != EAGAIN) { + virReportSystemError(errno, "%s", _("cannot write to stdout")); virConsoleShutdown(con); + } goto cleanup; } memmove(con->streamToTerminal.data, @@ -319,9 +342,16 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, if (!con->streamToTerminal.offset) virEventUpdateHandle(con->stdoutWatch, 0); - if (events & VIR_EVENT_HANDLE_ERROR || - events & VIR_EVENT_HANDLE_HANGUP) { + if (events & VIR_EVENT_HANDLE_ERROR) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IO error stdout")); virConsoleShutdown(con); + goto cleanup; + } + + if (events & VIR_EVENT_HANDLE_HANGUP) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdout")); + virConsoleShutdown(con); + goto cleanup; } cleanup: @@ -449,15 +479,24 @@ virshRunConsole(vshControl *ctl, while (!con->quit) { if (virCondWait(&con->cond, &con->parent.lock) < 0) { - VIR_ERROR(_("unable to wait on console condition")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to wait on console condition")); goto cleanup; } } - ret = 0; + if (con->error.code == VIR_ERR_OK) + ret = 0; cleanup: virConsoleShutdown(con); + + if (ret < 0) { + vshResetLibvirtError(); + virSetError(&con->error); + vshSaveLibvirtHelperError(); + } + virObjectUnlock(con); virObjectUnref(con); -- 1.8.3.1
participants (2)
-
Daniel P. Berrangé -
Nikolay Shirokovskiy