1. Patchew
  2. Libvirt
  3. View series

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

Roman Bolshakov posted 1 patch 4 years, 8 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190821133314.96107-1-r.bolshakov@yadro.com
tools/virsh-console.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
[libvirt] [PATCH v3] tools: console: Relax stream EOF handling
Posted by Roman Bolshakov 4 years, 8 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 correctly when EOF is reached.

Existing implementations of esx, fd, and remote streams behave the same
for virStreamFinish and virStreamAbort: they close the stream. So, we
can continue to use virStreamAbort to handle EOF and errors from
virStreamRecv but additonally we can report error if virStreamAbort
fails.

Fixes: 29f2b5248c6 ("tools: console: pass stream/fd errors to user")
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 tools/virsh-console.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index e16f841e57..a235a9a283 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -106,7 +106,9 @@ virConsoleShutdown(virConsolePtr con)
 
     if (con->st) {
         virStreamEventRemoveCallback(con->st);
-        virStreamAbort(con->st);
+        if (virStreamAbort(con->st) < 0)
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("cannot terminate console stream"));
         virStreamFree(con->st);
         con->st = NULL;
     }
@@ -172,10 +174,6 @@ virConsoleEventOnStream(virStreamPtr st,
         if (got == -2)
             goto cleanup; /* blocking */
         if (got <= 0) {
-            if (got == 0)
-                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                               _("console stream EOF"));
-
             virConsoleShutdown(con);
             goto cleanup;
         }
-- 
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] tools: console: Relax stream EOF handling
Posted by Michal Privoznik 4 years, 8 months ago 
On 8/21/19 3:33 PM, 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 correctly when EOF is reached.
> 
> Existing implementations of esx, fd, and remote streams behave the same
> for virStreamFinish and virStreamAbort: they close the stream. So, we
> can continue to use virStreamAbort to handle EOF and errors from
> virStreamRecv but additonally we can report error if virStreamAbort
> fails.
> 
> Fixes: 29f2b5248c6 ("tools: console: pass stream/fd errors to user")
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>   tools/virsh-console.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
> index e16f841e57..a235a9a283 100644
> --- a/tools/virsh-console.c
> +++ b/tools/virsh-console.c
> @@ -106,7 +106,9 @@ virConsoleShutdown(virConsolePtr con)
>   
>       if (con->st) {
>           virStreamEventRemoveCallback(con->st);
> -        virStreamAbort(con->st);
> +        if (virStreamAbort(con->st) < 0)
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("cannot terminate console stream"));
>           virStreamFree(con->st);
>           con->st = NULL;
>       }
> @@ -172,10 +174,6 @@ virConsoleEventOnStream(virStreamPtr st,
>           if (got == -2)
>               goto cleanup; /* blocking */
>           if (got <= 0) {
> -            if (got == 0)
> -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                               _("console stream EOF"));
> -

I still think we need to call virStreamFinish() in this case and not 
virStreamAbort() (hidden in virConsoleShutdown()). The reason is that 
when we read 0 bytes (= indication of EOF) the virStreamFinish() informs 
the sending side that we've read all the data and we've done so 
successfuly. If we abort the stream, it's sending a message that we've 
encountered an error on the very last packet in the stream, which is not 
true. It may not be that huge of the problem in case of ESX where the 
only difference between streamFinish() and streamAbort() is an error 
message that is or is not logged.


However, I will merge your patch because it's removing spurious error 
message and I'll post a patch to allow graceful console shutdown.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] tools: console: Relax stream EOF handling
Posted by Daniel Henrique Barboza 4 years, 8 months ago 

On 8/21/19 10:33 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 correctly when EOF is reached.
>
> Existing implementations of esx, fd, and remote streams behave the same
> for virStreamFinish and virStreamAbort: they close the stream. So, we
> can continue to use virStreamAbort to handle EOF and errors from
> virStreamRecv but additonally we can report error if virStreamAbort
> fails.
>
> Fixes: 29f2b5248c6 ("tools: console: pass stream/fd errors to user")
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>


>   tools/virsh-console.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
> index e16f841e57..a235a9a283 100644
> --- a/tools/virsh-console.c
> +++ b/tools/virsh-console.c
> @@ -106,7 +106,9 @@ virConsoleShutdown(virConsolePtr con)
>   
>       if (con->st) {
>           virStreamEventRemoveCallback(con->st);
> -        virStreamAbort(con->st);
> +        if (virStreamAbort(con->st) < 0)
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("cannot terminate console stream"));
>           virStreamFree(con->st);
>           con->st = NULL;
>       }
> @@ -172,10 +174,6 @@ virConsoleEventOnStream(virStreamPtr st,
>           if (got == -2)
>               goto cleanup; /* blocking */
>           if (got <= 0) {
> -            if (got == 0)
> -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                               _("console stream EOF"));
> -
>               virConsoleShutdown(con);
>               goto cleanup;
>           }

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.