| Message ID | 20170815085732.9794-2-ldoktor@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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...
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áš
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!
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áš
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áš
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áš
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
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(-)