Re: [Qemu-block] [Qemu-devel] [PATCH v8 3/4] qemu-img: introduce --target-image-opts for 'convert' command

[Fam Zheng]

On Tue, 05/09 10:48, Daniel P. Berrange wrote:
> The '--image-opts' flag indicates whether the source filename
> includes options. The target filename has to remain in the
> plain filename format though, since it needs to be passed to
> bdrv_create().  When using --skip-create though, it would be
> possible to use image-opts syntax. This adds --target-image-opts
> to indicate that the target filename includes options. Currently
> this mandates use of the --skip-create flag too.
> 
> Signed-off-by: Daniel P. Berrange 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH] block: curl: Allow passing cookies via QCryptoSecret

[Manos Pitsidianakis]

On Thu, May 04, 2017 at 04:00:06PM +0200, Peter Krempa wrote:

+cookie_secret = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE_SECRET);
+
+if (cookie && cookie_secret) {
+error_setg(errp,
+   "curl driver cannot handle both cookie and cookie secret");
+goto out_noclean;
+}
+
+if (cookie_secret) {
+s->cookie = qcrypto_secret_lookup_as_utf8(cookie_secret, errp);
+if (!s->cookie) {
+goto out_noclean;
+}
+} else {
+s->cookie = g_strdup(cookie);
+}


There's no check here for if both cookie and cookie_secret are NULL. 

Re: [Qemu-block] [Qemu-devel] [PATCH] block: curl: Allow passing cookies via QCryptoSecret

[Manos Pitsidianakis]

On Tue, May 09, 2017 at 02:52:38PM -0500, Eric Blake wrote:

On 05/09/2017 02:43 PM, Manos Pitsidianakis wrote:

On Thu, May 04, 2017 at 04:00:06PM +0200, Peter Krempa wrote:

+cookie_secret = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE_SECRET);
+
+if (cookie && cookie_secret) {
+error_setg(errp,
+   "curl driver cannot handle both cookie and cookie
secret");
+goto out_noclean;
+}
+
+if (cookie_secret) {
+s->cookie = qcrypto_secret_lookup_as_utf8(cookie_secret, errp);
+if (!s->cookie) {
+goto out_noclean;
+}
+} else {
+s->cookie = g_strdup(cookie);
+}


There's no check here for if both cookie and cookie_secret are NULL.


Is that a problem?  s->cookie ends up as NULL (thanks to g_strdup()
semantics), which merely means there's no cookie to be sent after all.


Ah yes, g_strdup(NULL) returns NULL. Apologies for the noise.



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






signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH] block: curl: Allow passing cookies via QCryptoSecret

[Eric Blake]

On 05/09/2017 02:43 PM, Manos Pitsidianakis wrote:
> On Thu, May 04, 2017 at 04:00:06PM +0200, Peter Krempa wrote:
>> +cookie_secret = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE_SECRET);
>> +
>> +if (cookie && cookie_secret) {
>> +error_setg(errp,
>> +   "curl driver cannot handle both cookie and cookie
>> secret");
>> +goto out_noclean;
>> +}
>> +
>> +if (cookie_secret) {
>> +s->cookie = qcrypto_secret_lookup_as_utf8(cookie_secret, errp);
>> +if (!s->cookie) {
>> +goto out_noclean;
>> +}
>> +} else {
>> +s->cookie = g_strdup(cookie);
>> +}
> 
> There's no check here for if both cookie and cookie_secret are NULL.

Is that a problem?  s->cookie ends up as NULL (thanks to g_strdup()
semantics), which merely means there's no cookie to be sent after all.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2] qemu-img: Check for backing image if specified during create

[John Snow]

On 05/09/2017 02:19 PM, Eric Blake wrote:
> On 05/09/2017 01:17 PM, Eric Blake wrote:
>> On 05/09/2017 01:09 PM, John Snow wrote:
>>> Or, rather, force the open of a backing image if one was specified
>>> for creation. Using a similar -unsafe option as rebase, allow qemu-img
>>> to ignore the backing file validation if possible.
>>>
> 
>>> +++ b/qemu-img.c
>>> @@ -145,9 +145,11 @@ static void QEMU_NORETURN help(void)
>>> "  'snapshot_id_or_name' is deprecated, use 'snapshot_param'\n"
>>> "instead\n"
>>> "  '-c' indicates that target image must be compressed (qcow 
>>> format only)\n"
>>> -   "  '-u' enables unsafe rebasing. It is assumed that old and new 
>>> backing file\n"
>>> -   "   match exactly. The image doesn't need a working backing 
>>> file before\n"
>>> -   "   rebasing in this case (useful for renaming the backing 
>>> file)\n"
>>> +   "  '-u' allows unsafe backing chains. For rebasing, it is 
>>> assumed that old and new\n"
>>> +   "   backing file match exactly. The image doesn't need a 
>>> working backing file\n"
>>> +   "   before rebasing in this case (useful for renaming the 
>>> backing file)\n"
> 
> Also, a '.' to end this sentence would be nice.
> 

d'oh.

I can respin if that's more convenient, as I am assuming Max or Kevin
will have something to say.

If not, either edit it if that's easier, or ask me to re-spin.

>>> +   "   For image creation, allow creating without attempting 
>>> to"
>>
>> Missing \n
>>
>> With that fixed,
>> Reviewed-by: Eric Blake 
>>
> 

Re: [Qemu-block] [PATCH v2] qemu-img: Check for backing image if specified during create

[Eric Blake]

On 05/09/2017 01:17 PM, Eric Blake wrote:
> On 05/09/2017 01:09 PM, John Snow wrote:
>> Or, rather, force the open of a backing image if one was specified
>> for creation. Using a similar -unsafe option as rebase, allow qemu-img
>> to ignore the backing file validation if possible.
>>

>> +++ b/qemu-img.c
>> @@ -145,9 +145,11 @@ static void QEMU_NORETURN help(void)
>> "  'snapshot_id_or_name' is deprecated, use 'snapshot_param'\n"
>> "instead\n"
>> "  '-c' indicates that target image must be compressed (qcow 
>> format only)\n"
>> -   "  '-u' enables unsafe rebasing. It is assumed that old and new 
>> backing file\n"
>> -   "   match exactly. The image doesn't need a working backing 
>> file before\n"
>> -   "   rebasing in this case (useful for renaming the backing 
>> file)\n"
>> +   "  '-u' allows unsafe backing chains. For rebasing, it is 
>> assumed that old and new\n"
>> +   "   backing file match exactly. The image doesn't need a 
>> working backing file\n"
>> +   "   before rebasing in this case (useful for renaming the 
>> backing file)\n"

Also, a '.' to end this sentence would be nice.

>> +   "   For image creation, allow creating without attempting to"
> 
> Missing \n
> 
> With that fixed,
> Reviewed-by: Eric Blake 
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2] qemu-img: Check for backing image if specified during create

[Eric Blake]

On 05/09/2017 01:09 PM, John Snow wrote:
> Or, rather, force the open of a backing image if one was specified
> for creation. Using a similar -unsafe option as rebase, allow qemu-img
> to ignore the backing file validation if possible.
> 
> It may not always be possible, as in the existing case when a filesize
> for the new image was not specified.
> 
> This is accomplished by shifting around the conditionals in
> bdrv_img_create, such that a backing file is always opened unless we
> provide BDRV_O_NO_BACKING. qemu-img is adjusted to pass this new flag
> when -u is provided to create.
> 
> Sorry for the heinous looking diffstat, but it's mostly whitespace.
> 
> Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1213786
> 
> Signed-off-by: John Snow 
> ---
> 
> v2: Rebased for 2.10
> Corrected some of my less cromulent grammar
> 

> +++ b/qemu-img.c
> @@ -145,9 +145,11 @@ static void QEMU_NORETURN help(void)
> "  'snapshot_id_or_name' is deprecated, use 'snapshot_param'\n"
> "instead\n"
> "  '-c' indicates that target image must be compressed (qcow 
> format only)\n"
> -   "  '-u' enables unsafe rebasing. It is assumed that old and new 
> backing file\n"
> -   "   match exactly. The image doesn't need a working backing 
> file before\n"
> -   "   rebasing in this case (useful for renaming the backing 
> file)\n"
> +   "  '-u' allows unsafe backing chains. For rebasing, it is assumed 
> that old and new\n"
> +   "   backing file match exactly. The image doesn't need a 
> working backing file\n"
> +   "   before rebasing in this case (useful for renaming the 
> backing file)\n"
> +   "   For image creation, allow creating without attempting to"

Missing \n

With that fixed,
Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v2] qemu-img: Check for backing image if specified during create

[John Snow]

Or, rather, force the open of a backing image if one was specified
for creation. Using a similar -unsafe option as rebase, allow qemu-img
to ignore the backing file validation if possible.

It may not always be possible, as in the existing case when a filesize
for the new image was not specified.

This is accomplished by shifting around the conditionals in
bdrv_img_create, such that a backing file is always opened unless we
provide BDRV_O_NO_BACKING. qemu-img is adjusted to pass this new flag
when -u is provided to create.

Sorry for the heinous looking diffstat, but it's mostly whitespace.

Inspired by: https://bugzilla.redhat.com/show_bug.cgi?id=1213786

Signed-off-by: John Snow 
---

v2: Rebased for 2.10
Corrected some of my less cromulent grammar


 block.c| 75 +++---
 qemu-img-cmds.hx   |  4 +--
 qemu-img.c | 16 ++
 tests/qemu-iotests/082 |  4 +--
 tests/qemu-iotests/082.out |  4 +--
 5 files changed, 55 insertions(+), 48 deletions(-)

diff --git a/block.c b/block.c
index 6c6bb3e..ca3e195 100644
--- a/block.c
+++ b/block.c
@@ -4284,38 +4284,38 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 // The size for the image must always be specified, with one exception:
 // If we are using a backing file, we can obtain the size from there
 size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
-if (size == -1) {
-if (backing_file) {
-BlockDriverState *bs;
-char *full_backing = g_new0(char, PATH_MAX);
-int64_t size;
-int back_flags;
-QDict *backing_options = NULL;
-
-bdrv_get_full_backing_filename_from_filename(filename, 
backing_file,
- full_backing, 
PATH_MAX,
- &local_err);
-if (local_err) {
-g_free(full_backing);
-goto out;
-}
-
-/* backing files always opened read-only */
-back_flags = flags;
-back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
-
-if (backing_fmt) {
-backing_options = qdict_new();
-qdict_put(backing_options, "driver",
-  qstring_from_str(backing_fmt));
-}
-
-bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
-   &local_err);
+if (backing_file && !(flags & BDRV_O_NO_BACKING)) {
+BlockDriverState *bs;
+char *full_backing = g_new0(char, PATH_MAX);
+int back_flags;
+QDict *backing_options = NULL;
+
+bdrv_get_full_backing_filename_from_filename(filename, backing_file,
+ full_backing, PATH_MAX,
+ &local_err);
+if (local_err) {
 g_free(full_backing);
-if (!bs) {
-goto out;
-}
+goto out;
+}
+
+/* backing files always opened read-only */
+back_flags = flags;
+back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
+
+if (backing_fmt) {
+backing_options = qdict_new();
+qdict_put(backing_options, "driver",
+  qstring_from_str(backing_fmt));
+}
+
+bs = bdrv_open(full_backing, NULL, backing_options, back_flags,
+   &local_err);
+g_free(full_backing);
+if (!bs) {
+goto out;
+}
+
+if (size == -1) {
 size = bdrv_getlength(bs);
 if (size < 0) {
 error_setg_errno(errp, -size, "Could not get size of '%s'",
@@ -4323,14 +4323,15 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 bdrv_unref(bs);
 goto out;
 }
-
 qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);
-
-bdrv_unref(bs);
-} else {
-error_setg(errp, "Image creation needs a size parameter");
-goto out;
 }
+
+bdrv_unref(bs);
+}
+
+if (size == -1) {
+error_setg(errp, "Image creation needs a size parameter");
+goto out;
 }
 
 if (!quiet) {
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index bf4ce59..1d230c6 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -22,9 +22,9 @@ STEXI
 ETEXI
 
 DEF("create", img_create,
-"create [-q] [--object objectdef] [-f fmt] [-b backing_file] [-F 
backing_fmt] [-o options] filename [size]")
+"create [-q] [--object objectdef] [-f fmt] [-b backing_file] [-F 
backing_fmt] [-u] [-o options] filename [size]")
 STEXI
-@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b 
@var{backing_file}] [-F @var{backing_fmt}] [-o @var{options}] @var{filename} 
[@var{size}

Re: [Qemu-block] [Qemu-devel] [PATCH] mirror: add sync mode incremental to drive-mirror and blockdev-mirror

[Stefan Hajnoczi]

On Mon, May 08, 2017 at 05:07:18PM -0400, John Snow wrote:
> 
> 
> On 05/08/2017 05:02 PM, Denis V. Lunev wrote:
> > On 05/08/2017 10:35 PM, Stefan Hajnoczi wrote:
> >> On Thu, May 04, 2017 at 12:54:40PM +0200, Daniel Kucera wrote:
> >>
> >> Seems like a logical extension along the same lines as the backup block
> >> job's dirty bitmap sync mode.
> >>
> >>> parameter bitmap chooses existing dirtymap instead of newly created
> >>> in mirror_start_job
> >>>
> >>> Signed-off-by: Daniel Kucera 
> > Can you pls describe the use case pls in a bit more details.
> > 
> > For now this could be a bit strange:
> > - dirty bitmap, which can be found via bdrv_create_dirty_bitmap
> >   could be read-only or read-write, i.e. being modified by writes
> >   or be read-only, which should not be modified. Thus adding
> >   r/o bitmap to the mirror could result in interesting things.
> > 
> 
> This patch as it was submitted does not put the bitmap into a read-only
> mode; it leaves it RW and modifies it as it processes the mirror command.
> 
> Though you do raise a good point; this bitmap is now in-use by a job and
> should not be allowed to be deleted by the user, but our existing
> mechanism treats a locked bitmap as one that is also in R/O mode. This
> would be a different use case.
> 
> > Minimally we should prohibit usage of r/o bitmaps this way.
> > 
> > So, why to use mirror, not backup for the case?
> > 
> 
> My guess is for pivot semantics.

Daniel posted his workflow in a previous revision of this series:

He is doing a variation on non-shared storage migration with the mirror
block job, but using the ZFS send operation to transfer the initial copy
of the disk.

Once ZFS send completes it's necessary to transfer all the blocks that
were dirtied while the transfer was taking place.

1. Create dirty bitmap and start tracking dirty blocks in QEMU.
2. Snapshot and send ZFS volume.
3. mirror sync=bitmap after ZFS send completes.
4. Live migrate.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v5 2/5] iotests: fix remainining tests to work with LUKS

[Eric Blake]

On 05/09/2017 12:33 PM, Daniel P. Berrange wrote:
> The tests 033, 140, 145 and 157 were all broken
> when run with LUKS, since they did not correctly use
> the required image opts args syntax to specify the
> decryption secret. Further, the 120 test simply does
> not make sense to run with luks, as the scenario
> exercised is not relevant.
> 
> The test 181 was broken when run with LUKS because
> it didn't take account of fact that $TEST_IMG was
> already in image opts syntax. The launch_qemu
> helper also didn't register the secret object
> providing the LUKS password.
> 
> Signed-off-by: Daniel P. Berrange 
> ---

> +++ b/tests/qemu-iotests/145
> @@ -43,8 +43,23 @@ _supported_proto generic
>  _supported_os Linux
>  
>  _make_test_img 1M
> -echo quit | $QEMU -nographic -hda "$TEST_IMG" -incoming 'exec:true' 
> -snapshot -serial none -monitor stdio |
> -_filter_qemu | _filter_hmp
> +
> +if test "$IMGOPTSSYNTAX" = "true"
> +then
> +SYSEMU_DRIVE_ARG=if=none,$TEST_IMG
> +SYSEMU_EXTRA_ARGS=""
> +if [ -n "$IMGKEYSECRET" ]; then
> + SECRET_ARG="secret,id=keysec0,data=$IMGKEYSECRET"
> + SYSEMU_EXTRA_ARGS="-object $SECRET_ARG"
> +fi
> +else
> +SYSEMU_DRIVE_ARG=if=none,file="$TEST_IMG",driver=$IMGFMT
> +SYSEMU_EXTRA_ARGS=""
> +fi
> +
> +echo quit | $QEMU -nographic $SYSEMU_EXTRA_ARGS  -drive $SYSEMU_DRIVE_ARG \

2 spaces before -drive looks a bit odd, but not a showstopper.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v5 5/5] iotests: chown LUKS device before qemu-io launches

[Daniel P. Berrange]

On some distros, whenever you close a block device file
descriptor there is a udev rule that resets the file
permissions. This can race with the test script when
we run qemu-io multiple times against the same block
device. Occasionally the second qemu-io invocation
will find udev has reset the permissions causing failure.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/149 |  13 +-
 tests/qemu-iotests/149.out | 344 ++---
 2 files changed, 178 insertions(+), 179 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 5faf585..bc628ce 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -23,6 +23,7 @@
 import subprocess
 import os
 import os.path
+import time
 
 import base64
 
@@ -186,7 +187,7 @@ def chown(config):
 msg = proc.communicate()[0]
 
 if proc.returncode != 0:
-raise Exception("Cannot change owner on %s" % path)
+raise Exception(msg)
 
 
 def cryptsetup_open(config):
@@ -271,6 +272,8 @@ def qemu_io_image_args(config, dev=False):
 def qemu_io_write_pattern(config, pattern, offset_mb, size_mb, dev=False):
 """Write a pattern of data to a LUKS image or device"""
 
+if dev:
+chown(config)
 args = ["-c", "write -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
 args.extend(qemu_io_image_args(config, dev))
 iotests.log("qemu-io " + " ".join(args), filters=[iotests.filter_test_dir])
@@ -281,6 +284,8 @@ def qemu_io_write_pattern(config, pattern, offset_mb, 
size_mb, dev=False):
 def qemu_io_read_pattern(config, pattern, offset_mb, size_mb, dev=False):
 """Read a pattern of data to a LUKS image or device"""
 
+if dev:
+chown(config)
 args = ["-c", "read -P 0x%x %dM %dM" % (pattern, offset_mb, size_mb)]
 args.extend(qemu_io_image_args(config, dev))
 iotests.log("qemu-io " + " ".join(args), filters=[iotests.filter_test_dir])
@@ -331,9 +336,6 @@ def test_once(config, qemu_img=False):
 cryptsetup_open(config)
 
 try:
-iotests.log("# Set dev owner")
-chown(config)
-
 iotests.log("# Write test pattern 0xa7")
 qemu_io_write_pattern(config, 0xa7, lowOffsetMB, 10, dev=True)
 iotests.log("# Write test pattern 0x13")
@@ -365,9 +367,6 @@ def test_once(config, qemu_img=False):
 cryptsetup_open(config)
 
 try:
-iotests.log("# Set dev owner")
-chown(config)
-
 iotests.log("# Read test pattern 0x91")
 qemu_io_read_pattern(config, 0x91, lowOffsetMB, 10, dev=True)
 iotests.log("# Read test pattern 0x5e")
diff --git a/tests/qemu-iotests/149.out b/tests/qemu-iotests/149.out
index 2f0454f..5dea00b 100644
--- a/tests/qemu-iotests/149.out
+++ b/tests/qemu-iotests/149.out
@@ -5,14 +5,14 @@ truncate TEST_DIR/luks-aes-256-xts-plain64-sha1.img --size 
4194304MB
 sudo cryptsetup -q -v luksFormat --cipher aes-xts-plain64 --key-size 512 
--hash sha1 --key-slot 0 --key-file - --iter-time 10 
TEST_DIR/luks-aes-256-xts-plain64-sha1.img
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-sha1.img 
qiotest-145-aes-256-xts-plain64-sha1
-# Set dev owner
-sudo chown UID:GID /dev/mapper/qiotest-145-aes-256-xts-plain64-sha1
 # Write test pattern 0xa7
+sudo chown UID:GID /dev/mapper/qiotest-145-aes-256-xts-plain64-sha1
 qemu-io -c write -P 0xa7 100M 10M --image-opts 
driver=file,filename=/dev/mapper/qiotest-145-aes-256-xts-plain64-sha1
 wrote 10485760/10485760 bytes at offset 104857600
 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 # Write test pattern 0x13
+sudo chown UID:GID /dev/mapper/qiotest-145-aes-256-xts-plain64-sha1
 qemu-io -c write -P 0x13 3145728M 10M --image-opts 
driver=file,filename=/dev/mapper/qiotest-145-aes-256-xts-plain64-sha1
 wrote 10485760/10485760 bytes at offset 3298534883328
 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -41,14 +41,14 @@ wrote 10485760/10485760 bytes at offset 3298534883328
 
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-sha1.img 
qiotest-145-aes-256-xts-plain64-sha1
-# Set dev owner
-sudo chown UID:GID /dev/mapper/qiotest-145-aes-256-xts-plain64-sha1
 # Read test pattern 0x91
+sudo chown UID:GID /dev/mapper/qiotest-145-aes-256-xts-plain64-sha1
 qemu-io -c read -P 0x91 100M 10M --image-opts 
driver=file,filename=/dev/mapper/qiotest-145-aes-256-xts-plain64-sha1
 read 10485760/10485760 bytes at offset 104857600
 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 # Read test pattern 0x5e
+sudo chown UID:GID /dev/mapper/qiotest-145-aes-256-xts-plain64-sha1
 qemu-io -c read -P 0x5e 3145728M 10M --image-opts 
driver=file,filename=/dev/mapper/qiotest-145-aes-256-xts-plain64-sha1
 read 10485760/10485760 bytes at offset 3298534883328
 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -65,14 +65,14 @@ Formatting 'TEST_DIR/luks-aes-256-xts-plain64-sha1.img', 
fmt=luks s

[Qemu-block] [PATCH v5 3/5] iotests: reduce PBKDF iterations when testing LUKS

[Daniel P. Berrange]

By default the PBKDF algorithm used with LUKS is tuned
based on the number of iterations to produce 1 second
of running time. This makes running the I/O test with
the LUKS format orders of magnitude slower than with
qcow2/raw formats.

When creating LUKS images, set the iteration time to
a 10ms to reduce the time overhead for LUKS, since
security does not matter in I/O tests.

Previously a full 'check -luks' would take

  $ time ./check -luks
  Passed all 22 tests

  real  23m9.988s
  user  21m46.223s
  sys   0m22.841s

Now it takes

  $ time ./check -luks
  Passed all 22 tests

  real  4m39.235s
  user  3m29.590s
  sys   0m24.234s

Still slow compared to qcow2/raw, but much improved
none the less.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/149   |   3 +
 tests/qemu-iotests/149.out   | 118 +++
 tests/qemu-iotests/common.filter |   3 +-
 tests/qemu-iotests/common.rc |   3 +
 4 files changed, 67 insertions(+), 60 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 8407251..f62e618 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -136,6 +136,7 @@ def cryptsetup_add_password(config, slot):
 args = ["luksAddKey", config.image_path(),
 "--key-slot", slot,
 "--key-file", "-",
+"--iter-time", "10",
 pwfile]
 
 cryptsetup(args, password)
@@ -164,6 +165,7 @@ def cryptsetup_format(config):
 args.extend(["--hash", config.hash])
 args.extend(["--key-slot", slot])
 args.extend(["--key-file", "-"])
+args.extend(["--iter-time", "10"])
 args.append(config.image_path())
 
 cryptsetup(args, password)
@@ -230,6 +232,7 @@ def qemu_img_create(config, size_mb):
 
 opts = [
 "key-secret=sec0",
+"iter-time=10",
 "cipher-alg=%s-%d" % (config.cipher, config.keylen),
 "cipher-mode=%s" % config.mode,
 "ivgen-alg=%s" % config.ivgen,
diff --git a/tests/qemu-iotests/149.out b/tests/qemu-iotests/149.out
index 90b5b55..b18c4e4 100644
--- a/tests/qemu-iotests/149.out
+++ b/tests/qemu-iotests/149.out
@@ -2,7 +2,7 @@
 # Create image
 truncate TEST_DIR/luks-aes-256-xts-plain64-sha1.img --size 4194304MB
 # Format image
-sudo cryptsetup -q -v luksFormat --cipher aes-xts-plain64 --key-size 512 
--hash sha1 --key-slot 0 --key-file - TEST_DIR/luks-aes-256-xts-plain64-sha1.img
+sudo cryptsetup -q -v luksFormat --cipher aes-xts-plain64 --key-size 512 
--hash sha1 --key-slot 0 --key-file - --iter-time 10 
TEST_DIR/luks-aes-256-xts-plain64-sha1.img
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-sha1.img 
qiotest-145-aes-256-xts-plain64-sha1
 # Set dev owner
@@ -60,8 +60,8 @@ unlink TEST_DIR/luks-aes-256-xts-plain64-sha1.img
 
 # = qemu-img aes-256-xts-plain64-sha1 =
 # Create image
-qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o 
key-secret=sec0,cipher-alg=aes-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1
 TEST_DIR/luks-aes-256-xts-plain64-sha1.img 4194304M
-Formatting 'TEST_DIR/luks-aes-256-xts-plain64-sha1.img', fmt=luks 
size=4398046511104 key-secret=sec0 cipher-alg=aes-256 cipher-mode=xts 
ivgen-alg=plain64 hash-alg=sha1
+qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o 
key-secret=sec0,iter-time=10,cipher-alg=aes-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1
 TEST_DIR/luks-aes-256-xts-plain64-sha1.img 4194304M
+Formatting 'TEST_DIR/luks-aes-256-xts-plain64-sha1.img', fmt=luks 
size=4398046511104 key-secret=sec0 cipher-alg=aes-256 cipher-mode=xts 
ivgen-alg=plain64 hash-alg=sha1 iter-time=10
 
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-sha1.img 
qiotest-145-aes-256-xts-plain64-sha1
@@ -122,7 +122,7 @@ unlink TEST_DIR/luks-aes-256-xts-plain64-sha1.img
 # Create image
 truncate TEST_DIR/luks-twofish-256-xts-plain64-sha1.img --size 4194304MB
 # Format image
-sudo cryptsetup -q -v luksFormat --cipher twofish-xts-plain64 --key-size 512 
--hash sha1 --key-slot 0 --key-file - 
TEST_DIR/luks-twofish-256-xts-plain64-sha1.img
+sudo cryptsetup -q -v luksFormat --cipher twofish-xts-plain64 --key-size 512 
--hash sha1 --key-slot 0 --key-file - --iter-time 10 
TEST_DIR/luks-twofish-256-xts-plain64-sha1.img
 # Open dev
 sudo cryptsetup -q -v luksOpen TEST_DIR/luks-twofish-256-xts-plain64-sha1.img 
qiotest-145-twofish-256-xts-plain64-sha1
 # Set dev owner
@@ -180,8 +180,8 @@ unlink TEST_DIR/luks-twofish-256-xts-plain64-sha1.img
 
 # = qemu-img twofish-256-xts-plain64-sha1 =
 # Create image
-qemu-img create -f luks --object secret,id=sec0,data=MTIzNDU2,format=base64 -o 
key-secret=sec0,cipher-alg=twofish-256,cipher-mode=xts,ivgen-alg=plain64,hash-alg=sha1
 TEST_DIR/luks-twofish-256-xts-plain64-sha1.img 4194304M
-Formatting 'TEST_DIR/luks-twofish-256-xts-plain64-sha1.img', fmt=

[Qemu-block] [PATCH v5 2/5] iotests: fix remainining tests to work with LUKS

[Daniel P. Berrange]

The tests 033, 140, 145 and 157 were all broken
when run with LUKS, since they did not correctly use
the required image opts args syntax to specify the
decryption secret. Further, the 120 test simply does
not make sense to run with luks, as the scenario
exercised is not relevant.

The test 181 was broken when run with LUKS because
it didn't take account of fact that $TEST_IMG was
already in image opts syntax. The launch_qemu
helper also didn't register the secret object
providing the LUKS password.

Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/033 | 12 ++--
 tests/qemu-iotests/120 |  1 +
 tests/qemu-iotests/140 | 11 ++-
 tests/qemu-iotests/145 | 19 +--
 tests/qemu-iotests/157 | 17 ++---
 tests/qemu-iotests/157.out | 16 
 tests/qemu-iotests/174 |  2 +-
 tests/qemu-iotests/181 | 21 -
 tests/qemu-iotests/common.qemu |  9 +++--
 9 files changed, 84 insertions(+), 24 deletions(-)

diff --git a/tests/qemu-iotests/033 b/tests/qemu-iotests/033
index 16edcf2..2cdfd13 100755
--- a/tests/qemu-iotests/033
+++ b/tests/qemu-iotests/033
@@ -50,10 +50,18 @@ do_test()
local align=$1
local iocmd=$2
local img=$3
+   if [ "$IMGOPTSSYNTAX" = "true" ]
+   then
+   IO_OPEN_ARG="$img"
+   IO_EXTRA_ARGS="--image-opts"
+   else
+   IO_OPEN_ARG="-o driver=$IMGFMT,file.align=$align blkdebug::$img"
+   IO_EXTRA_ARGS=""
+   fi
{
-   echo "open -o driver=$IMGFMT,file.align=$align blkdebug::$img"
+   echo "open $IO_OPEN_ARG"
echo $iocmd
-   } | $QEMU_IO
+   } | $QEMU_IO $IO_EXTRA_ARGS
 }
 
 for write_zero_cmd in "write -z" "aio_write -z"; do
diff --git a/tests/qemu-iotests/120 b/tests/qemu-iotests/120
index 4f88a67..f40b97d 100755
--- a/tests/qemu-iotests/120
+++ b/tests/qemu-iotests/120
@@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt generic
 _supported_proto file
 _supported_os Linux
+_unsupported_fmt luks
 
 _make_test_img 64M
 
diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
index 8c80a5a..0a2105c 100755
--- a/tests/qemu-iotests/140
+++ b/tests/qemu-iotests/140
@@ -52,8 +52,17 @@ _make_test_img 64k
 
 $QEMU_IO -c 'write -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io
 
+if test "$IMGOPTSSYNTAX" = "true"
+then
+SYSEMU_DRIVE_ARG=if=none,media=cdrom,id=drv,$TEST_IMG
+SYSEMU_EXTRA_ARGS=""
+else
+SYSEMU_DRIVE_ARG=if=none,media=cdrom,id=drv,file="$TEST_IMG",driver=$IMGFMT
+SYSEMU_EXTRA_ARGS=""
+fi
+
 keep_stderr=y \
-_launch_qemu -drive if=none,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT 
\
+_launch_qemu $SYSEMU_EXTRA_ARGS -drive $SYSEMU_DRIVE_ARG \
 2> >(_filter_nbd)
 
 _send_qemu_cmd $QEMU_HANDLE \
diff --git a/tests/qemu-iotests/145 b/tests/qemu-iotests/145
index e6c6bc4..9cfa940 100755
--- a/tests/qemu-iotests/145
+++ b/tests/qemu-iotests/145
@@ -43,8 +43,23 @@ _supported_proto generic
 _supported_os Linux
 
 _make_test_img 1M
-echo quit | $QEMU -nographic -hda "$TEST_IMG" -incoming 'exec:true' -snapshot 
-serial none -monitor stdio |
-_filter_qemu | _filter_hmp
+
+if test "$IMGOPTSSYNTAX" = "true"
+then
+SYSEMU_DRIVE_ARG=if=none,$TEST_IMG
+SYSEMU_EXTRA_ARGS=""
+if [ -n "$IMGKEYSECRET" ]; then
+   SECRET_ARG="secret,id=keysec0,data=$IMGKEYSECRET"
+   SYSEMU_EXTRA_ARGS="-object $SECRET_ARG"
+fi
+else
+SYSEMU_DRIVE_ARG=if=none,file="$TEST_IMG",driver=$IMGFMT
+SYSEMU_EXTRA_ARGS=""
+fi
+
+echo quit | $QEMU -nographic $SYSEMU_EXTRA_ARGS  -drive $SYSEMU_DRIVE_ARG \
+  -incoming 'exec:true' -snapshot -serial none -monitor stdio \
+  | _filter_qemu | _filter_hmp
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/157 b/tests/qemu-iotests/157
index 8d939cb..f5d22fa 100755
--- a/tests/qemu-iotests/157
+++ b/tests/qemu-iotests/157
@@ -43,7 +43,6 @@ _supported_os Linux
 
 function do_run_qemu()
 {
-echo Testing: "$@"
 (
 if ! test -t 0; then
 while read cmd; do
@@ -63,7 +62,18 @@ function run_qemu()
 
 
 size=128M
-drive="if=none,file=$TEST_IMG,driver=$IMGFMT"
+if test "$IMGOPTSSYNTAX" = "true"
+then
+SYSEMU_DRIVE_ARG=if=none,$TEST_IMG
+SYSEMU_EXTRA_ARGS=""
+if [ -n "$IMGKEYSECRET" ]; then
+   SECRET_ARG="secret,id=keysec0,data=$IMGKEYSECRET"
+   SYSEMU_EXTRA_ARGS="-object $SECRET_ARG"
+fi
+else
+SYSEMU_DRIVE_ARG=if=none,file="$TEST_IMG",driver=$IMGFMT
+SYSEMU_EXTRA_ARGS=""
+fi
 
 _make_test_img $size
 
@@ -76,8 +86,9 @@ echo
 
 for cache in "writeback" "writethrough"; do
 for wce in "" ",write-cache=auto" ",write-cache=on" ",write-cache=off"; do
+echo "Testing: cache='$cache' wce='$wce'"
 echo "info block" \
-| run_qemu -drive "$drive,cache=$cache" \
+| run_qemu $SYSEMU_EXTRA_ARGS -drive 
"$SYSEMU_DRIVE_ARG,cache=$cache" \

[Qemu-block] [PATCH v5 1/5] iotests: skip 159 & 170 with luks format

[Daniel P. Berrange]

While the qemu-img dd command does accept --image-opts
this is not sufficient to make it work with the LUKS
image yet. This is because bdrv_create() still always
requires the non-image-opts syntax.

Thus we must skip 159/170 with luks for now

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/159 | 1 +
 tests/qemu-iotests/170 | 1 +
 2 files changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/159 b/tests/qemu-iotests/159
index 825f05f..9b0e1ec 100755
--- a/tests/qemu-iotests/159
+++ b/tests/qemu-iotests/159
@@ -40,6 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt generic
 _supported_proto file
 _supported_os Linux
+_unsupported_fmt luks
 
 TEST_SIZES="5 512 1024 1999 1K 64K 1M"
 
diff --git a/tests/qemu-iotests/170 b/tests/qemu-iotests/170
index 5b335db..b79359f 100755
--- a/tests/qemu-iotests/170
+++ b/tests/qemu-iotests/170
@@ -40,6 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt generic
 _supported_proto file
 _supported_os Linux
+_unsupported_fmt luks
 
 echo
 echo "== Creating image =="
-- 
2.9.3

[Qemu-block] [PATCH v5 4/5] iotests: add more LUKS hash combination tests

[Daniel P. Berrange]

Add tests for sha224, sha512, sha384 and ripemd160 hash
algorithms.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 tests/qemu-iotests/149 |  10 +-
 tests/qemu-iotests/149.out | 482 -
 2 files changed, 484 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index f62e618..5faf585 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -457,8 +457,12 @@ configs = [
 
 
 # LUKS default but diff hash
+LUKSConfig("aes-256-xts-plain64-sha224",
+   "aes", 256, "xts", "plain64", None, "sha224"),
 LUKSConfig("aes-256-xts-plain64-sha256",
"aes", 256, "xts", "plain64", None, "sha256"),
+LUKSConfig("aes-256-xts-plain64-sha384",
+   "aes", 256, "xts", "plain64", None, "sha384"),
 LUKSConfig("aes-256-xts-plain64-sha512",
"aes", 256, "xts", "plain64", None, "sha512"),
 LUKSConfig("aes-256-xts-plain64-ripemd160",
@@ -504,12 +508,6 @@ blacklist = [
 
 # GCrypt doesn't support Twofish with 192 bit key
 "twofish-192-xts-plain64-sha1",
-
-# We don't have sha512 hash wired up yet
-"aes-256-xts-plain64-sha512",
-
-# We don't have ripemd160 hash wired up yet
-"aes-256-xts-plain64-ripemd160",
 ]
 
 whitelist = []
diff --git a/tests/qemu-iotests/149.out b/tests/qemu-iotests/149.out
index b18c4e4..2f0454f 100644
--- a/tests/qemu-iotests/149.out
+++ b/tests/qemu-iotests/149.out
@@ -1562,6 +1562,126 @@ unlink TEST_DIR/luks-serpent-192-xts-plain64-sha1.img
 
 Skipping cast6-128-xts-plain64-sha1 in blacklist
 Skipping cast6-192-xts-plain64-sha1 in blacklist
+# = dm-crypt aes-256-xts-plain64-sha224 =
+# Create image
+truncate TEST_DIR/luks-aes-256-xts-plain64-sha224.img --size 4194304MB
+# Format image
+sudo cryptsetup -q -v luksFormat --cipher aes-xts-plain64 --key-size 512 
--hash sha224 --key-slot 0 --key-file - --iter-time 10 
TEST_DIR/luks-aes-256-xts-plain64-sha224.img
+# Open dev
+sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-sha224.img 
qiotest-145-aes-256-xts-plain64-sha224
+# Set dev owner
+sudo chown UID:GID /dev/mapper/qiotest-145-aes-256-xts-plain64-sha224
+# Write test pattern 0xa7
+qemu-io -c write -P 0xa7 100M 10M --image-opts 
driver=file,filename=/dev/mapper/qiotest-145-aes-256-xts-plain64-sha224
+wrote 10485760/10485760 bytes at offset 104857600
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+# Write test pattern 0x13
+qemu-io -c write -P 0x13 3145728M 10M --image-opts 
driver=file,filename=/dev/mapper/qiotest-145-aes-256-xts-plain64-sha224
+wrote 10485760/10485760 bytes at offset 3298534883328
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+# Close dev
+sudo cryptsetup -q -v luksClose qiotest-145-aes-256-xts-plain64-sha224
+# Read test pattern 0xa7
+qemu-io -c read -P 0xa7 100M 10M --object 
secret,id=sec0,data=MTIzNDU2,format=base64 --image-opts 
driver=luks,key-secret=sec0,file.filename=TEST_DIR/luks-aes-256-xts-plain64-sha224.img
+read 10485760/10485760 bytes at offset 104857600
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+# Read test pattern 0x13
+qemu-io -c read -P 0x13 3145728M 10M --object 
secret,id=sec0,data=MTIzNDU2,format=base64 --image-opts 
driver=luks,key-secret=sec0,file.filename=TEST_DIR/luks-aes-256-xts-plain64-sha224.img
+read 10485760/10485760 bytes at offset 3298534883328
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+# Write test pattern 0x91
+qemu-io -c write -P 0x91 100M 10M --object 
secret,id=sec0,data=MTIzNDU2,format=base64 --image-opts 
driver=luks,key-secret=sec0,file.filename=TEST_DIR/luks-aes-256-xts-plain64-sha224.img
+wrote 10485760/10485760 bytes at offset 104857600
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+# Write test pattern 0x5e
+qemu-io -c write -P 0x5e 3145728M 10M --object 
secret,id=sec0,data=MTIzNDU2,format=base64 --image-opts 
driver=luks,key-secret=sec0,file.filename=TEST_DIR/luks-aes-256-xts-plain64-sha224.img
+wrote 10485760/10485760 bytes at offset 3298534883328
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+# Open dev
+sudo cryptsetup -q -v luksOpen TEST_DIR/luks-aes-256-xts-plain64-sha224.img 
qiotest-145-aes-256-xts-plain64-sha224
+# Set dev owner
+sudo chown UID:GID /dev/mapper/qiotest-145-aes-256-xts-plain64-sha224
+# Read test pattern 0x91
+qemu-io -c read -P 0x91 100M 10M --image-opts 
driver=file,filename=/dev/mapper/qiotest-145-aes-256-xts-plain64-sha224
+read 10485760/10485760 bytes at offset 104857600
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+# Read test pattern 0x5e
+qemu-io -c read -P 0x5e 3145728M 10M --image-opts 
driver=file,filename=/dev/mapper/qiotest-145-aes-256-xts-plain64-sha224
+read 10485760/10485760 bytes at offset 3298534883328
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+# Close dev
+sudo cryptsetup -q -v luksClose qiotest-145-aes-256-xts-plain64-sha224
+# Delete

[Qemu-block] [PATCH v5 0/5] Improve I/O tests coverage of LUKS

[Daniel P. Berrange]

The main goal of this series is to get the I/O tests passing
100% with LUKS when run with './check -luks'. It also adds a
few more combinations to the LUKS/dmcrypt interoperability
test.

To make LUKS testing not quite as slow, we drop the PBKDF
iteration count down to a very small value. This doesn't
remove all overhead, as formatting the volume will always
measure PBKDF timing over a 1 second interval.

Changed in v5:

 - Rebase to kevin/block  git tree instead of master
 - Fix LUKS compat in new test 181

Changed in v4:

 - Fix misc mistakes in syntax conversion (Max)
 - Drop changes to 120 & mark it unsupported (Max)
 - Use _unsupported_fmt (Max)

Changed in v3:

 - Fix some typos in commit message(s) (Eric)

Changed in v2:

 - Split off patch that change check.time recording since
   it was not a direct dependancy
 - Skip new 159 & 170 tests which don't work due to qemu-img
   dd limitations

Daniel P. Berrange (5):
  iotests: skip 159 & 170 with luks format
  iotests: fix remainining tests to work with LUKS
  iotests: reduce PBKDF iterations when testing LUKS
  iotests: add more LUKS hash combination tests
  iotests: chown LUKS device before qemu-io launches

 tests/qemu-iotests/033   |   12 +-
 tests/qemu-iotests/120   |1 +
 tests/qemu-iotests/140   |   11 +-
 tests/qemu-iotests/145   |   19 +-
 tests/qemu-iotests/149   |   26 +-
 tests/qemu-iotests/149.out   | 1002 --
 tests/qemu-iotests/157   |   17 +-
 tests/qemu-iotests/157.out   |   16 +-
 tests/qemu-iotests/159   |1 +
 tests/qemu-iotests/170   |1 +
 tests/qemu-iotests/174   |2 +-
 tests/qemu-iotests/181   |   21 +-
 tests/qemu-iotests/common.filter |3 +-
 tests/qemu-iotests/common.qemu   |9 +-
 tests/qemu-iotests/common.rc |3 +
 15 files changed, 844 insertions(+), 300 deletions(-)

-- 
2.9.3

Re: [Qemu-block] [PATCH 06/11] blockjob: move iostatus reset inside block_job_user_resume

[Jeff Cody]

On Mon, May 08, 2017 at 04:13:05PM +0200, Paolo Bonzini wrote:
> Outside blockjob.c, the block_job_iostatus_reset function is used once
> in the monitor and once in BlockBackend.  When we introduce the block
> job mutex, block_job_iostatus_reset's client is going to be the block
> layer (for which blockjob.c will take the block job mutex) rather than
> the monitor (which will take the block job mutex by itself).
> 
> The monitor's call to block_job_iostatus_reset from the monitor comes
> just before the sole call to block_job_user_resume, so reset the
> iostatus directly from block_job_iostatus_reset.  This will avoid
> the need to introduce separate block_job_iostatus_reset and
> block_job_iostatus_reset_locked APIs.
> 
> After making this change, move the function together with the others
> that were moved in the previous patch.
> 
> Reviewed-by: Stefan Hajnoczi 
> Reviewed-by: John Snow 
> Signed-off-by: Paolo Bonzini 
> ---
>  blockdev.c |  1 -
>  blockjob.c | 11 ++-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 64282065d8..cdec4ac82a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3726,7 +3726,6 @@ void qmp_block_job_resume(const char *device, Error 
> **errp)
>  }
>  
>  trace_qmp_block_job_resume(job);
> -block_job_iostatus_reset(job);
>  block_job_user_resume(job);
>  aio_context_release(aio_context);
>  }
> diff --git a/blockjob.c b/blockjob.c
> index 41dc2fe9f1..7edf779adc 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -412,6 +412,7 @@ void block_job_user_resume(BlockJob *job)
>  {
>  if (job && job->user_paused && job->pause_count > 0) {
>  job->user_paused = false;
> +block_job_iostatus_reset(job);
>  block_job_resume(job);
>  }
>  }
> @@ -427,11 +428,6 @@ void block_job_cancel(BlockJob *job)
>  }
>  }
>  
> -void block_job_iostatus_reset(BlockJob *job)
> -{
> -job->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
> -}
> -
>  static int block_job_finish_sync(BlockJob *job,
>   void (*finish)(BlockJob *, Error **errp),
>   Error **errp)
> @@ -767,6 +763,11 @@ void block_job_yield(BlockJob *job)
>  block_job_pause_point(job);
>  }
>  
> +void block_job_iostatus_reset(BlockJob *job)
> +{
> +job->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
> +}
> +
>  void block_job_event_ready(BlockJob *job)
>  {
>  job->ready = true;
> -- 
> 2.12.2
> 
>
Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH 05/11] blockjob: separate monitor and blockjob APIs

[Jeff Cody]

On Mon, May 08, 2017 at 04:13:04PM +0200, Paolo Bonzini wrote:
> We have two different headers for block job operations, blockjob.h
> and blockjob_int.h.  The former contains APIs called by the monitor,
> the latter contains APIs called by the block job drivers and the
> block layer itself.
> 
> Keep the two APIs separate in the blockjob.c file too.  This will
> be useful when transitioning away from the AioContext lock, because
> there will be locking policies for the two categories, too---the
> monitor will have to call new block_job_lock/unlock APIs, while blockjob
> APIs will take care of this for the users.
> 

Would it make sense to split this out into separate files, rather than
delineating it by placement in a single .c file?


> Reviewed-by: Stefan Hajnoczi 
> Signed-off-by: Paolo Bonzini 
> ---
>  blockjob.c | 390 
> -
>  1 file changed, 205 insertions(+), 185 deletions(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 85ad610cae..41dc2fe9f1 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -55,6 +55,21 @@ struct BlockJobTxn {
>  
>  static QLIST_HEAD(, BlockJob) block_jobs = 
> QLIST_HEAD_INITIALIZER(block_jobs);
>  
> +/*
> + * The block job API is composed of two categories of functions.
> + *
> + * The first includes functions used by the monitor.  The monitor is
> + * peculiar in that it accesses the block job list with block_job_get, and
> + * therefore needs consistency across block_job_get and the actual operation
> + * (e.g. block_job_set_speed).  The consistency is achieved with
> + * aio_context_acquire/release.  These functions are declared in blockjob.h.
> + *
> + * The second includes functions used by the block job drivers and sometimes
> + * by the core block layer.  These do not care about locking, because the
> + * whole coroutine runs under the AioContext lock, and are declared in
> + * blockjob_int.h.
> + */
> +
>  BlockJob *block_job_next(BlockJob *job)
>  {
>  if (!job) {
> @@ -216,90 +231,6 @@ int block_job_add_bdrv(BlockJob *job, const char *name, 
> BlockDriverState *bs,
>  return 0;
>  }
>  
> -void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> -   BlockDriverState *bs, uint64_t perm,
> -   uint64_t shared_perm, int64_t speed, int flags,
> -   BlockCompletionFunc *cb, void *opaque, Error **errp)
> -{
> -BlockBackend *blk;
> -BlockJob *job;
> -int ret;
> -
> -if (bs->job) {
> -error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
> -return NULL;
> -}
> -
> -if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
> -job_id = bdrv_get_device_name(bs);
> -if (!*job_id) {
> -error_setg(errp, "An explicit job ID is required for this node");
> -return NULL;
> -}
> -}
> -
> -if (job_id) {
> -if (flags & BLOCK_JOB_INTERNAL) {
> -error_setg(errp, "Cannot specify job ID for internal block job");
> -return NULL;
> -}
> -
> -if (!id_wellformed(job_id)) {
> -error_setg(errp, "Invalid job ID '%s'", job_id);
> -return NULL;
> -}
> -
> -if (block_job_get(job_id)) {
> -error_setg(errp, "Job ID '%s' already in use", job_id);
> -return NULL;
> -}
> -}
> -
> -blk = blk_new(perm, shared_perm);
> -ret = blk_insert_bs(blk, bs, errp);
> -if (ret < 0) {
> -blk_unref(blk);
> -return NULL;
> -}
> -
> -job = g_malloc0(driver->instance_size);
> -job->driver= driver;
> -job->id= g_strdup(job_id);
> -job->blk   = blk;
> -job->cb= cb;
> -job->opaque= opaque;
> -job->busy  = false;
> -job->paused= true;
> -job->pause_count   = 1;
> -job->refcnt= 1;
> -
> -error_setg(&job->blocker, "block device is in use by block job: %s",
> -   BlockJobType_lookup[driver->job_type]);
> -block_job_add_bdrv(job, "main node", bs, 0, BLK_PERM_ALL, &error_abort);
> -bs->job = job;
> -
> -blk_set_dev_ops(blk, &block_job_dev_ops, job);
> -bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
> -
> -QLIST_INSERT_HEAD(&block_jobs, job, job_list);
> -
> -blk_add_aio_context_notifier(blk, block_job_attached_aio_context,
> - block_job_detach_aio_context, job);
> -
> -/* Only set speed when necessary to avoid NotSupported error */
> -if (speed != 0) {
> -Error *local_err = NULL;
> -
> -block_job_set_speed(job, speed, &local_err);
> -if (local_err) {
> -block_job_unref(job);
> -error_propagate(errp, local_err);
> -return NULL;
> -}
> -}
> -return job;
> -}
> -
>  bool block_job_is_internal(BlockJob *job)
>  {
>  return (job->id == NULL);
> @@ -334,11 +2

Re: [Qemu-block] [PATCH 04/11] blockjob: introduce block_job_pause/resume_all

[Jeff Cody]

On Mon, May 08, 2017 at 04:13:03PM +0200, Paolo Bonzini wrote:
> Remove use of block_job_pause/resume from outside blockjob.c, thus
> making them static.  The new functions are used by the block layer,
> so place them in blockjob_int.h.
> 
> Reviewed-by: Stefan Hajnoczi 
> Reviewed-by: John Snow 
> Signed-off-by: Paolo Bonzini 
> ---
>  block/io.c   |  19 ++--
>  blockjob.c   | 114 
> ++-
>  include/block/blockjob.h |  16 --
>  include/block/blockjob_int.h |  14 ++
>  4 files changed, 86 insertions(+), 77 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index a7142e00e8..a54e5c8cea 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -26,6 +26,7 @@
>  #include "trace.h"
>  #include "sysemu/block-backend.h"
>  #include "block/blockjob.h"
> +#include "block/blockjob_int.h"
>  #include "block/block_int.h"
>  #include "qemu/cutils.h"
>  #include "qapi/error.h"
> @@ -301,16 +302,9 @@ void bdrv_drain_all_begin(void)
>  bool waited = true;
>  BlockDriverState *bs;
>  BdrvNextIterator it;
> -BlockJob *job = NULL;
>  GSList *aio_ctxs = NULL, *ctx;
>  
> -while ((job = block_job_next(job))) {
> -AioContext *aio_context = blk_get_aio_context(job->blk);
> -
> -aio_context_acquire(aio_context);
> -block_job_pause(job);
> -aio_context_release(aio_context);
> -}
> +block_job_pause_all();
>  
>  for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>  AioContext *aio_context = bdrv_get_aio_context(bs);
> @@ -354,7 +348,6 @@ void bdrv_drain_all_end(void)
>  {
>  BlockDriverState *bs;
>  BdrvNextIterator it;
> -BlockJob *job = NULL;
>  
>  for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>  AioContext *aio_context = bdrv_get_aio_context(bs);
> @@ -365,13 +358,7 @@ void bdrv_drain_all_end(void)
>  aio_context_release(aio_context);
>  }
>  
> -while ((job = block_job_next(job))) {
> -AioContext *aio_context = blk_get_aio_context(job->blk);
> -
> -aio_context_acquire(aio_context);
> -block_job_resume(job);
> -aio_context_release(aio_context);
> -}
> +block_job_resume_all();
>  }
>  
>  void bdrv_drain_all(void)
> diff --git a/blockjob.c b/blockjob.c
> index 5a722c3635..85ad610cae 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -55,36 +55,6 @@ struct BlockJobTxn {
>  
>  static QLIST_HEAD(, BlockJob) block_jobs = 
> QLIST_HEAD_INITIALIZER(block_jobs);
>  
> -static char *child_job_get_parent_desc(BdrvChild *c)
> -{
> -BlockJob *job = c->opaque;
> -return g_strdup_printf("%s job '%s'",
> -   BlockJobType_lookup[job->driver->job_type],
> -   job->id);
> -}
> -
> -static const BdrvChildRole child_job = {
> -.get_parent_desc= child_job_get_parent_desc,
> -.stay_at_node   = true,
> -};
> -
> -static void block_job_drained_begin(void *opaque)
> -{
> -BlockJob *job = opaque;
> -block_job_pause(job);
> -}
> -
> -static void block_job_drained_end(void *opaque)
> -{
> -BlockJob *job = opaque;
> -block_job_resume(job);
> -}
> -
> -static const BlockDevOps block_job_dev_ops = {
> -.drained_begin = block_job_drained_begin,
> -.drained_end = block_job_drained_end,
> -};
> -
>  BlockJob *block_job_next(BlockJob *job)
>  {
>  if (!job) {
> @@ -106,6 +76,21 @@ BlockJob *block_job_get(const char *id)
>  return NULL;
>  }
>  
> +static void block_job_pause(BlockJob *job)
> +{
> +job->pause_count++;
> +}
> +
> +static void block_job_resume(BlockJob *job)
> +{
> +assert(job->pause_count > 0);
> +job->pause_count--;
> +if (job->pause_count) {
> +return;
> +}
> +block_job_enter(job);
> +}
> +
>  static void block_job_ref(BlockJob *job)
>  {
>  ++job->refcnt;
> @@ -171,6 +156,36 @@ static void block_job_detach_aio_context(void *opaque)
>  block_job_unref(job);
>  }
>  
> +static char *child_job_get_parent_desc(BdrvChild *c)
> +{
> +BlockJob *job = c->opaque;
> +return g_strdup_printf("%s job '%s'",
> +   BlockJobType_lookup[job->driver->job_type],
> +   job->id);
> +}
> +
> +static const BdrvChildRole child_job = {
> +.get_parent_desc= child_job_get_parent_desc,
> +.stay_at_node   = true,
> +};
> +
> +static void block_job_drained_begin(void *opaque)
> +{
> +BlockJob *job = opaque;
> +block_job_pause(job);
> +}
> +
> +static void block_job_drained_end(void *opaque)
> +{
> +BlockJob *job = opaque;
> +block_job_resume(job);
> +}
> +
> +static const BlockDevOps block_job_dev_ops = {
> +.drained_begin = block_job_drained_begin,
> +.drained_end = block_job_drained_end,
> +};
> +
>  void block_job_remove_all_bdrv(BlockJob *job)
>  {
>  GSList *l;
> @@ -471,11 +486,6 @@ void block_job_complete(BlockJob *job, Error **errp)
>  job->driver->complete(job, errp);
>  }

Re: [Qemu-block] [PATCH 03/11] blockjob: introduce block_job_early_fail

[Jeff Cody]

On Mon, May 08, 2017 at 04:13:02PM +0200, Paolo Bonzini wrote:
> Outside blockjob.c, block_job_unref is only used when a block job fails
> to start, and block_job_ref is not used at all.  The reference counting
> thus is pretty well hidden.  Introduce a separate function to be used
> by block jobs; because block_job_ref and block_job_unref now become
> static, move them earlier in blockjob.c.
> 
> Reviewed-by: Stefan Hajnoczi 
> Reviewed-by: John Snow 
> Signed-off-by: Paolo Bonzini 
> ---
>  block/backup.c   |  2 +-
>  block/commit.c   |  2 +-
>  block/mirror.c   |  2 +-
>  blockjob.c   | 47 
> ++--
>  include/block/blockjob_int.h | 15 +++---
>  tests/test-blockjob.c| 10 +-
>  6 files changed, 39 insertions(+), 39 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index a4fb2884f9..5387fbd84e 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -692,7 +692,7 @@ BlockJob *backup_job_create(const char *job_id, 
> BlockDriverState *bs,
>  }
>  if (job) {
>  backup_clean(&job->common);
> -block_job_unref(&job->common);
> +block_job_early_fail(&job->common);
>  }
>  
>  return NULL;
> diff --git a/block/commit.c b/block/commit.c
> index 91d2c344f6..99e41c6e50 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -426,7 +426,7 @@ fail:
>  if (commit_top_bs) {
>  bdrv_set_backing_hd(overlay_bs, top, &error_abort);
>  }
> -block_job_unref(&s->common);
> +block_job_early_fail(&s->common);
>  }
>  
>  
> diff --git a/block/mirror.c b/block/mirror.c
> index 9f5eb692fd..6a6619ca71 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1252,7 +1252,7 @@ fail:
>  
>  g_free(s->replaces);
>  blk_unref(s->target);
> -block_job_unref(&s->common);
> +block_job_early_fail(&s->common);
>  }
>  
>  bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
> diff --git a/blockjob.c b/blockjob.c
> index 71187d0c9e..5a722c3635 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -106,6 +106,32 @@ BlockJob *block_job_get(const char *id)
>  return NULL;
>  }
>  
> +static void block_job_ref(BlockJob *job)
> +{
> +++job->refcnt;
> +}
> +
> +static void block_job_attached_aio_context(AioContext *new_context,
> +   void *opaque);
> +static void block_job_detach_aio_context(void *opaque);
> +
> +static void block_job_unref(BlockJob *job)
> +{
> +if (--job->refcnt == 0) {
> +BlockDriverState *bs = blk_bs(job->blk);
> +bs->job = NULL;
> +block_job_remove_all_bdrv(job);
> +blk_remove_aio_context_notifier(job->blk,
> +block_job_attached_aio_context,
> +block_job_detach_aio_context, job);
> +blk_unref(job->blk);
> +error_free(job->blocker);
> +g_free(job->id);
> +QLIST_REMOVE(job, job_list);
> +g_free(job);
> +}
> +}
> +
>  static void block_job_attached_aio_context(AioContext *new_context,
> void *opaque)
>  {
> @@ -293,26 +319,9 @@ void block_job_start(BlockJob *job)
>  bdrv_coroutine_enter(blk_bs(job->blk), job->co);
>  }
>  
> -void block_job_ref(BlockJob *job)
> -{
> -++job->refcnt;
> -}
> -
> -void block_job_unref(BlockJob *job)
> +void block_job_early_fail(BlockJob *job)
>  {
> -if (--job->refcnt == 0) {
> -BlockDriverState *bs = blk_bs(job->blk);
> -bs->job = NULL;
> -block_job_remove_all_bdrv(job);
> -blk_remove_aio_context_notifier(job->blk,
> -block_job_attached_aio_context,
> -block_job_detach_aio_context, job);
> -blk_unref(job->blk);
> -error_free(job->blocker);
> -g_free(job->id);
> -QLIST_REMOVE(job, job_list);
> -g_free(job);
> -}
> +block_job_unref(job);
>  }
>  
>  static void block_job_completed_single(BlockJob *job)
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index bfcc5d1241..45cdfd4ac1 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -156,21 +156,12 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType 
> type, int64_t ns);
>  void block_job_yield(BlockJob *job);
>  
>  /**
> - * block_job_ref:
> + * block_job_early_fail:
>   * @bs: The block device.
>   *
> - * Grab a reference to the block job. Should be paired with block_job_unref.
> + * The block job could not be started, free it.
>   */
> -void block_job_ref(BlockJob *job);
> -
> -/**
> - * block_job_unref:
> - * @bs: The block device.
> - *
> - * Release reference to the block job and release resources if it is the last
> - * reference.
> - */
> -void block_job_unref(BlockJob *job);
> +void block_job_early_fail(BlockJob *job);
>

Re: [Qemu-block] [PATCH 02/11] blockjob: remove iostatus_reset callback

[Jeff Cody]

On Mon, May 08, 2017 at 04:13:01PM +0200, Paolo Bonzini wrote:
> This is unused since commit 66a0fae ("blockjob: Don't touch BDS iostatus",
> 2016-05-19).
> 
> Reviewed-by: Stefan Hajnoczi 
> Reviewed-by: John Snow 
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Paolo Bonzini 
> ---
>  blockjob.c   | 3 ---
>  include/block/blockjob_int.h | 3 ---
>  2 files changed, 6 deletions(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 23022b3331..71187d0c9e 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -555,9 +555,6 @@ bool block_job_is_cancelled(BlockJob *job)
>  void block_job_iostatus_reset(BlockJob *job)
>  {
>  job->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
> -if (job->driver->iostatus_reset) {
> -job->driver->iostatus_reset(job);
> -}
>  }
>  
>  static int block_job_finish_sync(BlockJob *job,
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index 3f86cc5acc..bfcc5d1241 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -44,9 +44,6 @@ struct BlockJobDriver {
>  /** Optional callback for job types that support setting a speed limit */
>  void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
>  
> -/** Optional callback for job types that need to forward I/O status 
> reset */
> -void (*iostatus_reset)(BlockJob *job);
> -
>  /** Mandatory: Entrypoint for the Coroutine. */
>  CoroutineEntry *start;
>  
> -- 
> 2.12.2
> 
>
Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH 01/11] blockjob: remove unnecessary check

[Jeff Cody]

On Mon, May 08, 2017 at 04:13:00PM +0200, Paolo Bonzini wrote:
> !job is always checked prior to the call, drop it from here.
> 
> Reviewed-by: Stefan Hajnoczi 
> Signed-off-by: Paolo Bonzini 
> ---
>  blockjob.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 6e489327ff..23022b3331 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -480,7 +480,7 @@ static bool block_job_should_pause(BlockJob *job)
>  
>  bool block_job_user_paused(BlockJob *job)
>  {
> -return job ? job->user_paused : 0;
> +return job->user_paused;
>  }
>  
>  void coroutine_fn block_job_pause_point(BlockJob *job)
> -- 
> 2.12.2
> 
>
Reviewed-by: Jeff Cody 

Re: [Qemu-block] [PATCH v4] migration/block: move bdrv_is_allocated() into bb's AioContext

[Paolo Bonzini]

On 08/05/2017 22:54, Stefan Hajnoczi wrote:
> On Fri, May 05, 2017 at 04:03:49PM +0800, jemmy858...@gmail.com wrote:
>> From: Lidong Chen 
>>
>> when block migration with high-speed, mig_save_device_bulk hold the
>> BQL and invoke bdrv_is_allocated frequently. This patch moves
>> bdrv_is_allocated() into bb's AioContext. It will execute without
>> blocking other I/O activity.
>>
>> Signed-off-by: Lidong Chen 
>> ---
>>  v4 changelog:
>>   Use the prototype code written by Stefan and fix some bug.
>>   moves bdrv_is_allocated() into bb's AioContext.
>> ---
>>  migration/block.c | 48 +++-
>>  1 file changed, 39 insertions(+), 9 deletions(-)
> 
> Added Paolo because he's been reworking AioContext and locking.
> 
> The goal of this patch is to avoid waiting for bdrv_is_allocated() to
> complete while holding locks.  Do bdrv_is_allocated() in the AioContext
> so event processing continues after yield.

Since raw_co_get_block_status is coroutine-based I think you should
instead use the thread pool (thread_pool_submit_co) in
block/file-posix.c.  This way the lseek is done in a separate thread.

The problem is that find_allocation() is not atomic, so you need either
a CoMutex around raw_co_get_block_status's call to
thread_pool_submit_co, or a QemuMutex in find_allocation itself.

Thanks,

Paolo

>>
>> diff --git a/migration/block.c b/migration/block.c
>> index 060087f..c871361 100644
>> --- a/migration/block.c
>> +++ b/migration/block.c
>> @@ -263,6 +263,30 @@ static void blk_mig_read_cb(void *opaque, int ret)
>>  blk_mig_unlock();
>>  }
>>  
>> +typedef struct {
>> +int64_t *total_sectors;
>> +int64_t *cur_sector;
>> +BlockBackend *bb;
>> +QemuEvent event;
>> +} MigNextAllocatedClusterData;
>> +
>> +static void coroutine_fn mig_next_allocated_cluster(void *opaque)
>> +{
>> +MigNextAllocatedClusterData *data = opaque;
>> +int nr_sectors;
>> +
>> +/* Skip unallocated sectors; intentionally treats failure as
>> + * an allocated sector */
>> +while (*data->cur_sector < *data->total_sectors &&
>> +   !bdrv_is_allocated(blk_bs(data->bb), *data->cur_sector,
>> +  MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
>> +*data->cur_sector += nr_sectors;
>> +}
>> +
>> +bdrv_dec_in_flight(blk_bs(data->bb));
>> +qemu_event_set(&data->event);
>> +}
>> +
>>  /* Called with no lock taken.  */
>>  
>>  static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
>> @@ -274,17 +298,23 @@ static int mig_save_device_bulk(QEMUFile *f, 
>> BlkMigDevState *bmds)
>>  int nr_sectors;
>>  
>>  if (bmds->shared_base) {
>> +AioContext *bb_ctx;
>> +Coroutine *co;
>> +MigNextAllocatedClusterData data = {
>> +.cur_sector = &cur_sector,
>> +.total_sectors = &total_sectors,
>> +.bb = bb,
>> +};
>> +qemu_event_init(&data.event, false);
>> +
>>  qemu_mutex_lock_iothread();
>> -aio_context_acquire(blk_get_aio_context(bb));
>> -/* Skip unallocated sectors; intentionally treats failure as
>> - * an allocated sector */
>> -while (cur_sector < total_sectors &&
>> -   !bdrv_is_allocated(blk_bs(bb), cur_sector,
>> -  MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
>> -cur_sector += nr_sectors;
>> -}
>> -aio_context_release(blk_get_aio_context(bb));
>> +bdrv_inc_in_flight(blk_bs(bb));
> 
> Please add a comment explaining why bdrv_inc_in_flight() is invoked.
> 
>> +bb_ctx = blk_get_aio_context(bb);
>> +co = qemu_coroutine_create(mig_next_allocated_cluster, &data);
>> +aio_co_schedule(bb_ctx, co);
>>  qemu_mutex_unlock_iothread();
>> +
>> +qemu_event_wait(&data.event);
>>  }
>>  
>>  if (cur_sector >= total_sectors) {
>> -- 
>> 1.8.3.1
>>
>>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] Strange location of the "qemu-ga Invocation" chapter in the qemu-doc

[Paolo Bonzini]

On 09/05/2017 16:29, Thomas Huth wrote:
>  Hi all,
> 
> I noticed that the "qemu-ga Invocation" chapter in the QEMU doc is in a
> strange location - it's a sub-chapter of the "Disk images" chapter.
> Since the guest agent is not directly related to the handling of disk
> images, that sounds somewhat wrong to me - or do I miss something?
> Does anybody have a suggestion for a better location?

It should be part of a separate manual, probably, together with QMP and
other management stuff.  But for now just moving it to a @chapter would do.

Paolo

Re: [Qemu-block] [PATCH] block: curl: Allow passing cookies via QCryptoSecret

[Jeff Cody]

On Thu, May 04, 2017 at 04:00:06PM +0200, Peter Krempa wrote:
> Since cookies can contain sensitive data (session ID, etc ...) it is
> desired to hide them from the prying eyes of users. Add a possibility to
> pass them via the secret infrastructure.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1447413
> 
> Signed-off-by: Peter Krempa 
> ---
>  block/curl.c | 24 +++-
>  qapi/block-core.json | 12 ++--
>  2 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 2708d57c2f..483640b14a 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -85,6 +85,7 @@ static CURLMcode __curl_multi_socket_action(CURLM 
> *multi_handle,
>  #define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
>  #define CURL_BLOCK_OPT_TIMEOUT "timeout"
>  #define CURL_BLOCK_OPT_COOKIE"cookie"
> +#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret"
>  #define CURL_BLOCK_OPT_USERNAME "username"
>  #define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret"
>  #define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
> @@ -624,6 +625,11 @@ static QemuOptsList runtime_opts = {
>  .help = "Pass the cookie or list of cookies with each request"
>  },
>  {
> +.name = CURL_BLOCK_OPT_COOKIE_SECRET,
> +.type = QEMU_OPT_STRING,
> +.help = "ID of secret used as cookie passed with each request"
> +},
> +{
>  .name = CURL_BLOCK_OPT_USERNAME,
>  .type = QEMU_OPT_STRING,
>  .help = "Username for HTTP auth"
> @@ -657,6 +663,7 @@ static int curl_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  Error *local_err = NULL;
>  const char *file;
>  const char *cookie;
> +const char *cookie_secret;
>  double d;
>  const char *secretid;
>  const char *protocol_delimiter;
> @@ -693,7 +700,22 @@ static int curl_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
> 
>  cookie = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE);
> -s->cookie = g_strdup(cookie);
> +cookie_secret = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE_SECRET);
> +
> +if (cookie && cookie_secret) {
> +error_setg(errp,
> +   "curl driver cannot handle both cookie and cookie 
> secret");
> +goto out_noclean;
> +}
> +
> +if (cookie_secret) {
> +s->cookie = qcrypto_secret_lookup_as_utf8(cookie_secret, errp);
> +if (!s->cookie) {
> +goto out_noclean;
> +}
> +} else {
> +s->cookie = g_strdup(cookie);
> +}
> 
>  file = qemu_opt_get(opts, CURL_BLOCK_OPT_URL);
>  if (file == NULL) {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 87fb747ab6..b1643d2032 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2782,11 +2782,15 @@
>  #   "name1=content1; name2=content2;" as explained by
>  #   CURLOPT_COOKIE(3). Defaults to no cookies.
>  #
> +# @cookie-secret: ID of a QCryptoSecret object providing the cookie data in a
> +# secure way. See @cookie for the format. (since 2.10)
> +#
>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsCurlHttp',
>'base': 'BlockdevOptionsCurlBase',
> -  'data': { '*cookie': 'str' } }
> +  'data': { '*cookie': 'str',
> +'*cookie-secret': 'str'} }
> 
>  ##
>  # @BlockdevOptionsCurlHttps:
> @@ -2801,12 +2805,16 @@
>  # @sslverify:   Whether to verify the SSL certificate's validity (defaults to
>  #   true)
>  #
> +# @cookie-secret: ID of a QCryptoSecret object providing the cookie data in a
> +# secure way. See @cookie for the format. (since 2.10)
> +#
>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsCurlHttps',
>'base': 'BlockdevOptionsCurlBase',
>'data': { '*cookie': 'str',
> -'*sslverify': 'bool' } }
> +'*sslverify': 'bool',
> +'*cookie-secret': 'str'} }
> 
>  ##
>  # @BlockdevOptionsCurlFtp:
> -- 
> 2.12.2
> 
> 

Thanks,

Reviewed-by: Jeff Cody 

Also:

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc block

-Jeff

Re: [Qemu-block] [PATCH v2] nvme: Implement Write Zeroes

[Kevin Wolf]

Am 09.05.2017 um 17:09 hat Keith Busch geschrieben:
> On Tue, May 09, 2017 at 05:01:04PM +0200, Kevin Wolf wrote:
> > Am 05.05.2017 um 11:58 hat Christoph Hellwig geschrieben:
> > > Signed-off-by: Keith Busch 
> > > [hch: ported over from qemu-nvme.git to mainline]
> > > Signed-off-by: Christoph Hellwig 
> > 
> > Keith, can you give an Acked-by for this one? (I see that the code is
> > originally from you, but just to make sure that you agree with the
> > changes made and that it is merged into qemu.git this way.)
> 
> No problem, this still looks good to me. Thanks!
> 
> Acked-by: Keith Busch 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH v3 1/1] qemu-img: wait for convert coroutines to complete

[Kevin Wolf]

Am 26.04.2017 um 10:33 hat Anton Nefedov geschrieben:
> On error path (like i/o error in one of the coroutines), it's required to
>   - wait for coroutines completion before cleaning the common structures
>   - reenter dependent coroutines so they ever finish
> 
> Introduced in 2d9187bc65.
> 
> Signed-off-by: Anton Nefedov 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH v4 0/5] Improve I/O tests coverage of LUKS

[Kevin Wolf]

Am 02.05.2017 um 17:54 hat Daniel P. Berrange geschrieben:
> The main goal of this series is to get the I/O tests passing
> 100% with LUKS when run with './check -luks'. It also adds a
> few more combinations to the LUKS/dmcrypt interoperability
> test.
> 
> To make LUKS testing not quite as slow, we drop the PBKDF
> iteration count down to a very small value. This doesn't
> remove all overhead, as formatting the volume will always
> measure PBKDF timing over a 1 second interval.

It looks like patch 2 needs a rebase before it can be applied to my
block branch.

Kevin

Re: [Qemu-block] [PATCH v3] aio: add missing aio_notify() to aio_enable_external()

[Stefan Hajnoczi]

On Mon, May 08, 2017 at 02:07:05PM -0400, Stefan Hajnoczi wrote:
> The main loop uses aio_disable_external()/aio_enable_external() to
> temporarily disable processing of external AioContext clients like
> device emulation.
> 
> This allows monitor commands to quiesce I/O and prevent the guest from
> submitting new requests while a monitor command is in progress.
> 
> The aio_enable_external() API is currently broken when an IOThread is in
> aio_poll() waiting for fd activity when the main loop re-enables
> external clients.  Incrementing ctx->external_disable_cnt does not wake
> the IOThread from ppoll(2) so fd processing remains suspended and leads
> to unresponsive emulated devices.
> 
> This patch adds an aio_notify() call to aio_enable_external() so the
> IOThread is kicked out of ppoll(2) and will re-arm the file descriptors.
> 
> The bug can be reproduced as follows:
> 
>   $ qemu -M accel=kvm -m 1024 \
>  -object iothread,id=iothread0 \
>  -device virtio-scsi-pci,iothread=iothread0,id=virtio-scsi-pci0 \
>  -drive 
> if=none,id=drive0,aio=native,cache=none,format=raw,file=test.img \
>  -device scsi-hd,id=scsi-hd0,drive=drive0 \
>  -qmp tcp::,server,nowait
> 
>   $ scripts/qmp/qmp-shell localhost:
>   (qemu) blockdev-snapshot-sync device=drive0 snapshot-file=sn1.qcow2
>  mode=absolute-paths format=qcow2
> 
> After blockdev-snapshot-sync completes the SCSI disk will be
> unresponsive.  This leads to request timeouts inside the guest.
> 
> Reported-by: Qianqian Zhu 
> Suggested-by: Fam Zheng 
> Signed-off-by: Stefan Hajnoczi 
> ---
> v3:
>  * s/dec_fetch/fetch_dec/ [Fam]
> ---
>  include/block/aio.h | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH] block: curl: Allow passing cookies via QCryptoSecret

[Kevin Wolf]

Am 04.05.2017 um 16:00 hat Peter Krempa geschrieben:
> Since cookies can contain sensitive data (session ID, etc ...) it is
> desired to hide them from the prying eyes of users. Add a possibility to
> pass them via the secret infrastructure.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1447413
> 
> Signed-off-by: Peter Krempa 

I didn't really look at the patch, just adding a maintainer CC:

$ scripts/get_maintainer.pl -f block/curl.c
Jeff Cody  (supporter:CURL)
...

Kevin

>  block/curl.c | 24 +++-
>  qapi/block-core.json | 12 ++--
>  2 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 2708d57c2f..483640b14a 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -85,6 +85,7 @@ static CURLMcode __curl_multi_socket_action(CURLM 
> *multi_handle,
>  #define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
>  #define CURL_BLOCK_OPT_TIMEOUT "timeout"
>  #define CURL_BLOCK_OPT_COOKIE"cookie"
> +#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret"
>  #define CURL_BLOCK_OPT_USERNAME "username"
>  #define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret"
>  #define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
> @@ -624,6 +625,11 @@ static QemuOptsList runtime_opts = {
>  .help = "Pass the cookie or list of cookies with each request"
>  },
>  {
> +.name = CURL_BLOCK_OPT_COOKIE_SECRET,
> +.type = QEMU_OPT_STRING,
> +.help = "ID of secret used as cookie passed with each request"
> +},
> +{
>  .name = CURL_BLOCK_OPT_USERNAME,
>  .type = QEMU_OPT_STRING,
>  .help = "Username for HTTP auth"
> @@ -657,6 +663,7 @@ static int curl_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  Error *local_err = NULL;
>  const char *file;
>  const char *cookie;
> +const char *cookie_secret;
>  double d;
>  const char *secretid;
>  const char *protocol_delimiter;
> @@ -693,7 +700,22 @@ static int curl_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);
> 
>  cookie = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE);
> -s->cookie = g_strdup(cookie);
> +cookie_secret = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE_SECRET);
> +
> +if (cookie && cookie_secret) {
> +error_setg(errp,
> +   "curl driver cannot handle both cookie and cookie 
> secret");
> +goto out_noclean;
> +}
> +
> +if (cookie_secret) {
> +s->cookie = qcrypto_secret_lookup_as_utf8(cookie_secret, errp);
> +if (!s->cookie) {
> +goto out_noclean;
> +}
> +} else {
> +s->cookie = g_strdup(cookie);
> +}
> 
>  file = qemu_opt_get(opts, CURL_BLOCK_OPT_URL);
>  if (file == NULL) {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 87fb747ab6..b1643d2032 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2782,11 +2782,15 @@
>  #   "name1=content1; name2=content2;" as explained by
>  #   CURLOPT_COOKIE(3). Defaults to no cookies.
>  #
> +# @cookie-secret: ID of a QCryptoSecret object providing the cookie data in a
> +# secure way. See @cookie for the format. (since 2.10)
> +#
>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsCurlHttp',
>'base': 'BlockdevOptionsCurlBase',
> -  'data': { '*cookie': 'str' } }
> +  'data': { '*cookie': 'str',
> +'*cookie-secret': 'str'} }
> 
>  ##
>  # @BlockdevOptionsCurlHttps:
> @@ -2801,12 +2805,16 @@
>  # @sslverify:   Whether to verify the SSL certificate's validity (defaults to
>  #   true)
>  #
> +# @cookie-secret: ID of a QCryptoSecret object providing the cookie data in a
> +# secure way. See @cookie for the format. (since 2.10)
> +#
>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsCurlHttps',
>'base': 'BlockdevOptionsCurlBase',
>'data': { '*cookie': 'str',
> -'*sslverify': 'bool' } }
> +'*sslverify': 'bool',
> +'*cookie-secret': 'str'} }
> 
>  ##
>  # @BlockdevOptionsCurlFtp:
> -- 
> 2.12.2
> 
> 

Re: [Qemu-block] [Qemu-devel] [PATCH v7 5/5] shutdown: Expose bool cause in SHUTDOWN and RESET events

[Markus Armbruster]

Eric Blake  writes:

> On 05/09/2017 07:07 AM, Markus Armbruster wrote:
>> Eric Blake  writes:
>> 
>>> Libvirt would like to be able to distinguish between a SHUTDOWN
>>> event triggered solely by guest request and one triggered by a
>>> SIGTERM or other action on the host.  While qemu_kill_report() was
>>> already able to give different output to stderr based on whether a
>>> shutdown was triggered by a host signal (but NOT by a host UI event,
>>> such as clicking the X on the window), that information was then
>>> lost to management.  The previous patches improved things to use an
>>> enum throughout all callsites, so now we have something ready to
>>> expose through QMP.
>>>
>>> Here is output from 'virsh qemu-monitor-event --loop' with the
>>> patch installed:
>>>
>>> event SHUTDOWN at 1492639680.731251 for domain fedora_13: {"guest":true}
>>> event STOP at 1492639680.732116 for domain fedora_13: 
>>> event SHUTDOWN at 1492639680.732830 for domain fedora_13: {"guest":false}
>>>
>>> Note that libvirt runs qemu with -no-quit: the first SHUTDOWN event
>> 
>> Do you mean -no-shutdown?
>
> Oh, right. (Too many synonyms to choose from).
>
>> 
>>> was triggered by an action I took directly in the guest (shutdown -h),
>>> at which point qemu stops the vcpus and waits for libvirt to do any
>>> final cleanups; the second SHUTDOWN event is the result of libvirt
>>> sending SIGTERM now that it has completed cleanup.
>> 
>> The double shutdown is a bit weird.  To avoid it, we'd have to
>> distinguish between guest shutdown and QEMU termination.  shutdown -h in
>> the guest triggers termination only when QEMU is configured that way.
>> SIGTERM triggers shutdown only when the guest is running.  The result
>> would be neater, but I'm not sure it's worth the extra effort.
>
> And libvirt is already handling the double event emission - it only
> exposes the first event to the end-user (which is the one that will have
> the correct guest-vs-host flag); the second event (which will always be
> host) is elided because libvirt already knows it passed on the first
> event.  So changing it is outside the scope of this series.

Agreed.

>>> +++ b/vl.c
>>> @@ -1705,8 +1705,8 @@ void qemu_system_reset(ShutdownCause reason)
>>>  qemu_devices_reset();
>>>  }
>>>  if (reason) {
>>> -/* FIXME update event based on reason */
>>> -qapi_event_send_reset(&error_abort);
>>> +qapi_event_send_reset(reason >= SHUTDOWN_CAUSE_GUEST_SHUTDOWN,
>>> +  &error_abort);
>> 
>> Exploits enum ordering.  A comment in the enum definition warning not to
>> reorder its members would be in order.  Defining a suitable predicate
>> right next to it would do, too.
>
> As in, adding this to sysemu.h?

Yes, right next to the typedef.

> static inline bool shutdown_caused_by_guest(ShutdownCause cause) {
> return cause >= SHUTDOWN_CAUSE_GUEST_SHUTDOWN;
> }
>
> I can do that, if you like it.

Works for me.  A suitable comment in the enum would also work.

>> With at least the -no-quit in the commit message fixed (assuming it
>> needs fixing):
>
> Yes, it does need fixing.
>
>> 
>> Reviewed-by: Markus Armbruster 
>> 

Re: [Qemu-block] [PATCH v2] nvme: Implement Write Zeroes

[Keith Busch]

On Tue, May 09, 2017 at 05:01:04PM +0200, Kevin Wolf wrote:
> Am 05.05.2017 um 11:58 hat Christoph Hellwig geschrieben:
> > Signed-off-by: Keith Busch 
> > [hch: ported over from qemu-nvme.git to mainline]
> > Signed-off-by: Christoph Hellwig 
> 
> Keith, can you give an Acked-by for this one? (I see that the code is
> originally from you, but just to make sure that you agree with the
> changes made and that it is merged into qemu.git this way.)

No problem, this still looks good to me. Thanks!

Acked-by: Keith Busch 

Re: [Qemu-block] [PATCH v2] nvme: Implement Write Zeroes

[Kevin Wolf]

Am 05.05.2017 um 11:58 hat Christoph Hellwig geschrieben:
> Signed-off-by: Keith Busch 
> [hch: ported over from qemu-nvme.git to mainline]
> Signed-off-by: Christoph Hellwig 

Keith, can you give an Acked-by for this one? (I see that the code is
originally from you, but just to make sure that you agree with the
changes made and that it is merged into qemu.git this way.)

Kevin

>  hw/block/nvme.c | 26 ++
>  hw/block/nvme.h |  1 +
>  2 files changed, 27 insertions(+)
> 
>  Changes since v1:
>   - add BDRV_REQ_MAY_UNMAP flag
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index ae303d44e5..7428db9f0c 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -227,6 +227,29 @@ static uint16_t nvme_flush(NvmeCtrl *n, NvmeNamespace 
> *ns, NvmeCmd *cmd,
>  return NVME_NO_COMPLETE;
>  }
>  
> +static uint16_t nvme_write_zeros(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd 
> *cmd,
> +NvmeRequest *req)
> +{
> +NvmeRwCmd *rw = (NvmeRwCmd *)cmd;
> +const uint8_t lba_index = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
> +const uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds;
> +uint64_t slba = le64_to_cpu(rw->slba);
> +uint32_t nlb  = le16_to_cpu(rw->nlb) + 1;
> +uint64_t aio_slba = slba << (data_shift - BDRV_SECTOR_BITS);
> +uint32_t aio_nlb = nlb << (data_shift - BDRV_SECTOR_BITS);
> +
> +if (slba + nlb > ns->id_ns.nsze) {
> +return NVME_LBA_RANGE | NVME_DNR;
> +}
> +
> +req->has_sg = false;
> +block_acct_start(blk_get_stats(n->conf.blk), &req->acct, 0,
> + BLOCK_ACCT_WRITE);
> +req->aiocb = blk_aio_pwrite_zeroes(n->conf.blk, aio_slba, aio_nlb,
> +BDRV_REQ_MAY_UNMAP, nvme_rw_cb, req);
> +return NVME_NO_COMPLETE;
> +}
> +
>  static uint16_t nvme_rw(NvmeCtrl *n, NvmeNamespace *ns, NvmeCmd *cmd,
>  NvmeRequest *req)
>  {
> @@ -279,6 +302,8 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd, 
> NvmeRequest *req)
>  switch (cmd->opcode) {
>  case NVME_CMD_FLUSH:
>  return nvme_flush(n, ns, cmd, req);
> +case NVME_CMD_WRITE_ZEROS:
> +return nvme_write_zeros(n, ns, cmd, req);
>  case NVME_CMD_WRITE:
>  case NVME_CMD_READ:
>  return nvme_rw(n, ns, cmd, req);
> @@ -895,6 +920,7 @@ static int nvme_init(PCIDevice *pci_dev)
>  id->sqes = (0x6 << 4) | 0x6;
>  id->cqes = (0x4 << 4) | 0x4;
>  id->nn = cpu_to_le32(n->num_namespaces);
> +id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROS);
>  id->psd[0].mp = cpu_to_le16(0x9c4);
>  id->psd[0].enlat = cpu_to_le32(0x10);
>  id->psd[0].exlat = cpu_to_le32(0x4);
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 8fb0c10756..a0d15649f9 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -179,6 +179,7 @@ enum NvmeIoCommands {
>  NVME_CMD_READ   = 0x02,
>  NVME_CMD_WRITE_UNCOR= 0x04,
>  NVME_CMD_COMPARE= 0x05,
> +NVME_CMD_WRITE_ZEROS= 0x08,
>  NVME_CMD_DSM= 0x09,
>  };
>  
> -- 
> 2.11.0
> 

Re: [Qemu-block] [PATCH 0/6] block: Fix op blockers for inactive images

[Kevin Wolf]

Am 04.05.2017 um 18:52 hat Kevin Wolf geschrieben:
> Fam's image locking series introduced some special-casing in the file-posix
> driver that avoids taking locks when the image is inactive. While this works,
> it really isn't the job of the file-posix driver, but the core block layer
> should consider that inactive nodes require a lot less permissions.
> 
> This series integrates op blockers with bdrv_inactivate/invalidate_cache() to
> solve this problem gennerically, and removes the workaround in file-posix.

Applied to the block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] migration: add incremental drive-mirror and blockdev-mirror with dirtymap

[John Snow]

On 05/09/2017 03:35 AM, Daniel Kučera wrote:
> 
> 
> Hm, I suppose that's right, pending cache issues, perhaps?
> 
> (1) Write occurs; cached
> (2) Bitmap is added
> (3) Write occurs, cached
> (4) ZFS snapshot is taken
> (5) Data is flushed to backing storage.
> 
> Now, the ZFS snapshot is migrated, but is missing the writes that
> occurred in (1) and (3).
> 
> Next, we mirror the data in the bitmap, but it only includes the data
> from (3).
> 
> The target now appears to be missing the write from (1) -- maybe,
> depending on how the volume snapshot occurs.
> 
> 
> Yes, that's why I'm using cache=none. Libvirt doesn't allow you to
> migrate VM which uses drive cache anyway (unless you specify flag unsafe).

It will be important to document the exact use cases in which this mode
is "supported" and the shortcomings and cases in which it is not supported.

The final version should include a test and some documentation, but I
think the feature is workable once we pay attention to error conditions.

Thanks!

--js

Re: [Qemu-block] [PATCH v4] migration/block: move bdrv_is_allocated() into bb's AioContext

[Stefan Hajnoczi]

On Tue, May 09, 2017 at 08:59:30PM +0800, 858585 jemmy wrote:
> On Tue, May 9, 2017 at 4:54 AM, Stefan Hajnoczi  wrote:
> > On Fri, May 05, 2017 at 04:03:49PM +0800, jemmy858...@gmail.com wrote:
> >> From: Lidong Chen 
> >>
> >> when block migration with high-speed, mig_save_device_bulk hold the
> >> BQL and invoke bdrv_is_allocated frequently. This patch moves
> >> bdrv_is_allocated() into bb's AioContext. It will execute without
> >> blocking other I/O activity.
> >>
> >> Signed-off-by: Lidong Chen 
> >> ---
> >>  v4 changelog:
> >>   Use the prototype code written by Stefan and fix some bug.
> >>   moves bdrv_is_allocated() into bb's AioContext.
> >> ---
> >>  migration/block.c | 48 +++-
> >>  1 file changed, 39 insertions(+), 9 deletions(-)
> >
> > Added Paolo because he's been reworking AioContext and locking.
> >
> > The goal of this patch is to avoid waiting for bdrv_is_allocated() to
> > complete while holding locks.  Do bdrv_is_allocated() in the AioContext
> > so event processing continues after yield.
> 
> Hi Paolo:
> Some information about the problem.

Lidong, please see my comment below:

> >> @@ -274,17 +298,23 @@ static int mig_save_device_bulk(QEMUFile *f, 
> >> BlkMigDevState *bmds)
> >>  int nr_sectors;
> >>
> >>  if (bmds->shared_base) {
> >> +AioContext *bb_ctx;
> >> +Coroutine *co;
> >> +MigNextAllocatedClusterData data = {
> >> +.cur_sector = &cur_sector,
> >> +.total_sectors = &total_sectors,
> >> +.bb = bb,
> >> +};
> >> +qemu_event_init(&data.event, false);
> >> +
> >>  qemu_mutex_lock_iothread();
> >> -aio_context_acquire(blk_get_aio_context(bb));
> >> -/* Skip unallocated sectors; intentionally treats failure as
> >> - * an allocated sector */
> >> -while (cur_sector < total_sectors &&
> >> -   !bdrv_is_allocated(blk_bs(bb), cur_sector,
> >> -  MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
> >> -cur_sector += nr_sectors;
> >> -}
> >> -aio_context_release(blk_get_aio_context(bb));
> >> +bdrv_inc_in_flight(blk_bs(bb));
> >
> > Please add a comment explaining why bdrv_inc_in_flight() is invoked.


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH] block: Simplify BDRV_BLOCK_RAW recursion

[Stefan Hajnoczi]

On Thu, May 04, 2017 at 12:37:45PM -0500, Eric Blake wrote:
> Since we are already in coroutine context during the body of
> bdrv_co_get_block_status(), we can shave off a few layers of
> wrappers when recursing to query the protocol when a format driver
> returned BDRV_BLOCK_RAW.
> 
> Note that we are already using the correct recursion later on in
> the same function, when probing whether the protocol layer is sparse
> in order to find out if we can add BDRV_BLOCK_ZERO to an existing
> BDRV_BLOCK_DATA|BDRV_BLOCK_OFFSET_VALID.
> 
> Signed-off-by: Eric Blake 
> ---
>  block/io.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

[Qemu-block] Strange location of the "qemu-ga Invocation" chapter in the qemu-doc

[Thomas Huth]

 Hi all,

I noticed that the "qemu-ga Invocation" chapter in the QEMU doc is in a
strange location - it's a sub-chapter of the "Disk images" chapter.
Since the guest agent is not directly related to the handling of disk
images, that sounds somewhat wrong to me - or do I miss something?
Does anybody have a suggestion for a better location?

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v7 5/5] shutdown: Expose bool cause in SHUTDOWN and RESET events

[Eric Blake]

On 05/09/2017 07:07 AM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> Libvirt would like to be able to distinguish between a SHUTDOWN
>> event triggered solely by guest request and one triggered by a
>> SIGTERM or other action on the host.  While qemu_kill_report() was
>> already able to give different output to stderr based on whether a
>> shutdown was triggered by a host signal (but NOT by a host UI event,
>> such as clicking the X on the window), that information was then
>> lost to management.  The previous patches improved things to use an
>> enum throughout all callsites, so now we have something ready to
>> expose through QMP.
>>
>> Here is output from 'virsh qemu-monitor-event --loop' with the
>> patch installed:
>>
>> event SHUTDOWN at 1492639680.731251 for domain fedora_13: {"guest":true}
>> event STOP at 1492639680.732116 for domain fedora_13: 
>> event SHUTDOWN at 1492639680.732830 for domain fedora_13: {"guest":false}
>>
>> Note that libvirt runs qemu with -no-quit: the first SHUTDOWN event
> 
> Do you mean -no-shutdown?

Oh, right. (Too many synonyms to choose from).

> 
>> was triggered by an action I took directly in the guest (shutdown -h),
>> at which point qemu stops the vcpus and waits for libvirt to do any
>> final cleanups; the second SHUTDOWN event is the result of libvirt
>> sending SIGTERM now that it has completed cleanup.
> 
> The double shutdown is a bit weird.  To avoid it, we'd have to
> distinguish between guest shutdown and QEMU termination.  shutdown -h in
> the guest triggers termination only when QEMU is configured that way.
> SIGTERM triggers shutdown only when the guest is running.  The result
> would be neater, but I'm not sure it's worth the extra effort.

And libvirt is already handling the double event emission - it only
exposes the first event to the end-user (which is the one that will have
the correct guest-vs-host flag); the second event (which will always be
host) is elided because libvirt already knows it passed on the first
event.  So changing it is outside the scope of this series.


>> +++ b/vl.c
>> @@ -1705,8 +1705,8 @@ void qemu_system_reset(ShutdownCause reason)
>>  qemu_devices_reset();
>>  }
>>  if (reason) {
>> -/* FIXME update event based on reason */
>> -qapi_event_send_reset(&error_abort);
>> +qapi_event_send_reset(reason >= SHUTDOWN_CAUSE_GUEST_SHUTDOWN,
>> +  &error_abort);
> 
> Exploits enum ordering.  A comment in the enum definition warning not to
> reorder its members would be in order.  Defining a suitable predicate
> right next to it would do, too.

As in, adding this to sysemu.h?

static inline bool shutdown_caused_by_guest(ShutdownCause cause) {
return cause >= SHUTDOWN_CAUSE_GUEST_SHUTDOWN;
}

I can do that, if you like it.

> 
> With at least the -no-quit in the commit message fixed (assuming it
> needs fixing):

Yes, it does need fixing.

> 
> Reviewed-by: Markus Armbruster 
> 

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v6 13/18] qcow2: add support for LUKS encryption format

[Daniel P. Berrange]

On Wed, Apr 26, 2017 at 12:46:18PM -0500, Eric Blake wrote:
> On 04/25/2017 10:38 AM, Daniel P. Berrange wrote:
> > This adds support for using LUKS as an encryption format
> > with the qcow2 file, using the new encrypt.format parameter
> > to request "luks" format. e.g.
> > 
> >   # qemu-img create --object secret,data=123456,id=sec0 \
> >-f qcow2 -o encrypt.-format=luks,encrypt.key-secret=sec0 \
> 
> s/encrypt.-format/encrypt.format/
> 
> >test.qcow2 10G
> > 
> 
> > 
> > Aside from all the cryptographic differences implied by
> > use of the LUKS format, there is one further key difference
> > between the use of legacy AES and LUKS encryption in qcow2.
> > For LUKS, the initialiazation vectors are generated using
> > the host physical sector as the input, rather than the
> > guest virtual sector. This guarantees unique initialization
> > vectors for all sectors when qcow2 internal snapshots are
> > used, thus giving stronger protection against watermarking
> > attacks.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> 
> > @@ -165,6 +246,47 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
> > uint64_t start_offset,
> >  }
> >  break;
> >  
> > +case QCOW2_EXT_MAGIC_CRYPTO_HEADER: {
> > +unsigned int cflags = 0;
> > +if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
> > +error_setg(errp, "CRYPTO header extension only "
> > +   "expected with LUKS encryption method");
> > +return -EINVAL;
> > +}
> > +if (ext.len != sizeof(Qcow2CryptoHeaderExtension)) {
> > +error_setg(errp, "CRYPTO header extension size %u, "
> > +   "but expected size %zu", ext.len,
> > +   sizeof(Qcow2CryptoHeaderExtension));
> > +return -EINVAL;
> > +}
> > +
> > +ret = bdrv_pread(bs->file, offset, &s->crypto_header, ext.len);
> > +if (ret < 0) {
> > +error_setg_errno(errp, -ret,
> > + "Unable to read CRYPTO header extension");
> > +return ret;
> > +}
> > +be64_to_cpus(&s->crypto_header.offset);
> > +be64_to_cpus(&s->crypto_header.length);
> > +
> > +if ((s->crypto_header.offset % s->cluster_size) != 0) {
> > +error_setg(errp, "Encryption header offset '%" PRIu64 "' 
> > is "
> > +   "not a multiple of cluster size '%u'",
> > +   s->crypto_header.offset, s->cluster_size);
> > +return -EINVAL;
> > +}
> 
> Do we need to sanity check that crypto_header.length is not bogus?

The only sanity check I can see would be to put an arbitrary size limit
on the length ? I'm a little loathe to do that since LUKSv2 is going to
make the header extensible, and/or future LUKSv1 changes might adjust
padding, both making the header longer than it would be today. I would
like qemu-img to be able to open such future files, even if it then
refuses support for the features.

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

Re: [Qemu-block] [Qemu-devel] [PATCH 04/11] ide/ahci: add missing includes

[John Snow]

On 05/08/2017 07:58 PM, Eric Blake wrote:
> On 05/08/2017 06:39 PM, Philippe Mathieu-Daudé wrote:
>> qemu/include/hw/ide/ahci.h:260:16: error: field ‘sglist’ has incomplete type
>>  QEMUSGList sglist;
>> ^~
> 
> What are you doing to get this compilation error (configure options,
> platform, compiler, etc)?  I can't reproduce it. Is it something that
> pops up later when you remove includes from somewhere else, and you're
> just pre-emptively adding includes here to allow removal of includes later?
> 

"What he said," but if I had to guess, Philippe is testing that each
header can be compiled separately.

>> /qemu/include/hw/ide/ahci.h:272:5: error: unknown type name ‘IDEDMA’
>>  IDEDMA dma;
>>  ^~
>> qemu/include/hw/ide/ahci.h:273:5: error: unknown type name ‘IDEBus’
>>  IDEBus port;
>>  ^~
>> qemu/include/hw/ide/ahci.h:305:15: error: field ‘parent_obj’ has incomplete 
>> type
>>  PCIDevice parent_obj;
>>^~
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  include/hw/ide/ahci.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
>> index 0ca7c65820..293f9ebcd0 100644
>> --- a/include/hw/ide/ahci.h
>> +++ b/include/hw/ide/ahci.h
>> @@ -25,6 +25,8 @@
>>  #define HW_IDE_AHCI_H
>>  
>>  #include "hw/sysbus.h"
>> +#include "hw/ide/internal.h">> +#include "sysemu/dma.h"
>>  
>>  #define AHCI_MEM_BAR_SIZE 0x1000
>>  #define AHCI_MAX_PORTS32
>>
> 

Re: [Qemu-block] [PATCH v4] migration/block: move bdrv_is_allocated() into bb's AioContext

[858585 jemmy]

On Tue, May 9, 2017 at 4:54 AM, Stefan Hajnoczi  wrote:
> On Fri, May 05, 2017 at 04:03:49PM +0800, jemmy858...@gmail.com wrote:
>> From: Lidong Chen 
>>
>> when block migration with high-speed, mig_save_device_bulk hold the
>> BQL and invoke bdrv_is_allocated frequently. This patch moves
>> bdrv_is_allocated() into bb's AioContext. It will execute without
>> blocking other I/O activity.
>>
>> Signed-off-by: Lidong Chen 
>> ---
>>  v4 changelog:
>>   Use the prototype code written by Stefan and fix some bug.
>>   moves bdrv_is_allocated() into bb's AioContext.
>> ---
>>  migration/block.c | 48 +++-
>>  1 file changed, 39 insertions(+), 9 deletions(-)
>
> Added Paolo because he's been reworking AioContext and locking.
>
> The goal of this patch is to avoid waiting for bdrv_is_allocated() to
> complete while holding locks.  Do bdrv_is_allocated() in the AioContext
> so event processing continues after yield.

Hi Paolo:
Some information about the problem.
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01423.html

why bdrv_inc_in_flight() is needed.
blk_set_aio_context maybe invoked by vcpu thread. like this:
  blk_set_aio_context
   virtio_blk_data_plane_stop
virtio_pci_stop_ioeventfd
  virtio_pci_common_write
so bs->aio_context maybe change before mig_next_allocated_cluster().

I use this method to verify this patch:
run this command in guest os:
  while [ 1 ]; do rmmod virtio_blk; modprobe virtio_blk; done

   Thanks.

>
>>
>> diff --git a/migration/block.c b/migration/block.c
>> index 060087f..c871361 100644
>> --- a/migration/block.c
>> +++ b/migration/block.c
>> @@ -263,6 +263,30 @@ static void blk_mig_read_cb(void *opaque, int ret)
>>  blk_mig_unlock();
>>  }
>>
>> +typedef struct {
>> +int64_t *total_sectors;
>> +int64_t *cur_sector;
>> +BlockBackend *bb;
>> +QemuEvent event;
>> +} MigNextAllocatedClusterData;
>> +
>> +static void coroutine_fn mig_next_allocated_cluster(void *opaque)
>> +{
>> +MigNextAllocatedClusterData *data = opaque;
>> +int nr_sectors;
>> +
>> +/* Skip unallocated sectors; intentionally treats failure as
>> + * an allocated sector */
>> +while (*data->cur_sector < *data->total_sectors &&
>> +   !bdrv_is_allocated(blk_bs(data->bb), *data->cur_sector,
>> +  MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
>> +*data->cur_sector += nr_sectors;
>> +}
>> +
>> +bdrv_dec_in_flight(blk_bs(data->bb));
>> +qemu_event_set(&data->event);
>> +}
>> +
>>  /* Called with no lock taken.  */
>>
>>  static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
>> @@ -274,17 +298,23 @@ static int mig_save_device_bulk(QEMUFile *f, 
>> BlkMigDevState *bmds)
>>  int nr_sectors;
>>
>>  if (bmds->shared_base) {
>> +AioContext *bb_ctx;
>> +Coroutine *co;
>> +MigNextAllocatedClusterData data = {
>> +.cur_sector = &cur_sector,
>> +.total_sectors = &total_sectors,
>> +.bb = bb,
>> +};
>> +qemu_event_init(&data.event, false);
>> +
>>  qemu_mutex_lock_iothread();
>> -aio_context_acquire(blk_get_aio_context(bb));
>> -/* Skip unallocated sectors; intentionally treats failure as
>> - * an allocated sector */
>> -while (cur_sector < total_sectors &&
>> -   !bdrv_is_allocated(blk_bs(bb), cur_sector,
>> -  MAX_IS_ALLOCATED_SEARCH, &nr_sectors)) {
>> -cur_sector += nr_sectors;
>> -}
>> -aio_context_release(blk_get_aio_context(bb));
>> +bdrv_inc_in_flight(blk_bs(bb));
>
> Please add a comment explaining why bdrv_inc_in_flight() is invoked.
>
>> +bb_ctx = blk_get_aio_context(bb);
>> +co = qemu_coroutine_create(mig_next_allocated_cluster, &data);
>> +aio_co_schedule(bb_ctx, co);
>>  qemu_mutex_unlock_iothread();
>> +
>> +qemu_event_wait(&data.event);
>>  }
>>
>>  if (cur_sector >= total_sectors) {
>> --
>> 1.8.3.1
>>
>>

Re: [Qemu-block] [Qemu-devel] [PATCH v7 5/5] shutdown: Expose bool cause in SHUTDOWN and RESET events

[Markus Armbruster]

Eric Blake  writes:

> Libvirt would like to be able to distinguish between a SHUTDOWN
> event triggered solely by guest request and one triggered by a
> SIGTERM or other action on the host.  While qemu_kill_report() was
> already able to give different output to stderr based on whether a
> shutdown was triggered by a host signal (but NOT by a host UI event,
> such as clicking the X on the window), that information was then
> lost to management.  The previous patches improved things to use an
> enum throughout all callsites, so now we have something ready to
> expose through QMP.
>
> Note that for now, the decision was to expose ONLY a boolean,
> rather than promoting ShutdownCause to a QAPI enum; this is because
> libvirt has not expressed an interest in anything finer-grained.
> We can still add additional details, in a backwards-compatible
> manner, if a need later arises (if the addition happens before 2.10,
> we can replace the bool with an enum; otherwise, the enum will have
> to be in addition to the bool).
>
> Update expected iotest outputs to match the new data (complete
> coverage of the affected tests is obtained by -raw, -qcow2, and -nbd).
>
> Here is output from 'virsh qemu-monitor-event --loop' with the
> patch installed:
>
> event SHUTDOWN at 1492639680.731251 for domain fedora_13: {"guest":true}
> event STOP at 1492639680.732116 for domain fedora_13: 
> event SHUTDOWN at 1492639680.732830 for domain fedora_13: {"guest":false}
>
> Note that libvirt runs qemu with -no-quit: the first SHUTDOWN event

Do you mean -no-shutdown?

> was triggered by an action I took directly in the guest (shutdown -h),
> at which point qemu stops the vcpus and waits for libvirt to do any
> final cleanups; the second SHUTDOWN event is the result of libvirt
> sending SIGTERM now that it has completed cleanup.

The double shutdown is a bit weird.  To avoid it, we'd have to
distinguish between guest shutdown and QEMU termination.  shutdown -h in
the guest triggers termination only when QEMU is configured that way.
SIGTERM triggers shutdown only when the guest is running.  The result
would be neater, but I'm not sure it's worth the extra effort.

> See also https://bugzilla.redhat.com/1384007
>
> Signed-off-by: Eric Blake 
>
> ---
> v7: rebase to context
> v6: split out from 'shutdown: Add source information to SHUTDOWN and RESET'
> ---
>  qapi/event.json| 17 +
>  vl.c   |  8 
>  tests/qemu-iotests/071.out |  4 ++--
>  tests/qemu-iotests/081.out |  2 +-
>  tests/qemu-iotests/087.out | 12 ++--
>  tests/qemu-iotests/094.out |  2 +-
>  tests/qemu-iotests/117.out |  2 +-
>  tests/qemu-iotests/119.out |  2 +-
>  tests/qemu-iotests/120.out |  2 +-
>  tests/qemu-iotests/140.out |  2 +-
>  tests/qemu-iotests/143.out |  2 +-
>  tests/qemu-iotests/156.out |  2 +-
>  12 files changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/qapi/event.json b/qapi/event.json
> index e80f3f4..6d22b02 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -10,6 +10,10 @@
>  # Emitted when the virtual machine has shut down, indicating that qemu is
>  # about to exit.
>  #
> +# @guest: If true, the shutdown was triggered by a guest request (such as
> +# a guest-initiated ACPI shutdown request or other hardware-specific action)
> +# rather than a host request (such as sending qemu a SIGINT). (since 2.10)
> +#
>  # Note: If the command-line option "-no-shutdown" has been specified, qemu 
> will
>  # not exit, and a STOP event will eventually follow the SHUTDOWN event
>  #
> @@ -17,11 +21,11 @@
>  #
>  # Example:
>  #
> -# <- { "event": "SHUTDOWN",
> +# <- { "event": "SHUTDOWN", "data": { "guest": true },
>  #  "timestamp": { "seconds": 1267040730, "microseconds": 682951 } }
>  #
>  ##
> -{ 'event': 'SHUTDOWN' }
> +{ 'event': 'SHUTDOWN', 'data': { 'guest': 'bool' } }
>
>  ##
>  # @POWERDOWN:
> @@ -44,15 +48,20 @@
>  #
>  # Emitted when the virtual machine is reset
>  #
> +# @guest: If true, the reset was triggered by a guest request (such as
> +# a guest-initiated ACPI reboot request or other hardware-specific action)
> +# rather than a host request (such as the QMP command system_reset).
> +# (since 2.10)
> +#
>  # Since: 0.12.0
>  #
>  # Example:
>  #
> -# <- { "event": "RESET",
> +# <- { "event": "RESET", "data": { "guest": false },
>  #  "timestamp": { "seconds": 1267041653, "microseconds": 9518 } }
>  #
>  ##
> -{ 'event': 'RESET' }
> +{ 'event': 'RESET', 'data': { 'guest': 'bool' } }
>
>  ##
>  # @STOP:
> diff --git a/vl.c b/vl.c
> index 65487d9..5fd0e8f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1705,8 +1705,8 @@ void qemu_system_reset(ShutdownCause reason)
>  qemu_devices_reset();
>  }
>  if (reason) {
> -/* FIXME update event based on reason */
> -qapi_event_send_reset(&error_abort);
> +qapi_event_send_reset(reason >= SHUTDOWN_CAUSE_GUEST_SHUTDOWN,
> +  &error_abort);

Exploits enum ordering.  A 

Re: [Qemu-block] [PATCH 1/3] hw/block: Introduce rotational qdev property

[Kevin Wolf]

Am 06.05.2017 um 14:43 hat Aurelien Jarno geschrieben:
> The Linux kernel uses different I/O scheduler depending if the block
> device is a rotational device or not. Also it uses rotational devices
> to add entropy to the random pool.
> 
> This patch add a rotational qdev property so that the block device can
> be configured as a rotational device or a non-rotational device. Default
> to true to not change the default behavior.
> 
> Signed-off-by: Aurelien Jarno 
> ---
>  blockdev.c   | 4 
>  include/hw/block/block.h | 4 +++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 4d8cdedd54..c914420641 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -4037,6 +4037,10 @@ QemuOptsList qemu_common_drive_opts = {
>  .name = BDRV_OPT_READ_ONLY,
>  .type = QEMU_OPT_BOOL,
>  .help = "open drive file as read-only",
> +},{
> +.name = "rotational",
> +.type = QEMU_OPT_BOOL,
> +.help = "rotational drive (off, on)",
>  },
>  

This hunk is wrong (and unnecessary). It makes the -drive parser parse
rotational=... options without doing anything with them, so instead of
correctly producing an error that this isn't a block driver option, it
would be silently ignored.

-drive has a few legacy options that affect qdev devices, but we don't
want to add anything new there; people can just use -device. So the fix
for your patch is simply dropping this hunk.

> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index f3f6e8ef02..304a579eca 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -27,6 +27,7 @@ typedef struct BlockConf {
>  uint32_t cyls, heads, secs;
>  OnOffAuto wce;
>  bool share_rw;
> +bool rotational;
>  BlockdevOnError rerror;
>  BlockdevOnError werror;
>  } BlockConf;
> @@ -56,7 +57,8 @@ static inline unsigned int get_physical_block_exp(BlockConf 
> *conf)
> _conf.discard_granularity, -1), \
>  DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, \
>  ON_OFF_AUTO_AUTO), \
> -DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false)
> +DEFINE_PROP_BOOL("share-rw", _state, _conf.share_rw, false), \
> +DEFINE_PROP_BOOL("rotational", _state, _conf.rotational, true)
>  
>  #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)  \
>  DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \

Also not sure about this one. It adds the property to all block devices,
but only IDE and SCSI will actually look at it. Maybe it would be better
to add this property only to the individual devices that actually
support it.

Kevin

[Qemu-block] [PATCH v8 2/4] qemu-img: fix --image-opts usage with dd command

[Daniel P. Berrange]

The --image-opts flag can only be used to affect the parsing
of the source image. The target image has to be specified in
the traditional style regardless, since it needs to be passed
to the bdrv_create() API which does not support the new style
opts.

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

diff --git a/qemu-img.c b/qemu-img.c
index 34f49c2..d8fdcb1 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4239,8 +4239,13 @@ static int img_dd(int argc, char **argv)
 goto out;
 }
 
-blk2 = img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR,
-false, false);
+/* TODO, we can't honour --image-opts for the target,
+ * since it needs to be given in a format compatible
+ * with the bdrv_create() call above which does not
+ * support image-opts style.
+ */
+blk2 = img_open_file(out.filename, out_fmt, BDRV_O_RDWR,
+ false, false);
 
 if (!blk2) {
 ret = -1;
-- 
2.9.3

[Qemu-block] [PATCH v8 0/4] Improve convert and dd commands

[Daniel P. Berrange]

Update to

  v1: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05699.html
  v2: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00728.html
  v3: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04391.html
  v4: https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02153.html
  v5: https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg04109.html
  v6: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00215.html

This series is in response to Max pointing out that you cannot
use 'convert' for an encrypted target image.

The 'convert' and 'dd' commands need to first create the image
and then open it. The bdrv_create() method takes a set of options
for creating the image, which let us provide a key-secret for the
encryption key. When the commands then open the new image, they
don't provide any options, so the image is unable to be opened
due to lack of encryption key. It is also not possible to use
the --image-opts argument to provide structured options in the
target image name - it must be a plain filename to satisfy the
bdrv_create() API contract.

This series addresses these problems to some extent

 - Adds a new --target-image-opts flag which is used to say
   that the target filename is using structured options.
   It is *only* permitted to use this when -n is also set.
   ie the target image must be pre-created so convert/dd
   don't need to run bdrv_create().

 - When --target-image-opts is not used, add special case
   code that identifies options passed to bdrv_create()
   named "*key-secret" and adds them to the options used
   to open the new image

In future it is desirable to make --target-image-opts work even when -n is
*not* given. This requires considerable work to create a new bdrv_create()
API impl.

The first patch fixes a bug in the 'dd' command while the second adds support
for the missing '--object' arg to 'dd', allowing it to reference secrets when
opening files.  The last two patches implement the new features described above
for the 'convert' command.

NB v8 is based against git master once more, since the img_convert changes
previously in block-next have now merged.

Changed in v8:

 - Readd accidentally dropped check for compression (Max)
 - Fix indentation of variable declaration (Max)
 - Fix goto jump target (Max)

Changed in v7:

 - Drop the (accidentally included) revert patch (Eric)

Changed in v6:

 - Fix misc typos (Fam)
 - Resolve messy conflicts wrt max/block-next (Max)

Changed in v5:

 - Fix return value (Max)
 - Misc doc changes (Max)
 - Use error_abort (Max)

Changed in v4:

 - Refactor img_open_new_file in terms of img_open_file (Kevin)

Changed in v3:

 - Drop all patches affecting the 'dd' command except for the clear bug fix
   and the --object support. They can be re-considered once dd is rewritten
   to run ontop of convert.
 - Use consistent return/goto style in dd command (Max)
 - Fix error reporting when using compressed image and skip-create (Max)
 - Unconditionally create QDict when open files (Max)

Changed in v2:

 - Replace dd -n flag with support for conv=nocreat,notrunc
 - Misc typos (Eric, Fam)


Daniel P. Berrange (4):
  qemu-img: add support for --object with 'dd' command
  qemu-img: fix --image-opts usage with dd command
  qemu-img: introduce --target-image-opts for 'convert' command
  qemu-img: copy *key-secret opts when opening newly created files

 qemu-img-cmds.hx |   4 +-
 qemu-img.c   | 145 +++
 qemu-img.texi|  12 -
 3 files changed, 126 insertions(+), 35 deletions(-)

-- 
2.9.3

[Qemu-block] [PATCH v8 4/4] qemu-img: copy *key-secret opts when opening newly created files

[Daniel P. Berrange]

The qemu-img dd/convert commands will create an image file and
then try to open it. Historically it has been possible to open
new files without passing any options. With encrypted files
though, the *key-secret options are mandatory, so we need to
provide those options when opening the newly created file.

Reviewed-by: Max Reitz 
Reviewed-by: Fam Zheng 
Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 qemu-img.c | 41 +++--
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index bb79cfb..c865982 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -305,15 +305,17 @@ static BlockBackend *img_open_opts(const char *optstr,
 }
 
 static BlockBackend *img_open_file(const char *filename,
+   QDict *options,
const char *fmt, int flags,
bool writethrough, bool quiet)
 {
 BlockBackend *blk;
 Error *local_err = NULL;
-QDict *options = NULL;
 
 if (fmt) {
-options = qdict_new();
+if (!options) {
+options = qdict_new();
+}
 qdict_put(options, "driver", qstring_from_str(fmt));
 }
 
@@ -332,6 +334,33 @@ static BlockBackend *img_open_file(const char *filename,
 }
 
 
+static int img_add_key_secrets(void *opaque,
+   const char *name, const char *value,
+   Error **errp)
+{
+QDict *options = opaque;
+
+if (g_str_has_suffix(name, "key-secret")) {
+qdict_put(options, name, qstring_from_str(value));
+}
+
+return 0;
+}
+
+static BlockBackend *img_open_new_file(const char *filename,
+   QemuOpts *create_opts,
+   const char *fmt, int flags,
+   bool writethrough, bool quiet)
+{
+QDict *options = NULL;
+
+options = qdict_new();
+qemu_opt_foreach(create_opts, img_add_key_secrets, options, &error_abort);
+
+return img_open_file(filename, options, fmt, flags, writethrough, quiet);
+}
+
+
 static BlockBackend *img_open(bool image_opts,
   const char *filename,
   const char *fmt, int flags, bool writethrough,
@@ -351,7 +380,7 @@ static BlockBackend *img_open(bool image_opts,
 }
 blk = img_open_opts(filename, opts, flags, writethrough, quiet);
 } else {
-blk = img_open_file(filename, fmt, flags, writethrough, quiet);
+blk = img_open_file(filename, NULL, fmt, flags, writethrough, quiet);
 }
 return blk;
 }
@@ -2256,8 +2285,8 @@ static int img_convert(int argc, char **argv)
  * That has to wait for bdrv_create to be improved
  * to allow filenames in option syntax
  */
-s.target = img_open_file(out_filename, out_fmt, flags,
- writethrough, quiet);
+s.target = img_open_new_file(out_filename, opts, out_fmt,
+ flags, writethrough, quiet);
 }
 if (!s.target) {
 ret = -1;
@@ -4275,7 +4304,7 @@ static int img_dd(int argc, char **argv)
  * with the bdrv_create() call above which does not
  * support image-opts style.
  */
-blk2 = img_open_file(out.filename, out_fmt, BDRV_O_RDWR,
+blk2 = img_open_file(out.filename, NULL, out_fmt, BDRV_O_RDWR,
  false, false);
 
 if (!blk2) {
-- 
2.9.3

[Qemu-block] [PATCH v8 1/4] qemu-img: add support for --object with 'dd' command

[Daniel P. Berrange]

The qemu-img dd command added --image-opts support, but missed
the corresponding --object support. This prevented passing
secrets (eg auth passwords) needed by certain disk images.

Reviewed-by: Fam Zheng 
Reviewed-by: Max Reitz 
Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrange 
---
 qemu-img.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index c719636..34f49c2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4086,6 +4086,7 @@ static int img_dd(int argc, char **argv)
 };
 const struct option long_options[] = {
 { "help", no_argument, 0, 'h'},
+{ "object", required_argument, 0, OPTION_OBJECT},
 { "image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
 { 0, 0, 0, 0 }
 };
@@ -4110,6 +4111,15 @@ static int img_dd(int argc, char **argv)
 case 'h':
 help();
 break;
+case OPTION_OBJECT: {
+QemuOpts *opts;
+opts = qemu_opts_parse_noisily(&qemu_object_opts,
+   optarg, true);
+if (!opts) {
+ret = -1;
+goto out;
+}
+}   break;
 case OPTION_IMAGE_OPTS:
 image_opts = true;
 break;
@@ -4154,6 +4164,14 @@ static int img_dd(int argc, char **argv)
 ret = -1;
 goto out;
 }
+
+if (qemu_opts_foreach(&qemu_object_opts,
+  user_creatable_add_opts_foreach,
+  NULL, NULL)) {
+ret = -1;
+goto out;
+}
+
 blk1 = img_open(image_opts, in.filename, fmt, 0, false, false);
 
 if (!blk1) {
-- 
2.9.3

[Qemu-block] [PATCH v8 3/4] qemu-img: introduce --target-image-opts for 'convert' command

[Daniel P. Berrange]

The '--image-opts' flag indicates whether the source filename
includes options. The target filename has to remain in the
plain filename format though, since it needs to be passed to
bdrv_create().  When using --skip-create though, it would be
possible to use image-opts syntax. This adds --target-image-opts
to indicate that the target filename includes options. Currently
this mandates use of the --skip-create flag too.

Signed-off-by: Daniel P. Berrange 
---
 qemu-img-cmds.hx |  4 +--
 qemu-img.c   | 83 ++--
 qemu-img.texi| 12 ++--
 3 files changed, 69 insertions(+), 30 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index bf4ce59..c97572e 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -40,9 +40,9 @@ STEXI
 ETEXI
 
 DEF("convert", img_convert,
-"convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] 
[-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-s 
snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] 
[-W] filename [filename2 [...]] output_filename")
+"convert [--object objectdef] [--image-opts] [--target-image-opts] [-c] 
[-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B 
backing_file] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S 
sparse_size] [-m num_coroutines] [-W] filename [filename2 [...]] 
output_filename")
 STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] 
[-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B 
@var{backing_file}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l 
@var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] 
@var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] 
[-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O 
@var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-s 
@var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m 
@var{num_coroutines}] [-W] @var{filename} [@var{filename2} [...]] 
@var{output_filename}
 ETEXI
 
 DEF("dd", img_dd,
diff --git a/qemu-img.c b/qemu-img.c
index d8fdcb1..bb79cfb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -59,6 +59,7 @@ enum {
 OPTION_PATTERN = 260,
 OPTION_FLUSH_INTERVAL = 261,
 OPTION_NO_DRAIN = 262,
+OPTION_TARGET_IMAGE_OPTS = 263,
 };
 
 typedef enum OutputFormat {
@@ -1889,10 +1890,10 @@ static int convert_do_copy(ImgConvertState *s)
 static int img_convert(int argc, char **argv)
 {
 int c, bs_i, flags, src_flags = 0;
-const char *fmt = NULL, *out_fmt = "raw", *cache = "unsafe",
+const char *fmt = NULL, *out_fmt = NULL, *cache = "unsafe",
*src_cache = BDRV_DEFAULT_CACHE, *out_baseimg = NULL,
*out_filename, *out_baseimg_param, *snapshot_name = NULL;
-BlockDriver *drv, *proto_drv;
+BlockDriver *drv = NULL, *proto_drv = NULL;
 BlockDriverInfo bdi;
 BlockDriverState *out_bs;
 QemuOpts *opts = NULL, *sn_opts = NULL;
@@ -1900,7 +1901,7 @@ static int img_convert(int argc, char **argv)
 char *options = NULL;
 Error *local_err = NULL;
 bool writethrough, src_writethrough, quiet = false, image_opts = false,
- skip_create = false, progress = false;
+ skip_create = false, progress = false, tgt_image_opts = false;
 int64_t ret = -EINVAL;
 
 ImgConvertState s = (ImgConvertState) {
@@ -1916,6 +1917,7 @@ static int img_convert(int argc, char **argv)
 {"help", no_argument, 0, 'h'},
 {"object", required_argument, 0, OPTION_OBJECT},
 {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+{"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
 {0, 0, 0, 0}
 };
 c = getopt_long(argc, argv, ":hf:O:B:ce6o:s:l:S:pt:T:qnm:W",
@@ -2033,9 +2035,16 @@ static int img_convert(int argc, char **argv)
 case OPTION_IMAGE_OPTS:
 image_opts = true;
 break;
+case OPTION_TARGET_IMAGE_OPTS:
+tgt_image_opts = true;
+break;
 }
 }
 
+if (!out_fmt && !tgt_image_opts) {
+out_fmt = "raw";
+}
+
 if (qemu_opts_foreach(&qemu_object_opts,
   user_creatable_add_opts_foreach,
   NULL, NULL)) {
@@ -2047,12 +2056,22 @@ static int img_convert(int argc, char **argv)
 goto fail_getopt;
 }
 
+if (tgt_image_opts && !skip_create) {
+error_report("--target-image-opts requires use of -n flag");
+goto fail_getopt;
+}
+
 s.src_num = argc - optind - 1;
 out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
 
 if (options && has_help_option(options)) {
-ret = print_block_option_help(out_filename, out_fmt);
-goto fail_getopt;
+if (out_fmt) {
+ret = pr

Re: [Qemu-block] [PATCH v7 3/4] qemu-img: introduce --target-image-opts for 'convert' command

[Daniel P. Berrange]

On Wed, May 03, 2017 at 09:50:49PM +0200, Max Reitz wrote:
> On 02.05.2017 16:47, Daniel P. Berrange wrote:
> > The '--image-opts' flag indicates whether the source filename
> > includes options. The target filename has to remain in the
> > plain filename format though, since it needs to be passed to
> > bdrv_create().  When using --skip-create though, it would be
> > possible to use image-opts syntax. This adds --target-image-opts
> > to indicate that the target filename includes options. Currently
> > this mandates use of the --skip-create flag too.
> > 
> > Reviewed-by: Fam Zheng 
> > Reviewed-by: Eric Blake 
> 
> Sure you want to keep this, considering that there are quite some
> changes since v5?
> 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  qemu-img-cmds.hx |  4 +--
> >  qemu-img.c   | 77 
> > +---
> >  qemu-img.texi| 12 +++--
> >  3 files changed, 63 insertions(+), 30 deletions(-)
> 
> [...]
> 
> > diff --git a/qemu-img.c b/qemu-img.c
> > index d8fdcb1..94c8cea 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> 
> [...]
> 
> > @@ -1900,7 +1901,7 @@ static int img_convert(int argc, char **argv)
> >  char *options = NULL;
> >  Error *local_err = NULL;
> >  bool writethrough, src_writethrough, quiet = false, image_opts = false,
> > - skip_create = false, progress = false;
> > +skip_create = false, progress = false, tgt_image_opts = false;
> 
> Not sure about the indentation here. (I personally don't like spanning
> the declaration over multiple lines in the first place, but that's a
> different topic.) Indenting consecutive lines by four spaces is
> standard, but the indentation by five spaces had a reason.
> 
> I guess I'd personally rather keep the five-space indentation...

This change was just automatic reindent by the editor, I'll put it
back to 5.

> 
> >  int64_t ret = -EINVAL;
> >  
> >  ImgConvertState s = (ImgConvertState) {
> 
> [...]
> 
> > @@ -2047,12 +2056,22 @@ static int img_convert(int argc, char **argv)
> >  goto fail_getopt;
> >  }
> >  
> > +if (tgt_image_opts && !skip_create) {
> > +error_report("--target-image-opts requires use of -n flag");
> > +goto fail_getopt;
> > +}
> > +
> >  s.src_num = argc - optind - 1;
> >  out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
> >  
> >  if (options && has_help_option(options)) {
> > -ret = print_block_option_help(out_filename, out_fmt);
> > -goto fail_getopt;
> > +if (out_fmt) {
> > +ret = print_block_option_help(out_filename, out_fmt);
> > +goto out;
> 
> Shouldn't this remain goto fail_getopt;?

Yes

> 
> > +} else {
> > +error_report("Option help requires a format be specified");
> > +goto fail_getopt;
> > +}
> >  }
> >  
> >  if (s.src_num < 1) {
> 
> [...]
> 
> Why did you remove the compress &&
> !out_bs->drv->bdrv_co_pwritev_compressed check? I liked it. :-(

That's a rebase / conflict resolution mistake

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

Re: [Qemu-block] [Qemu-devel] [PATCH] block: Simplify BDRV_BLOCK_RAW recursion

[Fam Zheng]

On Thu, 05/04 12:37, Eric Blake wrote:
> Since we are already in coroutine context during the body of
> bdrv_co_get_block_status(), we can shave off a few layers of
> wrappers when recursing to query the protocol when a format driver
> returned BDRV_BLOCK_RAW.
> 
> Note that we are already using the correct recursion later on in
> the same function, when probing whether the protocol layer is sparse
> in order to find out if we can add BDRV_BLOCK_ZERO to an existing
> BDRV_BLOCK_DATA|BDRV_BLOCK_OFFSET_VALID.
> 
> Signed-off-by: Eric Blake 
> ---
>  block/io.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 40bd94f..fdd7485 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1784,8 +1784,8 @@ static int64_t coroutine_fn 
> bdrv_co_get_block_status(BlockDriverState *bs,
> 
>  if (ret & BDRV_BLOCK_RAW) {
>  assert(ret & BDRV_BLOCK_OFFSET_VALID);
> -ret = bdrv_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
> -*pnum, pnum, file);
> +ret = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
> +   *pnum, pnum, file);
>  goto out;
>  }
> 
> -- 
> 2.9.3
> 
> 

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [Qemu-devel] migration: add incremental drive-mirror and blockdev-mirror with dirtymap

[Daniel Kučera]

2017-05-08 19:29 GMT+02:00 John Snow :

>
>
> On 05/04/2017 03:45 AM, Daniel Kučera wrote:
> >
> > 2017-05-04 1:44 GMT+02:00 John Snow  > >:
> >
> >
> >
> > On 05/03/2017 03:56 AM, Daniel Kučera wrote:
> > > Hi all,
> > >
> > > this patch adds possibility to start mirroring since specific
> dirtyblock
> > > bitmap.
> > > The use-case is, for live migrations with ZFS volume used as block
> device:
> > > 1. make dirtyblock bitmap in qemu
> >
> > A "block dirty bitmap," I assume you mean. Through which interface?
> > "block dirty bitmap add" via QMP?
> >
> >
> > Yes.
> >
> >
> > > 2. make ZFS volume snapshot
> >
> > ZFS Volume Snapshot is going to be a filesystem-level operation,
> isn't
> > it? That is, creating this snapshot will necessarily create new dirty
> > sectors, yes?
> >
> >
> > No, we are using "zfs volumes" which are block devices (similar to LVM)
> >
> > # blockdev --report /dev/zstore/storage4
> > RORA   SSZ   BSZ   StartSecSize   Device
> > rw   256   512  4096  0 42949672960   /dev/zstore/storage4
> >
> > -drive
> > file=/dev/zstore/storage4,format=raw,if=none,id=drive-
> scsi0-0-0-0,cache=none,discard=unmap
> >
> >
>
> Ah, I see. Clearly I don't know much about ZFS in practice.
>
> > > 3. zfs send/receive the snapshot to target machine
> >
> > Why? Is this an attempt to make the process faster?
> >
> >
> > This preserves and transfers the whole chain of snapshots to destination
> > host, not only current state as it would be with drive-mirror sync: full
> >
> >
> >
> > > 4. start mirroring to destination with block map from step 1
> >
> > This makes me a little nervous: what guarantees do you have that the
> > bitmap and the ZFS snapshot were synchronous?
> >
> >
> > It doesn't have to be synchronous (or atomic). The "block dirty bitmap"
> > just needs to be done prior to ZFS snapshot. Those few writes done in
> > the meantime don't hurt to be done twice.
> >
>
> Hm, I suppose that's right, pending cache issues, perhaps?
>
> (1) Write occurs; cached
> (2) Bitmap is added
> (3) Write occurs, cached
> (4) ZFS snapshot is taken
> (5) Data is flushed to backing storage.
>
> Now, the ZFS snapshot is migrated, but is missing the writes that
> occurred in (1) and (3).
>
> Next, we mirror the data in the bitmap, but it only includes the data
> from (3).
>
> The target now appears to be missing the write from (1) -- maybe,
> depending on how the volume snapshot occurs.
>

Yes, that's why I'm using cache=none. Libvirt doesn't allow you to migrate
VM which uses drive cache anyway (unless you specify flag unsafe).


>
> >
> >
> > > 5. live migrate VM state to destination
> > >
> > > The point is, that I'm not able to live stream zfs changed data to
> > > destination
> > > to ensure same volume state in the moment of switchover of
> migrated VM
> > > to the new hypervisor.
> >
> > I'm a little concerned about the mixing of filesystem and block level
> > snapshots...
> >
> >
> > As I explained above, ZFS snapshots are also block level.
> >
> >
> >
> > >
> > >
> > > From 7317d731d51c5d743d7a4081b368f0a6862856d7 Mon Sep 17 00:00:00
> 2001
> >
> > What happened to your timestamp?
> >
> > > From: Daniel Kucera  daniel.kuc...@gmail.com>>
> > > Date: Tue, 2 May 2017 15:00:39 +
> > > Subject: [PATCH] migration: add incremental drive-mirror and
> blockdev-mirror
> >
> > Your actual email subject here, however, is missing the [PATCH] tag,
> > which is useful for it getting picked up by the patchew build bot.
> >
> > >  with dirtymap added parameter bitmap which will be used instead
> > of newly
> > >  created dirtymap in mirror_start_job
> > >
> > > Signed-off-by: Daniel Kucera  > >
> > > ---
> > >  block/mirror.c| 41
> > -
> > >  blockdev.c|  6 +-
> > >  include/block/block_int.h |  4 +++-
> > >  qapi/block-core.json  | 12 ++--
> > >  4 files changed, 42 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/block/mirror.c b/block/mirror.c
> > > index 9f5eb69..02b2f69 100644
> > > --- a/block/mirror.c
> > > +++ b/block/mirror.c
> > > @@ -49,7 +49,7 @@ typedef struct MirrorBlockJob {
> > >  BlockDriverState *to_replace;
> > >  /* Used to block operations on the drive-mirror-replace
> target */
> > >  Error *replace_blocker;
> > > -bool is_none_mode;
> > > +MirrorSyncMode sync_mode;
> > >  BlockMirrorBackingMode backing_mode;
> > >  BlockdevOnError on_source_error, on_target_error;
> > >  bool synced;
> > > @@ -523,7 +523,9 @@ static void mirror_exit(BlockJob *job, void
> > *opaque)
> > >  bdrv_child_try_set_perm(mirror_top_bs->backing, 0,
> BLK_PERM_ALL

Re: [Qemu-block] [Qemu-devel] [PATCH v5 06/10] qobject: Use simpler QDict/QList scalar insertion macros

[Markus Armbruster]

Alberto Garcia  writes:

> On Fri 28 Apr 2017 10:33:36 AM CEST, Markus Armbruster wrote:
>>> v5: rebase to master (Coccinelle found a couple new spots), squash 3
>>> patches into 1, adjust R-b to only list Markus (while there were other
>>> reviews on the pre-squashed patches, Markus was the only one on all 3)
>>
>> The block: part had
>>
>> Acked-by: Richard W.M. Jones 
>> Reviewed-by: Stefan Hajnoczi 
>> Reviewed-by: Alberto Garcia 
>>
>> The tests and qobject parts had
>>
>> Reviewed-by: Philippe Mathieu-Daudé 
>>
>> Richard, Stefan, Alberto, Philippe, let me know if you'd like me to
>> convert your R-by of parts to an Acked-by of the combined patch.  Feel
>> free to review the combined patch, of course.
>
> You can keep my R-by, I just reviewed the combined patch.

Done.  Thanks!