[PATCH v3 3/4] scripts/simplebench: add example usage of simplebench

[Vladimir Sementsov-Ogievskiy]

This example may be used as a template for custom benchmark.
It illustrates three things to prepare:
 - define bench_func
 - define test environments (columns)
 - define test cases (rows)
And final call of simplebench API.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 scripts/simplebench/bench-example.py | 80 
 1 file changed, 80 insertions(+)
 create mode 100644 scripts/simplebench/bench-example.py

diff --git a/scripts/simplebench/bench-example.py 
b/scripts/simplebench/bench-example.py
new file mode 100644
index 00..c642a5b891
--- /dev/null
+++ b/scripts/simplebench/bench-example.py
@@ -0,0 +1,80 @@
+#!/usr/bin/env python3
+#
+# Benchmark example
+#
+# Copyright (c) 2019 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 simplebench
+from bench_block_job import bench_block_copy, drv_file, drv_nbd
+
+
+def bench_func(env, case):
+""" Handle one "cell" of benchmarking table. """
+return bench_block_copy(env['qemu_binary'], env['cmd'],
+case['source'], case['target'])
+
+
+# You may set the following five variables to correct values, to turn this
+# example to real benchmark.
+ssd_source = '/path-to-raw-source-image-at-ssd'
+ssd_target = '/path-to-raw-target-image-at-ssd'
+hdd_target = '/path-to-raw-source-image-at-hdd'
+nbd_ip = 'nbd-ip-addr'
+nbd_port = 'nbd-port-number'
+
+# Test-cases are "rows" in benchmark resulting table, 'id' is a caption for
+# the row, other fields are handled by bench_func.
+test_cases = [
+{
+'id': 'ssd -> ssd',
+'source': drv_file(ssd_source),
+'target': drv_file(ssd_target)
+},
+{
+'id': 'ssd -> hdd',
+'source': drv_file(ssd_source),
+'target': drv_file(hdd_target)
+},
+{
+'id': 'ssd -> nbd',
+'source': drv_file(ssd_source),
+'target': drv_nbd(nbd_ip, nbd_port)
+},
+]
+
+# Test-envs are "columns" in benchmark resulting table, 'id is a caption for
+# the column, other fields are handled by bench_func.
+test_envs = [
+{
+'id': 'backup-1',
+'cmd': 'blockdev-backup',
+'qemu_binary': '/path-to-qemu-binary-1'
+},
+{
+'id': 'backup-2',
+'cmd': 'blockdev-backup',
+'qemu_binary': '/path-to-qemu-binary-2'
+},
+{
+'id': 'mirror',
+'cmd': 'blockdev-mirror',
+'qemu_binary': '/path-to-qemu-binary-1'
+}
+]
+
+result = simplebench.bench(bench_func, test_envs, test_cases, count=3)
+print(simplebench.ascii(result))
-- 
2.21.0

[PATCH v3 1/4] scripts/simplebench: add simplebench.py

[Vladimir Sementsov-Ogievskiy]

Add simple benchmark table creator.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 scripts/simplebench/simplebench.py | 128 +
 1 file changed, 128 insertions(+)
 create mode 100644 scripts/simplebench/simplebench.py

diff --git a/scripts/simplebench/simplebench.py 
b/scripts/simplebench/simplebench.py
new file mode 100644
index 00..59e7314ff6
--- /dev/null
+++ b/scripts/simplebench/simplebench.py
@@ -0,0 +1,128 @@
+#!/usr/bin/env python
+#
+# Simple benchmarking framework
+#
+# Copyright (c) 2019 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 .
+#
+
+
+def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
+"""Benchmark one test-case
+
+test_func   -- benchmarking function with prototype
+   test_func(env, case), which takes test_env and test_case
+   arguments and returns {'seconds': int} (which is benchmark
+   result) on success and {'error': str} on error. Returned
+   dict may contain any other additional fields.
+test_env-- test environment - opaque first argument for test_func
+test_case   -- test case - opaque second argument for test_func
+count   -- how many times to call test_func, to calculate average
+initial_run -- do initial run of test_func, which don't get into result
+
+Returns dict with the following fields:
+'runs': list of test_func results
+'average':  average seconds per run (exists only if at least one run
+succeeded)
+'delta':maximum delta between test_func result and the average
+(exists only if at least one run succeeded)
+'n-failed': number of failed runs (exists only if at least one run
+failed)
+"""
+if initial_run:
+print('  #initial run:')
+print('   ', test_func(test_env, test_case))
+
+runs = []
+for i in range(count):
+print('  #run {}'.format(i+1))
+res = test_func(test_env, test_case)
+print('   ', res)
+runs.append(res)
+
+result = {'runs': runs}
+
+successed = [r for r in runs if ('seconds' in r)]
+if successed:
+avg = sum(r['seconds'] for r in successed) / len(successed)
+result['average'] = avg
+result['delta'] = max(abs(r['seconds'] - avg) for r in successed)
+
+if len(successed) < count:
+result['n-failed'] = count - len(successed)
+
+return result
+
+
+def ascii_one(result):
+"""Return ASCII representation of bench_one() returned dict."""
+if 'average' in result:
+s = '{:.2f} +- {:.2f}'.format(result['average'], result['delta'])
+if 'n-failed' in result:
+s += '\n({} failed)'.format(result['n-failed'])
+return s
+else:
+return 'FAILED'
+
+
+def bench(test_func, test_envs, test_cases, *args, **vargs):
+"""Fill benchmark table
+
+test_func -- benchmarking function, see bench_one for description
+test_envs -- list of test environments, see bench_one
+test_cases -- list of test cases, see bench_one
+args, vargs -- additional arguments for bench_one
+
+Returns dict with the following fields:
+'envs':  test_envs
+'cases': test_cases
+'tab':   filled 2D array, where cell [i][j] is bench_one result for
+ test_cases[i] for test_envs[j] (i.e., rows are test cases and
+ columns are test environments)
+"""
+tab = {}
+results = {
+'envs': test_envs,
+'cases': test_cases,
+'tab': tab
+}
+n = 1
+n_tests = len(test_envs) * len(test_cases)
+for env in test_envs:
+for case in test_cases:
+print('Testing {}/{}: {} :: {}'.format(n, n_tests,
+   env['id'], case['id']))
+if case['id'] not in tab:
+tab[case['id']] = {}
+tab[case['id']][env['id']] = bench_one(test_func, env, case,
+   *args, **vargs)
+n += 1
+
+print('Done')
+return results
+
+
+def ascii(results):
+"""Return ASCII representation of bench() returned dict."""
+from tabulate import tabulate
+
+tab = [[""] + [c['id'] for c in results['envs']]]
+for case in results[

[PATCH v3 2/4] scripts/simplebench: add qemu/bench_block_job.py

[Vladimir Sementsov-Ogievskiy]

Add block-job benchmarking helper functions.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 scripts/simplebench/bench_block_job.py | 119 +
 1 file changed, 119 insertions(+)
 create mode 100755 scripts/simplebench/bench_block_job.py

diff --git a/scripts/simplebench/bench_block_job.py 
b/scripts/simplebench/bench_block_job.py
new file mode 100755
index 00..9808d696cf
--- /dev/null
+++ b/scripts/simplebench/bench_block_job.py
@@ -0,0 +1,119 @@
+#!/usr/bin/env python
+#
+# Benchmark block jobs
+#
+# Copyright (c) 2019 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 sys
+import os
+import socket
+import json
+
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
+from qemu.machine import QEMUMachine
+from qemu.qmp import QMPConnectError
+
+
+def bench_block_job(cmd, cmd_args, qemu_args):
+"""Benchmark block-job
+
+cmd   -- qmp command to run block-job (like blockdev-backup)
+cmd_args  -- dict of qmp command arguments
+qemu_args -- list of Qemu command line arguments, including path to Qemu
+ binary
+
+Returns {'seconds': int} on success and {'error': str} on failure, dict may
+contain addional 'vm-log' field. Return value is compatible with
+simplebench lib.
+"""
+
+vm = QEMUMachine(qemu_args[0], args=qemu_args[1:])
+
+try:
+vm.launch()
+except OSError as e:
+return {'error': 'popen failed: ' + str(e)}
+except (QMPConnectError, socket.timeout):
+return {'error': 'qemu failed: ' + str(vm.get_log())}
+
+try:
+res = vm.qmp(cmd, **cmd_args)
+if res != {'return': {}}:
+vm.shutdown()
+return {'error': '"{}" command failed: {}'.format(cmd, str(res))}
+
+e = vm.event_wait('JOB_STATUS_CHANGE')
+assert e['data']['status'] == 'created'
+start_ms = e['timestamp']['seconds'] * 100 + \
+e['timestamp']['microseconds']
+
+e = vm.events_wait((('BLOCK_JOB_READY', None),
+('BLOCK_JOB_COMPLETED', None),
+('BLOCK_JOB_FAILED', None)), timeout=True)
+if e['event'] not in ('BLOCK_JOB_READY', 'BLOCK_JOB_COMPLETED'):
+vm.shutdown()
+return {'error': 'block-job failed: ' + str(e),
+'vm-log': vm.get_log()}
+end_ms = e['timestamp']['seconds'] * 100 + \
+e['timestamp']['microseconds']
+finally:
+vm.shutdown()
+
+return {'seconds': (end_ms - start_ms) / 100.0}
+
+
+# Bench backup or mirror
+def bench_block_copy(qemu_binary, cmd, source, target):
+"""Helper to run bench_block_job() for mirror or backup"""
+assert cmd in ('blockdev-backup', 'blockdev-mirror')
+
+source['node-name'] = 'source'
+target['node-name'] = 'target'
+
+return bench_block_job(cmd,
+   {'job-id': 'job0', 'device': 'source',
+'target': 'target', 'sync': 'full'},
+   [qemu_binary,
+'-blockdev', json.dumps(source),
+'-blockdev', json.dumps(target)])
+
+
+def drv_file(filename):
+return {'driver': 'file', 'filename': filename,
+'cache': {'direct': True}, 'aio': 'native'}
+
+
+def drv_nbd(host, port):
+return {'driver': 'nbd',
+'server': {'type': 'inet', 'host': host, 'port': port}}
+
+
+if __name__ == '__main__':
+import sys
+
+if len(sys.argv) < 4:
+print('USAGE: {}  '
+  ' '
+  ''.format(sys.argv[0]))
+exit(1)
+
+res = bench_block_job(sys.argv[1], json.loads(sys.argv[2]), sys.argv[3:])
+if 'seconds' in res:
+print('{:.2f}'.format(res['seconds']))
+else:
+print(res)
-- 
2.21.0

[PATCH v3 0/4] benchmark util

[Vladimir Sementsov-Ogievskiy]

Hi all!

v3:
  move all to scripts/simplebench
  add myself as a maintainer of this thing

Here is simple benchmarking utility, to generate performance
comparison tables, like the following:

--  -  -  -
backup-1   backup-2   mirror
ssd -> ssd  0.43 +- 0.00   4.48 +- 0.06   4.38 +- 0.02
ssd -> hdd  10.60 +- 0.08  10.69 +- 0.18  10.57 +- 0.05
ssd -> nbd  33.81 +- 0.37  10.67 +- 0.17  10.07 +- 0.07
--  -  -  -

I'll use this benchmark in other series, hope someone
will like it.

Vladimir Sementsov-Ogievskiy (4):
  scripts/simplebench: add simplebench.py
  scripts/simplebench: add qemu/bench_block_job.py
  scripts/simplebench: add example usage of simplebench
  MAINTAINERS: add simplebench

 MAINTAINERS|   5 +
 scripts/simplebench/bench-example.py   |  80 
 scripts/simplebench/bench_block_job.py | 119 +++
 scripts/simplebench/simplebench.py | 128 +
 4 files changed, 332 insertions(+)
 create mode 100644 scripts/simplebench/bench-example.py
 create mode 100755 scripts/simplebench/bench_block_job.py
 create mode 100644 scripts/simplebench/simplebench.py

-- 
2.21.0

[PATCH v3 4/4] MAINTAINERS: add simplebench

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 MAINTAINERS | 5 +
 1 file changed, 5 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5e5e3e52d6..16d069adc5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2038,6 +2038,11 @@ F: python/qemu/*py
 F: scripts/*.py
 F: tests/*.py
 
+Benchmark util
+M: Vladimir Sementsov-Ogievskiy 
+S: Maintained
+F: scripts/simplebench/
+
 QAPI
 M: Markus Armbruster 
 M: Michael Roth 
-- 
2.21.0

Re: ping Re: [PATCH for-5.0 v2 0/3] benchmark util

[Vladimir Sementsov-Ogievskiy]

27.02.2020 23:09, Eduardo Habkost wrote:

Sorry, this is due to lack of bandwidth of maintainers who can
review those patches.

I have one suggestion: if you make your script self-contained
inside a scripts/ subdirectory, it would be simpler to merge it
without detailed reviews from others.


That works for me



The python/ subdirectory is supposed to appear on sys.path, so


Hmm. Ok, I think, we'll always be able to move shareable parts of simplebench
into python/ if needed. So it's OK to keep it in scripts. I just
thought that python/ is a new home for python scripts :) So, it's OK
to keep the whole thing at scripts/ for now.


maybe simplebench.py and qemu/bench_block_job.py can stay there,
but bench-example.py is not a loadable Python module and
shouldn't be there.

I see two possible options:

a) Moving everything to a scripts/simplebench subdirectory.
b) Moving only bench-example.py to scripts/, and do the sys.path
hacking the other scripts do.

On either case, please add your name to MAINTAINERS as the
maintainer of those new files.



OK, thanks!



On Thu, Feb 27, 2020 at 04:18:00PM +0300, Vladimir Sementsov-Ogievskiy wrote:

Hi!

Is problem in "S: Odd fixes" in Python section of MAINTAINERS?

Will it be correct, if I send a patch to MAINTAINERS, proposing
myself as maintainer of Python scripts and s/Odd fixes/Maintained/ ?

And then just send pull request with this series, as "nobody minds"?

08.02.2020 13:36, Vladimir Sementsov-Ogievskiy wrote:

pingg..

Hi! Could it be merged at all?

20.01.2020 12:10, Vladimir Sementsov-Ogievskiy wrote:

ping

26.11.2019 18:48, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

Here is simple benchmarking utility, to generate performance
comparison tables, like the following:

--  -  -  -
  backup-1   backup-2   mirror
ssd -> ssd  0.43 +- 0.00   4.48 +- 0.06   4.38 +- 0.02
ssd -> hdd  10.60 +- 0.08  10.69 +- 0.18  10.57 +- 0.05
ssd -> nbd  33.81 +- 0.37  10.67 +- 0.17  10.07 +- 0.07
--  -  -  -

This is a v2, as v1 was inside
   "[RFC 00/24] backup performance: block_status + async"

I'll use this benchmark in other series, hope someone
will like it.

Vladimir Sementsov-Ogievskiy (3):
    python: add simplebench.py
    python: add qemu/bench_block_job.py
    python: add example usage of simplebench

   python/bench-example.py    |  80 +
   python/qemu/bench_block_job.py | 115 +
   python/simplebench.py  | 128 +
   3 files changed, 323 insertions(+)
   create mode 100644 python/bench-example.py
   create mode 100755 python/qemu/bench_block_job.py
   create mode 100644 python/simplebench.py










--
Best regards,
Vladimir






--
Best regards,
Vladimir

Re: Strange data corruption issue with gluster (libgfapi) and ZFS

[Stefan Ring]

On Thu, Feb 27, 2020 at 10:12 PM Stefan Ring  wrote:
> Victory! I have a reproducer in the form of a plain C libgfapi client.
>
> However, I have not been able to trigger corruption by just executing
> the simple pattern in an artificial way. Currently, I need to feed my
> reproducer 2 GB of data that I streamed out of the qemu block driver.
> I get two possible end states out of my reproducer: The correct one or
> a corrupted one, where 48 KB are zeroed out. It takes no more than 10
> runs to get each of them at least once. The corrupted end state is
> exactly the same that I got from the real qemu process from where I
> obtained the streamed trace. This gives me a lot of confidence in the
> soundness of my reproducer.
>
> More details will follow.

Ok, so the exact sequence of activity around the corruption is this:

8700 and so on are the sequential request numbers. All of these
requests are writes. Blocks are 512 bytes.

8700
  grows the file to a certain size (2134144 blocks)

<8700 retires, nothing in flight>

8701
  writes 55 blocks inside currently allocated file range, close to the
end (7 blocks short)

8702
  writes 54 blocks from the end of 8701, growing the file by 47 blocks

<8702 retires, 8701 remains in flight>

8703
  writes from the end of 8702, growing the file by 81 blocks

<8703 retires, 8701 remains in flight>

8704
  writes 1623 blocks also from the end of 8702, growing the file by 1542 blocks

<8701 retires>
<8704 retires>

The exact range covered by 8703 ends up zeroed out.

If 8701 retires earlier (before 8702 is issued), everything is fine.

Re: Strange data corruption issue with gluster (libgfapi) and ZFS

[Stefan Ring]

On Tue, Feb 25, 2020 at 3:12 PM Stefan Ring  wrote:
>
> I find many instances with the following pattern:
>
> current file length (= max position + size written): p
> write request n writes from (p + hole_size), thus leaving a hole
> request n+1 writes exactly hole_size, starting from p, thus completely
> filling the hole
> The two requests' in-flight times overlap.
> hole_size can be almost any value (7-127).

Victory! I have a reproducer in the form of a plain C libgfapi client.

However, I have not been able to trigger corruption by just executing
the simple pattern in an artificial way. Currently, I need to feed my
reproducer 2 GB of data that I streamed out of the qemu block driver.
I get two possible end states out of my reproducer: The correct one or
a corrupted one, where 48 KB are zeroed out. It takes no more than 10
runs to get each of them at least once. The corrupted end state is
exactly the same that I got from the real qemu process from where I
obtained the streamed trace. This gives me a lot of confidence in the
soundness of my reproducer.

More details will follow.

Re: ping Re: [PATCH for-5.0 v2 0/3] benchmark util

[Eduardo Habkost]

Sorry, this is due to lack of bandwidth of maintainers who can
review those patches.

I have one suggestion: if you make your script self-contained
inside a scripts/ subdirectory, it would be simpler to merge it
without detailed reviews from others.

The python/ subdirectory is supposed to appear on sys.path, so
maybe simplebench.py and qemu/bench_block_job.py can stay there,
but bench-example.py is not a loadable Python module and
shouldn't be there.

I see two possible options:

a) Moving everything to a scripts/simplebench subdirectory.
b) Moving only bench-example.py to scripts/, and do the sys.path
   hacking the other scripts do.

On either case, please add your name to MAINTAINERS as the
maintainer of those new files.


On Thu, Feb 27, 2020 at 04:18:00PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi!
> 
> Is problem in "S: Odd fixes" in Python section of MAINTAINERS?
> 
> Will it be correct, if I send a patch to MAINTAINERS, proposing
> myself as maintainer of Python scripts and s/Odd fixes/Maintained/ ?
> 
> And then just send pull request with this series, as "nobody minds"?
> 
> 08.02.2020 13:36, Vladimir Sementsov-Ogievskiy wrote:
> > pingg..
> > 
> > Hi! Could it be merged at all?
> > 
> > 20.01.2020 12:10, Vladimir Sementsov-Ogievskiy wrote:
> > > ping
> > > 
> > > 26.11.2019 18:48, Vladimir Sementsov-Ogievskiy wrote:
> > > > Hi all!
> > > > 
> > > > Here is simple benchmarking utility, to generate performance
> > > > comparison tables, like the following:
> > > > 
> > > > --  -  -  -
> > > >  backup-1   backup-2   mirror
> > > > ssd -> ssd  0.43 +- 0.00   4.48 +- 0.06   4.38 +- 0.02
> > > > ssd -> hdd  10.60 +- 0.08  10.69 +- 0.18  10.57 +- 0.05
> > > > ssd -> nbd  33.81 +- 0.37  10.67 +- 0.17  10.07 +- 0.07
> > > > --  -  -  -
> > > > 
> > > > This is a v2, as v1 was inside
> > > >   "[RFC 00/24] backup performance: block_status + async"
> > > > 
> > > > I'll use this benchmark in other series, hope someone
> > > > will like it.
> > > > 
> > > > Vladimir Sementsov-Ogievskiy (3):
> > > >    python: add simplebench.py
> > > >    python: add qemu/bench_block_job.py
> > > >    python: add example usage of simplebench
> > > > 
> > > >   python/bench-example.py    |  80 +
> > > >   python/qemu/bench_block_job.py | 115 +
> > > >   python/simplebench.py  | 128 +
> > > >   3 files changed, 323 insertions(+)
> > > >   create mode 100644 python/bench-example.py
> > > >   create mode 100755 python/qemu/bench_block_job.py
> > > >   create mode 100644 python/simplebench.py
> > > > 
> > > 
> > > 
> > 
> > 
> 
> 
> -- 
> Best regards,
> Vladimir
> 

-- 
Eduardo

Re: [PATCH 3/3] iotests/138: Test leaks/corruptions fixed report

[Eric Blake]

On 2/27/20 11:02 AM, Max Reitz wrote:

Test that qemu-img check reports the number of leaks and corruptions
fixed in its JSON report (after a successful run).

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/138 | 41 --
  tests/qemu-iotests/138.out | 14 +
  2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
index 54b01046ad..25bfbd4cca 100755
--- a/tests/qemu-iotests/138
+++ b/tests/qemu-iotests/138
@@ -41,8 +41,10 @@ _supported_fmt qcow2
  _supported_proto file
  _supported_os Linux
  # With an external data file, data clusters are not refcounted
-# (and so qemu-img check does not check their refcount)
-_unsupported_imgopts data_file
+# (and so qemu-img check does not check their refcount);


Not this patch's problem, but is that a bug in 'qemu-img check' for not 
validating refcounts on an external data file?  Or is it merely this 
comment wording is not quite perfect?



+# we want to modify the refcounts, so we need them to have a specific
+# format (namely u16)
+_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
  
  echo

  echo '=== Check on an image with a multiple of 2^32 clusters ==='
@@ -65,6 +67,41 @@ poke_file "$TEST_IMG" $((2048 + 8)) 
"\x00\x80\x00\x00\x00\x00\x00\x00"
  # allocate memory", we have an error showing that l2 entry is invalid.
  _check_test_img
  
+echo

+echo '=== Check leaks-fixed/corruptions-fixed report'
+echo
+
+# After leaks and corruptions were fixed, those numbers should be
+# reported by qemu-img check
+_make_test_img 64k
+
+# Allocate data cluster
+$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+reftable_ofs=$(peek_file_be "$TEST_IMG" 48 8)
+refblock_ofs=$(peek_file_be "$TEST_IMG" $reftable_ofs 8)
+
+# Introduce a leak: Make the image header's refcount 2
+poke_file "$TEST_IMG" "$refblock_ofs" "\x00\x02"


Why not use your brand-new poke_file_be "$TEST_IMG" "$refblock_ofs" 2 2


+
+l1_ofs=$(peek_file_be "$TEST_IMG" 40 8)
+
+# Introduce a corruption: Drop the COPIED flag from the (first) L1 entry
+l1_entry=$(peek_file_be "$TEST_IMG" $l1_ofs 8)
+l1_entry=$((l1_entry & ~(1 << 63)))
+poke_file_be "$TEST_IMG" $l1_ofs 8 $l1_entry


Yep, the new function makes this task easier.  (You could also just peek 
1 byte at $((l1_ofs+7)) then write it back out with poke_file 
"$TEST_IMG" $((l1_ofs + 7)) $(printf '\\x%02x' $((val & 0xfe)))", but 
that just doesn't look as nice)



+
+echo
+# Should print the number of corruptions and leaks fixed
+# (Filter out all JSON fields (recognizable by their four-space
+# indentation), but keep the "-fixed" fields (by removing two spaces
+# from their indentation))
+# (Also filter out the L1 entry, because why not)
+_check_test_img -r all --output=json \
+| sed -e 's/^  \(.*\)-fixed"/\1-fixed"/' \
+| grep -v '^' \
+| sed -e "s/\\<$(printf %x $l1_entry)\\>/L1_ENTRY_VALUE/"


sed | grep | sed can often be done with a single sed:

... | sed -e 's/^  \(.*\)-fixed"/\1-fixed"/' \
   -e '/^/d' \
   -e "s/\\..."

Using \\< and \\> in the sed regex is a GNUism; do we want this test to 
run on BSD?


Reviewed-by: Eric Blake 

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

Re: [PATCH 2/3] iotests: Add poke_file_[bl]e functions

[Eric Blake]

On 2/27/20 11:02 AM, Max Reitz wrote:

Similarly to peek_file_[bl]e, we may want to write binary integers into
a file.  Currently, this often means messing around with poke_file and
raw binary strings.  I hope these functions make it a bit more
comfortable.

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/common.rc | 37 
  1 file changed, 37 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 4c246c0450..604f837668 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,6 +53,43 @@ poke_file()
  printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
  }
  
+# poke_file_le 'test.img' 512 2 65534

+poke_file_le()
+{


I like the interface.  However, the implementation is a bit bloated (but 
then again, that's why you cc'd me for review ;)



+local img=$1 ofs=$2 len=$3 val=$4 str=''
+
+for i in $(seq 0 $((len - 1))); do


No need to fork seq, when we can let bash do the iteration for us:

while ((len--)); do


+byte=$((val & 0xff))
+if [ $byte != 0 ]; then
+chr="$(printf "\x$(printf %x $byte)")"


Why are we doing two printf command substitutions instead of 1?


+else
+chr="\0"


Why do we have to special-case 0?  printf '\x00' does the right thing.


+fi
+str+="$chr"


I'd go with the faster str+=$(printf '\\x%02x' $((val & 0xff))), 
completely skipping the byte and chr variables.



+val=$((val >> 8))
+done
+
+poke_file "$img" "$ofs" "$str"
+}


So my version:

poke_file_le()
{
local img=$1 ofs=$2 len=$3 val=$4 str=
while ((len--)); do
str+=$(printf '\\x%02x' $((val & 0xff)))
val=$((val >> 8))
done
poke_file "$img" "$ofs" "$str"
}


+
+# poke_file_be 'test.img' 512 2 65279
+poke_file_be()
+{
+local img=$1 ofs=$2 len=$3 val=$4 str=''


And this one's even easier: we get big-endian for free from printf 
output, with a sed post-processing to add \x:


poke_file_be()
{
local str="$(printf "%0$(($3 * 2))x\n" $4 | sed 's/\(..\)/\\x\1/g')"
poke_file "$1" "$2" "$str"
}

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

[PATCH 1/2] qcow2: Make Qcow2AioTask store the full host offset

[Alberto Garcia]

The file_cluster_offset field of Qcow2AioTask stores a cluster-aligned
host offset. In practice this is not very useful because all users(*)
of this structure need the final host offset into the cluster, which
they calculate using

   host_offset = file_cluster_offset + offset_into_cluster(s, offset)

There is no reason why Qcow2AioTask cannot store host_offset directly
and that is what this patch does.

(*) compressed clusters are the exception: in this case what
file_cluster_offset was storing was the full compressed cluster
descriptor (offset + size). This does not change with this patch
but it is documented now.

Signed-off-by: Alberto Garcia 
---
 block/qcow2.c | 68 +--
 1 file changed, 33 insertions(+), 35 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3c754f616b..b2c7c8255e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -74,7 +74,7 @@ typedef struct {
 
 static int coroutine_fn
 qcow2_co_preadv_compressed(BlockDriverState *bs,
-   uint64_t file_cluster_offset,
+   uint64_t cluster_descriptor,
uint64_t offset,
uint64_t bytes,
QEMUIOVector *qiov,
@@ -2037,7 +2037,7 @@ out:
 
 static coroutine_fn int
 qcow2_co_preadv_encrypted(BlockDriverState *bs,
-   uint64_t file_cluster_offset,
+   uint64_t host_offset,
uint64_t offset,
uint64_t bytes,
QEMUIOVector *qiov,
@@ -2064,16 +2064,12 @@ qcow2_co_preadv_encrypted(BlockDriverState *bs,
 }
 
 BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
-ret = bdrv_co_pread(s->data_file,
-file_cluster_offset + offset_into_cluster(s, offset),
-bytes, buf, 0);
+ret = bdrv_co_pread(s->data_file, host_offset, bytes, buf, 0);
 if (ret < 0) {
 goto fail;
 }
 
-if (qcow2_co_decrypt(bs,
- file_cluster_offset + offset_into_cluster(s, offset),
- offset, buf, bytes) < 0)
+if (qcow2_co_decrypt(bs, host_offset, offset, buf, bytes) < 0)
 {
 ret = -EIO;
 goto fail;
@@ -2091,7 +2087,7 @@ typedef struct Qcow2AioTask {
 
 BlockDriverState *bs;
 QCow2ClusterType cluster_type; /* only for read */
-uint64_t file_cluster_offset;
+uint64_t host_offset; /* or full descriptor in compressed clusters */
 uint64_t offset;
 uint64_t bytes;
 QEMUIOVector *qiov;
@@ -2104,7 +2100,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState 
*bs,
AioTaskPool *pool,
AioTaskFunc func,
QCow2ClusterType cluster_type,
-   uint64_t file_cluster_offset,
+   uint64_t host_offset,
uint64_t offset,
uint64_t bytes,
QEMUIOVector *qiov,
@@ -2119,7 +2115,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState 
*bs,
 .bs = bs,
 .cluster_type = cluster_type,
 .qiov = qiov,
-.file_cluster_offset = file_cluster_offset,
+.host_offset = host_offset,
 .offset = offset,
 .bytes = bytes,
 .qiov_offset = qiov_offset,
@@ -2128,7 +2124,7 @@ static coroutine_fn int qcow2_add_task(BlockDriverState 
*bs,
 
 trace_qcow2_add_task(qemu_coroutine_self(), bs, pool,
  func == qcow2_co_preadv_task_entry ? "read" : "write",
- cluster_type, file_cluster_offset, offset, bytes,
+ cluster_type, host_offset, offset, bytes,
  qiov, qiov_offset);
 
 if (!pool) {
@@ -2142,13 +2138,12 @@ static coroutine_fn int qcow2_add_task(BlockDriverState 
*bs,
 
 static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
  QCow2ClusterType cluster_type,
- uint64_t file_cluster_offset,
+ uint64_t host_offset,
  uint64_t offset, uint64_t bytes,
  QEMUIOVector *qiov,
  size_t qiov_offset)
 {
 BDRVQcow2State *s = bs->opaque;
-int offset_in_cluster = offset_into_cluster(s, offset);
 
 switch (cluster_type) {
 case QCOW2_CLUSTER_ZERO_PLAIN:
@@ -2164,19 +2159,17 @@ static coroutine_fn int 
qcow2_co_preadv_task(BlockDriverState *bs,
qiov, qiov_offset, 0);
 
 case QCOW2_CLUSTER_COMPRESSED:
-return qcow2_co_preadv_compressed(bs, file_cluster_offset,
+  

[PATCH 2/2] qcow2: Convert qcow2_get_cluster_offset() into qcow2_get_host_offset()

[Alberto Garcia]

qcow2_get_cluster_offset() takes an (unaligned) guest offset and
returns the (aligned) offset of the corresponding cluster in the qcow2
image.

In practice none of the callers need to know where the cluster starts
so this patch makes the function calculate and return the final host
offset directly. The function is also renamed accordingly.

There is a pre-existing exception with compressed clusters: in this
case the function returns the complete cluster descriptor (containing
the offset and size of the compressed data). This does not change with
this patch but it is now documented.

Signed-off-by: Alberto Garcia 
---
 block/qcow2.h |  4 ++--
 block/qcow2-cluster.c | 38 ++
 block/qcow2.c | 24 +++-
 3 files changed, 31 insertions(+), 35 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 0942126232..f47ef6ca4e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -679,8 +679,8 @@ int qcow2_write_l1_entry(BlockDriverState *bs, int 
l1_index);
 int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
   uint8_t *buf, int nb_sectors, bool enc, Error 
**errp);
 
-int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
- unsigned int *bytes, uint64_t *cluster_offset);
+int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
+  unsigned int *bytes, uint64_t *host_offset);
 int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
unsigned int *bytes, uint64_t *host_offset,
QCowL2Meta **m);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 78c95dfa16..498330bb09 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -496,10 +496,15 @@ static int coroutine_fn 
do_perform_cow_write(BlockDriverState *bs,
 
 
 /*
- * get_cluster_offset
+ * get_host_offset
  *
- * For a given offset of the virtual disk, find the cluster type and offset in
- * the qcow2 file. The offset is stored in *cluster_offset.
+ * For a given offset of the virtual disk find the equivalent host
+ * offset in the qcow2 file and store it in *host_offset. Neither
+ * offset needs to be aligned to a cluster boundary.
+ *
+ * If the cluster is unallocated then *host_offset will be 0.
+ * If the cluster is compressed then *host_offset will contain the
+ * complete compressed cluster descriptor.
  *
  * On entry, *bytes is the maximum number of contiguous bytes starting at
  * offset that we are interested in.
@@ -511,12 +516,12 @@ static int coroutine_fn 
do_perform_cow_write(BlockDriverState *bs,
  * Returns the cluster type (QCOW2_CLUSTER_*) on success, -errno in error
  * cases.
  */
-int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
- unsigned int *bytes, uint64_t *cluster_offset)
+int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
+  unsigned int *bytes, uint64_t *host_offset)
 {
 BDRVQcow2State *s = bs->opaque;
 unsigned int l2_index;
-uint64_t l1_index, l2_offset, *l2_slice;
+uint64_t l1_index, l2_offset, *l2_slice, l2_entry;
 int c;
 unsigned int offset_in_cluster;
 uint64_t bytes_available, bytes_needed, nb_clusters;
@@ -537,7 +542,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
 bytes_needed = bytes_available;
 }
 
-*cluster_offset = 0;
+*host_offset = 0;
 
 /* seek to the l2 offset in the l1 table */
 
@@ -570,7 +575,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
 /* find the cluster offset for the given disk offset */
 
 l2_index = offset_to_l2_slice_index(s, offset);
-*cluster_offset = be64_to_cpu(l2_slice[l2_index]);
+l2_entry = be64_to_cpu(l2_slice[l2_index]);
 
 nb_clusters = size_to_clusters(s, bytes_needed);
 /* bytes_needed <= *bytes + offset_in_cluster, both of which are unsigned
@@ -578,7 +583,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
  * true */
 assert(nb_clusters <= INT_MAX);
 
-type = qcow2_get_cluster_type(bs, *cluster_offset);
+type = qcow2_get_cluster_type(bs, l2_entry);
 if (s->qcow_version < 3 && (type == QCOW2_CLUSTER_ZERO_PLAIN ||
 type == QCOW2_CLUSTER_ZERO_ALLOC)) {
 qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry found"
@@ -599,41 +604,42 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, 
uint64_t offset,
 }
 /* Compressed clusters can only be processed one by one */
 c = 1;
-*cluster_offset &= L2E_COMPRESSED_OFFSET_SIZE_MASK;
+*host_offset = l2_entry & L2E_COMPRESSED_OFFSET_SIZE_MASK;
 break;
 case QCOW2_CLUSTER_ZERO_PLAIN:
 case QCOW2_CLUSTER_UNALLOCATED:
 /* how many empty clusters ? */
 c = count_contiguous_clusters_unallocated(bs, nb_clusters,

[PATCH 0/2] Convert qcow2_get_cluster_offset() into qcow2_get_host_offset()

[Alberto Garcia]

Hi,

this is something I did while working on the subcluster series but
it's independent from it so I thought to send it already.

In short: qcow2_get_cluster_offset() returns a host cluster offset but
none of the callers actually wants the offset of the cluster, they
want the host offset into the cluster.

There's a pre-existing exception with compressed clusters. In this
case the returned value was overloaded to contain a cluster offset or
a compressed cluster descriptor, depending on the cluster type. This
is kind of ugly, and we could make it more explicit using a union or
something like that but I don't think it's worth the effort here, so I
just documented it.

Berto

Alberto Garcia (2):
  qcow2: Make Qcow2AioTask store the full host offset
  qcow2: Convert qcow2_get_cluster_offset() into qcow2_get_host_offset()

 block/qcow2.h |  4 +--
 block/qcow2-cluster.c | 38 --
 block/qcow2.c | 74 ++-
 3 files changed, 55 insertions(+), 61 deletions(-)

-- 
2.20.1

Re: [PATCH 1/3] qemu-img: Fix check's leak/corruption fix report

[Eric Blake]

On 2/27/20 11:02 AM, Max Reitz wrote:

There are two problems with qemu-img check's report on how many leaks
and/or corruptions have been fixed:

(1) ImageCheck.has_leaks_fixed and ImageCheck.has_corruptions_fixed are
only true when ImageCheck.leaks or ImageCheck.corruptions (respectively)
are non-zero.  qcow2's check implementation will set the latter to zero
after it has fixed leaks and corruptions, though, so leaks-fixed and
corruptions-fixed are actually never reported after successful repairs.
We should always report them when they are non-zero, just like all the
other fields of ImageCheck.

(2) After something has been fixed and we run the check a second time,
leaks_fixed and corruptions_fixed are taken from the first run; but
has_leaks_fixed and has_corruptions_fixed are not.  The second run
actually cannot fix anything, so with (1) fixed, has_leaks_fixed and
has_corruptions_fixed will always be false here.  (With (1) unfixed,
they will at least be false on successful runs, because then the number
of leaks and corruptions found in the second run should be 0.)
We should save has_leaks_fixed and has_corruptions_fixed just like we
save leaks_fixed and corruptions_fixed.

Signed-off-by: Max Reitz 
---
  qemu-img.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)



Reviewed-by: Eric Blake 

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

[PATCH 0/2] block: bdrv_reopen() with backing file in different AioContext

[Kevin Wolf]

Kevin Wolf (2):
  iotests: Refactor blockdev-reopen test for iothreads
  block: bdrv_reopen() with backing file in different AioContext

 block.c| 36 +-
 tests/qemu-iotests/245 | 40 --
 tests/qemu-iotests/245.out |  4 ++--
 3 files changed, 59 insertions(+), 21 deletions(-)

-- 
2.20.1

[PATCH 2/2] block: bdrv_reopen() with backing file in different AioContext

[Kevin Wolf]

This patch allows bdrv_reopen() (and therefore the x-blockdev-reopen QMP
command) to attach a node as the new backing file even if the node is in
a different AioContext than the parent if one of both nodes can be moved
to the AioContext of the other node.

Signed-off-by: Kevin Wolf 
---
 block.c| 36 +++-
 tests/qemu-iotests/245 |  8 +++-
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index 202c67e1e8..5dbba6cf31 100644
--- a/block.c
+++ b/block.c
@@ -3781,6 +3781,29 @@ static void bdrv_reopen_perm(BlockReopenQueue *q, 
BlockDriverState *bs,
 *shared = cumulative_shared_perms;
 }
 
+static bool bdrv_reopen_can_attach(BdrvChild *child,
+   BlockDriverState *new_child,
+   BlockDriverState *parent,
+   Error **errp)
+{
+AioContext *parent_ctx = bdrv_get_aio_context(parent);
+AioContext *child_ctx = bdrv_get_aio_context(new_child);
+GSList *ignore;
+bool ret;
+
+ignore = g_slist_prepend(NULL, child);
+ret = bdrv_can_set_aio_context(new_child, parent_ctx, &ignore, NULL);
+g_slist_free(ignore);
+if (ret) {
+return ret;
+}
+
+ignore = g_slist_prepend(NULL, child);
+ret = bdrv_can_set_aio_context(parent, child_ctx, &ignore, errp);
+g_slist_free(ignore);
+return ret;
+}
+
 /*
  * Take a BDRVReopenState and check if the value of 'backing' in the
  * reopen_state->options QDict is valid or not.
@@ -3832,16 +3855,11 @@ static int bdrv_reopen_parse_backing(BDRVReopenState 
*reopen_state,
 }
 
 /*
- * TODO: before removing the x- prefix from x-blockdev-reopen we
- * should move the new backing file into the right AioContext
- * instead of returning an error.
+ * Check AioContext compatibility so that the bdrv_set_backing_hd() call in
+ * bdrv_reopen_commit() won't fail.
  */
-if (new_backing_bs) {
-if (bdrv_get_aio_context(new_backing_bs) != bdrv_get_aio_context(bs)) {
-error_setg(errp, "Cannot use a new backing file "
-   "with a different AioContext");
-return -EINVAL;
-}
+if (!bdrv_reopen_can_attach(bs->backing, bs, new_backing_bs, errp)) {
+return -EINVAL;
 }
 
 /*
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 5a2cd5ed0e..d6135ec14d 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1010,18 +1010,16 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 # neither of them can switch to the other AioContext
 def test_iothreads_error(self):
 self.run_test_iothreads('iothread0', 'iothread1',
-"Cannot use a new backing file with a 
different AioContext")
+"Cannot change iothread of active block 
backend")
 
 def test_iothreads_compatible_users(self):
 self.run_test_iothreads('iothread0', 'iothread0')
 
 def test_iothreads_switch_backing(self):
-self.run_test_iothreads('iothread0', None,
-"Cannot use a new backing file with a 
different AioContext")
+self.run_test_iothreads('iothread0', None)
 
 def test_iothreads_switch_overlay(self):
-self.run_test_iothreads(None, 'iothread0',
-"Cannot use a new backing file with a 
different AioContext")
+self.run_test_iothreads(None, 'iothread0')
 
 if __name__ == '__main__':
 iotests.main(supported_fmts=["qcow2"],
-- 
2.20.1

[PATCH 1/2] iotests: Refactor blockdev-reopen test for iothreads

[Kevin Wolf]

We'll want to test more than one successful case in the future, so
prepare the test for that by a refactoring that runs each scenario in a
separate VM.

test_iothreads_switch_{backing,overlay} currently produce errors, but
these are cases that should actually work, by switching either the
backing file node or the overlay node to the AioContext of the other
node.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/245 | 42 +-
 tests/qemu-iotests/245.out |  4 ++--
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 489bf78bd0..5a2cd5ed0e 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -970,8 +970,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 self.assertEqual(self.get_node('hd1'), None)
 self.assert_qmp(self.get_node('hd2'), 'ro', True)
 
-# We don't allow setting a backing file that uses a different AioContext
-def test_iothreads(self):
+def run_test_iothreads(self, iothread_a, iothread_b, errmsg = None):
 opts = hd_opts(0)
 result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
 self.assert_qmp(result, 'return', {})
@@ -986,20 +985,43 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 result = self.vm.qmp('object-add', qom_type='iothread', id='iothread1')
 self.assert_qmp(result, 'return', {})
 
-result = self.vm.qmp('x-blockdev-set-iothread', node_name='hd0', 
iothread='iothread0')
+result = self.vm.qmp('device_add', driver='virtio-scsi', id='scsi0',
+ iothread=iothread_a)
 self.assert_qmp(result, 'return', {})
 
-self.reopen(opts, {'backing': 'hd2'}, "Cannot use a new backing file 
with a different AioContext")
-
-result = self.vm.qmp('x-blockdev-set-iothread', node_name='hd2', 
iothread='iothread1')
+result = self.vm.qmp('device_add', driver='virtio-scsi', id='scsi1',
+ iothread=iothread_b)
 self.assert_qmp(result, 'return', {})
 
-self.reopen(opts, {'backing': 'hd2'}, "Cannot use a new backing file 
with a different AioContext")
+if iothread_a:
+result = self.vm.qmp('device_add', driver='scsi-hd', drive='hd0',
+ share_rw=True, bus="scsi0.0")
+self.assert_qmp(result, 'return', {})
 
-result = self.vm.qmp('x-blockdev-set-iothread', node_name='hd2', 
iothread='iothread0')
-self.assert_qmp(result, 'return', {})
+if iothread_b:
+result = self.vm.qmp('device_add', driver='scsi-hd', drive='hd2',
+ share_rw=True, bus="scsi1.0")
+self.assert_qmp(result, 'return', {})
 
-self.reopen(opts, {'backing': 'hd2'})
+self.reopen(opts, {'backing': 'hd2'}, errmsg)
+self.vm.shutdown()
+
+# We don't allow setting a backing file that uses a different AioContext if
+# neither of them can switch to the other AioContext
+def test_iothreads_error(self):
+self.run_test_iothreads('iothread0', 'iothread1',
+"Cannot use a new backing file with a 
different AioContext")
+
+def test_iothreads_compatible_users(self):
+self.run_test_iothreads('iothread0', 'iothread0')
+
+def test_iothreads_switch_backing(self):
+self.run_test_iothreads('iothread0', None,
+"Cannot use a new backing file with a 
different AioContext")
+
+def test_iothreads_switch_overlay(self):
+self.run_test_iothreads(None, 'iothread0',
+"Cannot use a new backing file with a 
different AioContext")
 
 if __name__ == '__main__':
 iotests.main(supported_fmts=["qcow2"],
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index a19de5214d..682b93394d 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -1,6 +1,6 @@
-..
+.
 --
-Ran 18 tests
+Ran 21 tests
 
 OK
 {"execute": "job-finalize", "arguments": {"id": "commit0"}}
-- 
2.20.1

[PATCH 0/3] qemu-img: Fix check's leak/corruption fix report

[Max Reitz]

(Cc-ing Eric because of patch 2, mostly)

Hi,

While reviewing the “fix two small memleaks” series
(<20200227012950.12256-1-pannengy...@huawei.com>) I noticed that we save
ImageCheck.(leaks|corruptions)_fixed from the first run and overwrite
the values obtained in the second run (where they must be zero because
we do not request any fixes in that second run), but we do not overwrite
ImageCheck.has_(leaks|corruptions)_fixed after the second run.  That
smells fishy.

Furthermore, ImageCheck.has_(leaks|corruptions)_fixed are not set based
on whether (leaks|corruptions)_fixed is non-zero, but actually based on
whether the leaks and corruptions fields (respectively) are non-zero.
qcow2’s check implementation reduces the leaks and corruptions values
when it fixes them, because then there are no leaks and corruptions
after the check anymore.

All in all, after a successful run, you will not see
“qemu-img check --output=json” report corruptions-fixed or leaks-fixed.
Which is a shame.  So this series fixes that and adds a test to ensure
those fields are indeed reported.


Max Reitz (3):
  qemu-img: Fix check's leak/corruption fix report
  iotests: Add poke_file_[bl]e functions
  iotests/138: Test leaks/corruptions fixed report

 qemu-img.c   |  9 ++--
 tests/qemu-iotests/138   | 41 ++--
 tests/qemu-iotests/138.out   | 14 
 tests/qemu-iotests/common.rc | 37 
 4 files changed, 97 insertions(+), 4 deletions(-)

-- 
2.24.1

[PATCH 3/3] iotests/138: Test leaks/corruptions fixed report

[Max Reitz]

Test that qemu-img check reports the number of leaks and corruptions
fixed in its JSON report (after a successful run).

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/138 | 41 --
 tests/qemu-iotests/138.out | 14 +
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
index 54b01046ad..25bfbd4cca 100755
--- a/tests/qemu-iotests/138
+++ b/tests/qemu-iotests/138
@@ -41,8 +41,10 @@ _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
 # With an external data file, data clusters are not refcounted
-# (and so qemu-img check does not check their refcount)
-_unsupported_imgopts data_file
+# (and so qemu-img check does not check their refcount);
+# we want to modify the refcounts, so we need them to have a specific
+# format (namely u16)
+_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
 
 echo
 echo '=== Check on an image with a multiple of 2^32 clusters ==='
@@ -65,6 +67,41 @@ poke_file "$TEST_IMG" $((2048 + 8)) 
"\x00\x80\x00\x00\x00\x00\x00\x00"
 # allocate memory", we have an error showing that l2 entry is invalid.
 _check_test_img
 
+echo
+echo '=== Check leaks-fixed/corruptions-fixed report'
+echo
+
+# After leaks and corruptions were fixed, those numbers should be
+# reported by qemu-img check
+_make_test_img 64k
+
+# Allocate data cluster
+$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io
+
+reftable_ofs=$(peek_file_be "$TEST_IMG" 48 8)
+refblock_ofs=$(peek_file_be "$TEST_IMG" $reftable_ofs 8)
+
+# Introduce a leak: Make the image header's refcount 2
+poke_file "$TEST_IMG" "$refblock_ofs" "\x00\x02"
+
+l1_ofs=$(peek_file_be "$TEST_IMG" 40 8)
+
+# Introduce a corruption: Drop the COPIED flag from the (first) L1 entry
+l1_entry=$(peek_file_be "$TEST_IMG" $l1_ofs 8)
+l1_entry=$((l1_entry & ~(1 << 63)))
+poke_file_be "$TEST_IMG" $l1_ofs 8 $l1_entry
+
+echo
+# Should print the number of corruptions and leaks fixed
+# (Filter out all JSON fields (recognizable by their four-space
+# indentation), but keep the "-fixed" fields (by removing two spaces
+# from their indentation))
+# (Also filter out the L1 entry, because why not)
+_check_test_img -r all --output=json \
+| sed -e 's/^  \(.*\)-fixed"/\1-fixed"/' \
+| grep -v '^' \
+| sed -e "s/\\<$(printf %x $l1_entry)\\>/L1_ENTRY_VALUE/"
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/138.out b/tests/qemu-iotests/138.out
index aca7d47a80..79681e7cc9 100644
--- a/tests/qemu-iotests/138.out
+++ b/tests/qemu-iotests/138.out
@@ -9,4 +9,18 @@ ERROR: counting reference for region exceeding the end of the 
file by one cluste
 
 1 errors were found on the image.
 Data may be corrupted, or further writes to the image may corrupt it.
+
+=== Check leaks-fixed/corruptions-fixed report
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Leaked cluster 0 refcount=2 reference=1
+Repairing cluster 0 refcount=2 reference=1
+Repairing OFLAG_COPIED L2 cluster: l1_index=0 l1_entry=L1_ENTRY_VALUE 
refcount=1
+{
+  "corruptions-fixed": 1,
+  "leaks-fixed": 1,
+}
 *** done
-- 
2.24.1

[PATCH 1/3] qemu-img: Fix check's leak/corruption fix report

[Max Reitz]

There are two problems with qemu-img check's report on how many leaks
and/or corruptions have been fixed:

(1) ImageCheck.has_leaks_fixed and ImageCheck.has_corruptions_fixed are
only true when ImageCheck.leaks or ImageCheck.corruptions (respectively)
are non-zero.  qcow2's check implementation will set the latter to zero
after it has fixed leaks and corruptions, though, so leaks-fixed and
corruptions-fixed are actually never reported after successful repairs.
We should always report them when they are non-zero, just like all the
other fields of ImageCheck.

(2) After something has been fixed and we run the check a second time,
leaks_fixed and corruptions_fixed are taken from the first run; but
has_leaks_fixed and has_corruptions_fixed are not.  The second run
actually cannot fix anything, so with (1) fixed, has_leaks_fixed and
has_corruptions_fixed will always be false here.  (With (1) unfixed,
they will at least be false on successful runs, because then the number
of leaks and corruptions found in the second run should be 0.)
We should save has_leaks_fixed and has_corruptions_fixed just like we
save leaks_fixed and corruptions_fixed.

Signed-off-by: Max Reitz 
---
 qemu-img.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 7b7087dd60..c7567e1979 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -647,9 +647,9 @@ static int collect_image_check(BlockDriverState *bs,
 check->leaks= result.leaks;
 check->has_leaks= result.leaks != 0;
 check->corruptions_fixed= result.corruptions_fixed;
-check->has_corruptions_fixed= result.corruptions != 0;
+check->has_corruptions_fixed= result.corruptions_fixed != 0;
 check->leaks_fixed  = result.leaks_fixed;
-check->has_leaks_fixed  = result.leaks != 0;
+check->has_leaks_fixed  = result.leaks_fixed != 0;
 check->image_end_offset = result.image_end_offset;
 check->has_image_end_offset = result.image_end_offset != 0;
 check->total_clusters   = result.bfi.total_clusters;
@@ -803,9 +803,12 @@ static int img_check(int argc, char **argv)
 
 if (check->corruptions_fixed || check->leaks_fixed) {
 int corruptions_fixed, leaks_fixed;
+bool has_leaks_fixed, has_corruptions_fixed;
 
 leaks_fixed = check->leaks_fixed;
+has_leaks_fixed = check->has_leaks_fixed;
 corruptions_fixed   = check->corruptions_fixed;
+has_corruptions_fixed = check->has_corruptions_fixed;
 
 if (output_format == OFORMAT_HUMAN) {
 qprintf(quiet,
@@ -822,7 +825,9 @@ static int img_check(int argc, char **argv)
 ret = collect_image_check(bs, check, filename, fmt, 0);
 
 check->leaks_fixed  = leaks_fixed;
+check->has_leaks_fixed  = has_leaks_fixed;
 check->corruptions_fixed= corruptions_fixed;
+check->has_corruptions_fixed = has_corruptions_fixed;
 }
 
 if (!ret) {
-- 
2.24.1

[PATCH 2/3] iotests: Add poke_file_[bl]e functions

[Max Reitz]

Similarly to peek_file_[bl]e, we may want to write binary integers into
a file.  Currently, this often means messing around with poke_file and
raw binary strings.  I hope these functions make it a bit more
comfortable.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/common.rc | 37 
 1 file changed, 37 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 4c246c0450..604f837668 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,6 +53,43 @@ poke_file()
 printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
 }
 
+# poke_file_le 'test.img' 512 2 65534
+poke_file_le()
+{
+local img=$1 ofs=$2 len=$3 val=$4 str=''
+
+for i in $(seq 0 $((len - 1))); do
+byte=$((val & 0xff))
+if [ $byte != 0 ]; then
+chr="$(printf "\x$(printf %x $byte)")"
+else
+chr="\0"
+fi
+str+="$chr"
+val=$((val >> 8))
+done
+
+poke_file "$img" "$ofs" "$str"
+}
+
+# poke_file_be 'test.img' 512 2 65279
+poke_file_be()
+{
+local img=$1 ofs=$2 len=$3 val=$4 str=''
+
+for i in $(seq 0 $((len - 1))); do
+byte=$(((val >> ((len - 1 - i) * 8)) & 0xff))
+if [ $byte != 0 ]; then
+chr="$(printf "\x$(printf %x $byte)")"
+else
+chr="\0"
+fi
+str+=$chr
+done
+
+poke_file "$img" "$ofs" "$str"
+}
+
 # peek_file_le 'test.img' 512 2 => 65534
 peek_file_le()
 {
-- 
2.24.1

Re: [PATCH v2 0/2] fix two small memleaks

[Max Reitz]

On 27.02.20 02:29, Pan Nengyuan wrote:
> This series fix two small memleaks.
> 1. 'crypto_opts' forgot to free in qcow2_close(), do this cleanup in 
> qcow2_close();
> 2. Do free filename/format in collect_image_check() when we re-allocate it.  
> 
> v2->v1:
> - Instead of freeing part of fields in collect_image_check(), do discard the 
> old check object and allocate a new one in the caller to make more 
> sense.(suggested by Max Reitz)
> 
> Pan Nengyuan (2):
>   block/qcow2: do free crypto_opts in qcow2_close()
>   qemu-img: free memory before re-assign
> 
>  block/qcow2.c | 1 +
>  qemu-img.c| 2 ++
>  2 files changed, 3 insertions(+)

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] qcow2: Explicit mention of padding bytes

[Vladimir Sementsov-Ogievskiy]

27.02.2020 17:45, Eric Blake wrote:

Although we already covered the need for padding bytes with our
changes in commit 3ae3fcfa, commit 66fcbca5 just added one byte and
relied on the earlier text for implicitly covering 7 padding bytes.
For consistency with other parts of the header, it does not hurt to be
explicit that this version of the header is using padding bytes, and
if we later add other extension fields, we can rework the allocation
of those padding bytes to match those additions.

Signed-off-by: Eric Blake 
---
  docs/interop/qcow2.txt | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e244746e..193ac7c5c1af 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -209,6 +209,8 @@ version 2.
  Available compression type values:
  0: zlib 

+  105 - m:  Zero padding to round up the header size to the next
+multiple of 8.



Hmm. Strictly speaking, you defined it as one of additional fields. And by
this definition, we may start to check in qemu that these bytes are zero,
instead of ignoring them and keeping as is..

But may be it's just a nitpicking..


--
Best regards,
Vladimir

Re: [PATCH] qcow2: Explicit mention of padding bytes

[Max Reitz]

On 27.02.20 15:45, Eric Blake wrote:
> Although we already covered the need for padding bytes with our
> changes in commit 3ae3fcfa, commit 66fcbca5 just added one byte and
> relied on the earlier text for implicitly covering 7 padding bytes.
> For consistency with other parts of the header,

Those other places are about table entries where there is padding to the
next entry of the table, though.  In addition, for those other places
it’s the only mention that they are padded.

For the header, there is a whole own section describing the padding, so
I don’t quite see the point.

> it does not hurt to be
> explicit that this version of the header is using padding bytes, and
> if we later add other extension fields, we can rework the allocation
> of those padding bytes to match those additions.
> 
> Signed-off-by: Eric Blake 
> ---
>  docs/interop/qcow2.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index 5597e244746e..193ac7c5c1af 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -209,6 +209,8 @@ version 2.
>  Available compression type values:
>  0: zlib 
> 
> +  105 - m:  Zero padding to round up the header size to the next
> +multiple of 8.

And if we really want this, I think it might as well be specific, i.e.
“105 - 112”.  We would have to adjust it every time we add a new field
anyway.

Max

>  === Header padding ===
> 




signature.asc
Description: OpenPGP digital signature

Re: [PULL 24/31] fuzz: support for fork-based fuzzing.

[Alexander Bulekov]

On 200224 1135, Stefan Hajnoczi wrote:
> On Sat, Feb 22, 2020 at 05:34:29AM -0600, Eric Blake wrote:
> > On 2/22/20 2:50 AM, Stefan Hajnoczi wrote:
> > > From: Alexander Bulekov 
> > > 
> > > fork() is a simple way to ensure that state does not leak in between
> > > fuzzing runs. Unfortunately, the fuzzer mutation engine relies on
> > > bitmaps which contain coverage information for each fuzzing run, and
> > > these bitmaps should be copied from the child to the parent(where the
> > > mutation occurs). These bitmaps are created through compile-time
> > > instrumentation and they are not shared with fork()-ed processes, by
> > > default. To address this, we create a shared memory region, adjust its
> > > size and map it _over_ the counter region. Furthermore, libfuzzer
> > > doesn't generally expose the globals that specify the location of the
> > > counters/coverage bitmap. As a workaround, we rely on a custom linker
> > > script which forces all of the bitmaps we care about to be placed in a
> > > contiguous region, which is easy to locate and mmap over.
> > > 
> > > Signed-off-by: Alexander Bulekov 
> > > Reviewed-by: Stefan Hajnoczi 
> > > Reviewed-by: Darren Kenny 
> > > Message-id: 20200220041118.23264-16-alx...@bu.edu
> > > Signed-off-by: Stefan Hajnoczi 
> > > ---
> > 
> > Random drive-by observation:
> > 
> > > +++ b/tests/qtest/fuzz/fork_fuzz.ld
> > > @@ -0,0 +1,37 @@
> > > +/* We adjust linker script modification to place all of the stuff that 
> > > needs to
> > > + * persist across fuzzing runs into a contiguous seciton of memory. 
> > > Then, it is
> > 
> > section
> 
> Thanks, Eric!
> 
> Alex, please send follow-up patches to fix this typo and the 80
> character line limit issues identified by patchew (see patch email reply
> to this email thread).

Thank you Eric, Stefan!
Just sent out some fixes.

> Stefan

[PATCH] qcow2: Explicit mention of padding bytes

[Eric Blake]

Although we already covered the need for padding bytes with our
changes in commit 3ae3fcfa, commit 66fcbca5 just added one byte and
relied on the earlier text for implicitly covering 7 padding bytes.
For consistency with other parts of the header, it does not hurt to be
explicit that this version of the header is using padding bytes, and
if we later add other extension fields, we can rework the allocation
of those padding bytes to match those additions.

Signed-off-by: Eric Blake 
---
 docs/interop/qcow2.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e244746e..193ac7c5c1af 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -209,6 +209,8 @@ version 2.
 Available compression type values:
 0: zlib 

+  105 - m:  Zero padding to round up the header size to the next
+multiple of 8.

 === Header padding ===

-- 
2.25.1

Re: [PATCH v1 1/8] qcow2: introduce compression type feature

[Eric Blake]

On 2/27/20 8:30 AM, Vladimir Sementsov-Ogievskiy wrote:

But what is the benefit? We have already documented padding in the 
spec, and discussed it so much time... What is the problem with 
padding? And why to add 8 bytes for every new feature which needs 
only one byte?


Okay, so requiring an 8-byte field is not necessary.  But still, at 
least mentioning padding bytes (that may be assigned meanings later) 
is consistent with the rest of the document (for example, we have 
padding bits documented for the compatible/incompatible/autoclear 
feature bits), and reminds implementers to keep size rounded to a 
multiple of 8.




Yes, we can add something about it.. But I'm not sure we need, and I 
can't imaging correct short wording.



We have section about padding:

"
=== Header padding ===

@header_length must be a multiple of 8, which means that if the end of 
the last
additional field is not aligned, some padding is needed. This padding 
must be
zeroed, so that if some existing (or future) additional field will fall 
into
the padding, it will be interpreted accordingly to point [3.] of the 
previous

paragraph, i.e.  in the same manner as when this field is not present.
"


So, if we want to add something about 104-111, it should be added to 
this section, not to previous "=== Additional fields (version 3 and 
higher) ===".


And, if we want, to add something, we should consider both cases when 
compression type field exists and when not... What to write?


"105 - 111: These bytes are padding, if header length > 104. May be 
turned into new additional fields in future."


Sounds a bit strange... Keeping in mind that different versions of qemu 
may consider the same bytes as additional field or as padding, and it is 
correct.


Looking at the header extension, it can probably be as simple as:

105 - m: Zero padding to round up the header size to the next
 multiple of 8.

I guess I'll propose a patch to make my idea concrete.

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

Re: [PATCH v1 1/8] qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

27.02.2020 17:13, Eric Blake wrote:

On 2/27/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:


Hmm - I think it may be worth a tweak to qcow2.txt to call out:

104: compression_type
105 - 111: padding, must be 0

or else call out:

104-111: compression type

and just blindly use all 8 bytes for the value even though really only 1 or two 
values will ever be defined.  Of course, that moves the byte in question from 
104 to 111, thanks to our big endian encoding, but as this series is the first 
one installing a non-zero value in those 8 bytes, and as we just finished 
documenting that the header length must be a multiple of 8, there is no real 
impact - we can make such tweaks up until the 5.0 release.


But what is the benefit? We have already documented padding in the spec, and 
discussed it so much time... What is the problem with padding? And why to add 8 
bytes for every new feature which needs only one byte?


Okay, so requiring an 8-byte field is not necessary.  But still, at least 
mentioning padding bytes (that may be assigned meanings later) is consistent 
with the rest of the document (for example, we have padding bits documented for 
the compatible/incompatible/autoclear feature bits), and reminds implementers 
to keep size rounded to a multiple of 8.



Yes, we can add something about it.. But I'm not sure we need, and I can't 
imaging correct short wording.


We have section about padding:

"
=== Header padding ===

@header_length must be a multiple of 8, which means that if the end of the last
additional field is not aligned, some padding is needed. This padding must be
zeroed, so that if some existing (or future) additional field will fall into
the padding, it will be interpreted accordingly to point [3.] of the previous
paragraph, i.e.  in the same manner as when this field is not present.
"


So, if we want to add something about 104-111, it should be added to this section, not to 
previous "=== Additional fields (version 3 and higher) ===".

And, if we want, to add something, we should consider both cases when 
compression type field exists and when not... What to write?

"105 - 111: These bytes are padding, if header length > 104. May be turned into new 
additional fields in future."

Sounds a bit strange... Keeping in mind that different versions of qemu may 
consider the same bytes as additional field or as padding, and it is correct.


--
Best regards,
Vladimir

Re: [PATCH v6 8/9] iotests: don't use 'format' for drive_add

[Max Reitz]

On 27.02.20 01:06, John Snow wrote:
> It shadows (with a different type) the built-in format.
> Use something else.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/055| 3 ++-
>  tests/qemu-iotests/iotests.py | 6 +++---
>  2 files changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v6 6/9] iotests: use python logging for iotests.log()

[Max Reitz]

On 27.02.20 01:06, John Snow wrote:
> We can turn logging on/off globally instead of per-function.
> 
> Remove use_log from run_job, and use python logging to turn on
> diffable output when we run through a script entry point.
> 
> iotest 245 changes output order due to buffering reasons.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/030|  4 +--
>  tests/qemu-iotests/245|  1 +
>  tests/qemu-iotests/245.out| 24 -
>  tests/qemu-iotests/iotests.py | 50 +--
>  4 files changed, 45 insertions(+), 34 deletions(-)

[...]

> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index b02d7932fa..60c4c7f736 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -35,6 +35,14 @@
>  
>  assert sys.version_info >= (3, 6)
>  
> +# Use this logger for logging messages directly from the iotests module
> +logger = logging.getLogger('qemu.iotests')
> +logger.addHandler(logging.NullHandler())

Hm, I never see another handler added to this, so how can these messages
actually be printed?  Will enabling debug mode somehow make all loggers
print everything?

> +# Use this logger for messages that ought to be used for diff output.
> +test_logger = logging.getLogger('qemu.iotests.diff_io')

Also, why does logger get a null handler and this by default does not?
I’m asking because test_logger makes it look like you don’t necessarily
need a handler for output to be silently discarded.

Max

>  # This will not work if arguments contain spaces but is necessary if we
>  # want to support the override options that ./check supports.
>  qemu_img_args = [os.environ.get('QEMU_IMG_PROG', 'qemu-img')]



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v1 3/8] qcow2: add zstd cluster compression

[Vladimir Sementsov-Ogievskiy]

27.02.2020 17:11, Denis Plotnikov wrote:



On 27.02.2020 12:55, Vladimir Sementsov-Ogievskiy wrote:

27.02.2020 10:29, Denis Plotnikov wrote:

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
   src.img [zlib|zstd]_compressed.img
decompress cmd
   time ./qemu-img convert -O qcow2
   [zlib|zstd]_compressed.img uncompressed.img

    compression   decompression
  zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)    1.9  1.6 (-16 %)
user 65.0   15.8    5.3  2.5
sys   3.3    0.2    2.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
---
  block/qcow2-threads.c  | 122 +
  block/qcow2.c  |   7 +++
  configure  |  29 ++
  docs/interop/qcow2.txt |  18 ++
  qapi/block-core.json   |   3 +-
  5 files changed, 178 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 1c128e9840..e942c4d7e5 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
  #define ZLIB_CONST
  #include 
  +#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
  #include "qcow2.h"
  #include "block/thread-pool.h"
  #include "crypto.h"
@@ -164,6 +169,113 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
dest_size,
  return ret;
  }
  +#ifdef CONFIG_ZSTD
+
+#define ZSTD_LEN_BUF 4
+
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success


This doesn't match qcow2_co_compress definition. You should return 0 on success.

does it? I'd rather say it doesn't match to qcow2_co_compress description in 
the function comment, which we can change actually,
because qcow2_co_compress is used like:


Oh, yes, you are right. Then we should change the comment.



block/qcow2.c:

static coroutine_fn int
qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
  uint64_t offset, uint64_t bytes,
  QEMUIOVector *qiov, size_t qiov_offset)
{
     ...
     out_buf = g_malloc(s->cluster_size);

     out_len = qcow2_co_compress(bs, out_buf, s->cluster_size - 1,
     buf, s->cluster_size);
     if (out_len == -ENOMEM) {
     /* could not compress: write normal cluster */
     ret = qcow2_co_pwritev_part(bs, offset, bytes, qiov, qiov_offset, 0);
     if (ret < 0) {
     goto fail;
     }
     goto success;
     } else if (out_len < 0) {
     ret = -EINVAL;
     goto fail;
     }

     qemu_co_mutex_lock(&s->lock);
     ret = qcow2_alloc_compressed_cluster_offset(bs, offset, out_len, 

&cluster_offset);
     ...
}




+ *  -ENOMEM destination buffer is not enough to store compressed data
+ *  -EIO    on any other error
+ */
+
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+    size_t ret;
+
+    /*
+ * steal ZSTD_LEN_BUF bytes in the very beginng of the buffer


beginning


+ * to store compressed chunk size
+ */
+    char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
+
+    /*
+ * sanity check that we can store the compressed data length,
+ * and there is some space left for the compressor buffer
+ */
+    if (dest_size <= ZSTD_LEN_BUF) {
+    return -ENOMEM;
+    }
+
+    dest_size -= ZSTD_LEN_BUF;
+
+    ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
+
+    if (ZSTD_isError(ret)) {
+    if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
+    return -ENOMEM;
+    } else {
+    return -EIO;
+    }
+    }
+
+    /* paraniod sanity check that we can store the commpressed size */
+    if (ret > UINT_MAX) {
+    return -ENOMEM;
+    }


I'd use UINT32_MAX, possibly even more paranoid)

ok




+
+    /* store the compressed chunk size in the very beginning of the buffer */
+    stl_be_p(dest, ret);
+
+    return ret + ZSTD_LEN_BUF;


return 0;


+}
+
+/*
+ * qcow2_zstd_decompress()

Re: [PATCH v6 7/9] iotests: ignore import warnings from pylint

[Philippe Mathieu-Daudé]

On 2/27/20 1:06 AM, John Snow wrote:

The right way to solve this is to come up with a virtual environment
infrastructure that sets all the paths correctly, and/or to create
installable python modules that can be imported normally.

That's hard, so just silence this error for now.


I'm tempted to NAck this and require an "installable python module"...

Let's discuss why it is that hard!



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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 60c4c7f736..214f59995e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,6 +30,7 @@
  from collections import OrderedDict
  from typing import Collection
  
+# pylint: disable=import-error, wrong-import-position

  sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
  from qemu import qtest
  

Re: [PATCH v1 1/8] qcow2: introduce compression type feature

[Eric Blake]

On 2/27/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:


Hmm - I think it may be worth a tweak to qcow2.txt to call out:

104: compression_type
105 - 111: padding, must be 0

or else call out:

104-111: compression type

and just blindly use all 8 bytes for the value even though really only 
1 or two values will ever be defined.  Of course, that moves the byte 
in question from 104 to 111, thanks to our big endian encoding, but as 
this series is the first one installing a non-zero value in those 8 
bytes, and as we just finished documenting that the header length must 
be a multiple of 8, there is no real impact - we can make such tweaks 
up until the 5.0 release.


But what is the benefit? We have already documented padding in the spec, 
and discussed it so much time... What is the problem with padding? And 
why to add 8 bytes for every new feature which needs only one byte?


Okay, so requiring an 8-byte field is not necessary.  But still, at 
least mentioning padding bytes (that may be assigned meanings later) is 
consistent with the rest of the document (for example, we have padding 
bits documented for the compatible/incompatible/autoclear feature bits), 
and reminds implementers to keep size rounded to a multiple of 8.


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

Re: [PATCH v6 8/9] iotests: don't use 'format' for drive_add

[Philippe Mathieu-Daudé]

On 2/27/20 1:06 AM, John Snow wrote:

It shadows (with a different type) the built-in format.
Use something else.

Signed-off-by: John Snow 
---
  tests/qemu-iotests/055| 3 ++-
  tests/qemu-iotests/iotests.py | 6 +++---
  2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 82b9f5f47d..4175fff5e4 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -469,7 +469,8 @@ class TestDriveCompression(iotests.QMPTestCase):
  qemu_img('create', '-f', fmt, blockdev_target_img,
   str(TestDriveCompression.image_len), *args)
  if attach_target:
-self.vm.add_drive(blockdev_target_img, format=fmt, 
interface="none")
+self.vm.add_drive(blockdev_target_img,
+  img_format=fmt, interface="none")
  
  self.vm.launch()
  
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py

index 214f59995e..8bf640c632 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -492,21 +492,21 @@ def add_drive_raw(self, opts):
  self._args.append(opts)
  return self
  
-def add_drive(self, path, opts='', interface='virtio', format=imgfmt):

+def add_drive(self, path, opts='', interface='virtio', img_format=imgfmt):
  '''Add a virtio-blk drive to the VM'''
  options = ['if=%s' % interface,
 'id=drive%d' % self._num_drives]
  
  if path is not None:

  options.append('file=%s' % path)
-options.append('format=%s' % format)
+options.append('format=%s' % img_format)
  options.append('cache=%s' % cachemode)
  options.append('aio=%s' % aiomode)
  
  if opts:

  options.append(opts)
  
-if format == 'luks' and 'key-secret' not in opts:

+if img_format == 'luks' and 'key-secret' not in opts:
  # default luks support
  if luks_default_secret_object not in self._args:
  self.add_object(luks_default_secret_object)



Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v6 9/9] iotests: add pylintrc file

[Philippe Mathieu-Daudé]

On 2/27/20 1:06 AM, John Snow wrote:

Repeatable results. run `pylint iotests.py` and you should get a pass.

Signed-off-by: John Snow 
---
  tests/qemu-iotests/pylintrc | 20 
  1 file changed, 20 insertions(+)
  create mode 100644 tests/qemu-iotests/pylintrc

diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
new file mode 100644
index 00..feed506f75
--- /dev/null
+++ b/tests/qemu-iotests/pylintrc
@@ -0,0 +1,20 @@
+[MESSAGES CONTROL]
+
+# Disable the message, report, category or checker with the given id(s). You
+# can either give multiple identifiers separated by comma (,) or put this
+# option multiple times (only on the command line, not in the configuration
+# file where it should appear only once). You can also use "--disable=all" to
+# disable everything first and then reenable specific checks. For example, if
+# you want to run only the similarities checker, you can use "--disable=all
+# --enable=similarities". If you want to run only the classes checker, but have
+# no Warning level messages displayed, use "--disable=all --enable=classes
+# --disable=W".
+disable=invalid-name,
+missing-docstring,
+line-too-long,
+too-many-lines,
+too-few-public-methods,
+too-many-arguments,
+too-many-locals,
+too-many-branches,
+too-many-public-methods,
\ No newline at end of file



Can you run it in one of the CI jobs?

Re: [PATCH v1 3/8] qcow2: add zstd cluster compression

[Denis Plotnikov]

On 27.02.2020 12:55, Vladimir Sementsov-Ogievskiy wrote:

27.02.2020 10:29, Denis Plotnikov wrote:

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
   src.img [zlib|zstd]_compressed.img
decompress cmd
   time ./qemu-img convert -O qcow2
   [zlib|zstd]_compressed.img uncompressed.img

    compression   decompression
  zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)    1.9  1.6 (-16 %)
user 65.0   15.8    5.3  2.5
sys   3.3    0.2    2.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
---
  block/qcow2-threads.c  | 122 +
  block/qcow2.c  |   7 +++
  configure  |  29 ++
  docs/interop/qcow2.txt |  18 ++
  qapi/block-core.json   |   3 +-
  5 files changed, 178 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 1c128e9840..e942c4d7e5 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
  #define ZLIB_CONST
  #include 
  +#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
  #include "qcow2.h"
  #include "block/thread-pool.h"
  #include "crypto.h"
@@ -164,6 +169,113 @@ static ssize_t qcow2_zlib_decompress(void 
*dest, size_t dest_size,

  return ret;
  }
  +#ifdef CONFIG_ZSTD
+
+#define ZSTD_LEN_BUF 4
+
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success


This doesn't match qcow2_co_compress definition. You should return 0 
on success.
does it? I'd rather say it doesn't match to qcow2_co_compress 
description in the function comment, which we can change actually,

because qcow2_co_compress is used like:

block/qcow2.c:

static coroutine_fn int
qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
 uint64_t offset, uint64_t bytes,
 QEMUIOVector *qiov, size_t qiov_offset)
{
    ...
    out_buf = g_malloc(s->cluster_size);

    out_len = qcow2_co_compress(bs, out_buf, s->cluster_size - 1,
    buf, s->cluster_size);
    if (out_len == -ENOMEM) {
    /* could not compress: write normal cluster */
    ret = qcow2_co_pwritev_part(bs, offset, bytes, qiov, 
qiov_offset, 0);

    if (ret < 0) {
    goto fail;
    }
    goto success;
    } else if (out_len < 0) {
    ret = -EINVAL;
    goto fail;
    }

    qemu_co_mutex_lock(&s->lock);
    ret = qcow2_alloc_compressed_cluster_offset(bs, offset, out_len, 


&cluster_offset);
    ...
}



+ *  -ENOMEM destination buffer is not enough to store 
compressed data

+ *  -EIO    on any other error
+ */
+
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+    size_t ret;
+
+    /*
+ * steal ZSTD_LEN_BUF bytes in the very beginng of the buffer


beginning


+ * to store compressed chunk size
+ */
+    char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
+
+    /*
+ * sanity check that we can store the compressed data length,
+ * and there is some space left for the compressor buffer
+ */
+    if (dest_size <= ZSTD_LEN_BUF) {
+    return -ENOMEM;
+    }
+
+    dest_size -= ZSTD_LEN_BUF;
+
+    ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
+
+    if (ZSTD_isError(ret)) {
+    if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
+    return -ENOMEM;
+    } else {
+    return -EIO;
+    }
+    }
+
+    /* paraniod sanity check that we can store the commpressed size */
+    if (ret > UINT_MAX) {
+    return -ENOMEM;
+    }


I'd use UINT32_MAX, possibly even more paranoid)

ok




+
+    /* store the compressed chunk size in the very beginning of the 
buffer */

+    stl_be_p(dest, ret);
+
+    return ret + ZSTD_LEN_BUF;


return 0;


+}
+
+/*
+ * qcow2_zstd_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce 
exactly

+ * @dest_size bytes using zstd co

Re: [PATCH v6 4/9] iotest 258: use script_main

[Philippe Mathieu-Daudé]

On 2/27/20 1:06 AM, John Snow wrote:

Since this one is nicely factored to use a single entry point,
use script_main to run the tests.

Signed-off-by: John Snow 
---
  tests/qemu-iotests/258 | 11 ---
  1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/258 b/tests/qemu-iotests/258
index a65151dda6..e305a1502f 100755
--- a/tests/qemu-iotests/258
+++ b/tests/qemu-iotests/258
@@ -23,12 +23,6 @@ import iotests
  from iotests import log, qemu_img, qemu_io_silent, \
  filter_qmp_testfiles, filter_qmp_imgfmt
  
-# Need backing file and change-backing-file support

-iotests.script_initialize(
-supported_fmts=['qcow2', 'qed'],
-supported_platforms=['linux'],
-)
-
  # Returns a node for blockdev-add
  def node(node_name, path, backing=None, fmt=None, throttle=None):
  if fmt is None:
@@ -161,4 +155,7 @@ def main():
  test_concurrent_finish(False)
  
  if __name__ == '__main__':

-main()
+# Need backing file and change-backing-file support
+iotests.script_main(main,
+supported_fmts=['qcow2', 'qed'],
+supported_platforms=['linux'])



Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v1 6/8] iotests: add "compression type" for test output matching

[Eric Blake]

On 2/27/20 4:09 AM, Vladimir Sementsov-Ogievskiy wrote:

27.02.2020 13:04, Vladimir Sementsov-Ogievskiy wrote:

27.02.2020 10:29, Denis Plotnikov wrote:

Affected tests: 049, 060, 061, 065, 144, 182, 242, 255

After adding the compression type feature for qcow2, the compression 
type

is reported on image quering.

Add the corresponding values of the "compression type" for the tests' 
output

matching.


And this and the following patch.

Ideally, patch should not break any iotests. This means that all 
iotest updates
should be merged to the patch which changes their output. Probably, 
you can split
behavior-changing patch, to reduce iotest-updates per patch, but 
anyway, big patch

with a lot of iotests updates is better than patch which breaks iotests.


Or we can try to reduce behavior changes in case of zlib:

- keep header small in case of zlib, not adding zero field


No, I don't see the point in doing that.  It makes maintenance harder to 
try to minimize the header, for no gain (older images parse the larger 
header just fine).



- don't add feature table entry, if compress type is zlib
- don't report compression type on quering, if it is zlib

- then, all iotests output will be saved. And, then, if we need, we can 
change
these theree points one-by-one, updating iotests outputs. But I doubt 
that we

really need it, compatible behavior seems good enough.



[by the way, replying to this series is painful, since d...@vrtuozzo.com 
is not a valid email address]


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

Re: [PATCH v1 3/8] qcow2: add zstd cluster compression

[Eric Blake]

On 2/27/20 1:29 AM, Denis Plotnikov wrote:

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.




+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+size_t ret;
+
+/*
+ * steal ZSTD_LEN_BUF bytes in the very beginng of the buffer


beginning


+ * to store compressed chunk size
+ */
+char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
+
+/*
+ * sanity check that we can store the compressed data length,
+ * and there is some space left for the compressor buffer
+ */
+if (dest_size <= ZSTD_LEN_BUF) {
+return -ENOMEM;
+}
+
+dest_size -= ZSTD_LEN_BUF;
+
+ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
+
+if (ZSTD_isError(ret)) {
+if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
+return -ENOMEM;
+} else {
+return -EIO;
+}
+}
+
+/* paraniod sanity check that we can store the commpressed size */


paranoid, compressed


+if (ret > UINT_MAX) {
+return -ENOMEM;
+}


This is pointless.  Better is to ensure that we actually compressed data 
(the pigeonhole principle states that there are some inputs that MUST 
result in inflation, in order for most other inputs to result in 
compression).  But that check was satisfied by checking for 
ZSTD_error_dstSize_tooSmall, which is what happens for one of those 
uncompressible inputs.  Namely, zstd will never return a result larger 
than dest_size, and since dest_size is smaller than UINT_MAX on entry, 
this check is pointless.  But if you want something, I'd be okay with: 
assert(ret <= dest_size).



+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
  
  Available compression type values:

  0: zlib 
+1: zstd 
  
  
  === Header padding ===

@@ -575,11 +576,28 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 
8)):
  Another compressed cluster may map to the tail of the 
final
  sector used by this compressed cluster.
  
+The layout of the compressed data depends on the compression

+type used for the image (see compressed cluster layout).
+
  If a cluster is unallocated, read requests shall read the data from the 
backing
  file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
  no backing file or the backing file is smaller than the image, they shall read
  zeros for all parts that are not covered by the backing file.
  
+=== Compressed Cluster Layout ===

+
+The compressed cluster data has a layout depending on the compression
+type used for the image, as follows:
+
+Compressed data layout for the available compression types:
+(x = data_space_length - 1)
+
+0:  (default)  zlib :
+Byte  0 -  x: the compressed data content
+  all the space provided used for compressed data
+1:  zstd :
+Byte  0 -  3: the length of compressed data in bytes
+  4 -  x: the compressed data content
  
  == Snapshots ==
  
diff --git a/qapi/block-core.json b/qapi/block-core.json

index 873fbef3b5..4b6e576c44 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4401,11 +4401,12 @@
  # Compression type used in qcow2 image file
  #
  # @zlib:  zlib compression, see 
+# @zstd:  zstd compression, see 
  #
  # Since: 5.0
  ##
  { 'enum': 'Qcow2CompressionType',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
  


The spec and UI changes are okay.

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

Re: [PATCH v1 4/8] iotests: filter out compression_type

[Eric Blake]

On 2/27/20 1:29 AM, Denis Plotnikov wrote:

After adding compression type feature to qcow2 format, qemu framework
commands reporting the image settingd, e.g. "qemu-img create", started


settings


reporting the compression type for the image which breaks the iotests
output matching.

To fix it, add compression_type=zlib to the list of filtered image parameters.

Signed-off-by: Denis Plotnikov 
---
  tests/qemu-iotests/common.filter | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)


This should be squashed in to the patch that caused the breakage (3/8, 
if I'm right).




diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 3f8ee3e5f7..c6962d199c 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -152,7 +152,8 @@ _filter_img_create()
  -e "s# refcount_bits=[0-9]\\+##g" \
  -e "s# key-secret=[a-zA-Z0-9]\\+##g" \
  -e "s# iter-time=[0-9]\\+##g" \
--e "s# force_size=\\(on\\|off\\)##g"
+-e "s# force_size=\\(on\\|off\\)##g" \
+-e "s# compression_type=zlib##g"


Do you really want to hard-code just zlib, or should this be more 
generic as compression_type=[a-zA-Z0-9]\\+ as done on other lines like 
key-secret?


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

Re: [PATCH v6 5/9] iotests: Mark verify functions as private

[Max Reitz]

On 27.02.20 01:06, John Snow wrote:
> Discourage their use.
> 
> (Also, make pending patches not yet using the new entry points fail in a
> very obvious way.)
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 20 ++--
>  1 file changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v1 1/8] qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

27.02.2020 16:48, Eric Blake wrote:

On 2/27/20 1:29 AM, Denis Plotnikov wrote:

The patch adds some preparation parts for incompatible compression type
feature to Qcow2 that indicates which allow to use different compression


to qcow2, allowing the use of different


methods for image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Signed-off-by: Denis Plotnikov 
---
  block/qcow2.c | 105 ++
  block/qcow2.h |  31 ---
  include/block/block_int.h |   1 +
  qapi/block-core.json  |  22 +++-
  4 files changed, 150 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3c754f616b..2ccb2cabd1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1242,6 +1242,50 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
  return ret;
  }
+static int validate_compression_type(BDRVQcow2State *s, Error **errp)
+{
+    /*
+ * Sanity check
+ * according to qcow2 spec, the compression type is 1-byte field
+ * but in BDRVQcow2State the compression_type is enum sizeof(int)
+ * so, the max compression_type value is 255.
+ */
+    if (s->compression_type > 0xff) {
+    error_setg(errp, "qcow2: compression type value is too big");
+    return -EINVAL;
+    }


Hmm - I think it may be worth a tweak to qcow2.txt to call out:

104: compression_type
105 - 111: padding, must be 0

or else call out:

104-111: compression type

and just blindly use all 8 bytes for the value even though really only 1 or two 
values will ever be defined.  Of course, that moves the byte in question from 
104 to 111, thanks to our big endian encoding, but as this series is the first 
one installing a non-zero value in those 8 bytes, and as we just finished 
documenting that the header length must be a multiple of 8, there is no real 
impact - we can make such tweaks up until the 5.0 release.


But what is the benefit? We have already documented padding in the spec, and 
discussed it so much time... What is the problem with padding? And why to add 8 
bytes for every new feature which needs only one byte?




+
+    switch (s->compression_type) {
+    case QCOW2_COMPRESSION_TYPE_ZLIB:
+    break;
+
+    default:
+    error_setg(errp, "qcow2: unknown compression type: %u",
+   s->compression_type);
+    return -ENOTSUP;
+    }


Having two checks feels redundant, compared to just letting the default catch 
all unrecognized values in that field.


+
+    /*
+ * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
+ * the incompatible feature flag must be set
+ */
+    if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
+    if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) {
+    error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must not be set");
+    return -EINVAL;
+    }
+    } else {
+    if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
+    error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must be set");
+    return -EINVAL;
+    }
+    }


Matches what we documented in the spec.


+
+    return 0;
+}
+
  /* Called with s->lock held.  */
  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
    int flags, Error **errp)
@@ -1357,6 +1401,26 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
  s->compatible_features  = header.compatible_features;
  s->autoclear_features   = header.autoclear_features;
+    /*
+ * Handle compression type
+ * Older qcow2 images don't contain the compression type header.
+ * Distinguish them by the header length and use
+ * the only valid (default) compression type in that case
+ */
+    if (header.header_length > offsetof(QCowHeader, compression_type)) {
+    /*
+ * don't deal with endians since compression_type is 1 byte long
+ */
+    s->compression_type = header.compression_type;


Changes if you go with my suggestion of just making the compression_type field 
occupy 8 bytes in the qcow2 header.  (And if you want to keep it 1 byte, I 
still think the spec should call out explicit padding bytes).


+    } else {
+    s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+    }
+
+    ret = validate_compression_t

Re: [PATCH v6 4/9] iotest 258: use script_main

[Max Reitz]

On 27.02.20 01:06, John Snow wrote:
> Since this one is nicely factored to use a single entry point,
> use script_main to run the tests.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/258 | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v6 3/9] iotests: replace mutable list default args

[Max Reitz]

On 27.02.20 01:06, John Snow wrote:
> It's bad hygiene: if we modify this list, it will be modified across all
> invocations.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v1 1/8] qcow2: introduce compression type feature

[Eric Blake]

On 2/27/20 1:29 AM, Denis Plotnikov wrote:

The patch adds some preparation parts for incompatible compression type
feature to Qcow2 that indicates which allow to use different compression


to qcow2, allowing the use of different


methods for image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Signed-off-by: Denis Plotnikov 
---
  block/qcow2.c | 105 ++
  block/qcow2.h |  31 ---
  include/block/block_int.h |   1 +
  qapi/block-core.json  |  22 +++-
  4 files changed, 150 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3c754f616b..2ccb2cabd1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1242,6 +1242,50 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
  return ret;
  }
  
+static int validate_compression_type(BDRVQcow2State *s, Error **errp)

+{
+/*
+ * Sanity check
+ * according to qcow2 spec, the compression type is 1-byte field
+ * but in BDRVQcow2State the compression_type is enum sizeof(int)
+ * so, the max compression_type value is 255.
+ */
+if (s->compression_type > 0xff) {
+error_setg(errp, "qcow2: compression type value is too big");
+return -EINVAL;
+}


Hmm - I think it may be worth a tweak to qcow2.txt to call out:

104: compression_type
105 - 111: padding, must be 0

or else call out:

104-111: compression type

and just blindly use all 8 bytes for the value even though really only 1 
or two values will ever be defined.  Of course, that moves the byte in 
question from 104 to 111, thanks to our big endian encoding, but as this 
series is the first one installing a non-zero value in those 8 bytes, 
and as we just finished documenting that the header length must be a 
multiple of 8, there is no real impact - we can make such tweaks up 
until the 5.0 release.



+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+break;
+
+default:
+error_setg(errp, "qcow2: unknown compression type: %u",
+   s->compression_type);
+return -ENOTSUP;
+}


Having two checks feels redundant, compared to just letting the default 
catch all unrecognized values in that field.



+
+/*
+ * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
+ * the incompatible feature flag must be set
+ */
+if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
+if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) {
+error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must not be set");
+return -EINVAL;
+}
+} else {
+if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
+error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must be set");
+return -EINVAL;
+}
+}


Matches what we documented in the spec.


+
+return 0;
+}
+
  /* Called with s->lock held.  */
  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
@@ -1357,6 +1401,26 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
  s->compatible_features  = header.compatible_features;
  s->autoclear_features   = header.autoclear_features;
  
+/*

+ * Handle compression type
+ * Older qcow2 images don't contain the compression type header.
+ * Distinguish them by the header length and use
+ * the only valid (default) compression type in that case
+ */
+if (header.header_length > offsetof(QCowHeader, compression_type)) {
+/*
+ * don't deal with endians since compression_type is 1 byte long
+ */
+s->compression_type = header.compression_type;


Changes if you go with my suggestion of just making the compression_type 
field occupy 8 bytes in the qcow2 header.  (And if you want to keep it 1 
byte, I still think the spec should call out explicit padding bytes).



+} else {
+s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+}
+
+ret = validate_compression_type(s, errp);
+if (ret) {
+goto fail;
+}
+
  if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
  void *feature_table = NULL;
  qcow2_read_extensions(bs, header.header_length, ext_end,
@@ -2720,6 +2784,1

Re: [PATCH v6 2/9] iotests: add script_initialize

[Max Reitz]

On 27.02.20 01:06, John Snow wrote:
> Like script_main, but doesn't require a single point of entry.
> Replace all existing initialization sections with this drop-in replacement.
> 
> This brings debug support to all existing script-style iotests.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/149|  3 +-
>  tests/qemu-iotests/194|  4 +-
>  tests/qemu-iotests/202|  4 +-
>  tests/qemu-iotests/203|  4 +-
>  tests/qemu-iotests/206|  2 +-
>  tests/qemu-iotests/207|  6 ++-
>  tests/qemu-iotests/208|  2 +-
>  tests/qemu-iotests/209|  2 +-
>  tests/qemu-iotests/210|  6 ++-
>  tests/qemu-iotests/211|  6 ++-
>  tests/qemu-iotests/212|  6 ++-
>  tests/qemu-iotests/213|  6 ++-
>  tests/qemu-iotests/216|  4 +-
>  tests/qemu-iotests/218|  2 +-
>  tests/qemu-iotests/219|  2 +-
>  tests/qemu-iotests/222|  7 ++--
>  tests/qemu-iotests/224|  4 +-
>  tests/qemu-iotests/228|  6 ++-
>  tests/qemu-iotests/234|  4 +-
>  tests/qemu-iotests/235|  4 +-
>  tests/qemu-iotests/236|  2 +-
>  tests/qemu-iotests/237|  2 +-
>  tests/qemu-iotests/238|  2 +
>  tests/qemu-iotests/242|  2 +-
>  tests/qemu-iotests/246|  2 +-
>  tests/qemu-iotests/248|  2 +-
>  tests/qemu-iotests/254|  2 +-
>  tests/qemu-iotests/255|  2 +-
>  tests/qemu-iotests/256|  2 +-
>  tests/qemu-iotests/258|  7 ++--
>  tests/qemu-iotests/260|  4 +-
>  tests/qemu-iotests/262|  4 +-
>  tests/qemu-iotests/264|  4 +-
>  tests/qemu-iotests/277|  2 +
>  tests/qemu-iotests/280|  8 ++--
>  tests/qemu-iotests/283|  4 +-
>  tests/qemu-iotests/iotests.py | 73 +++
>  37 files changed, 128 insertions(+), 80 deletions(-)

Reviewed-by: Max Reitz 

[...]

> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index e8a0ea14fc..fdcf8a940c 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py

[...]

> @@ -1092,13 +1105,18 @@ def execute_unittest(output, verbosity, debug):
>  
>  sys.stderr.write(out)
>  
> -def execute_test(test_function=None,
> - supported_fmts=[],
> - supported_platforms=None,
> - supported_cache_modes=[], supported_aio_modes={},
> - unsupported_fmts=[], supported_protocols=[],
> - unsupported_protocols=[]):
> -"""Run either unittest or script-style tests."""
> +def execute_setup_common(supported_fmts: Collection[str] = (),

First time I see something like this, but I suppose it means any
collection (i.e. list or tuple in this case) that has str values?

Max

> + supported_platforms: Collection[str] = (),
> + supported_cache_modes: Collection[str] = (),
> + supported_aio_modes: Collection[str] = (),
> + unsupported_fmts: Collection[str] = (),
> + supported_protocols: Collection[str] = (),
> + unsupported_protocols: Collection[str] = ()) -> 
> bool:



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 00/20] hw: Clean up hw/i386 headers (and few alpha/hppa)

[Paolo Bonzini]

On 26/10/19 15:32, Laurent Vivier wrote:
> Le 26/10/2019 à 14:20, Philippe Mathieu-Daudé a écrit :
>> Hi,
>>
>> On 10/14/19 4:22 PM, Philippe Mathieu-Daudé wrote:
>>> This is a follow-up of Markus's cleanup series:
>>> Tame a few "touch this, recompile the world"
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg635748.html
>>>
>>> This part is mostly restricted to X86, but since some file from the
>>> Alpha/PA-RISC machines include "hw/i386/pc.h" I had to fix them
>>> too.
>>>
>>> Eventually I'll succeed at removing hw/i386/ dependency on non-X86
>>> platforms (Quest I started 2 years ago...).
>>>
>>> Regards,
>>>
>>> Phil.
>>>
>>> Philippe Mathieu-Daudé (20):
>>>    vl: Add missing "hw/boards.h" include
>>>    hw/southbridge/ich9: Removed unused headers
>>>    hw/input/pckbd: Remove unused "hw/i386/pc.h" header
>>>    hw/i386/ioapic_internal: Remove unused "hw/i386/ioapic.h" header
>>>    hw/timer: Remove unused "ui/console.h" header
>>>    hw/usb/dev-storage: Remove unused "ui/console.h" header
>>>    hw/i386/intel_iommu: Remove unused includes
>>>    hw/xen/xen_pt_load_rom: Remove unused includes
>>>    hw/alpha/alpha_sys: Remove unused "hw/ide.h" header
>>>    hw/alpha/dp264: Include "net/net.h"
>>>    hw/hppa/machine: Include "net/net.h"
>>>    hw/acpi/cpu_hotplug: Include "hw/pci/pci.h"
>>>    hw/timer/hpet: Include "exec/address-spaces.h"
>>>    hw/pci-host/q35: Include "qemu/range.h"
>>>    hw/i2c/smbus_ich9: Include "qemu/range.h"
>>>    hw/pci-host/piix: Include "qemu/range.h"
>>>    hw/acpi: Include "hw/mem/nvdimm.h"
>>>    hw/i386: Include "hw/mem/nvdimm.h"
>>>    hw/pci-host/q35: Remove unused includes
>>>    hw/i386/pc: Clean up includes
>> Laurent, since this series is fully reviewed, can it go via
>> your qemu-trivial tree?
> 
> I'll try but I'm not sure to have the time to do that before the softfreeze.

Ping :)

Paolo

Re: [PATCH v1 1/8] qcow2: introduce compression type feature

[Eric Blake]

On 2/27/20 2:21 AM, Vladimir Sementsov-Ogievskiy wrote:

27.02.2020 10:29, Denis Plotnikov wrote:

The patch adds some preparation parts for incompatible compression type
feature to Qcow2 that indicates which allow to use different compression
methods for image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression 
than ZLIB.


The default compression is ZLIB. Images created with ZLIB compression 
type

are backward compatible with older qemu versions.

Signed-off-by: Denis Plotnikov 
---
  block/qcow2.c | 105 ++
  block/qcow2.h |  31 ---
  include/block/block_int.h |   1 +
  qapi/block-core.json  |  22 +++-
  4 files changed, 150 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3c754f616b..2ccb2cabd1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c


Please, add to .git/config:

[diff]
     orderFile = /path/to/qemu/scripts/git.orderfile

This will force git format-patch to sort files in more comfortable order 
(header changes first, etc).


As I learned yesterday, git 2.23 and 2.24 have a bug where git 
format-patch fails to honor diff.orderfile (fixed in 2.25): 
https://bugzilla.redhat.com/show_bug.cgi?id=1807681
(and that explains why some of my recent patches have not been ordered 
the way I wanted, as Fedora 31 currently has a git from the broken 
window in time)



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

Re: [PATCH v4 08/10] nbd/server: introduce NBDExtentArray

[Eric Blake]

On 2/27/20 6:46 AM, Vladimir Sementsov-Ogievskiy wrote:

26.02.2020 18:06, Eric Blake wrote:

On 2/5/20 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:

Introduce NBDExtentArray class, to handle extents list creation in more
controlled way and with fewer OUT parameters in functions.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  nbd/server.c | 210 +--
  1 file changed, 118 insertions(+), 92 deletions(-)




+
+/* Further modifications of the array after conversion are abandoned */
+static void nbd_extent_array_convert_to_be(NBDExtentArray *ea)
+{
+    int i;
+
+    assert(!ea->converted_to_be);


Comment is stale - further modifications after conversion are a bug 
that aborts the program, not abandoned.


I always thought that "abandoned" is something that must not be done, so 
the word works here. But I don't know English well).


Rephrasing my comment, further modifications are "a bug that aborts the 
program", rather than "an ignored action that gets abandoned".



May be:

"No further modifications of the array allowed after converting to BE."? 


Yes, that wording is better.


Is it better?

Or just drop the comment.


That's also viable; the code reads fairly well even without the comment.

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

Re: ping Re: [PATCH for-5.0 v2 0/3] benchmark util

[Vladimir Sementsov-Ogievskiy]

Hi!

Is problem in "S: Odd fixes" in Python section of MAINTAINERS?

Will it be correct, if I send a patch to MAINTAINERS, proposing
myself as maintainer of Python scripts and s/Odd fixes/Maintained/ ?

And then just send pull request with this series, as "nobody minds"?

08.02.2020 13:36, Vladimir Sementsov-Ogievskiy wrote:

pingg..

Hi! Could it be merged at all?

20.01.2020 12:10, Vladimir Sementsov-Ogievskiy wrote:

ping

26.11.2019 18:48, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

Here is simple benchmarking utility, to generate performance
comparison tables, like the following:

--  -  -  -
 backup-1   backup-2   mirror
ssd -> ssd  0.43 +- 0.00   4.48 +- 0.06   4.38 +- 0.02
ssd -> hdd  10.60 +- 0.08  10.69 +- 0.18  10.57 +- 0.05
ssd -> nbd  33.81 +- 0.37  10.67 +- 0.17  10.07 +- 0.07
--  -  -  -

This is a v2, as v1 was inside
  "[RFC 00/24] backup performance: block_status + async"

I'll use this benchmark in other series, hope someone
will like it.

Vladimir Sementsov-Ogievskiy (3):
   python: add simplebench.py
   python: add qemu/bench_block_job.py
   python: add example usage of simplebench

  python/bench-example.py    |  80 +
  python/qemu/bench_block_job.py | 115 +
  python/simplebench.py  | 128 +
  3 files changed, 323 insertions(+)
  create mode 100644 python/bench-example.py
  create mode 100755 python/qemu/bench_block_job.py
  create mode 100644 python/simplebench.py










--
Best regards,
Vladimir

Re: [PATCH v2 3/3] qemu-img: Deprecate use of -b without -F

[Eric Blake]

On 2/27/20 1:09 AM, Peter Krempa wrote:

On Wed, Feb 26, 2020 at 20:39:28 -0600, Eric Blake wrote:

Creating an image that requires format probing of the backing image is
inherently unsafe (we've had several CVEs over the years based on
probes leaking information to the guest on a subsequent boot).  If our
probing algorithm ever changes, or if other tools like libvirt
determine a different probe result than we do, then subsequent use of
that backing file under a different format will present corrupted data
to the guest.  Start a deprecation clock so that future qemu-img can
refuse to create unsafe backing chains that would rely on probing.

However, there is one time where probing is safe: if we probe raw,
then it is safe to record that implicitly in the image (but we still
warn, as it's better to teach the user to supply -F always than to
make them guess when it is safe).

iotest 114 specifically wants to create an unsafe image for later
amendment rather than defaulting to our new default of recording a
probed format, so it needs an update.

Signed-off-by: Eric Blake 
---
  qemu-deprecated.texi   | 15 +++
  block.c| 21 -
  qemu-img.c |  8 +++-
  tests/qemu-iotests/114 |  4 ++--
  tests/qemu-iotests/114.out |  1 +
  5 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 66eca3a1dede..f99b49addccc 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -309,6 +309,21 @@ The above, converted to the current supported format:

  @section Related binaries

+@subsection qemu-img backing file without format (since 5.0.0)
+
+The use of @command{qemu-img create}, @command{qemu-img rebase},
+@command{qemu-img convert}, or @command{qemu-img ament} to create or


s/ament/amend/



Fixing.  (Do you ever wonder if I leave a typo in, just to check that 
reviewers were actually reviewing?  But in this case, it was a symptom 
of me posting a series late at night to start the review process, even 
though I _really_ need to post a v3 that adds even more iotest coverage 
of every new deprecation warning made possible in this patch)



+modify an image that depends on a backing file now recommends that an
+explicit backing format be provided.  This is for safety - if qemu
+probes a different format than what you thought, the data presented to
+the guest will be corrupt; similarly, presenting a raw image to a
+guest allows the guest a potential security exploit if a future probe
+sees non-raw.  To avoid warning messages, or even future refusal to
+create an unsafe image, you must pass @option{-o backing_fmt=} (or
+shorthand @option{-F}) to specify the intended backing format.  You
+may use @command{qemu-img rebase -u} to retroactively add a backing
+format to an existing image.


I'd add a note for users who wish to fix existing images (and I need
to add it to libvirt's knowledge base too) that when the user is unsure
of the backing image format and desn't trust that the image was handled
in a trusted way, they must not use the format detected by qemu-img info
if the image specifies a backing file, unless they also trust the
backing file. (Sorry as a non-native English speaker I can't express
this in a more elegant way.).


Good idea.  How about:

...to retroactively add a backing format to an existing image.  However, 
be aware that there are already potential security risks to using 
'qemu-img info' to probe the format of an untrusted backing image, when 
deciding what format to write into an image with 'qemu-img rebase -u'.






  @subsection qemu-img convert -n -o (since 4.2.0)

  All options specified in @option{-o} are image creation options, so


[...]


diff --git a/qemu-img.c b/qemu-img.c
index b9375427404d..e75ec1bdb555 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3637,7 +3637,13 @@ static int img_rebase(int argc, char **argv)
   * doesn't change when we switch the backing file.
   */
  if (out_baseimg && *out_baseimg) {
-ret = bdrv_change_backing_file(bs, out_baseimg, out_basefmt, false);
+if (blk_new_backing && !out_basefmt) {
+out_basefmt = blk_bs(blk_new_backing)->drv->format_name;
+warn_report("Deprecated use of backing file "
+"without explicit backing format, using "
+"detected format of %s", out_basefmt);


Isn't this a similar instance to what I've reported in the previous
version? You warn that the format is missing, but set out_basefmt to the
detected format , thus bdrv_change_backing_file will then write the
detected format into the overlay even when it previously did not?

I think this has to have the same check for raw-only as above.


Yes, I think I missed a spot (blame the late-night push to get the patch 
out the door).  I'll wait to see if I get review from the qemu side 
before posting v3, though.





+}
+ret = bdrv_change_backing_file(bs

Re: [PATCH v2 1/3] iotests: Specify explicit backing format where sensible

[Eric Blake]

On 2/27/20 3:19 AM, Ján Tomko wrote:

On a Wednesday in 2020, Eric Blake wrote:

There are many existing qcow2 images that specify a backing file but
no format.  This has been the source of CVEs in the past, but has
become more prominent of a problem now that libvirt has switched to
-blockdev.  With older -drive, at least the probing was always done by
qemu (so the only risk of a changed format between successive boots of
a guest was if qemu was upgraded and probed differently).  But with
newer -blockdev, libvirt must specify a format; if libvirt guesses raw
where the image was formatted, this results in data corruption visible
to the guest; conversely, if libvirt guesses qcow2 where qemu was
using raw, this can result in potential security holes, so modern
libvirt instead refuses to use images without explicit backing format.

The change in libvirt to reject images without explicit backing format
has pointed out that a number of tools have been far too reliant on
probing in the past.  It's time to set a better example in our own
iotests of properly setting this parameter.

iotest calls to create, rebase, convert, and amend are all impacted to
some degree.  It's a bit annoying that we are inconsistent on command
line - while all of those accept -o backing_file=...,backing_fmt=...,
the shortcuts are different: create and rebase have -b and -F, convert
has -B but no -F, and amend has no shortcuts.

Signed-off-by: Eric Blake 
---


[...]

Test #225 still uses -b without a format:

./check -vmdk 225


Oh, good catch (I only ran ./check -qcow2, -nbd, and -raw).

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

Re: [PATCH v2 1/3] iotests: Specify explicit backing format where sensible

[Eric Blake]

On 2/27/20 1:20 AM, Peter Krempa wrote:

On Wed, Feb 26, 2020 at 20:39:26 -0600, Eric Blake wrote:

There are many existing qcow2 images that specify a backing file but
no format.  This has been the source of CVEs in the past, but has
become more prominent of a problem now that libvirt has switched to
-blockdev.  With older -drive, at least the probing was always done by
qemu (so the only risk of a changed format between successive boots of
a guest was if qemu was upgraded and probed differently).  But with
newer -blockdev, libvirt must specify a format; if libvirt guesses raw
where the image was formatted, this results in data corruption visible
to the guest; conversely, if libvirt guesses qcow2 where qemu was
using raw, this can result in potential security holes, so modern
libvirt instead refuses to use images without explicit backing format.

The change in libvirt to reject images without explicit backing format
has pointed out that a number of tools have been far too reliant on
probing in the past.  It's time to set a better example in our own
iotests of properly setting this parameter.

iotest calls to create, rebase, convert, and amend are all impacted to
some degree.  It's a bit annoying that we are inconsistent on command
line - while all of those accept -o backing_file=...,backing_fmt=...,
the shortcuts are different: create and rebase have -b and -F, convert
has -B but no -F, and amend has no shortcuts.

Signed-off-by: Eric Blake 
---


[...]


  113 files changed, 414 insertions(+), 338 deletions(-)

diff --git a/tests/qemu-iotests/017 b/tests/qemu-iotests/017
index 0a4b854e6520..585512bb296b 100755
--- a/tests/qemu-iotests/017
+++ b/tests/qemu-iotests/017
@@ -66,7 +66,7 @@ echo "Creating test image with backing file"
  echo

  TEST_IMG=$TEST_IMG_SAVE
-_make_test_img -b "$TEST_IMG.base" 6G
+_make_test_img -b "$TEST_IMG.base" -F $IMGFMT 6G



My understanding of the intricacies of the qemu-iotest suite is not good
enoug to be able to review this patch. Specifically $IMGFMT in this
instance is also used in the '-f' switch of qemu-img in _make_test_img
and I don't know if it's expected for the backing file to share the
format.


That's fine; I'm hoping a qemu expert reviews as well.

$IMGFMT allows an iotest to be run on multiple formats (qcow2, qed, 
vmdk); most tests in this patch series either hard-coded the base image 
to be 'raw' (in which case I added -F raw) or to be the same type as the 
wrapper image under test (in which case it is -F $IMGFMT).  We have very 
few tests that would try to mix-and-match other formats, such as a qcow2 
on top of qed.


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

Re: [PATCH v6 1/9] iotests: do a light delinting

[Max Reitz]

On 27.02.20 01:06, John Snow wrote:
> This doesn't fix everything in here, but it does help clean up the
> pylint report considerably.
> 
> This should be 100% style changes only; the intent is to make pylint
> more useful by working on establishing a baseline for iotests that we
> can gate against in the future. This will be important if (when?) we
> begin adding type hints to our code base.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 88 ++-
>  1 file changed, 45 insertions(+), 43 deletions(-)

I feel like I’m the wrongest person there is for reviewing a Python
style-fixing patch, but here I am and so here I go:

> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 8815052eb5..e8a0ea14fc 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py

[...]

> @@ -245,8 +243,7 @@ def qemu_nbd_early_pipe(*args):
>' '.join(qemu_nbd_args + ['--fork'] + list(args
>  if exitcode == 0:
>  return exitcode, ''
> -else:
> -return exitcode, subp.communicate()[0]
> +return exitcode, subp.communicate()[0]

If we want to make such a change (which I don’t doubt we want), I think
it should be the other way around: Make the condition “exitcode != 0”,
so the final return is the return for the successful case.  (Just
because I think that’s how we usually do it, at least in the qemu code?)

[...]

> @@ -455,10 +452,9 @@ def file_path(*names, base_dir=test_dir):
>  def remote_filename(path):
>  if imgproto == 'file':
>  return path
> -elif imgproto == 'ssh':
> +if imgproto == 'ssh':

This seems like a weird thing to complain about for a coding style
check, but whatever.

(As in, I prefer the elif form)

>  return "ssh://%s@127.0.0.1:22%s" % (os.environ.get('USER'), path)
> -else:
> -raise Exception("Protocol %s not supported" % (imgproto))
> +raise Exception("Protocol %s not supported" % (imgproto))
>  
>  class VM(qtest.QEMUQtestMachine):
>  '''A QEMU VM'''

[...]

> @@ -756,12 +750,13 @@ def assert_block_path(self, root, path, expected_node, 
> graph=None):
>  assert node is not None, 'Cannot follow path %s%s' % (root, path)
>  
>  try:
> -node_id = next(edge['child'] for edge in graph['edges'] \
> - if edge['parent'] == node['id'] 
> and
> -edge['name'] == child_name)
> +node_id = next(edge['child'] for edge in graph['edges']
> +   if edge['parent'] == node['id'] and
> +   edge['name'] == child_name)

I don’t mind the if alignment, but I do mind not aligning the third line
to the “edge” above it (i.e. the third line is part of the condition, so
I’d align it to the “if” condition).

But then again it’s nothing new that I like to disagree with commonly
agreed-upon Python coding styles, so.

[...]

> @@ -891,13 +892,14 @@ def wait_until_completed(self, drive='drive0', 
> check_offset=True, wait=60.0,
>  self.assert_qmp(event, 'data/error', error)
>  self.assert_no_active_block_jobs()
>  return event
> -elif event['event'] == 'JOB_STATUS_CHANGE':
> +if event['event'] == 'JOB_STATUS_CHANGE':
>  self.assert_qmp(event, 'data/id', drive)
>  
>  def wait_ready(self, drive='drive0'):
>  '''Wait until a block job BLOCK_JOB_READY event'''
> -f = {'data': {'type': 'mirror', 'device': drive } }
> +f = {'data': {'type': 'mirror', 'device': drive}}
>  event = self.vm.event_wait(name='BLOCK_JOB_READY', match=f)
> +return event

Why not just “return self.vm.event_wait…”?

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v4 08/10] nbd/server: introduce NBDExtentArray

[Vladimir Sementsov-Ogievskiy]

26.02.2020 18:06, Eric Blake wrote:

On 2/5/20 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:

Introduce NBDExtentArray class, to handle extents list creation in more
controlled way and with fewer OUT parameters in functions.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  nbd/server.c | 210 +--
  1 file changed, 118 insertions(+), 92 deletions(-)




+
+/* Further modifications of the array after conversion are abandoned */
+static void nbd_extent_array_convert_to_be(NBDExtentArray *ea)
+{
+    int i;
+
+    assert(!ea->converted_to_be);


Comment is stale - further modifications after conversion are a bug that aborts 
the program, not abandoned.


I always thought that "abandoned" is something that must not be done, so the 
word works here. But I don't know English well).
May be:

"No further modifications of the array allowed after converting to BE."? Is it 
better?

Or just drop the comment.






  /*
- * Populate @extents from block status. Update @bytes to be the actual
- * length encoded (which may be smaller than the original), and update
- * @nb_extents to the number of extents used.
- *
- * Returns zero on success and -errno on bdrv_block_status_above failure.
+ * Add extent to NBDExtentArray. If extent can't be added (no available space),
+ * return -1.
+ * For safety, when returning -1 for the first time, .can_add is set to false,
+ * further call to nbd_extent_array_add() will crash.


s/further call/so further calls/


+ * (to avoid the situation, when after failing to add an extent (returned -1),
+ * user miss this failure and add another extent, which is successfully added
+ * (array is full, but new extent may be squashed into the last one), then we
+ * have invalid array with skipped extent)


Long comment with nested ().  I'm not sure it adds much value, I think it can 
safely be dropped.  But if it is kept, I suggest:

(this ensures that after a failure, no further extents can accidentally change 
the bounds of the last extent in the array)


works for me




   */
-static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
-  uint64_t *bytes, NBDExtent *extents,
-  unsigned int *nb_extents)
+static int nbd_extent_array_add(NBDExtentArray *ea,
+    uint32_t length, uint32_t flags)
  {
-    uint64_t remaining_bytes = *bytes;
-    NBDExtent *extent = extents, *extents_end = extents + *nb_extents;
-    bool first_extent = true;
+    assert(ea->can_add);
+
+    if (!length) {
+    return 0;
+    }
+
+    /* Extend previous extent if flags are the same */
+    if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) {
+    uint64_t sum = (uint64_t)length + ea->extents[ea->count - 1].length;
+
+    if (sum <= UINT32_MAX) {
+    ea->extents[ea->count - 1].length = sum;
+    ea->total_length += length;
+    return 0;
+    }
+    }
+
+    if (ea->count >= ea->nb_alloc) {
+    ea->can_add = false;
+    return -1;
+    }
+
+    ea->total_length += length;
+    ea->extents[ea->count] = (NBDExtent) {.length = length, .flags = flags};
+    ea->count++;
-    assert(*nb_extents);
-    while (remaining_bytes) {
+    return 0;
+}


Looks like you properly addressed my concerns from v3.

Comment changes are trivial, so you can add:

Reviewed-by: Eric Blake 



Thanks!

--
Best regards,
Vladimir

Re: [PATCH 08/20] hw/xen/xen_pt_load_rom: Remove unused includes

[Anthony PERARD]

On Mon, Oct 14, 2019 at 03:29:42PM +0100, Paul Durrant wrote:
> On Mon, 14 Oct 2019 at 15:27, Philippe Mathieu-Daudé  
> wrote:
> >
> > xen_pt_load_rom.c does not use any of these includes, remove them.
> >
> > Signed-off-by: Philippe Mathieu-Daudé 
> 
> Reviewed-by: Paul Durrant 

Hi,

I've added this patch to a pull requests for the xen.

Cheers,

-- 
Anthony PERARD

Re: [PATCH 1/2] iotests: add JobRunner class

[Max Reitz]

On 26.02.20 18:58, John Snow wrote:
> 
> 
> On 2/26/20 6:18 AM, Max Reitz wrote:
>> On 26.02.20 01:44, John Snow wrote:
>>> The idea is that instead of increasing the arguments to job_run all the
>>> time, create a more general-purpose job runner that can be subclassed to
>>> do interesting things with.
>>>
>>> Signed-off-by: John Snow 
>>> ---
>>>  tests/qemu-iotests/255|   9 +-
>>>  tests/qemu-iotests/257|  12 ++-
>>>  tests/qemu-iotests/287|  19 +++-
>>>  tests/qemu-iotests/iotests.py | 176 --
>>>  4 files changed, 158 insertions(+), 58 deletions(-)
>>
>> I like it!
>>
> 
> High praise!
> 
>> [...]
>>
>>> diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
>>> index 0ab58dc011..f06e6ff084 100755
>>> --- a/tests/qemu-iotests/287
>>> +++ b/tests/qemu-iotests/287
>>> @@ -165,13 +165,22 @@ def test_bitmap_populate(config):
>>>  if not config.disabled:
>>>  ebitmap.dirty_group(2)
>>>  
>>> +
>>> +class TestJobRunner(iotests.JobRunner):
>>> +def on_pending(self, event):
>>> +if config.mid_writes:
>>> +perform_writes(drive0, 2)
>>> +if not config.disabled:
>>> +ebitmap.dirty_group(2)
>>
>> I actually prefer inlining the pre_finalize() functions (over calling
>> the existing one), but then we can also remove the original function. :)
>>
> 
> Not sure I understand you correctly. You're saying you prefer this
> strategy where I inline the logic vs others where I call out to a function?

Yes.

> If so, I agree if only for purity -- the function looks and acts like a
> callback instead of a callback-that-calls-another-callback.

That was my thinking.

>>> +super().on_pending(event)
>>> +
>>>  job = populate(drive0, 'target', 'bitpop0')
>>>  assert job['return'] == {'return': {}}
>>> -vm.run_job(job['id'],
>>> -   auto_dismiss=job['auto-dismiss'],
>>> -   auto_finalize=job['auto-finalize'],
>>> -   pre_finalize=pre_finalize,
>>> -   cancel=config.cancel)
>>> +job_runner = TestJobRunner(vm, job['id'],
>>> +   auto_dismiss=job['auto-dismiss'],
>>> +   auto_finalize=job['auto-finalize'],
>>> +   cancel=config.cancel)
>>> +job_runner.run()
>>>  log('')
>>>  
>>>  
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 3390fab021..37a8b4d649 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -460,6 +460,130 @@ def remote_filename(path):
>>>  else:
>>>  raise Exception("Protocol %s not supported" % (imgproto))
>>>  
>>> +
>>> +class JobRunner:
>>
>> [...]
>>
>>> +def on_ready(self, event):
>>> +if self.logging:
>>> +self._vm.qmp_log('job-complete', id=self._id)
>>> +else:
>>> +self._vm.qmp('job-complete', id=self._id)
>>
>> I suppose this is a bug fix.  (The old version always called qmp_log.)
>>
> 
> Technically yes. It was needed for 040.
> 
>> But what about adding a do_qmp method to JobRunner that does the
>> “if self.logging { self._vm.qmp_log() } else { self._vm.qmp }” part so
>> we don’t have to inline that everywhere?
>>
>> Max
>>
> 
> I'll just clean up the logging series I had to do it at a more
> fundamental level.

OK.  So you’re looking to basically get VM.qmp() to log automatically if
necessary?  (or maybe qmp_log() to not log unless necessary)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 5/6] qmp.py: change event_wait to use a dict

[Vladimir Sementsov-Ogievskiy]

25.02.2020 3:56, John Snow wrote:

It's easier to work with than a list of tuples, because we can check the
keys for membership.

Signed-off-by: John Snow 
---
  python/qemu/machine.py| 10 +-
  tests/qemu-iotests/040| 12 ++--
  tests/qemu-iotests/260|  5 +++--
  tests/qemu-iotests/iotests.py | 16 
  4 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index 183d8f3d38..748de5f322 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -476,21 +476,21 @@ def event_wait(self, name, timeout=60.0, match=None):
  timeout: QEMUMonitorProtocol.pull_event timeout parameter.
  match: Optional match criteria. See event_match for details.
  """
-return self.events_wait([(name, match)], timeout)
+return self.events_wait({name: match}, timeout)
  
  def events_wait(self, events, timeout=60.0):

  """
  events_wait waits for and returns a named event from QMP with a 
timeout.
  
-events: a sequence of (name, match_criteria) tuples.

+events: a mapping containing {name: match_criteria}.
  The match criteria are optional and may be None.
  See event_match for details.
  timeout: QEMUMonitorProtocol.pull_event timeout parameter.
  """
  def _match(event):
-for name, match in events:
-if event['event'] == name and self.event_match(event, match):
-return True
+name = event['event']
+if name in events:
+return self.event_match(event, events[name])
  return False
  
  # Search cached events

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 32c82b4ec6..90b59081ff 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -485,12 +485,12 @@ class TestErrorHandling(iotests.QMPTestCase):
  
  def run_job(self, expected_events, error_pauses_job=False):

  match_device = {'data': {'device': 'job0'}}
-events = [
-('BLOCK_JOB_COMPLETED', match_device),
-('BLOCK_JOB_CANCELLED', match_device),
-('BLOCK_JOB_ERROR', match_device),
-('BLOCK_JOB_READY', match_device),
-]
+events = {
+'BLOCK_JOB_COMPLETED': match_device,
+'BLOCK_JOB_CANCELLED': match_device,
+'BLOCK_JOB_ERROR': match_device,
+'BLOCK_JOB_READY': match_device,
+}
  
  completed = False

  log = []
diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260
index 30c0de380d..b2fb045ddd 100755
--- a/tests/qemu-iotests/260
+++ b/tests/qemu-iotests/260
@@ -65,8 +65,9 @@ def test(persistent, restart):
  
  vm.qmp_log('block-commit', device='drive0', top=top,

 filters=[iotests.filter_qmp_testfiles])
-ev = vm.events_wait((('BLOCK_JOB_READY', None),
- ('BLOCK_JOB_COMPLETED', None)))
+ev = vm.events_wait({
+'BLOCK_JOB_READY': None,
+'BLOCK_JOB_COMPLETED': None })


may be better to keep it 2-lines. Or, otherwise, put closing "})" on a separate 
line too.


  log(filter_qmp_event(ev))
  if (ev['event'] == 'BLOCK_JOB_COMPLETED'):
  vm.shutdown()
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5d2990a0e4..3390fab021 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -604,14 +604,14 @@ def run_job(self, job, auto_finalize=True, 
auto_dismiss=False,
  """
  match_device = {'data': {'device': job}}
  match_id = {'data': {'id': job}}
-events = [
-('BLOCK_JOB_COMPLETED', match_device),
-('BLOCK_JOB_CANCELLED', match_device),
-('BLOCK_JOB_ERROR', match_device),
-('BLOCK_JOB_READY', match_device),
-('BLOCK_JOB_PENDING', match_id),
-('JOB_STATUS_CHANGE', match_id)
-]
+events = {
+'BLOCK_JOB_COMPLETED': match_device,
+'BLOCK_JOB_CANCELLED': match_device,
+'BLOCK_JOB_ERROR': match_device,
+'BLOCK_JOB_READY': match_device,
+'BLOCK_JOB_PENDING': match_id,
+'JOB_STATUS_CHANGE': match_id,
+}
  error = None
  while True:
  ev = filter_qmp_event(self.events_wait(events, timeout=wait))




Not sure that I like new interface more (neither that it is faster), but it 
works too:
Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 4/6] iotests: add hmp helper with logging

[Vladimir Sementsov-Ogievskiy]

25.02.2020 3:56, John Snow wrote:

Just a mild cleanup while I was here.

Signed-off-by: John Snow


Great!

Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 3/6] iotests: move bitmap helpers into their own file

[Vladimir Sementsov-Ogievskiy]

25.02.2020 3:56, John Snow wrote:

Signed-off-by: John Snow 
---
  tests/qemu-iotests/257| 110 +---
  tests/qemu-iotests/bitmaps.py | 131 ++
  2 files changed, 132 insertions(+), 109 deletions(-)
  create mode 100644 tests/qemu-iotests/bitmaps.py

diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
index 004a433b8b..2a81f9e30c 100755
--- a/tests/qemu-iotests/257
+++ b/tests/qemu-iotests/257
@@ -24,120 +24,12 @@ import os
  
  import iotests

  from iotests import log, qemu_img
+from bitmaps import EmulatedBitmap, GROUPS


Clean code movement, no changes. If test passes, it should be correct :)

The only thing: I'd prefer not exporting global variables and use 
bitmaps.GROUPS instead (even then, it's not very good interface but..)

with or without it:
Reviewed-by: Vladimir Sementsov-Ogievskiy 

  
  SIZE = 64 * 1024 * 1024

  GRANULARITY = 64 * 1024
  
  
-class Pattern:

-def __init__(self, byte, offset, size=GRANULARITY):
-self.byte = byte
-self.offset = offset
-self.size = size
-
-def bits(self, granularity):
-lower = self.offset // granularity
-upper = (self.offset + self.size - 1) // granularity
-return set(range(lower, upper + 1))
-
-
-class PatternGroup:
-"""Grouping of Pattern objects. Initialize with an iterable of Patterns."""
-def __init__(self, patterns):
-self.patterns = patterns
-
-def bits(self, granularity):
-"""Calculate the unique bits dirtied by this pattern grouping"""
-res = set()
-for pattern in self.patterns:
-res |= pattern.bits(granularity)
-return res
-
-
-GROUPS = [
-PatternGroup([
-# Batch 0: 4 clusters
-Pattern('0x49', 0x000),
-Pattern('0x6c', 0x010),   # 1M
-Pattern('0x6f', 0x200),   # 32M
-Pattern('0x76', 0x3ff)]), # 64M - 64K
-PatternGroup([
-# Batch 1: 6 clusters (3 new)
-Pattern('0x65', 0x000),   # Full overwrite
-Pattern('0x77', 0x00f8000),   # Partial-left (1M-32K)
-Pattern('0x72', 0x2008000),   # Partial-right (32M+32K)
-Pattern('0x69', 0x3fe)]), # Adjacent-left (64M - 128K)
-PatternGroup([
-# Batch 2: 7 clusters (3 new)
-Pattern('0x74', 0x001),   # Adjacent-right
-Pattern('0x69', 0x00e8000),   # Partial-left  (1M-96K)
-Pattern('0x6e', 0x2018000),   # Partial-right (32M+96K)
-Pattern('0x67', 0x3fe,
-2*GRANULARITY)]), # Overwrite [(64M-128K)-64M)
-PatternGroup([
-# Batch 3: 8 clusters (5 new)
-# Carefully chosen such that nothing re-dirties the one cluster
-# that copies out successfully before failure in Group #1.
-Pattern('0xaa', 0x001,
-3*GRANULARITY),   # Overwrite and 2x Adjacent-right
-Pattern('0xbb', 0x00d8000),   # Partial-left (1M-160K)
-Pattern('0xcc', 0x2028000),   # Partial-right (32M+160K)
-Pattern('0xdd', 0x3fc)]), # New; leaving a gap to the right
-]
-
-
-class EmulatedBitmap:
-def __init__(self, granularity=GRANULARITY):
-self._bits = set()
-self.granularity = granularity
-
-def dirty_bits(self, bits):
-self._bits |= set(bits)
-
-def dirty_group(self, n):
-self.dirty_bits(GROUPS[n].bits(self.granularity))
-
-def clear(self):
-self._bits = set()
-
-def clear_bits(self, bits):
-self._bits -= set(bits)
-
-def clear_bit(self, bit):
-self.clear_bits({bit})
-
-def clear_group(self, n):
-self.clear_bits(GROUPS[n].bits(self.granularity))
-
-@property
-def first_bit(self):
-return sorted(self.bits)[0]
-
-@property
-def bits(self):
-return self._bits
-
-@property
-def count(self):
-return len(self.bits)
-
-def compare(self, qmp_bitmap):
-"""
-Print a nice human-readable message checking that a bitmap as reported
-by the QMP interface has as many bits set as we expect it to.
-"""
-
-name = qmp_bitmap.get('name', '(anonymous)')
-log("= Checking Bitmap {:s} =".format(name))
-
-want = self.count
-have = qmp_bitmap['count'] // qmp_bitmap['granularity']
-
-log("expecting {:d} dirty sectors; have {:d}. {:s}".format(
-want, have, "OK!" if want == have else "ERROR!"))
-log('')
-
-
  class Drive:
  """Represents, vaguely, a drive attached to a VM.
  Includes format, graph, and device information."""
diff --git a/tests/qemu-iotests/bitmaps.py b/tests/qemu-iotests/bitmaps.py
new file mode 100644
index 00..522fc25171
--- /dev/null
+++ b/tests/qemu-iotests/bitmaps.py
@@ -0,0 +1,131 @@
+# Bitmap-related helper utilities
+#
+# Copyright (c) 2020 John Snow for Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GN

Re: [PATCH 2/6] qmp: expose block-dirty-bitmap-populate

[Vladimir Sementsov-Ogievskiy]

25.02.2020 3:56, John Snow wrote:

This is a new job-creating command.

Signed-off-by: John Snow 
---
  qapi/block-core.json  | 18 ++
  qapi/transaction.json |  2 ++
  blockdev.c| 78 +++
  3 files changed, 98 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index df1797681a..a8be1fb36b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2293,6 +2293,24 @@
  '*auto-finalize': 'bool',
  '*auto-dismiss': 'bool' } }
  
+##

+# @block-dirty-bitmap-populate:
+#
+# Creates a new job that writes a pattern into a dirty bitmap.
+#
+# Since: 5.0
+#
+# Example:
+#
+# -> { "execute": "block-dirty-bitmap-populate",
+#  "arguments": { "node": "drive0", "target": "bitmap0",
+# "job-id": "job0", "pattern": "allocate-top" } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'block-dirty-bitmap-populate', 'boxed': true,
+  'data': 'BlockDirtyBitmapPopulate' }
+
  ##
  # @BlockDirtyBitmapSha256:
  #
diff --git a/qapi/transaction.json b/qapi/transaction.json
index 04301f1be7..28521d5c7f 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -50,6 +50,7 @@
  # - @block-dirty-bitmap-enable: since 4.0
  # - @block-dirty-bitmap-disable: since 4.0
  # - @block-dirty-bitmap-merge: since 4.0
+# - @block-dirty-bitmap-populate: since 5.0
  # - @blockdev-backup: since 2.3
  # - @blockdev-snapshot: since 2.5
  # - @blockdev-snapshot-internal-sync: since 1.7
@@ -67,6 +68,7 @@
 'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
 'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
 'block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
+   'block-dirty-bitmap-populate': 'BlockDirtyBitmapPopulate',
 'blockdev-backup': 'BlockdevBackup',
 'blockdev-snapshot': 'BlockdevSnapshot',
 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
diff --git a/blockdev.c b/blockdev.c
index 011dcfec27..33c0e35399 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2314,6 +2314,67 @@ static void 
block_dirty_bitmap_remove_commit(BlkActionState *common)
  bdrv_release_dirty_bitmap(state->bitmap);
  }
  
+static void block_dirty_bitmap_populate_prepare(BlkActionState *common, Error **errp)


over80 line (not the only)


+{
+BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
common);


At first glance using *Backup* looks like a mistake. May be rename it, or at 
least add a comment.


+BlockDirtyBitmapPopulate *bitpop;
+BlockDriverState *bs;
+AioContext *aio_context;
+BdrvDirtyBitmap *bmap = NULL;
+int job_flags = JOB_DEFAULT;
+
+assert(common->action->type == 
TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_POPULATE);
+bitpop = common->action->u.block_dirty_bitmap_populate.data;
+
+bs = bdrv_lookup_bs(bitpop->node, bitpop->node, errp);
+if (!bs) {
+return;
+}
+
+aio_context = bdrv_get_aio_context(bs);
+aio_context_acquire(aio_context);
+state->bs = bs;
+
+bmap = bdrv_find_dirty_bitmap(bs, bitpop->name);


Could we use block_dirty_bitmap_lookup ?


+if (!bmap) {
+error_setg(errp, "Bitmap '%s' could not be found", bitpop->name);
+return;


aio context lock leaked


+}
+
+/* Paired with .clean() */
+bdrv_drained_begin(state->bs);
+
+if (!bitpop->has_on_error) {
+bitpop->on_error = BLOCKDEV_ON_ERROR_REPORT;
+}
+if (!bitpop->has_auto_finalize) {
+bitpop->auto_finalize = true;
+}
+if (!bitpop->has_auto_dismiss) {
+bitpop->auto_dismiss = true;
+}
+
+if (!bitpop->auto_finalize) {
+job_flags |= JOB_MANUAL_FINALIZE;
+}
+if (!bitpop->auto_dismiss) {
+job_flags |= JOB_MANUAL_DISMISS;
+}
+
+state->job = bitpop_job_create(
+bitpop->job_id,
+bs,
+bmap,
+bitpop->pattern,
+bitpop->on_error,
+job_flags,
+NULL, NULL,
+common->block_job_txn,
+errp);
+
+aio_context_release(aio_context);
+}
+
  static void abort_prepare(BlkActionState *common, Error **errp)
  {
  error_setg(errp, "Transaction aborted using Abort action");
@@ -2397,6 +2458,13 @@ static const BlkActionOps actions[] = {
  .commit = block_dirty_bitmap_remove_commit,
  .abort = block_dirty_bitmap_remove_abort,
  },
+[TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_POPULATE] = {
+.instance_size = sizeof(BlockdevBackupState),
+.prepare = block_dirty_bitmap_populate_prepare,
+.commit = blockdev_backup_commit,
+.abort = blockdev_backup_abort,
+.clean = blockdev_backup_clean,
+},
  /* Where are transactions for MIRROR, COMMIT and STREAM?
   * Although these blockjobs use transaction callbacks like the backup job,
   * these jobs do not necessarily adhere to transaction semantics.
@@ -3225,6 +3293,16 @@ void qmp_block_dirty_bitmap_merge(const char *node, 
const char *target,
  do

Re: [PATCH v1 8/8] iotests: 287: add qcow2 compression type test

[Vladimir Sementsov-Ogievskiy]

27.02.2020 10:29, Denis Plotnikov wrote:

The test checks fulfilling qcow2 requiriements for the compression
type feature and zstd compression type operability.

Signed-off-by: Denis Plotnikov 
---
  tests/qemu-iotests/287 | 123 +
  tests/qemu-iotests/287.out |  41 +
  tests/qemu-iotests/group   |   1 +
  3 files changed, 165 insertions(+)
  create mode 100755 tests/qemu-iotests/287
  create mode 100644 tests/qemu-iotests/287.out

diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
new file mode 100755
index 00..41b916f690
--- /dev/null
+++ b/tests/qemu-iotests/287
@@ -0,0 +1,123 @@
+#!/usr/bin/env bash
+#
+# Test case for an image using zstd compression
+#
+# 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 .
+#
+
+# creator
+owner=dplotni...@virtuozzo.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# standard environment
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+P=`echo "$QEMU_PROG" | sed "s/qemu-system-x86_64//"`
+
+grep "CONFIG_ZSTD=y" "$P"../config-host.mak >/dev/null
+RES=$?


Hmm. This will not work for other architectures and for
out of tree builds. Also, it checks config but not current
binary (they may be out of sync, or even unrelated).

Probably better try to create image with zstd compression type
and handle expected error.



+if (($RES)); then
+_notrun "ZSTD is disabled in the current configuration"
+fi
+
+# Test: when compression is zlib the incompatible is unset
+echo
+echo "=== Testing compression type incompatible bit setting for zlib ==="
+echo
+
+_make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: when compression differs from zlib the incompatible bit is set
+echo
+echo "=== Testing compression type incompatible bit setting for zstd ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Test: an image can't be openned if compression type is zlib and


opened


+#   incompatible feature compression type is set
+echo
+echo "=== Testing zlib with incompatible bit set  ==="
+echo
+
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 3
+# to make sure the bit was actually set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi


may be better to instead keep error output and just check it..


+
+# Test: an image can't be openned if compression type is NOT zlib and
+#   incompatible feature compression type is UNSET
+echo
+echo "=== Testing zstd with incompatible bit unset  ==="
+echo
+
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$PYTHON qcow2.py "$TEST_IMG" set-header incompatible_features 0
+# to make sure the bit was actually unset
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+$QEMU_IMG info "$TEST_IMG" 2>1 1>/dev/null
+if (($?==0)); then
+echo "Error: The image openned successfully. The image must not be openned"
+fi
+# Test: check compression type values
+echo
+echo "=== Testing compression type values  ==="
+echo
+# zlib=0
+IMGOPTS='compression_type=zlib' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# zstd=1
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+od -j104 -N1 -An -vtu1 "$TEST_IMG"
+
+# Test: using zstd compression, write to and read from an image
+echo
+echo "=== Testing reading and writing with zstd ==="
+echo
+
+CLUSTER_SIZE=65536
+IMGOPTS='compression_type=zstd' _make_test_img 64M
+$QEMU_IO -c "write -c 0 64k " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 0 10 " "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "read -v 65530 8" "$TEST_IMG" | _filter_qemu_io


Hmm output depends on default pattern. Better use "write -c -P 0x11 0 64k"
 (or any pattern you want), to make it explicit.


+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/287.ou

Re: [PATCH v1 6/8] iotests: add "compression type" for test output matching

[Vladimir Sementsov-Ogievskiy]

27.02.2020 13:04, Vladimir Sementsov-Ogievskiy wrote:

27.02.2020 10:29, Denis Plotnikov wrote:

Affected tests: 049, 060, 061, 065, 144, 182, 242, 255

After adding the compression type feature for qcow2, the compression type
is reported on image quering.

Add the corresponding values of the "compression type" for the tests' output
matching.


And this and the following patch.

Ideally, patch should not break any iotests. This means that all iotest updates
should be merged to the patch which changes their output. Probably, you can 
split
behavior-changing patch, to reduce iotest-updates per patch, but anyway, big 
patch
with a lot of iotests updates is better than patch which breaks iotests.


Or we can try to reduce behavior changes in case of zlib:

- keep header small in case of zlib, not adding zero field
- don't add feature table entry, if compress type is zlib
- don't report compression type on quering, if it is zlib

- then, all iotests output will be saved. And, then, if we need, we can change
these theree points one-by-one, updating iotests outputs. But I doubt that we
really need it, compatible behavior seems good enough.





Signed-off-by: Denis Plotnikov 
---
  tests/qemu-iotests/049.out | 102 ++---
  tests/qemu-iotests/060.out |   1 +
  tests/qemu-iotests/061.out |   6 +++
  tests/qemu-iotests/065 |  20 +---
  tests/qemu-iotests/144.out |   4 +-
  tests/qemu-iotests/182.out |   2 +-
  tests/qemu-iotests/242.out |   5 ++
  tests/qemu-iotests/255.out |   8 +--
  8 files changed, 82 insertions(+), 66 deletions(-)

diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
index affa55b341..a5cfba1756 100644
--- a/tests/qemu-iotests/049.out
+++ b/tests/qemu-iotests/049.out
@@ -4,90 +4,90 @@ QA output created by 049
  == 1. Traditional size parameter ==
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1024
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1024b
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1k
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1K
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1M
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1048576 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1048576 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1G
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1073741824 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1073741824 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1T
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1099511627776 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1099511627776 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1024.0
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1024.0b
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1.5k
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1536 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1536 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1.5K
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1536 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1536 cluster_size=65536 
lazy_refcounts=off refc

Re: [PATCH v1 6/8] iotests: add "compression type" for test output matching

[Vladimir Sementsov-Ogievskiy]

27.02.2020 10:29, Denis Plotnikov wrote:

Affected tests: 049, 060, 061, 065, 144, 182, 242, 255

After adding the compression type feature for qcow2, the compression type
is reported on image quering.

Add the corresponding values of the "compression type" for the tests' output
matching.


And this and the following patch.

Ideally, patch should not break any iotests. This means that all iotest updates
should be merged to the patch which changes their output. Probably, you can 
split
behavior-changing patch, to reduce iotest-updates per patch, but anyway, big 
patch
with a lot of iotests updates is better than patch which breaks iotests.



Signed-off-by: Denis Plotnikov 
---
  tests/qemu-iotests/049.out | 102 ++---
  tests/qemu-iotests/060.out |   1 +
  tests/qemu-iotests/061.out |   6 +++
  tests/qemu-iotests/065 |  20 +---
  tests/qemu-iotests/144.out |   4 +-
  tests/qemu-iotests/182.out |   2 +-
  tests/qemu-iotests/242.out |   5 ++
  tests/qemu-iotests/255.out |   8 +--
  8 files changed, 82 insertions(+), 66 deletions(-)

diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
index affa55b341..a5cfba1756 100644
--- a/tests/qemu-iotests/049.out
+++ b/tests/qemu-iotests/049.out
@@ -4,90 +4,90 @@ QA output created by 049
  == 1. Traditional size parameter ==
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1024

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1024b

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1k

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1K

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1M

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1048576 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1048576 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1G

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1073741824 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1073741824 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1T

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1099511627776 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1099511627776 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1024.0

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1024.0b

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1024 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1.5k

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1536 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1536 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1.5K

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1536 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1536 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1.5M

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1572864 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1572864 cluster_size=65536 
lazy_refcounts=off refcount_bits=16 compression_type=zlib
  
  qemu-img create -f qcow2 TEST_DIR/t.qcow2 1.5G

-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=1610612736 cluster_size=65536 

Re: [PATCH v1 5/8] iotests: fix header size, feature table size and backing file offset

[Vladimir Sementsov-Ogievskiy]

27.02.2020 10:29, Denis Plotnikov wrote:

Affected tests: 031, 036, 061

Because of adding the compression type feature, some size values in the
qcow2 v3 header are changed:

header_size +=8: 1 byte compression type
  7 bytes padding
feature_table += 48: incompatible feture compression type

backing_file_offset += 56 (8 + 48 -> header_change + fature_table_change)

Change the values for the test output comparison accordingly.


Again, this should be merged to the patch, which actually break iotests.



Signed-off-by: Denis Plotnikov 
---
  tests/qemu-iotests/031.out | 14 +++---
  tests/qemu-iotests/036.out |  4 ++--
  tests/qemu-iotests/061.out | 28 ++--
  3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/tests/qemu-iotests/031.out b/tests/qemu-iotests/031.out
index d535e407bc..ed51afe9ce 100644
--- a/tests/qemu-iotests/031.out
+++ b/tests/qemu-iotests/031.out
@@ -113,11 +113,11 @@ incompatible_features []
  compatible_features   []
  autoclear_features[]
  refcount_order4
-header_length 104
+header_length 112
  
  Header extension:

  magic 0x6803f857
-length192
+length240
  data  
  
  Header extension:

@@ -146,11 +146,11 @@ incompatible_features []
  compatible_features   []
  autoclear_features[]
  refcount_order4
-header_length 104
+header_length 112
  
  Header extension:

  magic 0x6803f857
-length192
+length240
  data  
  
  Header extension:

@@ -164,7 +164,7 @@ No errors were found on the image.
  
  magic 0x514649fb

  version   3
-backing_file_offset   0x178
+backing_file_offset   0x1b0
  backing_file_size 0x17
  cluster_bits  16
  size  67108864
@@ -179,7 +179,7 @@ incompatible_features []
  compatible_features   []
  autoclear_features[]
  refcount_order4
-header_length 104
+header_length 112
  
  Header extension:

  magic 0xe2792aca
@@ -188,7 +188,7 @@ data  'host_device'
  
  Header extension:

  magic 0x6803f857
-length192
+length240
  data  
  
  Header extension:

diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
index 0b52b934e1..fb509f6357 100644
--- a/tests/qemu-iotests/036.out
+++ b/tests/qemu-iotests/036.out
@@ -26,7 +26,7 @@ compatible_features   []
  autoclear_features[63]
  Header extension:
  magic 0x6803f857
-length192
+length240
  data  
  
  
@@ -38,7 +38,7 @@ compatible_features   []

  autoclear_features[]
  Header extension:
  magic 0x6803f857
-length192
+length240
  data  
  
  *** done

diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index 8b3091a412..cea7fedfdc 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -22,11 +22,11 @@ incompatible_features []
  compatible_features   [0]
  autoclear_features[]
  refcount_order4
-header_length 104
+header_length 112
  
  Header extension:

  magic 0x6803f857
-length192
+length240
  data  
  
  magic 0x514649fb

@@ -80,11 +80,11 @@ incompatible_features []
  compatible_features   [0]
  autoclear_features[]
  refcount_order4
-header_length 104
+header_length 112
  
  Header extension:

  magic 0x6803f857
-length192
+length240
  data  
  
  magic 0x514649fb

@@ -136,11 +136,11 @@ incompatible_features [0]
  compatible_features   [0]
  autoclear_features[]
  refcount_order4
-header_length 104
+header_length 112
  
  Header extension:

  magic 0x6803f857
-length192
+length240
  data  
  
  ERROR cluster 5 refcount=0 reference=1

@@ -191,11 +191,11 @@ incompatible_features []
  compatible_features   [42]
  autoclear_features[42]
  refcount_order4
-header_length 104
+header_length 112
  
  Header extension:

  magic 0x6803f857
-length192
+length240
  data  
  
  magic 0x514649fb

@@ -260,11 +260,11 @@ incompatible_features []
  compatib

Re: [PATCH v1 4/8] iotests: filter out compression_type

[Vladimir Sementsov-Ogievskiy]

27.02.2020 10:29, Denis Plotnikov wrote:

After adding compression type feature to qcow2 format, qemu framework
commands reporting the image settingd, e.g. "qemu-img create", started
reporting the compression type for the image which breaks the iotests
output matching.

To fix it, add compression_type=zlib to the list of filtered image parameters.


So, the first patch breaks iotests? Than it should be merged into first patch or
moved before it, to not break git bisect.



Signed-off-by: Denis Plotnikov 
---
  tests/qemu-iotests/common.filter | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 3f8ee3e5f7..c6962d199c 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -152,7 +152,8 @@ _filter_img_create()
  -e "s# refcount_bits=[0-9]\\+##g" \
  -e "s# key-secret=[a-zA-Z0-9]\\+##g" \
  -e "s# iter-time=[0-9]\\+##g" \
--e "s# force_size=\\(on\\|off\\)##g"
+-e "s# force_size=\\(on\\|off\\)##g" \
+-e "s# compression_type=zlib##g"
  }
  
  _filter_img_info()





--
Best regards,
Vladimir

Re: [PATCH v1 3/8] qcow2: add zstd cluster compression

[Vladimir Sementsov-Ogievskiy]

27.02.2020 10:29, Denis Plotnikov wrote:

zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is the only compression
method available.

The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G

The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.

compress cmd:
   time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
   src.img [zlib|zstd]_compressed.img
decompress cmd
   time ./qemu-img convert -O qcow2
   [zlib|zstd]_compressed.img uncompressed.img

compression   decompression
  zlib   zstd   zlib zstd

real 65.5   16.3 (-75 %)1.9  1.6 (-16 %)
user 65.0   15.85.3  2.5
sys   3.30.22.0  2.0

Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G

Signed-off-by: Denis Plotnikov 
---
  block/qcow2-threads.c  | 122 +
  block/qcow2.c  |   7 +++
  configure  |  29 ++
  docs/interop/qcow2.txt |  18 ++
  qapi/block-core.json   |   3 +-
  5 files changed, 178 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 1c128e9840..e942c4d7e5 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
  #define ZLIB_CONST
  #include 
  
+#ifdef CONFIG_ZSTD

+#include 
+#include 
+#endif
+
  #include "qcow2.h"
  #include "block/thread-pool.h"
  #include "crypto.h"
@@ -164,6 +169,113 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
dest_size,
  return ret;
  }
  
+#ifdef CONFIG_ZSTD

+
+#define ZSTD_LEN_BUF 4
+
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success


This doesn't match qcow2_co_compress definition. You should return 0 on success.


+ *  -ENOMEM destination buffer is not enough to store compressed data
+ *  -EIOon any other error
+ */
+
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+size_t ret;
+
+/*
+ * steal ZSTD_LEN_BUF bytes in the very beginng of the buffer


beginning


+ * to store compressed chunk size
+ */
+char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
+
+/*
+ * sanity check that we can store the compressed data length,
+ * and there is some space left for the compressor buffer
+ */
+if (dest_size <= ZSTD_LEN_BUF) {
+return -ENOMEM;
+}
+
+dest_size -= ZSTD_LEN_BUF;
+
+ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
+
+if (ZSTD_isError(ret)) {
+if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
+return -ENOMEM;
+} else {
+return -EIO;
+}
+}
+
+/* paraniod sanity check that we can store the commpressed size */
+if (ret > UINT_MAX) {
+return -ENOMEM;
+}


I'd use UINT32_MAX, possibly even more paranoid)


+
+/* store the compressed chunk size in the very beginning of the buffer */
+stl_be_p(dest, ret);
+
+return ret + ZSTD_LEN_BUF;


return 0;


+}
+
+/*
+ * qcow2_zstd_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  -EIO on any error
+ */
+


extra empty line.


+static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
+{
+/*
+ * zstd decompress wants to know the exact length of the data.
+ * For that purpose, on compression, the length is stored in
+ * the very beginning of the compressed buffer
+ */
+size_t s_size;
+const char *s_buf = ((const char *) src) + ZSTD_LEN_BUF;
+
+/*
+ * sanity check that we can read 4 byte the content length and
+ * and there is some content to decompress
+ */
+if (src_size <= ZSTD_LEN_BUF) {
+return -EIO;
+}
+
+s_size = ldl_be_p(src);
+
+/* sanity check that the buffer is big enough to read the content from */
+if (src_size - ZSTD_LEN_BUF < s_size) {
+return -EIO;
+}
+
+if (ZSTD_isError(
+ZSTD_decompress(dest, dest_size, s_buf, s_size))) {


hmm, it fi

Re: [PATCH v2 3/3] qemu-img: Deprecate use of -b without -F

[Ján Tomko]

On a Wednesday in 2020, Eric Blake wrote:

Creating an image that requires format probing of the backing image is
inherently unsafe (we've had several CVEs over the years based on
probes leaking information to the guest on a subsequent boot).  If our
probing algorithm ever changes, or if other tools like libvirt
determine a different probe result than we do, then subsequent use of
that backing file under a different format will present corrupted data
to the guest.  Start a deprecation clock so that future qemu-img can
refuse to create unsafe backing chains that would rely on probing.

However, there is one time where probing is safe: if we probe raw,
then it is safe to record that implicitly in the image (but we still
warn, as it's better to teach the user to supply -F always than to
make them guess when it is safe).

iotest 114 specifically wants to create an unsafe image for later
amendment rather than defaulting to our new default of recording a
probed format, so it needs an update.

Signed-off-by: Eric Blake 
---
qemu-deprecated.texi   | 15 +++
block.c| 21 -
qemu-img.c |  8 +++-
tests/qemu-iotests/114 |  4 ++--
tests/qemu-iotests/114.out |  1 +
5 files changed, 45 insertions(+), 4 deletions(-)



This seems to affect code paths that are used even outside of qemu-img,
should the commit message mention it?

Jano


signature.asc
Description: PGP signature

Re: [PATCH v2 2/3] block: Add support to warn on backing file change without format

[Ján Tomko]

On a Wednesday in 2020, Eric Blake wrote:

For now, this is a mechanical addition; all callers pass false. But
the next patch will use it to improve 'qemu-img rebase -u' when
selecting a backing file with no format.

Signed-off-by: Eric Blake 
---
include/block/block.h |  4 ++--
block.c   | 13 ++---
block/qcow2.c |  2 +-
block/stream.c|  2 +-
blockdev.c|  3 ++-
qemu-img.c|  4 ++--
6 files changed, 18 insertions(+), 10 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] iotests: Specify explicit backing format where sensible

[Ján Tomko]

On a Wednesday in 2020, Eric Blake wrote:

There are many existing qcow2 images that specify a backing file but
no format.  This has been the source of CVEs in the past, but has
become more prominent of a problem now that libvirt has switched to
-blockdev.  With older -drive, at least the probing was always done by
qemu (so the only risk of a changed format between successive boots of
a guest was if qemu was upgraded and probed differently).  But with
newer -blockdev, libvirt must specify a format; if libvirt guesses raw
where the image was formatted, this results in data corruption visible
to the guest; conversely, if libvirt guesses qcow2 where qemu was
using raw, this can result in potential security holes, so modern
libvirt instead refuses to use images without explicit backing format.

The change in libvirt to reject images without explicit backing format
has pointed out that a number of tools have been far too reliant on
probing in the past.  It's time to set a better example in our own
iotests of properly setting this parameter.

iotest calls to create, rebase, convert, and amend are all impacted to
some degree.  It's a bit annoying that we are inconsistent on command
line - while all of those accept -o backing_file=...,backing_fmt=...,
the shortcuts are different: create and rebase have -b and -F, convert
has -B but no -F, and amend has no shortcuts.

Signed-off-by: Eric Blake 
---


[...]

Test #225 still uses -b without a format:

./check -vmdk 225
QEMU  -- 
"/home/jtomko/work/qemu/build/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64"
 -nodefaults -display none -accel qtest
QEMU_IMG  -- "/home/jtomko/work/qemu/build/tests/qemu-iotests/../../qemu-img" 
QEMU_IO   -- "/home/jtomko/work/qemu/build/tests/qemu-iotests/../../qemu-io"  --cache writeback --aio threads -f vmdk
QEMU_NBD  -- "/home/jtomko/work/qemu/build/tests/qemu-iotests/../../qemu-nbd" 
IMGFMT-- vmdk

IMGPROTO  -- file
PLATFORM  -- Linux/x86_64 lpt 5.4.18-200.fc31.x86_64
TEST_DIR  -- /home/jtomko/work/qemu/build/tests/qemu-iotests/scratch
SOCK_DIR  -- /tmp/tmp.OQIdhLcITP
SOCKET_SCM_HELPER -- 
/home/jtomko/work/qemu/build/tests/qemu-iotests/socket_scm_helper

225  fail   [10:02:31] [10:02:32]output mismatch 
(see 225.out.bad)
--- /home/jtomko/work/qemu/tests/qemu-iotests/225.out   2018-09-07 
17:21:39.633931691 +0200
+++ /home/jtomko/work/qemu/build/tests/qemu-iotests/225.out.bad 2020-02-27 
10:02:32.362755677 +0100
@@ -1,6 +1,7 @@
 QA output created by 225
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
 Formatting 'TEST_DIR/t.IMGFMT.not_base', fmt=IMGFMT size=1048576
+qemu-img: warning: Deprecated use of backing file without explicit backing 
format (detected format of IMGFMT)
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.IMGFMT.base
 
 === Testing fitting VMDK backing image ===

Failures: 225
Failed 1 of 1 iotests


diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index aa911d266a13..322e31e2cd93 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -32,8 +32,12 @@ class TestSingleDrive(iotests.QMPTestCase):

def setUp(self):
iotests.create_image(backing_img, TestSingleDrive.image_len)
-qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
backing_img, mid_img)
-qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % 
mid_img, test_img)
+qemu_img('create', '-f', iotests.imgfmt,
+ '-o', 'backing_file=%s' % backing_img,
+ '-F', 'raw', mid_img)
+qemu_img('create', '-f', iotests.imgfmt,
+ '-o', 'backing_file=%s' % mid_img,
+ '-F', iotests.imgfmt, test_img)


Consider not mixing shortcuts with -o options.


qemu_io('-f', 'raw', '-c', 'write -P 0x1 0 512', backing_img)
qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 524288 512', mid_img)
self.vm = iotests.VM().add_drive("blkdebug::" + test_img,


With test #225 fixed:
Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature

Re: [RFC PATCH v3 19/27] qcow2: Add subcluster support to expand_zero_clusters_in_l1()

[Max Reitz]

On 26.02.20 18:19, Alberto Garcia wrote:
> On Fri 21 Feb 2020 03:57:27 PM CET, Max Reitz wrote:
>> As noted in v2, this function is only called when downgrading qcow2
>> images to v2.  It kind of made sense to just call set_l2_bitmap() in
>> v2, but now with the if () conditional...  I suppose it may make more
>> sense to assert that the image does not have subclusters at the
>> beginning of the function and be done with it.
> 
> Hmmm, you're right.
> 
>> OTOH, well, this does make ensuring that we have subcluster “support”
>> everywhere a bit easier because this way all set_l2_entry() calls are
>> accompanied by an “if (subclusters) { set_l2_bitmap() }” part.
> 
> Another alternative is to assert that the image does not have subcluster
> but still leave a comment after both set_l2_entry() calls explaining why
> there's no need to touch the bitmap.

Sounds good.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 1/3] iotests: Specify explicit backing format where sensible

[Ján Tomko]

On a Thursday in 2020, Peter Krempa wrote:

On Wed, Feb 26, 2020 at 20:39:26 -0600, Eric Blake wrote:

There are many existing qcow2 images that specify a backing file but
no format.  This has been the source of CVEs in the past, but has
become more prominent of a problem now that libvirt has switched to
-blockdev.  With older -drive, at least the probing was always done by
qemu (so the only risk of a changed format between successive boots of
a guest was if qemu was upgraded and probed differently).  But with
newer -blockdev, libvirt must specify a format; if libvirt guesses raw
where the image was formatted, this results in data corruption visible
to the guest; conversely, if libvirt guesses qcow2 where qemu was
using raw, this can result in potential security holes, so modern
libvirt instead refuses to use images without explicit backing format.

The change in libvirt to reject images without explicit backing format
has pointed out that a number of tools have been far too reliant on
probing in the past.  It's time to set a better example in our own
iotests of properly setting this parameter.

iotest calls to create, rebase, convert, and amend are all impacted to
some degree.  It's a bit annoying that we are inconsistent on command
line - while all of those accept -o backing_file=...,backing_fmt=...,
the shortcuts are different: create and rebase have -b and -F, convert
has -B but no -F, and amend has no shortcuts.

Signed-off-by: Eric Blake 
---


[...]


 113 files changed, 414 insertions(+), 338 deletions(-)

diff --git a/tests/qemu-iotests/017 b/tests/qemu-iotests/017
index 0a4b854e6520..585512bb296b 100755
--- a/tests/qemu-iotests/017
+++ b/tests/qemu-iotests/017
@@ -66,7 +66,7 @@ echo "Creating test image with backing file"
 echo

 TEST_IMG=$TEST_IMG_SAVE
-_make_test_img -b "$TEST_IMG.base" 6G
+_make_test_img -b "$TEST_IMG.base" -F $IMGFMT 6G



My understanding of the intricacies of the qemu-iotest suite is not good
enoug to be able to review this patch. Specifically $IMGFMT in this
instance is also used in the '-f' switch of qemu-img in _make_test_img
and I don't know if it's expected for the backing file to share the
format.


IMGFMT is also used for the earlier creation of the base image and
I did not see it changing in any of the tests.

Jano


signature.asc
Description: PGP signature

Re: [PATCH v1 2/8] qcow2: rework the cluster compression routine

[Vladimir Sementsov-Ogievskiy]

27.02.2020 10:29, Denis Plotnikov wrote:

The patch enables processing the image compression type defined
for the image and chooses an appropriate method for image clusters
(de)compression.

Signed-off-by: Denis Plotnikov 
---
  block/qcow2-threads.c | 77 +++
  1 file changed, 63 insertions(+), 14 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 77bb578cdf..1c128e9840 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -74,7 +74,9 @@ typedef struct Qcow2CompressData {
  } Qcow2CompressData;
  
  /*

- * qcow2_compress()
+ * qcow2_zlib_compress()
+ *
+ * Compress @src_size bytes of data using zlib compression method
   *
   * @dest - destination buffer, @dest_size bytes
   * @src - source buffer, @src_size bytes
@@ -83,8 +85,8 @@ typedef struct Qcow2CompressData {
   *  -ENOMEM destination buffer is not enough to store compressed data
   *  -EIOon any other error
   */
-static ssize_t qcow2_compress(void *dest, size_t dest_size,
-  const void *src, size_t src_size)
+static ssize_t qcow2_zlib_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
  {
  ssize_t ret;
  z_stream strm;
@@ -119,19 +121,19 @@ static ssize_t qcow2_compress(void *dest, size_t 
dest_size,
  }
  
  /*

- * qcow2_decompress()
+ * qcow2_zlib_decompress()
   *
   * Decompress some data (not more than @src_size bytes) to produce exactly
- * @dest_size bytes.
+ * @dest_size bytes using zlib compression method
   *
   * @dest - destination buffer, @dest_size bytes
   * @src - source buffer, @src_size bytes
   *
   * Returns: 0 on success
- *  -1 on fail
+ *  -EIO on failure
   */
-static ssize_t qcow2_decompress(void *dest, size_t dest_size,
-const void *src, size_t src_size)
+static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
  {
  int ret = 0;
  z_stream strm;
@@ -144,7 +146,7 @@ static ssize_t qcow2_decompress(void *dest, size_t 
dest_size,
  
  ret = inflateInit2(&strm, -12);

  if (ret != Z_OK) {
-return -1;
+return -EIO;
  }
  
  ret = inflate(&strm, Z_FINISH);

@@ -154,7 +156,7 @@ static ssize_t qcow2_decompress(void *dest, size_t 
dest_size,
   * @src buffer may be processed partly (because in qcow2 we know size 
of
   * compressed data with precision of one sector)
   */
-ret = -1;
+ret = -EIO;
  }
  
  inflateEnd(&strm);

@@ -189,20 +191,67 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
  return arg.ret;
  }
  
+/*

+ * qcow2_co_compress()
+ *
+ * Compress @src_size bytes of data using the compression
+ * method defined by the image compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  a negative error code on failure


Hmm, it's default semantics and it used without any comment for most of C
functions, so I don't think we need the comment. As well as dest/src
argument names are obvious enough. Still, I'm not against too.



+ */
  ssize_t coroutine_fn
  qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
const void *src, size_t src_size)
  {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_compress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_compress;
+break;
+
+default:
+return -ENOTSUP;


it can't be anything other. Maybe, better abort() ?


+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
  }
  
+/*

+ * qcow2_co_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using the compression method defined by the image
+ * compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  a negative error code on failure
+ */
  ssize_t coroutine_fn
  qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
  const void *src, size_t src_size)
  {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_decompress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_decompress;
+break;
+
+default:
+return -ENOTSUP;
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
  }
  
  




--
Best regards,
Vladimir

Re: [PATCH v1 1/8] qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

27.02.2020 10:29, Denis Plotnikov wrote:

The patch adds some preparation parts for incompatible compression type
feature to Qcow2 that indicates which allow to use different compression
methods for image clusters (de)compressing.

It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image, and thus,
for all image clusters.

The goal of the feature is to add support of other compression methods
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.

The default compression is ZLIB. Images created with ZLIB compression type
are backward compatible with older qemu versions.

Signed-off-by: Denis Plotnikov 
---
  block/qcow2.c | 105 ++
  block/qcow2.h |  31 ---
  include/block/block_int.h |   1 +
  qapi/block-core.json  |  22 +++-
  4 files changed, 150 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3c754f616b..2ccb2cabd1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c


Please, add to .git/config:

[diff]
orderFile = /path/to/qemu/scripts/git.orderfile

This will force git format-patch to sort files in more comfortable order 
(header changes first, etc).


@@ -1242,6 +1242,50 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
  return ret;
  }
  
+static int validate_compression_type(BDRVQcow2State *s, Error **errp)

+{
+/*
+ * Sanity check
+ * according to qcow2 spec, the compression type is 1-byte field
+ * but in BDRVQcow2State the compression_type is enum sizeof(int)
+ * so, the max compression_type value is 255.
+ */
+if (s->compression_type > 0xff) {
+error_setg(errp, "qcow2: compression type value is too big");
+return -EINVAL;
+}
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+break;
+
+default:
+error_setg(errp, "qcow2: unknown compression type: %u",
+   s->compression_type);
+return -ENOTSUP;
+}



honestly, I think that just

if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB) {
  error out
}

is enough, and don't see why to check > 0xff in separate..

But it works as is.


+
+/*
+ * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
+ * the incompatible feature flag must be set
+ */
+if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
+if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) {
+error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must not be set");
+return -EINVAL;
+}
+} else {



This is unreachable now.. But it's OK as preparation for further patches I 
think.


+if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
+error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must be set");
+return -EINVAL;
+}
+}
+
+return 0;
+}
+
  /* Called with s->lock held.  */
  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
@@ -1357,6 +1401,26 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
  s->compatible_features  = header.compatible_features;
  s->autoclear_features   = header.autoclear_features;
  
+/*

+ * Handle compression type
+ * Older qcow2 images don't contain the compression type header.
+ * Distinguish them by the header length and use
+ * the only valid (default) compression type in that case
+ */
+if (header.header_length > offsetof(QCowHeader, compression_type)) {
+/*
+ * don't deal with endians since compression_type is 1 byte long
+ */



this comment would be more appropriate above, where be-to-cpu transformation is 
done,
as actually previous "s->autoclear_features   = header.autoclear_features" 
doesn't
deal with endians too.


+s->compression_type = header.compression_type;
+} else {
+s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+}
+
+ret = validate_compression_type(s, errp);
+if (ret) {
+goto fail;
+}
+
  if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
  void *feature_table = NULL;
  qcow2_read_extensions(bs, header.header_length, ext_end,
@@ -2720,6 +2784,12 @@ int qcow2_update_header(BlockDriverState *bs)
  total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
  refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 3);
  
+ret = validate_compression_type(s, NULL);

+
+if (ret) {
+goto fail;
+}
+
  *header = (QCowHeader) {
  /* Version 2 fields */
  .magic