From mboxrd@z Thu Jan  1 00:00:00 1970
From: Lucas Meneghel Rodrigues <lmr@redhat.com>
Subject: Re: [PATCH 2/3] QMPMonitor: Introduce the send() method and
 wrappers
Date: Mon, 18 Oct 2010 09:42:44 -0200
Message-ID: <1287402164.2571.157.camel@freedom>
References: <1287175954-19465-1-git-send-email-lcapitulino@redhat.com>
	 <1287175954-19465-3-git-send-email-lcapitulino@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Cc: autotest@test.kernel.org, kvm@vger.kernel.org
To: Luiz Capitulino <lcapitulino@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:14708 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1754661Ab0JRLms (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 18 Oct 2010 07:42:48 -0400
In-Reply-To: <1287175954-19465-3-git-send-email-lcapitulino@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Fri, 2010-10-15 at 17:52 -0300, Luiz Capitulino wrote:
> This method directly sends data to the QMP monitor and returns its
> response, without any kind of special treatment or sanity checking.
> 
> Two simple wrappers are also introduced: cmd_obj() and cmd_qmp(),
> they provide some level of automation on building QMP commands.
> 
> All three methods are going to be used by the QMP test-suite.
> 
> NOTE: The methods send() and _get_command_output() are similar, but
>       I'm out of ideas on how to properly refactor them.

^ Rather we should probably refactor all the other commands in terms of
send() rather than _get_command_output(). This can be done on a later
patch, don't worry.

> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  client/tests/kvm/kvm_monitor.py |   60 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 60 insertions(+), 0 deletions(-)
> 
> diff --git a/client/tests/kvm/kvm_monitor.py b/client/tests/kvm/kvm_monitor.py
> index d77af31..63201b4 100644
> --- a/client/tests/kvm/kvm_monitor.py
> +++ b/client/tests/kvm/kvm_monitor.py
> @@ -604,6 +604,66 @@ class QMPMonitor(Monitor):
>          return self._greeting
>  
> 
> +    def send(self, data, timeout=20):
> +        """
> +        Send data to the QMP monitor and return its response.
> +
> +        @param data: Data to send
> +        @param timeout: Time duration to wait for response
> +        @return: QMP success or error response as a dictionary
> +        @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
> +        """
> +        # XXX: This method is similar to _get_command_output(), we should
> +        # refactor it
> +        if not self._acquire_lock(20):
> +            raise MonitorLockError("Could not acquire exclusive lock to send "
> +                                   "QMP command '%s'" % cmd)
> +        try:
> +            self._read_objects()
> +            self._socket.sendall(data)
> +            end_time = time.time() + timeout
> +            while time.time() < end_time:
> +                for obj in self._read_objects():
> +                    if isinstance(obj, dict):
> +                        if "return" in obj or "error" in obj:
> +                            return obj
> +                time.sleep(0.1)
> +            else:
> +                 raise MonitorProtocolError("Received no response (data: %s)"
> +                                            % str(data))

^ Here we have an indentation with an extra space. As a suggestion to
avoid such problems, you can allways use utils/reindent.py (this path is
relative to the autotest source tree directory

utils/reindent.py client/tests/kvm/kvm_monitor.py

Also, in order to catch up little mistakes, I also suggest the pylint
wrapper that we have:

utils/run_pylint.py client/tests/kvm/kvm_monitor.py

Anyway, my pre-commit scripts run it, just pointed out because it's nice
to know those helper tools.

> +        except socket.error:
> +            raise MonitorSendError("Could not send data '%s'" % str(data))
> +        finally:
> +            self._lock.release()
> +
> +
> +    def cmd_obj(self, cmd_obj, timeout=20):
> +        """
> +        Take a Python object, transforms it in JSON and send the resulting
> +        string to the QMP monitor. Return the monitor's response.
> +
> +        @param cmd_obj: Python object to transform in JSON
> +        @param timeout: Time duration to wait for response
> +        @return: QMP success or error response as a dictionary
> +        @note: raise same exceptions as send()
> +        """
> +        return self.send(json.dumps(cmd_obj) + "\n", timeout)

^ Wouldn't be better to name this method obj_to_qmp ?

> +    def cmd_qmp(self, command, arguments=None, id=None, timeout=20):
> +        """
> +        Build a QMP command from the passed arguments, return the monitor's
> +        response.
> +
> +        @param command: QMP command name
> +        @param arguments: Arguments in the form of a Python dictionary
> +        @param id: QMP command id
> +        @return: QMP success or error response as a dictionary
> +        @note: raise same exceptions as send_cmd()
> +        """
> +        return self.cmd_obj(self._build_cmd(command, arguments, id), timeout)
> +
>      # Command wrappers
>      # Note: all of the following functions raise exceptions in a similar manner
>      # to cmd() and _get_command_output().