1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] virsh: Allow graceful console shutdown

Michal Privoznik 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/f5a7d35614d3c0a49417ef707d06ba68c034884a.1566567216.git.mprivozn@redhat.com
tools/virsh-console.c | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
[libvirt] [PATCH] virsh: Allow graceful console shutdown
Posted by Michal Privoznik 4 years, 8 months ago 
Currently, whenever there's a regular EOF on the console stream
or an error the virStreamAbort() is called regardless. While this
may not actually break anything, we should call virStreamFinish()
to let the daemon know we've successfully received all the data
and are shutting down the stream gracefully.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tools/virsh-console.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/tools/virsh-console.c b/tools/virsh-console.c
index a235a9a283..900faa5087 100644
--- a/tools/virsh-console.c
+++ b/tools/virsh-console.c
@@ -97,7 +97,8 @@ virConsoleHandleSignal(int sig ATTRIBUTE_UNUSED)
 
 
 static void
-virConsoleShutdown(virConsolePtr con)
+virConsoleShutdown(virConsolePtr con,
+                   bool graceful)
 {
     virErrorPtr err = virGetLastError();
 
@@ -105,10 +106,18 @@ virConsoleShutdown(virConsolePtr con)
         virCopyLastError(&con->error);
 
     if (con->st) {
+        int rc;
+
         virStreamEventRemoveCallback(con->st);
-        if (virStreamAbort(con->st) < 0)
+        if (graceful)
+            rc = virStreamFinish(con->st);
+        else
+            rc = virStreamAbort(con->st);
+
+        if (rc < 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("cannot terminate console stream"));
+        }
         virStreamFree(con->st);
         con->st = NULL;
     }
@@ -160,7 +169,7 @@ virConsoleEventOnStream(virStreamPtr st,
         if (avail < 1024) {
             if (VIR_REALLOC_N(con->streamToTerminal.data,
                               con->streamToTerminal.length + 1024) < 0) {
-                virConsoleShutdown(con);
+                virConsoleShutdown(con, false);
                 goto cleanup;
             }
             con->streamToTerminal.length += 1024;
@@ -174,7 +183,7 @@ virConsoleEventOnStream(virStreamPtr st,
         if (got == -2)
             goto cleanup; /* blocking */
         if (got <= 0) {
-            virConsoleShutdown(con);
+            virConsoleShutdown(con, got == 0);
             goto cleanup;
         }
         con->streamToTerminal.offset += got;
@@ -193,7 +202,7 @@ virConsoleEventOnStream(virStreamPtr st,
         if (done == -2)
             goto cleanup; /* blocking */
         if (done < 0) {
-            virConsoleShutdown(con);
+            virConsoleShutdown(con, done == 0);
             goto cleanup;
         }
         memmove(con->terminalToStream.data,
@@ -214,7 +223,7 @@ virConsoleEventOnStream(virStreamPtr st,
 
     if (events & VIR_STREAM_EVENT_ERROR ||
         events & VIR_STREAM_EVENT_HANGUP) {
-        virConsoleShutdown(con);
+        virConsoleShutdown(con, false);
     }
 
  cleanup:
@@ -244,7 +253,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, false);
                 goto cleanup;
             }
             con->terminalToStream.length += 1024;
@@ -258,17 +267,17 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED,
         if (got < 0) {
             if (errno != EAGAIN) {
                 virReportSystemError(errno, "%s", _("cannot read from stdin"));
-                virConsoleShutdown(con);
+                virConsoleShutdown(con, false);
             }
             goto cleanup;
         }
         if (got == 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin"));
-            virConsoleShutdown(con);
+            virConsoleShutdown(con, false);
             goto cleanup;
         }
         if (con->terminalToStream.data[con->terminalToStream.offset] == con->escapeChar) {
-            virConsoleShutdown(con);
+            virConsoleShutdown(con, true);
             goto cleanup;
         }
 
@@ -281,13 +290,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, false);
         goto cleanup;
     }
 
     if (events & VIR_EVENT_HANDLE_HANGUP) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin"));
-        virConsoleShutdown(con);
+        virConsoleShutdown(con, false);
         goto cleanup;
     }
 
@@ -320,7 +329,7 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED,
         if (done < 0) {
             if (errno != EAGAIN) {
                 virReportSystemError(errno, "%s", _("cannot write to stdout"));
-                virConsoleShutdown(con);
+                virConsoleShutdown(con, false);
             }
             goto cleanup;
         }
@@ -342,13 +351,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, false);
         goto cleanup;
     }
 
     if (events & VIR_EVENT_HANDLE_HANGUP) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdout"));
-        virConsoleShutdown(con);
+        virConsoleShutdown(con, false);
         goto cleanup;
     }
 
@@ -487,7 +496,7 @@ virshRunConsole(vshControl *ctl,
         ret = 0;
 
  cleanup:
-    virConsoleShutdown(con);
+    virConsoleShutdown(con, ret == 0);
 
     if (ret < 0) {
         vshResetLibvirtError();
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Allow graceful console shutdown
Posted by Roman Bolshakov 4 years, 8 months ago 
On Fri, Aug 23, 2019 at 03:33:36PM +0200, Michal Privoznik wrote:
> Currently, whenever there's a regular EOF on the console stream
> or an error the virStreamAbort() is called regardless. While this
> may not actually break anything, we should call virStreamFinish()
> to let the daemon know we've successfully received all the data
> and are shutting down the stream gracefully.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  tools/virsh-console.c | 41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
> index a235a9a283..900faa5087 100644
> --- a/tools/virsh-console.c
> +++ b/tools/virsh-console.c
> @@ -193,7 +202,7 @@ virConsoleEventOnStream(virStreamPtr st,
>          if (done == -2)
>              goto cleanup; /* blocking */
>          if (done < 0) {
> -            virConsoleShutdown(con);
> +            virConsoleShutdown(con, done == 0);
>              goto cleanup;
>          }
>          memmove(con->terminalToStream.data,

done is always non-zero here. If that's expected, a constant would be a
bit more cleaner here to sign explicit abort, rather than conditional.

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>

Thanks,
Roman

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Allow graceful console shutdown
Posted by Michal Privoznik 4 years, 8 months ago 
On 8/23/19 7:40 PM, Roman Bolshakov wrote:
> On Fri, Aug 23, 2019 at 03:33:36PM +0200, Michal Privoznik wrote:
>> Currently, whenever there's a regular EOF on the console stream
>> or an error the virStreamAbort() is called regardless. While this
>> may not actually break anything, we should call virStreamFinish()
>> to let the daemon know we've successfully received all the data
>> and are shutting down the stream gracefully.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   tools/virsh-console.c | 41 +++++++++++++++++++++++++----------------
>>   1 file changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/tools/virsh-console.c b/tools/virsh-console.c
>> index a235a9a283..900faa5087 100644
>> --- a/tools/virsh-console.c
>> +++ b/tools/virsh-console.c
>> @@ -193,7 +202,7 @@ virConsoleEventOnStream(virStreamPtr st,
>>           if (done == -2)
>>               goto cleanup; /* blocking */
>>           if (done < 0) {
>> -            virConsoleShutdown(con);
>> +            virConsoleShutdown(con, done == 0);
>>               goto cleanup;
>>           }
>>           memmove(con->terminalToStream.data,
> 
> done is always non-zero here. If that's expected, a constant would be a
> bit more cleaner here to sign explicit abort, rather than conditional.

Ah, good point.

> 
> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> 

Thanks, I've pushed this.

Michal

--
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.