On Mon, Apr 7, 2014 at 3:37 PM, Apollon Oikonomopoulos <[email protected]>wrote:
> According to the QEMU Machine Protocol Specification, the messages sent
> by QMP as a response to a command can be of two types: either an error
> message (identified by the "error" key), or a success message
> (identified by the "return" key).
>
> Since we explicitly handle errors by raising a HypervisorError, there is
> no reason to return the whole response message; instead we return only
> the response part, which is the dual behavior of accepting commands from
> the callers without the surrounding {"execute": ...} object.
>
> We also unexport the RETURN_KEY constant, which is now only used
> internally.
>
> Signed-off-by: Apollon Oikonomopoulos <[email protected]>
> ---
> lib/hypervisor/hv_kvm/__init__.py | 4 ++--
> lib/hypervisor/hv_kvm/monitor.py | 18 ++++++++++++------
> test/py/ganeti.hypervisor.hv_kvm_unittest.py | 15 ++++++++-------
> 3 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/lib/hypervisor/hv_kvm/__init__.py
> b/lib/hypervisor/hv_kvm/__init__.py
> index 227dafd..496bc6b 100644
> --- a/lib/hypervisor/hv_kvm/__init__.py
> +++ b/lib/hypervisor/hv_kvm/__init__.py
> @@ -856,10 +856,10 @@ class KVMHypervisor(hv_base.BaseHypervisor):
> try:
> qmp = QmpConnection(self._InstanceQmpMonitor(instance_name))
> qmp.connect()
> - vcpus = len(qmp.Execute("query-cpus")[qmp.RETURN_KEY])
> + vcpus = len(qmp.Execute("query-cpus"))
> # Will fail if ballooning is not enabled, but we can then just
> resort to
> # the value above.
> - mem_bytes =
> qmp.Execute("query-balloon")[qmp.RETURN_KEY][qmp.ACTUAL_KEY]
> + mem_bytes = qmp.Execute("query-balloon")[qmp.ACTUAL_KEY]
> memory = mem_bytes / 1048576
> except errors.HypervisorError:
> pass
> diff --git a/lib/hypervisor/hv_kvm/monitor.py
> b/lib/hypervisor/hv_kvm/monitor.py
> index 8176a41..b93cacd 100644
> --- a/lib/hypervisor/hv_kvm/monitor.py
> +++ b/lib/hypervisor/hv_kvm/monitor.py
> @@ -189,7 +189,7 @@ class QmpConnection(MonitorSocket):
> _FIRST_MESSAGE_KEY = "QMP"
> _EVENT_KEY = "event"
> _ERROR_KEY = "error"
> - _RETURN_KEY = RETURN_KEY = "return"
> + _RETURN_KEY = "return"
> _ACTUAL_KEY = ACTUAL_KEY = "actual"
> _ERROR_CLASS_KEY = "class"
> _ERROR_DESC_KEY = "desc"
> @@ -339,7 +339,7 @@ class QmpConnection(MonitorSocket):
>
> """
> result = self.Execute(self._QUERY_COMMANDS)
> - return frozenset(com["name"] for com in result[self._RETURN_KEY])
> + return frozenset(com["name"] for com in result)
>
> def Execute(self, command, arguments=None):
> """Executes a QMP command and returns the response of the server.
> @@ -368,8 +368,11 @@ class QmpConnection(MonitorSocket):
> message[self._ARGUMENTS_KEY] = arguments
> self._Send(message)
>
> - # Events can occur between the sending of the command and the
> reception
> - # of the response, so we need to filter out messages with the event
> key.
> + # According the the QMP specification, there are only two reply types
> to a
> + # command: either error (containing the "error" key) or success
> (containing
> + # the "return" key). There is also a third possibility, that of an
> + # (unrelated to the command) asynchronous event notification,
> identified by
> + # the "event" key.
> while True:
> response = self._Recv()
> err = response[self._ERROR_KEY]
> @@ -380,5 +383,8 @@ class QmpConnection(MonitorSocket):
> err[self._ERROR_DESC_KEY],
> err[self._ERROR_CLASS_KEY]))
>
> - elif not response[self._EVENT_KEY]:
> - return response
> + elif response[self._EVENT_KEY]:
> + # Filter-out any asynchronous events
> + continue
> +
> + return response[self._RETURN_KEY]
> diff --git a/test/py/ganeti.hypervisor.hv_kvm_unittest.py b/test/py/
> ganeti.hypervisor.hv_kvm_unittest.py
> index bc153ba..aebf1f6 100755
> --- a/test/py/ganeti.hypervisor.hv_kvm_unittest.py
> +++ b/test/py/ganeti.hypervisor.hv_kvm_unittest.py
> @@ -189,10 +189,10 @@ class TestQmp(testutils.GanetiTestCase):
> ]
>
> expected_responses = [
> - {"return": {"enabled": True, "present": True}},
> - {"return": {}},
> - {"return": [{"name": "quit"}, {"name": "eject"}]},
> - {"return": {"running": True, "singlestep": False}},
> + {"enabled": True, "present": True},
> + {},
> + [{"name": "quit"}, {"name": "eject"}],
> + {"running": True, "singlestep": False},
> ]
>
> # Set up the stub
> @@ -207,11 +207,12 @@ class TestQmp(testutils.GanetiTestCase):
>
> # Format the script
> for request, expected_response in zip(requests, expected_responses):
> - response = qmp_connection.Execute(request)
> - msg = hv_kvm.QmpMessage(expected_response)
> + response = qmp_connection.Execute(request["execute"],
> + request["arguments"])
> + self.assertEqual(response, expected_response)
> + msg = hv_kvm.QmpMessage({"return": expected_response})
> self.assertEqual(len(str(msg).splitlines()), 1,
> msg="Got multi-line message")
> - self.assertEqual(response, msg)
>
> self.assertRaises(monitor.QmpCommandNotSupported,
> qmp_connection.Execute,
> --
> 1.9.1
>
>
LGTM, thanks.
--
Thomas Thrainer | Software Engineer | [email protected] |
Google Germany GmbH
Dienerstr. 12
80331 München
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores
