* [Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes
@ 2017-08-15 8:57 Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 01/10] " Lukáš Doktor
` (9 more replies)
0 siblings, 10 replies; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-15 8:57 UTC (permalink / raw)
To: qemu-devel; +Cc: ldoktor, famz, ehabkost, apahim, armbru, mreitz, jsnow, f4bug
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 in 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 in v3
- Don't use repr in the 5th patch in MonitorResponseError
Changes in v4
- Use correct git base (remove unwanted commits)
Changes in v5
- Avoid bool comparison
- Change report to return in one docstring
- Removed the unnecessary spaces around single-line docstring
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
2017-08-15 8:57 [Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes Lukáš Doktor
@ 2017-08-15 8:57 ` Lukáš Doktor
2017-08-15 11:55 ` Stefan Hajnoczi
2017-08-15 12:31 ` Markus Armbruster
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 02/10] qemu|qtest: Avoid dangerous arguments Lukáš Doktor
` (8 subsequent siblings)
9 siblings, 2 replies; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-15 8:57 UTC (permalink / raw)
To: qemu-devel; +Cc: ldoktor, famz, ehabkost, apahim, armbru, mreitz, jsnow, f4bug
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..466aaab 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 not os.path.exists(self._socket_scm_helper):
print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
return -1
fd_param = ["%s" % self._socket_scm_helper,
"%d" % self._qmp.get_sock_fd(),
"%s" % fd_file_path]
devnull = open('/dev/null', 'rb')
- p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
- stderr=sys.stderr)
- return p.wait()
+ proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
+ stderr=sys.stderr)
+ return proc.wait()
@staticmethod
def _remove_if_exists(path):
@@ -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 return 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v5 02/10] qemu|qtest: Avoid dangerous arguments
2017-08-15 8:57 [Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 01/10] " Lukáš Doktor
@ 2017-08-15 8:57 ` Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 03/10] qemu.py: Use iteritems rather than keys() Lukáš Doktor
` (7 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-15 8:57 UTC (permalink / raw)
To: qemu-devel; +Cc: ldoktor, famz, ehabkost, apahim, armbru, mreitz, jsnow, f4bug
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 466aaab..888f5b3 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v5 03/10] qemu.py: Use iteritems rather than keys()
2017-08-15 8:57 [Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 01/10] " Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 02/10] qemu|qtest: Avoid dangerous arguments Lukáš Doktor
@ 2017-08-15 8:57 ` Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 04/10] qemu.py: Simplify QMP key-conversion Lukáš Doktor
` (6 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-15 8:57 UTC (permalink / raw)
To: qemu-devel; +Cc: ldoktor, famz, ehabkost, apahim, armbru, mreitz, jsnow, f4bug
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 888f5b3..571f852 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v5 04/10] qemu.py: Simplify QMP key-conversion
2017-08-15 8:57 [Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes Lukáš Doktor
` (2 preceding siblings ...)
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 03/10] qemu.py: Use iteritems rather than keys() Lukáš Doktor
@ 2017-08-15 8:57 ` Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 05/10] qemu.py: Use custom exceptions rather than Exception Lukáš Doktor
` (5 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-15 8:57 UTC (permalink / raw)
To: qemu-devel; +Cc: ldoktor, famz, ehabkost, apahim, armbru, mreitz, jsnow, f4bug
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 571f852..5ba17d0 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v5 05/10] qemu.py: Use custom exceptions rather than Exception
2017-08-15 8:57 [Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes Lukáš Doktor
` (3 preceding siblings ...)
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 04/10] qemu.py: Simplify QMP key-conversion Lukáš Doktor
@ 2017-08-15 8:57 ` Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 06/10] qmp.py: Couple of pylint/style fixes Lukáš Doktor
` (4 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-15 8:57 UTC (permalink / raw)
To: qemu-devel; +Cc: ldoktor, famz, ehabkost, apahim, armbru, mreitz, jsnow, f4bug
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 5ba17d0..0c00e91 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v5 06/10] qmp.py: Couple of pylint/style fixes
2017-08-15 8:57 [Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes Lukáš Doktor
` (4 preceding siblings ...)
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 05/10] qemu.py: Use custom exceptions rather than Exception Lukáš Doktor
@ 2017-08-15 8:57 ` Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 07/10] qmp.py: Use object-based class for QEMUMonitorProtocol Lukáš Doktor
` (3 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-15 8:57 UTC (permalink / raw)
To: qemu-devel; +Cc: ldoktor, famz, ehabkost, apahim, armbru, mreitz, jsnow, f4bug
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v5 07/10] qmp.py: Use object-based class for QEMUMonitorProtocol
2017-08-15 8:57 [Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes Lukáš Doktor
` (5 preceding siblings ...)
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 06/10] qmp.py: Couple of pylint/style fixes Lukáš Doktor
@ 2017-08-15 8:57 ` Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 08/10] qmp.py: Avoid "has_key" usage Lukáš Doktor
` (2 subsequent siblings)
9 siblings, 0 replies; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-15 8:57 UTC (permalink / raw)
To: qemu-devel; +Cc: ldoktor, famz, ehabkost, apahim, armbru, mreitz, jsnow, f4bug
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v5 08/10] qmp.py: Avoid "has_key" usage
2017-08-15 8:57 [Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes Lukáš Doktor
` (6 preceding siblings ...)
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 07/10] qmp.py: Use object-based class for QEMUMonitorProtocol Lukáš Doktor
@ 2017-08-15 8:57 ` Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 09/10] qmp.py: Avoid overriding a builtin object Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 10/10] qtest.py: Few pylint/style fixes Lukáš Doktor
9 siblings, 0 replies; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-15 8:57 UTC (permalink / raw)
To: qemu-devel; +Cc: ldoktor, famz, ehabkost, apahim, armbru, mreitz, jsnow, f4bug
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v5 09/10] qmp.py: Avoid overriding a builtin object
2017-08-15 8:57 [Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes Lukáš Doktor
` (7 preceding siblings ...)
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 08/10] qmp.py: Avoid "has_key" usage Lukáš Doktor
@ 2017-08-15 8:57 ` Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 10/10] qtest.py: Few pylint/style fixes Lukáš Doktor
9 siblings, 0 replies; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-15 8:57 UTC (permalink / raw)
To: qemu-devel; +Cc: ldoktor, famz, ehabkost, apahim, armbru, mreitz, jsnow, f4bug
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH v5 10/10] qtest.py: Few pylint/style fixes
2017-08-15 8:57 [Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes Lukáš Doktor
` (8 preceding siblings ...)
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 09/10] qmp.py: Avoid overriding a builtin object Lukáš Doktor
@ 2017-08-15 8:57 ` Lukáš Doktor
9 siblings, 0 replies; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-15 8:57 UTC (permalink / raw)
To: qemu-devel; +Cc: ldoktor, famz, ehabkost, apahim, armbru, mreitz, jsnow, f4bug
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 01/10] " Lukáš Doktor
@ 2017-08-15 11:55 ` Stefan Hajnoczi
2017-08-15 12:31 ` Markus Armbruster
1 sibling, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2017-08-15 11:55 UTC (permalink / raw)
To: Lukáš Doktor
Cc: qemu-devel, famz, ehabkost, apahim, armbru, mreitz, jsnow, f4bug
[-- Attachment #1: Type: text/plain, Size: 413 bytes --]
On Tue, Aug 15, 2017 at 10:57:23AM +0200, Lukáš Doktor wrote:
> 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(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 01/10] " Lukáš Doktor
2017-08-15 11:55 ` Stefan Hajnoczi
@ 2017-08-15 12:31 ` Markus Armbruster
2017-08-16 16:01 ` Lukáš Doktor
1 sibling, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2017-08-15 12:31 UTC (permalink / raw)
To: Lukáš Doktor
Cc: qemu-devel, famz, ehabkost, apahim, mreitz, jsnow, f4bug
Lukáš Doktor <ldoktor@redhat.com> writes:
> 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..466aaab 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
Initialize a QEMUMachine
Rationale: it's __init__, not __create__, and "object" is redundant.
> +
> + @param binary: path to the qemu binary (str)
Drop (str), because what else could it be?
> + @param args: initial list of extra arguments
If this is the initial list, then what's the final list?
> + @param wrapper: list of arguments used as prefix to qemu binary
> + @param name: name of this object (used for log/monitor/... file names)
prefix for socket and log file names (default: qemu-PID)
> + @param test_dir: base location to put log/monitor/... files in
where to create socket and log file
Aside: test_dir is a lousy name.
> + @param monitor_address: custom address for QMP monitor
Yes, but what *is* a custom address? Its user _base_args() appears to
expect either a pair of strings (host, port) or a string filename.
> + @param socket_scm_helper: path to scm_helper binary (to forward fds)
What is an scm_helper, and why would I want to use it?
> + @param debug: enable debug mode (forwarded to QMP helper and such)
What is a QMP helper? To what else is debug forwarded?
> + @note: Qemu process is not started until launch() is used.
until launch().
> + '''
It's an improvement.
> 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 not os.path.exists(self._socket_scm_helper):
> print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
> return -1
> fd_param = ["%s" % self._socket_scm_helper,
> "%d" % self._qmp.get_sock_fd(),
> "%s" % fd_file_path]
> devnull = open('/dev/null', 'rb')
> - p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
> - stderr=sys.stderr)
> - return p.wait()
> + proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
> + stderr=sys.stderr)
> + return proc.wait()
>
> @staticmethod
> def _remove_if_exists(path):
> @@ -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)
Recommend to break the line between the last two arguments.
>
> 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
The bare except is almost certainly inappropriate. Are you sure we
should silence pylint?
Note that there's another bare except in launch(), visible in the
previous patch hunk. I guess pylint is okay with that one because it
re-throws the exception.
In case we should silence pylint: what's the scope of this magic pylint
comment? Can we use the symbolic disable=bare-except?
> 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'''
Make that "and return the response", because that's how
docs/interop/qmp-spec.txt calls the thing.
> 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 return result dict or on failure
> + raise exception with details.
> + '''
I'm afraid "result dict" is wrong: it need not be a dict. "on failure
raise exception" adds precious little information, and "with details"
adds none.
Perhaps:
Invoke a QMP command.
On success return the return value.
On failure raise an exception.
The last sentence is too vague, but it's hard to do better because...
> reply = self.qmp(cmd, conv_keys, **args)
> if reply is None:
> raise Exception("Monitor is closed")
... we raise Exception! This code is a *turd* (pardon my french), and
PEP8 / pylint violations are the least of its problems.
> @@ -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.'''
What is match?
name and timeout not worth mentioning?
> + # Test if 'match' is a recursive subset of 'event'; skips branch
> + # processing on match's value `None`
What's a "recursive subset"? What's "branch processing"?
There's an unusual set of quotes around `None`.
> + # {"foo": {"bar": 1} matches {"foo": None}
Aha: None servers as wildcard.
> + def _event_match(event, match=None):
Any particular reason for renaming this function?
> 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
Comma after execution, please.
> + of the qemu process.
> + '''
> return self._iolog
I understand this code was factored out of qemu-iotests for use by
qtest.py. That's okay, but I'd rather not serve as its maintainer.
-E2MUCH...
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
2017-08-15 12:31 ` Markus Armbruster
@ 2017-08-16 16:01 ` Lukáš Doktor
2017-08-16 16:58 ` Markus Armbruster
0 siblings, 1 reply; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-16 16:01 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, famz, ehabkost, apahim, mreitz, jsnow, f4bug
[-- Attachment #1: Type: text/plain, Size: 14415 bytes --]
Dne 15.8.2017 v 14:31 Markus Armbruster napsal(a):
> Lukáš Doktor <ldoktor@redhat.com> writes:
>
>> 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..466aaab 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
>
> Initialize a QEMUMachine
>
> Rationale: it's __init__, not __create__, and "object" is redundant.
>
sure
>> +
>> + @param binary: path to the qemu binary (str)
>
> Drop (str), because what else could it be?
it could be shlex.split of arguments to be passed to process. Anyway no strong opinion here so I dropping it...
>
>> + @param args: initial list of extra arguments
>
> If this is the initial list, then what's the final list?
>
It's the basic set of arguments which can be modified before the execution. Do you think it requires additional explanation, or would you like to improve it somehow?
>> + @param wrapper: list of arguments used as prefix to qemu binary
>> + @param name: name of this object (used for log/monitor/... file names)
> prefix for socket and log file names (default: qemu-PID)
>
Sure, both make sense to me.
>> + @param test_dir: base location to put log/monitor/... files in
>
> where to create socket and log file
>
> Aside: test_dir is a lousy name.
Agree but changing names is tricky as people might be using kwargs to set it. Anyway using your description here, keeping the possible rename for a separate patchset (if needed).
>
>> + @param monitor_address: custom address for QMP monitor
>
> Yes, but what *is* a custom address? Its user _base_args() appears to
> expect either a pair of strings (host, port) or a string filename.
>
If you insist I can add something like "a tuple(host, port) or string to specify path", but I find it unnecessary detailed...
>> + @param socket_scm_helper: path to scm_helper binary (to forward fds)
>
> What is an scm_helper, and why would I want to use it?
>
To forward a file descriptor. It's for example used in tests/qemu-iotests/045 or tests/qemu-iotests/147
>> + @param debug: enable debug mode (forwarded to QMP helper and such)
>
> What is a QMP helper? To what else is debug forwarded?
>
Debug is set in `self._debug` and can be consumed by anyone who has access to this variable. Currently that is the QMP, but people can inherit and use that variable to adjust their behavior.
>> + @note: Qemu process is not started until launch() is used.
>
> until launch().
>
OK
>> + '''
>
> It's an improvement.
>
>> 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 not os.path.exists(self._socket_scm_helper):
>> print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
>> return -1
>> fd_param = ["%s" % self._socket_scm_helper,
>> "%d" % self._qmp.get_sock_fd(),
>> "%s" % fd_file_path]
>> devnull = open('/dev/null', 'rb')
>> - p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
>> - stderr=sys.stderr)
>> - return p.wait()
>> + proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
>> + stderr=sys.stderr)
>> + return proc.wait()
>>
>> @staticmethod
>> def _remove_if_exists(path):
>> @@ -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)
>
> Recommend to break the line between the last two arguments.
>
I'm not used to do that, but I can most certainly do that. Do you want me to change all places (eg. in the next chunk)
>>
>> 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
>
> The bare except is almost certainly inappropriate. Are you sure we
> should silence pylint?
>
> Note that there's another bare except in launch(), visible in the
> previous patch hunk. I guess pylint is okay with that one because it
> re-throws the exception.
>
> In case we should silence pylint: what's the scope of this magic pylint
> comment? Can we use the symbolic disable=bare-except?
>
Yep, we can use symbolic name as well. Well more appropriate would be to catch `SystemExit` and `KeyboardInterrupt`, treat them and raise them and then ignore the rest of exceptions. I'll do that in the next version...
>> 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'''
>
> Make that "and return the response", because that's how
> docs/interop/qmp-spec.txt calls the thing.
>
Sure, no problem. I wanted to stress-out it's a dict and not encoded string, but it's probably given by the context.
>> 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 return result dict or on failure
>> + raise exception with details.
>> + '''
>
> I'm afraid "result dict" is wrong: it need not be a dict. "on failure
> raise exception" adds precious little information, and "with details"
> adds none.
>
> Perhaps:
>
> Invoke a QMP command.
> On success return the return value.
> On failure raise an exception.
>
That is quite more verbose than I'd like and result in the same (for non-native speaker), but I have no strong opinion here so changing it to your version in v2.
> The last sentence is too vague, but it's hard to do better because...
>
>> reply = self.qmp(cmd, conv_keys, **args)
>> if reply is None:
>> raise Exception("Monitor is closed")
>
> ... we raise Exception! This code is a *turd* (pardon my french), and
> PEP8 / pylint violations are the least of its problems.
>
Yep, adding rework-exceptions to my TODO list (for a future patchset)
>> @@ -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.'''
>
> What is match?
>
> name and timeout not worth mentioning?
>
IMO this does not require full-blown documentation, it's not really a user-facing API and sometimes shorter is better. Anyway if you insist I can add full documentation of each parameter. I can even add `:type:` and `:returns:` and perhaps some `:warns:` and `:notes:` and I should descend and note all the possible `:raises:`.
... the point I'm trying to make is I don't think it's necessary to go that much into details. Anyway if you think the params are necessary to list, then I can do that.
>> + # Test if 'match' is a recursive subset of 'event'; skips branch
>> + # processing on match's value `None`
>
> What's a "recursive subset"? What's "branch processing"?
>
> There's an unusual set of quotes around `None`.
>
Basically I was surprised why this function exists as one can directly compare dicts. It's because it works as the stock dict compare only on value "None" it breaks and assumes that "branch" matches. That's why I listed the example as I'm unsure how to explain it in a human language.
As for the `None`, in sphinx/doxygen it becomes "block" so I'm used to these, anyway people use `'`s and `"`s here so I'll change it in next version to be consistent.
>> + # {"foo": {"bar": 1} matches {"foo": None}
>
> Aha: None servers as wildcard.
Exactly.
>
>> + def _event_match(event, match=None):
>
> Any particular reason for renaming this function?
>
To emphasize it's internal, not a strong opinion but IMO looks better this way.
>> 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
>
> Comma after execution, please.
>
OK
>> + of the qemu process.
>> + '''
>> return self._iolog
>
> I understand this code was factored out of qemu-iotests for use by
> qtest.py. That's okay, but I'd rather not serve as its maintainer.
> -E2MUCH...
>
Yep, anyway it contains several useful methods/helpers and fixing basic issues is the simplest way to get to know the code (and the submitting process). Thank you for your time.
Regards,
Lukáš
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 502 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
2017-08-16 16:01 ` Lukáš Doktor
@ 2017-08-16 16:58 ` Markus Armbruster
2017-08-16 17:48 ` Lukáš Doktor
0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2017-08-16 16:58 UTC (permalink / raw)
To: Lukáš Doktor
Cc: famz, ehabkost, apahim, qemu-devel, mreitz, jsnow, f4bug
Lukáš Doktor <ldoktor@redhat.com> writes:
> Dne 15.8.2017 v 14:31 Markus Armbruster napsal(a):
>> Lukáš Doktor <ldoktor@redhat.com> writes:
>>
>>> 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..466aaab 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
>>
>> Initialize a QEMUMachine
>>
>> Rationale: it's __init__, not __create__, and "object" is redundant.
>>
>
> sure
>
>>> +
>>> + @param binary: path to the qemu binary (str)
>>
>> Drop (str), because what else could it be?
>
> it could be shlex.split of arguments to be passed to process. Anyway no strong opinion here so I dropping it...
>
>>
>>> + @param args: initial list of extra arguments
>>
>> If this is the initial list, then what's the final list?
>>
>
> It's the basic set of arguments which can be modified before the execution. Do you think it requires additional explanation, or would you like to improve it somehow?
Can this list of extra arguments really be *modified*? Adding more
arguments doesn't count for me --- I'd consider them added to the
"non-extra" arguments.
Drop "initial"?
>>> + @param wrapper: list of arguments used as prefix to qemu binary
>>> + @param name: name of this object (used for log/monitor/... file names)
>> prefix for socket and log file names (default: qemu-PID)
>>
>
> Sure, both make sense to me.
>
>>> + @param test_dir: base location to put log/monitor/... files in
>>
>> where to create socket and log file
>>
>> Aside: test_dir is a lousy name.
>
> Agree but changing names is tricky as people might be using kwargs to set it. Anyway using your description here, keeping the possible rename for a separate patchset (if needed).
I'm merely observing the lousiness of this name. I'm not asking you to
do anything about it :)
>>> + @param monitor_address: custom address for QMP monitor
>>
>> Yes, but what *is* a custom address? Its user _base_args() appears to
>> expect either a pair of strings (host, port) or a string filename.
>>
>
> If you insist I can add something like "a tuple(host, port) or string to specify path", but I find it unnecessary detailed...
I'm not the maintainer, I'm definitely not insisting on anything.
If you're aiming for brevity, then drop "custom".
>>> + @param socket_scm_helper: path to scm_helper binary (to forward fds)
>>
>> What is an scm_helper, and why would I want to use it?
>>
>
> To forward a file descriptor. It's for example used in tests/qemu-iotests/045 or tests/qemu-iotests/147
What about "socket_scm_helper: helper program, required for send_fd_scm()"?
>>> + @param debug: enable debug mode (forwarded to QMP helper and such)
>>
>> What is a QMP helper? To what else is debug forwarded?
>>
>
> Debug is set in `self._debug` and can be consumed by anyone who has access to this variable. Currently that is the QMP, but people can inherit and use that variable to adjust their behavior.
Drop the parenthesis?
>>> + @note: Qemu process is not started until launch() is used.
>>
>> until launch().
>>
>
> OK
One more thing: what the use of "@param"?
>>> + '''
>>
>> It's an improvement.
>>
>>> 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 not os.path.exists(self._socket_scm_helper):
>>> print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
>>> return -1
>>> fd_param = ["%s" % self._socket_scm_helper,
>>> "%d" % self._qmp.get_sock_fd(),
>>> "%s" % fd_file_path]
>>> devnull = open('/dev/null', 'rb')
>>> - p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
>>> - stderr=sys.stderr)
>>> - return p.wait()
>>> + proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
>>> + stderr=sys.stderr)
>>> + return proc.wait()
>>>
>>> @staticmethod
>>> def _remove_if_exists(path):
>>> @@ -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)
>>
>> Recommend to break the line between the last two arguments.
>>
>
> I'm not used to do that, but I can most certainly do that. Do you want me to change all places (eg. in the next chunk)
PEP8 asks you to limit all lines to a maximum of 79 characters. This
one is right at the maximum. Tolerable, but I prefer my lines a bit
shorter. Use your judgement.
>>>
>>> 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
>>
>> The bare except is almost certainly inappropriate. Are you sure we
>> should silence pylint?
>>
>> Note that there's another bare except in launch(), visible in the
>> previous patch hunk. I guess pylint is okay with that one because it
>> re-throws the exception.
>>
>> In case we should silence pylint: what's the scope of this magic pylint
>> comment? Can we use the symbolic disable=bare-except?
>>
>
> Yep, we can use symbolic name as well. Well more appropriate would be to catch `SystemExit` and `KeyboardInterrupt`, treat them and raise them and then ignore the rest of exceptions. I'll do that in the next version...
>
>>> 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'''
>>
>> Make that "and return the response", because that's how
>> docs/interop/qmp-spec.txt calls the thing.
>>
>
> Sure, no problem. I wanted to stress-out it's a dict and not encoded string, but it's probably given by the context.
"return the response dict" would be fine with me.
>>> 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 return result dict or on failure
>>> + raise exception with details.
>>> + '''
>>
>> I'm afraid "result dict" is wrong: it need not be a dict. "on failure
>> raise exception" adds precious little information, and "with details"
>> adds none.
>>
>> Perhaps:
>>
>> Invoke a QMP command.
>> On success return the return value.
>> On failure raise an exception.
>>
>
> That is quite more verbose than I'd like and result in the same (for non-native speaker), but I have no strong opinion here so changing it to your version in v2.
>
>> The last sentence is too vague, but it's hard to do better because...
>>
>>> reply = self.qmp(cmd, conv_keys, **args)
>>> if reply is None:
>>> raise Exception("Monitor is closed")
>>
>> ... we raise Exception! This code is a *turd* (pardon my french), and
>> PEP8 / pylint violations are the least of its problems.
>>
>
> Yep, adding rework-exceptions to my TODO list (for a future patchset)
>
>>> @@ -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.'''
>>
>> What is match?
>>
>> name and timeout not worth mentioning?
>>
>
> IMO this does not require full-blown documentation, it's not really a user-facing API and sometimes shorter is better. Anyway if you insist I can add full documentation of each parameter. I can even add `:type:` and `:returns:` and perhaps some `:warns:` and `:notes:` and I should descend and note all the possible `:raises:`.
>
> ... the point I'm trying to make is I don't think it's necessary to go that much into details. Anyway if you think the params are necessary to list, then I can do that.
Use your judgement. The more detailed comments you add, the more
questions you'll get ;)
>>> + # Test if 'match' is a recursive subset of 'event'; skips branch
>>> + # processing on match's value `None`
>>
>> What's a "recursive subset"? What's "branch processing"?
>>
>> There's an unusual set of quotes around `None`.
>>
>
> Basically I was surprised why this function exists as one can directly compare dicts. It's because it works as the stock dict compare only on value "None" it breaks and assumes that "branch" matches. That's why I listed the example as I'm unsure how to explain it in a human language.
>
> As for the `None`, in sphinx/doxygen it becomes "block" so I'm used to these, anyway people use `'`s and `"`s here so I'll change it in next version to be consistent.
According to git-grep, we're using neither sphinx nor doxygen right now.
>>> + # {"foo": {"bar": 1} matches {"foo": None}
>>
>> Aha: None servers as wildcard.
>
> Exactly.
>
>>
>>> + def _event_match(event, match=None):
>>
>> Any particular reason for renaming this function?
>>
>
> To emphasize it's internal, not a strong opinion but IMO looks better this way.
It's a nested function, how could it *not* be internal?
>>> 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
>>
>> Comma after execution, please.
>>
>
> OK
>
>>> + of the qemu process.
>>> + '''
>>> return self._iolog
>>
>> I understand this code was factored out of qemu-iotests for use by
>> qtest.py. That's okay, but I'd rather not serve as its maintainer.
>> -E2MUCH...
>>
>
> Yep, anyway it contains several useful methods/helpers and fixing basic issues is the simplest way to get to know the code (and the submitting process). Thank you for your time.
You're welcome!
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
2017-08-16 16:58 ` Markus Armbruster
@ 2017-08-16 17:48 ` Lukáš Doktor
2017-08-17 5:24 ` Markus Armbruster
0 siblings, 1 reply; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-16 17:48 UTC (permalink / raw)
To: Markus Armbruster
Cc: famz, ehabkost, apahim, qemu-devel, mreitz, jsnow, f4bug
[-- Attachment #1: Type: text/plain, Size: 17149 bytes --]
Dne 16.8.2017 v 18:58 Markus Armbruster napsal(a):
> Lukáš Doktor <ldoktor@redhat.com> writes:
>
>> Dne 15.8.2017 v 14:31 Markus Armbruster napsal(a):
>>> Lukáš Doktor <ldoktor@redhat.com> writes:
>>>
>>>> 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..466aaab 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
>>>
>>> Initialize a QEMUMachine
>>>
>>> Rationale: it's __init__, not __create__, and "object" is redundant.
>>>
>>
>> sure
>>
>>>> +
>>>> + @param binary: path to the qemu binary (str)
>>>
>>> Drop (str), because what else could it be?
>>
>> it could be shlex.split of arguments to be passed to process. Anyway no strong opinion here so I dropping it...
>>
>>>
>>>> + @param args: initial list of extra arguments
>>>
>>> If this is the initial list, then what's the final list?
>>>
>>
>> It's the basic set of arguments which can be modified before the execution. Do you think it requires additional explanation, or would you like to improve it somehow?
>
> Can this list of extra arguments really be *modified*? Adding more
> arguments doesn't count for me --- I'd consider them added to the
> "non-extra" arguments.
>
Yes, one can remove, shuffle or modify it.
> Drop "initial"?
I can do that but it can give false impression that the args will be present. Anyway it's probably just a corner case so I'll drop it.
>
>>>> + @param wrapper: list of arguments used as prefix to qemu binary
>>>> + @param name: name of this object (used for log/monitor/... file names)
>>> prefix for socket and log file names (default: qemu-PID)
>>>
>>
>> Sure, both make sense to me.
>>
>>>> + @param test_dir: base location to put log/monitor/... files in
>>>
>>> where to create socket and log file
>>>
>>> Aside: test_dir is a lousy name.
>>
>> Agree but changing names is tricky as people might be using kwargs to set it. Anyway using your description here, keeping the possible rename for a separate patchset (if needed).
>
> I'm merely observing the lousiness of this name. I'm not asking you to
> do anything about it :)
>
>>>> + @param monitor_address: custom address for QMP monitor
>>>
>>> Yes, but what *is* a custom address? Its user _base_args() appears to
>>> expect either a pair of strings (host, port) or a string filename.
>>>
>>
>> If you insist I can add something like "a tuple(host, port) or string to specify path", but I find it unnecessary detailed...
>
> I'm not the maintainer, I'm definitely not insisting on anything.
>
> If you're aiming for brevity, then drop "custom".
>
OK, removing in v6
>>>> + @param socket_scm_helper: path to scm_helper binary (to forward fds)
>>>
>>> What is an scm_helper, and why would I want to use it?
>>>
>>
>> To forward a file descriptor. It's for example used in tests/qemu-iotests/045 or tests/qemu-iotests/147
>
> What about "socket_scm_helper: helper program, required for send_fd_scm()"?
>
>>>> + @param debug: enable debug mode (forwarded to QMP helper and such)
>>>
>>> What is a QMP helper? To what else is debug forwarded?
>>>
>>
>> Debug is set in `self._debug` and can be consumed by anyone who has access to this variable. Currently that is the QMP, but people can inherit and use that variable to adjust their behavior.
>
> Drop the parenthesis?
>
OK
>>>> + @note: Qemu process is not started until launch() is used.
>>>
>>> until launch().
>>>
>>
>> OK
>
> One more thing: what the use of "@param"?
>
The API documentation can be autogenerated by doxygen, it uses those keywords to make it easier to read (and to create links, warnings, ...)
>>>> + '''
>>>
>>> It's an improvement.
>>>
>>>> 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 not os.path.exists(self._socket_scm_helper):
>>>> print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
>>>> return -1
>>>> fd_param = ["%s" % self._socket_scm_helper,
>>>> "%d" % self._qmp.get_sock_fd(),
>>>> "%s" % fd_file_path]
>>>> devnull = open('/dev/null', 'rb')
>>>> - p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
>>>> - stderr=sys.stderr)
>>>> - return p.wait()
>>>> + proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
>>>> + stderr=sys.stderr)
>>>> + return proc.wait()
>>>>
>>>> @staticmethod
>>>> def _remove_if_exists(path):
>>>> @@ -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)
>>>
>>> Recommend to break the line between the last two arguments.
>>>
>>
>> I'm not used to do that, but I can most certainly do that. Do you want me to change all places (eg. in the next chunk)
>
> PEP8 asks you to limit all lines to a maximum of 79 characters. This
> one is right at the maximum. Tolerable, but I prefer my lines a bit
> shorter. Use your judgement.
>
OK, I though you want to enforce the one-arg-per-line limit. I usually go with <=79 (+ '\n') so this is fine by me, but I'll put it into next line if that makes you happier.
>>>>
>>>> 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
>>>
>>> The bare except is almost certainly inappropriate. Are you sure we
>>> should silence pylint?
>>>
>>> Note that there's another bare except in launch(), visible in the
>>> previous patch hunk. I guess pylint is okay with that one because it
>>> re-throws the exception.
>>>
>>> In case we should silence pylint: what's the scope of this magic pylint
>>> comment? Can we use the symbolic disable=bare-except?
>>>
>>
>> Yep, we can use symbolic name as well. Well more appropriate would be to catch `SystemExit` and `KeyboardInterrupt`, treat them and raise them and then ignore the rest of exceptions. I'll do that in the next version...
>>
>>>> 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'''
>>>
>>> Make that "and return the response", because that's how
>>> docs/interop/qmp-spec.txt calls the thing.
>>>
>>
>> Sure, no problem. I wanted to stress-out it's a dict and not encoded string, but it's probably given by the context.
>
> "return the response dict" would be fine with me.
>
Yes, I like that.
>>>> 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 return result dict or on failure
>>>> + raise exception with details.
>>>> + '''
>>>
>>> I'm afraid "result dict" is wrong: it need not be a dict. "on failure
>>> raise exception" adds precious little information, and "with details"
>>> adds none.
>>>
>>> Perhaps:
>>>
>>> Invoke a QMP command.
>>> On success return the return value.
>>> On failure raise an exception.
>>>
>>
>> That is quite more verbose than I'd like and result in the same (for non-native speaker), but I have no strong opinion here so changing it to your version in v2.
>>
>>> The last sentence is too vague, but it's hard to do better because...
>>>
>>>> reply = self.qmp(cmd, conv_keys, **args)
>>>> if reply is None:
>>>> raise Exception("Monitor is closed")
>>>
>>> ... we raise Exception! This code is a *turd* (pardon my french), and
>>> PEP8 / pylint violations are the least of its problems.
>>>
>>
>> Yep, adding rework-exceptions to my TODO list (for a future patchset)
>>
>>>> @@ -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.'''
>>>
>>> What is match?
>>>
>>> name and timeout not worth mentioning?
>>>
>>
>> IMO this does not require full-blown documentation, it's not really a user-facing API and sometimes shorter is better. Anyway if you insist I can add full documentation of each parameter. I can even add `:type:` and `:returns:` and perhaps some `:warns:` and `:notes:` and I should descend and note all the possible `:raises:`.
>>
>> ... the point I'm trying to make is I don't think it's necessary to go that much into details. Anyway if you think the params are necessary to list, then I can do that.
>
> Use your judgement. The more detailed comments you add, the more
> questions you'll get ;)
>
Sure, I'll try to improve (but not too much) that in the v6.
>>>> + # Test if 'match' is a recursive subset of 'event'; skips branch
>>>> + # processing on match's value `None`
>>>
>>> What's a "recursive subset"? What's "branch processing"?
>>>
>>> There's an unusual set of quotes around `None`.
>>>
>>
>> Basically I was surprised why this function exists as one can directly compare dicts. It's because it works as the stock dict compare only on value "None" it breaks and assumes that "branch" matches. That's why I listed the example as I'm unsure how to explain it in a human language.
>>
>> As for the `None`, in sphinx/doxygen it becomes "block" so I'm used to these, anyway people use `'`s and `"`s here so I'll change it in next version to be consistent.
>
> According to git-grep, we're using neither sphinx nor doxygen right now.
>
yep, as I said I'll change it for `"`.
>>>> + # {"foo": {"bar": 1} matches {"foo": None}
>>>
>>> Aha: None servers as wildcard.
>>
>> Exactly.
>>
>>>
>>>> + def _event_match(event, match=None):
>>>
>>> Any particular reason for renaming this function?
>>>
>>
>> To emphasize it's internal, not a strong opinion but IMO looks better this way.
>
> It's a nested function, how could it *not* be internal?
>
It is always internal for the computer, but humans sometimes need more pointers. I can drop this part if you really dislike that change but I'm used to this notation.
>>>> 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
>>>
>>> Comma after execution, please.
>>>
>>
>> OK
>>
>>>> + of the qemu process.
>>>> + '''
>>>> return self._iolog
>>>
>>> I understand this code was factored out of qemu-iotests for use by
>>> qtest.py. That's okay, but I'd rather not serve as its maintainer.
>>> -E2MUCH...
>>>
>>
>> Yep, anyway it contains several useful methods/helpers and fixing basic issues is the simplest way to get to know the code (and the submitting process). Thank you for your time.
>
> You're welcome!
>
Thanks again,
Lukáš
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 502 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
2017-08-16 17:48 ` Lukáš Doktor
@ 2017-08-17 5:24 ` Markus Armbruster
2017-08-17 18:16 ` Lukáš Doktor
0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2017-08-17 5:24 UTC (permalink / raw)
To: Lukáš Doktor
Cc: famz, ehabkost, apahim, qemu-devel, mreitz, jsnow, f4bug
Lukáš Doktor <ldoktor@redhat.com> writes:
> Dne 16.8.2017 v 18:58 Markus Armbruster napsal(a):
>> Lukáš Doktor <ldoktor@redhat.com> writes:
>>
>>> Dne 15.8.2017 v 14:31 Markus Armbruster napsal(a):
>>>> Lukáš Doktor <ldoktor@redhat.com> writes:
>>>>
>>>>> 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..466aaab 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
>>>>
>>>> Initialize a QEMUMachine
>>>>
>>>> Rationale: it's __init__, not __create__, and "object" is redundant.
>>>>
>>>
>>> sure
>>>
>>>>> +
>>>>> + @param binary: path to the qemu binary (str)
>>>>
>>>> Drop (str), because what else could it be?
>>>
>>> it could be shlex.split of arguments to be passed to process. Anyway no strong opinion here so I dropping it...
>>>
>>>>
>>>>> + @param args: initial list of extra arguments
>>>>
>>>> If this is the initial list, then what's the final list?
>>>>
>>>
>>> It's the basic set of arguments which can be modified before the execution. Do you think it requires additional explanation, or would you like to improve it somehow?
>>
>> Can this list of extra arguments really be *modified*? Adding more
>> arguments doesn't count for me --- I'd consider them added to the
>> "non-extra" arguments.
>>
>
> Yes, one can remove, shuffle or modify it.
Bizarre :)
Who's "one"?
>> Drop "initial"?
>
> I can do that but it can give false impression that the args will be present. Anyway it's probably just a corner case so I'll drop it.
If it's intended to be modified, then keeping "initial" might be best.
Your choice.
>>
>>>>> + @param wrapper: list of arguments used as prefix to qemu binary
>>>>> + @param name: name of this object (used for log/monitor/... file names)
>>>> prefix for socket and log file names (default: qemu-PID)
>>>>
>>>
>>> Sure, both make sense to me.
>>>
>>>>> + @param test_dir: base location to put log/monitor/... files in
>>>>
>>>> where to create socket and log file
>>>>
>>>> Aside: test_dir is a lousy name.
>>>
>>> Agree but changing names is tricky as people might be using kwargs to set it. Anyway using your description here, keeping the possible rename for a separate patchset (if needed).
>>
>> I'm merely observing the lousiness of this name. I'm not asking you to
>> do anything about it :)
>>
>>>>> + @param monitor_address: custom address for QMP monitor
>>>>
>>>> Yes, but what *is* a custom address? Its user _base_args() appears to
>>>> expect either a pair of strings (host, port) or a string filename.
>>>>
>>>
>>> If you insist I can add something like "a tuple(host, port) or string to specify path", but I find it unnecessary detailed...
>>
>> I'm not the maintainer, I'm definitely not insisting on anything.
>>
>> If you're aiming for brevity, then drop "custom".
>>
>
> OK, removing in v6
>
>>>>> + @param socket_scm_helper: path to scm_helper binary (to forward fds)
>>>>
>>>> What is an scm_helper, and why would I want to use it?
>>>>
>>>
>>> To forward a file descriptor. It's for example used in tests/qemu-iotests/045 or tests/qemu-iotests/147
>>
>> What about "socket_scm_helper: helper program, required for send_fd_scm()"?
>>
>>>>> + @param debug: enable debug mode (forwarded to QMP helper and such)
>>>>
>>>> What is a QMP helper? To what else is debug forwarded?
>>>>
>>>
>>> Debug is set in `self._debug` and can be consumed by anyone who has access to this variable. Currently that is the QMP, but people can inherit and use that variable to adjust their behavior.
>>
>> Drop the parenthesis?
>>
>
> OK
>
>>>>> + @note: Qemu process is not started until launch() is used.
>>>>
>>>> until launch().
>>>>
>>>
>>> OK
>>
>> One more thing: what the use of "@param"?
>>
>
> The API documentation can be autogenerated by doxygen, it uses those keywords to make it easier to read (and to create links, warnings, ...)
"Can" or "could"? As far as I can tell, we aren't actually using
doxygen for anything, are we? Just like we aren't actually using
GTK-Doc. Yet its comment rash^H^H^H^Hannotations can be found here and
there, commonly mixed up with unannotated code, and often syntactically
invalid. No surprise, since they've never been processed by their
intended consumer.
If we decide we want to generate documentation, we should pick *one*
system and stick to it. Having every deveoper sprinkle "his" code with
the annotations he's used to from elsewhere will do us no good.
>>>>> + '''
>>>>
>>>> It's an improvement.
>>>>
>>>>> 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 not os.path.exists(self._socket_scm_helper):
>>>>> print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
>>>>> return -1
>>>>> fd_param = ["%s" % self._socket_scm_helper,
>>>>> "%d" % self._qmp.get_sock_fd(),
>>>>> "%s" % fd_file_path]
>>>>> devnull = open('/dev/null', 'rb')
>>>>> - p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
>>>>> - stderr=sys.stderr)
>>>>> - return p.wait()
>>>>> + proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
>>>>> + stderr=sys.stderr)
>>>>> + return proc.wait()
>>>>>
>>>>> @staticmethod
>>>>> def _remove_if_exists(path):
>>>>> @@ -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)
>>>>
>>>> Recommend to break the line between the last two arguments.
>>>>
>>>
>>> I'm not used to do that, but I can most certainly do that. Do you want me to change all places (eg. in the next chunk)
>>
>> PEP8 asks you to limit all lines to a maximum of 79 characters. This
>> one is right at the maximum. Tolerable, but I prefer my lines a bit
>> shorter. Use your judgement.
>>
>
> OK, I though you want to enforce the one-arg-per-line limit. I usually go with <=79 (+ '\n') so this is fine by me, but I'll put it into next line if that makes you happier.
Please wrap your lines in e-mail, too :)
>>>>>
>>>>> 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
>>>>
>>>> The bare except is almost certainly inappropriate. Are you sure we
>>>> should silence pylint?
>>>>
>>>> Note that there's another bare except in launch(), visible in the
>>>> previous patch hunk. I guess pylint is okay with that one because it
>>>> re-throws the exception.
>>>>
>>>> In case we should silence pylint: what's the scope of this magic pylint
>>>> comment? Can we use the symbolic disable=bare-except?
>>>>
>>>
>>> Yep, we can use symbolic name as well. Well more appropriate would be to catch `SystemExit` and `KeyboardInterrupt`, treat them and raise them and then ignore the rest of exceptions. I'll do that in the next version...
>>>
>>>>> 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'''
>>>>
>>>> Make that "and return the response", because that's how
>>>> docs/interop/qmp-spec.txt calls the thing.
>>>>
>>>
>>> Sure, no problem. I wanted to stress-out it's a dict and not encoded string, but it's probably given by the context.
>>
>> "return the response dict" would be fine with me.
>>
>
> Yes, I like that.
>
>>>>> 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 return result dict or on failure
>>>>> + raise exception with details.
>>>>> + '''
>>>>
>>>> I'm afraid "result dict" is wrong: it need not be a dict. "on failure
>>>> raise exception" adds precious little information, and "with details"
>>>> adds none.
>>>>
>>>> Perhaps:
>>>>
>>>> Invoke a QMP command.
>>>> On success return the return value.
>>>> On failure raise an exception.
>>>>
>>>
>>> That is quite more verbose than I'd like and result in the same (for non-native speaker), but I have no strong opinion here so changing it to your version in v2.
>>>
>>>> The last sentence is too vague, but it's hard to do better because...
>>>>
>>>>> reply = self.qmp(cmd, conv_keys, **args)
>>>>> if reply is None:
>>>>> raise Exception("Monitor is closed")
>>>>
>>>> ... we raise Exception! This code is a *turd* (pardon my french), and
>>>> PEP8 / pylint violations are the least of its problems.
>>>>
>>>
>>> Yep, adding rework-exceptions to my TODO list (for a future patchset)
>>>
>>>>> @@ -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.'''
>>>>
>>>> What is match?
>>>>
>>>> name and timeout not worth mentioning?
>>>>
>>>
>>> IMO this does not require full-blown documentation, it's not really a user-facing API and sometimes shorter is better. Anyway if you insist I can add full documentation of each parameter. I can even add `:type:` and `:returns:` and perhaps some `:warns:` and `:notes:` and I should descend and note all the possible `:raises:`.
>>>
>>> ... the point I'm trying to make is I don't think it's necessary to go that much into details. Anyway if you think the params are necessary to list, then I can do that.
>>
>> Use your judgement. The more detailed comments you add, the more
>> questions you'll get ;)
>>
>
> Sure, I'll try to improve (but not too much) that in the v6.
>
>>>>> + # Test if 'match' is a recursive subset of 'event'; skips branch
>>>>> + # processing on match's value `None`
>>>>
>>>> What's a "recursive subset"? What's "branch processing"?
>>>>
>>>> There's an unusual set of quotes around `None`.
>>>>
>>>
>>> Basically I was surprised why this function exists as one can directly compare dicts. It's because it works as the stock dict compare only on value "None" it breaks and assumes that "branch" matches. That's why I listed the example as I'm unsure how to explain it in a human language.
>>>
>>> As for the `None`, in sphinx/doxygen it becomes "block" so I'm used to these, anyway people use `'`s and `"`s here so I'll change it in next version to be consistent.
>>
>> According to git-grep, we're using neither sphinx nor doxygen right now.
>>
>
> yep, as I said I'll change it for `"`.
I'd prefer plain None over "None", because the former is clearly the
built-in constant None, while the latter looks like a string.
>>>>> + # {"foo": {"bar": 1} matches {"foo": None}
>>>>
>>>> Aha: None servers as wildcard.
>>>
>>> Exactly.
>>>
>>>>
>>>>> + def _event_match(event, match=None):
>>>>
>>>> Any particular reason for renaming this function?
>>>>
>>>
>>> To emphasize it's internal, not a strong opinion but IMO looks better this way.
>>
>> It's a nested function, how could it *not* be internal?
>>
>
> It is always internal for the computer, but humans sometimes need more pointers. I can drop this part if you really dislike that change but I'm used to this notation.
PEP8:
Method Names and Instance Variables
Use the function naming rules: lowercase with words separated by
underscores as necessary to improve readability.
Use one leading underscore only for non-public methods and instance
variables.
A nested function is not a method or instance variable.
>>>>> 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
>>>>
>>>> Comma after execution, please.
>>>>
>>>
>>> OK
>>>
>>>>> + of the qemu process.
>>>>> + '''
>>>>> return self._iolog
>>>>
>>>> I understand this code was factored out of qemu-iotests for use by
>>>> qtest.py. That's okay, but I'd rather not serve as its maintainer.
>>>> -E2MUCH...
>>>>
>>>
>>> Yep, anyway it contains several useful methods/helpers and fixing basic issues is the simplest way to get to know the code (and the submitting process). Thank you for your time.
>>
>> You're welcome!
>>
>
> Thanks again,
> Lukáš
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
2017-08-17 5:24 ` Markus Armbruster
@ 2017-08-17 18:16 ` Lukáš Doktor
0 siblings, 0 replies; 18+ messages in thread
From: Lukáš Doktor @ 2017-08-17 18:16 UTC (permalink / raw)
To: Markus Armbruster
Cc: famz, ehabkost, apahim, qemu-devel, mreitz, jsnow, f4bug
[-- Attachment #1: Type: text/plain, Size: 19993 bytes --]
Dne 17.8.2017 v 07:24 Markus Armbruster napsal(a):
> Lukáš Doktor <ldoktor@redhat.com> writes:
>
>> Dne 16.8.2017 v 18:58 Markus Armbruster napsal(a):
>>> Lukáš Doktor <ldoktor@redhat.com> writes:
>>>
>>>> Dne 15.8.2017 v 14:31 Markus Armbruster napsal(a):
>>>>> Lukáš Doktor <ldoktor@redhat.com> writes:
>>>>>
>>>>>> 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..466aaab 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
>>>>>
>>>>> Initialize a QEMUMachine
>>>>>
>>>>> Rationale: it's __init__, not __create__, and "object" is redundant.
>>>>>
>>>>
>>>> sure
>>>>
>>>>>> +
>>>>>> + @param binary: path to the qemu binary (str)
>>>>>
>>>>> Drop (str), because what else could it be?
>>>>
>>>> it could be shlex.split of arguments to be passed to process. Anyway no strong opinion here so I dropping it...
>>>>
>>>>>
>>>>>> + @param args: initial list of extra arguments
>>>>>
>>>>> If this is the initial list, then what's the final list?
>>>>>
>>>>
>>>> It's the basic set of arguments which can be modified before the execution. Do you think it requires additional explanation, or would you like to improve it somehow?
>>>
>>> Can this list of extra arguments really be *modified*? Adding more
>>> arguments doesn't count for me --- I'd consider them added to the
>>> "non-extra" arguments.
>>>
>>
>> Yes, one can remove, shuffle or modify it.
>
> Bizarre :)
>
> Who's "one"?
>
The user, or some inherited classes (eg. iotest.py). Currently nobody does that, but one might...
>>> Drop "initial"?
>>
>> I can do that but it can give false impression that the args will be present. Anyway it's probably just a corner case so I'll drop it.
>
> If it's intended to be modified, then keeping "initial" might be best.
> Your choice.
>
OK, let's not over-complicate it and just say it's 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)
>>>>> prefix for socket and log file names (default: qemu-PID)
>>>>>
>>>>
>>>> Sure, both make sense to me.
>>>>
>>>>>> + @param test_dir: base location to put log/monitor/... files in
>>>>>
>>>>> where to create socket and log file
>>>>>
>>>>> Aside: test_dir is a lousy name.
>>>>
>>>> Agree but changing names is tricky as people might be using kwargs to set it. Anyway using your description here, keeping the possible rename for a separate patchset (if needed).
>>>
>>> I'm merely observing the lousiness of this name. I'm not asking you to
>>> do anything about it :)
>>>
>>>>>> + @param monitor_address: custom address for QMP monitor
>>>>>
>>>>> Yes, but what *is* a custom address? Its user _base_args() appears to
>>>>> expect either a pair of strings (host, port) or a string filename.
>>>>>
>>>>
>>>> If you insist I can add something like "a tuple(host, port) or string to specify path", but I find it unnecessary detailed...
>>>
>>> I'm not the maintainer, I'm definitely not insisting on anything.
>>>
>>> If you're aiming for brevity, then drop "custom".
>>>
>>
>> OK, removing in v6
>>
>>>>>> + @param socket_scm_helper: path to scm_helper binary (to forward fds)
>>>>>
>>>>> What is an scm_helper, and why would I want to use it?
>>>>>
>>>>
>>>> To forward a file descriptor. It's for example used in tests/qemu-iotests/045 or tests/qemu-iotests/147
>>>
>>> What about "socket_scm_helper: helper program, required for send_fd_scm()"?
>>>
>>>>>> + @param debug: enable debug mode (forwarded to QMP helper and such)
>>>>>
>>>>> What is a QMP helper? To what else is debug forwarded?
>>>>>
>>>>
>>>> Debug is set in `self._debug` and can be consumed by anyone who has access to this variable. Currently that is the QMP, but people can inherit and use that variable to adjust their behavior.
>>>
>>> Drop the parenthesis?
>>>
>>
>> OK
>>
>>>>>> + @note: Qemu process is not started until launch() is used.
>>>>>
>>>>> until launch().
>>>>>
>>>>
>>>> OK
>>>
>>> One more thing: what the use of "@param"?
>>>
>>
>> The API documentation can be autogenerated by doxygen, it uses those keywords to make it easier to read (and to create links, warnings, ...)
>
> "Can" or "could"? As far as I can tell, we aren't actually using
> doxygen for anything, are we? Just like we aren't actually using
> GTK-Doc. Yet its comment rash^H^H^H^Hannotations can be found here and
> there, commonly mixed up with unannotated code, and often syntactically
> invalid. No surprise, since they've never been processed by their
> intended consumer.
>
> If we decide we want to generate documentation, we should pick *one*
> system and stick to it. Having every deveoper sprinkle "his" code with
> the annotations he's used to from elsewhere will do us no good.
>
I'm not aware of any official documentation (as those scripts are pretty much internal), anyway IDEs like them as well. I saw the docstring notation on other places (qmp.py, qtest.py so I went with that in this version.
PS: I'm used to sphinx notation (:param)
>>>>>> + '''
>>>>>
>>>>> It's an improvement.
>>>>>
>>>>>> 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 not os.path.exists(self._socket_scm_helper):
>>>>>> print >>sys.stderr, "%s does not exist" % self._socket_scm_helper
>>>>>> return -1
>>>>>> fd_param = ["%s" % self._socket_scm_helper,
>>>>>> "%d" % self._qmp.get_sock_fd(),
>>>>>> "%s" % fd_file_path]
>>>>>> devnull = open('/dev/null', 'rb')
>>>>>> - p = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
>>>>>> - stderr=sys.stderr)
>>>>>> - return p.wait()
>>>>>> + proc = subprocess.Popen(fd_param, stdin=devnull, stdout=sys.stdout,
>>>>>> + stderr=sys.stderr)
>>>>>> + return proc.wait()
>>>>>>
>>>>>> @staticmethod
>>>>>> def _remove_if_exists(path):
>>>>>> @@ -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)
>>>>>
>>>>> Recommend to break the line between the last two arguments.
>>>>>
>>>>
>>>> I'm not used to do that, but I can most certainly do that. Do you want me to change all places (eg. in the next chunk)
>>>
>>> PEP8 asks you to limit all lines to a maximum of 79 characters. This
>>> one is right at the maximum. Tolerable, but I prefer my lines a bit
>>> shorter. Use your judgement.
>>>
>>
>> OK, I though you want to enforce the one-arg-per-line limit. I usually go with <=79 (+ '\n') so this is fine by me, but I'll put it into next line if that makes you happier.
>
> Please wrap your lines in e-mail, too :)
>
Oh, sorry, this is a tough one... I'll take a look at the options (thunderbird)
>>>>>>
>>>>>> 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
>>>>>
>>>>> The bare except is almost certainly inappropriate. Are you sure we
>>>>> should silence pylint?
>>>>>
>>>>> Note that there's another bare except in launch(), visible in the
>>>>> previous patch hunk. I guess pylint is okay with that one because it
>>>>> re-throws the exception.
>>>>>
>>>>> In case we should silence pylint: what's the scope of this magic pylint
>>>>> comment? Can we use the symbolic disable=bare-except?
>>>>>
>>>>
>>>> Yep, we can use symbolic name as well. Well more appropriate would be to catch `SystemExit` and `KeyboardInterrupt`, treat them and raise them and then ignore the rest of exceptions. I'll do that in the next version...
>>>>
>>>>>> 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'''
>>>>>
>>>>> Make that "and return the response", because that's how
>>>>> docs/interop/qmp-spec.txt calls the thing.
>>>>>
>>>>
>>>> Sure, no problem. I wanted to stress-out it's a dict and not encoded string, but it's probably given by the context.
>>>
>>> "return the response dict" would be fine with me.
>>>
>>
>> Yes, I like that.
>>
>>>>>> 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 return result dict or on failure
>>>>>> + raise exception with details.
>>>>>> + '''
>>>>>
>>>>> I'm afraid "result dict" is wrong: it need not be a dict. "on failure
>>>>> raise exception" adds precious little information, and "with details"
>>>>> adds none.
>>>>>
>>>>> Perhaps:
>>>>>
>>>>> Invoke a QMP command.
>>>>> On success return the return value.
>>>>> On failure raise an exception.
>>>>>
>>>>
>>>> That is quite more verbose than I'd like and result in the same (for non-native speaker), but I have no strong opinion here so changing it to your version in v2.
>>>>
>>>>> The last sentence is too vague, but it's hard to do better because...
>>>>>
>>>>>> reply = self.qmp(cmd, conv_keys, **args)
>>>>>> if reply is None:
>>>>>> raise Exception("Monitor is closed")
>>>>>
>>>>> ... we raise Exception! This code is a *turd* (pardon my french), and
>>>>> PEP8 / pylint violations are the least of its problems.
>>>>>
>>>>
>>>> Yep, adding rework-exceptions to my TODO list (for a future patchset)
>>>>
>>>>>> @@ -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.'''
>>>>>
>>>>> What is match?
>>>>>
>>>>> name and timeout not worth mentioning?
>>>>>
>>>>
>>>> IMO this does not require full-blown documentation, it's not really a user-facing API and sometimes shorter is better. Anyway if you insist I can add full documentation of each parameter. I can even add `:type:` and `:returns:` and perhaps some `:warns:` and `:notes:` and I should descend and note all the possible `:raises:`.
>>>>
>>>> ... the point I'm trying to make is I don't think it's necessary to go that much into details. Anyway if you think the params are necessary to list, then I can do that.
>>>
>>> Use your judgement. The more detailed comments you add, the more
>>> questions you'll get ;)
>>>
>>
>> Sure, I'll try to improve (but not too much) that in the v6.
>>
>>>>>> + # Test if 'match' is a recursive subset of 'event'; skips branch
>>>>>> + # processing on match's value `None`
>>>>>
>>>>> What's a "recursive subset"? What's "branch processing"?
>>>>>
>>>>> There's an unusual set of quotes around `None`.
>>>>>
>>>>
>>>> Basically I was surprised why this function exists as one can directly compare dicts. It's because it works as the stock dict compare only on value "None" it breaks and assumes that "branch" matches. That's why I listed the example as I'm unsure how to explain it in a human language.
>>>>
>>>> As for the `None`, in sphinx/doxygen it becomes "block" so I'm used to these, anyway people use `'`s and `"`s here so I'll change it in next version to be consistent.
>>>
>>> According to git-grep, we're using neither sphinx nor doxygen right now.
>>>
>>
>> yep, as I said I'll change it for `"`.
>
> I'd prefer plain None over "None", because the former is clearly the
> built-in constant None, while the latter looks like a string.
>
Sure
>>>>>> + # {"foo": {"bar": 1} matches {"foo": None}
>>>>>
>>>>> Aha: None servers as wildcard.
>>>>
>>>> Exactly.
>>>>
>>>>>
>>>>>> + def _event_match(event, match=None):
>>>>>
>>>>> Any particular reason for renaming this function?
>>>>>
>>>>
>>>> To emphasize it's internal, not a strong opinion but IMO looks better this way.
>>>
>>> It's a nested function, how could it *not* be internal?
>>>
>>
>> It is always internal for the computer, but humans sometimes need more pointers. I can drop this part if you really dislike that change but I'm used to this notation.
>
> PEP8:
>
> Method Names and Instance Variables
>
> Use the function naming rules: lowercase with words separated by
> underscores as necessary to improve readability.
>
> Use one leading underscore only for non-public methods and instance
> variables.
>
> A nested function is not a method or instance variable.
>
OK, I'll keep the name and send v6 probably tomorrow.
Regards,
Lukáš
>>>>>> 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
>>>>>
>>>>> Comma after execution, please.
>>>>>
>>>>
>>>> OK
>>>>
>>>>>> + of the qemu process.
>>>>>> + '''
>>>>>> return self._iolog
>>>>>
>>>>> I understand this code was factored out of qemu-iotests for use by
>>>>> qtest.py. That's okay, but I'd rather not serve as its maintainer.
>>>>> -E2MUCH...
>>>>>
>>>>
>>>> Yep, anyway it contains several useful methods/helpers and fixing basic issues is the simplest way to get to know the code (and the submitting process). Thank you for your time.
>>>
>>> You're welcome!
>>>
>>
>> Thanks again,
>> Lukáš
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 502 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-08-17 18:16 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-15 8:57 [Qemu-devel] [PATCH v5 00/10] qemu.py: Pylint/style fixes Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 01/10] " Lukáš Doktor
2017-08-15 11:55 ` Stefan Hajnoczi
2017-08-15 12:31 ` Markus Armbruster
2017-08-16 16:01 ` Lukáš Doktor
2017-08-16 16:58 ` Markus Armbruster
2017-08-16 17:48 ` Lukáš Doktor
2017-08-17 5:24 ` Markus Armbruster
2017-08-17 18:16 ` Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 02/10] qemu|qtest: Avoid dangerous arguments Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 03/10] qemu.py: Use iteritems rather than keys() Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 04/10] qemu.py: Simplify QMP key-conversion Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 05/10] qemu.py: Use custom exceptions rather than Exception Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 06/10] qmp.py: Couple of pylint/style fixes Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 07/10] qmp.py: Use object-based class for QEMUMonitorProtocol Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 08/10] qmp.py: Avoid "has_key" usage Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 09/10] qmp.py: Avoid overriding a builtin object Lukáš Doktor
2017-08-15 8:57 ` [Qemu-devel] [PATCH v5 10/10] qtest.py: Few pylint/style fixes Lukáš Doktor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).