Dne 16.8.2017 v 18:58 Markus Armbruster napsal(a): > 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. >
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áš
signature.asc
Description: OpenPGP digital signature
