Re: [PATCH v4 0/7] iotests: Selfish patches

[Andrey Shinkevich]

On 14/10/2019 17:59, Max Reitz wrote:
> On 17.09.19 11:19, Max Reitz wrote:
>> Hi,
>>
>> Again, let me start with a link to an actually explanatory cover letter:
>> https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg01102.html
>>
>> v3:
>> https://lists.nongnu.org/archive/html/qemu-block/2019-08/msg00950.html
>>
>> v4:
>> - I merged the old patch 1 in the meantime
>>
>> - Patch 2: Adjusted the comment to make it more clear that it is
>> case_notrun() itself that will not skip the test case, as
>> requested by Andrey (I hope it fits what he had in mind, more
>> or less); kept the R-bs, because I somehow feel like that’s
>> the right thing to do here.
>>
>> - Patch 3: The func_wrapper returned by the skip_test_decorator has a
>> mandatory argument; make that and its required type explicit
>> (with an annotation), as suggested by John
>> (Kevin made me aware of the fact that annotations exist since
>> Python 3.0, it’s just that they didn’t mean anything back
>> then (neither do they really now, but whatever, it’s better
>> than a comment))
>>
>> - Patch 4: Resolved a conflict because of the change to patch 3
> 
> Thanks for the reviews, applied to my block branch:
> 
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
> 
> Max
> 

Good
-- 
With the best regards,
Andrey Shinkevich

[PATCH 1/2] tests/tcg/multiarch: fix code style in function main of test-mmap.c

[Wei Yang]

This file uses quite a different code style and changing just one line
would leads to some awkward appearance.

This is a preparation for the following replacement of
sysconf(_SC_PAGESIZE).

BTW, to depress ERROR message from checkpatch.pl, this patch replaces
strtoul with qemu_strtoul.

Signed-off-by: Wei Yang 
---
 tests/tcg/multiarch/test-mmap.c | 67 ++---
 1 file changed, 36 insertions(+), 31 deletions(-)

diff --git a/tests/tcg/multiarch/test-mmap.c b/tests/tcg/multiarch/test-mmap.c
index 11d0e777b1..9ea49e2307 100644
--- a/tests/tcg/multiarch/test-mmap.c
+++ b/tests/tcg/multiarch/test-mmap.c
@@ -456,49 +456,54 @@ void check_invalid_mmaps(void)
 
 int main(int argc, char **argv)
 {
-   char tempname[] = "/tmp/.cmmapXX";
-   unsigned int i;
-
-   /* Trust the first argument, otherwise probe the system for our
-  pagesize.  */
-   if (argc > 1)
-   pagesize = strtoul(argv[1], NULL, 0);
-   else
-   pagesize = sysconf(_SC_PAGESIZE);
+char tempname[] = "/tmp/.cmmapXX";
+unsigned int i;
+
+/*
+ * Trust the first argument, otherwise probe the system for our
+ * pagesize.
+ */
+if (argc > 1) {
+qemu_strtoul(argv[1], NULL, 0, );
+} else {
+pagesize = sysconf(_SC_PAGESIZE);
+}
 
-   /* Assume pagesize is a power of two.  */
-   pagemask = pagesize - 1;
-   dummybuf = malloc (pagesize);
-   printf ("pagesize=%u pagemask=%x\n", pagesize, pagemask);
+/* Assume pagesize is a power of two.  */
+pagemask = pagesize - 1;
+dummybuf = malloc(pagesize);
+printf("pagesize=%u pagemask=%x\n", pagesize, pagemask);
 
-   test_fd = mkstemp(tempname);
-   unlink(tempname);
+test_fd = mkstemp(tempname);
+unlink(tempname);
 
-   /* Fill the file with int's counting from zero and up.  */
+/* Fill the file with int's counting from zero and up.  */
 for (i = 0; i < (pagesize * 4) / sizeof i; i++) {
 checked_write(test_fd, , sizeof i);
 }
 
-   /* Append a few extra writes to make the file end at non 
-  page boundary.  */
+/*
+ * Append a few extra writes to make the file end at non
+ * page boundary.
+ */
 checked_write(test_fd, , sizeof i); i++;
 checked_write(test_fd, , sizeof i); i++;
 checked_write(test_fd, , sizeof i); i++;
 
-   test_fsize = lseek(test_fd, 0, SEEK_CUR);
+test_fsize = lseek(test_fd, 0, SEEK_CUR);
 
-   /* Run the tests.  */
-   check_aligned_anonymous_unfixed_mmaps();
-   check_aligned_anonymous_unfixed_colliding_mmaps();
-   check_aligned_anonymous_fixed_mmaps();
-   check_file_unfixed_mmaps();
-   check_file_fixed_mmaps();
-   check_file_fixed_eof_mmaps();
-   check_file_unfixed_eof_mmaps();
-   check_invalid_mmaps();
+/* Run the tests.  */
+check_aligned_anonymous_unfixed_mmaps();
+check_aligned_anonymous_unfixed_colliding_mmaps();
+check_aligned_anonymous_fixed_mmaps();
+check_file_unfixed_mmaps();
+check_file_fixed_mmaps();
+check_file_fixed_eof_mmaps();
+check_file_unfixed_eof_mmaps();
+check_invalid_mmaps();
 
-   /* Fails at the moment.  */
-   /* check_aligned_anonymous_fixed_mmaps_collide_with_host(); */
+/* Fails at the moment.  */
+/* check_aligned_anonymous_fixed_mmaps_collide_with_host(); */
 
-   return EXIT_SUCCESS;
+return EXIT_SUCCESS;
 }
-- 
2.17.1

[PATCH 2/2] core: replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size

[Wei Yang]

Signed-off-by: Wei Yang 
Suggested-by: "Dr. David Alan Gilbert" 
CC: Richard Henderson 
---
 block/file-posix.c  | 2 +-
 net/l2tpv3.c| 2 +-
 tests/tcg/multiarch/test-mmap.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 5d1995a07c..853ed42134 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2562,7 +2562,7 @@ static void check_cache_dropped(BlockDriverState *bs, 
Error **errp)
 off_t end;
 
 /* mincore(2) page status information requires 1 byte per page */
-page_size = sysconf(_SC_PAGESIZE);
+page_size = qemu_real_host_page_size;
 vec = g_malloc(DIV_ROUND_UP(window_size, page_size));
 
 end = raw_getlength(bs);
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 55fea17c0f..5f843240de 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -41,7 +41,7 @@
  * chosen to be sufficient to accommodate one packet with some headers
  */
 
-#define BUFFER_ALIGN sysconf(_SC_PAGESIZE)
+#define BUFFER_ALIGN qemu_real_host_page_size
 #define BUFFER_SIZE 2048
 #define IOVSIZE 2
 #define MAX_L2TPV3_MSGCNT 64
diff --git a/tests/tcg/multiarch/test-mmap.c b/tests/tcg/multiarch/test-mmap.c
index 9ea49e2307..370842e5c2 100644
--- a/tests/tcg/multiarch/test-mmap.c
+++ b/tests/tcg/multiarch/test-mmap.c
@@ -466,7 +466,7 @@ int main(int argc, char **argv)
 if (argc > 1) {
 qemu_strtoul(argv[1], NULL, 0, );
 } else {
-pagesize = sysconf(_SC_PAGESIZE);
+pagesize = qemu_real_host_page_size;
 }
 
 /* Assume pagesize is a power of two.  */
-- 
2.17.1

[PATCH 0/2] replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size

[Wei Yang]

This is a following up patch to cleanup page size, suggested by
"Dr. David Alan Gilbert" .

Patch 2 does the job, while during the cleanup I found test-mmap.c has quite a
lot code style problem. To make the code looks good, patch 1 is introduced to
make checkpatch.pl happy a little.

Wei Yang (2):
  tests/tcg/multiarch: fix code style in function main of test-mmap.c
  core: replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size

 block/file-posix.c  |  2 +-
 net/l2tpv3.c|  2 +-
 tests/tcg/multiarch/test-mmap.c | 67 ++---
 3 files changed, 38 insertions(+), 33 deletions(-)

-- 
2.17.1

Re: [PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size

[Richard Henderson]

On 10/14/19 2:36 PM, Wei Yang wrote:
> On Mon, Oct 14, 2019 at 10:15:02AM +0100, Dr. David Alan Gilbert wrote:
>> We also use sysconf(_SC_PAGESIZE) in a few places.
> 
> You mean need to replace them too?

I would say so, as a separate patch.


r~

Re: [PATCH 0/2] cleanup on page size

[Wei Yang]

Hi, All,

I got one page size related question, hope to get some hint.

There is one comment in page_size_init().

/* NOTE: we can always suppose that qemu_host_page_size >=
   TARGET_PAGE_SIZE */

The final result is true, since we compare qemu_host_page_size with
TARGET_PAGE_SIZE and if not qemu_host_page_size will be assigned to
TARGET_PAGE_SIZE.

Generally, there is no problem, but one corner case for migration.

In function ram_save_host_page(), it tries to save a whole host page. Or to be
specific, it tries to save a whole REAL host page.

The potential problem is when qemu_real_host_page_size < TARGET_PAGE_SIZE,
this whole host page would be a wrong range.

So I am wondering why we have the assumption written in page_size_init()?

I tried to dig out who and why we have this comment, but found the first
commit is

commit 54936004fddc52c321cb3f9a9a51140e782bed5d
Author: bellard 
Date:   Tue May 13 00:25:15 2003 +

mmap emulation


git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@158 
c046a42c-6fe2-441c-8c8c-71466251a162

There is no reason logged.

On Sun, Oct 13, 2019 at 10:11:43AM +0800, Wei Yang wrote:
>Patch 1 simplify the definition of xxx_PAGE_ALIGN.
>Patch 2 replaces getpagesize() with qemu_real_host_page_size. This one touch a
>volume of code. If some point is not correct, I'd appreciate your
>notification.
>
>Wei Yang (2):
>  cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
>  core: replace getpagesize() with qemu_real_host_page_size
>
> accel/kvm/kvm-all.c|  6 +++---
> backends/hostmem.c |  2 +-
> block.c|  4 ++--
> block/file-posix.c |  9 +
> block/io.c |  2 +-
> block/parallels.c  |  2 +-
> block/qcow2-cache.c|  2 +-
> contrib/vhost-user-gpu/vugbm.c |  2 +-
> exec.c |  6 +++---
> hw/intc/s390_flic_kvm.c|  2 +-
> hw/ppc/mac_newworld.c  |  2 +-
> hw/ppc/spapr_pci.c |  2 +-
> hw/rdma/vmw/pvrdma_main.c  |  2 +-
> hw/vfio/spapr.c|  7 ---
> include/exec/cpu-all.h |  7 +++
> include/exec/ram_addr.h|  2 +-
> include/qemu/osdep.h   |  4 ++--
> migration/migration.c  |  2 +-
> migration/postcopy-ram.c   |  4 ++--
> monitor/misc.c |  2 +-
> target/ppc/kvm.c   |  2 +-
> tests/vhost-user-bridge.c  |  8 
> util/mmap-alloc.c  | 10 +-
> util/oslib-posix.c |  4 ++--
> util/oslib-win32.c |  2 +-
> util/vfio-helpers.c| 12 ++--
> 26 files changed, 55 insertions(+), 54 deletions(-)
>
>-- 
>2.17.1

-- 
Wei Yang
Help you, Help me

Re: [PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size

[Wei Yang]

On Mon, Oct 14, 2019 at 10:15:02AM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> There are three page size in qemu:
>> 
>>   real host page size
>>   host page size
>>   target page size
>> 
>> All of them have dedicate variable to represent. For the last two, we
>> use the same form in the whole qemu project, while for the first one we
>> use two forms: qemu_real_host_page_size and getpagesize().
>> 
>> qemu_real_host_page_size is defined to be a replacement of
>> getpagesize(), so let it serve the role.
>
>We also use sysconf(_SC_PAGESIZE) in a few places.

You mean need to replace them too?

>
>Dave
>
>> 
>> [Note] Not fully tested for some arch or device.
>> 
>> Signed-off-by: Wei Yang 
>> ---
>>  accel/kvm/kvm-all.c|  6 +++---
>>  backends/hostmem.c |  2 +-
>>  block.c|  4 ++--
>>  block/file-posix.c |  9 +
>>  block/io.c |  2 +-
>>  block/parallels.c  |  2 +-
>>  block/qcow2-cache.c|  2 +-
>>  contrib/vhost-user-gpu/vugbm.c |  2 +-
>>  exec.c |  6 +++---
>>  hw/intc/s390_flic_kvm.c|  2 +-
>>  hw/ppc/mac_newworld.c  |  2 +-
>>  hw/ppc/spapr_pci.c |  2 +-
>>  hw/rdma/vmw/pvrdma_main.c  |  2 +-
>>  hw/vfio/spapr.c|  7 ---
>>  include/exec/ram_addr.h|  2 +-
>>  include/qemu/osdep.h   |  4 ++--
>>  migration/migration.c  |  2 +-
>>  migration/postcopy-ram.c   |  4 ++--
>>  monitor/misc.c |  2 +-
>>  target/ppc/kvm.c   |  2 +-
>>  tests/vhost-user-bridge.c  |  8 
>>  util/mmap-alloc.c  | 10 +-
>>  util/oslib-posix.c |  4 ++--
>>  util/oslib-win32.c |  2 +-
>>  util/vfio-helpers.c| 12 ++--
>>  25 files changed, 52 insertions(+), 50 deletions(-)
>> 
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index d2d96d73e8..140b0bd8f6 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -52,7 +52,7 @@
>>  /* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We
>>   * need to use the real host PAGE_SIZE, as that's what KVM will use.
>>   */
>> -#define PAGE_SIZE getpagesize()
>> +#define PAGE_SIZE qemu_real_host_page_size
>>  
>>  //#define DEBUG_KVM
>>  
>> @@ -507,7 +507,7 @@ static int 
>> kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>>  {
>>  ram_addr_t start = section->offset_within_region +
>> memory_region_get_ram_addr(section->mr);
>> -ram_addr_t pages = int128_get64(section->size) / getpagesize();
>> +ram_addr_t pages = int128_get64(section->size) / 
>> qemu_real_host_page_size;
>>  
>>  cpu_physical_memory_set_dirty_lebitmap(bitmap, start, pages);
>>  return 0;
>> @@ -1841,7 +1841,7 @@ static int kvm_init(MachineState *ms)
>>   * even with KVM.  TARGET_PAGE_SIZE is assumed to be the minimum
>>   * page size for the system though.
>>   */
>> -assert(TARGET_PAGE_SIZE <= getpagesize());
>> +assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size);
>>  
>>  s->sigmask_len = 8;
>>  
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 6d333dc23c..e773bdfa6e 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -304,7 +304,7 @@ size_t host_memory_backend_pagesize(HostMemoryBackend 
>> *memdev)
>>  #else
>>  size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
>>  {
>> -return getpagesize();
>> +return qemu_real_host_page_size;
>>  }
>>  #endif
>>  
>> diff --git a/block.c b/block.c
>> index 5944124845..98f47e2902 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -106,7 +106,7 @@ size_t bdrv_opt_mem_align(BlockDriverState *bs)
>>  {
>>  if (!bs || !bs->drv) {
>>  /* page size or 4k (hdd sector size) should be on the safe side */
>> -return MAX(4096, getpagesize());
>> +return MAX(4096, qemu_real_host_page_size);
>>  }
>>  
>>  return bs->bl.opt_mem_alignment;
>> @@ -116,7 +116,7 @@ size_t bdrv_min_mem_align(BlockDriverState *bs)
>>  {
>>  if (!bs || !bs->drv) {
>>  /* page size or 4k (hdd sector size) should be on the safe side */
>> -return MAX(4096, getpagesize());
>> +return MAX(4096, qemu_real_host_page_size);
>>  }
>>  
>>  return bs->bl.min_mem_alignment;
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index f12c06de2d..f60ac3f93f 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -322,7 +322,7 @@ static void raw_probe_alignment(BlockDriverState *bs, 
>> int fd, Error **errp)
>>  {
>>  BDRVRawState *s = bs->opaque;
>>  char *buf;
>> -size_t max_align = MAX(MAX_BLOCKSIZE, getpagesize());
>> +size_t max_align = MAX(MAX_BLOCKSIZE, qemu_real_host_page_size);
>>  size_t alignments[] = {1, 512, 1024, 2048, 4096};
>>  
>>  /* For SCSI generic devices the alignment is not 

[PULL v2 17/19] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

The only reason I can imagine for this strange code at the very-end of
bdrv_reopen_commit is the fact that bs->read_only updated after
calling drv->bdrv_reopen_commit in bdrv_reopen_commit. And in the same
time, prior to previous commit, qcow2_reopen_bitmaps_rw did a wrong
check for being writable, when actually it only need writable file
child not self.

So, as it's fixed, let's move things to correct place.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Acked-by: Max Reitz 
Message-id: 20190927122355.7344-10-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 include/block/block_int.h |  6 --
 block.c   | 19 ---
 block/qcow2.c | 15 ++-
 3 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 32fb493cbb..ca4ccac4c1 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -547,12 +547,6 @@ struct BlockDriver {
  uint64_t parent_perm, uint64_t parent_shared,
  uint64_t *nperm, uint64_t *nshared);
 
-/**
- * Bitmaps should be marked as 'IN_USE' in the image on reopening image
- * as rw. This handler should realize it. It also should unset readonly
- * field of BlockDirtyBitmap's in case of success.
- */
-int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
 bool (*bdrv_co_can_store_new_dirty_bitmap)(BlockDriverState *bs,
const char *name,
uint32_t granularity,
diff --git a/block.c b/block.c
index cf312258a9..dad5a3d8e0 100644
--- a/block.c
+++ b/block.c
@@ -3935,16 +3935,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 BlockDriver *drv;
 BlockDriverState *bs;
 BdrvChild *child;
-bool old_can_write, new_can_write;
 
 assert(reopen_state != NULL);
 bs = reopen_state->bs;
 drv = bs->drv;
 assert(drv != NULL);
 
-old_can_write =
-!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
-
 /* If there are any driver level actions to take */
 if (drv->bdrv_reopen_commit) {
 drv->bdrv_reopen_commit(reopen_state);
@@ -3988,21 +3984,6 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 }
 
 bdrv_refresh_limits(bs, NULL);
-
-new_can_write =
-!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
-if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
-Error *local_err = NULL;
-if (drv->bdrv_reopen_bitmaps_rw(bs, _err) < 0) {
-/* This is not fatal, bitmaps just left read-only, so all following
- * writes will fail. User can remove read-only bitmaps to unblock
- * writes.
- */
-error_reportf_err(local_err,
-  "%s: Failed to make dirty bitmaps writable: ",
-  bdrv_get_node_name(bs));
-}
-}
 }
 
 /*
diff --git a/block/qcow2.c b/block/qcow2.c
index 53a025703e..8b05933565 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1835,6 +1835,20 @@ fail:
 static void qcow2_reopen_commit(BDRVReopenState *state)
 {
 qcow2_update_options_commit(state->bs, state->opaque);
+if (state->flags & BDRV_O_RDWR) {
+Error *local_err = NULL;
+
+if (qcow2_reopen_bitmaps_rw(state->bs, _err) < 0) {
+/*
+ * This is not fatal, bitmaps just left read-only, so all following
+ * writes will fail. User can remove read-only bitmaps to unblock
+ * writes or retry reopen.
+ */
+error_reportf_err(local_err,
+  "%s: Failed to make dirty bitmaps writable: ",
+  bdrv_get_node_name(state->bs));
+}
+}
 g_free(state->opaque);
 }
 
@@ -5406,7 +5420,6 @@ BlockDriver bdrv_qcow2 = {
 .bdrv_detach_aio_context  = qcow2_detach_aio_context,
 .bdrv_attach_aio_context  = qcow2_attach_aio_context,
 
-.bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
 .bdrv_co_can_store_new_dirty_bitmap = qcow2_co_can_store_new_dirty_bitmap,
 .bdrv_co_remove_persistent_dirty_bitmap =
 qcow2_co_remove_persistent_dirty_bitmap,
-- 
2.21.0

[PULL v2 19/19] dirty-bitmaps: remove deprecated autoload parameter

[John Snow]

This parameter has been deprecated since 2.12.0 and is eligible for
removal. Remove this parameter as it is actually completely ignored;
let's not give false hope.

Signed-off-by: John Snow 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20191002232411.29968-1-js...@redhat.com
---
 qemu-deprecated.texi | 20 +++-
 qapi/block-core.json |  6 +-
 blockdev.c   |  6 --
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 01245e0b1c..7239e0959d 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -149,11 +149,6 @@ QEMU 4.1 has three options, please migrate to one of these 
three:
 
 @section QEMU Machine Protocol (QMP) commands
 
-@subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
-
-"autoload" parameter is now ignored. All bitmaps are automatically loaded
-from qcow2 images.
-
 @subsection query-block result field dirty-bitmaps[i].status (since 4.0)
 
 The ``status'' field of the ``BlockDirtyInfo'' structure, returned by
@@ -356,3 +351,18 @@ existing CPU models.  Management software that needs 
runnability
 guarantees must resolve the CPU model aliases using te
 ``alias-of'' field returned by the ``query-cpu-definitions'' QMP
 command.
+
+
+@node Recently removed features
+@appendix Recently removed features
+
+What follows is a record of recently removed, formerly deprecated
+features that serves as a record for users who have encountered
+trouble after a recent upgrade.
+
+@section QEMU Machine Protocol (QMP) commands
+
+@subsection block-dirty-bitmap-add "autoload" parameter (since 4.2.0)
+
+The "autoload" parameter has been ignored since 2.12.0. All bitmaps
+are automatically loaded from qcow2 images.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f66553aac7..b274aef713 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2052,10 +2052,6 @@
 #  Qcow2 disks support persistent bitmaps. Default is false for
 #  block-dirty-bitmap-add. (Since: 2.10)
 #
-# @autoload: ignored and deprecated since 2.12.
-#Currently, all dirty tracking bitmaps are loaded from Qcow2 on
-#open.
-#
 # @disabled: the bitmap is created in the disabled state, which means that
 #it will not track drive changes. The bitmap may be enabled with
 #block-dirty-bitmap-enable. Default is false. (Since: 4.0)
@@ -2064,7 +2060,7 @@
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
   'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
-'*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' } }
+'*persistent': 'bool', '*disabled': 'bool' } }
 
 ##
 # @BlockDirtyBitmapMergeSource:
diff --git a/blockdev.c b/blockdev.c
index d77e809623..03c7cd7651 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1966,7 +1966,6 @@ static void block_dirty_bitmap_add_prepare(BlkActionState 
*common,
 qmp_block_dirty_bitmap_add(action->node, action->name,
action->has_granularity, action->granularity,
action->has_persistent, action->persistent,
-   action->has_autoload, action->autoload,
action->has_disabled, action->disabled,
_err);
 
@@ -2858,7 +2857,6 @@ out:
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
 bool has_granularity, uint32_t granularity,
 bool has_persistent, bool persistent,
-bool has_autoload, bool autoload,
 bool has_disabled, bool disabled,
 Error **errp)
 {
@@ -2890,10 +2888,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
 persistent = false;
 }
 
-if (has_autoload) {
-warn_report("Autoload option is deprecated and its value is ignored");
-}
-
 if (!has_disabled) {
 disabled = false;
 }
-- 
2.21.0

[PULL v2 11/19] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Reopening bitmaps to RW was broken prior to previous commit. Check that
it works now.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190927122355.7344-4-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 tests/qemu-iotests/165 | 57 --
 tests/qemu-iotests/165.out |  4 +--
 2 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index 5650dc7c87..951ea011a2 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -43,10 +43,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
 os.remove(disk)
 
 def mkVm(self):
-return iotests.VM().add_drive(disk)
+return iotests.VM().add_drive(disk, opts='node-name=node0')
 
 def mkVmRo(self):
-return iotests.VM().add_drive(disk, opts='readonly=on')
+return iotests.VM().add_drive(disk, opts='readonly=on,node-name=node0')
 
 def getSha256(self):
 result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
@@ -102,6 +102,59 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
 
 self.vm.shutdown()
 
+def test_reopen_rw(self):
+self.vm = self.mkVm()
+self.vm.launch()
+self.qmpAddBitmap()
+
+# Calculate hashes
+
+self.writeRegions(regions1)
+sha256_1 = self.getSha256()
+
+self.writeRegions(regions2)
+sha256_2 = self.getSha256()
+assert sha256_1 != sha256_2 # Otherwise, it's not very interesting.
+
+result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
+ name='bitmap0')
+self.assert_qmp(result, 'return', {})
+
+# Start with regions1
+
+self.writeRegions(regions1)
+assert sha256_1 == self.getSha256()
+
+self.vm.shutdown()
+
+self.vm = self.mkVmRo()
+self.vm.launch()
+
+assert sha256_1 == self.getSha256()
+
+# Check that we are in RO mode and can't modify bitmap.
+self.writeRegions(regions2)
+assert sha256_1 == self.getSha256()
+
+# Reopen to RW
+result = self.vm.qmp('x-blockdev-reopen', **{
+'node-name': 'node0',
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': disk
+},
+'read-only': False
+})
+self.assert_qmp(result, 'return', {})
+
+# Check that bitmap is reopened to RW and we can write to it.
+self.writeRegions(regions2)
+assert sha256_2 == self.getSha256()
+
+self.vm.shutdown()
+
+
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2'],
  supported_protocols=['file'])
diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
index ae1213e6f8..fbc63e62f8 100644
--- a/tests/qemu-iotests/165.out
+++ b/tests/qemu-iotests/165.out
@@ -1,5 +1,5 @@
-.
+..
 --
-Ran 1 tests
+Ran 2 tests
 
 OK
-- 
2.21.0

[PULL v2 18/19] MAINTAINERS: Add Vladimir as a reviewer for bitmaps

[John Snow]

I already try to make sure all bitmaps patches have been reviewed by both
Red Hat and Virtuozzo anyway, so this formalizes the arrangement.

Fam meanwhile is no longer as active, so I am removing him as a co-maintainer
simply to reflect the current practice.

Signed-off-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20191005194448.16629-2-js...@redhat.com
---
 MAINTAINERS | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index fe4dc51b08..250ce8e7a1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1816,8 +1816,8 @@ F: qapi/transaction.json
 T: git https://repo.or.cz/qemu/armbru.git block-next
 
 Dirty Bitmaps
-M: Fam Zheng 
 M: John Snow 
+R: Vladimir Sementsov-Ogievskiy 
 L: qemu-block@nongnu.org
 S: Supported
 F: util/hbitmap.c
@@ -1826,7 +1826,6 @@ F: include/qemu/hbitmap.h
 F: include/block/dirty-bitmap.h
 F: tests/test-hbitmap.c
 F: docs/interop/bitmaps.rst
-T: git https://github.com/famz/qemu.git bitmaps
 T: git https://github.com/jnsnow/qemu.git bitmaps
 
 Character device backends
-- 
2.21.0

[PULL v2 14/19] block/qcow2-bitmap: do not remove bitmaps on reopen-ro

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

qcow2_reopen_bitmaps_ro wants to store bitmaps and then mark them all
readonly. But the latter don't work, as
qcow2_store_persistent_dirty_bitmaps removes bitmaps after storing.
It's OK for inactivation but bad idea for reopen-ro. And this leads to
the following bug:

Assume we have persistent bitmap 'bitmap0'.
Create external snapshot
  bitmap0 is stored and therefore removed
Commit snapshot
  now we have no bitmaps
Do some writes from guest (*)
  they are not marked in bitmap
Shutdown
Start
  bitmap0 is loaded as valid, but it is actually broken! It misses
  writes (*)
Incremental backup
  it will be inconsistent

So, let's stop removing bitmaps on reopen-ro. But don't rejoice:
reopening bitmaps to rw is broken too, so the whole scenario will not
work after this patch and we can't enable corresponding test cases in
260 iotests still. Reopening bitmaps rw will be fixed in the following
patches.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190927122355.7344-7-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block/qcow2.h|  3 ++-
 block/qcow2-bitmap.c | 49 ++--
 block/qcow2.c|  2 +-
 3 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 23a9898a54..5cccd87162 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -742,7 +742,8 @@ Qcow2BitmapInfoList 
*qcow2_get_bitmap_info_list(BlockDriverState *bs,
 Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+  bool release_stored, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
 bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
  const char *name,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index ebc1afccd3..f7dfb40256 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1440,7 +1440,32 @@ out:
 return ret;
 }
 
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+/*
+ * qcow2_store_persistent_dirty_bitmaps
+ *
+ * Stores persistent BdrvDirtyBitmap objects.
+ *
+ * @release_stored: if true, release BdrvDirtyBitmap's after storing to the
+ * image. This is used in two cases, both via qcow2_inactivate:
+ * 1. bdrv_close: It's correct to remove bitmaps on close.
+ * 2. migration: If bitmaps are migrated through migration channel via
+ *'dirty-bitmaps' migration capability they are not handled by this code.
+ *Otherwise, it's OK to drop BdrvDirtyBitmap's and reload them on
+ *invalidation.
+ *
+ * Anyway, it's correct to remove BdrvDirtyBitmap's on inactivation, as
+ * inactivation means that we lose control on disk, and therefore on bitmaps,
+ * we should sync them and do not touch more.
+ *
+ * Contrariwise, we don't want to release any bitmaps on just reopen-to-ro,
+ * when we need to store them, as image is still under our control, and it's
+ * good to keep all the bitmaps in read-only mode. Moreover, keeping them
+ * read-only is correct because this is what would happen if we opened the node
+ * readonly to begin with, and whether we opened directly or reopened to that
+ * state shouldn't matter for the state we get afterward.
+ */
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+  bool release_stored, Error **errp)
 {
 BdrvDirtyBitmap *bitmap;
 BDRVQcow2State *s = bs->opaque;
@@ -1551,20 +1576,14 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 g_free(tb);
 }
 
-QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-/* For safety, we remove bitmap after storing.
- * We may be here in two cases:
- * 1. bdrv_close. It's ok to drop bitmap.
- * 2. inactivation. It means migration without 'dirty-bitmaps'
- *capability, so bitmaps are not marked with
- *BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
- *and reload on invalidation.
- */
-if (bm->dirty_bitmap == NULL) {
-continue;
-}
+if (release_stored) {
+QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+if (bm->dirty_bitmap == NULL) {
+continue;
+}
 
-bdrv_release_dirty_bitmap(bm->dirty_bitmap);
+bdrv_release_dirty_bitmap(bm->dirty_bitmap);
+}
 }
 
 success:
@@ -1592,7 +1611,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error 
**errp)
 BdrvDirtyBitmap *bitmap;
 Error *local_err = NULL;
 
-qcow2_store_persistent_dirty_bitmaps(bs, _err);
+

[PULL v2 15/19] iotests: add test 260 to check bitmap life after snapshot + commit

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190927122355.7344-8-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 tests/qemu-iotests/260 | 89 ++
 tests/qemu-iotests/260.out | 52 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 142 insertions(+)
 create mode 100755 tests/qemu-iotests/260
 create mode 100644 tests/qemu-iotests/260.out

diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260
new file mode 100755
index 00..4f6082c9d2
--- /dev/null
+++ b/tests/qemu-iotests/260
@@ -0,0 +1,89 @@
+#!/usr/bin/env python
+#
+# Tests for temporary external snapshot when we have bitmaps.
+#
+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import iotests
+from iotests import qemu_img_create, file_path, log, filter_qmp_event
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
+base, top = file_path('base', 'top')
+size = 64 * 1024 * 3
+
+
+def print_bitmap(msg, vm):
+result = vm.qmp('query-block')['return'][0]
+if 'dirty-bitmaps' in result:
+bitmap = result['dirty-bitmaps'][0]
+log('{}: name={} dirty-clusters={}'.format(msg, bitmap['name'],
+bitmap['count'] // 64 // 1024))
+else:
+log(msg + ': not found')
+
+
+def test(persistent, restart):
+assert persistent or not restart
+log("\nTestcase {}persistent {} restart\n".format(
+'' if persistent else 'non-', 'with' if restart else 'without'))
+
+qemu_img_create('-f', iotests.imgfmt, base, str(size))
+
+vm = iotests.VM().add_drive(base)
+vm.launch()
+
+vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap0',
+   persistent=persistent)
+vm.hmp_qemu_io('drive0', 'write 0 64K')
+print_bitmap('initial bitmap', vm)
+
+vm.qmp_log('blockdev-snapshot-sync', device='drive0', snapshot_file=top,
+   format=iotests.imgfmt, filters=[iotests.filter_qmp_testfiles])
+vm.hmp_qemu_io('drive0', 'write 64K 512')
+print_bitmap('check that no bitmaps are in snapshot', vm)
+
+if restart:
+log("... Restart ...")
+vm.shutdown()
+vm = iotests.VM().add_drive(top)
+vm.launch()
+
+vm.qmp_log('block-commit', device='drive0', top=top,
+   filters=[iotests.filter_qmp_testfiles])
+ev = vm.events_wait((('BLOCK_JOB_READY', None),
+ ('BLOCK_JOB_COMPLETED', None)))
+log(filter_qmp_event(ev))
+if (ev['event'] == 'BLOCK_JOB_COMPLETED'):
+vm.shutdown()
+log(vm.get_log())
+exit()
+
+vm.qmp_log('block-job-complete', device='drive0')
+ev = vm.event_wait('BLOCK_JOB_COMPLETED')
+log(filter_qmp_event(ev))
+print_bitmap('check bitmap after commit', vm)
+
+vm.hmp_qemu_io('drive0', 'write 128K 64K')
+print_bitmap('check updated bitmap', vm)
+
+vm.shutdown()
+
+
+test(persistent=False, restart=False)
+test(persistent=True, restart=False)
+test(persistent=True, restart=True)
diff --git a/tests/qemu-iotests/260.out b/tests/qemu-iotests/260.out
new file mode 100644
index 00..2f0d98d036
--- /dev/null
+++ b/tests/qemu-iotests/260.out
@@ -0,0 +1,52 @@
+
+Testcase non-persistent without restart
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": 
"drive0", "persistent": false}}
+{"return": {}}
+initial bitmap: name=bitmap0 dirty-clusters=1
+{"execute": "blockdev-snapshot-sync", "arguments": {"device": "drive0", 
"format": "qcow2", "snapshot-file": "TEST_DIR/PID-top"}}
+{"return": {}}
+check that no bitmaps are in snapshot: not found
+{"execute": "block-commit", "arguments": {"device": "drive0", "top": 
"TEST_DIR/PID-top"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": 
"USECS", "seconds": "SECS"}}
+{"execute": "block-job-complete", "arguments": {"device": "drive0"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, 
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": 
{"microseconds": "USECS", "seconds": "SECS"}}
+check bitmap after commit: name=bitmap0 dirty-clusters=2
+check updated bitmap: name=bitmap0 dirty-clusters=3
+
+Testcase 

[PULL v2 12/19] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Firstly, no reason to optimize failure path. Then, function name is
ambiguous: it checks for readonly and similar things, but someone may
think that it will ignore normal bitmaps which was just unchanged, and
this is in bad relation with the fact that we should drop IN_USE flag
for unchanged bitmaps in the image.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190927122355.7344-5-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 include/block/dirty-bitmap.h |  1 -
 block/dirty-bitmap.c | 12 
 block/qcow2-bitmap.c | 23 +--
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 257f0f6704..958e7474fb 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -95,7 +95,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
-bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
 
 BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 6065db8094..4bbb251b2c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -839,18 +839,6 @@ bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap 
*bitmap)
 return bitmap->inconsistent;
 }
 
-bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
-{
-BdrvDirtyBitmap *bm;
-QLIST_FOREACH(bm, >dirty_bitmaps, list) {
-if (bm->persistent && !bm->readonly && !bm->skip_store) {
-return true;
-}
-}
-
-return false;
-}
-
 BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs)
 {
 return QLIST_FIRST(>dirty_bitmaps);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 99812b418b..6dfc083548 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1464,16 +1464,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 Qcow2Bitmap *bm;
 QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables;
 Qcow2BitmapTable *tb, *tb_next;
-
-if (!bdrv_has_changed_persistent_bitmaps(bs)) {
-/* nothing to do */
-return;
-}
-
-if (!can_write(bs)) {
-error_setg(errp, "No write access");
-return;
-}
+bool need_write = false;
 
 QSIMPLEQ_INIT(_tables);
 
@@ -1499,6 +1490,8 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 continue;
 }
 
+need_write = true;
+
 if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) {
 error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: 
",
   name);
@@ -1537,6 +1530,15 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 bm->dirty_bitmap = bitmap;
 }
 
+if (!need_write) {
+goto success;
+}
+
+if (!can_write(bs)) {
+error_setg(errp, "No write access");
+goto fail;
+}
+
 /* allocate clusters and store bitmaps */
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
 if (bm->dirty_bitmap == NULL) {
@@ -1578,6 +1580,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 bdrv_release_dirty_bitmap(bm->dirty_bitmap);
 }
 
+success:
 bitmap_list_free(bm_list);
 return;
 
-- 
2.21.0

[PULL v2 13/19] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

The function is unused, drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190927122355.7344-6-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block/qcow2.h|  2 --
 block/qcow2-bitmap.c | 15 +--
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 0f3d9b088e..23a9898a54 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -740,8 +740,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
 bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
 Error **errp);
-int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
- Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 6dfc083548..ebc1afccd3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1102,8 +1102,7 @@ Qcow2BitmapInfoList 
*qcow2_get_bitmap_info_list(BlockDriverState *bs,
 return list;
 }
 
-int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
- Error **errp)
+int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
 Qcow2BitmapList *bm_list;
@@ -,10 +1110,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, 
bool *header_updated,
 GSList *ro_dirty_bitmaps = NULL;
 int ret = 0;
 
-if (header_updated != NULL) {
-*header_updated = false;
-}
-
 if (s->nb_bitmaps == 0) {
 /* No bitmaps - nothing to do */
 return 0;
@@ -1156,9 +1151,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, 
bool *header_updated,
 error_setg_errno(errp, -ret, "Can't update bitmap directory");
 goto out;
 }
-if (header_updated != NULL) {
-*header_updated = true;
-}
 g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
 }
 
@@ -1169,11 +1161,6 @@ out:
 return ret;
 }
 
-int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
-{
-return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
-}
-
 /* Checks to see if it's safe to resize bitmaps */
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
 {
-- 
2.21.0

[PULL v2 10/19] block: reverse order for reopen commits

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

It's needed to fix reopening qcow2 with bitmaps to RW. Currently it
can't work, as qcow2 needs write access to file child, to mark bitmaps
in-image with IN_USE flag. But usually children goes after parents in
reopen queue and file child is still RO on qcow2 reopen commit. Reverse
reopen order to fix it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Acked-by: Max Reitz 
Acked-by: John Snow 
Message-id: 20190927122355.7344-3-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 0347632c6c..cf312258a9 100644
--- a/block.c
+++ b/block.c
@@ -3486,10 +3486,16 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 bs_entry->perms_checked = true;
 }
 
-/* If we reach this point, we have success and just need to apply the
- * changes
+/*
+ * If we reach this point, we have success and just need to apply the
+ * changes.
+ *
+ * Reverse order is used to comfort qcow2 driver: on commit it need to 
write
+ * IN_USE flag to the image, to mark bitmaps in the image as invalid. But
+ * children are usually goes after parents in reopen-queue, so go from last
+ * to first element.
  */
-QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
 bdrv_reopen_commit(_entry->state);
 }
 
-- 
2.21.0

[PULL v2 08/19] block/dirty-bitmap: refactor bdrv_dirty_bitmap_next

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

bdrv_dirty_bitmap_next is always used in same pattern. So, split it
into _next and _first, instead of combining two functions into one and
add FOR_EACH_DIRTY_BITMAP macro.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190916141911.5255-5-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 include/block/dirty-bitmap.h   |  9 +++--
 block.c|  4 +---
 block/dirty-bitmap.c   | 11 +++
 block/qcow2-bitmap.c   |  8 ++--
 migration/block-dirty-bitmap.c |  4 +---
 5 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 2f9b088e11..257f0f6704 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -96,8 +96,13 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap 
*bitmap);
 bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
-BdrvDirtyBitmap *bitmap);
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs);
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap);
+#define FOR_EACH_DIRTY_BITMAP(bs, bitmap) \
+for (bitmap = bdrv_dirty_bitmap_first(bs); bitmap; \
+ bitmap = bdrv_dirty_bitmap_next(bitmap))
+
 char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
 int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset,
 uint64_t bytes);
diff --git a/block.c b/block.c
index d19a4781a3..5721441697 100644
--- a/block.c
+++ b/block.c
@@ -5390,9 +5390,7 @@ static void coroutine_fn 
bdrv_co_invalidate_cache(BlockDriverState *bs,
 }
 }
 
-for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm;
- bm = bdrv_dirty_bitmap_next(bs, bm))
-{
+FOR_EACH_DIRTY_BITMAP(bs, bm) {
 bdrv_dirty_bitmap_skip_store(bm, false);
 }
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4e5c87a907..6065db8094 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -851,11 +851,14 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState 
*bs)
 return false;
 }
 
-BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
-BdrvDirtyBitmap *bitmap)
+BdrvDirtyBitmap *bdrv_dirty_bitmap_first(BlockDriverState *bs)
 {
-return bitmap == NULL ? QLIST_FIRST(>dirty_bitmaps) :
-QLIST_NEXT(bitmap, list);
+return QLIST_FIRST(>dirty_bitmaps);
+}
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BdrvDirtyBitmap *bitmap)
+{
+return QLIST_NEXT(bitmap, list);
 }
 
 char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp)
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 687087d2bc..99812b418b 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1488,9 +1488,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 }
 
 /* check constraints and names */
-for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
- bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
-{
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
 const char *name = bdrv_dirty_bitmap_name(bitmap);
 uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
 Qcow2Bitmap *bm;
@@ -1610,9 +1608,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error 
**errp)
 return -EINVAL;
 }
 
-for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap != NULL;
- bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
-{
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
 if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
 bdrv_dirty_bitmap_set_readonly(bitmap, true);
 }
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 793f249aa5..7eafface61 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -283,9 +283,7 @@ static int init_dirty_bitmap_migration(void)
 for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) {
 const char *name = bdrv_get_device_or_node_name(bs);
 
-for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
- bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
-{
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
 if (!bdrv_dirty_bitmap_name(bitmap)) {
 continue;
 }
-- 
2.21.0

[PULL v2 05/19] block/dirty-bitmap: drop meta

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Drop meta bitmaps, as they are unused.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190916141911.5255-2-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 include/block/dirty-bitmap.h |  5 
 block/dirty-bitmap.c | 46 
 2 files changed, 51 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 07503b03b5..973056778a 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -18,9 +18,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
   uint32_t granularity,
   const char *name,
   Error **errp);
-void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-   int chunk_size);
-void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
Error **errp);
@@ -54,7 +51,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes);
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
  int64_t offset, int64_t bytes);
-BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
 
@@ -96,7 +92,6 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
 void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t offset);
 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes);
 bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 03e0872b97..4ecf18d5df 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -31,7 +31,6 @@
 struct BdrvDirtyBitmap {
 QemuMutex *mutex;
 HBitmap *bitmap;/* Dirty bitmap implementation */
-HBitmap *meta;  /* Meta dirty bitmap */
 bool busy;  /* Bitmap is busy, it can't be used via QMP */
 BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
 char *name; /* Optional non-empty unique ID */
@@ -127,36 +126,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
 return bitmap;
 }
 
-/* bdrv_create_meta_dirty_bitmap
- *
- * Create a meta dirty bitmap that tracks the changes of bits in @bitmap. I.e.
- * when a dirty status bit in @bitmap is changed (either from reset to set or
- * the other way around), its respective meta dirty bitmap bit will be marked
- * dirty as well.
- *
- * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
- * @chunk_size: how many bytes of bitmap data does each bit in the meta bitmap
- * track.
- */
-void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
-   int chunk_size)
-{
-assert(!bitmap->meta);
-qemu_mutex_lock(bitmap->mutex);
-bitmap->meta = hbitmap_create_meta(bitmap->bitmap,
-   chunk_size * BITS_PER_BYTE);
-qemu_mutex_unlock(bitmap->mutex);
-}
-
-void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
-{
-assert(bitmap->meta);
-qemu_mutex_lock(bitmap->mutex);
-hbitmap_free_meta(bitmap->bitmap);
-bitmap->meta = NULL;
-qemu_mutex_unlock(bitmap->mutex);
-}
-
 int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap)
 {
 return bitmap->size;
@@ -320,7 +289,6 @@ static void 
bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
 assert(!bitmap->active_iterators);
 assert(!bdrv_dirty_bitmap_busy(bitmap));
 assert(!bdrv_dirty_bitmap_has_successor(bitmap));
-assert(!bitmap->meta);
 QLIST_REMOVE(bitmap, list);
 hbitmap_free(bitmap->bitmap);
 g_free(bitmap->name);
@@ -666,15 +634,6 @@ BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap 
*bitmap)
 return iter;
 }
 
-BdrvDirtyBitmapIter *bdrv_dirty_meta_iter_new(BdrvDirtyBitmap *bitmap)
-{
-BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
-hbitmap_iter_init(>hbi, bitmap->meta, 0);
-iter->bitmap = bitmap;
-bitmap->active_iterators++;
-return iter;
-}
-
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
 {
 if (!iter) {
@@ -821,11 +780,6 @@ int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
 return hbitmap_count(bitmap->bitmap);
 }
 
-int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
-{
-return hbitmap_count(bitmap->meta);

[PULL v2 06/19] block/dirty-bitmap: add bs link

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

Add bs field to BdrvDirtyBitmap structure. Drop BlockDriverState
parameter from bitmap APIs where possible.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190916141911.5255-3-vsement...@virtuozzo.com
[Rebased on top of block-copy. --js]
Signed-off-by: John Snow 
---
 include/block/dirty-bitmap.h   | 14 +-
 block/backup.c |  8 
 block/block-copy.c |  2 +-
 block/dirty-bitmap.c   | 24 
 block/mirror.c |  4 ++--
 block/qcow2-bitmap.c   |  6 +++---
 blockdev.c |  6 +++---
 migration/block-dirty-bitmap.c |  7 +++
 migration/block.c  |  4 ++--
 9 files changed, 35 insertions(+), 40 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 973056778a..2f9b088e11 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -18,21 +18,18 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
   uint32_t granularity,
   const char *name,
   Error **errp);
-int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
-   BdrvDirtyBitmap *bitmap,
+int bdrv_dirty_bitmap_create_successor(BdrvDirtyBitmap *bitmap,
Error **errp);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
-BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BdrvDirtyBitmap *bitmap,
 Error **errp);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
-   BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap *bitmap,
Error **errp);
 void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
 const char *name);
 int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
 Error **errp);
-void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
 int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
 Error **errp);
@@ -106,8 +103,7 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap 
*bitmap, uint64_t offset,
 uint64_t bytes);
 bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap,
uint64_t *offset, uint64_t *bytes);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
-  BdrvDirtyBitmap *bitmap,
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
   Error **errp);
 
 #endif
diff --git a/block/backup.c b/block/backup.c
index 46978c1785..dddcf77f53 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -98,13 +98,13 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, 
int ret)
  * We succeeded, or we always intended to sync the bitmap.
  * Delete this bitmap and install the child.
  */
-bm = bdrv_dirty_bitmap_abdicate(job->source_bs, job->sync_bitmap, 
NULL);
+bm = bdrv_dirty_bitmap_abdicate(job->sync_bitmap, NULL);
 } else {
 /*
  * We failed, or we never intended to sync the bitmap anyway.
  * Merge the successor back into the parent, keeping all data.
  */
-bm = bdrv_reclaim_dirty_bitmap(job->source_bs, job->sync_bitmap, NULL);
+bm = bdrv_reclaim_dirty_bitmap(job->sync_bitmap, NULL);
 }
 
 assert(bm);
@@ -402,7 +402,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 }
 
 /* Create a new bitmap, and freeze/disable this one. */
-if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
+if (bdrv_dirty_bitmap_create_successor(sync_bitmap, errp) < 0) {
 return NULL;
 }
 }
@@ -472,7 +472,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 
  error:
 if (sync_bitmap) {
-bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+bdrv_reclaim_dirty_bitmap(sync_bitmap, NULL);
 }
 if (job) {
 backup_clean(>common.job);
diff --git a/block/block-copy.c b/block/block-copy.c
index 0f76ea1e63..066e3a7274 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -60,7 +60,7 @@ void block_copy_state_free(BlockCopyState *s)
   

[PULL v2 16/19] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

- Correct check for write access to file child, and in correct place
  (only if we want to write).
- Support reopen rw -> rw (which will be used in following commit),
  for example, !bdrv_dirty_bitmap_readonly() is not a corruption if
  bitmap is marked IN_USE in the image.
- Consider unexpected bitmap as a corruption and check other
  combinations of in-image and in-RAM bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-id: 20190927122355.7344-9-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block/qcow2-bitmap.c | 77 +---
 1 file changed, 58 insertions(+), 19 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index f7dfb40256..98294a7696 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1108,18 +1108,14 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp)
 Qcow2BitmapList *bm_list;
 Qcow2Bitmap *bm;
 GSList *ro_dirty_bitmaps = NULL;
-int ret = 0;
+int ret = -EINVAL;
+bool need_header_update = false;
 
 if (s->nb_bitmaps == 0) {
 /* No bitmaps - nothing to do */
 return 0;
 }
 
-if (!can_write(bs)) {
-error_setg(errp, "Can't write to the image on reopening bitmaps rw");
-return -EINVAL;
-}
-
 bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
 if (bm_list == NULL) {
@@ -1128,32 +1124,75 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp)
 
 QSIMPLEQ_FOREACH(bm, bm_list, entry) {
 BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
-if (bitmap == NULL) {
-continue;
-}
 
-if (!bdrv_dirty_bitmap_readonly(bitmap)) {
-error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was 
"
-   "not marked as readonly. This is a bug, something went "
-   "wrong. All of the bitmaps may be corrupted", bm->name);
-ret = -EINVAL;
+if (!bitmap) {
+error_setg(errp, "Unexpected bitmap '%s' in image '%s'",
+   bm->name, bs->filename);
 goto out;
 }
 
-bm->flags |= BME_FLAG_IN_USE;
-ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+if (!(bm->flags & BME_FLAG_IN_USE)) {
+if (!bdrv_dirty_bitmap_readonly(bitmap)) {
+error_setg(errp, "Corruption: bitmap '%s' is not marked IN_USE 
"
+   "in the image '%s' and not marked readonly in RAM",
+   bm->name, bs->filename);
+goto out;
+}
+if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
+error_setg(errp, "Corruption: bitmap '%s' is inconsistent but "
+   "is not marked IN_USE in the image '%s'", bm->name,
+   bs->filename);
+goto out;
+}
+
+bm->flags |= BME_FLAG_IN_USE;
+need_header_update = true;
+} else {
+/*
+ * What if flags already has BME_FLAG_IN_USE ?
+ *
+ * 1. if we are reopening RW -> RW it's OK, of course.
+ * 2. if we are reopening RO -> RW:
+ *   2.1 if @bitmap is inconsistent, it's OK. It means that it was
+ *   inconsistent (IN_USE) when we loaded it
+ *   2.2 if @bitmap is not inconsistent. This seems to be 
impossible
+ *   and implies third party interaction. Let's error-out for
+ *   safety.
+ */
+if (bdrv_dirty_bitmap_readonly(bitmap) &&
+!bdrv_dirty_bitmap_inconsistent(bitmap))
+{
+error_setg(errp, "Corruption: bitmap '%s' is marked IN_USE "
+   "in the image '%s' but it is readonly and "
+   "consistent in RAM",
+   bm->name, bs->filename);
+goto out;
+}
+}
+
+if (bdrv_dirty_bitmap_readonly(bitmap)) {
+ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+}
 }
 
-if (ro_dirty_bitmaps != NULL) {
+if (need_header_update) {
+if (!can_write(bs->file->bs) || !(bs->file->perm & BLK_PERM_WRITE)) {
+error_setg(errp, "Failed to reopen bitmaps rw: no write access "
+   "the protocol file");
+goto out;
+}
+
 /* in_use flags must be updated */
 ret = update_ext_header_and_dir_in_place(bs, bm_list);
 if (ret < 0) {
-error_setg_errno(errp, -ret, "Can't update bitmap directory");
+error_setg_errno(errp, -ret, "Cannot update bitmap directory");
 goto out;
 }
-g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
 }
 
+

[PULL v2 07/19] block/dirty-bitmap: drop BdrvDirtyBitmap.mutex

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

mutex field is just a pointer to bs->dirty_bitmap_mutex, so no needs
to store it in BdrvDirtyBitmap when we have bs pointer in it (since
previous patch).

Drop mutex field. Constantly use bdrv_dirty_bitmaps_lock/unlock in
block/dirty-bitmap.c to make it more obvious that it's not per-bitmap
lock. Still, for simplicity, leave bdrv_dirty_bitmap_lock/unlock
functions as an external API.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190916141911.5255-4-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block/dirty-bitmap.c | 84 +---
 1 file changed, 41 insertions(+), 43 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 44453ff824..4e5c87a907 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -29,7 +29,6 @@
 #include "qemu/main-loop.h"
 
 struct BdrvDirtyBitmap {
-QemuMutex *mutex;
 BlockDriverState *bs;
 HBitmap *bitmap;/* Dirty bitmap implementation */
 bool busy;  /* Bitmap is busy, it can't be used via QMP */
@@ -72,12 +71,12 @@ static inline void 
bdrv_dirty_bitmaps_unlock(BlockDriverState *bs)
 
 void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap)
 {
-qemu_mutex_lock(bitmap->mutex);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 }
 
 void bdrv_dirty_bitmap_unlock(BdrvDirtyBitmap *bitmap)
 {
-qemu_mutex_unlock(bitmap->mutex);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 /* Called with BQL or dirty_bitmap lock taken.  */
@@ -117,7 +116,6 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState 
*bs,
 }
 bitmap = g_new0(BdrvDirtyBitmap, 1);
 bitmap->bs = bs;
-bitmap->mutex = >dirty_bitmap_mutex;
 bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity));
 bitmap->size = bitmap_size;
 bitmap->name = g_strdup(name);
@@ -151,9 +149,9 @@ static bool bdrv_dirty_bitmap_busy(const BdrvDirtyBitmap 
*bitmap)
 
 void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy)
 {
-qemu_mutex_lock(bitmap->mutex);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bitmap->busy = busy;
-qemu_mutex_unlock(bitmap->mutex);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 /* Called with BQL taken.  */
@@ -278,10 +276,10 @@ void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap 
*bitmap)
 /* Called with BQL taken. */
 void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
 {
-assert(bitmap->mutex == bitmap->successor->mutex);
-qemu_mutex_lock(bitmap->mutex);
+assert(bitmap->bs == bitmap->successor->bs);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bdrv_enable_dirty_bitmap_locked(bitmap->successor);
-qemu_mutex_unlock(bitmap->mutex);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 /* Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.  */
@@ -361,9 +359,9 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BdrvDirtyBitmap 
*parent,
 {
 BdrvDirtyBitmap *ret;
 
-qemu_mutex_lock(parent->mutex);
+bdrv_dirty_bitmaps_lock(parent->bs);
 ret = bdrv_reclaim_dirty_bitmap_locked(parent, errp);
-qemu_mutex_unlock(parent->mutex);
+bdrv_dirty_bitmaps_unlock(parent->bs);
 
 return ret;
 }
@@ -543,16 +541,16 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState 
*bs, const char *name,
 
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bitmap->disabled = true;
-bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bdrv_enable_dirty_bitmap_locked(bitmap);
-bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
@@ -593,9 +591,9 @@ bool bdrv_dirty_bitmap_get_locked(BdrvDirtyBitmap *bitmap, 
int64_t offset)
 bool bdrv_dirty_bitmap_get(BdrvDirtyBitmap *bitmap, int64_t offset)
 {
 bool ret;
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 ret = bdrv_dirty_bitmap_get_locked(bitmap, offset);
-bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 
 return ret;
 }
@@ -660,9 +658,9 @@ void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
int64_t offset, int64_t bytes)
 {
-bdrv_dirty_bitmap_lock(bitmap);
+bdrv_dirty_bitmaps_lock(bitmap->bs);
 bdrv_set_dirty_bitmap_locked(bitmap, offset, bytes);
-bdrv_dirty_bitmap_unlock(bitmap);
+bdrv_dirty_bitmaps_unlock(bitmap->bs);
 }
 
 /* Called within bdrv_dirty_bitmap_lock..unlock */
@@ -676,15 +674,15 @@ void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap 
*bitmap,
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
  

[PULL v2 09/19] block: switch reopen queue from QSIMPLEQ to QTAILQ

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

We'll need reverse-foreach in the following commit, QTAILQ support it,
so move to QTAILQ.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Message-id: 20190927122355.7344-2-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 include/block/block.h |  2 +-
 block.c   | 24 
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 792bb826db..89606bd9f8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -195,7 +195,7 @@ typedef struct HDGeometry {
 #define BDRV_BLOCK_EOF  0x20
 #define BDRV_BLOCK_RECURSE  0x40
 
-typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) 
BlockReopenQueue;
+typedef QTAILQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
 
 typedef struct BDRVReopenState {
 BlockDriverState *bs;
diff --git a/block.c b/block.c
index 5721441697..0347632c6c 100644
--- a/block.c
+++ b/block.c
@@ -1719,7 +1719,7 @@ typedef struct BlockReopenQueueEntry {
  bool prepared;
  bool perms_checked;
  BDRVReopenState state;
- QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
+ QTAILQ_ENTRY(BlockReopenQueueEntry) entry;
 } BlockReopenQueueEntry;
 
 /*
@@ -1732,7 +1732,7 @@ static int bdrv_reopen_get_flags(BlockReopenQueue *q, 
BlockDriverState *bs)
 BlockReopenQueueEntry *entry;
 
 if (q != NULL) {
-QSIMPLEQ_FOREACH(entry, q, entry) {
+QTAILQ_FOREACH(entry, q, entry) {
 if (entry->state.bs == bs) {
 return entry->state.flags;
 }
@@ -3249,7 +3249,7 @@ static bool bdrv_recurse_has_child(BlockDriverState *bs,
  * Adds a BlockDriverState to a simple queue for an atomic, transactional
  * reopen of multiple devices.
  *
- * bs_queue can either be an existing BlockReopenQueue that has had 
QSIMPLE_INIT
+ * bs_queue can either be an existing BlockReopenQueue that has had QTAILQ_INIT
  * already performed, or alternatively may be NULL a new BlockReopenQueue will
  * be created and initialized. This newly created BlockReopenQueue should be
  * passed back in for subsequent calls that are intended to be of the same
@@ -3290,7 +3290,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 if (bs_queue == NULL) {
 bs_queue = g_new0(BlockReopenQueue, 1);
-QSIMPLEQ_INIT(bs_queue);
+QTAILQ_INIT(bs_queue);
 }
 
 if (!options) {
@@ -3298,7 +3298,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 }
 
 /* Check if this BlockDriverState is already in the queue */
-QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
 if (bs == bs_entry->state.bs) {
 break;
 }
@@ -3354,7 +3354,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 if (!bs_entry) {
 bs_entry = g_new0(BlockReopenQueueEntry, 1);
-QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
+QTAILQ_INSERT_TAIL(bs_queue, bs_entry, entry);
 } else {
 qobject_unref(bs_entry->state.options);
 qobject_unref(bs_entry->state.explicit_options);
@@ -3455,7 +3455,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 
 assert(bs_queue != NULL);
 
-QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
 assert(bs_entry->state.bs->quiesce_counter > 0);
 if (bdrv_reopen_prepare(_entry->state, bs_queue, errp)) {
 goto cleanup;
@@ -3463,7 +3463,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 bs_entry->prepared = true;
 }
 
-QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
 BDRVReopenState *state = _entry->state;
 ret = bdrv_check_perm(state->bs, bs_queue, state->perm,
   state->shared_perm, NULL, NULL, errp);
@@ -3489,13 +3489,13 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
Error **errp)
 /* If we reach this point, we have success and just need to apply the
  * changes
  */
-QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
 bdrv_reopen_commit(_entry->state);
 }
 
 ret = 0;
 cleanup_perm:
-QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
 BDRVReopenState *state = _entry->state;
 
 if (!bs_entry->perms_checked) {
@@ -3512,7 +3512,7 @@ cleanup_perm:
 }
 }
 cleanup:
-QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
 if (ret) {
 if (bs_entry->prepared) {
 bdrv_reopen_abort(_entry->state);
@@ -3552,7 +3552,7 @@ static BlockReopenQueueEntry 

[PULL v2 04/19] block/qcow2: proper locking on bitmap add/remove paths

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

qmp_block_dirty_bitmap_add and do_block_dirty_bitmap_remove do acquire
aio context since 0a6c86d024c52b. But this is not enough: we also must
lock qcow2 mutex when access in-image metadata. Especially it concerns
freeing qcow2 clusters.

To achieve this, move qcow2_can_store_new_dirty_bitmap and
qcow2_remove_persistent_dirty_bitmap to coroutine context.

Since we work in coroutines in correct aio context, we don't need
context acquiring in blockdev.c anymore, drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190920082543.23444-4-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block/qcow2.h |  11 ++--
 include/block/block_int.h |  10 ++--
 block/dirty-bitmap.c  | 102 +++---
 block/qcow2-bitmap.c  |  24 ++---
 block/qcow2.c |   5 +-
 blockdev.c|  27 +++---
 6 files changed, 131 insertions(+), 48 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 08b4c15dc4..0f3d9b088e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -746,12 +746,13 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error 
**errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
-bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
-  const char *name,
-  uint32_t granularity,
-  Error **errp);
-int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char 
*name,
+bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ uint32_t granularity,
  Error **errp);
+int qcow2_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+const char *name,
+Error **errp);
 
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6b511dd889..32fb493cbb 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -553,13 +553,13 @@ struct BlockDriver {
  * field of BlockDirtyBitmap's in case of success.
  */
 int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
-bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
-const char *name,
-uint32_t granularity,
-Error **errp);
-int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+bool (*bdrv_co_can_store_new_dirty_bitmap)(BlockDriverState *bs,
const char *name,
+   uint32_t granularity,
Error **errp);
+int (*bdrv_co_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+  const char *name,
+  Error **errp);
 
 /**
  * Register/unregister a buffer for I/O. For example, when the driver is
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d1ae2e1922..03e0872b97 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -26,6 +26,7 @@
 #include "trace.h"
 #include "block/block_int.h"
 #include "block/blockjob.h"
+#include "qemu/main-loop.h"
 
 struct BdrvDirtyBitmap {
 QemuMutex *mutex;
@@ -455,18 +456,59 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState 
*bs)
  * not fail.
  * This function doesn't release corresponding BdrvDirtyBitmap.
  */
+static int coroutine_fn
+bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+   Error **errp)
+{
+if (bs->drv && bs->drv->bdrv_co_remove_persistent_dirty_bitmap) {
+return bs->drv->bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
+}
+
+return 0;
+}
+
+typedef struct BdrvRemovePersistentDirtyBitmapCo {
+BlockDriverState *bs;
+const char *name;
+Error **errp;
+int ret;
+} BdrvRemovePersistentDirtyBitmapCo;
+
+static void coroutine_fn
+bdrv_co_remove_persistent_dirty_bitmap_entry(void *opaque)
+{
+BdrvRemovePersistentDirtyBitmapCo *s = opaque;
+
+s->ret = bdrv_co_remove_persistent_dirty_bitmap(s->bs, s->name, s->errp);
+aio_wait_kick();
+}
+
 int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
 Error **errp)
 {
-if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
-return 

[PULL v2 03/19] block/dirty-bitmap: return int from bdrv_remove_persistent_dirty_bitmap

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

It's more comfortable to not deal with local_err.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190920082543.23444-3-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block/qcow2.h|  5 ++---
 include/block/block_int.h|  6 +++---
 include/block/dirty-bitmap.h |  5 ++---
 block/dirty-bitmap.c |  9 +
 block/qcow2-bitmap.c | 18 ++
 blockdev.c   |  7 +++
 6 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index f51f478e34..08b4c15dc4 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -750,9 +750,8 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
   const char *name,
   uint32_t granularity,
   Error **errp);
-void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
-  const char *name,
-  Error **errp);
+int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char 
*name,
+ Error **errp);
 
 ssize_t coroutine_fn
 qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 05056b308a..6b511dd889 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -557,9 +557,9 @@ struct BlockDriver {
 const char *name,
 uint32_t granularity,
 Error **errp);
-void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
-const char *name,
-Error **errp);
+int (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
+   const char *name,
+   Error **errp);
 
 /**
  * Register/unregister a buffer for I/O. For example, when the driver is
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 4b4b731b46..07503b03b5 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -37,9 +37,8 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, 
uint32_t flags,
 Error **errp);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
-void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- Error **errp);
+int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+Error **errp);
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 8f42015db9..d1ae2e1922 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -455,13 +455,14 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState 
*bs)
  * not fail.
  * This function doesn't release corresponding BdrvDirtyBitmap.
  */
-void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
- const char *name,
- Error **errp)
+int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+Error **errp)
 {
 if (bs->drv && bs->drv->bdrv_remove_persistent_dirty_bitmap) {
-bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
+return bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, name, errp);
 }
+
+return 0;
 }
 
 bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b2487101ed..9821c1628f 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1404,9 +1404,8 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList 
*bm_list,
 return NULL;
 }
 
-void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
-  const char *name,
-  Error **errp)
+int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char 
*name,
+ Error **errp)
 {
 int ret;
 BDRVQcow2State *s = bs->opaque;
@@ -1416,18 +1415,19 @@ void 
qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
 if (s->nb_bitmaps == 0) {
 /* Absence of the bitmap is not an error: see explanation above
  * 

[PULL v2 00/19] Bitmaps patches

[John Snow]

The following changes since commit c760cb77e511eb05094df67c1b30029a952efa35:

  Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20191011a' 
into staging (2019-10-14 16:09:52 +0100)

are available in the Git repository at:

  https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request

for you to fetch changes up to b2ca29ee390743c42a6062d44ee3b10fb51f9fa6:

  dirty-bitmaps: remove deprecated autoload parameter (2019-10-14 15:28:17 
-0400)


Pull request



John Snow (2):
  MAINTAINERS: Add Vladimir as a reviewer for bitmaps
  dirty-bitmaps: remove deprecated autoload parameter

Vladimir Sementsov-Ogievskiy (17):
  util/hbitmap: strict hbitmap_reset
  block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c
  block/dirty-bitmap: return int from
bdrv_remove_persistent_dirty_bitmap
  block/qcow2: proper locking on bitmap add/remove paths
  block/dirty-bitmap: drop meta
  block/dirty-bitmap: add bs link
  block/dirty-bitmap: drop BdrvDirtyBitmap.mutex
  block/dirty-bitmap: refactor bdrv_dirty_bitmap_next
  block: switch reopen queue from QSIMPLEQ to QTAILQ
  block: reverse order for reopen commits
  iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW
  block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps
  block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()
  block/qcow2-bitmap: do not remove bitmaps on reopen-ro
  iotests: add test 260 to check bitmap life after snapshot + commit
  block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
  qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit

 qemu-deprecated.texi   |  20 ++-
 qapi/block-core.json   |   6 +-
 block/qcow2.h  |  19 +--
 include/block/block.h  |   2 +-
 include/block/block_int.h  |  20 +--
 include/block/dirty-bitmap.h   |  34 ++--
 include/qemu/hbitmap.h |   5 +
 block.c|  79 +++--
 block/backup.c |   8 +-
 block/block-copy.c |   2 +-
 block/dirty-bitmap.c   | 290 +++--
 block/mirror.c |   4 +-
 block/qcow2-bitmap.c   | 212 +++-
 block/qcow2.c  |  22 ++-
 blockdev.c |  40 ++---
 migration/block-dirty-bitmap.c |  11 +-
 migration/block.c  |   4 +-
 tests/test-hbitmap.c   |   2 +-
 util/hbitmap.c |   4 +
 MAINTAINERS|   3 +-
 tests/qemu-iotests/165 |  57 ++-
 tests/qemu-iotests/165.out |   4 +-
 tests/qemu-iotests/260 |  89 ++
 tests/qemu-iotests/260.out |  52 ++
 tests/qemu-iotests/group   |   1 +
 25 files changed, 623 insertions(+), 367 deletions(-)
 create mode 100755 tests/qemu-iotests/260
 create mode 100644 tests/qemu-iotests/260.out

-- 
2.21.0

[PULL v2 02/19] block: move bdrv_can_store_new_dirty_bitmap to block/dirty-bitmap.c

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

block/dirty-bitmap.c seems to be more appropriate for it and
bdrv_remove_persistent_dirty_bitmap already in it.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
Message-id: 20190920082543.23444-2-vsement...@virtuozzo.com
Signed-off-by: John Snow 
---
 block.c  | 22 --
 block/dirty-bitmap.c | 22 ++
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 1946fc6f57..d19a4781a3 100644
--- a/block.c
+++ b/block.c
@@ -6582,25 +6582,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, 
BdrvChild *child, Error **errp)
 
 parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
 }
-
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
- uint32_t granularity, Error **errp)
-{
-BlockDriver *drv = bs->drv;
-
-if (!drv) {
-error_setg_errno(errp, ENOMEDIUM,
- "Can't store persistent bitmaps to %s",
- bdrv_get_device_or_node_name(bs));
-return false;
-}
-
-if (!drv->bdrv_can_store_new_dirty_bitmap) {
-error_setg_errno(errp, ENOTSUP,
- "Can't store persistent bitmaps to %s",
- bdrv_get_device_or_node_name(bs));
-return false;
-}
-
-return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
-}
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 134e0c9a0c..8f42015db9 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -464,6 +464,28 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState 
*bs,
 }
 }
 
+bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp)
+{
+BlockDriver *drv = bs->drv;
+
+if (!drv) {
+error_setg_errno(errp, ENOMEDIUM,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+return false;
+}
+
+if (!drv->bdrv_can_store_new_dirty_bitmap) {
+error_setg_errno(errp, ENOTSUP,
+ "Can't store persistent bitmaps to %s",
+ bdrv_get_device_or_node_name(bs));
+return false;
+}
+
+return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
+}
+
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
 bdrv_dirty_bitmap_lock(bitmap);
-- 
2.21.0

[PULL v2 01/19] util/hbitmap: strict hbitmap_reset

[John Snow]

From: Vladimir Sementsov-Ogievskiy 

hbitmap_reset has an unobvious property: it rounds requested region up.
It may provoke bugs, like in recently fixed write-blocking mode of
mirror: user calls reset on unaligned region, not keeping in mind that
there are possible unrelated dirty bytes, covered by rounded-up region
and information of this unrelated "dirtiness" will be lost.

Make hbitmap_reset strict: assert that arguments are aligned, allowing
only one exception when @start + @count == hb->orig_size. It's needed
to comfort users of hbitmap_next_dirty_area, which cares about
hb->orig_size.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
Message-Id: <20190806152611.280389-1-vsement...@virtuozzo.com>
[Maintainer edit: Max's suggestions from on-list. --js]
[Maintainer edit: Eric's suggestion for aligned macro. --js]
Signed-off-by: John Snow 
---
 include/qemu/hbitmap.h | 5 +
 tests/test-hbitmap.c   | 2 +-
 util/hbitmap.c | 4 
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 4afbe6292e..1bf944ca3d 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -132,6 +132,11 @@ void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t 
count);
  * @count: Number of bits to reset.
  *
  * Reset a consecutive range of bits in an HBitmap.
+ * @start and @count must be aligned to bitmap granularity. The only exception
+ * is resetting the tail of the bitmap: @count may be equal to hb->orig_size -
+ * @start, in this case @count may be not aligned. The sum of @start + @count 
is
+ * allowed to be greater than hb->orig_size, but only if @start < hb->orig_size
+ * and @start + @count = ALIGN_UP(hb->orig_size, granularity).
  */
 void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count);
 
diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
index eed5d288cb..e1f867085f 100644
--- a/tests/test-hbitmap.c
+++ b/tests/test-hbitmap.c
@@ -423,7 +423,7 @@ static void test_hbitmap_granularity(TestHBitmapData *data,
 hbitmap_test_check(data, 0);
 hbitmap_test_set(data, 0, 3);
 g_assert_cmpint(hbitmap_count(data->hb), ==, 4);
-hbitmap_test_reset(data, 0, 1);
+hbitmap_test_reset(data, 0, 2);
 g_assert_cmpint(hbitmap_count(data->hb), ==, 2);
 }
 
diff --git a/util/hbitmap.c b/util/hbitmap.c
index fd44c897ab..66db87c6ff 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -476,6 +476,10 @@ void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t 
count)
 /* Compute range in the last layer.  */
 uint64_t first;
 uint64_t last = start + count - 1;
+uint64_t gran = 1ULL << hb->granularity;
+
+assert(QEMU_IS_ALIGNED(start, gran));
+assert(QEMU_IS_ALIGNED(count, gran) || (start + count == hb->orig_size));
 
 trace_hbitmap_reset(hb, start, count,
 start >> hb->granularity, last >> hb->granularity);
-- 
2.21.0

Re: [PULL 01/19] util/hbitmap: strict hbitmap_reset

[John Snow]

On 10/11/19 7:18 PM, John Snow wrote:
> 
> 
> On 10/11/19 5:48 PM, Eric Blake wrote:
>> On 10/11/19 4:25 PM, John Snow wrote:
>>> From: Vladimir Sementsov-Ogievskiy 
>>>
>>> hbitmap_reset has an unobvious property: it rounds requested region up.
>>> It may provoke bugs, like in recently fixed write-blocking mode of
>>> mirror: user calls reset on unaligned region, not keeping in mind that
>>> there are possible unrelated dirty bytes, covered by rounded-up region
>>> and information of this unrelated "dirtiness" will be lost.
>>>
>>> Make hbitmap_reset strict: assert that arguments are aligned, allowing
>>> only one exception when @start + @count == hb->orig_size. It's needed
>>> to comfort users of hbitmap_next_dirty_area, which cares about
>>> hb->orig_size.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> Reviewed-by: Max Reitz 
>>> Message-Id: <20190806152611.280389-1-vsement...@virtuozzo.com>
>>> [Maintainer edit: Max's suggestions from on-list. --js]
>>> Signed-off-by: John Snow 
>>> ---
>>>   include/qemu/hbitmap.h | 5 +
>>>   tests/test-hbitmap.c   | 2 +-
>>>   util/hbitmap.c | 4 
>>>   3 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>
>>> +++ b/util/hbitmap.c
>>> @@ -476,6 +476,10 @@ void hbitmap_reset(HBitmap *hb, uint64_t start,
>>> uint64_t count)
>>>   /* Compute range in the last layer.  */
>>>   uint64_t first;
>>>   uint64_t last = start + count - 1;
>>> +    uint64_t gran = 1ULL << hb->granularity;
>>> +
>>> +    assert(!(start & (gran - 1)));
>>> +    assert(!(count & (gran - 1)) || (start + count == hb->orig_size));
>>
>> I know I'm replying a bit late (since this is now a pull request), but
>> would it be worth using the dedicated macro:
>>
>> assert(QEMU_IS_ALIGNED(start, gran));
>> assert(QEMU_IS_ALIGNED(count, gran) || start + count == hb->orig_size);
>>
>> instead of open-coding it?  (I would also drop the extra () around the
>> right half of ||). If we want it, that would now be a followup patch.

I've noticed that seasoned C programmers hate extra parentheses a lot.
I've noticed that I cannot remember operator precedence enough to ever
feel like this is actually an improvement.

Something about a nice weighted tree of ((expr1) || (expr2)) feels
soothing to my weary eyes. So, if it's not terribly important, I'd
prefer to leave it as-is.

(You may feel free to counter-educate me as desired.)

>>
> 
> If the PR doesn't make it for some reason, I can amend a cleanup patch
> for the next PR.
> 

by the way: GOOD NEWS! ...

--js

Re: [PATCH 00/20] hw: Clean up hw/i386 headers (and few alpha/hppa)

[John Snow]

On 10/14/19 10:22 AM, Philippe Mathieu-Daudé wrote:
> This is a follow-up of Markus's cleanup series:
> Tame a few "touch this, recompile the world"
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg635748.html
> 
> This part is mostly restricted to X86, but since some file from the
> Alpha/PA-RISC machines include "hw/i386/pc.h" I had to fix them
> too.
> 
> Eventually I'll succeed at removing hw/i386/ dependency on non-X86
> platforms (Quest I started 2 years ago...).
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (20):
>   vl: Add missing "hw/boards.h" include
>   hw/southbridge/ich9: Removed unused headers
>   hw/input/pckbd: Remove unused "hw/i386/pc.h" header
>   hw/i386/ioapic_internal: Remove unused "hw/i386/ioapic.h" header
>   hw/timer: Remove unused "ui/console.h" header
>   hw/usb/dev-storage: Remove unused "ui/console.h" header
>   hw/i386/intel_iommu: Remove unused includes
>   hw/xen/xen_pt_load_rom: Remove unused includes
>   hw/alpha/alpha_sys: Remove unused "hw/ide.h" header
>   hw/alpha/dp264: Include "net/net.h"
>   hw/hppa/machine: Include "net/net.h"
>   hw/acpi/cpu_hotplug: Include "hw/pci/pci.h"
>   hw/timer/hpet: Include "exec/address-spaces.h"
>   hw/pci-host/q35: Include "qemu/range.h"
>   hw/i2c/smbus_ich9: Include "qemu/range.h"
>   hw/pci-host/piix: Include "qemu/range.h"
>   hw/acpi: Include "hw/mem/nvdimm.h"
>   hw/i386: Include "hw/mem/nvdimm.h"
>   hw/pci-host/q35: Remove unused includes
>   hw/i386/pc: Clean up includes
> 
>  hw/acpi/cpu_hotplug.c |  1 +
>  hw/acpi/ich9.c|  2 +-
>  hw/acpi/piix4.c   |  1 +
>  hw/alpha/alpha_sys.h  |  1 -
>  hw/alpha/dp264.c  |  1 +
>  hw/hppa/machine.c |  1 +
>  hw/i2c/smbus_ich9.c   |  1 +
>  hw/i386/acpi-build.c  |  1 +
>  hw/i386/pc.c  |  1 +
>  hw/i386/pc_piix.c |  1 +
>  hw/i386/pc_q35.c  |  1 +
>  hw/input/pckbd.c  |  1 -
>  hw/isa/lpc_ich9.c |  2 --
>  hw/pci-host/piix.c|  1 +
>  hw/pci-host/q35.c |  1 +
>  hw/timer/hpet.c   |  2 +-
>  hw/timer/twl92230.c   |  1 -
>  hw/usb/dev-storage.c  |  1 -
>  hw/xen/xen_pt_load_rom.c  |  4 
>  include/hw/i386/ich9.h|  1 -
>  include/hw/i386/intel_iommu.h |  4 
>  include/hw/i386/ioapic_internal.h |  1 -
>  include/hw/i386/pc.h  | 12 +++-
>  include/hw/pci-host/q35.h |  8 +---
>  vl.c  |  1 +
>  25 files changed, 18 insertions(+), 34 deletions(-)
> 

Acked-by: John Snow 

Re: [PATCH 02/20] hw/southbridge/ich9: Removed unused headers

[John Snow]

On 10/14/19 10:22 AM, Philippe Mathieu-Daudé wrote:
> The ICH9 chipset is not X86/PC specific.
> 
> These files don't use anything declared by the "hw/i386/pc.h"
> or "hw/i386/ioapic.h" headers. Remove them.
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: John Snow 

> ---
>  hw/acpi/ich9.c | 1 -
>  hw/isa/lpc_ich9.c  | 2 --
>  include/hw/i386/ich9.h | 1 -
>  3 files changed, 4 deletions(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 2034dd749e..fdd0a6c79e 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -27,7 +27,6 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
> -#include "hw/i386/pc.h"
>  #include "hw/pci/pci.h"
>  #include "migration/vmstate.h"
>  #include "qemu/timer.h"
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 17c292e306..61cee2ae3a 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -35,10 +35,8 @@
>  #include "hw/isa/isa.h"
>  #include "hw/sysbus.h"
>  #include "migration/vmstate.h"
> -#include "hw/i386/pc.h"
>  #include "hw/irq.h"
>  #include "hw/isa/apm.h"
> -#include "hw/i386/ioapic.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/i386/ich9.h"
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index 72e803f6e2..a98d10b252 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -5,7 +5,6 @@
>  #include "hw/sysbus.h"
>  #include "hw/i386/pc.h"
>  #include "hw/isa/apm.h"
> -#include "hw/i386/ioapic.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pci_bridge.h"
> 

-- 
—js

Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN

[Juan Quintela]

Wei Yang  wrote:
> Use ROUND_UP() to define, which is a little bit easy to read.
>
> Signed-off-by: Wei Yang 

Reviewed-by: Juan Quintela 

Re: Occasional VM soft lockup when a remote cdrom is attached

[John Snow]

On 10/13/19 10:04 PM, Guoheyi wrote:
> Really appreciate your advice. Some comments below:
> 
> 
> On 2019/10/12 3:06, John Snow wrote:
>>
>> On 10/11/19 9:22 AM, Guoheyi wrote:
>>> Hi folks,
>>>
>>> We observed Linux on VM occasionally (at very low rate) got soft lockup
>>> when a remote cdrom is attached. The guest hangs up at below call trace:
>>>
>> That's certainly a new one to me :)
>>
>>> [Tue Oct8 23:02:53 2019]ata_scsi_queuecmd+0xe0/0x2a0 [libata]
>>>
>>> [Tue Oct8 23:02:53 2019]scsi_dispatch_cmd+0xec/0x288
>>>
>>> [Tue Oct8 23:02:53 2019]scsi_queue_rq+0x5f4/0x6b8
>>>
>>> [Tue Oct8 23:02:53 2019]blk_mq_dispatch_rq_list+0xb0/0x690
>>>
>>> [Tue Oct8 23:02:53 2019]blk_mq_do_dispatch_sched+0x8c/0x130
>>>
>>> [Tue Oct8 23:02:53 2019]blk_mq_sched_dispatch_requests+0x128/0x1f0
>>>
>>> [Tue Oct8 23:02:53 2019]__blk_mq_run_hw_queue+0x9c/0x128
>>>
>>> [Tue Oct8 23:02:53 2019]__blk_mq_delay_run_hw_queue+0x198/0x1d8
>>>
>>> [Tue Oct8 23:02:53 2019]blk_mq_run_hw_queue+0x68/0x180
>>>
>>> [Tue Oct8 23:02:53 2019]blk_mq_sched_insert_request+0xbc/0x210
>>>
>>> [Tue Oct8 23:02:53 2019]blk_execute_rq_nowait+0x118/0x168
>>>
>>> [Tue Oct8 23:02:53 2019]blk_execute_rq+0x74/0xd8
>>>
>>> [Tue Oct8 23:02:53 2019]__scsi_execute+0xd8/0x1e0
>>>
>>> [Tue Oct8 23:02:53 2019]sr_check_events+0xd4/0x2c8 [sr_mod]
>>>
>>> [Tue Oct8 23:02:53 2019]cdrom_check_events+0x34/0x50 [cdrom]
>>>
>>> [Tue Oct8 23:02:53 2019]sr_block_check_events+0xdc/0x108 [sr_mod]
>>>
>>> [Tue Oct8 23:02:53 2019]disk_check_events+0x60/0x198
>>>
>>> [Tue Oct8 23:02:53 2019]disk_events_workfn+0x24/0x30
>>>
>>> [Tue Oct8 23:02:53 2019]process_one_work+0x1b4/0x3f8
>>>
>>> [Tue Oct8 23:02:53 2019]worker_thread+0x54/0x470
>>>
>>> [Tue Oct8 23:02:53 2019]kthread+0x134/0x138
>>>
>>> [Tue Oct8 23:02:53 2019]ret_from_fork+0x10/0x18
>>>
>>>
>>> We are running the whole stack on ARM64 platforms, using rcdrom on host
>>> to connect a remote cdrom, which is appeared as "/dev/sr0" on the host.
>>> Our Linux kernel version is 4.19.36 and qemu version is 2.8.1, which is
>>> fairly old but I checked the mainline and found the work flow does not
>>> change much. And KVM is enabled.
>>>
>>> We provide the remote cdrom to guest as a block device, attached under
>>> ICH SATA bus.
>>>
>>>
>>> The work flow should be like this (please correct me if I was wrong):
>>>
>>> 1. There is a kworker thread in guest kernel which will check cdrom
>>> status periodically.
>>>
>>> 2. The call of "ata_scsi_queuecmd" in guest will write AHCI port
>>> register "PORT_CMD_ISSUE", so this VCPU thread is trapped out to qemu.
>>>
>>> 3. qemu will grab the BQL and then dispatch the access to
>>> ahci_port_write().
>>>
>>> 4. For this is a "get event status notification" command, qemu finally
>>> goes to cmd_get_event_status_notification() and then
>>> cdrom_is_inserted().
>>>
>> via
>>
>> cmd_get_event_status_notification (SCSI 0x4A)
>>    event_status_media
>>  blk_is_inserted
>>
>>> 5. In cdrom_is_inserted(), an ioctl to cdrom fd is issued.
>>>
>> Using the bdrv_host_cdrom BlockDriver, for the .bdrv_is_inserted
>> callback.
>>
>>> However, in the last step, we found the ioctl() may have large latency,
>>> for it is a virtual device of remote cdrom, when the remote server is
>>> busy and of poor performance. We have observed more than 8 seconds
>>> latency in half an hour test, and the latency might reach more than 20
>>> seconds when guest soft lockup occurred.
>>>
>> I'm not sure what can be done here. the host_cdrom driver has a few
>> methods to query state (cdrom_is_inserted, cdrom_eject,
>> cdrom_lock_medium) and in general code is going to rely on
>> bdrv_is_inserted returning a truthful answer.
>>
>> (I'm not sure we have callbacks established to tell when the backing
>> media we are ourselves relying on has gone away. Maybe it could be
>> added, but it's not there now. We could maybe cache the answer if we had
>> something reliable.)
>>
>> You could always try using the host_device driver instead of the
>> host_cdrom one, which will just treat it as a "normal" block device
>> instead of a CDROM one, and doesn't use any cdrom specific ioctls. It
>> might avoid the costly call.
> By following this work around, the cdrom device was represented to guest
> as a normal disk ("/dev/sdb"). We are not sure if this will cause more
> functional differences.
> 

What command line are you using? You should be able to specify the
emulator (ask for device ide-cd) separately from the backend (specify
block driver host_device.)

It might require the use of -blockdev command line flags instead of
sugared variants (-cdrom, -drive.)

>>
>>> My question is, is there any way to get around of this issue? Does it
>>> make sense for qemu to setup an IO thread to issue this ioctl() and let
>>> the VCPU thread return to guest as soon as possible? Or it is kernel's
>>> responsibility to break up the long time ioctl() and return to user
>>> space?
>>>
>> Yeah, I think you could probably try to make this change -- 

Re: [PULL 00/19] Bitmaps patches

[Peter Maydell]

On Fri, 11 Oct 2019 at 22:26, John Snow  wrote:
>
> The following changes since commit 98b2e3c9ab3abfe476a2b02f8f51813edb90e72d:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' 
> into staging (2019-10-08 16:08:35 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request
>
> for you to fetch changes up to b97d9a1014b61dd0980e7f4a0c9ca1e3b0aaa761:
>
>   dirty-bitmaps: remove deprecated autoload parameter (2019-10-09 17:02:45 
> -0400)
>
> 
> Pull request
>
> 
>

Hi -- this has a conflict in block/backup.c -- could you fix up
and resend, please ?

thanks
-- PMM

[PULL 05/15] replay: don't drain/flush bdrv queue while RR is working

[Kevin Wolf]

From: Pavel Dovgalyuk 

In record/replay mode bdrv queue is controlled by replay mechanism.
It does not allow saving or loading the snapshots
when bdrv queue is not empty. Stopping the VM is not blocked by nonempty
queue, but flushing the queue is still impossible there,
because it may cause deadlocks in replay mode.
This patch disables bdrv_drain_all and bdrv_flush_all in
record/replay mode.

Stopping the machine when the IO requests are not finished is needed
for the debugging. E.g., breakpoint may be set at the specified step,
and forcing the IO requests to finish may break the determinism
of the execution.

Signed-off-by: Pavel Dovgalyuk 
Acked-by: Kevin Wolf 
Signed-off-by: Kevin Wolf 
---
 block/io.c | 28 
 cpus.c |  2 --
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index 4f9ee97c2b..834841142a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -33,6 +33,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
+#include "sysemu/replay.h"
 
 #define NOT_DONE 0x7fff /* used while emulated sync operation in progress 
*/
 
@@ -600,6 +601,15 @@ void bdrv_drain_all_begin(void)
 return;
 }
 
+/*
+ * bdrv queue is managed by record/replay,
+ * waiting for finishing the I/O requests may
+ * be infinite
+ */
+if (replay_events_enabled()) {
+return;
+}
+
 /* AIO_WAIT_WHILE() with a NULL context can only be called from the main
  * loop AioContext, so make sure we're in the main context. */
 assert(qemu_get_current_aio_context() == qemu_get_aio_context());
@@ -629,6 +639,15 @@ void bdrv_drain_all_end(void)
 BlockDriverState *bs = NULL;
 int drained_end_counter = 0;
 
+/*
+ * bdrv queue is managed by record/replay,
+ * waiting for finishing the I/O requests may
+ * be endless
+ */
+if (replay_events_enabled()) {
+return;
+}
+
 while ((bs = bdrv_next_all_states(bs))) {
 AioContext *aio_context = bdrv_get_aio_context(bs);
 
@@ -2124,6 +2143,15 @@ int bdrv_flush_all(void)
 BlockDriverState *bs = NULL;
 int result = 0;
 
+/*
+ * bdrv queue is managed by record/replay,
+ * creating new flush request for stopping
+ * the VM may break the determinism
+ */
+if (replay_events_enabled()) {
+return result;
+}
+
 for (bs = bdrv_first(); bs; bs = bdrv_next()) {
 AioContext *aio_context = bdrv_get_aio_context(bs);
 int ret;
diff --git a/cpus.c b/cpus.c
index d2c61ff155..367f0657c5 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1097,7 +1097,6 @@ static int do_vm_stop(RunState state, bool send_stop)
 }
 
 bdrv_drain_all();
-replay_disable_events();
 ret = bdrv_flush_all();
 
 return ret;
@@ -2181,7 +2180,6 @@ int vm_prepare_start(void)
 /* We are sending this now, but the CPUs will be resumed shortly later */
 qapi_event_send_resume();
 
-replay_enable_events();
 cpu_enable_ticks();
 runstate_set(RUN_STATE_RUNNING);
 vm_state_notify(1, RUN_STATE_RUNNING);
-- 
2.20.1

[PULL 07/15] replay: add BH oneshot event for block layer

[Kevin Wolf]

From: Pavel Dovgalyuk 

Replay is capable of recording normal BH events, but sometimes
there are single use callbacks scheduled with aio_bh_schedule_oneshot
function. This patch enables recording and replaying such callbacks.
Block layer uses these events for calling the completion function.
Replaying these calls makes the execution deterministic.

Signed-off-by: Pavel Dovgalyuk 
Acked-by: Kevin Wolf 
Signed-off-by: Kevin Wolf 
---
 include/sysemu/replay.h  |  4 
 replay/replay-internal.h |  1 +
 block/block-backend.c|  9 ++---
 block/io.c   |  4 ++--
 block/iscsi.c|  5 +++--
 block/nfs.c  |  6 --
 block/null.c |  4 +++-
 block/nvme.c |  6 --
 block/rbd.c  |  5 +++--
 block/vxhs.c |  5 +++--
 replay/replay-events.c   | 16 
 stubs/replay-user.c  |  9 +
 stubs/Makefile.objs  |  1 +
 13 files changed, 59 insertions(+), 16 deletions(-)
 create mode 100644 stubs/replay-user.c

diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h
index dfc7a31c66..8df517298c 100644
--- a/include/sysemu/replay.h
+++ b/include/sysemu/replay.h
@@ -15,6 +15,7 @@
 #include "qapi/qapi-types-misc.h"
 #include "qapi/qapi-types-run-state.h"
 #include "qapi/qapi-types-ui.h"
+#include "block/aio.h"
 
 /* replay clock kinds */
 enum ReplayClockKind {
@@ -140,6 +141,9 @@ void replay_enable_events(void);
 bool replay_events_enabled(void);
 /*! Adds bottom half event to the queue */
 void replay_bh_schedule_event(QEMUBH *bh);
+/* Adds oneshot bottom half event to the queue */
+void replay_bh_schedule_oneshot_event(AioContext *ctx,
+QEMUBHFunc *cb, void *opaque);
 /*! Adds input event to the queue */
 void replay_input_event(QemuConsole *src, InputEvent *evt);
 /*! Adds input sync event to the queue */
diff --git a/replay/replay-internal.h b/replay/replay-internal.h
index afba9a3e0c..55fca1ac6b 100644
--- a/replay/replay-internal.h
+++ b/replay/replay-internal.h
@@ -51,6 +51,7 @@ enum ReplayEvents {
 
 enum ReplayAsyncEventKind {
 REPLAY_ASYNC_EVENT_BH,
+REPLAY_ASYNC_EVENT_BH_ONESHOT,
 REPLAY_ASYNC_EVENT_INPUT,
 REPLAY_ASYNC_EVENT_INPUT_SYNC,
 REPLAY_ASYNC_EVENT_CHAR_READ,
diff --git a/block/block-backend.c b/block/block-backend.c
index 1c605d5444..eb22ff306e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -18,6 +18,8 @@
 #include "hw/qdev-core.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/runstate.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/replay.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-block.h"
 #include "qemu/id.h"
@@ -1306,7 +1308,8 @@ BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
 acb->blk = blk;
 acb->ret = ret;
 
-aio_bh_schedule_oneshot(blk_get_aio_context(blk), error_callback_bh, acb);
+replay_bh_schedule_oneshot_event(blk_get_aio_context(blk),
+ error_callback_bh, acb);
 return >common;
 }
 
@@ -1362,8 +1365,8 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, 
int64_t offset, int bytes,
 
 acb->has_returned = true;
 if (acb->rwco.ret != NOT_DONE) {
-aio_bh_schedule_oneshot(blk_get_aio_context(blk),
-blk_aio_complete_bh, acb);
+replay_bh_schedule_oneshot_event(blk_get_aio_context(blk),
+ blk_aio_complete_bh, acb);
 }
 
 return >common;
diff --git a/block/io.c b/block/io.c
index 834841142a..8b6dace056 100644
--- a/block/io.c
+++ b/block/io.c
@@ -369,8 +369,8 @@ static void coroutine_fn 
bdrv_co_yield_to_drain(BlockDriverState *bs,
 if (bs) {
 bdrv_inc_in_flight(bs);
 }
-aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
-bdrv_co_drain_bh_cb, );
+replay_bh_schedule_oneshot_event(bdrv_get_aio_context(bs),
+ bdrv_co_drain_bh_cb, );
 
 qemu_coroutine_yield();
 /* If we are resumed from some other event (such as an aio completion or a
diff --git a/block/iscsi.c b/block/iscsi.c
index 506bf5f875..2ced15066a 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -40,6 +40,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/uuid.h"
+#include "sysemu/replay.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc.h"
 #include "qapi/qmp/qdict.h"
@@ -280,8 +281,8 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status,
 }
 
 if (iTask->co) {
-aio_bh_schedule_oneshot(iTask->iscsilun->aio_context,
- iscsi_co_generic_bh_cb, iTask);
+replay_bh_schedule_oneshot_event(iTask->iscsilun->aio_context,
+ iscsi_co_generic_bh_cb, iTask);
 } else {
 iTask->complete = 1;
 }
diff --git a/block/nfs.c b/block/nfs.c
index f39acfdb28..40f23495a0 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -37,6 +37,8 @@
 #include "qemu/option.h"
 #include "qemu/uri.h"
 

[PULL 06/15] replay: finish record/replay before closing the disks

[Kevin Wolf]

From: Pavel Dovgalyuk 

After recent updates block devices cannot be closed on qemu exit.
This happens due to the block request polling when replay is not finished.
Therefore now we stop execution recording before closing the block devices.

Signed-off-by: Pavel Dovgalyuk 
Signed-off-by: Kevin Wolf 
---
 replay/replay.c | 2 ++
 vl.c| 1 +
 2 files changed, 3 insertions(+)

diff --git a/replay/replay.c b/replay/replay.c
index 713395b33d..5cc25bd2f8 100644
--- a/replay/replay.c
+++ b/replay/replay.c
@@ -385,6 +385,8 @@ void replay_finish(void)
 g_free(replay_snapshot);
 replay_snapshot = NULL;
 
+replay_mode = REPLAY_MODE_NONE;
+
 replay_finish_events();
 }
 
diff --git a/vl.c b/vl.c
index fce9ce2364..57d4fd726f 100644
--- a/vl.c
+++ b/vl.c
@@ -4524,6 +4524,7 @@ int main(int argc, char **argv, char **envp)
 
 /* No more vcpu or device emulation activity beyond this point */
 vm_shutdown();
+replay_finish();
 
 job_cancel_sync_all();
 bdrv_close_all();
-- 
2.20.1

[PULL 08/15] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK

[Kevin Wolf]

From: Alberto Garcia 

The BDRV_REQ_NO_FALLBACK flag means that an operation should only be
performed if it can be offloaded or otherwise performed efficiently.

However a misaligned write request requires a RMW so we should return
an error and let the caller decide how to proceed.

This hits an assertion since commit c8bb23cbdb if the required
alignment is larger than the cluster size:

qemu-img create -f qcow2 -o cluster_size=2k img.qcow2 4G
qemu-io -c "open -o driver=qcow2,file.align=4k blkdebug::img.qcow2" \
-c 'write 0 512'
qemu-io: block/io.c:1127: bdrv_driver_pwritev: Assertion `!(flags & 
BDRV_REQ_NO_FALLBACK)' failed.
Aborted

The reason is that when writing to an unallocated cluster we try to
skip the copy-on-write part and zeroize it using BDRV_REQ_NO_FALLBACK
instead, resulting in a write request that is too small (2KB cluster
size vs 4KB required alignment).

Signed-off-by: Alberto Garcia 
Signed-off-by: Kevin Wolf 
---
 block/io.c |  7 +
 tests/qemu-iotests/268 | 55 ++
 tests/qemu-iotests/268.out |  7 +
 tests/qemu-iotests/group   |  1 +
 4 files changed, 70 insertions(+)
 create mode 100755 tests/qemu-iotests/268
 create mode 100644 tests/qemu-iotests/268.out

diff --git a/block/io.c b/block/io.c
index 8b6dace056..f0b86c1d19 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2090,6 +2090,13 @@ int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
 return ret;
 }
 
+/* If the request is misaligned then we can't make it efficient */
+if ((flags & BDRV_REQ_NO_FALLBACK) &&
+!QEMU_IS_ALIGNED(offset | bytes, align))
+{
+return -ENOTSUP;
+}
+
 bdrv_inc_in_flight(bs);
 /*
  * Align write if necessary by performing a read-modify-write cycle.
diff --git a/tests/qemu-iotests/268 b/tests/qemu-iotests/268
new file mode 100755
index 00..78c3f4db3a
--- /dev/null
+++ b/tests/qemu-iotests/268
@@ -0,0 +1,55 @@
+#!/usr/bin/env bash
+#
+# Test write request with required alignment larger than the cluster size
+#
+# Copyright (C) 2019 Igalia, S.L.
+# Author: Alberto Garcia 
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=be...@igalia.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+
+echo
+echo "== Required alignment larger than cluster size =="
+
+CLUSTER_SIZE=2k _make_test_img 1M
+# Since commit c8bb23cbdb writing to an unallocated cluster fills the
+# empty COW areas with bdrv_write_zeroes(flags=BDRV_REQ_NO_FALLBACK)
+$QEMU_IO -c "open -o driver=$IMGFMT,file.align=4k blkdebug::$TEST_IMG" \
+ -c "write 0 512" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/268.out b/tests/qemu-iotests/268.out
new file mode 100644
index 00..2ed6c68529
--- /dev/null
+++ b/tests/qemu-iotests/268.out
@@ -0,0 +1,7 @@
+QA output created by 268
+
+== Required alignment larger than cluster size ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5805a79d9e..4c861f7eed 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -278,3 +278,4 @@
 265 rw auto quick
 266 rw quick
 267 rw auto quick snapshot
+268 rw auto quick
-- 
2.20.1

[PULL 09/15] iotests/028: Fix for long $TEST_DIRs

[Kevin Wolf]

From: Max Reitz 

For long test image paths, the order of the "Formatting" line and the
"(qemu)" prompt after a drive_backup HMP command may be reversed.  In
fact, the interaction between the prompt and the line may lead to the
"Formatting" to being greppable at all after "read"-ing it (if the
prompt injects an IFS character into the "Formatting" string).

So just wait until we get a prompt.  At that point, the block job must
have been started, so "info block-jobs" will only return "No active
jobs" once it is done.

Reported-by: Thomas Huth 
Signed-off-by: Max Reitz 
Reviewed-by: John Snow 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/028 | 11 ---
 tests/qemu-iotests/028.out |  1 -
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028
index 71301ec6e5..bba1ee59ae 100755
--- a/tests/qemu-iotests/028
+++ b/tests/qemu-iotests/028
@@ -119,9 +119,14 @@ fi
 # Silence output since it contains the disk image path and QEMU's readline
 # character echoing makes it very hard to filter the output. Plus, there
 # is no telling how many times the command will repeat before succeeding.
-_send_qemu_cmd $h "drive_backup disk ${TEST_IMG}.copy" "(qemu)" >/dev/null
-_send_qemu_cmd $h "" "Formatting" | _filter_img_create
-qemu_cmd_repeat=20 _send_qemu_cmd $h "info block-jobs" "No active jobs" 
>/dev/null
+# (Note that creating the image results in a "Formatting..." message over
+# stdout, which is the same channel the monitor uses.  We cannot reliably
+# wait for it because the monitor output may interact with it in such a
+# way that _timed_wait_for cannot read it.  However, once the block job is
+# done, we know that the "Formatting..." message must have appeared
+# already, so the output is still deterministic.)
+silent=y _send_qemu_cmd $h "drive_backup disk ${TEST_IMG}.copy" "(qemu)"
+silent=y qemu_cmd_repeat=20 _send_qemu_cmd $h "info block-jobs" "No active 
jobs"
 _send_qemu_cmd $h "info block-jobs" "No active jobs"
 _send_qemu_cmd $h 'quit' ""
 
diff --git a/tests/qemu-iotests/028.out b/tests/qemu-iotests/028.out
index 7d54aeb003..37aed84436 100644
--- a/tests/qemu-iotests/028.out
+++ b/tests/qemu-iotests/028.out
@@ -468,7 +468,6 @@ No errors were found on the image.
 
 block-backup
 
-Formatting 'TEST_DIR/t.IMGFMT.copy', fmt=IMGFMT size=4294968832 
backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
 (qemu) info block-jobs
 No active jobs
 === IO: pattern 195
-- 
2.20.1

[PULL 00/15] Block layer patches

[Kevin Wolf]

The following changes since commit 22dbfdecc3c52228d3489da3fe81da92b21197bf:

  Merge remote-tracking branch 'remotes/awilliam/tags/vfio-update-20191010.0' 
into staging (2019-10-14 15:09:08 +0100)

are available in the Git repository at:

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

for you to fetch changes up to a1406a9262a087d9ec9627b88da13c4590b61dae:

  iotests: Test large write request to qcow2 file (2019-10-14 17:12:48 +0200)


Block layer patches:

- block: Fix crash with qcow2 partial cluster COW with small cluster
  sizes (misaligned write requests with BDRV_REQ_NO_FALLBACK)
- qcow2: Fix integer overflow potentially causing corruption with huge
  requests
- vhdx: Detect truncated image files
- tools: Support help options for --object
- Various block-related replay improvements
- iotests/028: Fix for long $TEST_DIRs


Alberto Garcia (1):
  block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK

Kevin Wolf (4):
  vl: Split off user_creatable_print_help()
  qemu-io: Support help options for --object
  qemu-img: Support help options for --object
  qemu-nbd: Support help options for --object

Max Reitz (3):
  iotests/028: Fix for long $TEST_DIRs
  qcow2: Limit total allocation range to INT_MAX
  iotests: Test large write request to qcow2 file

Pavel Dovgaluk (6):
  block: implement bdrv_snapshot_goto for blkreplay
  replay: disable default snapshot for record/replay
  replay: update docs for record/replay with block devices
  replay: don't drain/flush bdrv queue while RR is working
  replay: finish record/replay before closing the disks
  replay: add BH oneshot event for block layer

Peter Lieven (1):
  block/vhdx: add check for truncated image files

 docs/replay.txt |  12 +++-
 include/qom/object_interfaces.h |  12 
 include/sysemu/replay.h |   4 ++
 replay/replay-internal.h|   1 +
 block/blkreplay.c   |   8 +++
 block/block-backend.c   |   9 ++-
 block/io.c  |  39 -
 block/iscsi.c   |   5 +-
 block/nfs.c |   6 +-
 block/null.c|   4 +-
 block/nvme.c|   6 +-
 block/qcow2-cluster.c   |   5 +-
 block/rbd.c |   5 +-
 block/vhdx.c| 120 ++--
 block/vxhs.c|   5 +-
 cpus.c  |   2 -
 qemu-img.c  |  34 +++-
 qemu-io.c   |   9 ++-
 qemu-nbd.c  |   9 ++-
 qom/object_interfaces.c |  61 
 replay/replay-events.c  |  16 ++
 replay/replay.c |   2 +
 stubs/replay-user.c |   9 +++
 vl.c|  63 -
 stubs/Makefile.objs |   1 +
 tests/qemu-iotests/028  |  11 +++-
 tests/qemu-iotests/028.out  |   1 -
 tests/qemu-iotests/268  |  55 ++
 tests/qemu-iotests/268.out  |   7 +++
 tests/qemu-iotests/270  |  83 +++
 tests/qemu-iotests/270.out  |   9 +++
 tests/qemu-iotests/group|   2 +
 32 files changed, 504 insertions(+), 111 deletions(-)
 create mode 100644 stubs/replay-user.c
 create mode 100755 tests/qemu-iotests/268
 create mode 100644 tests/qemu-iotests/268.out
 create mode 100755 tests/qemu-iotests/270
 create mode 100644 tests/qemu-iotests/270.out

[PULL 04/15] replay: update docs for record/replay with block devices

[Kevin Wolf]

From: Pavel Dovgalyuk 

This patch updates the description of the command lines for using
record/replay with attached block devices.

Signed-off-by: Pavel Dovgalyuk 
Signed-off-by: Kevin Wolf 
---
 docs/replay.txt | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/docs/replay.txt b/docs/replay.txt
index ee6aee9861..ce97c3f72f 100644
--- a/docs/replay.txt
+++ b/docs/replay.txt
@@ -27,7 +27,7 @@ Usage of the record/replay:
  * First, record the execution with the following command line:
 qemu-system-i386 \
  -icount shift=7,rr=record,rrfile=replay.bin \
- -drive file=disk.qcow2,if=none,id=img-direct \
+ -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
  -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
  -device ide-hd,drive=img-blkreplay \
  -netdev user,id=net1 -device rtl8139,netdev=net1 \
@@ -35,7 +35,7 @@ Usage of the record/replay:
  * After recording, you can replay it by using another command line:
 qemu-system-i386 \
  -icount shift=7,rr=replay,rrfile=replay.bin \
- -drive file=disk.qcow2,if=none,id=img-direct \
+ -drive file=disk.qcow2,if=none,snapshot,id=img-direct \
  -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay \
  -device ide-hd,drive=img-blkreplay \
  -netdev user,id=net1 -device rtl8139,netdev=net1 \
@@ -223,7 +223,7 @@ Block devices record/replay module intercepts calls of
 bdrv coroutine functions at the top of block drivers stack.
 To record and replay block operations the drive must be configured
 as following:
- -drive file=disk.qcow2,if=none,id=img-direct
+ -drive file=disk.qcow2,if=none,snapshot,id=img-direct
  -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
  -device ide-hd,drive=img-blkreplay
 
@@ -252,6 +252,12 @@ This snapshot is created at start of recording and 
restored at start
 of replaying. It also can be loaded while replaying to roll back
 the execution.
 
+'snapshot' flag of the disk image must be removed to save the snapshots
+in the overlay (or original image) instead of using the temporary overlay.
+ -drive file=disk.ovl,if=none,id=img-direct
+ -drive driver=blkreplay,if=none,image=img-direct,id=img-blkreplay
+ -device ide-hd,drive=img-blkreplay
+
 Use QEMU monitor to create additional snapshots. 'savevm ' command
 created the snapshot and 'loadvm ' restores it. To prevent corruption
 of the original disk image, use overlay files linked to the original images.
-- 
2.20.1

[PULL 02/15] block: implement bdrv_snapshot_goto for blkreplay

[Kevin Wolf]

From: Pavel Dovgalyuk 

This patch enables making snapshots with blkreplay used in
block devices.
This function is required to make bdrv_snapshot_goto without
calling .bdrv_open which is not implemented.

Signed-off-by: Pavel Dovgalyuk 
Acked-by: Kevin Wolf 
Signed-off-by: Kevin Wolf 
---
 block/blkreplay.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/block/blkreplay.c b/block/blkreplay.c
index 2b7931b940..c96ac8f4bc 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -126,6 +126,12 @@ static int coroutine_fn 
blkreplay_co_flush(BlockDriverState *bs)
 return ret;
 }
 
+static int blkreplay_snapshot_goto(BlockDriverState *bs,
+   const char *snapshot_id)
+{
+return bdrv_snapshot_goto(bs->file->bs, snapshot_id, NULL);
+}
+
 static BlockDriver bdrv_blkreplay = {
 .format_name= "blkreplay",
 .instance_size  = 0,
@@ -140,6 +146,8 @@ static BlockDriver bdrv_blkreplay = {
 .bdrv_co_pwrite_zeroes  = blkreplay_co_pwrite_zeroes,
 .bdrv_co_pdiscard   = blkreplay_co_pdiscard,
 .bdrv_co_flush  = blkreplay_co_flush,
+
+.bdrv_snapshot_goto = blkreplay_snapshot_goto,
 };
 
 static void bdrv_blkreplay_init(void)
-- 
2.20.1

[PULL 03/15] replay: disable default snapshot for record/replay

[Kevin Wolf]

From: Pavel Dovgalyuk 

This patch disables setting '-snapshot' option on by default
in record/replay mode. This is needed for creating vmstates in record
and replay modes.

Signed-off-by: Pavel Dovgalyuk 
Acked-by: Kevin Wolf 
Signed-off-by: Kevin Wolf 
---
 vl.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 0a295e5d77..fce9ce2364 100644
--- a/vl.c
+++ b/vl.c
@@ -1203,7 +1203,7 @@ static void configure_blockdev(BlockdevOptionsQueue 
*bdo_queue,
 qapi_free_BlockdevOptions(bdo->bdo);
 g_free(bdo);
 }
-if (snapshot || replay_mode != REPLAY_MODE_NONE) {
+if (snapshot) {
 qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
   NULL, NULL);
 }
@@ -3066,7 +3066,13 @@ int main(int argc, char **argv, char **envp)
 drive_add(IF_PFLASH, -1, optarg, PFLASH_OPTS);
 break;
 case QEMU_OPTION_snapshot:
-snapshot = 1;
+{
+Error *blocker = NULL;
+snapshot = 1;
+error_setg(, QERR_REPLAY_NOT_SUPPORTED,
+   "-snapshot");
+replay_add_blocker(blocker);
+}
 break;
 case QEMU_OPTION_numa:
 opts = qemu_opts_parse_noisily(qemu_find_opts("numa"),
-- 
2.20.1

[PULL 01/15] block/vhdx: add check for truncated image files

[Kevin Wolf]

From: Peter Lieven 

qemu is currently not able to detect truncated vhdx image files.
Add a basic check if all allocated blocks are reachable at open and
report all errors during bdrv_co_check.

Signed-off-by: Peter Lieven 
Signed-off-by: Kevin Wolf 
---
 block/vhdx.c | 120 +++
 1 file changed, 103 insertions(+), 17 deletions(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index 6a09d0a55c..371f226286 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -24,6 +24,7 @@
 #include "qemu/option.h"
 #include "qemu/crc32c.h"
 #include "qemu/bswap.h"
+#include "qemu/error-report.h"
 #include "vhdx.h"
 #include "migration/blocker.h"
 #include "qemu/uuid.h"
@@ -235,6 +236,9 @@ static int vhdx_region_check(BDRVVHDXState *s, uint64_t 
start, uint64_t length)
 end = start + length;
 QLIST_FOREACH(r, >regions, entries) {
 if (!((start >= r->end) || (end <= r->start))) {
+error_report("VHDX region %" PRIu64 "-%" PRIu64 " overlaps with "
+ "region %" PRIu64 "-%." PRIu64, start, end, r->start,
+ r->end);
 ret = -EINVAL;
 goto exit;
 }
@@ -877,6 +881,95 @@ static void vhdx_calc_bat_entries(BDRVVHDXState *s)
 
 }
 
+static int vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
+{
+BDRVVHDXState *s = bs->opaque;
+int64_t image_file_size = bdrv_getlength(bs->file->bs);
+uint64_t payblocks = s->chunk_ratio;
+uint64_t i;
+int ret = 0;
+
+if (image_file_size < 0) {
+error_report("Could not determinate VHDX image file size.");
+return image_file_size;
+}
+
+for (i = 0; i < s->bat_entries; i++) {
+if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
+PAYLOAD_BLOCK_FULLY_PRESENT) {
+uint64_t offset = s->bat[i] & VHDX_BAT_FILE_OFF_MASK;
+/*
+ * Allow that the last block exists only partially. The VHDX spec
+ * states that the image file can only grow in blocksize 
increments,
+ * but QEMU created images with partial last blocks in the past.
+ */
+uint32_t block_length = MIN(s->block_size,
+bs->total_sectors * BDRV_SECTOR_SIZE - i * s->block_size);
+/*
+ * Check for BAT entry overflow.
+ */
+if (offset > INT64_MAX - s->block_size) {
+error_report("VHDX BAT entry %" PRIu64 " offset overflow.", i);
+ret = -EINVAL;
+if (!errcnt) {
+break;
+}
+(*errcnt)++;
+}
+/*
+ * Check if fully allocated BAT entries do not reside after
+ * end of the image file.
+ */
+if (offset >= image_file_size) {
+error_report("VHDX BAT entry %" PRIu64 " start offset %" PRIu64
+ " points after end of file (%" PRIi64 "). Image"
+ " has probably been truncated.",
+ i, offset, image_file_size);
+ret = -EINVAL;
+if (!errcnt) {
+break;
+}
+(*errcnt)++;
+} else if (offset + block_length > image_file_size) {
+error_report("VHDX BAT entry %" PRIu64 " end offset %" PRIu64
+ " points after end of file (%" PRIi64 "). Image"
+ " has probably been truncated.",
+ i, offset + block_length - 1, image_file_size);
+ret = -EINVAL;
+if (!errcnt) {
+break;
+}
+(*errcnt)++;
+}
+
+/*
+ * verify populated BAT field file offsets against
+ * region table and log entries
+ */
+if (payblocks--) {
+/* payload bat entries */
+int ret2;
+ret2 = vhdx_region_check(s, offset, s->block_size);
+if (ret2 < 0) {
+ret = -EINVAL;
+if (!errcnt) {
+break;
+}
+(*errcnt)++;
+}
+} else {
+payblocks = s->chunk_ratio;
+/*
+ * Once differencing files are supported, verify sector bitmap
+ * blocks here
+ */
+}
+}
+}
+
+return ret;
+}
+
 static void vhdx_close(BlockDriverState *bs)
 {
 BDRVVHDXState *s = bs->opaque;
@@ -981,25 +1074,15 @@ static int vhdx_open(BlockDriverState *bs, QDict 
*options, int flags,
 goto fail;
 }
 
-uint64_t payblocks = s->chunk_ratio;
-/* endian convert, and verify populated BAT field file offsets against
- * region table and log entries */
+/* endian convert populated BAT field entires 

[PATCH v3 4/4] iotests: Add test for failing mirror complete

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Tested-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: John Snow 
---
 tests/qemu-iotests/041 | 44 ++
 tests/qemu-iotests/041.out |  4 ++--
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 8568426311..d7be30b62b 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1121,6 +1121,50 @@ class TestOrphanedSource(iotests.QMPTestCase):
  target='dest-ro')
 self.assert_qmp(result, 'error/class', 'GenericError')
 
+def test_failing_permission_in_complete(self):
+self.assert_no_active_block_jobs()
+
+# Unshare consistent-read on the target
+# (The mirror job does not care)
+result = self.vm.qmp('blockdev-add',
+ driver='blkdebug',
+ node_name='dest-perm',
+ image='dest',
+ unshare_child_perms=['consistent-read'])
+self.assert_qmp(result, 'return', {})
+
+result = self.vm.qmp('blockdev-mirror', job_id='job', device='src',
+ sync='full', target='dest',
+ filter_node_name='mirror-filter')
+self.assert_qmp(result, 'return', {})
+
+# Require consistent-read on the source
+# (We can only add this node once the job has started, or it
+# will complain that it does not want to run on non-root nodes)
+result = self.vm.qmp('blockdev-add',
+ driver='blkdebug',
+ node_name='src-perm',
+ image='src',
+ take_child_perms=['consistent-read'])
+self.assert_qmp(result, 'return', {})
+
+# While completing, mirror will attempt to replace src by
+# dest, which must fail because src-perm requires
+# consistent-read but dest-perm does not share it; thus
+# aborting the job when it is supposed to complete
+self.complete_and_wait('job',
+   completion_error='Operation not permitted')
+
+# Assert that all of our nodes are still there (except for the
+# mirror filter, which should be gone despite the failure)
+nodes = self.vm.qmp('query-named-block-nodes')['return']
+nodes = [node['node-name'] for node in nodes]
+
+for expect in ('src', 'src-perm', 'dest', 'dest-perm'):
+self.assertTrue(expect in nodes, '%s disappeared' % expect)
+self.assertFalse('mirror-filter' in nodes,
+ 'Mirror filter node did not disappear')
+
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2', 'qed'],
  supported_protocols=['file'])
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 2c448b4239..f496be9197 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-..
+...
 --
-Ran 90 tests
+Ran 91 tests
 
 OK
-- 
2.21.0

[PATCH v3 3/4] iotests: Add @error to wait_until_completed

[Max Reitz]

Callers can use this new parameter to expect failure during the
completion process.

Signed-off-by: Max Reitz 
Reviewed-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/iotests.py | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 43759e4e27..7af77ef496 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -770,15 +770,20 @@ class QMPTestCase(unittest.TestCase):
 self.assert_no_active_block_jobs()
 return result
 
-def wait_until_completed(self, drive='drive0', check_offset=True, 
wait=60.0):
+def wait_until_completed(self, drive='drive0', check_offset=True, 
wait=60.0,
+ error=None):
 '''Wait for a block job to finish, returning the event'''
 while True:
 for event in self.vm.get_qmp_events(wait=wait):
 if event['event'] == 'BLOCK_JOB_COMPLETED':
 self.assert_qmp(event, 'data/device', drive)
-self.assert_qmp_absent(event, 'data/error')
-if check_offset:
-self.assert_qmp(event, 'data/offset', 
event['data']['len'])
+if error is None:
+self.assert_qmp_absent(event, 'data/error')
+if check_offset:
+self.assert_qmp(event, 'data/offset',
+event['data']['len'])
+else:
+self.assert_qmp(event, 'data/error', error)
 self.assert_no_active_block_jobs()
 return event
 elif event['event'] == 'JOB_STATUS_CHANGE':
@@ -796,7 +801,8 @@ class QMPTestCase(unittest.TestCase):
 self.assert_qmp(event, 'data/type', 'mirror')
 self.assert_qmp(event, 'data/offset', event['data']['len'])
 
-def complete_and_wait(self, drive='drive0', wait_ready=True):
+def complete_and_wait(self, drive='drive0', wait_ready=True,
+  completion_error=None):
 '''Complete a block job and wait for it to finish'''
 if wait_ready:
 self.wait_ready(drive=drive)
@@ -804,7 +810,7 @@ class QMPTestCase(unittest.TestCase):
 result = self.vm.qmp('block-job-complete', device=drive)
 self.assert_qmp(result, 'return', {})
 
-event = self.wait_until_completed(drive=drive)
+event = self.wait_until_completed(drive=drive, error=completion_error)
 self.assert_qmp(event, 'data/type', 'mirror')
 
 def pause_wait(self, job_id='job0'):
-- 
2.21.0

[PATCH v3 1/4] mirror: Do not dereference invalid pointers

[Max Reitz]

mirror_exit_common() may be called twice (if it is called from
mirror_prepare() and fails, it will be called from mirror_abort()
again).

In such a case, many of the pointers in the MirrorBlockJob object will
already be freed.  This can be seen most reliably for s->target, which
is set to NULL (and then dereferenced by blk_bs()).

Cc: qemu-sta...@nongnu.org
Fixes: 737efc1eda23b904fbe0e66b37715fb0e5c3e58b
Signed-off-by: Max Reitz 
Reviewed-by: John Snow 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/mirror.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index fe984efb90..706d80fced 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -620,11 +620,11 @@ static int mirror_exit_common(Job *job)
 {
 MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
 BlockJob *bjob = >common;
-MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque;
+MirrorBDSOpaque *bs_opaque;
 AioContext *replace_aio_context = NULL;
-BlockDriverState *src = s->mirror_top_bs->backing->bs;
-BlockDriverState *target_bs = blk_bs(s->target);
-BlockDriverState *mirror_top_bs = s->mirror_top_bs;
+BlockDriverState *src;
+BlockDriverState *target_bs;
+BlockDriverState *mirror_top_bs;
 Error *local_err = NULL;
 bool abort = job->ret < 0;
 int ret = 0;
@@ -634,6 +634,11 @@ static int mirror_exit_common(Job *job)
 }
 s->prepared = true;
 
+mirror_top_bs = s->mirror_top_bs;
+bs_opaque = mirror_top_bs->opaque;
+src = mirror_top_bs->backing->bs;
+target_bs = blk_bs(s->target);
+
 if (bdrv_chain_contains(src, target_bs)) {
 bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs);
 }
-- 
2.21.0

[PATCH v3 2/4] blkdebug: Allow taking/unsharing permissions

[Max Reitz]

Sometimes it is useful to be able to add a node to the block graph that
takes or unshare a certain set of permissions for debugging purposes.
This patch adds this capability to blkdebug.

(Note that you cannot make blkdebug release or share permissions that it
needs to take or cannot share, because this might result in assertion
failures in the block layer.  But if the blkdebug node has no parents,
it will not take any permissions and share everything by default, so you
can then freely choose what permissions to take and share.)

Signed-off-by: Max Reitz 
---
 qapi/block-core.json | 14 ++-
 block/blkdebug.c | 91 +++-
 2 files changed, 103 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index f66553aac7..054ce651de 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3453,6 +3453,16 @@
 #
 # @set-state:   array of state-change descriptions
 #
+# @take-child-perms: Permissions to take on @image in addition to what
+#is necessary anyway (which depends on how the
+#blkdebug node is used).  Defaults to none.
+#(since 4.2)
+#
+# @unshare-child-perms: Permissions not to share on @image in addition
+#   to what cannot be shared anyway (which depends
+#   on how the blkdebug node is used).  Defaults
+#   to none.  (since 4.2)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsBlkdebug',
@@ -3462,7 +3472,9 @@
 '*opt-write-zero': 'int32', '*max-write-zero': 'int32',
 '*opt-discard': 'int32', '*max-discard': 'int32',
 '*inject-error': ['BlkdebugInjectErrorOptions'],
-'*set-state': ['BlkdebugSetStateOptions'] } }
+'*set-state': ['BlkdebugSetStateOptions'],
+'*take-child-perms': ['BlockPermission'],
+'*unshare-child-perms': ['BlockPermission'] } }
 
 ##
 # @BlockdevOptionsBlklogwrites:
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5ae96c52b0..6807c03065 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -28,10 +28,14 @@
 #include "qemu/cutils.h"
 #include "qemu/config-file.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qapi/qapi-visit-block-core.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qlist.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/qobject-input-visitor.h"
 #include "sysemu/qtest.h"
 
 typedef struct BDRVBlkdebugState {
@@ -44,6 +48,9 @@ typedef struct BDRVBlkdebugState {
 uint64_t opt_discard;
 uint64_t max_discard;
 
+uint64_t take_child_perms;
+uint64_t unshare_child_perms;
+
 /* For blkdebug_refresh_filename() */
 char *config_file;
 
@@ -344,6 +351,67 @@ static void blkdebug_parse_filename(const char *filename, 
QDict *options,
 qdict_put_str(options, "x-image", filename);
 }
 
+static int blkdebug_parse_perm_list(uint64_t *dest, QDict *options,
+const char *prefix, Error **errp)
+{
+int ret = 0;
+QDict *subqdict = NULL;
+QObject *crumpled_subqdict = NULL;
+Visitor *v = NULL;
+BlockPermissionList *perm_list = NULL, *element;
+Error *local_err = NULL;
+
+qdict_extract_subqdict(options, , prefix);
+if (!qdict_size(subqdict)) {
+goto out;
+}
+
+crumpled_subqdict = qdict_crumple(subqdict, errp);
+if (!crumpled_subqdict) {
+ret = -EINVAL;
+goto out;
+}
+
+v = qobject_input_visitor_new(crumpled_subqdict);
+visit_type_BlockPermissionList(v, NULL, _list, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto out;
+}
+
+for (element = perm_list; element; element = element->next) {
+*dest |= UINT64_C(1) << element->value;
+}
+
+out:
+qapi_free_BlockPermissionList(perm_list);
+visit_free(v);
+qobject_unref(subqdict);
+qobject_unref(crumpled_subqdict);
+return ret;
+}
+
+static int blkdebug_parse_perms(BDRVBlkdebugState *s, QDict *options,
+Error **errp)
+{
+int ret;
+
+ret = blkdebug_parse_perm_list(>take_child_perms, options,
+   "take-child-perms.", errp);
+if (ret < 0) {
+return ret;
+}
+
+ret = blkdebug_parse_perm_list(>unshare_child_perms, options,
+   "unshare-child-perms.", errp);
+if (ret < 0) {
+return ret;
+}
+
+return 0;
+}
+
 static QemuOptsList runtime_opts = {
 .name = "blkdebug",
 .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
@@ -419,6 +487,12 @@ static int blkdebug_open(BlockDriverState *bs, QDict 
*options, int flags,
 /* Set initial state */
 s->state = 1;
 
+/* Parse permissions modifiers before opening the image file */
+ret = blkdebug_parse_perms(s, options, errp);
+if (ret < 0) {
+  

[PATCH v3 0/4] mirror: Do not dereference invalid pointers

[Max Reitz]

Hi,

v2’s cover letter should explain everything:

https://lists.nongnu.org/archive/html/qemu-block/2019-09/msg01079.html


v3:
- Patch 2: Use input visitor as proposed by Vladimir

git-backport-diff against v2:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/4:[] [--] 'mirror: Do not dereference invalid pointers'
002/4:[0041] [FC] 'blkdebug: Allow taking/unsharing permissions'
003/4:[] [--] 'iotests: Add @error to wait_until_completed'
004/4:[] [--] 'iotests: Add test for failing mirror complete'


Max Reitz (4):
  mirror: Do not dereference invalid pointers
  blkdebug: Allow taking/unsharing permissions
  iotests: Add @error to wait_until_completed
  iotests: Add test for failing mirror complete

 qapi/block-core.json  | 14 +-
 block/blkdebug.c  | 91 ++-
 block/mirror.c| 13 +++--
 tests/qemu-iotests/041| 44 +
 tests/qemu-iotests/041.out|  4 +-
 tests/qemu-iotests/iotests.py | 18 ---
 6 files changed, 170 insertions(+), 14 deletions(-)

-- 
2.21.0

Re: [PATCH 08/20] hw/xen/xen_pt_load_rom: Remove unused includes

[Paul Durrant]

On Mon, 14 Oct 2019 at 15:27, Philippe Mathieu-Daudé  wrote:
>
> xen_pt_load_rom.c does not use any of these includes, remove them.
>
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Paul Durrant 

> ---
>  hw/xen/xen_pt_load_rom.c | 4 
>  1 file changed, 4 deletions(-)
>
> diff --git a/hw/xen/xen_pt_load_rom.c b/hw/xen/xen_pt_load_rom.c
> index 307a5c93e2..a50a80837e 100644
> --- a/hw/xen/xen_pt_load_rom.c
> +++ b/hw/xen/xen_pt_load_rom.c
> @@ -3,12 +3,8 @@
>   */
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> -#include "hw/i386/pc.h"
>  #include "qemu/error-report.h"
> -#include "ui/console.h"
>  #include "hw/loader.h"
> -#include "monitor/monitor.h"
> -#include "qemu/range.h"
>  #include "hw/pci/pci.h"
>  #include "xen_pt.h"
>
> --
> 2.21.0
>

Re: [PATCH 0/2] qcow2: Limit total allocation range to INT_MAX

[Kevin Wolf]

Am 10.10.2019 um 12:08 hat Max Reitz geschrieben:
> Hi,
> 
> While looking for why handle_alloc_space() seems to cause issues on
> ppc64le+XFS (performance degradation and data corruption), I spotted
> this other issue.  It isn’t as bad, but still needs fixing.
> 
> See patch 1 for what is fixed and patch 2 for what breaks otherwise.

Thanks, applied to the block branch.

Kevin

Re: [PATCH v4 0/7] iotests: Selfish patches

[Max Reitz]

On 17.09.19 11:19, Max Reitz wrote:
> Hi,
> 
> Again, let me start with a link to an actually explanatory cover letter:
> https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg01102.html
> 
> v3:
> https://lists.nongnu.org/archive/html/qemu-block/2019-08/msg00950.html
> 
> v4:
> - I merged the old patch 1 in the meantime
> 
> - Patch 2: Adjusted the comment to make it more clear that it is
>case_notrun() itself that will not skip the test case, as
>requested by Andrey (I hope it fits what he had in mind, more
>or less); kept the R-bs, because I somehow feel like that’s
>the right thing to do here.
> 
> - Patch 3: The func_wrapper returned by the skip_test_decorator has a
>mandatory argument; make that and its required type explicit
>(with an annotation), as suggested by John
>(Kevin made me aware of the fact that annotations exist since
>Python 3.0, it’s just that they didn’t mean anything back
>then (neither do they really now, but whatever, it’s better
>than a comment))
> 
> - Patch 4: Resolved a conflict because of the change to patch 3

Thanks for the reviews, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max



signature.asc
Description: OpenPGP digital signature

[PATCH 17/20] hw/acpi: Include "hw/mem/nvdimm.h"

[Philippe Mathieu-Daudé]

Both ich9.c and piix4.c use methods/definitions declared in the
NVDIMM device header. Include it.

This fixes (when modifying unrelated headers):

  hw/acpi/ich9.c:507:46: error: use of undeclared identifier 'TYPE_NVDIMM'
if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
 ^
  hw/acpi/ich9.c:508:13: error: implicit declaration of function 
'nvdimm_acpi_plug_cb' is invalid in C99 
[-Werror,-Wimplicit-function-declaration]
nvdimm_acpi_plug_cb(hotplug_dev, dev);
^
  hw/acpi/piix4.c:403:46: error: use of undeclared identifier 'TYPE_NVDIMM'
if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
 ^
  hw/acpi/piix4.c:404:13: error: implicit declaration of function 
'nvdimm_acpi_plug_cb' is invalid in C99 
[-Werror,-Wimplicit-function-declaration]
nvdimm_acpi_plug_cb(hotplug_dev, dev);
^

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/acpi/ich9.c  | 1 +
 hw/acpi/piix4.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index fdd0a6c79e..4e74284b65 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -39,6 +39,7 @@
 
 #include "hw/i386/ich9.h"
 #include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
 
 //#define DEBUG
 
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 5742c3df87..11a3e33e5b 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -39,6 +39,7 @@
 #include "hw/acpi/cpu.h"
 #include "hw/hotplug.h"
 #include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
 #include "hw/acpi/memory_hotplug.h"
 #include "hw/acpi/acpi_dev_interface.h"
 #include "hw/xen/xen.h"
-- 
2.21.0

[PATCH 14/20] hw/pci-host/q35: Include "qemu/range.h"

[Philippe Mathieu-Daudé]

The MCHPCIState structure uses the Range type which is declared in
"qemu/range.h". Include it.

This fixes (when modifying unrelated headers):

  In file included from hw/pci-host/q35.c:32:
  include/hw/pci-host/q35.h:57:11: error: field has incomplete type 'Range' 
(aka 'struct Range')
  Range pci_hole;
^
  include/qemu/typedefs.h:116:16: note: forward declaration of 'struct Range'
  typedef struct Range Range;
 ^

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/pci-host/q35.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index b3bcf2e632..79a88d67b1 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -32,6 +32,7 @@
 #include "hw/acpi/ich9.h"
 #include "hw/pci-host/pam.h"
 #include "hw/i386/intel_iommu.h"
+#include "qemu/range.h"
 
 #define TYPE_Q35_HOST_DEVICE "q35-pcihost"
 #define Q35_HOST_DEVICE(obj) \
-- 
2.21.0

[PATCH 13/20] hw/timer/hpet: Include "exec/address-spaces.h"

[Philippe Mathieu-Daudé]

hw/timer/hpet.c calls address_space_stl_le() declared in
"exec/address-spaces.h". Include it.

This fixes (when modifying unrelated headers):

  hw/timer/hpet.c:210:31: error: use of undeclared identifier 
'address_space_memory'
  address_space_stl_le(_space_memory, timer->fsb >> 32,
   ^~~~

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/timer/hpet.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 4772cccfe3..6589d63ebb 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -35,6 +35,7 @@
 #include "hw/timer/mc146818rtc.h"
 #include "migration/vmstate.h"
 #include "hw/timer/i8254.h"
+#include "exec/address-spaces.h"
 
 //#define HPET_DEBUG
 #ifdef HPET_DEBUG
-- 
2.21.0

[PATCH 19/20] hw/pci-host/q35: Remove unused includes

[Philippe Mathieu-Daudé]

Only q35.c requires declarations from "hw/i386/pc.h", move it there.
Remove all the includes not used by "q35.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/pci-host/q35.c | 1 +
 include/hw/pci-host/q35.h | 7 ---
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 158d270b9f..918843d373 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -29,6 +29,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/i386/pc.h"
 #include "hw/pci-host/q35.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
index 79a88d67b1..534d90dbaf 100644
--- a/include/hw/pci-host/q35.h
+++ b/include/hw/pci-host/q35.h
@@ -22,16 +22,9 @@
 #ifndef HW_Q35_H
 #define HW_Q35_H
 
-#include "hw/isa/isa.h"
-#include "hw/sysbus.h"
-#include "hw/i386/pc.h"
-#include "hw/isa/apm.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pcie_host.h"
-#include "hw/acpi/acpi.h"
-#include "hw/acpi/ich9.h"
 #include "hw/pci-host/pam.h"
-#include "hw/i386/intel_iommu.h"
 #include "qemu/range.h"
 
 #define TYPE_Q35_HOST_DEVICE "q35-pcihost"
-- 
2.21.0

[PATCH 20/20] hw/i386/pc: Clean up includes

[Philippe Mathieu-Daudé]

Various headers are not required by hw/i386/pc.h:

 - "qemu/range.h"
 - "qemu/bitmap.h"
 - "qemu/module.h"
 - "exec/memory.h"
 - "hw/pci/pci.h"
 - "hw/mem/pc-dimm.h"
 - "hw/mem/nvdimm.h"
 - "net/net.h"

Remove them.

Add 3 headers that were missing:

 - "hw/hotplug.h"

   PCMachineState::acpi_dev is of type HotplugHandler

 - "qemu/notify.h"

   PCMachineState::machine_done is of type Notifier

 - "qapi/qapi-types-common.h"

   PCMachineState::vmport/smm is of type OnOffAuto

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/i386/pc.h | 12 +++-
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 6df4f4b6fb..e5c2dc9081 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -1,21 +1,15 @@
 #ifndef HW_PC_H
 #define HW_PC_H
 
-#include "exec/memory.h"
+#include "qemu/notify.h"
+#include "qapi/qapi-types-common.h"
 #include "hw/boards.h"
 #include "hw/isa/isa.h"
 #include "hw/block/fdc.h"
 #include "hw/block/flash.h"
-#include "net/net.h"
 #include "hw/i386/ioapic.h"
-
-#include "qemu/range.h"
-#include "qemu/bitmap.h"
-#include "qemu/module.h"
-#include "hw/pci/pci.h"
-#include "hw/mem/pc-dimm.h"
-#include "hw/mem/nvdimm.h"
 #include "hw/acpi/acpi_dev_interface.h"
+#include "hw/hotplug.h"
 
 #define HPET_INTCAP "hpet-intcap"
 
-- 
2.21.0

[PATCH 12/20] hw/acpi/cpu_hotplug: Include "hw/pci/pci.h"

[Philippe Mathieu-Daudé]

hw/acpi/cpu_hotplug.c calls pci_address_space_io(). Include
"hw/pci/pci.h" which declares it.

This fixes (when modifying unrelated headers):

  hw/acpi/cpu_hotplug.c:103:28: error: implicit declaration of function 
'pci_address_space_io' is invalid in C99 
[-Werror,-Wimplicit-function-declaration]
  MemoryRegion *parent = pci_address_space_io(PCI_DEVICE(gpe_cpu->device));
 ^

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/acpi/cpu_hotplug.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 6e8293aac9..7fb65d9065 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -14,6 +14,7 @@
 #include "qapi/error.h"
 #include "hw/core/cpu.h"
 #include "hw/i386/pc.h"
+#include "hw/pci/pci.h"
 #include "qemu/error-report.h"
 
 #define CPU_EJECT_METHOD "CPEJ"
-- 
2.21.0

[PATCH 18/20] hw/i386: Include "hw/mem/nvdimm.h"

[Philippe Mathieu-Daudé]

All this files use methods/definitions declared in the NVDIMM
device header. Include it.

This fixes (when modifying unrelated headers):

  hw/i386/acpi-build.c:2733:9: error: implicit declaration of function 
'nvdimm_build_acpi' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
^
  hw/i386/pc.c:1996:61: error: use of undeclared identifier 'TYPE_NVDIMM'
const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
^
  hw/i386/pc.c:2032:55: error: use of undeclared identifier 'TYPE_NVDIMM'
bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
  ^
  hw/i386/pc.c:2040:9: error: implicit declaration of function 'nvdimm_plug' is 
invalid in C99 [-Werror,-Wimplicit-function-declaration]
nvdimm_plug(ms->nvdimms_state);
^
  hw/i386/pc.c:2040:9: error: this function declaration is not a prototype 
[-Werror,-Wstrict-prototypes]
nvdimm_plug(ms->nvdimms_state);
^
  hw/i386/pc.c:2065:42: error: use of undeclared identifier 'TYPE_NVDIMM'
if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
 ^
  hw/i386/pc_i440fx.c:307:9: error: implicit declaration of function 
'nvdimm_init_acpi_state' is invalid in C99 
[-Werror,-Wimplicit-function-declaration]
nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
^
  hw/i386/pc_q35.c:332:9: error: implicit declaration of function 
'nvdimm_init_acpi_state' is invalid in C99 
[-Werror,-Wimplicit-function-declaration]
nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
^

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/i386/acpi-build.c | 1 +
 hw/i386/pc.c | 1 +
 hw/i386/pc_piix.c| 1 +
 hw/i386/pc_q35.c | 1 +
 4 files changed, 4 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4e0f9f425a..ac46936f63 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -48,6 +48,7 @@
 #include "hw/timer/mc146818rtc_regs.h"
 #include "migration/vmstate.h"
 #include "hw/mem/memory-device.h"
+#include "hw/mem/nvdimm.h"
 #include "sysemu/numa.h"
 #include "sysemu/reset.h"
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bcda50efcc..cff330802d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -73,6 +73,7 @@
 #include "hw/boards.h"
 #include "acpi-build.h"
 #include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
 #include "qapi/error.h"
 #include "qapi/qapi-visit-common.h"
 #include "qapi/visitor.h"
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 6824b72124..8651b6e2ec 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -58,6 +58,7 @@
 #include "migration/misc.h"
 #include "kvm_i386.h"
 #include "sysemu/numa.h"
+#include "hw/mem/nvdimm.h"
 
 #define MAX_IDE_BUS 2
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 8fad20f314..91ba231ef1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -53,6 +53,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "sysemu/numa.h"
+#include "hw/mem/nvdimm.h"
 
 /* ICH9 AHCI has 6 ports */
 #define MAX_SATA_PORTS 6
-- 
2.21.0

[PATCH 09/20] hw/alpha/alpha_sys: Remove unused "hw/ide.h" header

[Philippe Mathieu-Daudé]

alpha_sys.h does not use anything from the "hw/ide.h" header.
Remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/alpha/alpha_sys.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/alpha/alpha_sys.h b/hw/alpha/alpha_sys.h
index 4e127a6de8..9991535c0d 100644
--- a/hw/alpha/alpha_sys.h
+++ b/hw/alpha/alpha_sys.h
@@ -6,7 +6,6 @@
 #include "target/alpha/cpu-qom.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_host.h"
-#include "hw/ide.h"
 #include "hw/i386/pc.h"
 
 
-- 
2.21.0

[PATCH 16/20] hw/pci-host/piix: Include "qemu/range.h"

[Philippe Mathieu-Daudé]

hw/pci-host/piix.c calls various functions from the Range API.
Include "qemu/range.h" which declares them.

This fixes (when modifying unrelated headers):

  hw/pci-host/i440fx.c:54:11: error: field has incomplete type 'Range' (aka 
'struct Range')
  Range pci_hole;
   ^
  include/qemu/typedefs.h:116:16: note: forward declaration of 'struct Range'
  typedef struct Range Range;
 ^
  hw/pci-host/i440fx.c:126:9: error: implicit declaration of function 
'ranges_overlap' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
  if (ranges_overlap(address, len, I440FX_PAM, I440FX_PAM_SIZE) ||
  ^
  hw/pci-host/i440fx.c:126:9: error: this function declaration is not a 
prototype [-Werror,-Wstrict-prototypes]
  hw/pci-host/i440fx.c:127:9: error: implicit declaration of function 
'range_covers_byte' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
  range_covers_byte(address, len, I440FX_SMRAM)) {
  ^
  hw/pci-host/i440fx.c:127:9: error: this function declaration is not a 
prototype [-Werror,-Wstrict-prototypes]
  hw/pci-host/i440fx.c:189:13: error: implicit declaration of function 
'range_is_empty' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
  val64 = range_is_empty(>pci_hole) ? 0 : range_lob(>pci_hole);
  ^

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/pci-host/piix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 135c645535..76ed252a60 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/range.h"
 #include "hw/i386/pc.h"
 #include "hw/irq.h"
 #include "hw/pci/pci.h"
-- 
2.21.0

[PATCH 15/20] hw/i2c/smbus_ich9: Include "qemu/range.h"

[Philippe Mathieu-Daudé]

hw/i2c/smbus_ich9.c calls range_covers_byte(). Include "qemu/range.h"
which declares it.

This fixes (when modifying unrelated headers):

  hw/i2c/smbus_ich9.c:66:9: error: implicit declaration of function 
'range_covers_byte' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
  if (range_covers_byte(address, len, ICH9_SMB_HOSTC)) {
  ^

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/i2c/smbus_ich9.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index fd50fb851a..48f1ff4191 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -21,6 +21,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/range.h"
 #include "hw/i2c/pm_smbus.h"
 #include "hw/pci/pci.h"
 #include "migration/vmstate.h"
-- 
2.21.0

[PATCH 08/20] hw/xen/xen_pt_load_rom: Remove unused includes

[Philippe Mathieu-Daudé]

xen_pt_load_rom.c does not use any of these includes, remove them.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/xen/xen_pt_load_rom.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/hw/xen/xen_pt_load_rom.c b/hw/xen/xen_pt_load_rom.c
index 307a5c93e2..a50a80837e 100644
--- a/hw/xen/xen_pt_load_rom.c
+++ b/hw/xen/xen_pt_load_rom.c
@@ -3,12 +3,8 @@
  */
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "hw/i386/pc.h"
 #include "qemu/error-report.h"
-#include "ui/console.h"
 #include "hw/loader.h"
-#include "monitor/monitor.h"
-#include "qemu/range.h"
 #include "hw/pci/pci.h"
 #include "xen_pt.h"
 
-- 
2.21.0

[PATCH 11/20] hw/hppa/machine: Include "net/net.h"

[Philippe Mathieu-Daudé]

hw/hppa/machine.c uses NICInfo variables which are declared in
"net/net.h". Include it.

This fixes (when modifying unrelated headers):

  hw/hppa/machine.c:126:21: error: use of undeclared identifier 'nb_nics'
  for (i = 0; i < nb_nics; i++) {
  ^
  hw/hppa/machine.c:127:30: error: use of undeclared identifier 'nd_table'
  pci_nic_init_nofail(_table[i], pci_bus, "e1000", NULL);
   ^

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/hppa/machine.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 7e23675429..6c55ed0af1 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -20,6 +20,7 @@
 #include "qemu/units.h"
 #include "qapi/error.h"
 #include "qemu/log.h"
+#include "net/net.h"
 
 #define MAX_IDE_BUS 2
 
-- 
2.21.0

[PATCH 04/20] hw/i386/ioapic_internal: Remove unused "hw/i386/ioapic.h" header

[Philippe Mathieu-Daudé]

The "ioapic_internal.h" does not use anything from
"hw/i386/ioapic.h", remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/i386/ioapic_internal.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/hw/i386/ioapic_internal.h 
b/include/hw/i386/ioapic_internal.h
index d46c87c510..fe06938bda 100644
--- a/include/hw/i386/ioapic_internal.h
+++ b/include/hw/i386/ioapic_internal.h
@@ -23,7 +23,6 @@
 #define QEMU_IOAPIC_INTERNAL_H
 
 #include "exec/memory.h"
-#include "hw/i386/ioapic.h"
 #include "hw/sysbus.h"
 #include "qemu/notify.h"
 
-- 
2.21.0

[PATCH 03/20] hw/input/pckbd: Remove unused "hw/i386/pc.h" header

[Philippe Mathieu-Daudé]

The keyboard controller model don't need anything from
"hw/i386/pc.h". Remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/input/pckbd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index f0acfd86f7..2f09f780ba 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -26,7 +26,6 @@
 #include "qemu/log.h"
 #include "hw/isa/isa.h"
 #include "migration/vmstate.h"
-#include "hw/i386/pc.h"
 #include "hw/input/ps2.h"
 #include "hw/irq.h"
 #include "hw/input/i8042.h"
-- 
2.21.0

[PATCH 07/20] hw/i386/intel_iommu: Remove unused includes

[Philippe Mathieu-Daudé]

intel_iommu.h does not use any of these includes, remove them.

Signed-off-by: Philippe Mathieu-Daudé 
---
 include/hw/i386/intel_iommu.h | 4 
 1 file changed, 4 deletions(-)

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 66b931e526..a1c4afcda5 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -22,11 +22,7 @@
 #ifndef INTEL_IOMMU_H
 #define INTEL_IOMMU_H
 
-#include "sysemu/dma.h"
 #include "hw/i386/x86-iommu.h"
-#include "hw/i386/ioapic.h"
-#include "hw/pci/msi.h"
-#include "hw/sysbus.h"
 #include "qemu/iova-tree.h"
 
 #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
-- 
2.21.0

[PATCH 10/20] hw/alpha/dp264: Include "net/net.h"

[Philippe Mathieu-Daudé]

hw/alpha/dp264.c uses NICInfo variables which are declared in
"net/net.h". Include it.

This fixes (when modifying unrelated headers):

  hw/alpha/dp264.c:89:21: error: use of undeclared identifier 'nb_nics'
  for (i = 0; i < nb_nics; i++) {
  ^
  hw/alpha/dp264.c:90:30: error: use of undeclared identifier 'nd_table'
  pci_nic_init_nofail(_table[i], pci_bus, "e1000", NULL);
   ^

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/alpha/dp264.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
index 51feee8558..013a9d3510 100644
--- a/hw/alpha/dp264.c
+++ b/hw/alpha/dp264.c
@@ -20,6 +20,7 @@
 #include "hw/isa/superio.h"
 #include "hw/dma/i8257.h"
 #include "qemu/cutils.h"
+#include "net/net.h"
 
 #define MAX_IDE_BUS 2
 
-- 
2.21.0

[PATCH 06/20] hw/usb/dev-storage: Remove unused "ui/console.h" header

[Philippe Mathieu-Daudé]

The USB models related to storage don't need anything from
"ui/console.h". Remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/usb/dev-storage.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 8545193488..a2ff52d3e5 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -17,7 +17,6 @@
 #include "desc.h"
 #include "hw/qdev-properties.h"
 #include "hw/scsi/scsi.h"
-#include "ui/console.h"
 #include "migration/vmstate.h"
 #include "monitor/monitor.h"
 #include "sysemu/sysemu.h"
-- 
2.21.0

[PATCH 05/20] hw/timer: Remove unused "ui/console.h" header

[Philippe Mathieu-Daudé]

The timer models don't need anything from "ui/console.h".
Remove it.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/timer/hpet.c | 1 -
 hw/timer/twl92230.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 1ddae4e7d7..4772cccfe3 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -27,7 +27,6 @@
 #include "qemu/osdep.h"
 #include "hw/i386/pc.h"
 #include "hw/irq.h"
-#include "ui/console.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
diff --git a/hw/timer/twl92230.c b/hw/timer/twl92230.c
index 63bd13d2ca..d0011be89e 100644
--- a/hw/timer/twl92230.c
+++ b/hw/timer/twl92230.c
@@ -27,7 +27,6 @@
 #include "migration/qemu-file-types.h"
 #include "migration/vmstate.h"
 #include "sysemu/sysemu.h"
-#include "ui/console.h"
 #include "qemu/bcd.h"
 #include "qemu/module.h"
 
-- 
2.21.0

[PATCH 01/20] vl: Add missing "hw/boards.h" include

[Philippe Mathieu-Daudé]

vl.c calls machine_usb() declared in "hw/boards.h". Include it.

This fixes (when modifying unrelated headers):

  vl.c:1283:10: error: implicit declaration of function 'machine_usb' is 
invalid in C99 [-Werror,-Wimplicit-function-declaration]
  if (!machine_usb(current_machine)) {
   ^
  vl.c:1283:10: error: this function declaration is not a prototype 
[-Werror,-Wstrict-prototypes]
  vl.c:1283:22: error: use of undeclared identifier 'current_machine'
  if (!machine_usb(current_machine)) {
   ^

Signed-off-by: Philippe Mathieu-Daudé 
---
 vl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/vl.c b/vl.c
index 002bf4919e..e85b31df1b 100644
--- a/vl.c
+++ b/vl.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/units.h"
+#include "hw/boards.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "qemu-version.h"
-- 
2.21.0

[PATCH 02/20] hw/southbridge/ich9: Removed unused headers

[Philippe Mathieu-Daudé]

The ICH9 chipset is not X86/PC specific.

These files don't use anything declared by the "hw/i386/pc.h"
or "hw/i386/ioapic.h" headers. Remove them.

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/acpi/ich9.c | 1 -
 hw/isa/lpc_ich9.c  | 2 --
 include/hw/i386/ich9.h | 1 -
 3 files changed, 4 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 2034dd749e..fdd0a6c79e 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -27,7 +27,6 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
-#include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
 #include "migration/vmstate.h"
 #include "qemu/timer.h"
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 17c292e306..61cee2ae3a 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -35,10 +35,8 @@
 #include "hw/isa/isa.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
-#include "hw/i386/pc.h"
 #include "hw/irq.h"
 #include "hw/isa/apm.h"
-#include "hw/i386/ioapic.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bridge.h"
 #include "hw/i386/ich9.h"
diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index 72e803f6e2..a98d10b252 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -5,7 +5,6 @@
 #include "hw/sysbus.h"
 #include "hw/i386/pc.h"
 #include "hw/isa/apm.h"
-#include "hw/i386/ioapic.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci_bridge.h"
-- 
2.21.0

[PATCH 00/20] hw: Clean up hw/i386 headers (and few alpha/hppa)

[Philippe Mathieu-Daudé]

This is a follow-up of Markus's cleanup series:
Tame a few "touch this, recompile the world"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg635748.html

This part is mostly restricted to X86, but since some file from the
Alpha/PA-RISC machines include "hw/i386/pc.h" I had to fix them
too.

Eventually I'll succeed at removing hw/i386/ dependency on non-X86
platforms (Quest I started 2 years ago...).

Regards,

Phil.

Philippe Mathieu-Daudé (20):
  vl: Add missing "hw/boards.h" include
  hw/southbridge/ich9: Removed unused headers
  hw/input/pckbd: Remove unused "hw/i386/pc.h" header
  hw/i386/ioapic_internal: Remove unused "hw/i386/ioapic.h" header
  hw/timer: Remove unused "ui/console.h" header
  hw/usb/dev-storage: Remove unused "ui/console.h" header
  hw/i386/intel_iommu: Remove unused includes
  hw/xen/xen_pt_load_rom: Remove unused includes
  hw/alpha/alpha_sys: Remove unused "hw/ide.h" header
  hw/alpha/dp264: Include "net/net.h"
  hw/hppa/machine: Include "net/net.h"
  hw/acpi/cpu_hotplug: Include "hw/pci/pci.h"
  hw/timer/hpet: Include "exec/address-spaces.h"
  hw/pci-host/q35: Include "qemu/range.h"
  hw/i2c/smbus_ich9: Include "qemu/range.h"
  hw/pci-host/piix: Include "qemu/range.h"
  hw/acpi: Include "hw/mem/nvdimm.h"
  hw/i386: Include "hw/mem/nvdimm.h"
  hw/pci-host/q35: Remove unused includes
  hw/i386/pc: Clean up includes

 hw/acpi/cpu_hotplug.c |  1 +
 hw/acpi/ich9.c|  2 +-
 hw/acpi/piix4.c   |  1 +
 hw/alpha/alpha_sys.h  |  1 -
 hw/alpha/dp264.c  |  1 +
 hw/hppa/machine.c |  1 +
 hw/i2c/smbus_ich9.c   |  1 +
 hw/i386/acpi-build.c  |  1 +
 hw/i386/pc.c  |  1 +
 hw/i386/pc_piix.c |  1 +
 hw/i386/pc_q35.c  |  1 +
 hw/input/pckbd.c  |  1 -
 hw/isa/lpc_ich9.c |  2 --
 hw/pci-host/piix.c|  1 +
 hw/pci-host/q35.c |  1 +
 hw/timer/hpet.c   |  2 +-
 hw/timer/twl92230.c   |  1 -
 hw/usb/dev-storage.c  |  1 -
 hw/xen/xen_pt_load_rom.c  |  4 
 include/hw/i386/ich9.h|  1 -
 include/hw/i386/intel_iommu.h |  4 
 include/hw/i386/ioapic_internal.h |  1 -
 include/hw/i386/pc.h  | 12 +++-
 include/hw/pci-host/q35.h |  8 +---
 vl.c  |  1 +
 25 files changed, 18 insertions(+), 34 deletions(-)

-- 
2.21.0

Re: [PATCH v2] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK

[Kevin Wolf]

Am 14.10.2019 um 13:22 hat Alberto Garcia geschrieben:
> On Mon 14 Oct 2019 11:26:01 AM CEST, Kevin Wolf wrote:
> >> Signed-off-by: Alberto Garcia 
> >
> > Thanks, applied to the block branch.
> 
> I'm a bit late now, but a possible trivial optimization is to flip the
> conditions, because checking the flag should be faster than checking the
> alignment and it's going to be false in almost all cases:
> 
> if ((flags & BDRV_REQ_NO_FALLBACK) &&
> !QEMU_IS_ALIGNED(offset | bytes, align)) {
> return -ENOTSUP;
> }
> 
> Feel free to edit the commit if you want.

Ok, I'll update it.

Kevin

Re: [PATCH 1/4] vl: Split off user_creatable_print_help()

[Eric Blake]

On 10/14/19 4:55 AM, Kevin Wolf wrote:

Am 11.10.2019 um 23:35 hat Eric Blake geschrieben:

On 10/11/19 3:55 PM, Kevin Wolf wrote:

Printing help for --object is something that we don't only want in the


s/don't/not/


Can someone send a fix for the English grammar? It's obviously broken
and doesn't know what it wants. Actually, maybe do-support was a bad
idea and we should just revert it and restore consistent use of proper
verb-second word order?


Lol



(Hm, actually, since this seems to negate "only" rather than the verb,
does "...that we want not only in..." work without patching the
grammar?)


Yes, that formulation also works.



(Thanks for the correction.)

Kevin


system emulator, but also in tools that support --object. Move it into a
separate function in qom/object_interfaces.c to make the code accessible
for tools.

Signed-off-by: Kevin Wolf 


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

Re: [PATCH 4/5] iotests: Skip "make check-block" if QEMU does not support virtio-blk

[Kevin Wolf]

Am 14.10.2019 um 13:27 hat Thomas Huth geschrieben:
> On 14/10/2019 13.21, Kevin Wolf wrote:
> > Am 11.10.2019 um 16:50 hat Thomas Huth geschrieben:
> >> The next patch is going to add some python-based tests to the "auto"
> >> group, and these tests require virtio-blk to work properly. Running
> >> iotests without virtio-blk likely does not make too much sense anyway,
> >> so instead of adding a check for the availability of virtio-blk to each
> >> and every test (which does not sound very appealing), let's rather add
> >> a check for this at the top level in the check-block.sh script instead
> >> (so that it is possible to run "make check" without the "check-block"
> >> part for qemu-system-tricore for example).
> >>
> >> Signed-off-by: Thomas Huth 
> >> ---
> >>  tests/check-block.sh | 16 +++-
> >>  1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tests/check-block.sh b/tests/check-block.sh
> >> index 679aedec50..7582347ec2 100755
> >> --- a/tests/check-block.sh
> >> +++ b/tests/check-block.sh
> >> @@ -26,10 +26,24 @@ if grep -q "CFLAGS.*-fsanitize" config-host.mak 
> >> 2>/dev/null ; then
> >>  exit 0
> >>  fi
> >>  
> >> -if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then
> >> +if [ -n "$QEMU_PROG" ]; then
> >> +qemu_prog="$QEMU_PROG"
> >> +else
> >> +for binary in *-softmmu/qemu-system-* ; do
> >> +if [ -x "$binary" ]; then
> >> +qemu_prog="$binary"
> >> +break
> >> +fi
> > 
> > Wouldn't it be better to check the availability of virtio-blk here, so
> > that if the current binary doesn't support it, we keep searching and
> > maybe pick up a different binary that supports it?
> 
> That's a good idea, indeed, but then I also need to adjust the code in
> the "check" script accordingly.

I see. If this just copies the logic that qemu-iotests already uses, I
think I would be okay with taking it as is for now.

> > Or actually, should we work with a whitelist?
> 
> I don't think that a hard-coded list will work well: Since we introduced
> the Kconfig build system, it's now possible for example to also build an
> qemu-system-aarch64 binary that does not contain any of the boards that
> support virtio. So while virtio-blk is available by default in
> qemu-system-aarch64, some builds might not contain it.

Hm, good point. I'm just worried that the default config will end up
running the tests on a machine type where it works, but if you use the
wrong set of configure options, you end up with a setup where 'make
check' fails because it uses a machine type that the iotests don't
support.

Kevin

Re: [PATCH v3 07/16] qcow2: Write v3-compliant snapshot list on upgrade

[Max Reitz]

On 14.10.19 15:53, Eric Blake wrote:
> On 10/14/19 3:45 AM, Max Reitz wrote:
> 
 +    need_snapshot_update = false;
 +    for (i = 0; i < s->nb_snapshots; i++) {
 +    if (s->snapshots[i].extra_data_size <
 +    sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
 +    sizeof_field(QCowSnapshotExtraData, disk_size))
>>>
>>> Shorter as:
>>> if (s->snapshots[i].extra_data_size < sizeof(QCowSnapshotExtraData))
>>>
>>> but that's stylistic, so R-b still stands.
>>
>> Yes, but if we ever add fields to QCowSnapshotExtraData, we shouldn’t
>> count them here.  Therefore, I think we need to count exactly the fields
>> that the standard says are mandatory in v3.
> 
> If we ever add more fields, I'd prefer that we did something like:
> 
> struct QCowSnapshotExtraV3Minimum {
>     uint64_t vm_state_size_large;
>     uint64_t disk_size;
> };
> struct QCow3SnapshotExtraFull {
>     struct QCowSnapshotExtraV3Minimum base;
>     new fields...;
> };
> 
> and use sane naming to get at extra members based on the expected types,
> rather than trying to piecemeal portions of a type based on size.
> 
> Until we actually DO add more fields, why do we have to complicate the
> current code?

I don’t think it’s complicated, I find it very expressive.  There are
two fields, we check whether they are present; why, that’s obvious,
because those are the ones mandated by the standard.

If we just checked against sizeof(QCowSnapshotExtraData), I’d (as a
naïve reader) ask myself what that has to do with the standard.  I’d
need to look into the structure definition and see that it currently
contains exactly the fields that are mandated by the standard, and then
I’d think “But what if we ever add fields to this structure?”  The more
verbose version avoids this problem.

And I’m not really inclined to take your proposal above right now,
because that would mean having to touch a lot of code.  I prefer this
more verbose code over that.


Also, you explicitly agreed that the code in this patch is preferable to
a plain sizeof(extra) in v2:

https://lists.nongnu.org/archive/html/qemu-block/2019-08/msg00942.html

Max



signature.asc
Description: OpenPGP digital signature

Re: [RFC v5 025/126] scripts: add coccinelle script to use auto propagated errp

[Eric Blake]

On 10/14/19 3:19 AM, Vladimir Sementsov-Ogievskiy wrote:


+|
+-    error_propagate(errp, local_err);
+)
+ ...>
+ }
+


It looks like once this script is run, error_propagate_prepend() will have no 
clients.


No, it still have a bit, when working with error_copy, and/or moving errors 
from/to structures.


No public clients. So can we turn it into a static function only used by 
error.c?





Is there a non-generated cleanup patch that removes it (and once it is removed, 
it can also be removed from the .cocci script as no further clients will 
reappear later)?


Maybe.



Basically, if we can get error_propagate out of error.h, that's a good 
sign.  But it's not a show-stopper if we can't do it for some legitimate 
reason (such a reason might include that the definition of the 
ERRP_AUTO_PROPAGATE macro is easier to write if error_propagate remains 
in the .h), so we'll just have to see what is possible.


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

Re: [PATCH v3 07/16] qcow2: Write v3-compliant snapshot list on upgrade

[Eric Blake]

On 10/14/19 3:45 AM, Max Reitz wrote:


+    need_snapshot_update = false;
+    for (i = 0; i < s->nb_snapshots; i++) {
+    if (s->snapshots[i].extra_data_size <
+    sizeof_field(QCowSnapshotExtraData, vm_state_size_large) +
+    sizeof_field(QCowSnapshotExtraData, disk_size))


Shorter as:
if (s->snapshots[i].extra_data_size < sizeof(QCowSnapshotExtraData))

but that's stylistic, so R-b still stands.


Yes, but if we ever add fields to QCowSnapshotExtraData, we shouldn’t
count them here.  Therefore, I think we need to count exactly the fields
that the standard says are mandatory in v3.


If we ever add more fields, I'd prefer that we did something like:

struct QCowSnapshotExtraV3Minimum {
uint64_t vm_state_size_large;
uint64_t disk_size;
};
struct QCow3SnapshotExtraFull {
struct QCowSnapshotExtraV3Minimum base;
new fields...;
};

and use sane naming to get at extra members based on the expected types, 
rather than trying to piecemeal portions of a type based on size.


Until we actually DO add more fields, why do we have to complicate the 
current code?


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

Re: [PULL 00/36] Block patches

[Peter Maydell]

On Thu, 10 Oct 2019 at 12:43, Max Reitz  wrote:
>
> The following changes since commit 98b2e3c9ab3abfe476a2b02f8f51813edb90e72d:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' 
> into staging (2019-10-08 16:08:35 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/XanClic/qemu.git tags/pull-block-2019-10-10
>
> for you to fetch changes up to 35f05b2e2ee59e077bf949057dc0959ddd6e5249:
>
>   iotests/162: Fix for newer Linux 5.3+ (2019-10-10 12:13:23 +0200)
>
> 
> Block patches:
> - Parallelized request handling for qcow2
> - Backup job refactoring to use a filter node instead of before-write
>   notifiers
> - Add discard accounting information to file-posix nodes
> - Allow trivial reopening of nbd nodes
> - Some iotest fixes
>

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2
for any user-visible changes.

-- PMM

[PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is a fix for persistent bitmaps managing: we must check existent
but not yet stored bitmaps for qcow2-related constraints, like maximum
number of bitmaps in qcow2 image.

v2:

01: change assertion to error-return at function start
Be free to add
Reported-by: aihua liang 
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636
if it's appropriate
02: new test
Ohh, it takes about 4 minutes. Be free to drop it, as I doubt that
it worth to have. The case is simple, we may live without a
test.

Vladimir Sementsov-Ogievskiy (2):
  qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap
  iotests: add 269 to check maximum of bitmaps in qcow2

 block/qcow2-bitmap.c   | 41 +++--
 tests/qemu-iotests/269 | 47 ++
 tests/qemu-iotests/269.out |  3 +++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 69 insertions(+), 23 deletions(-)
 create mode 100755 tests/qemu-iotests/269
 create mode 100644 tests/qemu-iotests/269.out

-- 
2.21.0

[PATCH v2 2/2] iotests: add 269 to check maximum of bitmaps in qcow2

[Vladimir Sementsov-Ogievskiy]

Check that it's impossible to create more persistent bitmaps than qcow2
supports.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/269 | 47 ++
 tests/qemu-iotests/269.out |  3 +++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 51 insertions(+)
 create mode 100755 tests/qemu-iotests/269
 create mode 100644 tests/qemu-iotests/269.out

diff --git a/tests/qemu-iotests/269 b/tests/qemu-iotests/269
new file mode 100755
index 00..cf14d519ee
--- /dev/null
+++ b/tests/qemu-iotests/269
@@ -0,0 +1,47 @@
+#!/usr/bin/env python
+#
+# Test exceeding dirty bitmaps maximum amount in qcow2 image
+#
+# Copyright (c) 2019 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import iotests
+from iotests import qemu_img_create, file_path, log, filter_qmp_event
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
+img = file_path('img')
+size = 64 * 1024
+
+qemu_img_create('-f', iotests.imgfmt, img, str(size))
+vm = iotests.VM().add_drive(img)
+vm.launch()
+
+# Look at block/qcow2.h
+QCOW2_MAX_BITMAPS = 65535
+
+for i in range(QCOW2_MAX_BITMAPS):
+result = vm.qmp('block-dirty-bitmap-add', node='drive0',
+name='bitmap{}'.format(i), persistent=True)
+assert result['return'] == {}
+
+log("{} persistent bitmap already created, " \
+"let's try to create one more".format(QCOW2_MAX_BITMAPS))
+
+vm.qmp_log('block-dirty-bitmap-add', node='drive0',
+   name='bitmap{}'.format(QCOW2_MAX_BITMAPS), persistent=True)
+
+vm.shutdown()
diff --git a/tests/qemu-iotests/269.out b/tests/qemu-iotests/269.out
new file mode 100644
index 00..bcfa616a2b
--- /dev/null
+++ b/tests/qemu-iotests/269.out
@@ -0,0 +1,3 @@
+65535 persistent bitmap already created, let's try to create one more
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap65535", 
"node": "drive0", "persistent": true}}
+{"error": {"class": "GenericError", "desc": "Can't make bitmap 'bitmap65535' 
persistent in 'drive0': Maximum number of persistent bitmaps is already 
reached"}}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 0c1e5ef414..fe8274a204 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -279,3 +279,4 @@
 265 rw auto quick
 266 rw quick
 267 rw auto quick snapshot
+269
-- 
2.21.0

Re: [PATCH v2] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK

[Alberto Garcia]

On Mon 14 Oct 2019 11:26:01 AM CEST, Kevin Wolf wrote:
>> Signed-off-by: Alberto Garcia 
>
> Thanks, applied to the block branch.

I'm a bit late now, but a possible trivial optimization is to flip the
conditions, because checking the flag should be faster than checking the
alignment and it's going to be false in almost all cases:

if ((flags & BDRV_REQ_NO_FALLBACK) &&
!QEMU_IS_ALIGNED(offset | bytes, align)) {
return -ENOTSUP;
}

Feel free to edit the commit if you want.

Berto

[PATCH v2 1/2] qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

qcow2_can_store_new_dirty_bitmap works wrong, as it considers only
bitmaps already stored in the qcow2 image and ignores persistent
BdrvDirtyBitmap objects.

So, let's instead count persistent BdrvDirtyBitmaps. We load all qcow2
bitmaps on open, so there should not be any bitmap in the image for
which we don't have BdrvDirtyBitmaps version. If it is - it's a kind of
corruption, and no reason to check for corruptions here (open() and
close() are better places for it).

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/qcow2-bitmap.c | 41 ++---
 1 file changed, 18 insertions(+), 23 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 98294a7696..d5131181a3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1671,8 +1671,14 @@ bool coroutine_fn 
qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
   Error **errp)
 {
 BDRVQcow2State *s = bs->opaque;
-bool found;
-Qcow2BitmapList *bm_list;
+BdrvDirtyBitmap *bitmap;
+uint64_t bitmap_directory_size = 0;
+uint32_t nb_bitmaps = 0;
+
+if (bdrv_find_dirty_bitmap(bs, name)) {
+error_setg(errp, "Bitmap already exists: %s", name);
+return false;
+}
 
 if (s->qcow_version < 3) {
 /* Without autoclear_features, we would always have to assume
@@ -1688,38 +1694,27 @@ bool coroutine_fn 
qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
 goto fail;
 }
 
-if (s->nb_bitmaps == 0) {
-return true;
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
+if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
+nb_bitmaps++;
+bitmap_directory_size +=
+calc_dir_entry_size(strlen(bdrv_dirty_bitmap_name(bitmap)), 0);
+}
 }
+nb_bitmaps++;
+bitmap_directory_size += calc_dir_entry_size(strlen(name), 0);
 
-if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
+if (nb_bitmaps > QCOW2_MAX_BITMAPS) {
 error_setg(errp,
"Maximum number of persistent bitmaps is already reached");
 goto fail;
 }
 
-if (s->bitmap_directory_size + calc_dir_entry_size(strlen(name), 0) >
-QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
-{
+if (bitmap_directory_size > QCOW2_MAX_BITMAP_DIRECTORY_SIZE) {
 error_setg(errp, "Not enough space in the bitmap directory");
 goto fail;
 }
 
-qemu_co_mutex_lock(>lock);
-bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-   s->bitmap_directory_size, errp);
-qemu_co_mutex_unlock(>lock);
-if (bm_list == NULL) {
-goto fail;
-}
-
-found = find_bitmap_by_name(bm_list, name);
-bitmap_list_free(bm_list);
-if (found) {
-error_setg(errp, "Bitmap with the same name is already stored");
-goto fail;
-}
-
 return true;
 
 fail:
-- 
2.21.0

Re: [PATCH v2 0/2] fix qcow2_can_store_new_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

14.10.2019 14:51, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is a fix for persistent bitmaps managing: we must check existent
> but not yet stored bitmaps for qcow2-related constraints, like maximum
> number of bitmaps in qcow2 image.
> 
> v2:

main thing based on https://github.com/jnsnow/qemu bitmaps
Based-on: <20191011212550.27269-1-js...@redhat.com>

> 
> 01: change assertion to error-return at function start
>  Be free to add
>  Reported-by: aihua liang 
>  Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1712636
>  if it's appropriate
> 02: new test
>  Ohh, it takes about 4 minutes. Be free to drop it, as I doubt that
>  it worth to have. The case is simple, we may live without a
>  test.
> 
> Vladimir Sementsov-Ogievskiy (2):
>qcow2-bitmaps: fix qcow2_can_store_new_dirty_bitmap
>iotests: add 269 to check maximum of bitmaps in qcow2
> 
>   block/qcow2-bitmap.c   | 41 +++--
>   tests/qemu-iotests/269 | 47 ++
>   tests/qemu-iotests/269.out |  3 +++
>   tests/qemu-iotests/group   |  1 +
>   4 files changed, 69 insertions(+), 23 deletions(-)
>   create mode 100755 tests/qemu-iotests/269
>   create mode 100644 tests/qemu-iotests/269.out
> 


-- 
Best regards,
Vladimir

Re: [PATCH] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK

[Alberto Garcia]

On Mon 14 Oct 2019 12:11:52 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 13.10.2019 23:48, Alberto Garcia wrote:
>> The BDRV_REQ_NO_FALLBACK flag means that an operation should only be
>> performed if it can be offloaded or otherwise performed efficiently.
>
> As I know, BDRV_REQ_NO_FALLBACK is for write-zeros only, not about
> offloading..

I just used the same wording from the documentation in block.h:

/* Execute the request only if the operation can be offloaded or otherwise
 * be executed efficiently, but return an error instead of using a slow
 * fallback. */

Berto

Re: [PULL 1/1] test-bdrv-drain: fix iothread_join() hang

[Paolo Bonzini]

On 14/10/19 10:52, Stefan Hajnoczi wrote:
> tests/test-bdrv-drain can hang in tests/iothread.c:iothread_run():
> 
>   while (!atomic_read(>stopping)) {
>   aio_poll(iothread->ctx, true);
>   }
> 
> The iothread_join() function works as follows:
> 
>   void iothread_join(IOThread *iothread)
>   {
>   iothread->stopping = true;
>   aio_notify(iothread->ctx);
>   qemu_thread_join(>thread);
> 
> If iothread_run() checks iothread->stopping before the iothread_join()
> thread sets stopping to true, then aio_notify() may be optimized away
> and iothread_run() hangs forever in aio_poll().
> 
> The correct way to change iothread->stopping is from a BH that executes
> within iothread_run().  This ensures that iothread->stopping is checked
> after we set it to true.
> 
> This was already fixed for ./iothread.c (note this is a different source
> file!) by commit 2362a28ea11c145e1a13ae79342d76dc118a72a6 ("iothread:
> fix iothread_stop() race condition"), but not for tests/iothread.c.

Aha, I did have some kind of dejavu when sending the patch I have just
sent; let's see if this also fixes the test-aio-multithread assertion
failure.

Note that with this change the atomic read of iothread->stopping can go
away; I can send a separate patch later.

Paolo

> Fixes: 0c330a734b51c177ab8488932ac3b0c4d63a718a
>("aio: introduce aio_co_schedule and aio_co_wake")
> Reported-by: Dr. David Alan Gilbert 
> Reviewed-by: Paolo Bonzini 
> Message-Id: <20191003100103.331-1-stefa...@redhat.com>
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tests/iothread.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/iothread.c b/tests/iothread.c
> index 777d9eea46..13c9fdcd8d 100644
> --- a/tests/iothread.c
> +++ b/tests/iothread.c
> @@ -55,10 +55,16 @@ static void *iothread_run(void *opaque)
>  return NULL;
>  }
>  
> -void iothread_join(IOThread *iothread)
> +static void iothread_stop_bh(void *opaque)
>  {
> +IOThread *iothread = opaque;
> +
>  iothread->stopping = true;
> -aio_notify(iothread->ctx);
> +}
> +
> +void iothread_join(IOThread *iothread)
> +{
> +aio_bh_schedule_oneshot(iothread->ctx, iothread_stop_bh, iothread);
>  qemu_thread_join(>thread);
>  qemu_cond_destroy(>init_done_cond);
>  qemu_mutex_destroy(>init_done_lock);
> 

Re: [PATCH 4/5] iotests: Skip "make check-block" if QEMU does not support virtio-blk

[Thomas Huth]

On 14/10/2019 13.21, Kevin Wolf wrote:
> Am 11.10.2019 um 16:50 hat Thomas Huth geschrieben:
>> The next patch is going to add some python-based tests to the "auto"
>> group, and these tests require virtio-blk to work properly. Running
>> iotests without virtio-blk likely does not make too much sense anyway,
>> so instead of adding a check for the availability of virtio-blk to each
>> and every test (which does not sound very appealing), let's rather add
>> a check for this at the top level in the check-block.sh script instead
>> (so that it is possible to run "make check" without the "check-block"
>> part for qemu-system-tricore for example).
>>
>> Signed-off-by: Thomas Huth 
>> ---
>>  tests/check-block.sh | 16 +++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/check-block.sh b/tests/check-block.sh
>> index 679aedec50..7582347ec2 100755
>> --- a/tests/check-block.sh
>> +++ b/tests/check-block.sh
>> @@ -26,10 +26,24 @@ if grep -q "CFLAGS.*-fsanitize" config-host.mak 
>> 2>/dev/null ; then
>>  exit 0
>>  fi
>>  
>> -if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then
>> +if [ -n "$QEMU_PROG" ]; then
>> +qemu_prog="$QEMU_PROG"
>> +else
>> +for binary in *-softmmu/qemu-system-* ; do
>> +if [ -x "$binary" ]; then
>> +qemu_prog="$binary"
>> +break
>> +fi
> 
> Wouldn't it be better to check the availability of virtio-blk here, so
> that if the current binary doesn't support it, we keep searching and
> maybe pick up a different binary that supports it?

That's a good idea, indeed, but then I also need to adjust the code in
the "check" script accordingly.

> Or actually, should we work with a whitelist?

I don't think that a hard-coded list will work well: Since we introduced
the Kconfig build system, it's now possible for example to also build an
qemu-system-aarch64 binary that does not contain any of the boards that
support virtio. So while virtio-blk is available by default in
qemu-system-aarch64, some builds might not contain it.

 Thomas

[PATCH] tests: fix iothread_join() race condition

[Paolo Bonzini]

This is a similar race condition to the one fixed in commit 2362a28ea1
("iothread: fix iothread_stop() race condition", 2017-12-19), which in this case
was exposed as a failure in test-aio-multithread.  Here, the iothread->stopping
flag was set too soon, and the AioContext could be destroyed before the
last round of entering scheduled coroutines and bottom halves.  The fix is
similar.

Reported-by: Peter Maydell 
Signed-off-by: Paolo Bonzini 
---
 tests/iothread.c | 22 --
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/tests/iothread.c b/tests/iothread.c
index 777d9eea46..1e6d3e2218 100644
--- a/tests/iothread.c
+++ b/tests/iothread.c
@@ -26,6 +26,7 @@ struct IOThread {
 QemuMutex init_done_lock;
 QemuCond init_done_cond;/* is thread initialization done? */
 bool stopping;
+bool running;
 };
 
 static __thread IOThread *my_iothread;
@@ -47,7 +48,7 @@ static void *iothread_run(void *opaque)
 qemu_cond_signal(>init_done_cond);
 qemu_mutex_unlock(>init_done_lock);
 
-while (!atomic_read(>stopping)) {
+while (iothread->running) {
 aio_poll(iothread->ctx, true);
 }
 
@@ -55,10 +56,26 @@ static void *iothread_run(void *opaque)
 return NULL;
 }
 
+/* Runs in iothread_run() thread */
+static void iothread_stop_bh(void *opaque)
+{
+IOThread *iothread = opaque;
+
+iothread->running = false; /* stop iothread_run() */
+}
+
 void iothread_join(IOThread *iothread)
 {
+/* Forbid reentering iothread_join.  */
+assert(!iothread->stopping);
 iothread->stopping = true;
-aio_notify(iothread->ctx);
+
+/*
+ * Force the loop to run once more, so that already scheduled bottom
+ * halves and coroutines are executed.
+ */
+aio_bh_schedule_oneshot(iothread->ctx, iothread_stop_bh, iothread);
+
 qemu_thread_join(>thread);
 qemu_cond_destroy(>init_done_cond);
 qemu_mutex_destroy(>init_done_lock);
@@ -76,6 +93,7 @@ IOThread *iothread_new(void)
iothread, QEMU_THREAD_JOINABLE);
 
 /* Wait for initialization to complete */
+iothread->running = true;
 qemu_mutex_lock(>init_done_lock);
 while (iothread->ctx == NULL) {
 qemu_cond_wait(>init_done_cond,
-- 
2.21.0

Re: [PATCH 4/5] iotests: Skip "make check-block" if QEMU does not support virtio-blk

[Kevin Wolf]

Am 11.10.2019 um 16:50 hat Thomas Huth geschrieben:
> The next patch is going to add some python-based tests to the "auto"
> group, and these tests require virtio-blk to work properly. Running
> iotests without virtio-blk likely does not make too much sense anyway,
> so instead of adding a check for the availability of virtio-blk to each
> and every test (which does not sound very appealing), let's rather add
> a check for this at the top level in the check-block.sh script instead
> (so that it is possible to run "make check" without the "check-block"
> part for qemu-system-tricore for example).
> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/check-block.sh | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/check-block.sh b/tests/check-block.sh
> index 679aedec50..7582347ec2 100755
> --- a/tests/check-block.sh
> +++ b/tests/check-block.sh
> @@ -26,10 +26,24 @@ if grep -q "CFLAGS.*-fsanitize" config-host.mak 
> 2>/dev/null ; then
>  exit 0
>  fi
>  
> -if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then
> +if [ -n "$QEMU_PROG" ]; then
> +qemu_prog="$QEMU_PROG"
> +else
> +for binary in *-softmmu/qemu-system-* ; do
> +if [ -x "$binary" ]; then
> +qemu_prog="$binary"
> +break
> +fi

Wouldn't it be better to check the availability of virtio-blk here, so
that if the current binary doesn't support it, we keep searching and
maybe pick up a different binary that supports it?

Or actually, should we work with a whitelist? We already need separate
code for s390 and x86_64 in some places to choose between -pci and -ccw,
and the presence of some virtio-blk doesn't mean that we know the
specifics of how to add a virtio-blk device for this target. This
suggests that blindly using a random binary might not be possible, but
tests may have to be adapted before the target can be whitelisted.

Kevin

> +done
> +fi
> +if [ -z "$qemu_prog" ]; then
>  echo "No qemu-system binary available ==> Not running the qemu-iotests."
>  exit 0
>  fi
> +if ! "$qemu_prog" -M none -device help | grep virtio-blk >/dev/null 2>&1 ; 
> then
> +echo "$qemu_prog does not support virtio-blk ==> Not running the 
> qemu-iotests."
> +exit 0
> +fi
>  
>  if ! command -v bash >/dev/null 2>&1 ; then
>  echo "bash not available ==> Not running the qemu-iotests."
> -- 
> 2.18.1
> 

Re: [PATCH] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK

[Vladimir Sementsov-Ogievskiy]

14.10.2019 13:11, Vladimir Sementsov-Ogievskiy wrote:
> 13.10.2019 23:48, Alberto Garcia wrote:
>> The BDRV_REQ_NO_FALLBACK flag means that an operation should only be
>> performed if it can be offloaded or otherwise performed efficiently.
> 
> As I know, BDRV_REQ_NO_FALLBACK is for write-zeros only, not about 
> offloading..
> 
>>
>> However a misaligned write request requires a RMW so we should return
>> an error and let the caller decide how to proceed.
> 
> Because we can finish up with trying to to normal write (not write_zero) with
> BDRV_REQ_NO_FALLBACK flag, which is forbidden for bdrv_driver_pwritev, as it's
> shown in assertion below.
> 

Haha, I'm too late, see it's already queued, sorry.



-- 
Best regards,
Vladimir

Re: [PATCH] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK

[Vladimir Sementsov-Ogievskiy]

13.10.2019 23:48, Alberto Garcia wrote:
> The BDRV_REQ_NO_FALLBACK flag means that an operation should only be
> performed if it can be offloaded or otherwise performed efficiently.

As I know, BDRV_REQ_NO_FALLBACK is for write-zeros only, not about offloading..

> 
> However a misaligned write request requires a RMW so we should return
> an error and let the caller decide how to proceed.

Because we can finish up with trying to to normal write (not write_zero) with
BDRV_REQ_NO_FALLBACK flag, which is forbidden for bdrv_driver_pwritev, as it's
shown in assertion below.

> 
> This hits an assertion since commit c8bb23cbdb if the required
> alignment is larger than the cluster size:
> 
> qemu-img create -f qcow2 -o cluster_size=2k img.qcow2 4G
> qemu-io -c "open -o driver=qcow2,file.align=4k blkdebug::img.qcow2" \
>  -c 'write 0 512'
> qemu-io: block/io.c:1127: bdrv_driver_pwritev: Assertion `!(flags & 
> BDRV_REQ_NO_FALLBACK)' failed.
> Aborted
> 
> Signed-off-by: Alberto Garcia 
> ---
>   block/io.c |  6 +
>   tests/qemu-iotests/268 | 55 ++
>   tests/qemu-iotests/268.out |  7 +
>   tests/qemu-iotests/group   |  1 +
>   4 files changed, 69 insertions(+)
>   create mode 100755 tests/qemu-iotests/268
>   create mode 100644 tests/qemu-iotests/268.out
> 
> diff --git a/block/io.c b/block/io.c
> index 4f9ee97c2b..c5d4d029da 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2071,6 +2071,12 @@ int coroutine_fn bdrv_co_pwritev_part(BdrvChild *child,
>   return ret;
>   }
>   
> +/* If the request is misaligned then we can't make it efficient */
> +if (((offset & (align - 1)) || (bytes & (align - 1))) &&
> +(flags & BDRV_REQ_NO_FALLBACK)) {
> +return -ENOTSUP;
> +}
> +

So, if BDRV_REQ_NO_FALLBACK is only for write zeros, most correct place for 
this check
is bdrv_co_do_zero_pwritev().. But it's OK as is too.

Not long ago such checks was fixed to be QEMU_IS_ALIGNED, so with it used 
instead:
Reviewed-by: Vladimir Sementsov-Ogievskiy 

>   bdrv_inc_in_flight(bs);
>   /*
>* Align write if necessary by performing a read-modify-write cycle.
> diff --git a/tests/qemu-iotests/268 b/tests/qemu-iotests/268
> new file mode 100755
> index 00..895f6e593f
> --- /dev/null
> +++ b/tests/qemu-iotests/268
> @@ -0,0 +1,55 @@
> +#!/usr/bin/env bash
> +#
> +# Test write request with required alignment larger than the cluster size
> +#
> +# Copyright (C) 2019 Igalia, S.L.
> +# Author: Alberto Garcia 
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see .
> +#
> +
> +# creator
> +owner=be...@igalia.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +status=1 # failure is the default!
> +
> +_cleanup()
> +{
> +_cleanup_test_img
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +
> +echo
> +echo "== Required alignment larger than cluster size =="
> +
> +CLUSTER_SIZE=2k _make_test_img 1M
> +# Since commit c8bb23cbdb writing to an allocated cluster fills the
> +# empty COW areas with bdrv_write_zeroes(flags=BDRV_REQ_NO_FALLBACK)
> +$QEMU_IO -c "open -o driver=$IMGFMT,file.align=4k blkdebug::$TEST_IMG" \
> + -c "write 0 512" | _filter_qemu_io
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/268.out b/tests/qemu-iotests/268.out
> new file mode 100644
> index 00..2ed6c68529
> --- /dev/null
> +++ b/tests/qemu-iotests/268.out
> @@ -0,0 +1,7 @@
> +QA output created by 268
> +
> +== Required alignment larger than cluster size ==
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
> +wrote 512/512 bytes at offset 0
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 5805a79d9e..4c861f7eed 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -278,3 +278,4 @@
>   265 rw auto quick
>   266 rw quick
>   267 rw auto quick snapshot
> +268 rw auto quick
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2] iotests/028: Fix for long $TEST_DIRs

[Kevin Wolf]

Am 11.10.2019 um 14:18 hat Max Reitz geschrieben:
> For long test image paths, the order of the "Formatting" line and the
> "(qemu)" prompt after a drive_backup HMP command may be reversed.  In
> fact, the interaction between the prompt and the line may lead to the
> "Formatting" to being greppable at all after "read"-ing it (if the
> prompt injects an IFS character into the "Formatting" string).
> 
> So just wait until we get a prompt.  At that point, the block job must
> have been started, so "info block-jobs" will only return "No active
> jobs" once it is done.
> 
> Reported-by: Thomas Huth 
> Signed-off-by: Max Reitz 

Thanks, applied to the block branch.

Kevin

Re: [PATCH 1/4] vl: Split off user_creatable_print_help()

[Kevin Wolf]

Am 11.10.2019 um 23:35 hat Eric Blake geschrieben:
> On 10/11/19 3:55 PM, Kevin Wolf wrote:
> > Printing help for --object is something that we don't only want in the
> 
> s/don't/not/

Can someone send a fix for the English grammar? It's obviously broken
and doesn't know what it wants. Actually, maybe do-support was a bad
idea and we should just revert it and restore consistent use of proper
verb-second word order?

(Hm, actually, since this seems to negate "only" rather than the verb,
does "...that we want not only in..." work without patching the
grammar?)

(Thanks for the correction.)

Kevin

> > system emulator, but also in tools that support --object. Move it into a
> > separate function in qom/object_interfaces.c to make the code accessible
> > for tools.
> > 
> > Signed-off-by: Kevin Wolf 

Re: [PATCH 4/4] qemu-nbd: Support help options for --object

[Kevin Wolf]

Am 11.10.2019 um 23:39 hat Eric Blake geschrieben:
> On 10/11/19 3:55 PM, Kevin Wolf wrote:
> > Instead of parsing help options as normal object properties and
> > returning an error, provide the same help functionality as the system
> > emulator in qemu-nbd, too.
> > 
> > Signed-off-by: Kevin Wolf 
> 
> Missing a change in qemu-nbd.texi for man page coverage.

Hm... Both qemu-img and qemu-nbd manpages refer to qemu(1) for the
details. I wouldn't mind copying the text for '-object help' from there
anyway, but unfortunately it doesn't even exist. :-)

So this looks like a separate patch that fixes it for qemu, too.

> But the patch is a strict improvement, so even if we have to add a
> followup patch for documentation, I'm okay with:
> 
> Reviewed-by: Eric Blake 
> 
> This patch touches NBD, but I'm assuming it's easier to take the series
> through your tree.

Yes, thanks.

Kevin

Re: [PATCH v2] block: Reject misaligned write requests with BDRV_REQ_NO_FALLBACK

[Kevin Wolf]

Am 14.10.2019 um 10:15 hat Alberto Garcia geschrieben:
> The BDRV_REQ_NO_FALLBACK flag means that an operation should only be
> performed if it can be offloaded or otherwise performed efficiently.
> 
> However a misaligned write request requires a RMW so we should return
> an error and let the caller decide how to proceed.
> 
> This hits an assertion since commit c8bb23cbdb if the required
> alignment is larger than the cluster size:
> 
> qemu-img create -f qcow2 -o cluster_size=2k img.qcow2 4G
> qemu-io -c "open -o driver=qcow2,file.align=4k blkdebug::img.qcow2" \
> -c 'write 0 512'
> qemu-io: block/io.c:1127: bdrv_driver_pwritev: Assertion `!(flags & 
> BDRV_REQ_NO_FALLBACK)' failed.
> Aborted
> 
> The reason is that when writing to an unallocated cluster we try to
> skip the copy-on-write part and zeroize it using BDRV_REQ_NO_FALLBACK
> instead, resulting in a write request that is too small (2KB cluster
> size vs 4KB required alignment).
> 
> Signed-off-by: Alberto Garcia 

Thanks, applied to the block branch.

Kevin

Re: [RFC v5 000/126] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

12.10.2019 5:10, no-re...@patchew.org wrote:
> Patchew URL: 
> https://patchew.org/QEMU/20191011160552.22907-1-vsement...@virtuozzo.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-quick@centos7 build test. Please find the 
> testing commands and
> their output below. If you have Docker installed, you can probably reproduce 
> it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
>TESTiotest-qcow2: 013
>TESTcheck-unit: tests/test-block-iothread
>TESTcheck-unit: tests/test-image-locking
> warning: Failed to get shared "consistent read" lock
> Is another process using the image [/tmp/qtest.O66l3t]?
> warning: Failed to get shared "consistent read" lock
> Is another process using the image [/tmp/qtest.G0M9V8]?
>TESTcheck-unit: tests/test-x86-cpuid
>TESTcheck-unit: tests/test-xbzrle
> ---
>TESTiotest-qcow2: 267
> Failures: 054
> Failed 1 of 108 iotests
> make: *** [check-tests/check-block.sh] Error 1
> make: *** Waiting for unfinished jobs
>TESTcheck-qtest-aarch64: tests/qos-test
> Traceback (most recent call last):
> ---
>  raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
> '--label', 'com.qemu.instance.uuid=2c55fb61a63a409382f2b3a99fca93d9', '-u', 
> '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', 
> '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', 
> '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', 
> '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
> '/var/tmp/patchew-tester-tmp-usvi8fob/src/docker-src.2019-10-11-22.00.10.1933:/var/tmp/qemu:z,ro',
>  'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
> status 2.
> filter=--filter=label=com.qemu.instance.uuid=2c55fb61a63a409382f2b3a99fca93d9
> make[1]: *** [docker-run] Error 1
> make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-usvi8fob/src'
> make: *** [docker-run-test-quick@centos7] Error 2

Something strange. Who knows, what is it?

> 
> real10m33.714s
> user0m8.360s
> 
> 
> The full log is available at
> http://patchew.org/logs/20191011160552.22907-1-vsement...@virtuozzo.com/testing.docker-quick@centos7/?type=message.

""
Not Found

The requested resource was not found on this server.
""

> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-de...@redhat.com
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size

[Dr. David Alan Gilbert]

* Wei Yang (richardw.y...@linux.intel.com) wrote:
> There are three page size in qemu:
> 
>   real host page size
>   host page size
>   target page size
> 
> All of them have dedicate variable to represent. For the last two, we
> use the same form in the whole qemu project, while for the first one we
> use two forms: qemu_real_host_page_size and getpagesize().
> 
> qemu_real_host_page_size is defined to be a replacement of
> getpagesize(), so let it serve the role.

We also use sysconf(_SC_PAGESIZE) in a few places.

Dave

> 
> [Note] Not fully tested for some arch or device.
> 
> Signed-off-by: Wei Yang 
> ---
>  accel/kvm/kvm-all.c|  6 +++---
>  backends/hostmem.c |  2 +-
>  block.c|  4 ++--
>  block/file-posix.c |  9 +
>  block/io.c |  2 +-
>  block/parallels.c  |  2 +-
>  block/qcow2-cache.c|  2 +-
>  contrib/vhost-user-gpu/vugbm.c |  2 +-
>  exec.c |  6 +++---
>  hw/intc/s390_flic_kvm.c|  2 +-
>  hw/ppc/mac_newworld.c  |  2 +-
>  hw/ppc/spapr_pci.c |  2 +-
>  hw/rdma/vmw/pvrdma_main.c  |  2 +-
>  hw/vfio/spapr.c|  7 ---
>  include/exec/ram_addr.h|  2 +-
>  include/qemu/osdep.h   |  4 ++--
>  migration/migration.c  |  2 +-
>  migration/postcopy-ram.c   |  4 ++--
>  monitor/misc.c |  2 +-
>  target/ppc/kvm.c   |  2 +-
>  tests/vhost-user-bridge.c  |  8 
>  util/mmap-alloc.c  | 10 +-
>  util/oslib-posix.c |  4 ++--
>  util/oslib-win32.c |  2 +-
>  util/vfio-helpers.c| 12 ++--
>  25 files changed, 52 insertions(+), 50 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index d2d96d73e8..140b0bd8f6 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -52,7 +52,7 @@
>  /* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We
>   * need to use the real host PAGE_SIZE, as that's what KVM will use.
>   */
> -#define PAGE_SIZE getpagesize()
> +#define PAGE_SIZE qemu_real_host_page_size
>  
>  //#define DEBUG_KVM
>  
> @@ -507,7 +507,7 @@ static int 
> kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>  {
>  ram_addr_t start = section->offset_within_region +
> memory_region_get_ram_addr(section->mr);
> -ram_addr_t pages = int128_get64(section->size) / getpagesize();
> +ram_addr_t pages = int128_get64(section->size) / 
> qemu_real_host_page_size;
>  
>  cpu_physical_memory_set_dirty_lebitmap(bitmap, start, pages);
>  return 0;
> @@ -1841,7 +1841,7 @@ static int kvm_init(MachineState *ms)
>   * even with KVM.  TARGET_PAGE_SIZE is assumed to be the minimum
>   * page size for the system though.
>   */
> -assert(TARGET_PAGE_SIZE <= getpagesize());
> +assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size);
>  
>  s->sigmask_len = 8;
>  
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 6d333dc23c..e773bdfa6e 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -304,7 +304,7 @@ size_t host_memory_backend_pagesize(HostMemoryBackend 
> *memdev)
>  #else
>  size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
>  {
> -return getpagesize();
> +return qemu_real_host_page_size;
>  }
>  #endif
>  
> diff --git a/block.c b/block.c
> index 5944124845..98f47e2902 100644
> --- a/block.c
> +++ b/block.c
> @@ -106,7 +106,7 @@ size_t bdrv_opt_mem_align(BlockDriverState *bs)
>  {
>  if (!bs || !bs->drv) {
>  /* page size or 4k (hdd sector size) should be on the safe side */
> -return MAX(4096, getpagesize());
> +return MAX(4096, qemu_real_host_page_size);
>  }
>  
>  return bs->bl.opt_mem_alignment;
> @@ -116,7 +116,7 @@ size_t bdrv_min_mem_align(BlockDriverState *bs)
>  {
>  if (!bs || !bs->drv) {
>  /* page size or 4k (hdd sector size) should be on the safe side */
> -return MAX(4096, getpagesize());
> +return MAX(4096, qemu_real_host_page_size);
>  }
>  
>  return bs->bl.min_mem_alignment;
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f12c06de2d..f60ac3f93f 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -322,7 +322,7 @@ static void raw_probe_alignment(BlockDriverState *bs, int 
> fd, Error **errp)
>  {
>  BDRVRawState *s = bs->opaque;
>  char *buf;
> -size_t max_align = MAX(MAX_BLOCKSIZE, getpagesize());
> +size_t max_align = MAX(MAX_BLOCKSIZE, qemu_real_host_page_size);
>  size_t alignments[] = {1, 512, 1024, 2048, 4096};
>  
>  /* For SCSI generic devices the alignment is not really used.
> @@ -1131,13 +1131,14 @@ static void raw_refresh_limits(BlockDriverState *bs, 
> Error **errp)
>  
>  ret = sg_get_max_segments(s->fd);
>  if (ret > 0) {
> -bs->bl.max_transfer = 

Re: [RFC v5 000/126] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

12.10.2019 5:52, no-re...@patchew.org wrote:
> Patchew URL: 
> https://patchew.org/QEMU/20191011160552.22907-1-vsement...@virtuozzo.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Subject: [RFC v5 000/126] error: auto propagated local_err
> Type: series
> Message-id: 20191011160552.22907-1-vsement...@virtuozzo.com
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
> 
> Switched to a new branch 'test'
> 319b206 util/qemu-config.c: introduce ERRP_AUTO_PROPAGATE
> 3ee6567 tests/test-image-locking.c: introduce ERRP_AUTO_PROPAGATE
> 2e4f371 target/tilegx/cpu.c: introduce ERRP_AUTO_PROPAGATE
> 5f766d7 memory_mapping.c: introduce ERRP_AUTO_PROPAGATE
> 8969c6f iothread.c: introduce ERRP_AUTO_PROPAGATE
> 1534a92 hw/sd/ssi-sd.c: introduce ERRP_AUTO_PROPAGATE
> b5b1e0a hw/cpu/core.c: introduce ERRP_AUTO_PROPAGATE
> dd3f310 hw/core/bus.c: introduce ERRP_AUTO_PROPAGATE
> 21ee5f8 PVRDMA: introduce ERRP_AUTO_PROPAGATE
> 84365ab Replication: introduce ERRP_AUTO_PROPAGATE
> 225bbc5 vvfat: introduce ERRP_AUTO_PROPAGATE
> 2da58d6 vpc: introduce ERRP_AUTO_PROPAGATE
> e7fb0b9 blkdebug: introduce ERRP_AUTO_PROPAGATE
> 39d367e qcow: introduce ERRP_AUTO_PROPAGATE
> 1b7546c qcow2: introduce ERRP_AUTO_PROPAGATE
> c0352a1 raw: introduce ERRP_AUTO_PROPAGATE
> 612aebf qed: introduce ERRP_AUTO_PROPAGATE
> 155f6ea parallels: introduce ERRP_AUTO_PROPAGATE
> 5d3c3fd blkverify: introduce ERRP_AUTO_PROPAGATE
> 36132e6 blklogwrites: introduce ERRP_AUTO_PROPAGATE
> 785cfb4 Quorum: introduce ERRP_AUTO_PROPAGATE
> 5acba63 Bootdevice: introduce ERRP_AUTO_PROPAGATE
> 0e0d3fb NVMe Block Driver: introduce ERRP_AUTO_PROPAGATE
> 1cf6911 GLUSTER: introduce ERRP_AUTO_PROPAGATE
> cb4c8f7 CURL: introduce ERRP_AUTO_PROPAGATE
> 4e32493 SSH: introduce ERRP_AUTO_PROPAGATE
> abbecd4 NFS: introduce ERRP_AUTO_PROPAGATE
> f4f937d nbd: introduce ERRP_AUTO_PROPAGATE
> 7b638b3 iSCSI: introduce ERRP_AUTO_PROPAGATE
> 4727ffc VDI: introduce ERRP_AUTO_PROPAGATE
> 2c03145 VHDX: introduce ERRP_AUTO_PROPAGATE
> 7916a87 Sheepdog: introduce ERRP_AUTO_PROPAGATE
> db15fdd RBD: introduce ERRP_AUTO_PROPAGATE
> 05ea2cf VMDK: introduce ERRP_AUTO_PROPAGATE
> ff11144 Record/replay: introduce ERRP_AUTO_PROPAGATE
> a95fab2 colo: introduce ERRP_AUTO_PROPAGATE
> be71202 Sockets: introduce ERRP_AUTO_PROPAGATE
> 69a59b0 I/O Channels: introduce ERRP_AUTO_PROPAGATE
> e4f56f3 Cryptography: introduce ERRP_AUTO_PROPAGATE
> 4f5f412 Migration: introduce ERRP_AUTO_PROPAGATE
> 985da1a TPM: introduce ERRP_AUTO_PROPAGATE
> b19cdab Tracing: introduce ERRP_AUTO_PROPAGATE
> 3113fc7 SLIRP: introduce ERRP_AUTO_PROPAGATE
> 51e2f48 QMP: introduce ERRP_AUTO_PROPAGATE
> 1c0c827 QOM: introduce ERRP_AUTO_PROPAGATE
> fc0eec4 qga: introduce ERRP_AUTO_PROPAGATE
> af16041 QAPI: introduce ERRP_AUTO_PROPAGATE
> 21ed21e cryptodev: introduce ERRP_AUTO_PROPAGATE
> 7ab6e12 hostmem: introduce ERRP_AUTO_PROPAGATE
> 994c02c net: introduce ERRP_AUTO_PROPAGATE
> 26fe9a4 Human Monitor (HMP): introduce ERRP_AUTO_PROPAGATE
> 82b7f8b Main loop: introduce ERRP_AUTO_PROPAGATE
> 863100d Graphics: introduce ERRP_AUTO_PROPAGATE
> 45a8d41 SPICE: introduce ERRP_AUTO_PROPAGATE
> 6d967ec Memory API: introduce ERRP_AUTO_PROPAGATE
> 5645325 Dump: introduce ERRP_AUTO_PROPAGATE
> 6d795b4 cmdline: introduce ERRP_AUTO_PROPAGATE
> 5fceaa3 chardev: introduce ERRP_AUTO_PROPAGATE
> d551bda scsi: introduce ERRP_AUTO_PROPAGATE
> cc3d83e block: introduce ERRP_AUTO_PROPAGATE
> 75b948b Audio: introduce ERRP_AUTO_PROPAGATE
> c3fee2f XIVE: introduce ERRP_AUTO_PROPAGATE
> 42ba3e1 fw_cfg: introduce ERRP_AUTO_PROPAGATE
> 90c4efa virtio-gpu: introduce ERRP_AUTO_PROPAGATE
> 4db3f47 eepro100: introduce ERRP_AUTO_PROPAGATE
> d7634f4 NVDIMM: introduce ERRP_AUTO_PROPAGATE
> 706ee21 megasas: introduce ERRP_AUTO_PROPAGATE
> a037a5c virtio-rng: introduce ERRP_AUTO_PROPAGATE
> dcf1769 virtio-serial: introduce ERRP_AUTO_PROPAGATE
> 77d26d1 virtio-input: introduce ERRP_AUTO_PROPAGATE
> 7f62cb1 virtio-ccw: introduce ERRP_AUTO_PROPAGATE
> 2bdd860 virtio-blk: introduce ERRP_AUTO_PROPAGATE
> 026260e virtio-9p: introduce ERRP_AUTO_PROPAGATE
> 191c845 virtio: introduce ERRP_AUTO_PROPAGATE
> 24510de vhost: introduce ERRP_AUTO_PROPAGATE
> e8a1779 vfio-ccw: introduce ERRP_AUTO_PROPAGATE
> 00baaa3 VFIO: introduce ERRP_AUTO_PROPAGATE
> 361c201 USB (serial adapter): introduce ERRP_AUTO_PROPAGATE
> 0f70e97 USB: introduce ERRP_AUTO_PROPAGATE
> 9548378 SD (Secure Card): introduce ERRP_AUTO_PROPAGATE
> 90b472d SCSI: introduce ERRP_AUTO_PROPAGATE
> 312220a pflash: introduce ERRP_AUTO_PROPAGATE
> 47a7bb5 Network devices: introduce ERRP_AUTO_PROPAGATE
> bf2e1ef ACPI/SMBIOS: introduce ERRP_AUTO_PROPAGATE
> 98f6d04 PCI: introduce ERRP_AUTO_PROPAGATE
> e3e14fe IPack: introduce 

Re: [PULL 1/2] trace: add --group=all to tracing.txt

[Philippe Mathieu-Daudé]

Hi Stefan,

On 10/14/19 10:57 AM, Stefan Hajnoczi wrote:

tracetool needs to know the group name ("all", "root", or a specific
subdirectory).  Also remove the stdin redirection because tracetool.py
needs the path to the trace-events file.  Update the documentation.

Fixes: 2098c56a9bc5901e145fa5d4759f075808811685
("trace: move setting of group name into Makefiles")
Launchpad: https://bugs.launchpad.net/bugs/1844814


Sorry I didn't noticed that earlier, but on 
https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message 
we recommend using the 'Buglink' tag.

Not sure it's worth resending another pull request...


Reported-by: Philippe Mathieu-Daudé 
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Signed-off-by: Stefan Hajnoczi 
Message-Id: <20191009135154.10970-1-stefa...@redhat.com>
---
  docs/devel/tracing.txt | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/docs/devel/tracing.txt b/docs/devel/tracing.txt
index 8231bbf5d1..8c0376fefa 100644
--- a/docs/devel/tracing.txt
+++ b/docs/devel/tracing.txt
@@ -317,7 +317,8 @@ probes:
   --binary path/to/qemu-binary \
   --target-type system \
   --target-name x86_64 \
- qemu.stp
+ --group=all \
+ trace-events-all >qemu.stp
  
  To facilitate simple usage of systemtap where there merely needs to be printf

  logging of certain probes, a helper script "qemu-trace-stap" is provided.

[PULL 1/2] trace: add --group=all to tracing.txt

[Stefan Hajnoczi]

tracetool needs to know the group name ("all", "root", or a specific
subdirectory).  Also remove the stdin redirection because tracetool.py
needs the path to the trace-events file.  Update the documentation.

Fixes: 2098c56a9bc5901e145fa5d4759f075808811685
   ("trace: move setting of group name into Makefiles")
Launchpad: https://bugs.launchpad.net/bugs/1844814
Reported-by: Philippe Mathieu-Daudé 
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Philippe Mathieu-Daudé 
Tested-by: Philippe Mathieu-Daudé 
Signed-off-by: Stefan Hajnoczi 
Message-Id: <20191009135154.10970-1-stefa...@redhat.com>
---
 docs/devel/tracing.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/docs/devel/tracing.txt b/docs/devel/tracing.txt
index 8231bbf5d1..8c0376fefa 100644
--- a/docs/devel/tracing.txt
+++ b/docs/devel/tracing.txt
@@ -317,7 +317,8 @@ probes:
  --binary path/to/qemu-binary \
  --target-type system \
  --target-name x86_64 \
- qemu.stp
+ --group=all \
+ trace-events-all >qemu.stp
 
 To facilitate simple usage of systemtap where there merely needs to be printf
 logging of certain probes, a helper script "qemu-trace-stap" is provided.
-- 
2.21.0