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=-10.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 07BF0C07E95 for ; Thu, 8 Jul 2021 03:21:49 +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 4E23A61CD6 for ; Thu, 8 Jul 2021 03:21:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4E23A61CD6 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:38322 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1m1Kbj-0007kl-7U for qemu-devel@archiver.kernel.org; Wed, 07 Jul 2021 23:21:47 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:59890) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1m1Kah-00073N-R6 for qemu-devel@nongnu.org; Wed, 07 Jul 2021 23:20:43 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:40216) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1m1Kab-0000Do-3C for qemu-devel@nongnu.org; Wed, 07 Jul 2021 23:20:42 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1625714435; 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=ZaknVAgR993c7rYmer5N5OB2p+60TKsP0+sB2XYQFH0=; b=G9bl2HLyDTcpUWOD5/PIqjUpM6VNfulR42f5Bhx3Npg/4RPFNzFcfkOZfyXrPwqGeXejax ihMTtV8bYsvOPTvrH1j1V5uQazywS0QJP/coivMAfCrGm1tA9V7ckbASCfQH0OessDsgfV FrDw0VBP67ZLhta0YvJts9Fc+jSfM+0= Received: from mail-ot1-f71.google.com (mail-ot1-f71.google.com [209.85.210.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-318-o-XHJwaANd2896Yy3A02nA-1; Wed, 07 Jul 2021 23:20:29 -0400 X-MC-Unique: o-XHJwaANd2896Yy3A02nA-1 Received: by mail-ot1-f71.google.com with SMTP id p17-20020a0568301311b0290467775813d3so2983154otq.9 for ; Wed, 07 Jul 2021 20:20:29 -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=ZaknVAgR993c7rYmer5N5OB2p+60TKsP0+sB2XYQFH0=; b=lLo45DKG7ywg23eAqW16qEwJMv1Hd0KTQpay2vn16x3mgLHUZr2RSCAosbMNfkGI0y iKzFmZUOfg54CTSYDA1S54u/gKSrSgPcxiT//TuUPE1p87dLaBZgd+a5vvBwB2GlkWAg Jo/PrmEn6CWqJUSGnvFt8Tj4CU1+ZwNb+6BWxs8aDa1eQ1iAGz4GDnKpNWdnH9Fh5Gpi RpKf3JckVPFpeWPFCt/Kc/AiZJTJl6KFPPsBNlH6zHJIv2QNyX/EHBx5NxEHofL0CweI +ISG2L2+UFOCx8SKnQfwUUwYUBnQm1zzVOPmCbgiGACIGYtmGGdCgk+k/7x/fCl5/KEo Ihtg== X-Gm-Message-State: AOAM531D5jIyS/GCLy9AnRiyQunvzJxg6YwNixwRMZfHtyUH4HpDk2JD jCV4roXyxlHBIrgsoDWk1pHmtwjMvol/qyP9sbm8W5JnnNwnz9R/fuSMm3AMMQuUTfkR+zCFaJl NT4cQ981eY0EFou+Hrs970TO76U0V6Gc= X-Received: by 2002:a05:6808:1309:: with SMTP id y9mr1951299oiv.112.1625714428509; Wed, 07 Jul 2021 20:20:28 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyQgUcvXsd5hdxVSCWAfoweX1T4OhyPWEqZA0r7HDlsbmOxr0xXKrPffOS5f1NUVrylEy6FNDo+8O0PEL4mIEI= X-Received: by 2002:a05:6808:1309:: with SMTP id y9mr1951285oiv.112.1625714428208; Wed, 07 Jul 2021 20:20:28 -0700 (PDT) MIME-Version: 1.0 References: <20210702212603.26465-1-niteesh.gs@gmail.com> <20210702212603.26465-4-niteesh.gs@gmail.com> In-Reply-To: <20210702212603.26465-4-niteesh.gs@gmail.com> From: John Snow Date: Wed, 7 Jul 2021 23:20:17 -0400 Message-ID: Subject: Re: [PATCH 3/6] 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="000000000000fff6f005c69422bf" Received-SPF: pass client-ip=170.10.133.124; envelope-from=jsnow@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -41 X-Spam_score: -4.2 X-Spam_bar: ---- X-Spam_report: (-4.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-1.439, 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_H4=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: Cleber Rosa , Eduardo Habkost , qemu-devel Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --000000000000fff6f005c69422bf Content-Type: text/plain; charset="UTF-8" On Fri, Jul 2, 2021 at 5:26 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 | 246 +++++++++++++++++++++++++++++++++++ > python/setup.cfg | 16 ++- > 2 files changed, 261 insertions(+), 1 deletion(-) > create mode 100644 python/qemu/aqmp/aqmp_tui.py > > diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py > new file mode 100644 > index 0000000000..8e9e8ac8ff > --- /dev/null > +++ b/python/qemu/aqmp/aqmp_tui.py > @@ -0,0 +1,246 @@ > +# Copyright (c) 2021 > +# > +# Authors: > +# Niteesh Babu G S > +# > +# This work is licensed under the terms of the GNU GPL, version 2 or > +# later. See the COPYING file in the top-level directory. > + > +import argparse > +import asyncio > +import logging > +import signal > + > +import urwid > +import urwid_readline > + > +from .protocol import ConnectError > +from .qmp_protocol import QMP, ExecInterruptedError, ExecuteError > +from .util import create_task, pretty_traceback > + > + > +UPDATE_MSG = 'UPDATE_MSG' > + > + > +class StatusBar(urwid.Text): > + """ > + A simple Text widget that currently only shows connection status. > + """ > + def __init__(self, text=''): > + super().__init__(text, align='right') > + > + > +class Editor(urwid_readline.ReadlineEdit): > + """ > + Support urwid_readline features along with > + history support which lacks in urwid_readline > + """ > + def __init__(self, master): > + super().__init__(caption='> ', multiline=True) > + self.master = master > + self.history = [] > + self.last_index = -1 > + self.show_history = False > + > + def keypress(self, size, key): > + # TODO: Add some logic for down key and clean up logic if > possible. > + # Returning None means the key has been handled by this widget > + # which otherwise is propogated to the parent widget to be > + # handled > + msg = self.get_edit_text() > + if key == 'up' and not msg: > + # Show the history when 'up arrow' is pressed with no input > text. > + # NOTE: The show_history logic is necessary because in > 'multiline' > + # mode (which we use) 'up arrow' is used to move between > lines. > + self.show_history = True > + last_msg = self.history[self.last_index] if self.history else > '' > + self.set_edit_text(last_msg) > + self.edit_pos = len(last_msg) > + self.last_index += 1 > + elif key == 'up' and self.show_history: > + if self.last_index < len(self.history): > + self.set_edit_text(self.history[self.last_index]) > + self.edit_pos = len(self.history[self.last_index]) > + self.last_index += 1 > + elif key == 'meta enter': > + # When using multiline, enter inserts a new line into the > editor > + # send the input to the server on alt + enter > + self.master.cb_send_to_server(msg) > + self.history.insert(0, msg) > + self.set_edit_text('') > + self.last_index = 0 > + self.show_history = False > + else: > + self.show_history = False > + self.last_index = 0 > + return super().keypress(size, key) > + return None > + > + > +class EditorWidget(urwid.Filler): > + """ > + Wraps CustomEdit > + """ > + def __init__(self, master): > + super().__init__(Editor(master), valign='top') > + > + > +class HistoryBox(urwid.ListBox): > + """ > + Shows all the QMP message transmitted/received > + """ > + def __init__(self, master): > + self.master = master > + self.history = urwid.SimpleFocusListWalker([]) > + super().__init__(self.history) > + > + def add_to_history(self, history): > + self.history.append(urwid.Text(history)) > + if self.history: > + self.history.set_focus(len(self.history) - 1) > + > + > +class HistoryWindow(urwid.Frame): > + """ > + Composes the HistoryBox and EditorWidget > + """ > + def __init__(self, master): > + self.master = master > + self.editor = EditorWidget(master) > + self.editor_widget = urwid.LineBox(self.editor) > + self.history = HistoryBox(master) > + self.body = urwid.Pile([('weight', 80, self.history), > + ('weight', 10, self.editor_widget)]) > + super().__init__(self.body) > + urwid.connect_signal(self.master, UPDATE_MSG, > self.cb_add_to_history) > + > + def cb_add_to_history(self, msg): > + self.history.add_to_history(msg) > + > + > +class Window(urwid.Frame): > + """ > + This is going to be the main window that is going to compose other > + windows. In this stage it is unnecesssary but will be necessary in > + future when we will have multiple windows and want to the switch > between > + them and display overlays > + """ > + def __init__(self, master): > + self.master = master > + footer = StatusBar() > + body = HistoryWindow(master) > + super().__init__(body, footer=footer) > + > + > +class App(QMP): > + def __init__(self, address): > + urwid.register_signal(self.__class__, UPDATE_MSG) > Do we really need a custom signal? It looks like Urwid has some "default" ones... are they not sufficient? I suppose the idea here is that the 'UPDATE_MSG' signal means that we've updated the history list, so we need to re-render. If not, you may use type(self) here which looks just a little cleaner. > + self.window = Window(self) > + self.address = address > + self.aloop = asyncio.get_event_loop() > I would recommend delaying calling get_event_loop() until run(), just so that all of the loop management stuff is handled in one place. That way, the loop isn't "fixed" until we call run(). > + self.loop = None > + super().__init__() > + > + # Gracefully handle SIGTERM and SIGINT signals > + cancel_signals = [signal.SIGTERM, signal.SIGINT] > + for sig in cancel_signals: > + self.aloop.add_signal_handler(sig, self.kill_app) > If you agree with the above comment, this needs to move into run() as well. > + > + def _cb_outbound(self, msg): > + urwid.emit_signal(self, UPDATE_MSG, "<-- " + str(msg)) > + return msg > + > + def _cb_inbound(self, msg): > + urwid.emit_signal(self, UPDATE_MSG, "--> " + str(msg)) > + return msg > + > + async def wait_for_events(self): > + async for event in self.events: > + self.handle_event(event) > + > + async def _send_to_server(self, msg): > + # TODO: Handle more validation errors (eg: ValueError) > + try: > + response = await self._raw(bytes(msg, 'utf-8')) > + logging.info('Response: %s %s', response, type(response)) > You could log the responses in the inbound hook instead. I'd also use self.logger.debug instead of logging.info(...) so that you re-use the same logger instance. > + except ExecuteError: > + logging.info('Error response from server for msg: %s', msg) > self.logger.info() here. > + except ExecInterruptedError: > + logging.info('Error server disconnected before reply') > And same here. > + # FIXME: Handle this better > What ideas do you have for handling this better? What's wrong with it right now? > + # Show the disconnected message in the history window > + urwid.emit_signal(self, UPDATE_MSG, > + '{"error": "Server disconnected before > reply"}') > + self.window.footer.set_text("Server disconnected") > + except Exception as err: > + logging.info('Exception from _send_to_server: %s', str(err)) > use self.logger.error here, since it's an unhandled error. > + raise err > + > + def cb_send_to_server(self, msg): > + create_task(self._send_to_server(msg)) > + > I wish we didn't have to create tasks for this, but I suppose bridging asyncio and Urwid is just simply not very pretty. One thing to keep in mind is that when you create a task without a handle like this (i.e. you aren't saving the 'task' value anywhere), if that task exits with an Exception, it will cause Python to emit that "Unhandled Exception" warning that you see ... but only once the program otherwise ends. What will end up happening in practice is that the task will die without showing you the Exception. You might want to find a way to make Python crash a little more aggressively when an unhandled exception happens in a task, or otherwise make sure that ERROR level logging messages are visible directly in the TUI history pane, so that we can see te errors when they happen. > + def unhandled_input(self, key): > + if key == 'esc': > + self.kill_app() > + > + def kill_app(self): > + # TODO: Work on the disconnect logic > + create_task(self._kill_app()) > + > Yes, the next thing I'd like to see here is reconnection logic -- I made a little prototype in some code I gave you, but it probably needs to be touched up. I recall that my version would attempt to reconnect infinitely whenever the app was disconnected, regardless of what happened to cause the disconnection. What we likely want is only to reconnect on certain kinds of errors -- ConnectionResetError is likely a good candidate, but other kinds of problems are likely ones we want to STAY disconnected when encountering. We also probably want some logic like num_retries, and retry_delay. > + async def _kill_app(self): > + # It is ok to call disconnect even in disconnect state > + await self.disconnect() > Be aware that this raises Exceptions if the connection terminated ungracefully, i.e. the server hung up before we were expecting it. You might want to handle it (and do something related to connection retry management) first -- there are at least a few erorrs here that wouldn't be too strange to run into. I worry that when you hit 'esc' instead of ctrl^C, you'll see different behavior here -- because ctrl+C creates a task, if this raises an exception here, I think that we won't exit -- we'll get another unhandled exception that won't show up until the app exits. I'm not confident in this, but I think you should confirm that exiting both ways works exactly like you think it does. > + logging.info('disconnect finished, Exiting app') > self.logger.debug > + raise urwid.ExitMainLoop() > + > + def handle_event(self, event): > + if event['event'] == 'SHUTDOWN': > + self.window.footer.set_text('Server shutdown') > + > A bit spartan as an event handler, but it serves its purpose as a demonstration for the proof of concept. It'd be nice to have the footer show a [VM: {state}] status where the state maps 1:1 with qapi/run-state.json's @RunState enumeration. I made a quick hack that you saw, but it wasn't strictly correct. > + async def connect_server(self): > + try: > + await self.connect(self.address) > + self.window.footer.set_text("Connected to {:s}".format( > + f"{self.address[0]}:{self.address[1]}" > + if isinstance(self.address, tuple) > + else self.address > + )) > + except ConnectError as err: > + logging.debug('Cannot connect to server %s', str(err)) > + self.window.footer.set_text('Server shutdown') > Like in other places, I wonder what happens if we have an unhandled exception here, because this is running in a task. > + > + def run(self): > + self.aloop.set_debug(True) > Add a debug argument to run() and default it to False, and add a --debug flag to the argparser that turns this on conditionally instead. > + event_loop = urwid.AsyncioEventLoop(loop=self.aloop) > + self.loop = urwid.MainLoop(self.window, > + unhandled_input=self.unhandled_input, > + handle_mouse=True, > + event_loop=event_loop) > + > + create_task(self.wait_for_events(), self.aloop) > + create_task(self.connect_server(), self.aloop) > + try: > + self.loop.run() > + except Exception as err: > + logging.error('%s\n%s\n', str(err), pretty_traceback()) > + raise err > + > + > +def main(): > + parser = argparse.ArgumentParser(description='AQMP TUI') > + parser.add_argument('-a', '--address', metavar='IP:PORT', > required=True, > + help='Address of the QMP server', dest='address') > + parser.add_argument('--log', help='Address of the QMP server', > + dest='log_file') > + args = parser.parse_args() > + > + logging.basicConfig(filename=args.log_file, level=logging.DEBUG) > + > + address = args.address.split(':') > + address[1] = int(address[1]) > + > + App(tuple(address)).run() > I would take the address as a positional argument instead of with the '--address' flag to mimic how qmp-shell works. > > + > + > +if __name__ == '__main__': > + main() # type: ignore > diff --git a/python/setup.cfg b/python/setup.cfg > index c62803bffc..c6d38451eb 100644 > --- a/python/setup.cfg > +++ b/python/setup.cfg > @@ -81,8 +81,22 @@ namespace_packages = True > # fusepy has no type stubs: > allow_subclassing_any = True > > +[mypy-qemu.aqmp.aqmp_tui] > +disallow_untyped_defs = False > +disallow_incomplete_defs = False > +check_untyped_defs = False > Just keep in mind that we'll need to remove these particular ignores. The rest can stay. > +# urwid and urwid_readline have no type stubs: > +allow_subclassing_any = True > + > +# The following missing import directives are because these libraries do > not > +# provide type stubs. Allow them on an as-needed basis for mypy. > [mypy-fuse] > -# fusepy has no type stubs: > +ignore_missing_imports = True > + > +[mypy-urwid] > +ignore_missing_imports = True > + > +[mypy-urwid_readline] > ignore_missing_imports = True > > [pylint.messages control] > -- > 2.17.1 > > Looking good so far, let's focus on managing the connection state and making sure that Exceptions raised from task contexts are handled properly. I still need to look more deeply into the classes below App, but I wanted to give you your overdue feedback. Thank you for your patience! --js --000000000000fff6f005c69422bf Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Fri, Jul 2, 2021 at 5:26 PM G S Ni= teesh 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>
---
=C2=A0python/qemu/aqmp/aqmp_tui.py | 246 ++++++++++++++++++++++++++++++++++= +
=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, 261 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..8e9e8ac8ff
--- /dev/null
+++ b/python/qemu/aqmp/aqmp_tui.py
@@ -0,0 +1,246 @@
+# 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
+import signal
+
+import urwid
+import urwid_readline
+
+from .protocol import ConnectError
+from .qmp_protocol import QMP, ExecInterruptedError, ExecuteError
+from .util import create_task, pretty_traceback
+
+
+UPDATE_MSG =3D 'UPDATE_MSG'
+
+
+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 =3D EditorWidget(master)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.editor_widget =3D urwid.LineBox(self.edit= or)
+=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', 10, self.editor_w= idget)])
+=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 App(QMP):
+=C2=A0 =C2=A0 def __init__(self, address):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 urwid.register_signal(self.__class__, UPDATE_M= SG)

Do we really need a custom signal? = It looks like Urwid has some "default" ones... are they not suffi= cient? I suppose the idea here is that the 'UPDATE_MSG' signal mean= s that we've updated the history list, so we need to re-render.

If not, you may use type(self) here which looks just = a little cleaner.
=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 asyncio.get_event_loop()

I would recommend delaying calling get_event_= loop() until run(), just so that all of the loop management stuff is handle= d in one place. That way, the loop isn't "fixed" until we cal= l run().
=C2=A0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.loop =3D None
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 super().__init__()
+
+=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)

If you agree with the= above comment, this needs to move into run() as well.
=C2=A0=
+
+=C2=A0 =C2=A0 def _cb_outbound(self, msg):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 urwid.emit_signal(self, UPDATE_MSG, "<= -- " + str(msg))
+=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 urwid.emit_signal(self, UPDATE_MSG, "--&g= t; " + str(msg))
+=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)
+
+=C2=A0 =C2=A0 async def _send_to_server(self, msg):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 # TODO: Handle more validation errors (eg: Val= ueError)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 try:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 response =3D await self._raw(byt= es(msg, 'utf-8'))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 logging.info('Response: %s %s&= #39;, response, type(response))

You cou= ld log the responses in the inbound hook instead.
I'd also us= e self.logger.debug instead of logging.info= (...) so that you re-use the same logger instance.
=C2=A0=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 except ExecuteError:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 logging.info('Error response f= rom server for msg: %s', msg)

= =C2=A0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 except ExecInterruptedError:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 logging.info('Error server dis= connected before reply')

And same h= ere.
=C2=A0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # FIXME: Handle this better
<= /blockquote>

What ideas do you have for handling this be= tter? What's wrong with it right now?
=C2=A0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # Show the disconnected message = in the history window
+=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.window.footer.set_text(&quo= t;Server disconnected")
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 except Exception as err:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 logging.info('Exception from _= send_to_server: %s', str(err))

use = self.logger.error here, since it's an unhandled error.
= =C2=A0
+=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))
+

I wish we didn't have to create t= asks for this, but I suppose bridging asyncio and Urwid is just simply not = very pretty. One thing to keep in mind is that when you create a task witho= ut a handle like this (i.e. you aren't saving the 'task' value = anywhere), if that task exits with an Exception, it will cause Python to em= it that "Unhandled Exception" warning that you see ... but only o= nce the program otherwise ends. What will end up happening in practice is t= hat the task will die without showing you the Exception.

You might want to find a way to make Python crash a little more aggr= essively when an unhandled exception happens in a task, or otherwise make s= ure that ERROR level logging messages are visible directly in the TUI histo= ry pane, so that we can see te errors when they happen.
=C2= =A0
+=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())
+

Yes, the next thing I'd like to s= ee here is reconnection logic -- I made a little prototype in some code I g= ave you, but it probably needs to be touched up. I recall that my version w= ould attempt to reconnect infinitely whenever the app was disconnected, reg= ardless of what happened to cause the disconnection. What we likely want is= only to reconnect on certain kinds of errors -- ConnectionResetError is li= kely a good candidate, but other kinds of problems are likely ones we want = to STAY disconnected when encountering.

We also pr= obably want some logic like num_retries, and retry_delay.
=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 await self.disconnect()
<= br>
Be aware that this raises Exceptions if the connection termin= ated ungracefully, i.e. the server hung up before we were expecting it. You= might want to handle it (and do something related to connection retry mana= gement) first -- there are at least a few erorrs here that wouldn't be = too strange to run into.

I worry that when you hit= 'esc' instead of ctrl^C, you'll see different behavior here --= because ctrl+C creates a task, if this raises an exception here, I think t= hat we won't exit -- we'll get another unhandled exception that won= 't show up until the app exits. I'm not confident in this, but I th= ink you should confirm that exiting both ways works exactly like you think = it does.
=C2=A0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 logging.info('disconnect finished, Exiting a= pp')

self.logger.debug
=C2=A0
+=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 if event['event'] =3D=3D 'SHUTDOWN= ':
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.window.footer.set_text('= ;Server shutdown')
+

A bit spartan as an event handler, b= ut it serves its purpose as a demonstration for the proof of concept.
=

It'd be nice to have the footer show a [VM: {state}= ] status where the state maps 1:1 with qapi/run-state.json's @RunState = enumeration. I made a quick hack that you saw, but it wasn't strictly c= orrect.
=C2=A0
+=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 self.window.footer.set_text(&quo= t;Connected to {:s}".format(
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f"{self.addre= ss[0]}:{self.address[1]}"
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =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 =C2=A0 =C2=A0 else self.address<= br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 except ConnectError as err:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 logging.debug('Cannot connec= t to server %s', str(err))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.window.footer.set_text('= ;Server shutdown')

Like in other pl= aces, I wonder what happens if we have an unhandled exception here, because= this is running in a task.
=C2=A0
+
+=C2=A0 =C2=A0 def run(self):
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.aloop.set_debug(True)

Add a debug argument to run() and default it to False, and= add a --debug flag to the argparser that turns this on conditionally inste= ad.
=C2=A0
+=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 self.loop =3D urwid.MainLoop(self.window,
+=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 self.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 logging.error('%s\n%s\n'= , str(err), pretty_traceback())
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise err
+
+
+def main():
+=C2=A0 =C2=A0 parser =3D argparse.ArgumentParser(description=3D'AQMP T= UI')
+=C2=A0 =C2=A0 parser.add_argument('-a', '--address', metav= ar=3D'IP:PORT', required=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 help=3D'Address of the QMP server', dest=3D'address&= #39;)
+=C2=A0 =C2=A0 parser.add_argument('--log', help=3D'Address 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 dest=3D'log_file')
+=C2=A0 =C2=A0 args =3D parser.parse_args()
+
+=C2=A0 =C2=A0 logging.basicConfig(filename=3Dargs.log_file, level=3Dloggin= g.DEBUG)
+
+=C2=A0 =C2=A0 address =3D args.address.split(':')
+=C2=A0 =C2=A0 address[1] =3D int(address[1])
+
+=C2=A0 =C2=A0 App(tuple(address)).run()

I would take the address as a positional argument instead of with the = 9;--address' flag to mimic how qmp-shell works.
=C2=A0
+
+
+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 c62803bffc..c6d38451eb 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

Just keep= in mind that we'll need to remove these particular ignores. The rest c= an stay.
=C2=A0
+# 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


Looking good so far, let's focus o= n managing the connection state and making sure that Exceptions raised from= task contexts are handled properly. I still need to look more deeply into = the classes below App, but I wanted to give you your overdue feedback. Than= k you for your patience!

--js
--000000000000fff6f005c69422bf--