On 3/18/19 5:08 AM, Nikolay Shirokovskiy wrote:
We only check now for virObjectWait failures in virshRunConsole but we'd better check and for other failures too. And we need to shutdown console on error in the main thread.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- tools/virsh-console.c | 52 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 18 deletions(-)
diff --git a/tools/virsh-console.c b/tools/virsh-console.c index 763b395..9289221 100644 --- a/tools/virsh-console.c +++ b/tools/virsh-console.c @@ -112,8 +112,10 @@ 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); + } }
@@ -388,22 +390,35 @@ virshRunConsole(vshControl *ctl, if (virDomainOpenConsole(dom, dev_name, con->st, flags) < 0) goto cleanup;
- 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 ((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; + } + + virObjectRef(con); + if (virStreamEventAddCallback(con->st, + VIR_STREAM_EVENT_READABLE, + virConsoleEventOnStream, + con, + virObjectFreeCallback) < 0) { + virObjectUnref(con); + goto cleanup; + }
I didn't understand this pattern at first, but I see it's used by the qemu agent and monitor callbacks and it makes sense after refreshing my event loop memory: we add a reference because the loop callback carries around 'con' as an opaque data value, which is unref'd when the event handle is removed.
while (!con->quit) { if (virCondWait(&con->cond, &con->parent.lock) < 0) { @@ -415,6 +430,7 @@ virshRunConsole(vshControl *ctl, ret = 0;
cleanup: + virConsoleShutdown(con); virObjectUnlock(con); virObjectUnref(con);
And we need this here to actually trigger the handle unregistering, incase one of the Add* calls fails. So makes sense to me Reviewed-by: Cole Robinson <crobinso@redhat.com> - Cole