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.
AFAIK urwid has default signals only for inbuilt widgets like Button, Edit, and a few more. For eg the button class has a 'click' signal which we can connect to and is emitted on a button click. I had again gone through the document to check if there are any default one that we can use here. But I couldn't find any.
And yes we use UPDATE_MSG to notify the history list widget of a new message.
If not, you may use type(self) here which looks just a little cleaner.
Fixed.
+ 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().
Fixed.
+ 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.
Fixed.
+
+ 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.
Fixed.
+ except ExecuteError:
+ logging.info('Error response from server for msg: %s', msg)
Fixed
+ 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?
We can initiate a reconnect here and maybe add this request to a pending list and
prompt the user for a reissue automatically after reconnecting.
+ # 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.
Fixed.
+ 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.
Though discussed in IRC. Putting it here so that others can also see.
We are OK to create tasks with one-shot actions because the urwid loop takes care for handling the exceptions and crashing the App.
This relieves us from the pain of manually handling the task exceptions.
+ 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.
As you mentioned our primary goal is to have a proper base. I'll work on this feature once we are done reviewing this base.
+ 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.
Fixed.
+ logging.info('disconnect finished, Exiting app')
self.logger.debug
Fixed.
+ 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.
Sure will add in future revisions.
+ 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.
Fixed.
+
+ 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.
Fixed.
+ 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.
Fixed
+
+
+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.
Yup!
+# 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!
Thanks for your feedback :)