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@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
- Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
