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;
+    }
 
     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);
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list