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