Added a draft of AQMP TUI.
Implements the follwing basic features:
1) Command transmission/reception.
2) Shows events asynchronously.
3) Shows server status in the bottom status bar.
Also added necessary pylint, mypy configurations
Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
python/qemu/aqmp/aqmp_tui.py | 246 +++++++++++++++++++++++++++++++++++
python/setup.cfg | 16 ++-
2 files changed, 261 insertions(+), 1 deletion(-)
create mode 100644 python/qemu/aqmp/aqmp_tui.py
diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
new file mode 100644
index 0000000000..8e9e8ac8ff
--- /dev/null
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -0,0 +1,246 @@
+# Copyright (c) 2021
+#
+# Authors:
+# Niteesh Babu G S <niteesh.gs@gmail.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later. See the COPYING file in the top-level directory.
+
+import argparse
+import asyncio
+import logging
+import signal
+
+import urwid
+import urwid_readline
+
+from .protocol import ConnectError
+from .qmp_protocol import QMP, ExecInterruptedError, ExecuteError
+from .util import create_task, pretty_traceback
+
+
+UPDATE_MSG = 'UPDATE_MSG'
+
+
+class StatusBar(urwid.Text):
+ """
+ A simple Text widget that currently only shows connection status.
+ """
+ def __init__(self, text=''):
+ super().__init__(text, align='right')
+
+
+class Editor(urwid_readline.ReadlineEdit):
+ """
+ Support urwid_readline features along with
+ history support which lacks in urwid_readline
+ """
+ def __init__(self, master):
+ super().__init__(caption='> ', multiline=True)
+ self.master = master
+ self.history = []
+ self.last_index = -1
+ self.show_history = False
+
+ def keypress(self, size, key):
+ # TODO: Add some logic for down key and clean up logic if possible.
+ # Returning None means the key has been handled by this widget
+ # which otherwise is propogated to the parent widget to be
+ # handled
+ msg = self.get_edit_text()
+ if key == 'up' and not msg:
+ # Show the history when 'up arrow' is pressed with no input text.
+ # NOTE: The show_history logic is necessary because in 'multiline'
+ # mode (which we use) 'up arrow' is used to move between lines.
+ self.show_history = True
+ last_msg = self.history[self.last_index] if self.history else ''
+ self.set_edit_text(last_msg)
+ self.edit_pos = len(last_msg)
+ self.last_index += 1
+ elif key == 'up' and self.show_history:
+ if self.last_index < len(self.history):
+ self.set_edit_text(self.history[self.last_index])
+ self.edit_pos = len(self.history[self.last_index])
+ self.last_index += 1
+ elif key == 'meta enter':
+ # When using multiline, enter inserts a new line into the editor
+ # send the input to the server on alt + enter
+ self.master.cb_send_to_server(msg)
+ self.history.insert(0, msg)
+ self.set_edit_text('')
+ self.last_index = 0
+ self.show_history = False
+ else:
+ self.show_history = False
+ self.last_index = 0
+ return super().keypress(size, key)
+ return None
+
+
+class EditorWidget(urwid.Filler):
+ """
+ Wraps CustomEdit
+ """
+ def __init__(self, master):
+ super().__init__(Editor(master), valign='top')
+
+
+class HistoryBox(urwid.ListBox):
+ """
+ Shows all the QMP message transmitted/received
+ """
+ def __init__(self, master):
+ self.master = master
+ self.history = urwid.SimpleFocusListWalker([])
+ super().__init__(self.history)
+
+ def add_to_history(self, history):
+ self.history.append(urwid.Text(history))
+ if self.history:
+ self.history.set_focus(len(self.history) - 1)
+
+
+class HistoryWindow(urwid.Frame):
+ """
+ Composes the HistoryBox and EditorWidget
+ """
+ def __init__(self, master):
+ self.master = master
+ self.editor = EditorWidget(master)
+ self.editor_widget = urwid.LineBox(self.editor)
+ self.history = HistoryBox(master)
+ self.body = urwid.Pile([('weight', 80, self.history),
+ ('weight', 10, self.editor_widget)])
+ super().__init__(self.body)
+ urwid.connect_signal(self.master, UPDATE_MSG, self.cb_add_to_history)
+
+ def cb_add_to_history(self, msg):
+ self.history.add_to_history(msg)
+
+
+class Window(urwid.Frame):
+ """
+ This is going to be the main window that is going to compose other
+ windows. In this stage it is unnecesssary but will be necessary in
+ future when we will have multiple windows and want to the switch between
+ them and display overlays
+ """
+ def __init__(self, master):
+ self.master = master
+ footer = StatusBar()
+ body = HistoryWindow(master)
+ super().__init__(body, footer=footer)
+
+
+class App(QMP):
+ def __init__(self, address):
+ urwid.register_signal(self.__class__, UPDATE_MSG)
Do we really need a custom signal? It looks like Urwid has some "default" ones... are they not sufficient? I suppose the idea here is that the 'UPDATE_MSG' signal means that we've updated the history list, so we need to re-render.
If not, you may use type(self) here which looks just a little cleaner.
+ self.window = Window(self)
+ self.address = address
+ self.aloop = asyncio.get_event_loop()
I would recommend delaying calling get_event_loop() until run(), just so that all of the loop management stuff is handled in one place. That way, the loop isn't "fixed" until we call run().
+ self.loop = None
+ super().__init__()
+
+ # Gracefully handle SIGTERM and SIGINT signals
+ cancel_signals = [signal.SIGTERM, signal.SIGINT]
+ for sig in cancel_signals:
+ self.aloop.add_signal_handler(sig, self.kill_app)
If you agree with the above comment, this needs to move into run() as well.
+
+ def _cb_outbound(self, msg):
+ urwid.emit_signal(self, UPDATE_MSG, "<-- " + str(msg))
+ return msg
+
+ def _cb_inbound(self, msg):
+ urwid.emit_signal(self, UPDATE_MSG, "--> " + str(msg))
+ return msg
+
+ async def wait_for_events(self):
+ async for event in self.events:
+ self.handle_event(event)
+
+ async def _send_to_server(self, msg):
+ # TODO: Handle more validation errors (eg: ValueError)
+ try:
+ response = await self._raw(bytes(msg, 'utf-8'))
+ logging.info('Response: %s %s', response, type(response))
You could log the responses in the inbound hook instead.
I'd also use self.logger.debug instead of
logging.info(...) so that you re-use the same logger instance.
+ except ExecuteError:
+ logging.info('Error response from server for msg: %s', msg)
+ except ExecInterruptedError:
+ logging.info('Error server disconnected before reply')
And same here.
+ # FIXME: Handle this better
What ideas do you have for handling this better? What's wrong with it right now?
+ # Show the disconnected message in the history window
+ urwid.emit_signal(self, UPDATE_MSG,
+ '{"error": "Server disconnected before reply"}')
+ self.window.footer.set_text("Server disconnected")
+ except Exception as err:
+ logging.info('Exception from _send_to_server: %s', str(err))
use self.logger.error here, since it's an unhandled error.
+ raise err
+
+ def cb_send_to_server(self, msg):
+ create_task(self._send_to_server(msg))
+
I wish we didn't have to create tasks for this, but I suppose bridging asyncio and Urwid is just simply not very pretty. One thing to keep in mind is that when you create a task without a handle like this (i.e. you aren't saving the 'task' value anywhere), if that task exits with an Exception, it will cause Python to emit that "Unhandled Exception" warning that you see ... but only once the program otherwise ends. What will end up happening in practice is that the task will die without showing you the Exception.
You might want to find a way to make Python crash a little more aggressively when an unhandled exception happens in a task, or otherwise make sure that ERROR level logging messages are visible directly in the TUI history pane, so that we can see te errors when they happen.
An update on this:
However, I don't actually know right now if this handler applies to *tasks* or if it applies only to *coroutines*. What I do know is that in the *default event loop* in asyncio, if you have a task that raises an Exception, that Exception does not by default get printed out anywhere or otherwise cause any kind of obvious failure. It is normally completely invisible until you *await on the task*.
Here's an example of what I mean:
```
async def failing_task():
raise Exception("Where does this exception go?")
async def some_function():
# This will work, it won't raise an Exception
task = create_task(failing_task())
# As of right now, failing_task won't even have run yet. We still retain control.
# Yielding here allows the scheduled task to run and immediately fail.
await asyncio.sleep(0)
# When we reach this point, the task has definitely failed already.
# However, we won't have seen any error printed anywhere yet ...
await task # <-- Any exceptions raised by the task happen HERE
````
That's what happens when you *save a reference to the task being created*, but what happens when you don't save a reference to it?
```
async def failing_task():
raise Exception("Where does this exception go?")
async def some_function():
create_task(failing_task()) # This still works just fine.
await asyncio.sleep(0) # This will give the task a chance to run. It will fail.
# When we reach this point, the task has definitely failed already.
# However, we won't have seen any error printed anywhere yet ...
# Worse, we don't have a handle to the task to find what its error was!
# Where did its exception go?
# When this program ends, you will get Python warnings complaining that there were task results that were never retrieved.
```
This is why I am concerned about the usage of create_task() without saving a reference to the created task, because it makes Exception management trickier. However, I am willing to bet (but do not KNOW), that Urwid has its own exception handlers it's already using ... I would confirm that Exceptions raised in "orphaned" tasks have reliable behavior that you can count on; it's not obvious to me at a glance that it works like we expect.
If Urwid treats Exceptions from orphaned tasks by stopping the application, that's acceptable to me -- it means that anything we forget to handle ourselves will get logged and crash the app, which is actually quite an improvement over "the error just gets silently swallowed."
Make sense?
+ def unhandled_input(self, key):
+ if key == 'esc':
+ self.kill_app()
+
+ def kill_app(self):
+ # TODO: Work on the disconnect logic
+ create_task(self._kill_app())
+
Yes, the next thing I'd like to see here is reconnection logic -- I made a little prototype in some code I gave you, but it probably needs to be touched up. I recall that my version would attempt to reconnect infinitely whenever the app was disconnected, regardless of what happened to cause the disconnection. What we likely want is only to reconnect on certain kinds of errors -- ConnectionResetError is likely a good candidate, but other kinds of problems are likely ones we want to STAY disconnected when encountering.
We also probably want some logic like num_retries, and retry_delay.
+ async def _kill_app(self):
+ # It is ok to call disconnect even in disconnect state
+ await self.disconnect()
Be aware that this raises Exceptions if the connection terminated ungracefully, i.e. the server hung up before we were expecting it. You might want to handle it (and do something related to connection retry management) first -- there are at least a few erorrs here that wouldn't be too strange to run into.
I worry that when you hit 'esc' instead of ctrl^C, you'll see different behavior here -- because ctrl+C creates a task, if this raises an exception here, I think that we won't exit -- we'll get another unhandled exception that won't show up until the app exits. I'm not confident in this, but I think you should confirm that exiting both ways works exactly like you think it does.
+ logging.info('disconnect finished, Exiting app')
self.logger.debug
+ raise urwid.ExitMainLoop()
+
+ def handle_event(self, event):
+ if event['event'] == 'SHUTDOWN':
+ self.window.footer.set_text('Server shutdown')
+
A bit spartan as an event handler, but it serves its purpose as a demonstration for the proof of concept.
It'd be nice to have the footer show a [VM: {state}] status where the state maps 1:1 with qapi/run-state.json's @RunState enumeration. I made a quick hack that you saw, but it wasn't strictly correct.
+ async def connect_server(self):
+ try:
+ await self.connect(self.address)
+ self.window.footer.set_text("Connected to {:s}".format(
+ f"{self.address[0]}:{self.address[1]}"
+ if isinstance(self.address, tuple)
+ else self.address
+ ))
+ except ConnectError as err:
+ logging.debug('Cannot connect to server %s', str(err))
+ self.window.footer.set_text('Server shutdown')
Like in other places, I wonder what happens if we have an unhandled exception here, because this is running in a task.
+
+ def run(self):
+ self.aloop.set_debug(True)
Add a debug argument to run() and default it to False, and add a --debug flag to the argparser that turns this on conditionally instead.
+ event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
+ self.loop = urwid.MainLoop(self.window,
+ unhandled_input=self.unhandled_input,
+ handle_mouse=True,
+ event_loop=event_loop)
+
+ create_task(self.wait_for_events(), self.aloop)
+ create_task(self.connect_server(), self.aloop)
+ try:
+ self.loop.run()
+ except Exception as err:
+ logging.error('%s\n%s\n', str(err), pretty_traceback())
+ raise err
+
+
+def main():
+ parser = argparse.ArgumentParser(description='AQMP TUI')
+ parser.add_argument('-a', '--address', metavar='IP:PORT', required=True,
+ help='Address of the QMP server', dest='address')
+ parser.add_argument('--log', help='Address of the QMP server',
+ dest='log_file')
+ args = parser.parse_args()
+
+ logging.basicConfig(filename=args.log_file, level=logging.DEBUG)
+
+ address = args.address.split(':')
+ address[1] = int(address[1])
+
+ App(tuple(address)).run()
I would take the address as a positional argument instead of with the '--address' flag to mimic how qmp-shell works.
+
+
+if __name__ == '__main__':
+ main() # type: ignore
diff --git a/python/setup.cfg b/python/setup.cfg
index c62803bffc..c6d38451eb 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -81,8 +81,22 @@ namespace_packages = True
# fusepy has no type stubs:
allow_subclassing_any = True
+[mypy-qemu.aqmp.aqmp_tui]
+disallow_untyped_defs = False
+disallow_incomplete_defs = False
+check_untyped_defs = False
Just keep in mind that we'll need to remove these particular ignores. The rest can stay.
+# urwid and urwid_readline have no type stubs:
+allow_subclassing_any = True
+
+# The following missing import directives are because these libraries do not
+# provide type stubs. Allow them on an as-needed basis for mypy.
[mypy-fuse]
-# fusepy has no type stubs:
+ignore_missing_imports = True
+
+[mypy-urwid]
+ignore_missing_imports = True
+
+[mypy-urwid_readline]
ignore_missing_imports = True
[pylint.messages control]
--
2.17.1
Looking good so far, let's focus on managing the connection state and making sure that Exceptions raised from task contexts are handled properly. I still need to look more deeply into the classes below App, but I wanted to give you your overdue feedback. Thank you for your patience!
--js