[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: |
Lukáš Doktor |
Subject: |
Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes |
Date: |
Thu, 17 Aug 2017 20:16:01 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 |
Dne 17.8.2017 v 07:24 Markus Armbruster napsal(a):
> Lukáš Doktor <address@hidden> writes:
>
>> Dne 16.8.2017 v 18:58 Markus Armbruster napsal(a):
>>> 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.
>>>
>>
>> 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áš
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v5 08/10] qmp.py: Avoid "has_key" usage, (continued)
[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