From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6CD34C4338F for ; Thu, 5 Aug 2021 19:00:38 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 932E260F01 for ; Thu, 5 Aug 2021 19:00:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 932E260F01 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:39526 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mBibc-0007MJ-7j for qemu-devel@archiver.kernel.org; Thu, 05 Aug 2021 15:00:36 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:60146) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mBiZw-0006Id-9J for qemu-devel@nongnu.org; Thu, 05 Aug 2021 14:58:52 -0400 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:53236) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mBiZp-0001fr-Ke for qemu-devel@nongnu.org; Thu, 05 Aug 2021 14:58:50 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1628189923; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=qM6EViIJY5gcCE13U7zZ9Li9sahDKHLaAvVOomOkMuo=; b=VgHUmelpYiA9+Q3q8naF4g579wRSQaPyw5E/7zWwAALH7aizuAVs9Sbn1gZ/kOnOFqXtLm vvuevuB7uY+c9Yun6ioOQlhpqP1SUnoQZYmKJFjxWR7+cH9fCoQAIy0Z77XTaUlZ63jMs3 8BJ10IjFPQ4j9HEFOushd0N3urj9kCc= Received: from mail-oo1-f71.google.com (mail-oo1-f71.google.com [209.85.161.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-425-RBPbfnfnMFyoy_609Gb0iw-1; Thu, 05 Aug 2021 14:58:39 -0400 X-MC-Unique: RBPbfnfnMFyoy_609Gb0iw-1 Received: by mail-oo1-f71.google.com with SMTP id y10-20020a4ab40a0000b0290285068c6fc0so1957421oon.1 for ; Thu, 05 Aug 2021 11:58:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qM6EViIJY5gcCE13U7zZ9Li9sahDKHLaAvVOomOkMuo=; b=ZO60VI8dyGoZFRb/iJYOuKe/Mr8Wjvws3vKtr0x+VfhRea6LyciRuW6BhP7dFP28b2 cl9om7kdLJ8uqWUAOp7lQ/vlw94B3UH5b4k1D028hzlZR20ezZSZtVcGZQTjtUTZQNmd a7c7qvKfqL3jKF9QVru3TwHZ6ITrlPWI5RpjlKCk/KXjn86Z0HAqL9HiT5NiEwcNBNDq SfqnFFIhHCIsqf7cCZSWN55eSiCEQu2sZJcTSTyJGJDy6VtrN0k9RTcKDRrgTGRDdwAh M0N2hOciUt4PNLbCFbZeJGwipUqkNxr2pHFVWXiR+mx2OoN2yv4kc6VmYMXd+LRlzvZH udsQ== X-Gm-Message-State: AOAM530NrD9gAIlvR/mlGMXshtM/wyb5ukeGgHZy02Y13KqVNkJAsM2g PpVPk9fDn++KELr0P6p0lFYZ2qxdz4+8QUEbSG75NsKyEWnCEKq7D1WBB1L26eMrFdsR3aKoGRi i28JG5NzHBVJL/RQiq06ld+6EId7ysUo= X-Received: by 2002:a05:6808:695:: with SMTP id k21mr1866508oig.52.1628189919221; Thu, 05 Aug 2021 11:58:39 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz0/ytUHhSSNpqUdN8lmkk0JeGgBooZKX1NET+i8RGeADFpfYwZIwCr7lw60pfzJxSFpVW5T8AVhyxeklsRDT4= X-Received: by 2002:a05:6808:695:: with SMTP id k21mr1866487oig.52.1628189918982; Thu, 05 Aug 2021 11:58:38 -0700 (PDT) MIME-Version: 1.0 References: <20210730201846.5147-1-niteesh.gs@gmail.com> <20210730201846.5147-5-niteesh.gs@gmail.com> In-Reply-To: <20210730201846.5147-5-niteesh.gs@gmail.com> From: John Snow Date: Thu, 5 Aug 2021 14:58:28 -0400 Message-ID: Subject: Re: [PATCH v3 04/13] python/aqmp-tui: Add AQMP TUI draft To: G S Niteesh Babu Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=jsnow@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="000000000000bf8b4505c8d48108" Received-SPF: pass client-ip=216.205.24.124; envelope-from=jsnow@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -15 X-Spam_score: -1.6 X-Spam_bar: - X-Spam_report: (-1.6 / 5.0 requ) DKIMWL_WL_HIGH=-0.699, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Eduardo Habkost , Kashyap Chamarthy , Markus Armbruster , Wainer Moschetta , qemu-devel , Stefan Hajnoczi , Cleber Rosa , Eric Blake Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --000000000000bf8b4505c8d48108 Content-Type: text/plain; charset="UTF-8" On Fri, Jul 30, 2021 at 4:19 PM G S Niteesh Babu 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 > --- > 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 > +# > +# 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 > ') > + 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. --000000000000bf8b4505c8d48108 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Fri, Jul 30, 2021 at 4:19 PM G S N= iteesh Babu <niteesh.gs@gmail.co= m> 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>
---
=C2=A0python/qemu/aqmp/aqmp_tui.py | 333 ++++++++++++++++++++++++++++++++++= +
=C2=A0python/setup.cfg=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2= =A0 16 +-
=C2=A02 files changed, 348 insertions(+), 1 deletion(-)
=C2=A0create 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:
+#=C2=A0 Niteesh Babu G S <niteesh.gs@gmail.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or
+# later.=C2=A0 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 =3D 'UPDATE_MSG'

Th= is line still feels kind of "naked" on a cold read. It could use = a small comment.
=C2=A0
+
+# Using root logger to enable all loggers under qemu and asyncio
+LOGGER =3D logging.getLogger()

The com= ment 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-lev= el logger.

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

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

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

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

<= div>Message._serialize uses this technique to send JSON messages over the w= ire that have no newlines:

by not specifying an indent and includ= ing separators with no spaces, we can get a JSON representation with all th= e space squeezed out. You can add spaces and still omit the indent/newlines= and so on.
=C2=A0
+
+class App(QMPClient):
+=C2=A0 =C2=A0 def __init__(self, address):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 urwid.register_signal(type(self), UPDATE_MSG)<= br>

I found some hidden documentation about= this -- I was struggling to find it before:

S= o the first argument is meant to be "The class of an object sending th= e 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 th= e __init__ method here will explode ... Well, it's probably not too imp= ortant right now.
=C2=A0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.window =3D Window(self)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.address =3D address
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.aloop =3D None
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 super().__init__()
+
+=C2=A0 =C2=A0 def add_to_history(self, msg):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 urwid.emit_signal(self, UPDATE_MSG, msg)
+
+=C2=A0 =C2=A0 def _cb_outbound(self, msg):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # FIXME: I think the ideal way to omit these m= essages during in-TUI
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # logging will be to add a filter to the logge= r. We can use regex to
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # filter out messages starting with 'Reque= st:' or 'Response:' but I
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # think a better approach will be encapsulate = the message in an object
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # and filter based on the object. Encapsulatio= n of the message will
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # also be necessary when we want different for= matting of messages
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # inside TUI.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 handler =3D LOGGER.handlers[0]
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if not isinstance(handler, TUILogHandler):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGGER.debug('Request: %s= 9;, str(msg))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.add_to_history('<-- ' + str(ms= g))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 return msg
+
+=C2=A0 =C2=A0 def _cb_inbound(self, msg):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 handler =3D LOGGER.handlers[0]
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if not isinstance(handler, TUILogHandler):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGGER.debug('Response: %s&#= 39;, str(msg))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.add_to_history('--> ' + str(ms= g))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 return msg
+
+=C2=A0 =C2=A0 async def wait_for_events(self):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 async for event in self.events:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.handle_event(event)
+

Can handle_event be moved here so tha= t related functions are somewhat near each other?
=C2=A0
+=C2=A0 =C2=A0 async def _send_to_server(self, raw_msg):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # FIXME: Format the raw_msg in history view to= one line. It is not
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # pleasing to see multiple lines JSON object w= ith an error statement.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 try:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 msg =3D Message(bytes(raw_msg, e= ncoding=3D'utf-8'))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # Format multiline json into a s= ingle line JSON, since it is more
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # pleasing to look along with er= r message in TUI.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raw_msg =3D self.format_json(raw= _msg)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 await self._raw(msg, assign_id= =3D'id' not in msg)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 except (ValueError, TypeError) as err:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGGER.info('Invalid message= : %s', str(err))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.add_to_history(f'{raw_m= sg}: {err}')
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 except (DeserializationError, UnexpectedTypeEr= ror) as err:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGGER.info('Invalid message= : %s', err.error_message)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.add_to_history(f'{raw_m= sg}: {err.error_message}')
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 except ExecInterruptedError:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGGER.info('Error server di= sconnected before reply')
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 urwid.emit_signal(self, UPDATE_M= SG,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 '{"error": "Server disco= nnected before reply"}')
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._set_status("Server di= sconnected")
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 except Exception as err:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGGER.error('Exception from= _send_to_server: %s', str(err))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise err
+
+=C2=A0 =C2=A0 def cb_send_to_server(self, msg):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 create_task(self._send_to_server(msg))
+
+=C2=A0 =C2=A0 def unhandled_input(self, key):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if key =3D=3D 'esc':
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.kill_app()
+
+=C2=A0 =C2=A0 def kill_app(self):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # TODO: Work on the disconnect logic
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 create_task(self._kill_app())
+

That logic needs to be here -- each c= ommit needs to make sense by itself. If you've improved the connection = logic, it needs to be merged into this commit.
=C2=A0
+=C2=A0 =C2=A0 async def _kill_app(self):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # It is ok to call disconnect even in disconne= ct state
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 try:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 await self.disconnect()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGGER.debug('Disconnect fin= ished. Exiting app')
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 except Exception as err:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGGER.info('_kill_app: %s&#= 39;, str(err))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # Let the app crash after provid= ing a proper stack trace
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise err
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 raise urwid.ExitMainLoop()
+
+=C2=A0 =C2=A0 def handle_event(self, event):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # FIXME: Consider all states present in qapi/r= un-state.json
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if event['event'] =3D=3D 'SHUTDOWN= ':
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._set_status('Server shu= tdown')
+

This is an odd function to me because= it will *overwrite* the status bar with this event, but surely shortly the= reafter we're going to see a disconnection event from the transport its= elf, so this message will go away almost immediately, won't it? I under= stand that connection management stuff is improved in later patches, but th= e initial patch here would be better served not having a partial and incorr= ect implementation. You can just remove features that you intend to "b= uild out" in later commits. And if we remove this, then we don't n= eed wait_for_events() either, and they can be re-added in a later commit wh= en they're given a more full, proper treatment.
=C2=A0
+=C2=A0 =C2=A0 def _set_status(self, msg: str) -> None:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.window.footer.set_text(msg)
+
+=C2=A0 =C2=A0 def _get_formatted_address(self) -> str:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 addr =3D f'{self.address}'
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if isinstance(self.address, tuple):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 host, port =3D self.address
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 addr =3D f'{host}:{port}'= ;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 return addr

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

+
+=C2=A0 =C2=A0 async def connect_server(self):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 try:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 await self.connect(self.address)=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 addr =3D self._get_formatted_add= ress()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._set_status(f'Connected= to {addr}')
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 except ConnectError as err:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGGER.info('connect_server:= ConnectError %s', str(err))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._set_status('Server shu= tdown')

A failure to connect doesn&= #39;t necessary mean the server was shut down.
=C2=A0
+

I suspect this is changed in later co= mmits in this series, but you should probably set the footer to something b= efore we potentially hang on connect() and do a whole lot of nothing. LIke = other critiques about unfinished interfaces, once you've implemented th= em, they should be worked into this patch so that each commit makes sense b= y 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.
=C2=A0
+=C2=A0 =C2=A0 def run(self, debug=3DFalse):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.aloop =3D asyncio.get_event_loop()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.aloop.set_debug(debug)
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # Gracefully handle SIGTERM and SIGINT signals=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 cancel_signals =3D [signal.SIGTERM, signal.SIG= INT]
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 for sig in cancel_signals:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.aloop.add_signal_handler(si= g, self.kill_app)
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 event_loop =3D urwid.AsyncioEventLoop(loop=3Ds= elf.aloop)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 main_loop =3D urwid.MainLoop(urwid.AttrMap(sel= f.window, 'background'),
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unhandled_input=3Dself.= unhandled_input,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0handle_mouse=3DTrue, +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0event_loop=3Devent_loop= )
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 create_task(self.wait_for_events(), self.aloop= )
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 create_task(self.connect_server(), self.aloop)=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 try:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 main_loop.run()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 except Exception as err:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGGER.error('%s\n%s\n',= str(err), pretty_traceback())
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise err
+
+
+class StatusBar(urwid.Text):
+=C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 A simple Text widget that currently only shows connection st= atus.
+=C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 def __init__(self, text=3D''):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 super().__init__(text, align=3D'right'= )
+
+
+class Editor(urwid_readline.ReadlineEdit):
+=C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 Support urwid_readline features along with
+=C2=A0 =C2=A0 history support which lacks in urwid_readline
+=C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 def __init__(self, master):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 super().__init__(caption=3D'> ', mu= ltiline=3DTrue)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.master =3D master
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.history =3D []
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.last_index =3D -1
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.show_history =3D False
+
+=C2=A0 =C2=A0 def keypress(self, size, key):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # TODO: Add some logic for down key and clean = up logic if possible.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # Returning None means the key has been handle= d by this widget
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # which otherwise is propogated to the parent = widget to be
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # handled
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 msg =3D self.get_edit_text()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if key =3D=3D 'up' and not msg:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # Show the history when 'up = arrow' is pressed with no input text.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # NOTE: The show_history logic i= s necessary because in 'multiline'
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # mode (which we use) 'up ar= row' is used to move between lines.
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.show_history =3D True
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 last_msg =3D self.history[self.l= ast_index] if self.history else ''
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.set_edit_text(last_msg)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.edit_pos =3D len(last_msg)<= br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.last_index +=3D 1
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 elif key =3D=3D 'up' and self.show_his= tory:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if self.last_index < len(self= .history):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.set_edit_text= (self.history[self.last_index])
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.edit_pos =3D = len(self.history[self.last_index])
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.last_index += =3D 1
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 elif key =3D=3D 'meta enter':
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # When using multiline, enter in= serts a new line into the editor
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # send the input to the server o= n alt + enter
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.master.cb_send_to_server(ms= g)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.history.insert(0, msg)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.set_edit_text('') +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.last_index =3D 0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.show_history =3D False
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 else:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.show_history =3D False
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.last_index =3D 0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return super().keypress(size, ke= y)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 return None
+
+
+class EditorWidget(urwid.Filler):
+=C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 Wraps CustomEdit
+=C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 def __init__(self, master):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 super().__init__(Editor(master), valign=3D'= ;top')
+
+
+class HistoryBox(urwid.ListBox):
+=C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 Shows all the QMP message transmitted/received
+=C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 def __init__(self, master):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.master =3D master
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.history =3D urwid.SimpleFocusListWalker([= ])
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 super().__init__(self.history)
+
+=C2=A0 =C2=A0 def add_to_history(self, history):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.history.append(urwid.Text(history))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if self.history:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.history.set_focus(len(self.= history) - 1)
+
+
+class HistoryWindow(urwid.Frame):
+=C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 Composes the HistoryBox and EditorWidget
+=C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 def __init__(self, master):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.master =3D master
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.editor_widget =3D EditorWidget(master) +=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.editor =3D urwid.LineBox(self.editor_widg= et)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.history =3D HistoryBox(master)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.body =3D urwid.Pile([('weight', 8= 0, self.history),
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ('weight', 20, self.editor)]= )
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 super().__init__(self.body)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 urwid.connect_signal(self.master, UPDATE_MSG, = self.cb_add_to_history)
+
+=C2=A0 =C2=A0 def cb_add_to_history(self, msg):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.history.add_to_history(msg)
+
+
+class Window(urwid.Frame):
+=C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 This is going to be the main window that is going to compose= other
+=C2=A0 =C2=A0 windows. In this stage it is unnecesssary but will be necess= ary in
+=C2=A0 =C2=A0 future when we will have multiple windows and want to the sw= itch between
+=C2=A0 =C2=A0 them and display overlays
+=C2=A0 =C2=A0 """
+=C2=A0 =C2=A0 def __init__(self, master):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.master =3D master
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 footer =3D StatusBar()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 body =3D HistoryWindow(master)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 super().__init__(body, footer=3Dfooter)
+
+
+class TUILogHandler(Handler):
+=C2=A0 =C2=A0 def __init__(self, tui):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 super().__init__()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.tui =3D tui
+
+=C2=A0 =C2=A0 def emit(self, record):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 level =3D record.levelname
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 msg =3D record.getMessage()
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 msg =3D f'[{level}]: {msg}'
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.tui.add_to_history(msg)
+
+
+def main():
+=C2=A0 =C2=A0 parser =3D argparse.ArgumentParser(description=3D'AQMP T= UI')
+=C2=A0 =C2=A0 parser.add_argument('qmp_server', help=3D'Addres= s of the QMP server'
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 '< UNIX socket path | TCP addr:port >')
+=C2=A0 =C2=A0 parser.add_argument('--log-file', help=3D'The Lo= g file name')
+=C2=A0 =C2=A0 parser.add_argument('--log-level', default=3D'WA= RNING',
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 help=3D'Log level <CRITICAL|ERROR|WARNING|INFO|DEBUG|>= ')
+=C2=A0 =C2=A0 parser.add_argument('--asyncio-debug', action=3D'= ;store_true',
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 help=3D'Enable debug mode for asyncio loop'
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 'Generates lot of output, makes TUI unusable when'
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 'logs are logged in the TUI itself.'
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 'Use only when logging to a file')
=
Needs spaces between the lines here so that the output reads= correctly.
=C2=A0
+=C2=A0 =C2=A0 args =3D parser.parse_args()
+
+=C2=A0 =C2=A0 try:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 address =3D QEMUMonitorProtocol.parse_address(= args.qmp_server)
+=C2=A0 =C2=A0 except QMPBadPortError as err:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 parser.error(err)
+
+=C2=A0 =C2=A0 app =3D App(address)
+
+=C2=A0 =C2=A0 if args.log_file:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGGER.addHandler(logging.FileHandler(args.log= _file))
+=C2=A0 =C2=A0 else:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 LOGGER.addHandler(TUILogHandler(app))
+
+=C2=A0 =C2=A0 log_level =3D logging.getLevelName(args.log_level)
+=C2=A0 =C2=A0 # getLevelName returns 'Level {log_level}' when a in= valid level is passed.
+=C2=A0 =C2=A0 if log_level =3D=3D f'Level {args.log_level}':
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 parser.error('Invalid log level')
+=C2=A0 =C2=A0 LOGGER.setLevel(log_level)
+

I think you could do:

<= div>LOGGER.setLevel(logging.getLevelName(args.log_level))

And it'll fail if the level was unknown, but work otherwise. Th= is would avoid a kind of precarious string-comparison check, which I worry = is fragile.
=C2=A0
+=C2=A0 =C2=A0 app.run(args.asyncio_debug)
+
+
+if __name__ =3D=3D '__main__':
+=C2=A0 =C2=A0 main()=C2=A0 # 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 =3D True
=C2=A0# fusepy has no type stubs:
=C2=A0allow_subclassing_any =3D True

+[mypy-qemu.aqmp.aqmp_tui]
+disallow_untyped_defs =3D False
+disallow_incomplete_defs =3D False
+check_untyped_defs =3D False
+# urwid and urwid_readline have no type stubs:
+allow_subclassing_any =3D True
+
+# The following missing import directives are because these libraries do n= ot
+# provide type stubs. Allow them on an as-needed basis for mypy.
=C2=A0[mypy-fuse]
-# fusepy has no type stubs:
+ignore_missing_imports =3D True
+
+[mypy-urwid]
+ignore_missing_imports =3D True
+
+[mypy-urwid_readline]
=C2=A0ignore_missing_imports =3D True

=C2=A0[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 wa= iting.
--000000000000bf8b4505c8d48108--