[PATCH v6 24/25] gitlab: add python linters to CI

[John Snow]

Add python3.6 to the fedora container image: we need it to run the
linters against that explicit version to make sure we don't break our
minimum version promise.

Add pipenv so that we can fetch precise versions of pip packages we need
to guarantee test reproducability.

Signed-off-by: John Snow 
---
 .gitlab-ci.yml | 12 
 tests/docker/dockerfiles/fedora.docker |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index dcb6317aace..a371c0c7163 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -779,6 +779,18 @@ check-patch:
 GIT_DEPTH: 1000
   allow_failure: true
 
+
+check-python:
+  stage: test
+  image: $CI_REGISTRY_IMAGE/qemu/fedora:latest
+  script:
+- cd python
+- make venv-check
+  variables:
+GIT_DEPTH: 1000
+  needs:
+job: amd64-fedora-container
+
 check-dco:
   stage: build
   image: $CI_REGISTRY_IMAGE/qemu/centos8:latest
diff --git a/tests/docker/dockerfiles/fedora.docker 
b/tests/docker/dockerfiles/fedora.docker
index 915fdc1845e..6908d69ac37 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -84,6 +84,7 @@ ENV PACKAGES \
 numactl-devel \
 perl \
 perl-Test-Harness \
+pipenv \
 pixman-devel \
 python3 \
 python3-PyYAML \
@@ -93,6 +94,7 @@ ENV PACKAGES \
 python3-pip \
 python3-sphinx \
 python3-virtualenv \
+python3.6 \
 rdma-core-devel \
 SDL2-devel \
 snappy-devel \
-- 
2.30.2

[PATCH v6 25/25] python: add tox support

[John Snow]

This is intended to be a manually run, non-CI script.

Use tox to test the linters against all python versions from 3.6 to
3.9. This will only work if you actually have those versions installed
locally, but Fedora makes this easy:

> sudo dnf install python36 python37 python38 python39

Unlike the pipenv tests (make venv-check), this pulls "whichever"
versions of the python packages, so they are unpinned and may break as
time goes on. In the case that breakages are found, setup.cfg should be
amended accordingly to avoid the bad dependant versions, or the code
should be amended to work around the issue.

Signed-off-by: John Snow 
---
 python/README.rst |  2 ++
 python/.gitignore |  1 +
 python/Makefile   |  7 ++-
 python/setup.cfg  |  1 +
 python/tox.ini| 13 +
 5 files changed, 23 insertions(+), 1 deletion(-)
 create mode 100644 python/tox.ini

diff --git a/python/README.rst b/python/README.rst
index 3e09d20c23c..7360dee32be 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -49,3 +49,5 @@ Files in this directory
   this package; it is referenced by ``setup.cfg``.
 - ``setup.cfg`` houses setuptools package configuration.
 - ``setup.py`` is the setuptools installer used by pip; See above.
+- ``tox.ini`` houses configuration for tox, which runs tests against
+  several Python versions to test compatibility.
diff --git a/python/.gitignore b/python/.gitignore
index e27c99e009c..d92e3f4bcca 100644
--- a/python/.gitignore
+++ b/python/.gitignore
@@ -17,3 +17,4 @@ qemu.egg-info/
 
 # virtual environments (pipenv et al)
 .venv/
+.tox/
diff --git a/python/Makefile b/python/Makefile
index 184f59e5634..a01db823318 100644
--- a/python/Makefile
+++ b/python/Makefile
@@ -16,6 +16,8 @@ help:
@echo ""
@echo "make check:  run linters using the current environment."
@echo ""
+   @echo "make check-tox:  run linters using multiple python versions."
+   @echo ""
@echo "make clean:  remove build output."
@echo ""
@echo "make distclean:  remove venv files, qemu package forwarder, and"
@@ -35,8 +37,11 @@ develop:
 check:
@avocado --config avocado.cfg run tests/
 
+check-tox:
+   @tox
+
 clean:
rm -rf build/ dist/
 
 distclean: clean
-   rm -rf qemu.egg-info/ .venv/
+   rm -rf qemu.egg-info/ .venv/ .tox/
diff --git a/python/setup.cfg b/python/setup.cfg
index 364b68434ca..a7c5f636790 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -33,6 +33,7 @@ devel =
 extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
 exclude = __pycache__,
   .venv,
+  .tox,
 
 [mypy]
 strict = True
diff --git a/python/tox.ini b/python/tox.ini
new file mode 100644
index 000..d8fe503b477
--- /dev/null
+++ b/python/tox.ini
@@ -0,0 +1,13 @@
+# tox (https://tox.readthedocs.io/) is a tool for running tests
+# in multiple virtualenvs. This configuration file will run the
+# test suite on all supported python versions. To use it, "pip install tox"
+# and then run "tox" from this directory.
+
+[tox]
+envlist = py36, py37, py38, py39
+
+[testenv]
+allowlist_externals = make
+deps = .[devel]
+commands =
+make check
-- 
2.30.2

[PATCH v6 22/25] python: add Makefile for some common tasks

[John Snow]

Add "make venv" to create the pipenv-managed virtual environment that
contains our explicitly pinned dependencies.

Add "make check" to run the python linters [in the host execution
environment].

Add "make venv-check" which combines the above two: create/update the
venv, then run the linters in that explicitly managed environment.

Add "make develop" which canonizes the runes needed to get both the
linting pre-requisites (the "[devel]" part), and the editable
live-install (the "-e" part) of these python libraries.

make clean: delete miscellaneous python packaging output possibly
created by pipenv, pip, or other python packaging utilities

make distclean: delete the above, the .venv, and the editable "qemu"
package forwarder (qemu.egg-info) if there is one.

Signed-off-by: John Snow 
---
 python/README.rst |  3 +++
 python/Makefile   | 42 ++
 2 files changed, 45 insertions(+)
 create mode 100644 python/Makefile

diff --git a/python/README.rst b/python/README.rst
index e107bd12a69..3e09d20c23c 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -35,6 +35,9 @@ Files in this directory
 - ``qemu/`` Python package source directory.
 - ``tests/`` Python package tests directory.
 - ``avocado.cfg`` Configuration for the Avocado test-runner.
+  Used by ``make check`` et al.
+- ``Makefile`` provides some common testing/installation invocations.
+  Try ``make help`` to see available targets.
 - ``MANIFEST.in`` is read by python setuptools, it specifies additional files
   that should be included by a source distribution.
 - ``PACKAGE.rst`` is used as the README file that is visible on PyPI.org.
diff --git a/python/Makefile b/python/Makefile
new file mode 100644
index 000..184f59e5634
--- /dev/null
+++ b/python/Makefile
@@ -0,0 +1,42 @@
+.PHONY: help venv venv-check check clean distclean develop
+
+help:
+   @echo "python packaging help:"
+   @echo ""
+   @echo "make venv:   Create pipenv's virtual environment."
+   @echo "NOTE: Requires Python 3.6 and pipenv."
+   @echo "  Will download packages from PyPI."
+   @echo "Hint: (On Fedora): 'sudo dnf install python36 pipenv'"
+   @echo ""
+   @echo "make venv-check: run linters using pipenv's virtual environment."
+   @echo "Hint: If you don't know which test to run, run this one!"
+   @echo ""
+   @echo "make develop:Install deps for 'make check', and"
+   @echo " the qemu libs in editable/development mode."
+   @echo ""
+   @echo "make check:  run linters using the current environment."
+   @echo ""
+   @echo "make clean:  remove build output."
+   @echo ""
+   @echo "make distclean:  remove venv files, qemu package forwarder, and"
+   @echo " everything from 'make clean'."
+
+venv: .venv
+.venv: Pipfile.lock
+   @PIPENV_VENV_IN_PROJECT=1 pipenv sync --dev --keep-outdated
+   @touch .venv
+
+venv-check: venv
+   @pipenv run make check
+
+develop:
+   pip3 install -e .[devel]
+
+check:
+   @avocado --config avocado.cfg run tests/
+
+clean:
+   rm -rf build/ dist/
+
+distclean: clean
+   rm -rf qemu.egg-info/ .venv/
-- 
2.30.2

[PATCH v6 18/25] python/qemu: add isort to pipenv

[John Snow]

isort 5.0.0 through 5.0.4 has a bug that causes it to misinterpret
certain "from ..." clauses that are not related to imports.

isort < 5.1.1 has a bug where it does not handle comments near import
statements correctly.

Require 5.1.2 or greater.

isort can be run with 'isort -c qemu' from the python root.

Signed-off-by: John Snow 
---
 python/Pipfile  | 1 +
 python/Pipfile.lock | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/python/Pipfile b/python/Pipfile
index bb2e34b381d..fb7c8d142ee 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -5,6 +5,7 @@ verify_ssl = true
 
 [dev-packages]
 flake8 = ">=3.6.0"
+isort = ">=5.1.2"
 mypy = ">=0.770"
 pylint = ">=2.7.0"
 
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 2b1567b359c..030d5683147 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
 "_meta": {
 "hash": {
-"sha256": 
"5e5b76eb2f6d8e833b79d9f083f08e0087b3e663275a00b84a6799419aa8a776"
+"sha256": 
"e1f54e4d7fc287bdff731659614f5f9bbea2f1aee122ba04506ab26c4007440e"
 },
 "pipfile-spec": 6,
 "requires": {
@@ -46,7 +46,7 @@
 
"sha256:0a943902919f65c5684ac4e0154b1ad4fac6dcaa5d9f3426b732f1c8b5419be6",
 
"sha256:2bb1680aad211e3c9944dbce1d4ba09a989f04e238296c87fe2139faa26d655d"
 ],
-"markers": "python_version >= '3.6' and python_version < '4.0'",
+"index": "pypi",
 "version": "==5.8.0"
 },
 "lazy-object-proxy": {
-- 
2.30.2

[PATCH v6 19/25] python/qemu: add qemu package itself to pipenv

[John Snow]

This adds the python qemu packages themselves to the pipenv manifest.
'pipenv sync' will create a virtual environment sufficient to use the SDK.
'pipenv sync --dev' will create a virtual environment sufficient to use
and test the SDK (with pylint, mypy, isort, flake8, etc.)

The qemu packages are installed in 'editable' mode; all changes made to
the python package inside the git tree will be reflected in the
installed package without reinstallation. This includes changes made
via git pull and so on.

Signed-off-by: John Snow 
Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 
---
 python/Pipfile  | 1 +
 python/Pipfile.lock | 9 +++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/python/Pipfile b/python/Pipfile
index fb7c8d142ee..fb23455eadd 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -10,6 +10,7 @@ mypy = ">=0.770"
 pylint = ">=2.7.0"
 
 [packages]
+qemu = {editable = true,path = "."}
 
 [requires]
 python_version = "3.6"
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 030d5683147..5d3de43609d 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
 "_meta": {
 "hash": {
-"sha256": 
"e1f54e4d7fc287bdff731659614f5f9bbea2f1aee122ba04506ab26c4007440e"
+"sha256": 
"986164b4c690953890066f288b48c3d84c63df86fc8fa30a26e9001d5b0968e0"
 },
 "pipfile-spec": 6,
 "requires": {
@@ -15,7 +15,12 @@
 }
 ]
 },
-"default": {},
+"default": {
+"qemu": {
+"editable": true,
+"path": "."
+}
+},
 "develop": {
 "astroid": {
 "hashes": [
-- 
2.30.2

[PATCH v6 17/25] python: move .isort.cfg into setup.cfg

[John Snow]

Signed-off-by: John Snow 
Reviewed-by: Cleber Rosa 
---
 python/.isort.cfg | 7 ---
 python/setup.cfg  | 8 
 2 files changed, 8 insertions(+), 7 deletions(-)
 delete mode 100644 python/.isort.cfg

diff --git a/python/.isort.cfg b/python/.isort.cfg
deleted file mode 100644
index 6d0fd6cc0d3..000
--- a/python/.isort.cfg
+++ /dev/null
@@ -1,7 +0,0 @@
-[settings]
-force_grid_wrap=4
-force_sort_within_sections=True
-include_trailing_comma=True
-line_length=72
-lines_after_imports=2
-multi_line_output=3
\ No newline at end of file
diff --git a/python/setup.cfg b/python/setup.cfg
index cdd8477292b..8a2c200a581 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -58,3 +58,11 @@ good-names=i,
 [pylint.similarities]
 # Ignore imports when computing similarities.
 ignore-imports=yes
+
+[isort]
+force_grid_wrap=4
+force_sort_within_sections=True
+include_trailing_comma=True
+line_length=72
+lines_after_imports=2
+multi_line_output=3
-- 
2.30.2

[PATCH v6 16/25] python: add mypy to pipenv

[John Snow]

0.730 appears to be about the oldest version that works with the
features we want, including nice human readable output (to make sure
iotest 297 passes), and type-parameterized Popen generics.

0.770, however, supports adding 'strict' to the config file, so require
at least 0.770.

Now that we are checking a namespace package, we need to tell mypy to
allow PEP420 namespaces, so modify the mypy config as part of the move.

mypy can now be run from the python root by typing 'mypy -p qemu'.

A note on mypy invocation: Running it as "mypy qemu/" changes the import
path detection mechanisms in mypy slightly, and it will fail. See
https://github.com/python/mypy/issues/8584 for a decent entry point with
more breadcrumbs on the various behaviors that contribute to this subtle
difference.

Signed-off-by: John Snow 
Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 
---
 python/Pipfile  |  1 +
 python/Pipfile.lock | 37 -
 python/setup.cfg|  1 +
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/python/Pipfile b/python/Pipfile
index cc43445ef8f..bb2e34b381d 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -5,6 +5,7 @@ verify_ssl = true
 
 [dev-packages]
 flake8 = ">=3.6.0"
+mypy = ">=0.770"
 pylint = ">=2.7.0"
 
 [packages]
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index dd2fafadd61..2b1567b359c 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
 "_meta": {
 "hash": {
-"sha256": 
"4a4a9c6e09d9c5ce670fe4d067052a743dc3eff93c4790f9da31a0db2a66f8fc"
+"sha256": 
"5e5b76eb2f6d8e833b79d9f083f08e0087b3e663275a00b84a6799419aa8a776"
 },
 "pipfile-spec": 6,
 "requires": {
@@ -84,6 +84,41 @@
 ],
 "version": "==0.6.1"
 },
+"mypy": {
+"hashes": [
+
"sha256:0d0a87c0e7e3a9becdfbe936c981d32e5ee0ccda3e0f07e1ef2c3d1a817cf73e",
+
"sha256:25adde9b862f8f9aac9d2d11971f226bd4c8fbaa89fb76bdadb267ef22d10064",
+
"sha256:28fb5479c494b1bab244620685e2eb3c3f988d71fd5d64cc753195e8ed53df7c",
+
"sha256:2f9b3407c58347a452fc0736861593e105139b905cca7d097e413453a1d650b4",
+
"sha256:33f159443db0829d16f0a8d83d94df3109bb6dd801975fe86bacb9bf71628e97",
+
"sha256:3f2aca7f68580dc2508289c729bd49ee929a436208d2b2b6aab15745a70a57df",
+
"sha256:499c798053cdebcaa916eef8cd733e5584b5909f789de856b482cd7d069bdad8",
+
"sha256:4eec37370483331d13514c3f55f446fc5248d6373e7029a29ecb7b7494851e7a",
+
"sha256:552a815579aa1e995f39fd05dde6cd378e191b063f031f2acfe73ce9fb7f9e56",
+
"sha256:5873888fff1c7cf5b71efbe80e0e73153fe9212fafdf8e44adfe4c20ec9f82d7",
+
"sha256:61a3d5b97955422964be6b3baf05ff2ce7f26f52c85dd88db11d5e03e146a3a6",
+
"sha256:674e822aa665b9fd75130c6c5f5ed9564a38c6cea6a6432ce47eafb68ee578c5",
+
"sha256:7ce3175801d0ae5fdfa79b4f0cfed08807af4d075b402b7e294e6aa72af9aa2a",
+
"sha256:9743c91088d396c1a5a3c9978354b61b0382b4e3c440ce83cf77994a43e8c521",
+
"sha256:9f94aac67a2045ec719ffe6111df543bac7874cee01f41928f6969756e030564",
+
"sha256:a26f8ec704e5a7423c8824d425086705e381b4f1dfdef6e3a1edab7ba174ec49",
+
"sha256:abf7e0c3cf117c44d9285cc6128856106183938c68fd4944763003decdcfeb66",
+
"sha256:b09669bcda124e83708f34a94606e01b614fa71931d356c1f1a5297ba11f110a",
+
"sha256:cd07039aa5df222037005b08fbbfd69b3ab0b0bd7a07d7906de75ae52c4e3119",
+
"sha256:d23e0ea196702d918b60c8288561e722bf437d82cb7ef2edcd98cfa38905d506",
+
"sha256:d65cc1df038ef55a99e617431f0553cd77763869eebdf9042403e16089fe746c",
+
"sha256:d7da2e1d5f558c37d6e8c1246f1aec1e7349e4913d8fb3cb289a35de573fe2eb"
+],
+"index": "pypi",
+"version": "==0.812"
+},
+"mypy-extensions": {
+"hashes": [
+
"sha256:090fedd75945a69ae91ce1303b5824f428daf5a028d2f6ab8a299250a846f15d",
+
"sha256:2d82818f5bb3e369420cb3c4060a7970edba416647068eb4c5343488a6c604a8"
+],
+"version": "==0.4.3"
+},
 "pycodestyle": {
 "hashes": [
 
"sha256:514f76d918fcc0b55c6680472f0a37970994e07bbb80725808c17089be302068",
diff --git a/python/setup.cfg b/python/setup.cfg
index a3fcf67f8d2..cdd8477292b 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -28,6 +28,7 @@ exclude = __pycache__,
 strict = True
 python_version = 3.6
 warn_unused_configs = True
+namespace_packages = True
 
 [pylint.messages control]
 # Disable the message, report, category or checker with the given id(s). You
-- 
2.30.2

[PATCH v6 23/25] python: add .gitignore

[John Snow]

Ignore *Python* build and package output (build, dist, qemu.egg-info);
these files are not created as part of a QEMU build.

Ignore miscellaneous cached python confetti (__pycache__, *.pyc,
.mypy_cache).

Ignore .idea (pycharm) .vscode, and .venv (pipenv et al).

Signed-off-by: John Snow 
---
 python/.gitignore | 19 +++
 1 file changed, 19 insertions(+)
 create mode 100644 python/.gitignore

diff --git a/python/.gitignore b/python/.gitignore
new file mode 100644
index 000..e27c99e009c
--- /dev/null
+++ b/python/.gitignore
@@ -0,0 +1,19 @@
+# python bytecode cache
+*.pyc
+__pycache__/
+
+# linter/tooling cache
+.mypy_cache/
+.cache/
+
+# python packaging
+build/
+dist/
+qemu.egg-info/
+
+# editor config
+.idea/
+.vscode/
+
+# virtual environments (pipenv et al)
+.venv/
-- 
2.30.2

[PATCH v6 21/25] python: add avocado-framework and tests

[John Snow]

Try using avocado to manage our various tests; even though right now
they're only invoking shell scripts and not really running any
python-native code.

Create tests/, and add shell scripts which call out to mypy, flake8,
pylint and isort to enforce the standards in this directory.

Add avocado-framework to the setup.cfg development dependencies, and add
avocado.cfg to store some preferences for how we'd like the test output
to look.

Finally, add avocado-framework to the Pipfile environment and lock the
new dependencies. We are using avocado >= 87.0 here to take advantage of
some features that Cleber has helpfully added to make the test output
here *very* friendly and easy to read for developers that might chance
upon the output in Gitlab CI.

[Note: ALL of the dependencies get updated to the most modern versions
that exist at the time of this writing. No way around it that I have
seen. Not ideal, but so it goes.]

Provided you have the right development dependencies (mypy, flake8,
isort, pylint, and now avocado-framework) You should be able to run
"avocado --config avocado.cfg run tests/" from the python folder to run
all of these linters with the correct arguments.

(A forthcoming commit adds the much easier 'make check'.)

Signed-off-by: John Snow 
---
 python/README.rst  |   2 +
 python/Pipfile.lock| 104 ++---
 python/avocado.cfg |  10 
 python/setup.cfg   |   1 +
 python/tests/flake8.sh |   2 +
 python/tests/isort.sh  |   2 +
 python/tests/mypy.sh   |   2 +
 python/tests/pylint.sh |   2 +
 8 files changed, 77 insertions(+), 48 deletions(-)
 create mode 100644 python/avocado.cfg
 create mode 100755 python/tests/flake8.sh
 create mode 100755 python/tests/isort.sh
 create mode 100755 python/tests/mypy.sh
 create mode 100755 python/tests/pylint.sh

diff --git a/python/README.rst b/python/README.rst
index e27ba0130ba..e107bd12a69 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -33,6 +33,8 @@ Files in this directory
 ---
 
 - ``qemu/`` Python package source directory.
+- ``tests/`` Python package tests directory.
+- ``avocado.cfg`` Configuration for the Avocado test-runner.
 - ``MANIFEST.in`` is read by python setuptools, it specifies additional files
   that should be included by a source distribution.
 - ``PACKAGE.rst`` is used as the README file that is visible on PyPI.org.
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 18f3bba08f2..2995ede77cd 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -24,27 +24,35 @@
 "develop": {
 "astroid": {
 "hashes": [
-
"sha256:6b0ed1af831570e500e2437625979eaa3b36011f66ddfc4ce930128610258ca9",
-
"sha256:cd80bf957c49765dce6d92c43163ff9d2abc43132ce64d4b1b47717c6d2522df"
+
"sha256:4db03ab5fc3340cf619dbc25e42c2cc3755154ce6009469766d7143d1fc2ee4e",
+
"sha256:8a398dfce302c13f14bab13e2b14fe385d32b73f4e4853b9bdfb64598baa1975"
 ],
-"markers": "python_version >= '3.6'",
-"version": "==2.5.2"
+"markers": "python_version ~= '3.6'",
+"version": "==2.5.6"
+},
+"avocado-framework": {
+"hashes": [
+
"sha256:3fca7226d7d164f124af8a741e7fa658ff4345a0738ddc32907631fd688b38ed",
+
"sha256:48ac254c0ae2ef0c0ceeb38e3d3df0388718eda8f48b3ab55b30b252839f42b1"
+],
+"markers": "python_version >= '3.5'",
+"version": "==87.0"
 },
 "flake8": {
 "hashes": [
-
"sha256:12d05ab02614b6aee8df7c36b97d1a3b2372761222b19b58621355e82acddcff",
-
"sha256:78873e372b12b093da7b5e5ed302e8ad9e988b38b063b61ad937f26ca58fc5f0"
+
"sha256:07528381786f2a6237b061f6e96610a4167b226cb926e2aa2b6b1d78057c576b",
+
"sha256:bf8fd46d844f616e8d47905ef3a3384edae6b4e9beb0c5101e25e3110907"
 ],
 "markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3, 3.4'",
-"version": "==3.9.0"
+"version": "==3.9.2"
 },
 "importlib-metadata": {
 "hashes": [
-
"sha256:1cedf994a9b6885dcbb7ed40b24c332b1de3956319f4b1a0f07c0621d453accc",
-
"sha256:c9c1b6c7dbc62084f3e6a614a194eb16ded7947736c18e3300125d5c0a7a8b3c"
+
"sha256:8c501196e49fb9df5df43833bdb1e4328f64847763ec8a50703148b73784d581",
+
"sha256:d7eb1dea6d6a6086f8be21784cc9e3bcfa55872b52309bc5fad53a8ea65d"
 ],
 "markers": "python_version < '3.8'",
-"version": "==3.9.1"
+"version": "==4.0.1"
 },
 "isort": {
 "hashes": [
@@ -142,11 +150,11 @@
 },
 "pylint": {
 "hashes": [
-
"sha256:0e21d3b80b96740909d77206d741aa3ce0b06b41be375d92e1f3244a274c1f8a",
-

[PATCH v6 20/25] python: add devel package requirements to setuptools

[John Snow]

setuptools doesn't have a formal understanding of development requires,
but it has an optional feataures section. Fine; add a "devel" feature
and add the requirements to it.

To avoid duplication, we can modify pipenv to install qemu[devel]
instead. This enables us to run invocations like "pip install -e
.[devel]" and test the package on bleeding-edge packages beyond those
specified in Pipfile.lock.

Importantly, this also allows us to install the qemu development
packages in a non-networked mode: `pip3 install --no-index -e .[devel]`
will now fail if the proper development dependencies are not already
met. This can be useful for automated build scripts where fetching
network packages may be undesirable.

Signed-off-by: John Snow 
---
 python/Pipfile  |  5 +
 python/Pipfile.lock | 14 +-
 python/setup.cfg|  9 +
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/python/Pipfile b/python/Pipfile
index fb23455eadd..e7acb8cefa4 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -4,10 +4,7 @@ url = "https://pypi.org/simple;
 verify_ssl = true
 
 [dev-packages]
-flake8 = ">=3.6.0"
-isort = ">=5.1.2"
-mypy = ">=0.770"
-pylint = ">=2.7.0"
+qemu = {editable = true, extras = ["devel"], path = "."}
 
 [packages]
 qemu = {editable = true,path = "."}
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 5d3de43609d..18f3bba08f2 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
 "_meta": {
 "hash": {
-"sha256": 
"986164b4c690953890066f288b48c3d84c63df86fc8fa30a26e9001d5b0968e0"
+"sha256": 
"eff562a688ebc6f3ffe67494dbb804b883e2159ad81c4d55d96da9f7aec13e91"
 },
 "pipfile-spec": 6,
 "requires": {
@@ -35,7 +35,7 @@
 
"sha256:12d05ab02614b6aee8df7c36b97d1a3b2372761222b19b58621355e82acddcff",
 
"sha256:78873e372b12b093da7b5e5ed302e8ad9e988b38b063b61ad937f26ca58fc5f0"
 ],
-"index": "pypi",
+"markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3, 3.4'",
 "version": "==3.9.0"
 },
 "importlib-metadata": {
@@ -51,7 +51,7 @@
 
"sha256:0a943902919f65c5684ac4e0154b1ad4fac6dcaa5d9f3426b732f1c8b5419be6",
 
"sha256:2bb1680aad211e3c9944dbce1d4ba09a989f04e238296c87fe2139faa26d655d"
 ],
-"index": "pypi",
+"markers": "python_version >= '3.6' and python_version < '4.0'",
 "version": "==5.8.0"
 },
 "lazy-object-proxy": {
@@ -114,7 +114,7 @@
 
"sha256:d65cc1df038ef55a99e617431f0553cd77763869eebdf9042403e16089fe746c",
 
"sha256:d7da2e1d5f558c37d6e8c1246f1aec1e7349e4913d8fb3cb289a35de573fe2eb"
 ],
-"index": "pypi",
+"markers": "python_version >= '3.5'",
 "version": "==0.812"
 },
 "mypy-extensions": {
@@ -145,9 +145,13 @@
 
"sha256:0e21d3b80b96740909d77206d741aa3ce0b06b41be375d92e1f3244a274c1f8a",
 
"sha256:d09b0b07ba06bcdff463958f53f23df25e740ecd81895f7d2699ec04bbd8dc3b"
 ],
-"index": "pypi",
+"markers": "python_version ~= '3.6'",
 "version": "==2.7.2"
 },
+"qemu": {
+"editable": true,
+"path": "."
+},
 "toml": {
 "hashes": [
 
"sha256:806143ae5bfb6a3c6e736a764057db0e6a0e05e338b5630894a5f779cabb4f9b",
diff --git a/python/setup.cfg b/python/setup.cfg
index 8a2c200a581..9d941386921 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -19,6 +19,15 @@ classifiers =
 python_requires = >= 3.6
 packages = find_namespace:
 
+[options.extras_require]
+# Run `pipenv lock` when changing these requirements.
+devel =
+flake8 >= 3.6.0
+isort >= 5.1.2
+mypy >= 0.770
+pylint >= 2.7.0
+
+
 [flake8]
 extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
 exclude = __pycache__,
-- 
2.30.2

[PATCH v6 15/25] python: move mypy.ini into setup.cfg

[John Snow]

mypy supports reading its configuration values from a central project
configuration file; do so.

Signed-off-by: John Snow 
---
 python/mypy.ini  | 4 
 python/setup.cfg | 5 +
 2 files changed, 5 insertions(+), 4 deletions(-)
 delete mode 100644 python/mypy.ini

diff --git a/python/mypy.ini b/python/mypy.ini
deleted file mode 100644
index 1a581c5f1ea..000
--- a/python/mypy.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[mypy]
-strict = True
-python_version = 3.6
-warn_unused_configs = True
diff --git a/python/setup.cfg b/python/setup.cfg
index f21a1c42fc0..a3fcf67f8d2 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -24,6 +24,11 @@ extend-ignore = E722  # Prefer pylint's bare-except checks 
to flake8's
 exclude = __pycache__,
   .venv,
 
+[mypy]
+strict = True
+python_version = 3.6
+warn_unused_configs = True
+
 [pylint.messages control]
 # Disable the message, report, category or checker with the given id(s). You
 # can either give multiple identifiers separated by comma (,) or put this
-- 
2.30.2

[PATCH v6 10/25] python: move pylintrc into setup.cfg

[John Snow]

Delete the empty settings now that it's sharing a home with settings for
other tools.

pylint can now be run from this folder as "pylint qemu".

Signed-off-by: John Snow 
Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 
---
 python/qemu/machine/pylintrc | 58 
 python/setup.cfg | 29 ++
 2 files changed, 29 insertions(+), 58 deletions(-)
 delete mode 100644 python/qemu/machine/pylintrc

diff --git a/python/qemu/machine/pylintrc b/python/qemu/machine/pylintrc
deleted file mode 100644
index 3f69205000d..000
--- a/python/qemu/machine/pylintrc
+++ /dev/null
@@ -1,58 +0,0 @@
-[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,
-   c,
-[VARIABLES]
-
-[STRING]
-
-[SPELLING]
-
-[FORMAT]
-
-[SIMILARITIES]
-
-# Ignore imports when computing similarities.
-ignore-imports=yes
-
-[TYPECHECK]
-
-[CLASSES]
-
-[IMPORTS]
-
-[DESIGN]
-
-[EXCEPTIONS]
diff --git a/python/setup.cfg b/python/setup.cfg
index e7f8ab23815..20b24372a4a 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -18,3 +18,32 @@ classifiers =
 [options]
 python_requires = >= 3.6
 packages = find_namespace:
+
+[pylint.messages control]
+# Disable the message, report, category or checker with the given id(s). You
+# can either give multiple identifiers separated by comma (,) or put this
+# 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,
+
+[pylint.basic]
+# Good variable names which should always be accepted, separated by a comma.
+good-names=i,
+   j,
+   k,
+   ex,
+   Run,
+   _,
+   fd,
+   c,
+
+[pylint.similarities]
+# Ignore imports when computing similarities.
+ignore-imports=yes
-- 
2.30.2

[PATCH v6 09/25] python: add pylint import exceptions

[John Snow]

Pylint 2.5.x - 2.7.x have regressions that make import checking
inconsistent, see:

https://github.com/PyCQA/pylint/issues/3609
https://github.com/PyCQA/pylint/issues/3624
https://github.com/PyCQA/pylint/issues/3651

Pinning to 2.4.4 is worse, because it mandates versions of shared
dependencies that are too old for features we want in isort and mypy.
Oh well.

Signed-off-by: John Snow 
Reviewed-by: Cleber Rosa 
---
 python/qemu/machine/__init__.py | 3 +++
 python/qemu/machine/machine.py  | 2 +-
 python/qemu/machine/qtest.py| 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
index 98302ea31e7..728f27adbed 100644
--- a/python/qemu/machine/__init__.py
+++ b/python/qemu/machine/__init__.py
@@ -22,6 +22,9 @@
 # the COPYING file in the top-level directory.
 #
 
+# pylint: disable=import-error
+# see: https://github.com/PyCQA/pylint/issues/3624
+# see: https://github.com/PyCQA/pylint/issues/3651
 from .machine import QEMUMachine
 from .qtest import QEMUQtestMachine, QEMUQtestProtocol
 
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index dea343afeba..6fe41d83cb5 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -38,7 +38,7 @@
 Type,
 )
 
-from qemu.qmp import (
+from qemu.qmp import (  # pylint: disable=import-error
 QEMUMonitorProtocol,
 QMPMessage,
 QMPReturnValue,
diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index 53926e434a7..c3adf4e3012 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -26,7 +26,7 @@
 TextIO,
 )
 
-from qemu.qmp import SocketAddrT
+from qemu.qmp import SocketAddrT  # pylint: disable=import-error
 
 from .machine import QEMUMachine
 
-- 
2.30.2

[PATCH v6 14/25] python: Add flake8 to pipenv

[John Snow]

flake8 3.5.x does not support the --extend-ignore syntax used in the
.flake8 file to gracefully extend default ignores, so 3.6.x is our
minimum requirement. There is no known upper bound.

flake8 can be run from the python/ directory with no arguments.

Signed-off-by: John Snow 
Reviewed-by: Cleber Rosa 
Tested-by: Cleber Rosa 
---
 python/Pipfile  |  1 +
 python/Pipfile.lock | 55 ++---
 2 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/python/Pipfile b/python/Pipfile
index aaee00581eb..cc43445ef8f 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -4,6 +4,7 @@ url = "https://pypi.org/simple;
 verify_ssl = true
 
 [dev-packages]
+flake8 = ">=3.6.0"
 pylint = ">=2.7.0"
 
 [packages]
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index b5e556d265b..dd2fafadd61 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
 "_meta": {
 "hash": {
-"sha256": 
"2016d2670f203ed4ffac9f59cdfbc540f209ad76eff28e484f245f401d524f5c"
+"sha256": 
"4a4a9c6e09d9c5ce670fe4d067052a743dc3eff93c4790f9da31a0db2a66f8fc"
 },
 "pipfile-spec": 6,
 "requires": {
@@ -25,6 +25,22 @@
 "markers": "python_version >= '3.6'",
 "version": "==2.5.2"
 },
+"flake8": {
+"hashes": [
+
"sha256:12d05ab02614b6aee8df7c36b97d1a3b2372761222b19b58621355e82acddcff",
+
"sha256:78873e372b12b093da7b5e5ed302e8ad9e988b38b063b61ad937f26ca58fc5f0"
+],
+"index": "pypi",
+"version": "==3.9.0"
+},
+"importlib-metadata": {
+"hashes": [
+
"sha256:1cedf994a9b6885dcbb7ed40b24c332b1de3956319f4b1a0f07c0621d453accc",
+
"sha256:c9c1b6c7dbc62084f3e6a614a194eb16ded7947736c18e3300125d5c0a7a8b3c"
+],
+"markers": "python_version < '3.8'",
+"version": "==3.9.1"
+},
 "isort": {
 "hashes": [
 
"sha256:0a943902919f65c5684ac4e0154b1ad4fac6dcaa5d9f3426b732f1c8b5419be6",
@@ -68,6 +84,22 @@
 ],
 "version": "==0.6.1"
 },
+"pycodestyle": {
+"hashes": [
+
"sha256:514f76d918fcc0b55c6680472f0a37970994e07bbb80725808c17089be302068",
+
"sha256:c389c1d06bf7904078ca03399a4816f974a1d590090fecea0c63ec26ebaf1cef"
+],
+"markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3'",
+"version": "==2.7.0"
+},
+"pyflakes": {
+"hashes": [
+
"sha256:7893783d01b8a89811dd72d7dfd4d84ff098e5eed95cfa8905b22bbffe52efc3",
+
"sha256:f5bc8ecabc05bb9d291eb5203d6810b49040f6ff446a756326104746cc00c1db"
+],
+"markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3'",
+"version": "==2.3.1"
+},
 "pylint": {
 "hashes": [
 
"sha256:0e21d3b80b96740909d77206d741aa3ce0b06b41be375d92e1f3244a274c1f8a",
@@ -81,7 +113,7 @@
 
"sha256:806143ae5bfb6a3c6e736a764057db0e6a0e05e338b5630894a5f779cabb4f9b",
 
"sha256:b3bda1d108d5dd99f4a20d24d9c348e91c4db7ab1b749200bded2f839ccbe68f"
 ],
-"markers": "python_version >= '2.6' and python_version not in 
'3.0, 3.1, 3.2, 3.3'",
+"markers": "python_version >= '2.6' and python_version not in 
'3.0, 3.1, 3.2'",
 "version": "==0.10.2"
 },
 "typed-ast": {
@@ -117,14 +149,31 @@
 
"sha256:d746a437cdbca200622385305aedd9aef68e8a645e385cc483bdc5e488f07166",
 
"sha256:e683e409e5c45d5c9082dc1daf13f6374300806240719f95dc783d1fc942af10"
 ],
-"markers": "python_version < '3.8' and implementation_name == 
'cpython'",
+"markers": "implementation_name == 'cpython' and python_version < 
'3.8'",
 "version": "==1.4.2"
 },
+"typing-extensions": {
+"hashes": [
+
"sha256:7cb407020f00f7bfc3cb3e7881628838e69d8f3fcab2f64742a5e76b2f841918",
+
"sha256:99d4073b617d30288f569d3f13d2bd7548c3a7e4c8de87db09a9d29bb3a4a60c",
+
"sha256:dafc7639cde7f1b6e1acc0f457842a83e722ccca8eef5270af2d74792619a89f"
+],
+"markers": "python_version < '3.8'",
+"version": "==3.7.4.3"
+},
 "wrapt": {
 "hashes": [
 
"sha256:b62ffa81fb85f4332a4f609cab4ac40709470da05643a082ec1eb88e6d9b97d7"
 ],
 "version": "==1.12.1"
+},
+"zipp": {
+"hashes": [
+
"sha256:3607921face881ba3e026887d8150cca609d517579abe052ac81fc5aeffdbd76",
+
"sha256:51cb66cc54621609dd593d1787f286ee42a5c0adbb4b29abea5a63edc3e03098"
+],
+"markers": 

[PATCH v6 06/25] python: add directory structure README.rst files

[John Snow]

Add short readmes to python/, python/qemu/, python/qemu/machine,
python/qemu/qmp, and python/qemu/utils that explain the directory
hierarchy. These readmes are visible when browsing the source on
e.g. gitlab/github and are designed to help new developers/users quickly
make sense of the source tree.

They are not designed for inclusion in a published manual.

Signed-off-by: John Snow 
---
 python/README.rst  | 41 ++
 python/qemu/README.rst |  8 +++
 python/qemu/machine/README.rst |  9 
 python/qemu/qmp/README.rst |  9 
 python/qemu/utils/README.rst   |  7 ++
 5 files changed, 74 insertions(+)
 create mode 100644 python/README.rst
 create mode 100644 python/qemu/README.rst
 create mode 100644 python/qemu/machine/README.rst
 create mode 100644 python/qemu/qmp/README.rst
 create mode 100644 python/qemu/utils/README.rst

diff --git a/python/README.rst b/python/README.rst
new file mode 100644
index 000..7a0dc5dff4a
--- /dev/null
+++ b/python/README.rst
@@ -0,0 +1,41 @@
+QEMU Python Tooling
+===
+
+This directory houses Python tooling used by the QEMU project to build,
+configure, and test QEMU. It is organized by namespace (``qemu``), and
+then by package (e.g. ``qemu/machine``, ``qemu/qmp``, etc).
+
+``setup.py`` is used by ``pip`` to install this tooling to the current
+environment. ``setup.cfg`` provides the packaging configuration used by
+setup.py in a setuptools specific format. You will generally invoke it
+by doing one of the following:
+
+1. ``pip3 install .`` will install these packages to your current
+   environment. If you are inside a virtual environment, they will
+   install there. If you are not, it will attempt to install to the
+   global environment, which is not recommended.
+
+2. ``pip3 install --user .`` will install these packages to your user's
+   local python packages. If you are inside of a virtual environment,
+   this will fail.
+
+If you append the ``-e`` argument, pip will install in "editable" mode;
+which installs a version of the package that installs a forwarder
+pointing to these files, such that the package always reflects the
+latest version in your git tree.
+
+See `Installing packages using pip and virtual environments
+`_
+for more information.
+
+
+Files in this directory
+---
+
+- ``qemu/`` Python package source directory.
+- ``PACKAGE.rst`` is used as the README file that is visible on PyPI.org.
+- ``README.rst`` you are here!
+- ``VERSION`` contains the PEP-440 compliant version used to describe
+  this package; it is referenced by ``setup.cfg``.
+- ``setup.cfg`` houses setuptools package configuration.
+- ``setup.py`` is the setuptools installer used by pip; See above.
diff --git a/python/qemu/README.rst b/python/qemu/README.rst
new file mode 100644
index 000..d04943f526c
--- /dev/null
+++ b/python/qemu/README.rst
@@ -0,0 +1,8 @@
+QEMU Python Namespace
+=
+
+This directory serves as the root of a `Python PEP 420 implicit
+namespace package `_.
+
+Each directory below is assumed to be an installable Python package that
+is available under the ``qemu.`` namespace.
diff --git a/python/qemu/machine/README.rst b/python/qemu/machine/README.rst
new file mode 100644
index 000..ac2b4fffb42
--- /dev/null
+++ b/python/qemu/machine/README.rst
@@ -0,0 +1,9 @@
+qemu.machine package
+
+
+This package provides core utilities used for testing and debugging
+QEMU. It is used by the iotests, vm tests, acceptance tests, and several
+other utilities in the ./scripts directory. It is not a fully-fledged
+SDK and it is subject to change at any time.
+
+See the documentation in ``__init__.py`` for more information.
diff --git a/python/qemu/qmp/README.rst b/python/qemu/qmp/README.rst
new file mode 100644
index 000..c21951491cf
--- /dev/null
+++ b/python/qemu/qmp/README.rst
@@ -0,0 +1,9 @@
+qemu.qmp package
+
+
+This package provides a library used for connecting to and communicating
+with QMP servers. It is used extensively by iotests, vm tests,
+acceptance tests, and other utilities in the ./scripts directory. It is
+not a fully-fledged SDK and is subject to change at any time.
+
+See the documentation in ``__init__.py`` for more information.
diff --git a/python/qemu/utils/README.rst b/python/qemu/utils/README.rst
new file mode 100644
index 000..975fbf4d7de
--- /dev/null
+++ b/python/qemu/utils/README.rst
@@ -0,0 +1,7 @@
+qemu.utils package
+==
+
+This package provides miscellaneous utilities used for testing and
+debugging QEMU. It is used primarily by the vm and acceptance tests.
+
+See the documentation in ``__init__.py`` for more information.
-- 
2.30.2

[PATCH v6 13/25] python: add excluded dirs to flake8 config

[John Snow]

Instruct flake8 to avoid certain well-known directories created by
python tooling that it ought not check.

Note that at-present, nothing actually creates a ".venv" directory; but
it is in such widespread usage as a de-facto location for a developer's
virtual environment that it should be excluded anyway. A forthcoming
commit canonizes this with a "make venv" command.

Signed-off-by: John Snow 
---
 python/setup.cfg | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/python/setup.cfg b/python/setup.cfg
index 9ecb2902006..f21a1c42fc0 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -21,6 +21,8 @@ packages = find_namespace:
 
 [flake8]
 extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
+exclude = __pycache__,
+  .venv,
 
 [pylint.messages control]
 # Disable the message, report, category or checker with the given id(s). You
-- 
2.30.2

[PATCH v6 12/25] python: move flake8 config to setup.cfg

[John Snow]

Update the comment concerning the flake8 exception to match commit
42c0dd12, whose commit message stated:

A note on the flake8 exception: flake8 will warn on *any* bare except,
but pylint's is context-aware and will suppress the warning if you
re-raise the exception.

Signed-off-by: John Snow 
Reviewed-by: Cleber Rosa 
---
 python/qemu/machine/.flake8 | 2 --
 python/setup.cfg| 3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)
 delete mode 100644 python/qemu/machine/.flake8

diff --git a/python/qemu/machine/.flake8 b/python/qemu/machine/.flake8
deleted file mode 100644
index 45d8146f3f5..000
--- a/python/qemu/machine/.flake8
+++ /dev/null
@@ -1,2 +0,0 @@
-[flake8]
-extend-ignore = E722  # Pylint handles this, but smarter.
\ No newline at end of file
diff --git a/python/setup.cfg b/python/setup.cfg
index 20b24372a4a..9ecb2902006 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -19,6 +19,9 @@ classifiers =
 python_requires = >= 3.6
 packages = find_namespace:
 
+[flake8]
+extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
+
 [pylint.messages control]
 # Disable the message, report, category or checker with the given id(s). You
 # can either give multiple identifiers separated by comma (,) or put this
-- 
2.30.2

[PATCH v6 08/25] python: Add pipenv support

[John Snow]

pipenv is a tool used for managing virtual environments with pinned,
explicit dependencies. It is used for precisely recreating python
virtual environments.

pipenv uses two files to do this:

(1) Pipfile, which is similar in purpose and scope to what setup.cfg
lists. It specifies the requisite minimum to get a functional
environment for using this package.

(2) Pipfile.lock, which is similar in purpose to `pip freeze >
requirements.txt`. It specifies a canonical virtual environment used for
deployment or testing. This ensures that all users have repeatable
results.

The primary benefit of using this tool is to ensure *rock solid*
repeatable CI results with a known set of packages. Although I endeavor
to support as many versions as I can, the fluid nature of the Python
toolchain often means tailoring code for fairly specific versions.

Note that pipenv is *not* required to install or use this module; this is
purely for the sake of repeatable testing by CI or developers.

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

Pipfile will specify our version minimums, while Pipfile.lock specifies
an exact loadout of packages that were known to operate correctly. This
latter file provides the real value for easy setup of container images
and CI environments.

Signed-off-by: John Snow 
Reviewed-by: Cleber Rosa 
---
 python/README.rst |  3 +++
 python/Pipfile| 11 +++
 2 files changed, 14 insertions(+)
 create mode 100644 python/Pipfile

diff --git a/python/README.rst b/python/README.rst
index 86364367261..e27ba0130ba 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -36,6 +36,9 @@ Files in this directory
 - ``MANIFEST.in`` is read by python setuptools, it specifies additional files
   that should be included by a source distribution.
 - ``PACKAGE.rst`` is used as the README file that is visible on PyPI.org.
+- ``Pipfile`` is used by Pipenv to generate ``Pipfile.lock``.
+- ``Pipfile.lock`` is a set of pinned package dependencies that this package
+  is tested under in our CI suite. It is used by ``make venv-check``.
 - ``README.rst`` you are here!
 - ``VERSION`` contains the PEP-440 compliant version used to describe
   this package; it is referenced by ``setup.cfg``.
diff --git a/python/Pipfile b/python/Pipfile
new file mode 100644
index 000..9534830b5eb
--- /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.30.2

[PATCH v6 11/25] python: add pylint to pipenv

[John Snow]

We are specifying >= pylint 2.7.x for several reasons:

1. For setup.cfg support, added in pylint 2.5.x
2. To specify a version that has incompatibly dropped
   bad-whitespace checks (2.6.x)
3. 2.7.x fixes "unsubscriptable" warnings in Python 3.9

Signed-off-by: John Snow 
---
 python/Pipfile  |   1 +
 python/Pipfile.lock | 130 
 2 files changed, 131 insertions(+)
 create mode 100644 python/Pipfile.lock

diff --git a/python/Pipfile b/python/Pipfile
index 9534830b5eb..aaee00581eb 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -4,6 +4,7 @@ url = "https://pypi.org/simple;
 verify_ssl = true
 
 [dev-packages]
+pylint = ">=2.7.0"
 
 [packages]
 
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
new file mode 100644
index 000..b5e556d265b
--- /dev/null
+++ b/python/Pipfile.lock
@@ -0,0 +1,130 @@
+{
+"_meta": {
+"hash": {
+"sha256": 
"2016d2670f203ed4ffac9f59cdfbc540f209ad76eff28e484f245f401d524f5c"
+},
+"pipfile-spec": 6,
+"requires": {
+"python_version": "3.6"
+},
+"sources": [
+{
+"name": "pypi",
+"url": "https://pypi.org/simple;,
+"verify_ssl": true
+}
+]
+},
+"default": {},
+"develop": {
+"astroid": {
+"hashes": [
+
"sha256:6b0ed1af831570e500e2437625979eaa3b36011f66ddfc4ce930128610258ca9",
+
"sha256:cd80bf957c49765dce6d92c43163ff9d2abc43132ce64d4b1b47717c6d2522df"
+],
+"markers": "python_version >= '3.6'",
+"version": "==2.5.2"
+},
+"isort": {
+"hashes": [
+
"sha256:0a943902919f65c5684ac4e0154b1ad4fac6dcaa5d9f3426b732f1c8b5419be6",
+
"sha256:2bb1680aad211e3c9944dbce1d4ba09a989f04e238296c87fe2139faa26d655d"
+],
+"markers": "python_version >= '3.6' and python_version < '4.0'",
+"version": "==5.8.0"
+},
+"lazy-object-proxy": {
+"hashes": [
+
"sha256:17e0967ba374fc24141738c69736da90e94419338fd4c7c7bef01ee26b339653",
+
"sha256:1fee665d2638491f4d6e55bd483e15ef21f6c8c2095f235fef72601021e64f61",
+
"sha256:22ddd618cefe54305df49e4c069fa65715be4ad0e78e8d252a33debf00f6ede2",
+
"sha256:24a5045889cc2729033b3e604d496c2b6f588c754f7a62027ad4437a7ecc4837",
+
"sha256:410283732af311b51b837894fa2f24f2c0039aa7f220135192b38fcc42bd43d3",
+
"sha256:4732c765372bd78a2d6b2150a6e99d00a78ec963375f236979c0626b97ed8e43",
+
"sha256:489000d368377571c6f982fba6497f2aa13c6d1facc40660963da62f5c379726",
+
"sha256:4f60460e9f1eb632584c9685bccea152f4ac2130e299784dbaf9fae9f49891b3",
+
"sha256:5743a5ab42ae40caa8421b320ebf3a998f89c85cdc8376d6b2e00bd12bd1b587",
+
"sha256:85fb7608121fd5621cc4377a8961d0b32ccf84a7285b4f1d21988b2eae2868e8",
+
"sha256:9698110e36e2df951c7c36b6729e96429c9c32b3331989ef19976592c5f3c77a",
+
"sha256:9d397bf41caad3f489e10774667310d73cb9c4258e9aed94b9ec734b34b495fd",
+
"sha256:b579f8acbf2bdd9ea200b1d5dea36abd93cabf56cf626ab9c744a432e15c815f",
+
"sha256:b865b01a2e7f96db0c5d12cfea590f98d8c5ba64ad222300d93ce6ff9138bcad",
+
"sha256:bf34e368e8dd976423396555078def5cfc3039ebc6fc06d1ae2c5a65eebbcde4",
+
"sha256:c6938967f8528b3668622a9ed3b31d145fab161a32f5891ea7b84f6b790be05b",
+
"sha256:d1c2676e3d840852a2de7c7d5d76407c772927addff8d742b9808fe0afccebdf",
+
"sha256:d7124f52f3bd259f510651450e18e0fd081ed82f3c08541dffc7b94b883aa981",
+
"sha256:d900d949b707778696fdf01036f58c9876a0d8bfe116e8d220cfd4b15f14e741",
+
"sha256:ebfd274dcd5133e0afae738e6d9da4323c3eb021b3e13052d8cbd0e457b1256e",
+
"sha256:ed361bb83436f117f9917d282a456f9e5009ea12fd6de8742d1a4752c3017e93",
+
"sha256:f5144c75445ae3ca2057faac03fda5a902eff196702b0a24daf1d6ce0650514b"
+],
+"markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3, 3.4, 3.5'",
+"version": "==1.6.0"
+},
+"mccabe": {
+"hashes": [
+
"sha256:ab8a6258860da4b6677da4bd2fe5dc2c659cff31b3ee4f7f5d64e79735b80d42",
+
"sha256:dd8d182285a0fe56bace7f45b5e7d1a6ebcbf524e8f3bd87eb0f125271b8831f"
+],
+"version": "==0.6.1"
+},
+"pylint": {
+"hashes": [
+
"sha256:0e21d3b80b96740909d77206d741aa3ce0b06b41be375d92e1f3244a274c1f8a",
+
"sha256:d09b0b07ba06bcdff463958f53f23df25e740ecd81895f7d2699ec04bbd8dc3b"
+],
+"index": "pypi",
+"version": "==2.7.2"
+},
+"toml": {
+"hashes": [
+ 

[PATCH v6 07/25] python: add MANIFEST.in

[John Snow]

When creating a source distribution via 'python3 setup.py sdist', the
VERSION and PACKAGE.rst files aren't bundled by default. Create a
MANIFEST.in file that instructs the build tools to include these so that
installation from source dists won't fail.

(This invocation is required by 'tox', as well as by the tooling needed
to upload packages to PyPI.)

Signed-off-by: John Snow 
---
 python/README.rst  | 2 ++
 python/MANIFEST.in | 2 ++
 2 files changed, 4 insertions(+)
 create mode 100644 python/MANIFEST.in

diff --git a/python/README.rst b/python/README.rst
index 7a0dc5dff4a..86364367261 100644
--- a/python/README.rst
+++ b/python/README.rst
@@ -33,6 +33,8 @@ Files in this directory
 ---
 
 - ``qemu/`` Python package source directory.
+- ``MANIFEST.in`` is read by python setuptools, it specifies additional files
+  that should be included by a source distribution.
 - ``PACKAGE.rst`` is used as the README file that is visible on PyPI.org.
 - ``README.rst`` you are here!
 - ``VERSION`` contains the PEP-440 compliant version used to describe
diff --git a/python/MANIFEST.in b/python/MANIFEST.in
new file mode 100644
index 000..2b1ee8b4e72
--- /dev/null
+++ b/python/MANIFEST.in
@@ -0,0 +1,2 @@
+include VERSION
+include PACKAGE.rst
\ No newline at end of file
-- 
2.30.2

[PATCH v6 05/25] python: add VERSION file

[John Snow]

Python infrastructure as it exists today is not capable reliably of
single-sourcing a package version from a parent directory. The authors
of pip are working to correct this, but as of today this is not possible.

The problem is that when using pip to build and install a python
package, it copies files over to a temporary directory and performs its
build there. This loses access to any information in the parent
directory, including git itself.

Further, Python versions have a standard (PEP 440) that may or may not
follow QEMU's versioning. In general, it does; but naturally QEMU does
not follow PEP 440. To avoid any automatically-generated conflict, a
manual version file is preferred.


I am proposing:

- Python tooling follows the QEMU version, indirectly, but with a major
  version of 0 to indicate that the API is not expected to be
  stable. This would mean version 0.5.2.0, 0.5.1.1, 0.5.3.0, etc.

- In the event that a Python package needs to be updated independently
  of the QEMU version, a pre-release alpha version should be preferred,
  but *only* after inclusion to the qemu development or stable branches.

  e.g. 0.5.2.0a1, 0.5.2.0a2, and so on should be preferred prior to
  5.2.0's release.

- The Python core tooling makes absolutely no version compatibility
  checks or constraints. It *may* work with releases of QEMU from the
  past or future, but it is not required to.

  i.e., "qemu.machine" will, for now, remain in lock-step with QEMU.

- We reserve the right to split the qemu package into independently
  versioned subpackages at a later date. This might allow for us to
  begin versioning QMP independently from QEMU at a later date, if
  we so choose.


Implement this versioning scheme by adding a VERSION file and setting it
to 0.6.0.0a1.

Signed-off-by: John Snow 
Reviewed-by: Cleber Rosa 
---
 python/VERSION   | 1 +
 python/setup.cfg | 1 +
 2 files changed, 2 insertions(+)
 create mode 100644 python/VERSION

diff --git a/python/VERSION b/python/VERSION
new file mode 100644
index 000..c19f3b832b7
--- /dev/null
+++ b/python/VERSION
@@ -0,0 +1 @@
+0.6.1.0a1
diff --git a/python/setup.cfg b/python/setup.cfg
index dd71640fc2f..e7f8ab23815 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -1,5 +1,6 @@
 [metadata]
 name = qemu
+version = file:VERSION
 maintainer = QEMU Developer Team
 maintainer_email = qemu-de...@nongnu.org
 url = https://www.qemu.org/
-- 
2.30.2

[PATCH v6 01/25] iotests/297: add --namespace-packages to mypy arguments

[John Snow]

mypy is kind of weird about how it handles imports. For legacy reasons,
it won't load PEP 420 namespaces, because of logic implemented prior to
that becoming a standard.

So, if you plan on using any, you have to pass
--namespace-packages. Alright, fine.

Signed-off-by: John Snow 
Reviewed-by: Cleber Rosa 
---
 tests/qemu-iotests/297 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index a37910b42d9..433b7323368 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -95,6 +95,7 @@ def run_linters():
 '--warn-redundant-casts',
 '--warn-unused-ignores',
 '--no-implicit-reexport',
+'--namespace-packages',
 filename),
env=env,
check=False,
-- 
2.30.2

[PATCH v6 04/25] python: add qemu package installer

[John Snow]

Add setup.cfg and setup.py, necessary for installing a package via
pip. Add a ReST document (PACKAGE.rst) explaining the basics of what
this package is for and who to contact for more information. This
document will be used as the landing page for the package on PyPI.

I am not yet using a pyproject.toml style package manifest, because
"editable" installs are not defined (yet?) by PEP-517/518.

I consider editable installs crucial for development, though they have
(apparently) always been somewhat poorly defined.

Pip now (19.2 and later) now supports editable installs for projects
using pyproject.toml manifests, but might require the use of the
--no-use-pep517 flag, which somewhat defeats the point.

For now, while the dust settles, stick with the de-facto
setup.py/setup.cfg combination supported by setuptools. It will be worth
re-evaluating this point again in the future when our supported build
platforms all ship a fairly modern pip.

Additional reading on this matter:

https://github.com/pypa/packaging-problems/issues/256
https://github.com/pypa/pip/issues/6334
https://github.com/pypa/pip/issues/6375
https://github.com/pypa/pip/issues/6434
https://github.com/pypa/pip/issues/6438

Signed-off-by: John Snow 
---
 python/PACKAGE.rst | 33 +
 python/setup.cfg   | 19 +++
 python/setup.py| 23 +++
 3 files changed, 75 insertions(+)
 create mode 100644 python/PACKAGE.rst
 create mode 100644 python/setup.cfg
 create mode 100755 python/setup.py

diff --git a/python/PACKAGE.rst b/python/PACKAGE.rst
new file mode 100644
index 000..1bbfe1b58e2
--- /dev/null
+++ b/python/PACKAGE.rst
@@ -0,0 +1,33 @@
+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.
+
+Usage
+-
+
+The ``qemu.qmp`` subpackage provides a library for communicating with
+QMP servers. The ``qemu.machine`` subpackage offers rudimentary
+facilities for launching and managing QEMU processes. Refer to each
+package's documentation
+(``>>> help(qemu.qmp)``, ``>>> help(qemu.machine)``)
+for more information.
+
+Contributing
+
+
+This package is maintained by John Snow  as part of
+the QEMU source tree. Contributions are welcome and follow the `QEMU
+patch submission process
+`_, which involves
+sending patches to the QEMU development mailing list.
+
+John maintains a `GitLab staging branch
+`_, and there is an
+official `GitLab mirror `_.
+
+Please report bugs on the `QEMU issue tracker
+`_ and tag ``@jsnow`` in
+the report.
diff --git a/python/setup.cfg b/python/setup.cfg
new file mode 100644
index 000..dd71640fc2f
--- /dev/null
+++ b/python/setup.cfg
@@ -0,0 +1,19 @@
+[metadata]
+name = qemu
+maintainer = QEMU Developer Team
+maintainer_email = qemu-de...@nongnu.org
+url = https://www.qemu.org/
+download_url = https://www.qemu.org/download/
+description = QEMU Python Build, Debug and SDK tooling.
+long_description = file:PACKAGE.rst
+long_description_content_type = text/x-rst
+classifiers =
+Development Status :: 3 - Alpha
+License :: OSI Approved :: GNU General Public License v2 (GPLv2)
+Natural Language :: English
+Operating System :: OS Independent
+Programming Language :: Python :: 3 :: Only
+
+[options]
+python_requires = >= 3.6
+packages = find_namespace:
diff --git a/python/setup.py b/python/setup.py
new file mode 100755
index 000..2014f81b757
--- /dev/null
+++ b/python/setup.py
@@ -0,0 +1,23 @@
+#!/usr/bin/env python3
+"""
+QEMU tooling installer script
+Copyright (c) 2020-2021 John Snow for Red Hat, Inc.
+"""
+
+import setuptools
+import pkg_resources
+
+
+def main():
+"""
+QEMU tooling installer
+"""
+
+# 
https://medium.com/@daveshawley/safely-using-setup-cfg-for-metadata-1babbe54c108
+pkg_resources.require('setuptools>=39.2')
+
+setuptools.setup()
+
+
+if __name__ == '__main__':
+main()
-- 
2.30.2

[PATCH v6 02/25] python: create qemu packages

[John Snow]

move python/qemu/*.py to python/qemu/[machine, qmp]/*.py and update import
directives across the tree.

This is done to create a PEP420 namespace package, in which we may
create subpackages. To do this, the namespace directory ("qemu") should
not have any modules in it. Those files will go into new 'machine' and 'qmp'
subpackages instead.

Implement machine/__init__.py making the top-level classes and functions
from its various modules available directly inside the package. Change
qmp.py to qmp/__init__.py similarly, such that all of the useful QMP
library classes are available directly from "qemu.qmp" instead of
"qemu.qmp.qmp".

Signed-off-by: John Snow 


---

Note for reviewers: in the next patch, I add a utils sub-package and
move qemu/machine/accel.py to qemu/utils/accel.py. If we like it that
way, we can squash it in here if we want, or just leave it as its own
follow-up patch, I am just leaving it as something that will be easy to
un-do or change for now.

Signed-off-by: John Snow 
---
 python/{qemu => }/.isort.cfg|  0
 python/qemu/__init__.py | 11 --
 python/qemu/{ => machine}/.flake8   |  0
 python/qemu/machine/__init__.py | 41 +
 python/qemu/{ => machine}/accel.py  |  0
 python/qemu/{ => machine}/console_socket.py |  0
 python/qemu/{ => machine}/machine.py| 16 +---
 python/qemu/{ => machine}/pylintrc  |  0
 python/qemu/{ => machine}/qtest.py  |  3 +-
 python/qemu/{qmp.py => qmp/__init__.py} | 12 +-
 tests/acceptance/avocado_qemu/__init__.py   |  4 +-
 tests/acceptance/virtio-gpu.py  |  2 +-
 tests/qemu-iotests/300  |  4 +-
 tests/qemu-iotests/iotests.py   |  2 +-
 tests/vm/aarch64vm.py   |  2 +-
 tests/vm/basevm.py  |  3 +-
 16 files changed, 73 insertions(+), 27 deletions(-)
 rename python/{qemu => }/.isort.cfg (100%)
 delete mode 100644 python/qemu/__init__.py
 rename python/qemu/{ => machine}/.flake8 (100%)
 create mode 100644 python/qemu/machine/__init__.py
 rename python/qemu/{ => machine}/accel.py (100%)
 rename python/qemu/{ => machine}/console_socket.py (100%)
 rename python/qemu/{ => machine}/machine.py (98%)
 rename python/qemu/{ => machine}/pylintrc (100%)
 rename python/qemu/{ => machine}/qtest.py (99%)
 rename python/qemu/{qmp.py => qmp/__init__.py} (96%)

diff --git a/python/qemu/.isort.cfg b/python/.isort.cfg
similarity index 100%
rename from python/qemu/.isort.cfg
rename to python/.isort.cfg
diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
deleted file mode 100644
index 4ca06c34a41..000
--- 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 
-#
-# 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/.flake8 b/python/qemu/machine/.flake8
similarity index 100%
rename from python/qemu/.flake8
rename to python/qemu/machine/.flake8
diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
new file mode 100644
index 000..0ac6c1e36e3
--- /dev/null
+++ b/python/qemu/machine/__init__.py
@@ -0,0 +1,41 @@
+"""
+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.
+
+- QEMUQtestProtocol: Connect to, send/receive qtest messages.
+
+- list_accel: List available accelerators
+- kvm_available: Probe for KVM support
+- tcg_available: Probe for TCG support
+"""
+
+# Copyright (C) 2020-2021 John Snow for Red Hat Inc.
+# Copyright (C) 2015-2016 Red Hat Inc.
+# Copyright (C) 2012 IBM Corp.
+#
+# Authors:
+#  John Snow 
+#  Fam Zheng 
+#
+# 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 kvm_available, list_accel, tcg_available
+from .machine import QEMUMachine
+from .qtest import QEMUQtestMachine, QEMUQtestProtocol
+
+
+__all__ = (
+'list_accel',
+'kvm_available',
+'tcg_available',
+'QEMUMachine',
+'QEMUQtestProtocol',
+'QEMUQtestMachine',
+)
diff --git a/python/qemu/accel.py b/python/qemu/machine/accel.py
similarity index 100%
rename from python/qemu/accel.py
rename to python/qemu/machine/accel.py
diff --git a/python/qemu/console_socket.py 
b/python/qemu/machine/console_socket.py
similarity index 100%
rename from python/qemu/console_socket.py
rename to python/qemu/machine/console_socket.py
diff --git a/python/qemu/machine.py b/python/qemu/machine/machine.py
similarity index 98%
rename from python/qemu/machine.py
rename to python/qemu/machine/machine.py
index 0df5b2f386f..dea343afeba 100644
--- 

[PATCH v6 03/25] python: create utils sub-package

[John Snow]

Create a space for miscellaneous things that don't belong strictly in
"qemu.machine" nor "qemu.qmp" packages.

Signed-off-by: John Snow 
---
 python/qemu/machine/__init__.py   |  8 
 python/qemu/utils/__init__.py | 23 +++
 python/qemu/{machine => utils}/accel.py   |  0
 tests/acceptance/avocado_qemu/__init__.py |  4 ++--
 tests/acceptance/virtio-gpu.py|  2 +-
 tests/vm/aarch64vm.py |  2 +-
 tests/vm/basevm.py|  3 ++-
 7 files changed, 29 insertions(+), 13 deletions(-)
 create mode 100644 python/qemu/utils/__init__.py
 rename python/qemu/{machine => utils}/accel.py (100%)

diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
index 0ac6c1e36e3..98302ea31e7 100644
--- a/python/qemu/machine/__init__.py
+++ b/python/qemu/machine/__init__.py
@@ -8,10 +8,6 @@
  - QEMUQtestMachine: VM class, with a qtest socket.
 
 - QEMUQtestProtocol: Connect to, send/receive qtest messages.
-
-- list_accel: List available accelerators
-- kvm_available: Probe for KVM support
-- tcg_available: Probe for TCG support
 """
 
 # Copyright (C) 2020-2021 John Snow for Red Hat Inc.
@@ -26,15 +22,11 @@
 # the COPYING file in the top-level directory.
 #
 
-from .accel import kvm_available, list_accel, tcg_available
 from .machine import QEMUMachine
 from .qtest import QEMUQtestMachine, QEMUQtestProtocol
 
 
 __all__ = (
-'list_accel',
-'kvm_available',
-'tcg_available',
 'QEMUMachine',
 'QEMUQtestProtocol',
 'QEMUQtestMachine',
diff --git a/python/qemu/utils/__init__.py b/python/qemu/utils/__init__.py
new file mode 100644
index 000..edf807a93e5
--- /dev/null
+++ b/python/qemu/utils/__init__.py
@@ -0,0 +1,23 @@
+"""
+QEMU development and testing utilities
+
+This library provides a small handful of utilities for performing various tasks
+not directly related to the launching of a VM.
+
+The only module included at present is accel; its public functions are
+repeated here for your convenience:
+
+- list_accel: List available accelerators
+- kvm_available: Probe for KVM support
+- tcg_available: Prove for TCG support
+"""
+
+# pylint: disable=import-error
+from .accel import kvm_available, list_accel, tcg_available
+
+
+__all__ = (
+'list_accel',
+'kvm_available',
+'tcg_available',
+)
diff --git a/python/qemu/machine/accel.py b/python/qemu/utils/accel.py
similarity index 100%
rename from python/qemu/machine/accel.py
rename to python/qemu/utils/accel.py
diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index ff7bf81f1a9..5f60892c2c4 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -41,8 +41,8 @@
 sys.path.append(os.path.join(SOURCE_DIR, 'python'))
 
 from qemu.machine import QEMUMachine
-from qemu.machine import kvm_available
-from qemu.machine import tcg_available
+from qemu.utils import kvm_available
+from qemu.utils import tcg_available
 
 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/virtio-gpu.py b/tests/acceptance/virtio-gpu.py
index 0685e30bcae..e7979343e93 100644
--- a/tests/acceptance/virtio-gpu.py
+++ b/tests/acceptance/virtio-gpu.py
@@ -10,7 +10,7 @@
 from avocado_qemu import exec_command_and_wait_for_pattern
 from avocado_qemu import is_readable_executable_file
 
-from qemu.machine import kvm_available
+from qemu.utils import kvm_available
 
 import os
 import socket
diff --git a/tests/vm/aarch64vm.py b/tests/vm/aarch64vm.py
index 39ff99b0859..b00cce07eb8 100644
--- a/tests/vm/aarch64vm.py
+++ b/tests/vm/aarch64vm.py
@@ -14,7 +14,7 @@
 import sys
 import subprocess
 import basevm
-from qemu.machine import kvm_available
+from qemu.utils import kvm_available
 
 # This is the config needed for current version of QEMU.
 # This works for both kvm and tcg.
diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 12d08cf2b1b..a3867fdf88e 100644
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -19,7 +19,8 @@
 import time
 import datetime
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.machine import kvm_available, QEMUMachine
+from qemu.machine import QEMUMachine
+from qemu.utils import kvm_available
 import subprocess
 import hashlib
 import argparse
-- 
2.30.2

[PATCH v6 00/25] python: create installable package

[John Snow]

Based-on: <20210512214642.2803189-1-js...@redhat.com>
CI: https://gitlab.com/jsnow/qemu/-/pipelines/302010131
GitLab: https://gitlab.com/jsnow/qemu/-/tree/python-package-mk3
MR: https://gitlab.com/jsnow/qemu/-/merge_requests/4

I invite you to leave review comments on my mock merge request on
gitlab, submitted against my own mirror. I will, as always, also respond
to feedback on-list.

ABOUT
=

This series factors the python/qemu directory as an installable
package. It does not yet actually change the mechanics of how any other
python source in the tree actually consumes it (yet), beyond the import
path -- some import statements change in a few places.

RATIONALE
=

The primary motivation of this series is primarily to formalize our
dependencies on mypy, flake8, isort, and pylint alongside versions that
are known to work. It does this using the setup.cfg and setup.py
files. It also adds explicitly pinned versions (using Pipfile.lock) of
these dependencies that should behave in a repeatable and known way for
developers and CI environments both. Lastly, it enables those CI checks
such that we can enforce Python coding quality checks via the CI tests.

An auxiliary motivation is that this package is formatted in such a way
that it COULD be uploaded to https://pypi.org/project/qemu and installed
independently of qemu.git with `pip install qemu`, but that button
remains *unpushed* and this series *will not* cause any such
releases. We have time to debate finer points like API guarantees and
versioning even after this series is merged.

Other bits of interest
--

With the python tooling as a proper package, you can install this
package in editable or production mode to a virtual environment, your
local user environment, or your system packages. The primary benefit of
this is to gain access to QMP tooling regardless of CWD, without needing
to battle sys.path (and confounding other python analysis tools).

For example: when developing, you may go to qemu/python/ and run `make
venv` followed by `pipenv shell` to activate a virtual environment that
contains the qemu python packages. These packages will always reflect
the current version of the source files in the tree. When you are
finished, you can simply exit the shell (^d) to remove these packages
from your python environment.

When not developing, you could install a version of this package to your
environment outright to gain access to the QMP and QEMUMachine classes
for lightweight scripting and testing by using pip: "pip install
[--user] ."

TESTING THIS SERIES
===

First of all, nothing should change. Without any intervention,
everything should behave exactly as it did before. The only new
information here comes from how to interact with and run the linters
that will be enforcing code quality standards in this subdirectory.

There are various invocations available that will test subtly different
combinations using subtly different environments. I am assuming some
light knowledge of Python environments and installing Python packages
here. If you have questions, I would be delighted to answer them.

To test the new tests, CD to ./python/ first, and then:

0. Try "make" or "make help" to get a sense of this series.

1. Try "make venv && pipenv shell" to get a venv with the package
   installed to it in editable mode. Ctrl+d exits this venv shell. While
   in this shell, any python script that uses "from qemu.[qmp|machine]
   import ..." should work correctly regardless of where the script is,
   or what your CWD is.

   This will pull some packages from PyPI and install them into the
   virtual environment, leaving your normal environment untouched.

   You will need Python 3.6 and pipenv installed on your system to do
   this step. For Fedora: "dnf install python36 pipenv" will do the
   trick. If you don't have this, skip down to #4 and onwards.

2. Try "make check" while still in the shell to run the Python linters
using the venv built in the previous step. This will run avocado, which
will in turn execute mypy, flake8, isort and pylint with the correct
arguments.

3. Having exited the shell from above, try "make venv-check". This will
create and update the venv if needed, then run 'make check' within the
context of that shell. It should pass as long as the above did. You
should be able to run "make distclean" prior to running "make
venv-check" and have the entire process work start to finish.

4. Still outside of the venv, you may try running "make check". This
will not install anything, but unless you have the right Python
dependencies installed, these tests may fail for you. You might try
using "pip install --user .[devel]" to install the development packages
needed to run the tests successfully to your local user's python
environment. Once done, you will probably want to "pip uninstall qemu"
to remove the qemu packages to avoid them interfering with other things.

5. "make distclean" will delete the venv 

[PATCH 02/10] python/machine: use subprocess.DEVNULL instead of open(os.path.devnull)

[John Snow]

One less file resource to manage, and it helps quiet some pylint >=
2.8.0 warnings about not using a with-context manager for the open call.

Signed-off-by: John Snow 
---
 python/qemu/machine.py | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 6e44bda337e..41f51bd27d0 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -223,9 +223,8 @@ def send_fd_scm(self, fd: Optional[int] = None,
 assert fd is not None
 fd_param.append(str(fd))
 
-devnull = open(os.path.devnull, 'rb')
 proc = subprocess.Popen(
-fd_param, stdin=devnull, stdout=subprocess.PIPE,
+fd_param, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE,
 stderr=subprocess.STDOUT, close_fds=False
 )
 output = proc.communicate()[0]
@@ -393,7 +392,6 @@ def _launch(self) -> None:
 """
 Launch the VM and establish a QMP connection
 """
-devnull = open(os.path.devnull, 'rb')
 self._pre_launch()
 self._qemu_full_args = tuple(
 chain(self._wrapper,
@@ -403,7 +401,7 @@ def _launch(self) -> None:
 )
 LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
 self._popen = subprocess.Popen(self._qemu_full_args,
-   stdin=devnull,
+   stdin=subprocess.DEVNULL,
stdout=self._qemu_log_file,
stderr=subprocess.STDOUT,
shell=False,
-- 
2.30.2

[PATCH 09/10] iotests: silence spurious consider-using-with warnings

[John Snow]

In a few cases, we can't use 'with ...' because they belong to
long-running classes that need those items to stay open at the end of
the block. We're handling it, so tell pylint to shush.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py| 2 +-
 tests/qemu-iotests/testrunner.py | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5d5ec40429b..e09c991b84e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -311,7 +311,7 @@ def qemu_nbd_popen(*args):
 cmd.extend(args)
 
 log('Start NBD server')
-p = subprocess.Popen(cmd)
+p = subprocess.Popen(cmd)  # pylint: disable=consider-using-with
 try:
 while not os.path.exists(pid_file):
 if p.poll() is not None:
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 1fc61fcaa34..34fb551c01b 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -258,6 +258,7 @@ def do_run_test(self, test: str) -> TestResult:
 
 t0 = time.time()
 with f_bad.open('w', encoding="utf-8") as f:
+# pylint: disable=consider-using-with
 proc = subprocess.Popen(args, cwd=str(f_test.parent), env=env,
 stdout=f, stderr=subprocess.STDOUT)
 try:
-- 
2.30.2

[PATCH 07/10] iotests: use subprocess.run where possible

[John Snow]

pylint 2.8.x adds warnings whenever we use Popen calls without using
'with', so it's desirable to convert synchronous calls to run()
invocations where applicable.

(Though, this trades one pylint warning for another due to a pylint bug,
which I've silenced with a pragma and a link to the bug.)

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5af01828951..46deb7f4dd4 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -113,15 +113,16 @@ def qemu_tool_pipe_and_status(tool: str, args: 
Sequence[str],
 Run a tool and return both its output and its exit code
 """
 stderr = subprocess.STDOUT if connect_stderr else None
-subp = subprocess.Popen(args,
-stdout=subprocess.PIPE,
-stderr=stderr,
-universal_newlines=True)
-output = subp.communicate()[0]
-if subp.returncode < 0:
+res = subprocess.run(args,
+ stdout=subprocess.PIPE,
+ stderr=stderr,
+ universal_newlines=True,
+ check=False)
+output = res.stdout
+if res.returncode < 0:
 cmd = ' '.join(args)
-sys.stderr.write(f'{tool} received signal {-subp.returncode}: {cmd}\n')
-return (output, subp.returncode)
+sys.stderr.write(f'{tool} received signal {-res.returncode}: {cmd}\n')
+return (output, res.returncode)
 
 def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
 """
@@ -1153,6 +1154,8 @@ def _verify_virtio_scsi_pci_or_ccw() -> None:
 
 
 def supports_quorum():
+# https://github.com/PyCQA/astroid/issues/689
+# pylint: disable=unsupported-membership-test
 return 'quorum' in qemu_img_pipe('--help')
 
 def verify_quorum():
-- 
2.30.2

[PATCH 10/10] iotests: ensure that QemuIoInteractive definitely closes

[John Snow]

More on the lines of quieting pylint 2.8.x, though to make it obvious
that we definitely handle the cleanup here, I elected to bolster the
close() method here.

1. Check for the process having terminated early more rigorously by
checking poll() directly.

2. Change the prompt read into an assertion.

3. Ensure that the final communicate() call *definitely* closes the
socket, adding a timeout and a final kill just in case.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e09c991b84e..12e876fa67d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -238,20 +238,27 @@ def qemu_io_silent_check(*args):
 class QemuIoInteractive:
 def __init__(self, *args):
 self.args = qemu_io_args_no_fmt + list(args)
+
+# Resource cleaned up via close()
+# pylint: disable=consider-using-with
 self._p = subprocess.Popen(self.args, stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
universal_newlines=True)
+if self._p.poll():
+# Failed to start.
+out = self._p.stdout.read()
+raise subprocess.SubprocessError(out, self._p.poll())
+
+# Eat the first prompt
 out = self._p.stdout.read(9)
-if out != 'qemu-io> ':
-# Most probably qemu-io just failed to start.
-# Let's collect the whole output and exit.
-out += self._p.stdout.read()
-self._p.wait(timeout=1)
-raise ValueError(out)
+assert out == 'qemu-io> ', "Did not understand qemu-io prompt"
 
 def close(self):
-self._p.communicate('q\n')
+try:
+self._p.communicate('q\n', timeout=5)
+except subprocess.TimeoutExpired:
+self._p.kill()
 
 def _read_output(self):
 pattern = 'qemu-io> '
-- 
2.30.2

[PATCH 05/10] python/machine: Disable pylint warning for open() in _pre_launch

[John Snow]

Shift the open() call later so that the pylint pragma applies *only* to
that one open() call. Add a note that suggests why this is safe: the
resource is unconditionally cleaned up in _post_shutdown().

_post_shutdown is called after failed launches (see launch()), and
unconditionally after every call to shutdown(), and therefore also on
__exit__.

Signed-off-by: John Snow 
---
 python/qemu/machine.py | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index c13ff9b32bf..8f86303b48f 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -308,7 +308,6 @@ def _pre_launch(self) -> None:
 self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-",
   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')
 
 if self._console_set:
 self._remove_files.append(self._console_address)
@@ -323,6 +322,11 @@ def _pre_launch(self) -> None:
 nickname=self._name
 )
 
+# NOTE: Make sure any opened resources are *definitely* freed in
+# _post_shutdown()!
+# pylint: disable=consider-using-with
+self._qemu_log_file = open(self._qemu_log_path, 'wb')
+
 def _post_launch(self) -> None:
 if self._qmp_connection:
 self._qmp.accept()
-- 
2.30.2

[PATCH 08/10] iotests: use 'with open()' where applicable

[John Snow]

More pylint 2.8.x warning hushing: use open's context manager where it's
applicable to do so to avoid a warning.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 46deb7f4dd4..5d5ec40429b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -335,13 +335,12 @@ def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
 
 def create_image(name, size):
 '''Create a fully-allocated raw image with sector markers'''
-file = open(name, 'wb')
-i = 0
-while i < size:
-sector = struct.pack('>l504xl', i // 512, i // 512)
-file.write(sector)
-i = i + 512
-file.close()
+with open(name, 'wb') as outfile:
+i = 0
+while i < size:
+sector = struct.pack('>l504xl', i // 512, i // 512)
+outfile.write(sector)
+i = i + 512
 
 def image_size(img):
 '''Return image's virtual size'''
-- 
2.30.2

[PATCH 00/10] Python: delint iotests, machine.py and console_socket.py

[John Snow]

gitlab CI: https://gitlab.com/jsnow/qemu/-/pipelines/301924893
branch: https://gitlab.com/jsnow/qemu/-/commits/python-package-pre-cleanup

This series serves as a pre-requisite for packaging the python series
and getting the linters running via CI. The first patch fixes a linter
error we've had for a while now; the subsequent 9 fix a new warning that
was recently added to pylint 2.8.x.

If there's nobody opposed, I'll take it through my Python queue,
including the iotests bits.

John Snow (10):
  python/console_socket: avoid one-letter variable
  python/machine: use subprocess.DEVNULL instead of
open(os.path.devnull)
  python/machine: use subprocess.run instead of subprocess.Popen
  python/console_socket: Add a pylint ignore
  python/machine: Disable pylint warning for open() in _pre_launch
  python/machine: disable warning for Popen in _launch()
  iotests: use subprocess.run where possible
  iotests: use 'with open()' where applicable
  iotests: silence spurious consider-using-with warnings
  iotests: ensure that QemuIoInteractive definitely closes

 python/qemu/console_socket.py| 11 ---
 python/qemu/machine.py   | 28 ++--
 tests/qemu-iotests/iotests.py| 55 +++-
 tests/qemu-iotests/testrunner.py |  1 +
 4 files changed, 57 insertions(+), 38 deletions(-)

-- 
2.30.2

[PATCH 01/10] python/console_socket: avoid one-letter variable

[John Snow]

Fixes pylint warnings.

Signed-off-by: John Snow 
Reviewed-by: Cleber Rosa 
---
 python/qemu/console_socket.py | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index ac21130e446..87237bebef7 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -46,11 +46,11 @@ def __init__(self, address: str, file: Optional[str] = None,
 self._drain_thread = self._thread_start()
 
 def __repr__(self) -> str:
-s = super().__repr__()
-s = s.rstrip(">")
-s = "%s,  logfile=%s, drain_thread=%s>" % (s, self._logfile,
-   self._drain_thread)
-return s
+tmp = super().__repr__()
+tmp = tmp.rstrip(">")
+tmp = "%s,  logfile=%s, drain_thread=%s>" % (tmp, self._logfile,
+ self._drain_thread)
+return tmp
 
 def _drain_fn(self) -> None:
 """Drains the socket and runs while the socket is open."""
-- 
2.30.2

[PATCH 03/10] python/machine: use subprocess.run instead of subprocess.Popen

[John Snow]

use run() instead of Popen() -- to assert to pylint that we are not
forgetting to close a long-running program.

Signed-off-by: John Snow 
---
 python/qemu/machine.py | 15 +--
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 41f51bd27d0..c13ff9b32bf 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -223,13 +223,16 @@ def send_fd_scm(self, fd: Optional[int] = None,
 assert fd is not None
 fd_param.append(str(fd))
 
-proc = subprocess.Popen(
-fd_param, stdin=subprocess.DEVNULL, stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT, close_fds=False
+proc = subprocess.run(
+fd_param,
+stdin=subprocess.DEVNULL,
+stdout=subprocess.PIPE,
+stderr=subprocess.STDOUT,
+check=True,
+close_fds=False,
 )
-output = proc.communicate()[0]
-if output:
-LOG.debug(output)
+if proc.stdout:
+LOG.debug(proc.stdout)
 
 return proc.returncode
 
-- 
2.30.2

[PATCH 04/10] python/console_socket: Add a pylint ignore

[John Snow]

We manage cleaning up this resource ourselves. Pylint should shush.

Signed-off-by: John Snow 
---
 python/qemu/console_socket.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/python/qemu/console_socket.py b/python/qemu/console_socket.py
index 87237bebef7..8c4ff598ad7 100644
--- a/python/qemu/console_socket.py
+++ b/python/qemu/console_socket.py
@@ -39,6 +39,7 @@ def __init__(self, address: str, file: Optional[str] = None,
 self.connect(address)
 self._logfile = None
 if file:
+# pylint: disable=consider-using-with
 self._logfile = open(file, "bw")
 self._open = True
 self._drain_thread = None
-- 
2.30.2

[PATCH 06/10] python/machine: disable warning for Popen in _launch()

[John Snow]

We handle this resource rather meticulously in
shutdown/kill/wait/__exit__ et al, through the laborious mechanisms in
_do_shutdown().

Quiet this pylint warning here.

Signed-off-by: John Snow 
---
 python/qemu/machine.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 8f86303b48f..0df5b2f386f 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -407,6 +407,9 @@ def _launch(self) -> None:
   self._args)
 )
 LOG.debug('VM launch command: %r', ' '.join(self._qemu_full_args))
+
+# Cleaning up of this subprocess is guaranteed by _do_shutdown.
+# pylint: disable=consider-using-with
 self._popen = subprocess.Popen(self._qemu_full_args,
stdin=subprocess.DEVNULL,
stdout=self._qemu_log_file,
-- 
2.30.2

Re: [PATCH] hmp: Fix loadvm to resume the VM on success instead of failure

[Dr. David Alan Gilbert]

* Kevin Wolf (kw...@redhat.com) wrote:
> Commit f61fe11aa6f broke hmp_loadvm() by adding an incorrect negation
> when converting from 0/-errno return values to a bool value. The result
> is that loadvm resumes the VM now if it failed and keeps it stopped if
> it failed. Fix it to restore the old behaviour and do it the other way
> around.

I think your wording is wrong there - you have two 'if it failed'

> Fixes: f61fe11aa6f7f8f0ffe4ddaa56a8108f3ab57854
> Cc: qemu-sta...@nongnu.org
> Reported-by: Yanhui Ma 
> Signed-off-by: Kevin Wolf 
> ---
>  monitor/hmp-cmds.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 0ad5b77477..cc15d9b6ee 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1133,7 +1133,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
>  
>  vm_stop(RUN_STATE_RESTORE_VM);
>  
> -if (!load_snapshot(name, NULL, false, NULL, ) && saved_vm_running) {
> +if (load_snapshot(name, NULL, false, NULL, ) && saved_vm_running) {
>  vm_start();

So if I'm reading htat right, that gets you it restarting it if
load_snapshot returns true, and it returns true if the load_snapshot
worked; i.e. if we were running and we succesfully load a snasphot
then we carry on running.

Other than the commit message, it makes sense tome:


Reviewed-by: Dr. David Alan Gilbert 

>  }

>  hmp_handle_error(mon, err);
> -- 
> 2.30.2
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 5/6] co-shared-resource: protect with a mutex

[Paolo Bonzini]

On 12/05/21 17:44, Stefan Hajnoczi wrote:

If we follow this strategy basically any data structure used
by coroutines needs its own fine-grained lock (like Java's Object base
class which has its own lock).


Maybe not all, but only those that use CoQueue?  Interestingly, I was a 
bit less okay with ProgressMeter and didn't think twice about the other two.


Paolo

[PATCH v2 4/4] iotests/297: Cover tests/

[Max Reitz]

297 so far does not check the named tests, which reside in the tests/
directory (i.e. full path tests/qemu-iotests/tests).  Fix it.

Thanks to the previous two commits, all named tests pass its scrutiny,
so we do not have to add anything to SKIP_FILES.

Signed-off-by: Max Reitz 
Reviewed-by: Willian Rampazzo 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Kevin Wolf 
---
 tests/qemu-iotests/297 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index e3244d40a0..ce0e82e279 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -55,8 +55,9 @@ def is_python_file(filename):
 
 
 def run_linters():
-files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
- if is_python_file(filename)]
+named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
+check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
+files = [filename for filename in check_tests if is_python_file(filename)]
 
 iotests.logger.debug('Files to be checked:')
 iotests.logger.debug(', '.join(sorted(files)))
-- 
2.31.1

[PATCH v2 2/4] migrate-bitmaps-postcopy-test: Fix pylint warnings

[Max Reitz]

pylint complains that discards1_sha256 and all_discards_sha256 are first
set in non-__init__ methods.

These variables are not really class-variables anyway, so let them
instead be returned by start_postcopy(), thus silencing pylint.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Max Reitz 
---
 .../tests/migrate-bitmaps-postcopy-test | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test 
b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
index 584062b412..00ebb5c251 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
@@ -132,10 +132,10 @@ class 
TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
 
 result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',
node='drive0', name='bitmap0')
-self.discards1_sha256 = result['return']['sha256']
+discards1_sha256 = result['return']['sha256']
 
 # Check, that updating the bitmap by discards works
-assert self.discards1_sha256 != empty_sha256
+assert discards1_sha256 != empty_sha256
 
 # We want to calculate resulting sha256. Do it in bitmap0, so, disable
 # other bitmaps
@@ -148,7 +148,7 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
 
 result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',
node='drive0', name='bitmap0')
-self.all_discards_sha256 = result['return']['sha256']
+all_discards_sha256 = result['return']['sha256']
 
 # Now, enable some bitmaps, to be updated during migration
 for i in range(2, nb_bitmaps, 2):
@@ -173,10 +173,11 @@ class 
TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
 
 event_resume = self.vm_b.event_wait('RESUME')
 self.vm_b_events.append(event_resume)
-return event_resume
+return (event_resume, discards1_sha256, all_discards_sha256)
 
 def test_postcopy_success(self):
-event_resume = self.start_postcopy()
+event_resume, discards1_sha256, all_discards_sha256 = \
+self.start_postcopy()
 
 # enabled bitmaps should be updated
 apply_discards(self.vm_b, discards2)
@@ -217,7 +218,7 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
 for i in range(0, nb_bitmaps, 5):
 result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256',
node='drive0', name='bitmap{}'.format(i))
-sha = self.discards1_sha256 if i % 2 else self.all_discards_sha256
+sha = discards1_sha256 if i % 2 else all_discards_sha256
 self.assert_qmp(result, 'return/sha256', sha)
 
 def test_early_shutdown_destination(self):
-- 
2.31.1

[PATCH v2 1/4] iotests/297: Drop 169 and 199 from the skip list

[Max Reitz]

169 and 199 have been renamed and moved to tests/ (commit a44be0334be:
"iotests: rename and move 169 and 199 tests"), so we can drop them from
the skip list.

Signed-off-by: Max Reitz 
Reviewed-by: Willian Rampazzo 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Kevin Wolf 
---
 tests/qemu-iotests/297 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index a37910b42d..e3244d40a0 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -29,7 +29,7 @@ import iotests
 SKIP_FILES = (
 '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
 '096', '118', '124', '132', '136', '139', '147', '148', '149',
-'151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
+'151', '152', '155', '163', '165', '194', '196', '202',
 '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
 '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
 '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
-- 
2.31.1

[PATCH v2 3/4] migrate-bitmaps-test: Fix pylint warnings

[Max Reitz]

There are a couple of things pylint takes issue with:
- The "time" import is unused
- The import order (iotests should come last)
- get_bitmap_hash() doesn't use @self and so should be a function
- Semicolons at the end of some lines
- Parentheses after "if"
- Some lines are too long (80 characters instead of 79)
- inject_test_case()'s @name parameter shadows a top-level @name
  variable
- "lambda self: mc(self)" is equivalent to just "mc"
- Always put two empty lines after a function
- f'exec: cat > /dev/null' does not need to be an f-string

Fix them.

Signed-off-by: Max Reitz 
Reviewed-by: Willian Rampazzo 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Kevin Wolf 
---
 tests/qemu-iotests/tests/migrate-bitmaps-test | 38 ++-
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-test 
b/tests/qemu-iotests/tests/migrate-bitmaps-test
index a5c7bc83e0..fb5ffbb8c4 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-test
@@ -20,11 +20,10 @@
 #
 
 import os
-import iotests
-import time
 import itertools
 import operator
 import re
+import iotests
 from iotests import qemu_img, qemu_img_create, Timeout
 
 
@@ -37,6 +36,12 @@ mig_cmd = 'exec: cat > ' + mig_file
 incoming_cmd = 'exec: cat ' + mig_file
 
 
+def get_bitmap_hash(vm):
+result = vm.qmp('x-debug-block-dirty-bitmap-sha256',
+node='drive0', name='bitmap0')
+return result['return']['sha256']
+
+
 class TestDirtyBitmapMigration(iotests.QMPTestCase):
 def tearDown(self):
 self.vm_a.shutdown()
@@ -62,21 +67,16 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 params['persistent'] = True
 
 result = vm.qmp('block-dirty-bitmap-add', **params)
-self.assert_qmp(result, 'return', {});
-
-def get_bitmap_hash(self, vm):
-result = vm.qmp('x-debug-block-dirty-bitmap-sha256',
-node='drive0', name='bitmap0')
-return result['return']['sha256']
+self.assert_qmp(result, 'return', {})
 
 def check_bitmap(self, vm, sha256):
 result = vm.qmp('x-debug-block-dirty-bitmap-sha256',
 node='drive0', name='bitmap0')
 if sha256:
-self.assert_qmp(result, 'return/sha256', sha256);
+self.assert_qmp(result, 'return/sha256', sha256)
 else:
 self.assert_qmp(result, 'error/desc',
-"Dirty bitmap 'bitmap0' not found");
+"Dirty bitmap 'bitmap0' not found")
 
 def do_test_migration_resume_source(self, persistent, migrate_bitmaps):
 granularity = 512
@@ -97,7 +97,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 self.add_bitmap(self.vm_a, granularity, persistent)
 for r in regions:
 self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % r)
-sha256 = self.get_bitmap_hash(self.vm_a)
+sha256 = get_bitmap_hash(self.vm_a)
 
 result = self.vm_a.qmp('migrate', uri=mig_cmd)
 while True:
@@ -106,7 +106,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 break
 while True:
 result = self.vm_a.qmp('query-status')
-if (result['return']['status'] == 'postmigrate'):
+if result['return']['status'] == 'postmigrate':
 break
 
 # test that bitmap is still here
@@ -164,7 +164,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 self.add_bitmap(self.vm_a, granularity, persistent)
 for r in regions:
 self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % r)
-sha256 = self.get_bitmap_hash(self.vm_a)
+sha256 = get_bitmap_hash(self.vm_a)
 
 if pre_shutdown:
 self.vm_a.shutdown()
@@ -214,16 +214,17 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 self.check_bitmap(self.vm_b, sha256 if persistent else False)
 
 
-def inject_test_case(klass, name, method, *args, **kwargs):
+def inject_test_case(klass, suffix, method, *args, **kwargs):
 mc = operator.methodcaller(method, *args, **kwargs)
-setattr(klass, 'test_' + method + name, lambda self: mc(self))
+setattr(klass, 'test_' + method + suffix, mc)
+
 
 for cmb in list(itertools.product((True, False), repeat=5)):
 name = ('_' if cmb[0] else '_not_') + 'persistent_'
 name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
 name += '_online' if cmb[2] else '_offline'
 name += '_shared' if cmb[3] else '_nonshared'
-if (cmb[4]):
+if cmb[4]:
 name += '__pre_shutdown'
 
 inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
@@ -270,7 +271,8 @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
 self.assert_qmp(result, 'return', {})
 
 # Check that the bitmaps are there
-for node in self.vm.qmp('query-named-block-nodes', 
flat=True)['return']:
+

[PATCH v2 0/4] iotests/297: Cover tests/

[Max Reitz]

v1: https://lists.nongnu.org/archive/html/qemu-block/2021-03/msg01471.html


Hi,

When reviewing Vladimir’s new addition to tests/, I noticed that 297 so
far does not cover named tests.  That isn’t so good.

This series makes it cover them, and because tests/ is rather sparse at
this point, I decided to also fix up the two tests in there that don’t
pass pylint’s scrutiny yet.  I think it would be nice if we could keep
all of tests/ clean.


v2:
- Changed patch 2 as per Vladimir’s suggestion
  (i.e. don’t let discards1_sha256 and all_discards_sha256 be class
  variables at all)


git-backport-diff against v1:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/4:[] [--] 'iotests/297: Drop 169 and 199 from the skip list'
002/4:[0016] [FC] 'migrate-bitmaps-postcopy-test: Fix pylint warnings'
003/4:[] [--] 'migrate-bitmaps-test: Fix pylint warnings'
004/4:[] [--] 'iotests/297: Cover tests/'


Max Reitz (4):
  iotests/297: Drop 169 and 199 from the skip list
  migrate-bitmaps-postcopy-test: Fix pylint warnings
  migrate-bitmaps-test: Fix pylint warnings
  iotests/297: Cover tests/

 tests/qemu-iotests/297|  7 ++--
 .../tests/migrate-bitmaps-postcopy-test   | 13 ---
 tests/qemu-iotests/tests/migrate-bitmaps-test | 38 ++-
 3 files changed, 31 insertions(+), 27 deletions(-)

-- 
2.31.1

Re: [PATCH 0/4] iotests/297: Cover tests/

[Max Reitz]

On 05.05.21 10:02, Vladimir Sementsov-Ogievskiy wrote:

Hi!

Kindly remind. Didn't you forget? I'm responsible for these two tests, 
let me know if you don't have time, and I'll resend this series :)


Sorry, I forgot indeed... v2 coming right up. :)

Max

Re: [PATCH v3 0/8] block: refactor write threshold

[Max Reitz]

On 06.05.21 11:06, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

v3:
01-04,06,07: add Max's r-b.
01: improve commit msg and comments
03: improve commit msg
05: add more comments and qemu/atomic.h include
08: new, replacement for v2:08,09

Vladimir Sementsov-Ogievskiy (8):
   block/write-threshold: don't use write notifiers
   block: drop write notifiers
   test-write-threshold: rewrite test_threshold_(not_)trigger tests
   block/write-threshold: drop extra APIs
   block/write-threshold: don't use aio context lock
   test-write-threshold: drop extra tests
   test-write-threshold: drop extra TestStruct structure
   write-threshold: deal with includes

  include/block/block_int.h |  19 ++---
  include/block/write-threshold.h   |  31 +++--
  block.c   |   1 -
  block/io.c|  11 +--
  block/write-threshold.c   | 111 +++---
  tests/unit/test-write-threshold.c |  90 ++--
  6 files changed, 52 insertions(+), 211 deletions(-)


Thanks, I’ve applied all patches but patch 5 to my block branch, with 
the changes Eric has suggested:


https://github.com/XanClic/qemu/commits/block

Max

Re: [PATCH v2] qemu-iotests: fix pylint 2.8 consider-using-with error

[Max Reitz]

On 10.05.21 21:04, Emanuele Giuseppe Esposito wrote:

pylint 2.8 introduces consider-using-with error, suggesting
to use the 'with' block statement when possible.

Modify all subprocess.Popen call to use the 'with' statement,
except the one in __init__ of QemuIoInteractive class, since
it is assigned to a class field and used in other methods.

Signed-off-by: Emanuele Giuseppe Esposito 
---
v1 -> v2:
* fix indentation [Max]
* explain why we disabled the check in QemuIoInteractive's __init__ [Max]


Thanks!

Applied to my block branch:

https://github.com/XanClic/qemu/commits/block

Max

Re: [PATCH v3 0/8] block: refactor write threshold

[Stefan Hajnoczi]

On Thu, May 06, 2021 at 12:06:13PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> v3:
> 01-04,06,07: add Max's r-b.
> 01: improve commit msg and comments
> 03: improve commit msg
> 05: add more comments and qemu/atomic.h include
> 08: new, replacement for v2:08,09
> 
> Vladimir Sementsov-Ogievskiy (8):
>   block/write-threshold: don't use write notifiers
>   block: drop write notifiers
>   test-write-threshold: rewrite test_threshold_(not_)trigger tests
>   block/write-threshold: drop extra APIs
>   block/write-threshold: don't use aio context lock
>   test-write-threshold: drop extra tests
>   test-write-threshold: drop extra TestStruct structure
>   write-threshold: deal with includes
> 
>  include/block/block_int.h |  19 ++---
>  include/block/write-threshold.h   |  31 +++--
>  block.c   |   1 -
>  block/io.c|  11 +--
>  block/write-threshold.c   | 111 +++---
>  tests/unit/test-write-threshold.c |  90 ++--
>  6 files changed, 52 insertions(+), 211 deletions(-)
> 
> -- 
> 2.29.2
> 

Aside from comments:

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [PATCH v2 0/4] virtio: Improve boot time of virtio-scsi-pci and virtio-blk-pci

[Stefan Hajnoczi]

On Fri, May 07, 2021 at 06:59:01PM +0200, Greg Kurz wrote:
> Now that virtio-scsi-pci and virtio-blk-pci map 1 virtqueue per vCPU,
> a serious slow down may be observed on setups with a big enough number
> of vCPUs.
> 
> Exemple with a pseries guest on a bi-POWER9 socket system (128 HW threads):
> 
>   virtio-scsi  virtio-blk
> 
> 1 0m20.922s   0m21.346s
> 2 0m21.230s   0m20.350s
> 4 0m21.761s   0m20.997s
> 8 0m22.770s   0m20.051s
> 160m22.038s   0m19.994s
> 320m22.928s   0m20.803s
> 640m26.583s   0m22.953s
> 128   0m41.273s   0m32.333s
> 256   2m4.727s1m16.924s
> 384   6m5.563s3m26.186s
> 
> Both perf and gprof indicate that QEMU is hogging CPUs when setting up
> the ioeventfds:
> 
>  67.88%  swapper [kernel.kallsyms]  [k] power_pmu_enable
>   9.47%  qemu-kvm[kernel.kallsyms]  [k] smp_call_function_single
>   8.64%  qemu-kvm[kernel.kallsyms]  [k] power_pmu_enable
> =>2.79%  qemu-kvmqemu-kvm   [.] memory_region_ioeventfd_before
> =>2.12%  qemu-kvmqemu-kvm   [.] 
> address_space_update_ioeventfds
>   0.56%  kworker/8:0-mm  [kernel.kallsyms]  [k] smp_call_function_single
> 
> address_space_update_ioeventfds() is called when committing an MR
> transaction, i.e. for each ioeventfd with the current code base,
> and it internally loops on all ioventfds:
> 
> static void address_space_update_ioeventfds(AddressSpace *as)
> {
> [...]
> FOR_EACH_FLAT_RANGE(fr, view) {
> for (i = 0; i < fr->mr->ioeventfd_nb; ++i) {
> 
> This means that the setup of ioeventfds for these devices has
> quadratic time complexity.
> 
> This series simply changes the device models to extend the transaction
> to all virtqueueues, like already done in the past in the generic
> code with 710fccf80d78 ("virtio: improve virtio devices initialization
> time").
> 
> Only virtio-scsi and virtio-blk are covered here, but a similar change
> might also be beneficial to other device types such as host-scsi-pci,
> vhost-user-scsi-pci and vhost-user-blk-pci.
> 
>   virtio-scsi  virtio-blk
> 
> 1 0m21.271s   0m22.076s
> 2 0m20.912s   0m19.716s
> 4 0m20.508s   0m19.310s
> 8 0m21.374s   0m20.273s
> 160m21.559s   0m21.374s
> 320m22.532s   0m21.271s
> 640m26.550s   0m22.007s
> 128   0m29.115s   0m27.446s
> 256   0m44.752s   0m41.004s
> 384   1m2.884s0m58.023s
> 
> This should fix https://bugzilla.redhat.com/show_bug.cgi?id=1927108
> which reported the issue for virtio-scsi-pci.
> 
> Changes since v1:
> - Add some comments (Stefan)
> - Drop optimization on the error path in patch 2 (Stefan)
> 
> Changes since RFC:
> 
> As suggested by Stefan, splimplify the code by directly beginning and
> committing the memory transaction from the device model, without all
> the virtio specific proxying code and no changes needed in the memory
> subsystem.
> 
> Greg Kurz (4):
>   virtio-blk: Fix rollback path in virtio_blk_data_plane_start()
>   virtio-blk: Configure all host notifiers in a single MR transaction
>   virtio-scsi: Set host notifiers and callbacks separately
>   virtio-scsi: Configure all host notifiers in a single MR transaction
> 
>  hw/block/dataplane/virtio-blk.c | 45 -
>  hw/scsi/virtio-scsi-dataplane.c | 72 -
>  2 files changed, 97 insertions(+), 20 deletions(-)
> 
> -- 
> 2.26.3
> 

Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

Re: [PATCH 0/3] hw/virtio: Constify VirtIOFeature

[Stefan Hajnoczi]

On Tue, May 11, 2021 at 12:41:54PM +0200, Philippe Mathieu-Daudé wrote:
> Trivial patches to keep VirtIOFeature arrays read-only
> (better safe than sorry).
> 
> Philippe Mathieu-Daudé (3):
>   hw/virtio: Pass virtio_feature_get_config_size() a const argument
>   virtio-blk: Constify VirtIOFeature feature_sizes[]
>   virtio-net: Constify VirtIOFeature feature_sizes[]
> 
>  include/hw/virtio/virtio.h | 2 +-
>  hw/block/virtio-blk.c  | 2 +-
>  hw/net/virtio-net.c| 2 +-
>  hw/virtio/virtio.c | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> -- 
> 2.26.3
> 
> 

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [PATCH 4/6] progressmeter: protect with a mutex

[Stefan Hajnoczi]

On Mon, May 10, 2021 at 10:59:39AM +0200, Emanuele Giuseppe Esposito wrote:
> Progressmeter is protected by the AioContext mutex, which
> is taken by the block jobs and their caller (like blockdev).
> 
> We would like to remove the dependency of block layer code on the
> AioContext mutex, since most drivers and the core I/O code are already
> not relying on it.
> 
> The simplest thing to do is to add a mutex to be able to provide
> an accurate snapshot of the progress values to the caller.
> 
> Signed-off-by: Emanuele Giuseppe Esposito 
> ---
>  blockjob.c| 33 +
>  include/qemu/progress_meter.h | 31 +++
>  job-qmp.c |  8 ++--
>  job.c |  3 +++
>  qemu-img.c|  9 ++---
>  5 files changed, 71 insertions(+), 13 deletions(-)

Unlike the next two patches where I had comments, here adding the lock
makes sense to me - the point of the progress meter is to report numbers
to the monitor thread so thread-safety is a key part of the API's value.

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [PATCH 5/6] co-shared-resource: protect with a mutex

[Stefan Hajnoczi]

On Mon, May 10, 2021 at 10:59:40AM +0200, Emanuele Giuseppe Esposito wrote:
> co-shared-resource is currently not thread-safe, as also reported
> in co-shared-resource.h. Add a QemuMutex because co_try_get_from_shres
> can also be invoked from non-coroutine context.
> 
> Signed-off-by: Emanuele Giuseppe Esposito 
> ---
>  util/qemu-co-shared-resource.c | 26 ++
>  1 file changed, 22 insertions(+), 4 deletions(-)

Hmm...this thread-safety change is more fine-grained than I was
expecting. If we follow this strategy basically any data structure used
by coroutines needs its own fine-grained lock (like Java's Object base
class which has its own lock).

I'm not sure I like it since callers may still need coarser grained
locks to protect their own state or synchronize access to multiple
items of data. Also, some callers may not need thread-safety.

Can the caller to be responsible for locking instead (e.g. using
CoMutex)?

> diff --git a/util/qemu-co-shared-resource.c b/util/qemu-co-shared-resource.c
> index 1c83cd9d29..c455d02a1e 100644
> --- a/util/qemu-co-shared-resource.c
> +++ b/util/qemu-co-shared-resource.c
> @@ -32,6 +32,7 @@ struct SharedResource {
>  uint64_t available;
>  
>  CoQueue queue;
> +QemuMutex lock;

Please add a comment indicating what this lock protects.

Thread safety should also be documented in the header file so API users
know what to expect.


signature.asc
Description: PGP signature

Re: [PATCH 6/6] aiopool: protect with a mutex

[Stefan Hajnoczi]

On Mon, May 10, 2021 at 10:59:41AM +0200, Emanuele Giuseppe Esposito wrote:
> Divide the fields in AioTaskPool in IN and Status, and
> introduce a CoQueue instead of .wait to take care of suspending
> and resuming the calling coroutine, and a lock to protect the
> busy_tasks counter accesses and the AioTask .ret field.

The thread-safety concerns with the aio_task.h API are unclear to me.
The API is designed to have a "main" coroutine that adds task
functions to the pool and waits for them to complete. Task functions
execute in coroutines (up to the pool's max_busy_tasks limit).

It seems like the API was designed to be called only from its main
coroutine so why make everything thread-safe? Is there a caller that
shares an AioTaskPool between threads? Or will the task functions switch
threads somehow?

What exactly is the new thread-safety model? Please document it.
Unfortunately aio_task.h doesn't have doc comments already but it will
be necessary if there are thread-safety concerns.


signature.asc
Description: PGP signature

Re: [PATCH v2] block: Improve backing file validation

[Kevin Wolf]

Am 11.05.2021 um 10:35 hat Daniel P. Berrangé geschrieben:
> On Tue, May 11, 2021 at 01:55:18PM +0800, Li Zhijian wrote:
> > Image below user cases:
> > case 1:
> > ```
> > $ qemu-img create -f raw source.raw 1G
> > $ qemu-img create -f qcow2 -F raw -b source.raw ./source.raw
> > qemu-img info source.raw
> > image: source.raw
> > file format: qcow2
> > virtual size: 193K (197120 bytes)
> > disk size: 196K
> > cluster_size: 65536
> > backing file: source.raw <<
> > backing file format: raw
> > Format specific information:
> > compat: 1.1
> > lazy refcounts: false
> > refcount bits: 16
> > corrupt: false
> > ```
> > 
> > case 2:
> > ```
> > $ qemu-img create -f raw source.raw 1G
> > $ ln -sf source.raw destination.qcow2
> > $ qemu-img create -f qcow2 -F raw -b source.raw ./destination.qcow2
> > qemu-img info source.raw
> > image: source.raw
> > file format: qcow2 <<
> > virtual size: 2.0G (2147483648 bytes)
> > disk size: 196K
> > cluster_size: 65536
> > backing file: source.raw
> > backing file format: raw
> > Format specific information:
> > compat: 1.1
> > lazy refcounts: false
> > refcount bits: 16
> > corrupt: false
> > ```
> > Generally, we don't expect to corrupte the source.raw anyway, while
> > actually it does.
> > 
> > Here we check their inode number instead of file name.
> > 
> > Suggested-by: Daniel P. Berrangé 
> > Signed-off-by: Li Zhijian 
> > 
> > ---
> > v2: utilize stat() instead of realpath() (Daniel)
> > 
> > Signed-off-by: Li Zhijian 
> > ---
> >  block.c | 39 ---
> >  1 file changed, 32 insertions(+), 7 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 9ad725d205..db4ae57959 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -6431,6 +6431,37 @@ bool bdrv_op_blocker_is_empty(BlockDriverState *bs)
> >  return true;
> >  }
> >  
> > +static bool validate_backing_file(const char *filename,
> > +  const char *backing_file, Error **errp)
> > +{
> > +struct stat filename_stat, backing_stat;
> > +
> > +if (backing_file[0] == '\0') {
> > +error_setg(errp, "Expected backing file name, got empty string");
> > +goto out;
> > +}
> > +
> > +/* check whether filename and backing_file are refering to the same 
> > file */
> > +if (stat(backing_file, _stat) == -1) {
> > +error_setg(errp, "Cannot stat backing file %s", backing_file);
> > +goto out;
> > +}
> > +if (stat(filename, _stat) == -1) {
> > +/* Simply consider filename doesn't exist, no need to further 
> > check */
> > +return true;
> > +}
> > +if ((filename_stat.st_dev == backing_stat.st_dev) &&
> > +(filename_stat.st_ino == backing_stat.st_ino)) {
> > +error_setg(errp, "Error: Trying to create an image with the "
> > + "same filename as the backing file");
> > +goto out;
> > +}
> > +
> > +return true;
> > +out:
> > +return false;
> > +}
> > +
> >  void bdrv_img_create(const char *filename, const char *fmt,
> >   const char *base_filename, const char *base_fmt,
> >   char *options, uint64_t img_size, int flags, bool 
> > quiet,
> > @@ -6507,13 +6538,7 @@ void bdrv_img_create(const char *filename, const 
> > char *fmt,
> >  
> >  backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
> >  if (backing_file) {
> > -if (!strcmp(filename, backing_file)) {
> > -error_setg(errp, "Error: Trying to create an image with the "
> > - "same filename as the backing file");
> > -goto out;
> > -}
> > -if (backing_file[0] == '\0') {
> > -error_setg(errp, "Expected backing file name, got empty 
> > string");
> > +if (!validate_backing_file(filename, backing_file, errp)) {
> >  goto out;
> >  }
> >  }
> 
> Thinking about this again, this seems to be quite high in the generic block
> layer code. As such I don't think we can assume that the backing file here
> is actually a plain file on disk. IIUC the backing file could still be any
> of the block drivers. Only once we get down into the protocol specific
> drivers can be validate the type of backend.

Yes, you definitely can't assume that filename is really a local file
name here. It could be any other protocol supported by QEMU, or even use
the json: pseudo-protocol.

> I'm not sure what the right way to deal with that is, so perhaps Kevin or
> Max can make a suggestion.

Can we just keep the backing file open with write permissions unshared
so that locking will fail for the new image? Or would that error
condition be detected too late so that the image would already be
truncated?

Kevin

Re: [PATCH 0/3] vhost-user-blk-test: add tests for the vhost-user-blk server

[Kevin Wolf]

Am 11.05.2021 um 10:23 hat Stefan Hajnoczi geschrieben:
> On Mon, Mar 22, 2021 at 09:23:24AM +, Stefan Hajnoczi wrote:
> > These patches add a qtest for the vhost-user-blk server. CI found several
> > issues that caused these patches to be dropped from Michael Tsirkin and 
> > Kevin
> > Wolf's pull requests in the past. Hopefully they will go in smoothly this 
> > time!
> > 
> > Coiby Xu (1):
> >   test: new qTest case to test the vhost-user-blk-server
> > 
> > Stefan Hajnoczi (2):
> >   tests/qtest: add multi-queue test case to vhost-user-blk-test
> >   vhost-user-blk-test: test discard/write zeroes invalid inputs
> > 
> >  MAINTAINERS |   2 +
> >  tests/qtest/libqos/vhost-user-blk.h |  48 ++
> >  tests/qtest/libqos/vhost-user-blk.c | 130 
> >  tests/qtest/vhost-user-blk-test.c   | 989 
> >  tests/qtest/libqos/meson.build  |   1 +
> >  tests/qtest/meson.build |   4 +
> >  6 files changed, 1174 insertions(+)
> >  create mode 100644 tests/qtest/libqos/vhost-user-blk.h
> >  create mode 100644 tests/qtest/libqos/vhost-user-blk.c
> >  create mode 100644 tests/qtest/vhost-user-blk-test.c
> 
> Ping for QEMU 6.1

Thanks, applied to the block branch.

Kevin


signature.asc
Description: PGP signature

Re: [PATCH] block/export: improve vu_blk_sect_range_ok()

[Kevin Wolf]

Am 11.05.2021 um 10:25 hat Stefan Hajnoczi geschrieben:
> On Wed, Mar 31, 2021 at 03:27:27PM +0100, Stefan Hajnoczi wrote:
> > The checks in vu_blk_sect_range_ok() assume VIRTIO_BLK_SECTOR_SIZE is
> > equal to BDRV_SECTOR_SIZE. This is true, but let's add a
> > QEMU_BUILD_BUG_ON() to make it explicit.
> > 
> > We might as well check that the request buffer size is a multiple of
> > VIRTIO_BLK_SECTOR_SIZE while we're at it.
> > 
> > Suggested-by: Max Reitz 
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> >  block/export/vhost-user-blk-server.c | 9 -
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> Ping for QEMU 6.1.

Thanks, applied to the block branch.

Kevin


signature.asc
Description: PGP signature

Re: [PATCH 0/6] block-copy: make helper APIs thread safe

[Stefan Hajnoczi]

On Mon, May 10, 2021 at 10:59:35AM +0200, Emanuele Giuseppe Esposito wrote:
> This serie of patches bring thread safety to the smaller APIs used by
> block-copy, namely ratelimit, progressmeter, co-shared-resource
> and aiotask.
> The end goal is to reduce the usage of the global
> AioContexlock in block-copy, by introducing smaller granularity
> locks thus on making the block layer thread safe. 

I'm not sure "global" is accurate. Block jobs can deal with arbitrary
AioContexts, not just a global AioContext. The lock is per-AioContext
rather than global. Or did I miss something?


signature.asc
Description: PGP signature

Re: making a qdev bus available from a (non-qtree?) device

[Markus Armbruster]

Klaus Jensen  writes:

> Hi all,
>
> I need some help with grok'ing qdev busses. Stefan, Michael - David
> suggested on IRC that I CC'ed you guys since you might have solved a 
> similar issue with virtio devices. I've tried to study how that works,
> but I'm not exactly sure how to apply it to the issue I'm having.
>
> Currently, to support multiple namespaces on the emulated nvme device,
> one can do something like this:
>
>   -device nvme,id=nvme-ctrl-0,serial=foo,...
>   -device nvme-ns,id=nvme-ns-0,bus=nvme-ctrl-0,...
>   -device nvme-ns,id-nvme-ns-1,bus=nvme-ctrl-0,...
>
> The nvme device creates an 'nvme-bus' and the nvme-ns devices has
> dc->bus_type = TYPE_NVME_BUS. This all works very well and provides a 
> nice overview in `info qtree`:
>
>   bus: main-system-bus
>   type System
> ...
> dev: q35-pcihost, id ""
>   ..
>   bus: pcie.0
>   type PCIE
>   ..
>   dev: nvme, id "nvme-ctrl-0"
> ..
> bus: nvme-ctrl-0
>   type nvme-bus
>   dev: nvme-ns, id "nvme-ns-0"
> ..
>   dev: nvme-ns, id "nvme-ns-1"
> ..
>
>
> Nice and qdevy.
>
> We have since introduced support for NVM Subsystems through an
> nvme-subsys device. The nvme-subsys device is just a TYPE_DEVICE and 
> does not show in `info qtree`

Yes.

Most devices plug into a bus.  DeviceClass member @bus_type specifies
the type of bus they plug into, and DeviceState member @parent_bus
points to the actual BusState.  Example: PCI devices plug into a PCI
bus, and have ->bus_type = TYPE_PCI_BUS.

Some devices don't.  @bus_type and @parent_bus are NULL then.

Most buses are provided by a device.  BusState member @parent points to
the device.

The main-system-bus isn't.  Its @parent is null.

"info qtree" only shows the qtree rooted at main-system-bus.  It doesn't
show qtrees rooted at bus-less devices or device-less buses other than
main-system-bus.  I doubt such buses exist.

>   (I wonder if this should actually just
> have been an -object?).

Does nvme-subsys expose virtual hardware to the guest?  Memory, IRQs,
...

If yes, it needs to be a device.

If no, object may be more appropriate.  Tell us more about what it does.


> Anyway. The nvme device has a 'subsys' link 
> parameter and we use this to manage the namespaces across the
> subsystem that may contain several nvme devices (controllers). The
> problem is that this doesnt work too well with unplugging since if the
> nvme device is `device_del`'ed, the nvme-ns devices on the nvme-bus
> are unrealized which is not what we want. We really want the
> namespaces to linger, preferably on an nvme-bus of the nvme-subsys
> device so they can be attached to other nvme devices that may show up
> (or already exist) in the subsystem.
>
> The core problem I'm having is that I can't seem to create an nvme-bus
> from the nvme-subsys device and make it available to the nvme-ns
> device on the command line:
>
>   -device nvme-subsys,id=nvme-subsys-0,...
>   -device nvme-ns,bus=nvme-subsys-0
>
> The above results in 'No 'nvme-bus' bus found for device 'nvme-ns',
> even though I do `qbus_create_inplace()` just like the nvme
> device. However, I *can* reparent the nvme-ns device in its realize()
> method, so if I instead define it like so:
>
>   -device nvme-subsys,id=nvme-subsys-0,...
>   -device nvme,id=nvme-ctrl-0,subsys=nvme-subsys-0
>   -device nvme-ns,bus=nvme-ctrl-0
>
> I can then call `qdev_set_parent_bus()` and set the parent bus to the
> bus creates in the nvme-subsys device. This solves the problem since
> the namespaces are not "garbage collected" when the nvme device is
> removed, but it just feels wrong you know? Also, if possible, I'd of
> course really like to retain the nice entries in `info qtree`.

I'm afraid I'm too ignorant on NVME to give useful advice.

Can you give us a brief primer on the aspects of physical NVME devices
you'd like to model in QEMU?  What are "controllers", "namespaces", and
"subsystems", and how do they work together?

Once we understand the relevant aspects of physical devices, we can
discuss how to best model them in QEMU.

Re: [PATCH v3 21/33] qemu-socket: pass monitor link to socket_get_fd directly

[Daniel P . Berrangé]

On Wed, May 12, 2021 at 12:40:03PM +0300, Roman Kagan wrote:
> On Mon, Apr 19, 2021 at 10:34:49AM +0100, Daniel P. Berrangé wrote:
> > On Fri, Apr 16, 2021 at 11:08:59AM +0300, Vladimir Sementsov-Ogievskiy 
> > wrote:
> > > Detecting monitor by current coroutine works bad when we are not in
> > > coroutine context. And that's exactly so in nbd reconnect code, where
> > > qio_channel_socket_connect_sync() is called from thread.
> > > 
> > > Add a possibility to pass monitor by hand, to be used in the following
> > > commit.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy 
> > > ---
> > >  include/io/channel-socket.h| 20 
> > >  include/qemu/sockets.h |  2 +-
> > >  io/channel-socket.c| 17 +
> > >  tests/unit/test-util-sockets.c | 16 
> > >  util/qemu-sockets.c| 10 +-
> > >  5 files changed, 47 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> > > index e747e63514..6d0915420d 100644
> > > --- a/include/io/channel-socket.h
> > > +++ b/include/io/channel-socket.h
> > > @@ -78,6 +78,23 @@ qio_channel_socket_new_fd(int fd,
> > >Error **errp);
> > >  
> > >  
> > > +/**
> > > + * qio_channel_socket_connect_sync_mon:
> > > + * @ioc: the socket channel object
> > > + * @addr: the address to connect to
> > > + * @mon: current monitor. If NULL, it will be detected by
> > > + *   current coroutine.
> > > + * @errp: pointer to a NULL-initialized error object
> > > + *
> > > + * Attempt to connect to the address @addr. This method
> > > + * will run in the foreground so the caller will not regain
> > > + * execution control until the connection is established or
> > > + * an error occurs.
> > > + */
> > > +int qio_channel_socket_connect_sync_mon(QIOChannelSocket *ioc,
> > > +SocketAddress *addr,
> > > +Monitor *mon,
> > > +Error **errp);
> > 
> > I don't really like exposing the concept of the QEMU monitor in
> > the IO layer APIs. IMHO these ought to remain completely separate
> > subsystems from the API pov,
> 
> Agreed. 
> 
> > and we ought to fix this problem by
> > making monitor_cur() work better in the scenario required.
> 
> Would it make sense instead to resolve the fdstr into actual file
> descriptor number in the context where monitor_cur() works and makes
> sense, prior to passing it to the connection thread?

Yes, arguably the root problem is caused by the util/qemu-sockets.c
code directly referring to the current monitor. This has resultde in
the slightly strange scenario where we have two distinct semantics
for the 'fd' SocketAddressType

 @fd: decimal is for file descriptor number, otherwise a file descriptor name.
  Named file descriptors are permitted in monitor commands, in combination
  with the 'getfd' command. Decimal file descriptors are permitted at
  startup or other contexts where no monitor context is active.

Now these distinct semantics kinda make sense from the POV of the
management application, but we've let the dual semantics propagate
all the way down our I/O stack.

Folowing your idea, we could have  'socket_address_resolve_monitor_fd'
method which takes a 'SocketAddress' and returns a new 'SocketAddress'
with the real FD filled in.  We then call this method in any codepath
which is getting a 'SocketAddress' struct from the monitor.

The util/qemu-sockets.c code then only has to think about real FDs,
and the monitor_get_fd() call only occurs right at the top level.

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 v3 21/33] qemu-socket: pass monitor link to socket_get_fd directly

[Roman Kagan]

On Mon, Apr 19, 2021 at 10:34:49AM +0100, Daniel P. Berrangé wrote:
> On Fri, Apr 16, 2021 at 11:08:59AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > Detecting monitor by current coroutine works bad when we are not in
> > coroutine context. And that's exactly so in nbd reconnect code, where
> > qio_channel_socket_connect_sync() is called from thread.
> > 
> > Add a possibility to pass monitor by hand, to be used in the following
> > commit.
> > 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy 
> > ---
> >  include/io/channel-socket.h| 20 
> >  include/qemu/sockets.h |  2 +-
> >  io/channel-socket.c| 17 +
> >  tests/unit/test-util-sockets.c | 16 
> >  util/qemu-sockets.c| 10 +-
> >  5 files changed, 47 insertions(+), 18 deletions(-)
> > 
> > diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h
> > index e747e63514..6d0915420d 100644
> > --- a/include/io/channel-socket.h
> > +++ b/include/io/channel-socket.h
> > @@ -78,6 +78,23 @@ qio_channel_socket_new_fd(int fd,
> >Error **errp);
> >  
> >  
> > +/**
> > + * qio_channel_socket_connect_sync_mon:
> > + * @ioc: the socket channel object
> > + * @addr: the address to connect to
> > + * @mon: current monitor. If NULL, it will be detected by
> > + *   current coroutine.
> > + * @errp: pointer to a NULL-initialized error object
> > + *
> > + * Attempt to connect to the address @addr. This method
> > + * will run in the foreground so the caller will not regain
> > + * execution control until the connection is established or
> > + * an error occurs.
> > + */
> > +int qio_channel_socket_connect_sync_mon(QIOChannelSocket *ioc,
> > +SocketAddress *addr,
> > +Monitor *mon,
> > +Error **errp);
> 
> I don't really like exposing the concept of the QEMU monitor in
> the IO layer APIs. IMHO these ought to remain completely separate
> subsystems from the API pov,

Agreed. 

> and we ought to fix this problem by
> making monitor_cur() work better in the scenario required.

Would it make sense instead to resolve the fdstr into actual file
descriptor number in the context where monitor_cur() works and makes
sense, prior to passing it to the connection thread?

Roman.

Re: [PATCH v3 19/33] block/nbd: split nbd_handle_updated_info out of nbd_client_handshake()

[Roman Kagan]

On Fri, Apr 16, 2021 at 11:08:57AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> To be reused in the following patch.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/nbd.c | 99 ++---
>  1 file changed, 57 insertions(+), 42 deletions(-)

Reviewed-by: Roman Kagan 

Re: making a qdev bus available from a (non-qtree?) device

[Peter Maydell]

On Wed, 12 May 2021 at 04:39, Philippe Mathieu-Daudé  wrote:
> IIUC, while we can have unattached drives, we can't (by design) have
> qdev unattached to qbus.

You can (and we do), but it is a bit of a problem because a
device not attached to a qbus will not get automatically reset,
and so you need to arrange to reset it manually somehow.


-- PMM

[RFC PATCH] block/io.c: Flush parent for quorum in generic code

[Zhang Chen]

Fix the issue from this patch:
[PATCH] block: Flush all children in generic code
>From 883833e29cb800b4d92b5d4736252f4004885191

Quorum driver do not have the primary child.
It will caused guest block flush issue when use quorum and NBD.
The vm guest flushes failed，and then guest filesystem is shutdown.

Signed-off-by: Zhang Chen 
Reported-by: Minghao Yuan 
---
 block/io.c | 31 ++-
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/block/io.c b/block/io.c
index 35b6c56efc..4dc1873cb9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2849,6 +2849,13 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 BdrvChild *child;
 int current_gen;
 int ret = 0;
+bool no_primary_child = false;
+
+/* Quorum drivers do not have the primary child. */
+if (!primary_child) {
+primary_child = bs->file;
+no_primary_child = true;
+}
 
 bdrv_inc_in_flight(bs);
 
@@ -2886,12 +2893,12 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 
 /* But don't actually force it to the disk with cache=unsafe */
 if (bs->open_flags & BDRV_O_NO_FLUSH) {
-goto flush_children;
+goto flush_data;
 }
 
 /* Check if we really need to flush anything */
 if (bs->flushed_gen == current_gen) {
-goto flush_children;
+goto flush_data;
 }
 
 BLKDBG_EVENT(primary_child, BLKDBG_FLUSH_TO_DISK);
@@ -2938,13 +2945,19 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
 /* Now flush the underlying protocol.  It will also have BDRV_O_NO_FLUSH
  * in the case of cache=unsafe, so there are no useless flushes.
  */
-flush_children:
-ret = 0;
-QLIST_FOREACH(child, >children, next) {
-if (child->perm & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) {
-int this_child_ret = bdrv_co_flush(child->bs);
-if (!ret) {
-ret = this_child_ret;
+flush_data:
+if (no_primary_child) {
+/* Flush parent */
+ret = bs->file ? bdrv_co_flush(bs->file->bs) : 0;
+} else {
+/* Flush childrens */
+ret = 0;
+QLIST_FOREACH(child, >children, next) {
+if (child->perm & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) {
+int this_child_ret = bdrv_co_flush(child->bs);
+if (!ret) {
+ret = this_child_ret;
+}
 }
 }
 }
-- 
2.25.1

Re: [PATCH v3 31/33] block/nbd: add nbd_clinent_connected() helper

[Vladimir Sementsov-Ogievskiy]

12.05.2021 10:06, Paolo Bonzini wrote:

On 16/04/21 10:09, Vladimir Sementsov-Ogievskiy wrote:

We already have two similar helpers for other state. Let's add another
one for convenience.

Signed-off-by: Vladimir Sementsov-Ogievskiy


The whole usage of load-acquire for the connected state is completely 
meaningless, but that's not your fault. :)  For now let's keep it as is, and 
we'll clean it up later.



It would be nice, thanks!


--
Best regards,
Vladimir

Re: [PATCH v3 06/33] util/async: aio_co_schedule(): support reschedule in same ctx

[Vladimir Sementsov-Ogievskiy]

12.05.2021 09:56, Paolo Bonzini wrote:

On 16/04/21 10:08, Vladimir Sementsov-Ogievskiy wrote:

With the following patch we want to call wake coroutine from thread.
And it doesn't work with aio_co_wake:
Assume we have no iothreads.
Assume we have a coroutine A, which waits in the yield point for
external aio_co_wake(), and no progress can be done until it happen.
Main thread is in blocking aio_poll() (for example, in blk_read()).

Now, in a separate thread we do aio_co_wake(). It calls  aio_co_enter(),
which goes through last "else" branch and do aio_context_acquire(ctx).


I don't understand.  Why doesn't aio_co_enter go through the ctx != 
qemu_get_current_aio_context() branch and just do aio_co_schedule?  That was at 
least the idea behind aio_co_wake and aio_co_enter.



Because ctx is exactly qemu_get_current_aio_context(), as we are not in 
iothread but in nbd connection thread. So, qemu_get_current_aio_context() 
returns qemu_aio_context.


--
Best regards,
Vladimir

Re: [PATCH 0/3] hw/virtio: Constify VirtIOFeature

[Stefano Garzarella]

On Tue, May 11, 2021 at 12:41:54PM +0200, Philippe Mathieu-Daudé wrote:

Trivial patches to keep VirtIOFeature arrays read-only
(better safe than sorry).

Philippe Mathieu-Daudé (3):
 hw/virtio: Pass virtio_feature_get_config_size() a const argument
 virtio-blk: Constify VirtIOFeature feature_sizes[]
 virtio-net: Constify VirtIOFeature feature_sizes[]

include/hw/virtio/virtio.h | 2 +-
hw/block/virtio-blk.c  | 2 +-
hw/net/virtio-net.c| 2 +-
hw/virtio/virtio.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)



Reviewed-by: Stefano Garzarella 

Re: [PATCH 4/6] progressmeter: protect with a mutex

[Vladimir Sementsov-Ogievskiy]

11.05.2021 15:28, Paolo Bonzini wrote:

On 10/05/21 13:28, Vladimir Sementsov-Ogievskiy wrote:



Could we instead add a c file and add the structure private? Then we'll have 
progress_new() and progress_free() APIs instead.

This way, it would be a lot simpler to control that nobady use structure fields 
directly.


I don't know...  I prefer embedding structs to heap allocation.




I agree that embedding looks better from allocation point of view.. But IMHO 
encapsulation guarantee of private structure is better feature. Especially when 
we have getter/setter methods. Even the patch itself is example: to review this 
carefully, I should somehow check that every use of the structure is updated 
(grepping the code, or maybe change field name and recompile, to catch every 
occurrence of it).. And if patch makes structure private and adds 
setters/getters, it can be reviewed without applying.

And we have to call _init/_destroy anyway, so it's not more complex to call 
_new/_free instead.

--
Best regards,
Vladimir

Re: [PATCH v3 31/33] block/nbd: add nbd_clinent_connected() helper

[Paolo Bonzini]

On 16/04/21 10:09, Vladimir Sementsov-Ogievskiy wrote:

We already have two similar helpers for other state. Let's add another
one for convenience.

Signed-off-by: Vladimir Sementsov-Ogievskiy


The whole usage of load-acquire for the connected state is completely 
meaningless, but that's not your fault. :)  For now let's keep it as is, 
and we'll clean it up later.


Paolo

Re: [PATCH v3 06/33] util/async: aio_co_schedule(): support reschedule in same ctx

[Paolo Bonzini]

On 16/04/21 10:08, Vladimir Sementsov-Ogievskiy wrote:

With the following patch we want to call wake coroutine from thread.
And it doesn't work with aio_co_wake:
Assume we have no iothreads.
Assume we have a coroutine A, which waits in the yield point for
external aio_co_wake(), and no progress can be done until it happen.
Main thread is in blocking aio_poll() (for example, in blk_read()).

Now, in a separate thread we do aio_co_wake(). It calls  aio_co_enter(),
which goes through last "else" branch and do aio_context_acquire(ctx).


I don't understand.  Why doesn't aio_co_enter go through the ctx != 
qemu_get_current_aio_context() branch and just do aio_co_schedule?  That 
was at least the idea behind aio_co_wake and aio_co_enter.


Paolo

Re: [PATCH v3 00/33] block/nbd: rework client connection

[Paolo Bonzini]

On 16/04/21 10:08, Vladimir Sementsov-Ogievskiy wrote:

The series substitutes "[PATCH v2 00/10] block/nbd: move connection code to separate 
file"
Supersedes: <20210408140827.332915-1-vsement...@virtuozzo.com>
so it's called v3

block/nbd.c is overcomplicated. These series is a big refactoring, which
finally drops all the complications around drained sections and context
switching, including abuse of bs->in_flight counter.

Also, at the end of the series we don't cancel reconnect on drained
sections (and don't cancel requests waiting for reconnect on drained
section begin), which fixes a problem reported by Roman.

The series is also available at tag up-nbd-client-connection-v3 in
git https://src.openvz.org/scm/~vsementsov/qemu.git


I have independently done some rework of the connection state machine, 
mostly in order to use the QemuCoSleep API instead of aio_co_wake.  In 
general it seems to be independent of this work.  I'll review this series.


Paolo


v3:
Changes in first part of the series (main thing is not using refcnt, but 
instead (modified) Roman's patch):

01-04: new
05: add Roman's r-b
06: new
07: now, new aio_co_schedule(NULL, thr->wait_co) is used
08: reworked, we now need also bool detached, as we don't have refcnt
09,10: add Roman's r-b
11: rebased, don't modify nbd_free_connect_thread() name at this point
12: add Roman's r-b
13: new
14: rebased

Other patches are new.

Roman Kagan (2):
   block/nbd: fix channel object leak
   block/nbd: ensure ->connection_thread is always valid

Vladimir Sementsov-Ogievskiy (31):
   block/nbd: fix how state is cleared on nbd_open() failure paths
   block/nbd: nbd_client_handshake(): fix leak of s->ioc
   block/nbd: BDRVNBDState: drop unused connect_err and connect_status
   util/async: aio_co_schedule(): support reschedule in same ctx
   block/nbd: simplify waking of nbd_co_establish_connection()
   block/nbd: drop thr->state
   block/nbd: bs-independent interface for nbd_co_establish_connection()
   block/nbd: make nbd_co_establish_connection_cancel() bs-independent
   block/nbd: rename NBDConnectThread to NBDClientConnection
   block/nbd: introduce nbd_client_connection_new()
   block/nbd: introduce nbd_client_connection_release()
   nbd: move connection code from block/nbd to nbd/client-connection
   nbd/client-connection: use QEMU_LOCK_GUARD
   nbd/client-connection: add possibility of negotiation
   nbd/client-connection: implement connection retry
   nbd/client-connection: shutdown connection on release
   block/nbd: split nbd_handle_updated_info out of nbd_client_handshake()
   block/nbd: use negotiation of NBDClientConnection
   qemu-socket: pass monitor link to socket_get_fd directly
   block/nbd: pass monitor directly to connection thread
   block/nbd: nbd_teardown_connection() don't touch s->sioc
   block/nbd: drop BDRVNBDState::sioc
   nbd/client-connection: return only one io channel
   block-coroutine-wrapper: allow non bdrv_ prefix
   block/nbd: split nbd_co_do_establish_connection out of
 nbd_reconnect_attempt
   nbd/client-connection: do qio_channel_set_delay(false)
   nbd/client-connection: add option for non-blocking connection attempt
   block/nbd: reuse nbd_co_do_establish_connection() in nbd_open()
   block/nbd: add nbd_clinent_connected() helper
   block/nbd: safer transition to receiving request
   block/nbd: drop connection_co

  block/coroutines.h |   6 +
  include/block/aio.h|   2 +-
  include/block/nbd.h|  19 +
  include/io/channel-socket.h|  20 +
  include/qemu/sockets.h |   2 +-
  block/nbd.c| 908 +++--
  io/channel-socket.c|  17 +-
  nbd/client-connection.c| 364 
  nbd/client.c   |   2 -
  tests/unit/test-util-sockets.c |  16 +-
  util/async.c   |   8 +
  util/qemu-sockets.c|  10 +-
  nbd/meson.build|   1 +
  scripts/block-coroutine-wrapper.py |   7 +-
  14 files changed, 666 insertions(+), 716 deletions(-)
  create mode 100644 nbd/client-connection.c

Re: [PATCH v3 16/33] nbd/client-connection: add possibility of negotiation

[Vladimir Sementsov-Ogievskiy]

11.05.2021 13:45, Roman Kagan wrote:

On Fri, Apr 16, 2021 at 11:08:54AM +0300, Vladimir Sementsov-Ogievskiy wrote:

Add arguments and logic to support nbd negotiation in the same thread
after successful connection.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/nbd.h |   9 +++-
  block/nbd.c |   4 +-
  nbd/client-connection.c | 105 ++--
  3 files changed, 109 insertions(+), 9 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 57381be76f..5d86e6a393 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -409,11 +409,16 @@ const char *nbd_err_lookup(int err);
  /* nbd/client-connection.c */
  typedef struct NBDClientConnection NBDClientConnection;
  
-NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr);

+NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr,
+   bool do_negotiation,
+   const char *export_name,
+   const char *x_dirty_bitmap,
+   QCryptoTLSCreds *tlscreds);
  void nbd_client_connection_release(NBDClientConnection *conn);
  
  QIOChannelSocket *coroutine_fn

-nbd_co_establish_connection(NBDClientConnection *conn, Error **errp);
+nbd_co_establish_connection(NBDClientConnection *conn, NBDExportInfo *info,
+QIOChannel **ioc, Error **errp);
  
  void coroutine_fn nbd_co_establish_connection_cancel(NBDClientConnection *conn);
  
diff --git a/block/nbd.c b/block/nbd.c

index 9bd68dcf10..5e63caaf4b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -361,7 +361,7 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState 
*s)
  s->ioc = NULL;
  }
  
-s->sioc = nbd_co_establish_connection(s->conn, NULL);

+s->sioc = nbd_co_establish_connection(s->conn, NULL, NULL, NULL);
  if (!s->sioc) {
  ret = -ECONNREFUSED;
  goto out;
@@ -2033,7 +2033,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, 
int flags,
  goto fail;
  }
  
-s->conn = nbd_client_connection_new(s->saddr);

+s->conn = nbd_client_connection_new(s->saddr, false, NULL, NULL, NULL);
  
  /*

   * establish TCP connection, return error if it fails
diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index b45a0bd5f6..ae4a77f826 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -30,14 +30,19 @@
  #include "qapi/clone-visitor.h"
  
  struct NBDClientConnection {

-/* Initialization constants */
+/* Initialization constants, never change */
  SocketAddress *saddr; /* address to connect to */
+QCryptoTLSCreds *tlscreds;
+NBDExportInfo initial_info;
+bool do_negotiation;
  
  /*

   * Result of last attempt. Valid in FAIL and SUCCESS states.
   * If you want to steal error, don't forget to set pointer to NULL.
   */
+NBDExportInfo updated_info;
  QIOChannelSocket *sioc;
+QIOChannel *ioc;
  Error *err;
  
  QemuMutex mutex;

@@ -47,12 +52,25 @@ struct NBDClientConnection {
  Coroutine *wait_co; /* nbd_co_establish_connection() wait in yield() */
  };
  
-NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr)

+NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr,
+   bool do_negotiation,
+   const char *export_name,
+   const char *x_dirty_bitmap,
+   QCryptoTLSCreds *tlscreds)
  {
  NBDClientConnection *conn = g_new(NBDClientConnection, 1);
  
+object_ref(OBJECT(tlscreds));

  *conn = (NBDClientConnection) {
  .saddr = QAPI_CLONE(SocketAddress, saddr),
+.tlscreds = tlscreds,
+.do_negotiation = do_negotiation,
+
+.initial_info.request_sizes = true,
+.initial_info.structured_reply = true,
+.initial_info.base_allocation = true,
+.initial_info.x_dirty_bitmap = g_strdup(x_dirty_bitmap),
+.initial_info.name = g_strdup(export_name ?: "")
  };
  
  qemu_mutex_init(>mutex);

@@ -68,9 +86,59 @@ static void 
nbd_client_connection_do_free(NBDClientConnection *conn)
  }
  error_free(conn->err);
  qapi_free_SocketAddress(conn->saddr);
+object_unref(OBJECT(conn->tlscreds));
+g_free(conn->initial_info.x_dirty_bitmap);
+g_free(conn->initial_info.name);
  g_free(conn);
  }
  
+/*

+ * Connect to @addr and do NBD negotiation if @info is not null. If @tlscreds
+ * given @outioc is provided. @outioc is provided only on success.  The call 
may


s/given/are given/
s/provided/returned/g


+ * be cancelled in parallel by simply qio_channel_shutdown(sioc).


I assume by "in parallel" you mean "from another thread", I'd suggest to
spell 

Re: [PATCH v3 18/33] nbd/client-connection: shutdown connection on release

[Vladimir Sementsov-Ogievskiy]

12.05.2021 00:08, Roman Kagan wrote:

On Fri, Apr 16, 2021 at 11:08:56AM +0300, Vladimir Sementsov-Ogievskiy wrote:

Now, when thread can do negotiation and retry, it may run relatively
long. We need a mechanism to stop it, when user is not interested in
result anymore. So, on nbd_client_connection_release() let's shutdown
the socked, and do not retry connection if thread is detached.


This kinda answers my question to the previous patch about reconnect
cancellation.


Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  nbd/client-connection.c | 36 ++--
  1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index 002bd91f42..54f73c6c24 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -158,9 +158,13 @@ static void *connect_thread_func(void *opaque)
  uint64_t timeout = 1;
  uint64_t max_timeout = 16;
  
-while (true) {

+qemu_mutex_lock(>mutex);
+while (!conn->detached) {
+assert(!conn->sioc);
  conn->sioc = qio_channel_socket_new();
  
+qemu_mutex_unlock(>mutex);

+
  error_free(conn->err);
  conn->err = NULL;
  conn->updated_info = conn->initial_info;
@@ -171,14 +175,20 @@ static void *connect_thread_func(void *opaque)
  conn->updated_info.x_dirty_bitmap = NULL;
  conn->updated_info.name = NULL;
  
+qemu_mutex_lock(>mutex);

+
  if (ret < 0) {
  object_unref(OBJECT(conn->sioc));
  conn->sioc = NULL;
-if (conn->do_retry) {
+if (conn->do_retry && !conn->detached) {
+qemu_mutex_unlock(>mutex);
+
  sleep(timeout);
  if (timeout < max_timeout) {
  timeout *= 2;
  }
+
+qemu_mutex_lock(>mutex);
  continue;
  }
  }
@@ -186,15 +196,17 @@ static void *connect_thread_func(void *opaque)
  break;
  }
  
-WITH_QEMU_LOCK_GUARD(>mutex) {

-assert(conn->running);
-conn->running = false;
-if (conn->wait_co) {
-aio_co_schedule(NULL, conn->wait_co);
-conn->wait_co = NULL;
-}
-do_free = conn->detached;
+/* mutex is locked */
+
+assert(conn->running);
+conn->running = false;
+if (conn->wait_co) {
+aio_co_schedule(NULL, conn->wait_co);
+conn->wait_co = NULL;
  }
+do_free = conn->detached;
+
+qemu_mutex_unlock(>mutex);


This basically reverts some hunks from patch 15 "nbd/client-connection:
use QEMU_LOCK_GUARD".  How about dropping them there (they weren't an
obvious improvement even then).



OK, will do



  
  if (do_free) {

  nbd_client_connection_do_free(conn);
@@ -215,6 +227,10 @@ void nbd_client_connection_release(NBDClientConnection 
*conn)
  if (conn->running) {
  conn->detached = true;
  }
+if (conn->sioc) {
+qio_channel_shutdown(QIO_CHANNEL(conn->sioc),
+ QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+}
  do_free = !conn->running && !conn->detached;
  qemu_mutex_unlock(>mutex);
  
--

2.29.2




--
Best regards,
Vladimir

Re: [PATCH v4 06/11] block: make BlockLimits::max_pwrite_zeroes 64bit

[Vladimir Sementsov-Ogievskiy]

12.05.2021 00:19, Eric Blake wrote:

On 3/24/21 3:51 PM, Vladimir Sementsov-Ogievskiy wrote:

We are going to support 64 bit write-zeroes requests. Now update the
limit variable. It's absolutely safe. The variable is set in some
drivers, and used in bdrv_co_do_pwrite_zeroes().

Update also max_write_zeroes variable in bdrv_co_do_pwrite_zeroes(), so
that bdrv_co_do_pwrite_zeroes() is now prepared to 64bit requests. The
remaining logic including num, offset and bytes variables is already
supporting 64bit requests.

So the only thing that prevents 64 bit requests is limiting
max_write_zeroes variable to INT_MAX in bdrv_co_do_pwrite_zeroes().
We'll drop this limitation after updating all block drivers.

Ah, we also have bdrv_check_request32() in bdrv_co_pwritev_part(). It
will be modified to do bdrv_check_request() for write-zeroes path.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/block_int.h | 7 +++
  block/io.c| 2 +-
  2 files changed, 4 insertions(+), 5 deletions(-)




+++ b/include/block/block_int.h
@@ -676,10 +676,9 @@ typedef struct BlockLimits {
   * that is set. May be 0 if bl.request_alignment is good enough */
  uint32_t pdiscard_alignment;
  
-/* Maximum number of bytes that can zeroized at once (since it is

- * signed, it must be < 2G, if set). Must be multiple of
- * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */
-int32_t max_pwrite_zeroes;
+/* Maximum number of bytes that can zeroized at once. Must be multiple of
+ * pwrite_zeroes_alignment. May be 0 if no inherent 64-bit limit */


Is the comment still right?

Leaving as 0 is the easiest way for a driver to say "default limit", but
I would feel safer with the default being 2G-align rather than 63-bit
limit.  And it is a 63-bit limit, not 64-bit, if the driver opts in to
INT64_MAX.



May be, just s/no inherent 64-bit limit/no limit/ ?

I think that no-limit is better default: let's motivate drivers to be 64bit. 
And if they don't want, they should specify limit by hand.

Hmm, also, you missed v5 of this series, rebased on master. There almost no 
changes, so your answers should apply to it as well.




+int64_t max_pwrite_zeroes;
  
  /* Optimal alignment for write zeroes requests in bytes. A power

   * of 2 is best but not mandatory.  Must be a multiple of
diff --git a/block/io.c b/block/io.c
index 55095dd08e..79e600af27 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1836,7 +1836,7 @@ static int coroutine_fn 
bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
  int head = 0;
  int tail = 0;
  
-int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);

+int64_t max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX);


You are correct that for now we have no behavior change; a driver opting
in to a larger limit will still be clamped until we revisit this patch
later to drop the MIN() - but I agree with your approach of keeping
MIN() here until all drivers are audited.




--
Best regards,
Vladimir