Lukáš Doktor <ldok...@redhat.com> writes: > Dne 15.8.2017 v 14:31 Markus Armbruster napsal(a): >> 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. >> > > 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!