[PATCH RFC 00/32] python/qemu: refactor as installable package

[PATCH RFC 00/32] python/qemu: refactor as installable package

[John Snow]

Hey, I got lost on my way to the store and I accidentally got 32 patches
that convert our python library into something that passes pylint,
flake8, and mypy --strict.

...So, a few things:

1. This is just an RFC. The actual design of these libraries really
needs adjusted to be more pythonic. In general this means less
Optional[T] return types and raising more exceptions. This could be
handled later, but we ought to address it before publishing, if we do.

2. We still need to think carefully about how we package this, if we
even want to package it, what the license on the top-level package
should be, etc.

3. We should consider how to version it. For now, I'm using a lockstep
versioning.

4. You can install this package using pip3 or python3 setup.py to a
virtual environment or to your real one. From there, any python code in
the QEMU tree that imports these modules will work with no sys.path
hacking or custom PYTHONPATH exports.

5. You don't have to install it, though. I left all of the usual hacks
in place in the rest of the tree so that everything will just keep
working exactly as-is for right now. It's just that you COULD install it.

6. Here's a cool trick if you don't know about it yet:

> cd qemu/python/qemu
> pip3 install --user -e .

This will install the package in "develop" mode, which installs a
forwarder package. When you update your source tree, the installed
package stays "up to date" with the most recent edits.

Alright, have fun, stay safe!

John Snow (32):
  python/qemu: create qemu.lib module
  scripts/qmp: Fix shebang and imports
  python//machine.py: remove bare except
  python/qemu/lib: delint, add pylintrc
  python/qemu/lib: delint; add flake8 config
  python/qemu: formalize as package
  python/qemu: add README.rst
  python/qemu: Add Pipfile
  python/qemu: add pylint to Pipfile
  python/qemu: Add flake8 to Pipfile
  python/qemu/lib: remove Python2 style super() calls
  python/qemu/lib: fix socket.makefile() typing
  python/qemu/lib: Adjust traceback typing
  python//qmp.py: use True/False for non/blocking modes
  python//qmp.py: Define common types
  python//qmp.py: re-absorb MonitorResponseError
  python//qmp.py: Do not return None from cmd_obj
  python//qmp.py: add casts to JSON deserialization
  python//qmp.py: add QMPProtocolError
  python//qmp.py: assert sockfile is not None
  python//machine.py: remove logging configuration
  python//machine.py: Fix monitor address typing
  python//machine.py: reorder __init__
  python//machine.py: Don't modify state in _base_args()
  python//machine.py: Handle None events in event_wait
  python//machine.py: use qmp.command
  python//machine.py: Add _qmp access shim
  python//machine.py: fix _popen access
  python//qtest.py: Check before accessing _qtest
  python/qemu/lib: make 'args' style arguments immutable
  python/qemu: add mypy to Pipfile
  python/qemu/lib: Add mypy type annotations

 python/README.rst                         |   6 +
 python/qemu/README.rst                    |   8 +
 python/Pipfile                            |  14 +
 python/Pipfile.lock                       | 187 +++++++++++++
 python/qemu/__init__.py                   |  11 -
 python/qemu/lib/.flake8                   |   2 +
 python/qemu/lib/__init__.py               |  57 ++++
 python/qemu/{ => lib}/accel.py            |  17 +-
 python/qemu/{ => lib}/machine.py          | 320 +++++++++++++---------
 python/qemu/lib/pylintrc                  |  58 ++++
 python/qemu/{ => lib}/qmp.py              | 140 +++++++---
 python/qemu/lib/qtest.py                  | 160 +++++++++++
 python/qemu/qtest.py                      | 119 --------
 python/setup.py                           |  50 ++++
 scripts/device-crash-test                 |   2 +-
 scripts/qmp/qemu-ga-client                |   2 +-
 scripts/qmp/qmp                           |   4 +-
 scripts/qmp/qmp-shell                     |   2 +-
 scripts/qmp/qom-fuse                      |   4 +-
 scripts/qmp/qom-get                       |   6 +-
 scripts/qmp/qom-list                      |   4 +-
 scripts/qmp/qom-set                       |   6 +-
 scripts/qmp/qom-tree                      |   6 +-
 scripts/render_block_graph.py             |   5 +-
 scripts/simplebench/bench_block_job.py    |   4 +-
 tests/acceptance/avocado_qemu/__init__.py |   2 +-
 tests/acceptance/boot_linux.py            |   3 +-
 tests/acceptance/virtio_check_params.py   |   2 +-
 tests/acceptance/virtio_version.py        |   2 +-
 tests/migration/guestperf/engine.py       |   2 +-
 tests/qemu-iotests/235                    |   2 +-
 tests/qemu-iotests/iotests.py             |   2 +-
 tests/vm/basevm.py                        |   6 +-
 33 files changed, 881 insertions(+), 334 deletions(-)
 create mode 100644 python/README.rst
 create mode 100644 python/qemu/README.rst
 create mode 100644 python/Pipfile
 create mode 100644 python/Pipfile.lock
 delete mode 100644 python/qemu/__init__.py
 create mode 100644 python/qemu/lib/.flake8
 create mode 100644 python/qemu/lib/__init__.py
 rename python/qemu/{ => lib}/accel.py (86%)
 rename python/qemu/{ => lib}/machine.py (67%)
 create mode 100644 python/qemu/lib/pylintrc
 rename python/qemu/{ => lib}/qmp.py (70%)
 create mode 100644 python/qemu/lib/qtest.py
 delete mode 100644 python/qemu/qtest.py
 create mode 100755 python/setup.py

-- 
2.21.1

[PATCH RFC 01/32] python/qemu: create qemu.lib module

[John Snow]

move python/qemu/*.py to python/qemu/lib/*.py.

To create a namespace package, the 'qemu' directory itself shouldn't
have module files in it. Thus, these files will go under a 'lib' package
directory instead.

Bolster the lib/__init__.py file a little bit, Make the top-level
classes and functions available directly inside the `qemu.lib`
namespace, to facilitate a convenient shorthand:

> from qemu.lib import QEMUQtestMachine, QEMUMonitorProtocol

Lastly, update all of the existing import directives.

(Note: these scripts were not necessarily tested to see if they still
work. Some of these scripts are in obvious states of disrepair and it is
beyond the scope of this patch to attempt to fix them.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/__init__.py                   | 11 -----
 python/qemu/lib/__init__.py               | 57 +++++++++++++++++++++++
 python/qemu/{ => lib}/accel.py            |  0
 python/qemu/{ => lib}/machine.py          |  0
 python/qemu/{ => lib}/qmp.py              |  0
 python/qemu/{ => lib}/qtest.py            |  0
 scripts/device-crash-test                 |  2 +-
 scripts/qmp/qemu-ga-client                |  2 +-
 scripts/qmp/qmp-shell                     |  2 +-
 scripts/render_block_graph.py             |  3 +-
 scripts/simplebench/bench_block_job.py    |  4 +-
 tests/acceptance/avocado_qemu/__init__.py |  2 +-
 tests/acceptance/boot_linux.py            |  3 +-
 tests/acceptance/virtio_check_params.py   |  2 +-
 tests/acceptance/virtio_version.py        |  2 +-
 tests/migration/guestperf/engine.py       |  2 +-
 tests/qemu-iotests/235                    |  2 +-
 tests/qemu-iotests/iotests.py             |  2 +-
 tests/vm/basevm.py                        |  6 +--
 19 files changed, 74 insertions(+), 28 deletions(-)
 delete mode 100644 python/qemu/__init__.py
 create mode 100644 python/qemu/lib/__init__.py
 rename python/qemu/{ => lib}/accel.py (100%)
 rename python/qemu/{ => lib}/machine.py (100%)
 rename python/qemu/{ => lib}/qmp.py (100%)
 rename python/qemu/{ => lib}/qtest.py (100%)

diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
deleted file mode 100644
index 4ca06c34a4..0000000000
--- a/python/qemu/__init__.py
+++ /dev/null
@@ -1,11 +0,0 @@
-# QEMU library
-#
-# Copyright (C) 2015-2016 Red Hat Inc.
-# Copyright (C) 2012 IBM Corp.
-#
-# Authors:
-#  Fam Zheng <famz@redhat.com>
-#
-# This work is licensed under the terms of the GNU GPL, version 2.  See
-# the COPYING file in the top-level directory.
-#
diff --git a/python/qemu/lib/__init__.py b/python/qemu/lib/__init__.py
new file mode 100644
index 0000000000..afc587bfdc
--- /dev/null
+++ b/python/qemu/lib/__init__.py
@@ -0,0 +1,57 @@
+"""
+QEMU development and testing library.
+
+This library provides a few high-level classes for driving QEMU from a
+test suite, not intended for production use.
+
+- QEMUMachine: Configure and Boot a QEMU VM
+ - QEMUQtestMachine: VM class, with a qtest socket.
+
+- QEMUMonitorProtocol: Connect to, send/receive QMP messages.
+- QEMUQtestProtocol: Connect to, send/receive qtest message.
+
+- list_accel: List available accelerators
+- kvm_available: Probe for KVM support
+- tcg_available: Probe for TCG support
+"""
+
+# Copyright (C) 2020 John Snow for Red Hat Inc.
+# Copyright (C) 2015-2016 Red Hat Inc.
+# Copyright (C) 2012 IBM Corp.
+#
+# Authors:
+#  John Snow <jsnow@redhat.com>
+#  Fam Zheng <fam@euphon.net>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.  See
+# the COPYING file in the top-level directory.
+#
+
+from .accel import (
+    list_accel,
+    kvm_available,
+    tcg_available,
+)
+
+from .qmp import (
+    QEMUMonitorProtocol,
+)
+
+from .machine import (
+    QEMUMachine,
+)
+
+from .qtest import (
+    QEMUQtestProtocol,
+    QEMUQtestMachine,
+)
+
+__all__ = (
+    'list_accel',
+    'kvm_available',
+    'tcg_available',
+    'QEMUMonitorProtocol',
+    'QEMUMachine',
+    'QEMUQtestProtocol',
+    'QEMUQtestMachine',
+)
diff --git a/python/qemu/accel.py b/python/qemu/lib/accel.py
similarity index 100%
rename from python/qemu/accel.py
rename to python/qemu/lib/accel.py
diff --git a/python/qemu/machine.py b/python/qemu/lib/machine.py
similarity index 100%
rename from python/qemu/machine.py
rename to python/qemu/lib/machine.py
diff --git a/python/qemu/qmp.py b/python/qemu/lib/qmp.py
similarity index 100%
rename from python/qemu/qmp.py
rename to python/qemu/lib/qmp.py
diff --git a/python/qemu/qtest.py b/python/qemu/lib/qtest.py
similarity index 100%
rename from python/qemu/qtest.py
rename to python/qemu/lib/qtest.py
diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index 305d0427af..49efd4abd7 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -35,7 +35,7 @@ import argparse
 from itertools import chain
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
-from qemu.machine import QEMUMachine
+from qemu.lib import QEMUMachine
 
 logger = logging.getLogger('device-crash-test')
 dbg = logger.debug
diff --git a/scripts/qmp/qemu-ga-client b/scripts/qmp/qemu-ga-client
index ce122984a9..db27cfcd55 100755
--- a/scripts/qmp/qemu-ga-client
+++ b/scripts/qmp/qemu-ga-client
@@ -42,7 +42,7 @@ import base64
 import random
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu import qmp
+from qemu.lib import qmp
 
 
 class QemuGuestAgent(qmp.QEMUMonitorProtocol):
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
index a01d31de1e..2441f4ae01 100755
--- a/scripts/qmp/qmp-shell
+++ b/scripts/qmp/qmp-shell
@@ -75,7 +75,7 @@ import atexit
 import re
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu import qmp
+from qemu.lib import qmp
 
 if sys.version_info[0] == 2:
     input = raw_input
diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
index 409b4321f2..8048d9fbbe 100755
--- a/scripts/render_block_graph.py
+++ b/scripts/render_block_graph.py
@@ -25,7 +25,8 @@
 from graphviz import Digraph
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
-from qemu.machine import MonitorResponseError
+from qemu.lib import QEMUMonitorProtocol
+from qemu.lib.machine import MonitorResponseError
 
 
 def perm(arr):
diff --git a/scripts/simplebench/bench_block_job.py b/scripts/simplebench/bench_block_job.py
index 9808d696cf..dbac12ddec 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -25,8 +25,8 @@
 import json
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.machine import QEMUMachine
-from qemu.qmp import QMPConnectError
+from qemu.lib import QEMUMachine
+from qemu.lib.qmp import QMPConnectError
 
 
 def bench_block_job(cmd, cmd_args, qemu_args):
diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
index 59e7b4f763..bc433d4f40 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -32,7 +32,7 @@
 
 sys.path.append(os.path.join(SOURCE_DIR, 'python'))
 
-from qemu.machine import QEMUMachine
+from qemu.lib import QEMUMachine
 
 def is_readable_executable_file(path):
     return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index 075a386300..4a51e1324b 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -12,8 +12,7 @@
 
 from avocado_qemu import Test, BUILD_DIR
 
-from qemu.accel import kvm_available
-from qemu.accel import tcg_available
+from qemu.lib import kvm_available, tcg_available
 
 from avocado.utils import cloudinit
 from avocado.utils import network
diff --git a/tests/acceptance/virtio_check_params.py b/tests/acceptance/virtio_check_params.py
index 87e6c839d1..be8069778a 100644
--- a/tests/acceptance/virtio_check_params.py
+++ b/tests/acceptance/virtio_check_params.py
@@ -23,7 +23,7 @@
 import logging
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.machine import QEMUMachine
+from qemu.lib import QEMUMachine
 from avocado_qemu import Test
 from avocado import skip
 
diff --git a/tests/acceptance/virtio_version.py b/tests/acceptance/virtio_version.py
index 33593c29dd..726f50c603 100644
--- a/tests/acceptance/virtio_version.py
+++ b/tests/acceptance/virtio_version.py
@@ -12,7 +12,7 @@
 import os
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.machine import QEMUMachine
+from qemu.lib import QEMUMachine
 from avocado_qemu import Test
 
 # Virtio Device IDs:
diff --git a/tests/migration/guestperf/engine.py b/tests/migration/guestperf/engine.py
index fd63c66601..22dab849f9 100644
--- a/tests/migration/guestperf/engine.py
+++ b/tests/migration/guestperf/engine.py
@@ -29,7 +29,7 @@
 
 sys.path.append(os.path.join(os.path.dirname(__file__),
                              '..', '..', '..', 'python'))
-from qemu.machine import QEMUMachine
+from qemu.lib import QEMUMachine
 
 
 class Engine(object):
diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index d1b10ac36b..13a565934c 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -25,7 +25,7 @@ from iotests import qemu_img_create, qemu_io, file_path, log
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 
-from qemu.machine import QEMUMachine
+from qemu.lib import QEMUMachine
 
 iotests.script_initialize(supported_fmts=['qcow2'])
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6c0e781af7..c69ac07e69 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -34,7 +34,7 @@
 
 # pylint: disable=import-error, wrong-import-position
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu import qtest
+from qemu.lib import qtest
 
 assert sys.version_info >= (3, 6)
 
diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 756ccf7aca..4c89c62fb8 100644
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -18,9 +18,6 @@
 import logging
 import time
 import datetime
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.accel import kvm_available
-from qemu.machine import QEMUMachine
 import subprocess
 import hashlib
 import optparse
@@ -30,6 +27,9 @@
 import multiprocessing
 import traceback
 
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
+from qemu.lib import kvm_available, QEMUMachine
+
 SSH_KEY = open(os.path.join(os.path.dirname(__file__),
                "..", "keys", "id_rsa")).read()
 SSH_PUB_KEY = open(os.path.join(os.path.dirname(__file__),
-- 
2.21.1

[PATCH RFC 02/32] scripts/qmp: Fix shebang and imports

[John Snow]

There's more wrong with these scripts; They are in various stages of
disrepair. That's beyond the scope of this current patchset.

This just mechanically corrects the imports and the shebangs, as part of
ensuring that the python/qemu/lib refactoring didn't break anything
needlessly.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qmp/qmp      | 4 +++-
 scripts/qmp/qom-fuse | 4 +++-
 scripts/qmp/qom-get  | 6 ++++--
 scripts/qmp/qom-list | 4 +++-
 scripts/qmp/qom-set  | 6 ++++--
 scripts/qmp/qom-tree | 6 ++++--
 6 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/scripts/qmp/qmp b/scripts/qmp/qmp
index 0625fc2aba..5981f7c414 100755
--- a/scripts/qmp/qmp
+++ b/scripts/qmp/qmp
@@ -11,7 +11,9 @@
 # See the COPYING file in the top-level directory.
 
 import sys, os
-from qmp import QEMUMonitorProtocol
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
+from qemu.lib import QEMUMonitorProtocol
 
 def print_response(rsp, prefix=[]):
     if type(rsp) == list:
diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
index 6bada2c33d..6006f1d562 100755
--- a/scripts/qmp/qom-fuse
+++ b/scripts/qmp/qom-fuse
@@ -15,7 +15,9 @@ import fuse, stat
 from fuse import Fuse
 import os, posix
 from errno import *
-from qmp import QEMUMonitorProtocol
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
+from qemu.lib import QEMUMonitorProtocol
 
 fuse.fuse_python_api = (0, 2)
 
diff --git a/scripts/qmp/qom-get b/scripts/qmp/qom-get
index 007b4cd442..1d04d1b119 100755
--- a/scripts/qmp/qom-get
+++ b/scripts/qmp/qom-get
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 ##
 # QEMU Object Model test tools
 #
@@ -13,7 +13,9 @@
 
 import sys
 import os
-from qmp import QEMUMonitorProtocol
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
+from qemu.lib import QEMUMonitorProtocol
 
 cmd, args = sys.argv[0], sys.argv[1:]
 socket_path = None
diff --git a/scripts/qmp/qom-list b/scripts/qmp/qom-list
index 03bda3446b..94e3bc21ba 100755
--- a/scripts/qmp/qom-list
+++ b/scripts/qmp/qom-list
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 ##
 # QEMU Object Model test tools
 #
@@ -13,6 +13,8 @@
 
 import sys
 import os
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qmp import QEMUMonitorProtocol
 
 cmd, args = sys.argv[0], sys.argv[1:]
diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
index c37fe78b00..a78080a4fc 100755
--- a/scripts/qmp/qom-set
+++ b/scripts/qmp/qom-set
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 ##
 # QEMU Object Model test tools
 #
@@ -13,7 +13,9 @@
 
 import sys
 import os
-from qmp import QEMUMonitorProtocol
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
+from qemu.lib import QEMUMonitorProtocol
 
 cmd, args = sys.argv[0], sys.argv[1:]
 socket_path = None
diff --git a/scripts/qmp/qom-tree b/scripts/qmp/qom-tree
index 1c8acf61e7..4866c15581 100755
--- a/scripts/qmp/qom-tree
+++ b/scripts/qmp/qom-tree
@@ -1,4 +1,4 @@
-#!/usr/bin/python
+#!/usr/bin/env python3
 ##
 # QEMU Object Model test tools
 #
@@ -15,7 +15,9 @@
 
 import sys
 import os
-from qmp import QEMUMonitorProtocol
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
+from qemu.lib import QEMUMonitorProtocol
 
 cmd, args = sys.argv[0], sys.argv[1:]
 socket_path = None
-- 
2.21.1

[PATCH RFC 03/32] python//machine.py: remove bare except

[John Snow]

Catch only the timeout error; if there are other problems, allow the
stack trace to be visible.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index b9a98e2c86..e3ea523571 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -342,7 +342,26 @@ def wait(self):
         self._load_io_log()
         self._post_shutdown()
 
-    def shutdown(self, has_quit=False):
+    def _issue_shutdown(self, has_quit: bool = False) -> None:
+        """
+        Shutdown the VM.
+        """
+        if not self.is_running():
+            return
+
+        if self._qmp is not None:
+            if not has_quit:
+                self._qmp.cmd('quit')
+            self._qmp.close()
+
+            try:
+                self._popen.wait(timeout=3)
+            except subprocess.TimeoutExpired:
+                self._popen.kill()
+
+        self._popen.wait()
+
+    def shutdown(self, has_quit: bool = False) -> None:
         """
         Terminate the VM and clean up
         """
@@ -353,17 +372,7 @@ def shutdown(self, has_quit=False):
             self._console_socket.close()
             self._console_socket = None
 
-        if self.is_running():
-            if self._qmp:
-                try:
-                    if not has_quit:
-                        self._qmp.cmd('quit')
-                    self._qmp.close()
-                    self._popen.wait(timeout=3)
-                except:
-                    self._popen.kill()
-            self._popen.wait()
-
+        self._issue_shutdown(has_quit)
         self._load_io_log()
         self._post_shutdown()
 
-- 
2.21.1

[PATCH RFC 04/32] python/qemu/lib: delint, add pylintrc

[John Snow]

Bring our these files up to speed with pylint 2.5.0.
Add a pylintrc file to formalize which pylint subset
we are targeting.

The similarity ignore is there to suppress similarity
reports across imports, which for typing constants,
are going to trigger this report erroneously.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py |  6 ++--
 python/qemu/lib/pylintrc   | 58 ++++++++++++++++++++++++++++++++++++++
 python/qemu/lib/qtest.py   | 42 +++++++++++++++++----------
 3 files changed, 88 insertions(+), 18 deletions(-)
 create mode 100644 python/qemu/lib/pylintrc

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index e3ea523571..c79fc8fb89 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -58,7 +58,7 @@ def __init__(self, reply):
         self.reply = reply
 
 
-class QEMUMachine(object):
+class QEMUMachine:
     """
     A QEMU VM
 
@@ -242,7 +242,7 @@ def _base_args(self):
                          'chardev=mon,mode=control'])
         if self._machine is not None:
             args.extend(['-machine', self._machine])
-        for i in range(self._console_index):
+        for _ in range(self._console_index):
             args.extend(['-serial', 'null'])
         if self._console_set:
             self._console_address = os.path.join(self._sock_dir,
@@ -383,7 +383,7 @@ def shutdown(self, has_quit: bool = False) -> None:
                 command = ' '.join(self._qemu_full_args)
             else:
                 command = ''
-            LOG.warning(msg, -exitcode, command)
+            LOG.warning(msg, -int(exitcode), command)
 
         self._launched = False
 
diff --git a/python/qemu/lib/pylintrc b/python/qemu/lib/pylintrc
new file mode 100644
index 0000000000..5d6ae7367d
--- /dev/null
+++ b/python/qemu/lib/pylintrc
@@ -0,0 +1,58 @@
+[MASTER]
+
+[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
+# option multiple times (only on the command line, not in the configuration
+# file where it should appear only once). You can also use "--disable=all" to
+# disable everything first and then reenable specific checks. For example, if
+# you want to run only the similarities checker, you can use "--disable=all
+# --enable=similarities". If you want to run only the classes checker, but have
+# no Warning level messages displayed, use "--disable=all --enable=classes
+# --disable=W".
+disable=too-many-arguments,
+        too-many-instance-attributes,
+        too-many-public-methods,
+
+[REPORTS]
+
+[REFACTORING]
+
+[MISCELLANEOUS]
+
+[LOGGING]
+
+[BASIC]
+
+# Good variable names which should always be accepted, separated by a comma.
+good-names=i,
+           j,
+           k,
+           ex,
+           Run,
+           _,
+           fd,
+
+[VARIABLES]
+
+[STRING]
+
+[SPELLING]
+
+[FORMAT]
+
+[SIMILARITIES]
+
+# Ignore imports when computing similarities.
+ignore-imports=yes
+
+[TYPECHECK]
+
+[CLASSES]
+
+[IMPORTS]
+
+[DESIGN]
+
+[EXCEPTIONS]
diff --git a/python/qemu/lib/qtest.py b/python/qemu/lib/qtest.py
index d24ad04256..53d814c064 100644
--- a/python/qemu/lib/qtest.py
+++ b/python/qemu/lib/qtest.py
@@ -1,5 +1,11 @@
-# QEMU qtest library
-#
+"""
+QEMU qtest library
+
+qtest offers the QEMUQtestProtocol and QEMUQTestMachine classes, which
+offer a connection to QEMU's qtest protocol socket, and a qtest-enabled
+subclass of QEMUMachine, respectively.
+"""
+
 # Copyright (C) 2015 Red Hat Inc.
 #
 # Authors:
@@ -17,19 +23,21 @@
 from .machine import QEMUMachine
 
 
-class QEMUQtestProtocol(object):
+class QEMUQtestProtocol:
+    """
+    QEMUQtestProtocol implements a connection to a qtest socket.
+
+    :param address: QEMU address, can be either a unix socket path (string)
+                    or a tuple in the form ( address, port ) for a TCP
+                    connection
+    :param server: server mode, listens on the socket (bool)
+    :raise socket.error: on socket connection errors
+
+    .. note::
+       No conection is estabalished by __init__(), this is done
+       by the connect() or accept() methods.
+    """
     def __init__(self, address, server=False):
-        """
-        Create a QEMUQtestProtocol object.
-
-        @param address: QEMU address, can be either a unix socket path (string)
-                        or a tuple in the form ( address, port ) for a TCP
-                        connection
-        @param server: server mode, listens on the socket (bool)
-        @raise socket.error on socket connection errors
-        @note No connection is established, this is done by the connect() or
-              accept() methods
-        """
         self._address = address
         self._sock = self._get_sock()
         self._sockfile = None
@@ -73,15 +81,19 @@ def cmd(self, qtest_cmd):
         return resp
 
     def close(self):
+        """Close this socket."""
         self._sock.close()
         self._sockfile.close()
 
     def settimeout(self, timeout):
+        """Set a timeout, in seconds."""
         self._sock.settimeout(timeout)
 
 
 class QEMUQtestMachine(QEMUMachine):
-    '''A QEMU VM'''
+    """
+    A QEMU VM, with a qtest socket available.
+    """
 
     def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
                  socket_scm_helper=None, sock_dir=None):
-- 
2.21.1

[PATCH RFC 05/32] python/qemu/lib: delint; add flake8 config

[John Snow]

Mostly, ignore the "no bare except" rule, because flake8 is not
contextual and cannot determine if we re-raise. Pylint can, though, so
always prefer pylint for that.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/.flake8    |  2 ++
 python/qemu/lib/accel.py   |  9 ++++++---
 python/qemu/lib/machine.py | 13 +++++++++----
 python/qemu/lib/qmp.py     |  4 ++--
 4 files changed, 19 insertions(+), 9 deletions(-)
 create mode 100644 python/qemu/lib/.flake8

diff --git a/python/qemu/lib/.flake8 b/python/qemu/lib/.flake8
new file mode 100644
index 0000000000..45d8146f3f
--- /dev/null
+++ b/python/qemu/lib/.flake8
@@ -0,0 +1,2 @@
+[flake8]
+extend-ignore = E722  # Pylint handles this, but smarter.
\ No newline at end of file
diff --git a/python/qemu/lib/accel.py b/python/qemu/lib/accel.py
index 36ae85791e..7fabe62920 100644
--- a/python/qemu/lib/accel.py
+++ b/python/qemu/lib/accel.py
@@ -23,11 +23,12 @@
 # Mapping host architecture to any additional architectures it can
 # support which often includes its 32 bit cousin.
 ADDITIONAL_ARCHES = {
-    "x86_64" : "i386",
-    "aarch64" : "armhf",
-    "ppc64le" : "ppc64",
+    "x86_64": "i386",
+    "aarch64": "armhf",
+    "ppc64le": "ppc64",
 }
 
+
 def list_accel(qemu_bin):
     """
     List accelerators enabled in the QEMU binary.
@@ -47,6 +48,7 @@ def list_accel(qemu_bin):
     # Skip the first line which is the header.
     return [acc.strip() for acc in out.splitlines()[1:]]
 
+
 def kvm_available(target_arch=None, qemu_bin=None):
     """
     Check if KVM is available using the following heuristic:
@@ -69,6 +71,7 @@ def kvm_available(target_arch=None, qemu_bin=None):
         return False
     return True
 
+
 def tcg_available(qemu_bin):
     """
     Check if TCG is available.
diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index c79fc8fb89..4b260fa2cb 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -29,6 +29,7 @@
 
 LOG = logging.getLogger(__name__)
 
+
 class QEMUMachineError(Exception):
     """
     Exception called when an error in QEMUMachine happens.
@@ -62,7 +63,8 @@ class QEMUMachine:
     """
     A QEMU VM
 
-    Use this object as a context manager to ensure the QEMU process terminates::
+    Use this object as a context manager to ensure
+    the QEMU process terminates::
 
         with VM(binary) as vm:
             ...
@@ -188,8 +190,10 @@ def send_fd_scm(self, fd=None, file_path=None):
             fd_param.append(str(fd))
 
         devnull = open(os.path.devnull, 'rb')
-        proc = subprocess.Popen(fd_param, stdin=devnull, stdout=subprocess.PIPE,
-                                stderr=subprocess.STDOUT, close_fds=False)
+        proc = subprocess.Popen(
+            fd_param, stdin=devnull, stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT, close_fds=False
+        )
         output = proc.communicate()[0]
         if output:
             LOG.debug(output)
@@ -491,7 +495,8 @@ def event_wait(self, name, timeout=60.0, match=None):
 
     def events_wait(self, events, timeout=60.0):
         """
-        events_wait waits for and returns a named event from QMP with a timeout.
+        events_wait waits for and returns a named event
+        from QMP with a timeout.
 
         events: a sequence of (name, match_criteria) tuples.
                 The match criteria are optional and may be None.
diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
index d6c9b2f4b1..6ae7693965 100644
--- a/python/qemu/lib/qmp.py
+++ b/python/qemu/lib/qmp.py
@@ -168,8 +168,8 @@ def accept(self, timeout=15.0):
 
         @param timeout: timeout in seconds (nonnegative float number, or
                         None). The value passed will set the behavior of the
-                        underneath QMP socket as described in [1]. Default value
-                        is set to 15.0.
+                        underneath QMP socket as described in [1].
+                        Default value is set to 15.0.
         @return QMP greeting dict
         @raise OSError on socket connection errors
         @raise QMPConnectError if the greeting is not received
-- 
2.21.1

[PATCH RFC 06/32] python/qemu: formalize as package

[John Snow]

NB: I am choosing Python 3.6 here. Although our minimum requirement is
3.5, this code is used only by iotests (so far) under which we have been
using a minimum version of 3.6.

3.6 is being preferred here for variable type hint capability, which
enables us to use mypy for this package.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/README.rst |  6 ++++++
 python/setup.py   | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 python/README.rst
 create mode 100755 python/setup.py

diff --git a/python/README.rst b/python/README.rst
new file mode 100644
index 0000000000..25f6d93fd5
--- /dev/null
+++ b/python/README.rst
@@ -0,0 +1,6 @@
+QEMU Python Tooling
+-------------------
+
+This package provides QEMU tooling used by the QEMU project to build,
+configure, and test QEMU. It is not a fully-fledged SDK and it is subject
+to change at any time.
diff --git a/python/setup.py b/python/setup.py
new file mode 100755
index 0000000000..f897ceac97
--- /dev/null
+++ b/python/setup.py
@@ -0,0 +1,50 @@
+#!/usr/bin/env3 python
+"""
+QEMU tooling installer script
+Copyright (c) 2020 John Snow for Red Hat, Inc.
+"""
+
+import setuptools
+
+def main():
+    """
+    QEMU tooling installer
+    """
+
+    kwargs = {
+        'name': 'qemu',
+        'use_scm_version': {
+            'root': '..',
+            'relative_to': __file__,
+        },
+        'maintainer': 'QEMU Developer Team',
+        'maintainer_email': 'qemu-devel@nongnu.org',
+        'url': 'https://www.qemu.org/',
+        'download_url': 'https://www.qemu.org/download/',
+        'packages': setuptools.find_namespace_packages(),
+        'description': 'QEMU Python Build, Debug and SDK tooling.',
+        'classifiers': [
+            'Development Status :: 5 - Production/Stable',
+            'License :: OSI Approved :: GNU General Public License v2 (GPLv2)',
+            'Natural Language :: English',
+            'Operating System :: OS Independent',
+        ],
+        'platforms': [],
+        'keywords': [],
+        'setup_requires': [
+            'setuptools',
+            'setuptools_scm',
+        ],
+        'install_requires': [
+        ],
+        'python_requires': '>=3.6',
+        'long_description_content_type': 'text/x-rst',
+    }
+
+    with open("README.rst", "r") as fh:
+        kwargs['long_description'] = fh.read()
+
+    setuptools.setup(**kwargs)
+
+if __name__ == '__main__':
+    main()
-- 
2.21.1

[PATCH RFC 07/32] python/qemu: add README.rst

[John Snow]

Add a short readme that explains the package hierarchy, which will be
visible while browsing the source on e.g. gitlab/github.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/README.rst | 8 ++++++++
 1 file changed, 8 insertions(+)
 create mode 100644 python/qemu/README.rst

diff --git a/python/qemu/README.rst b/python/qemu/README.rst
new file mode 100644
index 0000000000..96447b9061
--- /dev/null
+++ b/python/qemu/README.rst
@@ -0,0 +1,8 @@
+python/qemu/
+------------
+
+This directory serves as the root of a `Python PEP 420 implicit
+namespace package <<https://www.python.org/dev/peps/pep-0420/>`_.
+
+Each directory below is assumed to be an installable Python module that
+will be available under the ``qemu.<module>`` namespace.
-- 
2.21.1

[PATCH RFC 08/32] python/qemu: Add Pipfile

[John Snow]

pipenv is a tool used for managing virtual environments with precisely
specified dependencies. It is separate from the dependencies listed in
setup.py, which are not pinned to precise versions.

Note that pipenv is not required to install or use this module; this is
just a convenience for in-tree developing.

Here, a "blank" pipfile is added with no dependencies, but specifies
Python 3.6 for the virtual environment.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Pipfile | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 python/Pipfile

diff --git a/python/Pipfile b/python/Pipfile
new file mode 100644
index 0000000000..9534830b5e
--- /dev/null
+++ b/python/Pipfile
@@ -0,0 +1,11 @@
+[[source]]
+name = "pypi"
+url = "https://pypi.org/simple"
+verify_ssl = true
+
+[dev-packages]
+
+[packages]
+
+[requires]
+python_version = "3.6"
-- 
2.21.1

[PATCH RFC 09/32] python/qemu: add pylint to Pipfile

[John Snow]

A bug in pylint 2.5.1 and 2.5.2 causes false positives for
relative imports. This version is pinned at 2.5.0 until a fix is
available.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Pipfile      |   1 +
 python/Pipfile.lock | 123 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)
 create mode 100644 python/Pipfile.lock

diff --git a/python/Pipfile b/python/Pipfile
index 9534830b5e..ddb2b5a518 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -4,6 +4,7 @@ url = "https://pypi.org/simple"
 verify_ssl = true
 
 [dev-packages]
+pylint = "==2.5.0"
 
 [packages]
 
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
new file mode 100644
index 0000000000..e6faa832e4
--- /dev/null
+++ b/python/Pipfile.lock
@@ -0,0 +1,123 @@
+{
+    "_meta": {
+        "hash": {
+            "sha256": "7815dedfd7481b645389153dd45e9adb82c72956d0efc74d8f087497624b75e4"
+        },
+        "pipfile-spec": 6,
+        "requires": {
+            "python_version": "3.6"
+        },
+        "sources": [
+            {
+                "name": "pypi",
+                "url": "https://pypi.org/simple",
+                "verify_ssl": true
+            }
+        ]
+    },
+    "default": {},
+    "develop": {
+        "astroid": {
+            "hashes": [
+                "sha256:4c17cea3e592c21b6e222f673868961bad77e1f985cb1694ed077475a89229c1",
+                "sha256:d8506842a3faf734b81599c8b98dcc423de863adcc1999248480b18bd31a0f38"
+            ],
+            "version": "==2.4.1"
+        },
+        "isort": {
+            "hashes": [
+                "sha256:54da7e92468955c4fceacd0c86bd0ec997b0e1ee80d97f67c35a78b719dccab1",
+                "sha256:6e811fcb295968434526407adb8796944f1988c5b65e8139058f2014cbe100fd"
+            ],
+            "version": "==4.3.21"
+        },
+        "lazy-object-proxy": {
+            "hashes": [
+                "sha256:0c4b206227a8097f05c4dbdd323c50edf81f15db3b8dc064d08c62d37e1a504d",
+                "sha256:194d092e6f246b906e8f70884e620e459fc54db3259e60cf69a4d66c3fda3449",
+                "sha256:1be7e4c9f96948003609aa6c974ae59830a6baecc5376c25c92d7d697e684c08",
+                "sha256:4677f594e474c91da97f489fea5b7daa17b5517190899cf213697e48d3902f5a",
+                "sha256:48dab84ebd4831077b150572aec802f303117c8cc5c871e182447281ebf3ac50",
+                "sha256:5541cada25cd173702dbd99f8e22434105456314462326f06dba3e180f203dfd",
+                "sha256:59f79fef100b09564bc2df42ea2d8d21a64fdcda64979c0fa3db7bdaabaf6239",
+                "sha256:8d859b89baf8ef7f8bc6b00aa20316483d67f0b1cbf422f5b4dc56701c8f2ffb",
+                "sha256:9254f4358b9b541e3441b007a0ea0764b9d056afdeafc1a5569eee1cc6c1b9ea",
+                "sha256:9651375199045a358eb6741df3e02a651e0330be090b3bc79f6d0de31a80ec3e",
+                "sha256:97bb5884f6f1cdce0099f86b907aa41c970c3c672ac8b9c8352789e103cf3156",
+                "sha256:9b15f3f4c0f35727d3a0fba4b770b3c4ebbb1fa907dbcc046a1d2799f3edd142",
+                "sha256:a2238e9d1bb71a56cd710611a1614d1194dc10a175c1e08d75e1a7bcc250d442",
+                "sha256:a6ae12d08c0bf9909ce12385803a543bfe99b95fe01e752536a60af2b7797c62",
+                "sha256:ca0a928a3ddbc5725be2dd1cf895ec0a254798915fb3a36af0964a0a4149e3db",
+                "sha256:cb2c7c57005a6804ab66f106ceb8482da55f5314b7fcb06551db1edae4ad1531",
+                "sha256:d74bb8693bf9cf75ac3b47a54d716bbb1a92648d5f781fc799347cfc95952383",
+                "sha256:d945239a5639b3ff35b70a88c5f2f491913eb94871780ebfabb2568bd58afc5a",
+                "sha256:eba7011090323c1dadf18b3b689845fd96a61ba0a1dfbd7f24b921398affc357",
+                "sha256:efa1909120ce98bbb3777e8b6f92237f5d5c8ea6758efea36a473e1d38f7d3e4",
+                "sha256:f3900e8a5de27447acbf900b4750b0ddfd7ec1ea7fbaf11dfa911141bc522af0"
+            ],
+            "version": "==1.4.3"
+        },
+        "mccabe": {
+            "hashes": [
+                "sha256:ab8a6258860da4b6677da4bd2fe5dc2c659cff31b3ee4f7f5d64e79735b80d42",
+                "sha256:dd8d182285a0fe56bace7f45b5e7d1a6ebcbf524e8f3bd87eb0f125271b8831f"
+            ],
+            "version": "==0.6.1"
+        },
+        "pylint": {
+            "hashes": [
+                "sha256:588e114e3f9a1630428c35b7dd1c82c1c93e1b0e78ee312ae4724c5e1a1e0245",
+                "sha256:bd556ba95a4cf55a1fc0004c00cf4560b1e70598a54a74c6904d933c8f3bd5a8"
+            ],
+            "index": "pypi",
+            "version": "==2.5.0"
+        },
+        "six": {
+            "hashes": [
+                "sha256:236bdbdce46e6e6a3d61a337c0f8b763ca1e8717c03b369e87a7ec7ce1319c0a",
+                "sha256:8f3cd2e254d8f793e7f3d6d9df77b92252b52637291d0f0da013c76ea2724b6c"
+            ],
+            "version": "==1.14.0"
+        },
+        "toml": {
+            "hashes": [
+                "sha256:229f81c57791a41d65e399fc06bf0848bab550a9dfd5ed66df18ce5f05e73d5c",
+                "sha256:235682dd292d5899d361a811df37e04a8828a5b1da3115886b73cf81ebc9100e"
+            ],
+            "version": "==0.10.0"
+        },
+        "typed-ast": {
+            "hashes": [
+                "sha256:0666aa36131496aed8f7be0410ff974562ab7eeac11ef351def9ea6fa28f6355",
+                "sha256:0c2c07682d61a629b68433afb159376e24e5b2fd4641d35424e462169c0a7919",
+                "sha256:249862707802d40f7f29f6e1aad8d84b5aa9e44552d2cc17384b209f091276aa",
+                "sha256:24995c843eb0ad11a4527b026b4dde3da70e1f2d8806c99b7b4a7cf491612652",
+                "sha256:269151951236b0f9a6f04015a9004084a5ab0d5f19b57de779f908621e7d8b75",
+                "sha256:4083861b0aa07990b619bd7ddc365eb7fa4b817e99cf5f8d9cf21a42780f6e01",
+                "sha256:498b0f36cc7054c1fead3d7fc59d2150f4d5c6c56ba7fb150c013fbc683a8d2d",
+                "sha256:4e3e5da80ccbebfff202a67bf900d081906c358ccc3d5e3c8aea42fdfdfd51c1",
+                "sha256:6daac9731f172c2a22ade6ed0c00197ee7cc1221aa84cfdf9c31defeb059a907",
+                "sha256:715ff2f2df46121071622063fc7543d9b1fd19ebfc4f5c8895af64a77a8c852c",
+                "sha256:73d785a950fc82dd2a25897d525d003f6378d1cb23ab305578394694202a58c3",
+                "sha256:8c8aaad94455178e3187ab22c8b01a3837f8ee50e09cf31f1ba129eb293ec30b",
+                "sha256:8ce678dbaf790dbdb3eba24056d5364fb45944f33553dd5869b7580cdbb83614",
+                "sha256:aaee9905aee35ba5905cfb3c62f3e83b3bec7b39413f0a7f19be4e547ea01ebb",
+                "sha256:bcd3b13b56ea479b3650b82cabd6b5343a625b0ced5429e4ccad28a8973f301b",
+                "sha256:c9e348e02e4d2b4a8b2eedb48210430658df6951fa484e59de33ff773fbd4b41",
+                "sha256:d205b1b46085271b4e15f670058ce182bd1199e56b317bf2ec004b6a44f911f6",
+                "sha256:d43943ef777f9a1c42bf4e552ba23ac77a6351de620aa9acf64ad54933ad4d34",
+                "sha256:d5d33e9e7af3b34a40dc05f498939f0ebf187f07c385fd58d591c533ad8562fe",
+                "sha256:fc0fea399acb12edbf8a628ba8d2312f583bdbdb3335635db062fa98cf71fca4",
+                "sha256:fe460b922ec15dd205595c9b5b99e2f056fd98ae8f9f56b888e7a17dc2b757e7"
+            ],
+            "markers": "implementation_name == 'cpython' and python_version < '3.8'",
+            "version": "==1.4.1"
+        },
+        "wrapt": {
+            "hashes": [
+                "sha256:b62ffa81fb85f4332a4f609cab4ac40709470da05643a082ec1eb88e6d9b97d7"
+            ],
+            "version": "==1.12.1"
+        }
+    }
+}
-- 
2.21.1

[PATCH RFC 10/32] python/qemu: Add flake8 to Pipfile

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Pipfile      |  1 +
 python/Pipfile.lock | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/python/Pipfile b/python/Pipfile
index ddb2b5a518..e396e56e06 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -5,6 +5,7 @@ verify_ssl = true
 
 [dev-packages]
 pylint = "==2.5.0"
+flake8 = "*"
 
 [packages]
 
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index e6faa832e4..15cd8a918f 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
     "_meta": {
         "hash": {
-            "sha256": "7815dedfd7481b645389153dd45e9adb82c72956d0efc74d8f087497624b75e4"
+            "sha256": "58115144ace0f646b5c62da260fb4867ac0a0e485de3f5b0a56c7854afa21f5b"
         },
         "pipfile-spec": 6,
         "requires": {
@@ -24,6 +24,21 @@
             ],
             "version": "==2.4.1"
         },
+        "entrypoints": {
+            "hashes": [
+                "sha256:589f874b313739ad35be6e0cd7efde2a4e9b6fea91edcc34e58ecbb8dbe56d19",
+                "sha256:c70dd71abe5a8c85e55e12c19bd91ccfeec11a6e99044204511f9ed547d48451"
+            ],
+            "version": "==0.3"
+        },
+        "flake8": {
+            "hashes": [
+                "sha256:45681a117ecc81e870cbf1262835ae4af5e7a8b08e40b944a8a6e6b895914cfb",
+                "sha256:49356e766643ad15072a789a20915d3c91dc89fd313ccd71802303fd67e4deca"
+            ],
+            "index": "pypi",
+            "version": "==3.7.9"
+        },
         "isort": {
             "hashes": [
                 "sha256:54da7e92468955c4fceacd0c86bd0ec997b0e1ee80d97f67c35a78b719dccab1",
@@ -64,6 +79,20 @@
             ],
             "version": "==0.6.1"
         },
+        "pycodestyle": {
+            "hashes": [
+                "sha256:95a2219d12372f05704562a14ec30bc76b05a5b297b21a5dfe3f6fac3491ae56",
+                "sha256:e40a936c9a450ad81df37f549d676d127b1b66000a6c500caa2b085bc0ca976c"
+            ],
+            "version": "==2.5.0"
+        },
+        "pyflakes": {
+            "hashes": [
+                "sha256:17dbeb2e3f4d772725c777fabc446d5634d1038f234e77343108ce445ea69ce0",
+                "sha256:d976835886f8c5b31d47970ed689944a0262b5f3afa00a5a7b4dc81e5449f8a2"
+            ],
+            "version": "==2.1.1"
+        },
         "pylint": {
             "hashes": [
                 "sha256:588e114e3f9a1630428c35b7dd1c82c1c93e1b0e78ee312ae4724c5e1a1e0245",
-- 
2.21.1

[PATCH RFC 11/32] python/qemu/lib: remove Python2 style super() calls

[John Snow]

Use the Python3 style instead.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py |  2 +-
 python/qemu/lib/qtest.py   | 15 +++++++--------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index 4b260fa2cb..b2f0412197 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -55,7 +55,7 @@ def __init__(self, reply):
             desc = reply["error"]["desc"]
         except KeyError:
             desc = reply
-        super(MonitorResponseError, self).__init__(desc)
+        super().__init__(desc)
         self.reply = reply
 
 
diff --git a/python/qemu/lib/qtest.py b/python/qemu/lib/qtest.py
index 53d814c064..7943487c2b 100644
--- a/python/qemu/lib/qtest.py
+++ b/python/qemu/lib/qtest.py
@@ -101,29 +101,28 @@ def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
             name = "qemu-%d" % os.getpid()
         if sock_dir is None:
             sock_dir = test_dir
-        super(QEMUQtestMachine,
-              self).__init__(binary, args, name=name, test_dir=test_dir,
-                             socket_scm_helper=socket_scm_helper,
-                             sock_dir=sock_dir)
+        super().__init__(binary, args, name=name, test_dir=test_dir,
+                         socket_scm_helper=socket_scm_helper,
+                         sock_dir=sock_dir)
         self._qtest = None
         self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
 
     def _base_args(self):
-        args = super(QEMUQtestMachine, self)._base_args()
+        args = super()._base_args()
         args.extend(['-qtest', 'unix:path=' + self._qtest_path,
                      '-accel', 'qtest'])
         return args
 
     def _pre_launch(self):
-        super(QEMUQtestMachine, self)._pre_launch()
+        super()._pre_launch()
         self._qtest = QEMUQtestProtocol(self._qtest_path, server=True)
 
     def _post_launch(self):
-        super(QEMUQtestMachine, self)._post_launch()
+        super()._post_launch()
         self._qtest.accept()
 
     def _post_shutdown(self):
-        super(QEMUQtestMachine, self)._post_shutdown()
+        super()._post_shutdown()
         self._remove_if_exists(self._qtest_path)
 
     def qtest(self, cmd):
-- 
2.21.1

[PATCH RFC 12/32] python/qemu/lib: fix socket.makefile() typing

[John Snow]

Note:

A bug in typeshed (https://github.com/python/typeshed/issues/3977)
misinterprets the type of makefile(). Work around this by explicitly
stating that we are opening a text-mode file.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/qmp.py   | 10 +++++++---
 python/qemu/lib/qtest.py | 12 ++++++++----
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
index 6ae7693965..73d49050ed 100644
--- a/python/qemu/lib/qmp.py
+++ b/python/qemu/lib/qmp.py
@@ -11,6 +11,10 @@
 import errno
 import socket
 import logging
+from typing import (
+    Optional,
+    TextIO,
+)
 
 
 class QMPError(Exception):
@@ -61,7 +65,7 @@ def __init__(self, address, server=False, nickname=None):
         self.__events = []
         self.__address = address
         self.__sock = self.__get_sock()
-        self.__sockfile = None
+        self.__sockfile: Optional[TextIO] = None
         self._nickname = nickname
         if self._nickname:
             self.logger = logging.getLogger('QMP').getChild(self._nickname)
@@ -157,7 +161,7 @@ def connect(self, negotiate=True):
         @raise QMPCapabilitiesError if fails to negotiate capabilities
         """
         self.__sock.connect(self.__address)
-        self.__sockfile = self.__sock.makefile()
+        self.__sockfile = self.__sock.makefile(mode='r')
         if negotiate:
             return self.__negotiate_capabilities()
         return None
@@ -180,7 +184,7 @@ def accept(self, timeout=15.0):
         """
         self.__sock.settimeout(timeout)
         self.__sock, _ = self.__sock.accept()
-        self.__sockfile = self.__sock.makefile()
+        self.__sockfile = self.__sock.makefile(mode='r')
         return self.__negotiate_capabilities()
 
     def cmd_obj(self, qmp_cmd):
diff --git a/python/qemu/lib/qtest.py b/python/qemu/lib/qtest.py
index 7943487c2b..4c88590eb0 100644
--- a/python/qemu/lib/qtest.py
+++ b/python/qemu/lib/qtest.py
@@ -19,6 +19,7 @@
 
 import socket
 import os
+from typing import Optional, TextIO
 
 from .machine import QEMUMachine
 
@@ -40,7 +41,7 @@ class QEMUQtestProtocol:
     def __init__(self, address, server=False):
         self._address = address
         self._sock = self._get_sock()
-        self._sockfile = None
+        self._sockfile: Optional[TextIO] = None
         if server:
             self._sock.bind(self._address)
             self._sock.listen(1)
@@ -59,7 +60,7 @@ def connect(self):
         @raise socket.error on socket connection errors
         """
         self._sock.connect(self._address)
-        self._sockfile = self._sock.makefile()
+        self._sockfile = self._sock.makefile(mode='r')
 
     def accept(self):
         """
@@ -68,7 +69,7 @@ def accept(self):
         @raise socket.error on socket connection errors
         """
         self._sock, _ = self._sock.accept()
-        self._sockfile = self._sock.makefile()
+        self._sockfile = self._sock.makefile(mode='r')
 
     def cmd(self, qtest_cmd):
         """
@@ -76,6 +77,7 @@ def cmd(self, qtest_cmd):
 
         @param qtest_cmd: qtest command text to be sent
         """
+        assert self._sockfile is not None
         self._sock.sendall((qtest_cmd + "\n").encode('utf-8'))
         resp = self._sockfile.readline()
         return resp
@@ -83,7 +85,9 @@ def cmd(self, qtest_cmd):
     def close(self):
         """Close this socket."""
         self._sock.close()
-        self._sockfile.close()
+        if self._sockfile:
+            self._sockfile.close()
+            self._sockfile = None
 
     def settimeout(self, timeout):
         """Set a timeout, in seconds."""
-- 
2.21.1

[PATCH RFC 13/32] python/qemu/lib: Adjust traceback typing

[John Snow]

mypy considers it incorrect to use `bool` to statically return false,
because it will assume that it could conceivably return True, and gives
different analysis in that case. Use a None return to achieve the same
effect, but make mypy happy.

Note: Pylint considers function signatures as code that might trip the
duplicate-code checker. I'd rather not disable this as it does not
trigger often in practice, so I'm disabling it as a one-off and filed a
change request; see https://github.com/PyCQA/pylint/issues/3619

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py |  8 ++++++--
 python/qemu/lib/qmp.py     | 10 ++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index b2f0412197..2f94c851ed 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -24,6 +24,8 @@
 import shutil
 import socket
 import tempfile
+from typing import Optional, Type
+from types import TracebackType
 
 from . import qmp
 
@@ -127,9 +129,11 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
     def __enter__(self):
         return self
 
-    def __exit__(self, exc_type, exc_val, exc_tb):
+    def __exit__(self,
+                 exc_type: Optional[Type[BaseException]],
+                 exc_val: Optional[BaseException],
+                 exc_tb: Optional[TracebackType]) -> None:
         self.shutdown()
-        return False
 
     def add_monitor_null(self):
         """
diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
index 73d49050ed..b91c9d5c1c 100644
--- a/python/qemu/lib/qmp.py
+++ b/python/qemu/lib/qmp.py
@@ -14,7 +14,9 @@
 from typing import (
     Optional,
     TextIO,
+    Type,
 )
+from types import TracebackType
 
 
 class QMPError(Exception):
@@ -146,10 +148,14 @@ def __enter__(self):
         # Implement context manager enter function.
         return self
 
-    def __exit__(self, exc_type, exc_value, exc_traceback):
+    def __exit__(self,
+                 # pylint: disable=duplicate-code
+                 # see https://github.com/PyCQA/pylint/issues/3619
+                 exc_type: Optional[Type[BaseException]],
+                 exc_val: Optional[BaseException],
+                 exc_tb: Optional[TracebackType]) -> None:
         # Implement context manager exit function.
         self.close()
-        return False
 
     def connect(self, negotiate=True):
         """
-- 
2.21.1

[PATCH RFC 14/32] python//qmp.py: use True/False for non/blocking modes

[John Snow]

The type system doesn't want integers.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/qmp.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
index b91c9d5c1c..a634c4e26c 100644
--- a/python/qemu/lib/qmp.py
+++ b/python/qemu/lib/qmp.py
@@ -120,14 +120,14 @@ def __get_events(self, wait=False):
         """
 
         # Check for new events regardless and pull them into the cache:
-        self.__sock.setblocking(0)
+        self.__sock.setblocking(False)
         try:
             self.__json_read()
         except OSError as err:
             if err.errno == errno.EAGAIN:
                 # No data available
                 pass
-        self.__sock.setblocking(1)
+        self.__sock.setblocking(True)
 
         # Wait for new events, if needed.
         # if wait is 0.0, this means "no wait" and is also implicitly false.
-- 
2.21.1

[PATCH RFC 15/32] python//qmp.py: Define common types

[John Snow]

Define some common types that we'll need to annotate a lot of other
functions going forward.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/qmp.py | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
index a634c4e26c..911da59888 100644
--- a/python/qemu/lib/qmp.py
+++ b/python/qemu/lib/qmp.py
@@ -12,13 +12,31 @@
 import socket
 import logging
 from typing import (
+    Any,
+    Dict,
     Optional,
     TextIO,
     Type,
+    Tuple,
+    Union,
 )
 from types import TracebackType
 
 
+# QMPMessage is a QMP Message of any kind.
+# e.g. {'yee': 'haw'}
+#
+# QMPReturnValue is the inner value of return values only.
+# {'return': {}} is the QMPMessage,
+# {} is the QMPReturnValue.
+QMPMessage = Dict[str, Any]
+QMPReturnValue = Dict[str, Any]
+
+InternetAddrT = Tuple[str, str]
+UnixAddrT = str
+SocketAddrT = Union[InternetAddrT, UnixAddrT]
+
+
 class QMPError(Exception):
     """
     QMP base exception
-- 
2.21.1

[PATCH RFC 16/32] python//qmp.py: re-absorb MonitorResponseError

[John Snow]

When I initially split this out, I considered this more of a machine
error than a QMP protocol error, but I think that's misguided.

Move this back to qmp.py and name it QMPResponseError. Convert
qmp.command() to use this exception type.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py    | 15 +--------------
 python/qemu/lib/qmp.py        | 17 +++++++++++++++--
 scripts/render_block_graph.py |  4 ++--
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index 2f94c851ed..c31bf7cabb 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -48,19 +48,6 @@ class QEMUMachineAddDeviceError(QEMUMachineError):
     """
 
 
-class MonitorResponseError(qmp.QMPError):
-    """
-    Represents erroneous QMP monitor reply
-    """
-    def __init__(self, reply):
-        try:
-            desc = reply["error"]["desc"]
-        except KeyError:
-            desc = reply
-        super().__init__(desc)
-        self.reply = reply
-
-
 class QEMUMachine:
     """
     A QEMU VM
@@ -433,7 +420,7 @@ def command(self, cmd, conv_keys=True, **args):
         if reply is None:
             raise qmp.QMPError("Monitor is closed")
         if "error" in reply:
-            raise MonitorResponseError(reply)
+            raise qmp.QMPResponseError(reply)
         return reply["return"]
 
     def get_qmp_event(self, wait=False):
diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
index 911da59888..82f86b4e45 100644
--- a/python/qemu/lib/qmp.py
+++ b/python/qemu/lib/qmp.py
@@ -61,6 +61,19 @@ class QMPTimeoutError(QMPError):
     """
 
 
+class QMPResponseError(QMPError):
+    """
+    Represents erroneous QMP monitor reply
+    """
+    def __init__(self, reply: QMPMessage):
+        try:
+            desc = reply['error']['desc']
+        except KeyError:
+            desc = reply
+        super().__init__(desc)
+        self.reply = reply
+
+
 class QEMUMonitorProtocol:
     """
     Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
@@ -250,8 +263,8 @@ def command(self, cmd, **kwds):
         Build and send a QMP command to the monitor, report errors if any
         """
         ret = self.cmd(cmd, kwds)
-        if "error" in ret:
-            raise Exception(ret['error']['desc'])
+        if 'error' in ret:
+            raise QMPResponseError(ret)
         return ret['return']
 
     def pull_event(self, wait=False):
diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
index 8048d9fbbe..332ab49a91 100755
--- a/scripts/render_block_graph.py
+++ b/scripts/render_block_graph.py
@@ -26,7 +26,7 @@
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
 from qemu.lib import QEMUMonitorProtocol
-from qemu.lib.machine import MonitorResponseError
+from qemu.lib.qmp import QMPResponseError
 
 
 def perm(arr):
@@ -103,7 +103,7 @@ def command(self, cmd):
         reply = json.loads(subprocess.check_output(ar))
 
         if 'error' in reply:
-            raise MonitorResponseError(reply)
+            raise QEMUResponseError(reply)
 
         return reply['return']
 
-- 
2.21.1

[PATCH RFC 17/32] python//qmp.py: Do not return None from cmd_obj

[John Snow]

This makes typing the qmp library difficult, as it necessitates wrapping
Optional[] around the type for every return type up the stack. At some
point, it becomes difficult to discern or remember why it's None instead
of the expected object.

Use the python exception system to tell us exactly why we didn't get an
object. Remove this special-cased return.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/qmp.py | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
index 82f86b4e45..0036204218 100644
--- a/python/qemu/lib/qmp.py
+++ b/python/qemu/lib/qmp.py
@@ -224,22 +224,18 @@ def accept(self, timeout=15.0):
         self.__sockfile = self.__sock.makefile(mode='r')
         return self.__negotiate_capabilities()
 
-    def cmd_obj(self, qmp_cmd):
+    def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
         """
         Send a QMP command to the QMP Monitor.
 
         @param qmp_cmd: QMP command to be sent as a Python dict
-        @return QMP response as a Python dict or None if the connection has
-                been closed
+        @return QMP response as a Python dict
         """
         self.logger.debug(">>> %s", qmp_cmd)
-        try:
-            self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
-        except OSError as err:
-            if err.errno == errno.EPIPE:
-                return None
-            raise err
+        self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
         resp = self.__json_read()
+        if resp is None:
+            raise QMPConnectError("Unexpected empty reply from server")
         self.logger.debug("<<< %s", resp)
         return resp
 
-- 
2.21.1

[PATCH RFC 18/32] python//qmp.py: add casts to JSON deserialization

[John Snow]

mypy and python type hints are not powerful enough to properly describe
JSON messages in Python 3.6. The best we can do, generally, is describe
them as Dict[str, Any].

Add casts to coerce this type for static analysis; but do NOT enforce
this type at runtime in any way.

Note: Python 3.8 adds a TypedDict construct which allows for the
description of more arbitrary Dictionary shapes. There is a third-party
module, "Pydantic", which is compatible with 3.6 that can be used
instead of the JSON library that parses JSON messages to fully-typed
Python objects, and may be preferable in some cases.

(That is well beyond the scope of this commit or series.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/qmp.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
index 0036204218..e460234f2e 100644
--- a/python/qemu/lib/qmp.py
+++ b/python/qemu/lib/qmp.py
@@ -13,6 +13,7 @@
 import logging
 from typing import (
     Any,
+    cast,
     Dict,
     Optional,
     TextIO,
@@ -129,7 +130,10 @@ def __json_read(self, only_event=False):
             data = self.__sockfile.readline()
             if not data:
                 return None
-            resp = json.loads(data)
+            # By definition, any JSON received from QMP is a QMPMessage,
+            # and we are asserting only at static analysis time that it
+            # has a particular shape.
+            resp = cast(QMPMessage, json.loads(data))
             if 'event' in resp:
                 self.logger.debug("<<< %s", resp)
                 self.__events.append(resp)
@@ -261,7 +265,7 @@ def command(self, cmd, **kwds):
         ret = self.cmd(cmd, kwds)
         if 'error' in ret:
             raise QMPResponseError(ret)
-        return ret['return']
+        return cast(QMPReturnValue, ret['return'])
 
     def pull_event(self, wait=False):
         """
-- 
2.21.1

[PATCH RFC 19/32] python//qmp.py: add QMPProtocolError

[John Snow]

In the case that we receive a reply but are unable to understand it, use
this exception name to indicate that case.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/qmp.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
index e460234f2e..5fb16f4b42 100644
--- a/python/qemu/lib/qmp.py
+++ b/python/qemu/lib/qmp.py
@@ -62,6 +62,12 @@ class QMPTimeoutError(QMPError):
     """
 
 
+class QMPProtocolError(QMPError):
+    """
+    QMP protocol error; unexpected response
+    """
+
+
 class QMPResponseError(QMPError):
     """
     Represents erroneous QMP monitor reply
@@ -265,6 +271,10 @@ def command(self, cmd, **kwds):
         ret = self.cmd(cmd, kwds)
         if 'error' in ret:
             raise QMPResponseError(ret)
+        if 'return' not in ret:
+            raise QMPProtocolError(
+                "'return' key not found in QMP response '{}'".format(str(ret))
+            )
         return cast(QMPReturnValue, ret['return'])
 
     def pull_event(self, wait=False):
-- 
2.21.1

[PATCH RFC 20/32] python//qmp.py: assert sockfile is not None

[John Snow]

In truth, if you don't do this, you'll just get a TypeError
exception. Now, you'll get an AssertionError.

Is this tangibly better? No.
Does mypy complain less? Yes.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/qmp.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
index 5fb16f4b42..1aefc00c93 100644
--- a/python/qemu/lib/qmp.py
+++ b/python/qemu/lib/qmp.py
@@ -132,6 +132,7 @@ def __negotiate_capabilities(self):
         raise QMPCapabilitiesError
 
     def __json_read(self, only_event=False):
+        assert self.__sockfile is not None
         while True:
             data = self.__sockfile.readline()
             if not data:
-- 
2.21.1

[PATCH RFC 21/32] python//machine.py: remove logging configuration

[John Snow]

Python 3.5 and above do not print a warning when logging is not
configured. As a library, it's best practice to leave logging
configuration to the client executable.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index c31bf7cabb..e92afe8649 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -110,9 +110,6 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
         self._console_socket = None
         self._remove_files = []
 
-        # just in case logging wasn't configured by the main script:
-        logging.basicConfig()
-
     def __enter__(self):
         return self
 
-- 
2.21.1

[PATCH RFC 22/32] python//machine.py: Fix monitor address typing

[John Snow]

Prior to this, it's difficult for mypy to intuit what the concrete type
of the monitor address is; it has difficulty inferring the type across
two variables.

Create _monitor_address as a property that always returns a valid
address to simply static type analysis.

To preserve our ability to clean up, use a simple boolean to indicate
whether or not we should try to clean up the sock file after execution.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py | 45 ++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index e92afe8649..6a4aea7725 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -24,10 +24,14 @@
 import shutil
 import socket
 import tempfile
-from typing import Optional, Type
+from typing import (
+    Optional,
+    Type,
+)
 from types import TracebackType
 
 from . import qmp
+from .qmp import SocketAddrT
 
 LOG = logging.getLogger(__name__)
 
@@ -61,7 +65,8 @@ class QEMUMachine:
     """
 
     def __init__(self, binary, args=None, wrapper=None, name=None,
-                 test_dir="/var/tmp", monitor_address=None,
+                 test_dir="/var/tmp",
+                 monitor_address: Optional[SocketAddrT] = None,
                  socket_scm_helper=None, sock_dir=None):
         '''
         Initialize a QEMUMachine
@@ -84,8 +89,14 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
         if sock_dir is None:
             sock_dir = test_dir
         self._name = name
-        self._monitor_address = monitor_address
-        self._vm_monitor = None
+        if monitor_address is not None:
+            self._monitor_address = monitor_address
+            self._remove_monitor_sockfile = False
+        else:
+            self._monitor_address = os.path.join(
+                sock_dir, f"{name}-monitor.sock"
+            )
+            self._remove_monitor_sockfile = True
         self._qemu_log_path = None
         self._qemu_log_file = None
         self._popen = None
@@ -223,15 +234,17 @@ def _load_io_log(self):
 
     def _base_args(self):
         args = ['-display', 'none', '-vga', 'none']
+
         if self._qmp_set:
             if isinstance(self._monitor_address, tuple):
-                moncdev = "socket,id=mon,host=%s,port=%s" % (
-                    self._monitor_address[0],
-                    self._monitor_address[1])
+                moncdev = "socket,id=mon,host={},port={}".format(
+                    *self._monitor_address
+                )
             else:
-                moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
+                moncdev = f"socket,id=mon,path={self._monitor_address}"
             args.extend(['-chardev', moncdev, '-mon',
                          'chardev=mon,mode=control'])
+
         if self._machine is not None:
             args.extend(['-machine', self._machine])
         for _ in range(self._console_index):
@@ -256,14 +269,14 @@ def _pre_launch(self):
         self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
         if self._qmp_set:
-            if self._monitor_address is not None:
-                self._vm_monitor = self._monitor_address
-            else:
-                self._vm_monitor = os.path.join(self._sock_dir,
-                                                self._name + "-monitor.sock")
-                self._remove_files.append(self._vm_monitor)
-            self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor, server=True,
-                                                nickname=self._name)
+            if self._remove_monitor_sockfile:
+                assert isinstance(self._monitor_address, str)
+                self._remove_files.append(self._monitor_address)
+            self._qmp = qmp.QEMUMonitorProtocol(
+                self._monitor_address,
+                server=True,
+                nickname=self._name
+            )
 
     def _post_launch(self):
         if self._qmp:
-- 
2.21.1

[PATCH RFC 23/32] python//machine.py: reorder __init__

[John Snow]

Put the init arg handling all at the top, and mostly in order (deviating
when one is dependent on another), and put what is effectively runtime
state declaration at the bottom.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index 6a4aea7725..beb31be453 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -80,38 +80,43 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
         @param socket_scm_helper: helper program, required for send_fd_scm()
         @note: Qemu process is not started until launch() is used.
         '''
+        # Direct user configuration
+
+        self._binary = binary
+
         if args is None:
             args = []
+        # Copy mutable input: we will be modifying our copy
+        self._args = list(args)
+
         if wrapper is None:
             wrapper = []
-        if name is None:
-            name = "qemu-%d" % os.getpid()
-        if sock_dir is None:
-            sock_dir = test_dir
-        self._name = name
+        self._wrapper = wrapper
+
+        self._name = name or "qemu-%d" % os.getpid()
+        self._test_dir = test_dir
+        self._sock_dir = sock_dir or self._test_dir
+        self._socket_scm_helper = socket_scm_helper
+
         if monitor_address is not None:
             self._monitor_address = monitor_address
             self._remove_monitor_sockfile = False
         else:
             self._monitor_address = os.path.join(
-                sock_dir, f"{name}-monitor.sock"
+                self._sock_dir, f"{self._name}-monitor.sock"
             )
             self._remove_monitor_sockfile = True
+
+        # Runstate
         self._qemu_log_path = None
         self._qemu_log_file = None
         self._popen = None
-        self._binary = binary
-        self._args = list(args)     # Force copy args in case we modify them
-        self._wrapper = wrapper
         self._events = []
         self._iolog = None
-        self._socket_scm_helper = socket_scm_helper
         self._qmp_set = True   # Enable QMP monitor by default.
         self._qmp = None
         self._qemu_full_args = None
-        self._test_dir = test_dir
         self._temp_dir = None
-        self._sock_dir = sock_dir
         self._launched = False
         self._machine = None
         self._console_index = 0
-- 
2.21.1

[PATCH RFC 24/32] python//machine.py: Don't modify state in _base_args()

[John Snow]

Don't append to the _remove_files list during _base_args; instead do so
during _launch. Rework _base_args as a @property to help facilitate
this impression.

This has the additional benefit of making the type of _console_address
easier to analyze statically.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py | 16 ++++++++++------
 python/qemu/lib/qtest.py   | 11 ++++++++---
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index beb31be453..8548c7c32d 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -25,6 +25,7 @@
 import socket
 import tempfile
 from typing import (
+    List,
     Optional,
     Type,
 )
@@ -122,7 +123,9 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
         self._console_index = 0
         self._console_set = False
         self._console_device_type = None
-        self._console_address = None
+        self._console_address = os.path.join(
+            self._sock_dir, f"{self._name}-console.sock"
+        )
         self._console_socket = None
         self._remove_files = []
 
@@ -237,7 +240,8 @@ def _load_io_log(self):
             with open(self._qemu_log_path, "r") as iolog:
                 self._iolog = iolog.read()
 
-    def _base_args(self):
+    @property
+    def _base_args(self) -> List[str]:
         args = ['-display', 'none', '-vga', 'none']
 
         if self._qmp_set:
@@ -255,9 +259,6 @@ def _base_args(self):
         for _ in range(self._console_index):
             args.extend(['-serial', 'null'])
         if self._console_set:
-            self._console_address = os.path.join(self._sock_dir,
-                                                 self._name + "-console.sock")
-            self._remove_files.append(self._console_address)
             chardev = ('socket,id=console,path=%s,server,nowait' %
                        self._console_address)
             args.extend(['-chardev', chardev])
@@ -273,6 +274,9 @@ def _pre_launch(self):
         self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
         self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
+        if self._console_set:
+            self._remove_files.append(self._console_address)
+
         if self._qmp_set:
             if self._remove_monitor_sockfile:
                 assert isinstance(self._monitor_address, str)
@@ -332,7 +336,7 @@ def _launch(self):
         devnull = open(os.path.devnull, 'rb')
         self._pre_launch()
         self._qemu_full_args = (self._wrapper + [self._binary] +
-                                self._base_args() + self._args)
+                                self._base_args + self._args)
         LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
         self._popen = subprocess.Popen(self._qemu_full_args,
                                        stdin=devnull,
diff --git a/python/qemu/lib/qtest.py b/python/qemu/lib/qtest.py
index 4c88590eb0..a8be0c782f 100644
--- a/python/qemu/lib/qtest.py
+++ b/python/qemu/lib/qtest.py
@@ -19,7 +19,11 @@
 
 import socket
 import os
-from typing import Optional, TextIO
+from typing import (
+    List,
+    Optional,
+    TextIO,
+)
 
 from .machine import QEMUMachine
 
@@ -111,8 +115,9 @@ def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
         self._qtest = None
         self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
 
-    def _base_args(self):
-        args = super()._base_args()
+    @property
+    def _base_args(self) -> List[str]:
+        args = super()._base_args
         args.extend(['-qtest', 'unix:path=' + self._qtest_path,
                      '-accel', 'qtest'])
         return args
-- 
2.21.1

[PATCH RFC 25/32] python//machine.py: Handle None events in event_wait

[John Snow]

If the timeout is 0, we can get None back. Handle this explicitly.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index 8548c7c32d..61ee3a0e81 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -32,7 +32,7 @@
 from types import TracebackType
 
 from . import qmp
-from .qmp import SocketAddrT
+from .qmp import SocketAddrT, QMPMessage
 
 LOG = logging.getLogger(__name__)
 
@@ -519,6 +519,8 @@ def _match(event):
                     return True
             return False
 
+        event: Optional[QMPMessage]
+
         # Search cached events
         for event in self._events:
             if _match(event):
@@ -528,6 +530,8 @@ def _match(event):
         # Poll for new events
         while True:
             event = self._qmp.pull_event(wait=timeout)
+            if event is None:
+                break
             if _match(event):
                 return event
             self._events.append(event)
-- 
2.21.1

[PATCH RFC 26/32] python//machine.py: use qmp.command

[John Snow]

machine.py and qmp.py both do the same thing here; refactor machine.py
to use qmp.py's functionality more directly.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index 61ee3a0e81..34e6b6f9e9 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -25,6 +25,8 @@
 import socket
 import tempfile
 from typing import (
+    Any,
+    Dict,
     List,
     Optional,
     Type,
@@ -416,17 +418,23 @@ def set_qmp_monitor(self, enabled=True):
             self._qmp_set = False
             self._qmp = None
 
-    def qmp(self, cmd, conv_keys=True, **args):
-        """
-        Invoke a QMP command and return the response dict
-        """
+    @classmethod
+    def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
         qmp_args = dict()
         for key, value in args.items():
-            if conv_keys:
+            if _conv_keys:
                 qmp_args[key.replace('_', '-')] = value
             else:
                 qmp_args[key] = value
+        return qmp_args
 
+    def qmp(self, cmd: str,
+            conv_keys: bool = True,
+            **args: Any) -> QMPMessage:
+        """
+        Invoke a QMP command and return the response dict
+        """
+        qmp_args = self._qmp_args(conv_keys, **args)
         return self._qmp.cmd(cmd, args=qmp_args)
 
     def command(self, cmd, conv_keys=True, **args):
@@ -435,12 +443,8 @@ def command(self, cmd, conv_keys=True, **args):
         On success return the response dict.
         On failure raise an exception.
         """
-        reply = self.qmp(cmd, conv_keys, **args)
-        if reply is None:
-            raise qmp.QMPError("Monitor is closed")
-        if "error" in reply:
-            raise qmp.QMPResponseError(reply)
-        return reply["return"]
+        qmp_args = self._qmp_args(conv_keys, **args)
+        return self._qmp.command(cmd, **qmp_args)
 
     def get_qmp_event(self, wait=False):
         """
-- 
2.21.1

[PATCH RFC 27/32] python//machine.py: Add _qmp access shim

[John Snow]

Like many other Optional[] types, it's not always a given that this
object will be set. Wrap it in a type-shim that raises a meaningful
error and will always return a concrete type.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index 34e6b6f9e9..dfa8449b62 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -117,7 +117,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
         self._events = []
         self._iolog = None
         self._qmp_set = True   # Enable QMP monitor by default.
-        self._qmp = None
+        self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
         self._qemu_full_args = None
         self._temp_dir = None
         self._launched = False
@@ -283,7 +283,7 @@ def _pre_launch(self):
             if self._remove_monitor_sockfile:
                 assert isinstance(self._monitor_address, str)
                 self._remove_files.append(self._monitor_address)
-            self._qmp = qmp.QEMUMonitorProtocol(
+            self._qmp_connection = qmp.QEMUMonitorProtocol(
                 self._monitor_address,
                 server=True,
                 nickname=self._name
@@ -416,7 +416,13 @@ def set_qmp_monitor(self, enabled=True):
             self._qmp_set = True
         else:
             self._qmp_set = False
-            self._qmp = None
+            self._qmp_connection = None
+
+    @property
+    def _qmp(self) -> qmp.QEMUMonitorProtocol:
+        if self._qmp_connection is None:
+            raise QEMUMachineError("Attempt to access QMP with no connection")
+        return self._qmp_connection
 
     @classmethod
     def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
-- 
2.21.1

[PATCH RFC 28/32] python//machine.py: fix _popen access

[John Snow]

As always, Optional[T] causes problems with unchecked access. Add a
helper that asserts the pipe is present before we attempt to talk with
it.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index dfa8449b62..fb1a02b53c 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -113,7 +113,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
         # Runstate
         self._qemu_log_path = None
         self._qemu_log_file = None
-        self._popen = None
+        self._popen: Optional['subprocess.Popen[bytes]'] = None
         self._events = []
         self._iolog = None
         self._qmp_set = True   # Enable QMP monitor by default.
@@ -225,6 +225,12 @@ def is_running(self):
         """Returns true if the VM is running."""
         return self._popen is not None and self._popen.poll() is None
 
+    @property
+    def _subp(self) -> 'subprocess.Popen[bytes]':
+        if self._popen is None:
+            raise QEMUMachineError('Subprocess pipe not present')
+        return self._popen
+
     def exitcode(self):
         """Returns the exit code if possible, or None."""
         if self._popen is None:
@@ -235,7 +241,7 @@ def get_pid(self):
         """Returns the PID of the running process, or None."""
         if not self.is_running():
             return None
-        return self._popen.pid
+        return self._subp.pid
 
     def _load_io_log(self):
         if self._qemu_log_path is not None:
@@ -352,7 +358,7 @@ def wait(self):
         """
         Wait for the VM to power off
         """
-        self._popen.wait()
+        self._subp.wait()
         if self._qmp:
             self._qmp.close()
         self._load_io_log()
@@ -371,11 +377,11 @@ def _issue_shutdown(self, has_quit: bool = False) -> None:
             self._qmp.close()
 
             try:
-                self._popen.wait(timeout=3)
+                self._subp.wait(timeout=3)
             except subprocess.TimeoutExpired:
-                self._popen.kill()
+                self._subp.kill()
 
-        self._popen.wait()
+        self._subp.wait()
 
     def shutdown(self, has_quit: bool = False) -> None:
         """
-- 
2.21.1

[PATCH RFC 29/32] python//qtest.py: Check before accessing _qtest

[John Snow]

It can be None; so add assertions or exceptions where appropriate to
guard the access accordingly.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/qtest.py | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/python/qemu/lib/qtest.py b/python/qemu/lib/qtest.py
index a8be0c782f..05c63a1d58 100644
--- a/python/qemu/lib/qtest.py
+++ b/python/qemu/lib/qtest.py
@@ -126,7 +126,8 @@ def _pre_launch(self):
         super()._pre_launch()
         self._qtest = QEMUQtestProtocol(self._qtest_path, server=True)
 
-    def _post_launch(self):
+    def _post_launch(self) -> None:
+        assert self._qtest is not None
         super()._post_launch()
         self._qtest.accept()
 
@@ -134,6 +135,13 @@ def _post_shutdown(self):
         super()._post_shutdown()
         self._remove_if_exists(self._qtest_path)
 
-    def qtest(self, cmd):
-        '''Send a qtest command to guest'''
+    def qtest(self, cmd: str) -> str:
+        """
+        Send a qtest command to the guest.
+
+        :param cmd: qtest command to send
+        :return: qtest server response
+        """
+        if self._qtest is None:
+            raise RuntimeError("qtest socket not available")
         return self._qtest.cmd(cmd)
-- 
2.21.1

[PATCH RFC 30/32] python/qemu/lib: make 'args' style arguments immutable

[John Snow]

These arguments don't need to be mutable and aren't really used as
such. Clarify their types as immutable and adjust code to match where
necessary.

In general, It's probably best not to accept a user-defined mutable
object and store it as internal object state unless there's a strong
justification for doing so. Instead, try to use generic types as input
with empty tuples as the default, and coerce to list where necessary.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/machine.py | 30 +++++++++++++++++-------------
 python/qemu/lib/qtest.py   | 16 ++++++++++++----
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index fb1a02b53c..ec2bb28b86 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -18,6 +18,7 @@
 #
 
 import errno
+from itertools import chain
 import logging
 import os
 import subprocess
@@ -29,6 +30,8 @@
     Dict,
     List,
     Optional,
+    Sequence,
+    Tuple,
     Type,
 )
 from types import TracebackType
@@ -67,8 +70,12 @@ class QEMUMachine:
         # vm is guaranteed to be shut down here
     """
 
-    def __init__(self, binary, args=None, wrapper=None, name=None,
-                 test_dir="/var/tmp",
+    def __init__(self,
+                 binary: str,
+                 args: Sequence[str] = (),
+                 wrapper: Sequence[str] = (),
+                 name: Optional[str] = None,
+                 test_dir: str = "/var/tmp",
                  monitor_address: Optional[SocketAddrT] = None,
                  socket_scm_helper=None, sock_dir=None):
         '''
@@ -86,14 +93,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
         # Direct user configuration
 
         self._binary = binary
-
-        if args is None:
-            args = []
-        # Copy mutable input: we will be modifying our copy
         self._args = list(args)
-
-        if wrapper is None:
-            wrapper = []
         self._wrapper = wrapper
 
         self._name = name or "qemu-%d" % os.getpid()
@@ -118,7 +118,7 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
         self._iolog = None
         self._qmp_set = True   # Enable QMP monitor by default.
         self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
-        self._qemu_full_args = None
+        self._qemu_full_args: Tuple[str, ...] = ()
         self._temp_dir = None
         self._launched = False
         self._machine = None
@@ -323,7 +323,7 @@ def launch(self):
             raise QEMUMachineError('VM already launched')
 
         self._iolog = None
-        self._qemu_full_args = None
+        self._qemu_full_args = ()
         try:
             self._launch()
             self._launched = True
@@ -343,8 +343,12 @@ def _launch(self):
         """
         devnull = open(os.path.devnull, 'rb')
         self._pre_launch()
-        self._qemu_full_args = (self._wrapper + [self._binary] +
-                                self._base_args + self._args)
+        self._qemu_full_args = tuple(
+            chain(self._wrapper,
+                  [self._binary],
+                  self._base_args,
+                  self._args)
+        )
         LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
         self._popen = subprocess.Popen(self._qemu_full_args,
                                        stdin=devnull,
diff --git a/python/qemu/lib/qtest.py b/python/qemu/lib/qtest.py
index 05c63a1d58..ae4661d4d3 100644
--- a/python/qemu/lib/qtest.py
+++ b/python/qemu/lib/qtest.py
@@ -22,6 +22,7 @@
 from typing import (
     List,
     Optional,
+    Sequence,
     TextIO,
 )
 
@@ -103,8 +104,13 @@ class QEMUQtestMachine(QEMUMachine):
     A QEMU VM, with a qtest socket available.
     """
 
-    def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
-                 socket_scm_helper=None, sock_dir=None):
+    def __init__(self,
+                 binary: str,
+                 args: Sequence[str] = (),
+                 name: Optional[str] = None,
+                 test_dir: str = "/var/tmp",
+                 socket_scm_helper: Optional[str] = None,
+                 sock_dir: Optional[str] = None):
         if name is None:
             name = "qemu-%d" % os.getpid()
         if sock_dir is None:
@@ -118,8 +124,10 @@ def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
     @property
     def _base_args(self) -> List[str]:
         args = super()._base_args
-        args.extend(['-qtest', 'unix:path=' + self._qtest_path,
-                     '-accel', 'qtest'])
+        args.extend([
+            '-qtest', f"unix:path={self._qtest_path}",
+            '-accel', 'qtest'
+        ])
         return args
 
     def _pre_launch(self):
-- 
2.21.1

[PATCH RFC 31/32] python/qemu: add mypy to Pipfile

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/Pipfile      |  1 +
 python/Pipfile.lock | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/python/Pipfile b/python/Pipfile
index e396e56e06..80bd6c3188 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -6,6 +6,7 @@ verify_ssl = true
 [dev-packages]
 pylint = "==2.5.0"
 flake8 = "*"
+mypy = "*"
 
 [packages]
 
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 15cd8a918f..085f23fe8a 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
     "_meta": {
         "hash": {
-            "sha256": "58115144ace0f646b5c62da260fb4867ac0a0e485de3f5b0a56c7854afa21f5b"
+            "sha256": "17a30ead8719d80349ff5473bda3133fd4559a4e83d3f9d669631ac4b2171502"
         },
         "pipfile-spec": 6,
         "requires": {
@@ -79,6 +79,33 @@
             ],
             "version": "==0.6.1"
         },
+        "mypy": {
+            "hashes": [
+                "sha256:15b948e1302682e3682f11f50208b726a246ab4e6c1b39f9264a8796bb416aa2",
+                "sha256:219a3116ecd015f8dca7b5d2c366c973509dfb9a8fc97ef044a36e3da66144a1",
+                "sha256:3b1fc683fb204c6b4403a1ef23f0b1fac8e4477091585e0c8c54cbdf7d7bb164",
+                "sha256:3beff56b453b6ef94ecb2996bea101a08f1f8a9771d3cbf4988a61e4d9973761",
+                "sha256:7687f6455ec3ed7649d1ae574136835a4272b65b3ddcf01ab8704ac65616c5ce",
+                "sha256:7ec45a70d40ede1ec7ad7f95b3c94c9cf4c186a32f6bacb1795b60abd2f9ef27",
+                "sha256:86c857510a9b7c3104cf4cde1568f4921762c8f9842e987bc03ed4f160925754",
+                "sha256:8a627507ef9b307b46a1fea9513d5c98680ba09591253082b4c48697ba05a4ae",
+                "sha256:8dfb69fbf9f3aeed18afffb15e319ca7f8da9642336348ddd6cab2713ddcf8f9",
+                "sha256:a34b577cdf6313bf24755f7a0e3f3c326d5c1f4fe7422d1d06498eb25ad0c600",
+                "sha256:a8ffcd53cb5dfc131850851cc09f1c44689c2812d0beb954d8138d4f5fc17f65",
+                "sha256:b90928f2d9eb2f33162405f32dde9f6dcead63a0971ca8a1b50eb4ca3e35ceb8",
+                "sha256:c56ffe22faa2e51054c5f7a3bc70a370939c2ed4de308c690e7949230c995913",
+                "sha256:f91c7ae919bbc3f96cd5e5b2e786b2b108343d1d7972ea130f7de27fdd547cf3"
+            ],
+            "index": "pypi",
+            "version": "==0.770"
+        },
+        "mypy-extensions": {
+            "hashes": [
+                "sha256:090fedd75945a69ae91ce1303b5824f428daf5a028d2f6ab8a299250a846f15d",
+                "sha256:2d82818f5bb3e369420cb3c4060a7970edba416647068eb4c5343488a6c604a8"
+            ],
+            "version": "==0.4.3"
+        },
         "pycodestyle": {
             "hashes": [
                 "sha256:95a2219d12372f05704562a14ec30bc76b05a5b297b21a5dfe3f6fac3491ae56",
@@ -142,6 +169,14 @@
             "markers": "implementation_name == 'cpython' and python_version < '3.8'",
             "version": "==1.4.1"
         },
+        "typing-extensions": {
+            "hashes": [
+                "sha256:6e95524d8a547a91e08f404ae485bbb71962de46967e1b71a0cb89af24e761c5",
+                "sha256:79ee589a3caca649a9bfd2a8de4709837400dfa00b6cc81962a1e6a1815969ae",
+                "sha256:f8d2bd89d25bc39dabe7d23df520442fa1d8969b82544370e03d88b5a591c392"
+            ],
+            "version": "==3.7.4.2"
+        },
         "wrapt": {
             "hashes": [
                 "sha256:b62ffa81fb85f4332a4f609cab4ac40709470da05643a082ec1eb88e6d9b97d7"
-- 
2.21.1

[PATCH RFC 32/32] python/qemu/lib: Add mypy type annotations

[John Snow]

These should all be purely annotations with no changes in behavior at
all.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/lib/accel.py   |  8 ++--
 python/qemu/lib/machine.py | 94 +++++++++++++++++++++-----------------
 python/qemu/lib/qmp.py     | 44 ++++++++++--------
 python/qemu/lib/qtest.py   | 27 ++++++-----
 4 files changed, 99 insertions(+), 74 deletions(-)

diff --git a/python/qemu/lib/accel.py b/python/qemu/lib/accel.py
index 7fabe62920..4325114e51 100644
--- a/python/qemu/lib/accel.py
+++ b/python/qemu/lib/accel.py
@@ -17,6 +17,7 @@
 import logging
 import os
 import subprocess
+from typing import List, Optional
 
 LOG = logging.getLogger(__name__)
 
@@ -29,7 +30,7 @@
 }
 
 
-def list_accel(qemu_bin):
+def list_accel(qemu_bin: str) -> List[str]:
     """
     List accelerators enabled in the QEMU binary.
 
@@ -49,7 +50,8 @@ def list_accel(qemu_bin):
     return [acc.strip() for acc in out.splitlines()[1:]]
 
 
-def kvm_available(target_arch=None, qemu_bin=None):
+def kvm_available(target_arch: Optional[str] = None,
+                  qemu_bin: Optional[str] = None) -> bool:
     """
     Check if KVM is available using the following heuristic:
       - Kernel module is present in the host;
@@ -72,7 +74,7 @@ def kvm_available(target_arch=None, qemu_bin=None):
     return True
 
 
-def tcg_available(qemu_bin):
+def tcg_available(qemu_bin: str) -> bool:
     """
     Check if TCG is available.
 
diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
index ec2bb28b86..c84db2bf8f 100644
--- a/python/qemu/lib/machine.py
+++ b/python/qemu/lib/machine.py
@@ -27,6 +27,7 @@
 import tempfile
 from typing import (
     Any,
+    BinaryIO,
     Dict,
     List,
     Optional,
@@ -37,7 +38,7 @@
 from types import TracebackType
 
 from . import qmp
-from .qmp import SocketAddrT, QMPMessage
+from .qmp import QMPMessage, QMPReturnValue, SocketAddrT
 
 LOG = logging.getLogger(__name__)
 
@@ -60,7 +61,7 @@ class QEMUMachineAddDeviceError(QEMUMachineError):
 
 class QEMUMachine:
     """
-    A QEMU VM
+    A QEMU VM.
 
     Use this object as a context manager to ensure
     the QEMU process terminates::
@@ -77,7 +78,8 @@ def __init__(self,
                  name: Optional[str] = None,
                  test_dir: str = "/var/tmp",
                  monitor_address: Optional[SocketAddrT] = None,
-                 socket_scm_helper=None, sock_dir=None):
+                 socket_scm_helper: Optional[str] = None,
+                 sock_dir: Optional[str] = None):
         '''
         Initialize a QEMUMachine
 
@@ -111,27 +113,27 @@ def __init__(self,
             self._remove_monitor_sockfile = True
 
         # Runstate
-        self._qemu_log_path = None
-        self._qemu_log_file = None
+        self._qemu_log_path: Optional[str] = None
+        self._qemu_log_file: Optional[BinaryIO] = None
         self._popen: Optional['subprocess.Popen[bytes]'] = None
-        self._events = []
-        self._iolog = None
+        self._events: List[QMPMessage] = []
+        self._iolog: Optional[str] = None
         self._qmp_set = True   # Enable QMP monitor by default.
         self._qmp_connection: Optional[qmp.QEMUMonitorProtocol] = None
         self._qemu_full_args: Tuple[str, ...] = ()
-        self._temp_dir = None
+        self._temp_dir: Optional[str] = None
         self._launched = False
-        self._machine = None
+        self._machine: Optional[str] = None
         self._console_index = 0
         self._console_set = False
-        self._console_device_type = None
+        self._console_device_type: Optional[str] = None
         self._console_address = os.path.join(
             self._sock_dir, f"{self._name}-console.sock"
         )
-        self._console_socket = None
-        self._remove_files = []
+        self._console_socket: Optional[socket.socket] = None
+        self._remove_files: List[str] = []
 
-    def __enter__(self):
+    def __enter__(self) -> 'QEMUMachine':
         return self
 
     def __exit__(self,
@@ -140,14 +142,15 @@ def __exit__(self,
                  exc_tb: Optional[TracebackType]) -> None:
         self.shutdown()
 
-    def add_monitor_null(self):
+    def add_monitor_null(self) -> None:
         """
         This can be used to add an unused monitor instance.
         """
         self._args.append('-monitor')
         self._args.append('null')
 
-    def add_fd(self, fd, fdset, opaque, opts=''):
+    def add_fd(self, fd: int, fdset: int,
+               opaque: str, opts: str = '') -> 'QEMUMachine':
         """
         Pass a file descriptor to the VM
         """
@@ -166,7 +169,8 @@ def add_fd(self, fd, fdset, opaque, opts=''):
         self._args.append(','.join(options))
         return self
 
-    def send_fd_scm(self, fd=None, file_path=None):
+    def send_fd_scm(self, fd: Optional[int] = None,
+                    file_path: Optional[str] = None) -> int:
         """
         Send an fd or file_path to socket_scm_helper.
 
@@ -210,7 +214,7 @@ def send_fd_scm(self, fd=None, file_path=None):
         return proc.returncode
 
     @staticmethod
-    def _remove_if_exists(path):
+    def _remove_if_exists(path: str) -> None:
         """
         Remove file object at path if it exists
         """
@@ -221,7 +225,7 @@ def _remove_if_exists(path):
                 return
             raise
 
-    def is_running(self):
+    def is_running(self) -> bool:
         """Returns true if the VM is running."""
         return self._popen is not None and self._popen.poll() is None
 
@@ -231,19 +235,19 @@ def _subp(self) -> 'subprocess.Popen[bytes]':
             raise QEMUMachineError('Subprocess pipe not present')
         return self._popen
 
-    def exitcode(self):
+    def exitcode(self) -> Optional[int]:
         """Returns the exit code if possible, or None."""
         if self._popen is None:
             return None
         return self._popen.poll()
 
-    def get_pid(self):
+    def get_pid(self) -> Optional[int]:
         """Returns the PID of the running process, or None."""
         if not self.is_running():
             return None
         return self._subp.pid
 
-    def _load_io_log(self):
+    def _load_io_log(self) -> None:
         if self._qemu_log_path is not None:
             with open(self._qemu_log_path, "r") as iolog:
                 self._iolog = iolog.read()
@@ -277,7 +281,7 @@ def _base_args(self) -> List[str]:
                 args.extend(['-device', device])
         return args
 
-    def _pre_launch(self):
+    def _pre_launch(self) -> None:
         self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
         self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
         self._qemu_log_file = open(self._qemu_log_path, 'wb')
@@ -295,11 +299,11 @@ def _pre_launch(self):
                 nickname=self._name
             )
 
-    def _post_launch(self):
+    def _post_launch(self) -> None:
         if self._qmp:
             self._qmp.accept()
 
-    def _post_shutdown(self):
+    def _post_shutdown(self) -> None:
         if self._qemu_log_file is not None:
             self._qemu_log_file.close()
             self._qemu_log_file = None
@@ -313,7 +317,7 @@ def _post_shutdown(self):
         while len(self._remove_files) > 0:
             self._remove_if_exists(self._remove_files.pop())
 
-    def launch(self):
+    def launch(self) -> None:
         """
         Launch the VM and make sure we cleanup and expose the
         command line/output in case of exception
@@ -337,7 +341,7 @@ def launch(self):
                 LOG.debug('Output: %r', self._iolog)
             raise
 
-    def _launch(self):
+    def _launch(self) -> None:
         """
         Launch the VM and establish a QMP connection
         """
@@ -358,7 +362,7 @@ def _launch(self):
                                        close_fds=False)
         self._post_launch()
 
-    def wait(self):
+    def wait(self) -> None:
         """
         Wait for the VM to power off
         """
@@ -413,7 +417,7 @@ def shutdown(self, has_quit: bool = False) -> None:
 
         self._launched = False
 
-    def set_qmp_monitor(self, enabled=True):
+    def set_qmp_monitor(self, enabled: bool = True) -> None:
         """
         Set the QMP monitor.
 
@@ -453,7 +457,9 @@ def qmp(self, cmd: str,
         qmp_args = self._qmp_args(conv_keys, **args)
         return self._qmp.cmd(cmd, args=qmp_args)
 
-    def command(self, cmd, conv_keys=True, **args):
+    def command(self, cmd: str,
+                conv_keys: bool = True,
+                **args: Any) -> QMPReturnValue:
         """
         Invoke a QMP command.
         On success return the response dict.
@@ -462,7 +468,7 @@ def command(self, cmd, conv_keys=True, **args):
         qmp_args = self._qmp_args(conv_keys, **args)
         return self._qmp.command(cmd, **qmp_args)
 
-    def get_qmp_event(self, wait=False):
+    def get_qmp_event(self, wait: bool = False) -> Optional[QMPMessage]:
         """
         Poll for one queued QMP events and return it
         """
@@ -470,7 +476,7 @@ def get_qmp_event(self, wait=False):
             return self._events.pop(0)
         return self._qmp.pull_event(wait=wait)
 
-    def get_qmp_events(self, wait=False):
+    def get_qmp_events(self, wait: bool = False) -> List[QMPMessage]:
         """
         Poll for queued QMP events and return a list of dicts
         """
@@ -481,7 +487,7 @@ def get_qmp_events(self, wait=False):
         return events
 
     @staticmethod
-    def event_match(event, match=None):
+    def event_match(event: Any, match: Optional[Any]) -> bool:
         """
         Check if an event matches optional match criteria.
 
@@ -511,9 +517,11 @@ def event_match(event, match=None):
             return True
         except TypeError:
             # either match or event wasn't iterable (not a dict)
-            return match == event
+            return bool(match == event)
 
-    def event_wait(self, name, timeout=60.0, match=None):
+    def event_wait(self, name: str,
+                   timeout: float = 60.0,
+                   match: Optional[QMPMessage] = None) -> Optional[QMPMessage]:
         """
         event_wait waits for and returns a named event from QMP with a timeout.
 
@@ -523,7 +531,9 @@ def event_wait(self, name, timeout=60.0, match=None):
         """
         return self.events_wait([(name, match)], timeout)
 
-    def events_wait(self, events, timeout=60.0):
+    def events_wait(self,
+                    events: Sequence[Tuple[str, Any]],
+                    timeout: float = 60.0) -> Optional[QMPMessage]:
         """
         events_wait waits for and returns a named event
         from QMP with a timeout.
@@ -533,7 +543,7 @@ def events_wait(self, events, timeout=60.0):
                 See event_match for details.
         timeout: QEMUMonitorProtocol.pull_event timeout parameter.
         """
-        def _match(event):
+        def _match(event: QMPMessage) -> bool:
             for name, match in events:
                 if event['event'] == name and self.event_match(event, match):
                     return True
@@ -558,20 +568,20 @@ def _match(event):
 
         return None
 
-    def get_log(self):
+    def get_log(self) -> Optional[str]:
         """
         After self.shutdown or failed qemu execution, this returns the output
         of the qemu process.
         """
         return self._iolog
 
-    def add_args(self, *args):
+    def add_args(self, *args: str) -> None:
         """
         Adds to the list of extra arguments to be given to the QEMU binary
         """
         self._args.extend(args)
 
-    def set_machine(self, machine_type):
+    def set_machine(self, machine_type: str) -> None:
         """
         Sets the machine type
 
@@ -580,7 +590,9 @@ def set_machine(self, machine_type):
         """
         self._machine = machine_type
 
-    def set_console(self, device_type=None, console_index=0):
+    def set_console(self,
+                    device_type: Optional[str] = None,
+                    console_index: int = 0) -> None:
         """
         Sets the device type for a console device
 
@@ -611,7 +623,7 @@ def set_console(self, device_type=None, console_index=0):
         self._console_index = console_index
 
     @property
-    def console_socket(self):
+    def console_socket(self) -> socket.socket:
         """
         Returns a socket connected to the console
         """
diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
index 1aefc00c93..1ae535ddb3 100644
--- a/python/qemu/lib/qmp.py
+++ b/python/qemu/lib/qmp.py
@@ -15,6 +15,7 @@
     Any,
     cast,
     Dict,
+    List,
     Optional,
     TextIO,
     Type,
@@ -90,7 +91,9 @@ class QEMUMonitorProtocol:
     #: Logger object for debugging messages
     logger = logging.getLogger('QMP')
 
-    def __init__(self, address, server=False, nickname=None):
+    def __init__(self, address: SocketAddrT,
+                 server: bool = False,
+                 nickname: Optional[str] = None):
         """
         Create a QEMUMonitorProtocol class.
 
@@ -102,7 +105,7 @@ def __init__(self, address, server=False, nickname=None):
         @note No connection is established, this is done by the connect() or
               accept() methods
         """
-        self.__events = []
+        self.__events: List[QMPMessage] = []
         self.__address = address
         self.__sock = self.__get_sock()
         self.__sockfile: Optional[TextIO] = None
@@ -114,14 +117,14 @@ def __init__(self, address, server=False, nickname=None):
             self.__sock.bind(self.__address)
             self.__sock.listen(1)
 
-    def __get_sock(self):
+    def __get_sock(self) -> socket.socket:
         if isinstance(self.__address, tuple):
             family = socket.AF_INET
         else:
             family = socket.AF_UNIX
         return socket.socket(family, socket.SOCK_STREAM)
 
-    def __negotiate_capabilities(self):
+    def __negotiate_capabilities(self) -> QMPMessage:
         greeting = self.__json_read()
         if greeting is None or "QMP" not in greeting:
             raise QMPConnectError
@@ -131,7 +134,7 @@ def __negotiate_capabilities(self):
             return greeting
         raise QMPCapabilitiesError
 
-    def __json_read(self, only_event=False):
+    def __json_read(self, only_event: bool = False) -> Optional[QMPMessage]:
         assert self.__sockfile is not None
         while True:
             data = self.__sockfile.readline()
@@ -148,7 +151,7 @@ def __json_read(self, only_event=False):
                     continue
             return resp
 
-    def __get_events(self, wait=False):
+    def __get_events(self, wait: Union[bool, float] = False) -> None:
         """
         Check for new events in the stream and cache them in __events.
 
@@ -186,7 +189,7 @@ def __get_events(self, wait=False):
                 raise QMPConnectError("Error while reading from socket")
             self.__sock.settimeout(None)
 
-    def __enter__(self):
+    def __enter__(self) -> 'QEMUMonitorProtocol':
         # Implement context manager enter function.
         return self
 
@@ -199,7 +202,7 @@ def __exit__(self,
         # Implement context manager exit function.
         self.close()
 
-    def connect(self, negotiate=True):
+    def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
         """
         Connect to the QMP Monitor and perform capabilities negotiation.
 
@@ -214,7 +217,7 @@ def connect(self, negotiate=True):
             return self.__negotiate_capabilities()
         return None
 
-    def accept(self, timeout=15.0):
+    def accept(self, timeout: float = 15.0) -> QMPMessage:
         """
         Await connection from QMP Monitor and perform capabilities negotiation.
 
@@ -250,7 +253,9 @@ def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
         self.logger.debug("<<< %s", resp)
         return resp
 
-    def cmd(self, name, args=None, cmd_id=None):
+    def cmd(self, name: str,
+            args: Optional[Dict[str, Any]] = None,
+            cmd_id: Optional[Any] = None) -> QMPMessage:
         """
         Build a QMP command and send it to the QMP Monitor.
 
@@ -258,14 +263,14 @@ def cmd(self, name, args=None, cmd_id=None):
         @param args: command arguments (dict)
         @param cmd_id: command id (dict, list, string or int)
         """
-        qmp_cmd = {'execute': name}
+        qmp_cmd: QMPMessage = {'execute': name}
         if args:
             qmp_cmd['arguments'] = args
         if cmd_id:
             qmp_cmd['id'] = cmd_id
         return self.cmd_obj(qmp_cmd)
 
-    def command(self, cmd, **kwds):
+    def command(self, cmd: str, **kwds: Any) -> QMPReturnValue:
         """
         Build and send a QMP command to the monitor, report errors if any
         """
@@ -278,7 +283,8 @@ def command(self, cmd, **kwds):
             )
         return cast(QMPReturnValue, ret['return'])
 
-    def pull_event(self, wait=False):
+    def pull_event(self,
+                   wait: Union[bool, float] = False) -> Optional[QMPMessage]:
         """
         Pulls a single event.
 
@@ -298,7 +304,7 @@ def pull_event(self, wait=False):
             return self.__events.pop(0)
         return None
 
-    def get_events(self, wait=False):
+    def get_events(self, wait: bool = False) -> List[QMPMessage]:
         """
         Get a list of available QMP events.
 
@@ -315,13 +321,13 @@ def get_events(self, wait=False):
         self.__get_events(wait)
         return self.__events
 
-    def clear_events(self):
+    def clear_events(self) -> None:
         """
         Clear current list of pending events.
         """
         self.__events = []
 
-    def close(self):
+    def close(self) -> None:
         """
         Close the socket and socket file.
         """
@@ -330,7 +336,7 @@ def close(self):
         if self.__sockfile:
             self.__sockfile.close()
 
-    def settimeout(self, timeout):
+    def settimeout(self, timeout: float) -> None:
         """
         Set the socket timeout.
 
@@ -339,7 +345,7 @@ def settimeout(self, timeout):
         """
         self.__sock.settimeout(timeout)
 
-    def get_sock_fd(self):
+    def get_sock_fd(self) -> int:
         """
         Get the socket file descriptor.
 
@@ -347,7 +353,7 @@ def get_sock_fd(self):
         """
         return self.__sock.fileno()
 
-    def is_scm_available(self):
+    def is_scm_available(self) -> bool:
         """
         Check if the socket allows for SCM_RIGHTS.
 
diff --git a/python/qemu/lib/qtest.py b/python/qemu/lib/qtest.py
index ae4661d4d3..df6300cd4f 100644
--- a/python/qemu/lib/qtest.py
+++ b/python/qemu/lib/qtest.py
@@ -24,6 +24,8 @@
     Optional,
     Sequence,
     TextIO,
+    Tuple,
+    Union,
 )
 
 from .machine import QEMUMachine
@@ -43,7 +45,8 @@ class QEMUQtestProtocol:
        No conection is estabalished by __init__(), this is done
        by the connect() or accept() methods.
     """
-    def __init__(self, address, server=False):
+    def __init__(self, address: Union[Tuple[str, str], str],
+                 server: bool = False):
         self._address = address
         self._sock = self._get_sock()
         self._sockfile: Optional[TextIO] = None
@@ -51,14 +54,14 @@ def __init__(self, address, server=False):
             self._sock.bind(self._address)
             self._sock.listen(1)
 
-    def _get_sock(self):
+    def _get_sock(self) -> socket.socket:
         if isinstance(self._address, tuple):
             family = socket.AF_INET
         else:
             family = socket.AF_UNIX
         return socket.socket(family, socket.SOCK_STREAM)
 
-    def connect(self):
+    def connect(self) -> None:
         """
         Connect to the qtest socket.
 
@@ -67,7 +70,7 @@ def connect(self):
         self._sock.connect(self._address)
         self._sockfile = self._sock.makefile(mode='r')
 
-    def accept(self):
+    def accept(self) -> None:
         """
         Await connection from QEMU.
 
@@ -76,7 +79,7 @@ def accept(self):
         self._sock, _ = self._sock.accept()
         self._sockfile = self._sock.makefile(mode='r')
 
-    def cmd(self, qtest_cmd):
+    def cmd(self, qtest_cmd: str) -> str:
         """
         Send a qtest command on the wire.
 
@@ -87,14 +90,16 @@ def cmd(self, qtest_cmd):
         resp = self._sockfile.readline()
         return resp
 
-    def close(self):
-        """Close this socket."""
+    def close(self) -> None:
+        """
+        Close this socket.
+        """
         self._sock.close()
         if self._sockfile:
             self._sockfile.close()
             self._sockfile = None
 
-    def settimeout(self, timeout):
+    def settimeout(self, timeout: Optional[float]) -> None:
         """Set a timeout, in seconds."""
         self._sock.settimeout(timeout)
 
@@ -118,7 +123,7 @@ def __init__(self,
         super().__init__(binary, args, name=name, test_dir=test_dir,
                          socket_scm_helper=socket_scm_helper,
                          sock_dir=sock_dir)
-        self._qtest = None
+        self._qtest: Optional[QEMUQtestProtocol] = None
         self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
 
     @property
@@ -130,7 +135,7 @@ def _base_args(self) -> List[str]:
         ])
         return args
 
-    def _pre_launch(self):
+    def _pre_launch(self) -> None:
         super()._pre_launch()
         self._qtest = QEMUQtestProtocol(self._qtest_path, server=True)
 
@@ -139,7 +144,7 @@ def _post_launch(self) -> None:
         super()._post_launch()
         self._qtest.accept()
 
-    def _post_shutdown(self):
+    def _post_shutdown(self) -> None:
         super()._post_shutdown()
         self._remove_if_exists(self._qtest_path)
 
-- 
2.21.1

Re: [PATCH RFC 11/32] python/qemu/lib: remove Python2 style super() calls

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> Use the Python3 style instead.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   python/qemu/lib/machine.py |  2 +-
>   python/qemu/lib/qtest.py   | 15 +++++++--------
>   2 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
> index 4b260fa2cb..b2f0412197 100644
> --- a/python/qemu/lib/machine.py
> +++ b/python/qemu/lib/machine.py
> @@ -55,7 +55,7 @@ def __init__(self, reply):
>               desc = reply["error"]["desc"]
>           except KeyError:
>               desc = reply
> -        super(MonitorResponseError, self).__init__(desc)
> +        super().__init__(desc)
>           self.reply = reply
>   
>   
> diff --git a/python/qemu/lib/qtest.py b/python/qemu/lib/qtest.py
> index 53d814c064..7943487c2b 100644
> --- a/python/qemu/lib/qtest.py
> +++ b/python/qemu/lib/qtest.py
> @@ -101,29 +101,28 @@ def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
>               name = "qemu-%d" % os.getpid()
>           if sock_dir is None:
>               sock_dir = test_dir
> -        super(QEMUQtestMachine,
> -              self).__init__(binary, args, name=name, test_dir=test_dir,
> -                             socket_scm_helper=socket_scm_helper,
> -                             sock_dir=sock_dir)
> +        super().__init__(binary, args, name=name, test_dir=test_dir,
> +                         socket_scm_helper=socket_scm_helper,
> +                         sock_dir=sock_dir)
>           self._qtest = None
>           self._qtest_path = os.path.join(sock_dir, name + "-qtest.sock")
>   
>       def _base_args(self):
> -        args = super(QEMUQtestMachine, self)._base_args()
> +        args = super()._base_args()
>           args.extend(['-qtest', 'unix:path=' + self._qtest_path,
>                        '-accel', 'qtest'])
>           return args
>   
>       def _pre_launch(self):
> -        super(QEMUQtestMachine, self)._pre_launch()
> +        super()._pre_launch()
>           self._qtest = QEMUQtestProtocol(self._qtest_path, server=True)
>   
>       def _post_launch(self):
> -        super(QEMUQtestMachine, self)._post_launch()
> +        super()._post_launch()
>           self._qtest.accept()
>   
>       def _post_shutdown(self):
> -        super(QEMUQtestMachine, self)._post_shutdown()
> +        super()._post_shutdown()
>           self._remove_if_exists(self._qtest_path)
>   
>       def qtest(self, cmd):
> 

Re: [PATCH RFC 14/32] python//qmp.py: use True/False for non/blocking modes

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> The type system doesn't want integers.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/lib/qmp.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
> index b91c9d5c1c..a634c4e26c 100644
> --- a/python/qemu/lib/qmp.py
> +++ b/python/qemu/lib/qmp.py
> @@ -120,14 +120,14 @@ def __get_events(self, wait=False):
>           """
>   
>           # Check for new events regardless and pull them into the cache:
> -        self.__sock.setblocking(0)
> +        self.__sock.setblocking(False)
>           try:
>               self.__json_read()
>           except OSError as err:
>               if err.errno == errno.EAGAIN:
>                   # No data available
>                   pass
> -        self.__sock.setblocking(1)
> +        self.__sock.setblocking(True)
>   
>           # Wait for new events, if needed.
>           # if wait is 0.0, this means "no wait" and is also implicitly false.
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH RFC 16/32] python//qmp.py: re-absorb MonitorResponseError

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> When I initially split this out, I considered this more of a machine
> error than a QMP protocol error, but I think that's misguided.
> 
> Move this back to qmp.py and name it QMPResponseError. Convert
> qmp.command() to use this exception type.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   python/qemu/lib/machine.py    | 15 +--------------
>   python/qemu/lib/qmp.py        | 17 +++++++++++++++--
>   scripts/render_block_graph.py |  4 ++--
>   3 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
> index 2f94c851ed..c31bf7cabb 100644
> --- a/python/qemu/lib/machine.py
> +++ b/python/qemu/lib/machine.py
> @@ -48,19 +48,6 @@ class QEMUMachineAddDeviceError(QEMUMachineError):
>       """
>   
>   
> -class MonitorResponseError(qmp.QMPError):
> -    """
> -    Represents erroneous QMP monitor reply
> -    """
> -    def __init__(self, reply):
> -        try:
> -            desc = reply["error"]["desc"]
> -        except KeyError:
> -            desc = reply
> -        super().__init__(desc)
> -        self.reply = reply
> -
> -
>   class QEMUMachine:
>       """
>       A QEMU VM
> @@ -433,7 +420,7 @@ def command(self, cmd, conv_keys=True, **args):
>           if reply is None:
>               raise qmp.QMPError("Monitor is closed")
>           if "error" in reply:
> -            raise MonitorResponseError(reply)
> +            raise qmp.QMPResponseError(reply)
>           return reply["return"]
>   
>       def get_qmp_event(self, wait=False):
> diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
> index 911da59888..82f86b4e45 100644
> --- a/python/qemu/lib/qmp.py
> +++ b/python/qemu/lib/qmp.py
> @@ -61,6 +61,19 @@ class QMPTimeoutError(QMPError):
>       """
>   
>   
> +class QMPResponseError(QMPError):
> +    """
> +    Represents erroneous QMP monitor reply
> +    """
> +    def __init__(self, reply: QMPMessage):
> +        try:
> +            desc = reply['error']['desc']
> +        except KeyError:
> +            desc = reply
> +        super().__init__(desc)
> +        self.reply = reply
> +
> +
>   class QEMUMonitorProtocol:
>       """
>       Provide an API to connect to QEMU via QEMU Monitor Protocol (QMP) and then
> @@ -250,8 +263,8 @@ def command(self, cmd, **kwds):
>           Build and send a QMP command to the monitor, report errors if any
>           """
>           ret = self.cmd(cmd, kwds)
> -        if "error" in ret:
> -            raise Exception(ret['error']['desc'])
> +        if 'error' in ret:
> +            raise QMPResponseError(ret)
>           return ret['return']
>   
>       def pull_event(self, wait=False):
> diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
> index 8048d9fbbe..332ab49a91 100755
> --- a/scripts/render_block_graph.py
> +++ b/scripts/render_block_graph.py
> @@ -26,7 +26,7 @@
>   
>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'python'))
>   from qemu.lib import QEMUMonitorProtocol
> -from qemu.lib.machine import MonitorResponseError
> +from qemu.lib.qmp import QMPResponseError
>   
>   
>   def perm(arr):
> @@ -103,7 +103,7 @@ def command(self, cmd):
>           reply = json.loads(subprocess.check_output(ar))
>   
>           if 'error' in reply:
> -            raise MonitorResponseError(reply)
> +            raise QEMUResponseError(reply)
>   
>           return reply['return']
>   
> 

Re: [PATCH RFC 19/32] python//qmp.py: add QMPProtocolError

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> In the case that we receive a reply but are unable to understand it, use
> this exception name to indicate that case.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/lib/qmp.py | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
> index e460234f2e..5fb16f4b42 100644
> --- a/python/qemu/lib/qmp.py
> +++ b/python/qemu/lib/qmp.py
> @@ -62,6 +62,12 @@ class QMPTimeoutError(QMPError):
>       """
>   
>   
> +class QMPProtocolError(QMPError):
> +    """
> +    QMP protocol error; unexpected response
> +    """
> +
> +
>   class QMPResponseError(QMPError):
>       """
>       Represents erroneous QMP monitor reply
> @@ -265,6 +271,10 @@ def command(self, cmd, **kwds):
>           ret = self.cmd(cmd, kwds)
>           if 'error' in ret:
>               raise QMPResponseError(ret)
> +        if 'return' not in ret:
> +            raise QMPProtocolError(
> +                "'return' key not found in QMP response '{}'".format(str(ret))
> +            )
>           return cast(QMPReturnValue, ret['return'])
>   
>       def pull_event(self, wait=False):
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH RFC 21/32] python//machine.py: remove logging configuration

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> Python 3.5 and above do not print a warning when logging is not
> configured. As a library, it's best practice to leave logging
> configuration to the client executable.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/lib/machine.py | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
> index c31bf7cabb..e92afe8649 100644
> --- a/python/qemu/lib/machine.py
> +++ b/python/qemu/lib/machine.py
> @@ -110,9 +110,6 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
>           self._console_socket = None
>           self._remove_files = []
>   
> -        # just in case logging wasn't configured by the main script:
> -        logging.basicConfig()
> -
>       def __enter__(self):
>           return self
>   
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH RFC 23/32] python//machine.py: reorder __init__

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> Put the init arg handling all at the top, and mostly in order (deviating
> when one is dependent on another), and put what is effectively runtime
> state declaration at the bottom.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/lib/machine.py | 29 +++++++++++++++++------------
>   1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
> index 6a4aea7725..beb31be453 100644
> --- a/python/qemu/lib/machine.py
> +++ b/python/qemu/lib/machine.py
> @@ -80,38 +80,43 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
>           @param socket_scm_helper: helper program, required for send_fd_scm()
>           @note: Qemu process is not started until launch() is used.
>           '''
> +        # Direct user configuration
> +
> +        self._binary = binary
> +
>           if args is None:
>               args = []
> +        # Copy mutable input: we will be modifying our copy
> +        self._args = list(args)
> +
>           if wrapper is None:
>               wrapper = []
> -        if name is None:
> -            name = "qemu-%d" % os.getpid()
> -        if sock_dir is None:
> -            sock_dir = test_dir
> -        self._name = name
> +        self._wrapper = wrapper
> +
> +        self._name = name or "qemu-%d" % os.getpid()
> +        self._test_dir = test_dir
> +        self._sock_dir = sock_dir or self._test_dir
> +        self._socket_scm_helper = socket_scm_helper
> +
>           if monitor_address is not None:
>               self._monitor_address = monitor_address
>               self._remove_monitor_sockfile = False
>           else:
>               self._monitor_address = os.path.join(
> -                sock_dir, f"{name}-monitor.sock"
> +                self._sock_dir, f"{self._name}-monitor.sock"
>               )
>               self._remove_monitor_sockfile = True
> +
> +        # Runstate
>           self._qemu_log_path = None
>           self._qemu_log_file = None
>           self._popen = None
> -        self._binary = binary
> -        self._args = list(args)     # Force copy args in case we modify them
> -        self._wrapper = wrapper
>           self._events = []
>           self._iolog = None
> -        self._socket_scm_helper = socket_scm_helper
>           self._qmp_set = True   # Enable QMP monitor by default.
>           self._qmp = None
>           self._qemu_full_args = None
> -        self._test_dir = test_dir
>           self._temp_dir = None
> -        self._sock_dir = sock_dir
>           self._launched = False
>           self._machine = None
>           self._console_index = 0
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH RFC 29/32] python//qtest.py: Check before accessing _qtest

[Philippe Mathieu-Daudé]

On 5/14/20 7:54 AM, John Snow wrote:
> It can be None; so add assertions or exceptions where appropriate to
> guard the access accordingly.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/lib/qtest.py | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/python/qemu/lib/qtest.py b/python/qemu/lib/qtest.py
> index a8be0c782f..05c63a1d58 100644
> --- a/python/qemu/lib/qtest.py
> +++ b/python/qemu/lib/qtest.py
> @@ -126,7 +126,8 @@ def _pre_launch(self):
>           super()._pre_launch()
>           self._qtest = QEMUQtestProtocol(self._qtest_path, server=True)
>   
> -    def _post_launch(self):
> +    def _post_launch(self) -> None:
> +        assert self._qtest is not None
>           super()._post_launch()
>           self._qtest.accept()
>   
> @@ -134,6 +135,13 @@ def _post_shutdown(self):
>           super()._post_shutdown()
>           self._remove_if_exists(self._qtest_path)
>   
> -    def qtest(self, cmd):
> -        '''Send a qtest command to guest'''
> +    def qtest(self, cmd: str) -> str:
> +        """
> +        Send a qtest command to the guest.
> +
> +        :param cmd: qtest command to send
> +        :return: qtest server response
> +        """
> +        if self._qtest is None:
> +            raise RuntimeError("qtest socket not available")
>           return self._qtest.cmd(cmd)
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH RFC 03/32] python//machine.py: remove bare except

[Eric Blake]

On 5/14/20 12:53 AM, John Snow wrote:
> Catch only the timeout error; if there are other problems, allow the
> stack trace to be visible.
> 

A lot of patches in this series start with "python//" - is that 
intentional, or should it be a single slash?

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/qemu/lib/machine.py | 33 +++++++++++++++++++++------------
>   1 file changed, 21 insertions(+), 12 deletions(-)
> 


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH RFC 03/32] python//machine.py: remove bare except

[John Snow]

On 5/14/20 9:55 AM, Eric Blake wrote:
> On 5/14/20 12:53 AM, John Snow wrote:
>> Catch only the timeout error; if there are other problems, allow the
>> stack trace to be visible.
>>
> 
> A lot of patches in this series start with "python//" - is that
> intentional, or should it be a single slash?
> 

Was trying to imply an elided path structure, because
"python/qemu/lib/qmp.py" et al is way too chatty.

Feel free to suggest better subject names!

Re: [PATCH RFC 00/32] python/qemu: refactor as installable package

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:

>    python//qmp.py: use True/False for non/blocking modes
>    python//qmp.py: Define common types
>    python//qmp.py: re-absorb MonitorResponseError
>    python//qmp.py: Do not return None from cmd_obj
>    python//qmp.py: add casts to JSON deserialization
>    python//qmp.py: add QMPProtocolError
>    python//qmp.py: assert sockfile is not None
>    python//machine.py: remove logging configuration
>    python//machine.py: Fix monitor address typing
>    python//machine.py: reorder __init__
>    python//machine.py: Don't modify state in _base_args()
>    python//machine.py: Handle None events in event_wait
>    python//machine.py: use qmp.command
>    python//machine.py: Add _qmp access shim
>    python//machine.py: fix _popen access
>    python//qtest.py: Check before accessing _qtest

Maybe remove double // in patch subjects :)

Re: [PATCH RFC 00/32] python/qemu: refactor as installable package

[John Snow]

On 5/18/20 8:41 AM, Philippe Mathieu-Daudé wrote:
> On 5/14/20 7:53 AM, John Snow wrote:
> 
>>    python//qmp.py: use True/False for non/blocking modes
>>    python//qmp.py: Define common types
>>    python//qmp.py: re-absorb MonitorResponseError
>>    python//qmp.py: Do not return None from cmd_obj
>>    python//qmp.py: add casts to JSON deserialization
>>    python//qmp.py: add QMPProtocolError
>>    python//qmp.py: assert sockfile is not None
>>    python//machine.py: remove logging configuration
>>    python//machine.py: Fix monitor address typing
>>    python//machine.py: reorder __init__
>>    python//machine.py: Don't modify state in _base_args()
>>    python//machine.py: Handle None events in event_wait
>>    python//machine.py: use qmp.command
>>    python//machine.py: Add _qmp access shim
>>    python//machine.py: fix _popen access
>>    python//qtest.py: Check before accessing _qtest
> 
> Maybe remove double // in patch subjects :)
> 

Sure, if it's causing problems. I meant to imply an elided path
structure. I guess I can do e.g.

"python/qmp"
"python/machine"

and so on, if it matters. We don't really have a standard or anything.
Let me know what you'd prefer and I'll happily change it.

¯\_(ツ)_/¯

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[Vladimir Sementsov-Ogievskiy]

14.05.2020 08:53, John Snow wrote:
> move python/qemu/*.py to python/qemu/lib/*.py.
> 
> To create a namespace package, the 'qemu' directory itself shouldn't
> have module files in it. Thus, these files will go under a 'lib' package
> directory instead.

Hmm..

On the first glance, it looks better to have

   from qemu import QEMUMachine

than
  
   from qemu.lib import QEMUMachine

why do we need this extra ".lib" part?

Is it needed only for internal use?

Assume we have installed qemu package. Can we write

   from qemu import QEMUMachine

? Or we still need qemu.lib ?

I don't remember any python package, which made me to write "import from package_name.lib ..."


-- 
Best regards,
Vladimir

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[John Snow]

On 5/18/20 2:14 PM, Vladimir Sementsov-Ogievskiy wrote:
> 14.05.2020 08:53, John Snow wrote:
>> move python/qemu/*.py to python/qemu/lib/*.py.
>>
>> To create a namespace package, the 'qemu' directory itself shouldn't
>> have module files in it. Thus, these files will go under a 'lib' package
>> directory instead.
> 
> Hmm..
> 
> On the first glance, it looks better to have
> 
>   from qemu import QEMUMachine
> 
> than
>  
>   from qemu.lib import QEMUMachine
> 
> why do we need this extra ".lib" part?
> 
> Is it needed only for internal use?
> 
> Assume we have installed qemu package. Can we write
> 
>   from qemu import QEMUMachine
> 
> ? Or we still need qemu.lib ?
> 
> I don't remember any python package, which made me to write "import from
> package_name.lib ..."
> 
> 

It's a strategy to create "qemu" as a PEP420 namespace package; i.e.
"qemu" forms a namespace, but you need a name for the actual package
underneath it.

"qemu.lib" is one package, with qmp, qtest, and machine modules. "qemu"
isn't really a package in this system, it's just a namespace.

The idea is that this allows us to create a more modular rollout of
various python scripts and services as desired instead of monolithically
bundling them all inside of a "qemu" package.

It also allows us to fork or split out the sub-packages to separate
repos, if we wish. i.e., let's say we create a "qemu.sdk" subpackage, we
can eventually fork it off into its own repo with its own installer and
so forth. These subpackages can be installed and managed separately.

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[Vladimir Sementsov-Ogievskiy]

18.05.2020 21:23, John Snow wrote:
> 
> 
> On 5/18/20 2:14 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 14.05.2020 08:53, John Snow wrote:
>>> move python/qemu/*.py to python/qemu/lib/*.py.
>>>
>>> To create a namespace package, the 'qemu' directory itself shouldn't
>>> have module files in it. Thus, these files will go under a 'lib' package
>>> directory instead.
>>
>> Hmm..
>>
>> On the first glance, it looks better to have
>>
>>    from qemu import QEMUMachine
>>
>> than
>>   
>>    from qemu.lib import QEMUMachine
>>
>> why do we need this extra ".lib" part?
>>
>> Is it needed only for internal use?
>>
>> Assume we have installed qemu package. Can we write
>>
>>    from qemu import QEMUMachine
>>
>> ? Or we still need qemu.lib ?
>>
>> I don't remember any python package, which made me to write "import from
>> package_name.lib ..."
>>
>>
> 
> It's a strategy to create "qemu" as a PEP420 namespace package; i.e.
> "qemu" forms a namespace, but you need a name for the actual package
> underneath it.
> 
> "qemu.lib" is one package, with qmp, qtest, and machine modules. "qemu"
> isn't really a package in this system, it's just a namespace.
> 
> The idea is that this allows us to create a more modular rollout of
> various python scripts and services as desired instead of monolithically
> bundling them all inside of a "qemu" package.
> 
> It also allows us to fork or split out the sub-packages to separate
> repos, if we wish. i.e., let's say we create a "qemu.sdk" subpackage, we
> can eventually fork it off into its own repo with its own installer and
> so forth. These subpackages can be installed and managed separately.
> 

Okay, I understand.. No real objections than.

Still, maybe, everything should not go into lib, maybe something like

qemu/vm/  - qmp, QEMUMachine, etc
qemu/qtest/  - qtest

would be more user friendly? But I'm not sure. I just thought that "lib" is too generic.

-- 
Best regards,
Vladimir

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[John Snow]

On 5/18/20 3:33 PM, Vladimir Sementsov-Ogievskiy wrote:
> 18.05.2020 21:23, John Snow wrote:
>>
>>
>> On 5/18/20 2:14 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.05.2020 08:53, John Snow wrote:
>>>> move python/qemu/*.py to python/qemu/lib/*.py.
>>>>
>>>> To create a namespace package, the 'qemu' directory itself shouldn't
>>>> have module files in it. Thus, these files will go under a 'lib'
>>>> package
>>>> directory instead.
>>>
>>> Hmm..
>>>
>>> On the first glance, it looks better to have
>>>
>>>    from qemu import QEMUMachine
>>>
>>> than
>>>      from qemu.lib import QEMUMachine
>>>
>>> why do we need this extra ".lib" part?
>>>
>>> Is it needed only for internal use?
>>>
>>> Assume we have installed qemu package. Can we write
>>>
>>>    from qemu import QEMUMachine
>>>
>>> ? Or we still need qemu.lib ?
>>>
>>> I don't remember any python package, which made me to write "import from
>>> package_name.lib ..."
>>>
>>>
>>
>> It's a strategy to create "qemu" as a PEP420 namespace package; i.e.
>> "qemu" forms a namespace, but you need a name for the actual package
>> underneath it.
>>
>> "qemu.lib" is one package, with qmp, qtest, and machine modules. "qemu"
>> isn't really a package in this system, it's just a namespace.
>>
>> The idea is that this allows us to create a more modular rollout of
>> various python scripts and services as desired instead of monolithically
>> bundling them all inside of a "qemu" package.
>>
>> It also allows us to fork or split out the sub-packages to separate
>> repos, if we wish. i.e., let's say we create a "qemu.sdk" subpackage, we
>> can eventually fork it off into its own repo with its own installer and
>> so forth. These subpackages can be installed and managed separately.
>>
> 
> Okay, I understand.. No real objections than.
> 
> Still, maybe, everything should not go into lib, maybe something like
> 
> qemu/vm/  - qmp, QEMUMachine, etc
> qemu/qtest/  - qtest
> 
> would be more user friendly? But I'm not sure. I just thought that "lib"
> is too generic.
> 

lib is a very generic name, I agree.

Splitting accel, qmp and QEMUMachine in one package and keeping qtest in
another is fine too. I'm not sure if I like "vm" for the name of that
core package, though.

I want to avoid using "qemu/sdk" because I have some plans for trying to
generate and package a "real" SDK using that namespace.

"devkit"? "testkit"? "core"? Naming things is always the worst part.

--js

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[Vladimir Sementsov-Ogievskiy]

19.05.2020 03:27, John Snow wrote:
> 
> 
> On 5/18/20 3:33 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 18.05.2020 21:23, John Snow wrote:
>>>
>>>
>>> On 5/18/20 2:14 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> 14.05.2020 08:53, John Snow wrote:
>>>>> move python/qemu/*.py to python/qemu/lib/*.py.
>>>>>
>>>>> To create a namespace package, the 'qemu' directory itself shouldn't
>>>>> have module files in it. Thus, these files will go under a 'lib'
>>>>> package
>>>>> directory instead.
>>>>
>>>> Hmm..
>>>>
>>>> On the first glance, it looks better to have
>>>>
>>>>     from qemu import QEMUMachine
>>>>
>>>> than
>>>>       from qemu.lib import QEMUMachine
>>>>
>>>> why do we need this extra ".lib" part?
>>>>
>>>> Is it needed only for internal use?
>>>>
>>>> Assume we have installed qemu package. Can we write
>>>>
>>>>     from qemu import QEMUMachine
>>>>
>>>> ? Or we still need qemu.lib ?
>>>>
>>>> I don't remember any python package, which made me to write "import from
>>>> package_name.lib ..."
>>>>
>>>>
>>>
>>> It's a strategy to create "qemu" as a PEP420 namespace package; i.e.
>>> "qemu" forms a namespace, but you need a name for the actual package
>>> underneath it.
>>>
>>> "qemu.lib" is one package, with qmp, qtest, and machine modules. "qemu"
>>> isn't really a package in this system, it's just a namespace.
>>>
>>> The idea is that this allows us to create a more modular rollout of
>>> various python scripts and services as desired instead of monolithically
>>> bundling them all inside of a "qemu" package.
>>>
>>> It also allows us to fork or split out the sub-packages to separate
>>> repos, if we wish. i.e., let's say we create a "qemu.sdk" subpackage, we
>>> can eventually fork it off into its own repo with its own installer and
>>> so forth. These subpackages can be installed and managed separately.
>>>
>>
>> Okay, I understand.. No real objections than.
>>
>> Still, maybe, everything should not go into lib, maybe something like
>>
>> qemu/vm/  - qmp, QEMUMachine, etc
>> qemu/qtest/  - qtest
>>
>> would be more user friendly? But I'm not sure. I just thought that "lib"
>> is too generic.
>>
> 
> lib is a very generic name, I agree.
> 
> Splitting accel, qmp and QEMUMachine in one package and keeping qtest in
> another is fine too. I'm not sure if I like "vm" for the name of that
> core package, though.
> 
> I want to avoid using "qemu/sdk" because I have some plans for trying to
> generate and package a "real" SDK using that namespace.
> 
> "devkit"? "testkit"? "core"? Naming things is always the worst part.
> 

I think, "core" sounds good.



-- 
Best regards,
Vladimir

Re: [PATCH RFC 00/32] python/qemu: refactor as installable package

[John Snow]

Ping, please give this series a look-over. It looks big, but the changes
themselves are actually fairly tiny.

Here's a TOC:

Patch 1 moves files and renames import statements.
Patches 2-3 do some basic delinting.
Patches 4-5 do more basic delinting and add flake8/pylintrc cfg
Patches 6-10 add setup.py, pipfile, a readme, etc.
Patch 11 is a final bit of removing python2 isms.

Patches 12-32 (!) are all mypy typing fixes, and I have broken these out
in great care:

Patches 12-30 all fix one mypy issue each with bite-sized refactors each.

Patch 31 adds a mypy configuration to the package.

Patch 32 is a giant patch that has __NO__ runtime side-effects, it JUST
adds the remaining mypy annotations.


At the end of the series, you should find that mypy *strict*, flake8,
and pylint all pass 100%.

(Note: you MAY need specific versions of these tools to have them pass.
The pipfile included will help you target the correct versions.)

--js

On 5/14/20 1:53 AM, John Snow wrote:
> Hey, I got lost on my way to the store and I accidentally got 32 patches
> that convert our python library into something that passes pylint,
> flake8, and mypy --strict.
> 
> ...So, a few things:
> 
> 1. This is just an RFC. The actual design of these libraries really
> needs adjusted to be more pythonic. In general this means less
> Optional[T] return types and raising more exceptions. This could be
> handled later, but we ought to address it before publishing, if we do.
> 
> 2. We still need to think carefully about how we package this, if we
> even want to package it, what the license on the top-level package
> should be, etc.
> 
> 3. We should consider how to version it. For now, I'm using a lockstep
> versioning.
> 
> 4. You can install this package using pip3 or python3 setup.py to a
> virtual environment or to your real one. From there, any python code in
> the QEMU tree that imports these modules will work with no sys.path
> hacking or custom PYTHONPATH exports.
> 
> 5. You don't have to install it, though. I left all of the usual hacks
> in place in the rest of the tree so that everything will just keep
> working exactly as-is for right now. It's just that you COULD install it.
> 
> 6. Here's a cool trick if you don't know about it yet:
> 
>> cd qemu/python/qemu
>> pip3 install --user -e .
> 
> This will install the package in "develop" mode, which installs a
> forwarder package. When you update your source tree, the installed
> package stays "up to date" with the most recent edits.
> 
> Alright, have fun, stay safe!
> 
> John Snow (32):
>   python/qemu: create qemu.lib module
>   scripts/qmp: Fix shebang and imports
>   python//machine.py: remove bare except
>   python/qemu/lib: delint, add pylintrc
>   python/qemu/lib: delint; add flake8 config
>   python/qemu: formalize as package
>   python/qemu: add README.rst
>   python/qemu: Add Pipfile
>   python/qemu: add pylint to Pipfile
>   python/qemu: Add flake8 to Pipfile
>   python/qemu/lib: remove Python2 style super() calls
>   python/qemu/lib: fix socket.makefile() typing
>   python/qemu/lib: Adjust traceback typing
>   python//qmp.py: use True/False for non/blocking modes
>   python//qmp.py: Define common types
>   python//qmp.py: re-absorb MonitorResponseError
>   python//qmp.py: Do not return None from cmd_obj
>   python//qmp.py: add casts to JSON deserialization
>   python//qmp.py: add QMPProtocolError
>   python//qmp.py: assert sockfile is not None
>   python//machine.py: remove logging configuration
>   python//machine.py: Fix monitor address typing
>   python//machine.py: reorder __init__
>   python//machine.py: Don't modify state in _base_args()
>   python//machine.py: Handle None events in event_wait
>   python//machine.py: use qmp.command
>   python//machine.py: Add _qmp access shim
>   python//machine.py: fix _popen access
>   python//qtest.py: Check before accessing _qtest
>   python/qemu/lib: make 'args' style arguments immutable
>   python/qemu: add mypy to Pipfile
>   python/qemu/lib: Add mypy type annotations
> 
>  python/README.rst                         |   6 +
>  python/qemu/README.rst                    |   8 +
>  python/Pipfile                            |  14 +
>  python/Pipfile.lock                       | 187 +++++++++++++
>  python/qemu/__init__.py                   |  11 -
>  python/qemu/lib/.flake8                   |   2 +
>  python/qemu/lib/__init__.py               |  57 ++++
>  python/qemu/{ => lib}/accel.py            |  17 +-
>  python/qemu/{ => lib}/machine.py          | 320 +++++++++++++---------
>  python/qemu/lib/pylintrc                  |  58 ++++
>  python/qemu/{ => lib}/qmp.py              | 140 +++++++---
>  python/qemu/lib/qtest.py                  | 160 +++++++++++
>  python/qemu/qtest.py                      | 119 --------
>  python/setup.py                           |  50 ++++
>  scripts/device-crash-test                 |   2 +-
>  scripts/qmp/qemu-ga-client                |   2 +-
>  scripts/qmp/qmp                           |   4 +-
>  scripts/qmp/qmp-shell                     |   2 +-
>  scripts/qmp/qom-fuse                      |   4 +-
>  scripts/qmp/qom-get                       |   6 +-
>  scripts/qmp/qom-list                      |   4 +-
>  scripts/qmp/qom-set                       |   6 +-
>  scripts/qmp/qom-tree                      |   6 +-
>  scripts/render_block_graph.py             |   5 +-
>  scripts/simplebench/bench_block_job.py    |   4 +-
>  tests/acceptance/avocado_qemu/__init__.py |   2 +-
>  tests/acceptance/boot_linux.py            |   3 +-
>  tests/acceptance/virtio_check_params.py   |   2 +-
>  tests/acceptance/virtio_version.py        |   2 +-
>  tests/migration/guestperf/engine.py       |   2 +-
>  tests/qemu-iotests/235                    |   2 +-
>  tests/qemu-iotests/iotests.py             |   2 +-
>  tests/vm/basevm.py                        |   6 +-
>  33 files changed, 881 insertions(+), 334 deletions(-)
>  create mode 100644 python/README.rst
>  create mode 100644 python/qemu/README.rst
>  create mode 100644 python/Pipfile
>  create mode 100644 python/Pipfile.lock
>  delete mode 100644 python/qemu/__init__.py
>  create mode 100644 python/qemu/lib/.flake8
>  create mode 100644 python/qemu/lib/__init__.py
>  rename python/qemu/{ => lib}/accel.py (86%)
>  rename python/qemu/{ => lib}/machine.py (67%)
>  create mode 100644 python/qemu/lib/pylintrc
>  rename python/qemu/{ => lib}/qmp.py (70%)
>  create mode 100644 python/qemu/lib/qtest.py
>  delete mode 100644 python/qemu/qtest.py
>  create mode 100755 python/setup.py
> 

-- 
—js

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[Philippe Mathieu-Daudé]

On 5/19/20 12:54 PM, Vladimir Sementsov-Ogievskiy wrote:
> 19.05.2020 03:27, John Snow wrote:
>>
>>
>> On 5/18/20 3:33 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> 18.05.2020 21:23, John Snow wrote:
>>>>
>>>>
>>>> On 5/18/20 2:14 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 14.05.2020 08:53, John Snow wrote:
>>>>>> move python/qemu/*.py to python/qemu/lib/*.py.
>>>>>>
>>>>>> To create a namespace package, the 'qemu' directory itself shouldn't
>>>>>> have module files in it. Thus, these files will go under a 'lib'
>>>>>> package
>>>>>> directory instead.
>>>>>
>>>>> Hmm..
>>>>>
>>>>> On the first glance, it looks better to have
>>>>>
>>>>>     from qemu import QEMUMachine
>>>>>
>>>>> than
>>>>>       from qemu.lib import QEMUMachine
>>>>>
>>>>> why do we need this extra ".lib" part?
>>>>>
>>>>> Is it needed only for internal use?
>>>>>
>>>>> Assume we have installed qemu package. Can we write
>>>>>
>>>>>     from qemu import QEMUMachine
>>>>>
>>>>> ? Or we still need qemu.lib ?
>>>>>
>>>>> I don't remember any python package, which made me to write "import
>>>>> from
>>>>> package_name.lib ..."
>>>>>
>>>>>
>>>>
>>>> It's a strategy to create "qemu" as a PEP420 namespace package; i.e.
>>>> "qemu" forms a namespace, but you need a name for the actual package
>>>> underneath it.
>>>>
>>>> "qemu.lib" is one package, with qmp, qtest, and machine modules. "qemu"
>>>> isn't really a package in this system, it's just a namespace.
>>>>
>>>> The idea is that this allows us to create a more modular rollout of
>>>> various python scripts and services as desired instead of
>>>> monolithically
>>>> bundling them all inside of a "qemu" package.
>>>>
>>>> It also allows us to fork or split out the sub-packages to separate
>>>> repos, if we wish. i.e., let's say we create a "qemu.sdk"
>>>> subpackage, we
>>>> can eventually fork it off into its own repo with its own installer and
>>>> so forth. These subpackages can be installed and managed separately.
>>>>
>>>
>>> Okay, I understand.. No real objections than.
>>>
>>> Still, maybe, everything should not go into lib, maybe something like
>>>
>>> qemu/vm/  - qmp, QEMUMachine, etc
>>> qemu/qtest/  - qtest

I'm not sure this part is relevant now, as we have not good projection
of what/who/how this package will be consumed.

I suppose by VM you mean VirtualMachine. I find it confusing. Maybe
simply "machine"? We also have 'tools' and 'user-space processes'.

QMP is protocol, common to all. "qemu.core.qmp"?

We also have the gdb(stub) protocol, common to machine(system) & user.

The block layer has its classes, "qemu.block"?

>>>
>>> would be more user friendly? But I'm not sure. I just thought that "lib"
>>> is too generic.
>>>
>>
>> lib is a very generic name, I agree.
>>
>> Splitting accel, qmp and QEMUMachine in one package and keeping qtest in
>> another is fine too. I'm not sure if I like "vm" for the name of that
>> core package, though.
>>
>> I want to avoid using "qemu/sdk" because I have some plans for trying to
>> generate and package a "real" SDK using that namespace.
>>
>> "devkit"? "testkit"? "core"? Naming things is always the worst part.
>>
> 
> I think, "core" sounds good.

Agreed.

Re: [PATCH RFC 03/32] python//machine.py: remove bare except

[Philippe Mathieu-Daudé]

On 5/14/20 4:26 PM, John Snow wrote:
> 
> 
> On 5/14/20 9:55 AM, Eric Blake wrote:
>> On 5/14/20 12:53 AM, John Snow wrote:
>>> Catch only the timeout error; if there are other problems, allow the
>>> stack trace to be visible.
>>>
>>
>> A lot of patches in this series start with "python//" - is that
>> intentional, or should it be a single slash?
>>
> 
> Was trying to imply an elided path structure, because
> "python/qemu/lib/qmp.py" et al is way too chatty.
> 
> Feel free to suggest better subject names!
> 

Eliding "qemu/lib/" is OK. I wouldn't not use double //, it sounds like
a recurrent typo.

Re: [PATCH RFC 03/32] python//machine.py: remove bare except

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> Catch only the timeout error; if there are other problems, allow the
> stack trace to be visible.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/machine.py | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
> index b9a98e2c86..e3ea523571 100644
> --- a/python/qemu/lib/machine.py
> +++ b/python/qemu/lib/machine.py
> @@ -342,7 +342,26 @@ def wait(self):
>          self._load_io_log()
>          self._post_shutdown()
>  
> -    def shutdown(self, has_quit=False):
> +    def _issue_shutdown(self, has_quit: bool = False) -> None:
> +        """
> +        Shutdown the VM.
> +        """
> +        if not self.is_running():
> +            return
> +
> +        if self._qmp is not None:
> +            if not has_quit:
> +                self._qmp.cmd('quit')
> +            self._qmp.close()
> +
> +            try:
> +                self._popen.wait(timeout=3)
> +            except subprocess.TimeoutExpired:
> +                self._popen.kill()
> +
> +        self._popen.wait()
> +
> +    def shutdown(self, has_quit: bool = False) -> None:
>          """
>          Terminate the VM and clean up
>          """
> @@ -353,17 +372,7 @@ def shutdown(self, has_quit=False):
>              self._console_socket.close()
>              self._console_socket = None
>  
> -        if self.is_running():
> -            if self._qmp:
> -                try:
> -                    if not has_quit:
> -                        self._qmp.cmd('quit')
> -                    self._qmp.close()
> -                    self._popen.wait(timeout=3)
> -                except:
> -                    self._popen.kill()
> -            self._popen.wait()
> -
> +        self._issue_shutdown(has_quit)
>          self._load_io_log()
>          self._post_shutdown()
>  
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[Daniel P. Berrangé]

On Mon, May 18, 2020 at 08:27:54PM -0400, John Snow wrote:
> 
> 
> On 5/18/20 3:33 PM, Vladimir Sementsov-Ogievskiy wrote:
> > 18.05.2020 21:23, John Snow wrote:
> >>
> >>
> >> On 5/18/20 2:14 PM, Vladimir Sementsov-Ogievskiy wrote:
> >>> 14.05.2020 08:53, John Snow wrote:
> >>>> move python/qemu/*.py to python/qemu/lib/*.py.
> >>>>
> >>>> To create a namespace package, the 'qemu' directory itself shouldn't
> >>>> have module files in it. Thus, these files will go under a 'lib'
> >>>> package
> >>>> directory instead.
> >>>
> >>> Hmm..
> >>>
> >>> On the first glance, it looks better to have
> >>>
> >>>    from qemu import QEMUMachine
> >>>
> >>> than
> >>>      from qemu.lib import QEMUMachine
> >>>
> >>> why do we need this extra ".lib" part?
> >>>
> >>> Is it needed only for internal use?
> >>>
> >>> Assume we have installed qemu package. Can we write
> >>>
> >>>    from qemu import QEMUMachine
> >>>
> >>> ? Or we still need qemu.lib ?
> >>>
> >>> I don't remember any python package, which made me to write "import from
> >>> package_name.lib ..."
> >>>
> >>>
> >>
> >> It's a strategy to create "qemu" as a PEP420 namespace package; i.e.
> >> "qemu" forms a namespace, but you need a name for the actual package
> >> underneath it.
> >>
> >> "qemu.lib" is one package, with qmp, qtest, and machine modules. "qemu"
> >> isn't really a package in this system, it's just a namespace.
> >>
> >> The idea is that this allows us to create a more modular rollout of
> >> various python scripts and services as desired instead of monolithically
> >> bundling them all inside of a "qemu" package.
> >>
> >> It also allows us to fork or split out the sub-packages to separate
> >> repos, if we wish. i.e., let's say we create a "qemu.sdk" subpackage, we
> >> can eventually fork it off into its own repo with its own installer and
> >> so forth. These subpackages can be installed and managed separately.
> >>
> > 
> > Okay, I understand.. No real objections than.
> > 
> > Still, maybe, everything should not go into lib, maybe something like
> > 
> > qemu/vm/  - qmp, QEMUMachine, etc
> > qemu/qtest/  - qtest
> > 
> > would be more user friendly? But I'm not sure. I just thought that "lib"
> > is too generic.
> > 
> 
> lib is a very generic name, I agree.
> 
> Splitting accel, qmp and QEMUMachine in one package and keeping qtest in
> another is fine too. I'm not sure if I like "vm" for the name of that
> core package, though.
> 
> I want to avoid using "qemu/sdk" because I have some plans for trying to
> generate and package a "real" SDK using that namespace.
> 
> "devkit"? "testkit"? "core"? Naming things is always the worst part.

I'd suggest  "machine", as in

  from qemu.machine import  kvm_available, QEMUMachine

I wouldn't over-think the module naming as it has so little impact on
the code usage - it usually only appears in the "import" statement.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[Philippe Mathieu-Daudé]

On 5/26/20 5:22 PM, Daniel P. Berrangé wrote:
> On Mon, May 18, 2020 at 08:27:54PM -0400, John Snow wrote:
>>
>>
>> On 5/18/20 3:33 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> 18.05.2020 21:23, John Snow wrote:
>>>>
>>>>
>>>> On 5/18/20 2:14 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 14.05.2020 08:53, John Snow wrote:
>>>>>> move python/qemu/*.py to python/qemu/lib/*.py.
>>>>>>
>>>>>> To create a namespace package, the 'qemu' directory itself shouldn't
>>>>>> have module files in it. Thus, these files will go under a 'lib'
>>>>>> package
>>>>>> directory instead.
>>>>>
>>>>> Hmm..
>>>>>
>>>>> On the first glance, it looks better to have
>>>>>
>>>>>    from qemu import QEMUMachine
>>>>>
>>>>> than
>>>>>      from qemu.lib import QEMUMachine
>>>>>
>>>>> why do we need this extra ".lib" part?
>>>>>
>>>>> Is it needed only for internal use?
>>>>>
>>>>> Assume we have installed qemu package. Can we write
>>>>>
>>>>>    from qemu import QEMUMachine
>>>>>
>>>>> ? Or we still need qemu.lib ?
>>>>>
>>>>> I don't remember any python package, which made me to write "import from
>>>>> package_name.lib ..."
>>>>>
>>>>>
>>>>
>>>> It's a strategy to create "qemu" as a PEP420 namespace package; i.e.
>>>> "qemu" forms a namespace, but you need a name for the actual package
>>>> underneath it.
>>>>
>>>> "qemu.lib" is one package, with qmp, qtest, and machine modules. "qemu"
>>>> isn't really a package in this system, it's just a namespace.
>>>>
>>>> The idea is that this allows us to create a more modular rollout of
>>>> various python scripts and services as desired instead of monolithically
>>>> bundling them all inside of a "qemu" package.
>>>>
>>>> It also allows us to fork or split out the sub-packages to separate
>>>> repos, if we wish. i.e., let's say we create a "qemu.sdk" subpackage, we
>>>> can eventually fork it off into its own repo with its own installer and
>>>> so forth. These subpackages can be installed and managed separately.
>>>>
>>>
>>> Okay, I understand.. No real objections than.
>>>
>>> Still, maybe, everything should not go into lib, maybe something like
>>>
>>> qemu/vm/  - qmp, QEMUMachine, etc
>>> qemu/qtest/  - qtest
>>>
>>> would be more user friendly? But I'm not sure. I just thought that "lib"
>>> is too generic.
>>>
>>
>> lib is a very generic name, I agree.
>>
>> Splitting accel, qmp and QEMUMachine in one package and keeping qtest in
>> another is fine too. I'm not sure if I like "vm" for the name of that
>> core package, though.
>>
>> I want to avoid using "qemu/sdk" because I have some plans for trying to
>> generate and package a "real" SDK using that namespace.
>>
>> "devkit"? "testkit"? "core"? Naming things is always the worst part.
> 
> I'd suggest  "machine", as in
> 
>   from qemu.machine import  kvm_available, QEMUMachine
> 
> I wouldn't over-think the module naming as it has so little impact on
> the code usage - it usually only appears in the "import" statement.

Don't forget linux-user binaries.

> 
> 
> Regards,
> Daniel
> 

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[Daniel P. Berrangé]

On Tue, May 26, 2020 at 05:23:42PM +0200, Philippe Mathieu-Daudé wrote:
> On 5/26/20 5:22 PM, Daniel P. Berrangé wrote:
> > On Mon, May 18, 2020 at 08:27:54PM -0400, John Snow wrote:
> >>
> >>
> >> On 5/18/20 3:33 PM, Vladimir Sementsov-Ogievskiy wrote:
> >>> 18.05.2020 21:23, John Snow wrote:
> >>>>
> >>>>
> >>>> On 5/18/20 2:14 PM, Vladimir Sementsov-Ogievskiy wrote:
> >>>>> 14.05.2020 08:53, John Snow wrote:
> >>>>>> move python/qemu/*.py to python/qemu/lib/*.py.
> >>>>>>
> >>>>>> To create a namespace package, the 'qemu' directory itself shouldn't
> >>>>>> have module files in it. Thus, these files will go under a 'lib'
> >>>>>> package
> >>>>>> directory instead.
> >>>>>
> >>>>> Hmm..
> >>>>>
> >>>>> On the first glance, it looks better to have
> >>>>>
> >>>>>    from qemu import QEMUMachine
> >>>>>
> >>>>> than
> >>>>>      from qemu.lib import QEMUMachine
> >>>>>
> >>>>> why do we need this extra ".lib" part?
> >>>>>
> >>>>> Is it needed only for internal use?
> >>>>>
> >>>>> Assume we have installed qemu package. Can we write
> >>>>>
> >>>>>    from qemu import QEMUMachine
> >>>>>
> >>>>> ? Or we still need qemu.lib ?
> >>>>>
> >>>>> I don't remember any python package, which made me to write "import from
> >>>>> package_name.lib ..."
> >>>>>
> >>>>>
> >>>>
> >>>> It's a strategy to create "qemu" as a PEP420 namespace package; i.e.
> >>>> "qemu" forms a namespace, but you need a name for the actual package
> >>>> underneath it.
> >>>>
> >>>> "qemu.lib" is one package, with qmp, qtest, and machine modules. "qemu"
> >>>> isn't really a package in this system, it's just a namespace.
> >>>>
> >>>> The idea is that this allows us to create a more modular rollout of
> >>>> various python scripts and services as desired instead of monolithically
> >>>> bundling them all inside of a "qemu" package.
> >>>>
> >>>> It also allows us to fork or split out the sub-packages to separate
> >>>> repos, if we wish. i.e., let's say we create a "qemu.sdk" subpackage, we
> >>>> can eventually fork it off into its own repo with its own installer and
> >>>> so forth. These subpackages can be installed and managed separately.
> >>>>
> >>>
> >>> Okay, I understand.. No real objections than.
> >>>
> >>> Still, maybe, everything should not go into lib, maybe something like
> >>>
> >>> qemu/vm/  - qmp, QEMUMachine, etc
> >>> qemu/qtest/  - qtest
> >>>
> >>> would be more user friendly? But I'm not sure. I just thought that "lib"
> >>> is too generic.
> >>>
> >>
> >> lib is a very generic name, I agree.
> >>
> >> Splitting accel, qmp and QEMUMachine in one package and keeping qtest in
> >> another is fine too. I'm not sure if I like "vm" for the name of that
> >> core package, though.
> >>
> >> I want to avoid using "qemu/sdk" because I have some plans for trying to
> >> generate and package a "real" SDK using that namespace.
> >>
> >> "devkit"? "testkit"? "core"? Naming things is always the worst part.
> > 
> > I'd suggest  "machine", as in
> > 
> >   from qemu.machine import  kvm_available, QEMUMachine
> > 
> > I wouldn't over-think the module naming as it has so little impact on
> > the code usage - it usually only appears in the "import" statement.
> 
> Don't forget linux-user binaries.

That's why I suggested ".machine", as all the APIs there currently
are focused on the machine emulators, and the linx-user binaries
share essentially nothing in common with softmmu binaries in terms
of control APIs / CLI config. We can add a "qemu.user" package
later if we have stuff related to that to expose

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 02/32] scripts/qmp: Fix shebang and imports

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> There's more wrong with these scripts; They are in various stages of
> disrepair. That's beyond the scope of this current patchset.
> 
> This just mechanically corrects the imports and the shebangs, as part of
> ensuring that the python/qemu/lib refactoring didn't break anything
> needlessly.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Whoops, I address this in a patch series I'm working on, too. I'll keep
my patch in there for now until this one makes it in, or vice-versa.

https://www.mail-archive.com/qemu-block@nongnu.org/msg66801.html

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  scripts/qmp/qmp      | 4 +++-
>  scripts/qmp/qom-fuse | 4 +++-
>  scripts/qmp/qom-get  | 6 ++++--
>  scripts/qmp/qom-list | 4 +++-
>  scripts/qmp/qom-set  | 6 ++++--
>  scripts/qmp/qom-tree | 6 ++++--
>  6 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/scripts/qmp/qmp b/scripts/qmp/qmp
> index 0625fc2aba..5981f7c414 100755
> --- a/scripts/qmp/qmp
> +++ b/scripts/qmp/qmp
> @@ -11,7 +11,9 @@
>  # See the COPYING file in the top-level directory.
>  
>  import sys, os
> -from qmp import QEMUMonitorProtocol
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> +from qemu.lib import QEMUMonitorProtocol
>  
>  def print_response(rsp, prefix=[]):
>      if type(rsp) == list:
> diff --git a/scripts/qmp/qom-fuse b/scripts/qmp/qom-fuse
> index 6bada2c33d..6006f1d562 100755
> --- a/scripts/qmp/qom-fuse
> +++ b/scripts/qmp/qom-fuse
> @@ -15,7 +15,9 @@ import fuse, stat
>  from fuse import Fuse
>  import os, posix
>  from errno import *
> -from qmp import QEMUMonitorProtocol
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> +from qemu.lib import QEMUMonitorProtocol
>  
>  fuse.fuse_python_api = (0, 2)
>  
> diff --git a/scripts/qmp/qom-get b/scripts/qmp/qom-get
> index 007b4cd442..1d04d1b119 100755
> --- a/scripts/qmp/qom-get
> +++ b/scripts/qmp/qom-get
> @@ -1,4 +1,4 @@
> -#!/usr/bin/python
> +#!/usr/bin/env python3
>  ##
>  # QEMU Object Model test tools
>  #
> @@ -13,7 +13,9 @@
>  
>  import sys
>  import os
> -from qmp import QEMUMonitorProtocol
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> +from qemu.lib import QEMUMonitorProtocol
>  
>  cmd, args = sys.argv[0], sys.argv[1:]
>  socket_path = None
> diff --git a/scripts/qmp/qom-list b/scripts/qmp/qom-list
> index 03bda3446b..94e3bc21ba 100755
> --- a/scripts/qmp/qom-list
> +++ b/scripts/qmp/qom-list
> @@ -1,4 +1,4 @@
> -#!/usr/bin/python
> +#!/usr/bin/env python3
>  ##
>  # QEMU Object Model test tools
>  #
> @@ -13,6 +13,8 @@
>  
>  import sys
>  import os
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
>  from qmp import QEMUMonitorProtocol
>  
>  cmd, args = sys.argv[0], sys.argv[1:]
> diff --git a/scripts/qmp/qom-set b/scripts/qmp/qom-set
> index c37fe78b00..a78080a4fc 100755
> --- a/scripts/qmp/qom-set
> +++ b/scripts/qmp/qom-set
> @@ -1,4 +1,4 @@
> -#!/usr/bin/python
> +#!/usr/bin/env python3
>  ##
>  # QEMU Object Model test tools
>  #
> @@ -13,7 +13,9 @@
>  
>  import sys
>  import os
> -from qmp import QEMUMonitorProtocol
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> +from qemu.lib import QEMUMonitorProtocol
>  
>  cmd, args = sys.argv[0], sys.argv[1:]
>  socket_path = None
> diff --git a/scripts/qmp/qom-tree b/scripts/qmp/qom-tree
> index 1c8acf61e7..4866c15581 100755
> --- a/scripts/qmp/qom-tree
> +++ b/scripts/qmp/qom-tree
> @@ -1,4 +1,4 @@
> -#!/usr/bin/python
> +#!/usr/bin/env python3
>  ##
>  # QEMU Object Model test tools
>  #
> @@ -15,7 +15,9 @@
>  
>  import sys
>  import os
> -from qmp import QEMUMonitorProtocol
> +
> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> +from qemu.lib import QEMUMonitorProtocol
>  
>  cmd, args = sys.argv[0], sys.argv[1:]
>  socket_path = None
> 

Re: [PATCH RFC 04/32] python/qemu/lib: delint, add pylintrc

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> Bring our these files up to speed with pylint 2.5.0.
> Add a pylintrc file to formalize which pylint subset
> we are targeting.
> 
> The similarity ignore is there to suppress similarity
> reports across imports, which for typing constants,
> are going to trigger this report erroneously.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/machine.py |  6 ++--
>  python/qemu/lib/pylintrc   | 58 ++++++++++++++++++++++++++++++++++++++
>  python/qemu/lib/qtest.py   | 42 +++++++++++++++++----------
>  3 files changed, 88 insertions(+), 18 deletions(-)
>  create mode 100644 python/qemu/lib/pylintrc
> 
> diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
> index e3ea523571..c79fc8fb89 100644
> --- a/python/qemu/lib/machine.py
> +++ b/python/qemu/lib/machine.py
> @@ -58,7 +58,7 @@ def __init__(self, reply):
>          self.reply = reply
>  
>  
> -class QEMUMachine(object):
> +class QEMUMachine:
>      """
>      A QEMU VM
>  
> @@ -242,7 +242,7 @@ def _base_args(self):
>                           'chardev=mon,mode=control'])
>          if self._machine is not None:
>              args.extend(['-machine', self._machine])
> -        for i in range(self._console_index):
> +        for _ in range(self._console_index):
>              args.extend(['-serial', 'null'])
>          if self._console_set:
>              self._console_address = os.path.join(self._sock_dir,
> @@ -383,7 +383,7 @@ def shutdown(self, has_quit: bool = False) -> None:
>                  command = ' '.join(self._qemu_full_args)
>              else:
>                  command = ''
> -            LOG.warning(msg, -exitcode, command)
> +            LOG.warning(msg, -int(exitcode), command)
>  
>          self._launched = False
>  
> diff --git a/python/qemu/lib/pylintrc b/python/qemu/lib/pylintrc
> new file mode 100644
> index 0000000000..5d6ae7367d
> --- /dev/null
> +++ b/python/qemu/lib/pylintrc
> @@ -0,0 +1,58 @@
> +[MASTER]
> +
> +[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
> +# option multiple times (only on the command line, not in the configuration
> +# file where it should appear only once). You can also use "--disable=all" to
> +# disable everything first and then reenable specific checks. For example, if
> +# you want to run only the similarities checker, you can use "--disable=all
> +# --enable=similarities". If you want to run only the classes checker, but have
> +# no Warning level messages displayed, use "--disable=all --enable=classes
> +# --disable=W".
> +disable=too-many-arguments,
> +        too-many-instance-attributes,
> +        too-many-public-methods,

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +
> +[REPORTS]
> +
> +[REFACTORING]
> +
> +[MISCELLANEOUS]
> +
> +[LOGGING]
> +
> +[BASIC]
> +
> +# Good variable names which should always be accepted, separated by a comma.
> +good-names=i,
> +           j,
> +           k,
> +           ex,
> +           Run,
> +           _,
> +           fd,
> +
> +[VARIABLES]
> +
> +[STRING]
> +
> +[SPELLING]
> +
> +[FORMAT]
> +
> +[SIMILARITIES]
> +
> +# Ignore imports when computing similarities.
> +ignore-imports=yes
> +
> +[TYPECHECK]
> +
> +[CLASSES]
> +
> +[IMPORTS]
> +
> +[DESIGN]
> +
> +[EXCEPTIONS]
> diff --git a/python/qemu/lib/qtest.py b/python/qemu/lib/qtest.py
> index d24ad04256..53d814c064 100644
> --- a/python/qemu/lib/qtest.py
> +++ b/python/qemu/lib/qtest.py
> @@ -1,5 +1,11 @@
> -# QEMU qtest library
> -#
> +"""
> +QEMU qtest library
> +
> +qtest offers the QEMUQtestProtocol and QEMUQTestMachine classes, which
> +offer a connection to QEMU's qtest protocol socket, and a qtest-enabled
> +subclass of QEMUMachine, respectively.
> +"""
> +
>  # Copyright (C) 2015 Red Hat Inc.
>  #
>  # Authors:
> @@ -17,19 +23,21 @@
>  from .machine import QEMUMachine
>  
>  
> -class QEMUQtestProtocol(object):
> +class QEMUQtestProtocol:
> +    """
> +    QEMUQtestProtocol implements a connection to a qtest socket.
> +
> +    :param address: QEMU address, can be either a unix socket path (string)
> +                    or a tuple in the form ( address, port ) for a TCP
> +                    connection
> +    :param server: server mode, listens on the socket (bool)
> +    :raise socket.error: on socket connection errors
> +
> +    .. note::
> +       No conection is estabalished by __init__(), this is done
> +       by the connect() or accept() methods.
> +    """
>      def __init__(self, address, server=False):
> -        """
> -        Create a QEMUQtestProtocol object.
> -
> -        @param address: QEMU address, can be either a unix socket path (string)
> -                        or a tuple in the form ( address, port ) for a TCP
> -                        connection
> -        @param server: server mode, listens on the socket (bool)
> -        @raise socket.error on socket connection errors
> -        @note No connection is established, this is done by the connect() or
> -              accept() methods
> -        """
>          self._address = address
>          self._sock = self._get_sock()
>          self._sockfile = None
> @@ -73,15 +81,19 @@ def cmd(self, qtest_cmd):
>          return resp
>  
>      def close(self):
> +        """Close this socket."""
>          self._sock.close()
>          self._sockfile.close()
>  
>      def settimeout(self, timeout):
> +        """Set a timeout, in seconds."""
>          self._sock.settimeout(timeout)
>  
>  
>  class QEMUQtestMachine(QEMUMachine):
> -    '''A QEMU VM'''
> +    """
> +    A QEMU VM, with a qtest socket available.
> +    """
>  
>      def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
>                   socket_scm_helper=None, sock_dir=None):
> 

Re: [PATCH RFC 05/32] python/qemu/lib: delint; add flake8 config

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> Mostly, ignore the "no bare except" rule, because flake8 is not
> contextual and cannot determine if we re-raise. Pylint can, though, so
> always prefer pylint for that.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/.flake8    |  2 ++
>  python/qemu/lib/accel.py   |  9 ++++++---
>  python/qemu/lib/machine.py | 13 +++++++++----
>  python/qemu/lib/qmp.py     |  4 ++--
>  4 files changed, 19 insertions(+), 9 deletions(-)
>  create mode 100644 python/qemu/lib/.flake8
> 
> diff --git a/python/qemu/lib/.flake8 b/python/qemu/lib/.flake8
> new file mode 100644
> index 0000000000..45d8146f3f
> --- /dev/null
> +++ b/python/qemu/lib/.flake8
> @@ -0,0 +1,2 @@
> +[flake8]
> +extend-ignore = E722  # Pylint handles this, but smarter.
> \ No newline at end of file
> diff --git a/python/qemu/lib/accel.py b/python/qemu/lib/accel.py
> index 36ae85791e..7fabe62920 100644
> --- a/python/qemu/lib/accel.py
> +++ b/python/qemu/lib/accel.py
> @@ -23,11 +23,12 @@
>  # Mapping host architecture to any additional architectures it can
>  # support which often includes its 32 bit cousin.
>  ADDITIONAL_ARCHES = {
> -    "x86_64" : "i386",
> -    "aarch64" : "armhf",
> -    "ppc64le" : "ppc64",
> +    "x86_64": "i386",
> +    "aarch64": "armhf",
> +    "ppc64le": "ppc64",
>  }
>  
> +
>  def list_accel(qemu_bin):
>      """
>      List accelerators enabled in the QEMU binary.
> @@ -47,6 +48,7 @@ def list_accel(qemu_bin):
>      # Skip the first line which is the header.
>      return [acc.strip() for acc in out.splitlines()[1:]]
>  
> +
>  def kvm_available(target_arch=None, qemu_bin=None):
>      """
>      Check if KVM is available using the following heuristic:
> @@ -69,6 +71,7 @@ def kvm_available(target_arch=None, qemu_bin=None):
>          return False
>      return True
>  
> +
>  def tcg_available(qemu_bin):
>      """
>      Check if TCG is available.
> diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
> index c79fc8fb89..4b260fa2cb 100644
> --- a/python/qemu/lib/machine.py
> +++ b/python/qemu/lib/machine.py
> @@ -29,6 +29,7 @@
>  
>  LOG = logging.getLogger(__name__)
>  
> +
>  class QEMUMachineError(Exception):
>      """
>      Exception called when an error in QEMUMachine happens.
> @@ -62,7 +63,8 @@ class QEMUMachine:
>      """
>      A QEMU VM
>  
> -    Use this object as a context manager to ensure the QEMU process terminates::
> +    Use this object as a context manager to ensure
> +    the QEMU process terminates::
>  
>          with VM(binary) as vm:
>              ...
> @@ -188,8 +190,10 @@ def send_fd_scm(self, fd=None, file_path=None):
>              fd_param.append(str(fd))
>  
>          devnull = open(os.path.devnull, 'rb')
> -        proc = subprocess.Popen(fd_param, stdin=devnull, stdout=subprocess.PIPE,
> -                                stderr=subprocess.STDOUT, close_fds=False)
> +        proc = subprocess.Popen(
> +            fd_param, stdin=devnull, stdout=subprocess.PIPE,
> +            stderr=subprocess.STDOUT, close_fds=False
> +        )
>          output = proc.communicate()[0]
>          if output:
>              LOG.debug(output)
> @@ -491,7 +495,8 @@ def event_wait(self, name, timeout=60.0, match=None):
>  
>      def events_wait(self, events, timeout=60.0):
>          """
> -        events_wait waits for and returns a named event from QMP with a timeout.
> +        events_wait waits for and returns a named event
> +        from QMP with a timeout.
>  
>          events: a sequence of (name, match_criteria) tuples.
>                  The match criteria are optional and may be None.
> diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
> index d6c9b2f4b1..6ae7693965 100644
> --- a/python/qemu/lib/qmp.py
> +++ b/python/qemu/lib/qmp.py
> @@ -168,8 +168,8 @@ def accept(self, timeout=15.0):
>  
>          @param timeout: timeout in seconds (nonnegative float number, or
>                          None). The value passed will set the behavior of the
> -                        underneath QMP socket as described in [1]. Default value
> -                        is set to 15.0.
> +                        underneath QMP socket as described in [1].
> +                        Default value is set to 15.0.
>          @return QMP greeting dict
>          @raise OSError on socket connection errors
>          @raise QMPConnectError if the greeting is not received
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH RFC 06/32] python/qemu: formalize as package

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> NB: I am choosing Python 3.6 here. Although our minimum requirement is
> 3.5, this code is used only by iotests (so far) under which we have been
> using a minimum version of 3.6.

Preferably with another Ack-by from someone familiar with Python iotests:
Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> 3.6 is being preferred here for variable type hint capability, which
> enables us to use mypy for this package.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/README.rst |  6 ++++++
>  python/setup.py   | 50 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>  create mode 100644 python/README.rst
>  create mode 100755 python/setup.py
> 
> diff --git a/python/README.rst b/python/README.rst
> new file mode 100644
> index 0000000000..25f6d93fd5
> --- /dev/null
> +++ b/python/README.rst
> @@ -0,0 +1,6 @@
> +QEMU Python Tooling
> +-------------------
> +
> +This package provides QEMU tooling used by the QEMU project to build,
> +configure, and test QEMU. It is not a fully-fledged SDK and it is subject
> +to change at any time.
> diff --git a/python/setup.py b/python/setup.py
> new file mode 100755
> index 0000000000..f897ceac97
> --- /dev/null
> +++ b/python/setup.py
> @@ -0,0 +1,50 @@
> +#!/usr/bin/env3 python
> +"""
> +QEMU tooling installer script
> +Copyright (c) 2020 John Snow for Red Hat, Inc.
> +"""
> +
> +import setuptools
> +
> +def main():
> +    """
> +    QEMU tooling installer
> +    """
> +
> +    kwargs = {
> +        'name': 'qemu',
> +        'use_scm_version': {
> +            'root': '..',
> +            'relative_to': __file__,
> +        },
> +        'maintainer': 'QEMU Developer Team',
> +        'maintainer_email': 'qemu-devel@nongnu.org',
> +        'url': 'https://www.qemu.org/',
> +        'download_url': 'https://www.qemu.org/download/',
> +        'packages': setuptools.find_namespace_packages(),
> +        'description': 'QEMU Python Build, Debug and SDK tooling.',
> +        'classifiers': [
> +            'Development Status :: 5 - Production/Stable',
> +            'License :: OSI Approved :: GNU General Public License v2 (GPLv2)',
> +            'Natural Language :: English',
> +            'Operating System :: OS Independent',
> +        ],
> +        'platforms': [],
> +        'keywords': [],
> +        'setup_requires': [
> +            'setuptools',
> +            'setuptools_scm',
> +        ],
> +        'install_requires': [
> +        ],
> +        'python_requires': '>=3.6',
> +        'long_description_content_type': 'text/x-rst',
> +    }
> +
> +    with open("README.rst", "r") as fh:
> +        kwargs['long_description'] = fh.read()
> +
> +    setuptools.setup(**kwargs)
> +
> +if __name__ == '__main__':
> +    main()
> 

Re: [PATCH RFC 13/32] python/qemu/lib: Adjust traceback typing

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> mypy considers it incorrect to use `bool` to statically return false,
> because it will assume that it could conceivably return True, and gives
> different analysis in that case. Use a None return to achieve the same
> effect, but make mypy happy.
> 
> Note: Pylint considers function signatures as code that might trip the
> duplicate-code checker. I'd rather not disable this as it does not
> trigger often in practice, so I'm disabling it as a one-off and filed a
> change request; see https://github.com/PyCQA/pylint/issues/3619
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/machine.py |  8 ++++++--
>  python/qemu/lib/qmp.py     | 10 ++++++++--
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
> index b2f0412197..2f94c851ed 100644
> --- a/python/qemu/lib/machine.py
> +++ b/python/qemu/lib/machine.py
> @@ -24,6 +24,8 @@
>  import shutil
>  import socket
>  import tempfile
> +from typing import Optional, Type
> +from types import TracebackType
>  
>  from . import qmp
>  
> @@ -127,9 +129,11 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
>      def __enter__(self):
>          return self
>  
> -    def __exit__(self, exc_type, exc_val, exc_tb):
> +    def __exit__(self,
> +                 exc_type: Optional[Type[BaseException]],
> +                 exc_val: Optional[BaseException],
> +                 exc_tb: Optional[TracebackType]) -> None:
>          self.shutdown()
> -        return False
>  
>      def add_monitor_null(self):
>          """
> diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
> index 73d49050ed..b91c9d5c1c 100644
> --- a/python/qemu/lib/qmp.py
> +++ b/python/qemu/lib/qmp.py
> @@ -14,7 +14,9 @@
>  from typing import (
>      Optional,
>      TextIO,
> +    Type,
>  )
> +from types import TracebackType
>  
>  
>  class QMPError(Exception):
> @@ -146,10 +148,14 @@ def __enter__(self):
>          # Implement context manager enter function.
>          return self
>  
> -    def __exit__(self, exc_type, exc_value, exc_traceback):
> +    def __exit__(self,
> +                 # pylint: disable=duplicate-code
> +                 # see https://github.com/PyCQA/pylint/issues/3619
> +                 exc_type: Optional[Type[BaseException]],
> +                 exc_val: Optional[BaseException],
> +                 exc_tb: Optional[TracebackType]) -> None:
>          # Implement context manager exit function.
>          self.close()
> -        return False
>  
>      def connect(self, negotiate=True):
>          """
> 

Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH RFC 18/32] python//qmp.py: add casts to JSON deserialization

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> mypy and python type hints are not powerful enough to properly describe
> JSON messages in Python 3.6. The best we can do, generally, is describe
> them as Dict[str, Any].
> 
> Add casts to coerce this type for static analysis; but do NOT enforce
> this type at runtime in any way.
> 
> Note: Python 3.8 adds a TypedDict construct which allows for the
> description of more arbitrary Dictionary shapes. There is a third-party
> module, "Pydantic", which is compatible with 3.6 that can be used
> instead of the JSON library that parses JSON messages to fully-typed
> Python objects, and may be preferable in some cases.
> 
> (That is well beyond the scope of this commit or series.)
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/qmp.py | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
> index 0036204218..e460234f2e 100644
> --- a/python/qemu/lib/qmp.py
> +++ b/python/qemu/lib/qmp.py
> @@ -13,6 +13,7 @@
>  import logging
>  from typing import (
>      Any,
> +    cast,
>      Dict,
>      Optional,
>      TextIO,
> @@ -129,7 +130,10 @@ def __json_read(self, only_event=False):
>              data = self.__sockfile.readline()
>              if not data:
>                  return None
> -            resp = json.loads(data)
> +            # By definition, any JSON received from QMP is a QMPMessage,
> +            # and we are asserting only at static analysis time that it
> +            # has a particular shape.
> +            resp = cast(QMPMessage, json.loads(data))
>              if 'event' in resp:
>                  self.logger.debug("<<< %s", resp)
>                  self.__events.append(resp)
> @@ -261,7 +265,7 @@ def command(self, cmd, **kwds):
>          ret = self.cmd(cmd, kwds)
>          if 'error' in ret:
>              raise QMPResponseError(ret)
> -        return ret['return']
> +        return cast(QMPReturnValue, ret['return'])
>  
>      def pull_event(self, wait=False):
>          """
> 

Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH RFC 20/32] python//qmp.py: assert sockfile is not None

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> In truth, if you don't do this, you'll just get a TypeError
> exception. Now, you'll get an AssertionError.
> 
> Is this tangibly better? No.
> Does mypy complain less? Yes.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/qmp.py | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
> index 5fb16f4b42..1aefc00c93 100644
> --- a/python/qemu/lib/qmp.py
> +++ b/python/qemu/lib/qmp.py
> @@ -132,6 +132,7 @@ def __negotiate_capabilities(self):
>          raise QMPCapabilitiesError
>  
>      def __json_read(self, only_event=False):
> +        assert self.__sockfile is not None
>          while True:
>              data = self.__sockfile.readline()
>              if not data:
> 

Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH RFC 20/32] python//qmp.py: assert sockfile is not None

[Philippe Mathieu-Daudé]

On 5/26/20 6:03 PM, Philippe Mathieu-Daudé wrote:
> On 5/14/20 7:53 AM, John Snow wrote:
>> In truth, if you don't do this, you'll just get a TypeError
>> exception. Now, you'll get an AssertionError.
>>
>> Is this tangibly better? No.
>> Does mypy complain less? Yes.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  python/qemu/lib/qmp.py | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
>> index 5fb16f4b42..1aefc00c93 100644
>> --- a/python/qemu/lib/qmp.py
>> +++ b/python/qemu/lib/qmp.py
>> @@ -132,6 +132,7 @@ def __negotiate_capabilities(self):
>>          raise QMPCapabilitiesError
>>  
>>      def __json_read(self, only_event=False):
>> +        assert self.__sockfile is not None
>>          while True:
>>              data = self.__sockfile.readline()
>>              if not data:
>>
> 
> Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>

I meant:

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[John Snow]

On 5/26/20 11:25 AM, Daniel P. Berrangé wrote:
> On Tue, May 26, 2020 at 05:23:42PM +0200, Philippe Mathieu-Daudé wrote:
>> On 5/26/20 5:22 PM, Daniel P. Berrangé wrote:
>>> On Mon, May 18, 2020 at 08:27:54PM -0400, John Snow wrote:
>>>>
>>>>
>>>> On 5/18/20 3:33 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 18.05.2020 21:23, John Snow wrote:
>>>>>>
>>>>>>
>>>>>> On 5/18/20 2:14 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> 14.05.2020 08:53, John Snow wrote:
>>>>>>>> move python/qemu/*.py to python/qemu/lib/*.py.
>>>>>>>>
>>>>>>>> To create a namespace package, the 'qemu' directory itself shouldn't
>>>>>>>> have module files in it. Thus, these files will go under a 'lib'
>>>>>>>> package
>>>>>>>> directory instead.
>>>>>>>
>>>>>>> Hmm..
>>>>>>>
>>>>>>> On the first glance, it looks better to have
>>>>>>>
>>>>>>>    from qemu import QEMUMachine
>>>>>>>
>>>>>>> than
>>>>>>>      from qemu.lib import QEMUMachine
>>>>>>>
>>>>>>> why do we need this extra ".lib" part?
>>>>>>>
>>>>>>> Is it needed only for internal use?
>>>>>>>
>>>>>>> Assume we have installed qemu package. Can we write
>>>>>>>
>>>>>>>    from qemu import QEMUMachine
>>>>>>>
>>>>>>> ? Or we still need qemu.lib ?
>>>>>>>
>>>>>>> I don't remember any python package, which made me to write "import from
>>>>>>> package_name.lib ..."
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> It's a strategy to create "qemu" as a PEP420 namespace package; i.e.
>>>>>> "qemu" forms a namespace, but you need a name for the actual package
>>>>>> underneath it.
>>>>>>
>>>>>> "qemu.lib" is one package, with qmp, qtest, and machine modules. "qemu"
>>>>>> isn't really a package in this system, it's just a namespace.
>>>>>>
>>>>>> The idea is that this allows us to create a more modular rollout of
>>>>>> various python scripts and services as desired instead of monolithically
>>>>>> bundling them all inside of a "qemu" package.
>>>>>>
>>>>>> It also allows us to fork or split out the sub-packages to separate
>>>>>> repos, if we wish. i.e., let's say we create a "qemu.sdk" subpackage, we
>>>>>> can eventually fork it off into its own repo with its own installer and
>>>>>> so forth. These subpackages can be installed and managed separately.
>>>>>>
>>>>>
>>>>> Okay, I understand.. No real objections than.
>>>>>
>>>>> Still, maybe, everything should not go into lib, maybe something like
>>>>>
>>>>> qemu/vm/  - qmp, QEMUMachine, etc
>>>>> qemu/qtest/  - qtest
>>>>>
>>>>> would be more user friendly? But I'm not sure. I just thought that "lib"
>>>>> is too generic.
>>>>>
>>>>
>>>> lib is a very generic name, I agree.
>>>>
>>>> Splitting accel, qmp and QEMUMachine in one package and keeping qtest in
>>>> another is fine too. I'm not sure if I like "vm" for the name of that
>>>> core package, though.
>>>>
>>>> I want to avoid using "qemu/sdk" because I have some plans for trying to
>>>> generate and package a "real" SDK using that namespace.
>>>>
>>>> "devkit"? "testkit"? "core"? Naming things is always the worst part.
>>>
>>> I'd suggest  "machine", as in
>>>
>>>   from qemu.machine import  kvm_available, QEMUMachine
>>>
>>> I wouldn't over-think the module naming as it has so little impact on
>>> the code usage - it usually only appears in the "import" statement.
>>
>> Don't forget linux-user binaries.
> 
> That's why I suggested ".machine", as all the APIs there currently
> are focused on the machine emulators, and the linx-user binaries
> share essentially nothing in common with softmmu binaries in terms
> of control APIs / CLI config. We can add a "qemu.user" package
> later if we have stuff related to that to expose
> 
I'm re-ordering the series to front-load the linting and type-checking;
and the package organization will now come second, in a separate series.

Module naming isn't a big deal right now, but if we package it and
upload to PyPI it will be something we shouldn't change frivolously.

Daniel, are you suggesting we split it like this? --

- qemu.machine (machine.py, qtest.py, accel.py?)
- qemu.monitor (qmp.py)

the only one that's really truly weird is accel.py?, which is just kind
of a misc function. I guess it can go in `qemu.machine` for now and if
we adopt a `qemu.user` later, we can pull it out into a common area if
we need to.

--js

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[Daniel P. Berrangé]

On Wed, May 27, 2020 at 10:28:44AM -0400, John Snow wrote:
> 
> 
> On 5/26/20 11:25 AM, Daniel P. Berrangé wrote:
> > On Tue, May 26, 2020 at 05:23:42PM +0200, Philippe Mathieu-Daudé wrote:
> >> On 5/26/20 5:22 PM, Daniel P. Berrangé wrote:
> >>> On Mon, May 18, 2020 at 08:27:54PM -0400, John Snow wrote:
> >>>>
> >>>>
> >>>> On 5/18/20 3:33 PM, Vladimir Sementsov-Ogievskiy wrote:
> >>>>> 18.05.2020 21:23, John Snow wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 5/18/20 2:14 PM, Vladimir Sementsov-Ogievskiy wrote:
> >>>>>>> 14.05.2020 08:53, John Snow wrote:
> >>>>>>>> move python/qemu/*.py to python/qemu/lib/*.py.
> >>>>>>>>
> >>>>>>>> To create a namespace package, the 'qemu' directory itself shouldn't
> >>>>>>>> have module files in it. Thus, these files will go under a 'lib'
> >>>>>>>> package
> >>>>>>>> directory instead.
> >>>>>>>
> >>>>>>> Hmm..
> >>>>>>>
> >>>>>>> On the first glance, it looks better to have
> >>>>>>>
> >>>>>>>    from qemu import QEMUMachine
> >>>>>>>
> >>>>>>> than
> >>>>>>>      from qemu.lib import QEMUMachine
> >>>>>>>
> >>>>>>> why do we need this extra ".lib" part?
> >>>>>>>
> >>>>>>> Is it needed only for internal use?
> >>>>>>>
> >>>>>>> Assume we have installed qemu package. Can we write
> >>>>>>>
> >>>>>>>    from qemu import QEMUMachine
> >>>>>>>
> >>>>>>> ? Or we still need qemu.lib ?
> >>>>>>>
> >>>>>>> I don't remember any python package, which made me to write "import from
> >>>>>>> package_name.lib ..."
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>> It's a strategy to create "qemu" as a PEP420 namespace package; i.e.
> >>>>>> "qemu" forms a namespace, but you need a name for the actual package
> >>>>>> underneath it.
> >>>>>>
> >>>>>> "qemu.lib" is one package, with qmp, qtest, and machine modules. "qemu"
> >>>>>> isn't really a package in this system, it's just a namespace.
> >>>>>>
> >>>>>> The idea is that this allows us to create a more modular rollout of
> >>>>>> various python scripts and services as desired instead of monolithically
> >>>>>> bundling them all inside of a "qemu" package.
> >>>>>>
> >>>>>> It also allows us to fork or split out the sub-packages to separate
> >>>>>> repos, if we wish. i.e., let's say we create a "qemu.sdk" subpackage, we
> >>>>>> can eventually fork it off into its own repo with its own installer and
> >>>>>> so forth. These subpackages can be installed and managed separately.
> >>>>>>
> >>>>>
> >>>>> Okay, I understand.. No real objections than.
> >>>>>
> >>>>> Still, maybe, everything should not go into lib, maybe something like
> >>>>>
> >>>>> qemu/vm/  - qmp, QEMUMachine, etc
> >>>>> qemu/qtest/  - qtest
> >>>>>
> >>>>> would be more user friendly? But I'm not sure. I just thought that "lib"
> >>>>> is too generic.
> >>>>>
> >>>>
> >>>> lib is a very generic name, I agree.
> >>>>
> >>>> Splitting accel, qmp and QEMUMachine in one package and keeping qtest in
> >>>> another is fine too. I'm not sure if I like "vm" for the name of that
> >>>> core package, though.
> >>>>
> >>>> I want to avoid using "qemu/sdk" because I have some plans for trying to
> >>>> generate and package a "real" SDK using that namespace.
> >>>>
> >>>> "devkit"? "testkit"? "core"? Naming things is always the worst part.
> >>>
> >>> I'd suggest  "machine", as in
> >>>
> >>>   from qemu.machine import  kvm_available, QEMUMachine
> >>>
> >>> I wouldn't over-think the module naming as it has so little impact on
> >>> the code usage - it usually only appears in the "import" statement.
> >>
> >> Don't forget linux-user binaries.
> > 
> > That's why I suggested ".machine", as all the APIs there currently
> > are focused on the machine emulators, and the linx-user binaries
> > share essentially nothing in common with softmmu binaries in terms
> > of control APIs / CLI config. We can add a "qemu.user" package
> > later if we have stuff related to that to expose
> > 
> I'm re-ordering the series to front-load the linting and type-checking;
> and the package organization will now come second, in a separate series.
> 
> Module naming isn't a big deal right now, but if we package it and
> upload to PyPI it will be something we shouldn't change frivolously.
> 
> Daniel, are you suggesting we split it like this? --
> 
> - qemu.machine (machine.py, qtest.py, accel.py?)
> - qemu.monitor (qmp.py)

I was actually suggesting  everything in qemu.machine, but I guess
qemu.monitor makes sense, given that this is an interface both for
controlling QEMU and the guest agent.

> the only one that's really truly weird is accel.py?, which is just kind
> of a misc function. I guess it can go in `qemu.machine` for now and if
> we adopt a `qemu.user` later, we can pull it out into a common area if
> we need to.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH RFC 26/32] python//machine.py: use qmp.command

[John Snow]

[...]

>  
> -    def qmp(self, cmd, conv_keys=True, **args):
> -        """
> -        Invoke a QMP command and return the response dict
> -        """
> +    @classmethod
> +    def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
>          qmp_args = dict()
>          for key, value in args.items():
> -            if conv_keys:
> +            if _conv_keys:
>                  qmp_args[key.replace('_', '-')] = value
>              else:
>                  qmp_args[key] = value
> +        return qmp_args
>  
> +    def qmp(self, cmd: str,
> +            conv_keys: bool = True,
> +            **args: Any) -> QMPMessage:

This creates an interesting problem with iotests 297:


-Success: no issues found in 1 source file
+iotests.py:563: error: Argument 2 to "qmp" of "QEMUMachine" has
incompatible type "**Dict[str, str]"; expected "bool"
+Found 1 error in 1 file (checked 1 source file)


def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
    cmd = 'human-monitor-command'
    kwargs = {'command-line': command_line}
    if use_log:
        return self.qmp_log(cmd, **kwargs)
    else:
        return self.qmp(cmd, **kwargs)

It seems like mypy is unable to understand that we are passing keyword
arguments, and instead believes we're passing something to the conv_keys
parameter.

(Is this a bug...?)

Even amending the function signature to indicate that conv_keys should
only ever appear as a keyword argument doesn't seem to help.

I'll have to think about a nice way to fix this; removing conv_keys out
of the argument namespace seems like the best approach.

qmp(cmd, foo=bar, hello=world)
qmp(cmd, **conv_keys(foo=bar, hello=world))

...but now this function looks really annoying to call.

Uh, I'll play around with this, but let me know if you have any cool ideas.

--js

Re: [PATCH RFC 05/32] python/qemu/lib: delint; add flake8 config

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> Mostly, ignore the "no bare except" rule, because flake8 is not
> contextual and cannot determine if we re-raise. Pylint can, though, so
> always prefer pylint for that.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/.flake8    |  2 ++
>  python/qemu/lib/accel.py   |  9 ++++++---
>  python/qemu/lib/machine.py | 13 +++++++++----
>  python/qemu/lib/qmp.py     |  4 ++--
>  4 files changed, 19 insertions(+), 9 deletions(-)
>  create mode 100644 python/qemu/lib/.flake8

Thanks, applied to my python-next tree:
https://gitlab.com/philmd/qemu/commits/python-next

Re: [PATCH RFC 11/32] python/qemu/lib: remove Python2 style super() calls

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> Use the Python3 style instead.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/machine.py |  2 +-
>  python/qemu/lib/qtest.py   | 15 +++++++--------
>  2 files changed, 8 insertions(+), 9 deletions(-)

Thanks, applied to my python-next tree:
https://gitlab.com/philmd/qemu/commits/python-next

Re: [PATCH RFC 12/32] python/qemu/lib: fix socket.makefile() typing

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> Note:
> 
> A bug in typeshed (https://github.com/python/typeshed/issues/3977)
> misinterprets the type of makefile(). Work around this by explicitly
> stating that we are opening a text-mode file.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/qmp.py   | 10 +++++++---
>  python/qemu/lib/qtest.py | 12 ++++++++----
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
> index 6ae7693965..73d49050ed 100644
> --- a/python/qemu/lib/qmp.py
> +++ b/python/qemu/lib/qmp.py
> @@ -11,6 +11,10 @@
>  import errno
>  import socket
>  import logging
> +from typing import (
> +    Optional,
> +    TextIO,
> +)
>  
>  
>  class QMPError(Exception):
> @@ -61,7 +65,7 @@ def __init__(self, address, server=False, nickname=None):
>          self.__events = []
>          self.__address = address
>          self.__sock = self.__get_sock()
> -        self.__sockfile = None
> +        self.__sockfile: Optional[TextIO] = None
>          self._nickname = nickname
>          if self._nickname:
>              self.logger = logging.getLogger('QMP').getChild(self._nickname)
> @@ -157,7 +161,7 @@ def connect(self, negotiate=True):
>          @raise QMPCapabilitiesError if fails to negotiate capabilities
>          """
>          self.__sock.connect(self.__address)
> -        self.__sockfile = self.__sock.makefile()
> +        self.__sockfile = self.__sock.makefile(mode='r')
>          if negotiate:
>              return self.__negotiate_capabilities()
>          return None
> @@ -180,7 +184,7 @@ def accept(self, timeout=15.0):
>          """
>          self.__sock.settimeout(timeout)
>          self.__sock, _ = self.__sock.accept()
> -        self.__sockfile = self.__sock.makefile()
> +        self.__sockfile = self.__sock.makefile(mode='r')
>          return self.__negotiate_capabilities()
>  
>      def cmd_obj(self, qmp_cmd):
> diff --git a/python/qemu/lib/qtest.py b/python/qemu/lib/qtest.py
> index 7943487c2b..4c88590eb0 100644
> --- a/python/qemu/lib/qtest.py
> +++ b/python/qemu/lib/qtest.py
> @@ -19,6 +19,7 @@
>  
>  import socket
>  import os
> +from typing import Optional, TextIO
>  
>  from .machine import QEMUMachine
>  
> @@ -40,7 +41,7 @@ class QEMUQtestProtocol:
>      def __init__(self, address, server=False):
>          self._address = address
>          self._sock = self._get_sock()
> -        self._sockfile = None
> +        self._sockfile: Optional[TextIO] = None
>          if server:
>              self._sock.bind(self._address)
>              self._sock.listen(1)
> @@ -59,7 +60,7 @@ def connect(self):
>          @raise socket.error on socket connection errors
>          """
>          self._sock.connect(self._address)
> -        self._sockfile = self._sock.makefile()
> +        self._sockfile = self._sock.makefile(mode='r')
>  
>      def accept(self):
>          """
> @@ -68,7 +69,7 @@ def accept(self):
>          @raise socket.error on socket connection errors
>          """
>          self._sock, _ = self._sock.accept()
> -        self._sockfile = self._sock.makefile()
> +        self._sockfile = self._sock.makefile(mode='r')
>  
>      def cmd(self, qtest_cmd):
>          """
> @@ -76,6 +77,7 @@ def cmd(self, qtest_cmd):
>  
>          @param qtest_cmd: qtest command text to be sent
>          """
> +        assert self._sockfile is not None
>          self._sock.sendall((qtest_cmd + "\n").encode('utf-8'))
>          resp = self._sockfile.readline()
>          return resp
> @@ -83,7 +85,9 @@ def cmd(self, qtest_cmd):
>      def close(self):
>          """Close this socket."""
>          self._sock.close()
> -        self._sockfile.close()
> +        if self._sockfile:
> +            self._sockfile.close()
> +            self._sockfile = None
>  
>      def settimeout(self, timeout):
>          """Set a timeout, in seconds."""
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

And applied to my python-next tree:
https://gitlab.com/philmd/qemu/commits/python-next

Re: [PATCH RFC 13/32] python/qemu/lib: Adjust traceback typing

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> mypy considers it incorrect to use `bool` to statically return false,
> because it will assume that it could conceivably return True, and gives
> different analysis in that case. Use a None return to achieve the same
> effect, but make mypy happy.
> 
> Note: Pylint considers function signatures as code that might trip the
> duplicate-code checker. I'd rather not disable this as it does not
> trigger often in practice, so I'm disabling it as a one-off and filed a
> change request; see https://github.com/PyCQA/pylint/issues/3619
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/machine.py |  8 ++++++--
>  python/qemu/lib/qmp.py     | 10 ++++++++--
>  2 files changed, 14 insertions(+), 4 deletions(-)

Thanks, applied to my python-next tree:
https://gitlab.com/philmd/qemu/commits/python-next

Re: [PATCH RFC 14/32] python//qmp.py: use True/False for non/blocking modes

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> The type system doesn't want integers.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/qmp.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
> index b91c9d5c1c..a634c4e26c 100644
> --- a/python/qemu/lib/qmp.py
> +++ b/python/qemu/lib/qmp.py
> @@ -120,14 +120,14 @@ def __get_events(self, wait=False):
>          """
>  
>          # Check for new events regardless and pull them into the cache:
> -        self.__sock.setblocking(0)
> +        self.__sock.setblocking(False)
>          try:
>              self.__json_read()
>          except OSError as err:
>              if err.errno == errno.EAGAIN:
>                  # No data available
>                  pass
> -        self.__sock.setblocking(1)
> +        self.__sock.setblocking(True)
>  
>          # Wait for new events, if needed.
>          # if wait is 0.0, this means "no wait" and is also implicitly false.
> 

Thanks, applied to my python-next tree:
https://gitlab.com/philmd/qemu/commits/python-next

Re: [PATCH RFC 20/32] python//qmp.py: assert sockfile is not None

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> In truth, if you don't do this, you'll just get a TypeError
> exception. Now, you'll get an AssertionError.
> 
> Is this tangibly better? No.
> Does mypy complain less? Yes.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/qmp.py | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/python/qemu/lib/qmp.py b/python/qemu/lib/qmp.py
> index 5fb16f4b42..1aefc00c93 100644
> --- a/python/qemu/lib/qmp.py
> +++ b/python/qemu/lib/qmp.py
> @@ -132,6 +132,7 @@ def __negotiate_capabilities(self):
>          raise QMPCapabilitiesError
>  
>      def __json_read(self, only_event=False):
> +        assert self.__sockfile is not None
>          while True:
>              data = self.__sockfile.readline()
>              if not data:
> 

Thanks, applied to my python-next tree:
https://gitlab.com/philmd/qemu/commits/python-next

Re: [PATCH RFC 21/32] python//machine.py: remove logging configuration

[Philippe Mathieu-Daudé]

On 5/14/20 7:53 AM, John Snow wrote:
> Python 3.5 and above do not print a warning when logging is not
> configured. As a library, it's best practice to leave logging
> configuration to the client executable.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/machine.py | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/python/qemu/lib/machine.py b/python/qemu/lib/machine.py
> index c31bf7cabb..e92afe8649 100644
> --- a/python/qemu/lib/machine.py
> +++ b/python/qemu/lib/machine.py
> @@ -110,9 +110,6 @@ def __init__(self, binary, args=None, wrapper=None, name=None,
>          self._console_socket = None
>          self._remove_files = []
>  
> -        # just in case logging wasn't configured by the main script:
> -        logging.basicConfig()
> -
>      def __enter__(self):
>          return self
>  
> 

Thanks, applied to my python-next tree:
https://gitlab.com/philmd/qemu/commits/python-next

Re: [PATCH RFC 29/32] python//qtest.py: Check before accessing _qtest

[Philippe Mathieu-Daudé]

On 5/14/20 7:54 AM, John Snow wrote:
> It can be None; so add assertions or exceptions where appropriate to
> guard the access accordingly.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/lib/qtest.py | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/python/qemu/lib/qtest.py b/python/qemu/lib/qtest.py
> index a8be0c782f..05c63a1d58 100644
> --- a/python/qemu/lib/qtest.py
> +++ b/python/qemu/lib/qtest.py
> @@ -126,7 +126,8 @@ def _pre_launch(self):
>          super()._pre_launch()
>          self._qtest = QEMUQtestProtocol(self._qtest_path, server=True)
>  
> -    def _post_launch(self):
> +    def _post_launch(self) -> None:
> +        assert self._qtest is not None
>          super()._post_launch()
>          self._qtest.accept()
>  
> @@ -134,6 +135,13 @@ def _post_shutdown(self):
>          super()._post_shutdown()
>          self._remove_if_exists(self._qtest_path)
>  
> -    def qtest(self, cmd):
> -        '''Send a qtest command to guest'''
> +    def qtest(self, cmd: str) -> str:
> +        """
> +        Send a qtest command to the guest.
> +
> +        :param cmd: qtest command to send
> +        :return: qtest server response
> +        """
> +        if self._qtest is None:
> +            raise RuntimeError("qtest socket not available")
>          return self._qtest.cmd(cmd)
> 

Thanks, applied to my python-next tree:
https://gitlab.com/philmd/qemu/commits/python-next

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[Kevin Wolf]

Am 14.05.2020 um 07:53 hat John Snow geschrieben:
> move python/qemu/*.py to python/qemu/lib/*.py.
> 
> To create a namespace package, the 'qemu' directory itself shouldn't
> have module files in it. Thus, these files will go under a 'lib' package
> directory instead.
> 
> Bolster the lib/__init__.py file a little bit, Make the top-level
> classes and functions available directly inside the `qemu.lib`
> namespace, to facilitate a convenient shorthand:
> 
> > from qemu.lib import QEMUQtestMachine, QEMUMonitorProtocol
> 
> Lastly, update all of the existing import directives.
> 
> (Note: these scripts were not necessarily tested to see if they still
> work. Some of these scripts are in obvious states of disrepair and it is
> beyond the scope of this patch to attempt to fix them.)
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/__init__.py                   | 11 -----

When trying to reproduce your mypy problem, I was wondering why mypy was
complaining that it couldn't find qemu.lib. The reason is that removing
__init__.py from qemu means it's not a valid module any more. If I
recreate it locally, mypy stops complaining.

So I think we need to leave this file here.

Kevin

Re: [PATCH RFC 26/32] python//machine.py: use qmp.command

[Kevin Wolf]

Am 29.05.2020 um 02:18 hat John Snow geschrieben:
> 
> [...]
> 
> >  
> > -    def qmp(self, cmd, conv_keys=True, **args):
> > -        """
> > -        Invoke a QMP command and return the response dict
> > -        """
> > +    @classmethod
> > +    def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
> >          qmp_args = dict()
> >          for key, value in args.items():
> > -            if conv_keys:
> > +            if _conv_keys:
> >                  qmp_args[key.replace('_', '-')] = value
> >              else:
> >                  qmp_args[key] = value
> > +        return qmp_args
> >  
> > +    def qmp(self, cmd: str,
> > +            conv_keys: bool = True,
> > +            **args: Any) -> QMPMessage:
> 
> This creates an interesting problem with iotests 297:
> 
> 
> -Success: no issues found in 1 source file
> +iotests.py:563: error: Argument 2 to "qmp" of "QEMUMachine" has
> incompatible type "**Dict[str, str]"; expected "bool"
> +Found 1 error in 1 file (checked 1 source file)
> 
> 
> def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
>     cmd = 'human-monitor-command'
>     kwargs = {'command-line': command_line}
>     if use_log:
>         return self.qmp_log(cmd, **kwargs)
>     else:
>         return self.qmp(cmd, **kwargs)
> 
> It seems like mypy is unable to understand that we are passing keyword
> arguments, and instead believes we're passing something to the conv_keys
> parameter.
> 
> (Is this a bug...?)

It took some experimenting, but in the end, I think the logic behind it
is reasonable enough. If you have kwargs: Dict[str, T] and pass it as
**kwargs, mypy checks that passing T to _every_ keyword argument is fine
because generally speaking you have to expect that the name of any
argument could be present in kwargs. (In the specific example, of
course, we know all the keys, but mypy doesn't look at the content of
the dict.)

With this in mind, getting rid of the error is very simple because you
only need to make sure that T is Any instead of str:

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7eb714b8e5..0258f1359a 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -556,7 +556,7 @@ class VM(qtest.QEMUQtestMachine):
 
     def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
         cmd = 'human-monitor-command'
-        kwargs = {'command-line': command_line}
+        kwargs: Dict[str, Any] = {'command-line': command_line}
         if use_log:
             return self.qmp_log(cmd, **kwargs)
         else:

Kevin

Re: [PATCH RFC 26/32] python//machine.py: use qmp.command

[Kevin Wolf]

Am 02.06.2020 um 12:18 hat Kevin Wolf geschrieben:
> Am 29.05.2020 um 02:18 hat John Snow geschrieben:
> > 
> > [...]
> > 
> > >  
> > > -    def qmp(self, cmd, conv_keys=True, **args):
> > > -        """
> > > -        Invoke a QMP command and return the response dict
> > > -        """
> > > +    @classmethod
> > > +    def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
> > >          qmp_args = dict()
> > >          for key, value in args.items():
> > > -            if conv_keys:
> > > +            if _conv_keys:
> > >                  qmp_args[key.replace('_', '-')] = value
> > >              else:
> > >                  qmp_args[key] = value
> > > +        return qmp_args
> > >  
> > > +    def qmp(self, cmd: str,
> > > +            conv_keys: bool = True,
> > > +            **args: Any) -> QMPMessage:
> > 
> > This creates an interesting problem with iotests 297:
> > 
> > 
> > -Success: no issues found in 1 source file
> > +iotests.py:563: error: Argument 2 to "qmp" of "QEMUMachine" has
> > incompatible type "**Dict[str, str]"; expected "bool"
> > +Found 1 error in 1 file (checked 1 source file)
> > 
> > 
> > def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
> >     cmd = 'human-monitor-command'
> >     kwargs = {'command-line': command_line}
> >     if use_log:
> >         return self.qmp_log(cmd, **kwargs)
> >     else:
> >         return self.qmp(cmd, **kwargs)
> > 
> > It seems like mypy is unable to understand that we are passing keyword
> > arguments, and instead believes we're passing something to the conv_keys
> > parameter.
> > 
> > (Is this a bug...?)
> 
> It took some experimenting, but in the end, I think the logic behind it
> is reasonable enough. If you have kwargs: Dict[str, T] and pass it as
> **kwargs, mypy checks that passing T to _every_ keyword argument is fine
> because generally speaking you have to expect that the name of any
> argument could be present in kwargs. (In the specific example, of
> course, we know all the keys, but mypy doesn't look at the content of
> the dict.)
> 
> With this in mind, getting rid of the error is very simple because you
> only need to make sure that T is Any instead of str:
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 7eb714b8e5..0258f1359a 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -556,7 +556,7 @@ class VM(qtest.QEMUQtestMachine):
>  
>      def hmp(self, command_line: str, use_log: bool = False) -> QMPResponse:
>          cmd = 'human-monitor-command'
> -        kwargs = {'command-line': command_line}
> +        kwargs: Dict[str, Any] = {'command-line': command_line}
>          if use_log:
>              return self.qmp_log(cmd, **kwargs)
>          else:

There is actually also a second way to get rid of the error, which is
specifying conv_keys explicitly so that the value from kwargs wouldn't
be used (if anything, we would get an exception because the value was
specified twice). So the patch below makes the mypy error go away, too.

I think I prefer the explicit Dict[str, Any] because it doesn't end up
duplicating the default value, but I just wanted to mention that this
option exists, too.

Kevin


diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7eb714b8e5..86b14f6152 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -560,7 +560,7 @@ class VM(qtest.QEMUQtestMachine):
         if use_log:
             return self.qmp_log(cmd, **kwargs)
         else:
-            return self.qmp(cmd, **kwargs)
+            return self.qmp(cmd, True, **kwargs)
 
     def pause_drive(self, drive: str, event: Optional[str] = None) -> None:
         """Pause drive r/w operations"""

Re: [PATCH RFC 03/32] python//machine.py: remove bare except

[Kevin Wolf]

Am 14.05.2020 um 07:53 hat John Snow geschrieben:
> Catch only the timeout error; if there are other problems, allow the
> stack trace to be visible.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Having a visible stack trace is nice, but don't we still want to kill
the qemu process to be sure that it's gone even though we can't wait for
it to be shut down cleanly?

Kevin

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[Vladimir Sementsov-Ogievskiy]

26.05.2020 18:07, Philippe Mathieu-Daudé wrote:
> On 5/19/20 12:54 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 19.05.2020 03:27, John Snow wrote:
>>>
>>>
>>> On 5/18/20 3:33 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> 18.05.2020 21:23, John Snow wrote:
>>>>>
>>>>>
>>>>> On 5/18/20 2:14 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> 14.05.2020 08:53, John Snow wrote:
>>>>>>> move python/qemu/*.py to python/qemu/lib/*.py.
>>>>>>>
>>>>>>> To create a namespace package, the 'qemu' directory itself shouldn't
>>>>>>> have module files in it. Thus, these files will go under a 'lib'
>>>>>>> package
>>>>>>> directory instead.
>>>>>>
>>>>>> Hmm..
>>>>>>
>>>>>> On the first glance, it looks better to have
>>>>>>
>>>>>>      from qemu import QEMUMachine
>>>>>>
>>>>>> than
>>>>>>        from qemu.lib import QEMUMachine
>>>>>>
>>>>>> why do we need this extra ".lib" part?
>>>>>>
>>>>>> Is it needed only for internal use?
>>>>>>
>>>>>> Assume we have installed qemu package. Can we write
>>>>>>
>>>>>>      from qemu import QEMUMachine
>>>>>>
>>>>>> ? Or we still need qemu.lib ?
>>>>>>
>>>>>> I don't remember any python package, which made me to write "import
>>>>>> from
>>>>>> package_name.lib ..."
>>>>>>
>>>>>>
>>>>>
>>>>> It's a strategy to create "qemu" as a PEP420 namespace package; i.e.
>>>>> "qemu" forms a namespace, but you need a name for the actual package
>>>>> underneath it.
>>>>>
>>>>> "qemu.lib" is one package, with qmp, qtest, and machine modules. "qemu"
>>>>> isn't really a package in this system, it's just a namespace.
>>>>>
>>>>> The idea is that this allows us to create a more modular rollout of
>>>>> various python scripts and services as desired instead of
>>>>> monolithically
>>>>> bundling them all inside of a "qemu" package.
>>>>>
>>>>> It also allows us to fork or split out the sub-packages to separate
>>>>> repos, if we wish. i.e., let's say we create a "qemu.sdk"
>>>>> subpackage, we
>>>>> can eventually fork it off into its own repo with its own installer and
>>>>> so forth. These subpackages can be installed and managed separately.
>>>>>
>>>>
>>>> Okay, I understand.. No real objections than.
>>>>
>>>> Still, maybe, everything should not go into lib, maybe something like
>>>>
>>>> qemu/vm/  - qmp, QEMUMachine, etc
>>>> qemu/qtest/  - qtest
> 
> I'm not sure this part is relevant now, as we have not good projection
> of what/who/how this package will be consumed.
> 
> I suppose by VM you mean VirtualMachine. I find it confusing. Maybe
> simply "machine"? We also have 'tools' and 'user-space processes'.
> 
> QMP is protocol, common to all. "qemu.core.qmp"?
> 
> We also have the gdb(stub) protocol, common to machine(system) & user.
> 
> The block layer has its classes, "qemu.block"?

Sounds good. But I see now that we shouldn't care too much about this, as we
just don't know how it all will be used..

> 
>>>>
>>>> would be more user friendly? But I'm not sure. I just thought that "lib"
>>>> is too generic.
>>>>
>>>
>>> lib is a very generic name, I agree.
>>>
>>> Splitting accel, qmp and QEMUMachine in one package and keeping qtest in
>>> another is fine too. I'm not sure if I like "vm" for the name of that
>>> core package, though.
>>>
>>> I want to avoid using "qemu/sdk" because I have some plans for trying to
>>> generate and package a "real" SDK using that namespace.
>>>
>>> "devkit"? "testkit"? "core"? Naming things is always the worst part.
>>>
>>
>> I think, "core" sounds good.
> 
> Agreed.
> 


-- 
Best regards,
Vladimir

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[John Snow]

On 6/2/20 6:08 AM, Kevin Wolf wrote:
> Am 14.05.2020 um 07:53 hat John Snow geschrieben:
>> move python/qemu/*.py to python/qemu/lib/*.py.
>>
>> To create a namespace package, the 'qemu' directory itself shouldn't
>> have module files in it. Thus, these files will go under a 'lib' package
>> directory instead.
>>
>> Bolster the lib/__init__.py file a little bit, Make the top-level
>> classes and functions available directly inside the `qemu.lib`
>> namespace, to facilitate a convenient shorthand:
>>
>>> from qemu.lib import QEMUQtestMachine, QEMUMonitorProtocol
>>
>> Lastly, update all of the existing import directives.
>>
>> (Note: these scripts were not necessarily tested to see if they still
>> work. Some of these scripts are in obvious states of disrepair and it is
>> beyond the scope of this patch to attempt to fix them.)
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  python/qemu/__init__.py                   | 11 -----
> 
> When trying to reproduce your mypy problem, I was wondering why mypy was
> complaining that it couldn't find qemu.lib. The reason is that removing
> __init__.py from qemu means it's not a valid module any more. If I
> recreate it locally, mypy stops complaining.
> 
> So I think we need to leave this file here.
> 
> Kevin
> 

Depends. You'll want --namespace-packages to parse a PEP420 namespace.

(It's not a given we definitely want a PEP420 namespace, but that's what
I created here.)

--js

Re: [PATCH RFC 03/32] python//machine.py: remove bare except

[John Snow]

On 6/2/20 7:01 AM, Kevin Wolf wrote:
> Am 14.05.2020 um 07:53 hat John Snow geschrieben:
>> Catch only the timeout error; if there are other problems, allow the
>> stack trace to be visible.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Having a visible stack trace is nice, but don't we still want to kill
> the qemu process to be sure that it's gone even though we can't wait for
> it to be shut down cleanly?
> 
> Kevin
> 

Ah, yes. I should kill and re-raise. I think Vladimir checked in some
code here recently that kept this patch from applying, so I'll have to
re-tune it anyway.

Thanks for the spot.

--js

Re: [PATCH RFC 26/32] python//machine.py: use qmp.command

[John Snow]

On 6/2/20 6:26 AM, Kevin Wolf wrote:
> I think I prefer the explicit Dict[str, Any] because it doesn't end up
> duplicating the default value, but I just wanted to mention that this
> option exists, too.

Oh, I see what's going on here now! Thanks, that's very helpful! In this
case, for now, I think I like your first solution, too.

Someday in the future, I want to look into strictly typed generated SDK
objects where mypy *does* know the types of every field in a QMP command
object.

That's not today, and it's probably not soon, (and it might be never,)
but until then, explicitly hinting the kwargs to be Dict[str, Any] is
reasonable enough.

Thank you so much!

--js

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[Kevin Wolf]

Am 02.06.2020 um 18:44 hat John Snow geschrieben:
> 
> 
> On 6/2/20 6:08 AM, Kevin Wolf wrote:
> > Am 14.05.2020 um 07:53 hat John Snow geschrieben:
> >> move python/qemu/*.py to python/qemu/lib/*.py.
> >>
> >> To create a namespace package, the 'qemu' directory itself shouldn't
> >> have module files in it. Thus, these files will go under a 'lib' package
> >> directory instead.
> >>
> >> Bolster the lib/__init__.py file a little bit, Make the top-level
> >> classes and functions available directly inside the `qemu.lib`
> >> namespace, to facilitate a convenient shorthand:
> >>
> >>> from qemu.lib import QEMUQtestMachine, QEMUMonitorProtocol
> >>
> >> Lastly, update all of the existing import directives.
> >>
> >> (Note: these scripts were not necessarily tested to see if they still
> >> work. Some of these scripts are in obvious states of disrepair and it is
> >> beyond the scope of this patch to attempt to fix them.)
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> ---
> >>  python/qemu/__init__.py                   | 11 -----
> > 
> > When trying to reproduce your mypy problem, I was wondering why mypy was
> > complaining that it couldn't find qemu.lib. The reason is that removing
> > __init__.py from qemu means it's not a valid module any more. If I
> > recreate it locally, mypy stops complaining.
> > 
> > So I think we need to leave this file here.
> > 
> > Kevin
> > 
> 
> Depends. You'll want --namespace-packages to parse a PEP420 namespace.
> 
> (It's not a given we definitely want a PEP420 namespace, but that's what
> I created here.)

I'm just running 297, so if this is how mypy should be called now, the
series needs to make a change to 297.

Kevin

Re: [PATCH RFC 01/32] python/qemu: create qemu.lib module

[John Snow]

On 6/3/20 5:00 AM, Kevin Wolf wrote:
> Am 02.06.2020 um 18:44 hat John Snow geschrieben:
>>
>>
>> On 6/2/20 6:08 AM, Kevin Wolf wrote:
>>> Am 14.05.2020 um 07:53 hat John Snow geschrieben:
>>>> move python/qemu/*.py to python/qemu/lib/*.py.
>>>>
>>>> To create a namespace package, the 'qemu' directory itself shouldn't
>>>> have module files in it. Thus, these files will go under a 'lib' package
>>>> directory instead.
>>>>
>>>> Bolster the lib/__init__.py file a little bit, Make the top-level
>>>> classes and functions available directly inside the `qemu.lib`
>>>> namespace, to facilitate a convenient shorthand:
>>>>
>>>>> from qemu.lib import QEMUQtestMachine, QEMUMonitorProtocol
>>>>
>>>> Lastly, update all of the existing import directives.
>>>>
>>>> (Note: these scripts were not necessarily tested to see if they still
>>>> work. Some of these scripts are in obvious states of disrepair and it is
>>>> beyond the scope of this patch to attempt to fix them.)
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>  python/qemu/__init__.py                   | 11 -----
>>>
>>> When trying to reproduce your mypy problem, I was wondering why mypy was
>>> complaining that it couldn't find qemu.lib. The reason is that removing
>>> __init__.py from qemu means it's not a valid module any more. If I
>>> recreate it locally, mypy stops complaining.
>>>
>>> So I think we need to leave this file here.
>>>
>>> Kevin
>>>
>>
>> Depends. You'll want --namespace-packages to parse a PEP420 namespace.
>>
>> (It's not a given we definitely want a PEP420 namespace, but that's what
>> I created here.)
> 
> I'm just running 297, so if this is how mypy should be called now, the
> series needs to make a change to 297.
> 
> Kevin
> 

Yes, sorry -- 297 wasn't merged yet when I wrote the 32 patch RFC, and
my followup series to add mypy support is now factored to occur prior to
the library split.

I have corrected this in the more recent postings, sorry for the
inconvenience.