Re: [Qemu-devel] [PATCH v5 01/10] qemu.py: Pylint/style fixes

[Markus Armbruster]

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"?

>> 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áš