* [PATCH 0/6] python: AQMP-TUI Prototype
@ 2021-07-02 21:25 G S Niteesh Babu
2021-07-02 21:25 ` [PATCH 1/6] python: disable pylint errors for aqmp-tui G S Niteesh Babu
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: G S Niteesh Babu @ 2021-07-02 21:25 UTC (permalink / raw)
To: jsnow; +Cc: G S Niteesh Babu, Cleber Rosa, Eduardo Habkost, qemu-devel
GitLab: https://gitlab.com/niteesh.gs/qemu/-/commits/aqmp-tui-prototype-v1/
CI: https://gitlab.com/niteesh.gs/qemu/-/pipelines/330532044
Based-on: <20210701041313.1696009-1-jsnow@redhat.com>
[PATCH 00/20] python: introduce Asynchronous QMP package
This patch series introduces AQMP-TUI prototype. This prototype has been
helpfull in letting us try out different ideas and giving some insights
into things that we had to take care of in the upcoming TUI. It was also
helpfull in finding out bugs in the AQMP library.
The intent for this patch series is to get comments on the architectural
design of the prototype. These comments will lay down the foundation for
the upcoming TUI.
G S Niteesh Babu (6):
python: disable pylint errors for aqmp-tui
python: Add dependencies for AQMP TUI
python/aqmp-tui: Add AQMP TUI draft
python: add optional pygments dependency
python/aqmp-tui: add syntax highlighting
python: add entry point for aqmp-tui
python/Pipfile.lock | 20 +++
python/qemu/aqmp/aqmp_tui.py | 267 +++++++++++++++++++++++++++++++++++
python/setup.cfg | 31 +++-
3 files changed, 317 insertions(+), 1 deletion(-)
create mode 100644 python/qemu/aqmp/aqmp_tui.py
--
2.17.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/6] python: disable pylint errors for aqmp-tui
2021-07-02 21:25 [PATCH 0/6] python: AQMP-TUI Prototype G S Niteesh Babu
@ 2021-07-02 21:25 ` G S Niteesh Babu
2021-07-20 18:00 ` John Snow
2021-07-02 21:25 ` [PATCH 2/6] python: Add dependencies for AQMP TUI G S Niteesh Babu
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: G S Niteesh Babu @ 2021-07-02 21:25 UTC (permalink / raw)
To: jsnow; +Cc: G S Niteesh Babu, Cleber Rosa, Eduardo Habkost, qemu-devel
Disable missing-docstring and fixme pylint warnings.
This is because since the AQMP is just a prototype
it is currently not documented properly and lot
of todo and fixme's are still in place.
Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
python/setup.cfg | 2 ++
1 file changed, 2 insertions(+)
diff --git a/python/setup.cfg b/python/setup.cfg
index bce8807702..1a552d672a 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -89,6 +89,8 @@ ignore_missing_imports = True
# no Warning level messages displayed, use "--disable=all --enable=classes
# --disable=W".
disable=too-many-function-args, # mypy handles this with less false positives.
+ missing-docstring, # FIXME
+ fixme, # FIXME
[pylint.basic]
# Good variable names which should always be accepted, separated by a comma.
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/6] python: Add dependencies for AQMP TUI
2021-07-02 21:25 [PATCH 0/6] python: AQMP-TUI Prototype G S Niteesh Babu
2021-07-02 21:25 ` [PATCH 1/6] python: disable pylint errors for aqmp-tui G S Niteesh Babu
@ 2021-07-02 21:25 ` G S Niteesh Babu
2021-07-08 1:39 ` John Snow
2021-07-02 21:26 ` [PATCH 3/6] python/aqmp-tui: Add AQMP TUI draft G S Niteesh Babu
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: G S Niteesh Babu @ 2021-07-02 21:25 UTC (permalink / raw)
To: jsnow; +Cc: G S Niteesh Babu, Cleber Rosa, Eduardo Habkost, qemu-devel
Added dependencies for the upcoming AQMP TUI under the optional
'tui' group.
The same dependencies have also been added under the devel group
since no work around has been found for optional groups to imply
other optional groups.
Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
python/Pipfile.lock | 12 ++++++++++++
python/setup.cfg | 7 +++++++
2 files changed, 19 insertions(+)
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 8ab41a3f60..76cf1e4930 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -289,6 +289,18 @@
"markers": "python_version < '3.8'",
"version": "==3.10.0.0"
},
+ "urwid": {
+ "hashes": [
+ "sha256:588bee9c1cb208d0906a9f73c613d2bd32c3ed3702012f51efe318a3f2127eae"
+ ],
+ "version": "==2.1.2"
+ },
+ "urwid-readline": {
+ "hashes": [
+ "sha256:018020cbc864bb5ed87be17dc26b069eae2755cb29f3a9c569aac3bded1efaf4"
+ ],
+ "version": "==0.13"
+ },
"virtualenv": {
"hashes": [
"sha256:14fdf849f80dbb29a4eb6caa9875d476ee2a5cf76a5f5415fa2f1606010ab467",
diff --git a/python/setup.cfg b/python/setup.cfg
index 1a552d672a..c62803bffc 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -44,11 +44,18 @@ devel =
mypy >= 0.770
pylint >= 2.8.0
tox >= 3.18.0
+ urwid >= 2.1.2
+ urwid-readline >= 0.13
# Provides qom-fuse functionality
fuse =
fusepy >= 2.0.4
+# AQMP TUI dependencies
+tui =
+ urwid >= 2.1.2
+ urwid-readline >= 0.13
+
[options.entry_points]
console_scripts =
qom = qemu.qmp.qom:main
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/6] python/aqmp-tui: Add AQMP TUI draft
2021-07-02 21:25 [PATCH 0/6] python: AQMP-TUI Prototype G S Niteesh Babu
2021-07-02 21:25 ` [PATCH 1/6] python: disable pylint errors for aqmp-tui G S Niteesh Babu
2021-07-02 21:25 ` [PATCH 2/6] python: Add dependencies for AQMP TUI G S Niteesh Babu
@ 2021-07-02 21:26 ` G S Niteesh Babu
2021-07-08 3:20 ` John Snow
2021-07-02 21:26 ` [PATCH 4/6] python: add optional pygments dependency G S Niteesh Babu
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: G S Niteesh Babu @ 2021-07-02 21:26 UTC (permalink / raw)
To: jsnow; +Cc: G S Niteesh Babu, Cleber Rosa, Eduardo Habkost, qemu-devel
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)
+ self.window = Window(self)
+ self.address = address
+ self.aloop = asyncio.get_event_loop()
+ 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)
+
+ 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))
+ except ExecuteError:
+ logging.info('Error response from server for msg: %s', msg)
+ except ExecInterruptedError:
+ logging.info('Error server disconnected before reply')
+ # FIXME: Handle this better
+ # 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))
+ raise err
+
+ def cb_send_to_server(self, msg):
+ create_task(self._send_to_server(msg))
+
+ 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())
+
+ async def _kill_app(self):
+ # It is ok to call disconnect even in disconnect state
+ await self.disconnect()
+ logging.info('disconnect finished, Exiting app')
+ raise urwid.ExitMainLoop()
+
+ def handle_event(self, event):
+ if event['event'] == 'SHUTDOWN':
+ self.window.footer.set_text('Server shutdown')
+
+ 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')
+
+ def run(self):
+ self.aloop.set_debug(True)
+ 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()
+
+
+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
+# 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/6] python: add optional pygments dependency
2021-07-02 21:25 [PATCH 0/6] python: AQMP-TUI Prototype G S Niteesh Babu
` (2 preceding siblings ...)
2021-07-02 21:26 ` [PATCH 3/6] python/aqmp-tui: Add AQMP TUI draft G S Niteesh Babu
@ 2021-07-02 21:26 ` G S Niteesh Babu
2021-07-02 21:26 ` [PATCH 5/6] python/aqmp-tui: add syntax highlighting G S Niteesh Babu
2021-07-02 21:26 ` [PATCH 6/6] python: add entry point for aqmp-tui G S Niteesh Babu
5 siblings, 0 replies; 14+ messages in thread
From: G S Niteesh Babu @ 2021-07-02 21:26 UTC (permalink / raw)
To: jsnow; +Cc: G S Niteesh Babu, Cleber Rosa, Eduardo Habkost, qemu-devel
Added pygments as optional dependency for AQMP TUI.
This is required for the upcoming syntax highlighting feature
in AQMP TUI.
The dependency has also been added in the devel optional group.
Added mypy 'ignore_missing_imports' for pygments since it does
not have any type stubs.
Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
python/Pipfile.lock | 8 ++++++++
python/setup.cfg | 5 +++++
2 files changed, 13 insertions(+)
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 76cf1e4930..2c6d779348 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -200,6 +200,14 @@
],
"version": "==2.0.0"
},
+ "pygments": {
+ "hashes": [
+ "sha256:a18f47b506a429f6f4b9df81bb02beab9ca21d0a5fee38ed15aef65f0545519f",
+ "sha256:d66e804411278594d764fc69ec36ec13d9ae9147193a1740cd34d272ca383b8e"
+ ],
+ "markers": "python_version >= '3.5'",
+ "version": "==2.9.0"
+ },
"pylint": {
"hashes": [
"sha256:082a6d461b54f90eea49ca90fff4ee8b6e45e8029e5dbd72f6107ef84f3779c0",
diff --git a/python/setup.cfg b/python/setup.cfg
index c6d38451eb..4782fe5241 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -46,6 +46,7 @@ devel =
tox >= 3.18.0
urwid >= 2.1.2
urwid-readline >= 0.13
+ Pygments >= 2.9.0
# Provides qom-fuse functionality
fuse =
@@ -55,6 +56,7 @@ fuse =
tui =
urwid >= 2.1.2
urwid-readline >= 0.13
+ Pygments >= 2.9.0
[options.entry_points]
console_scripts =
@@ -99,6 +101,9 @@ ignore_missing_imports = True
[mypy-urwid_readline]
ignore_missing_imports = True
+[mypy-pygments]
+ignore_missing_imports = True
+
[pylint.messages control]
# Disable the message, report, category or checker with the given id(s). You
# can either give multiple identifiers separated by comma (,) or put this
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/6] python/aqmp-tui: add syntax highlighting
2021-07-02 21:25 [PATCH 0/6] python: AQMP-TUI Prototype G S Niteesh Babu
` (3 preceding siblings ...)
2021-07-02 21:26 ` [PATCH 4/6] python: add optional pygments dependency G S Niteesh Babu
@ 2021-07-02 21:26 ` G S Niteesh Babu
2021-07-02 21:26 ` [PATCH 6/6] python: add entry point for aqmp-tui G S Niteesh Babu
5 siblings, 0 replies; 14+ messages in thread
From: G S Niteesh Babu @ 2021-07-02 21:26 UTC (permalink / raw)
To: jsnow; +Cc: G S Niteesh Babu, Cleber Rosa, Eduardo Habkost, qemu-devel
Add syntax highlighting for the incoming and outgoing QMP messages.
This is achieved using the pygments module which was added in a
previous commit.
The current implementation is a really simple one which doesn't
allow for any configuration. In future this has to be improved
to allow for easier theme config using an external config of
some sort.
Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
python/qemu/aqmp/aqmp_tui.py | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
index 8e9e8ac8ff..03cb70a523 100644
--- a/python/qemu/aqmp/aqmp_tui.py
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -11,6 +11,8 @@
import logging
import signal
+from pygments import lexers
+from pygments import token as Token
import urwid
import urwid_readline
@@ -21,6 +23,16 @@
UPDATE_MSG = 'UPDATE_MSG'
+palette = [
+ (Token.Punctuation, '', '', '', 'h15,bold', 'g7'),
+ (Token.Text, '', '', '', '', 'g7'),
+ (Token.Name.Tag, '', '', '', 'bold,#f88', 'g7'),
+ (Token.Literal.Number.Integer, '', '', '', '#fa0', 'g7'),
+ (Token.Literal.String.Double, '', '', '', '#6f6', 'g7'),
+ (Token.Keyword.Constant, '', '', '', '#6af', 'g7'),
+ ('background', '', 'black', '', '', 'g7'),
+]
+
class StatusBar(urwid.Text):
"""
@@ -115,7 +127,11 @@ def __init__(self, master):
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)
+ formatted = []
+ lexer = lexers.JsonLexer() # pylint: disable=no-member
+ for token in lexer.get_tokens(msg):
+ formatted.append(token)
+ self.history.add_to_history(formatted)
class Window(urwid.Frame):
@@ -139,6 +155,7 @@ def __init__(self, address):
self.address = address
self.aloop = asyncio.get_event_loop()
self.loop = None
+ self.screen = urwid.raw_display.Screen()
super().__init__()
# Gracefully handle SIGTERM and SIGINT signals
@@ -210,10 +227,14 @@ def handle_event(self, event):
self.window.footer.set_text('Server shutdown')
def run(self):
+ self.screen.set_terminal_properties(256)
+
self.aloop.set_debug(True)
event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
- self.loop = urwid.MainLoop(self.window,
+ self.loop = urwid.MainLoop(urwid.AttrMap(self.window, 'background'),
unhandled_input=self.unhandled_input,
+ screen=self.screen,
+ palette=palette,
handle_mouse=True,
event_loop=event_loop)
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 6/6] python: add entry point for aqmp-tui
2021-07-02 21:25 [PATCH 0/6] python: AQMP-TUI Prototype G S Niteesh Babu
` (4 preceding siblings ...)
2021-07-02 21:26 ` [PATCH 5/6] python/aqmp-tui: add syntax highlighting G S Niteesh Babu
@ 2021-07-02 21:26 ` G S Niteesh Babu
2021-07-08 2:14 ` John Snow
5 siblings, 1 reply; 14+ messages in thread
From: G S Niteesh Babu @ 2021-07-02 21:26 UTC (permalink / raw)
To: jsnow; +Cc: G S Niteesh Babu, Cleber Rosa, Eduardo Habkost, qemu-devel
Add an entry point for aqmp-tui. This will allow it to be run from
the command line using "aqmp-tui -a localhost:1234"
Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
python/setup.cfg | 1 +
1 file changed, 1 insertion(+)
diff --git a/python/setup.cfg b/python/setup.cfg
index 4782fe5241..23e30185f4 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -68,6 +68,7 @@ console_scripts =
qom-fuse = qemu.qmp.qom_fuse:QOMFuse.entry_point [fuse]
qemu-ga-client = qemu.qmp.qemu_ga_client:main
qmp-shell = qemu.qmp.qmp_shell:main
+ aqmp-tui = qemu.aqmp.aqmp_tui:main
[flake8]
extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's
--
2.17.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/6] python: Add dependencies for AQMP TUI
2021-07-02 21:25 ` [PATCH 2/6] python: Add dependencies for AQMP TUI G S Niteesh Babu
@ 2021-07-08 1:39 ` John Snow
0 siblings, 0 replies; 14+ messages in thread
From: John Snow @ 2021-07-08 1:39 UTC (permalink / raw)
To: G S Niteesh Babu; +Cc: Cleber Rosa, Eduardo Habkost, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]
On Fri, Jul 2, 2021 at 5:26 PM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:
> Added dependencies for the upcoming AQMP TUI under the optional
> 'tui' group.
>
> The same dependencies have also been added under the devel group
> since no work around has been found for optional groups to imply
> other optional groups.
>
> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
>
One thing that I notice is that if you already have a .tox environment,
adding new dependencies won't cause .tox to regenerate its environments,
forcing me to delete .tox and try again.
Maybe it's possible to update the Makefile so that changes to 'setup.cfg'
will coerce Tox to rebuild?
Looks good otherwise, thank you for taking the time to re-order your
patches for the list, and congrats on your first submission :)
--js
> ---
> python/Pipfile.lock | 12 ++++++++++++
> python/setup.cfg | 7 +++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/python/Pipfile.lock b/python/Pipfile.lock
> index 8ab41a3f60..76cf1e4930 100644
> --- a/python/Pipfile.lock
> +++ b/python/Pipfile.lock
> @@ -289,6 +289,18 @@
> "markers": "python_version < '3.8'",
> "version": "==3.10.0.0"
> },
> + "urwid": {
> + "hashes": [
> +
> "sha256:588bee9c1cb208d0906a9f73c613d2bd32c3ed3702012f51efe318a3f2127eae"
> + ],
> + "version": "==2.1.2"
> + },
> + "urwid-readline": {
> + "hashes": [
> +
> "sha256:018020cbc864bb5ed87be17dc26b069eae2755cb29f3a9c569aac3bded1efaf4"
> + ],
> + "version": "==0.13"
> + },
> "virtualenv": {
> "hashes": [
>
> "sha256:14fdf849f80dbb29a4eb6caa9875d476ee2a5cf76a5f5415fa2f1606010ab467",
> diff --git a/python/setup.cfg b/python/setup.cfg
> index 1a552d672a..c62803bffc 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -44,11 +44,18 @@ devel =
> mypy >= 0.770
> pylint >= 2.8.0
> tox >= 3.18.0
> + urwid >= 2.1.2
> + urwid-readline >= 0.13
>
> # Provides qom-fuse functionality
> fuse =
> fusepy >= 2.0.4
>
> +# AQMP TUI dependencies
> +tui =
> + urwid >= 2.1.2
> + urwid-readline >= 0.13
> +
> [options.entry_points]
> console_scripts =
> qom = qemu.qmp.qom:main
> --
> 2.17.1
>
>
[-- Attachment #2: Type: text/html, Size: 3605 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 6/6] python: add entry point for aqmp-tui
2021-07-02 21:26 ` [PATCH 6/6] python: add entry point for aqmp-tui G S Niteesh Babu
@ 2021-07-08 2:14 ` John Snow
0 siblings, 0 replies; 14+ messages in thread
From: John Snow @ 2021-07-08 2:14 UTC (permalink / raw)
To: G S Niteesh Babu; +Cc: Cleber Rosa, Eduardo Habkost, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2802 bytes --]
On Fri, Jul 2, 2021 at 5:26 PM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:
> Add an entry point for aqmp-tui. This will allow it to be run from
> the command line using "aqmp-tui -a localhost:1234"
>
> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
> ---
> python/setup.cfg | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/python/setup.cfg b/python/setup.cfg
> index 4782fe5241..23e30185f4 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -68,6 +68,7 @@ console_scripts =
> qom-fuse = qemu.qmp.qom_fuse:QOMFuse.entry_point [fuse]
> qemu-ga-client = qemu.qmp.qemu_ga_client:main
> qmp-shell = qemu.qmp.qmp_shell:main
> + aqmp-tui = qemu.aqmp.aqmp_tui:main
>
I was going to suggest that you could use [tui] at the end here to protect
the script from being run when we don't have the optional dependency group
installed, but even with it, I get a pretty nasty error:
Traceback (most recent call last):
File "/home/jsnow/src/qemu/python/.pyvenv/bin/aqmp-tui", line 33, in
<module>
sys.exit(load_entry_point('qemu==0.6.1.0a1', 'console_scripts',
'aqmp-tui')())
File "/home/jsnow/src/qemu/python/.pyvenv/bin/aqmp-tui", line 25, in
importlib_load_entry_point
return next(matches).load()
File "/usr/lib64/python3.9/importlib/metadata.py", line 77, in load
module = import_module(match.group('module'))
File "/usr/lib64/python3.9/importlib/__init__.py", line 127, in
import_module
return _bootstrap._gcd_import(name[level:], package, level)
File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
File "<frozen importlib._bootstrap>", line 986, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 680, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 855, in exec_module
File "<frozen importlib._bootstrap>", line 228, in
_call_with_frames_removed
File
"/home/jsnow/src/qemu/python/.pyvenv/lib64/python3.9/site-packages/qemu/aqmp/aqmp_tui.py",
line 14, in <module>
from pygments import lexers
ModuleNotFoundError: No module named 'pygments'
It looks like this feature isn't working for me ... I'm not sure I know why.
In theory it should work:
https://setuptools.readthedocs.io/en/latest/userguide/entry_point.html#dependency-management
We might have to make our own custom entry point script that guards this a
little bit better if we can't solve this mystery. The goal is either to:
(1) Do not install an aqmp-tui script at all if we don't select the
optional TUI group, or
(2) Have the script error out early with a nice error message explaining
what optional dependencies it requires.
>
> [flake8]
> extend-ignore = E722 # Prefer pylint's bare-except checks to flake8's
> --
> 2.17.1
>
>
[-- Attachment #2: Type: text/html, Size: 3977 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/6] python/aqmp-tui: Add AQMP TUI draft
2021-07-02 21:26 ` [PATCH 3/6] python/aqmp-tui: Add AQMP TUI draft G S Niteesh Babu
@ 2021-07-08 3:20 ` John Snow
2021-07-08 17:20 ` John Snow
2021-07-13 22:08 ` Niteesh G. S.
0 siblings, 2 replies; 14+ messages in thread
From: John Snow @ 2021-07-08 3:20 UTC (permalink / raw)
To: G S Niteesh Babu; +Cc: Cleber Rosa, Eduardo Habkost, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 14931 bytes --]
On Fri, Jul 2, 2021 at 5:26 PM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:
> 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)
>
self.logger.info() here.
> + 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.
> + 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
[-- Attachment #2: Type: text/html, Size: 21402 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/6] python/aqmp-tui: Add AQMP TUI draft
2021-07-08 3:20 ` John Snow
@ 2021-07-08 17:20 ` John Snow
2021-07-13 22:08 ` Niteesh G. S.
1 sibling, 0 replies; 14+ messages in thread
From: John Snow @ 2021-07-08 17:20 UTC (permalink / raw)
To: G S Niteesh Babu; +Cc: Cleber Rosa, Eduardo Habkost, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 18301 bytes --]
On Wed, Jul 7, 2021 at 11:20 PM John Snow <jsnow@redhat.com> wrote:
>
>
> On Fri, Jul 2, 2021 at 5:26 PM G S Niteesh Babu <niteesh.gs@gmail.com>
> wrote:
>
>> 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)
>>
>
> self.logger.info() here.
>
>
>> + 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:
I was re-reading the asyncio docs last night and I was reminded that
asyncio has an exception handler mechanism that you can set:
https://docs.python.org/3/library/asyncio-eventloop.html#error-handling-api
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
>
[-- Attachment #2: Type: text/html, Size: 25635 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/6] python/aqmp-tui: Add AQMP TUI draft
2021-07-08 3:20 ` John Snow
2021-07-08 17:20 ` John Snow
@ 2021-07-13 22:08 ` Niteesh G. S.
1 sibling, 0 replies; 14+ messages in thread
From: Niteesh G. S. @ 2021-07-13 22:08 UTC (permalink / raw)
To: John Snow; +Cc: Cleber Rosa, Eduardo Habkost, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 16650 bytes --]
On Thu, Jul 8, 2021 at 8:50 AM John Snow <jsnow@redhat.com> wrote:
>
>
> On Fri, Jul 2, 2021 at 5:26 PM G S Niteesh Babu <niteesh.gs@gmail.com>
> wrote:
>
>> 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)
>>
>
> self.logger.info() here.
>
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 :)
>
> --js
>
[-- Attachment #2: Type: text/html, Size: 28414 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/6] python: disable pylint errors for aqmp-tui
2021-07-02 21:25 ` [PATCH 1/6] python: disable pylint errors for aqmp-tui G S Niteesh Babu
@ 2021-07-20 18:00 ` John Snow
2021-07-20 18:30 ` John Snow
0 siblings, 1 reply; 14+ messages in thread
From: John Snow @ 2021-07-20 18:00 UTC (permalink / raw)
To: G S Niteesh Babu; +Cc: Cleber Rosa, Eduardo Habkost, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]
On Fri, Jul 2, 2021 at 5:26 PM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:
> Disable missing-docstring and fixme pylint warnings.
> This is because since the AQMP is just a prototype
> it is currently not documented properly and lot
> of todo and fixme's are still in place.
>
> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
> ---
> python/setup.cfg | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/python/setup.cfg b/python/setup.cfg
> index bce8807702..1a552d672a 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -89,6 +89,8 @@ ignore_missing_imports = True
> # no Warning level messages displayed, use "--disable=all --enable=classes
> # --disable=W".
> disable=too-many-function-args, # mypy handles this with less false
> positives.
> + missing-docstring, # FIXME
> + fixme, # FIXME
>
You aren't actually using any FIXME statements in this branch right now
that I can see, so you don't need that suppression. It could be removed in
V3.
>
> [pylint.basic]
> # Good variable names which should always be accepted, separated by a
> comma.
> --
> 2.17.1
>
>
[-- Attachment #2: Type: text/html, Size: 1776 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/6] python: disable pylint errors for aqmp-tui
2021-07-20 18:00 ` John Snow
@ 2021-07-20 18:30 ` John Snow
0 siblings, 0 replies; 14+ messages in thread
From: John Snow @ 2021-07-20 18:30 UTC (permalink / raw)
To: G S Niteesh Babu; +Cc: Cleber Rosa, Eduardo Habkost, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]
On Tue, Jul 20, 2021 at 2:00 PM John Snow <jsnow@redhat.com> wrote:
>
>
> On Fri, Jul 2, 2021 at 5:26 PM G S Niteesh Babu <niteesh.gs@gmail.com>
> wrote:
>
>> Disable missing-docstring and fixme pylint warnings.
>> This is because since the AQMP is just a prototype
>> it is currently not documented properly and lot
>> of todo and fixme's are still in place.
>>
>> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
>> ---
>> python/setup.cfg | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/python/setup.cfg b/python/setup.cfg
>> index bce8807702..1a552d672a 100644
>> --- a/python/setup.cfg
>> +++ b/python/setup.cfg
>> @@ -89,6 +89,8 @@ ignore_missing_imports = True
>> # no Warning level messages displayed, use "--disable=all
>> --enable=classes
>> # --disable=W".
>> disable=too-many-function-args, # mypy handles this with less false
>> positives.
>> + missing-docstring, # FIXME
>> + fixme, # FIXME
>>
>
> You aren't actually using any FIXME statements in this branch right now
> that I can see, so you don't need that suppression. It could be removed in
> V3.
>
>
Sorry, I was mistaken, this will trigger on the TODO entries that you *are*
using.
--js
[-- Attachment #2: Type: text/html, Size: 2029 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-07-20 18:33 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-02 21:25 [PATCH 0/6] python: AQMP-TUI Prototype G S Niteesh Babu
2021-07-02 21:25 ` [PATCH 1/6] python: disable pylint errors for aqmp-tui G S Niteesh Babu
2021-07-20 18:00 ` John Snow
2021-07-20 18:30 ` John Snow
2021-07-02 21:25 ` [PATCH 2/6] python: Add dependencies for AQMP TUI G S Niteesh Babu
2021-07-08 1:39 ` John Snow
2021-07-02 21:26 ` [PATCH 3/6] python/aqmp-tui: Add AQMP TUI draft G S Niteesh Babu
2021-07-08 3:20 ` John Snow
2021-07-08 17:20 ` John Snow
2021-07-13 22:08 ` Niteesh G. S.
2021-07-02 21:26 ` [PATCH 4/6] python: add optional pygments dependency G S Niteesh Babu
2021-07-02 21:26 ` [PATCH 5/6] python/aqmp-tui: add syntax highlighting G S Niteesh Babu
2021-07-02 21:26 ` [PATCH 6/6] python: add entry point for aqmp-tui G S Niteesh Babu
2021-07-08 2:14 ` John Snow
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.