Re: [PATCH 3/4] tests/qtest: Only run fuzz-megasas-test if megasas device is available

[Alexander Bulekov]

On 210115 1609, Philippe Mathieu-Daudé wrote:
> This test fails when QEMU is built without the megasas device,
> restrict it to its availability.

Should we just make a separate directory for fuzzer tests and have a
separate source file for each reproducer (or for each device)? That way,
we avoid confusion about what to do with new reproducers: they always go
into e.g. tests/qtest/reproducers/device_name.c 

Re: [PATCH 2/4] tests/qtest: Make fuzz-test generic to all targets

[Thomas Huth]

On 15/01/2021 16.09, Philippe Mathieu-Daudé wrote:

Tests in fuzz-test's main() already check for the supported
architecture before adding tests, therefore this test is not
specific to the X86 target. Move it to the generic set.


As long as it does not run any test on non-x86, it does not make sense to 
move it to the generic set, does it? We'd only waste compile cycles that way?


 Thomas

Re: [PATCH 1/4] tests/qtest: Remove TPM tests

[Stefan Berger]

On 1/15/21 1:40 PM, Stefan Berger wrote:

On 1/15/21 11:06 AM, Philippe Mathieu-Daudé wrote:

On 1/15/21 4:53 PM, Stefan Berger wrote:

On 1/15/21 10:52 AM, Philippe Mathieu-Daudé wrote:

Subject is incorrect, this is not a removal of the tests, but
removal of their execution. The tests are still in the repository.
This is more of a disablement.

How do you compile / run them to have the LeakSanitizer checks?

I used:

../configure --cc=clang --enable-sanitizers && make check-qtest

$ clang -v
clang version 10.0.1 (Fedora 10.0.1-3.fc32)

This was previously covered by patchew CI. I just figured
patchew is running without the LeakSanitizer since commit
6f89ec7442e ("docker: test-debug: disable LeakSanitizer"):

  docker: test-debug: disable LeakSanitizer

  There are just too many leaks in device-introspect-test (especially 
for

  the plethora of arm and aarch64 boards) to make LeakSanitizer useful;
  disable it for now.


I only get short stack traces:


Indirect leak of 852840 byte(s) in 207 object(s) allocated from:
    #0 0x561a8c2f8b57 in calloc 
(/home/stefanb/tmp/qemu-tip/build/tests/qtest/tpm-crb-swtpm-test+0x23fb57)

    #1 0x14f0963069b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0)
    #2 0x561a8c4c2508 in json_parser_parse 
/home/stefanb/tmp/qemu-tip/build/../qobject/json-parser.c:580:14
    #3 0x561a8c4a99aa in json_message_process_token 
/home/stefanb/tmp/qemu-tip/build/../qobject/json-streamer.c:92:12
    #4 0x561a8c4b6cfb in json_lexer_feed_char 
/home/stefanb/tmp/qemu-tip/build/../qobject/json-lexer.c:313:13


Indirect leak of 6624 byte(s) in 207 object(s) allocated from:
    #0 0x561a8c2f8b57 in calloc 
(/home/stefanb/tmp/qemu-tip/build/tests/qtest/tpm-crb-swtpm-test+0x23fb57)

    #1 0x14f0963069b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0)

Indirect leak of 1449 byte(s) in 207 object(s) allocated from:
    #0 0x561a8c2f899f in malloc 
(/home/stefanb/tmp/qemu-tip/build/tests/qtest/tpm-crb-swtpm-test+0x23f99f)

    #1 0x14f096306958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)

How can I see more of those?



I now added -fno-omit-frame-pointer to configure (should it not be 
there?) and it now shows some useful stacktraces.



diff --git a/configure b/configure
index 155dda124c..ed86b5ca32 100755
--- a/configure
+++ b/configure
@@ -5308,7 +5308,7 @@ if test "$gprof" = "yes" ; then
 fi

 if test "$have_asan" = "yes"; then
-  QEMU_CFLAGS="-fsanitize=address $QEMU_CFLAGS"
+  QEMU_CFLAGS="-fsanitize=address -fno-omit-frame-pointer $QEMU_CFLAGS"
   QEMU_LDFLAGS="-fsanitize=address $QEMU_LDFLAGS"
   if test "$have_asan_iface_h" = "no" ; then
   echo "ASAN build enabled, but ASAN header missing." \
diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c


This is my TPM related fix. Maybe it resolve the issue for you also?


index 5a33a6ef0f..b70cc32d60 100644
--- a/tests/qtest/tpm-util.c
+++ b/tests/qtest/tpm-util.c
@@ -250,7 +250,7 @@ void tpm_util_wait_for_migration_complete(QTestState 
*who)

 status = qdict_get_str(rsp_return, "status");
 completed = strcmp(status, "completed") == 0;
 g_assert_cmpstr(status, !=,  "failed");
-    qobject_unref(rsp_return);
+    qobject_unref(rsp);
 if (completed) {
 return;
 }

Now I see ppc64 related leaks:

Direct leak of 200 byte(s) in 1 object(s) allocated from:
    #0 0x14c9b743c837 in __interceptor_calloc (/lib64/libasan.so.6+0xb0837)
    #1 0x14c9b6e8b9b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0)
    #2 0x55c5e7130a1a in qemu_init_vcpu ../softmmu/cpus.c:618
    #3 0x55c5e68b30c0 in ppc_cpu_realize 
../target/ppc/translate_init.c.inc:10146

    #4 0x55c5e7539c08 in device_set_realized ../hw/core/qdev.c:761
    #5 0x55c5e714aa38 in property_set_bool ../qom/object.c:2255
    #6 0x55c5e7145d52 in object_property_set ../qom/object.c:1400
    #7 0x55c5e714f99f in object_property_set_qobject 
../qom/qom-qobject.c:28

    #8 0x55c5e71465f4 in object_property_set_bool ../qom/object.c:1470
    #9 0x55c5e666ae21 in spapr_realize_vcpu ../hw/ppc/spapr_cpu_core.c:254
    #10 0x55c5e666ae21 in spapr_cpu_core_realize 
../hw/ppc/spapr_cpu_core.c:337

    #11 0x55c5e7539c08 in device_set_realized ../hw/core/qdev.c:761
    #12 0x55c5e714aa38 in property_set_bool ../qom/object.c:2255
    #13 0x55c5e7145d52 in object_property_set ../qom/object.c:1400
    #14 0x55c5e714f99f in object_property_set_qobject 
../qom/qom-qobject.c:28

    #15 0x55c5e71465f4 in object_property_set_bool ../qom/object.c:1470
    #16 0x55c5e5c7553c in qdev_device_add ../softmmu/qdev-monitor.c:665
    #17 0x55c5e6fd4cc4 in device_init_func ../softmmu/vl.c:1201
    #18 0x55c5e78fc7bb in qemu_opts_foreach ../util/qemu-option.c:1147
    #19 0x55c5e6fc8912 in qemu_create_cli_devices ../softmmu/vl.c:2488
    #20 0x55c5e6fc8912 in qmp_x_exit_preconfig ../softmmu/vl.c:2527
    #21 0x55c5e6fcfb4b in qemu_init ../softmmu/vl.c:3533
    #22 0x55c5e5b18e78 in main ../softmmu/main.c:49
    #23 0x14c9b50fa041 in __libc_start_main 

Re: [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach

[Willian Rampazzo]

On Fri, Jan 15, 2021 at 2:43 PM Max Reitz  wrote:
>
> Instead of checking iotests.py only, check all Python files in the
> qemu-iotests/ directory.  Of course, most of them do not pass, so there
> is an extensive skip list for now.  (The only files that do pass are
> 209, 254, 283, and iotests.py.)
>
> (Alternatively, we could have the opposite, i.e. an explicit list of
> files that we do want to check, but I think it is better to check files
> by default.)
>
> Unless started in debug mode (./check -d), the output has no information
> on which files are tested, so we will not have a problem e.g. with
> backports, where some files may be missing when compared to upstream.
>
> Besides the technical rewrite, some more things are changed:
>
> - For the pylint invocation, PYTHONPATH is adjusted.  This mirrors
>   setting MYPYPATH for mypy.
>
> - Also, MYPYPATH is now derived from PYTHONPATH, so that we include
>   paths set by the environment.  Maybe at some point we want to let the
>   check script add '../../python/' to PYTHONPATH so that iotests.py does
>   not need to do that.
>
> - Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
>   comments.  TODO is fine, we do not need 297 to complain about such
>   comments.
>
> - The "Success" line from mypy's output is suppressed, because (A) it
>   does not add useful information, and (B) it would leak information
>   about the files having been tested to the reference output, which we
>   decidedly do not want.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy 
> Signed-off-by: Max Reitz 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/297 | 110 +
>  tests/qemu-iotests/297.out |   5 +-
>  2 files changed, 90 insertions(+), 25 deletions(-)
>
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 5c5420712b..fa9e2cac78 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -1,4 +1,4 @@
> -#!/usr/bin/env bash
> +#!/usr/bin/env python3
>  #
>  # Copyright (C) 2020 Red Hat, Inc.
>  #
> @@ -15,30 +15,96 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see .
>
> -seq=$(basename $0)
> -echo "QA output created by $seq"
> +import os
> +import re
> +import shutil
> +import subprocess
> +import sys
>
> -status=1   # failure is the default!
> +import iotests
>
> -# get standard environment
> -. ./common.rc
>
> -if ! type -p "pylint-3" > /dev/null; then
> -_notrun "pylint-3 not found"
> -fi
> -if ! type -p "mypy" > /dev/null; then
> -_notrun "mypy not found"
> -fi
> +# TODO: Empty this list!
> +SKIP_FILES = (
> +'030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
> +'096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
> +'151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
> +'203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
> +'218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
> +'240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
> +'262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
> +'299', '300', '302', '303', '304', '307',
> +'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
> +)
>
> -pylint-3 --score=n iotests.py
>
> -MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any 
> \
> ---disallow-any-generics --disallow-incomplete-defs \
> ---disallow-untyped-decorators --no-implicit-optional \
> ---warn-redundant-casts --warn-unused-ignores \
> ---no-implicit-reexport iotests.py
> +def is_python_file(filename):
> +if not os.path.isfile(filename):
> +return False
>
> -# success, all done
> -echo "*** done"
> -rm -f $seq.full
> -status=0
> +if filename.endswith('.py'):
> +return True
> +
> +with open(filename) as f:
> +try:
> +first_line = f.readline()
> +return re.match('^#!.*python', first_line) is not None
> +except UnicodeDecodeError:  # Ignore binary files
> +return False
> +
> +
> +def run_linters():
> +files = [filename for filename in (set(os.listdir('.')) - 
> set(SKIP_FILES))
> + if is_python_file(filename)]
> +
> +iotests.logger.debug('Files to be checked:')
> +iotests.logger.debug(', '.join(sorted(files)))
> +
> +print('=== pylint ===')
> +sys.stdout.flush()
> +
> +# Todo notes are fine, but fixme's or xxx's should probably just be
> +# fixed (in tests, at least)
> +env = os.environ.copy()
> +try:
> +env['PYTHONPATH'] += ':../../python/'

Do you have any objection to using os.path.dirname and os.path.join
here? This would make the code more pythonic.

> +except KeyError:
> +env['PYTHONPATH'] = '../../python/'

Same here. You could do it once, before the 'try' and use it inside.

Other than 

Re: [PATCH v4 01/10] iotests.py: Assume a couple of variables as given

[Willian Rampazzo]

On Fri, Jan 15, 2021 at 2:43 PM Max Reitz  wrote:
>
> There are a couple of environment variables that we fetch with
> os.environ.get() without supplying a default.  Clearly they are required
> and expected to be set by the ./check script (as evidenced by
> execute_setup_common(), which checks for test_dir and
> qemu_default_machine to be set, and aborts if they are not).
>
> Using .get() this way has the disadvantage of returning an Optional[str]
> type, which mypy will complain about when tests just assume these values
> to be str.
>
> Use [] instead, which raises a KeyError for environment variables that
> are not set.  When this exception is raised, catch it and move the abort
> code from execute_setup_common() there.
>
> Drop the 'assert iotests.sock_dir is not None' from iotest 300, because
> that sort of thing is precisely what this patch wants to prevent.
>
> Signed-off-by: Max Reitz 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/300|  1 -
>  tests/qemu-iotests/iotests.py | 26 +-
>  2 files changed, 13 insertions(+), 14 deletions(-)
>

Reviewed-by: Willian Rampazzo 

Re: [PATCH 1/4] tests/qtest: Remove TPM tests

[Stefan Berger]

On 1/15/21 11:06 AM, Philippe Mathieu-Daudé wrote:

On 1/15/21 4:53 PM, Stefan Berger wrote:

On 1/15/21 10:52 AM, Philippe Mathieu-Daudé wrote:

Subject is incorrect, this is not a removal of the tests, but
removal of their execution. The tests are still in the repository.
This is more of a disablement.

How do you compile / run them to have the LeakSanitizer checks?

I used:

../configure --cc=clang --enable-sanitizers && make check-qtest

$ clang -v
clang version 10.0.1 (Fedora 10.0.1-3.fc32)

This was previously covered by patchew CI. I just figured
patchew is running without the LeakSanitizer since commit
6f89ec7442e ("docker: test-debug: disable LeakSanitizer"):

  docker: test-debug: disable LeakSanitizer

  There are just too many leaks in device-introspect-test (especially for
  the plethora of arm and aarch64 boards) to make LeakSanitizer useful;
  disable it for now.


I only get short stack traces:


Indirect leak of 852840 byte(s) in 207 object(s) allocated from:
    #0 0x561a8c2f8b57 in calloc 
(/home/stefanb/tmp/qemu-tip/build/tests/qtest/tpm-crb-swtpm-test+0x23fb57)

    #1 0x14f0963069b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0)
    #2 0x561a8c4c2508 in json_parser_parse 
/home/stefanb/tmp/qemu-tip/build/../qobject/json-parser.c:580:14
    #3 0x561a8c4a99aa in json_message_process_token 
/home/stefanb/tmp/qemu-tip/build/../qobject/json-streamer.c:92:12
    #4 0x561a8c4b6cfb in json_lexer_feed_char 
/home/stefanb/tmp/qemu-tip/build/../qobject/json-lexer.c:313:13


Indirect leak of 6624 byte(s) in 207 object(s) allocated from:
    #0 0x561a8c2f8b57 in calloc 
(/home/stefanb/tmp/qemu-tip/build/tests/qtest/tpm-crb-swtpm-test+0x23fb57)

    #1 0x14f0963069b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0)

Indirect leak of 1449 byte(s) in 207 object(s) allocated from:
    #0 0x561a8c2f899f in malloc 
(/home/stefanb/tmp/qemu-tip/build/tests/qtest/tpm-crb-swtpm-test+0x23f99f)

    #1 0x14f096306958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)

How can I see more of those?


   Stefan

Re: [RFC PATCH 0/5] hw/block/nvme: support multi-path for ctrl/ns

[Minwoo Im]

On 21-01-15 18:47:20, Klaus Jensen wrote:
> On Jan 15 09:35, Keith Busch wrote:
> > On Fri, Jan 15, 2021 at 02:57:45PM +0100, Klaus Jensen wrote:
> > > 
> > > As you already mentioned, the problem I see with this approach is that
> > > we have separate namespaces attached to separate controllers. So we are
> > > faking it to the max and if I/O starts going through the other
> > > controller we end up on a namespace that is unrelated (different data).
> > > Havoc ensues.
> > > 
> > > My approach looks a lot like yours, but I hacked around this by adding
> > > extra 'ctrl-0', 'ctrl-1', ..., link-parameters to the namespace device,
> > > replacing the bus. This works well because the namespace then just
> > > registers with multiple controllers. But adding more parameters like
> > > that just isnt nice, so I've been trying to figure out how to allow a
> > > parameter to be specified multiple times, so we could just do more
> > > 'ctrl'-parameters.
> > > 
> > > Alas, since I started thinking about namespace sharing I have been
> > > regretting that I didn't reverse the bus-mechanic for namespace
> > > attachment. It would align better with the theory of operation if it was
> > > the controllers that attached to namespaces, and not the other way
> > > around. So what I would actually really prefer, is that we had multiple
> > > 'ns' link parameters on the controller device.
> > 
> > Would this work better if we introduce a new device in the nvme hierarchy:
> > the nvme-subsystem? You could attach multi-path namespaces and
> > controllers to that, and namespaces you don't want shared can attach
> > directly to controllers like they do today. You could also auto-assign
> > cntlid, and you wouldn't need to duplicate serial numbers in your
> > parameters.
> 
> I kinda POC'ed that, but I think I tried to make it work with a bus and
> walking it and all kinds of fancy stuff.
> 
> I think it can just be a 'link' parameter, so something like:
> 
>   -device nvme-subsys,id=subsys0

Do we have any plan for default subsys hierarchy?  Or is it going to be
a mandatory root node of nvme controllers and namespaces?

>   -device nvme,id=nvme0,subsys=subsys0
>   -device nvme,id=nvme1,subsys=subsys0
>   -device nvme-ns,id=shared-ns1,nsid=1,subsys=subsys0

In this case, what is the default set-up for shared-ns1?  Is this
namespace going to be ready right after the two nvme controllers being
realized?  If so, do we iterate all the namespace devices in the NVM
subsystem and attach them to this controller in the initial time?

If so, I agree with this approach.

>   -device nvme-ns,id=private-ns2,nsid=2,bus=nvme0

This must be the case what Keith mentioned of directly attaching to a
controller.  It looks nice.  But, one concerning point here is that, in
!shared namespace, if we don't specify 'subsys' property here to attach
it to directly to a controller, it means it implicitly will belong to
the subsys0 where the nvme0 belongs to.  It means that user should give
nsid different than 1 which is already shared.

So, how do we make subsys property as a mandatory for namespace device
and provide optional choice for bus.  If bus is given to a controller,
then it can mean a private namespace, otherwise it can be shared among
controllers in a subsystem.

Re: [PATCH v4 03/10] iotests: Move try_remove to iotests.py

[Willian Rampazzo]

On Fri, Jan 15, 2021 at 2:43 PM Max Reitz  wrote:
>
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/124|  8 +---
>  tests/qemu-iotests/iotests.py | 11 +++
>  2 files changed, 8 insertions(+), 11 deletions(-)
>

Reviewed-by: Willian Rampazzo 

Re: [PATCH v4 05/10] iotests/129: Do not check @busy

[Willian Rampazzo]

On Fri, Jan 15, 2021 at 2:43 PM Max Reitz  wrote:
>
> @busy is false when the job is paused, which happens all the time
> because that is how jobs yield (e.g. for mirror at least since commit
> 565ac01f8d3).
>
> Back when 129 was added (2015), perhaps there was no better way of
> checking whether the job was still actually running.  Now we have the
> @status field (as of 58b295ba52c, i.e. 2018), which can give us exactly
> that information.
>
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/129 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Willian Rampazzo 

Re: [PATCH v4 06/10] iotests/129: Use throttle node

[Willian Rampazzo]

On Fri, Jan 15, 2021 at 2:43 PM Max Reitz  wrote:
>
> Throttling on the BB has not affected block jobs in a while, so it is
> possible that one of the jobs in 129 finishes before the VM is stopped.
> We can fix that by running the job from a throttle node.
>
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/129 | 37 +
>  1 file changed, 13 insertions(+), 24 deletions(-)

Reviewed-by: Willian Rampazzo 

Re: [PATCH v4 07/10] iotests/129: Actually test a commit job

[Willian Rampazzo]

On Fri, Jan 15, 2021 at 2:43 PM Max Reitz  wrote:
>
> Before this patch, test_block_commit() performs an active commit, which
> under the hood is a mirror job.  If we want to test various different
> block jobs, we should perhaps run an actual commit job instead.
>
> Doing so requires adding an overlay above the source node before the
> commit is done (and then specifying the source node as the top node for
> the commit job).
>
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/129 | 27 +--
>  1 file changed, 25 insertions(+), 2 deletions(-)

Reviewed-by: Willian Rampazzo 

Re: [PATCH v4 10/10] iotests/300: Clean up pylint and mypy complaints

[Willian Rampazzo]

On Fri, Jan 15, 2021 at 2:43 PM Max Reitz  wrote:
>
> And consequentially drop it from 297's skip list.
>
> Signed-off-by: Max Reitz 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/297 |  2 +-
>  tests/qemu-iotests/300 | 18 +++---
>  2 files changed, 16 insertions(+), 4 deletions(-)
>

Reviewed-by: Willian Rampazzo 

Re: [RFC PATCH 0/5] hw/block/nvme: support multi-path for ctrl/ns

[Keith Busch]

On Fri, Jan 15, 2021 at 06:47:20PM +0100, Klaus Jensen wrote:
> Cool!

I thought so too :)
 
> Question: NSIDs must be the same on each controller for shared
> namespaces, but can private namespaces "share" nsid across controllers
> in the subsystem?  I don't think the spec is clear on that point.

The namespace NSID has to be unique within the entire subsystem, whether
they're shared or private.

Re: [RFC PATCH 0/5] hw/block/nvme: support multi-path for ctrl/ns

[Klaus Jensen]

On Jan 15 09:35, Keith Busch wrote:
> On Fri, Jan 15, 2021 at 02:57:45PM +0100, Klaus Jensen wrote:
> > 
> > As you already mentioned, the problem I see with this approach is that
> > we have separate namespaces attached to separate controllers. So we are
> > faking it to the max and if I/O starts going through the other
> > controller we end up on a namespace that is unrelated (different data).
> > Havoc ensues.
> > 
> > My approach looks a lot like yours, but I hacked around this by adding
> > extra 'ctrl-0', 'ctrl-1', ..., link-parameters to the namespace device,
> > replacing the bus. This works well because the namespace then just
> > registers with multiple controllers. But adding more parameters like
> > that just isnt nice, so I've been trying to figure out how to allow a
> > parameter to be specified multiple times, so we could just do more
> > 'ctrl'-parameters.
> > 
> > Alas, since I started thinking about namespace sharing I have been
> > regretting that I didn't reverse the bus-mechanic for namespace
> > attachment. It would align better with the theory of operation if it was
> > the controllers that attached to namespaces, and not the other way
> > around. So what I would actually really prefer, is that we had multiple
> > 'ns' link parameters on the controller device.
> 
> Would this work better if we introduce a new device in the nvme hierarchy:
> the nvme-subsystem? You could attach multi-path namespaces and
> controllers to that, and namespaces you don't want shared can attach
> directly to controllers like they do today. You could also auto-assign
> cntlid, and you wouldn't need to duplicate serial numbers in your
> parameters.

I kinda POC'ed that, but I think I tried to make it work with a bus and
walking it and all kinds of fancy stuff.

I think it can just be a 'link' parameter, so something like:

  -device nvme-subsys,id=subsys0
  -device nvme,id=nvme0,subsys=subsys0
  -device nvme,id=nvme1,subsys=subsys0
  -device nvme-ns,id=shared-ns1,nsid=1,subsys=subsys0
  -device nvme-ns,id=private-ns2,nsid=2,bus=nvme0

When a controller "registers" with the subsystem it attaches to all
namespaces known, and when a namespace attaches, it attaches to all
controllers known. We can even add a 'detached' bool parameter to the
namespace and keep controllers from attaching, but allowing for later
attachment.

Cool!

Question: NSIDs must be the same on each controller for shared
namespaces, but can private namespaces "share" nsid across controllers
in the subsystem?  I don't think the spec is clear on that point.


signature.asc
Description: PGP signature

[PATCH v4 10/10] iotests/300: Clean up pylint and mypy complaints

[Max Reitz]

And consequentially drop it from 297's skip list.

Signed-off-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/297 |  2 +-
 tests/qemu-iotests/300 | 18 +++---
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index d2d9292839..ce7f85cfe0 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -33,7 +33,7 @@ SKIP_FILES = (
 '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
 '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
 '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
-'299', '300', '302', '303', '304', '307',
+'299', '302', '303', '304', '307',
 'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
 )
 
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index b864a21d5e..4115f90578 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -22,7 +22,11 @@ import os
 import random
 import re
 from typing import Dict, List, Optional, Union
+
 import iotests
+
+# Import qemu after iotests.py has amended sys.path
+# pylint: disable=wrong-import-order
 import qemu
 
 BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]
@@ -110,10 +114,14 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 If @msg is None, check that there has not been any error.
 """
 self.vm_b.shutdown()
+
+log = self.vm_b.get_log()
+assert log is not None  # Loaded after shutdown
+
 if msg is None:
-self.assertNotIn('qemu-system-', self.vm_b.get_log())
+self.assertNotIn('qemu-system-', log)
 else:
-self.assertIn(msg, self.vm_b.get_log())
+self.assertIn(msg, log)
 
 @staticmethod
 def mapping(node_name: str, node_alias: str,
@@ -445,9 +453,13 @@ class 
TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
 
 # Check for the error in the source's log
 self.vm_a.shutdown()
+
+log = self.vm_a.get_log()
+assert log is not None  # Loaded after shutdown
+
 self.assertIn(f"Cannot migrate bitmap '{name}' on node "
   f"'{self.src_node_name}': Name is longer than 255 bytes",
-  self.vm_a.get_log())
+  log)
 
 # Expect abnormal shutdown of the destination VM because of
 # the failed migration
-- 
2.29.2

[PATCH v4 08/10] iotests/129: Limit mirror job's buffer size

[Max Reitz]

Issuing 'stop' on the VM drains all nodes.  If the mirror job has many
large requests in flight, this may lead to significant I/O that looks a
bit like 'stop' would make the job try to complete (which is what 129
should verify not to happen).

We can limit the I/O in flight by limiting the buffer size, so mirror
will make very little progress during the 'stop' drain.

(We do not need to do anything about commit, which has a buffer size of
512 kB by default; or backup, which goes cluster by cluster.  Once we
have asynchronous requests for backup, that will change, but then we can
fine-tune the backup job to only perform a single request on a very
small chunk, too.)

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

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 104be6dded..80a5db521b 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -67,7 +67,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 def test_drive_mirror(self):
 self.do_test_stop("drive-mirror", device="drive0",
   target=self.target_img, format=iotests.imgfmt,
-  sync="full")
+  sync="full", buf_size=65536)
 
 def test_drive_backup(self):
 self.do_test_stop("drive-backup", device="drive0",
-- 
2.29.2

[PATCH v4 05/10] iotests/129: Do not check @busy

[Max Reitz]

@busy is false when the job is paused, which happens all the time
because that is how jobs yield (e.g. for mirror at least since commit
565ac01f8d3).

Back when 129 was added (2015), perhaps there was no better way of
checking whether the job was still actually running.  Now we have the
@status field (as of 58b295ba52c, i.e. 2018), which can give us exactly
that information.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/129 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 2fc65ada6a..dd23bb2e5a 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -69,7 +69,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 result = self.vm.qmp("stop")
 self.assert_qmp(result, 'return', {})
 result = self.vm.qmp("query-block-jobs")
-self.assert_qmp(result, 'return[0]/busy', True)
+self.assert_qmp(result, 'return[0]/status', 'running')
 self.assert_qmp(result, 'return[0]/ready', False)
 
 def test_drive_mirror(self):
-- 
2.29.2

[PATCH v4 07/10] iotests/129: Actually test a commit job

[Max Reitz]

Before this patch, test_block_commit() performs an active commit, which
under the hood is a mirror job.  If we want to test various different
block jobs, we should perhaps run an actual commit job instead.

Doing so requires adding an overlay above the source node before the
commit is done (and then specifying the source node as the top node for
the commit job).

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/129 | 27 +--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index d40e2db24e..104be6dded 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -26,6 +26,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 test_img = os.path.join(iotests.test_dir, 'test.img')
 target_img = os.path.join(iotests.test_dir, 'target.img')
 base_img = os.path.join(iotests.test_dir, 'base.img')
+overlay_img = os.path.join(iotests.test_dir, 'overlay.img')
 
 def setUp(self):
 iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
@@ -36,6 +37,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
 
 source_drive = 'driver=throttle,' \
+   'node-name=source,' \
'throttle-group=tg0,' \
f'file.driver={iotests.imgfmt},' \
f'file.file.filename={self.test_img}'
@@ -45,7 +47,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 
 def tearDown(self):
 self.vm.shutdown()
-for img in (self.test_img, self.target_img, self.base_img):
+for img in (self.test_img, self.target_img, self.base_img,
+self.overlay_img):
 iotests.try_remove(img)
 
 def do_test_stop(self, cmd, **args):
@@ -72,7 +75,27 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
   sync="full")
 
 def test_block_commit(self):
-self.do_test_stop("block-commit", device="drive0")
+# Add overlay above the source node so that we actually use a
+# commit job instead of a mirror job
+
+iotests.qemu_img('create', '-f', iotests.imgfmt, self.overlay_img,
+ '1G')
+
+result = self.vm.qmp('blockdev-add', **{
+ 'node-name': 'overlay',
+ 'driver': iotests.imgfmt,
+ 'file': {
+ 'driver': 'file',
+ 'filename': self.overlay_img
+ }
+ })
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('blockdev-snapshot',
+ node='source', overlay='overlay')
+self.assert_qmp(result, 'return', {})
+
+self.do_test_stop('block-commit', device='drive0', top_node='source')
 
 if __name__ == '__main__':
 iotests.main(supported_fmts=["qcow2"],
-- 
2.29.2

[PATCH v4 09/10] iotests/129: Clean up pylint and mypy complaints

[Max Reitz]

And consequentially drop it from 297's skip list.

Signed-off-by: Max Reitz 
Reviewed-by: Willian Rampazzo 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/129 | 4 ++--
 tests/qemu-iotests/297 | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 80a5db521b..9a56217bf8 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -20,7 +20,6 @@
 
 import os
 import iotests
-import time
 
 class TestStopWithBlockJob(iotests.QMPTestCase):
 test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -32,7 +31,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
 iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
  "-b", self.base_img, '-F', iotests.imgfmt)
-iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', 
self.test_img)
+iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M',
+self.test_img)
 self.vm = iotests.VM()
 self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
 
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index fa9e2cac78..d2d9292839 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -27,7 +27,7 @@ import iotests
 # TODO: Empty this list!
 SKIP_FILES = (
 '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
-'096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
+'096', '118', '124', '132', '136', '139', '147', '148', '149',
 '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
 '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
 '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
-- 
2.29.2

[PATCH v4 00/10] iotests: Fix 129 and expand 297’s reach

[Max Reitz]

Cover letters:
v1: https://lists.nongnu.org/archive/html/qemu-block/2021-01/msg00254.html
v2: https://lists.nongnu.org/archive/html/qemu-block/2021-01/msg00296.html
v3: https://lists.nongnu.org/archive/html/qemu-block/2021-01/msg00371.html

git:
https://github.com/XanClic/qemu.git fix-129-2-v4
https://git.xanclic.moe/XanClic/qemu.git fix-129-2-v4


Hi,

See the v1 cover letter above for the main point of this series (it’s
just that all patch indices are shifted up by two).


The main change in v2 is the extension of iotest 297 to run pylint and
mypy not only on iotests.py, but on every Python file in the
qemu-iotests/ directory that isn’t part of a skip list.

The main changes in v3 are that 297 is rewritten in Python, that patch 1
is added (which helps tests to pass mypy scrutiny without having to
assert that vital variables such as iotests.test_dir are not None), and
that patch 10 is added (because I was already modifying 300 in patch 1,
so I thought i might as well).


Change in v4 (from v3):
- Patch 2:
  - Fix flake8 complaints (PEP8 violations)
  - Modify only a copy of os.environ, and pass that down to pylint and
mypy (instead of accidentally modifying os.environ itself)
  - s/text=True/universal_newlines=True/
(@text was added in Python 3.7, but qemu requires only 3.6)

- Patch 6:
  - Fix flake8 complaints (PEP8 violations); kept R-bs, it’s just a
question of indentation

- Patch 9:
  - Mention modification to 297 in the commit message

- Patch 10:
  - Mention modification to 297 in the commit message
  - s/PYTHONPATH/sys.path/
  - Fix flake8 complaints (PEP8 violations)


git-backport-diff of v3 <-> v4:

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

001/10:[] [--] 'iotests.py: Assume a couple of variables as given'
002/10:[0007] [FC] 'iotests/297: Rewrite in Python and extend reach'
003/10:[] [--] 'iotests: Move try_remove to iotests.py'
004/10:[] [--] 'iotests/129: Remove test images in tearDown()'
005/10:[] [--] 'iotests/129: Do not check @busy'
006/10:[0004] [FC] 'iotests/129: Use throttle node'
007/10:[] [-C] 'iotests/129: Actually test a commit job'
008/10:[] [--] 'iotests/129: Limit mirror job's buffer size'
009/10:[] [--] 'iotests/129: Clean up pylint and mypy complaints'
010/10:[0006] [FC] 'iotests/300: Clean up pylint and mypy complaints'


Max Reitz (10):
  iotests.py: Assume a couple of variables as given
  iotests/297: Rewrite in Python and extend reach
  iotests: Move try_remove to iotests.py
  iotests/129: Remove test images in tearDown()
  iotests/129: Do not check @busy
  iotests/129: Use throttle node
  iotests/129: Actually test a commit job
  iotests/129: Limit mirror job's buffer size
  iotests/129: Clean up pylint and mypy complaints
  iotests/300: Clean up pylint and mypy complaints

 tests/qemu-iotests/124|   8 +--
 tests/qemu-iotests/129|  72 +-
 tests/qemu-iotests/297| 110 +++---
 tests/qemu-iotests/297.out|   5 +-
 tests/qemu-iotests/300|  19 --
 tests/qemu-iotests/iotests.py |  37 ++--
 6 files changed, 169 insertions(+), 82 deletions(-)

-- 
2.29.2

[PATCH v4 06/10] iotests/129: Use throttle node

[Max Reitz]

Throttling on the BB has not affected block jobs in a while, so it is
possible that one of the jobs in 129 finishes before the VM is stopped.
We can fix that by running the job from a throttle node.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/129 | 37 +
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index dd23bb2e5a..d40e2db24e 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -32,20 +32,18 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
  "-b", self.base_img, '-F', iotests.imgfmt)
 iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', 
self.test_img)
-self.vm = iotests.VM().add_drive(self.test_img)
+self.vm = iotests.VM()
+self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
+
+source_drive = 'driver=throttle,' \
+   'throttle-group=tg0,' \
+   f'file.driver={iotests.imgfmt},' \
+   f'file.file.filename={self.test_img}'
+
+self.vm.add_drive(None, source_drive)
 self.vm.launch()
 
 def tearDown(self):
-params = {"device": "drive0",
-  "bps": 0,
-  "bps_rd": 0,
-  "bps_wr": 0,
-  "iops": 0,
-  "iops_rd": 0,
-  "iops_wr": 0,
- }
-result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
- **params)
 self.vm.shutdown()
 for img in (self.test_img, self.target_img, self.base_img):
 iotests.try_remove(img)
@@ -53,33 +51,24 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 def do_test_stop(self, cmd, **args):
 """Test 'stop' while block job is running on a throttled drive.
 The 'stop' command shouldn't drain the job"""
-params = {"device": "drive0",
-  "bps": 1024,
-  "bps_rd": 0,
-  "bps_wr": 0,
-  "iops": 0,
-  "iops_rd": 0,
-  "iops_wr": 0,
- }
-result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
- **params)
-self.assert_qmp(result, 'return', {})
 result = self.vm.qmp(cmd, **args)
 self.assert_qmp(result, 'return', {})
+
 result = self.vm.qmp("stop")
 self.assert_qmp(result, 'return', {})
 result = self.vm.qmp("query-block-jobs")
+
 self.assert_qmp(result, 'return[0]/status', 'running')
 self.assert_qmp(result, 'return[0]/ready', False)
 
 def test_drive_mirror(self):
 self.do_test_stop("drive-mirror", device="drive0",
-  target=self.target_img,
+  target=self.target_img, format=iotests.imgfmt,
   sync="full")
 
 def test_drive_backup(self):
 self.do_test_stop("drive-backup", device="drive0",
-  target=self.target_img,
+  target=self.target_img, format=iotests.imgfmt,
   sync="full")
 
 def test_block_commit(self):
-- 
2.29.2

[PATCH v4 04/10] iotests/129: Remove test images in tearDown()

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Willian Rampazzo 
---
 tests/qemu-iotests/129 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 0e13244d85..2fc65ada6a 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -47,6 +47,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 result = self.vm.qmp("block_set_io_throttle", conv_keys=False,
  **params)
 self.vm.shutdown()
+for img in (self.test_img, self.target_img, self.base_img):
+iotests.try_remove(img)
 
 def do_test_stop(self, cmd, **args):
 """Test 'stop' while block job is running on a throttled drive.
-- 
2.29.2

[PATCH v4 03/10] iotests: Move try_remove to iotests.py

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/124|  8 +---
 tests/qemu-iotests/iotests.py | 11 +++
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 3705cbb6b3..e40eeb50b9 100755
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -22,6 +22,7 @@
 
 import os
 import iotests
+from iotests import try_remove
 
 
 def io_write_patterns(img, patterns):
@@ -29,13 +30,6 @@ def io_write_patterns(img, patterns):
 iotests.qemu_io('-c', 'write -P%s %s %s' % pattern, img)
 
 
-def try_remove(img):
-try:
-os.remove(img)
-except OSError:
-pass
-
-
 def transaction_action(action, **kwargs):
 return {
 'type': action,
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 52facb8e04..a69b4cdc4e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -523,12 +523,15 @@ class FilePath:
 return False
 
 
+def try_remove(img):
+try:
+os.remove(img)
+except OSError:
+pass
+
 def file_path_remover():
 for path in reversed(file_path_remover.paths):
-try:
-os.remove(path)
-except OSError:
-pass
+try_remove(path)
 
 
 def file_path(*names, base_dir=test_dir):
-- 
2.29.2

[PATCH v4 01/10] iotests.py: Assume a couple of variables as given

[Max Reitz]

There are a couple of environment variables that we fetch with
os.environ.get() without supplying a default.  Clearly they are required
and expected to be set by the ./check script (as evidenced by
execute_setup_common(), which checks for test_dir and
qemu_default_machine to be set, and aborts if they are not).

Using .get() this way has the disadvantage of returning an Optional[str]
type, which mypy will complain about when tests just assume these values
to be str.

Use [] instead, which raises a KeyError for environment variables that
are not set.  When this exception is raised, catch it and move the abort
code from execute_setup_common() there.

Drop the 'assert iotests.sock_dir is not None' from iotest 300, because
that sort of thing is precisely what this patch wants to prevent.

Signed-off-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/300|  1 -
 tests/qemu-iotests/iotests.py | 26 +-
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 5b75121b84..b864a21d5e 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -27,7 +27,6 @@ import qemu
 
 BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]
 
-assert iotests.sock_dir is not None
 mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
 
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index dcdcd0387f..52facb8e04 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -75,12 +75,20 @@ qemu_opts = os.environ.get('QEMU_OPTIONS', 
'').strip().split(' ')
 
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
-test_dir = os.environ.get('TEST_DIR')
-sock_dir = os.environ.get('SOCK_DIR')
 output_dir = os.environ.get('OUTPUT_DIR', '.')
-cachemode = os.environ.get('CACHEMODE')
-aiomode = os.environ.get('AIOMODE')
-qemu_default_machine = os.environ.get('QEMU_DEFAULT_MACHINE')
+
+try:
+test_dir = os.environ['TEST_DIR']
+sock_dir = os.environ['SOCK_DIR']
+cachemode = os.environ['CACHEMODE']
+aiomode = os.environ['AIOMODE']
+qemu_default_machine = os.environ['QEMU_DEFAULT_MACHINE']
+except KeyError:
+# We are using these variables as proxies to indicate that we're
+# not being run via "check". There may be other things set up by
+# "check" that individual test cases rely on.
+sys.stderr.write('Please run this test via the "check" script\n')
+sys.exit(os.EX_USAGE)
 
 socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
 
@@ -1294,14 +1302,6 @@ def execute_setup_common(supported_fmts: Sequence[str] = 
(),
 """
 # Note: Python 3.6 and pylint do not like 'Collection' so use 'Sequence'.
 
-# We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
-# indicate that we're not being run via "check". There may be
-# other things set up by "check" that individual test cases rely
-# on.
-if test_dir is None or qemu_default_machine is None:
-sys.stderr.write('Please run this test via the "check" script\n')
-sys.exit(os.EX_USAGE)
-
 debug = '-d' in sys.argv
 if debug:
 sys.argv.remove('-d')
-- 
2.29.2

[PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach

[Max Reitz]

Instead of checking iotests.py only, check all Python files in the
qemu-iotests/ directory.  Of course, most of them do not pass, so there
is an extensive skip list for now.  (The only files that do pass are
209, 254, 283, and iotests.py.)

(Alternatively, we could have the opposite, i.e. an explicit list of
files that we do want to check, but I think it is better to check files
by default.)

Unless started in debug mode (./check -d), the output has no information
on which files are tested, so we will not have a problem e.g. with
backports, where some files may be missing when compared to upstream.

Besides the technical rewrite, some more things are changed:

- For the pylint invocation, PYTHONPATH is adjusted.  This mirrors
  setting MYPYPATH for mypy.

- Also, MYPYPATH is now derived from PYTHONPATH, so that we include
  paths set by the environment.  Maybe at some point we want to let the
  check script add '../../python/' to PYTHONPATH so that iotests.py does
  not need to do that.

- Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
  comments.  TODO is fine, we do not need 297 to complain about such
  comments.

- The "Success" line from mypy's output is suppressed, because (A) it
  does not add useful information, and (B) it would leak information
  about the files having been tested to the reference output, which we
  decidedly do not want.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/297 | 110 +
 tests/qemu-iotests/297.out |   5 +-
 2 files changed, 90 insertions(+), 25 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 5c5420712b..fa9e2cac78 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/usr/bin/env python3
 #
 # Copyright (C) 2020 Red Hat, Inc.
 #
@@ -15,30 +15,96 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see .
 
-seq=$(basename $0)
-echo "QA output created by $seq"
+import os
+import re
+import shutil
+import subprocess
+import sys
 
-status=1   # failure is the default!
+import iotests
 
-# get standard environment
-. ./common.rc
 
-if ! type -p "pylint-3" > /dev/null; then
-_notrun "pylint-3 not found"
-fi
-if ! type -p "mypy" > /dev/null; then
-_notrun "mypy not found"
-fi
+# TODO: Empty this list!
+SKIP_FILES = (
+'030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
+'096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
+'151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
+'203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
+'218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
+'240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
+'262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
+'299', '300', '302', '303', '304', '307',
+'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
+)
 
-pylint-3 --score=n iotests.py
 
-MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \
---disallow-any-generics --disallow-incomplete-defs \
---disallow-untyped-decorators --no-implicit-optional \
---warn-redundant-casts --warn-unused-ignores \
---no-implicit-reexport iotests.py
+def is_python_file(filename):
+if not os.path.isfile(filename):
+return False
 
-# success, all done
-echo "*** done"
-rm -f $seq.full
-status=0
+if filename.endswith('.py'):
+return True
+
+with open(filename) as f:
+try:
+first_line = f.readline()
+return re.match('^#!.*python', first_line) is not None
+except UnicodeDecodeError:  # Ignore binary files
+return False
+
+
+def run_linters():
+files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
+ if is_python_file(filename)]
+
+iotests.logger.debug('Files to be checked:')
+iotests.logger.debug(', '.join(sorted(files)))
+
+print('=== pylint ===')
+sys.stdout.flush()
+
+# Todo notes are fine, but fixme's or xxx's should probably just be
+# fixed (in tests, at least)
+env = os.environ.copy()
+try:
+env['PYTHONPATH'] += ':../../python/'
+except KeyError:
+env['PYTHONPATH'] = '../../python/'
+subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
+   env=env, check=False)
+
+print('=== mypy ===')
+sys.stdout.flush()
+
+# We have to call mypy separately for each file.  Otherwise, it
+# will interpret all given files as belonging together (i.e., they
+# may not both define the same classes, etc.; most notably, they
+# must not both define the __main__ module).
+env['MYPYPATH'] = env['PYTHONPATH']
+for filename in 

Re: [RFC PATCH 0/5] hw/block/nvme: support multi-path for ctrl/ns

[Keith Busch]

On Fri, Jan 15, 2021 at 02:57:45PM +0100, Klaus Jensen wrote:
> 
> As you already mentioned, the problem I see with this approach is that
> we have separate namespaces attached to separate controllers. So we are
> faking it to the max and if I/O starts going through the other
> controller we end up on a namespace that is unrelated (different data).
> Havoc ensues.
> 
> My approach looks a lot like yours, but I hacked around this by adding
> extra 'ctrl-0', 'ctrl-1', ..., link-parameters to the namespace device,
> replacing the bus. This works well because the namespace then just
> registers with multiple controllers. But adding more parameters like
> that just isnt nice, so I've been trying to figure out how to allow a
> parameter to be specified multiple times, so we could just do more
> 'ctrl'-parameters.
> 
> Alas, since I started thinking about namespace sharing I have been
> regretting that I didn't reverse the bus-mechanic for namespace
> attachment. It would align better with the theory of operation if it was
> the controllers that attached to namespaces, and not the other way
> around. So what I would actually really prefer, is that we had multiple
> 'ns' link parameters on the controller device.

Would this work better if we introduce a new device in the nvme hierarchy:
the nvme-subsystem? You could attach multi-path namespaces and
controllers to that, and namespaces you don't want shared can attach
directly to controllers like they do today. You could also auto-assign
cntlid, and you wouldn't need to duplicate serial numbers in your
parameters.

Re: [PATCH 1/4] tests/qtest: Remove TPM tests

[Philippe Mathieu-Daudé]

On 1/15/21 5:06 PM, Philippe Mathieu-Daudé wrote:
> On 1/15/21 4:53 PM, Stefan Berger wrote:
>> On 1/15/21 10:52 AM, Philippe Mathieu-Daudé wrote:
>>> Subject is incorrect, this is not a removal of the tests, but
>>> removal of their execution. The tests are still in the repository.
>>> This is more of a disablement.
>>
>> How do you compile / run them to have the LeakSanitizer checks?
> 
> I used:
> 
> ../configure --cc=clang --enable-sanitizers && make check-qtest
> 
> $ clang -v
> clang version 10.0.1 (Fedora 10.0.1-3.fc32)
> 
> This was previously covered by patchew CI. I just figured
> patchew is running without the LeakSanitizer since commit
> 6f89ec7442e ("docker: test-debug: disable LeakSanitizer"):
> 
>  docker: test-debug: disable LeakSanitizer
> 
>  There are just too many leaks in device-introspect-test (especially for
>  the plethora of arm and aarch64 boards) to make LeakSanitizer useful;
>  disable it for now.

So if this expected, maybe the correct fix is to have meson use
ASAN_OPTIONS=detect_leaks=0 automatically when running the qtests?

Re: [PATCH 1/4] tests/qtest: Remove TPM tests

[Philippe Mathieu-Daudé]

On 1/15/21 4:53 PM, Stefan Berger wrote:
> On 1/15/21 10:52 AM, Philippe Mathieu-Daudé wrote:
>> Subject is incorrect, this is not a removal of the tests, but
>> removal of their execution. The tests are still in the repository.
>> This is more of a disablement.
> 
> How do you compile / run them to have the LeakSanitizer checks?

I used:

../configure --cc=clang --enable-sanitizers && make check-qtest

$ clang -v
clang version 10.0.1 (Fedora 10.0.1-3.fc32)

This was previously covered by patchew CI. I just figured
patchew is running without the LeakSanitizer since commit
6f89ec7442e ("docker: test-debug: disable LeakSanitizer"):

 docker: test-debug: disable LeakSanitizer

 There are just too many leaks in device-introspect-test (especially for
 the plethora of arm and aarch64 boards) to make LeakSanitizer useful;
 disable it for now.

Re: [PATCH 1/4] tests/qtest: Remove TPM tests

[Stefan Berger]

On 1/15/21 10:52 AM, Philippe Mathieu-Daudé wrote:

Subject is incorrect, this is not a removal of the tests, but
removal of their execution. The tests are still in the repository.
This is more of a disablement.


How do you compile / run them to have the LeakSanitizer checks?

Re: [PATCH 1/4] tests/qtest: Remove TPM tests

[Philippe Mathieu-Daudé]

Subject is incorrect, this is not a removal of the tests, but
removal of their execution. The tests are still in the repository.
This is more of a disablement.

On 1/15/21 4:09 PM, Philippe Mathieu-Daudé wrote:
> The TPM tests are failing, and no further tests are run,
> making the rest of the testsuite pointless:
> 
>   $ make check-qtest
>   =
>   ==3330026==ERROR: LeakSanitizer: detected memory leaks
...
>   SUMMARY: AddressSanitizer: 449172 byte(s) leaked in 324 allocation(s).
>   make: *** [Makefile.mtest:1025: run-test-126] Error 1
> 
> Remove these tests to be able to run the rest.
> 
> Cc: Stefan Berger 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/qtest/meson.build | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 16d04625b8b..bcbb04d2bb4 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -41,10 +41,6 @@
>(config_all_devices.has_key('CONFIG_USB_UHCI') and 
>\
> config_all_devices.has_key('CONFIG_USB_EHCI') ? ['usb-hcd-ehci-test'] : 
> []) +\
>(config_all_devices.has_key('CONFIG_USB_XHCI_NEC') ? ['usb-hcd-xhci-test'] 
> : []) +\
> -  (config_all_devices.has_key('CONFIG_TPM_CRB') ? ['tpm-crb-test'] : []) +   
>\
> -  (config_all_devices.has_key('CONFIG_TPM_CRB') ? ['tpm-crb-swtpm-test'] : 
> []) +\
> -  (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test'] : []) 
> +  \
> -  (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-swtpm-test'] 
> : []) +\
>(config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) 
> +  \
>qtests_pci +   
>\
>['fdc-test',
> 

Re: [PATCH 7/7] block/rbd: change request alignment to 1 byte

[Peter Lieven]

Am 15.01.21 um 16:27 schrieb Jason Dillaman:
> On Thu, Jan 14, 2021 at 2:59 PM Peter Lieven  wrote:
>> Am 14.01.21 um 20:19 schrieb Jason Dillaman:
>>> On Sun, Dec 27, 2020 at 11:42 AM Peter Lieven  wrote:
 since we implement byte interfaces and librbd supports aio on byte 
 granularity we can lift
 the 512 byte alignment.

 Signed-off-by: Peter Lieven 
 ---
  block/rbd.c | 2 --
  1 file changed, 2 deletions(-)

 diff --git a/block/rbd.c b/block/rbd.c
 index 27b4404adf..8673e8f553 100644
 --- a/block/rbd.c
 +++ b/block/rbd.c
 @@ -223,8 +223,6 @@ done:
  static void qemu_rbd_refresh_limits(BlockDriverState *bs, Error **errp)
  {
  BDRVRBDState *s = bs->opaque;
 -/* XXX Does RBD support AIO on less than 512-byte alignment? */
 -bs->bl.request_alignment = 512;
>>> Just a suggestion, but perhaps improve discard alignment, max discard,
>>> optimal alignment (if that's something QEMU handles internally) if not
>>> overridden by the user.
>>
>> Qemu supports max_discard and discard_alignment. Is there a call to get 
>> these limits
>>
>> from librbd?
>>
>>
>> What do you mean by optimal_alignment? The object size?
> krbd does a good job of initializing defaults [1] where optimal and
> discard alignment is 64KiB (can actually be 4KiB now), max IO size for
> writes, discards, and write-zeroes is the object size * the stripe
> count.


Okay, I will have a look at it. If qemu issues a write, discard, write_zero 
greater than

obj_size  * stripe count will librbd split it internally or will the request 
fail?


Regarding the alignment it seems that rbd_dev->opts->alloc_size is something 
that comes from the device

configuration and not from rbd? I don't have that information inside the Qemu 
RBD driver.


Peter

Re: [PATCH 7/7] block/rbd: change request alignment to 1 byte

[Jason Dillaman]

On Thu, Jan 14, 2021 at 2:59 PM Peter Lieven  wrote:
>
> Am 14.01.21 um 20:19 schrieb Jason Dillaman:
> > On Sun, Dec 27, 2020 at 11:42 AM Peter Lieven  wrote:
> >> since we implement byte interfaces and librbd supports aio on byte 
> >> granularity we can lift
> >> the 512 byte alignment.
> >>
> >> Signed-off-by: Peter Lieven 
> >> ---
> >>  block/rbd.c | 2 --
> >>  1 file changed, 2 deletions(-)
> >>
> >> diff --git a/block/rbd.c b/block/rbd.c
> >> index 27b4404adf..8673e8f553 100644
> >> --- a/block/rbd.c
> >> +++ b/block/rbd.c
> >> @@ -223,8 +223,6 @@ done:
> >>  static void qemu_rbd_refresh_limits(BlockDriverState *bs, Error **errp)
> >>  {
> >>  BDRVRBDState *s = bs->opaque;
> >> -/* XXX Does RBD support AIO on less than 512-byte alignment? */
> >> -bs->bl.request_alignment = 512;
> > Just a suggestion, but perhaps improve discard alignment, max discard,
> > optimal alignment (if that's something QEMU handles internally) if not
> > overridden by the user.
>
>
> Qemu supports max_discard and discard_alignment. Is there a call to get these 
> limits
>
> from librbd?
>
>
> What do you mean by optimal_alignment? The object size?

krbd does a good job of initializing defaults [1] where optimal and
discard alignment is 64KiB (can actually be 4KiB now), max IO size for
writes, discards, and write-zeroes is the object size * the stripe
count.

> Peter
>
>
>

[1] https://github.com/torvalds/linux/blob/master/drivers/block/rbd.c#L4981

-- 
Jason

[PATCH 4/4] tests/qtest: Only run fuzz-virtio-scsi when virtio-scsi is available

[Philippe Mathieu-Daudé]

This test fails when QEMU is built without the virtio-scsi device,
restrict it to its availability.

Signed-off-by: Philippe Mathieu-Daudé 
---
Cc: "Michael S. Tsirkin" 

Note when running check-qtest-i386 I still get this failure:

  qemu-system-i386: Cannot map used

it comes from virtio_init_region_cache().
---
 tests/qtest/fuzz-test.c | 51 
 tests/qtest/fuzz-virtio-scsi-test.c | 75 +
 MAINTAINERS |  1 +
 tests/qtest/meson.build |  1 +
 4 files changed, 77 insertions(+), 51 deletions(-)
 create mode 100644 tests/qtest/fuzz-virtio-scsi-test.c

diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 6188fbb8e96..d112798afe3 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -25,55 +25,6 @@ static void test_lp1878642_pci_bus_get_irq_level_assert(void)
 qtest_quit(s);
 }
 
-/*
- * Here a MemoryRegionCache pointed to an MMIO region but had a
- * larger size than the underlying region.
- */
-static void test_mmio_oob_from_memory_region_cache(void)
-{
-QTestState *s;
-
-s = qtest_init("-M pc-q35-5.2 -display none -m 512M "
-  "-device virtio-scsi,num_queues=8,addr=03.0 ");
-
-qtest_outl(s, 0xcf8, 0x80001811);
-qtest_outb(s, 0xcfc, 0x6e);
-qtest_outl(s, 0xcf8, 0x80001824);
-qtest_outl(s, 0xcf8, 0x80001813);
-qtest_outl(s, 0xcfc, 0xa08);
-qtest_outl(s, 0xcf8, 0x80001802);
-qtest_outl(s, 0xcfc, 0x5a175a63);
-qtest_outb(s, 0x6e08, 0x9e);
-qtest_writeb(s, 0x9f003, 0xff);
-qtest_writeb(s, 0x9f004, 0x01);
-qtest_writeb(s, 0x9e012, 0x0e);
-qtest_writeb(s, 0x9e01b, 0x0e);
-qtest_writeb(s, 0x9f006, 0x01);
-qtest_writeb(s, 0x9f008, 0x01);
-qtest_writeb(s, 0x9f00a, 0x01);
-qtest_writeb(s, 0x9f00c, 0x01);
-qtest_writeb(s, 0x9f00e, 0x01);
-qtest_writeb(s, 0x9f010, 0x01);
-qtest_writeb(s, 0x9f012, 0x01);
-qtest_writeb(s, 0x9f014, 0x01);
-qtest_writeb(s, 0x9f016, 0x01);
-qtest_writeb(s, 0x9f018, 0x01);
-qtest_writeb(s, 0x9f01a, 0x01);
-qtest_writeb(s, 0x9f01c, 0x01);
-qtest_writeb(s, 0x9f01e, 0x01);
-qtest_writeb(s, 0x9f020, 0x01);
-qtest_writeb(s, 0x9f022, 0x01);
-qtest_writeb(s, 0x9f024, 0x01);
-qtest_writeb(s, 0x9f026, 0x01);
-qtest_writeb(s, 0x9f028, 0x01);
-qtest_writeb(s, 0x9f02a, 0x01);
-qtest_writeb(s, 0x9f02c, 0x01);
-qtest_writeb(s, 0x9f02e, 0x01);
-qtest_writeb(s, 0x9f030, 0x01);
-qtest_outb(s, 0x6e10, 0x00);
-qtest_quit(s);
-}
-
 int main(int argc, char **argv)
 {
 const char *arch = qtest_get_arch();
@@ -83,8 +34,6 @@ int main(int argc, char **argv)
 if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
 qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
test_lp1878642_pci_bus_get_irq_level_assert);
-qtest_add_func("fuzz/test_mmio_oob_from_memory_region_cache",
-   test_mmio_oob_from_memory_region_cache);
 }
 
 return g_test_run();
diff --git a/tests/qtest/fuzz-virtio-scsi-test.c 
b/tests/qtest/fuzz-virtio-scsi-test.c
new file mode 100644
index 000..aaf6d10e189
--- /dev/null
+++ b/tests/qtest/fuzz-virtio-scsi-test.c
@@ -0,0 +1,75 @@
+/*
+ * QTest fuzzer-generated testcase for virtio-scsi device
+ *
+ * Copyright (c) 2020 Li Qiang 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "libqos/libqtest.h"
+
+/*
+ * Here a MemoryRegionCache pointed to an MMIO region but had a
+ * larger size than the underlying region.
+ */
+static void test_mmio_oob_from_memory_region_cache(void)
+{
+QTestState *s;
+
+s = qtest_init("-M pc-q35-5.2 -display none -m 512M "
+   "-device virtio-scsi,num_queues=8,addr=03.0 ");
+
+qtest_outl(s, 0xcf8, 0x80001811);
+qtest_outb(s, 0xcfc, 0x6e);
+qtest_outl(s, 0xcf8, 0x80001824);
+qtest_outl(s, 0xcf8, 0x80001813);
+qtest_outl(s, 0xcfc, 0xa08);
+qtest_outl(s, 0xcf8, 0x80001802);
+qtest_outl(s, 0xcfc, 0x5a175a63);
+qtest_outb(s, 0x6e08, 0x9e);
+qtest_writeb(s, 0x9f003, 0xff);
+qtest_writeb(s, 0x9f004, 0x01);
+qtest_writeb(s, 0x9e012, 0x0e);
+qtest_writeb(s, 0x9e01b, 0x0e);
+qtest_writeb(s, 0x9f006, 0x01);
+qtest_writeb(s, 0x9f008, 0x01);
+qtest_writeb(s, 0x9f00a, 0x01);
+qtest_writeb(s, 0x9f00c, 0x01);
+qtest_writeb(s, 0x9f00e, 0x01);
+qtest_writeb(s, 0x9f010, 0x01);
+qtest_writeb(s, 0x9f012, 0x01);
+qtest_writeb(s, 0x9f014, 0x01);
+qtest_writeb(s, 0x9f016, 0x01);
+qtest_writeb(s, 0x9f018, 0x01);
+qtest_writeb(s, 0x9f01a, 0x01);
+qtest_writeb(s, 0x9f01c, 0x01);
+qtest_writeb(s, 0x9f01e, 0x01);
+qtest_writeb(s, 0x9f020, 0x01);
+qtest_writeb(s, 0x9f022, 0x01);
+qtest_writeb(s, 0x9f024, 0x01);
+qtest_writeb(s, 0x9f026, 

[PATCH 2/4] tests/qtest: Make fuzz-test generic to all targets

[Philippe Mathieu-Daudé]

Tests in fuzz-test's main() already check for the supported
architecture before adding tests, therefore this test is not
specific to the X86 target. Move it to the generic set.

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/qtest/meson.build | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index bcbb04d2bb4..874f5d34674 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -13,7 +13,9 @@
   'qom-test',
   'test-hmp',
   'qos-test',
+  'fuzz-test',
 ]
+
 if config_host.has_key('CONFIG_MODULES')
   qtests_generic += [ 'modules-test' ]
 endif
@@ -50,7 +52,6 @@
'bios-tables-test',
'rtc-test',
'i440fx-test',
-   'fuzz-test',
'fw_cfg-test',
'device-plug-test',
'drive_del-test',
-- 
2.26.2

[PATCH 1/4] tests/qtest: Remove TPM tests

[Philippe Mathieu-Daudé]

The TPM tests are failing, and no further tests are run,
making the rest of the testsuite pointless:

  $ make check-qtest
  =
  ==3330026==ERROR: LeakSanitizer: detected memory leaks

  Indirect leak of 444960 byte(s) in 108 object(s) allocated from:
  #0 0x55a2df5adb87 in calloc (tests/qtest/tpm-crb-swtpm-test+0x266b87)
  #1 0x7f507bbff9b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0)
  #2 0x55a2df898766 in parse_object qobject/json-parser.c:318:12
  #3 0x55a2df897d86 in parse_value qobject/json-parser.c:546:16
  #4 0x55a2df8979be in json_parser_parse qobject/json-parser.c:580:14
  #5 0x55a2df81ccc1 in json_message_process_token 
qobject/json-streamer.c:92:12
  #6 0x55a2df85f773 in json_lexer_feed_char qobject/json-lexer.c:313:13
  #7 0x55a2df85eb04 in json_lexer_feed qobject/json-lexer.c:350:9
  #8 0x55a2df81d7ed in json_message_parser_feed 
qobject/json-streamer.c:121:5
  #9 0x55a2df5f15f9 in qmp_fd_receive tests/qtest/libqtest.c:614:9
  #10 0x55a2df5f1dda in qtest_qmp_receive_dict tests/qtest/libqtest.c:636:12
  #11 0x55a2df5ef444 in qtest_qmp_receive tests/qtest/libqtest.c:624:27
  #12 0x55a2df5f3a2d in qtest_vqmp tests/qtest/libqtest.c:715:12
  #13 0x55a2df5efa62 in qtest_qmp tests/qtest/libqtest.c:756:16
  #14 0x55a2df5eb480 in tpm_util_wait_for_migration_complete 
tests/qtest/tpm-util.c:245:15
  #15 0x55a2df5e4167 in tpm_test_swtpm_migration_test 
tests/qtest/tpm-tests.c:117:5
  #16 0x55a2df5e340c in tpm_crb_swtpm_migration_test 
tests/qtest/tpm-crb-swtpm-test.c:44:5
  #17 0x7f507bc2229d  (/lib64/libglib-2.0.so.0+0x7b29d)

  Indirect leak of 3456 byte(s) in 108 object(s) allocated from:
  #0 0x55a2df5adb87 in calloc (tests/qtest/tpm-crb-swtpm-test+0x266b87)
  #1 0x7f507bbff9b0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x589b0)
  #2 0x55a2df7886af in qdict_put_obj qobject/qdict.c:126:17
  #3 0x55a2df89d706 in parse_pair qobject/json-parser.c:300:5
  #4 0x55a2df898889 in parse_object qobject/json-parser.c:327:13
  #5 0x55a2df897d86 in parse_value qobject/json-parser.c:546:16
  #6 0x55a2df8979be in json_parser_parse qobject/json-parser.c:580:14
  #7 0x55a2df81ccc1 in json_message_process_token 
qobject/json-streamer.c:92:12
  #8 0x55a2df85f773 in json_lexer_feed_char qobject/json-lexer.c:313:13
  #9 0x55a2df85eb04 in json_lexer_feed qobject/json-lexer.c:350:9
  #10 0x55a2df81d7ed in json_message_parser_feed 
qobject/json-streamer.c:121:5
  #11 0x55a2df5f15f9 in qmp_fd_receive tests/qtest/libqtest.c:614:9
  #12 0x55a2df5f1dda in qtest_qmp_receive_dict tests/qtest/libqtest.c:636:12
  #13 0x55a2df5ef444 in qtest_qmp_receive tests/qtest/libqtest.c:624:27
  #14 0x55a2df5f3a2d in qtest_vqmp tests/qtest/libqtest.c:715:12
  #15 0x55a2df5efa62 in qtest_qmp tests/qtest/libqtest.c:756:16
  #16 0x55a2df5eb480 in tpm_util_wait_for_migration_complete 
tests/qtest/tpm-util.c:245:15
  #17 0x55a2df5e4167 in tpm_test_swtpm_migration_test 
tests/qtest/tpm-tests.c:117:5
  #18 0x55a2df5e340c in tpm_crb_swtpm_migration_test 
tests/qtest/tpm-crb-swtpm-test.c:44:5
  #19 0x7f507bc2229d  (/lib64/libglib-2.0.so.0+0x7b29d)

  Indirect leak of 756 byte(s) in 108 object(s) allocated from:
  #0 0x55a2df5ad9cf in malloc (tests/qtest/tpm-crb-swtpm-test+0x2669cf)
  #1 0x7f507bbff958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)

  SUMMARY: AddressSanitizer: 449172 byte(s) leaked in 324 allocation(s).
  make: *** [Makefile.mtest:1025: run-test-126] Error 1

Remove these tests to be able to run the rest.

Cc: Stefan Berger 
Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/qtest/meson.build | 4 
 1 file changed, 4 deletions(-)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 16d04625b8b..bcbb04d2bb4 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -41,10 +41,6 @@
   (config_all_devices.has_key('CONFIG_USB_UHCI') and   
 \
config_all_devices.has_key('CONFIG_USB_EHCI') ? ['usb-hcd-ehci-test'] : []) 
+\
   (config_all_devices.has_key('CONFIG_USB_XHCI_NEC') ? ['usb-hcd-xhci-test'] : 
[]) +\
-  (config_all_devices.has_key('CONFIG_TPM_CRB') ? ['tpm-crb-test'] : []) + 
 \
-  (config_all_devices.has_key('CONFIG_TPM_CRB') ? ['tpm-crb-swtpm-test'] : []) 
+\
-  (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test'] : []) + 
 \
-  (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-swtpm-test'] : 
[]) +\
   (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) + 
 \
   qtests_pci + 
 \
   ['fdc-test',
-- 
2.26.2

Re: [PATCH 6/7] block/rbd: add write zeroes support

[Jason Dillaman]

On Thu, Jan 14, 2021 at 2:41 PM Peter Lieven  wrote:
>
> Am 14.01.21 um 20:19 schrieb Jason Dillaman:
> > On Sun, Dec 27, 2020 at 11:42 AM Peter Lieven  wrote:
> >> Signed-off-by: Peter Lieven 
> >> ---
> >>  block/rbd.c | 31 ++-
> >>  1 file changed, 30 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/block/rbd.c b/block/rbd.c
> >> index 2d77d0007f..27b4404adf 100644
> >> --- a/block/rbd.c
> >> +++ b/block/rbd.c
> >> @@ -63,7 +63,8 @@ typedef enum {
> >>  RBD_AIO_READ,
> >>  RBD_AIO_WRITE,
> >>  RBD_AIO_DISCARD,
> >> -RBD_AIO_FLUSH
> >> +RBD_AIO_FLUSH,
> >> +RBD_AIO_WRITE_ZEROES
> >>  } RBDAIOCmd;
> >>
> >>  typedef struct BDRVRBDState {
> >> @@ -221,8 +222,12 @@ done:
> >>
> >>  static void qemu_rbd_refresh_limits(BlockDriverState *bs, Error **errp)
> >>  {
> >> +BDRVRBDState *s = bs->opaque;
> >>  /* XXX Does RBD support AIO on less than 512-byte alignment? */
> >>  bs->bl.request_alignment = 512;
> >> +#ifdef LIBRBD_SUPPORTS_WRITE_ZEROES
> >> +bs->bl.pwrite_zeroes_alignment = s->object_size;
> > The optimal alignment is 512 bytes -- but it can safely work just fine
> > down to 1 byte alignment since it will pad the request with additional
> > writes if needed.
>
>
> Okay and this will likely be faster than having Qemu doing that request 
> split, right?
>
> If we drop the alignment hint Qemu will pass the original request.
>
>
> >
> >> +#endif
> >>  }
> >>
> >>
> >> @@ -695,6 +700,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
> >> *options, int flags,
> >>  }
> >>
> >>  s->aio_context = bdrv_get_aio_context(bs);
> >> +#ifdef LIBRBD_SUPPORTS_WRITE_ZEROES
> >> +bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
> >> +#endif
> >>
> >>  /* When extending regular files, we get zeros from the OS */
> >>  bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;
> >> @@ -808,6 +816,11 @@ static int coroutine_fn 
> >> qemu_rbd_start_co(BlockDriverState *bs,
> >>  case RBD_AIO_FLUSH:
> >>  r = rbd_aio_flush(s->image, c);
> >>  break;
> >> +#ifdef LIBRBD_SUPPORTS_WRITE_ZEROES
> >> +case RBD_AIO_WRITE_ZEROES:
> >> +r = rbd_aio_write_zeroes(s->image, offset, bytes, c, 0, 0);
> >> +break;
> >> +#endif
> >>  default:
> >>  r = -EINVAL;
> >>  }
> >> @@ -880,6 +893,19 @@ static int coroutine_fn 
> >> qemu_rbd_co_pdiscard(BlockDriverState *bs,
> >>  return qemu_rbd_start_co(bs, offset, count, NULL, 0, RBD_AIO_DISCARD);
> >>  }
> >>
> >> +#ifdef LIBRBD_SUPPORTS_WRITE_ZEROES
> >> +static int
> >> +coroutine_fn qemu_rbd_co_pwrite_zeroes(BlockDriverState *bs, int64_t 
> >> offset,
> >> +  int count, BdrvRequestFlags flags)
> >> +{
> >> +if (!(flags & BDRV_REQ_MAY_UNMAP)) {
> >> +return -ENOTSUP;
> >> +}
> > There is a "RBD_WRITE_ZEROES_FLAG_THICK_PROVISION" flag that you can
> > use to optionally disable unmap.
>
>
> I have seen that. If you want I can support for this, too. But afaik this
>
> is only available since Octopus release?

True -- I didn't backport that support to Nautilus since it was a new
feature vs the bug-fix that the write-zeroes API was introduced to
fix.

>
> Peter
>
>


-- 
Jason

[PATCH 3/4] tests/qtest: Only run fuzz-megasas-test if megasas device is available

[Philippe Mathieu-Daudé]

This test fails when QEMU is built without the megasas device,
restrict it to its availability.

Signed-off-by: Philippe Mathieu-Daudé 
---
 tests/qtest/fuzz-megasas-test.c | 49 +
 tests/qtest/fuzz-test.c | 25 -
 MAINTAINERS |  1 +
 tests/qtest/meson.build |  4 ++-
 4 files changed, 53 insertions(+), 26 deletions(-)
 create mode 100644 tests/qtest/fuzz-megasas-test.c

diff --git a/tests/qtest/fuzz-megasas-test.c b/tests/qtest/fuzz-megasas-test.c
new file mode 100644
index 000..940a76bf25a
--- /dev/null
+++ b/tests/qtest/fuzz-megasas-test.c
@@ -0,0 +1,49 @@
+/*
+ * QTest fuzzer-generated testcase for megasas device
+ *
+ * Copyright (c) 2020 Li Qiang 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include "libqos/libqtest.h"
+
+/*
+ * This used to trigger the assert in scsi_dma_complete
+ * https://bugs.launchpad.net/qemu/+bug/1878263
+ */
+static void test_lp1878263_megasas_zero_iov_cnt(void)
+{
+QTestState *s;
+
+s = qtest_init("-nographic -monitor none -serial none "
+   "-M q35 -device megasas -device scsi-cd,drive=null0 "
+   "-blockdev driver=null-co,read-zeroes=on,node-name=null0");
+qtest_outl(s, 0xcf8, 0x80001818);
+qtest_outl(s, 0xcfc, 0xc101);
+qtest_outl(s, 0xcf8, 0x8000181c);
+qtest_outl(s, 0xcf8, 0x80001804);
+qtest_outw(s, 0xcfc, 0x7);
+qtest_outl(s, 0xcf8, 0x8000186a);
+qtest_writeb(s, 0x14, 0xfe);
+qtest_writeb(s, 0x0, 0x02);
+qtest_outb(s, 0xc1c0, 0x17);
+qtest_quit(s);
+}
+
+int main(int argc, char **argv)
+{
+const char *arch = qtest_get_arch();
+
+g_test_init(, , NULL);
+
+if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+qtest_add_func("fuzz/test_lp1878263_megasas_zero_iov_cnt",
+   test_lp1878263_megasas_zero_iov_cnt);
+}
+
+return g_test_run();
+}
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index cdb1100a0b8..6188fbb8e96 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -11,29 +11,6 @@
 
 #include "libqos/libqtest.h"
 
-/*
- * This used to trigger the assert in scsi_dma_complete
- * https://bugs.launchpad.net/qemu/+bug/1878263
- */
-static void test_lp1878263_megasas_zero_iov_cnt(void)
-{
-QTestState *s;
-
-s = qtest_init("-nographic -monitor none -serial none "
-   "-M q35 -device megasas -device scsi-cd,drive=null0 "
-   "-blockdev driver=null-co,read-zeroes=on,node-name=null0");
-qtest_outl(s, 0xcf8, 0x80001818);
-qtest_outl(s, 0xcfc, 0xc101);
-qtest_outl(s, 0xcf8, 0x8000181c);
-qtest_outl(s, 0xcf8, 0x80001804);
-qtest_outw(s, 0xcfc, 0x7);
-qtest_outl(s, 0xcf8, 0x8000186a);
-qtest_writeb(s, 0x14, 0xfe);
-qtest_writeb(s, 0x0, 0x02);
-qtest_outb(s, 0xc1c0, 0x17);
-qtest_quit(s);
-}
-
 static void test_lp1878642_pci_bus_get_irq_level_assert(void)
 {
 QTestState *s;
@@ -104,8 +81,6 @@ int main(int argc, char **argv)
 g_test_init(, , NULL);
 
 if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-qtest_add_func("fuzz/test_lp1878263_megasas_zero_iov_cnt",
-   test_lp1878263_megasas_zero_iov_cnt);
 qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
test_lp1878642_pci_bus_get_irq_level_assert);
 qtest_add_func("fuzz/test_mmio_oob_from_memory_region_cache",
diff --git a/MAINTAINERS b/MAINTAINERS
index cb0656aec3d..b2ef820a9fa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1925,6 +1925,7 @@ S: Supported
 F: hw/scsi/megasas.c
 F: hw/scsi/mfi.h
 F: tests/qtest/megasas-test.c
+F: tests/qtest/fuzz-megasas-test.c
 
 Network packet abstractions
 M: Dmitry Fleytman 
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 874f5d34674..a24e7f1c34a 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -4,7 +4,9 @@
   subdir_done()
 endif
 
-qtests_generic = [
+qtests_generic = \
+  (config_all_devices.has_key('CONFIG_MEGASAS_SCSI_PCI') ? 
['fuzz-megasas-test'] : []) + \
+  [
   'cdrom-test',
   'device-introspect-test',
   'machine-none-test',
-- 
2.26.2

[PATCH 0/4] tests/qtest: Fixes fuzz-tests

[Philippe Mathieu-Daudé]

tests/qtest/fuzz-test seems to have bitrotten.
Fix it to make it useful.

Philippe Mathieu-Daudé (4):
  tests/qtest: Remove TPM tests
  tests/qtest: Make fuzz-test generic to all targets
  tests/qtest: Only run fuzz-megasas-test if megasas device is available
  tests/qtest: Only run fuzz-virtio-scsi when virtio-scsi is available

 tests/qtest/fuzz-megasas-test.c | 49 +++
 tests/qtest/fuzz-test.c | 76 -
 tests/qtest/fuzz-virtio-scsi-test.c | 75 
 MAINTAINERS |  2 +
 tests/qtest/meson.build | 12 ++---
 5 files changed, 132 insertions(+), 82 deletions(-)
 create mode 100644 tests/qtest/fuzz-megasas-test.c
 create mode 100644 tests/qtest/fuzz-virtio-scsi-test.c

-- 
2.26.2

Re: [PATCH v3 08/10] iotests/129: Limit mirror job's buffer size

[Willian Rampazzo]

On Thu, Jan 14, 2021 at 2:37 PM Max Reitz  wrote:
>
> Issuing 'stop' on the VM drains all nodes.  If the mirror job has many
> large requests in flight, this may lead to significant I/O that looks a
> bit like 'stop' would make the job try to complete (which is what 129
> should verify not to happen).
>
> We can limit the I/O in flight by limiting the buffer size, so mirror
> will make very little progress during the 'stop' drain.
>
> (We do not need to do anything about commit, which has a buffer size of
> 512 kB by default; or backup, which goes cluster by cluster.  Once we
> have asynchronous requests for backup, that will change, but then we can
> fine-tune the backup job to only perform a single request on a very
> small chunk, too.)
>
> Signed-off-by: Max Reitz 
> Reviewed-by: Eric Blake 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/129 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Willian Rampazzo 

Re: [PATCH v3 04/10] iotests/129: Remove test images in tearDown()

[Willian Rampazzo]

On Thu, Jan 14, 2021 at 2:29 PM Max Reitz  wrote:
>
> Signed-off-by: Max Reitz 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> Reviewed-by: Eric Blake 
> ---
>  tests/qemu-iotests/129 | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Willian Rampazzo 

Re: [PATCH v3 09/10] iotests/129: Clean up pylint and mypy complaints

[Willian Rampazzo]

On Fri, Jan 15, 2021 at 6:30 AM Max Reitz  wrote:
>
> On 14.01.21 21:02, Willian Rampazzo wrote:
> > On Thu, Jan 14, 2021 at 2:41 PM Max Reitz  wrote:
> >>
> >> Signed-off-by: Max Reitz 
> >> ---
> >>   tests/qemu-iotests/129 | 4 ++--
> >>   tests/qemu-iotests/297 | 2 +-
> >>   2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
> >> index 6d21470cd7..201d9e0a0b 100755
> >> --- a/tests/qemu-iotests/129
> >> +++ b/tests/qemu-iotests/129
> >> @@ -20,7 +20,6 @@
> >>
> >>   import os
> >>   import iotests
> >> -import time
> >>
> >>   class TestStopWithBlockJob(iotests.QMPTestCase):
> >>   test_img = os.path.join(iotests.test_dir, 'test.img')
> >> @@ -32,7 +31,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
> >>   iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, 
> >> "1G")
> >>   iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
> >>"-b", self.base_img, '-F', iotests.imgfmt)
> >> -iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 
> >> 128M', self.test_img)
> >> +iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 
> >> 128M',
> >> +self.test_img)
> >>   self.vm = iotests.VM()
> >>   self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')
> >>
> >> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> >> index bfa26d280b..1dce1d1b1c 100755
> >> --- a/tests/qemu-iotests/297
> >> +++ b/tests/qemu-iotests/297
> >> @@ -27,7 +27,7 @@ import iotests
> >>   # TODO: Empty this list!
> >>   SKIP_FILES = (
> >>   '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
> >> -'096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
> >> +'096', '118', '124', '132', '136', '139', '147', '148', '149',
> >
> > Is this also part of mypy/pylint cleanup? It seems you are doing more
> > than that here. It would be good to have this listed in the commit
> > message.
>
> Sure, why not.  Something like “And consequentially drop it from 297's
> skip list.”?

+1 Thanks!

>
> Though I think making a test pass pylint+mypy complaints basically means
> exactly to remove it from 297's skip list and then making 297 pass, so
> I’m not entirely sure it’s necessary.  But it can’t hurt, so.
>
> > Despite that,
> >
> > Reviewed-by: Willian Rampazzo 
>
> Thanks!
>
> Max
>
> >>   '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
> >>   '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
> >>   '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
> >> --
> >> 2.29.2
> >>
> >>
> >
>

Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands

[Bin Meng]

Hi Francisco,

On Fri, Jan 15, 2021 at 8:26 PM Francisco Iglesias
 wrote:
>
> Hi Bin,
>
> On [2021 Jan 15] Fri 10:07:52, Bin Meng wrote:
> > Hi Francisco,
> >
> > On Fri, Jan 15, 2021 at 2:13 AM Francisco Iglesias
> >  wrote:
> > >
> > > Hi Bin,
> > >
> > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote:
> > > > From: Bin Meng 
> > > >
> > > > The m25p80 model uses s->needed_bytes to indicate how many follow-up
> > > > bytes are expected to be received after it receives a command. For
> > > > example, depending on the address mode, either 3-byte address or
> > > > 4-byte address is needed.
> > > >
> > > > For fast read family commands, some dummy cycles are required after
> > > > sending the address bytes, and the dummy cycles need to be counted
> > > > in s->needed_bytes. This is where the mess began.
> > > >
> > > > As the variable name (needed_bytes) indicates, the unit is in byte.
> > > > It is not in bit, or cycle. However for some reason the model has
> > > > been using the number of dummy cycles for s->needed_bytes. The right
> > > > approach is to convert the number of dummy cycles to bytes based on
> > > > the SPI protocol, for example, 6 dummy cycles for the Fast Read Quad
> > > > I/O (EBh) should be converted to 3 bytes per the formula (6 * 4 / 8).
> > >
> > > While not being the original implementor I must assume that above 
> > > solution was
> > > considered but not chosen by the developers due to it is inaccuracy (it
> > > wouldn't be possible to model exacly 6 dummy cycles, only a multiple of 8,
> > > meaning that if the controller is wrongly programmed to generate 7 the 
> > > error
> > > wouldn't be caught and the controller will still be considered 
> > > "correct"). Now
> > > that we have this detail in the implementation I'm in favor of keeping 
> > > it, this
> > > also because the detail is already in use for catching exactly above 
> > > error.
> > >
> >
> > I found no clue from the commit message that my proposed solution here
> > was ever considered, otherwise all SPI controller models supporting
> > software generation should have been found out seriously broken long
> > time ago!
>
>
> The controllers you are referring to might lack support for commands requiring
> dummy clock cycles but I really hope they work with the other commands? If so 
> I

I am not sure why you view dummy clock cycles as something special
that needs some special support from the SPI controller. For the case
1 controller, it's nothing special from the controller perspective,
just like sending out a command, or address bytes, or data. The
controller just shifts data bit by bit from its tx fifo and that's it.
In the Xilinx GQSPI controller case, the dummy cycles can either be
sent via a regular data (the case 1 controller) in the tx fifo, or
automatically generated (case 2 controller) by the hardware.

> don't think it is fair to call them 'seriously broken' (and else we should
> probably let the maintainers know about it). Most likely the lack of support

I called it "seriously broken" because current implementation only
considered one type of SPI controllers while completely ignoring the
other type.

> for the commands is because no request has been made for them. Also there is
> one controller that has support.

Definitely it's not "no request". Nearly all SPI flashes support the
Fast Read (0Bh) command today, and 0Bh requires a dummy cycle. This is
"seriously broken" for those case 1 type controllers because they
cannot read anything from the m25p80 model at all. Unless the guest
software being tested only uses Read (03h) command which is not
affected. But I can't find a software that uses Read instead of Fast
Read.

> > The issue you pointed out that we require the total number of dummy
> > bits should be multiple of 8 is true, that's why I added the
> > unimplemented log message in this series (patch 2/3/4) to warn users
> > if this expectation is not met. However this will not cause any issue
> > when running U-Boot or Linux, because both spi-nor drivers expect the
> > same assumption as we do here.
> >
> > See U-Boot spi_nor_read_data() and Linux spi_nor_spimem_read_data(),
> > there is a logic to calculate the dummy bytes needed for fast read
> > command:
> >
> > /* convert the dummy cycles to the number of bytes */
> > op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
> >
> > Note the default dummy cycles configuration for all flashes I have
> > looked into as of today, meets the multiple of 8 assumption. On some
> > flashes the dummy cycle number is configurable, and if it's been
> > configured to be an odd value, it would not work on U-Boot/Linux in
> > the first place.
> >
> > > >
> > > > Things get complicated when interacting with different SPI or QSPI
> > > > flash controllers. There are major two cases:
> > > >
> > > > - Dummy bytes prepared by drivers, and wrote to the controller fifo.
> > > >   For such case, driver will calculate the correct number of dummy
> > > >   

[PATCH] hw/block/nvme: add zoned I/O commands to nvme_io_opc_str()

[Minwoo Im]

Currently, Zoned I/O commands are parsed as unknown:
  pci_nvme_io_cmd cid 768 nsid 1 sqid 4 opc 0x79 opname 'NVME_NVM_CMD_UNKNOWN'

Parse zoned I/O commands along with other I/O commands to print.

Signed-off-by: Minwoo Im 
---
 hw/block/nvme.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index b7fbcca39d9f..0c1cb6fd2549 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -66,6 +66,9 @@ static inline const char *nvme_io_opc_str(uint8_t opc)
 case NVME_CMD_READ: return "NVME_NVM_CMD_READ";
 case NVME_CMD_WRITE_ZEROES: return "NVME_NVM_CMD_WRITE_ZEROES";
 case NVME_CMD_DSM:  return "NVME_NVM_CMD_DSM";
+case NVME_CMD_ZONE_MGMT_SEND:   return "NVME_NVM_CMD_ZONE_MGMT_SEND";
+case NVME_CMD_ZONE_MGMT_RECV:   return "NVME_NVM_CMD_ZONE_MGMT_RECV";
+case NVME_CMD_ZONE_APPEND:  return "NVME_NVM_CMD_ZONE_APPEND";
 default:return "NVME_NVM_CMD_UNKNOWN";
 }
 }
-- 
2.17.1

Re: [PATCH] hw/block/nvme: add zoned I/O commands to nvme_io_opc_str()

[Minwoo Im]

Oh, I think I missed that one :-).

Thanks!

Re: [RFC PATCH 0/5] hw/block/nvme: support multi-path for ctrl/ns

[Klaus Jensen]

On Jan 15 21:05, Minwoo Im wrote:
> Hello,
> 
> This series added support for multi-path I/O with multi-controllers and
> namespace sharing.  By supporting these features, we can test Linux
> kernel mpath(multi-path) code with this NVMe device.
> 
> Patches from the first to third added multi-controller support in a NVM
> subsystem by adding a mpath.ctrl parameter to nvme device.  The rest of
> the patches added namespace sharing support in a NVM subsystem with two
> or more controllers by adding mpath.ns parameter to nvme-ns device.
> 
> Multi-path enabled in kernel with this series for two controllers with a
> namespace:
> 
>   root@vm:~/work# nvme list -v
>   NVM Express Subsystems
> 
>   SubsystemSubsystem-NQN  
>   Controllers
>    
> 
>  
>   nvme-subsys0 nqn.2019-08.org.qemu:serial
>   nvme0, nvme1
> 
>   NVM Express Controllers
> 
>   Device   SN   MN   FR   
> TxPort AddressSubsystemNamespaces
>      
>  -- --  
>   nvme0serial   QEMU NVMe Ctrl   1.0  
> pcie   :01:00.0   nvme-subsys0 nvme0n1
>   nvme1serial   QEMU NVMe Ctrl   1.0  
> pcie   :02:00.0   nvme-subsys0 nvme0n1
> 
>   NVM Express Namespaces
> 
>   Device   NSID Usage  Format   
> Controllers
>     --  
> 
>   nvme0n1  1268.44  MB / 268.44  MB512   B +  0 B   nvme0, 
> nvme1
> 
> The reason why I put 'RFC' tag to this series is mostly about the last
> patch "hw/block/nvme: add namespace sharing param for mpath".  It seems
> like QEMU block backing device does not support to be shared among two
> or more -device(s).  It means that we just can't give same drive=
> property to multiple nvme-ns devices.  This patch has just let -device
> maps to -drive one-to-one(1:1), but if namespae sharing is detected and
> setup by the host kernel, then a single block device will be selected
> for the NVM subsystem.  I'm not sure this is a good start for this
> feature, so I put the RFC tag here.
> 
> Please kindly review!
> 

Hi Minwoo,

First - super awesome that we get this discussion going. I've been
hacking around this a couple of times, but I've never been happy with
the approach.

As you already mentioned, the problem I see with this approach is that
we have separate namespaces attached to separate controllers. So we are
faking it to the max and if I/O starts going through the other
controller we end up on a namespace that is unrelated (different data).
Havoc ensues.

My approach looks a lot like yours, but I hacked around this by adding
extra 'ctrl-0', 'ctrl-1', ..., link-parameters to the namespace device,
replacing the bus. This works well because the namespace then just
registers with multiple controllers. But adding more parameters like
that just isnt nice, so I've been trying to figure out how to allow a
parameter to be specified multiple times, so we could just do more
'ctrl'-parameters.

Alas, since I started thinking about namespace sharing I have been
regretting that I didn't reverse the bus-mechanic for namespace
attachment. It would align better with the theory of operation if it was
the controllers that attached to namespaces, and not the other way
around. So what I would actually really prefer, is that we had multiple
'ns' link parameters on the controller device.

   -device nvme-ns,id=a,nsid=1,...
   -device nvme-ns,id=b,nsid=2,...
   -device nvme-ns,id=c,nsid=3,...
   -device nvme,cntlid=0,serial=foo,ns=a,ns=b
   -device nvme,cntlid=1,serial=foo,ns=a,ns=c

But I havn't been able to figure out how to kick QOM into doing this.
And I'm definitely not sure this is the way to go. Should we instead
introduce a 'nvme-subsys' device and walk a bus?

I'd really appreciate some input on how we should model this if anyone
has any thoughts. And I think we should consider stuff like detached
namespaces as well. Support for Namespace Management. The whole shabang.


signature.asc
Description: PGP signature

Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands

[Bin Meng]

Hi Havard,

On Fri, Jan 15, 2021 at 11:29 AM Havard Skinnemoen
 wrote:
>
> Hi Bin,
>
> On Thu, Jan 14, 2021 at 6:08 PM Bin Meng  wrote:
> >
> > Hi Francisco,
> >
> > On Fri, Jan 15, 2021 at 2:13 AM Francisco Iglesias
> >  wrote:
> > >
> > > Hi Bin,
> > >
> > > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote:
> > > > From: Bin Meng 
> > > >
> > > > The m25p80 model uses s->needed_bytes to indicate how many follow-up
> > > > bytes are expected to be received after it receives a command. For
> > > > example, depending on the address mode, either 3-byte address or
> > > > 4-byte address is needed.
> > > >
> > > > For fast read family commands, some dummy cycles are required after
> > > > sending the address bytes, and the dummy cycles need to be counted
> > > > in s->needed_bytes. This is where the mess began.
> > > >
> > > > As the variable name (needed_bytes) indicates, the unit is in byte.
> > > > It is not in bit, or cycle. However for some reason the model has
> > > > been using the number of dummy cycles for s->needed_bytes. The right
> > > > approach is to convert the number of dummy cycles to bytes based on
> > > > the SPI protocol, for example, 6 dummy cycles for the Fast Read Quad
> > > > I/O (EBh) should be converted to 3 bytes per the formula (6 * 4 / 8).
> > >
> > > While not being the original implementor I must assume that above 
> > > solution was
> > > considered but not chosen by the developers due to it is inaccuracy (it
> > > wouldn't be possible to model exacly 6 dummy cycles, only a multiple of 8,
> > > meaning that if the controller is wrongly programmed to generate 7 the 
> > > error
> > > wouldn't be caught and the controller will still be considered 
> > > "correct"). Now
> > > that we have this detail in the implementation I'm in favor of keeping 
> > > it, this
> > > also because the detail is already in use for catching exactly above 
> > > error.
> > >
> >
> > I found no clue from the commit message that my proposed solution here
> > was ever considered, otherwise all SPI controller models supporting
> > software generation should have been found out seriously broken long
> > time ago!
> >
> > The issue you pointed out that we require the total number of dummy
> > bits should be multiple of 8 is true, that's why I added the
> > unimplemented log message in this series (patch 2/3/4) to warn users
> > if this expectation is not met. However this will not cause any issue
> > when running U-Boot or Linux, because both spi-nor drivers expect the
> > same assumption as we do here.
> >
> > See U-Boot spi_nor_read_data() and Linux spi_nor_spimem_read_data(),
> > there is a logic to calculate the dummy bytes needed for fast read
> > command:
> >
> > /* convert the dummy cycles to the number of bytes */
> > op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
> >
> > Note the default dummy cycles configuration for all flashes I have
> > looked into as of today, meets the multiple of 8 assumption. On some
> > flashes the dummy cycle number is configurable, and if it's been
> > configured to be an odd value, it would not work on U-Boot/Linux in
> > the first place.
> >
> > > >
> > > > Things get complicated when interacting with different SPI or QSPI
> > > > flash controllers. There are major two cases:
> > > >
> > > > - Dummy bytes prepared by drivers, and wrote to the controller fifo.
> > > >   For such case, driver will calculate the correct number of dummy
> > > >   bytes and write them into the tx fifo. Fixing the m25p80 model will
> > > >   fix flashes working with such controllers.
> > >
> > > Above can be fixed while still keeping the detailed dummy cycle 
> > > implementation
> > > inside m25p80. Perhaps one of the following could be looked into: 
> > > configurating
> > > the amount, letting the spi ctrl fetch the amount from m25p80 or by 
> > > inheriting
> > > some functionality handling this in the SPI controller. Or a mixture of 
> > > above.
> >
> > Please send patches to explain this in detail how this is going to
> > work. I am open to all possible solutions.
> >
> > >
> > > > - Dummy bytes not prepared by drivers. Drivers just tell the hardware
> > > >   the dummy cycle configuration via some registers, and hardware will
> > > >   automatically generate dummy cycles for us. Fixing the m25p80 model
> > > >   is not enough, and we will need to fix the SPI/QSPI models for such
> > > >   controllers.
> > > >
> > > > This series fixes the mess in the m25p80 from the flash side first,
> > >
> > > Considering the problems solved by the solution in tree I find m25p80 
> > > pretty
> > > clean, at least I don't see any clearly better way for accurately 
> > > modeling the
> > > dummy clock cycles. Counting bits instead of bytes would for example still
> > > force the controllers to mark which bits to count (when transmitting one 
> > > dummy
> > > byte from a txfifo on four lines (Quad command) it generates 2 dummy clock
> > > cycles since it takes two cycles to 

Re: [PATCH] hw/block/nvme: add zoned I/O commands to nvme_io_opc_str()

[Klaus Jensen]

On Jan 15 22:48, Minwoo Im wrote:
> Currently, Zoned I/O commands are parsed as unknown:
>   pci_nvme_io_cmd cid 768 nsid 1 sqid 4 opc 0x79 opname 'NVME_NVM_CMD_UNKNOWN'
> 
> Parse zoned I/O commands along with other I/O commands to print.
> 
> Signed-off-by: Minwoo Im 
> ---
>  hw/block/nvme.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index b7fbcca39d9f..0c1cb6fd2549 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -66,6 +66,9 @@ static inline const char *nvme_io_opc_str(uint8_t opc)
>  case NVME_CMD_READ: return "NVME_NVM_CMD_READ";
>  case NVME_CMD_WRITE_ZEROES: return "NVME_NVM_CMD_WRITE_ZEROES";
>  case NVME_CMD_DSM:  return "NVME_NVM_CMD_DSM";
> +case NVME_CMD_ZONE_MGMT_SEND:   return "NVME_NVM_CMD_ZONE_MGMT_SEND";
> +case NVME_CMD_ZONE_MGMT_RECV:   return "NVME_NVM_CMD_ZONE_MGMT_RECV";
> +case NVME_CMD_ZONE_APPEND:  return "NVME_NVM_CMD_ZONE_APPEND";
>  default:return "NVME_NVM_CMD_UNKNOWN";
>  }
>  }
> -- 
> 2.17.1
> 

I have this in a series under review. Compare is also missing ;)

"[PATCH 5/6] hw/block/nvme: add missing string representations for commands"


signature.asc
Description: PGP signature

Re: [PATCH v3 11/10] iotests: add flake8 linter

[Vladimir Sementsov-Ogievskiy]

15.01.2021 15:03, Max Reitz wrote:

On 15.01.21 12:53, Vladimir Sementsov-Ogievskiy wrote:

pylint is good, but doesn't cover the PEP8. Let's add flake8, to be
sure that our code sutisfy PEP8. Add new linter and fix some code style
in checked files.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi!

Here is my small addition to Max's series, hope you like it!

Note, that this is not the first occurrence of 'flake8' in Qemu:

 # git grep flake8
 python/qemu/.flake8:[flake8]
 scripts/qapi/.flake8:[flake8]
 scripts/qapi/.flake8:extend-ignore = E722  # Prefer pylint's bare-except 
checks to flake8's


  tests/qemu-iotests/129    |  6 ++-
  tests/qemu-iotests/254    |  2 +-
  tests/qemu-iotests/297    | 21 ++---
  tests/qemu-iotests/297.out    |  1 +
  tests/qemu-iotests/300    |  4 +-
  tests/qemu-iotests/iotests.py | 88 +--
  6 files changed, 106 insertions(+), 16 deletions(-)


Looks reasonable to me, but perhaps it should just be a dedicated series.  I 
think there’s enough in here to justify that.


Not a problem, I can resend :)




diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 201d9e0a0b..28ec1d 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -21,6 +21,7 @@
  import os
  import iotests
+
  class TestStopWithBlockJob(iotests.QMPTestCase):
  test_img = os.path.join(iotests.test_dir, 'test.img')
  target_img = os.path.join(iotests.test_dir, 'target.img')
@@ -39,8 +40,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
  source_drive = 'driver=throttle,' \
 'node-name=source,' \
 'throttle-group=tg0,' \
-  f'file.driver={iotests.imgfmt},' \
-  f'file.file.filename={self.test_img}'
+   f'file.driver={iotests.imgfmt},' \
+   f'file.file.filename={self.test_img}'


Interesting, when indenting this, I was wondering whether pylint would 
complain.  I was so glad it didn’t.  I really don’t like PEP8.

(Though I understand that style guides like PEP8 are there specifically so when 
someone like me goes “but I like this style better :(”, everyone else can say 
“but you’re objectively wrong”.  So me hating it kind of is its point, I guess.)



When I noted how you indent 'string' with f'string', I thought "o, interesting 
idea".. I just very like the idea of standardized code-style for the language, so I 
just used to follow PEP8 since first time I learned about it.

--
Best regards,
Vladimir

Re: [RFC PATCH 0/2] Allow changing bs->file on reopen

[Kashyap Chamarthy]

On Fri, Jan 15, 2021 at 02:02:36PM +0100, Alberto Garcia wrote:
> Hi,

Hi, 

> during the past months we talked about making x-blockdev-reopen stable
> API, and one of the missing things was having support for changing
> bs->file. See here for the discusssion (I can't find the message from
> Kashyap that started the thread in the web archives):
> 
>https://lists.gnu.org/archive/html/qemu-block/2020-10/msg00922.html

Yeah, I noticed that too -- seems like it got "lost" somehow :-(.  For
the record, I've attached here the original e-mail I sent on 06-OCT-2020
that started the above thread.

Thanks for working on this!

> I was testing this and one of the problems that I found was that
> removing a filter node using this command is tricky because of the
> permission system, see here for details:
> 
>https://lists.gnu.org/archive/html/qemu-block/2020-12/msg00092.html
> 
> The good news is that Vladimir posted a set of patches that changes
> the way that permissions are updated on reopen:
> 
>https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00745.html
> 
> I was testing if this would be useful to solve the problem that I
> mentioned earlier and it seems to be the case so I wrote a patch to
> add support for changing bs->file, along with a couple of test cases.
> 
> This is still an RFC but you can see the idea.
> 
> These patches apply on top of Vladimir's branch:
> 
> git: https://src.openvz.org/scm/~vsementsov/qemu.git
> tag: up-block-topologic-perm-v2
> 
> Opinions are very welcome!
> 
> Berto
> 
> Alberto Garcia (2):
>   block: Allow changing bs->file on reopen
>   iotests: Update 245 to support replacing files with x-blockdev-reopen
> 
>  include/block/block.h  |  1 +
>  block.c| 61 ++
>  tests/qemu-iotests/245 | 61 +++---
>  tests/qemu-iotests/245.out |  4 +--
>  4 files changed, 121 insertions(+), 6 deletions(-)
> 
> -- 
> 2.20.1
> 

-- 
/kashyap
Date: Tue, 6 Oct 2020 11:10:01 +0200
From: Kashyap Chamarthy 
To: qemu-de...@nongnu.org, qemu-block@nongnu.org
Cc: be...@igalia.com, ebl...@redhat.com
Subject: Plans to bring QMP 'x-blockdev-reopen' out of experimental?
Message-ID: <20201006091001.GA64583@paraplu>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hi, folks

If this was already discussed on the list, please point me to the
thread.  I took a quick look at my local archives, I didn't find any,
besides patches to tests.

I learn that `x-blockdev-reopen` enables a couple of interesting use
cases:

(#) Allowing one to live-change the backing file to point to a different
location, with the target having content identical to original.
This one I was already familiar with.

(#) Yesterday I learnt another use case from Peter Krempa and Eric
Blake.  Allow me to quote (paraphrasing) Eric's example from IRC.
E.g.  we have (where 'overlay1' has a bitmap):

base <- overlay1

Then create a temporary snapshot (which results a bitmap being
created in 'overlay2', because 'overlay1' had one):

base <- overlay1 <- overlay2

If you want to do a `block-commit` to merge 'overlay2' back into
'overlay1', currently upstream QEMU does not merge the bitmap states
from 'overlay2' back into 'overlay1' properly.  This current
limitation is because QEMU can't merge the bitmaps unless it can
reopen 'overlay1' [for read-write] _prior_ to doing the commit — but
the only way to do that is with `x-blockdev-reopen`.

- - -

>From an old chat with Berto on #qemu, he was looking for some more
robust testing, before lifting it out of experimental mode, as it was a
rather complicated command to implement.

Currently, I see there are some 'qemu-iotests' that exercise
'x-blockdev-reopen': 155, 165, 245, and 248.  What else kind of tests
can give more confidence?

(I personally don't have an urgent need for this, so I'm not trying to
rush anything. :-))

-- 
/kashyap

Re: [PATCH v6 08/11] iotests: add testenv.py

[Vladimir Sementsov-Ogievskiy]

15.01.2021 16:20, Kevin Wolf wrote:

Am 15.01.2021 um 14:10 hat Vladimir Sementsov-Ogievskiy geschrieben:

15.01.2021 15:45, Kevin Wolf wrote:

Am 15.01.2021 um 13:19 hat Vladimir Sementsov-Ogievskiy geschrieben:

15.01.2021 14:18, Kevin Wolf wrote:

Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben:

Add TestEnv class, which will handle test environment in a new python
iotests running framework.

Difference with current ./check interface:
- -v (verbose) option dropped, as it is unused

- -xdiff option is dropped, until somebody complains that it is needed
- same for -n option

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
tests/qemu-iotests/testenv.py | 328 ++
1 file changed, 328 insertions(+)
create mode 100755 tests/qemu-iotests/testenv.py



[..]


+def init_binaries(self):
+"""Init binary path variables:
+ PYTHON (for bash tests)
+ QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, QEMU_NBD_PROG, QSD_PROG
+ SOCKET_SCM_HELPER
+"""
+self.python = '/usr/bin/python3 -B'


This doesn't look right, we need to respect the Python binary set in
configure (which I think we get from common.env)


Oh, I missed the change. Then I should just drop this self.python.


Do we still get the value from elsewhere or do we need to manually parse
common.env?


Hmm.. Good question. We have either parse common.env, and still create 
self.python variable.

Or drop it, and include common.env directly to bash tests. For this we'll need 
to export

BUILD_IOTESTS, and do
  . $BUILD_IOTESTS/common.env

in common.rc..


check uses it, too, for running Python test cases.



But new check (written in python) doesn't.. Should I keep bash check, which 
will have only one line to call check.py with help of PYTHON?


--
Best regards,
Vladimir

Re: [PATCH v6 08/11] iotests: add testenv.py

[Kevin Wolf]

Am 15.01.2021 um 14:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 15.01.2021 15:45, Kevin Wolf wrote:
> > Am 15.01.2021 um 13:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 15.01.2021 14:18, Kevin Wolf wrote:
> > > > Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > > Add TestEnv class, which will handle test environment in a new python
> > > > > iotests running framework.
> > > > > 
> > > > > Difference with current ./check interface:
> > > > > - -v (verbose) option dropped, as it is unused
> > > > > 
> > > > > - -xdiff option is dropped, until somebody complains that it is needed
> > > > > - same for -n option
> > > > > 
> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy 
> > > > > ---
> > > > >tests/qemu-iotests/testenv.py | 328 
> > > > > ++
> > > > >1 file changed, 328 insertions(+)
> > > > >create mode 100755 tests/qemu-iotests/testenv.py
> > > > > 
> 
> [..]
> 
> > > > > +def init_binaries(self):
> > > > > +"""Init binary path variables:
> > > > > + PYTHON (for bash tests)
> > > > > + QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, QEMU_NBD_PROG, 
> > > > > QSD_PROG
> > > > > + SOCKET_SCM_HELPER
> > > > > +"""
> > > > > +self.python = '/usr/bin/python3 -B'
> > > > 
> > > > This doesn't look right, we need to respect the Python binary set in
> > > > configure (which I think we get from common.env)
> > > 
> > > Oh, I missed the change. Then I should just drop this self.python.
> > 
> > Do we still get the value from elsewhere or do we need to manually parse
> > common.env?
> 
> Hmm.. Good question. We have either parse common.env, and still create 
> self.python variable.
> 
> Or drop it, and include common.env directly to bash tests. For this we'll 
> need to export
> 
> BUILD_IOTESTS, and do
>  . $BUILD_IOTESTS/common.env
> 
> in common.rc..

check uses it, too, for running Python test cases.

Kevin

Re: [PATCH v6 08/11] iotests: add testenv.py

[Vladimir Sementsov-Ogievskiy]

15.01.2021 15:45, Kevin Wolf wrote:

Am 15.01.2021 um 13:19 hat Vladimir Sementsov-Ogievskiy geschrieben:

15.01.2021 14:18, Kevin Wolf wrote:

Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben:

Add TestEnv class, which will handle test environment in a new python
iotests running framework.

Difference with current ./check interface:
- -v (verbose) option dropped, as it is unused

- -xdiff option is dropped, until somebody complains that it is needed
- same for -n option

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
   tests/qemu-iotests/testenv.py | 328 ++
   1 file changed, 328 insertions(+)
   create mode 100755 tests/qemu-iotests/testenv.py



[..]


+def init_binaries(self):
+"""Init binary path variables:
+ PYTHON (for bash tests)
+ QEMU_PROG, QEMU_IMG_PROG, QEMU_IO_PROG, QEMU_NBD_PROG, QSD_PROG
+ SOCKET_SCM_HELPER
+"""
+self.python = '/usr/bin/python3 -B'


This doesn't look right, we need to respect the Python binary set in
configure (which I think we get from common.env)


Oh, I missed the change. Then I should just drop this self.python.


Do we still get the value from elsewhere or do we need to manually parse
common.env?


Hmm.. Good question. We have either parse common.env, and still create 
self.python variable.

Or drop it, and include common.env directly to bash tests. For this we'll need 
to export

BUILD_IOTESTS, and do
 . $BUILD_IOTESTS/common.env

in common.rc..







+def root(*names):
+return os.path.join(self.build_root, *names)
+
+arch = os.uname().machine
+if 'ppc64' in arch:
+arch = 'ppc64'
+
+self.qemu_prog = os.getenv('QEMU_PROG', root(f'qemu-system-{arch}'))
+self.qemu_img_prog = os.getenv('QEMU_IMG_PROG', root('qemu-img'))
+self.qemu_io_prog = os.getenv('QEMU_IO_PROG', root('qemu-io'))
+self.qemu_nbd_prog = os.getenv('QEMU_NBD_PROG', root('qemu-nbd'))
+self.qsd_prog = os.getenv('QSD_PROG', root('storage-daemon',
+   'qemu-storage-daemon'))
+
+for b in [self.qemu_img_prog, self.qemu_io_prog, self.qemu_nbd_prog,
+  self.qemu_prog, self.qsd_prog]:
+if not os.path.exists(b):
+exit('Not such file: ' + b)
+if not os.access(b, os.X_OK):
+exit('Not executable: ' + b)
+
+helper_path = os.path.join(self.build_iotests, 'socket_scm_helper')
+if os.access(helper_path, os.X_OK):
+self.socket_scm_helper = helper_path  # SOCKET_SCM_HELPER
+
+def __init__(self, argv: List[str]) -> None:
+"""Parse args and environment"""
+
+# Initialize generic paths: build_root, build_iotests, source_iotests,
+# which are needed to initialize some environment variables. They are
+# used by init_*() functions as well.
+
+
+if os.path.islink(sys.argv[0]):
+# called from the build tree
+self.source_iotests = os.path.dirname(os.readlink(sys.argv[0]))
+self.build_iotests = os.path.dirname(os.path.abspath(sys.argv[0]))
+else:
+# called from the source tree
+self.source_iotests = os.getcwd()
+self.build_iotests = self.source_iotests
+
+self.build_root = os.path.join(self.build_iotests, '..', '..')
+
+self.init_handle_argv(argv)
+self.init_directories()
+self.init_binaries()
+
+# QEMU_OPTIONS
+self.qemu_options = '-nodefaults -display none -accel qtest'
+machine_map = (
+(('arm', 'aarch64'), 'virt'),


How does this work? Won't we check for "qemu-system-('arm', 'aarch64')"
below, which we'll never find?


Hmm. I just took existing logic from check:

[..]
   case "$QEMU_PROG" in
   *qemu-system-arm|*qemu-system-aarch64)
   export QEMU_OPTIONS="$QEMU_OPTIONS -machine virt"
   ;;
[..]


What I mean is that the code below doesn't look like it's prepared to
interpret a tuple like ('arm', 'aarch64'), it expects a single string:




+('avr', 'mega2560'),
+('rx', 'gdbsim-r5f562n8'),
+('tricore', 'tricore_testboard')
+)
+for suffix, machine in machine_map:
+if self.qemu_prog.endswith(f'qemu-system-{suffix}'):


Here we get effectively:

 suffix: Tuple[str, str] = ('arm', 'aarch64')

The formatted string uses str(suffix), which makes the result
"qemu-system-('arm', 'aarch64')".

Or am I misunderstanding something here?


Ah, you are right:) Will fix. I interpreted your

  Won't we check for "qemu-system-('arm', 'aarch64')"

as
  
  Won't we check for "qemu-system-arm" or "qemu-system-aarch64"




--
Best regards,
Vladimir

[RFC PATCH 1/2] block: Allow changing bs->file on reopen

[Alberto Garcia]

When the x-blockdev-reopen was added it allowed reconfiguring the
graph by replacing backing files, but changing the 'file' option was
forbidden. Because of this restriction some operations are not
possible, notably inserting and removing block filters.

This patch adds support for replacing the 'file' option. This is
similar to replacing the backing file and the user is likewise
responsible for the correctness of the resulting graph, otherwise this
can lead to data corruption.

Signed-off-by: Alberto Garcia 
---
 include/block/block.h  |  1 +
 block.c| 61 ++
 tests/qemu-iotests/245 |  7 ++---
 3 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 82271d9ccd..6dd687a69e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -196,6 +196,7 @@ typedef struct BDRVReopenState {
 bool backing_missing;
 bool replace_backing_bs;  /* new_backing_bs is ignored if this is false */
 BlockDriverState *old_backing_bs; /* keep pointer for permissions update */
+BlockDriverState *old_file_bs;/* keep pointer for permissions update */
 uint64_t perm, shared_perm;
 QDict *options;
 QDict *explicit_options;
diff --git a/block.c b/block.c
index 576b145cbf..114788e58e 100644
--- a/block.c
+++ b/block.c
@@ -3978,6 +3978,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 refresh_list = bdrv_topological_dfs(refresh_list, found,
 state->old_backing_bs);
 }
+if (state->old_file_bs) {
+refresh_list = bdrv_topological_dfs(refresh_list, found,
+state->old_file_bs);
+}
 }
 
 ret = bdrv_list_refresh_perms(refresh_list, bs_queue, , errp);
@@ -4196,6 +4200,57 @@ static int bdrv_reopen_parse_backing(BDRVReopenState 
*reopen_state,
 return 0;
 }
 
+static int bdrv_reopen_parse_file(BDRVReopenState *reopen_state,
+  GSList **tran,
+  Error **errp)
+{
+BlockDriverState *bs = reopen_state->bs;
+BlockDriverState *new_file_bs;
+QObject *value;
+const char *str;
+
+value = qdict_get(reopen_state->options, "file");
+if (value == NULL) {
+return 0;
+}
+
+/* The 'file' option only allows strings */
+assert(qobject_type(value) == QTYPE_QSTRING);
+
+str = qobject_get_try_str(value);
+new_file_bs = bdrv_lookup_bs(NULL, str, errp);
+if (new_file_bs == NULL) {
+return -EINVAL;
+} else if (bdrv_recurse_has_child(new_file_bs, bs)) {
+error_setg(errp, "Making '%s' a file of '%s' "
+   "would create a cycle", str, bs->node_name);
+return -EINVAL;
+}
+
+assert(bs->file && bs->file->bs);
+
+/* If 'file' points to the current child then there's nothing to do */
+if (bs->file->bs == new_file_bs) {
+return 0;
+}
+
+/* Check AioContext compatibility */
+if (!bdrv_reopen_can_attach(bs, bs->file, new_file_bs, errp)) {
+return -EINVAL;
+}
+
+/* At the moment only backing links are frozen */
+assert(!bs->file->frozen);
+
+/* Store the old file bs because we'll need to refresh its permissions */
+reopen_state->old_file_bs = bs->file->bs;
+
+/* And finally replace the child */
+bdrv_replace_child(bs->file, new_file_bs, tran);
+
+return 0;
+}
+
 /*
  * Prepares a BlockDriverState for reopen. All changes are staged in the
  * 'opaque' field of the BDRVReopenState, which is used and allocated by
@@ -4347,6 +4402,12 @@ static int bdrv_reopen_prepare(BDRVReopenState 
*reopen_state,
 }
 qdict_del(reopen_state->options, "backing");
 
+ret = bdrv_reopen_parse_file(reopen_state, set_backings_tran, errp);
+if (ret < 0) {
+goto error;
+}
+qdict_del(reopen_state->options, "file");
+
 /* Options that are not handled are only okay if they are unchanged
  * compared to the old state. It is expected that some options are only
  * used for the initial open, but not reopen (e.g. filename) */
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index e60c8326d3..f9d68b3958 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -145,8 +145,8 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 
'driver'")
 self.reopen(opts, {'driver': ''}, "Invalid parameter ''")
 self.reopen(opts, {'driver': None}, "Invalid parameter type for 
'driver', expected: string")
-self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 
'file'")
-self.reopen(opts, {'file': ''}, "Cannot change the option 'file'")
+self.reopen(opts, {'file': 'not-found'}, "Cannot find device= nor 
node_name=not-found")
+self.reopen(opts, {'file': ''}, "Cannot find device= 

[RFC PATCH 2/2] iotests: Update 245 to support replacing files with x-blockdev-reopen

[Alberto Garcia]

Signed-off-by: Alberto Garcia 
---
 tests/qemu-iotests/245 | 54 +-
 tests/qemu-iotests/245.out |  4 +--
 2 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index f9d68b3958..bad6911f0c 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -78,7 +78,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 for line in log.split("\n"):
 if line.startswith("Pattern verification failed"):
 raise Exception("%s (command #%d)" % (line, found))
-if re.match("read .*/.* bytes at offset", line):
+if re.match("(read|wrote) .*/.* bytes at offset", line):
 found += 1
 self.assertEqual(found, self.total_io_cmds,
  "Expected output of %d qemu-io commands, found %d" %
@@ -536,6 +536,58 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 
'bv')
 self.assert_qmp(result, 'return', {})
 
+def test_replace_file(self):
+qemu_img('create', '-f', 'raw', hd_path[0], '10k')
+qemu_img('create', '-f', 'raw', hd_path[1], '10k')
+
+hd0_opts = {'driver': 'file',
+'node-name': 'hd0-file',
+'filename':  hd_path[0] }
+hd1_opts = {'driver': 'file',
+'node-name': 'hd1-file',
+'filename':  hd_path[1] }
+
+opts = {'driver': 'raw', 'node-name': 'hd', 'file': 'hd0-file'}
+
+result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts)
+self.assert_qmp(result, 'return', {})
+result = self.vm.qmp('blockdev-add', conv_keys = False, **hd1_opts)
+self.assert_qmp(result, 'return', {})
+result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+self.assert_qmp(result, 'return', {})
+
+self.run_qemu_io("hd", "read  -P 0 0 10k")
+self.run_qemu_io("hd", "write -P 0xa0 0 10k")
+
+self.reopen(opts, {'file': 'hd1-file'})
+self.run_qemu_io("hd", "read  -P 0 0 10k")
+self.run_qemu_io("hd", "write -P 0xa1 0 10k")
+
+self.reopen(opts, {'file': 'hd0-file'})
+self.run_qemu_io("hd", "read  -P 0xa0 0 10k")
+
+self.reopen(opts, {'file': 'hd1-file'})
+self.run_qemu_io("hd", "read  -P 0xa1 0 10k")
+
+def test_insert_throttle_filter(self):
+hd0_opts = hd_opts(0)
+result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts)
+self.assert_qmp(result, 'return', {})
+
+opts = { 'qom-type': 'throttle-group', 'id': 'group0',
+ 'props': { 'limits': { 'iops-total': 1000 } } }
+result = self.vm.qmp('object-add', conv_keys = False, **opts)
+self.assert_qmp(result, 'return', {})
+
+opts = { 'driver': 'throttle', 'node-name': 'throttle0',
+ 'throttle-group': 'group0', 'file': 'hd0-file' }
+result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
+self.assert_qmp(result, 'return', {})
+
+self.reopen(hd0_opts, {'file': 'throttle0'})
+
+self.reopen(hd0_opts, {'file': 'hd0-file'})
+
 # Misc reopen tests with different block drivers
 @iotests.skip_if_unsupported(['quorum', 'throttle'])
 def test_misc_drivers(self):
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 4b33dcaf5c..537a2b5b63 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -10,8 +10,8 @@
 {"return": {}}
 {"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
-.
+...
 --
-Ran 21 tests
+Ran 23 tests
 
 OK
-- 
2.20.1

[RFC PATCH 0/2] Allow changing bs->file on reopen

[Alberto Garcia]

Hi,

during the past months we talked about making x-blockdev-reopen stable
API, and one of the missing things was having support for changing
bs->file. See here for the discusssion (I can't find the message from
Kashyap that started the thread in the web archives):

   https://lists.gnu.org/archive/html/qemu-block/2020-10/msg00922.html

I was testing this and one of the problems that I found was that
removing a filter node using this command is tricky because of the
permission system, see here for details:

   https://lists.gnu.org/archive/html/qemu-block/2020-12/msg00092.html

The good news is that Vladimir posted a set of patches that changes
the way that permissions are updated on reopen:

   https://lists.gnu.org/archive/html/qemu-block/2020-11/msg00745.html

I was testing if this would be useful to solve the problem that I
mentioned earlier and it seems to be the case so I wrote a patch to
add support for changing bs->file, along with a couple of test cases.

This is still an RFC but you can see the idea.

These patches apply on top of Vladimir's branch:

git: https://src.openvz.org/scm/~vsementsov/qemu.git
tag: up-block-topologic-perm-v2

Opinions are very welcome!

Berto

Alberto Garcia (2):
  block: Allow changing bs->file on reopen
  iotests: Update 245 to support replacing files with x-blockdev-reopen

 include/block/block.h  |  1 +
 block.c| 61 ++
 tests/qemu-iotests/245 | 61 +++---
 tests/qemu-iotests/245.out |  4 +--
 4 files changed, 121 insertions(+), 6 deletions(-)

-- 
2.20.1

Re: To start multiple KVM guests from one qcow2 image with transient disk option

[Peter Krempa]

On Thu, Jan 07, 2021 at 16:32:59 -0500, Masayoshi Mizuma wrote:
> On Thu, Jan 07, 2021 at 09:05:42AM +0100, Peter Krempa wrote:
> > On Tue, Jan 05, 2021 at 15:12:55 +0100, Peter Krempa wrote:
> > > On Mon, Jan 04, 2021 at 15:30:19 -0500, Masayoshi Mizuma wrote:
> > > > On Sat, Dec 19, 2020 at 11:30:39PM -0500, Masayoshi Mizuma wrote:

[...]

> Guest0 QMP:
> 
>   
> {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.TRANSIENT-Guest0","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}
> 
> Guest1 QMP:
> 
>   
> {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.TRANSIENT-Guest1","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}

Anyways, if you want, you can try implement this using the
frontend-hotplug approach for 'virtio' and 'scsi' disks, but I can't
guarantee that it will be accepted upstream in cases when the
implementation will turn out to be too hairy.

Re: [PATCH v6 08/11] iotests: add testenv.py

[Kevin Wolf]

Am 15.01.2021 um 13:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 15.01.2021 14:18, Kevin Wolf wrote:
> > Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > Add TestEnv class, which will handle test environment in a new python
> > > iotests running framework.
> > > 
> > > Difference with current ./check interface:
> > > - -v (verbose) option dropped, as it is unused
> > > 
> > > - -xdiff option is dropped, until somebody complains that it is needed
> > > - same for -n option
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy 
> > > ---
> > >   tests/qemu-iotests/testenv.py | 328 ++
> > >   1 file changed, 328 insertions(+)
> > >   create mode 100755 tests/qemu-iotests/testenv.py
> > > 
> > > diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> > > new file mode 100755
> > > index 00..ecaf76fb85
> > > --- /dev/null
> > > +++ b/tests/qemu-iotests/testenv.py
> > > @@ -0,0 +1,328 @@
> > > +#!/usr/bin/env python3
> > > +#
> > > +# Parse command line options to manage test environment variables.
> > > +#
> > > +# Copyright (c) 2020 Virtuozzo International GmbH
> > > +#
> > > +# This program is free software; you can redistribute it and/or modify
> > > +# it under the terms of the GNU General Public License as published by
> > > +# the Free Software Foundation; either version 2 of the License, or
> > > +# (at your option) any later version.
> > > +#
> > > +# This program is distributed in the hope that it will be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +# GNU General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU General Public License
> > > +# along with this program.  If not, see .
> > > +#
> > > +
> > > +import os
> > > +import sys
> > > +import tempfile
> > > +from pathlib import Path
> > > +import shutil
> > > +import collections
> > > +import subprocess
> > > +import argparse
> > > +from typing import List, Dict
> > > +
> > > +
> > > +def get_default_machine(qemu_prog: str) -> str:
> > > +outp = subprocess.run([qemu_prog, '-machine', 'help'], check=True,
> > > +  text=True, stdout=subprocess.PIPE).stdout
> > > +
> > > +machines = outp.split('\n')
> > > +default_machine = next(m for m in machines if m.endswith(' 
> > > (default)'))
> > > +default_machine = default_machine.split(' ', 1)[0]
> > > +
> > > +alias_suf = ' (alias of {})'.format(default_machine)
> > > +alias = next((m for m in machines if m.endswith(alias_suf)), None)
> > > +if alias is not None:
> > > +default_machine = alias.split(' ', 1)[0]
> > > +
> > > +return default_machine
> > > +
> > > +
> > > +class TestEnv:
> > > +"""
> > > +Manage system environment for running tests
> > > +
> > > +The following variables are supported/provided. They are represented 
> > > by
> > > +lower-cased TestEnv attributes.
> > > +"""
> > > +env_variables = ['PYTHONPATH', 'TEST_DIR', 'SOCK_DIR', 
> > > 'SAMPLE_IMG_DIR',
> > > + 'OUTPUT_DIR', 'PYTHON', 'QEMU_PROG', 
> > > 'QEMU_IMG_PROG',
> > > + 'QEMU_IO_PROG', 'QEMU_NBD_PROG', 'QSD_PROG',
> > > + 'SOCKET_SCM_HELPER', 'QEMU_OPTIONS', 
> > > 'QEMU_IMG_OPTIONS',
> > > + 'QEMU_IO_OPTIONS', 'QEMU_NBD_OPTIONS', 'IMGOPTS',
> > > + 'IMGFMT', 'IMGPROTO', 'AIOMODE', 'CACHEMODE',
> > > + 'VALGRIND_QEMU', 'CACHEMODE_IS_DEFAULT', 
> > > 'IMGFMT_GENERIC',
> > > + 'IMGOPTSSYNTAX', 'IMGKEYSECRET', 
> > > 'QEMU_DEFAULT_MACHINE']
> > > +
> > > +def get_env(self) -> Dict[str, str]:
> > > +env = {}
> > > +for v in self.env_variables:
> > > +val = getattr(self, v.lower(), None)
> > > +if val is not None:
> > > +env[v] = val
> > > +
> > > +return env
> > > +
> > > +_argparser = None
> > > +@classmethod
> > > +def get_argparser(cls) -> argparse.ArgumentParser:
> > > +if cls._argparser is not None:
> > > +return cls._argparser
> > > +
> > > +p = argparse.ArgumentParser(description="= test environment 
> > > options =",
> > > +add_help=False, 
> > > usage=argparse.SUPPRESS)
> > > +
> > > +p.add_argument('-d', dest='debug', action='store_true', 
> > > help='debug')
> > > +p.add_argument('-misalign', action='store_true',
> > > +   help='misalign memory allocations')
> > > +
> > > +p.set_defaults(imgfmt='raw', imgproto='file')
> > > +
> > > +format_list = ['raw', 'bochs', 'cloop', 'parallels', 'qcow', 
> > > 'qcow2',
> > > +   'qed', 'vdi', 'vpc', 'vhdx', 'vmdk', 'luks', 
> > > 'dmg']
> > > +g = 

Re: [PATCH v3 02/10] iotests/297: Rewrite in Python and extend reach

[Vladimir Sementsov-Ogievskiy]

15.01.2021 14:57, Max Reitz wrote:

On 15.01.21 12:13, Vladimir Sementsov-Ogievskiy wrote:

14.01.2021 20:02, Max Reitz wrote:

Instead of checking iotests.py only, check all Python files in the
qemu-iotests/ directory.  Of course, most of them do not pass, so there
is an extensive skip list for now.  (The only files that do pass are
209, 254, 283, and iotests.py.)

(Alternatively, we could have the opposite, i.e. an explicit list of
files that we do want to check, but I think it is better to check files
by default.)

Unless started in debug mode (./check -d), the output has no information
on which files are tested, so we will not have a problem e.g. with
backports, where some files may be missing when compared to upstream.

Besides the technical rewrite, some more things are changed:

- For the pylint invocation, PYTHONPATH is adjusted.  This mirrors
   setting MYPYPATH for mypy.

- Also, MYPYPATH is now derived from PYTHONPATH, so that we include
   paths set by the environment.  Maybe at some point we want to let the
   check script add '../../python/' to PYTHONPATH so that iotests.py does
   not need to do that.

- Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
   comments.  TODO is fine, we do not need 297 to complain about such
   comments.

- The "Success" line from mypy's output is suppressed, because (A) it
   does not add useful information, and (B) it would leak information
   about the files having been tested to the reference output, which we
   decidedly do not want.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/297 | 109 +
  tests/qemu-iotests/297.out |   5 +-
  2 files changed, 89 insertions(+), 25 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 5c5420712b..bfa26d280b 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/usr/bin/env python3
  #
  # Copyright (C) 2020 Red Hat, Inc.
  #
@@ -15,30 +15,95 @@
  # You should have received a copy of the GNU General Public License
  # along with this program.  If not, see .
-seq=$(basename $0)
-echo "QA output created by $seq"
+import os
+import re
+import shutil
+import subprocess
+import sys
-status=1    # failure is the default!
+import iotests
-# get standard environment
-. ./common.rc
-if ! type -p "pylint-3" > /dev/null; then
-    _notrun "pylint-3 not found"
-fi
-if ! type -p "mypy" > /dev/null; then
-    _notrun "mypy not found"
-fi
+# TODO: Empty this list!
+SKIP_FILES = (
+    '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
+    '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
+    '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
+    '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
+    '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
+    '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
+    '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
+    '299', '300', '302', '303', '304', '307',
+    'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
+)
-pylint-3 --score=n iotests.py
-MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \
-    --disallow-any-generics --disallow-incomplete-defs \
-    --disallow-untyped-decorators --no-implicit-optional \
-    --warn-redundant-casts --warn-unused-ignores \
-    --no-implicit-reexport iotests.py
+def is_python_file(filename):
+    if not os.path.isfile(filename):
+    return False
-# success, all done
-echo "*** done"
-rm -f $seq.full
-status=0
+    if filename.endswith('.py'):
+    return True
+
+    with open(filename) as f:
+    try:
+    first_line = f.readline()
+    return re.match('^#!.*python', first_line) is not None
+    except UnicodeDecodeError: # Ignore binary files


PEP8 asks for two spaces before inline comment


Wow.  I really hate PEP8.


Wow, it's unexpected :) I like it since first meet..




+    return False
+


and two empty lines here

(good ALE vim plugin runs flake8, mypy and pylint asynchronously for me and 
shows these things)


I’d like to argue that that isn’t good, but I need to quench my anger. Perhaps 
one day I can quench it sufficiently to install ALE myself.


+def run_linters():
+    files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
+ if is_python_file(filename)]
+
+    iotests.logger.debug('Files to be checked:')
+    iotests.logger.debug(', '.join(sorted(files)))


O, good.


+
+    print('=== pylint ===')
+    sys.stdout.flush()


Should not hurt.. But why? Should be flushed anyway as print will put a '\n'.. 
And why you care about it at all?


When pylint fails, I can see its result above the '=== pylint ===' line, even 
though its output is on stdout.  I suspect the Python 

Re: [PATCH 0/9] hw/block: m25p80: Fix the mess of dummy bytes needed for fast read commands

[Francisco Iglesias]

Hi Bin,

On [2021 Jan 15] Fri 10:07:52, Bin Meng wrote:
> Hi Francisco,
> 
> On Fri, Jan 15, 2021 at 2:13 AM Francisco Iglesias
>  wrote:
> >
> > Hi Bin,
> >
> > On [2021 Jan 14] Thu 23:08:53, Bin Meng wrote:
> > > From: Bin Meng 
> > >
> > > The m25p80 model uses s->needed_bytes to indicate how many follow-up
> > > bytes are expected to be received after it receives a command. For
> > > example, depending on the address mode, either 3-byte address or
> > > 4-byte address is needed.
> > >
> > > For fast read family commands, some dummy cycles are required after
> > > sending the address bytes, and the dummy cycles need to be counted
> > > in s->needed_bytes. This is where the mess began.
> > >
> > > As the variable name (needed_bytes) indicates, the unit is in byte.
> > > It is not in bit, or cycle. However for some reason the model has
> > > been using the number of dummy cycles for s->needed_bytes. The right
> > > approach is to convert the number of dummy cycles to bytes based on
> > > the SPI protocol, for example, 6 dummy cycles for the Fast Read Quad
> > > I/O (EBh) should be converted to 3 bytes per the formula (6 * 4 / 8).
> >
> > While not being the original implementor I must assume that above solution 
> > was
> > considered but not chosen by the developers due to it is inaccuracy (it
> > wouldn't be possible to model exacly 6 dummy cycles, only a multiple of 8,
> > meaning that if the controller is wrongly programmed to generate 7 the error
> > wouldn't be caught and the controller will still be considered "correct"). 
> > Now
> > that we have this detail in the implementation I'm in favor of keeping it, 
> > this
> > also because the detail is already in use for catching exactly above error.
> >
> 
> I found no clue from the commit message that my proposed solution here
> was ever considered, otherwise all SPI controller models supporting
> software generation should have been found out seriously broken long
> time ago!


The controllers you are referring to might lack support for commands requiring
dummy clock cycles but I really hope they work with the other commands? If so I
don't think it is fair to call them 'seriously broken' (and else we should
probably let the maintainers know about it). Most likely the lack of support
for the commands is because no request has been made for them. Also there is
one controller that has support.


> 
> The issue you pointed out that we require the total number of dummy
> bits should be multiple of 8 is true, that's why I added the
> unimplemented log message in this series (patch 2/3/4) to warn users
> if this expectation is not met. However this will not cause any issue
> when running U-Boot or Linux, because both spi-nor drivers expect the
> same assumption as we do here.
> 
> See U-Boot spi_nor_read_data() and Linux spi_nor_spimem_read_data(),
> there is a logic to calculate the dummy bytes needed for fast read
> command:
> 
> /* convert the dummy cycles to the number of bytes */
> op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
> 
> Note the default dummy cycles configuration for all flashes I have
> looked into as of today, meets the multiple of 8 assumption. On some
> flashes the dummy cycle number is configurable, and if it's been
> configured to be an odd value, it would not work on U-Boot/Linux in
> the first place.
> 
> > >
> > > Things get complicated when interacting with different SPI or QSPI
> > > flash controllers. There are major two cases:
> > >
> > > - Dummy bytes prepared by drivers, and wrote to the controller fifo.
> > >   For such case, driver will calculate the correct number of dummy
> > >   bytes and write them into the tx fifo. Fixing the m25p80 model will
> > >   fix flashes working with such controllers.
> >
> > Above can be fixed while still keeping the detailed dummy cycle 
> > implementation
> > inside m25p80. Perhaps one of the following could be looked into: 
> > configurating
> > the amount, letting the spi ctrl fetch the amount from m25p80 or by 
> > inheriting
> > some functionality handling this in the SPI controller. Or a mixture of 
> > above.
> 
> Please send patches to explain this in detail how this is going to
> work. I am open to all possible solutions.

In that case I suggest that you instead try with a device property
'model_dummy_bytes' used to select to convert the accurate dummy clock cycle
count to dummy bytes inside m25p80. Below is an example on how to modify the
decode_fast_read_cmd function (the other commands requiring dummy clock cycles
can follow a similar pattern). This way the fifo mode will be able to work the
way you desire while also keeping the current functionality intact. Suddenly
removing functionality (features) will take users by surprise. 


static void decode_fast_read_cmd(Flash *s)
{
uint8_t dummy_clk_cycles = 0;
uint8_t extra_bytes;

s->needed_bytes = get_addr_length(s);

/* Obtain the number of dummy clock cycles needed */
switch 

[PATCH] hw/block/nvme: error if drive less than a zone size

[Minwoo Im]

If a user gives backing device file less than a single zone size, the
namespace capacity will be reported to 0 and the kerenl will fail to
allocate namespace silently.

This patch errors in case that num_zones are 0 which is backing device
is less than a single zone size.

Signed-off-by: Minwoo Im 
---
 hw/block/nvme-ns.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 274eaf61b721..98690d030379 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -137,6 +137,13 @@ static int nvme_ns_zoned_check_calc_geometry(NvmeNamespace 
*ns, Error **errp)
 ns->num_zones = ns->size / lbasz / ns->zone_size;
 
 /* Do a few more sanity checks of ZNS properties */
+if (!ns->num_zones) {
+error_setg(errp,
+   "num_zone is 0, drive must be larger than a zone %luB",
+   zone_size);
+return -1;
+}
+
 if (ns->params.max_open_zones > ns->num_zones) {
 error_setg(errp,
"max_open_zones value %u exceeds the number of zones %u",
-- 
2.17.1

Re: [PATCH v6 08/11] iotests: add testenv.py

[Vladimir Sementsov-Ogievskiy]

15.01.2021 14:18, Kevin Wolf wrote:

Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben:

Add TestEnv class, which will handle test environment in a new python
iotests running framework.

Difference with current ./check interface:
- -v (verbose) option dropped, as it is unused

- -xdiff option is dropped, until somebody complains that it is needed
- same for -n option

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  tests/qemu-iotests/testenv.py | 328 ++
  1 file changed, 328 insertions(+)
  create mode 100755 tests/qemu-iotests/testenv.py

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
new file mode 100755
index 00..ecaf76fb85
--- /dev/null
+++ b/tests/qemu-iotests/testenv.py
@@ -0,0 +1,328 @@
+#!/usr/bin/env python3
+#
+# Parse command line options to manage test environment variables.
+#
+# Copyright (c) 2020 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import sys
+import tempfile
+from pathlib import Path
+import shutil
+import collections
+import subprocess
+import argparse
+from typing import List, Dict
+
+
+def get_default_machine(qemu_prog: str) -> str:
+outp = subprocess.run([qemu_prog, '-machine', 'help'], check=True,
+  text=True, stdout=subprocess.PIPE).stdout
+
+machines = outp.split('\n')
+default_machine = next(m for m in machines if m.endswith(' (default)'))
+default_machine = default_machine.split(' ', 1)[0]
+
+alias_suf = ' (alias of {})'.format(default_machine)
+alias = next((m for m in machines if m.endswith(alias_suf)), None)
+if alias is not None:
+default_machine = alias.split(' ', 1)[0]
+
+return default_machine
+
+
+class TestEnv:
+"""
+Manage system environment for running tests
+
+The following variables are supported/provided. They are represented by
+lower-cased TestEnv attributes.
+"""
+env_variables = ['PYTHONPATH', 'TEST_DIR', 'SOCK_DIR', 'SAMPLE_IMG_DIR',
+ 'OUTPUT_DIR', 'PYTHON', 'QEMU_PROG', 'QEMU_IMG_PROG',
+ 'QEMU_IO_PROG', 'QEMU_NBD_PROG', 'QSD_PROG',
+ 'SOCKET_SCM_HELPER', 'QEMU_OPTIONS', 'QEMU_IMG_OPTIONS',
+ 'QEMU_IO_OPTIONS', 'QEMU_NBD_OPTIONS', 'IMGOPTS',
+ 'IMGFMT', 'IMGPROTO', 'AIOMODE', 'CACHEMODE',
+ 'VALGRIND_QEMU', 'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC',
+ 'IMGOPTSSYNTAX', 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE']
+
+def get_env(self) -> Dict[str, str]:
+env = {}
+for v in self.env_variables:
+val = getattr(self, v.lower(), None)
+if val is not None:
+env[v] = val
+
+return env
+
+_argparser = None
+@classmethod
+def get_argparser(cls) -> argparse.ArgumentParser:
+if cls._argparser is not None:
+return cls._argparser
+
+p = argparse.ArgumentParser(description="= test environment options =",
+add_help=False, usage=argparse.SUPPRESS)
+
+p.add_argument('-d', dest='debug', action='store_true', help='debug')
+p.add_argument('-misalign', action='store_true',
+   help='misalign memory allocations')
+
+p.set_defaults(imgfmt='raw', imgproto='file')
+
+format_list = ['raw', 'bochs', 'cloop', 'parallels', 'qcow', 'qcow2',
+   'qed', 'vdi', 'vpc', 'vhdx', 'vmdk', 'luks', 'dmg']
+g = p.add_argument_group(
+'image format options',
+'The following options sets IMGFMT environment variable. '


s/sets/set the/


+'At most one chose is allowed, default is "raw"')


s/chose/choice/


+g = g.add_mutually_exclusive_group()
+for fmt in format_list:
+g.add_argument('-' + fmt, dest='imgfmt', action='store_const',
+   const=fmt)
+
+protocol_list = ['file', 'rbd', 'sheepdoc', 'nbd', 'ssh', 'nfs',
+ 'fuse']
+g = p.add_argument_group(
+'image protocol options',
+'The following options sets IMGPROTO environment variably. '
+'At most one chose is allowed, default is "file"')


Same as above, but also s/variably/variable/.

Do we consider these environment 

[RFC PATCH 3/5] hw/block/nvme: add multi-controller param for mpath

[Minwoo Im]

Added mpath.ctrl parameter to set multi-controller bit[1] in CMIC field
in Identify Controller data structure.  It will indicate that a NVM
subsystem can have two or more controllers in the subsystem.

To set up multi-controller in a NVM subsystem, user needs to give same
serial parameter to the controllers (-device), but different cntlid
parameter to each controllers (-device).

Example:

  -device nvme,ctrlid=0,serial=foo,...
  -device nvme,ctrlid=1,serial=foo,...

The example above prepares two different controllers in a NVM subsystem
with the same serial which leads to same subsystem NQN.

Signed-off-by: Minwoo Im 
---
 hw/block/nvme.c | 10 ++
 hw/block/nvme.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 132e61c0ee7b..50b349cf9ea3 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -57,6 +57,11 @@
  *   This value will be reported through Identify Controller data structure
  *   with a field named CNTLID[79:78].
  *
+ * - `mpath.ctrl`
+ *   Multi-path I/O with multi-controller (default: false). A NVM subsystem
+ *   can hold two or more controllers.  This will be reflected to Identify
+ *   Controller data structure CMIC[76] field.
+ *
  * - `zoned.append_size_limit`
  *   The maximum I/O size in bytes that is allowed in Zone Append command.
  *   The default is 128KiB. Since internally this this value is maintained as
@@ -4284,6 +4289,10 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice 
*pci_dev)
 n->bar.intmc = n->bar.intms = 0;
 
 id->cntlid = n->params.cntlid;
+
+if (n->params.mpath_ctrl) {
+id->cmic |= NVME_CMIC_MULTI_CTRL;
+}
 }
 
 static void nvme_realize(PCIDevice *pci_dev, Error **errp)
@@ -4364,6 +4373,7 @@ static Property nvme_props[] = {
 DEFINE_PROP_UINT32("aer_max_queued", NvmeCtrl, params.aer_max_queued, 64),
 DEFINE_PROP_UINT8("mdts", NvmeCtrl, params.mdts, 7),
 DEFINE_PROP_BOOL("use-intel-id", NvmeCtrl, params.use_intel_id, false),
+DEFINE_PROP_BOOL("mpath.ctrl", NvmeCtrl, params.mpath_ctrl, false),
 DEFINE_PROP_SIZE32("zoned.append_size_limit", NvmeCtrl, params.zasl_bs,
NVME_DEFAULT_MAX_ZA_SIZE),
 DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 6aa9e89ac5a8..73c9c2cff247 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -21,6 +21,7 @@ typedef struct NvmeParams {
 uint8_t  mdts;
 bool use_intel_id;
 uint32_t zasl_bs;
+bool mpath_ctrl;
 } NvmeParams;
 
 typedef struct NvmeAsyncEvent {
-- 
2.17.1

[RFC PATCH 1/5] hw/block/nvme: add controller id parameter

[Minwoo Im]

There is Contrller ID field in Identify Controller data structure and
nvme device has never set this field, 0 by default.

Added a parameter for controller identifier in a NVM subsystem.  This is
reflected to Identify Controller data structrue of the controller.  This
parameter is helpful when a user wants to set up multi-controller in a
NVM subsystem.

Signed-off-by: Minwoo Im 
---
 hw/block/nvme.c | 10 ++
 hw/block/nvme.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index cf0fe28fe6eb..132e61c0ee7b 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -23,6 +23,7 @@
  *  max_ioqpairs=, \
  *  aerl=, aer_max_queued=, \
  *  mdts=,zoned.append_size_limit= \
+ *  cntlid=
  *  -device nvme-ns,drive=,bus=,nsid=,\
  *  zoned=
  *
@@ -50,6 +51,12 @@
  *   completion when there are no outstanding AERs. When the maximum number of
  *   enqueued events are reached, subsequent events will be dropped.
  *
+ * - `cntlid`
+ *   NVM subsystem unique controller identifier (default: 0).  This property
+ *   is used if a user wants to set up multi-controller in a NVM subsystem.
+ *   This value will be reported through Identify Controller data structure
+ *   with a field named CNTLID[79:78].
+ *
  * - `zoned.append_size_limit`
  *   The maximum I/O size in bytes that is allowed in Zone Append command.
  *   The default is 128KiB. Since internally this this value is maintained as
@@ -4275,6 +4282,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice 
*pci_dev)
 
 n->bar.vs = NVME_SPEC_VER;
 n->bar.intmc = n->bar.intms = 0;
+
+id->cntlid = n->params.cntlid;
 }
 
 static void nvme_realize(PCIDevice *pci_dev, Error **errp)
@@ -4345,6 +4354,7 @@ static Property nvme_props[] = {
 DEFINE_BLOCK_PROPERTIES(NvmeCtrl, namespace.blkconf),
 DEFINE_PROP_LINK("pmrdev", NvmeCtrl, pmrdev, TYPE_MEMORY_BACKEND,
  HostMemoryBackend *),
+DEFINE_PROP_UINT32("cntlid", NvmeCtrl, params.cntlid, 0),
 DEFINE_PROP_STRING("serial", NvmeCtrl, params.serial),
 DEFINE_PROP_UINT32("cmb_size_mb", NvmeCtrl, params.cmb_size_mb, 0),
 DEFINE_PROP_UINT32("num_queues", NvmeCtrl, params.num_queues, 0),
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index b7fbcca39d9f..6aa9e89ac5a8 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -11,6 +11,7 @@
 
 typedef struct NvmeParams {
 char *serial;
+uint32_t cntlid;
 uint32_t num_queues; /* deprecated since 5.1 */
 uint32_t max_ioqpairs;
 uint16_t msix_qsize;
-- 
2.17.1

[RFC PATCH 2/5] nvme: add CMIC enum value for Identify Controller

[Minwoo Im]

Added Controller Multi-path I/O and Namespace Sharing Capabilities
(CMIC) field to support multi-controller in the following patches.

This field is in Identify Controller data structure in [76].

Signed-off-by: Minwoo Im 
---
 include/block/nvme.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index 9494246f1f59..e83ec1e124c6 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -941,6 +941,10 @@ enum NvmeIdCtrlLpa {
 NVME_LPA_EXTENDED = 1 << 2,
 };
 
+enum NvmeIdCtrlCmic {
+NVME_CMIC_MULTI_CTRL= 1 << 1,
+};
+
 #define NVME_CTRL_SQES_MIN(sqes) ((sqes) & 0xf)
 #define NVME_CTRL_SQES_MAX(sqes) (((sqes) >> 4) & 0xf)
 #define NVME_CTRL_CQES_MIN(cqes) ((cqes) & 0xf)
-- 
2.17.1

[RFC PATCH 5/5] hw/block/nvme: add namespace sharing param for mpath

[Minwoo Im]

Added mpath.ns parameter to set multi-namespace bit[0] in NMIC field in
Identify Namespace data structure.  It will indicate that the namespace
can be shared by two or more controllers.

Example:

  To share the namespace between two controllers in a NVM subsystem,
first multi-controllers should be prepared with the same serial:

  -device nvme,id=nvme0,ctrlid=0,serial=foo,...
  -device nvme,id=nvme1,ctrlid=1,serial=foo,...

Then, we can prepare namespace device with mpath.ns enabled.  Also, we
must give the same UUID for those two devices with the same Namespace
ID.

  -device nvme-ns,uuid=,bus=nvme0,nsid=1,...
  -device nvme-ns,uuid=,bus=nvme1,nsid=1,...

Signed-off-by: Minwoo Im 
---
 hw/block/nvme-ns.c | 14 --
 hw/block/nvme-ns.h |  2 ++
 hw/block/nvme.c|  6 ++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
index 274eaf61b721..cedacda9ebab 100644
--- a/hw/block/nvme-ns.c
+++ b/hw/block/nvme-ns.c
@@ -32,13 +32,18 @@
 
 #define MIN_DISCARD_GRANULARITY (4 * KiB)
 
-static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
+static int nvme_ns_init(NvmeCtrl *n, NvmeNamespace *ns, Error **errp)
 {
 BlockDriverInfo bdi;
 NvmeIdNs *id_ns = >id_ns;
 int lba_index = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
 int npdg;
 
+if (!n->params.mpath_ctrl && ns->params.mpath_ns) {
+error_setg(errp, "mpath.ctrl must be enabled for ns sharing");
+return -1;
+}
+
 ns->id_ns.dlfeat = 0x9;
 
 id_ns->lbaf[lba_index].ds = 31 - clz32(ns->blkconf.logical_block_size);
@@ -63,6 +68,10 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
 
 id_ns->npda = id_ns->npdg = npdg - 1;
 
+if (ns->params.mpath_ns) {
+id_ns->nmic |= NVME_NMIC_NS_SHARED;
+}
+
 return 0;
 }
 
@@ -314,7 +323,7 @@ int nvme_ns_setup(NvmeCtrl *n, NvmeNamespace *ns, Error 
**errp)
 return -1;
 }
 
-if (nvme_ns_init(ns, errp)) {
+if (nvme_ns_init(n, ns, errp)) {
 return -1;
 }
 if (ns->params.zoned) {
@@ -371,6 +380,7 @@ static Property nvme_ns_props[] = {
 DEFINE_BLOCK_PROPERTIES(NvmeNamespace, blkconf),
 DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0),
 DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid),
+DEFINE_PROP_BOOL("mpath.ns", NvmeNamespace, params.mpath_ns, false),
 DEFINE_PROP_BOOL("zoned", NvmeNamespace, params.zoned, false),
 DEFINE_PROP_SIZE("zoned.zone_size", NvmeNamespace, params.zone_size_bs,
  NVME_DEFAULT_ZONE_SIZE),
diff --git a/hw/block/nvme-ns.h b/hw/block/nvme-ns.h
index f8f3c28c360b..d1f2518f7210 100644
--- a/hw/block/nvme-ns.h
+++ b/hw/block/nvme-ns.h
@@ -29,6 +29,8 @@ typedef struct NvmeNamespaceParams {
 uint32_t nsid;
 QemuUUID uuid;
 
+bool mpath_ns;
+
 bool zoned;
 bool cross_zone_read;
 uint64_t zone_size_bs;
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 50b349cf9ea3..e4aa15345c12 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -62,6 +62,12 @@
  *   can hold two or more controllers.  This will be reflected to Identify
  *   Controller data structure CMIC[76] field.
  *
+ * Setting `mpath.ctrl` to true enables the following properties to configure
+ * multi-path for a namespace:
+ * mpath.ns=
+ * If set to true, namespace sharing in a NVM subsystem is enabled.
+ * This will be reflected to Identify Namespace data structure.
+ *
  * - `zoned.append_size_limit`
  *   The maximum I/O size in bytes that is allowed in Zone Append command.
  *   The default is 128KiB. Since internally this this value is maintained as
-- 
2.17.1

[RFC PATCH 0/5] hw/block/nvme: support multi-path for ctrl/ns

[Minwoo Im]

Hello,

This series added support for multi-path I/O with multi-controllers and
namespace sharing.  By supporting these features, we can test Linux
kernel mpath(multi-path) code with this NVMe device.

Patches from the first to third added multi-controller support in a NVM
subsystem by adding a mpath.ctrl parameter to nvme device.  The rest of
the patches added namespace sharing support in a NVM subsystem with two
or more controllers by adding mpath.ns parameter to nvme-ns device.

Multi-path enabled in kernel with this series for two controllers with a
namespace:

  root@vm:~/work# nvme list -v
  NVM Express Subsystems

  SubsystemSubsystem-NQN
Controllers
   

 
  nvme-subsys0 nqn.2019-08.org.qemu:serial  
nvme0, nvme1

  NVM Express Controllers

  Device   SN   MN   FR 
  TxPort AddressSubsystemNamespaces
     
 -- --  
  nvme0serial   QEMU NVMe Ctrl   1.0
  pcie   :01:00.0   nvme-subsys0 nvme0n1
  nvme1serial   QEMU NVMe Ctrl   1.0
  pcie   :02:00.0   nvme-subsys0 nvme0n1

  NVM Express Namespaces

  Device   NSID Usage  Format   Controllers
    --  

  nvme0n1  1268.44  MB / 268.44  MB512   B +  0 B   nvme0, nvme1

The reason why I put 'RFC' tag to this series is mostly about the last
patch "hw/block/nvme: add namespace sharing param for mpath".  It seems
like QEMU block backing device does not support to be shared among two
or more -device(s).  It means that we just can't give same drive=
property to multiple nvme-ns devices.  This patch has just let -device
maps to -drive one-to-one(1:1), but if namespae sharing is detected and
setup by the host kernel, then a single block device will be selected
for the NVM subsystem.  I'm not sure this is a good start for this
feature, so I put the RFC tag here.

Please kindly review!

Thanks,

Minwoo Im (5):
  hw/block/nvme: add controller id parameter
  nvme: add CMIC enum value for Identify Controller
  hw/block/nvme: add multi-controller param for mpath
  nvme: add NMIC enum value for Identify Namespace
  hw/block/nvme: add namespace sharing param for mpath

 hw/block/nvme-ns.c   | 14 --
 hw/block/nvme-ns.h   |  2 ++
 hw/block/nvme.c  | 26 ++
 hw/block/nvme.h  |  2 ++
 include/block/nvme.h |  8 
 5 files changed, 50 insertions(+), 2 deletions(-)

-- 
2.17.1

[RFC PATCH 4/5] nvme: add NMIC enum value for Identify Namespace

[Minwoo Im]

Added Namespace Multi-path I/O and Namespace Sharing Capabilities (NMIC)
field to support shared namespace from controller(s).

This field is in Identify Namespace data structure in [30].

Signed-off-by: Minwoo Im 
---
 include/block/nvme.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/include/block/nvme.h b/include/block/nvme.h
index e83ec1e124c6..dd7b5ba9ef4c 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -1110,6 +1110,10 @@ enum NvmeNsIdentifierType {
 NVME_NIDT_CSI   = 0x04,
 };
 
+enum NvmeNsNmic {
+NVME_NMIC_NS_SHARED = 1 << 0,
+};
+
 enum NvmeCsi {
 NVME_CSI_NVM= 0x00,
 NVME_CSI_ZONED  = 0x02,
-- 
2.17.1

Re: [PATCH v3 11/10] iotests: add flake8 linter

[Max Reitz]

On 15.01.21 12:53, Vladimir Sementsov-Ogievskiy wrote:

pylint is good, but doesn't cover the PEP8. Let's add flake8, to be
sure that our code sutisfy PEP8. Add new linter and fix some code style
in checked files.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi!

Here is my small addition to Max's series, hope you like it!

Note, that this is not the first occurrence of 'flake8' in Qemu:

 # git grep flake8
 python/qemu/.flake8:[flake8]
 scripts/qapi/.flake8:[flake8]
 scripts/qapi/.flake8:extend-ignore = E722  # Prefer pylint's bare-except 
checks to flake8's


  tests/qemu-iotests/129|  6 ++-
  tests/qemu-iotests/254|  2 +-
  tests/qemu-iotests/297| 21 ++---
  tests/qemu-iotests/297.out|  1 +
  tests/qemu-iotests/300|  4 +-
  tests/qemu-iotests/iotests.py | 88 +--
  6 files changed, 106 insertions(+), 16 deletions(-)


Looks reasonable to me, but perhaps it should just be a dedicated 
series.  I think there’s enough in here to justify that.



diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 201d9e0a0b..28ec1d 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -21,6 +21,7 @@
  import os
  import iotests
  
+

  class TestStopWithBlockJob(iotests.QMPTestCase):
  test_img = os.path.join(iotests.test_dir, 'test.img')
  target_img = os.path.join(iotests.test_dir, 'target.img')
@@ -39,8 +40,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
  source_drive = 'driver=throttle,' \
 'node-name=source,' \
 'throttle-group=tg0,' \
-  f'file.driver={iotests.imgfmt},' \
-  f'file.file.filename={self.test_img}'
+   f'file.driver={iotests.imgfmt},' \
+   f'file.file.filename={self.test_img}'


Interesting, when indenting this, I was wondering whether pylint would 
complain.  I was so glad it didn’t.  I really don’t like PEP8.


(Though I understand that style guides like PEP8 are there specifically 
so when someone like me goes “but I like this style better :(”, everyone 
else can say “but you’re objectively wrong”.  So me hating it kind of is 
its point, I guess.)


Max

Re: [PATCH v3 10/10] iotests/300: Clean up pylint and mypy complaints

[Max Reitz]

On 15.01.21 12:30, Vladimir Sementsov-Ogievskiy wrote:

14.01.2021 20:03, Max Reitz wrote:

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/297 |  2 +-
  tests/qemu-iotests/300 | 18 +++---
  2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 1dce1d1b1c..03d8604538 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -33,7 +33,7 @@ SKIP_FILES = (
  '218', '219', '222', '224', '228', '234', '235', '236', '237', 
'238',
  '240', '242', '245', '246', '248', '255', '256', '257', '258', 
'260',
  '262', '264', '266', '274', '277', '280', '281', '295', '296', 
'298',

-    '299', '300', '302', '303', '304', '307',
+    '299', '302', '303', '304', '307',



  'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
  )
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index b864a21d5e..64d388a8fa 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -22,7 +22,11 @@ import os
  import random
  import re
  from typing import Dict, List, Optional, Union
+
  import iotests
+
+# Import qemu after iotests.py has amended the PYTHONPATH


you mean os.path, exactly,

  sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 
'python'))


yes?


Indeed.


+# pylint: disable=wrong-import-order
  import qemu
  BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]
@@ -110,10 +114,14 @@ class 
TestDirtyBitmapMigration(iotests.QMPTestCase):

  If @msg is None, check that there has not been any error.
  """
  self.vm_b.shutdown()
+
+    log = self.vm_b.get_log()
+    assert log is not None # Loaded after shutdown
+
  if msg is None:
-    self.assertNotIn('qemu-system-', self.vm_b.get_log())
+    self.assertNotIn('qemu-system-', log)
  else:
-    self.assertIn(msg, self.vm_b.get_log())
+    self.assertIn(msg, log)
  @staticmethod
  def mapping(node_name: str, node_alias: str,
@@ -445,9 +453,13 @@ class 
TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):

  # Check for the error in the source's log
  self.vm_a.shutdown()
+
+    log = self.vm_a.get_log()
+    assert log is not None # Loaded after shutdown
+
  self.assertIn(f"Cannot migrate bitmap '{name}' on node "
    f"'{self.src_node_name}': Name is longer than 
255 bytes",

-  self.vm_a.get_log())
+  log)
  # Expect abnormal shutdown of the destination VM because of
  # the failed migration



Your new comments are the only PEP8 complains in the 300 iotest:

flake8 300
300:119:31: E261 at least two spaces before inline comment
300:458:31: E261 at least two spaces before inline comment

So, I'd fix them.


Haha, sure :)


anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy 


Thanks!

Re: [PATCH v3 06/10] iotests/129: Use throttle node

[Max Reitz]

On 15.01.21 12:16, Vladimir Sementsov-Ogievskiy wrote:

14.01.2021 20:03, Max Reitz wrote:

Throttling on the BB has not affected block jobs in a while, so it is
possible that one of the jobs in 129 finishes before the VM is stopped.
We can fix that by running the job from a throttle node.

Signed-off-by: Max Reitz
Reviewed-by: Eric Blake


you forget my :)
Reviewed-by: Vladimir Sementsov-Ogievskiy 


Sorry O:)

Re: [PATCH v3 02/10] iotests/297: Rewrite in Python and extend reach

[Max Reitz]

On 15.01.21 12:13, Vladimir Sementsov-Ogievskiy wrote:

14.01.2021 20:02, Max Reitz wrote:

Instead of checking iotests.py only, check all Python files in the
qemu-iotests/ directory.  Of course, most of them do not pass, so there
is an extensive skip list for now.  (The only files that do pass are
209, 254, 283, and iotests.py.)

(Alternatively, we could have the opposite, i.e. an explicit list of
files that we do want to check, but I think it is better to check files
by default.)

Unless started in debug mode (./check -d), the output has no information
on which files are tested, so we will not have a problem e.g. with
backports, where some files may be missing when compared to upstream.

Besides the technical rewrite, some more things are changed:

- For the pylint invocation, PYTHONPATH is adjusted.  This mirrors
   setting MYPYPATH for mypy.

- Also, MYPYPATH is now derived from PYTHONPATH, so that we include
   paths set by the environment.  Maybe at some point we want to let the
   check script add '../../python/' to PYTHONPATH so that iotests.py does
   not need to do that.

- Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
   comments.  TODO is fine, we do not need 297 to complain about such
   comments.

- The "Success" line from mypy's output is suppressed, because (A) it
   does not add useful information, and (B) it would leak information
   about the files having been tested to the reference output, which we
   decidedly do not want.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/297 | 109 +
  tests/qemu-iotests/297.out |   5 +-
  2 files changed, 89 insertions(+), 25 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 5c5420712b..bfa26d280b 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/usr/bin/env python3
  #
  # Copyright (C) 2020 Red Hat, Inc.
  #
@@ -15,30 +15,95 @@
  # You should have received a copy of the GNU General Public License
  # along with this program.  If not, see .
-seq=$(basename $0)
-echo "QA output created by $seq"
+import os
+import re
+import shutil
+import subprocess
+import sys
-status=1    # failure is the default!
+import iotests
-# get standard environment
-. ./common.rc
-if ! type -p "pylint-3" > /dev/null; then
-    _notrun "pylint-3 not found"
-fi
-if ! type -p "mypy" > /dev/null; then
-    _notrun "mypy not found"
-fi
+# TODO: Empty this list!
+SKIP_FILES = (
+    '030', '040', '041', '044', '045', '055', '056', '057', '065', 
'093',
+    '096', '118', '124', '129', '132', '136', '139', '147', '148', 
'149',
+    '151', '152', '155', '163', '165', '169', '194', '196', '199', 
'202',
+    '203', '205', '206', '207', '208', '210', '211', '212', '213', 
'216',
+    '218', '219', '222', '224', '228', '234', '235', '236', '237', 
'238',
+    '240', '242', '245', '246', '248', '255', '256', '257', '258', 
'260',
+    '262', '264', '266', '274', '277', '280', '281', '295', '296', 
'298',

+    '299', '300', '302', '303', '304', '307',
+    'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
+)
-pylint-3 --score=n iotests.py
-MYPYPATH=../../python/ mypy --warn-unused-configs 
--disallow-subclassing-any \

-    --disallow-any-generics --disallow-incomplete-defs \
-    --disallow-untyped-decorators --no-implicit-optional \
-    --warn-redundant-casts --warn-unused-ignores \
-    --no-implicit-reexport iotests.py
+def is_python_file(filename):
+    if not os.path.isfile(filename):
+    return False
-# success, all done
-echo "*** done"
-rm -f $seq.full
-status=0
+    if filename.endswith('.py'):
+    return True
+
+    with open(filename) as f:
+    try:
+    first_line = f.readline()
+    return re.match('^#!.*python', first_line) is not None
+    except UnicodeDecodeError: # Ignore binary files


PEP8 asks for two spaces before inline comment


Wow.  I really hate PEP8.


+    return False
+


and two empty lines here

(good ALE vim plugin runs flake8, mypy and pylint asynchronously for me 
and shows these things)


I’d like to argue that that isn’t good, but I need to quench my anger. 
Perhaps one day I can quench it sufficiently to install ALE myself.



+def run_linters():
+    files = [filename for filename in (set(os.listdir('.')) - 
set(SKIP_FILES))

+ if is_python_file(filename)]
+
+    iotests.logger.debug('Files to be checked:')
+    iotests.logger.debug(', '.join(sorted(files)))


O, good.


+
+    print('=== pylint ===')
+    sys.stdout.flush()


Should not hurt.. But why? Should be flushed anyway as print will put a 
'\n'.. And why you care about it at all?


When pylint fails, I can see its result above the '=== pylint ===' line, 
even though its output is on stdout.  I suspect the Python output indeed 
isn’t flushed on \n.


(Test it by dropping the flush(), and then also 

[PATCH v3 11/10] iotests: add flake8 linter

[Vladimir Sementsov-Ogievskiy]

pylint is good, but doesn't cover the PEP8. Let's add flake8, to be
sure that our code sutisfy PEP8. Add new linter and fix some code style
in checked files.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

Hi!

Here is my small addition to Max's series, hope you like it!

Note, that this is not the first occurrence of 'flake8' in Qemu:

# git grep flake8
python/qemu/.flake8:[flake8]
scripts/qapi/.flake8:[flake8]
scripts/qapi/.flake8:extend-ignore = E722  # Prefer pylint's bare-except 
checks to flake8's


 tests/qemu-iotests/129|  6 ++-
 tests/qemu-iotests/254|  2 +-
 tests/qemu-iotests/297| 21 ++---
 tests/qemu-iotests/297.out|  1 +
 tests/qemu-iotests/300|  4 +-
 tests/qemu-iotests/iotests.py | 88 +--
 6 files changed, 106 insertions(+), 16 deletions(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 201d9e0a0b..28ec1d 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -21,6 +21,7 @@
 import os
 import iotests
 
+
 class TestStopWithBlockJob(iotests.QMPTestCase):
 test_img = os.path.join(iotests.test_dir, 'test.img')
 target_img = os.path.join(iotests.test_dir, 'target.img')
@@ -39,8 +40,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 source_drive = 'driver=throttle,' \
'node-name=source,' \
'throttle-group=tg0,' \
-  f'file.driver={iotests.imgfmt},' \
-  f'file.file.filename={self.test_img}'
+   f'file.driver={iotests.imgfmt},' \
+   f'file.file.filename={self.test_img}'
 
 self.vm.add_drive(None, source_drive)
 self.vm.launch()
@@ -97,6 +98,7 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
 
 self.do_test_stop('block-commit', device='drive0', top_node='source')
 
+
 if __name__ == '__main__':
 iotests.main(supported_fmts=["qcow2"],
  supported_protocols=["file"])
diff --git a/tests/qemu-iotests/254 b/tests/qemu-iotests/254
index 150e58be8e..798f316e36 100755
--- a/tests/qemu-iotests/254
+++ b/tests/qemu-iotests/254
@@ -74,7 +74,7 @@ log("query-block: device = {}, node-name = {}, 
dirty-bitmaps:".format(
 result['device'], result['inserted']['node-name']))
 log(result['dirty-bitmaps'], indent=2)
 log("\nbitmaps in backing image:")
-log(result['inserted']['image']['backing-image']['format-specific'] \
+log(result['inserted']['image']['backing-image']['format-specific']
 ['data']['bitmaps'], indent=2)
 
 vm.shutdown()
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 03d8604538..b79c341a3c 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -49,9 +49,10 @@ def is_python_file(filename):
 try:
 first_line = f.readline()
 return re.match('^#!.*python', first_line) is not None
-except UnicodeDecodeError: # Ignore binary files
+except UnicodeDecodeError:  # Ignore binary files
 return False
 
+
 def run_linters():
 files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
  if is_python_file(filename)]
@@ -59,16 +60,22 @@ def run_linters():
 iotests.logger.debug('Files to be checked:')
 iotests.logger.debug(', '.join(sorted(files)))
 
-print('=== pylint ===')
-sys.stdout.flush()
-
-# Todo notes are fine, but fixme's or xxx's should probably just be
-# fixed (in tests, at least)
 env = os.environ
 try:
 env['PYTHONPATH'] += ':../../python/'
 except KeyError:
 env['PYTHONPATH'] = '../../python/'
+
+print('=== flake8 ===')
+sys.stdout.flush()
+
+subprocess.run(('flake8', *files), env=env, check=False)
+
+print('=== pylint ===')
+sys.stdout.flush()
+
+# Todo notes are fine, but fixme's or xxx's should probably just be
+# fixed (in tests, at least)
 subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
env=env, check=False)
 
@@ -102,7 +109,7 @@ def run_linters():
 print(p.stdout)
 
 
-for linter in ('pylint-3', 'mypy'):
+for linter in ('flake8', 'pylint-3', 'mypy'):
 if shutil.which(linter) is None:
 iotests.notrun(f'{linter} not found')
 
diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
index f2e1314d10..46bf3e665d 100644
--- a/tests/qemu-iotests/297.out
+++ b/tests/qemu-iotests/297.out
@@ -1,2 +1,3 @@
+=== flake8 ===
 === pylint ===
 === mypy ===
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 64d388a8fa..bbd227ff73 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -116,7 +116,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 self.vm_b.shutdown()
 
 log = self.vm_b.get_log()
-assert log is not None # Loaded after shutdown
+assert log is not None  # Loaded after shutdown
 
 if msg is None:
 

Re: [PATCH for-5.2 00/10] block/export: vhost-user-blk server tests and input validation

[Peter Maydell]

On Tue, 17 Nov 2020 at 09:18, Michael S. Tsirkin  wrote:
>
> On Wed, Nov 11, 2020 at 12:43:21PM +, Stefan Hajnoczi wrote:
> > The vhost-user-blk server test was already in Michael Tsirkin's recent vhost
> > pull request, but was dropped because it exposed vhost-user regressions
> > (b7c1bd9d7848 and the Based-on tag below). Now that the vhost-user 
> > regressions
> > are fixed we can re-introduce the test case.
> >
> > This series adds missing input validation that led to a Coverity report. The
> > virtio-blk read, write, discard, and write zeroes commands need to check
> > sector/byte ranges and other inputs. This solves the issue Peter Maydell 
> > raised
> > in "[PATCH for-5.2] block/export/vhost-user-blk-server.c: Avoid potential
> > integer overflow".
> >
> > Merging just the input validation patches would be possible too, but I 
> > prefer
> > to merge the corresponding tests so the code is exercised by the CI.
>
>
> Peter reports this hanging for him so I dropped this for now.
> Pls work with him to resolve, and resubmit.
> If possible let's defer the addition of new tests and just fix existing
> ones for 5.2.

Ping! This series was trying to fix a Coverity error -- it got dropped
for 5.2 but now 6.0 is open what are the plans for it?

thanks
-- PMM

Re: [PATCH v3 10/10] iotests/300: Clean up pylint and mypy complaints

[Vladimir Sementsov-Ogievskiy]

14.01.2021 20:03, Max Reitz wrote:

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/297 |  2 +-
  tests/qemu-iotests/300 | 18 +++---
  2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 1dce1d1b1c..03d8604538 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -33,7 +33,7 @@ SKIP_FILES = (
  '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
  '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
  '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
-'299', '300', '302', '303', '304', '307',
+'299', '302', '303', '304', '307',
  'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
  )
  
diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300

index b864a21d5e..64d388a8fa 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -22,7 +22,11 @@ import os
  import random
  import re
  from typing import Dict, List, Optional, Union
+
  import iotests
+
+# Import qemu after iotests.py has amended the PYTHONPATH


you mean os.path, exactly,

 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))

yes?


+# pylint: disable=wrong-import-order
  import qemu
  
  BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]

@@ -110,10 +114,14 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
  If @msg is None, check that there has not been any error.
  """
  self.vm_b.shutdown()
+
+log = self.vm_b.get_log()
+assert log is not None # Loaded after shutdown
+
  if msg is None:
-self.assertNotIn('qemu-system-', self.vm_b.get_log())
+self.assertNotIn('qemu-system-', log)
  else:
-self.assertIn(msg, self.vm_b.get_log())
+self.assertIn(msg, log)
  
  @staticmethod

  def mapping(node_name: str, node_alias: str,
@@ -445,9 +453,13 @@ class 
TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
  
  # Check for the error in the source's log

  self.vm_a.shutdown()
+
+log = self.vm_a.get_log()
+assert log is not None # Loaded after shutdown
+
  self.assertIn(f"Cannot migrate bitmap '{name}' on node "
f"'{self.src_node_name}': Name is longer than 255 
bytes",
-  self.vm_a.get_log())
+  log)
  
  # Expect abnormal shutdown of the destination VM because of

  # the failed migration



Your new comments are the only PEP8 complains in the 300 iotest:

flake8 300
300:119:31: E261 at least two spaces before inline comment
300:458:31: E261 at least two spaces before inline comment

So, I'd fix them.

anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy 


--
Best regards,
Vladimir

Re: [PATCH v3 02/10] iotests/297: Rewrite in Python and extend reach

[Vladimir Sementsov-Ogievskiy]

14.01.2021 20:02, Max Reitz wrote:

Instead of checking iotests.py only, check all Python files in the
qemu-iotests/ directory.  Of course, most of them do not pass, so there
is an extensive skip list for now.  (The only files that do pass are
209, 254, 283, and iotests.py.)

(Alternatively, we could have the opposite, i.e. an explicit list of
files that we do want to check, but I think it is better to check files
by default.)

Unless started in debug mode (./check -d), the output has no information
on which files are tested, so we will not have a problem e.g. with
backports, where some files may be missing when compared to upstream.

Besides the technical rewrite, some more things are changed:

- For the pylint invocation, PYTHONPATH is adjusted.  This mirrors
   setting MYPYPATH for mypy.

- Also, MYPYPATH is now derived from PYTHONPATH, so that we include
   paths set by the environment.  Maybe at some point we want to let the
   check script add '../../python/' to PYTHONPATH so that iotests.py does
   not need to do that.

- Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
   comments.  TODO is fine, we do not need 297 to complain about such
   comments.

- The "Success" line from mypy's output is suppressed, because (A) it
   does not add useful information, and (B) it would leak information
   about the files having been tested to the reference output, which we
   decidedly do not want.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/297 | 109 +
  tests/qemu-iotests/297.out |   5 +-
  2 files changed, 89 insertions(+), 25 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 5c5420712b..bfa26d280b 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -1,4 +1,4 @@
-#!/usr/bin/env bash
+#!/usr/bin/env python3
  #
  # Copyright (C) 2020 Red Hat, Inc.
  #
@@ -15,30 +15,95 @@
  # You should have received a copy of the GNU General Public License
  # along with this program.  If not, see .
  
-seq=$(basename $0)

-echo "QA output created by $seq"
+import os
+import re
+import shutil
+import subprocess
+import sys
  
-status=1	# failure is the default!

+import iotests
  
-# get standard environment

-. ./common.rc
  
-if ! type -p "pylint-3" > /dev/null; then

-_notrun "pylint-3 not found"
-fi
-if ! type -p "mypy" > /dev/null; then
-_notrun "mypy not found"
-fi
+# TODO: Empty this list!
+SKIP_FILES = (
+'030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
+'096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
+'151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
+'203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
+'218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
+'240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
+'262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
+'299', '300', '302', '303', '304', '307',
+'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
+)
  
-pylint-3 --score=n iotests.py
  
-MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \

---disallow-any-generics --disallow-incomplete-defs \
---disallow-untyped-decorators --no-implicit-optional \
---warn-redundant-casts --warn-unused-ignores \
---no-implicit-reexport iotests.py
+def is_python_file(filename):
+if not os.path.isfile(filename):
+return False
  
-# success, all done

-echo "*** done"
-rm -f $seq.full
-status=0
+if filename.endswith('.py'):
+return True
+
+with open(filename) as f:
+try:
+first_line = f.readline()
+return re.match('^#!.*python', first_line) is not None
+except UnicodeDecodeError: # Ignore binary files


PEP8 asks for two spaces before inline comment


+return False
+


and two empty lines here

(good ALE vim plugin runs flake8, mypy and pylint asynchronously for me and 
shows these things)


+def run_linters():
+files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
+ if is_python_file(filename)]
+
+iotests.logger.debug('Files to be checked:')
+iotests.logger.debug(', '.join(sorted(files)))


O, good.


+
+print('=== pylint ===')
+sys.stdout.flush()


Should not hurt.. But why? Should be flushed anyway as print will put a '\n'.. 
And why you care about it at all?


+
+# Todo notes are fine, but fixme's or xxx's should probably just be
+# fixed (in tests, at least)
+env = os.environ
+try:
+env['PYTHONPATH'] += ':../../python/'
+except KeyError:
+env['PYTHONPATH'] = '../../python/'
+subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
+   env=env, check=False)


as I understand, you don't need env argument, as 

Re: [PATCH v3 09/10] iotests/129: Clean up pylint and mypy complaints

[Vladimir Sementsov-Ogievskiy]

14.01.2021 20:03, Max Reitz wrote:

Signed-off-by: Max Reitz



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v3 06/10] iotests/129: Use throttle node

[Vladimir Sementsov-Ogievskiy]

14.01.2021 20:03, Max Reitz wrote:

Throttling on the BB has not affected block jobs in a while, so it is
possible that one of the jobs in 129 finishes before the VM is stopped.
We can fix that by running the job from a throttle node.

Signed-off-by: Max Reitz
Reviewed-by: Eric Blake


you forget my :)
Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v6 08/11] iotests: add testenv.py

[Kevin Wolf]

Am 09.01.2021 um 13:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Add TestEnv class, which will handle test environment in a new python
> iotests running framework.
> 
> Difference with current ./check interface:
> - -v (verbose) option dropped, as it is unused
> 
> - -xdiff option is dropped, until somebody complains that it is needed
> - same for -n option
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/testenv.py | 328 ++
>  1 file changed, 328 insertions(+)
>  create mode 100755 tests/qemu-iotests/testenv.py
> 
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> new file mode 100755
> index 00..ecaf76fb85
> --- /dev/null
> +++ b/tests/qemu-iotests/testenv.py
> @@ -0,0 +1,328 @@
> +#!/usr/bin/env python3
> +#
> +# Parse command line options to manage test environment variables.
> +#
> +# Copyright (c) 2020 Virtuozzo International GmbH
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see .
> +#
> +
> +import os
> +import sys
> +import tempfile
> +from pathlib import Path
> +import shutil
> +import collections
> +import subprocess
> +import argparse
> +from typing import List, Dict
> +
> +
> +def get_default_machine(qemu_prog: str) -> str:
> +outp = subprocess.run([qemu_prog, '-machine', 'help'], check=True,
> +  text=True, stdout=subprocess.PIPE).stdout
> +
> +machines = outp.split('\n')
> +default_machine = next(m for m in machines if m.endswith(' (default)'))
> +default_machine = default_machine.split(' ', 1)[0]
> +
> +alias_suf = ' (alias of {})'.format(default_machine)
> +alias = next((m for m in machines if m.endswith(alias_suf)), None)
> +if alias is not None:
> +default_machine = alias.split(' ', 1)[0]
> +
> +return default_machine
> +
> +
> +class TestEnv:
> +"""
> +Manage system environment for running tests
> +
> +The following variables are supported/provided. They are represented by
> +lower-cased TestEnv attributes.
> +"""
> +env_variables = ['PYTHONPATH', 'TEST_DIR', 'SOCK_DIR', 'SAMPLE_IMG_DIR',
> + 'OUTPUT_DIR', 'PYTHON', 'QEMU_PROG', 'QEMU_IMG_PROG',
> + 'QEMU_IO_PROG', 'QEMU_NBD_PROG', 'QSD_PROG',
> + 'SOCKET_SCM_HELPER', 'QEMU_OPTIONS', 'QEMU_IMG_OPTIONS',
> + 'QEMU_IO_OPTIONS', 'QEMU_NBD_OPTIONS', 'IMGOPTS',
> + 'IMGFMT', 'IMGPROTO', 'AIOMODE', 'CACHEMODE',
> + 'VALGRIND_QEMU', 'CACHEMODE_IS_DEFAULT', 
> 'IMGFMT_GENERIC',
> + 'IMGOPTSSYNTAX', 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE']
> +
> +def get_env(self) -> Dict[str, str]:
> +env = {}
> +for v in self.env_variables:
> +val = getattr(self, v.lower(), None)
> +if val is not None:
> +env[v] = val
> +
> +return env
> +
> +_argparser = None
> +@classmethod
> +def get_argparser(cls) -> argparse.ArgumentParser:
> +if cls._argparser is not None:
> +return cls._argparser
> +
> +p = argparse.ArgumentParser(description="= test environment options 
> =",
> +add_help=False, usage=argparse.SUPPRESS)
> +
> +p.add_argument('-d', dest='debug', action='store_true', help='debug')
> +p.add_argument('-misalign', action='store_true',
> +   help='misalign memory allocations')
> +
> +p.set_defaults(imgfmt='raw', imgproto='file')
> +
> +format_list = ['raw', 'bochs', 'cloop', 'parallels', 'qcow', 'qcow2',
> +   'qed', 'vdi', 'vpc', 'vhdx', 'vmdk', 'luks', 'dmg']
> +g = p.add_argument_group(
> +'image format options',
> +'The following options sets IMGFMT environment variable. '

s/sets/set the/

> +'At most one chose is allowed, default is "raw"')

s/chose/choice/

> +g = g.add_mutually_exclusive_group()
> +for fmt in format_list:
> +g.add_argument('-' + fmt, dest='imgfmt', action='store_const',
> +   const=fmt)
> +
> +protocol_list = ['file', 'rbd', 'sheepdoc', 'nbd', 'ssh', 'nfs',
> + 'fuse']
> +g = p.add_argument_group(
> +'image protocol options',
> +

Re: [PATCH v3 03/10] iotests: Move try_remove to iotests.py

[Vladimir Sementsov-Ogievskiy]

14.01.2021 20:02, Max Reitz wrote:

Signed-off-by: Max Reitz
Reviewed-by: Eric Blake


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH] hw/block/nvme: fix for non-msix machines

[Philippe Mathieu-Daudé]

On 1/15/21 10:46 AM, Klaus Jensen wrote:
> On Jan 15 10:37, Philippe Mathieu-Daudé wrote:
>> On 1/15/21 8:07 AM, Klaus Jensen wrote:
>>> On Jan 12 14:20, Philippe Mathieu-Daudé wrote:
 On 1/12/21 1:47 PM, Klaus Jensen wrote:
> From: Klaus Jensen 
>
> Commit 1c0c2163aa08 ("hw/block/nvme: verify msix_init_exclusive_bar()
> return value") had the unintended effect of breaking support on
> several platforms not supporting MSI-X.
>
> Still check for errors, but only report that MSI-X is unsupported
> instead of bailing out.
>
>>
>> BTW maybe worth adding:
>>
>> Cc: qemu-sta...@nongnu.org
>>
>> So this patch get backported in the next stable release based on 5.2.
>>
> 
> Thanks Philippe,
> 
> I messed up the bounce, but I sent it on to stable now.

I meant in the patch description itself before the other tags
(qemu-stable scan for commits with the "Cc: qemu-sta...@nongnu.org"
tag).

Re: [PATCH] hw/block/nvme: fix for non-msix machines

[Klaus Jensen]

On Jan 15 10:37, Philippe Mathieu-Daudé wrote:
> On 1/15/21 8:07 AM, Klaus Jensen wrote:
> > On Jan 12 14:20, Philippe Mathieu-Daudé wrote:
> >> On 1/12/21 1:47 PM, Klaus Jensen wrote:
> >>> From: Klaus Jensen 
> >>>
> >>> Commit 1c0c2163aa08 ("hw/block/nvme: verify msix_init_exclusive_bar()
> >>> return value") had the unintended effect of breaking support on
> >>> several platforms not supporting MSI-X.
> >>>
> >>> Still check for errors, but only report that MSI-X is unsupported
> >>> instead of bailing out.
> >>>
> 
> BTW maybe worth adding:
> 
> Cc: qemu-sta...@nongnu.org
> 
> So this patch get backported in the next stable release based on 5.2.
> 

Thanks Philippe,

I messed up the bounce, but I sent it on to stable now.


signature.asc
Description: PGP signature

Re: [PATCH] hw/block/nvme: fix for non-msix machines

[Philippe Mathieu-Daudé]

On 1/15/21 8:07 AM, Klaus Jensen wrote:
> On Jan 12 14:20, Philippe Mathieu-Daudé wrote:
>> On 1/12/21 1:47 PM, Klaus Jensen wrote:
>>> From: Klaus Jensen 
>>>
>>> Commit 1c0c2163aa08 ("hw/block/nvme: verify msix_init_exclusive_bar()
>>> return value") had the unintended effect of breaking support on
>>> several platforms not supporting MSI-X.
>>>
>>> Still check for errors, but only report that MSI-X is unsupported
>>> instead of bailing out.
>>>

BTW maybe worth adding:

Cc: qemu-sta...@nongnu.org

So this patch get backported in the next stable release based on 5.2.

>>> Reported-by: Guenter Roeck 
>>> Fixes: 1c0c2163aa08 ("hw/block/nvme: verify msix_init_exclusive_bar() 
>>> return value")
>>> Fixes: fbf2e5375e33 ("hw/block/nvme: Verify msix_vector_use() returned 
>>> value")
>>> Signed-off-by: Klaus Jensen 
>>> ---
>>>  hw/block/nvme.c | 31 ++-
>>>  1 file changed, 22 insertions(+), 9 deletions(-)
>>
>> Reviewed-by: Philippe Mathieu-Daudé 
>>
> 
> Thanks! Applied to nvme-next.
> 

Re: [PATCH v3 09/10] iotests/129: Clean up pylint and mypy complaints

[Max Reitz]

On 14.01.21 21:02, Willian Rampazzo wrote:

On Thu, Jan 14, 2021 at 2:41 PM Max Reitz  wrote:


Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/129 | 4 ++--
  tests/qemu-iotests/297 | 2 +-
  2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/129 b/tests/qemu-iotests/129
index 6d21470cd7..201d9e0a0b 100755
--- a/tests/qemu-iotests/129
+++ b/tests/qemu-iotests/129
@@ -20,7 +20,6 @@

  import os
  import iotests
-import time

  class TestStopWithBlockJob(iotests.QMPTestCase):
  test_img = os.path.join(iotests.test_dir, 'test.img')
@@ -32,7 +31,8 @@ class TestStopWithBlockJob(iotests.QMPTestCase):
  iotests.qemu_img('create', '-f', iotests.imgfmt, self.base_img, "1G")
  iotests.qemu_img('create', '-f', iotests.imgfmt, self.test_img,
   "-b", self.base_img, '-F', iotests.imgfmt)
-iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M', 
self.test_img)
+iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x5d 1M 128M',
+self.test_img)
  self.vm = iotests.VM()
  self.vm.add_object('throttle-group,id=tg0,x-bps-total=1024')

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index bfa26d280b..1dce1d1b1c 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -27,7 +27,7 @@ import iotests
  # TODO: Empty this list!
  SKIP_FILES = (
  '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
-'096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
+'096', '118', '124', '132', '136', '139', '147', '148', '149',


Is this also part of mypy/pylint cleanup? It seems you are doing more
than that here. It would be good to have this listed in the commit
message.


Sure, why not.  Something like “And consequentially drop it from 297's 
skip list.”?


Though I think making a test pass pylint+mypy complaints basically means 
exactly to remove it from 297's skip list and then making 297 pass, so 
I’m not entirely sure it’s necessary.  But it can’t hurt, so.



Despite that,

Reviewed-by: Willian Rampazzo 


Thanks!

Max


  '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
  '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
  '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
--
2.29.2

Re: [PATCH v3 01/10] iotests.py: Assume a couple of variables as given

[Vladimir Sementsov-Ogievskiy]

14.01.2021 20:02, Max Reitz wrote:

There are a couple of environment variables that we fetch with
os.environ.get() without supplying a default.  Clearly they are required
and expected to be set by the ./check script (as evidenced by
execute_setup_common(), which checks for test_dir and
qemu_default_machine to be set, and aborts if they are not).

Using .get() this way has the disadvantage of returning an Optional[str]
type, which mypy will complain about when tests just assume these values
to be str.

Use [] instead, which raises a KeyError for environment variables that
are not set.  When this exception is raised, catch it and move the abort
code from execute_setup_common() there.

Drop the 'assert iotests.sock_dir is not None' from iotest 300, because
that sort of thing is precisely what this patch wants to prevent.

Signed-off-by: Max Reitz



Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH v4 3/3] hw/blocl/nvme: trigger async event during injecting smart warning

[Philippe Mathieu-Daudé]

On 1/15/21 4:27 AM, zhenwei pi wrote:
> During smart critical warning injection by setting property from QMP
> command, also try to trigger asynchronous event.
> 
> Suggested by Keith, if a event has already been raised, there is no
> need to enqueue the duplicate event any more.
> 
> Signed-off-by: zhenwei pi 
> ---
>  hw/block/nvme.c  | 48 +---
>  include/block/nvme.h |  1 +
>  2 files changed, 42 insertions(+), 7 deletions(-)

Typo "blocl" in subject ;) No need to repost.

Re: [PATCH v4 1/3] block/nvme: introduce bit 5 for critical warning

[Philippe Mathieu-Daudé]

On 1/15/21 4:27 AM, zhenwei pi wrote:
> According to NVMe spec 1.4 section

"According to NVMe spec 1.4 section 5.14.1.2"

> , introduce bit 5
> for "Persistent Memory Region has become read-only or unreliable".
> 
> Signed-off-by: zhenwei pi 
> ---
>  include/block/nvme.h | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé