[Qemu-devel] [PULL 00/15] Python queue, 2017-09-15

[Qemu-devel] [PULL 00/15] Python queue, 2017-09-15

[Eduardo Habkost]

I have been queueing some patches for Python modules and scripts,
recently, and submitted a MAINTAINERS patch to include Cleber and
me as maintainers.

However, I don't want to delay this pull request to avoid
conflicts if people send additional patches for those scripts, so
I'm sending this pull request before the MAINTAINERS patch is
applied.

The following changes since commit 5faf2d376af3cb4eb92da44c2580e08d39832caa:

  Merge remote-tracking branch 'remotes/huth/tags/check-20170915' into staging (2017-09-15 20:29:44 +0100)

are available in the git repository at:

  git://github.com/ehabkost/qemu.git tags/python-next-pull-request

for you to fetch changes up to b92a0011b1220aff549ae82c6104014d25e339ef:

  qemu.py: include debug information on launch error (2017-09-15 20:12:00 -0300)

----------------------------------------------------------------
Python queue, 2017-09-15

----------------------------------------------------------------

Amador Pahim (5):
  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

Lukáš Doktor (10):
  qemu.py: Pylint/style fixes
  qemu|qtest: Avoid dangerous arguments
  qemu.py: Use iteritems rather than keys()
  qemu.py: Simplify QMP key-conversion
  qemu.py: Use custom exceptions rather than Exception
  qmp.py: Couple of pylint/style fixes
  qmp.py: Use object-based class for QEMUMonitorProtocol
  qmp.py: Avoid "has_key" usage
  qmp.py: Avoid overriding a builtin object
  qtest.py: Few pylint/style fixes

 scripts/qemu.py       | 145 +++++++++++++++++++++++++++++++++++++++-----------
 scripts/qmp/qmp.py    |  49 ++++++++++-------
 scripts/qtest.py      |  13 +++--
 scripts/qmp/qmp-shell |   4 +-
 4 files changed, 151 insertions(+), 60 deletions(-)

-- 
2.13.5

[Qemu-devel] [PULL 01/15] qemu.py: Pylint/style fixes

[Eduardo Habkost]

From: Lukáš Doktor <ldoktor@redhat.com>

No actual code changes, just several pylint/style fixes and docstring
clarifications.

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20170818142613.32394-2-ldoktor@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qemu.py | 73 +++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 55 insertions(+), 18 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 4d8ee10943..b45e691538 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -30,8 +30,22 @@ class QEMUMachine(object):
         # vm is guaranteed to be shut down here
     '''
 
-    def __init__(self, binary, args=[], wrapper=[], name=None, test_dir="/var/tmp",
-                 monitor_address=None, socket_scm_helper=None, debug=False):
+    def __init__(self, binary, args=[], wrapper=[], name=None,
+                 test_dir="/var/tmp", monitor_address=None,
+                 socket_scm_helper=None, debug=False):
+        '''
+        Initialize a QEMUMachine
+
+        @param binary: path to the qemu binary
+        @param args: list of extra arguments
+        @param wrapper: list of arguments used as prefix to qemu binary
+        @param name: prefix for socket and log file names (default: qemu-PID)
+        @param test_dir: where to create socket and log file
+        @param monitor_address: address for QMP monitor
+        @param socket_scm_helper: helper program, required for send_fd_scm()"
+        @param debug: enable debug mode
+        @note: Qemu process is not started until launch() is used.
+        '''
         if name is None:
             name = "qemu-%d" % os.getpid()
         if monitor_address is None:
@@ -40,12 +54,13 @@ class QEMUMachine(object):
         self._qemu_log_path = os.path.join(test_dir, name + ".log")
         self._popen = None
         self._binary = binary
-        self._args = list(args) # Force copy args in case we modify them
+        self._args = list(args)     # Force copy args in case we modify them
         self._wrapper = wrapper
         self._events = []
         self._iolog = None
         self._socket_scm_helper = socket_scm_helper
         self._debug = debug
+        self._qmp = None
 
     def __enter__(self):
         return self
@@ -78,16 +93,16 @@ class QEMUMachine(object):
         if self._socket_scm_helper is None:
             print >>sys.stderr, "No path to socket_scm_helper set"
             return -1
-        if os.path.exists(self._socket_scm_helper) == False:
+        if not os.path.exists(self._socket_scm_helper):
             print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
             return -1
         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()
+        proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
+                                stderr=sys.stderr)
+        return proc.wait()
 
     @staticmethod
     def _remove_if_exists(path):
@@ -113,8 +128,8 @@ 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()
+        with open(self._qemu_log_path, "r") as iolog:
+            self._iolog = iolog.read()
 
     def _base_args(self):
         if isinstance(self._monitor_address, tuple):
@@ -128,7 +143,8 @@ class QEMUMachine(object):
                 '-display', 'none', '-vga', 'none']
 
     def _pre_launch(self):
-        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True,
+        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
+                                                server=True,
                                                 debug=self._debug)
 
     def _post_launch(self):
@@ -145,9 +161,11 @@ class QEMUMachine(object):
         qemulog = open(self._qemu_log_path, 'wb')
         try:
             self._pre_launch()
-            args = self._wrapper + [self._binary] + self._base_args() + self._args
+            args = (self._wrapper + [self._binary] + self._base_args() +
+                    self._args)
             self._popen = subprocess.Popen(args, stdin=devnull, stdout=qemulog,
-                                           stderr=subprocess.STDOUT, shell=False)
+                                           stderr=subprocess.STDOUT,
+                                           shell=False)
             self._post_launch()
         except:
             if self.is_running():
@@ -168,23 +186,30 @@ class QEMUMachine(object):
 
             exitcode = self._popen.wait()
             if exitcode < 0:
-                sys.stderr.write('qemu received signal %i: %s\n' % (-exitcode, ' '.join(self._args)))
+                sys.stderr.write('qemu received signal %i: %s\n'
+                                 % (-exitcode, ' '.join(self._args)))
             self._load_io_log()
             self._post_shutdown()
 
     underscore_to_dash = string.maketrans('_', '-')
+
     def qmp(self, cmd, conv_keys=True, **args):
-        '''Invoke a QMP command and return the result dict'''
+        '''Invoke a QMP command and return the response dict'''
         qmp_args = dict()
-        for k in args.keys():
+        for key in args.keys():
             if conv_keys:
-                qmp_args[k.translate(self.underscore_to_dash)] = args[k]
+                qmp_args[key.translate(self.underscore_to_dash)] = args[key]
             else:
-                qmp_args[k] = args[k]
+                qmp_args[key] = args[key]
 
         return self._qmp.cmd(cmd, args=qmp_args)
 
     def command(self, cmd, conv_keys=True, **args):
+        '''
+        Invoke a QMP command.
+        On success return the response dict.
+        On failure raise an exception.
+        '''
         reply = self.qmp(cmd, conv_keys, **args)
         if reply is None:
             raise Exception("Monitor is closed")
@@ -207,7 +232,15 @@ class QEMUMachine(object):
         return events
 
     def event_wait(self, name, timeout=60.0, match=None):
-        # Test if 'match' is a recursive subset of 'event'
+        '''
+        Wait for specified timeout on named event in QMP; optionally filter
+        results by match.
+
+        The 'match' is checked to be a recursive subset of the 'event'; skips
+        branch processing on match's value None
+           {"foo": {"bar": 1}} matches {"foo": None}
+           {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
+        '''
         def event_match(event, match=None):
             if match is None:
                 return True
@@ -240,4 +273,8 @@ class QEMUMachine(object):
         return None
 
     def get_log(self):
+        '''
+        After self.shutdown or failed qemu execution, this returns the output
+        of the qemu process.
+        '''
         return self._iolog
-- 
2.13.5

[Qemu-devel] [PULL 02/15] qemu|qtest: Avoid dangerous arguments

[Eduardo Habkost]

From: Lukáš Doktor <ldoktor@redhat.com>

The list object is mutable in python and potentially might modify other
object's arguments when used as default argument. Reproducer:

    >>> vm1 = QEMUMachine("qemu")
    >>> vm2 = QEMUMachine("qemu")
    >>> vm1._wrapper.append("foo")
    >>> print vm2._wrapper
    ['foo']

In this case the `args` is actually copied so it would be safe to keep
it, but it's not a good practice to keep it. The same issue applies in
inherited qtest module.

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-Id: <20170818142613.32394-3-ldoktor@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qemu.py  | 6 +++++-
 scripts/qtest.py | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index b45e691538..afd98a290e 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -30,7 +30,7 @@ class QEMUMachine(object):
         # vm is guaranteed to be shut down here
     '''
 
-    def __init__(self, binary, args=[], wrapper=[], name=None,
+    def __init__(self, binary, args=None, wrapper=None, name=None,
                  test_dir="/var/tmp", monitor_address=None,
                  socket_scm_helper=None, debug=False):
         '''
@@ -46,6 +46,10 @@ class QEMUMachine(object):
         @param debug: enable debug mode
         @note: Qemu process is not started until launch() is used.
         '''
+        if args is None:
+            args = []
+        if wrapper is None:
+            wrapper = []
         if name is None:
             name = "qemu-%d" % os.getpid()
         if monitor_address is None:
diff --git a/scripts/qtest.py b/scripts/qtest.py
index d5aecb5f49..ab183c0635 100644
--- a/scripts/qtest.py
+++ b/scripts/qtest.py
@@ -79,7 +79,7 @@ class QEMUQtestProtocol(object):
 class QEMUQtestMachine(qemu.QEMUMachine):
     '''A QEMU VM'''
 
-    def __init__(self, binary, args=[], name=None, test_dir="/var/tmp",
+    def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
                  socket_scm_helper=None):
         if name is None:
             name = "qemu-%d" % os.getpid()
-- 
2.13.5

[Qemu-devel] [PULL 03/15] qemu.py: Use iteritems rather than keys()

[Eduardo Habkost]

From: Lukáš Doktor <ldoktor@redhat.com>

Let's avoid creating an in-memory list of keys and query for each value
and use `iteritems` which is an iterator of key-value pairs.

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20170818142613.32394-4-ldoktor@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qemu.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index afd98a290e..d8c169b31e 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -200,11 +200,11 @@ class QEMUMachine(object):
     def qmp(self, cmd, conv_keys=True, **args):
         '''Invoke a QMP command and return the response dict'''
         qmp_args = dict()
-        for key in args.keys():
+        for key, value in args.iteritems():
             if conv_keys:
-                qmp_args[key.translate(self.underscore_to_dash)] = args[key]
+                qmp_args[key.translate(self.underscore_to_dash)] = value
             else:
-                qmp_args[key] = args[key]
+                qmp_args[key] = value
 
         return self._qmp.cmd(cmd, args=qmp_args)
 
-- 
2.13.5

[Qemu-devel] [PULL 04/15] qemu.py: Simplify QMP key-conversion

[Eduardo Habkost]

From: Lukáš Doktor <ldoktor@redhat.com>

The QMP key conversion consist of '_'s to be replaced with '-'s, which
can easily be done by a single `str.replace` method which is faster and
does not require `string` module import.

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Message-Id: <20170818142613.32394-5-ldoktor@redhat.com>
Reviewed-by: Cleber Rosa <crosa@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qemu.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index d8c169b31e..bde8da8fe7 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -13,7 +13,6 @@
 #
 
 import errno
-import string
 import os
 import sys
 import subprocess
@@ -195,14 +194,12 @@ class QEMUMachine(object):
             self._load_io_log()
             self._post_shutdown()
 
-    underscore_to_dash = string.maketrans('_', '-')
-
     def qmp(self, cmd, conv_keys=True, **args):
         '''Invoke a QMP command and return the response dict'''
         qmp_args = dict()
         for key, value in args.iteritems():
             if conv_keys:
-                qmp_args[key.translate(self.underscore_to_dash)] = value
+                qmp_args[key.replace('_', '-')] = value
             else:
                 qmp_args[key] = value
 
-- 
2.13.5

[Qemu-devel] [PULL 05/15] qemu.py: Use custom exceptions rather than Exception

[Eduardo Habkost]

From: Lukáš Doktor <ldoktor@redhat.com>

The naked Exception should not be widely used. It makes sense to be a
bit more specific and use better-suited custom exceptions. As a benefit
we can store the full reply in the exception in case someone needs it
when catching the exception.

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Message-Id: <20170818142613.32394-6-ldoktor@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qemu.py | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index bde8da8fe7..7c6802609a 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -19,6 +19,19 @@ import subprocess
 import qmp.qmp
 
 
+class MonitorResponseError(qmp.qmp.QMPError):
+    '''
+    Represents erroneous QMP monitor reply
+    '''
+    def __init__(self, reply):
+        try:
+            desc = reply["error"]["desc"]
+        except KeyError:
+            desc = reply
+        super(MonitorResponseError, self).__init__(desc)
+        self.reply = reply
+
+
 class QEMUMachine(object):
     '''A QEMU VM
 
@@ -213,9 +226,9 @@ class QEMUMachine(object):
         '''
         reply = self.qmp(cmd, conv_keys, **args)
         if reply is None:
-            raise Exception("Monitor is closed")
+            raise qmp.qmp.QMPError("Monitor is closed")
         if "error" in reply:
-            raise Exception(reply["error"]["desc"])
+            raise MonitorResponseError(reply)
         return reply["return"]
 
     def get_qmp_event(self, wait=False):
-- 
2.13.5

[Qemu-devel] [PULL 06/15] qmp.py: Couple of pylint/style fixes

[Eduardo Habkost]

From: Lukáš Doktor <ldoktor@redhat.com>

No actual code changes, just initializing attributes earlier to avoid
AttributeError on early introspection, a few pylint/style fixes and
docstring clarifications.

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20170818142613.32394-7-ldoktor@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qmp/qmp.py | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 62d3651967..782d1ac5df 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -13,19 +13,30 @@ import errno
 import socket
 import sys
 
+
 class QMPError(Exception):
     pass
 
+
 class QMPConnectError(QMPError):
     pass
 
+
 class QMPCapabilitiesError(QMPError):
     pass
 
+
 class QMPTimeoutError(QMPError):
     pass
 
+
 class QEMUMonitorProtocol:
+
+    #: Socket's error class
+    error = socket.error
+    #: Socket's timeout
+    timeout = socket.timeout
+
     def __init__(self, address, server=False, debug=False):
         """
         Create a QEMUMonitorProtocol class.
@@ -42,6 +53,7 @@ class QEMUMonitorProtocol:
         self.__address = address
         self._debug = debug
         self.__sock = self.__get_sock()
+        self.__sockfile = None
         if server:
             self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
             self.__sock.bind(self.__address)
@@ -56,7 +68,7 @@ class QEMUMonitorProtocol:
 
     def __negotiate_capabilities(self):
         greeting = self.__json_read()
-        if greeting is None or not greeting.has_key('QMP'):
+        if greeting is None or "QMP" not in greeting:
             raise QMPConnectError
         # Greeting seems ok, negotiate capabilities
         resp = self.cmd('qmp_capabilities')
@@ -78,8 +90,6 @@ class QEMUMonitorProtocol:
                     continue
             return resp
 
-    error = socket.error
-
     def __get_events(self, wait=False):
         """
         Check for new events in the stream and cache them in __events.
@@ -89,8 +99,8 @@ class QEMUMonitorProtocol:
 
         @raise QMPTimeoutError: If a timeout float is provided and the timeout
                                 period elapses.
-        @raise QMPConnectError: If wait is True but no events could be retrieved
-                                or if some other error occurred.
+        @raise QMPConnectError: If wait is True but no events could be
+                                retrieved or if some other error occurred.
         """
 
         # Check for new events regardless and pull them into the cache:
@@ -175,7 +185,7 @@ class QEMUMonitorProtocol:
         @param args: command arguments (dict)
         @param id: command id (dict, list, string or int)
         """
-        qmp_cmd = { 'execute': name }
+        qmp_cmd = {'execute': name}
         if args:
             qmp_cmd['arguments'] = args
         if id:
@@ -183,6 +193,9 @@ class QEMUMonitorProtocol:
         return self.cmd_obj(qmp_cmd)
 
     def command(self, cmd, **kwds):
+        """
+        Build and send a QMP command to the monitor, report errors if any
+        """
         ret = self.cmd(cmd, kwds)
         if ret.has_key('error'):
             raise Exception(ret['error']['desc'])
@@ -190,15 +203,15 @@ class QEMUMonitorProtocol:
 
     def pull_event(self, wait=False):
         """
-        Get and delete the first available QMP event.
+        Pulls a single event.
 
         @param wait (bool): block until an event is available.
         @param wait (float): If wait is a float, treat it as a timeout value.
 
         @raise QMPTimeoutError: If a timeout float is provided and the timeout
                                 period elapses.
-        @raise QMPConnectError: If wait is True but no events could be retrieved
-                                or if some other error occurred.
+        @raise QMPConnectError: If wait is True but no events could be
+                                retrieved or if some other error occurred.
 
         @return The first available QMP event, or None.
         """
@@ -217,8 +230,8 @@ class QEMUMonitorProtocol:
 
         @raise QMPTimeoutError: If a timeout float is provided and the timeout
                                 period elapses.
-        @raise QMPConnectError: If wait is True but no events could be retrieved
-                                or if some other error occurred.
+        @raise QMPConnectError: If wait is True but no events could be
+                                retrieved or if some other error occurred.
 
         @return The list of available QMP events.
         """
@@ -235,8 +248,6 @@ class QEMUMonitorProtocol:
         self.__sock.close()
         self.__sockfile.close()
 
-    timeout = socket.timeout
-
     def settimeout(self, timeout):
         self.__sock.settimeout(timeout)
 
-- 
2.13.5

[Qemu-devel] [PULL 07/15] qmp.py: Use object-based class for QEMUMonitorProtocol

[Eduardo Habkost]

From: Lukáš Doktor <ldoktor@redhat.com>

There is no need to define QEMUMonitorProtocol as old-style class.

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Message-Id: <20170818142613.32394-8-ldoktor@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qmp/qmp.py    | 2 +-
 scripts/qmp/qmp-shell | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 782d1ac5df..95ff5cc39a 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -30,7 +30,7 @@ class QMPTimeoutError(QMPError):
     pass
 
 
-class QEMUMonitorProtocol:
+class QEMUMonitorProtocol(object):
 
     #: Socket's error class
     error = socket.error
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 860ffb27f2..be449de621 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -106,7 +106,7 @@ class FuzzyJSON(ast.NodeTransformer):
 #       _execute_cmd()). Let's design a better one.
 class QMPShell(qmp.QEMUMonitorProtocol):
     def __init__(self, address, pretty=False):
-        qmp.QEMUMonitorProtocol.__init__(self, self.__get_address(address))
+        super(QMPShell, self).__init__(self.__get_address(address))
         self._greeting = None
         self._completer = None
         self._pretty = pretty
@@ -281,7 +281,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
         return True
 
     def connect(self, negotiate):
-        self._greeting = qmp.QEMUMonitorProtocol.connect(self, negotiate)
+        self._greeting = super(QMPShell, self).connect(negotiate)
         self.__completer_setup()
 
     def show_banner(self, msg='Welcome to the QMP low-level shell!'):
-- 
2.13.5

[Qemu-devel] [PULL 08/15] qmp.py: Avoid "has_key" usage

[Eduardo Habkost]

From: Lukáš Doktor <ldoktor@redhat.com>

The "has_key" is deprecated in favor of "__in__" operator.

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20170818142613.32394-9-ldoktor@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qmp/qmp.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 95ff5cc39a..f2f5a9b296 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -197,7 +197,7 @@ class QEMUMonitorProtocol(object):
         Build and send a QMP command to the monitor, report errors if any
         """
         ret = self.cmd(cmd, kwds)
-        if ret.has_key('error'):
+        if "error" in ret:
             raise Exception(ret['error']['desc'])
         return ret['return']
 
-- 
2.13.5

[Qemu-devel] [PULL 09/15] qmp.py: Avoid overriding a builtin object

[Eduardo Habkost]

From: Lukáš Doktor <ldoktor@redhat.com>

The "id" is a builtin method to get object's identity and should not be
overridden. This might bring some issues in case someone was directly
calling "cmd(..., id=id)" but I haven't found such usage on brief search
for "cmd\(.*id=".

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Message-Id: <20170818142613.32394-10-ldoktor@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qmp/qmp.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index f2f5a9b296..ef12e8a1a0 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
             print >>sys.stderr, "QMP:<<< %s" % resp
         return resp
 
-    def cmd(self, name, args=None, id=None):
+    def cmd(self, name, args=None, cmd_id=None):
         """
         Build a QMP command and send it to the QMP Monitor.
 
         @param name: command name (string)
         @param args: command arguments (dict)
-        @param id: command id (dict, list, string or int)
+        @param cmd_id: command id (dict, list, string or int)
         """
         qmp_cmd = {'execute': name}
         if args:
             qmp_cmd['arguments'] = args
-        if id:
-            qmp_cmd['id'] = id
+        if cmd_id:
+            qmp_cmd['id'] = cmd_id
         return self.cmd_obj(qmp_cmd)
 
     def command(self, cmd, **kwds):
-- 
2.13.5

[Qemu-devel] [PULL 10/15] qtest.py: Few pylint/style fixes

[Eduardo Habkost]

From: Lukáš Doktor <ldoktor@redhat.com>

No actual code changes, just few pylint/style fixes.

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-Id: <20170818142613.32394-11-ldoktor@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qtest.py | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/scripts/qtest.py b/scripts/qtest.py
index ab183c0635..df0daf26ca 100644
--- a/scripts/qtest.py
+++ b/scripts/qtest.py
@@ -11,14 +11,11 @@
 # Based on qmp.py.
 #
 
-import errno
 import socket
-import string
 import os
-import subprocess
-import qmp.qmp
 import qemu
 
+
 class QEMUQtestProtocol(object):
     def __init__(self, address, server=False):
         """
@@ -83,8 +80,10 @@ class QEMUQtestMachine(qemu.QEMUMachine):
                  socket_scm_helper=None):
         if name is None:
             name = "qemu-%d" % os.getpid()
-        super(QEMUQtestMachine, self).__init__(binary, args, name=name, test_dir=test_dir,
-                                               socket_scm_helper=socket_scm_helper)
+        super(QEMUQtestMachine,
+              self).__init__(binary, args, name=name, test_dir=test_dir,
+                             socket_scm_helper=socket_scm_helper)
+        self._qtest = None
         self._qtest_path = os.path.join(test_dir, name + "-qtest.sock")
 
     def _base_args(self):
-- 
2.13.5

[Qemu-devel] [PULL 11/15] qemu.py: fix is_running() return before first launch()

[Eduardo Habkost]

From: Amador Pahim <apahim@redhat.com>

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>
Message-Id: <20170901112829.2571-2-apahim@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qemu.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 7c6802609a..f80b008f7f 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -131,7 +131,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] [PULL 12/15] qemu.py: avoid writing to stdout/stderr

[Eduardo Habkost]

From: Amador Pahim <apahim@redhat.com>

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>
Message-Id: <20170901112829.2571-3-apahim@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qemu.py | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index f80b008f7f..8d3d24dd2b 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -13,12 +13,22 @@
 #
 
 import errno
+import logging
 import os
 import sys
 import subprocess
 import qmp.qmp
 
 
+LOG = logging.getLogger(__name__)
+
+
+class QEMUMachineError(Exception):
+    """
+    Exception called when an error in QEMUMachine happens.
+    """
+
+
 class MonitorResponseError(qmp.qmp.QMPError):
     '''
     Represents erroneous QMP monitor reply
@@ -107,18 +117,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 not os.path.exists(self._socket_scm_helper):
-            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')
-        proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
-                                stderr=sys.stderr)
-        return proc.wait()
+        proc = subprocess.Popen(fd_param, stdin=devnull, stdout=subprocess.PIPE,
+                                stderr=subprocess.STDOUT)
+        output = proc.communicate()[0]
+        if output:
+            LOG.debug(output)
+
+        return proc.returncode
 
     @staticmethod
     def _remove_if_exists(path):
@@ -202,8 +215,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] [PULL 13/15] qemu.py: use os.path.null instead of /dev/null

[Eduardo Habkost]

From: Amador Pahim <apahim@redhat.com>

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

Signed-off-by: Amador Pahim <apahim@redhat.com>
Message-Id: <20170901112829.2571-4-apahim@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qemu.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 8d3d24dd2b..c9bcaafe41 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -124,7 +124,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')
         proc = subprocess.Popen(fd_param, stdin=devnull, stdout=subprocess.PIPE,
                                 stderr=subprocess.STDOUT)
         output = proc.communicate()[0]
@@ -186,7 +186,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] [PULL 14/15] qemu.py: improve message on negative exit code

[Eduardo Habkost]

From: Amador Pahim <apahim@redhat.com>

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>
Message-Id: <20170901112829.2571-5-apahim@redhat.com>
[ehabkost: removed superfluous parenthesis]
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qemu.py | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index c9bcaafe41..9440261ac3 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -87,6 +87,7 @@ class QEMUMachine(object):
         self._socket_scm_helper = socket_scm_helper
         self._debug = debug
         self._qmp = None
+        self._qemu_full_args = None
 
     def __enter__(self):
         return self
@@ -186,13 +187,16 @@ 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,
+            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()
@@ -212,14 +216,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)
+
     def qmp(self, cmd, conv_keys=True, **args):
         '''Invoke a QMP command and return the response dict'''
         qmp_args = dict()
-- 
2.13.5

[Qemu-devel] [PULL 15/15] qemu.py: include debug information on launch error

[Eduardo Habkost]

From: Amador Pahim <apahim@redhat.com>

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>
Message-Id: <20170901112829.2571-6-apahim@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qemu.py | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 9440261ac3..8c67595ec8 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -187,6 +187,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')
@@ -206,6 +207,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

Re: [Qemu-devel] [PULL 00/15] Python queue, 2017-09-15

[Peter Maydell]

On 16 September 2017 at 00:37, Eduardo Habkost <ehabkost@redhat.com> wrote:
> I have been queueing some patches for Python modules and scripts,
> recently, and submitted a MAINTAINERS patch to include Cleber and
> me as maintainers.
>
> However, I don't want to delay this pull request to avoid
> conflicts if people send additional patches for those scripts, so
> I'm sending this pull request before the MAINTAINERS patch is
> applied.
>
> The following changes since commit 5faf2d376af3cb4eb92da44c2580e08d39832caa:
>
>   Merge remote-tracking branch 'remotes/huth/tags/check-20170915' into staging (2017-09-15 20:29:44 +0100)
>
> are available in the git repository at:
>
>   git://github.com/ehabkost/qemu.git tags/python-next-pull-request
>
> for you to fetch changes up to b92a0011b1220aff549ae82c6104014d25e339ef:
>
>   qemu.py: include debug information on launch error (2017-09-15 20:12:00 -0300)
>
> ----------------------------------------------------------------
> Python queue, 2017-09-15
>

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 14/15] qemu.py: improve message on negative exit code

[Daniel P. Berrange]

On Fri, Sep 15, 2017 at 08:37:38PM -0300, Eduardo Habkost wrote:
> From: Amador Pahim <apahim@redhat.com>
> 
> 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>
> Message-Id: <20170901112829.2571-5-apahim@redhat.com>
> [ehabkost: removed superfluous parenthesis]
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  scripts/qemu.py | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index c9bcaafe41..9440261ac3 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -87,6 +87,7 @@ class QEMUMachine(object):
>          self._socket_scm_helper = socket_scm_helper
>          self._debug = debug
>          self._qmp = None
> +        self._qemu_full_args = None
>  
>      def __enter__(self):
>          return self
> @@ -186,13 +187,16 @@ 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,
> +            self._qemu_full_args = self._wrapper + [self._binary] +
> +                                    self._base_args() + self._args

FYI, this change causes a syntax error because you need either the ()
brackets, or an explicit \ continuation. Unfortunately this wasn't
caught by Peter's merge tests, since this code is only execised by
the qemu iotests which aren't run from a 'make check'.

If someone has cycles, it would be great to write some unit tests
directly targetting the qemu.py and qmp.py code, so that we get test
coverage of it independantly of the iotest execution.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PULL 14/15] qemu.py: improve message on negative exit code

[Eduardo Habkost]

On Mon, Sep 18, 2017 at 10:44:00AM +0100, Daniel P. Berrange wrote:
> On Fri, Sep 15, 2017 at 08:37:38PM -0300, Eduardo Habkost wrote:
> > From: Amador Pahim <apahim@redhat.com>
> > 
> > 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>
> > Message-Id: <20170901112829.2571-5-apahim@redhat.com>
> > [ehabkost: removed superfluous parenthesis]
> > Reviewed-by: Fam Zheng <famz@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  scripts/qemu.py | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/scripts/qemu.py b/scripts/qemu.py
> > index c9bcaafe41..9440261ac3 100644
> > --- a/scripts/qemu.py
> > +++ b/scripts/qemu.py
> > @@ -87,6 +87,7 @@ class QEMUMachine(object):
> >          self._socket_scm_helper = socket_scm_helper
> >          self._debug = debug
> >          self._qmp = None
> > +        self._qemu_full_args = None
> >  
> >      def __enter__(self):
> >          return self
> > @@ -186,13 +187,16 @@ 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,
> > +            self._qemu_full_args = self._wrapper + [self._binary] +
> > +                                    self._base_args() + self._args
> 
> FYI, this change causes a syntax error because you need either the ()
> brackets, or an explicit \ continuation. Unfortunately this wasn't
> caught by Peter's merge tests, since this code is only execised by
> the qemu iotests which aren't run from a 'make check'.
> 
> If someone has cycles, it would be great to write some unit tests
> directly targetting the qemu.py and qmp.py code, so that we get test
> coverage of it independantly of the iotest execution.

Oops, sorry!  I thought "make check" was exercising that code
path.

-- 
Eduardo