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

[Qemu-devel] [PATCH v2 00/10] 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áš

Changes v1->v2
 - Squashed 2nd and 10th patches into 2nd one
 - Use repr() in MonitorResponseError's description
 - Improved commit message of the 6th patch
 - Two tweaks to docstrings changed in the 6th patch
 - Also updated qmp-shell to use new-style super calls (7th patch)
 - Fixed the accidental rename of qmp `cmd_id` (kept the id->cmd_id patch)
 - Changed the style of the style-fix in the 10th commit

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       | 98 +++++++++++++++++++++++++++++++++++++--------------
 scripts/qmp/qmp-shell |  4 +--
 scripts/qmp/qmp.py    | 49 ++++++++++++++++----------
 scripts/qtest.py      | 13 ++++---
 4 files changed, 109 insertions(+), 55 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH v2 01/10] 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 v2 02/10] qemu|qtest: 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. The same issue applies in
inherited qtest module.

Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
Reviewed-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 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:
diff --git a/scripts/qtest.py b/scripts/qtest.py
index d5aecb5..ab183c0 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.9.4

[Qemu-devel] [PATCH v2 03/10] 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>
Reviewed-by: Eduardo Habkost <ehabkost@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)
 
-- 
2.9.4

[Qemu-devel] [PATCH v2 04/10] 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>
Reviewed-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 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 v2 05/10] 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>
Reviewed-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 5948e19..e6df54c 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__(repr(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 v2 06/10] qmp.py: Couple of pylint/style fixes

[Lukáš Doktor]

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>
---
 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..782d1ac 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.9.4

[Qemu-devel] [PATCH v2 07/10] 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qmp/qmp-shell | 4 ++--
 scripts/qmp/qmp.py    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index 860ffb2..be449de 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!'):
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 782d1ac..95ff5cc 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 v2 08/10] 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>
Reviewed-by: Eduardo Habkost <ehabkost@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 95ff5cc..f2f5a9b 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.9.4

[Qemu-devel] [PATCH v2 09/10] 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 f2f5a9b..ef12e8a 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.9.4

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

diff --git a/scripts/qtest.py b/scripts/qtest.py
index ab183c0..df0daf2 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.9.4

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

[Philippe Mathieu-Daudé]

On Tue, Jul 25, 2017 at 12:09 PM, Lukáš Doktor <ldoktor@redhat.com> wrote:
> 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>

> ---
>  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..782d1ac 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.9.4
>

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

[Eduardo Habkost]

On Tue, Jul 25, 2017 at 05:09:46PM +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>
> Reviewed-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 5948e19..e6df54c 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__(repr(desc))

This will end up calling Exception.__init__.  I previously
suggested repr(desc) above(*) because I didn't know what happened
when the Exception.__init__ argument is not a string.

I couldn't find any documentation on the right argument types for
Exception.__init__.  The examples in the Python documentation[1]
don't call Exception.__init__ at all: they simply implement
__str__().

However, based on my testing and on the BaseException
documentation[2], I believe repr() isn't really necessary:

"If str() or unicode() is called on an instance of this class,
the representation of the argument(s) to the instance are
returned, or the empty string when there were no arguments."

So your previous version was good, already.

But maybe we could do this instead, to make the constructor as
simple as possible:

class MonitorResponseError(qmp.qmp.QMPError):
    def __init__(self, reply):
        self.reply = reply

    def __str__(self):
        return self.reply.get('error', {}).get('desc', repr(self.reply))


[1] https://docs.python.org/2/tutorial/errors.html#user-defined-exceptions
[2] https://docs.python.org/2/library/exceptions.html#exceptions.BaseException

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 02/10] qemu|qtest: Avoid dangerous arguments

[John Snow]

On 07/25/2017 11:09 AM, 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. The same issue applies in
> inherited qtest module.
> 

ohh, grr, this trips me up in Python all the time. Definitely best to 
remove such usages where possible.

> Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.com>

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

[John Snow]

On 07/25/2017 11:09 AM, Lukáš Doktor wrote:
> No actual code changes, just few pylint/style fixes.
> 

More than welcome, given the state of our python scripts.

> Signed-off-by: Lukáš Doktor <ldoktor@redhat.com>
> ---
>   scripts/qtest.py | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/qtest.py b/scripts/qtest.py
> index ab183c0..df0daf2 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):
> 

Reviewed-by: John Snow <jsnow@redhat.com>

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

[Eduardo Habkost]

On Tue, Jul 25, 2017 at 05:09:50PM +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>

I don't see the benefit of the patch, as the function is very
short and unlikely to ever use the id() builtin.  But I am not
against it if it will silence code analyzer warnings.


> ---
>  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 f2f5a9b..ef12e8a 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.9.4
> 

-- 
Eduardo

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

[Lukáš Doktor]

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

Dne 25.7.2017 v 18:06 Eduardo Habkost napsal(a):
> On Tue, Jul 25, 2017 at 05:09:46PM +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>
>> Reviewed-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 5948e19..e6df54c 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__(repr(desc))
> 
> This will end up calling Exception.__init__.  I previously
> suggested repr(desc) above(*) because I didn't know what happened
> when the Exception.__init__ argument is not a string.
> 
> I couldn't find any documentation on the right argument types for
> Exception.__init__.  The examples in the Python documentation[1]
> don't call Exception.__init__ at all: they simply implement
> __str__().
> 
> However, based on my testing and on the BaseException
> documentation[2], I believe repr() isn't really necessary:
> 
> "If str() or unicode() is called on an instance of this class,
> the representation of the argument(s) to the instance are
> returned, or the empty string when there were no arguments."
> 
> So your previous version was good, already.
> 
> But maybe we could do this instead, to make the constructor as
> simple as possible:
> 
> class MonitorResponseError(qmp.qmp.QMPError):
>     def __init__(self, reply):
>         self.reply = reply
> 
>     def __str__(self):
>         return self.reply.get('error', {}).get('desc', repr(self.reply))
> 
Well I know I can implement custom `__str__` class, but the default one simply stores the argument as string and reports it, so for simple strings I usually go with super call and with complex ones with custom `__str__`. Anyway if this suits this project style better I'll change it in v2.

Lukáš

> 
> [1] https://docs.python.org/2/tutorial/errors.html#user-defined-exceptions
> [2] https://docs.python.org/2/library/exceptions.html#exceptions.BaseException
> 
>> +        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
>>
> 



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

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

[Lukáš Doktor]

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

Dne 25.7.2017 v 21:34 Eduardo Habkost napsal(a):
> On Tue, Jul 25, 2017 at 05:09:50PM +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>
> 
> I don't see the benefit of the patch, as the function is very
> short and unlikely to ever use the id() builtin.  But I am not
> against it if it will silence code analyzer warnings.
> 
For me the biggest benefit is it decreases the probability of someone copying the same "idea" to other pieces of code, because, you know, developers are using copy&paste way too often.

Anyway pylint does complain about this issue. I can either ignore it, skip it with an informative comment `# use id for backward compatibility pylint: disable=W0622` or use this patch to fix it properly. I'm inclined towards fixing it (until it poisons other parts of the code), you are in favor of ignoring it, so let's see whether someone else has another opinion, otherwise I'll remove it in v3.

Lukáš

> 
>> ---
>>  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 f2f5a9b..ef12e8a 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.9.4
>>
> 



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

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

[Eduardo Habkost]

On Wed, Jul 26, 2017 at 06:39:28AM +0200, Lukáš Doktor wrote:
> Dne 25.7.2017 v 18:06 Eduardo Habkost napsal(a):
> > On Tue, Jul 25, 2017 at 05:09:46PM +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>
> >> Reviewed-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 5948e19..e6df54c 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__(repr(desc))
> > 
> > This will end up calling Exception.__init__.  I previously
> > suggested repr(desc) above(*) because I didn't know what happened
> > when the Exception.__init__ argument is not a string.
> > 
> > I couldn't find any documentation on the right argument types for
> > Exception.__init__.  The examples in the Python documentation[1]
> > don't call Exception.__init__ at all: they simply implement
> > __str__().
> > 
> > However, based on my testing and on the BaseException
> > documentation[2], I believe repr() isn't really necessary:
> > 
> > "If str() or unicode() is called on an instance of this class,
> > the representation of the argument(s) to the instance are
> > returned, or the empty string when there were no arguments."
> > 
> > So your previous version was good, already.
> > 
> > But maybe we could do this instead, to make the constructor as
> > simple as possible:
> > 
> > class MonitorResponseError(qmp.qmp.QMPError):
> >     def __init__(self, reply):
> >         self.reply = reply
> > 
> >     def __str__(self):
> >         return self.reply.get('error', {}).get('desc', repr(self.reply))
> > 
> Well I know I can implement custom `__str__` class, but the
> default one simply stores the argument as string and reports
> it, so for simple strings I usually go with super call and with
> complex ones with custom `__str__`. Anyway if this suits this
> project style better I'll change it in v2.

If you prefer it, your previous version (without repr) was good
enough to me.

-- 
Eduardo

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

[Eduardo Habkost]

On Wed, Jul 26, 2017 at 06:46:30AM +0200, Lukáš Doktor wrote:
> Dne 25.7.2017 v 21:34 Eduardo Habkost napsal(a):
> > On Tue, Jul 25, 2017 at 05:09:50PM +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>
> > 
> > I don't see the benefit of the patch, as the function is very
> > short and unlikely to ever use the id() builtin.  But I am not
> > against it if it will silence code analyzer warnings.
> > 
> For me the biggest benefit is it decreases the probability of
> someone copying the same "idea" to other pieces of code,
> because, you know, developers are using copy&paste way too
> often.
> 
> Anyway pylint does complain about this issue. I can either
> ignore it, skip it with an informative comment `# use id for
> backward compatibility pylint: disable=W0622` or use this patch
> to fix it properly. I'm inclined towards fixing it (until it
> poisons other parts of the code), you are in favor of ignoring
> it, so let's see whether someone else has another opinion,
> otherwise I'll remove it in v3.

If pylint complains about it, it won't hurt to make it happy.

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

-- 
Eduardo