[Qemu-block] [PATCH for-2.12] hw/block/pflash_cfi: fix off-by-one error
ASAN reported:
hw/block/pflash_cfi02.c:245:33: runtime error: index 82 out of bounds for
type 'uint8_t [82]'
Since the 'cfi_len' member is not used, remove it to keep the code safer.
Reported-by: AddressSanitizer
Signed-off-by: Philippe Mathieu-Daudé
---
hw/block/pflash_cfi01.c | 10 --
hw/block/pflash_cfi02.c | 9 -
2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 1113ab1ccf..2e8284001d 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -90,7 +90,6 @@ struct pflash_t {
uint16_t ident1;
uint16_t ident2;
uint16_t ident3;
-uint8_t cfi_len;
uint8_t cfi_table[0x52];
uint64_t counter;
unsigned int writeblock_size;
@@ -153,7 +152,7 @@ static uint32_t pflash_cfi_query(pflash_t *pfl, hwaddr
offset)
boff = offset >> (ctz32(pfl->bank_width) +
ctz32(pfl->max_device_width) - ctz32(pfl->device_width));
-if (boff > pfl->cfi_len) {
+if (boff >= sizeof(pfl->cfi_table)) {
return 0;
}
/* Now we will construct the CFI response generated by a single
@@ -385,10 +384,10 @@ static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
boff = boff >> 2;
}
-if (boff > pfl->cfi_len) {
-ret = 0;
-} else {
+if (boff < sizeof(pfl->cfi_table)) {
ret = pfl->cfi_table[boff];
+} else {
+ret = 0;
}
} else {
/* If we have a read larger than the bank_width, combine multiple
@@ -791,7 +790,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error
**errp)
pfl->cmd = 0;
pfl->status = 0;
/* Hardcoded CFI table */
-pfl->cfi_len = 0x52;
/* Standard "QRY" string */
pfl->cfi_table[0x10] = 'Q';
pfl->cfi_table[0x11] = 'R';
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index c81ddd3a99..75d1ae1026 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -83,7 +83,6 @@ struct pflash_t {
uint16_t ident3;
uint16_t unlock_addr0;
uint16_t unlock_addr1;
-uint8_t cfi_len;
uint8_t cfi_table[0x52];
QEMUTimer *timer;
/* The device replicates the flash memory across its memory space. Emulate
@@ -235,10 +234,11 @@ static uint32_t pflash_read (pflash_t *pfl, hwaddr offset,
break;
case 0x98:
/* CFI query mode */
-if (boff > pfl->cfi_len)
-ret = 0;
-else
+if (boff < sizeof(pfl->cfi_table)) {
ret = pfl->cfi_table[boff];
+} else {
+ret = 0;
+}
break;
}
@@ -663,7 +663,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error
**errp)
pfl->cmd = 0;
pfl->status = 0;
/* Hardcoded CFI table (mostly from SG29 Spansion flash) */
-pfl->cfi_len = 0x52;
/* Standard "QRY" string */
pfl->cfi_table[0x10] = 'Q';
pfl->cfi_table[0x11] = 'R';
--
2.16.3
Re: [Qemu-block] [PULL for-2.12 0/1] Block patches
On 4 April 2018 at 17:10, Jeff Cody wrote: > The following changes since commit fd69ad866b62ca8ed4337ffee83b6d82a4e99282: > > Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging > (2018-04-04 14:00:07 +0100) > > are available in the git repository at: > > git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request > > for you to fetch changes up to bfb15b4becf2c9cf484bc3bf893a37c8f3c15cb3: > > block/rbd: remove processed options from qdict (2018-04-04 12:05:13 -0400) > > > Regression fix for 2.12 > > > Jeff Cody (1): > block/rbd: remove processed options from qdict > > block/rbd.c | 7 +++ > 1 file changed, 7 insertions(+) Applied, thanks. -- PMM
Re: [Qemu-block] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix
On 04/04/2018 01:37 PM, Kevin Wolf wrote: > Am 04.04.2018 um 20:16 hat Eric Blake geschrieben: >> On 04/04/2018 10:54 AM, Kevin Wolf wrote: >> > Should we also add a deprecation warning for 'socket' and update the > deprecation documentation, so we can start the clock ticking on getting > rid of maintaining the back-compat glue forever? > We only strictly need it in introspection at the time when we remove the > old option. But maybe it would be nicer to wait with the deprecation > error_report() until libvirt has a chance to avoid the warning. Makes sense - so no deprecation warning for 2.12, and the timeline for performing a deprecation is shifted by a couple of releases after the first time we introduce proper introspection support. Which makes this patch fine as-is. -- 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 for-2.12] gluster: Fix blockdev-add with server.N.type=unix
Am 04.04.2018 um 20:16 hat Eric Blake geschrieben: > On 04/04/2018 10:54 AM, Kevin Wolf wrote: > > >>> Should we also add a deprecation warning for 'socket' and update the > >>> deprecation documentation, so we can start the clock ticking on getting > >>> rid of maintaining the back-compat glue forever? > >> > >> Well, that won't be as easy. Since there is at least one qemu release > >> which declared this in the QAPI schema but did not support using it it's > >> hard for libvirt to detect that this was fixed, and thus we can't infer > >> a capability which would be used to switch to the new-syntax only. > > > > Markus wanted to add some way to declare capabilities for a command in > > the schema that could be queried with schema introspection and would > > cover such cases. I think we even discussed this with you at KVM Forum? > > > > Eric, do you happen to know if there were any patches on the list > > related to this? > > I don't recall seeing any patches from Markus before his leave, and > doubt anyone else has tackled anything along these lines. It's too late > for 2.12, at any rate, so even if we had such a feature implemented in > time for 2.13, it won't help introspecting the 2.12 release. We only strictly need it in introspection at the time when we remove the old option. But maybe it would be nicer to wait with the deprecation error_report() until libvirt has a chance to avoid the warning. Kevin signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix
On 04/04/2018 10:54 AM, Kevin Wolf wrote: >>> Should we also add a deprecation warning for 'socket' and update the >>> deprecation documentation, so we can start the clock ticking on getting >>> rid of maintaining the back-compat glue forever? >> >> Well, that won't be as easy. Since there is at least one qemu release >> which declared this in the QAPI schema but did not support using it it's >> hard for libvirt to detect that this was fixed, and thus we can't infer >> a capability which would be used to switch to the new-syntax only. > > Markus wanted to add some way to declare capabilities for a command in > the schema that could be queried with schema introspection and would > cover such cases. I think we even discussed this with you at KVM Forum? > > Eric, do you happen to know if there were any patches on the list > related to this? I don't recall seeing any patches from Markus before his leave, and doubt anyone else has tackled anything along these lines. It's too late for 2.12, at any rate, so even if we had such a feature implemented in time for 2.13, it won't help introspecting the 2.12 release. -- 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 for-2.12 v2] qemu-iotests: update 185 output
On 2018-04-04 17:01, Stefan Hajnoczi wrote:
> Commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
> vm_shutdown()") added a bdrv_drain_all() call. As a side-effect of the
> drain operation the block job iterates one more time than before. The
> 185 output no longer matches and the test is failing now.
>
> It may be possible to avoid the superfluous block job iteration, but
> that type of patch is not suitable late in the QEMU 2.12 release cycle.
>
> This patch simply updates the 185 output file. The new behavior is
> correct, just not optimal, so make the test pass again.
>
> Fixes: 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
> vm_shutdown()")
> Cc: Kevin Wolf
> Cc: QingFeng Hao
> Signed-off-by: Stefan Hajnoczi
> ---
> tests/qemu-iotests/185 | 10 ++
> tests/qemu-iotests/185.out | 12 +++-
> 2 files changed, 13 insertions(+), 9 deletions(-)
On tmpfs, this isn't enough to let the test pass. There, the active
commit job finishes before the quit is sent, resulting in this diff:
--- tests/qemu-iotests/185.out 2018-04-04 18:10:02.015935435 +0200
+++ tests/qemu-iotests/185.out.bad 2018-04-04 18:10:21.045473817 +0200
@@ -26,9 +26,9 @@
{"return": {}}
{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
"event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304,
"offset": 4194304, "speed": 65536, "type": "commit"}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
"event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
"event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304,
"offset": 4194304, "speed": 65536, "type": "commit"}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP},
"event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len":
4194304, "offset": 4194304, "speed": 65536, "type": "commit"}}
=== Start mirror job and exit qemu ===
This seems to be independent of whether there is actually data on
TEST_IMG (the commit source), so something doesn't seem quite right with
the block job throttling here...?
Max
[Qemu-block] [PULL for-2.12 0/1] Block patches
The following changes since commit fd69ad866b62ca8ed4337ffee83b6d82a4e99282: Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2018-04-04 14:00:07 +0100) are available in the git repository at: git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request for you to fetch changes up to bfb15b4becf2c9cf484bc3bf893a37c8f3c15cb3: block/rbd: remove processed options from qdict (2018-04-04 12:05:13 -0400) Regression fix for 2.12 Jeff Cody (1): block/rbd: remove processed options from qdict block/rbd.c | 7 +++ 1 file changed, 7 insertions(+) -- 2.13.6
[Qemu-block] [PULL for-2.12 1/1] block/rbd: remove processed options from qdict
Commit 4bfb274 added some QAPIfication of option parsing in
qemu_rbd_open(). We need to remove all the options we processed,
otherwise in bdrv_open_inherit() we will think the remaining options are
invalid.
(This needs to go in 2.12 to avoid a regression that prevents rbd
from being opened.)
Suggested-by: Kevin Wolf
Signed-off-by: Jeff Cody
Reviewed-by: Kevin Wolf
---
block/rbd.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/block/rbd.c b/block/rbd.c
index 5b64849dc6..c9359d0ad8 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -623,6 +623,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
BlockdevOptionsRbd *opts = NULL;
Visitor *v;
QObject *crumpled = NULL;
+const QDictEntry *e;
Error *local_err = NULL;
const char *filename;
char *keypairs, *secretid;
@@ -671,6 +672,12 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
goto out;
}
+/* Remove the processed options from the QDict (the visitor processes
+ * _all_ options in the QDict) */
+while ((e = qdict_first(options))) {
+qdict_del(options, e->key);
+}
+
r = qemu_rbd_connect(&s->cluster, &s->io_ctx, opts,
!(flags & BDRV_O_NOCACHE), keypairs, secretid, errp);
if (r < 0) {
--
2.13.6
Re: [Qemu-block] [PATCH for-2.12] block/rbd: remove processed options from qdict
Am 04.04.2018 um 17:53 hat Jeff Cody geschrieben: > Commit 4bfb274 added some QAPIfication of option parsing in > qemu_rbd_open(). We need to remove all the options we processed, > otherwise in bdrv_open_inherit() we will think the remaining options are > invalid. > > (This needs to go in 2.12 to avoid a regression that prevents rbd > from being opened.) > > Suggested-by: Kevin Wolf > Signed-off-by: Jeff Cody Reviewed-by: Kevin Wolf
Re: [Qemu-block] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix
Am 04.04.2018 um 17:41 hat Peter Krempa geschrieben: > On Tue, Apr 03, 2018 at 08:09:49 -0500, Eric Blake wrote: > > On 04/03/2018 06:08 AM, Kevin Wolf wrote: > > > The legacy command line interface gets the socket path from an option > > > called 'socket'. QAPI in contract uses SocketAddress, where the > > > corresponding option is called 'path'. > > > > > > Fix the gluster block driver to accept both 'socket' and 'path', with > > > 'path' being the preferred syntax. > > > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1545155 > > > > > > Cc: qemu-sta...@nongnu.org > > > Signed-off-by: Kevin Wolf > > > --- > > > block/gluster.c | 21 + > > > 1 file changed, 17 insertions(+), 4 deletions(-) > > Thanks for fixing this. I'm using the new syntax in the -blockdev code > in libvirt and since I'm qapi- schema-checking the results, it would > not be possible to use the legacy approach. -blockdev is validated against the schema on the QEMU side, too. The legacy option can only be used with -drive. > > Reviewed-by: Eric Blake > > > > Should we also add a deprecation warning for 'socket' and update the > > deprecation documentation, so we can start the clock ticking on getting > > rid of maintaining the back-compat glue forever? > > Well, that won't be as easy. Since there is at least one qemu release > which declared this in the QAPI schema but did not support using it it's > hard for libvirt to detect that this was fixed, and thus we can't infer > a capability which would be used to switch to the new-syntax only. Markus wanted to add some way to declare capabilities for a command in the schema that could be queried with schema introspection and would cover such cases. I think we even discussed this with you at KVM Forum? Eric, do you happen to know if there were any patches on the list related to this? Kevin signature.asc Description: PGP signature
[Qemu-block] [PATCH for-2.12] block/rbd: remove processed options from qdict
Commit 4bfb274 added some QAPIfication of option parsing in
qemu_rbd_open(). We need to remove all the options we processed,
otherwise in bdrv_open_inherit() we will think the remaining options are
invalid.
(This needs to go in 2.12 to avoid a regression that prevents rbd
from being opened.)
Suggested-by: Kevin Wolf
Signed-off-by: Jeff Cody
---
block/rbd.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/block/rbd.c b/block/rbd.c
index 5b64849dc6..c9359d0ad8 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -623,6 +623,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
BlockdevOptionsRbd *opts = NULL;
Visitor *v;
QObject *crumpled = NULL;
+const QDictEntry *e;
Error *local_err = NULL;
const char *filename;
char *keypairs, *secretid;
@@ -671,6 +672,12 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict
*options, int flags,
goto out;
}
+/* Remove the processed options from the QDict (the visitor processes
+ * _all_ options in the QDict) */
+while ((e = qdict_first(options))) {
+qdict_del(options, e->key);
+}
+
r = qemu_rbd_connect(&s->cluster, &s->io_ctx, opts,
!(flags & BDRV_O_NOCACHE), keypairs, secretid, errp);
if (r < 0) {
--
2.13.6
Re: [Qemu-block] [PATCH for-2.12] gluster: Fix blockdev-add with server.N.type=unix
On Tue, Apr 03, 2018 at 08:09:49 -0500, Eric Blake wrote: > On 04/03/2018 06:08 AM, Kevin Wolf wrote: > > The legacy command line interface gets the socket path from an option > > called 'socket'. QAPI in contract uses SocketAddress, where the > > corresponding option is called 'path'. > > > > Fix the gluster block driver to accept both 'socket' and 'path', with > > 'path' being the preferred syntax. > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1545155 > > > > Cc: qemu-sta...@nongnu.org > > Signed-off-by: Kevin Wolf > > --- > > block/gluster.c | 21 + > > 1 file changed, 17 insertions(+), 4 deletions(-) Thanks for fixing this. I'm using the new syntax in the -blockdev code in libvirt and since I'm qapi- schema-checking the results, it would not be possible to use the legacy approach. > > Reviewed-by: Eric Blake > > Should we also add a deprecation warning for 'socket' and update the > deprecation documentation, so we can start the clock ticking on getting > rid of maintaining the back-compat glue forever? Well, that won't be as easy. Since there is at least one qemu release which declared this in the QAPI schema but did not support using it it's hard for libvirt to detect that this was fixed, and thus we can't infer a capability which would be used to switch to the new-syntax only. signature.asc Description: PGP signature
Re: [Qemu-block] [Qemu-devel] [PATCH for-2.12 v2] qemu-iotests: update 185 output
On 04/04/2018 10:01 AM, Stefan Hajnoczi wrote:
> Commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
> vm_shutdown()") added a bdrv_drain_all() call. As a side-effect of the
> drain operation the block job iterates one more time than before. The
> 185 output no longer matches and the test is failing now.
>
> It may be possible to avoid the superfluous block job iteration, but
> that type of patch is not suitable late in the QEMU 2.12 release cycle.
>
> This patch simply updates the 185 output file. The new behavior is
> correct, just not optimal, so make the test pass again.
>
> Fixes: 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
> vm_shutdown()")
> Cc: Kevin Wolf
> Cc: QingFeng Hao
> Signed-off-by: Stefan Hajnoczi
> ---
> tests/qemu-iotests/185 | 10 ++
> tests/qemu-iotests/185.out | 12 +++-
> 2 files changed, 13 insertions(+), 9 deletions(-)
> @@ -102,11 +101,14 @@ echo
> # command to be received (after receiving the command, the rest runs
> # synchronously, so jobs can arbitrarily continue or complete).
> #
> +# Jobs present while QEMU is terminating iterate once more due to
> +# bdrv_drain_all().
Do we want to add something like: "FIXME: removing the bdrv_drain_all()
may be desirable in a future patch; and would warrant a change to the
expected output."? Either way,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [RFC PATCH 0/8] qemu-img convert with copy offloading
On 04/04/2018 15:49, Fam Zheng wrote:
>> I posted concerns about the bdrv_co_map_range() interface. It would be
>> safer to only have a copy_range() interface without exposing how data is
>> mapped outside the driver where race conditions can occur and the format
>> driver no longer has full control over file layout.
> It's a good point, but I couldn't think of a way to implement copy_range
> between
> two format drivers: both of them need to recurse down to their bs->file and
> what
> we eventually want is a copy_file_range() on two fds (or an iscsi equivalent):
>
> src[qcow2] -> dst[raw]
> | |
> v v
> src[file]-> dst[file]
> | |
> v v
>fd1 -> fd2
> copy_file_range
>
> Maybe we should add BlockDriver.bdrv_co_map_range_{prepare,commit,abort} and
> call them from bdrv_co_copy_range(). This way the code path works pretty much
> the same way to .bdrv_co_preadv/pwritev.
Or you could have bdrv_co_copy_range verify the mapping and return
-EAGAIN if it is not valid anymore? (Then bdrv_co_map_range should fill
in a BlockDriverMapping struct, or something like that).
Paolo
Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-iotests: update 185 output
On Wed, Apr 04, 2018 at 02:59:14PM +0800, QingFeng Hao wrote: > 在 2018/4/3 22:03, Stefan Hajnoczi 写道: > Need we add the comment in 185.out that this change is to make the test pass > for the superfluous block job iteration? Thanks, I have sent v2. Stefan signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH] qemu-iotests: update 185 output
On Tue, Apr 03, 2018 at 04:13:36PM +0200, Kevin Wolf wrote:
> Am 03.04.2018 um 16:03 hat Stefan Hajnoczi geschrieben:
> > Commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
> > vm_shutdown()") added a bdrv_drain_all() call. As a side-effect of the
> > drain operation the block job iterates one more time than before. The
> > 185 output no longer matches and the test is failing now.
> >
> > It may be possible to avoid the superfluous block job iteration, but
> > that type of patch is not suitable late in the QEMU 2.12 release cycle.
> >
> > This patch simply updates the 185 output file. The new behavior is
> > correct, just not optimal, so make the test pass again.
> >
> > Fixes: 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
> > vm_shutdown()")
> > Cc: Kevin Wolf
> > Cc: QingFeng Hao
> > Signed-off-by: Stefan Hajnoczi
>
> Do you want to update the comment in the 185 test case code that
> explains which values we expect?
Thanks, I have sent v2.
Stefan
signature.asc
Description: PGP signature
[Qemu-block] [PATCH for-2.12 v2] qemu-iotests: update 185 output
Commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
vm_shutdown()") added a bdrv_drain_all() call. As a side-effect of the
drain operation the block job iterates one more time than before. The
185 output no longer matches and the test is failing now.
It may be possible to avoid the superfluous block job iteration, but
that type of patch is not suitable late in the QEMU 2.12 release cycle.
This patch simply updates the 185 output file. The new behavior is
correct, just not optimal, so make the test pass again.
Fixes: 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce vm_shutdown()")
Cc: Kevin Wolf
Cc: QingFeng Hao
Signed-off-by: Stefan Hajnoczi
---
tests/qemu-iotests/185 | 10 ++
tests/qemu-iotests/185.out | 12 +++-
2 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/tests/qemu-iotests/185 b/tests/qemu-iotests/185
index f5b47e4c1a..298d88d04e 100755
--- a/tests/qemu-iotests/185
+++ b/tests/qemu-iotests/185
@@ -92,9 +92,8 @@ echo === Start commit job and exit qemu ===
echo
# Note that the reference output intentionally includes the 'offset' field in
-# BLOCK_JOB_CANCELLED events for all of the following block jobs. They are
-# predictable and any change in the offsets would hint at a bug in the job
-# throttling code.
+# BLOCK_JOB_* events for all of the following block jobs. They are predictable
+# and any change in the offsets would hint at a bug in the job throttling code.
#
# In order to achieve these predictable offsets, all of the following tests
# use speed=65536. Each job will perform exactly one iteration before it has
@@ -102,11 +101,14 @@ echo
# command to be received (after receiving the command, the rest runs
# synchronously, so jobs can arbitrarily continue or complete).
#
+# Jobs present while QEMU is terminating iterate once more due to
+# bdrv_drain_all().
+#
# The buffer size for commit and streaming is 512k (waiting for 8 seconds after
# the first request), for active commit and mirror it's large enough to cover
# the full 4M, and for backup it's the qcow2 cluster size, which we know is
# 64k. As all of these are at least as large as the speed, we are sure that the
-# offset doesn't advance after the first iteration before qemu exits.
+# offset advances exactly twice before qemu exits.
_send_qemu_cmd $h \
"{ 'execute': 'block-commit',
diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out
index 57eaf8d699..2c4b04de73 100644
--- a/tests/qemu-iotests/185.out
+++ b/tests/qemu-iotests/185.out
@@ -20,7 +20,7 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864
backing_file=TEST_DIR/t.q
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset":
524288, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset":
1048576, "speed": 65536, "type": "commit"}}
=== Start active commit job and exit qemu ===
@@ -28,16 +28,18 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864
backing_file=TEST_DIR/t.q
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "commit"}}
=== Start mirror job and exit qemu ===
{"return": {}}
Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 cluster_size=65536
lazy_refcounts=off refcount_bits=16
{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "mirror"}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "mirr
Re: [Qemu-block] [PULL 0/9] Block layer patches for 2.12.0-rc2
On 3 April 2018 at 17:33, Kevin Wolf wrote: > The following changes since commit f184de7553272223d6af731d7d623a7cebf710b5: > > Merge remote-tracking branch > 'remotes/riscv/tags/riscv-qemu-2.12-critical-fixes' into staging (2018-03-31 > 09:42:33 +0100) > > are available in the git repository at: > > git://repo.or.cz/qemu/kevin.git tags/for-upstream > > for you to fetch changes up to 9c1386d3ff76c5983529884e3d8420df958c5a29: > > Merge remote-tracking branch 'mreitz/tags/pull-block-2018-04-03' into > queue-block (2018-04-03 17:48:45 +0200) > > > Block layer patches > > > Alberto Garcia (3): > iotests: Update 051 and 186 after commit 1454509726719e0933c > iotests: Update 186 after commit ac64273c66ab136c44043259162 > iotests: Test abnormally large size in compressed cluster descriptor > > Jeff Cody (1): > block: handle invalid lseek returns gracefully > > Kevin Wolf (2): > gluster: Fix blockdev-add with server.N.type=unix > Merge remote-tracking branch 'mreitz/tags/pull-block-2018-04-03' into > queue-block > > Lukáš Doktor (1): > qemu-iotests: Use ppc64 qemu_arch on ppc64le host > > Max Reitz (2): > block/file-posix: Fix fully preallocated truncate > iotests: Test preallocated truncate of 2G image > > Vladimir Sementsov-Ogievskiy (1): > iotests: fix 208 for luks format Applied, thanks. -- PMM
Re: [Qemu-block] [RFC PATCH 0/8] qemu-img convert with copy offloading
On Wed, 04/04 14:23, Stefan Hajnoczi wrote:
> On Thu, Mar 29, 2018 at 07:09:06PM +0800, Fam Zheng wrote:
> > [Posting a preview RFC for the general idea discussion and internal API
> > review.
> > Libiscsi support is being worked on in the meantime.]
> >
> > This series introduces block layer API for copy offloading and makes use of
> > it
> > in qemu-img convert.
> >
> > For now we implemented the operation in local file protocol with
> > copy_file_range(2). Besides that it's possible to add similar to iscsi, nfs
> > and potentially more.
> >
> > As far as its usage goes, in addition to qemu-img convert, we can emulate
> > offloading in scsi-disk (EXTENDED COPY), and do similar to drive-mirror.
> >
> > The new bdrv_co_map_range can also be an alternative way to implement format
> > drivers in the future, once we make block/io.c use it in preadv/pwritev
> > paths.
>
> I posted concerns about the bdrv_co_map_range() interface. It would be
> safer to only have a copy_range() interface without exposing how data is
> mapped outside the driver where race conditions can occur and the format
> driver no longer has full control over file layout.
It's a good point, but I couldn't think of a way to implement copy_range between
two format drivers: both of them need to recurse down to their bs->file and what
we eventually want is a copy_file_range() on two fds (or an iscsi equivalent):
src[qcow2] -> dst[raw]
| |
v v
src[file]-> dst[file]
| |
v v
fd1 -> fd2
copy_file_range
Maybe we should add BlockDriver.bdrv_co_map_range_{prepare,commit,abort} and
call them from bdrv_co_copy_range(). This way the code path works pretty much
the same way to .bdrv_co_preadv/pwritev.
Fam
Re: [Qemu-block] [RFC PATCH 0/8] qemu-img convert with copy offloading
On Thu, Mar 29, 2018 at 07:09:06PM +0800, Fam Zheng wrote: > [Posting a preview RFC for the general idea discussion and internal API > review. > Libiscsi support is being worked on in the meantime.] > > This series introduces block layer API for copy offloading and makes use of it > in qemu-img convert. > > For now we implemented the operation in local file protocol with > copy_file_range(2). Besides that it's possible to add similar to iscsi, nfs > and potentially more. > > As far as its usage goes, in addition to qemu-img convert, we can emulate > offloading in scsi-disk (EXTENDED COPY), and do similar to drive-mirror. > > The new bdrv_co_map_range can also be an alternative way to implement format > drivers in the future, once we make block/io.c use it in preadv/pwritev paths. I posted concerns about the bdrv_co_map_range() interface. It would be safer to only have a copy_range() interface without exposing how data is mapped outside the driver where race conditions can occur and the format driver no longer has full control over file layout. Stefan signature.asc Description: PGP signature
Re: [Qemu-block] [RFC PATCH 4/8] file-posix: Implement bdrv_co_copy_range
On Thu, Mar 29, 2018 at 07:09:10PM +0800, Fam Zheng wrote:
> +static ssize_t handle_aiocb_copy_range(RawPosixAIOData *aiocb)
> +{
> +#ifndef HAS_COPY_FILE_RANGE
> +return -ENOTSUP;
> +#else
> +uint64_t bytes = aiocb->aio_nbytes;
> +off_t in_off = aiocb->aio_offset;
> +off_t out_off = aiocb->offset2;
> +
> +while (bytes) {
> +ssize_t ret = copy_file_range(aiocb->aio_fildes, &in_off,
> + aiocb->fd2, &out_off,
> + bytes, 0);
> +if (ret < 0) {
> +return -errno;
> +}
EINTR should retry.
signature.asc
Description: PGP signature
Re: [Qemu-block] [RFC PATCH 1/8] block: Introduce bdrv_co_map_range API
On Thu, Mar 29, 2018 at 07:09:07PM +0800, Fam Zheng wrote: > diff --git a/block/io.c b/block/io.c > index bd9a19a9c4..1b4cfcacb1 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -2826,3 +2826,47 @@ void bdrv_unregister_buf(BlockDriverState *bs, void > *host) > bdrv_unregister_buf(child->bs, host); > } > } > + > +int coroutine_fn bdrv_co_map_range(BdrvChild *child, int64_t offset, > + int64_t bytes, int64_t *pnum, int64_t > *map, s/map/file_offset/ is clearer, since this function has self-expanatory 'offset' and 'file' arguments. This is the "offset within the file" -> file_offset. > +/** > + * Get or create offset mappings for a virtual range. > + * > + * @offset: the offset of the beginning of the range. Adding the units and avoiding reusing "offset" in the description: "byte position of the beginning of the range" > + * @bytes: the maximum bytes of the range to map. > + * @pnum: at return, this will point to the number of bytes in the > returned > + * mapping status > + * @map: at return, this will point to the offset which the range maps to > + * @file: at return, this will point to the file where the data is > mapped. > + * If it is set to bs, it means the mapping is terminal and the result > can > + * be used for read/write and copy_range; if it is NULL, it means the > range > + * doesn't map to a file, or no I/O to any file is necessary; otherwise, > it > + * means the query should be passed down to an underlying > format/protocol. What does "the query should be passed down to an underlying format/protocol" mean? > + * @flags: flags for the request. BDRV_REQ_ALLOCATE means the range > + * should be allocated if necessary. > + */ What are return values? Please include a statement describing how long the mapping is considered stable. For example, a qcow2 copy-on-write could "move" the data somewhere else. Therefore it's not safe to store the value of "map" and use it again later on. Additionally, please explain what the caller is allowed to do with the returned information. Using qcow2 copy-on-write as an example again, it is not okay to write to "map" since that bypasses qcow2's COW mechanism. signature.asc Description: PGP signature
Re: [Qemu-block] [PULL 0/3] Block patches for 2.12-rc2
On 3 April 2018 at 17:13, Jeff Cody wrote: > The following changes since commit f184de7553272223d6af731d7d623a7cebf710b5: > > Merge remote-tracking branch > 'remotes/riscv/tags/riscv-qemu-2.12-critical-fixes' into staging (2018-03-31 > 09:42:33 +0100) > > are available in the git repository at: > > git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request > > for you to fetch changes up to 9dae635afa98f83688806861cefe77ff1b4d76a8: > > gluster: Fix blockdev-add with server.N.type=unix (2018-04-03 09:57:14 > -0400) > > > Blockjob and Gluster patches > > > Kevin Wolf (1): > gluster: Fix blockdev-add with server.N.type=unix > > Marc-André Lureau (2): > blockjob: leak fix, remove from txn when failing early > blockjob: use qapi enum helpers > > block/gluster.c | 21 + > blockjob.c | 28 +--- > 2 files changed, 34 insertions(+), 15 deletions(-) Applied, thanks. -- PMM
Re: [Qemu-block] [RFC PATCH 1/8] block: Introduce bdrv_co_map_range API
On Thu, Mar 29, 2018 at 07:09:07PM +0800, Fam Zheng wrote: > +int coroutine_fn bdrv_co_map_range(BdrvChild *child, int64_t offset, > + int64_t bytes, int64_t *pnum, int64_t > *map, I just noticed that .bdrv_co_block_status() also calls the argument "map". In that case, let's keep it "map" unless you want to rename "map" to "file_offset" in .bdrv_co_block_status() too. Stefan signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH 0/3] iotests: blacklist bochs and cloop for 205 and 208
Am 30.03.2018 um 17:16 hat Vladimir Sementsov-Ogievskiy geschrieben: > Blacklist these formats, as they don't support image creation, as they > say: > > ./qemu-img create -f bochs x 1m > qemu-img: x: Format driver 'bochs' does not support image creation > > > ./qemu-img create -f cloop x 1m > qemu-img: x: Format driver 'cloop' does not support image creation Thanks, applied to the block branch. (But if you feel like implementing the IMGFMT_GENERIC thing on top, I wouldn't mind. :-)) Kevin
Re: [Qemu-block] [Qemu-devel] [PULL 1/9] gluster: Fix blockdev-add with server.N.type=unix
Am 03.04.2018 um 23:03 hat Jeff Cody geschrieben: > On Tue, Apr 03, 2018 at 06:33:52PM +0200, Kevin Wolf wrote: > > The legacy command line interface gets the socket path from an option > > called 'socket'. QAPI in contract uses SocketAddress, where the > > corresponding option is called 'path'. > > > > Fix the gluster block driver to accept both 'socket' and 'path', with > > 'path' being the preferred syntax. > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1545155 > > > > Cc: qemu-sta...@nongnu.org > > Signed-off-by: Kevin Wolf > > Reviewed-by: Eric Blake > > Oops, you and I had a pull request collision on this patch (it is in both of > our PRs); Sorry, my fault, I didn't know you were pulling it in through your > tree. Or my fault for applying a patch to something that is maintained by you. Anyway, I think git should sort it out without a problem. Kevin
Re: [Qemu-block] [PATCH 1/3] iotests.py: improve verify_image_format helper
Am 04.04.2018 um 10:48 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 03.04.2018 16:54, Kevin Wolf wrote:
> > Am 30.03.2018 um 17:16 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > Add an assert (we don't want set both arguments) and remove
> > > duplication.
> > >
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy
> > > ---
> > > tests/qemu-iotests/iotests.py | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> > > index b5d7945..83c454d 100644
> > > --- a/tests/qemu-iotests/iotests.py
> > > +++ b/tests/qemu-iotests/iotests.py
> > > @@ -532,9 +532,9 @@ def notrun(reason):
> > > sys.exit(0)
> > > def verify_image_format(supported_fmts=[], unsupported_fmts=[]):
> > > -if supported_fmts and (imgfmt not in supported_fmts):
> > > -notrun('not suitable for this image format: %s' % imgfmt)
> > > -if unsupported_fmts and (imgfmt in unsupported_fmts):
> > > +assert not (supported_fmts and unsupported_fmts)
> > > +not_sup = supported_fmts and (imgfmt not in supported_fmts)
> > > +if not_sup or (imgfmt in unsupported_fmts):
> > > notrun('not suitable for this image format: %s' % imgfmt)
> > Before the change, we accepted None for both parameters. Now None is
> > still accepted for supported_fmts, but not for unsupported_fmts any
> > more.
> >
> > I don't think we actually make use of None for either, so I don't really
> > mind whether we allow it or not, but we should be consistent between
> > both parameters.
> >
> > Kevin
>
> I think, we should not care about it. The function takes lists. So, you can
> 1. pass a parameter, which must be list
> 2. do not pass it, it will become [] by default.
>
> So if someone pass None directly, its a bug. Like if someone will pass int
> or float..
Yeah, brain fart. Somehow I thought you could just check 'imgfmt not in
supported_fmts' without checking 'supported_fmt' first, but obviously
that would make the default that nothing is accepted. Your version is
fine.
Kevin
Re: [Qemu-block] [PATCH 1/3] iotests.py: improve verify_image_format helper
03.04.2018 16:54, Kevin Wolf wrote:
Am 30.03.2018 um 17:16 hat Vladimir Sementsov-Ogievskiy geschrieben:
Add an assert (we don't want set both arguments) and remove
duplication.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/iotests.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index b5d7945..83c454d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -532,9 +532,9 @@ def notrun(reason):
sys.exit(0)
def verify_image_format(supported_fmts=[], unsupported_fmts=[]):
-if supported_fmts and (imgfmt not in supported_fmts):
-notrun('not suitable for this image format: %s' % imgfmt)
-if unsupported_fmts and (imgfmt in unsupported_fmts):
+assert not (supported_fmts and unsupported_fmts)
+not_sup = supported_fmts and (imgfmt not in supported_fmts)
+if not_sup or (imgfmt in unsupported_fmts):
notrun('not suitable for this image format: %s' % imgfmt)
Before the change, we accepted None for both parameters. Now None is
still accepted for supported_fmts, but not for unsupported_fmts any
more.
I don't think we actually make use of None for either, so I don't really
mind whether we allow it or not, but we should be consistent between
both parameters.
Kevin
I think, we should not care about it. The function takes lists. So, you can
1. pass a parameter, which must be list
2. do not pass it, it will become [] by default.
So if someone pass None directly, its a bug. Like if someone will pass
int or float..
--
Best regards,
Vladimir
Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-iotests: update 185 output
在 2018/4/3 22:03, Stefan Hajnoczi 写道:
> Commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
> vm_shutdown()") added a bdrv_drain_all() call. As a side-effect of the
> drain operation the block job iterates one more time than before. The
> 185 output no longer matches and the test is failing now.
>
> It may be possible to avoid the superfluous block job iteration, but
> that type of patch is not suitable late in the QEMU 2.12 release cycle.
>
> This patch simply updates the 185 output file. The new behavior is
> correct, just not optimal, so make the test pass again.
>
> Fixes: 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
> vm_shutdown()")
> Cc: Kevin Wolf
> Cc: QingFeng Hao
> Signed-off-by: Stefan Hajnoczi
> ---
> tests/qemu-iotests/185.out | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out
> index 57eaf8d699..2c4b04de73 100644
> --- a/tests/qemu-iotests/185.out
> +++ b/tests/qemu-iotests/185.out
> @@ -20,7 +20,7 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864
> backing_file=TEST_DIR/t.q
> {"return": {}}
> {"return": {}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "SHUTDOWN", "data": {"guest": false}}
> -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset":
> 524288, "speed": 65536, "type": "commit"}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset":
> 1048576, "speed": 65536, "type": "commit"}}
Need we add the comment in 185.out that this change is to make the test pass
for the superfluous block job iteration?
thanks
>
> === Start active commit job and exit qemu ===
>
> @@ -28,16 +28,18 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864
> backing_file=TEST_DIR/t.q
> {"return": {}}
> {"return": {}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "SHUTDOWN", "data": {"guest": false}}
> -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset":
> 4194304, "speed": 65536, "type": "commit"}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset":
> 4194304, "speed": 65536, "type": "commit"}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset":
> 4194304, "speed": 65536, "type": "commit"}}
[...]
>
--
Regards
QingFeng Hao
