Re: [PATCH v5 05/12] python/machine.py: Prohibit multiple shutdown() calls

[Cleber Rosa <crosa@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3263 bytes --]

On Fri, Jul 10, 2020 at 01:06:42AM -0400, John Snow wrote:
> If the VM is not launched, don't try to shut it down. As a change,
> _post_shutdown now unconditionally also calls _early_cleanup in order to
> offer comprehensive object cleanup in failure cases.
> 
> As a courtesy, treat it as a NOP instead of rejecting it as an
> error. This is slightly nicer for acceptance tests where vm.shutdown()
> is issued unconditionally in tearDown callbacks.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine.py | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index cac466fbe6..e66a7d66dd 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -283,6 +283,13 @@ def _post_launch(self):
>              self._qmp.accept()
>  
>      def _post_shutdown(self):
> +        """
> +        Called to cleanup the VM instance after the process has exited.
> +        May also be called after a failed launch.
> +        """
> +        # Comprehensive reset for the failed launch case:
> +        self._early_cleanup()
> +

I'm not following why this is needed here.  AFAICT, the closing of the
console socket file was introduced when the QEMU process was alive,
and doing I/O on the serial device attached to the console file (and
was only necessary because of that).  In those scenarios, a race
between the time of sending the QMP command to quit, and getting a
response from QMP could occur.

But here, IIUC, there's no expectations for the QEMU process to be alive.
To the best of my understanding and testing, this line did not contribute
to the robustness of the shutdown behavior.

Finally, given that currently, only the closing of the console socket
is done within _early_cleanup(), and that is conditional, this also does
no harm AFAICT.  If more early cleanups operations were needed, then I
would feel less bothered about calling it here.

>          if self._qmp:
>              self._qmp.close()
>              self._qmp = None
> @@ -328,7 +335,7 @@ def launch(self):
>              self._launch()
>              self._launched = True
>          except:
> -            self.shutdown()
> +            self._post_shutdown()
>  
>              LOG.debug('Error launching VM')
>              if self._qemu_full_args:
> @@ -357,6 +364,8 @@ def _launch(self):
>      def _early_cleanup(self) -> None:
>          """
>          Perform any cleanup that needs to happen before the VM exits.
> +
> +        Called additionally by _post_shutdown for comprehensive cleanup.
>          """
>          # If we keep the console socket open, we may deadlock waiting
>          # for QEMU to exit, while QEMU is waiting for the socket to
> @@ -377,6 +386,9 @@ def shutdown(self, has_quit=False, hard=False):
>          """
>          Terminate the VM and clean up
>          """
> +        if not self._launched:
> +            return
> +

Because of the additional logic, it'd be a good opportunity to make
the docstring more accurate.  This method may now *not* do *any* of if
termination and cleaning (while previously it would attempt those
anyhow).

- Cleber.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]