Re: [Qemu-block] [PATCH v3 2/8] iotests: Prefer null-co over null-aio

[Thomas Huth]

On 8/19/19 10:18 PM, Max Reitz wrote:
> We use null-co basically everywhere in the iotests.  Unless we want to
> test null-aio specifically, we should use it instead (for consistency).
> 
> Signed-off-by: Max Reitz 
> Reviewed-by: John Snow 
> ---
>  tests/qemu-iotests/093 | 7 +++
>  tests/qemu-iotests/245 | 2 +-
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
> index 3c4f5173ce..50c1e7f2ec 100755
> --- a/tests/qemu-iotests/093
> +++ b/tests/qemu-iotests/093
> @@ -267,13 +267,12 @@ class ThrottleTestCoroutine(ThrottleTestCase):
>  test_img = "null-co://"
>  
>  class ThrottleTestGroupNames(iotests.QMPTestCase):
> -test_img = "null-aio://"
>  max_drives = 3
>  
>  def setUp(self):
>  self.vm = iotests.VM()
>  for i in range(0, self.max_drives):
> -self.vm.add_drive(self.test_img,
> +self.vm.add_drive("null-co://",
>
> "throttling.iops-total=100,file.read-zeroes=on")
>  self.vm.launch()
>  
> @@ -376,10 +375,10 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
>  
>  def test_removable_media(self):
>  # Add a couple of dummy nodes named cd0 and cd1
> -result = self.vm.qmp("blockdev-add", driver="null-aio",
> +result = self.vm.qmp("blockdev-add", driver="null-co",
>   read_zeroes=True, node_name="cd0")
>  self.assert_qmp(result, 'return', {})
> -result = self.vm.qmp("blockdev-add", driver="null-aio",
> +result = self.vm.qmp("blockdev-add", driver="null-co",
>   read_zeroes=True, node_name="cd1")
>  self.assert_qmp(result, 'return', {})
>  
> diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
> index bc1ceb9792..ae169778b0 100644
> --- a/tests/qemu-iotests/245
> +++ b/tests/qemu-iotests/245
> @@ -598,7 +598,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
>  ##
>  ## null ##
>  ##
> -opts = {'driver': 'null-aio', 'node-name': 'root', 'size': 1024}
> +opts = {'driver': 'null-co', 'node-name': 'root', 'size': 1024}
>  
>  result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
>  self.assert_qmp(result, 'return', {})
> 

Reviewed-by: Thomas Huth 

Re: [Qemu-block] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Thomas Huth]

On 8/19/19 10:18 PM, Max Reitz wrote:
> null-aio may not be whitelisted.  Skip all test cases that require it.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/093 | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
> index 50c1e7f2ec..f03fa24a07 100755
> --- a/tests/qemu-iotests/093
> +++ b/tests/qemu-iotests/093
> @@ -24,7 +24,7 @@ import iotests
>  nsec_per_sec = 10
>  
>  class ThrottleTestCase(iotests.QMPTestCase):
> -test_img = "null-aio://"
> +test_driver = "null-aio"
>  max_drives = 3
>  
>  def blockstats(self, device):
> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>  return stat['rd_bytes'], stat['rd_operations'], 
> stat['wr_bytes'], stat['wr_operations']
>  raise Exception("Device not found for blockstats: %s" % device)
>  
> +def required_drivers(self):
> +return [self.test_driver]
> +
> +@iotests.skip_if_unsupported(required_drivers)
>  def setUp(self):
>  self.vm = iotests.VM()
>  for i in range(0, self.max_drives):
> -self.vm.add_drive(self.test_img, "file.read-zeroes=on")
> +self.vm.add_drive(self.test_driver + "://", 
> "file.read-zeroes=on")
>  self.vm.launch()
>  
>  def tearDown(self):
> @@ -264,7 +268,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
>  self.assertEqual(self.blockstats('drive1')[0], 4096)
>  
>  class ThrottleTestCoroutine(ThrottleTestCase):
> -test_img = "null-co://"
> +test_driver = "null-co"
>  
>  class ThrottleTestGroupNames(iotests.QMPTestCase):
>  max_drives = 3
> @@ -425,4 +429,6 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
>  
>  
>  if __name__ == '__main__':
> +if 'null-co' not in iotests.supported_formats():
> +iotests.notrun('null-co driver support missing')
>  iotests.main(supported_fmts=["raw"])

Maybe also mention null-co in the patch description?

 Thomas

Re: [Qemu-block] [PATCH v3 1/8] iotests: Add -display none to the qemu options

[Thomas Huth]

On 8/19/19 10:18 PM, Max Reitz wrote:
> Without this argument, qemu will print an angry message about not being
> able to connect to a display server if $DISPLAY is not set.  For me,
> that breaks iotests.supported_formats() because it thus only sees
> ["Could", "not", "connect"] as the supported formats.
> 
> Signed-off-by: Max Reitz 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  tests/qemu-iotests/check | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index c24874ff4a..a58232eefb 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -581,13 +581,13 @@ export QEMU_PROG="$(type -p "$QEMU_PROG")"
>  
>  case "$QEMU_PROG" in
>  *qemu-system-arm|*qemu-system-aarch64)
> -export QEMU_OPTIONS="-nodefaults -machine virt,accel=qtest"
> +export QEMU_OPTIONS="-nodefaults -display none -machine 
> virt,accel=qtest"
>  ;;
>  *qemu-system-tricore)
> -export QEMU_OPTIONS="-nodefaults -machine 
> tricore_testboard,accel=qtest"
> +export QEMU_OPTIONS="-nodefaults -display none -machine 
> tricore_testboard,accel=qtest"
>  ;;
>  *)
> -export QEMU_OPTIONS="-nodefaults -machine accel=qtest"
> +export QEMU_OPTIONS="-nodefaults -display none -machine accel=qtest"
>  ;;
>  esac

Reviewed-by: Thomas Huth 

Re: [Qemu-block] [Qemu-devel] [qemu-s390x] [PATCH v7 33/42] exec: Replace device_endian with MemOp

[Edgar E. Iglesias]

On Mon, 19 Aug. 2019, 23:01 Richard Henderson, 
wrote:

> On 8/19/19 11:29 AM, Paolo Bonzini wrote:
> > On 19/08/19 20:28, Paolo Bonzini wrote:
> >> On 16/08/19 12:12, Thomas Huth wrote:
> >>> This patch is *huge*, more than 800kB. It keeps being stuck in the the
> >>> filter of the qemu-s390x list each time you send it. Please:
> >>>
> >>> 1) Try to break it up in more digestible pieces, e.g. change only one
> >>> subsystem at a time (this is also better reviewable by people who are
> >>> interested in one area)
> >>
> >> This is not really possible, since the patch is basically a
> >> search-and-replace.  You could perhaps use some magic
> >> ("DEVICE_MEMOP_ENDIAN" or something like that) to allow a split, but it
> >> would introduce more complication than anything else.
> >
> > I'm stupid, at this point of the series it _would_ be possible to split
> > the patch by subsystem.  Still not sure it would be actually an
> advantage.
>
> It might be easier to review if we split by symbol, one rename per patch
> over
> the entire code base.
>
>
> r~
>

Or if we review your script (I assume this wasn't a manual change). I'm not
sure it's realistic to have review on the entire patch or patches.

Best regards,
Edgar

>

Re: [Qemu-block] [Qemu-devel] [PATCH] block/io.c: fix for the allocation failure

[Eric Blake]

On 8/19/19 3:53 PM, Denis V. Lunev wrote:

 Or even better, fix the call site of fallocate() to skip attempting an
 unaligned fallocate(), and just directly return ENOTSUP, rather than
 trying to diagnose EINVAL after the fact.

>>> No way. Single ENOTSUP will turn off fallocate() support on caller side
>>> while
>>> aligned (99.99% of calls) works normally.
>> I didn't mean skip fallocate() unconditionally, only when unaligned:
>>
>> if (request not aligned enough)
>>return -ENOTSUP;
>> fallocate() ...
>>
>> so that the 99.99% requests that ARE aligned get to use fallocate()
>> normally.
>>
> static int handle_aiocb_write_zeroes(void *opaque)
> {
> ...
> #ifdef CONFIG_FALLOCATE_ZERO_RANGE
>     if (s->has_write_zeroes) {
>     int ret = do_fallocate(s->fd, FALLOC_FL_ZERO_RANGE,
>    aiocb->aio_offset, aiocb->aio_nbytes);
>     if (ret == 0 || ret != -ENOTSUP) {
>     return ret;
>     }
>     s->has_write_zeroes = false;
>     }
> #endif
> 
> thus, right now, single ENOTSUP disables fallocate
> functionality completely setting s->has_write_zeroes
> to false and that is pretty much correct.
> 
> ENOTSUP is "static" error code which returns persistent
> ENOTSUP under any consequences.

Not always true. And the block layer doesn't expect it to be true. It is
perfectly fine for one invocation to return ENOTSUP ('I can't handle
this request, so fall back to pwrite for me) and the next to just work
('this one was aligned, so I handled it just fine).  It just means that
you have to be more careful with the logic: never set
s->has_write_zeroes=false if you skipped the fallocate, or if the
fallocate failed due to EINVAL rather than ENOTSUP (but still report
ENOTSUP to the block layer, to document that you want the EINVAL for
unaligned request to be turned into a fallback to pwrite).

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [qemu-s390x] [PATCH v7 33/42] exec: Replace device_endian with MemOp

[Richard Henderson]

On 8/19/19 11:29 AM, Paolo Bonzini wrote:
> On 19/08/19 20:28, Paolo Bonzini wrote:
>> On 16/08/19 12:12, Thomas Huth wrote:
>>> This patch is *huge*, more than 800kB. It keeps being stuck in the the
>>> filter of the qemu-s390x list each time you send it. Please:
>>>
>>> 1) Try to break it up in more digestible pieces, e.g. change only one
>>> subsystem at a time (this is also better reviewable by people who are
>>> interested in one area)
>>
>> This is not really possible, since the patch is basically a
>> search-and-replace.  You could perhaps use some magic
>> ("DEVICE_MEMOP_ENDIAN" or something like that) to allow a split, but it
>> would introduce more complication than anything else.
> 
> I'm stupid, at this point of the series it _would_ be possible to split
> the patch by subsystem.  Still not sure it would be actually an advantage.

It might be easier to review if we split by symbol, one rename per patch over
the entire code base.


r~

Re: [Qemu-block] [Qemu-devel] [PATCH] block/io.c: fix for the allocation failure

[Denis V. Lunev]

On 8/19/19 11:30 PM, Eric Blake wrote:
> On 8/19/19 2:46 PM, Denis V. Lunev wrote:
>> On 8/17/19 5:56 PM, Eric Blake wrote:
>>> On 8/17/19 9:49 AM, Eric Blake wrote:
>>>
> This change is a regression of sorts.  Now, you are unconditionally
> attempting the fallback for ALL failures (such as EIO) and for all
> drivers, even when that was not previously attempted and increases the
> traffic.  I think we should revert this patch and instead fix the
> fallocate() path to convert whatever ACTUAL errno you got from unaligned
> fallocate failure into ENOTSUP (that is, just the file-posix.c location
> that failed), while leaving all other errors as immediately fatal.
>>> Or even better, fix the call site of fallocate() to skip attempting an
>>> unaligned fallocate(), and just directly return ENOTSUP, rather than
>>> trying to diagnose EINVAL after the fact.
>>>
>> No way. Single ENOTSUP will turn off fallocate() support on caller side
>> while
>> aligned (99.99% of calls) works normally.
> I didn't mean skip fallocate() unconditionally, only when unaligned:
>
> if (request not aligned enough)
>return -ENOTSUP;
> fallocate() ...
>
> so that the 99.99% requests that ARE aligned get to use fallocate()
> normally.
>
static int handle_aiocb_write_zeroes(void *opaque)
{
...
#ifdef CONFIG_FALLOCATE_ZERO_RANGE
    if (s->has_write_zeroes) {
    int ret = do_fallocate(s->fd, FALLOC_FL_ZERO_RANGE,
   aiocb->aio_offset, aiocb->aio_nbytes);
    if (ret == 0 || ret != -ENOTSUP) {
    return ret;
    }
    s->has_write_zeroes = false;
    }
#endif

thus, right now, single ENOTSUP disables fallocate
functionality completely setting s->has_write_zeroes
to false and that is pretty much correct.

ENOTSUP is "static" error code which returns persistent
ENOTSUP under any consequences. Its handling usually
disables some functionality.

This is why original idea is proposed.

Den

Re: [Qemu-block] [PATCH] iotests: Add more "skip_if_unsupported" statements to the python tests

[Max Reitz]

On 19.08.19 21:13, Max Reitz wrote:
> On 19.08.19 11:21, Thomas Huth wrote:
>> The python code already contains a possibility to skip tests if the
>> corresponding driver is not available in the qemu binary - use it
>> in more spots to avoid that the tests are failing if the driver has
>> been disabled.
>>
>> Signed-off-by: Thomas Huth 
>> ---
>>  tests/qemu-iotests/030 |  3 +++
>>  tests/qemu-iotests/040 |  2 ++
>>  tests/qemu-iotests/041 | 14 +-
>>  tests/qemu-iotests/245 |  2 ++
>>  4 files changed, 20 insertions(+), 1 deletion(-)
> 
> [...]
> 
>> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
>> index 26bf1701eb..f45d20fbe0 100755
>> --- a/tests/qemu-iotests/041
>> +++ b/tests/qemu-iotests/041
>> @@ -817,6 +817,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
>>  image_len = 1 * 1024 * 1024 # MB
>>  IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ]
>>  
>> +@iotests.skip_if_unsupported(['quorum'])
>>  def setUp(self):
>>  self.vm = iotests.VM()
> 
> It’s clear that none of these tests can run if there is no quorum
> support, because setUp() creates a quorum node.  I think it would be
> nice if it would suffice to just skip everything automatically if
> setUp() is skipped and not have to bother about each of the test cases.
> 
> Coincidentally (:-)), I have a patch to do that, namely “iotests: Allow
> skipping test cases” in my “iotests: Selfish patches” series:
> 
> https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg01106.html
> 
> Yes, that means you cannot use an annotation because it needs @self to
> be able to skip the test.  Hm... But I think I can make that work by
> simply s/case_notrun/args[0].case_skip/ in skip_if_unsupported()?

(Sent a v3 now that does that.)

(Max)



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v3 8/8] iotests: Cache supported_formats()

[Max Reitz]

Signed-off-by: Max Reitz 
---
 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 8f315538e9..f6492b355f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -884,9 +884,17 @@ def qemu_pipe(*args):
 def supported_formats(read_only=False):
 '''Set 'read_only' to True to check ro-whitelist
Otherwise, rw-whitelist is checked'''
-format_message = qemu_pipe("-drive", "format=help")
-line = 1 if read_only else 0
-return format_message.splitlines()[line].split(":")[1].split()
+
+if not hasattr(supported_formats, "formats"):
+supported_formats.formats = {}
+
+if read_only not in supported_formats.formats:
+format_message = qemu_pipe("-drive", "format=help")
+line = 1 if read_only else 0
+supported_formats.formats[read_only] = \
+format_message.splitlines()[line].split(":")[1].split()
+
+return supported_formats.formats[read_only]
 
 def skip_if_unsupported(required_formats=[], read_only=False):
 '''Skip Test Decorator
-- 
2.21.0

Re: [Qemu-block] [PATCH v2 16/16] iotests: Test qcow2's snapshot table handling

[Eric Blake]

On 8/19/19 1:56 PM, Max Reitz wrote:
> Add a test how our qcow2 driver handles extra data in snapshot table
> entries, and how it repairs overly long snapshot tables.
> 
> Signed-off-by: Max Reitz 
> ---

> +++ b/tests/qemu-iotests/261.out
> @@ -0,0 +1,346 @@
> +QA output created by 261
> +
> +=== Create v2 template ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT.v2.orig', fmt=IMGFMT size=67108864
> +No errors were found on the image.
> +Snapshots in TEST_DIR/t.IMGFMT.v2.orig:
> +  [0]
> +ID: 1
> +Name: sn0
> +Extra data size: 0
> +  [1]
> +ID: 2
> +Name: sn1
> +Extra data size: 42
> +VM state size: 0
> +Disk size: 67108864
> +Unknown extra data: very important data

Hmm - possibly one more patch to write - but when checking snapshots for
accuracy, do we want to insist that the 32-bit truncated VM state size
is either 0 or matches the low 32-bits of the 64-bit VM state size
field?  Any mismatch between those fields (other than the 32-bit field
being left 0 because we knew to use the 64-bit field) might be a hint of
a possible corruption.  But there's no good way to correct it other than
wiping the 32-bit field to 0; and for a v2 image, any change we make to
the 32-bit field might actually make the snapshot unusable for an older
client that doesn't know how to use the 64-bit field.  So maybe we just
overlook that.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] block/io.c: fix for the allocation failure

[Eric Blake]

On 8/19/19 2:46 PM, Denis V. Lunev wrote:
> On 8/17/19 5:56 PM, Eric Blake wrote:
>> On 8/17/19 9:49 AM, Eric Blake wrote:
>>
 This change is a regression of sorts.  Now, you are unconditionally
 attempting the fallback for ALL failures (such as EIO) and for all
 drivers, even when that was not previously attempted and increases the
 traffic.  I think we should revert this patch and instead fix the
 fallocate() path to convert whatever ACTUAL errno you got from unaligned
 fallocate failure into ENOTSUP (that is, just the file-posix.c location
 that failed), while leaving all other errors as immediately fatal.
>> Or even better, fix the call site of fallocate() to skip attempting an
>> unaligned fallocate(), and just directly return ENOTSUP, rather than
>> trying to diagnose EINVAL after the fact.
>>
> No way. Single ENOTSUP will turn off fallocate() support on caller side
> while
> aligned (99.99% of calls) works normally.

I didn't mean skip fallocate() unconditionally, only when unaligned:

if (request not aligned enough)
   return -ENOTSUP;
fallocate() ...

so that the 99.99% requests that ARE aligned get to use fallocate()
normally.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v3 3/8] iotests: Allow skipping test cases

[Max Reitz]

case_notrun() does not actually skip the current test case.  It just
adds a "notrun" note and then returns to the caller, who manually has to
skip the test.  Generally, skipping a test case is as simple as
returning from the current function, but not always: For example, this
model does not allow skipping tests already in the setUp() function.

Thus, add a QMPTestCase.case_skip() function that invokes case_notrun()
and then self.skipTest().  To make this work, we need to filter the
information on how many test cases were skipped from the unittest
output.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 21 ++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 84438e837c..2f53baf633 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -802,6 +802,11 @@ class QMPTestCase(unittest.TestCase):
 return self.pause_wait(job_id)
 return result
 
+def case_skip(self, reason):
+'''Skip this test case'''
+case_notrun(reason)
+self.skipTest(reason)
+
 
 def notrun(reason):
 '''Skip this test suite'''
@@ -813,7 +818,10 @@ def notrun(reason):
 sys.exit(0)
 
 def case_notrun(reason):
-'''Skip this test case'''
+'''Mark this test case as not having been run, but do not actually
+skip it; that is left to the caller.  See QMPTestCase.case_skip()
+for a variant that actually skips the current test case.'''
+
 # Each test in qemu-iotests has a number ("seq")
 seq = os.path.basename(sys.argv[0])
 
@@ -904,8 +912,15 @@ def execute_unittest(output, verbosity, debug):
 unittest.main(testRunner=runner)
 finally:
 if not debug:
-sys.stderr.write(re.sub(r'Ran (\d+) tests? in [\d.]+s',
-r'Ran \1 tests', output.getvalue()))
+out = output.getvalue()
+out = re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 tests', out)
+
+# Hide skipped tests from the reference output
+out = re.sub(r'OK \(skipped=\d+\)', 'OK', out)
+out_first_line, out_rest = out.split('\n', 1)
+out = out_first_line.replace('s', '.') + '\n' + out_rest
+
+sys.stderr.write(out)
 
 def execute_test(test_function=None,
  supported_fmts=[], supported_oses=['linux'],
-- 
2.21.0

[Qemu-block] [PATCH v3 0/8] iotests: Selfish patches

[Max Reitz]

Hi,

Nothing has changed too much since the previous version, thus I’ll start
with a link to its cover letter:

https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg01102.html


What has changed is this:

v3:
- Patches 2 and 3: Resolved rebase conflicts

- Added patch 4.  It seems useful to support the new case-skipping
  method in @iotests.skip_if_unsupported so that annotation works even
  with setUp().

- Added patch 5.  This has been proposed by Kevin back in v1 (for a
  slightly different reason, though).
  Now that I added patch 4, it would be nice if I could make use of it
  for patches 6 and 7, but the problem is that both tests store the
  driver name in a class attribute.  Therefore, we need patch 5 that
  allows us to query that attribute’s value in skip_if_unsupported().

- Patches 6 and 7: Make use of the fact that the annotation works now
  (Also, I dropped supported_null_drivers, because there is no point in
  having it – especially with patch 8.)
  ((Also, resolved rebase conflicts.))

- Added patch 8.  It just itches me that we call qemu every time we
  inquire the list of supported formats, and this patch eases that itch.


git-backport-diff against v2:

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/8:[] [--] 'iotests: Add -display none to the qemu options'
002/8:[0004] [FC] 'iotests: Prefer null-co over null-aio'
003/8:[0003] [FC] 'iotests: Allow skipping test cases'
004/8:[down] 'iotests: Use case_skip() in skip_if_unsupported()'
005/8:[down] 'iotests: Let skip_if_unsupported() accept a method'
006/8:[0014] [FC] 'iotests: Test driver whitelisting in 093'
007/8:[0010] [FC] 'iotests: Test driver whitelisting in 136'
008/8:[down] 'iotests: Cache supported_formats()'


Max Reitz (8):
  iotests: Add -display none to the qemu options
  iotests: Prefer null-co over null-aio
  iotests: Allow skipping test cases
  iotests: Use case_skip() in skip_if_unsupported()
  iotests: Let skip_if_unsupported() accept a method
  iotests: Test driver whitelisting in 093
  iotests: Test driver whitelisting in 136
  iotests: Cache supported_formats()

 tests/qemu-iotests/093| 19 +--
 tests/qemu-iotests/136| 14 +++
 tests/qemu-iotests/245|  2 +-
 tests/qemu-iotests/check  |  6 ++---
 tests/qemu-iotests/iotests.py | 45 ---
 5 files changed, 62 insertions(+), 24 deletions(-)

-- 
2.21.0

[Qemu-block] [PATCH v3 7/8] iotests: Test driver whitelisting in 136

[Max Reitz]

null-aio may not be whitelisted.  Skip all test cases that require it.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/136 | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
index a46a7b7630..012ea111ac 100755
--- a/tests/qemu-iotests/136
+++ b/tests/qemu-iotests/136
@@ -30,7 +30,7 @@ bad_offset = bad_sector * 512
 blkdebug_file = os.path.join(iotests.test_dir, 'blkdebug.conf')
 
 class BlockDeviceStatsTestCase(iotests.QMPTestCase):
-test_img = "null-aio://"
+test_driver = "null-aio"
 total_rd_bytes = 0
 total_rd_ops = 0
 total_wr_bytes = 0
@@ -67,6 +67,10 @@ sector = "%d"
 ''' % (bad_sector, bad_sector))
 file.close()
 
+def required_drivers(self):
+return [self.test_driver]
+
+@iotests.skip_if_unsupported(required_drivers)
 def setUp(self):
 drive_args = []
 drive_args.append("stats-intervals.0=%d" % interval_length)
@@ -76,8 +80,8 @@ sector = "%d"
   (self.account_failed and "on" or "off"))
 drive_args.append("file.image.read-zeroes=on")
 self.create_blkdebug_file()
-self.vm = iotests.VM().add_drive('blkdebug:%s:%s' %
- (blkdebug_file, self.test_img),
+self.vm = iotests.VM().add_drive('blkdebug:%s:%s://' %
+ (blkdebug_file, self.test_driver),
  ','.join(drive_args))
 self.vm.launch()
 # Set an initial value for the clock
@@ -337,7 +341,9 @@ class 
BlockDeviceStatsTestAccountBoth(BlockDeviceStatsTestCase):
 account_failed = True
 
 class BlockDeviceStatsTestCoroutine(BlockDeviceStatsTestCase):
-test_img = "null-co://"
+test_driver = "null-co"
 
 if __name__ == '__main__':
+if 'null-co' not in iotests.supported_formats():
+iotests.notrun('null-co driver support missing')
 iotests.main(supported_fmts=["raw"])
-- 
2.21.0

[Qemu-block] [PATCH v3 5/8] iotests: Let skip_if_unsupported() accept a method

[Max Reitz]

This lets tests use skip_if_unsupported() with a potentially variable
list of required formats.

Suggested-by: Kevin Wolf 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 726f904f50..8f315538e9 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -893,8 +893,12 @@ def skip_if_unsupported(required_formats=[], 
read_only=False):
Runs the test if all the required formats are whitelisted'''
 def skip_test_decorator(func):
 def func_wrapper(*args, **kwargs):
-usf_list = list(set(required_formats) -
-set(supported_formats(read_only)))
+if callable(required_formats):
+fmts = required_formats(args[0])
+else:
+fmts = required_formats
+
+usf_list = list(set(fmts) - set(supported_formats(read_only)))
 if usf_list:
 args[0].case_skip('{}: formats {} are not whitelisted'.format(
 args[0], usf_list))
-- 
2.21.0

[Qemu-block] [PATCH v3 4/8] iotests: Use case_skip() in skip_if_unsupported()

[Max Reitz]

skip_if_unsupported() should use the stronger variant case_skip(),
because this allows it to be used even with setUp() (in a meaningful
way).

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 2f53baf633..726f904f50 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -896,7 +896,7 @@ def skip_if_unsupported(required_formats=[], 
read_only=False):
 usf_list = list(set(required_formats) -
 set(supported_formats(read_only)))
 if usf_list:
-case_notrun('{}: formats {} are not whitelisted'.format(
+args[0].case_skip('{}: formats {} are not whitelisted'.format(
 args[0], usf_list))
 else:
 return func(*args, **kwargs)
-- 
2.21.0

[Qemu-block] [PATCH v3 1/8] iotests: Add -display none to the qemu options

[Max Reitz]

Without this argument, qemu will print an angry message about not being
able to connect to a display server if $DISPLAY is not set.  For me,
that breaks iotests.supported_formats() because it thus only sees
["Could", "not", "connect"] as the supported formats.

Signed-off-by: Max Reitz 
Reviewed-by: Philippe Mathieu-Daudé 
---
 tests/qemu-iotests/check | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index c24874ff4a..a58232eefb 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -581,13 +581,13 @@ export QEMU_PROG="$(type -p "$QEMU_PROG")"
 
 case "$QEMU_PROG" in
 *qemu-system-arm|*qemu-system-aarch64)
-export QEMU_OPTIONS="-nodefaults -machine virt,accel=qtest"
+export QEMU_OPTIONS="-nodefaults -display none -machine 
virt,accel=qtest"
 ;;
 *qemu-system-tricore)
-export QEMU_OPTIONS="-nodefaults -machine 
tricore_testboard,accel=qtest"
+export QEMU_OPTIONS="-nodefaults -display none -machine 
tricore_testboard,accel=qtest"
 ;;
 *)
-export QEMU_OPTIONS="-nodefaults -machine accel=qtest"
+export QEMU_OPTIONS="-nodefaults -display none -machine accel=qtest"
 ;;
 esac
 
-- 
2.21.0

Re: [Qemu-block] [PATCH v3 2/8] iotests: Prefer null-co over null-aio

[Max Reitz]

On 19.08.19 22:18, Max Reitz wrote:
> We use null-co basically everywhere in the iotests.  Unless we want to
> test null-aio specifically, we should use it instead (for consistency).
> 
> Signed-off-by: Max Reitz 
> Reviewed-by: John Snow 

Hm, sorry, I just noticed that I probably should have dropped this R-b. :-/

(I mean, apart from the rebase conflict, nothing has changed, but still.)

Max

> ---
>  tests/qemu-iotests/093 | 7 +++
>  tests/qemu-iotests/245 | 2 +-
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
> index 3c4f5173ce..50c1e7f2ec 100755
> --- a/tests/qemu-iotests/093
> +++ b/tests/qemu-iotests/093
> @@ -267,13 +267,12 @@ class ThrottleTestCoroutine(ThrottleTestCase):
>  test_img = "null-co://"
>  
>  class ThrottleTestGroupNames(iotests.QMPTestCase):
> -test_img = "null-aio://"
>  max_drives = 3
>  
>  def setUp(self):
>  self.vm = iotests.VM()
>  for i in range(0, self.max_drives):
> -self.vm.add_drive(self.test_img,
> +self.vm.add_drive("null-co://",
>
> "throttling.iops-total=100,file.read-zeroes=on")
>  self.vm.launch()
>  
> @@ -376,10 +375,10 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
>  
>  def test_removable_media(self):
>  # Add a couple of dummy nodes named cd0 and cd1
> -result = self.vm.qmp("blockdev-add", driver="null-aio",
> +result = self.vm.qmp("blockdev-add", driver="null-co",
>   read_zeroes=True, node_name="cd0")
>  self.assert_qmp(result, 'return', {})
> -result = self.vm.qmp("blockdev-add", driver="null-aio",
> +result = self.vm.qmp("blockdev-add", driver="null-co",
>   read_zeroes=True, node_name="cd1")
>  self.assert_qmp(result, 'return', {})
>  
> diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
> index bc1ceb9792..ae169778b0 100644
> --- a/tests/qemu-iotests/245
> +++ b/tests/qemu-iotests/245
> @@ -598,7 +598,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
>  ##
>  ## null ##
>  ##
> -opts = {'driver': 'null-aio', 'node-name': 'root', 'size': 1024}
> +opts = {'driver': 'null-co', 'node-name': 'root', 'size': 1024}
>  
>  result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
>  self.assert_qmp(result, 'return', {})
> 




signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Max Reitz]

null-aio may not be whitelisted.  Skip all test cases that require it.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/093 | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index 50c1e7f2ec..f03fa24a07 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -24,7 +24,7 @@ import iotests
 nsec_per_sec = 10
 
 class ThrottleTestCase(iotests.QMPTestCase):
-test_img = "null-aio://"
+test_driver = "null-aio"
 max_drives = 3
 
 def blockstats(self, device):
@@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
 return stat['rd_bytes'], stat['rd_operations'], 
stat['wr_bytes'], stat['wr_operations']
 raise Exception("Device not found for blockstats: %s" % device)
 
+def required_drivers(self):
+return [self.test_driver]
+
+@iotests.skip_if_unsupported(required_drivers)
 def setUp(self):
 self.vm = iotests.VM()
 for i in range(0, self.max_drives):
-self.vm.add_drive(self.test_img, "file.read-zeroes=on")
+self.vm.add_drive(self.test_driver + "://", "file.read-zeroes=on")
 self.vm.launch()
 
 def tearDown(self):
@@ -264,7 +268,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
 self.assertEqual(self.blockstats('drive1')[0], 4096)
 
 class ThrottleTestCoroutine(ThrottleTestCase):
-test_img = "null-co://"
+test_driver = "null-co"
 
 class ThrottleTestGroupNames(iotests.QMPTestCase):
 max_drives = 3
@@ -425,4 +429,6 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
 
 
 if __name__ == '__main__':
+if 'null-co' not in iotests.supported_formats():
+iotests.notrun('null-co driver support missing')
 iotests.main(supported_fmts=["raw"])
-- 
2.21.0

[Qemu-block] [PATCH v3 2/8] iotests: Prefer null-co over null-aio

[Max Reitz]

We use null-co basically everywhere in the iotests.  Unless we want to
test null-aio specifically, we should use it instead (for consistency).

Signed-off-by: Max Reitz 
Reviewed-by: John Snow 
---
 tests/qemu-iotests/093 | 7 +++
 tests/qemu-iotests/245 | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index 3c4f5173ce..50c1e7f2ec 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -267,13 +267,12 @@ class ThrottleTestCoroutine(ThrottleTestCase):
 test_img = "null-co://"
 
 class ThrottleTestGroupNames(iotests.QMPTestCase):
-test_img = "null-aio://"
 max_drives = 3
 
 def setUp(self):
 self.vm = iotests.VM()
 for i in range(0, self.max_drives):
-self.vm.add_drive(self.test_img,
+self.vm.add_drive("null-co://",
   "throttling.iops-total=100,file.read-zeroes=on")
 self.vm.launch()
 
@@ -376,10 +375,10 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
 
 def test_removable_media(self):
 # Add a couple of dummy nodes named cd0 and cd1
-result = self.vm.qmp("blockdev-add", driver="null-aio",
+result = self.vm.qmp("blockdev-add", driver="null-co",
  read_zeroes=True, node_name="cd0")
 self.assert_qmp(result, 'return', {})
-result = self.vm.qmp("blockdev-add", driver="null-aio",
+result = self.vm.qmp("blockdev-add", driver="null-co",
  read_zeroes=True, node_name="cd1")
 self.assert_qmp(result, 'return', {})
 
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index bc1ceb9792..ae169778b0 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -598,7 +598,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
 ##
 ## null ##
 ##
-opts = {'driver': 'null-aio', 'node-name': 'root', 'size': 1024}
+opts = {'driver': 'null-co', 'node-name': 'root', 'size': 1024}
 
 result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
 self.assert_qmp(result, 'return', {})
-- 
2.21.0

Re: [Qemu-block] [PATCH v2 14/16] qcow2: Fix v3 snapshot table entry compliancy

[Eric Blake]

On 8/19/19 1:56 PM, Max Reitz wrote:
> qcow2 v3 images require every snapshot table entry to have at least 16
> bytes of extra data.  If they do not, let qemu-img check -r all fix it.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/qcow2-snapshot.c | 18 ++
>  1 file changed, 18 insertions(+)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] block/io.c: fix for the allocation failure

[Denis V. Lunev]

On 8/17/19 5:56 PM, Eric Blake wrote:
> On 8/17/19 9:49 AM, Eric Blake wrote:
>
>>> This change is a regression of sorts.  Now, you are unconditionally
>>> attempting the fallback for ALL failures (such as EIO) and for all
>>> drivers, even when that was not previously attempted and increases the
>>> traffic.  I think we should revert this patch and instead fix the
>>> fallocate() path to convert whatever ACTUAL errno you got from unaligned
>>> fallocate failure into ENOTSUP (that is, just the file-posix.c location
>>> that failed), while leaving all other errors as immediately fatal.
> Or even better, fix the call site of fallocate() to skip attempting an
> unaligned fallocate(), and just directly return ENOTSUP, rather than
> trying to diagnose EINVAL after the fact.
>
No way. Single ENOTSUP will turn off fallocate() support on caller side
while
aligned (99.99% of calls) works normally.

Den

Re: [Qemu-block] [PATCH v2 13/16] qcow2: Repair snapshot table with too many entries

[Eric Blake]

On 8/19/19 1:55 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---

Short on the reasoning why this isn't a problem in practice.  (Again,
because we only do it via opt-in qemu-img -r; you can already learn if
qemu-img will have problem with your file created externally without
destroying the image, and elect to not have qemu-img clean it if you
don't like the algorithm qemu-img will use).

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 12/16] qcow2: Fix overly long snapshot tables

[Eric Blake]

On 8/19/19 1:55 PM, Max Reitz wrote:
> We currently refuse to open qcow2 images with overly long snapshot
> tables.  This patch makes qemu-img check -r all drop all offending
> entries past what we deem acceptable.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/qcow2-snapshot.c | 88 +-
>  1 file changed, 78 insertions(+), 10 deletions(-)

I know I was reluctant in v1, but you also managed to convince me that
it really takes a LOT of effort to get a table with that many entries.
And a user has to opt-in to 'qemu-img -r' (it may discard a snapshot
they value, but that beats not being able to use the image under qemu at
all, and we don't erase it for plain 'qemu-img check').  So I'm okay
with this going in.  Maybe the commit message can state this sort of
reasoning.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 11/16] qcow2: Keep track of the snapshot table length

[Eric Blake]

On 8/19/19 1:55 PM, Max Reitz wrote:
> When repairing the snapshot table, we truncate entries that have too
> much extra data.  This frees up space that we do not have to count
> towards the snapshot table size.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/qcow2-snapshot.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 10/16] qcow2: Fix broken snapshot table entries

[Eric Blake]

On 8/19/19 1:55 PM, Max Reitz wrote:
> The only case where we currently reject snapshot table entries is when
> they have too much extra data.  Fix them with qemu-img check -r all by
> counting it as a corruption, reducing their extra_data_size, and then
> letting qcow2_check_fix_snapshot_table() do the rest.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/qcow2-snapshot.c | 67 +++---
>  1 file changed, 56 insertions(+), 11 deletions(-)
> 

> @@ -64,6 +80,8 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
>  s->snapshots = g_new0(QCowSnapshot, s->nb_snapshots);
>  
>  for(i = 0; i < s->nb_snapshots; i++) {
> +bool truncate_unknown_extra_data = false;

Worth adding space after 'for' while in the vicinity?

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data

[Eric Blake]

On 8/19/19 1:55 PM, Max Reitz wrote:
> The qcow2 specification says to ignore unknown extra data fields in
> snapshot table entries.  Currently, we discard it whenever we update the
> image, which is a bit different from "ignore".
> 
> This patch makes the qcow2 driver keep all unknown extra data fields
> when updating an image's snapshot table.
> 

> @@ -80,31 +80,53 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error 
> **errp)
>  sn->date_sec = be32_to_cpu(h.date_sec);
>  sn->date_nsec = be32_to_cpu(h.date_nsec);
>  sn->vm_clock_nsec = be64_to_cpu(h.vm_clock_nsec);
> -extra_data_size = be32_to_cpu(h.extra_data_size);
> +sn->extra_data_size = be32_to_cpu(h.extra_data_size);
>  
>  id_str_size = be16_to_cpu(h.id_str_size);
>  name_size = be16_to_cpu(h.name_size);
>  
> -/* Read extra data */
> +if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) {
> +ret = -EFBIG;
> +error_setg(errp, "Too much extra metadata in snapshot table "
> +   "entry %i", i);
> +goto fail;

We fail if extra_data_size is > 1024...


> +if (sn->extra_data_size > sizeof(extra)) {
> +/* Store unknown extra data */
> +size_t unknown_extra_data_size =
> +sn->extra_data_size - sizeof(extra);
> +

But read at most 1008 bytes into sn->unknown_extra_data.

> @@ -234,6 +257,22 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>  }
>  offset += sizeof(extra);
>  
> +if (sn->extra_data_size > sizeof(extra)) {
> +size_t unknown_extra_data_size =
> +sn->extra_data_size - sizeof(extra);
> +
> +/* qcow2_read_snapshots() ensures no unbounded allocation */
> +assert(unknown_extra_data_size <= BDRV_REQUEST_MAX_BYTES);

So this assertion is quite loose in what it permits; tighter would be

assert(unknown_extra_data_size <= QCOW_MAX_SNAPSHOT_EXTRA_DATA -
sizeof(extra))

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 07/16] qcow2: Write v3-compliant snapshot list on upgrade

[Eric Blake]

On 8/19/19 1:55 PM, Max Reitz wrote:
> qcow2 v3 requires every snapshot table entry to have two extra data
> fields: The 64-bit VM state size, and the virtual disk size.  Both are
> optional for v2 images, so they may not be present.
> 
> qcow2_upgrade() therefore should update the snapshot table to ensure all
> entries have these extra data fields.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1727347
> Reported-by: Eric Blake 
> Signed-off-by: Max Reitz 
> ---
>  block/qcow2.c | 32 ++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 

> @@ -4768,7 +4770,33 @@ static int qcow2_upgrade(BlockDriverState *bs, int 
> target_version,
>  /* There are no other versions (yet) that you can upgrade to */
>  assert(target_version == 3);
>  
> -status_cb(bs, 0, 1, cb_opaque);
> +status_cb(bs, 0, 2, cb_opaque);
> +
> +/*
> + * In v2, snapshots do not need to have extra data.  v3 requires
> + * the 64-bit VM state size and the virtual disk size to be
> + * present.
> + * qcow2_write_snapshots() will always write the list in the
> + * v3-compliant format.
> + */
> +need_snapshot_update = false;
> +for (i = 0; i < s->nb_snapshots; i++) {
> +if (s->snapshots[i].extra_data_size <
> +sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
> +sizeof_field(QCowSnapshotExtraData, disk_size))

sizeof(extra) would be more concise than two sizeof_field() added
together, but might cause problems if we later expand the size of extra
for other reasons, but don't revisit this code.  So I actually like what
you did here.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data

[Eric Blake]

On 8/19/19 1:55 PM, Max Reitz wrote:
> The qcow2 specification says to ignore unknown extra data fields in
> snapshot table entries.  Currently, we discard it whenever we update the
> image, which is a bit different from "ignore".
> 
> This patch makes the qcow2 driver keep all unknown extra data fields
> when updating an image's snapshot table.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/qcow2.h  |  5 
>  block/qcow2-snapshot.c | 61 +++---
>  2 files changed, 56 insertions(+), 10 deletions(-)
> 

> @@ -162,7 +184,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>  sn = s->snapshots + i;
>  offset = ROUND_UP(offset, 8);
>  offset += sizeof(h);
> -offset += sizeof(extra);
> +offset += MAX(sizeof(extra), sn->extra_data_size);

Why would we ever have less than sizeof(extra) bytes to write on output,
since we always produce the fields on creation and synthesize the
missing fields of extra on read?  Can't you rewrite this as:

assert(sn->extra_data_size >= sizeof(extra));
offset += sn->extra_data_size;

Otherwise,

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] iotests: Add more "skip_if_unsupported" statements to the python tests

[Max Reitz]

On 19.08.19 11:21, Thomas Huth wrote:
> The python code already contains a possibility to skip tests if the
> corresponding driver is not available in the qemu binary - use it
> in more spots to avoid that the tests are failing if the driver has
> been disabled.
> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/qemu-iotests/030 |  3 +++
>  tests/qemu-iotests/040 |  2 ++
>  tests/qemu-iotests/041 | 14 +-
>  tests/qemu-iotests/245 |  2 ++
>  4 files changed, 20 insertions(+), 1 deletion(-)

[...]

> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index 26bf1701eb..f45d20fbe0 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041
> @@ -817,6 +817,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
>  image_len = 1 * 1024 * 1024 # MB
>  IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ]
>  
> +@iotests.skip_if_unsupported(['quorum'])
>  def setUp(self):
>  self.vm = iotests.VM()

It’s clear that none of these tests can run if there is no quorum
support, because setUp() creates a quorum node.  I think it would be
nice if it would suffice to just skip everything automatically if
setUp() is skipped and not have to bother about each of the test cases.

Coincidentally (:-)), I have a patch to do that, namely “iotests: Allow
skipping test cases” in my “iotests: Selfish patches” series:

https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg01106.html

Yes, that means you cannot use an annotation because it needs @self to
be able to skip the test.  Hm... But I think I can make that work by
simply s/case_notrun/args[0].case_skip/ in skip_if_unsupported()?

Max



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v2 15/16] iotests: Add peek_file* functions

[Max Reitz]

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

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 5502c3da2f..78decfd5d5 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -53,6 +53,26 @@ poke_file()
 printf "$3" | dd "of=$1" bs=1 "seek=$2" conv=notrunc &>/dev/null
 }
 
+# peek_file_le 'test.img' 512 2 => 65534
+peek_file_le()
+{
+# Wrap in echo $() to strip spaces
+echo $(od -j"$2" -N"$3" --endian=little -An -vtu"$3" "$1")
+}
+
+# peek_file_be 'test.img' 512 2 => 65279
+peek_file_be()
+{
+# Wrap in echo $() to strip spaces
+echo $(od -j"$2" -N"$3" --endian=big -An -vtu"$3" "$1")
+}
+
+# peek_file_raw 'test.img' 512 2 => '\xff\xfe'
+peek_file_raw()
+{
+dd if="$1" bs=1 skip="$2" count="$3" status=none
+}
+
 
 if ! . ./common.config
 then
-- 
2.21.0

Re: [Qemu-block] [PATCH v2 02/16] qcow2: Use endof()

[Eric Blake]

On 8/19/19 1:55 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  block/qcow2-snapshot.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)

What, the file got larger in spite of using a helper macro for ease of
readability?  Good thing that 'lines of code' is not the only metric of
goodness ;)

> 
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index d0e7fa9311..752883e5c3 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -92,11 +92,12 @@ int qcow2_read_snapshots(BlockDriverState *bs)
>  }
>  offset += extra_data_size;
>  
> -if (extra_data_size >= 8) {
> +if (extra_data_size >= endof(QCowSnapshotExtraData,
> + vm_state_size_large)) {
>  sn->vm_state_size = be64_to_cpu(extra.vm_state_size_large);
>  }
>  
> -if (extra_data_size >= 16) {
> +if (extra_data_size >= endof(QCowSnapshotExtraData, disk_size)) {

Yes, that is nicer.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 01/16] include: Move endof() up from hw/virtio/virtio.h

[Eric Blake]

On 8/19/19 1:55 PM, Max Reitz wrote:
> endof() is a useful macro, we can make use of it outside of virtio.
> 
> Signed-off-by: Max Reitz 
> ---
>  include/hw/virtio/virtio.h |  7 ---
>  include/qemu/compiler.h|  7 +++
>  hw/block/virtio-blk.c  |  4 ++--
>  hw/net/virtio-net.c| 10 +-
>  4 files changed, 14 insertions(+), 14 deletions(-)
> 

> +++ b/include/qemu/compiler.h
> @@ -60,6 +60,13 @@
>  
>  #define sizeof_field(type, field) sizeof(((type *)0)->field)
>  
> +/*
> + * Calculate the number of bytes up to and including the given 'field' of
> + * 'container'.
> + */
> +#define endof(container, field) \
> +(offsetof(container, field) + sizeof_field(container, field))

Bike-shedding: I might have done s/container/type/ as part of the
motion, to match the above definition of sizeof_field (and in C, we tend
to refer to 'type's, not 'container's).  But doesn't affect correctness
of the patch.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v2 13/16] qcow2: Repair snapshot table with too many entries

[Max Reitz]

Signed-off-by: Max Reitz 
---
 block/qcow2-snapshot.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 366d9f574c..dac8a778e4 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -444,6 +444,14 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,
 s->snapshots_offset = be64_to_cpu(snapshot_table_pointer.snapshots_offset);
 s->nb_snapshots = be32_to_cpu(snapshot_table_pointer.nb_snapshots);
 
+if (s->nb_snapshots > QCOW_MAX_SNAPSHOTS && (fix & BDRV_FIX_ERRORS)) {
+fprintf(stderr, "Discarding %u overhanging snapshots\n",
+s->nb_snapshots - QCOW_MAX_SNAPSHOTS);
+
+nb_clusters_reduced += s->nb_snapshots - QCOW_MAX_SNAPSHOTS;
+s->nb_snapshots = QCOW_MAX_SNAPSHOTS;
+}
+
 ret = qcow2_validate_table(bs, s->snapshots_offset, s->nb_snapshots,
sizeof(QCowSnapshotHeader),
sizeof(QCowSnapshotHeader) * QCOW_MAX_SNAPSHOTS,
@@ -452,6 +460,12 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,
 result->check_errors++;
 error_reportf_err(local_err, "ERROR ");
 
+if (s->nb_snapshots > QCOW_MAX_SNAPSHOTS) {
+fprintf(stderr, "You can force-remove all %u overhanging snapshots 
"
+"with qemu-img check -r all\n",
+s->nb_snapshots - QCOW_MAX_SNAPSHOTS);
+}
+
 /* We did not read the snapshot table, so invalidate this information 
*/
 s->snapshots_offset = 0;
 s->nb_snapshots = 0;
-- 
2.21.0

[Qemu-block] [PATCH v2 10/16] qcow2: Fix broken snapshot table entries

[Max Reitz]

The only case where we currently reject snapshot table entries is when
they have too much extra data.  Fix them with qemu-img check -r all by
counting it as a corruption, reducing their extra_data_size, and then
letting qcow2_check_fix_snapshot_table() do the rest.

Signed-off-by: Max Reitz 
---
 block/qcow2-snapshot.c | 67 +++---
 1 file changed, 56 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index b526a8f819..53dc1635ec 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -44,7 +44,23 @@ void qcow2_free_snapshots(BlockDriverState *bs)
 s->nb_snapshots = 0;
 }
 
-int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
+/*
+ * If @repair is true, try to repair a broken snapshot table instead
+ * of just returning an error:
+ *
+ * - If there were snapshots with too much extra metadata, increment
+ *   *extra_data_dropped for each.
+ *   This requires the caller to eventually rewrite the whole snapshot
+ *   table, which requires cluster allocation.  Therefore, this should
+ *   be done only after qcow2_check_refcounts() made sure the refcount
+ *   structures are valid.
+ *   (In the meantime, the image is still valid because
+ *   qcow2_check_refcounts() does not do anything with snapshots'
+ *   extra data.)
+ */
+static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
+   int *extra_data_dropped,
+   Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
 QCowSnapshotHeader h;
@@ -64,6 +80,8 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
 s->snapshots = g_new0(QCowSnapshot, s->nb_snapshots);
 
 for(i = 0; i < s->nb_snapshots; i++) {
+bool truncate_unknown_extra_data = false;
+
 /* Read statically sized part of the snapshot header */
 offset = ROUND_UP(offset, 8);
 ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
@@ -86,10 +104,21 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error 
**errp)
 name_size = be16_to_cpu(h.name_size);
 
 if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) {
-ret = -EFBIG;
-error_setg(errp, "Too much extra metadata in snapshot table "
-   "entry %i", i);
-goto fail;
+if (!repair) {
+ret = -EFBIG;
+error_setg(errp, "Too much extra metadata in snapshot table "
+   "entry %i", i);
+error_append_hint(errp, "You can force-remove this extra "
+  "metadata with qemu-img check -r all\n");
+goto fail;
+}
+
+fprintf(stderr, "Discarding too much extra metadata in snapshot "
+"table entry %i (%" PRIu32 " > %u)\n",
+i, sn->extra_data_size, QCOW_MAX_SNAPSHOT_EXTRA_DATA);
+
+(*extra_data_dropped)++;
+truncate_unknown_extra_data = true;
 }
 
 /* Read known extra data */
@@ -113,18 +142,26 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error 
**errp)
 }
 
 if (sn->extra_data_size > sizeof(extra)) {
-/* Store unknown extra data */
-size_t unknown_extra_data_size =
-sn->extra_data_size - sizeof(extra);
+uint64_t extra_data_end;
+size_t unknown_extra_data_size;
+
+extra_data_end = offset + sn->extra_data_size - sizeof(extra);
 
+if (truncate_unknown_extra_data) {
+sn->extra_data_size = QCOW_MAX_SNAPSHOT_EXTRA_DATA;
+}
+
+/* Store unknown extra data */
+unknown_extra_data_size = sn->extra_data_size - sizeof(extra);
 sn->unknown_extra_data = g_malloc(unknown_extra_data_size);
 ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data,
  unknown_extra_data_size);
 if (ret < 0) {
-error_setg_errno(errp, -ret, "Failed to read snapshot table");
+error_setg_errno(errp, -ret,
+ "Failed to read snapshot table");
 goto fail;
 }
-offset += unknown_extra_data_size;
+offset = extra_data_end;
 }
 
 /* Read snapshot ID */
@@ -163,6 +200,11 @@ fail:
 return ret;
 }
 
+int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
+{
+return qcow2_do_read_snapshots(bs, false, NULL, errp);
+}
+
 /* add at the end of the file a new list of snapshots */
 int qcow2_write_snapshots(BlockDriverState *bs)
 {
@@ -328,6 +370,7 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,
 {
 BDRVQcow2State *s = bs->opaque;
 Error *local_err = NULL;
+int extra_data_dropped = 0;
 int ret;
 struct {
 uint32_t nb_snapshots;
@@ -363,7 +406,8 @@ int coroutine_fn 
qcow2_check_read_

[Qemu-block] [PATCH v2 07/16] qcow2: Write v3-compliant snapshot list on upgrade

[Max Reitz]

qcow2 v3 requires every snapshot table entry to have two extra data
fields: The 64-bit VM state size, and the virtual disk size.  Both are
optional for v2 images, so they may not be present.

qcow2_upgrade() therefore should update the snapshot table to ensure all
entries have these extra data fields.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1727347
Reported-by: Eric Blake 
Signed-off-by: Max Reitz 
---
 block/qcow2.c | 32 ++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 190a3874c1..2219639e11 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4759,7 +4759,9 @@ static int qcow2_upgrade(BlockDriverState *bs, int 
target_version,
  Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
+bool need_snapshot_update;
 int current_version = s->qcow_version;
+int i;
 int ret;
 
 /* This is qcow2_upgrade(), not qcow2_downgrade() */
@@ -4768,7 +4770,33 @@ static int qcow2_upgrade(BlockDriverState *bs, int 
target_version,
 /* There are no other versions (yet) that you can upgrade to */
 assert(target_version == 3);
 
-status_cb(bs, 0, 1, cb_opaque);
+status_cb(bs, 0, 2, cb_opaque);
+
+/*
+ * In v2, snapshots do not need to have extra data.  v3 requires
+ * the 64-bit VM state size and the virtual disk size to be
+ * present.
+ * qcow2_write_snapshots() will always write the list in the
+ * v3-compliant format.
+ */
+need_snapshot_update = false;
+for (i = 0; i < s->nb_snapshots; i++) {
+if (s->snapshots[i].extra_data_size <
+sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
+sizeof_field(QCowSnapshotExtraData, disk_size))
+{
+need_snapshot_update = true;
+break;
+}
+}
+if (need_snapshot_update) {
+ret = qcow2_write_snapshots(bs);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to update the snapshot 
table");
+return ret;
+}
+}
+status_cb(bs, 1, 2, cb_opaque);
 
 s->qcow_version = target_version;
 ret = qcow2_update_header(bs);
@@ -4777,7 +4805,7 @@ static int qcow2_upgrade(BlockDriverState *bs, int 
target_version,
 error_setg_errno(errp, -ret, "Failed to update the image header");
 return ret;
 }
-status_cb(bs, 1, 1, cb_opaque);
+status_cb(bs, 2, 2, cb_opaque);
 
 return 0;
 }
-- 
2.21.0

[Qemu-block] [PATCH v2 16/16] iotests: Test qcow2's snapshot table handling

[Max Reitz]

Add a test how our qcow2 driver handles extra data in snapshot table
entries, and how it repairs overly long snapshot tables.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/261 | 523 +
 tests/qemu-iotests/261.out | 346 
 tests/qemu-iotests/group   |   1 +
 3 files changed, 870 insertions(+)
 create mode 100755 tests/qemu-iotests/261
 create mode 100644 tests/qemu-iotests/261.out

diff --git a/tests/qemu-iotests/261 b/tests/qemu-iotests/261
new file mode 100755
index 00..fb96bcfbe2
--- /dev/null
+++ b/tests/qemu-iotests/261
@@ -0,0 +1,523 @@
+#!/usr/bin/env bash
+#
+# Test case for qcow2's handling of extra data in snapshot table entries
+# (and more generally, how certain cases of broken snapshot tables are
+# handled)
+#
+# Copyright (C) 2019 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 .
+#
+
+# creator
+owner=mre...@redhat.com
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+rm -f "$TEST_IMG".v{2,3}.orig
+rm -f "$TEST_DIR"/sn{0,1,2}{,-pre,-extra,-post}
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+# (1) We create a v2 image that supports nothing but refcount_bits=16
+# (2) We do some refcount management on our own which expects
+# refcount_bits=16
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+
+# Parameters:
+#   $1: image filename
+#   $2: snapshot table entry offset in the image
+snapshot_table_entry_size()
+{
+id_len=$(peek_file_be "$1" $(($2 + 12)) 2)
+name_len=$(peek_file_be "$1" $(($2 + 14)) 2)
+extra_len=$(peek_file_be "$1" $(($2 + 36)) 4)
+
+full_len=$((40 + extra_len + id_len + name_len))
+echo $(((full_len + 7) / 8 * 8))
+}
+
+# Parameter:
+#   $1: image filename
+print_snapshot_table()
+{
+nb_entries=$(peek_file_be "$1" 60 4)
+offset=$(peek_file_be "$1" 64 8)
+
+echo "Snapshots in $1:" | _filter_testdir | _filter_imgfmt
+
+for ((i = 0; i < nb_entries; i++)); do
+id_len=$(peek_file_be "$1" $((offset + 12)) 2)
+name_len=$(peek_file_be "$1" $((offset + 14)) 2)
+extra_len=$(peek_file_be "$1" $((offset + 36)) 4)
+
+extra_ofs=$((offset + 40))
+id_ofs=$((extra_ofs + extra_len))
+name_ofs=$((id_ofs + id_len))
+
+echo "  [$i]"
+echo "ID: $(peek_file_raw "$1" $id_ofs $id_len)"
+echo "Name: $(peek_file_raw "$1" $name_ofs $name_len)"
+echo "Extra data size: $extra_len"
+if [ $extra_len -ge 8 ]; then
+echo "VM state size: $(peek_file_be "$1" $extra_ofs 8)"
+fi
+if [ $extra_len -ge 16 ]; then
+echo "Disk size: $(peek_file_be "$1" $((extra_ofs + 8)) 8)"
+fi
+if [ $extra_len -gt 16 ]; then
+echo 'Unknown extra data:' \
+"$(peek_file_raw "$1" $((extra_ofs + 16)) $((extra_len - 16)) \
+   | tr -d '\0')"
+fi
+
+offset=$((offset + $(snapshot_table_entry_size "$1" $offset)))
+done
+}
+
+# Mark clusters as allocated; works only in refblock 0 (i.e. before
+# cluster #32768).
+# Parameters:
+#   $1: Start offset of what to allocate
+#   $2: End offset (exclusive)
+refblock0_allocate()
+{
+reftable_ofs=$(peek_file_be "$TEST_IMG" 48 8)
+refblock_ofs=$(peek_file_be "$TEST_IMG" $reftable_ofs 8)
+
+cluster=$(($1 / 65536))
+ecluster=$((($2 + 65535) / 65536))
+
+while [ $cluster -lt $ecluster ]; do
+if [ $cluster -ge 32768 ]; then
+echo "*** Abort: Cluster $cluster exceeds refblock 0 ***"
+exit 1
+fi
+poke_file "$TEST_IMG" $((refblock_ofs + cluster * 2)) '\x00\x01'
+cluster=$((cluster + 1))
+done
+}
+
+
+echo
+echo '=== Create v2 template ==='
+echo
+
+# Create v2 image with a snapshot table with three entries:
+# [0]: No extra data (valid with v2, not valid with v3)
+# [1]: Has extra data unknown to qemu
+# [2]: Has the 64-bit VM state size, but not the disk size (again,
+#  valid with v2, not valid with v3)
+
+TEST_IMG="$TEST_IMG.v2.orig" IMGOPTS='compat=0.10' _make_test_img 64

[Qemu-block] [PATCH v2 14/16] qcow2: Fix v3 snapshot table entry compliancy

[Max Reitz]

qcow2 v3 images require every snapshot table entry to have at least 16
bytes of extra data.  If they do not, let qemu-img check -r all fix it.

Signed-off-by: Max Reitz 
---
 block/qcow2-snapshot.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index dac8a778e4..5ab64da1ec 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -516,6 +516,24 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,
 result->corruptions -= nb_clusters_reduced;
 }
 
+/*
+ * All of v3 images' snapshot table entries need to have at least
+ * 16 bytes of extra data.
+ */
+if (s->qcow_version >= 3) {
+int i;
+for (i = 0; i < s->nb_snapshots; i++) {
+if (s->snapshots[i].extra_data_size <
+sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
+sizeof_field(QCowSnapshotExtraData, disk_size))
+{
+result->corruptions++;
+fprintf(stderr, "%s snapshot table entry %i is incomplete\n",
+fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+}
+}
+}
+
 return 0;
 }
 
-- 
2.21.0

[Qemu-block] [PATCH v2 09/16] qcow2: Add qcow2_check_fix_snapshot_table()

[Max Reitz]

qcow2_check_read_snapshot_table() can perform consistency checks, but it
cannot fix everything.  Specifically, it cannot allocate new clusters,
because that should wait until the refcount structures are known to be
consistent (i.e., after qcow2_check_refcounts()).  Thus, it cannot call
qcow2_write_snapshots().

Do that in qcow2_check_fix_snapshot_table(), which is called after
qcow2_check_refcounts().

Currently, there is nothing that would set result->corruptions, so this
is a no-op.  A follow-up patch will change that.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2.h  |  3 +++
 block/qcow2-snapshot.c | 25 +
 block/qcow2.c  |  9 -
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 50c7eefb5b..6823c37e82 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -715,6 +715,9 @@ int qcow2_write_snapshots(BlockDriverState *bs);
 int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
  BdrvCheckResult *result,
  BdrvCheckMode fix);
+int coroutine_fn qcow2_check_fix_snapshot_table(BlockDriverState *bs,
+BdrvCheckResult *result,
+BdrvCheckMode fix);
 
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index d667bfd935..b526a8f819 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -380,6 +380,31 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,
 return 0;
 }
 
+int coroutine_fn qcow2_check_fix_snapshot_table(BlockDriverState *bs,
+BdrvCheckResult *result,
+BdrvCheckMode fix)
+{
+BDRVQcow2State *s = bs->opaque;
+int ret;
+
+if (result->corruptions && (fix & BDRV_FIX_ERRORS)) {
+qemu_co_mutex_unlock(&s->lock);
+ret = qcow2_write_snapshots(bs);
+qemu_co_mutex_lock(&s->lock);
+if (ret < 0) {
+result->check_errors++;
+fprintf(stderr, "ERROR failed to update snapshot table: %s\n",
+strerror(-ret));
+return ret;
+}
+
+result->corruptions_fixed += result->corruptions;
+result->corruptions = 0;
+}
+
+return 0;
+}
+
 static void find_new_snapshot_id(BlockDriverState *bs,
  char *id_str, int id_str_size)
 {
diff --git a/block/qcow2.c b/block/qcow2.c
index 56c764de0b..a32ff20458 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -595,13 +595,20 @@ static int coroutine_fn 
qcow2_co_check_locked(BlockDriverState *bs,
 memset(result, 0, sizeof(*result));
 
 ret = qcow2_check_read_snapshot_table(bs, &snapshot_res, fix);
-qcow2_add_check_result(result, &snapshot_res, false);
 if (ret < 0) {
+qcow2_add_check_result(result, &snapshot_res, false);
 return ret;
 }
 
 ret = qcow2_check_refcounts(bs, &refcount_res, fix);
 qcow2_add_check_result(result, &refcount_res, true);
+if (ret < 0) {
+qcow2_add_check_result(result, &snapshot_res, false);
+return ret;
+}
+
+ret = qcow2_check_fix_snapshot_table(bs, &snapshot_res, fix);
+qcow2_add_check_result(result, &snapshot_res, false);
 if (ret < 0) {
 return ret;
 }
-- 
2.21.0

[Qemu-block] [PATCH v2 12/16] qcow2: Fix overly long snapshot tables

[Max Reitz]

We currently refuse to open qcow2 images with overly long snapshot
tables.  This patch makes qemu-img check -r all drop all offending
entries past what we deem acceptable.

Signed-off-by: Max Reitz 
---
 block/qcow2-snapshot.c | 88 +-
 1 file changed, 78 insertions(+), 10 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 582eb3386a..366d9f574c 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -29,15 +29,24 @@
 #include "qemu/error-report.h"
 #include "qemu/cutils.h"
 
+static void qcow2_free_single_snapshot(BlockDriverState *bs, int i)
+{
+BDRVQcow2State *s = bs->opaque;
+
+assert(i >= 0 && i < s->nb_snapshots);
+g_free(s->snapshots[i].name);
+g_free(s->snapshots[i].id_str);
+g_free(s->snapshots[i].unknown_extra_data);
+memset(&s->snapshots[i], 0, sizeof(s->snapshots[i]));
+}
+
 void qcow2_free_snapshots(BlockDriverState *bs)
 {
 BDRVQcow2State *s = bs->opaque;
 int i;
 
 for(i = 0; i < s->nb_snapshots; i++) {
-g_free(s->snapshots[i].name);
-g_free(s->snapshots[i].id_str);
-g_free(s->snapshots[i].unknown_extra_data);
+qcow2_free_single_snapshot(bs, i);
 }
 g_free(s->snapshots);
 s->snapshots = NULL;
@@ -48,6 +57,14 @@ void qcow2_free_snapshots(BlockDriverState *bs)
  * If @repair is true, try to repair a broken snapshot table instead
  * of just returning an error:
  *
+ * - If the snapshot table was too long, set *nb_clusters_reduced to
+ *   the number of snapshots removed off the end.
+ *   The caller will update the on-disk nb_snapshots accordingly;
+ *   this leaks clusters, but is safe.
+ *   (The on-disk information must be updated before
+ *   qcow2_check_refcounts(), because that function relies on
+ *   s->nb_snapshots to reflect the on-disk value.)
+ *
  * - If there were snapshots with too much extra metadata, increment
  *   *extra_data_dropped for each.
  *   This requires the caller to eventually rewrite the whole snapshot
@@ -59,6 +76,7 @@ void qcow2_free_snapshots(BlockDriverState *bs)
  *   extra data.)
  */
 static int qcow2_do_read_snapshots(BlockDriverState *bs, bool repair,
+   int *nb_clusters_reduced,
int *extra_data_dropped,
Error **errp)
 {
@@ -67,7 +85,7 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool 
repair,
 QCowSnapshotExtraData extra;
 QCowSnapshot *sn;
 int i, id_str_size, name_size;
-int64_t offset;
+int64_t offset, pre_sn_offset;
 uint64_t table_length = 0;
 int ret;
 
@@ -83,6 +101,7 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, 
bool repair,
 for(i = 0; i < s->nb_snapshots; i++) {
 bool truncate_unknown_extra_data = false;
 
+pre_sn_offset = offset;
 table_length = ROUND_UP(table_length, 8);
 
 /* Read statically sized part of the snapshot header */
@@ -197,9 +216,31 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, 
bool repair,
 if (table_length > QCOW_MAX_SNAPSHOTS_SIZE ||
 offset - s->snapshots_offset > INT_MAX)
 {
-ret = -EFBIG;
-error_setg(errp, "Snapshot table is too big");
-goto fail;
+if (!repair) {
+ret = -EFBIG;
+error_setg(errp, "Snapshot table is too big");
+error_append_hint(errp, "You can force-remove all %u "
+  "overhanging snapshots with qemu-img check "
+  "-r all\n", s->nb_snapshots - i);
+goto fail;
+}
+
+fprintf(stderr, "Discarding %u overhanging snapshots (snapshot "
+"table is too big)\n", s->nb_snapshots - i);
+
+*nb_clusters_reduced += (s->nb_snapshots - i);
+
+/* Discard current snapshot also */
+qcow2_free_single_snapshot(bs, i);
+
+/*
+ * This leaks all the rest of the snapshot table and the
+ * snapshots' clusters, but we run in check -r all mode,
+ * so qcow2_check_refcounts() will take care of it.
+ */
+s->nb_snapshots = i;
+offset = pre_sn_offset;
+break;
 }
 }
 
@@ -214,7 +255,7 @@ fail:
 
 int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
 {
-return qcow2_do_read_snapshots(bs, false, NULL, errp);
+return qcow2_do_read_snapshots(bs, false, NULL, NULL, errp);
 }
 
 /* add at the end of the file a new list of snapshots */
@@ -382,6 +423,7 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,
 {
 BDRVQcow2State *s = bs->opaque;
 Error *local_err = NULL;
+int nb_clusters_reduced = 0;
 int extra_data_dropped = 0;
 int ret;
 struct {
@@ -419,7 +461,8 @@ int coroutine_fn 
qcow2_check_read_snapshot_table(BlockDriverState *bs,

[Qemu-block] [PATCH v2 08/16] qcow2: Separate qcow2_check_read_snapshot_table()

[Max Reitz]

Reading the snapshot table can fail.  That is a problem when we want to
repair the image.

Therefore, stop reading the snapshot table in qcow2_do_open() in check
mode.  Instead, add a new function qcow2_check_read_snapshot_table()
that reads the snapshot table at a later point.  In the future, we want
to handle errors here and fix them.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2.h  |  4 +++
 block/qcow2-snapshot.c | 58 
 block/qcow2.c  | 76 --
 3 files changed, 120 insertions(+), 18 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 77586d81b9..50c7eefb5b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -712,6 +712,10 @@ void qcow2_free_snapshots(BlockDriverState *bs);
 int qcow2_read_snapshots(BlockDriverState *bs, Error **errp);
 int qcow2_write_snapshots(BlockDriverState *bs);
 
+int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
+ BdrvCheckResult *result,
+ BdrvCheckMode fix);
+
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
unsigned table_size);
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index e3bf4c9776..d667bfd935 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -322,6 +322,64 @@ fail:
 return ret;
 }
 
+int coroutine_fn qcow2_check_read_snapshot_table(BlockDriverState *bs,
+ BdrvCheckResult *result,
+ BdrvCheckMode fix)
+{
+BDRVQcow2State *s = bs->opaque;
+Error *local_err = NULL;
+int ret;
+struct {
+uint32_t nb_snapshots;
+uint64_t snapshots_offset;
+} QEMU_PACKED snapshot_table_pointer;
+
+/* qcow2_do_open() discards this information in check mode */
+ret = bdrv_pread(bs->file, offsetof(QCowHeader, nb_snapshots),
+ &snapshot_table_pointer, sizeof(snapshot_table_pointer));
+if (ret < 0) {
+result->check_errors++;
+fprintf(stderr, "ERROR failed to read the snapshot table pointer from "
+"the image header: %s\n", strerror(-ret));
+return ret;
+}
+
+s->snapshots_offset = be64_to_cpu(snapshot_table_pointer.snapshots_offset);
+s->nb_snapshots = be32_to_cpu(snapshot_table_pointer.nb_snapshots);
+
+ret = qcow2_validate_table(bs, s->snapshots_offset, s->nb_snapshots,
+   sizeof(QCowSnapshotHeader),
+   sizeof(QCowSnapshotHeader) * QCOW_MAX_SNAPSHOTS,
+   "snapshot table", &local_err);
+if (ret < 0) {
+result->check_errors++;
+error_reportf_err(local_err, "ERROR ");
+
+/* We did not read the snapshot table, so invalidate this information 
*/
+s->snapshots_offset = 0;
+s->nb_snapshots = 0;
+
+return ret;
+}
+
+qemu_co_mutex_unlock(&s->lock);
+ret = qcow2_read_snapshots(bs, &local_err);
+qemu_co_mutex_lock(&s->lock);
+if (ret < 0) {
+result->check_errors++;
+error_reportf_err(local_err,
+  "ERROR failed to read the snapshot table: ");
+
+/* We did not read the snapshot table, so invalidate this information 
*/
+s->snapshots_offset = 0;
+s->nb_snapshots = 0;
+
+return ret;
+}
+
+return 0;
+}
+
 static void find_new_snapshot_id(BlockDriverState *bs,
  char *id_str, int id_str_size)
 {
diff --git a/block/qcow2.c b/block/qcow2.c
index 2219639e11..56c764de0b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -568,11 +568,40 @@ int qcow2_mark_consistent(BlockDriverState *bs)
 return 0;
 }
 
+static void qcow2_add_check_result(BdrvCheckResult *out,
+   const BdrvCheckResult *src,
+   bool set_allocation_info)
+{
+out->corruptions += src->corruptions;
+out->leaks += src->leaks;
+out->check_errors += src->check_errors;
+out->corruptions_fixed += src->corruptions_fixed;
+out->leaks_fixed += src->leaks_fixed;
+
+if (set_allocation_info) {
+out->image_end_offset = src->image_end_offset;
+out->bfi = src->bfi;
+}
+}
+
 static int coroutine_fn qcow2_co_check_locked(BlockDriverState *bs,
   BdrvCheckResult *result,
   BdrvCheckMode fix)
 {
-int ret = qcow2_check_refcounts(bs, result, fix);
+BdrvCheckResult snapshot_res = {};
+BdrvCheckResult refcount_res = {};
+int ret;
+
+memset(result, 0, sizeof(*result));
+
+ret = qcow2_check_read_snapshot_table(bs, &snapshot_res, fix);
+qcow2_add_check_result(result, &snapshot_res, false);
+if (ret < 0) {
+ret

[Qemu-block] [PATCH v2 11/16] qcow2: Keep track of the snapshot table length

[Max Reitz]

When repairing the snapshot table, we truncate entries that have too
much extra data.  This frees up space that we do not have to count
towards the snapshot table size.

Signed-off-by: Max Reitz 
---
 block/qcow2-snapshot.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 53dc1635ec..582eb3386a 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -68,6 +68,7 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool 
repair,
 QCowSnapshot *sn;
 int i, id_str_size, name_size;
 int64_t offset;
+uint64_t table_length = 0;
 int ret;
 
 if (!s->nb_snapshots) {
@@ -82,6 +83,8 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, bool 
repair,
 for(i = 0; i < s->nb_snapshots; i++) {
 bool truncate_unknown_extra_data = false;
 
+table_length = ROUND_UP(table_length, 8);
+
 /* Read statically sized part of the snapshot header */
 offset = ROUND_UP(offset, 8);
 ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
@@ -184,7 +187,16 @@ static int qcow2_do_read_snapshots(BlockDriverState *bs, 
bool repair,
 offset += name_size;
 sn->name[name_size] = '\0';
 
-if (offset - s->snapshots_offset > QCOW_MAX_SNAPSHOTS_SIZE) {
+/* Note that the extra data may have been truncated */
+table_length += sizeof(h) + sn->extra_data_size + id_str_size +
+name_size;
+if (!repair) {
+assert(table_length == offset - s->snapshots_offset);
+}
+
+if (table_length > QCOW_MAX_SNAPSHOTS_SIZE ||
+offset - s->snapshots_offset > INT_MAX)
+{
 ret = -EFBIG;
 error_setg(errp, "Snapshot table is too big");
 goto fail;
-- 
2.21.0

[Qemu-block] [PATCH v2 03/16] qcow2: Add Error ** to qcow2_read_snapshots()

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2.h  | 2 +-
 block/qcow2-snapshot.c | 7 ++-
 block/qcow2.c  | 3 +--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index fc1b0d3c1e..175708cee0 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -704,7 +704,7 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
 Error **errp);
 
 void qcow2_free_snapshots(BlockDriverState *bs);
-int qcow2_read_snapshots(BlockDriverState *bs);
+int qcow2_read_snapshots(BlockDriverState *bs, Error **errp);
 
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 752883e5c3..80d32e4504 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -43,7 +43,7 @@ void qcow2_free_snapshots(BlockDriverState *bs)
 s->nb_snapshots = 0;
 }
 
-int qcow2_read_snapshots(BlockDriverState *bs)
+int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
 QCowSnapshotHeader h;
@@ -68,6 +68,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 offset = ROUND_UP(offset, 8);
 ret = bdrv_pread(bs->file, offset, &h, sizeof(h));
 if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to read snapshot table");
 goto fail;
 }
 
@@ -88,6 +89,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 ret = bdrv_pread(bs->file, offset, &extra,
  MIN(sizeof(extra), extra_data_size));
 if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to read snapshot table");
 goto fail;
 }
 offset += extra_data_size;
@@ -107,6 +109,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 sn->id_str = g_malloc(id_str_size + 1);
 ret = bdrv_pread(bs->file, offset, sn->id_str, id_str_size);
 if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to read snapshot table");
 goto fail;
 }
 offset += id_str_size;
@@ -116,6 +119,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 sn->name = g_malloc(name_size + 1);
 ret = bdrv_pread(bs->file, offset, sn->name, name_size);
 if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to read snapshot table");
 goto fail;
 }
 offset += name_size;
@@ -123,6 +127,7 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 
 if (offset - s->snapshots_offset > QCOW_MAX_SNAPSHOTS_SIZE) {
 ret = -EFBIG;
+error_setg(errp, "Snapshot table is too big");
 goto fail;
 }
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index 7c5a4859f7..dbb50bc2f8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1578,9 +1578,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 s->snapshots_offset = header.snapshots_offset;
 s->nb_snapshots = header.nb_snapshots;
 
-ret = qcow2_read_snapshots(bs);
+ret = qcow2_read_snapshots(bs, errp);
 if (ret < 0) {
-error_setg_errno(errp, -ret, "Could not read snapshots");
 goto fail;
 }
 
-- 
2.21.0

[Qemu-block] [PATCH v2 05/16] qcow2: Make qcow2_write_snapshots() public

[Max Reitz]

Updating the snapshot list will be useful when upgrading a v2 image to
v3, so we will need to call this function in qcow2.c.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2.h  | 1 +
 block/qcow2-snapshot.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 290a48b77e..77586d81b9 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -710,6 +710,7 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
 
 void qcow2_free_snapshots(BlockDriverState *bs);
 int qcow2_read_snapshots(BlockDriverState *bs, Error **errp);
+int qcow2_write_snapshots(BlockDriverState *bs);
 
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 120cb7fa09..e3bf4c9776 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -164,7 +164,7 @@ fail:
 }
 
 /* add at the end of the file a new list of snapshots */
-static int qcow2_write_snapshots(BlockDriverState *bs)
+int qcow2_write_snapshots(BlockDriverState *bs)
 {
 BDRVQcow2State *s = bs->opaque;
 QCowSnapshot *sn;
-- 
2.21.0

[Qemu-block] [PATCH v2 06/16] qcow2: Put qcow2_upgrade() into its own function

[Max Reitz]

This does not make sense right now, but it will make sense once we need
to do more than to just update s->qcow_version.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
---
 block/qcow2.c | 43 ++-
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index dbb50bc2f8..190a3874c1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4749,12 +4749,46 @@ static int qcow2_downgrade(BlockDriverState *bs, int 
target_version,
 return 0;
 }
 
+/*
+ * Upgrades an image's version.  While newer versions encompass all
+ * features of older versions, some things may have to be presented
+ * differently.
+ */
+static int qcow2_upgrade(BlockDriverState *bs, int target_version,
+ BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+ Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+int current_version = s->qcow_version;
+int ret;
+
+/* This is qcow2_upgrade(), not qcow2_downgrade() */
+assert(target_version > current_version);
+
+/* There are no other versions (yet) that you can upgrade to */
+assert(target_version == 3);
+
+status_cb(bs, 0, 1, cb_opaque);
+
+s->qcow_version = target_version;
+ret = qcow2_update_header(bs);
+if (ret < 0) {
+s->qcow_version = current_version;
+error_setg_errno(errp, -ret, "Failed to update the image header");
+return ret;
+}
+status_cb(bs, 1, 1, cb_opaque);
+
+return 0;
+}
+
 typedef enum Qcow2AmendOperation {
 /* This is the value Qcow2AmendHelperCBInfo::last_operation will be
  * statically initialized to so that the helper CB can discern the first
  * invocation from an operation change */
 QCOW2_NO_OPERATION = 0,
 
+QCOW2_UPGRADING,
 QCOW2_CHANGING_REFCOUNT_ORDER,
 QCOW2_DOWNGRADING,
 } Qcow2AmendOperation;
@@ -4937,17 +4971,16 @@ static int qcow2_amend_options(BlockDriverState *bs, 
QemuOpts *opts,
 helper_cb_info = (Qcow2AmendHelperCBInfo){
 .original_status_cb = status_cb,
 .original_cb_opaque = cb_opaque,
-.total_operations = (new_version < old_version)
+.total_operations = (new_version != old_version)
   + (s->refcount_bits != refcount_bits)
 };
 
 /* Upgrade first (some features may require compat=1.1) */
 if (new_version > old_version) {
-s->qcow_version = new_version;
-ret = qcow2_update_header(bs);
+helper_cb_info.current_operation = QCOW2_UPGRADING;
+ret = qcow2_upgrade(bs, new_version, &qcow2_amend_helper_cb,
+&helper_cb_info, errp);
 if (ret < 0) {
-s->qcow_version = old_version;
-error_setg_errno(errp, -ret, "Failed to update the image header");
 return ret;
 }
 }
-- 
2.21.0

[Qemu-block] [PATCH v2 02/16] qcow2: Use endof()

[Max Reitz]

Signed-off-by: Max Reitz 
---
 block/qcow2-snapshot.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index d0e7fa9311..752883e5c3 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -92,11 +92,12 @@ int qcow2_read_snapshots(BlockDriverState *bs)
 }
 offset += extra_data_size;
 
-if (extra_data_size >= 8) {
+if (extra_data_size >= endof(QCowSnapshotExtraData,
+ vm_state_size_large)) {
 sn->vm_state_size = be64_to_cpu(extra.vm_state_size_large);
 }
 
-if (extra_data_size >= 16) {
+if (extra_data_size >= endof(QCowSnapshotExtraData, disk_size)) {
 sn->disk_size = be64_to_cpu(extra.disk_size);
 } else {
 sn->disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
@@ -251,7 +252,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 }
 
 QEMU_BUILD_BUG_ON(offsetof(QCowHeader, snapshots_offset) !=
-offsetof(QCowHeader, nb_snapshots) + sizeof(header_data.nb_snapshots));
+  endof(QCowHeader, nb_snapshots));
 
 header_data.nb_snapshots= cpu_to_be32(s->nb_snapshots);
 header_data.snapshots_offset= cpu_to_be64(snapshots_offset);
-- 
2.21.0

[Qemu-block] [PATCH v2 04/16] qcow2: Keep unknown extra snapshot data

[Max Reitz]

The qcow2 specification says to ignore unknown extra data fields in
snapshot table entries.  Currently, we discard it whenever we update the
image, which is a bit different from "ignore".

This patch makes the qcow2 driver keep all unknown extra data fields
when updating an image's snapshot table.

Signed-off-by: Max Reitz 
---
 block/qcow2.h  |  5 
 block/qcow2-snapshot.c | 61 +++---
 2 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 175708cee0..290a48b77e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -61,6 +61,9 @@
  * space for snapshot names and IDs */
 #define QCOW_MAX_SNAPSHOTS_SIZE (1024 * QCOW_MAX_SNAPSHOTS)
 
+/* Maximum amount of extra data per snapshot table entry to accept */
+#define QCOW_MAX_SNAPSHOT_EXTRA_DATA 1024
+
 /* Bitmap header extension constraints */
 #define QCOW2_MAX_BITMAPS 65535
 #define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS)
@@ -178,6 +181,8 @@ typedef struct QCowSnapshot {
 uint32_t date_sec;
 uint32_t date_nsec;
 uint64_t vm_clock_nsec;
+uint32_t extra_data_size;
+void *unknown_extra_data; /* Extra data past QCowSnapshotExtraData */
 } QCowSnapshot;
 
 struct Qcow2Cache;
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 80d32e4504..120cb7fa09 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -37,6 +37,7 @@ void qcow2_free_snapshots(BlockDriverState *bs)
 for(i = 0; i < s->nb_snapshots; i++) {
 g_free(s->snapshots[i].name);
 g_free(s->snapshots[i].id_str);
+g_free(s->snapshots[i].unknown_extra_data);
 }
 g_free(s->snapshots);
 s->snapshots = NULL;
@@ -51,7 +52,6 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
 QCowSnapshot *sn;
 int i, id_str_size, name_size;
 int64_t offset;
-uint32_t extra_data_size;
 int ret;
 
 if (!s->nb_snapshots) {
@@ -80,31 +80,53 @@ int qcow2_read_snapshots(BlockDriverState *bs, Error **errp)
 sn->date_sec = be32_to_cpu(h.date_sec);
 sn->date_nsec = be32_to_cpu(h.date_nsec);
 sn->vm_clock_nsec = be64_to_cpu(h.vm_clock_nsec);
-extra_data_size = be32_to_cpu(h.extra_data_size);
+sn->extra_data_size = be32_to_cpu(h.extra_data_size);
 
 id_str_size = be16_to_cpu(h.id_str_size);
 name_size = be16_to_cpu(h.name_size);
 
-/* Read extra data */
+if (sn->extra_data_size > QCOW_MAX_SNAPSHOT_EXTRA_DATA) {
+ret = -EFBIG;
+error_setg(errp, "Too much extra metadata in snapshot table "
+   "entry %i", i);
+goto fail;
+}
+
+/* Read known extra data */
 ret = bdrv_pread(bs->file, offset, &extra,
- MIN(sizeof(extra), extra_data_size));
+ MIN(sizeof(extra), sn->extra_data_size));
 if (ret < 0) {
 error_setg_errno(errp, -ret, "Failed to read snapshot table");
 goto fail;
 }
-offset += extra_data_size;
+offset += MIN(sizeof(extra), sn->extra_data_size);
 
-if (extra_data_size >= endof(QCowSnapshotExtraData,
- vm_state_size_large)) {
+if (sn->extra_data_size >= endof(QCowSnapshotExtraData,
+ vm_state_size_large)) {
 sn->vm_state_size = be64_to_cpu(extra.vm_state_size_large);
 }
 
-if (extra_data_size >= endof(QCowSnapshotExtraData, disk_size)) {
+if (sn->extra_data_size >= endof(QCowSnapshotExtraData, disk_size)) {
 sn->disk_size = be64_to_cpu(extra.disk_size);
 } else {
 sn->disk_size = bs->total_sectors * BDRV_SECTOR_SIZE;
 }
 
+if (sn->extra_data_size > sizeof(extra)) {
+/* Store unknown extra data */
+size_t unknown_extra_data_size =
+sn->extra_data_size - sizeof(extra);
+
+sn->unknown_extra_data = g_malloc(unknown_extra_data_size);
+ret = bdrv_pread(bs->file, offset, sn->unknown_extra_data,
+ unknown_extra_data_size);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to read snapshot table");
+goto fail;
+}
+offset += unknown_extra_data_size;
+}
+
 /* Read snapshot ID */
 sn->id_str = g_malloc(id_str_size + 1);
 ret = bdrv_pread(bs->file, offset, sn->id_str, id_str_size);
@@ -162,7 +184,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 sn = s->snapshots + i;
 offset = ROUND_UP(offset, 8);
 offset += sizeof(h);
-offset += sizeof(extra);
+offset += MAX(sizeof(extra), sn->extra_data_size);
 offset += strlen(sn->id_str);
 offset += strlen(sn->name);
 
@@ -209,7 +231,8 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
 h.da

[Qemu-block] [PATCH v2 01/16] include: Move endof() up from hw/virtio/virtio.h

[Max Reitz]

endof() is a useful macro, we can make use of it outside of virtio.

Signed-off-by: Max Reitz 
---
 include/hw/virtio/virtio.h |  7 ---
 include/qemu/compiler.h|  7 +++
 hw/block/virtio-blk.c  |  4 ++--
 hw/net/virtio-net.c| 10 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 48e8d04ff6..ef083af550 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -37,13 +37,6 @@ static inline hwaddr vring_align(hwaddr addr,
 return QEMU_ALIGN_UP(addr, align);
 }
 
-/*
- * Calculate the number of bytes up to and including the given 'field' of
- * 'container'.
- */
-#define virtio_endof(container, field) \
-(offsetof(container, field) + sizeof_field(container, field))
-
 typedef struct VirtIOFeature {
 uint64_t flags;
 size_t end;
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 09fc44cca4..dab3d7fad4 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -60,6 +60,13 @@
 
 #define sizeof_field(type, field) sizeof(((type *)0)->field)
 
+/*
+ * Calculate the number of bytes up to and including the given 'field' of
+ * 'container'.
+ */
+#define endof(container, field) \
+(offsetof(container, field) + sizeof_field(container, field))
+
 /* Convert from a base type to a parent type, with compile time checking.  */
 #ifdef __GNUC__
 #define DO_UPCAST(type, field, dev) ( __extension__ ( { \
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 18851601cb..1ad6a784f0 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -41,9 +41,9 @@
  */
 static VirtIOFeature feature_sizes[] = {
 {.flags = 1ULL << VIRTIO_BLK_F_DISCARD,
- .end = virtio_endof(struct virtio_blk_config, discard_sector_alignment)},
+ .end = endof(struct virtio_blk_config, discard_sector_alignment)},
 {.flags = 1ULL << VIRTIO_BLK_F_WRITE_ZEROES,
- .end = virtio_endof(struct virtio_blk_config, write_zeroes_may_unmap)},
+ .end = endof(struct virtio_blk_config, write_zeroes_may_unmap)},
 {}
 };
 
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9f11422337..2c4909c5f9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -90,15 +90,15 @@ static inline __virtio16 *virtio_net_rsc_ext_num_dupacks(
 
 static VirtIOFeature feature_sizes[] = {
 {.flags = 1ULL << VIRTIO_NET_F_MAC,
- .end = virtio_endof(struct virtio_net_config, mac)},
+ .end = endof(struct virtio_net_config, mac)},
 {.flags = 1ULL << VIRTIO_NET_F_STATUS,
- .end = virtio_endof(struct virtio_net_config, status)},
+ .end = endof(struct virtio_net_config, status)},
 {.flags = 1ULL << VIRTIO_NET_F_MQ,
- .end = virtio_endof(struct virtio_net_config, max_virtqueue_pairs)},
+ .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
 {.flags = 1ULL << VIRTIO_NET_F_MTU,
- .end = virtio_endof(struct virtio_net_config, mtu)},
+ .end = endof(struct virtio_net_config, mtu)},
 {.flags = 1ULL << VIRTIO_NET_F_SPEED_DUPLEX,
- .end = virtio_endof(struct virtio_net_config, duplex)},
+ .end = endof(struct virtio_net_config, duplex)},
 {}
 };
 
-- 
2.21.0

[Qemu-block] [PATCH v2 00/16] qcow2: Let check -r all repair some snapshot bits

[Max Reitz]

Hi,

See the v1 cover letter here if you haven’t already:
https://lists.nongnu.org/archive/html/qemu-block/2019-07/msg01290.html

I kept all patches from the previous version because Eric deemed the
patches I might have dropped useful.

I kept the way I implemented fixing overly long snapshot tables, namely
by discarding all snapshots past the maximum end.  Eric criticized this,
because this is indiscriminate (and the last ones have been created the
most recently, but I’m not sure whether we’d generally rather keep the
oldest or the most recent ones.  Maybe best would be to drop every
second snapshot? :-)).
I kept it as it is, because everything else would require user
interaction.  We currently have no interactive check mode, and I feel
like this is not a place where we need it badly: We introduced our
current snapshot limits in such a way that no sane image would ever hit
them.  If you do hit them, changes are that your image is already
corrupted, so you probably no longer care about your snapshots anyway
and just want to copy the active layer off.

Furthermore, I felt like an interactive mode would be pretty hard to
test thoroughly.  I don’t want to introduce code that probably nobody
will ever use and that is hardly tested.

I also tried Eric’s suggestion of using bdrv_preadv() in
qcow2_read_snapshots() to read all variable data at once, and while it
was very promising, it doesn’t allow us to skip data (which we must do
when we want to truncate extra data down to a sane size).  So all in all
I couldn’t make it work.


So, the changes in v2:
- Patch 1: Eric asked from some magic numbers to be removed (very
   reasonable), and endof() will help with that.
- Patch 2: See, it helps!
- Patch 4: Rebased conflicts (because of patch 2), and I should use
   BDRV_REQUEST_MAX_BYTES instead of INT_MAX when I mean the
   former
- Patch 7: Drop magic number [Eric]
- Patch 8: Drop magic number [Eric]
- Patch 10: Instead of dropping all unknown extra data when there is too
much, just truncate it down to the maximum [Eric]
- Patch 11: When some snapshot table entry has too much extra data and
we truncate it, that shortens the snapshot table length, so
we should keep that in mind when calculating whether the
table is too long
- Patch 12: Rebase conflicts, and dropped a magic number [Eric]
- Patch 14: Drop magic number [Eric]
- Patch 16: Simplified padding calculation [Eric], fixed one test case
(I used “sn-size” instead of “sn_size”, oops), and added a
test case for what happens when a snapshot table entry is
too big and contains an entry with too much extra data (the
latter should be fixed first and then we should look into
whether the table is still too long)


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/16:[down] 'include: Move endof() up from hw/virtio/virtio.h'
002/16:[down] 'qcow2: Use endof()'
003/16:[] [--] 'qcow2: Add Error ** to qcow2_read_snapshots()'
004/16:[0012] [FC] 'qcow2: Keep unknown extra snapshot data'
005/16:[] [--] 'qcow2: Make qcow2_write_snapshots() public'
006/16:[] [--] 'qcow2: Put qcow2_upgrade() into an own function'
007/16:[0005] [FC] 'qcow2: Write v3-compliant snapshot list on upgrade'
008/16:[0004] [FC] 'qcow2: Separate qcow2_check_read_snapshot_table()'
009/16:[] [--] 'qcow2: Add qcow2_check_fix_snapshot_table()'
010/16:[0038] [FC] 'qcow2: Fix broken snapshot table entries'
011/16:[down] 'qcow2: Keep track of the snapshot table length'
012/16:[0003] [FC] 'qcow2: Fix overly long snapshot tables'
013/16:[] [--] 'qcow2: Repair snapshot table with too many entries'
014/16:[0005] [FC] 'qcow2: Fix v3 snapshot table entry compliancy'
015/16:[] [--] 'iotests: Add peek_file* functions'
016/16:[0125] [FC] 'iotests: Test qcow2's snapshot table handling'


Max Reitz (16):
  include: Move endof() up from hw/virtio/virtio.h
  qcow2: Use endof()
  qcow2: Add Error ** to qcow2_read_snapshots()
  qcow2: Keep unknown extra snapshot data
  qcow2: Make qcow2_write_snapshots() public
  qcow2: Put qcow2_upgrade() into its own function
  qcow2: Write v3-compliant snapshot list on upgrade
  qcow2: Separate qcow2_check_read_snapshot_table()
  qcow2: Add qcow2_check_fix_snapshot_table()
  qcow2: Fix broken snapshot table entries
  qcow2: Keep track of the snapshot table length
  qcow2: Fix overly long snapshot tables
  qcow2: Repair snapshot table with too many entries
  qcow2: Fix v3 snapshot table entry compliancy
  iotests: Add peek_file* functions
  iotests: Test qcow2's snapshot table handling

 block/qcow2.h|  15 +-
 include/hw/virtio/virtio.h   |   7 -
 include/qemu/compiler.h  |   7 +
 block/qcow2-snapshot.c   | 323

Re: [Qemu-block] [qemu-s390x] [Qemu-devel] [PATCH v7 33/42] exec: Replace device_endian with MemOp

[Paolo Bonzini]

On 19/08/19 20:28, Paolo Bonzini wrote:
> On 16/08/19 12:12, Thomas Huth wrote:
>> This patch is *huge*, more than 800kB. It keeps being stuck in the the
>> filter of the qemu-s390x list each time you send it. Please:
>>
>> 1) Try to break it up in more digestible pieces, e.g. change only one
>> subsystem at a time (this is also better reviewable by people who are
>> interested in one area)
> 
> This is not really possible, since the patch is basically a
> search-and-replace.  You could perhaps use some magic
> ("DEVICE_MEMOP_ENDIAN" or something like that) to allow a split, but it
> would introduce more complication than anything else.

I'm stupid, at this point of the series it _would_ be possible to split
the patch by subsystem.  Still not sure it would be actually an advantage.

Paolo

> Agreed on the HTML though. :)
> 
> Paolo
> 

Re: [Qemu-block] [qemu-s390x] [Qemu-devel] [PATCH v7 33/42] exec: Replace device_endian with MemOp

[Paolo Bonzini]

On 16/08/19 12:12, Thomas Huth wrote:
> This patch is *huge*, more than 800kB. It keeps being stuck in the the
> filter of the qemu-s390x list each time you send it. Please:
> 
> 1) Try to break it up in more digestible pieces, e.g. change only one
> subsystem at a time (this is also better reviewable by people who are
> interested in one area)

This is not really possible, since the patch is basically a
search-and-replace.  You could perhaps use some magic
("DEVICE_MEMOP_ENDIAN" or something like that) to allow a split, but it
would introduce more complication than anything else.

Agreed on the HTML though. :)

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH v7 25/42] hw/misc: Declare device little or big endian

[Paolo Bonzini]

On 16/08/19 12:04, Philippe Mathieu-Daudé wrote:
>> diff --git a/hw/misc/a9scu.c b/hw/misc/a9scu.c
>> index 4307f00..3de8cd3 100644
>> --- a/hw/misc/a9scu.c
>> +++ b/hw/misc/a9scu.c
>> @@ -94,7 +94,7 @@ static void a9_scu_write(void *opaque, hwaddr offset,
>>  static const MemoryRegionOps a9_scu_ops = {
>>      .read = a9_scu_read,
>>      .write = a9_scu_write,
>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
> Uh, I doubt that.
> 

... why? :)

Remember that BE32 and BE8 ARM OSes still are "natively" little-endian.

Paolo

Re: [Qemu-block] [PATCH] nbd: Advertise multi-conn for shared read-only connections

[Eric Blake]

On 8/17/19 8:31 PM, Nir Soffer wrote:
>>> Also, for qemu-nbd, shouldn't we allow -e only together with -r ?
>>
>> I'm reluctant to; it might break whatever existing user is okay exposing
>> it (although such users are questionable, so maybe we can argue they
>> were already broken).  Maybe it's time to start a deprecation cycle?
>>
> 
> man qemu-nbd (on Centos 7.6) says:
> 
>-e, --shared=num
>Allow up to num clients to share the device (default 1)
> 
> I see that in qemu-img 4.1 there is a note about consistency with writers:
> 
>-e, --shared=num
>Allow up to num clients to share the device (default 1). Safe
> for readers, but for now, consistency is not guaranteed between multiple
> writers.
> But it is not clear what are the consistency guarantees.
> 
> Supporting multiple writers is important. oVirt is giving the user a URL
> (since 4.3), and the user
> can use multiple connections using the same URL, each having a connection
> to the same qemu-nbd
> socket. I know that some backup vendors tried to use multiple connections
> to speed up backups, and
> they may try to do this also for restore.
> 
> An interesting use case would be using multiple connections on client side
> to write in parallel to
> same image, when every client is writing different ranges.

Good to know.

> 
> Do we have real issue in qemu-nbd serving multiple clients writing to
> different parts of
> the same image?

If a server advertises multi-conn on a writable image, then clients have
stronger guarantees about behavior on what happens with flush on one
client vs. write in another, to the point that you can make some better
assumptions about image consistency, including what one client will read
after another has written.  But as long as multiple clients only ever
access distinct portions of the disk, then multi-conn is not important
to that client (whether for reading or for writing).

So it sounds like I have no reason to deprecate qemu-nbd -e 2, even for
writable images.

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH] nbd: Tolerate more errors to structured reply request

[Eric Blake]

A server may have a reason to reject a request for structured replies,
beyond just not recognizing them as a valid request.  It doesn't hurt
us to continue talking to such a server; otherwise 'qemu-nbd --list'
of such a server fails to display all possible details about the
export.

Encountered when temporarily tweaking nbdkit to reply with
NBD_REP_ERR_POLICY.  Present since structured reply support was first
added (commit d795299b reused starttls handling, but starttls has to
reject all errors).

Signed-off-by: Eric Blake 
---
 nbd/client.c | 39 +++
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index 8f524c3e3502..204f6e928d14 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -1,5 +1,5 @@
 /*
- *  Copyright (C) 2016-2018 Red Hat, Inc.
+ *  Copyright (C) 2016-2019 Red Hat, Inc.
  *  Copyright (C) 2005  Anthony Liguori 
  *
  *  Network Block Device Client Side
@@ -141,17 +141,19 @@ static int nbd_receive_option_reply(QIOChannel *ioc, 
uint32_t opt,
 return 0;
 }

-/* If reply represents success, return 1 without further action.
- * If reply represents an error, consume the optional payload of
- * the packet on ioc.  Then return 0 for unsupported (so the client
- * can fall back to other approaches), or -1 with errp set for other
- * errors.
+/*
+ * If reply represents success, return 1 without further action.  If
+ * reply represents an error, consume the optional payload of the
+ * packet on ioc.  Then return 0 for unsupported (so the client can
+ * fall back to other approaches), where @strict determines if only
+ * ERR_UNSUP or all errors fit that category, or -1 with errp set for
+ * other errors.
  */
 static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
-Error **errp)
+bool strict, Error **errp)
 {
 char *msg = NULL;
-int result = -1;
+int result = strict ? -1 : 0;

 if (!(reply->type & (1 << 31))) {
 return 1;
@@ -162,6 +164,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
NBDOptionReply *reply,
 error_setg(errp, "server error %" PRIu32
" (%s) message is too long",
reply->type, nbd_rep_lookup(reply->type));
+result = -1;
 goto cleanup;
 }
 msg = g_malloc(reply->length + 1);
@@ -169,6 +172,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, 
NBDOptionReply *reply,
 error_prepend(errp, "Failed to read option error %" PRIu32
   " (%s) message: ",
   reply->type, nbd_rep_lookup(reply->type));
+result = -1;
 goto cleanup;
 }
 msg[reply->length] = '\0';
@@ -257,7 +261,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, 
char **description,
 if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, &reply, errp) < 0) {
 return -1;
 }
-error = nbd_handle_reply_err(ioc, &reply, errp);
+error = nbd_handle_reply_err(ioc, &reply, true, errp);
 if (error <= 0) {
 return error;
 }
@@ -370,7 +374,7 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
 if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
 return -1;
 }
-error = nbd_handle_reply_err(ioc, &reply, errp);
+error = nbd_handle_reply_err(ioc, &reply, true, errp);
 if (error <= 0) {
 return error;
 }
@@ -545,12 +549,15 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
 }
 }

-/* nbd_request_simple_option: Send an option request, and parse the reply
+/*
+ * nbd_request_simple_option: Send an option request, and parse the reply.
+ * @strict controls whether ERR_UNSUP or all errors produce 0 status.
  * return 1 for successful negotiation,
  *0 if operation is unsupported,
  *-1 with errp set for any other error
  */
-static int nbd_request_simple_option(QIOChannel *ioc, int opt, Error **errp)
+static int nbd_request_simple_option(QIOChannel *ioc, int opt, bool strict,
+ Error **errp)
 {
 NBDOptionReply reply;
 int error;
@@ -562,7 +569,7 @@ static int nbd_request_simple_option(QIOChannel *ioc, int 
opt, Error **errp)
 if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
 return -1;
 }
-error = nbd_handle_reply_err(ioc, &reply, errp);
+error = nbd_handle_reply_err(ioc, &reply, strict, errp);
 if (error <= 0) {
 return error;
 }
@@ -594,7 +601,7 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
 QIOChannelTLS *tioc;
 struct NBDTLSHandshakeData data = { 0 };

-ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, errp);
+ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, true, errp);
 if (ret <= 0) {
 if (ret == 0) {
 error_setg(errp, "Server don't support STARTTLS optio

Re: [Qemu-block] [PATCH v4 00/14] block: Try to create well-typed json:{} filenames

[Max Reitz]

Ping

On 24.06.19 19:39, Max Reitz wrote:
> Hi,
> 
> There are two explanations of this cover letter, a relative one (to v3)
> and an absolute one.
> 
> 
> *** Important note ***
> 
> The final patch in this series is an example that converts most of
> block-core.json to use default values where possible.  We may decide to
> take it or not.  It isn’t important for the main purpose of this series,
> so I’d be very much fine with chopping it off.
> 
> (It does have a nice diff stat, though.)
> 
> *** Important note end ***
> 
> 
> Relative explanation:
> 
> The actual functional goal of this series is to allow all blockdev
> options that can be represented with -drive to have an equivalent with
> -blockdev (safe for rbd’s =keyvalue-pairs).
> 
> To this end, qcow(2)’s encryption needs an “auto” format which can
> automatically deduce the format from the image header.  To make things
> nicer, I decided (already in v1) to make this format optional so users
> could just specify encrypt.secret and let the format driver figure out
> the rest.
> 
> Until v3, this was implemented by letting the discriminator of flat
> unions be optional, as long as a default-value is provided.  Markus
> (rightfully) complained that this is very specific and would be covered
> by just having default values for QAPI struct members in general.
> So now this version implements this.  This is a bit more complicated
> than just implementing a default-variant, mainly because the latter only
> needs to accept enum values, whereas a generally usable “default” should
> accept values of all QAPI types (to the extent what is reasonable).
> 
> So what was (until v3)
> 
>   { 'union': 'Foo',
> 'base': { '*discr': 'SomeEnum' },
> 'discriminator': 'discr',
> 'default-variant': 'value1',
> 'data': { 'value1': 'Bar', 'value2': 'Baz' } }
> 
> becomes
> 
>   { 'union': 'Foo',
> 'base': { '*discr': { 'type': 'SomeEnum', 'default': 'value1' } },
> 'discriminator': 'discr',
> 'data': { 'value1': 'Bar', 'value2': 'Baz' } }
> 
> 
> 
> Absolute explanation:
> 
> When qemu reports json:{} filename, it just uses whatever type you gave
> an option in.  With -drive, all options are strings and they do not have
> to pass the test of the typing firewall of the QAPI schema, so you just
> get strings thrown back at you even if that does not match the schema.
> (Also, if you use json:{} yourself, you’re free to give the options as
> strings as well.)
> 
> Example:
> 
> $ ./qemu-img info --image-opts driver=raw,size=512,file.driver=null-co
> image: json:{"driver": "raw", "size": "512", "file": {"driver": "null-co"}}
> 
> @size is supposed to be an integer, according to the schema, so the
> correct result would be (which is what you get after this series):
> 
> $ ./qemu-img info --image-opts driver=raw,size=512,file.driver=null-co
> image: json:{"driver": "raw", "size": 512, "file": {"driver": "null-co"}}
> 
> 
> This is achieved by patch 11, which makes bdrv_refresh_filename() run
> the options through the flat-confused input visitor, and then through
> the output visitor to get all to the correct type.  If anything fails,
> the result is as before (hence the “Try” in the title).
> 
> There are cases where this cannot work.  Those are the ones where -drive
> accepts something that is not allowed by the QAPI schema.  One of these
> cases is rbd’s =keyvalue-pairs, which is just broken altogether, so
> let’s simply ignore that.  (I don’t think anybody’s going to complain
> that the json:{} filename they get is not correctly typed after they’ve
> used that option.)
> 
> The other case (I know of) is qcow(2)’s encryption.  In the QAPI schema,
> encrypt.format is not optional because it is the discriminator for which
> kind of options to use.  However, for -drive, it is optional because the
> qcow2 driver can infer the encryption format from the image header.
> 
> The solution that’s proposed by this series is to make flat union
> discriminators optional and provide a default.  This is accomplished by
> generally allowing default values to be provided for QAPI struct
> members.
> 
> Both AES and LUKS encryption allow only a key-secret option, so we can
> add a new pseudo-format “auto” that accepts exactly that option and
> makes the qcow2 driver read the real format from the image header.  This
> pseudo-format is made the default for encrypt.format, and thus you can
> then specify encrypt.key-secret without having to specify
> encrypt.format (while still adhering to the QAPI schema).
> 
> 
> So, in this series:
> - The QAPI code generator is modified to allow default values for
>   optional struct members.  This in turn allows flat union
>   discriminators be optional, too, but only if a default value is
>   provided.
>   - Accordingly, documentation, tests, and introspection are adjusted.
> 
> - This is used to make qcow’s and qcow2’s encrypt.format parameter
>   optional.  It now defaults to “from-image” which is a new
>   pseudo-format that al

[Qemu-block] [PULL 17/17] doc: Preallocation does not require writing zeroes

[Max Reitz]

When preallocating an encrypted qcow2 image, it just lets the protocol
driver write data and then does not mark the clusters as zero.
Therefore, reading this image will yield effectively random data.

As such, we have not fulfilled the promise of always writing zeroes when
preallocating an image in a while.  It seems that nobody has really
cared, so change the documentation to conform to qemu's actual behavior.

Signed-off-by: Max Reitz 
Message-id: 20190711132935.13070-1-mre...@redhat.com
Reviewed-by: Eric Blake 
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Maxim Levitsky 
Signed-off-by: Max Reitz 
---
 qapi/block-core.json | 9 +
 docs/qemu-block-drivers.texi | 4 ++--
 qemu-img.texi| 4 ++--
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index a5ab38db99..e6edd641f1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5178,10 +5178,11 @@
 # @off: no preallocation
 # @metadata: preallocate only for metadata
 # @falloc: like @full preallocation but allocate disk space by
-#  posix_fallocate() rather than writing zeros.
-# @full: preallocate all data by writing zeros to device to ensure disk
-#space is really available. @full preallocation also sets up
-#metadata correctly.
+#  posix_fallocate() rather than writing data.
+# @full: preallocate all data by writing it to the device to ensure
+#disk space is really available. This data may or may not be
+#zero, depending on the image format and storage.
+#@full preallocation also sets up metadata correctly.
 #
 # Since: 2.2
 ##
diff --git a/docs/qemu-block-drivers.texi b/docs/qemu-block-drivers.texi
index 91ab0eceae..c02547e28c 100644
--- a/docs/qemu-block-drivers.texi
+++ b/docs/qemu-block-drivers.texi
@@ -31,8 +31,8 @@ Supported options:
 @item preallocation
 Preallocation mode (allowed values: @code{off}, @code{falloc}, @code{full}).
 @code{falloc} mode preallocates space for image by calling posix_fallocate().
-@code{full} mode preallocates space for image by writing zeros to underlying
-storage.
+@code{full} mode preallocates space for image by writing data to underlying
+storage.  This data may or may not be zero, depending on the storage location.
 @end table
 
 @item qcow2
diff --git a/qemu-img.texi b/qemu-img.texi
index c8e9bba515..b5156d6316 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -666,8 +666,8 @@ Supported options:
 @item preallocation
 Preallocation mode (allowed values: @code{off}, @code{falloc}, @code{full}).
 @code{falloc} mode preallocates space for image by calling posix_fallocate().
-@code{full} mode preallocates space for image by writing zeros to underlying
-storage.
+@code{full} mode preallocates space for image by writing data to underlying
+storage.  This data may or may not be zero, depending on the storage location.
 @end table
 
 @item qcow2
-- 
2.21.0

[Qemu-block] [PULL 13/17] vdi: Make block_status recurse for fixed images

[Max Reitz]

Suggested-by: Vladimir Sementsov-Ogievskiy 
Fixes: 69f47505ee66afaa513305de0c1895a224e52c45
Signed-off-by: Max Reitz 
Message-id: 20190725155512.9827-2-mre...@redhat.com
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block/vdi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/vdi.c b/block/vdi.c
index 0caa3f281d..806ba7f53c 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -542,7 +542,8 @@ static int coroutine_fn 
vdi_co_block_status(BlockDriverState *bs,
 *map = s->header.offset_data + (uint64_t)bmap_entry * s->block_size +
 index_in_block;
 *file = bs->file->bs;
-return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID |
+(s->header.image_type == VDI_TYPE_STATIC ? BDRV_BLOCK_RECURSE : 0);
 }
 
 static int coroutine_fn
-- 
2.21.0

[Qemu-block] [PATCH v9] qemu-io: add pattern file for write command

[Denis Plotnikov]

The patch allows to provide a pattern file for write
command. There was no similar ability before.

Signed-off-by: Denis Plotnikov 
---
v9:
  * replace flag cast to int with bool [Eric]
  * fix the error message [Eric]
  * use qemu_io_free instead of qemu_vfree [Eric]
  * add function description [Eric]

v8: fix according to Max's comments
  * get rid of unnecessary buffer for the pattern
  * buffer allocation just in bytes
  * take into account the missalign offset
  * don't copy file name
  * changed char* to const char* in input params

v7:
  * fix variable naming
  * make code more readable
  * extend help for write command

v6:
  * the pattern file is read once to reduce io

v5:
  * file name initiated with null to make compilers happy

v4:
  * missing signed-off clause added

v3:
  * missing file closing added
  * exclusive flags processing changed
  * buffer void* converted to char* to fix pointer arithmetics
  * file reading error processing added
---
 qemu-io-cmds.c | 97 ++
 1 file changed, 91 insertions(+), 6 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 09750a23ce..f7bdfe673b 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -351,6 +351,77 @@ static void qemu_io_free(void *p)
 qemu_vfree(p);
 }
 
+/*
+ * qemu_io_alloc_from_file()
+ *
+ * Allocates the buffer and populates it with the content of the given file
+ * up to @len bytes. If the file length is less then @len, then the buffer
+ * is populated with then file content cyclically.
+ *
+ * @blk - the block backend where the buffer content is going to be written to
+ * @len - the buffer length
+ * @file_name - the file to copy the content from
+ *
+ * Returns: the buffer pointer on success
+ *  NULL on error
+ */
+static void *qemu_io_alloc_from_file(BlockBackend *blk, size_t len,
+ const char *file_name)
+{
+char *buf, *buf_origin;
+FILE *f = fopen(file_name, "r");
+int pattern_len;
+
+if (!f) {
+perror(file_name);
+return NULL;
+}
+
+if (qemuio_misalign) {
+len += MISALIGN_OFFSET;
+}
+
+buf_origin = buf = blk_blockalign(blk, len);
+
+if (qemuio_misalign) {
+buf_origin += MISALIGN_OFFSET;
+}
+
+pattern_len = fread(buf_origin, 1, len, f);
+
+if (ferror(f)) {
+perror(file_name);
+goto error;
+}
+
+if (pattern_len == 0) {
+fprintf(stderr, "%s: file is empty\n", file_name);
+goto error;
+}
+
+fclose(f);
+
+if (len > pattern_len) {
+len -= pattern_len;
+buf += pattern_len;
+
+while (len > 0) {
+size_t len_to_copy = MIN(pattern_len, len);
+
+memcpy(buf, buf_origin, len_to_copy);
+
+len -= len_to_copy;
+buf += len_to_copy;
+}
+}
+
+return buf_origin;
+
+error:
+qemu_io_free(buf_origin);
+return NULL;
+}
+
 static void dump_buffer(const void *buffer, int64_t offset, int64_t len)
 {
 uint64_t i;
@@ -949,6 +1020,7 @@ static void write_help(void)
 " -n, -- with -z, don't allow slow fallback\n"
 " -p, -- ignored for backwards compatibility\n"
 " -P, -- use different pattern to fill file\n"
+" -s, -- use a pattern file to fill the write buffer\n"
 " -C, -- report statistics in a machine parsable format\n"
 " -q, -- quiet mode, do not show I/O statistics\n"
 " -u, -- with -z, allow unmapping\n"
@@ -965,7 +1037,7 @@ static const cmdinfo_t write_cmd = {
 .perm   = BLK_PERM_WRITE,
 .argmin = 2,
 .argmax = -1,
-.args   = "[-bcCfnquz] [-P pattern] off len",
+.args   = "[-bcCfnquz] [-P pattern | -s source_file] off len",
 .oneline= "writes a number of bytes at a specified offset",
 .help   = write_help,
 };
@@ -974,7 +1046,7 @@ static int write_f(BlockBackend *blk, int argc, char 
**argv)
 {
 struct timeval t1, t2;
 bool Cflag = false, qflag = false, bflag = false;
-bool Pflag = false, zflag = false, cflag = false;
+bool Pflag = false, zflag = false, cflag = false, sflag = false;
 int flags = 0;
 int c, cnt, ret;
 char *buf = NULL;
@@ -983,8 +1055,9 @@ static int write_f(BlockBackend *blk, int argc, char 
**argv)
 /* Some compilers get confused and warn if this is not initialized.  */
 int64_t total = 0;
 int pattern = 0xcd;
+const char *file_name = NULL;
 
-while ((c = getopt(argc, argv, "bcCfnpP:quz")) != -1) {
+while ((c = getopt(argc, argv, "bcCfnpP:quzs:")) != -1) {
 switch (c) {
 case 'b':
 bflag = true;
@@ -1020,6 +1093,10 @@ static int write_f(BlockBackend *blk, int argc, char 
**argv)
 case 'z':
 zflag = true;
 break;
+case 's':
+sflag = true;
+file_name = optarg;
+break;
 default:
 qemuio_command_usage(&write_cmd);
 return -EINVAL;
@@ -1051,8 +1128,9 @@ static int write_

[Qemu-block] [PULL 11/17] iotests: Test convert -n to pre-filled image

[Max Reitz]

Signed-off-by: Max Reitz 
Message-id: 20190724171239.8764-11-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/122 | 17 +
 tests/qemu-iotests/122.out |  8 
 2 files changed, 25 insertions(+)

diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
index 85c3a8d047..059011ebb1 100755
--- a/tests/qemu-iotests/122
+++ b/tests/qemu-iotests/122
@@ -257,6 +257,23 @@ for min_sparse in 4k 8k; do
 $QEMU_IMG map --output=json "$TEST_IMG".orig | _filter_qemu_img_map
 done
 
+
+echo
+echo '=== -n to a non-zero image ==='
+echo
+
+# Keep source zero
+_make_test_img 64M
+
+# Output is not zero, but has bdrv_has_zero_init() == 1
+TEST_IMG="$TEST_IMG".orig _make_test_img 64M
+$QEMU_IO -c "write -P 42 0 64k" "$TEST_IMG".orig | _filter_qemu_io
+
+# Convert with -n, which should not assume that the target is zeroed
+$QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG".orig
+
+$QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index c576705284..849b6cc2ef 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -220,4 +220,12 @@ convert -c -S 8k
 { "start": 9216, "length": 8192, "depth": 0, "zero": true, "data": false},
 { "start": 17408, "length": 1024, "depth": 0, "zero": false, "data": true},
 { "start": 18432, "length": 67090432, "depth": 0, "zero": true, "data": false}]
+
+=== -n to a non-zero image ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Images are identical.
 *** done
-- 
2.21.0

[Qemu-block] [PULL 15/17] vpc: Do not return RAW from block_status

[Max Reitz]

vpc is not really a passthrough driver, even when using the fixed
subformat (where host and guest offsets are equal).  It should handle
preallocation like all other drivers do, namely by returning
DATA | RECURSE instead of RAW.

There is no tangible difference but the fact that bdrv_is_allocated() no
longer falls through to the protocol layer.

Signed-off-by: Max Reitz 
Message-id: 20190725155512.9827-4-mre...@redhat.com
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block/vpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vpc.c b/block/vpc.c
index d4776ee8a5..b25aab0425 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -737,7 +737,7 @@ static int coroutine_fn 
vpc_co_block_status(BlockDriverState *bs,
 *pnum = bytes;
 *map = offset;
 *file = bs->file->bs;
-return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID;
+return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_RECURSE;
 }
 
 qemu_co_mutex_lock(&s->lock);
-- 
2.21.0

[Qemu-block] [PULL 14/17] vmdk: Make block_status recurse for flat extents

[Max Reitz]

Fixes: 69f47505ee66afaa513305de0c1895a224e52c45
Signed-off-by: Max Reitz 
Message-id: 20190725155512.9827-3-mre...@redhat.com
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 block/vmdk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index bd36ece125..fd78fd0ccf 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1692,6 +1692,9 @@ static int coroutine_fn 
vmdk_co_block_status(BlockDriverState *bs,
 if (!extent->compressed) {
 ret |= BDRV_BLOCK_OFFSET_VALID;
 *map = cluster_offset + index_in_cluster;
+if (extent->flat) {
+ret |= BDRV_BLOCK_RECURSE;
+}
 }
 *file = extent->file->bs;
 break;
-- 
2.21.0

[Qemu-block] [PULL 09/17] vhdx: Fix .bdrv_has_zero_init()

[Max Reitz]

Fixed VHDX images cannot guarantee to be zero-initialized.  If the image
has the "fixed" subformat, forward the call to the underlying storage
node.

Reported-by: Stefano Garzarella 
Signed-off-by: Max Reitz 
Message-id: 20190724171239.8764-9-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Signed-off-by: Max Reitz 
---
 block/vhdx.c | 26 +-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index a02d1c99a7..6a09d0a55c 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2075,6 +2075,30 @@ static int coroutine_fn vhdx_co_check(BlockDriverState 
*bs,
 return 0;
 }
 
+static int vhdx_has_zero_init(BlockDriverState *bs)
+{
+BDRVVHDXState *s = bs->opaque;
+int state;
+
+/*
+ * Check the subformat: Fixed images have all BAT entries present,
+ * dynamic images have none (right after creation).  It is
+ * therefore enough to check the first BAT entry.
+ */
+if (!s->bat_entries) {
+return 1;
+}
+
+state = s->bat[0] & VHDX_BAT_STATE_BIT_MASK;
+if (state == PAYLOAD_BLOCK_FULLY_PRESENT) {
+/* Fixed subformat */
+return bdrv_has_zero_init(bs->file->bs);
+}
+
+/* Dynamic subformat */
+return 1;
+}
+
 static QemuOptsList vhdx_create_opts = {
 .name = "vhdx-create-opts",
 .head = QTAILQ_HEAD_INITIALIZER(vhdx_create_opts.head),
@@ -2128,7 +2152,7 @@ static BlockDriver bdrv_vhdx = {
 .bdrv_co_create_opts= vhdx_co_create_opts,
 .bdrv_get_info  = vhdx_get_info,
 .bdrv_co_check  = vhdx_co_check,
-.bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_has_zero_init = vhdx_has_zero_init,
 
 .create_opts= &vhdx_create_opts,
 };
-- 
2.21.0

[Qemu-block] [PULL 10/17] iotests: Convert to preallocated encrypted qcow2

[Max Reitz]

Add a test case for converting an empty image (which only returns zeroes
when read) to a preallocated encrypted qcow2 image.
qcow2_has_zero_init() should return 0 then, thus forcing qemu-img
convert to create zero clusters.

Signed-off-by: Max Reitz 
Acked-by: Stefano Garzarella 
Tested-by: Stefano Garzarella 
Message-id: 20190724171239.8764-10-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/188 | 20 +++-
 tests/qemu-iotests/188.out |  4 
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/188 b/tests/qemu-iotests/188
index be7278aa65..afca44df54 100755
--- a/tests/qemu-iotests/188
+++ b/tests/qemu-iotests/188
@@ -48,7 +48,7 @@ SECRETALT="secret,id=sec0,data=platypus"
 
 _make_test_img --object $SECRET -o 
"encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10" $size
 
-IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.key-secret=sec0"
+IMGSPEC="driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
 
 QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
 
@@ -68,6 +68,24 @@ echo
 echo "== verify open failure with wrong password =="
 $QEMU_IO --object $SECRETALT -c "read -P 0xa 0 $size" --image-opts $IMGSPEC | 
_filter_qemu_io | _filter_testdir
 
+_cleanup_test_img
+
+echo
+echo "== verify that has_zero_init returns false when preallocating =="
+
+# Empty source file
+if [ -n "$TEST_IMG_FILE" ]; then
+TEST_IMG_FILE="${TEST_IMG_FILE}.orig" _make_test_img $size
+else
+TEST_IMG="${TEST_IMG}.orig" _make_test_img $size
+fi
+
+$QEMU_IMG convert -O "$IMGFMT" --object $SECRET \
+-o 
"encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,preallocation=metadata"
 \
+"${TEST_IMG}.orig" "$TEST_IMG"
+
+$QEMU_IMG compare --object $SECRET --image-opts "${IMGSPEC}.orig" "$IMGSPEC"
+
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/188.out b/tests/qemu-iotests/188.out
index 97b1402671..c568ef3701 100644
--- a/tests/qemu-iotests/188.out
+++ b/tests/qemu-iotests/188.out
@@ -15,4 +15,8 @@ read 16777216/16777216 bytes at offset 0
 
 == verify open failure with wrong password ==
 qemu-io: can't open: Invalid password, cannot unlock any keyslot
+
+== verify that has_zero_init returns false when preallocating ==
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=16777216
+Images are identical.
 *** done
-- 
2.21.0

[Qemu-block] [PULL 08/17] vdi: Fix .bdrv_has_zero_init()

[Max Reitz]

Static VDI images cannot guarantee to be zero-initialized.  If the image
has been statically allocated, forward the call to the underlying
storage node.

Reported-by: Stefano Garzarella 
Signed-off-by: Max Reitz 
Reviewed-by: Stefan Weil 
Acked-by: Stefano Garzarella 
Tested-by: Stefano Garzarella 
Message-id: 20190724171239.8764-8-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Signed-off-by: Max Reitz 
---
 block/vdi.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/vdi.c b/block/vdi.c
index b9845a4cbd..0caa3f281d 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -988,6 +988,17 @@ static void vdi_close(BlockDriverState *bs)
 error_free(s->migration_blocker);
 }
 
+static int vdi_has_zero_init(BlockDriverState *bs)
+{
+BDRVVdiState *s = bs->opaque;
+
+if (s->header.image_type == VDI_TYPE_STATIC) {
+return bdrv_has_zero_init(bs->file->bs);
+} else {
+return 1;
+}
+}
+
 static QemuOptsList vdi_create_opts = {
 .name = "vdi-create-opts",
 .head = QTAILQ_HEAD_INITIALIZER(vdi_create_opts.head),
@@ -1028,7 +1039,7 @@ static BlockDriver bdrv_vdi = {
 .bdrv_child_perm  = bdrv_format_default_perms,
 .bdrv_co_create  = vdi_co_create,
 .bdrv_co_create_opts = vdi_co_create_opts,
-.bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_has_zero_init  = vdi_has_zero_init,
 .bdrv_co_block_status = vdi_co_block_status,
 .bdrv_make_empty = vdi_make_empty,
 
-- 
2.21.0

[Qemu-block] [PULL 12/17] iotests: Full mirror to existing non-zero image

[Max Reitz]

The result of a sync=full mirror should always be the equal to the
input.  Therefore, existing images should be treated as potentially
non-zero and thus should be explicitly initialized to be zero
beforehand.

Signed-off-by: Max Reitz 
Message-id: 20190724171239.8764-12-mre...@redhat.com
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/041 | 62 +++---
 tests/qemu-iotests/041.out |  4 +--
 2 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 26bf1701eb..8bc8f81db7 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -741,8 +741,15 @@ class TestUnbackedSource(iotests.QMPTestCase):
 def setUp(self):
 qemu_img('create', '-f', iotests.imgfmt, test_img,
  str(TestUnbackedSource.image_len))
-self.vm = iotests.VM().add_drive(test_img)
+self.vm = iotests.VM()
 self.vm.launch()
+result = self.vm.qmp('blockdev-add', node_name='drive0',
+ driver=iotests.imgfmt,
+ file={
+ 'driver': 'file',
+ 'filename': test_img,
+ })
+self.assert_qmp(result, 'return', {})
 
 def tearDown(self):
 self.vm.shutdown()
@@ -751,7 +758,7 @@ class TestUnbackedSource(iotests.QMPTestCase):
 
 def test_absolute_paths_full(self):
 self.assert_no_active_block_jobs()
-result = self.vm.qmp('drive-mirror', device='drive0',
+result = self.vm.qmp('drive-mirror', job_id='drive0', device='drive0',
  sync='full', target=target_img,
  mode='absolute-paths')
 self.assert_qmp(result, 'return', {})
@@ -760,7 +767,7 @@ class TestUnbackedSource(iotests.QMPTestCase):
 
 def test_absolute_paths_top(self):
 self.assert_no_active_block_jobs()
-result = self.vm.qmp('drive-mirror', device='drive0',
+result = self.vm.qmp('drive-mirror', job_id='drive0', device='drive0',
  sync='top', target=target_img,
  mode='absolute-paths')
 self.assert_qmp(result, 'return', {})
@@ -769,13 +776,60 @@ class TestUnbackedSource(iotests.QMPTestCase):
 
 def test_absolute_paths_none(self):
 self.assert_no_active_block_jobs()
-result = self.vm.qmp('drive-mirror', device='drive0',
+result = self.vm.qmp('drive-mirror', job_id='drive0', device='drive0',
  sync='none', target=target_img,
  mode='absolute-paths')
 self.assert_qmp(result, 'return', {})
 self.complete_and_wait()
 self.assert_no_active_block_jobs()
 
+def test_existing_full(self):
+qemu_img('create', '-f', iotests.imgfmt, target_img,
+ str(self.image_len))
+qemu_io('-c', 'write -P 42 0 64k', target_img)
+
+self.assert_no_active_block_jobs()
+result = self.vm.qmp('drive-mirror', job_id='drive0', device='drive0',
+ sync='full', target=target_img, mode='existing')
+self.assert_qmp(result, 'return', {})
+self.complete_and_wait()
+self.assert_no_active_block_jobs()
+
+result = self.vm.qmp('blockdev-del', node_name='drive0')
+self.assert_qmp(result, 'return', {})
+
+self.assertTrue(iotests.compare_images(test_img, target_img),
+'target image does not match source after mirroring')
+
+def test_blockdev_full(self):
+qemu_img('create', '-f', iotests.imgfmt, target_img,
+ str(self.image_len))
+qemu_io('-c', 'write -P 42 0 64k', target_img)
+
+result = self.vm.qmp('blockdev-add', node_name='target',
+ driver=iotests.imgfmt,
+ file={
+ 'driver': 'file',
+ 'filename': target_img,
+ })
+self.assert_qmp(result, 'return', {})
+
+self.assert_no_active_block_jobs()
+result = self.vm.qmp('blockdev-mirror', job_id='drive0', 
device='drive0',
+ sync='full', target='target')
+self.assert_qmp(result, 'return', {})
+self.complete_and_wait()
+self.assert_no_active_block_jobs()
+
+result = self.vm.qmp('blockdev-del', node_name='drive0')
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('blockdev-del', node_name='target')
+self.assert_qmp(result, 'return', {})
+
+self.assertTrue(iotests.compare_images(test_img, target_img),
+'target image does not match source after mirroring')
+
 class TestGranularity(iotests.QMPTestCase):
 image_len = 10 * 1024 * 1024 # MB
 
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index e071

[Qemu-block] [PULL 16/17] iotests: Fix 141 when run with qed

[Max Reitz]

69f47505ee has changed qcow2 in such a way that the commit job run in
test 141 (and 144[1]) returns before it emits the READY event.  However,
141 also runs with qed, where the order is still the other way around.
Just filter out the {"return": {}} so the test passes for qed again.

[1] 144 only runs with qcow2, so it is fine as it is.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Fixes: 69f47505ee66afaa513305de0c1895a224e52c45
Signed-off-by: Max Reitz 
Message-id: 20190809185253.17535-1-mre...@redhat.com
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/141   | 9 +++--
 tests/qemu-iotests/141.out   | 5 -
 tests/qemu-iotests/common.filter | 5 +
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
index 2197a82d45..8c2ae79f2b 100755
--- a/tests/qemu-iotests/141
+++ b/tests/qemu-iotests/141
@@ -58,16 +58,21 @@ test_blockjob()
   }}}" \
 'return'
 
+# If "$2" is an event, we may or may not see it before the
+# {"return": {}}.  Therefore, filter the {"return": {}} out both
+# here and in the next command.  (Naturally, if we do not see it
+# here, we will see it before the next command can be executed,
+# so it will appear in the next _send_qemu_cmd's output.)
 _send_qemu_cmd $QEMU_HANDLE \
 "$1" \
 "$2" \
-| _filter_img_create
+| _filter_img_create | _filter_qmp_empty_return
 
 # We want this to return an error because the block job is still running
 _send_qemu_cmd $QEMU_HANDLE \
 "{'execute': 'blockdev-del',
   'arguments': {'node-name': 'drv0'}}" \
-'error' | _filter_generated_node_ids
+'error' | _filter_generated_node_ids | _filter_qmp_empty_return
 
 _send_qemu_cmd $QEMU_HANDLE \
 "{'execute': 'block-job-cancel',
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
index 4d71d9dcae..dbd3bdef6c 100644
--- a/tests/qemu-iotests/141.out
+++ b/tests/qemu-iotests/141.out
@@ -10,7 +10,6 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/m.
 Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
-{"return": {}}
 {"error": {"class": "GenericError", "desc": "Node drv0 is in use"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "job0"}}
@@ -27,7 +26,6 @@ Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_READY", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 
0, "type": "mirror"}}
-{"return": {}}
 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device 
is in use by block job: mirror"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "job0"}}
@@ -42,7 +40,6 @@ Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 
backing_file=TEST_DIR/t.
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
-{"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"BLOCK_JOB_READY", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 
0, "type": "commit"}}
 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device 
is in use by block job: commit"}}
@@ -61,7 +58,6 @@ wrote 1048576/1048576 bytes at offset 0
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": 
"JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
-{"return": {}}
 {"error": {"class": "GenericError", "desc": "Node drv0 is in use"}}
 {"retur

[Qemu-block] [PULL 03/17] mirror: Fix bdrv_has_zero_init() use

[Max Reitz]

bdrv_has_zero_init() only has meaning for newly created images or image
areas.  If the mirror job itself did not create the image, it cannot
rely on bdrv_has_zero_init()'s result to carry any meaning.

This is the case for drive-mirror with mode=existing and always for
blockdev-mirror.

Note that we only have to zero-initialize the target with sync=full,
because other modes actually do not promise that the target will contain
the same data as the source after the job -- sync=top only promises to
copy anything allocated in the top layer, and sync=none will only copy
new I/O.  (Which is how mirror has always handled it.)

Signed-off-by: Max Reitz 
Message-id: 20190724171239.8764-3-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Signed-off-by: Max Reitz 
---
 include/block/block_int.h   |  2 ++
 block/mirror.c  | 11 ---
 blockdev.c  | 16 +---
 tests/test-block-iothread.c |  2 +-
 4 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index aa697f1f69..8fa011654a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1115,6 +1115,7 @@ BlockJob *commit_active_start(const char *job_id, 
BlockDriverState *bs,
  * @buf_size: The amount of data that can be in flight at one time.
  * @mode: Whether to collapse all images in the chain to the target.
  * @backing_mode: How to establish the target's backing chain after completion.
+ * @zero_target: Whether the target should be explicitly zero-initialized
  * @on_source_error: The action to take upon error reading from the source.
  * @on_target_error: The action to take upon error writing to the target.
  * @unmap: Whether to unmap target where source sectors only contain zeroes.
@@ -1134,6 +1135,7 @@ void mirror_start(const char *job_id, BlockDriverState 
*bs,
   int creation_flags, int64_t speed,
   uint32_t granularity, int64_t buf_size,
   MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
+  bool zero_target,
   BlockdevOnError on_source_error,
   BlockdevOnError on_target_error,
   bool unmap, const char *filter_node_name,
diff --git a/block/mirror.c b/block/mirror.c
index 2b870683f1..853e2c7510 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -51,6 +51,8 @@ typedef struct MirrorBlockJob {
 Error *replace_blocker;
 bool is_none_mode;
 BlockMirrorBackingMode backing_mode;
+/* Whether the target image requires explicit zero-initialization */
+bool zero_target;
 MirrorCopyMode copy_mode;
 BlockdevOnError on_source_error, on_target_error;
 bool synced;
@@ -767,7 +769,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 int ret;
 int64_t count;
 
-if (base == NULL && !bdrv_has_zero_init(target_bs)) {
+if (s->zero_target) {
 if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
 bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
 return 0;
@@ -1515,6 +1517,7 @@ static BlockJob *mirror_start_job(
  const char *replaces, int64_t speed,
  uint32_t granularity, int64_t buf_size,
  BlockMirrorBackingMode backing_mode,
+ bool zero_target,
  BlockdevOnError on_source_error,
  BlockdevOnError on_target_error,
  bool unmap,
@@ -1643,6 +1646,7 @@ static BlockJob *mirror_start_job(
 s->on_target_error = on_target_error;
 s->is_none_mode = is_none_mode;
 s->backing_mode = backing_mode;
+s->zero_target = zero_target;
 s->copy_mode = copy_mode;
 s->base = base;
 s->granularity = granularity;
@@ -1747,6 +1751,7 @@ void mirror_start(const char *job_id, BlockDriverState 
*bs,
   int creation_flags, int64_t speed,
   uint32_t granularity, int64_t buf_size,
   MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
+  bool zero_target,
   BlockdevOnError on_source_error,
   BlockdevOnError on_target_error,
   bool unmap, const char *filter_node_name,
@@ -1764,7 +1769,7 @@ void mirror_start(const char *job_id, BlockDriverState 
*bs,
 is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
 base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
 mirror_start_job(job_id, bs, creation_flags, target, replaces,
- speed, granularity, buf_size, backing_mode,
+ speed, granularity, buf_size, backing_mode, zero_target,
  on_source_error, on_target_error, unmap, NULL, NULL,
  &mirror_job_driver, is_none_mode, base, false,
  filter_node_name, true, copy_mode, errp);
@@ -1791,7 +1796,7 @@ BlockJob *commit_active_start(

[Qemu-block] [PULL 07/17] qcow2: Fix .bdrv_has_zero_init()

[Max Reitz]

If a qcow2 file is preallocated, it can no longer guarantee that it
initially appears as filled with zeroes.

So implement .bdrv_has_zero_init() by checking whether the file is
preallocated; if so, forward the call to the underlying storage node,
except for when it is encrypted: Encrypted preallocated images always
return effectively random data, so .bdrv_has_zero_init() must always
return 0 for them.

.bdrv_has_zero_init_truncate() can remain bdrv_has_zero_init_1(),
because it presupposes PREALLOC_MODE_OFF.

Reported-by: Stefano Garzarella 
Signed-off-by: Max Reitz 
Message-id: 20190724171239.8764-7-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Signed-off-by: Max Reitz 
---
 block/qcow2.c | 29 -
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index ea3b42fdac..7c5a4859f7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4632,6 +4632,33 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs,
 return spec_info;
 }
 
+static int qcow2_has_zero_init(BlockDriverState *bs)
+{
+BDRVQcow2State *s = bs->opaque;
+bool preallocated;
+
+if (qemu_in_coroutine()) {
+qemu_co_mutex_lock(&s->lock);
+}
+/*
+ * Check preallocation status: Preallocated images have all L2
+ * tables allocated, nonpreallocated images have none.  It is
+ * therefore enough to check the first one.
+ */
+preallocated = s->l1_size > 0 && s->l1_table[0] != 0;
+if (qemu_in_coroutine()) {
+qemu_co_mutex_unlock(&s->lock);
+}
+
+if (!preallocated) {
+return 1;
+} else if (bs->encrypted) {
+return 0;
+} else {
+return bdrv_has_zero_init(s->data_file->bs);
+}
+}
+
 static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
   int64_t pos)
 {
@@ -5187,7 +5214,7 @@ BlockDriver bdrv_qcow2 = {
 .bdrv_child_perm  = bdrv_format_default_perms,
 .bdrv_co_create_opts  = qcow2_co_create_opts,
 .bdrv_co_create   = qcow2_co_create,
-.bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_has_zero_init   = qcow2_has_zero_init,
 .bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
 .bdrv_co_block_status = qcow2_co_block_status,
 
-- 
2.21.0

[Qemu-block] [PULL 06/17] block: Use bdrv_has_zero_init_truncate()

[Max Reitz]

vhdx and parallels call bdrv_has_zero_init() when they do not really
care about an image's post-create state but only about what happens when
you grow an image.  That is a bit ugly, and also overly safe when
growing preallocated images without preallocating the new areas.

Let them use bdrv_has_zero_init_truncate() instead.

Signed-off-by: Max Reitz 
Message-id: 20190724171239.8764-6-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Reviewed-by: Stefano Garzarella 
[mreitz: Added commit message]
Signed-off-by: Max Reitz 
---
 block/parallels.c | 2 +-
 block/vhdx.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 00fae125d1..7cd2714b69 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -835,7 +835,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
 goto fail_options;
 }
 
-if (!bdrv_has_zero_init(bs->file->bs)) {
+if (!bdrv_has_zero_init_truncate(bs->file->bs)) {
 s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
 }
 
diff --git a/block/vhdx.c b/block/vhdx.c
index d6070b6fa8..a02d1c99a7 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1282,7 +1282,7 @@ static coroutine_fn int vhdx_co_writev(BlockDriverState 
*bs, int64_t sector_num,
 /* Queue another write of zero buffers if the underlying file
  * does not zero-fill on file extension */
 
-if (bdrv_has_zero_init(bs->file->bs) == 0) {
+if (bdrv_has_zero_init_truncate(bs->file->bs) == 0) {
 use_zero_buffers = true;
 
 /* zero fill the front, if any */
-- 
2.21.0

[Qemu-block] [PULL 05/17] block: Implement .bdrv_has_zero_init_truncate()

[Max Reitz]

We need to implement .bdrv_has_zero_init_truncate() for every block
driver that supports truncation and has a .bdrv_has_zero_init()
implementation.

Implement it the same way each driver implements .bdrv_has_zero_init().
This is at least not any more unsafe than what we had before.

Signed-off-by: Max Reitz 
Message-id: 20190724171239.8764-5-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Reviewed-by: Stefano Garzarella 
Signed-off-by: Max Reitz 
---
 block/file-posix.c | 1 +
 block/file-win32.c | 1 +
 block/gluster.c| 4 
 block/nfs.c| 1 +
 block/qcow2.c  | 1 +
 block/qed.c| 1 +
 block/raw-format.c | 6 ++
 block/rbd.c| 1 +
 block/sheepdog.c   | 1 +
 block/ssh.c| 1 +
 10 files changed, 18 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index e41e91e075..fbeb0068db 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2938,6 +2938,7 @@ BlockDriver bdrv_file = {
 .bdrv_co_create = raw_co_create,
 .bdrv_co_create_opts = raw_co_create_opts,
 .bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
 .bdrv_co_block_status = raw_co_block_status,
 .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
 .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
diff --git a/block/file-win32.c b/block/file-win32.c
index 6b2d67b239..41f55dfece 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -635,6 +635,7 @@ BlockDriver bdrv_file = {
 .bdrv_close = raw_close,
 .bdrv_co_create_opts = raw_co_create_opts,
 .bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
 
 .bdrv_aio_preadv= raw_aio_preadv,
 .bdrv_aio_pwritev   = raw_aio_pwritev,
diff --git a/block/gluster.c b/block/gluster.c
index f64dc5b01e..64028b2cba 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1567,6 +1567,7 @@ static BlockDriver bdrv_gluster = {
 .bdrv_co_writev   = qemu_gluster_co_writev,
 .bdrv_co_flush_to_disk= qemu_gluster_co_flush_to_disk,
 .bdrv_has_zero_init   = qemu_gluster_has_zero_init,
+.bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
 .bdrv_co_pdiscard = qemu_gluster_co_pdiscard,
 #endif
@@ -1598,6 +1599,7 @@ static BlockDriver bdrv_gluster_tcp = {
 .bdrv_co_writev   = qemu_gluster_co_writev,
 .bdrv_co_flush_to_disk= qemu_gluster_co_flush_to_disk,
 .bdrv_has_zero_init   = qemu_gluster_has_zero_init,
+.bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
 .bdrv_co_pdiscard = qemu_gluster_co_pdiscard,
 #endif
@@ -1629,6 +1631,7 @@ static BlockDriver bdrv_gluster_unix = {
 .bdrv_co_writev   = qemu_gluster_co_writev,
 .bdrv_co_flush_to_disk= qemu_gluster_co_flush_to_disk,
 .bdrv_has_zero_init   = qemu_gluster_has_zero_init,
+.bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
 .bdrv_co_pdiscard = qemu_gluster_co_pdiscard,
 #endif
@@ -1666,6 +1669,7 @@ static BlockDriver bdrv_gluster_rdma = {
 .bdrv_co_writev   = qemu_gluster_co_writev,
 .bdrv_co_flush_to_disk= qemu_gluster_co_flush_to_disk,
 .bdrv_has_zero_init   = qemu_gluster_has_zero_init,
+.bdrv_has_zero_init_truncate  = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
 .bdrv_co_pdiscard = qemu_gluster_co_pdiscard,
 #endif
diff --git a/block/nfs.c b/block/nfs.c
index ed0cce63bb..0ec50953e4 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -863,6 +863,7 @@ static BlockDriver bdrv_nfs = {
 .create_opts= &nfs_create_opts,
 
 .bdrv_has_zero_init = nfs_has_zero_init,
+.bdrv_has_zero_init_truncate= nfs_has_zero_init,
 .bdrv_get_allocated_file_size   = nfs_get_allocated_file_size,
 .bdrv_co_truncate   = nfs_file_co_truncate,
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 59cff1d4cb..ea3b42fdac 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5188,6 +5188,7 @@ BlockDriver bdrv_qcow2 = {
 .bdrv_co_create_opts  = qcow2_co_create_opts,
 .bdrv_co_create   = qcow2_co_create,
 .bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
 .bdrv_co_block_status = qcow2_co_block_status,
 
 .bdrv_co_preadv = qcow2_co_preadv,
diff --git a/block/qed.c b/block/qed.c
index d0dcc5f14d..0d8fd507aa 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1669,6 +1669,7 @@ static BlockDriver bdrv_qed = {
 .bdrv_co_create   = bdrv_qed_co_create,
 .bdrv_co_create_opts  = bdrv_qed_co_create_opts,
 .bdrv_has_zero_init   = bdrv_has_zero_init_1,
+.bdrv_has_zero_init_truncate = bdrv_has_zero_init_1,
 .bdrv_co_block_status = bdrv_qed_co_block_status,
 .bdrv_co_rea

[Qemu-block] [PULL 02/17] qemu-img: Fix bdrv_has_zero_init() use in convert

[Max Reitz]

bdrv_has_zero_init() only has meaning for newly created images or image
areas.  If qemu-img convert did not create the image itself, it cannot
rely on bdrv_has_zero_init()'s result to carry any meaning.

Signed-off-by: Max Reitz 
Message-id: 20190724171239.8764-2-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Reviewed-by: Stefano Garzarella 
Signed-off-by: Max Reitz 
---
 qemu-img.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index c920e3564c..7daa05e51a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1578,6 +1578,7 @@ typedef struct ImgConvertState {
 bool has_zero_init;
 bool compressed;
 bool unallocated_blocks_are_zero;
+bool target_is_new;
 bool target_has_backing;
 int64_t target_backing_sectors; /* negative if unknown */
 bool wr_in_order;
@@ -1975,9 +1976,11 @@ static int convert_do_copy(ImgConvertState *s)
 int64_t sector_num = 0;
 
 /* Check whether we have zero initialisation or can get it efficiently */
-s->has_zero_init = s->min_sparse && !s->target_has_backing
- ? bdrv_has_zero_init(blk_bs(s->target))
- : false;
+if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
+s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
+} else {
+s->has_zero_init = false;
+}
 
 if (!s->has_zero_init && !s->target_has_backing &&
 bdrv_can_write_zeroes_with_unmap(blk_bs(s->target)))
@@ -2428,6 +2431,8 @@ static int img_convert(int argc, char **argv)
 }
 }
 
+s.target_is_new = !skip_create;
+
 flags = s.min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
 ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
 if (ret < 0) {
-- 
2.21.0

[Qemu-block] [PULL 04/17] block: Add bdrv_has_zero_init_truncate()

[Max Reitz]

No .bdrv_has_zero_init() implementation returns 1 if growing the file
would add non-zero areas (at least with PREALLOC_MODE_OFF), so using it
in lieu of this new function was always safe.

But on the other hand, it is possible that growing an image that is not
zero-initialized would still add a zero-initialized area, like when
using nonpreallocating truncation on a preallocated image.  For callers
that care only about truncation, not about creation with potential
preallocation, this new function is useful.

Alternatively, we could have added a PreallocMode parameter to
bdrv_has_zero_init().  But the only user would have been qemu-img
convert, which does not have a plain PreallocMode value right now -- it
would have to parse the creation option to obtain it.  Therefore, the
simpler solution is to let bdrv_has_zero_init() inquire the
preallocation status and add the new bdrv_has_zero_init_truncate() that
presupposes PREALLOC_MODE_OFF.

Signed-off-by: Max Reitz 
Message-id: 20190724171239.8764-4-mre...@redhat.com
Reviewed-by: Maxim Levitsky 
Reviewed-by: Stefano Garzarella 
Signed-off-by: Max Reitz 
---
 include/block/block.h |  1 +
 include/block/block_int.h |  7 +++
 block.c   | 21 +
 3 files changed, 29 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index 89e40318cf..124ad40809 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -443,6 +443,7 @@ int bdrv_pdiscard(BdrvChild *child, int64_t offset, int64_t 
bytes);
 int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
 int bdrv_has_zero_init_1(BlockDriverState *bs);
 int bdrv_has_zero_init(BlockDriverState *bs);
+int bdrv_has_zero_init_truncate(BlockDriverState *bs);
 bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs);
 bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
 int bdrv_block_status(BlockDriverState *bs, int64_t offset,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8fa011654a..ceec8c2f56 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -419,9 +419,16 @@ struct BlockDriver {
 /*
  * Returns 1 if newly created images are guaranteed to contain only
  * zeros, 0 otherwise.
+ * Must return 0 if .bdrv_has_zero_init_truncate() returns 0.
  */
 int (*bdrv_has_zero_init)(BlockDriverState *bs);
 
+/*
+ * Returns 1 if new areas added by growing the image with
+ * PREALLOC_MODE_OFF contain only zeros, 0 otherwise.
+ */
+int (*bdrv_has_zero_init_truncate)(BlockDriverState *bs);
+
 /* Remove fd handlers, timers, and other event loop callbacks so the event
  * loop is no longer in use.  Called with no in-flight requests and in
  * depth-first traversal order with parents before child nodes.
diff --git a/block.c b/block.c
index 3e698e9cab..874a29a983 100644
--- a/block.c
+++ b/block.c
@@ -5078,6 +5078,27 @@ int bdrv_has_zero_init(BlockDriverState *bs)
 return 0;
 }
 
+int bdrv_has_zero_init_truncate(BlockDriverState *bs)
+{
+if (!bs->drv) {
+return 0;
+}
+
+if (bs->backing) {
+/* Depends on the backing image length, but better safe than sorry */
+return 0;
+}
+if (bs->drv->bdrv_has_zero_init_truncate) {
+return bs->drv->bdrv_has_zero_init_truncate(bs);
+}
+if (bs->file && bs->drv->is_filter) {
+return bdrv_has_zero_init_truncate(bs->file->bs);
+}
+
+/* safe default */
+return 0;
+}
+
 bool bdrv_unallocated_blocks_are_zero(BlockDriverState *bs)
 {
 BlockDriverInfo bdi;
-- 
2.21.0

[Qemu-block] [PULL 01/17] LUKS: support preallocation

[Max Reitz]

From: Maxim Levitsky 

preallocation=off and preallocation=metadata
both allocate luks header only, and preallocation=falloc/full
is passed to underlying file.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534951

Signed-off-by: Maxim Levitsky 
Message-id: 20190716161901.1430-1-mlevi...@redhat.com
Signed-off-by: Max Reitz 
---
 qapi/block-core.json |  6 +-
 block/crypto.c   | 30 +++---
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index e9364a4a29..a5ab38db99 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4212,13 +4212,17 @@
 #
 # @file Node to create the image format on
 # @size Size of the virtual disk in bytes
+# @preallocationPreallocation mode for the new image
+#   (since: 4.2)
+#   (default: off; allowed values: off, metadata, falloc, full)
 #
 # Since: 2.12
 ##
 { 'struct': 'BlockdevCreateOptionsLUKS',
   'base': 'QCryptoBlockCreateOptionsLUKS',
   'data': { 'file': 'BlockdevRef',
-'size': 'size' } }
+'size': 'size',
+'*preallocation':   'PreallocMode' } }
 
 ##
 # @BlockdevCreateOptionsNfs:
diff --git a/block/crypto.c b/block/crypto.c
index 8237424ae6..7eb698774e 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -74,6 +74,7 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block,
 struct BlockCryptoCreateData {
 BlockBackend *blk;
 uint64_t size;
+PreallocMode prealloc;
 };
 
 
@@ -112,7 +113,7 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
  * available to the guest, so we must take account of that
  * which will be used by the crypto header
  */
-return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF,
+return blk_truncate(data->blk, data->size + headerlen, data->prealloc,
 errp);
 }
 
@@ -251,6 +252,7 @@ static int block_crypto_open_generic(QCryptoBlockFormat 
format,
 static int block_crypto_co_create_generic(BlockDriverState *bs,
   int64_t size,
   QCryptoBlockCreateOptions *opts,
+  PreallocMode prealloc,
   Error **errp)
 {
 int ret;
@@ -266,9 +268,14 @@ static int block_crypto_co_create_generic(BlockDriverState 
*bs,
 goto cleanup;
 }
 
+if (prealloc == PREALLOC_MODE_METADATA) {
+prealloc = PREALLOC_MODE_OFF;
+}
+
 data = (struct BlockCryptoCreateData) {
 .blk = blk,
 .size = size,
+.prealloc = prealloc,
 };
 
 crypto = qcrypto_block_create(opts, NULL,
@@ -500,6 +507,7 @@ block_crypto_co_create_luks(BlockdevCreateOptions 
*create_options, Error **errp)
 BlockdevCreateOptionsLUKS *luks_opts;
 BlockDriverState *bs = NULL;
 QCryptoBlockCreateOptions create_opts;
+PreallocMode preallocation = PREALLOC_MODE_OFF;
 int ret;
 
 assert(create_options->driver == BLOCKDEV_DRIVER_LUKS);
@@ -515,8 +523,12 @@ block_crypto_co_create_luks(BlockdevCreateOptions 
*create_options, Error **errp)
 .u.luks = *qapi_BlockdevCreateOptionsLUKS_base(luks_opts),
 };
 
+if (luks_opts->has_preallocation) {
+preallocation = luks_opts->preallocation;
+}
+
 ret = block_crypto_co_create_generic(bs, luks_opts->size, &create_opts,
- errp);
+ preallocation, errp);
 if (ret < 0) {
 goto fail;
 }
@@ -534,12 +546,24 @@ static int coroutine_fn 
block_crypto_co_create_opts_luks(const char *filename,
 QCryptoBlockCreateOptions *create_opts = NULL;
 BlockDriverState *bs = NULL;
 QDict *cryptoopts;
+PreallocMode prealloc;
+char *buf = NULL;
 int64_t size;
 int ret;
+Error *local_err = NULL;
 
 /* Parse options */
 size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
 
+buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
+prealloc = qapi_enum_parse(&PreallocMode_lookup, buf,
+   PREALLOC_MODE_OFF, &local_err);
+g_free(buf);
+if (local_err) {
+error_propagate(errp, local_err);
+return -EINVAL;
+}
+
 cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL,
  &block_crypto_create_opts_luks,
  true);
@@ -565,7 +589,7 @@ static int coroutine_fn 
block_crypto_co_create_opts_luks(const char *filename,
 }
 
 /* Create format layer */
-ret = block_crypto_co_create_generic(bs, size, create_opts, errp);
+ret = block_crypto_co_create_generic(bs, size, create_opts, prealloc, 
errp);
 if (ret < 0) {
 goto fail;
 }
-- 
2.21.0

[Qemu-block] [PULL 00/17] Block patches

[Max Reitz]

The following changes since commit 3fbd3405d2b0604ea530fc7a1828f19da1e95ff9:

  Merge remote-tracking branch 
'remotes/huth-gitlab/tags/pull-request-2019-08-17' into staging (2019-08-19 
14:14:09 +0100)

are available in the Git repository at:

  https://github.com/XanClic/qemu.git tags/pull-block-2019-08-19

for you to fetch changes up to fa27c478102a6b5d1c6b02c005607ad9404b915f:

  doc: Preallocation does not require writing zeroes (2019-08-19 17:13:26 +0200)


Block patches:
- preallocation=falloc/full support for LUKS
- Various minor fixes


Max Reitz (16):
  qemu-img: Fix bdrv_has_zero_init() use in convert
  mirror: Fix bdrv_has_zero_init() use
  block: Add bdrv_has_zero_init_truncate()
  block: Implement .bdrv_has_zero_init_truncate()
  block: Use bdrv_has_zero_init_truncate()
  qcow2: Fix .bdrv_has_zero_init()
  vdi: Fix .bdrv_has_zero_init()
  vhdx: Fix .bdrv_has_zero_init()
  iotests: Convert to preallocated encrypted qcow2
  iotests: Test convert -n to pre-filled image
  iotests: Full mirror to existing non-zero image
  vdi: Make block_status recurse for fixed images
  vmdk: Make block_status recurse for flat extents
  vpc: Do not return RAW from block_status
  iotests: Fix 141 when run with qed
  doc: Preallocation does not require writing zeroes

Maxim Levitsky (1):
  LUKS: support preallocation

 qapi/block-core.json | 15 +---
 include/block/block.h|  1 +
 include/block/block_int.h|  9 +
 block.c  | 21 +++
 block/crypto.c   | 30 ++--
 block/file-posix.c   |  1 +
 block/file-win32.c   |  1 +
 block/gluster.c  |  4 +++
 block/mirror.c   | 11 --
 block/nfs.c  |  1 +
 block/parallels.c|  2 +-
 block/qcow2.c| 30 +++-
 block/qed.c  |  1 +
 block/raw-format.c   |  6 
 block/rbd.c  |  1 +
 block/sheepdog.c |  1 +
 block/ssh.c  |  1 +
 block/vdi.c  | 16 +++--
 block/vhdx.c | 28 +--
 block/vmdk.c |  3 ++
 block/vpc.c  |  2 +-
 blockdev.c   | 16 +++--
 qemu-img.c   | 11 --
 tests/test-block-iothread.c  |  2 +-
 docs/qemu-block-drivers.texi |  4 +--
 qemu-img.texi|  4 +--
 tests/qemu-iotests/041   | 62 +---
 tests/qemu-iotests/041.out   |  4 +--
 tests/qemu-iotests/122   | 17 +
 tests/qemu-iotests/122.out   |  8 +
 tests/qemu-iotests/141   |  9 +++--
 tests/qemu-iotests/141.out   |  5 ---
 tests/qemu-iotests/188   | 20 ++-
 tests/qemu-iotests/188.out   |  4 +++
 tests/qemu-iotests/common.filter |  5 +++
 35 files changed, 313 insertions(+), 43 deletions(-)

-- 
2.21.0

[Qemu-block] [PATCH v3 3/3] qcow2: add zstd cluster compression

[Denis Plotnikov]

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

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

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

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

   compression   decompression
 zlib   zstd   zlib zstd

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

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

Signed-off-by: Denis Plotnikov 
---
 block/qcow2-threads.c  | 94 ++
 block/qcow2.c  |  6 +++
 configure  | 34 +++
 docs/interop/qcow2.txt | 20 +
 qapi/block-core.json   |  3 +-
 5 files changed, 156 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 14b5bd76fb..85d04e6c2e 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
 #define ZLIB_CONST
 #include 
 
+#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
+
 #include "qcow2.h"
 #include "block/thread-pool.h"
 #include "crypto.h"
@@ -165,6 +170,85 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
dest_size,
 return ret;
 }
 
+#ifdef CONFIG_ZSTD
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *  -ENOMEM destination buffer is not enough to store compressed data
+ *  -EIOon any other error
+ */
+
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
+{
+ssize_t ret;
+uint32_t *c_size = dest;
+/* steal some bytes to store compressed chunk size */
+char *d_buf = ((char *) dest) + sizeof(*c_size);
+
+if (dest_size < sizeof(*c_size)) {
+return -ENOMEM;
+}
+
+dest_size -= sizeof(*c_size);
+
+ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
+
+if (ZSTD_isError(ret)) {
+if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
+return -ENOMEM;
+} else {
+return -EIO;
+}
+}
+
+/* store the compressed chunk size in the very beginning of the buffer */
+*c_size = ret;
+
+return ret + sizeof(*c_size);
+}
+
+/*
+ * qcow2_zstd_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  -EIO on any error
+ */
+
+static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
+{
+ssize_t ret;
+/*
+ * zstd decompress wants to know the exact length of the data
+ * for that purpose, on the compression the length is stored in
+ * the very beginning of the compressed buffer
+ */
+const uint32_t *s_size = src;
+const char *s_buf = ((const char *) src) + sizeof(*s_size);
+
+ret = ZSTD_decompress(dest, dest_size, s_buf, *s_size);
+
+if (ZSTD_isError(ret)) {
+return -EIO;
+}
+
+return 0;
+}
+#endif
+
 static int qcow2_compress_pool_func(void *opaque)
 {
 Qcow2CompressData *data = opaque;
@@ -216,6 +300,11 @@ qcow2_co_compress(BlockDriverState *bs, void *dest, size_t 
dest_size,
 fn = qcow2_zlib_compress;
 break;
 
+#ifdef CONFIG_ZSTD
+case QCOW2_COMPRESSION_TYPE_ZSTD:
+fn = qcow2_zstd_compress;
+break;
+#endif
 default:
 return -ENOTSUP;
 }
@@ -248,6 +337,11 @@ qcow2_co_decompress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 fn = qcow2_zlib_decompress;
 break;
 
+#ifdef CONFIG_ZSTD
+case QCOW2_COMPRESSION_TYPE_ZSTD:
+fn = qcow2_zstd_decompress;
+break;
+#endif
 default:
 return -ENOTSUP;
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index 4e07b7e9ec..dfb7b52033 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1201,6 +1201,9 @@ static int check_compres

[Qemu-block] [PATCH v3 2/3] qcow2: rework the cluster compression routine

[Denis Plotnikov]

The patch allow to process image compression type defined
in the image header and choose an appropriate method for
image clusters (de)compression.

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

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 3b1e63fe41..14b5bd76fb 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -73,8 +73,11 @@ typedef struct Qcow2CompressData {
 Qcow2CompressFunc func;
 } Qcow2CompressData;
 
+
 /*
- * qcow2_compress()
+ * qcow2_zlib_compress()
+ *
+ * Compress @src_size bytes of data using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
@@ -83,8 +86,8 @@ typedef struct Qcow2CompressData {
  *  -ENOMEM destination buffer is not enough to store compressed data
  *  -EIOon any other error
  */
-static ssize_t qcow2_compress(void *dest, size_t dest_size,
-  const void *src, size_t src_size)
+static ssize_t qcow2_zlib_compress(void *dest, size_t dest_size,
+   const void *src, size_t src_size)
 {
 ssize_t ret;
 z_stream strm;
@@ -119,19 +122,19 @@ static ssize_t qcow2_compress(void *dest, size_t 
dest_size,
 }
 
 /*
- * qcow2_decompress()
+ * qcow2_zlib_decompress()
  *
  * Decompress some data (not more than @src_size bytes) to produce exactly
- * @dest_size bytes.
+ * @dest_size bytes using zlib compression method
  *
  * @dest - destination buffer, @dest_size bytes
  * @src - source buffer, @src_size bytes
  *
  * Returns: 0 on success
- *  -1 on fail
+ *  -EIO on fail
  */
-static ssize_t qcow2_decompress(void *dest, size_t dest_size,
-const void *src, size_t src_size)
+static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
 {
 int ret = 0;
 z_stream strm;
@@ -144,7 +147,7 @@ static ssize_t qcow2_decompress(void *dest, size_t 
dest_size,
 
 ret = inflateInit2(&strm, -12);
 if (ret != Z_OK) {
-return -1;
+return -EIO;
 }
 
 ret = inflate(&strm, Z_FINISH);
@@ -154,7 +157,7 @@ static ssize_t qcow2_decompress(void *dest, size_t 
dest_size,
  * @src buffer may be processed partly (because in qcow2 we know size 
of
  * compressed data with precision of one sector)
  */
-ret = -1;
+ret = -EIO;
 }
 
 inflateEnd(&strm);
@@ -189,20 +192,67 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, 
size_t dest_size,
 return arg.ret;
 }
 
+/*
+ * qcow2_co_compress()
+ *
+ * Compress @src_size bytes of data using the compression
+ * method defined by the image compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  a negative error code on fail
+ */
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
   const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_compress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_compress;
+break;
+
+default:
+return -ENOTSUP;
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
+/*
+ * qcow2_co_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using the compression method defined by the image
+ * compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *  a negative error code on fail
+ */
 ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
 const void *src, size_t src_size)
 {
-return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
-qcow2_decompress);
+BDRVQcow2State *s = bs->opaque;
+Qcow2CompressFunc fn;
+
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+fn = qcow2_zlib_decompress;
+break;
+
+default:
+return -ENOTSUP;
+}
+
+return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
 }
 
 
-- 
2.17.0

[Qemu-block] [PATCH v3 0/3] qcow2: add zstd cluster compression

[Denis Plotnikov]

v3:
* relax the compression type setting requirement when
  the compression type is not zlib [Eric, Kevin]
* add compression type values to the spec [Eric]
* fix wording in the spec and descriptions [Eric]
* fix functions descriptions [Max]
* fix zstd (de)compression functions flaws [Max]
* fix zstd related parts of configure file [Max]
* rebased to v4.1.0-rc5 and chenged the series version aiming to 4.2

v2:
* relax the compression type setting restriction in the spec
* fix qcow2 header size checking
* fix error processing and messaging
* fix qcow2 image specific info reporting
* set Qcow2CompressionType zstd config dependant
* add zstd compressed cluster format description to the spec

v1:
* extend qcow2 header instead of adding a new incompatible extension header
specification re-written accordingly
* enable zstd compression via config
* fix zstd (de)compression functions
* fix comments/description
* fix function naming

---
The goal of the patch-set is to enable qcow2 to use zstd compression for
clusters. ZSTD provides better (de)compression performance than currently
used ZLIB. Using it will improve perforamnce (reduce compression time)
when the compressed clusters is used, e.g backup scenarios.

Also, the patch-set extends qcow2 specification by adding compression_type
feature. The feature enables adding ZSTD and another compression algorithms
in the future.

Here is some measurements ZSTD vs ZLIB:

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

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

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


The results:
compression decompression
zlib zstd zlib zstd

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

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

Denis Plotnikov (3):
  qcow2: introduce compression type feature
  qcow2: rework the cluster compression routine
  qcow2: add zstd cluster compression

 block/qcow2-threads.c | 172 ++
 block/qcow2.c | 100 ++
 block/qcow2.h |  26 --
 configure |  34 
 docs/interop/qcow2.txt|  39 -
 include/block/block_int.h |   1 +
 qapi/block-core.json  |  23 -
 7 files changed, 371 insertions(+), 24 deletions(-)

-- 
2.17.0

[Qemu-block] [PATCH v3 1/3] qcow2: introduce compression type feature

[Denis Plotnikov]

The patch adds some preparation parts for incompatible compression type
feature to QCOW2 header that indicates that *all* compressed clusters
must be (de)compressed using a certain compression type.

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

The goal of the feature is to add support of other compression algorithms
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
It works roughly 2x faster than ZLIB providing a comparable compression ratio
and therefore provides a performance advantage in backup scenarios.

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

Signed-off-by: Denis Plotnikov 
---
 block/qcow2.c | 94 +++
 block/qcow2.h | 26 ---
 docs/interop/qcow2.txt| 19 +++-
 include/block/block_int.h |  1 +
 qapi/block-core.json  | 22 -
 5 files changed, 152 insertions(+), 10 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 039bdc2f7e..4e07b7e9ec 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1197,6 +1197,32 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
 return ret;
 }
 
+static int check_compression_type(BDRVQcow2State *s, Error **errp)
+{
+switch (s->compression_type) {
+case QCOW2_COMPRESSION_TYPE_ZLIB:
+break;
+
+default:
+error_setg(errp, "qcow2: unknown compression type: %u",
+   s->compression_type);
+return -ENOTSUP;
+}
+
+/*
+ * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
+ * the incompatible feature flag must be set
+ */
+
+if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB &&
+!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
+error_setg(errp, "qcow2: Invalid compression type setting");
+return -EINVAL;
+}
+
+return 0;
+}
+
 /* Called with s->lock held.  */
 static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
   int flags, Error **errp)
@@ -1312,6 +1338,35 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
*bs, QDict *options,
 s->compatible_features  = header.compatible_features;
 s->autoclear_features   = header.autoclear_features;
 
+/*
+ * Handle compression type
+ * Older qcow2 images don't contain the compression type header.
+ * Distinguish them by the header length and use
+ * the only valid (default) compression type in that case
+ */
+if (header.header_length > offsetof(QCowHeader, compression_type)) {
+/* sanity check that we can read a compression type */
+size_t min_len = offsetof(QCowHeader, compression_type) +
+ sizeof(header.compression_type);
+if (header.header_length < min_len) {
+error_setg(errp,
+   "Could not read compression type, "
+   "qcow2 header is too short");
+ret = -EINVAL;
+goto fail;
+}
+
+header.compression_type = be32_to_cpu(header.compression_type);
+s->compression_type = header.compression_type;
+} else {
+s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+}
+
+ret = check_compression_type(s, errp);
+if (ret) {
+goto fail;
+}
+
 if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
 void *feature_table = NULL;
 qcow2_read_extensions(bs, header.header_length, ext_end,
@@ -2516,6 +2571,12 @@ int qcow2_update_header(BlockDriverState *bs)
 total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
 refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 3);
 
+ret = check_compression_type(s, NULL);
+
+if (ret) {
+goto fail;
+}
+
 *header = (QCowHeader) {
 /* Version 2 fields */
 .magic  = cpu_to_be32(QCOW_MAGIC),
@@ -2538,6 +2599,7 @@ int qcow2_update_header(BlockDriverState *bs)
 .autoclear_features = cpu_to_be64(s->autoclear_features),
 .refcount_order = cpu_to_be32(s->refcount_order),
 .header_length  = cpu_to_be32(header_length),
+.compression_type   = cpu_to_be32(s->compression_type),
 };
 
 /* For older versions, write a shorter header */
@@ -2635,6 +2697,11 @@ int qcow2_update_header(BlockDriverState *bs)
 .bit  = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
 .name = "lazy refcounts",
 },
+{
+.type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
+.bit  = QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
+.name = "compression type",
+},
 };
 
 ret = header_ext_add(buf, QCOW2_E

Re: [Qemu-block] [PATCH 02/13] qcrypto-luks: misc refactoring

[Maxim Levitsky]

On Thu, 2019-08-15 at 17:40 -0400, John Snow wrote:
> 
> On 8/14/19 4:22 PM, Maxim Levitsky wrote:
> > This is also a preparation for key read/write/erase functions
> > 
> 
> This is a matter of taste and I am not usually reviewing LUKS patches
> (So don't take me too seriously), but I would prefer not to have "misc"
> patches and instead split things out by individual changes along with a
> nice commit message for each change.
> 
> > * use master key len from the header
> 
> This touches enough lines that you could make it its own patch, I think.
> 
> > * prefer to use crypto params in the QCryptoBlockLUKS
> >   over passing them as function arguments
> 
> I think the same is true here, and highlighting which variables you are
> sticking into state instead of leaving as functional parameters would be
> nice to see without all the other changes.
> 
> > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME
> 
> This can likely be squashed with whichever patch of yours first needs to
> use it, because it's so short.
> 
> > * Add comments to various crypto parameters in the QCryptoBlockLUKS
> > 
> 
> Can probably be squashed with item #2.

I mostly agree with you! I usually write everything as one big patch,
and then split it. 
It takes time but this has the benefit of
much less overhead during the development, and it forces kind of self
review on me while doing the split.

This patch I probably didn't split enough, and I'll do it later when I send the 
next
revision. 

I only had split things to the extent that all the patches are readable and 
reviewable to avoid wasting time,
on stuff that I will have to probably rewrite anyway.

Thanks a lot for the feedback,

Best regards,
Maxim Levitsky




> 
> 
> > Signed-off-by: Maxim Levitsky 
> > ---
> >  crypto/block-luks.c | 213 ++--
> >  1 file changed, 105 insertions(+), 108 deletions(-)
> > 
> > diff --git a/crypto/block-luks.c b/crypto/block-luks.c
> > index 409ab50f20..48213abde7 100644
> > --- a/crypto/block-luks.c
> > +++ b/crypto/block-luks.c
> > @@ -70,6 +70,8 @@ typedef struct QCryptoBlockLUKSKeySlot 
> > QCryptoBlockLUKSKeySlot;
> >  
> >  #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL
> >  
> > +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME 2000
> > +
> >  static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = 
> > {
> >  'L', 'U', 'K', 'S', 0xBA, 0xBE
> >  };
> > @@ -199,13 +201,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct 
> > QCryptoBlockLUKSHeader) != 592);
> >  struct QCryptoBlockLUKS {
> >  QCryptoBlockLUKSHeader header;
> >  
> > -/* Cache parsed versions of what's in header fields,
> > - * as we can't rely on QCryptoBlock.cipher being
> > - * non-NULL */
> > +/* Main encryption algorithm used for encryption*/
> >  QCryptoCipherAlgorithm cipher_alg;
> > +
> > +/* Mode of encryption for the selected encryption algorithm */
> >  QCryptoCipherMode cipher_mode;
> > +
> > +/* Initialization vector generation algorithm */
> >  QCryptoIVGenAlgorithm ivgen_alg;
> > +
> > +/* Hash algorithm used for IV generation*/
> >  QCryptoHashAlgorithm ivgen_hash_alg;
> > +
> > +/*
> > + * Encryption algorithm used for IV generation.
> > + * Usually the same as main encryption algorithm
> > + */
> > +QCryptoCipherAlgorithm ivgen_cipher_alg;
> > +
> > +/* Hash algorithm used in pbkdf2 function */
> >  QCryptoHashAlgorithm hash_alg;
> >  };
> >  
> > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm 
> > cipher,
> >  }
> >  }
> >  
> > +static int masterkeylen(QCryptoBlockLUKS *luks)
> > +{
> > +return luks->header.key_bytes;
> > +}
> > +
> > +
> 
> generally QEMU uses snake_case_names; please spell as "master_key_len".
> 
> >  /*
> >   * Given a key slot, and user password, this will attempt to unlock
> >   * the master encryption key from the key slot.
> > @@ -410,21 +430,15 @@ 
> > qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
> >   */
> >  static int
> >  qcrypto_block_luks_load_key(QCryptoBlock *block,
> > -QCryptoBlockLUKSKeySlot *slot,
> > +uint slot_idx,
> >  const char *password,
> > -QCryptoCipherAlgorithm cipheralg,
> > -QCryptoCipherMode ciphermode,
> > -QCryptoHashAlgorithm hash,
> > -QCryptoIVGenAlgorithm ivalg,
> > -QCryptoCipherAlgorithm ivcipheralg,
> > -QCryptoHashAlgorithm ivhash,
> >  uint8_t *masterkey,
> > -size_t masterkeylen,
> >  QCryptoBlockReadFunc readfunc,
> >  void *opaque,
> >  Error **errp)
> >  {
> >  QCryptoBlockLUKS *luks = block->opaque;
> > +QCryptoBlockLUKSKey

Re: [Qemu-block] [PULL 0/3] Run iotests during "make check"

[Peter Maydell]

On Sat, 17 Aug 2019 at 09:54, Thomas Huth  wrote:
>
>  Hi Peter,
>
> the following changes since commit afd760539308a5524accf964107cdb1d54a059e3:
>
>   Merge remote-tracking branch 
> 'remotes/pmaydell/tags/pull-target-arm-20190816' into staging (2019-08-16 
> 17:21:40 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/huth/qemu.git tags/pull-request-2019-08-17
>
> for you to fetch changes up to 72e031f3b80a421b309ce0d1759b26e428f944db:
>
>   gitlab-ci: Remove qcow2 tests that are handled by "make check" already 
> (2019-08-17 09:06:17 +0200)
>
> 
> - Run the iotest during "make check"
> 
>
> Paolo Bonzini (1):
>   block: fix NetBSD qemu-iotests failure
>
> Thomas Huth (2):
>   tests: Run the iotests during "make check" again
>   gitlab-ci: Remove qcow2 tests that are handled by "make check" already



Applied, thanks.

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

-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management

[Maxim Levitsky]

On Thu, 2019-08-15 at 10:00 -0500, Eric Blake wrote:
> On 8/15/19 9:44 AM, Maxim Levitsky wrote:
> 
> > > > > Does the idea of a union type with a default value for the 
> > > > > discriminator
> > > > > help?  Maybe we have a discriminator which defaults to 'auto', and 
> > > > > add a
> > > > > union branch 'auto':'any'.  During creation, if the "driver":"auto"
> > > > > branch is selected (usually implicitly by omitting "driver", but also
> > > > > possible explicitly), the creation attempt is rejected as invalid
> > > > > regardless of the contents of the remaining 'any'.  But during amend
> > > > > usage, if the 'auto' branch is selected, we then add in the proper
> > > > > "driver":"xyz" and reparse the QAPI object to determine if the 
> > > > > remaining
> > > > > fields in 'any' still meet the specification for the required driver 
> > > > > branch.
> > > > > 
> > > > > This idea may still require some tweaks to the QAPI generator, but 
> > > > > it's
> > > > > the best I can come up with for a way to parse an arbitrary JSON 
> > > > > object
> > > > > with unknown validation, then reparse it again after adding more
> > > > > information that would constrain the parse differently.
> > > > 
> > > > Feels like this would be a lot of code just to allow the client to omit
> > > > passing a value that it knows anyway. If this were a human interface, I
> > > > could understand the desire to make commands less verbose, but for QMP I
> > > > honestly don't see the point when it's not trivial.
> > > 
> > > Seconded.
> > 
> > 
> > But what about my suggestion of adding something like:
> > 
> > { 'union': 'BlockdevAmendOptions',
> > 
> >   'base': {
> >   'node-name': 'str' },
> > 
> >   'discriminator': { 'get_block_driver(node-name)' } ,
> 
> Not worth it. It makes the QAPI generator more complex (to invoke
> arbitrary code instead of a fixed name) just to avoid a little bit of
> complexity in the caller (which is assumed to be a computer, and thus
> shouldn't have a hard time providing a sane 'driver' unconditionally).
> An HMP wrapper around the QMP command can do whatever magic it needs to
> omit driver, but making driver mandatory for QMP is just fine.

All right! I kind of not agree with that, since I think even though QMP is a 
machine language,
it still should be consistent since humans still use it, even if this is humans 
that code some
tool that use it.

I won't argue with you though, let it be like that.

Best regards,
Maxim Levitsky

> 

Re: [Qemu-block] [PULL 00/36] Bitmaps patches

[Peter Maydell]

On Sat, 17 Aug 2019 at 00:13, John Snow  wrote:
>
> The following changes since commit afd760539308a5524accf964107cdb1d54a059e3:
>
>   Merge remote-tracking branch 
> 'remotes/pmaydell/tags/pull-target-arm-20190816' into staging (2019-08-16 
> 17:21:40 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request
>
> for you to fetch changes up to a5f8a60b3eafd5563af48546d5d126d448e62ac5:
>
>   tests/test-hbitmap: test next_zero and _next_dirty_area after truncate 
> (2019-08-16 18:29:43 -0400)
>
> 
> Pull request
>
> Rebase notes:
>
> 011/36:[0003] [FC] 'block/backup: upgrade copy_bitmap to BdrvDirtyBitmap'
> 016/36:[] [-C] 'iotests: Add virtio-scsi device helper'
> 017/36:[0002] [FC] 'iotests: add test 257 for bitmap-mode backups'
> 030/36:[0011] [FC] 'block/backup: teach TOP to never copy unallocated regions'
> 032/36:[0018] [FC] 'iotests/257: test traditional sync modes'
>
> 11: A new hbitmap call was added late in 4.1, changed to
> bdrv_dirty_bitmap_next_zero.
> 16: Context-only (self.has_quit is new context in 040)
> 17: Removed 'auto' to follow upstream trends in iotest fashion
> 30: Handled explicitly on-list with R-B from Max.
> 32: Fix capitalization in test, as mentioned on-list.
>


Applied, thanks.

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

-- PMM

[Qemu-block] [PATCH] iotests: Add more "skip_if_unsupported" statements to the python tests

[Thomas Huth]

The python code already contains a possibility to skip tests if the
corresponding driver is not available in the qemu binary - use it
in more spots to avoid that the tests are failing if the driver has
been disabled.

Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/030 |  3 +++
 tests/qemu-iotests/040 |  2 ++
 tests/qemu-iotests/041 | 14 +-
 tests/qemu-iotests/245 |  2 ++
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 1b69f318c6..18ad24ccdf 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -530,6 +530,7 @@ class TestQuorum(iotests.QMPTestCase):
 children = []
 backing = []
 
+@iotests.skip_if_unsupported(['quorum'])
 def setUp(self):
 opts = ['driver=quorum', 'vote-threshold=2']
 
@@ -552,6 +553,7 @@ class TestQuorum(iotests.QMPTestCase):
 self.vm.add_drive(path = None, opts = ','.join(opts))
 self.vm.launch()
 
+@iotests.skip_if_unsupported(['quorum'])
 def tearDown(self):
 self.vm.shutdown()
 for img in self.children:
@@ -559,6 +561,7 @@ class TestQuorum(iotests.QMPTestCase):
 for img in self.backing:
 os.remove(img)
 
+@iotests.skip_if_unsupported(['quorum'])
 def test_stream_quorum(self):
 if not iotests.supports_quorum():
 return
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index aa0b1847e3..0ec4fb71a9 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -110,6 +110,7 @@ class TestSingleDrive(ImageCommitTestCase):
 self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xab 0 
524288', backing_img).find("verification failed"))
 self.assertEqual(-1, qemu_io('-f', 'raw', '-c', 'read -P 0xef 524288 
524288', backing_img).find("verification failed"))
 
+@iotests.skip_if_unsupported(['throttle'])
 def test_commit_with_filter_and_quit(self):
 result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg')
 self.assert_qmp(result, 'return', {})
@@ -129,6 +130,7 @@ class TestSingleDrive(ImageCommitTestCase):
 self.has_quit = True
 
 # Same as above, but this time we add the filter after starting the job
+@iotests.skip_if_unsupported(['throttle'])
 def test_commit_plus_filter_and_quit(self):
 result = self.vm.qmp('object-add', qom_type='throttle-group', id='tg')
 self.assert_qmp(result, 'return', {})
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 26bf1701eb..f45d20fbe0 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -817,6 +817,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
 image_len = 1 * 1024 * 1024 # MB
 IMAGES = [ quorum_img1, quorum_img2, quorum_img3 ]
 
+@iotests.skip_if_unsupported(['quorum'])
 def setUp(self):
 self.vm = iotests.VM()
 
@@ -841,7 +842,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
 result = self.vm.qmp("blockdev-add", **args)
 self.assert_qmp(result, 'return', {})
 
-
+@iotests.skip_if_unsupported(['quorum'])
 def tearDown(self):
 self.vm.shutdown()
 for i in self.IMAGES + [ quorum_repair_img, quorum_snapshot_file ]:
@@ -851,6 +852,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
 except OSError:
 pass
 
+@iotests.skip_if_unsupported(['quorum'])
 def test_complete(self):
 if not iotests.supports_quorum():
 return
@@ -870,6 +872,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
 self.assertTrue(iotests.compare_images(quorum_img2, quorum_repair_img),
 'target image does not match source after mirroring')
 
+@iotests.skip_if_unsupported(['quorum'])
 def test_cancel(self):
 if not iotests.supports_quorum():
 return
@@ -887,6 +890,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
 self.assert_has_block_node(None, quorum_img3)
 self.vm.shutdown()
 
+@iotests.skip_if_unsupported(['quorum'])
 def test_cancel_after_ready(self):
 if not iotests.supports_quorum():
 return
@@ -906,6 +910,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
 self.assertTrue(iotests.compare_images(quorum_img2, quorum_repair_img),
 'target image does not match source after mirroring')
 
+@iotests.skip_if_unsupported(['quorum'])
 def test_pause(self):
 if not iotests.supports_quorum():
 return
@@ -934,6 +939,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
 self.assertTrue(iotests.compare_images(quorum_img2, quorum_repair_img),
 'target image does not match source after mirroring')
 
+@iotests.skip_if_unsupported(['quorum'])
 def test_medium_not_found(self):
 if not iotests.supports_quorum():
 return
@@ -948,6 +954,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
  target=q

Re: [Qemu-block] [Qemu-devel] [PATCH] iotests: Check for enabled drivers before testing them

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190819075348.4078-1-th...@redhat.com/



Hi,

This series failed build test on s390x host. Please find the details below.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e

echo
echo "=== ENV ==="
env

echo
echo "=== PACKAGES ==="
rpm -qa

echo
echo "=== UNAME ==="
uname -a

CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

  CC  mips-softmmu/trace/control-target.o
  CC  mips-softmmu/trace/generated-helpers.o
  LINKmips-softmmu/qemu-system-mips
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:209: qemu-system-mipsel] Error 1
make: *** [Makefile:472: mipsel-softmmu/all] Error 2
make: *** Waiting for unfinished jobs
---
  CC  hppa-softmmu/hw/block/vhost-user-blk.o
  CC  microblaze-softmmu/balloon.o
  CC  hppa-softmmu/hw/block/dataplane/virtio-blk.o
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:209: qemu-system-mips] Error 1
make: *** [Makefile:472: mips-softmmu/all] Error 2
  CC  hppa-softmmu/hw/char/virtio-serial-bus.o
---
  CC  microblaze-softmmu/trace/generated-helpers.o
  LINKmicroblaze-softmmu/qemu-system-microblaze
  LINKhppa-softmmu/qemu-system-hppa
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:209: qemu-system-microblaze] Error 1
make: *** [Makefile:472: microblaze-softmmu/all] Error 2
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:209: qemu-system-hppa] Error 1
make: *** [Makefile:472: hppa-softmmu/all] Error 2
=== OUTPUT END ===


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

[Qemu-block] [PATCH] iotests: Check for enabled drivers before testing them

[Thomas Huth]

It is possible to enable only a subset of the block drivers with the
"--block-drv-rw-whitelist" option of the "configure" script. All other
drivers are marked as unusable (or only included as read-only with the
"--block-drv-ro-whitelist" option). If an iotest is now using such a
disabled block driver, it is failing - which is bad, since at least the
tests in the "auto" group should be able to deal with this situation.
Thus let's introduce a "_require_drivers" function that can be used by
the shell tests to check for the availability of certain drivers first,
and marks the test as "not run" if one of the drivers is missing.

Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/071   |  1 +
 tests/qemu-iotests/081   |  1 +
 tests/qemu-iotests/099   |  1 +
 tests/qemu-iotests/184   |  1 +
 tests/qemu-iotests/common.rc | 13 +
 5 files changed, 17 insertions(+)

diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
index 1cca9233d0..fab52b 100755
--- a/tests/qemu-iotests/071
+++ b/tests/qemu-iotests/071
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto file
+_require_drivers blkdebug blkverify
 
 do_run_qemu()
 {
diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index c418bab093..1695781bc0 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt raw
 _supported_proto file
 _supported_os Linux
+_require_drivers quorum
 
 do_run_qemu()
 {
diff --git a/tests/qemu-iotests/099 b/tests/qemu-iotests/099
index ae02f27afe..c3cf66798a 100755
--- a/tests/qemu-iotests/099
+++ b/tests/qemu-iotests/099
@@ -42,6 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow qcow2 qed vdi vhdx vmdk vpc
 _supported_proto file
 _supported_os Linux
+_require_drivers blkdebug blkverify
 _unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" 
\
 "subformat=twoGbMaxExtentSparse"
 
diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184
index cb0c181228..33dd8d2a4f 100755
--- a/tests/qemu-iotests/184
+++ b/tests/qemu-iotests/184
@@ -33,6 +33,7 @@ trap "exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_os Linux
+_require_drivers throttle
 
 do_run_qemu()
 {
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 5502c3da2f..7d4e68846f 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -520,5 +520,18 @@ _require_command()
 [ -x "$c" ] || _notrun "$1 utility required, skipped this test"
 }
 
+# Check that a set of drivers has been whitelisted in the QEMU binary
+#
+_require_drivers()
+{
+available=$($QEMU -drive format=help | grep 'Supported formats:')
+for driver
+do
+if ! echo "$available" | grep -q "$driver"; then
+_notrun "$driver not available"
+fi
+done
+}
+
 # make sure this script returns success
 true
-- 
2.18.1