[Qemu-devel] [PATCH v12 0/6] scripts/qemu.py fixes and cleanups

[Qemu-devel] [PATCH v12 0/6] scripts/qemu.py fixes and cleanups

[Amador Pahim]

Changes v11->v12:
 - Rebase.
 - Drop the already queued commit (qemu.py: remove unused import)
 - Fix code filling the _monitor_address when it's None.
 - Drop the redundant commit checking whether VM was running on launch()
 - Improve the exception message when launch() is called twice with no
   shutdown() in between.
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.

*** SUBJECT HERE ***

*** BLURB HERE ***

Amador Pahim (6):
  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: don't launch again before shutdown()

 scripts/qemu.py | 93 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 62 insertions(+), 31 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v12 1/6] 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 | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 9bfdf6d37d..453a67250a 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -18,6 +18,8 @@ import os
 import sys
 import subprocess
 import qmp.qmp
+import shutil
+import tempfile
 
 
 LOG = logging.getLogger(__name__)
@@ -73,10 +75,11 @@ 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._vm_monitor = None
+        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
@@ -86,6 +89,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()
@@ -168,36 +173,50 @@ class QEMUMachine(object):
                 self._monitor_address[0],
                 self._monitor_address[1])
         else:
-            moncdev = 'socket,id=mon,path=%s' % self._monitor_address
+            moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
         return ['-chardev', moncdev,
                 '-mon', 'chardev=mon,mode=control',
                 '-display', 'none', '-vga', 'none']
 
     def _pre_launch(self):
-        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
+        self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
+        if self._monitor_address is not None:
+            self._vm_monitor = self._monitor_address
+        else:
+            self._vm_monitor = 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._vm_monitor,
                                                 server=True)
 
     def _post_launch(self):
         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.14.3

[Qemu-devel] [PATCH v12 2/6] 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>
Reviewed-by: Eduardo Habkost <ehabkost@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 453a67250a..0c690499be 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -206,20 +206,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()
@@ -234,6 +228,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.14.3

[Qemu-devel] [PATCH v12 3/6] 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>
Reviewed-by: Eduardo Habkost <ehabkost@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 0c690499be..d6661870ab 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -164,8 +164,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):
@@ -258,8 +259,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.14.3

[Qemu-devel] [PATCH v12 4/6] 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>
eviewed-by: Eduardo Habkost <ehabkost@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 d6661870ab..874ac2e424 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -151,12 +151,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.14.3

[Qemu-devel] [PATCH v12 5/6] 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>
Reviewed-by: Eduardo Habkost <ehabkost@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 874ac2e424..42a3fa5251 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -216,11 +216,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.14.3

[Qemu-devel] [PATCH v12 6/6] 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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 42a3fa5251..6c80edfd04 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -91,6 +91,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()
@@ -211,10 +212,15 @@ class QEMUMachine(object):
         Launch the VM and make sure we cleanup and expose the
         command line/output in case of exception
         """
+
+        if self._launched:
+            raise QEMUMachineError('VM already launched')
+
         self._iolog = None
         self._qemu_full_args = None
         try:
             self._launch()
+            self._launched = True
         except:
             self.shutdown()
 
@@ -267,6 +273,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.14.3

Re: [Qemu-devel] [PATCH v12 2/6] qemu.py: refactor launch()

[Eric Blake]

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

On 01/22/2018 02:50 PM, Amador Pahim wrote:
> This is just an refactor to separate the exception handler from the

s/an refactor/a refactor/

> actual launch procedure, improving the readability and making future
> maintenances in this piece of code easier.
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Amador Pahim <apahim@redhat.com>
> ---
>  scripts/qemu.py | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v12 1/6] qemu.py: better control of created files

[Eduardo Habkost]

On Mon, Jan 22, 2018 at 09:50:28PM +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 | 39 +++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 9bfdf6d37d..453a67250a 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -18,6 +18,8 @@ import os
>  import sys
>  import subprocess
>  import qmp.qmp
> +import shutil
> +import tempfile
>  
>  
>  LOG = logging.getLogger(__name__)
> @@ -73,10 +75,11 @@ 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._vm_monitor = None
> +        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
> @@ -86,6 +89,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()
> @@ -168,36 +173,50 @@ class QEMUMachine(object):
>                  self._monitor_address[0],
>                  self._monitor_address[1])

If _monitor_address now needs to be translated to _vm_monitor,
I'd like to remove all usage of _monitor_address outside
_pre_launch, to avoid confusion between the two attributes.

This can be done in a follow-up patch.

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

>          else:
> -            moncdev = 'socket,id=mon,path=%s' % self._monitor_address
> +            moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
>          return ['-chardev', moncdev,
>                  '-mon', 'chardev=mon,mode=control',
>                  '-display', 'none', '-vga', 'none']
>  
>      def _pre_launch(self):
> -        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
> +        self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
> +        if self._monitor_address is not None:
> +            self._vm_monitor = self._monitor_address
> +        else:
> +            self._vm_monitor = 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._vm_monitor,
>                                                  server=True)
>  
>      def _post_launch(self):
>          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.14.3
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v12 0/6] scripts/qemu.py fixes and cleanups

[Eduardo Habkost]

Queued on python-next, thanks!

On Mon, Jan 22, 2018 at 09:50:27PM +0100, Amador Pahim wrote:
> Changes v11->v12:
>  - Rebase.
>  - Drop the already queued commit (qemu.py: remove unused import)
>  - Fix code filling the _monitor_address when it's None.
>  - Drop the redundant commit checking whether VM was running on launch()
>  - Improve the exception message when launch() is called twice with no
>    shutdown() in between.
> 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.
> 
> *** SUBJECT HERE ***
> 
> *** BLURB HERE ***
> 
> Amador Pahim (6):
>   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: don't launch again before shutdown()
> 
>  scripts/qemu.py | 93 ++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 62 insertions(+), 31 deletions(-)
> 
> -- 
> 2.14.3
> 
> 

-- 
Eduardo