Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] block: add block device shared field

[Fam Zheng]

On Tue, 09/05 10:56, Stefan Hajnoczi wrote:
> On Fri, Sep 01, 2017 at 08:10:54PM +, Brian Steffens wrote:
> > This adds a boolean option called 'shared' to block devices. It defaults
> > to off/false. When enabled for a particular block device, the 'shared' 
> > option
> > causes the block migration code to skip over syncing of that device. This
> > allows controlling exactly which block devices get synced during a 
> > migration.
> > 
> > Signed-off-by: Brian Steffens 
> > ---
> >  block.c   | 7 +++
> >  block/qapi.c  | 2 ++
> >  include/block/block.h | 1 +
> >  include/block/block_int.h | 3 +++
> >  migration/block.c | 4 
> >  qapi/block-core.json  | 2 +-
> >  6 files changed, 18 insertions(+), 1 deletion(-)
> 
> Thanks for the patch!  Please email the relevant maintainers:
> 
>   $ scripts/get_maintainer.pl -f block.c
>   Kevin Wolf  (supporter:Block layer core)
>   Max Reitz  (supporter:Block layer core)
>   qemu-block@nongnu.org (open list:Block layer core)
>   qemu-de...@nongnu.org (open list:All patches CC here)
> 
> I have CCed them so they see this email.
> 
> > diff --git a/migration/block.c b/migration/block.c
> > index 9171f60028..b347c3dc61 100644
> > --- a/migration/block.c
> > +++ b/migration/block.c
> > @@ -402,6 +402,10 @@ static int init_blk_migration(QEMUFile *f)
> >  bmds_bs = g_malloc0(num_bs * sizeof(*bmds_bs));
> >  
> >  for (i = 0, bs = bdrv_first(); bs; bs = bdrv_next(), i++) {
> > +if (bs->shared) {

If we go with this approach, I'd prefer a more specific name like
bs->skip_block_migration; "shared" has a lot meanings so is less readable.

Fam

> > +continue;
> > +}
> > +

Re: [Qemu-block] [PATCH v10 0/6] fsdev: qmp interface for io throttling

[Eric Blake]

On 09/04/2017 11:07 AM, Pradeep Jagadeesh wrote:
> These patches provide the qmp interface, to query the io throttle 
> status of the all fsdev devices that are present in a vm.
> also, it provides an interface to set the io throttle parameters of a
> fsdev to a required value. some of the patches also remove the duplicate
> code that was present in block and fsdev files. 
> 
> Pradeep Jagadeesh (6):
>   throttle: factor out duplicate code
>   qmp: Create IOThrottle structure
>   throttle: move out function to reuse the code
>   hmp: create a throttle initialization function for code reusability
>   fsdev: QMP interface for throttling
>   fsdev: hmp interface for throttling

I know you're already up to v10, but your code changes conflict with
Manos throttling work, which has now landed on Kevin's branch:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg01051.html

T: git git://repo.or.cz/qemu/kevin.git block

How hard is it for you to post a rebased v11 on top of Manos' work?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v9 6/6] qemu-iotests: add 184 for throttle filter driver

[Eric Blake]

On 09/05/2017 02:06 PM, Kevin Wolf wrote:
> Am 05.09.2017 um 18:16 hat Kevin Wolf geschrieben:
>> Am 25.08.2017 um 15:20 hat Manos Pitsidianakis geschrieben:
>>> Reviewed-by: Alberto Garcia 
>>> Signed-off-by: Manos Pitsidianakis 
>>
>> Does this test actually (still) pass for you? I can't see that it's
>> related to any recent change in master, but this is the diff that I get.
>>
>> I can update the reference output while applying, but obviously if it's
>> currently passing for you, it will fail after I "fix" it.
> 
> For the record, we discussed this on IRC. The test works correctly on
> master, but on my block branch there is a conflict with "block: pass
> bdrv_* methods to bs->file by default in block filters".
> 
> The correct action is to merge this throttle driver series after the
> conflicting patch because throttle doesn't implement .bdrv_get_info and
> needs the forwarding that the other patch implements.
> 
> I updated the test output accordingly and applied the series to my block
> branch.

Could you also squash this in to 5/6? (as long as we're intentionally
basing throttle on top of defaults, then we should use the right default
instead of duplicating things)

diff --git i/block/throttle.c w/block/throttle.c
index 7b33613372..5bca76300f 100644
--- i/block/throttle.c
+++ w/block/throttle.c
@@ -197,19 +197,6 @@ static bool
throttle_recurse_is_first_non_filter(BlockDriverState *bs,
 return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
 }

-static int64_t coroutine_fn
throttle_co_get_block_status(BlockDriverState *bs,
- int64_t
sector_num,
- int nb_sectors,
- int *pnum,
-
BlockDriverState **file)
-{
-assert(bs->file && bs->file->bs);
-*pnum = nb_sectors;
-*file = bs->file->bs;
-return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID |
-   (sector_num << BDRV_SECTOR_BITS);
-}
-
 static BlockDriver bdrv_throttle = {
 .format_name=   "throttle",
 .protocol_name  =   "throttle",
@@ -237,7 +224,7 @@ static BlockDriver bdrv_throttle = {
 .bdrv_reopen_prepare=   throttle_reopen_prepare,
 .bdrv_reopen_commit =   throttle_reopen_commit,
 .bdrv_reopen_abort  =   throttle_reopen_abort,
-.bdrv_co_get_block_status   =   throttle_co_get_block_status,
+.bdrv_co_get_block_status   =
bdrv_co_get_block_status_from_file,

 .is_filter  =   true,
 };


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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH] qemu-iotests: Make test 192 use QMP; convert it to Python

[Eric Blake]

On 09/04/2017 08:28 AM, Kashyap Chamarthy wrote:
> On Mon, Sep 04, 2017 at 08:55:23PM +0800, Fam Zheng wrote:
>> Hi Kashyap,
>>
>> On Mon, 09/04 13:16, Kashyap Chamarthy wrote:

>>> The test 192 ("Test NBD export with '-incoming' (non-shared
>>> storage migration use case from libvirt")) is currently using HMP.
>>> Replace the HMP usage with QMP, as the upstream preference seems to be:
>>> "Use QMP where possible, unless you're explicitly testing something
>>> related to HMP".

Kevin actually argued that keeping some HMP coverage is a GOOD thing:

https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg00798.html

>>
>> As an improvement maybe you could rebase to Stefan's "iotests: clean up
>> resources using context managers" series and switch to "with" for the temp 
>> file
>> and VM.
> 
> Good point.  I did notice that thread[*] about resource clean up.  And I
> see it's already applied to 'block-next'.  Will rebase and change to the
> 'with' statement.

I'm not sure if this patch helps us any; I personally find the unit-test
style python iotests harder to debug than the ones that produce
diff-able nnn.out files (debugging an nnn.out file that has only a list
of . doesn't make it easy to reproduce).  If the test already runs
well in shell, what does the conversion to Python actually buy us?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH] qcow2: move qcow2_store_persistent_dirty_bitmaps() before cache flushing

[Eric Blake]

On 09/04/2017 05:18 AM, Pavel Butsykin wrote:
> After calling qcow2_inactivate(), all qcow2 caches must be flushed, but this
> may not happen, because the last call qcow2_store_persistent_dirty_bitmaps()
> can lead to marking l2/refcont cache as dirty.
> 
> Let's move qcow2_store_persistent_dirty_bitmaps() before the caсhe flushing
> to fix it.
> 
> Signed-off-by: Pavel Butsykin 
> ---
>  block/qcow2.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 

Should this cc: qemu-stable?

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH 0/3] nbd: Use common read/write-all qio functions

[Eric Blake]

Now that Daniel just added convenience qio channel looping functions,
we might as well use it in NBD instead of open-coding our own.  In
doing so, fix a couple of short-falls noticed in the qio code.

If Dan is happy with this, I can take the series through the NBD queue.

[The diffstat is slightly misleading - it's a net reduction in lines
of code, but the added comments hide that fact]

Eric Blake (3):
  io: Yield rather than wait when already in coroutine
  io: Add new qio_channel_read{,v}_all_eof functions
  nbd: Use new qio_channel_*_all() functions

 include/block/nbd.h|  2 --
 include/io/channel.h   | 53 
 nbd/nbd-internal.h | 41 ++-
 block/nbd-client.c | 15 ++--
 io/channel.c   | 60 ++
 nbd/common.c   | 45 --
 tests/qemu-iotests/083.out |  8 +++
 7 files changed, 121 insertions(+), 103 deletions(-)

-- 
2.13.5

[Qemu-block] [PATCH 1/3] io: Yield rather than wait when already in coroutine

[Eric Blake]

The new qio_channel_{read,write}{,v}_all functions are documented
as yielding until data is available.  When used on a blocking
channel, this yield is done via qio_channel_wait() which spawns
a new coroutine under the hood (so it is the new entry point that
yields as needed); but if we are already in a coroutine (at which
point QIO_CHANNEL_ERR_BLOCK is only possible if we are a
non-blocking channel), we want to yield the current coroutine
instead of spawning a new one.

Signed-off-by: Eric Blake 
---
 io/channel.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/io/channel.c b/io/channel.c
index 5e8c2f0a91..9e62794cab 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -105,7 +105,11 @@ int qio_channel_readv_all(QIOChannel *ioc,
 ssize_t len;
 len = qio_channel_readv(ioc, local_iov, nlocal_iov, errp);
 if (len == QIO_CHANNEL_ERR_BLOCK) {
-qio_channel_wait(ioc, G_IO_IN);
+if (qemu_in_coroutine()) {
+qio_channel_yield(ioc, G_IO_IN);
+} else {
+qio_channel_wait(ioc, G_IO_IN);
+}
 continue;
 } else if (len < 0) {
 goto cleanup;
@@ -143,7 +147,11 @@ int qio_channel_writev_all(QIOChannel *ioc,
 ssize_t len;
 len = qio_channel_writev(ioc, local_iov, nlocal_iov, errp);
 if (len == QIO_CHANNEL_ERR_BLOCK) {
-qio_channel_wait(ioc, G_IO_OUT);
+if (qemu_in_coroutine()) {
+qio_channel_yield(ioc, G_IO_OUT);
+} else {
+qio_channel_wait(ioc, G_IO_OUT);
+}
 continue;
 }
 if (len < 0) {
-- 
2.13.5

[Qemu-block] [PATCH 2/3] io: Add new qio_channel_read{, v}_all_eof functions

[Eric Blake]

Some callers want to distinguish between clean EOF (no bytes read)
vs. a short read (at least one byte read, but EOF encountered
before reaching the desired length), as it allows clients the
ability to do a graceful shutdown when a server shuts down at
defined safe points in the protocol, rather than treating all
shutdown scenarios as an error due to EOF.  However, we don't want
to require all callers to have to check for early EOF.  So add
another wrapper function that can be used by the callers that care
about the distinction.

Signed-off-by: Eric Blake 
---
 include/io/channel.h | 53 
 io/channel.c | 48 +++
 2 files changed, 93 insertions(+), 8 deletions(-)

diff --git a/include/io/channel.h b/include/io/channel.h
index 8f25893c45..3995e243a3 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -269,6 +269,36 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
 Error **errp);

 /**
+ * qio_channel_readv_all_eof:
+ * @ioc: the channel object
+ * @iov: the array of memory regions to read data into
+ * @niov: the length of the @iov array
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Read data from the IO channel, storing it in the
+ * memory regions referenced by @iov. Each element
+ * in the @iov will be fully populated with data
+ * before the next one is used. The @niov parameter
+ * specifies the total number of elements in @iov.
+ *
+ * The function will wait for all requested data
+ * to be read, yielding from the current coroutine
+ * if required.
+ *
+ * If end-of-file occurs before any data is read,
+ * no error is reported; otherwise, if it occurs
+ * before all requested data has been read, an error
+ * will be reported.
+ *
+ * Returns: 1 if all bytes were read, 0 if end-of-file
+ *  occurs without data, or -1 on error
+ */
+int qio_channel_readv_all_eof(QIOChannel *ioc,
+  const struct iovec *iov,
+  size_t niov,
+  Error **errp);
+
+/**
  * qio_channel_readv_all:
  * @ioc: the channel object
  * @iov: the array of memory regions to read data into
@@ -383,6 +413,28 @@ ssize_t qio_channel_write(QIOChannel *ioc,
   Error **errp);

 /**
+ * qio_channel_read_all_eof:
+ * @ioc: the channel object
+ * @buf: the memory region to read data into
+ * @buflen: the number of bytes to @buf
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Reads @buflen bytes into @buf, possibly blocking or (if the
+ * channel is non-blocking) yielding from the current coroutine
+ * multiple times until the entire content is read. If end-of-file
+ * occurs immediately it is not an error, but if it occurs after
+ * data has been read it will return an error rather than a
+ * short-read. Otherwise behaves as qio_channel_read().
+ *
+ * Returns: 1 if all bytes were read, 0 if end-of-file occurs
+ *  without data, or -1 on error
+ */
+int qio_channel_read_all_eof(QIOChannel *ioc,
+ char *buf,
+ size_t buflen,
+ Error **errp);
+
+/**
  * qio_channel_read_all:
  * @ioc: the channel object
  * @buf: the memory region to read data into
@@ -401,6 +453,7 @@ int qio_channel_read_all(QIOChannel *ioc,
  char *buf,
  size_t buflen,
  Error **errp);
+
 /**
  * qio_channel_write_all:
  * @ioc: the channel object
diff --git a/io/channel.c b/io/channel.c
index 9e62794cab..ec4b86de7c 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -86,16 +86,16 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc,
 }


-
-int qio_channel_readv_all(QIOChannel *ioc,
-  const struct iovec *iov,
-  size_t niov,
-  Error **errp)
+int qio_channel_readv_all_eof(QIOChannel *ioc,
+  const struct iovec *iov,
+  size_t niov,
+  Error **errp)
 {
 int ret = -1;
 struct iovec *local_iov = g_new(struct iovec, niov);
 struct iovec *local_iov_head = local_iov;
 unsigned int nlocal_iov = niov;
+bool partial = false;

 nlocal_iov = iov_copy(local_iov, nlocal_iov,
   iov, niov,
@@ -114,21 +114,43 @@ int qio_channel_readv_all(QIOChannel *ioc,
 } else if (len < 0) {
 goto cleanup;
 } else if (len == 0) {
-error_setg(errp,
-   "Unexpected end-of-file before all bytes were read");
+if (partial) {
+error_setg(errp,
+   "Unexpected end-of-file before all bytes were 
read");
+} else {
+ret = 0;
+}
 goto cleanup;
 }

+partial = true;
 

Re: [Qemu-block] [PATCH v9 6/6] qemu-iotests: add 184 for throttle filter driver

[Kevin Wolf]

Am 05.09.2017 um 18:16 hat Kevin Wolf geschrieben:
> Am 25.08.2017 um 15:20 hat Manos Pitsidianakis geschrieben:
> > Reviewed-by: Alberto Garcia 
> > Signed-off-by: Manos Pitsidianakis 
> 
> Does this test actually (still) pass for you? I can't see that it's
> related to any recent change in master, but this is the diff that I get.
> 
> I can update the reference output while applying, but obviously if it's
> currently passing for you, it will fail after I "fix" it.

For the record, we discussed this on IRC. The test works correctly on
master, but on my block branch there is a conflict with "block: pass
bdrv_* methods to bs->file by default in block filters".

The correct action is to merge this throttle driver series after the
conflicting patch because throttle doesn't implement .bdrv_get_info and
needs the forwarding that the other patch implements.

I updated the test output accordingly and applied the series to my block
branch.

Kevin

Re: [Qemu-block] [PATCH v9 6/6] qemu-iotests: add 184 for throttle filter driver

[Kevin Wolf]

Am 25.08.2017 um 15:20 hat Manos Pitsidianakis geschrieben:
> Reviewed-by: Alberto Garcia 
> Signed-off-by: Manos Pitsidianakis 

Does this test actually (still) pass for you? I can't see that it's
related to any recent change in master, but this is the diff that I get.

I can update the reference output while applying, but obviously if it's
currently passing for you, it will fail after I "fix" it.

Kevin


184[18:07:39] [18:07:39] - output mismatch (see 184.out.bad)
--- /home/kwolf/source/qemu/tests/qemu-iotests/184.out  2017-09-05 
18:07:30.751340633 +0200
+++ 184.out.bad 2017-09-05 18:07:39.707341854 +0200
@@ -30,8 +30,10 @@
 "image": {
 "virtual-size": 67108864,
 "filename": "json:{\"throttle-group\": \"group0\", \"driver\": 
\"throttle\", \"file\": {\"driver\": \"qcow2\", \"file\": {\"driver\": 
\"file\", \"filename\": \"TEST_DIR/t.qcow2\"}}}",
+"cluster-size": 65536,
 "format": "throttle",
-"actual-size": 200704
+"actual-size": 200704,
+"dirty-flag": false
 },
 "iops_wr": 0,
 "ro": false,

Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] block: add block device shared field

[Brian Steffens]

Thanks for taking a look at the patch and fixing the missing CC
addresses!

> Have you considered extending the 'migrate' command with a list of
> drives instead?

That was my original plan but I thought having information on whether a
device is shared or not could potentially be useful to other systems. If
not then you're right, it would be a lighter touch to add it only to the
'migrate' command.

> In case you haven't seen it, the preferred approach for non-shared
> storage migration is drive-mirror + NBD.  The block/migration.c code is
> an older feature that is less flexible.  More info on drive-mirror +
> NBD:

I didn't realize drive-mirror + NBD was the preferred approach. Are
there any plans to drop support for the block/migration.c style?

Re: [Qemu-block] [PATCH v9 4/6] block: convert ThrottleGroup to object with QOM

[Kevin Wolf]

Am 25.08.2017 um 15:20 hat Manos Pitsidianakis geschrieben:
> ThrottleGroup is converted to an object. This will allow the future
> throttle block filter drive easy creation and configuration of throttle
> groups in QMP and cli.
> 
> A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> struct for all throttle configuration needs in QMP.
> 
> ThrottleGroups can be created via CLI as
> -object throttle-group,id=foo,x-iops-total=100,x-..
> where x-* are individual limit properties. Since we can't add non-scalar
> properties in -object this interface must be used instead. However,
> setting these properties must be disabled after initialization because
> certain combinations of limits are forbidden and thus configuration
> changes should be done in one transaction. The individual properties
> will go away when support for non-scalar values in CLI is implemented
> and thus are marked as experimental.
> 
> ThrottleGroup also has a `limits` property that uses the ThrottleLimits
> struct.  It can be used to create ThrottleGroups or set the
> configuration in existing groups as follows:
> 
> { "execute": "object-add",
>   "arguments": {
> "qom-type": "throttle-group",
> "id": "foo",
> "props" : {
>   "limits": {
>   "iops-total": 100
>   }
> }
>   }
> }
> { "execute" : "qom-set",
> "arguments" : {
> "path" : "foo",
> "property" : "limits",
> "value" : {
> "iops-total" : 99
> }
> }
> }
> 
> This also means a group's configuration can be fetched with qom-get.
> 
> Signed-off-by: Manos Pitsidianakis 

> +static bool throttle_group_can_be_deleted(UserCreatable *uc, Error **errp)
> +{
> +return OBJECT(uc)->ref == 1;
> +}
> +
> +static void throttle_group_obj_class_init(ObjectClass *klass, void 
> *class_data)
> +{
> +size_t i = 0;
> +UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
> +
> +ucc->complete = throttle_group_obj_complete;
> +ucc->can_be_deleted = throttle_group_can_be_deleted;

block/throttle-groups.c: In function 'throttle_group_obj_class_init':
block/throttle-groups.c:880:25: error: assignment from incompatible pointer 
type [-Werror=incompatible-pointer-types]
 ucc->can_be_deleted = throttle_group_can_be_deleted;

This is from a semantic conflict with commit 3beacfb9 ('qom: Remove
unused errp parameter from can_be_deleted()'). If the rest of the series
is okay, I'll just remove the **errp parameter while applying.

Kevin

[Qemu-block] [PATCH 0/3] iotests: cure s390x failures by switching to ccw

[Cornelia Huck]

Recent changes in s390x made pci support dependant on the zpci cpu
feature, which is not provided on all models (and not on by default).
This means we cannot instatiate pci devices on a standard qemu
invocation for s390x. Moreover, the zpci instructions are not even
wired up for tcg yet, so actually doing anything with those pci devices
is bound to fail on tcg.

Let's follow the existing example in 068 and switch to the (default)
virtio-ccw transport on s390x. The changes for 051 and 067 are split
out as they require adding an output file for s390x (the actual command
lines are part of the output).

Cornelia Huck (3):
  iotests: use -ccw on s390x for 040, 139, and 182
  iotests: use -ccw on s390x for 051
  iotests: use -ccw on s390x for 067

 tests/qemu-iotests/040 |   6 +-
 tests/qemu-iotests/051 |   9 +-
 tests/qemu-iotests/051.s390-ccw-virtio.out | 434 +++
 tests/qemu-iotests/067 |  11 +-
 tests/qemu-iotests/067.s390-ccw-virtio.out | 458 +
 tests/qemu-iotests/139 |  12 +-
 tests/qemu-iotests/182 |  13 +-
 7 files changed, 936 insertions(+), 7 deletions(-)
 create mode 100644 tests/qemu-iotests/051.s390-ccw-virtio.out
 create mode 100644 tests/qemu-iotests/067.s390-ccw-virtio.out

-- 
2.13.5

[Qemu-block] [PATCH 1/3] iotests: use -ccw on s390x for 040, 139, and 182

[Cornelia Huck]

The default cpu model on s390x does not provide zPCI, which is
not yet wired up on tcg. Moreover, virtio-ccw is the standard
on s390x, so use the -ccw instead of the -pci versions of virtio
devices on s390x.

Signed-off-by: Cornelia Huck 
---
 tests/qemu-iotests/040 |  6 +-
 tests/qemu-iotests/139 | 12 ++--
 tests/qemu-iotests/182 | 13 +++--
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 95b7510571..c284d08796 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -82,7 +82,11 @@ class TestSingleDrive(ImageCommitTestCase):
 qemu_io('-f', 'raw', '-c', 'write -P 0xab 0 524288', backing_img)
 qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288', 
mid_img)
 self.vm = iotests.VM().add_drive(test_img, 
"node-name=top,backing.node-name=mid,backing.backing.node-name=base", 
interface="none")
-self.vm.add_device("virtio-scsi-pci")
+if iotests.qemu_default_machine == 's390-ccw-virtio':
+self.vm.add_device("virtio-scsi-ccw")
+else:
+self.vm.add_device("virtio-scsi-pci")
+
 self.vm.add_device("scsi-hd,id=scsi0,drive=drive0")
 self.vm.launch()
 
diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
index 50cf40fbd5..f8f02808a9 100644
--- a/tests/qemu-iotests/139
+++ b/tests/qemu-iotests/139
@@ -25,13 +25,21 @@ import time
 
 base_img = os.path.join(iotests.test_dir, 'base.img')
 new_img = os.path.join(iotests.test_dir, 'new.img')
+if iotests.qemu_default_machine == 's390-ccw-virtio':
+default_virtio_blk = 'virtio-blk-ccw'
+else:
+default_virtio_blk = 'virtio-blk-pci'
 
 class TestBlockdevDel(iotests.QMPTestCase):
 
 def setUp(self):
 iotests.qemu_img('create', '-f', iotests.imgfmt, base_img, '1M')
 self.vm = iotests.VM()
-self.vm.add_device("virtio-scsi-pci,id=virtio-scsi")
+if iotests.qemu_default_machine == 's390-ccw-virtio':
+self.vm.add_device("virtio-scsi-ccw,id=virtio-scsi")
+else:
+self.vm.add_device("virtio-scsi-pci,id=virtio-scsi")
+
 self.vm.launch()
 
 def tearDown(self):
@@ -87,7 +95,7 @@ class TestBlockdevDel(iotests.QMPTestCase):
 self.checkBlockDriverState(node, expect_error)
 
 # Add a device model
-def addDeviceModel(self, device, backend, driver = 'virtio-blk-pci'):
+def addDeviceModel(self, device, backend, driver = default_virtio_blk):
 result = self.vm.qmp('device_add', id = device,
  driver = driver, drive = backend)
 self.assert_qmp(result, 'return', {})
diff --git a/tests/qemu-iotests/182 b/tests/qemu-iotests/182
index 7ecbb22604..2e078ceed8 100755
--- a/tests/qemu-iotests/182
+++ b/tests/qemu-iotests/182
@@ -45,17 +45,26 @@ _supported_os Linux
 
 size=32M
 
+case "$QEMU_DEFAULT_MACHINE" in
+  s390-ccw-virtio)
+  virtioblk=virtio-blk-ccw
+  ;;
+  *)
+  virtioblk=virtio-blk-pci
+  ;;
+esac
+
 _make_test_img $size
 
 echo "Starting QEMU"
 _launch_qemu -drive file=$TEST_IMG,if=none,id=drive0,file.locking=on \
--device virtio-blk-pci,drive=drive0
+-device $virtioblk,drive=drive0
 
 echo
 echo "Starting a second QEMU using the same image should fail"
 echo 'quit' | $QEMU -monitor stdio \
 -drive file=$TEST_IMG,if=none,id=drive0,file.locking=on \
--device virtio-blk-pci,drive=drive0 2>&1 | _filter_testdir 2>&1 |
+-device $virtioblk,drive=drive0 2>&1 | _filter_testdir 2>&1 |
 _filter_qemu |
 sed -e '/falling back to POSIX file/d' \
 -e '/locks can be lost unexpectedly/d'
-- 
2.13.5

[Qemu-block] [PATCH 3/3] iotests: use -ccw on s390x for 067

[Cornelia Huck]

The default cpu model on s390x does not provide zPCI, which is
not yet wired up on tcg. Moreover, virtio-ccw is the standard
on s390x, so use the -ccw instead of the -pci versions of virtio
devices on s390x.

Provide an output file for s390x.

Signed-off-by: Cornelia Huck 
---
 tests/qemu-iotests/067 |  11 +-
 tests/qemu-iotests/067.s390-ccw-virtio.out | 458 +
 2 files changed, 468 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemu-iotests/067.s390-ccw-virtio.out

diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067
index 5d4ca4bc61..c5db04fb50 100755
--- a/tests/qemu-iotests/067
+++ b/tests/qemu-iotests/067
@@ -37,6 +37,15 @@ _supported_os Linux
 # Because anything other than 16 would change the output of query-block
 _unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
 
+case "$QEMU_DEFAULT_MACHINE" in
+  s390-ccw-virtio)
+  hba=virtio-scsi-ccw
+  ;;
+  *)
+  hba=virtio-scsi-pci
+  ;;
+esac
+
 function do_run_qemu()
 {
 echo Testing: "$@"
@@ -141,7 +150,7 @@ echo
 echo === Empty drive with -device and device_del ===
 echo
 
-run_qemu -device virtio-scsi-pci -device scsi-cd,id=cd0 <

[Qemu-block] [PATCH 2/3] iotests: use -ccw on s390x for 051

[Cornelia Huck]

The default cpu model on s390x does not provide zPCI, which is
not yet wired up on tcg. Moreover, virtio-ccw is the standard
on s390x, so use the -ccw instead of the -pci versions of virtio
devices on s390x.

Provide an output file for s390x.

Signed-off-by: Cornelia Huck 
---
 tests/qemu-iotests/051 |   9 +-
 tests/qemu-iotests/051.s390-ccw-virtio.out | 434 +
 2 files changed, 442 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemu-iotests/051.s390-ccw-virtio.out

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index c8cfc764bc..f6ad0f4f0b 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -103,7 +103,14 @@ echo
 echo === Device without drive ===
 echo
 
-run_qemu -device virtio-scsi-pci -device scsi-hd
+case "$QEMU_DEFAULT_MACHINE" in
+  s390-ccw-virtio)
+  run_qemu -device virtio-scsi-ccw -device scsi-hd
+  ;;
+  *)
+  run_qemu -device virtio-scsi-pci -device scsi-hd
+  ;;
+esac
 
 echo
 echo === Overriding backing file ===
diff --git a/tests/qemu-iotests/051.s390-ccw-virtio.out 
b/tests/qemu-iotests/051.s390-ccw-virtio.out
new file mode 100644
index 00..7555f0b73e
--- /dev/null
+++ b/tests/qemu-iotests/051.s390-ccw-virtio.out
@@ -0,0 +1,434 @@
+QA output created by 051
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/t.IMGFMT.base
+
+=== Unknown option ===
+
+Testing: -drive 
file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=,if=none,id=drive0
+QEMU_PROG: -drive 
file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=,if=none,id=drive0: Block format 
'qcow2' does not support the option 'unknown_opt'
+
+Testing: -drive 
file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on,if=none,id=drive0
+QEMU_PROG: -drive 
file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on,if=none,id=drive0: Block 
format 'qcow2' does not support the option 'unknown_opt'
+
+Testing: -drive 
file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234,if=none,id=drive0
+QEMU_PROG: -drive 
file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234,if=none,id=drive0: Block 
format 'qcow2' does not support the option 'unknown_opt'
+
+Testing: -drive 
file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo,if=none,id=drive0
+QEMU_PROG: -drive 
file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo,if=none,id=drive0: Block 
format 'qcow2' does not support the option 'unknown_opt'
+
+
+=== Unknown protocol option ===
+
+Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,file.unknown_opt=
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,file.unknown_opt=: Block 
protocol 'file' doesn't support the option 'unknown_opt'
+
+Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,file.unknown_opt=on
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,file.unknown_opt=on: 
Block protocol 'file' doesn't support the option 'unknown_opt'
+
+Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,file.unknown_opt=1234
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,file.unknown_opt=1234: 
Block protocol 'file' doesn't support the option 'unknown_opt'
+
+Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,file.unknown_opt=foo
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,file.unknown_opt=foo: 
Block protocol 'file' doesn't support the option 'unknown_opt'
+
+
+=== Invalid format ===
+
+Testing: -drive file=TEST_DIR/t.qcow2,format=foo
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=foo: Unknown driver 'foo'
+
+Testing: -drive file=TEST_DIR/t.qcow2,driver=foo
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=foo: Unknown driver 'foo'
+
+Testing: -drive file=TEST_DIR/t.qcow2,driver=raw,format=qcow2
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=raw,format=qcow2: Cannot 
specify both 'driver' and 'format'
+
+Testing: -drive file=TEST_DIR/t.qcow2,driver=qcow2,format=qcow2
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,driver=qcow2,format=qcow2: Cannot 
specify both 'driver' and 'format'
+
+
+=== Device without drive ===
+
+Testing: -device virtio-scsi-ccw -device scsi-hd
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) QEMU_PROG: -device scsi-hd: drive property not set
+
+
+=== Overriding backing file ===
+
+Testing: -drive 
file=TEST_DIR/t.qcow2,driver=qcow2,backing.file.filename=TEST_DIR/t.qcow2.orig,if=none,id=drive0
 -nodefaults
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) info block
+drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+Removable device: not locked, tray closed
+Cache mode:   writeback
+Backing file: TEST_DIR/t.qcow2.orig (chain depth: 1)
+(qemu) quit
+
+Testing: -drive 
file=TEST_DIR/t.qcow2,driver=raw,backing.file.filename=TEST_DIR/t.qcow2.orig
+QEMU_PROG: -drive 
file=TEST_DIR/t.qcow2,driver=raw,backing.file.filename=TEST_DIR/t.qcow2.orig: 
Driver doesn't support backing files
+
+Testing: -drive 
file=TEST_DIR/t.qcow2,file.backing.driver=file,file.backing.filename=TEST_DIR/t.qcow2.orig
+QEMU_PROG: -drive 

Re: [Qemu-block] [PATCH v3 4/7] block: remove legacy I/O throttling

[Stefan Hajnoczi]

On Fri, Aug 25, 2017 at 04:23:29PM +0300, Manos Pitsidianakis wrote:
>  void blk_io_limits_disable(BlockBackend *blk)
>  {
> -assert(blk->public.throttle_group_member.throttle_state);
> -bdrv_drained_begin(blk_bs(blk));
> -throttle_group_unregister_tgm(>public.throttle_group_member);
> -bdrv_drained_end(blk_bs(blk));
> +BlockDriverState *bs, *throttle_node;
> +
> +throttle_node = blk_get_public(blk)->throttle_node;
> +
> +assert(throttle_node);
> +
> +bs = throttle_node->file->bs;
> +bdrv_drained_begin(bs);
> +
> +/* Ref throttle_node's child bs to ensure it won't go away */
> +bdrv_ref(bs);

Is this really necessary?  bdrv_replace_node() also takes a temporary
reference:

  bdrv_ref(to);
  bdrv_replace_child_noperm(c, to);
  bdrv_unref(from);

Re: [Qemu-block] [PATCH v9 0/6] add throttle block driver filter

[Stefan Hajnoczi]

On Fri, Aug 25, 2017 at 04:20:22PM +0300, Manos Pitsidianakis wrote:
> This series adds a throttle block driver filter. Currently throttling is done
> at the BlockBackend level. Using block driver interfaces we can move the
> throttling to any point in the BDS graph using a throttle node which uses the
> existing throttling code. This allows for potentially more complex
> configurations (throttling at any point in the graph, chained filters)
> 
> v9:
>   fix suggestions by stefanha in patch 4
>   add 'throttle_group_can_be_deleted' function in patch 4
> 
> v8:
>   fix suggestions by berto
> 
> v7:
>   remove anonymous groups
>   error on missing throttle-group option on block/throttle.c
>   change switch case format in block/throttle-groups.c (berto)
> 
> v6:
>   don't allow named group limit configuration in block/throttle.c; allow 
> either
> -drive driver=throttle,file.filename=foo.qcow2, \
> limits.iops-total=...
>   or
> -drive driver=throttle,file.filename=foo.qcow2,throttle-group=bar
>   fixes suggested by berto
> 
> v5:
>   fix crash in 'add aio_context field in ThrottleGroupMember'
>   fix suggestions in block/throttle.c
> 
> v4:
>   fix suggestions in block/throttle.c
>   fix suggestions in block/throttle_groups.c
>   add doc note in BlockDevOptionsThrottle
> 
> v3:
>   fix style error in 'add aio_context field in ThrottleGroupMember'
> 
> v2:
>   change QOM throttle group object name
>   print valid ranges for uint on error
>   move frees in throttle_group_obj_finalize()
>   split throttle_group_{set,get}()
>   add throttle_recurse_is_first_non_filter()
> 
> 
> Manos Pitsidianakis (6):
>   block: move ThrottleGroup membership to ThrottleGroupMember
>   block: add aio_context field in ThrottleGroupMember
>   block: tidy ThrottleGroupMember initializations
>   block: convert ThrottleGroup to object with QOM
>   block: add throttle block filter driver
>   qemu-iotests: add 184 for throttle filter driver
> 
>  qapi/block-core.json|  66 +++-
>  include/block/throttle-groups.h |  52 ++-
>  include/qemu/throttle-options.h |  60 +++-
>  include/qemu/throttle.h |   3 +
>  include/sysemu/block-backend.h  |  20 +-
>  block/block-backend.c   |  62 ++--
>  block/qapi.c|   8 +-
>  block/throttle-groups.c | 748 
> ++--
>  block/throttle.c| 250 ++
>  blockdev.c  |   4 +-
>  tests/test-throttle.c   | 111 +++---
>  util/throttle.c | 151 
>  block/Makefile.objs |   1 +
>  tests/qemu-iotests/184  | 205 +++
>  tests/qemu-iotests/184.out  | 300 
>  tests/qemu-iotests/group|   1 +
>  16 files changed, 1721 insertions(+), 321 deletions(-)
>  create mode 100644 block/throttle.c
>  create mode 100755 tests/qemu-iotests/184
>  create mode 100644 tests/qemu-iotests/184.out
> 
> -- 
> 2.11.0
> 

Reviewed-by: Stefan Hajnoczi 

Re: [Qemu-block] [PATCH v9 5/6] block: add throttle block filter driver

[Stefan Hajnoczi]

On Fri, Aug 25, 2017 at 04:20:27PM +0300, Manos Pitsidianakis wrote:
> +static int throttle_configure_tgm(BlockDriverState *bs,
> +  ThrottleGroupMember *tgm,
> +  QDict *options, Error **errp)
> +{
> +int ret;
> +const char *group_name;
> +Error *local_err = NULL;
> +QemuOpts *opts = qemu_opts_create(_opts, NULL, 0, _abort);
> +
> +qemu_opts_absorb_qdict(opts, options, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +ret = -EINVAL;
> +goto fin;
> +}
> +
> +group_name = qemu_opt_get(opts, QEMU_OPT_THROTTLE_GROUP_NAME);
> +if (!group_name) {
> +error_setg(errp, "Please specify a throttle group.");

error_setg() messages do not end with punctuation:

  error_setg(errp, "Please specify a throttle group");

This is not worth respinning for though.

> +ret = -EINVAL;
> +goto fin;
> +} else if (!throttle_group_exists(group_name)) {
> +error_setg(errp, "Throttle group '%s' does not exist.", group_name);

Same here.

Anyway:

Reviewed-by: Stefan Hajnoczi 

Re: [Qemu-block] [PATCH v9 4/6] block: convert ThrottleGroup to object with QOM

[Stefan Hajnoczi]

On Fri, Aug 25, 2017 at 04:20:26PM +0300, Manos Pitsidianakis wrote:
> ThrottleGroup is converted to an object. This will allow the future
> throttle block filter drive easy creation and configuration of throttle
> groups in QMP and cli.
> 
> A new QAPI struct, ThrottleLimits, is introduced to provide a shared
> struct for all throttle configuration needs in QMP.
> 
> ThrottleGroups can be created via CLI as
> -object throttle-group,id=foo,x-iops-total=100,x-..
> where x-* are individual limit properties. Since we can't add non-scalar
> properties in -object this interface must be used instead. However,
> setting these properties must be disabled after initialization because
> certain combinations of limits are forbidden and thus configuration
> changes should be done in one transaction. The individual properties
> will go away when support for non-scalar values in CLI is implemented
> and thus are marked as experimental.
> 
> ThrottleGroup also has a `limits` property that uses the ThrottleLimits
> struct.  It can be used to create ThrottleGroups or set the
> configuration in existing groups as follows:
> 
> { "execute": "object-add",
>   "arguments": {
> "qom-type": "throttle-group",
> "id": "foo",
> "props" : {
>   "limits": {
>   "iops-total": 100
>   }
> }
>   }
> }
> { "execute" : "qom-set",
> "arguments" : {
> "path" : "foo",
> "property" : "limits",
> "value" : {
> "iops-total" : 99
> }
> }
> }
> 
> This also means a group's configuration can be fetched with qom-get.
> 
> Signed-off-by: Manos Pitsidianakis 
> ---
>  qapi/block-core.json|  48 +
>  include/block/throttle-groups.h |   3 +
>  include/qemu/throttle-options.h |  59 --
>  include/qemu/throttle.h |   3 +
>  block/throttle-groups.c | 422 
> 
>  tests/test-throttle.c   |   1 +
>  util/throttle.c | 151 ++
>  7 files changed, 627 insertions(+), 60 deletions(-)

Reviewed-by: Stefan Hajnoczi 

Re: [Qemu-block] [Qemu-devel] Persistent bitmaps for non-qcow2 formats

[Fam Zheng]

On Tue, 09/05 15:27, Kevin Wolf wrote:
> Am 05.09.2017 um 15:18 hat Fam Zheng geschrieben:
> > On Tue, 09/05 15:01, Kevin Wolf wrote:
> > > Am 28.08.2017 um 04:57 hat Fam Zheng geschrieben:
> > > > On Fri, 08/25 15:44, Max Reitz wrote:
> > > > > Well, OK.  The main argument against supporting anything but qcow2 is
> > > > > "if you want features, use qcow2; and we are working on making qcow2 
> > > > > as
> > > > > fast as possible."  I think that's a very good argument still.  At 
> > > > > some
> > > > > point I (and probably others, too) had the idea of making qcow2 files 
> > > > > in
> > > > > raw layout: 
> > > > 
> > > > Yes! I think this idea makes a whole lot of sense, too. Metadata tables 
> > > > can be
> > > > generated so old implementation can still use it.
> > > 
> > > Maybe a nice way to attack this would be "huge pages", i.e. have a L1
> > > entry flag that tells "this points directly to a huge cluster instead of
> > > an L2 table". Gives us 512 MB clusters with a 64k cluster size, or a
> > > maximum of 512 GB clusters with a 2 MB cluster size.
> > > 
> > > Huge clusters would only be used by qemu-img create if the respective
> > > option is given, and only with preallocation.
> > 
> > Then this image is not usable on old QEMUs, right? So this is going to be an
> > incompatible_feature, whereas with the linear mapping L1/L2 tables 
> > approach, it
> > can be an autoclear_feature.
> 
> Ah yes, that's true. Maybe that's important enough that just a single
> global flag is better, even if it's less flexible.
> 
> Of course, this approach also means that we still need to write all of
> the L2 tables and use disk space for them, even though newer versions
> will never look at them.

To speculate^Wmitigate, "qemu-img create" is free to pick a larger cluster size
when this feature is used?

Fam

Re: [Qemu-block] [Qemu-devel] Persistent bitmaps for non-qcow2 formats

[Kevin Wolf]

Am 05.09.2017 um 15:18 hat Fam Zheng geschrieben:
> On Tue, 09/05 15:01, Kevin Wolf wrote:
> > Am 28.08.2017 um 04:57 hat Fam Zheng geschrieben:
> > > On Fri, 08/25 15:44, Max Reitz wrote:
> > > > Well, OK.  The main argument against supporting anything but qcow2 is
> > > > "if you want features, use qcow2; and we are working on making qcow2 as
> > > > fast as possible."  I think that's a very good argument still.  At some
> > > > point I (and probably others, too) had the idea of making qcow2 files in
> > > > raw layout: 
> > > 
> > > Yes! I think this idea makes a whole lot of sense, too. Metadata tables 
> > > can be
> > > generated so old implementation can still use it.
> > 
> > Maybe a nice way to attack this would be "huge pages", i.e. have a L1
> > entry flag that tells "this points directly to a huge cluster instead of
> > an L2 table". Gives us 512 MB clusters with a 64k cluster size, or a
> > maximum of 512 GB clusters with a 2 MB cluster size.
> > 
> > Huge clusters would only be used by qemu-img create if the respective
> > option is given, and only with preallocation.
> 
> Then this image is not usable on old QEMUs, right? So this is going to be an
> incompatible_feature, whereas with the linear mapping L1/L2 tables approach, 
> it
> can be an autoclear_feature.

Ah yes, that's true. Maybe that's important enough that just a single
global flag is better, even if it's less flexible.

Of course, this approach also means that we still need to write all of
the L2 tables and use disk space for them, even though newer versions
will never look at them.

Kevin

Re: [Qemu-block] [Qemu-devel] Persistent bitmaps for non-qcow2 formats

[Fam Zheng]

On Tue, 09/05 15:01, Kevin Wolf wrote:
> Am 28.08.2017 um 04:57 hat Fam Zheng geschrieben:
> > On Fri, 08/25 15:44, Max Reitz wrote:
> > > Well, OK.  The main argument against supporting anything but qcow2 is
> > > "if you want features, use qcow2; and we are working on making qcow2 as
> > > fast as possible."  I think that's a very good argument still.  At some
> > > point I (and probably others, too) had the idea of making qcow2 files in
> > > raw layout: 
> > 
> > Yes! I think this idea makes a whole lot of sense, too. Metadata tables can 
> > be
> > generated so old implementation can still use it.
> 
> Maybe a nice way to attack this would be "huge pages", i.e. have a L1
> entry flag that tells "this points directly to a huge cluster instead of
> an L2 table". Gives us 512 MB clusters with a 64k cluster size, or a
> maximum of 512 GB clusters with a 2 MB cluster size.
> 
> Huge clusters would only be used by qemu-img create if the respective
> option is given, and only with preallocation.

Then this image is not usable on old QEMUs, right? So this is going to be an
incompatible_feature, whereas with the linear mapping L1/L2 tables approach, it
can be an autoclear_feature.

Fam

Re: [Qemu-block] Persistent bitmaps for non-qcow2 formats

[Kevin Wolf]

Am 23.08.2017 um 19:44 hat John Snow geschrieben:
> On 08/23/2017 01:31 PM, Max Reitz wrote:
> > On 2017-08-22 21:07, John Snow wrote:
> > 
> > [...]
> > 
> >> (3) Add either a new flag that turns qcow2's backing file into a full
> >> R/W backing file, or add a new extension to qcow2 entirely (bypassing
> >> the traditional backing file mechanism to avoid confusion for older
> >> tooling) that adds a new read-write backing file field.
> >>
> >> This RW backing file field will be used for all reads AND writes; the
> >> qcow2 in question becomes a metadata container on top of the BDS chain.
> >> We can re-use Vladimir's bitmap persistence extension to save bitmaps in
> >> a qcow2 shell.
> >>
> >> The qcow2 becomes effectively a metadata cache for a new (essentially)
> >> filter node that handles features such as bitmaps. This could also be
> >> used to provide allocation map data for RAW files and other goodies down
> >> the road.
> >>
> >> Hopefully this achieves our desire to not create new formats AND our
> >> desire to concentrate features (and debugging, testing, etc) into qcow2,
> >> while allowing users to "have bitmaps with raw files."
> >>
> >> Of course, in this scenario, users now have two files: a qcow2 wrapper
> >> and the actual raw file in question; but regardless of how we were going
> >> to solve this, a raw file necessitates an external file of some sort,
> >> else we give up the idea that it was a raw file.
> >>
> >>
> >> Sound good?
> > 
> > While I don't quite like the idea of R/W backing files, I guess "don't
> > quite like" is still rather good.
> 
> Yeah, it's not necessarily my first pick, but it might be the least bad.

As for backing files always being read-only, that ship has sailed long
ago. We really never had it because I think HMP 'commit' was introduced
at the same time as backing files, but at least it was still contained
in synchronous function protected by the BQL. But at latest since we
introduced live block jobs, it's not a reasonable assumption to make any
more.

At this point, I have no problems with adding more users of writable
backing files any more. We'll have to use op blockers to make it safe.

> If you have any suggestions or alternatives for a way to accomplish the
> same in a way that does not leave any, even faint, bad taste in your
> mouth you are more than welcome to suggest it.

Maybe one thing that I don't want to dismiss right away would be a
separate BlockDriver for passthrough qcow2 images so that we don't
clutter all functions with special cases. But I'm not compeltely
convinced of it either.

I think I once had a use case for clusters that are marked as
"read/write to backing file", but I can't seem to remember it now. If we
had such a per-cluster setting, then a separate BlockDriver wouldn't be
an option, obviously.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v6 29/29] libqtest: Rename qtest_init() to qtest_start()

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> We already have another qtest_init() in the tree, for the
> top-level qtest.c device; having two functions with different
> signatures is confusing.  Rename the libqtest version to
> qtest_start() to eliminate the duplication.

This is too much code churn for my taste, and I also do not like the
idea of naming the function qtest_start() - since this was a function
with different semantics before your patch 28/29, so this will cause
confusion for all the people who are used to the old qtest_start()
function or who want to backport patches that have done after this
change to a code level before this change.

If you are really bugged by the qtest_init() name clash, I think it's
way easier if you rename the qtest_init() in the qtest.c file instead.

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH v6 28/29] libqtest: Remove qtest_start() and qtest_end() shortcuts

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> Remove the trivial wrappers qtest_start() and qtest_end(), to make
> it obvious in the rest of the testsuite where we are still relying on
> global_qtest.  Doing this makes it easier to see what remaining
> cleanups will be needed if we don't want an implicit dependency
> on global state.  Many tests can also take advantage of qtest_init()
> doing formatting of args, avoiding a temporary local variable.
> 
> Signed-off-by: Eric Blake 
> ---
>  tests/libqtest.h   | 26 --
>  tests/libqtest.c   |  4 +--
>  tests/ac97-test.c  |  4 +--
>  tests/boot-order-test.c| 11 +++-
>  tests/boot-serial-test.c   | 12 +++--
>  tests/device-introspect-test.c | 24 -
>  tests/display-vga-test.c   | 18 +
>  tests/drive_del-test.c | 17 ++--
>  tests/ds1338-test.c|  2 +-
>  tests/e1000-test.c | 10 ++-
>  tests/e1000e-test.c| 14 +++---
>  tests/eepro100-test.c  | 11 ++--
>  tests/endianness-test.c| 33 +--
>  tests/es1370-test.c|  4 +--
>  tests/fdc-test.c   |  4 +--
>  tests/hd-geo-test.c| 16 +--
>  tests/i440fx-test.c| 16 +--
>  tests/i82801b11-test.c |  5 ++--
>  tests/ide-test.c   |  4 +--
>  tests/intel-hda-test.c | 11 
>  tests/ioh3420-test.c   |  7 ++---
>  tests/ipmi-bt-test.c   | 11 +++-
>  tests/ipmi-kcs-test.c  |  5 +---
>  tests/ipoctal232-test.c|  5 ++--
>  tests/ivshmem-test.c   |  4 +--
>  tests/libqos/libqos.c  |  2 +-
>  tests/m25p80-test.c|  9 +++
>  tests/m48t59-test.c|  2 +-
>  tests/ne2000-test.c|  4 +--
>  tests/numa-test.c  | 28 ++--
>  tests/nvme-test.c  |  7 ++---
>  tests/pc-cpu-test.c| 26 +++---
>  tests/pcnet-test.c |  4 +--
>  tests/pnv-xscom-test.c | 14 ++
>  tests/prom-env-test.c  | 13 -
>  tests/pvpanic-test.c   |  4 +--
>  tests/pxe-test.c   | 14 --
>  tests/q35-test.c   |  8 +++---
>  tests/qom-test.c   |  7 ++---
>  tests/rtc-test.c   |  2 +-
>  tests/rtl8139-test.c   |  4 +--
>  tests/spapr-phb-test.c |  5 ++--
>  tests/tco-test.c   | 12 -
>  tests/test-arm-mptimer.c   |  4 +--
>  tests/test-filter-mirror.c | 16 +--
>  tests/test-filter-redirector.c | 60 
> --
>  tests/test-hmp.c   |  7 ++---
>  tests/test-netfilter.c |  9 +++
>  tests/test-x86-cpuid-compat.c  | 13 -
>  tests/tmp105-test.c|  2 +-
>  tests/tpci200-test.c   |  4 +--
>  tests/usb-hcd-ehci-test.c  | 25 +-
>  tests/usb-hcd-ohci-test.c  |  4 +--
>  tests/usb-hcd-xhci-test.c  |  4 +--
>  tests/virtio-balloon-test.c|  4 +--
>  tests/virtio-blk-test.c| 13 -
>  tests/virtio-console-test.c|  8 +++---
>  tests/virtio-net-test.c|  4 +--
>  tests/virtio-rng-test.c|  4 +--
>  tests/virtio-serial-test.c |  4 +--
>  tests/vmgenid-test.c   | 29 ++--
>  tests/vmxnet3-test.c   |  4 +--
>  62 files changed, 267 insertions(+), 394 deletions(-)
> 
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index d338de3e22..0459526187 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -509,32 +509,6 @@ void qtest_add_data_func_full(const char *str, void 
> *data,
>  void qtest_add_abrt_handler(GHookFunc fn, const void *data);
> 
>  /**
> - * qtest_start:
> - * @args: other arguments to pass to QEMU
> - *
> - * Start QEMU and assign the resulting #QTestState to a global variable.
> - * The global variable is used by "shortcut" functions documented below.
> - *
> - * Returns: #QTestState instance.
> - */
> -static inline QTestState *qtest_start(const char *args)
> -{
> -global_qtest = qtest_init("%s", args);
> -return global_qtest;
> -}
> -
> -/**
> - * qtest_end:
> - *
> - * Shut down the QEMU process started by qtest_start().
> - */
> -static inline void qtest_end(void)
> -{
> -qtest_quit(global_qtest);
> -global_qtest = NULL;
> -}
> -
> -/**
>   * qmp:
>   * @fmt...: QMP message to send to qemu
>   *
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 18facbf130..fa4e47c137 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -970,7 +970,7 @@ void qtest_cb_for_every_machine(void (*cb)(const char 
> *machine))
>  QString *qstr;
>  const char *mname;
> 
> -qtest_start("-machine none");
> +global_qtest = qtest_init("-machine none");
>  response = qmp("{ 'execute': 'query-machines' }");
>  g_assert(response);
>  list = 

Re: [Qemu-block] [Qemu-devel] Persistent bitmaps for non-qcow2 formats

[Kevin Wolf]

Am 28.08.2017 um 04:57 hat Fam Zheng geschrieben:
> On Fri, 08/25 15:44, Max Reitz wrote:
> > Well, OK.  The main argument against supporting anything but qcow2 is
> > "if you want features, use qcow2; and we are working on making qcow2 as
> > fast as possible."  I think that's a very good argument still.  At some
> > point I (and probably others, too) had the idea of making qcow2 files in
> > raw layout: 
> 
> Yes! I think this idea makes a whole lot of sense, too. Metadata tables can be
> generated so old implementation can still use it.

Maybe a nice way to attack this would be "huge pages", i.e. have a L1
entry flag that tells "this points directly to a huge cluster instead of
an L2 table". Gives us 512 MB clusters with a 64k cluster size, or a
maximum of 512 GB clusters with a 2 MB cluster size.

Huge clusters would only be used by qemu-img create if the respective
option is given, and only with preallocation.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v6 16/29] libqos: Use explicit QTestState for virtio operations

[Paolo Bonzini]

>  typedef struct QVirtioDevice {
>  const QVirtioBus *bus;
> +QTestState *qts;
>  /* Device type */
>  uint16_t device_type;
>  } QVirtioDevice;
> @@ -35,12 +36,14 @@ typedef struct QVirtQueue {
>  uint16_t last_used_idx;
>  bool indirect;
>  bool event;
> +QTestState *qts;
>  } QVirtQueue;
>
>  typedef struct QVRingIndirectDesc {
>  uint64_t desc; /* This points to an array fo struct vring_desc */
>  uint16_t index;
>  uint16_t elem;
> +QTestState *qts;
>  } QVRingIndirectDesc;

Adding QTestState to QVRingIndirectDesc and QVirtQueue sounds somewhat
ugly to me. I think they should either rather have a pointer to the
associated QVirtioDevice, or the functions where this is needed
(qvring_init() for example) should get a "QTestState *" parameter instead.


I agree with Thomas and even extend it to QVirtioDevice. If there is a
has-a hierarchy between libqos objects, only the topmost one (such as the
virtio mmio bus device, and the PCI host bridge---or possibly even the
machine object exclusively) should have a QTestState. Everyone else can
access it implicitly through operations on its bus.

Test code on the other hand can use global_qtest implicitly when they
prepare/read buffers that the devices do DMA from/to.

Paolo



Just my 0.02 €.

 Thomas

Re: [Qemu-block] [PATCH v4] docs: add qemu-block-drivers(7) man page

[Stefan Hajnoczi]

On Tue, Sep 05, 2017 at 11:03:03AM +0200, Kevin Wolf wrote:
> Am 04.09.2017 um 18:26 hat Stefan Hajnoczi geschrieben:
> > Block driver documentation is available in qemu-doc.html.  It would be
> > convenient to have documentation for formats, protocols, and filter
> > drivers in a man page.
> > 
> > Extract the relevant part of qemu-doc.html into a new file called
> > docs/qemu-block-drivers.texi.  This file can also be built as a
> > stand-alone document (man, html, etc).
> > 
> > Signed-off-by: Stefan Hajnoczi 
> 
> I'm getting a few warning during the build:
> 
> docs/qemu-block-drivers.texi:790: warning: @setfilename after the first 
> element
> docs/qemu-block-drivers.texi:790: warning: @setfilename after the first 
> element
> docs/qemu-block-drivers.texi:791: warning: multiple @settitle
> docs/qemu-block-drivers.texi:791: warning: multiple @settitle

Thanks, will fix!

Stefan

Re: [Qemu-block] Persistent bitmaps for non-qcow2 formats

[Kevin Wolf]

Am 22.08.2017 um 21:07 hat John Snow geschrieben:
> Well, we knew we'd want this sooner or later. I've got some pings
> downstream over whether or not we support persistent bitmaps for
> non-qcow2 formats.
> 
> Currently: no, we don't.

I was going to write some things about r/w backing file, and I might
still do that later, but for now, let me take a step back:

What is the use case here?

Using a specific image format is not a value in and of itself; it should
be chosen depending on which features you need. What prevents those
downstream users from simply choosing qcow2 for their data, too?

Kevin

Re: [Qemu-block] [PATCH v3 2/5] qemu-iotests: remove file cleanup from bash tests

[Kevin Wolf]

Am 30.08.2017 um 18:52 hat Jeff Cody geschrieben:
> All files for a given test are now self-contained in a subdirectory,
> and therefore the "./check" script can do all file-related cleanup
> without any help.
> 
> This removes file cleanups from the bash tests.  The only cleanup left
> is whatever is needed to kill any spawned processes; e.g. _cleanup_qemu.
> 
> Reviewed-by: Eric Blake 
> Signed-off-by: Jeff Cody 

What about non-file protocols? _cleanup_test_img() does more than just a
few 'rm' commands. For NBD and vxhs it also kills the server process and
for sheepdog and rbd it uses their specific tools to delete the image
from the cluster because it's not simply a local file.

Kevin

Re: [Qemu-block] [PATCH v2 0/7] Refactor DMG driver to have chunk size independence

[Ashijeet Acharya]

On Tue, Sep 5, 2017 at 4:28 PM, Stefan Hajnoczi  wrote:

> On Wed, Aug 30, 2017 at 06:32:52PM +0530, Ashijeet Acharya wrote:
> > On Tue, Aug 29, 2017 at 8:55 PM, Stefan Hajnoczi 
> wrote:
> >
> > > On Sun, Aug 20, 2017 at 1:47 PM, Ashijeet Acharya
> > >  wrote:
> > > > On Fri, May 5, 2017 at 7:29 PM, Stefan Hajnoczi 
> > > wrote:
> > > >>
> > > >> On Thu, Apr 27, 2017 at 01:36:30PM +0530, Ashijeet Acharya wrote:
> > > >> > This series helps to provide chunk size independence for DMG
> driver to
> > > >> > prevent
> > > >> > denial-of-service in cases where untrusted files are being
> accessed by
> > > >> > the user.
> > > >>
> > > >> The core of the chunk size dependence problem are these lines:
> > > >>
> > > >> s->compressed_chunk = qemu_try_blockalign(bs->file->bs,
> > > >>
>  ds.max_compressed_size +
> > > 1);
> > > >> s->uncompressed_chunk = qemu_try_blockalign(bs->file->bs,
> > > >> 512 *
> > > >> ds.max_sectors_per_chunk);
> > > >>
> > > >> The refactoring needs to eliminate these buffers because their size
> is
> > > >> controlled by the untrusted input file.
> > > >
> > > >
> > > > Oh okay, I understand now. But wouldn't I still need to allocate some
> > > memory
> > > > for these buffers to be able to use them for the compressed chunks
> case
> > > you
> > > > mentioned below. Instead of letting the DMG images control the size
> of
> > > these
> > > > buffers, maybe I can hard-code the size of these buffers instead?
> > > >
> > > >>
> > > >>
> > > >> After applying your patches these lines remain unchanged and we
> still
> > > >> cannot use input files that have a 250 MB chunk size, for example.
> So
> > > >> I'm not sure how this series is supposed to work.
> > > >>
> > > >> Here is the approach I would take:
> > > >>
> > > >> In order to achieve this dmg_read_chunk() needs to be scrapped.  It
> is
> > > >> designed to read a full chunk.  The new model does not read full
> chunks
> > > >> anymore.
> > > >>
> > > >> Uncompressed reads or zeroes should operate directly on qiov, not
> > > >> s->uncompressed_chunk.  This code will be dropped:
> > > >>
> > > >> data = s->uncompressed_chunk + sector_offset_in_chunk * 512;
> > > >> qemu_iovec_from_buf(qiov, i * 512, data, 512);
> > > >
> > > >
> > > > I have never worked with qiov before, are there any places where I
> can
> > > refer
> > > > to inside other drivers to get the idea of how to use it directly (I
> am
> > > > searching by myself in the meantime...)?
> > >
> > > A QEMUIOVector is a utility type for struct iovec iov[] processing.
> > > See util/iov.c.  This is called "vectored" or "scatter-gather" I/O.
> > >
> > > Instead of transferring data to/from a single  tuple,
> > > they take an array [].  For example, the buffer "Hello
> > > world" could be split into two elements:
> > > [{"Hello ", strlen("Hello ")},
> > >  {"world", strlen("world")}]
> > >
> > > Vectored I/O is often used because it eliminates memory copies.  Say
> > > you have a network packet header struct and also a data payload array.
> > > Traditionally you would have to allocate a new buffer large enough for
> > > both header and payload, copy the header and payload into the buffer,
> > > and finally give this temporary buffer to the I/O function.  This is
> > > inefficient.  With vectored I/O you can create a vector with two
> > > elements, the header and the payload, and the I/O function can process
> > > them without needing a temporary buffer copy.
> > >
> >
> > Thanks for the detailed explanation, I think I understood the concept now
> > and how to use qiov efficiently.
> > Correct me if I am wrong here. In order to use qiov directly for
> > uncompressed chunks:
> >
> > 1. Declare a new local_qiov inside dmg_co_preadv (let's say)
>
> No, the operation should use qiov directly if the chunk is uncompressed.
>
> A temporary buffer is only needed if the data needs to be uncompressed.
>

Yes, I had a chat with John and he cleared most of my doubts on how to
approach it correctly now without using a temporary buffer.


>
> > 2. Initialize it with qemu_iovec_init()
> > 3. Reset it with qemu_iovec_reset() (this is because we will perform this
> > action in a loop and thus need to reset it before every loop?)
> > 4. Declare a buffer "uncompressed_buf" and allocate it with
> > qemu_try_blockalign()
> > 5. Add this buffer to our local_qiov using qemu_iovec_add()
> > 6. Read data from file directly into local_qiov using bdrv_co_preadv()
> > 7. On success concatenate it with the qiov passed into the main
> > dmg_co_preadv() function.
> >
> > I think this method only works for uncompressed chunks. For the
> compressed
> > ones, I believe we will still need to do it in the existing way, i.e.
> read
> > chunk from file -> decompress into output buffer -> use
> > qemu_iovec_from_buf() because we cannot 

Re: [Qemu-block] [Qemu-devel] [PATCH v6 14/29] libqos: Use explicit QTestState for fw_cfg operations

[Thomas Huth]

On 05.09.2017 12:12, Thomas Huth wrote:
> On 01.09.2017 20:03, Eric Blake wrote:
>> Drop one more client of global_qtest by teaching all fw_cfg test
>> functionality (invoked through alloc-pc) to pass in an explicit
>> QTestState, adjusting all callers.  In particular, fw_cfg-test
>> had to reorder things to create the test state prior to creating
>> the fw_cfg.
>>
>> Signed-off-by: Eric Blake 
>> ---
>>  tests/libqos/fw_cfg.h   | 10 ++
>>  tests/libqos/libqos.h   |  2 +-
>>  tests/libqos/malloc-pc.h|  4 ++--
>>  tests/libqos/malloc-spapr.h |  2 +-
>>  tests/libqos/malloc.h   |  1 +
>>  tests/boot-order-test.c |  6 +++---
>>  tests/e1000e-test.c |  2 +-
>>  tests/fw_cfg-test.c | 14 ++
>>  tests/ide-test.c|  2 +-
>>  tests/libqos/fw_cfg.c   | 14 --
>>  tests/libqos/libqos.c   |  2 +-
>>  tests/libqos/malloc-pc.c|  8 
>>  tests/libqos/malloc-spapr.c |  4 ++--
>>  tests/vhost-user-test.c |  2 +-
>>  14 files changed, 38 insertions(+), 35 deletions(-)
>>
>> diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
>> index e8371b2317..396dd4ee1e 100644
>> --- a/tests/libqos/fw_cfg.h
>> +++ b/tests/libqos/fw_cfg.h
>> @@ -15,10 +15,12 @@
>>
>>
>>  typedef struct QFWCFG QFWCFG;
>> +typedef struct QTestState QTestState;
> 
> Not sure, but I slightly remember that typedeffing a struct like this in
> multiple places can cause compiler warnings or errors with certain
> versions of GCC or clang? So a file that includes both, fw_cfg.h and
> libqtest.h will then fail to compile?
> 
> I think it would be better to change the include order in the .c files
> instead, so that libqtest.h is always included before fw_cfg.h.

Ah, well, I just saw that you also sent a fixup patch for this. Anyway,
I'm not a fan of including header files from other header files, so
changing the include order in the .c files sounds like the better
solution to me.

 Thomas

Re: [Qemu-block] [PATCH v2 0/7] Refactor DMG driver to have chunk size independence

[Stefan Hajnoczi]

On Wed, Aug 30, 2017 at 06:32:52PM +0530, Ashijeet Acharya wrote:
> On Tue, Aug 29, 2017 at 8:55 PM, Stefan Hajnoczi  wrote:
> 
> > On Sun, Aug 20, 2017 at 1:47 PM, Ashijeet Acharya
> >  wrote:
> > > On Fri, May 5, 2017 at 7:29 PM, Stefan Hajnoczi 
> > wrote:
> > >>
> > >> On Thu, Apr 27, 2017 at 01:36:30PM +0530, Ashijeet Acharya wrote:
> > >> > This series helps to provide chunk size independence for DMG driver to
> > >> > prevent
> > >> > denial-of-service in cases where untrusted files are being accessed by
> > >> > the user.
> > >>
> > >> The core of the chunk size dependence problem are these lines:
> > >>
> > >> s->compressed_chunk = qemu_try_blockalign(bs->file->bs,
> > >>   ds.max_compressed_size +
> > 1);
> > >> s->uncompressed_chunk = qemu_try_blockalign(bs->file->bs,
> > >> 512 *
> > >> ds.max_sectors_per_chunk);
> > >>
> > >> The refactoring needs to eliminate these buffers because their size is
> > >> controlled by the untrusted input file.
> > >
> > >
> > > Oh okay, I understand now. But wouldn't I still need to allocate some
> > memory
> > > for these buffers to be able to use them for the compressed chunks case
> > you
> > > mentioned below. Instead of letting the DMG images control the size of
> > these
> > > buffers, maybe I can hard-code the size of these buffers instead?
> > >
> > >>
> > >>
> > >> After applying your patches these lines remain unchanged and we still
> > >> cannot use input files that have a 250 MB chunk size, for example.  So
> > >> I'm not sure how this series is supposed to work.
> > >>
> > >> Here is the approach I would take:
> > >>
> > >> In order to achieve this dmg_read_chunk() needs to be scrapped.  It is
> > >> designed to read a full chunk.  The new model does not read full chunks
> > >> anymore.
> > >>
> > >> Uncompressed reads or zeroes should operate directly on qiov, not
> > >> s->uncompressed_chunk.  This code will be dropped:
> > >>
> > >> data = s->uncompressed_chunk + sector_offset_in_chunk * 512;
> > >> qemu_iovec_from_buf(qiov, i * 512, data, 512);
> > >
> > >
> > > I have never worked with qiov before, are there any places where I can
> > refer
> > > to inside other drivers to get the idea of how to use it directly (I am
> > > searching by myself in the meantime...)?
> >
> > A QEMUIOVector is a utility type for struct iovec iov[] processing.
> > See util/iov.c.  This is called "vectored" or "scatter-gather" I/O.
> >
> > Instead of transferring data to/from a single  tuple,
> > they take an array [].  For example, the buffer "Hello
> > world" could be split into two elements:
> > [{"Hello ", strlen("Hello ")},
> >  {"world", strlen("world")}]
> >
> > Vectored I/O is often used because it eliminates memory copies.  Say
> > you have a network packet header struct and also a data payload array.
> > Traditionally you would have to allocate a new buffer large enough for
> > both header and payload, copy the header and payload into the buffer,
> > and finally give this temporary buffer to the I/O function.  This is
> > inefficient.  With vectored I/O you can create a vector with two
> > elements, the header and the payload, and the I/O function can process
> > them without needing a temporary buffer copy.
> >
> 
> Thanks for the detailed explanation, I think I understood the concept now
> and how to use qiov efficiently.
> Correct me if I am wrong here. In order to use qiov directly for
> uncompressed chunks:
> 
> 1. Declare a new local_qiov inside dmg_co_preadv (let's say)

No, the operation should use qiov directly if the chunk is uncompressed.

A temporary buffer is only needed if the data needs to be uncompressed.

> 2. Initialize it with qemu_iovec_init()
> 3. Reset it with qemu_iovec_reset() (this is because we will perform this
> action in a loop and thus need to reset it before every loop?)
> 4. Declare a buffer "uncompressed_buf" and allocate it with
> qemu_try_blockalign()
> 5. Add this buffer to our local_qiov using qemu_iovec_add()
> 6. Read data from file directly into local_qiov using bdrv_co_preadv()
> 7. On success concatenate it with the qiov passed into the main
> dmg_co_preadv() function.
> 
> I think this method only works for uncompressed chunks. For the compressed
> ones, I believe we will still need to do it in the existing way, i.e. read
> chunk from file -> decompress into output buffer -> use
> qemu_iovec_from_buf() because we cannot read directly since data is in
> compressed form. Makes sense?
> 
> 
> > > I got clearly what you are trying
> > > to say, but don't know how to implement it. I think, don't we already do
> > > that for the zeroed chunks in DMG in dmg_co_preadv()?
> >
> > Yes, dmg_co_preadv() directly zeroes the qiov.  It doesn't use
> > s->compressed_chunk/s->uncompressed_chunk.
> >
> > >
> > >>
> > >>
> > >> 

Re: [Qemu-block] [PATCH] block: Cleanup BMDS in bdrv_close_all

[Stefan Hajnoczi]

On Tue, Sep 05, 2017 at 11:28:44AM +0100, Stefan Hajnoczi wrote:
> On Wed, Aug 30, 2017 at 06:06:05PM +0800, Fam Zheng wrote:
> > This fixes the assertion due to op blockers added by BMDS:
> > 
> > block.c:3248: bdrv_delete: Assertion `bdrv_op_blocker_is_empty(bs)' failed.
> > 
> > Reproducer: simply start block migration and quit QEMU before it ends.
> > 
> > Cc: qemu-sta...@nongnu.org
> > Signed-off-by: Fam Zheng 
> > ---
> >  block.c | 2 ++
> >  migration/block.c   | 2 +-
> >  migration/block.h   | 1 +
> >  stubs/Makefile.objs | 1 +
> >  stubs/block-migration.c | 6 ++
> >  5 files changed, 11 insertions(+), 1 deletion(-)
> >  create mode 100644 stubs/block-migration.c
> 
> Thanks, applied to my block tree:
> https://github.com/stefanha/qemu/commits/block

Dropped again pending the issue Kevin raised.

Stefan

Re: [Qemu-block] [Qemu-devel] [PATCH v6 17/29] ahci-test: Drop dependence on global_qtest

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> Managing parallel connections to two different monitors via
> the implicit global_qtest makes it hard to copy-and-paste code
> to tests that are not aware of the implicit state; the
> management of global_qtest is even harder to follow because
> it was masked behind set_context().
> 
> Instead, explicitly pass QTestState* around (generally, by
> reusing the member already present in ahci->parent QOSState),
> and call explicit qtest_* functions on all places that
> interact with a monitor.
> 
> We can assert that the conversion is correct by checking that
> global_qtest remains NULL throughout the test (a later patch
> that changes global_qtest to not be a public global variable
> will drop the assertions).
> 
> Bonus: there were several spots that were constructing a JSON
> string, then passing that through qmp() as the format, rather
> than directly using qmp() to construct the JSON.  Fixing that
> gets us one step closer to enabling -Wformat checking on
> constructed JSON.
> 
> Signed-off-by: Eric Blake 
> ---
>  tests/libqos/libqos.h|  1 -
>  tests/ahci-test.c| 83 
> +++-
>  tests/libqos/ahci.c  | 45 +-
>  tests/libqos/libqos-pc.c |  2 +-
>  tests/libqos/libqos.c| 37 ++---
>  5 files changed, 73 insertions(+), 95 deletions(-)

Might be easier to review if you'd split the changes to libqos.c into a
separate patch. But anyway:

Reviewed-by: Thomas Huth 

Re: [Qemu-block] [PATCH] block: Cleanup BMDS in bdrv_close_all

[Stefan Hajnoczi]

On Wed, Aug 30, 2017 at 06:06:05PM +0800, Fam Zheng wrote:
> This fixes the assertion due to op blockers added by BMDS:
> 
> block.c:3248: bdrv_delete: Assertion `bdrv_op_blocker_is_empty(bs)' failed.
> 
> Reproducer: simply start block migration and quit QEMU before it ends.
> 
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Fam Zheng 
> ---
>  block.c | 2 ++
>  migration/block.c   | 2 +-
>  migration/block.h   | 1 +
>  stubs/Makefile.objs | 1 +
>  stubs/block-migration.c | 6 ++
>  5 files changed, 11 insertions(+), 1 deletion(-)
>  create mode 100644 stubs/block-migration.c

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

Stefan

Re: [Qemu-block] [Qemu-devel] [PATCH v6 16/29] libqos: Use explicit QTestState for virtio operations

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> Drop one more client of global_qtest by teaching all virtio test
> functionality to pass in an explicit QTestState in constructors,
> where it is then reused for later access.  Adjust all callers.
> This gets us one step closer to eliminating implicit use of
> global_qtest.
> 
> Signed-off-by: Eric Blake 
> ---
>  tests/libqos/virtio-mmio.h |  3 +-
>  tests/libqos/virtio.h  |  5 ++-
>  tests/libqos/virtio-mmio.c | 54 +---
>  tests/libqos/virtio-pci.c  |  3 ++
>  tests/libqos/virtio.c  | 77 
> ++
>  tests/virtio-blk-test.c|  3 +-
>  6 files changed, 84 insertions(+), 61 deletions(-)
> 
> diff --git a/tests/libqos/virtio-mmio.h b/tests/libqos/virtio-mmio.h
> index e3e52b9ce1..bd01386054 100644
> --- a/tests/libqos/virtio-mmio.h
> +++ b/tests/libqos/virtio-mmio.h
> @@ -41,6 +41,7 @@ typedef struct QVirtioMMIODevice {
> 
>  extern const QVirtioBus qvirtio_mmio;
> 
> -QVirtioMMIODevice *qvirtio_mmio_init_device(uint64_t addr, uint32_t 
> page_size);
> +QVirtioMMIODevice *qvirtio_mmio_init_device(QTestState *qts, uint64_t addr,
> +uint32_t page_size);
> 
>  #endif
> diff --git a/tests/libqos/virtio.h b/tests/libqos/virtio.h
> index 8fbcd1869c..d180d54fc4 100644
> --- a/tests/libqos/virtio.h
> +++ b/tests/libqos/virtio.h
> @@ -19,6 +19,7 @@ typedef struct QVirtioBus QVirtioBus;
> 
>  typedef struct QVirtioDevice {
>  const QVirtioBus *bus;
> +QTestState *qts;
>  /* Device type */
>  uint16_t device_type;
>  } QVirtioDevice;
> @@ -35,12 +36,14 @@ typedef struct QVirtQueue {
>  uint16_t last_used_idx;
>  bool indirect;
>  bool event;
> +QTestState *qts;
>  } QVirtQueue;
> 
>  typedef struct QVRingIndirectDesc {
>  uint64_t desc; /* This points to an array fo struct vring_desc */
>  uint16_t index;
>  uint16_t elem;
> +QTestState *qts;
>  } QVRingIndirectDesc;

Adding QTestState to QVRingIndirectDesc and QVirtQueue sounds somewhat
ugly to me. I think they should either rather have a pointer to the
associated QVirtioDevice, or the functions where this is needed
(qvring_init() for example) should get a "QTestState *" parameter instead.

Just my 0.02 €.

 Thomas

Re: [Qemu-block] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

[Kevin Wolf]

Am 05.09.2017 um 12:05 hat Daniel P. Berrange geschrieben:
> On Tue, Sep 05, 2017 at 11:52:15AM +0200, Kevin Wolf wrote:
> > Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
> > > Signed-off-by: Daniel P. Berrange 
> > > ---
> > >  block/crypto.c | 32 
> > >  1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> > I'm actually not sure about this one. Anything that is left after
> > patch 3 is probably not the arbitrary unit that qemu uses internally
> > for some interfaces, but the unit in which data is encrypted.
> > 
> > Basically, if just for fun we ever changed the unit of things like
> > bdrv_write() from 512 to 4096, then everything that needs to or at least
> > can be changed to use 4096 is BDRV_SECTOR_SIZE. But everything that
> > needs to stay on 512 (like I suspect most of the occurrences in the
> > crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).
> 
> Yeah for sure LUKSv1 & legacy qcow2 encryption need to stay 512
> forever, so if BDRV_SECTOR_SIZE were to change that would be a
> problem.
> 
> I wrote this on the assumption that we would never change BDRV_SECTOR_SIZE
> though. If we did need different sector sizes in the block layer I figured
> it would surely end up being a dynamic property per disk, rather than just
> changing the compile time constant. So from that POV I thought it ok to
> use BDRV_SECTOR_SIZE.

Yes, I agree that we'll never BDRV_SECTOR_SIZE in practice. I just use
this as a way to check whether this is really BDRV_SECTOR_SIZE, or
whether we have two different quantities that just happen to be both
512.

For example, QDICT_BUCKET_MAX is also 512, but that doesn't make it
right to use here, even though it works and is unlikely to change.

What we may actually want to do at some point (won't be in the near
future, though) is removing BDRV_SECTOR_SIZE because it refers to an
arbitrary unit in APIs and we're in the process of making everything
byte-based instead. This is another way to check whether some value is
BDRV_SECTOR_SIZE: If it will remain even after removing APIs with
sector-based parameters, it's probably something different.

> Perhaps I should instead add a qcrypto_block_get_sector_size() API though
> and use that, so we can fetch the sector size per encryption scheme in
> case we ever get a scheme using a non-512 sector size for encryption.

If you have a use for it, sure. Otherwise, I'd just suggest introducing
another constant for now.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v6 14/29] libqos: Use explicit QTestState for fw_cfg operations

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> Drop one more client of global_qtest by teaching all fw_cfg test
> functionality (invoked through alloc-pc) to pass in an explicit
> QTestState, adjusting all callers.  In particular, fw_cfg-test
> had to reorder things to create the test state prior to creating
> the fw_cfg.
> 
> Signed-off-by: Eric Blake 
> ---
>  tests/libqos/fw_cfg.h   | 10 ++
>  tests/libqos/libqos.h   |  2 +-
>  tests/libqos/malloc-pc.h|  4 ++--
>  tests/libqos/malloc-spapr.h |  2 +-
>  tests/libqos/malloc.h   |  1 +
>  tests/boot-order-test.c |  6 +++---
>  tests/e1000e-test.c |  2 +-
>  tests/fw_cfg-test.c | 14 ++
>  tests/ide-test.c|  2 +-
>  tests/libqos/fw_cfg.c   | 14 --
>  tests/libqos/libqos.c   |  2 +-
>  tests/libqos/malloc-pc.c|  8 
>  tests/libqos/malloc-spapr.c |  4 ++--
>  tests/vhost-user-test.c |  2 +-
>  14 files changed, 38 insertions(+), 35 deletions(-)
> 
> diff --git a/tests/libqos/fw_cfg.h b/tests/libqos/fw_cfg.h
> index e8371b2317..396dd4ee1e 100644
> --- a/tests/libqos/fw_cfg.h
> +++ b/tests/libqos/fw_cfg.h
> @@ -15,10 +15,12 @@
> 
> 
>  typedef struct QFWCFG QFWCFG;
> +typedef struct QTestState QTestState;

Not sure, but I slightly remember that typedeffing a struct like this in
multiple places can cause compiler warnings or errors with certain
versions of GCC or clang? So a file that includes both, fw_cfg.h and
libqtest.h will then fail to compile?

I think it would be better to change the include order in the .c files
instead, so that libqtest.h is always included before fw_cfg.h.

 Thomas

Re: [Qemu-block] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

[Daniel P. Berrange]

On Tue, Sep 05, 2017 at 11:52:15AM +0200, Kevin Wolf wrote:
> Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  block/crypto.c | 32 
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> I'm actually not sure about this one. Anything that is left after
> patch 3 is probably not the arbitrary unit that qemu uses internally
> for some interfaces, but the unit in which data is encrypted.
> 
> Basically, if just for fun we ever changed the unit of things like
> bdrv_write() from 512 to 4096, then everything that needs to or at least
> can be changed to use 4096 is BDRV_SECTOR_SIZE. But everything that
> needs to stay on 512 (like I suspect most of the occurrences in the
> crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).

Yeah for sure LUKSv1 & legacy qcow2 encryption need to stay 512
forever, so if BDRV_SECTOR_SIZE were to change that would be a
problem.

I wrote this on the assumption that we would never change BDRV_SECTOR_SIZE
though. If we did need different sector sizes in the block layer I figured
it would surely end up being a dynamic property per disk, rather than just
changing the compile time constant. So from that POV I thought it ok to
use BDRV_SECTOR_SIZE.

Perhaps I should instead add a qcrypto_block_get_sector_size() API though
and use that, so we can fetch the sector size per encryption scheme in
case we ever get a scheme using a non-512 sector size for encryption.

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

Re: [Qemu-block] [PATCH v4] block: document semantics of bdrv_co_preadv|pwritev

[Stefan Hajnoczi]

On Thu, Aug 31, 2017 at 11:54:56AM +0100, Daniel P. Berrange wrote:
> Reviewed-by: Stefan Hajnoczi 
> Reviewed-by: Eric Blake 
> Signed-off-by: Daniel P. Berrange 
> ---
> 
> Changed in v4:
> 
>   - Fix typo in commit message
> 
>  include/block/block_int.h | 31 +++
>  1 file changed, 31 insertions(+)

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

Stefan

Re: [Qemu-block] [PATCH 2/2] iotests: blacklist 194 with the luks driver

[Stefan Hajnoczi]

On Fri, Sep 01, 2017 at 11:54:34AM +0100, Daniel P. Berrange wrote:
> The 194 test has alot of code that assumes a simple image file. Rewriting
> this to work with luks is possible, but non-trivial, so blacklist the
> luks format for now.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  tests/qemu-iotests/194| 1 +
>  tests/qemu-iotests/iotests.py | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi 

Re: [Qemu-block] [PATCH] block: Cleanup BMDS in bdrv_close_all

[Juan Quintela]

Kevin Wolf  wrote:
> Am 05.09.2017 um 10:54 hat Fam Zheng geschrieben:
>> On Tue, 09/05 10:44, Kevin Wolf wrote:
>> > Am 30.08.2017 um 12:06 hat Fam Zheng geschrieben:
>> > > This fixes the assertion due to op blockers added by BMDS:
>> > > 
>> > > block.c:3248: bdrv_delete: Assertion `bdrv_op_blocker_is_empty(bs)' 
>> > > failed.
>> > > 
>> > > Reproducer: simply start block migration and quit QEMU before it ends.
>> > > 
>> > > Cc: qemu-sta...@nongnu.org
>> > > Signed-off-by: Fam Zheng 
>> > > ---
>> > >  block.c | 2 ++
>> > >  migration/block.c   | 2 +-
>> > >  migration/block.h   | 1 +
>> > >  stubs/Makefile.objs | 1 +
>> > >  stubs/block-migration.c | 6 ++
>> > >  5 files changed, 11 insertions(+), 1 deletion(-)
>> > >  create mode 100644 stubs/block-migration.c
>> > > 
>> > > diff --git a/block.c b/block.c
>> > > index 3308814bba..508a57274d 100644
>> > > --- a/block.c
>> > > +++ b/block.c
>> > > @@ -43,6 +43,7 @@
>> > >  #include "qemu/cutils.h"
>> > >  #include "qemu/id.h"
>> > >  #include "qapi/util.h"
>> > > +#include "migration/block.h"
>> > >  
>> > >  #ifdef CONFIG_BSD
>> > >  #include 
>> > > @@ -3111,6 +3112,7 @@ static void bdrv_close(BlockDriverState *bs)
>> > >  
>> > >  void bdrv_close_all(void)
>> > >  {
>> > > +block_migration_cleanup_bmds();
>> > >  block_job_cancel_sync_all();
>> > >  nbd_export_close_all();
>> > 
>> > This is before bdrv_drain_all(). Can't we still have a block migration
>> > request in flight, whose callback will then dereference a stale pointer?
>> 
>> You're right, bdrv_drain_all should be called first.
>
> Actually, looking a bit closer, what prevents the migration thread from
> starting new requests even after bdrv_drain_all()? Maybe what we really
> need to do is cancelling the migration before calling bdrv_close_all().

I was wondering *where* to put this call inside the migration cleanup
code, but I got to the conclusion that I was not sure that the migration
cancellation code got called when you just do a "quit".

Later, Juan.

Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] block: add block device shared field

[Stefan Hajnoczi]

On Fri, Sep 01, 2017 at 08:10:54PM +, Brian Steffens wrote:
> This adds a boolean option called 'shared' to block devices. It defaults
> to off/false. When enabled for a particular block device, the 'shared' option
> causes the block migration code to skip over syncing of that device. This
> allows controlling exactly which block devices get synced during a migration.
> 
> Signed-off-by: Brian Steffens 
> ---
>  block.c   | 7 +++
>  block/qapi.c  | 2 ++
>  include/block/block.h | 1 +
>  include/block/block_int.h | 3 +++
>  migration/block.c | 4 
>  qapi/block-core.json  | 2 +-
>  6 files changed, 18 insertions(+), 1 deletion(-)

Thanks for the patch!  Please email the relevant maintainers:

  $ scripts/get_maintainer.pl -f block.c
  Kevin Wolf  (supporter:Block layer core)
  Max Reitz  (supporter:Block layer core)
  qemu-block@nongnu.org (open list:Block layer core)
  qemu-de...@nongnu.org (open list:All patches CC here)

I have CCed them so they see this email.

> diff --git a/migration/block.c b/migration/block.c
> index 9171f60028..b347c3dc61 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -402,6 +402,10 @@ static int init_blk_migration(QEMUFile *f)
>  bmds_bs = g_malloc0(num_bs * sizeof(*bmds_bs));
>  
>  for (i = 0, bs = bdrv_first(); bs; bs = bdrv_next(), i++) {
> +if (bs->shared) {
> +continue;
> +}
> +

Have you considered extending the 'migrate' command with a list of
drives instead?

  { 'command': 'migrate',
'data': {'*blk': 'bool',
 '*drives': ['str'], <--- new!
 ...}}

That way the set of drives doesn't need to be decided until migration
time.  It avoids adding new state to BlockDriverState that is used only
by the legacy block/migration.c code.

In case you haven't seen it, the preferred approach for non-shared
storage migration is drive-mirror + NBD.  The block/migration.c code is
an older feature that is less flexible.  More info on drive-mirror +
NBD:
http://wiki.libvirt.org/page/NBD_storage_migration
Slide 29 in 
http://events.linuxfoundation.org/sites/events/files/slides/A-Practical-Look-at-QEMU-Block-Layer-Primitives.pdf

Stefan

Re: [Qemu-block] [PATCH v2 2/4] block: use BDRV_SECTOR_SIZE in crypto driver

[Kevin Wolf]

Am 31.08.2017 um 13:05 hat Daniel P. Berrange geschrieben:
> Signed-off-by: Daniel P. Berrange 
> ---
>  block/crypto.c | 32 
>  1 file changed, 16 insertions(+), 16 deletions(-)

I'm actually not sure about this one. Anything that is left after
patch 3 is probably not the arbitrary unit that qemu uses internally
for some interfaces, but the unit in which data is encrypted.

Basically, if just for fun we ever changed the unit of things like
bdrv_write() from 512 to 4096, then everything that needs to or at least
can be changed to use 4096 is BDRV_SECTOR_SIZE. But everything that
needs to stay on 512 (like I suspect most of the occurrences in the
crypto driver) is a different constant really (QCRYPTO_SECTOR_SIZE?).

Kevin

Re: [Qemu-block] [PATCH v4] block: document semantics of bdrv_co_preadv|pwritev

[Kevin Wolf]

Am 31.08.2017 um 12:54 hat Daniel P. Berrange geschrieben:
> Reviewed-by: Stefan Hajnoczi 
> Reviewed-by: Eric Blake 
> Signed-off-by: Daniel P. Berrange 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v6 12/29] libqos: Track QTestState with QPCIBus

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> When initializing a QPCIBus, track which QTestState the bus is
> associated with (so that a later patch can then explicitly use
> that test state for all communication on the bus, rather than
> blindly relying on global_qtest).  Update the initialization
> functions to take another parameter, and update all callers to
> pass in state (for now, most callers get away with passing the
> current global_qtest as the current state, although this required
> fixing the order of initialization to ensure qtest_start() is
> called before qpci_init*() in rtl8139-test, and provided an
> opportunity to pass in the allocator in e1000e-test).
> 
> Signed-off-by: Eric Blake 
> ---
[...]
> diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
> index 6226546c28..c95428e1cb 100644
> --- a/tests/libqos/libqos.c
> +++ b/tests/libqos/libqos.c
> @@ -26,8 +26,8 @@ QOSState *qtest_vboot(QOSOps *ops, const char *cmdline_fmt, 
> va_list ap)
>  if (ops->init_allocator) {
>  qs->alloc = ops->init_allocator(ALLOC_NO_FLAGS);
>  }
> -if (ops->qpci_init && qs->alloc) {
> -qs->pcibus = ops->qpci_init(qs->alloc);
> +if (ops->qpci_init) {

Why did you remove the check for qs->alloc?

> +qs->pcibus = ops->qpci_init(qs->qts, qs->alloc);
>  }
>  }
> 
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index 02ce49927a..85b34c6d13 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -115,11 +115,14 @@ static void qpci_pc_config_writel(QPCIBus *bus, int 
> devfn, uint8_t offset, uint3
>  outl(0xcfc, value);
>  }
> 
> -QPCIBus *qpci_init_pc(QGuestAllocator *alloc)
> +QPCIBus *qpci_init_pc(QTestState *qts, QGuestAllocator *alloc)
>  {
>  QPCIBusPC *ret;
> 
> +assert(qts);
> +
>  ret = g_malloc(sizeof(*ret));
> +ret->bus.qts = qts;
> 
>  ret->bus.pio_readb = qpci_pc_pio_readb;
>  ret->bus.pio_readw = qpci_pc_pio_readw;
> diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
> index 2043f1e123..cd9b8f52d2 100644
> --- a/tests/libqos/pci-spapr.c
> +++ b/tests/libqos/pci-spapr.c
> @@ -154,11 +154,14 @@ static void qpci_spapr_config_writel(QPCIBus *bus, int 
> devfn, uint8_t offset,
>  #define SPAPR_PCI_MMIO32_WIN_SIZE0x8000 /* 2 GiB */
>  #define SPAPR_PCI_IO_WIN_SIZE0x1
> 
> -QPCIBus *qpci_init_spapr(QGuestAllocator *alloc)
> +QPCIBus *qpci_init_spapr(QTestState *qts, QGuestAllocator *alloc)
>  {
>  QPCIBusSPAPR *ret;
> 
> +assert(qts);
> +
>  ret = g_malloc(sizeof(*ret));

+1 for using g_malloc0 here instead.

> +ret->bus.qts = qts;
> 
>  ret->alloc = alloc;
> 
> @@ -201,6 +204,7 @@ QPCIBus *qpci_init_spapr(QGuestAllocator *alloc)
>  ret->bus.mmio_alloc_ptr = ret->mmio32.pci_base;
>  ret->bus.mmio_limit = ret->mmio32.pci_base + ret->mmio32.size;
> 
> +

Superfluous white space change.

>  return >bus;
>  }

 Thomas

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] iotests: rewrite 192 to use _launch_qemu to fix LUKS support

[Kevin Wolf]

Am 01.09.2017 um 16:22 hat Eric Blake geschrieben:
> On 09/01/2017 05:54 AM, Daniel P. Berrange wrote:
> > The LUKS driver requires extra args to QEMU to setup passwords.
> > The _launch_qemu function takes care of this, so convert the
> > test to use this function and use correct -drive syntax
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  tests/qemu-iotests/192 | 23 ---
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> > 
> 
> >  
> > -{
> > -echo "nbd_server_start unix:$TEST_DIR/nbd"
> > -echo "nbd_server_add -w drive0"
> > -echo "q"
> > -} | $QEMU -nodefaults -display none -monitor stdio \
> > --drive format=$IMGFMT,file=$TEST_IMG,if=ide,id=drive0 \
> > --incoming defer 2>&1 | _filter_testdir | _filter_qemu | _filter_hmp
> 
> Can this test use QMP instead of HMP? But that's an independent question.

As far as I am concerned, there is nothing wrong with having HMP tests.
HMP commands already call QMP internally (except for the generic QMP
code path below the command handlers), so using HMP actually increases
the test coverage in most cases. We should try to have a good mix of
test cases using QMP and test cases using HMP.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH v4] docs: add qemu-block-drivers(7) man page

[Kevin Wolf]

Am 04.09.2017 um 18:26 hat Stefan Hajnoczi geschrieben:
> Block driver documentation is available in qemu-doc.html.  It would be
> convenient to have documentation for formats, protocols, and filter
> drivers in a man page.
> 
> Extract the relevant part of qemu-doc.html into a new file called
> docs/qemu-block-drivers.texi.  This file can also be built as a
> stand-alone document (man, html, etc).
> 
> Signed-off-by: Stefan Hajnoczi 

I'm getting a few warning during the build:

docs/qemu-block-drivers.texi:790: warning: @setfilename after the first element
docs/qemu-block-drivers.texi:790: warning: @setfilename after the first element
docs/qemu-block-drivers.texi:791: warning: multiple @settitle
docs/qemu-block-drivers.texi:791: warning: multiple @settitle

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH] block: Cleanup BMDS in bdrv_close_all

[Fam Zheng]

On Tue, 09/05 10:44, Kevin Wolf wrote:
> Am 30.08.2017 um 12:06 hat Fam Zheng geschrieben:
> > This fixes the assertion due to op blockers added by BMDS:
> > 
> > block.c:3248: bdrv_delete: Assertion `bdrv_op_blocker_is_empty(bs)' failed.
> > 
> > Reproducer: simply start block migration and quit QEMU before it ends.
> > 
> > Cc: qemu-sta...@nongnu.org
> > Signed-off-by: Fam Zheng 
> > ---
> >  block.c | 2 ++
> >  migration/block.c   | 2 +-
> >  migration/block.h   | 1 +
> >  stubs/Makefile.objs | 1 +
> >  stubs/block-migration.c | 6 ++
> >  5 files changed, 11 insertions(+), 1 deletion(-)
> >  create mode 100644 stubs/block-migration.c
> > 
> > diff --git a/block.c b/block.c
> > index 3308814bba..508a57274d 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -43,6 +43,7 @@
> >  #include "qemu/cutils.h"
> >  #include "qemu/id.h"
> >  #include "qapi/util.h"
> > +#include "migration/block.h"
> >  
> >  #ifdef CONFIG_BSD
> >  #include 
> > @@ -3111,6 +3112,7 @@ static void bdrv_close(BlockDriverState *bs)
> >  
> >  void bdrv_close_all(void)
> >  {
> > +block_migration_cleanup_bmds();
> >  block_job_cancel_sync_all();
> >  nbd_export_close_all();
> 
> This is before bdrv_drain_all(). Can't we still have a block migration
> request in flight, whose callback will then dereference a stale pointer?

You're right, bdrv_drain_all should be called first.

Fam

Re: [Qemu-block] [PATCH] block: Cleanup BMDS in bdrv_close_all

[Kevin Wolf]

Am 30.08.2017 um 12:06 hat Fam Zheng geschrieben:
> This fixes the assertion due to op blockers added by BMDS:
> 
> block.c:3248: bdrv_delete: Assertion `bdrv_op_blocker_is_empty(bs)' failed.
> 
> Reproducer: simply start block migration and quit QEMU before it ends.
> 
> Cc: qemu-sta...@nongnu.org
> Signed-off-by: Fam Zheng 
> ---
>  block.c | 2 ++
>  migration/block.c   | 2 +-
>  migration/block.h   | 1 +
>  stubs/Makefile.objs | 1 +
>  stubs/block-migration.c | 6 ++
>  5 files changed, 11 insertions(+), 1 deletion(-)
>  create mode 100644 stubs/block-migration.c
> 
> diff --git a/block.c b/block.c
> index 3308814bba..508a57274d 100644
> --- a/block.c
> +++ b/block.c
> @@ -43,6 +43,7 @@
>  #include "qemu/cutils.h"
>  #include "qemu/id.h"
>  #include "qapi/util.h"
> +#include "migration/block.h"
>  
>  #ifdef CONFIG_BSD
>  #include 
> @@ -3111,6 +3112,7 @@ static void bdrv_close(BlockDriverState *bs)
>  
>  void bdrv_close_all(void)
>  {
> +block_migration_cleanup_bmds();
>  block_job_cancel_sync_all();
>  nbd_export_close_all();

This is before bdrv_drain_all(). Can't we still have a block migration
request in flight, whose callback will then dereference a stale pointer?

Kevin