[PATCH v2 15/15] 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| 82 ---
 tests/qemu-iotests/297.out|  2 -
 tests/qemu-iotests/linters.py | 14 +-
 3 files changed, 2 insertions(+), 96 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 ee78a627359..000
--- a/tests/qemu-iotests/297
+++ /dev/null
@@ -1,82 +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 .
-
-import os
-import subprocess
-import sys
-from typing import List
-
-import iotests
-import linters
-
-
-# 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:
-linters.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
-
-linters.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']
-
-linters.run_linter('mypy', files, env=env, suppress_output=True)
-
-
-def main() -> None:
-files = linters.get_test_files()
-
-iotests.logger.debug('Files to be checked:')
-iotests.logger.debug(', '.join(sorted(files)))
-
-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)
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 65c4c4e8272..486b7125c1d 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,25 +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,
-) -> None:
+def run_linter(tool: str, args: List[str]) -> 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,
 )
 
 
-- 
2.31.1

[PATCH v2 13/15] 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 
Reviewed-by: Hanna Reitz 
---
 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 46c28fdcda0..65c4c4e8272 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -93,7 +93,9 @@ def show_usage() -> None:
 if sys.argv[1] == '--pylint':
 run_linter('pylint', files)
 elif sys.argv[1] == '--mypy':
-run_linter('mypy', files)
+# mypy bug #9852; disable incremental checking as a workaround.
+args = ['--no-incremental'] + files
+run_linter('mypy', args)
 else:
 print(f"Unrecognized argument: '{sys.argv[1]}'", file=sys.stderr)
 show_usage()
-- 
2.31.1

[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 .
 
 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 a copy of the GNU General Public License
+# along with this program.  If not, se

[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 entry point for linting via Python CI
  iote

Re: [PATCH V5] block/rbd: implement bdrv_co_block_status

[Kevin Wolf]

Am 12.10.2021 um 17:22 hat Peter Lieven geschrieben:
> the qemu rbd driver currently lacks support for bdrv_co_block_status.
> This results mainly in incorrect progress during block operations (e.g.
> qemu-img convert with an rbd image as source).
> 
> This patch utilizes the rbd_diff_iterate2 call from librbd to detect
> allocated and unallocated (all zero areas).
> 
> To avoid querying the ceph OSDs for the answer this is only done if
> the image has the fast-diff feature which depends on the object-map and
> exclusive-lock features. In this case it is guaranteed that the information
> is present in memory in the librbd client and thus very fast.
> 
> If fast-diff is not available all areas are reported to be allocated
> which is the current behaviour if bdrv_co_block_status is not implemented.
> 
> Signed-off-by: Peter Lieven 

Thanks, applied to the block branch.

Kevin

Re: [PATCH] block: Fail gracefully when blockdev-snapshot creates loops

[Kevin Wolf]

Am 18.10.2021 um 16:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 18.10.2021 16:47, Kevin Wolf wrote:
> > Using blockdev-snapshot to append a node as an overlay to itself, or to
> > any of its parents, causes crashes. Catch the condition and return an
> > error for these cases instead.
> > 
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1824363
> > Signed-off-by: Kevin Wolf 
> > ---
> >   block.c| 10 ++
> >   tests/qemu-iotests/085 | 31 ++-
> >   tests/qemu-iotests/085.out | 33 ++---
> >   3 files changed, 70 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 45f653a88b..231dddf655 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -84,6 +84,9 @@ static BlockDriverState *bdrv_open_inherit(const char 
> > *filename,
> >  BdrvChildRole child_role,
> >  Error **errp);
> > +static bool bdrv_recurse_has_child(BlockDriverState *bs,
> > +   BlockDriverState *child);
> > +
> >   static void bdrv_replace_child_noperm(BdrvChild *child,
> > BlockDriverState *new_bs);
> >   static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
> > @@ -2673,6 +2676,7 @@ static void bdrv_replace_child_noperm(BdrvChild 
> > *child,
> >   int drain_saldo;
> >   assert(!child->frozen);
> > +assert(old_bs != new_bs);
> >   if (old_bs && new_bs) {
> >   assert(bdrv_get_aio_context(old_bs) == 
> > bdrv_get_aio_context(new_bs));
> > @@ -2892,6 +2896,12 @@ static int bdrv_attach_child_noperm(BlockDriverState 
> > *parent_bs,
> >   assert(parent_bs->drv);
> > +if (bdrv_recurse_has_child(child_bs, parent_bs)) {
> > +error_setg(errp, "Making '%s' a %s child of '%s' would create a 
> > cycle",
> > +   parent_bs->node_name, child_name, child_bs->node_name);
> 
> Seems, child_bs and parent_bs should be swapped.

Oops, thanks. I'm fixing it up while applying.

Kevin

Re: [PATCH v2] block/file-posix: Fix return value translation for AIO discards.

[Kevin Wolf]

Am 19.10.2021 um 13:09 hat Ari Sundholm geschrieben:
> AIO discards regressed as a result of the following commit:
>   0dfc7af2 block/file-posix: Optimize for macOS
> 
> When trying to run blkdiscard within a Linux guest, the request would
> fail, with some errors in dmesg:
> 
>  [ snip ] 
> [4.010070] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK
> driverbyte=DRIVER_SENSE
> [4.011061] sd 2:0:0:0: [sda] tag#0 Sense Key : Aborted Command
> [current]
> [4.011061] sd 2:0:0:0: [sda] tag#0 Add. Sense: I/O process
> terminated
> [4.011061] sd 2:0:0:0: [sda] tag#0 CDB: Unmap/Read sub-channel 42
> 00 00 00 00 00 00 00 18 00
> [4.011061] blk_update_request: I/O error, dev sda, sector 0
>  [ snip ] 
> 
> This turns out to be a result of a flaw in changes to the error value
> translation logic in handle_aiocb_discard(). The default return value
> may be left untranslated in some configurations, and the wrong variable
> is used in one translation.
> 
> Fix both issues.
> 
> Signed-off-by: Ari Sundholm 
> Signed-off-by: Emil Karlson 
> Reviewed-by: Akihiko Odaki 
> Reviewed-by: Stefan Hajnoczi 
> 
> Cc: qemu-sta...@nongnu.org
> Fixes: 0dfc7af2b28 ("block/file-posix: Optimize for macOS")

Thanks, applied to the block branch.

Kevin

[PATCH v2] block/file-posix: Fix return value translation for AIO discards.

[Ari Sundholm]

AIO discards regressed as a result of the following commit:
0dfc7af2 block/file-posix: Optimize for macOS

When trying to run blkdiscard within a Linux guest, the request would
fail, with some errors in dmesg:

 [ snip ] 
[4.010070] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK
driverbyte=DRIVER_SENSE
[4.011061] sd 2:0:0:0: [sda] tag#0 Sense Key : Aborted Command
[current]
[4.011061] sd 2:0:0:0: [sda] tag#0 Add. Sense: I/O process
terminated
[4.011061] sd 2:0:0:0: [sda] tag#0 CDB: Unmap/Read sub-channel 42
00 00 00 00 00 00 00 18 00
[4.011061] blk_update_request: I/O error, dev sda, sector 0
 [ snip ] 

This turns out to be a result of a flaw in changes to the error value
translation logic in handle_aiocb_discard(). The default return value
may be left untranslated in some configurations, and the wrong variable
is used in one translation.

Fix both issues.

Signed-off-by: Ari Sundholm 
Signed-off-by: Emil Karlson 
Reviewed-by: Akihiko Odaki 
Reviewed-by: Stefan Hajnoczi 

Cc: qemu-sta...@nongnu.org
Fixes: 0dfc7af2b28 ("block/file-posix: Optimize for macOS")
---

v1 -> v2:
* Add Reviewed-by, Cc to qemu-stable and Fixes lines

 block/file-posix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 53be0bdc1b..6def2a4cba 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1807,7 +1807,7 @@ static int handle_aiocb_copy_range(void *opaque)
 static int handle_aiocb_discard(void *opaque)
 {
 RawPosixAIOData *aiocb = opaque;
-int ret = -EOPNOTSUPP;
+int ret = -ENOTSUP;
 BDRVRawState *s = aiocb->bs->opaque;
 
 if (!s->has_discard) {
@@ -1829,7 +1829,7 @@ static int handle_aiocb_discard(void *opaque)
 #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
 ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
aiocb->aio_offset, aiocb->aio_nbytes);
-ret = translate_err(-errno);
+ret = translate_err(ret);
 #elif defined(__APPLE__) && (__MACH__)
 fpunchhole_t fpunchhole;
 fpunchhole.fp_flags = 0;
-- 
2.31.1

Re: [PATCH v2 12/15] virtio-net: Store failover primary opts pointer locally

[Kevin Wolf]

Am 19.10.2021 um 10:06 hat Laurent Vivier geschrieben:
> On 08/10/2021 15:34, Kevin Wolf wrote:
> > Instead of accessing the global QemuOptsList, which really belong to the
> > command line parser and shouldn't be accessed from devices, store a
> > pointer to the QemuOpts in a new VirtIONet field.
> > 
> > This is not the final state, but just an intermediate step to get rid of
> > QemuOpts in devices. It will later be replaced with an options QDict.
> > 
> > Before this patch, two "primary" devices could be hidden for the same
> > standby device, but only one of them would actually be enabled and the
> > other one would be kept hidden forever, so this doesn't make sense.
> > After this patch, configuring a second primary device is an error.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >   include/hw/virtio/virtio-net.h |  1 +
> >   hw/net/virtio-net.c| 26 ++
> >   2 files changed, 19 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> > index 824a69c23f..d118c95f69 100644
> > --- a/include/hw/virtio/virtio-net.h
> > +++ b/include/hw/virtio/virtio-net.h
> > @@ -209,6 +209,7 @@ struct VirtIONet {
> >   bool failover_primary_hidden;
> >   bool failover;
> >   DeviceListener primary_listener;
> > +QemuOpts *primary_opts;
> >   Notifier migration_state;
> >   VirtioNetRssData rss_data;
> >   struct NetRxPkt *rx_pkt;
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index a17d5739fc..ed9a9012e9 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -858,27 +858,24 @@ static DeviceState 
> > *failover_find_primary_device(VirtIONet *n)
> >   static void failover_add_primary(VirtIONet *n, Error **errp)
> >   {
> >   Error *err = NULL;
> > -QemuOpts *opts;
> > -char *id;
> >   DeviceState *dev = failover_find_primary_device(n);
> >   if (dev) {
> >   return;
> >   }
> > -id = failover_find_primary_device_id(n);
> > -if (!id) {
> > +if (!n->primary_opts) {
> >   error_setg(errp, "Primary device not found");
> >   error_append_hint(errp, "Virtio-net failover will not work. Make "
> > "sure primary device has parameter"
> > " failover_pair_id=%s\n", n->netclient_name);
> >   return;
> >   }
> > -opts = qemu_opts_find(qemu_find_opts("device"), id);
> > -g_assert(opts); /* cannot be NULL because id was found using opts list 
> > */
> > -dev = qdev_device_add(opts, &err);
> > +
> > +dev = qdev_device_add(n->primary_opts, &err);
> >   if (err) {
> > -qemu_opts_del(opts);
> > +qemu_opts_del(n->primary_opts);
> > +n->primary_opts = NULL;
> >   } else {
> >   object_unref(OBJECT(dev));
> >   }
> > @@ -3317,6 +3314,19 @@ static bool 
> > failover_hide_primary_device(DeviceListener *listener,
> >   return false;
> >   }
> > +if (n->primary_opts) {
> > +error_setg(errp, "Cannot attach more than one primary device to 
> > '%s'",
> > +   n->netclient_name);
> > +return false;
> > +}
> > +
> 
> This part has introduced a regression, I've sent a patch to fix that.
> 
> https://patchew.org/QEMU/20211019071532.682717-1-lviv...@redhat.com/

Thanks for catching this! The fix looks good to me.

Kevin

Re: [PATCH] block/file-posix: Fix return value translation for AIO discards.

[Stefan Hajnoczi]

On Tue, Oct 19, 2021 at 05:40:13AM +0200, Philippe Mathieu-Daudé wrote:
> +Stefan
> 
> On 10/18/21 20:07, Ari Sundholm wrote:
> > AIO discards regressed as a result of the following commit:
> > 0dfc7af2 block/file-posix: Optimize for macOS
> > 
> > When trying to run blkdiscard within a Linux guest, the request would
> > fail, with some errors in dmesg:
> > 
> >  [ snip ] 
> > [4.010070] sd 2:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_OK
> > driverbyte=DRIVER_SENSE
> > [4.011061] sd 2:0:0:0: [sda] tag#0 Sense Key : Aborted Command
> > [current]
> > [4.011061] sd 2:0:0:0: [sda] tag#0 Add. Sense: I/O process
> > terminated
> > [4.011061] sd 2:0:0:0: [sda] tag#0 CDB: Unmap/Read sub-channel 42
> > 00 00 00 00 00 00 00 18 00
> > [4.011061] blk_update_request: I/O error, dev sda, sector 0
> >  [ snip ] 
> > 
> > This turns out to be a result of a flaw in changes to the error value
> > translation logic in handle_aiocb_discard(). The default return value
> > may be left untranslated in some configurations, and the wrong variable
> > is used in one translation.
> > 
> > Fix both issues.
> 
> Worth including:
> 
> Cc: qemu-sta...@nongnu.org
> Fixes: 0dfc7af2b28 ("block/file-posix: Optimize for macOS")
> 
> > Signed-off-by: Ari Sundholm 
> > Signed-off-by: Emil Karlson 
> > ---
> >  block/file-posix.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 53be0bdc1b..6def2a4cba 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -1807,7 +1807,7 @@ static int handle_aiocb_copy_range(void *opaque)
> >  static int handle_aiocb_discard(void *opaque)
> >  {
> >  RawPosixAIOData *aiocb = opaque;
> > -int ret = -EOPNOTSUPP;
> > +int ret = -ENOTSUP;
> >  BDRVRawState *s = aiocb->bs->opaque;
> >  
> >  if (!s->has_discard) {
> > @@ -1829,7 +1829,7 @@ static int handle_aiocb_discard(void *opaque)
> >  #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> >  ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | 
> > FALLOC_FL_KEEP_SIZE,
> > aiocb->aio_offset, aiocb->aio_nbytes);
> > -ret = translate_err(-errno);
> > +ret = translate_err(ret);
> >  #elif defined(__APPLE__) && (__MACH__)
> >  fpunchhole_t fpunchhole;
> >  fpunchhole.fp_flags = 0;
> > 
> 

Thanks for the fix!

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [PULL 37/40] monitor: Tidy up find_device_state()

[Markus Armbruster]

Markus Armbruster  writes:

> Christian Borntraeger  writes:
>
>> Am 13.10.21 um 11:07 schrieb Paolo Bonzini:
>>> From: Markus Armbruster 
>>> Commit 6287d827d4 "monitor: allow device_del to accept QOM paths"
>>> extended find_device_state() to accept QOM paths in addition to qdev
>>> IDs.  This added a checked conversion to TYPE_DEVICE at the end, which
>>> duplicates the check done for the qdev ID case earlier, except it sets
>>> a *different* error: GenericError "ID is not a hotpluggable device"
>>> when passed a QOM path, and DeviceNotFound "Device 'ID' not found"
>>> when passed a qdev ID.  Fortunately, the latter won't happen as long
>>> as we add only devices to /machine/peripheral/.
>>> Earlier, commit b6cc36abb2 "qdev: device_del: Search for to be
>>> unplugged device in 'peripheral' container" rewrote the lookup by qdev
>>> ID to use QOM instead of qdev_find_recursive(), so it can handle
>>> buss-less devices.  It does so by constructing an absolute QOM path.
>>> Works, but object_resolve_path_component() is easier.  Switching to it
>>> also gets rid of the unclean duplication described above.
>>> While there, avoid converting to TYPE_DEVICE twice, first to check
>>> whether it's possible, and then for real.
>>
>> This one broke qemu iotest 280 on s390:
>>
>>
>> 280   fail   [13:06:19] [13:06:19]   0.3s   (last: 0.3s)  output 
>> mismatch (see 280.out.bad)
>> --- /home/cborntra/REPOS/qemu/tests/qemu-iotests/280.out
>> +++ 280.out.bad
>> @@ -37,14 +37,14 @@
>>  === Resume the VM and simulate a write request ===
>>  {"execute": "cont", "arguments": {}}
>>  {"return": {}}
>> -{"return": ""}
>> +{"return": "Error: Device 'vda/virtio-backend' not found\r\n"}
>>
>>  === Commit it to the backing file ===
>>  {"execute": "block-commit", "arguments": {"auto-dismiss": false, "device": 
>> "top-fmt", "job-id": "job0", "top-node": "top-fmt"}}
>>  {"return": {}}
>>  {"execute": "job-complete", "arguments": {"id": "job0"}}
>>  {"return": {}}
>> -{"data": {"device": "job0", "len": 65536, "offset": 65536, "speed": 0, 
>> "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": 
>> "USECS", "seconds": "SECS"}}
>> -{"data": {"device": "job0", "len": 65536, "offset": 65536, "speed": 0, 
>> "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
>> {"microseconds": "USECS", "seconds": "SECS"}}
>> +{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": 
>> "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": 
>> "USECS", "seconds": "SECS"}}
>> +{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": 
>> "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": 
>> "USECS", "seconds": "SECS"}}
>>  {"execute": "job-dismiss", "arguments": {"id": "job0"}}
>>  {"return": {}}
>> Failures: 280
>> Failed 1 of 1 iotests
>
> Reproduced.  I'll dig deeper.  Thanks!

Classical case of failing to adhere to "read only the code": reading the
documentation lodged "this is a qdev ID" in my brain, blinding me to the
fact that the code actually treats it as a QOM path relative to
/machine/peripheral/.

Sorry!

Please try "[PATCH] monitor: Fix find_device_state() for IDs containing
slashes".

Re: [PATCH v2 12/15] virtio-net: Store failover primary opts pointer locally

[Laurent Vivier]

On 08/10/2021 15:34, Kevin Wolf wrote:

Instead of accessing the global QemuOptsList, which really belong to the
command line parser and shouldn't be accessed from devices, store a
pointer to the QemuOpts in a new VirtIONet field.

This is not the final state, but just an intermediate step to get rid of
QemuOpts in devices. It will later be replaced with an options QDict.

Before this patch, two "primary" devices could be hidden for the same
standby device, but only one of them would actually be enabled and the
other one would be kept hidden forever, so this doesn't make sense.
After this patch, configuring a second primary device is an error.

Signed-off-by: Kevin Wolf 
---
  include/hw/virtio/virtio-net.h |  1 +
  hw/net/virtio-net.c| 26 ++
  2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 824a69c23f..d118c95f69 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -209,6 +209,7 @@ struct VirtIONet {
  bool failover_primary_hidden;
  bool failover;
  DeviceListener primary_listener;
+QemuOpts *primary_opts;
  Notifier migration_state;
  VirtioNetRssData rss_data;
  struct NetRxPkt *rx_pkt;
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a17d5739fc..ed9a9012e9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -858,27 +858,24 @@ static DeviceState 
*failover_find_primary_device(VirtIONet *n)
  static void failover_add_primary(VirtIONet *n, Error **errp)
  {
  Error *err = NULL;
-QemuOpts *opts;
-char *id;
  DeviceState *dev = failover_find_primary_device(n);
  
  if (dev) {

  return;
  }
  
-id = failover_find_primary_device_id(n);

-if (!id) {
+if (!n->primary_opts) {
  error_setg(errp, "Primary device not found");
  error_append_hint(errp, "Virtio-net failover will not work. Make "
"sure primary device has parameter"
" failover_pair_id=%s\n", n->netclient_name);
  return;
  }
-opts = qemu_opts_find(qemu_find_opts("device"), id);
-g_assert(opts); /* cannot be NULL because id was found using opts list */
-dev = qdev_device_add(opts, &err);
+
+dev = qdev_device_add(n->primary_opts, &err);
  if (err) {
-qemu_opts_del(opts);
+qemu_opts_del(n->primary_opts);
+n->primary_opts = NULL;
  } else {
  object_unref(OBJECT(dev));
  }
@@ -3317,6 +3314,19 @@ static bool failover_hide_primary_device(DeviceListener 
*listener,
  return false;
  }
  
+if (n->primary_opts) {

+error_setg(errp, "Cannot attach more than one primary device to '%s'",
+   n->netclient_name);
+return false;
+}
+


This part has introduced a regression, I've sent a patch to fix that.

https://patchew.org/QEMU/20211019071532.682717-1-lviv...@redhat.com/

Thanks,
Laurent