[Qemu-block] [PATCH] iscsi: Add chap and "initiator-name" etc as per drive options

[Fam Zheng]

Previously we use "-iscsi id=target-iqn,user=foo,password=bar,..." to
specify iscsi connection parameters, unfortunately it doesn't work with
qemu-img.

This patch adds per drive options to iscsi driver so that at least
qemu-img can use the "json:{...}" filename magic.

Signed-off-by: Fam Zheng 
---
 block/iscsi.c | 83 +--
 1 file changed, 64 insertions(+), 19 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 5002916..9efb9ec 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1011,7 +1011,9 @@ retry:
 return 0;
 }
 
-static void parse_chap(struct iscsi_context *iscsi, const char *target,
+static void parse_chap(struct iscsi_context *iscsi,
+   QemuOpts *img_opts,
+   const char *target,
Error **errp)
 {
 QemuOptsList *list;
@@ -1025,19 +1027,22 @@ static void parse_chap(struct iscsi_context *iscsi, 
const char *target,
 }
 
 opts = qemu_opts_find(list, target);
-if (opts == NULL) {
+if (!opts) {
 opts = QTAILQ_FIRST(>head);
-if (!opts) {
-return;
-}
 }
 
-user = qemu_opt_get(opts, "user");
+user = qemu_opt_get(img_opts, "user");
+if (!user && opts) {
+user = qemu_opt_get(opts, "user");
+}
 if (!user) {
 return;
 }
 
-password = qemu_opt_get(opts, "password");
+password = qemu_opt_get(img_opts, "password");
+if (!password && opts) {
+password = qemu_opt_get(opts, "password");
+}
 if (!password) {
 error_setg(errp, "CHAP username specified but no password was given");
 return;
@@ -1048,13 +1053,20 @@ static void parse_chap(struct iscsi_context *iscsi, 
const char *target,
 }
 }
 
-static void parse_header_digest(struct iscsi_context *iscsi, const char 
*target,
+static void parse_header_digest(struct iscsi_context *iscsi,
+QemuOpts *img_opts,
+const char *target,
 Error **errp)
 {
 QemuOptsList *list;
 QemuOpts *opts;
 const char *digest = NULL;
 
+digest = qemu_opt_get(img_opts, "header-digest");
+if (digest) {
+goto found;
+}
+
 list = qemu_find_opts("iscsi");
 if (!list) {
 return;
@@ -1073,6 +1085,7 @@ static void parse_header_digest(struct iscsi_context 
*iscsi, const char *target,
 return;
 }
 
+found:
 if (!strcmp(digest, "CRC32C")) {
 iscsi_set_header_digest(iscsi, ISCSI_HEADER_DIGEST_CRC32C);
 } else if (!strcmp(digest, "NONE")) {
@@ -1086,7 +1099,7 @@ static void parse_header_digest(struct iscsi_context 
*iscsi, const char *target,
 }
 }
 
-static char *parse_initiator_name(const char *target)
+static char *parse_initiator_name(QemuOpts *img_opts, const char *target)
 {
 QemuOptsList *list;
 QemuOpts *opts;
@@ -1094,6 +1107,11 @@ static char *parse_initiator_name(const char *target)
 char *iscsi_name;
 UuidInfo *uuid_info;
 
+name = qemu_opt_get(img_opts, "initiator-name");
+if (name) {
+return g_strdup(name);
+}
+
 list = qemu_find_opts("iscsi");
 if (list) {
 opts = qemu_opts_find(list, target);
@@ -1120,12 +1138,17 @@ static char *parse_initiator_name(const char *target)
 return iscsi_name;
 }
 
-static int parse_timeout(const char *target)
+static int parse_timeout(QemuOpts *img_opts, const char *target)
 {
 QemuOptsList *list;
 QemuOpts *opts;
 const char *timeout;
 
+timeout = qemu_opt_get(img_opts, "iscsi");
+if (timeout) {
+goto out;
+}
+
 list = qemu_find_opts("iscsi");
 if (list) {
 opts = qemu_opts_find(list, target);
@@ -1134,13 +1157,14 @@ static int parse_timeout(const char *target)
 }
 if (opts) {
 timeout = qemu_opt_get(opts, "timeout");
-if (timeout) {
-return atoi(timeout);
-}
 }
 }
-
-return 0;
+out:
+if (timeout) {
+return atoi(timeout);
+} else {
+return 0;
+}
 }
 
 static void iscsi_nop_timed_event(void *opaque)
@@ -1229,6 +1253,27 @@ static QemuOptsList runtime_opts = {
 .name = "filename",
 .type = QEMU_OPT_STRING,
 .help = "URL to the iscsi image",
+},{
+.name = "user",
+.type = QEMU_OPT_STRING,
+.help = "username for CHAP authentication to target",
+},{
+.name = "password",
+.type = QEMU_OPT_STRING,
+.help = "password for CHAP authentication to target",
+},{
+.name = "header-digest",
+.type = QEMU_OPT_STRING,
+.help = "HeaderDigest setting. "
+"{CRC32C|CRC32C-NONE|NONE-CRC32C|NONE}",
+},{
+.name = "initiator-name",
+.type = QEMU_OPT_STRING,
+.help = "Initiator iqn 

Re: [Qemu-block] [PATCH] ide: fix ATAPI command permissions

[Michael Tokarev]

09.09.2015 19:28, John Snow wrote:
> We're a little too lenient with what we'll let an ATAPI drive handle.
> Clamp down on the IDE command execution table to remove CD_OK permissions
> from commands that are not and have never been ATAPI commands.

FWIW, this issue has been assigned CVE-2015-6855 identifier, which
can be reflected in the commit message when applying.  Since this
issue has security impact, it might be a good idea to add

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

Thanks,

/mjt

Re: [Qemu-block] [PATCH v3 1/4] block: rename BlockdevSnapshot to BlockdevSnapshotSync

[Max Reitz]

On 10.09.2015 15:39, Alberto Garcia wrote:
> We will introduce the 'blockdev-snapshot' command that will require
> its own struct for the parameters, so we need to rename this one in
> order to avoid name clashes.
> 
> Signed-off-by: Alberto Garcia 
> Reviewed-by: Eric Blake 
> ---
>  blockdev.c   | 2 +-
>  qapi-schema.json | 2 +-
>  qapi/block-core.json | 8 
>  3 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 4/4] block: add tests for the 'blockdev-snapshot' command

[Eric Blake]

On 09/10/2015 07:39 AM, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia 
> ---
>  tests/qemu-iotests/085 | 97 
> +++---
>  tests/qemu-iotests/085.out | 34 +++-
>  2 files changed, 123 insertions(+), 8 deletions(-)
> 

>  
> +# ${1}: unique identifier for the snapshot filename
> +# ${2}: true: ignore backing images (default); false: don't ignore them
> +function add_snapshot_image()
> +{
> +base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
> +snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
> +_make_test_img -b "${base_image}" "$size"
> +mv "${TEST_IMG}" "${snapshot_file}"
> +cmd="{ 'execute': 'blockdev-add', 'arguments':
> +   { 'options':
> + { 'driver': 'qcow2', 'node-name': 'snap_"${1}"',
> +   'ignore-backing': "${2:-true}", 'file':

Needs to be reworked to use 'backing':'' instead of 'ignore-backing'.

But overall looks like a sane set of tests to cover both positive and
negative expected behavior.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v4 29/38] blockdev: Add blockdev-insert-medium

[Max Reitz]

On 11.09.2015 09:30, Wen Congyang wrote:
> On 09/11/2015 03:09 AM, Max Reitz wrote:
>> On 10.09.2015 03:12, Wen Congyang wrote:
>>> On 09/09/2015 08:59 PM, Max Reitz wrote:
 On 09.09.2015 12:01, Wen Congyang wrote:
> On 09/09/2015 05:20 AM, Max Reitz wrote:
>> On 08.09.2015 11:13, Wen Congyang wrote:
>>> On 07/21/2015 01:45 AM, Max Reitz wrote:
 And a helper function for that, which directly takes a pointer to the
 BDS to be inserted instead of its node-name (which will be used for
 implementing 'change' using blockdev-insert-medium).

 Signed-off-by: Max Reitz 
 ---
  blockdev.c   | 48 
 
  qapi/block-core.json | 17 +
  qmp-commands.hx  | 37 +
  3 files changed, 102 insertions(+)

 diff --git a/blockdev.c b/blockdev.c
 index 481760a..a80d0e2 100644
 --- a/blockdev.c
 +++ b/blockdev.c
 @@ -2164,6 +2164,54 @@ void qmp_blockdev_remove_medium(const char 
 *device, Error **errp)
  }
  }
  
 +static void qmp_blockdev_insert_anon_medium(const char *device,
 +BlockDriverState *bs, 
 Error **errp)
 +{
 +BlockBackend *blk;
 +bool has_device;
 +
 +blk = blk_by_name(device);
 +if (!blk) {
 +error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
 +  "Device '%s' not found", device);
 +return;
 +}
 +
 +/* For BBs without a device, we can exchange the BDS tree at will 
 */
 +has_device = blk_get_attached_dev(blk);
 +
 +if (has_device && !blk_dev_has_removable_media(blk)) {
 +error_setg(errp, "Device '%s' is not removable", device);
 +return;
 +}
 +
 +if (has_device && !blk_dev_is_tray_open(blk)) {
 +error_setg(errp, "Tray of device '%s' is not open", device);
 +return;
 +}
 +
 +if (blk_bs(blk)) {
 +error_setg(errp, "There already is a medium in device '%s'", 
 device);
 +return;
 +}
 +
 +blk_insert_bs(blk, bs);
 +}
 +
 +void qmp_blockdev_insert_medium(const char *device, const char 
 *node_name,
 +Error **errp)
 +{
 +BlockDriverState *bs;
 +
 +bs = bdrv_find_node(node_name);
 +if (!bs) {
 +error_setg(errp, "Node '%s' not found", node_name);
 +return;
 +}
>>>
>>> Hmm, it is OK if the bs is not top BDS?
>>
>> I think so, yes. Generally, there's probably no reason to do that, but I
>> don't know why we should not allow that case. For instance, you might
>> want to make a backing file available read-only somewhere.
>>
>> It should be impossible to make it available writable, and it should not
>> be allowed to start a block-commit operation while the backing file can
>> be accessed by the guest, but this should be achieved using op blockers.
>>
>> What we need for this to work are fine-grained op blockers, I think. But
>> working around that for now by only allowing to insert top BDS won't
>> work, since you can still start block jobs which target top BDS, too
>> (e.g. blockdev-backup can write to a BDS/BB that is visible to the 
>> guest).
>>
>> All in all, I think it's fine to insert non-top BDS, but we should
>> definitely worry about which exact BDS one can insert once we have
>> fine-grained op blockers.
>
> A BDS can be written by its parent, its block backend, a block job..
> So I think we should have some way to avoid more than two sources writing
> to it, otherwise the data may be corrupted.

 Yes, and that would be op blockers.

 As I said, using blockdev-backup you can write to a BB that can be
 written to by the guest as well. I think this is a bug, but it is a bug
 that needs to be fixed by having better op blockers in place, which Jeff
 Cody is working on.

 Regarding this series, I don't consider this to be too big of an issue.
 Yes, if you are working with floppy disks, you can have the case of a
 block job and the guest writing to the BDS at the same time. But I can't
 really imagine who would use floppy disks and block jobs at the same
 time (people who still use floppy disks for their VMs don't strike me as
 the kind of people who use the management features of qemu, especially
 not for those floppy disks).

 Other than 

Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

[Max Reitz]

On 10.09.2015 15:39, Alberto Garcia wrote:
> If set to true, the image will be opened with the BDRV_O_NO_BACKING
> flag. This is useful for creating snapshots using images opened with
> blockdev-add, since they are not supposed to have a backing image
> before the operation.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block.c  | 5 +
>  qapi/block-core.json | 6 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)

Ignorant of any possible previous discussion that might have taken
place: The documentation for @backing says it may be set to the empty
string in order to achieve exactly that.

So why do we need the new flag? Because "backing: ''" is ugly?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

[Eric Blake]

On 09/10/2015 07:39 AM, Alberto Garcia wrote:
> If set to true, the image will be opened with the BDRV_O_NO_BACKING
> flag. This is useful for creating snapshots using images opened with
> blockdev-add, since they are not supposed to have a backing image
> before the operation.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block.c  | 5 +
>  qapi/block-core.json | 6 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)

> 
> diff --git a/block.c b/block.c
> index 22d3b0e..4be32fb 100644
> --- a/block.c
> +++ b/block.c
> @@ -1469,6 +1469,11 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>  
>  assert(drvname || !(flags & BDRV_O_PROTOCOL));
>  
> +if (qdict_get_try_bool(options, "ignore-backing", false)) {
> +flags |= BDRV_O_NO_BACKING;
> +}
> +qdict_del(options, "ignore-backing");

What happens if the user specified "ignore-backing":true, "backing":...?
 Should that be a hard error?

>  { 'struct': 'BlockdevOptionsGenericCOWFormat',
>'base': 'BlockdevOptionsGenericFormat',
> -  'data': { '*backing': 'BlockdevRef' } }
> +  'data': { '*backing': 'BlockdevRef',
> +'*ignore-backing': 'bool' } }

Depending on whether the answer to my question is that we already behave
sanely and don't leave a BlockdevRef dangling if the caller mixes the
two approaches, then:
Reviewed-by: Eric Blake 

But design-wise, would it make sense to support:

"backing":null

as an explicit request to not open a backing file?  Right now, qapi does
not have a way to express 'null' as part of an alternate type; but if it
did, BlockdevRef would merely add 'null' as one of its allowed
alternates.  Then we wouldn't need ignore-backing from the QMP
perspective. But I'm still not sure how it would map to the command line
perspective.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

[Max Reitz]

On 11.09.2015 19:28, Kevin Wolf wrote:
> Am 11.09.2015 um 19:21 hat Max Reitz geschrieben:
>> On 10.09.2015 15:39, Alberto Garcia wrote:
>>> If set to true, the image will be opened with the BDRV_O_NO_BACKING
>>> flag. This is useful for creating snapshots using images opened with
>>> blockdev-add, since they are not supposed to have a backing image
>>> before the operation.
>>>
>>> Signed-off-by: Alberto Garcia 
>>> ---
>>>  block.c  | 5 +
>>>  qapi/block-core.json | 6 +-
>>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> Ignorant of any possible previous discussion that might have taken
>> place: The documentation for @backing says it may be set to the empty
>> string in order to achieve exactly that.
>>
>> So why do we need the new flag? Because "backing: ''" is ugly?
> 
> I guess it's just because you're the only one who actually reads the
> documentation. When discussing this, I didn't remember that we already
> had a way to express this (an additional bool wouldn't have been my
> favourite solution anyway). Thanks for catching this.

I read the patch, it was part of the context. ;-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

[Kevin Wolf]

Am 11.09.2015 um 19:21 hat Max Reitz geschrieben:
> On 10.09.2015 15:39, Alberto Garcia wrote:
> > If set to true, the image will be opened with the BDRV_O_NO_BACKING
> > flag. This is useful for creating snapshots using images opened with
> > blockdev-add, since they are not supposed to have a backing image
> > before the operation.
> > 
> > Signed-off-by: Alberto Garcia 
> > ---
> >  block.c  | 5 +
> >  qapi/block-core.json | 6 +-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> Ignorant of any possible previous discussion that might have taken
> place: The documentation for @backing says it may be set to the empty
> string in order to achieve exactly that.
> 
> So why do we need the new flag? Because "backing: ''" is ugly?

I guess it's just because you're the only one who actually reads the
documentation. When discussing this, I didn't remember that we already
had a way to express this (an additional bool wouldn't have been my
favourite solution anyway). Thanks for catching this.

Kevin


pgp2d4z1gM5b_.pgp
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Daniel P. Berrange]

On Fri, Sep 11, 2015 at 11:36:10AM +0200, Alberto Garcia wrote:
> On Fri 11 Sep 2015 11:14:33 AM CEST, Fam Zheng  wrote:
> 
> >> > Another advantage for bdrv_aio_poll() is, in main loop we will not
> >> > need a separate AioContext in changes like:
> >> > 
> >> > http://patchwork.ozlabs.org/patch/514968/
> >> > 
> >> > Because nested aio_poll will automatically be limited to only
> >> > process block layer events. My idea is to eventually let main loop
> >> > use aio_poll
> >> 
> >> That would be a step back.  Using GSource is useful because it lets
> >> you integrate libraries such as GTK+.
> >
> > Can we move GTK to a separate GSource thread?
> 
> I think that GTK should always run in the main thread, or at least the
> one running the default main loop / GMainContext.

In theory GTK can run from a separate thread, but my experiance with
GTK and threads is that you end up in a world of hurt if you try to
anything except use the main thread, so I'd really recommend against
it. Even if you do extensive testing locally, things still break across
different GTK versions people have, so its just not worth the pain.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

Re: [Qemu-block] [Qemu-devel] [PATCH v4 29/38] blockdev: Add blockdev-insert-medium

[Wen Congyang]

On 09/11/2015 03:09 AM, Max Reitz wrote:
> On 10.09.2015 03:12, Wen Congyang wrote:
>> On 09/09/2015 08:59 PM, Max Reitz wrote:
>>> On 09.09.2015 12:01, Wen Congyang wrote:
 On 09/09/2015 05:20 AM, Max Reitz wrote:
> On 08.09.2015 11:13, Wen Congyang wrote:
>> On 07/21/2015 01:45 AM, Max Reitz wrote:
>>> And a helper function for that, which directly takes a pointer to the
>>> BDS to be inserted instead of its node-name (which will be used for
>>> implementing 'change' using blockdev-insert-medium).
>>>
>>> Signed-off-by: Max Reitz 
>>> ---
>>>  blockdev.c   | 48 
>>> 
>>>  qapi/block-core.json | 17 +
>>>  qmp-commands.hx  | 37 +
>>>  3 files changed, 102 insertions(+)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 481760a..a80d0e2 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -2164,6 +2164,54 @@ void qmp_blockdev_remove_medium(const char 
>>> *device, Error **errp)
>>>  }
>>>  }
>>>  
>>> +static void qmp_blockdev_insert_anon_medium(const char *device,
>>> +BlockDriverState *bs, 
>>> Error **errp)
>>> +{
>>> +BlockBackend *blk;
>>> +bool has_device;
>>> +
>>> +blk = blk_by_name(device);
>>> +if (!blk) {
>>> +error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>>> +  "Device '%s' not found", device);
>>> +return;
>>> +}
>>> +
>>> +/* For BBs without a device, we can exchange the BDS tree at will 
>>> */
>>> +has_device = blk_get_attached_dev(blk);
>>> +
>>> +if (has_device && !blk_dev_has_removable_media(blk)) {
>>> +error_setg(errp, "Device '%s' is not removable", device);
>>> +return;
>>> +}
>>> +
>>> +if (has_device && !blk_dev_is_tray_open(blk)) {
>>> +error_setg(errp, "Tray of device '%s' is not open", device);
>>> +return;
>>> +}
>>> +
>>> +if (blk_bs(blk)) {
>>> +error_setg(errp, "There already is a medium in device '%s'", 
>>> device);
>>> +return;
>>> +}
>>> +
>>> +blk_insert_bs(blk, bs);
>>> +}
>>> +
>>> +void qmp_blockdev_insert_medium(const char *device, const char 
>>> *node_name,
>>> +Error **errp)
>>> +{
>>> +BlockDriverState *bs;
>>> +
>>> +bs = bdrv_find_node(node_name);
>>> +if (!bs) {
>>> +error_setg(errp, "Node '%s' not found", node_name);
>>> +return;
>>> +}
>>
>> Hmm, it is OK if the bs is not top BDS?
>
> I think so, yes. Generally, there's probably no reason to do that, but I
> don't know why we should not allow that case. For instance, you might
> want to make a backing file available read-only somewhere.
>
> It should be impossible to make it available writable, and it should not
> be allowed to start a block-commit operation while the backing file can
> be accessed by the guest, but this should be achieved using op blockers.
>
> What we need for this to work are fine-grained op blockers, I think. But
> working around that for now by only allowing to insert top BDS won't
> work, since you can still start block jobs which target top BDS, too
> (e.g. blockdev-backup can write to a BDS/BB that is visible to the guest).
>
> All in all, I think it's fine to insert non-top BDS, but we should
> definitely worry about which exact BDS one can insert once we have
> fine-grained op blockers.

 A BDS can be written by its parent, its block backend, a block job..
 So I think we should have some way to avoid more than two sources writing
 to it, otherwise the data may be corrupted.
>>>
>>> Yes, and that would be op blockers.
>>>
>>> As I said, using blockdev-backup you can write to a BB that can be
>>> written to by the guest as well. I think this is a bug, but it is a bug
>>> that needs to be fixed by having better op blockers in place, which Jeff
>>> Cody is working on.
>>>
>>> Regarding this series, I don't consider this to be too big of an issue.
>>> Yes, if you are working with floppy disks, you can have the case of a
>>> block job and the guest writing to the BDS at the same time. But I can't
>>> really imagine who would use floppy disks and block jobs at the same
>>> time (people who still use floppy disks for their VMs don't strike me as
>>> the kind of people who use the management features of qemu, especially
>>> not for those floppy disks).
>>>
>>> Other than that, this function (blockdev-insert-medium) can only be used
>>> for optical ROM devices (I don't think we have CD/DVD-RW support, do
>>> we?), so it's much less of an 

Re: [Qemu-block] [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Alberto Garcia]

On Fri 11 Sep 2015 11:14:33 AM CEST, Fam Zheng  wrote:

>> > Another advantage for bdrv_aio_poll() is, in main loop we will not
>> > need a separate AioContext in changes like:
>> > 
>> > http://patchwork.ozlabs.org/patch/514968/
>> > 
>> > Because nested aio_poll will automatically be limited to only
>> > process block layer events. My idea is to eventually let main loop
>> > use aio_poll
>> 
>> That would be a step back.  Using GSource is useful because it lets
>> you integrate libraries such as GTK+.
>
> Can we move GTK to a separate GSource thread?

I think that GTK should always run in the main thread, or at least the
one running the default main loop / GMainContext.

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Fri, 09/11 11:36, Alberto Garcia wrote:
> On Fri 11 Sep 2015 11:14:33 AM CEST, Fam Zheng  wrote:
> 
> >> > Another advantage for bdrv_aio_poll() is, in main loop we will not
> >> > need a separate AioContext in changes like:
> >> > 
> >> > http://patchwork.ozlabs.org/patch/514968/
> >> > 
> >> > Because nested aio_poll will automatically be limited to only
> >> > process block layer events. My idea is to eventually let main loop
> >> > use aio_poll
> >> 
> >> That would be a step back.  Using GSource is useful because it lets
> >> you integrate libraries such as GTK+.
> >
> > Can we move GTK to a separate GSource thread?
> 
> I think that GTK should always run in the main thread, or at least the
> one running the default main loop / GMainContext.

Yeah it's basically GMainContext staying in the main thread and
block/net/chardev I/O put in a new AioContext thread.

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Fri, 09/11 10:15, Paolo Bonzini wrote:
> 
> 
> On 09/09/2015 05:22, Fam Zheng wrote:
> > Another advantage for bdrv_aio_poll() is, in main loop we will not need
> > a separate AioContext in changes like:
> > 
> > http://patchwork.ozlabs.org/patch/514968/
> > 
> > Because nested aio_poll will automatically be limited to only process block
> > layer events. My idea is to eventually let main loop use aio_poll
> 
> That would be a step back.  Using GSource is useful because it lets you
> integrate libraries such as GTK+.

Can we move GTK to a separate GSource thread?

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 11/09/2015 11:36, Alberto Garcia wrote:
> > > > Because nested aio_poll will automatically be limited to only
> > > > process block layer events. My idea is to eventually let main loop
> > > > use aio_poll
> > > 
> > > That would be a step back.  Using GSource is useful because it lets
> > > you integrate libraries such as GTK+.
> >
> > Can we move GTK to a separate GSource thread?
>
> I think that GTK should always run in the main thread, or at least the
> one running the default main loop / GMainContext.

Agreed.  The glib main loop is a positive thing, not a negative one.

Paolo

Re: [Qemu-block] [Qemu-devel] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 09/09/2015 05:22, Fam Zheng wrote:
> Another advantage for bdrv_aio_poll() is, in main loop we will not need
> a separate AioContext in changes like:
> 
> http://patchwork.ozlabs.org/patch/514968/
> 
> Because nested aio_poll will automatically be limited to only process block
> layer events. My idea is to eventually let main loop use aio_poll

That would be a step back.  Using GSource is useful because it lets you
integrate libraries such as GTK+.

Paolo

> , which means
> we would also move chardev onto it. It would be neat to put all fds of the 
> main
> thread into a single AioContext.

[Qemu-block] [PULL 22/23] vmdk: Fix next_cluster_sector for compressed write

[Kevin Wolf]

From: Radoslav Gerganov 

When the VMDK is streamOptimized (or compressed), the
next_cluster_sector must not be incremented by a fixed number of
sectors. Instead of this, it must be rounded up to the next consecutive
sector. Fixing this results in much smaller compressed images.

Signed-off-by: Radoslav Gerganov 
Reviewed-by: Fam Zheng 
Signed-off-by: Kevin Wolf 
---
 block/vmdk.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 7bdc3d0..be0d640 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1324,8 +1324,12 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t 
cluster_offset,
 
 write_end_sector = DIV_ROUND_UP(write_offset + write_len, 
BDRV_SECTOR_SIZE);
 
-extent->next_cluster_sector = MAX(extent->next_cluster_sector,
-  write_end_sector);
+if (extent->compressed) {
+extent->next_cluster_sector = write_end_sector;
+} else {
+extent->next_cluster_sector = MAX(extent->next_cluster_sector,
+  write_end_sector);
+}
 
 if (ret != write_len) {
 ret = ret < 0 ? ret : -EIO;
-- 
1.8.3.1

[Qemu-block] [PULL 23/23] qcow2: Make qcow2_alloc_bytes() more explicit

[Kevin Wolf]

From: Max Reitz 

In case of -EAGAIN returned by update_refcount(), we should discard the
cluster offset we were trying to allocate and request a new one, because
in theory that old offset might now be taken by a refcount block.

In practice, this was not the case due to update_refcount() generally
returning strictly monotonic increasing cluster offsets. However, this
behavior is not set in stone, and it is also not obvious when looking at
qcow2_alloc_bytes() alone, so we should not rely on it.

Reported-by: Kevin Wolf 
Signed-off-by: Max Reitz 
Signed-off-by: Kevin Wolf 
---
 block/qcow2-refcount.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 5f67798..3579c4d 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -949,11 +949,17 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
 
 if (!offset || ROUND_UP(offset, s->cluster_size) != new_cluster) {
 offset = new_cluster;
+free_in_cluster = s->cluster_size;
+} else {
+free_in_cluster += s->cluster_size;
 }
 }
 
 assert(offset);
 ret = update_refcount(bs, offset, size, 1, false, QCOW2_DISCARD_NEVER);
+if (ret < 0) {
+offset = 0;
+}
 } while (ret == -EAGAIN);
 if (ret < 0) {
 return ret;
-- 
1.8.3.1

[Qemu-block] [PULL 12/23] qcow2: Move qcow2_update_options() call up

[Kevin Wolf]

qcow2_update_options() only updates some variables in BDRVQcowState and
doesn't really depend on other parts of it being initialised yet, so it
can be moved so that it immediately follows the other half of option
handling code in qcow2_open().

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
---
 block/qcow2.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 4dd0699..be6b3c2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -979,6 +979,15 @@ static int qcow2_open(BlockDriverState *bs, QDict 
*options, int flags,
 s->cache_clean_interval = cache_clean_interval;
 cache_clean_timer_init(bs, bdrv_get_aio_context(bs));
 
+/* Enable lazy_refcounts according to image and command line options */
+ret = qcow2_update_options(bs, opts, flags, errp);
+if (ret < 0) {
+goto fail;
+}
+
+qemu_opts_del(opts);
+opts = NULL;
+
 s->cluster_cache = g_malloc(s->cluster_size);
 /* one more sector for decompressed data alignment */
 s->cluster_data = qemu_try_blockalign(bs->file, QCOW_MAX_CRYPT_CLUSTERS
@@ -1063,15 +1072,6 @@ static int qcow2_open(BlockDriverState *bs, QDict 
*options, int flags,
 }
 }
 
-/* Enable lazy_refcounts according to image and command line options */
-ret = qcow2_update_options(bs, opts, flags, errp);
-if (ret < 0) {
-goto fail;
-}
-
-qemu_opts_del(opts);
-opts = NULL;
-
 #ifdef DEBUG_ALLOC
 {
 BdrvCheckResult result = {0};
-- 
1.8.3.1

[Qemu-block] [PULL 05/23] block: Drop bdrv_find_whitelisted_format()

[Kevin Wolf]

From: Max Reitz 

It is unused by now, so we can drop it.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 block.c   | 7 ---
 include/block/block.h | 2 --
 2 files changed, 9 deletions(-)

diff --git a/block.c b/block.c
index 7c61555..3de83e6 100644
--- a/block.c
+++ b/block.c
@@ -313,13 +313,6 @@ static int bdrv_is_whitelisted(BlockDriver *drv, bool 
read_only)
 return 0;
 }
 
-BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
-  bool read_only)
-{
-BlockDriver *drv = bdrv_find_format(format_name);
-return drv && bdrv_is_whitelisted(drv, read_only) ? drv : NULL;
-}
-
 typedef struct CreateCo {
 BlockDriver *drv;
 char *filename;
diff --git a/include/block/block.h b/include/block/block.h
index ab4518c..e539194 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -193,8 +193,6 @@ BlockDriver *bdrv_find_protocol(const char *filename,
 bool allow_protocol_prefix,
 Error **errp);
 BlockDriver *bdrv_find_format(const char *format_name);
-BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
-  bool readonly);
 int bdrv_create(BlockDriver *drv, const char* filename,
 QemuOpts *opts, Error **errp);
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
-- 
1.8.3.1

[Qemu-block] [PULL 10/23] qcow2: Improve error message

[Kevin Wolf]

Eric says that "any" sounds better than "either", and my non-native
feeling says the same, so let's change it.

Suggested-by: Eric Blake 
Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 9b09e01..7f06d37 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1030,7 +1030,7 @@ static int qcow2_open(BlockDriverState *bs, QDict 
*options, int flags,
 overlap_check_template = QCOW2_OL_ALL;
 } else {
 error_setg(errp, "Unsupported value '%s' for qcow2 option "
-   "'overlap-check'. Allowed are either of the following: "
+   "'overlap-check'. Allowed are any of the following: "
"none, constant, cached, all", opt_overlap_check);
 ret = -EINVAL;
 goto fail;
-- 
1.8.3.1

[Qemu-block] [PULL 03/23] block: Drop drv parameter from bdrv_open_inherit()

[Kevin Wolf]

From: Max Reitz 

Now that this parameter is effectively unused, we can drop it and just
pass NULL to bdrv_fill_options().

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 block.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index 0e1b4b4..52fa7f8 100644
--- a/block.c
+++ b/block.c
@@ -85,8 +85,7 @@ static QLIST_HEAD(, BlockDriver) bdrv_drivers =
 static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
  const char *reference, QDict *options, int flags,
  BlockDriverState *parent,
- const BdrvChildRole *child_role,
- BlockDriver *drv, Error **errp);
+ const BdrvChildRole *child_role, Error **errp);
 
 static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
 /* If non-zero, use only whitelisted block drivers */
@@ -1227,8 +1226,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options, Error **errp)
 assert(bs->backing_hd == NULL);
 ret = bdrv_open_inherit(_hd,
 *backing_filename ? backing_filename : NULL,
-NULL, options, 0, bs, _backing,
-NULL, _err);
+NULL, options, 0, bs, _backing, _err);
 if (ret < 0) {
 bdrv_unref(backing_hd);
 backing_hd = NULL;
@@ -1291,7 +1289,7 @@ BdrvChild *bdrv_open_child(const char *filename,
 
 bs = NULL;
 ret = bdrv_open_inherit(, filename, reference, image_options, 0,
-parent, child_role, NULL, errp);
+parent, child_role, errp);
 if (ret < 0) {
 goto done;
 }
@@ -1422,11 +1420,11 @@ out:
 static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
  const char *reference, QDict *options, int flags,
  BlockDriverState *parent,
- const BdrvChildRole *child_role,
- BlockDriver *drv, Error **errp)
+ const BdrvChildRole *child_role, Error **errp)
 {
 int ret;
 BlockDriverState *file = NULL, *bs;
+BlockDriver *drv = NULL;
 const char *drvname;
 Error *local_err = NULL;
 int snapshot_flags = 0;
@@ -1476,13 +1474,12 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 flags = child_role->inherit_flags(parent->open_flags);
 }
 
-ret = bdrv_fill_options(, , , drv, _err);
+ret = bdrv_fill_options(, , , NULL, _err);
 if (local_err) {
 goto fail;
 }
 
 /* Find the right image format driver */
-drv = NULL;
 drvname = qdict_get_try_str(options, "driver");
 if (drvname) {
 drv = bdrv_find_format(drvname);
@@ -1640,7 +1637,7 @@ int bdrv_open(BlockDriverState **pbs, const char 
*filename,
   const char *reference, QDict *options, int flags, Error **errp)
 {
 return bdrv_open_inherit(pbs, filename, reference, options, flags, NULL,
- NULL, NULL, errp);
+ NULL, errp);
 }
 
 typedef struct BlockReopenQueueEntry {
-- 
1.8.3.1

[Qemu-block] [PULL 11/23] qcow2: Factor out qcow2_update_options()

[Kevin Wolf]

Eventually we want to be able to change options at runtime. As a first
step towards that goal, separate some option handling code from the
general initialisation code in qcow2_open().

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
---
 block/qcow2.c | 135 +-
 1 file changed, 76 insertions(+), 59 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7f06d37..4dd0699 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -589,6 +589,80 @@ static void read_cache_sizes(BlockDriverState *bs, 
QemuOpts *opts,
 }
 }
 
+static int qcow2_update_options(BlockDriverState *bs, QemuOpts *opts,
+int flags, Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+const char *opt_overlap_check, *opt_overlap_check_template;
+int overlap_check_template = 0;
+int i;
+int ret;
+
+s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
+(s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
+
+s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
+s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
+s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
+qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
+  flags & BDRV_O_UNMAP);
+s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
+qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
+s->discard_passthrough[QCOW2_DISCARD_OTHER] =
+qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
+
+opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
+opt_overlap_check_template = qemu_opt_get(opts, 
QCOW2_OPT_OVERLAP_TEMPLATE);
+if (opt_overlap_check_template && opt_overlap_check &&
+strcmp(opt_overlap_check_template, opt_overlap_check))
+{
+error_setg(errp, "Conflicting values for qcow2 options '"
+   QCOW2_OPT_OVERLAP "' ('%s') and '" 
QCOW2_OPT_OVERLAP_TEMPLATE
+   "' ('%s')", opt_overlap_check, opt_overlap_check_template);
+ret = -EINVAL;
+goto fail;
+}
+if (!opt_overlap_check) {
+opt_overlap_check = opt_overlap_check_template ?: "cached";
+}
+
+if (!strcmp(opt_overlap_check, "none")) {
+overlap_check_template = 0;
+} else if (!strcmp(opt_overlap_check, "constant")) {
+overlap_check_template = QCOW2_OL_CONSTANT;
+} else if (!strcmp(opt_overlap_check, "cached")) {
+overlap_check_template = QCOW2_OL_CACHED;
+} else if (!strcmp(opt_overlap_check, "all")) {
+overlap_check_template = QCOW2_OL_ALL;
+} else {
+error_setg(errp, "Unsupported value '%s' for qcow2 option "
+   "'overlap-check'. Allowed are any of the following: "
+   "none, constant, cached, all", opt_overlap_check);
+ret = -EINVAL;
+goto fail;
+}
+
+s->overlap_check = 0;
+for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
+/* overlap-check defines a template bitmask, but every flag may be
+ * overwritten through the associated boolean option */
+s->overlap_check |=
+qemu_opt_get_bool(opts, overlap_bool_option_names[i],
+  overlap_check_template & (1 << i)) << i;
+}
+
+if (s->use_lazy_refcounts && s->qcow_version < 3) {
+error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
+   "qemu 1.1 compatibility level");
+ret = -EINVAL;
+goto fail;
+}
+
+ret = 0;
+fail:
+return ret;
+}
+
 static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
   Error **errp)
 {
@@ -600,8 +674,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, 
int flags,
 Error *local_err = NULL;
 uint64_t ext_end;
 uint64_t l1_vm_state_index;
-const char *opt_overlap_check, *opt_overlap_check_template;
-int overlap_check_template = 0;
 uint64_t l2_cache_size, refcount_cache_size;
 uint64_t cache_clean_interval;
 
@@ -992,69 +1064,14 @@ static int qcow2_open(BlockDriverState *bs, QDict 
*options, int flags,
 }
 
 /* Enable lazy_refcounts according to image and command line options */
-s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
-(s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
-
-s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
-s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
-s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
-qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
-  flags & BDRV_O_UNMAP);
-s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
-qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
-s->discard_passthrough[QCOW2_DISCARD_OTHER] =
-qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
-
-

[Qemu-block] [PULL 02/23] block: Drop drv parameter from bdrv_open()

[Kevin Wolf]

From: Max Reitz 

Now that this parameter is effectively unused, we can drop it and just
pass NULL on to bdrv_open_inherit().

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 block.c   | 9 -
 block/block-backend.c | 2 +-
 block/parallels.c | 2 +-
 block/qcow.c  | 2 +-
 block/qcow2.c | 6 +++---
 block/qed.c   | 2 +-
 block/sheepdog.c  | 5 ++---
 block/vdi.c   | 2 +-
 block/vhdx.c  | 2 +-
 block/vmdk.c  | 7 +++
 block/vpc.c   | 2 +-
 block/vvfat.c | 2 +-
 blockdev.c| 8 
 include/block/block.h | 3 +--
 14 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/block.c b/block.c
index d0b9101..0e1b4b4 100644
--- a/block.c
+++ b/block.c
@@ -1391,7 +1391,7 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int 
flags, Error **errp)
 bs_snapshot = bdrv_new();
 
 ret = bdrv_open(_snapshot, NULL, NULL, snapshot_options,
-flags, NULL, _err);
+flags, _err);
 if (ret < 0) {
 error_propagate(errp, local_err);
 goto out;
@@ -1637,11 +1637,10 @@ close_and_fail:
 }
 
 int bdrv_open(BlockDriverState **pbs, const char *filename,
-  const char *reference, QDict *options, int flags,
-  BlockDriver *drv, Error **errp)
+  const char *reference, QDict *options, int flags, Error **errp)
 {
 return bdrv_open_inherit(pbs, filename, reference, options, flags, NULL,
- NULL, drv, errp);
+ NULL, NULL, errp);
 }
 
 typedef struct BlockReopenQueueEntry {
@@ -3846,7 +3845,7 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 
 bs = NULL;
 ret = bdrv_open(, full_backing, NULL, backing_options,
-back_flags, NULL, _err);
+back_flags, _err);
 g_free(full_backing);
 if (ret < 0) {
 goto out;
diff --git a/block/block-backend.c b/block/block-backend.c
index aee8a12..c2e8732 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -126,7 +126,7 @@ BlockBackend *blk_new_open(const char *name, const char 
*filename,
 return NULL;
 }
 
-ret = bdrv_open(>bs, filename, reference, options, flags, NULL, errp);
+ret = bdrv_open(>bs, filename, reference, options, flags, errp);
 if (ret < 0) {
 blk_unref(blk);
 return NULL;
diff --git a/block/parallels.c b/block/parallels.c
index 046b568..5cd6ec3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -476,7 +476,7 @@ static int parallels_create(const char *filename, QemuOpts 
*opts, Error **errp)
 
 file = NULL;
 ret = bdrv_open(, filename, NULL, NULL,
-BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, _err);
+BDRV_O_RDWR | BDRV_O_PROTOCOL, _err);
 if (ret < 0) {
 error_propagate(errp, local_err);
 return ret;
diff --git a/block/qcow.c b/block/qcow.c
index 01fba54..6e35db1 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -793,7 +793,7 @@ static int qcow_create(const char *filename, QemuOpts 
*opts, Error **errp)
 
 qcow_bs = NULL;
 ret = bdrv_open(_bs, filename, NULL, NULL,
-BDRV_O_RDWR | BDRV_O_PROTOCOL, NULL, _err);
+BDRV_O_RDWR | BDRV_O_PROTOCOL, _err);
 if (ret < 0) {
 error_propagate(errp, local_err);
 goto cleanup;
diff --git a/block/qcow2.c b/block/qcow2.c
index 867b43b..a707d8d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1975,7 +1975,7 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
 
 bs = NULL;
 ret = bdrv_open(, filename, NULL, NULL, BDRV_O_RDWR | BDRV_O_PROTOCOL,
-NULL, _err);
+_err);
 if (ret < 0) {
 error_propagate(errp, local_err);
 return ret;
@@ -2038,7 +2038,7 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
 qdict_put(options, "driver", qstring_from_str("qcow2"));
 ret = bdrv_open(, filename, NULL, options,
 BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH,
-NULL, _err);
+_err);
 if (ret < 0) {
 error_propagate(errp, local_err);
 goto out;
@@ -2092,7 +2092,7 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
 qdict_put(options, "driver", qstring_from_str("qcow2"));
 ret = bdrv_open(, filename, NULL, options,
 BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
-NULL, _err);
+_err);
 if (local_err) {
 error_propagate(errp, local_err);
 goto out;
diff --git a/block/qed.c b/block/qed.c
index 954ed00..a7ff1d9 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -583,7 +583,7 @@ static int 

[Qemu-block] [PULL 09/23] qemu-io: Add command 'reopen'

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 qemu-io-cmds.c | 90 ++
 1 file changed, 90 insertions(+)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 53477e1..d6572a8 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1979,6 +1979,95 @@ static const cmdinfo_t map_cmd = {
.oneline= "prints the allocated areas of a file",
 };
 
+static void reopen_help(void)
+{
+printf(
+"\n"
+" Changes the open options of an already opened image\n"
+"\n"
+" Example:\n"
+" 'reopen -o lazy-refcounts=on' - activates lazy refcount writeback on a qcow2 
image\n"
+"\n"
+" -r, -- Reopen the image read-only\n"
+" -c, -- Change the cache mode to the given value\n"
+" -o, -- Changes block driver options (cf. 'open' command)\n"
+"\n");
+}
+
+static int reopen_f(BlockBackend *blk, int argc, char **argv);
+
+static QemuOptsList reopen_opts = {
+.name = "reopen",
+.merge_lists = true,
+.head = QTAILQ_HEAD_INITIALIZER(reopen_opts.head),
+.desc = {
+/* no elements => accept any params */
+{ /* end of list */ }
+},
+};
+
+static const cmdinfo_t reopen_cmd = {
+   .name   = "reopen",
+   .argmin = 0,
+   .argmax = -1,
+   .cfunc  = reopen_f,
+   .args   = "[-r] [-c cache] [-o options]",
+   .oneline= "reopens an image with new options",
+   .help   = reopen_help,
+};
+
+static int reopen_f(BlockBackend *blk, int argc, char **argv)
+{
+BlockDriverState *bs = blk_bs(blk);
+QemuOpts *qopts;
+QDict *opts;
+int c;
+int flags = bs->open_flags;
+
+BlockReopenQueue *brq;
+Error *local_err = NULL;
+
+while ((c = getopt(argc, argv, "c:o:r")) != -1) {
+switch (c) {
+case 'c':
+if (bdrv_parse_cache_flags(optarg, ) < 0) {
+error_report("Invalid cache option: %s", optarg);
+return 0;
+}
+break;
+case 'o':
+if (!qemu_opts_parse_noisily(_opts, optarg, 0)) {
+qemu_opts_reset(_opts);
+return 0;
+}
+break;
+case 'r':
+flags &= ~BDRV_O_RDWR;
+break;
+default:
+qemu_opts_reset(_opts);
+return qemuio_command_usage(_cmd);
+}
+}
+
+if (optind != argc) {
+qemu_opts_reset(_opts);
+return qemuio_command_usage(_cmd);
+}
+
+qopts = qemu_opts_find(_opts, NULL);
+opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
+qemu_opts_reset(_opts);
+
+brq = bdrv_reopen_queue(NULL, bs, opts, flags);
+bdrv_reopen_multiple(brq, _err);
+if (local_err) {
+error_report_err(local_err);
+}
+
+return 0;
+}
+
 static int break_f(BlockBackend *blk, int argc, char **argv)
 {
 int ret;
@@ -2266,6 +2355,7 @@ static void __attribute((constructor)) 
init_qemuio_commands(void)
 qemuio_add_command(_cmd);
 qemuio_add_command(_cmd);
 qemuio_add_command(_cmd);
+qemuio_add_command(_cmd);
 qemuio_add_command(_cmd);
 qemuio_add_command(_break_cmd);
 qemuio_add_command(_cmd);
-- 
1.8.3.1

[Qemu-block] [PULL 00/23] Block layer patches

[Kevin Wolf]

The following changes since commit 30c38c90bd3f1bb105ebc069ac1821067c980b7c:

  scripts/qemu-gdb: Add brief comment describing usage (2015-09-11 17:14:50 
+0100)

are available in the git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 1fcbcc93872953d08cd35830d1169fed19196290:

  qcow2: Make qcow2_alloc_bytes() more explicit (2015-09-11 20:03:02 +0200)


Block layer patches


Kevin Wolf (14):
  qcow2: Rename BDRVQcowState to BDRVQcow2State
  block: Allow specifying driver-specific options to reopen
  qemu-io: Remove duplicate 'open' error message
  qemu-io: Add command 'reopen'
  qcow2: Improve error message
  qcow2: Factor out qcow2_update_options()
  qcow2: Move qcow2_update_options() call up
  qcow2: Move rest of option handling to qcow2_update_options()
  qcow2: Leave s unchanged on qcow2_update_options() failure
  qcow2: Fix memory leak in qcow2_update_options() error path
  qcow2: Make qcow2_update_options() suitable for transactions
  qcow2: Support updating driver-specific options in reopen
  qemu-iotests: Reopen qcow2 with lazy-refcounts change
  qemu-iotests: More qcow2 reopen tests

Max Reitz (8):
  block: Always pass NULL as drv for bdrv_open()
  block: Drop drv parameter from bdrv_open()
  block: Drop drv parameter from bdrv_open_inherit()
  block: Drop drv parameter from bdrv_fill_options()
  block: Drop bdrv_find_whitelisted_format()
  qcow2: Make size_to_clusters() return uint64_t
  iotests: Add test for checking large image files
  qcow2: Make qcow2_alloc_bytes() more explicit

Radoslav Gerganov (1):
  vmdk: Fix next_cluster_sector for compressed write

 block.c| 150 +++---
 block/block-backend.c  |   2 +-
 block/commit.c |   4 +-
 block/parallels.c  |   2 +-
 block/qcow.c   |   2 +-
 block/qcow2-cache.c|  14 +-
 block/qcow2-cluster.c  |  76 +++
 block/qcow2-refcount.c |  74 ---
 block/qcow2-snapshot.c |  20 +-
 block/qcow2.c  | 486 +
 block/qcow2.h  |  26 +--
 block/qed.c|   2 +-
 block/sheepdog.c   |   5 +-
 block/vdi.c|   2 +-
 block/vhdx.c   |   2 +-
 block/vmdk.c   |  15 +-
 block/vpc.c|   2 +-
 block/vvfat.c  |   8 +-
 blockdev.c |  72 +++
 include/block/block.h  |   9 +-
 qemu-io-cmds.c |  90 +
 qemu-io.c  |   1 -
 tests/qemu-iotests/039 |  27 +++
 tests/qemu-iotests/039.out |  18 ++
 tests/qemu-iotests/137 | 145 ++
 tests/qemu-iotests/137.out |  42 
 tests/qemu-iotests/138 |  73 +++
 tests/qemu-iotests/138.out |   9 +
 tests/qemu-iotests/group   |   2 +
 29 files changed, 972 insertions(+), 408 deletions(-)
 create mode 100755 tests/qemu-iotests/137
 create mode 100644 tests/qemu-iotests/137.out
 create mode 100755 tests/qemu-iotests/138
 create mode 100644 tests/qemu-iotests/138.out

[Qemu-block] [PULL 21/23] iotests: Add test for checking large image files

[Kevin Wolf]

From: Max Reitz 

Add a test for checking a qcow2 file with a multiple of 2^32 clusters.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/138 | 73 ++
 tests/qemu-iotests/138.out |  9 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 83 insertions(+)
 create mode 100755 tests/qemu-iotests/138
 create mode 100644 tests/qemu-iotests/138.out

diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
new file mode 100755
index 000..a5c3464
--- /dev/null
+++ b/tests/qemu-iotests/138
@@ -0,0 +1,73 @@
+#!/bin/bash
+#
+# General test case for qcow2's image check
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=mre...@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+echo
+echo '=== Check on an image with a multiple of 2^32 clusters ==='
+echo
+
+IMGOPTS=$(_optstr_add "$IMGOPTS" "cluster_size=512") \
+_make_test_img 512
+
+# Allocate L2 table
+$QEMU_IO -c 'write 0 512' "$TEST_IMG" | _filter_qemu_io
+
+# Put the data cluster at a multiple of 2 TB, resulting in the image apparently
+# having a multiple of 2^32 clusters
+# (To be more specific: It is at 32 PB)
+poke_file "$TEST_IMG" 2048 "\x80\x80\x00\x00\x00\x00\x00\x00"
+
+# An offset of 32 PB results in qemu-img check having to allocate an in-memory
+# refcount table of 128 TB (16 bit refcounts, 512 byte clusters).
+# This should be generally too much for any system and thus fail.
+# What this test is checking is that the qcow2 driver actually tries to 
allocate
+# such a large amount of memory (and is consequently aborting) instead of 
having
+# truncated the cluster count somewhere (which would result in much less memory
+# being allocated and then a segfault occurring).
+_check_test_img
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/138.out b/tests/qemu-iotests/138.out
new file mode 100644
index 000..3fe911f
--- /dev/null
+++ b/tests/qemu-iotests/138.out
@@ -0,0 +1,9 @@
+QA output created by 138
+
+=== Check on an image with a multiple of 2^32 clusters ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=512
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qemu-img: Check failed: Cannot allocate memory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 3a6a8f0..439b1d2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -135,3 +135,4 @@
 134 rw auto quick
 135 rw auto
 137 rw auto
+138 rw auto quick
-- 
1.8.3.1

[Qemu-block] [PULL 13/23] qcow2: Move rest of option handling to qcow2_update_options()

[Kevin Wolf]

With this commit, the handling of driver-specific options in
qcow2_open() is completely separated out into qcow2_update_options().

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block/qcow2.c | 134 +-
 1 file changed, 68 insertions(+), 66 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index be6b3c2..cf6992e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -589,15 +589,77 @@ static void read_cache_sizes(BlockDriverState *bs, 
QemuOpts *opts,
 }
 }
 
-static int qcow2_update_options(BlockDriverState *bs, QemuOpts *opts,
+static int qcow2_update_options(BlockDriverState *bs, QDict *options,
 int flags, Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
+QemuOpts *opts = NULL;
 const char *opt_overlap_check, *opt_overlap_check_template;
 int overlap_check_template = 0;
+uint64_t l2_cache_size, refcount_cache_size;
+uint64_t cache_clean_interval;
 int i;
+Error *local_err = NULL;
 int ret;
 
+opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
+qemu_opts_absorb_qdict(opts, options, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto fail;
+}
+
+/* get L2 table/refcount block cache size from command line options */
+read_cache_sizes(bs, opts, _cache_size, _cache_size,
+ _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto fail;
+}
+
+l2_cache_size /= s->cluster_size;
+if (l2_cache_size < MIN_L2_CACHE_SIZE) {
+l2_cache_size = MIN_L2_CACHE_SIZE;
+}
+if (l2_cache_size > INT_MAX) {
+error_setg(errp, "L2 cache size too big");
+ret = -EINVAL;
+goto fail;
+}
+
+refcount_cache_size /= s->cluster_size;
+if (refcount_cache_size < MIN_REFCOUNT_CACHE_SIZE) {
+refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE;
+}
+if (refcount_cache_size > INT_MAX) {
+error_setg(errp, "Refcount cache size too big");
+ret = -EINVAL;
+goto fail;
+}
+
+/* alloc L2 table/refcount block cache */
+s->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
+s->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
+if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) {
+error_setg(errp, "Could not allocate metadata caches");
+ret = -ENOMEM;
+goto fail;
+}
+
+/* New interval for cache cleanup timer */
+cache_clean_interval =
+qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL, 0);
+if (cache_clean_interval > UINT_MAX) {
+error_setg(errp, "Cache clean interval too big");
+ret = -EINVAL;
+goto fail;
+}
+s->cache_clean_interval = cache_clean_interval;
+cache_clean_timer_init(bs, bdrv_get_aio_context(bs));
+
+/* Enable lazy_refcounts according to image and command line options */
 s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
 (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
 
@@ -660,6 +722,9 @@ static int qcow2_update_options(BlockDriverState *bs, 
QemuOpts *opts,
 
 ret = 0;
 fail:
+qemu_opts_del(opts);
+opts = NULL;
+
 return ret;
 }
 
@@ -670,12 +735,9 @@ static int qcow2_open(BlockDriverState *bs, QDict 
*options, int flags,
 unsigned int len, i;
 int ret = 0;
 QCowHeader header;
-QemuOpts *opts = NULL;
 Error *local_err = NULL;
 uint64_t ext_end;
 uint64_t l1_vm_state_index;
-uint64_t l2_cache_size, refcount_cache_size;
-uint64_t cache_clean_interval;
 
 ret = bdrv_pread(bs->file, 0, , sizeof(header));
 if (ret < 0) {
@@ -923,71 +985,12 @@ static int qcow2_open(BlockDriverState *bs, QDict 
*options, int flags,
 }
 }
 
-/* get L2 table/refcount block cache size from command line options */
-opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
-qemu_opts_absorb_qdict(opts, options, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-ret = -EINVAL;
-goto fail;
-}
-
-read_cache_sizes(bs, opts, _cache_size, _cache_size,
- _err);
-if (local_err) {
-error_propagate(errp, local_err);
-ret = -EINVAL;
-goto fail;
-}
-
-l2_cache_size /= s->cluster_size;
-if (l2_cache_size < MIN_L2_CACHE_SIZE) {
-l2_cache_size = MIN_L2_CACHE_SIZE;
-}
-if (l2_cache_size > INT_MAX) {
-error_setg(errp, "L2 cache size too big");
-ret = -EINVAL;
-goto fail;
-}
-
-refcount_cache_size /= s->cluster_size;
-if (refcount_cache_size < MIN_REFCOUNT_CACHE_SIZE) {
-refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE;
-}
-if (refcount_cache_size > INT_MAX) {
-error_setg(errp, "Refcount cache size too big");
-  

[Qemu-block] [PULL 06/23] qcow2: Rename BDRVQcowState to BDRVQcow2State

[Kevin Wolf]

BDRVQcowState is already used by qcow1, and gdb is always confused which
one to use. Rename the qcow2 one so they can be distinguished.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Alberto Garcia 
---
 block/qcow2-cache.c| 14 -
 block/qcow2-cluster.c  | 48 +++
 block/qcow2-refcount.c | 58 ++---
 block/qcow2-snapshot.c | 20 ++---
 block/qcow2.c  | 78 +-
 block/qcow2.h  | 22 +++---
 6 files changed, 120 insertions(+), 120 deletions(-)

diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index 046f5b8..7b14c5c 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -55,14 +55,14 @@ struct Qcow2Cache {
 static inline void *qcow2_cache_get_table_addr(BlockDriverState *bs,
 Qcow2Cache *c, int table)
 {
-BDRVQcowState *s = bs->opaque;
+BDRVQcow2State *s = bs->opaque;
 return (uint8_t *) c->table_array + (size_t) table * s->cluster_size;
 }
 
 static inline int qcow2_cache_get_table_idx(BlockDriverState *bs,
   Qcow2Cache *c, void *table)
 {
-BDRVQcowState *s = bs->opaque;
+BDRVQcow2State *s = bs->opaque;
 ptrdiff_t table_offset = (uint8_t *) table - (uint8_t *) c->table_array;
 int idx = table_offset / s->cluster_size;
 assert(idx >= 0 && idx < c->size && table_offset % s->cluster_size == 0);
@@ -73,7 +73,7 @@ static void qcow2_cache_table_release(BlockDriverState *bs, 
Qcow2Cache *c,
   int i, int num_tables)
 {
 #if QEMU_MADV_DONTNEED != QEMU_MADV_INVALID
-BDRVQcowState *s = bs->opaque;
+BDRVQcow2State *s = bs->opaque;
 void *t = qcow2_cache_get_table_addr(bs, c, i);
 int align = getpagesize();
 size_t mem_size = (size_t) s->cluster_size * num_tables;
@@ -121,7 +121,7 @@ void qcow2_cache_clean_unused(BlockDriverState *bs, 
Qcow2Cache *c)
 
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables)
 {
-BDRVQcowState *s = bs->opaque;
+BDRVQcow2State *s = bs->opaque;
 Qcow2Cache *c;
 
 c = g_new0(Qcow2Cache, 1);
@@ -172,7 +172,7 @@ static int qcow2_cache_flush_dependency(BlockDriverState 
*bs, Qcow2Cache *c)
 
 static int qcow2_cache_entry_flush(BlockDriverState *bs, Qcow2Cache *c, int i)
 {
-BDRVQcowState *s = bs->opaque;
+BDRVQcow2State *s = bs->opaque;
 int ret = 0;
 
 if (!c->entries[i].dirty || !c->entries[i].offset) {
@@ -229,7 +229,7 @@ static int qcow2_cache_entry_flush(BlockDriverState *bs, 
Qcow2Cache *c, int i)
 
 int qcow2_cache_flush(BlockDriverState *bs, Qcow2Cache *c)
 {
-BDRVQcowState *s = bs->opaque;
+BDRVQcow2State *s = bs->opaque;
 int result = 0;
 int ret;
 int i;
@@ -306,7 +306,7 @@ int qcow2_cache_empty(BlockDriverState *bs, Qcow2Cache *c)
 static int qcow2_cache_do_get(BlockDriverState *bs, Qcow2Cache *c,
 uint64_t offset, void **table, bool read_from_disk)
 {
-BDRVQcowState *s = bs->opaque;
+BDRVQcow2State *s = bs->opaque;
 int i;
 int ret;
 int lookup_index;
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 61309ae..412ee27 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -32,7 +32,7 @@
 int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
 bool exact_size)
 {
-BDRVQcowState *s = bs->opaque;
+BDRVQcow2State *s = bs->opaque;
 int new_l1_size2, ret, i;
 uint64_t *new_l1_table;
 int64_t old_l1_table_offset, old_l1_size;
@@ -148,7 +148,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t 
min_size,
 static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
 uint64_t **l2_table)
 {
-BDRVQcowState *s = bs->opaque;
+BDRVQcow2State *s = bs->opaque;
 int ret;
 
 ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset, (void**) l2_table);
@@ -163,7 +163,7 @@ static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
 #define L1_ENTRIES_PER_SECTOR (512 / 8)
 int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
 {
-BDRVQcowState *s = bs->opaque;
+BDRVQcow2State *s = bs->opaque;
 uint64_t buf[L1_ENTRIES_PER_SECTOR] = { 0 };
 int l1_start_index;
 int i, ret;
@@ -203,7 +203,7 @@ int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
 
 static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
 {
-BDRVQcowState *s = bs->opaque;
+BDRVQcow2State *s = bs->opaque;
 uint64_t old_l2_offset;
 uint64_t *l2_table = NULL;
 int64_t l2_offset;
@@ -339,7 +339,7 @@ static int count_contiguous_free_clusters(uint64_t 
nb_clusters, uint64_t *l2_tab
 /* The crypt function is compatible with the linux cryptoloop
algorithm for < 4 GB images. NOTE: out_buf == in_buf is
supported */
-int qcow2_encrypt_sectors(BDRVQcowState *s, int64_t 

[Qemu-block] [PULL 15/23] qcow2: Fix memory leak in qcow2_update_options() error path

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
---
 block/qcow2.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index c61d996..374a56d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -597,8 +597,8 @@ static int qcow2_update_options(BlockDriverState *bs, QDict 
*options,
 const char *opt_overlap_check, *opt_overlap_check_template;
 int overlap_check_template = 0;
 uint64_t l2_cache_size, refcount_cache_size;
-Qcow2Cache *l2_table_cache;
-Qcow2Cache *refcount_block_cache;
+Qcow2Cache *l2_table_cache = NULL;
+Qcow2Cache *refcount_block_cache = NULL;
 uint64_t cache_clean_interval;
 bool use_lazy_refcounts;
 int i;
@@ -735,6 +735,14 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
 
 ret = 0;
 fail:
+if (ret < 0) {
+if (l2_table_cache) {
+qcow2_cache_destroy(bs, l2_table_cache);
+}
+if (refcount_block_cache) {
+qcow2_cache_destroy(bs, refcount_block_cache);
+}
+}
 qemu_opts_del(opts);
 opts = NULL;
 
-- 
1.8.3.1

[Qemu-block] [PULL 14/23] qcow2: Leave s unchanged on qcow2_update_options() failure

[Kevin Wolf]

On return, either all new options should be applied to BDRVQcowState (on
success), or all of the old settings should be preserved (on failure).

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block/qcow2.c | 57 +++--
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index cf6992e..c61d996 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -597,7 +597,10 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
 const char *opt_overlap_check, *opt_overlap_check_template;
 int overlap_check_template = 0;
 uint64_t l2_cache_size, refcount_cache_size;
+Qcow2Cache *l2_table_cache;
+Qcow2Cache *refcount_block_cache;
 uint64_t cache_clean_interval;
+bool use_lazy_refcounts;
 int i;
 Error *local_err = NULL;
 int ret;
@@ -640,9 +643,9 @@ static int qcow2_update_options(BlockDriverState *bs, QDict 
*options,
 }
 
 /* alloc L2 table/refcount block cache */
-s->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
-s->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
-if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) {
+l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
+refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
+if (l2_table_cache == NULL || refcount_block_cache == NULL) {
 error_setg(errp, "Could not allocate metadata caches");
 ret = -ENOMEM;
 goto fail;
@@ -656,23 +659,18 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
 ret = -EINVAL;
 goto fail;
 }
-s->cache_clean_interval = cache_clean_interval;
-cache_clean_timer_init(bs, bdrv_get_aio_context(bs));
 
 /* Enable lazy_refcounts according to image and command line options */
-s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
+use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
 (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
+if (use_lazy_refcounts && s->qcow_version < 3) {
+error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
+   "qemu 1.1 compatibility level");
+ret = -EINVAL;
+goto fail;
+}
 
-s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
-s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
-s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
-qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
-  flags & BDRV_O_UNMAP);
-s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
-qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
-s->discard_passthrough[QCOW2_DISCARD_OTHER] =
-qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
-
+/* Overlap check options */
 opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
 opt_overlap_check_template = qemu_opt_get(opts, 
QCOW2_OPT_OVERLAP_TEMPLATE);
 if (opt_overlap_check_template && opt_overlap_check &&
@@ -704,6 +702,10 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
 goto fail;
 }
 
+/*
+ * Start updating fields in BDRVQcow2State.
+ * After this point no failure is allowed any more.
+ */
 s->overlap_check = 0;
 for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
 /* overlap-check defines a template bitmask, but every flag may be
@@ -713,12 +715,23 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
   overlap_check_template & (1 << i)) << i;
 }
 
-if (s->use_lazy_refcounts && s->qcow_version < 3) {
-error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
-   "qemu 1.1 compatibility level");
-ret = -EINVAL;
-goto fail;
-}
+s->l2_table_cache = l2_table_cache;
+s->refcount_block_cache = refcount_block_cache;
+
+s->use_lazy_refcounts = use_lazy_refcounts;
+
+s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
+s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
+s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
+qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
+  flags & BDRV_O_UNMAP);
+s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
+qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
+s->discard_passthrough[QCOW2_DISCARD_OTHER] =
+qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
+
+s->cache_clean_interval = cache_clean_interval;
+cache_clean_timer_init(bs, bdrv_get_aio_context(bs));
 
 ret = 0;
 fail:
-- 
1.8.3.1

[Qemu-block] [PULL 07/23] block: Allow specifying driver-specific options to reopen

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block.c   | 42 +++---
 block/commit.c|  4 ++--
 include/block/block.h |  4 +++-
 3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 3de83e6..6268e37 100644
--- a/block.c
+++ b/block.c
@@ -1636,6 +1636,9 @@ typedef struct BlockReopenQueueEntry {
  *
  * bs is the BlockDriverState to add to the reopen queue.
  *
+ * options contains the changed options for the associated bs
+ * (the BlockReopenQueue takes ownership)
+ *
  * flags contains the open flags for the associated bs
  *
  * returns a pointer to bs_queue, which is either the newly allocated
@@ -1643,18 +1646,28 @@ typedef struct BlockReopenQueueEntry {
  *
  */
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
-BlockDriverState *bs, int flags)
+BlockDriverState *bs,
+QDict *options, int flags)
 {
 assert(bs != NULL);
 
 BlockReopenQueueEntry *bs_entry;
 BdrvChild *child;
+QDict *old_options;
 
 if (bs_queue == NULL) {
 bs_queue = g_new0(BlockReopenQueue, 1);
 QSIMPLEQ_INIT(bs_queue);
 }
 
+if (!options) {
+options = qdict_new();
+}
+
+old_options = qdict_clone_shallow(bs->options);
+qdict_join(options, old_options, false);
+QDECREF(old_options);
+
 /* bdrv_open() masks this flag out */
 flags &= ~BDRV_O_PROTOCOL;
 
@@ -1666,13 +1679,15 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 }
 
 child_flags = child->role->inherit_flags(flags);
-bdrv_reopen_queue(bs_queue, child->bs, child_flags);
+/* TODO Pass down child flags (backing.*, extents.*, ...) */
+bdrv_reopen_queue(bs_queue, child->bs, NULL, child_flags);
 }
 
 bs_entry = g_new0(BlockReopenQueueEntry, 1);
 QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
 
 bs_entry->state.bs = bs;
+bs_entry->state.options = options;
 bs_entry->state.flags = flags;
 
 return bs_queue;
@@ -1725,6 +1740,7 @@ cleanup:
 if (ret && bs_entry->prepared) {
 bdrv_reopen_abort(_entry->state);
 }
+QDECREF(bs_entry->state.options);
 g_free(bs_entry);
 }
 g_free(bs_queue);
@@ -1737,7 +1753,7 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, 
Error **errp)
 {
 int ret = -1;
 Error *local_err = NULL;
-BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, bdrv_flags);
+BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags);
 
 ret = bdrv_reopen_multiple(queue, _err);
 if (local_err != NULL) {
@@ -1813,6 +1829,26 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 goto error;
 }
 
+/* Options that are not handled are only okay if they are unchanged
+ * compared to the old state. It is expected that some options are only
+ * used for the initial open, but not reopen (e.g. filename) */
+if (qdict_size(reopen_state->options)) {
+const QDictEntry *entry = qdict_first(reopen_state->options);
+
+do {
+QString *new_obj = qobject_to_qstring(entry->value);
+const char *new = qstring_get_str(new_obj);
+const char *old = qdict_get_try_str(reopen_state->bs->options,
+entry->key);
+
+if (!old || strcmp(new, old)) {
+error_setg(errp, "Cannot change the option '%s'", entry->key);
+ret = -EINVAL;
+goto error;
+}
+} while ((entry = qdict_next(reopen_state->options, entry)));
+}
+
 ret = 0;
 
 error:
diff --git a/block/commit.c b/block/commit.c
index 7312a5b..d12e26f 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -236,11 +236,11 @@ void commit_start(BlockDriverState *bs, BlockDriverState 
*base,
 
 /* convert base & overlay_bs to r/w, if necessary */
 if (!(orig_base_flags & BDRV_O_RDWR)) {
-reopen_queue = bdrv_reopen_queue(reopen_queue, base,
+reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,
  orig_base_flags | BDRV_O_RDWR);
 }
 if (!(orig_overlay_flags & BDRV_O_RDWR)) {
-reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs,
+reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,
  orig_overlay_flags | BDRV_O_RDWR);
 }
 if (reopen_queue) {
diff --git a/include/block/block.h b/include/block/block.h
index e539194..ef67353 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -147,6 +147,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, 
BlockReopenQueueEntry) BlockReopenQueue;
 typedef struct BDRVReopenState {
 BlockDriverState *bs;
 int flags;
+QDict 

[Qemu-block] [PULL 01/23] block: Always pass NULL as drv for bdrv_open()

[Kevin Wolf]

From: Max Reitz 

Change all callers of bdrv_open() to pass the driver name in the options
QDict instead of passing its BlockDriver pointer.

Signed-off-by: Max Reitz 
Reviewed-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 block.c   | 24 ++--
 block/qcow2.c | 16 -
 block/vvfat.c |  8 +--
 blockdev.c| 72 +++
 4 files changed, 57 insertions(+), 63 deletions(-)

diff --git a/block.c b/block.c
index cb5d7ae..d0b9101 100644
--- a/block.c
+++ b/block.c
@@ -1385,11 +1385,13 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int 
flags, Error **errp)
   qstring_from_str("file"));
 qdict_put(snapshot_options, "file.filename",
   qstring_from_str(tmp_filename));
+qdict_put(snapshot_options, "driver",
+  qstring_from_str("qcow2"));
 
 bs_snapshot = bdrv_new();
 
 ret = bdrv_open(_snapshot, NULL, NULL, snapshot_options,
-flags, _qcow2, _err);
+flags, NULL, _err);
 if (ret < 0) {
 error_propagate(errp, local_err);
 goto out;
@@ -3739,7 +3741,6 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 const char *backing_fmt, *backing_file;
 int64_t size;
 BlockDriver *drv, *proto_drv;
-BlockDriver *backing_drv = NULL;
 Error *local_err = NULL;
 int ret = 0;
 
@@ -3813,14 +3814,6 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 }
 
 backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
-if (backing_fmt) {
-backing_drv = bdrv_find_format(backing_fmt);
-if (!backing_drv) {
-error_setg(errp, "Unknown backing file format '%s'",
-   backing_fmt);
-goto out;
-}
-}
 
 // 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
@@ -3831,6 +3824,7 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 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,
@@ -3844,9 +3838,15 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 back_flags =
 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 = NULL;
-ret = bdrv_open(, full_backing, NULL, NULL, back_flags,
-backing_drv, _err);
+ret = bdrv_open(, full_backing, NULL, backing_options,
+back_flags, NULL, _err);
 g_free(full_backing);
 if (ret < 0) {
 goto out;
diff --git a/block/qcow2.c b/block/qcow2.c
index ea34ae2..867b43b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1873,8 +1873,10 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
  QemuOpts *opts, int version, int refcount_order,
  Error **errp)
 {
-/* Calculate cluster_bits */
 int cluster_bits;
+QDict *options;
+
+/* Calculate cluster_bits */
 cluster_bits = ctz32(cluster_size);
 if (cluster_bits < MIN_CLUSTER_BITS || cluster_bits > MAX_CLUSTER_BITS ||
 (1 << cluster_bits) != cluster_size)
@@ -2032,9 +2034,11 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
  * refcount of the cluster that is occupied by the header and the refcount
  * table)
  */
-ret = bdrv_open(, filename, NULL, NULL,
+options = qdict_new();
+qdict_put(options, "driver", qstring_from_str("qcow2"));
+ret = bdrv_open(, filename, NULL, options,
 BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH,
-_qcow2, _err);
+NULL, _err);
 if (ret < 0) {
 error_propagate(errp, local_err);
 goto out;
@@ -2084,9 +2088,11 @@ static int qcow2_create2(const char *filename, int64_t 
total_size,
 bs = NULL;
 
 /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
-ret = bdrv_open(, filename, NULL, NULL,
+options = qdict_new();
+qdict_put(options, "driver", qstring_from_str("qcow2"));
+ret = bdrv_open(, filename, NULL, options,
 BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
-_qcow2, _err);
+NULL, _err);
 if (local_err) {
 error_propagate(errp, local_err);
 

[Qemu-block] [PULL 20/23] qcow2: Make size_to_clusters() return uint64_t

[Kevin Wolf]

From: Max Reitz 

Sadly, some images may have more clusters than what can be represented
using a plain int. We should be prepared for that case (in
qcow2_check_refcounts() we actually were trying to catch that case, but
since size_to_clusters() truncated the returned value, that check never
did anything useful).

Cc: qemu-stable 
Signed-off-by: Max Reitz 
Signed-off-by: Kevin Wolf 
---
 block/qcow2-cluster.c  | 28 ++--
 block/qcow2-refcount.c | 10 +++---
 block/qcow2.h  |  6 +++---
 3 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 412ee27..6ede629 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -298,7 +298,7 @@ fail:
  * as contiguous. (This allows it, for example, to stop at the first compressed
  * cluster which may require a different handling)
  */
-static int count_contiguous_clusters(uint64_t nb_clusters, int cluster_size,
+static int count_contiguous_clusters(int nb_clusters, int cluster_size,
 uint64_t *l2_table, uint64_t stop_flags)
 {
 int i;
@@ -321,7 +321,7 @@ static int count_contiguous_clusters(uint64_t nb_clusters, 
int cluster_size,
return i;
 }
 
-static int count_contiguous_free_clusters(uint64_t nb_clusters, uint64_t 
*l2_table)
+static int count_contiguous_free_clusters(int nb_clusters, uint64_t *l2_table)
 {
 int i;
 
@@ -495,6 +495,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
 if (nb_needed > nb_available) {
 nb_needed = nb_available;
 }
+assert(nb_needed <= INT_MAX);
 
 *cluster_offset = 0;
 
@@ -530,6 +531,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
 
 l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
 *cluster_offset = be64_to_cpu(l2_table[l2_index]);
+
+/* nb_needed <= INT_MAX, thus nb_clusters <= INT_MAX, too */
 nb_clusters = size_to_clusters(s, nb_needed << 9);
 
 ret = qcow2_get_cluster_type(*cluster_offset);
@@ -960,7 +963,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t 
guest_offset,
 int l2_index;
 uint64_t cluster_offset;
 uint64_t *l2_table;
-unsigned int nb_clusters;
+uint64_t nb_clusters;
 unsigned int keep_clusters;
 int ret;
 
@@ -979,6 +982,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t 
guest_offset,
 
 l2_index = offset_to_l2_index(s, guest_offset);
 nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
+assert(nb_clusters <= INT_MAX);
 
 /* Find L2 entry for the first involved cluster */
 ret = get_cluster_table(bs, guest_offset, _table, _index);
@@ -1061,7 +1065,7 @@ out:
  * restarted, but the whole request should not be failed.
  */
 static int do_alloc_cluster_offset(BlockDriverState *bs, uint64_t guest_offset,
-uint64_t *host_offset, unsigned int *nb_clusters)
+   uint64_t *host_offset, uint64_t 
*nb_clusters)
 {
 BDRVQcow2State *s = bs->opaque;
 
@@ -1079,7 +1083,7 @@ static int do_alloc_cluster_offset(BlockDriverState *bs, 
uint64_t guest_offset,
 *host_offset = cluster_offset;
 return 0;
 } else {
-int ret = qcow2_alloc_clusters_at(bs, *host_offset, *nb_clusters);
+int64_t ret = qcow2_alloc_clusters_at(bs, *host_offset, *nb_clusters);
 if (ret < 0) {
 return ret;
 }
@@ -1115,7 +1119,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t 
guest_offset,
 int l2_index;
 uint64_t *l2_table;
 uint64_t entry;
-unsigned int nb_clusters;
+uint64_t nb_clusters;
 int ret;
 
 uint64_t alloc_cluster_offset;
@@ -1133,6 +1137,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t 
guest_offset,
 
 l2_index = offset_to_l2_index(s, guest_offset);
 nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
+assert(nb_clusters <= INT_MAX);
 
 /* Find L2 entry for the first involved cluster */
 ret = get_cluster_table(bs, guest_offset, _table, _index);
@@ -1426,7 +1431,8 @@ int qcow2_decompress_cluster(BlockDriverState *bs, 
uint64_t cluster_offset)
  * clusters.
  */
 static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
-unsigned int nb_clusters, enum qcow2_discard_type type, bool full_discard)
+ uint64_t nb_clusters, enum qcow2_discard_type 
type,
+ bool full_discard)
 {
 BDRVQcow2State *s = bs->opaque;
 uint64_t *l2_table;
@@ -1441,6 +1447,7 @@ static int discard_single_l2(BlockDriverState *bs, 
uint64_t offset,
 
 /* Limit nb_clusters to one L2 table */
 nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
+assert(nb_clusters <= INT_MAX);
 
 for (i = 0; i < nb_clusters; i++) {
 uint64_t old_l2_entry;
@@ -1503,7 +1510,7 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t 
offset,
 {
 

[Qemu-block] [PULL 17/23] qcow2: Support updating driver-specific options in reopen

[Kevin Wolf]

For updating the cache sizes, disabling lazy refcounts and updating the
clean_cache_timer there is a bit more to do than just changing the
variables, but otherwise we're all set for changing options during
bdrv_reopen().

Just implement the missing pieces and hook the functions up in
bdrv_reopen().

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block/qcow2.c | 81 ---
 1 file changed, 72 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 64ba3cb..56ad808 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -649,7 +649,24 @@ static int qcow2_update_options_prepare(BlockDriverState 
*bs,
 goto fail;
 }
 
-/* alloc L2 table/refcount block cache */
+/* alloc new L2 table/refcount block cache, flush old one */
+if (s->l2_table_cache) {
+ret = qcow2_cache_flush(bs, s->l2_table_cache);
+if (ret) {
+error_setg_errno(errp, -ret, "Failed to flush the L2 table cache");
+goto fail;
+}
+}
+
+if (s->refcount_block_cache) {
+ret = qcow2_cache_flush(bs, s->refcount_block_cache);
+if (ret) {
+error_setg_errno(errp, -ret,
+ "Failed to flush the refcount block cache");
+goto fail;
+}
+}
+
 r->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
 r->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
 if (r->l2_table_cache == NULL || r->refcount_block_cache == NULL) {
@@ -660,14 +677,15 @@ static int qcow2_update_options_prepare(BlockDriverState 
*bs,
 
 /* New interval for cache cleanup timer */
 r->cache_clean_interval =
-qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL, 0);
+qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL,
+s->cache_clean_interval);
 if (r->cache_clean_interval > UINT_MAX) {
 error_setg(errp, "Cache clean interval too big");
 ret = -EINVAL;
 goto fail;
 }
 
-/* Enable lazy_refcounts according to image and command line options */
+/* lazy-refcounts; flush if going from enabled to disabled */
 r->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
 (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
 if (r->use_lazy_refcounts && s->qcow_version < 3) {
@@ -677,6 +695,14 @@ static int qcow2_update_options_prepare(BlockDriverState 
*bs,
 goto fail;
 }
 
+if (s->use_lazy_refcounts && !r->use_lazy_refcounts) {
+ret = qcow2_mark_clean(bs);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "Failed to disable lazy refcounts");
+goto fail;
+}
+}
+
 /* Overlap check options */
 opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
 opt_overlap_check_template = qemu_opt_get(opts, 
QCOW2_OPT_OVERLAP_TEMPLATE);
@@ -741,6 +767,12 @@ static void qcow2_update_options_commit(BlockDriverState 
*bs,
 BDRVQcow2State *s = bs->opaque;
 int i;
 
+if (s->l2_table_cache) {
+qcow2_cache_destroy(bs, s->l2_table_cache);
+}
+if (s->refcount_block_cache) {
+qcow2_cache_destroy(bs, s->refcount_block_cache);
+}
 s->l2_table_cache = r->l2_table_cache;
 s->refcount_block_cache = r->refcount_block_cache;
 
@@ -751,8 +783,11 @@ static void qcow2_update_options_commit(BlockDriverState 
*bs,
 s->discard_passthrough[i] = r->discard_passthrough[i];
 }
 
-s->cache_clean_interval = r->cache_clean_interval;
-cache_clean_timer_init(bs, bdrv_get_aio_context(bs));
+if (s->cache_clean_interval != r->cache_clean_interval) {
+cache_clean_timer_del(bs);
+s->cache_clean_interval = r->cache_clean_interval;
+cache_clean_timer_init(bs, bdrv_get_aio_context(bs));
+}
 }
 
 static void qcow2_update_options_abort(BlockDriverState *bs,
@@ -1199,26 +1234,52 @@ static int qcow2_set_key(BlockDriverState *bs, const 
char *key)
 return 0;
 }
 
-/* We have no actual commit/abort logic for qcow2, but we need to write out any
- * unwritten data if we reopen read-only. */
 static int qcow2_reopen_prepare(BDRVReopenState *state,
 BlockReopenQueue *queue, Error **errp)
 {
+Qcow2ReopenState *r;
 int ret;
 
+r = g_new0(Qcow2ReopenState, 1);
+state->opaque = r;
+
+ret = qcow2_update_options_prepare(state->bs, r, state->options,
+   state->flags, errp);
+if (ret < 0) {
+goto fail;
+}
+
+/* We need to write out any unwritten data if we reopen read-only. */
 if ((state->flags & BDRV_O_RDWR) == 0) {
 ret = bdrv_flush(state->bs);
 if (ret < 0) {
-return ret;
+goto fail;
 }
 
 ret = qcow2_mark_clean(state->bs);
 if (ret < 0) {
-return ret;
+goto 

[Qemu-block] [PULL 16/23] qcow2: Make qcow2_update_options() suitable for transactions

[Kevin Wolf]

Before we can allow updating options at runtime with bdrv_reopen(), we
need to split the function into prepare/commit/abort parts.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block/qcow2.c | 113 +-
 1 file changed, 73 insertions(+), 40 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 374a56d..64ba3cb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -589,18 +589,25 @@ static void read_cache_sizes(BlockDriverState *bs, 
QemuOpts *opts,
 }
 }
 
-static int qcow2_update_options(BlockDriverState *bs, QDict *options,
-int flags, Error **errp)
+typedef struct Qcow2ReopenState {
+Qcow2Cache *l2_table_cache;
+Qcow2Cache *refcount_block_cache;
+bool use_lazy_refcounts;
+int overlap_check;
+bool discard_passthrough[QCOW2_DISCARD_MAX];
+uint64_t cache_clean_interval;
+} Qcow2ReopenState;
+
+static int qcow2_update_options_prepare(BlockDriverState *bs,
+Qcow2ReopenState *r,
+QDict *options, int flags,
+Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
 QemuOpts *opts = NULL;
 const char *opt_overlap_check, *opt_overlap_check_template;
 int overlap_check_template = 0;
 uint64_t l2_cache_size, refcount_cache_size;
-Qcow2Cache *l2_table_cache = NULL;
-Qcow2Cache *refcount_block_cache = NULL;
-uint64_t cache_clean_interval;
-bool use_lazy_refcounts;
 int i;
 Error *local_err = NULL;
 int ret;
@@ -643,27 +650,27 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
 }
 
 /* alloc L2 table/refcount block cache */
-l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
-refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
-if (l2_table_cache == NULL || refcount_block_cache == NULL) {
+r->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
+r->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
+if (r->l2_table_cache == NULL || r->refcount_block_cache == NULL) {
 error_setg(errp, "Could not allocate metadata caches");
 ret = -ENOMEM;
 goto fail;
 }
 
 /* New interval for cache cleanup timer */
-cache_clean_interval =
+r->cache_clean_interval =
 qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL, 0);
-if (cache_clean_interval > UINT_MAX) {
+if (r->cache_clean_interval > UINT_MAX) {
 error_setg(errp, "Cache clean interval too big");
 ret = -EINVAL;
 goto fail;
 }
 
 /* Enable lazy_refcounts according to image and command line options */
-use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
+r->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
 (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
-if (use_lazy_refcounts && s->qcow_version < 3) {
+if (r->use_lazy_refcounts && s->qcow_version < 3) {
 error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
"qemu 1.1 compatibility level");
 ret = -EINVAL;
@@ -702,49 +709,75 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
 goto fail;
 }
 
-/*
- * Start updating fields in BDRVQcow2State.
- * After this point no failure is allowed any more.
- */
-s->overlap_check = 0;
+r->overlap_check = 0;
 for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
 /* overlap-check defines a template bitmask, but every flag may be
  * overwritten through the associated boolean option */
-s->overlap_check |=
+r->overlap_check |=
 qemu_opt_get_bool(opts, overlap_bool_option_names[i],
   overlap_check_template & (1 << i)) << i;
 }
 
-s->l2_table_cache = l2_table_cache;
-s->refcount_block_cache = refcount_block_cache;
-
-s->use_lazy_refcounts = use_lazy_refcounts;
-
-s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
-s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
-s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
+r->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
+r->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
+r->discard_passthrough[QCOW2_DISCARD_REQUEST] =
 qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
   flags & BDRV_O_UNMAP);
-s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
+r->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
 qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
-s->discard_passthrough[QCOW2_DISCARD_OTHER] =
+r->discard_passthrough[QCOW2_DISCARD_OTHER] =
 qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
 
-s->cache_clean_interval = cache_clean_interval;
-cache_clean_timer_init(bs, 

[Qemu-block] [PULL 18/23] qemu-iotests: Reopen qcow2 with lazy-refcounts change

[Kevin Wolf]

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/039 | 27 +++
 tests/qemu-iotests/039.out | 18 ++
 2 files changed, 45 insertions(+)

diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 617f397..9e9b379 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -147,6 +147,33 @@ $PYTHON qcow2.py "$TEST_IMG".base dump-header | grep 
incompatible_features
 _check_test_img
 TEST_IMG="$TEST_IMG".base _check_test_img
 
+echo
+echo "== Changing lazy_refcounts setting at runtime =="
+
+IMGOPTS="compat=1.1,lazy_refcounts=off"
+_make_test_img $size
+
+$QEMU_IO -c "reopen -o lazy-refcounts=on" \
+ -c "write -P 0x5a 0 512" \
+ -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
+| _filter_qemu_io
+
+# The dirty bit must be set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_check_test_img
+
+IMGOPTS="compat=1.1,lazy_refcounts=on"
+_make_test_img $size
+
+$QEMU_IO -c "reopen -o lazy-refcounts=off" \
+ -c "write -P 0x5a 0 512" \
+ -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
+| _filter_qemu_io
+
+# The dirty bit must not be set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_check_test_img
+
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
index d8c5a44..03a31c5 100644
--- a/tests/qemu-iotests/039.out
+++ b/tests/qemu-iotests/039.out
@@ -74,4 +74,22 @@ incompatible_features 0x0
 incompatible_features 0x0
 No errors were found on the image.
 No errors were found on the image.
+
+== Changing lazy_refcounts setting at runtime ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+./common.config: Killed  ( exec "$QEMU_IO_PROG" 
$QEMU_IO_OPTIONS "$@" )
+incompatible_features 0x1
+ERROR cluster 5 refcount=0 reference=1
+ERROR OFLAG_COPIED data cluster: l2_entry=8005 refcount=0
+
+2 errors were found on the image.
+Data may be corrupted, or further writes to the image may corrupt it.
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+./common.config: Killed  ( exec "$QEMU_IO_PROG" 
$QEMU_IO_OPTIONS "$@" )
+incompatible_features 0x0
+No errors were found on the image.
 *** done
-- 
1.8.3.1

[Qemu-block] [PULL 08/23] qemu-io: Remove duplicate 'open' error message

[Kevin Wolf]

qemu_opts_parse_noisily() already prints an error message with the exact
reason why the parsing failed. No need to add another less specific one.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 qemu-io.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/qemu-io.c b/qemu-io.c
index f1e3a67..269f17c 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -156,7 +156,6 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
 break;
 case 'o':
 if (!qemu_opts_parse_noisily(_opts, optarg, false)) {
-printf("could not parse option list -- %s\n", optarg);
 qemu_opts_reset(_opts);
 return 0;
 }
-- 
1.8.3.1

Re: [Qemu-block] [PATCH v4 16/38] block: Add BlockBackendRootState

[Eric Blake]

On 07/20/2015 11:45 AM, Max Reitz wrote:
> This structure will store some of the state of the root BDS if the BDS
> tree is removed, so that state can be restored once a new BDS tree is
> inserted.
> 
> Signed-off-by: Max Reitz 
> ---
>  block/block-backend.c  | 37 +
>  include/block/block_int.h  | 10 ++
>  include/qemu/typedefs.h|  1 +
>  include/sysemu/block-backend.h |  2 ++
>  4 files changed, 50 insertions(+)

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

[Eric Blake]

On 09/11/2015 11:28 AM, Eric Blake wrote:

> But design-wise, would it make sense to support:
> 
> "backing":null

Just read Max's response; it sounds like we already have "backing":""
(and don't need "backing":null) for what we want. So maybe we don't need
this patch after all.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command

[Max Reitz]

On 10.09.2015 15:39, Alberto Garcia wrote:
> One of the limitations of the 'blockdev-snapshot-sync' command is that
> it does not allow passing BlockdevOptions to the newly created
> snapshots, so they are always opened using the default values.
> 
> Extending the command to allow passing options is not a practical
> solution because there is overlap between those options and some of
> the existing parameters of the command.
> 
> This patch introduces a new 'blockdev-snapshot' command with a simpler
> interface: it just takes two references to existing block devices that
> will be used as the source and target for the snapshot.
> 
> Since the main difference between the two commands is that one of them
> creates and opens the target image, while the other uses an already
> opened one, the bulk of the implementation is shared.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  blockdev.c   | 163 
> ---
>  qapi-schema.json |   2 +
>  qapi/block-core.json |  26 
>  qmp-commands.hx  |  29 +
>  4 files changed, 160 insertions(+), 60 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Fam Zheng]

On Fri, 09/11 12:39, Kevin Wolf wrote:
> Am 29.07.2015 um 06:42 hat Fam Zheng geschrieben:
> > v2: Switch to disable/enable model. [Paolo]
> > 
> > Most existing nested aio_poll()'s in block layer are inconsiderate of
> > dispatching potential new r/w requests from ioeventfds and nbd exports, 
> > which
> > might result in responsiveness issues (e.g. bdrv_drain_all will not return 
> > when
> > new requests keep coming), or even wrong semantics (e.g. qmp_transaction 
> > cannot
> > enforce atomicity due to aio_poll in bdrv_drain_all).
> > 
> > Previous attampts to address this issue include new op blocker[1], 
> > bdrv_lock[2]
> > and nested AioContext (patches not posted to qemu-devel).
> > 
> > This approach is based on the idea proposed by Paolo Bonzini. The original 
> > idea
> > is introducing "aio_context_disable_client / aio_context_enable_client to
> > filter AioContext handlers according to the "client", e.g.
> > AIO_CLIENT_DATAPLANE (ioeventfd), AIO_CLIENT_PROTOCOL, 
> > AIO_CLIENT_NBD_SERVER,
> > AIO_CLIENT_CONTEXT, ... Extend aio_set_{event_notifier,fd}_handler to pass a
> > client (type)." 
> > 
> > After this series, block layer aio_poll() will only process those "protocol"
> > fds that are used in block I/O, plus the ctx->notifier for aio_notify();  
> > other
> > aio_poll()'s keep unchanged.
> > 
> > The biggest advantage over approaches [1] and [2] is, no change is needed in
> > virtio-{blk,scsi}-dataplane code, also this doesn't depend on converting 
> > QMP to
> > coroutines.
> 
> It seems that I haven't replied on the mailing list yet, even though I
> think I already said this in person at KVM Forum: This series fixes only
> a special case of the real problem, which is that bdrv_drain/all at a
> single point doesn't make a lot of sense, but needs to be replaced by a
> whole section with exclusive access, like a bdrv_drained_begin/end pair.
> 
> To be clear: Anything that works with types of users instead of
> individual users is bound to fall short of being a complete solution. I
> don't prefer partial solutions when we know there is a bigger problem.
> 
> This series addresses your immediate need of protecting against new data
> plane requests, which it arguably achieves. The second case I always
> have in mind is Berto's case where he has multiple streaming block jobs
> in the same backing file chain [1].
> 
> This involves a bdrv_reopen() of the target BDS to make it writable, and
> bdrv_reopen() uses bdrv_drain_all() so drivers don't have to cope with
> running requests while reopening themselves. It can however involve
> nested event loops for synchronous operations like bdrv_flush(), and if
> those can process completions of block jobs, which can respond by doing
> anything to the respective node, things can go wrong.

Just to get a better idea of bdrv_drained_begin/end, could you explain how to
use the pair to fix the above problem?

> 
> You don't solve this by adding client types (then problematic request
> would be PROTOCOL in your proposal and you can never exclude that), but
> you really need to have bdrv_drained_being/end pairs, where only
> requests issued in between are processed and everything else waits.

What do you mean by "only requests issued in between are processed"? Where are
the requests from?

Fam

Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/11] aio: Introduce handler type to fix nested aio_poll for dataplane

[Kevin Wolf]

Am 11.09.2015 um 13:46 hat Fam Zheng geschrieben:
> On Fri, 09/11 12:39, Kevin Wolf wrote:
> > Am 29.07.2015 um 06:42 hat Fam Zheng geschrieben:
> > > v2: Switch to disable/enable model. [Paolo]
> > > 
> > > Most existing nested aio_poll()'s in block layer are inconsiderate of
> > > dispatching potential new r/w requests from ioeventfds and nbd exports, 
> > > which
> > > might result in responsiveness issues (e.g. bdrv_drain_all will not 
> > > return when
> > > new requests keep coming), or even wrong semantics (e.g. qmp_transaction 
> > > cannot
> > > enforce atomicity due to aio_poll in bdrv_drain_all).
> > > 
> > > Previous attampts to address this issue include new op blocker[1], 
> > > bdrv_lock[2]
> > > and nested AioContext (patches not posted to qemu-devel).
> > > 
> > > This approach is based on the idea proposed by Paolo Bonzini. The 
> > > original idea
> > > is introducing "aio_context_disable_client / aio_context_enable_client to
> > > filter AioContext handlers according to the "client", e.g.
> > > AIO_CLIENT_DATAPLANE (ioeventfd), AIO_CLIENT_PROTOCOL, 
> > > AIO_CLIENT_NBD_SERVER,
> > > AIO_CLIENT_CONTEXT, ... Extend aio_set_{event_notifier,fd}_handler to 
> > > pass a
> > > client (type)." 
> > > 
> > > After this series, block layer aio_poll() will only process those 
> > > "protocol"
> > > fds that are used in block I/O, plus the ctx->notifier for aio_notify();  
> > > other
> > > aio_poll()'s keep unchanged.
> > > 
> > > The biggest advantage over approaches [1] and [2] is, no change is needed 
> > > in
> > > virtio-{blk,scsi}-dataplane code, also this doesn't depend on converting 
> > > QMP to
> > > coroutines.
> > 
> > It seems that I haven't replied on the mailing list yet, even though I
> > think I already said this in person at KVM Forum: This series fixes only
> > a special case of the real problem, which is that bdrv_drain/all at a
> > single point doesn't make a lot of sense, but needs to be replaced by a
> > whole section with exclusive access, like a bdrv_drained_begin/end pair.
> > 
> > To be clear: Anything that works with types of users instead of
> > individual users is bound to fall short of being a complete solution. I
> > don't prefer partial solutions when we know there is a bigger problem.
> > 
> > This series addresses your immediate need of protecting against new data
> > plane requests, which it arguably achieves. The second case I always
> > have in mind is Berto's case where he has multiple streaming block jobs
> > in the same backing file chain [1].
> > 
> > This involves a bdrv_reopen() of the target BDS to make it writable, and
> > bdrv_reopen() uses bdrv_drain_all() so drivers don't have to cope with
> > running requests while reopening themselves. It can however involve
> > nested event loops for synchronous operations like bdrv_flush(), and if
> > those can process completions of block jobs, which can respond by doing
> > anything to the respective node, things can go wrong.
> 
> Just to get a better idea of bdrv_drained_begin/end, could you explain how to
> use the pair to fix the above problem?

How to use it is easy part: In bdrv_reopen_multiple(), you would replace
the existing bdrv_drain_all() with begin and you would add the
corresponding end right before the return statement.

> > You don't solve this by adding client types (then problematic request
> > would be PROTOCOL in your proposal and you can never exclude that), but
> > you really need to have bdrv_drained_being/end pairs, where only
> > requests issued in between are processed and everything else waits.
> 
> What do you mean by "only requests issued in between are processed"? Where are
> the requests from?

Generally speaking, you would have code that looks like this:

bdrv_drain_begin()
...
bdrv_something_synchronous()
...
bdrv_drain_end()

You want to process everything that is necessary for completing
bdrv_something_synchronous(), but nothing else.

The trickier question is how to implement this. I know that it's much
easier to say that your series doesn't work than actually proposing
something else that works...

One relatively obvious answer we found when we discussed this a while
back was some kind of a recursive CoRwLock (reader = in-flight request;
writer = drained section), but that requires obviously that you're
running in a coroutine if you want to do something with a drained
request queue.

I'm also not totally happy with the requirement of taking a reader lock
more or less everywhere. But I'm not sure yet if there is a good
alternative that can achieve the same.

This needs some more thought, I guess.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH] iscsi: Add chap and "initiator-name" etc as per drive options

[Eric Blake]

On 09/11/2015 12:00 AM, Fam Zheng wrote:
> Previously we use "-iscsi id=target-iqn,user=foo,password=bar,..." to
> specify iscsi connection parameters, unfortunately it doesn't work with
> qemu-img.
> 
> This patch adds per drive options to iscsi driver so that at least
> qemu-img can use the "json:{...}" filename magic.
> 
> Signed-off-by: Fam Zheng 
> ---
>  block/iscsi.c | 83 
> +--
>  1 file changed, 64 insertions(+), 19 deletions(-)

It would be nice to also add a matching BlockdevOptionsIscsi to
qapi/block-core.json, to allow setting these structured options from
QMP.  Separate patch is fine, but we need to do the work for ALL of the
remaining block devices eventually, and now that you are structuring the
command line is a good time to think about it.


>  static void iscsi_nop_timed_event(void *opaque)
> @@ -1229,6 +1253,27 @@ static QemuOptsList runtime_opts = {
>  .name = "filename",
>  .type = QEMU_OPT_STRING,
>  .help = "URL to the iscsi image",
> +},{
> +.name = "user",
> +.type = QEMU_OPT_STRING,
> +.help = "username for CHAP authentication to target",
> +},{
> +.name = "password",
> +.type = QEMU_OPT_STRING,
> +.help = "password for CHAP authentication to target",
> +},{

Also, this requires passing the password in the command line. We
_really_ need to solve the problem of allowing the password to be passed
via a fd or other QMP command, rather than on the command line.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature