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

[Lukáš Doktor]

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?

>> +        @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).

> 
>> +        @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...

>> +        @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

>> +        @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.

>> +        @note: Qemu process is not started until launch() is used.
> 
> until launch().
> 

OK

>> +        '''
> 
> 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)

>>  
>>      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.

>>          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.

>> +        # 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.

>> +        #    {"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.

>>              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.

Regards,
Lukáš

signature.asc
Description: OpenPGP digital signature