On 10/18/2010 08:45 PM, Luiz Capitulino wrote:
> On Mon, 18 Oct 2010 15:13:39 +0200
> Michael Goldish <mgoldish@xxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
