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

[John Snow]

On 7/13/20 10:48 PM, Cleber Rosa wrote:
> 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).
> 

The docstring gets hit with a polish beam later in the series, but still
neglects that it might do nothing.

I squashed in a change to patch #10 and pushed to my gitlab to add this
clarification to the docstring.

https://gitlab.com/jsnow/qemu/-/tree/python-package-shutdown

https://gitlab.com/jsnow/qemu/-/commit/b3f14deb730fda9bb2a28a9366a8096d3617f4f4