Re: [PATCH v4 03/24] python: create qemu packages

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:35PM -0500, John Snow wrote:
> move python/qemu/*.py to python/qemu/[machine, qmp]/*.py and update import
> directives across the tree.
> 
> This is done to create a PEP420 namespace package, in which we may
> create subpackages. To do this, the namespace directory ("qemu") should
> not have any modules in it. Those files will go into new 'machine' and 'qmp'
> subpackages instead.
> 
> Implement machine/__init__.py making the top-level classes and functions
> from its various modules available directly inside the package. Change
> qmp.py to qmp/__init__.py similarly, such that all of the useful QMP
> library classes are available directly from "qemu.qmp" instead of
> "qemu.qmp.qmp".
> 
> Signed-off-by: John Snow 
> 
> 
> ---
> 
> Note for reviewers: in the next patch, I add a utils sub-package and
> move qemu/machine/accel.py to qemu/utils/accel.py. If we like it that
> way, we can squash it in here if we want, or just leave it as its own
> follow-up patch, I am just leaving it as something that will be easy to
> un-do or change for now.

IMO this is fine as it is.

> 
> Signed-off-by: John Snow 
> ---
>  python/{qemu => }/.isort.cfg|  0
>  python/qemu/__init__.py | 11 --
>  python/qemu/{ => machine}/.flake8   |  0
>  python/qemu/machine/__init__.py | 41 +
>  python/qemu/{ => machine}/accel.py  |  0
>  python/qemu/{ => machine}/console_socket.py |  0
>  python/qemu/{ => machine}/machine.py| 16 +---
>  python/qemu/{ => machine}/pylintrc  |  0
>  python/qemu/{ => machine}/qtest.py  |  3 +-
>  python/qemu/{qmp.py => qmp/__init__.py} | 12 +-
>  tests/acceptance/boot_linux.py  |  3 +-
>  tests/qemu-iotests/300  |  4 +-
>  tests/qemu-iotests/iotests.py   |  2 +-
>  tests/vm/basevm.py  |  3 +-
>  14 files changed, 70 insertions(+), 25 deletions(-)
>  rename python/{qemu => }/.isort.cfg (100%)
>  delete mode 100644 python/qemu/__init__.py
>  rename python/qemu/{ => machine}/.flake8 (100%)
>  create mode 100644 python/qemu/machine/__init__.py
>  rename python/qemu/{ => machine}/accel.py (100%)
>  rename python/qemu/{ => machine}/console_socket.py (100%)
>  rename python/qemu/{ => machine}/machine.py (98%)
>  rename python/qemu/{ => machine}/pylintrc (100%)
>  rename python/qemu/{ => machine}/qtest.py (99%)
>  rename python/qemu/{qmp.py => qmp/__init__.py} (96%)
> 
> diff --git a/python/qemu/.isort.cfg b/python/.isort.cfg
> similarity index 100%
> rename from python/qemu/.isort.cfg
> rename to python/.isort.cfg
> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
> deleted file mode 100644
> index 4ca06c34a41..000
> --- a/python/qemu/__init__.py
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -# QEMU library
> -#
> -# Copyright (C) 2015-2016 Red Hat Inc.
> -# Copyright (C) 2012 IBM Corp.
> -#
> -# Authors:
> -#  Fam Zheng 
> -#
> -# This work is licensed under the terms of the GNU GPL, version 2.  See
> -# the COPYING file in the top-level directory.
> -#
> diff --git a/python/qemu/.flake8 b/python/qemu/machine/.flake8
> similarity index 100%
> rename from python/qemu/.flake8
> rename to python/qemu/machine/.flake8
> diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
> new file mode 100644
> index 000..27b0b19abd3
> --- /dev/null
> +++ b/python/qemu/machine/__init__.py
> @@ -0,0 +1,41 @@
> +"""
> +QEMU development and testing library.
> +
> +This library provides a few high-level classes for driving QEMU from a
> +test suite, not intended for production use.
> +
> +- QEMUMachine: Configure and Boot a QEMU VM
> + - QEMUQtestMachine: VM class, with a qtest socket.
> +
> +- QEMUQtestProtocol: Connect to, send/receive qtest messages.
> +
> +- list_accel: List available accelerators
> +- kvm_available: Probe for KVM support
> +- tcg_available: Probe for TCG support
> +"""
> +
> +# Copyright (C) 2020 John Snow for Red Hat Inc.
> +# Copyright (C) 2015-2016 Red Hat Inc.
> +# Copyright (C) 2012 IBM Corp.
> +#
> +# Authors:
> +#  John Snow 
> +#  Fam Zheng 
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2.  See
> +# the COPYING file in the top-level directory.
> +#
> +
> +from .accel import kvm_available, list_accel, tcg_available
> +from .machine import QEMUMachine
> +from .qtest import QEMUQtestMachine, QEMUQtestProtocol
> +
> +
> +__all__ = (
> +'list_accel',
> +'kvm_available',
> +'tcg_available',
> +'QEMUMachine',
> +'QEMUQtestProtocol',
> +'QEMUQtestMachine',
> +)
> diff --git a/python/qemu/accel.py b/python/qemu/machine/accel.py
> similarity index 100%
> rename from python/qemu/accel.py
> rename to python/qemu/machine/accel.py
> diff --git a/python/qemu/console_socket.py 
> b/python/qemu/machine/console_socket.py
> similarity index 100%
> rename from python/qemu/console_socket.py
> rename to 

Re: [PATCH v4 02/24] iotests/297: add --namespace-packages to mypy arguments

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:34PM -0500, John Snow wrote:
> mypy is kind of weird about how it handles imports. For legacy reasons,
> it won't load PEP 420 namespaces, because of logic implemented prior to
> that becoming a standard.
> 
> So, if you plan on using any, you have to pass
> --namespace-packages. Alright, fine.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/297 | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 01/24] python/console_socket: avoid one-letter variable

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:33PM -0500, John Snow wrote:
> Fixes pylint warnings.
> 
> Signed-off-by: John Snow 
> ---
>  python/qemu/console_socket.py | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Cleber Rosa 


signature.asc
Description: PGP signature

Re: [PATCH v4 00/24] python: create installable package

[Cleber Rosa]

On Thu, Feb 11, 2021 at 01:58:32PM -0500, John Snow wrote:
> This series factors the python/qemu directory as an installable
> package. It does not yet actually change the mechanics of how any other
> python source in the tree actually consumes it (yet), beyond the import
> path. (some import statements change in a few places.)
> 
> git: https://gitlab.com/jsnow/qemu/-/commits/python-package-mk3
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/254940717
> (New CI job: https://gitlab.com/jsnow/qemu/-/jobs/1024230604)
> 
> The primary motivation of this series is primarily to formalize our
> dependencies on mypy, flake8, isort, and pylint alongside versions that
> are known to work. It does this using the setup.cfg and setup.py
> files. It also adds explicitly pinned versions (using Pipfile.lock) of
> these dependencies that should behave in a repeatable and known way for
> developers and CI environments both. Lastly, it enables those CI checks
> such that we can enforce Python coding quality checks via the CI tests.
> 
> An auxiliary motivation is that this package is formatted in such a way
> that it COULD be uploaded to https://pypi.org/project/qemu and installed
> independently of qemu.git with `pip install qemu`, but that button
> remains *unpushed* and this series *will not* cause any such
> releases. We have time to debate finer points like API guarantees and
> versioning even after this series is merged.
> 
> Some other things this enables that might be of interest:
> 
> With the python tooling as a proper package, you can install this
> package in editable or production mode to a virtual environment, your
> local user environment, or your system packages. The primary benefit of
> this is to gain access to QMP tooling regardless of CWD, without needing
> to battle sys.path (and confounding other python analysis tools).
> 
> For example: when developing, you may go to qemu/python/ and run `make
> venv` followed by `pipenv shell` to activate a virtual environment that
> contains the qemu python packages. These packages will always reflect
> the current version of the source files in the tree. When you are
> finished, you can simply exit the shell (^d) to remove these packages
> from your python environment.
> 
> When not developing, you could install a version of this package to your
> environment outright to gain access to the QMP and QEMUMachine classes
> for lightweight scripting and testing by using pip: "pip install [--user] ."
> 
> TESTING THIS SERIES:
> 
> First of all, nothing should change. Without any intervention,
> everything should behave exactly as it was before. The only new
> information here comes from how to interact with and run the linters
> that will be enforcing code quality standards in this subdirectory.
> 
> To test those, CD to qemu/python first, and then:
> 
> 1. Try "make venv && pipenv shell" to get a venv with the package
> installed to it in editable mode. Ctrl+d exits this venv shell. While in
> this shell, any python script that uses "from qemu.[qmp|machine] import
> ..." should work correctly regardless of where the script is, or what
> your CWD is.
>

Ack here, works as expected.

> You will need Python 3.6 installed on your system to do this step. For
> Fedora: "dnf install python3.6" will do the trick.
>

You may have explained this before, so forgive me asking again.  Why
is this dependent on a given Python version, and not a *minimum*
Python version? Are the checkers themselves susceptible to different
behavior depending on the Python version used?  If so, any hint on the
strategy for developing with say, Python 3.8, and then having issues
caught only on Python 3.6?

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

Works as expected.  I'll provide more feedback at the specific patches.

> 3. Having exited the shell from above, try "make venv-check". This will
> create and update the venv if needed, then run 'make check' within the
> context of that shell. It should pass as long as the above did.
>

If this makes into a documentation (or on a v5), you may just want to
tell users to run "deactivate" instead of exiting the shell completely.

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

This is good info for completeness, but I wonder if "make check"
should exist at all.  If it's a requirement for "make check-venv", 

Re: [PATCH 2/6] Python: expose QEMUMachine's temporary directory

[Cleber Rosa]

On Fri, Feb 12, 2021 at 12:35:26AM +0100, Philippe Mathieu-Daudé wrote:
> On 2/11/21 11:01 PM, Cleber Rosa wrote:
> > Each instance of qemu.machine.QEMUMachine currently has a "test
> > directory", which may not have any relation to a "test", and it's
> > really a temporary directory.
> > 
> > Users instantiating the QEMUMachine class will be able to set the
> > location of the directory that will *contain* the QEMUMachine unique
> > temporary directory, so that parameter name has been changed from
> > test_dir to base_temp_dir.
> > 
> > A property has been added to allow users to access it without using
> > private attributes, and with that, the directory is created on first
> > use of the property.
> > 
> > Signed-off-by: Cleber Rosa 
> > ---
> >  python/qemu/machine.py | 24 
> >  python/qemu/qtest.py   |  6 +++---
> >  tests/acceptance/virtio-gpu.py |  2 +-
> >  tests/qemu-iotests/iotests.py  |  2 +-
> >  4 files changed, 21 insertions(+), 13 deletions(-)
> > 
> > diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> > index 6e44bda337..b379fcbe72 100644
> > --- a/python/qemu/machine.py
> > +++ b/python/qemu/machine.py
> > @@ -84,7 +84,7 @@ class QEMUMachine:
> >   args: Sequence[str] = (),
> >   wrapper: Sequence[str] = (),
> >   name: Optional[str] = None,
> > - test_dir: str = "/var/tmp",
> > + base_temp_dir: str = "/var/tmp",
> 
> Not this patch fault, but I see we use /var/tmp since commit
> 66613974468 ("scripts: refactor the VM class in iotests for reuse").
> Can we use an OS agnostic method to get temp storage directory instead?
> 

Sounds like a reasonable thing to do.  I do remember a few issues with
Python's tempfile.gettempdir() returning '/tmp' on most Linux/Unix,
dur to the fact that '/tmp' is a tmpfs filesystem on most modern Linux
systems.

Anyway, I agree that hardcoding '/var/tmp' needs to be reconsidered.

Cheers,
- Cleber.


signature.asc
Description: PGP signature

Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Philippe Mathieu-Daudé]

On 2/11/21 11:40 PM, Eric Blake wrote:
> On 2/11/21 8:26 AM, Philippe Mathieu-Daudé wrote:
>> We are going to switch the 'null-co' default 'read-zeroes' value
>> from FALSE to TRUE in the next commit. First explicit the FALSE
>> value when it is not set.
> 
> Grammar suggestion, along with a suggestion for an additional sentence
> to make the intent of this commit clearer:
> 
> As a first step, request an explicit FALSE value rather than relying on
> the defaults.  This is intended to be a purely mechanical adjustment for
> no performance behavior in the tests; later patches may then flip or
> elide the explicit choice for tests where performance does not matter.
> 
>>
>> Suggested-by: Eric Blake 
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>> - Missing: 056 & 155. I couldn't figure out the proper syntax,
>>   any help welcomed...
> 
> 056 - looks like just one line:
> self.vm =
> iotests.VM().add_drive_raw("file=blkdebug::null-co://,id=drive0,align=65536,driver=blkdebug")
> 
> the best way to add it here would be rewriting that line to use blockdev
> syntax rather than blkdebug: URI syntax.  The other question is whether
> it is a noticeable time difference when the default is flipped in 2/2.
> 
> 155 - looks like several uses such as:
> 
> class TestBlockdevMirrorForcedBacking(MirrorBaseClass):
> cmd = 'blockdev-mirror'
> existing = True
> target_backing = None
> target_blockdev_backing = { 'driver': 'null-co' }
> target_real_backing = 'null-co://'
> 
> 
>> - I'm unsure about 162, this doesn't seem to use the null-co
>>   driver but rather testing global syntax.
> 
> Concur; I don't see any reason to worry about this one (but mentioning
> it in the commit message can't hurt in case someone asks later)
> 
> # blkdebug expects all of its arguments to be strings, but its
> # bdrv_refresh_filename() implementation should not assume that they
> have been
> # passed as strings in the original options QDict.
> # So this should emit blkdebug:42:null-co:// as the filename:
> touch 42
> $QEMU_IMG info 'json:{"driver": "blkdebug", "config": 42,
>   "image.driver": "null-co"}' \
> 
> 
>> ---
>>  docs/devel/testing.rst | 14 +++---
>>  tests/qtest/fuzz/generic_fuzz_configs.h| 11 ++-
>>  tests/test-bdrv-drain.c| 10 --
>>  tests/acceptance/virtio_check_params.py|  2 +-
>>  tests/perf/block/qcow2/convert-blockstatus |  6 +++---
>>  tests/qemu-iotests/040 |  2 +-
> 
> You did a pretty good hunt for culprits!
> 
>>  tests/qemu-iotests/041 | 12 
>>  tests/qemu-iotests/051 |  2 +-
>>  tests/qemu-iotests/051.out |  2 +-
>>  tests/qemu-iotests/051.pc.out  |  4 ++--
> 
> and for the fallout to the iotests.
> 
> I did not audit for which tests are easy candidates for dropping the
> explicit read-zeroes=false (that is, where the extra time in allowing
> the flipped default doesn't penalize the test), but am okay giving this
> patch:
> 
> Reviewed-by: Eric Blake 

Thanks for your help. I'll address your comments and respin.

Regards,

Phil.

Re: [PATCH 6/6] tests/acceptance/virtio-gpu.py: preserve virtio-user-gpu log

[Philippe Mathieu-Daudé]

On 2/11/21 11:01 PM, Cleber Rosa wrote:

"Preserve log" ...
> At location already prepared for keeping the test's log files.
> 
> While at it, log info about its location (in the main test log
> file), instead of printing it out.
> 
> Reference: 
> https://avocado-framework.readthedocs.io/en/85.0/api/test/avocado.html#avocado.Test.logdir
> Signed-off-by: Cleber Rosa 
> ---
>  tests/acceptance/virtio-gpu.py | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Using full sentence:
Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 2/6] Python: expose QEMUMachine's temporary directory

[Philippe Mathieu-Daudé]

On 2/11/21 11:01 PM, Cleber Rosa wrote:
> Each instance of qemu.machine.QEMUMachine currently has a "test
> directory", which may not have any relation to a "test", and it's
> really a temporary directory.
> 
> Users instantiating the QEMUMachine class will be able to set the
> location of the directory that will *contain* the QEMUMachine unique
> temporary directory, so that parameter name has been changed from
> test_dir to base_temp_dir.
> 
> A property has been added to allow users to access it without using
> private attributes, and with that, the directory is created on first
> use of the property.
> 
> Signed-off-by: Cleber Rosa 
> ---
>  python/qemu/machine.py | 24 
>  python/qemu/qtest.py   |  6 +++---
>  tests/acceptance/virtio-gpu.py |  2 +-
>  tests/qemu-iotests/iotests.py  |  2 +-
>  4 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 6e44bda337..b379fcbe72 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -84,7 +84,7 @@ class QEMUMachine:
>   args: Sequence[str] = (),
>   wrapper: Sequence[str] = (),
>   name: Optional[str] = None,
> - test_dir: str = "/var/tmp",
> + base_temp_dir: str = "/var/tmp",

Not this patch fault, but I see we use /var/tmp since commit
66613974468 ("scripts: refactor the VM class in iotests for reuse").
Can we use an OS agnostic method to get temp storage directory instead?

Re: [PATCH v2 3/4] utils: Deprecate hex-with-suffix sizes

[Philippe Mathieu-Daudé]

On 2/11/21 9:44 PM, Eric Blake wrote:
> Supporting '0x20M' looks odd, particularly since we have a 'B' suffix
> that is ambiguous for bytes, as well as a less-frequently-used 'E'
> suffix for extremely large exibytes.  In practice, people using hex
> inputs are specifying values in bytes (and would have written
> 0x200, or possibly relied on default_suffix in the case of
> qemu_strtosz_MiB), and the use of scaling suffixes makes the most
> sense for inputs in decimal (where the user would write 32M).  But
> rather than outright dropping support for hex-with-suffix, let's
> follow our deprecation policy.  Sadly, since qemu_strtosz() does not
> have an Err** parameter, and plumbing that in would be a much larger
> task, we instead go with just directly emitting the deprecation
> warning to stderr.
> 
> Signed-off-by: Eric Blake 
> ---
>  docs/system/deprecated.rst |  8 
>  util/cutils.c  | 10 +-
>  2 files changed, 17 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Eric Blake]

On 2/11/21 8:26 AM, Philippe Mathieu-Daudé wrote:
> We are going to switch the 'null-co' default 'read-zeroes' value
> from FALSE to TRUE in the next commit. First explicit the FALSE
> value when it is not set.

Grammar suggestion, along with a suggestion for an additional sentence
to make the intent of this commit clearer:

As a first step, request an explicit FALSE value rather than relying on
the defaults.  This is intended to be a purely mechanical adjustment for
no performance behavior in the tests; later patches may then flip or
elide the explicit choice for tests where performance does not matter.

> 
> Suggested-by: Eric Blake 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> - Missing: 056 & 155. I couldn't figure out the proper syntax,
>   any help welcomed...

056 - looks like just one line:
self.vm =
iotests.VM().add_drive_raw("file=blkdebug::null-co://,id=drive0,align=65536,driver=blkdebug")

the best way to add it here would be rewriting that line to use blockdev
syntax rather than blkdebug: URI syntax.  The other question is whether
it is a noticeable time difference when the default is flipped in 2/2.

155 - looks like several uses such as:

class TestBlockdevMirrorForcedBacking(MirrorBaseClass):
cmd = 'blockdev-mirror'
existing = True
target_backing = None
target_blockdev_backing = { 'driver': 'null-co' }
target_real_backing = 'null-co://'


> - I'm unsure about 162, this doesn't seem to use the null-co
>   driver but rather testing global syntax.

Concur; I don't see any reason to worry about this one (but mentioning
it in the commit message can't hurt in case someone asks later)

# blkdebug expects all of its arguments to be strings, but its
# bdrv_refresh_filename() implementation should not assume that they
have been
# passed as strings in the original options QDict.
# So this should emit blkdebug:42:null-co:// as the filename:
touch 42
$QEMU_IMG info 'json:{"driver": "blkdebug", "config": 42,
  "image.driver": "null-co"}' \


> ---
>  docs/devel/testing.rst | 14 +++---
>  tests/qtest/fuzz/generic_fuzz_configs.h| 11 ++-
>  tests/test-bdrv-drain.c| 10 --
>  tests/acceptance/virtio_check_params.py|  2 +-
>  tests/perf/block/qcow2/convert-blockstatus |  6 +++---
>  tests/qemu-iotests/040 |  2 +-

You did a pretty good hunt for culprits!

>  tests/qemu-iotests/041 | 12 
>  tests/qemu-iotests/051 |  2 +-
>  tests/qemu-iotests/051.out |  2 +-
>  tests/qemu-iotests/051.pc.out  |  4 ++--

and for the fallout to the iotests.

I did not audit for which tests are easy candidates for dropping the
explicit read-zeroes=false (that is, where the extra time in allowing
the flipped default doesn't penalize the test), but am okay giving this
patch:

Reviewed-by: Eric Blake 


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

[PATCH 5/6] Acceptance Tests: distinguish between temp and logs dir

[Cleber Rosa]

Logs can be very important to debug issues, and currently QEMUMachine
instances will remove logs that are created under the temporary
directories.

With this change, the stdout and stderr generated by the QEMU process
started by QEMUMachine will always be kept along the test results
directory.

Signed-off-by: Cleber Rosa 
---
 python/qemu/machine.py| 16 ++--
 tests/acceptance/avocado_qemu/__init__.py |  3 ++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index b379fcbe72..1f4119e2b4 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -89,7 +89,8 @@ class QEMUMachine:
  socket_scm_helper: Optional[str] = None,
  sock_dir: Optional[str] = None,
  drain_console: bool = False,
- console_log: Optional[str] = None):
+ console_log: Optional[str] = None,
+ log_dir: Optional[str] = None):
 '''
 Initialize a QEMUMachine
 
@@ -103,6 +104,7 @@ class QEMUMachine:
 @param sock_dir: where to create socket (defaults to base_temp_dir)
 @param drain_console: (optional) True to drain console socket to buffer
 @param console_log: (optional) path to console log file
+@param log_dir: where to create and keep log files
 @note: Qemu process is not started until launch() is used.
 '''
 # Direct user configuration
@@ -114,6 +116,7 @@ class QEMUMachine:
 self._name = name or "qemu-%d" % os.getpid()
 self._base_temp_dir = base_temp_dir
 self._sock_dir = sock_dir or self._base_temp_dir
+self._log_dir = log_dir
 self._socket_scm_helper = socket_scm_helper
 
 if monitor_address is not None:
@@ -303,7 +306,7 @@ class QEMUMachine:
 return args
 
 def _pre_launch(self) -> None:
-self._qemu_log_path = os.path.join(self.temp_dir, self._name + ".log")
+self._qemu_log_path = os.path.join(self.log_dir, self._name + ".log")
 self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
 if self._console_set:
@@ -752,3 +755,12 @@ class QEMUMachine:
 self._temp_dir = tempfile.mkdtemp(prefix="qemu-machine-",
   dir=self._base_temp_dir)
 return self._temp_dir
+
+@property
+def log_dir(self) -> str:
+"""
+Returns a directory to be used for writing logs
+"""
+if self._log_dir is None:
+return self.temp_dir
+return self._log_dir
diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index 94b78fd7c8..ac9be1eb66 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -173,9 +173,10 @@ class Test(avocado.Test):
 def _new_vm(self, name, *args):
 self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
 vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
- sock_dir=self._sd.name)
+ sock_dir=self._sd.name, log_dir=self.logdir)
 self.log.debug('QEMUMachine "%s" created', name)
 self.log.debug('QEMUMachine "%s" temp_dir: %s', name, vm.temp_dir)
+self.log.debug('QEMUMachine "%s" log_dir: %s', name, vm.log_dir)
 if args:
 vm.add_args(*args)
 return vm
-- 
2.25.4

[PATCH 4/6] Acceptance Tests: log information when creating QEMUMachine

[Cleber Rosa]

Including its base temporary directory, given that information useful
for debugging can be put there.

Signed-off-by: Cleber Rosa 
---
 tests/acceptance/avocado_qemu/__init__.py | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index b7ab836455..94b78fd7c8 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -170,10 +170,12 @@ class Test(avocado.Test):
 if self.qemu_bin is None:
 self.cancel("No QEMU binary defined or found in the build tree")
 
-def _new_vm(self, *args):
+def _new_vm(self, name, *args):
 self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
 vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
  sock_dir=self._sd.name)
+self.log.debug('QEMUMachine "%s" created', name)
+self.log.debug('QEMUMachine "%s" temp_dir: %s', name, vm.temp_dir)
 if args:
 vm.add_args(*args)
 return vm
@@ -186,7 +188,7 @@ class Test(avocado.Test):
 if not name:
 name = str(uuid.uuid4())
 if self._vms.get(name) is None:
-self._vms[name] = self._new_vm(*args)
+self._vms[name] = self._new_vm(name, *args)
 if self.machine is not None:
 self._vms[name].set_machine(self.machine)
 return self._vms[name]
-- 
2.25.4

[PATCH 1/6] Python: close the log file kept by QEMUMachine before reading it

[Cleber Rosa]

Closing a file that is open for writing, and then reading from it
sounds like a better idea than the opposite, given that the content
will be flushed.

Reference: https://docs.python.org/3/library/io.html#io.IOBase.close
Signed-off-by: Cleber Rosa 
---
 python/qemu/machine.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 7a40f4604b..6e44bda337 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -337,12 +337,12 @@ class QEMUMachine:
 self._qmp.close()
 self._qmp_connection = None
 
-self._load_io_log()
-
 if self._qemu_log_file is not None:
 self._qemu_log_file.close()
 self._qemu_log_file = None
 
+self._load_io_log()
+
 self._qemu_log_path = None
 
 if self._temp_dir is not None:
-- 
2.25.4

[PATCH 0/6] Python / Acceptance Tests: improve logging

[Cleber Rosa]

The location and amount of information kept while using QEMUMachine in
Acceptance Tests is currently not optimal.

This improves the situation by using the Test's log directory (an
Avocado standard feature) as the default location to keep logs,
instead of the temporary directory currently used.

Users will be able to find "qemu-$PID.log" files under the test log
directories, containing all the stdout/stderr generated by the QEMU
binary.

Cleber Rosa (6):
  Python: close the log file kept by QEMUMachine before reading it
  Python: expose QEMUMachine's temporary directory
  Acceptance Tests: use the job work directory for created VMs
  Acceptance Tests: log information when creating QEMUMachine
  Acceptance Tests: distinguish between temp and logs dir
  tests/acceptance/virtio-gpu.py: preserve virtio-user-gpu log

 python/qemu/machine.py| 42 +--
 python/qemu/qtest.py  |  6 ++--
 tests/acceptance/avocado_qemu/__init__.py | 10 --
 tests/acceptance/virtio-gpu.py|  5 +--
 tests/qemu-iotests/iotests.py |  2 +-
 5 files changed, 45 insertions(+), 20 deletions(-)

-- 
2.25.4

[PATCH 6/6] tests/acceptance/virtio-gpu.py: preserve virtio-user-gpu log

[Cleber Rosa]

At location already prepared for keeping the test's log files.

While at it, log info about its location (in the main test log
file), instead of printing it out.

Reference: 
https://avocado-framework.readthedocs.io/en/85.0/api/test/avocado.html#avocado.Test.logdir
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/virtio-gpu.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/acceptance/virtio-gpu.py b/tests/acceptance/virtio-gpu.py
index 8d689eb820..ab1a4c1a71 100644
--- a/tests/acceptance/virtio-gpu.py
+++ b/tests/acceptance/virtio-gpu.py
@@ -119,10 +119,11 @@ class VirtioGPUx86(Test):
 os.set_inheritable(vug_sock.fileno(), True)
 
 self._vug_log_path = os.path.join(
-self.vm.temp_dir, "vhost-user-gpu.log"
+self.logdir, "vhost-user-gpu.log"
 )
 self._vug_log_file = open(self._vug_log_path, "wb")
-print(self._vug_log_path)
+self.log.info('Complete vhost-user-gpu.log file can be '
+  'found at %s', self._vug_log_path)
 
 vugp = subprocess.Popen(
 [vug, "--virgl", "--fd=%d" % vug_sock.fileno()],
-- 
2.25.4

[PATCH 3/6] Acceptance Tests: use the job work directory for created VMs

[Cleber Rosa]

The QEMUMachine uses a base temporary directory for all temporary
needs.  By setting it to the Avocado's workdir, it's possible to
keep the temporary files during debugging sessions much more
easily by setting the "--keep-tmp" command line option.

Reference: 
https://avocado-framework.readthedocs.io/en/85.0/api/test/avocado.html#avocado.Test.workdir
Reference: 
https://avocado-framework.readthedocs.io/en/85.0/config/index.html#run-keep-tmp
Signed-off-by: Cleber Rosa 
---
 tests/acceptance/avocado_qemu/__init__.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index bf54e419da..b7ab836455 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -172,7 +172,8 @@ class Test(avocado.Test):
 
 def _new_vm(self, *args):
 self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
-vm = QEMUMachine(self.qemu_bin, sock_dir=self._sd.name)
+vm = QEMUMachine(self.qemu_bin, base_temp_dir=self.workdir,
+ sock_dir=self._sd.name)
 if args:
 vm.add_args(*args)
 return vm
-- 
2.25.4

Re: [PATCH] hw/sd: sdhci: Do not transfer any data when command fails

[Alexander Bulekov]

On 210211 1154, Alexander Bulekov wrote:
...
> I applied this along with <20210208193450.2689517-1-f4...@amsat.org>
> "hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress"
> 
> I ran through the entire OSS-Fuzz corpus, and could not reproduce the
> crash.
> 
> Tested-by: Alexander Bulekov 
> 
Hi Bin,
Phil explained to me that this patch should fix the problem independent
of 
"hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress"

With only this patch, there are still crashes. Here are three
reproducers:

Some of these are quite long, so here are pastebins for convenience:
Repro 1: https://paste.debian.net/plain/1185137
Repro 2: https://paste.debian.net/plain/1185141
Repro 3: https://paste.debian.net/plain/1185136

Just wget and pipe them into
 ./qemu-system-i386 -display none -machine accel=qtest -nographic \
-m 512M -nodefaults -device sdhci-pci,sd-spec-version=3 \
-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
-device sd-card,drive=mydrive -qtest stdio

 Repro 1 
cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest -nographic \
-m 512M -nodefaults -device sdhci-pci,sd-spec-version=3 \
-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
-device sd-card,drive=mydrive -qtest stdio
outl 0xcf8 0x80001010
outl 0xcfc 0xfbefff00
outl 0xcf8 0x80001001
outl 0xcfc 0x0600
write 0xfbefff2c 0x1 0x05
write 0xfbefff0f 0x1 0x37
write 0xfbefff0a 0x1 0x01
write 0xfbefff0f 0x1 0x29
write 0xfbefff0f 0x1 0x02
write 0xfbefff0f 0x1 0x03
write 0xfbefff04 0x1 0x01
write 0xfbefff05 0x1 0x01
write 0xfbefff07 0x1 0x02
write 0xfbefff0c 0x1 0x33
write 0xfbefff0e 0x1 0x20
write 0xfbefff0f 0x1 0x00
write 0xfbefff2a 0x1 0x01
write 0xfbefff0c 0x1 0x00
write 0xfbefff03 0x1 0x00
write 0xfbefff05 0x1 0x00
write 0xfbefff2a 0x1 0x02
write 0xfbefff0c 0x1 0x32
write 0xfbefff01 0x1 0x01
write 0xfbefff02 0x1 0x01
write 0xfbefff03 0x1 0x01
EOF

 Stack Trace 1 
==929953==ERROR: AddressSanitizer: heap-buffer-overflow on address 
0x61531880 at pc 0x564cf01ceae7 bp 0x7ffe17361e10 sp 0x7ffe173615d8
READ of size 520027904 at 0x61531880 thread T0
#0 0x564cf01ceae6 in __asan_memcpy 
(/home/alxndr/Development/qemu/build/qemu-system-i386+0x2a8cae6)
#1 0x564cf19111a5 in flatview_write_continue 
/home/alxndr/Development/qemu/build/../softmmu/physmem.c:2781:13
#2 0x564cf1906beb in flatview_write 
/home/alxndr/Development/qemu/build/../softmmu/physmem.c:2816:14
#3 0x564cf1906beb in address_space_write 
/home/alxndr/Development/qemu/build/../softmmu/physmem.c:2908:18
#4 0x564cf096348c in dma_memory_rw_relaxed 
/home/alxndr/Development/qemu/include/sysemu/dma.h:88:12
#5 0x564cf096348c in dma_memory_rw 
/home/alxndr/Development/qemu/include/sysemu/dma.h:127:12
#6 0x564cf096348c in dma_memory_write 
/home/alxndr/Development/qemu/include/sysemu/dma.h:163:12
#7 0x564cf096348c in sdhci_sdma_transfer_multi_blocks 
/home/alxndr/Development/qemu/build/../hw/sd/sdhci.c:619:13
#8 0x564cf097237d in sdhci_write 
/home/alxndr/Development/qemu/build/../hw/sd/sdhci.c:1131:17
#9 0x564cf154333c in memory_region_write_accessor 
/home/alxndr/Development/qemu/build/../softmmu/memory.c:491:5

 Repro 2 

cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest -nographic \
-m 512M -nodefaults -device sdhci-pci,sd-spec-version=3 \
-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
-device sd-card,drive=mydrive -qtest stdio
outl 0xcf8 0x80001013
outl 0xcfc 0x91
outl 0xcf8 0x80001001
outl 0xcfc 0x0600
write 0x912c 0x1 0x05
write 0x910f 0x1 0x37
write 0x910a 0x1 0x01
write 0x910f 0x1 0x29
write 0x910f 0x1 0x02
write 0x910f 0x1 0x03
write 0x0 0x1 0x01
write 0x8 0x1 0x01
write 0x10 0x1 0x01
write 0x18 0x1 0x01
write 0x20 0x1 0x01
write 0x28 0x1 0x01
write 0x30 0x1 0x01
write 0x38 0x1 0x01
write 0x40 0x1 0x01
write 0x48 0x1 0x01
write 0x50 0x1 0x01
write 0x58 0x1 0x01
write 0x60 0x1 0x01
write 0x68 0x1 0x01
write 0x70 0x1 0x01
write 0x9105 0x1 0x02
write 0x9107 0x1 0x20
write 0x78 0x1 0x01
write 0x80 0x1 0x01
write 0x88 0x1 0x01
write 0x90 0x1 0x01
write 0x98 0x1 0x01
write 0xa0 0x1 0x01
write 0xa8 0x1 0x01
write 0xb0 0x1 0x01
write 0xb8 0x1 0x01
write 0xc0 0x1 0x01
write 0x910e 0x1 0x21
write 0x9128 0x1 0x10
write 0x910c 0x1 0x01
write 0x910f 0x1 0x06
write 0xc8 0x1 0x01
write 0xd0 0x1 0x01
write 0xd8 0x1 0x01
write 0xe0 0x1 0x01
write 0xe8 0x1 0x01
write 0xf0 0x1 0x01
write 0xf8 0x1 0x01
write 0x100 0x1 0x01
write 0x108 0x1 0x01
write 0x110 0x1 0x01
write 0x118 0x1 0x01
write 0x120 0x1 0x01
write 0x128 0x1 0x01
write 0x130 0x1 0x01
write 0x138 0x1 0x01
write 0x140 0x1 0x01
write 0x148 0x1 0x01
write 0x150 0x1 0x01
write 0x158 0x1 0x01
write 0x160 0x1 0x01
write 0x168 0x1 0x01
write 0x170 0x1 0x01
write 0x178 0x1 0x01
write 0x180 0x1 0x01
write 0x188 0x1 0x01
write 0x190 0x1 0x01
write 0x198 0x1 0x01
write 0x1a0 0x1 0x01
write 0x1a8 0x1 0x01
write 0x1b0 0x1 0x01
write 

[PATCH v2 3/4] utils: Deprecate hex-with-suffix sizes

[Eric Blake]

Supporting '0x20M' looks odd, particularly since we have a 'B' suffix
that is ambiguous for bytes, as well as a less-frequently-used 'E'
suffix for extremely large exibytes.  In practice, people using hex
inputs are specifying values in bytes (and would have written
0x200, or possibly relied on default_suffix in the case of
qemu_strtosz_MiB), and the use of scaling suffixes makes the most
sense for inputs in decimal (where the user would write 32M).  But
rather than outright dropping support for hex-with-suffix, let's
follow our deprecation policy.  Sadly, since qemu_strtosz() does not
have an Err** parameter, and plumbing that in would be a much larger
task, we instead go with just directly emitting the deprecation
warning to stderr.

Signed-off-by: Eric Blake 
---
 docs/system/deprecated.rst |  8 
 util/cutils.c  | 10 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 2fcac7861e07..113c2e933f1b 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -146,6 +146,14 @@ library enabled as a cryptography provider.
 Neither the ``nettle`` library, or the built-in cryptography provider are
 supported on FIPS enabled hosts.

+hexadecimal sizes with scaling multipliers (since 6.0)
+''
+
+Input parameters that take a size value should only use a size suffix
+(such as 'k' or 'M') when the base is written in decimal, and not when
+the value is hexadecimal.  That is, '0x20M' is deprecated, and should
+be written either as '32M' or as '0x200'.
+
 QEMU Machine Protocol (QMP) commands
 

diff --git a/util/cutils.c b/util/cutils.c
index 22d27b0fd21b..6a8a175e0d71 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -250,6 +250,9 @@ static int64_t suffix_mul(char suffix, int64_t unit)
  *   fractional portion is truncated to byte
  * - 0x7fEE - hexadecimal, unit determined by @default_suffix
  *
+ * The following cause a deprecation warning, and may be removed in the future
+ * - 0xabc{kKmMgGtTpP} - hex with scaling suffix
+ *
  * The following are intentionally not supported
  * - octal, such as 08
  * - fractional hex, such as 0x1.8
@@ -272,7 +275,7 @@ static int do_strtosz(const char *nptr, const char **end,
 int retval;
 const char *endptr, *f;
 unsigned char c;
-bool mul_required = false;
+bool mul_required = false, hex = false;
 uint64_t val;
 int64_t mul;
 double fraction = 0.0;
@@ -298,6 +301,7 @@ static int do_strtosz(const char *nptr, const char **end,
 retval = -EINVAL;
 goto out;
 }
+hex = true;
 } else if (*endptr == '.') {
 /*
  * Input looks like a fraction.  Make sure even 1.k works
@@ -320,6 +324,10 @@ static int do_strtosz(const char *nptr, const char **end,
 c = *endptr;
 mul = suffix_mul(c, unit);
 if (mul > 0) {
+if (hex) {
+warn_report("Using a multiplier suffix on hex numbers "
+"is deprecated: %s", nptr);
+}
 endptr++;
 } else {
 mul = suffix_mul(default_suffix, unit);
-- 
2.30.1

[PATCH v2 2/4] utils: Improve qemu_strtosz() to have 64 bits of precision

[Eric Blake]

We have multiple clients of qemu_strtosz (qemu-io, the opts visitor,
the keyval visitor), and it gets annoying that edge-case testing is
impacted by implicit rounding to 53 bits of precision due to parsing
with strtod().  As an example posted by Rich Jones:
 $ nbdkit memory $(( 2**63 - 2**30 )) --run \
   'build/qemu-io -f raw "$uri" -c "w -P 3 $(( 2**63 - 2**30 - 512 )) 512" '
 write failed: Input/output error

because 9223372035781033472 got rounded to 0x7fffc000 which is
out of bounds.

It is also worth noting that our existing parser, by virtue of using
strtod(), accepts decimal AND hex numbers, even though test-cutils
previously lacked any coverage of the latter until the previous patch.
We do have existing clients that expect a hex parse to work (for
example, iotest 33 using qemu-io -c "write -P 0xa 0x200 0x400"), but
strtod() parses "08" as 8 rather than as an invalid octal number, so
we know there are no clients that depend on octal.  Our use of
strtod() also means that "0x1.8k" would actually parse as 1536 (the
fraction is 8/16), rather than 1843 (if the fraction were 8/10); but
as this was not covered in the testsuite, I have no qualms forbidding
hex fractions as invalid, so this patch declares that the use of
fractions is only supported with decimal input, and enhances the
testsuite to document that.

Our previous use of strtod() meant that -1 parsed as a negative; now
that we parse with strtoull(), negative values can wrap around modulo
2^64, so we have to explicitly check whether the user passed in a '-';
and make it consistent to also reject '-0'.  This has the minor effect
of treating negative values as EINVAL (with no change to endptr)
rather than ERANGE (with endptr advanced to what was parsed), visible
in the updated iotest output.

We also had no testsuite coverage of "1.1e0k", which happened to parse
under strtod() but is unlikely to occur in practice; as long as we are
making things more robust, it is easy enough to reject the use of
exponents in a strtod parse.

The fix is done by breaking the parse into an integer prefix (no loss
in precision), rejecting negative values (since we can no longer rely
on strtod() to do that), determining if a decimal or hexadecimal parse
was intended (with the new restriction that a fractional hex parse is
not allowed), and where appropriate, using a floating point fractional
parse (where we also scan to reject use of exponents in the fraction).
The bulk of the patch is then updates to the testsuite to match our
new precision, as well as adding new cases we reject (whether they
were rejected or inadvertently accepted before).

Signed-off-by: Eric Blake 

---

Note that this approach differs from what has been attempted in the
past; see the thread starting at
https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg00852.html
That approach tried to parse both as strtoull and strtod and take
whichever was longer, but that was harder to document.
---
 tests/test-cutils.c  | 74 ++
 tests/test-keyval.c  | 35 +
 tests/test-qemu-opts.c   | 33 
 util/cutils.c| 90 
 tests/qemu-iotests/049.out   | 14 +++--
 tests/qemu-iotests/178.out.qcow2 |  3 +-
 tests/qemu-iotests/178.out.raw   |  3 +-
 7 files changed, 158 insertions(+), 94 deletions(-)

diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index 6d9802e00b1b..bad3a6099389 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -1978,8 +1978,6 @@ static void test_qemu_strtosz_simple(void)
 g_assert_cmpint(err, ==, 0);
 g_assert_cmpint(res, ==, 12345);

-/* Note: precision is 53 bits since we're parsing with strtod() */
-
 str = "9007199254740991"; /* 2^53-1 */
 err = qemu_strtosz(str, , );
 g_assert_cmpint(err, ==, 0);
@@ -1992,10 +1990,10 @@ static void test_qemu_strtosz_simple(void)
 g_assert_cmpint(res, ==, 0x20);
 g_assert(endptr == str + 16);

-str = "9007199254740993"; /* 2^53+1 FIXME loss of precision is a bug */
+str = "9007199254740993"; /* 2^53+1 */
 err = qemu_strtosz(str, , );
 g_assert_cmpint(err, ==, 0);
-g_assert_cmpint(res, ==, 0x20); /* rounded to 53 bits */
+g_assert_cmpint(res, ==, 0x21);
 g_assert(endptr == str + 16);

 str = "18446744073709549568"; /* 0xf800 (53 msbs set) */
@@ -2004,16 +2002,17 @@ static void test_qemu_strtosz_simple(void)
 g_assert_cmpint(res, ==, 0xf800);
 g_assert(endptr == str + 20);

-str = "18446744073709550591"; /* 0xfbff FIXME */
+str = "18446744073709550591"; /* 0xfbff */
 err = qemu_strtosz(str, , );
 g_assert_cmpint(err, ==, 0);
-g_assert_cmpint(res, ==, 0xf800); /* rounded to 53 bits */
+g_assert_cmpint(res, ==, 0xfbff);
 g_assert(endptr == str + 20);

-/*
- * FIXME 

[PATCH v2 0/4] improve do_strtosz precision

[Eric Blake]

Parsing sizes with only 53 bits of precision is surprising; it's time
to fix it to use a full 64 bits of precision.

v1 was here:
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01800.html

Since then:
- split testsuite improvements from code changes [Vladimir]
- more tests for more corner cases [Vladimir, Rich, Dan]
- fix handling of '123-45' when endptr is non-NULL [Vladimir]
- fix handling of '1.k'
- actually enable deprecation of '0x1k' [Vladimir]
- include missing deprecation text for rounded fractions
- improved commit messages

I'm still not sure I like patch 4, but it's at least worth considering.

Eric Blake (4):
  utils: Enhance testsuite for do_strtosz()
  utils: Improve qemu_strtosz() to have 64 bits of precision
  utils: Deprecate hex-with-suffix sizes
  utils: Deprecate inexact fractional suffix sizes

 docs/system/deprecated.rst   |  17 
 tests/test-cutils.c  | 168 ++-
 tests/test-keyval.c  |  39 ---
 tests/test-qemu-opts.c   |  37 ---
 util/cutils.c| 103 +++
 tests/qemu-iotests/049.out   |  14 ++-
 tests/qemu-iotests/178.out.qcow2 |   3 +-
 tests/qemu-iotests/178.out.raw   |   3 +-
 8 files changed, 305 insertions(+), 79 deletions(-)

-- 
2.30.1

[PATCH v2 1/4] utils: Enhance testsuite for do_strtosz()

[Eric Blake]

Enhance our testsuite coverage of do_strtosz() to cover some things we
know that existing users want to continue working (hex bytes), as well
as some things that accidentally work but shouldn't (hex fractions) or
accidentally fail but that users want to work (64-bit precision on
byte values).  This includes fixing a typo in the comment regarding
our parsing near 2^64.

Signed-off-by: Eric Blake 
---
 tests/test-cutils.c | 154 
 1 file changed, 143 insertions(+), 11 deletions(-)

diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index 1aa8351520ae..6d9802e00b1b 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -1960,11 +1960,19 @@ static void test_qemu_strtosz_simple(void)
 g_assert_cmpint(res, ==, 0);
 g_assert(endptr == str + 1);

-str = "12345";
+/* Leading 0 gives decimal results, not octal */
+str = "08";
+err = qemu_strtosz(str, , );
+g_assert_cmpint(err, ==, 0);
+g_assert_cmpint(res, ==, 8);
+g_assert(endptr == str + 2);
+
+/* Leading space is ignored */
+str = " 12345";
 err = qemu_strtosz(str, , );
 g_assert_cmpint(err, ==, 0);
 g_assert_cmpint(res, ==, 12345);
-g_assert(endptr == str + 5);
+g_assert(endptr == str + 6);

 err = qemu_strtosz(str, NULL, );
 g_assert_cmpint(err, ==, 0);
@@ -1984,7 +1992,7 @@ static void test_qemu_strtosz_simple(void)
 g_assert_cmpint(res, ==, 0x20);
 g_assert(endptr == str + 16);

-str = "9007199254740993"; /* 2^53+1 */
+str = "9007199254740993"; /* 2^53+1 FIXME loss of precision is a bug */
 err = qemu_strtosz(str, , );
 g_assert_cmpint(err, ==, 0);
 g_assert_cmpint(res, ==, 0x20); /* rounded to 53 bits */
@@ -1996,14 +2004,42 @@ static void test_qemu_strtosz_simple(void)
 g_assert_cmpint(res, ==, 0xf800);
 g_assert(endptr == str + 20);

-str = "18446744073709550591"; /* 0xfbff */
+str = "18446744073709550591"; /* 0xfbff FIXME */
 err = qemu_strtosz(str, , );
 g_assert_cmpint(err, ==, 0);
 g_assert_cmpint(res, ==, 0xf800); /* rounded to 53 bits */
 g_assert(endptr == str + 20);

-/* 0x7e00..0x7fff get rounded to
- * 0x8000, thus -ERANGE; see test_qemu_strtosz_erange() */
+/*
+ * FIXME 0xfe00..0x get rounded to
+ * 2^64, thus -ERANGE; see test_qemu_strtosz_erange()
+ */
+}
+
+static void test_qemu_strtosz_hex(void)
+{
+const char *str;
+const char *endptr;
+int err;
+uint64_t res = 0xbaadf00d;
+
+str = "0x0";
+err = qemu_strtosz(str, , );
+g_assert_cmpint(err, ==, 0);
+g_assert_cmpint(res, ==, 0);
+g_assert(endptr == str + 3);
+
+str = "0xab";
+err = qemu_strtosz(str, , );
+g_assert_cmpint(err, ==, 0);
+g_assert_cmpint(res, ==, 171);
+g_assert(endptr == str + 4);
+
+str = "0xae";
+err = qemu_strtosz(str, , );
+g_assert_cmpint(err, ==, 0);
+g_assert_cmpint(res, ==, 174);
+g_assert(endptr == str + 4);
 }

 static void test_qemu_strtosz_units(void)
@@ -2064,14 +2100,36 @@ static void test_qemu_strtosz_units(void)

 static void test_qemu_strtosz_float(void)
 {
-const char *str = "12.345M";
+const char *str;
 int err;
 const char *endptr;
 uint64_t res = 0xbaadf00d;

+str = "0.5E";
 err = qemu_strtosz(str, , );
 g_assert_cmpint(err, ==, 0);
-g_assert_cmpint(res, ==, 12.345 * MiB);
+g_assert_cmpint(res, ==, EiB / 2);
+g_assert(endptr == str + 4);
+
+/* For convenience, a fraction of 0 is tolerated even on bytes */
+str = "1.0B";
+err = qemu_strtosz(str, , );
+g_assert_cmpint(err, ==, 0);
+g_assert_cmpint(res, ==, 1);
+g_assert(endptr == str + 4);
+
+/* An empty fraction is tolerated */
+str = "1.k";
+err = qemu_strtosz(str, , );
+g_assert_cmpint(err, ==, 0);
+g_assert_cmpint(res, ==, 1024);
+g_assert(endptr == str + 3);
+
+/* For convenience, we permit values that are not byte-exact */
+str = "12.345M";
+err = qemu_strtosz(str, , );
+g_assert_cmpint(err, ==, 0);
+g_assert_cmpint(res, ==, (uint64_t) (12.345 * MiB));
 g_assert(endptr == str + 7);
 }

@@ -2106,6 +2164,47 @@ static void test_qemu_strtosz_invalid(void)
 err = qemu_strtosz(str, , );
 g_assert_cmpint(err, ==, -EINVAL);
 g_assert(endptr == str);
+
+/* Fractional values require scale larger than bytes */
+/* FIXME endptr shouldn't move on -EINVAL */
+str = "1.1B";
+err = qemu_strtosz(str, , );
+g_assert_cmpint(err, ==, -EINVAL);
+g_assert(endptr == str + 4);
+
+/* FIXME endptr shouldn't move on -EINVAL */
+str = "1.1";
+err = qemu_strtosz(str, , );
+g_assert_cmpint(err, ==, -EINVAL);
+g_assert(endptr == str + 3);
+
+/* FIXME we should reject floating point exponents */
+str = "1.5e1k";
+err = qemu_strtosz(str, , 

Re: [PATCH 2/2] file-posix: Cache next hole

[Vladimir Sementsov-Ogievskiy]

11.02.2021 20:22, Max Reitz wrote:

We have repeatedly received reports that SEEK_HOLE and SEEK_DATA are
slow on certain filesystems and/or under certain circumstances.  That is
why we generally try to avoid it (which is why bdrv_co_block_status()
has the @want_zero parameter, and which is why qcow2 has a metadata
preallocation detection, so we do not fall through to the protocol layer
to discover which blocks are zero, unless that is really necessary
(i.e., for metadata-preallocated images)).

In addition to those measures, we can also try to speed up zero
detection by letting file-posix cache some hole location information,
namely where the next hole after the most recently queried offset is.
This helps especially for images that are (nearly) fully allocated,
which is coincidentally also the case where querying for zero
information cannot gain us much.

Note that this of course only works so long as we have no concurrent
writers to the image, which is the case when the WRITE capability is not
shared.

Alternatively (or perhaps as an improvement in the future), we could let
file-posix keep track of what it knows is zero and what it knows is
non-zero with bitmaps, which would help images that actually have a
significant number of holes (where this implementation here cannot do
much).  But for such images, SEEK_HOLE/DATA are generally faster (they
do not need to seek through the whole file), and the performance lost by
querying the block status does not feel as bad because it is outweighed
by the performance that can be saved by special-cases zeroed areas, so
focussing on images that are (nearly) fully allocated is more important.

Signed-off-by: Max Reitz 


I'll look at it tomorrow... Just wanted to note that something similar was 
proposed by Kevin some time ago:

<20190124141731.21509-1-kw...@redhat.com>
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06271.html

(o_0 two years ago.. time passes so fast)

--
Best regards,
Vladimir

Re: [PATCH 2/2] travis: remove travis configuration and all references to Travis CI

[Wainer dos Santos Moschetta]

Hi,

On 2/9/21 11:32 AM, Thomas Huth wrote:

On 09/02/2021 14.50, Daniel P. Berrangé wrote:

The Travis CI system QEMU has been using has removed the unlimited free
usage model, replacing it with a one-time only grant of CI minutes that
is not renewed. The QEMU CI jobs quickly exhaust maintainer's free CI
credits, leaving them unable to test with Travis. This is not a
sustainable situation, so we have no choice by to discontinue use of
Travis. GitLab CI is now the primary target, with Cirrus CI filling
in some platform gaps where needed.


I've currently got a series in flight that moves some of the remaining 
jobs to gitlab-CI:


https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01924.html

Could you please hold this patch 'til my series got merged first?

Also I think we could still wait some more weeks with the final 
removal of the travis-CI either 'til travis-ci.org got shut down 
completely (and thus we cannot use it for QEMU at all anymore), or 
until we finally got the s390x and aarch64 runners up and running in 
the gitlab-CI.


It's reasonable to me.

- Wainer




 Thomas

Re: [PATCH] hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress

[Alexander Bulekov]

On 210211 2045, Philippe Mathieu-Daudé wrote:
> Hi Alexander,
> 
> On 2/11/21 6:04 PM, Alexander Bulekov wrote:
> > On 210208 2034, Philippe Mathieu-Daudé wrote:
> >> Per the "SD Host Controller Simplified Specification Version 2.00"
> >> spec. 'Table 2-4 : Block Size Register':
> >>
> >>   Transfer Block Size [...] can be accessed only if no
> >>   transaction is executing (i.e., after a transaction has stopped).
> >>   Read operations during transfers may return an invalid value,
> >>   and write operations shall be ignored.
> >>
> >> Transactions will update 'data_count', so do not modify 'blksize'
> >> and 'blkcnt' when 'data_count' is used. This fixes:
> >>
> >> $ cat << EOF | qemu-system-x86_64 -qtest stdio -monitor none \
> >>-nographic -serial none -M pc-q35-5.0 \
> >>-device sdhci-pci,sd-spec-version=3 \
> >>-device sd-card,drive=mydrive \
> >>-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive
> >>   outl 0xcf8 0x80001810
> >>   outl 0xcfc 0xe1068000
> >>   outl 0xcf8 0x80001814
> >>   outl 0xcf8 0x80001804
> >>   outw 0xcfc 0x7
> >>   outl 0xcf8 0x8000fa20
> >>   write 0xe106802c 0x1 0x0f
> >>   write 0xe1068004 0xc 0x2801d10101fbff28a384
> >>   write 0xe106800c 0x1f 
> >> 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
> >>   write 0xe1068003 0x28 
> >> 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
> >>   write 0xe1068003 0x1 0xfe
> >>   EOF
> >>   =
> >>   ==2686219==ERROR: AddressSanitizer: heap-buffer-overflow on address 
> >> 0x6153bb00 at pc 0x55ab469f456c bp 0x7ffee71be330 sp 0x7ffee71bdae0
> >>   WRITE of size 4 at 0x6153bb00 thread T0
> >>   #0 0x55ab469f456b in __asan_memcpy (qemu-system-i386+0x1cea56b)
> >>   #1 0x55ab483dc396 in stl_he_p include/qemu/bswap.h:353:5
> >>   #2 0x55ab483af5e4 in stn_he_p include/qemu/bswap.h:546:1
> >>   #3 0x55ab483aeb4b in flatview_read_continue softmmu/physmem.c:2839:13
> >>   #4 0x55ab483b0705 in flatview_read softmmu/physmem.c:2877:12
> >>   #5 0x55ab483b028e in address_space_read_full 
> >> softmmu/physmem.c:2890:18
> >>   #6 0x55ab483b1294 in address_space_rw softmmu/physmem.c:2918:16
> >>   #7 0x55ab479374a2 in dma_memory_rw_relaxed include/sysemu/dma.h:88:12
> >>   #8 0x55ab47936f50 in dma_memory_rw include/sysemu/dma.h:127:12
> >>   #9 0x55ab4793665f in dma_memory_read include/sysemu/dma.h:145:12
> >>   #10 0x55ab4792f176 in sdhci_sdma_transfer_multi_blocks 
> >> hw/sd/sdhci.c:639:13
> >>   #11 0x55ab4793dc9d in sdhci_write hw/sd/sdhci.c:1129:17
> >>   #12 0x55ab483f8db8 in memory_region_write_accessor 
> >> softmmu/memory.c:491:5
> >>   #13 0x55ab483f868a in access_with_adjusted_size 
> >> softmmu/memory.c:552:18
> >>   #14 0x55ab483f6da5 in memory_region_dispatch_write 
> >> softmmu/memory.c:1501:16
> >>   #15 0x55ab483c3b11 in flatview_write_continue 
> >> softmmu/physmem.c:2774:23
> >>   #16 0x55ab483b0eb6 in flatview_write softmmu/physmem.c:2814:14
> >>   #17 0x55ab483b0a3e in address_space_write softmmu/physmem.c:2906:18
> >>   #18 0x55ab48465c56 in qtest_process_command softmmu/qtest.c:654:9
> >>
> >>   0x6153bb00 is located 0 bytes to the right of 512-byte region 
> >> [0x6153b900,0x6153bb00)
> >>   allocated by thread T0 here:
> >>   #0 0x55ab469f58a7 in calloc (qemu-system-i386+0x1ceb8a7)
> >>   #1 0x7f21d678f9b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0)
> >>   #2 0x55ab479530ed in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5
> >>   #3 0x55ab476f102a in pci_qdev_realize hw/pci/pci.c:2108:9
> >>   #4 0x55ab48baaad2 in device_set_realized hw/core/qdev.c:761:13
> >>
> >>   SUMMARY: AddressSanitizer: heap-buffer-overflow 
> >> (qemu-system-i386+0x1cea56b) in __asan_memcpy
> >>   Shadow bytes around the buggy address:
> >> 0x0c2a7710: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> >> 0x0c2a7720: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> 0x0c2a7730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> 0x0c2a7740: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> 0x0c2a7750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >>   =>0x0c2a7760:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> >> 0x0c2a7770: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> >> 0x0c2a7780: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> >> 0x0c2a7790: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> >> 0x0c2a77a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> >> 0x0c2a77b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> >>   Shadow byte legend (one shadow byte represents 8 application bytes):
> >> Addressable:   00
> >> Heap left redzone:   fa
> >> Freed heap region:   fd
> >>   ==2686219==ABORTING
> >>
> >> Fixes: 

Re: [PATCH] hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress

[Philippe Mathieu-Daudé]

Hi Alexander,

On 2/11/21 6:04 PM, Alexander Bulekov wrote:
> On 210208 2034, Philippe Mathieu-Daudé wrote:
>> Per the "SD Host Controller Simplified Specification Version 2.00"
>> spec. 'Table 2-4 : Block Size Register':
>>
>>   Transfer Block Size [...] can be accessed only if no
>>   transaction is executing (i.e., after a transaction has stopped).
>>   Read operations during transfers may return an invalid value,
>>   and write operations shall be ignored.
>>
>> Transactions will update 'data_count', so do not modify 'blksize'
>> and 'blkcnt' when 'data_count' is used. This fixes:
>>
>> $ cat << EOF | qemu-system-x86_64 -qtest stdio -monitor none \
>>-nographic -serial none -M pc-q35-5.0 \
>>-device sdhci-pci,sd-spec-version=3 \
>>-device sd-card,drive=mydrive \
>>-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive
>>   outl 0xcf8 0x80001810
>>   outl 0xcfc 0xe1068000
>>   outl 0xcf8 0x80001814
>>   outl 0xcf8 0x80001804
>>   outw 0xcfc 0x7
>>   outl 0xcf8 0x8000fa20
>>   write 0xe106802c 0x1 0x0f
>>   write 0xe1068004 0xc 0x2801d10101fbff28a384
>>   write 0xe106800c 0x1f 
>> 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
>>   write 0xe1068003 0x28 
>> 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
>>   write 0xe1068003 0x1 0xfe
>>   EOF
>>   =
>>   ==2686219==ERROR: AddressSanitizer: heap-buffer-overflow on address 
>> 0x6153bb00 at pc 0x55ab469f456c bp 0x7ffee71be330 sp 0x7ffee71bdae0
>>   WRITE of size 4 at 0x6153bb00 thread T0
>>   #0 0x55ab469f456b in __asan_memcpy (qemu-system-i386+0x1cea56b)
>>   #1 0x55ab483dc396 in stl_he_p include/qemu/bswap.h:353:5
>>   #2 0x55ab483af5e4 in stn_he_p include/qemu/bswap.h:546:1
>>   #3 0x55ab483aeb4b in flatview_read_continue softmmu/physmem.c:2839:13
>>   #4 0x55ab483b0705 in flatview_read softmmu/physmem.c:2877:12
>>   #5 0x55ab483b028e in address_space_read_full softmmu/physmem.c:2890:18
>>   #6 0x55ab483b1294 in address_space_rw softmmu/physmem.c:2918:16
>>   #7 0x55ab479374a2 in dma_memory_rw_relaxed include/sysemu/dma.h:88:12
>>   #8 0x55ab47936f50 in dma_memory_rw include/sysemu/dma.h:127:12
>>   #9 0x55ab4793665f in dma_memory_read include/sysemu/dma.h:145:12
>>   #10 0x55ab4792f176 in sdhci_sdma_transfer_multi_blocks 
>> hw/sd/sdhci.c:639:13
>>   #11 0x55ab4793dc9d in sdhci_write hw/sd/sdhci.c:1129:17
>>   #12 0x55ab483f8db8 in memory_region_write_accessor 
>> softmmu/memory.c:491:5
>>   #13 0x55ab483f868a in access_with_adjusted_size softmmu/memory.c:552:18
>>   #14 0x55ab483f6da5 in memory_region_dispatch_write 
>> softmmu/memory.c:1501:16
>>   #15 0x55ab483c3b11 in flatview_write_continue softmmu/physmem.c:2774:23
>>   #16 0x55ab483b0eb6 in flatview_write softmmu/physmem.c:2814:14
>>   #17 0x55ab483b0a3e in address_space_write softmmu/physmem.c:2906:18
>>   #18 0x55ab48465c56 in qtest_process_command softmmu/qtest.c:654:9
>>
>>   0x6153bb00 is located 0 bytes to the right of 512-byte region 
>> [0x6153b900,0x6153bb00)
>>   allocated by thread T0 here:
>>   #0 0x55ab469f58a7 in calloc (qemu-system-i386+0x1ceb8a7)
>>   #1 0x7f21d678f9b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0)
>>   #2 0x55ab479530ed in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5
>>   #3 0x55ab476f102a in pci_qdev_realize hw/pci/pci.c:2108:9
>>   #4 0x55ab48baaad2 in device_set_realized hw/core/qdev.c:761:13
>>
>>   SUMMARY: AddressSanitizer: heap-buffer-overflow 
>> (qemu-system-i386+0x1cea56b) in __asan_memcpy
>>   Shadow bytes around the buggy address:
>> 0x0c2a7710: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>> 0x0c2a7720: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 0x0c2a7730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 0x0c2a7740: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 0x0c2a7750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>>   =>0x0c2a7760:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>> 0x0c2a7770: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>> 0x0c2a7780: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>> 0x0c2a7790: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>> 0x0c2a77a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>> 0x0c2a77b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>>   Shadow byte legend (one shadow byte represents 8 application bytes):
>> Addressable:   00
>> Heap left redzone:   fa
>> Freed heap region:   fd
>>   ==2686219==ABORTING
>>
>> Fixes: CVE-2020-17380
>> Fixes: CVE-2020-25085
>> Signed-off-by: Philippe Mathieu-Daudé 
> 
> I applied this along with 
> <1612868085-72809-1-git-send-email-bmeng...@gmail.com>
> "hw/sd: sdhci: Do not transfer any data when command fails"
> 
> I ran 

Re: [PATCH] hw/sd: sdhci: Do not transfer any data when command fails

[Philippe Mathieu-Daudé]

On 2/11/21 9:52 AM, Mauro Matteo Cascella wrote:
> Hello,
> 
> On Wed, Feb 10, 2021 at 11:27 PM Alistair Francis  
> wrote:
>>
>> On Tue, Feb 9, 2021 at 2:55 AM Bin Meng  wrote:
>>>
>>> At the end of sdhci_send_command(), it starts a data transfer if
>>> the command register indicates a data is associated. However the
>>> data transfer should only be initiated when the command execution
>>> has succeeded.
>>>
>>> Cc: qemu-sta...@nongnu.org
>>> Fixes: CVE-2020-17380
>>> Fixes: CVE-2020-25085
>>> Reported-by: Alexander Bulekov 
>>> Reported-by: Sergej Schumilo (Ruhr-University Bochum)
>>> Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
>>> Reported-by: Simon Wrner (Ruhr-University Bochum)
>>> Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
>>
>> Isn't this already fixed?

The previous patch was enough to catch the previous reproducer,
but something changed elsewhere making the same reproducer crash
QEMU again...

> It turned out the bug was still reproducible on master. I'm actually
> thinking of assigning a new CVE for this, to make it possible for
> distros to apply this fix.

It sounds fair. Do you have an ETA for the new CVE?

> --
> Mauro Matteo Cascella
> Red Hat Product Security
> PGP-Key ID: BB3410B0
> 
> 

Re: [PATCH 2/2] file-posix: Cache next hole

[Eric Blake]

On 2/11/21 11:22 AM, Max Reitz wrote:
> We have repeatedly received reports that SEEK_HOLE and SEEK_DATA are
> slow on certain filesystems and/or under certain circumstances.  That is
> why we generally try to avoid it (which is why bdrv_co_block_status()
> has the @want_zero parameter, and which is why qcow2 has a metadata
> preallocation detection, so we do not fall through to the protocol layer
> to discover which blocks are zero, unless that is really necessary
> (i.e., for metadata-preallocated images)).
> 
> In addition to those measures, we can also try to speed up zero
> detection by letting file-posix cache some hole location information,
> namely where the next hole after the most recently queried offset is.
> This helps especially for images that are (nearly) fully allocated,
> which is coincidentally also the case where querying for zero
> information cannot gain us much.
> 
> Note that this of course only works so long as we have no concurrent
> writers to the image, which is the case when the WRITE capability is not
> shared.
> 
> Alternatively (or perhaps as an improvement in the future), we could let
> file-posix keep track of what it knows is zero and what it knows is
> non-zero with bitmaps, which would help images that actually have a
> significant number of holes (where this implementation here cannot do
> much).  But for such images, SEEK_HOLE/DATA are generally faster (they
> do not need to seek through the whole file), and the performance lost by
> querying the block status does not feel as bad because it is outweighed
> by the performance that can be saved by special-cases zeroed areas, so
> focussing on images that are (nearly) fully allocated is more important.

focusing

> 
> Signed-off-by: Max Reitz 
> ---
>  block/file-posix.c | 81 +-
>  1 file changed, 80 insertions(+), 1 deletion(-)
> 

>  static int find_allocation(BlockDriverState *bs, off_t start,
> off_t *data, off_t *hole)
>  {
> -#if defined SEEK_HOLE && defined SEEK_DATA
>  BDRVRawState *s = bs->opaque;
> +
> +if (s->next_zero_offset_valid) {
> +if (start >= s->next_zero_offset_from && start < 
> s->next_zero_offset) {
> +*data = start;
> +*hole = s->next_zero_offset;
> +return 0;
> +}
> +}
> +
> +#if defined SEEK_HOLE && defined SEEK_DATA

Why move the #if? If SEEK_HOLE is not defined, s->next_zero_offset_valid
should never be set, because we'll treat the entire image as data.  But
at the same time, it doesn't hurt, so doesn't stop my review.

Reviewed-by: Eric Blake 

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

Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Philippe Mathieu-Daudé]

Hi Vladimir,

On 2/11/21 5:29 PM, Vladimir Sementsov-Ogievskiy wrote:
> 11.02.2021 17:26, Philippe Mathieu-Daudé wrote:
>> We are going to switch the 'null-co' default 'read-zeroes' value
>> from FALSE to TRUE in the next commit. First explicit the FALSE
>> value when it is not set.
>>
>> Suggested-by: Eric Blake 
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>> - Missing: 056 & 155. I couldn't figure out the proper syntax,
>>    any help welcomed...
>> - I'm unsure about 162, this doesn't seem to use the null-co
>>    driver but rather testing global syntax.
>> ---
>>   docs/devel/testing.rst | 14 +++---
>>   tests/qtest/fuzz/generic_fuzz_configs.h    | 11 ++-
>>   tests/test-bdrv-drain.c    | 10 --
>>   tests/acceptance/virtio_check_params.py    |  2 +-
>>   tests/perf/block/qcow2/convert-blockstatus |  6 +++---
>>   tests/qemu-iotests/040 |  2 +-
>>   tests/qemu-iotests/041 | 12 
>>   tests/qemu-iotests/051 |  2 +-
>>   tests/qemu-iotests/051.out |  2 +-
>>   tests/qemu-iotests/051.pc.out  |  4 ++--
>>   tests/qemu-iotests/087 |  6 --
>>   tests/qemu-iotests/118 |  2 +-
>>   tests/qemu-iotests/133 |  2 +-
>>   tests/qemu-iotests/153 |  8 
>>   tests/qemu-iotests/184 |  2 ++
>>   tests/qemu-iotests/184.out | 10 +-
>>   tests/qemu-iotests/218 |  3 +++
>>   tests/qemu-iotests/224 |  3 ++-
>>   tests/qemu-iotests/224.out |  8 
>>   tests/qemu-iotests/225 |  2 +-
>>   tests/qemu-iotests/227 |  4 ++--
>>   tests/qemu-iotests/227.out |  4 ++--
>>   tests/qemu-iotests/228 |  2 +-
>>   tests/qemu-iotests/235 |  1 +
>>   tests/qemu-iotests/245 |  2 +-
>>   tests/qemu-iotests/270 |  2 +-
>>   tests/qemu-iotests/283 |  3 ++-
>>   tests/qemu-iotests/283.out |  4 ++--
>>   tests/qemu-iotests/299 |  1 +
>>   tests/qemu-iotests/299.out |  2 +-
>>   tests/qemu-iotests/300 |  4 ++--
> 
> Why do you think these tests will work bad with new default?

As I don't understand the tests, this was the deal with Eric :)

"OK to change default if current default is explicited" then
block maintainers could audit each case and see which one can
safely use read-zeroes=true.

> 
> At least everything under tests/qemu-iotests/ and tests/test-bdrv-drain
> are not about performance
> 
> tests/perf/block/qcow2/convert-blockstatus is OK with new default too.

OK, I'll see who should send these patches on top with Eric & Max.

Thank for your review,

Phil.

Re: [PATCH 1/2] block/mirror: Fix mirror_top's permissions

[Eric Blake]

On 2/11/21 11:22 AM, Max Reitz wrote:
> mirror_top currently shares all permissions, and takes only the WRITE
> permission (if some parent has taken that permission, too).
> 
> That is wrong, though; mirror_top is a filter, so it should take
> permissions like any other filter does.  For example, if the parent
> needs CONSISTENT_READ, we need to take that, too, and if it cannot share
> the WRITE permission, we cannot share it either.
> 
> The exception is when mirror_top is used for active commit, where we
> cannot take CONSISTENT_READ (because it is deliberately unshared above
> the base node) and where we must share WRITE (so that it is shared for
> all images in the backing chain, so the mirror job can take it for the
> target BB).
> 
> Signed-off-by: Max Reitz 
> ---
>  block/mirror.c | 32 +---
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 

Reviewed-by: Eric Blake 

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

[PATCH v4 20/24] python: add pytest and tests

[John Snow]

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

Create tests/, and add test_lint.py which calls out to mypy, flake8,
pylint and isort to enforce the standards in this directory.

Add pytest to the setup.cfg development dependencies; add a pytest
configuration section as well; run it in verbose mode.

Finally, add pytest to the Pipfile environment and lock the new
dependencies.

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

Signed-off-by: John Snow 
---
 python/Pipfile.lock   | 57 ++-
 python/setup.cfg  |  5 
 python/tests/test_lint.py | 28 +++
 3 files changed, 89 insertions(+), 1 deletion(-)
 create mode 100644 python/tests/test_lint.py

diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 85b3124a491..0fa1a14a9b2 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -30,6 +30,14 @@
 "markers": "python_version >= '3.5'",
 "version": "==2.4.2"
 },
+"attrs": {
+"hashes": [
+
"sha256:31b2eced602aa8423c2aea9c76a724617ed67cf9513173fd3a4f03e3a929c7e6",
+
"sha256:832aa3cde19744e49938b91fea06d69ecb9e649c93ba974535d08ad92164f700"
+],
+"markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3'",
+"version": "==20.3.0"
+},
 "flake8": {
 "hashes": [
 
"sha256:749dbbd6bfd0cf1318af27bf97a14e28e5ff548ef8e5b1566ccfb25a11e7c839",
@@ -43,9 +51,16 @@
 
"sha256:ace61d5fc652dc280e7b6b4ff732a9c2d40db2c0f92bc6cb74e07b73d53a1771",
 
"sha256:fa5daa4477a7414ae34e95942e4dd07f62adf589143c875c133c1e53c4eff38d"
 ],
-"markers": "python_version < '3.8'",
+"markers": "python_version < '3.8' and python_version < '3.8'",
 "version": "==3.4.0"
 },
+"iniconfig": {
+"hashes": [
+
"sha256:011e24c64b7f47f6ebd835bb12a743f2fbe9a26d4cecaa7f53bc4f35ee9da8b3",
+
"sha256:bc3af051d7d14b2ee5ef9969666def0cd1a000e121eaea580d4a313df4b37f32"
+],
+"version": "==1.1.1"
+},
 "isort": {
 "hashes": [
 
"sha256:c729845434366216d320e936b8ad6f9d681aab72dc7cbc2d51bedc3582f3ad1e",
@@ -123,6 +138,30 @@
 ],
 "version": "==0.4.3"
 },
+"packaging": {
+"hashes": [
+
"sha256:5b327ac1320dc863dca72f4514ecc086f31186744b84a230374cc1fd776feae5",
+
"sha256:67714da7f7bc052e064859c05c595155bd1ee9f69f76557e21f051443c20947a"
+],
+"markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3'",
+"version": "==20.9"
+},
+"pluggy": {
+"hashes": [
+
"sha256:15b2acde666561e1298d71b523007ed7364de07029219b604cf808bfa1c765b0",
+
"sha256:966c145cd83c96502c3c3868f50408687b38434af77734af1e9ca461a4081d2d"
+],
+"markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3'",
+"version": "==0.13.1"
+},
+"py": {
+"hashes": [
+
"sha256:21b81bda15b66ef5e1a777a21c4dcd9c20ad3efd0b3f817e7a809035269e1bd3",
+
"sha256:3b80836aa6d1feeaa108e046da6423ab8f6ceda6468545ae8d02d9d58d18818a"
+],
+"markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3'",
+"version": "==1.10.0"
+},
 "pycodestyle": {
 "hashes": [
 
"sha256:2295e7b2f6b5bd100585ebcb1f616591b652db8a741695b3d8f5d28bdc934367",
@@ -147,6 +186,22 @@
 "markers": "python_version >= '3.5'",
 "version": "==2.6.0"
 },
+"pyparsing": {
+"hashes": [
+
"sha256:c203ec8783bf771a155b207279b9bccb8dea02d8f0c9e5f8ead507bc3246ecc1",
+
"sha256:ef9d7589ef3c200abe66653d3f1ab1033c3c419ae9b9bdb1240a85b024efc88b"
+],
+"markers": "python_version >= '2.6' and python_version not in 
'3.0, 3.1, 3.2'",
+"version": "==2.4.7"
+},
+"pytest": {
+"hashes": [
+
"sha256:9d1edf9e7d0b84d72ea3dbcdfd22b35fb543a5e8f2a60092dd578936bf63d7f9",
+
"sha256:b574b57423e818210672e07ca1fa90aaf194a4f63f3ab909a2c67ebb22913839"
+],
+"markers": "python_version >= '3.6'",
+"version": "==6.2.2"
+},
 "qemu": {
 "editable": true,
 "path": "."
diff --git a/python/setup.cfg b/python/setup.cfg
index 

[PATCH v4 22/24] python: add Makefile for some common tasks

[John Snow]

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

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

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

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

make distclean: delete the above, and the .venv, too.

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

diff --git a/python/Makefile b/python/Makefile
new file mode 100644
index 000..51915eaec3b
--- /dev/null
+++ b/python/Makefile
@@ -0,0 +1,35 @@
+.PHONY: help venv venv-check check clean distclean
+
+help:
+   @echo "python packaging help:"
+   @echo ""
+   @echo "make venv:   Create pipenv's virtual environment."
+   @echo "NOTE: Requires Python 3.6 and pipenv."
+   @echo "  Will download packages from PyPI."
+   @echo "HINT: On Fedora: 'sudo dnf install python36 pipenv'"
+   @echo ""
+   @echo "make venv-check: run linters using pipenv's virtual environment."
+   @echo ""
+   @echo "make check:  run linters using the current environment."
+   @echo "Hint: Install deps with: 'pip install \".[devel]\"'"
+   @echo ""
+   @echo "make clean:  remove build output."
+   @echo ""
+   @echo "make distclean:  remove venv files and everything from 'clean'."
+
+
+venv: .venv
+.venv: Pipfile.lock
+   @PIPENV_VENV_IN_PROJECT=1 pipenv sync --dev --keep-outdated
+
+venv-check: venv
+   @pipenv run make check
+
+check:
+   @pytest
+
+clean:
+   rm -rf build/ dist/
+
+distclean: clean
+   rm -rf qemu.egg.info/ .venv
-- 
2.29.2

[PATCH v4 16/24] python: move .isort.cfg into setup.cfg

[John Snow]

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

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

[PATCH v4 13/24] python: Add flake8 to pipenv

[John Snow]

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

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

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

diff --git a/python/Pipfile b/python/Pipfile
index 1e58986c895..d1f7045f680 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -4,6 +4,7 @@ url = "https://pypi.org/simple;
 verify_ssl = true
 
 [dev-packages]
+flake8 = ">=3.6.0"
 pylint = ">=2.6.0"
 
 [packages]
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 4506335b7d9..869b0bdf67f 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
 "_meta": {
 "hash": {
-"sha256": 
"b7ac1f2ad73bc166244c0378298afba64951a16bb749b81a9668dc41f40f941c"
+"sha256": 
"9f6d4857a6c72ad40fc1ec1e58cdb76f187a2986ac4156f0027e5eb798ec69a9"
 },
 "pipfile-spec": 6,
 "requires": {
@@ -25,6 +25,22 @@
 "markers": "python_version >= '3.5'",
 "version": "==2.4.2"
 },
+"flake8": {
+"hashes": [
+
"sha256:749dbbd6bfd0cf1318af27bf97a14e28e5ff548ef8e5b1566ccfb25a11e7c839",
+
"sha256:aadae8761ec651813c24be05c6f7b4680857ef6afaae4651a4eccaef97ce6c3b"
+],
+"index": "pypi",
+"version": "==3.8.4"
+},
+"importlib-metadata": {
+"hashes": [
+
"sha256:ace61d5fc652dc280e7b6b4ff732a9c2d40db2c0f92bc6cb74e07b73d53a1771",
+
"sha256:fa5daa4477a7414ae34e95942e4dd07f62adf589143c875c133c1e53c4eff38d"
+],
+"markers": "python_version < '3.8'",
+"version": "==3.4.0"
+},
 "isort": {
 "hashes": [
 
"sha256:c729845434366216d320e936b8ad6f9d681aab72dc7cbc2d51bedc3582f3ad1e",
@@ -67,6 +83,22 @@
 ],
 "version": "==0.6.1"
 },
+"pycodestyle": {
+"hashes": [
+
"sha256:2295e7b2f6b5bd100585ebcb1f616591b652db8a741695b3d8f5d28bdc934367",
+
"sha256:c58a7d2815e0e8d7972bf1803331fb0152f867bd89adf8a01dfd55085434192e"
+],
+"markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3'",
+"version": "==2.6.0"
+},
+"pyflakes": {
+"hashes": [
+
"sha256:0d94e0e05a19e57a99444b6ddcf9a6eb2e5c68d3ca1e98e90707af8152c90a92",
+
"sha256:35b2d75ee967ea93b55750aa9edbbf72813e06a66ba54438df2cfac9e3c27fc8"
+],
+"markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3'",
+"version": "==2.2.0"
+},
 "pylint": {
 "hashes": [
 
"sha256:bb4a908c9dadbc3aac18860550e870f58e1a02c9f2c204fdf5693d73be061210",
@@ -127,11 +159,28 @@
 "markers": "implementation_name == 'cpython' and python_version < 
'3.8'",
 "version": "==1.4.2"
 },
+"typing-extensions": {
+"hashes": [
+
"sha256:7cb407020f00f7bfc3cb3e7881628838e69d8f3fcab2f64742a5e76b2f841918",
+
"sha256:99d4073b617d30288f569d3f13d2bd7548c3a7e4c8de87db09a9d29bb3a4a60c",
+
"sha256:dafc7639cde7f1b6e1acc0f457842a83e722ccca8eef5270af2d74792619a89f"
+],
+"markers": "python_version < '3.8'",
+"version": "==3.7.4.3"
+},
 "wrapt": {
 "hashes": [
 
"sha256:b62ffa81fb85f4332a4f609cab4ac40709470da05643a082ec1eb88e6d9b97d7"
 ],
 "version": "==1.12.1"
+},
+"zipp": {
+"hashes": [
+
"sha256:102c24ef8f171fd729d46599845e95c7ab894a4cf45f5de11a44cc7444fb1108",
+
"sha256:ed5eee1974372595f9e416cc7bbeeb12335201d8081ca8a0743c954d4446e5cb"
+],
+"markers": "python_version >= '3.6'",
+"version": "==3.4.0"
 }
 }
 }
-- 
2.29.2

[PATCH v4 07/24] python: add directory structure README.rst files

[John Snow]

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

They are not designed for inclusion in a published manual.

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

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

[PATCH v4 11/24] python: add pylint to pipenv

[John Snow]

We are specifying >= pylint 2.6.x for two reasons:

1. For setup.cfg support, added in pylint 2.5.x
2. To clarify that we are using a version that has incompatibly dropped
bad-whitespace checks.

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

diff --git a/python/Pipfile b/python/Pipfile
index 9534830b5eb..1e58986c895 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -4,6 +4,7 @@ url = "https://pypi.org/simple;
 verify_ssl = true
 
 [dev-packages]
+pylint = ">=2.6.0"
 
 [packages]
 
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
new file mode 100644
index 000..4506335b7d9
--- /dev/null
+++ b/python/Pipfile.lock
@@ -0,0 +1,137 @@
+{
+"_meta": {
+"hash": {
+"sha256": 
"b7ac1f2ad73bc166244c0378298afba64951a16bb749b81a9668dc41f40f941c"
+},
+"pipfile-spec": 6,
+"requires": {
+"python_version": "3.6"
+},
+"sources": [
+{
+"name": "pypi",
+"url": "https://pypi.org/simple;,
+"verify_ssl": true
+}
+]
+},
+"default": {},
+"develop": {
+"astroid": {
+"hashes": [
+
"sha256:2f4078c2a41bf377eea06d71c9d2ba4eb8f6b1af2135bec277d8f12bb703",
+
"sha256:bc58d83eb610252fd8de6363e39d4f1d0619c894b0ed24603b881c02e64c7386"
+],
+"markers": "python_version >= '3.5'",
+"version": "==2.4.2"
+},
+"isort": {
+"hashes": [
+
"sha256:c729845434366216d320e936b8ad6f9d681aab72dc7cbc2d51bedc3582f3ad1e",
+
"sha256:fff4f0c04e1825522ce6949973e83110a6e907750cd92d128b0d14dbffdc"
+],
+"markers": "python_version >= '3.6' and python_version < '4.0'",
+"version": "==5.7.0"
+},
+"lazy-object-proxy": {
+"hashes": [
+
"sha256:0c4b206227a8097f05c4dbdd323c50edf81f15db3b8dc064d08c62d37e1a504d",
+
"sha256:194d092e6f246b906e8f70884e620e459fc54db3259e60cf69a4d66c3fda3449",
+
"sha256:1be7e4c9f96948003609aa6c974ae59830a6baecc5376c25c92d7d697e684c08",
+
"sha256:4677f594e474c91da97f489fea5b7daa17b5517190899cf213697e48d3902f5a",
+
"sha256:48dab84ebd4831077b150572aec802f303117c8cc5c871e182447281ebf3ac50",
+
"sha256:5541cada25cd173702dbd99f8e22434105456314462326f06dba3e180f203dfd",
+
"sha256:59f79fef100b09564bc2df42ea2d8d21a64fdcda64979c0fa3db7bdaabaf6239",
+
"sha256:8d859b89baf8ef7f8bc6b00aa20316483d67f0b1cbf422f5b4dc56701c8f2ffb",
+
"sha256:9254f4358b9b541e3441b007a0ea0764b9d056afdeafc1a5569eee1cc6c1b9ea",
+
"sha256:9651375199045a358eb6741df3e02a651e0330be090b3bc79f6d0de31a80ec3e",
+
"sha256:97bb5884f6f1cdce0099f86b907aa41c970c3c672ac8b9c8352789e103cf3156",
+
"sha256:9b15f3f4c0f35727d3a0fba4b770b3c4ebbb1fa907dbcc046a1d2799f3edd142",
+
"sha256:a2238e9d1bb71a56cd710611a1614d1194dc10a175c1e08d75e1a7bcc250d442",
+
"sha256:a6ae12d08c0bf9909ce12385803a543bfe99b95fe01e752536a60af2b7797c62",
+
"sha256:ca0a928a3ddbc5725be2dd1cf895ec0a254798915fb3a36af0964a0a4149e3db",
+
"sha256:cb2c7c57005a6804ab66f106ceb8482da55f5314b7fcb06551db1edae4ad1531",
+
"sha256:d74bb8693bf9cf75ac3b47a54d716bbb1a92648d5f781fc799347cfc95952383",
+
"sha256:d945239a5639b3ff35b70a88c5f2f491913eb94871780ebfabb2568bd58afc5a",
+
"sha256:eba7011090323c1dadf18b3b689845fd96a61ba0a1dfbd7f24b921398affc357",
+
"sha256:efa1909120ce98bbb3777e8b6f92237f5d5c8ea6758efea36a473e1d38f7d3e4",
+
"sha256:f3900e8a5de27447acbf900b4750b0ddfd7ec1ea7fbaf11dfa911141bc522af0"
+],
+"markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3'",
+"version": "==1.4.3"
+},
+"mccabe": {
+"hashes": [
+
"sha256:ab8a6258860da4b6677da4bd2fe5dc2c659cff31b3ee4f7f5d64e79735b80d42",
+
"sha256:dd8d182285a0fe56bace7f45b5e7d1a6ebcbf524e8f3bd87eb0f125271b8831f"
+],
+"version": "==0.6.1"
+},
+"pylint": {
+"hashes": [
+
"sha256:bb4a908c9dadbc3aac18860550e870f58e1a02c9f2c204fdf5693d73be061210",
+
"sha256:bfe68f020f8a0fece830a22dd4d5dddb4ecc6137db04face4c3420a46a52239f"
+],
+"index": "pypi",
+"version": "==2.6.0"
+},
+"six": {
+"hashes": [
+
"sha256:30639c035cdb23534cd4aa2dd52c3bf48f06e5f4a941509c8bafd8ce11080259",
+

[PATCH v4 18/24] python/qemu: add qemu package itself to pipenv

[John Snow]

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

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

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

diff --git a/python/Pipfile b/python/Pipfile
index 75b96f29d87..214fb175e7a 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -10,6 +10,7 @@ mypy = ">=0.770"
 pylint = ">=2.6.0"
 
 [packages]
+qemu = {editable = true,path = "."}
 
 [requires]
 python_version = "3.6"
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 201e735c27a..4b4402f49e5 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
 "_meta": {
 "hash": {
-"sha256": 
"b89c7a1b8a414f2a4cd708964123fb427d55419ee0b39e088bf2e7d4fbc11979"
+"sha256": 
"e38d142c3fadc2f2ed849e86f7ebd14e25974dc12228751490aef5a9ee074f2f"
 },
 "pipfile-spec": 6,
 "requires": {
@@ -15,7 +15,12 @@
 }
 ]
 },
-"default": {},
+"default": {
+"qemu": {
+"editable": true,
+"path": "."
+}
+},
 "develop": {
 "astroid": {
 "hashes": [
-- 
2.29.2

[PATCH v4 21/24] python: add excluded dirs to flake8 config

[John Snow]

Following patches make obvious that we ought to ignore certain
directories to avoid wildly erroneous flake8 output.

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

diff --git a/python/setup.cfg b/python/setup.cfg
index 2c5943277d7..2c12d9ab89b 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -31,6 +31,8 @@ devel =
 
 [flake8]
 extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
+exclude = __pycache__,
+  .venv,
 
 [mypy]
 strict = True
-- 
2.29.2

[PATCH v4 19/24] python: add devel package requirements to setuptools

[John Snow]

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

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

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

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

diff --git a/python/Pipfile b/python/Pipfile
index 214fb175e7a..e7acb8cefa4 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -4,10 +4,7 @@ url = "https://pypi.org/simple;
 verify_ssl = true
 
 [dev-packages]
-flake8 = ">=3.6.0"
-isort = ">=5.0.5"
-mypy = ">=0.770"
-pylint = ">=2.6.0"
+qemu = {editable = true, extras = ["devel"], path = "."}
 
 [packages]
 qemu = {editable = true,path = "."}
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 4b4402f49e5..85b3124a491 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
 "_meta": {
 "hash": {
-"sha256": 
"e38d142c3fadc2f2ed849e86f7ebd14e25974dc12228751490aef5a9ee074f2f"
+"sha256": 
"eff562a688ebc6f3ffe67494dbb804b883e2159ad81c4d55d96da9f7aec13e91"
 },
 "pipfile-spec": 6,
 "requires": {
@@ -35,7 +35,7 @@
 
"sha256:749dbbd6bfd0cf1318af27bf97a14e28e5ff548ef8e5b1566ccfb25a11e7c839",
 
"sha256:aadae8761ec651813c24be05c6f7b4680857ef6afaae4651a4eccaef97ce6c3b"
 ],
-"index": "pypi",
+"markers": "python_version >= '2.7' and python_version not in 
'3.0, 3.1, 3.2, 3.3'",
 "version": "==3.8.4"
 },
 "importlib-metadata": {
@@ -51,7 +51,7 @@
 
"sha256:c729845434366216d320e936b8ad6f9d681aab72dc7cbc2d51bedc3582f3ad1e",
 
"sha256:fff4f0c04e1825522ce6949973e83110a6e907750cd92d128b0d14dbffdc"
 ],
-"index": "pypi",
+"markers": "python_version >= '3.6' and python_version < '4.0'",
 "version": "==5.7.0"
 },
 "lazy-object-proxy": {
@@ -113,7 +113,7 @@
 
"sha256:e497a544391f733eca922fdcb326d19e894789cd4ff61d48b4b195776476c5cf",
 
"sha256:f5fdf935a46aa20aa937f2478480ebf4be9186e98e49cc3843af9a5795a49a25"
 ],
-"index": "pypi",
+"markers": "python_version >= '3.5'",
 "version": "==0.800"
 },
 "mypy-extensions": {
@@ -144,9 +144,13 @@
 
"sha256:bb4a908c9dadbc3aac18860550e870f58e1a02c9f2c204fdf5693d73be061210",
 
"sha256:bfe68f020f8a0fece830a22dd4d5dddb4ecc6137db04face4c3420a46a52239f"
 ],
-"index": "pypi",
+"markers": "python_version >= '3.5'",
 "version": "==2.6.0"
 },
+"qemu": {
+"editable": true,
+"path": "."
+},
 "six": {
 "hashes": [
 
"sha256:30639c035cdb23534cd4aa2dd52c3bf48f06e5f4a941509c8bafd8ce11080259",
diff --git a/python/setup.cfg b/python/setup.cfg
index bc90d52b9ca..11c361501e8 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -19,6 +19,15 @@ classifiers =
 python_requires = >= 3.6
 packages = find_namespace:
 
+[options.extras_require]
+# Run `pipenv lock` when changing these requirements.
+devel =
+flake8 >= 3.6.0
+isort >= 5.0.5
+mypy >= 0.770
+pylint >= 2.6.0
+
+
 [flake8]
 extend-ignore = E722  # Prefer pylint's bare-except checks to flake8's
 
-- 
2.29.2

[PATCH v4 14/24] python: move mypy.ini into setup.cfg

[John Snow]

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

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

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

[PATCH v4 23/24] python: add .gitignore

[John Snow]

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

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

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

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

diff --git a/python/.gitignore b/python/.gitignore
new file mode 100644
index 000..78c522768bc
--- /dev/null
+++ b/python/.gitignore
@@ -0,0 +1,9 @@
+*.pyc
+.idea/
+.mypy_cache/
+.pytest_cache/
+.venv/
+__pycache__/
+build/
+dist/
+qemu.egg-info/
-- 
2.29.2

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

[John Snow]

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

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

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

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 28a83afb914..d1ae3972956 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -633,6 +633,16 @@ check-patch:
 GIT_DEPTH: 1000
   allow_failure: true
 
+
+check-python:
+  stage: build
+  image: $CI_REGISTRY_IMAGE/qemu/fedora:latest
+  script:
+- cd python
+- make venv-check
+  variables:
+GIT_DEPTH: 1000
+
 check-dco:
   stage: build
   image: $CI_REGISTRY_IMAGE/qemu/centos8:latest
diff --git a/tests/docker/dockerfiles/fedora.docker 
b/tests/docker/dockerfiles/fedora.docker
index 0d7602abbeb..1262b8c5e03 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -84,6 +84,7 @@ ENV PACKAGES \
 numactl-devel \
 perl \
 perl-Test-Harness \
+pipenv \
 pixman-devel \
 python3 \
 python3-PyYAML \
@@ -93,6 +94,7 @@ ENV PACKAGES \
 python3-pip \
 python3-sphinx \
 python3-virtualenv \
+python3.6 \
 rdma-core-devel \
 SDL2-devel \
 snappy-devel \
-- 
2.29.2

[PATCH v4 12/24] python: move flake8 config to setup.cfg

[John Snow]

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

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

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

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

[PATCH v4 17/24] python/qemu: add isort to pipenv

[John Snow]

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

Require 5.0.5 or greater.

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

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

diff --git a/python/Pipfile b/python/Pipfile
index 51c537b0d10..75b96f29d87 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -5,6 +5,7 @@ verify_ssl = true
 
 [dev-packages]
 flake8 = ">=3.6.0"
+isort = ">=5.0.5"
 mypy = ">=0.770"
 pylint = ">=2.6.0"
 
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 5f8aab117c3..201e735c27a 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
 "_meta": {
 "hash": {
-"sha256": 
"65f010a8f2e55e9870d2b7e0d8af129516097d23abf2504f396552748b067ade"
+"sha256": 
"b89c7a1b8a414f2a4cd708964123fb427d55419ee0b39e088bf2e7d4fbc11979"
 },
 "pipfile-spec": 6,
 "requires": {
@@ -46,7 +46,7 @@
 
"sha256:c729845434366216d320e936b8ad6f9d681aab72dc7cbc2d51bedc3582f3ad1e",
 
"sha256:fff4f0c04e1825522ce6949973e83110a6e907750cd92d128b0d14dbffdc"
 ],
-"markers": "python_version >= '3.6' and python_version < '4.0'",
+"index": "pypi",
 "version": "==5.7.0"
 },
 "lazy-object-proxy": {
-- 
2.29.2

[PATCH v4 15/24] python: add mypy to pipenv

[John Snow]

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

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

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

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

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

diff --git a/python/Pipfile b/python/Pipfile
index d1f7045f680..51c537b0d10 100644
--- a/python/Pipfile
+++ b/python/Pipfile
@@ -5,6 +5,7 @@ verify_ssl = true
 
 [dev-packages]
 flake8 = ">=3.6.0"
+mypy = ">=0.770"
 pylint = ">=2.6.0"
 
 [packages]
diff --git a/python/Pipfile.lock b/python/Pipfile.lock
index 869b0bdf67f..5f8aab117c3 100644
--- a/python/Pipfile.lock
+++ b/python/Pipfile.lock
@@ -1,7 +1,7 @@
 {
 "_meta": {
 "hash": {
-"sha256": 
"9f6d4857a6c72ad40fc1ec1e58cdb76f187a2986ac4156f0027e5eb798ec69a9"
+"sha256": 
"65f010a8f2e55e9870d2b7e0d8af129516097d23abf2504f396552748b067ade"
 },
 "pipfile-spec": 6,
 "requires": {
@@ -83,6 +83,41 @@
 ],
 "version": "==0.6.1"
 },
+"mypy": {
+"hashes": [
+
"sha256:0d2fc8beb99cd88f2d7e20d69131353053fbecea17904ee6f0348759302c52fa",
+
"sha256:2b216eacca0ec0ee124af9429bfd858d5619a0725ee5f88057e6e076f9eb1a7b",
+
"sha256:319ee5c248a7c3f94477f92a729b7ab06bf8a6d04447ef3aa8c9ba2aa47c6dcf",
+
"sha256:3e0c159a7853e3521e3f582adb1f3eac66d0b0639d434278e2867af3a8c62653",
+
"sha256:5615785d3e2f4f03ab7697983d82c4b98af5c321614f51b8f1034eb9ebe48363",
+
"sha256:5ff616787122774f510caeb7b980542a7ccbe3f00837a304ea85cd56e488",
+
"sha256:6f8425fecd2ba6007e526209bb985ce7f49ed0d2ac1cc1a44f243380a06a84fb",
+
"sha256:74f5aa50d0866bc6fb8e213441c41e466c86678c800700b87b012ed11c0a13e0",
+
"sha256:90b6f46dc2181d74f80617deca611925d7e63007cf416397358aa42efb593e07",
+
"sha256:947126195bfe4709c360e89b40114c6746ae248f04d379dca6f6ab677aa07641",
+
"sha256:a301da58d566aca05f8f449403c710c50a9860782148332322decf73a603280b",
+
"sha256:aa9d4901f3ee1a986a3a79fe079ffbf7f999478c281376f48faa31daaa814e86",
+
"sha256:b9150db14a48a8fa114189bfe49baccdff89da8c6639c2717750c7ae62316738",
+
"sha256:b95068a3ce3b50332c40e31a955653be245666a4bc7819d3c8898aa9fb9ea496",
+
"sha256:ca7ad5aed210841f1e77f5f2f7d725b62c78fa77519312042c719ed2ab937876",
+
"sha256:d16c54b0dffb861dc6318a8730952265876d90c5101085a4bc56913e8521ba19",
+
"sha256:e0202e37756ed09daf4b0ba64ad2c245d357659e014c3f51d8cd0681ba66940a",
+
"sha256:e1c84c65ff6d69fb42958ece5b1255394714e0aac4df5ffe151bc4fe19c7600a",
+
"sha256:e32b7b282c4ed4e378bba8b8dfa08e1cfa6f6574067ef22f86bee5b1039de0c9",
+
"sha256:e3b8432f8df19e3c11235c4563a7250666dc9aa7cdda58d21b4177b20256ca9f",
+
"sha256:e497a544391f733eca922fdcb326d19e894789cd4ff61d48b4b195776476c5cf",
+
"sha256:f5fdf935a46aa20aa937f2478480ebf4be9186e98e49cc3843af9a5795a49a25"
+],
+"index": "pypi",
+"version": "==0.800"
+},
+"mypy-extensions": {
+"hashes": [
+
"sha256:090fedd75945a69ae91ce1303b5824f428daf5a028d2f6ab8a299250a846f15d",
+
"sha256:2d82818f5bb3e369420cb3c4060a7970edba416647068eb4c5343488a6c604a8"
+],
+"version": "==0.4.3"
+},
 "pycodestyle": {
 "hashes": [
 
"sha256:2295e7b2f6b5bd100585ebcb1f616591b652db8a741695b3d8f5d28bdc934367",
diff --git a/python/setup.cfg b/python/setup.cfg
index 0bd55c5e373..f919e95f95b 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -26,6 +26,7 @@ extend-ignore = E722  # Prefer pylint's bare-except checks to 
flake8's
 strict = True
 python_version = 3.6
 warn_unused_configs = True
+namespace_packages = True
 
 [pylint.messages control]
 # Disable the message, report, category or checker with the given id(s). You
-- 
2.29.2

[PATCH v4 09/24] python: add pylint import exceptions

[John Snow]

Pylint 2.5.x and 2.6.x have regressions that make import checking
inconsistent, see:

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

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

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

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

[PATCH v4 10/24] python: move pylintrc into setup.cfg

[John Snow]

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

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

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

diff --git a/python/qemu/machine/pylintrc b/python/qemu/machine/pylintrc
deleted file mode 100644
index 3f69205000d..000
--- a/python/qemu/machine/pylintrc
+++ /dev/null
@@ -1,58 +0,0 @@
-[MASTER]
-
-[MESSAGES CONTROL]
-
-# Disable the message, report, category or checker with the given id(s). You
-# can either give multiple identifiers separated by comma (,) or put this
-# option multiple times (only on the command line, not in the configuration
-# file where it should appear only once). You can also use "--disable=all" to
-# disable everything first and then reenable specific checks. For example, if
-# you want to run only the similarities checker, you can use "--disable=all
-# --enable=similarities". If you want to run only the classes checker, but have
-# no Warning level messages displayed, use "--disable=all --enable=classes
-# --disable=W".
-disable=too-many-arguments,
-too-many-instance-attributes,
-too-many-public-methods,
-
-[REPORTS]
-
-[REFACTORING]
-
-[MISCELLANEOUS]
-
-[LOGGING]
-
-[BASIC]
-
-# Good variable names which should always be accepted, separated by a comma.
-good-names=i,
-   j,
-   k,
-   ex,
-   Run,
-   _,
-   fd,
-   c,
-[VARIABLES]
-
-[STRING]
-
-[SPELLING]
-
-[FORMAT]
-
-[SIMILARITIES]
-
-# Ignore imports when computing similarities.
-ignore-imports=yes
-
-[TYPECHECK]
-
-[CLASSES]
-
-[IMPORTS]
-
-[DESIGN]
-
-[EXCEPTIONS]
diff --git a/python/setup.cfg b/python/setup.cfg
index e7f8ab23815..20b24372a4a 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -18,3 +18,32 @@ classifiers =
 [options]
 python_requires = >= 3.6
 packages = find_namespace:
+
+[pylint.messages control]
+# Disable the message, report, category or checker with the given id(s). You
+# can either give multiple identifiers separated by comma (,) or put this
+# option multiple times (only on the command line, not in the configuration
+# file where it should appear only once). You can also use "--disable=all" to
+# disable everything first and then reenable specific checks. For example, if
+# you want to run only the similarities checker, you can use "--disable=all
+# --enable=similarities". If you want to run only the classes checker, but have
+# no Warning level messages displayed, use "--disable=all --enable=classes
+# --disable=W".
+disable=too-many-arguments,
+too-many-instance-attributes,
+too-many-public-methods,
+
+[pylint.basic]
+# Good variable names which should always be accepted, separated by a comma.
+good-names=i,
+   j,
+   k,
+   ex,
+   Run,
+   _,
+   fd,
+   c,
+
+[pylint.similarities]
+# Ignore imports when computing similarities.
+ignore-imports=yes
-- 
2.29.2

[PATCH v4 06/24] python: add VERSION file

[John Snow]

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

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

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


I am proposing:

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

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

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

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

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

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


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

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

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

[PATCH v4 08/24] python: Add pipenv support

[John Snow]

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

pipenv uses two files to do this:

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

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

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

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

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

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

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

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

[PATCH v4 05/24] python: add qemu package installer

[John Snow]

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

I am not yet using a pyproject.toml style package manifest, because
"editable" installs are not defined by PEP-517 and pip did not have
support for this for some time; I consider the feature necessary for
development.

Use a light-weight setup.py instead.

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

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

[PATCH v4 04/24] python: create utils sub-package

[John Snow]

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

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

diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
index 27b0b19abd3..891a8f784b5 100644
--- a/python/qemu/machine/__init__.py
+++ b/python/qemu/machine/__init__.py
@@ -8,10 +8,6 @@
  - QEMUQtestMachine: VM class, with a qtest socket.
 
 - QEMUQtestProtocol: Connect to, send/receive qtest messages.
-
-- list_accel: List available accelerators
-- kvm_available: Probe for KVM support
-- tcg_available: Probe for TCG support
 """
 
 # Copyright (C) 2020 John Snow for Red Hat Inc.
@@ -26,15 +22,11 @@
 # the COPYING file in the top-level directory.
 #
 
-from .accel import kvm_available, list_accel, tcg_available
 from .machine import QEMUMachine
 from .qtest import QEMUQtestMachine, QEMUQtestProtocol
 
 
 __all__ = (
-'list_accel',
-'kvm_available',
-'tcg_available',
 'QEMUMachine',
 'QEMUQtestProtocol',
 'QEMUQtestMachine',
diff --git a/python/qemu/utils/__init__.py b/python/qemu/utils/__init__.py
new file mode 100644
index 000..edf807a93e5
--- /dev/null
+++ b/python/qemu/utils/__init__.py
@@ -0,0 +1,23 @@
+"""
+QEMU development and testing utilities
+
+This library provides a small handful of utilities for performing various tasks
+not directly related to the launching of a VM.
+
+The only module included at present is accel; its public functions are
+repeated here for your convenience:
+
+- list_accel: List available accelerators
+- kvm_available: Probe for KVM support
+- tcg_available: Prove for TCG support
+"""
+
+# pylint: disable=import-error
+from .accel import kvm_available, list_accel, tcg_available
+
+
+__all__ = (
+'list_accel',
+'kvm_available',
+'tcg_available',
+)
diff --git a/python/qemu/machine/accel.py b/python/qemu/utils/accel.py
similarity index 100%
rename from python/qemu/machine/accel.py
rename to python/qemu/utils/accel.py
diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
index 212365fd185..824cf03d5f4 100644
--- a/tests/acceptance/boot_linux.py
+++ b/tests/acceptance/boot_linux.py
@@ -12,7 +12,7 @@
 
 from avocado_qemu import Test, BUILD_DIR
 
-from qemu.machine import kvm_available, tcg_available
+from qemu.utils import kvm_available, tcg_available
 
 from avocado.utils import cloudinit
 from avocado.utils import network
diff --git a/tests/acceptance/virtio-gpu.py b/tests/acceptance/virtio-gpu.py
index 211f02932f2..73671476aa0 100644
--- a/tests/acceptance/virtio-gpu.py
+++ b/tests/acceptance/virtio-gpu.py
@@ -10,7 +10,7 @@
 from avocado_qemu import exec_command_and_wait_for_pattern
 from avocado_qemu import is_readable_executable_file
 
-from qemu.accel import kvm_available
+from qemu.utils import kvm_available
 
 import os
 import socket
diff --git a/tests/acceptance/virtiofs_submounts.py 
b/tests/acceptance/virtiofs_submounts.py
index 949ca87a837..6b684253a60 100644
--- a/tests/acceptance/virtiofs_submounts.py
+++ b/tests/acceptance/virtiofs_submounts.py
@@ -9,7 +9,7 @@
 from avocado_qemu import wait_for_console_pattern
 from avocado.utils import ssh
 
-from qemu.accel import kvm_available
+from qemu.utils import kvm_available
 
 from boot_linux import BootLinux
 
diff --git a/tests/vm/aarch64vm.py b/tests/vm/aarch64vm.py
index d70ab843b6b..b00cce07eb8 100644
--- a/tests/vm/aarch64vm.py
+++ b/tests/vm/aarch64vm.py
@@ -14,7 +14,7 @@
 import sys
 import subprocess
 import basevm
-from qemu.accel import kvm_available
+from qemu.utils import kvm_available
 
 # This is the config needed for current version of QEMU.
 # This works for both kvm and tcg.
diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
index 12d08cf2b1b..a3867fdf88e 100644
--- a/tests/vm/basevm.py
+++ b/tests/vm/basevm.py
@@ -19,7 +19,8 @@
 import time
 import datetime
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-from qemu.machine import kvm_available, QEMUMachine
+from qemu.machine import QEMUMachine
+from qemu.utils import kvm_available
 import subprocess
 import hashlib
 import argparse
-- 
2.29.2

[PATCH v4 02/24] iotests/297: add --namespace-packages to mypy arguments

[John Snow]

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

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

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

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

[PATCH v4 03/24] python: create qemu packages

[John Snow]

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

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

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

Signed-off-by: John Snow 


---

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

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

diff --git a/python/qemu/.isort.cfg b/python/.isort.cfg
similarity index 100%
rename from python/qemu/.isort.cfg
rename to python/.isort.cfg
diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
deleted file mode 100644
index 4ca06c34a41..000
--- a/python/qemu/__init__.py
+++ /dev/null
@@ -1,11 +0,0 @@
-# QEMU library
-#
-# Copyright (C) 2015-2016 Red Hat Inc.
-# Copyright (C) 2012 IBM Corp.
-#
-# Authors:
-#  Fam Zheng 
-#
-# This work is licensed under the terms of the GNU GPL, version 2.  See
-# the COPYING file in the top-level directory.
-#
diff --git a/python/qemu/.flake8 b/python/qemu/machine/.flake8
similarity index 100%
rename from python/qemu/.flake8
rename to python/qemu/machine/.flake8
diff --git a/python/qemu/machine/__init__.py b/python/qemu/machine/__init__.py
new file mode 100644
index 000..27b0b19abd3
--- /dev/null
+++ b/python/qemu/machine/__init__.py
@@ -0,0 +1,41 @@
+"""
+QEMU development and testing library.
+
+This library provides a few high-level classes for driving QEMU from a
+test suite, not intended for production use.
+
+- QEMUMachine: Configure and Boot a QEMU VM
+ - QEMUQtestMachine: VM class, with a qtest socket.
+
+- QEMUQtestProtocol: Connect to, send/receive qtest messages.
+
+- list_accel: List available accelerators
+- kvm_available: Probe for KVM support
+- tcg_available: Probe for TCG support
+"""
+
+# Copyright (C) 2020 John Snow for Red Hat Inc.
+# Copyright (C) 2015-2016 Red Hat Inc.
+# Copyright (C) 2012 IBM Corp.
+#
+# Authors:
+#  John Snow 
+#  Fam Zheng 
+#
+# This work is licensed under the terms of the GNU GPL, version 2.  See
+# the COPYING file in the top-level directory.
+#
+
+from .accel import kvm_available, list_accel, tcg_available
+from .machine import QEMUMachine
+from .qtest import QEMUQtestMachine, QEMUQtestProtocol
+
+
+__all__ = (
+'list_accel',
+'kvm_available',
+'tcg_available',
+'QEMUMachine',
+'QEMUQtestProtocol',
+'QEMUQtestMachine',
+)
diff --git a/python/qemu/accel.py b/python/qemu/machine/accel.py
similarity index 100%
rename from python/qemu/accel.py
rename to python/qemu/machine/accel.py
diff --git a/python/qemu/console_socket.py 
b/python/qemu/machine/console_socket.py
similarity index 100%
rename from python/qemu/console_socket.py
rename to python/qemu/machine/console_socket.py
diff --git a/python/qemu/machine.py b/python/qemu/machine/machine.py
similarity index 98%
rename from python/qemu/machine.py
rename to python/qemu/machine/machine.py
index 7a40f4604be..dd6ce289350 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine/machine.py
@@ -38,8 +38,14 @@
 Type,
 )
 
-from . import 

[PATCH v4 00/24] python: create installable package

[John Snow]

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

git: https://gitlab.com/jsnow/qemu/-/commits/python-package-mk3
CI: https://gitlab.com/jsnow/qemu/-/pipelines/254940717
(New CI job: https://gitlab.com/jsnow/qemu/-/jobs/1024230604)

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

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

Some other things this enables that might be of interest:

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

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

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

TESTING THIS SERIES:

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

To test those, CD to qemu/python first, and then:

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

You will need Python 3.6 installed on your system to do this step. For
Fedora: "dnf install python3.6" will do the trick.

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

3. Having exited the shell from above, try "make venv-check". This will
create and update the venv if needed, then run 'make check' within the
context of that shell. It should pass as long as the above did.

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

5. "make distclean" will delete the venv and any temporary files that
may have been created by packaging, installing, testing, etc.

Changelog:

- Moved qemu/machine/accel.py to qemu/utils/accel.py
- Integrated CI patches into this series
- Changed version of package to 0.6.0.0a1
- Misc different changes for import statements in e.g. iotests/VM tests
- Modified iotests invocation of mypy ever so slightly

Reviewer notes:

- The VERSION hack may be imperfect, but at 0.x and without uploading it
  to PyPI, we have *all* the time in the world to fine-tune it later.
- The CI integration may not be perfect, but it is better than *nothing*,
  so I think it's worth doing even in an imperfect state.

John Snow (24):
  python/console_socket: avoid one-letter variable
  iotests/297: add --namespace-packages to mypy arguments
  python: create qemu packages
  python: create utils sub-package
  python: add qemu package installer
  python: add VERSION file
  

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

[John Snow]

Fixes pylint warnings.

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

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

Re: [PATCH] hw/block/nvme: add broadcast nsid support flush command

[Klaus Jensen]

On Jan 25 21:42, Klaus Jensen wrote:
> From: Gollu Appalanaidu 
> 
> Add support for using the broadcast nsid to issue a flush on all
> namespaces through a single command.
> 

Applied to nvme-next.


signature.asc
Description: PGP signature

Re: [PATCH 2/2] hw/block/nvme: add write uncorrectable command

[Klaus Jensen]

On Feb 12 00:37, Keith Busch wrote:
> On Thu, Feb 11, 2021 at 09:43:05AM +0100, Klaus Jensen wrote:
> > On Feb 11 12:37, Keith Busch wrote:
> > 
> > > Is there a use case with a real qemu guest wanting this?
> > 
> > Like for the extended metadata case (which also does not have a lot of
> > "public" exposure, but definitely have enterprise use), our main
> > motivation here was to ease testing for compliance suites and frameworks
> > like SPDK. 
> 
> I'm okay with the metadata patches.
> 
> > I'm honestly have no clue what so ever what a real world use
> > of Write Uncorrectable would be. It's been in the spec since 1.0, so
> > there must have been some reason, Is it just to align with SCSI WRITE
> > LONG? I'm not SCSI expert at all, but from what I can read it looks like
> > that was also intended as a feature for testing read error conditions.
> 
> I don't think it's for testing purposes.
> 
> If you need to send a burst of non-atomic writes (ex: writing a RAID
> stripe), a power failure can result in an inconsistent state where you
> don't know at a block level which ones have old data or new data. If you
> Write Uncorrectable first, you can never read old data, and thus have no
> "write hole".
> 
> Journalling solves this better, and I'm not aware of any real
> implementation relying on uncorrectable.

Right, thanks! I'm aware of the RAID write hole issue, but I did not
consider Write Uncorrectable as a possible means to solve it.


signature.asc
Description: PGP signature

[PATCH 1/2] block/mirror: Fix mirror_top's permissions

[Max Reitz]

mirror_top currently shares all permissions, and takes only the WRITE
permission (if some parent has taken that permission, too).

That is wrong, though; mirror_top is a filter, so it should take
permissions like any other filter does.  For example, if the parent
needs CONSISTENT_READ, we need to take that, too, and if it cannot share
the WRITE permission, we cannot share it either.

The exception is when mirror_top is used for active commit, where we
cannot take CONSISTENT_READ (because it is deliberately unshared above
the base node) and where we must share WRITE (so that it is shared for
all images in the backing chain, so the mirror job can take it for the
target BB).

Signed-off-by: Max Reitz 
---
 block/mirror.c | 32 +---
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 8e1ad6eceb..1edfc3cc14 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -89,6 +89,7 @@ typedef struct MirrorBlockJob {
 typedef struct MirrorBDSOpaque {
 MirrorBlockJob *job;
 bool stop;
+bool is_commit;
 } MirrorBDSOpaque;
 
 struct MirrorOp {
@@ -1513,13 +1514,27 @@ static void bdrv_mirror_top_child_perm(BlockDriverState 
*bs, BdrvChild *c,
 return;
 }
 
-/* Must be able to forward guest writes to the real image */
-*nperm = 0;
-if (perm & BLK_PERM_WRITE) {
-*nperm |= BLK_PERM_WRITE;
-}
+bdrv_default_perms(bs, c, role, reopen_queue,
+   perm, shared, nperm, nshared);
 
-*nshared = BLK_PERM_ALL;
+if (s->is_commit) {
+/*
+ * For commit jobs, we cannot take CONSISTENT_READ, because
+ * that permission is unshared for everything above the base
+ * node (except for filters on the base node).
+ * We also have to force-share the WRITE permission, or
+ * otherwise we would block ourselves at the base node (if
+ * writes are blocked for a node, they are also blocked for
+ * its backing file).
+ * (We could also share RESIZE, because it may be needed for
+ * the target if its size is less than the top node's; but
+ * bdrv_default_perms_for_cow() automatically shares RESIZE
+ * for backing nodes if WRITE is shared, so there is no need
+ * to do it here.)
+ */
+*nperm &= ~BLK_PERM_CONSISTENT_READ;
+*nshared |= BLK_PERM_WRITE;
+}
 }
 
 /* Dummy node that provides consistent read to its users without requiring it
@@ -1583,6 +1598,8 @@ static BlockJob *mirror_start_job(
 return NULL;
 }
 
+target_is_backing = bdrv_chain_contains(bs, target);
+
 /* In the case of active commit, add dummy driver to provide consistent
  * reads on the top, while disabling it in the intermediate nodes, and make
  * the backing chain writable. */
@@ -1605,6 +1622,8 @@ static BlockJob *mirror_start_job(
 bs_opaque = g_new0(MirrorBDSOpaque, 1);
 mirror_top_bs->opaque = bs_opaque;
 
+bs_opaque->is_commit = target_is_backing;
+
 /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
  * it alive until block_job_create() succeeds even if bs has no parent. */
 bdrv_ref(mirror_top_bs);
@@ -1646,7 +1665,6 @@ static BlockJob *mirror_start_job(
 target_perms = BLK_PERM_WRITE;
 target_shared_perms = BLK_PERM_WRITE_UNCHANGED;
 
-target_is_backing = bdrv_chain_contains(bs, target);
 if (target_is_backing) {
 int64_t bs_size, target_size;
 bs_size = bdrv_getlength(bs);
-- 
2.29.2

Re: [PULL v2 0/3] emulated nvme fixes

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20210211152139.1004257-1-...@irrelevant.dk/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210211152139.1004257-1-...@irrelevant.dk
Subject: [PULL v2 0/3] emulated nvme fixes

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] patchew/20210211152139.1004257-1-...@irrelevant.dk -> 
patchew/20210211152139.1004257-1-...@irrelevant.dk
Switched to a new branch 'test'

=== OUTPUT BEGIN ===
checkpatch.pl: no revisions returned for revlist 'base..'
=== OUTPUT END ===

Test command exited with code: 255


The full log is available at
http://patchew.org/logs/20210211152139.1004257-1-...@irrelevant.dk/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[PATCH 0/2] file-posix: Cache next hole

[Max Reitz]

Hi,

On Launchpad, Alexandre has reported a performance problem with
SEEK_HOLE on filesystems with much fragmentation:

  https://bugs.launchpad.net/qemu/+bug/1912224

This isn’t the first report we’ve received on SEEK_HOLE being slow, and
we’ve already taken measures to address this issue.  For example,
block-status has a @want_zero parameter so that we won’t do such a seek
operation unless the caller needs that information.  qcow2 tries to
avoid falling through to the protocol level, because most of the time,
it itself knows well which clusters are zero and which aren’t.

But both of these measures don’t work e.g. when mirroring a raw file,
when we want to look up zero areas (because it may speed up the mirror
and save space), and where we don’t have a format layer that has
randomly accessible zero information.

Alexandre proposed caching the offset of the next hole, which we can do
in file-posix (unless the WRITE permission is shared), and which works
best for images that are (nearly) fully allocated.  Such images are also
the ones that profit the least off of the time lost on SEEK_HOLE
operations, i.e. where it’s most important to keep that time low.

I understand that caching filesystem information in file-posix is a bit
weird, but I’d like to hear what you think.  Alexandre has reported that
it alleviated his problem quite significantly (see comment 8 in the LP
report).


(Speaking of “unless the WRITE permission is shared”: mirror_top is a
bit broken in that it takes no permissions (but WRITE if necessary) and
shares everything.  That seems wrong.  Patch 1 addresses that, so that
patch 2 can actually do something when mirroring an image.)


Max Reitz (2):
  block/mirror: Fix mirror_top's permissions
  file-posix: Cache next hole

 block/file-posix.c | 81 +-
 block/mirror.c | 32 ++
 2 files changed, 105 insertions(+), 8 deletions(-)

-- 
2.29.2

[PATCH 2/2] file-posix: Cache next hole

[Max Reitz]

We have repeatedly received reports that SEEK_HOLE and SEEK_DATA are
slow on certain filesystems and/or under certain circumstances.  That is
why we generally try to avoid it (which is why bdrv_co_block_status()
has the @want_zero parameter, and which is why qcow2 has a metadata
preallocation detection, so we do not fall through to the protocol layer
to discover which blocks are zero, unless that is really necessary
(i.e., for metadata-preallocated images)).

In addition to those measures, we can also try to speed up zero
detection by letting file-posix cache some hole location information,
namely where the next hole after the most recently queried offset is.
This helps especially for images that are (nearly) fully allocated,
which is coincidentally also the case where querying for zero
information cannot gain us much.

Note that this of course only works so long as we have no concurrent
writers to the image, which is the case when the WRITE capability is not
shared.

Alternatively (or perhaps as an improvement in the future), we could let
file-posix keep track of what it knows is zero and what it knows is
non-zero with bitmaps, which would help images that actually have a
significant number of holes (where this implementation here cannot do
much).  But for such images, SEEK_HOLE/DATA are generally faster (they
do not need to seek through the whole file), and the performance lost by
querying the block status does not feel as bad because it is outweighed
by the performance that can be saved by special-cases zeroed areas, so
focussing on images that are (nearly) fully allocated is more important.

Signed-off-by: Max Reitz 
---
 block/file-posix.c | 81 +-
 1 file changed, 80 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 05079b40ca..2ca0a2e05b 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -172,6 +172,11 @@ typedef struct BDRVRawState {
 } stats;
 
 PRManager *pr_mgr;
+
+bool can_cache_next_zero_offset;
+bool next_zero_offset_valid;
+uint64_t next_zero_offset_from;
+uint64_t next_zero_offset;
 } BDRVRawState;
 
 typedef struct BDRVRawReopenState {
@@ -2049,7 +2054,25 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov,
int flags)
 {
+BDRVRawState *s = bs->opaque;
+
 assert(flags == 0);
+
+/*
+ * If offset is just above s->next_zero_offset, the hole that was
+ * reportedly there might be removed from the file (because only
+ * whole filesystem clusters can be zeroed).  But that does not
+ * matter, because block-status does not care about whether there
+ * actually is a hole, but just about whether there are zeroes
+ * there - and this write will not make those zeroes non-zero.
+ */
+if (s->next_zero_offset_valid &&
+offset <= s->next_zero_offset &&
+offset + bytes > s->next_zero_offset)
+{
+s->next_zero_offset_valid = false;
+}
+
 return raw_co_prw(bs, offset, bytes, qiov, QEMU_AIO_WRITE);
 }
 
@@ -2183,6 +2206,10 @@ static int coroutine_fn raw_co_truncate(BlockDriverState 
*bs, int64_t offset,
 struct stat st;
 int ret;
 
+if (s->next_zero_offset_valid && offset < s->next_zero_offset) {
+s->next_zero_offset_valid = false;
+}
+
 if (fstat(s->fd, )) {
 ret = -errno;
 error_setg_errno(errp, -ret, "Failed to fstat() the file");
@@ -2616,8 +2643,17 @@ static int coroutine_fn 
raw_co_delete_file(BlockDriverState *bs,
 static int find_allocation(BlockDriverState *bs, off_t start,
off_t *data, off_t *hole)
 {
-#if defined SEEK_HOLE && defined SEEK_DATA
 BDRVRawState *s = bs->opaque;
+
+if (s->next_zero_offset_valid) {
+if (start >= s->next_zero_offset_from && start < s->next_zero_offset) {
+*data = start;
+*hole = s->next_zero_offset;
+return 0;
+}
+}
+
+#if defined SEEK_HOLE && defined SEEK_DATA
 off_t offs;
 
 /*
@@ -2716,6 +2752,7 @@ static int coroutine_fn 
raw_co_block_status(BlockDriverState *bs,
 int64_t *map,
 BlockDriverState **file)
 {
+BDRVRawState *s = bs->opaque;
 off_t data = 0, hole = 0;
 int ret;
 
@@ -2734,6 +2771,7 @@ static int coroutine_fn 
raw_co_block_status(BlockDriverState *bs,
 }
 
 ret = find_allocation(bs, offset, , );
+s->next_zero_offset_valid = false;
 if (ret == -ENXIO) {
 /* Trailing hole */
 *pnum = bytes;
@@ -2761,6 +2799,12 @@ static int coroutine_fn 
raw_co_block_status(BlockDriverState *bs,
 }
 
 ret = BDRV_BLOCK_DATA;
+
+if (s->can_cache_next_zero_offset) {
+s->next_zero_offset_valid = true;
+s->next_zero_offset_from = 

Re: [PATCH] hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress

[Alexander Bulekov]

On 210208 2034, Philippe Mathieu-Daudé wrote:
> Per the "SD Host Controller Simplified Specification Version 2.00"
> spec. 'Table 2-4 : Block Size Register':
> 
>   Transfer Block Size [...] can be accessed only if no
>   transaction is executing (i.e., after a transaction has stopped).
>   Read operations during transfers may return an invalid value,
>   and write operations shall be ignored.
> 
> Transactions will update 'data_count', so do not modify 'blksize'
> and 'blkcnt' when 'data_count' is used. This fixes:
> 
> $ cat << EOF | qemu-system-x86_64 -qtest stdio -monitor none \
>-nographic -serial none -M pc-q35-5.0 \
>-device sdhci-pci,sd-spec-version=3 \
>-device sd-card,drive=mydrive \
>-drive if=sd,index=0,file=null-co://,format=raw,id=mydrive
>   outl 0xcf8 0x80001810
>   outl 0xcfc 0xe1068000
>   outl 0xcf8 0x80001814
>   outl 0xcf8 0x80001804
>   outw 0xcfc 0x7
>   outl 0xcf8 0x8000fa20
>   write 0xe106802c 0x1 0x0f
>   write 0xe1068004 0xc 0x2801d10101fbff28a384
>   write 0xe106800c 0x1f 
> 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
>   write 0xe1068003 0x28 
> 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
>   write 0xe1068003 0x1 0xfe
>   EOF
>   =
>   ==2686219==ERROR: AddressSanitizer: heap-buffer-overflow on address 
> 0x6153bb00 at pc 0x55ab469f456c bp 0x7ffee71be330 sp 0x7ffee71bdae0
>   WRITE of size 4 at 0x6153bb00 thread T0
>   #0 0x55ab469f456b in __asan_memcpy (qemu-system-i386+0x1cea56b)
>   #1 0x55ab483dc396 in stl_he_p include/qemu/bswap.h:353:5
>   #2 0x55ab483af5e4 in stn_he_p include/qemu/bswap.h:546:1
>   #3 0x55ab483aeb4b in flatview_read_continue softmmu/physmem.c:2839:13
>   #4 0x55ab483b0705 in flatview_read softmmu/physmem.c:2877:12
>   #5 0x55ab483b028e in address_space_read_full softmmu/physmem.c:2890:18
>   #6 0x55ab483b1294 in address_space_rw softmmu/physmem.c:2918:16
>   #7 0x55ab479374a2 in dma_memory_rw_relaxed include/sysemu/dma.h:88:12
>   #8 0x55ab47936f50 in dma_memory_rw include/sysemu/dma.h:127:12
>   #9 0x55ab4793665f in dma_memory_read include/sysemu/dma.h:145:12
>   #10 0x55ab4792f176 in sdhci_sdma_transfer_multi_blocks 
> hw/sd/sdhci.c:639:13
>   #11 0x55ab4793dc9d in sdhci_write hw/sd/sdhci.c:1129:17
>   #12 0x55ab483f8db8 in memory_region_write_accessor 
> softmmu/memory.c:491:5
>   #13 0x55ab483f868a in access_with_adjusted_size softmmu/memory.c:552:18
>   #14 0x55ab483f6da5 in memory_region_dispatch_write 
> softmmu/memory.c:1501:16
>   #15 0x55ab483c3b11 in flatview_write_continue softmmu/physmem.c:2774:23
>   #16 0x55ab483b0eb6 in flatview_write softmmu/physmem.c:2814:14
>   #17 0x55ab483b0a3e in address_space_write softmmu/physmem.c:2906:18
>   #18 0x55ab48465c56 in qtest_process_command softmmu/qtest.c:654:9
> 
>   0x6153bb00 is located 0 bytes to the right of 512-byte region 
> [0x6153b900,0x6153bb00)
>   allocated by thread T0 here:
>   #0 0x55ab469f58a7 in calloc (qemu-system-i386+0x1ceb8a7)
>   #1 0x7f21d678f9b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0)
>   #2 0x55ab479530ed in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5
>   #3 0x55ab476f102a in pci_qdev_realize hw/pci/pci.c:2108:9
>   #4 0x55ab48baaad2 in device_set_realized hw/core/qdev.c:761:13
> 
>   SUMMARY: AddressSanitizer: heap-buffer-overflow 
> (qemu-system-i386+0x1cea56b) in __asan_memcpy
>   Shadow bytes around the buggy address:
> 0x0c2a7710: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c2a7720: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0c2a7730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0c2a7740: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0c2a7750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   =>0x0c2a7760:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> 0x0c2a7770: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c2a7780: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c2a7790: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c2a77a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
> 0x0c2a77b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   Shadow byte legend (one shadow byte represents 8 application bytes):
> Addressable:   00
> Heap left redzone:   fa
> Freed heap region:   fd
>   ==2686219==ABORTING
> 
> Fixes: CVE-2020-17380
> Fixes: CVE-2020-25085
> Signed-off-by: Philippe Mathieu-Daudé 

I applied this along with <1612868085-72809-1-git-send-email-bmeng...@gmail.com>
"hw/sd: sdhci: Do not transfer any data when command fails"

I ran through the entire OSS-Fuzz corpus, and could not reproduce the
crash.

Tested-by: Alexander Bulekov 
Thanks

> ---
> Cc: Mauro Matteo Cascella 
> Cc: Alexander 

Re: [PATCH] hw/sd: sdhci: Do not transfer any data when command fails

[Alexander Bulekov]

On 210209 1854, Bin Meng wrote:
> At the end of sdhci_send_command(), it starts a data transfer if
> the command register indicates a data is associated. However the
> data transfer should only be initiated when the command execution
> has succeeded.
> 
> Cc: qemu-sta...@nongnu.org
> Fixes: CVE-2020-17380
> Fixes: CVE-2020-25085
> Reported-by: Alexander Bulekov 
> Reported-by: Sergej Schumilo (Ruhr-University Bochum)
> Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
> Reported-by: Simon Wrner (Ruhr-University Bochum)
> Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
> Signed-off-by: Bin Meng 

I applied this along with <20210208193450.2689517-1-f4...@amsat.org>
"hw/sd/sdhci: Do not modify BlockSizeRegister if transaction in progress"

I ran through the entire OSS-Fuzz corpus, and could not reproduce the
crash.

Tested-by: Alexander Bulekov 

Thanks

> ---
> 
>  hw/sd/sdhci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 8ffa539..0450110 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -326,6 +326,7 @@ static void sdhci_send_command(SDHCIState *s)
>  SDRequest request;
>  uint8_t response[16];
>  int rlen;
> +bool cmd_failure = false;
>  
>  s->errintsts = 0;
>  s->acmd12errsts = 0;
> @@ -349,6 +350,7 @@ static void sdhci_send_command(SDHCIState *s)
>  trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
> s->rspreg[1], s->rspreg[0]);
>  } else {
> +cmd_failure = true;
>  trace_sdhci_error("timeout waiting for command response");
>  if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
>  s->errintsts |= SDHC_EIS_CMDTIMEOUT;
> @@ -369,7 +371,7 @@ static void sdhci_send_command(SDHCIState *s)
>  
>  sdhci_update_irq(s);
>  
> -if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
> +if (!cmd_failure && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
>  s->data_count = 0;
>  sdhci_data_transfer(s);
>  }
> -- 
> 2.7.4
> 

Re: [PULL v2 0/3] emulated nvme fixes

[Peter Maydell]

On Thu, 11 Feb 2021 at 15:21, Klaus Jensen  wrote:
>
> From: Klaus Jensen 
>
> Hi Peter,
>
> Two small fixes for emulated nvme for regressions reported by Alexander
> Graf and Bin Meng.
>
> Sorry for the noise with v1. This should be good and also got the full
> CI treatment.
>
> The following changes since commit 83339e21d05c824ebc9131d644f25c23d0e41ecf:
>
>   Merge remote-tracking branch 
> 'remotes/stefanha-gitlab/tags/block-pull-request' into staging (2021-02-10 
> 15:42:20 +)
>
> are available in the Git repository at:
>
>   git://git.infradead.org/qemu-nvme.git tags/nvme-fixes-pull-request
>
> for you to fetch changes up to 832a59e43b5d8b8a9c2b2565008ebea1059d539d:
>
>   hw/block/nvme: fix error handling in nvme_ns_realize (2021-02-11 14:23:08 
> +0100)
>
> 
> Two small fixes for regressions reported by Alexander Graf and Bin Meng.
>
> v2: spotted one bug in the error handling.


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0
for any user-visible changes.

-- PMM

Re: [PATCH] hw/block/nvme: add broadcast nsid support flush command

[Keith Busch]

On Wed, Feb 10, 2021 at 07:42:17AM +0100, Klaus Jensen wrote:
> 
> Is that an Acked-by? ;)

Yep,

Acked-by: Keith Busch 

Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Vladimir Sementsov-Ogievskiy]

11.02.2021 17:26, Philippe Mathieu-Daudé wrote:

We are going to switch the 'null-co' default 'read-zeroes' value
from FALSE to TRUE in the next commit. First explicit the FALSE
value when it is not set.

Suggested-by: Eric Blake 
Signed-off-by: Philippe Mathieu-Daudé 
---
- Missing: 056 & 155. I couldn't figure out the proper syntax,
   any help welcomed...
- I'm unsure about 162, this doesn't seem to use the null-co
   driver but rather testing global syntax.
---
  docs/devel/testing.rst | 14 +++---
  tests/qtest/fuzz/generic_fuzz_configs.h| 11 ++-
  tests/test-bdrv-drain.c| 10 --
  tests/acceptance/virtio_check_params.py|  2 +-
  tests/perf/block/qcow2/convert-blockstatus |  6 +++---
  tests/qemu-iotests/040 |  2 +-
  tests/qemu-iotests/041 | 12 
  tests/qemu-iotests/051 |  2 +-
  tests/qemu-iotests/051.out |  2 +-
  tests/qemu-iotests/051.pc.out  |  4 ++--
  tests/qemu-iotests/087 |  6 --
  tests/qemu-iotests/118 |  2 +-
  tests/qemu-iotests/133 |  2 +-
  tests/qemu-iotests/153 |  8 
  tests/qemu-iotests/184 |  2 ++
  tests/qemu-iotests/184.out | 10 +-
  tests/qemu-iotests/218 |  3 +++
  tests/qemu-iotests/224 |  3 ++-
  tests/qemu-iotests/224.out |  8 
  tests/qemu-iotests/225 |  2 +-
  tests/qemu-iotests/227 |  4 ++--
  tests/qemu-iotests/227.out |  4 ++--
  tests/qemu-iotests/228 |  2 +-
  tests/qemu-iotests/235 |  1 +
  tests/qemu-iotests/245 |  2 +-
  tests/qemu-iotests/270 |  2 +-
  tests/qemu-iotests/283 |  3 ++-
  tests/qemu-iotests/283.out |  4 ++--
  tests/qemu-iotests/299 |  1 +
  tests/qemu-iotests/299.out |  2 +-
  tests/qemu-iotests/300 |  4 ++--


Why do you think these tests will work bad with new default?

At least everything under tests/qemu-iotests/ and tests/test-bdrv-drain are not 
about performance

tests/perf/block/qcow2/convert-blockstatus is OK with new default too.


--
Best regards,
Vladimir

Re: [PATCH v2 2/2] block/null: Enable 'read-zeroes' mode by default

[Stefan Hajnoczi]

On Thu, Feb 11, 2021 at 03:26:56PM +0100, Philippe Mathieu-Daudé wrote:
> The null-co driver is meant for (performance) testing.
> By default, read operation does nothing, the provided buffer
> is not filled with zero values and its content is unchanged.
> 
> This performance 'feature' becomes an issue from a security
> perspective.  For example, using the default null-co driver,
> buf[] is uninitialized, the blk_pread() call succeeds and we
> then access uninitialized memory:
> 
>   static int guess_disk_lchs(BlockBackend *blk,
>  int *pcylinders, int *pheads,
>  int *psectors)
>   {
>   uint8_t buf[BDRV_SECTOR_SIZE];
>   ...
> 
>   if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
>   return -1;
>   }
>   /* test msdos magic */
>   if (buf[510] != 0x55 || buf[511] != 0xaa) {
>   return -1;
>   }
> 
> We could audit all the uninitialized buffers and the
> bdrv_co_preadv() handlers, but it is simpler to change the
> default of this testing driver. Performance tests will have
> to adapt and use 'null-co,read-zeroes=off'.
> 
> Suggested-by: Max Reitz 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  block/null.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [PATCH v2 2/2] migration: dirty-bitmap: Allow control of bitmap persistance

[Vladimir Sementsov-Ogievskiy]

10.02.2021 19:53, Peter Krempa wrote:

Bitmap's source persistance is transported over the migration stream and
the destination mirrors it. In some cases the destination might want to
persist bitmaps which are not persistent on the source (e.g. the result
of merge of bitmaps from a number of layers on the source when migrating
into a squashed image) but currently it would need to create another set
of persistent bitmaps and merge them.

This patch adds a 'transform' property to the alias map which allows to
override the persistance of migrated bitmaps both on the source and
destination sides.

Signed-off-by: Peter Krempa 
---

v2:
  - grammar fixes (Eric)
  - added 'transform' object to group other possible transformations (Vladimir)
  - transformation can also be used on source (Vladimir)
  - put bmap_inner directly into DBMLoadState for deduplication  (Vladimir)

  migration/block-dirty-bitmap.c | 38 +++---
  qapi/migration.json| 20 +-
  2 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 0169f672df..a05bf74073 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -138,6 +138,13 @@ typedef struct LoadBitmapState {
  bool enabled;
  } LoadBitmapState;

+typedef struct AliasMapInnerBitmap {
+char *string;
+
+/* 'transform' properties borrowed from QAPI */
+BitmapMigrationBitmapAliasTransform *transform;
+} AliasMapInnerBitmap;
+
  /* State of the dirty bitmap migration (DBM) during load process */
  typedef struct DBMLoadState {
  uint32_t flags;
@@ -148,6 +155,7 @@ typedef struct DBMLoadState {
  BdrvDirtyBitmap *bitmap;

  bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start 
*/
+AliasMapInnerBitmap *bmap_inner;

  /*
   * cancelled
@@ -169,10 +177,6 @@ typedef struct DBMState {

  static DBMState dbm_state;

-typedef struct AliasMapInnerBitmap {
-char *string;
-} AliasMapInnerBitmap;
-
  static void free_alias_map_inner_bitmap(void *amin_ptr)
  {
  AliasMapInnerBitmap *amin = amin_ptr;
@@ -330,6 +334,7 @@ static GHashTable *construct_alias_map(const 
BitmapMigrationNodeAliasList *bbm,

  bmap_inner = g_new0(AliasMapInnerBitmap, 1);
  bmap_inner->string = g_strdup(bmap_map_to);
+bmap_inner->transform = bmba->transform;


We rely on the fact that bmba->transform will not be freed.. Is it correct, can 
migration parameters change at some unexpected moment?
Anyway it's strange that we copy string, but just add reference to transform 
structure.

Looks like we need QAPI_CLONE() here. And we can clone the whole bmba, then we 
can drop AliasMapInnerBitmap structure at all (hmm and if go this way, this 
should be done in a previous patch).

Other than that the logic and new QAPI interface looks OK for me.

Also, we need a test-case for new feature in tests/qemu-iotests/300



  g_hash_table_insert(bitmaps_map,
  g_strdup(bmap_map_from), bmap_inner);
@@ -547,6 +552,7 @@ static int add_bitmaps_to_list(DBMSaveState *s, 
BlockDriverState *bs,
  }

  FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
+BitmapMigrationBitmapAliasTransform *bitmap_transform = NULL;
  bitmap_name = bdrv_dirty_bitmap_name(bitmap);
  if (!bitmap_name) {
  continue;
@@ -567,6 +573,7 @@ static int add_bitmaps_to_list(DBMSaveState *s, 
BlockDriverState *bs,
  }

  bitmap_alias = bmap_inner->string;
+bitmap_transform = bmap_inner->transform;
  } else {
  if (strlen(bitmap_name) > UINT8_MAX) {
  error_report("Cannot migrate bitmap '%s' on node '%s': "
@@ -592,8 +599,15 @@ static int add_bitmaps_to_list(DBMSaveState *s, 
BlockDriverState *bs,
  if (bdrv_dirty_bitmap_enabled(bitmap)) {
  dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_ENABLED;
  }
-if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
-dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+if (bitmap_transform &&
+bitmap_transform->has_persistent) {
+if (bitmap_transform->persistent) {
+dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+}
+} else {
+if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
+dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+}
  }

  QSIMPLEQ_INSERT_TAIL(>dbms_list, dbms, entry);
@@ -801,6 +815,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, 
DBMLoadState *s)
  uint32_t granularity = qemu_get_be32(f);
  uint8_t flags = qemu_get_byte(f);
  LoadBitmapState *b;
+bool persistent;

  if (s->cancelled) {
  return 0;
@@ -825,7 +840,15 @@ static int dirty_bitmap_load_start(QEMUFile *f, 
DBMLoadState *s)
  return -EINVAL;
  }

-if 

Re: [PATCH] hw/sd: sdhci: Do not transfer any data when command fails

[Alexander Bulekov]

On 210209 1854, Bin Meng wrote:
> At the end of sdhci_send_command(), it starts a data transfer if
> the command register indicates a data is associated. However the
> data transfer should only be initiated when the command execution
> has succeeded.
> 
> Cc: qemu-sta...@nongnu.org
> Fixes: CVE-2020-17380
> Fixes: CVE-2020-25085
> Reported-by: Alexander Bulekov 
> Reported-by: Sergej Schumilo (Ruhr-University Bochum)
> Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
> Reported-by: Simon Wrner (Ruhr-University Bochum)

Reported-by: Muhammad Ramdhan 
(don't know how to get the email from a launchpad report)

and probably:
Buglink: https://bugs.launchpad.net/qemu/+bug/1909418

> Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
> Signed-off-by: Bin Meng 
> ---
> 
>  hw/sd/sdhci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 8ffa539..0450110 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -326,6 +326,7 @@ static void sdhci_send_command(SDHCIState *s)
>  SDRequest request;
>  uint8_t response[16];
>  int rlen;
> +bool cmd_failure = false;
>  
>  s->errintsts = 0;
>  s->acmd12errsts = 0;
> @@ -349,6 +350,7 @@ static void sdhci_send_command(SDHCIState *s)
>  trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
> s->rspreg[1], s->rspreg[0]);
>  } else {
> +cmd_failure = true;
>  trace_sdhci_error("timeout waiting for command response");
>  if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
>  s->errintsts |= SDHC_EIS_CMDTIMEOUT;
> @@ -369,7 +371,7 @@ static void sdhci_send_command(SDHCIState *s)
>  
>  sdhci_update_irq(s);
>  
> -if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
> +if (!cmd_failure && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
>  s->data_count = 0;
>  sdhci_data_transfer(s);
>  }
> -- 
> 2.7.4
> 

Re: [PULL v4 14/27] io: add qio_channel_readv_full_all_eof & qio_channel_readv_full_all helpers

[Jag Raman]

> On Feb 11, 2021, at 10:46 AM, Daniel P. Berrangé  wrote:
> 
> On Thu, Feb 11, 2021 at 04:34:40PM +0100, Max Reitz wrote:
>> On 10.02.21 10:26, Stefan Hajnoczi wrote:
>>> From: Elena Ufimtseva 
>>> 
>>> Adds qio_channel_readv_full_all_eof() and qio_channel_readv_full_all()
>>> to read both data and FDs. Refactors existing code to use these helpers.
>>> 
>>> Signed-off-by: Elena Ufimtseva 
>>> Signed-off-by: John G Johnson 
>>> Signed-off-by: Jagannathan Raman 
>>> Acked-by: Daniel P. Berrangé 
>>> Message-id: 
>>> b059c4cc0fb741e794d644c144cc21372cad877d.1611938319.git.jag.ra...@oracle.com
>>> Signed-off-by: Stefan Hajnoczi 
>>> ---
>>>  include/io/channel.h |  53 +++
>>>  io/channel.c | 101 ++-
>>>  2 files changed, 134 insertions(+), 20 deletions(-)
>> 
>> [...]
>> 
>>> diff --git a/io/channel.c b/io/channel.c
>>> index 0d4b8b5160..4555021b62 100644
>>> --- a/io/channel.c
>>> +++ b/io/channel.c
>> 
>> [...]
>> 
>>> @@ -135,20 +193,23 @@ int qio_channel_readv_all_eof(QIOChannel *ioc,
>>>  return ret;
>>>  }
>>> -int qio_channel_readv_all(QIOChannel *ioc,
>>> -  const struct iovec *iov,
>>> -  size_t niov,
>>> -  Error **errp)
>>> +int qio_channel_readv_full_all(QIOChannel *ioc,
>>> +   const struct iovec *iov,
>>> +   size_t niov,
>>> +   int **fds, size_t *nfds,
>>> +   Error **errp)
>>>  {
>>> -int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp);
>>> +int ret = qio_channel_readv_full_all_eof(ioc, iov, niov, fds, nfds, 
>>> errp);
>>>  if (ret == 0) {
>>> -ret = -1;
>>> -error_setg(errp,
>>> -   "Unexpected end-of-file before all bytes were read");
>>> -} else if (ret == 1) {
>>> -ret = 0;
>>> +error_prepend(errp,
>>> +  "Unexpected end-of-file before all data were read.");
>>> +return -1;
>> 
>> This change breaks iotest 083 (i.e., it segfaults), because
>> qio_channel_readv_full_all_eof() doesn’t set *errp when it returns 0, so
>> there is no error to prepend.
> 
> Opps, yes, this needs to be error_setg() not error_prepend()

Hi Max & Daniel,

We will send a patch shortly to address this issue.

Thank you very much!

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

Re: [PULL v4 14/27] io: add qio_channel_readv_full_all_eof & qio_channel_readv_full_all helpers

[Daniel P . Berrangé]

On Thu, Feb 11, 2021 at 04:34:40PM +0100, Max Reitz wrote:
> On 10.02.21 10:26, Stefan Hajnoczi wrote:
> > From: Elena Ufimtseva 
> > 
> > Adds qio_channel_readv_full_all_eof() and qio_channel_readv_full_all()
> > to read both data and FDs. Refactors existing code to use these helpers.
> > 
> > Signed-off-by: Elena Ufimtseva 
> > Signed-off-by: John G Johnson 
> > Signed-off-by: Jagannathan Raman 
> > Acked-by: Daniel P. Berrangé 
> > Message-id: 
> > b059c4cc0fb741e794d644c144cc21372cad877d.1611938319.git.jag.ra...@oracle.com
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> >   include/io/channel.h |  53 +++
> >   io/channel.c | 101 ++-
> >   2 files changed, 134 insertions(+), 20 deletions(-)
> 
> [...]
> 
> > diff --git a/io/channel.c b/io/channel.c
> > index 0d4b8b5160..4555021b62 100644
> > --- a/io/channel.c
> > +++ b/io/channel.c
> 
> [...]
> 
> > @@ -135,20 +193,23 @@ int qio_channel_readv_all_eof(QIOChannel *ioc,
> >   return ret;
> >   }
> > -int qio_channel_readv_all(QIOChannel *ioc,
> > -  const struct iovec *iov,
> > -  size_t niov,
> > -  Error **errp)
> > +int qio_channel_readv_full_all(QIOChannel *ioc,
> > +   const struct iovec *iov,
> > +   size_t niov,
> > +   int **fds, size_t *nfds,
> > +   Error **errp)
> >   {
> > -int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp);
> > +int ret = qio_channel_readv_full_all_eof(ioc, iov, niov, fds, nfds, 
> > errp);
> >   if (ret == 0) {
> > -ret = -1;
> > -error_setg(errp,
> > -   "Unexpected end-of-file before all bytes were read");
> > -} else if (ret == 1) {
> > -ret = 0;
> > +error_prepend(errp,
> > +  "Unexpected end-of-file before all data were read.");
> > +return -1;
> 
> This change breaks iotest 083 (i.e., it segfaults), because
> qio_channel_readv_full_all_eof() doesn’t set *errp when it returns 0, so
> there is no error to prepend.

Opps, yes, this needs to be error_setg() not error_prepend()


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

Re: [PATCH 2/2] hw/block/nvme: add write uncorrectable command

[Keith Busch]

On Thu, Feb 11, 2021 at 09:43:05AM +0100, Klaus Jensen wrote:
> On Feb 11 12:37, Keith Busch wrote:
> 
> > Is there a use case with a real qemu guest wanting this?
> 
> Like for the extended metadata case (which also does not have a lot of
> "public" exposure, but definitely have enterprise use), our main
> motivation here was to ease testing for compliance suites and frameworks
> like SPDK. 

I'm okay with the metadata patches.

> I'm honestly have no clue what so ever what a real world use
> of Write Uncorrectable would be. It's been in the spec since 1.0, so
> there must have been some reason, Is it just to align with SCSI WRITE
> LONG? I'm not SCSI expert at all, but from what I can read it looks like
> that was also intended as a feature for testing read error conditions.

I don't think it's for testing purposes.

If you need to send a burst of non-atomic writes (ex: writing a RAID
stripe), a power failure can result in an inconsistent state where you
don't know at a block level which ones have old data or new data. If you
Write Uncorrectable first, you can never read old data, and thus have no
"write hole".

Journalling solves this better, and I'm not aware of any real
implementation relying on uncorrectable.

Re: [PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Alexander Bulekov]

On 210211 1526, Philippe Mathieu-Daudé wrote:
> The null-co driver doesn't zeroize buffer in its default config,
> because it is designed for testing and tests want to run fast.
> However this confuses security researchers (access to uninit
> buffers).
> 

Interesting.. Is there an example bug report, where it raised alarms
because of an un-zeroed null-co:// buffer?
-Alex

> A one-line patch supposed which became a painful one, because
> there is so many different syntax to express the same usage:
> 
>  opt = qdict_new();
>  qdict_put_str(opt, "read-zeroes", "off");
>  null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL,
>  _abort);
> 
> vm.qmp('blockdev-add', driver='null-co', read_zeroes=False, ...)
> 
> vm.add_drive_raw("id=drive0,driver=null-co,read-zeroes=off,if=none")
> 
> blk0 = { 'node-name': 'src',
> 'driver': 'null-co',
> 'read-zeroes': 'off' }
> 
> 'file': {
> 'driver': 'null-co',
> 'read-zeroes': False,
> }
> 
> "file": {
> "driver": "null-co",
> "read-zeroes": "off"
> }
> 
> { "execute": "blockdev-add",
>   "arguments": {
>   "driver": "null-co",
>   "read-zeroes": false,
>   "node-name": "disk0"
> }
> }
> 
> opts = {'driver': 'null-co,read-zeroes=off', 'node-name': 'root', 'size': 
> 1024}
> 
> qemu -drive driver=null-co,read-zeroes=off
> 
> qemu-io ... "json:{'driver': 'null-co', 'read-zeroes': false, 'size': 65536}"
> 
> qemu-img null-co://,read-zeroes=off
> 
> qemu-img ... -o 
> data_file="json:{'driver':'null-co',,'read-zeroes':false,,'size':'4294967296'}"
> 
> There are probably more.
> 
> Anyhow, the iotests I am not sure and should be audited are 056, 155
> (I don't understand the syntax) and 162.
> 
> Please review,
> 
> Phil.
> 
> Philippe Mathieu-Daud=C3=A9 (2):
>   block: Explicit null-co uses 'read-zeroes=3Dfalse'
>   block/null: Enable 'read-zeroes' mode by default
> 
>  docs/devel/testing.rst | 14 +++---
>  tests/qtest/fuzz/generic_fuzz_configs.h| 11 ++-
>  block/null.c   |  2 +-
>  tests/test-bdrv-drain.c| 10 --
>  tests/acceptance/virtio_check_params.py|  2 +-
>  tests/perf/block/qcow2/convert-blockstatus |  6 +++---
>  tests/qemu-iotests/040 |  2 +-
>  tests/qemu-iotests/041 | 12 
>  tests/qemu-iotests/051 |  2 +-
>  tests/qemu-iotests/051.out |  2 +-
>  tests/qemu-iotests/051.pc.out  |  4 ++--
>  tests/qemu-iotests/087 |  6 --
>  tests/qemu-iotests/118 |  2 +-
>  tests/qemu-iotests/133 |  2 +-
>  tests/qemu-iotests/153 |  8 
>  tests/qemu-iotests/184 |  2 ++
>  tests/qemu-iotests/184.out | 10 +-
>  tests/qemu-iotests/218 |  3 +++
>  tests/qemu-iotests/224 |  3 ++-
>  tests/qemu-iotests/224.out |  8 
>  tests/qemu-iotests/225 |  2 +-
>  tests/qemu-iotests/227 |  4 ++--
>  tests/qemu-iotests/227.out |  4 ++--
>  tests/qemu-iotests/228 |  2 +-
>  tests/qemu-iotests/235 |  1 +
>  tests/qemu-iotests/245 |  2 +-
>  tests/qemu-iotests/270 |  2 +-
>  tests/qemu-iotests/283 |  3 ++-
>  tests/qemu-iotests/283.out |  4 ++--
>  tests/qemu-iotests/299 |  1 +
>  tests/qemu-iotests/299.out |  2 +-
>  tests/qemu-iotests/300 |  4 ++--
>  32 files changed, 82 insertions(+), 60 deletions(-)
> 
> --=20
> 2.26.2
> 
> 

Re: [PULL v4 14/27] io: add qio_channel_readv_full_all_eof & qio_channel_readv_full_all helpers

[Max Reitz]

On 10.02.21 10:26, Stefan Hajnoczi wrote:

From: Elena Ufimtseva 

Adds qio_channel_readv_full_all_eof() and qio_channel_readv_full_all()
to read both data and FDs. Refactors existing code to use these helpers.

Signed-off-by: Elena Ufimtseva 
Signed-off-by: John G Johnson 
Signed-off-by: Jagannathan Raman 
Acked-by: Daniel P. Berrangé 
Message-id: 
b059c4cc0fb741e794d644c144cc21372cad877d.1611938319.git.jag.ra...@oracle.com
Signed-off-by: Stefan Hajnoczi 
---
  include/io/channel.h |  53 +++
  io/channel.c | 101 ++-
  2 files changed, 134 insertions(+), 20 deletions(-)


[...]


diff --git a/io/channel.c b/io/channel.c
index 0d4b8b5160..4555021b62 100644
--- a/io/channel.c
+++ b/io/channel.c


[...]


@@ -135,20 +193,23 @@ int qio_channel_readv_all_eof(QIOChannel *ioc,
  return ret;
  }
  
-int qio_channel_readv_all(QIOChannel *ioc,

-  const struct iovec *iov,
-  size_t niov,
-  Error **errp)
+int qio_channel_readv_full_all(QIOChannel *ioc,
+   const struct iovec *iov,
+   size_t niov,
+   int **fds, size_t *nfds,
+   Error **errp)
  {
-int ret = qio_channel_readv_all_eof(ioc, iov, niov, errp);
+int ret = qio_channel_readv_full_all_eof(ioc, iov, niov, fds, nfds, errp);
  
  if (ret == 0) {

-ret = -1;
-error_setg(errp,
-   "Unexpected end-of-file before all bytes were read");
-} else if (ret == 1) {
-ret = 0;
+error_prepend(errp,
+  "Unexpected end-of-file before all data were read.");
+return -1;


This change breaks iotest 083 (i.e., it segfaults), because 
qio_channel_readv_full_all_eof() doesn’t set *errp when it returns 0, so 
there is no error to prepend.


Also, I think usually we don’t let error messages end in full stops.

Max

Re: [PATCH] hw/block/nvme: drain namespaces on sq deletion

[Klaus Jensen]

On Feb 11 22:49, Minwoo Im wrote:
> On 21-02-11 13:07:08, Klaus Jensen wrote:
> > On Feb 11 11:49, Minwoo Im wrote:
> > > On 21-01-27 14:15:05, Klaus Jensen wrote:
> > > > From: Klaus Jensen 
> > > > 
> > > > For most commands, when issuing an AIO, the BlockAIOCB is stored in the
> > > > NvmeRequest aiocb pointer when the AIO is issued. The purpose of storing
> > > > this is to allow the AIO to be cancelled when deleting submission
> > > > queues (it is currently not used for Abort).
> > > > 
> > > > Since the addition of the Dataset Management command and Zoned
> > > > Namespaces, NvmeRequests may involve more than one AIO and the AIOs are
> > > > issued without saving a reference to the BlockAIOCB. This is a problem
> > > > since nvme_del_sq will attempt to cancel outstanding AIOs, potentially
> > > > with an invalid BlockAIOCB.
> > > > 
> > > > Fix this by instead of explicitly cancelling the requests, just allow
> > > > the AIOs to complete by draining the namespace blockdevs.
> > > > 
> > > > Signed-off-by: Klaus Jensen 
> > > > ---
> > > >  hw/block/nvme.c | 18 +-
> > > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > > index 316858fd8adf..91f6fb6da1e2 100644
> > > > --- a/hw/block/nvme.c
> > > > +++ b/hw/block/nvme.c
> > > > @@ -403,6 +403,7 @@ static void nvme_req_clear(NvmeRequest *req)
> > > >  {
> > > >  req->ns = NULL;
> > > >  req->opaque = NULL;
> > > > +req->aiocb = NULL;
> > > >  memset(>cqe, 0x0, sizeof(req->cqe));
> > > >  req->status = NVME_SUCCESS;
> > > >  }
> > > > @@ -2396,6 +2397,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, 
> > > > NvmeRequest *req)
> > > >  NvmeSQueue *sq;
> > > >  NvmeCQueue *cq;
> > > >  uint16_t qid = le16_to_cpu(c->qid);
> > > > +int i;
> > > >  
> > > >  if (unlikely(!qid || nvme_check_sqid(n, qid))) {
> > > >  trace_pci_nvme_err_invalid_del_sq(qid);
> > > > @@ -2404,12 +2406,18 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, 
> > > > NvmeRequest *req)
> > > >  
> > > >  trace_pci_nvme_del_sq(qid);
> > > >  
> > > > -sq = n->sq[qid];
> > > > -while (!QTAILQ_EMPTY(>out_req_list)) {
> > > > -r = QTAILQ_FIRST(>out_req_list);
> > > > -assert(r->aiocb);
> > > > -blk_aio_cancel(r->aiocb);
> > > > +for (i = 1; i <= n->num_namespaces; i++) {
> > > > +NvmeNamespace *ns = nvme_ns(n, i);
> > > > +if (!ns) {
> > > > +continue;
> > > > +}
> > > > +
> > > > +nvme_ns_drain(ns);
> > > 
> > > If we just drain the entire namespaces here, commands which has nothing
> > > to do with the target sq to be deleted will be drained.  And this might
> > > be a burden for a single SQ deletion.
> > > 
> > 
> > That is true. But how often would you dynamically delete and create I/O
> > submission queues in the fast path?
> 
> Delete I/O queues are not that often in the working NVMe controller, but
> it might be a good case for the exception test from the host side like:
> I/O queue deletion during I/O workloads.  If delete I/O queues are
> returning by aborting their own requests only and quickly respond to the
> host, then I think it might be a good one to test with.  Handling
> requests gracefully sometimes don't cause corner cases from the host
> point-of-view.  But, QEMU is not only for the host testing, so I am not
> sure that QEMU NVMe device should handle things gracefully or try to do
> something exactly as the real hardware(but, we don't know all the
> hardware behavior ;)).
> 
> (But, Right. If I'm only talking about the kernel, then kernel does not
> delete queues during the fast-path hot workloads.  But it's sometimes
> great to test something on their own driver or application)
> 
> > > By the way, agree with the multiple AIOs references problem for newly 
> > > added
> > > commands.  But, shouldn't we manage the inflight AIO request references 
> > > for
> > > the newlly added commands with some other way and kill them all
> > > explicitly as it was?  Maybe some of list for AIOCBs?
> > 
> > I was hesitant to add more stuff to NvmeRequest (like a QTAILQ to track
> > this). Getting a steady-state with draining was an easy fix.
> 
> Graceful handling is easy to go with.  I am not expert for the overall
> purpose of the QEMU NVMe device model, but I'm curious that which one we
> need to take first between `Easy to go vs. What device should do`.
> 

Alright, point taken :)

I'll post an RFC patch that tracks this properly instead of halfass'ing
it.


signature.asc
Description: PGP signature

[PULL v2 2/3] hw/block/nvme: Fix a build error in nvme_get_feature()

[Klaus Jensen]

From: Bin Meng 

Current QEMU HEAD nvme.c does not compile with the default GCC 5.4
on a Ubuntu 16.04 host:

  hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this 
function [-Werror=maybe-uninitialized]
 trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
 ^
  hw/block/nvme.c:3150:14: note: ‘result’ was declared here
 uint32_t result;
  ^

Explicitly initialize the result to fix it.

Fixes: aa5e55e3b07e ("hw/block/nvme: open code for volatile write cache")
Fixes: Coverity CID 1446371
Signed-off-by: Bin Meng 
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 02390f1f5230..fb83636abdc1 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3228,6 +3228,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest 
*req)
 result = ns->features.err_rec;
 goto out;
 case NVME_VOLATILE_WRITE_CACHE:
+result = 0;
 for (i = 1; i <= n->num_namespaces; i++) {
 ns = nvme_ns(n, i);
 if (!ns) {
-- 
2.30.0

[PULL v2 3/3] hw/block/nvme: fix error handling in nvme_ns_realize

[Klaus Jensen]

From: Klaus Jensen 

nvme_ns_realize passes errp to nvme_register_namespaces, but then try to
prepend errp with local_err.

Just remove the local_err and use errp directly.

Fixes: 15d024d4aa9b ("hw/block/nvme: split setup and register for namespace")
Cc: Minwoo Im 
Reviewed-by: Minwoo Im 
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme-ns.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index dfed71a950fa..93ac6e107a09 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -358,17 +358,12 @@ static void nvme_ns_realize(DeviceState *dev, Error 
**errp)
 NvmeNamespace *ns = NVME_NS(dev);
 BusState *s = qdev_get_parent_bus(dev);
 NvmeCtrl *n = NVME(s->parent);
-Error *local_err = NULL;
 
-if (nvme_ns_setup(ns, _err)) {
-error_propagate_prepend(errp, local_err,
-"could not setup namespace: ");
+if (nvme_ns_setup(ns, errp)) {
 return;
 }
 
 if (nvme_register_namespace(n, ns, errp)) {
-error_propagate_prepend(errp, local_err,
-"could not register namespace: ");
 return;
 }
 
-- 
2.30.0

[PULL v2 1/3] hw/block/nvme: fix legacy namespace registration

[Klaus Jensen]

From: Klaus Jensen 

Moving namespace registration to the nvme-ns realization function had
the unintended side-effect of breaking legacy namespace registration.
Fix this.

Fixes: 15d024d4aa9b ("hw/block/nvme: split setup and register for namespace")
Reported-by: Alexander Graf 
Cc: Minwoo Im 
Tested-by: Alexander Graf 
Reviewed-by: Minwoo Im 
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5ce21b7100b3..02390f1f5230 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4507,6 +4507,10 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
**errp)
 if (nvme_ns_setup(ns, errp)) {
 return;
 }
+
+if (nvme_register_namespace(n, ns, errp)) {
+return;
+}
 }
 }
 
-- 
2.30.0

[PULL v2 0/3] emulated nvme fixes

[Klaus Jensen]

From: Klaus Jensen 

Hi Peter,

Two small fixes for emulated nvme for regressions reported by Alexander
Graf and Bin Meng.

Sorry for the noise with v1. This should be good and also got the full
CI treatment.

The following changes since commit 83339e21d05c824ebc9131d644f25c23d0e41ecf:

  Merge remote-tracking branch 
'remotes/stefanha-gitlab/tags/block-pull-request' into staging (2021-02-10 
15:42:20 +)

are available in the Git repository at:

  git://git.infradead.org/qemu-nvme.git tags/nvme-fixes-pull-request

for you to fetch changes up to 832a59e43b5d8b8a9c2b2565008ebea1059d539d:

  hw/block/nvme: fix error handling in nvme_ns_realize (2021-02-11 14:23:08 
+0100)


Two small fixes for regressions reported by Alexander Graf and Bin Meng.

v2: spotted one bug in the error handling.



Bin Meng (1):
  hw/block/nvme: Fix a build error in nvme_get_feature()

Klaus Jensen (2):
  hw/block/nvme: fix legacy namespace registration
  hw/block/nvme: fix error handling in nvme_ns_realize

 hw/block/nvme-ns.c | 7 +--
 hw/block/nvme.c| 5 +
 2 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.30.0

Re: [PATCH] hw/block/nvme: fix legacy namespace registration

[Alexander Graf]

On 11.02.21 11:54, Klaus Jensen wrote:

From: Klaus Jensen 

Moving namespace registration to the nvme-ns realization function had
the unintended side-effect of breaking legacy namespace registration.
Fix this.

Fixes: 15d024d4aa9b ("hw/block/nvme: split setup and register for namespace")
Reported-by: Alexander Graf 
Cc: Minwoo Im 
Signed-off-by: Klaus Jensen 



I can confirm that this fixes the upstream regression. Thanks a lot. 
Please work on a way with Peter to pull this into the tree as quickly as 
possible, so that we maintain a working master branch with NVMe.


Tested-by: Alexander Graf 


Thanks!

Alex



---
  hw/block/nvme.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5ce21b7100b3..d36e14fe13e2 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4507,6 +4507,12 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
**errp)
  if (nvme_ns_setup(ns, errp)) {
  return;
  }
+
+if (nvme_register_namespace(n, ns, errp)) {
+error_propagate_prepend(errp, local_err,
+"could not register namespace: ");
+return;
+}
  }
  }
  

Re: [PULL 32/56] hw/block/nvme: split setup and register for namespace

[Alexander Graf]

Hi Klaus,

On 09.02.21 08:30, Klaus Jensen wrote:

From: Minwoo Im 

In NVMe, namespace is being attached to process I/O.  We register NVMe
namespace to a controller via nvme_register_namespace() during
nvme_ns_setup().  This is main reason of receiving NvmeCtrl object
instance to this function to map the namespace to a controller.

To make namespace instance more independent, it should be split into two
parts: setup and register.  This patch split them into two differnt
parts, and finally nvme_ns_setup() does not have nothing to do with
NvmeCtrl instance at all.

This patch is a former patch to introduce NVMe subsystem scheme to the
existing design especially for multi-path.  In that case, it should be
split into two to make namespace independent from a controller.

Signed-off-by: Minwoo Im 
Signed-off-by: Klaus Jensen 



In latest master, I can no longer access NVMe devices from edk2. Git 
bisect pointed me to this commit.


To reproduce:

  $ ./build/qemu-system-x86_64 -enable-kvm -pflash 
build/pc-bios/edk2-x86_64-code.fd -drive 
file=image.raw,if=none,id=d,snapshot=on -device nvme,serial=1234,drive=d 
-nographic -net none


You will see that before this commit, edk2 is able to enumerate the 
block device, while after this commit it does not find it.



good:

Mapping table
      FS0: Alias(s):HD1b:;BLK2:
         
PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(1,GPT,7A866FF6-0A12-4911-A4ED-D0565BEB41A2,0x80,0x64000)

     BLK0: Alias(s):
          PciRoot(0x0)/Pci(0x1,0x1)/Ata(0x0)
     BLK1: Alias(s):
         
PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)

     BLK3: Alias(s):
         
PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(2,GPT,F070566B-C58D-4F13-9B92-64F1794385E7,0x64080,0x4)

     BLK4: Alias(s):
         
PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(3,GPT,EDE86BE3-C64F-4ACB-B4AA-5E6C0135D335,0xA4080,0x315BF1B)



bad:

Mapping table
     BLK0: Alias(s):
          PciRoot(0x0)/Pci(0x1,0x1)/Ata(0x0)



Thanks,

Alex

[PATCH v2 2/2] block/null: Enable 'read-zeroes' mode by default

[Philippe Mathieu-Daudé]

The null-co driver is meant for (performance) testing.
By default, read operation does nothing, the provided buffer
is not filled with zero values and its content is unchanged.

This performance 'feature' becomes an issue from a security
perspective.  For example, using the default null-co driver,
buf[] is uninitialized, the blk_pread() call succeeds and we
then access uninitialized memory:

  static int guess_disk_lchs(BlockBackend *blk,
 int *pcylinders, int *pheads,
 int *psectors)
  {
  uint8_t buf[BDRV_SECTOR_SIZE];
  ...

  if (blk_pread(blk, 0, buf, BDRV_SECTOR_SIZE) < 0) {
  return -1;
  }
  /* test msdos magic */
  if (buf[510] != 0x55 || buf[511] != 0xaa) {
  return -1;
  }

We could audit all the uninitialized buffers and the
bdrv_co_preadv() handlers, but it is simpler to change the
default of this testing driver. Performance tests will have
to adapt and use 'null-co,read-zeroes=off'.

Suggested-by: Max Reitz 
Signed-off-by: Philippe Mathieu-Daudé 
---
 block/null.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/null.c b/block/null.c
index cc9b1d4ea72..f9658fd70ac 100644
--- a/block/null.c
+++ b/block/null.c
@@ -93,7 +93,7 @@ static int null_file_open(BlockDriverState *bs, QDict 
*options, int flags,
 error_setg(errp, "latency-ns is invalid");
 ret = -EINVAL;
 }
-s->read_zeroes = qemu_opt_get_bool(opts, NULL_OPT_ZEROES, false);
+s->read_zeroes = qemu_opt_get_bool(opts, NULL_OPT_ZEROES, true);
 qemu_opts_del(opts);
 bs->supported_write_flags = BDRV_REQ_FUA;
 return ret;
-- 
2.26.2

[PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Philippe Mathieu-Daudé]

We are going to switch the 'null-co' default 'read-zeroes' value
from FALSE to TRUE in the next commit. First explicit the FALSE
value when it is not set.

Suggested-by: Eric Blake 
Signed-off-by: Philippe Mathieu-Daudé 
---
- Missing: 056 & 155. I couldn't figure out the proper syntax,
  any help welcomed...
- I'm unsure about 162, this doesn't seem to use the null-co
  driver but rather testing global syntax.
---
 docs/devel/testing.rst | 14 +++---
 tests/qtest/fuzz/generic_fuzz_configs.h| 11 ++-
 tests/test-bdrv-drain.c| 10 --
 tests/acceptance/virtio_check_params.py|  2 +-
 tests/perf/block/qcow2/convert-blockstatus |  6 +++---
 tests/qemu-iotests/040 |  2 +-
 tests/qemu-iotests/041 | 12 
 tests/qemu-iotests/051 |  2 +-
 tests/qemu-iotests/051.out |  2 +-
 tests/qemu-iotests/051.pc.out  |  4 ++--
 tests/qemu-iotests/087 |  6 --
 tests/qemu-iotests/118 |  2 +-
 tests/qemu-iotests/133 |  2 +-
 tests/qemu-iotests/153 |  8 
 tests/qemu-iotests/184 |  2 ++
 tests/qemu-iotests/184.out | 10 +-
 tests/qemu-iotests/218 |  3 +++
 tests/qemu-iotests/224 |  3 ++-
 tests/qemu-iotests/224.out |  8 
 tests/qemu-iotests/225 |  2 +-
 tests/qemu-iotests/227 |  4 ++--
 tests/qemu-iotests/227.out |  4 ++--
 tests/qemu-iotests/228 |  2 +-
 tests/qemu-iotests/235 |  1 +
 tests/qemu-iotests/245 |  2 +-
 tests/qemu-iotests/270 |  2 +-
 tests/qemu-iotests/283 |  3 ++-
 tests/qemu-iotests/283.out |  4 ++--
 tests/qemu-iotests/299 |  1 +
 tests/qemu-iotests/299.out |  2 +-
 tests/qemu-iotests/300 |  4 ++--
 31 files changed, 81 insertions(+), 59 deletions(-)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 209f9d8172f..45f1a674384 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -216,13 +216,13 @@ code.
 Both Python and Bash frameworks in iotests provide helpers to manage test
 images. They can be used to create and clean up images under the test
 directory. If no I/O or any protocol specific feature is needed, it is often
-more convenient to use the pseudo block driver, ``null-co://``, as the test
-image, which doesn't require image creation or cleaning up. Avoid system-wide
-devices or files whenever possible, such as ``/dev/null`` or ``/dev/zero``.
-Otherwise, image locking implications have to be considered.  For example,
-another application on the host may have locked the file, possibly leading to a
-test failure.  If using such devices are explicitly desired, consider adding
-``locking=off`` option to disable image locking.
+more convenient to use the pseudo block driver, ``null-co://,read-zeroes=off``,
+as the test image, which doesn't require image creation or cleaning up. Avoid
+system-wide devices or files whenever possible, such as ``/dev/null`` or
+``/dev/zero``. Otherwise, image locking implications have to be considered.
+For example, another application on the host may have locked the file, possibly
+leading to a test failure.  If using such devices are explicitly desired,
+consider adding ``locking=off`` option to disable image locking.
 
 Test case groups
 
diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h 
b/tests/qtest/fuzz/generic_fuzz_configs.h
index 5d599765c4b..dd5a7aeff0d 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -38,13 +38,13 @@ const generic_fuzz_config predefined_configs[] = {
 },{
 .name = "virtio-blk",
 .args = "-machine q35 -device virtio-blk,drive=disk0 "
-"-drive file=null-co://,id=disk0,if=none,format=raw",
+"-drive 
file=null-co://,file.read-zeroes=off,id=disk0,if=none,format=raw",
 .objects = "virtio*",
 },{
 .name = "virtio-scsi",
 .args = "-machine q35 -device virtio-scsi,num_queues=8 "
 "-device scsi-hd,drive=disk0 "
-"-drive file=null-co://,id=disk0,if=none,format=raw",
+"-drive 
file=null-co://,file.read-zeroes=off,id=disk0,if=none,format=raw",
 .objects = "scsi* virtio*",
 },{
 .name = "virtio-gpu",
@@ -119,7 +119,7 @@ const generic_fuzz_config predefined_configs[] = {
 },{
 .name = "ahci-hd",
 .args = "-machine q35 -nodefaults "
-"-drive file=null-co://,if=none,format=raw,id=disk0 "
+"-drive 
file=null-co://,file.read-zeroes=off,if=none,format=raw,id=disk0 "
 "-device ide-hd,drive=disk0",
 .objects = "*ahci*",
 },{
@@ 

[PATCH v2 0/2] block: Use 'read-zeroes=true' mode by default with 'null-co' driver

[Philippe Mathieu-Daudé]

The null-co driver doesn't zeroize buffer in its default config,
because it is designed for testing and tests want to run fast.
However this confuses security researchers (access to uninit
buffers).

A one-line patch supposed which became a painful one, because
there is so many different syntax to express the same usage:

 opt = qdict_new();
 qdict_put_str(opt, "read-zeroes", "off");
 null_bs = bdrv_open("null-co://", NULL, opt, BDRV_O_RDWR | BDRV_O_PROTOCOL,
 _abort);

vm.qmp('blockdev-add', driver='null-co', read_zeroes=False, ...)

vm.add_drive_raw("id=drive0,driver=null-co,read-zeroes=off,if=none")

blk0 = { 'node-name': 'src',
'driver': 'null-co',
'read-zeroes': 'off' }

'file': {
'driver': 'null-co',
'read-zeroes': False,
}

"file": {
"driver": "null-co",
"read-zeroes": "off"
}

{ "execute": "blockdev-add",
  "arguments": {
  "driver": "null-co",
  "read-zeroes": false,
  "node-name": "disk0"
}
}

opts = {'driver': 'null-co,read-zeroes=off', 'node-name': 'root', 'size': 1024}

qemu -drive driver=null-co,read-zeroes=off

qemu-io ... "json:{'driver': 'null-co', 'read-zeroes': false, 'size': 65536}"

qemu-img null-co://,read-zeroes=off

qemu-img ... -o 
data_file="json:{'driver':'null-co',,'read-zeroes':false,,'size':'4294967296'}"

There are probably more.

Anyhow, the iotests I am not sure and should be audited are 056, 155
(I don't understand the syntax) and 162.

Please review,

Phil.

Philippe Mathieu-Daud=C3=A9 (2):
  block: Explicit null-co uses 'read-zeroes=3Dfalse'
  block/null: Enable 'read-zeroes' mode by default

 docs/devel/testing.rst | 14 +++---
 tests/qtest/fuzz/generic_fuzz_configs.h| 11 ++-
 block/null.c   |  2 +-
 tests/test-bdrv-drain.c| 10 --
 tests/acceptance/virtio_check_params.py|  2 +-
 tests/perf/block/qcow2/convert-blockstatus |  6 +++---
 tests/qemu-iotests/040 |  2 +-
 tests/qemu-iotests/041 | 12 
 tests/qemu-iotests/051 |  2 +-
 tests/qemu-iotests/051.out |  2 +-
 tests/qemu-iotests/051.pc.out  |  4 ++--
 tests/qemu-iotests/087 |  6 --
 tests/qemu-iotests/118 |  2 +-
 tests/qemu-iotests/133 |  2 +-
 tests/qemu-iotests/153 |  8 
 tests/qemu-iotests/184 |  2 ++
 tests/qemu-iotests/184.out | 10 +-
 tests/qemu-iotests/218 |  3 +++
 tests/qemu-iotests/224 |  3 ++-
 tests/qemu-iotests/224.out |  8 
 tests/qemu-iotests/225 |  2 +-
 tests/qemu-iotests/227 |  4 ++--
 tests/qemu-iotests/227.out |  4 ++--
 tests/qemu-iotests/228 |  2 +-
 tests/qemu-iotests/235 |  1 +
 tests/qemu-iotests/245 |  2 +-
 tests/qemu-iotests/270 |  2 +-
 tests/qemu-iotests/283 |  3 ++-
 tests/qemu-iotests/283.out |  4 ++--
 tests/qemu-iotests/299 |  1 +
 tests/qemu-iotests/299.out |  2 +-
 tests/qemu-iotests/300 |  4 ++--
 32 files changed, 82 insertions(+), 60 deletions(-)

--=20
2.26.2

Re: [PATCH] hw/block/nvme: drain namespaces on sq deletion

[Minwoo Im]

On 21-02-11 13:07:08, Klaus Jensen wrote:
> On Feb 11 11:49, Minwoo Im wrote:
> > On 21-01-27 14:15:05, Klaus Jensen wrote:
> > > From: Klaus Jensen 
> > > 
> > > For most commands, when issuing an AIO, the BlockAIOCB is stored in the
> > > NvmeRequest aiocb pointer when the AIO is issued. The purpose of storing
> > > this is to allow the AIO to be cancelled when deleting submission
> > > queues (it is currently not used for Abort).
> > > 
> > > Since the addition of the Dataset Management command and Zoned
> > > Namespaces, NvmeRequests may involve more than one AIO and the AIOs are
> > > issued without saving a reference to the BlockAIOCB. This is a problem
> > > since nvme_del_sq will attempt to cancel outstanding AIOs, potentially
> > > with an invalid BlockAIOCB.
> > > 
> > > Fix this by instead of explicitly cancelling the requests, just allow
> > > the AIOs to complete by draining the namespace blockdevs.
> > > 
> > > Signed-off-by: Klaus Jensen 
> > > ---
> > >  hw/block/nvme.c | 18 +-
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 316858fd8adf..91f6fb6da1e2 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -403,6 +403,7 @@ static void nvme_req_clear(NvmeRequest *req)
> > >  {
> > >  req->ns = NULL;
> > >  req->opaque = NULL;
> > > +req->aiocb = NULL;
> > >  memset(>cqe, 0x0, sizeof(req->cqe));
> > >  req->status = NVME_SUCCESS;
> > >  }
> > > @@ -2396,6 +2397,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, 
> > > NvmeRequest *req)
> > >  NvmeSQueue *sq;
> > >  NvmeCQueue *cq;
> > >  uint16_t qid = le16_to_cpu(c->qid);
> > > +int i;
> > >  
> > >  if (unlikely(!qid || nvme_check_sqid(n, qid))) {
> > >  trace_pci_nvme_err_invalid_del_sq(qid);
> > > @@ -2404,12 +2406,18 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, 
> > > NvmeRequest *req)
> > >  
> > >  trace_pci_nvme_del_sq(qid);
> > >  
> > > -sq = n->sq[qid];
> > > -while (!QTAILQ_EMPTY(>out_req_list)) {
> > > -r = QTAILQ_FIRST(>out_req_list);
> > > -assert(r->aiocb);
> > > -blk_aio_cancel(r->aiocb);
> > > +for (i = 1; i <= n->num_namespaces; i++) {
> > > +NvmeNamespace *ns = nvme_ns(n, i);
> > > +if (!ns) {
> > > +continue;
> > > +}
> > > +
> > > +nvme_ns_drain(ns);
> > 
> > If we just drain the entire namespaces here, commands which has nothing
> > to do with the target sq to be deleted will be drained.  And this might
> > be a burden for a single SQ deletion.
> > 
> 
> That is true. But how often would you dynamically delete and create I/O
> submission queues in the fast path?

Delete I/O queues are not that often in the working NVMe controller, but
it might be a good case for the exception test from the host side like:
I/O queue deletion during I/O workloads.  If delete I/O queues are
returning by aborting their own requests only and quickly respond to the
host, then I think it might be a good one to test with.  Handling
requests gracefully sometimes don't cause corner cases from the host
point-of-view.  But, QEMU is not only for the host testing, so I am not
sure that QEMU NVMe device should handle things gracefully or try to do
something exactly as the real hardware(but, we don't know all the
hardware behavior ;)).

(But, Right. If I'm only talking about the kernel, then kernel does not
delete queues during the fast-path hot workloads.  But it's sometimes
great to test something on their own driver or application)

> > By the way, agree with the multiple AIOs references problem for newly added
> > commands.  But, shouldn't we manage the inflight AIO request references for
> > the newlly added commands with some other way and kill them all
> > explicitly as it was?  Maybe some of list for AIOCBs?
> 
> I was hesitant to add more stuff to NvmeRequest (like a QTAILQ to track
> this). Getting a steady-state with draining was an easy fix.

Graceful handling is easy to go with.  I am not expert for the overall
purpose of the QEMU NVMe device model, but I'm curious that which one we
need to take first between `Easy to go vs. What device should do`.

Re: [PATCH] hw/block/nvme: fix legacy namespace registration

[Minwoo Im]

Thanks Klaus,

Reviewed-by: Minwoo Im 

Re: [PATCH] hw/block/nvme: fix error handling in nvme_ns_realize

[Minwoo Im]

Reviewed-by: Minwoo Im 

[PATCH] hw/block/nvme: fix error handling in nvme_ns_realize

[Klaus Jensen]

From: Klaus Jensen 

nvme_ns_realize passes errp to nvme_register_namespaces, but then try to
prepend errp with local_err.

Just remove the local_err and use errp directly.

Fixes: 15d024d4aa9b ("hw/block/nvme: split setup and register for namespace")
Cc: Minwoo Im 
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme-ns.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index dfed71a950fa..93ac6e107a09 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -358,17 +358,12 @@ static void nvme_ns_realize(DeviceState *dev, Error 
**errp)
 NvmeNamespace *ns = NVME_NS(dev);
 BusState *s = qdev_get_parent_bus(dev);
 NvmeCtrl *n = NVME(s->parent);
-Error *local_err = NULL;
 
-if (nvme_ns_setup(ns, _err)) {
-error_propagate_prepend(errp, local_err,
-"could not setup namespace: ");
+if (nvme_ns_setup(ns, errp)) {
 return;
 }
 
 if (nvme_register_namespace(n, ns, errp)) {
-error_propagate_prepend(errp, local_err,
-"could not register namespace: ");
 return;
 }
 
-- 
2.30.0

Re: [PATCH 3/7] block/qcow2: use compressed write cache

[Vladimir Sementsov-Ogievskiy]

10.02.2021 20:11, Max Reitz wrote:

On 29.01.21 17:50, Vladimir Sementsov-Ogievskiy wrote:

Introduce a new option: compressed-cache-size, with default to 64
clusters (to be not less than 64 default max-workers for backup job).

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  qapi/block-core.json   |  8 +++-
  block/qcow2.h  |  4 ++
  block/qcow2-refcount.c | 13 +++
  block/qcow2.c  | 87 --
  4 files changed, 108 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9f555d5c1d..e0be6657f3 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3202,6 +3202,11 @@
  # an image, the data file name is loaded from the image
  # file. (since 4.0)
  #
+# @compressed-cache-size: The maximum size of compressed write cache in
+# bytes. If positive must be not less than
+# cluster size. 0 disables the feature. Default
+# is 64 * cluster_size. (since 6.0)


Do we need this, really?  If you don’t use compression, the cache won’t use any 
memory, right?  Do you plan on using this option?

I’d just set it to a sane default.


OK for me



OTOH, “a sane default” poses two questions, namely whether 64 * cluster_size is 
reasonable – with subclusters, the cluster size may be rather high, so 64 * 
cluster_size may well be like 128 MB.  Are 64 clusters really necessary for a 
reasonable performance?

Second, I think I could live with a rather high default if clusters are flushed 
as soon as they are full.  OTOH, as I briefly touched on, in practice, I 
suppose compressed images are just written to constantly, so even if clusters 
are flushed as soon as they are full, the cache will still remain full all the 
time.


Different topic: Why is the cache disableable?  I thought there are no 
downsides?



to compare performance for example..


--
Best regards,
Vladimir

[PULL 0/2] emulated nvme fixes

[Klaus Jensen]

From: Klaus Jensen 

Hi Peter,

Two small fixes for emulated nvme for regressions reported by Alexander
Graf and Bin Meng.

Please pull!

The following changes since commit 83339e21d05c824ebc9131d644f25c23d0e41ecf:

  Merge remote-tracking branch 
'remotes/stefanha-gitlab/tags/block-pull-request' into staging (2021-02-10 
15:42:20 +)

are available in the Git repository at:

  git://git.infradead.org/qemu-nvme.git tags/nvme-fixes-pull-request

for you to fetch changes up to b4471900d5328b66eeecdbc79de83992cc109d04:

  hw/block/nvme: Fix a build error in nvme_get_feature() (2021-02-11 13:46:50 
+0100)


Two small fixes for regressions reported by Alexander Graf and Bin Meng.



Bin Meng (1):
  hw/block/nvme: Fix a build error in nvme_get_feature()

Klaus Jensen (1):
  hw/block/nvme: fix legacy namespace registration

 hw/block/nvme.c | 7 +++
 1 file changed, 7 insertions(+)

-- 
2.30.0

Re: [PULL 1/2] hw/block/nvme: fix legacy namespace registration

[Klaus Jensen]

On Feb 11 13:52, Klaus Jensen wrote:
> From: Klaus Jensen 
> 
> Moving namespace registration to the nvme-ns realization function had
> the unintended side-effect of breaking legacy namespace registration.
> Fix this.
> 
> Fixes: 15d024d4aa9b ("hw/block/nvme: split setup and register for namespace")
> Reported-by: Alexander Graf 
> Cc: Minwoo Im 
> Tested-by: Alexander Graf 
> Signed-off-by: Klaus Jensen 
> ---
>  hw/block/nvme.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 5ce21b7100b3..d36e14fe13e2 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -4507,6 +4507,12 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
> **errp)
>  if (nvme_ns_setup(ns, errp)) {
>  return;
>  }
> +
> +if (nvme_register_namespace(n, ns, errp)) {
> +error_propagate_prepend(errp, local_err,
> +"could not register namespace: ");
> +return;
> +}
>  }
>  }
>  

Argh, sorry Peter.

I just spotted a mistake here. Please ignore until v2...


signature.asc
Description: PGP signature

[PULL 1/2] hw/block/nvme: fix legacy namespace registration

[Klaus Jensen]

From: Klaus Jensen 

Moving namespace registration to the nvme-ns realization function had
the unintended side-effect of breaking legacy namespace registration.
Fix this.

Fixes: 15d024d4aa9b ("hw/block/nvme: split setup and register for namespace")
Reported-by: Alexander Graf 
Cc: Minwoo Im 
Tested-by: Alexander Graf 
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5ce21b7100b3..d36e14fe13e2 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4507,6 +4507,12 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
**errp)
 if (nvme_ns_setup(ns, errp)) {
 return;
 }
+
+if (nvme_register_namespace(n, ns, errp)) {
+error_propagate_prepend(errp, local_err,
+"could not register namespace: ");
+return;
+}
 }
 }
 
-- 
2.30.0

[PULL 2/2] hw/block/nvme: Fix a build error in nvme_get_feature()

[Klaus Jensen]

From: Bin Meng 

Current QEMU HEAD nvme.c does not compile with the default GCC 5.4
on a Ubuntu 16.04 host:

  hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized in this 
function [-Werror=maybe-uninitialized]
 trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
 ^
  hw/block/nvme.c:3150:14: note: ‘result’ was declared here
 uint32_t result;
  ^

Explicitly initialize the result to fix it.

Fixes: aa5e55e3b07e ("hw/block/nvme: open code for volatile write cache")
Signed-off-by: Bin Meng 
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d36e14fe13e2..31295e5eeb84 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -3228,6 +3228,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeRequest 
*req)
 result = ns->features.err_rec;
 goto out;
 case NVME_VOLATILE_WRITE_CACHE:
+result = 0;
 for (i = 1; i <= n->num_namespaces; i++) {
 ns = nvme_ns(n, i);
 if (!ns) {
-- 
2.30.0

Re: [PATCH 2/7] block/qcow2: introduce cache for compressed writes

[Vladimir Sementsov-Ogievskiy]

you may jump first to my last inline answer

10.02.2021 20:07, Max Reitz wrote:

On 29.01.21 17:50, Vladimir Sementsov-Ogievskiy wrote:

Compressed writes and O_DIRECT are not friends: they works too slow,
because compressed writes does many small unaligned to 512 writes.

Let's introduce an internal cache, so that compressed writes may work
well when O_DIRECT is on.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/qcow2.h    |  29 +
  block/qcow2-compressed-write-cache.c | 770 +++
  block/meson.build    |   1 +
  3 files changed, 800 insertions(+)
  create mode 100644 block/qcow2-compressed-write-cache.c

diff --git a/block/qcow2.h b/block/qcow2.h
index 0678073b74..fbdedf89fa 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -322,6 +322,8 @@ typedef struct Qcow2BitmapHeaderExt {
  uint64_t bitmap_directory_offset;
  } QEMU_PACKED Qcow2BitmapHeaderExt;
+typedef struct Qcow2CompressedWriteCache Qcow2CompressedWriteCache;
+
  #define QCOW2_MAX_THREADS 4
  typedef struct BDRVQcow2State {
@@ -1010,4 +1012,31 @@ int coroutine_fn
  qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_offset,
   uint64_t guest_offset, void *buf, size_t len);
+Qcow2CompressedWriteCache *qcow2_compressed_cache_new(BdrvChild *data_file,
+  int64_t cluster_size,
+  int64_t cache_size);
+void qcow2_compressed_cache_free(Qcow2CompressedWriteCache *s);
+int coroutine_fn
+qcow2_compressed_cache_co_read(Qcow2CompressedWriteCache *s, int64_t offset,
+   int64_t bytes, void *buf);
+int coroutine_fn
+qcow2_compressed_cache_co_write(Qcow2CompressedWriteCache *s, int64_t offset,
+    int64_t bytes, void *buf);
+void coroutine_fn
+qcow2_compressed_cache_co_set_cluster_end(Qcow2CompressedWriteCache *s,
+  int64_t cluster_data_end);
+int coroutine_fn
+qcow2_compressed_cache_co_flush(Qcow2CompressedWriteCache *s);
+int qcow2_compressed_cache_flush(BlockDriverState *bs,
+ Qcow2CompressedWriteCache *state);
+int coroutine_fn
+qcow2_compressed_cache_co_stop_flush(Qcow2CompressedWriteCache *s);
+int qcow2_compressed_cache_stop_flush(BlockDriverState *bs,
+  Qcow2CompressedWriteCache *s);
+void qcow2_compressed_cache_set_size(Qcow2CompressedWriteCache *s,
+ int64_t size);
+void coroutine_fn
+qcow2_compressed_cache_co_discard(Qcow2CompressedWriteCache *s,
+  int64_t cluster_offset);
+


It would be nice if these functions had their interface documented somewhere.


I tried to comment dificult things in .c... Is there a prefernce, where to 
document
how and what function does, in .h or in .c ?




  #endif
diff --git a/block/qcow2-compressed-write-cache.c 
b/block/qcow2-compressed-write-cache.c
new file mode 100644
index 00..7bb92cb550
--- /dev/null
+++ b/block/qcow2-compressed-write-cache.c
@@ -0,0 +1,770 @@
+/*
+ * Write cache for qcow2 compressed writes
+ *
+ * Copyright (c) 2021 Virtuozzo International GmbH.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+
+#include "block/block_int.h"
+#include "block/block-gen.h"
+#include "qemu/coroutine.h"
+#include "qapi/qapi-events-block-core.h"
+#include "qcow2.h"
+
+typedef struct CacheExtent {
+    int64_t offset;
+    int64_t bytes;
+    void *buf;
+    QLIST_ENTRY(CacheExtent) next;
+} CacheExtent;
+
+typedef struct CacheCluster {


It isn’t immediately clear what these two structures mean by just their name, 
because “extent” has no meaning in the context of qcow2.


It's not important for the cache are extents compressed clusters or not.. So 
I'd keep more generic name



I understand CacheExtent to basically be a 

Re: [PATCH 1/2] tests/docker: remove travis container

[Alex Bennée]

Daniel P. Berrangé  writes:

> The travis container that we have no longer matches what travis
> currently uses. As all x86 jobs are being moved to GitLab CI too,
> there is no compelling reason to update the travis container. It
> is simpler to just remove it.
>
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Wainer dos Santos Moschetta 
> Signed-off-by: Daniel P. Berrangé 

Queued to testing/next, thanks.

-- 
Alex Bennée

Re: [PATCH] hw/block/nvme: drain namespaces on sq deletion

[Klaus Jensen]

On Feb 11 11:49, Minwoo Im wrote:
> On 21-01-27 14:15:05, Klaus Jensen wrote:
> > From: Klaus Jensen 
> > 
> > For most commands, when issuing an AIO, the BlockAIOCB is stored in the
> > NvmeRequest aiocb pointer when the AIO is issued. The purpose of storing
> > this is to allow the AIO to be cancelled when deleting submission
> > queues (it is currently not used for Abort).
> > 
> > Since the addition of the Dataset Management command and Zoned
> > Namespaces, NvmeRequests may involve more than one AIO and the AIOs are
> > issued without saving a reference to the BlockAIOCB. This is a problem
> > since nvme_del_sq will attempt to cancel outstanding AIOs, potentially
> > with an invalid BlockAIOCB.
> > 
> > Fix this by instead of explicitly cancelling the requests, just allow
> > the AIOs to complete by draining the namespace blockdevs.
> > 
> > Signed-off-by: Klaus Jensen 
> > ---
> >  hw/block/nvme.c | 18 +-
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 316858fd8adf..91f6fb6da1e2 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -403,6 +403,7 @@ static void nvme_req_clear(NvmeRequest *req)
> >  {
> >  req->ns = NULL;
> >  req->opaque = NULL;
> > +req->aiocb = NULL;
> >  memset(>cqe, 0x0, sizeof(req->cqe));
> >  req->status = NVME_SUCCESS;
> >  }
> > @@ -2396,6 +2397,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest 
> > *req)
> >  NvmeSQueue *sq;
> >  NvmeCQueue *cq;
> >  uint16_t qid = le16_to_cpu(c->qid);
> > +int i;
> >  
> >  if (unlikely(!qid || nvme_check_sqid(n, qid))) {
> >  trace_pci_nvme_err_invalid_del_sq(qid);
> > @@ -2404,12 +2406,18 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, 
> > NvmeRequest *req)
> >  
> >  trace_pci_nvme_del_sq(qid);
> >  
> > -sq = n->sq[qid];
> > -while (!QTAILQ_EMPTY(>out_req_list)) {
> > -r = QTAILQ_FIRST(>out_req_list);
> > -assert(r->aiocb);
> > -blk_aio_cancel(r->aiocb);
> > +for (i = 1; i <= n->num_namespaces; i++) {
> > +NvmeNamespace *ns = nvme_ns(n, i);
> > +if (!ns) {
> > +continue;
> > +}
> > +
> > +nvme_ns_drain(ns);
> 
> If we just drain the entire namespaces here, commands which has nothing
> to do with the target sq to be deleted will be drained.  And this might
> be a burden for a single SQ deletion.
> 

That is true. But how often would you dynamically delete and create I/O
submission queues in the fast path?

> By the way, agree with the multiple AIOs references problem for newly added
> commands.  But, shouldn't we manage the inflight AIO request references for
> the newlly added commands with some other way and kill them all
> explicitly as it was?  Maybe some of list for AIOCBs?

I was hesitant to add more stuff to NvmeRequest (like a QTAILQ to track
this). Getting a steady-state with draining was an easy fix.


signature.asc
Description: PGP signature

Re: [PULL 32/56] hw/block/nvme: split setup and register for namespace

[Klaus Jensen]

On Feb 11 12:40, Philippe Mathieu-Daudé wrote:
> On 2/11/21 10:53 AM, Alexander Graf wrote:
> > Hi Klaus,
> > 
> > On 09.02.21 08:30, Klaus Jensen wrote:
> >> From: Minwoo Im 
> >>
> >> In NVMe, namespace is being attached to process I/O.  We register NVMe
> >> namespace to a controller via nvme_register_namespace() during
> >> nvme_ns_setup().  This is main reason of receiving NvmeCtrl object
> >> instance to this function to map the namespace to a controller.
> >>
> >> To make namespace instance more independent, it should be split into two
> >> parts: setup and register.  This patch split them into two differnt
> >> parts, and finally nvme_ns_setup() does not have nothing to do with
> >> NvmeCtrl instance at all.
> >>
> >> This patch is a former patch to introduce NVMe subsystem scheme to the
> >> existing design especially for multi-path.  In that case, it should be
> >> split into two to make namespace independent from a controller.
> >>
> >> Signed-off-by: Minwoo Im 
> >> Signed-off-by: Klaus Jensen 
> > 
> > 
> > In latest master, I can no longer access NVMe devices from edk2. Git
> > bisect pointed me to this commit.
> > 
> > To reproduce:
> > 
> >   $ ./build/qemu-system-x86_64 -enable-kvm -pflash
> > build/pc-bios/edk2-x86_64-code.fd -drive
> > file=image.raw,if=none,id=d,snapshot=on -device nvme,serial=1234,drive=d
> > -nographic -net none
> > 
> > You will see that before this commit, edk2 is able to enumerate the
> > block device, while after this commit it does not find it.
> > 
> > 
> > good:
> > 
> > Mapping table
> >   FS0: Alias(s):HD1b:;BLK2:
> > �
> > PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(1,GPT,7A866FF6-0A12-4911-A4ED-D0565BEB41A2,0x80,0x64000)
> > 
> >  BLK0: Alias(s):
> >   PciRoot(0x0)/Pci(0x1,0x1)/Ata(0x0)
> >  BLK1: Alias(s):
> > � PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)
> >  BLK3: Alias(s):
> > �
> > PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(2,GPT,F070566B-C58D-4F13-9B92-64F1794385E7,0x64080,0x4)
> > 
> >  BLK4: Alias(s):
> > �
> > PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(3,GPT,EDE86BE3-C64F-4ACB-B4AA-5E6C0135D335,0xA4080,0x315BF1B)
> 
> Good integration test for the emulated NVMe subsystem!
> 
> tests/acceptance/boot_linux_console.py should provide trivial template.
> 

Yes. This is something we really need. I will priorities this together
with documentation :)

Thanks for pointing me in the direction of this Philippe.


signature.asc
Description: PGP signature

Re: [PULL 32/56] hw/block/nvme: split setup and register for namespace

[Philippe Mathieu-Daudé]

On 2/11/21 10:53 AM, Alexander Graf wrote:
> Hi Klaus,
> 
> On 09.02.21 08:30, Klaus Jensen wrote:
>> From: Minwoo Im 
>>
>> In NVMe, namespace is being attached to process I/O.  We register NVMe
>> namespace to a controller via nvme_register_namespace() during
>> nvme_ns_setup().  This is main reason of receiving NvmeCtrl object
>> instance to this function to map the namespace to a controller.
>>
>> To make namespace instance more independent, it should be split into two
>> parts: setup and register.  This patch split them into two differnt
>> parts, and finally nvme_ns_setup() does not have nothing to do with
>> NvmeCtrl instance at all.
>>
>> This patch is a former patch to introduce NVMe subsystem scheme to the
>> existing design especially for multi-path.  In that case, it should be
>> split into two to make namespace independent from a controller.
>>
>> Signed-off-by: Minwoo Im 
>> Signed-off-by: Klaus Jensen 
> 
> 
> In latest master, I can no longer access NVMe devices from edk2. Git
> bisect pointed me to this commit.
> 
> To reproduce:
> 
>   $ ./build/qemu-system-x86_64 -enable-kvm -pflash
> build/pc-bios/edk2-x86_64-code.fd -drive
> file=image.raw,if=none,id=d,snapshot=on -device nvme,serial=1234,drive=d
> -nographic -net none
> 
> You will see that before this commit, edk2 is able to enumerate the
> block device, while after this commit it does not find it.
> 
> 
> good:
> 
> Mapping table
>   FS0: Alias(s):HD1b:;BLK2:
> �
> PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(1,GPT,7A866FF6-0A12-4911-A4ED-D0565BEB41A2,0x80,0x64000)
> 
>  BLK0: Alias(s):
>   PciRoot(0x0)/Pci(0x1,0x1)/Ata(0x0)
>  BLK1: Alias(s):
> � PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)
>  BLK3: Alias(s):
> �
> PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(2,GPT,F070566B-C58D-4F13-9B92-64F1794385E7,0x64080,0x4)
> 
>  BLK4: Alias(s):
> �
> PciRoot(0x0)/Pci(0x3,0x0)/NVMe(0x1,00-00-00-00-00-00-00-00)/HD(3,GPT,EDE86BE3-C64F-4ACB-B4AA-5E6C0135D335,0xA4080,0x315BF1B)

Good integration test for the emulated NVMe subsystem!

tests/acceptance/boot_linux_console.py should provide trivial template.

> 
> 
> 
> bad:
> 
> Mapping table
>  BLK0: Alias(s):
>   PciRoot(0x0)/Pci(0x1,0x1)/Ata(0x0)
> 
> 
> 
> Thanks,
> 
> Alex
> 
> 
> 

[PATCH] hw/block/nvme: fix legacy namespace registration

[Klaus Jensen]

From: Klaus Jensen 

Moving namespace registration to the nvme-ns realization function had
the unintended side-effect of breaking legacy namespace registration.
Fix this.

Fixes: 15d024d4aa9b ("hw/block/nvme: split setup and register for namespace")
Reported-by: Alexander Graf 
Cc: Minwoo Im 
Signed-off-by: Klaus Jensen 
---
 hw/block/nvme.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5ce21b7100b3..d36e14fe13e2 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4507,6 +4507,12 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
**errp)
 if (nvme_ns_setup(ns, errp)) {
 return;
 }
+
+if (nvme_register_namespace(n, ns, errp)) {
+error_propagate_prepend(errp, local_err,
+"could not register namespace: ");
+return;
+}
 }
 }
 
-- 
2.30.0

Re: [PULL 32/56] hw/block/nvme: split setup and register for namespace

[Klaus Jensen]

On Feb 11 10:53, Alexander Graf wrote:
> Hi Klaus,
> 
> On 09.02.21 08:30, Klaus Jensen wrote:
> > From: Minwoo Im 
> > 
> > In NVMe, namespace is being attached to process I/O.  We register NVMe
> > namespace to a controller via nvme_register_namespace() during
> > nvme_ns_setup().  This is main reason of receiving NvmeCtrl object
> > instance to this function to map the namespace to a controller.
> > 
> > To make namespace instance more independent, it should be split into two
> > parts: setup and register.  This patch split them into two differnt
> > parts, and finally nvme_ns_setup() does not have nothing to do with
> > NvmeCtrl instance at all.
> > 
> > This patch is a former patch to introduce NVMe subsystem scheme to the
> > existing design especially for multi-path.  In that case, it should be
> > split into two to make namespace independent from a controller.
> > 
> > Signed-off-by: Minwoo Im 
> > Signed-off-by: Klaus Jensen 
> 
> 
> In latest master, I can no longer access NVMe devices from edk2. Git bisect
> pointed me to this commit.
> 

Hi Alexander,

Thanks for reporting this. This patch should fix it, I'll queue it up.

diff --git i/hw/block/nvme.c w/hw/block/nvme.c
index 5ce21b7100b3..d36e14fe13e2 100644
--- i/hw/block/nvme.c
+++ w/hw/block/nvme.c
@@ -4507,6 +4507,12 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
**errp)
 if (nvme_ns_setup(ns, errp)) {
 return;
 }
+
+if (nvme_register_namespace(n, ns, errp)) {
+error_propagate_prepend(errp, local_err,
+"could not register namespace: ");
+return;
+}
 }
 }



signature.asc
Description: PGP signature

Re: [RFC PATCH 3/3] hw/block/nvme: add nvme_inject_state HMP command

[Dr. David Alan Gilbert]

* Klaus Jensen (i...@irrelevant.dk) wrote:
> On Feb 11 04:52, Minwoo Im wrote:
> > nvme_inject_state command is to give a controller state to be.
> > Human Monitor Interface(HMP) supports users to make controller to a
> > specified state of:
> > 
> > normal: Normal state (no injection)
> > cmd-interrupted:Commands will be interrupted internally
> > 
> > This patch is just a start to give dynamic command from the HMP to the
> > QEMU NVMe device model.  If "cmd-interrupted" state is given, then the
> > controller will return all the CQ entries with Command Interrupts status
> > code.
> > 
> > Usage:
> > -device nvme,id=nvme0,
> > 
> > (qemu) nvme_inject_state nvme0 cmd-interrupted
> > 
> > 
> > 
> > (qemu) nvme_inject_state nvme0 normal
> > 
> > This feature is required to test Linux kernel NVMe driver for the
> > command retry feature.
> > 
> 
> This is super cool and commands like this feel much nicer than the
> qom-set approach that the SMART critical warning feature took.
> 
> But... looking at the existing commands I don't think we can "bloat" it
> up with a device specific command like this, but I don't know the policy
> around this.
> 
> If an HMP command is out, then we should be able to make do with the
> qom-set approach just fine though.

HMP is there to make life easy for Humans debugging; if it makes sense from an
NVMe perspective for test/debug then I'm OK with it from an HMP side.
Note that if it was for more common use than debug/test then you'd want
to make it go via QMP and make sure it was a stable interface that was
going to live for a longtime; but for test uses HMP is fine.

Dave

> > Signed-off-by: Minwoo Im 
> > ---
> >  hmp-commands.hx   | 13 
> >  hw/block/nvme.c   | 49 +++
> >  hw/block/nvme.h   |  8 +++
> >  include/monitor/hmp.h |  1 +
> >  4 files changed, 71 insertions(+)
> > 
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index d4001f9c5dc6..ef288c567b46 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -1307,6 +1307,19 @@ SRST
> >Inject PCIe AER error
> >  ERST
> >  
> > +{
> > +.name   = "nvme_inject_state",
> > +.args_type  = "id:s,state:s",
> > +.params = "id [normal|cmd-interrupted]",
> > +.help   = "inject controller/namespace state",
> > +.cmd= hmp_nvme_inject_state,
> > +},
> > +
> > +SRST
> > +``nvme_inject_state``
> > +  Inject NVMe controller/namespace state
> > +ERST
> > +
> >  {
> >  .name   = "netdev_add",
> >  .args_type  = "netdev:O",
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 6d3c554a0e99..42cf5bd113e6 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -123,6 +123,7 @@
> >  #include "sysemu/sysemu.h"
> >  #include "qapi/error.h"
> >  #include "qapi/visitor.h"
> > +#include "qapi/qmp/qdict.h"
> >  #include "sysemu/hostmem.h"
> >  #include "sysemu/block-backend.h"
> >  #include "exec/memory.h"
> > @@ -132,6 +133,7 @@
> >  #include "trace.h"
> >  #include "nvme.h"
> >  #include "nvme-ns.h"
> > +#include "monitor/monitor.h"
> >  
> >  #define NVME_MAX_IOQPAIRS 0x
> >  #define NVME_DB_SIZE  4
> > @@ -966,6 +968,14 @@ static void nvme_enqueue_req_completion(NvmeCQueue 
> > *cq, NvmeRequest *req)
> >  {
> >  assert(cq->cqid == req->sq->cqid);
> >  
> > +/*
> > + * Override request status field if controller state has been injected 
> > by
> > + * the QMP.
> > + */
> > +if (cq->ctrl->state == NVME_STATE_CMD_INTERRUPTED) {
> > +req->status = NVME_COMMAND_INTERRUPTED;
> > +}
> > +
> >  if (req->status != NVME_SUCCESS) {
> >  if (cq->ctrl->features.acre && nvme_should_retry(req)) {
> >  if (cq->ctrl->params.cmd_retry_delay > 0) {
> > @@ -5025,4 +5035,43 @@ static void nvme_register_types(void)
> >  type_register_static(_bus_info);
> >  }
> >  
> > +static void nvme_inject_state(NvmeCtrl *n, NvmeState state)
> > +{
> > +n->state = state;
> > +}
> > +
> > +static const char *nvme_states[] = {
> > +[NVME_STATE_NORMAL] = "normal",
> > +[NVME_STATE_CMD_INTERRUPTED]= "cmd-interrupted",
> > +};
> > +
> > +void hmp_nvme_inject_state(Monitor *mon, const QDict *qdict)
> > +{
> > +const char *id = qdict_get_str(qdict, "id");
> > +const char *state = qdict_get_str(qdict, "state");
> > +PCIDevice *dev;
> > +NvmeCtrl *n;
> > +int ret, i;
> > +
> > +ret = pci_qdev_find_device(id, );
> > +if (ret < 0) {
> > +monitor_printf(mon, "invalid device id %s\n", id);
> > +return;
> > +}
> > +
> > +n = NVME(dev);
> > +
> > +for (i = 0; i < ARRAY_SIZE(nvme_states); i++) {
> > +if (!strcmp(nvme_states[i], state)) {
> > +nvme_inject_state(n, i);
> > +monitor_printf(mon,
> > +   "-device nvme,id=%s: state %s injected\n",
> > +