Lukáš Doktor <ldok...@redhat.com> writes: > No actual code changes, just several pylint/style fixes and docstring > clarifications. > > Signed-off-by: Lukáš Doktor <ldok...@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...