[libvirt] [PATCH v3 2/5] tools: console: cleanup console on errors in main thread

[Cole Robinson]

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 at 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 at redhat.com>

- Cole