Re: [PATCH v2] tools: console: Relax stream EOF handling

[Daniel Henrique Barboza <danielhb413@xxxxxxxxx>]

The comment I have here is that you're changing virConsoleShutdown
API for all callers, with this new boolean value 'needAbort', because of a
scenario (virStreamRecv being called before) that happens only on
virConsoleEventOnStream.


This is what I wondered in the review of the previous version of this
patch:

"Shouldn't we call virStreamFinish if got=0 and only report an error if
virStreamFinish returns -1?"

I tried it out and it worked like a charm for me:


diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index e16f841..998662c 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -172,9 +172,12 @@ virConsoleEventOnStream(virStreamPtr st,
         if (got == -2)
             goto cleanup; /* blocking */
         if (got <= 0) {
-            if (got == 0)
-                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                               _("console stream EOF"));
+            if (got == 0) {
+                got = virStreamFinish(st);
+                if (got != 0)
+                    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                                   _("console stream EOF"));
+            }

             virConsoleShutdown(con);
             goto cleanup;


I didn't see any errors by calling virStreamFinish() before virStreamAbort()
being called by virConsoleShutdown() right after (and by reading the code,
it appears to be an OK usage), so I am skeptical about safeguarding the
virStreamAbort() call with a boolean value like you're making here.

To be clear, I'm not saying your patch is wrong - and perhaps there is a 
case
for that safeguard inside virConsoleShutdown like you're doing here. My
point here is that if the code I shown above isn't breaking anything, 
I'd rather
go for that.




Thanks,


DHB


On 8/12/19 8:15 AM, Roman Bolshakov wrote:
> Regular VM shutdown triggers the error for existing session of virsh
> console and it returns with non-zero exit code:
>    error: internal error: console stream EOF
>
> The message and status code are misleading because there's no real
> error. virStreamRecv returns 0 when the end of the stream is reached.
> In that case, virStreamFinish should be used to get confirmation of
> stream completion. virSteramAbort is used otherwise to terminate the
> stream early before an EOF.
>
> Fixes: 29f2b5248c6 ("tools: console: pass stream/fd errors to user")
> Signed-off-by: Roman Bolshakov <r.bolshakov@xxxxxxxxx>
> ---
> Changes since v1:
>   - Added needAbort parameter to virConsoleShutdown to specify if finish
>     or abort is needed
>   - Use it (needAbort = false) to finish stream after EOF from
>     virStreamRecv
>   - Print internal error if virStreamFinish or virStreamAbort do not
>     succeed
>
>   tools/virsh-console.c | 44 ++++++++++++++++++++++++-------------------
>   1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
> index e16f841e57..ad514a99a7 100644
> --- a/tools/virsh-console.c
> +++ b/tools/virsh-console.c
> @@ -97,7 +97,7 @@ virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED)
>   
>   
>   static void
> -virConsoleShutdown(virConsolePtr con)
> +virConsoleShutdown(virConsolePtr con, bool needAbort)
>   {
>       virErrorPtr err = virGetLastError();
>   
> @@ -105,8 +105,15 @@ virConsoleShutdown(virConsolePtr con)
>           virCopyLastError(&con->error);
>   
>       if (con->st) {
> +        int termError;
>           virStreamEventRemoveCallback(con->st);
> -        virStreamAbort(con->st);
> +        if (needAbort)
> +            termError = virStreamAbort(con->st);
> +        else
> +            termError = virStreamFinish(con->st);
> +        if (termError < 0)
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("cannot terminate console stream"));
>           virStreamFree(con->st);
>           con->st = NULL;
>       }
> @@ -158,7 +165,7 @@ virConsoleEventOnStream(virStreamPtr st,
>           if (avail < 1024) {
>               if (VIR_REALLOC_N(con->streamToTerminal.data,
>                                 con->streamToTerminal.length + 1024) < 0) {
> -                virConsoleShutdown(con);
> +                virConsoleShutdown(con, true);
>                   goto cleanup;
>               }
>               con->streamToTerminal.length += 1024;
> @@ -172,11 +179,10 @@ virConsoleEventOnStream(virStreamPtr st,
>           if (got == -2)
>               goto cleanup; /* blocking */
>           if (got <= 0) {
> +            bool needAbort = true;
>               if (got == 0)
> -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                               _("console stream EOF"));
> -
> -            virConsoleShutdown(con);
> +                needAbort = false;
> +            virConsoleShutdown(con, needAbort);
>               goto cleanup;
>           }
>           con->streamToTerminal.offset += got;
> @@ -195,7 +201,7 @@ virConsoleEventOnStream(virStreamPtr st,
>           if (done == -2)
>               goto cleanup; /* blocking */
>           if (done < 0) {
> -            virConsoleShutdown(con);
> +            virConsoleShutdown(con, true);
>               goto cleanup;
>           }
>           memmove(con->terminalToStream.data,
> @@ -216,7 +222,7 @@ virConsoleEventOnStream(virStreamPtr st,
>   
>       if (events & VIR_STREAM_EVENT_ERROR ||
>           events & VIR_STREAM_EVENT_HANGUP) {
> -        virConsoleShutdown(con);
> +        virConsoleShutdown(con, true);
>       }
>   
>    cleanup:
> @@ -246,7 +252,7 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>           if (avail < 1024) {
>               if (VIR_REALLOC_N(con->terminalToStream.data,
>                                 con->terminalToStream.length + 1024) < 0) {
> -                virConsoleShutdown(con);
> +                virConsoleShutdown(con, true);
>                   goto cleanup;
>               }
>               con->terminalToStream.length += 1024;
> @@ -260,17 +266,17 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>           if (got < 0) {
>               if (errno != EAGAIN) {
>                   virReportSystemError(errno, "%s", _("cannot read from stdin"));
> -                virConsoleShutdown(con);
> +                virConsoleShutdown(con, true);
>               }
>               goto cleanup;
>           }
>           if (got == 0) {
>               virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin"));
> -            virConsoleShutdown(con);
> +            virConsoleShutdown(con, true);
>               goto cleanup;
>           }
>           if (con->terminalToStream.data[con->terminalToStream.offset] == con->escapeChar) {
> -            virConsoleShutdown(con);
> +            virConsoleShutdown(con, true);
>               goto cleanup;
>           }
>   
> @@ -283,13 +289,13 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
>   
>       if (events & VIR_EVENT_HANDLE_ERROR) {
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IO error on stdin"));
> -        virConsoleShutdown(con);
> +        virConsoleShutdown(con, true);
>           goto cleanup;
>       }
>   
>       if (events & VIR_EVENT_HANDLE_HANGUP) {
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin"));
> -        virConsoleShutdown(con);
> +        virConsoleShutdown(con, true);
>           goto cleanup;
>       }
>   
> @@ -322,7 +328,7 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
>           if (done < 0) {
>               if (errno != EAGAIN) {
>                   virReportSystemError(errno, "%s", _("cannot write to stdout"));
> -                virConsoleShutdown(con);
> +                virConsoleShutdown(con, true);
>               }
>               goto cleanup;
>           }
> @@ -344,13 +350,13 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
>   
>       if (events & VIR_EVENT_HANDLE_ERROR) {
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IO error stdout"));
> -        virConsoleShutdown(con);
> +        virConsoleShutdown(con, true);
>           goto cleanup;
>       }
>   
>       if (events & VIR_EVENT_HANDLE_HANGUP) {
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdout"));
> -        virConsoleShutdown(con);
> +        virConsoleShutdown(con, true);
>           goto cleanup;
>       }
>   
> @@ -489,7 +495,7 @@ virshRunConsole(vshControl *ctl,
>           ret = 0;
>   
>    cleanup:
> -    virConsoleShutdown(con);
> +    virConsoleShutdown(con, true);
>   
>       if (ret < 0) {
>           vshResetLibvirtError();

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