[Qemu-devel] [PATCH v8 00/13] scripts/qemu.py fixes and cleanups

[Qemu-devel] [PATCH v8 00/13] scripts/qemu.py fixes and cleanups

[Amador Pahim]

Changes v1->v2:
 - Style fixes to make checkpatch.pl happy.
 - Rebased.
Changes v2->v3:
 - Fix typo in patch 3 ("qemu.py: make 'args' public") commit message.
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 v4->v5:
 - Break the cleanup commit into logical changes and include in the
   commit messages the rationale for making them.
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 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 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.

Amador Pahim (13):
  qemu.py: fix is_running() return before first launch()
  qemu.py: avoid writing to stdout/stderr
  qemu.py: use os.path.null instead of /dev/null
  qemu.py: improve message on negative exit code
  qemu.py: include debug information on launch error
  qemu.py: make sure we only remove files we create
  qemu.py: close _qemu_log_path on cleanup
  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 | 136 +++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 101 insertions(+), 35 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH v8 01/13] qemu.py: fix is_running() return before first launch()

[Amador Pahim]

is_running() returns None when called before the first time we
call launch():

    >>> import qemu
    >>> vm = qemu.QEMUMachine('qemu-system-x86_64')
    >>> vm.is_running()
    >>>

It should return False instead. This patch fixes that.

For consistence, this patch removes the parenthesis from the
second clause as it's not really needed.

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

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 880e3e8219..0ae5d39414 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -86,7 +86,7 @@ class QEMUMachine(object):
             raise
 
     def is_running(self):
-        return self._popen and (self._popen.returncode is None)
+        return self._popen is not None and self._popen.returncode is None
 
     def exitcode(self):
         if self._popen is None:
-- 
2.13.5

[Qemu-devel] [PATCH v8 02/13] qemu.py: avoid writing to stdout/stderr

[Amador Pahim]

This module should not write directly to stdout/stderr. Instead, it
should either raise exceptions or just log the messages and let the
callers handle them and decide what to do. For example, scripts could
choose to send the log messages stderr or/and write them to a file if
verbose or debugging mode is enabled.

This patch replaces the writes to stderr by an exception in the
send_fd_scm() when _socket_scm_helper is not set or not present. In the
same method, the subprocess Popen will now redirect the stdout/stderr to
logging.debug instead of writing to system stderr. As consequence, since
the Popen.communicate() is now used (in order to get the stdout), the
further call to wait() became redundant and was replaced by
Popen.returncode.

The shutdown() message on negative exit code will now be logged
to logging.warn instead of written to system stderr.

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

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 0ae5d39414..aca6fa4d82 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -13,6 +13,7 @@
 #
 
 import errno
+import logging
 import string
 import os
 import sys
@@ -20,6 +21,15 @@ import subprocess
 import qmp.qmp
 
 
+LOG = logging.getLogger(__name__)
+
+
+class QEMUMachineError(Exception):
+    """
+    Exception called when an error in QEMUMachine happens.
+    """
+
+
 class QEMUMachine(object):
     '''A QEMU VM'''
 
@@ -62,18 +72,21 @@ class QEMUMachine(object):
         # In iotest.py, the qmp should always use unix socket.
         assert self._qmp.is_scm_available()
         if self._socket_scm_helper is None:
-            print >>sys.stderr, "No path to socket_scm_helper set"
-            return -1
+            raise QEMUMachineError("No path to socket_scm_helper set")
         if os.path.exists(self._socket_scm_helper) == False:
-            print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
-            return -1
+            raise QEMUMachineError("%s does not exist" %
+                                   self._socket_scm_helper)
         fd_param = ["%s" % self._socket_scm_helper,
                     "%d" % self._qmp.get_sock_fd(),
                     "%s" % fd_file_path]
         devnull = open('/dev/null', 'rb')
-        p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
-                             stderr=sys.stderr)
-        return p.wait()
+        p = subprocess.Popen(fd_param, stdin=devnull, stdout=subprocess.PIPE,
+                             stderr=subprocess.STDOUT)
+        output = p.communicate()[0]
+        if output:
+            LOG.debug(output)
+
+        return p.returncode
 
     @staticmethod
     def _remove_if_exists(path):
@@ -154,7 +167,8 @@ class QEMUMachine(object):
 
             exitcode = self._popen.wait()
             if exitcode < 0:
-                sys.stderr.write('qemu received signal %i: %s\n' % (-exitcode, ' '.join(self._args)))
+                LOG.warn('qemu received signal %i: %s', -exitcode,
+                          ' '.join(self._args))
             self._load_io_log()
             self._post_shutdown()
 
-- 
2.13.5

[Qemu-devel] [PATCH v8 03/13] qemu.py: use os.path.null instead of /dev/null

[Amador Pahim]

For increased portability, let's use os.path.devnull.

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 aca6fa4d82..a6e06291ea 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -79,7 +79,7 @@ class QEMUMachine(object):
         fd_param = ["%s" % self._socket_scm_helper,
                     "%d" % self._qmp.get_sock_fd(),
                     "%s" % fd_file_path]
-        devnull = open('/dev/null', 'rb')
+        devnull = open(os.path.devnull, 'rb')
         p = subprocess.Popen(fd_param, stdin=devnull, stdout=subprocess.PIPE,
                              stderr=subprocess.STDOUT)
         output = p.communicate()[0]
@@ -140,7 +140,7 @@ class QEMUMachine(object):
 
     def launch(self):
         '''Launch the VM and establish a QMP connection'''
-        devnull = open('/dev/null', 'rb')
+        devnull = open(os.path.devnull, 'rb')
         qemulog = open(self._qemu_log_path, 'wb')
         try:
             self._pre_launch()
-- 
2.13.5

[Qemu-devel] [PATCH v8 04/13] qemu.py: improve message on negative exit code

[Amador Pahim]

The current message shows 'self._args', which contains only part of the
options used in the Qemu command line.

This patch makes the qemu full args list an instance variable and then
uses it in the negative exit code message.

Message was moved outside the 'if is_running' block to make sure it will
be logged if the VM finishes before the call to shutdown().

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

diff --git a/scripts/qemu.py b/scripts/qemu.py
index a6e06291ea..670c048569 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -49,6 +49,7 @@ class QEMUMachine(object):
         self._iolog = None
         self._socket_scm_helper = socket_scm_helper
         self._debug = debug
+        self._qemu_full_args = None
 
     # This can be used to add an unused monitor instance.
     def add_monitor_telnet(self, ip, port):
@@ -140,13 +141,18 @@ class QEMUMachine(object):
 
     def launch(self):
         '''Launch the VM and establish a QMP connection'''
+        self._qemu_full_args = None
         devnull = open(os.path.devnull, 'rb')
         qemulog = open(self._qemu_log_path, 'wb')
         try:
             self._pre_launch()
-            args = self._wrapper + [self._binary] + self._base_args() + self._args
-            self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
-                                           stderr=subprocess.STDOUT, shell=False)
+            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,
+                                           stderr=subprocess.STDOUT,
+                                           shell=False)
             self._post_launch()
         except:
             if self.is_running():
@@ -164,14 +170,20 @@ class QEMUMachine(object):
                 self._qmp.close()
             except:
                 self._popen.kill()
+            self._popen.wait()
 
-            exitcode = self._popen.wait()
-            if exitcode < 0:
-                LOG.warn('qemu received signal %i: %s', -exitcode,
-                          ' '.join(self._args))
             self._load_io_log()
             self._post_shutdown()
 
+        exitcode = self.exitcode()
+        if exitcode is not None and exitcode < 0:
+            msg = 'qemu received signal %i: %s'
+            if self._qemu_full_args:
+                command = ' '.join(self._qemu_full_args)
+            else:
+                command = ''
+            LOG.warn(msg, exitcode, command)
+
     underscore_to_dash = string.maketrans('_', '-')
     def qmp(self, cmd, conv_keys=True, **args):
         '''Invoke a QMP command and return the result dict'''
-- 
2.13.5

[Qemu-devel] [PATCH v8 05/13] qemu.py: include debug information on launch error

[Amador Pahim]

When launching a VM, if an exception happens and the VM is not
initiated, it might be useful to see the qemu command line and
the qemu command output.

This patch creates that message. Notice that self._iolog needs to be
cleaned up in the beginning of the launch() to make sure we will not
expose the qemu log from a previous launch if the current one fails.

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 670c048569..3ebe5ee0a4 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -141,6 +141,7 @@ class QEMUMachine(object):
 
     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')
@@ -160,6 +161,12 @@ class QEMUMachine(object):
                 self._popen.wait()
             self._load_io_log()
             self._post_shutdown()
+
+            LOG.debug('Error launching VM')
+            if self._qemu_full_args:
+                LOG.debug('Command: %r', ' '.join(self._qemu_full_args))
+            if self._iolog:
+                LOG.debug('Output: %r', self._iolog)
             raise
 
     def shutdown(self):
-- 
2.13.5

[Qemu-devel] [PATCH v8 06/13] qemu.py: make sure we only remove files we create

[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 make sure we only
create files that are not pre-existent and to populate a list of
controlled files. This list will then be used as the reference of
files to remove during the cleanup (post_shutdown()).

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

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 3ebe5ee0a4..c26e1412f9 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -41,6 +41,7 @@ class QEMUMachine(object):
             monitor_address = os.path.join(test_dir, name + "-monitor.sock")
         self._monitor_address = monitor_address
         self._qemu_log_path = os.path.join(test_dir, name + ".log")
+        self._qemu_log_fd = None
         self._popen = None
         self._binary = binary
         self._args = list(args) # Force copy args in case we modify them
@@ -50,6 +51,7 @@ class QEMUMachine(object):
         self._socket_scm_helper = socket_scm_helper
         self._debug = debug
         self._qemu_full_args = None
+        self._created_files = []
 
     # This can be used to add an unused monitor instance.
     def add_monitor_telnet(self, ip, port):
@@ -128,30 +130,44 @@ class QEMUMachine(object):
                 '-display', 'none', '-vga', 'none']
 
     def _pre_launch(self):
-        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True,
-                                                debug=self._debug)
+        try:
+            self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
+                                                    server=True,
+                                                    debug=self._debug)
+        except:
+            raise
+        else:
+            if not isinstance(self._monitor_address, tuple):
+                self._created_files.append(self._monitor_address)
+
+        try:
+            flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY
+            os.open(self._qemu_log_path, flags)
+        except:
+            raise
+        else:
+            self._created_files.append(self._qemu_log_path)
+            self._qemu_log_fd = open(self._qemu_log_path, 'wb')
 
     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)
+        while self._created_files:
+            self._remove_if_exists(self._created_files.pop())
 
     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_fd,
                                            stderr=subprocess.STDOUT,
                                            shell=False)
             self._post_launch()
-- 
2.13.5

[Qemu-devel] [PATCH v8 07/13] qemu.py: close _qemu_log_path on cleanup

[Amador Pahim]

We are opening the _qemu_log_path during the launch() but we are
forgetting to close it.

This patch makes sure we will close the self._qemu_log_path during the
cleanup (post_shutdown()).

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 c26e1412f9..1b77fec48b 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -153,6 +153,9 @@ class QEMUMachine(object):
         self._qmp.accept()
 
     def _post_shutdown(self):
+        if self._qemu_log_fd is not None:
+            self._qemu_log_fd.close()
+
         while self._created_files:
             self._remove_if_exists(self._created_files.pop())
 
-- 
2.13.5

[Qemu-devel] [PATCH v8 08/13] 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.

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 1b77fec48b..06f07bfaa2 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -160,20 +160,14 @@ class QEMUMachine(object):
             self._remove_if_exists(self._created_files.pop())
 
     def launch(self):
-        '''Launch the VM and establish a QMP connection'''
+        '''
+        Try to 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_fd,
-                                           stderr=subprocess.STDOUT,
-                                           shell=False)
-            self._post_launch()
+            self._launch()
         except:
             if self.is_running():
                 self._popen.kill()
@@ -188,6 +182,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_fd,
+                                       stderr=subprocess.STDOUT,
+                                       shell=False)
+        self._post_launch()
+
     def shutdown(self):
         '''Terminate the VM and clean up'''
         if self.is_running():
-- 
2.13.5

[Qemu-devel] [PATCH v8 09/13] 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 cleanup to
always happen in shutdown, regardless the VM running state.

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 06f07bfaa2..ede734328b 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -205,8 +205,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.5

[Qemu-devel] [PATCH v8 10/13] 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.

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 ede734328b..87a2212b77 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -102,12 +102,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.5

[Qemu-devel] [PATCH v8 11/13] 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().

Due to this change, shutdown() can be now called even before the
creation of the _qemu_log_path. So, to avoid errors with _load_io_log(),
this patch makes sure we will only read the _qemu_log_path if it was
previously created by us.

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

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 87a2212b77..03e4cc34b7 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -115,8 +115,9 @@ class QEMUMachine(object):
         return self._popen.pid
 
     def _load_io_log(self):
-        with open(self._qemu_log_path, "r") as fh:
-            self._iolog = fh.read()
+        if self._qemu_log_path in self._created_files:
+            with open(self._qemu_log_path, "r") as fh:
+                self._iolog = fh.read()
 
     def _base_args(self):
         if isinstance(self._monitor_address, tuple):
@@ -169,11 +170,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.5

[Qemu-devel] [PATCH v8 12/13] qemu.py: launch vm only if it's not running

[Amador Pahim]

A new call to launch() with a running VM will fall in exception and
consequently call shutdown().

This patch makes launch() to raise an exception when it's called with VM
already running.

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 03e4cc34b7..363f649a7e 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -165,6 +165,9 @@ class QEMUMachine(object):
         Try to 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.5

[Qemu-devel] [PATCH v8 13/13] 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 363f649a7e..05fca4d268 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -52,6 +52,7 @@ class QEMUMachine(object):
         self._debug = debug
         self._qemu_full_args = None
         self._created_files = []
+        self._pending_shutdown = False
 
     # This can be used to add an unused monitor instance.
     def add_monitor_telnet(self, ip, port):
@@ -168,10 +169,14 @@ class QEMUMachine(object):
         if self.is_running():
             raise QEMUMachineError('VM already running')
 
+        if self._pending_shutdown:
+            raise QEMUMachineError('Shutdown pending after previous launch')
+
         self._iolog = None
         self._qemu_full_args = None
         try:
             self._launch()
+            self._pending_shutdown = True
         except:
             self.shutdown()
 
@@ -217,6 +222,8 @@ class QEMUMachine(object):
                 command = ''
             LOG.warn(msg, exitcode, command)
 
+        self._pending_shutdown = False
+
     underscore_to_dash = string.maketrans('_', '-')
     def qmp(self, cmd, conv_keys=True, **args):
         '''Invoke a QMP command and return the result dict'''
-- 
2.13.5

Re: [Qemu-devel] [PATCH v8 03/13] qemu.py: use os.path.null instead of /dev/null

[Philippe Mathieu-Daudé]

On 09/01/2017 08:28 AM, Amador Pahim wrote:
> For increased portability, let's use os.path.devnull.
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   scripts/qemu.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index aca6fa4d82..a6e06291ea 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -79,7 +79,7 @@ class QEMUMachine(object):
>           fd_param = ["%s" % self._socket_scm_helper,
>                       "%d" % self._qmp.get_sock_fd(),
>                       "%s" % fd_file_path]
> -        devnull = open('/dev/null', 'rb')
> +        devnull = open(os.path.devnull, 'rb')
>           p = subprocess.Popen(fd_param, stdin=devnull, stdout=subprocess.PIPE,
>                                stderr=subprocess.STDOUT)
>           output = p.communicate()[0]
> @@ -140,7 +140,7 @@ class QEMUMachine(object):
>   
>       def launch(self):
>           '''Launch the VM and establish a QMP connection'''
> -        devnull = open('/dev/null', 'rb')
> +        devnull = open(os.path.devnull, 'rb')
>           qemulog = open(self._qemu_log_path, 'wb')
>           try:
>               self._pre_launch()
> 

Re: [Qemu-devel] [PATCH v8 01/13] qemu.py: fix is_running() return before first launch()

[Fam Zheng]

On Fri, 09/01 13:28, Amador Pahim wrote:
> is_running() returns None when called before the first time we
> call launch():
> 
>     >>> import qemu
>     >>> vm = qemu.QEMUMachine('qemu-system-x86_64')
>     >>> vm.is_running()
>     >>>
> 
> It should return False instead. This patch fixes that.
> 
> For consistence, this patch removes the parenthesis from the
> second clause as it's not really needed.
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v8 02/13] qemu.py: avoid writing to stdout/stderr

[Fam Zheng]

On Fri, 09/01 13:28, Amador Pahim wrote:
> This module should not write directly to stdout/stderr. Instead, it
> should either raise exceptions or just log the messages and let the
> callers handle them and decide what to do. For example, scripts could
> choose to send the log messages stderr or/and write them to a file if
> verbose or debugging mode is enabled.
> 
> This patch replaces the writes to stderr by an exception in the
> send_fd_scm() when _socket_scm_helper is not set or not present. In the
> same method, the subprocess Popen will now redirect the stdout/stderr to
> logging.debug instead of writing to system stderr. As consequence, since
> the Popen.communicate() is now used (in order to get the stdout), the
> further call to wait() became redundant and was replaced by
> Popen.returncode.
> 
> The shutdown() message on negative exit code will now be logged
> to logging.warn instead of written to system stderr.
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v8 03/13] qemu.py: use os.path.null instead of /dev/null

[Fam Zheng]

On Fri, 09/01 13:28, Amador Pahim wrote:
> For increased portability, let's use os.path.devnull.
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v8 04/13] qemu.py: improve message on negative exit code

[Fam Zheng]

On Fri, 09/01 13:28, Amador Pahim wrote:
> The current message shows 'self._args', which contains only part of the
> options used in the Qemu command line.
> 
> This patch makes the qemu full args list an instance variable and then
> uses it in the negative exit code message.
> 
> Message was moved outside the 'if is_running' block to make sure it will
> be logged if the VM finishes before the call to shutdown().
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>
> ---
>  scripts/qemu.py | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index a6e06291ea..670c048569 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -49,6 +49,7 @@ class QEMUMachine(object):
>          self._iolog = None
>          self._socket_scm_helper = socket_scm_helper
>          self._debug = debug
> +        self._qemu_full_args = None
>  
>      # This can be used to add an unused monitor instance.
>      def add_monitor_telnet(self, ip, port):
> @@ -140,13 +141,18 @@ class QEMUMachine(object):
>  
>      def launch(self):
>          '''Launch the VM and establish a QMP connection'''
> +        self._qemu_full_args = None
>          devnull = open(os.path.devnull, 'rb')
>          qemulog = open(self._qemu_log_path, 'wb')
>          try:
>              self._pre_launch()
> -            args = self._wrapper + [self._binary] + self._base_args() + self._args
> -            self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
> -                                           stderr=subprocess.STDOUT, shell=False)
> +            self._qemu_full_args = (self._wrapper + [self._binary] +
> +                                    self._base_args() + self._args)

The parentheses seem superfluous. With those removed:

Reviewed-by: Fam Zheng <famz@redhat.com>

> +            self._popen = subprocess.Popen(self._qemu_full_args,
> +                                           stdin=devnull,
> +                                           stdout=qemulog,
> +                                           stderr=subprocess.STDOUT,
> +                                           shell=False)
>              self._post_launch()
>          except:
>              if self.is_running():
> @@ -164,14 +170,20 @@ class QEMUMachine(object):
>                  self._qmp.close()
>              except:
>                  self._popen.kill()
> +            self._popen.wait()
>  
> -            exitcode = self._popen.wait()
> -            if exitcode < 0:
> -                LOG.warn('qemu received signal %i: %s', -exitcode,
> -                          ' '.join(self._args))
>              self._load_io_log()
>              self._post_shutdown()
>  
> +        exitcode = self.exitcode()
> +        if exitcode is not None and exitcode < 0:
> +            msg = 'qemu received signal %i: %s'
> +            if self._qemu_full_args:
> +                command = ' '.join(self._qemu_full_args)
> +            else:
> +                command = ''
> +            LOG.warn(msg, exitcode, command)
> +
>      underscore_to_dash = string.maketrans('_', '-')
>      def qmp(self, cmd, conv_keys=True, **args):
>          '''Invoke a QMP command and return the result dict'''
> -- 
> 2.13.5
> 
> 

Re: [Qemu-devel] [PATCH v8 05/13] qemu.py: include debug information on launch error

[Fam Zheng]

On Fri, 09/01 13:28, Amador Pahim wrote:
> When launching a VM, if an exception happens and the VM is not
> initiated, it might be useful to see the qemu command line and
> the qemu command output.
> 
> This patch creates that message. Notice that self._iolog needs to be
> cleaned up in the beginning of the launch() to make sure we will not
> expose the qemu log from a previous launch if the current one fails.
> 
> 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 670c048569..3ebe5ee0a4 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -141,6 +141,7 @@ class QEMUMachine(object):
>  
>      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')
> @@ -160,6 +161,12 @@ class QEMUMachine(object):
>                  self._popen.wait()
>              self._load_io_log()
>              self._post_shutdown()
> +
> +            LOG.debug('Error launching VM')
> +            if self._qemu_full_args:
> +                LOG.debug('Command: %r', ' '.join(self._qemu_full_args))
> +            if self._iolog:
> +                LOG.debug('Output: %r', self._iolog)
>              raise
>  
>      def shutdown(self):
> -- 
> 2.13.5
> 

It would be nice if these messages are added to the exception obj, but it's
already an improvement over what we have now, so:

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v8 07/13] qemu.py: close _qemu_log_path on cleanup

[Fam Zheng]

On Fri, 09/01 13:28, Amador Pahim wrote:
> We are opening the _qemu_log_path during the launch() but we are
> forgetting to close it.
> 
> This patch makes sure we will close the self._qemu_log_path during the
> cleanup (post_shutdown()).
> 
> 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 c26e1412f9..1b77fec48b 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -153,6 +153,9 @@ class QEMUMachine(object):
>          self._qmp.accept()
>  
>      def _post_shutdown(self):
> +        if self._qemu_log_fd is not None:
> +            self._qemu_log_fd.close()
> +
>          while self._created_files:
>              self._remove_if_exists(self._created_files.pop())
>  
> -- 
> 2.13.5
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v8 08/13] qemu.py: refactor launch()

[Fam Zheng]

On Fri, 09/01 13:28, 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.
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v8 09/13] qemu.py: always cleanup on shutdown()

[Fam Zheng]

On Fri, 09/01 13:28, 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 cleanup to
> always happen in shutdown, regardless the VM running state.
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v8 10/13] qemu.py: use poll() instead of 'returncode'

[Fam Zheng]

On Fri, 09/01 13:28, 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.
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v8 06/13] qemu.py: make sure we only remove files we create

[Fam Zheng]

On Fri, 09/01 13:28, 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 make sure we only
> create files that are not pre-existent and to populate a list of
> controlled files. This list will then be used as the reference of
> files to remove during the cleanup (post_shutdown()).
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>
> ---
>  scripts/qemu.py | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 3ebe5ee0a4..c26e1412f9 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -41,6 +41,7 @@ class QEMUMachine(object):
>              monitor_address = os.path.join(test_dir, name + "-monitor.sock")
>          self._monitor_address = monitor_address
>          self._qemu_log_path = os.path.join(test_dir, name + ".log")
> +        self._qemu_log_fd = None
>          self._popen = None
>          self._binary = binary
>          self._args = list(args) # Force copy args in case we modify them
> @@ -50,6 +51,7 @@ class QEMUMachine(object):
>          self._socket_scm_helper = socket_scm_helper
>          self._debug = debug
>          self._qemu_full_args = None
> +        self._created_files = []
>  
>      # This can be used to add an unused monitor instance.
>      def add_monitor_telnet(self, ip, port):
> @@ -128,30 +130,44 @@ class QEMUMachine(object):
>                  '-display', 'none', '-vga', 'none']
>  
>      def _pre_launch(self):
> -        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True,
> -                                                debug=self._debug)
> +        try:
> +            self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
> +                                                    server=True,
> +                                                    debug=self._debug)
> +        except:
> +            raise

What's the point of "except: raise"? It seems useless.

> +        else:
> +            if not isinstance(self._monitor_address, tuple):
> +                self._created_files.append(self._monitor_address)
> +
> +        try:
> +            flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY
> +            os.open(self._qemu_log_path, flags)

Why change to os.open() instead of open()?

> +        except:
> +            raise
> +        else:
> +            self._created_files.append(self._qemu_log_path)
> +            self._qemu_log_fd = open(self._qemu_log_path, 'wb')
>  
>      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)
> +        while self._created_files:
> +            self._remove_if_exists(self._created_files.pop())
>  
>      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_fd,
>                                             stderr=subprocess.STDOUT,
>                                             shell=False)
>              self._post_launch()
> -- 
> 2.13.5
> 

Fam

Re: [Qemu-devel] [PATCH v8 12/13] qemu.py: launch vm only if it's not running

[Fam Zheng]

On Fri, 09/01 13:28, Amador Pahim wrote:
> A new call to launch() with a running VM will fall in exception and
> consequently call shutdown().
> 
> This patch makes launch() to raise an exception when it's called with VM
> already running.
> 
> 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 03e4cc34b7..363f649a7e 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -165,6 +165,9 @@ class QEMUMachine(object):
>          Try to 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.5
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v8 11/13] qemu.py: cleanup redundant calls in launch()

[Fam Zheng]

On Fri, 09/01 13:28, 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().
> 
> Due to this change, shutdown() can be now called even before the
> creation of the _qemu_log_path. So, to avoid errors with _load_io_log(),
> this patch makes sure we will only read the _qemu_log_path if it was
> previously created by us.
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>
> ---
>  scripts/qemu.py | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 87a2212b77..03e4cc34b7 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -115,8 +115,9 @@ class QEMUMachine(object):
>          return self._popen.pid
>  
>      def _load_io_log(self):
> -        with open(self._qemu_log_path, "r") as fh:
> -            self._iolog = fh.read()
> +        if self._qemu_log_path in self._created_files:
> +            with open(self._qemu_log_path, "r") as fh:
> +                self._iolog = fh.read()
>  
>      def _base_args(self):
>          if isinstance(self._monitor_address, tuple):
> @@ -169,11 +170,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.5
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

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

[Fam Zheng]

On Fri, 09/01 13:28, 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 363f649a7e..05fca4d268 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -52,6 +52,7 @@ class QEMUMachine(object):
>          self._debug = debug
>          self._qemu_full_args = None
>          self._created_files = []
> +        self._pending_shutdown = False

The name is very poor, it can be confused as "user requested a shutdown but the
action is pending for some reason". Maybe "self._launched"?

Fam

>  
>      # This can be used to add an unused monitor instance.
>      def add_monitor_telnet(self, ip, port):
> @@ -168,10 +169,14 @@ class QEMUMachine(object):
>          if self.is_running():
>              raise QEMUMachineError('VM already running')
>  
> +        if self._pending_shutdown:
> +            raise QEMUMachineError('Shutdown pending after previous launch')
> +
>          self._iolog = None
>          self._qemu_full_args = None
>          try:
>              self._launch()
> +            self._pending_shutdown = True
>          except:
>              self.shutdown()
>  
> @@ -217,6 +222,8 @@ class QEMUMachine(object):
>                  command = ''
>              LOG.warn(msg, exitcode, command)
>  
> +        self._pending_shutdown = False
> +
>      underscore_to_dash = string.maketrans('_', '-')
>      def qmp(self, cmd, conv_keys=True, **args):
>          '''Invoke a QMP command and return the result dict'''
> -- 
> 2.13.5
> 
> 

Re: [Qemu-devel] [PATCH v8 00/13] scripts/qemu.py fixes and cleanups

[Stefan Hajnoczi]

On Fri, Sep 01, 2017 at 01:28:16PM +0200, Amador Pahim wrote:

Please update the patches with Reviewed-bys from past revisions so I
know which patches I've already reviewed.

If someone has posted a R-b then it should be included in the next
revision unless you substantially modify that patch.

Thanks,
Stefan

Re: [Qemu-devel] [PATCH v8 00/13] scripts/qemu.py fixes and cleanups

[Eduardo Habkost]

Patches 01-05 were queued on my python-next branch:
https://github.com/ehabkost/qemu/commits/python-next

Please use python-next as base for v9, as I plan to submit a pull
request with the contents of python-next soon.


On Fri, Sep 01, 2017 at 01:28:16PM +0200, Amador Pahim wrote:
> Changes v1->v2:
>  - Style fixes to make checkpatch.pl happy.
>  - Rebased.
> Changes v2->v3:
>  - Fix typo in patch 3 ("qemu.py: make 'args' public") commit message.
> 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 v4->v5:
>  - Break the cleanup commit into logical changes and include in the
>    commit messages the rationale for making them.
> 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 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 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.
> 
> Amador Pahim (13):
>   qemu.py: fix is_running() return before first launch()
>   qemu.py: avoid writing to stdout/stderr
>   qemu.py: use os.path.null instead of /dev/null
>   qemu.py: improve message on negative exit code
>   qemu.py: include debug information on launch error
>   qemu.py: make sure we only remove files we create
>   qemu.py: close _qemu_log_path on cleanup
>   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 | 136 +++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 101 insertions(+), 35 deletions(-)
> 
> -- 
> 2.13.5
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v8 06/13] qemu.py: make sure we only remove files we create

[Amador Pahim]

On Tue, Sep 5, 2017 at 5:18 AM, Fam Zheng <famz@redhat.com> wrote:
> On Fri, 09/01 13:28, 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 make sure we only
>> create files that are not pre-existent and to populate a list of
>> controlled files. This list will then be used as the reference of
>> files to remove during the cleanup (post_shutdown()).
>>
>> Signed-off-by: Amador Pahim <apahim@redhat.com>
>> ---
>>  scripts/qemu.py | 30 +++++++++++++++++++++++-------
>>  1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index 3ebe5ee0a4..c26e1412f9 100644
>> --- a/scripts/qemu.py
>> +++ b/scripts/qemu.py
>> @@ -41,6 +41,7 @@ class QEMUMachine(object):
>>              monitor_address = os.path.join(test_dir, name + "-monitor.sock")
>>          self._monitor_address = monitor_address
>>          self._qemu_log_path = os.path.join(test_dir, name + ".log")
>> +        self._qemu_log_fd = None
>>          self._popen = None
>>          self._binary = binary
>>          self._args = list(args) # Force copy args in case we modify them
>> @@ -50,6 +51,7 @@ class QEMUMachine(object):
>>          self._socket_scm_helper = socket_scm_helper
>>          self._debug = debug
>>          self._qemu_full_args = None
>> +        self._created_files = []
>>
>>      # This can be used to add an unused monitor instance.
>>      def add_monitor_telnet(self, ip, port):
>> @@ -128,30 +130,44 @@ class QEMUMachine(object):
>>                  '-display', 'none', '-vga', 'none']
>>
>>      def _pre_launch(self):
>> -        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True,
>> -                                                debug=self._debug)
>> +        try:
>> +            self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
>> +                                                    server=True,
>> +                                                    debug=self._debug)
>> +        except:
>> +            raise
>
> What's the point of "except: raise"? It seems useless.

The point is to execute the block in the else only when no exception
happens. When some exception happens, I want to raise it without
executing the else block.

>
>> +        else:
>> +            if not isinstance(self._monitor_address, tuple):
>> +                self._created_files.append(self._monitor_address)
>> +
>> +        try:
>> +            flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY
>> +            os.open(self._qemu_log_path, flags)
>
> Why change to os.open() instead of open()?

I want to create the file only if it does not exist. The open() flag
'x' is available only in python 3.3. For python <3.3, we need the
os.open() to have that feature.

>
>> +        except:
>> +            raise
>> +        else:
>> +            self._created_files.append(self._qemu_log_path)
>> +            self._qemu_log_fd = open(self._qemu_log_path, 'wb')
>>
>>      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)
>> +        while self._created_files:
>> +            self._remove_if_exists(self._created_files.pop())
>>
>>      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_fd,
>>                                             stderr=subprocess.STDOUT,
>>                                             shell=False)
>>              self._post_launch()
>> --
>> 2.13.5
>>
>
> Fam

Re: [Qemu-devel] [PATCH v8 06/13] qemu.py: make sure we only remove files we create

[Eduardo Habkost]

On Thu, Sep 14, 2017 at 09:38:13PM +0200, Amador Pahim wrote:
> On Tue, Sep 5, 2017 at 5:18 AM, Fam Zheng <famz@redhat.com> wrote:
> > On Fri, 09/01 13:28, 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 make sure we only
> >> create files that are not pre-existent and to populate a list of
> >> controlled files. This list will then be used as the reference of
> >> files to remove during the cleanup (post_shutdown()).
> >>
> >> Signed-off-by: Amador Pahim <apahim@redhat.com>
> >> ---
> >>  scripts/qemu.py | 30 +++++++++++++++++++++++-------
> >>  1 file changed, 23 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/scripts/qemu.py b/scripts/qemu.py
> >> index 3ebe5ee0a4..c26e1412f9 100644
> >> --- a/scripts/qemu.py
> >> +++ b/scripts/qemu.py
> >> @@ -41,6 +41,7 @@ class QEMUMachine(object):
> >>              monitor_address = os.path.join(test_dir, name + "-monitor.sock")
> >>          self._monitor_address = monitor_address
> >>          self._qemu_log_path = os.path.join(test_dir, name + ".log")
> >> +        self._qemu_log_fd = None
> >>          self._popen = None
> >>          self._binary = binary
> >>          self._args = list(args) # Force copy args in case we modify them
> >> @@ -50,6 +51,7 @@ class QEMUMachine(object):
> >>          self._socket_scm_helper = socket_scm_helper
> >>          self._debug = debug
> >>          self._qemu_full_args = None
> >> +        self._created_files = []
> >>
> >>      # This can be used to add an unused monitor instance.
> >>      def add_monitor_telnet(self, ip, port):
> >> @@ -128,30 +130,44 @@ class QEMUMachine(object):
> >>                  '-display', 'none', '-vga', 'none']
> >>
> >>      def _pre_launch(self):
> >> -        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True,
> >> -                                                debug=self._debug)
> >> +        try:
> >> +            self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
> >> +                                                    server=True,
> >> +                                                    debug=self._debug)
> >> +        except:
> >> +            raise
> >
> > What's the point of "except: raise"? It seems useless.
> 
> The point is to execute the block in the else only when no exception
> happens. When some exception happens, I want to raise it without
> executing the else block.

Isn't this exactly what Python does when an exception is raised
with no "try" block?


> 
> >
> >> +        else:
> >> +            if not isinstance(self._monitor_address, tuple):
> >> +                self._created_files.append(self._monitor_address)
> >> +
> >> +        try:
> >> +            flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY
> >> +            os.open(self._qemu_log_path, flags)
> >
> > Why change to os.open() instead of open()?
> 
> I want to create the file only if it does not exist. The open() flag
> 'x' is available only in python 3.3. For python <3.3, we need the
> os.open() to have that feature.

I'm not sure this extra complexity is really necessary.  We could
fix all that by using mkdtemp() and deleting the temporary
directory on shutdown.

> 
> >
> >> +        except:
> >> +            raise
> >> +        else:
> >> +            self._created_files.append(self._qemu_log_path)
> >> +            self._qemu_log_fd = open(self._qemu_log_path, 'wb')
> >>
> >>      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)
> >> +        while self._created_files:
> >> +            self._remove_if_exists(self._created_files.pop())
> >>
> >>      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_fd,
> >>                                             stderr=subprocess.STDOUT,
> >>                                             shell=False)
> >>              self._post_launch()
> >> --
> >> 2.13.5
> >>
> >
> > Fam

-- 
Eduardo

Re: [Qemu-devel] [PATCH v8 06/13] qemu.py: make sure we only remove files we create

[Amador Pahim]

On Thu, Sep 14, 2017 at 9:46 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Sep 14, 2017 at 09:38:13PM +0200, Amador Pahim wrote:
>> On Tue, Sep 5, 2017 at 5:18 AM, Fam Zheng <famz@redhat.com> wrote:
>> > On Fri, 09/01 13:28, 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 make sure we only
>> >> create files that are not pre-existent and to populate a list of
>> >> controlled files. This list will then be used as the reference of
>> >> files to remove during the cleanup (post_shutdown()).
>> >>
>> >> Signed-off-by: Amador Pahim <apahim@redhat.com>
>> >> ---
>> >>  scripts/qemu.py | 30 +++++++++++++++++++++++-------
>> >>  1 file changed, 23 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> >> index 3ebe5ee0a4..c26e1412f9 100644
>> >> --- a/scripts/qemu.py
>> >> +++ b/scripts/qemu.py
>> >> @@ -41,6 +41,7 @@ class QEMUMachine(object):
>> >>              monitor_address = os.path.join(test_dir, name + "-monitor.sock")
>> >>          self._monitor_address = monitor_address
>> >>          self._qemu_log_path = os.path.join(test_dir, name + ".log")
>> >> +        self._qemu_log_fd = None
>> >>          self._popen = None
>> >>          self._binary = binary
>> >>          self._args = list(args) # Force copy args in case we modify them
>> >> @@ -50,6 +51,7 @@ class QEMUMachine(object):
>> >>          self._socket_scm_helper = socket_scm_helper
>> >>          self._debug = debug
>> >>          self._qemu_full_args = None
>> >> +        self._created_files = []
>> >>
>> >>      # This can be used to add an unused monitor instance.
>> >>      def add_monitor_telnet(self, ip, port):
>> >> @@ -128,30 +130,44 @@ class QEMUMachine(object):
>> >>                  '-display', 'none', '-vga', 'none']
>> >>
>> >>      def _pre_launch(self):
>> >> -        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True,
>> >> -                                                debug=self._debug)
>> >> +        try:
>> >> +            self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
>> >> +                                                    server=True,
>> >> +                                                    debug=self._debug)
>> >> +        except:
>> >> +            raise
>> >
>> > What's the point of "except: raise"? It seems useless.
>>
>> The point is to execute the block in the else only when no exception
>> happens. When some exception happens, I want to raise it without
>> executing the else block.
>
> Isn't this exactly what Python does when an exception is raised
> with no "try" block?

Sure, cleaning this up.

>
>
>>
>> >
>> >> +        else:
>> >> +            if not isinstance(self._monitor_address, tuple):
>> >> +                self._created_files.append(self._monitor_address)
>> >> +
>> >> +        try:
>> >> +            flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY
>> >> +            os.open(self._qemu_log_path, flags)
>> >
>> > Why change to os.open() instead of open()?
>>
>> I want to create the file only if it does not exist. The open() flag
>> 'x' is available only in python 3.3. For python <3.3, we need the
>> os.open() to have that feature.
>
> I'm not sure this extra complexity is really necessary.  We could
> fix all that by using mkdtemp() and deleting the temporary
> directory on shutdown.

I thought about that, but I foresee the question: hat happens if
between the mkdtemp and the file creation (i.e. self._qemu_log_path)
someone goes in that directory and creates a file with the same name
of the self._qemu_log_path? Are we going to overwrite it? Ok, very
unlikely, but possible. This extra step takes care of that.

>
>>
>> >
>> >> +        except:
>> >> +            raise
>> >> +        else:
>> >> +            self._created_files.append(self._qemu_log_path)
>> >> +            self._qemu_log_fd = open(self._qemu_log_path, 'wb')
>> >>
>> >>      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)
>> >> +        while self._created_files:
>> >> +            self._remove_if_exists(self._created_files.pop())
>> >>
>> >>      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_fd,
>> >>                                             stderr=subprocess.STDOUT,
>> >>                                             shell=False)
>> >>              self._post_launch()
>> >> --
>> >> 2.13.5
>> >>
>> >
>> > Fam
>
> --
> Eduardo

Re: [Qemu-devel] [PATCH v8 06/13] qemu.py: make sure we only remove files we create

[Eduardo Habkost]

On Thu, Sep 14, 2017 at 10:05:50PM +0200, Amador Pahim wrote:
> On Thu, Sep 14, 2017 at 9:46 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Thu, Sep 14, 2017 at 09:38:13PM +0200, Amador Pahim wrote:
> >> On Tue, Sep 5, 2017 at 5:18 AM, Fam Zheng <famz@redhat.com> wrote:
> >> > On Fri, 09/01 13:28, Amador Pahim wrote:
[...]
> >> >> +        else:
> >> >> +            if not isinstance(self._monitor_address, tuple):
> >> >> +                self._created_files.append(self._monitor_address)
> >> >> +
> >> >> +        try:
> >> >> +            flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY
> >> >> +            os.open(self._qemu_log_path, flags)
> >> >
> >> > Why change to os.open() instead of open()?
> >>
> >> I want to create the file only if it does not exist. The open() flag
> >> 'x' is available only in python 3.3. For python <3.3, we need the
> >> os.open() to have that feature.
> >
> > I'm not sure this extra complexity is really necessary.  We could
> > fix all that by using mkdtemp() and deleting the temporary
> > directory on shutdown.
> 
> I thought about that, but I foresee the question: hat happens if
> between the mkdtemp and the file creation (i.e. self._qemu_log_path)
> someone goes in that directory and creates a file with the same name
> of the self._qemu_log_path? Are we going to overwrite it? Ok, very
> unlikely, but possible. This extra step takes care of that.

If someone creates a file inside a directory we created using
mkdtemp(), we will just delete it.  Why would that be a problem?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v8 06/13] qemu.py: make sure we only remove files we create

[Amador Pahim]

On Thu, Sep 14, 2017 at 10:18 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Sep 14, 2017 at 10:05:50PM +0200, Amador Pahim wrote:
>> On Thu, Sep 14, 2017 at 9:46 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > On Thu, Sep 14, 2017 at 09:38:13PM +0200, Amador Pahim wrote:
>> >> On Tue, Sep 5, 2017 at 5:18 AM, Fam Zheng <famz@redhat.com> wrote:
>> >> > On Fri, 09/01 13:28, Amador Pahim wrote:
> [...]
>> >> >> +        else:
>> >> >> +            if not isinstance(self._monitor_address, tuple):
>> >> >> +                self._created_files.append(self._monitor_address)
>> >> >> +
>> >> >> +        try:
>> >> >> +            flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY
>> >> >> +            os.open(self._qemu_log_path, flags)
>> >> >
>> >> > Why change to os.open() instead of open()?
>> >>
>> >> I want to create the file only if it does not exist. The open() flag
>> >> 'x' is available only in python 3.3. For python <3.3, we need the
>> >> os.open() to have that feature.
>> >
>> > I'm not sure this extra complexity is really necessary.  We could
>> > fix all that by using mkdtemp() and deleting the temporary
>> > directory on shutdown.
>>
>> I thought about that, but I foresee the question: hat happens if
>> between the mkdtemp and the file creation (i.e. self._qemu_log_path)
>> someone goes in that directory and creates a file with the same name
>> of the self._qemu_log_path? Are we going to overwrite it? Ok, very
>> unlikely, but possible. This extra step takes care of that.
>
> If someone creates a file inside a directory we created using
> mkdtemp(), we will just delete it.  Why would that be a problem?

Ok then. That simplifies the control a lot.
Thanks.

>
> --
> Eduardo

Re: [Qemu-devel] [PATCH v8 04/13] qemu.py: improve message on negative exit code

[Kevin Wolf]

Am 05.09.2017 um 05:02 hat Fam Zheng geschrieben:
> On Fri, 09/01 13:28, Amador Pahim wrote:
> > The current message shows 'self._args', which contains only part of the
> > options used in the Qemu command line.
> > 
> > This patch makes the qemu full args list an instance variable and then
> > uses it in the negative exit code message.
> > 
> > Message was moved outside the 'if is_running' block to make sure it will
> > be logged if the VM finishes before the call to shutdown().
> > 
> > Signed-off-by: Amador Pahim <apahim@redhat.com>
> > ---
> >  scripts/qemu.py | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/scripts/qemu.py b/scripts/qemu.py
> > index a6e06291ea..670c048569 100644
> > --- a/scripts/qemu.py
> > +++ b/scripts/qemu.py
> > @@ -49,6 +49,7 @@ class QEMUMachine(object):
> >          self._iolog = None
> >          self._socket_scm_helper = socket_scm_helper
> >          self._debug = debug
> > +        self._qemu_full_args = None
> >  
> >      # This can be used to add an unused monitor instance.
> >      def add_monitor_telnet(self, ip, port):
> > @@ -140,13 +141,18 @@ class QEMUMachine(object):
> >  
> >      def launch(self):
> >          '''Launch the VM and establish a QMP connection'''
> > +        self._qemu_full_args = None
> >          devnull = open(os.path.devnull, 'rb')
> >          qemulog = open(self._qemu_log_path, 'wb')
> >          try:
> >              self._pre_launch()
> > -            args = self._wrapper + [self._binary] + self._base_args() + self._args
> > -            self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
> > -                                           stderr=subprocess.STDOUT, shell=False)
> > +            self._qemu_full_args = (self._wrapper + [self._binary] +
> > +                                    self._base_args() + self._args)
> 
> The parentheses seem superfluous. With those removed:
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>

Congratulations, with this advice you just killed all Python-based
qemu-iotests and filled up my inbox with CI failure messages. :-)

+Traceback (most recent call last):
+  File "194", line 22, in <module>
+    import iotests
+  File "/home/kwolf/source/qemu/tests/qemu-iotests/iotests.py", line 27, in <module>
+    import qtest
+  File "/home/kwolf/source/qemu/tests/qemu-iotests/../../scripts/qtest.py", line 16, in <module>
+    import qemu
+  File "/home/kwolf/source/qemu/tests/qemu-iotests/../../scripts/qemu.py", line 196
+    self._qemu_full_args = self._wrapper + [self._binary] +
+                                                          ^
+SyntaxError: invalid syntax

Kevin

Re: [Qemu-devel] [PATCH v8 04/13] qemu.py: improve message on negative exit code

[Fam Zheng]

On Mon, 09/18 07:18, Kevin Wolf wrote:
> Am 05.09.2017 um 05:02 hat Fam Zheng geschrieben:
> > On Fri, 09/01 13:28, Amador Pahim wrote:
> > > The current message shows 'self._args', which contains only part of the
> > > options used in the Qemu command line.
> > > 
> > > This patch makes the qemu full args list an instance variable and then
> > > uses it in the negative exit code message.
> > > 
> > > Message was moved outside the 'if is_running' block to make sure it will
> > > be logged if the VM finishes before the call to shutdown().
> > > 
> > > Signed-off-by: Amador Pahim <apahim@redhat.com>
> > > ---
> > >  scripts/qemu.py | 26 +++++++++++++++++++-------
> > >  1 file changed, 19 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/scripts/qemu.py b/scripts/qemu.py
> > > index a6e06291ea..670c048569 100644
> > > --- a/scripts/qemu.py
> > > +++ b/scripts/qemu.py
> > > @@ -49,6 +49,7 @@ class QEMUMachine(object):
> > >          self._iolog = None
> > >          self._socket_scm_helper = socket_scm_helper
> > >          self._debug = debug
> > > +        self._qemu_full_args = None
> > >  
> > >      # This can be used to add an unused monitor instance.
> > >      def add_monitor_telnet(self, ip, port):
> > > @@ -140,13 +141,18 @@ class QEMUMachine(object):
> > >  
> > >      def launch(self):
> > >          '''Launch the VM and establish a QMP connection'''
> > > +        self._qemu_full_args = None
> > >          devnull = open(os.path.devnull, 'rb')
> > >          qemulog = open(self._qemu_log_path, 'wb')
> > >          try:
> > >              self._pre_launch()
> > > -            args = self._wrapper + [self._binary] + self._base_args() + self._args
> > > -            self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
> > > -                                           stderr=subprocess.STDOUT, shell=False)
> > > +            self._qemu_full_args = (self._wrapper + [self._binary] +
> > > +                                    self._base_args() + self._args)
> > 
> > The parentheses seem superfluous. With those removed:
> > 
> > Reviewed-by: Fam Zheng <famz@redhat.com>
> 
> Congratulations, with this advice you just killed all Python-based
> qemu-iotests and filled up my inbox with CI failure messages. :-)

Oops. Why doesn't python understand such multi line expressions impllied by "+"
just like in "[,]"?

Iotests on patchew is not far away. (The docker test-block pull request was
bounced a few times, unfortunately, for unrelated reasons)

Fam

Re: [Qemu-devel] [PATCH v8 04/13] qemu.py: improve message on negative exit code

[Kevin Wolf]

Am 18.09.2017 um 07:39 hat Fam Zheng geschrieben:
> On Mon, 09/18 07:18, Kevin Wolf wrote:
> > Am 05.09.2017 um 05:02 hat Fam Zheng geschrieben:
> > > On Fri, 09/01 13:28, Amador Pahim wrote:
> > > > The current message shows 'self._args', which contains only part of the
> > > > options used in the Qemu command line.
> > > > 
> > > > This patch makes the qemu full args list an instance variable and then
> > > > uses it in the negative exit code message.
> > > > 
> > > > Message was moved outside the 'if is_running' block to make sure it will
> > > > be logged if the VM finishes before the call to shutdown().
> > > > 
> > > > Signed-off-by: Amador Pahim <apahim@redhat.com>
> > > > ---
> > > >  scripts/qemu.py | 26 +++++++++++++++++++-------
> > > >  1 file changed, 19 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/scripts/qemu.py b/scripts/qemu.py
> > > > index a6e06291ea..670c048569 100644
> > > > --- a/scripts/qemu.py
> > > > +++ b/scripts/qemu.py
> > > > @@ -49,6 +49,7 @@ class QEMUMachine(object):
> > > >          self._iolog = None
> > > >          self._socket_scm_helper = socket_scm_helper
> > > >          self._debug = debug
> > > > +        self._qemu_full_args = None
> > > >  
> > > >      # This can be used to add an unused monitor instance.
> > > >      def add_monitor_telnet(self, ip, port):
> > > > @@ -140,13 +141,18 @@ class QEMUMachine(object):
> > > >  
> > > >      def launch(self):
> > > >          '''Launch the VM and establish a QMP connection'''
> > > > +        self._qemu_full_args = None
> > > >          devnull = open(os.path.devnull, 'rb')
> > > >          qemulog = open(self._qemu_log_path, 'wb')
> > > >          try:
> > > >              self._pre_launch()
> > > > -            args = self._wrapper + [self._binary] + self._base_args() + self._args
> > > > -            self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
> > > > -                                           stderr=subprocess.STDOUT, shell=False)
> > > > +            self._qemu_full_args = (self._wrapper + [self._binary] +
> > > > +                                    self._base_args() + self._args)
> > > 
> > > The parentheses seem superfluous. With those removed:
> > > 
> > > Reviewed-by: Fam Zheng <famz@redhat.com>
> > 
> > Congratulations, with this advice you just killed all Python-based
> > qemu-iotests and filled up my inbox with CI failure messages. :-)
> 
> Oops. Why doesn't python understand such multi line expressions
> impllied by "+" just like in "[,]"?

To be honest, I didn't know that either until now. But apparently the
different kinds of brackets (including those around function arguments)
are the only things that allow multiline expressions in Python.

> Iotests on patchew is not far away. (The docker test-block pull
> request was bounced a few times, unfortunately, for unrelated reasons)

Nice!

Kevin

Re: [Qemu-devel] [PATCH v8 04/13] qemu.py: improve message on negative exit code

[Fam Zheng]

On Mon, 09/18 08:44, Kevin Wolf wrote:
> > > > > +            self._qemu_full_args = (self._wrapper + [self._binary] +
> > > > > +                                    self._base_args() + self._args)
> > > > 
> > > > The parentheses seem superfluous. With those removed:
> > > > 
> > > > Reviewed-by: Fam Zheng <famz@redhat.com>
> > > 
> > > Congratulations, with this advice you just killed all Python-based
> > > qemu-iotests and filled up my inbox with CI failure messages. :-)
> > 
> > Oops. Why doesn't python understand such multi line expressions
> > impllied by "+" just like in "[,]"?
> 
> To be honest, I didn't know that either until now. But apparently the
> different kinds of brackets (including those around function arguments)
> are the only things that allow multiline expressions in Python.

Yes, and now I read that PEP recommends using brackets for multilines compared
to backslashes. I was totally wrong with this advice.

Fam

Re: [Qemu-devel] [PATCH v8 02/13] qemu.py: avoid writing to stdout/stderr

[Kevin Wolf]

Am 01.09.2017 um 13:28 hat Amador Pahim geschrieben:
> This module should not write directly to stdout/stderr. Instead, it
> should either raise exceptions or just log the messages and let the
> callers handle them and decide what to do. For example, scripts could
> choose to send the log messages stderr or/and write them to a file if
> verbose or debugging mode is enabled.
> 
> This patch replaces the writes to stderr by an exception in the
> send_fd_scm() when _socket_scm_helper is not set or not present. In the
> same method, the subprocess Popen will now redirect the stdout/stderr to
> logging.debug instead of writing to system stderr. As consequence, since
> the Popen.communicate() is now used (in order to get the stdout), the
> further call to wait() became redundant and was replaced by
> Popen.returncode.
> 
> The shutdown() message on negative exit code will now be logged
> to logging.warn instead of written to system stderr.
> 
> Signed-off-by: Amador Pahim <apahim@redhat.com>
> ---
>  scripts/qemu.py | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 0ae5d39414..aca6fa4d82 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -13,6 +13,7 @@
>  #
>  
>  import errno
> +import logging
>  import string
>  import os
>  import sys
> @@ -20,6 +21,15 @@ import subprocess
>  import qmp.qmp
>  
>  
> +LOG = logging.getLogger(__name__)

This logger isn't properly initialised, so whenever it would actually be
used, the only thing that ends up in the qemu-iotests output is:

+No handlers could be found for logger "qemu"

The actual error message (that was previously logged to stderr) doesn't
make it to the output any more.

I find it sad how the recent test infrastructure "improvements" tend to
break the tests instead of making them easier to work with. Please be a
bit more careful when touching this code, and *test* your code before
you submit it. I expect from anyone that they test their code, but if we
don't even manage to do that for the tests themselves, I'm losing hope.

Kevin

Re: [Qemu-devel] [PATCH v8 02/13] qemu.py: avoid writing to stdout/stderr

[Eduardo Habkost]

On Thu, Sep 21, 2017 at 12:35:08PM +0200, Kevin Wolf wrote:
> Am 01.09.2017 um 13:28 hat Amador Pahim geschrieben:
> > This module should not write directly to stdout/stderr. Instead, it
> > should either raise exceptions or just log the messages and let the
> > callers handle them and decide what to do. For example, scripts could
> > choose to send the log messages stderr or/and write them to a file if
> > verbose or debugging mode is enabled.
> > 
> > This patch replaces the writes to stderr by an exception in the
> > send_fd_scm() when _socket_scm_helper is not set or not present. In the
> > same method, the subprocess Popen will now redirect the stdout/stderr to
> > logging.debug instead of writing to system stderr. As consequence, since
> > the Popen.communicate() is now used (in order to get the stdout), the
> > further call to wait() became redundant and was replaced by
> > Popen.returncode.
> > 
> > The shutdown() message on negative exit code will now be logged
> > to logging.warn instead of written to system stderr.
> > 
> > Signed-off-by: Amador Pahim <apahim@redhat.com>
> > ---
> >  scripts/qemu.py | 30 ++++++++++++++++++++++--------
> >  1 file changed, 22 insertions(+), 8 deletions(-)
> > 
> > diff --git a/scripts/qemu.py b/scripts/qemu.py
> > index 0ae5d39414..aca6fa4d82 100644
> > --- a/scripts/qemu.py
> > +++ b/scripts/qemu.py
> > @@ -13,6 +13,7 @@
> >  #
> >  
> >  import errno
> > +import logging
> >  import string
> >  import os
> >  import sys
> > @@ -20,6 +21,15 @@ import subprocess
> >  import qmp.qmp
> >  
> >  
> > +LOG = logging.getLogger(__name__)
> 
> This logger isn't properly initialised, so whenever it would actually be
> used, the only thing that ends up in the qemu-iotests output is:
> 
> +No handlers could be found for logger "qemu"
> 
> The actual error message (that was previously logged to stderr) doesn't
> make it to the output any more.
> 
> I find it sad how the recent test infrastructure "improvements" tend to
> break the tests instead of making them easier to work with. Please be a
> bit more careful when touching this code, and *test* your code before
> you submit it. I expect from anyone that they test their code, but if we
> don't even manage to do that for the tests themselves, I'm losing hope.

Is the test that was broken triggered as part of "make check"?
What can we do to ensure it is?

-- 
Eduardo