[PATCH v3 00/13] AQMP TUI Draft

[PATCH v3 00/13] AQMP TUI Draft

[G S Niteesh Babu]

Hello all,

Gitlab: https://gitlab.com/niteesh.gs/qemu/-/commits/aqmp-tui-prototype-v3
CI: https://gitlab.com/niteesh.gs/qemu/-/pipelines/345738265

Major revision since V2:
1) Refined QMP connection manager.
2) Added static typing.
3) Allow highlighting specific messages in history box.
4) Allow copying of QMP message from TUI using keyboard shortcut alt-c.

G S Niteesh Babu (13):
  python/aqmp: Fix wait_closed work-around for python 3.6
  python: disable pylint errors for aqmp-tui
  python: Add dependencies for AQMP TUI
  python/aqmp-tui: Add AQMP TUI draft
  python: add entry point for aqmp-tui
  python/aqmp-tui: Added type annotations for aqmp-tui
  python: add optional pygments dependency
  python/aqmp-tui: add syntax highlighting
  python/aqmp-tui: Add QMP connection manager
  python/aqmp-tui: Add scrolling to history box
  python/aqmp-tui: Add ability to highlight messages
  python/aqmp-tui: Add pyperclip dependency
  python/aqmp-tui: Allow copying message from TUI

 python/Pipfile.lock          |  42 +++
 python/qemu/aqmp/aqmp_tui.py | 519 +++++++++++++++++++++++++++++++++++
 python/qemu/aqmp/util.py     |  12 +-
 python/setup.cfg             |  34 ++-
 4 files changed, 605 insertions(+), 2 deletions(-)
 create mode 100644 python/qemu/aqmp/aqmp_tui.py

-- 
2.17.1

[PATCH v3 01/13] python/aqmp: Fix wait_closed work-around for python 3.6

[G S Niteesh Babu]

Before this patch the wait_closed work-around for python 3.6
fails during disconnect.
This is a temproray work around for which might be fixed in the
future or will be completely removed when the minimum python
version is raised to 3.7.

This patch was originally written by John Snow <jsnow@redhat.com>

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
 python/qemu/aqmp/util.py | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/python/qemu/aqmp/util.py b/python/qemu/aqmp/util.py
index de0df44cbd..eaa5fc7d5f 100644
--- a/python/qemu/aqmp/util.py
+++ b/python/qemu/aqmp/util.py
@@ -134,7 +134,17 @@ def is_closing(writer: asyncio.StreamWriter) -> bool:
 
     while not transport.is_closing():
         await asyncio.sleep(0)
-    await flush(writer)
+
+    # This is an ugly workaround, but it's the best I can come up with.
+    sock = transport.get_extra_info('socket')
+
+    if sock is None:
+        # Our transport doesn't have a socket? ...
+        # Nothing we can reasonably do.
+        return
+
+    while sock.fileno() != -1:
+        await asyncio.sleep(0)
 
 
 def asyncio_run(coro: Coroutine[Any, Any, T], *, debug: bool = False) -> T:
-- 
2.17.1

[PATCH v3 02/13] python: disable pylint errors for aqmp-tui

[G S Niteesh Babu]

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 2573cd7bfb..7a30dd5b09 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -90,6 +90,8 @@ ignore_missing_imports = True
 # --disable=W".
 disable=too-many-function-args,  # mypy handles this with less false positives.
         no-member,  # mypy also handles this better.
+        missing-docstring, # FIXME
+        fixme, # FIXME
 
 [pylint.basic]
 # Good variable names which should always be accepted, separated by a comma.
-- 
2.17.1

[PATCH v3 03/13] python: Add dependencies for AQMP TUI

[G S Niteesh Babu]

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    |  8 ++++++++
 2 files changed, 20 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 7a30dd5b09..d106a0ed7a 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
@@ -133,5 +140,6 @@ allowlist_externals = make
 deps =
     .[devel]
     .[fuse]  # Workaround to trigger tox venv rebuild
+    .[tui]   # Workaround to trigger tox venv rebuild
 commands =
     make check
-- 
2.17.1

[PATCH v3 04/13] python/aqmp-tui: Add AQMP TUI draft

[G S Niteesh Babu]

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 | 333 +++++++++++++++++++++++++++++++++++
 python/setup.cfg             |  16 +-
 2 files changed, 348 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..ec9eba0aa7
--- /dev/null
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -0,0 +1,333 @@
+# 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
+from logging import Handler
+import signal
+
+import urwid
+import urwid_readline
+
+from ..qmp import QEMUMonitorProtocol, QMPBadPortError
+from .message import DeserializationError, Message, UnexpectedTypeError
+from .protocol import ConnectError
+from .qmp_client import ExecInterruptedError, QMPClient
+from .util import create_task, pretty_traceback
+
+
+UPDATE_MSG = 'UPDATE_MSG'
+
+# Using root logger to enable all loggers under qemu and asyncio
+LOGGER = logging.getLogger()
+
+
+def format_json(msg):
+    """
+    Formats given multiline JSON message into a single line message.
+    Converting into single line is more asthetically pleasing when looking
+    along with error messages compared to multiline JSON.
+    """
+    # FIXME: Use better formatting mechanism. Might break at more complex JSON
+    # data.
+    msg = msg.replace('\n', '')
+    words = msg.split(' ')
+    words = [word for word in words if word != '']
+    return ' '.join(words)
+
+
+class App(QMPClient):
+    def __init__(self, address):
+        urwid.register_signal(type(self), UPDATE_MSG)
+        self.window = Window(self)
+        self.address = address
+        self.aloop = None
+        super().__init__()
+
+    def add_to_history(self, msg):
+        urwid.emit_signal(self, UPDATE_MSG, msg)
+
+    def _cb_outbound(self, msg):
+        # FIXME: I think the ideal way to omit these messages during in-TUI
+        # logging will be to add a filter to the logger. We can use regex to
+        # filter out messages starting with 'Request:' or 'Response:' but I
+        # think a better approach will be encapsulate the message in an object
+        # and filter based on the object. Encapsulation of the message will
+        # also be necessary when we want different formatting of messages
+        # inside TUI.
+        handler = LOGGER.handlers[0]
+        if not isinstance(handler, TUILogHandler):
+            LOGGER.debug('Request: %s', str(msg))
+        self.add_to_history('<-- ' + str(msg))
+        return msg
+
+    def _cb_inbound(self, msg):
+        handler = LOGGER.handlers[0]
+        if not isinstance(handler, TUILogHandler):
+            LOGGER.debug('Response: %s', str(msg))
+        self.add_to_history('--> ' + 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, raw_msg):
+        # FIXME: Format the raw_msg in history view to one line. It is not
+        # pleasing to see multiple lines JSON object with an error statement.
+        try:
+            msg = Message(bytes(raw_msg, encoding='utf-8'))
+            # Format multiline json into a single line JSON, since it is more
+            # pleasing to look along with err message in TUI.
+            raw_msg = self.format_json(raw_msg)
+            await self._raw(msg, assign_id='id' not in msg)
+        except (ValueError, TypeError) as err:
+            LOGGER.info('Invalid message: %s', str(err))
+            self.add_to_history(f'{raw_msg}: {err}')
+        except (DeserializationError, UnexpectedTypeError) as err:
+            LOGGER.info('Invalid message: %s', err.error_message)
+            self.add_to_history(f'{raw_msg}: {err.error_message}')
+        except ExecInterruptedError:
+            LOGGER.info('Error server disconnected before reply')
+            urwid.emit_signal(self, UPDATE_MSG,
+                              '{"error": "Server disconnected before reply"}')
+            self._set_status("Server disconnected")
+        except Exception as err:
+            LOGGER.error('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
+        try:
+            await self.disconnect()
+            LOGGER.debug('Disconnect finished. Exiting app')
+        except Exception as err:
+            LOGGER.info('_kill_app: %s', str(err))
+            # Let the app crash after providing a proper stack trace
+            raise err
+        raise urwid.ExitMainLoop()
+
+    def handle_event(self, event):
+        # FIXME: Consider all states present in qapi/run-state.json
+        if event['event'] == 'SHUTDOWN':
+            self._set_status('Server shutdown')
+
+    def _set_status(self, msg: str) -> None:
+        self.window.footer.set_text(msg)
+
+    def _get_formatted_address(self) -> str:
+        addr = f'{self.address}'
+        if isinstance(self.address, tuple):
+            host, port = self.address
+            addr = f'{host}:{port}'
+        return addr
+
+    async def connect_server(self):
+        try:
+            await self.connect(self.address)
+            addr = self._get_formatted_address()
+            self._set_status(f'Connected to {addr}')
+        except ConnectError as err:
+            LOGGER.info('connect_server: ConnectError %s', str(err))
+            self._set_status('Server shutdown')
+
+    def run(self, debug=False):
+        self.aloop = asyncio.get_event_loop()
+        self.aloop.set_debug(debug)
+
+        # 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)
+
+        event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
+        main_loop = urwid.MainLoop(urwid.AttrMap(self.window, 'background'),
+                                   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:
+            main_loop.run()
+        except Exception as err:
+            LOGGER.error('%s\n%s\n', str(err), pretty_traceback())
+            raise err
+
+
+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_widget = EditorWidget(master)
+        self.editor = urwid.LineBox(self.editor_widget)
+        self.history = HistoryBox(master)
+        self.body = urwid.Pile([('weight', 80, self.history),
+                                ('weight', 20, self.editor)])
+        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 TUILogHandler(Handler):
+    def __init__(self, tui):
+        super().__init__()
+        self.tui = tui
+
+    def emit(self, record):
+        level = record.levelname
+        msg = record.getMessage()
+        msg = f'[{level}]: {msg}'
+        self.tui.add_to_history(msg)
+
+
+def main():
+    parser = argparse.ArgumentParser(description='AQMP TUI')
+    parser.add_argument('qmp_server', help='Address of the QMP server'
+                        '< UNIX socket path | TCP addr:port >')
+    parser.add_argument('--log-file', help='The Log file name')
+    parser.add_argument('--log-level', default='WARNING',
+                        help='Log level <CRITICAL|ERROR|WARNING|INFO|DEBUG|>')
+    parser.add_argument('--asyncio-debug', action='store_true',
+                        help='Enable debug mode for asyncio loop'
+                        'Generates lot of output, makes TUI unusable when'
+                        'logs are logged in the TUI itself.'
+                        'Use only when logging to a file')
+    args = parser.parse_args()
+
+    try:
+        address = QEMUMonitorProtocol.parse_address(args.qmp_server)
+    except QMPBadPortError as err:
+        parser.error(err)
+
+    app = App(address)
+
+    if args.log_file:
+        LOGGER.addHandler(logging.FileHandler(args.log_file))
+    else:
+        LOGGER.addHandler(TUILogHandler(app))
+
+    log_level = logging.getLevelName(args.log_level)
+    # getLevelName returns 'Level {log_level}' when a invalid level is passed.
+    if log_level == f'Level {args.log_level}':
+        parser.error('Invalid log level')
+    LOGGER.setLevel(log_level)
+
+    app.run(args.asyncio_debug)
+
+
+if __name__ == '__main__':
+    main()  # type: ignore
diff --git a/python/setup.cfg b/python/setup.cfg
index d106a0ed7a..50f9894468 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

[PATCH v3 05/13] python: add entry point for aqmp-tui

[G S Niteesh Babu]

Add an entry point for aqmp-tui. This will allow it to be run from
the command line using "aqmp-tui localhost:1234"
More options available in the TUI can be found using "aqmp-tui -h"

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 50f9894468..8cd9ac0d81 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -66,6 +66,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 [tui]
 
 [flake8]
 extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
-- 
2.17.1

[PATCH v3 06/13] python/aqmp-tui: Added type annotations for aqmp-tui

[G S Niteesh Babu]

This patch adds type annotations for aqmp-tui using
the mypy library.

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
 python/qemu/aqmp/aqmp_tui.py | 79 ++++++++++++++++++++----------------
 python/setup.cfg             |  3 --
 2 files changed, 43 insertions(+), 39 deletions(-)

diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
index ec9eba0aa7..ab9ada793a 100644
--- a/python/qemu/aqmp/aqmp_tui.py
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -9,8 +9,15 @@
 import argparse
 import asyncio
 import logging
-from logging import Handler
+from logging import Handler, LogRecord
 import signal
+from typing import (
+    Any,
+    List,
+    Optional,
+    Tuple,
+    Union,
+)
 
 import urwid
 import urwid_readline
@@ -22,13 +29,13 @@
 from .util import create_task, pretty_traceback
 
 
-UPDATE_MSG = 'UPDATE_MSG'
+UPDATE_MSG: str = 'UPDATE_MSG'
 
 # Using root logger to enable all loggers under qemu and asyncio
 LOGGER = logging.getLogger()
 
 
-def format_json(msg):
+def format_json(msg: str) -> str:
     """
     Formats given multiline JSON message into a single line message.
     Converting into single line is more asthetically pleasing when looking
@@ -43,17 +50,17 @@ def format_json(msg):
 
 
 class App(QMPClient):
-    def __init__(self, address):
+    def __init__(self, address: Union[str, Tuple[str, int]]) -> None:
         urwid.register_signal(type(self), UPDATE_MSG)
         self.window = Window(self)
         self.address = address
-        self.aloop = None
+        self.aloop: Optional[Any] = None  # FIXME: Use more concrete type.
         super().__init__()
 
-    def add_to_history(self, msg):
+    def add_to_history(self, msg: str) -> None:
         urwid.emit_signal(self, UPDATE_MSG, msg)
 
-    def _cb_outbound(self, msg):
+    def _cb_outbound(self, msg: Message) -> Message:
         # FIXME: I think the ideal way to omit these messages during in-TUI
         # logging will be to add a filter to the logger. We can use regex to
         # filter out messages starting with 'Request:' or 'Response:' but I
@@ -67,25 +74,25 @@ def _cb_outbound(self, msg):
         self.add_to_history('<-- ' + str(msg))
         return msg
 
-    def _cb_inbound(self, msg):
+    def _cb_inbound(self, msg: Message) -> Message:
         handler = LOGGER.handlers[0]
         if not isinstance(handler, TUILogHandler):
             LOGGER.debug('Response: %s', str(msg))
         self.add_to_history('--> ' + str(msg))
         return msg
 
-    async def wait_for_events(self):
+    async def wait_for_events(self) -> None:
         async for event in self.events:
             self.handle_event(event)
 
-    async def _send_to_server(self, raw_msg):
+    async def _send_to_server(self, raw_msg: str) -> None:
         # FIXME: Format the raw_msg in history view to one line. It is not
         # pleasing to see multiple lines JSON object with an error statement.
         try:
             msg = Message(bytes(raw_msg, encoding='utf-8'))
             # Format multiline json into a single line JSON, since it is more
             # pleasing to look along with err message in TUI.
-            raw_msg = self.format_json(raw_msg)
+            raw_msg = format_json(raw_msg)
             await self._raw(msg, assign_id='id' not in msg)
         except (ValueError, TypeError) as err:
             LOGGER.info('Invalid message: %s', str(err))
@@ -102,18 +109,18 @@ def _cb_inbound(self, msg):
             LOGGER.error('Exception from _send_to_server: %s', str(err))
             raise err
 
-    def cb_send_to_server(self, msg):
+    def cb_send_to_server(self, msg: str) -> None:
         create_task(self._send_to_server(msg))
 
-    def unhandled_input(self, key):
+    def unhandled_input(self, key: str) -> None:
         if key == 'esc':
             self.kill_app()
 
-    def kill_app(self):
+    def kill_app(self) -> None:
         # TODO: Work on the disconnect logic
         create_task(self._kill_app())
 
-    async def _kill_app(self):
+    async def _kill_app(self) -> None:
         # It is ok to call disconnect even in disconnect state
         try:
             await self.disconnect()
@@ -124,7 +131,7 @@ def kill_app(self):
             raise err
         raise urwid.ExitMainLoop()
 
-    def handle_event(self, event):
+    def handle_event(self, event: Message) -> None:
         # FIXME: Consider all states present in qapi/run-state.json
         if event['event'] == 'SHUTDOWN':
             self._set_status('Server shutdown')
@@ -139,7 +146,7 @@ def _get_formatted_address(self) -> str:
             addr = f'{host}:{port}'
         return addr
 
-    async def connect_server(self):
+    async def connect_server(self) -> None:
         try:
             await self.connect(self.address)
             addr = self._get_formatted_address()
@@ -148,7 +155,7 @@ def _get_formatted_address(self) -> str:
             LOGGER.info('connect_server: ConnectError %s', str(err))
             self._set_status('Server shutdown')
 
-    def run(self, debug=False):
+    def run(self, debug: bool = False) -> None:
         self.aloop = asyncio.get_event_loop()
         self.aloop.set_debug(debug)
 
@@ -176,7 +183,7 @@ class StatusBar(urwid.Text):
     """
     A simple Text widget that currently only shows connection status.
     """
-    def __init__(self, text=''):
+    def __init__(self, text: str = ''):
         super().__init__(text, align='right')
 
 
@@ -185,14 +192,14 @@ class Editor(urwid_readline.ReadlineEdit):
     Support urwid_readline features along with
     history support which lacks in urwid_readline
     """
-    def __init__(self, master):
+    def __init__(self, master: App) -> None:
         super().__init__(caption='> ', multiline=True)
         self.master = master
-        self.history = []
-        self.last_index = -1
-        self.show_history = False
+        self.history: List[str] = []
+        self.last_index: int = -1
+        self.show_history: bool = False
 
-    def keypress(self, size, key):
+    def keypress(self, size: Tuple[int, int], key: str) -> Optional[str]:
         # 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
@@ -223,7 +230,7 @@ def keypress(self, size, key):
         else:
             self.show_history = False
             self.last_index = 0
-            return super().keypress(size, key)
+            return super().keypress(size, key)  # type: ignore
         return None
 
 
@@ -231,7 +238,7 @@ class EditorWidget(urwid.Filler):
     """
     Wraps CustomEdit
     """
-    def __init__(self, master):
+    def __init__(self, master: App) -> None:
         super().__init__(Editor(master), valign='top')
 
 
@@ -239,12 +246,12 @@ class HistoryBox(urwid.ListBox):
     """
     Shows all the QMP message transmitted/received
     """
-    def __init__(self, master):
+    def __init__(self, master: App) -> None:
         self.master = master
         self.history = urwid.SimpleFocusListWalker([])
         super().__init__(self.history)
 
-    def add_to_history(self, history):
+    def add_to_history(self, history: str) -> None:
         self.history.append(urwid.Text(history))
         if self.history:
             self.history.set_focus(len(self.history) - 1)
@@ -254,7 +261,7 @@ class HistoryWindow(urwid.Frame):
     """
     Composes the HistoryBox and EditorWidget
     """
-    def __init__(self, master):
+    def __init__(self, master: App) -> None:
         self.master = master
         self.editor_widget = EditorWidget(master)
         self.editor = urwid.LineBox(self.editor_widget)
@@ -264,7 +271,7 @@ def __init__(self, master):
         super().__init__(self.body)
         urwid.connect_signal(self.master, UPDATE_MSG, self.cb_add_to_history)
 
-    def cb_add_to_history(self, msg):
+    def cb_add_to_history(self, msg: str) -> None:
         self.history.add_to_history(msg)
 
 
@@ -275,7 +282,7 @@ class Window(urwid.Frame):
     future when we will have multiple windows and want to the switch between
     them and display overlays
     """
-    def __init__(self, master):
+    def __init__(self, master: App) -> None:
         self.master = master
         footer = StatusBar()
         body = HistoryWindow(master)
@@ -283,18 +290,18 @@ def __init__(self, master):
 
 
 class TUILogHandler(Handler):
-    def __init__(self, tui):
+    def __init__(self, tui: App) -> None:
         super().__init__()
         self.tui = tui
 
-    def emit(self, record):
+    def emit(self, record: LogRecord) -> None:
         level = record.levelname
         msg = record.getMessage()
         msg = f'[{level}]: {msg}'
         self.tui.add_to_history(msg)
 
 
-def main():
+def main() -> None:
     parser = argparse.ArgumentParser(description='AQMP TUI')
     parser.add_argument('qmp_server', help='Address of the QMP server'
                         '< UNIX socket path | TCP addr:port >')
@@ -311,7 +318,7 @@ def main():
     try:
         address = QEMUMonitorProtocol.parse_address(args.qmp_server)
     except QMPBadPortError as err:
-        parser.error(err)
+        parser.error(str(err))
 
     app = App(address)
 
@@ -330,4 +337,4 @@ def main():
 
 
 if __name__ == '__main__':
-    main()  # type: ignore
+    main()
diff --git a/python/setup.cfg b/python/setup.cfg
index 8cd9ac0d81..11c6240aba 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -83,9 +83,6 @@ namespace_packages = True
 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
 
-- 
2.17.1

[PATCH v3 07/13] python: add optional pygments dependency

[G S Niteesh Babu]

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 11c6240aba..bbb7306c3d 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 =
@@ -97,6 +99,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

[PATCH v3 08/13] python/aqmp-tui: add syntax highlighting

[G S Niteesh Babu]

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 | 52 +++++++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 12 deletions(-)

diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
index ab9ada793a..0d5ec62cb7 100644
--- a/python/qemu/aqmp/aqmp_tui.py
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -19,6 +19,8 @@
     Union,
 )
 
+from pygments import lexers
+from pygments import token as Token
 import urwid
 import urwid_readline
 
@@ -35,6 +37,22 @@
 LOGGER = logging.getLogger()
 
 
+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'),
+    ('DEBUG', '', '', '', '#ddf', 'g7'),
+    ('INFO', '', '', '', 'g100', 'g7'),
+    ('WARNING', '', '', '', '#ff6', 'g7'),
+    ('ERROR', '', '', '', '#a00', 'g7'),
+    ('CRITICAL', '', '', '', '#a00', 'g7'),
+    ('background', '', 'black', '', '', 'g7'),
+]
+
+
 def format_json(msg: str) -> str:
     """
     Formats given multiline JSON message into a single line message.
@@ -57,17 +75,14 @@ def __init__(self, address: Union[str, Tuple[str, int]]) -> None:
         self.aloop: Optional[Any] = None  # FIXME: Use more concrete type.
         super().__init__()
 
-    def add_to_history(self, msg: str) -> None:
-        urwid.emit_signal(self, UPDATE_MSG, msg)
+    def add_to_history(self, msg: str, level: Optional[str] = None) -> None:
+        urwid.emit_signal(self, UPDATE_MSG, msg, level)
 
     def _cb_outbound(self, msg: Message) -> Message:
         # FIXME: I think the ideal way to omit these messages during in-TUI
-        # logging will be to add a filter to the logger. We can use regex to
-        # filter out messages starting with 'Request:' or 'Response:' but I
-        # think a better approach will be encapsulate the message in an object
-        # and filter based on the object. Encapsulation of the message will
-        # also be necessary when we want different formatting of messages
-        # inside TUI.
+        # logging will be to add a filter to the logger. We can use
+        # regex/startswith to filter out messages starting with 'Request:' or
+        # 'Response:'. If possible please suggest other ideas.
         handler = LOGGER.handlers[0]
         if not isinstance(handler, TUILogHandler):
             LOGGER.debug('Request: %s', str(msg))
@@ -156,6 +171,9 @@ def _get_formatted_address(self) -> str:
             self._set_status('Server shutdown')
 
     def run(self, debug: bool = False) -> None:
+        screen = urwid.raw_display.Screen()
+        screen.set_terminal_properties(256)
+
         self.aloop = asyncio.get_event_loop()
         self.aloop.set_debug(debug)
 
@@ -167,6 +185,8 @@ def run(self, debug: bool = False) -> None:
         event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
         main_loop = urwid.MainLoop(urwid.AttrMap(self.window, 'background'),
                                    unhandled_input=self.unhandled_input,
+                                   screen=screen,
+                                   palette=palette,
                                    handle_mouse=True,
                                    event_loop=event_loop)
 
@@ -251,7 +271,8 @@ def __init__(self, master: App) -> None:
         self.history = urwid.SimpleFocusListWalker([])
         super().__init__(self.history)
 
-    def add_to_history(self, history: str) -> None:
+    def add_to_history(self,
+                       history: Union[str, List[Tuple[str, str]]]) -> None:
         self.history.append(urwid.Text(history))
         if self.history:
             self.history.set_focus(len(self.history) - 1)
@@ -271,8 +292,15 @@ def __init__(self, master: App) -> None:
         super().__init__(self.body)
         urwid.connect_signal(self.master, UPDATE_MSG, self.cb_add_to_history)
 
-    def cb_add_to_history(self, msg: str) -> None:
-        self.history.add_to_history(msg)
+    def cb_add_to_history(self, msg: str, level: Optional[str] = None) -> None:
+        formatted = []
+        if level:
+            formatted.append((level, msg))
+        else:
+            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):
@@ -298,7 +326,7 @@ def emit(self, record: LogRecord) -> None:
         level = record.levelname
         msg = record.getMessage()
         msg = f'[{level}]: {msg}'
-        self.tui.add_to_history(msg)
+        self.tui.add_to_history(msg, level)
 
 
 def main() -> None:
-- 
2.17.1

[PATCH v3 09/13] python/aqmp-tui: Add QMP connection manager

[G S Niteesh Babu]

Instead of manually connecting and disconnecting from the
server. We now rely on the runstate to manage the QMP
connection.

Along with this the ability to reconnect on certain exceptions
has also been added.

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
 python/qemu/aqmp/aqmp_tui.py | 109 ++++++++++++++++++++++++++++++-----
 1 file changed, 94 insertions(+), 15 deletions(-)

diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
index 0d5ec62cb7..ef91883fa5 100644
--- a/python/qemu/aqmp/aqmp_tui.py
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -25,8 +25,9 @@
 import urwid_readline
 
 from ..qmp import QEMUMonitorProtocol, QMPBadPortError
+from .error import ProtocolError
 from .message import DeserializationError, Message, UnexpectedTypeError
-from .protocol import ConnectError
+from .protocol import ConnectError, Runstate
 from .qmp_client import ExecInterruptedError, QMPClient
 from .util import create_task, pretty_traceback
 
@@ -67,12 +68,24 @@ def format_json(msg: str) -> str:
     return ' '.join(words)
 
 
+def type_name(mtype: Any) -> str:
+    """
+    Returns the type name
+    """
+    return type(mtype).__name__
+
+
 class App(QMPClient):
-    def __init__(self, address: Union[str, Tuple[str, int]]) -> None:
+    def __init__(self, address: Union[str, Tuple[str, int]], num_retries: int,
+                 retry_delay: Optional[int]) -> None:
         urwid.register_signal(type(self), UPDATE_MSG)
         self.window = Window(self)
         self.address = address
         self.aloop: Optional[Any] = None  # FIXME: Use more concrete type.
+        self.num_retries = num_retries
+        self.retry_delay = retry_delay
+        self.retry: bool = False
+        self.disconnecting: bool = False
         super().__init__()
 
     def add_to_history(self, msg: str, level: Optional[str] = None) -> None:
@@ -119,7 +132,7 @@ def _cb_inbound(self, msg: Message) -> Message:
             LOGGER.info('Error server disconnected before reply')
             urwid.emit_signal(self, UPDATE_MSG,
                               '{"error": "Server disconnected before reply"}')
-            self._set_status("Server disconnected")
+            await self.disconnect()
         except Exception as err:
             LOGGER.error('Exception from _send_to_server: %s', str(err))
             raise err
@@ -136,15 +149,29 @@ def kill_app(self) -> None:
         create_task(self._kill_app())
 
     async def _kill_app(self) -> None:
-        # It is ok to call disconnect even in disconnect state
+        await self.disconnect()
+        LOGGER.debug('Disconnect finished. Exiting app')
+        raise urwid.ExitMainLoop()
+
+    async def disconnect(self) -> None:
+        if self.disconnecting:
+            return
         try:
-            await self.disconnect()
-            LOGGER.debug('Disconnect finished. Exiting app')
+            self.disconnecting = True
+            await super().disconnect()
+            self.retry = True
+        except EOFError as err:
+            LOGGER.info('disconnect: %s', type_name(err))
+            self.retry = True
+        except ProtocolError as err:
+            LOGGER.info('disconnect: %s', type_name(err))
+            self.retry = False
         except Exception as err:
-            LOGGER.info('_kill_app: %s', str(err))
-            # Let the app crash after providing a proper stack trace
+            LOGGER.error('disconnect: Unhandled exception %s', str(err))
+            self.retry = False
             raise err
-        raise urwid.ExitMainLoop()
+        finally:
+            self.disconnecting = False
 
     def handle_event(self, event: Message) -> None:
         # FIXME: Consider all states present in qapi/run-state.json
@@ -161,14 +188,61 @@ def _get_formatted_address(self) -> str:
             addr = f'{host}:{port}'
         return addr
 
-    async def connect_server(self) -> None:
+    async def _retry_connection(self) -> Optional[str]:
+        current_retries = 0
+        err = None
+        # Increase in power sequence of 2 if no delay is provided
+        cur_delay = 1
+        inc_delay = 2
+        if self.retry_delay:
+            inc_delay = 1
+            cur_delay = self.retry_delay
+        # initial try
+        await self.connect_server()
+        while self.retry and current_retries < self.num_retries:
+            LOGGER.info('Connection Failed, retrying in %d', cur_delay)
+            status = f'[Retry #{current_retries} ({cur_delay}s)]'
+            self._set_status(status)
+
+            await asyncio.sleep(cur_delay)
+
+            err = await self.connect_server()
+            cur_delay *= inc_delay
+            # Cap delay to 5mins
+            cur_delay = min(cur_delay, 5 * 60)
+            current_retries += 1
+        # If all retries failed report the last error
+        LOGGER.info('All retries failed: %s', str(err))
+        return type_name(err)
+
+    async def manage_connection(self) -> None:
+        while True:
+            if self.runstate == Runstate.IDLE:
+                LOGGER.info('Trying to reconnect')
+                err = await self._retry_connection()
+                # If retry is still true then, we have exhausted all our tries.
+                if self.retry:
+                    self._set_status(f'Error: {err}')
+                else:
+                    addr = self._get_formatted_address()
+                    self._set_status(f'[Connected {addr}]')
+            elif self.runstate == Runstate.DISCONNECTING:
+                self._set_status('[Disconnected]')
+                await self.disconnect()
+                # check if a retry is needed
+                if self.runstate == Runstate.IDLE:
+                    continue
+            await self.runstate_changed()
+
+    async def connect_server(self) -> Optional[str]:
         try:
             await self.connect(self.address)
-            addr = self._get_formatted_address()
-            self._set_status(f'Connected to {addr}')
+            self.retry = False
         except ConnectError as err:
             LOGGER.info('connect_server: ConnectError %s', str(err))
-            self._set_status('Server shutdown')
+            self.retry = True
+            return type_name(err)
+        return None
 
     def run(self, debug: bool = False) -> None:
         screen = urwid.raw_display.Screen()
@@ -191,7 +265,7 @@ def run(self, debug: bool = False) -> None:
                                    event_loop=event_loop)
 
         create_task(self.wait_for_events(), self.aloop)
-        create_task(self.connect_server(), self.aloop)
+        create_task(self.manage_connection(), self.aloop)
         try:
             main_loop.run()
         except Exception as err:
@@ -333,6 +407,11 @@ def main() -> None:
     parser = argparse.ArgumentParser(description='AQMP TUI')
     parser.add_argument('qmp_server', help='Address of the QMP server'
                         '< UNIX socket path | TCP addr:port >')
+    parser.add_argument('--num-retries', type=int, default=10,
+                        help='Number of times to reconnect before giving up')
+    parser.add_argument('--retry-delay', type=int,
+                        help='Time(s) to wait before next retry.'
+                        'Default action is to increase delay in powers of 2')
     parser.add_argument('--log-file', help='The Log file name')
     parser.add_argument('--log-level', default='WARNING',
                         help='Log level <CRITICAL|ERROR|WARNING|INFO|DEBUG|>')
@@ -348,7 +427,7 @@ def main() -> None:
     except QMPBadPortError as err:
         parser.error(str(err))
 
-    app = App(address)
+    app = App(address, args.num_retries, args.retry_delay)
 
     if args.log_file:
         LOGGER.addHandler(logging.FileHandler(args.log_file))
-- 
2.17.1

[PATCH v3 10/13] python/aqmp-tui: Add scrolling to history box

[G S Niteesh Babu]

Adds scroll support to history box. The list can now be scrolled
using arrow keys, page up/down and the mouse.

The current implementation requires the widget to be in focus
to enable scrolling. Therefore the user has to click on the widget
before scrolling.

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
 python/qemu/aqmp/aqmp_tui.py | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
index ef91883fa5..fb828b1a27 100644
--- a/python/qemu/aqmp/aqmp_tui.py
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -351,6 +351,19 @@ def add_to_history(self,
         if self.history:
             self.history.set_focus(len(self.history) - 1)
 
+    def mouse_event(self, size: Tuple[int, int], _event: str, button: float,
+                    _x: int, _y: int, focus: bool) -> None:
+        # Scroll only on focus. Therefore it is required to
+        # click on the widget to enable scrolling.
+        if not focus:
+            return
+        # button == 4 represents scroll up event
+        if button == 4.0:
+            super().keypress(size, 'up')
+        # button == 5 represents scroll down event
+        elif button == 5.0:
+            super().keypress(size, 'down')
+
 
 class HistoryWindow(urwid.Frame):
     """
-- 
2.17.1

[PATCH v3 11/13] python/aqmp-tui: Add ability to highlight messages

[G S Niteesh Babu]

Adds ability to highlight messages in the history box. The messages
can be selected using up/down arrow keys.
This can be enhanced in the future to apply specific settings to
a particular message.

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
 python/qemu/aqmp/aqmp_tui.py | 50 ++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
index fb828b1a27..4bae0d4e89 100644
--- a/python/qemu/aqmp/aqmp_tui.py
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -344,6 +344,7 @@ def __init__(self, master: App) -> None:
         self.master = master
         self.history = urwid.SimpleFocusListWalker([])
         super().__init__(self.history)
+        self.highlighting = -1
 
     def add_to_history(self,
                        history: Union[str, List[Tuple[str, str]]]) -> None:
@@ -351,8 +352,57 @@ def add_to_history(self,
         if self.history:
             self.history.set_focus(len(self.history) - 1)
 
+    def _remove_highlighting(self) -> None:
+        assert self.highlighting != -1
+        pos = self.highlighting
+        widget = self.history[pos]
+        widget = widget.original_widget
+        self.history[pos] = widget
+
+    def _update_highlighting(self) -> None:
+        assert self.highlighting != -1
+        pos = self.highlighting
+        widget = self.history[pos]
+        self.history[pos] = urwid.LineBox(widget)
+
+    def keypress(self, size: Tuple[int, int], key: str) -> Optional[str]:
+        if key == 'up':
+            if self.highlighting != -1:
+                pos = self.highlighting
+                self._remove_highlighting()
+                pos = max(pos - 1, 0)
+                self.highlighting = pos
+            else:
+                self.highlighting = len(self.history) - 1
+            self._update_highlighting()
+            self.change_focus(size, self.highlighting)
+            return None
+        if key == 'down':
+            pos = self.highlighting
+            if pos == -1:
+                return None
+
+            self._remove_highlighting()
+            if pos == len(self.history) - 1:
+                self.highlighting = -1
+            else:
+                self.highlighting = pos + 1
+                self._update_highlighting()
+                self.change_focus(size, self.highlighting)
+            return None
+
+        # Remove highlighting if someother key is pressed
+        if self.highlighting != -1:
+            self._remove_highlighting()
+            self.highlighting = -1
+        return super().keypress(size, key)  # type: ignore
+
     def mouse_event(self, size: Tuple[int, int], _event: str, button: float,
                     _x: int, _y: int, focus: bool) -> None:
+        if self.highlighting != -1:
+            self._remove_highlighting()
+            self.highlighting = -1
+
         # Scroll only on focus. Therefore it is required to
         # click on the widget to enable scrolling.
         if not focus:
-- 
2.17.1

[PATCH v3 12/13] python/aqmp-tui: Add pyperclip dependency

[G S Niteesh Babu]

This dependency is required to enable copying from the TUI
using special keys to the system clipboard.

pyperclip works out of the box on windows and macos but requires
xsel/xclip to be installed on linux machines.

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
 python/Pipfile.lock | 22 ++++++++++++++++++++++
 python/setup.cfg    |  5 +++++
 2 files changed, 27 insertions(+)

diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 2c6d779348..3544c8703d 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -45,6 +45,14 @@
             "index": "pypi",
             "version": "==87.0"
         },
+        "backports.entry-points-selectable": {
+            "hashes": [
+                "sha256:988468260ec1c196dab6ae1149260e2f5472c9110334e5d51adcb77867361f6a",
+                "sha256:a6d9a871cde5e15b4c4a53e3d43ba890cc6861ec1332c9c2428c92f977192acc"
+            ],
+            "markers": "python_version >= '2.7'",
+            "version": "==1.1.0"
+        },
         "distlib": {
             "hashes": [
                 "sha256:106fef6dc37dd8c0e2c0a60d3fca3e77460a48907f335fa28420463a6f799736",
@@ -169,6 +177,14 @@
             "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'",
             "version": "==20.9"
         },
+        "platformdirs": {
+            "hashes": [
+                "sha256:4666d822218db6a262bdfdc9c39d21f23b4cfdb08af331a81e92751daf6c866c",
+                "sha256:632daad3ab546bd8e6af0537d09805cec458dce201bccfe23012df73332e181e"
+            ],
+            "markers": "python_version >= '3.6'",
+            "version": "==2.2.0"
+        },
         "pluggy": {
             "hashes": [
                 "sha256:15b2acde666561e1298d71b523007ed7364de07029219b604cf808bfa1c765b0",
@@ -224,6 +240,12 @@
             "markers": "python_version >= '2.6' and python_version not in '3.0, 3.1, 3.2, 3.3'",
             "version": "==2.4.7"
         },
+        "pyperclip": {
+            "hashes": [
+                "sha256:105254a8b04934f0bc84e9c24eb360a591aaf6535c9def5f29d92af107a9bf57"
+            ],
+            "version": "==1.8.2"
+        },
         "qemu": {
             "editable": true,
             "path": "."
diff --git a/python/setup.cfg b/python/setup.cfg
index bbb7306c3d..683c0b1d00 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -47,6 +47,7 @@ devel =
     urwid >= 2.1.2
     urwid-readline >= 0.13
     Pygments >= 2.9.0
+    pyperclip >= 1.8.2
 
 # Provides qom-fuse functionality
 fuse =
@@ -57,6 +58,7 @@ tui =
     urwid >= 2.1.2
     urwid-readline >= 0.13
     Pygments >= 2.9.0
+    pyperclip >= 1.8.2
 
 [options.entry_points]
 console_scripts =
@@ -102,6 +104,9 @@ ignore_missing_imports = True
 [mypy-pygments]
 ignore_missing_imports = True
 
+[mypy-pyperclip]
+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

[PATCH v3 13/13] python/aqmp-tui: Allow copying message from TUI

[G S Niteesh Babu]

This commit adds a feature that enables use to copy
messages from the TUI after highlighting the message
in the history box using up/down arrow keys and pressing
alt-c.

Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
---
 python/qemu/aqmp/aqmp_tui.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
index 4bae0d4e89..434f431a35 100644
--- a/python/qemu/aqmp/aqmp_tui.py
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -21,6 +21,7 @@
 
 from pygments import lexers
 from pygments import token as Token
+import pyperclip
 import urwid
 import urwid_readline
 
@@ -390,6 +391,14 @@ def keypress(self, size: Tuple[int, int], key: str) -> Optional[str]:
                 self._update_highlighting()
                 self.change_focus(size, self.highlighting)
             return None
+        if key == 'meta c':
+            if self.highlighting == -1:
+                return None
+            widget = self.history[self.highlighting].original_widget
+            text = widget.get_text()[0]
+            LOGGER.info('Text is %s', text)
+            pyperclip.copy(text)
+            return None
 
         # Remove highlighting if someother key is pressed
         if self.highlighting != -1:
-- 
2.17.1

Re: [PATCH v3 01/13] python/aqmp: Fix wait_closed work-around for python 3.6

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:

> Before this patch the wait_closed work-around for python 3.6
> fails during disconnect.
> This is a temproray work around for which might be fixed in the
> future or will be completely removed when the minimum python
> version is raised to 3.7.
>
> This patch was originally written by John Snow <jsnow@redhat.com>
>
> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
> ---
>  python/qemu/aqmp/util.py | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/python/qemu/aqmp/util.py b/python/qemu/aqmp/util.py
> index de0df44cbd..eaa5fc7d5f 100644
> --- a/python/qemu/aqmp/util.py
> +++ b/python/qemu/aqmp/util.py
> @@ -134,7 +134,17 @@ def is_closing(writer: asyncio.StreamWriter) -> bool:
>
>      while not transport.is_closing():
>          await asyncio.sleep(0)
> -    await flush(writer)
> +
> +    # This is an ugly workaround, but it's the best I can come up with.
> +    sock = transport.get_extra_info('socket')
> +
> +    if sock is None:
> +        # Our transport doesn't have a socket? ...
> +        # Nothing we can reasonably do.
> +        return
> +
> +    while sock.fileno() != -1:
> +        await asyncio.sleep(0)
>
>
>  def asyncio_run(coro: Coroutine[Any, Any, T], *, debug: bool = False) ->
> T:
> --
> 2.17.1
>
>
Sorry for the trouble. This is now included in the v3 version of my series
and can be dropped. I hope.

[-- Attachment #2: Type: text/html, Size: 2101 bytes --]

Re: [PATCH v3 02/13] python: disable pylint errors for aqmp-tui

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

On Fri, Jul 30, 2021 at 4:19 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 2573cd7bfb..7a30dd5b09 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -90,6 +90,8 @@ ignore_missing_imports = True
>  # --disable=W".
>  disable=too-many-function-args,  # mypy handles this with less false
> positives.
>          no-member,  # mypy also handles this better.
> +        missing-docstring, # FIXME
> +        fixme, # FIXME
>
>
Please put some attention into removing the missing-docstring flag. At this
point, anything that is "FIXME" should either be fixed or re-worded as a
"TODO" and an exemption added to the pylint configuration such that "TODO"
is allowed but "FIXME" is not.

Take a look at pylint --generate-rcfile and find this section:

[MISCELLANEOUS]

# List of note tags to take in consideration, separated by a comma.
notes=FIXME,
      XXX,
      TODO


>  [pylint.basic]
>  # Good variable names which should always be accepted, separated by a
> comma.
> --
> 2.17.1
>
>

[-- Attachment #2: Type: text/html, Size: 2149 bytes --]

Re: [PATCH v3 03/13] python: Add dependencies for AQMP TUI

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 426 bytes --]

On Fri, Jul 30, 2021 at 4:19 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>
>

Reviewed-by: John Snow <jsnow@redhat.com>

[-- Attachment #2: Type: text/html, Size: 891 bytes --]

Re: [PATCH v3 04/13] python/aqmp-tui: Add AQMP TUI draft

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 18442 bytes --]

On Fri, Jul 30, 2021 at 4:19 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 | 333 +++++++++++++++++++++++++++++++++++
>  python/setup.cfg             |  16 +-
>  2 files changed, 348 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..ec9eba0aa7
> --- /dev/null
> +++ b/python/qemu/aqmp/aqmp_tui.py
> @@ -0,0 +1,333 @@
> +# 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
> +from logging import Handler
> +import signal
> +
> +import urwid
> +import urwid_readline
> +
> +from ..qmp import QEMUMonitorProtocol, QMPBadPortError
> +from .message import DeserializationError, Message, UnexpectedTypeError
> +from .protocol import ConnectError
> +from .qmp_client import ExecInterruptedError, QMPClient
> +from .util import create_task, pretty_traceback
> +
> +
> +UPDATE_MSG = 'UPDATE_MSG'
>

This line still feels kind of "naked" on a cold read. It could use a small
comment.


> +
> +# Using root logger to enable all loggers under qemu and asyncio
> +LOGGER = logging.getLogger()
>

The comment isn't quite true; this is the root logger -- but you go on to
use it to directly log messages. I don't think you should; use a
module-level logger.

(1) Create a module-level logger that is named after the current module
name (e.g. qemu.aqmp.aqmp_tui) and use that for logging messages relating
to this module:
LOGGER = logging.getLogger(__name__)

(2) Where you need to address the root logger, just use `root_logger =
logging.getLogger() ` .... I think the main() function is the only place
you might need this.



> +
> +
> +def format_json(msg):
> +    """
> +    Formats given multiline JSON message into a single line message.
> +    Converting into single line is more asthetically pleasing when looking
> +    along with error messages compared to multiline JSON.
> +    """
> +    # FIXME: Use better formatting mechanism. Might break at more complex
> JSON
> +    # data.
> +    msg = msg.replace('\n', '')
> +    words = msg.split(' ')
> +    words = [word for word in words if word != '']
> +    return ' '.join(words)
> +
>

You can use the JSON module to do this,
https://docs.python.org/3/library/json.html#json.dumps

Message._serialize uses this technique to send JSON messages over the wire
that have no newlines:
https://gitlab.com/jsnow/qemu/-/blob/python-async-qmp-aqmp/python/qemu/aqmp/message.py#L132

by not specifying an indent and including separators with no spaces, we can
get a JSON representation with all the space squeezed out. You can add
spaces and still omit the indent/newlines and so on.


> +
> +class App(QMPClient):
> +    def __init__(self, address):
> +        urwid.register_signal(type(self), UPDATE_MSG)
>

I found some hidden documentation about this -- I was struggling to find it
before:
https://github.com/urwid/urwid/blob/master/urwid/signals.py#L62

So the first argument is meant to be "The class of an object sending the
signal". Alright. And it looks like we only emit from add_to_history and in
_send_to_server, so I guess that checks out!

One thing I am noticing is that these signals are global and not
per-instance, so if for some reason we ever create a second App I wonder if
the __init__ method here will explode ... Well, it's probably not too
important right now.

>
> +        self.window = Window(self)
> +        self.address = address
> +        self.aloop = None
> +        super().__init__()
> +
> +    def add_to_history(self, msg):
> +        urwid.emit_signal(self, UPDATE_MSG, msg)
> +
> +    def _cb_outbound(self, msg):
> +        # FIXME: I think the ideal way to omit these messages during
> in-TUI
> +        # logging will be to add a filter to the logger. We can use regex
> to
> +        # filter out messages starting with 'Request:' or 'Response:' but
> I
> +        # think a better approach will be encapsulate the message in an
> object
> +        # and filter based on the object. Encapsulation of the message
> will
> +        # also be necessary when we want different formatting of messages
> +        # inside TUI.
>
+        handler = LOGGER.handlers[0]
> +        if not isinstance(handler, TUILogHandler):
> +            LOGGER.debug('Request: %s', str(msg))
> +        self.add_to_history('<-- ' + str(msg))
> +        return msg
> +
> +    def _cb_inbound(self, msg):
> +        handler = LOGGER.handlers[0]
> +        if not isinstance(handler, TUILogHandler):
> +            LOGGER.debug('Response: %s', str(msg))
> +        self.add_to_history('--> ' + str(msg))
> +        return msg
> +
> +    async def wait_for_events(self):
> +        async for event in self.events:
> +            self.handle_event(event)
> +
>

Can handle_event be moved here so that related functions are somewhat near
each other?


> +    async def _send_to_server(self, raw_msg):
> +        # FIXME: Format the raw_msg in history view to one line. It is not
> +        # pleasing to see multiple lines JSON object with an error
> statement.
> +        try:
> +            msg = Message(bytes(raw_msg, encoding='utf-8'))
> +            # Format multiline json into a single line JSON, since it is
> more
> +            # pleasing to look along with err message in TUI.
> +            raw_msg = self.format_json(raw_msg)
> +            await self._raw(msg, assign_id='id' not in msg)
> +        except (ValueError, TypeError) as err:
> +            LOGGER.info('Invalid message: %s', str(err))
> +            self.add_to_history(f'{raw_msg}: {err}')
> +        except (DeserializationError, UnexpectedTypeError) as err:
> +            LOGGER.info('Invalid message: %s', err.error_message)
> +            self.add_to_history(f'{raw_msg}: {err.error_message}')
> +        except ExecInterruptedError:
> +            LOGGER.info('Error server disconnected before reply')
> +            urwid.emit_signal(self, UPDATE_MSG,
> +                              '{"error": "Server disconnected before
> reply"}')
> +            self._set_status("Server disconnected")
> +        except Exception as err:
> +            LOGGER.error('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())
> +
>

That logic needs to be here -- each commit needs to make sense by itself.
If you've improved the connection logic, it needs to be merged into this
commit.


> +    async def _kill_app(self):
> +        # It is ok to call disconnect even in disconnect state
> +        try:
> +            await self.disconnect()
> +            LOGGER.debug('Disconnect finished. Exiting app')
> +        except Exception as err:
> +            LOGGER.info('_kill_app: %s', str(err))
> +            # Let the app crash after providing a proper stack trace
> +            raise err
> +        raise urwid.ExitMainLoop()
> +
> +    def handle_event(self, event):
> +        # FIXME: Consider all states present in qapi/run-state.json
> +        if event['event'] == 'SHUTDOWN':
> +            self._set_status('Server shutdown')
> +
>

This is an odd function to me because it will *overwrite* the status bar
with this event, but surely shortly thereafter we're going to see a
disconnection event from the transport itself, so this message will go away
almost immediately, won't it? I understand that connection management stuff
is improved in later patches, but the initial patch here would be better
served not having a partial and incorrect implementation. You can just
remove features that you intend to "build out" in later commits. And if we
remove this, then we don't need wait_for_events() either, and they can be
re-added in a later commit when they're given a more full, proper treatment.


> +    def _set_status(self, msg: str) -> None:
> +        self.window.footer.set_text(msg)
> +
> +    def _get_formatted_address(self) -> str:
> +        addr = f'{self.address}'
> +        if isinstance(self.address, tuple):
> +            host, port = self.address
> +            addr = f'{host}:{port}'
> +        return addr
>

Don't format addr twice in the TCP case -- make sure it formats it once. It
won't matter much for performance in something like Python, but it's
misleading when reading it.

+
> +    async def connect_server(self):
> +        try:
> +            await self.connect(self.address)
> +            addr = self._get_formatted_address()
> +            self._set_status(f'Connected to {addr}')
> +        except ConnectError as err:
> +            LOGGER.info('connect_server: ConnectError %s', str(err))
> +            self._set_status('Server shutdown')
>

A failure to connect doesn't necessary mean the server was shut down.


> +
>

I suspect this is changed in later commits in this series, but you should
probably set the footer to something before we potentially hang on
connect() and do a whole lot of nothing. LIke other critiques about
unfinished interfaces, once you've implemented them, they should be worked
into this patch so that each commit makes sense by itself. Reviewers will
have to read these patches and make sense of them. If they have to flip
through your entire series to understand what it will eventually look like,
it's a lot more work and you're less likely to get good or helpful reviews.


> +    def run(self, debug=False):
> +        self.aloop = asyncio.get_event_loop()
> +        self.aloop.set_debug(debug)
> +
> +        # 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)
> +
> +        event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
> +        main_loop = urwid.MainLoop(urwid.AttrMap(self.window,
> 'background'),
> +                                   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:
> +            main_loop.run()
> +        except Exception as err:
> +            LOGGER.error('%s\n%s\n', str(err), pretty_traceback())
> +            raise err
> +
> +
> +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_widget = EditorWidget(master)
> +        self.editor = urwid.LineBox(self.editor_widget)
> +        self.history = HistoryBox(master)
> +        self.body = urwid.Pile([('weight', 80, self.history),
> +                                ('weight', 20, self.editor)])
> +        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 TUILogHandler(Handler):
> +    def __init__(self, tui):
> +        super().__init__()
> +        self.tui = tui
> +
> +    def emit(self, record):
> +        level = record.levelname
> +        msg = record.getMessage()
> +        msg = f'[{level}]: {msg}'
> +        self.tui.add_to_history(msg)
> +
> +
> +def main():
> +    parser = argparse.ArgumentParser(description='AQMP TUI')
> +    parser.add_argument('qmp_server', help='Address of the QMP server'
> +                        '< UNIX socket path | TCP addr:port >')
> +    parser.add_argument('--log-file', help='The Log file name')
> +    parser.add_argument('--log-level', default='WARNING',
> +                        help='Log level
> <CRITICAL|ERROR|WARNING|INFO|DEBUG|>')
> +    parser.add_argument('--asyncio-debug', action='store_true',
> +                        help='Enable debug mode for asyncio loop'
> +                        'Generates lot of output, makes TUI unusable when'
> +                        'logs are logged in the TUI itself.'
> +                        'Use only when logging to a file')
>

Needs spaces between the lines here so that the output reads correctly.


> +    args = parser.parse_args()
> +
> +    try:
> +        address = QEMUMonitorProtocol.parse_address(args.qmp_server)
> +    except QMPBadPortError as err:
> +        parser.error(err)
> +
> +    app = App(address)
> +
> +    if args.log_file:
> +        LOGGER.addHandler(logging.FileHandler(args.log_file))
> +    else:
> +        LOGGER.addHandler(TUILogHandler(app))
> +
> +    log_level = logging.getLevelName(args.log_level)
> +    # getLevelName returns 'Level {log_level}' when a invalid level is
> passed.
> +    if log_level == f'Level {args.log_level}':
> +        parser.error('Invalid log level')
> +    LOGGER.setLevel(log_level)
> +
>

I think you could do:

LOGGER.setLevel(logging.getLevelName(args.log_level))

And it'll fail if the level was unknown, but work otherwise. This would
avoid a kind of precarious string-comparison check, which I worry is
fragile.


> +    app.run(args.asyncio_debug)
> +
> +
> +if __name__ == '__main__':
> +    main()  # type: ignore
> diff --git a/python/setup.cfg b/python/setup.cfg
> index d106a0ed7a..50f9894468 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
>
>
I have some more thoughts, but need to research them and wanted to give
some feedback first so you weren't waiting.

[-- Attachment #2: Type: text/html, Size: 24686 bytes --]

Re: [PATCH v3 04/13] python/aqmp-tui: Add AQMP TUI draft

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 15303 bytes --]

On Fri, Jul 30, 2021 at 4:19 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 | 333 +++++++++++++++++++++++++++++++++++
>  python/setup.cfg             |  16 +-
>  2 files changed, 348 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..ec9eba0aa7
> --- /dev/null
> +++ b/python/qemu/aqmp/aqmp_tui.py
> @@ -0,0 +1,333 @@
> +# 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
> +from logging import Handler
> +import signal
> +
> +import urwid
> +import urwid_readline
> +
> +from ..qmp import QEMUMonitorProtocol, QMPBadPortError
> +from .message import DeserializationError, Message, UnexpectedTypeError
> +from .protocol import ConnectError
> +from .qmp_client import ExecInterruptedError, QMPClient
> +from .util import create_task, pretty_traceback
> +
> +
> +UPDATE_MSG = 'UPDATE_MSG'
> +
> +# Using root logger to enable all loggers under qemu and asyncio
> +LOGGER = logging.getLogger()
> +
> +
> +def format_json(msg):
> +    """
> +    Formats given multiline JSON message into a single line message.
> +    Converting into single line is more asthetically pleasing when looking
> +    along with error messages compared to multiline JSON.
> +    """
> +    # FIXME: Use better formatting mechanism. Might break at more complex
> JSON
> +    # data.
> +    msg = msg.replace('\n', '')
> +    words = msg.split(' ')
> +    words = [word for word in words if word != '']
> +    return ' '.join(words)
> +
> +
> +class App(QMPClient):
> +    def __init__(self, address):
> +        urwid.register_signal(type(self), UPDATE_MSG)
> +        self.window = Window(self)
> +        self.address = address
> +        self.aloop = None
> +        super().__init__()
> +
> +    def add_to_history(self, msg):
> +        urwid.emit_signal(self, UPDATE_MSG, msg)
> +
> +    def _cb_outbound(self, msg):
> +        # FIXME: I think the ideal way to omit these messages during
> in-TUI
> +        # logging will be to add a filter to the logger. We can use regex
> to
> +        # filter out messages starting with 'Request:' or 'Response:' but
> I
> +        # think a better approach will be encapsulate the message in an
> object
> +        # and filter based on the object. Encapsulation of the message
> will
> +        # also be necessary when we want different formatting of messages
> +        # inside TUI.
> +        handler = LOGGER.handlers[0]
> +        if not isinstance(handler, TUILogHandler):
> +            LOGGER.debug('Request: %s', str(msg))
> +        self.add_to_history('<-- ' + str(msg))
> +        return msg
> +
> +    def _cb_inbound(self, msg):
> +        handler = LOGGER.handlers[0]
> +        if not isinstance(handler, TUILogHandler):
> +            LOGGER.debug('Response: %s', str(msg))
> +        self.add_to_history('--> ' + 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, raw_msg):
> +        # FIXME: Format the raw_msg in history view to one line. It is not
> +        # pleasing to see multiple lines JSON object with an error
> statement.
> +        try:
> +            msg = Message(bytes(raw_msg, encoding='utf-8'))
> +            # Format multiline json into a single line JSON, since it is
> more
> +            # pleasing to look along with err message in TUI.
> +            raw_msg = self.format_json(raw_msg)
> +            await self._raw(msg, assign_id='id' not in msg)
> +        except (ValueError, TypeError) as err:
> +            LOGGER.info('Invalid message: %s', str(err))
> +            self.add_to_history(f'{raw_msg}: {err}')
> +        except (DeserializationError, UnexpectedTypeError) as err:
> +            LOGGER.info('Invalid message: %s', err.error_message)
> +            self.add_to_history(f'{raw_msg}: {err.error_message}')
> +        except ExecInterruptedError:
> +            LOGGER.info('Error server disconnected before reply')
> +            urwid.emit_signal(self, UPDATE_MSG,
> +                              '{"error": "Server disconnected before
> reply"}')
> +            self._set_status("Server disconnected")
> +        except Exception as err:
> +            LOGGER.error('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
> +        try:
> +            await self.disconnect()
> +            LOGGER.debug('Disconnect finished. Exiting app')
> +        except Exception as err:
> +            LOGGER.info('_kill_app: %s', str(err))
> +            # Let the app crash after providing a proper stack trace
> +            raise err
> +        raise urwid.ExitMainLoop()
> +
> +    def handle_event(self, event):
> +        # FIXME: Consider all states present in qapi/run-state.json
> +        if event['event'] == 'SHUTDOWN':
> +            self._set_status('Server shutdown')
> +
> +    def _set_status(self, msg: str) -> None:
> +        self.window.footer.set_text(msg)
> +
> +    def _get_formatted_address(self) -> str:
> +        addr = f'{self.address}'
> +        if isinstance(self.address, tuple):
> +            host, port = self.address
> +            addr = f'{host}:{port}'
> +        return addr
> +
> +    async def connect_server(self):
> +        try:
> +            await self.connect(self.address)
> +            addr = self._get_formatted_address()
> +            self._set_status(f'Connected to {addr}')
> +        except ConnectError as err:
> +            LOGGER.info('connect_server: ConnectError %s', str(err))
> +            self._set_status('Server shutdown')
> +
> +    def run(self, debug=False):
> +        self.aloop = asyncio.get_event_loop()
> +        self.aloop.set_debug(debug)
> +
> +        # 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)
> +
> +        event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
> +        main_loop = urwid.MainLoop(urwid.AttrMap(self.window,
> 'background'),
> +                                   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:
> +            main_loop.run()
> +        except Exception as err:
> +            LOGGER.error('%s\n%s\n', str(err), pretty_traceback())
> +            raise err
> +
> +
> +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)
>

cb_send_to_server calls create_task(App._send_to_server(msg)) at which
point in the coroutine you finally do msg = Message(bytes(raw_msg,
encoding='utf-8')). However, you can do your message conversion *first*,
here in the Editor.

That way, you can avoid erasing the editor bar when the message is garbled
and the user has a chance to fix the corrupted message *first* before we
dispatch to the callback-async-machine. That should reduce quite a lot of
the error cases in _send_to_server and helps separate out the two kinds of
errors we expect to see here:

1. The user typed something that's garbage, and we didn't actually even
send it (because we couldn't), and
2. We sent (or tried to send) the message; but a failure occurred.


> +            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_widget = EditorWidget(master)
> +        self.editor = urwid.LineBox(self.editor_widget)
> +        self.history = HistoryBox(master)
> +        self.body = urwid.Pile([('weight', 80, self.history),
> +                                ('weight', 20, self.editor)])
> +        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 TUILogHandler(Handler):
> +    def __init__(self, tui):
> +        super().__init__()
> +        self.tui = tui
> +
> +    def emit(self, record):
> +        level = record.levelname
> +        msg = record.getMessage()
> +        msg = f'[{level}]: {msg}'
> +        self.tui.add_to_history(msg)
> +
> +
> +def main():
> +    parser = argparse.ArgumentParser(description='AQMP TUI')
> +    parser.add_argument('qmp_server', help='Address of the QMP server'
> +                        '< UNIX socket path | TCP addr:port >')
> +    parser.add_argument('--log-file', help='The Log file name')
> +    parser.add_argument('--log-level', default='WARNING',
> +                        help='Log level
> <CRITICAL|ERROR|WARNING|INFO|DEBUG|>')
> +    parser.add_argument('--asyncio-debug', action='store_true',
> +                        help='Enable debug mode for asyncio loop'
> +                        'Generates lot of output, makes TUI unusable when'
> +                        'logs are logged in the TUI itself.'
> +                        'Use only when logging to a file')
> +    args = parser.parse_args()
> +
> +    try:
> +        address = QEMUMonitorProtocol.parse_address(args.qmp_server)
> +    except QMPBadPortError as err:
> +        parser.error(err)
> +
> +    app = App(address)
> +
> +    if args.log_file:
> +        LOGGER.addHandler(logging.FileHandler(args.log_file))
> +    else:
> +        LOGGER.addHandler(TUILogHandler(app))
> +
> +    log_level = logging.getLevelName(args.log_level)
> +    # getLevelName returns 'Level {log_level}' when a invalid level is
> passed.
> +    if log_level == f'Level {args.log_level}':
> +        parser.error('Invalid log level')
> +    LOGGER.setLevel(log_level)
> +
> +    app.run(args.asyncio_debug)
> +
> +
> +if __name__ == '__main__':
> +    main()  # type: ignore
> diff --git a/python/setup.cfg b/python/setup.cfg
> index d106a0ed7a..50f9894468 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
>
>

[-- Attachment #2: Type: text/html, Size: 19015 bytes --]

Re: [PATCH v3 05/13] python: add entry point for aqmp-tui

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

On Fri, Jul 30, 2021 at 4:19 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 localhost:1234"
> More options available in the TUI can be found using "aqmp-tui -h"
>
> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
>

Reviewed-by: John Snow <jsnow@redhat.com>

[-- Attachment #2: Type: text/html, Size: 844 bytes --]

Re: [PATCH v3 06/13] python/aqmp-tui: Added type annotations for aqmp-tui

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 10858 bytes --]

On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:

> This patch adds type annotations for aqmp-tui using
> the mypy library.
>
>
Awesome, thanks for taking a swing at this. Looks like it wasn't as bad as
I was fearing.


> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
> ---
>  python/qemu/aqmp/aqmp_tui.py | 79 ++++++++++++++++++++----------------
>  python/setup.cfg             |  3 --
>  2 files changed, 43 insertions(+), 39 deletions(-)
>
> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
> index ec9eba0aa7..ab9ada793a 100644
> --- a/python/qemu/aqmp/aqmp_tui.py
> +++ b/python/qemu/aqmp/aqmp_tui.py
> @@ -9,8 +9,15 @@
>  import argparse
>  import asyncio
>  import logging
> -from logging import Handler
> +from logging import Handler, LogRecord
>  import signal
> +from typing import (
> +    Any,
> +    List,
> +    Optional,
> +    Tuple,
> +    Union,
> +)
>
>  import urwid
>  import urwid_readline
> @@ -22,13 +29,13 @@
>  from .util import create_task, pretty_traceback
>
>
> -UPDATE_MSG = 'UPDATE_MSG'
> +UPDATE_MSG: str = 'UPDATE_MSG'
>
>  # Using root logger to enable all loggers under qemu and asyncio
>  LOGGER = logging.getLogger()
>
>
> -def format_json(msg):
> +def format_json(msg: str) -> str:
>      """
>      Formats given multiline JSON message into a single line message.
>      Converting into single line is more asthetically pleasing when looking
> @@ -43,17 +50,17 @@ def format_json(msg):
>
>
>  class App(QMPClient):
> -    def __init__(self, address):
> +    def __init__(self, address: Union[str, Tuple[str, int]]) -> None:
>          urwid.register_signal(type(self), UPDATE_MSG)
>          self.window = Window(self)
>          self.address = address
> -        self.aloop = None
> +        self.aloop: Optional[Any] = None  # FIXME: Use more concrete type.
>

I ran into this in util.py; you want Optional[asyncio.AbstractEventLoop]
here.


>          super().__init__()
>
> -    def add_to_history(self, msg):
> +    def add_to_history(self, msg: str) -> None:
>          urwid.emit_signal(self, UPDATE_MSG, msg)
>
> -    def _cb_outbound(self, msg):
> +    def _cb_outbound(self, msg: Message) -> Message:
>          # FIXME: I think the ideal way to omit these messages during
> in-TUI
>          # logging will be to add a filter to the logger. We can use regex
> to
>          # filter out messages starting with 'Request:' or 'Response:' but
> I
> @@ -67,25 +74,25 @@ def _cb_outbound(self, msg):
>          self.add_to_history('<-- ' + str(msg))
>          return msg
>
> -    def _cb_inbound(self, msg):
> +    def _cb_inbound(self, msg: Message) -> Message:
>          handler = LOGGER.handlers[0]
>          if not isinstance(handler, TUILogHandler):
>              LOGGER.debug('Response: %s', str(msg))
>          self.add_to_history('--> ' + str(msg))
>          return msg
>
> -    async def wait_for_events(self):
> +    async def wait_for_events(self) -> None:
>          async for event in self.events:
>              self.handle_event(event)
>
> -    async def _send_to_server(self, raw_msg):
> +    async def _send_to_server(self, raw_msg: str) -> None:
>          # FIXME: Format the raw_msg in history view to one line. It is not
>          # pleasing to see multiple lines JSON object with an error
> statement.
>          try:
>              msg = Message(bytes(raw_msg, encoding='utf-8'))
>              # Format multiline json into a single line JSON, since it is
> more
>              # pleasing to look along with err message in TUI.
> -            raw_msg = self.format_json(raw_msg)
> +            raw_msg = format_json(raw_msg)
>              await self._raw(msg, assign_id='id' not in msg)
>          except (ValueError, TypeError) as err:
>              LOGGER.info('Invalid message: %s', str(err))
> @@ -102,18 +109,18 @@ def _cb_inbound(self, msg):
>              LOGGER.error('Exception from _send_to_server: %s', str(err))
>              raise err
>
> -    def cb_send_to_server(self, msg):
> +    def cb_send_to_server(self, msg: str) -> None:
>          create_task(self._send_to_server(msg))
>
> -    def unhandled_input(self, key):
> +    def unhandled_input(self, key: str) -> None:
>          if key == 'esc':
>              self.kill_app()
>
> -    def kill_app(self):
> +    def kill_app(self) -> None:
>          # TODO: Work on the disconnect logic
>          create_task(self._kill_app())
>
> -    async def _kill_app(self):
> +    async def _kill_app(self) -> None:
>          # It is ok to call disconnect even in disconnect state
>          try:
>              await self.disconnect()
> @@ -124,7 +131,7 @@ def kill_app(self):
>              raise err
>          raise urwid.ExitMainLoop()
>
> -    def handle_event(self, event):
> +    def handle_event(self, event: Message) -> None:
>          # FIXME: Consider all states present in qapi/run-state.json
>          if event['event'] == 'SHUTDOWN':
>              self._set_status('Server shutdown')
> @@ -139,7 +146,7 @@ def _get_formatted_address(self) -> str:
>              addr = f'{host}:{port}'
>          return addr
>
> -    async def connect_server(self):
> +    async def connect_server(self) -> None:
>          try:
>              await self.connect(self.address)
>              addr = self._get_formatted_address()
> @@ -148,7 +155,7 @@ def _get_formatted_address(self) -> str:
>              LOGGER.info('connect_server: ConnectError %s', str(err))
>              self._set_status('Server shutdown')
>
> -    def run(self, debug=False):
> +    def run(self, debug: bool = False) -> None:
>          self.aloop = asyncio.get_event_loop()
>          self.aloop.set_debug(debug)
>
> @@ -176,7 +183,7 @@ class StatusBar(urwid.Text):
>      """
>      A simple Text widget that currently only shows connection status.
>      """
> -    def __init__(self, text=''):
> +    def __init__(self, text: str = ''):
>          super().__init__(text, align='right')
>
>
> @@ -185,14 +192,14 @@ class Editor(urwid_readline.ReadlineEdit):
>      Support urwid_readline features along with
>      history support which lacks in urwid_readline
>      """
> -    def __init__(self, master):
> +    def __init__(self, master: App) -> None:
>          super().__init__(caption='> ', multiline=True)
>          self.master = master
> -        self.history = []
> -        self.last_index = -1
> -        self.show_history = False
> +        self.history: List[str] = []
> +        self.last_index: int = -1
> +        self.show_history: bool = False
>
> -    def keypress(self, size, key):
> +    def keypress(self, size: Tuple[int, int], key: str) -> Optional[str]:
>          # 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
> @@ -223,7 +230,7 @@ def keypress(self, size, key):
>          else:
>              self.show_history = False
>              self.last_index = 0
> -            return super().keypress(size, key)
> +            return super().keypress(size, key)  # type: ignore
>

try using cast(Optional[str], super().keypress(size, key)) instead of the
type: ignore. It doesn't make a gigantic difference, really, but it looks
"narrower" in scope and will probably confuse *me* less in the future.


>          return None
>
>
> @@ -231,7 +238,7 @@ class EditorWidget(urwid.Filler):
>      """
>      Wraps CustomEdit
>      """
> -    def __init__(self, master):
> +    def __init__(self, master: App) -> None:
>          super().__init__(Editor(master), valign='top')
>
>
> @@ -239,12 +246,12 @@ class HistoryBox(urwid.ListBox):
>      """
>      Shows all the QMP message transmitted/received
>      """
> -    def __init__(self, master):
> +    def __init__(self, master: App) -> None:
>          self.master = master
>          self.history = urwid.SimpleFocusListWalker([])
>          super().__init__(self.history)
>
> -    def add_to_history(self, history):
> +    def add_to_history(self, history: str) -> None:
>          self.history.append(urwid.Text(history))
>          if self.history:
>              self.history.set_focus(len(self.history) - 1)
> @@ -254,7 +261,7 @@ class HistoryWindow(urwid.Frame):
>      """
>      Composes the HistoryBox and EditorWidget
>      """
> -    def __init__(self, master):
> +    def __init__(self, master: App) -> None:
>          self.master = master
>          self.editor_widget = EditorWidget(master)
>          self.editor = urwid.LineBox(self.editor_widget)
> @@ -264,7 +271,7 @@ def __init__(self, master):
>          super().__init__(self.body)
>          urwid.connect_signal(self.master, UPDATE_MSG,
> self.cb_add_to_history)
>
> -    def cb_add_to_history(self, msg):
> +    def cb_add_to_history(self, msg: str) -> None:
>          self.history.add_to_history(msg)
>
>
> @@ -275,7 +282,7 @@ class Window(urwid.Frame):
>      future when we will have multiple windows and want to the switch
> between
>      them and display overlays
>      """
> -    def __init__(self, master):
> +    def __init__(self, master: App) -> None:
>          self.master = master
>          footer = StatusBar()
>          body = HistoryWindow(master)
> @@ -283,18 +290,18 @@ def __init__(self, master):
>
>
>  class TUILogHandler(Handler):
> -    def __init__(self, tui):
> +    def __init__(self, tui: App) -> None:
>          super().__init__()
>          self.tui = tui
>
> -    def emit(self, record):
> +    def emit(self, record: LogRecord) -> None:
>          level = record.levelname
>          msg = record.getMessage()
>          msg = f'[{level}]: {msg}'
>          self.tui.add_to_history(msg)
>
>
> -def main():
> +def main() -> None:
>      parser = argparse.ArgumentParser(description='AQMP TUI')
>      parser.add_argument('qmp_server', help='Address of the QMP server'
>                          '< UNIX socket path | TCP addr:port >')
> @@ -311,7 +318,7 @@ def main():
>      try:
>          address = QEMUMonitorProtocol.parse_address(args.qmp_server)
>      except QMPBadPortError as err:
> -        parser.error(err)
> +        parser.error(str(err))
>
>      app = App(address)
>
> @@ -330,4 +337,4 @@ def main():
>
>
>  if __name__ == '__main__':
> -    main()  # type: ignore
> +    main()
> diff --git a/python/setup.cfg b/python/setup.cfg
> index 8cd9ac0d81..11c6240aba 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -83,9 +83,6 @@ namespace_packages = True
>  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
>
> --
> 2.17.1
>
>
Please squash this into the first patch, but it looks good, thanks!

--js

[-- Attachment #2: Type: text/html, Size: 14050 bytes --]

Re: [PATCH v3 06/13] python/aqmp-tui: Added type annotations for aqmp-tui

[Niteesh G. S.]

[-- Attachment #1: Type: text/plain, Size: 11314 bytes --]

On Fri, Aug 6, 2021 at 1:26 AM John Snow <jsnow@redhat.com> wrote:

>
>
> On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
> wrote:
>
>> This patch adds type annotations for aqmp-tui using
>> the mypy library.
>>
>>
> Awesome, thanks for taking a swing at this. Looks like it wasn't as bad as
> I was fearing.
>
>
>> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
>> ---
>>  python/qemu/aqmp/aqmp_tui.py | 79 ++++++++++++++++++++----------------
>>  python/setup.cfg             |  3 --
>>  2 files changed, 43 insertions(+), 39 deletions(-)
>>
>> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
>> index ec9eba0aa7..ab9ada793a 100644
>> --- a/python/qemu/aqmp/aqmp_tui.py
>> +++ b/python/qemu/aqmp/aqmp_tui.py
>> @@ -9,8 +9,15 @@
>>  import argparse
>>  import asyncio
>>  import logging
>> -from logging import Handler
>> +from logging import Handler, LogRecord
>>  import signal
>> +from typing import (
>> +    Any,
>> +    List,
>> +    Optional,
>> +    Tuple,
>> +    Union,
>> +)
>>
>>  import urwid
>>  import urwid_readline
>> @@ -22,13 +29,13 @@
>>  from .util import create_task, pretty_traceback
>>
>>
>> -UPDATE_MSG = 'UPDATE_MSG'
>> +UPDATE_MSG: str = 'UPDATE_MSG'
>>
>>  # Using root logger to enable all loggers under qemu and asyncio
>>  LOGGER = logging.getLogger()
>>
>>
>> -def format_json(msg):
>> +def format_json(msg: str) -> str:
>>      """
>>      Formats given multiline JSON message into a single line message.
>>      Converting into single line is more asthetically pleasing when
>> looking
>> @@ -43,17 +50,17 @@ def format_json(msg):
>>
>>
>>  class App(QMPClient):
>> -    def __init__(self, address):
>> +    def __init__(self, address: Union[str, Tuple[str, int]]) -> None:
>>          urwid.register_signal(type(self), UPDATE_MSG)
>>          self.window = Window(self)
>>          self.address = address
>> -        self.aloop = None
>> +        self.aloop: Optional[Any] = None  # FIXME: Use more concrete
>> type.
>>
>
> I ran into this in util.py; you want Optional[asyncio.AbstractEventLoop]
> here.
>
Thanks. Fixed.

>
>
>>          super().__init__()
>>
>> -    def add_to_history(self, msg):
>> +    def add_to_history(self, msg: str) -> None:
>>          urwid.emit_signal(self, UPDATE_MSG, msg)
>>
>> -    def _cb_outbound(self, msg):
>> +    def _cb_outbound(self, msg: Message) -> Message:
>>          # FIXME: I think the ideal way to omit these messages during
>> in-TUI
>>          # logging will be to add a filter to the logger. We can use
>> regex to
>>          # filter out messages starting with 'Request:' or 'Response:'
>> but I
>> @@ -67,25 +74,25 @@ def _cb_outbound(self, msg):
>>          self.add_to_history('<-- ' + str(msg))
>>          return msg
>>
>> -    def _cb_inbound(self, msg):
>> +    def _cb_inbound(self, msg: Message) -> Message:
>>          handler = LOGGER.handlers[0]
>>          if not isinstance(handler, TUILogHandler):
>>              LOGGER.debug('Response: %s', str(msg))
>>          self.add_to_history('--> ' + str(msg))
>>          return msg
>>
>> -    async def wait_for_events(self):
>> +    async def wait_for_events(self) -> None:
>>          async for event in self.events:
>>              self.handle_event(event)
>>
>> -    async def _send_to_server(self, raw_msg):
>> +    async def _send_to_server(self, raw_msg: str) -> None:
>>          # FIXME: Format the raw_msg in history view to one line. It is
>> not
>>          # pleasing to see multiple lines JSON object with an error
>> statement.
>>          try:
>>              msg = Message(bytes(raw_msg, encoding='utf-8'))
>>              # Format multiline json into a single line JSON, since it is
>> more
>>              # pleasing to look along with err message in TUI.
>> -            raw_msg = self.format_json(raw_msg)
>> +            raw_msg = format_json(raw_msg)
>>              await self._raw(msg, assign_id='id' not in msg)
>>          except (ValueError, TypeError) as err:
>>              LOGGER.info('Invalid message: %s', str(err))
>> @@ -102,18 +109,18 @@ def _cb_inbound(self, msg):
>>              LOGGER.error('Exception from _send_to_server: %s', str(err))
>>              raise err
>>
>> -    def cb_send_to_server(self, msg):
>> +    def cb_send_to_server(self, msg: str) -> None:
>>          create_task(self._send_to_server(msg))
>>
>> -    def unhandled_input(self, key):
>> +    def unhandled_input(self, key: str) -> None:
>>          if key == 'esc':
>>              self.kill_app()
>>
>> -    def kill_app(self):
>> +    def kill_app(self) -> None:
>>          # TODO: Work on the disconnect logic
>>          create_task(self._kill_app())
>>
>> -    async def _kill_app(self):
>> +    async def _kill_app(self) -> None:
>>          # It is ok to call disconnect even in disconnect state
>>          try:
>>              await self.disconnect()
>> @@ -124,7 +131,7 @@ def kill_app(self):
>>              raise err
>>          raise urwid.ExitMainLoop()
>>
>> -    def handle_event(self, event):
>> +    def handle_event(self, event: Message) -> None:
>>          # FIXME: Consider all states present in qapi/run-state.json
>>          if event['event'] == 'SHUTDOWN':
>>              self._set_status('Server shutdown')
>> @@ -139,7 +146,7 @@ def _get_formatted_address(self) -> str:
>>              addr = f'{host}:{port}'
>>          return addr
>>
>> -    async def connect_server(self):
>> +    async def connect_server(self) -> None:
>>          try:
>>              await self.connect(self.address)
>>              addr = self._get_formatted_address()
>> @@ -148,7 +155,7 @@ def _get_formatted_address(self) -> str:
>>              LOGGER.info('connect_server: ConnectError %s', str(err))
>>              self._set_status('Server shutdown')
>>
>> -    def run(self, debug=False):
>> +    def run(self, debug: bool = False) -> None:
>>          self.aloop = asyncio.get_event_loop()
>>          self.aloop.set_debug(debug)
>>
>> @@ -176,7 +183,7 @@ class StatusBar(urwid.Text):
>>      """
>>      A simple Text widget that currently only shows connection status.
>>      """
>> -    def __init__(self, text=''):
>> +    def __init__(self, text: str = ''):
>>          super().__init__(text, align='right')
>>
>>
>> @@ -185,14 +192,14 @@ class Editor(urwid_readline.ReadlineEdit):
>>      Support urwid_readline features along with
>>      history support which lacks in urwid_readline
>>      """
>> -    def __init__(self, master):
>> +    def __init__(self, master: App) -> None:
>>          super().__init__(caption='> ', multiline=True)
>>          self.master = master
>> -        self.history = []
>> -        self.last_index = -1
>> -        self.show_history = False
>> +        self.history: List[str] = []
>> +        self.last_index: int = -1
>> +        self.show_history: bool = False
>>
>> -    def keypress(self, size, key):
>> +    def keypress(self, size: Tuple[int, int], key: str) -> Optional[str]:
>>          # 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
>> @@ -223,7 +230,7 @@ def keypress(self, size, key):
>>          else:
>>              self.show_history = False
>>              self.last_index = 0
>> -            return super().keypress(size, key)
>> +            return super().keypress(size, key)  # type: ignore
>>
>
> try using cast(Optional[str], super().keypress(size, key)) instead of the
> type: ignore. It doesn't make a gigantic difference, really, but it looks
> "narrower" in scope and will probably confuse *me* less in the future.
>
Fixed

>
>
>>          return None
>>
>>
>> @@ -231,7 +238,7 @@ class EditorWidget(urwid.Filler):
>>      """
>>      Wraps CustomEdit
>>      """
>> -    def __init__(self, master):
>> +    def __init__(self, master: App) -> None:
>>          super().__init__(Editor(master), valign='top')
>>
>>
>> @@ -239,12 +246,12 @@ class HistoryBox(urwid.ListBox):
>>      """
>>      Shows all the QMP message transmitted/received
>>      """
>> -    def __init__(self, master):
>> +    def __init__(self, master: App) -> None:
>>          self.master = master
>>          self.history = urwid.SimpleFocusListWalker([])
>>          super().__init__(self.history)
>>
>> -    def add_to_history(self, history):
>> +    def add_to_history(self, history: str) -> None:
>>          self.history.append(urwid.Text(history))
>>          if self.history:
>>              self.history.set_focus(len(self.history) - 1)
>> @@ -254,7 +261,7 @@ class HistoryWindow(urwid.Frame):
>>      """
>>      Composes the HistoryBox and EditorWidget
>>      """
>> -    def __init__(self, master):
>> +    def __init__(self, master: App) -> None:
>>          self.master = master
>>          self.editor_widget = EditorWidget(master)
>>          self.editor = urwid.LineBox(self.editor_widget)
>> @@ -264,7 +271,7 @@ def __init__(self, master):
>>          super().__init__(self.body)
>>          urwid.connect_signal(self.master, UPDATE_MSG,
>> self.cb_add_to_history)
>>
>> -    def cb_add_to_history(self, msg):
>> +    def cb_add_to_history(self, msg: str) -> None:
>>          self.history.add_to_history(msg)
>>
>>
>> @@ -275,7 +282,7 @@ class Window(urwid.Frame):
>>      future when we will have multiple windows and want to the switch
>> between
>>      them and display overlays
>>      """
>> -    def __init__(self, master):
>> +    def __init__(self, master: App) -> None:
>>          self.master = master
>>          footer = StatusBar()
>>          body = HistoryWindow(master)
>> @@ -283,18 +290,18 @@ def __init__(self, master):
>>
>>
>>  class TUILogHandler(Handler):
>> -    def __init__(self, tui):
>> +    def __init__(self, tui: App) -> None:
>>          super().__init__()
>>          self.tui = tui
>>
>> -    def emit(self, record):
>> +    def emit(self, record: LogRecord) -> None:
>>          level = record.levelname
>>          msg = record.getMessage()
>>          msg = f'[{level}]: {msg}'
>>          self.tui.add_to_history(msg)
>>
>>
>> -def main():
>> +def main() -> None:
>>      parser = argparse.ArgumentParser(description='AQMP TUI')
>>      parser.add_argument('qmp_server', help='Address of the QMP server'
>>                          '< UNIX socket path | TCP addr:port >')
>> @@ -311,7 +318,7 @@ def main():
>>      try:
>>          address = QEMUMonitorProtocol.parse_address(args.qmp_server)
>>      except QMPBadPortError as err:
>> -        parser.error(err)
>> +        parser.error(str(err))
>>
>>      app = App(address)
>>
>> @@ -330,4 +337,4 @@ def main():
>>
>>
>>  if __name__ == '__main__':
>> -    main()  # type: ignore
>> +    main()
>> diff --git a/python/setup.cfg b/python/setup.cfg
>> index 8cd9ac0d81..11c6240aba 100644
>> --- a/python/setup.cfg
>> +++ b/python/setup.cfg
>> @@ -83,9 +83,6 @@ namespace_packages = True
>>  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
>>
>> --
>> 2.17.1
>>
>>
> Please squash this into the first patch, but it looks good, thanks!
>
Thanks.

>
> --js
>

[-- Attachment #2: Type: text/html, Size: 15360 bytes --]

Re: [PATCH v3 01/13] python/aqmp: Fix wait_closed work-around for python 3.6

[Niteesh G. S.]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

On Thu, Aug 5, 2021 at 10:58 PM John Snow <jsnow@redhat.com> wrote:

>
>
> On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
> wrote:
>
>> Before this patch the wait_closed work-around for python 3.6
>> fails during disconnect.
>> This is a temproray work around for which might be fixed in the
>> future or will be completely removed when the minimum python
>> version is raised to 3.7.
>>
>> This patch was originally written by John Snow <jsnow@redhat.com>
>>
>> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
>> ---
>>  python/qemu/aqmp/util.py | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/python/qemu/aqmp/util.py b/python/qemu/aqmp/util.py
>> index de0df44cbd..eaa5fc7d5f 100644
>> --- a/python/qemu/aqmp/util.py
>> +++ b/python/qemu/aqmp/util.py
>> @@ -134,7 +134,17 @@ def is_closing(writer: asyncio.StreamWriter) -> bool:
>>
>>      while not transport.is_closing():
>>          await asyncio.sleep(0)
>> -    await flush(writer)
>> +
>> +    # This is an ugly workaround, but it's the best I can come up with.
>> +    sock = transport.get_extra_info('socket')
>> +
>> +    if sock is None:
>> +        # Our transport doesn't have a socket? ...
>> +        # Nothing we can reasonably do.
>> +        return
>> +
>> +    while sock.fileno() != -1:
>> +        await asyncio.sleep(0)
>>
>>
>>  def asyncio_run(coro: Coroutine[Any, Any, T], *, debug: bool = False) ->
>> T:
>> --
>> 2.17.1
>>
>>
> Sorry for the trouble. This is now included in the v3 version of my series
> and can be dropped. I hope.
>
Thanks. I'll remove this in the upcoming v4

[-- Attachment #2: Type: text/html, Size: 2658 bytes --]

Re: [PATCH v3 04/13] python/aqmp-tui: Add AQMP TUI draft

[Niteesh G. S.]

[-- Attachment #1: Type: text/plain, Size: 15856 bytes --]

On Fri, Aug 6, 2021 at 12:41 AM John Snow <jsnow@redhat.com> wrote:

>
>
> On Fri, Jul 30, 2021 at 4:19 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 | 333 +++++++++++++++++++++++++++++++++++
>>  python/setup.cfg             |  16 +-
>>  2 files changed, 348 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..ec9eba0aa7
>> --- /dev/null
>> +++ b/python/qemu/aqmp/aqmp_tui.py
>> @@ -0,0 +1,333 @@
>> +# 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
>> +from logging import Handler
>> +import signal
>> +
>> +import urwid
>> +import urwid_readline
>> +
>> +from ..qmp import QEMUMonitorProtocol, QMPBadPortError
>> +from .message import DeserializationError, Message, UnexpectedTypeError
>> +from .protocol import ConnectError
>> +from .qmp_client import ExecInterruptedError, QMPClient
>> +from .util import create_task, pretty_traceback
>> +
>> +
>> +UPDATE_MSG = 'UPDATE_MSG'
>> +
>> +# Using root logger to enable all loggers under qemu and asyncio
>> +LOGGER = logging.getLogger()
>> +
>> +
>> +def format_json(msg):
>> +    """
>> +    Formats given multiline JSON message into a single line message.
>> +    Converting into single line is more asthetically pleasing when
>> looking
>> +    along with error messages compared to multiline JSON.
>> +    """
>> +    # FIXME: Use better formatting mechanism. Might break at more
>> complex JSON
>> +    # data.
>> +    msg = msg.replace('\n', '')
>> +    words = msg.split(' ')
>> +    words = [word for word in words if word != '']
>> +    return ' '.join(words)
>> +
>> +
>> +class App(QMPClient):
>> +    def __init__(self, address):
>> +        urwid.register_signal(type(self), UPDATE_MSG)
>> +        self.window = Window(self)
>> +        self.address = address
>> +        self.aloop = None
>> +        super().__init__()
>> +
>> +    def add_to_history(self, msg):
>> +        urwid.emit_signal(self, UPDATE_MSG, msg)
>> +
>> +    def _cb_outbound(self, msg):
>> +        # FIXME: I think the ideal way to omit these messages during
>> in-TUI
>> +        # logging will be to add a filter to the logger. We can use
>> regex to
>> +        # filter out messages starting with 'Request:' or 'Response:'
>> but I
>> +        # think a better approach will be encapsulate the message in an
>> object
>> +        # and filter based on the object. Encapsulation of the message
>> will
>> +        # also be necessary when we want different formatting of messages
>> +        # inside TUI.
>> +        handler = LOGGER.handlers[0]
>> +        if not isinstance(handler, TUILogHandler):
>> +            LOGGER.debug('Request: %s', str(msg))
>> +        self.add_to_history('<-- ' + str(msg))
>> +        return msg
>> +
>> +    def _cb_inbound(self, msg):
>> +        handler = LOGGER.handlers[0]
>> +        if not isinstance(handler, TUILogHandler):
>> +            LOGGER.debug('Response: %s', str(msg))
>> +        self.add_to_history('--> ' + 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, raw_msg):
>> +        # FIXME: Format the raw_msg in history view to one line. It is
>> not
>> +        # pleasing to see multiple lines JSON object with an error
>> statement.
>> +        try:
>> +            msg = Message(bytes(raw_msg, encoding='utf-8'))
>> +            # Format multiline json into a single line JSON, since it is
>> more
>> +            # pleasing to look along with err message in TUI.
>> +            raw_msg = self.format_json(raw_msg)
>> +            await self._raw(msg, assign_id='id' not in msg)
>> +        except (ValueError, TypeError) as err:
>> +            LOGGER.info('Invalid message: %s', str(err))
>> +            self.add_to_history(f'{raw_msg}: {err}')
>> +        except (DeserializationError, UnexpectedTypeError) as err:
>> +            LOGGER.info('Invalid message: %s', err.error_message)
>> +            self.add_to_history(f'{raw_msg}: {err.error_message}')
>> +        except ExecInterruptedError:
>> +            LOGGER.info('Error server disconnected before reply')
>> +            urwid.emit_signal(self, UPDATE_MSG,
>> +                              '{"error": "Server disconnected before
>> reply"}')
>> +            self._set_status("Server disconnected")
>> +        except Exception as err:
>> +            LOGGER.error('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
>> +        try:
>> +            await self.disconnect()
>> +            LOGGER.debug('Disconnect finished. Exiting app')
>> +        except Exception as err:
>> +            LOGGER.info('_kill_app: %s', str(err))
>> +            # Let the app crash after providing a proper stack trace
>> +            raise err
>> +        raise urwid.ExitMainLoop()
>> +
>> +    def handle_event(self, event):
>> +        # FIXME: Consider all states present in qapi/run-state.json
>> +        if event['event'] == 'SHUTDOWN':
>> +            self._set_status('Server shutdown')
>> +
>> +    def _set_status(self, msg: str) -> None:
>> +        self.window.footer.set_text(msg)
>> +
>> +    def _get_formatted_address(self) -> str:
>> +        addr = f'{self.address}'
>> +        if isinstance(self.address, tuple):
>> +            host, port = self.address
>> +            addr = f'{host}:{port}'
>> +        return addr
>> +
>> +    async def connect_server(self):
>> +        try:
>> +            await self.connect(self.address)
>> +            addr = self._get_formatted_address()
>> +            self._set_status(f'Connected to {addr}')
>> +        except ConnectError as err:
>> +            LOGGER.info('connect_server: ConnectError %s', str(err))
>> +            self._set_status('Server shutdown')
>> +
>> +    def run(self, debug=False):
>> +        self.aloop = asyncio.get_event_loop()
>> +        self.aloop.set_debug(debug)
>> +
>> +        # 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)
>> +
>> +        event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
>> +        main_loop = urwid.MainLoop(urwid.AttrMap(self.window,
>> 'background'),
>> +                                   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:
>> +            main_loop.run()
>> +        except Exception as err:
>> +            LOGGER.error('%s\n%s\n', str(err), pretty_traceback())
>> +            raise err
>> +
>> +
>> +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)
>>
>
> cb_send_to_server calls create_task(App._send_to_server(msg)) at which
> point in the coroutine you finally do msg = Message(bytes(raw_msg,
> encoding='utf-8')). However, you can do your message conversion *first*,
> here in the Editor.
>
> That way, you can avoid erasing the editor bar when the message is garbled
> and the user has a chance to fix the corrupted message *first* before we
> dispatch to the callback-async-machine. That should reduce quite a lot of
> the error cases in _send_to_server and helps separate out the two kinds of
> errors we expect to see here:
>
> 1. The user typed something that's garbage, and we didn't actually even
> send it (because we couldn't), and
> 2. We sent (or tried to send) the message; but a failure occurred.
>
Thanks. Refactored this.

>
>
>> +            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_widget = EditorWidget(master)
>> +        self.editor = urwid.LineBox(self.editor_widget)
>> +        self.history = HistoryBox(master)
>> +        self.body = urwid.Pile([('weight', 80, self.history),
>> +                                ('weight', 20, self.editor)])
>> +        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 TUILogHandler(Handler):
>> +    def __init__(self, tui):
>> +        super().__init__()
>> +        self.tui = tui
>> +
>> +    def emit(self, record):
>> +        level = record.levelname
>> +        msg = record.getMessage()
>> +        msg = f'[{level}]: {msg}'
>> +        self.tui.add_to_history(msg)
>> +
>> +
>> +def main():
>> +    parser = argparse.ArgumentParser(description='AQMP TUI')
>> +    parser.add_argument('qmp_server', help='Address of the QMP server'
>> +                        '< UNIX socket path | TCP addr:port >')
>> +    parser.add_argument('--log-file', help='The Log file name')
>> +    parser.add_argument('--log-level', default='WARNING',
>> +                        help='Log level
>> <CRITICAL|ERROR|WARNING|INFO|DEBUG|>')
>> +    parser.add_argument('--asyncio-debug', action='store_true',
>> +                        help='Enable debug mode for asyncio loop'
>> +                        'Generates lot of output, makes TUI unusable
>> when'
>> +                        'logs are logged in the TUI itself.'
>> +                        'Use only when logging to a file')
>> +    args = parser.parse_args()
>> +
>> +    try:
>> +        address = QEMUMonitorProtocol.parse_address(args.qmp_server)
>> +    except QMPBadPortError as err:
>> +        parser.error(err)
>> +
>> +    app = App(address)
>> +
>> +    if args.log_file:
>> +        LOGGER.addHandler(logging.FileHandler(args.log_file))
>> +    else:
>> +        LOGGER.addHandler(TUILogHandler(app))
>> +
>> +    log_level = logging.getLevelName(args.log_level)
>> +    # getLevelName returns 'Level {log_level}' when a invalid level is
>> passed.
>> +    if log_level == f'Level {args.log_level}':
>> +        parser.error('Invalid log level')
>> +    LOGGER.setLevel(log_level)
>> +
>> +    app.run(args.asyncio_debug)
>> +
>> +
>> +if __name__ == '__main__':
>> +    main()  # type: ignore
>> diff --git a/python/setup.cfg b/python/setup.cfg
>> index d106a0ed7a..50f9894468 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
>>
>>

[-- Attachment #2: Type: text/html, Size: 19946 bytes --]

Re: [PATCH v3 04/13] python/aqmp-tui: Add AQMP TUI draft

[Niteesh G. S.]

[-- Attachment #1: Type: text/plain, Size: 20221 bytes --]

On Fri, Aug 6, 2021 at 12:28 AM John Snow <jsnow@redhat.com> wrote:

>
>
> On Fri, Jul 30, 2021 at 4:19 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 | 333 +++++++++++++++++++++++++++++++++++
>>  python/setup.cfg             |  16 +-
>>  2 files changed, 348 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..ec9eba0aa7
>> --- /dev/null
>> +++ b/python/qemu/aqmp/aqmp_tui.py
>> @@ -0,0 +1,333 @@
>> +# 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
>> +from logging import Handler
>> +import signal
>> +
>> +import urwid
>> +import urwid_readline
>> +
>> +from ..qmp import QEMUMonitorProtocol, QMPBadPortError
>> +from .message import DeserializationError, Message, UnexpectedTypeError
>> +from .protocol import ConnectError
>> +from .qmp_client import ExecInterruptedError, QMPClient
>> +from .util import create_task, pretty_traceback
>> +
>> +
>> +UPDATE_MSG = 'UPDATE_MSG'
>>
>
> This line still feels kind of "naked" on a cold read. It could use a small
> comment.
>
Fixed.

>
>
>> +
>> +# Using root logger to enable all loggers under qemu and asyncio
>> +LOGGER = logging.getLogger()
>>
>
> The comment isn't quite true; this is the root logger -- but you go on to
> use it to directly log messages. I don't think you should; use a
> module-level logger.
>
> (1) Create a module-level logger that is named after the current module
> name (e.g. qemu.aqmp.aqmp_tui) and use that for logging messages relating
> to this module:
> LOGGER = logging.getLogger(__name__)
>
> (2) Where you need to address the root logger, just use `root_logger =
> logging.getLogger() ` .... I think the main() function is the only place
> you might need this.
>
The way of logging in the TUI is changed such that, all logging is done
through the root level logger. The handlers and levels are installed to the
root level
logger to allow logging from other libraries to be rerouted to the screen
or file.

>
>
>
>> +
>> +
>> +def format_json(msg):
>> +    """
>> +    Formats given multiline JSON message into a single line message.
>> +    Converting into single line is more asthetically pleasing when
>> looking
>> +    along with error messages compared to multiline JSON.
>> +    """
>> +    # FIXME: Use better formatting mechanism. Might break at more
>> complex JSON
>> +    # data.
>> +    msg = msg.replace('\n', '')
>> +    words = msg.split(' ')
>> +    words = [word for word in words if word != '']
>> +    return ' '.join(words)
>> +
>>
>
> You can use the JSON module to do this,
> https://docs.python.org/3/library/json.html#json.dumps
>
> Message._serialize uses this technique to send JSON messages over the wire
> that have no newlines:
>
> https://gitlab.com/jsnow/qemu/-/blob/python-async-qmp-aqmp/python/qemu/aqmp/message.py#L132
>
> by not specifying an indent and including separators with no spaces, we
> can get a JSON representation with all the space squeezed out. You can add
> spaces and still omit the indent/newlines and so on.
>
But will this work for invalid JSON messages? As far as I have tried it
doesn't work.

>
>
>> +
>> +class App(QMPClient):
>> +    def __init__(self, address):
>> +        urwid.register_signal(type(self), UPDATE_MSG)
>>
>
> I found some hidden documentation about this -- I was struggling to find
> it before:
> https://github.com/urwid/urwid/blob/master/urwid/signals.py#L62
>
> So the first argument is meant to be "The class of an object sending the
> signal". Alright. And it looks like we only emit from add_to_history and in
> _send_to_server, so I guess that checks out!
>
> One thing I am noticing is that these signals are global and not
> per-instance, so if for some reason we ever create a second App I wonder if
> the __init__ method here will explode ... Well, it's probably not too
> important right now.
>
>>
>> +        self.window = Window(self)
>> +        self.address = address
>> +        self.aloop = None
>> +        super().__init__()
>> +
>> +    def add_to_history(self, msg):
>> +        urwid.emit_signal(self, UPDATE_MSG, msg)
>> +
>> +    def _cb_outbound(self, msg):
>> +        # FIXME: I think the ideal way to omit these messages during
>> in-TUI
>> +        # logging will be to add a filter to the logger. We can use
>> regex to
>> +        # filter out messages starting with 'Request:' or 'Response:'
>> but I
>> +        # think a better approach will be encapsulate the message in an
>> object
>> +        # and filter based on the object. Encapsulation of the message
>> will
>> +        # also be necessary when we want different formatting of messages
>> +        # inside TUI.
>>
> +        handler = LOGGER.handlers[0]
>> +        if not isinstance(handler, TUILogHandler):
>> +            LOGGER.debug('Request: %s', str(msg))
>> +        self.add_to_history('<-- ' + str(msg))
>> +        return msg
>> +
>> +    def _cb_inbound(self, msg):
>> +        handler = LOGGER.handlers[0]
>> +        if not isinstance(handler, TUILogHandler):
>> +            LOGGER.debug('Response: %s', str(msg))
>> +        self.add_to_history('--> ' + str(msg))
>> +        return msg
>> +
>> +    async def wait_for_events(self):
>> +        async for event in self.events:
>> +            self.handle_event(event)
>> +
>>
>
> Can handle_event be moved here so that related functions are somewhat near
> each other?
>
Fixed.

>
>
>> +    async def _send_to_server(self, raw_msg):
>> +        # FIXME: Format the raw_msg in history view to one line. It is
>> not
>> +        # pleasing to see multiple lines JSON object with an error
>> statement.
>> +        try:
>> +            msg = Message(bytes(raw_msg, encoding='utf-8'))
>> +            # Format multiline json into a single line JSON, since it is
>> more
>> +            # pleasing to look along with err message in TUI.
>> +            raw_msg = self.format_json(raw_msg)
>> +            await self._raw(msg, assign_id='id' not in msg)
>> +        except (ValueError, TypeError) as err:
>> +            LOGGER.info('Invalid message: %s', str(err))
>> +            self.add_to_history(f'{raw_msg}: {err}')
>> +        except (DeserializationError, UnexpectedTypeError) as err:
>> +            LOGGER.info('Invalid message: %s', err.error_message)
>> +            self.add_to_history(f'{raw_msg}: {err.error_message}')
>> +        except ExecInterruptedError:
>> +            LOGGER.info('Error server disconnected before reply')
>> +            urwid.emit_signal(self, UPDATE_MSG,
>> +                              '{"error": "Server disconnected before
>> reply"}')
>> +            self._set_status("Server disconnected")
>> +        except Exception as err:
>> +            LOGGER.error('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())
>> +
>>
>
> That logic needs to be here -- each commit needs to make sense by itself.
> If you've improved the connection logic, it needs to be merged into this
> commit.
>
Through this to-do, I meant to add the connection manager. I'll remove this
TODO since it's kind of misleading.

>
>
>> +    async def _kill_app(self):
>> +        # It is ok to call disconnect even in disconnect state
>> +        try:
>> +            await self.disconnect()
>> +            LOGGER.debug('Disconnect finished. Exiting app')
>> +        except Exception as err:
>> +            LOGGER.info('_kill_app: %s', str(err))
>> +            # Let the app crash after providing a proper stack trace
>> +            raise err
>> +        raise urwid.ExitMainLoop()
>> +
>> +    def handle_event(self, event):
>> +        # FIXME: Consider all states present in qapi/run-state.json
>> +        if event['event'] == 'SHUTDOWN':
>> +            self._set_status('Server shutdown')
>> +
>>
>
> This is an odd function to me because it will *overwrite* the status bar
> with this event, but surely shortly thereafter we're going to see a
> disconnection event from the transport itself, so this message will go away
> almost immediately, won't it?
>
It won't be overwritten in this patch. Once the server
shutdown/disconnects, the generated SHUTDOWN event is handled to update the
status bar and the library internally calls disconnect and that's it.
Only in the later patches do we decide whether to reconnect or not
that's when there is a chance of overwriting. But in this patch,
overwriting won't occur.

> I understand that connection management stuff is improved in later
> patches, but the initial patch here would be better served not having a
> partial and incorrect implementation. You can just remove features that you
> intend to "build out" in later commits. And if we remove this, then we
> don't need wait_for_events() either, and they can be re-added in a later
> commit when they're given a more full, proper treatment.
>
>
>> +    def _set_status(self, msg: str) -> None:
>> +        self.window.footer.set_text(msg)
>> +
>> +    def _get_formatted_address(self) -> str:
>> +        addr = f'{self.address}'
>> +        if isinstance(self.address, tuple):
>> +            host, port = self.address
>> +            addr = f'{host}:{port}'
>> +        return addr
>>
>
> Don't format addr twice in the TCP case -- make sure it formats it once.
> It won't matter much for performance in something like Python, but it's
> misleading when reading it.
>
Fixed.

>
> +
>> +    async def connect_server(self):
>> +        try:
>> +            await self.connect(self.address)
>> +            addr = self._get_formatted_address()
>> +            self._set_status(f'Connected to {addr}')
>> +        except ConnectError as err:
>> +            LOGGER.info('connect_server: ConnectError %s', str(err))
>> +            self._set_status('Server shutdown')
>>
>
> A failure to connect doesn't necessary mean the server was shut down.
>
Fixed.
It now shows [ConnectError: <Short description>] eg: [ConnectError: Failed
to establish connection].

>
>
>> +
>>
>
> I suspect this is changed in later commits in this series, but you should
> probably set the footer to something before we potentially hang on
> connect() and do a whole lot of nothing. LIke other critiques about
> unfinished interfaces, once you've implemented them, they should be worked
> into this patch so that each commit makes sense by itself. Reviewers will
> have to read these patches and make sense of them. If they have to flip
> through your entire series to understand what it will eventually look like,
> it's a lot more work and you're less likely to get good or helpful reviews.
>
>
>> +    def run(self, debug=False):
>> +        self.aloop = asyncio.get_event_loop()
>> +        self.aloop.set_debug(debug)
>> +
>> +        # 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)
>> +
>> +        event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
>> +        main_loop = urwid.MainLoop(urwid.AttrMap(self.window,
>> 'background'),
>> +                                   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:
>> +            main_loop.run()
>> +        except Exception as err:
>> +            LOGGER.error('%s\n%s\n', str(err), pretty_traceback())
>> +            raise err
>> +
>> +
>> +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_widget = EditorWidget(master)
>> +        self.editor = urwid.LineBox(self.editor_widget)
>> +        self.history = HistoryBox(master)
>> +        self.body = urwid.Pile([('weight', 80, self.history),
>> +                                ('weight', 20, self.editor)])
>> +        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 TUILogHandler(Handler):
>> +    def __init__(self, tui):
>> +        super().__init__()
>> +        self.tui = tui
>> +
>> +    def emit(self, record):
>> +        level = record.levelname
>> +        msg = record.getMessage()
>> +        msg = f'[{level}]: {msg}'
>> +        self.tui.add_to_history(msg)
>> +
>> +
>> +def main():
>> +    parser = argparse.ArgumentParser(description='AQMP TUI')
>> +    parser.add_argument('qmp_server', help='Address of the QMP server'
>> +                        '< UNIX socket path | TCP addr:port >')
>> +    parser.add_argument('--log-file', help='The Log file name')
>> +    parser.add_argument('--log-level', default='WARNING',
>> +                        help='Log level
>> <CRITICAL|ERROR|WARNING|INFO|DEBUG|>')
>> +    parser.add_argument('--asyncio-debug', action='store_true',
>> +                        help='Enable debug mode for asyncio loop'
>> +                        'Generates lot of output, makes TUI unusable
>> when'
>> +                        'logs are logged in the TUI itself.'
>> +                        'Use only when logging to a file')
>>
>
> Needs spaces between the lines here so that the output reads correctly.
>
The output renders properly for me. Can you please post a pic if it doesn't
render correctly for you?

>
>
>> +    args = parser.parse_args()
>> +
>> +    try:
>> +        address = QEMUMonitorProtocol.parse_address(args.qmp_server)
>> +    except QMPBadPortError as err:
>> +        parser.error(err)
>> +
>> +    app = App(address)
>> +
>> +    if args.log_file:
>> +        LOGGER.addHandler(logging.FileHandler(args.log_file))
>> +    else:
>> +        LOGGER.addHandler(TUILogHandler(app))
>> +
>> +    log_level = logging.getLevelName(args.log_level)
>> +    # getLevelName returns 'Level {log_level}' when a invalid level is
>> passed.
>> +    if log_level == f'Level {args.log_level}':
>> +        parser.error('Invalid log level')
>> +    LOGGER.setLevel(log_level)
>> +
>>
>
> I think you could do:
>
> LOGGER.setLevel(logging.getLevelName(args.log_level))
>
> And it'll fail if the level was unknown, but work otherwise. This would
> avoid a kind of precarious string-comparison check, which I worry is
> fragile.
>
Fixed.

>
>
>> +    app.run(args.asyncio_debug)
>> +
>> +
>> +if __name__ == '__main__':
>> +    main()  # type: ignore
>> diff --git a/python/setup.cfg b/python/setup.cfg
>> index d106a0ed7a..50f9894468 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
>>
>>
> I have some more thoughts, but need to research them and wanted to give
> some feedback first so you weren't waiting.
>
Thanks for the feedback.

[-- Attachment #2: Type: text/html, Size: 29317 bytes --]

Re: [PATCH v3 07/13] python: add optional pygments dependency

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 483 bytes --]

On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:

> 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>
>

LGTM

Reviewed-by: John Snow <jsnow@redhat.com>

[-- Attachment #2: Type: text/html, Size: 983 bytes --]

Re: [PATCH v3 08/13] python/aqmp-tui: add syntax highlighting

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 6326 bytes --]

On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:

> 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 | 52 +++++++++++++++++++++++++++---------
>  1 file changed, 40 insertions(+), 12 deletions(-)
>
> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
> index ab9ada793a..0d5ec62cb7 100644
> --- a/python/qemu/aqmp/aqmp_tui.py
> +++ b/python/qemu/aqmp/aqmp_tui.py
> @@ -19,6 +19,8 @@
>      Union,
>  )
>
> +from pygments import lexers
> +from pygments import token as Token
>  import urwid
>  import urwid_readline
>
> @@ -35,6 +37,22 @@
>  LOGGER = logging.getLogger()
>
>
> +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'),
> +    ('DEBUG', '', '', '', '#ddf', 'g7'),
> +    ('INFO', '', '', '', 'g100', 'g7'),
> +    ('WARNING', '', '', '', '#ff6', 'g7'),
> +    ('ERROR', '', '', '', '#a00', 'g7'),
> +    ('CRITICAL', '', '', '', '#a00', 'g7'),
> +    ('background', '', 'black', '', '', 'g7'),
> +]
> +
> +
>  def format_json(msg: str) -> str:
>      """
>      Formats given multiline JSON message into a single line message.
> @@ -57,17 +75,14 @@ def __init__(self, address: Union[str, Tuple[str,
> int]]) -> None:
>          self.aloop: Optional[Any] = None  # FIXME: Use more concrete type.
>          super().__init__()
>
> -    def add_to_history(self, msg: str) -> None:
> -        urwid.emit_signal(self, UPDATE_MSG, msg)
> +    def add_to_history(self, msg: str, level: Optional[str] = None) ->
> None:
> +        urwid.emit_signal(self, UPDATE_MSG, msg, level)
>
>      def _cb_outbound(self, msg: Message) -> Message:
>          # FIXME: I think the ideal way to omit these messages during
> in-TUI
> -        # logging will be to add a filter to the logger. We can use regex
> to
> -        # filter out messages starting with 'Request:' or 'Response:' but
> I
> -        # think a better approach will be encapsulate the message in an
> object
> -        # and filter based on the object. Encapsulation of the message
> will
> -        # also be necessary when we want different formatting of messages
> -        # inside TUI.
> +        # logging will be to add a filter to the logger. We can use
> +        # regex/startswith to filter out messages starting with
> 'Request:' or
> +        # 'Response:'. If possible please suggest other ideas.
>

We'll want to fix this up earlier in the series -- the finished version of
your series shouldn't have FIXME comments, and later patches in the series
shouldn't edit comments added earlier. We chatted on IRC about the right
strategy for filtering log messages, so I think V4 should have a strategy
in place to avoid this comment, right?


>          handler = LOGGER.handlers[0]
>          if not isinstance(handler, TUILogHandler):
>              LOGGER.debug('Request: %s', str(msg))
> @@ -156,6 +171,9 @@ def _get_formatted_address(self) -> str:
>              self._set_status('Server shutdown')
>
>      def run(self, debug: bool = False) -> None:
> +        screen = urwid.raw_display.Screen()
> +        screen.set_terminal_properties(256)
>

I'm not sure if this will work everywhere, but ... it's fine for now, I
assume.


> +
>          self.aloop = asyncio.get_event_loop()
>          self.aloop.set_debug(debug)
>
> @@ -167,6 +185,8 @@ def run(self, debug: bool = False) -> None:
>          event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
>          main_loop = urwid.MainLoop(urwid.AttrMap(self.window,
> 'background'),
>                                     unhandled_input=self.unhandled_input,
> +                                   screen=screen,
> +                                   palette=palette,
>                                     handle_mouse=True,
>                                     event_loop=event_loop)
>
> @@ -251,7 +271,8 @@ def __init__(self, master: App) -> None:
>          self.history = urwid.SimpleFocusListWalker([])
>          super().__init__(self.history)
>
> -    def add_to_history(self, history: str) -> None:
> +    def add_to_history(self,
> +                       history: Union[str, List[Tuple[str, str]]]) ->
> None:
>          self.history.append(urwid.Text(history))
>          if self.history:
>              self.history.set_focus(len(self.history) - 1)
> @@ -271,8 +292,15 @@ def __init__(self, master: App) -> None:
>          super().__init__(self.body)
>          urwid.connect_signal(self.master, UPDATE_MSG,
> self.cb_add_to_history)
>
> -    def cb_add_to_history(self, msg: str) -> None:
> -        self.history.add_to_history(msg)
> +    def cb_add_to_history(self, msg: str, level: Optional[str] = None) ->
> None:
> +        formatted = []
> +        if level:
> +            formatted.append((level, msg))
> +        else:
> +            lexer = lexers.JsonLexer()  # pylint: disable=no-member
> +            for token in lexer.get_tokens(msg):
> +                formatted.append(token)
> +        self.history.add_to_history(formatted)
>

This looks like it's conflating having a debugging level with receiving a
message that's already formatted.

i.e., the code here assumes that if it receives a level, it is ALSO already
formatted. That smells like trouble -- but I think there are some changes
to the logging and rendering you had planned for V4 anyway, I believe?


>
>
>  class Window(urwid.Frame):
> @@ -298,7 +326,7 @@ def emit(self, record: LogRecord) -> None:
>          level = record.levelname
>          msg = record.getMessage()
>          msg = f'[{level}]: {msg}'
> -        self.tui.add_to_history(msg)
> +        self.tui.add_to_history(msg, level)
>
>
>  def main() -> None:
> --
> 2.17.1
>
>

[-- Attachment #2: Type: text/html, Size: 8692 bytes --]

Re: [PATCH v3 08/13] python/aqmp-tui: add syntax highlighting

[Niteesh G. S.]

[-- Attachment #1: Type: text/plain, Size: 7542 bytes --]

On Tue, Aug 17, 2021 at 1:14 AM John Snow <jsnow@redhat.com> wrote:

>
>
> On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
> wrote:
>
>> 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 | 52 +++++++++++++++++++++++++++---------
>>  1 file changed, 40 insertions(+), 12 deletions(-)
>>
>> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
>> index ab9ada793a..0d5ec62cb7 100644
>> --- a/python/qemu/aqmp/aqmp_tui.py
>> +++ b/python/qemu/aqmp/aqmp_tui.py
>> @@ -19,6 +19,8 @@
>>      Union,
>>  )
>>
>> +from pygments import lexers
>> +from pygments import token as Token
>>  import urwid
>>  import urwid_readline
>>
>> @@ -35,6 +37,22 @@
>>  LOGGER = logging.getLogger()
>>
>>
>> +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'),
>> +    ('DEBUG', '', '', '', '#ddf', 'g7'),
>> +    ('INFO', '', '', '', 'g100', 'g7'),
>> +    ('WARNING', '', '', '', '#ff6', 'g7'),
>> +    ('ERROR', '', '', '', '#a00', 'g7'),
>> +    ('CRITICAL', '', '', '', '#a00', 'g7'),
>> +    ('background', '', 'black', '', '', 'g7'),
>> +]
>> +
>> +
>>  def format_json(msg: str) -> str:
>>      """
>>      Formats given multiline JSON message into a single line message.
>> @@ -57,17 +75,14 @@ def __init__(self, address: Union[str, Tuple[str,
>> int]]) -> None:
>>          self.aloop: Optional[Any] = None  # FIXME: Use more concrete
>> type.
>>          super().__init__()
>>
>> -    def add_to_history(self, msg: str) -> None:
>> -        urwid.emit_signal(self, UPDATE_MSG, msg)
>> +    def add_to_history(self, msg: str, level: Optional[str] = None) ->
>> None:
>> +        urwid.emit_signal(self, UPDATE_MSG, msg, level)
>>
>>      def _cb_outbound(self, msg: Message) -> Message:
>>          # FIXME: I think the ideal way to omit these messages during
>> in-TUI
>> -        # logging will be to add a filter to the logger. We can use
>> regex to
>> -        # filter out messages starting with 'Request:' or 'Response:'
>> but I
>> -        # think a better approach will be encapsulate the message in an
>> object
>> -        # and filter based on the object. Encapsulation of the message
>> will
>> -        # also be necessary when we want different formatting of messages
>> -        # inside TUI.
>> +        # logging will be to add a filter to the logger. We can use
>> +        # regex/startswith to filter out messages starting with
>> 'Request:' or
>> +        # 'Response:'. If possible please suggest other ideas.
>>
>
> We'll want to fix this up earlier in the series -- the finished version of
> your series shouldn't have FIXME comments, and later patches in the series
> shouldn't edit comments added earlier. We chatted on IRC about the right
> strategy for filtering log messages, so I think V4 should have a strategy
> in place to avoid this comment, right?
>
Yes, this has been addressed in the v4.
We avoid logging messages when we log the messages to the TUI screen. This
is done by checking if TUILogHandler is installed on the logger or not.

>
>
>>          handler = LOGGER.handlers[0]
>>          if not isinstance(handler, TUILogHandler):
>>              LOGGER.debug('Request: %s', str(msg))
>> @@ -156,6 +171,9 @@ def _get_formatted_address(self) -> str:
>>              self._set_status('Server shutdown')
>>
>>      def run(self, debug: bool = False) -> None:
>> +        screen = urwid.raw_display.Screen()
>> +        screen.set_terminal_properties(256)
>>
>
> I'm not sure if this will work everywhere, but ... it's fine for now, I
> assume.
>
I'll add a note here.

> +
>>          self.aloop = asyncio.get_event_loop()
>>          self.aloop.set_debug(debug)
>>
>> @@ -167,6 +185,8 @@ def run(self, debug: bool = False) -> None:
>>          event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
>>          main_loop = urwid.MainLoop(urwid.AttrMap(self.window,
>> 'background'),
>>                                     unhandled_input=self.unhandled_input,
>> +                                   screen=screen,
>> +                                   palette=palette,
>>                                     handle_mouse=True,
>>                                     event_loop=event_loop)
>>
>> @@ -251,7 +271,8 @@ def __init__(self, master: App) -> None:
>>          self.history = urwid.SimpleFocusListWalker([])
>>          super().__init__(self.history)
>>
>> -    def add_to_history(self, history: str) -> None:
>> +    def add_to_history(self,
>> +                       history: Union[str, List[Tuple[str, str]]]) ->
>> None:
>>          self.history.append(urwid.Text(history))
>>          if self.history:
>>              self.history.set_focus(len(self.history) - 1)
>> @@ -271,8 +292,15 @@ def __init__(self, master: App) -> None:
>>          super().__init__(self.body)
>>          urwid.connect_signal(self.master, UPDATE_MSG,
>> self.cb_add_to_history)
>>
>> -    def cb_add_to_history(self, msg: str) -> None:
>> -        self.history.add_to_history(msg)
>> +    def cb_add_to_history(self, msg: str, level: Optional[str] = None)
>> -> None:
>> +        formatted = []
>> +        if level:
>> +            formatted.append((level, msg))
>> +        else:
>> +            lexer = lexers.JsonLexer()  # pylint: disable=no-member
>> +            for token in lexer.get_tokens(msg):
>> +                formatted.append(token)
>> +        self.history.add_to_history(formatted)
>>
>
> This looks like it's conflating having a debugging level with receiving a
> message that's already formatted.
>
> i.e., the code here assumes that if it receives a level, it is ALSO
> already formatted. That smells like trouble -- but I think there are some
> changes to the logging and rendering you had planned for V4 anyway, I
> believe?
>
Why do you think it is trouble?
When a level is provided, it is most probably going to be text messages or
QMP messages with text. In either case, there is no formatting to be done.
In the second case, we don't have a JSON formattor that can format
a JSON message along with text without breaking. So I thought it is better
to leave the formatting to the user and just use the level to provide
syntax highlighting.

One thing I have changed in v4 is the level is prefixed to the messages
rather than the user adding it manually.
i.e. in v3 -
         self.add_to_history('[ERROR]: This is error', 'ERROR')   results
in  [ERROR]: This is error
     in v4 -
         self.add_to_history('This is error', 'ERROR')   results in
[ERROR]: This is error


>>  class Window(urwid.Frame):
>> @@ -298,7 +326,7 @@ def emit(self, record: LogRecord) -> None:
>>          level = record.levelname
>>          msg = record.getMessage()
>>          msg = f'[{level}]: {msg}'
>> -        self.tui.add_to_history(msg)
>> +        self.tui.add_to_history(msg, level)
>>
>>
>>  def main() -> None:
>> --
>> 2.17.1
>>
>>

[-- Attachment #2: Type: text/html, Size: 11525 bytes --]

Re: [PATCH v3 09/13] python/aqmp-tui: Add QMP connection manager

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 9711 bytes --]

On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
wrote:

> Instead of manually connecting and disconnecting from the
> server. We now rely on the runstate to manage the QMP
> connection.
>
> Along with this the ability to reconnect on certain exceptions
> has also been added.
>
> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
> ---
>  python/qemu/aqmp/aqmp_tui.py | 109 ++++++++++++++++++++++++++++++-----
>  1 file changed, 94 insertions(+), 15 deletions(-)
>
> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
> index 0d5ec62cb7..ef91883fa5 100644
> --- a/python/qemu/aqmp/aqmp_tui.py
> +++ b/python/qemu/aqmp/aqmp_tui.py
> @@ -25,8 +25,9 @@
>  import urwid_readline
>
>  from ..qmp import QEMUMonitorProtocol, QMPBadPortError
> +from .error import ProtocolError
>  from .message import DeserializationError, Message, UnexpectedTypeError
> -from .protocol import ConnectError
> +from .protocol import ConnectError, Runstate
>  from .qmp_client import ExecInterruptedError, QMPClient
>  from .util import create_task, pretty_traceback
>
> @@ -67,12 +68,24 @@ def format_json(msg: str) -> str:
>      return ' '.join(words)
>
>
> +def type_name(mtype: Any) -> str:
> +    """
> +    Returns the type name
> +    """
> +    return type(mtype).__name__
>

This is a lot of lines for something that doesn't do very much -- do we
really need it?


> +
> +
>  class App(QMPClient):
> -    def __init__(self, address: Union[str, Tuple[str, int]]) -> None:
> +    def __init__(self, address: Union[str, Tuple[str, int]], num_retries:
> int,
> +                 retry_delay: Optional[int]) -> None:
>          urwid.register_signal(type(self), UPDATE_MSG)
>          self.window = Window(self)
>          self.address = address
>          self.aloop: Optional[Any] = None  # FIXME: Use more concrete type.
> +        self.num_retries = num_retries
> +        self.retry_delay = retry_delay
> +        self.retry: bool = False
> +        self.disconnecting: bool = False
>

Why is this one needed again ? ...


>          super().__init__()
>
>      def add_to_history(self, msg: str, level: Optional[str] = None) ->
> None:
> @@ -119,7 +132,7 @@ def _cb_inbound(self, msg: Message) -> Message:
>              LOGGER.info('Error server disconnected before reply')
>              urwid.emit_signal(self, UPDATE_MSG,
>                                '{"error": "Server disconnected before
> reply"}')
> -            self._set_status("Server disconnected")
> +            await self.disconnect()
>          except Exception as err:
>              LOGGER.error('Exception from _send_to_server: %s', str(err))
>              raise err
> @@ -136,15 +149,29 @@ def kill_app(self) -> None:
>          create_task(self._kill_app())
>
>      async def _kill_app(self) -> None:
> -        # It is ok to call disconnect even in disconnect state
> +        await self.disconnect()
> +        LOGGER.debug('Disconnect finished. Exiting app')
> +        raise urwid.ExitMainLoop()
> +
> +    async def disconnect(self) -> None:
> +        if self.disconnecting:
> +            return
>          try:
> -            await self.disconnect()
> -            LOGGER.debug('Disconnect finished. Exiting app')
> +            self.disconnecting = True
> +            await super().disconnect()
> +            self.retry = True
> +        except EOFError as err:
> +            LOGGER.info('disconnect: %s', type_name(err))
> +            self.retry = True
> +        except ProtocolError as err:
> +            LOGGER.info('disconnect: %s', type_name(err))
> +            self.retry = False
>          except Exception as err:
> -            LOGGER.info('_kill_app: %s', str(err))
> -            # Let the app crash after providing a proper stack trace
> +            LOGGER.error('disconnect: Unhandled exception %s', str(err))
> +            self.retry = False
>              raise err
> -        raise urwid.ExitMainLoop()
> +        finally:
> +            self.disconnecting = False
>
>      def handle_event(self, event: Message) -> None:
>          # FIXME: Consider all states present in qapi/run-state.json
> @@ -161,14 +188,61 @@ def _get_formatted_address(self) -> str:
>              addr = f'{host}:{port}'
>          return addr
>
> -    async def connect_server(self) -> None:
> +    async def _retry_connection(self) -> Optional[str]:
> +        current_retries = 0
> +        err = None
> +        # Increase in power sequence of 2 if no delay is provided
> +        cur_delay = 1
> +        inc_delay = 2
> +        if self.retry_delay:
> +            inc_delay = 1
> +            cur_delay = self.retry_delay
> +        # initial try
> +        await self.connect_server()
> +        while self.retry and current_retries < self.num_retries:
> +            LOGGER.info('Connection Failed, retrying in %d', cur_delay)
> +            status = f'[Retry #{current_retries} ({cur_delay}s)]'
> +            self._set_status(status)
> +
> +            await asyncio.sleep(cur_delay)
> +
> +            err = await self.connect_server()
> +            cur_delay *= inc_delay
> +            # Cap delay to 5mins
> +            cur_delay = min(cur_delay, 5 * 60)
> +            current_retries += 1
> +        # If all retries failed report the last error
> +        LOGGER.info('All retries failed: %s', str(err))
> +        return type_name(err)
>

I had suggested something like an exponential backoff, but maybe a constant
delay would be a little cleaner to implement for right now without getting
too fancy over it. If you go with a simpler retry algorithm, do you think
you could clean up the logic in the retry loop here a bit more?

Something like:

for _ in range(num_retries):
    try:
        whatever_you_have_to_do_to_connect()
        return
    except ConnectError as err:
        LOGGER.info(...etc)
    await asyncio.sleep(whatever_the_delay_is)
# ran out of retries here, presumably the connection manager will just go
idle until the user interferes some other way.

In particular, I think passing around the name of the exception is a little
dubious -- we should be logging with the actual Exception we've received.


> +
> +    async def manage_connection(self) -> None:
> +        while True:
> +            if self.runstate == Runstate.IDLE:
> +                LOGGER.info('Trying to reconnect')
>

But will this be true upon the very first boot? This message might not be
right.


> +                err = await self._retry_connection()
>

This seems named oddly too, since it might be the initial attempt and not
necessarily a reconnection or a retry.


> +                # If retry is still true then, we have exhausted all our
> tries.
> +                if self.retry:
> +                    self._set_status(f'Error: {err}')
>
+                else:
> +                    addr = self._get_formatted_address()
> +                    self._set_status(f'[Connected {addr}]')
> +            elif self.runstate == Runstate.DISCONNECTING:
> +                self._set_status('[Disconnected]')
> +                await self.disconnect()
> +                # check if a retry is needed
>

Is this required? I would have hoped that after calling disconnect that the
state would have again changed to IDLE and you wouldn't need this clause
here.


> +                if self.runstate == Runstate.IDLE:
> +                    continue
> +            await self.runstate_changed()
> +
> +    async def connect_server(self) -> Optional[str]:
>          try:
>              await self.connect(self.address)
> -            addr = self._get_formatted_address()
> -            self._set_status(f'Connected to {addr}')
> +            self.retry = False
>          except ConnectError as err:
>              LOGGER.info('connect_server: ConnectError %s', str(err))
> -            self._set_status('Server shutdown')
> +            self.retry = True
> +            return type_name(err)
> +        return None
>
>      def run(self, debug: bool = False) -> None:
>          screen = urwid.raw_display.Screen()
> @@ -191,7 +265,7 @@ def run(self, debug: bool = False) -> None:
>                                     event_loop=event_loop)
>
>          create_task(self.wait_for_events(), self.aloop)
> -        create_task(self.connect_server(), self.aloop)
> +        create_task(self.manage_connection(), self.aloop)
>          try:
>              main_loop.run()
>          except Exception as err:
> @@ -333,6 +407,11 @@ def main() -> None:
>      parser = argparse.ArgumentParser(description='AQMP TUI')
>      parser.add_argument('qmp_server', help='Address of the QMP server'
>                          '< UNIX socket path | TCP addr:port >')
> +    parser.add_argument('--num-retries', type=int, default=10,
> +                        help='Number of times to reconnect before giving
> up')
> +    parser.add_argument('--retry-delay', type=int,
> +                        help='Time(s) to wait before next retry.'
> +                        'Default action is to increase delay in powers of
> 2')
>      parser.add_argument('--log-file', help='The Log file name')
>      parser.add_argument('--log-level', default='WARNING',
>                          help='Log level
> <CRITICAL|ERROR|WARNING|INFO|DEBUG|>')
> @@ -348,7 +427,7 @@ def main() -> None:
>      except QMPBadPortError as err:
>          parser.error(str(err))
>
> -    app = App(address)
> +    app = App(address, args.num_retries, args.retry_delay)
>
>      if args.log_file:
>          LOGGER.addHandler(logging.FileHandler(args.log_file))
> --
> 2.17.1
>
>
Right idea overall - possibly needs some polish and to be integrated with
an earlier patch to avoid the intermediate FIXMEs.

Thanks,
--js

[-- Attachment #2: Type: text/html, Size: 13709 bytes --]

Re: [PATCH v3 09/13] python/aqmp-tui: Add QMP connection manager

[Niteesh G. S.]

[-- Attachment #1: Type: text/plain, Size: 13588 bytes --]

On Tue, Aug 17, 2021 at 10:21 AM John Snow <jsnow@redhat.com> wrote:

>
>
> On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
> wrote:
>
>> Instead of manually connecting and disconnecting from the
>> server. We now rely on the runstate to manage the QMP
>> connection.
>>
>> Along with this the ability to reconnect on certain exceptions
>> has also been added.
>>
>> Signed-off-by: G S Niteesh Babu <niteesh.gs@gmail.com>
>> ---
>>  python/qemu/aqmp/aqmp_tui.py | 109 ++++++++++++++++++++++++++++++-----
>>  1 file changed, 94 insertions(+), 15 deletions(-)
>>
>> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
>> index 0d5ec62cb7..ef91883fa5 100644
>> --- a/python/qemu/aqmp/aqmp_tui.py
>> +++ b/python/qemu/aqmp/aqmp_tui.py
>> @@ -25,8 +25,9 @@
>>  import urwid_readline
>>
>>  from ..qmp import QEMUMonitorProtocol, QMPBadPortError
>> +from .error import ProtocolError
>>  from .message import DeserializationError, Message, UnexpectedTypeError
>> -from .protocol import ConnectError
>> +from .protocol import ConnectError, Runstate
>>  from .qmp_client import ExecInterruptedError, QMPClient
>>  from .util import create_task, pretty_traceback
>>
>> @@ -67,12 +68,24 @@ def format_json(msg: str) -> str:
>>      return ' '.join(words)
>>
>>
>> +def type_name(mtype: Any) -> str:
>> +    """
>> +    Returns the type name
>> +    """
>> +    return type(mtype).__name__
>>
>
> This is a lot of lines for something that doesn't do very much -- do we
> really need it?
>
No. This has been removed in v4.

>
>
>> +
>> +
>>  class App(QMPClient):
>> -    def __init__(self, address: Union[str, Tuple[str, int]]) -> None:
>> +    def __init__(self, address: Union[str, Tuple[str, int]],
>> num_retries: int,
>> +                 retry_delay: Optional[int]) -> None:
>>          urwid.register_signal(type(self), UPDATE_MSG)
>>          self.window = Window(self)
>>          self.address = address
>>          self.aloop: Optional[Any] = None  # FIXME: Use more concrete
>> type.
>> +        self.num_retries = num_retries
>> +        self.retry_delay = retry_delay
>> +        self.retry: bool = False
>> +        self.disconnecting: bool = False
>>
>
> Why is this one needed again ? ...
>
A race condition occurs in protocol.py line 597
The reason behind this is there are two disconnect calls initiated. The
first one via kill_app
and the second one via manage_connection when the state is set to
disconnecting by the first call.
One of the calls set's the state to IDLE(protocol.py:584) after it has
finished disconnecting, meanwhile
the second call is somehow in the process of disconnecting and assert the
state to be in DISCONNECTING
in protocol.py:597, which it is not since it has been set to IDLE by the
first call.

If I don't gaurd against the second call I get the following exception
------------------------------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/niteesh/development/qemu/python/.venv/bin/aqmp-tui", line 33,
in <module>
    sys.exit(load_entry_point('qemu', 'console_scripts', 'aqmp-tui')())
  File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line
695, in main
    app.run(args.asyncio_debug)
  File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line
444, in run
    raise err
  File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line
441, in run
    main_loop.run()
  File
"/home/niteesh/development/qemu/python/.venv/lib/python3.6/site-packages/urwid/main_loop.py",
line 287, in run
    self._run()
  File
"/home/niteesh/development/qemu/python/.venv/lib/python3.6/site-packages/urwid/main_loop.py",
line 385, in _run
    self.event_loop.run()
  File
"/home/niteesh/development/qemu/python/.venv/lib/python3.6/site-packages/urwid/main_loop.py",
line 1494, in run
    reraise(*exc_info)
  File
"/home/niteesh/development/qemu/python/.venv/lib/python3.6/site-packages/urwid/compat.py",
line 58, in reraise
    raise value
  File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line
391, in manage_connection
    await self.disconnect()
  File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line
312, in disconnect
    raise err
  File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line
300, in disconnect
    await super().disconnect()
  File "/home/niteesh/development/qemu/python/qemu/aqmp/protocol.py", line
302, in disconnect
    await self._wait_disconnect()
  File "/home/niteesh/development/qemu/python/qemu/aqmp/protocol.py", line
583, in _wait_disconnect
    self._cleanup()
  File "/home/niteesh/development/qemu/python/qemu/aqmp/qmp_client.py",
line 331, in _cleanup
    super()._cleanup()
  File "/home/niteesh/development/qemu/python/qemu/aqmp/protocol.py", line
597, in _cleanup
    assert self.runstate == Runstate.DISCONNECTING
AssertionError
-------------------------------------------------------------------------------------------


>
>>          super().__init__()
>>
>>      def add_to_history(self, msg: str, level: Optional[str] = None) ->
>> None:
>> @@ -119,7 +132,7 @@ def _cb_inbound(self, msg: Message) -> Message:
>>              LOGGER.info('Error server disconnected before reply')
>>              urwid.emit_signal(self, UPDATE_MSG,
>>                                '{"error": "Server disconnected before
>> reply"}')
>> -            self._set_status("Server disconnected")
>> +            await self.disconnect()
>>          except Exception as err:
>>              LOGGER.error('Exception from _send_to_server: %s', str(err))
>>              raise err
>> @@ -136,15 +149,29 @@ def kill_app(self) -> None:
>>          create_task(self._kill_app())
>>
>>      async def _kill_app(self) -> None:
>> -        # It is ok to call disconnect even in disconnect state
>> +        await self.disconnect()
>> +        LOGGER.debug('Disconnect finished. Exiting app')
>> +        raise urwid.ExitMainLoop()
>> +
>> +    async def disconnect(self) -> None:
>> +        if self.disconnecting:
>> +            return
>>          try:
>> -            await self.disconnect()
>> -            LOGGER.debug('Disconnect finished. Exiting app')
>> +            self.disconnecting = True
>> +            await super().disconnect()
>> +            self.retry = True
>> +        except EOFError as err:
>> +            LOGGER.info('disconnect: %s', type_name(err))
>> +            self.retry = True
>> +        except ProtocolError as err:
>> +            LOGGER.info('disconnect: %s', type_name(err))
>> +            self.retry = False
>>          except Exception as err:
>> -            LOGGER.info('_kill_app: %s', str(err))
>> -            # Let the app crash after providing a proper stack trace
>> +            LOGGER.error('disconnect: Unhandled exception %s', str(err))
>> +            self.retry = False
>>              raise err
>> -        raise urwid.ExitMainLoop()
>> +        finally:
>> +            self.disconnecting = False
>>
>>      def handle_event(self, event: Message) -> None:
>>          # FIXME: Consider all states present in qapi/run-state.json
>> @@ -161,14 +188,61 @@ def _get_formatted_address(self) -> str:
>>              addr = f'{host}:{port}'
>>          return addr
>>
>> -    async def connect_server(self) -> None:
>> +    async def _retry_connection(self) -> Optional[str]:
>> +        current_retries = 0
>> +        err = None
>> +        # Increase in power sequence of 2 if no delay is provided
>> +        cur_delay = 1
>> +        inc_delay = 2
>> +        if self.retry_delay:
>> +            inc_delay = 1
>> +            cur_delay = self.retry_delay
>> +        # initial try
>> +        await self.connect_server()
>> +        while self.retry and current_retries < self.num_retries:
>> +            LOGGER.info('Connection Failed, retrying in %d', cur_delay)
>> +            status = f'[Retry #{current_retries} ({cur_delay}s)]'
>> +            self._set_status(status)
>> +
>> +            await asyncio.sleep(cur_delay)
>> +
>> +            err = await self.connect_server()
>> +            cur_delay *= inc_delay
>> +            # Cap delay to 5mins
>> +            cur_delay = min(cur_delay, 5 * 60)
>> +            current_retries += 1
>> +        # If all retries failed report the last error
>> +        LOGGER.info('All retries failed: %s', str(err))
>> +        return type_name(err)
>>
>
> I had suggested something like an exponential backoff, but maybe a
> constant delay would be a little cleaner to implement for right now without
> getting too fancy over it. If you go with a simpler retry algorithm, do you
> think you could clean up the logic in the retry loop here a bit more?
>
Yes, we can. I'll refactor it to constant delay.


> Something like:
>
> for _ in range(num_retries):
>     try:
>         whatever_you_have_to_do_to_connect()
>         return
>     except ConnectError as err:
>         LOGGER.info(...etc)
>     await asyncio.sleep(whatever_the_delay_is)
> # ran out of retries here, presumably the connection manager will just go
> idle until the user interferes some other way.
>

> In particular, I think passing around the name of the exception is a
> little dubious -- we should be logging with the actual Exception we've
> received.
>
This has been fixed in V4. We pass the exception now instead of just
passing around the name.

>
>
>> +
>> +    async def manage_connection(self) -> None:
>> +        while True:
>> +            if self.runstate == Runstate.IDLE:
>> +                LOGGER.info('Trying to reconnect')
>>
>
> But will this be true upon the very first boot? This message might not be
> right.
>
Yes, it also occurs in the first boot. I'll fix this in the V3.

>
>
>> +                err = await self._retry_connection()
>>
>
> This seems named oddly too, since it might be the initial attempt and not
> necessarily a reconnection or a retry.
>
Will fix that.

>
>
>> +                # If retry is still true then, we have exhausted all our
>> tries.
>> +                if self.retry:
>> +                    self._set_status(f'Error: {err}')
>>
> +                else:
>> +                    addr = self._get_formatted_address()
>> +                    self._set_status(f'[Connected {addr}]')
>> +            elif self.runstate == Runstate.DISCONNECTING:
>> +                self._set_status('[Disconnected]')
>> +                await self.disconnect()
>> +                # check if a retry is needed
>>
>
> Is this required? I would have hoped that after calling disconnect that
> the state would have again changed to IDLE and you wouldn't need this
> clause here.
>
After you mentioned it I too felt it was redundant. But on removing it the
whole app freezes when trying to exit.
I logged the state after the call to disconnect, instead of being in the
IDLE state, it is still in DISCONNECTING state.
I suspect this results in the constant infinite looping which doesn't give
other coroutines a chance to run and blocks
the event loop thus resulting in the freezing of the app. But I am not sure
why the state isn't changing to IDLE.

>
>
>> +                if self.runstate == Runstate.IDLE:
>> +                    continue
>> +            await self.runstate_changed()
>> +
>> +    async def connect_server(self) -> Optional[str]:
>>          try:
>>              await self.connect(self.address)
>> -            addr = self._get_formatted_address()
>> -            self._set_status(f'Connected to {addr}')
>> +            self.retry = False
>>          except ConnectError as err:
>>              LOGGER.info('connect_server: ConnectError %s', str(err))
>> -            self._set_status('Server shutdown')
>> +            self.retry = True
>> +            return type_name(err)
>> +        return None
>>
>>      def run(self, debug: bool = False) -> None:
>>          screen = urwid.raw_display.Screen()
>> @@ -191,7 +265,7 @@ def run(self, debug: bool = False) -> None:
>>                                     event_loop=event_loop)
>>
>>          create_task(self.wait_for_events(), self.aloop)
>> -        create_task(self.connect_server(), self.aloop)
>> +        create_task(self.manage_connection(), self.aloop)
>>          try:
>>              main_loop.run()
>>          except Exception as err:
>> @@ -333,6 +407,11 @@ def main() -> None:
>>      parser = argparse.ArgumentParser(description='AQMP TUI')
>>      parser.add_argument('qmp_server', help='Address of the QMP server'
>>                          '< UNIX socket path | TCP addr:port >')
>> +    parser.add_argument('--num-retries', type=int, default=10,
>> +                        help='Number of times to reconnect before giving
>> up')
>> +    parser.add_argument('--retry-delay', type=int,
>> +                        help='Time(s) to wait before next retry.'
>> +                        'Default action is to increase delay in powers
>> of 2')
>>      parser.add_argument('--log-file', help='The Log file name')
>>      parser.add_argument('--log-level', default='WARNING',
>>                          help='Log level
>> <CRITICAL|ERROR|WARNING|INFO|DEBUG|>')
>> @@ -348,7 +427,7 @@ def main() -> None:
>>      except QMPBadPortError as err:
>>          parser.error(str(err))
>>
>> -    app = App(address)
>> +    app = App(address, args.num_retries, args.retry_delay)
>>
>>      if args.log_file:
>>          LOGGER.addHandler(logging.FileHandler(args.log_file))
>> --
>> 2.17.1
>>
>>
> Right idea overall - possibly needs some polish and to be integrated with
> an earlier patch to avoid the intermediate FIXMEs.
>
> Thanks,
> --js
>

[-- Attachment #2: Type: text/html, Size: 20835 bytes --]

Re: [PATCH v3 04/13] python/aqmp-tui: Add AQMP TUI draft

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 4579 bytes --]

On Fri, Aug 13, 2021 at 11:11 AM Niteesh G. S. <niteesh.gs@gmail.com> wrote:

>
> On Fri, Aug 6, 2021 at 12:28 AM John Snow <jsnow@redhat.com> wrote:
>
>>
>> On Fri, Jul 30, 2021 at 4:19 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>
>>>
>>
...


> +
>>> +# Using root logger to enable all loggers under qemu and asyncio
>>> +LOGGER = logging.getLogger()
>>>
>>
>> The comment isn't quite true; this is the root logger -- but you go on to
>> use it to directly log messages. I don't think you should; use a
>> module-level logger.
>>
>> (1) Create a module-level logger that is named after the current module
>> name (e.g. qemu.aqmp.aqmp_tui) and use that for logging messages relating
>> to this module:
>> LOGGER = logging.getLogger(__name__)
>>
>> (2) Where you need to address the root logger, just use `root_logger =
>> logging.getLogger() ` .... I think the main() function is the only place
>> you might need this.
>>
> The way of logging in the TUI is changed such that, all logging is done
> through the root level logger. The handlers and levels are installed to
> the root level
> logger to allow logging from other libraries to be rerouted to the screen
> or file.
>

We talked about this on IRC, so I'll assume our disagreement in vision here
is cleared up ... or at least different :)


> +
>>> +
>>> +def format_json(msg):
>>> +    """
>>> +    Formats given multiline JSON message into a single line message.
>>> +    Converting into single line is more asthetically pleasing when
>>> looking
>>> +    along with error messages compared to multiline JSON.
>>> +    """
>>> +    # FIXME: Use better formatting mechanism. Might break at more
>>> complex JSON
>>> +    # data.
>>> +    msg = msg.replace('\n', '')
>>> +    words = msg.split(' ')
>>> +    words = [word for word in words if word != '']
>>> +    return ' '.join(words)
>>> +
>>>
>>
>> You can use the JSON module to do this,
>> https://docs.python.org/3/library/json.html#json.dumps
>>
>> Message._serialize uses this technique to send JSON messages over the
>> wire that have no newlines:
>>
>> https://gitlab.com/jsnow/qemu/-/blob/python-async-qmp-aqmp/python/qemu/aqmp/message.py#L132
>>
>> by not specifying an indent and including separators with no spaces, we
>> can get a JSON representation with all the space squeezed out. You can add
>> spaces and still omit the indent/newlines and so on.
>>
>

> But will this work for invalid JSON messages? As far as I have tried it
> doesn't work.
>

Ah, I see ... Nope, that requires a valid JSON message ... Do we *need* to
pretty-print invalid JSON?


> +
>>> +def main():
>>> +    parser = argparse.ArgumentParser(description='AQMP TUI')
>>> +    parser.add_argument('qmp_server', help='Address of the QMP server'
>>> +                        '< UNIX socket path | TCP addr:port >')
>>> +    parser.add_argument('--log-file', help='The Log file name')
>>> +    parser.add_argument('--log-level', default='WARNING',
>>> +                        help='Log level
>>> <CRITICAL|ERROR|WARNING|INFO|DEBUG|>')
>>> +    parser.add_argument('--asyncio-debug', action='store_true',
>>> +                        help='Enable debug mode for asyncio loop'
>>> +                        'Generates lot of output, makes TUI unusable
>>> when'
>>> +                        'logs are logged in the TUI itself.'
>>> +                        'Use only when logging to a file')
>>>
>>
>> Needs spaces between the lines here so that the output reads correctly.
>>
> The output renders properly for me. Can you please post a pic if it
> doesn't render correctly for you?
>

No screenshot needed, just try running with '--help'. When you concatenate
strings like this:

parser.add_argument('--asyncio-debug', action='store_true',
                    help='Enable debug mode for asyncio loop'
                    'Generates lot of output, makes TUI unusable when'
                    'logs are logged in the TUI itself.'
                    'Use only when logging to a file')
Python is going to do this:

help='Enable debug mode for asyncio loop' + 'Generates lot of output, makes
TUI unusable when' + ...

so you'll get a string like this:

help='Enable debug mode for asyncio loopGenerates lot of output, makes TUI
unusable when' + ...

[-- Attachment #2: Type: text/html, Size: 8613 bytes --]

Re: [PATCH v3 08/13] python/aqmp-tui: add syntax highlighting

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 7548 bytes --]

On Mon, Aug 16, 2021 at 5:20 PM Niteesh G. S. <niteesh.gs@gmail.com> wrote:

>
>
> On Tue, Aug 17, 2021 at 1:14 AM John Snow <jsnow@redhat.com> wrote:
>
>>
>>
>> On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
>> wrote:
>>
>>> 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 | 52 +++++++++++++++++++++++++++---------
>>>  1 file changed, 40 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
>>> index ab9ada793a..0d5ec62cb7 100644
>>> --- a/python/qemu/aqmp/aqmp_tui.py
>>> +++ b/python/qemu/aqmp/aqmp_tui.py
>>> @@ -19,6 +19,8 @@
>>>      Union,
>>>  )
>>>
>>> +from pygments import lexers
>>> +from pygments import token as Token
>>>  import urwid
>>>  import urwid_readline
>>>
>>> @@ -35,6 +37,22 @@
>>>  LOGGER = logging.getLogger()
>>>
>>>
>>> +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'),
>>> +    ('DEBUG', '', '', '', '#ddf', 'g7'),
>>> +    ('INFO', '', '', '', 'g100', 'g7'),
>>> +    ('WARNING', '', '', '', '#ff6', 'g7'),
>>> +    ('ERROR', '', '', '', '#a00', 'g7'),
>>> +    ('CRITICAL', '', '', '', '#a00', 'g7'),
>>> +    ('background', '', 'black', '', '', 'g7'),
>>> +]
>>> +
>>> +
>>>  def format_json(msg: str) -> str:
>>>      """
>>>      Formats given multiline JSON message into a single line message.
>>> @@ -57,17 +75,14 @@ def __init__(self, address: Union[str, Tuple[str,
>>> int]]) -> None:
>>>          self.aloop: Optional[Any] = None  # FIXME: Use more concrete
>>> type.
>>>          super().__init__()
>>>
>>> -    def add_to_history(self, msg: str) -> None:
>>> -        urwid.emit_signal(self, UPDATE_MSG, msg)
>>> +    def add_to_history(self, msg: str, level: Optional[str] = None) ->
>>> None:
>>> +        urwid.emit_signal(self, UPDATE_MSG, msg, level)
>>>
>>>      def _cb_outbound(self, msg: Message) -> Message:
>>>          # FIXME: I think the ideal way to omit these messages during
>>> in-TUI
>>> -        # logging will be to add a filter to the logger. We can use
>>> regex to
>>> -        # filter out messages starting with 'Request:' or 'Response:'
>>> but I
>>> -        # think a better approach will be encapsulate the message in an
>>> object
>>> -        # and filter based on the object. Encapsulation of the message
>>> will
>>> -        # also be necessary when we want different formatting of
>>> messages
>>> -        # inside TUI.
>>> +        # logging will be to add a filter to the logger. We can use
>>> +        # regex/startswith to filter out messages starting with
>>> 'Request:' or
>>> +        # 'Response:'. If possible please suggest other ideas.
>>>
>>
>> We'll want to fix this up earlier in the series -- the finished version
>> of your series shouldn't have FIXME comments, and later patches in the
>> series shouldn't edit comments added earlier. We chatted on IRC about the
>> right strategy for filtering log messages, so I think V4 should have a
>> strategy in place to avoid this comment, right?
>>
> Yes, this has been addressed in the v4.
> We avoid logging messages when we log the messages to the TUI screen. This
> is done by checking if TUILogHandler is installed on the logger or not.
>
>>
>>
>>>          handler = LOGGER.handlers[0]
>>>          if not isinstance(handler, TUILogHandler):
>>>              LOGGER.debug('Request: %s', str(msg))
>>> @@ -156,6 +171,9 @@ def _get_formatted_address(self) -> str:
>>>              self._set_status('Server shutdown')
>>>
>>>      def run(self, debug: bool = False) -> None:
>>> +        screen = urwid.raw_display.Screen()
>>> +        screen.set_terminal_properties(256)
>>>
>>
>> I'm not sure if this will work everywhere, but ... it's fine for now, I
>> assume.
>>
> I'll add a note here.
>
>> +
>>>          self.aloop = asyncio.get_event_loop()
>>>          self.aloop.set_debug(debug)
>>>
>>> @@ -167,6 +185,8 @@ def run(self, debug: bool = False) -> None:
>>>          event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
>>>          main_loop = urwid.MainLoop(urwid.AttrMap(self.window,
>>> 'background'),
>>>                                     unhandled_input=self.unhandled_input,
>>> +                                   screen=screen,
>>> +                                   palette=palette,
>>>                                     handle_mouse=True,
>>>                                     event_loop=event_loop)
>>>
>>> @@ -251,7 +271,8 @@ def __init__(self, master: App) -> None:
>>>          self.history = urwid.SimpleFocusListWalker([])
>>>          super().__init__(self.history)
>>>
>>> -    def add_to_history(self, history: str) -> None:
>>> +    def add_to_history(self,
>>> +                       history: Union[str, List[Tuple[str, str]]]) ->
>>> None:
>>>          self.history.append(urwid.Text(history))
>>>          if self.history:
>>>              self.history.set_focus(len(self.history) - 1)
>>> @@ -271,8 +292,15 @@ def __init__(self, master: App) -> None:
>>>          super().__init__(self.body)
>>>          urwid.connect_signal(self.master, UPDATE_MSG,
>>> self.cb_add_to_history)
>>>
>>> -    def cb_add_to_history(self, msg: str) -> None:
>>> -        self.history.add_to_history(msg)
>>> +    def cb_add_to_history(self, msg: str, level: Optional[str] = None)
>>> -> None:
>>> +        formatted = []
>>> +        if level:
>>> +            formatted.append((level, msg))
>>> +        else:
>>> +            lexer = lexers.JsonLexer()  # pylint: disable=no-member
>>> +            for token in lexer.get_tokens(msg):
>>> +                formatted.append(token)
>>> +        self.history.add_to_history(formatted)
>>>
>>
>> This looks like it's conflating having a debugging level with receiving a
>> message that's already formatted.
>>
>> i.e., the code here assumes that if it receives a level, it is ALSO
>> already formatted. That smells like trouble -- but I think there are some
>> changes to the logging and rendering you had planned for V4 anyway, I
>> believe?
>>
> Why do you think it is trouble?
> When a level is provided, it is most probably going to be text messages or
> QMP messages with text. In either case, there is no formatting to be done.
> In the second case, we don't have a JSON formattor that can format
> a JSON message along with text without breaking. So I thought it is better
> to leave the formatting to the user and just use the level to provide
> syntax highlighting.
>
>
Because it's an indirect type inference -- you are *indirectly* measuring
whether or not a message has been formatted yet. Using the presence or
absence of 'level' is an indirect way of proving the condition; are there
not more direct ways?

Talking on IRC, I see that you have plans for an abstracted Item view that
might make this a null point, though -- so I'll drop this line of
interrogation for now.

Thanks!

[-- Attachment #2: Type: text/html, Size: 10852 bytes --]

Re: [PATCH v3 09/13] python/aqmp-tui: Add QMP connection manager

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 5367 bytes --]

On Tue, Aug 17, 2021 at 3:07 PM Niteesh G. S. <niteesh.gs@gmail.com> wrote:

>
>
> On Tue, Aug 17, 2021 at 10:21 AM John Snow <jsnow@redhat.com> wrote:
>
>>
>>
>> On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
>> wrote:
>>
>>
[...]


>
>>
>>> +
>>> +
>>>  class App(QMPClient):
>>> -    def __init__(self, address: Union[str, Tuple[str, int]]) -> None:
>>> +    def __init__(self, address: Union[str, Tuple[str, int]],
>>> num_retries: int,
>>> +                 retry_delay: Optional[int]) -> None:
>>>          urwid.register_signal(type(self), UPDATE_MSG)
>>>          self.window = Window(self)
>>>          self.address = address
>>>          self.aloop: Optional[Any] = None  # FIXME: Use more concrete
>>> type.
>>> +        self.num_retries = num_retries
>>> +        self.retry_delay = retry_delay
>>> +        self.retry: bool = False
>>> +        self.disconnecting: bool = False
>>>
>>
>> Why is this one needed again ? ...
>>
>

> A race condition occurs in protocol.py line 597
> The reason behind this is there are two disconnect calls initiated. The
> first one via kill_app
> and the second one via manage_connection when the state is set to
> disconnecting by the first call.
> One of the calls set's the state to IDLE(protocol.py:584) after it has
> finished disconnecting, meanwhile
> the second call is somehow in the process of disconnecting and assert the
> state to be in DISCONNECTING
> in protocol.py:597, which it is not since it has been set to IDLE by the
> first call.
>
> If I don't gaurd against the second call I get the following exception
>
> ------------------------------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/home/niteesh/development/qemu/python/.venv/bin/aqmp-tui", line
> 33, in <module>
>     sys.exit(load_entry_point('qemu', 'console_scripts', 'aqmp-tui')())
>   File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line
> 695, in main
>     app.run(args.asyncio_debug)
>   File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line
> 444, in run
>     raise err
>   File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line
> 441, in run
>     main_loop.run()
>   File
> "/home/niteesh/development/qemu/python/.venv/lib/python3.6/site-packages/urwid/main_loop.py",
> line 287, in run
>     self._run()
>   File
> "/home/niteesh/development/qemu/python/.venv/lib/python3.6/site-packages/urwid/main_loop.py",
> line 385, in _run
>     self.event_loop.run()
>   File
> "/home/niteesh/development/qemu/python/.venv/lib/python3.6/site-packages/urwid/main_loop.py",
> line 1494, in run
>     reraise(*exc_info)
>   File
> "/home/niteesh/development/qemu/python/.venv/lib/python3.6/site-packages/urwid/compat.py",
> line 58, in reraise
>     raise value
>   File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line
> 391, in manage_connection
>     await self.disconnect()
>   File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line
> 312, in disconnect
>     raise err
>   File "/home/niteesh/development/qemu/python/qemu/aqmp/aqmp_tui.py", line
> 300, in disconnect
>     await super().disconnect()
>   File "/home/niteesh/development/qemu/python/qemu/aqmp/protocol.py", line
> 302, in disconnect
>     await self._wait_disconnect()
>   File "/home/niteesh/development/qemu/python/qemu/aqmp/protocol.py", line
> 583, in _wait_disconnect
>     self._cleanup()
>   File "/home/niteesh/development/qemu/python/qemu/aqmp/qmp_client.py",
> line 331, in _cleanup
>     super()._cleanup()
>   File "/home/niteesh/development/qemu/python/qemu/aqmp/protocol.py", line
> 597, in _cleanup
>     assert self.runstate == Runstate.DISCONNECTING
> AssertionError
>
> -------------------------------------------------------------------------------------------
>

Hm, OK. I'm not sure if this is a bug on my part or not yet, I'll
investigate.


>      def add_to_history(self, msg: str, level: Optional[str] = None) ->
>>> None:
>>> @@ -119,7 +132,7 @@ def _cb_inbound(self, msg: Message) -> Message:
>>>              LOGGER.info('Error server disconnected before reply')
>>>              urwid.emit_signal(self, UPDATE_MSG,
>>>                                '{"error": "Server disconnected before
>>> reply"}')
>>> -            self._set_status("Server disconnected")
>>> +            await self.disconnect()
>>>          except Exception as err:
>>>              LOGGER.error('Exception from _send_to_server: %s', str(err))
>>>              raise err
>>> @@ -136,15 +149,29 @@ def kill_app(self) -> None:
>>>          create_task(self._kill_app())
>>
>> Is this required? I would have hoped that after calling disconnect that
>> the state would have again changed to IDLE and you wouldn't need this
>> clause here.
>>
> After you mentioned it I too felt it was redundant. But on removing it the
> whole app freezes when trying to exit.
> I logged the state after the call to disconnect, instead of being in the
> IDLE state, it is still in DISCONNECTING state.
> I suspect this results in the constant infinite looping which doesn't give
> other coroutines a chance to run and blocks
> the event loop thus resulting in the freezing of the app. But I am not
> sure why the state isn't changing to IDLE.
>

Hmm ... That may well be a bug in AQMP then. I will investigate.

[-- Attachment #2: Type: text/html, Size: 9074 bytes --]

Re: [PATCH v3 04/13] python/aqmp-tui: Add AQMP TUI draft

[Niteesh G. S.]

[-- Attachment #1: Type: text/plain, Size: 5047 bytes --]

On Wed, Aug 18, 2021 at 11:52 PM John Snow <jsnow@redhat.com> wrote:

>
>
> On Fri, Aug 13, 2021 at 11:11 AM Niteesh G. S. <niteesh.gs@gmail.com>
> wrote:
>
>>
>> On Fri, Aug 6, 2021 at 12:28 AM John Snow <jsnow@redhat.com> wrote:
>>
>>>
>>> On Fri, Jul 30, 2021 at 4:19 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>
>>>>
>>>
> ...
>
>
>> +
>>>> +# Using root logger to enable all loggers under qemu and asyncio
>>>> +LOGGER = logging.getLogger()
>>>>
>>>
>>> The comment isn't quite true; this is the root logger -- but you go on
>>> to use it to directly log messages. I don't think you should; use a
>>> module-level logger.
>>>
>>> (1) Create a module-level logger that is named after the current module
>>> name (e.g. qemu.aqmp.aqmp_tui) and use that for logging messages relating
>>> to this module:
>>> LOGGER = logging.getLogger(__name__)
>>>
>>> (2) Where you need to address the root logger, just use `root_logger =
>>> logging.getLogger() ` .... I think the main() function is the only place
>>> you might need this.
>>>
>> The way of logging in the TUI is changed such that, all logging is done
>> through the root level logger. The handlers and levels are installed to
>> the root level
>> logger to allow logging from other libraries to be rerouted to the screen
>> or file.
>>
>
> We talked about this on IRC, so I'll assume our disagreement in vision
> here is cleared up ... or at least different :)
>
Maybe we'll come back to this on v4 :)

>
>
>> +
>>>> +
>>>> +def format_json(msg):
>>>> +    """
>>>> +    Formats given multiline JSON message into a single line message.
>>>> +    Converting into single line is more asthetically pleasing when
>>>> looking
>>>> +    along with error messages compared to multiline JSON.
>>>> +    """
>>>> +    # FIXME: Use better formatting mechanism. Might break at more
>>>> complex JSON
>>>> +    # data.
>>>> +    msg = msg.replace('\n', '')
>>>> +    words = msg.split(' ')
>>>> +    words = [word for word in words if word != '']
>>>> +    return ' '.join(words)
>>>> +
>>>>
>>>
>>> You can use the JSON module to do this,
>>> https://docs.python.org/3/library/json.html#json.dumps
>>>
>>> Message._serialize uses this technique to send JSON messages over the
>>> wire that have no newlines:
>>>
>>> https://gitlab.com/jsnow/qemu/-/blob/python-async-qmp-aqmp/python/qemu/aqmp/message.py#L132
>>>
>>> by not specifying an indent and including separators with no spaces, we
>>> can get a JSON representation with all the space squeezed out. You can add
>>> spaces and still omit the indent/newlines and so on.
>>>
>>
>
>> But will this work for invalid JSON messages? As far as I have tried it
>> doesn't work.
>>
>
> Ah, I see ... Nope, that requires a valid JSON message ... Do we *need* to
> pretty-print invalid JSON?
>
IMHO Yes it looks pretty.

[1, true, 3]: QMP message is not a JSON object.
This one looks much better than the below one
[ 1,
       true,
3 ]: QMP message is not a JSON object.


>
>> +
>>>> +def main():
>>>> +    parser = argparse.ArgumentParser(description='AQMP TUI')
>>>> +    parser.add_argument('qmp_server', help='Address of the QMP server'
>>>> +                        '< UNIX socket path | TCP addr:port >')
>>>> +    parser.add_argument('--log-file', help='The Log file name')
>>>> +    parser.add_argument('--log-level', default='WARNING',
>>>> +                        help='Log level
>>>> <CRITICAL|ERROR|WARNING|INFO|DEBUG|>')
>>>> +    parser.add_argument('--asyncio-debug', action='store_true',
>>>> +                        help='Enable debug mode for asyncio loop'
>>>> +                        'Generates lot of output, makes TUI unusable
>>>> when'
>>>> +                        'logs are logged in the TUI itself.'
>>>> +                        'Use only when logging to a file')
>>>>
>>>
>>> Needs spaces between the lines here so that the output reads correctly.
>>>
>> The output renders properly for me. Can you please post a pic if it
>> doesn't render correctly for you?
>>
>
> No screenshot needed, just try running with '--help'. When you concatenate
> strings like this:
>
> parser.add_argument('--asyncio-debug', action='store_true',
>                     help='Enable debug mode for asyncio loop'
>                     'Generates lot of output, makes TUI unusable when'
>                     'logs are logged in the TUI itself.'
>                     'Use only when logging to a file')
> Python is going to do this:
>
> help='Enable debug mode for asyncio loop' + 'Generates lot of output,
> makes TUI unusable when' + ...
>
> so you'll get a string like this:
>
> help='Enable debug mode for asyncio loopGenerates lot of output, makes TUI
> unusable when' + ...
>
Thanks. Fixed.

[-- Attachment #2: Type: text/html, Size: 10158 bytes --]

Re: [PATCH v3 09/13] python/aqmp-tui: Add QMP connection manager

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

On Wed, Aug 18, 2021 at 3:36 PM John Snow <jsnow@redhat.com> wrote:

> On Tue, Aug 17, 2021 at 3:07 PM Niteesh G. S. <niteesh.gs@gmail.com>
> wrote:
>
>> On Tue, Aug 17, 2021 at 10:21 AM John Snow <jsnow@redhat.com> wrote:
>>
>>> On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu <niteesh.gs@gmail.com>
>>> wrote:
>>>
>>
> Is this required? I would have hoped that after calling disconnect that
>>> the state would have again changed to IDLE and you wouldn't need this
>>> clause here.
>>>
>> After you mentioned it I too felt it was redundant. But on removing it
>> the whole app freezes when trying to exit.
>> I logged the state after the call to disconnect, instead of being in the
>> IDLE state, it is still in DISCONNECTING state.
>> I suspect this results in the constant infinite looping which doesn't
>> give other coroutines a chance to run and blocks
>> the event loop thus resulting in the freezing of the app. But I am not
>> sure why the state isn't changing to IDLE.
>>
>
> Hmm ... That may well be a bug in AQMP then. I will investigate.
>

No, it's not -- It's just tricky. The "problem" is that the
runstate_changed() event only returns once the runstate has changed *after*
you await it. So this code is perfectly correct and I am just bad at
reading.

--js

[-- Attachment #2: Type: text/html, Size: 2922 bytes --]