Re: [PATCH 6/6] qga/commands-posix: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

24.03.2020 23:03, Eric Blake wrote:

On 3/24/20 10:36 AM, Vladimir Sementsov-Ogievskiy wrote:

local_err is used several times in guest_suspend(). Setting non-NULL
local_err will crash, so let's zero it after freeing. Also fix possible
leak of local_err in final if().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  qga/commands-posix.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 93474ff770..cc69b82704 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1773,6 +1773,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
  }
  error_free(local_err);
+    local_err = NULL;


Let's show this with more context.


static void guest_suspend(SuspendMode mode, Error **errp)
{
    Error *local_err = NULL;
    bool mode_supported = false;

    if (systemd_supports_mode(mode, _err)) {


Hmm - we have an even earlier bug that needs fixing.  Note that systemd_supports_mode() returns a 
bool AND conditionally sets errp.  But it is inconsistent: it has the following table of actions 
based on the results of run_process_child() on "systemctl status" coupled with the man 
page on "systemctl status" return values:
-1 (unable to run systemctl) -> errp set, return false
0 (unit is active) -> errp left unchanged, return false
1 (unit not failed) -> errp left unchanged, return true
2 (unused) -> errp left unchanged, return true
3 (unit not active) -> errp left unchanged, return true
4 (no such unit) -> errp left unchanged, return false
5+ (unexpected from systemctl) -> errp left unchanged, return false

But the comments in systemd_supports_mode() claim that ANY status < 4 (other 
than -1, which means we did not run systemctl) should count as the service 
existing, even though the most common status is 3.  If our comment is to be 
believed, then we should return true, not false, for status 0.

Now, back to _this_ function:


    mode_supported = true;
    systemd_suspend(mode, _err);


Okay - if we get here (whether from status 1-3, or with systemd_supports_mode 
fixed to support status 0-3), local_err is still unset prior to calling 
systemd_suspend(), and we are guaranteed that after the call, either we 
suspended successfully or local_err is now set.


    }

    if (!local_err) {
    return;
    }


So if returned, we succeeded at systemd_suspend, and there is nothing further 
to do; but if we get past that point, we don't know if it was 
systemd_supports_mode that failed or systemd_suspend that failed, and we don't 
know if local_err is set.


No, we know that is set, as we check exactly this and return if not set.





    error_free(local_err);
+    local_err = NULL;


Yet, we blindly throw away local_err, without trying to report it.  If that's 
the case, then WHY are we passing in local_err?  Wouldn't it be better to pass 
in NULL (we really don't care about the error message), and/or fix 
systemd_suspend() to return a bool just like systemd_supports_mode, and/or fix 
systemd_supports_mode to guarantee that it sets errp when returning false?



I agree that this is a strange function and its logic is weird. But I don't 
know what the logic should be. My patch is still valid to just fix obvious 
use-after-free and possible leak. It doesn't fix the logic.


--
Best regards,
Vladimir

[PATCH v9 13/14] iotests: Mark verify functions as private

[John Snow]

Mark the verify functions as "private" with a leading underscore, to
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 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index fbca0f2a40..a356dd1b45 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1000,7 +1000,7 @@ def case_notrun(reason):
 open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
 '[case not run] ' + reason + '\n')
 
-def verify_image_format(supported_fmts=(), unsupported_fmts=()):
+def _verify_image_format(supported_fmts=(), unsupported_fmts=()):
 assert not (supported_fmts and unsupported_fmts)
 
 if 'generic' in supported_fmts and \
@@ -1014,7 +1014,7 @@ def verify_image_format(supported_fmts=(), 
unsupported_fmts=()):
 if not_sup or (imgfmt in unsupported_fmts):
 notrun('not suitable for this image format: %s' % imgfmt)
 
-def verify_protocol(supported=(), unsupported=()):
+def _verify_protocol(supported=(), unsupported=()):
 assert not (supported and unsupported)
 
 if 'generic' in supported:
@@ -1024,7 +1024,7 @@ def verify_protocol(supported=(), unsupported=()):
 if not_sup or (imgproto in unsupported):
 notrun('not suitable for this protocol: %s' % imgproto)
 
-def verify_platform(supported=(), unsupported=()):
+def _verify_platform(supported=(), unsupported=()):
 if any((sys.platform.startswith(x) for x in unsupported)):
 notrun('not suitable for this OS: %s' % sys.platform)
 
@@ -1032,11 +1032,11 @@ def verify_platform(supported=(), unsupported=()):
 if not any((sys.platform.startswith(x) for x in supported)):
 notrun('not suitable for this OS: %s' % sys.platform)
 
-def verify_cache_mode(supported_cache_modes=()):
+def _verify_cache_mode(supported_cache_modes=()):
 if supported_cache_modes and (cachemode not in supported_cache_modes):
 notrun('not suitable for this cache mode: %s' % cachemode)
 
-def verify_aio_mode(supported_aio_modes=()):
+def _verify_aio_mode(supported_aio_modes=()):
 if supported_aio_modes and (aiomode not in supported_aio_modes):
 notrun('not suitable for this aio mode: %s' % aiomode)
 
@@ -1163,11 +1163,11 @@ def execute_setup_common(supported_fmts: 
Collection[str] = (),
 sys.stderr.write('Please run this test via the "check" script\n')
 sys.exit(os.EX_USAGE)
 
-verify_image_format(supported_fmts, unsupported_fmts)
-verify_protocol(supported_protocols, unsupported_protocols)
-verify_platform(supported=supported_platforms)
-verify_cache_mode(supported_cache_modes)
-verify_aio_mode(supported_aio_modes)
+_verify_image_format(supported_fmts, unsupported_fmts)
+_verify_protocol(supported_protocols, unsupported_protocols)
+_verify_platform(supported=supported_platforms)
+_verify_cache_mode(supported_cache_modes)
+_verify_aio_mode(supported_aio_modes)
 
 debug = '-d' in sys.argv
 if debug:
-- 
2.21.1

[PATCH v9 10/14] iotests: add hmp helper with logging

[John Snow]

Just a mild cleanup while I was here.

Although we now have universal qmp logging on or off, many existing
callers to hmp functions don't expect that output to be logged, which
causes quite a few changes in the test output.

For now, just offer a use_log parameter.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 30 +-
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e12d6e533e..4faee06f14 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -540,25 +540,29 @@ def add_incoming(self, addr):
 self._args.append(addr)
 return self
 
-def pause_drive(self, drive, event=None):
-'''Pause drive r/w operations'''
+def hmp(self, command_line: str, use_log: bool = False):
+cmd = 'human-monitor-command'
+kwargs = {'command-line': command_line}
+if use_log:
+return self.qmp_log(cmd, **kwargs)
+else:
+return self.qmp(cmd, **kwargs)
+
+def pause_drive(self, drive: str, event: Optional[str] = None) -> None:
+"""Pause drive r/w operations"""
 if not event:
 self.pause_drive(drive, "read_aio")
 self.pause_drive(drive, "write_aio")
 return
-self.qmp('human-monitor-command',
- command_line='qemu-io %s "break %s bp_%s"'
- % (drive, event, drive))
+self.hmp(f'qemu-io {drive} "break {event} bp_{drive}"')
 
-def resume_drive(self, drive):
-self.qmp('human-monitor-command',
- command_line='qemu-io %s "remove_break bp_%s"'
- % (drive, drive))
+def resume_drive(self, drive: str) -> None:
+"""Resume drive r/w operations"""
+self.hmp(f'qemu-io {drive} "remove_break bp_{drive}"')
 
-def hmp_qemu_io(self, drive, cmd):
-'''Write to a given drive using an HMP command'''
-return self.qmp('human-monitor-command',
-command_line='qemu-io %s "%s"' % (drive, cmd))
+def hmp_qemu_io(self, drive: str, cmd: str, use_log: bool = False) -> None:
+"""Write to a given drive using an HMP command"""
+return self.hmp(f'qemu-io {drive} "{cmd}"', use_log=use_log)
 
 def flatten_qmp_object(self, obj, output=None, basestr=''):
 if output is None:
-- 
2.21.1

[PATCH v9 09/14] iotests: limit line length to 79 chars

[John Snow]

79 is the PEP8 recommendation. This recommendation works well for
reading patch diffs in TUI email clients.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 64 +++
 tests/qemu-iotests/pylintrc   |  6 +++-
 2 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3a049ece5b..e12d6e533e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -80,9 +80,11 @@
 def qemu_img(*args):
 '''Run qemu-img and return the exit code'''
 devnull = open('/dev/null', 'r+')
-exitcode = subprocess.call(qemu_img_args + list(args), stdin=devnull, 
stdout=devnull)
+exitcode = subprocess.call(qemu_img_args + list(args),
+   stdin=devnull, stdout=devnull)
 if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n'
+ % (-exitcode, ' '.join(qemu_img_args + list(args
 return exitcode
 
 def ordered_qmp(qmsg, conv_keys=True):
@@ -121,7 +123,8 @@ def qemu_img_verbose(*args):
 '''Run qemu-img without suppressing its output and return the exit code'''
 exitcode = subprocess.call(qemu_img_args + list(args))
 if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n'
+ % (-exitcode, ' '.join(qemu_img_args + list(args
 return exitcode
 
 def qemu_img_pipe(*args):
@@ -132,7 +135,8 @@ def qemu_img_pipe(*args):
 universal_newlines=True)
 exitcode = subp.wait()
 if exitcode < 0:
-sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' 
'.join(qemu_img_args + list(args
+sys.stderr.write('qemu-img received signal %i: %s\n'
+ % (-exitcode, ' '.join(qemu_img_args + list(args
 return subp.communicate()[0]
 
 def qemu_img_log(*args):
@@ -162,7 +166,8 @@ def qemu_io(*args):
 universal_newlines=True)
 exitcode = subp.wait()
 if exitcode < 0:
-sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' 
'.join(args)))
+sys.stderr.write('qemu-io received signal %i: %s\n'
+ % (-exitcode, ' '.join(args)))
 return subp.communicate()[0]
 
 def qemu_io_log(*args):
@@ -284,10 +289,13 @@ def filter_test_dir(msg):
 def filter_win32(msg):
 return win32_re.sub("", msg)
 
-qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* \([0-9\/.inf]* 
[EPTGMKiBbytes]*\/sec and [0-9\/.inf]* ops\/sec\)")
+qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* "
+r"\([0-9\/.inf]* [EPTGMKiBbytes]*\/sec "
+r"and [0-9\/.inf]* ops\/sec\)")
 def filter_qemu_io(msg):
 msg = filter_win32(msg)
-return qemu_io_re.sub("X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)", 
msg)
+return qemu_io_re.sub("X ops; XX:XX:XX.X "
+  "(XXX YYY/sec and XXX ops/sec)", msg)
 
 chown_re = re.compile(r"chown [0-9]+:[0-9]+")
 def filter_chown(msg):
@@ -339,7 +347,9 @@ def filter_img_info(output, filename):
 line = line.replace(filename, 'TEST_IMG') \
.replace(imgfmt, 'IMGFMT')
 line = re.sub('iters: [0-9]+', 'iters: XXX', line)
-line = re.sub('uuid: [-a-f0-9]+', 'uuid: 
----', line)
+line = re.sub('uuid: [-a-f0-9]+',
+  'uuid: ----',
+  line)
 line = re.sub('cid: [0-9]+', 'cid: XX', line)
 lines.append(line)
 return '\n'.join(lines)
@@ -537,11 +547,13 @@ def pause_drive(self, drive, event=None):
 self.pause_drive(drive, "write_aio")
 return
 self.qmp('human-monitor-command',
- command_line='qemu-io %s "break %s bp_%s"' % (drive, event, 
drive))
+ command_line='qemu-io %s "break %s bp_%s"'
+ % (drive, event, drive))
 
 def resume_drive(self, drive):
 self.qmp('human-monitor-command',
- command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
drive))
+ command_line='qemu-io %s "remove_break bp_%s"'
+ % (drive, drive))
 
 def hmp_qemu_io(self, drive, cmd):
 '''Write to a given drive using an HMP command'''
@@ -801,16 +813,18 @@ def dictpath(self, d, path):
 idx = int(idx)
 
 if not isinstance(d, dict) or component not in d:
-self.fail('failed path traversal for "%s" in "%s"' % (path, 
str(d)))
+self.fail(f'failed path traversal for "{path}" in "{d}"')
 d = d[component]
 
 if m:

[PATCH v9 14/14] iotests: use python logging for iotests.log()

[John Snow]

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.


An extended note on python logging:

A NullHandler is added to `qemu.iotests` to stop output from being
generated if this code is used as a library without configuring logging.
A NullHandler is only needed at the root, so a duplicate handler is not
needed for `qemu.iotests.diff_io`.

When logging is not configured, messages at the 'WARNING' levels or
above are printed with default settings. The NullHandler stops this from
occurring, which is considered good hygiene for code used as a library.

See https://docs.python.org/3/howto/logging.html#library-config

When logging is actually enabled (always at the behest of an explicit
call by a client script), a root logger is implicitly created at the
root, which allows messages to propagate upwards and be handled/emitted
from the root logger with default settings.

When we want iotest logging, we attach a handler to the
qemu.iotests.diff_io logger and disable propagation to avoid possible
double-printing.

For more information on python logging infrastructure, I highly
recommend downloading the pip package `logging_tree`, which provides
convenient visualizations of the hierarchical logging configuration
under different circumstances.

See https://pypi.org/project/logging_tree/ for more information.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/030|  4 +--
 tests/qemu-iotests/155|  2 +-
 tests/qemu-iotests/245|  1 +
 tests/qemu-iotests/245.out| 24 
 tests/qemu-iotests/iotests.py | 52 +++
 5 files changed, 45 insertions(+), 38 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index aa911d266a..104e3cee1b 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -411,8 +411,8 @@ class TestParallelOps(iotests.QMPTestCase):
 result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
 self.assert_qmp(result, 'return', {})
 
-self.vm.run_job(job='drive0', auto_dismiss=True, use_log=False)
-self.vm.run_job(job='node4', auto_dismiss=True, use_log=False)
+self.vm.run_job(job='drive0', auto_dismiss=True)
+self.vm.run_job(job='node4', auto_dismiss=True)
 self.assert_no_active_block_jobs()
 
 # Test a block-stream and a block-commit job in parallel
diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155
index 571bce9de4..cb371d4649 100755
--- a/tests/qemu-iotests/155
+++ b/tests/qemu-iotests/155
@@ -188,7 +188,7 @@ class MirrorBaseClass(BaseClass):
 
 self.assert_qmp(result, 'return', {})
 
-self.vm.run_job('mirror-job', use_log=False, auto_finalize=False,
+self.vm.run_job('mirror-job', auto_finalize=False,
 pre_finalize=self.openBacking, auto_dismiss=True)
 
 def testFull(self):
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index 1001275a44..4f5f0bb901 100755
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -1027,5 +1027,6 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 self.run_test_iothreads(None, 'iothread0')
 
 if __name__ == '__main__':
+iotests.activate_logging()
 iotests.main(supported_fmts=["qcow2"],
  supported_protocols=["file"])
diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out
index 682b93394d..4b33dcaf5c 100644
--- a/tests/qemu-iotests/245.out
+++ b/tests/qemu-iotests/245.out
@@ -1,17 +1,17 @@
+{"execute": "job-finalize", "arguments": {"id": "commit0"}}
+{"return": {}}
+{"data": {"id": "commit0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "commit0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-finalize", "arguments": {"id": "stream0"}}
+{"return": {}}
+{"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", 
"timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, 
"type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
 .
 --
 

[PATCH v9 07/14] iotests: drop pre-Python 3.4 compatibility code

[John Snow]

We no longer need to accommodate 3.4, drop this code.
(The lines were > 79 chars and it stood out.)

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 2a0e22a3db..c93c6b4557 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -359,12 +359,9 @@ def log(msg, filters=(), indent=None):
 for flt in filters:
 msg = flt(msg)
 if isinstance(msg, (dict, list)):
-# Python < 3.4 needs to know not to add whitespace when 
pretty-printing:
-separators = (', ', ': ') if indent is None else (',', ': ')
 # Don't sort if it's already sorted
 do_sort = not isinstance(msg, OrderedDict)
-print(json.dumps(msg, sort_keys=do_sort,
- indent=indent, separators=separators))
+print(json.dumps(msg, sort_keys=do_sort, indent=indent))
 else:
 print(msg)
 
-- 
2.21.1

[PATCH v9 11/14] iotests: add script_initialize

[John Snow]

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 
Reviewed-by: Max Reitz 
---
 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 | 75 +++
 37 files changed, 129 insertions(+), 81 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index b4a21bf7b7..852768f80a 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -382,8 +382,7 @@ def test_once(config, qemu_img=False):
 
 
 # Obviously we only work with the luks image format
-iotests.verify_image_format(supported_fmts=['luks'])
-iotests.verify_platform()
+iotests.script_initialize(supported_fmts=['luks'])
 
 # We need sudo in order to run cryptsetup to create
 # dm-crypt devices. This is safe to use on any
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 9dc1bd3510..8b1f720af4 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -21,8 +21,8 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'],
+  supported_platforms=['linux'])
 
 with iotests.FilePath('source.img') as source_img_path, \
  iotests.FilePath('dest.img') as dest_img_path, \
diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202
index 920a8683ef..e3900a44d1 100755
--- a/tests/qemu-iotests/202
+++ b/tests/qemu-iotests/202
@@ -24,8 +24,8 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'],
+  supported_platforms=['linux'])
 
 with iotests.FilePath('disk0.img') as disk0_img_path, \
  iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203
index 49eff5d405..4b4bd3307d 100755
--- a/tests/qemu-iotests/203
+++ b/tests/qemu-iotests/203
@@ -24,8 +24,8 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
-iotests.verify_platform(['linux'])
+iotests.script_initialize(supported_fmts=['qcow2'],
+  supported_platforms=['linux'])
 
 with iotests.FilePath('disk0.img') as disk0_img_path, \
  iotests.FilePath('disk1.img') as disk1_img_path, \
diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index e2b50ae24d..f42432a838 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -23,7 +23,7 @@
 import iotests
 from iotests import imgfmt
 
-iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2'])
 
 with iotests.FilePath('t.qcow2') as disk_path, \
  iotests.FilePath('t.qcow2.base') as backing_path, \
diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index 3d9c1208ca..a6621410da 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -24,8 +24,10 @@ import iotests
 import subprocess
 import re
 
-iotests.verify_image_format(supported_fmts=['raw'])
-iotests.verify_protocol(supported=['ssh'])
+iotests.script_initialize(
+supported_fmts=['raw'],
+supported_protocols=['ssh'],
+)
 
 def filter_hash(qmsg):
 def _filter(key, value):
diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
index 1c3fc8c7fd..6cb642f821 100755
--- a/tests/qemu-iotests/208
+++ b/tests/qemu-iotests/208
@@ -22,7 +22,7 @@
 
 import iotests
 

[PATCH v9 12/14] iotest 258: use script_main

[John Snow]

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

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 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'])
-- 
2.21.1

[PATCH v9 04/14] iotests: replace mutable list default args

[John Snow]

It's bad hygiene: if we modify this list, it will be modified across all
invocations.

(Remaining bad usages are fixed in a subsequent patch which changes the
function signature anyway.)

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0eccca88e0..20da488ad6 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -139,7 +139,7 @@ def qemu_img_log(*args):
 log(result, filters=[filter_testfiles])
 return result
 
-def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
+def img_info_log(filename, filter_path=None, imgopts=False, extra_args=()):
 args = ['info']
 if imgopts:
 args.append('--image-opts')
@@ -353,7 +353,7 @@ def _filter(_key, value):
 return value
 return filter_qmp(qmsg, _filter)
 
-def log(msg, filters=[], indent=None):
+def log(msg, filters=(), indent=None):
 '''Logs either a string message or a JSON serializable message (like QMP).
 If indent is provided, JSON serializable messages are pretty-printed.'''
 for flt in filters:
@@ -569,7 +569,7 @@ def get_qmp_events_filtered(self, wait=60.0):
 result.append(filter_qmp_event(ev))
 return result
 
-def qmp_log(self, cmd, filters=[], indent=None, **kwargs):
+def qmp_log(self, cmd, filters=(), indent=None, **kwargs):
 full_cmd = OrderedDict((
 ("execute", cmd),
 ("arguments", ordered_qmp(kwargs))
@@ -973,7 +973,7 @@ def case_notrun(reason):
 open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
 '[case not run] ' + reason + '\n')
 
-def verify_image_format(supported_fmts=[], unsupported_fmts=[]):
+def verify_image_format(supported_fmts=(), unsupported_fmts=()):
 assert not (supported_fmts and unsupported_fmts)
 
 if 'generic' in supported_fmts and \
@@ -987,7 +987,7 @@ def verify_image_format(supported_fmts=[], 
unsupported_fmts=[]):
 if not_sup or (imgfmt in unsupported_fmts):
 notrun('not suitable for this image format: %s' % imgfmt)
 
-def verify_protocol(supported=[], unsupported=[]):
+def verify_protocol(supported=(), unsupported=()):
 assert not (supported and unsupported)
 
 if 'generic' in supported:
@@ -1006,11 +1006,11 @@ def verify_platform(supported=None, unsupported=None):
 if not any((sys.platform.startswith(x) for x in supported)):
 notrun('not suitable for this OS: %s' % sys.platform)
 
-def verify_cache_mode(supported_cache_modes=[]):
+def verify_cache_mode(supported_cache_modes=()):
 if supported_cache_modes and (cachemode not in supported_cache_modes):
 notrun('not suitable for this cache mode: %s' % cachemode)
 
-def verify_aio_mode(supported_aio_modes=[]):
+def verify_aio_mode(supported_aio_modes=()):
 if supported_aio_modes and (aiomode not in supported_aio_modes):
 notrun('not suitable for this aio mode: %s' % aiomode)
 
@@ -1050,7 +1050,7 @@ def supported_formats(read_only=False):
 
 return supported_formats.formats[read_only]
 
-def skip_if_unsupported(required_formats=[], read_only=False):
+def skip_if_unsupported(required_formats=(), read_only=False):
 '''Skip Test Decorator
Runs the test if all the required formats are whitelisted'''
 def skip_test_decorator(func):
@@ -1101,11 +1101,11 @@ def execute_unittest(output, verbosity, debug):
 sys.stderr.write(out)
 
 def execute_test(test_function=None,
- supported_fmts=[],
+ supported_fmts=(),
  supported_platforms=None,
- supported_cache_modes=[], supported_aio_modes={},
- unsupported_fmts=[], supported_protocols=[],
- unsupported_protocols=[]):
+ supported_cache_modes=(), supported_aio_modes=(),
+ unsupported_fmts=(), supported_protocols=(),
+ unsupported_protocols=()):
 """Run either unittest or script-style tests."""
 
 # We are using TEST_DIR and QEMU_DEFAULT_MACHINE as proxies to
-- 
2.21.1

[PATCH v9 06/14] iotests: alphabetize standard imports

[John Snow]

I had to fix a merge conflict, so do this tiny harmless thing while I'm
here.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 20da488ad6..2a0e22a3db 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,19 +16,19 @@
 # along with this program.  If not, see .
 #
 
+import atexit
+from collections import OrderedDict
+import faulthandler
+import io
+import json
+import logging
 import os
 import re
+import signal
+import struct
 import subprocess
-import unittest
 import sys
-import struct
-import json
-import signal
-import logging
-import atexit
-import io
-from collections import OrderedDict
-import faulthandler
+import unittest
 
 # pylint: disable=import-error, wrong-import-position
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
-- 
2.21.1

[PATCH v9 05/14] iotests: add pylintrc file

[John Snow]

This allows others to get repeatable results with pylint. If you run
`pylint iotests.py`, you should see a 100% pass.

Signed-off-by: John Snow 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/pylintrc | 22 ++
 1 file changed, 22 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..8720b6a0de
--- /dev/null
+++ b/tests/qemu-iotests/pylintrc
@@ -0,0 +1,22 @@
+[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,
+no-else-return,
+too-many-lines,
+too-few-public-methods,
+too-many-arguments,
+too-many-locals,
+too-many-branches,
+too-many-public-methods,
+# These are temporary, and should be removed:
+missing-docstring,
+line-too-long,
-- 
2.21.1

[PATCH v9 02/14] iotests: don't use 'format' for drive_add

[John Snow]

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

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 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 886ae962ae..7f486e6c4b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -485,21 +485,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)
-- 
2.21.1

[PATCH v9 08/14] iotests: touch up log function signature

[John Snow]

Representing nested, recursive data structures in mypy is notoriously
difficult; the best we can reliably do right now is denote the atom
types as "Any" while describing the general shape of the data.

Regardless, this fully annotates the log() function.

Typing notes:

TypeVar is a Type variable that can optionally be constrained by a
sequence of possible types. This variable is bound per-invocation such
that the signature for filter=() requires that its callables take e.g. a
str and return a str.

Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index c93c6b4557..3a049ece5b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -28,6 +28,7 @@
 import struct
 import subprocess
 import sys
+from typing import (Any, Callable, Dict, Iterable, List, Optional, TypeVar)
 import unittest
 
 # pylint: disable=import-error, wrong-import-position
@@ -353,9 +354,16 @@ def _filter(_key, value):
 return value
 return filter_qmp(qmsg, _filter)
 
-def log(msg, filters=(), indent=None):
-'''Logs either a string message or a JSON serializable message (like QMP).
-If indent is provided, JSON serializable messages are pretty-printed.'''
+
+Msg = TypeVar('Msg', Dict[str, Any], List[Any], str)
+
+def log(msg: Msg,
+filters: Iterable[Callable[[Msg], Msg]] = (),
+indent: Optional[int] = None) -> None:
+"""
+Logs either a string message or a JSON serializable message (like QMP).
+If indent is provided, JSON serializable messages are pretty-printed.
+"""
 for flt in filters:
 msg = flt(msg)
 if isinstance(msg, (dict, list)):
-- 
2.21.1

[PATCH v9 03/14] iotests: ignore import warnings from pylint

[John Snow]

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.

Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
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 7f486e6c4b..0eccca88e0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,6 +30,7 @@
 from collections import OrderedDict
 import faulthandler
 
+# pylint: disable=import-error, wrong-import-position
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
 
-- 
2.21.1

[PATCH v9 00/14] iotests: use python logging

[John Snow]

This series uses python logging to enable output conditionally on
iotests.log(). We unify an initialization call (which also enables
debugging output for those tests with -d) and then make the switch
inside of iotests.

It will help alleviate the need to create logged/unlogged versions
of all the various helpers we have made.

Also, I got lost and accidentally delinted iotests while I was here.
Sorry about that. By version 9, it's now the overwhelming focus of
this series. No good deed, etc.

V9:

001/14:[] [-C] 'iotests: do a light delinting'
002/14:[] [--] 'iotests: don't use 'format' for drive_add'
003/14:[] [-C] 'iotests: ignore import warnings from pylint'
004/14:[] [--] 'iotests: replace mutable list default args'
005/14:[] [--] 'iotests: add pylintrc file'
006/14:[down]  'iotests: alphabetize standard imports'
007/14:[down]  'iotests: drop pre-Python 3.4 compatibility code'
008/14:[down]  'iotests: touch up log function signature'
009/14:[] [--] 'iotests: limit line length to 79 chars'
010/14:[down]  'iotests: add hmp helper with logging'
011/14:[0004] [FC] 'iotests: add script_initialize'
012/14:[] [--] 'iotest 258: use script_main'
013/14:[] [--] 'iotests: Mark verify functions as private'
014/14:[0001] [FC] 'iotests: use python logging for iotests.log()'

006: New.
007: Split from old patch.
008: Split from old patch; enhanced a little to justify its own patch.
010: New, pulled in from bitmap-populate series. Helps line length.
011: Reflow columns for long `typing` import list. (Kept RB.)
014: New blank line. (Kept RB.)

V8:
- Split out the little drop of Python 3.4 code. (Phil)
- Change line continuation styles (QEMU Memorial Choir)
- Rebase changes; remove use_log from more places, adjust test output.

V7:
- All delinting patches are now entirely front-loaded.
- Redid delinting to avoid "correcting" no-else-return statements.
- Moved more mutable list corrections into patch 4, to make it standalone.
- Moved pylintrc up to patch 5. Disabled no-else-return.
- Added patch 6 to require line length checks.
  (Some python 3.4 compatibility code is removed as a consequence.)
- Patch 7 changes slightly as a result of patch 4 changes.
- Added some logging explainer into patch 10.
  (Patch changes slightly because of patch 6.)

V6:
 - It's been so long since V5, let's just look at it anew.

John Snow (14):
  iotests: do a light delinting
  iotests: don't use 'format' for drive_add
  iotests: ignore import warnings from pylint
  iotests: replace mutable list default args
  iotests: add pylintrc file
  iotests: alphabetize standard imports
  iotests: drop pre-Python 3.4 compatibility code
  iotests: touch up log function signature
  iotests: limit line length to 79 chars
  iotests: add hmp helper with logging
  iotests: add script_initialize
  iotest 258: use script_main
  iotests: Mark verify functions as private
  iotests: use python logging for iotests.log()

 tests/qemu-iotests/030|   4 +-
 tests/qemu-iotests/055|   3 +-
 tests/qemu-iotests/149|   3 +-
 tests/qemu-iotests/155|   2 +-
 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/245|   1 +
 tests/qemu-iotests/245.out|  24 +--
 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|  10 +-
 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 | 356 --
 tests/qemu-iotests/pylintrc   |  26 +++
 43 files changed, 333 insertions(+), 221 deletions(-)
 create mode 100644 tests/qemu-iotests/pylintrc

-- 
2.21.1

[PATCH v9 01/14] iotests: do a light delinting

[John Snow]

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.

Signed-off-by: John Snow 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 83 ++-
 1 file changed, 43 insertions(+), 40 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7bc4934cd2..886ae962ae 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,11 +16,9 @@
 # along with this program.  If not, see .
 #
 
-import errno
 import os
 import re
 import subprocess
-import string
 import unittest
 import sys
 import struct
@@ -35,7 +33,7 @@
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
 
-assert sys.version_info >= (3,6)
+assert sys.version_info >= (3, 6)
 
 faulthandler.enable()
 
@@ -141,11 +139,11 @@ def qemu_img_log(*args):
 return result
 
 def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
-args = [ 'info' ]
+args = ['info']
 if imgopts:
 args.append('--image-opts')
 else:
-args += [ '-f', imgfmt ]
+args += ['-f', imgfmt]
 args += extra_args
 args.append(filename)
 
@@ -224,7 +222,7 @@ def cmd(self, cmd):
 # quit command is in close(), '\n' is added automatically
 assert '\n' not in cmd
 cmd = cmd.strip()
-assert cmd != 'q' and cmd != 'quit'
+assert cmd not in ('q', 'quit')
 self._p.stdin.write(cmd + '\n')
 self._p.stdin.flush()
 return self._read_output()
@@ -246,10 +244,8 @@ def qemu_nbd_early_pipe(*args):
 sys.stderr.write('qemu-nbd received signal %i: %s\n' %
  (-exitcode,
   ' '.join(qemu_nbd_args + ['--fork'] + list(args
-if exitcode == 0:
-return exitcode, ''
-else:
-return exitcode, subp.communicate()[0]
+
+return exitcode, subp.communicate()[0] if exitcode else ''
 
 def qemu_nbd_popen(*args):
 '''Run qemu-nbd in daemon mode and return the parent's exit code'''
@@ -313,7 +309,7 @@ def filter_qmp(qmsg, filter_fn):
 items = qmsg.items()
 
 for k, v in items:
-if isinstance(v, list) or isinstance(v, dict):
+if isinstance(v, (dict, list)):
 qmsg[k] = filter_qmp(v, filter_fn)
 else:
 qmsg[k] = filter_fn(k, v)
@@ -324,7 +320,7 @@ def filter_testfiles(msg):
 return msg.replace(prefix, 'TEST_DIR/PID-')
 
 def filter_qmp_testfiles(qmsg):
-def _filter(key, value):
+def _filter(_key, value):
 if is_str(value):
 return filter_testfiles(value)
 return value
@@ -350,7 +346,7 @@ def filter_imgfmt(msg):
 return msg.replace(imgfmt, 'IMGFMT')
 
 def filter_qmp_imgfmt(qmsg):
-def _filter(key, value):
+def _filter(_key, value):
 if is_str(value):
 return filter_imgfmt(value)
 return value
@@ -361,7 +357,7 @@ def log(msg, filters=[], indent=None):
 If indent is provided, JSON serializable messages are pretty-printed.'''
 for flt in filters:
 msg = flt(msg)
-if isinstance(msg, dict) or isinstance(msg, list):
+if isinstance(msg, (dict, list)):
 # Python < 3.4 needs to know not to add whitespace when 
pretty-printing:
 separators = (', ', ': ') if indent is None else (',', ': ')
 # Don't sort if it's already sorted
@@ -372,14 +368,14 @@ def log(msg, filters=[], indent=None):
 print(msg)
 
 class Timeout:
-def __init__(self, seconds, errmsg = "Timeout"):
+def __init__(self, seconds, errmsg="Timeout"):
 self.seconds = seconds
 self.errmsg = errmsg
 def __enter__(self):
 signal.signal(signal.SIGALRM, self.timeout)
 signal.setitimer(signal.ITIMER_REAL, self.seconds)
 return self
-def __exit__(self, type, value, traceback):
+def __exit__(self, exc_type, value, traceback):
 signal.setitimer(signal.ITIMER_REAL, 0)
 return False
 def timeout(self, signum, frame):
@@ -388,7 +384,7 @@ def timeout(self, signum, frame):
 def file_pattern(name):
 return "{0}-{1}".format(os.getpid(), name)
 
-class FilePaths(object):
+class FilePaths:
 """
 FilePaths is an auto-generated filename that cleans itself up.
 
@@ -535,11 +531,11 @@ def pause_drive(self, drive, event=None):
 self.pause_drive(drive, "write_aio")
 return
 self.qmp('human-monitor-command',
-command_line='qemu-io %s "break %s bp_%s"' % (drive, 
event, drive))
+ command_line='qemu-io %s "break %s bp_%s"' % (drive, event, 
drive))
 
 def resume_drive(self, drive):
 

Re: [PULL 0/2] Ide patches

[John Snow]

On 3/24/20 3:55 PM, John Snow wrote:
> The following changes since commit 736cf607e40674776d752acc201f565723e86045:
> 
>   Update version for v5.0.0-rc0 release (2020-03-24 17:50:00 +)
> 
> are available in the Git repository at:
> 
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
> 
> for you to fetch changes up to 51058b3b3bcbe62506cf191fca1c0d679bb80f2b:
> 
>   hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs() (2020-03-24 
> 15:52:16 -0400)
> 
> 
> Pull request: IDE
> 
> Admittedly the first one is not a crisis fix; but I think it's low-risk to
> include for rc1.
> 
> The second one is yours, and will shush coverity.
> 
> 
> 
> Peter Maydell (1):
>   hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()
> 
> Sven Schnelle (1):
>   fdc/i8257: implement verify transfer mode
> 
>  include/hw/isa/isa.h |  1 -
>  hw/block/fdc.c   | 61 +---
>  hw/dma/i8257.c   | 20 ++-
>  hw/ide/sii3112.c |  8 +++---
>  4 files changed, 35 insertions(+), 55 deletions(-)
> 

NACK. Mark Cave-Ayland is sending additional fixes.

--js

[PATCH for-5.0 3/3] cmd646-ide: use qdev gpio rather than qemu_allocate_irqs()

[Mark Cave-Ayland]

This prevents the memory from qemu_allocate_irqs() from being leaked which
can in some cases be spotted by Coverity (CID 1421984).

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/cmd646.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 699f25824d..c254631485 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -249,8 +249,8 @@ static void cmd646_pci_config_write(PCIDevice *d, uint32_t 
addr, uint32_t val,
 static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
 {
 PCIIDEState *d = PCI_IDE(dev);
+DeviceState *ds = DEVICE(dev);
 uint8_t *pci_conf = dev->config;
-qemu_irq *irq;
 int i;
 
 pci_conf[PCI_CLASS_PROG] = 0x8f;
@@ -291,16 +291,15 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error 
**errp)
 /* TODO: RST# value should be 0 */
 pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
 
-irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
+qdev_init_gpio_in(ds, cmd646_set_irq, 2);
 for (i = 0; i < 2; i++) {
-ide_bus_new(>bus[i], sizeof(d->bus[i]), DEVICE(dev), i, 2);
-ide_init2(>bus[i], irq[i]);
+ide_bus_new(>bus[i], sizeof(d->bus[i]), ds, i, 2);
+ide_init2(>bus[i], qdev_get_gpio_in(ds, i));
 
 bmdma_init(>bus[i], >bmdma[i], d);
 d->bmdma[i].bus = >bus[i];
 ide_register_restart_cb(>bus[i]);
 }
-g_free(irq);
 }
 
 static void pci_cmd646_ide_exitfn(PCIDevice *dev)
-- 
2.20.1

[PATCH for-5.0 1/3] via-ide: don't use PCI level for legacy IRQs

[Mark Cave-Ayland]

The PCI level calculation was accidentally left in when rebasing from a
previous patchset. Since both IRQs are driven separately, the value
being passed into the IRQ handler should be used directly.

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/via.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 8de4945cc1..2a55b7fbc6 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -112,7 +112,6 @@ static void via_ide_set_irq(void *opaque, int n, int level)
 d->config[0x70 + n * 8] &= ~0x80;
 }
 
-level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
 qemu_set_irq(isa_get_irq(NULL, 14 + n), level);
 }
 
-- 
2.20.1

[PATCH for-5.0 0/3] ide: fix potential memory leaks (plus one via-ide bugfix)

[Mark Cave-Ayland]

This was supposed to be a simple patchset to switch via-ide and cmd646-ide
over to use qdev gpio in the same way as Peter's patch did for sil3112, but
at the same time I spotted a silly mistake in my last set of via-ide
patches which is included as patch 1.

I'm not sure exactly why Coverity CID 1421984 isn't triggered by the
via-ide and cmd646-ide devices, however given the simplicity of the fix it
seems worth doing just to keep everything the same and ensure it won't
suddenly appear in future.

The via-ide changes were tested using the instructions provided by Zoltan
for MIPS fulong2e and PPC pegasos2, whilst the cmd646 change was tested
using one of my SPARC64 Linux test images.

Signed-off-by: Mark Cave-Ayland 


Mark Cave-Ayland (3):
  via-ide: don't use PCI level for legacy IRQs
  via-ide: use qdev gpio rather than qemu_allocate_irqs()
  cmd646-ide: use qdev gpio rather than qemu_allocate_irqs()

 hw/ide/cmd646.c | 9 -
 hw/ide/via.c| 7 ---
 2 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH for-5.0 2/3] via-ide: use qdev gpio rather than qemu_allocate_irqs()

[Mark Cave-Ayland]

This prevents the memory from qemu_allocate_irqs() from being leaked which
can in some cases be spotted by Coverity (CID 1421984).

Signed-off-by: Mark Cave-Ayland 
---
 hw/ide/via.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 2a55b7fbc6..be09912b33 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -160,6 +160,7 @@ static void via_ide_reset(DeviceState *dev)
 static void via_ide_realize(PCIDevice *dev, Error **errp)
 {
 PCIIDEState *d = PCI_IDE(dev);
+DeviceState *ds = DEVICE(dev);
 uint8_t *pci_conf = dev->config;
 int i;
 
@@ -187,9 +188,10 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
 bmdma_setup_bar(d);
 pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, >bmdma_bar);
 
+qdev_init_gpio_in(ds, via_ide_set_irq, 2);
 for (i = 0; i < 2; i++) {
-ide_bus_new(>bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-ide_init2(>bus[i], qemu_allocate_irq(via_ide_set_irq, d, i));
+ide_bus_new(>bus[i], sizeof(d->bus[i]), ds, i, 2);
+ide_init2(>bus[i], qdev_get_gpio_in(ds, i));
 
 bmdma_init(>bus[i], >bmdma[i], d);
 d->bmdma[i].bus = >bus[i];
-- 
2.20.1

Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[John Snow]

On 3/24/20 4:43 PM, Mark Cave-Ayland wrote:
> On 23/03/2020 15:17, Peter Maydell wrote:
> 
>> Coverity points out (CID 1421984) that we are leaking the
>> memory returned by qemu_allocate_irqs(). We can avoid this
>> leak by switching to using qdev_init_gpio_in(); the base
>> class finalize will free the irqs that this allocates under
>> the hood.
>>
>> Signed-off-by: Peter Maydell 
>> ---
>> This is how the 'use qdev gpio' approach to fixing the leak looks.
>> Disclaimer: I have only tested this with "make check", nothing more.
>>
>>  hw/ide/sii3112.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
>> index 06605d7af2b..2ae6f5d9df6 100644
>> --- a/hw/ide/sii3112.c
>> +++ b/hw/ide/sii3112.c
>> @@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
>> **errp)
>>  {
>>  SiI3112PCIState *d = SII3112_PCI(dev);
>>  PCIIDEState *s = PCI_IDE(dev);
>> +DeviceState *ds = DEVICE(dev);
>>  MemoryRegion *mr;
>> -qemu_irq *irq;
>>  int i;
>>  
>>  pci_config_set_interrupt_pin(dev->config, 1);
>> @@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
>> **errp)
>>  memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", >mmio, 0, 
>> 16);
>>  pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
>>  
>> -irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
>> +qdev_init_gpio_in(ds, sii3112_set_irq, 2);
>>  for (i = 0; i < 2; i++) {
>>  ide_bus_new(>bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
>> -ide_init2(>bus[i], irq[i]);
>> +ide_init2(>bus[i], qdev_get_gpio_in(ds, i));
>>  
>>  bmdma_init(>bus[i], >bmdma[i], s);
>>  s->bmdma[i].bus = >bus[i];
> 
> Looks like there is similar use of qemu_allocate_irqs() in via-ide and 
> cmd646-ide,
> and also reviewing my latest via-ide changes I spotted a silly mistake which 
> was
> obviously left in from a previous experimental version.
> 
> I'm not sure why Coverity doesn't pick up these other occurrences, however 
> I'll send
> along a patchset for this shortly.
> 

OK;

I will rescind my PR and will re-send it with your patches included.

--js

Re: [PATCH] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[Mark Cave-Ayland]

On 23/03/2020 15:17, Peter Maydell wrote:

> Coverity points out (CID 1421984) that we are leaking the
> memory returned by qemu_allocate_irqs(). We can avoid this
> leak by switching to using qdev_init_gpio_in(); the base
> class finalize will free the irqs that this allocates under
> the hood.
> 
> Signed-off-by: Peter Maydell 
> ---
> This is how the 'use qdev gpio' approach to fixing the leak looks.
> Disclaimer: I have only tested this with "make check", nothing more.
> 
>  hw/ide/sii3112.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
> index 06605d7af2b..2ae6f5d9df6 100644
> --- a/hw/ide/sii3112.c
> +++ b/hw/ide/sii3112.c
> @@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
> **errp)
>  {
>  SiI3112PCIState *d = SII3112_PCI(dev);
>  PCIIDEState *s = PCI_IDE(dev);
> +DeviceState *ds = DEVICE(dev);
>  MemoryRegion *mr;
> -qemu_irq *irq;
>  int i;
>  
>  pci_config_set_interrupt_pin(dev->config, 1);
> @@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
> **errp)
>  memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", >mmio, 0, 16);
>  pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
>  
> -irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
> +qdev_init_gpio_in(ds, sii3112_set_irq, 2);
>  for (i = 0; i < 2; i++) {
>  ide_bus_new(>bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
> -ide_init2(>bus[i], irq[i]);
> +ide_init2(>bus[i], qdev_get_gpio_in(ds, i));
>  
>  bmdma_init(>bus[i], >bmdma[i], s);
>  s->bmdma[i].bus = >bus[i];

Looks like there is similar use of qemu_allocate_irqs() in via-ide and 
cmd646-ide,
and also reviewing my latest via-ide changes I spotted a silly mistake which was
obviously left in from a previous experimental version.

I'm not sure why Coverity doesn't pick up these other occurrences, however I'll 
send
along a patchset for this shortly.


ATB,

Mark.

Re: [PATCH 6/6] qga/commands-posix: fix use after free of local_err

[Eric Blake]

On 3/24/20 10:36 AM, Vladimir Sementsov-Ogievskiy wrote:

local_err is used several times in guest_suspend(). Setting non-NULL
local_err will crash, so let's zero it after freeing. Also fix possible
leak of local_err in final if().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  qga/commands-posix.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 93474ff770..cc69b82704 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1773,6 +1773,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
  }
  
  error_free(local_err);

+local_err = NULL;


Let's show this with more context.


static void guest_suspend(SuspendMode mode, Error **errp)
{
Error *local_err = NULL;
bool mode_supported = false;

if (systemd_supports_mode(mode, _err)) {


Hmm - we have an even earlier bug that needs fixing.  Note that 
systemd_supports_mode() returns a bool AND conditionally sets errp.  But 
it is inconsistent: it has the following table of actions based on the 
results of run_process_child() on "systemctl status" coupled with the 
man page on "systemctl status" return values:

-1 (unable to run systemctl) -> errp set, return false
0 (unit is active) -> errp left unchanged, return false
1 (unit not failed) -> errp left unchanged, return true
2 (unused) -> errp left unchanged, return true
3 (unit not active) -> errp left unchanged, return true
4 (no such unit) -> errp left unchanged, return false
5+ (unexpected from systemctl) -> errp left unchanged, return false

But the comments in systemd_supports_mode() claim that ANY status < 4 
(other than -1, which means we did not run systemctl) should count as 
the service existing, even though the most common status is 3.  If our 
comment is to be believed, then we should return true, not false, for 
status 0.


Now, back to _this_ function:


mode_supported = true;
systemd_suspend(mode, _err);


Okay - if we get here (whether from status 1-3, or with 
systemd_supports_mode fixed to support status 0-3), local_err is still 
unset prior to calling systemd_suspend(), and we are guaranteed that 
after the call, either we suspended successfully or local_err is now set.



}

if (!local_err) {
return;
}


So if returned, we succeeded at systemd_suspend, and there is nothing 
further to do; but if we get past that point, we don't know if it was 
systemd_supports_mode that failed or systemd_suspend that failed, and we 
don't know if local_err is set.




error_free(local_err);
+local_err = NULL;


Yet, we blindly throw away local_err, without trying to report it.  If 
that's the case, then WHY are we passing in local_err?  Wouldn't it be 
better to pass in NULL (we really don't care about the error message), 
and/or fix systemd_suspend() to return a bool just like 
systemd_supports_mode, and/or fix systemd_supports_mode to guarantee 
that it sets errp when returning false?


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

Re: block stream and bitmaps

[Vladimir Sementsov-Ogievskiy]

24.03.2020 22:19, John Snow wrote:



On 3/24/20 6:18 AM, Kevin Wolf wrote:

Am 23.03.2020 um 19:06 hat John Snow geschrieben:

Hi Kevin,

I'm hoping to get some preliminary ideas from you (capped at five
minutes' effort?) on this subject. My ideas here are a bit shaky, I only
have really rough notions here.

We want to use bitmaps with 'drive' semantics; i.e. tracking changes to
the visible guest data. What we have are bitmaps with node semantics,
tracking changes to the data at a particular level in the graph.

For commit, this isn't a big deal: we can disable bitmaps in the backing
file while we commit and then re-enable it on completion. We usually
have a separate bitmap enabled on the root node that is recording writes
during this time, and can be moved later.

For streaming, this is more challenging: new writes will dirty the
bitmap, but so will writes from the stream job itself.

Semantically, we want to ignore writes from the stream while recording
them from the guest -- differentiating based on source.


No, based on source is actually not what you want. What you really want
is that BDRV_REQ_WRITE_UNCHANGED doesn't mark any blocks dirty.



This is why I sent the mail, I figured you'd know the better incision
point, and I was right!


We discussed this specific case of streaming at FOSDEM (with Paolo and
probably Nir). Paolo was even convinced that unchanged writes already
behave like this, but we agreed that dirtying blocks for them would be a
bug. After checking that the code is indeed buggy, I was planning to
send a patch, but never got around to actually do that. Sorry about
that.



Glad to hear it has been given consideration, though!


Bitmaps aren't really geared to do that right now. With the changes to
Bdrv Roles that Max was engineering, do you think it's possible to add
some kind of write source discrimination to bitmaps, or is that too messy?


I don't think it would work because copy-on-read requests come from the
same parent node as writes (no matter whether the legacy code in
block/io.c or a copy-on-read filter node is used).



Oh, understood. Rule that approach out, then.


For both commit and stream, it might be nice to say: "This bitmap is
enabled, but ignores writes from [all? specific types? specific
instances?] jobs.


Commit is a bit trickier, because it's not WRITE_UNCHANGED. The result
is only unchanged for the top layer, but not for the backing file you're
committing to. Not sure if we can represent this condition somehow.



Nothing comes to mind apart from a semantic that applies to a graph
subsection instead of an individual node.

i.e. UNCHANGED as applied to [A --> B].

Not saying that's reasonable to develop... or necessarily even possible
to enforce, just nothing else comes to mind.


Or, I wonder if what we truly want is some kind of bitmap "forwarder"
object on block-backend objects that represent the semantic drive view,
and only writes through that *backend* get forwarded to the bitmaps
attached to whatever node the bitmap is actually associated with.

(That might wind up causing weird problems too, though... since those
objects are no longer intended to be user-addressable, managing that
configuration might get intensely strange.)


Hm... Drive-based does suggest that it's managed at the BlockBackend
level. So having a bitmap that isn't added as a dirty bitmap to the BDS,
but only to the BB does make sense to me. The BB would be addressed
with the qdev ID of the device, as usual (which underlines that it's
really per device).



That's the rough idea, though if it's needed or not is unclear. We might
be able to get by with node semantics if we jazz them up enough...?

Working around all the edge bases of a drive-semantic bitmap seem
difficult to reason about.

In general, it should likely be made persistent against the root-most
node to which writes are routed to a protocol node.
In the common case, that means the top-most qcow2 format node of a chain.

But, many of our job filters also route writes to format nodes too, so
which one is the "canonical" store? Is it even possible to define?

Hm.

I suppose we can always allow bitmaps being attached to the BB and
*disallow them* from being made persistent; but simply exist as an
in-memory tool to help ease the pain of managing data consistency during
critical sections.

We can offer merge-to, merge-from semantics for these in-memory-only
bitmaps. Maybe.

Still mulling it over.


We can just add "empty" filter node, and move bitmaps to it and than back
again. If I understand correctly, this gives same semantics like with
moving to BB, but without any additional BB logic. We just need "do-nothing"
filter driver for it.




The part that's unclear to me is how to make such bitmaps persistent.
You can change the root node of a BB and even remove the root node
completely (for removable devices; but even changing is technically
remove followed by insert), so you may need to move the bitmap around
between image files 

[PULL 2/2] hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

[John Snow]

From: Peter Maydell 

Coverity points out (CID 1421984) that we are leaking the
memory returned by qemu_allocate_irqs(). We can avoid this
leak by switching to using qdev_init_gpio_in(); the base
class finalize will free the irqs that this allocates under
the hood.

Signed-off-by: Peter Maydell 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: John Snow 
Tested-by: BALATON Zoltan 
Message-id: 20200323151715.29454-1-peter.mayd...@linaro.org
[Maintainer edit: replace `DEVICE(dev)` by `ds` --js]
Signed-off-by: John Snow 
---
 hw/ide/sii3112.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 06605d7af2..d69079c3d9 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -251,8 +251,8 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
 {
 SiI3112PCIState *d = SII3112_PCI(dev);
 PCIIDEState *s = PCI_IDE(dev);
+DeviceState *ds = DEVICE(dev);
 MemoryRegion *mr;
-qemu_irq *irq;
 int i;
 
 pci_config_set_interrupt_pin(dev->config, 1);
@@ -280,10 +280,10 @@ static void sii3112_pci_realize(PCIDevice *dev, Error 
**errp)
 memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", >mmio, 0, 16);
 pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
 
-irq = qemu_allocate_irqs(sii3112_set_irq, d, 2);
+qdev_init_gpio_in(ds, sii3112_set_irq, 2);
 for (i = 0; i < 2; i++) {
-ide_bus_new(>bus[i], sizeof(s->bus[i]), DEVICE(dev), i, 1);
-ide_init2(>bus[i], irq[i]);
+ide_bus_new(>bus[i], sizeof(s->bus[i]), ds, i, 1);
+ide_init2(>bus[i], qdev_get_gpio_in(ds, i));
 
 bmdma_init(>bus[i], >bmdma[i], s);
 s->bmdma[i].bus = >bus[i];
-- 
2.21.1

[PULL 0/2] Ide patches

[John Snow]

The following changes since commit 736cf607e40674776d752acc201f565723e86045:

  Update version for v5.0.0-rc0 release (2020-03-24 17:50:00 +)

are available in the Git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 51058b3b3bcbe62506cf191fca1c0d679bb80f2b:

  hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs() (2020-03-24 
15:52:16 -0400)


Pull request: IDE

Admittedly the first one is not a crisis fix; but I think it's low-risk to
include for rc1.

The second one is yours, and will shush coverity.



Peter Maydell (1):
  hw/ide/sii3112: Use qdev gpio rather than qemu_allocate_irqs()

Sven Schnelle (1):
  fdc/i8257: implement verify transfer mode

 include/hw/isa/isa.h |  1 -
 hw/block/fdc.c   | 61 +---
 hw/dma/i8257.c   | 20 ++-
 hw/ide/sii3112.c |  8 +++---
 4 files changed, 35 insertions(+), 55 deletions(-)

-- 
2.21.1

[PULL 1/2] fdc/i8257: implement verify transfer mode

[John Snow]

From: Sven Schnelle 

While working on the Tulip driver i tried to write some Teledisk images to
a floppy image which didn't work. Turned out that Teledisk checks the written
data by issuing a READ command to the FDC but running the DMA controller
in VERIFY mode. As we ignored the DMA request in that case, the DMA transfer
never finished, and Teledisk reported an error.

The i8257 spec says about verify transfers:

3) DMA verify, which does not actually involve the transfer of data. When an
8257 channel is in the DMA verify mode, it will respond the same as described
for transfer operations, except that no memory or I/O read/write control signals
will be generated.

Hervé proposed to remove all the dma_mode_ok stuff from fdc to have a more
clear boundary between DMA and FDC, so this patch also does that.

Suggested-by: Hervé Poussineau 
Signed-off-by: Sven Schnelle 
Reviewed-by: Hervé Poussineau 
Signed-off-by: John Snow 
---
 include/hw/isa/isa.h |  1 -
 hw/block/fdc.c   | 61 +---
 hw/dma/i8257.c   | 20 ++-
 3 files changed, 31 insertions(+), 51 deletions(-)

diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index e9ac1f1205..59a4d4b50a 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -56,7 +56,6 @@ typedef int (*IsaDmaTransferHandler)(void *opaque, int nchan, 
int pos,
 typedef struct IsaDmaClass {
 InterfaceClass parent;
 
-IsaDmaTransferMode (*get_transfer_mode)(IsaDma *obj, int nchan);
 bool (*has_autoinitialization)(IsaDma *obj, int nchan);
 int (*read_memory)(IsaDma *obj, int nchan, void *buf, int pos, int len);
 int (*write_memory)(IsaDma *obj, int nchan, void *buf, int pos, int len);
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 22e954e0dc..33bc9e2f92 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1714,53 +1714,28 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int 
direction)
 }
 fdctrl->eot = fdctrl->fifo[6];
 if (fdctrl->dor & FD_DOR_DMAEN) {
-IsaDmaTransferMode dma_mode;
+/* DMA transfer is enabled. */
 IsaDmaClass *k = ISADMA_GET_CLASS(fdctrl->dma);
-bool dma_mode_ok;
-/* DMA transfer are enabled. Check if DMA channel is well programmed */
-dma_mode = k->get_transfer_mode(fdctrl->dma, fdctrl->dma_chann);
-FLOPPY_DPRINTF("dma_mode=%d direction=%d (%d - %d)\n",
-   dma_mode, direction,
-   (128 << fdctrl->fifo[5]) *
+
+FLOPPY_DPRINTF("direction=%d (%d - %d)\n",
+   direction, (128 << fdctrl->fifo[5]) *
(cur_drv->last_sect - ks + 1), fdctrl->data_len);
-switch (direction) {
-case FD_DIR_SCANE:
-case FD_DIR_SCANL:
-case FD_DIR_SCANH:
-dma_mode_ok = (dma_mode == ISADMA_TRANSFER_VERIFY);
-break;
-case FD_DIR_WRITE:
-dma_mode_ok = (dma_mode == ISADMA_TRANSFER_WRITE);
-break;
-case FD_DIR_READ:
-dma_mode_ok = (dma_mode == ISADMA_TRANSFER_READ);
-break;
-case FD_DIR_VERIFY:
-dma_mode_ok = true;
-break;
-default:
-dma_mode_ok = false;
-break;
-}
-if (dma_mode_ok) {
-/* No access is allowed until DMA transfer has completed */
-fdctrl->msr &= ~FD_MSR_RQM;
-if (direction != FD_DIR_VERIFY) {
-/* Now, we just have to wait for the DMA controller to
- * recall us...
- */
-k->hold_DREQ(fdctrl->dma, fdctrl->dma_chann);
-k->schedule(fdctrl->dma);
-} else {
-/* Start transfer */
-fdctrl_transfer_handler(fdctrl, fdctrl->dma_chann, 0,
-fdctrl->data_len);
-}
-return;
+
+/* No access is allowed until DMA transfer has completed */
+fdctrl->msr &= ~FD_MSR_RQM;
+if (direction != FD_DIR_VERIFY) {
+/*
+ * Now, we just have to wait for the DMA controller to
+ * recall us...
+ */
+k->hold_DREQ(fdctrl->dma, fdctrl->dma_chann);
+k->schedule(fdctrl->dma);
 } else {
-FLOPPY_DPRINTF("bad dma_mode=%d direction=%d\n", dma_mode,
-   direction);
+/* Start transfer */
+fdctrl_transfer_handler(fdctrl, fdctrl->dma_chann, 0,
+fdctrl->data_len);
 }
+return;
 }
 FLOPPY_DPRINTF("start non-DMA transfer\n");
 fdctrl->msr |= FD_MSR_NONDMA | FD_MSR_RQM;
diff --git a/hw/dma/i8257.c b/hw/dma/i8257.c
index ef15c06d77..1b3435ab58 100644
--- a/hw/dma/i8257.c
+++ b/hw/dma/i8257.c
@@ -292,12 +292,6 @@ static uint64_t i8257_read_cont(void *opaque, hwaddr 
nport, unsigned size)
 return val;
 }
 
-static IsaDmaTransferMode 

Re: [PATCH 5/6] migration/ram: fix use after free of local_err

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote:
> local_err is used again in migration_bitmap_sync_precopy() after
> precopy_notify(), so we must zero it. Otherwise try to set
> non-NULL local_err will crash.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  migration/ram.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index c12cfdbe26..04f13feb2e 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -980,6 +980,7 @@ static void migration_bitmap_sync_precopy(RAMState *rs)
>   */
>  if (precopy_notify(PRECOPY_NOTIFY_BEFORE_BITMAP_SYNC, _err)) {
>  error_report_err(local_err);
> +local_err = NULL;

Reviewed-by: Dr. David Alan Gilbert 

and queued.


>  }
>  
>  migration_bitmap_sync(rs);
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 4/6] migration/colo: fix use after free of local_err

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (vsement...@virtuozzo.com) wrote:
> local_err is used again in secondary_vm_do_failover() after
> replication_stop_all(), so we must zero it. Otherwise try to set
> non-NULL local_err will crash.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Reviewed-by: Dr. David Alan Gilbert 

I'll queue this

> ---
>  migration/colo.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/colo.c b/migration/colo.c
> index 44942c4e23..a54ac84f41 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -93,6 +93,7 @@ static void secondary_vm_do_failover(void)
>  replication_stop_all(true, _err);
>  if (local_err) {
>  error_report_err(local_err);
> +local_err = NULL;
>  }
>  
>  /* Notify all filters of all NIC to do checkpoint */
> -- 
> 2.21.0
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v2 4/4] sheepdog: Consistently set bdrv_has_zero_init_truncate

[John Snow]

On 3/24/20 1:42 PM, Eric Blake wrote:
> block_int.h claims that .bdrv_has_zero_init must return 0 if
> .bdrv_has_zero_init_truncate does likewise; but this is violated if
> only the former callback is provided if .bdrv_co_truncate also exists.
> When adding the latter callback, it was mistakenly added to only one
> of the three possible sheepdog instantiations.
> 
> Fixes: 1dcaf527
> Signed-off-by: Eric Blake 
> ---
>  block/sheepdog.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index cfa84338a2d6..522c16a93676 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -3269,6 +3269,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
>  .bdrv_co_create   = sd_co_create,
>  .bdrv_co_create_opts  = sd_co_create_opts,
>  .bdrv_has_zero_init   = bdrv_has_zero_init_1,
> +.bdrv_has_zero_init_truncate  = bdrv_has_zero_init_1,
>  .bdrv_getlength   = sd_getlength,
>  .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
>  .bdrv_co_truncate = sd_co_truncate,
> @@ -3307,6 +3308,7 @@ static BlockDriver bdrv_sheepdog_unix = {
>  .bdrv_co_create   = sd_co_create,
>  .bdrv_co_create_opts  = sd_co_create_opts,
>  .bdrv_has_zero_init   = bdrv_has_zero_init_1,
> +.bdrv_has_zero_init_truncate  = bdrv_has_zero_init_1,
>  .bdrv_getlength   = sd_getlength,
>  .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
>  .bdrv_co_truncate = sd_co_truncate,
> 

Reviewed-by: John Snow 

Re: block stream and bitmaps

[John Snow]

On 3/24/20 6:18 AM, Kevin Wolf wrote:
> Am 23.03.2020 um 19:06 hat John Snow geschrieben:
>> Hi Kevin,
>>
>> I'm hoping to get some preliminary ideas from you (capped at five
>> minutes' effort?) on this subject. My ideas here are a bit shaky, I only
>> have really rough notions here.
>>
>> We want to use bitmaps with 'drive' semantics; i.e. tracking changes to
>> the visible guest data. What we have are bitmaps with node semantics,
>> tracking changes to the data at a particular level in the graph.
>>
>> For commit, this isn't a big deal: we can disable bitmaps in the backing
>> file while we commit and then re-enable it on completion. We usually
>> have a separate bitmap enabled on the root node that is recording writes
>> during this time, and can be moved later.
>>
>> For streaming, this is more challenging: new writes will dirty the
>> bitmap, but so will writes from the stream job itself.
>>
>> Semantically, we want to ignore writes from the stream while recording
>> them from the guest -- differentiating based on source.
> 
> No, based on source is actually not what you want. What you really want
> is that BDRV_REQ_WRITE_UNCHANGED doesn't mark any blocks dirty.
> 

This is why I sent the mail, I figured you'd know the better incision
point, and I was right!

> We discussed this specific case of streaming at FOSDEM (with Paolo and
> probably Nir). Paolo was even convinced that unchanged writes already
> behave like this, but we agreed that dirtying blocks for them would be a
> bug. After checking that the code is indeed buggy, I was planning to
> send a patch, but never got around to actually do that. Sorry about
> that.
> 

Glad to hear it has been given consideration, though!

>> Bitmaps aren't really geared to do that right now. With the changes to
>> Bdrv Roles that Max was engineering, do you think it's possible to add
>> some kind of write source discrimination to bitmaps, or is that too messy?
> 
> I don't think it would work because copy-on-read requests come from the
> same parent node as writes (no matter whether the legacy code in
> block/io.c or a copy-on-read filter node is used).
> 

Oh, understood. Rule that approach out, then.

>> For both commit and stream, it might be nice to say: "This bitmap is
>> enabled, but ignores writes from [all? specific types? specific
>> instances?] jobs.
> 
> Commit is a bit trickier, because it's not WRITE_UNCHANGED. The result
> is only unchanged for the top layer, but not for the backing file you're
> committing to. Not sure if we can represent this condition somehow.
> 

Nothing comes to mind apart from a semantic that applies to a graph
subsection instead of an individual node.

i.e. UNCHANGED as applied to [A --> B].

Not saying that's reasonable to develop... or necessarily even possible
to enforce, just nothing else comes to mind.

>> Or, I wonder if what we truly want is some kind of bitmap "forwarder"
>> object on block-backend objects that represent the semantic drive view,
>> and only writes through that *backend* get forwarded to the bitmaps
>> attached to whatever node the bitmap is actually associated with.
>>
>> (That might wind up causing weird problems too, though... since those
>> objects are no longer intended to be user-addressable, managing that
>> configuration might get intensely strange.)
> 
> Hm... Drive-based does suggest that it's managed at the BlockBackend
> level. So having a bitmap that isn't added as a dirty bitmap to the BDS,
> but only to the BB does make sense to me. The BB would be addressed
> with the qdev ID of the device, as usual (which underlines that it's
> really per device).
> 

That's the rough idea, though if it's needed or not is unclear. We might
be able to get by with node semantics if we jazz them up enough...?

Working around all the edge bases of a drive-semantic bitmap seem
difficult to reason about.

In general, it should likely be made persistent against the root-most
node to which writes are routed to a protocol node.
In the common case, that means the top-most qcow2 format node of a chain.

But, many of our job filters also route writes to format nodes too, so
which one is the "canonical" store? Is it even possible to define?

Hm.

I suppose we can always allow bitmaps being attached to the BB and
*disallow them* from being made persistent; but simply exist as an
in-memory tool to help ease the pain of managing data consistency during
critical sections.

We can offer merge-to, merge-from semantics for these in-memory-only
bitmaps. Maybe.

Still mulling it over.

> The part that's unclear to me is how to make such bitmaps persistent.
> You can change the root node of a BB and even remove the root node
> completely (for removable devices; but even changing is technically
> remove followed by insert), so you may need to move the bitmap around
> between image files and at least for some time you might not have any
> place to store the bitmap.
> 

Yeah, we've had discussions about this in the past. 

Re: [PATCH for-5.0 v2 3/3] iotests/138: Test leaks/corruptions fixed report

[Eric Blake]

On 3/24/20 12:27 PM, Max Reitz wrote:

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

While touching the _unsupported_imgopts line, adjust the note on why
data_file does not work with this test: The current comment sounds a bit
like it is a mistake for qemu-img check not to check external data
files' refcounts.  But there are no such refcounts, so it is no mistake.
Just say that qemu-img check does not do much for external data files,
and this is why this test does not work with them.

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..1d5b0bed6d 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
+# (so qemu-img check would not do much);


Works for me.


+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"/' \
+  -e '/^/d' \
+  -e "s/\\([^0-9a-f]\\)$(printf %x 
$l1_entry)\\([^0-9a-f]\\)/\1L1_ENTRY_VALUE\2/"


It's fun to see the post-review results.  Given the limited times where 
the third -e fires in the output,



+++ b/tests/qemu-iotests/138.out



+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


it could be written with less typing as:

-e "s/=$(printf %x $l1_entry) /=L1_ENTRY_VALUE /"

but that's not essential.  So either way,

Reviewed-by: Eric Blake 

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

Re: [PATCH for-5.0 v2 2/3] iotests: Add poke_file_[bl]e functions

[Eric Blake]

On 3/24/20 12:27 PM, 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 
Code-suggested-by: Eric Blake 
---
  tests/qemu-iotests/common.rc | 24 
  1 file changed, 24 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 4c246c0450..bf3b9fdea0 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,6 +53,30 @@ poke_file()
  printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
  }
  
+# poke_file_le $img_filename $offset $byte_width $value

+# Example: poke_file_le "$TEST_IMG" 512 2 65534
+poke_file_le()


Yep, that's nicer.


+{
+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"
+}


and so is that (but I'm biased, here :)

Reviewed-by: Eric Blake 

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

[PATCH v2 4/4] sheepdog: Consistently set bdrv_has_zero_init_truncate

[Eric Blake]

block_int.h claims that .bdrv_has_zero_init must return 0 if
.bdrv_has_zero_init_truncate does likewise; but this is violated if
only the former callback is provided if .bdrv_co_truncate also exists.
When adding the latter callback, it was mistakenly added to only one
of the three possible sheepdog instantiations.

Fixes: 1dcaf527
Signed-off-by: Eric Blake 
---
 block/sheepdog.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index cfa84338a2d6..522c16a93676 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -3269,6 +3269,7 @@ static BlockDriver bdrv_sheepdog_tcp = {
 .bdrv_co_create   = sd_co_create,
 .bdrv_co_create_opts  = sd_co_create_opts,
 .bdrv_has_zero_init   = bdrv_has_zero_init_1,
+.bdrv_has_zero_init_truncate  = bdrv_has_zero_init_1,
 .bdrv_getlength   = sd_getlength,
 .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
 .bdrv_co_truncate = sd_co_truncate,
@@ -3307,6 +3308,7 @@ static BlockDriver bdrv_sheepdog_unix = {
 .bdrv_co_create   = sd_co_create,
 .bdrv_co_create_opts  = sd_co_create_opts,
 .bdrv_has_zero_init   = bdrv_has_zero_init_1,
+.bdrv_has_zero_init_truncate  = bdrv_has_zero_init_1,
 .bdrv_getlength   = sd_getlength,
 .bdrv_get_allocated_file_size = sd_get_allocated_file_size,
 .bdrv_co_truncate = sd_co_truncate,
-- 
2.26.0.rc2

[PATCH v2 2/4] qcow2: List autoclear bit names in header

[Eric Blake]

The feature table is supposed to advertise the name of all feature
bits that we support; however, we forgot to update the table for
autoclear bits.  While at it, move the table to read-only memory in
code, and tweak the qcow2 spec to name the second autoclear bit.
Update iotests that are affected by the longer header length.

Fixes: 88ddffae
Fixes: 93c24936
Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 

---
[v2] tweak name of "bitmaps" autoclear bit [Vladimir]
---
 docs/interop/qcow2.txt |  3 ++-
 block/qcow2.c  | 12 +++-
 tests/qemu-iotests/031.out |  8 
 tests/qemu-iotests/036.out |  4 ++--
 tests/qemu-iotests/061.out | 14 +++---
 5 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e244746e..640e0eca4000 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -143,7 +143,8 @@ the next fields through header_length.
 bit is unset, the bitmaps extension data must 
be
 considered inconsistent.

-Bit 1:  If this bit is set, the external data file can
+Bit 1:  Raw external data bit
+If this bit is set, the external data file can
 be read as a consistent standalone raw image
 without looking at the qcow2 metadata.

diff --git a/block/qcow2.c b/block/qcow2.c
index 5a7f30e692a2..67b0c214fba4 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2825,7 +2825,7 @@ int qcow2_update_header(BlockDriverState *bs)

 /* Feature table */
 if (s->qcow_version >= 3) {
-Qcow2Feature features[] = {
+static const Qcow2Feature features[] = {
 {
 .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
 .bit  = QCOW2_INCOMPAT_DIRTY_BITNR,
@@ -2846,6 +2846,16 @@ int qcow2_update_header(BlockDriverState *bs)
 .bit  = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
 .name = "lazy refcounts",
 },
+{
+.type = QCOW2_FEAT_TYPE_AUTOCLEAR,
+.bit  = QCOW2_AUTOCLEAR_BITMAPS_BITNR,
+.name = "bitmaps",
+},
+{
+.type = QCOW2_FEAT_TYPE_AUTOCLEAR,
+.bit  = QCOW2_AUTOCLEAR_DATA_FILE_RAW_BITNR,
+.name = "raw external data",
+},
 };

 ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FEATURE_TABLE,
diff --git a/tests/qemu-iotests/031.out b/tests/qemu-iotests/031.out
index d535e407bc30..46f97c5a4ea4 100644
--- a/tests/qemu-iotests/031.out
+++ b/tests/qemu-iotests/031.out
@@ -117,7 +117,7 @@ header_length 104

 Header extension:
 magic 0x6803f857
-length192
+length288
 data  

 Header extension:
@@ -150,7 +150,7 @@ header_length 104

 Header extension:
 magic 0x6803f857
-length192
+length288
 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   0x1d8
 backing_file_size 0x17
 cluster_bits  16
 size  67108864
@@ -188,7 +188,7 @@ data  'host_device'

 Header extension:
 magic 0x6803f857
-length192
+length288
 data  

 Header extension:
diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
index 0b52b934e115..23b699ce0622 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
+length288
 data  


@@ -38,7 +38,7 @@ compatible_features   []
 autoclear_features[]
 Header extension:
 magic 0x6803f857
-length192
+length288
 data  

 *** done
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index 8b3091a412bc..413cc4e0f4ab 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -26,7 +26,7 @@ header_length 104

 Header extension:
 magic 0x6803f857
-length192
+length288
 data  

 magic 0x514649fb
@@ -84,7 +84,7 @@ header_length 104

 Header extension:
 magic 0x6803f857
-length192
+length288
 data  

 magic 0x514649fb
@@ -140,7 

[PATCH v2 3/4] qcow2: Avoid feature name extension on small cluster size

[Eric Blake]

As the feature name table can be quite large (over 9k if all 64 bits
of all three feature fields have names; a mere 8 features leaves only
8 bytes for a backing file name in a 512-byte cluster), it is unwise
to emit this optional header in images with small cluster sizes.

Update iotest 036 to skip running on small cluster sizes; meanwhile,
note that iotest 061 never passed on alternative cluster sizes
(however, I limited this patch to tests with output affected by adding
feature names, rather than auditing for other tests that are not
robust to alternative cluster sizes).

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 block/qcow2.c  | 11 +--
 tests/qemu-iotests/036 |  6 --
 tests/qemu-iotests/061 |  6 --
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 67b0c214fba4..9475ace57163 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2823,8 +2823,15 @@ int qcow2_update_header(BlockDriverState *bs)
 buflen -= ret;
 }

-/* Feature table */
-if (s->qcow_version >= 3) {
+/*
+ * Feature table.  A mere 8 feature names occupies 392 bytes, and
+ * when coupled with the v3 minimum header of 104 bytes plus the
+ * 8-byte end-of-extension marker, that would leave only 8 bytes
+ * for a backing file name in an image with 512-byte clusters.
+ * Thus, we choose to omit this header for cluster sizes 4k and
+ * smaller.
+ */
+if (s->qcow_version >= 3 && s->cluster_size > 4096) {
 static const Qcow2Feature features[] = {
 {
 .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
index 512598421c20..cf522de7a1aa 100755
--- a/tests/qemu-iotests/036
+++ b/tests/qemu-iotests/036
@@ -44,8 +44,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 # Only qcow2v3 and later supports feature bits;
-# qcow2.py does not support external data files
-_unsupported_imgopts 'compat=0.10' data_file
+# qcow2.py does not support external data files;
+# this test requires a cluster size large enough for the feature table
+_unsupported_imgopts 'compat=0.10' data_file \
+'cluster_size=\(512\|1024\|2048\|4096\)'

 echo
 echo === Image with unknown incompatible feature bit ===
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index 36b040491fef..ce285d308408 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -44,8 +44,10 @@ _supported_os Linux
 # Conversion between different compat versions can only really work
 # with refcount_bits=16;
 # we have explicit tests for data_file here, but the whole test does
-# not work with it
-_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file
+# not work with it;
+# we have explicit tests for various cluster sizes, the remaining tests
+# require the default 64k cluster
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' data_file 
cluster_size

 echo
 echo "=== Testing version downgrade with zero expansion ==="
-- 
2.26.0.rc2

[PATCH v2 1/4] qcow2: Comment typo fixes

[Eric Blake]

Various trivial typos noticed while working on this file.

Signed-off-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
 block/qcow2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d1da3d91db21..5a7f30e692a2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -177,7 +177,7 @@ static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock 
*block, size_t offset,
 }


-/* 
+/*
  * read qcow2 extension and fill bs
  * start reading from start_offset
  * finish reading upon magic of value 0 or when end_offset reached
@@ -3255,7 +3255,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
Error **errp)
  * inconsistency later.
  *
  * We do need a refcount table because growing the refcount table means
- * allocating two new refcount blocks - the seconds of which would be at
+ * allocating two new refcount blocks - the second of which would be at
  * 2 GB for 64k clusters, and we don't want to have a 2 GB initial file
  * size for any qcow2 image.
  */
@@ -3500,7 +3500,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
Error **errp)
 goto out;
 }

-/* Want a backing file? There you go.*/
+/* Want a backing file? There you go. */
 if (qcow2_opts->has_backing_file) {
 const char *backing_format = NULL;

-- 
2.26.0.rc2

[PATCH for-5.0 v2 0/4] bug fixes extracted from larger qcow2 zero bit work

[Eric Blake]

My proposal [1] to add an autoclear all-zero-content bit to the qcow2
format has now stalled into 5.1 territory, but several patches in my
initial proposal are uncontroversial and obvious bug fixes worth
having in 5.0.

[1] https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg08075.html

Eric Blake (4):
  qcow2: Comment typo fixes
  qcow2: List autoclear bit names in header
  qcow2: Avoid feature name extension on small cluster size
  sheepdog: Consistently set bdrv_has_zero_init_truncate

 docs/interop/qcow2.txt |  3 ++-
 block/qcow2.c  | 29 +++--
 block/sheepdog.c   |  2 ++
 tests/qemu-iotests/031.out |  8 
 tests/qemu-iotests/036 |  6 --
 tests/qemu-iotests/036.out |  4 ++--
 tests/qemu-iotests/061 |  6 --
 tests/qemu-iotests/061.out | 14 +++---
 8 files changed, 48 insertions(+), 24 deletions(-)

-- 
2.26.0.rc2

Re: [PATCH v8 07/11] iotests: limit line length to 79 chars

[Max Reitz]

On 24.03.20 18:09, John Snow wrote:
> 
> 
> On 3/24/20 11:12 AM, Max Reitz wrote:
>> On 24.03.20 16:02, Max Reitz wrote:
>>> On 17.03.20 01:41, John Snow wrote:
 79 is the PEP8 recommendation. This recommendation works well for
 reading patch diffs in TUI email clients.
>>>
>>> Also for my very GUI-y diff program (kompare).
>>>
 Signed-off-by: John Snow 
 ---
  tests/qemu-iotests/iotests.py | 64 +++
  tests/qemu-iotests/pylintrc   |  6 +++-
  2 files changed, 47 insertions(+), 23 deletions(-)

 diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
 index 3d90fb157d..75fd697d77 100644
 --- a/tests/qemu-iotests/iotests.py
 +++ b/tests/qemu-iotests/iotests.py
>>>
>>> [...]
>>>
 @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None):
  self.pause_drive(drive, "write_aio")
  return
  self.qmp('human-monitor-command',
 - command_line='qemu-io %s "break %s bp_%s"' % (drive, 
 event, drive))
 + command_line='qemu-io %s "break %s bp_%s"'
 + % (drive, event, drive))
>>>
>>> Can we put this value in a variable instead?  I don’t like the %
>>> aligning with the parameter name instead of the string value.  (I also
>>> don’t like how there are no spaces around the assignment =, but around
>>> the %, even though the % binds more strongly.)
>>>
  
> 
> I think I had another patch somewhere that added an HMP helper that
> fixes this issue for this particular instance.
> 
> I can send that separately as a follow-up. I think otherwise,
> unfortunately, "we" "decided" that this indent style is "best".
> 
> So I think that this patch is "correct".

Perhaps it’s the best if we assume that we had to do it this way, but
can’t we just do a

qemu_io_cmd = f'qemu-io {drive} "break {event} bp_{drive}"'
self.qmp('human-monitor-command', command_line=qemu_io_cmd)

?

(Or maybe an f-string inline.  I was thinking of a separate variable
because I wasn’t aware that f-strings would be an option until I got to
later hunks of this patch...)

> (All of the other options for indent styles seemed to be worse visually,
> or actively go against PEP8. While PEP8 is not a bible, every conscious
> choice to disregard it generally means you will be fighting a CQA tool
> at some other point in time. I have therefore adopted a "When in Rome"
> policy to reduce friction wherever possible with pylint, flake8, mypy,
> pycharm, and so on.)
> 
> 
> ((I would prefer we begin to deprecate uses of % and begin using
> .format() and f-strings wherever possible to help alleviate this kind of
> syntax, too -- but I must insist that's for another series.))

Hm.  But you do change something to f-strings below, why not here?

Max



signature.asc
Description: OpenPGP digital signature

[PATCH for-5.0 v2 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).

While touching the _unsupported_imgopts line, adjust the note on why
data_file does not work with this test: The current comment sounds a bit
like it is a mistake for qemu-img check not to check external data
files' refcounts.  But there are no such refcounts, so it is no mistake.
Just say that qemu-img check does not do much for external data files,
and this is why this test does not work with them.

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..1d5b0bed6d 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
+# (so qemu-img check would not do much);
+# 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_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
+
+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"/' \
+  -e '/^/d' \
+  -e "s/\\([^0-9a-f]\\)$(printf %x 
$l1_entry)\\([^0-9a-f]\\)/\1L1_ENTRY_VALUE\2/"
+
 # 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.25.1

[PATCH for-5.0 v2 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 
Reviewed-by: Eric Blake 
---
 qemu-img.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index afddf33f08..b167376bd7 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.25.1

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

[Max Reitz]

Branch: https://github.com/XanClic/qemu.git fix-check-result-v2
Branch: https://git.xanclic.moe/XanClic/qemu.git fix-check-result-v2

v1: https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg01418.html


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.


v2:
- Patch 2:
  - Rewrote the new functions as per Eric’s suggestions (thanks a lot!)
  - Changed the functions’ “doc strings” to not just show an example,
but perhaps more importantly what each parameter means
- Patch 3:
  - Eric said the pre-existing comment explaining why 138 doesn’t
supported data_file made it sound like qemu-img check had a bug.
I’m sure it does have bugs, but this isn’t one, so I tried to
explain it a bit differently.
  - Use poke_file_be in another place
  - Convert sed | grep | sed to a single sed, as per Eric’s suggestion,
and avoid \< and \>


git-backport-diff against v1:

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

001/3:[] [--] 'qemu-img: Fix check's leak/corruption fix report'
002/3:[0029] [FC] 'iotests: Add poke_file_[bl]e functions'
003/3:[0008] [FC] 'iotests/138: Test leaks/corruptions fixed report'


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 | 24 +
 4 files changed, 84 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH for-5.0 v2 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 
Code-suggested-by: Eric Blake 
---
 tests/qemu-iotests/common.rc | 24 
 1 file changed, 24 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 4c246c0450..bf3b9fdea0 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,6 +53,30 @@ poke_file()
 printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
 }
 
+# poke_file_le $img_filename $offset $byte_width $value
+# Example: poke_file_le "$TEST_IMG" 512 2 65534
+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 $img_filename $offset $byte_width $value
+# Example: poke_file_be "$TEST_IMG" 512 2 65279
+poke_file_be()
+{
+local img=$1 ofs=$2 len=$3 val=$4
+local str=$(printf "%0$((len * 2))x\n" $val | sed 's/\(..\)/\\x\1/g')
+
+poke_file "$img" "$ofs" "$str"
+}
+
 # peek_file_le 'test.img' 512 2 => 65534
 peek_file_le()
 {
-- 
2.25.1

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[John Snow]

On 3/24/20 11:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> local_err is used again in mirror_exit_common() after
> bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
> non-NULL local_err will crash.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/mirror.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 447051dbc6..6203e5946e 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
>  bdrv_set_backing_hd(target_bs, backing, _err);
>  if (local_err) {
>  error_report_err(local_err);
> +local_err = NULL;
>  ret = -EPERM;
>  }
>  }
> 

Reviewed-by: John Snow 

Re: [PATCH v8 07/11] iotests: limit line length to 79 chars

[John Snow]

On 3/24/20 11:12 AM, Max Reitz wrote:
> On 24.03.20 16:02, Max Reitz wrote:
>> On 17.03.20 01:41, John Snow wrote:
>>> 79 is the PEP8 recommendation. This recommendation works well for
>>> reading patch diffs in TUI email clients.
>>
>> Also for my very GUI-y diff program (kompare).
>>
>>> Signed-off-by: John Snow 
>>> ---
>>>  tests/qemu-iotests/iotests.py | 64 +++
>>>  tests/qemu-iotests/pylintrc   |  6 +++-
>>>  2 files changed, 47 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 3d90fb157d..75fd697d77 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>
>> [...]
>>
>>> @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None):
>>>  self.pause_drive(drive, "write_aio")
>>>  return
>>>  self.qmp('human-monitor-command',
>>> - command_line='qemu-io %s "break %s bp_%s"' % (drive, 
>>> event, drive))
>>> + command_line='qemu-io %s "break %s bp_%s"'
>>> + % (drive, event, drive))
>>
>> Can we put this value in a variable instead?  I don’t like the %
>> aligning with the parameter name instead of the string value.  (I also
>> don’t like how there are no spaces around the assignment =, but around
>> the %, even though the % binds more strongly.)
>>
>>>  

I think I had another patch somewhere that added an HMP helper that
fixes this issue for this particular instance.

I can send that separately as a follow-up. I think otherwise,
unfortunately, "we" "decided" that this indent style is "best".

So I think that this patch is "correct".


(All of the other options for indent styles seemed to be worse visually,
or actively go against PEP8. While PEP8 is not a bible, every conscious
choice to disregard it generally means you will be fighting a CQA tool
at some other point in time. I have therefore adopted a "When in Rome"
policy to reduce friction wherever possible with pylint, flake8, mypy,
pycharm, and so on.)


((I would prefer we begin to deprecate uses of % and begin using
.format() and f-strings wherever possible to help alleviate this kind of
syntax, too -- but I must insist that's for another series.))


>>>  def resume_drive(self, drive):
>>>  self.qmp('human-monitor-command',
>>> - command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
>>> drive))
>>> + command_line='qemu-io %s "remove_break bp_%s"'
>>> + % (drive, drive))
>>>  
>>>  def hmp_qemu_io(self, drive, cmd):
>>>  '''Write to a given drive using an HMP command'''
>>> @@ -793,16 +805,18 @@ def dictpath(self, d, path):
>>>  idx = int(idx)
>>>  
>>>  if not isinstance(d, dict) or component not in d:
>>> -self.fail('failed path traversal for "%s" in "%s"' % 
>>> (path, str(d)))
>>> +self.fail(f'failed path traversal for "{path}" in "{d}"')
>>
>> Do we require 3.6 so that f-strings are guaranteed to work?  (I thought
>> we didn’t.  I’d be happy to use them.)
> 
> Oh.  Of course we do.  It says so in this file that this whole series is
> about.
> 

Yup. I didn't like the idea of iotests using a newer version of python
than our build system, but my concern was not shared, so we get to use
f-strings for non-buildtime tools.

End of support for Python 3.5 will be 2020-09-13; so we'll get to use
f-strings everywhere else soon, too.

--js

Re: [PATCH v4] block/nvme: introduce PMR support from NVMe 1.4 spec

[Andrzej Jakowski]

On 3/23/20 6:28 AM, Stefan Hajnoczi wrote:
> Excellent, thank you!
> 
> Reviewed-by: Stefan Hajnoczi 

Awesome, thx! Not sure about process...
Is this patch now staged for inclusion in QEMU?

Re: [PATCH v8 06/11] iotests: drop Python 3.4 compatibility code

[John Snow]

On 3/24/20 10:54 AM, Max Reitz wrote:
> On 17.03.20 01:41, John Snow wrote:
>> We no longer need to accommodate 3.4, drop this code.
> 
> Pre-3.4, actually.
> 
>> (Also, the line is over 79 characters, so drop it.)
>>
>> Touch up the docstring a little bit while we're here.
>>
>> Signed-off-by: John Snow 
>> ---
>>  tests/qemu-iotests/iotests.py | 14 +++---
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 7cd74e7cb1..3d90fb157d 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -22,6 +22,7 @@
>>  import unittest
>>  import sys
>>  import struct
>> +from typing import Optional
>>  import json
>>  import signal
>>  import logging
>> @@ -350,18 +351,17 @@ def _filter(_key, value):
>>  return value
>>  return filter_qmp(qmsg, _filter)
>>  
>> -def log(msg, filters=(), indent=None):
>> -'''Logs either a string message or a JSON serializable message (like 
>> QMP).
>> -If indent is provided, JSON serializable messages are pretty-printed.'''
>> +def log(msg, filters=(), indent: Optional[int] = None) -> None:
>> +"""
>> +Logs either a string message or a JSON serializable message (like QMP).
>> +If indent is provided, JSON serializable messages are pretty-printed.
>> +"""
> 
> I feel like I should complain about this unrelated (I think?) change,
> but I won’t.
> 

It just seemed like so little to have in its own little patch, and I
wasn't prepared to fix the docstrings in the whole file ...

... It will be a future patch that tidies up this whole file and drops
the missing-docstring ignore from pylintrc.

> Reviewed-by: Max Reitz 
> 

Thanks.

>>  for flt in filters:
>>  msg = flt(msg)
>>  if isinstance(msg, (dict, list)):
>> -# Python < 3.4 needs to know not to add whitespace when 
>> pretty-printing:
>> -separators = (', ', ': ') if indent is None else (',', ': ')
>>  # Don't sort if it's already sorted
>>  do_sort = not isinstance(msg, OrderedDict)
>> -print(json.dumps(msg, sort_keys=do_sort,
>> - indent=indent, separators=separators))
>> +print(json.dumps(msg, sort_keys=do_sort, indent=indent))
>>  else:
>>  print(msg)
>>  
>>
> 
> 

Re: [PATCH v8 11/11] iotests: use python logging for iotests.log()

[Max Reitz]

On 17.03.20 01:41, 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.
> 
> 
> An extended note on python logging:

Thanks!

> A NullHandler is added to `qemu.iotests` to stop output from being
> generated if this code is used as a library without configuring logging.
> A NullHandler is only needed at the root, so a duplicate handler is not
> needed for `qemu.iotests.diff_io`.
> 
> When logging is not configured, messages at the 'WARNING' levels or
> above are printed with default settings. The NullHandler stops this from
> occurring, which is considered good hygiene for code used as a library.
> 
> See https://docs.python.org/3/howto/logging.html#library-config
> 
> When logging is actually enabled (always at the behest of an explicit
> call by a client script), a root logger is implicitly created at the
> root, which allows messages to propagate upwards and be handled/emitted
> from the root logger with default settings.
> 
> When we want iotest logging, we attach a handler to the
> qemu.iotests.diff_io logger and disable propagation to avoid possible
> double-printing.
> 
> For more information on python logging infrastructure, I highly
> recommend downloading the pip package `logging_tree`, which provides
> convenient visualizations of the hierarchical logging configuration
> under different circumstances.
> 
> See https://pypi.org/project/logging_tree/ for more information.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/030|  4 +--
>  tests/qemu-iotests/155|  2 +-
>  tests/qemu-iotests/245|  1 +
>  tests/qemu-iotests/245.out| 24 
>  tests/qemu-iotests/iotests.py | 53 ---
>  5 files changed, 46 insertions(+), 38 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH for 5.0] block: fix bdrv_root_attach_child forget to unref child_bs

[Kevin Wolf]

Am 24.03.2020 um 16:59 hat Vladimir Sementsov-Ogievskiy geschrieben:
> bdrv_root_attach_child promises to drop child_bs reference on failure.
> It does it on first handled failure path, but not on the second. Fix
> that.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Thanks, applied to the block branch.

Kevin

[PATCH for 5.0] block: fix bdrv_root_attach_child forget to unref child_bs

[Vladimir Sementsov-Ogievskiy]

bdrv_root_attach_child promises to drop child_bs reference on failure.
It does it on first handled failure path, but not on the second. Fix
that.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block.c b/block.c
index a2542c977b..6713db773d 100644
--- a/block.c
+++ b/block.c
@@ -2612,6 +2612,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState 
*child_bs,
 error_propagate(errp, local_err);
 g_free(child);
 bdrv_abort_perm_update(child_bs);
+bdrv_unref(child_bs);
 return NULL;
 }
 }
-- 
2.21.0

Re: [PATCH 2/6] block/mirror: fix use after free of local_err

[Eric Blake]

On 3/24/20 10:36 AM, Vladimir Sementsov-Ogievskiy wrote:

local_err is used again in mirror_exit_common() after
bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
non-NULL local_err will crash.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/mirror.c | 1 +
  1 file changed, 1 insertion(+)


Reviewed-by: Eric Blake 



diff --git a/block/mirror.c b/block/mirror.c
index 447051dbc6..6203e5946e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
  bdrv_set_backing_hd(target_bs, backing, _err);
  if (local_err) {
  error_report_err(local_err);
+local_err = NULL;
  ret = -EPERM;
  }
  }



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

Re: [PATCH for-5.0 0/6] Several error use-after-free

[Richard Henderson]

On 3/24/20 8:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> Vladimir Sementsov-Ogievskiy (6):
>   scripts/coccinelle: add error-use-after-free.cocci
>   block/mirror: fix use after free of local_err
>   dump/win_dump: fix use after free of err
>   migration/colo: fix use after free of local_err
>   migration/ram: fix use after free of local_err
>   qga/commands-posix: fix use after free of local_err

Whole series:
Reviewed-by: Richard Henderson 


r~

[PATCH 6/6] qga/commands-posix: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

local_err is used several times in guest_suspend(). Setting non-NULL
local_err will crash, so let's zero it after freeing. Also fix possible
leak of local_err in final if().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qga/commands-posix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 93474ff770..cc69b82704 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1773,6 +1773,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
 }
 
 error_free(local_err);
+local_err = NULL;
 
 if (pmutils_supports_mode(mode, _err)) {
 mode_supported = true;
@@ -1784,6 +1785,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
 }
 
 error_free(local_err);
+local_err = NULL;
 
 if (linux_sys_state_supports_mode(mode, _err)) {
 mode_supported = true;
@@ -1791,6 +1793,7 @@ static void guest_suspend(SuspendMode mode, Error **errp)
 }
 
 if (!mode_supported) {
+error_free(local_err);
 error_setg(errp,
"the requested suspend mode is not supported by the guest");
 } else {
-- 
2.21.0

[PATCH 5/6] migration/ram: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

local_err is used again in migration_bitmap_sync_precopy() after
precopy_notify(), so we must zero it. Otherwise try to set
non-NULL local_err will crash.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 migration/ram.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/ram.c b/migration/ram.c
index c12cfdbe26..04f13feb2e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -980,6 +980,7 @@ static void migration_bitmap_sync_precopy(RAMState *rs)
  */
 if (precopy_notify(PRECOPY_NOTIFY_BEFORE_BITMAP_SYNC, _err)) {
 error_report_err(local_err);
+local_err = NULL;
 }
 
 migration_bitmap_sync(rs);
-- 
2.21.0

[PATCH 4/6] migration/colo: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

local_err is used again in secondary_vm_do_failover() after
replication_stop_all(), so we must zero it. Otherwise try to set
non-NULL local_err will crash.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 migration/colo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/colo.c b/migration/colo.c
index 44942c4e23..a54ac84f41 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -93,6 +93,7 @@ static void secondary_vm_do_failover(void)
 replication_stop_all(true, _err);
 if (local_err) {
 error_report_err(local_err);
+local_err = NULL;
 }
 
 /* Notify all filters of all NIC to do checkpoint */
-- 
2.21.0

[PATCH for-5.0 0/6] Several error use-after-free

[Vladimir Sementsov-Ogievskiy]

Hi all!

I accidentally found use-after-free of local_err in mirror, and decided
to search for similar cases with help of small coccinelle script
(patch 01). Happily, there no many cases.

Better to fix zero Error* pointer after each freeing everywhere, but
this is too much for 5.0 and most of these cases will be covered by
error-auto-propagation series.

Note also, that there are still a lot of use-after-free cases possible
when error is not local variable but field of some structure, shared by
several functions.

Vladimir Sementsov-Ogievskiy (6):
  scripts/coccinelle: add error-use-after-free.cocci
  block/mirror: fix use after free of local_err
  dump/win_dump: fix use after free of err
  migration/colo: fix use after free of local_err
  migration/ram: fix use after free of local_err
  qga/commands-posix: fix use after free of local_err

 scripts/coccinelle/error-use-after-free.cocci | 52 +++
 block/mirror.c|  1 +
 dump/win_dump.c   |  4 +-
 migration/colo.c  |  1 +
 migration/ram.c   |  1 +
 qga/commands-posix.c  |  3 ++
 MAINTAINERS   |  1 +
 7 files changed, 60 insertions(+), 3 deletions(-)
 create mode 100644 scripts/coccinelle/error-use-after-free.cocci

-- 
2.21.0

[PATCH 3/6] dump/win_dump: fix use after free of err

[Vladimir Sementsov-Ogievskiy]

It's possible that we'll try to set err twice (or more). It's bad, it
will crash.

Instead, use warn_report().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 dump/win_dump.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/dump/win_dump.c b/dump/win_dump.c
index eda2a48974..652c7bad99 100644
--- a/dump/win_dump.c
+++ b/dump/win_dump.c
@@ -304,13 +304,11 @@ static void restore_context(WinDumpHeader64 *h,
 struct saved_context *saved_ctx)
 {
 int i;
-Error *err = NULL;
 
 for (i = 0; i < h->NumberProcessors; i++) {
 if (cpu_memory_rw_debug(first_cpu, saved_ctx[i].addr,
 (uint8_t *)_ctx[i].ctx, sizeof(WinContext), 1)) {
-error_setg(, "win-dump: failed to restore CPU #%d context", i);
-warn_report_err(err);
+warn_report("win-dump: failed to restore CPU #%d context", i);
 }
 }
 }
-- 
2.21.0

[PATCH 2/6] block/mirror: fix use after free of local_err

[Vladimir Sementsov-Ogievskiy]

local_err is used again in mirror_exit_common() after
bdrv_set_backing_hd(), so we must zero it. Otherwise try to set
non-NULL local_err will crash.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/mirror.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/mirror.c b/block/mirror.c
index 447051dbc6..6203e5946e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
 bdrv_set_backing_hd(target_bs, backing, _err);
 if (local_err) {
 error_report_err(local_err);
+local_err = NULL;
 ret = -EPERM;
 }
 }
-- 
2.21.0

[PATCH 1/6] scripts/coccinelle: add error-use-after-free.cocci

[Vladimir Sementsov-Ogievskiy]

Add script to find and fix trivial use-after-free of Error objects.
How to use:
spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
 --macro-file scripts/cocci-macro-file.h --in-place \
 --no-show-diff ( FILES... | --use-gitgrep . )

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 scripts/coccinelle/error-use-after-free.cocci | 52 +++
 MAINTAINERS   |  1 +
 2 files changed, 53 insertions(+)
 create mode 100644 scripts/coccinelle/error-use-after-free.cocci

diff --git a/scripts/coccinelle/error-use-after-free.cocci 
b/scripts/coccinelle/error-use-after-free.cocci
new file mode 100644
index 00..7cfa42355b
--- /dev/null
+++ b/scripts/coccinelle/error-use-after-free.cocci
@@ -0,0 +1,52 @@
+// Find and fix trivial use-after-free of Error objects
+//
+// 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
+// .
+//
+// How to use:
+// spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
+//  --macro-file scripts/cocci-macro-file.h --in-place \
+//  --no-show-diff ( FILES... | --use-gitgrep . )
+
+@ exists@
+identifier fn, fn2;
+expression err;
+@@
+
+ fn(...)
+ {
+ <...
+(
+ error_free(err);
++err = NULL;
+|
+ error_report_err(err);
++err = NULL;
+|
+ error_reportf_err(err, ...);
++err = NULL;
+|
+ warn_report_err(err);
++err = NULL;
+|
+ warn_reportf_err(err, ...);
++err = NULL;
+)
+ ... when != err = NULL
+ when != exit(...)
+ fn2(..., err, ...)
+ ...>
+ }
diff --git a/MAINTAINERS b/MAINTAINERS
index b5c86ec494..ba97cc43fc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2037,6 +2037,7 @@ F: include/qemu/error-report.h
 F: qapi/error.json
 F: util/error.c
 F: util/qemu-error.c
+F: scripts/coccinelle/*err*.cocci
 
 GDB stub
 M: Alex Bennée 
-- 
2.21.0

Re: [PATCH] qcow2: Forbid discard in qcow2 v2 images with backing files

[Alberto Garcia]

On Tue 24 Mar 2020 03:46:07 PM CET, Eric Blake wrote:
>> -$QEMU_IO -c 'discard 0k 64k' "$TEST_IMG" | _filter_qemu_io
>> +poke_file "$TEST_IMG" '262144' "\x00\x00\x00\x00\x00\x00\x00\x00" # 0x4 
>> - L2 entry
>> +poke_file "$TEST_IMG" '131082' "\x00\x00" # 0x2000a - Refcount entry
>
> Instead of writing '262144' ... # 0x4, you could write
> $((0x4)) in-place.  Similarly for 131082 vs. 0x2000a.

The exiting poke_file line in that test was using base 10 so I decided
to use it too for consistency.

I actually realized that $rb_offset and $l2_offset are defined, so I
could use those too.

> Also, Max has pending patches for adding poke_file_be; if those land
> first, this becomes simpler as:
>
> poke_file_be "$TEST_IMG" $((0x4)) 8 0 # L2 entry
> poke_file_be "$TEST_IMG" $((0x2000a)) 2 0 # Refcount entry

I'm fine if those lines are changed when the patch is committed.

Berto

Re: [PATCH v8 07/11] iotests: limit line length to 79 chars

[Max Reitz]

On 24.03.20 16:02, Max Reitz wrote:
> On 17.03.20 01:41, John Snow wrote:
>> 79 is the PEP8 recommendation. This recommendation works well for
>> reading patch diffs in TUI email clients.
> 
> Also for my very GUI-y diff program (kompare).
> 
>> Signed-off-by: John Snow 
>> ---
>>  tests/qemu-iotests/iotests.py | 64 +++
>>  tests/qemu-iotests/pylintrc   |  6 +++-
>>  2 files changed, 47 insertions(+), 23 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 3d90fb157d..75fd697d77 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
> 
> [...]
> 
>> @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None):
>>  self.pause_drive(drive, "write_aio")
>>  return
>>  self.qmp('human-monitor-command',
>> - command_line='qemu-io %s "break %s bp_%s"' % (drive, 
>> event, drive))
>> + command_line='qemu-io %s "break %s bp_%s"'
>> + % (drive, event, drive))
> 
> Can we put this value in a variable instead?  I don’t like the %
> aligning with the parameter name instead of the string value.  (I also
> don’t like how there are no spaces around the assignment =, but around
> the %, even though the % binds more strongly.)
> 
>>  
>>  def resume_drive(self, drive):
>>  self.qmp('human-monitor-command',
>> - command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
>> drive))
>> + command_line='qemu-io %s "remove_break bp_%s"'
>> + % (drive, drive))
>>  
>>  def hmp_qemu_io(self, drive, cmd):
>>  '''Write to a given drive using an HMP command'''
>> @@ -793,16 +805,18 @@ def dictpath(self, d, path):
>>  idx = int(idx)
>>  
>>  if not isinstance(d, dict) or component not in d:
>> -self.fail('failed path traversal for "%s" in "%s"' % (path, 
>> str(d)))
>> +self.fail(f'failed path traversal for "{path}" in "{d}"')
> 
> Do we require 3.6 so that f-strings are guaranteed to work?  (I thought
> we didn’t.  I’d be happy to use them.)

Oh.  Of course we do.  It says so in this file that this whole series is
about.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v8 07/11] iotests: limit line length to 79 chars

[Max Reitz]

On 17.03.20 01:41, John Snow wrote:
> 79 is the PEP8 recommendation. This recommendation works well for
> reading patch diffs in TUI email clients.

Also for my very GUI-y diff program (kompare).

> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 64 +++
>  tests/qemu-iotests/pylintrc   |  6 +++-
>  2 files changed, 47 insertions(+), 23 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 3d90fb157d..75fd697d77 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py

[...]

> @@ -529,11 +539,13 @@ def pause_drive(self, drive, event=None):
>  self.pause_drive(drive, "write_aio")
>  return
>  self.qmp('human-monitor-command',
> - command_line='qemu-io %s "break %s bp_%s"' % (drive, event, 
> drive))
> + command_line='qemu-io %s "break %s bp_%s"'
> + % (drive, event, drive))

Can we put this value in a variable instead?  I don’t like the %
aligning with the parameter name instead of the string value.  (I also
don’t like how there are no spaces around the assignment =, but around
the %, even though the % binds more strongly.)

>  
>  def resume_drive(self, drive):
>  self.qmp('human-monitor-command',
> - command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
> drive))
> + command_line='qemu-io %s "remove_break bp_%s"'
> + % (drive, drive))
>  
>  def hmp_qemu_io(self, drive, cmd):
>  '''Write to a given drive using an HMP command'''
> @@ -793,16 +805,18 @@ def dictpath(self, d, path):
>  idx = int(idx)
>  
>  if not isinstance(d, dict) or component not in d:
> -self.fail('failed path traversal for "%s" in "%s"' % (path, 
> str(d)))
> +self.fail(f'failed path traversal for "{path}" in "{d}"')

Do we require 3.6 so that f-strings are guaranteed to work?  (I thought
we didn’t.  I’d be happy to use them.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v8 06/11] iotests: drop Python 3.4 compatibility code

[Max Reitz]

On 17.03.20 01:41, John Snow wrote:
> We no longer need to accommodate 3.4, drop this code.

Pre-3.4, actually.

> (Also, the line is over 79 characters, so drop it.)
> 
> Touch up the docstring a little bit while we're here.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 7cd74e7cb1..3d90fb157d 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -22,6 +22,7 @@
>  import unittest
>  import sys
>  import struct
> +from typing import Optional
>  import json
>  import signal
>  import logging
> @@ -350,18 +351,17 @@ def _filter(_key, value):
>  return value
>  return filter_qmp(qmsg, _filter)
>  
> -def log(msg, filters=(), indent=None):
> -'''Logs either a string message or a JSON serializable message (like 
> QMP).
> -If indent is provided, JSON serializable messages are pretty-printed.'''
> +def log(msg, filters=(), indent: Optional[int] = None) -> None:
> +"""
> +Logs either a string message or a JSON serializable message (like QMP).
> +If indent is provided, JSON serializable messages are pretty-printed.
> +"""

I feel like I should complain about this unrelated (I think?) change,
but I won’t.

Reviewed-by: Max Reitz 

>  for flt in filters:
>  msg = flt(msg)
>  if isinstance(msg, (dict, list)):
> -# Python < 3.4 needs to know not to add whitespace when 
> pretty-printing:
> -separators = (', ', ': ') if indent is None else (',', ': ')
>  # Don't sort if it's already sorted
>  do_sort = not isinstance(msg, OrderedDict)
> -print(json.dumps(msg, sort_keys=do_sort,
> - indent=indent, separators=separators))
> +print(json.dumps(msg, sort_keys=do_sort, indent=indent))
>  else:
>  print(msg)
>  
> 




signature.asc
Description: OpenPGP digital signature

Re: [PULL 0/6] Block patches for 5.0-rc0

[Peter Maydell]

On Tue, 24 Mar 2020 at 12:21, Max Reitz  wrote:
>
> The following changes since commit f1e748d27996e0cd8269db837a32e453dd55930a:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' 
> into staging (2020-03-23 20:54:24 +)
>
> are available in the Git repository at:
>
>   https://github.com/XanClic/qemu.git tags/pull-block-2020-03-24
>
> for you to fetch changes up to c264e5d2f9f5d73977eac8e5d084f727b3d07ea9:
>
>   iotests/026: Move v3-exclusive test to new file (2020-03-24 12:05:31 +0100)
>
> 
> Block patches for 5.0-rc0:
> - Use-after-free fix
> - Fix for a memleak in an error path
> - Preventative measures against other potential use-after-frees, and
>   against NULL deferences at runtime
> - iotest fixes

Applied, thanks.

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

-- PMM

Re: [PATCH v8 05/11] iotests: add pylintrc file

[Max Reitz]

On 17.03.20 01:40, John Snow wrote:
> This allows others to get repeatable results with pylint. If you run
> `pylint iotests.py`, you should see a 100% pass.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/pylintrc | 22 ++
>  1 file changed, 22 insertions(+)
>  create mode 100644 tests/qemu-iotests/pylintrc

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] nvme: Print 'cqid' for nvme_del_cq

[Philippe Mathieu-Daudé]

On 3/24/20 3:06 PM, Minwoo Im wrote:

The given argument for this trace should be cqid, not sqid.

Signed-off-by: Minwoo Im 
---
  hw/block/trace-events | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/trace-events b/hw/block/trace-events
index f78939fa9da1..bf6d11b58b85 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -37,7 +37,7 @@ nvme_rw(const char *verb, uint32_t blk_count, uint64_t 
byte_count, uint64_t lba)
  nvme_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t qsize, uint16_t qflags) "create submission queue, 
addr=0x%"PRIx64", sqid=%"PRIu16", cqid=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16""
  nvme_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t size, uint16_t qflags, int ien) "create completion queue, 
addr=0x%"PRIx64", cqid=%"PRIu16", vector=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16", 
ien=%d"
  nvme_del_sq(uint16_t qid) "deleting submission queue sqid=%"PRIu16""
-nvme_del_cq(uint16_t cqid) "deleted completion queue, sqid=%"PRIu16""
+nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
  nvme_identify_ctrl(void) "identify controller"
  nvme_identify_ns(uint16_t ns) "identify namespace, nsid=%"PRIu16""
  nvme_identify_nslist(uint16_t ns) "identify namespace list, nsid=%"PRIu16""



Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] qcow2: Forbid discard in qcow2 v2 images with backing files

[Eric Blake]

On 3/23/20 2:44 PM, Alberto Garcia wrote:

A discard request deallocates the selected clusters so they read back
as zeroes. This is done by clearing the cluster offset field and
setting QCOW_OFLAG_ZERO in the L2 entry.

This flag is however only supported when qcow_version >= 3. In older
images the cluster is simply deallocated, exposing any possible stale
data from the backing file.

Since discard is an advisory operation it's safer to simply forbid it
in this scenario.

Note that we are adding this check to qcow2_co_pdiscard() and not to
qcow2_cluster_discard() or discard_in_l2_slice() because the last
two are also used by qcow2_snapshot_create() to discard the clusters
used by the VM state. In this case there's no risk of exposing stale
data to the guest and we really want that the clusters are always
discarded.

Signed-off-by: Alberto Garcia 
---
  block/qcow2.c  |  6 +++
  tests/qemu-iotests/060 |  5 ++-
  tests/qemu-iotests/060.out |  2 -
  tests/qemu-iotests/289 | 90 ++
  tests/qemu-iotests/289.out | 52 ++
  tests/qemu-iotests/group   |  1 +
  6 files changed, 152 insertions(+), 4 deletions(-)
  create mode 100755 tests/qemu-iotests/289
  create mode 100644 tests/qemu-iotests/289.out


The actual fix is much smaller than the iotest fallout ;)


+++ b/tests/qemu-iotests/060
@@ -167,9 +167,10 @@ _make_test_img -o 'compat=0.10' -b "$BACKING_IMG" 1G
  # Write two clusters, the second one enforces creation of an L2 table after
  # the first data cluster.
  $QEMU_IO -c 'write 0k 64k' -c 'write 512M 64k' "$TEST_IMG" | _filter_qemu_io
-# Discard the first cluster. This cluster will soon enough be reallocated and
+# Free the first cluster. This cluster will soon enough be reallocated and
  # used for COW.
-$QEMU_IO -c 'discard 0k 64k' "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" '262144' "\x00\x00\x00\x00\x00\x00\x00\x00" # 0x4 - 
L2 entry
+poke_file "$TEST_IMG" '131082' "\x00\x00" # 0x2000a - Refcount entry


Instead of writing '262144' ... # 0x4, you could write $((0x4)) 
in-place.  Similarly for 131082 vs. 0x2000a.


Also, Max has pending patches for adding poke_file_be; if those land 
first, this becomes simpler as:


poke_file_be "$TEST_IMG" $((0x4)) 8 0 # L2 entry
poke_file_be "$TEST_IMG" $((0x2000a)) 2 0 # Refcount entry


+++ b/tests/qemu-iotests/289
@@ -0,0 +1,90 @@
+#!/usr/bin/env bash
+#
+# Test how 'qemu-io -c discard' behaves on v2 and v3 qcow2 images
At any rate, the new test looks reasonable to me. I see you have other 
review comments for improving it, with thos in, you can add


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 3/10/20 12:22 PM, Max Reitz wrote:

   +# 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=''


Noticing that this is not in yet, I have one more suggestion:

The initial doc comment is not helpful without reading the rest of the 
function: Is 512 the offset or the value being written?  Better might be:


# poke_file_le test.img $offset $width $value



+
+# poke_file_be 'test.img' 512 2 65279
+poke_file_be()


and here, too.

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

Re: [PATCH v8 04/11] iotests: replace mutable list default args

[Max Reitz]

On 17.03.20 01:40, John Snow wrote:
> It's bad hygiene: if we modify this list, it will be modified across all
> invocations.
> 
> (Remaining bad usages are fixed in a subsequent patch which changes the
> function signature anyway.)
> 
> Signed-off-by: John Snow 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  tests/qemu-iotests/iotests.py | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] block: make BlockConf.*_size properties 32-bit

[Eric Blake]

On 2/13/20 7:55 AM, Roman Kagan wrote:

On Thu, Feb 13, 2020 at 06:47:10AM -0600, Eric Blake wrote:

On 2/13/20 2:01 AM, Roman Kagan wrote:

On Wed, Feb 12, 2020 at 03:44:19PM -0600, Eric Blake wrote:

On 2/11/20 5:54 AM, Roman Kagan wrote:

Devices (virtio-blk, scsi, etc.) and the block layer are happy to use
32-bit for logical_block_size, physical_block_size, and min_io_size.
However, the properties in BlockConf are defined as uint16_t limiting
the values to 32768.

This appears unnecessary tight, and we've seen bigger block sizes handy
at times.


What larger sizes?  I could see 64k or maybe even 1M block sizes,...


We played exactly with these two :)



Make them 32 bit instead and lift the limitation.



@@ -716,30 +716,32 @@ const PropertyInfo qdev_prop_pci_devfn = {
/* --- blocksize --- */
+#define MIN_BLOCK_SIZE 512
+#define MAX_BLOCK_SIZE 2147483648


...but 2G block sizes are going to have tremendous performance problems.

I'm not necessarily opposed to the widening to a 32-bit type, but think you
need more justification or a smaller number for the max block size,


I thought any smaller value would just be arbitrary and hard to reason
about, so I went ahead with the max value that fit in the type and could
be made visibile to the guest.


You've got bigger problems than what is visible to the guest. block/qcow2.c
operates on a cluster at a time; if you are stating that it now requires
reading multiple clusters to operate on one, qcow2 will have to do lots of
wasteful read-modify-write cycles.


I'm failing to see how this is supposed to happen.  The guest will issue
requests bigger than the cluster size; why would it cause RMW?

Big logical_block_size would cause RMW in the guest if it wants to
perform smaller writes, but that's up to the user to take this tradeoff,
isn't it?


You really need a strong reason to
support a maximum larger than 2M other than just "so the guest can
experiment with it".


Do I get you right that your suggestion is to cap the block size
property at 2MB?


Yes, for now, I think 2M is a better maximum than 2G or 4G unless you 
have benchmark data to prove that a larger maximum does not cause problems.


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

[PATCH] nvme: Print 'cqid' for nvme_del_cq

[Minwoo Im]

The given argument for this trace should be cqid, not sqid.

Signed-off-by: Minwoo Im 
---
 hw/block/trace-events | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/trace-events b/hw/block/trace-events
index f78939fa9da1..bf6d11b58b85 100644
--- a/hw/block/trace-events
+++ b/hw/block/trace-events
@@ -37,7 +37,7 @@ nvme_rw(const char *verb, uint32_t blk_count, uint64_t 
byte_count, uint64_t lba)
 nvme_create_sq(uint64_t addr, uint16_t sqid, uint16_t cqid, uint16_t qsize, 
uint16_t qflags) "create submission queue, addr=0x%"PRIx64", sqid=%"PRIu16", 
cqid=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16""
 nvme_create_cq(uint64_t addr, uint16_t cqid, uint16_t vector, uint16_t size, 
uint16_t qflags, int ien) "create completion queue, addr=0x%"PRIx64", 
cqid=%"PRIu16", vector=%"PRIu16", qsize=%"PRIu16", qflags=%"PRIu16", ien=%d"
 nvme_del_sq(uint16_t qid) "deleting submission queue sqid=%"PRIu16""
-nvme_del_cq(uint16_t cqid) "deleted completion queue, sqid=%"PRIu16""
+nvme_del_cq(uint16_t cqid) "deleted completion queue, cqid=%"PRIu16""
 nvme_identify_ctrl(void) "identify controller"
 nvme_identify_ns(uint16_t ns) "identify namespace, nsid=%"PRIu16""
 nvme_identify_nslist(uint16_t ns) "identify namespace list, nsid=%"PRIu16""
-- 
2.17.1

Re: [PATCH v8 03/11] iotests: ignore import warnings from pylint

[Max Reitz]

On 17.03.20 01:40, 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.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v8 01/11] iotests: do a light delinting

[Max Reitz]

On 17.03.20 01:40, 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.
> 
> Signed-off-by: John Snow 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  tests/qemu-iotests/iotests.py | 83 ++-
>  1 file changed, 43 insertions(+), 40 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: Potential Null dereference

[Kevin Wolf]

Am 24.03.2020 um 13:37 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 24.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote:
> > Aha, new crashes! Let's look at them.
> > 
> > 41 and 155 failed with crash, 141 without but I see "+{"error": {"class": 
> > "GenericError", "desc": "Block device drv0 is in use"}}" in its output.
> > 
> > Hmm, but all crashes are because of assert(QTAILQ_EMPTY(_bdrv_states)); 
> > in bdrv_close_all..
> > 
> > So, we have a problem, but another one..
> 
> Investigate it a bit more.
> 
> Accordingly to comment, bdrv_attach_child unrefs child bs even on failure, so 
> let's do
> 
> --- a/block.c
> +++ b/block.c
> @@ -2768,6 +2768,7 @@ void bdrv_set_backing_hd_ex(BlockDriverState *bs, 
> BlockDriverState *backing_hd,
> 
>  if (bdrv_attach_child_fail) {
>  bs->backing = NULL;
> +bdrv_unref(backing_hd);
>  error_setg(errp, "Unpredicted failure :)");
>  } else {
>  bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
> _backing,
> 
> 
> - then, all three tests fails, but without crashes. OK.
> 
> The only interesting thing is that, it seems that bdrv_attach_child doesn't 
> unref child on all failure paths.
> 
> It calls bdrv_root_attach_child..
> 
> which doesn't unref child on the path
> 
> if (bdrv_get_aio_context(child_bs) != ctx) {
>   ...
>if (ret < 0) {
>  error_propagate(errp, local_err);
>  g_free(child);
>  bdrv_abort_perm_update(child_bs);
>  return NULL;
>}
> }
> 
> or am I wrong?

I think you're right, we need a bdrv_unref() there. The function comment
makes it clear that bdrv_root_attach_child() is supposed to consume the
reference both in success and error cases.

Kevin

Re: [PATCH v2] qcow2: Forbid discard in qcow2 v2 images with backing files

[Max Reitz]

On 24.03.20 13:16, Alberto Garcia wrote:
> A discard request deallocates the selected clusters so they read back
> as zeroes. This is done by clearing the cluster offset field and
> setting QCOW_OFLAG_ZERO in the L2 entry.
> 
> This flag is however only supported when qcow_version >= 3. In older
> images the cluster is simply deallocated, exposing any possible stale
> data from the backing file.
> 
> Since discard is an advisory operation it's safer to simply forbid it
> in this scenario.
> 
> Note that we are adding this check to qcow2_co_pdiscard() and not to
> qcow2_cluster_discard() or discard_in_l2_slice() because the last
> two are also used by qcow2_snapshot_create() to discard the clusters
> used by the VM state. In this case there's no risk of exposing stale
> data to the guest and we really want that the clusters are always
> discarded.
> 
> Signed-off-by: Alberto Garcia 
> ---
> v2:
> 
> - Don't create the image with compat=0.10 in iotest 060 [Max]
> - Use $TEST_IMG.base for the backing image name in iotest 289 [Max]
> - Add list of unsupported options to iotest 289 [Max]
> 
>  block/qcow2.c  |  6 +++
>  tests/qemu-iotests/060 | 10 ++---
>  tests/qemu-iotests/060.out |  2 -
>  tests/qemu-iotests/289 | 91 ++
>  tests/qemu-iotests/289.out | 52 ++
>  tests/qemu-iotests/group   |  1 +
>  6 files changed, 154 insertions(+), 8 deletions(-)
>  create mode 100755 tests/qemu-iotests/289
>  create mode 100644 tests/qemu-iotests/289.out

Reviewed-by: Max Reitz 

(Maybe someone else wants to give a comment still, though)



signature.asc
Description: OpenPGP digital signature

Re: Potential Null dereference

[Vladimir Sementsov-Ogievskiy]

24.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote:

24.03.2020 12:50, Kevin Wolf wrote:

Am 24.03.2020 um 08:14 hat Philippe Mathieu-Daudé geschrieben:

On 3/24/20 4:05 AM, Mansour Ahmadi wrote:

Hi,

Nullness of  needs to be checked here:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221

pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...


Do you have a reproducer? It's not obvious to me how bs->backing could
be NULL here.



While it is done at 2 other locations:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477


Commit 18775ff3269 made the change for mirror, however its commit
message is terse and doesn't say anything about the scenario where it
would happen. We also didn't add a test case for it. I would have
expected that failure to add the backing file would immediately error
out and not try to refresh the filename first.

backup-top.c has the check from the beginning. I assume it just copied
it from mirror.

Vladimir, do you remember the details?



No :(
But I believe that "Backing may be zero after failed bdrv_attach_child in 
bdrv_set_backing_hd, which leads to SIGSEGV." means real problem, probably 
reproduced on some experiment branch, not on master.

Let's try most simple check:

apply the following:

=
diff --git a/include/block/block.h b/include/block/block.h
index e569a4d747..dc20b55075 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -298,6 +298,8 @@ BdrvChild *bdrv_open_child(const char *filename,
     const BdrvChildRole *child_role,
     bool allow_none, Error **errp);
  BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+    bool bdrv_attach_child_fail, Error **errp);
  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
   Error **errp);
  int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
diff --git a/block.c b/block.c
index a2542c977b..21b6124d73 100644
--- a/block.c
+++ b/block.c
@@ -2743,8 +2743,8 @@ static bool bdrv_inherits_from_recursive(BlockDriverState 
*child,
   * Sets the backing file link of a BDS. A new reference is created; callers
   * which don't need their own reference any more must call bdrv_unref().
   */
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
- Error **errp)
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+    bool bdrv_attach_child_fail, Error **errp)
  {
  bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
  bdrv_inherits_from_recursive(backing_hd, bs);
@@ -2766,8 +2766,14 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd,
  goto out;
  }

-    bs->backing = bdrv_attach_child(bs, backing_hd, "backing", _backing,
-    errp);
+    if (bdrv_attach_child_fail) {
+    bs->backing = NULL;
+    error_setg(errp, "Unpredicted failure :)");
+    } else {
+    bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
_backing,
+    errp);
+    }
+
  /* If backing_hd was already part of bs's backing chain, and
   * inherits_from pointed recursively to bs then let's update it to
   * point directly to bs (else it will become NULL). */
@@ -2779,6 +2785,12 @@ out:
  bdrv_refresh_limits(bs, NULL);
  }

+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+ Error **errp)
+{
+    bdrv_set_backing_hd_ex(bs, backing_hd, false, errp);
+}
  /*
   * Opens the backing file for a BlockDriverState if not yet open
   *
diff --git a/block/mirror.c b/block/mirror.c
index 447051dbc6..907bb2b718 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -675,7 +675,7 @@ static int mirror_exit_common(Job *job)
  if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
  BlockDriverState *backing = s->is_none_mode ? src : s->base;
  if (backing_bs(target_bs) != backing) {
-    bdrv_set_backing_hd(target_bs, backing, _err);
+    bdrv_set_backing_hd_ex(target_bs, backing, true, _err);
  if (local_err) {
  error_report_err(local_err);
  ret = -EPERM;
@@ -1477,6 +1477,7 @@ static void 
bdrv_mirror_top_refresh_filename(BlockDriverState *bs)
  if (bs->backing == NULL) {
  /* we can be here after failed bdrv_attach_child in
   * bdrv_set_backing_hd */
+    abort();
  return;
  }
  pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),

[PULL 6/6] iotests/026: Move v3-exclusive test to new file

[Max Reitz]

data_file does not work with v2, and we probably want 026 to keep
working for v2 images.  Thus, open a new file for v3-exclusive error
path test cases.

Fixes: 81311255f217859413c94f2cd9cebf2684bbda94
   (“iotests/026: Test EIO on allocation in a data-file”)
Signed-off-by: Max Reitz 
Message-Id: <20200311140707.1243218-1-mre...@redhat.com>
Reviewed-by: John Snow 
Tested-by: John Snow 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/026 | 31 ---
 tests/qemu-iotests/026.out |  6 --
 tests/qemu-iotests/026.out.nocache |  6 --
 tests/qemu-iotests/289 | 89 ++
 tests/qemu-iotests/289.out |  8 +++
 tests/qemu-iotests/group   |  1 +
 6 files changed, 98 insertions(+), 43 deletions(-)
 create mode 100755 tests/qemu-iotests/289
 create mode 100644 tests/qemu-iotests/289.out

diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
index b05a4692cf..b9713eb591 100755
--- a/tests/qemu-iotests/026
+++ b/tests/qemu-iotests/026
@@ -240,37 +240,6 @@ $QEMU_IO -c "write 0 $CLUSTER_SIZE" "$BLKDBG_TEST_IMG" | 
_filter_qemu_io
 
 _check_test_img
 
-echo
-echo === Avoid freeing external data clusters on failure ===
-echo
-
-# Similar test as the last one, except we test what happens when there
-# is an error when writing to an external data file instead of when
-# writing to a preallocated zero cluster
-_make_test_img -o "data_file=$TEST_IMG.data_file" $CLUSTER_SIZE
-
-# Put blkdebug above the data-file, and a raw node on top of that so
-# that blkdebug will see a write_aio event and emit an error
-$QEMU_IO -c "write 0 $CLUSTER_SIZE" \
-"json:{
- 'driver': 'qcow2',
- 'file': { 'driver': 'file', 'filename': '$TEST_IMG' },
- 'data-file': {
- 'driver': 'raw',
- 'file': {
- 'driver': 'blkdebug',
- 'config': '$TEST_DIR/blkdebug.conf',
- 'image': {
- 'driver': 'file',
- 'filename': '$TEST_IMG.data_file'
- }
- }
- }
- }" \
-| _filter_qemu_io
-
-_check_test_img
-
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/026.out b/tests/qemu-iotests/026.out
index c1b3b58482..83989996ff 100644
--- a/tests/qemu-iotests/026.out
+++ b/tests/qemu-iotests/026.out
@@ -653,10 +653,4 @@ wrote 1024/1024 bytes at offset 0
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 write failed: Input/output error
 No errors were found on the image.
-
-=== Avoid freeing external data clusters on failure ===
-
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024 
data_file=TEST_DIR/t.IMGFMT.data_file
-write failed: Input/output error
-No errors were found on the image.
 *** done
diff --git a/tests/qemu-iotests/026.out.nocache 
b/tests/qemu-iotests/026.out.nocache
index 8d5001648a..9359d26d7e 100644
--- a/tests/qemu-iotests/026.out.nocache
+++ b/tests/qemu-iotests/026.out.nocache
@@ -661,10 +661,4 @@ wrote 1024/1024 bytes at offset 0
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 write failed: Input/output error
 No errors were found on the image.
-
-=== Avoid freeing external data clusters on failure ===
-
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024 
data_file=TEST_DIR/t.IMGFMT.data_file
-write failed: Input/output error
-No errors were found on the image.
 *** done
diff --git a/tests/qemu-iotests/289 b/tests/qemu-iotests/289
new file mode 100755
index 00..1c11d4030e
--- /dev/null
+++ b/tests/qemu-iotests/289
@@ -0,0 +1,89 @@
+#!/usr/bin/env bash
+#
+# qcow2 v3-exclusive error path testing
+# (026 tests paths common to v2 and v3)
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+rm "$TEST_DIR/blkdebug.conf"
+rm -f "$TEST_IMG.data_file"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+_supported_fmt qcow2
+_supported_proto file
+# This is a v3-exclusive test;
+# As for data_file, error paths often very much depend on whether
+# there is an external data file or not; so we create one exactly when
+# we want to test it
+_unsupported_imgopts 

[PULL 0/6] Block patches for 5.0-rc0

[Max Reitz]

The following changes since commit f1e748d27996e0cd8269db837a32e453dd55930a:

  Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' into 
staging (2020-03-23 20:54:24 +)

are available in the Git repository at:

  https://github.com/XanClic/qemu.git tags/pull-block-2020-03-24

for you to fetch changes up to c264e5d2f9f5d73977eac8e5d084f727b3d07ea9:

  iotests/026: Move v3-exclusive test to new file (2020-03-24 12:05:31 +0100)


Block patches for 5.0-rc0:
- Use-after-free fix
- Fix for a memleak in an error path
- Preventative measures against other potential use-after-frees, and
  against NULL deferences at runtime
- iotest fixes


Eric Blake (1):
  block: Avoid memleak on qcow2 image info failure

Max Reitz (2):
  iotests: Fix cleanup path in some tests
  iotests/026: Move v3-exclusive test to new file

Philippe Mathieu-Daudé (1):
  block: Assert BlockDriver::format_name is not NULL

Vladimir Sementsov-Ogievskiy (2):
  block: bdrv_set_backing_bs: fix use-after-free
  block/qcow2: zero data_file child after free

 block.c|  3 +-
 block/qcow2.c  |  3 +
 tests/qemu-iotests/026 | 31 ---
 tests/qemu-iotests/026.out |  6 --
 tests/qemu-iotests/026.out.nocache |  6 --
 tests/qemu-iotests/085 |  1 +
 tests/qemu-iotests/087 |  6 ++
 tests/qemu-iotests/279 |  2 +-
 tests/qemu-iotests/289 | 89 ++
 tests/qemu-iotests/289.out |  8 +++
 tests/qemu-iotests/group   |  1 +
 11 files changed, 111 insertions(+), 45 deletions(-)
 create mode 100755 tests/qemu-iotests/289
 create mode 100644 tests/qemu-iotests/289.out

-- 
2.25.1

[PULL 5/6] iotests: Fix cleanup path in some tests

[Max Reitz]

Some iotests leave behind some external data file when run for qcow2
with -o data_file.  Fix that.

Signed-off-by: Max Reitz 
Message-Id: <20200224171631.384314-1-mre...@redhat.com>
Reviewed-by: Eric Blake 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/085 | 1 +
 tests/qemu-iotests/087 | 6 ++
 tests/qemu-iotests/279 | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 46981dbb64..dd3c993a2d 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -39,6 +39,7 @@ SNAPSHOTS=10
 _cleanup()
 {
 _cleanup_qemu
+_cleanup_test_img
 for i in $(seq 1 ${SNAPSHOTS})
 do
 _rm_test_img "${TEST_DIR}/${i}-${snapshot_virt0}"
diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index d6c8613419..bdfdad3454 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -26,6 +26,12 @@ 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
+
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
diff --git a/tests/qemu-iotests/279 b/tests/qemu-iotests/279
index 30d29b1cb2..75a4747e6b 100755
--- a/tests/qemu-iotests/279
+++ b/tests/qemu-iotests/279
@@ -26,7 +26,7 @@ status=1  # failure is the default!
 _cleanup()
 {
 _cleanup_test_img
-rm -f "$TEST_IMG.mid"
+_rm_test_img "$TEST_IMG.mid"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
-- 
2.25.1

[PULL 3/6] block: bdrv_set_backing_bs: fix use-after-free

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy 

There is a use-after-free possible: bdrv_unref_child() leaves
bs->backing freed but not NULL. bdrv_attach_child may produce nested
polling loop due to drain, than access of freed pointer is possible.

I've produced the following crash on 30 iotest with modified code. It
does not reproduce on master, but still seems possible:

#0  __strcmp_avx2 () at /lib64/libc.so.6
#1  bdrv_backing_overridden (bs=0x55c9d3cc2060) at block.c:6350
#2  bdrv_refresh_filename (bs=0x55c9d3cc2060) at block.c:6404
#3  bdrv_backing_attach (c=0x55c9d48e5520) at block.c:1063
#4  bdrv_replace_child_noperm
(child=child@entry=0x55c9d48e5520,
new_bs=new_bs@entry=0x55c9d3cc2060) at block.c:2290
#5  bdrv_replace_child
(child=child@entry=0x55c9d48e5520,
new_bs=new_bs@entry=0x55c9d3cc2060) at block.c:2320
#6  bdrv_root_attach_child
(child_bs=child_bs@entry=0x55c9d3cc2060,
child_name=child_name@entry=0x55c9d241d478 "backing",
child_role=child_role@entry=0x55c9d26ecee0 ,
ctx=, perm=, shared_perm=21,
opaque=0x55c9d3c5a3d0, errp=0x7ffd117108e0) at block.c:2424
#7  bdrv_attach_child
(parent_bs=parent_bs@entry=0x55c9d3c5a3d0,
child_bs=child_bs@entry=0x55c9d3cc2060,
child_name=child_name@entry=0x55c9d241d478 "backing",
child_role=child_role@entry=0x55c9d26ecee0 ,
errp=errp@entry=0x7ffd117108e0) at block.c:5876
#8  in bdrv_set_backing_hd
(bs=bs@entry=0x55c9d3c5a3d0,
backing_hd=backing_hd@entry=0x55c9d3cc2060,
errp=errp@entry=0x7ffd117108e0)
at block.c:2576
#9  stream_prepare (job=0x55c9d49d84a0) at block/stream.c:150
#10 job_prepare (job=0x55c9d49d84a0) at job.c:761
#11 job_txn_apply (txn=, fn=) at
job.c:145
#12 job_do_finalize (job=0x55c9d49d84a0) at job.c:778
#13 job_completed_txn_success (job=0x55c9d49d84a0) at job.c:832
#14 job_completed (job=0x55c9d49d84a0) at job.c:845
#15 job_completed (job=0x55c9d49d84a0) at job.c:836
#16 job_exit (opaque=0x55c9d49d84a0) at job.c:864
#17 aio_bh_call (bh=0x55c9d471a160) at util/async.c:117
#18 aio_bh_poll (ctx=ctx@entry=0x55c9d3c46720) at util/async.c:117
#19 aio_poll (ctx=ctx@entry=0x55c9d3c46720,
blocking=blocking@entry=true)
at util/aio-posix.c:728
#20 bdrv_parent_drained_begin_single (poll=true, c=0x55c9d3d558f0)
at block/io.c:121
#21 bdrv_parent_drained_begin_single (c=c@entry=0x55c9d3d558f0,
poll=poll@entry=true)
at block/io.c:114
#22 bdrv_replace_child_noperm
(child=child@entry=0x55c9d3d558f0,
new_bs=new_bs@entry=0x55c9d3d27300) at block.c:2258
#23 bdrv_replace_child
(child=child@entry=0x55c9d3d558f0,
new_bs=new_bs@entry=0x55c9d3d27300) at block.c:2320
#24 bdrv_root_attach_child
(child_bs=child_bs@entry=0x55c9d3d27300,
child_name=child_name@entry=0x55c9d241d478 "backing",
child_role=child_role@entry=0x55c9d26ecee0 ,
ctx=, perm=, shared_perm=21,
opaque=0x55c9d3cc2060, errp=0x7ffd11710c60) at block.c:2424
#25 bdrv_attach_child
(parent_bs=parent_bs@entry=0x55c9d3cc2060,
child_bs=child_bs@entry=0x55c9d3d27300,
child_name=child_name@entry=0x55c9d241d478 "backing",
child_role=child_role@entry=0x55c9d26ecee0 ,
errp=errp@entry=0x7ffd11710c60) at block.c:5876
#26 bdrv_set_backing_hd
(bs=bs@entry=0x55c9d3cc2060,
backing_hd=backing_hd@entry=0x55c9d3d27300,
errp=errp@entry=0x7ffd11710c60)
at block.c:2576
#27 stream_prepare (job=0x55c9d495ead0) at block/stream.c:150
...

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20200316060631.30052-2-vsement...@virtuozzo.com>
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 6b984dc883..cccae5add9 100644
--- a/block.c
+++ b/block.c
@@ -2760,10 +2760,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd,
 
 if (bs->backing) {
 bdrv_unref_child(bs, bs->backing);
+bs->backing = NULL;
 }
 
 if (!backing_hd) {
-bs->backing = NULL;
 goto out;
 }
 
-- 
2.25.1

[PULL 4/6] block/qcow2: zero data_file child after free

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy 

data_file being NULL doesn't seem to be a correct state, but it's
better than dead pointer and simpler to debug.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20200316060631.30052-3-vsement...@virtuozzo.com>
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block/qcow2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index e08917ed84..d1da3d91db 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1758,6 +1758,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 g_free(s->image_data_file);
 if (has_data_file(bs)) {
 bdrv_unref_child(bs, s->data_file);
+s->data_file = NULL;
 }
 g_free(s->unknown_header_fields);
 cleanup_unknown_header_ext(bs);
@@ -2621,6 +2622,7 @@ static void qcow2_close(BlockDriverState *bs)
 
 if (has_data_file(bs)) {
 bdrv_unref_child(bs, s->data_file);
+s->data_file = NULL;
 }
 
 qcow2_refcount_close(bs);
-- 
2.25.1

[PULL 1/6] block: Avoid memleak on qcow2 image info failure

[Max Reitz]

From: Eric Blake 

If we fail to get bitmap info, we must not leak the encryption info.

Fixes: b8968c875f403
Fixes: Coverity CID 1421894
Signed-off-by: Eric Blake 
Message-Id: <20200320183620.1112123-1-ebl...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Andrey Shinkevich 
Tested-by: Andrey Shinkevich 
Signed-off-by: Max Reitz 
---
 block/qcow2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index d44b45633d..e08917ed84 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4811,6 +4811,7 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs,
 if (local_err) {
 error_propagate(errp, local_err);
 qapi_free_ImageInfoSpecific(spec_info);
+qapi_free_QCryptoBlockInfo(encrypt_info);
 return NULL;
 }
 *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
-- 
2.25.1

[PULL 2/6] block: Assert BlockDriver::format_name is not NULL

[Max Reitz]

From: Philippe Mathieu-Daudé 

bdrv_do_find_format() calls strcmp() using BlockDriver::format_name
as argument, which must not be NULL. Assert this field is not null
when we register a block driver in bdrv_register().

Reported-by: Mansour Ahmadi 
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <2020031835.23856-1-phi...@redhat.com>
Reviewed-by: Alberto Garcia 
Signed-off-by: Max Reitz 
---
 block.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block.c b/block.c
index a2542c977b..6b984dc883 100644
--- a/block.c
+++ b/block.c
@@ -363,6 +363,7 @@ char *bdrv_get_full_backing_filename(BlockDriverState *bs, 
Error **errp)
 
 void bdrv_register(BlockDriver *bdrv)
 {
+assert(bdrv->format_name);
 QLIST_INSERT_HEAD(_drivers, bdrv, list);
 }
 
-- 
2.25.1

[PATCH v2] qcow2: Forbid discard in qcow2 v2 images with backing files

[Alberto Garcia]

A discard request deallocates the selected clusters so they read back
as zeroes. This is done by clearing the cluster offset field and
setting QCOW_OFLAG_ZERO in the L2 entry.

This flag is however only supported when qcow_version >= 3. In older
images the cluster is simply deallocated, exposing any possible stale
data from the backing file.

Since discard is an advisory operation it's safer to simply forbid it
in this scenario.

Note that we are adding this check to qcow2_co_pdiscard() and not to
qcow2_cluster_discard() or discard_in_l2_slice() because the last
two are also used by qcow2_snapshot_create() to discard the clusters
used by the VM state. In this case there's no risk of exposing stale
data to the guest and we really want that the clusters are always
discarded.

Signed-off-by: Alberto Garcia 
---
v2:

- Don't create the image with compat=0.10 in iotest 060 [Max]
- Use $TEST_IMG.base for the backing image name in iotest 289 [Max]
- Add list of unsupported options to iotest 289 [Max]

 block/qcow2.c  |  6 +++
 tests/qemu-iotests/060 | 10 ++---
 tests/qemu-iotests/060.out |  2 -
 tests/qemu-iotests/289 | 91 ++
 tests/qemu-iotests/289.out | 52 ++
 tests/qemu-iotests/group   |  1 +
 6 files changed, 154 insertions(+), 8 deletions(-)
 create mode 100755 tests/qemu-iotests/289
 create mode 100644 tests/qemu-iotests/289.out

diff --git a/block/qcow2.c b/block/qcow2.c
index d44b45633d..7bb7e392e1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3763,6 +3763,12 @@ static coroutine_fn int 
qcow2_co_pdiscard(BlockDriverState *bs,
 int ret;
 BDRVQcow2State *s = bs->opaque;
 
+/* If the image does not support QCOW_OFLAG_ZERO then discarding
+ * clusters could expose stale data from the backing file. */
+if (s->qcow_version < 3 && bs->backing) {
+return -ENOTSUP;
+}
+
 if (!QEMU_IS_ALIGNED(offset | bytes, s->cluster_size)) {
 assert(bytes < s->cluster_size);
 /* Ignore partial clusters, except for the special case of the
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 043f12904a..8820290be6 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -160,16 +160,14 @@ TEST_IMG=$BACKING_IMG _make_test_img 1G
 
 $QEMU_IO -c 'write 0k 64k' "$BACKING_IMG" | _filter_qemu_io
 
-# compat=0.10 is required in order to make the following discard actually
-# unallocate the sector rather than make it a zero sector - we want COW, after
-# all.
-_make_test_img -o 'compat=0.10' -b "$BACKING_IMG" 1G
+_make_test_img -b "$BACKING_IMG" 1G
 # Write two clusters, the second one enforces creation of an L2 table after
 # the first data cluster.
 $QEMU_IO -c 'write 0k 64k' -c 'write 512M 64k' "$TEST_IMG" | _filter_qemu_io
-# Discard the first cluster. This cluster will soon enough be reallocated and
+# Free the first cluster. This cluster will soon enough be reallocated and
 # used for COW.
-$QEMU_IO -c 'discard 0k 64k' "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" '262144' "\x00\x00\x00\x00\x00\x00\x00\x00" # 0x4 - 
L2 entry
+poke_file "$TEST_IMG" '131082' "\x00\x00" # 0x2000a - Refcount entry
 # Now, corrupt the image by marking the second L2 table cluster as free.
 poke_file "$TEST_IMG" '131084' "\x00\x00" # 0x2000c
 # Start a write operation requiring COW on the image stopping it right before
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index d27692a33c..09caaea865 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -105,8 +105,6 @@ wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 65536/65536 bytes at offset 536870912
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-discard 65536/65536 bytes at offset 0
-64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qcow2: Marking image as corrupt: Preventing invalid write on metadata 
(overlaps with active L2 table); further corruption events will be suppressed
 blkdebug: Suspended request '0'
 write failed: Input/output error
diff --git a/tests/qemu-iotests/289 b/tests/qemu-iotests/289
new file mode 100755
index 00..629c992d79
--- /dev/null
+++ b/tests/qemu-iotests/289
@@ -0,0 +1,91 @@
+#!/usr/bin/env bash
+#
+# Test how 'qemu-io -c discard' behaves on v2 and v3 qcow2 images
+#
+# Copyright (C) 2020 Igalia, S.L.
+# Author: Alberto Garcia 
+#
+# 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

Re: [PATCH] qcow2: Forbid discard in qcow2 v2 images with backing files

[Max Reitz]

On 23.03.20 20:44, Alberto Garcia wrote:
> A discard request deallocates the selected clusters so they read back
> as zeroes. This is done by clearing the cluster offset field and
> setting QCOW_OFLAG_ZERO in the L2 entry.
> 
> This flag is however only supported when qcow_version >= 3. In older
> images the cluster is simply deallocated, exposing any possible stale
> data from the backing file.
> 
> Since discard is an advisory operation it's safer to simply forbid it
> in this scenario.
> 
> Note that we are adding this check to qcow2_co_pdiscard() and not to
> qcow2_cluster_discard() or discard_in_l2_slice() because the last
> two are also used by qcow2_snapshot_create() to discard the clusters
> used by the VM state. In this case there's no risk of exposing stale
> data to the guest and we really want that the clusters are always
> discarded.

Sounds good to me.

> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.c  |  6 +++
>  tests/qemu-iotests/060 |  5 ++-
>  tests/qemu-iotests/060.out |  2 -
>  tests/qemu-iotests/289 | 90 ++
>  tests/qemu-iotests/289.out | 52 ++
>  tests/qemu-iotests/group   |  1 +
>  6 files changed, 152 insertions(+), 4 deletions(-)
>  create mode 100755 tests/qemu-iotests/289
>  create mode 100644 tests/qemu-iotests/289.out
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d44b45633d..7bb7e392e1 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3763,6 +3763,12 @@ static coroutine_fn int 
> qcow2_co_pdiscard(BlockDriverState *bs,
>  int ret;
>  BDRVQcow2State *s = bs->opaque;
>  
> +/* If the image does not support QCOW_OFLAG_ZERO then discarding
> + * clusters could expose stale data from the backing file. */
> +if (s->qcow_version < 3 && bs->backing) {
> +return -ENOTSUP;
> +}
> +
>  if (!QEMU_IS_ALIGNED(offset | bytes, s->cluster_size)) {
>  assert(bytes < s->cluster_size);
>  /* Ignore partial clusters, except for the special case of the
> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
> index 043f12904a..4a4fdfb1e1 100755
> --- a/tests/qemu-iotests/060
> +++ b/tests/qemu-iotests/060
> @@ -167,9 +167,10 @@ _make_test_img -o 'compat=0.10' -b "$BACKING_IMG" 1G

More context: This image is created with -o 'compat=0.10', just because
a discard on such an image would result in the cluster being freed.  We
can drop that compat=0.10 bit now.

>  # Write two clusters, the second one enforces creation of an L2 table after
>  # the first data cluster.
>  $QEMU_IO -c 'write 0k 64k' -c 'write 512M 64k' "$TEST_IMG" | _filter_qemu_io
> -# Discard the first cluster. This cluster will soon enough be reallocated and
> +# Free the first cluster. This cluster will soon enough be reallocated and
>  # used for COW.
> -$QEMU_IO -c 'discard 0k 64k' "$TEST_IMG" | _filter_qemu_io
> +poke_file "$TEST_IMG" '262144' "\x00\x00\x00\x00\x00\x00\x00\x00" # 0x4 
> - L2 entry
> +poke_file "$TEST_IMG" '131082' "\x00\x00" # 0x2000a - Refcount entry
>  # Now, corrupt the image by marking the second L2 table cluster as free.
>  poke_file "$TEST_IMG" '131084' "\x00\x00" # 0x2000c
>  # Start a write operation requiring COW on the image stopping it right before

[...]

> diff --git a/tests/qemu-iotests/289 b/tests/qemu-iotests/289
> new file mode 100755
> index 00..13b4984721
> --- /dev/null
> +++ b/tests/qemu-iotests/289

[...]

> +_cleanup()
> +{
> +_cleanup_test_img
> +rm -f "$TEST_IMG.backing"

I’d call the image $TEST_IMG.base so _cleanup_test_img picks up on it.
(rm-ing test images is also wrong, because with external data files,
there will be more than one file.  It doesn’t matter here anyway because
this test doesn’t support external data files, but, well.)

> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux

I’d mark the compat option unsupported because this test will ignore it.
 Furthermore, the refcount_bits and data_file options are really
unsupported, because they won’t work with compat=0.10.

The test itself looks good.

Max



signature.asc
Description: OpenPGP digital signature

Re: Potential Null dereference

[Vladimir Sementsov-Ogievskiy]

24.03.2020 12:50, Kevin Wolf wrote:

Am 24.03.2020 um 08:14 hat Philippe Mathieu-Daudé geschrieben:

On 3/24/20 4:05 AM, Mansour Ahmadi wrote:

Hi,

Nullness of  needs to be checked here:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221

pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...


Do you have a reproducer? It's not obvious to me how bs->backing could
be NULL here.



While it is done at 2 other locations:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477


Commit 18775ff3269 made the change for mirror, however its commit
message is terse and doesn't say anything about the scenario where it
would happen. We also didn't add a test case for it. I would have
expected that failure to add the backing file would immediately error
out and not try to refresh the filename first.

backup-top.c has the check from the beginning. I assume it just copied
it from mirror.

Vladimir, do you remember the details?



No :(
But I believe that "Backing may be zero after failed bdrv_attach_child in 
bdrv_set_backing_hd, which leads to SIGSEGV." means real problem, probably 
reproduced on some experiment branch, not on master.

Let's try most simple check:

apply the following:

=
diff --git a/include/block/block.h b/include/block/block.h
index e569a4d747..dc20b55075 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -298,6 +298,8 @@ BdrvChild *bdrv_open_child(const char *filename,
const BdrvChildRole *child_role,
bool allow_none, Error **errp);
 BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+bool bdrv_attach_child_fail, Error **errp);
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
  Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
diff --git a/block.c b/block.c
index a2542c977b..21b6124d73 100644
--- a/block.c
+++ b/block.c
@@ -2743,8 +2743,8 @@ static bool bdrv_inherits_from_recursive(BlockDriverState 
*child,
  * Sets the backing file link of a BDS. A new reference is created; callers
  * which don't need their own reference any more must call bdrv_unref().
  */
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
- Error **errp)
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+bool bdrv_attach_child_fail, Error **errp)
 {
 bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
 bdrv_inherits_from_recursive(backing_hd, bs);
@@ -2766,8 +2766,14 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd,
 goto out;
 }

-bs->backing = bdrv_attach_child(bs, backing_hd, "backing", _backing,
-errp);
+if (bdrv_attach_child_fail) {
+bs->backing = NULL;
+error_setg(errp, "Unpredicted failure :)");
+} else {
+bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
_backing,
+errp);
+}
+
 /* If backing_hd was already part of bs's backing chain, and
  * inherits_from pointed recursively to bs then let's update it to
  * point directly to bs (else it will become NULL). */
@@ -2779,6 +2785,12 @@ out:
 bdrv_refresh_limits(bs, NULL);
 }

+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+ Error **errp)
+{
+bdrv_set_backing_hd_ex(bs, backing_hd, false, errp);
+}
 /*
  * Opens the backing file for a BlockDriverState if not yet open
  *
diff --git a/block/mirror.c b/block/mirror.c
index 447051dbc6..907bb2b718 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -675,7 +675,7 @@ static int mirror_exit_common(Job *job)
 if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
 BlockDriverState *backing = s->is_none_mode ? src : s->base;
 if (backing_bs(target_bs) != backing) {
-bdrv_set_backing_hd(target_bs, backing, _err);
+bdrv_set_backing_hd_ex(target_bs, backing, true, _err);
 if (local_err) {
 error_report_err(local_err);
 ret = -EPERM;
@@ -1477,6 +1477,7 @@ static void 
bdrv_mirror_top_refresh_filename(BlockDriverState *bs)
 if (bs->backing == NULL) {
 /* we can be here after failed bdrv_attach_child in
  * bdrv_set_backing_hd */
+abort();
 return;
 }
 pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),


==

[root@kvm qemu-iotests]# git grep -il mirror ??? | xargs
030 041 044 

Re: [PATCH] iotests/026: Move v3-exclusive test to new file

[Max Reitz]

On 11.03.20 15:07, Max Reitz wrote:
> data_file does not work with v2, and we probably want 026 to keep
> working for v2 images.  Thus, open a new file for v3-exclusive error
> path test cases.
> 
> Fixes: 81311255f217859413c94f2cd9cebf2684bbda94
>(“iotests/026: Test EIO on allocation in a data-file”)
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/026 | 31 ---
>  tests/qemu-iotests/026.out |  6 --
>  tests/qemu-iotests/026.out.nocache |  6 --
>  tests/qemu-iotests/289 | 89 ++
>  tests/qemu-iotests/289.out |  8 +++
>  tests/qemu-iotests/group   |  1 +
>  6 files changed, 98 insertions(+), 43 deletions(-)
>  create mode 100755 tests/qemu-iotests/289
>  create mode 100644 tests/qemu-iotests/289.out

Thanks for the review and test, applied to my block branch.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] iotests: Fix cleanup path in some tests

[Max Reitz]

On 24.02.20 18:16, Max Reitz wrote:
> Some iotests leave behind some external data file when run for qcow2
> with -o data_file.  Fix that.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/085 | 1 +
>  tests/qemu-iotests/087 | 6 ++
>  tests/qemu-iotests/279 | 2 +-
>  3 files changed, 8 insertions(+), 1 deletion(-)

Thanks for the review, applied to my block branch.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] iotests: drop group file

[Kevin Wolf]

Am 24.03.2020 um 11:21 hat Daniel P. Berrangé geschrieben:
> On Tue, Mar 24, 2020 at 01:02:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > 24.03.2020 12:36, Kevin Wolf wrote:
> > > Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > > When sending iotests to upstream or do patch porting from one branch
> > > > to another we very often have to resolve conflicts in group file, as
> > > > many absolutely independent features are intersecting by this file.
> > > > These conflicts are simple, but imagine how much time we all have
> > > > already spent on resolving them? Let's finally get rid of group file.
> > > > 
> > > > This patch transposes group info: instead of collecting it in one file,
> > > > let each test define its groups by itself.
> > > > 
> > > > Several steps are done to achive it:
> > > > 
> > > > 1. Define groups in test files automatically:
> > > > 
> > > >  grep '^[0-9]\{3\} ' group | while read line; do
> > > >  file=$(awk '{print $1}' <<< "$line");
> > > >  groups=$(sed -e 's/^... //' <<< "$line");
> > > >  awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
> > > >  cat tmp > $file;
> > > >  done
> > > > 
> > > > 2. Copy groups documentation into docs/devel/testing.rst, which already
> > > > has a section about iotests.
> > > > 
> > > > 3. Modify check script to work without group file.
> > > > 
> > > > Here is a logic change: before, even if test do not belong to any
> > > > group (only iotest 142 currently) it should be defined in group
> > > > file. Now, test is not forced to define any group. Instead check
> > > > considers all files with names matching [0-9][0-9][0-9] as tests.
> > > 
> > > This has both a positive and a negative effect: Now you don't have to
> > > modify another file when you add a new test, but it will be picked up
> > > automatically. However, if you want to disable a test, you could
> > > previously just remove it from groups (or comment it out), and now you
> > > have to delete the test instead.
> > 
> > Hmm. Probably, you could add it to group "disabled", and run check -x 
> > disabled.
> 
> As a developer you don't really want to be making changes to git tracked
> files in order to temporarily skip a test, as then git reports them as
> modified & you risk accidentally committing throwaway changes.
> 
> Better to have a separate groups.local file to record local overrides
> in a non-tracked file.

For locally disabling tests, we have that 'expunged' file that nobody
knows about and that I just found when we were reformatting the ./check
output...

I wouldn't want to use that for disabling the tests in a downstream
repository, though, it should stay a local thing even there.

Kevin

Re: [PATCH] iotests: drop group file

[Daniel P . Berrangé]

On Tue, Mar 24, 2020 at 01:02:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 24.03.2020 12:36, Kevin Wolf wrote:
> > Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > When sending iotests to upstream or do patch porting from one branch
> > > to another we very often have to resolve conflicts in group file, as
> > > many absolutely independent features are intersecting by this file.
> > > These conflicts are simple, but imagine how much time we all have
> > > already spent on resolving them? Let's finally get rid of group file.
> > > 
> > > This patch transposes group info: instead of collecting it in one file,
> > > let each test define its groups by itself.
> > > 
> > > Several steps are done to achive it:
> > > 
> > > 1. Define groups in test files automatically:
> > > 
> > >  grep '^[0-9]\{3\} ' group | while read line; do
> > >  file=$(awk '{print $1}' <<< "$line");
> > >  groups=$(sed -e 's/^... //' <<< "$line");
> > >  awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
> > >  cat tmp > $file;
> > >  done
> > > 
> > > 2. Copy groups documentation into docs/devel/testing.rst, which already
> > > has a section about iotests.
> > > 
> > > 3. Modify check script to work without group file.
> > > 
> > > Here is a logic change: before, even if test do not belong to any
> > > group (only iotest 142 currently) it should be defined in group
> > > file. Now, test is not forced to define any group. Instead check
> > > considers all files with names matching [0-9][0-9][0-9] as tests.
> > 
> > This has both a positive and a negative effect: Now you don't have to
> > modify another file when you add a new test, but it will be picked up
> > automatically. However, if you want to disable a test, you could
> > previously just remove it from groups (or comment it out), and now you
> > have to delete the test instead.
> 
> Hmm. Probably, you could add it to group "disabled", and run check -x 
> disabled.

As a developer you don't really want to be making changes to git tracked
files in order to temporarily skip a test, as then git reports them as
modified & you risk accidentally committing throwaway changes.

Better to have a separate groups.local file to record local overrides
in a non-tracked file.


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

Re: [PATCH v2 07/12] tests/acceptance: Remove shebang header

[Alex Bennée]

Philippe Mathieu-Daudé  writes:

> Patch created mechanically by running:
>
>   $ chmod 644 $(git grep -lF '#!/usr/bin/env python' \
>   | xargs grep -L 'if __name__.*__main__')
>   $ sed -i "/^#\!\/usr\/bin\/\(env\ \)\?python.\?$/d" \
>   $(git grep -lF '#!/usr/bin/env python' \
>   | xargs grep -L 'if __name__.*__main__')

OK, but my question is why? Aren't shebangs considered good practice for
finding the executable for a script?

If the acceptance scripts are special in this regard we should say why
in the commit message.

>
> Reported-by: Vladimir Sementsov-Ogievskiy 
> Suggested-by: Stefan Hajnoczi 
> Reviewed-by: Wainer dos Santos Moschetta 
> Acked-by: Stefan Hajnoczi 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/acceptance/virtio_seg_max_adjust.py  | 1 -
>  tests/acceptance/x86_cpu_model_versions.py | 1 -
>  2 files changed, 2 deletions(-)
>  mode change 100755 => 100644 tests/acceptance/virtio_seg_max_adjust.py
>
> diff --git a/tests/acceptance/virtio_seg_max_adjust.py 
> b/tests/acceptance/virtio_seg_max_adjust.py
> old mode 100755
> new mode 100644
> index 5458573138..8d4f24da49
> --- a/tests/acceptance/virtio_seg_max_adjust.py
> +++ b/tests/acceptance/virtio_seg_max_adjust.py
> @@ -1,4 +1,3 @@
> -#!/usr/bin/env python
>  #
>  # Test virtio-scsi and virtio-blk queue settings for all machine types
>  #
> diff --git a/tests/acceptance/x86_cpu_model_versions.py 
> b/tests/acceptance/x86_cpu_model_versions.py
> index 90558d9a71..01ff614ec2 100644
> --- a/tests/acceptance/x86_cpu_model_versions.py
> +++ b/tests/acceptance/x86_cpu_model_versions.py
> @@ -1,4 +1,3 @@
> -#!/usr/bin/env python
>  #
>  # Basic validation of x86 versioned CPU models and CPU model aliases
>  #


-- 
Alex Bennée

Re: block stream and bitmaps

[Kevin Wolf]

Am 23.03.2020 um 19:06 hat John Snow geschrieben:
> Hi Kevin,
> 
> I'm hoping to get some preliminary ideas from you (capped at five
> minutes' effort?) on this subject. My ideas here are a bit shaky, I only
> have really rough notions here.
> 
> We want to use bitmaps with 'drive' semantics; i.e. tracking changes to
> the visible guest data. What we have are bitmaps with node semantics,
> tracking changes to the data at a particular level in the graph.
> 
> For commit, this isn't a big deal: we can disable bitmaps in the backing
> file while we commit and then re-enable it on completion. We usually
> have a separate bitmap enabled on the root node that is recording writes
> during this time, and can be moved later.
> 
> For streaming, this is more challenging: new writes will dirty the
> bitmap, but so will writes from the stream job itself.
> 
> Semantically, we want to ignore writes from the stream while recording
> them from the guest -- differentiating based on source.

No, based on source is actually not what you want. What you really want
is that BDRV_REQ_WRITE_UNCHANGED doesn't mark any blocks dirty.

We discussed this specific case of streaming at FOSDEM (with Paolo and
probably Nir). Paolo was even convinced that unchanged writes already
behave like this, but we agreed that dirtying blocks for them would be a
bug. After checking that the code is indeed buggy, I was planning to
send a patch, but never got around to actually do that. Sorry about
that.

> Bitmaps aren't really geared to do that right now. With the changes to
> Bdrv Roles that Max was engineering, do you think it's possible to add
> some kind of write source discrimination to bitmaps, or is that too messy?

I don't think it would work because copy-on-read requests come from the
same parent node as writes (no matter whether the legacy code in
block/io.c or a copy-on-read filter node is used).

> For both commit and stream, it might be nice to say: "This bitmap is
> enabled, but ignores writes from [all? specific types? specific
> instances?] jobs.

Commit is a bit trickier, because it's not WRITE_UNCHANGED. The result
is only unchanged for the top layer, but not for the backing file you're
committing to. Not sure if we can represent this condition somehow.

> Or, I wonder if what we truly want is some kind of bitmap "forwarder"
> object on block-backend objects that represent the semantic drive view,
> and only writes through that *backend* get forwarded to the bitmaps
> attached to whatever node the bitmap is actually associated with.
> 
> (That might wind up causing weird problems too, though... since those
> objects are no longer intended to be user-addressable, managing that
> configuration might get intensely strange.)

Hm... Drive-based does suggest that it's managed at the BlockBackend
level. So having a bitmap that isn't added as a dirty bitmap to the BDS,
but only to the BB does make sense to me. The BB would be addressed
with the qdev ID of the device, as usual (which underlines that it's
really per device).

The part that's unclear to me is how to make such bitmaps persistent.
You can change the root node of a BB and even remove the root node
completely (for removable devices; but even changing is technically
remove followed by insert), so you may need to move the bitmap around
between image files and at least for some time you might not have any
place to store the bitmap.

Or you say that you store it in one specific node, be it the root node
of the BB or not, and it will always stay there no matter how you change
the graph and whether the BB and that node are even in the same subtree.
That node would just get an additonal refcount, so you can't remove it
until the BB goes away.

Unless you already have a better plan (I hope you do, I didn't think
about it for more than a few minutes), maybe the latter would actually
be the most reasonable solution.

Kevin

Re: [PATCH] iotests: drop group file

[Denis V. Lunev]

On 3/24/20 12:36 PM, Kevin Wolf wrote:
> Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> When sending iotests to upstream or do patch porting from one branch
>> to another we very often have to resolve conflicts in group file, as
>> many absolutely independent features are intersecting by this file.
>> These conflicts are simple, but imagine how much time we all have
>> already spent on resolving them? Let's finally get rid of group file.
>>
>> This patch transposes group info: instead of collecting it in one file,
>> let each test define its groups by itself.
>>
>> Several steps are done to achive it:
>>
>> 1. Define groups in test files automatically:
>>
>> grep '^[0-9]\{3\} ' group | while read line; do
>> file=$(awk '{print $1}' <<< "$line");
>> groups=$(sed -e 's/^... //' <<< "$line");
>> awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
>> cat tmp > $file;
>> done
>>
>> 2. Copy groups documentation into docs/devel/testing.rst, which already
>>has a section about iotests.
>>
>> 3. Modify check script to work without group file.
>>
>>Here is a logic change: before, even if test do not belong to any
>>group (only iotest 142 currently) it should be defined in group
>>file. Now, test is not forced to define any group. Instead check
>>considers all files with names matching [0-9][0-9][0-9] as tests.
> This has both a positive and a negative effect: Now you don't have to
> modify another file when you add a new test, but it will be picked up
> automatically. However, if you want to disable a test, you could
> previously just remove it from groups (or comment it out), and now you
> have to delete the test instead.
>
> Downstream, I think we still disable a few tests because we're compiling
> out features that are required for some tests to pass, and deleting the
> test cases completely would probably move conflicts just to a different
> place.
>
> So I think we need a method to distuingish an enabled test that is in no
> group from a disabled test.
+1 for blacklist.local file

>
>>check script is also refactored to make it simple to do next cool
>>thing about iotests: allow meaningful names for test-case files.
> This one would also require us to be able to distinguish test case files
> from arbitrary other files.
>
>> -#
>> -# test-group association ... one line per test
>> -#
>> -001 rw auto quick
>> -002 rw auto quick
>> -003 rw auto
>> -004 rw auto quick
>> -005 img auto quick
>> -# 006 was removed, do not reuse
> We lose these comments without a replacement. I wonder whether it's
> important or if we can think of another way to make sure that numbers
> aren't reused. (I'm not completely sure any more why we decided that we
> don't want to reuse numbers. Maybe because of backports?)
we could generate this file with a starter script with proper
option.

Den

Re: [PATCH] iotests: drop group file

[Vladimir Sementsov-Ogievskiy]

24.03.2020 12:51, Daniel P. Berrangé wrote:

On Tue, Mar 24, 2020 at 10:36:29AM +0100, Kevin Wolf wrote:

Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:

When sending iotests to upstream or do patch porting from one branch
to another we very often have to resolve conflicts in group file, as
many absolutely independent features are intersecting by this file.
These conflicts are simple, but imagine how much time we all have
already spent on resolving them? Let's finally get rid of group file.

This patch transposes group info: instead of collecting it in one file,
let each test define its groups by itself.

Several steps are done to achive it:

1. Define groups in test files automatically:

 grep '^[0-9]\{3\} ' group | while read line; do
 file=$(awk '{print $1}' <<< "$line");
 groups=$(sed -e 's/^... //' <<< "$line");
 awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
 cat tmp > $file;
 done

2. Copy groups documentation into docs/devel/testing.rst, which already
has a section about iotests.

3. Modify check script to work without group file.

Here is a logic change: before, even if test do not belong to any
group (only iotest 142 currently) it should be defined in group
file. Now, test is not forced to define any group. Instead check
considers all files with names matching [0-9][0-9][0-9] as tests.


This has both a positive and a negative effect: Now you don't have to
modify another file when you add a new test, but it will be picked up
automatically. However, if you want to disable a test, you could
previously just remove it from groups (or comment it out), and now you
have to delete the test instead.

Downstream, I think we still disable a few tests because we're compiling
out features that are required for some tests to pass, and deleting the
test cases completely would probably move conflicts just to a different
place.

So I think we need a method to distuingish an enabled test that is in no
group from a disabled test.



Can we just have a file "blacklist.local" (which doesn't exist by default)
where you list tests to skip locally ?

Or a "group.local" such that any info in this group.local file will replace
the default info extracted from the test file ?


check script is also refactored to make it simple to do next cool
thing about iotests: allow meaningful names for test-case files.


This one would also require us to be able to distinguish test case files
from arbitrary other files.


A test-X.sh or a XX.test  naming convention ?


-#
-# test-group association ... one line per test
-#
-001 rw auto quick
-002 rw auto quick
-003 rw auto
-004 rw auto quick
-005 img auto quick
-# 006 was removed, do not reuse


We lose these comments without a replacement. I wonder whether it's
important or if we can think of another way to make sure that numbers
aren't reused. (I'm not completely sure any more why we decided that we
don't want to reuse numbers. Maybe because of backports?)


The key goal of the patch is to avoid conflicts from clashing changes
between branches. To full achieve that goal we need to both avoid
touching the shared groups file, but more than than, we must avoid
creating clashing test filenames.

If we keep using 3-digit filenames then the goal of this patch is
not achieved, as the risk of clashes is still higher. IOW, we must
switch to a more verbose naming convention to increase the entropy
and thus eliminate risk of clashes. If this is done, then the idea
of reserving previously used test names ceases to be something to
worry about.


Hm. You are right, better to solve all these problems together.


--
Best regards,
Vladimir

Re: [PATCH] iotests: drop group file

[Vladimir Sementsov-Ogievskiy]

24.03.2020 12:36, Kevin Wolf wrote:

Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:

When sending iotests to upstream or do patch porting from one branch
to another we very often have to resolve conflicts in group file, as
many absolutely independent features are intersecting by this file.
These conflicts are simple, but imagine how much time we all have
already spent on resolving them? Let's finally get rid of group file.

This patch transposes group info: instead of collecting it in one file,
let each test define its groups by itself.

Several steps are done to achive it:

1. Define groups in test files automatically:

 grep '^[0-9]\{3\} ' group | while read line; do
 file=$(awk '{print $1}' <<< "$line");
 groups=$(sed -e 's/^... //' <<< "$line");
 awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
 cat tmp > $file;
 done

2. Copy groups documentation into docs/devel/testing.rst, which already
has a section about iotests.

3. Modify check script to work without group file.

Here is a logic change: before, even if test do not belong to any
group (only iotest 142 currently) it should be defined in group
file. Now, test is not forced to define any group. Instead check
considers all files with names matching [0-9][0-9][0-9] as tests.


This has both a positive and a negative effect: Now you don't have to
modify another file when you add a new test, but it will be picked up
automatically. However, if you want to disable a test, you could
previously just remove it from groups (or comment it out), and now you
have to delete the test instead.


Hmm. Probably, you could add it to group "disabled", and run check -x disabled.



Downstream, I think we still disable a few tests because we're compiling
out features that are required for some tests to pass, and deleting the
test cases completely would probably move conflicts just to a different
place.


I came to fixing all test in downstream, correctly skipping test-cases which
needs compiled-out feature. We don't comment out tests in group, all tests
should pass or be correctly skipped by check.



So I think we need a method to distuingish an enabled test that is in no
group from a disabled test.


What about "disabled" group?




check script is also refactored to make it simple to do next cool
thing about iotests: allow meaningful names for test-case files.


This one would also require us to be able to distinguish test case files
from arbitrary other files.


I think, something like "all files started with 'test-' prefix"




-#
-# test-group association ... one line per test
-#
-001 rw auto quick
-002 rw auto quick
-003 rw auto
-004 rw auto quick
-005 img auto quick
-# 006 was removed, do not reuse


We lose these comments without a replacement. I wonder whether it's
important or if we can think of another way to make sure that numbers
aren't reused. (I'm not completely sure any more why we decided that we
don't want to reuse numbers. Maybe because of backports?)



Hmm.. Okay, if we had this test in past, than drop, than reuse number for the
another test, we possibly may break bisecting.

I can add notes about it into documentation (nobody will read it). Anyway, I
don't think it is too bad.

And if we move to textual file-names for tests, we will not reuse old removed
numbers anyway.

--
Best regards,
Vladimir

Re: [PATCH] iotests: drop group file

[Daniel P . Berrangé]

On Tue, Mar 24, 2020 at 10:36:29AM +0100, Kevin Wolf wrote:
> Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > When sending iotests to upstream or do patch porting from one branch
> > to another we very often have to resolve conflicts in group file, as
> > many absolutely independent features are intersecting by this file.
> > These conflicts are simple, but imagine how much time we all have
> > already spent on resolving them? Let's finally get rid of group file.
> > 
> > This patch transposes group info: instead of collecting it in one file,
> > let each test define its groups by itself.
> > 
> > Several steps are done to achive it:
> > 
> > 1. Define groups in test files automatically:
> > 
> > grep '^[0-9]\{3\} ' group | while read line; do
> > file=$(awk '{print $1}' <<< "$line");
> > groups=$(sed -e 's/^... //' <<< "$line");
> > awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
> > cat tmp > $file;
> > done
> > 
> > 2. Copy groups documentation into docs/devel/testing.rst, which already
> >has a section about iotests.
> > 
> > 3. Modify check script to work without group file.
> > 
> >Here is a logic change: before, even if test do not belong to any
> >group (only iotest 142 currently) it should be defined in group
> >file. Now, test is not forced to define any group. Instead check
> >considers all files with names matching [0-9][0-9][0-9] as tests.
> 
> This has both a positive and a negative effect: Now you don't have to
> modify another file when you add a new test, but it will be picked up
> automatically. However, if you want to disable a test, you could
> previously just remove it from groups (or comment it out), and now you
> have to delete the test instead.
> 
> Downstream, I think we still disable a few tests because we're compiling
> out features that are required for some tests to pass, and deleting the
> test cases completely would probably move conflicts just to a different
> place.
> 
> So I think we need a method to distuingish an enabled test that is in no
> group from a disabled test.


Can we just have a file "blacklist.local" (which doesn't exist by default)
where you list tests to skip locally ?

Or a "group.local" such that any info in this group.local file will replace
the default info extracted from the test file ?

> >check script is also refactored to make it simple to do next cool
> >thing about iotests: allow meaningful names for test-case files.
> 
> This one would also require us to be able to distinguish test case files
> from arbitrary other files.

A test-X.sh or a XX.test  naming convention ?

> > -#
> > -# test-group association ... one line per test
> > -#
> > -001 rw auto quick
> > -002 rw auto quick
> > -003 rw auto
> > -004 rw auto quick
> > -005 img auto quick
> > -# 006 was removed, do not reuse
> 
> We lose these comments without a replacement. I wonder whether it's
> important or if we can think of another way to make sure that numbers
> aren't reused. (I'm not completely sure any more why we decided that we
> don't want to reuse numbers. Maybe because of backports?)

The key goal of the patch is to avoid conflicts from clashing changes
between branches. To full achieve that goal we need to both avoid
touching the shared groups file, but more than than, we must avoid
creating clashing test filenames.

If we keep using 3-digit filenames then the goal of this patch is
not achieved, as the risk of clashes is still higher. IOW, we must
switch to a more verbose naming convention to increase the entropy
and thus eliminate risk of clashes. If this is done, then the idea
of reserving previously used test names ceases to be something to
worry about.

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

Re: Potential Null dereference

[Kevin Wolf]

Am 24.03.2020 um 08:14 hat Philippe Mathieu-Daudé geschrieben:
> On 3/24/20 4:05 AM, Mansour Ahmadi wrote:
> > Hi,
> > 
> > Nullness of  needs to be checked here:
> > https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221
> > 
> > pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...

Do you have a reproducer? It's not obvious to me how bs->backing could
be NULL here.

> > 
> > While it is done at 2 other locations:
> > https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
> > https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477

Commit 18775ff3269 made the change for mirror, however its commit
message is terse and doesn't say anything about the scenario where it
would happen. We also didn't add a test case for it. I would have
expected that failure to add the backing file would immediately error
out and not try to refresh the filename first.

backup-top.c has the check from the beginning. I assume it just copied
it from mirror.

Vladimir, do you remember the details?

Kevin

Re: [PULL for-5.0 0/1] Block patches

[Peter Maydell]

On Mon, 23 Mar 2020 at 19:24, Stefan Hajnoczi  wrote:
>
> The following changes since commit 29e0855c5af62bbb0b0b6fed792e004dad92ba95:
>
>   Merge remote-tracking branch 'remotes/elmarco/tags/slirp-pull-request' into 
> staging (2020-03-22 21:00:38 +)
>
> are available in the Git repository at:
>
>   https://github.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to ff807d559205a434fd37d3343f01a0ab128bd190:
>
>   aio-posix: fix io_uring with external events (2020-03-23 11:05:44 +)
>
> 
> Pull request
>
> 
>
> Stefan Hajnoczi (1):
>   aio-posix: fix io_uring with external events
>
>  util/fdmon-io_uring.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)


Applied, thanks.

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

-- PMM

Re: [PATCH] iotests: drop group file

[Kevin Wolf]

Am 24.03.2020 um 08:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> When sending iotests to upstream or do patch porting from one branch
> to another we very often have to resolve conflicts in group file, as
> many absolutely independent features are intersecting by this file.
> These conflicts are simple, but imagine how much time we all have
> already spent on resolving them? Let's finally get rid of group file.
> 
> This patch transposes group info: instead of collecting it in one file,
> let each test define its groups by itself.
> 
> Several steps are done to achive it:
> 
> 1. Define groups in test files automatically:
> 
> grep '^[0-9]\{3\} ' group | while read line; do
> file=$(awk '{print $1}' <<< "$line");
> groups=$(sed -e 's/^... //' <<< "$line");
> awk "NR==2{print \"# group: $groups\"}1" $file > tmp;
> cat tmp > $file;
> done
> 
> 2. Copy groups documentation into docs/devel/testing.rst, which already
>has a section about iotests.
> 
> 3. Modify check script to work without group file.
> 
>Here is a logic change: before, even if test do not belong to any
>group (only iotest 142 currently) it should be defined in group
>file. Now, test is not forced to define any group. Instead check
>considers all files with names matching [0-9][0-9][0-9] as tests.

This has both a positive and a negative effect: Now you don't have to
modify another file when you add a new test, but it will be picked up
automatically. However, if you want to disable a test, you could
previously just remove it from groups (or comment it out), and now you
have to delete the test instead.

Downstream, I think we still disable a few tests because we're compiling
out features that are required for some tests to pass, and deleting the
test cases completely would probably move conflicts just to a different
place.

So I think we need a method to distuingish an enabled test that is in no
group from a disabled test.

>check script is also refactored to make it simple to do next cool
>thing about iotests: allow meaningful names for test-case files.

This one would also require us to be able to distinguish test case files
from arbitrary other files.

> -#
> -# test-group association ... one line per test
> -#
> -001 rw auto quick
> -002 rw auto quick
> -003 rw auto
> -004 rw auto quick
> -005 img auto quick
> -# 006 was removed, do not reuse

We lose these comments without a replacement. I wonder whether it's
important or if we can think of another way to make sure that numbers
aren't reused. (I'm not completely sure any more why we decided that we
don't want to reuse numbers. Maybe because of backports?)

Kevin