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

[Qemu-devel] [PATCH v3 00/14] 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

Changes v2->v3
 - Don't use repr in the 5th patch in MonitorResponseError

Daniel P. Berrange (1):
  qcow: fix memory leaks related to encryption

Kevin Wolf (1):
  qemu-iotests: Fix reference output for 186

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

Peter Maydell (1):
  Update version for v2.10.0-rc0 release

Vladimir Sementsov-Ogievskiy (1):
  qcow2-bitmap: fix bitmap_free

[Qemu-devel] [PATCH v3 01/14] qcow: fix memory leaks related to encryption

[Lukáš Doktor]

From: "Daniel P. Berrange" <berrange@redhat.com>

Fix leak of the 'encryptopts' string, which was mistakenly
declared const.

Fix leak of QemuOpts entry which should not have been deleted
from the opts array.

Reported by: coverity
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-id: 20170714103105.5781-1-berrange@redhat.com
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow.c  | 5 +++--
 block/qcow2.c | 7 ++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 66827d6..c08cdc4 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -768,7 +768,7 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
     Error *local_err = NULL;
     int ret;
     BlockBackend *qcow_blk;
-    const char *encryptfmt = NULL;
+    char *encryptfmt = NULL;
     QDict *options;
     QDict *encryptopts = NULL;
     QCryptoBlockCreateOptions *crypto_opts = NULL;
@@ -793,7 +793,7 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
             goto cleanup;
         }
     } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
-        encryptfmt = "aes";
+        encryptfmt = g_strdup("aes");
     }
 
     ret = bdrv_create_file(filename, opts, &local_err);
@@ -908,6 +908,7 @@ exit:
     blk_unref(qcow_blk);
 cleanup:
     QDECREF(encryptopts);
+    g_free(encryptfmt);
     qcrypto_block_free(crypto);
     qapi_free_QCryptoBlockCreateOptions(crypto_opts);
     g_free(backing_file);
diff --git a/block/qcow2.c b/block/qcow2.c
index 90efa44..d7c600b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2905,7 +2905,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     int version;
     uint64_t refcount_bits;
     int refcount_order;
-    const char *encryptfmt = NULL;
+    char *encryptfmt = NULL;
     Error *local_err = NULL;
     int ret;
 
@@ -2916,14 +2916,14 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     backing_fmt = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FMT);
     encryptfmt = qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT_FORMAT);
     if (encryptfmt) {
-        if (qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT)) {
+        if (qemu_opt_get(opts, BLOCK_OPT_ENCRYPT)) {
             error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
                        BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
             ret = -EINVAL;
             goto finish;
         }
     } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
-        encryptfmt = "aes";
+        encryptfmt = g_strdup("aes");
     }
     cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
     if (local_err) {
@@ -2983,6 +2983,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 finish:
     g_free(backing_file);
     g_free(backing_fmt);
+    g_free(encryptfmt);
     g_free(buf);
     return ret;
 }
-- 
2.9.4

[Qemu-devel] [PATCH v3 02/14] qcow2-bitmap: fix bitmap_free

[Lukáš Doktor]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Fix possible crash on error path in
qcow2_remove_persistent_dirty_bitmap. Although bitmap_free was added in
88ddffae8fc the bug was introduced later in commit 469c71edc72 (when
qcow2_remove_persistent_dirty_bitmap was added).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-id: 20170714123341.373857-1-vsementsov@virtuozzo.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-bitmap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3e8735a..e8d3bdb 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -487,6 +487,10 @@ static inline void bitmap_directory_to_be(uint8_t *dir, size_t size)
 
 static void bitmap_free(Qcow2Bitmap *bm)
 {
+    if (bm == NULL) {
+        return;
+    }
+
     g_free(bm->name);
     g_free(bm);
 }
-- 
2.9.4

[Qemu-devel] [PATCH v3 03/14] qemu-iotests: Fix reference output for 186

[Lukáš Doktor]

From: Kevin Wolf <kwolf@redhat.com>

Commits 70f17a1 ('error: Revert unwanted change of warning messages')
and e1824e5 ('qemu-iotests: Test 'info block'') had a semantic merge
conflict, which results in failure for qemu-iotests case 186. Fix the
reference output to consider the changes of 70f17a1.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-id: 1500973176-29235-1-git-send-email-kwolf@redhat.com
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/186.out | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/186.out b/tests/qemu-iotests/186.out
index b963b12..b8bf9a2 100644
--- a/tests/qemu-iotests/186.out
+++ b/tests/qemu-iotests/186.out
@@ -442,7 +442,7 @@ ide0-cd0 (NODE_NAME): null-co:// (null-co, read-only)
     Cache mode:       writeback
 (qemu) quit
 
-warning: qemu-system-x86_64: -drive if=scsi,driver=null-co: bus=0,unit=0 is deprecated with this machine type
+qemu-system-x86_64: -drive if=scsi,driver=null-co: warning: bus=0,unit=0 is deprecated with this machine type
 Testing: -drive if=scsi,driver=null-co
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) info block
@@ -451,7 +451,7 @@ scsi0-hd0 (NODE_NAME): null-co:// (null-co)
     Cache mode:       writeback
 (qemu) quit
 
-warning: qemu-system-x86_64: -drive if=scsi,media=cdrom: bus=0,unit=0 is deprecated with this machine type
+qemu-system-x86_64: -drive if=scsi,media=cdrom: warning: bus=0,unit=0 is deprecated with this machine type
 Testing: -drive if=scsi,media=cdrom
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) info block
@@ -460,7 +460,7 @@ scsi0-cd0: [not inserted]
     Removable device: not locked, tray closed
 (qemu) quit
 
-warning: qemu-system-x86_64: -drive if=scsi,driver=null-co,media=cdrom: bus=0,unit=0 is deprecated with this machine type
+qemu-system-x86_64: -drive if=scsi,driver=null-co,media=cdrom: warning: bus=0,unit=0 is deprecated with this machine type
 Testing: -drive if=scsi,driver=null-co,media=cdrom
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) info block
-- 
2.9.4

[Qemu-devel] [PATCH v3 04/14] Update version for v2.10.0-rc0 release

[Lukáš Doktor]

From: Peter Maydell <peter.maydell@linaro.org>

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 VERSION | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/VERSION b/VERSION
index 12ca1d1..a0700e6 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-2.9.50
+2.9.90
-- 
2.9.4

[Qemu-devel] [PATCH v3 05/14] 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 v3 06/14] 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>
Reviewed-by: John Snow <jsnow@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 v3 07/14] 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 v3 08/14] 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 v3 09/14] 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..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 v3 10/14] 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>
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

[Qemu-devel] [PATCH v3 11/14] 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 v3 12/14] 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 v3 13/14] 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/qmp/qmp.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 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 v3 14/14] 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>
Reviewed-by: John Snow <jsnow@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 v3 00/14] qemu.py: Pylint/style fixes

[Lukáš Doktor]

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

Dne 26.7.2017 v 16:35 Lukáš Doktor napsal(a):
> 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
> 
> Changes v2->v3
>  - Don't use repr in the 5th patch in MonitorResponseError
> 
> Daniel P. Berrange (1):
>   qcow: fix memory leaks related to encryption
> 
> Kevin Wolf (1):
>   qemu-iotests: Fix reference output for 186
> 
> 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
> 
> Peter Maydell (1):
>   Update version for v2.10.0-rc0 release
> 
> Vladimir Sementsov-Ogievskiy (1):
>   qcow2-bitmap: fix bitmap_free
> 

Oups, I'm sorry I based it on a wrong master... I'll send v4 without those.

Lukáš


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