[Qemu-devel] [PATCH 00/11] qemu.py: Pylint/style fixes

[Qemu-devel] [PATCH 00/11] qemu.py: Pylint/style fixes

[Lukáš Doktor]

Hello guys,

I'm reading the available python modules to exercise qemu and while reading them
I fixed some issues that caught my attention. It usually starts with a simple
pylint/docstring fixes and slowly graduates to more controversial ones so I'm
open to suggestion to remove some of them.

Kind regards,
Lukáš

Lukáš Doktor (11):
  qemu.py: Pylint/style fixes
  qemu.py: 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
  qtest.py: Avoid using mutable list as default argument

 scripts/qemu.py    | 98 +++++++++++++++++++++++++++++++++++++++---------------
 scripts/qmp/qmp.py | 49 ++++++++++++++++-----------
 scripts/qtest.py   | 14 ++++----
 3 files changed, 108 insertions(+), 53 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 01/11] qemu.py: Pylint/style fixes

[Lukáš Doktor]

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

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
---
 scripts/qemu.py | 76 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 23 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 880e3e8..191c916 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -23,8 +23,22 @@ import qmp.qmp
 class QEMUMachine(object):
     '''A QEMU VM'''
 
-    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):
+        '''
+        Create a QEMUMachine object
+
+        @param binary: path to the qemu binary (str)
+        @param args: initial list of extra arguments
+        @param wrapper: list of arguments used as prefix to qemu binary
+        @param name: name of this object (used for log/monitor/... file names)
+        @param test_dir: base location to put log/monitor/... files in
+        @param monitor_address: custom address for QMP monitor
+        @param socket_scm_helper: path to scm_helper binary (to forward fds)
+        @param debug: enable debug mode (forwarded to QMP helper and such)
+        @note: Qemu process is not started until launch() is used.
+        '''
         if name is None:
             name = "qemu-%d" % os.getpid()
         if monitor_address is None:
@@ -33,12 +47,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
 
     # This can be used to add an unused monitor instance.
     def add_monitor_telnet(self, ip, port):
@@ -64,16 +79,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 os.path.exists(self._socket_scm_helper) is False:
             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):
@@ -99,8 +114,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):
@@ -114,8 +129,8 @@ class QEMUMachine(object):
                 '-display', 'none', '-vga', 'none']
 
     def _pre_launch(self):
-        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address, server=True,
-                                                debug=self._debug)
+        self._qmp = qmp.qmp.QEMUMonitorProtocol(self._monitor_address,
+                                                server=True, debug=self._debug)
 
     def _post_launch(self):
         self._qmp.accept()
@@ -131,9 +146,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():
@@ -149,28 +166,34 @@ class QEMUMachine(object):
             try:
                 self._qmp.cmd('quit')
                 self._qmp.close()
-            except:
+            except:     # kill VM on any failure pylint: disable=W0702
                 self._popen.kill()
 
             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'''
         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 and on success report result dict or on failure
+        raise exception with details.
+        '''
         reply = self.qmp(cmd, conv_keys, **args)
         if reply is None:
             raise Exception("Monitor is closed")
@@ -193,15 +216,18 @@ class QEMUMachine(object):
         return events
 
     def event_wait(self, name, timeout=60.0, match=None):
-        # Test if 'match' is a recursive subset of 'event'
-        def event_match(event, match=None):
+        ''' Wait for event in QMP, optionally filter results by match. '''
+        # Test if 'match' is a recursive subset of 'event'; skips branch
+        # processing on match's value `None`
+        #    {"foo": {"bar": 1} matches {"foo": None}
+        def _event_match(event, match=None):
             if match is None:
                 return True
 
             for key in match:
                 if key in event:
                     if isinstance(event[key], dict):
-                        if not event_match(event[key], match[key]):
+                        if not _event_match(event[key], match[key]):
                             return False
                     elif event[key] != match[key]:
                         return False
@@ -212,18 +238,22 @@ class QEMUMachine(object):
 
         # Search cached events
         for event in self._events:
-            if (event['event'] == name) and event_match(event, match):
+            if (event['event'] == name) and _event_match(event, match):
                 self._events.remove(event)
                 return event
 
         # Poll for new events
         while True:
             event = self._qmp.pull_event(wait=timeout)
-            if (event['event'] == name) and event_match(event, match):
+            if (event['event'] == name) and _event_match(event, match):
                 return event
             self._events.append(event)
 
         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.9.4

[Qemu-devel] [PATCH 02/11] qemu.py: Avoid dangerous arguments

[Lukáš Doktor]

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.

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
---
 scripts/qemu.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 191c916..66fd863 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -23,7 +23,7 @@ import qmp.qmp
 class QEMUMachine(object):
     '''A QEMU VM'''
 
-    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):
         '''
@@ -39,6 +39,10 @@ class QEMUMachine(object):
         @param debug: enable debug mode (forwarded to QMP helper and such)
         @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:
-- 
2.9.4

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

[Lukáš Doktor]

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>
---
 scripts/qemu.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 66fd863..7e95c25 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -185,11 +185,11 @@ class QEMUMachine(object):
     def qmp(self, cmd, conv_keys=True, **args):
         '''Invoke a QMP command and return the result 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.9.4

[Qemu-devel] [PATCH 04/11] qemu.py: Simplify QMP key-conversion

[Lukáš Doktor]

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>
---
 scripts/qemu.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 7e95c25..5948e19 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -13,7 +13,6 @@
 #
 
 import errno
-import string
 import os
 import sys
 import subprocess
@@ -180,14 +179,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 result 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.9.4

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

[Lukáš Doktor]

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>
---
 scripts/qemu.py | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 5948e19..2bd522f 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'''
 
@@ -197,9 +210,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.9.4

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

[Lukáš Doktor]

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

Signed-off-by: Lukáš Doktor <ldoktor@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 62d3651..bb4d614 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 when 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.
+        Get and pop the first available QMP 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.9.4

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

[Lukáš Doktor]

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

Signed-off-by: Lukáš Doktor <ldoktor@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 bb4d614..68f3420 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
-- 
2.9.4

[Qemu-devel] [PATCH 08/11] qmp.py: Avoid "has_key" usage

[Lukáš Doktor]

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

Signed-off-by: Lukáš Doktor <ldoktor@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 68f3420..a14b001 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 when any
         """
         ret = self.cmd(cmd, kwds)
-        if ret.has_key('error'):
+        if "error" in ret:
             raise Exception(ret['error']['desc'])
         return ret['return']
 
-- 
2.9.4

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

[Lukáš Doktor]

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>
---
 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 a14b001..c3e0206 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['cmd_id'] = cmd_id
         return self.cmd_obj(qmp_cmd)
 
     def command(self, cmd, **kwds):
-- 
2.9.4

[Qemu-devel] [PATCH 10/11] qtest.py: Few pylint/style fixes

[Lukáš Doktor]

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

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
---
 scripts/qtest.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/qtest.py b/scripts/qtest.py
index d5aecb5..e4b5788 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,11 @@ 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)
+        scm_helper = socket_scm_helper
+        super(QEMUQtestMachine, self).__init__(binary, args, name=name,
+                                               test_dir=test_dir,
+                                               socket_scm_helper=scm_helper)
+        self._qtest = None
         self._qtest_path = os.path.join(test_dir, name + "-qtest.sock")
 
     def _base_args(self):
-- 
2.9.4

[Qemu-devel] [PATCH 11/11] qtest.py: Avoid using mutable list as default argument

[Lukáš Doktor]

The list is a mutable object and is dangerous to use it. Recently the
QEMUMachine was adjusted to allow None as default, let's use it here as
well.

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
---
 scripts/qtest.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qtest.py b/scripts/qtest.py
index e4b5788..ba3233c 100644
--- a/scripts/qtest.py
+++ b/scripts/qtest.py
@@ -76,7 +76,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.9.4

Re: [Qemu-devel] [PATCH 02/11] qemu.py: Avoid dangerous arguments

[Eduardo Habkost]

On Thu, Jul 20, 2017 at 06:28:06PM +0200, Lukáš Doktor wrote:
> 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.
> 
> Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH 03/11] qemu.py: Use iteritems rather than keys()

[Eduardo Habkost]

On Thu, Jul 20, 2017 at 06:28:07PM +0200, Lukáš Doktor wrote:
> 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>

-- 
Eduardo

Re: [Qemu-devel] [PATCH 04/11] qemu.py: Simplify QMP key-conversion

[Eduardo Habkost]

On Thu, Jul 20, 2017 at 06:28:08PM +0200, Lukáš Doktor wrote:
> 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>

-- 
Eduardo

Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception

[Eduardo Habkost]

On Thu, Jul 20, 2017 at 06:28:09PM +0200, Lukáš Doktor wrote:
> 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>
> ---
>  scripts/qemu.py | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 5948e19..2bd522f 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

This would require every user of self.reply to first check if
it's a string or dictionary.  All because of the "Monitor is
closed" case below:

> +
> +
>  class QEMUMachine(object):
>      '''A QEMU VM'''
>  
> @@ -197,9 +210,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")

Should we really use the same exception type for this, if it's
not really a QMP monitor error reply, and won't even have a real
QMP reply in self.reply?


>          if "error" in reply:
> -            raise Exception(reply["error"]["desc"])
> +            raise MonitorResponseError(reply)
>          return reply["return"]
>  
>      def get_qmp_event(self, wait=False):
> -- 
> 2.9.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 07/11] qmp.py: Use object-based class for QEMUMonitorProtocol

[Eduardo Habkost]

On Thu, Jul 20, 2017 at 06:28:11PM +0200, Lukáš Doktor wrote:
> There is no need to define QEMUMonitorProtocol as old-style class.
> 
> Signed-off-by: Lukáš Doktor <ldoktor@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 bb4d614..68f3420 100644
> --- a/scripts/qmp/qmp.py
> +++ b/scripts/qmp/qmp.py
> @@ -30,7 +30,7 @@ class QMPTimeoutError(QMPError):
>      pass
>  
>  
> -class QEMUMonitorProtocol:
> +class QEMUMonitorProtocol(object):

I don't fully understand the consequences of changing to
new-style classes when using old-style SuperClass.__init__()
calls in the __init__().  Should we change QMPShell.__init__() to
use super()?


>  
>      #: Socket's error class
>      error = socket.error
> -- 
> 2.9.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 08/11] qmp.py: Avoid "has_key" usage

[Eduardo Habkost]

On Thu, Jul 20, 2017 at 06:28:12PM +0200, Lukáš Doktor wrote:
> 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>

-- 
Eduardo

Re: [Qemu-devel] [PATCH 09/11] qmp.py: Avoid overriding a builtin object

[Eduardo Habkost]

On Thu, Jul 20, 2017 at 06:28:13PM +0200, Lukáš Doktor wrote:
> 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>
> ---
>  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 a14b001..c3e0206 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['cmd_id'] = cmd_id

The member sent through the monitor should still be called "id".
i.e.:

    qmp_cmd['id'] = cmd_id


>          return self.cmd_obj(qmp_cmd)
>  
>      def command(self, cmd, **kwds):
> -- 
> 2.9.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 10/11] qtest.py: Few pylint/style fixes

[Eduardo Habkost]

On Thu, Jul 20, 2017 at 06:28:14PM +0200, Lukáš Doktor wrote:
[...]
> @@ -83,8 +80,11 @@ 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)
> +        scm_helper = socket_scm_helper

Why is this necessary?

> +        super(QEMUQtestMachine, self).__init__(binary, args, name=name,
> +                                               test_dir=test_dir,
> +                                               socket_scm_helper=scm_helper)
> +        self._qtest = None
>          self._qtest_path = os.path.join(test_dir, name + "-qtest.sock")
>  
>      def _base_args(self):
> -- 
> 2.9.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 11/11] qtest.py: Avoid using mutable list as default argument

[Eduardo Habkost]

On Thu, Jul 20, 2017 at 06:28:15PM +0200, Lukáš Doktor wrote:
> The list is a mutable object and is dangerous to use it. Recently the
> QEMUMachine was adjusted to allow None as default, let's use it here as
> well.
> 
> Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>

This patch requires patch 02/11 and both are pretty small.
I suggest squashing them together.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 01/11] qemu.py: Pylint/style fixes

[Philipp Hahn]

Hi,

Am 20.07.2017 um 18:28 schrieb Lukáš Doktor:
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
...
> @@ -64,16 +79,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 os.path.exists(self._socket_scm_helper) is False:

if not os.path.exists(...):

Philipp

Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception

[Lukáš Doktor]

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

Dne 20.7.2017 v 20:27 Eduardo Habkost napsal(a):
> On Thu, Jul 20, 2017 at 06:28:09PM +0200, Lukáš Doktor wrote:
>> 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>
>> ---
>>  scripts/qemu.py | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index 5948e19..2bd522f 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
> 
> This would require every user of self.reply to first check if
> it's a string or dictionary.  All because of the "Monitor is
> closed" case below:
> 
I haven't used it for the `Monitor is closed` exception, so it's just to be able to store really broken reply safely. Anyway people can check whether the reply is a dict, or I can add `is_valid_reply` property which would check for `[error][desc]` presence (which is a bit more precise than just plain dict type checking).

>> +
>> +
>>  class QEMUMachine(object):
>>      '''A QEMU VM'''
>>  
>> @@ -197,9 +210,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")
> 
> Should we really use the same exception type for this, if it's
> not really a QMP monitor error reply, and won't even have a real
> QMP reply in self.reply?
> 
I wasn't sure but the same exception can be caused by other failures when obtaining replies so I think in case the monitor is closed we might expect the same exception. Would importing it in the top level of this module to become `qemu.QMPError` work for you better, or would you prefer IOError, RuntimeError or another custom exception?

Lukáš

> 
>>          if "error" in reply:
>> -            raise Exception(reply["error"]["desc"])
>> +            raise MonitorResponseError(reply)
>>          return reply["return"]
>>  
>>      def get_qmp_event(self, wait=False):
>> -- 
>> 2.9.4
>>
> 



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

Re: [Qemu-devel] [PATCH 07/11] qmp.py: Use object-based class for QEMUMonitorProtocol

[Lukáš Doktor]

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

Dne 20.7.2017 v 20:35 Eduardo Habkost napsal(a):
> On Thu, Jul 20, 2017 at 06:28:11PM +0200, Lukáš Doktor wrote:
>> There is no need to define QEMUMonitorProtocol as old-style class.
>>
>> Signed-off-by: Lukáš Doktor <ldoktor@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 bb4d614..68f3420 100644
>> --- a/scripts/qmp/qmp.py
>> +++ b/scripts/qmp/qmp.py
>> @@ -30,7 +30,7 @@ class QMPTimeoutError(QMPError):
>>      pass
>>  
>>  
>> -class QEMUMonitorProtocol:
>> +class QEMUMonitorProtocol(object):
> 
> I don't fully understand the consequences of changing to
> new-style classes when using old-style SuperClass.__init__()
> calls in the __init__().  Should we change QMPShell.__init__() to
> use super()?
> 
The consequence is it becomes a proper object with full object model and less workarounds. It also consumes a bit more memory but are the only available mode in py3.

As for the old-style superclass, it works, but the correct approach is to replace it with `super` call. I'll address that in the v2 (I only checked for `.py` files but there are many python sources in qemu tree without the proper extension. I still need to get used to this.).

Lukáš

> 
>>  
>>      #: Socket's error class
>>      error = socket.error
>> -- 
>> 2.9.4
>>
> 



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

Re: [Qemu-devel] [PATCH 09/11] qmp.py: Avoid overriding a builtin object

[Lukáš Doktor]

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

Dne 20.7.2017 v 20:38 Eduardo Habkost napsal(a):
> On Thu, Jul 20, 2017 at 06:28:13PM +0200, Lukáš Doktor wrote:
>> 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>
>> ---
>>  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 a14b001..c3e0206 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['cmd_id'] = cmd_id
> 
> The member sent through the monitor should still be called "id".
> i.e.:
> 
>     qmp_cmd['id'] = cmd_id
> 
Oups, sorry, automatic rename changed it and I forgot to fix this one back. I'll address this in v2. The main problem with this patch is it could break named arguments (`cmd(..., id=id)` calls) so I'm not sure it's worth including. But as mentioned in commit message I grepped full sources so we better fix this before someone starts using it.

Lukáš

> 
>>          return self.cmd_obj(qmp_cmd)
>>  
>>      def command(self, cmd, **kwds):
>> -- 
>> 2.9.4
>>
> 



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

Re: [Qemu-devel] [PATCH 10/11] qtest.py: Few pylint/style fixes

[Lukáš Doktor]

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

Dne 20.7.2017 v 20:42 Eduardo Habkost napsal(a):
> On Thu, Jul 20, 2017 at 06:28:14PM +0200, Lukáš Doktor wrote:
> [...]
>> @@ -83,8 +80,11 @@ 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)
>> +        scm_helper = socket_scm_helper
> 
> Why is this necessary?
> 
to avoid > 80 chars line. It should be optimized-out by the python compiler so it should not slow down the execution. Alternative solution is to use:

    super(QEMUQtestMachine,
          self.__init__(...)

which looks IMO uglier, but I can use that in v2, should that be your preferred style.

Lukáš

>> +        super(QEMUQtestMachine, self).__init__(binary, args, name=name,
>> +                                               test_dir=test_dir,
>> +                                               socket_scm_helper=scm_helper)
>> +        self._qtest = None
>>          self._qtest_path = os.path.join(test_dir, name + "-qtest.sock")
>>  
>>      def _base_args(self):
>> -- 
>> 2.9.4
>>
> 



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

Re: [Qemu-devel] [PATCH 11/11] qtest.py: Avoid using mutable list as default argument

[Lukáš Doktor]

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

Dne 20.7.2017 v 20:44 Eduardo Habkost napsal(a):
> On Thu, Jul 20, 2017 at 06:28:15PM +0200, Lukáš Doktor wrote:
>> The list is a mutable object and is dangerous to use it. Recently the
>> QEMUMachine was adjusted to allow None as default, let's use it here as
>> well.
>>
>> Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
> 
> This patch requires patch 02/11 and both are pretty small.
> I suggest squashing them together.
> 

Sure, I'll change it in the v2.

Lukáš


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

Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception

[Eduardo Habkost]

On Fri, Jul 21, 2017 at 08:37:34AM +0200, Lukáš Doktor wrote:
> Dne 20.7.2017 v 20:27 Eduardo Habkost napsal(a):
> > On Thu, Jul 20, 2017 at 06:28:09PM +0200, Lukáš Doktor wrote:
> >> 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>
> >> ---
> >>  scripts/qemu.py | 17 +++++++++++++++--
> >>  1 file changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/scripts/qemu.py b/scripts/qemu.py
> >> index 5948e19..2bd522f 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
> > 
> > This would require every user of self.reply to first check if
> > it's a string or dictionary.  All because of the "Monitor is
> > closed" case below:
> > 
> I haven't used it for the `Monitor is closed` exception, so
> it's just to be able to store really broken reply safely.
> Anyway people can check whether the reply is a dict, or I can
> add `is_valid_reply` property which would check for
> `[error][desc]` presence (which is a bit more precise than just
> plain dict type checking).


Oops, I wasn't paying enough attention, sorry.  self.reply is
actually always set to the response from the monitor.

If you are just trying a safe fallback for 'desc' if the response
broken, what about using repr(reply) or json.dumps(reply) if
reply['error']['desc'] isn't set?

> 
> >> +
> >> +
> >>  class QEMUMachine(object):
> >>      '''A QEMU VM'''
> >>  
> >> @@ -197,9 +210,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")
> > 
> > Should we really use the same exception type for this, if it's
> > not really a QMP monitor error reply, and won't even have a real
> > QMP reply in self.reply?
> > 
> I wasn't sure but the same exception can be caused by other
> failures when obtaining replies so I think in case the monitor
> is closed we might expect the same exception. Would importing
> it in the top level of this module to become `qemu.QMPError`
> work for you better, or would you prefer IOError, RuntimeError
> or another custom exception?

I was not paying enough attention.  QMPError sounds good to me.

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

> 
> Lukáš
> 
> > 
> >>          if "error" in reply:
> >> -            raise Exception(reply["error"]["desc"])
> >> +            raise MonitorResponseError(reply)
> >>          return reply["return"]
> >>  
> >>      def get_qmp_event(self, wait=False):
> >> -- 
> >> 2.9.4
> >>
> > 
> 
> 




-- 
Eduardo

Re: [Qemu-devel] [PATCH 07/11] qmp.py: Use object-based class for QEMUMonitorProtocol

[Eduardo Habkost]

On Fri, Jul 21, 2017 at 08:50:22AM +0200, Lukáš Doktor wrote:
> Dne 20.7.2017 v 20:35 Eduardo Habkost napsal(a):
> > On Thu, Jul 20, 2017 at 06:28:11PM +0200, Lukáš Doktor wrote:
> >> There is no need to define QEMUMonitorProtocol as old-style class.
> >>
> >> Signed-off-by: Lukáš Doktor <ldoktor@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 bb4d614..68f3420 100644
> >> --- a/scripts/qmp/qmp.py
> >> +++ b/scripts/qmp/qmp.py
> >> @@ -30,7 +30,7 @@ class QMPTimeoutError(QMPError):
> >>      pass
> >>  
> >>  
> >> -class QEMUMonitorProtocol:
> >> +class QEMUMonitorProtocol(object):
> > 
> > I don't fully understand the consequences of changing to
> > new-style classes when using old-style SuperClass.__init__()
> > calls in the __init__().  Should we change QMPShell.__init__() to
> > use super()?
> > 
> The consequence is it becomes a proper object with full object
> model and less workarounds. It also consumes a bit more memory
> but are the only available mode in py3.
> 
> As for the old-style superclass, it works, but the correct
> approach is to replace it with `super` call. I'll address that
> in the v2 (I only checked for `.py` files but there are many
> python sources in qemu tree without the proper extension. I
> still need to get used to this.).

Thanks for the explanation.

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH 09/11] qmp.py: Avoid overriding a builtin object

[Eduardo Habkost]

On Fri, Jul 21, 2017 at 08:53:49AM +0200, Lukáš Doktor wrote:
> Dne 20.7.2017 v 20:38 Eduardo Habkost napsal(a):
> > On Thu, Jul 20, 2017 at 06:28:13PM +0200, Lukáš Doktor wrote:
> >> 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>
> >> ---
> >>  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 a14b001..c3e0206 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['cmd_id'] = cmd_id
> > 
> > The member sent through the monitor should still be called "id".
> > i.e.:
> > 
> >     qmp_cmd['id'] = cmd_id
> > 
> Oups, sorry, automatic rename changed it and I forgot to fix
> this one back. I'll address this in v2. The main problem with
> this patch is it could break named arguments (`cmd(..., id=id)`
> calls) so I'm not sure it's worth including. But as mentioned
> in commit message I grepped full sources so we better fix this
> before someone starts using it.

I'm not convinced we need this patch, either.  What exactly
breaks if we don't apply this patch and somebody tries to use
cmd(..., id=id)?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 10/11] qtest.py: Few pylint/style fixes

[Eduardo Habkost]

On Fri, Jul 21, 2017 at 08:57:34AM +0200, Lukáš Doktor wrote:
> Dne 20.7.2017 v 20:42 Eduardo Habkost napsal(a):
> > On Thu, Jul 20, 2017 at 06:28:14PM +0200, Lukáš Doktor wrote:
> > [...]
> >> @@ -83,8 +80,11 @@ 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)
> >> +        scm_helper = socket_scm_helper
> > 
> > Why is this necessary?
> > 
> to avoid > 80 chars line. It should be optimized-out by the
> python compiler so it should not slow down the execution.
> Alternative solution is to use:
> 
>     super(QEMUQtestMachine,
>           self.__init__(...)
> 
> which looks IMO uglier, but I can use that in v2, should that be your preferred style.

I think that would be better.  The purpose of the extra variable
isn't clear when reading the code, making it more confusing.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 02/11] qemu.py: Avoid dangerous arguments

[Philippe Mathieu-Daudé]

On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
> 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.
> 
> Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>

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

> ---
>   scripts/qemu.py | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 191c916..66fd863 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -23,7 +23,7 @@ import qmp.qmp
>   class QEMUMachine(object):
>       '''A QEMU VM'''
>   
> -    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):
>           '''
> @@ -39,6 +39,10 @@ class QEMUMachine(object):
>           @param debug: enable debug mode (forwarded to QMP helper and such)
>           @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:
> 

Re: [Qemu-devel] [PATCH 03/11] qemu.py: Use iteritems rather than keys()

[Philippe Mathieu-Daudé]

On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
> 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: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   scripts/qemu.py | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 66fd863..7e95c25 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -185,11 +185,11 @@ class QEMUMachine(object):
>       def qmp(self, cmd, conv_keys=True, **args):
>           '''Invoke a QMP command and return the result 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)
>   
> 

Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes

[Philippe Mathieu-Daudé]

Hi Lukáš,

Since comment/indent fixes and code changes are not related I'd rather 
see this split in at least 2 patches.

On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
> No actual code changes, just a few pylint/style fixes and docstring
> clarifications.
> 
> Signed-off-by: Lukáš Doktor <ldoktor@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 62d3651..bb4d614 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
> +

ok

>       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:

ok

>               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 when any

I'm not native english speaker but I think "if any" sounds better.

> +        """
>           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.
> +        Get and pop the first available QMP event.

Both sentences seems unclear to me, regarding the function name... I 
wonder if removing this comment makes this function clearer.

>   
>           @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)
>   
> 

Regards,

Phil.

Re: [Qemu-devel] [PATCH 08/11] qmp.py: Avoid "has_key" usage

[Philippe Mathieu-Daudé]

On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
> The "has_key" is deprecated in favor of "__in__" operator.
> 
> Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>

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

> ---
>   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 68f3420..a14b001 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 when any
>           """
>           ret = self.cmd(cmd, kwds)
> -        if ret.has_key('error'):
> +        if "error" in ret:
>               raise Exception(ret['error']['desc'])
>           return ret['return']
>   
> 

Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception

[Lukáš Doktor]

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

Dne 21.7.2017 v 20:42 Eduardo Habkost napsal(a):
> On Fri, Jul 21, 2017 at 08:37:34AM +0200, Lukáš Doktor wrote:
>> Dne 20.7.2017 v 20:27 Eduardo Habkost napsal(a):
>>> On Thu, Jul 20, 2017 at 06:28:09PM +0200, Lukáš Doktor wrote:
>>>> 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>
>>>> ---
>>>>  scripts/qemu.py | 17 +++++++++++++++--
>>>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>>>> index 5948e19..2bd522f 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
>>>
>>> This would require every user of self.reply to first check if
>>> it's a string or dictionary.  All because of the "Monitor is
>>> closed" case below:
>>>
>> I haven't used it for the `Monitor is closed` exception, so
>> it's just to be able to store really broken reply safely.
>> Anyway people can check whether the reply is a dict, or I can
>> add `is_valid_reply` property which would check for
>> `[error][desc]` presence (which is a bit more precise than just
>> plain dict type checking).
> 
> 
> Oops, I wasn't paying enough attention, sorry.  self.reply is
> actually always set to the response from the monitor.
> 
> If you are just trying a safe fallback for 'desc' if the response
> broken, what about using repr(reply) or json.dumps(reply) if
> reply['error']['desc'] isn't set?
> 
I could use repr, but I'd prefer keeping it the way it is as you could use `isinstance` to see whether it's dict and interact with it (if needed, eg. on negative testing, which is my motivation for storing the full response).

Lukáš

>>
>>>> +
>>>> +
>>>>  class QEMUMachine(object):
>>>>      '''A QEMU VM'''
>>>>  
>>>> @@ -197,9 +210,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")
>>>
>>> Should we really use the same exception type for this, if it's
>>> not really a QMP monitor error reply, and won't even have a real
>>> QMP reply in self.reply?
>>>
>> I wasn't sure but the same exception can be caused by other
>> failures when obtaining replies so I think in case the monitor
>> is closed we might expect the same exception. Would importing
>> it in the top level of this module to become `qemu.QMPError`
>> work for you better, or would you prefer IOError, RuntimeError
>> or another custom exception?
> 
> I was not paying enough attention.  QMPError sounds good to me.
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
>>
>> Lukáš
>>
>>>
>>>>          if "error" in reply:
>>>> -            raise Exception(reply["error"]["desc"])
>>>> +            raise MonitorResponseError(reply)
>>>>          return reply["return"]
>>>>  
>>>>      def get_qmp_event(self, wait=False):
>>>> -- 
>>>> 2.9.4
>>>>
>>>
>>
>>
> 
> 
> 
> 



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

Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes

[Lukáš Doktor]

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

Dne 22.7.2017 v 03:30 Philippe Mathieu-Daudé napsal(a):
> Hi Lukáš,
> 
> Since comment/indent fixes and code changes are not related I'd rather see this split in at least 2 patches.
> 
Hello Philippe, thank you for the review, I'm wondering what code changes you have in mind? This is commit should not bring any code changes, just code reorganization (like including the self.* attributes on top of the file)

> On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
>> No actual code changes, just a few pylint/style fixes and docstring
>> clarifications.
>>
>> Signed-off-by: Lukáš Doktor <ldoktor@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 62d3651..bb4d614 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
>> +
> 
> ok
> 
>>       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:
> 
> ok
> 
>>               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 when any
> 
> I'm not native english speaker but I think "if any" sounds better.
> 
Yep, you are right.

>> +        """
>>           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.
>> +        Get and pop the first available QMP event.
> 
> Both sentences seems unclear to me, regarding the function name... I wonder if removing this comment makes this function clearer.
> 
I was trying to avoid confusion with the delete. How about `Pulls/waits for a single event`? I'd like to keep the rest of the docstring as the details might be useful.

Lukáš

>>             @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)
>>  
> 
> Regards,
> 
> Phil.



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

Re: [Qemu-devel] [PATCH 09/11] qmp.py: Avoid overriding a builtin object

[Lukáš Doktor]

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

Dne 21.7.2017 v 20:46 Eduardo Habkost napsal(a):
> On Fri, Jul 21, 2017 at 08:53:49AM +0200, Lukáš Doktor wrote:
>> Dne 20.7.2017 v 20:38 Eduardo Habkost napsal(a):
>>> On Thu, Jul 20, 2017 at 06:28:13PM +0200, Lukáš Doktor wrote:
>>>> 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>
>>>> ---
>>>>  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 a14b001..c3e0206 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['cmd_id'] = cmd_id
>>>
>>> The member sent through the monitor should still be called "id".
>>> i.e.:
>>>
>>>     qmp_cmd['id'] = cmd_id
>>>
>> Oups, sorry, automatic rename changed it and I forgot to fix
>> this one back. I'll address this in v2. The main problem with
>> this patch is it could break named arguments (`cmd(..., id=id)`
>> calls) so I'm not sure it's worth including. But as mentioned
>> in commit message I grepped full sources so we better fix this
>> before someone starts using it.
> 
> I'm not convinced we need this patch, either.  What exactly
> breaks if we don't apply this patch and somebody tries to use
> cmd(..., id=id)?
> 

It'll work properly. The only issue is the `id` method will be unavailable inside that function. So let's say you want to add `print` method to debug issue with duplicate arguments, you'd be unable to use `id` to distinguish between them. The other issue is that people will see it and copy&paste this ugliness to other projects poisoning the world :-)

Therefor I'd like to include it, but I'm prepared to yield.

Lukáš


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

Re: [Qemu-devel] [PATCH 10/11] qtest.py: Few pylint/style fixes

[Lukáš Doktor]

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

Dne 21.7.2017 v 20:56 Eduardo Habkost napsal(a):
> On Fri, Jul 21, 2017 at 08:57:34AM +0200, Lukáš Doktor wrote:
>> Dne 20.7.2017 v 20:42 Eduardo Habkost napsal(a):
>>> On Thu, Jul 20, 2017 at 06:28:14PM +0200, Lukáš Doktor wrote:
>>> [...]
>>>> @@ -83,8 +80,11 @@ 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)
>>>> +        scm_helper = socket_scm_helper
>>>
>>> Why is this necessary?
>>>
>> to avoid > 80 chars line. It should be optimized-out by the
>> python compiler so it should not slow down the execution.
>> Alternative solution is to use:
>>
>>     super(QEMUQtestMachine,
>>           self.__init__(...)
>>
>> which looks IMO uglier, but I can use that in v2, should that be your preferred style.
> 
> I think that would be better.  The purpose of the extra variable
> isn't clear when reading the code, making it more confusing.
> 

OK, will fix in v2.
Lukáš


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

Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception

[Eduardo Habkost]

On Mon, Jul 24, 2017 at 02:13:09PM +0200, Lukáš Doktor wrote:
> Dne 21.7.2017 v 20:42 Eduardo Habkost napsal(a):
> > On Fri, Jul 21, 2017 at 08:37:34AM +0200, Lukáš Doktor wrote:
> >> Dne 20.7.2017 v 20:27 Eduardo Habkost napsal(a):
> >>> On Thu, Jul 20, 2017 at 06:28:09PM +0200, Lukáš Doktor wrote:
> >>>> 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>
> >>>> ---
> >>>>  scripts/qemu.py | 17 +++++++++++++++--
> >>>>  1 file changed, 15 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/scripts/qemu.py b/scripts/qemu.py
> >>>> index 5948e19..2bd522f 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
> >>>
> >>> This would require every user of self.reply to first check if
> >>> it's a string or dictionary.  All because of the "Monitor is
> >>> closed" case below:
> >>>
> >> I haven't used it for the `Monitor is closed` exception, so
> >> it's just to be able to store really broken reply safely.
> >> Anyway people can check whether the reply is a dict, or I can
> >> add `is_valid_reply` property which would check for
> >> `[error][desc]` presence (which is a bit more precise than just
> >> plain dict type checking).
> > 
> > 
> > Oops, I wasn't paying enough attention, sorry.  self.reply is
> > actually always set to the response from the monitor.
> > 
> > If you are just trying a safe fallback for 'desc' if the response
> > broken, what about using repr(reply) or json.dumps(reply) if
> > reply['error']['desc'] isn't set?
> > 
> I could use repr, but I'd prefer keeping it the way it is as
> you could use `isinstance` to see whether it's dict and
> interact with it (if needed, eg. on negative testing, which is
> my motivation for storing the full response).

This makes sense for self.reply, but I'm thinking about the
argument to Exception.__init__().  I'm worried that things might
break if the argument is not a string.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 05/11] qemu.py: Use custom exceptions rather than Exception

[Lukáš Doktor]

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

Dne 24.7.2017 v 17:32 Eduardo Habkost napsal(a):
> On Mon, Jul 24, 2017 at 02:13:09PM +0200, Lukáš Doktor wrote:
>> Dne 21.7.2017 v 20:42 Eduardo Habkost napsal(a):
>>> On Fri, Jul 21, 2017 at 08:37:34AM +0200, Lukáš Doktor wrote:
>>>> Dne 20.7.2017 v 20:27 Eduardo Habkost napsal(a):
>>>>> On Thu, Jul 20, 2017 at 06:28:09PM +0200, Lukáš Doktor wrote:
>>>>>> 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>
>>>>>> ---
>>>>>>  scripts/qemu.py | 17 +++++++++++++++--
>>>>>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>>>>>> index 5948e19..2bd522f 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
>>>>>
>>>>> This would require every user of self.reply to first check if
>>>>> it's a string or dictionary.  All because of the "Monitor is
>>>>> closed" case below:
>>>>>
>>>> I haven't used it for the `Monitor is closed` exception, so
>>>> it's just to be able to store really broken reply safely.
>>>> Anyway people can check whether the reply is a dict, or I can
>>>> add `is_valid_reply` property which would check for
>>>> `[error][desc]` presence (which is a bit more precise than just
>>>> plain dict type checking).
>>>
>>>
>>> Oops, I wasn't paying enough attention, sorry.  self.reply is
>>> actually always set to the response from the monitor.
>>>
>>> If you are just trying a safe fallback for 'desc' if the response
>>> broken, what about using repr(reply) or json.dumps(reply) if
>>> reply['error']['desc'] isn't set?
>>>
>> I could use repr, but I'd prefer keeping it the way it is as
>> you could use `isinstance` to see whether it's dict and
>> interact with it (if needed, eg. on negative testing, which is
>> my motivation for storing the full response).
> 
> This makes sense for self.reply, but I'm thinking about the
> argument to Exception.__init__().  I'm worried that things might
> break if the argument is not a string.
> 

I see. It's python so it'll simply use `__str__` method, but you are right that for the exception description the use of `repr` is better. I'll change it in v2 (keeping the `self.reply` as is).
Lukáš


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

Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes

[Philippe Mathieu-Daudé]

Hi Lukáš,

On 07/24/2017 09:36 AM, Lukáš Doktor wrote:
> Dne 22.7.2017 v 03:30 Philippe Mathieu-Daudé napsal(a):
>> Hi Lukáš,
>>
>> Since comment/indent fixes and code changes are not related I'd rather see this split in at least 2 patches.
>>
> Hello Philippe, thank you for the review, I'm wondering what code changes you have in mind? This is commit should not bring any code changes, just code reorganization (like including the self.* attributes on top of the file)
> 
>> On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
>>> No actual code changes, just a few pylint/style fixes and docstring
>>> clarifications.
>>>
>>> Signed-off-by: Lukáš Doktor <ldoktor@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
[...]
>>>        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

I was thinking about this line which is new. Now the declaration and 
initialization are done in __init__() while before it was only 
declared/initialized in connect() or accept().

I'm not expert of python interpreter/jit internals but expect the 
generated code to be slightly different, even if achieving the same.

not a bit deal, just about wording ;)

>>>            if server:
>>>                self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
>>>                self.__sock.bind(self.__address)

Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes

[Lukáš Doktor]

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

Dne 25.7.2017 v 08:04 Philippe Mathieu-Daudé napsal(a):
> Hi Lukáš,
> 
> On 07/24/2017 09:36 AM, Lukáš Doktor wrote:
>> Dne 22.7.2017 v 03:30 Philippe Mathieu-Daudé napsal(a):
>>> Hi Lukáš,
>>>
>>> Since comment/indent fixes and code changes are not related I'd rather see this split in at least 2 patches.
>>>
>> Hello Philippe, thank you for the review, I'm wondering what code changes you have in mind? This is commit should not bring any code changes, just code reorganization (like including the self.* attributes on top of the file)
>>
>>> On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
>>>> No actual code changes, just a few pylint/style fixes and docstring
>>>> clarifications.
>>>>
>>>> Signed-off-by: Lukáš Doktor <ldoktor@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
> [...]
>>>>        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
> 
> I was thinking about this line which is new. Now the declaration and initialization are done in __init__() while before it was only declared/initialized in connect() or accept().
> 
> I'm not expert of python interpreter/jit internals but expect the generated code to be slightly different, even if achieving the same.
> 
> not a bit deal, just about wording ;)
> 
Well the difference is that before `connect` you get `AttributeError` when looking for `self.__sockfile` while with this patch you'll get `None`. In this code nobody queries for `self.__sockfile` before the `connect` so it should not change the behavior of this code so I consider that as a `style` fix as it's ugly to extend attributes later in code (with some exceptions like Namespace-objects..). Anyway if you insist I can split those patches.

Lukáš

>>>>            if server:
>>>>                self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
>>>>                self.__sock.bind(self.__address)



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

Re: [Qemu-devel] [PATCH 06/11] qmp.py: Couple of pylint/style fixes

[Philippe Mathieu-Daudé]

On Tue, Jul 25, 2017 at 3:13 AM, Lukáš Doktor <ldoktor@redhat.com> wrote:
> Dne 25.7.2017 v 08:04 Philippe Mathieu-Daudé napsal(a):
>> Hi Lukáš,
>>
>> On 07/24/2017 09:36 AM, Lukáš Doktor wrote:
>>> Dne 22.7.2017 v 03:30 Philippe Mathieu-Daudé napsal(a):
>>>> Hi Lukáš,
>>>>
>>>> Since comment/indent fixes and code changes are not related I'd rather see this split in at least 2 patches.
>>>>
>>> Hello Philippe, thank you for the review, I'm wondering what code changes you have in mind? This is commit should not bring any code changes, just code reorganization (like including the self.* attributes on top of the file)
>>>
>>>> On 07/20/2017 01:28 PM, Lukáš Doktor wrote:
>>>>> No actual code changes, just a few pylint/style fixes and docstring
>>>>> clarifications.
>>>>>
>>>>> Signed-off-by: Lukáš Doktor <ldoktor@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
>> [...]
>>>>>        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
>>
>> I was thinking about this line which is new. Now the declaration and initialization are done in __init__() while before it was only declared/initialized in connect() or accept().
>>
>> I'm not expert of python interpreter/jit internals but expect the generated code to be slightly different, even if achieving the same.
>>
>> not a bit deal, just about wording ;)
>>
> Well the difference is that before `connect` you get `AttributeError` when looking for `self.__sockfile` while with this patch you'll get `None`. In this code nobody queries for `self.__sockfile` before the `connect` so it should not change the behavior of this code so I consider that as a `style` fix as it's ugly to extend attributes later in code (with some exceptions like Namespace-objects..). Anyway if you insist I can split those patches.

I'm not insisting ;) Just add something like "also initialize
__sockfile to avoid AttributeError while introspecting object before
any call to connect/accept" in the commit message and that's fine to
me.