Re: [PATCH 2/3] QMPMonitor: Introduce the send() method and wrappers

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 18 Oct 2010 09:42:44 -0200
Lucas Meneghel Rodrigues <lmr@redhat.com> wrote:

> On Fri, 2010-10-15 at 17:52 -0300, Luiz Capitulino wrote:
> > This method directly sends data to the QMP monitor and returns its
> > response, without any kind of special treatment or sanity checking.
> > 
> > Two simple wrappers are also introduced: cmd_obj() and cmd_qmp(),
> > they provide some level of automation on building QMP commands.
> > 
> > All three methods are going to be used by the QMP test-suite.
> > 
> > NOTE: The methods send() and _get_command_output() are similar, but
> >       I'm out of ideas on how to properly refactor them.
> 
> ^ Rather we should probably refactor all the other commands in terms of
> send() rather than _get_command_output(). This can be done on a later
> patch, don't worry.
> 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  client/tests/kvm/kvm_monitor.py |   60 +++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 60 insertions(+), 0 deletions(-)
> > 
> > diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
> > index d77af31..63201b4 100644
> > --- a/client/tests/kvm/kvm_monitor.py
> > +++ b/client/tests/kvm/kvm_monitor.py
> > @@ -604,6 +604,66 @@ class QMPMonitor(Monitor):
> >          return self._greeting
> >  
> > 
> > +    def send(self, data, timeout=20):
> > +        """
> > +        Send data to the QMP monitor and return its response.
> > +
> > +        @param data: Data to send
> > +        @param timeout: Time duration to wait for response
> > +        @return: QMP success or error response as a dictionary
> > +        @raise MonitorLockError: Raised if the lock cannot be acquired
> > +        @raise MonitorSendError: Raised if the command cannot be sent
> > +        @raise MonitorProtocolError: Raised if no response is received
> > +        """
> > +        # XXX: This method is similar to _get_command_output(), we should
> > +        # refactor it
> > +        if not self._acquire_lock(20):
> > +            raise MonitorLockError("Could not acquire exclusive lock to send "
> > +                                   "QMP command '%s'" % cmd)
> > +        try:
> > +            self._read_objects()
> > +            self._socket.sendall(data)
> > +            end_time = time.time() + timeout
> > +            while time.time() < end_time:
> > +                for obj in self._read_objects():
> > +                    if isinstance(obj, dict):
> > +                        if "return" in obj or "error" in obj:
> > +                            return obj
> > +                time.sleep(0.1)
> > +            else:
> > +                 raise MonitorProtocolError("Received no response (data: %s)"
> > +                                            % str(data))
> 
> ^ Here we have an indentation with an extra space. As a suggestion to
> avoid such problems, you can allways use utils/reindent.py (this path is
> relative to the autotest source tree directory
> 
> utils/reindent.py client/tests/kvm/kvm_monitor.py
> 
> Also, in order to catch up little mistakes, I also suggest the pylint
> wrapper that we have:
> 
> utils/run_pylint.py client/tests/kvm/kvm_monitor.py
> 
> Anyway, my pre-commit scripts run it, just pointed out because it's nice
> to know those helper tools.

I can do it, as this is likely going to need a respin.

> > +        except socket.error:
> > +            raise MonitorSendError("Could not send data '%s'" % str(data))
> > +        finally:
> > +            self._lock.release()
> > +
> > +
> > +    def cmd_obj(self, cmd_obj, timeout=20):
> > +        """
> > +        Take a Python object, transforms it in JSON and send the resulting
> > +        string to the QMP monitor. Return the monitor's response.
> > +
> > +        @param cmd_obj: Python object to transform in JSON
> > +        @param timeout: Time duration to wait for response
> > +        @return: QMP success or error response as a dictionary
> > +        @note: raise same exceptions as send()
> > +        """
> > +        return self.send(json.dumps(cmd_obj) + "\n", timeout)
> 
> ^ Wouldn't be better to name this method obj_to_qmp ?

I don't think so, 'obj_to_qmp' gives me the impression that the method's
input is going to get some transformation and then returned. IOW, I can't
tell from that name that a command is going to be sent.

> > +    def cmd_qmp(self, command, arguments=None, id=None, timeout=20):
> > +        """
> > +        Build a QMP command from the passed arguments, return the monitor's
> > +        response.
> > +
> > +        @param command: QMP command name
> > +        @param arguments: Arguments in the form of a Python dictionary
> > +        @param id: QMP command id
> > +        @return: QMP success or error response as a dictionary
> > +        @note: raise same exceptions as send_cmd()
> > +        """
> > +        return self.cmd_obj(self._build_cmd(command, arguments, id), timeout)
> > +
> >      # Command wrappers
> >      # Note: all of the following functions raise exceptions in a similar manner
> >      # to cmd() and _get_command_output().
> 
>