[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes |
Date: |
Wed, 16 Aug 2017 18:58:56 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Lukáš Doktor <address@hidden> writes:
> Dne 15.8.2017 v 14:31 Markus Armbruster napsal(a):
>> Lukáš Doktor <address@hidden> writes:
>>
>>> No actual code changes, just several pylint/style fixes and docstring
>>> clarifications.
>>>
>>> Signed-off-by: Lukáš Doktor <address@hidden>
>>> ---
>>> 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!
- [Qemu-devel] [PATCH v5 04/10] qemu.py: Simplify QMP key-conversion, (continued)
- [Qemu-devel] [PATCH v5 04/10] qemu.py: Simplify QMP key-conversion, Lukáš Doktor, 2017/08/15
- [Qemu-devel] [PATCH v5 06/10] qmp.py: Couple of pylint/style fixes, Lukáš Doktor, 2017/08/15
- [Qemu-devel] [PATCH v5 07/10] qmp.py: Use object-based class for QEMUMonitorProtocol, Lukáš Doktor, 2017/08/15
- [Qemu-devel] [PATCH v5 08/10] qmp.py: Avoid "has_key" usage, Lukáš Doktor, 2017/08/15
- [Qemu-devel] [PATCH v5 09/10] qmp.py: Avoid overriding a builtin object, Lukáš Doktor, 2017/08/15
- [Qemu-devel] [PATCH v5 10/10] qtest.py: Few pylint/style fixes, Lukáš Doktor, 2017/08/15
- [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes, Lukáš Doktor, 2017/08/15
[Qemu-devel] [PATCH v5 02/10] qemu|qtest: Avoid dangerous arguments, Lukáš Doktor, 2017/08/15
[Qemu-devel] [PATCH v5 03/10] qemu.py: Use iteritems rather than keys(), Lukáš Doktor, 2017/08/15