[KVM-AUTOTEST PATCH] KVM test: kvm_subprocess.py: reset _thread_kill_requested in kill_tail_threads()

[KVM-AUTOTEST PATCH] KVM test: kvm_subprocess.py: reset _thread_kill_requested in kill_tail_threads()

[Michael Goldish]

Reset the flag only after all relevant child threads have been joined.

Resetting the flag in one of the child threads causes a race condition, where
the other child threads will not detect that the flag was set and will
therefore not terminate.

Signed-off-by: Michael Goldish <mgoldish@redhat.com>
---
 client/tests/kvm/kvm_subprocess.py |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/client/tests/kvm/kvm_subprocess.py b/client/tests/kvm/kvm_subprocess.py
index 580f71d..8321bb3 100755
--- a/client/tests/kvm/kvm_subprocess.py
+++ b/client/tests/kvm/kvm_subprocess.py
@@ -561,6 +561,7 @@ def kill_tail_threads():
     for t in threading.enumerate():
         if hasattr(t, "name") and t.name.startswith("tail_thread"):
             t.join(10)
+    _thread_kill_requested = False
 
 
 class kvm_tail(kvm_spawn):
@@ -706,7 +707,6 @@ class kvm_tail(kvm_spawn):
             while True:
                 global _thread_kill_requested
                 if _thread_kill_requested:
-                    _thread_kill_requested = False
                     return
                 try:
                     # See if there's any data to read from the pipe
-- 
1.5.5.6

[KVM-AUTOTEST PATCH] KVM test: whql_client_install: fix retrieval of server_dns_suffix

[Michael Goldish]

Currently REG_SZ is incorrectly used if no dns suffix is set.

Signed-off-by: Michael Goldish <mgoldish@redhat.com>
---
 client/tests/kvm/tests/whql_client_install.py |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/client/tests/kvm/tests/whql_client_install.py b/client/tests/kvm/tests/whql_client_install.py
index d866df7..84b91bc 100644
--- a/client/tests/kvm/tests/whql_client_install.py
+++ b/client/tests/kvm/tests/whql_client_install.py
@@ -60,7 +60,11 @@ def run_whql_client_install(test, params, env):
     server_workgroup = server_workgroup.splitlines()[-1]
     regkey = r"HKLM\SYSTEM\CurrentControlSet\Services\Tcpip\Parameters"
     cmd = "reg query %s /v Domain" % regkey
-    server_dns_suffix = server_session.get_command_output(cmd).split()[-1]
+    o = server_session.get_command_output(cmd).strip().splitlines()[-1]
+    try:
+        server_dns_suffix = o.split(None, 2)[2]
+    except IndexError:
+        server_dns_suffix = ""
 
     # Delete the client machine from the server's data store (if it's there)
     server_session.get_command_output("cd %s" % server_studio_path)
@@ -86,7 +90,7 @@ def run_whql_client_install(test, params, env):
 
     # Set the client machine's DNS suffix
     logging.info("Setting DNS suffix to '%s'" % server_dns_suffix)
-    cmd = "reg add %s /v Domain /d %s /f" % (regkey, server_dns_suffix)
+    cmd = 'reg add %s /v Domain /d "%s" /f' % (regkey, server_dns_suffix)
     if session.get_command_status(cmd, timeout=300) != 0:
         raise error.TestError("Could not set the client's DNS suffix")
 
-- 
1.5.5.6

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

[Michael Goldish]

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

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)
-- 
1.5.5.6

[KVM-AUTOTEST PATCH] KVM test: kvm_monitor.py: add __str__() method to QMPCmdError exception

[Michael Goldish]

Signed-off-by: Michael Goldish <mgoldish@redhat.com>
---
 client/tests/kvm/kvm_monitor.py |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
index b887060..40be77d 100644
--- a/client/tests/kvm/kvm_monitor.py
+++ b/client/tests/kvm/kvm_monitor.py
@@ -38,7 +38,9 @@ class MonitorNotSupportedError(MonitorError):
 
 
 class QMPCmdError(MonitorError):
-    pass
+    def __str__(self):
+        return ("QMP command '%s' failed (arguments: %r, error message: %r)" %
+                tuple(self.args))
 
 
 class Monitor:
@@ -501,8 +503,8 @@ class QMPMonitor(Monitor):
         @raise MonitorSendError: Raised if the command cannot be sent
         @raise MonitorProtocolError: Raised if no response is received
         @raise QMPCmdError: Raised if the response is an error message
-                (the exception's args are (msg, data) where msg is a string and
-                data is the error data)
+                (the exception's args are (cmd, args, data) where data is the
+                error data)
         """
         if not self._acquire_lock(20):
             raise MonitorLockError("Could not acquire exclusive lock to send "
@@ -524,7 +526,7 @@ class QMPMonitor(Monitor):
             if "return" in r:
                 return r["return"]
             if "error" in r:
-                raise QMPCmdError("QMP command '%s' failed" % cmd, r["error"])
+                raise QMPCmdError(cmd, args, r["error"])
 
         finally:
             self._lock.release()
-- 
1.5.5.6

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

[Lucas Meneghel Rodrigues]

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?	

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

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

[Luiz Capitulino]

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

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

[Luiz Capitulino]

On Mon, 18 Oct 2010 15:13:39 +0200
Michael Goldish <mgoldish@redhat.com> 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.

I did a quick review and it looks good to me. I have only one comment
(which should not prevent this from being merged):

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

Is there a reason not to use select? Like the following example (which changes
the HumanMonitor class, not the QMPMonitor one):

diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
index 6bff07f..cfe05bf 100644
--- a/client/tests/kvm/kvm_monitor.py
+++ b/client/tests/kvm/kvm_monitor.py
@@ -4,7 +4,7 @@ Interfaces to the QEMU monitor.
 @copyright: 2008-2010 Red Hat Inc.
 """
 
-import socket, time, threading, logging
+import socket, time, threading, logging, select
 import kvm_utils
 try:
     import json
@@ -58,7 +58,6 @@ class Monitor:
         self.filename = filename
         self._lock = threading.RLock()
         self._socket = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
-        self._socket.setblocking(False)
 
         try:
             self._socket.connect(filename)
@@ -105,13 +104,14 @@ class Monitor:
     def _recvall(self):
         s = ""
         while True:
-            try:
+            i, o, e = select.select([self._socket], [], [], 0)
+            if len(i) > 0:
                 data = self._socket.recv(1024)
-            except socket.error:
-                break
-            if not data:
+                if not data:
+                    break
+                s += data
+            else:
                 break
-            s += data
         return s
 
 
@@ -158,19 +158,19 @@ class HumanMonitor(Monitor):
     # Private methods
 
     def _read_up_to_qemu_prompt(self, timeout=20):
-        o = ""
-        end_time = time.time() + timeout
-        while time.time() < end_time:
-            try:
+        buf = ""
+        while True:
+            i, o, e = select.select([self._socket], [], [], timeout)
+            if len(i) > 0:
                 data = self._socket.recv(1024)
-                if not data:
-                    break
-                o += data
-                if o.splitlines()[-1].split()[-1] == "(qemu)":
-                    return True, "\n".join(o.splitlines()[:-1])
-            except (socket.error, IndexError):
-                time.sleep(0.01)
-        return False, "\n".join(o.splitlines())
+                buf += data
+                try:
+                    if buf.splitlines()[-1].split()[-1] == "(qemu)":
+                        return True, "\n".join(buf.splitlines()[:-1])
+                except IndexError:
+                    continue
+            else:
+                return False, "\n".join(buf.splitlines())
 
 
     def _send_command(self, command):


Besides of getting a slightly simpler code, select has the major benefit
of not sleeping when there's data available. This is a problem with the
current implementation: if data is available before or during the sleep()
call, you'll sit there for the specified timeout, no matter what.

I'm not exactly saying we should change right now, maybe those 0.01 seconds
won't matter in practice (and I haven't measured anything), but I'd like
to know if you've considered using select.

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

[Lucas Meneghel Rodrigues]

On Mon, 2010-10-18 at 16:25 -0200, Luiz Capitulino wrote:
> 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.

Ok, commited:

http://autotest.kernel.org/changeset/4866

About the select() suggestion, I think it does make sense to use it, but
I also want to hear from Michael if he had found any problems with using
select while testing the patches. So the patch is integrated, and you
can write your v2 of the patches based on it, and any further
improvement is possible, once the base function names are already there.

Cheers,

Lucas

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

[Michael Goldish]

On 10/18/2010 08:45 PM, Luiz Capitulino wrote:
> On Mon, 18 Oct 2010 15:13:39 +0200
> Michael Goldish <mgoldish@redhat.com> 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.
> 
> I did a quick review and it looks good to me. I have only one comment
> (which should not prevent this from being merged):
> 
>> -    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)
> 
> Is there a reason not to use select? Like the following example (which changes
> the HumanMonitor class, not the QMPMonitor one):
> 
> diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
> index 6bff07f..cfe05bf 100644
> --- a/client/tests/kvm/kvm_monitor.py
> +++ b/client/tests/kvm/kvm_monitor.py
> @@ -4,7 +4,7 @@ Interfaces to the QEMU monitor.
>  @copyright: 2008-2010 Red Hat Inc.
>  """
>  
> -import socket, time, threading, logging
> +import socket, time, threading, logging, select
>  import kvm_utils
>  try:
>      import json
> @@ -58,7 +58,6 @@ class Monitor:
>          self.filename = filename
>          self._lock = threading.RLock()
>          self._socket = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
> -        self._socket.setblocking(False)
>  
>          try:
>              self._socket.connect(filename)
> @@ -105,13 +104,14 @@ class Monitor:
>      def _recvall(self):
>          s = ""
>          while True:
> -            try:
> +            i, o, e = select.select([self._socket], [], [], 0)
> +            if len(i) > 0:
>                  data = self._socket.recv(1024)
> -            except socket.error:
> -                break
> -            if not data:
> +                if not data:
> +                    break
> +                s += data
> +            else:
>                  break
> -            s += data
>          return s
>  
>  
> @@ -158,19 +158,19 @@ class HumanMonitor(Monitor):
>      # Private methods
>  
>      def _read_up_to_qemu_prompt(self, timeout=20):
> -        o = ""
> -        end_time = time.time() + timeout
> -        while time.time() < end_time:
> -            try:
> +        buf = ""
> +        while True:
> +            i, o, e = select.select([self._socket], [], [], timeout)
> +            if len(i) > 0:
>                  data = self._socket.recv(1024)
> -                if not data:
> -                    break
> -                o += data
> -                if o.splitlines()[-1].split()[-1] == "(qemu)":
> -                    return True, "\n".join(o.splitlines()[:-1])
> -            except (socket.error, IndexError):
> -                time.sleep(0.01)
> -        return False, "\n".join(o.splitlines())
> +                buf += data
> +                try:
> +                    if buf.splitlines()[-1].split()[-1] == "(qemu)":
> +                        return True, "\n".join(buf.splitlines()[:-1])
> +                except IndexError:
> +                    continue
> +            else:
> +                return False, "\n".join(buf.splitlines())
>  
>  
>      def _send_command(self, command):
> 
> 
> Besides of getting a slightly simpler code, select has the major benefit
> of not sleeping when there's data available. This is a problem with the
> current implementation: if data is available before or during the sleep()
> call, you'll sit there for the specified timeout, no matter what.
> 
> I'm not exactly saying we should change right now, maybe those 0.01 seconds
> won't matter in practice (and I haven't measured anything), but I'd like
> to know if you've considered using select.

I've considered it, but I don't quite remember why I didn't use it in
the first place.  Let me re-read the code more thoroughly and get back
to you.