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

Roman Bolshakov posted 1 patch 4 years, 7 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190812111556.50439-1-r.bolshakov@yadro.com
There is a newer version of this series
tools/virsh-console.c | 44 ++++++++++++++++++++++++-------------------
1 file changed, 25 insertions(+), 19 deletions(-)
[libvirt] [PATCH v2] tools: console: Relax stream EOF handling
Posted by Roman Bolshakov 4 years, 7 months ago
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@yadro.com>
---
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();
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] tools: console: Relax stream EOF handling
Posted by Daniel Henrique Barboza 4 years, 7 months ago
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@yadro.com>
> ---
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] tools: console: Relax stream EOF handling
Posted by Michal Privoznik 4 years, 7 months ago
On 8/13/19 12:01 AM, Daniel Henrique Barboza wrote:
> 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.

I agree that this is better solution. One way to view libvirt streams is 
as a pipe. What is written on one side appears on the other. In the 
analogy, if a reader gets 0 bytes from the pipe on read() they know it's 
EOF and they call close() (or virStreamFinish() in this case).

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] tools: console: Relax stream EOF handling
Posted by Roman Bolshakov 4 years, 7 months ago
On Mon, Aug 12, 2019 at 07:01:47PM -0300, Daniel Henrique Barboza wrote:
> 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.
> 
> 

Existing implementations of virStreamFinish and virStreamAbort for esx,
remote and fd streams behave exactly the same. They close the steam
using the same function. I haven't found any other difference. So if we
intend to minimize changes, we should use v1. It will work exactly the
same except it calls virStreamAbort to close the stream. I can also add
a check to print the error if virStreamAbort in virConsoleShutdown
fails.

However, if we want to be correct from documentation standpoint, we
should use v2.

Thanks,
Roman

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