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

[Michael Goldish <mgoldish@redhat.com>]

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.