[PATCH v2 11/15] iotests: split linters.py out from 297

[John Snow]

Now, 297 is just the iotests-specific incantations and linters.py is as
minimal as I can think to make it. The only remaining element in here
that ought to be configuration and not code is the list of skip files,
but they're still numerous enough that repeating them for mypy and
pylint configurations both would be ... a hassle.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297| 72 +
 tests/qemu-iotests/linters.py | 76 +++
 2 files changed, 87 insertions(+), 61 deletions(-)
 create mode 100644 tests/qemu-iotests/linters.py

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b7d9d6077b3..ee78a627359 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -17,74 +17,24 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-import re
 import subprocess
 import sys
-from typing import List, Mapping, Optional
+from typing import List
 
 import iotests
+import linters
 
 
-# TODO: Empty this list!
-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', '194', '196', '202',
-'203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
-'218', '219', '224', '228', '234', '235', '236', '237', '238',
-'240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
-'262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
-'299', '302', '303', '304', '307',
-'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
-)
-
-
-def is_python_file(filename):
-if not os.path.isfile(filename):
-return False
-
-if filename.endswith('.py'):
-return True
-
-with open(filename, encoding='utf-8') as f:
-try:
-first_line = f.readline()
-return re.match('^#!.*python', first_line) is not None
-except UnicodeDecodeError:  # Ignore binary files
-return False
-
-
-def get_test_files() -> List[str]:
-named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
-check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
-return list(filter(is_python_file, check_tests))
-
-
-def run_linter(
-tool: str,
-args: List[str],
-env: Optional[Mapping[str, str]] = None,
-suppress_output: bool = False,
-) -> None:
-"""
-Run a python-based linting tool.
-
-:param suppress_output: If True, suppress all stdout/stderr output.
-:raise CalledProcessError: If the linter process exits with failure.
-"""
-subprocess.run(
-('python3', '-m', tool, *args),
-env=env,
-check=True,
-stdout=subprocess.PIPE if suppress_output else None,
-stderr=subprocess.STDOUT if suppress_output else None,
-universal_newlines=True,
-)
+# Looking for something?
+#
+#  List of files to exclude from linting: linters.py
+#  mypy configuration:mypy.ini
+#  pylint configuration:  pylintrc
 
 
 def check_linter(linter: str) -> bool:
 try:
-run_linter(linter, ['--version'], suppress_output=True)
+linters.run_linter(linter, ['--version'], suppress_output=True)
 except subprocess.CalledProcessError:
 iotests.case_notrun(f"'{linter}' not found")
 return False
@@ -98,7 +48,7 @@ def test_pylint(files: List[str]) -> None:
 if not check_linter('pylint'):
 return
 
-run_linter('pylint', files)
+linters.run_linter('pylint', files)
 
 
 def test_mypy(files: List[str]) -> None:
@@ -111,11 +61,11 @@ def test_mypy(files: List[str]) -> None:
 env = os.environ.copy()
 env['MYPYPATH'] = env['PYTHONPATH']
 
-run_linter('mypy', files, env=env, suppress_output=True)
+linters.run_linter('mypy', files, env=env, suppress_output=True)
 
 
 def main() -> None:
-files = get_test_files()
+files = linters.get_test_files()
 
 iotests.logger.debug('Files to be checked:')
 iotests.logger.debug(', '.join(sorted(files)))
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
new file mode 100644
index 000..c515c7afe36
--- /dev/null
+++ b/tests/qemu-iotests/linters.py
@@ -0,0 +1,76 @@
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received 

[PATCH v2 10/15] iotests/297: split test into sub-cases

[John Snow]

Take iotest 297's main() test function and split it into two sub-cases
that can be skipped individually. We can also drop custom environment
setup from the pylint test as it isn't needed.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 63 ++
 1 file changed, 39 insertions(+), 24 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b2ad8d1cbe0..b7d9d6077b3 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -82,36 +82,51 @@ def run_linter(
 )
 
 
+def check_linter(linter: str) -> bool:
+try:
+run_linter(linter, ['--version'], suppress_output=True)
+except subprocess.CalledProcessError:
+iotests.case_notrun(f"'{linter}' not found")
+return False
+return True
+
+
+def test_pylint(files: List[str]) -> None:
+print('=== pylint ===')
+sys.stdout.flush()
+
+if not check_linter('pylint'):
+return
+
+run_linter('pylint', files)
+
+
+def test_mypy(files: List[str]) -> None:
+print('=== mypy ===')
+sys.stdout.flush()
+
+if not check_linter('mypy'):
+return
+
+env = os.environ.copy()
+env['MYPYPATH'] = env['PYTHONPATH']
+
+run_linter('mypy', files, env=env, suppress_output=True)
+
+
 def main() -> None:
-for linter in ('pylint', 'mypy'):
-try:
-run_linter(linter, ['--version'], suppress_output=True)
-except subprocess.CalledProcessError:
-iotests.notrun(f"'{linter}' not found")
-
 files = get_test_files()
 
 iotests.logger.debug('Files to be checked:')
 iotests.logger.debug(', '.join(sorted(files)))
 
-env = os.environ.copy()
-env['MYPYPATH'] = env['PYTHONPATH']
-
-print('=== pylint ===')
-sys.stdout.flush()
-try:
-run_linter('pylint', files, env=env)
-except subprocess.CalledProcessError:
-# pylint failure will be caught by diffing the IO.
-pass
-
-print('=== mypy ===')
-sys.stdout.flush()
-try:
-run_linter('mypy', files, env=env, suppress_output=True)
-except subprocess.CalledProcessError as exc:
-if exc.output:
-print(exc.output)
+for test in (test_pylint, test_mypy):
+try:
+test(files)
+except subprocess.CalledProcessError as exc:
+# Linter failure will be caught by diffing the IO.
+if exc.output:
+print(exc.output)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH v2 12/15] iotests/linters: Add entry point for linting via Python CI

[John Snow]

We need at least a tiny little shim here to join test file discovery
with test invocation. This logic could conceivably be hosted somewhere
in python/, but I felt it was strictly the least-rude thing to keep the
test logic here in iotests/, even if this small function isn't itself an
iotest.

Note that we don't actually even need the executable bit here, we'll be
relying on the ability to run this module as a script using Python CLI
arguments. No chance it gets misunderstood as an actual iotest that way.

(It's named, not in tests/, doesn't have the execute bit, and doesn't
have an execution shebang.)

Signed-off-by: John Snow 
---
 tests/qemu-iotests/linters.py | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index c515c7afe36..46c28fdcda0 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -16,6 +16,7 @@
 import os
 import re
 import subprocess
+import sys
 from typing import List, Mapping, Optional
 
 
@@ -74,3 +75,29 @@ def run_linter(
 stderr=subprocess.STDOUT if suppress_output else None,
 universal_newlines=True,
 )
+
+
+def main() -> None:
+"""
+Used by the Python CI system as an entry point to run these linters.
+"""
+def show_usage() -> None:
+print(f"Usage: {sys.argv[0]} < --mypy | --pylint >", file=sys.stderr)
+sys.exit(1)
+
+if len(sys.argv) != 2:
+show_usage()
+
+files = get_test_files()
+
+if sys.argv[1] == '--pylint':
+run_linter('pylint', files)
+elif sys.argv[1] == '--mypy':
+run_linter('mypy', files)
+else:
+print(f"Unrecognized argument: '{sys.argv[1]}'", file=sys.stderr)
+show_usage()
+
+
+if __name__ == '__main__':
+main()
-- 
2.31.1

[PATCH v2 09/15] iotests/297: update tool availability checks

[John Snow]

As mentioned in 'iotests/297: Don't rely on distro-specific linter
binaries', these checks are overly strict. Update them to be in-line
with how we actually invoke the linters themselves.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 76d6a23f531..b2ad8d1cbe0 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -18,7 +18,6 @@
 
 import os
 import re
-import shutil
 import subprocess
 import sys
 from typing import List, Mapping, Optional
@@ -84,9 +83,11 @@ def run_linter(
 
 
 def main() -> None:
-for linter in ('pylint-3', 'mypy'):
-if shutil.which(linter) is None:
-iotests.notrun(f'{linter} not found')
+for linter in ('pylint', 'mypy'):
+try:
+run_linter(linter, ['--version'], suppress_output=True)
+except subprocess.CalledProcessError:
+iotests.notrun(f"'{linter}' not found")
 
 files = get_test_files()
 
-- 
2.31.1

[PATCH v2 08/15] iotests/297: Change run_linter() to raise an exception on failure

[John Snow]

Instead of using a process return code as the python function return
value (or just not returning anything at all), allow run_linter() to
raise an exception instead.

The responsibility for printing output on error shifts from the function
itself to the caller, who will know best how to present/format that
information. (Also, "suppress_output" is now a lot more accurate of a
parameter name.)

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 24 ++--
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index d21673a2929..76d6a23f531 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -70,22 +70,18 @@ def run_linter(
 """
 Run a python-based linting tool.
 
-If suppress_output is True, capture stdout/stderr of the child
-process and only print that information back to stdout if the child
-process's return code was non-zero.
+:param suppress_output: If True, suppress all stdout/stderr output.
+:raise CalledProcessError: If the linter process exits with failure.
 """
-p = subprocess.run(
+subprocess.run(
 ('python3', '-m', tool, *args),
 env=env,
-check=False,
+check=True,
 stdout=subprocess.PIPE if suppress_output else None,
 stderr=subprocess.STDOUT if suppress_output else None,
 universal_newlines=True,
 )
 
-if suppress_output and p.returncode != 0:
-print(p.stdout)
-
 
 def main() -> None:
 for linter in ('pylint-3', 'mypy'):
@@ -102,11 +98,19 @@ def main() -> None:
 
 print('=== pylint ===')
 sys.stdout.flush()
-run_linter('pylint', files, env=env)
+try:
+run_linter('pylint', files, env=env)
+except subprocess.CalledProcessError:
+# pylint failure will be caught by diffing the IO.
+pass
 
 print('=== mypy ===')
 sys.stdout.flush()
-run_linter('mypy', files, env=env, suppress_output=True)
+try:
+run_linter('mypy', files, env=env, suppress_output=True)
+except subprocess.CalledProcessError as exc:
+if exc.output:
+print(exc.output)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH v2 04/15] iotests/297: Create main() function

[John Snow]

Instead of running "run_linters" directly, create a main() function that
will be responsible for environment setup, leaving run_linters()
responsible only for execution of the linters.

(That environment setup will be moved over in forthcoming commits.)

Signed-off-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Hanna Reitz 
---
 tests/qemu-iotests/297 | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 15b54594c11..163ebc8ebfd 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -89,8 +89,12 @@ def run_linters():
 print(p.stdout)
 
 
-for linter in ('pylint-3', 'mypy'):
-if shutil.which(linter) is None:
-iotests.notrun(f'{linter} not found')
+def main() -> None:
+for linter in ('pylint-3', 'mypy'):
+if shutil.which(linter) is None:
+iotests.notrun(f'{linter} not found')
 
-iotests.script_main(run_linters)
+run_linters()
+
+
+iotests.script_main(main)
-- 
2.31.1

[PATCH v2 02/15] iotests/297: Split mypy configuration out into mypy.ini

[John Snow]

More separation of code and configuration.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
 tests/qemu-iotests/297  | 14 +-
 tests/qemu-iotests/mypy.ini | 12 
 2 files changed, 13 insertions(+), 13 deletions(-)
 create mode 100644 tests/qemu-iotests/mypy.ini

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index bc3a0ceb2aa..b8101e6024a 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -73,19 +73,7 @@ def run_linters():
 sys.stdout.flush()
 
 env['MYPYPATH'] = env['PYTHONPATH']
-p = subprocess.run(('mypy',
-'--warn-unused-configs',
-'--disallow-subclassing-any',
-'--disallow-any-generics',
-'--disallow-incomplete-defs',
-'--disallow-untyped-decorators',
-'--no-implicit-optional',
-'--warn-redundant-casts',
-'--warn-unused-ignores',
-'--no-implicit-reexport',
-'--namespace-packages',
-'--scripts-are-modules',
-*files),
+p = subprocess.run(('mypy', *files),
env=env,
check=False,
stdout=subprocess.PIPE,
diff --git a/tests/qemu-iotests/mypy.ini b/tests/qemu-iotests/mypy.ini
new file mode 100644
index 000..4c0339f5589
--- /dev/null
+++ b/tests/qemu-iotests/mypy.ini
@@ -0,0 +1,12 @@
+[mypy]
+disallow_any_generics = True
+disallow_incomplete_defs = True
+disallow_subclassing_any = True
+disallow_untyped_decorators = True
+implicit_reexport = False
+namespace_packages = True
+no_implicit_optional = True
+scripts_are_modules = True
+warn_redundant_casts = True
+warn_unused_configs = True
+warn_unused_ignores = True
-- 
2.31.1

[PATCH v2 01/15] iotests/297: Move pylint config into pylintrc

[John Snow]

Move --score=n and --notes=XXX,FIXME into pylintrc. This pulls
configuration out of code, which I think is probably a good thing in
general.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
 tests/qemu-iotests/297  |  4 +---
 tests/qemu-iotests/pylintrc | 16 
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 91ec34d9521..bc3a0ceb2aa 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -65,10 +65,8 @@ def run_linters():
 print('=== pylint ===')
 sys.stdout.flush()
 
-# Todo notes are fine, but fixme's or xxx's should probably just be
-# fixed (in tests, at least)
 env = os.environ.copy()
-subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
+subprocess.run(('pylint-3', *files),
env=env, check=False)
 
 print('=== mypy ===')
diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index 8cb4e1d6a6d..32ab77b8bb9 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -31,6 +31,22 @@ disable=invalid-name,
 too-many-statements,
 consider-using-f-string,
 
+
+[REPORTS]
+
+# Activate the evaluation score.
+score=no
+
+
+[MISCELLANEOUS]
+
+# List of note tags to take in consideration, separated by a comma.
+# TODO notes are fine, but FIXMEs or XXXs should probably just be
+# fixed (in tests, at least).
+notes=FIXME,
+  XXX,
+
+
 [FORMAT]
 
 # Maximum number of characters on a single line.
-- 
2.31.1

[PATCH v2 14/15] python: Add iotest linters to test suite

[John Snow]

Run mypy and pylint on the iotests files directly from the Python CI
test infrastructure. This ensures that any accidental breakages to the
qemu.[qmp|aqmp|machine|utils] packages will be caught by that test
suite.

It also ensures that these linters are run with well-known versions and
test against a wide variety of python versions, which helps to find
accidental cross-version python compatibility issues.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
 python/tests/iotests-mypy.sh   | 4 
 python/tests/iotests-pylint.sh | 4 
 2 files changed, 8 insertions(+)
 create mode 100755 python/tests/iotests-mypy.sh
 create mode 100755 python/tests/iotests-pylint.sh

diff --git a/python/tests/iotests-mypy.sh b/python/tests/iotests-mypy.sh
new file mode 100755
index 000..ee764708199
--- /dev/null
+++ b/python/tests/iotests-mypy.sh
@@ -0,0 +1,4 @@
+#!/bin/sh -e
+
+cd ../tests/qemu-iotests/
+python3 -m linters --mypy
diff --git a/python/tests/iotests-pylint.sh b/python/tests/iotests-pylint.sh
new file mode 100755
index 000..4cae03424b4
--- /dev/null
+++ b/python/tests/iotests-pylint.sh
@@ -0,0 +1,4 @@
+#!/bin/sh -e
+
+cd ../tests/qemu-iotests/
+python3 -m linters --pylint
-- 
2.31.1

[PATCH v2 06/15] iotests/297: Split run_linters apart into run_pylint and run_mypy

[John Snow]

Move environment setup into main(), and split the actual linter
execution into run_pylint and run_mypy, respectively.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
 tests/qemu-iotests/297 | 38 --
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index c1bddb9ce0e..189bcaf5f94 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,7 +21,7 @@ import re
 import shutil
 import subprocess
 import sys
-from typing import List
+from typing import List, Mapping, Optional
 
 import iotests
 
@@ -61,23 +61,19 @@ def get_test_files() -> List[str]:
 return list(filter(is_python_file, check_tests))
 
 
-def run_linters():
-files = get_test_files()
+def run_pylint(
+files: List[str],
+env: Optional[Mapping[str, str]] = None,
+) -> None:
 
-iotests.logger.debug('Files to be checked:')
-iotests.logger.debug(', '.join(sorted(files)))
-
-print('=== pylint ===')
-sys.stdout.flush()
-
-env = os.environ.copy()
 subprocess.run(('python3', '-m', 'pylint', *files),
env=env, check=False)
 
-print('=== mypy ===')
-sys.stdout.flush()
 
-env['MYPYPATH'] = env['PYTHONPATH']
+def run_mypy(
+files: List[str],
+env: Optional[Mapping[str, str]] = None,
+) -> None:
 p = subprocess.run(('python3', '-m', 'mypy', *files),
env=env,
check=False,
@@ -94,7 +90,21 @@ def main() -> None:
 if shutil.which(linter) is None:
 iotests.notrun(f'{linter} not found')
 
-run_linters()
+files = get_test_files()
+
+iotests.logger.debug('Files to be checked:')
+iotests.logger.debug(', '.join(sorted(files)))
+
+env = os.environ.copy()
+env['MYPYPATH'] = env['PYTHONPATH']
+
+print('=== pylint ===')
+sys.stdout.flush()
+run_pylint(files, env=env)
+
+print('=== mypy ===')
+sys.stdout.flush()
+run_mypy(files, env=env)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH v2 07/15] iotests/297: refactor run_[mypy|pylint] as generic execution shim

[John Snow]

There's virtually nothing special here anymore; we can combine these
into a single, rather generic function.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 42 ++
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 189bcaf5f94..d21673a2929 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -61,27 +61,29 @@ def get_test_files() -> List[str]:
 return list(filter(is_python_file, check_tests))
 
 
-def run_pylint(
-files: List[str],
-env: Optional[Mapping[str, str]] = None,
+def run_linter(
+tool: str,
+args: List[str],
+env: Optional[Mapping[str, str]] = None,
+suppress_output: bool = False,
 ) -> None:
+"""
+Run a python-based linting tool.
 
-subprocess.run(('python3', '-m', 'pylint', *files),
-   env=env, check=False)
+If suppress_output is True, capture stdout/stderr of the child
+process and only print that information back to stdout if the child
+process's return code was non-zero.
+"""
+p = subprocess.run(
+('python3', '-m', tool, *args),
+env=env,
+check=False,
+stdout=subprocess.PIPE if suppress_output else None,
+stderr=subprocess.STDOUT if suppress_output else None,
+universal_newlines=True,
+)
 
-
-def run_mypy(
-files: List[str],
-env: Optional[Mapping[str, str]] = None,
-) -> None:
-p = subprocess.run(('python3', '-m', 'mypy', *files),
-   env=env,
-   check=False,
-   stdout=subprocess.PIPE,
-   stderr=subprocess.STDOUT,
-   universal_newlines=True)
-
-if p.returncode != 0:
+if suppress_output and p.returncode != 0:
 print(p.stdout)
 
 
@@ -100,11 +102,11 @@ def main() -> None:
 
 print('=== pylint ===')
 sys.stdout.flush()
-run_pylint(files, env=env)
+run_linter('pylint', files, env=env)
 
 print('=== mypy ===')
 sys.stdout.flush()
-run_mypy(files, env=env)
+run_linter('mypy', files, env=env, suppress_output=True)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH v2 05/15] iotests/297: Don't rely on distro-specific linter binaries

[John Snow]

'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or "python3 -m
mypy" to access these scripts instead. This style of invocation will
prefer the "correct" tool when run in a virtual environment.

Note that we still check for "pylint-3" before the test begins -- this
check is now "overly strict", but shouldn't cause anything that was
already running correctly to start failing. This is addressed by a
commit later in this series;
  'iotests/297: update tool availability checks'.

Signed-off-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Hanna Reitz 
---
 tests/qemu-iotests/297 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 163ebc8ebfd..c1bddb9ce0e 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -71,14 +71,14 @@ def run_linters():
 sys.stdout.flush()
 
 env = os.environ.copy()
-subprocess.run(('pylint-3', *files),
+subprocess.run(('python3', '-m', 'pylint', *files),
env=env, check=False)
 
 print('=== mypy ===')
 sys.stdout.flush()
 
 env['MYPYPATH'] = env['PYTHONPATH']
-p = subprocess.run(('mypy', *files),
+p = subprocess.run(('python3', '-m', 'mypy', *files),
env=env,
check=False,
stdout=subprocess.PIPE,
-- 
2.31.1

[PATCH v2 03/15] iotests/297: Add get_files() function

[John Snow]

Split out file discovery into its own method to begin separating out
configuration/setup and test execution.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
 tests/qemu-iotests/297 | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b8101e6024a..15b54594c11 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,6 +21,7 @@ import re
 import shutil
 import subprocess
 import sys
+from typing import List
 
 import iotests
 
@@ -54,10 +55,14 @@ def is_python_file(filename):
 return False
 
 
-def run_linters():
+def get_test_files() -> List[str]:
 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)]
+return list(filter(is_python_file, check_tests))
+
+
+def run_linters():
+files = get_test_files()
 
 iotests.logger.debug('Files to be checked:')
 iotests.logger.debug(', '.join(sorted(files)))
-- 
2.31.1

[PATCH v2 00/15] python/iotests: Run iotest linters during Python CI

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest-pt2
CI: https://gitlab.com/jsnow/qemu/-/pipelines/388626603

(There's no real rush on my part for this particular series, so review
at-leisure, I'm just getting my edits back out onto the list. The AQMP
series is more important to me in the short-term. I suppose this would
be good to have prior to the RC testing phase, though.)

Factor out pylint and mypy configuration from iotest 297 so that the
Python test infrastructure in python/ can also run these linters. This
will enable what is essentially iotest #297 to run via GitLab CI, via
the 'check-python-pipenv' and 'check-python-tox' tests with new
'iotests-pylint' and 'iotests-mypy' cases.

This series generally aims to split "linter configuration" out of code
so that both iotest #297 and the Python test suite can both invoke the
same linters (nearly) identically.

iotest #297 is left as a valid way to run these tests as a convenience
for block layer developers until I can integrate environment setup and
test execution as a part of 'make check' or similar to serve as a
replacement. This invocation (at present) just trusts you've installed
the right packages into the right places with the right versions, as it
always has. Future patches may begin to build the Python testing
environment as part of 'make check', but there are some details to
hammer out there, first.

v5:

Broadly:

 - Remove int return from linting helpers
 - Tighten tool availability checks
 - Explicitly ensured no regressions on 297 mid-series

In detail:

001/15:[] [--] 'iotests/297: Move pylint config into pylintrc'
002/15:[] [--] 'iotests/297: Split mypy configuration out into mypy.ini'
003/15:[] [--] 'iotests/297: Add get_files() function'
004/15:[] [--] 'iotests/297: Create main() function'
005/15:[0002] [FC] 'iotests/297: Don't rely on distro-specific linter binaries'
006/15:[0023] [FC] 'iotests/297: Split run_linters apart into run_pylint and 
run_mypy'
007/15:[0006] [FC] 'iotests/297: refactor run_[mypy|pylint] as generic 
execution shim'
008/15:[down] 'iotests/297: Change run_linter() to raise an exception on 
failure'
009/15:[down] 'iotests/297: update tool availability checks'
010/15:[down] 'iotests/297: split test into sub-cases'
011/15:[0051] [FC] 'iotests: split linters.py out from 297'
012/15:[0021] [FC] 'iotests/linters: Add entry point for linting via Python CI'
013/15:[0004] [FC] 'iotests/linters: Add workaround for mypy bug #9852'
014/15:[] [--] 'python: Add iotest linters to test suite'
015/15:[0072] [FC] 'iotests: [RFC] drop iotest 297'

- 005: Fixed extra parenthesis. (Oops.)
- 006: Squashed with intermediate patch from v1.
- 007: Removed 'int' return from run_linter()
- 008-010: New.
- 011: Modified comment, otherwise just rebased.
- 012: Rewrote CLI handling to be more thorough.
- 013: Rebase changes only.
- 015: Rebase changes, not that it matters!

v4:

- Some optimizations and touchups were included in 'PATCH v2 0/6]
  iotests: update environment and linting configuration' instead, upon
  which this series is now based.
- Rewrote most patches, being more aggressive about the factoring
  between "iotest" and "linter invocation". The patches are smaller now.
- Almost all configuration is split out into mypy.ini and pylintrc.
- Remove the PWD/CWD juggling that the previous versions added; it's not
  strictly needed for this integration. We can re-add it later if we
  wind up needing it for something.
- mypy and pylintrc tests are split into separate tests. The GitLab CI
  now lists them as two separate test cases, so it's easier to see what
  is failing and why. (And how long those tests take.) It is also now
  therefore possible to ask avocado to run just one or the other.
- mypy bug workaround is only applied strictly in cases where it is
  needed, optimizing speed of iotest 297.

v3:
 - Added patch 1 which has been submitted separately upstream,
   but was necessary for testing.
 - Rebased on top of hreitz/block, which fixed some linting issues.
 - Added a workaround for a rather nasty mypy bug ... >:(

v2:
 - Added patches 1-5 which do some more delinting.
 - Added patch 8, which scans subdirs for tests to lint.
 - Added patch 17, which improves the speed of mypy analysis.
 - Patch 14 is different because of the new patch 8.

John Snow (15):
  iotests/297: Move pylint config into pylintrc
  iotests/297: Split mypy configuration out into mypy.ini
  iotests/297: Add get_files() function
  iotests/297: Create main() function
  iotests/297: Don't rely on distro-specific linter binaries
  iotests/297: Split run_linters apart into run_pylint and run_mypy
  iotests/297: refactor run_[mypy|pylint] as generic execution shim
  iotests/297: Change run_linter() to raise an exception on failure
  iotests/297: update tool availability checks
  iotests/297: split test into sub-cases
  iotests: split linters.py out from 297
  iotests/linters: Add entr

Re: iotest 030 SIGSEGV

[John Snow]

On Thu, Oct 14, 2021 at 9:20 AM Hanna Reitz  wrote:

> On 13.10.21 23:50, John Snow wrote:
> > In trying to replace the QMP library backend, I have now twice
> > stumbled upon a SIGSEGV in iotest 030 in the last three weeks or so.
> >
> > I didn't have debug symbols on at the time, so I've got only this
> > stack trace:
> >
> > (gdb) thread apply all bt
> >
> > Thread 8 (Thread 0x7f0a6b8c4640 (LWP 1873554)):
> > #0  0x7f0a748a53ff in poll () at /lib64/libc.so.6
> > #1  0x7f0a759bfa36 in g_main_context_iterate.constprop () at
> > /lib64/libglib-2.0.so.0
> > #2  0x7f0a7596d163 in g_main_loop_run () at /lib64/libglib-2.0.so.0
> > #3  0x557dac31d121 in iothread_run
> > (opaque=opaque@entry=0x557dadd98800) at ../../iothread.c:73
> > #4  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a6b8c3650) at
> > ../../util/qemu-thread-posix.c:557
> > #5  0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0
> > #6  0x7f0a748b04c3 in clone () at /lib64/libc.so.6
> >
> > Thread 7 (Thread 0x7f0a6b000640 (LWP 1873555)):
> > #0  0x7f0a747ed7d2 in sigtimedwait () at /lib64/libc.so.6
> > #1  0x7f0a74b72cdc in sigwait () at /lib64/libpthread.so.0
> > #2  0x557dac2e403b in dummy_cpu_thread_fn
> > (arg=arg@entry=0x557dae041c10) at ../../accel/dummy-cpus.c:46
> > #3  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a6afff650) at
> > ../../util/qemu-thread-posix.c:557
> > #4  0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0
> > #5  0x7f0a748b04c3 in clone () at /lib64/libc.so.6
> >
> > Thread 6 (Thread 0x7f0a56afa640 (LWP 1873582)):
> > #0  0x7f0a74b71308 in do_futex_wait.constprop () at
> > /lib64/libpthread.so.0
> > #1  0x7f0a74b71433 in __new_sem_wait_slow.constprop.0 () at
> > /lib64/libpthread.so.0
> > #2  0x557dac4d8f1f in qemu_sem_timedwait
> > (sem=sem@entry=0x557dadd62878, ms=ms@entry=1) at
> > ../../util/qemu-thread-posix.c:327
> > #3  0x557dac4f5ac4 in worker_thread
> > (opaque=opaque@entry=0x557dadd62800) at ../../util/thread-pool.c:91
> > #4  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a56af9650) at
> > ../../util/qemu-thread-posix.c:557
> > #5  0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0
> > #6  0x7f0a748b04c3 in clone () at /lib64/libc.so.6
> >
> > Thread 5 (Thread 0x7f0a57dff640 (LWP 1873580)):
> > #0  0x7f0a74b71308 in do_futex_wait.constprop () at
> > /lib64/libpthread.so.0
> > #1  0x7f0a74b71433 in __new_sem_wait_slow.constprop.0 () at
> > /lib64/libpthread.so.0
> > #2  0x557dac4d8f1f in qemu_sem_timedwait
> > (sem=sem@entry=0x557dadd62878, ms=ms@entry=1) at
> > ../../util/qemu-thread-posix.c:327
> > #3  0x557dac4f5ac4 in worker_thread
> > (opaque=opaque@entry=0x557dadd62800) at ../../util/thread-pool.c:91
> > #4  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a57dfe650) at
> > ../../util/qemu-thread-posix.c:557
> > #5  0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0
> > #6  0x7f0a748b04c3 in clone () at /lib64/libc.so.6
> >
> > Thread 4 (Thread 0x7f0a572fb640 (LWP 1873581)):
> > #0  0x7f0a74b7296f in pread64 () at /lib64/libpthread.so.0
> > #1  0x557dac39f18f in pread64 (__offset=,
> > __nbytes=, __buf=, __fd=)
> > at /usr/include/bits/unistd.h:105
> > #2  handle_aiocb_rw_linear (aiocb=aiocb@entry=0x7f0a573fc150,
> > buf=0x7f0a6a47e000 '\377' ...) at
> > ../../block/file-posix.c:1481
> > #3  0x557dac39f664 in handle_aiocb_rw (opaque=0x7f0a573fc150) at
> > ../../block/file-posix.c:1521
> > #4  0x557dac4f5b54 in worker_thread
> > (opaque=opaque@entry=0x557dadd62800) at ../../util/thread-pool.c:104
> > #5  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a572fa650) at
> > ../../util/qemu-thread-posix.c:557
> > #6  0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0
> > #7  0x7f0a748b04c3 in clone () at /lib64/libc.so.6
> >
> > Thread 3 (Thread 0x7f0a714e8640 (LWP 1873552)):
> > #0  0x7f0a748aaedd in syscall () at /lib64/libc.so.6
> > #1  0x557dac4d916a in qemu_futex_wait (val=,
> > f=) at /home/jsnow/src/qemu/include/qemu/futex.h:29
> > #2  qemu_event_wait (ev=ev@entry=0x557dace1f1e8
> > ) at ../../util/qemu-thread-posix.c:480
> > #3  0x557dac4e189a in call_rcu_thread (opaque=opaque@entry=0x0) at
> > ../../util/rcu.c:258
> > #4  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a714e7650) at
> > ../../util/qemu-thread-posix.c:557
> > #5  0x7f0a74b683f9 in start_thread () at /l

[PATCH v4 2/8] python/machine: Handle QMP errors on close more meticulously

[John Snow]

To use the AQMP backend, Machine just needs to be a little more diligent
about what happens when closing a QMP connection. The operation is no
longer a freebie in the async world; it may return errors encountered in
the async bottom half on incoming message receipt, etc.

(AQMP's disconnect, ultimately, serves as the quiescence point where all
async contexts are gathered together, and any final errors reported at
that point.)

Because async QMP continues to check for messages asynchronously, it's
almost certainly likely that the loop will have exited due to EOF after
issuing the last 'quit' command. That error will ultimately be bubbled
up when attempting to close the QMP connection. The manager class here
then is free to discard it -- if it was expected.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
 python/qemu/machine/machine.py | 48 +-
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 0bd40bc2f76..a0cf69786b4 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -342,9 +342,15 @@ def _post_shutdown(self) -> None:
 # Comprehensive reset for the failed launch case:
 self._early_cleanup()
 
-if self._qmp_connection:
-self._qmp.close()
-self._qmp_connection = None
+try:
+self._close_qmp_connection()
+except Exception as err:  # pylint: disable=broad-except
+LOG.warning(
+"Exception closing QMP connection: %s",
+str(err) if str(err) else type(err).__name__
+)
+finally:
+assert self._qmp_connection is None
 
 self._close_qemu_log_file()
 
@@ -420,6 +426,31 @@ def _launch(self) -> None:
close_fds=False)
 self._post_launch()
 
+def _close_qmp_connection(self) -> None:
+"""
+Close the underlying QMP connection, if any.
+
+Dutifully report errors that occurred while closing, but assume
+that any error encountered indicates an abnormal termination
+process and not a failure to close.
+"""
+if self._qmp_connection is None:
+return
+
+try:
+self._qmp.close()
+except EOFError:
+# EOF can occur as an Exception here when using the Async
+# QMP backend. It indicates that the server closed the
+# stream. If we successfully issued 'quit' at any point,
+# then this was expected. If the remote went away without
+# our permission, it's worth reporting that as an abnormal
+# shutdown case.
+if not (self._user_killed or self._quit_issued):
+raise
+finally:
+self._qmp_connection = None
+
 def _early_cleanup(self) -> None:
 """
 Perform any cleanup that needs to happen before the VM exits.
@@ -460,9 +491,14 @@ def _soft_shutdown(self, timeout: Optional[int]) -> None:
 self._early_cleanup()
 
 if self._qmp_connection:
-if not self._quit_issued:
-# Might raise ConnectionReset
-self.qmp('quit')
+try:
+if not self._quit_issued:
+# May raise ExecInterruptedError or StateError if the
+# connection dies or has *already* died.
+self.qmp('quit')
+finally:
+# Regardless, we want to quiesce the connection.
+self._close_qmp_connection()
 
 # May raise subprocess.TimeoutExpired
 self._subp.wait(timeout=timeout)
-- 
2.31.1

[PATCH v4 7/8] python/aqmp: Create sync QMP wrapper for iotests

[John Snow]

This is a wrapper around the async QMPClient that mimics the old,
synchronous QEMUMonitorProtocol class. It is designed to be
interchangeable with the old implementation.

It does not, however, attempt to mimic Exception compatibility.

Signed-off-by: John Snow 
Acked-by: Hanna Reitz 
---
 python/qemu/aqmp/legacy.py | 138 +
 1 file changed, 138 insertions(+)
 create mode 100644 python/qemu/aqmp/legacy.py

diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
new file mode 100644
index 000..9e7b9fb80b9
--- /dev/null
+++ b/python/qemu/aqmp/legacy.py
@@ -0,0 +1,138 @@
+"""
+Sync QMP Wrapper
+
+This class pretends to be qemu.qmp.QEMUMonitorProtocol.
+"""
+
+import asyncio
+from typing import (
+Awaitable,
+List,
+Optional,
+TypeVar,
+Union,
+)
+
+import qemu.qmp
+from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
+
+from .qmp_client import QMPClient
+
+
+# pylint: disable=missing-docstring
+
+
+class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
+def __init__(self, address: SocketAddrT,
+ server: bool = False,
+ nickname: Optional[str] = None):
+
+# pylint: disable=super-init-not-called
+self._aqmp = QMPClient(nickname)
+self._aloop = asyncio.get_event_loop()
+self._address = address
+self._timeout: Optional[float] = None
+
+_T = TypeVar('_T')
+
+def _sync(
+self, future: Awaitable[_T], timeout: Optional[float] = None
+) -> _T:
+return self._aloop.run_until_complete(
+asyncio.wait_for(future, timeout=timeout)
+)
+
+def _get_greeting(self) -> Optional[QMPMessage]:
+if self._aqmp.greeting is not None:
+# pylint: disable=protected-access
+return self._aqmp.greeting._asdict()
+return None
+
+# __enter__ and __exit__ need no changes
+# parse_address needs no changes
+
+def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
+self._aqmp.await_greeting = negotiate
+self._aqmp.negotiate = negotiate
+
+self._sync(
+self._aqmp.connect(self._address)
+)
+return self._get_greeting()
+
+def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
+self._aqmp.await_greeting = True
+self._aqmp.negotiate = True
+
+self._sync(
+self._aqmp.accept(self._address),
+timeout
+)
+
+ret = self._get_greeting()
+assert ret is not None
+return ret
+
+def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
+return dict(
+self._sync(
+# pylint: disable=protected-access
+
+# _raw() isn't a public API, because turning off
+# automatic ID assignment is discouraged. For
+# compatibility with iotests *only*, do it anyway.
+self._aqmp._raw(qmp_cmd, assign_id=False),
+self._timeout
+)
+)
+
+# Default impl of cmd() delegates to cmd_obj
+
+def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+return self._sync(
+self._aqmp.execute(cmd, kwds),
+self._timeout
+)
+
+def pull_event(self,
+   wait: Union[bool, float] = False) -> Optional[QMPMessage]:
+if not wait:
+# wait is False/0: "do not wait, do not except."
+if self._aqmp.events.empty():
+return None
+
+# If wait is 'True', wait forever. If wait is False/0, the events
+# queue must not be empty; but it still needs some real amount
+# of time to complete.
+timeout = None
+if wait and isinstance(wait, float):
+timeout = wait
+
+return dict(
+self._sync(
+self._aqmp.events.get(),
+timeout
+)
+)
+
+def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]:
+events = [dict(x) for x in self._aqmp.events.clear()]
+if events:
+return events
+
+event = self.pull_event(wait)
+return [event] if event is not None else []
+
+def clear_events(self) -> None:
+self._aqmp.events.clear()
+
+def close(self) -> None:
+self._sync(
+self._aqmp.disconnect()
+)
+
+def settimeout(self, timeout: Optional[float]) -> None:
+self._timeout = timeout
+
+def send_fd_scm(self, fd: int) -> None:
+self._aqmp.send_fd_scm(fd)
-- 
2.31.1

[PATCH v4 8/8] python, iotests: replace qmp with aqmp

[John Snow]

Swap out the synchronous QEMUMonitorProtocol from qemu.qmp with the sync
wrapper from qemu.aqmp instead.

Add an escape hatch in the form of the environment variable
QEMU_PYTHON_LEGACY_QMP which allows you to cajole QEMUMachine into using
the old implementation, proving that both implementations work
concurrently.

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

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index a0cf69786b4..a487c397459 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -41,7 +41,6 @@
 )
 
 from qemu.qmp import (  # pylint: disable=import-error
-QEMUMonitorProtocol,
 QMPMessage,
 QMPReturnValue,
 SocketAddrT,
@@ -50,6 +49,12 @@
 from . import console_socket
 
 
+if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
+from qemu.qmp import QEMUMonitorProtocol
+else:
+from qemu.aqmp.legacy import QEMUMonitorProtocol
+
+
 LOG = logging.getLogger(__name__)
 
 
-- 
2.31.1

[PATCH v4 4/8] iotests: Accommodate async QMP Exception classes

[John Snow]

(But continue to support the old ones for now, too.)

There are very few cases of any user of QEMUMachine or a subclass
thereof relying on a QMP Exception type. If you'd like to check for
yourself, you want to grep for all of the derivatives of QMPError,
excluding 'AQMPError' and its derivatives. That'd be these:

- QMPError
- QMPConnectError
- QMPCapabilitiesError
- QMPTimeoutError
- QMPProtocolError
- QMPResponseError
- QMPBadPortError


Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
 scripts/simplebench/bench_block_job.py| 3 ++-
 tests/qemu-iotests/tests/mirror-top-perms | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/scripts/simplebench/bench_block_job.py 
b/scripts/simplebench/bench_block_job.py
index 4f03c121697..a403c35b08f 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -28,6 +28,7 @@
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu.machine import QEMUMachine
 from qemu.qmp import QMPConnectError
+from qemu.aqmp import ConnectError
 
 
 def bench_block_job(cmd, cmd_args, qemu_args):
@@ -49,7 +50,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
 vm.launch()
 except OSError as e:
 return {'error': 'popen failed: ' + str(e)}
-except (QMPConnectError, socket.timeout):
+except (QMPConnectError, ConnectError, socket.timeout):
 return {'error': 'qemu failed: ' + str(vm.get_log())}
 
 try:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms 
b/tests/qemu-iotests/tests/mirror-top-perms
index 3d475aa3a54..a2d5c269d7a 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,8 +21,9 @@
 
 import os
 
-from qemu import qmp
+from qemu.aqmp import ConnectError
 from qemu.machine import machine
+from qemu.qmp import QMPConnectError
 
 import iotests
 from iotests import qemu_img
@@ -102,7 +103,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
 self.vm_b.launch()
 print('ERROR: VM B launched successfully, this should not have '
   'happened')
-except qmp.QMPConnectError:
+except (QMPConnectError, ConnectError):
 assert 'Is another process using the image' in self.vm_b.get_log()
 
 result = self.vm.qmp('block-job-cancel',
-- 
2.31.1

[PATCH v4 3/8] python/aqmp: Remove scary message

[John Snow]

The scary message interferes with the iotests output. Coincidentally, if
iotests works by removing this, then it's good evidence that we don't
really need to scare people away from using it.

Signed-off-by: John Snow 
Acked-by: Hanna Reitz 
---
 python/qemu/aqmp/__init__.py | 12 
 1 file changed, 12 deletions(-)

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index d1b0e4dc3d3..880d5b6fa7f 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,7 +22,6 @@
 # the COPYING file in the top-level directory.
 
 import logging
-import warnings
 
 from .error import AQMPError
 from .events import EventListener
@@ -31,17 +30,6 @@
 from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient
 
 
-_WMSG = """
-
-The Asynchronous QMP library is currently in development and its API
-should be considered highly fluid and subject to change. It should
-not be used by any other scripts checked into the QEMU tree.
-
-Proceed with caution!
-"""
-
-warnings.warn(_WMSG, FutureWarning)
-
 # Suppress logging unless an application engages it.
 logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler())
 
-- 
2.31.1

[PATCH v4 5/8] iotests: Conditionally silence certain AQMP errors

[John Snow]

AQMP likes to be very chatty about errors it encounters. In general,
this is good because it allows us to get good diagnostic information for
otherwise complex async failures.

For example, during a failed QMP connection attempt, we might see:

+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Negotiation failed: EOFError
+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Failed to establish session: EOFError

This might be nice in iotests output, because failure scenarios
involving the new QMP library will be spelled out plainly in the output
diffs.

For tests that are intentionally causing this scenario though, filtering
that log output could be a hassle. For now, add a context manager that
simply lets us toggle this output off during a critical region.

(Additionally, a forthcoming patch allows the use of either legacy or
async QMP to be toggled with an environment variable. In this
circumstance, we can't amend the iotest output to just always expect the
error message, either. Just suppress it for now. More rigorous log
filtering can be investigated later if/when it is deemed safe to
permanently replace the legacy QMP library.)

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
 tests/qemu-iotests/iotests.py | 20 +++-
 tests/qemu-iotests/tests/mirror-top-perms | 12 
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e5fff6ddcfc..e2f9d873ada 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,7 +30,7 @@
 import subprocess
 import sys
 import time
-from typing import (Any, Callable, Dict, Iterable,
+from typing import (Any, Callable, Dict, Iterable, Iterator,
 List, Optional, Sequence, TextIO, Tuple, Type, TypeVar)
 import unittest
 
@@ -114,6 +114,24 @@
 sample_img_dir = os.environ['SAMPLE_IMG_DIR']
 
 
+@contextmanager
+def change_log_level(
+logger_name: str, level: int = logging.CRITICAL) -> Iterator[None]:
+"""
+Utility function for temporarily changing the log level of a logger.
+
+This can be used to silence errors that are expected or uninteresting.
+"""
+_logger = logging.getLogger(logger_name)
+current_level = _logger.level
+_logger.setLevel(level)
+
+try:
+yield
+finally:
+_logger.setLevel(current_level)
+
+
 def unarchive_sample_image(sample, fname):
 sample_fname = os.path.join(sample_img_dir, sample + '.bz2')
 with bz2.open(sample_fname) as f_in, open(fname, 'wb') as f_out:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms 
b/tests/qemu-iotests/tests/mirror-top-perms
index a2d5c269d7a..0a51a613f39 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -26,7 +26,7 @@ from qemu.machine import machine
 from qemu.qmp import QMPConnectError
 
 import iotests
-from iotests import qemu_img
+from iotests import change_log_level, qemu_img
 
 
 image_size = 1 * 1024 * 1024
@@ -100,9 +100,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
 self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
 self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
 try:
-self.vm_b.launch()
-print('ERROR: VM B launched successfully, this should not have '
-  'happened')
+# Silence AQMP errors temporarily.
+# TODO: Remove this and just allow the errors to be logged when
+# AQMP fully replaces QMP.
+with change_log_level('qemu.aqmp'):
+self.vm_b.launch()
+print('ERROR: VM B launched successfully, '
+  'this should not have happened')
 except (QMPConnectError, ConnectError):
 assert 'Is another process using the image' in self.vm_b.get_log()
 
-- 
2.31.1

[PATCH v4 6/8] iotests/300: avoid abnormal shutdown race condition

[John Snow]

Wait for the destination VM to close itself instead of racing to shut it
down first, which produces different error log messages from AQMP
depending on precisely when we tried to shut it down.

(For example: We may try to issue 'quit' immediately prior to the target
VM closing its QMP socket, which will cause an ECONNRESET error to be
logged. Waiting for the VM to exit itself avoids the race on shutdown
behavior.)

Reported-by: Hanna Reitz 
Signed-off-by: John Snow 
---
 tests/qemu-iotests/300 | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 10f9f2a8da6..bbea7248005 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -24,8 +24,6 @@ import random
 import re
 from typing import Dict, List, Optional
 
-from qemu.machine import machine
-
 import iotests
 
 
@@ -461,12 +459,10 @@ class 
TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
   f"'{self.src_node_name}': Name is longer than 255 bytes",
   log)
 
-# Expect abnormal shutdown of the destination VM because of
-# the failed migration
-try:
-self.vm_b.shutdown()
-except machine.AbnormalShutdown:
-pass
+# Destination VM will terminate w/ error of its own accord
+# due to the failed migration.
+self.vm_b.wait()
+assert self.vm_b.exitcode() > 0
 
 def test_aliased_bitmap_name_too_long(self) -> None:
 # Longer than the maximum for bitmap names
-- 
2.31.1

[PATCH v4 0/8] Switch iotests to using Async QMP

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-iotest-wrapper
CI: https://gitlab.com/jsnow/qemu/-/pipelines/387972757

Hiya,

This series continues where the last two AQMP series left off and adds a
synchronous 'legacy' wrapper around the new AQMP interface, then drops
it straight into iotests to prove that AQMP is functional and totally
cool and fine. The disruption and churn to iotests is pretty minimal.

In the event that a regression happens and I am not physically proximate
to inflict damage upon, one may set the QEMU_PYTHON_LEGACY_QMP variable
to any non-empty string as it pleases you to engage the QMP machinery
you are used to.

I'd like to try and get this committed early in the 6.2 development
cycle to give ample time to smooth over any possible regressions. I've
tested it locally and via gitlab CI, across Python versions 3.6 through
3.10, and "worksforme". If something bad happens, we can revert the
actual switch-flip very trivially.

V4:

001/8:[] [--] 'python/machine: remove has_quit argument'
002/8:[] [--] 'python/machine: Handle QMP errors on close more meticulously'
003/8:[] [--] 'python/aqmp: Remove scary message'
004/8:[] [--] 'iotests: Accommodate async QMP Exception classes'
005/8:[] [--] 'iotests: Conditionally silence certain AQMP errors'
006/8:[down] 'iotests/300: avoid abnormal shutdown race condition'
007/8:[] [--] 'python/aqmp: Create sync QMP wrapper for iotests'
008/8:[] [--] 'python, iotests: replace qmp with aqmp'

006: Added to address a race condition in iotest 300.
 (Thanks for the repro, Hanna)

V3:

001/7:[] [--] 'python/machine: remove has_quit argument'
002/7:[0002] [FC] 'python/machine: Handle QMP errors on close more meticulously'
003/7:[] [--] 'python/aqmp: Remove scary message'
004/7:[0006] [FC] 'iotests: Accommodate async QMP Exception classes'
005/7:[0003] [FC] 'iotests: Conditionally silence certain AQMP errors'
006/7:[0009] [FC] 'python/aqmp: Create sync QMP wrapper for iotests'
007/7:[] [--] 'python, iotests: replace qmp with aqmp'

002: Account for force-kill cases, too.
003: Shuffled earlier into the series to prevent a mid-series regression.
004: Rewrite the imports to be less "heterogeneous" ;)
005: Add in a TODO for me to trip over in the future.
006: Fix a bug surfaced by a new iotest where waiting with pull_event for a
 timeout of 0.0 will cause a timeout exception to be raised even if there
 was an event ready to be read.

V2: A distant dream, half-remembered.
V1: Apocrypha.

John Snow (8):
  python/machine: remove has_quit argument
  python/machine: Handle QMP errors on close more meticulously
  python/aqmp: Remove scary message
  iotests: Accommodate async QMP Exception classes
  iotests: Conditionally silence certain AQMP errors
  iotests/300: avoid abnormal shutdown race condition
  python/aqmp: Create sync QMP wrapper for iotests
  python, iotests: replace qmp with aqmp

 python/qemu/aqmp/__init__.py  |  12 --
 python/qemu/aqmp/legacy.py| 138 ++
 python/qemu/machine/machine.py|  85 +
 scripts/simplebench/bench_block_job.py|   3 +-
 tests/qemu-iotests/040|   7 +-
 tests/qemu-iotests/218|   2 +-
 tests/qemu-iotests/255|   2 +-
 tests/qemu-iotests/300|  12 +-
 tests/qemu-iotests/iotests.py |  20 +++-
 tests/qemu-iotests/tests/mirror-top-perms |  17 ++-
 10 files changed, 242 insertions(+), 56 deletions(-)
 create mode 100644 python/qemu/aqmp/legacy.py

-- 
2.31.1

[PATCH v4 1/8] python/machine: remove has_quit argument

[John Snow]

If we spy on the QMP commands instead, we don't need callers to remember
to pass it. Seems like a fair trade-off.

The one slightly weird bit is overloading this instance variable for
wait(), where we use it to mean "don't issue the qmp 'quit'
command". This means that wait() will "fail" if the QEMU process does
not terminate of its own accord.

In most cases, we probably did already actually issue quit -- some
iotests do this -- but in some others, we may be waiting for QEMU to
terminate for some other reason, such as a test wherein we tell the
guest (directly) to shut down.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
 python/qemu/machine/machine.py | 34 +++---
 tests/qemu-iotests/040 |  7 +--
 tests/qemu-iotests/218 |  2 +-
 tests/qemu-iotests/255 |  2 +-
 4 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 056d340e355..0bd40bc2f76 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -170,6 +170,7 @@ def __init__(self,
 self._console_socket: Optional[socket.socket] = None
 self._remove_files: List[str] = []
 self._user_killed = False
+self._quit_issued = False
 
 def __enter__(self: _T) -> _T:
 return self
@@ -368,6 +369,7 @@ def _post_shutdown(self) -> None:
 command = ''
 LOG.warning(msg, -int(exitcode), command)
 
+self._quit_issued = False
 self._user_killed = False
 self._launched = False
 
@@ -443,15 +445,13 @@ def _hard_shutdown(self) -> None:
 self._subp.kill()
 self._subp.wait(timeout=60)
 
-def _soft_shutdown(self, timeout: Optional[int],
-   has_quit: bool = False) -> None:
+def _soft_shutdown(self, timeout: Optional[int]) -> None:
 """
 Perform early cleanup, attempt to gracefully shut down the VM, and wait
 for it to terminate.
 
 :param timeout: Timeout in seconds for graceful shutdown.
 A value of None is an infinite wait.
-:param has_quit: When True, don't attempt to issue 'quit' QMP command
 
 :raise ConnectionReset: On QMP communication errors
 :raise subprocess.TimeoutExpired: When timeout is exceeded waiting for
@@ -460,21 +460,19 @@ def _soft_shutdown(self, timeout: Optional[int],
 self._early_cleanup()
 
 if self._qmp_connection:
-if not has_quit:
+if not self._quit_issued:
 # Might raise ConnectionReset
-self._qmp.cmd('quit')
+self.qmp('quit')
 
 # May raise subprocess.TimeoutExpired
 self._subp.wait(timeout=timeout)
 
-def _do_shutdown(self, timeout: Optional[int],
- has_quit: bool = False) -> None:
+def _do_shutdown(self, timeout: Optional[int]) -> None:
 """
 Attempt to shutdown the VM gracefully; fallback to a hard shutdown.
 
 :param timeout: Timeout in seconds for graceful shutdown.
 A value of None is an infinite wait.
-:param has_quit: When True, don't attempt to issue 'quit' QMP command
 
 :raise AbnormalShutdown: When the VM could not be shut down gracefully.
 The inner exception will likely be ConnectionReset or
@@ -482,13 +480,13 @@ def _do_shutdown(self, timeout: Optional[int],
 may result in its own exceptions, likely subprocess.TimeoutExpired.
 """
 try:
-self._soft_shutdown(timeout, has_quit)
+self._soft_shutdown(timeout)
 except Exception as exc:
 self._hard_shutdown()
 raise AbnormalShutdown("Could not perform graceful shutdown") \
 from exc
 
-def shutdown(self, has_quit: bool = False,
+def shutdown(self,
  hard: bool = False,
  timeout: Optional[int] = 30) -> None:
 """
@@ -498,7 +496,6 @@ def shutdown(self, has_quit: bool = False,
 If the VM has not yet been launched, or shutdown(), wait(), or kill()
 have already been called, this method does nothing.
 
-:param has_quit: When true, do not attempt to issue 'quit' QMP command.
 :param hard: When true, do not attempt graceful shutdown, and
  suppress the SIGKILL warning log message.
 :param timeout: Optional timeout in seconds for graceful shutdown.
@@ -512,7 +509,7 @@ def shutdown(self, has_quit: bool = False,
 self._user_killed = True
 self._hard_shutdown()
 else:
-self._do_shutdown(timeout, has_quit)
+self._do_shutdown(timeout)
 finally:
 self._post_shutdown()
 
@@ -529,7 +526,8 @@ def wait(se

iotest 030 SIGSEGV

[John Snow]

In trying to replace the QMP library backend, I have now twice stumbled
upon a SIGSEGV in iotest 030 in the last three weeks or so.

I didn't have debug symbols on at the time, so I've got only this stack
trace:

(gdb) thread apply all bt

Thread 8 (Thread 0x7f0a6b8c4640 (LWP 1873554)):
#0  0x7f0a748a53ff in poll () at /lib64/libc.so.6
#1  0x7f0a759bfa36 in g_main_context_iterate.constprop () at
/lib64/libglib-2.0.so.0
#2  0x7f0a7596d163 in g_main_loop_run () at /lib64/libglib-2.0.so.0
#3  0x557dac31d121 in iothread_run (opaque=opaque@entry=0x557dadd98800)
at ../../iothread.c:73
#4  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a6b8c3650) at
../../util/qemu-thread-posix.c:557
#5  0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0
#6  0x7f0a748b04c3 in clone () at /lib64/libc.so.6

Thread 7 (Thread 0x7f0a6b000640 (LWP 1873555)):
#0  0x7f0a747ed7d2 in sigtimedwait () at /lib64/libc.so.6
#1  0x7f0a74b72cdc in sigwait () at /lib64/libpthread.so.0
#2  0x557dac2e403b in dummy_cpu_thread_fn (arg=arg@entry=0x557dae041c10)
at ../../accel/dummy-cpus.c:46
#3  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a6afff650) at
../../util/qemu-thread-posix.c:557
#4  0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0
#5  0x7f0a748b04c3 in clone () at /lib64/libc.so.6

Thread 6 (Thread 0x7f0a56afa640 (LWP 1873582)):
#0  0x7f0a74b71308 in do_futex_wait.constprop () at
/lib64/libpthread.so.0
#1  0x7f0a74b71433 in __new_sem_wait_slow.constprop.0 () at
/lib64/libpthread.so.0
#2  0x557dac4d8f1f in qemu_sem_timedwait (sem=sem@entry=0x557dadd62878,
ms=ms@entry=1) at ../../util/qemu-thread-posix.c:327
#3  0x557dac4f5ac4 in worker_thread (opaque=opaque@entry=0x557dadd62800)
at ../../util/thread-pool.c:91
#4  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a56af9650) at
../../util/qemu-thread-posix.c:557
#5  0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0
#6  0x7f0a748b04c3 in clone () at /lib64/libc.so.6

Thread 5 (Thread 0x7f0a57dff640 (LWP 1873580)):
#0  0x7f0a74b71308 in do_futex_wait.constprop () at
/lib64/libpthread.so.0
#1  0x7f0a74b71433 in __new_sem_wait_slow.constprop.0 () at
/lib64/libpthread.so.0
#2  0x557dac4d8f1f in qemu_sem_timedwait (sem=sem@entry=0x557dadd62878,
ms=ms@entry=1) at ../../util/qemu-thread-posix.c:327
#3  0x557dac4f5ac4 in worker_thread (opaque=opaque@entry=0x557dadd62800)
at ../../util/thread-pool.c:91
#4  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a57dfe650) at
../../util/qemu-thread-posix.c:557
#5  0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0
#6  0x7f0a748b04c3 in clone () at /lib64/libc.so.6

Thread 4 (Thread 0x7f0a572fb640 (LWP 1873581)):
#0  0x7f0a74b7296f in pread64 () at /lib64/libpthread.so.0
#1  0x557dac39f18f in pread64 (__offset=,
__nbytes=, __buf=, __fd=) at
/usr/include/bits/unistd.h:105
#2  handle_aiocb_rw_linear (aiocb=aiocb@entry=0x7f0a573fc150,
buf=0x7f0a6a47e000 '\377' ...) at
../../block/file-posix.c:1481
#3  0x557dac39f664 in handle_aiocb_rw (opaque=0x7f0a573fc150) at
../../block/file-posix.c:1521
#4  0x557dac4f5b54 in worker_thread (opaque=opaque@entry=0x557dadd62800)
at ../../util/thread-pool.c:104
#5  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a572fa650) at
../../util/qemu-thread-posix.c:557
#6  0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0
#7  0x7f0a748b04c3 in clone () at /lib64/libc.so.6

Thread 3 (Thread 0x7f0a714e8640 (LWP 1873552)):
#0  0x7f0a748aaedd in syscall () at /lib64/libc.so.6
#1  0x557dac4d916a in qemu_futex_wait (val=,
f=) at /home/jsnow/src/qemu/include/qemu/futex.h:29
#2  qemu_event_wait (ev=ev@entry=0x557dace1f1e8 ) at
../../util/qemu-thread-posix.c:480
#3  0x557dac4e189a in call_rcu_thread (opaque=opaque@entry=0x0) at
../../util/rcu.c:258
#4  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a714e7650) at
../../util/qemu-thread-posix.c:557
#5  0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0
#6  0x7f0a748b04c3 in clone () at /lib64/libc.so.6

Thread 2 (Thread 0x7f0a70ae5640 (LWP 1873553)):
#0  0x7f0a74b71308 in do_futex_wait.constprop () at
/lib64/libpthread.so.0
#1  0x7f0a74b71433 in __new_sem_wait_slow.constprop.0 () at
/lib64/libpthread.so.0
#2  0x557dac4d8f1f in qemu_sem_timedwait (sem=sem@entry=0x557dadd62878,
ms=ms@entry=1) at ../../util/qemu-thread-posix.c:327
#3  0x557dac4f5ac4 in worker_thread (opaque=opaque@entry=0x557dadd62800)
at ../../util/thread-pool.c:91
#4  0x557dac4d7f89 in qemu_thread_start (args=0x7f0a70ae4650) at
../../util/qemu-thread-posix.c:557
#5  0x7f0a74b683f9 in start_thread () at /lib64/libpthread.so.0
#6  0x7f0a748b04c3 in clone () at /lib64/libc.so.6

Thread 1 (Thread 0x7f0a714ebec0 (LWP 1873551)):
#0  bdrv_inherits_from_recursive (parent=parent@entry=0x557dadfb5050,
child=0xafafafafafafafaf, child@entry=0x557dae857010) at ../../block.c:3124
#1  

Re: [PATCH v3 0/7] Switch iotests to using Async QMP

[John Snow]

On Wed, Oct 13, 2021 at 10:49 AM Hanna Reitz  wrote:

> On 13.10.21 16:00, John Snow wrote:
> >
> >
> > On Wed, Oct 13, 2021 at 8:51 AM John Snow  wrote:
> >
> >
> >
> > On Wed, Oct 13, 2021 at 4:45 AM Hanna Reitz 
> wrote:
> >
> > On 13.10.21 00:34, John Snow wrote:
> > > Based-on: <20211012214152.802483-1-js...@redhat.com>
> > >[PULL 00/10] Python patches
> > > GitLab:
> >
> https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-iotest-wrapper
> > > CI: https://gitlab.com/jsnow/qemu/-/pipelines/387210591
> > >
> > > Hiya,
> > >
> > > This series continues where the last two AQMP series left
> > off and adds a
> > > synchronous 'legacy' wrapper around the new AQMP interface,
> > then drops
> > > it straight into iotests to prove that AQMP is functional
> > and totally
> > > cool and fine. The disruption and churn to iotests is pretty
> > minimal.
> > >
> > > In the event that a regression happens and I am not
> > physically proximate
> > > to inflict damage upon, one may set the
> > QEMU_PYTHON_LEGACY_QMP variable
> > > to any non-empty string as it pleases you to engage the QMP
> > machinery
> > > you are used to.
> > >
> > > I'd like to try and get this committed early in the 6.2
> > development
> > > cycle to give ample time to smooth over any possible
> > regressions. I've
> > > tested it locally and via gitlab CI, across Python versions
> > 3.6 through
> > > 3.10, and "worksforme". If something bad happens, we can
> > revert the
> > > actual switch-flip very trivially.
> >
> > So running iotests locally, I got one failure:
> >
> > $ TEST_DIR=/tmp/vdi-tests ./check -c writethrough -vdi 300
> > [...]
> > 300 fail   [10:28:06] [10:28:11]
> > 5.1s output mismatch (see 300.out.bad)
> > --- /home/maxx/projects/qemu/tests/qemu-iotests/300.out
> > +++ 300.out.bad
> > @@ -1,4 +1,5 @@
> > -...
> >
>  +..ERROR:qemu.aqmp.qmp_client.qemu-b-222963:Task.Reader:
> >
> > ConnectionResetError: [Errno 104] Connection reset by peer
> > +.
> >
>--
> >   Ran 39 tests
> > [...]
> >
> >
> > Oh, unfortunate.
> >
> >
> > I’m afraid I can’t really give a reproducer or anything.  It
> > feels like
> >
> >
> > Thank you for the report!
> >
> > just some random spurious timing-related error. Although then
> > again,
> > 300 does have an `except machine.AbnormalShutdown` clause at one
> > point...  So perhaps that’s the culprit, and we need to
> > disable logging
> > there.
> >
> >
> > I'll investigate!
> >
> >
> > Unfortunately, even in a loop some 150 times I couldn't reproduce this
> > one. As you point out, it appears to be just a failure caused by
> > logging. The test logic itself completes as expected.
> >
> > Still, I would expect, on a "clean" shutdown of the destination host
> > (where the destination process fails to load the migration stream and
> > voluntarily exits with an error code) to end with a FIN/ACK for TCP or
> > ... uh, whatever happens for a UNIX socket. Where's the Connection
> > Reset coming from? Did the destination VM process *crash*?
> >
> > I'm not so sure that I *should* silence this error, but I also can't
> > reproduce it at all to answer these questions, so uh. uhhh. I guess I
> > will just hammer it on a loop a few hundred times more and see if I
> > get lucky.
>
> I could reproduce it, by running 20 instances concurrently.  (Needs a
> change to testrunner.py, so that the reference outputs don’t collide:
>
> diff --git a/tests/qemu-iotests/testrunner.py
> b/tests/qemu-iotests/testrunner.py
> index a56b6da396..fd0a3a1eeb 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -221,7 +221,7 @@ def find_refe

Re: [PATCH 10/13] iotests/linters: Add entry point for linting via Python CI

[John Snow]

On Wed, Oct 13, 2021 at 8:11 AM Hanna Reitz  wrote:

> On 04.10.21 23:05, John Snow wrote:
> > We need at least a tiny little shim here to join test file discovery
> > with test invocation. This logic could conceivably be hosted somewhere
> > in python/, but I felt it was strictly the least-rude thing to keep the
> > test logic here in iotests/, even if this small function isn't itself an
> > iotest.
> >
> > Note that we don't actually even need the executable bit here, we'll be
> > relying on the ability to run this module as a script using Python CLI
> > arguments. No chance it gets misunderstood as an actual iotest that way.
> >
> > (It's named, not in tests/, doesn't have the execute bit, and doesn't
> > have an execution shebang.)
> >
> > Signed-off-by: John Snow 
> >
> > ---
> >
> > (1) I think that the test file discovery logic and skip list belong
> together,
> >  and that those items belong in iotests/. I think they also belong in
> >  whichever directory pylintrc and mypy.ini are in, also in iotests/.
>
> Agreed.
>
> > (2) Moving this logic into python/tests/ is challenging because I'd have
> >  to import iotests code from elsewhere in the source tree, which just
> >  inverts an existing problem I have been trying to rid us of --
> >  needing to muck around with PYTHONPATH or sys.path hacking in python
> >  scripts. I'm keen to avoid this.
>
> OK.
>
> > (3) If we moved all python tests into tests/ and gave them *.py
> >  extensions, we wouldn't even need the test discovery functions
> >  anymore, and all of linters.py could be removed entirely, including
> >  this execution shim. We could rely on mypy/pylint's own file
> >  discovery mechanisms at that point. More work than I'm up for with
> >  just this series, but I could be coaxed into doing it if there was
> >  some promise of not rejecting all that busywork ;)
>
> I believe the only real value this would gain is that the tests would
> have nicer names and we would have to delint them.  If we find those
> goals to justify the effort, then we can put in the effort; and we can
> do that slowly, test by test.  I don’t think we must do it now in one
> big lump just to get rid of the file discovery functions.
>
>
Yeah, I agree -- just do it over time and as-needed. I'm sure I will be
bothered by something-or-other sooner-or-later and I'll wind up doing it
anyway. Just maybe not this week!


> > Signed-off-by: John Snow 
> > ---
> >   tests/qemu-iotests/linters.py | 18 ++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/linters.py
> b/tests/qemu-iotests/linters.py
> > index f6a2dc139fd..191df173064 100644
> > --- a/tests/qemu-iotests/linters.py
> > +++ b/tests/qemu-iotests/linters.py
> > @@ -16,6 +16,7 @@
> >   import os
> >   import re
> >   import subprocess
> > +import sys
> >   from typing import List, Mapping, Optional
> >
> >
> > @@ -81,3 +82,20 @@ def run_linter(
> >
> >   return p.returncode
> >
> > +
> > +def main() -> int:
> > +"""
> > +Used by the Python CI system as an entry point to run these linters.
> > +"""
> > +files = get_test_files()
> > +
> > +if sys.argv[1] == '--pylint':
> > +return run_linter('pylint', files)
> > +elif sys.argv[1] == '--mypy':
> > +return run_linter('mypy', files)
>
> So I can run it as `python linters.py --pylint foo bar` and it won’t
> complain? :)
>
> I don’t feel like it’s important, but, well, it isn’t right either.
>
>
Alright. I hacked it together to be "minimal" in terms of SLOC, but I can
make it ... less minimal.

Re: [PATCH 09/13] iotests: split linters.py out from 297

[John Snow]

On Wed, Oct 13, 2021 at 7:50 AM Hanna Reitz  wrote:

> On 04.10.21 23:04, John Snow wrote:
> > Now, 297 is just the iotests-specific incantations and linters.py is as
> > minimal as I can think to make it. The only remaining element in here
> > that ought to be configuration and not code is the list of skip files,
>
> Yeah...
>
> > but they're still numerous enough that repeating them for mypy and
> > pylint configurations both would be ... a hassle.
>
> I agree.
>
> > Signed-off-by: John Snow 
> > ---
> >   tests/qemu-iotests/297| 72 +++---
> >   tests/qemu-iotests/linters.py | 83 +++
> >   2 files changed, 88 insertions(+), 67 deletions(-)
> >   create mode 100644 tests/qemu-iotests/linters.py
>
> I’d like to give an A-b because now the statuscode-returning function is
> in a library.  But I already gave an A-b on the last patch precisely
> because of the interface, and I shouldn’t be so grumpy.
>
> Reviewed-by: Hanna Reitz 
>
>
I'm not entirely sure I understand your dislike(?) of status codes. I'm not
trying to ignore the feedback, but I don't think I understand it fully.

Would it be better if I removed check=False and allowed the functions to
raise an Exception on non-zero subprocess return? (Possibly having the
function itself print the stdout on the error case before re-raising.)

--js

Re: [PATCH 05/13] iotests/297: Create main() function

[John Snow]

On Wed, Oct 13, 2021 at 7:03 AM Hanna Reitz  wrote:

> On 04.10.21 23:04, John Snow wrote:
> > Instead of running "run_linters" directly, create a main() function that
> > will be responsible for environment setup, leaving run_linters()
> > responsible only for execution of the linters.
> >
> > (That environment setup will be moved over in forthcoming commits.)
> >
> > Signed-off-by: John Snow 
> > Reviewed-by: Vladimir Sementsov-Ogievskiy 
> > Reviewed-by: Philippe Mathieu-Daudé 
> > Reviewed-by: Hanna Reitz 
> > ---
> >   tests/qemu-iotests/297 | 12 
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index 65b1e7058c2..f9fcb039e27 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -89,8 +89,12 @@ def run_linters():
> >   print(p.stdout)
> >
> >
> > -for linter in ('pylint-3', 'mypy'):
> > -if shutil.which(linter) is None:
> > -iotests.notrun(f'{linter} not found')
> > +def main() -> None:
> > +for linter in ('pylint-3', 'mypy'):
> > +if shutil.which(linter) is None:
> > +iotests.notrun(f'{linter} not found')
>
> Now that I see it here: Given patch 4, shouldn’t we replace
> `shutil.which()` by some other check?
>
>
Yeah, ideally. Sorry, I was lazy and didn't do that part yet. Nobody had
asked O:-)

I'll bolster the previous patch for the next go-around. (Or maybe I'll send
a fixup patch to the list, depending on what the rest of your replies look
like.)

--js

Re: [PATCH 02/13] iotests/297: Split mypy configuration out into mypy.ini

[John Snow]

On Wed, Oct 13, 2021 at 6:53 AM Hanna Reitz  wrote:

> On 04.10.21 23:04, John Snow wrote:
> > More separation of code and configuration.
> >
> > Signed-off-by: John Snow 
> > ---
> >   tests/qemu-iotests/297  | 14 +-
> >   tests/qemu-iotests/mypy.ini | 12 
> >   2 files changed, 13 insertions(+), 13 deletions(-)
> >   create mode 100644 tests/qemu-iotests/mypy.ini
>
> Reviewed-by: Hanna Reitz 
>
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index bc3a0ceb2aa..b8101e6024a 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -73,19 +73,7 @@ def run_linters():
> >   sys.stdout.flush()
> >
> >   env['MYPYPATH'] = env['PYTHONPATH']
> > -p = subprocess.run(('mypy',
> > -'--warn-unused-configs',
> > -'--disallow-subclassing-any',
> > -'--disallow-any-generics',
> > -'--disallow-incomplete-defs',
> > -'--disallow-untyped-decorators',
> > -'--no-implicit-optional',
> > -'--warn-redundant-casts',
> > -'--warn-unused-ignores',
> > -'--no-implicit-reexport',
> > -'--namespace-packages',
> > -'--scripts-are-modules',
> > -*files),
> > +p = subprocess.run(('mypy', *files),
> >  env=env,
> >  check=False,
> >  stdout=subprocess.PIPE,
> > diff --git a/tests/qemu-iotests/mypy.ini b/tests/qemu-iotests/mypy.ini
> > new file mode 100644
> > index 000..4c0339f5589
> > --- /dev/null
> > +++ b/tests/qemu-iotests/mypy.ini
> > @@ -0,0 +1,12 @@
> > +[mypy]
> > +disallow_any_generics = True
> > +disallow_incomplete_defs = True
> > +disallow_subclassing_any = True
> > +disallow_untyped_decorators = True
> > +implicit_reexport = False
>
> Out of curiosity: Any reason you chose to invert this one, but none of
> the rest?  (i.e. no_implicit_optional = True -> implicit_optional =
> False; or disallow* = True -> allow* = False)
>
>
Anything written as '--no-xxx' I wrote as 'xxx = False', but
"implicit_optional = False" isn't a supported option, so that one kept the
"no" in it.

--js

Re: [PATCH v3 0/7] Switch iotests to using Async QMP

[John Snow]

On Wed, Oct 13, 2021 at 8:51 AM John Snow  wrote:

>
>
> On Wed, Oct 13, 2021 at 4:45 AM Hanna Reitz  wrote:
>
>> On 13.10.21 00:34, John Snow wrote:
>> > Based-on: <20211012214152.802483-1-js...@redhat.com>
>> >[PULL 00/10] Python patches
>> > GitLab:
>> https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-iotest-wrapper
>> > CI: https://gitlab.com/jsnow/qemu/-/pipelines/387210591
>> >
>> > Hiya,
>> >
>> > This series continues where the last two AQMP series left off and adds a
>> > synchronous 'legacy' wrapper around the new AQMP interface, then drops
>> > it straight into iotests to prove that AQMP is functional and totally
>> > cool and fine. The disruption and churn to iotests is pretty minimal.
>> >
>> > In the event that a regression happens and I am not physically proximate
>> > to inflict damage upon, one may set the QEMU_PYTHON_LEGACY_QMP variable
>> > to any non-empty string as it pleases you to engage the QMP machinery
>> > you are used to.
>> >
>> > I'd like to try and get this committed early in the 6.2 development
>> > cycle to give ample time to smooth over any possible regressions. I've
>> > tested it locally and via gitlab CI, across Python versions 3.6 through
>> > 3.10, and "worksforme". If something bad happens, we can revert the
>> > actual switch-flip very trivially.
>>
>> So running iotests locally, I got one failure:
>>
>> $ TEST_DIR=/tmp/vdi-tests ./check -c writethrough -vdi 300
>> [...]
>> 300 fail   [10:28:06] [10:28:11]
>> 5.1s output mismatch (see 300.out.bad)
>> --- /home/maxx/projects/qemu/tests/qemu-iotests/300.out
>> +++ 300.out.bad
>> @@ -1,4 +1,5 @@
>> -...
>> +..ERROR:qemu.aqmp.qmp_client.qemu-b-222963:Task.Reader:
>> ConnectionResetError: [Errno 104] Connection reset by peer
>> +.
>>   --
>>   Ran 39 tests
>> [...]
>>
>>
> Oh, unfortunate.
>
>
>>
>> I’m afraid I can’t really give a reproducer or anything.  It feels like
>>
>
> Thank you for the report!
>
>
>> just some random spurious timing-related error.  Although then again,
>> 300 does have an `except machine.AbnormalShutdown` clause at one
>> point...  So perhaps that’s the culprit, and we need to disable logging
>> there.
>>
>>
> I'll investigate!
>

Unfortunately, even in a loop some 150 times I couldn't reproduce this one.
As you point out, it appears to be just a failure caused by logging. The
test logic itself completes as expected.

Still, I would expect, on a "clean" shutdown of the destination host (where
the destination process fails to load the migration stream and voluntarily
exits with an error code) to end with a FIN/ACK for TCP or ... uh, whatever
happens for a UNIX socket. Where's the Connection Reset coming from? Did
the destination VM process *crash*?

I'm not so sure that I *should* silence this error, but I also can't
reproduce it at all to answer these questions, so uh. uhhh. I guess I will
just hammer it on a loop a few hundred times more and see if I get lucky.

Re: [PATCH v3 0/7] Switch iotests to using Async QMP

[John Snow]

On Wed, Oct 13, 2021 at 4:45 AM Hanna Reitz  wrote:

> On 13.10.21 00:34, John Snow wrote:
> > Based-on: <20211012214152.802483-1-js...@redhat.com>
> >[PULL 00/10] Python patches
> > GitLab:
> https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-iotest-wrapper
> > CI: https://gitlab.com/jsnow/qemu/-/pipelines/387210591
> >
> > Hiya,
> >
> > This series continues where the last two AQMP series left off and adds a
> > synchronous 'legacy' wrapper around the new AQMP interface, then drops
> > it straight into iotests to prove that AQMP is functional and totally
> > cool and fine. The disruption and churn to iotests is pretty minimal.
> >
> > In the event that a regression happens and I am not physically proximate
> > to inflict damage upon, one may set the QEMU_PYTHON_LEGACY_QMP variable
> > to any non-empty string as it pleases you to engage the QMP machinery
> > you are used to.
> >
> > I'd like to try and get this committed early in the 6.2 development
> > cycle to give ample time to smooth over any possible regressions. I've
> > tested it locally and via gitlab CI, across Python versions 3.6 through
> > 3.10, and "worksforme". If something bad happens, we can revert the
> > actual switch-flip very trivially.
>
> So running iotests locally, I got one failure:
>
> $ TEST_DIR=/tmp/vdi-tests ./check -c writethrough -vdi 300
> [...]
> 300 fail   [10:28:06] [10:28:11]
> 5.1s output mismatch (see 300.out.bad)
> --- /home/maxx/projects/qemu/tests/qemu-iotests/300.out
> +++ 300.out.bad
> @@ -1,4 +1,5 @@
> -...
> +..ERROR:qemu.aqmp.qmp_client.qemu-b-222963:Task.Reader:
> ConnectionResetError: [Errno 104] Connection reset by peer
> +.
>   --
>   Ran 39 tests
> [...]
>
>
Oh, unfortunate.


>
> I’m afraid I can’t really give a reproducer or anything.  It feels like
>

Thank you for the report!


> just some random spurious timing-related error.  Although then again,
> 300 does have an `except machine.AbnormalShutdown` clause at one
> point...  So perhaps that’s the culprit, and we need to disable logging
> there.
>
>
I'll investigate!

[PATCH v3 2/7] python/machine: Handle QMP errors on close more meticulously

[John Snow]

To use the AQMP backend, Machine just needs to be a little more diligent
about what happens when closing a QMP connection. The operation is no
longer a freebie in the async world; it may return errors encountered in
the async bottom half on incoming message receipt, etc.

(AQMP's disconnect, ultimately, serves as the quiescence point where all
async contexts are gathered together, and any final errors reported at
that point.)

Because async QMP continues to check for messages asynchronously, it's
almost certainly likely that the loop will have exited due to EOF after
issuing the last 'quit' command. That error will ultimately be bubbled
up when attempting to close the QMP connection. The manager class here
then is free to discard it -- if it was expected.

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

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 0bd40bc2f76..a0cf69786b4 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -342,9 +342,15 @@ def _post_shutdown(self) -> None:
 # Comprehensive reset for the failed launch case:
 self._early_cleanup()
 
-if self._qmp_connection:
-self._qmp.close()
-self._qmp_connection = None
+try:
+self._close_qmp_connection()
+except Exception as err:  # pylint: disable=broad-except
+LOG.warning(
+"Exception closing QMP connection: %s",
+str(err) if str(err) else type(err).__name__
+)
+finally:
+assert self._qmp_connection is None
 
 self._close_qemu_log_file()
 
@@ -420,6 +426,31 @@ def _launch(self) -> None:
close_fds=False)
 self._post_launch()
 
+def _close_qmp_connection(self) -> None:
+"""
+Close the underlying QMP connection, if any.
+
+Dutifully report errors that occurred while closing, but assume
+that any error encountered indicates an abnormal termination
+process and not a failure to close.
+"""
+if self._qmp_connection is None:
+return
+
+try:
+self._qmp.close()
+except EOFError:
+# EOF can occur as an Exception here when using the Async
+# QMP backend. It indicates that the server closed the
+# stream. If we successfully issued 'quit' at any point,
+# then this was expected. If the remote went away without
+# our permission, it's worth reporting that as an abnormal
+# shutdown case.
+if not (self._user_killed or self._quit_issued):
+raise
+finally:
+self._qmp_connection = None
+
 def _early_cleanup(self) -> None:
 """
 Perform any cleanup that needs to happen before the VM exits.
@@ -460,9 +491,14 @@ def _soft_shutdown(self, timeout: Optional[int]) -> None:
 self._early_cleanup()
 
 if self._qmp_connection:
-if not self._quit_issued:
-# Might raise ConnectionReset
-self.qmp('quit')
+try:
+if not self._quit_issued:
+# May raise ExecInterruptedError or StateError if the
+# connection dies or has *already* died.
+self.qmp('quit')
+finally:
+# Regardless, we want to quiesce the connection.
+self._close_qmp_connection()
 
 # May raise subprocess.TimeoutExpired
 self._subp.wait(timeout=timeout)
-- 
2.31.1

[PATCH v3 7/7] python, iotests: replace qmp with aqmp

[John Snow]

Swap out the synchronous QEMUMonitorProtocol from qemu.qmp with the sync
wrapper from qemu.aqmp instead.

Add an escape hatch in the form of the environment variable
QEMU_PYTHON_LEGACY_QMP which allows you to cajole QEMUMachine into using
the old implementatin, proving that both implementations work
concurrently.

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

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index a0cf69786b4..a487c397459 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -41,7 +41,6 @@
 )
 
 from qemu.qmp import (  # pylint: disable=import-error
-QEMUMonitorProtocol,
 QMPMessage,
 QMPReturnValue,
 SocketAddrT,
@@ -50,6 +49,12 @@
 from . import console_socket
 
 
+if os.environ.get('QEMU_PYTHON_LEGACY_QMP'):
+from qemu.qmp import QEMUMonitorProtocol
+else:
+from qemu.aqmp.legacy import QEMUMonitorProtocol
+
+
 LOG = logging.getLogger(__name__)
 
 
-- 
2.31.1

[PATCH v3 6/7] python/aqmp: Create sync QMP wrapper for iotests

[John Snow]

This is a wrapper around the async QMPClient that mimics the old,
synchronous QEMUMonitorProtocol class. It is designed to be
interchangeable with the old implementation.

It does not, however, attempt to mimic Exception compatibility.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/legacy.py | 138 +
 1 file changed, 138 insertions(+)
 create mode 100644 python/qemu/aqmp/legacy.py

diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
new file mode 100644
index 000..9e7b9fb80b9
--- /dev/null
+++ b/python/qemu/aqmp/legacy.py
@@ -0,0 +1,138 @@
+"""
+Sync QMP Wrapper
+
+This class pretends to be qemu.qmp.QEMUMonitorProtocol.
+"""
+
+import asyncio
+from typing import (
+Awaitable,
+List,
+Optional,
+TypeVar,
+Union,
+)
+
+import qemu.qmp
+from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
+
+from .qmp_client import QMPClient
+
+
+# pylint: disable=missing-docstring
+
+
+class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
+def __init__(self, address: SocketAddrT,
+ server: bool = False,
+ nickname: Optional[str] = None):
+
+# pylint: disable=super-init-not-called
+self._aqmp = QMPClient(nickname)
+self._aloop = asyncio.get_event_loop()
+self._address = address
+self._timeout: Optional[float] = None
+
+_T = TypeVar('_T')
+
+def _sync(
+self, future: Awaitable[_T], timeout: Optional[float] = None
+) -> _T:
+return self._aloop.run_until_complete(
+asyncio.wait_for(future, timeout=timeout)
+)
+
+def _get_greeting(self) -> Optional[QMPMessage]:
+if self._aqmp.greeting is not None:
+# pylint: disable=protected-access
+return self._aqmp.greeting._asdict()
+return None
+
+# __enter__ and __exit__ need no changes
+# parse_address needs no changes
+
+def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
+self._aqmp.await_greeting = negotiate
+self._aqmp.negotiate = negotiate
+
+self._sync(
+self._aqmp.connect(self._address)
+)
+return self._get_greeting()
+
+def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
+self._aqmp.await_greeting = True
+self._aqmp.negotiate = True
+
+self._sync(
+self._aqmp.accept(self._address),
+timeout
+)
+
+ret = self._get_greeting()
+assert ret is not None
+return ret
+
+def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
+return dict(
+self._sync(
+# pylint: disable=protected-access
+
+# _raw() isn't a public API, because turning off
+# automatic ID assignment is discouraged. For
+# compatibility with iotests *only*, do it anyway.
+self._aqmp._raw(qmp_cmd, assign_id=False),
+self._timeout
+)
+)
+
+# Default impl of cmd() delegates to cmd_obj
+
+def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+return self._sync(
+self._aqmp.execute(cmd, kwds),
+self._timeout
+)
+
+def pull_event(self,
+   wait: Union[bool, float] = False) -> Optional[QMPMessage]:
+if not wait:
+# wait is False/0: "do not wait, do not except."
+if self._aqmp.events.empty():
+return None
+
+# If wait is 'True', wait forever. If wait is False/0, the events
+# queue must not be empty; but it still needs some real amount
+# of time to complete.
+timeout = None
+if wait and isinstance(wait, float):
+timeout = wait
+
+return dict(
+self._sync(
+self._aqmp.events.get(),
+timeout
+)
+)
+
+def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]:
+events = [dict(x) for x in self._aqmp.events.clear()]
+if events:
+return events
+
+event = self.pull_event(wait)
+return [event] if event is not None else []
+
+def clear_events(self) -> None:
+self._aqmp.events.clear()
+
+def close(self) -> None:
+self._sync(
+self._aqmp.disconnect()
+)
+
+def settimeout(self, timeout: Optional[float]) -> None:
+self._timeout = timeout
+
+def send_fd_scm(self, fd: int) -> None:
+self._aqmp.send_fd_scm(fd)
-- 
2.31.1

[PATCH v3 5/7] iotests: Conditionally silence certain AQMP errors

[John Snow]

AQMP likes to be very chatty about errors it encounters. In general,
this is good because it allows us to get good diagnostic information for
otherwise complex async failures.

For example, during a failed QMP connection attempt, we might see:

+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Negotiation failed: EOFError
+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Failed to establish session: EOFError

This might be nice in iotests output, because failure scenarios
involving the new QMP library will be spelled out plainly in the output
diffs.

For tests that are intentionally causing this scenario though, filtering
that log output could be a hassle. For now, add a context manager that
simply lets us toggle this output off during a critical region.

(Additionally, a forthcoming patch allows the use of either legacy or
async QMP to be toggled with an environment variable. In this
circumstance, we can't amend the iotest output to just always expect the
error message, either. Just suppress it for now. More rigorous log
filtering can be investigated later if/when it is deemed safe to
permanently replace the legacy QMP library.)

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 20 +++-
 tests/qemu-iotests/tests/mirror-top-perms | 12 
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e5fff6ddcfc..e2f9d873ada 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,7 +30,7 @@
 import subprocess
 import sys
 import time
-from typing import (Any, Callable, Dict, Iterable,
+from typing import (Any, Callable, Dict, Iterable, Iterator,
 List, Optional, Sequence, TextIO, Tuple, Type, TypeVar)
 import unittest
 
@@ -114,6 +114,24 @@
 sample_img_dir = os.environ['SAMPLE_IMG_DIR']
 
 
+@contextmanager
+def change_log_level(
+logger_name: str, level: int = logging.CRITICAL) -> Iterator[None]:
+"""
+Utility function for temporarily changing the log level of a logger.
+
+This can be used to silence errors that are expected or uninteresting.
+"""
+_logger = logging.getLogger(logger_name)
+current_level = _logger.level
+_logger.setLevel(level)
+
+try:
+yield
+finally:
+_logger.setLevel(current_level)
+
+
 def unarchive_sample_image(sample, fname):
 sample_fname = os.path.join(sample_img_dir, sample + '.bz2')
 with bz2.open(sample_fname) as f_in, open(fname, 'wb') as f_out:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms 
b/tests/qemu-iotests/tests/mirror-top-perms
index a2d5c269d7a..0a51a613f39 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -26,7 +26,7 @@ from qemu.machine import machine
 from qemu.qmp import QMPConnectError
 
 import iotests
-from iotests import qemu_img
+from iotests import change_log_level, qemu_img
 
 
 image_size = 1 * 1024 * 1024
@@ -100,9 +100,13 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
 self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
 self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
 try:
-self.vm_b.launch()
-print('ERROR: VM B launched successfully, this should not have '
-  'happened')
+# Silence AQMP errors temporarily.
+# TODO: Remove this and just allow the errors to be logged when
+# AQMP fully replaces QMP.
+with change_log_level('qemu.aqmp'):
+self.vm_b.launch()
+print('ERROR: VM B launched successfully, '
+  'this should not have happened')
 except (QMPConnectError, ConnectError):
 assert 'Is another process using the image' in self.vm_b.get_log()
 
-- 
2.31.1

[PATCH v3 0/7] Switch iotests to using Async QMP

[John Snow]

Based-on: <20211012214152.802483-1-js...@redhat.com>
  [PULL 00/10] Python patches
GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-iotest-wrapper
CI: https://gitlab.com/jsnow/qemu/-/pipelines/387210591

Hiya,

This series continues where the last two AQMP series left off and adds a
synchronous 'legacy' wrapper around the new AQMP interface, then drops
it straight into iotests to prove that AQMP is functional and totally
cool and fine. The disruption and churn to iotests is pretty minimal.

In the event that a regression happens and I am not physically proximate
to inflict damage upon, one may set the QEMU_PYTHON_LEGACY_QMP variable
to any non-empty string as it pleases you to engage the QMP machinery
you are used to.

I'd like to try and get this committed early in the 6.2 development
cycle to give ample time to smooth over any possible regressions. I've
tested it locally and via gitlab CI, across Python versions 3.6 through
3.10, and "worksforme". If something bad happens, we can revert the
actual switch-flip very trivially.

V3:

001/7:[] [--] 'python/machine: remove has_quit argument'
002/7:[0002] [FC] 'python/machine: Handle QMP errors on close more meticulously'
003/7:[] [--] 'python/aqmp: Remove scary message'
004/7:[0006] [FC] 'iotests: Accommodate async QMP Exception classes'
005/7:[0003] [FC] 'iotests: Conditionally silence certain AQMP errors'
006/7:[0009] [FC] 'python/aqmp: Create sync QMP wrapper for iotests'
007/7:[] [--] 'python, iotests: replace qmp with aqmp'

002: Account for force-kill cases, too.
003: Shuffled earlier into the series to prevent a mid-series regression.
004: Rewrite the imports to be less "heterogeneous" ;)
005: Add in a TODO for me to trip over in the future.
006: Fix a bug surfaced by a new iotest where waiting with pull_event for a
 timeout of 0.0 will cause a timeout exception to be raised even if there
 was an event ready to be read.

V2:

001/17:[] [--] 'python/aqmp: add greeting property to QMPClient'
002/17:[] [--] 'python/aqmp: add .empty() method to EventListener'
003/17:[] [--] 'python/aqmp: Return cleared events from 
EventListener.clear()'
004/17:[0007] [FC] 'python/aqmp: add send_fd_scm'
005/17:[down] 'python/aqmp: Add dict conversion method to Greeting object'
006/17:[down] 'python/aqmp: Reduce severity of EOFError-caused loop 
terminations'
007/17:[down] 'python/aqmp: Disable logging messages by default'

008/17:[0002] [FC] 'python/qmp: clear events on get_events() call'
009/17:[] [--] 'python/qmp: add send_fd_scm directly to QEMUMonitorProtocol'
010/17:[] [--] 'python, iotests: remove socket_scm_helper'
011/17:[0013] [FC] 'python/machine: remove has_quit argument'
012/17:[down] 'python/machine: Handle QMP errors on close more meticulously'

013/17:[0009] [FC] 'iotests: Accommodate async QMP Exception classes'
014/17:[down] 'iotests: Conditionally silence certain AQMP errors'

015/17:[0016] [FC] 'python/aqmp: Create sync QMP wrapper for iotests'
016/17:[0002] [FC] 'python/aqmp: Remove scary message'
017/17:[] [--] 'python, iotests: replace qmp with aqmp'

- Rebased on jsnow/python, which was recently rebased on origin/master.
- Make aqmp's send_fd_scm method bark if the socket isn't AF_UNIX (Hanna)
- Uh... modify send_fd_scm so it doesn't break when Python 3.11 comes out ...
  See the commit message for more detail.
- Drop the "python/aqmp: Create MessageModel and StandaloneModel class"
  patch and replace with a far simpler method that just adds an
  _asdict() method.
- Add patches 06 and 07 to change how the AQMP library handles logging.
- Adjust docstring in patch 08 (Hanna)
- Rename "_has_quit" attribute to "_quid_issued" (Hanna)
- Renamed patch 12, simplified the logic in _soft_shutdown a tiny bit.
- Fixed bad exception handling logic in 13 (Hanna)
- Introduce a helper in patch 14 to silence log output when it's unwanted.
- Small addition of _get_greeting() helper in patch 15, coinciding with the
  new patch 05 here.
- Contextual changes in 16.

John Snow (7):
  python/machine: remove has_quit argument
  python/machine: Handle QMP errors on close more meticulously
  python/aqmp: Remove scary message
  iotests: Accommodate async QMP Exception classes
  iotests: Conditionally silence certain AQMP errors
  python/aqmp: Create sync QMP wrapper for iotests
  python, iotests: replace qmp with aqmp

 python/qemu/aqmp/__init__.py  |  12 --
 python/qemu/aqmp/legacy.py| 138 ++
 python/qemu/machine/machine.py|  85 +
 scripts/simplebench/bench_block_job.py|   3 +-
 tests/qemu-iotests/040|   7 +-
 tests/qemu-iotests/218|   2 +-
 tests/qemu-iotests/255|   2 +-
 tests/qemu-iotests/iotests.py |  20 +++-
 tests/qemu-iotests/tests/mirror-top-perms |  17 ++-
 9 files changed, 238 insertions(+), 48 del

[PATCH v3 3/7] python/aqmp: Remove scary message

[John Snow]

The scary message interferes with the iotests output. Coincidentally, if
iotests works by removing this, then it's good evidence that we don't
really need to scare people away from using it.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/__init__.py | 12 
 1 file changed, 12 deletions(-)

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index d1b0e4dc3d3..880d5b6fa7f 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,7 +22,6 @@
 # the COPYING file in the top-level directory.
 
 import logging
-import warnings
 
 from .error import AQMPError
 from .events import EventListener
@@ -31,17 +30,6 @@
 from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient
 
 
-_WMSG = """
-
-The Asynchronous QMP library is currently in development and its API
-should be considered highly fluid and subject to change. It should
-not be used by any other scripts checked into the QEMU tree.
-
-Proceed with caution!
-"""
-
-warnings.warn(_WMSG, FutureWarning)
-
 # Suppress logging unless an application engages it.
 logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler())
 
-- 
2.31.1

[PATCH v3 4/7] iotests: Accommodate async QMP Exception classes

[John Snow]

(But continue to support the old ones for now, too.)

There are very few cases of any user of QEMUMachine or a subclass
thereof relying on a QMP Exception type. If you'd like to check for
yourself, you want to grep for all of the derivatives of QMPError,
excluding 'AQMPError' and its derivatives. That'd be these:

- QMPError
- QMPConnectError
- QMPCapabilitiesError
- QMPTimeoutError
- QMPProtocolError
- QMPResponseError
- QMPBadPortError


Signed-off-by: John Snow 
---
 scripts/simplebench/bench_block_job.py| 3 ++-
 tests/qemu-iotests/tests/mirror-top-perms | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/scripts/simplebench/bench_block_job.py 
b/scripts/simplebench/bench_block_job.py
index 4f03c121697..a403c35b08f 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -28,6 +28,7 @@
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu.machine import QEMUMachine
 from qemu.qmp import QMPConnectError
+from qemu.aqmp import ConnectError
 
 
 def bench_block_job(cmd, cmd_args, qemu_args):
@@ -49,7 +50,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
 vm.launch()
 except OSError as e:
 return {'error': 'popen failed: ' + str(e)}
-except (QMPConnectError, socket.timeout):
+except (QMPConnectError, ConnectError, socket.timeout):
 return {'error': 'qemu failed: ' + str(vm.get_log())}
 
 try:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms 
b/tests/qemu-iotests/tests/mirror-top-perms
index 3d475aa3a54..a2d5c269d7a 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,8 +21,9 @@
 
 import os
 
-from qemu import qmp
+from qemu.aqmp import ConnectError
 from qemu.machine import machine
+from qemu.qmp import QMPConnectError
 
 import iotests
 from iotests import qemu_img
@@ -102,7 +103,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
 self.vm_b.launch()
 print('ERROR: VM B launched successfully, this should not have '
   'happened')
-except qmp.QMPConnectError:
+except (QMPConnectError, ConnectError):
 assert 'Is another process using the image' in self.vm_b.get_log()
 
 result = self.vm.qmp('block-job-cancel',
-- 
2.31.1

[PATCH v3 1/7] python/machine: remove has_quit argument

[John Snow]

If we spy on the QMP commands instead, we don't need callers to remember
to pass it. Seems like a fair trade-off.

The one slightly weird bit is overloading this instance variable for
wait(), where we use it to mean "don't issue the qmp 'quit'
command". This means that wait() will "fail" if the QEMU process does
not terminate of its own accord.

In most cases, we probably did already actually issue quit -- some
iotests do this -- but in some others, we may be waiting for QEMU to
terminate for some other reason, such as a test wherein we tell the
guest (directly) to shut down.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
 python/qemu/machine/machine.py | 34 +++---
 tests/qemu-iotests/040 |  7 +--
 tests/qemu-iotests/218 |  2 +-
 tests/qemu-iotests/255 |  2 +-
 4 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 056d340e355..0bd40bc2f76 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -170,6 +170,7 @@ def __init__(self,
 self._console_socket: Optional[socket.socket] = None
 self._remove_files: List[str] = []
 self._user_killed = False
+self._quit_issued = False
 
 def __enter__(self: _T) -> _T:
 return self
@@ -368,6 +369,7 @@ def _post_shutdown(self) -> None:
 command = ''
 LOG.warning(msg, -int(exitcode), command)
 
+self._quit_issued = False
 self._user_killed = False
 self._launched = False
 
@@ -443,15 +445,13 @@ def _hard_shutdown(self) -> None:
 self._subp.kill()
 self._subp.wait(timeout=60)
 
-def _soft_shutdown(self, timeout: Optional[int],
-   has_quit: bool = False) -> None:
+def _soft_shutdown(self, timeout: Optional[int]) -> None:
 """
 Perform early cleanup, attempt to gracefully shut down the VM, and wait
 for it to terminate.
 
 :param timeout: Timeout in seconds for graceful shutdown.
 A value of None is an infinite wait.
-:param has_quit: When True, don't attempt to issue 'quit' QMP command
 
 :raise ConnectionReset: On QMP communication errors
 :raise subprocess.TimeoutExpired: When timeout is exceeded waiting for
@@ -460,21 +460,19 @@ def _soft_shutdown(self, timeout: Optional[int],
 self._early_cleanup()
 
 if self._qmp_connection:
-if not has_quit:
+if not self._quit_issued:
 # Might raise ConnectionReset
-self._qmp.cmd('quit')
+self.qmp('quit')
 
 # May raise subprocess.TimeoutExpired
 self._subp.wait(timeout=timeout)
 
-def _do_shutdown(self, timeout: Optional[int],
- has_quit: bool = False) -> None:
+def _do_shutdown(self, timeout: Optional[int]) -> None:
 """
 Attempt to shutdown the VM gracefully; fallback to a hard shutdown.
 
 :param timeout: Timeout in seconds for graceful shutdown.
 A value of None is an infinite wait.
-:param has_quit: When True, don't attempt to issue 'quit' QMP command
 
 :raise AbnormalShutdown: When the VM could not be shut down gracefully.
 The inner exception will likely be ConnectionReset or
@@ -482,13 +480,13 @@ def _do_shutdown(self, timeout: Optional[int],
 may result in its own exceptions, likely subprocess.TimeoutExpired.
 """
 try:
-self._soft_shutdown(timeout, has_quit)
+self._soft_shutdown(timeout)
 except Exception as exc:
 self._hard_shutdown()
 raise AbnormalShutdown("Could not perform graceful shutdown") \
 from exc
 
-def shutdown(self, has_quit: bool = False,
+def shutdown(self,
  hard: bool = False,
  timeout: Optional[int] = 30) -> None:
 """
@@ -498,7 +496,6 @@ def shutdown(self, has_quit: bool = False,
 If the VM has not yet been launched, or shutdown(), wait(), or kill()
 have already been called, this method does nothing.
 
-:param has_quit: When true, do not attempt to issue 'quit' QMP command.
 :param hard: When true, do not attempt graceful shutdown, and
  suppress the SIGKILL warning log message.
 :param timeout: Optional timeout in seconds for graceful shutdown.
@@ -512,7 +509,7 @@ def shutdown(self, has_quit: bool = False,
 self._user_killed = True
 self._hard_shutdown()
 else:
-self._do_shutdown(timeout, has_quit)
+self._do_shutdown(timeout)
 finally:
 self._post_shutdown()
 
@@ -529,7 +526,8 @@ def wait(se

[PULL 10/10] python, iotests: remove socket_scm_helper

[John Snow]

It's not used anymore, now.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
Reviewed-by: Paolo Bonzini 
Message-id: 20210923004938.363-11-js...@redhat.com
Signed-off-by: John Snow 
---
 tests/qemu-iotests/socket_scm_helper.c | 136 -
 python/qemu/machine/machine.py |   3 -
 python/qemu/machine/qtest.py   |   2 -
 tests/Makefile.include |   1 -
 tests/meson.build  |   4 -
 tests/qemu-iotests/iotests.py  |   3 -
 tests/qemu-iotests/meson.build |   5 -
 tests/qemu-iotests/testenv.py  |   8 +-
 8 files changed, 1 insertion(+), 161 deletions(-)
 delete mode 100644 tests/qemu-iotests/socket_scm_helper.c
 delete mode 100644 tests/qemu-iotests/meson.build

diff --git a/tests/qemu-iotests/socket_scm_helper.c 
b/tests/qemu-iotests/socket_scm_helper.c
deleted file mode 100644
index eb76d31aa94..000
--- a/tests/qemu-iotests/socket_scm_helper.c
+++ /dev/null
@@ -1,136 +0,0 @@
-/*
- * SCM_RIGHTS with unix socket help program for test
- *
- * Copyright IBM, Inc. 2013
- *
- * Authors:
- *  Wenchao Xia
- *
- * This work is licensed under the terms of the GNU LGPL, version 2 or later.
- * See the COPYING.LIB file in the top-level directory.
- */
-
-#include "qemu/osdep.h"
-#include 
-#include 
-
-/* #define SOCKET_SCM_DEBUG */
-
-/*
- * @fd and @fd_to_send will not be checked for validation in this function,
- * a blank will be sent as iov data to notify qemu.
- */
-static int send_fd(int fd, int fd_to_send)
-{
-struct msghdr msg;
-struct iovec iov[1];
-int ret;
-char control[CMSG_SPACE(sizeof(int))];
-struct cmsghdr *cmsg;
-
-memset(, 0, sizeof(msg));
-memset(control, 0, sizeof(control));
-
-/* Send a blank to notify qemu */
-iov[0].iov_base = (void *)" ";
-iov[0].iov_len = 1;
-
-msg.msg_iov = iov;
-msg.msg_iovlen = 1;
-
-msg.msg_control = control;
-msg.msg_controllen = sizeof(control);
-
-cmsg = CMSG_FIRSTHDR();
-
-cmsg->cmsg_len = CMSG_LEN(sizeof(int));
-cmsg->cmsg_level = SOL_SOCKET;
-cmsg->cmsg_type = SCM_RIGHTS;
-memcpy(CMSG_DATA(cmsg), _to_send, sizeof(int));
-
-do {
-ret = sendmsg(fd, , 0);
-} while (ret < 0 && errno == EINTR);
-
-if (ret < 0) {
-fprintf(stderr, "Failed to send msg, reason: %s\n", strerror(errno));
-}
-
-return ret;
-}
-
-/* Convert string to fd number. */
-static int get_fd_num(const char *fd_str, bool silent)
-{
-int sock;
-char *err;
-
-errno = 0;
-sock = strtol(fd_str, , 10);
-if (errno) {
-if (!silent) {
-fprintf(stderr, "Failed in strtol for socket fd, reason: %s\n",
-strerror(errno));
-}
-return -1;
-}
-if (!*fd_str || *err || sock < 0) {
-if (!silent) {
-fprintf(stderr, "bad numerical value for socket fd '%s'\n", 
fd_str);
-}
-return -1;
-}
-
-return sock;
-}
-
-/*
- * To make things simple, the caller needs to specify:
- * 1. socket fd.
- * 2. path of the file to be sent.
- */
-int main(int argc, char **argv, char **envp)
-{
-int sock, fd, ret;
-
-#ifdef SOCKET_SCM_DEBUG
-int i;
-for (i = 0; i < argc; i++) {
-fprintf(stderr, "Parameter %d: %s\n", i, argv[i]);
-}
-#endif
-
-if (argc != 3) {
-fprintf(stderr,
-"Usage: %s < socket-fd > < file-path >\n",
-argv[0]);
-return EXIT_FAILURE;
-}
-
-
-sock = get_fd_num(argv[1], false);
-if (sock < 0) {
-return EXIT_FAILURE;
-}
-
-fd = get_fd_num(argv[2], true);
-if (fd < 0) {
-/* Now only open a file in readonly mode for test purpose. If more
-   precise control is needed, use python script in file operation, 
which
-   is supposed to fork and exec this program. */
-fd = open(argv[2], O_RDONLY);
-if (fd < 0) {
-fprintf(stderr, "Failed to open file '%s'\n", argv[2]);
-return EXIT_FAILURE;
-}
-}
-
-ret = send_fd(sock, fd);
-if (ret < 0) {
-close(fd);
-return EXIT_FAILURE;
-}
-
-close(fd);
-return EXIT_SUCCESS;
-}
diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 1c6532a3d68..056d340e355 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -98,7 +98,6 @@ def __init__(self,
  name: Optional[str] = None,
  base_temp_dir: str = "/var/tmp",
  monitor_address: Optional[SocketAddrT] = None,
- socket_scm_helper: Optional[str] = None,
  sock_dir: Optional[str] = None,
  drain_console: bool = False,
  console_log: Optional[str] = None,
@@ -113,7 +112,6 @@ def __init__(self,

[PULL 07/10] python/aqmp: Disable logging messages by default

[John Snow]

AQMP is a library, and ideally it should not print error diagnostics
unless a user opts into seeing them. By default, Python will print all
WARNING, ERROR or CRITICAL messages to screen if no logging
configuration has been created by a client application.

In AQMP's case, ERROR logging statements are used to report additional
detail about runtime failures that will also eventually be reported to the
client library via an Exception, so these messages should not be
rendered by default.

(Why bother to have them at all, then? In async contexts, there may be
multiple Exceptions and we are only able to report one of them back to
the client application. It is not reasonably easy to predict ahead of
time if one or more of these Exceptions will be squelched. Therefore,
it's useful to log intermediate failures to help make sense of the
ultimate, resulting failure.)

Add a NullHandler that will suppress these messages until a client
application opts into logging via logging.basicConfig or similar. Note
that upon calling basicConfig(), this handler will *not* suppress these
messages from being displayed by the client's configuration.

Signed-off-by: John Snow 
Reviewed-by: Paolo Bonzini 
Reviewed-by: Eric Blake 
Message-id: 20210923004938.363-8-js...@redhat.com
Signed-off-by: John Snow 
---
 python/qemu/aqmp/__init__.py | 4 
 1 file changed, 4 insertions(+)

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index ab1782999cf..d1b0e4dc3d3 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -21,6 +21,7 @@
 # This work is licensed under the terms of the GNU GPL, version 2.  See
 # the COPYING file in the top-level directory.
 
+import logging
 import warnings
 
 from .error import AQMPError
@@ -41,6 +42,9 @@
 
 warnings.warn(_WMSG, FutureWarning)
 
+# Suppress logging unless an application engages it.
+logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler())
+
 
 # The order of these fields impact the Sphinx documentation order.
 __all__ = (
-- 
2.31.1

[PULL 09/10] python/qmp: add send_fd_scm directly to QEMUMonitorProtocol

[John Snow]

It turns out you can do this directly from Python ... and because of
this, you don't need to worry about setting the inheritability of the
fds or spawning another process.

Doing this is helpful because it allows QEMUMonitorProtocol to keep its
file descriptor and socket object as private implementation
details. /that/ is helpful in turn because it allows me to write a
compatible, alternative implementation.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
Reviewed-by: Paolo Bonzini 
Message-id: 20210923004938.363-10-js...@redhat.com
Signed-off-by: John Snow 
---
 python/qemu/machine/machine.py | 44 +++---
 python/qemu/qmp/__init__.py| 21 +++-
 2 files changed, 18 insertions(+), 47 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index ae945ca3c94..1c6532a3d68 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -213,48 +213,22 @@ def add_fd(self: _T, fd: int, fdset: int,
 def send_fd_scm(self, fd: Optional[int] = None,
 file_path: Optional[str] = None) -> int:
 """
-Send an fd or file_path to socket_scm_helper.
+Send an fd or file_path to the remote via SCM_RIGHTS.
 
-Exactly one of fd and file_path must be given.
-If it is file_path, the helper will open that file and pass its own fd.
+Exactly one of fd and file_path must be given.  If it is
+file_path, the file will be opened read-only and the new file
+descriptor will be sent to the remote.
 """
-# In iotest.py, the qmp should always use unix socket.
-assert self._qmp.is_scm_available()
-if self._socket_scm_helper is None:
-raise QEMUMachineError("No path to socket_scm_helper set")
-if not os.path.exists(self._socket_scm_helper):
-raise QEMUMachineError("%s does not exist" %
-   self._socket_scm_helper)
-
-# This did not exist before 3.4, but since then it is
-# mandatory for our purpose
-if hasattr(os, 'set_inheritable'):
-os.set_inheritable(self._qmp.get_sock_fd(), True)
-if fd is not None:
-os.set_inheritable(fd, True)
-
-fd_param = ["%s" % self._socket_scm_helper,
-"%d" % self._qmp.get_sock_fd()]
-
 if file_path is not None:
 assert fd is None
-fd_param.append(file_path)
+with open(file_path, "rb") as passfile:
+fd = passfile.fileno()
+self._qmp.send_fd_scm(fd)
 else:
 assert fd is not None
-fd_param.append(str(fd))
+self._qmp.send_fd_scm(fd)
 
-proc = subprocess.run(
-fd_param,
-stdin=subprocess.DEVNULL,
-stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT,
-check=False,
-close_fds=False,
-)
-if proc.stdout:
-LOG.debug(proc.stdout)
-
-return proc.returncode
+return 0
 
 @staticmethod
 def _remove_if_exists(path: str) -> None:
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
index c27594b66a2..358c0971d06 100644
--- a/python/qemu/qmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -21,6 +21,7 @@
 import json
 import logging
 import socket
+import struct
 from types import TracebackType
 from typing import (
 Any,
@@ -408,18 +409,14 @@ def settimeout(self, timeout: Optional[float]) -> None:
 raise ValueError(msg)
 self.__sock.settimeout(timeout)
 
-def get_sock_fd(self) -> int:
+def send_fd_scm(self, fd: int) -> None:
 """
-Get the socket file descriptor.
-
-@return The file descriptor number.
-"""
-return self.__sock.fileno()
-
-def is_scm_available(self) -> bool:
+Send a file descriptor to the remote via SCM_RIGHTS.
 """
-Check if the socket allows for SCM_RIGHTS.
+if self.__sock.family != socket.AF_UNIX:
+raise RuntimeError("Can't use SCM_RIGHTS on non-AF_UNIX socket.")
 
-@return True if SCM_RIGHTS is available, otherwise False.
-"""
-return self.__sock.family == socket.AF_UNIX
+self.__sock.sendmsg(
+[b' '],
+[(socket.SOL_SOCKET, socket.SCM_RIGHTS, struct.pack('@i', fd))]
+)
-- 
2.31.1

[PULL 08/10] python/qmp: clear events on get_events() call

[John Snow]

All callers in the tree *already* clear the events after a call to
get_events(). Do it automatically instead and update callsites to remove
the manual clear call.

These semantics are quite a bit easier to emulate with async QMP, and
nobody appears to be abusing some emergent properties of what happens if
you decide not to clear them, so let's dial down to the dumber, simpler
thing.

Specifically: callers of clear() right after a call to get_events() are
more likely expressing their desire to not see any events they just
retrieved, whereas callers of clear_events() not in relation to a recent
call to pull_event/get_events are likely expressing their desire to
simply drop *all* pending events straight onto the floor. In the sync
world, this is safe enough; in the async world it's nearly impossible to
promise that nothing happens between getting and clearing the
events.

Making the retrieval also clear the queue is vastly simpler.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
Reviewed-by: Paolo Bonzini 
Message-id: 20210923004938.363-9-js...@redhat.com
Signed-off-by: John Snow 
---
 python/qemu/machine/machine.py | 1 -
 python/qemu/qmp/__init__.py| 6 --
 python/qemu/qmp/qmp_shell.py   | 1 -
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 34131884a57..ae945ca3c94 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -631,7 +631,6 @@ def get_qmp_events(self, wait: bool = False) -> 
List[QMPMessage]:
 events = self._qmp.get_events(wait=wait)
 events.extend(self._events)
 del self._events[:]
-self._qmp.clear_events()
 return events
 
 @staticmethod
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
index 269516a79b9..c27594b66a2 100644
--- a/python/qemu/qmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -361,7 +361,7 @@ def pull_event(self,
 
 def get_events(self, wait: bool = False) -> List[QMPMessage]:
 """
-Get a list of available QMP events.
+Get a list of available QMP events and clear all pending events.
 
 @param wait (bool): block until an event is available.
 @param wait (float): If wait is a float, treat it as a timeout value.
@@ -374,7 +374,9 @@ def get_events(self, wait: bool = False) -> 
List[QMPMessage]:
 @return The list of available QMP events.
 """
 self.__get_events(wait)
-return self.__events
+events = self.__events
+self.__events = []
+return events
 
 def clear_events(self) -> None:
 """
diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/qmp/qmp_shell.py
index 337acfce2d2..e7d7eb18f19 100644
--- a/python/qemu/qmp/qmp_shell.py
+++ b/python/qemu/qmp/qmp_shell.py
@@ -381,7 +381,6 @@ def read_exec_command(self) -> bool:
 if cmdline == '':
 for event in self.get_events():
 print(event)
-self.clear_events()
 return True
 
 return self._execute_cmd(cmdline)
-- 
2.31.1

[PULL 06/10] python/aqmp: Reduce severity of EOFError-caused loop terminations

[John Snow]

When we encounter an EOFError, we don't know if it's an "error" in the
perspective of the user of the library yet. Therefore, we should not log
it as an error. Reduce the severity of this logging message to "INFO" to
indicate that it's something that we expect to occur during the normal
operation of the library.

Signed-off-by: John Snow 
Reviewed-by: Paolo Bonzini 
Reviewed-by: Eric Blake 
Message-id: 20210923004938.363-7-js...@redhat.com
Signed-off-by: John Snow 
---
 python/qemu/aqmp/protocol.py | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 32e78749c11..ae1df240260 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -721,8 +721,11 @@ async def _bh_loop_forever(self, async_fn: _TaskFN, name: 
str) -> None:
 self.logger.debug("Task.%s: cancelled.", name)
 return
 except BaseException as err:
-self.logger.error("Task.%s: %s",
-  name, exception_summary(err))
+self.logger.log(
+logging.INFO if isinstance(err, EOFError) else logging.ERROR,
+"Task.%s: %s",
+name, exception_summary(err)
+)
 self.logger.debug("Task.%s: failure:\n%s\n",
   name, pretty_traceback())
 self._schedule_disconnect()
-- 
2.31.1

[PULL 02/10] python/aqmp: add .empty() method to EventListener

[John Snow]

Synchronous clients may want to know if they're about to block waiting
for an event or not. A method such as this is necessary to implement a
compatible interface for the old QEMUMonitorProtocol using the new async
internals.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
Reviewed-by: Paolo Bonzini 
Message-id: 20210923004938.363-3-js...@redhat.com
Signed-off-by: John Snow 
---
 python/qemu/aqmp/events.py | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/python/qemu/aqmp/events.py b/python/qemu/aqmp/events.py
index fb81d216102..271899f6b82 100644
--- a/python/qemu/aqmp/events.py
+++ b/python/qemu/aqmp/events.py
@@ -556,6 +556,12 @@ async def get(self) -> Message:
 """
 return await self._queue.get()
 
+def empty(self) -> bool:
+"""
+Return `True` if there are no pending events.
+"""
+return self._queue.empty()
+
 def clear(self) -> None:
 """
 Clear this listener of all pending events.
-- 
2.31.1

[PULL 05/10] python/aqmp: Add dict conversion method to Greeting object

[John Snow]

The iotests interface expects to return the greeting as a dict; AQMP
offers it as a rich object.

Signed-off-by: John Snow 
Reviewed-by: Paolo Bonzini 
Reviewed-by: Eric Blake 
Message-id: 20210923004938.363-6-js...@redhat.com
Signed-off-by: John Snow 
---
 python/qemu/aqmp/models.py | 13 +
 1 file changed, 13 insertions(+)

diff --git a/python/qemu/aqmp/models.py b/python/qemu/aqmp/models.py
index 24c94123ac0..de87f878047 100644
--- a/python/qemu/aqmp/models.py
+++ b/python/qemu/aqmp/models.py
@@ -8,8 +8,10 @@
 # pylint: disable=too-few-public-methods
 
 from collections import abc
+import copy
 from typing import (
 Any,
+Dict,
 Mapping,
 Optional,
 Sequence,
@@ -66,6 +68,17 @@ def __init__(self, raw: Mapping[str, Any]):
 self._check_member('QMP', abc.Mapping, "JSON object")
 self.QMP = QMPGreeting(self._raw['QMP'])
 
+def _asdict(self) -> Dict[str, object]:
+"""
+For compatibility with the iotests sync QMP wrapper.
+
+The legacy QMP interface needs Greetings as a garden-variety Dict.
+
+This interface is private in the hopes that it will be able to
+be dropped again in the near-future. Caller beware!
+"""
+return dict(copy.deepcopy(self._raw))
+
 
 class QMPGreeting(Model):
 """
-- 
2.31.1

[PULL 04/10] python/aqmp: add send_fd_scm

[John Snow]

Add an implementation for send_fd_scm to the async QMP implementation.
Like socket_scm_helper mentions, a non-empty payload is required for
QEMU to process the ancillary data. A space is most useful because it
does not disturb the parsing of subsequent JSON objects.

A note on "voiding the warranty":

Python 3.11 removes support for calling sendmsg directly from a
transport's socket. There is no other interface for doing this, our use
case is, I suspect, "quite unique".

As far as I can tell, this is safe to do -- send_fd_scm is a synchronous
function and we can be guaranteed that the async coroutines will *not* be
running when it is invoked. In testing, it works correctly.

I investigated quite thoroughly the possibility of creating my own
asyncio Transport (The class that ultimately manages the raw socket
object) so that I could manage the socket myself, but this is so wildly
invasive and unportable I scrapped the idea. It would involve a lot of
copy-pasting of various python utilities and classes just to re-create
the same infrastructure, and for extremely little benefit. Nah.

Just boldly void the warranty instead, while I try to follow up on
https://bugs.python.org/issue43232

Signed-off-by: John Snow 
Reviewed-by: Paolo Bonzini 
Reviewed-by: Eric Blake 
Message-id: 20210923004938.363-5-js...@redhat.com
Signed-off-by: John Snow 
---
 python/qemu/aqmp/qmp_client.py | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index d2ad7459f9f..f987da02eb0 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -9,6 +9,8 @@
 
 import asyncio
 import logging
+import socket
+import struct
 from typing import (
 Dict,
 List,
@@ -624,3 +626,23 @@ async def execute(self, cmd: str,
 """
 msg = self.make_execute_msg(cmd, arguments, oob=oob)
 return await self.execute_msg(msg)
+
+@upper_half
+@require(Runstate.RUNNING)
+def send_fd_scm(self, fd: int) -> None:
+"""
+Send a file descriptor to the remote via SCM_RIGHTS.
+"""
+assert self._writer is not None
+sock = self._writer.transport.get_extra_info('socket')
+
+if sock.family != socket.AF_UNIX:
+raise AQMPError("Sending file descriptors requires a UNIX socket.")
+
+# Void the warranty sticker.
+# Access to sendmsg in asyncio is scheduled for removal in Python 3.11.
+sock = sock._sock  # pylint: disable=protected-access
+sock.sendmsg(
+[b' '],
+[(socket.SOL_SOCKET, socket.SCM_RIGHTS, struct.pack('@i', fd))]
+)
-- 
2.31.1

[PULL 03/10] python/aqmp: Return cleared events from EventListener.clear()

[John Snow]

This serves two purposes:

(1) It is now possible to discern whether or not clear() removed any
event(s) from the queue with absolute certainty, and

(2) It is now very easy to get a List of all pending events in one
chunk, which is useful for the sync bridge.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
Reviewed-by: Paolo Bonzini 
Message-id: 20210923004938.363-4-js...@redhat.com
Signed-off-by: John Snow 
---
 python/qemu/aqmp/events.py | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/python/qemu/aqmp/events.py b/python/qemu/aqmp/events.py
index 271899f6b82..5f7150c78d4 100644
--- a/python/qemu/aqmp/events.py
+++ b/python/qemu/aqmp/events.py
@@ -562,7 +562,7 @@ def empty(self) -> bool:
 """
 return self._queue.empty()
 
-def clear(self) -> None:
+def clear(self) -> List[Message]:
 """
 Clear this listener of all pending events.
 
@@ -570,17 +570,22 @@ def clear(self) -> None:
 pending FIFO queue synchronously. It can be also be used to
 manually clear any pending events, if desired.
 
+:return: The cleared events, if any.
+
 .. warning::
 Take care when discarding events. Cleared events will be
 silently tossed on the floor. All events that were ever
 accepted by this listener are visible in `history()`.
 """
+events = []
 while True:
 try:
-self._queue.get_nowait()
+events.append(self._queue.get_nowait())
 except asyncio.QueueEmpty:
 break
 
+return events
+
 def __aiter__(self) -> AsyncIterator[Message]:
 return self
 
-- 
2.31.1

[PULL 01/10] python/aqmp: add greeting property to QMPClient

[John Snow]

Expose the greeting as a read-only property of QMPClient so it can be
retrieved at-will.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
Reviewed-by: Paolo Bonzini 
Message-id: 20210923004938.363-2-js...@redhat.com
Signed-off-by: John Snow 
---
 python/qemu/aqmp/qmp_client.py | 5 +
 1 file changed, 5 insertions(+)

diff --git a/python/qemu/aqmp/qmp_client.py b/python/qemu/aqmp/qmp_client.py
index 82e9dab124c..d2ad7459f9f 100644
--- a/python/qemu/aqmp/qmp_client.py
+++ b/python/qemu/aqmp/qmp_client.py
@@ -224,6 +224,11 @@ def __init__(self, name: Optional[str] = None) -> None:
 'asyncio.Queue[QMPClient._PendingT]'
 ] = {}
 
+@property
+def greeting(self) -> Optional[Greeting]:
+"""The `Greeting` from the QMP server, if any."""
+return self._greeting
+
 @upper_half
 async def _establish_session(self) -> None:
 """
-- 
2.31.1

[PULL 00/10] Python patches

[John Snow]

The following changes since commit bfd9a76f9c143d450ab5545dedfa74364b39fc56:

  Merge remote-tracking branch 'remotes/stsquad/tags/pull-for-6.2-121021-2' 
into staging (2021-10-12 06:16:25 -0700)

are available in the Git repository at:

  https://gitlab.com/jsnow/qemu.git tags/python-pull-request

for you to fetch changes up to c163c723ef92d0f629d015902396f2c67328b2e5:

  python, iotests: remove socket_scm_helper (2021-10-12 12:22:11 -0400)


Pull request



John Snow (10):
  python/aqmp: add greeting property to QMPClient
  python/aqmp: add .empty() method to EventListener
  python/aqmp: Return cleared events from EventListener.clear()
  python/aqmp: add send_fd_scm
  python/aqmp: Add dict conversion method to Greeting object
  python/aqmp: Reduce severity of EOFError-caused loop terminations
  python/aqmp: Disable logging messages by default
  python/qmp: clear events on get_events() call
  python/qmp: add send_fd_scm directly to QEMUMonitorProtocol
  python, iotests: remove socket_scm_helper

 tests/qemu-iotests/socket_scm_helper.c | 136 -
 python/qemu/aqmp/__init__.py   |   4 +
 python/qemu/aqmp/events.py |  15 ++-
 python/qemu/aqmp/models.py |  13 +++
 python/qemu/aqmp/protocol.py   |   7 +-
 python/qemu/aqmp/qmp_client.py |  27 +
 python/qemu/machine/machine.py |  48 ++---
 python/qemu/machine/qtest.py   |   2 -
 python/qemu/qmp/__init__.py|  27 +++--
 python/qemu/qmp/qmp_shell.py   |   1 -
 tests/Makefile.include |   1 -
 tests/meson.build  |   4 -
 tests/qemu-iotests/iotests.py  |   3 -
 tests/qemu-iotests/meson.build |   5 -
 tests/qemu-iotests/testenv.py  |   8 +-
 15 files changed, 85 insertions(+), 216 deletions(-)
 delete mode 100644 tests/qemu-iotests/socket_scm_helper.c
 delete mode 100644 tests/qemu-iotests/meson.build

-- 
2.31.1

Re: [PATCH v2 13/17] iotests: Accommodate async QMP Exception classes

[John Snow]

On Tue, Oct 12, 2021 at 12:06 PM Hanna Reitz  wrote:

> On 23.09.21 02:49, John Snow wrote:
> > (But continue to support the old ones for now, too.)
> >
> > There are very few cases of any user of QEMUMachine or a subclass
> > thereof relying on a QMP Exception type. If you'd like to check for
> > yourself, you want to grep for all of the derivatives of QMPError,
> > excluding 'AQMPError' and its derivatives. That'd be these:
> >
> > - QMPError
> > - QMPConnectError
> > - QMPCapabilitiesError
> > - QMPTimeoutError
> > - QMPProtocolError
> > - QMPResponseError
> > - QMPBadPortError
> >
> >
> > Signed-off-by: John Snow 
> > ---
> >   scripts/simplebench/bench_block_job.py| 3 ++-
> >   tests/qemu-iotests/tests/mirror-top-perms | 3 ++-
> >   2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/simplebench/bench_block_job.py
> b/scripts/simplebench/bench_block_job.py
> > index 4f03c121697..a403c35b08f 100755
> > --- a/scripts/simplebench/bench_block_job.py
> > +++ b/scripts/simplebench/bench_block_job.py
> > @@ -28,6 +28,7 @@
> >   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
> 'python'))
> >   from qemu.machine import QEMUMachine
> >   from qemu.qmp import QMPConnectError
> > +from qemu.aqmp import ConnectError
> >
> >
> >   def bench_block_job(cmd, cmd_args, qemu_args):
> > @@ -49,7 +50,7 @@ def bench_block_job(cmd, cmd_args, qemu_args):
> >   vm.launch()
> >   except OSError as e:
> >   return {'error': 'popen failed: ' + str(e)}
> > -except (QMPConnectError, socket.timeout):
> > +except (QMPConnectError, ConnectError, socket.timeout):
> >   return {'error': 'qemu failed: ' + str(vm.get_log())}
> >
> >   try:
> > diff --git a/tests/qemu-iotests/tests/mirror-top-perms
> b/tests/qemu-iotests/tests/mirror-top-perms
> > index 2fc8dd66e0a..9fe315e3b01 100755
> > --- a/tests/qemu-iotests/tests/mirror-top-perms
> > +++ b/tests/qemu-iotests/tests/mirror-top-perms
> > @@ -26,6 +26,7 @@ from iotests import qemu_img
> >   # Import qemu after iotests.py has amended sys.path
> >   # pylint: disable=wrong-import-order
> >   import qemu
> > +from qemu.aqmp import ConnectError
>
> With this change, the test emits the “AQMP is in development” warning,
> breaking the test.  Do we want to pull patch 16 before this patch?
>
>
Oh, good spot, I hadn't considered this.

Uh, yeah, I'll have to front-load the other patch.


> (I also wonder whether we want to import QMPConnectError, too, because
> the `except (qemu.qmp.*, *)` below looks so... heterogeneous.)
>

Will do.

(I don't think I've ever had code critiqued as "heterogeneous" before !)

>
> Hanna
>
> >   image_size = 1 * 1024 * 1024
> > @@ -102,7 +103,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
> >   self.vm_b.launch()
> >   print('ERROR: VM B launched successfully, this should not
> have '
> > 'happened')
> > -except qemu.qmp.QMPConnectError:
> > +except (qemu.qmp.QMPConnectError, ConnectError):
> >   assert 'Is another process using the image' in
> self.vm_b.get_log()
> >
> >   result = self.vm.qmp('block-job-cancel',
>
>

Re: [PATCH v2 12/17] python/machine: Handle QMP errors on close more meticulously

[John Snow]

On Wed, Sep 22, 2021 at 8:50 PM John Snow  wrote:

> To use the AQMP backend, Machine just needs to be a little more diligent
> about what happens when closing a QMP connection. The operation is no
> longer a freebie in the async world; it may return errors encountered in
> the async bottom half on incoming message receipt, etc.
>
> (AQMP's disconnect, ultimately, serves as the quiescence point where all
> async contexts are gathered together, and any final errors reported at
> that point.)
>
> Because async QMP continues to check for messages asynchronously, it's
> almost certainly likely that the loop will have exited due to EOF after
> issuing the last 'quit' command. That error will ultimately be bubbled
> up when attempting to close the QMP connection. The manager class here
> then is free to discard it -- if it was expected.
>
> Signed-off-by: John Snow 
>
> ---
>
> Yes, I regret that this class has become quite a dumping ground for
> complexity around the exit path. It's in need of a refactor to help
> separate out the exception handling and cleanup mechanisms from the
> VM-related stuff, but it's not a priority to do that just yet -- but
> it's on the list.
>
> Signed-off-by: John Snow 
> ---
>  python/qemu/machine/machine.py | 48 +-
>  1 file changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/python/qemu/machine/machine.py
> b/python/qemu/machine/machine.py
> index 0bd40bc2f76..c33a78a2d9f 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -342,9 +342,15 @@ def _post_shutdown(self) -> None:
>  # Comprehensive reset for the failed launch case:
>  self._early_cleanup()
>
> -if self._qmp_connection:
> -self._qmp.close()
> -self._qmp_connection = None
> +try:
> +self._close_qmp_connection()
> +except Exception as err:  # pylint: disable=broad-except
> +LOG.warning(
> +"Exception closing QMP connection: %s",
> +str(err) if str(err) else type(err).__name__
> +)
> +finally:
> +assert self._qmp_connection is None
>
>  self._close_qemu_log_file()
>
> @@ -420,6 +426,31 @@ def _launch(self) -> None:
> close_fds=False)
>  self._post_launch()
>
> +def _close_qmp_connection(self) -> None:
> +"""
> +Close the underlying QMP connection, if any.
> +
> +Dutifully report errors that occurred while closing, but assume
> +that any error encountered indicates an abnormal termination
> +process and not a failure to close.
> +"""
> +if self._qmp_connection is None:
> +return
> +
> +try:
> +self._qmp.close()
> +except EOFError:
> +# EOF can occur as an Exception here when using the Async
> +# QMP backend. It indicates that the server closed the
> +# stream. If we successfully issued 'quit' at any point,
> +# then this was expected. If the remote went away without
> +# our permission, it's worth reporting that as an abnormal
> +# shutdown case.
> +if not self._quit_issued:
>

I strongly suspect that this line needs to actually be "if not
(self._user_killed or self._quit_issued):" to also handle the force-kill
cases.

I wrote a different sync compatibility layer in a yet-to-be-published
branch and noticed this code still allows EOFError through. Why it did not
seem to come up in *this* series I still don't know -- I think the timing
of when exactly the coroutines get scheduled can be finnicky depending on
what else is running. So, sometimes, we manage to close the loop before we
get EOF and sometimes we don't.

I am mulling over adding a "tolerate_eof: bool = False" argument to
disconnect() to allow the caller to handle this stuff with a little less
boilerplate. Anyone have strong feelings on that?

(Ultimately, because there's no canonical "goodbye" in-band message, the
QMP layer itself can never really know if EOF was expected or not -- that's
really up to whomever is managing the connection, but it does add a layer
of complexity around the exit path that I am starting to find is a
nuisance. Not to mention that there are likely many possible cases in which
we might expect the remote to disappear on us that don't involve using QMP
at all -- kill is one, using the guest agent to coerce the guest into
shutdown is another. Networking and migration are other theoretical(?)
avenues for intended disconnects.)


> +raise
> +

Re: [PATCH v2 12/17] python/machine: Handle QMP errors on close more meticulously

[John Snow]

On Thu, Oct 7, 2021 at 11:08 AM Eric Blake  wrote:

> On Wed, Sep 22, 2021 at 08:49:33PM -0400, John Snow wrote:
> > To use the AQMP backend, Machine just needs to be a little more diligent
> > about what happens when closing a QMP connection. The operation is no
> > longer a freebie in the async world; it may return errors encountered in
> > the async bottom half on incoming message receipt, etc.
> >
> > (AQMP's disconnect, ultimately, serves as the quiescence point where all
> > async contexts are gathered together, and any final errors reported at
> > that point.)
> >
> > Because async QMP continues to check for messages asynchronously, it's
> > almost certainly likely that the loop will have exited due to EOF after
> > issuing the last 'quit' command. That error will ultimately be bubbled
> > up when attempting to close the QMP connection. The manager class here
> > then is free to discard it -- if it was expected.
> >
> > Signed-off-by: John Snow 
> >
> > ---
> >
> > Yes, I regret that this class has become quite a dumping ground for
> > complexity around the exit path. It's in need of a refactor to help
> > separate out the exception handling and cleanup mechanisms from the
> > VM-related stuff, but it's not a priority to do that just yet -- but
> > it's on the list.
> >
> > Signed-off-by: John Snow 
>
> This second S-o-b won't matter.
>
> Reviewed-by: Eric Blake 
>

Sorry, it's a bug with git-publish I've just not invested time into fixing.
It happens when I use a '---' to add an additional note for reviewers.
git-publish likes to add the second line because it doesn't "see" the first
one anymore. It's harmless, ultimately, but ... it sure does make me look
like I have no idea what I'm doing ;)

--js

Re: [PATCH v2 04/17] python/aqmp: add send_fd_scm

[John Snow]

On Thu, Oct 7, 2021 at 10:52 AM Eric Blake  wrote:

> On Wed, Sep 22, 2021 at 08:49:25PM -0400, John Snow wrote:
> > The single space is indeed required to successfully transmit the file
> > descriptor to QEMU.
>
> Sending fds requires a payload of at least one byte, but I don't think
> that qemu cares which byte.  Thus, while your choice of space is fine,
> the commit message may be a bit misleading at implying it must be
> space.
>
>
OK, I'll rephrase. (Space winds up being useful in particular because it
doesn't mess with the parsing for subsequent JSON objects sent over the
wire.)

(Idle curiosity: Is it possible to make QEMU accept an empty payload here?
I understand that for compatibility reasons it wouldn't change much for the
python lib even if we did, but I'm curious.)


> >
> > Python 3.11 removes support for calling sendmsg directly from a
> > transport's socket. There is no other interface for doing this, our use
> > case is, I suspect, "quite unique".
> >
> > As far as I can tell, this is safe to do -- send_fd_scm is a synchronous
> > function and we can be guaranteed that the async coroutines will *not* be
> > running when it is invoked. In testing, it works correctly.
> >
> > I investigated quite thoroughly the possibility of creating my own
> > asyncio Transport (The class that ultimately manages the raw socket
> > object) so that I could manage the socket myself, but this is so wildly
> > invasive and unportable I scrapped the idea. It would involve a lot of
> > copy-pasting of various python utilities and classes just to re-create
> > the same infrastructure, and for extremely little benefit. Nah.
> >
> > Just boldly void the warranty instead, while I try to follow up on
> > https://bugs.python.org/issue43232
>
> Bummer that we have to do that, but at least you are documenting the
> problems and pursuing a remedy upstream.
>
>
Yeah. I suspect our use case is so niche that it's not likely to get
traction, but I'll try again. This sort of thing might make it harder to
use projects like pypy, so it does feel like a defeat. Still, where there's
a will, there's a way, right? :)

--js

Re: [PATCH v2 15/17] python/aqmp: Create sync QMP wrapper for iotests

[John Snow]

On Wed, Oct 6, 2021 at 10:32 AM Paolo Bonzini  wrote:

> On 06/10/21 16:24, John Snow wrote:
> >
> > I had plans at one point to make a sync.py, but with an interface that
> > matched async QMP itself more closely. I spent some time trying to
> > research how to make a "magic" sync wrapper around async QMP, and hit a
> > few trouble spots. I've still got the patch, but I felt some pressure to
> > try and switch iotests over as fast as possible to get more
> > trial-by-fire time this release cycle. I named them "sync.py" and
> > "legacy.py" in my branch accordingly. Of course, I made a beeline
> > straight for the iotests version, so now it looks odd. I may yet try to
> > clean up the other version, possibly converting legacy.py to work in
> > terms of sync.py, and then converting users in iotests so that I can
> > drop legacy.py.
>
> Got it.  So maybe aqmp.qmp_next or aqmp.qmp_experimental?  What I mean
> is, it's all but legacy. :)
>
>
Mmm, yeah I guess I meant "legacy interface" and not "legacy
implementation" ;)

I could do 'compat.py' for the iotests-compatible interface and 'sync.py'
for the "modern" one?

--js

Re: [PATCH v2 00/17] Switch iotests to using Async QMP

[John Snow]

On Wed, Oct 6, 2021 at 6:14 AM Paolo Bonzini  wrote:

> On 23/09/21 02:49, John Snow wrote:
> > Based-on: <20210915162955.333025-1-js...@redhat.com>
> >[PATCH v4 00/27] python: introduce Asynchronous QMP package
> > GitLab:
> https://gitlab.com/jsnow/qemu/-/commits/python-aqmp-iotest-wrapper
> > CI: https://gitlab.com/jsnow/qemu/-/pipelines/375637927
> >
> > Hiya,
> >
> > This series continues where the first AQMP series left off and adds a
> > synchronous 'legacy' wrapper around the new AQMP interface, then drops
> > it straight into iotests to prove that AQMP is functional and totally
> > cool and fine. The disruption and churn to iotests is extremely minimal.
> > (There's actually a net negative SLOC in tests/qemu-iotests.)
> >
> > In the event that a regression happens and I am not physically proximate
> > to inflict damage upon, one may set the QEMU_PYTHON_LEGACY_QMP variable
> > to any non-empty string as it pleases you to engage the QMP machinery
> > you are used to.
> >
> > I'd like to try and get this committed early in the 6.2 development
> > cycle to give ample time to smooth over any possible regressions. I've
> > tested it locally and via gitlab CI, across Python versions 3.6 through
> > 3.10, and "worksforme". If something bad happens, we can revert the
> > actual switch-flip very trivially.
> >
> > Layout:
> >
> > Patches 1-7: ./python/qemu/aqmp changes that serve as pre-requisites.
> > Patches 8-12: other ./python changes that ease the transition.
> > Patches 13-14: iotest changes to support the new QMP backend.
> > Patches 15-17: Make the switch.
> >
> > V2:
> >
> > 001/17:[] [--] 'python/aqmp: add greeting property to QMPClient'
> > 002/17:[] [--] 'python/aqmp: add .empty() method to EventListener'
> > 003/17:[] [--] 'python/aqmp: Return cleared events from
> EventListener.clear()'
> > 004/17:[0007] [FC] 'python/aqmp: add send_fd_scm'
> > 005/17:[down] 'python/aqmp: Add dict conversion method to Greeting
> object'
> > 006/17:[down] 'python/aqmp: Reduce severity of EOFError-caused loop
> terminations'
> > 007/17:[down] 'python/aqmp: Disable logging messages by default'
> >
> > 008/17:[0002] [FC] 'python/qmp: clear events on get_events() call'
> > 009/17:[] [--] 'python/qmp: add send_fd_scm directly to
> QEMUMonitorProtocol'
> > 010/17:[] [--] 'python, iotests: remove socket_scm_helper'
> > 011/17:[0013] [FC] 'python/machine: remove has_quit argument'
> > 012/17:[down] 'python/machine: Handle QMP errors on close more
> meticulously'
> >
> > 013/17:[0009] [FC] 'iotests: Accommodate async QMP Exception classes'
> > 014/17:[down] 'iotests: Conditionally silence certain AQMP errors'
> >
> > 015/17:[0016] [FC] 'python/aqmp: Create sync QMP wrapper for iotests'
> > 016/17:[0002] [FC] 'python/aqmp: Remove scary message'
> > 017/17:[] [--] 'python, iotests: replace qmp with aqmp'
> >
> > - Rebased on jsnow/python, which was recently rebased on origin/master.
> > - Make aqmp's send_fd_scm method bark if the socket isn't AF_UNIX (Hanna)
> > - Uh... modify send_fd_scm so it doesn't break when Python 3.11 comes
> out ...
> >See the commit message for more detail.
> > - Drop the "python/aqmp: Create MessageModel and StandaloneModel class"
> >patch and replace with a far simpler method that just adds an
> >_asdict() method.
> > - Add patches 06 and 07 to change how the AQMP library handles logging.
> > - Adjust docstring in patch 08 (Hanna)
> > - Rename "_has_quit" attribute to "_quid_issued" (Hanna)
> > - Renamed patch 12, simplified the logic in _soft_shutdown a tiny bit.
> > - Fixed bad exception handling logic in 13 (Hanna)
> > - Introduce a helper in patch 14 to silence log output when it's
> unwanted.
> > - Small addition of _get_greeting() helper in patch 15, coinciding with
> the
> >new patch 05 here.
> > - Contextual changes in 16.
> >
> > John Snow (17):
> >python/aqmp: add greeting property to QMPClient
> >python/aqmp: add .empty() method to EventListener
> >python/aqmp: Return cleared events from EventListener.clear()
> >python/aqmp: add send_fd_scm
> >python/aqmp: Add dict conversion method to Greeting object
> >python/aqmp: Reduce severity of EOFError-caused loop terminations
> >python/aqmp: Disable logging messages by default
> >python/qmp: clear events on get_events() call
> >python/qmp: add send_fd_scm directly to QEMUMonitorProtocol
> >  

Re: [PATCH v2 15/17] python/aqmp: Create sync QMP wrapper for iotests

[John Snow]

On Wed, Oct 6, 2021 at 6:13 AM Paolo Bonzini  wrote:

> On 23/09/21 02:49, John Snow wrote:
> > This is a wrapper around the async QMPClient that mimics the old,
> > synchronous QEMUMonitorProtocol class. It is designed to be
> > interchangeable with the old implementation.
> >
> > It does not, however, attempt to mimic Exception compatibility.
> >
> > Signed-off-by: John Snow 
> > Acked-by: Hanna Reitz 
>
> I don't like the name (of course).  qemu-iotests shows that there is a
> use for sync wrappers so, with similar reasoning to patch 16, there's no
> need to scare people away.  Why not just qemu.aqmp.sync?
>
> Paolo
>
>
I had plans at one point to make a sync.py, but with an interface that
matched async QMP itself more closely. I spent some time trying to research
how to make a "magic" sync wrapper around async QMP, and hit a few trouble
spots. I've still got the patch, but I felt some pressure to try and switch
iotests over as fast as possible to get more trial-by-fire time this
release cycle. I named them "sync.py" and "legacy.py" in my branch
accordingly. Of course, I made a beeline straight for the iotests version,
so now it looks odd. I may yet try to clean up the other version, possibly
converting legacy.py to work in terms of sync.py, and then converting users
in iotests so that I can drop legacy.py.

(Mabe. I am not heavily committed to any one particular approach here,
other than being very motivated to get AQMP tested widely a good bit before
rc0 to have a chance to smooth over any lingering problems that might
exist.)

Thanks for taking a look! I am more than happy to stage 1-9 myself, but I
will wait for Hanna's approval on 10-14 in particular.

--js

Re: [PATCH 12/15] iotests: Disable AQMP logging under non-debug modes

[John Snow]

On Mon, Oct 4, 2021 at 2:32 PM John Snow  wrote:

>
>
> On Mon, Oct 4, 2021 at 6:12 AM Hanna Reitz  wrote:
>
>> On 18.09.21 04:14, John Snow wrote:
>> >
>> >
>> > On Fri, Sep 17, 2021 at 8:58 PM John Snow > > <mailto:js...@redhat.com>> wrote:
>> >
>> >
>> >
>> > On Fri, Sep 17, 2021 at 10:30 AM Hanna Reitz > > <mailto:hre...@redhat.com>> wrote:
>> >
>> > On 17.09.21 07:40, John Snow wrote:
>> > > Disable the aqmp logger, which likes to (at the moment)
>> > print out
>> > > intermediate warnings and errors that cause session
>> > termination; disable
>> > > them so they don't interfere with the job output.
>> > >
>> > > Leave any "CRITICAL" warnings enabled though, those are ones
>> > that we
>> > > should never see, no matter what.
>> >
>> > I mean, looks OK to me, but from what I understand (i.e.
>> little),
>> > qmp_client doesn’t log CRITICAL messages, at least I can’t see
>> > any. Only
>> > ERRORs.
>> >
>> >
>> > There's *one* critical message in protocol.py, used for a
>> > circumstance that I *think* should be impossible. I do not think I
>> > currently use any WARNING level statements.
>> >
>> > I guess I’m missing some CRITICAL messages in external
>> > functions called
>> > from qmp_client.py, but shouldn’t we still keep ERRORs?
>> >
>> >
>> > ...Mayybe?
>> >
>> > The errors logged by AQMP are *almost always* raised as Exceptions
>> > somewhere else, eventually. Sometimes when we encounter them in
>> > one context, we need to save them and then re-raise them in a
>> > different execution context. There's one good exception to this:
>> > My pal, EOFError.
>> >
>> > If the reader context encounters EOF, it raises EOFError and this
>> > causes a disconnect to be scheduled asynchronously. *Any*
>> > Exception that causes a disconnect to be scheduled asynchronously
>> > is dutifully logged as an ERROR. At this point in the code, we
>> > don't really know if the user of the library considers this an
>> > "error" yet or not. I've waffled a lot on how exactly to treat
>> > this circumstance. ...Hm, I guess that's really the only case
>> > where I have an error that really ought to be suppressed. I
>> > suppose what I will do here is: if the exception happens to be an
>> > EOFError I will drop the severity of the log message down to INFO.
>> > I don't know why it takes being challenged on this stuff to start
>> > thinking clearly about it, but here we are. Thank you for your
>> > feedback :~)
>> >
>> > --js
>> >
>> >
>> > Oh, CI testing reminds me of why I am a liar here.
>> >
>> > the mirror-top-perms test intentionally expects not to be able to
>> > connect, but we're treated to these two additional lines of output:
>> >
>> > +ERROR:qemu.aqmp.qmp_client.qemub-2536319:Negotiation failed: EOFError
>> > +ERROR:qemu.aqmp.qmp_client.qemub-2536319:Failed to establish session:
>> > EOFError
>> >
>> > Uh. I guess a temporary suppression in mirror-top-perms, then ...?
>>
>> Sounds right to me, if that’s simple enough.
>>
>> (By the way, I understand it right that you want to lower the severity
>> of EOFErrors to INFO only on disconnect, right?  Which is why they’re
>> still logged as ERRORs here, because they aren’t occurring on
>> disconnects?)
>>
>>
> More or less, yeah.
>
> When an EOFError causes the reader coroutine to halt (because it can't
> read the next message), I decided (in v2) to drop that one particular
> logging message down to "INFO", because it might -- or might not be -- an
> expected occurrence from the point of view of whoever is managing the QMP
> connection. Maybe it was expected (The test used qemu-guest-agent or
> something else to make the guest shutdown, taking QEMU down with it without
> the knowledge of the QMP library layer) or maybe it was unexpected (the QMP
> remote really just disappeared from us on a whim). There's no way to know,
> so it probably isn't right to consider it an error.
>
> In the connection case, I left it as an ERROR beca

[PATCH 12/13] python: Add iotest linters to test suite

[John Snow]

Run mypy and pylint on the iotests files directly from the Python CI
test infrastructure. This ensures that any accidental breakages to the
qemu.[qmp|aqmp|machine|utils] packages will be caught by that test
suite.

It also ensures that these linters are run with well-known versions and
test against a wide variety of python versions, which helps to find
accidental cross-version python compatibility issues.

Signed-off-by: John Snow 
---
 python/tests/iotests-mypy.sh   | 4 
 python/tests/iotests-pylint.sh | 4 
 2 files changed, 8 insertions(+)
 create mode 100755 python/tests/iotests-mypy.sh
 create mode 100755 python/tests/iotests-pylint.sh

diff --git a/python/tests/iotests-mypy.sh b/python/tests/iotests-mypy.sh
new file mode 100755
index 000..ee764708199
--- /dev/null
+++ b/python/tests/iotests-mypy.sh
@@ -0,0 +1,4 @@
+#!/bin/sh -e
+
+cd ../tests/qemu-iotests/
+python3 -m linters --mypy
diff --git a/python/tests/iotests-pylint.sh b/python/tests/iotests-pylint.sh
new file mode 100755
index 000..4cae03424b4
--- /dev/null
+++ b/python/tests/iotests-pylint.sh
@@ -0,0 +1,4 @@
+#!/bin/sh -e
+
+cd ../tests/qemu-iotests/
+python3 -m linters --pylint
-- 
2.31.1

[PATCH 08/13] iotests/297: refactor run_[mypy|pylint] as generic execution shim

[John Snow]

There's virtually nothing special here anymore; we can combine these
into a single, rather generic function.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 46 +++---
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 91029dbb34e..4c54dd39b46 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -61,29 +61,33 @@ def get_test_files() -> List[str]:
 return list(filter(is_python_file, check_tests))
 
 
-def run_pylint(
-files: List[str],
-env: Optional[Mapping[str, str]] = None,
-) -> None:
+def run_linter(
+tool: str,
+args: List[str],
+env: Optional[Mapping[str, str]] = None,
+suppress_output: bool = False,
+) -> int:
+"""
+Run a python-based linting tool.
 
-subprocess.run(('python3', '-m', 'pylint', *files),
-   env=env, check=False)
+If suppress_output is True, capture stdout/stderr of the child
+process and only print that information back to stdout if the child
+process's return code was non-zero.
+"""
+p = subprocess.run(
+('python3', '-m', tool, *args),
+env=env,
+check=False,
+stdout=subprocess.PIPE if suppress_output else None,
+stderr=subprocess.STDOUT if suppress_output else None,
+universal_newlines=True,
+)
 
-
-def run_mypy(
-files: List[str],
-env: Optional[Mapping[str, str]] = None,
-) -> None:
-p = subprocess.run((('python3', '-m', 'mypy', *files),
-   env=env,
-   check=False,
-   stdout=subprocess.PIPE,
-   stderr=subprocess.STDOUT,
-   universal_newlines=True)
-
-if p.returncode != 0:
+if suppress_output and p.returncode != 0:
 print(p.stdout)
 
+return p.returncode
+
 
 def main() -> None:
 for linter in ('pylint-3', 'mypy'):
@@ -100,11 +104,11 @@ def main() -> None:
 
 print('=== pylint ===')
 sys.stdout.flush()
-run_pylint(files, env=env)
+run_linter('pylint', files, env=env)
 
 print('=== mypy ===')
 sys.stdout.flush()
-run_mypy(files, env=env)
+run_linter('mypy', files, env=env, suppress_output=True)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH 06/13] iotests/297: Separate environment setup from test execution

[John Snow]

Move environment setup into main(), leaving pure test execution behind
in run_linters().

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 23 ++-
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index f9fcb039e27..fcbab0631be 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,7 +21,7 @@ import re
 import shutil
 import subprocess
 import sys
-from typing import List
+from typing import List, Mapping, Optional
 
 import iotests
 
@@ -61,23 +61,20 @@ def get_test_files() -> List[str]:
 return list(filter(is_python_file, check_tests))
 
 
-def run_linters():
-files = get_test_files()
-
-iotests.logger.debug('Files to be checked:')
-iotests.logger.debug(', '.join(sorted(files)))
+def run_linters(
+files: List[str],
+env: Optional[Mapping[str, str]] = None,
+) -> None:
 
 print('=== pylint ===')
 sys.stdout.flush()
 
-env = os.environ.copy()
 subprocess.run(('python3', '-m', 'pylint', *files),
env=env, check=False)
 
 print('=== mypy ===')
 sys.stdout.flush()
 
-env['MYPYPATH'] = env['PYTHONPATH']
 p = subprocess.run((('python3', '-m', 'mypy', *files),
env=env,
check=False,
@@ -94,7 +91,15 @@ def main() -> None:
 if shutil.which(linter) is None:
 iotests.notrun(f'{linter} not found')
 
-run_linters()
+files = get_test_files()
+
+iotests.logger.debug('Files to be checked:')
+iotests.logger.debug(', '.join(sorted(files)))
+
+env = os.environ.copy()
+env['MYPYPATH'] = env['PYTHONPATH']
+
+run_linters(files, env=env)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH 10/13] iotests/linters: Add entry point for linting via Python CI

[John Snow]

We need at least a tiny little shim here to join test file discovery
with test invocation. This logic could conceivably be hosted somewhere
in python/, but I felt it was strictly the least-rude thing to keep the
test logic here in iotests/, even if this small function isn't itself an
iotest.

Note that we don't actually even need the executable bit here, we'll be
relying on the ability to run this module as a script using Python CLI
arguments. No chance it gets misunderstood as an actual iotest that way.

(It's named, not in tests/, doesn't have the execute bit, and doesn't
have an execution shebang.)

Signed-off-by: John Snow 

---

(1) I think that the test file discovery logic and skip list belong together,
and that those items belong in iotests/. I think they also belong in
whichever directory pylintrc and mypy.ini are in, also in iotests/.

(2) Moving this logic into python/tests/ is challenging because I'd have
to import iotests code from elsewhere in the source tree, which just
inverts an existing problem I have been trying to rid us of --
needing to muck around with PYTHONPATH or sys.path hacking in python
scripts. I'm keen to avoid this.

(3) If we moved all python tests into tests/ and gave them *.py
extensions, we wouldn't even need the test discovery functions
anymore, and all of linters.py could be removed entirely, including
this execution shim. We could rely on mypy/pylint's own file
discovery mechanisms at that point. More work than I'm up for with
just this series, but I could be coaxed into doing it if there was
some promise of not rejecting all that busywork ;)

Signed-off-by: John Snow 
---
 tests/qemu-iotests/linters.py | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index f6a2dc139fd..191df173064 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -16,6 +16,7 @@
 import os
 import re
 import subprocess
+import sys
 from typing import List, Mapping, Optional
 
 
@@ -81,3 +82,20 @@ def run_linter(
 
 return p.returncode
 
+
+def main() -> int:
+"""
+Used by the Python CI system as an entry point to run these linters.
+"""
+files = get_test_files()
+
+if sys.argv[1] == '--pylint':
+return run_linter('pylint', files)
+elif sys.argv[1] == '--mypy':
+return run_linter('mypy', files)
+
+raise ValueError(f"Unrecognized argument(s): '{sys.argv[1:]}'")
+
+
+if __name__ == '__main__':
+sys.exit(main())
-- 
2.31.1

[PATCH 13/13] iotests: [RFC] drop iotest 297

[John Snow]

(This is highlighting a what-if, which might make it clear why any
special infrastructure is still required at all. It's not intended to
actually be merged at this step -- running JUST the iotest linters from
e.g. 'make check' is not yet accommodated, so there's no suitable
replacement for 297 for block test authors.)

Drop 297. As a consequence, we no longer need to pass an environment
variable to the mypy/pylint invocations, so that can be dropped. We also
now no longer need to hide output-except-on-error, so that can be
dropped as well.

The only thing that necessitates any special running logic anymore is
the skip list and the python-test-detection code. Without those, we
could easily codify the tests as simply:

[pylint|mypy] *.py tests/*.py

... and drop this entire file. We're not quite there yet, though.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297| 52 ---
 tests/qemu-iotests/297.out|  2 --
 tests/qemu-iotests/linters.py | 20 ++
 3 files changed, 2 insertions(+), 72 deletions(-)
 delete mode 100755 tests/qemu-iotests/297
 delete mode 100644 tests/qemu-iotests/297.out

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
deleted file mode 100755
index f79c80216bf..000
--- a/tests/qemu-iotests/297
+++ /dev/null
@@ -1,52 +0,0 @@
-#!/usr/bin/env python3
-# group: meta
-#
-# Copyright (C) 2020 Red Hat, Inc.
-#
-# This program is free software; you can redistribute it and/or modify
-# it under the terms of the GNU General Public License as published by
-# the Free Software Foundation; either version 2 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-import os
-import shutil
-import sys
-
-import iotests
-import linters
-
-
-# Looking for the list of files to exclude from linting? See linters.py.
-
-
-def main() -> None:
-for linter in ('pylint-3', 'mypy'):
-if shutil.which(linter) is None:
-iotests.notrun(f'{linter} not found')
-
-files = linters.get_test_files()
-
-iotests.logger.debug('Files to be checked:')
-iotests.logger.debug(', '.join(sorted(files)))
-
-env = os.environ.copy()
-env['MYPYPATH'] = env['PYTHONPATH']
-
-print('=== pylint ===')
-sys.stdout.flush()
-linters.run_linter('pylint', files, env=env)
-
-print('=== mypy ===')
-sys.stdout.flush()
-linters.run_linter('mypy', files, env=env, suppress_output=True)
-
-
-iotests.script_main(main)
diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
deleted file mode 100644
index f2e1314d104..000
--- a/tests/qemu-iotests/297.out
+++ /dev/null
@@ -1,2 +0,0 @@
-=== pylint ===
-=== mypy ===
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index 83fcc5a960c..ca90604d8d9 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -17,7 +17,7 @@
 import re
 import subprocess
 import sys
-from typing import List, Mapping, Optional
+from typing import List
 
 
 # TODO: Empty this list!
@@ -55,31 +55,15 @@ def get_test_files() -> List[str]:
 return list(filter(is_python_file, check_tests))
 
 
-def run_linter(
-tool: str,
-args: List[str],
-env: Optional[Mapping[str, str]] = None,
-suppress_output: bool = False,
-) -> int:
+def run_linter(tool: str, args: List[str]) -> int:
 """
 Run a python-based linting tool.
-
-If suppress_output is True, capture stdout/stderr of the child
-process and only print that information back to stdout if the child
-process's return code was non-zero.
 """
 p = subprocess.run(
 ('python3', '-m', tool, *args),
-env=env,
 check=False,
-stdout=subprocess.PIPE if suppress_output else None,
-stderr=subprocess.STDOUT if suppress_output else None,
-universal_newlines=True,
 )
 
-if suppress_output and p.returncode != 0:
-print(p.stdout)
-
 return p.returncode
 
 
-- 
2.31.1

[PATCH 11/13] iotests/linters: Add workaround for mypy bug #9852

[John Snow]

This one is insidious: if you write an import as "from {namespace}
import {subpackage}" as mirror-top-perms (now) does, mypy will fail on
every-other invocation *if* the package being imported is a typed,
installed, namespace-scoped package.

Upsettingly, that's exactly what 'qemu.[aqmp|qmp|machine]' et al are in
the context of Python CI tests.

Now, I could just edit mirror-top-perms to avoid this invocation, but
since I tripped on a landmine, I might as well head it off at the pass
and make sure nobody else trips on that same landmine.

It seems to have something to do with the order in which files are
checked as well, meaning the random order in which set(os.listdir())
produces the list of files to test will cause problems intermittently
and not just strictly "every other run".

This will be fixed in mypy >= 0.920, which is not released yet. The
workaround for now is to disable incremental checking, which avoids the
issue.

Note: This workaround is not applied when running iotest 297 directly,
because the bug does not surface there! Given the nature of CI jobs not
starting with any stale cache to begin with, this really only has a
half-second impact on manual runs of the Python test suite when executed
directly by a developer on their local machine. The workaround may be
removed when the Python package requirements can stipulate mypy 0.920 or
higher, which can happen as soon as it is released. (Barring any
unforseen compatibility issues that 0.920 may bring with it.)


See also:
 https://github.com/python/mypy/issues/11010
 https://github.com/python/mypy/issues/9852

Signed-off-by: John Snow 
---
 tests/qemu-iotests/linters.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index 191df173064..83fcc5a960c 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -92,7 +92,9 @@ def main() -> int:
 if sys.argv[1] == '--pylint':
 return run_linter('pylint', files)
 elif sys.argv[1] == '--mypy':
-return run_linter('mypy', files)
+# mypy bug #9852; disable incremental checking as a workaround.
+args = ['--no-incremental'] + files
+return run_linter('mypy', args)
 
 raise ValueError(f"Unrecognized argument(s): '{sys.argv[1:]}'")
 
-- 
2.31.1

[PATCH 05/13] iotests/297: Create main() function

[John Snow]

Instead of running "run_linters" directly, create a main() function that
will be responsible for environment setup, leaving run_linters()
responsible only for execution of the linters.

(That environment setup will be moved over in forthcoming commits.)

Signed-off-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Hanna Reitz 
---
 tests/qemu-iotests/297 | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 65b1e7058c2..f9fcb039e27 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -89,8 +89,12 @@ def run_linters():
 print(p.stdout)
 
 
-for linter in ('pylint-3', 'mypy'):
-if shutil.which(linter) is None:
-iotests.notrun(f'{linter} not found')
+def main() -> None:
+for linter in ('pylint-3', 'mypy'):
+if shutil.which(linter) is None:
+iotests.notrun(f'{linter} not found')
 
-iotests.script_main(run_linters)
+run_linters()
+
+
+iotests.script_main(main)
-- 
2.31.1

[PATCH 07/13] iotests/297: Split run_linters apart into run_pylint and run_mypy

[John Snow]

Signed-off-by: John Snow 

---

Note, this patch really ought to be squashed with the next one, but I am
performing a move known as "Hedging my bets."
It's easier to squash than de-squash :)

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 19 ---
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index fcbab0631be..91029dbb34e 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -61,20 +61,19 @@ def get_test_files() -> List[str]:
 return list(filter(is_python_file, check_tests))
 
 
-def run_linters(
+def run_pylint(
 files: List[str],
 env: Optional[Mapping[str, str]] = None,
 ) -> None:
 
-print('=== pylint ===')
-sys.stdout.flush()
-
 subprocess.run(('python3', '-m', 'pylint', *files),
env=env, check=False)
 
-print('=== mypy ===')
-sys.stdout.flush()
 
+def run_mypy(
+files: List[str],
+env: Optional[Mapping[str, str]] = None,
+) -> None:
 p = subprocess.run((('python3', '-m', 'mypy', *files),
env=env,
check=False,
@@ -99,7 +98,13 @@ def main() -> None:
 env = os.environ.copy()
 env['MYPYPATH'] = env['PYTHONPATH']
 
-run_linters(files, env=env)
+print('=== pylint ===')
+sys.stdout.flush()
+run_pylint(files, env=env)
+
+print('=== mypy ===')
+sys.stdout.flush()
+run_mypy(files, env=env)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH 09/13] iotests: split linters.py out from 297

[John Snow]

Now, 297 is just the iotests-specific incantations and linters.py is as
minimal as I can think to make it. The only remaining element in here
that ought to be configuration and not code is the list of skip files,
but they're still numerous enough that repeating them for mypy and
pylint configurations both would be ... a hassle.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297| 72 +++---
 tests/qemu-iotests/linters.py | 83 +++
 2 files changed, 88 insertions(+), 67 deletions(-)
 create mode 100644 tests/qemu-iotests/linters.py

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 4c54dd39b46..f79c80216bf 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -17,76 +17,14 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-import re
 import shutil
-import subprocess
 import sys
-from typing import List, Mapping, Optional
 
 import iotests
+import linters
 
 
-# TODO: Empty this list!
-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', '194', '196', '202',
-'203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
-'218', '219', '224', '228', '234', '235', '236', '237', '238',
-'240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
-'262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
-'299', '302', '303', '304', '307',
-'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
-)
-
-
-def is_python_file(filename):
-if not os.path.isfile(filename):
-return False
-
-if filename.endswith('.py'):
-return True
-
-with open(filename, encoding='utf-8') as f:
-try:
-first_line = f.readline()
-return re.match('^#!.*python', first_line) is not None
-except UnicodeDecodeError:  # Ignore binary files
-return False
-
-
-def get_test_files() -> List[str]:
-named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
-check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
-return list(filter(is_python_file, check_tests))
-
-
-def run_linter(
-tool: str,
-args: List[str],
-env: Optional[Mapping[str, str]] = None,
-suppress_output: bool = False,
-) -> int:
-"""
-Run a python-based linting tool.
-
-If suppress_output is True, capture stdout/stderr of the child
-process and only print that information back to stdout if the child
-process's return code was non-zero.
-"""
-p = subprocess.run(
-('python3', '-m', tool, *args),
-env=env,
-check=False,
-stdout=subprocess.PIPE if suppress_output else None,
-stderr=subprocess.STDOUT if suppress_output else None,
-universal_newlines=True,
-)
-
-if suppress_output and p.returncode != 0:
-print(p.stdout)
-
-return p.returncode
+# Looking for the list of files to exclude from linting? See linters.py.
 
 
 def main() -> None:
@@ -94,7 +32,7 @@ def main() -> None:
 if shutil.which(linter) is None:
 iotests.notrun(f'{linter} not found')
 
-files = get_test_files()
+files = linters.get_test_files()
 
 iotests.logger.debug('Files to be checked:')
 iotests.logger.debug(', '.join(sorted(files)))
@@ -104,11 +42,11 @@ def main() -> None:
 
 print('=== pylint ===')
 sys.stdout.flush()
-run_linter('pylint', files, env=env)
+linters.run_linter('pylint', files, env=env)
 
 print('=== mypy ===')
 sys.stdout.flush()
-run_linter('mypy', files, env=env, suppress_output=True)
+linters.run_linter('mypy', files, env=env, suppress_output=True)
 
 
 iotests.script_main(main)
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
new file mode 100644
index 000..f6a2dc139fd
--- /dev/null
+++ b/tests/qemu-iotests/linters.py
@@ -0,0 +1,83 @@
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import os
+import re
+import subprocess
+from typing import List, Mapping, Optional
+
+
+# TODO: Empty this list!
+SKIP_FILES = (
+'030', '040', '041', '044', '

[PATCH 01/13] iotests/297: Move pylint config into pylintrc

[John Snow]

Move --score=n and --notes=XXX,FIXME into pylintrc. This pulls
configuration out of code, which I think is probably a good thing in
general.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297  |  4 +---
 tests/qemu-iotests/pylintrc | 16 
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 91ec34d9521..bc3a0ceb2aa 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -65,10 +65,8 @@ def run_linters():
 print('=== pylint ===')
 sys.stdout.flush()
 
-# Todo notes are fine, but fixme's or xxx's should probably just be
-# fixed (in tests, at least)
 env = os.environ.copy()
-subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
+subprocess.run(('pylint-3', *files),
env=env, check=False)
 
 print('=== mypy ===')
diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index 8cb4e1d6a6d..32ab77b8bb9 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -31,6 +31,22 @@ disable=invalid-name,
 too-many-statements,
 consider-using-f-string,
 
+
+[REPORTS]
+
+# Activate the evaluation score.
+score=no
+
+
+[MISCELLANEOUS]
+
+# List of note tags to take in consideration, separated by a comma.
+# TODO notes are fine, but FIXMEs or XXXs should probably just be
+# fixed (in tests, at least).
+notes=FIXME,
+  XXX,
+
+
 [FORMAT]
 
 # Maximum number of characters on a single line.
-- 
2.31.1

[PATCH 00/13] python/iotests: Run iotest linters during Python CI

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest-pt2
CI: https://gitlab.com/jsnow/qemu/-/pipelines/382320226
Based-on: <20210923180715.4168522-1-js...@redhat.com>
  [PATCH v2 0/6] iotests: update environment and linting configuration
  (Staged at kwolf/block sans patch #2, not needed here.)

Factor out pylint and mypy configuration from iotest 297 so that the
Python test infrastructure in python/ can also run these linters. This
will enable what is essentially iotest #297 to run via GitLab CI.

This series generally aims to split "linter configuration" out of code
so that both iotest #297 and the Python test suite can both invoke the
same linters (nearly) identically.

The differences occur where the Python entryway involves setting up a
virtual environment and installing linter packages pinned at specific
versions so that the CI test can be guaranteed to behave
deterministically.

iotest #297 is left as a way to run these tests as a convenience until I
can integrate environment setup and test execution as a part of 'make
check' or similar to serve as a replacement. This invocation just trusts
you've installed the right packages into the right places with the right
versions, as it always has.

V4:

- Some optimizations and touchups were included in 'PATCH v2 0/6]
  iotests: update environment and linting configuration' instead, upon
  which this series is now based.
- Rewrote most patches, being more aggressive about the factoring
  between "iotest" and "linter invocation". The patches are smaller now.
- Almost all configuration is split out into mypy.ini and pylintrc.
- Remove the PWD/CWD juggling that the previous versions added; it's not
  strictly needed for this integration. We can re-add it later if we
  wind up needing it for something.
- mypy and pylintrc tests are split into separate tests. The GitLab CI
  now lists them as two separate test cases, so it's easier to see what
  is failing and why. (And how long those tests take.) It is also now
  therefore possible to ask avocado to run just one or the other.
- mypy bug workaround is only applied strictly in cases where it is
  needed, optimizing speed of iotest 297.

V3:
 - Added patch 1 which has been submitted separately upstream,
   but was necessary for testing.
 - Rebased on top of hreitz/block, which fixed some linting issues.
 - Added a workaround for a rather nasty mypy bug ... >:(

V2:
 - Added patches 1-5 which do some more delinting.
 - Added patch 8, which scans subdirs for tests to lint.
 - Added patch 17, which improves the speed of mypy analysis.
 - Patch 14 is different because of the new patch 8.

John Snow (13):
  iotests/297: Move pylint config into pylintrc
  iotests/297: Split mypy configuration out into mypy.ini
  iotests/297: Add get_files() function
  iotests/297: Don't rely on distro-specific linter binaries
  iotests/297: Create main() function
  iotests/297: Separate environment setup from test execution
  iotests/297: Split run_linters apart into run_pylint and run_mypy
  iotests/297: refactor run_[mypy|pylint] as generic execution shim
  iotests: split linters.py out from 297
  iotests/linters: Add entry point for linting via Python CI
  iotests/linters: Add workaround for mypy bug #9852
  python: Add iotest linters to test suite
  iotests: [RFC] drop iotest 297

 python/tests/iotests-mypy.sh   |  4 ++
 python/tests/iotests-pylint.sh |  4 ++
 tests/qemu-iotests/297.out |  2 -
 tests/qemu-iotests/{297 => linters.py} | 88 ++
 tests/qemu-iotests/mypy.ini| 12 
 tests/qemu-iotests/pylintrc| 16 +
 6 files changed, 71 insertions(+), 55 deletions(-)
 create mode 100755 python/tests/iotests-mypy.sh
 create mode 100755 python/tests/iotests-pylint.sh
 delete mode 100644 tests/qemu-iotests/297.out
 rename tests/qemu-iotests/{297 => linters.py} (52%)
 mode change 100755 => 100644
 create mode 100644 tests/qemu-iotests/mypy.ini

-- 
2.31.1

[PATCH 04/13] iotests/297: Don't rely on distro-specific linter binaries

[John Snow]

'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or "python3 -m
mypy" to access these scripts instead. This style of invocation will
prefer the "correct" tool when run in a virtual environment.

Note that we still check for "pylint-3" before the test begins -- this
check is now "overly strict", but shouldn't cause anything that was
already running correctly to start failing.

Signed-off-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Hanna Reitz 
---
 tests/qemu-iotests/297 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 15b54594c11..65b1e7058c2 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -71,14 +71,14 @@ def run_linters():
 sys.stdout.flush()
 
 env = os.environ.copy()
-subprocess.run(('pylint-3', *files),
+subprocess.run(('python3', '-m', 'pylint', *files),
env=env, check=False)
 
 print('=== mypy ===')
 sys.stdout.flush()
 
 env['MYPYPATH'] = env['PYTHONPATH']
-p = subprocess.run(('mypy', *files),
+p = subprocess.run((('python3', '-m', 'mypy', *files),
env=env,
check=False,
stdout=subprocess.PIPE,
-- 
2.31.1

[PATCH 03/13] iotests/297: Add get_files() function

[John Snow]

Split out file discovery into its own method to begin separating out
configuration/setup and test execution.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297 | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b8101e6024a..15b54594c11 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,6 +21,7 @@ import re
 import shutil
 import subprocess
 import sys
+from typing import List
 
 import iotests
 
@@ -54,10 +55,14 @@ def is_python_file(filename):
 return False
 
 
-def run_linters():
+def get_test_files() -> List[str]:
 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)]
+return list(filter(is_python_file, check_tests))
+
+
+def run_linters():
+files = get_test_files()
 
 iotests.logger.debug('Files to be checked:')
 iotests.logger.debug(', '.join(sorted(files)))
-- 
2.31.1

[PATCH 02/13] iotests/297: Split mypy configuration out into mypy.ini

[John Snow]

More separation of code and configuration.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/297  | 14 +-
 tests/qemu-iotests/mypy.ini | 12 
 2 files changed, 13 insertions(+), 13 deletions(-)
 create mode 100644 tests/qemu-iotests/mypy.ini

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index bc3a0ceb2aa..b8101e6024a 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -73,19 +73,7 @@ def run_linters():
 sys.stdout.flush()
 
 env['MYPYPATH'] = env['PYTHONPATH']
-p = subprocess.run(('mypy',
-'--warn-unused-configs',
-'--disallow-subclassing-any',
-'--disallow-any-generics',
-'--disallow-incomplete-defs',
-'--disallow-untyped-decorators',
-'--no-implicit-optional',
-'--warn-redundant-casts',
-'--warn-unused-ignores',
-'--no-implicit-reexport',
-'--namespace-packages',
-'--scripts-are-modules',
-*files),
+p = subprocess.run(('mypy', *files),
env=env,
check=False,
stdout=subprocess.PIPE,
diff --git a/tests/qemu-iotests/mypy.ini b/tests/qemu-iotests/mypy.ini
new file mode 100644
index 000..4c0339f5589
--- /dev/null
+++ b/tests/qemu-iotests/mypy.ini
@@ -0,0 +1,12 @@
+[mypy]
+disallow_any_generics = True
+disallow_incomplete_defs = True
+disallow_subclassing_any = True
+disallow_untyped_decorators = True
+implicit_reexport = False
+namespace_packages = True
+no_implicit_optional = True
+scripts_are_modules = True
+warn_redundant_casts = True
+warn_unused_configs = True
+warn_unused_ignores = True
-- 
2.31.1

Re: [PATCH v3 11/16] iotests/297: return error code from run_linters()

[John Snow]

On Mon, Oct 4, 2021 at 3:45 AM Hanna Reitz  wrote:

> On 22.09.21 22:18, John Snow wrote:
> >
> >
> > On Fri, Sep 17, 2021 at 7:00 AM Hanna Reitz  > <mailto:hre...@redhat.com>> wrote:
>
> [...]
>
> >
> > As you say, run_linters() to me seems very iotests-specific still: It
> > emits a specific output that is compared against a reference output.
> > Fine for 297, but not fine for a function provided by a
> > linters.py, I’d say.
> >
>
> I’d prefer run_linters() to return something like a Map[str,
> > Optional[str]], that would map a tool to its output in case of error,
> > i.e. ideally:
> >
> > `{'pylint': None, 'mypy': None}`
> >
>
>
> > Splitting the test apart into two sub-tests is completely reasonable.
> > Python CI right now has individual tests for pylint, mypy, etc.
> >
> > 297 could format it something like
> >
> > ```
> > for tool, output in run_linters().items():
> >  print(f'=== {tool} ===')
> >  if output is not None:
> >  print(output)
> > ```
> >
> > Or something.
> >
> > To check for error, you could put a Python script in python/tests
> > that
> > checks `any(output is not None for output in
> > run_linters().values())` or
> > something (and on error print the output).
> >
> >
> > Pulling out run_linters() into an external file and having it print
> > something to stdout just seems too iotests-centric to me.  I
> > suppose as
> > long as the return code is right (which this patch is for) it should
> > work for Avocado’s simple tests, too (which I don’t like very much
> > either, by the way, because they too seem archaic to me), but,
> > well.  It
> > almost seems like the Avocado test should just run ./check then.
> >
> >
> > Yeh. Ideally, we'd just have a mypy.ini and a pylintrc that configures
> > the tests adequately, and then 297 (or whomever else) would just call
> > the linters which would in turn read the same configuration. This
> > series is somewhat of a stop-gap to measure the temperature of the
> > room to see how important it was to leave 297 inside of iotests. So, I
> > did the conservative thing that's faster to review even if it now
> > looks *slightly* fishy.
> >
> > As for things being archaic or not ... possibly, but why involve extra
> > complexity if it isn't warranted?
>
> My opinion is that I find an interface of “prints something to stdout
> and returns an integer status code” to be non-intuitive and thus rather
> complex actually.  That’s why I’d prefer different complexity, namely
> one which has a more intuitive interface.
>
>
I'm not sure I follow, though, because ultimately what we're trying to do
is run terminal commands as part of a broader test suite. Returning status
codes and printing output is what they do. We can't escape that paradigm,
so is it really necessary to abstract away from it?


> I’m also not sure where the extra complexity would be for a
> `run_linters() -> Map[str, Optional[str]]`, because 297 just needs the
> loop suggested above over `run_linters().items()`, and as for the
> Avocado test...
>
> > a simple pass/fail works perfectly well.
>
> I don’t find `any(error_msg for error_msg in run_linters().values())`
> much more complex than pass/fail.
>
> (Note: Above, I called it `output`.  Probably should have called it
> `error_msg` like I did now to clarify that it’s `None` in case of
> success and a string otherwise.)
>
> > (And the human can read the output to understand WHY it failed.) If
> > you want more rigorous analytics for some reason, we can discuss the
> > use cases and figure out how to represent that information, but that's
> > definitely a bit beyond scope here.
>
> [...]
>
> > Like, can’t we have a Python script in python/tests that imports
> > linters.py, invokes run_linters() and sensibly checks the output? Or,
> > you know, at the very least not have run_linters() print anything to
> > stdout and not have it return an integer code. linters.py:main()
> > can do
> > that conversion.
> >
> >
> > Well, I certainly don't want to bother parsing output from python
> > tools and trying to make sure that it works sensibly cross-version and
> > cross-distro. The return code being 0/non-zero is vastly simpler. Let
> > the human read the output log on failure cases to get a sense of what
> > exactly went wrong. Is there some reason why pars

Re: [PATCH v3 07/16] iotests/297: Don't rely on distro-specific linter binaries

[John Snow]

On Mon, Oct 4, 2021 at 4:17 AM Hanna Reitz  wrote:

> On 22.09.21 21:53, John Snow wrote:
> > (This email just explains python packaging stuff. No action items in
> > here. Skim away.)
> >
> > On Fri, Sep 17, 2021 at 5:43 AM Hanna Reitz  > <mailto:hre...@redhat.com>> wrote:
> >
> > On 16.09.21 06:09, John Snow wrote:
> > > 'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or
> > "python3 -m
> > > mypy" to access these scripts instead. This style of invocation
> will
> > > prefer the "correct" tool when run in a virtual environment.
> > >
> > > Note that we still check for "pylint-3" before the test begins
> > -- this
> > > check is now "overly strict", but shouldn't cause anything that was
> > > already running correctly to start failing.
> > >
> > > Signed-off-by: John Snow  > <mailto:js...@redhat.com>>
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy
> > mailto:vsement...@virtuozzo.com>>
> > > Reviewed-by: Philippe Mathieu-Daudé  > <mailto:phi...@redhat.com>>
> > > ---
> > >   tests/qemu-iotests/297 | 45
> > --
> > >   1 file changed, 26 insertions(+), 19 deletions(-)
> >
> > I know it sounds silly, but to be honest I have no idea if replacing
> > `mypy` by `python3 -m mypy` is correct, because no mypy documentation
> > seems to suggest it.
> >
> >
> > Right, I don't think it's necessarily documented that you can do this.
> > It just happens to be a very convenient way to invoke the same script
> > without needing to know *where* mypy is. You let python figure out
> > where it's going to import mypy from, and it handles the rest.
> >
> > (This also makes it easier to use things like mypy, pylint etc with an
> > explicitly specified PYTHON interpreter. I don't happen to do that in
> > this patch, but ... we could.)
> >
> >  From what I understand, that’s generally how Python “binaries” work,
> > though (i.e., installed as a module invokable with `python -m`,
> > and then
> > providing some stub binary that, well, effectively does this, but
> > kind
> > of in a weird way, I just don’t understand it), and none of the
> > parameters seem to be hurt in this conversion, so:
> >
> >
> > Right. Technically, any python package can ask for any number of
> > executables to be installed, but the setuptools packaging ecosystem
> > provides a way to "generate" these based on package configuration. I
> > use a few in our own Python packages. If you look in python/setup.cfg,
> > you'll see stuff like this:
> >
> > [options.entry_points]
> > console_scripts =
> > qom = qemu.qmp.qom:main
> > qom-set = qemu.qmp.qom:QOMSet.entry_point
> > qom-get = qemu.qmp.qom:QOMGet.entry_point
> > qom-list = qemu.qmp.qom:QOMList.entry_point
> > qom-tree = qemu.qmp.qom:QOMTree.entry_point
> > qom-fuse = qemu.qmp.qom_fuse:QOMFuse.entry_point [fuse]
> > qemu-ga-client = qemu.qmp.qemu_ga_client:main
> > qmp-shell = qemu.qmp.qmp_shell:main
> >
> > These entries cause those weird little binary wrapper scripts to be
> > generated for you when the package is *installed*. So our packages
> > will put 'qmp-shell' and 'qom-tree' into your $PATH*.The stuff to the
> > right of the equals sign is just a pointer to a function that can be
> > executed that implements the CLI command. qemu.qmp.qmp_shell points to
> > the module to import, and ':main' points to the function to run.
> >
> > The last bit of this is that many, though not all (and there's zero
> > requirement this has to be true), python packages that implement CLI
> > commands will often have a stanza in their __init__.py module that
> > says something like this:
> >
> > if __name__ == '__main__':
> > do_the_command_line_stuff()
> >
> > Alternatively, a package can include a literal __main__.py file that
> > python knows to check for, and this module is the one that gets
> > executed for `python3 -m mypackage` invocations. This is what mypy does.
> >
> > Those are the magical blurbs that allow you to execute a module as if
> > it were a script by running "python3 -m mymodule" -- that hooks
> > directly into that little execution stanza. For python code
> > distributed as packages, that's the real reas

Re: [PATCH 12/15] iotests: Disable AQMP logging under non-debug modes

[John Snow]

On Mon, Oct 4, 2021 at 6:12 AM Hanna Reitz  wrote:

> On 18.09.21 04:14, John Snow wrote:
> >
> >
> > On Fri, Sep 17, 2021 at 8:58 PM John Snow  > <mailto:js...@redhat.com>> wrote:
> >
> >
> >
> > On Fri, Sep 17, 2021 at 10:30 AM Hanna Reitz  > <mailto:hre...@redhat.com>> wrote:
> >
> > On 17.09.21 07:40, John Snow wrote:
> > > Disable the aqmp logger, which likes to (at the moment)
> > print out
> > > intermediate warnings and errors that cause session
> > termination; disable
> > > them so they don't interfere with the job output.
> > >
> > > Leave any "CRITICAL" warnings enabled though, those are ones
> > that we
> > > should never see, no matter what.
> >
> > I mean, looks OK to me, but from what I understand (i.e. little),
> > qmp_client doesn’t log CRITICAL messages, at least I can’t see
> > any. Only
> > ERRORs.
> >
> >
> > There's *one* critical message in protocol.py, used for a
> > circumstance that I *think* should be impossible. I do not think I
> > currently use any WARNING level statements.
> >
> > I guess I’m missing some CRITICAL messages in external
> > functions called
> > from qmp_client.py, but shouldn’t we still keep ERRORs?
> >
> >
> > ...Mayybe?
> >
> > The errors logged by AQMP are *almost always* raised as Exceptions
> > somewhere else, eventually. Sometimes when we encounter them in
> > one context, we need to save them and then re-raise them in a
> > different execution context. There's one good exception to this:
> > My pal, EOFError.
> >
> > If the reader context encounters EOF, it raises EOFError and this
> > causes a disconnect to be scheduled asynchronously. *Any*
> > Exception that causes a disconnect to be scheduled asynchronously
> > is dutifully logged as an ERROR. At this point in the code, we
> > don't really know if the user of the library considers this an
> > "error" yet or not. I've waffled a lot on how exactly to treat
> > this circumstance. ...Hm, I guess that's really the only case
> > where I have an error that really ought to be suppressed. I
> > suppose what I will do here is: if the exception happens to be an
> > EOFError I will drop the severity of the log message down to INFO.
> > I don't know why it takes being challenged on this stuff to start
> > thinking clearly about it, but here we are. Thank you for your
> > feedback :~)
> >
> > --js
> >
> >
> > Oh, CI testing reminds me of why I am a liar here.
> >
> > the mirror-top-perms test intentionally expects not to be able to
> > connect, but we're treated to these two additional lines of output:
> >
> > +ERROR:qemu.aqmp.qmp_client.qemub-2536319:Negotiation failed: EOFError
> > +ERROR:qemu.aqmp.qmp_client.qemub-2536319:Failed to establish session:
> > EOFError
> >
> > Uh. I guess a temporary suppression in mirror-top-perms, then ...?
>
> Sounds right to me, if that’s simple enough.
>
> (By the way, I understand it right that you want to lower the severity
> of EOFErrors to INFO only on disconnect, right?  Which is why they’re
> still logged as ERRORs here, because they aren’t occurring on disconnects?)
>
>
More or less, yeah.

When an EOFError causes the reader coroutine to halt (because it can't read
the next message), I decided (in v2) to drop that one particular logging
message down to "INFO", because it might -- or might not be -- an expected
occurrence from the point of view of whoever is managing the QMP
connection. Maybe it was expected (The test used qemu-guest-agent or
something else to make the guest shutdown, taking QEMU down with it without
the knowledge of the QMP library layer) or maybe it was unexpected (the QMP
remote really just disappeared from us on a whim). There's no way to know,
so it probably isn't right to consider it an error.

In the connection case, I left it as an ERROR because the caller asked us
to connect to an endpoint and we were unable to, which feels unambiguous.
It will be ultimately reported via Exceptions as qemu.aqmp.ConnectError,
with additional information available in fields of that exception object.
Even though the exception is reported to the caller, I decided to log the
occurrence anyway, because I felt like it should be the job of the library
to make a good log and not the caller's responsibility to catch

Re: Running 297 from GitLab CI

[John Snow]

On Fri, Oct 1, 2021 at 4:21 AM Kevin Wolf  wrote:

> Am 30.09.2021 um 23:28 hat John Snow geschrieben:
> > Hiya, I was talking this over with Hanna in review to '[PATCH v3 00/16]
> > python/iotests: Run iotest linters during Python CI' [1] and I have some
> > doubt about what you'd personally like to see happen, here.
> >
> > In a nutshell, I split out 'linters.py' from 297 and keep all of the
> > iotest-bits in 297 and all of the generic "run the linters" bits in
> > linters.py, then I run linters.py from the GitLab python CI jobs.
> >
> > I did this so that iotest #297 would continue to work exactly as it had,
> > but trying to serve "two masters" in the form of two test suites means
> some
> > non-beautiful design decisions. Hanna suggested we just outright drop
> test
> > 297 to possibly improve the factoring of the tests.
> >
> > I don't want to do that unless you give it the go-ahead, though. I wanted
> > to hear your feelings on if we still want to keep 297 around or not.
>
> My basic requirement is that the checks are run somewhere in my local
> testing before I prepare a pull request. This means it could be done by
> iotests in any test that runs for -raw or -qcow2, or in 'make check'.
>
> So if you have a replacement somewhere in 'make check', dropping 297 is
> fine with me. If I have to run something entirely different, you may
> need to invest a bit more effort to convince me. ;-)
>
>
I love a good set of solid criteria ;-)

Understood! At the moment it *would* require a separate invocation. The
Python tests that run under CI generally set up their own environments to
ensure they'll run with minimum fuss or intervention from humans, though
there is an invocation in that Makefile that performs no environment setup
whatsoever -- but since no automated test uses it, it's not really a big
problem if your environment is "wrong" for that one. (But that also makes
it useless for make check!) It's similar to how iotest 297 does not really
check to see what version of pylint or mypy you might have, so sometimes
that test fails if your environment isn't suitable. But that one isn't part
of 'make check' either, so this feels like a bridge we've never crossed
anywhere in the whole source tree.

I have so far abstained from introducing such a step into 'make check'
because it might require some additional engineering to ensure that I get
the temporary directories all correct, tolerate the different types of
builds we do, uses the correct Python interpreter for all steps, etc.

So for now I'll propose leaving 297 as-is for convenience, but I will start
working towards finding the right way to include those tests from 'make
check'. I think there's no barrier (other than subjectively, beauty) to
leaving both avenues in for running the test for the time being. Maybe I
will find a way to convince Hanna to accept the interim solution as "well,
not THAT ugly."

Thanks for your input!

--js

Running 297 from GitLab CI

[John Snow]

Hiya, I was talking this over with Hanna in review to '[PATCH v3 00/16]
python/iotests: Run iotest linters during Python CI' [1] and I have some
doubt about what you'd personally like to see happen, here.

In a nutshell, I split out 'linters.py' from 297 and keep all of the
iotest-bits in 297 and all of the generic "run the linters" bits in
linters.py, then I run linters.py from the GitLab python CI jobs.

I did this so that iotest #297 would continue to work exactly as it had,
but trying to serve "two masters" in the form of two test suites means some
non-beautiful design decisions. Hanna suggested we just outright drop test
297 to possibly improve the factoring of the tests.

I don't want to do that unless you give it the go-ahead, though. I wanted
to hear your feelings on if we still want to keep 297 around or not.

--js

[1] https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg05787.html

Re: [PATCH 0/4] qemu-img compare --stat

[John Snow]

On Wed, Sep 29, 2021 at 9:34 AM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:

> Hi all!
>
> Recently we faced the following task:
>
> Customer comes and say: incremental backup images are too fat. Does you
> incremental backup works correct?
>
> What to answer? We should check something. At least check that
> incremental images doesn't store same data twice. And we don't have a
> tool for it. I just wrote a simple python script to compare raw files
> cluster-by-cluster. Then we've mounted the qcow2 images with help of
> qemu-nbd, the resulting /dev/nbd* were compared and we proved that
> incremental backups don't store same data.
>
>
Good idea. I love diagnostic tools!


> But that leads to idea that some kind of that script would be good to
> have at hand.
>
> So, here is a new option for qemu-img compare, that is a lot more
> powerful and effective than original script, and allows to compare and
> calculate statistics, i.e. how many clusters differs, how many
> clusters changed from unallocated to data, and so on.
>
> For examples of output look at the test in patch 04.
>
> Vladimir Sementsov-Ogievskiy (4):
>   qemu-img: implement compare --stat
>   qemu-img: make --block-size optional for compare --stat
>   qemu-img: add --shallow option for qemu-img compare --stat
>   iotests: add qemu-img-compare-stat test
>
>  docs/tools/qemu-img.rst   |  29 +-
>  qemu-img.c| 275 +-
>  qemu-img-cmds.hx  |   4 +-
>  .../qemu-iotests/tests/qemu-img-compare-stat  |  88 ++
>

And new tests! :-)


>  .../tests/qemu-img-compare-stat.out   | 106 +++
>  5 files changed, 484 insertions(+), 18 deletions(-)
>  create mode 100755 tests/qemu-iotests/tests/qemu-img-compare-stat
>  create mode 100644 tests/qemu-iotests/tests/qemu-img-compare-stat.out
>
>
>

Re: [PATCH v2 0/6] iotests: update environment and linting configuration

[John Snow]

On Thu, Sep 23, 2021 at 2:07 PM John Snow  wrote:

> GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest-pt1
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/376236687
>
> This series partially supersedes:
>   [PATCH v3 00/16] python/iotests: Run iotest linters during Python CI'
>
> Howdy, this is good stuff we want even if we aren't yet in agreement
> about the best way to run iotest 297 from CI.
>
> - Update linting config to tolerate pylint 2.11.1
> - Eliminate sys.path hacking in individual test files
> - make mypy execution in test 297 faster
>
> The rest of the actual "run at CI time" stuff can get handled separately
> and later pending some discussion on the other series.
>
> V2:
>
> 001/6:[0011] [FC] 'iotests: add 'qemu' package location to PYTHONPATH in
> testenv'
> 002/6:[0025] [FC] 'iotests: add warning for rogue 'qemu' packages'
>
> - Squashed in a small optimization from Vladimir to 001, kept R-Bs.
> - Fixed the package detection logic to not panic if it can't find
>   'qemu' at all (kwolf)
> - Updated commit messages for the first two patches.
>
> --js
>
> John Snow (6):
>   iotests: add 'qemu' package location to PYTHONPATH in testenv
>   iotests: add warning for rogue 'qemu' packages
>   iotests/linters: check mypy files all at once
>   iotests/mirror-top-perms: Adjust imports
>   iotests/migrate-bitmaps-test: delint
>   iotests: Update for pylint 2.11.1
>
>  tests/qemu-iotests/235|  2 -
>  tests/qemu-iotests/297| 50 ---
>  tests/qemu-iotests/300|  7 ++-
>  tests/qemu-iotests/iotests.py |  2 -
>  tests/qemu-iotests/pylintrc   |  6 ++-
>  tests/qemu-iotests/testenv.py | 39 ---
>  tests/qemu-iotests/testrunner.py  |  7 +--
>  tests/qemu-iotests/tests/migrate-bitmaps-test | 50 +++
>  tests/qemu-iotests/tests/mirror-top-perms | 12 ++---
>  9 files changed, 99 insertions(+), 76 deletions(-)
>
> --
> 2.31.1
>
>
>
Patch 2 can just be dropped, and everything else is reviewed, so I think
this can be staged at your leisure.

--js

Re: [PATCH v2 2/6] iotests: add warning for rogue 'qemu' packages

[John Snow]

On Thu, Sep 23, 2021 at 4:27 PM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:

> 23.09.2021 21:44, John Snow wrote:
> >
> >
> > On Thu, Sep 23, 2021 at 2:32 PM Vladimir Sementsov-Ogievskiy <
> vsement...@virtuozzo.com <mailto:vsement...@virtuozzo.com>> wrote:
> >
> > 23.09.2021 21:07, John Snow wrote:
> >  > Add a warning for when 'iotests' runs against a qemu namespace
> that
> >  > isn't the one in the source tree. This might occur if you have
> >  > (accidentally) installed the Python namespace package to your
> local
> >  > packages.
> >  >
> >  > (I'm not going to say that this is because I bit myself with this,
> >  > but you can fill in the blanks.)
> >  >
> >  > In the future, we will pivot to always preferring a specific
> installed
> >  > instance of qemu python packages managed directly by iotests. For
> now
> >  > simply warn if there is an ambiguity over which instance that
> iotests
> >  > might use.
> >  >
> >  > Example: If a user has navigated to ~/src/qemu/python and executed
> >  > `pip install .`, you will see output like this when running
> `./check`:
> >  >
> >  > WARNING: 'qemu' python packages will be imported from outside the
> source tree ('/home/jsnow/src/qemu/python')
> >  >   Importing instead from
> '/home/jsnow/.local/lib/python3.9/site-packages/qemu'
> >  >
> >  > Signed-off-by: John Snow  js...@redhat.com>>
> >  > ---
> >  >   tests/qemu-iotests/testenv.py | 24 
> >  >   1 file changed, 24 insertions(+)
> >  >
> >  > diff --git a/tests/qemu-iotests/testenv.py
> b/tests/qemu-iotests/testenv.py
> >  > index 99a57a69f3a..1c0f6358538 100644
> >  > --- a/tests/qemu-iotests/testenv.py
> >  > +++ b/tests/qemu-iotests/testenv.py
> >  > @@ -16,6 +16,8 @@
> >  >   # along with this program.  If not, see <
> http://www.gnu.org/licenses/ <http://www.gnu.org/licenses/>>.
> >  >   #
> >  >
> >  > +import importlib.util
> >  > +import logging
> >  >   import os
> >  >   import sys
> >  >   import tempfile
> >  > @@ -112,6 +114,27 @@ def init_directories(self) -> None:
> >  >   # Path where qemu goodies live in this source tree.
> >  >   qemu_srctree_path = Path(__file__,
> '../../../python').resolve()
> >  >
> >  > +# Warn if we happen to be able to find qemu namespace
> packages
> >  > +# (using qemu.qmp as a bellwether) from an unexpected
> location.
> >  > +# i.e. the package is already installed in the user's
> environment.
> >  > +try:
> >  > +qemu_spec = importlib.util.find_spec('qemu.qmp')
> >  > +except ModuleNotFoundError:
> >  > +qemu_spec = None
> >  > +
> >  > +if qemu_spec and qemu_spec.origin:
> >  > +spec_path = Path(qemu_spec.origin)
> >  > +try:
> >  > +_ = spec_path.relative_to(qemu_srctree_path)
> >
> > It took some time and looking at specification trying to understand
> what's going on here :)
> >
> > Could we just use:
> >
> > if not Path(qemu_spec.origin).is_relative_to(qemu_srctree_path):
> >  ... logging ...
> >
> >
> > Nope, that's 3.9+ only. (I made the same mistake.)
>
> Oh :(
>
> OK
>
> >
> >
> >  > +except ValueError:
> >
> >  > +self._logger.warning(
> >  > +"WARNING: 'qemu' python packages will be
> imported from"
> >  > +" outside the source tree ('%s')",
> >  > +qemu_srctree_path)
>
> why not use f-strings ? :)
>
>
The logging subsystem still uses % formatting by default, you can see I'm
not actually using the % operator to apply the values -- the Python docs
claim this is for "efficiency" because the string doesn't have to be
evaluated unless the logging statement is actually emitted, but that gain
sounds mostly theoretical to me, because Python still has eager evaluation
of passed arguments ... unless I've missed something.

Either way, using f-strings on logging calls gives

Re: [PATCH v2 2/6] iotests: add warning for rogue 'qemu' packages

[John Snow]

On Thu, Sep 23, 2021 at 2:32 PM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:

> 23.09.2021 21:07, John Snow wrote:
> > Add a warning for when 'iotests' runs against a qemu namespace that
> > isn't the one in the source tree. This might occur if you have
> > (accidentally) installed the Python namespace package to your local
> > packages.
> >
> > (I'm not going to say that this is because I bit myself with this,
> > but you can fill in the blanks.)
> >
> > In the future, we will pivot to always preferring a specific installed
> > instance of qemu python packages managed directly by iotests. For now
> > simply warn if there is an ambiguity over which instance that iotests
> > might use.
> >
> > Example: If a user has navigated to ~/src/qemu/python and executed
> > `pip install .`, you will see output like this when running `./check`:
> >
> > WARNING: 'qemu' python packages will be imported from outside the source
> tree ('/home/jsnow/src/qemu/python')
> >   Importing instead from
> '/home/jsnow/.local/lib/python3.9/site-packages/qemu'
> >
> > Signed-off-by: John Snow 
> > ---
> >   tests/qemu-iotests/testenv.py | 24 
> >   1 file changed, 24 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/testenv.py
> b/tests/qemu-iotests/testenv.py
> > index 99a57a69f3a..1c0f6358538 100644
> > --- a/tests/qemu-iotests/testenv.py
> > +++ b/tests/qemu-iotests/testenv.py
> > @@ -16,6 +16,8 @@
> >   # along with this program.  If not, see <http://www.gnu.org/licenses/
> >.
> >   #
> >
> > +import importlib.util
> > +import logging
> >   import os
> >   import sys
> >   import tempfile
> > @@ -112,6 +114,27 @@ def init_directories(self) -> None:
> >   # Path where qemu goodies live in this source tree.
> >   qemu_srctree_path = Path(__file__, '../../../python').resolve()
> >
> > +# Warn if we happen to be able to find qemu namespace packages
> > +# (using qemu.qmp as a bellwether) from an unexpected location.
> > +# i.e. the package is already installed in the user's
> environment.
> > +try:
> > +qemu_spec = importlib.util.find_spec('qemu.qmp')
> > +except ModuleNotFoundError:
> > +qemu_spec = None
> > +
> > +if qemu_spec and qemu_spec.origin:
> > +spec_path = Path(qemu_spec.origin)
> > +try:
> > +_ = spec_path.relative_to(qemu_srctree_path)
>
> It took some time and looking at specification trying to understand what's
> going on here :)
>
> Could we just use:
>
> if not Path(qemu_spec.origin).is_relative_to(qemu_srctree_path):
> ... logging ...
>
>
Nope, that's 3.9+ only. (I made the same mistake.)


> > +except ValueError:
>
> +self._logger.warning(
> > +"WARNING: 'qemu' python packages will be imported
> from"
> > +" outside the source tree ('%s')",
> > +qemu_srctree_path)
> > +self._logger.warning(
> > +" Importing instead from '%s'",
> > +spec_path.parents[1])
> > +
>
> Also, I'd move this new chunk of code to a separate function (may be even
> out of class, as the only usage of self is self._logger, which you
> introduce with this patch. Still a method would be OK too). And then, just
> call it from __init__(). Just to keep init_directories() simpler. And with
> this new code we don't init any directories to pass to further test
> execution, it's just a check for runtime environment.
>
>
I wanted to keep the wiggly python import logic all in one place so that it
was harder to accidentally forget a piece of it if/when we adjust it. I can
create a standalone function for it, but I'd need to stash that
qemu_srctree_path variable somewhere if we want to call that runtime check
from somewhere else, because I don't want to compute it twice. Is it still
worth doing in your opinion if I just create a method/function and pass it
the qemu_srctree_path variable straight from init_directories()?

Not adding _logger is valid, though. I almost removed it myself. I'll
squash that in.


> >   self.pythonpath = os.pathsep.join(filter(None, (
> >   self.source_iotests,
> >   str(qemu_srctree_path),
> > @@ -230,6 +253,7 @@ def __init__(self, imgfmt: str, imgproto: str,
> aiomode: str,
> >
> >   self.build_root = os.path.join(self.build_iotests, '..', '..')
> >
> > +self._logger = logging.getLogger('qemu.iotests')
> >   self.init_directories()
> >   self.init_binaries()
> >
> >
>
>
> --
> Best regards,
> Vladimir
>
>

[PATCH v2 5/6] iotests/migrate-bitmaps-test: delint

[John Snow]

Mostly uninteresting stuff. Move the test injections under a function
named main() so that the variables used during that process aren't in
the global scope.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Hanna Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Kevin Wolf 
---
 tests/qemu-iotests/tests/migrate-bitmaps-test | 50 +++
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-test 
b/tests/qemu-iotests/tests/migrate-bitmaps-test
index dc431c35b35..c23df3d75c7 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-test
@@ -19,10 +19,11 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
-import os
 import itertools
 import operator
+import os
 import re
+
 import iotests
 from iotests import qemu_img, qemu_img_create, Timeout
 
@@ -224,25 +225,6 @@ def inject_test_case(klass, suffix, method, *args, 
**kwargs):
 setattr(klass, 'test_' + method + suffix, lambda self: mc(self))
 
 
-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]:
-name += '__pre_shutdown'
-
-inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
- *list(cmb))
-
-for cmb in list(itertools.product((True, False), repeat=2)):
-name = ('_' if cmb[0] else '_not_') + 'persistent_'
-name += ('_' if cmb[1] else '_not_') + 'migbitmap'
-
-inject_test_case(TestDirtyBitmapMigration, name,
- 'do_test_migration_resume_source', *list(cmb))
-
-
 class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
 def setUp(self):
 qemu_img_create('-f', iotests.imgfmt, base_a, size)
@@ -304,6 +286,30 @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
 self.assert_qmp(result, 'return', {})
 
 
+def main() -> None:
+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]:
+name += '__pre_shutdown'
+
+inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
+ *list(cmb))
+
+for cmb in list(itertools.product((True, False), repeat=2)):
+name = ('_' if cmb[0] else '_not_') + 'persistent_'
+name += ('_' if cmb[1] else '_not_') + 'migbitmap'
+
+inject_test_case(TestDirtyBitmapMigration, name,
+ 'do_test_migration_resume_source', *list(cmb))
+
+iotests.main(
+supported_fmts=['qcow2'],
+supported_protocols=['file']
+)
+
+
 if __name__ == '__main__':
-iotests.main(supported_fmts=['qcow2'],
- supported_protocols=['file'])
+main()
-- 
2.31.1

[PATCH v2 6/6] iotests: Update for pylint 2.11.1

[John Snow]

1. Ignore the new f-strings warning, we're not interested in doing a
   full conversion at this time.

2. Just mute the unbalanced-tuple-unpacking warning, it's not a real
   error in this case and muting the dozens of callsites is just not
   worth it.

3. Add encodings to read_text().

Signed-off-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Kevin Wolf 
---
 tests/qemu-iotests/pylintrc  | 6 +-
 tests/qemu-iotests/testrunner.py | 7 ---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index f2c0b522ac0..8cb4e1d6a6d 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -19,13 +19,17 @@ disable=invalid-name,
 too-many-public-methods,
 # pylint warns about Optional[] etc. as unsubscriptable in 3.9
 unsubscriptable-object,
+# pylint's static analysis causes false positivies for file_path();
+# If we really care to make it statically knowable, we'll use mypy.
+unbalanced-tuple-unpacking,
 # Sometimes we need to disable a newly introduced pylint warning.
 # Doing so should not produce a warning in older versions of pylint.
 bad-option-value,
 # These are temporary, and should be removed:
 missing-docstring,
 too-many-return-statements,
-too-many-statements
+too-many-statements,
+consider-using-f-string,
 
 [FORMAT]
 
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 4a6ec421ed6..a56b6da3968 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -266,12 +266,13 @@ def do_run_test(self, test: str) -> TestResult:
   diff=file_diff(str(f_reference), str(f_bad)))
 
 if f_notrun.exists():
-return TestResult(status='not run',
-  description=f_notrun.read_text().strip())
+return TestResult(
+status='not run',
+description=f_notrun.read_text(encoding='utf-8').strip())
 
 casenotrun = ''
 if f_casenotrun.exists():
-casenotrun = f_casenotrun.read_text()
+casenotrun = f_casenotrun.read_text(encoding='utf-8')
 
 diff = file_diff(str(f_reference), str(f_bad))
 if diff:
-- 
2.31.1

[PATCH v2 4/6] iotests/mirror-top-perms: Adjust imports

[John Snow]

We need to import subpackages from the qemu namespace package; importing
the namespace package alone doesn't bring the subpackages with it --
unless someone else (like iotests.py) imports them too.

Adjust the imports.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Hanna Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Kevin Wolf 
---
 tests/qemu-iotests/tests/mirror-top-perms | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/tests/mirror-top-perms 
b/tests/qemu-iotests/tests/mirror-top-perms
index 73138a0ef91..3d475aa3a54 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,7 +21,8 @@
 
 import os
 
-import qemu
+from qemu import qmp
+from qemu.machine import machine
 
 import iotests
 from iotests import qemu_img
@@ -46,7 +47,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
 def tearDown(self):
 try:
 self.vm.shutdown()
-except qemu.machine.machine.AbnormalShutdown:
+except machine.AbnormalShutdown:
 pass
 
 if self.vm_b is not None:
@@ -101,7 +102,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
 self.vm_b.launch()
 print('ERROR: VM B launched successfully, this should not have '
   'happened')
-except qemu.qmp.QMPConnectError:
+except qmp.QMPConnectError:
 assert 'Is another process using the image' in self.vm_b.get_log()
 
 result = self.vm.qmp('block-job-cancel',
-- 
2.31.1

[PATCH v2 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv

[John Snow]

We can drop the sys.path hacking in various places by doing
this. Additionally, by doing it in one place right up top, we can print
interesting warnings in case the environment does not look correct. (See
next commit.)

If we ever decide to change how the environment is crafted, all of the
"help me find my python packages" goop is all in one place, right in one
function.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Kevin Wolf 
---
 tests/qemu-iotests/235|  2 --
 tests/qemu-iotests/297|  6 --
 tests/qemu-iotests/300|  7 +++
 tests/qemu-iotests/iotests.py |  2 --
 tests/qemu-iotests/testenv.py | 15 +--
 tests/qemu-iotests/tests/mirror-top-perms |  7 +++
 6 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
index 8aed45f9a76..4de920c3801 100755
--- a/tests/qemu-iotests/235
+++ b/tests/qemu-iotests/235
@@ -24,8 +24,6 @@ import os
 import iotests
 from iotests import qemu_img_create, qemu_io, file_path, log
 
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-
 from qemu.machine import QEMUMachine
 
 iotests.script_initialize(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b04cba53667..467b712280e 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -68,12 +68,6 @@ def run_linters():
 # Todo notes are fine, but fixme's or xxx's should probably just be
 # fixed (in tests, at least)
 env = os.environ.copy()
-qemu_module_path = os.path.join(os.path.dirname(__file__),
-'..', '..', 'python')
-try:
-env['PYTHONPATH'] += os.pathsep + qemu_module_path
-except KeyError:
-env['PYTHONPATH'] = qemu_module_path
 subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
env=env, check=False)
 
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index fe94de84edd..10f9f2a8da6 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -24,12 +24,11 @@ import random
 import re
 from typing import Dict, List, Optional
 
-import iotests
-
-# Import qemu after iotests.py has amended sys.path
-# pylint: disable=wrong-import-order
 from qemu.machine import machine
 
+import iotests
+
+
 BlockBitmapMapping = List[Dict[str, object]]
 
 mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ce06cf56304..b06ad76e0c5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -36,8 +36,6 @@
 
 from contextlib import contextmanager
 
-# pylint: disable=import-error, wrong-import-position
-sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu.machine import qtest
 from qemu.qmp import QMPMessage
 
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 70da0d60c80..99a57a69f3a 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -108,12 +108,15 @@ def init_directories(self) -> None:
  SAMPLE_IMG_DIR
  OUTPUT_DIR
 """
-self.pythonpath = os.getenv('PYTHONPATH')
-if self.pythonpath:
-self.pythonpath = self.source_iotests + os.pathsep + \
-self.pythonpath
-else:
-self.pythonpath = self.source_iotests
+
+# Path where qemu goodies live in this source tree.
+qemu_srctree_path = Path(__file__, '../../../python').resolve()
+
+self.pythonpath = os.pathsep.join(filter(None, (
+self.source_iotests,
+str(qemu_srctree_path),
+os.getenv('PYTHONPATH'),
+)))
 
 self.test_dir = os.getenv('TEST_DIR',
   os.path.join(os.getcwd(), 'scratch'))
diff --git a/tests/qemu-iotests/tests/mirror-top-perms 
b/tests/qemu-iotests/tests/mirror-top-perms
index 2fc8dd66e0a..73138a0ef91 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -20,13 +20,12 @@
 #
 
 import os
+
+import qemu
+
 import iotests
 from iotests import qemu_img
 
-# Import qemu after iotests.py has amended sys.path
-# pylint: disable=wrong-import-order
-import qemu
-
 
 image_size = 1 * 1024 * 1024
 source = os.path.join(iotests.test_dir, 'source.img')
-- 
2.31.1

[PATCH v2 3/6] iotests/linters: check mypy files all at once

[John Snow]

We can circumvent the '__main__' redefinition problem by passing
--scripts-are-modules. Take mypy out of the loop per-filename and check
everything in one go: it's quite a bit faster.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Kevin Wolf 
---
 tests/qemu-iotests/297 | 44 +++---
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 467b712280e..91ec34d9521 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -74,32 +74,28 @@ def run_linters():
 print('=== mypy ===')
 sys.stdout.flush()
 
-# We have to call mypy separately for each file.  Otherwise, it
-# will interpret all given files as belonging together (i.e., they
-# may not both define the same classes, etc.; most notably, they
-# must not both define the __main__ module).
 env['MYPYPATH'] = env['PYTHONPATH']
-for filename in files:
-p = subprocess.run(('mypy',
-'--warn-unused-configs',
-'--disallow-subclassing-any',
-'--disallow-any-generics',
-'--disallow-incomplete-defs',
-'--disallow-untyped-decorators',
-'--no-implicit-optional',
-'--warn-redundant-casts',
-'--warn-unused-ignores',
-'--no-implicit-reexport',
-'--namespace-packages',
-filename),
-   env=env,
-   check=False,
-   stdout=subprocess.PIPE,
-   stderr=subprocess.STDOUT,
-   universal_newlines=True)
+p = subprocess.run(('mypy',
+'--warn-unused-configs',
+'--disallow-subclassing-any',
+'--disallow-any-generics',
+'--disallow-incomplete-defs',
+'--disallow-untyped-decorators',
+'--no-implicit-optional',
+'--warn-redundant-casts',
+'--warn-unused-ignores',
+'--no-implicit-reexport',
+'--namespace-packages',
+'--scripts-are-modules',
+*files),
+   env=env,
+   check=False,
+   stdout=subprocess.PIPE,
+   stderr=subprocess.STDOUT,
+   universal_newlines=True)
 
-if p.returncode != 0:
-print(p.stdout)
+if p.returncode != 0:
+print(p.stdout)
 
 
 for linter in ('pylint-3', 'mypy'):
-- 
2.31.1

[PATCH v2 0/6] iotests: update environment and linting configuration

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest-pt1
CI: https://gitlab.com/jsnow/qemu/-/pipelines/376236687

This series partially supersedes:
  [PATCH v3 00/16] python/iotests: Run iotest linters during Python CI'

Howdy, this is good stuff we want even if we aren't yet in agreement
about the best way to run iotest 297 from CI.

- Update linting config to tolerate pylint 2.11.1
- Eliminate sys.path hacking in individual test files
- make mypy execution in test 297 faster

The rest of the actual "run at CI time" stuff can get handled separately
and later pending some discussion on the other series.

V2:

001/6:[0011] [FC] 'iotests: add 'qemu' package location to PYTHONPATH in 
testenv'
002/6:[0025] [FC] 'iotests: add warning for rogue 'qemu' packages'

- Squashed in a small optimization from Vladimir to 001, kept R-Bs.
- Fixed the package detection logic to not panic if it can't find
  'qemu' at all (kwolf)
- Updated commit messages for the first two patches.

--js

John Snow (6):
  iotests: add 'qemu' package location to PYTHONPATH in testenv
  iotests: add warning for rogue 'qemu' packages
  iotests/linters: check mypy files all at once
  iotests/mirror-top-perms: Adjust imports
  iotests/migrate-bitmaps-test: delint
  iotests: Update for pylint 2.11.1

 tests/qemu-iotests/235|  2 -
 tests/qemu-iotests/297| 50 ---
 tests/qemu-iotests/300|  7 ++-
 tests/qemu-iotests/iotests.py |  2 -
 tests/qemu-iotests/pylintrc   |  6 ++-
 tests/qemu-iotests/testenv.py | 39 ---
 tests/qemu-iotests/testrunner.py  |  7 +--
 tests/qemu-iotests/tests/migrate-bitmaps-test | 50 +++
 tests/qemu-iotests/tests/mirror-top-perms | 12 ++---
 9 files changed, 99 insertions(+), 76 deletions(-)

-- 
2.31.1

[PATCH v2 2/6] iotests: add warning for rogue 'qemu' packages

[John Snow]

Add a warning for when 'iotests' runs against a qemu namespace that
isn't the one in the source tree. This might occur if you have
(accidentally) installed the Python namespace package to your local
packages.

(I'm not going to say that this is because I bit myself with this,
but you can fill in the blanks.)

In the future, we will pivot to always preferring a specific installed
instance of qemu python packages managed directly by iotests. For now
simply warn if there is an ambiguity over which instance that iotests
might use.

Example: If a user has navigated to ~/src/qemu/python and executed
`pip install .`, you will see output like this when running `./check`:

WARNING: 'qemu' python packages will be imported from outside the source tree 
('/home/jsnow/src/qemu/python')
 Importing instead from 
'/home/jsnow/.local/lib/python3.9/site-packages/qemu'

Signed-off-by: John Snow 
---
 tests/qemu-iotests/testenv.py | 24 
 1 file changed, 24 insertions(+)

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 99a57a69f3a..1c0f6358538 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -16,6 +16,8 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
+import importlib.util
+import logging
 import os
 import sys
 import tempfile
@@ -112,6 +114,27 @@ def init_directories(self) -> None:
 # Path where qemu goodies live in this source tree.
 qemu_srctree_path = Path(__file__, '../../../python').resolve()
 
+# Warn if we happen to be able to find qemu namespace packages
+# (using qemu.qmp as a bellwether) from an unexpected location.
+# i.e. the package is already installed in the user's environment.
+try:
+qemu_spec = importlib.util.find_spec('qemu.qmp')
+except ModuleNotFoundError:
+qemu_spec = None
+
+if qemu_spec and qemu_spec.origin:
+spec_path = Path(qemu_spec.origin)
+try:
+_ = spec_path.relative_to(qemu_srctree_path)
+except ValueError:
+self._logger.warning(
+"WARNING: 'qemu' python packages will be imported from"
+" outside the source tree ('%s')",
+qemu_srctree_path)
+self._logger.warning(
+" Importing instead from '%s'",
+spec_path.parents[1])
+
 self.pythonpath = os.pathsep.join(filter(None, (
 self.source_iotests,
 str(qemu_srctree_path),
@@ -230,6 +253,7 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
 
 self.build_root = os.path.join(self.build_iotests, '..', '..')
 
+self._logger = logging.getLogger('qemu.iotests')
 self.init_directories()
 self.init_binaries()
 
-- 
2.31.1

Re: [PATCH 1/6] iotests: add 'qemu' package location to PYTHONPATH in testenv

[John Snow]

On Thu, Sep 23, 2021 at 11:20 AM Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:

> 23.09.2021 03:16, John Snow wrote:
> > We can drop the sys.path hacking in various places by doing
> > this. Additionally, by doing it in one place right up top, we can print
> > interesting warnings in case the environment does not look correct.
> >
> > If we ever decide to change how the environment is crafted, all of the
> > "help me find my python packages" goop is all in one place, right in one
> > function.
> >
> > Signed-off-by: John Snow 
>
> Hurrah!!
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
>
> > ---
> >   tests/qemu-iotests/235|  2 --
> >   tests/qemu-iotests/297|  6 --
> >   tests/qemu-iotests/300|  7 +++
> >   tests/qemu-iotests/iotests.py |  2 --
> >   tests/qemu-iotests/testenv.py | 14 +-
> >   tests/qemu-iotests/tests/mirror-top-perms |  7 +++
> >   6 files changed, 15 insertions(+), 23 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/235 b/tests/qemu-iotests/235
> > index 8aed45f9a76..4de920c3801 100755
> > --- a/tests/qemu-iotests/235
> > +++ b/tests/qemu-iotests/235
> > @@ -24,8 +24,6 @@ import os
> >   import iotests
> >   from iotests import qemu_img_create, qemu_io, file_path, log
> >
> > -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
> 'python'))
> > -
> >   from qemu.machine import QEMUMachine
> >
> >   iotests.script_initialize(supported_fmts=['qcow2'])
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index b04cba53667..467b712280e 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -68,12 +68,6 @@ def run_linters():
> >   # Todo notes are fine, but fixme's or xxx's should probably just be
> >   # fixed (in tests, at least)
> >   env = os.environ.copy()
> > -qemu_module_path = os.path.join(os.path.dirname(__file__),
> > -'..', '..', 'python')
> > -try:
> > -env['PYTHONPATH'] += os.pathsep + qemu_module_path
> > -except KeyError:
> > -env['PYTHONPATH'] = qemu_module_path
> >   subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX',
> *files),
> >  env=env, check=False)
> >
> > diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
> > index fe94de84edd..10f9f2a8da6 100755
> > --- a/tests/qemu-iotests/300
> > +++ b/tests/qemu-iotests/300
> > @@ -24,12 +24,11 @@ import random
> >   import re
> >   from typing import Dict, List, Optional
> >
> > -import iotests
> > -
> > -# Import qemu after iotests.py has amended sys.path
> > -# pylint: disable=wrong-import-order
> >   from qemu.machine import machine
> >
> > +import iotests
> > +
> > +
> >   BlockBitmapMapping = List[Dict[str, object]]
> >
> >   mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
> > diff --git a/tests/qemu-iotests/iotests.py
> b/tests/qemu-iotests/iotests.py
> > index ce06cf56304..b06ad76e0c5 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -36,8 +36,6 @@
> >
> >   from contextlib import contextmanager
> >
> > -# pylint: disable=import-error, wrong-import-position
> > -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
> 'python'))
> >   from qemu.machine import qtest
> >   from qemu.qmp import QMPMessage
> >
> > diff --git a/tests/qemu-iotests/testenv.py
> b/tests/qemu-iotests/testenv.py
> > index 70da0d60c80..88104dace90 100644
> > --- a/tests/qemu-iotests/testenv.py
> > +++ b/tests/qemu-iotests/testenv.py
> > @@ -108,12 +108,16 @@ def init_directories(self) -> None:
> >SAMPLE_IMG_DIR
> >OUTPUT_DIR
> >   """
> > +
> > +# Path where qemu goodies live in this source tree.
> > +qemu_srctree_path = Path(__file__, '../../../python').resolve()
> > +
> >   self.pythonpath = os.getenv('PYTHONPATH')
> > -if self.pythonpath:
> > -self.pythonpath = self.source_iotests + os.pathsep + \
> > -self.pythonpath
> > -else:
> > -self.pythonpath = self.source_iotests
> > +self.pythonpath = os.pathsep.join(filter(None, (
> > +self.source_iotests,
> > +str(qemu_sr

Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages

[John Snow]

On Thu, Sep 23, 2021 at 7:09 AM Daniel P. Berrangé 
wrote:

> On Wed, Sep 22, 2021 at 08:16:21PM -0400, John Snow wrote:
> > Add a warning for when 'iotests' runs against a qemu namespace that
> > isn't the one in the source tree. This might occur if you have
> > (accidentally) installed the Python namespace package to your local
> > packages.
>
> IIUC, it is/was a valid use case to run the iotests on arbitrary
> QEMU outside the source tree ie a distro packaged binary set.
>
> Are we not allowing the tests/qemu-iotests/* content to be
> run outside the context of the main source tree for this
> purpose ?
>
> eg  consider if Fedora/RHEL builds put tests/qemu-iotests/* into
> a 'qemu-iotests' RPM, which was installed and used with a distro
> package python-qemu ?
>
>
(1) "isn't the one in the source tree" is imprecise language here. The real
key is that it must be the QEMU namespace located at "
testenv.py/../../../python/qemu". This usually means the source tree, but
it'd work in any identically structured filesystem hierarchy.

(2) The preceding patch might help illustrate this. At present the iotests
expect to be able to find the 'qemu' python packages by navigating
directories starting from their own location (and not CWD). What patch 1
does is to centralize the logic that has to go "searching" for the python
packages into the init_directories method in testenv.py so that each
individual test doesn't have to repeat it.

i.e. before patch 1, iotests.py does this:
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
'python'))

so we'll always crawl to ../../python from wherever iotests.py is.

After patch 1, we're going to crawl to the same location, but starting from
testenv.py instead. testenv.py and iotests.py are in the same directory, so
I expect whatever worked before (and in whatever environment) will continue
working after. I can't say with full certainty in what circumstances we
support running iotests, but I don't think I have introduced any new
limitation with this.


> > (I'm not going to say that this is because I bit myself with this,
> > but. You can fill in the blanks.)
> > Signed-off-by: John Snow 
> > ---
> >  tests/qemu-iotests/testenv.py | 21 -
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qemu-iotests/testenv.py
> b/tests/qemu-iotests/testenv.py
> > index 88104dace90..8a43b193af5 100644
> > --- a/tests/qemu-iotests/testenv.py
> > +++ b/tests/qemu-iotests/testenv.py
> > @@ -16,6 +16,8 @@
> >  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >  #
> >
> > +import importlib.util
> > +import logging
> >  import os
> >  import sys
> >  import tempfile
> > @@ -25,7 +27,7 @@
> >  import random
> >  import subprocess
> >  import glob
> > -from typing import List, Dict, Any, Optional, ContextManager
> > +from typing import List, Dict, Any, Optional, ContextManager, cast
> >
> >  DEF_GDB_OPTIONS = 'localhost:12345'
> >
> > @@ -112,6 +114,22 @@ def init_directories(self) -> None:
> >  # Path where qemu goodies live in this source tree.
> >  qemu_srctree_path = Path(__file__, '../../../python').resolve()
> >
> > +# warn if we happen to be able to find 'qemu' packages from an
> > +# unexpected location (i.e. the package is already installed in
> > +# the user's environment)
> > +qemu_spec = importlib.util.find_spec('qemu.qmp')
> > +if qemu_spec:
> > +spec_path = Path(cast(str, qemu_spec.origin))
> > +try:
> > +_ = spec_path.relative_to(qemu_srctree_path)
> > +except ValueError:
> > +self._logger.warning(
> > +"WARNING: 'qemu' package will be imported from
> outside "
> > +"the source tree!")
> > +self._logger.warning(
> > +"Importing from: '%s'",
> > +spec_path.parents[1])
>
> It feels to me like the scenario  we're blocking here is actally
> the scenario we want to aim for.
>
>
It isn't blocking it, it's just a warning. At present, iotests expects that
it's using the version of the python packages that are in the source tree /
tarball / whatever. Since I converted those packages to be installable to
your environment, I introduced an ambiguity about which version iotests is
actually using: the version you installed, or the version in the source
tree? This is just merely a warning to let you know that iotests is
technically doing someth

Re: [PATCH 2/6] iotests: add warning for rogue 'qemu' packages

[John Snow]

On Thu, Sep 23, 2021 at 7:17 AM Kevin Wolf  wrote:

> Am 23.09.2021 um 12:57 hat Kevin Wolf geschrieben:
> > Am 23.09.2021 um 02:16 hat John Snow geschrieben:
> > > Add a warning for when 'iotests' runs against a qemu namespace that
> > > isn't the one in the source tree. This might occur if you have
> > > (accidentally) installed the Python namespace package to your local
> > > packages.
> > >
> > > (I'm not going to say that this is because I bit myself with this,
> > > but. You can fill in the blanks.)
> > >
> > > Signed-off-by: John Snow 
> > > ---
> > >  tests/qemu-iotests/testenv.py | 21 -
> > >  1 file changed, 20 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tests/qemu-iotests/testenv.py
> b/tests/qemu-iotests/testenv.py
> > > index 88104dace90..8a43b193af5 100644
> > > --- a/tests/qemu-iotests/testenv.py
> > > +++ b/tests/qemu-iotests/testenv.py
> > > @@ -16,6 +16,8 @@
> > >  # along with this program.  If not, see <http://www.gnu.org/licenses/
> >.
> > >  #
> > >
> > > +import importlib.util
> > > +import logging
> > >  import os
> > >  import sys
> > >  import tempfile
> > > @@ -25,7 +27,7 @@
> > >  import random
> > >  import subprocess
> > >  import glob
> > > -from typing import List, Dict, Any, Optional, ContextManager
> > > +from typing import List, Dict, Any, Optional, ContextManager, cast
> > >
> > >  DEF_GDB_OPTIONS = 'localhost:12345'
> > >
> > > @@ -112,6 +114,22 @@ def init_directories(self) -> None:
> > >  # Path where qemu goodies live in this source tree.
> > >  qemu_srctree_path = Path(__file__,
> '../../../python').resolve()
> > >
> > > +# warn if we happen to be able to find 'qemu' packages from an
> > > +# unexpected location (i.e. the package is already installed
> in
> > > +# the user's environment)
> > > +qemu_spec = importlib.util.find_spec('qemu.qmp')
> > > +if qemu_spec:
> > > +spec_path = Path(cast(str, qemu_spec.origin))
> >
> > You're casting Optional[str] to str here. The source type is not obvious
> > from the local code, so unless you spend some time actively figuring it
> > out, this could be casting anything to str. But even knowing this, just
> > casting Optional away looks fishy anyway.
> >
> > Looking up the ModuleSpec docs, it seems okay to assume that it's never
> > None in our case, but wouldn't it be much cleaner to just 'assert
> > qemu_spec.origin' first and then use it without the cast?
> >
>

OK. I suppose I was thinking: "It's always going to be a string, and if it
isn't, something else will explode below, surely, so the assertion is
redundant", but I don't want code that makes people wonder, so principle of
least surprise it is.


> > > +try:
> > > +_ = spec_path.relative_to(qemu_srctree_path)
> > > +except ValueError:
> > > +self._logger.warning(
> > > +"WARNING: 'qemu' package will be imported from
> outside "
> > > +"the source tree!")
> > > +self._logger.warning(
> > > +"Importing from: '%s'",
> > > +spec_path.parents[1])
> > > +
> > >  self.pythonpath = os.getenv('PYTHONPATH')
> > >  self.pythonpath = os.pathsep.join(filter(None, (
> > >  self.source_iotests,
> > > @@ -231,6 +249,7 @@ def __init__(self, imgfmt: str, imgproto: str,
> aiomode: str,
> > >
> > >  self.build_root = os.path.join(self.build_iotests, '..', '..')
> > >
> > > +self._logger = logging.getLogger('qemu.iotests')
> > >  self.init_directories()
> > >  self.init_binaries()
> >
> > Looks good otherwise.
>
> Well, actually there is a major problem: We'll pass the right PYTHONPATH
> for the test scripts that we're calling, but this script itself doesn't
> use it yet. So what I get is:
>
> Traceback (most recent call last):
>   File "/home/kwolf/source/qemu/tests/qemu-iotests/build/check", line 120,
> in 
> env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
>   File "/home/kwolf/source/qemu/tests/qemu-iotests/testenv.py", line 253,
> in __init__
> self.init_directories()
>   File "/home/kwolf/source/qemu/tests/qemu-iotests/testenv.py", line 120,
> in init_directories
> qemu_spec = importlib.util.find_spec('qemu.qmp')
>   File "/usr/lib64/python3.9/importlib/util.py", line 94, in find_spec
> parent = __import__(parent_name, fromlist=['__path__'])
> ModuleNotFoundError: No module named 'qemu'
>
> Kevin
>
>
Tch. So, it turns out with namespace packages that once you remove the
subpackages (pip uninstall qemu), it leaves the empty namespace folder
behind. This is also the reason I directly use 'qemu.qmp' as a bellwether
here, to make sure we're prodding a package and not just an empty namespace
with nothing in it.

I'll fix these, thanks.

[PATCH v2 12/17] python/machine: Handle QMP errors on close more meticulously

[John Snow]

To use the AQMP backend, Machine just needs to be a little more diligent
about what happens when closing a QMP connection. The operation is no
longer a freebie in the async world; it may return errors encountered in
the async bottom half on incoming message receipt, etc.

(AQMP's disconnect, ultimately, serves as the quiescence point where all
async contexts are gathered together, and any final errors reported at
that point.)

Because async QMP continues to check for messages asynchronously, it's
almost certainly likely that the loop will have exited due to EOF after
issuing the last 'quit' command. That error will ultimately be bubbled
up when attempting to close the QMP connection. The manager class here
then is free to discard it -- if it was expected.

Signed-off-by: John Snow 

---

Yes, I regret that this class has become quite a dumping ground for
complexity around the exit path. It's in need of a refactor to help
separate out the exception handling and cleanup mechanisms from the
VM-related stuff, but it's not a priority to do that just yet -- but
it's on the list.

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

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 0bd40bc2f76..c33a78a2d9f 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -342,9 +342,15 @@ def _post_shutdown(self) -> None:
 # Comprehensive reset for the failed launch case:
 self._early_cleanup()
 
-if self._qmp_connection:
-self._qmp.close()
-self._qmp_connection = None
+try:
+self._close_qmp_connection()
+except Exception as err:  # pylint: disable=broad-except
+LOG.warning(
+"Exception closing QMP connection: %s",
+str(err) if str(err) else type(err).__name__
+)
+finally:
+assert self._qmp_connection is None
 
 self._close_qemu_log_file()
 
@@ -420,6 +426,31 @@ def _launch(self) -> None:
close_fds=False)
 self._post_launch()
 
+def _close_qmp_connection(self) -> None:
+"""
+Close the underlying QMP connection, if any.
+
+Dutifully report errors that occurred while closing, but assume
+that any error encountered indicates an abnormal termination
+process and not a failure to close.
+"""
+if self._qmp_connection is None:
+return
+
+try:
+self._qmp.close()
+except EOFError:
+# EOF can occur as an Exception here when using the Async
+# QMP backend. It indicates that the server closed the
+# stream. If we successfully issued 'quit' at any point,
+# then this was expected. If the remote went away without
+# our permission, it's worth reporting that as an abnormal
+# shutdown case.
+if not self._quit_issued:
+raise
+finally:
+self._qmp_connection = None
+
 def _early_cleanup(self) -> None:
 """
 Perform any cleanup that needs to happen before the VM exits.
@@ -460,9 +491,14 @@ def _soft_shutdown(self, timeout: Optional[int]) -> None:
 self._early_cleanup()
 
 if self._qmp_connection:
-if not self._quit_issued:
-# Might raise ConnectionReset
-self.qmp('quit')
+try:
+if not self._quit_issued:
+# May raise ExecInterruptedError or StateError if the
+# connection dies or has *already* died.
+self.qmp('quit')
+finally:
+# Regardless, we want to quiesce the connection.
+self._close_qmp_connection()
 
 # May raise subprocess.TimeoutExpired
 self._subp.wait(timeout=timeout)
-- 
2.31.1

[PATCH v2 11/17] python/machine: remove has_quit argument

[John Snow]

If we spy on the QMP commands instead, we don't need callers to remember
to pass it. Seems like a fair trade-off.

The one slightly weird bit is overloading this instance variable for
wait(), where we use it to mean "don't issue the qmp 'quit'
command". This means that wait() will "fail" if the QEMU process does
not terminate of its own accord.

In most cases, we probably did already actually issue quit -- some
iotests do this -- but in some others, we may be waiting for QEMU to
terminate for some other reason, such as a test wherein we tell the
guest (directly) to shut down.

Signed-off-by: John Snow 
---
 python/qemu/machine/machine.py | 34 +++---
 tests/qemu-iotests/040 |  7 +--
 tests/qemu-iotests/218 |  2 +-
 tests/qemu-iotests/255 |  2 +-
 4 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 056d340e355..0bd40bc2f76 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -170,6 +170,7 @@ def __init__(self,
 self._console_socket: Optional[socket.socket] = None
 self._remove_files: List[str] = []
 self._user_killed = False
+self._quit_issued = False
 
 def __enter__(self: _T) -> _T:
 return self
@@ -368,6 +369,7 @@ def _post_shutdown(self) -> None:
 command = ''
 LOG.warning(msg, -int(exitcode), command)
 
+self._quit_issued = False
 self._user_killed = False
 self._launched = False
 
@@ -443,15 +445,13 @@ def _hard_shutdown(self) -> None:
 self._subp.kill()
 self._subp.wait(timeout=60)
 
-def _soft_shutdown(self, timeout: Optional[int],
-   has_quit: bool = False) -> None:
+def _soft_shutdown(self, timeout: Optional[int]) -> None:
 """
 Perform early cleanup, attempt to gracefully shut down the VM, and wait
 for it to terminate.
 
 :param timeout: Timeout in seconds for graceful shutdown.
 A value of None is an infinite wait.
-:param has_quit: When True, don't attempt to issue 'quit' QMP command
 
 :raise ConnectionReset: On QMP communication errors
 :raise subprocess.TimeoutExpired: When timeout is exceeded waiting for
@@ -460,21 +460,19 @@ def _soft_shutdown(self, timeout: Optional[int],
 self._early_cleanup()
 
 if self._qmp_connection:
-if not has_quit:
+if not self._quit_issued:
 # Might raise ConnectionReset
-self._qmp.cmd('quit')
+self.qmp('quit')
 
 # May raise subprocess.TimeoutExpired
 self._subp.wait(timeout=timeout)
 
-def _do_shutdown(self, timeout: Optional[int],
- has_quit: bool = False) -> None:
+def _do_shutdown(self, timeout: Optional[int]) -> None:
 """
 Attempt to shutdown the VM gracefully; fallback to a hard shutdown.
 
 :param timeout: Timeout in seconds for graceful shutdown.
 A value of None is an infinite wait.
-:param has_quit: When True, don't attempt to issue 'quit' QMP command
 
 :raise AbnormalShutdown: When the VM could not be shut down gracefully.
 The inner exception will likely be ConnectionReset or
@@ -482,13 +480,13 @@ def _do_shutdown(self, timeout: Optional[int],
 may result in its own exceptions, likely subprocess.TimeoutExpired.
 """
 try:
-self._soft_shutdown(timeout, has_quit)
+self._soft_shutdown(timeout)
 except Exception as exc:
 self._hard_shutdown()
 raise AbnormalShutdown("Could not perform graceful shutdown") \
 from exc
 
-def shutdown(self, has_quit: bool = False,
+def shutdown(self,
  hard: bool = False,
  timeout: Optional[int] = 30) -> None:
 """
@@ -498,7 +496,6 @@ def shutdown(self, has_quit: bool = False,
 If the VM has not yet been launched, or shutdown(), wait(), or kill()
 have already been called, this method does nothing.
 
-:param has_quit: When true, do not attempt to issue 'quit' QMP command.
 :param hard: When true, do not attempt graceful shutdown, and
  suppress the SIGKILL warning log message.
 :param timeout: Optional timeout in seconds for graceful shutdown.
@@ -512,7 +509,7 @@ def shutdown(self, has_quit: bool = False,
 self._user_killed = True
 self._hard_shutdown()
 else:
-self._do_shutdown(timeout, has_quit)
+self._do_shutdown(timeout)
 finally:
 self._post_shutdown()
 
@@ -529,7 +526,8 @@ def wait(se

[PATCH v2 15/17] python/aqmp: Create sync QMP wrapper for iotests

[John Snow]

This is a wrapper around the async QMPClient that mimics the old,
synchronous QEMUMonitorProtocol class. It is designed to be
interchangeable with the old implementation.

It does not, however, attempt to mimic Exception compatibility.

Signed-off-by: John Snow 
Acked-by: Hanna Reitz 
---
 python/qemu/aqmp/legacy.py | 135 +
 1 file changed, 135 insertions(+)
 create mode 100644 python/qemu/aqmp/legacy.py

diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
new file mode 100644
index 000..a75dbc599c0
--- /dev/null
+++ b/python/qemu/aqmp/legacy.py
@@ -0,0 +1,135 @@
+"""
+Sync QMP Wrapper
+
+This class pretends to be qemu.qmp.QEMUMonitorProtocol.
+"""
+
+import asyncio
+from typing import (
+Awaitable,
+List,
+Optional,
+TypeVar,
+Union,
+)
+
+import qemu.qmp
+from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT
+
+from .qmp_client import QMPClient
+
+
+# pylint: disable=missing-docstring
+
+
+class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol):
+def __init__(self, address: SocketAddrT,
+ server: bool = False,
+ nickname: Optional[str] = None):
+
+# pylint: disable=super-init-not-called
+self._aqmp = QMPClient(nickname)
+self._aloop = asyncio.get_event_loop()
+self._address = address
+self._timeout: Optional[float] = None
+
+_T = TypeVar('_T')
+
+def _sync(
+self, future: Awaitable[_T], timeout: Optional[float] = None
+) -> _T:
+return self._aloop.run_until_complete(
+asyncio.wait_for(future, timeout=timeout)
+)
+
+def _get_greeting(self) -> Optional[QMPMessage]:
+if self._aqmp.greeting is not None:
+# pylint: disable=protected-access
+return self._aqmp.greeting._asdict()
+return None
+
+# __enter__ and __exit__ need no changes
+# parse_address needs no changes
+
+def connect(self, negotiate: bool = True) -> Optional[QMPMessage]:
+self._aqmp.await_greeting = negotiate
+self._aqmp.negotiate = negotiate
+
+self._sync(
+self._aqmp.connect(self._address)
+)
+return self._get_greeting()
+
+def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
+self._aqmp.await_greeting = True
+self._aqmp.negotiate = True
+
+self._sync(
+self._aqmp.accept(self._address),
+timeout
+)
+
+ret = self._get_greeting()
+assert ret is not None
+return ret
+
+def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage:
+return dict(
+self._sync(
+# pylint: disable=protected-access
+
+# _raw() isn't a public API, because turning off
+# automatic ID assignment is discouraged. For
+# compatibility with iotests *only*, do it anyway.
+self._aqmp._raw(qmp_cmd, assign_id=False),
+self._timeout
+)
+)
+
+# Default impl of cmd() delegates to cmd_obj
+
+def command(self, cmd: str, **kwds: object) -> QMPReturnValue:
+return self._sync(
+self._aqmp.execute(cmd, kwds),
+self._timeout
+)
+
+def pull_event(self,
+   wait: Union[bool, float] = False) -> Optional[QMPMessage]:
+if wait is False:
+# Return None if there's no event ready to go
+if self._aqmp.events.empty():
+return None
+
+timeout = None
+if isinstance(wait, float):
+timeout = wait
+
+return dict(
+self._sync(
+self._aqmp.events.get(),
+timeout
+)
+)
+
+def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]:
+events = [dict(x) for x in self._aqmp.events.clear()]
+if events:
+return events
+
+event = self.pull_event(wait)
+return [event] if event is not None else []
+
+def clear_events(self) -> None:
+self._aqmp.events.clear()
+
+def close(self) -> None:
+self._sync(
+self._aqmp.disconnect()
+)
+
+def settimeout(self, timeout: Optional[float]) -> None:
+self._timeout = timeout
+
+def send_fd_scm(self, fd: int) -> None:
+self._aqmp.send_fd_scm(fd)
-- 
2.31.1

[PATCH v2 09/17] python/qmp: add send_fd_scm directly to QEMUMonitorProtocol

[John Snow]

It turns out you can do this directly from Python ... and because of
this, you don't need to worry about setting the inheritability of the
fds or spawning another process.

Doing this is helpful because it allows QEMUMonitorProtocol to keep its
file descriptor and socket object as private implementation
details. /that/ is helpful in turn because it allows me to write a
compatible, alternative implementation.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
 python/qemu/machine/machine.py | 44 +++---
 python/qemu/qmp/__init__.py| 21 +++-
 2 files changed, 18 insertions(+), 47 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index ae945ca3c94..1c6532a3d68 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -213,48 +213,22 @@ def add_fd(self: _T, fd: int, fdset: int,
 def send_fd_scm(self, fd: Optional[int] = None,
 file_path: Optional[str] = None) -> int:
 """
-Send an fd or file_path to socket_scm_helper.
+Send an fd or file_path to the remote via SCM_RIGHTS.
 
-Exactly one of fd and file_path must be given.
-If it is file_path, the helper will open that file and pass its own fd.
+Exactly one of fd and file_path must be given.  If it is
+file_path, the file will be opened read-only and the new file
+descriptor will be sent to the remote.
 """
-# In iotest.py, the qmp should always use unix socket.
-assert self._qmp.is_scm_available()
-if self._socket_scm_helper is None:
-raise QEMUMachineError("No path to socket_scm_helper set")
-if not os.path.exists(self._socket_scm_helper):
-raise QEMUMachineError("%s does not exist" %
-   self._socket_scm_helper)
-
-# This did not exist before 3.4, but since then it is
-# mandatory for our purpose
-if hasattr(os, 'set_inheritable'):
-os.set_inheritable(self._qmp.get_sock_fd(), True)
-if fd is not None:
-os.set_inheritable(fd, True)
-
-fd_param = ["%s" % self._socket_scm_helper,
-"%d" % self._qmp.get_sock_fd()]
-
 if file_path is not None:
 assert fd is None
-fd_param.append(file_path)
+with open(file_path, "rb") as passfile:
+fd = passfile.fileno()
+self._qmp.send_fd_scm(fd)
 else:
 assert fd is not None
-fd_param.append(str(fd))
+self._qmp.send_fd_scm(fd)
 
-proc = subprocess.run(
-fd_param,
-stdin=subprocess.DEVNULL,
-stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT,
-check=False,
-close_fds=False,
-)
-if proc.stdout:
-LOG.debug(proc.stdout)
-
-return proc.returncode
+return 0
 
 @staticmethod
 def _remove_if_exists(path: str) -> None:
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
index c27594b66a2..358c0971d06 100644
--- a/python/qemu/qmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -21,6 +21,7 @@
 import json
 import logging
 import socket
+import struct
 from types import TracebackType
 from typing import (
 Any,
@@ -408,18 +409,14 @@ def settimeout(self, timeout: Optional[float]) -> None:
 raise ValueError(msg)
 self.__sock.settimeout(timeout)
 
-def get_sock_fd(self) -> int:
+def send_fd_scm(self, fd: int) -> None:
 """
-Get the socket file descriptor.
-
-@return The file descriptor number.
-"""
-return self.__sock.fileno()
-
-def is_scm_available(self) -> bool:
+Send a file descriptor to the remote via SCM_RIGHTS.
 """
-Check if the socket allows for SCM_RIGHTS.
+if self.__sock.family != socket.AF_UNIX:
+raise RuntimeError("Can't use SCM_RIGHTS on non-AF_UNIX socket.")
 
-@return True if SCM_RIGHTS is available, otherwise False.
-"""
-return self.__sock.family == socket.AF_UNIX
+self.__sock.sendmsg(
+[b' '],
+[(socket.SOL_SOCKET, socket.SCM_RIGHTS, struct.pack('@i', fd))]
+)
-- 
2.31.1

[PATCH v2 14/17] iotests: Conditionally silence certain AQMP errors

[John Snow]

AQMP likes to be very chatty about errors it encounters. In general,
this is good because it allows us to get good diagnostic information for
otherwise complex async failures.

For example, during a failed QMP connection attempt, we might see:

+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Negotiation failed: EOFError
+ERROR:qemu.aqmp.qmp_client.qemub-2536319:Failed to establish session: EOFError

This might be nice in iotests output, because failure scenarios
involving the new QMP library will be spelled out plainly in the output
diffs.

For tests that are intentionally causing this scenario though, filtering
that log output could be a hassle. For now, add a context manager that
simply lets us toggle this output off during a critical region.

(Additionally, a forthcoming patch allows the use of either legacy or
async QMP to be toggled with an environment variable. In this
circumstance, we can't amend the iotest output to just always expect the
error message, either. Just suppress it for now. More rigorous log
filtering can be investigated later if/when it is deemed safe to
permanently replace the legacy QMP library.)

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 20 +++-
 tests/qemu-iotests/tests/mirror-top-perms |  9 +
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 9afa258a405..4d39b86ed85 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,7 +30,7 @@
 import subprocess
 import sys
 import time
-from typing import (Any, Callable, Dict, Iterable,
+from typing import (Any, Callable, Dict, Iterable, Iterator,
 List, Optional, Sequence, TextIO, Tuple, Type, TypeVar)
 import unittest
 
@@ -116,6 +116,24 @@
 sample_img_dir = os.environ['SAMPLE_IMG_DIR']
 
 
+@contextmanager
+def change_log_level(
+logger_name: str, level: int = logging.CRITICAL) -> Iterator[None]:
+"""
+Utility function for temporarily changing the log level of a logger.
+
+This can be used to silence errors that are expected or uninteresting.
+"""
+_logger = logging.getLogger(logger_name)
+current_level = _logger.level
+_logger.setLevel(level)
+
+try:
+yield
+finally:
+_logger.setLevel(current_level)
+
+
 def unarchive_sample_image(sample, fname):
 sample_fname = os.path.join(sample_img_dir, sample + '.bz2')
 with bz2.open(sample_fname) as f_in, open(fname, 'wb') as f_out:
diff --git a/tests/qemu-iotests/tests/mirror-top-perms 
b/tests/qemu-iotests/tests/mirror-top-perms
index 9fe315e3b01..5a34ec655e2 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -21,7 +21,7 @@
 
 import os
 import iotests
-from iotests import qemu_img
+from iotests import change_log_level, qemu_img
 
 # Import qemu after iotests.py has amended sys.path
 # pylint: disable=wrong-import-order
@@ -100,9 +100,10 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
 self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
 self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
 try:
-self.vm_b.launch()
-print('ERROR: VM B launched successfully, this should not have '
-  'happened')
+with change_log_level('qemu.aqmp'):
+self.vm_b.launch()
+print('ERROR: VM B launched successfully, '
+  'this should not have happened')
 except (qemu.qmp.QMPConnectError, ConnectError):
 assert 'Is another process using the image' in self.vm_b.get_log()
 
-- 
2.31.1

[PATCH v2 08/17] python/qmp: clear events on get_events() call

[John Snow]

All callers in the tree *already* clear the events after a call to
get_events(). Do it automatically instead and update callsites to remove
the manual clear call.

These semantics are quite a bit easier to emulate with async QMP, and
nobody appears to be abusing some emergent properties of what happens if
you decide not to clear them, so let's dial down to the dumber, simpler
thing.

Specifically: callers of clear() right after a call to get_events() are
more likely expressing their desire to not see any events they just
retrieved, whereas callers of clear_events() not in relation to a recent
call to pull_event/get_events are likely expressing their desire to
simply drop *all* pending events straight onto the floor. In the sync
world, this is safe enough; in the async world it's nearly impossible to
promise that nothing happens between getting and clearing the
events.

Making the retrieval also clear the queue is vastly simpler.

Signed-off-by: John Snow 
Reviewed-by: Hanna Reitz 
---
 python/qemu/machine/machine.py | 1 -
 python/qemu/qmp/__init__.py| 6 --
 python/qemu/qmp/qmp_shell.py   | 1 -
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index 34131884a57..ae945ca3c94 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -631,7 +631,6 @@ def get_qmp_events(self, wait: bool = False) -> 
List[QMPMessage]:
 events = self._qmp.get_events(wait=wait)
 events.extend(self._events)
 del self._events[:]
-self._qmp.clear_events()
 return events
 
 @staticmethod
diff --git a/python/qemu/qmp/__init__.py b/python/qemu/qmp/__init__.py
index 269516a79b9..c27594b66a2 100644
--- a/python/qemu/qmp/__init__.py
+++ b/python/qemu/qmp/__init__.py
@@ -361,7 +361,7 @@ def pull_event(self,
 
 def get_events(self, wait: bool = False) -> List[QMPMessage]:
 """
-Get a list of available QMP events.
+Get a list of available QMP events and clear all pending events.
 
 @param wait (bool): block until an event is available.
 @param wait (float): If wait is a float, treat it as a timeout value.
@@ -374,7 +374,9 @@ def get_events(self, wait: bool = False) -> 
List[QMPMessage]:
 @return The list of available QMP events.
 """
 self.__get_events(wait)
-return self.__events
+events = self.__events
+self.__events = []
+return events
 
 def clear_events(self) -> None:
 """
diff --git a/python/qemu/qmp/qmp_shell.py b/python/qemu/qmp/qmp_shell.py
index 337acfce2d2..e7d7eb18f19 100644
--- a/python/qemu/qmp/qmp_shell.py
+++ b/python/qemu/qmp/qmp_shell.py
@@ -381,7 +381,6 @@ def read_exec_command(self) -> bool:
 if cmdline == '':
 for event in self.get_events():
 print(event)
-self.clear_events()
 return True
 
 return self._execute_cmd(cmdline)
-- 
2.31.1

[PATCH v2 07/17] python/aqmp: Disable logging messages by default

[John Snow]

AQMP is a library, and ideally it should not print error diagnostics
unless a user opts into seeing them. By default, Python will print all
WARNING, ERROR or CRITICAL messages to screen if no logging
configuration has been created by a client application.

In AQMP's case, ERROR logging statements are used to report additional
detail about runtime failures that will also eventually be reported to the
client library via an Exception, so these messages should not be
rendered by default.

(Why bother to have them at all, then? In async contexts, there may be
multiple Exceptions and we are only able to report one of them back to
the client application. It is not reasonably easy to predict ahead of
time if one or more of these Exceptions will be squelched. Therefore,
it's useful to log intermediate failures to help make sense of the
ultimate, resulting failure.)

Add a NullHandler that will suppress these messages until a client
application opts into logging via logging.basicConfig or similar. Note
that upon calling basicConfig(), this handler will *not* suppress these
messages from being displayed by the client's configuration.

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

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index ab1782999cf..d1b0e4dc3d3 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -21,6 +21,7 @@
 # This work is licensed under the terms of the GNU GPL, version 2.  See
 # the COPYING file in the top-level directory.
 
+import logging
 import warnings
 
 from .error import AQMPError
@@ -41,6 +42,9 @@
 
 warnings.warn(_WMSG, FutureWarning)
 
+# Suppress logging unless an application engages it.
+logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler())
+
 
 # The order of these fields impact the Sphinx documentation order.
 __all__ = (
-- 
2.31.1

[PATCH v2 16/17] python/aqmp: Remove scary message

[John Snow]

The scary message interferes with the iotests output. Coincidentally, if
iotests works by removing this, then it's good evidence that we don't
really need to scare people away from using it.

Signed-off-by: John Snow 
---
 python/qemu/aqmp/__init__.py | 12 
 1 file changed, 12 deletions(-)

diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py
index d1b0e4dc3d3..880d5b6fa7f 100644
--- a/python/qemu/aqmp/__init__.py
+++ b/python/qemu/aqmp/__init__.py
@@ -22,7 +22,6 @@
 # the COPYING file in the top-level directory.
 
 import logging
-import warnings
 
 from .error import AQMPError
 from .events import EventListener
@@ -31,17 +30,6 @@
 from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient
 
 
-_WMSG = """
-
-The Asynchronous QMP library is currently in development and its API
-should be considered highly fluid and subject to change. It should
-not be used by any other scripts checked into the QEMU tree.
-
-Proceed with caution!
-"""
-
-warnings.warn(_WMSG, FutureWarning)
-
 # Suppress logging unless an application engages it.
 logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler())
 
-- 
2.31.1