Catch only the timeout error; if there are other problems, allow the
stack trace to be visible.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index b9a98e2c86..e3ea523571 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -342,7 +342,26 @@ def wait(self):
         self._load_io_log()
         self._post_shutdown()
 
-    def shutdown(self, has_quit=False):
+    def _issue_shutdown(self, has_quit: bool = False) -> None:
+        """
+        Shutdown the VM.
+        """
+        if not self.is_running():
+            return
+
+        if self._qmp is not None:
+            if not has_quit:
+                self._qmp.cmd('quit')
+            self._qmp.close()
+
+            try:
+                self._popen.wait(timeout=3)
+            except subprocess.TimeoutExpired:
+                self._popen.kill()
+
+        self._popen.wait()
+
+    def shutdown(self, has_quit: bool = False) -> None:
         """
         Terminate the VM and clean up
         """
@@ -353,17 +372,7 @@ def shutdown(self, has_quit=False):
             self._console_socket.close()
             self._console_socket = None
 
-        if self.is_running():
-            if self._qmp:
-                try:
-                    if not has_quit:
-                        self._qmp.cmd('quit')
-                    self._qmp.close()
-                    self._popen.wait(timeout=3)
-                except:
-                    self._popen.kill()
-            self._popen.wait()
-
+        self._issue_shutdown(has_quit)
         self._load_io_log()
         self._post_shutdown()
 
-- 
2.21.1

On 5/14/20 12:53 AM, John Snow wrote:
> Catch only the timeout error; if there are other problems, allow the
> stack trace to be visible.
> 

A lot of patches in this series start with "python//" - is that 
intentional, or should it be a single slash?

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/lib/machine.py | 33 +++++++++++++++++++++------------
>   1 file changed, 21 insertions(+), 12 deletions(-)
> 


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 5/14/20 9:55 AM, Eric Blake wrote:
> On 5/14/20 12:53 AM, John Snow wrote:
>> Catch only the timeout error; if there are other problems, allow the
>> stack trace to be visible.
>>
> 
> A lot of patches in this series start with "python//" - is that
> intentional, or should it be a single slash?
> 

Was trying to imply an elided path structure, because
"python/qemu/lib/qmp.py" et al is way too chatty.

Feel free to suggest better subject names!

On 5/14/20 4:26 PM, John Snow wrote:
> 
> 
> On 5/14/20 9:55 AM, Eric Blake wrote:
>> On 5/14/20 12:53 AM, John Snow wrote:
>>> Catch only the timeout error; if there are other problems, allow the
>>> stack trace to be visible.
>>>
>>
>> A lot of patches in this series start with "python//" - is that
>> intentional, or should it be a single slash?
>>
> 
> Was trying to imply an elided path structure, because
> "python/qemu/lib/qmp.py" et al is way too chatty.
> 
> Feel free to suggest better subject names!
> 

Eliding "qemu/lib/" is OK. I wouldn't not use double //, it sounds like
a recurrent typo.

On 5/14/20 7:53 AM, John Snow wrote:
> Catch only the timeout error; if there are other problems, allow the
> stack trace to be visible.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/machine.py | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
> index b9a98e2c86..e3ea523571 100644
> --- a/python/qemu/lib/machine.py
> +++ b/python/qemu/lib/machine.py
> @@ -342,7 +342,26 @@ def wait(self):
>          self._load_io_log()
>          self._post_shutdown()
>  
> -    def shutdown(self, has_quit=False):
> +    def _issue_shutdown(self, has_quit: bool = False) -> None:
> +        """
> +        Shutdown the VM.
> +        """
> +        if not self.is_running():
> +            return
> +
> +        if self._qmp is not None:
> +            if not has_quit:
> +                self._qmp.cmd('quit')
> +            self._qmp.close()
> +
> +            try:
> +                self._popen.wait(timeout=3)
> +            except subprocess.TimeoutExpired:
> +                self._popen.kill()
> +
> +        self._popen.wait()
> +
> +    def shutdown(self, has_quit: bool = False) -> None:
>          """
>          Terminate the VM and clean up
>          """
> @@ -353,17 +372,7 @@ def shutdown(self, has_quit=False):
>              self._console_socket.close()
>              self._console_socket = None
>  
> -        if self.is_running():
> -            if self._qmp:
> -                try:
> -                    if not has_quit:
> -                        self._qmp.cmd('quit')
> -                    self._qmp.close()
> -                    self._popen.wait(timeout=3)
> -                except:
> -                    self._popen.kill()
> -            self._popen.wait()
> -
> +        self._issue_shutdown(has_quit)
>          self._load_io_log()
>          self._post_shutdown()
>  
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Am 14.05.2020 um 07:53 hat John Snow geschrieben:
> Catch only the timeout error; if there are other problems, allow the
> stack trace to be visible.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Having a visible stack trace is nice, but don't we still want to kill
the qemu process to be sure that it's gone even though we can't wait for
it to be shut down cleanly?

Kevin

On 6/2/20 7:01 AM, Kevin Wolf wrote:
> Am 14.05.2020 um 07:53 hat John Snow geschrieben:
>> Catch only the timeout error; if there are other problems, allow the
>> stack trace to be visible.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Having a visible stack trace is nice, but don't we still want to kill
> the qemu process to be sure that it's gone even though we can't wait for
> it to be shut down cleanly?
> 
> Kevin
> 

Ah, yes. I should kill and re-raise. I think Vladimir checked in some
code here recently that kept this patch from applying, so I'll have to
re-tune it anyway.

Thanks for the spot.

--js