Re: [PATCH resend] block/replication.c: Properly attach children

[Vladimir Sementsov-Ogievskiy]

04.07.2021 13:58, Lukas Straub wrote:

The replication driver needs access to the children block-nodes of
it's child so it can issue bdrv_make_empty to manage the replication.
However, it does this by directly copying the BdrvChilds, which is
wrong.

Fix this by properly attaching the block-nodes with
bdrv_attach_child().

Also, remove a workaround introduced in commit
6ecbc6c52672db5c13805735ca02784879ce8285
"replication: Avoid blk_make_empty() on read-only child".

Signed-off-by: Lukas Straub 
---

Fix CC: email address so the mailing list doesn't reject it.

  block/replication.c | 94 +
  1 file changed, 61 insertions(+), 33 deletions(-)

diff --git a/block/replication.c b/block/replication.c
index 52163f2d1f..426d2b741a 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -166,7 +166,12 @@ static void replication_child_perm(BlockDriverState *bs, 
BdrvChild *c,
 uint64_t perm, uint64_t shared,
 uint64_t *nperm, uint64_t *nshared)
  {
-*nperm = BLK_PERM_CONSISTENT_READ;
+if (c == bs->file) {


You can't rely on bs->file being set at this point. Look at replication_open() 
bs->file is set after bdrv_open_child() call finished. bdrv_open_child() among 
other things will call bdrv_refresh_perms() on parent bs.


+*nperm = BLK_PERM_CONSISTENT_READ;
+} else {
+*nperm = 0;
+}
+
  if ((bs->open_flags & (BDRV_O_INACTIVE | BDRV_O_RDWR)) == BDRV_O_RDWR) {
  *nperm |= BLK_PERM_WRITE;
  }
@@ -340,17 +345,7 @@ static void secondary_do_checkpoint(BDRVReplicationState 
*s, Error **errp)
  return;
  }
  
-BlockBackend *blk = blk_new(qemu_get_current_aio_context(),

-BLK_PERM_WRITE, BLK_PERM_ALL);
-blk_insert_bs(blk, s->hidden_disk->bs, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
-blk_unref(blk);
-return;
-}
-
-ret = blk_make_empty(blk, errp);
-blk_unref(blk);
+ret = bdrv_make_empty(s->hidden_disk, errp);
  if (ret < 0) {
  return;
  }
@@ -365,27 +360,35 @@ static void reopen_backing_file(BlockDriverState *bs, 
bool writable,
  Error **errp)
  {
  BDRVReplicationState *s = bs->opaque;
+BdrvChild *hidden_disk, *secondary_disk;
  BlockReopenQueue *reopen_queue = NULL;
  
+/*

+ * s->hidden_disk and s->secondary_disk may not be set yet, as they will
+ * only be set after the children are writable.
+ */
+hidden_disk = bs->file->bs->backing;
+secondary_disk = hidden_disk->bs->backing;
+
  if (writable) {
-s->orig_hidden_read_only = bdrv_is_read_only(s->hidden_disk->bs);
-s->orig_secondary_read_only = bdrv_is_read_only(s->secondary_disk->bs);
+s->orig_hidden_read_only = bdrv_is_read_only(hidden_disk->bs);
+s->orig_secondary_read_only = bdrv_is_read_only(secondary_disk->bs);
  }
  
-bdrv_subtree_drained_begin(s->hidden_disk->bs);

-bdrv_subtree_drained_begin(s->secondary_disk->bs);
+bdrv_subtree_drained_begin(hidden_disk->bs);
+bdrv_subtree_drained_begin(secondary_disk->bs);
  
  if (s->orig_hidden_read_only) {

  QDict *opts = qdict_new();
  qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
-reopen_queue = bdrv_reopen_queue(reopen_queue, s->hidden_disk->bs,
+reopen_queue = bdrv_reopen_queue(reopen_queue, hidden_disk->bs,
   opts, true);
  }
  
  if (s->orig_secondary_read_only) {

  QDict *opts = qdict_new();
  qdict_put_bool(opts, BDRV_OPT_READ_ONLY, !writable);
-reopen_queue = bdrv_reopen_queue(reopen_queue, s->secondary_disk->bs,
+reopen_queue = bdrv_reopen_queue(reopen_queue, secondary_disk->bs,
   opts, true);
  }
  
@@ -393,8 +396,8 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,

  bdrv_reopen_multiple(reopen_queue, errp);
  }
  
-bdrv_subtree_drained_end(s->hidden_disk->bs);

-bdrv_subtree_drained_end(s->secondary_disk->bs);
+bdrv_subtree_drained_end(hidden_disk->bs);
+bdrv_subtree_drained_end(secondary_disk->bs);
  }
  
  static void backup_job_cleanup(BlockDriverState *bs)

@@ -451,6 +454,7 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
  BlockDriverState *bs = rs->opaque;
  BDRVReplicationState *s;
  BlockDriverState *top_bs;
+BdrvChild *active_disk, *hidden_disk, *secondary_disk;
  int64_t active_length, hidden_length, disk_length;
  AioContext *aio_context;
  Error *local_err = NULL;
@@ -488,32 +492,32 @@ static void replication_start(ReplicationState *rs, 
ReplicationMode mode,
  case REPLICATION_MODE_PRIMARY:
  break;
  case REPLICATION_MODE_SECONDARY:
-s->active_disk = bs->file;
-if (!s-

Re: [PATCH v2 3/3] qapi: deprecate drive-backup

[Vladimir Sementsov-Ogievskiy]

09.06.2021 13:49, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:


08.06.2021 14:12, Markus Armbruster wrote:

Vladimir Sementsov-Ogievskiy  writes:

[...]


TODO: We also need to deprecate drive-backup transaction action..
But union members in QAPI doesn't support 'deprecated' feature. I tried
to dig a bit, but failed :/ Markus, could you please help with it? At
least by advice?


There are two closely related things in play here: the union branch and
the corresponding enum value.

So far, the QAPI schema language doesn't support tacking feature flags
to either.

If an enum value is deprecated, any union branches corresponding to it
must also be deprecated (because their use requires using the deprecated
enum value).

The converse is not true, but I can't see a use for deprecating a union
branch without also deprecating the enum member.

I think we can implement feature flags just for enum members, then
document that 'deprecated' enum value implies corresponding union
branches are also deprecated.

I have unfinished patches implementing feature flags for enum members.

Since TransactionAction is a simple union, the corresponding enum is
implicit.  We can make it explicit by converting to a flat union.
Simple unions need to die anyway.



Does BlockStatsSpecific from qapi/block-core.json a correct example of flat union you 
mean? I can make patch to convert TransactionAction to be similar if that helps 
(discriminator field should be called "type", yes?).


 From docs/devel/qapi-code-gen.txt:

 A simple union can always be re-written as a flat union where the base
 class has a single member named 'type', and where each branch of the
 union has a struct with a single member named 'data'.  That is,

  { 'union': 'Simple', 'data': { 'one': 'str', 'two': 'int' } }

 is identical on the wire to:

  { 'enum': 'Enum', 'data': ['one', 'two'] }
  { 'struct': 'Branch1', 'data': { 'data': 'str' } }
  { 'struct': 'Branch2', 'data': { 'data': 'int' } }
  { 'union': 'Flat', 'base': { 'type': 'Enum' }, 'discriminator': 'type',
'data': { 'one': 'Branch1', 'two': 'Branch2' } }

The generated C isn't identical, but adjusting the code using it should
be straightforward.


Does this make sense?



Yes if it helps)

Did you also look at John's 
https://gitlab.com/jsnow/qemu/-/commits/hack-deprecate-union-branches/ ?


Not yet.


I hope you and John will send patches that you have, I'll help with reviewing 
(keep me in CC), and finally we'll get the feature.


Sounds like a plan.  I need to get my post-vacation e-mail pileup under
control first.



Hi!

Kindly remind in the case you forget :) Or may be I miss some patches?

--
Best regards,
Vladimir

[PATCH] vhost-user: Fix backends without multiqueue support

[Kevin Wolf]

dev->max_queues was never initialised for backends that don't support
VHOST_USER_PROTOCOL_F_MQ, so it would use 0 as the maximum number of
queues to check against and consequently fail for any such backend.

Set it to 1 if the backend doesn't have multiqueue support.

Fixes: c90bd505a3e8210c23d69fecab9ee6f56ec4a161
Signed-off-by: Kevin Wolf 
---
 hw/virtio/vhost-user.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 1ac4a2ebec..29ea2b4fce 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1913,7 +1913,10 @@ static int vhost_user_backend_init(struct vhost_dev 
*dev, void *opaque,
 if (err < 0) {
 return -EPROTO;
 }
+} else {
+dev->max_queues = 1;
 }
+
 if (dev->num_queues && dev->max_queues < dev->num_queues) {
 error_setg(errp, "The maximum number of queues supported by the "
"backend is %" PRIu64, dev->max_queues);
-- 
2.31.1

Re: [PATCH 8/9] tests/acceptance: Remove now unused pow2ceil()

[Willian Rampazzo]

On Wed, Jun 23, 2021 at 3:10 PM Philippe Mathieu-Daudé  wrote:
>
> We don't use pow2ceil() anymore, remove it.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/acceptance/boot_linux_console.py | 12 
>  1 file changed, 12 deletions(-)
>

Reviewed-by: Willian Rampazzo 

Re: [RFC PATCH 6/9] tests/acceptance: Use image_expand() in test_arm_orangepi_bionic_20_08

[Willian Rampazzo]

On Wed, Jun 23, 2021 at 3:09 PM Philippe Mathieu-Daudé  wrote:
>
> U-Boot expects the SD card size to be at least 2GiB, so
> expand the SD card image to this size before using it.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> TODO: U-Boot reference?
> ---
>  tests/acceptance/boot_linux_console.py | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
>

Reviewed-by: Willian Rampazzo 

Re: [PATCH 5/9] tests/acceptance: Use image_expand() in test_arm_orangepi_uboot_netbsd9

[Willian Rampazzo]

On Wed, Jun 23, 2021 at 3:08 PM Philippe Mathieu-Daudé  wrote:
>
> The NetBSD OrangePi image must be at least 2GiB, not less.
> Expand the SD card image to this size before using it.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/acceptance/boot_linux_console.py | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>

Reviewed-by: Willian Rampazzo 

Re: [PATCH 4/9] tests/acceptance: Extract image_expand() helper

[Willian Rampazzo]

On Wed, Jun 23, 2021 at 3:06 PM Philippe Mathieu-Daudé  wrote:
>
> To be able to expand an image to a non-power-of-2 value,
> extract image_expand() from image_pow2ceil_expand().
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/acceptance/boot_linux_console.py | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)
>

Reviewed-by: Willian Rampazzo 

Re: [PATCH 3/9] tests/acceptance: Tag NetBSD tests as 'os:netbsd'

[Willian Rampazzo]

On Wed, Jun 23, 2021 at 3:03 PM Philippe Mathieu-Daudé  wrote:
>
> Avocado allows us to select set of tests using tags.
> When wanting to run all tests using a NetBSD guest OS,
> it is convenient to have them tagged, add the 'os:netbsd'
> tag.
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/acceptance/boot_linux_console.py | 1 +
>  tests/acceptance/ppc_prep_40p.py   | 2 ++
>  2 files changed, 3 insertions(+)
>

Reviewed-by: Willian Rampazzo 

Re: [ovirt-users] Re: Any way to terminate stuck export task

[Nir Soffer]

On Mon, Jul 5, 2021 at 3:36 PM Gianluca Cecchi
 wrote:
>
> On Mon, Jul 5, 2021 at 2:13 PM Nir Soffer  wrote:
>>
>>
>> >
>> > vdsm 14342  3270  0 11:17 ?00:00:03 /usr/bin/qemu-img convert 
>> > -p -t none -T none -f raw 
>> > /rhev/data-center/mnt/blockSD/679c0725-75fb-4af7-bff1-7c447c5d789c/images/530b3e7f-4ce4-4051-9cac-1112f5f9e8b5/d2a89b5e-7d62-4695-96d8-b762ce52b379
>> >  -O raw -o preallocation=falloc 
>> > /rhev/data-center/mnt/172.16.1.137:_nas_EXPORT-DOMAIN/20433d5d-9d82-4079-9252-0e746ce54106/images/530b3e7f-4ce4-4051-9cac-1112f5f9e8b5/d2a89b5e-7d62-4695-96d8-b762ce52b379
>>
>> -o preallocation + NFS 4.0 + very slow NFS is your problem.
>>
>> qemu-img is using posix-fallocate() to preallocate the entire image at
>> the start of the copy. With NFS 4.2
>> this uses fallocate() linux specific syscall that allocates the space
>> very efficiently in no time. With older
>> NFS versions, this becomes a very slow loop, writing one byte for
>> every 4k block.
>>
>> If you see -o preallocation, it means you are using an old vdsm
>> version, we stopped using -o preallocation
>> in 4.4.2, see https://bugzilla.redhat.com/1850267.
>
>
> OK. As I said at the beginning the environment is latest 4.3
> We are going to upgrade to 4.4 and we are making some complimentary backups, 
> for safeness.
>
>>
>> > On the hypervisor the ls commands quite hang, so from another hypervisor I 
>> > see that the disk size seems to remain at 4Gb even if timestamp updates...
>> >
>> > # ll 
>> > /rhev/data-center/mnt/172.16.1.137\:_nas_EXPORT-DOMAIN/20433d5d-9d82-4079-9252-0e746ce54106/images/530b3e7f-4ce4-4051-9cac-1112f5f9e8b5/
>> > total 4260941
>> > -rw-rw. 1 nobody nobody 4363202560 Jul  5 11:23 
>> > d2a89b5e-7d62-4695-96d8-b762ce52b379
>> > -rw-r--r--. 1 nobody nobody261 Jul  5 11:17 
>> > d2a89b5e-7d62-4695-96d8-b762ce52b379.meta
>> >
>> > On host console I see a throughput of 4mbit/s...
>> >
>> > # strace -p 14342
>>
>> This shows only the main thread use -f use -f to show all threads.
>
>
>  # strace -f -p 14342
> strace: Process 14342 attached with 2 threads
> [pid 14342] ppoll([{fd=9, events=POLLIN|POLLERR|POLLHUP}], 1, NULL, NULL, 8 
> 
> [pid 14343] pwrite64(12, "\0", 1, 16474968063) = 1
> [pid 14343] pwrite64(12, "\0", 1, 16474972159) = 1
> [pid 14343] pwrite64(12, "\0", 1, 16474976255) = 1
> [pid 14343] pwrite64(12, "\0", 1, 16474980351) = 1
> [pid 14343] pwrite64(12, "\0", 1, 16474984447) = 1
> [pid 14343] pwrite64(12, "\0", 1, 16474988543) = 1
> [pid 14343] pwrite64(12, "\0", 1, 16474992639) = 1
> [pid 14343] pwrite64(12, "\0", 1, 16474996735) = 1
> [pid 14343] pwrite64(12, "\0", 1, 16475000831) = 1
> [pid 14343] pwrite64(12, "\0", 1, 16475004927) = 1

qemu-img is busy in posix_fallocate(), wiring one byte to every 4k block.

If you add -tt -T (as I suggested), we can see how much time each write takes,
which may explain why this takes so much time.

strace -f -p 14342 --tt -T

> . . . and so on . . .
>
>
>> >
>> > This is a test oVirt env so I can wait and eventually test something...
>> > Let me know your suggestions
>>
>> I would start by changing the NFS storage domain to version 4.2.
>
>
> I'm going to try. RIght now I have set it to the default of autonegotiated...
>
>>
>> 1. kill the hang qemu-img (it will probably cannot be killed, but worth 
>> trying)
>> 2. deactivate the storage domain
>> 3. fix the ownership on the storage domain (should be vdsm:kvm, not
>> nobody:nobody)3.
>
>
> Unfortunately it is an appliance. I have asked the guys that have it in 
> charge if we can set them.
> Thanks for the other concepts explained.
>
> Gianluca

Re: [ovirt-users] Re: Any way to terminate stuck export task

[Nir Soffer]

On Mon, Jul 5, 2021 at 4:06 PM Strahil Nikolov  wrote:
>
> >Disks on the export domain are never used by a running VM so there is
> no reason to
> preallocate them. The system should always use sparse disks when
> copying to export
> domain.
>
> >When importing disks from export domain, the system should reconstruct
> the original disk
> configuration (e.g. raw-preallocated).
>
> Hey Nir,
>
> I think you are wrong. In order to minimize the downtime , many users would 
> use storage migration while the VM is running, then they power off, detach 
> and attach on the new location , power on and live migrate while the VM works.

Live storage migration (move disk while vm is running) is possible only between
data domains, and requires no downtime and no detach/attach are needed.

I'm not sure if it is possible to export a vm to export domain when
the vm is running,
(maybe exporting snapshot works in 4.4). Anway, assuming you can export while
the vm is running, the target disk will never be used by any vm.

When the export is done, you need to import the vm back to the same or other
system, copying the disk to a data domain.

So we have:

original disk: raw-preallocated on data domain 1
exported disk: raw-sparse or qcow2-sparse on export domain
target disk: raw-preallocated on data domain 2

There is no reason to use a preallocated disk for the temporary disk created
in the export domain.

Nir

[PATCH v5 3/3] block/io: Merge discard request alignments

[Akihiko Odaki]

Signed-off-by: Akihiko Odaki 
---
 block/io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/io.c b/block/io.c
index 323854d0633..db19ae2bd9c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -125,6 +125,8 @@ void bdrv_parent_drained_begin_single(BdrvChild *c, bool 
poll)
 
 static void bdrv_merge_limits(BlockLimits *dst, const BlockLimits *src)
 {
+dst->pdiscard_alignment = MAX(dst->pdiscard_alignment,
+  src->pdiscard_alignment);
 dst->opt_transfer = MAX(dst->opt_transfer, src->opt_transfer);
 dst->max_transfer = MIN_NON_ZERO(dst->max_transfer, src->max_transfer);
 dst->opt_mem_alignment = MAX(dst->opt_mem_alignment,
-- 
2.30.1 (Apple Git-130)

[PATCH v5 2/3] block: Add backend_defaults property

[Akihiko Odaki]

backend_defaults property allow users to control if default block
properties should be decided with backend information.

If it is off, any backend information will be discarded, which is
suitable if you plan to perform live migration to a different disk backend.

If it is on, a block device may utilize backend information more
aggressively.

By default, it is auto, which uses backend information for block
sizes and ignores the others, which is consistent with the older
versions.

Signed-off-by: Akihiko Odaki 
---
 hw/block/block.c   | 42 ++
 include/hw/block/block.h   |  3 +++
 tests/qemu-iotests/172.out | 38 ++
 3 files changed, 79 insertions(+), 4 deletions(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index 1e34573da71..d47ebf005ad 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -65,24 +65,58 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
 {
 BlockBackend *blk = conf->blk;
 BlockSizes blocksizes;
-int backend_ret;
+BlockDriverState *bs;
+bool use_blocksizes;
+bool use_bs;
+
+switch (conf->backend_defaults) {
+case ON_OFF_AUTO_AUTO:
+use_blocksizes = !blk_probe_blocksizes(blk, &blocksizes);
+use_bs = false;
+break;
+
+case ON_OFF_AUTO_ON:
+use_blocksizes = !blk_probe_blocksizes(blk, &blocksizes);
+bs = blk_bs(blk);
+use_bs = bs;
+break;
+
+case ON_OFF_AUTO_OFF:
+use_blocksizes = false;
+use_bs = false;
+break;
+
+default:
+abort();
+}
 
-backend_ret = blk_probe_blocksizes(blk, &blocksizes);
 /* fill in detected values if they are not defined via qemu command line */
 if (!conf->physical_block_size) {
-if (!backend_ret) {
+if (use_blocksizes) {
conf->physical_block_size = blocksizes.phys;
 } else {
 conf->physical_block_size = BDRV_SECTOR_SIZE;
 }
 }
 if (!conf->logical_block_size) {
-if (!backend_ret) {
+if (use_blocksizes) {
 conf->logical_block_size = blocksizes.log;
 } else {
 conf->logical_block_size = BDRV_SECTOR_SIZE;
 }
 }
+if (use_bs) {
+if (!conf->opt_io_size) {
+conf->opt_io_size = bs->bl.opt_transfer;
+}
+if (conf->discard_granularity == -1) {
+if (bs->bl.pdiscard_alignment) {
+conf->discard_granularity = bs->bl.pdiscard_alignment;
+} else if (bs->bl.request_alignment != 1) {
+conf->discard_granularity = bs->bl.request_alignment;
+}
+}
+}
 
 if (conf->logical_block_size > conf->physical_block_size) {
 error_setg(errp,
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index c172cbe65f1..5902c0440a5 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -19,6 +19,7 @@
 
 typedef struct BlockConf {
 BlockBackend *blk;
+OnOffAuto backend_defaults;
 uint32_t physical_block_size;
 uint32_t logical_block_size;
 uint32_t min_io_size;
@@ -48,6 +49,8 @@ static inline unsigned int get_physical_block_exp(BlockConf 
*conf)
 }
 
 #define DEFINE_BLOCK_PROPERTIES_BASE(_state, _conf) \
+DEFINE_PROP_ON_OFF_AUTO("backend_defaults", _state, \
+_conf.backend_defaults, ON_OFF_AUTO_AUTO),  \
 DEFINE_PROP_BLOCKSIZE("logical_block_size", _state, \
   _conf.logical_block_size),\
 DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,\
diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out
index d53f61d0dee..4cf4d536b46 100644
--- a/tests/qemu-iotests/172.out
+++ b/tests/qemu-iotests/172.out
@@ -21,6 +21,7 @@ Testing:
   dev: floppy, id ""
 unit = 0 (0x0)
 drive = "floppy0"
+backend_defaults = "auto"
 logical_block_size = 512 (512 B)
 physical_block_size = 512 (512 B)
 min_io_size = 0 (0 B)
@@ -48,6 +49,7 @@ Testing: -fda TEST_DIR/t.qcow2
   dev: floppy, id ""
 unit = 0 (0x0)
 drive = "floppy0"
+backend_defaults = "auto"
 logical_block_size = 512 (512 B)
 physical_block_size = 512 (512 B)
 min_io_size = 0 (0 B)
@@ -85,6 +87,7 @@ Testing: -fdb TEST_DIR/t.qcow2
   dev: floppy, id ""
 unit = 1 (0x1)
 drive = "floppy1"
+backend_defaults = "auto"
 logical_block_size = 512 (512 B)
 physical_block_size = 512 (512 B)
 min_io_size = 0 (0 B)
@@ -96,6 +99,7 @@ Testing: -fdb TEST_DIR/t.qcow2
   dev: floppy, id ""
 unit = 0 (0x0)
 drive = "floppy0"
+ 

[PATCH v5 1/3] block/file-posix: Optimize for macOS

[Akihiko Odaki]

This commit introduces "punch hole" operation and optimizes transfer
block size for macOS.

Thanks to Konstantin Nazarov for detailed analysis of a flaw in an
old version of this change:
https://gist.github.com/akihikodaki/87df4149e7ca87f18dc56807ec5a1bc5#gistcomment-3654667

Signed-off-by: Akihiko Odaki 
---
 block/file-posix.c | 27 +--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index b3fbb9bd63a..80da1f59a60 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -44,6 +44,7 @@
 #if defined(__APPLE__) && (__MACH__)
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1217,6 +1218,15 @@ static void raw_refresh_limits(BlockDriverState *bs, 
Error **errp)
 {
 BDRVRawState *s = bs->opaque;
 
+#if defined(__APPLE__) && (__MACH__)
+struct statfs buf;
+
+if (!fstatfs(s->fd, &buf)) {
+bs->bl.opt_transfer = buf.f_iosize;
+bs->bl.pdiscard_alignment = buf.f_bsize;
+}
+#endif
+
 if (bs->sg) {
 int ret = sg_get_max_transfer_length(s->fd);
 
@@ -1557,6 +1567,7 @@ out:
 }
 }
 
+#if defined(CONFIG_FALLOCATE) || defined(BLKZEROOUT) || defined(BLKDISCARD)
 static int translate_err(int err)
 {
 if (err == -ENODEV || err == -ENOSYS || err == -EOPNOTSUPP ||
@@ -1565,6 +1576,7 @@ static int translate_err(int err)
 }
 return err;
 }
+#endif
 
 #ifdef CONFIG_FALLOCATE
 static int do_fallocate(int fd, int mode, off_t offset, off_t len)
@@ -1777,16 +1789,27 @@ static int handle_aiocb_discard(void *opaque)
 }
 } while (errno == EINTR);
 
-ret = -errno;
+ret = translate_err(-errno);
 #endif
 } else {
 #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
 ret = do_fallocate(s->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
aiocb->aio_offset, aiocb->aio_nbytes);
+ret = translate_err(-errno);
+#elif defined(__APPLE__) && (__MACH__)
+fpunchhole_t fpunchhole;
+fpunchhole.fp_flags = 0;
+fpunchhole.reserved = 0;
+fpunchhole.fp_offset = aiocb->aio_offset;
+fpunchhole.fp_length = aiocb->aio_nbytes;
+if (fcntl(s->fd, F_PUNCHHOLE, &fpunchhole) == -1) {
+ret = errno == ENODEV ? -ENOTSUP : -errno;
+} else {
+ret = 0;
+}
 #endif
 }
 
-ret = translate_err(ret);
 if (ret == -ENOTSUP) {
 s->has_discard = false;
 }
-- 
2.30.1 (Apple Git-130)

Re: [ovirt-users] Re: Any way to terminate stuck export task

[Gianluca Cecchi]

On Mon, Jul 5, 2021 at 2:13 PM Nir Soffer  wrote:

>
> >
> > vdsm 14342  3270  0 11:17 ?00:00:03 /usr/bin/qemu-img
> convert -p -t none -T none -f raw
> /rhev/data-center/mnt/blockSD/679c0725-75fb-4af7-bff1-7c447c5d789c/images/530b3e7f-4ce4-4051-9cac-1112f5f9e8b5/d2a89b5e-7d62-4695-96d8-b762ce52b379
> -O raw -o preallocation=falloc /rhev/data-center/mnt/172.16.1.137:
> _nas_EXPORT-DOMAIN/20433d5d-9d82-4079-9252-0e746ce54106/images/530b3e7f-4ce4-4051-9cac-1112f5f9e8b5/d2a89b5e-7d62-4695-96d8-b762ce52b379
>
> -o preallocation + NFS 4.0 + very slow NFS is your problem.
>
> qemu-img is using posix-fallocate() to preallocate the entire image at
> the start of the copy. With NFS 4.2
> this uses fallocate() linux specific syscall that allocates the space
> very efficiently in no time. With older
> NFS versions, this becomes a very slow loop, writing one byte for
> every 4k block.
>
> If you see -o preallocation, it means you are using an old vdsm
> version, we stopped using -o preallocation
> in 4.4.2, see https://bugzilla.redhat.com/1850267.
>

OK. As I said at the beginning the environment is latest 4.3
We are going to upgrade to 4.4 and we are making some complimentary
backups, for safeness.


> > On the hypervisor the ls commands quite hang, so from another hypervisor
> I see that the disk size seems to remain at 4Gb even if timestamp updates...
> >
> > # ll /rhev/data-center/mnt/172.16.1.137
> \:_nas_EXPORT-DOMAIN/20433d5d-9d82-4079-9252-0e746ce54106/images/530b3e7f-4ce4-4051-9cac-1112f5f9e8b5/
> > total 4260941
> > -rw-rw. 1 nobody nobody 4363202560 Jul  5 11:23
> d2a89b5e-7d62-4695-96d8-b762ce52b379
> > -rw-r--r--. 1 nobody nobody261 Jul  5 11:17
> d2a89b5e-7d62-4695-96d8-b762ce52b379.meta
> >
> > On host console I see a throughput of 4mbit/s...
> >
> > # strace -p 14342
>
> This shows only the main thread use -f use -f to show all threads.
>

 # strace -f -p 14342
strace: Process 14342 attached with 2 threads
[pid 14342] ppoll([{fd=9, events=POLLIN|POLLERR|POLLHUP}], 1, NULL, NULL, 8

[pid 14343] pwrite64(12, "\0", 1, 16474968063) = 1
[pid 14343] pwrite64(12, "\0", 1, 16474972159) = 1
[pid 14343] pwrite64(12, "\0", 1, 16474976255) = 1
[pid 14343] pwrite64(12, "\0", 1, 16474980351) = 1
[pid 14343] pwrite64(12, "\0", 1, 16474984447) = 1
[pid 14343] pwrite64(12, "\0", 1, 16474988543) = 1
[pid 14343] pwrite64(12, "\0", 1, 16474992639) = 1
[pid 14343] pwrite64(12, "\0", 1, 16474996735) = 1
[pid 14343] pwrite64(12, "\0", 1, 16475000831) = 1
[pid 14343] pwrite64(12, "\0", 1, 16475004927) = 1
. . . and so on . . .


>
> > This is a test oVirt env so I can wait and eventually test something...
> > Let me know your suggestions
>
> I would start by changing the NFS storage domain to version 4.2.
>

I'm going to try. RIght now I have set it to the default of
autonegotiated...


> 1. kill the hang qemu-img (it will probably cannot be killed, but worth
> trying)
> 2. deactivate the storage domain
> 3. fix the ownership on the storage domain (should be vdsm:kvm, not
> nobody:nobody)3.
>

Unfortunately it is an appliance. I have asked the guys that have it in
charge if we can set them.
Thanks for the other concepts explained.

Gianluca

Re: [ovirt-users] Re: Any way to terminate stuck export task

[Nir Soffer]

On Mon, Jul 5, 2021 at 12:50 PM Gianluca Cecchi
 wrote:
>
> On Sun, Jul 4, 2021 at 1:01 PM Nir Soffer  wrote:
>>
>> On Sun, Jul 4, 2021 at 11:30 AM Strahil Nikolov  
>> wrote:
>> >
>> > Isn't it better to strace it before killing qemu-img .
>>
>> It may be too late, but it may help to understand why this qemu-img
>> run got stuck.
>>
>
> Hi, thanks for your answers and suggestions.
> That env was a production one and so I was forced to power off the hypervisor 
> and power on it again (it was a maintenance window with all the VMs powered 
> down anyway). I was also unable to put the host into maintenance because it 
> replied that there were some tasks running, even after the kill, because the 
> 2 processes (the VM had 2 disks to export and so two qemu-img processes) 
> remained in defunct and after several minutes no change in web admin feedback 
> about the process
>
> My first suspicion was something related to fw congestion because the 
> hypervisor network and the nas appliance were in different networks and I 
> wasn't sure if a fw was in place for it
> But on a test oVirt environment with same oVirt version and with the same 
> network for hypervisors I was able to put a Linux server with the same 
> network as the nas and configure it as nfs server.
> And the export went with a throughput of about 50MB/s, so no fw problem.
> A VM with 55Gb disk exported in 19 minutes.
>
> So I got the rights to mount the nas on the test env and mounted it as export 
> domain and now I have the same problems I can debug.
> The same VM with only one disk (55Gb). The process:
>
> vdsm 14342  3270  0 11:17 ?00:00:03 /usr/bin/qemu-img convert -p 
> -t none -T none -f raw 
> /rhev/data-center/mnt/blockSD/679c0725-75fb-4af7-bff1-7c447c5d789c/images/530b3e7f-4ce4-4051-9cac-1112f5f9e8b5/d2a89b5e-7d62-4695-96d8-b762ce52b379
>  -O raw -o preallocation=falloc 
> /rhev/data-center/mnt/172.16.1.137:_nas_EXPORT-DOMAIN/20433d5d-9d82-4079-9252-0e746ce54106/images/530b3e7f-4ce4-4051-9cac-1112f5f9e8b5/d2a89b5e-7d62-4695-96d8-b762ce52b379

-o preallocation + NFS 4.0 + very slow NFS is your problem.

qemu-img is using posix-fallocate() to preallocate the entire image at
the start of the copy. With NFS 4.2
this uses fallocate() linux specific syscall that allocates the space
very efficiently in no time. With older
NFS versions, this becomes a very slow loop, writing one byte for
every 4k block.

If you see -o preallocation, it means you are using an old vdsm
version, we stopped using -o preallocation
in 4.4.2, see https://bugzilla.redhat.com/1850267.

> On the hypervisor the ls commands quite hang, so from another hypervisor I 
> see that the disk size seems to remain at 4Gb even if timestamp updates...
>
> # ll 
> /rhev/data-center/mnt/172.16.1.137\:_nas_EXPORT-DOMAIN/20433d5d-9d82-4079-9252-0e746ce54106/images/530b3e7f-4ce4-4051-9cac-1112f5f9e8b5/
> total 4260941
> -rw-rw. 1 nobody nobody 4363202560 Jul  5 11:23 
> d2a89b5e-7d62-4695-96d8-b762ce52b379
> -rw-r--r--. 1 nobody nobody261 Jul  5 11:17 
> d2a89b5e-7d62-4695-96d8-b762ce52b379.meta
>
> On host console I see a throughput of 4mbit/s...
>
> # strace -p 14342

This shows only the main thread use -f use -f to show all threads.

> strace: Process 14342 attached
> ppoll([{fd=9, events=POLLIN|POLLERR|POLLHUP}], 1, NULL, NULL, 8
>
> # ll /proc/14342/fd
> hangs...
>
> # nfsstat -v
> Client packet stats:
> packetsudptcptcpconn
> 0  0  0  0
>
> Client rpc stats:
> calls  retransauthrefrsh
> 31171856   0  31186615
>
> Client nfs v4:
> null read writecommit   open open_conf
> 0 0% 2339179   7% 14872911 47% 7233  0% 74956 0% 2 0%
> open_noatopen_dgrdclosesetattr  fsinfo   renew
> 2312347   7% 0 0% 2387293   7% 240% 230% 5 0%
> setclntidconfirm  lock locktlockuaccess
> 3 0% 3 0% 8 0% 8 0% 5 0% 1342746   4%
> getattr  lookup   lookup_root  remove   rename   link
> 3031001   9% 71551 0% 7 0% 74590 0% 6 0% 0 0%
> symlink  create   pathconf statfs   readlink readdir
> 0 0% 9 0% 160% 4548231  14% 0 0% 98506 0%
> server_caps  delegreturn  getacl   setacl   fs_locations rel_lkowner
> 390% 140% 0 0% 0 0% 0 0% 0 0%
> secinfo  exchange_id  create_ses   destroy_ses  sequence get_lease_t
> 0 0% 0 0% 4 0% 2 0% 1 0% 0 0%
> reclaim_comp layoutgetgetdevinfo   layoutcommit layoutreturn getdevlist
> 0 0% 2 0% 0 0% 0 0% 0 0% 0 0%
> (null)
> 5 0%
>
>
> # vmstat 3
> procs ---memory-- ---swap-- -io -system-- 
> --cpu-
>  r  b   swpd   

Re: [ovirt-users] Re: Any way to terminate stuck export task

[Gianluca Cecchi]

On Mon, Jul 5, 2021 at 11:56 AM Strahil Nikolov 
wrote:

> That NFS looks like it is not properly configured -> nobody:nobody is not
> suposed to be seen.
>
> Change the ownership from nfs side to 36:36. Also, you can define
> (all_squash,anonuid=36,anongid=36) as export options.
>
>
> Best Regards,
> Strahil Nikolov
>
>
I have those options in my test with a Linux box exporting via NFS. But
from the appliance point of view I have to check if it is possible... It is
not under my control and I don't know that appliance architecture. Anyone?

Gianluca

Re: [RFC] block/mirror: fix file-system went to read-only after block-mirror

[Vladimir Sementsov-Ogievskiy]

24.06.2021 15:06, Jinhua Cao wrote:

1) Configure the VM disk as prdm.
...

   
   
   
   
   
   

...
Mount the disk in guest and keep the disk writing data continuously during 
block-mirror,
the file-system went to read-only after block-mirror.

2) This commit 6cdbceb12cf[mirror: Add filter-node-name to blockdev-mirror] 
introduces
mirror_top_bs which does not set default function for 
mirror_top_bs->drv->bdrv_co_ioctl.

3) The function bdrv_co_ioctl in block/io.c will be called during block-mirror, 
in this
function, the judgment is as follow:
---
 if (!drv || (!drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl)) {
 co.ret = -ENOTSUP;
 goto out;
 }
---
The mirror_top_bs does not set drv->bdrv_aio_ioctl or drv->bdrv_co_ioctl which 
result this
return -ENOTSUP. So the file-system went to read-only after block-mirror.

4) This patch set a default function for mirror_top_bs->drv->bdrv_aio_ioctl, 
fix this problem.
---
  block/mirror.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index 019f6deaa5..63b788ec39 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1480,6 +1480,12 @@ static int coroutine_fn 
bdrv_mirror_top_flush(BlockDriverState *bs)
  return bdrv_co_flush(bs->backing->bs);
  }
  
+static int coroutine_fn bdrv_mirror_top_ioctl(BlockDriverState *bs,

+unsigned long int req, void *buf)
+{
+return 0;
+}


I'm not very familiar with .bdrv_co_ioctl kitchen in Qemu, but intuitively this 
seems wrong to me: you do nothing and report success in this handler.

So, probably more correct would be at least call bdrv_co_ioctl() on 
bs->backing->bs, which is filtered child of mirror_top.

This raise another question: what exactly this ioctl does? If it changes the 
device like write operation, we should somehow handle it to update dirty 
bitmap, otherwise mirror will not work correctly. In this way, it seems 
correctly to not implement .bdrv_co_ioctl in mirror_top, and keep it returning 
ENOTSUP, as we really can't support it..


+
  static int coroutine_fn bdrv_mirror_top_pwrite_zeroes(BlockDriverState *bs,
  int64_t offset, int bytes, BdrvRequestFlags flags)
  {
@@ -1555,6 +1561,7 @@ static BlockDriver bdrv_mirror_top = {
  .bdrv_co_pwrite_zeroes  = bdrv_mirror_top_pwrite_zeroes,
  .bdrv_co_pdiscard   = bdrv_mirror_top_pdiscard,
  .bdrv_co_flush  = bdrv_mirror_top_flush,
+.bdrv_co_ioctl  = bdrv_mirror_top_ioctl,
  .bdrv_refresh_filename  = bdrv_mirror_top_refresh_filename,
  .bdrv_child_perm= bdrv_mirror_top_child_perm,
  




--
Best regards,
Vladimir

Re: [PATCH 06/14] iotest 302: use img_info_log() helper

[Vladimir Sementsov-Ogievskiy]

05.07.2021 12:15, Vladimir Sementsov-Ogievskiy wrote:

Instead of qemu_img_log("info", ..) use generic helper img_info_log().

img_info_log() has smarter logic. For example it use filter_img_info()
to filter output, which in turns filter a compression type. So it will
help us in future when we implement a possibility to use zstd
compression by default (with help of some runtime config file or maybe
build option). For now to test you should recompile qemu with a small
patch:

 --- a/block/qcow2.c
 +++ b/block/qcow2.c
 @@ -3540,6 +3540,11 @@ qcow2_co_create(BlockdevCreateOptions 
*create_options, Error **errp)
  }
  }

 +if (!qcow2_opts->has_compression_type && version >= 3) {
 +qcow2_opts->has_compression_type = true;
 +qcow2_opts->compression_type = QCOW2_COMPRESSION_TYPE_ZSTD;
 +}
 +
  if (qcow2_opts->has_compression_type &&
  qcow2_opts->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB) {

Signed-off-by: Vladimir Sementsov-Ogievskiy


Wow, that was bad idea to insert patch into commit message even with indent: it 
breaks rpm build for me.

So, reword like this:

build option). For now to test you should recompile qemu with a small
addition into block/qcow2.c before
  "if (qcow2_opts->has_compression_type":

if (!qcow2_opts->has_compression_type && version >= 3) {

qcow2_opts->has_compression_type = true;
qcow2_opts->compression_type = QCOW2_COMPRESSION_TYPE_ZSTD;
}


--
Best regards,
Vladimir

Re: [PATCH 0/2] util/async: print leaked BH name when AioContext finalizes

[Stefan Hajnoczi]

On Wed, Apr 14, 2021 at 09:02:45PM +0100, Stefan Hajnoczi wrote:
> Eric Ernst and I debugged a BH leak and it was more involved than it should 
> be.
> The problem is that BHs don't have a human-readable identifier, so low-level
> debugging techniques and inferences about the code are required to figure out
> which BH was leaked in production environments without easy debug access.
> 
> The leak ended up already being fixed upstream but let's improve diagnostics
> for leaked BHs so that this becomes quick and easy in the future.
> 
> Stefan Hajnoczi (2):
>   util/async: add a human-readable name to BHs for debugging
>   util/async: print leaked BH name when AioContext finalizes
> 
>  include/block/aio.h| 31 ---
>  include/qemu/main-loop.h   |  4 +++-
>  tests/unit/ptimer-test-stubs.c |  2 +-
>  util/async.c   | 25 +
>  util/main-loop.c   |  4 ++--
>  5 files changed, 55 insertions(+), 11 deletions(-)
> 
> -- 
> 2.30.2
> 

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

Stefan


signature.asc
Description: PGP signature

Re: [v5 1/3] block/file-posix: Optimize for macOS

[Stefan Hajnoczi]

On Fri, Apr 02, 2021 at 09:02:54AM -0700, Akihiko Odaki wrote:
> This commit introduces "punch hole" operation and optimizes transfer
> block size for macOS.
> 
> Thanks to Konstantin Nazarov for detailed analysis of a flaw in an
> old version of this change:
> https://gist.github.com/akihikodaki/87df4149e7ca87f18dc56807ec5a1bc5#gistcomment-3654667
> 
> Signed-off-by: Akihiko Odaki 
> ---
>  block/file-posix.c | 27 +--
>  1 file changed, 25 insertions(+), 2 deletions(-)

I encountered the following qemu-iotests failure after applying this
series:

172 fail   [11:09:32] [11:09:36]   3.6s 
output mismatch (see 172.out.bad)
--- /home/stefanha/qemu/tests/qemu-iotests/172.out
+++ 172.out.bad
@@ -21,6 +21,7 @@
   dev: floppy, id ""
 unit = 0 (0x0)
 drive = "floppy0"
+backend_defaults = "auto"
 logical_block_size = 512 (512 B)
 physical_block_size = 512 (512 B)
 min_io_size = 0 (0 B)
...

You can run the test case like this:

  $ (cd tests/qemu-iotests && ./check -qcow2 172)

It looks like the 172.out test output file just needs to be updated to
include the new backend_defaults property.

Stefan


signature.asc
Description: PGP signature

Re: [ovirt-users] Re: Any way to terminate stuck export task

[Strahil Nikolov]

That NFS looks like it is not properly configured -> nobody:nobody is not 
suposed to be seen.
Change the ownership from nfs side to 36:36. Also, you can define 
(all_squash,anonuid=36,anongid=36) as export options.
 

Best Regards,Strahil Nikolov
 
  On Mon, Jul 5, 2021 at 12:52, Gianluca Cecchi 
wrote:   ___
Users mailing list -- us...@ovirt.org
To unsubscribe send an email to users-le...@ovirt.org
Privacy Statement: https://www.ovirt.org/privacy-policy.html
oVirt Code of Conduct: 
https://www.ovirt.org/community/about/community-guidelines/
List Archives: 
https://lists.ovirt.org/archives/list/us...@ovirt.org/message/6HE5TY3GTB32JFIOHAUQ2HMNJVRQDEDK/
  

Re: [PATCH] hw/sd: sdhci: Enable 64-bit system bus capability in the default SD/MMC host controller

[Philippe Mathieu-Daudé]

On 6/23/21 8:59 PM, Joanne Koong wrote:
> The default SD/MMC host controller uses SD spec v2.00. 64-bit system bus 
> capability
> was added in v2.
> 
> In this change, we arrive at 0x157834b4 by computing (0x057834b4 | (1ul << 
> 28))
> where 28 represents the BUS64BIT SDHC_CAPAB field.
> 
> Signed-off-by: Joanne Koong 
> ---
>  hw/sd/sdhci-internal.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, series applied to sdmmc-next tree.

Re: [PATCH 0/3] hw/sd: Check for valid address range in SEND_WRITE_PROT (CMD30)

[Philippe Mathieu-Daudé]

On 7/2/21 5:58 PM, Philippe Mathieu-Daudé wrote:
> Trivial fix for https://gitlab.com/qemu-project/qemu/-/issues/450
> 
> Missing review: patch #3
> 
> Philippe Mathieu-Daudé (3):
>   hw/sd: When card is in wrong state, log which state it is
>   hw/sd: Extract address_in_range() helper, log invalid accesses
>   hw/sd: Check for valid address range in SEND_WRITE_PROT (CMD30)

Thanks, series applied to sdmmc-next tree.

Re: [ovirt-users] Re: Any way to terminate stuck export task

[Gianluca Cecchi]

On Sun, Jul 4, 2021 at 1:01 PM Nir Soffer  wrote:

> On Sun, Jul 4, 2021 at 11:30 AM Strahil Nikolov 
> wrote:
> >
> > Isn't it better to strace it before killing qemu-img .
>
> It may be too late, but it may help to understand why this qemu-img
> run got stuck.
>
>
Hi, thanks for your answers and suggestions.
That env was a production one and so I was forced to power off the
hypervisor and power on it again (it was a maintenance window with all the
VMs powered down anyway). I was also unable to put the host into
maintenance because it replied that there were some tasks running, even
after the kill, because the 2 processes (the VM had 2 disks to export and
so two qemu-img processes) remained in defunct and after several minutes no
change in web admin feedback about the process

My first suspicion was something related to fw congestion because the
hypervisor network and the nas appliance were in different networks and I
wasn't sure if a fw was in place for it
But on a test oVirt environment with same oVirt version and with the same
network for hypervisors I was able to put a Linux server with the same
network as the nas and configure it as nfs server.
And the export went with a throughput of about 50MB/s, so no fw problem.
A VM with 55Gb disk exported in 19 minutes.

So I got the rights to mount the nas on the test env and mounted it as
export domain and now I have the same problems I can debug.
The same VM with only one disk (55Gb). The process:

vdsm 14342  3270  0 11:17 ?00:00:03 /usr/bin/qemu-img convert
-p -t none -T none -f raw
/rhev/data-center/mnt/blockSD/679c0725-75fb-4af7-bff1-7c447c5d789c/images/530b3e7f-4ce4-4051-9cac-1112f5f9e8b5/d2a89b5e-7d62-4695-96d8-b762ce52b379
-O raw -o preallocation=falloc /rhev/data-center/mnt/172.16.1.137:
_nas_EXPORT-DOMAIN/20433d5d-9d82-4079-9252-0e746ce54106/images/530b3e7f-4ce4-4051-9cac-1112f5f9e8b5/d2a89b5e-7d62-4695-96d8-b762ce52b379

On the hypervisor the ls commands quite hang, so from another hypervisor I
see that the disk size seems to remain at 4Gb even if timestamp updates...

# ll /rhev/data-center/mnt/172.16.1.137
\:_nas_EXPORT-DOMAIN/20433d5d-9d82-4079-9252-0e746ce54106/images/530b3e7f-4ce4-4051-9cac-1112f5f9e8b5/
total 4260941
-rw-rw. 1 nobody nobody 4363202560 Jul  5 11:23
d2a89b5e-7d62-4695-96d8-b762ce52b379
-rw-r--r--. 1 nobody nobody261 Jul  5 11:17
d2a89b5e-7d62-4695-96d8-b762ce52b379.meta

On host console I see a throughput of 4mbit/s...

# strace -p 14342
strace: Process 14342 attached
ppoll([{fd=9, events=POLLIN|POLLERR|POLLHUP}], 1, NULL, NULL, 8

# ll /proc/14342/fd
hangs...

# nfsstat -v
Client packet stats:
packetsudptcptcpconn
0  0  0  0

Client rpc stats:
calls  retransauthrefrsh
31171856   0  31186615

Client nfs v4:
null read writecommit   open open_conf

0 0% 2339179   7% 14872911 47% 7233  0% 74956 0% 2
0%
open_noatopen_dgrdclosesetattr  fsinfo   renew

2312347   7% 0 0% 2387293   7% 240% 230% 5
0%
setclntidconfirm  lock locktlockuaccess

3 0% 3 0% 8 0% 8 0% 5 0% 1342746
4%
getattr  lookup   lookup_root  remove   rename   link

3031001   9% 71551 0% 7 0% 74590 0% 6 0% 0
0%
symlink  create   pathconf statfs   readlink readdir

0 0% 9 0% 160% 4548231  14% 0 0% 98506
0%
server_caps  delegreturn  getacl   setacl   fs_locations
rel_lkowner
390% 140% 0 0% 0 0% 0 0% 0
0%
secinfo  exchange_id  create_ses   destroy_ses  sequence
get_lease_t
0 0% 0 0% 4 0% 2 0% 1 0% 0
0%
reclaim_comp layoutgetgetdevinfo   layoutcommit layoutreturn getdevlist

0 0% 2 0% 0 0% 0 0% 0 0% 0
0%
(null)
5 0%


# vmstat 3
procs ---memory-- ---swap-- -io -system--
--cpu-
 r  b   swpd   free   buff  cache   si   sobibo   in   cs us sy id
wa st
 3  1  0 82867112 437548 70665800054 100  0  0
100  0  0
 0  1  0 82867024 437548 706662000  1708 0 3720 8638  0  0
95  4  0
 4  1  0 82868728 437552 706661600   875 9 3004 8457  0  0
95  4  0
 0  1  0 82869600 437552 706663600  1785 6 2982 8359  0  0
95  4  0

I see the blocked process that is my qemu-img one...

In messages of hypervisor

Jul  5 11:33:06 node4 kernel: INFO: task qemu-img:14343 blocked for more
than 120 seconds.
Jul  5 11:33:06 node4 kernel: "echo 0 >
/proc/sys/kernel/hung_task_timeout_secs" disables this message.
Jul  5 11:33:06 node4 kernel: qemu-imgD 9d960e7e1080 0
14343   3328 0x0080
Jul  5 11:33:06 node4 kernel: Call Trace:
Jul  5 11:33:06 node4 kernel: [] ?
sched_clock_cpu+0x85/0xc0
Ju

[PATCH 09/14] iotests/common.rc: introduce _qcow2_dump_header helper

[Vladimir Sementsov-Ogievskiy]

We'll use it in tests instead of explicit qcow2.py. Then we are going
to add some filtering in _qcow2_dump_header.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/common.rc | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 4cae5b2d70..ee4b9d795e 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -994,5 +994,15 @@ _require_one_device_of()
 _notrun "$* not available"
 }
 
+_qcow2_dump_header()
+{
+img="$1"
+if [ -z "$img" ]; then
+img="$TEST_IMG"
+fi
+
+$PYTHON qcow2.py "$img" dump-header
+}
+
 # make sure this script returns success
 true
-- 
2.29.2

[PATCH 11/14] iotests: bash tests: filter compression type

[Vladimir Sementsov-Ogievskiy]

We want iotests pass with both the default zlib compression and with
IMGOPTS='compression_type=zstd'.

Actually the only test that is interested in real compression type in
test output is 287 (test for qcow2 compression type), so implement
specific option for it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/060.out   |  2 +-
 tests/qemu-iotests/061.out   | 12 ++--
 tests/qemu-iotests/082.out   | 14 +++---
 tests/qemu-iotests/198.out   |  4 ++--
 tests/qemu-iotests/287   |  8 
 tests/qemu-iotests/common.filter |  7 +++
 tests/qemu-iotests/common.rc | 14 +-
 7 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index b74540bafb..329977d9b9 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -17,7 +17,7 @@ virtual size: 64 MiB (67108864 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 corrupt: true
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index ee30da2665..11b6404186 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -524,7 +524,7 @@ virtual size: 64 MiB (67108864 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 data file: TEST_DIR/t.IMGFMT.data
@@ -551,7 +551,7 @@ virtual size: 64 MiB (67108864 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 data file: foo
@@ -566,7 +566,7 @@ virtual size: 64 MiB (67108864 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 data file raw: false
@@ -582,7 +582,7 @@ virtual size: 64 MiB (67108864 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 data file: TEST_DIR/t.IMGFMT.data
@@ -596,7 +596,7 @@ virtual size: 64 MiB (67108864 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 data file: TEST_DIR/t.IMGFMT.data
@@ -611,7 +611,7 @@ virtual size: 64 MiB (67108864 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 data file: TEST_DIR/t.IMGFMT.data
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index b70c12c139..f8e2e039fc 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -17,7 +17,7 @@ virtual size: 128 MiB (134217728 bytes)
 cluster_size: 4096
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: true
 refcount bits: 16
 corrupt: false
@@ -31,7 +31,7 @@ virtual size: 128 MiB (134217728 bytes)
 cluster_size: 8192
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: true
 refcount bits: 16
 corrupt: false
@@ -329,7 +329,7 @@ virtual size: 128 MiB (134217728 bytes)
 cluster_size: 4096
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: true
 refcount bits: 16
 corrupt: false
@@ -342,7 +342,7 @@ virtual size: 128 MiB (134217728 bytes)
 cluster_size: 8192
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: true
 refcount bits: 16
 corrupt: false
@@ -639,7 +639,7 @@ virtual size: 128 MiB (134217728 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: true
 refcount bits: 16
 corrupt: false
@@ -652,7 +652,7 @@ virtual size: 130 MiB (136314880 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 corrupt: false
@@ -665,7 +665,7 @@ virtual size: 132 MiB (138412032 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: true
 refcount bits: 16
 corrupt: false
dif

[PATCH 07/14] qcow2: simple case support for downgrading of qcow2 images with zstd

[Vladimir Sementsov-Ogievskiy]

If image doesn't have any compressed cluster we can easily switch to
zlib compression, which may allow to downgrade the image.

That's mostly needed to support IMGOPTS='compression_type=zstd' in some
iotests which do qcow2 downgrade.

While being here also fix checkpatch complain against '#' in printf
formatting.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2.c | 58 +--
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index ee4530cdbd..bed3354474 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5221,6 +5221,38 @@ static int qcow2_load_vmstate(BlockDriverState *bs, 
QEMUIOVector *qiov,
 qiov->size, qiov, 0, 0);
 }
 
+static int qcow2_has_compressed_clusters(BlockDriverState *bs)
+{
+int64_t offset = 0;
+int64_t bytes = bdrv_getlength(bs);
+
+if (bytes < 0) {
+return bytes;
+}
+
+while (bytes != 0) {
+int ret;
+QCow2SubclusterType type;
+unsigned int cur_bytes = MIN(INT_MAX, bytes);
+uint64_t host_offset;
+
+ret = qcow2_get_host_offset(bs, offset, &cur_bytes, &host_offset,
+&type);
+if (ret < 0) {
+return ret;
+}
+
+if (type == QCOW2_SUBCLUSTER_COMPRESSED) {
+return 1;
+}
+
+offset += cur_bytes;
+bytes -= cur_bytes;
+}
+
+return 0;
+}
+
 /*
  * Downgrades an image's version. To achieve this, any incompatible features
  * have to be removed.
@@ -5278,9 +5310,10 @@ static int qcow2_downgrade(BlockDriverState *bs, int 
target_version,
  * the first place; if that happens nonetheless, returning -ENOTSUP is the
  * best thing to do anyway */
 
-if (s->incompatible_features) {
+if (s->incompatible_features & ~QCOW2_INCOMPAT_COMPRESSION) {
 error_setg(errp, "Cannot downgrade an image with incompatible features 
"
-   "%#" PRIx64 " set", s->incompatible_features);
+   "0x%" PRIx64 " set",
+   s->incompatible_features & ~QCOW2_INCOMPAT_COMPRESSION);
 return -ENOTSUP;
 }
 
@@ -5298,6 +5331,27 @@ static int qcow2_downgrade(BlockDriverState *bs, int 
target_version,
 return ret;
 }
 
+if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION) {
+ret = qcow2_has_compressed_clusters(bs);
+if (ret < 0) {
+error_setg(errp, "Failed to check block status");
+return -EINVAL;
+}
+if (ret) {
+error_setg(errp, "Cannot downgrade an image with zstd compression "
+   "type and existing compressed clusters");
+return -ENOTSUP;
+}
+/*
+ * No compressed clusters for now, so just chose default zlib
+ * compression.
+ */
+s->incompatible_features = 0;
+s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+}
+
+assert(s->incompatible_features == 0);
+
 s->qcow_version = target_version;
 ret = qcow2_update_header(bs);
 if (ret < 0) {
-- 
2.29.2

[PATCH 06/14] iotest 302: use img_info_log() helper

[Vladimir Sementsov-Ogievskiy]

Instead of qemu_img_log("info", ..) use generic helper img_info_log().

img_info_log() has smarter logic. For example it use filter_img_info()
to filter output, which in turns filter a compression type. So it will
help us in future when we implement a possibility to use zstd
compression by default (with help of some runtime config file or maybe
build option). For now to test you should recompile qemu with a small
patch:

--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3540,6 +3540,11 @@ qcow2_co_create(BlockdevCreateOptions 
*create_options, Error **errp)
 }
 }

+if (!qcow2_opts->has_compression_type && version >= 3) {
+qcow2_opts->has_compression_type = true;
+qcow2_opts->compression_type = QCOW2_COMPRESSION_TYPE_ZSTD;
+}
+
 if (qcow2_opts->has_compression_type &&
 qcow2_opts->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB) {

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/302 | 3 ++-
 tests/qemu-iotests/302.out | 7 +++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/302 b/tests/qemu-iotests/302
index 5695af4914..2180dbc896 100755
--- a/tests/qemu-iotests/302
+++ b/tests/qemu-iotests/302
@@ -34,6 +34,7 @@ from iotests import (
 qemu_img_measure,
 qemu_io,
 qemu_nbd_popen,
+img_info_log,
 )
 
 iotests.script_initialize(supported_fmts=["qcow2"])
@@ -99,7 +100,7 @@ with tarfile.open(tar_file, "w") as tar:
 nbd_uri)
 
 iotests.log("=== Converted image info ===")
-qemu_img_log("info", nbd_uri)
+img_info_log(nbd_uri)
 
 iotests.log("=== Converted image check ===")
 qemu_img_log("check", nbd_uri)
diff --git a/tests/qemu-iotests/302.out b/tests/qemu-iotests/302.out
index e2f6077e83..3e7c281b91 100644
--- a/tests/qemu-iotests/302.out
+++ b/tests/qemu-iotests/302.out
@@ -6,14 +6,13 @@ virtual size: 448 KiB (458752 bytes)
 disk size: unavailable
 
 === Converted image info ===
-image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
-file format: qcow2
+image: TEST_IMG
+file format: IMGFMT
 virtual size: 1 GiB (1073741824 bytes)
-disk size: unavailable
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 corrupt: false
-- 
2.29.2

[PATCH 12/14] iotests 60: more accurate set dirty bit in qcow2 header

[Vladimir Sementsov-Ogievskiy]

Don't touch other incompatible bits, like compression-type. This makes
the test pass with IMGOPTS='compression_type=zstd'.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/060 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index d1e3204d4e..df87d600f7 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -326,7 +326,7 @@ _make_test_img 64M
 # Let the refblock appear unaligned
 poke_file "$TEST_IMG" "$rt_offset""\x00\x00\x00\x00\xff\xff\x2a\x00"
 # Mark the image dirty, thus forcing an automatic check when opening it
-poke_file "$TEST_IMG" 72 "\x00\x00\x00\x00\x00\x00\x00\x01"
+$PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 0
 # Open the image (qemu should refuse to do so)
 $QEMU_IO -c close "$TEST_IMG" 2>&1 | _filter_testdir | _filter_imgfmt
 
-- 
2.29.2

[PATCH 08/14] iotests/common.rc: _make_test_img(): smarter compressiont_type handling

[Vladimir Sementsov-Ogievskiy]

Like it is done in iotests.py in qemu_img_create_prepare_args(), let's
not follow compression_type=zstd of IMGOPTS if test creates image in
old format.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/common.rc | 8 
 1 file changed, 8 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index cbbf6d7c7f..4cae5b2d70 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -438,6 +438,14 @@ _make_test_img()
 backing_file=$param
 continue
 elif $opts_param; then
+if [[ "$param" == *"compat=0"* ]]; then
+# If user specified zstd compression type in IMGOPTS, this will
+# just not work. So, let's imply forcing zlib compression when
+# test creates image in old version of the format.
+# Similarly works qemu_img_create_prepare_args() in iotests.py
+optstr=$(echo "$optstr" | $SED -e 's/compression_type=\w\+//')
+optstr=$(_optstr_add "$optstr" "compression_type=zlib")
+fi
 optstr=$(_optstr_add "$optstr" "$param")
 opts_param=false
 continue
-- 
2.29.2

[PATCH 03/14] iotest 303: explicit compression type

[Vladimir Sementsov-Ogievskiy]

The test prints qcow2 header fields which depends on chosen compression
type. So, let's be explicit in what compression type we want and
independent of IMGOPTS. Test both existing compression types.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/303 | 25 -
 tests/qemu-iotests/303.out | 30 +-
 2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
index 425544c064..9dee2bdfb8 100755
--- a/tests/qemu-iotests/303
+++ b/tests/qemu-iotests/303
@@ -53,12 +53,19 @@ def add_bitmap(num, begin, end, disabled):
 log('')
 
 
-qemu_img_create('-f', iotests.imgfmt, disk, '10M')
-
-add_bitmap(1, 0, 6, False)
-add_bitmap(2, 6, 8, True)
-dump = ['./qcow2.py', disk, 'dump-header']
-subprocess.run(dump)
-# Dump the metadata in JSON format
-dump.append('-j')
-subprocess.run(dump)
+def test(compression_type: str, json_output: bool) -> None:
+qemu_img_create('-f', iotests.imgfmt,
+'-o', f'compression_type={compression_type}',
+disk, '10M')
+add_bitmap(1, 0, 6, False)
+add_bitmap(2, 6, 8, True)
+
+cmd = ['./qcow2.py', disk, 'dump-header']
+if json_output:
+cmd.append('-j')
+
+subprocess.run(cmd)
+
+
+test('zlib', False)
+test('zstd', True)
diff --git a/tests/qemu-iotests/303.out b/tests/qemu-iotests/303.out
index 7c16998587..b3c70827b7 100644
--- a/tests/qemu-iotests/303.out
+++ b/tests/qemu-iotests/303.out
@@ -80,6 +80,34 @@ extra_data_size   0
 Bitmap table   typesize offset
 0  all-zeroes  00
 
+Add bitmap 1
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 2097152
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 4194304
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 5242880
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+
+Add bitmap 2
+wrote 1048576/1048576 bytes at offset 6291456
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 7340032
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+
 {
 "magic": 1363560955,
 "version": 3,
@@ -94,7 +122,7 @@ Bitmap table   typesize offset
 "refcount_table_clusters": 1,
 "nb_snapshots": 0,
 "snapshot_offset": 0,
-"incompatible_features": 0,
+"incompatible_features": 8,
 "compatible_features": 0,
 "autoclear_features": 1,
 "refcount_order": 4,
-- 
2.29.2

[PATCH 10/14] iotests: massive use _qcow2_dump_header

[Vladimir Sementsov-Ogievskiy]

We are going to add filtering in _qcow2_dump_header and want all tests
use it.

The patch is generated by commands:
  cd tests/qemu-iotests
  sed -ie 's/$PYTHON qcow2.py "$TEST_IMG" dump-header\($\| 
\)/_qcow2_dump_header\1/' ??? tests/*

(the difficulty is to avoid converting dump-header-exts)

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/031 |  6 +++---
 tests/qemu-iotests/036 |  6 +++---
 tests/qemu-iotests/039 | 20 ++--
 tests/qemu-iotests/060 | 20 ++--
 tests/qemu-iotests/061 | 36 ++--
 tests/qemu-iotests/137 |  2 +-
 tests/qemu-iotests/287 |  8 
 7 files changed, 49 insertions(+), 49 deletions(-)

diff --git a/tests/qemu-iotests/031 b/tests/qemu-iotests/031
index 58b57a0ef2..648112f796 100755
--- a/tests/qemu-iotests/031
+++ b/tests/qemu-iotests/031
@@ -58,21 +58,21 @@ for compat in "compat=0.10" "compat=1.1"; do
 echo
 _make_test_img -o $compat 64M
 $PYTHON qcow2.py "$TEST_IMG" add-header-ext 0x12345678 "This is a test 
header extension"
-$PYTHON qcow2.py "$TEST_IMG" dump-header
+_qcow2_dump_header
 _check_test_img
 
 echo
 echo === Rewrite header with no backing file ===
 echo
 $QEMU_IMG rebase -u -b "" "$TEST_IMG"
-$PYTHON qcow2.py "$TEST_IMG" dump-header
+_qcow2_dump_header
 _check_test_img
 
 echo
 echo === Add a backing file and format ===
 echo
 $QEMU_IMG rebase -u -b "/some/backing/file/path" -F host_device "$TEST_IMG"
-$PYTHON qcow2.py "$TEST_IMG" dump-header
+_qcow2_dump_header
 done
 
 # success, all done
diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
index 5e567012a8..f703605e44 100755
--- a/tests/qemu-iotests/036
+++ b/tests/qemu-iotests/036
@@ -58,7 +58,7 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 63
 
 # Without feature table
 $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857
-$PYTHON qcow2.py "$TEST_IMG" dump-header | grep features
+_qcow2_dump_header | grep features
 $PYTHON qcow2.py "$TEST_IMG" dump-header-exts
 _img_info
 
@@ -107,7 +107,7 @@ echo === Create image with unknown autoclear feature bit ===
 echo
 _make_test_img 64M
 $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63
-$PYTHON qcow2.py "$TEST_IMG" dump-header | grep features
+_qcow2_dump_header | grep features
 $PYTHON qcow2.py "$TEST_IMG" dump-header-exts
 
 echo
@@ -115,7 +115,7 @@ echo === Repair image ===
 echo
 _check_test_img -r all
 
-$PYTHON qcow2.py "$TEST_IMG" dump-header | grep features
+_qcow2_dump_header | grep features
 $PYTHON qcow2.py "$TEST_IMG" dump-header-exts
 
 # success, all done
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 12b2c7fa7b..8e783a8380 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -59,7 +59,7 @@ _make_test_img -o "compat=1.1,lazy_refcounts=on" $size
 $QEMU_IO -c "write -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
 
 # The dirty bit must not be set
-$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_qcow2_dump_header | grep incompatible_features
 _check_test_img
 
 echo
@@ -73,7 +73,7 @@ $QEMU_IO -c "write -P 0x5a 0 512" \
 | _filter_qemu_io
 
 # The dirty bit must be set
-$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_qcow2_dump_header | grep incompatible_features
 _check_test_img
 
 echo
@@ -82,7 +82,7 @@ echo "== Read-only access must still work =="
 $QEMU_IO -r -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
 
 # The dirty bit must be set
-$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_qcow2_dump_header | grep incompatible_features
 
 echo
 echo "== Repairing the image file must succeed =="
@@ -90,7 +90,7 @@ echo "== Repairing the image file must succeed =="
 _check_test_img -r all
 
 # The dirty bit must not be set
-$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_qcow2_dump_header | grep incompatible_features
 
 echo
 echo "== Data should still be accessible after repair =="
@@ -108,12 +108,12 @@ $QEMU_IO -c "write -P 0x5a 0 512" \
 | _filter_qemu_io
 
 # The dirty bit must be set
-$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_qcow2_dump_header | grep incompatible_features
 
 $QEMU_IO -c "write 0 512" "$TEST_IMG" | _filter_qemu_io
 
 # The dirty bit must not be set
-$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_qcow2_dump_header | grep incompatible_features
 
 echo
 echo "== Creating an image file with lazy_refcounts=off =="
@@ -126,7 +126,7 @@ $QEMU_IO -c "write -P 0x5a 0 512" \
 | _filter_qemu_io
 
 # The dirty bit must not be set since lazy_refcounts=off
-$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_qcow2_dump_header | grep incompatible_features
 _check_test_img
 
 echo
@@ -141,7 +141,7 @@ $QEMU_IO -c "write 0 512" "$TEST_IMG" | _filter_qemu_io
 $QEMU_IMG commit "$TEST_IMG"
 
 # The dirty bit must not be set
-$PYTHON qcow2.py "$TEST_IMG" 

[PATCH 05/14] iotests.py: filter compression type out

[Vladimir Sementsov-Ogievskiy]

We want iotests pass with both the default zlib compression and with
IMGOPTS='compression_type=zstd'.

Actually the only test that is interested in real compression type in
test output is 287 (test for qcow2 compression type) and it's in bash.
So for now we can safely filter out compression type in all qcow2
tests.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/206.out| 10 +++---
 tests/qemu-iotests/242.out| 10 +++---
 tests/qemu-iotests/255.out|  8 ++---
 tests/qemu-iotests/274.out| 68 +--
 tests/qemu-iotests/280.out|  2 +-
 tests/qemu-iotests/iotests.py | 13 ++-
 6 files changed, 61 insertions(+), 50 deletions(-)

diff --git a/tests/qemu-iotests/206.out b/tests/qemu-iotests/206.out
index b68c443867..253209eca9 100644
--- a/tests/qemu-iotests/206.out
+++ b/tests/qemu-iotests/206.out
@@ -18,7 +18,7 @@ virtual size: 128 MiB (134217728 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 corrupt: false
@@ -42,7 +42,7 @@ virtual size: 64 MiB (67108864 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 corrupt: false
@@ -66,7 +66,7 @@ virtual size: 32 MiB (33554432 bytes)
 cluster_size: 2097152
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: true
 refcount bits: 1
 corrupt: false
@@ -92,7 +92,7 @@ backing file: TEST_IMG.base
 backing file format: IMGFMT
 Format specific information:
 compat: 0.10
-compression type: zlib
+compression type: COMPRESSION_TYPE
 refcount bits: 16
 
 === Successful image creation (encrypted) ===
@@ -109,7 +109,7 @@ encrypted: yes
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 encrypt:
diff --git a/tests/qemu-iotests/242.out b/tests/qemu-iotests/242.out
index 3759c99284..ce231424a7 100644
--- a/tests/qemu-iotests/242.out
+++ b/tests/qemu-iotests/242.out
@@ -12,7 +12,7 @@ virtual size: 1 MiB (1048576 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 refcount bits: 16
 corrupt: false
@@ -34,7 +34,7 @@ virtual size: 1 MiB (1048576 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 bitmaps:
 [0]:
@@ -68,7 +68,7 @@ virtual size: 1 MiB (1048576 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 bitmaps:
 [0]:
@@ -110,7 +110,7 @@ virtual size: 1 MiB (1048576 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 bitmaps:
 [0]:
@@ -161,7 +161,7 @@ virtual size: 1 MiB (1048576 bytes)
 cluster_size: 65536
 Format specific information:
 compat: 1.1
-compression type: zlib
+compression type: COMPRESSION_TYPE
 lazy refcounts: false
 bitmaps:
 [0]:
diff --git a/tests/qemu-iotests/255.out b/tests/qemu-iotests/255.out
index 33b7f22de3..bd3b13474e 100644
--- a/tests/qemu-iotests/255.out
+++ b/tests/qemu-iotests/255.out
@@ -3,9 +3,9 @@ Finishing a commit job with background reads
 
 === Create backing chain and start VM ===
 
-Formatting 'TEST_DIR/PID-t.qcow2.mid', fmt=qcow2 cluster_size=65536 
extended_l2=off compression_type=zlib size=134217728 lazy_refcounts=off 
refcount_bits=16
+Formatting 'TEST_DIR/PID-t.qcow2.mid', fmt=qcow2 cluster_size=65536 
extended_l2=off compression_type=COMPRESSION_TYPE size=134217728 
lazy_refcounts=off refcount_bits=16
 
-Formatting 'TEST_DIR/PID-t.qcow2', fmt=qcow2 cluster_size=65536 
extended_l2=off compression_type=zlib size=134217728 lazy_refcounts=off 
refcount_bits=16
+Formatting 'TEST_DIR/PID-t.qcow2', fmt=qcow2 cluster_size=65536 
extended_l2=off compression_type=COMPRESSION_TYPE size=134217728 
lazy_refcounts=off refcount_bits=16
 
 === Start background read requests ===
 
@@ -23,9 +23,9 @@ Closing the VM while a job is being cancelled
 
 === Create images and start VM ===
 
-Formatting 'TEST_DIR/PID-src.qcow2', fmt=qcow2 cluster_size=65536 
extended_l2=off compression_type=zlib size=134217728 lazy_refcounts=off 
refcount_bits=16
+Formatting 'TEST_DIR/PID-src.qcow2', fmt=qcow2 cluster_size=65536 
extended_l2=off compression_type=COMPRESSION_TYPE size=134217728 
lazy_refcounts=off refcount_bits=16
 
-Formatti

[PATCH 14/14] iotest 214: explicit compression type

[Vladimir Sementsov-Ogievskiy]

The test-case "Corrupted size field in compressed cluster descriptor"
heavily depends on zlib compression type. So, make it explicit. This
way test passes with IMGOPTS='compression_type=zstd'.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/214 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
index 0889089d81..c66e246ba2 100755
--- a/tests/qemu-iotests/214
+++ b/tests/qemu-iotests/214
@@ -51,7 +51,7 @@ echo
 # The L2 entries of the two compressed clusters are located at
 # 0x80 and 0x88, their original values are 0x400800a0
 # and 0x400800a00802 (5 sectors for compressed data each).
-_make_test_img 8M -o cluster_size=2M
+_make_test_img 8M -o cluster_size=2M,compression_type=zlib
 $QEMU_IO -c "write -c -P 0x11 0 2M" -c "write -c -P 0x11 2M 2M" "$TEST_IMG" \
  2>&1 | _filter_qemu_io | _filter_testdir
 
-- 
2.29.2

[PATCH 02/14] iotests.py: qemu_img*("create"): support IMGOPTS='compression_type=zstd'

[Vladimir Sementsov-Ogievskiy]

Adding support of IMGOPTS (like in bash tests) allows user to pass a
lot of different options. Still, some may require additional logic.

Now we want compression_type option, so add some smart logic around it:
ignore compression_type=zstd in IMGOPTS, if test want qcow2 in
compatibility mode. As well, ignore compression_type for non-qcow2
formats.

Note that we may instead add support only to qemu_img_create(), but
that works bad:

1. We'll have to update a lot of tests to use qemu_img_create instead
   of qemu_img('create'). (still, we may want do it anyway, but no
   reason to create a dependancy between task of supporting IMGOPTS and
   updating a lot of tests)

2. Some tests use qemu_img_pipe('create', ..) - even more work on
   updating

3. Even if we update all tests to go through qemu_img_create, we'll
   need a way to avoid creating new tests using qemu_img*('create') -
   add assertions.. That doesn't seem good.

So, let's add support of IMGOPTS to most generic
qemu_img_pipe_and_status().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/iotests.py | 48 ++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0d99dd841f..80f0cb4f42 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -16,6 +16,7 @@
 # along with this program.  If not, see .
 #
 
+import argparse
 import atexit
 import bz2
 from collections import OrderedDict
@@ -41,6 +42,19 @@
 from qemu.machine import qtest
 from qemu.qmp import QMPMessage
 
+
+def optstr2dict(opts: str) -> Dict[str, str]:
+if not opts:
+return {}
+
+return {arr[0]: arr[1] for arr in
+(opt.split('=', 1) for opt in opts.strip().split(','))}
+
+
+def dict2optstr(opts: Dict[str, str]) -> str:
+return ','.join(f'{k}={v}' for k, v in opts.items())
+
+
 # Use this logger for logging messages directly from the iotests module
 logger = logging.getLogger('qemu.iotests')
 logger.addHandler(logging.NullHandler())
@@ -57,6 +71,8 @@
 if os.environ.get('QEMU_IMG_OPTIONS'):
 qemu_img_args += os.environ['QEMU_IMG_OPTIONS'].strip().split(' ')
 
+imgopts = optstr2dict(os.environ.get('IMGOPTS', ''))
+
 qemu_io_args = [os.environ.get('QEMU_IO_PROG', 'qemu-io')]
 if os.environ.get('QEMU_IO_OPTIONS'):
 qemu_io_args += os.environ['QEMU_IO_OPTIONS'].strip().split(' ')
@@ -121,11 +137,41 @@ def qemu_tool_pipe_and_status(tool: str, args: 
Sequence[str],
{-subp.returncode}: {cmd}\n')
 return (output, subp.returncode)
 
+def qemu_img_create_prepare_args(args: List[str]) -> List[str]:
+if not args or args[0] != 'create':
+return list(args)
+args = args[1:]
+
+p = argparse.ArgumentParser(allow_abbrev=False)
+# -o option may be specified several times
+p.add_argument('-o', action='append', default=[])
+p.add_argument('-f')
+parsed, remaining = p.parse_known_args(args)
+
+opts = optstr2dict(','.join(parsed.o))
+
+compat = 'compat' in opts and opts['compat'][0] == '0'
+for k, v in imgopts.items():
+if k in opts:
+continue
+if k == 'compression_type' and (compat or parsed.f != 'qcow2'):
+continue
+opts[k] = v
+
+result = ['create']
+if parsed.f is not None:
+result += ['-f', parsed.f]
+if opts:
+result += ['-o', dict2optstr(opts)]
+result += remaining
+
+return result
+
 def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
 """
 Run qemu-img and return both its output and its exit code
 """
-full_args = qemu_img_args + list(args)
+full_args = qemu_img_args + qemu_img_create_prepare_args(list(args))
 return qemu_tool_pipe_and_status('qemu-img', full_args)
 
 def qemu_img(*args: str) -> int:
-- 
2.29.2

[PATCH 13/14] iotest 39: use _qcow2_dump_header

[Vladimir Sementsov-Ogievskiy]

_qcow2_dump_header has filter for compression type, so this change
makes test pass with IMGOPTS='compression_type=zstd'.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/039 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 8e783a8380..00d379cde2 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -142,7 +142,7 @@ $QEMU_IMG commit "$TEST_IMG"
 
 # The dirty bit must not be set
 _qcow2_dump_header | grep incompatible_features
-$PYTHON qcow2.py "$TEST_IMG".base dump-header | grep incompatible_features
+_qcow2_dump_header "$TEST_IMG".base | grep incompatible_features
 
 _check_test_img
 TEST_IMG="$TEST_IMG".base _check_test_img
-- 
2.29.2

[PATCH 01/14] iotests.py: img_info_log(): rename imgopts argument

[Vladimir Sementsov-Ogievskiy]

We are going to support IMGOPTS environment variable like in bash
tests. Corresponding global variable in iotests.py should be called
imgopts. So to not interfere with function argument, rename it in
advance.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/210| 8 
 tests/qemu-iotests/iotests.py | 5 +++--
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/210 b/tests/qemu-iotests/210
index 5a62ed4dd1..79b4967225 100755
--- a/tests/qemu-iotests/210
+++ b/tests/qemu-iotests/210
@@ -62,7 +62,7 @@ with iotests.FilePath('t.luks') as disk_path, \
 'driver=luks,file.driver=file,file.filename=%s,key-secret=keysec0' % 
(disk_path),
 filter_path=disk_path,
 extra_args=['--object', 'secret,id=keysec0,data=foo'],
-imgopts=True)
+use_image_opts=True)
 
 #
 # Successful image creation (with non-default options)
@@ -96,7 +96,7 @@ with iotests.FilePath('t.luks') as disk_path, \
 'driver=luks,file.driver=file,file.filename=%s,key-secret=keysec0' % 
(disk_path),
 filter_path=disk_path,
 extra_args=['--object', 'secret,id=keysec0,data=foo'],
-imgopts=True)
+use_image_opts=True)
 
 #
 # Invalid BlockdevRef
@@ -132,7 +132,7 @@ with iotests.FilePath('t.luks') as disk_path, \
 'driver=luks,file.driver=file,file.filename=%s,key-secret=keysec0' % 
(disk_path),
 filter_path=disk_path,
 extra_args=['--object', 'secret,id=keysec0,data=foo'],
-imgopts=True)
+use_image_opts=True)
 
 #
 # Invalid sizes
@@ -176,4 +176,4 @@ with iotests.FilePath('t.luks') as disk_path, \
 'driver=luks,file.driver=file,file.filename=%s,key-secret=keysec0' % 
(disk_path),
 filter_path=disk_path,
 extra_args=['--object', 'secret,id=keysec0,data=foo'],
-imgopts=True)
+use_image_opts=True)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 89663dac06..0d99dd841f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -187,9 +187,10 @@ def qemu_img_log(*args):
 log(result, filters=[filter_testfiles])
 return result
 
-def img_info_log(filename, filter_path=None, imgopts=False, extra_args=()):
+def img_info_log(filename, filter_path=None, use_image_opts=False,
+ extra_args=()):
 args = ['info']
-if imgopts:
+if use_image_opts:
 args.append('--image-opts')
 else:
 args += ['-f', imgfmt]
-- 
2.29.2

[PATCH 00/14] iotests: support zstd

[Vladimir Sementsov-Ogievskiy]

Hi all!

These series makes all test pass with

   IMGOPTS='compression_type=zstd'

Also, python iotests start to support IMGOPTS (they didn't before).

Also, tests works if enable compression type zstd by default. There is
no such config option currently, probably it will appear in future or
we'll go some another way (like external config file, like
/etc/qemu.conf). For now you may test with a simple patch like:

--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3539,6 +3539,11 @@ qcow2_co_create(BlockdevCreateOptions 
*create_options, Error **errp)
 goto out;
 }
 }
+
+if (!qcow2_opts->has_compression_type && version >= 3) {
+qcow2_opts->has_compression_type = true;
+qcow2_opts->compression_type = QCOW2_COMPRESSION_TYPE_ZSTD;
+}
 
 if (qcow2_opts->has_compression_type &&
 qcow2_opts->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB) {

We want to use zstd compression type by default in Virtuozzo 8. This is
the first step, which is good anyway: improve iotests.

Vladimir Sementsov-Ogievskiy (14):
  iotests.py: img_info_log(): rename imgopts argument
  iotests.py: qemu_img*("create"): support
IMGOPTS='compression_type=zstd'
  iotest 303: explicit compression type
  iotest 065: explicit compression type
  iotests.py: filter compression type out
  iotest 302: use img_info_log() helper
  qcow2: simple case support for downgrading of qcow2 images with zstd
  iotests/common.rc: _make_test_img(): smarter compressiont_type
handling
  iotests/common.rc: introduce _qcow2_dump_header helper
  iotests: massive use _qcow2_dump_header
  iotests: bash tests: filter compression type
  iotests 60: more accurate set dirty bit in qcow2 header
  iotest 39: use _qcow2_dump_header
  iotest 214: explicit compression type

 block/qcow2.c| 58 ++-
 tests/qemu-iotests/031   |  6 +--
 tests/qemu-iotests/036   |  6 +--
 tests/qemu-iotests/039   | 22 +--
 tests/qemu-iotests/060   | 22 +--
 tests/qemu-iotests/060.out   |  2 +-
 tests/qemu-iotests/061   | 36 -
 tests/qemu-iotests/061.out   | 12 +++---
 tests/qemu-iotests/065   | 14 +++
 tests/qemu-iotests/082.out   | 14 +++
 tests/qemu-iotests/137   |  2 +-
 tests/qemu-iotests/198.out   |  4 +-
 tests/qemu-iotests/206.out   | 10 ++---
 tests/qemu-iotests/210   |  8 ++--
 tests/qemu-iotests/214   |  2 +-
 tests/qemu-iotests/242.out   | 10 ++---
 tests/qemu-iotests/255.out   |  8 ++--
 tests/qemu-iotests/274.out   | 68 
 tests/qemu-iotests/280.out   |  2 +-
 tests/qemu-iotests/287   |  8 ++--
 tests/qemu-iotests/302   |  3 +-
 tests/qemu-iotests/302.out   |  7 ++--
 tests/qemu-iotests/303   | 25 +++-
 tests/qemu-iotests/303.out   | 30 +-
 tests/qemu-iotests/common.filter |  7 
 tests/qemu-iotests/common.rc | 30 ++
 tests/qemu-iotests/iotests.py| 66 +--
 27 files changed, 333 insertions(+), 149 deletions(-)

-- 
2.29.2

[PATCH 04/14] iotest 065: explicit compression type

[Vladimir Sementsov-Ogievskiy]

The test checks different options. It of course fails if set
IMGOPTS='compression_type=zstd'. So, let's be explicit in what
compression type we want and independent of IMGOPTS. Test both existing
compression types.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/065 | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index 3c2ca27627..22203ed480 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -96,17 +96,17 @@ class TestQCow2(TestQemuImgInfo):
 
 class TestQCow3NotLazy(TestQemuImgInfo):
 '''Testing a qcow2 version 3 image with lazy refcounts disabled'''
-img_options = 'compat=1.1,lazy_refcounts=off'
+img_options = 'compat=1.1,lazy_refcounts=off,compression_type=zstd'
 json_compare = { 'compat': '1.1', 'lazy-refcounts': False,
  'refcount-bits': 16, 'corrupt': False,
- 'compression-type': 'zlib', 'extended-l2': False }
-human_compare = [ 'compat: 1.1', 'compression type: zlib',
+ 'compression-type': 'zstd', 'extended-l2': False }
+human_compare = [ 'compat: 1.1', 'compression type: zstd',
   'lazy refcounts: false', 'refcount bits: 16',
   'corrupt: false', 'extended l2: false' ]
 
 class TestQCow3Lazy(TestQemuImgInfo):
 '''Testing a qcow2 version 3 image with lazy refcounts enabled'''
-img_options = 'compat=1.1,lazy_refcounts=on'
+img_options = 'compat=1.1,lazy_refcounts=on,compression_type=zlib'
 json_compare = { 'compat': '1.1', 'lazy-refcounts': True,
  'refcount-bits': 16, 'corrupt': False,
  'compression-type': 'zlib', 'extended-l2': False }
@@ -117,7 +117,7 @@ class TestQCow3Lazy(TestQemuImgInfo):
 class TestQCow3NotLazyQMP(TestQMP):
 '''Testing a qcow2 version 3 image with lazy refcounts disabled, opening
with lazy refcounts enabled'''
-img_options = 'compat=1.1,lazy_refcounts=off'
+img_options = 'compat=1.1,lazy_refcounts=off,compression_type=zlib'
 qemu_options = 'lazy-refcounts=on'
 compare = { 'compat': '1.1', 'lazy-refcounts': False,
 'refcount-bits': 16, 'corrupt': False,
@@ -127,11 +127,11 @@ class TestQCow3NotLazyQMP(TestQMP):
 class TestQCow3LazyQMP(TestQMP):
 '''Testing a qcow2 version 3 image with lazy refcounts enabled, opening
with lazy refcounts disabled'''
-img_options = 'compat=1.1,lazy_refcounts=on'
+img_options = 'compat=1.1,lazy_refcounts=on,compression_type=zstd'
 qemu_options = 'lazy-refcounts=off'
 compare = { 'compat': '1.1', 'lazy-refcounts': True,
 'refcount-bits': 16, 'corrupt': False,
-'compression-type': 'zlib', 'extended-l2': False }
+'compression-type': 'zstd', 'extended-l2': False }
 
 TestImageInfoSpecific = None
 TestQemuImgInfo = None
-- 
2.29.2

Re: [PATCH 3/9] tests/acceptance: Tag NetBSD tests as 'os:netbsd'

[Philippe Mathieu-Daudé]

Hi Niek,

On 7/4/21 2:35 PM, Niek Linnenbank wrote:
> for test_arm_orangepi_uboot_netbsd9:
> 
> Reviewed-by: Niek Linnenbank  >

Thanks for the review. Does your R-b tag applies for this single
patch or all patches related to test_arm_orangepi_uboot_netbsd9
in this series (3-5)?

Re: [PATCH v2] docs: document file-posix locking protocol

[Denis V. Lunev]

On 7/5/21 10:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> 03.07.2021 17:50, Nir Soffer wrote:
>> On Sat, Jul 3, 2021 at 4:51 PM Vladimir Sementsov-Ogievskiy
>>  wrote:
>>>
>>> Let's document how we use file locks in file-posix driver, to allow
>>> external programs to "communicate" in this way with Qemu.
>>
>> This makes the locking implementation public, so qemu can never change
>> it without breaking external programs. I'm not sure this is an issue
>> since
>> even now qemu cannot change without breaking compatibility with older
>> qemu versions.
>
> Yes, that's my thought too. I think, that's enough to say that we
> actually have "public" protocol, just undocumented.
>
> Note, that breaking that compatibility may break shared migration, and
> migration without one host (which may be used for live upgrade of qemu).
>
>>
>> Maybe a better way to integrate with external programs is to provide
>> a library/tool to perform locking?
>>
>> For example we can have tool like:
>>
>>     qemu-img lock [how] image command
>>
>> This example will take the lock specified by "how" on image while
>> "command"
>> is running.
>
> Having a parallel process, that takes locks "for us" is a pain. At
> least we should handle unexpected death of that process. Some
> filesystems may not allow opening file in two processes in some
> circumstances. And it just breaks normal operation with file locks:
> lock should be taken by the process that use it.
>
> Library has GPL limitation of use.

and there are also some important consequences. 3rd party implements
QCOW2 support in a 3rd party way. Thus it opens the image and creates
3rd party data structures for it.

It handles metadata processing etc. Thus once the "locking" library will
be ready to operate, some bits indicating that the image is in use would
be on the filesystem, f.e. "busy" state and thus we would need to perform
the "locking" in QEMU code through a very specific QEMU data structure
creation. The library could do locking first, but in that case 3rd party
code would have same problems.

In general, this is not only QCOW2 problem, such locking is a problem
for all supported formats and thus we come to the dilemma:
- to document
- or to provide an utility
In that case we should provide locking for all "alien" formats, which
is looking overcomplicated at my opinion.

In general, the format itself is opened with providing the information
for all 3rd parties to integrate with. The locking is an important part
of interoperability and thus should be published too.

Den

Re: [PATCH resend] nbd: register yank function earlier

[Vladimir Sementsov-Ogievskiy]

04.07.2021 13:56, Lukas Straub wrote:

Although unlikely, qemu might hang in nbd_send_request().

Allow recovery in this case by registering the yank function before
calling it.

Signed-off-by: Lukas Straub



Reviewed-by: Vladimir Sementsov-Ogievskiy 


--
Best regards,
Vladimir

Re: [PATCH] nbd: register yank function earlier

[Vladimir Sementsov-Ogievskiy]

04.07.2021 01:07, Lukas Straub wrote:

Although unlikely, qemu might hang in nbd_send_request().

Allow recovery in this case by registering the yank function before
calling it.

Signed-off-by: Lukas Straub


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH] hw/sd: sdhci: Enable 64-bit system bus capability in the default SD/MMC host controller

[Bin Meng]

On Thu, Jun 24, 2021 at 3:01 AM Joanne Koong  wrote:
>
> The default SD/MMC host controller uses SD spec v2.00. 64-bit system bus 
> capability
> was added in v2.
>
> In this change, we arrive at 0x157834b4 by computing (0x057834b4 | (1ul << 
> 28))
> where 28 represents the BUS64BIT SDHC_CAPAB field.
>
> Signed-off-by: Joanne Koong 
> ---
>  hw/sd/sdhci-internal.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng 

Re: [PATCH v2] docs: document file-posix locking protocol

[Vladimir Sementsov-Ogievskiy]

03.07.2021 17:50, Nir Soffer wrote:

On Sat, Jul 3, 2021 at 4:51 PM Vladimir Sementsov-Ogievskiy
 wrote:


Let's document how we use file locks in file-posix driver, to allow
external programs to "communicate" in this way with Qemu.


This makes the locking implementation public, so qemu can never change
it without breaking external programs. I'm not sure this is an issue since
even now qemu cannot change without breaking compatibility with older
qemu versions.


Yes, that's my thought too. I think, that's enough to say that we actually have 
"public" protocol, just undocumented.

Note, that breaking that compatibility may break shared migration, and 
migration without one host (which may be used for live upgrade of qemu).



Maybe a better way to integrate with external programs is to provide
a library/tool to perform locking?

For example we can have tool like:

qemu-img lock [how] image command

This example will take the lock specified by "how" on image while "command"
is running.


Having a parallel process, that takes locks "for us" is a pain. At least we 
should handle unexpected death of that process. Some filesystems may not allow opening 
file in two processes in some circumstances. And it just breaks normal operation with 
file locks: lock should be taken by the process that use it.

Library has GPL limitation of use.




Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

v2: improve some descriptions
 add examples
 add notice about old bad POSIX file locks

  docs/system/qemu-block-drivers.rst.inc | 186 +
  1 file changed, 186 insertions(+)

diff --git a/docs/system/qemu-block-drivers.rst.inc 
b/docs/system/qemu-block-drivers.rst.inc
index 16225710eb..74fb71600d 100644
--- a/docs/system/qemu-block-drivers.rst.inc
+++ b/docs/system/qemu-block-drivers.rst.inc
@@ -909,3 +909,189 @@ some additional tasks, hooking io requests.
.. option:: prealloc-size

  How much to preallocate (in bytes), default 128M.
+
+Image locking protocol
+~~
+
+QEMU holds rd locks and never rw locks. Instead, GETLK fcntl is used with 
F_WRLCK
+to handle permissions as described below.
+QEMU process may rd-lock the following bytes of the image with corresponding
+meaning:
+
+Permission bytes. If permission byte is rd-locked, it means that some process
+uses corresponding permission on that file.
+
+ByteOperation
+100 read
+  Lock holder can read
+101 write
+  Lock holder can write
+102 write-unchanged
+  Lock holder can write same data if it sure, that this write doesn't
+  break concurrent readers. This is mostly used internally in Qemu
+  and it wouldn't be good idea to exploit it somehow.
+103 resize
+  Lock holder can resize the file. "write" permission is also required
+  for resizing, so lock byte 103 only if you also lock byte 101.
+104 graph-mod
+  Undefined. QEMU may sometimes locks this byte, but external programs
+  should not. QEMU will stop locking this byte in future
+
+Unshare bytes. If permission byte is rd-locked, it means that some process
+does not allow the others use corresponding options on that file.
+
+ByteOperation
+200 read
+  Lock holder don't allow read operation to other processes.
+201 write
+  Lock holder don't allow write operation to other processes. This
+  still allows others to do write-uncahnged operations. Better not
+  exploit outside of Qemu.
+202 write-unchanged
+  Lock holder don't allow write-unchanged operation to other processes.
+203 resize
+  Lock holder don't allow resizing the file by other processes.
+204 graph-mod
+  Undefined. QEMU may sometimes locks this byte, but external programs
+  should not. QEMU will stop locking this byte in future
+
+Handling the permissions works as follows: assume we want to open the file to 
do
+some operations and in the same time want to disallow some operation to other
+processes. So, we want to lock some of the bytes described above. We operate as
+follows:
+
+1. rd-lock all needed bytes, both "permission" bytes and "unshare" bytes.
+
+2. For each "unshare" byte we rd-locked, do GETLK that "tries" to wr-lock
+corresponding "permission" byte. So, we check is there any other process that
+uses the permission we want to unshare. If it exists we fail.
+
+3. For each "permission" byte we rd-locked, do GETLK that "tries" to wr-lock
+corresponding "unshare" byte. So, we check is there any other process that
+unshares the permission we want to have. If it exists we fail.
+
+Important notice: Qemu may fallback to POSIX file locks only if OFD locks
+unavailable. Other programs should behave similarly: use POSIX file locks
+only if OFD locks unavailable and if you are OK with drawbacks of POSIX
+file locks (for example, they are lost on close() of any file descriptor
+for that file).


Worth an example.


+

Re: [PATCH 3/3] hw/sd: Check for valid address range in SEND_WRITE_PROT (CMD30)

[Bin Meng]

On Fri, Jul 2, 2021 at 11:59 PM Philippe Mathieu-Daudé  wrote:
>
> OSS-Fuzz found sending illegal addresses when querying the write
> protection bits triggers an assertion:
>
>   qemu-fuzz-i386: hw/sd/sd.c:824: uint32_t sd_wpbits(SDState *, uint64_t): 
> Assertion `wpnum < sd->wpgrps_size' failed.
>   ==11578== ERROR: libFuzzer: deadly signal
>   #8 0x7628e091 in __assert_fail
>   #9 0x588f1a3c in sd_wpbits hw/sd/sd.c:824:9
>   #10 0x588dd271 in sd_normal_command hw/sd/sd.c:1383:38
>   #11 0x588d777c in sd_do_command hw/sd/sd.c
>   #12 0x58cb25a0 in sdbus_do_command hw/sd/core.c:100:16
>   #13 0x58e02a9a in sdhci_send_command hw/sd/sdhci.c:337:12
>   #14 0x58dffa46 in sdhci_write hw/sd/sdhci.c:1187:9
>   #15 0x598b9d76 in memory_region_write_accessor softmmu/memory.c:489:5
>
> Similarly to commit 8573378e62d ("hw/sd: fix out-of-bounds check
> for multi block reads"), check the address range before sending
> the status of the write protection bits.
>
> Include the qtest reproducer provided by Alexander Bulekov:
>
>   $ make check-qtest-i386
>   ...
>   Running test qtest-i386/fuzz-sdcard-test
>   qemu-system-i386: ../hw/sd/sd.c:824: sd_wpbits: Assertion `wpnum < 
> sd->wpgrps_size' failed.
>
> Reported-by: OSS-Fuzz (Issue 29225)
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/450
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/sd/sd.c |  5 +++
>  tests/qtest/fuzz-sdcard-test.c | 66 ++
>  MAINTAINERS|  3 +-
>  tests/qtest/meson.build|  1 +
>  4 files changed, 74 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qtest/fuzz-sdcard-test.c
>

Reviewed-by: Bin Meng 

[PATCH v8 15/16] qemu-iotests: add option to show qemu binary logs on stdout

[Emanuele Giuseppe Esposito]

Using the flag -p, allow the qemu binary to print to stdout.

Also create the common function _close_qemu_log_file() to
avoid accessing machine.py private fields directly and have
duplicate code.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 python/qemu/machine/machine.py | 9 ++---
 tests/qemu-iotests/check   | 4 +++-
 tests/qemu-iotests/iotests.py  | 8 
 tests/qemu-iotests/testenv.py  | 9 +++--
 4 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py
index fdf2fc0e9c..c9d344d955 100644
--- a/python/qemu/machine/machine.py
+++ b/python/qemu/machine/machine.py
@@ -338,6 +338,11 @@ def _post_launch(self) -> None:
 if self._qmp_connection:
 self._qmp.accept(self._qmp_timer)
 
+def _close_qemu_log_file(self) -> None:
+if self._qemu_log_file is not None:
+self._qemu_log_file.close()
+self._qemu_log_file = None
+
 def _post_shutdown(self) -> None:
 """
 Called to cleanup the VM instance after the process has exited.
@@ -350,9 +355,7 @@ def _post_shutdown(self) -> None:
 self._qmp.close()
 self._qmp_connection = None
 
-if self._qemu_log_file is not None:
-self._qemu_log_file.close()
-self._qemu_log_file = None
+self._close_qemu_log_file()
 
 self._load_io_log()
 
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index ebd27946db..da1bfb839e 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -36,6 +36,8 @@ def make_argparser() -> argparse.ArgumentParser:
help='pretty print output for make check')
 
 p.add_argument('-d', dest='debug', action='store_true', help='debug')
+p.add_argument('-p', dest='print', action='store_true',
+help='redirects qemu\'s stdout and stderr to the test output')
 p.add_argument('-gdb', action='store_true',
help="start gdbserver with $GDB_OPTIONS options \
 ('localhost:12345' if $GDB_OPTIONS is empty)")
@@ -119,7 +121,7 @@ if __name__ == '__main__':
   aiomode=args.aiomode, cachemode=args.cachemode,
   imgopts=args.imgopts, misalign=args.misalign,
   debug=args.debug, valgrind=args.valgrind,
-  gdb=args.gdb)
+  gdb=args.gdb, qprint=args.print)
 
 if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
 if not args.tests:
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 74fa56840d..2cf5ff965b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -79,6 +79,8 @@
 if gdb_qemu_env:
 qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ')
 
+qemu_print = os.environ.get('PRINT_QEMU', False)
+
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
 output_dir = os.environ.get('OUTPUT_DIR', '.')
@@ -613,6 +615,12 @@ def _post_shutdown(self) -> None:
 else:
 os.remove(valgrind_filename)
 
+def _pre_launch(self) -> None:
+super()._pre_launch()
+if qemu_print:
+# set QEMU binary output to stdout
+self._close_qemu_log_file()
+
 def add_object(self, opts):
 self._args.append('-object')
 self._args.append(opts)
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 8bf154376f..70da0d60c8 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -74,7 +74,7 @@ class TestEnv(ContextManager['TestEnv']):
  'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
  'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
  'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
- 'GDB_OPTIONS']
+ 'GDB_OPTIONS', 'PRINT_QEMU']
 
 def prepare_subprocess(self, args: List[str]) -> Dict[str, str]:
 if self.debug:
@@ -181,7 +181,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
  misalign: bool = False,
  debug: bool = False,
  valgrind: bool = False,
- gdb: bool = False) -> None:
+ gdb: bool = False,
+ qprint: bool = False) -> None:
 self.imgfmt = imgfmt
 self.imgproto = imgproto
 self.aiomode = aiomode
@@ -189,6 +190,9 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
 self.misalign = misalign
 self.debug = debug
 
+if qprint:
+self.print_qemu = 'y'
+
 if gdb:
 self.gdb_options = os.getenv('GDB_OPTIONS', DEF_GDB_OPTIONS)
 if not self.gdb_options:
@@ -299,6 +303,7 @@ def print_env(self) -> None:
 SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
 GDB_OPTIONS   -- {GDB_OPTIONS}

[PATCH v8 16/16] docs/devel/testing: add -p option to the debug section of QEMU iotests

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 docs/devel/testing.rst | 4 
 1 file changed, 4 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 719accdb1e..e5311cb167 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -249,6 +249,10 @@ a failing test:
 * ``-d`` (debug) just increases the logging verbosity, showing
   for example the QMP commands and answers.
 
+* ``-p`` (print) redirect QEMU’s stdout and stderr to the test output,
+  instead of saving it into a log file in
+  ``$TEST_DIR/qemu-machine-``.
+
 Test case groups
 
 
-- 
2.31.1

[PATCH v8 14/16] docs/devel/testing: add -valgrind option to the debug section of QEMU iotests

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 docs/devel/testing.rst | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 8b24e6fb47..719accdb1e 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -240,6 +240,12 @@ a failing test:
   If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored,
   regardless on whether it is set or not.
 
+* ``-valgrind`` attaches a valgrind instance to QEMU. If it detects
+  warnings, it will print and save the log in
+  ``$TEST_DIR/.valgrind``.
+  The final command line will be ``valgrind --log-file=$TEST_DIR/
+  .valgrind --error-exitcode=99 $QEMU ...``
+
 * ``-d`` (debug) just increases the logging verbosity, showing
   for example the QMP commands and answers.
 
-- 
2.31.1

[PATCH v8 11/16] qemu-iotests: extend QMP socket timeout when using valgrind

[Emanuele Giuseppe Esposito]

As with gdbserver, valgrind delays the test execution, so
the default QMP socket timeout and the generic class
Timeout in iotests.py timeouts too soon.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-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 6aa1dc48ba..26c580f9e7 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -488,13 +488,13 @@ def __init__(self, seconds, errmsg="Timeout"):
 self.seconds = seconds
 self.errmsg = errmsg
 def __enter__(self):
-if qemu_gdb:
+if qemu_gdb or qemu_valgrind:
 return self
 signal.signal(signal.SIGALRM, self.timeout)
 signal.setitimer(signal.ITIMER_REAL, self.seconds)
 return self
 def __exit__(self, exc_type, value, traceback):
-if qemu_gdb:
+if qemu_gdb or qemu_valgrind:
 return False
 signal.setitimer(signal.ITIMER_REAL, 0)
 return False
@@ -590,7 +590,7 @@ class VM(qtest.QEMUQtestMachine):
 
 def __init__(self, path_suffix=''):
 name = "qemu%s-%d" % (path_suffix, os.getpid())
-timer = 15.0 if not qemu_gdb else None
+timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None
 super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
  name=name,
  base_temp_dir=test_dir,
-- 
2.31.1

[PATCH v8 10/16] qemu-iotests: extend the check script to prepare supporting valgrind for python tests

[Emanuele Giuseppe Esposito]

Currently, the check script only parses the option and sets the
VALGRIND_QEMU environmental variable to "y".
Add another local python variable that prepares the command line,
identical to the one provided in the test scripts.

Because the python script does not know in advance the valgrind
PID to assign to the log file name, use the "%p" flag in valgrind
log file name that automatically puts the process PID at runtime.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/check  |  7 ---
 tests/qemu-iotests/iotests.py | 11 +++
 tests/qemu-iotests/testenv.py |  1 +
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 4365bb8066..ebd27946db 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -39,6 +39,10 @@ def make_argparser() -> argparse.ArgumentParser:
 p.add_argument('-gdb', action='store_true',
help="start gdbserver with $GDB_OPTIONS options \
 ('localhost:12345' if $GDB_OPTIONS is empty)")
+p.add_argument('-valgrind', action='store_true',
+help='use valgrind, sets VALGRIND_QEMU environment '
+'variable')
+
 p.add_argument('-misalign', action='store_true',
help='misalign memory allocations')
 p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -88,9 +92,6 @@ def make_argparser() -> argparse.ArgumentParser:
 g_bash.add_argument('-o', dest='imgopts',
 help='options to pass to qemu-img create/convert, '
 'sets IMGOPTS environment variable')
-g_bash.add_argument('-valgrind', action='store_true',
-help='use valgrind, sets VALGRIND_QEMU environment '
-'variable')
 
 g_sel = p.add_argument_group('test selecting options',
  'The following options specify test set '
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e7e3d92d3e..6aa1dc48ba 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -96,6 +96,17 @@
 sys.stderr.write('Please run this test via the "check" script\n')
 sys.exit(os.EX_USAGE)
 
+qemu_valgrind = []
+if os.environ.get('VALGRIND_QEMU') == "y" and \
+os.environ.get('NO_VALGRIND') != "y":
+valgrind_logfile = "--log-file=" + test_dir
+# %p allows to put the valgrind process PID, since
+# we don't know it a priori (subprocess.Popen is
+# not yet invoked)
+valgrind_logfile += "/%p.valgrind"
+
+qemu_valgrind = ['valgrind', valgrind_logfile, '--error-exitcode=99']
+
 socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
 
 luks_default_secret_object = 'secret,id=keysec0,data=' + \
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 8501c6caf5..8bf154376f 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -298,6 +298,7 @@ def print_env(self) -> None:
 SOCK_DIR  -- {SOCK_DIR}
 SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
 GDB_OPTIONS   -- {GDB_OPTIONS}
+VALGRIND_QEMU -- {VALGRIND_QEMU}
 """
 
 args = collections.defaultdict(str, self.get_env())
-- 
2.31.1

[PATCH v8 07/16] qemu_iotests: insert gdbserver command line as wrapper for qemu binary

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/iotests.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e176a84620..e7e3d92d3e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -580,7 +580,8 @@ class VM(qtest.QEMUQtestMachine):
 def __init__(self, path_suffix=''):
 name = "qemu%s-%d" % (path_suffix, os.getpid())
 timer = 15.0 if not qemu_gdb else None
-super().__init__(qemu_prog, qemu_opts, name=name,
+super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
+ name=name,
  base_temp_dir=test_dir,
  socket_scm_helper=socket_scm_helper,
  sock_dir=sock_dir, qmp_timer=timer)
-- 
2.31.1

[PATCH v8 13/16] qemu-iotests: insert valgrind command line as wrapper for qemu binary

[Emanuele Giuseppe Esposito]

If -gdb and -valgrind are both defined, return an error.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/iotests.py | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 85d8c0abbb..74fa56840d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -591,7 +591,11 @@ class VM(qtest.QEMUQtestMachine):
 def __init__(self, path_suffix=''):
 name = "qemu%s-%d" % (path_suffix, os.getpid())
 timer = 15.0 if not (qemu_gdb or qemu_valgrind) else None
-super().__init__(qemu_prog, qemu_opts, wrapper=qemu_gdb,
+if qemu_gdb and qemu_valgrind:
+sys.stderr.write('gdb and valgrind are mutually exclusive\n')
+sys.exit(1)
+wrapper = qemu_gdb if qemu_gdb else qemu_valgrind
+super().__init__(qemu_prog, qemu_opts, wrapper=wrapper,
  name=name,
  base_temp_dir=test_dir,
  socket_scm_helper=socket_scm_helper,
-- 
2.31.1

[PATCH v8 05/16] qemu-iotests: add option to attach gdbserver

[Emanuele Giuseppe Esposito]

Define -gdb flag and GDB_OPTIONS environment variable
to python tests to attach a gdbserver to each qemu instance.
This patch only adds and parses this flag, it does not yet add
the implementation for it.

if -gdb is not provided but $GDB_OPTIONS is set, ignore the
environment variable.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/check  |  6 +-
 tests/qemu-iotests/iotests.py |  5 +
 tests/qemu-iotests/testenv.py | 17 +++--
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 2dd529eb75..4365bb8066 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -36,6 +36,9 @@ def make_argparser() -> argparse.ArgumentParser:
help='pretty print output for make check')
 
 p.add_argument('-d', dest='debug', action='store_true', help='debug')
+p.add_argument('-gdb', action='store_true',
+   help="start gdbserver with $GDB_OPTIONS options \
+('localhost:12345' if $GDB_OPTIONS is empty)")
 p.add_argument('-misalign', action='store_true',
help='misalign memory allocations')
 p.add_argument('--color', choices=['on', 'off', 'auto'],
@@ -114,7 +117,8 @@ if __name__ == '__main__':
 env = TestEnv(imgfmt=args.imgfmt, imgproto=args.imgproto,
   aiomode=args.aiomode, cachemode=args.cachemode,
   imgopts=args.imgopts, misalign=args.misalign,
-  debug=args.debug, valgrind=args.valgrind)
+  debug=args.debug, valgrind=args.valgrind,
+  gdb=args.gdb)
 
 if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
 if not args.tests:
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6b0db4ce54..c86f239d81 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -74,6 +74,11 @@
 qemu_prog = os.environ.get('QEMU_PROG', 'qemu')
 qemu_opts = os.environ.get('QEMU_OPTIONS', '').strip().split(' ')
 
+gdb_qemu_env = os.environ.get('GDB_OPTIONS')
+qemu_gdb = []
+if gdb_qemu_env:
+qemu_gdb = ['gdbserver'] + gdb_qemu_env.strip().split(' ')
+
 imgfmt = os.environ.get('IMGFMT', 'raw')
 imgproto = os.environ.get('IMGPROTO', 'file')
 output_dir = os.environ.get('OUTPUT_DIR', '.')
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 0c3fe75636..8501c6caf5 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -27,6 +27,7 @@
 import glob
 from typing import List, Dict, Any, Optional, ContextManager
 
+DEF_GDB_OPTIONS = 'localhost:12345'
 
 def isxfile(path: str) -> bool:
 return os.path.isfile(path) and os.access(path, os.X_OK)
@@ -72,7 +73,8 @@ class TestEnv(ContextManager['TestEnv']):
  'QEMU_NBD_OPTIONS', 'IMGOPTS', 'IMGFMT', 'IMGPROTO',
  'AIOMODE', 'CACHEMODE', 'VALGRIND_QEMU',
  'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
- 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_']
+ 'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_',
+ 'GDB_OPTIONS']
 
 def prepare_subprocess(self, args: List[str]) -> Dict[str, str]:
 if self.debug:
@@ -178,7 +180,8 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: str,
  imgopts: Optional[str] = None,
  misalign: bool = False,
  debug: bool = False,
- valgrind: bool = False) -> None:
+ valgrind: bool = False,
+ gdb: bool = False) -> None:
 self.imgfmt = imgfmt
 self.imgproto = imgproto
 self.aiomode = aiomode
@@ -186,6 +189,15 @@ def __init__(self, imgfmt: str, imgproto: str, aiomode: 
str,
 self.misalign = misalign
 self.debug = debug
 
+if gdb:
+self.gdb_options = os.getenv('GDB_OPTIONS', DEF_GDB_OPTIONS)
+if not self.gdb_options:
+# cover the case 'export GDB_OPTIONS='
+self.gdb_options = DEF_GDB_OPTIONS
+elif 'GDB_OPTIONS' in os.environ:
+# to not propagate it in prepare_subprocess()
+del os.environ['GDB_OPTIONS']
+
 if valgrind:
 self.valgrind_qemu = 'y'
 
@@ -285,6 +297,7 @@ def print_env(self) -> None:
 TEST_DIR  -- {TEST_DIR}
 SOCK_DIR  -- {SOCK_DIR}
 SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
+GDB_OPTIONS   -- {GDB_OPTIONS}
 """
 
 args = collections.defaultdict(str, self.get_env())
-- 
2.31.1

[PATCH v8 04/16] docs/devel/testing: add debug section to the QEMU iotests chapter

[Emanuele Giuseppe Esposito]

Introduce the "Debugging a test case" section, in preparation
to the additional flags that will be added in the next patches.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 docs/devel/testing.rst | 8 
 1 file changed, 8 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 4e42392810..9d6a8f8636 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -224,6 +224,14 @@ another application on the host may have locked the file, 
possibly leading to a
 test failure.  If using such devices are explicitly desired, consider adding
 ``locking=off`` option to disable image locking.
 
+Debugging a test case
+---
+The following options to the ``check`` script can be useful when debugging
+a failing test:
+
+* ``-d`` (debug) just increases the logging verbosity, showing
+  for example the QMP commands and answers.
+
 Test case groups
 
 
-- 
2.31.1

[PATCH v8 08/16] qemu-iotests: add gdbserver option to script tests too

[Emanuele Giuseppe Esposito]

Remove read timer in test script when GDB_OPTIONS are set,
so that the bash tests won't timeout while running gdb.

The only limitation here is that running a script with gdbserver
will make the test output mismatch with the expected
results, making the test fail.

Signed-off-by: Emanuele Giuseppe Esposito 
---
 tests/qemu-iotests/common.qemu | 7 ++-
 tests/qemu-iotests/common.rc   | 8 +++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 0fc52d20d7..cbca757b49 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -85,7 +85,12 @@ _timed_wait_for()
 timeout=yes
 
 QEMU_STATUS[$h]=0
-while IFS= read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
+read_timeout="-t ${QEMU_COMM_TIMEOUT}"
+if [ ! -z ${GDB_OPTIONS} ]; then
+read_timeout=
+fi
+
+while IFS= read ${read_timeout} resp <&${QEMU_OUT[$h]}
 do
 if [ -n "$capture_events" ]; then
 capture=0
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index cbbf6d7c7f..a1ef2b5c2f 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -166,8 +166,14 @@ _qemu_wrapper()
 if [ -n "${QEMU_NEED_PID}" ]; then
 echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
 fi
+
+GDB=""
+if [ ! -z ${GDB_OPTIONS} ]; then
+GDB="gdbserver ${GDB_OPTIONS}"
+fi
+
 VALGRIND_QEMU="${VALGRIND_QEMU_VM}" _qemu_proc_exec 
"${VALGRIND_LOGFILE}" \
-"$QEMU_PROG" $QEMU_OPTIONS "$@"
+$GDB "$QEMU_PROG" $QEMU_OPTIONS "$@"
 )
 RETVAL=$?
 _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
-- 
2.31.1

[PATCH v8 12/16] qemu-iotests: allow valgrind to read/delete the generated log file

[Emanuele Giuseppe Esposito]

When using -valgrind on the script tests, it generates a log file
in $TEST_DIR that is either read (if valgrind finds problems) or
otherwise deleted. Provide the same exact behavior when using
-valgrind on the python tests.

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/iotests.py | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 26c580f9e7..85d8c0abbb 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -598,6 +598,17 @@ def __init__(self, path_suffix=''):
  sock_dir=sock_dir, qmp_timer=timer)
 self._num_drives = 0
 
+def _post_shutdown(self) -> None:
+super()._post_shutdown()
+if not qemu_valgrind or not self._popen:
+return
+valgrind_filename =  f"{test_dir}/{self._popen.pid}.valgrind"
+if self.exitcode() == 99:
+with open(valgrind_filename) as f:
+print(f.read())
+else:
+os.remove(valgrind_filename)
+
 def add_object(self, opts):
 self._args.append('-object')
 self._args.append(opts)
-- 
2.31.1

[PATCH v8 09/16] docs/devel/testing: add -gdb option to the debugging section of QEMU iotests

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 docs/devel/testing.rst | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 9d6a8f8636..8b24e6fb47 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -229,6 +229,17 @@ Debugging a test case
 The following options to the ``check`` script can be useful when debugging
 a failing test:
 
+* ``-gdb`` wraps every QEMU invocation in a ``gdbserver``, which waits for a
+  connection from a gdb client.  The options given to ``gdbserver`` (e.g. the
+  address on which to listen for connections) are taken from the 
``$GDB_OPTIONS``
+  environment variable.  By default (if ``$GDB_OPTIONS`` is empty), it listens 
on
+  ``localhost:12345``.
+  It is possible to connect to it for example with
+  ``gdb -iex "target remote $addr"``, where ``$addr`` is the address
+  ``gdbserver`` listens on.
+  If the ``-gdb`` option is not used, ``$GDB_OPTIONS`` is ignored,
+  regardless on whether it is set or not.
+
 * ``-d`` (debug) just increases the logging verbosity, showing
   for example the QMP commands and answers.
 
-- 
2.31.1

[PATCH v8 03/16] python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine

[Emanuele Giuseppe Esposito]

Signed-off-by: Emanuele Giuseppe Esposito 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Reviewed-by: Max Reitz 
Acked-by: John Snow 
---
 python/qemu/machine/qtest.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/python/qemu/machine/qtest.py b/python/qemu/machine/qtest.py
index 33a86a9d69..dc2b5ccfb1 100644
--- a/python/qemu/machine/qtest.py
+++ b/python/qemu/machine/qtest.py
@@ -112,6 +112,7 @@ class QEMUQtestMachine(QEMUMachine):
 def __init__(self,
  binary: str,
  args: Sequence[str] = (),
+ wrapper: Sequence[str] = (),
  name: Optional[str] = None,
  base_temp_dir: str = "/var/tmp",
  socket_scm_helper: Optional[str] = None,
@@ -121,7 +122,8 @@ def __init__(self,
 name = "qemu-%d" % os.getpid()
 if sock_dir is None:
 sock_dir = base_temp_dir
-super().__init__(binary, args, name=name, base_temp_dir=base_temp_dir,
+super().__init__(binary, args, wrapper=wrapper, name=name,
+ base_temp_dir=base_temp_dir,
  socket_scm_helper=socket_scm_helper,
  sock_dir=sock_dir, qmp_timer=qmp_timer)
 self._qtest: Optional[QEMUQtestProtocol] = None
-- 
2.31.1