[Qemu-devel] [PATCH v11 0/8] scripts/qemu.py fixes and cleanups

[Qemu-devel] [PATCH v11 0/8] scripts/qemu.py fixes and cleanups

[Amador Pahim]

Changes v10->v11:
 - Fix messed indentation.
 - Rename self._qemu_log_fd to self._qemu_log_file.
 - Patch changelog in reverse order.
Changes v9->v10:
 - Keep method _remove_if_exists().
Changes v8->v9:
 - Some commits were already merged. Rebased the remaining ones.
 - New commit to remove unused import.
 - VM files control logic changed to use a temporary directory.
 - Better naming for variable controlling the need of a shutdown before
   next launch.
Changes v7->v8:
 - Rebased.
 - Reorder commits to avoid break->fix sequence.
 - Split commits "use poll() instead of 'returncode'" and "refactor
   launch()".
 - Don't ignore errors in _load_io_log(). Instead, check if we created
   the file before reading it.
 - Use LOG.warn() instead of LOG.debug() for the negative exit code
   message.
 - Fix the exception name called in commits "launch vm only if it's not
   running" and "don't launch again before shutdown()".
 - Minor style fixes.
Changes v6->v7:
 - Split commits in self-contained/atomic changes.
 - Addressed the comments from previous version, basically improving the
   logging messages and the control over created files. See individual
   commit messages for details.
Changes v5->v6:
 - Remove the commit to rename self._args.
 - Fix is_running() return before first call to maunch().
 - Use python logging system.
 - Include the full command line on negative exit code error message.
 - Use os.path.null instead of /dev/null.
 - Improve the control over the created/deleted files.
Changes v4->v5:
 - Break the cleanup commit into logical changes and include in the
   commit messages the rationale for making them.
Changes v3->v4:
 - Squash the 2 first commits since they are co-dependant.
 - Cleanup launch() and shutdown().
 - Reorder the commits, putting the rename of self._args first.
 - Rebased.
Changes v2->v3:
 - Fix typo in patch 3 ("qemu.py: make 'args' public") commit message.
Changes v1->v2:
 - Style fixes to make checkpatch.pl happy.
 - Rebased.

Amador Pahim (8):
  qemu.py: remove unused import
  qemu.py: better control of created files
  qemu.py: refactor launch()
  qemu.py: always cleanup on shutdown()
  qemu.py: use poll() instead of 'returncode'
  qemu.py: cleanup redundant calls in launch()
  qemu.py: launch vm only if it's not running
  qemu.py: don't launch again before shutdown()

 scripts/qemu.py | 89 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 30 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH v11 1/8] qemu.py: remove unused import

[Amador Pahim]

Removing 'import sys' as it's not used anywhere.

Signed-off-by: Amador Pahim <apahim@redhat.com>
---
 scripts/qemu.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 9bfdf6d37d..65d9ad688c 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -15,7 +15,6 @@
 import errno
 import logging
 import os
-import sys
 import subprocess
 import qmp.qmp
 
-- 
2.13.6

[Qemu-devel] [PATCH v11 2/8] qemu.py: better control of created files

[Amador Pahim]

To launch a VM, we need to create basically two files: the monitor
socket (if it's a UNIX socket) and the qemu log file.

For the qemu log file, we currently just open the path, which will
create the file if it does not exist or overwrite the file if it does
exist.

For the monitor socket, if it already exists, we are currently removing
it, even if it's not created by us.

This patch moves to _pre_launch() the responsibility to create a
temporary directory to host the files so we can remove the whole
directory on _post_shutdown().

Signed-off-by: Amador Pahim <apahim@redhat.com>
---
 scripts/qemu.py | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 65d9ad688c..d5b1cde044 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -17,6 +17,8 @@ import logging
 import os
 import subprocess
 import qmp.qmp
+import shutil
+import tempfile
 
 
 LOG = logging.getLogger(__name__)
@@ -72,10 +74,10 @@ class QEMUMachine(object):
             wrapper = []
         if name is None:
             name = "qemu-%d" % os.getpid()
-        if monitor_address is None:
-            monitor_address = os.path.join(test_dir, name + "-monitor.sock")
+        self._name = name
         self._monitor_address = monitor_address
-        self._qemu_log_path = os.path.join(test_dir, name + ".log")
+        self._qemu_log_path = None
+        self._qemu_log_file = None
         self._popen = None
         self._binary = binary
         self._args = list(args)     # Force copy args in case we modify them
@@ -85,6 +87,8 @@ class QEMUMachine(object):
         self._socket_scm_helper = socket_scm_helper
         self._qmp = None
         self._qemu_full_args = None
+        self._test_dir = test_dir
+        self._temp_dir = None
 
         # just in case logging wasn't configured by the main script:
         logging.basicConfig()
@@ -173,6 +177,13 @@ class QEMUMachine(object):
                 '-display', 'none', '-vga', 'none']
 
     def _pre_launch(self):
+        self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
+        if not isinstance(self._monitor_address, tuple):
+            self._monitor_address = os.path.join(self._temp_dir,
+                                                 self._name + "-monitor.sock")
+        self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
+        self._qemu_log_file = open(self._qemu_log_path, 'wb')
+
         self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
                                                 server=True)
 
@@ -180,23 +191,28 @@ class QEMUMachine(object):
         self._qmp.accept()
 
     def _post_shutdown(self):
-        if not isinstance(self._monitor_address, tuple):
-            self._remove_if_exists(self._monitor_address)
-        self._remove_if_exists(self._qemu_log_path)
+        if self._qemu_log_file is not None:
+            self._qemu_log_file.close()
+            self._qemu_log_file = None
+
+        self._qemu_log_path = None
+
+        if self._temp_dir is not None:
+            shutil.rmtree(self._temp_dir)
+            self._temp_dir = None
 
     def launch(self):
         '''Launch the VM and establish a QMP connection'''
         self._iolog = None
         self._qemu_full_args = None
         devnull = open(os.path.devnull, 'rb')
-        qemulog = open(self._qemu_log_path, 'wb')
         try:
             self._pre_launch()
             self._qemu_full_args = (self._wrapper + [self._binary] +
                                     self._base_args() + self._args)
             self._popen = subprocess.Popen(self._qemu_full_args,
                                            stdin=devnull,
-                                           stdout=qemulog,
+                                           stdout=self._qemu_log_file,
                                            stderr=subprocess.STDOUT,
                                            shell=False)
             self._post_launch()
-- 
2.13.6

[Qemu-devel] [PATCH v11 3/8] qemu.py: refactor launch()

[Amador Pahim]

This is just an refactor to separate the exception handler from the
actual launch procedure, improving the readability and making future
maintenances in this piece of code easier.

Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Amador Pahim <apahim@redhat.com>
---
 scripts/qemu.py | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index d5b1cde044..28aa3712c7 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -202,20 +202,14 @@ class QEMUMachine(object):
             self._temp_dir = None
 
     def launch(self):
-        '''Launch the VM and establish a QMP connection'''
+        """
+        Launch the VM and make sure we cleanup and expose the
+        command line/output in case of exception
+        """
         self._iolog = None
         self._qemu_full_args = None
-        devnull = open(os.path.devnull, 'rb')
         try:
-            self._pre_launch()
-            self._qemu_full_args = (self._wrapper + [self._binary] +
-                                    self._base_args() + self._args)
-            self._popen = subprocess.Popen(self._qemu_full_args,
-                                           stdin=devnull,
-                                           stdout=self._qemu_log_file,
-                                           stderr=subprocess.STDOUT,
-                                           shell=False)
-            self._post_launch()
+            self._launch()
         except:
             if self.is_running():
                 self._popen.kill()
@@ -230,6 +224,19 @@ class QEMUMachine(object):
                 LOG.debug('Output: %r', self._iolog)
             raise
 
+    def _launch(self):
+        '''Launch the VM and establish a QMP connection'''
+        devnull = open(os.path.devnull, 'rb')
+        self._pre_launch()
+        self._qemu_full_args = (self._wrapper + [self._binary] +
+                                self._base_args() + self._args)
+        self._popen = subprocess.Popen(self._qemu_full_args,
+                                       stdin=devnull,
+                                       stdout=self._qemu_log_file,
+                                       stderr=subprocess.STDOUT,
+                                       shell=False)
+        self._post_launch()
+
     def wait(self):
         '''Wait for the VM to power off'''
         self._popen.wait()
-- 
2.13.6

[Qemu-devel] [PATCH v11 4/8] qemu.py: always cleanup on shutdown()

[Amador Pahim]

Currently we only cleanup on shutdown() if the VM is running.

To make sure we will always cleanup, this patch makes the
self._load_io_log() and the self._post_shutdown() to
always be called on shutdown(), regardless the VM running state.

Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Amador Pahim <apahim@redhat.com>
---
 scripts/qemu.py | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 28aa3712c7..7bd10b1a1d 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -162,8 +162,9 @@ class QEMUMachine(object):
         return self._popen.pid
 
     def _load_io_log(self):
-        with open(self._qemu_log_path, "r") as iolog:
-            self._iolog = iolog.read()
+        if self._qemu_log_path is not None:
+            with open(self._qemu_log_path, "r") as iolog:
+                self._iolog = iolog.read()
 
     def _base_args(self):
         if isinstance(self._monitor_address, tuple):
@@ -254,8 +255,8 @@ class QEMUMachine(object):
                 self._popen.kill()
             self._popen.wait()
 
-            self._load_io_log()
-            self._post_shutdown()
+        self._load_io_log()
+        self._post_shutdown()
 
         exitcode = self.exitcode()
         if exitcode is not None and exitcode < 0:
-- 
2.13.6

[Qemu-devel] [PATCH v11 5/8] qemu.py: use poll() instead of 'returncode'

[Amador Pahim]

The 'returncode' Popen attribute is not guaranteed to be updated. It
actually depends on a call to either poll(), wait() or communicate().

On the other hand, poll() will: "Check if child process has terminated.
Set and return returncode attribute."

Let's use the poll() to check whether the process is running and to get
the updated process exit code, when the process is finished.

Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Amador Pahim <apahim@redhat.com>
---
 scripts/qemu.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 7bd10b1a1d..d3824a7a0b 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -149,12 +149,12 @@ class QEMUMachine(object):
             raise
 
     def is_running(self):
-        return self._popen is not None and self._popen.returncode is None
+        return self._popen is not None and self._popen.poll() is None
 
     def exitcode(self):
         if self._popen is None:
             return None
-        return self._popen.returncode
+        return self._popen.poll()
 
     def get_pid(self):
         if not self.is_running():
-- 
2.13.6

[Qemu-devel] [PATCH v11 6/8] qemu.py: cleanup redundant calls in launch()

[Amador Pahim]

Now that shutdown() is guaranteed to always execute self._load_io_log()
and self._post_shutdown(), their calls in 'except' became redundant and
we can safely replace it by a call to shutdown().

Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Amador Pahim <apahim@redhat.com>
---
 scripts/qemu.py | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index d3824a7a0b..305d7bd098 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -212,11 +212,7 @@ class QEMUMachine(object):
         try:
             self._launch()
         except:
-            if self.is_running():
-                self._popen.kill()
-                self._popen.wait()
-            self._load_io_log()
-            self._post_shutdown()
+            self.shutdown()
 
             LOG.debug('Error launching VM')
             if self._qemu_full_args:
-- 
2.13.6

[Qemu-devel] [PATCH v11 7/8] qemu.py: launch vm only if it's not running

[Amador Pahim]

If we allow launching VM again we will loose track of the first launched
VM.

Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Amador Pahim <apahim@redhat.com>
---
 scripts/qemu.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 305d7bd098..0b0b61be39 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -207,6 +207,9 @@ class QEMUMachine(object):
         Launch the VM and make sure we cleanup and expose the
         command line/output in case of exception
         """
+        if self.is_running():
+            raise QEMUMachineError('VM already running')
+
         self._iolog = None
         self._qemu_full_args = None
         try:
-- 
2.13.6

[Qemu-devel] [PATCH v11 8/8] qemu.py: don't launch again before shutdown()

[Amador Pahim]

If a VM is launched, files are created and a cleanup is required before
a new launch. This cleanup is executed by shutdown(), so shutdown() must
be called even if the VM is manually terminated (i.e. using kill).

This patch creates a control to make sure launch() will not be executed
again if shutdown() is not called after the previous launch().

Signed-off-by: Amador Pahim <apahim@redhat.com>
---
 scripts/qemu.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 0b0b61be39..862920099c 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -89,6 +89,7 @@ class QEMUMachine(object):
         self._qemu_full_args = None
         self._test_dir = test_dir
         self._temp_dir = None
+        self._launched = False
 
         # just in case logging wasn't configured by the main script:
         logging.basicConfig()
@@ -210,10 +211,14 @@ class QEMUMachine(object):
         if self.is_running():
             raise QEMUMachineError('VM already running')
 
+        if self._launched:
+            raise QEMUMachineError('Shutdown pending after previous launch')
+
         self._iolog = None
         self._qemu_full_args = None
         try:
             self._launch()
+            self._launched = True
         except:
             self.shutdown()
 
@@ -266,6 +271,8 @@ class QEMUMachine(object):
                 command = ''
             LOG.warn(msg, exitcode, command)
 
+        self._launched = False
+
     def qmp(self, cmd, conv_keys=True, **args):
         '''Invoke a QMP command and return the response dict'''
         qmp_args = dict()
-- 
2.13.6

Re: [Qemu-devel] [PATCH v11 2/8] qemu.py: better control of created files

[Eduardo Habkost]

Hi Amador,

First of all, sorry for reviewing this so late.

On Tue, Nov 14, 2017 at 11:22:40AM +0100, Amador Pahim wrote:
> To launch a VM, we need to create basically two files: the monitor
> socket (if it's a UNIX socket) and the qemu log file.
> 
> For the qemu log file, we currently just open the path, which will
> create the file if it does not exist or overwrite the file if it does
> exist.
> 
> For the monitor socket, if it already exists, we are currently removing
> it, even if it's not created by us.
> 
> This patch moves to _pre_launch() the responsibility to create a
> temporary directory to host the files so we can remove the whole
> directory on _post_shutdown().
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>
> ---
>  scripts/qemu.py | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 65d9ad688c..d5b1cde044 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -17,6 +17,8 @@ import logging
>  import os
>  import subprocess
>  import qmp.qmp
> +import shutil
> +import tempfile
>  
>  
>  LOG = logging.getLogger(__name__)
> @@ -72,10 +74,10 @@ class QEMUMachine(object):
>              wrapper = []
>          if name is None:
>              name = "qemu-%d" % os.getpid()
> -        if monitor_address is None:
> -            monitor_address = os.path.join(test_dir, name + "-monitor.sock")
> +        self._name = name
>          self._monitor_address = monitor_address
> -        self._qemu_log_path = os.path.join(test_dir, name + ".log")
> +        self._qemu_log_path = None
> +        self._qemu_log_file = None
>          self._popen = None
>          self._binary = binary
>          self._args = list(args)     # Force copy args in case we modify them
> @@ -85,6 +87,8 @@ class QEMUMachine(object):
>          self._socket_scm_helper = socket_scm_helper
>          self._qmp = None
>          self._qemu_full_args = None
> +        self._test_dir = test_dir
> +        self._temp_dir = None
>  
>          # just in case logging wasn't configured by the main script:
>          logging.basicConfig()
> @@ -173,6 +177,13 @@ class QEMUMachine(object):
>                  '-display', 'none', '-vga', 'none']
>  
>      def _pre_launch(self):
> +        self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
> +        if not isinstance(self._monitor_address, tuple):
> +            self._monitor_address = os.path.join(self._temp_dir,
> +                                                 self._name + "-monitor.sock")

Won't this break QEMUMachine(..., monitor_addres='/some/unix/path')?

What about checking if self._monitor_address is None instead?

The rest looks good to me.

> +        self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
> +        self._qemu_log_file = open(self._qemu_log_path, 'wb')
> +
>          self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
>                                                  server=True)
>  
> @@ -180,23 +191,28 @@ class QEMUMachine(object):
>          self._qmp.accept()
>  
>      def _post_shutdown(self):
> -        if not isinstance(self._monitor_address, tuple):
> -            self._remove_if_exists(self._monitor_address)
> -        self._remove_if_exists(self._qemu_log_path)
> +        if self._qemu_log_file is not None:
> +            self._qemu_log_file.close()
> +            self._qemu_log_file = None
> +
> +        self._qemu_log_path = None
> +
> +        if self._temp_dir is not None:
> +            shutil.rmtree(self._temp_dir)
> +            self._temp_dir = None
>  
>      def launch(self):
>          '''Launch the VM and establish a QMP connection'''
>          self._iolog = None
>          self._qemu_full_args = None
>          devnull = open(os.path.devnull, 'rb')
> -        qemulog = open(self._qemu_log_path, 'wb')
>          try:
>              self._pre_launch()
>              self._qemu_full_args = (self._wrapper + [self._binary] +
>                                      self._base_args() + self._args)
>              self._popen = subprocess.Popen(self._qemu_full_args,
>                                             stdin=devnull,
> -                                           stdout=qemulog,
> +                                           stdout=self._qemu_log_file,
>                                             stderr=subprocess.STDOUT,
>                                             shell=False)
>              self._post_launch()
> -- 
> 2.13.6
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 1/8] qemu.py: remove unused import

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:39AM +0100, Amador Pahim wrote:
> Removing 'import sys' as it's not used anywhere.
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>

Queued on python-next.  Thanks, and sorry for taking 2 months to
do it.

> ---
>  scripts/qemu.py | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 9bfdf6d37d..65d9ad688c 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -15,7 +15,6 @@
>  import errno
>  import logging
>  import os
> -import sys
>  import subprocess
>  import qmp.qmp
>  
> -- 
> 2.13.6
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 3/8] qemu.py: refactor launch()

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:41AM +0100, Amador Pahim wrote:
> This is just an refactor to separate the exception handler from the
> actual launch procedure, improving the readability and making future
> maintenances in this piece of code easier.
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Amador Pahim <apahim@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

The only thing preventing me from queueing it right now is the
dependency on patch 2/8.

> ---
>  scripts/qemu.py | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index d5b1cde044..28aa3712c7 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -202,20 +202,14 @@ class QEMUMachine(object):
>              self._temp_dir = None
>  
>      def launch(self):
> -        '''Launch the VM and establish a QMP connection'''
> +        """
> +        Launch the VM and make sure we cleanup and expose the
> +        command line/output in case of exception
> +        """
>          self._iolog = None
>          self._qemu_full_args = None
> -        devnull = open(os.path.devnull, 'rb')
>          try:
> -            self._pre_launch()
> -            self._qemu_full_args = (self._wrapper + [self._binary] +
> -                                    self._base_args() + self._args)
> -            self._popen = subprocess.Popen(self._qemu_full_args,
> -                                           stdin=devnull,
> -                                           stdout=self._qemu_log_file,
> -                                           stderr=subprocess.STDOUT,
> -                                           shell=False)
> -            self._post_launch()
> +            self._launch()
>          except:
>              if self.is_running():
>                  self._popen.kill()
> @@ -230,6 +224,19 @@ class QEMUMachine(object):
>                  LOG.debug('Output: %r', self._iolog)
>              raise
>  
> +    def _launch(self):
> +        '''Launch the VM and establish a QMP connection'''
> +        devnull = open(os.path.devnull, 'rb')
> +        self._pre_launch()
> +        self._qemu_full_args = (self._wrapper + [self._binary] +
> +                                self._base_args() + self._args)
> +        self._popen = subprocess.Popen(self._qemu_full_args,
> +                                       stdin=devnull,
> +                                       stdout=self._qemu_log_file,
> +                                       stderr=subprocess.STDOUT,
> +                                       shell=False)
> +        self._post_launch()
> +
>      def wait(self):
>          '''Wait for the VM to power off'''
>          self._popen.wait()
> -- 
> 2.13.6
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 4/8] qemu.py: always cleanup on shutdown()

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:42AM +0100, Amador Pahim wrote:
> Currently we only cleanup on shutdown() if the VM is running.
> 
> To make sure we will always cleanup, this patch makes the
> self._load_io_log() and the self._post_shutdown() to
> always be called on shutdown(), regardless the VM running state.
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Amador Pahim <apahim@redhat.com>
> ---
>  scripts/qemu.py | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 28aa3712c7..7bd10b1a1d 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -162,8 +162,9 @@ class QEMUMachine(object):
>          return self._popen.pid
>  
>      def _load_io_log(self):
> -        with open(self._qemu_log_path, "r") as iolog:
> -            self._iolog = iolog.read()
> +        if self._qemu_log_path is not None:
> +            with open(self._qemu_log_path, "r") as iolog:
> +                self._iolog = iolog.read()
>  
>      def _base_args(self):
>          if isinstance(self._monitor_address, tuple):
> @@ -254,8 +255,8 @@ class QEMUMachine(object):
>                  self._popen.kill()
>              self._popen.wait()
>  
> -            self._load_io_log()
> -            self._post_shutdown()
> +        self._load_io_log()

_load_io_log() is now safe to call at any time thanks to the hunk
above.

> +        self._post_shutdown()

_post_shutdown() is safe to call at any time thanks to patch 2/8.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

>  
>          exitcode = self.exitcode()
>          if exitcode is not None and exitcode < 0:
> -- 
> 2.13.6
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 5/8] qemu.py: use poll() instead of 'returncode'

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:43AM +0100, Amador Pahim wrote:
> The 'returncode' Popen attribute is not guaranteed to be updated. It
> actually depends on a call to either poll(), wait() or communicate().
> 
> On the other hand, poll() will: "Check if child process has terminated.
> Set and return returncode attribute."
> 
> Let's use the poll() to check whether the process is running and to get
> the updated process exit code, when the process is finished.
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Amador Pahim <apahim@redhat.com>
> ---
>  scripts/qemu.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 7bd10b1a1d..d3824a7a0b 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -149,12 +149,12 @@ class QEMUMachine(object):
>              raise
>  
>      def is_running(self):
> -        return self._popen is not None and self._popen.returncode is None
> +        return self._popen is not None and self._popen.poll() is None

This should be safe now, after patch 4/8.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 6/8] qemu.py: cleanup redundant calls in launch()

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:44AM +0100, Amador Pahim wrote:
> Now that shutdown() is guaranteed to always execute self._load_io_log()
> and self._post_shutdown(), their calls in 'except' became redundant and
> we can safely replace it by a call to shutdown().
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Amador Pahim <apahim@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> ---
>  scripts/qemu.py | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index d3824a7a0b..305d7bd098 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -212,11 +212,7 @@ class QEMUMachine(object):
>          try:
>              self._launch()
>          except:
> -            if self.is_running():
> -                self._popen.kill()
> -                self._popen.wait()
> -            self._load_io_log()
> -            self._post_shutdown()
> +            self.shutdown()
>  
>              LOG.debug('Error launching VM')
>              if self._qemu_full_args:
> -- 
> 2.13.6
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 7/8] qemu.py: launch vm only if it's not running

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:45AM +0100, Amador Pahim wrote:
> If we allow launching VM again we will loose track of the first launched
> VM.
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Amador Pahim <apahim@redhat.com>
> ---
>  scripts/qemu.py | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 305d7bd098..0b0b61be39 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -207,6 +207,9 @@ class QEMUMachine(object):
>          Launch the VM and make sure we cleanup and expose the
>          command line/output in case of exception
>          """
> +        if self.is_running():
> +            raise QEMUMachineError('VM already running')

Patch 8/8 seems to make this redundant, as is_running() can't
return True if _launched is True.

I suggest replacing this and patch 8/8 with a single check:

        if self._launched:
            raise QEMUMachineError('VM already launched')

-- 
Eduardo

Re: [Qemu-devel] [PATCH v11 8/8] qemu.py: don't launch again before shutdown()

[Eduardo Habkost]

On Tue, Nov 14, 2017 at 11:22:46AM +0100, Amador Pahim wrote:
> If a VM is launched, files are created and a cleanup is required before
> a new launch. This cleanup is executed by shutdown(), so shutdown() must
> be called even if the VM is manually terminated (i.e. using kill).
> 
> This patch creates a control to make sure launch() will not be executed
> again if shutdown() is not called after the previous launch().
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>
> ---
>  scripts/qemu.py | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 0b0b61be39..862920099c 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -89,6 +89,7 @@ class QEMUMachine(object):
>          self._qemu_full_args = None
>          self._test_dir = test_dir
>          self._temp_dir = None
> +        self._launched = False
>  
>          # just in case logging wasn't configured by the main script:
>          logging.basicConfig()
> @@ -210,10 +211,14 @@ class QEMUMachine(object):
>          if self.is_running():
>              raise QEMUMachineError('VM already running')
>  
> +        if self._launched:
> +            raise QEMUMachineError('Shutdown pending after previous launch')

I believe "VM already launched" would be clearer than "shutdown
pending". 

-- 
Eduardo