Re: [Autotest] [KVM-AUTOTEST PATCH] [RFC] KVM test: kvm_monitor.py: refactor _get_command_output()

[Luiz Capitulino <lcapitulino@redhat.com>]

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

> On Mon, 2010-10-18 at 15:13 +0200, Michael Goldish wrote:
> > Instead of _get_command_output() and friends, introduce the following methods:
> > 
> > * QMP:
> >   - _send(): send raw data without waiting for a response
> >   - _get_response(): get the response to a previously sent command
> >   - cmd(): send a command with arguments, return response, raise an exception
> >            if the command fails
> >   - cmd_raw(): send a raw string, return response dict without postprocessing
> >   - cmd_obj(): send a Python object (converted to JSON), return response dict
> >                without postprocessing
> >   - cmd_qmp(): send a command with arguments, return response dict without
> >                postprocessing
> > 
> >   cmd() is useful for common monitor usage.  cmd_raw(), cmd_obj() and
> >   cmd_qmp() are required by Luiz Capitulino's test suite.  The difference
> >   between cmd() and cmd_qmp() is that the latter does not perform any checks
> >   on the response dict.  Note that cmd_raw() is functionally equivalent to
> >   send() from Luiz's patch.  I propose that we use the name cmd_raw() because
> >   send() implies sending data without caring about the response, whereas
> >   cmd_raw() implies doing exactly what cmd_obj() and cmd_qmp() do, but using
> >   raw data.
> > 
> > * Human monitor:
> >   - _send(): send raw data without waiting for a response
> >   - cmd(): send a command, return response
> 
> This patch does look reasonable to me. Luiz, what about rewriting your
> patchset after I apply this to upstream?	

Yeah, looks like a good idea.

> 
> > Signed-off-by: Michael Goldish <mgoldish@redhat.com>
> > ---
> >  client/tests/kvm/kvm_monitor.py |  235 ++++++++++++++++++++++----------------
> >  1 files changed, 136 insertions(+), 99 deletions(-)
> > 
> > diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
> > index 6bff07f..b887060 100644
> > --- a/client/tests/kvm/kvm_monitor.py
> > +++ b/client/tests/kvm/kvm_monitor.py
> > @@ -130,7 +130,7 @@ class HumanMonitor(Monitor):
> >                  suppress_exceptions is False
> >          @raise MonitorProtocolError: Raised if the initial (qemu) prompt isn't
> >                  found and suppress_exceptions is False
> > -        @note: Other exceptions may be raised.  See _get_command_output's
> > +        @note: Other exceptions may be raised.  See cmd()'s
> >                  docstring.
> >          """
> >          try:
> > @@ -146,7 +146,7 @@ class HumanMonitor(Monitor):
> >                                             "Output so far: %r" % o)
> >  
> >              # Save the output of 'help' for future use
> > -            self._help_str = self._get_command_output("help")
> > +            self._help_str = self.cmd("help")
> >  
> >          except MonitorError, e:
> >              if suppress_exceptions:
> > @@ -173,31 +173,32 @@ class HumanMonitor(Monitor):
> >          return False, "\n".join(o.splitlines())
> >  
> > 
> > -    def _send_command(self, command):
> > +    def _send(self, cmd):
> >          """
> >          Send a command without waiting for output.
> >  
> > -        @param command: Command to send
> > -        @return: True if successful, False otherwise
> > +        @param cmd: Command to send
> >          @raise MonitorLockError: Raised if the lock cannot be acquired
> >          @raise MonitorSendError: Raised if the command cannot be sent
> >          """
> >          if not self._acquire_lock(20):
> >              raise MonitorLockError("Could not acquire exclusive lock to send "
> > -                                   "monitor command '%s'" % command)
> > +                                   "monitor command '%s'" % cmd)
> >  
> >          try:
> >              try:
> > -                self._socket.sendall(command + "\n")
> > +                self._socket.sendall(cmd + "\n")
> >              except socket.error:
> >                  raise MonitorSendError("Could not send monitor command '%s'" %
> > -                                       command)
> > +                                       cmd)
> >  
> >          finally:
> >              self._lock.release()
> >  
> > 
> > -    def _get_command_output(self, command, timeout=20):
> > +    # Public methods
> > +
> > +    def cmd(self, command, timeout=20):
> >          """
> >          Send command to the monitor.
> >  
> > @@ -217,7 +218,7 @@ class HumanMonitor(Monitor):
> >              # Read any data that might be available
> >              self._recvall()
> >              # Send command
> > -            self._send_command(command)
> > +            self._send(command)
> >              # Read output
> >              s, o = self._read_up_to_qemu_prompt(timeout)
> >              # Remove command echo from output
> > @@ -234,8 +235,6 @@ class HumanMonitor(Monitor):
> >              self._lock.release()
> >  
> > 
> > -    # Public methods
> > -
> >      def is_responsive(self):
> >          """
> >          Make sure the monitor is responsive by sending a command.
> > @@ -243,7 +242,7 @@ class HumanMonitor(Monitor):
> >          @return: True if responsive, False otherwise
> >          """
> >          try:
> > -            self._get_command_output("info status")
> > +            self.cmd("info status")
> >              return True
> >          except MonitorError:
> >              return False
> > @@ -252,39 +251,22 @@ class HumanMonitor(Monitor):
> >      # Command wrappers
> >      # Notes:
> >      # - All of the following commands raise exceptions in a similar manner to
> > -    #   cmd() and _get_command_output().
> > +    #   cmd().
> >      # - A command wrapper should use self._help_str if it requires information
> >      #   about the monitor's capabilities.
> >  
> > -    def cmd(self, command, timeout=20):
> > -        """
> > -        Send a simple command with no parameters and return its output.
> > -        Should only be used for commands that take no parameters and are
> > -        implemented under the same name for both the human and QMP monitors.
> > -
> > -        @param command: Command to send
> > -        @param timeout: Time duration to wait for (qemu) prompt after command
> > -        @return: The output of the command
> > -        @raise MonitorLockError: Raised if the lock cannot be acquired
> > -        @raise MonitorSendError: Raised if the command cannot be sent
> > -        @raise MonitorProtocolError: Raised if the (qemu) prompt cannot be
> > -                found after sending the command
> > -        """
> > -        return self._get_command_output(command, timeout)
> > -
> > -
> >      def quit(self):
> >          """
> >          Send "quit" without waiting for output.
> >          """
> > -        self._send_command("quit")
> > +        self._send("quit")
> >  
> > 
> >      def info(self, what):
> >          """
> >          Request info about something and return the output.
> >          """
> > -        return self._get_command_output("info %s" % what)
> > +        return self.cmd("info %s" % what)
> >  
> > 
> >      def query(self, what):
> > @@ -301,7 +283,7 @@ class HumanMonitor(Monitor):
> >          @param filename: Location for the screendump
> >          @return: The command's output
> >          """
> > -        return self._get_command_output("screendump %s" % filename)
> > +        return self.cmd("screendump %s" % filename)
> >  
> > 
> >      def migrate(self, uri, full_copy=False, incremental_copy=False, wait=False):
> > @@ -323,7 +305,7 @@ class HumanMonitor(Monitor):
> >          if incremental_copy:
> >              cmd += " -i"
> >          cmd += " %s" % uri
> > -        return self._get_command_output(cmd)
> > +        return self.cmd(cmd)
> >  
> > 
> >      def migrate_set_speed(self, value):
> > @@ -333,7 +315,7 @@ class HumanMonitor(Monitor):
> >          @param value: Speed in bytes/sec
> >          @return: The command's output
> >          """
> > -        return self._get_command_output("migrate_set_speed %s" % value)
> > +        return self.cmd("migrate_set_speed %s" % value)
> >  
> > 
> >      def sendkey(self, keystr, hold_time=1):
> > @@ -344,7 +326,7 @@ class HumanMonitor(Monitor):
> >          @param hold_time: Hold time in ms (should normally stay 1 ms)
> >          @return: The command's output
> >          """
> > -        return self._get_command_output("sendkey %s %s" % (keystr, hold_time))
> > +        return self.cmd("sendkey %s %s" % (keystr, hold_time))
> >  
> > 
> >      def mouse_move(self, dx, dy):
> > @@ -355,7 +337,7 @@ class HumanMonitor(Monitor):
> >          @param dy: Y amount
> >          @return: The command's output
> >          """
> > -        return self._get_command_output("mouse_move %d %d" % (dx, dy))
> > +        return self.cmd("mouse_move %d %d" % (dx, dy))
> >  
> > 
> >      def mouse_button(self, state):
> > @@ -365,7 +347,7 @@ class HumanMonitor(Monitor):
> >          @param state: Button state (1=L, 2=M, 4=R)
> >          @return: The command's output
> >          """
> > -        return self._get_command_output("mouse_button %d" % state)
> > +        return self.cmd("mouse_button %d" % state)
> >  
> > 
> >  class QMPMonitor(Monitor):
> > @@ -387,7 +369,7 @@ class QMPMonitor(Monitor):
> >          @raise MonitorNotSupportedError: Raised if json isn't available and
> >                  suppress_exceptions is False
> >          @note: Other exceptions may be raised if the qmp_capabilities command
> > -                fails.  See _get_command_output's docstring.
> > +                fails.  See cmd()'s docstring.
> >          """
> >          try:
> >              Monitor.__init__(self, name, filename)
> > @@ -417,7 +399,7 @@ class QMPMonitor(Monitor):
> >                  raise MonitorProtocolError("No QMP greeting message received")
> >  
> >              # Issue qmp_capabilities
> > -            self._get_command_output("qmp_capabilities")
> > +            self.cmd("qmp_capabilities")
> >  
> >          except MonitorError, e:
> >              if suppress_exceptions:
> > @@ -470,33 +452,46 @@ class QMPMonitor(Monitor):
> >          return objs
> >  
> > 
> > -    def _send_command(self, cmd, args=None, id=None):
> > +    def _send(self, data):
> >          """
> > -        Send command without waiting for response.
> > +        Send raw data without waiting for response.
> >  
> > -        @param cmd: Command to send
> > -        @param args: A dict containing command arguments, or None
> > -        @raise MonitorLockError: Raised if the lock cannot be acquired
> > -        @raise MonitorSendError: Raised if the command cannot be sent
> > +        @param data: Data to send
> > +        @raise MonitorSendError: Raised if the data cannot be sent
> >          """
> > -        if not self._acquire_lock(20):
> > -            raise MonitorLockError("Could not acquire exclusive lock to send "
> > -                                   "QMP command '%s'" % cmd)
> > -
> >          try:
> > -            cmdobj = self._build_cmd(cmd, args, id)
> > -            try:
> > -                self._socket.sendall(json.dumps(cmdobj) + "\n")
> > -            except socket.error:
> > -                raise MonitorSendError("Could not send QMP command '%s'" % cmd)
> > +            self._socket.sendall(data)
> > +        except socket.error:
> > +            raise MonitorSendError("Could not send data: %r" % data)
> >  
> > -        finally:
> > -            self._lock.release()
> >  
> > +    def _get_response(self, id=None, timeout=20):
> > +        """
> > +        Read a response from the QMP monitor.
> >  
> > -    def _get_command_output(self, cmd, args=None, timeout=20):
> > +        @param id: If not None, look for a response with this id
> > +        @param timeout: Time duration to wait for response
> > +        @return: The response dict, or None if none was found
> >          """
> > -        Send monitor command and wait for response.
> > +        end_time = time.time() + timeout
> > +        while time.time() < end_time:
> > +            for obj in self._read_objects():
> > +                if isinstance(obj, dict):
> > +                    if id is not None and obj.get("id") != id:
> > +                        continue
> > +                    if "return" in obj or "error" in obj:
> > +                        return obj
> > +            time.sleep(0.1)
> > +
> > +
> > +    # Public methods
> > +
> > +    def cmd(self, cmd, args=None, timeout=20):
> > +        """
> > +        Send a QMP monitor command and return the response.
> > +
> > +        Note: an id is automatically assigned to the command and the response
> > +        is checked for the presence of the same id.
> >  
> >          @param cmd: Command to send
> >          @param args: A dict containing command arguments, or None
> > @@ -518,28 +513,86 @@ class QMPMonitor(Monitor):
> >              self._read_objects()
> >              # Send command
> >              id = kvm_utils.generate_random_string(8)
> > -            self._send_command(cmd, args, id)
> > +            self._send(json.dumps(self._build_cmd(cmd, args, id)) + "\n")
> >              # Read response
> > -            end_time = time.time() + timeout
> > -            while time.time() < end_time:
> > -                for obj in self._read_objects():
> > -                    if isinstance(obj, dict) and obj.get("id") == id:
> > -                        if "return" in obj:
> > -                            return obj["return"]
> > -                        elif "error" in obj:
> > -                            raise QMPCmdError("QMP command '%s' failed" % cmd,
> > -                                              obj["error"])
> > -                time.sleep(0.1)
> > -            # No response found
> > -            raise MonitorProtocolError("Received no response to QMP command "
> > -                                       "'%s', or received a response with an "
> > -                                       "incorrect id" % cmd)
> > +            r = self._get_response(id, timeout)
> > +            if r is None:
> > +                raise MonitorProtocolError("Received no response to QMP "
> > +                                           "command '%s', or received a "
> > +                                           "response with an incorrect id"
> > +                                           % cmd)
> > +            if "return" in r:
> > +                return r["return"]
> > +            if "error" in r:
> > +                raise QMPCmdError("QMP command '%s' failed" % cmd, r["error"])
> >  
> >          finally:
> >              self._lock.release()
> >  
> > 
> > -    # Public methods
> > +    def cmd_raw(self, data, timeout=20):
> > +        """
> > +        Send a raw string to the QMP monitor and return the response.
> > +        Unlike cmd(), return the raw response dict without performing any
> > +        checks on it.
> > +
> > +        @param data: The data to send
> > +        @param timeout: Time duration to wait for response
> > +        @return: The response received
> > +        @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
> > +        """
> > +        if not self._acquire_lock(20):
> > +            raise MonitorLockError("Could not acquire exclusive lock to send "
> > +                                   "data: %r" % data)
> > +
> > +        try:
> > +            self._read_objects()
> > +            self._send(data)
> > +            r = self._get_response(None, timeout)
> > +            if r is None:
> > +                raise MonitorProtocolError("Received no response to data: %r" %
> > +                                           data)
> > +            return r
> > +
> > +        finally:
> > +            self._lock.release()
> > +
> > +
> > +    def cmd_obj(self, obj, timeout=20):
> > +        """
> > +        Transform a Python object to JSON, send the resulting string to the QMP
> > +        monitor, and return the response.
> > +        Unlike cmd(), return the raw response dict without performing any
> > +        checks on it.
> > +
> > +        @param obj: The object to send
> > +        @param timeout: Time duration to wait for response
> > +        @return: The response received
> > +        @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
> > +        """
> > +        return self.cmd_raw(json.dumps(obj) + "\n")
> > +
> > +
> > +    def cmd_qmp(self, cmd, args=None, id=None, timeout=20):
> > +        """
> > +        Build a QMP command from the passed arguments, send it to the monitor
> > +        and return the response.
> > +        Unlike cmd(), return the raw response dict without performing any
> > +        checks on it.
> > +
> > +        @param obj: The object to send
> > +        @param timeout: Time duration to wait for response
> > +        @return: The response received
> > +        @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
> > +        """
> > +        return self.cmd_obj(self._build_cmd(cmd, args, id), timeout)
> > +
> >  
> >      def is_responsive(self):
> >          """
> > @@ -548,7 +601,7 @@ class QMPMonitor(Monitor):
> >          @return: True if responsive, False otherwise
> >          """
> >          try:
> > -            self._get_command_output("query-status")
> > +            self.cmd("query-status")
> >              return True
> >          except MonitorError:
> >              return False
> > @@ -599,36 +652,20 @@ class QMPMonitor(Monitor):
> >  
> >      # Command wrappers
> >      # Note: all of the following functions raise exceptions in a similar manner
> > -    # to cmd() and _get_command_output().
> > -
> > -    def cmd(self, command, timeout=20):
> > -        """
> > -        Send a simple command with no parameters and return its output.
> > -        Should only be used for commands that take no parameters and are
> > -        implemented under the same name for both the human and QMP monitors.
> > -
> > -        @param command: Command to send
> > -        @param timeout: Time duration to wait for response
> > -        @return: The response to the command
> > -        @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
> > -        """
> > -        return self._get_command_output(command, timeout=timeout)
> > -
> > +    # to cmd().
> >  
> >      def quit(self):
> >          """
> >          Send "quit" and return the response.
> >          """
> > -        return self._get_command_output("quit")
> > +        return self.cmd("quit")
> >  
> > 
> >      def info(self, what):
> >          """
> >          Request info about something and return the response.
> >          """
> > -        return self._get_command_output("query-%s" % what)
> > +        return self.cmd("query-%s" % what)
> >  
> > 
> >      def query(self, what):
> > @@ -646,7 +683,7 @@ class QMPMonitor(Monitor):
> >          @return: The response to the command
> >          """
> >          args = {"filename": filename}
> > -        return self._get_command_output("screendump", args)
> > +        return self.cmd("screendump", args)
> >  
> > 
> >      def migrate(self, uri, full_copy=False, incremental_copy=False, wait=False):
> > @@ -662,7 +699,7 @@ class QMPMonitor(Monitor):
> >          args = {"uri": uri,
> >                  "blk": full_copy,
> >                  "inc": incremental_copy}
> > -        return self._get_command_output("migrate", args)
> > +        return self.cmd("migrate", args)
> >  
> > 
> >      def migrate_set_speed(self, value):
> > @@ -673,4 +710,4 @@ class QMPMonitor(Monitor):
> >          @return: The response to the command
> >          """
> >          args = {"value": value}
> > -        return self._get_command_output("migrate_set_speed", args)
> > +        return self.cmd("migrate_set_speed", args)
> 
>