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

[John Snow]

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.
> 

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

--js

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] Question about migration confirm phase

[Jim Fehlig]

I've been investigating a lockd lock ordering bug in a migration error handling 
path in the libxl driver. In the perform phase, the src calls 
virDomainLockProcessPause to release the lock before sending the VM to dst. In 
this case the send fails for other reasons and an attempt is made to reacquire 
the lock with virDomainLockProcessResume. But that fails since the dst has not 
finished cleaning up the failed VM and releasing the lock it acquired when 
starting to receive the VM. My immediate reaction was "why not reacquire the 
lock in the confirm phase", but then I saw my older comment a few lines later 
in 
the perform phase code

 /*
  * Confirm phase will not be executed if perform fails. End the
  * job started in begin phase.
  */

Is that just a bug in the implementation, or is it intended to skip the confirm 
phase if perform fails?

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Eric Blake]

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.


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 0/6] Enable ramfb parameter for mediated devices

[Jonathon Jongsma]

This patch series is a continuation of a previous series that was already
mostly merged. The one unmerged patch from the previous series is patch 6 in
this series and has been rebased to git master. The other 5 patches are an
attempt to address some things that Cole suggested in the previous review.  I'm
not sure whether I split the patches up too finely or not, but I'm happy to
combine a couple of these patches if requested.

Jonathon Jongsma (6):
  qemu: set domain capabilities for ramfb device
  qemu: use VIR_AUTOUNREF in qemuDomainDeviceDefValidate()
  qemu: fix domain device validation
  qemu: consolidate video validation
  qemu: unify domain caps validation for video devices
  qemu: add 'ramfb' attribute for mediated devices

 docs/formatdomain.html.in |   8 +
 docs/schemas/domaincommon.rng |   5 +
 src/conf/domain_capabilities.c|  17 ++-
 src/conf/domain_conf.c|  11 ++
 src/conf/domain_conf.h|   1 +
 src/qemu/qemu_capabilities.c  |   2 +
 src/qemu/qemu_command.c   |  17 ++-
 src/qemu/qemu_domain.c| 142 +-
 src/qemu/qemu_process.c   |  62 
 .../domaincapsschemadata/qemu_3.0.0.s390x.xml |   1 +
 .../qemu_3.1.0.x86_64.xml |   1 +
 .../qemu_4.0.0.x86_64.xml |   1 +
 .../qemu_4.1.0.x86_64.xml |   1 +
 .../qemu_4.2.0.x86_64.xml |   1 +
 ...tdev-mdev-display-ramfb.x86_64-latest.args |  37 +
 .../hostdev-mdev-display-ramfb.xml|  33 
 tests/qemuxml2argvtest.c  |   1 +
 17 files changed, 203 insertions(+), 138 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/hostdev-mdev-display-ramfb.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-ramfb.xml

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 3/6] qemu: fix domain device validation

[Jonathon Jongsma]

When the virDomainCapsDeviceDefValidate() function returned an error
status (-1), we were aborting the function early, but returning the
default return value (0). This patch properly returns an error in that
case.

Signed-off-by: Jonathon Jongsma 
---
 src/qemu/qemu_domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 516ae7e444..bc455e7da3 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7213,7 +7213,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
 if ((ret = qemuDomainDeviceDefValidateAddress(dev, qemuCaps)) < 0)
 return ret;
 
-if (virDomainCapsDeviceDefValidate(domCaps, dev, def) < 0)
+if ((ret = virDomainCapsDeviceDefValidate(domCaps, dev, def)) < 0)
 return ret;
 
 switch ((virDomainDeviceType)dev->type) {
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PULL 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 b3398a13c20..8d293f2b64e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -739,7 +739,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 ebc1afccd3d..f7dfb40256e 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);
+

[libvirt] [PULL 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 01245e0b1c4..7239e0959da 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 e6edd641f18..e4975ece4ab 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1987,10 +1987,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)
@@ -1999,7 +1995,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 9b6eca66430..873aba3c27f 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PULL 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 1e54486ad14..9ceca23ef75 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -546,12 +546,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 c548885608d..ba09d97e0a2 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 7ab1aad9779..b652bf884e5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1834,6 +1834,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);
 }
 
@@ -5262,7 +5276,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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 6/6] qemu: add 'ramfb' attribute for mediated devices

[Jonathon Jongsma]

The 'ramfb' attribute provides a framebuffer to the guest that can be
used as a boot display for the vgpu

For example, the following configuration can be used to provide a vgpu
with a boot display:







Signed-off-by: Jonathon Jongsma 
---
 docs/formatdomain.html.in |  8 
 docs/schemas/domaincommon.rng |  5 +++
 src/conf/domain_conf.c| 11 ++
 src/conf/domain_conf.h|  1 +
 src/qemu/qemu_command.c   | 17 -
 ...tdev-mdev-display-ramfb.x86_64-latest.args | 37 +++
 .../hostdev-mdev-display-ramfb.xml| 33 +
 tests/qemuxml2argvtest.c  |  1 +
 8 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 
tests/qemuxml2argvdata/hostdev-mdev-display-ramfb.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/hostdev-mdev-display-ramfb.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 7ec57135f6..81e8cafe87 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4846,6 +4846,14 @@
   graphical framebuffer in order to
   use this attribute, currently only supported with VNC, Spice and
   egl-headless graphics devices.
+
+  Since version 5.9.0, there is an optional
+  ramfb attribute for devices with
+  model='vfio-pci'. Supported values are either
+  on or off (default is 'off').  When
+  enabled, this attribute provides a memory framebuffer device to the
+  guest.  This framebuffer will be used as a boot display when a vgpu
+  device is the primary display.
   
 Note: There are also some implications on the usage of guest's
 address type depending on the model attribute,
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index ead5a25068..57e0dcf6ed 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4769,6 +4769,11 @@
 vfio-ap
   
 
+
+  
+
+  
+
 
   
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c1705a07b6..d6cf2132d7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8161,6 +8161,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
 VIR_AUTOFREE(char *) backendStr = NULL;
 VIR_AUTOFREE(char *) model = NULL;
 VIR_AUTOFREE(char *) display = NULL;
+VIR_AUTOFREE(char *) ramfb = NULL;
 
 /* @managed can be read from the xml document - it is always an
  * attribute of the toplevel element, no matter what type of
@@ -8176,6 +8177,7 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
 rawio = virXMLPropString(node, "rawio");
 model = virXMLPropString(node, "model");
 display = virXMLPropString(node, "display");
+ramfb = virXMLPropString(node, "ramfb");
 
 /* @type is passed in from the caller rather than read from the
  * xml document, because it is specified in different places for
@@ -8284,6 +8286,15 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
display);
 return -1;
 }
+
+if (ramfb &&
+(mdevsrc->ramfb = virTristateSwitchTypeFromString(ramfb)) <= 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("unknown value '%s' for  attribute "
+ "'ramfb'"),
+   ramfb);
+return -1;
+}
 }
 
 switch (def->source.subsys.type) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index b7ae57aa9d..c73034b5a7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -262,6 +262,7 @@ struct _virDomainHostdevSubsysMediatedDev {
 int model;  /* enum virMediatedDeviceModelType */
 int display; /* virTristateSwitch */
 char uuidstr[VIR_UUID_STRING_BUFLEN];   /* mediated device's uuid string */
+int ramfb; /* virTristateSwitch */
 };
 
 typedef enum {
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 50cc3bdf7c..049a5b54d9 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5417,6 +5417,17 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager,
 return virBufferContentAndReset();
 }
 
+static const char *
+qemuBuildHostdevMdevModelTypeString(virDomainHostdevSubsysMediatedDevPtr mdev)
+{
+/* when the 'ramfb' attribute is set, we must use the nohotplug variant
+ * rather than 'vfio-pci' */
+if (mdev->model == VIR_MDEV_MODEL_TYPE_VFIO_PCI &&
+mdev->ramfb == VIR_TRISTATE_SWITCH_ON)
+return "vfio-pci-nohotplug";
+
+return virMediatedDeviceModelTypeToString(mdev->model);
+}
 
 char *
 qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
@@ -5431,7 +5442,7 @@ qemuBuildHostdevMediatedDevStr(const virDomainDef *def,
 if (!(mdevPath = 

[libvirt] [PULL 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 f7dfb40256e..98294a76965 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);
 }
 
+

[libvirt] [PATCH v2 5/6] qemu: unify domain caps validation for video devices

[Jonathon Jongsma]

As suggested by Cole, this patch uses the domain capabilities to
validate the supported video model types. By using the domain
capabilities, the validation will automatically adjust when new enum
members are added.

Signed-off-by: Jonathon Jongsma 
---
 src/conf/domain_capabilities.c | 17 -
 src/qemu/qemu_domain.c | 45 --
 2 files changed, 16 insertions(+), 46 deletions(-)

diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index fe93388cce..baf33b4722 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -686,6 +686,19 @@ virDomainCapsDeviceRNGDefValidate(virDomainCapsPtr const 
caps,
 return 0;
 }
 
+static int
+virDomainCapsDeviceVideoDefValidate(virDomainCapsPtr const caps,
+  const virDomainVideoDefPtr dev)
+{
+if (ENUM_VALUE_MISSING(caps->video.modelType, dev->type)) {
+ENUM_VALUE_ERROR("video model",
+ virDomainVideoTypeToString(dev->type));
+return -1;
+}
+
+return 0;
+}
+
 
 int
 virDomainCapsDeviceDefValidate(virDomainCapsPtr const caps,
@@ -698,6 +711,9 @@ virDomainCapsDeviceDefValidate(virDomainCapsPtr const caps,
 case VIR_DOMAIN_DEVICE_RNG:
 ret = virDomainCapsDeviceRNGDefValidate(caps, dev->data.rng);
 break;
+case VIR_DOMAIN_DEVICE_VIDEO:
+ret = virDomainCapsDeviceVideoDefValidate(caps, dev->data.video);
+break;
 
 case VIR_DOMAIN_DEVICE_DISK:
 case VIR_DOMAIN_DEVICE_REDIRDEV:
@@ -706,7 +722,6 @@ virDomainCapsDeviceDefValidate(virDomainCapsPtr const caps,
 case VIR_DOMAIN_DEVICE_CHR:
 case VIR_DOMAIN_DEVICE_SMARTCARD:
 case VIR_DOMAIN_DEVICE_HOSTDEV:
-case VIR_DOMAIN_DEVICE_VIDEO:
 case VIR_DOMAIN_DEVICE_MEMORY:
 case VIR_DOMAIN_DEVICE_VSOCK:
 case VIR_DOMAIN_DEVICE_INPUT:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index def90a0f7d..bb34638427 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5717,51 +5717,6 @@ qemuDomainDeviceDefValidateVideo(const virDomainVideoDef 
*video,
 return -1;
 }
 } else {
-switch ((virDomainVideoType) video->type) {
-case VIR_DOMAIN_VIDEO_TYPE_NONE:
-return 0;
-case VIR_DOMAIN_VIDEO_TYPE_XEN:
-case VIR_DOMAIN_VIDEO_TYPE_VBOX:
-case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
-case VIR_DOMAIN_VIDEO_TYPE_GOP:
-case VIR_DOMAIN_VIDEO_TYPE_DEFAULT:
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("video type '%s' is not supported with QEMU"),
-   virDomainVideoTypeToString(video->type));
-return -1;
-case VIR_DOMAIN_VIDEO_TYPE_VGA:
-case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
-case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
-case VIR_DOMAIN_VIDEO_TYPE_QXL:
-case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
-case VIR_DOMAIN_VIDEO_TYPE_BOCHS:
-case VIR_DOMAIN_VIDEO_TYPE_RAMFB:
-case VIR_DOMAIN_VIDEO_TYPE_LAST:
-break;
-}
-if ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) ||
-(video->type == VIR_DOMAIN_VIDEO_TYPE_CIRRUS &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) ||
-(video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) ||
-(video->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) ||
-(video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU)) ||
-(video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
- video->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW)) ||
-(video->type == VIR_DOMAIN_VIDEO_TYPE_BOCHS &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_BOCHS_DISPLAY)) ||
-(video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&
- !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RAMFB))) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("this QEMU does not support '%s' video device"),
-   virDomainVideoTypeToString(video->type));
-return -1;
-}
-
 if (!video->primary &&
 video->type != VIR_DOMAIN_VIDEO_TYPE_QXL &&
 video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PULL 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 3ca814850e0..a08c92a4162 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-bl...@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/6] qemu: set domain capabilities for ramfb device

[Jonathon Jongsma]

commit 9bfcf0f62d9cf16db526a948242a7409ae883209 added the
QEMU_CAPS_DEVICE_RAMFB capability but did not set the domain capability.
This patch sets the domain capabilities for the ramfb device and updates
the tests.

Signed-off-by: Jonathon Jongsma 
---
 src/qemu/qemu_capabilities.c | 2 ++
 tests/domaincapsschemadata/qemu_3.0.0.s390x.xml  | 1 +
 tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml | 1 +
 tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml | 1 +
 tests/domaincapsschemadata/qemu_4.1.0.x86_64.xml | 1 +
 tests/domaincapsschemadata/qemu_4.2.0.x86_64.xml | 1 +
 6 files changed, 7 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 091e48c7e1..11559d3d92 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5400,6 +5400,8 @@ virQEMUCapsFillDomainDeviceVideoCaps(virQEMUCapsPtr 
qemuCaps,
 VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, VIR_DOMAIN_VIDEO_TYPE_VIRTIO);
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_BOCHS_DISPLAY))
 VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, VIR_DOMAIN_VIDEO_TYPE_BOCHS);
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_RAMFB))
+VIR_DOMAIN_CAPS_ENUM_SET(dev->modelType, VIR_DOMAIN_VIDEO_TYPE_RAMFB);
 
 return 0;
 }
diff --git a/tests/domaincapsschemadata/qemu_3.0.0.s390x.xml 
b/tests/domaincapsschemadata/qemu_3.0.0.s390x.xml
index 0e81e2ea33..d32f7b5875 100644
--- a/tests/domaincapsschemadata/qemu_3.0.0.s390x.xml
+++ b/tests/domaincapsschemadata/qemu_3.0.0.s390x.xml
@@ -158,6 +158,7 @@
 
   
 virtio
+ramfb
   
 
 
diff --git a/tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml 
b/tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml
index 059403cebc..ecb1f06e90 100644
--- a/tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_3.1.0.x86_64.xml
@@ -128,6 +128,7 @@
 qxl
 virtio
 bochs
+ramfb
   
 
 
diff --git a/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml 
b/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml
index eb24b9a604..2a475f20be 100644
--- a/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_4.0.0.x86_64.xml
@@ -128,6 +128,7 @@
 qxl
 virtio
 bochs
+ramfb
   
 
 
diff --git a/tests/domaincapsschemadata/qemu_4.1.0.x86_64.xml 
b/tests/domaincapsschemadata/qemu_4.1.0.x86_64.xml
index f5685d2068..07b869859c 100644
--- a/tests/domaincapsschemadata/qemu_4.1.0.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_4.1.0.x86_64.xml
@@ -132,6 +132,7 @@
 qxl
 virtio
 bochs
+ramfb
   
 
 
diff --git a/tests/domaincapsschemadata/qemu_4.2.0.x86_64.xml 
b/tests/domaincapsschemadata/qemu_4.2.0.x86_64.xml
index 5bd376bb2e..2f86eac7f0 100644
--- a/tests/domaincapsschemadata/qemu_4.2.0.x86_64.xml
+++ b/tests/domaincapsschemadata/qemu_4.2.0.x86_64.xml
@@ -132,6 +132,7 @@
 qxl
 virtio
 bochs
+ramfb
   
 
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PULL 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 000..4f6082c9d22
--- /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 000..2f0d98d0365
--- /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 

[libvirt] [PATCH v2 4/6] qemu: consolidate video validation

[Jonathon Jongsma]

Move video validation logic from qemuProcessStartValidateVideo() to
qemuDomainDeviceDefValidateVideo() (which is in fact called from the
aforementioned function).

Signed-off-by: Jonathon Jongsma 
---
 src/qemu/qemu_domain.c  | 172 +---
 src/qemu/qemu_process.c |  62 ---
 2 files changed, 107 insertions(+), 127 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index bc455e7da3..def90a0f7d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5706,83 +5706,125 @@ qemuDomainDeviceDefValidateHostdev(const 
virDomainHostdevDef *hostdev,
 
 
 static int
-qemuDomainDeviceDefValidateVideo(const virDomainVideoDef *video)
+qemuDomainDeviceDefValidateVideo(const virDomainVideoDef *video,
+ virQEMUCapsPtr qemuCaps)
 {
-switch ((virDomainVideoType) video->type) {
-case VIR_DOMAIN_VIDEO_TYPE_NONE:
-return 0;
-case VIR_DOMAIN_VIDEO_TYPE_XEN:
-case VIR_DOMAIN_VIDEO_TYPE_VBOX:
-case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
-case VIR_DOMAIN_VIDEO_TYPE_GOP:
-case VIR_DOMAIN_VIDEO_TYPE_DEFAULT:
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("video type '%s' is not supported with QEMU"),
-   virDomainVideoTypeToString(video->type));
-return -1;
-case VIR_DOMAIN_VIDEO_TYPE_VGA:
-case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
-case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
-case VIR_DOMAIN_VIDEO_TYPE_QXL:
-case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
-case VIR_DOMAIN_VIDEO_TYPE_BOCHS:
-case VIR_DOMAIN_VIDEO_TYPE_RAMFB:
-case VIR_DOMAIN_VIDEO_TYPE_LAST:
-break;
-}
-
-if (!video->primary &&
-video->type != VIR_DOMAIN_VIDEO_TYPE_QXL &&
-video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("video type '%s' is only valid as primary "
- "video device"),
-   virDomainVideoTypeToString(video->type));
-return -1;
-}
-
-if (video->accel && video->accel->accel2d == VIR_TRISTATE_SWITCH_ON) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("qemu does not support the accel2d setting"));
-return -1;
-}
-
-if (video->type == VIR_DOMAIN_VIDEO_TYPE_QXL) {
-if (video->vram > (UINT_MAX / 1024)) {
-virReportError(VIR_ERR_OVERFLOW,
-   _("value for 'vram' must be less than '%u'"),
-   UINT_MAX / 1024);
+if (video->backend == VIR_DOMAIN_VIDEO_BACKEND_TYPE_VHOSTUSER) {
+if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VHOST_USER_GPU)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("this QEMU does not support 'vhost-user' video 
device"));
+return -1;
+}
+} else {
+switch ((virDomainVideoType) video->type) {
+case VIR_DOMAIN_VIDEO_TYPE_NONE:
+return 0;
+case VIR_DOMAIN_VIDEO_TYPE_XEN:
+case VIR_DOMAIN_VIDEO_TYPE_VBOX:
+case VIR_DOMAIN_VIDEO_TYPE_PARALLELS:
+case VIR_DOMAIN_VIDEO_TYPE_GOP:
+case VIR_DOMAIN_VIDEO_TYPE_DEFAULT:
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("video type '%s' is not supported with QEMU"),
+   virDomainVideoTypeToString(video->type));
+return -1;
+case VIR_DOMAIN_VIDEO_TYPE_VGA:
+case VIR_DOMAIN_VIDEO_TYPE_CIRRUS:
+case VIR_DOMAIN_VIDEO_TYPE_VMVGA:
+case VIR_DOMAIN_VIDEO_TYPE_QXL:
+case VIR_DOMAIN_VIDEO_TYPE_VIRTIO:
+case VIR_DOMAIN_VIDEO_TYPE_BOCHS:
+case VIR_DOMAIN_VIDEO_TYPE_RAMFB:
+case VIR_DOMAIN_VIDEO_TYPE_LAST:
+break;
+}
+if ((video->type == VIR_DOMAIN_VIDEO_TYPE_VGA &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VGA)) ||
+(video->type == VIR_DOMAIN_VIDEO_TYPE_CIRRUS &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_CIRRUS_VGA)) ||
+(video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMWARE_SVGA)) ||
+(video->type == VIR_DOMAIN_VIDEO_TYPE_QXL &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL)) ||
+(video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU)) ||
+(video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
+ video->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU_CCW)) ||
+(video->type == VIR_DOMAIN_VIDEO_TYPE_BOCHS &&
+ !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_BOCHS_DISPLAY)) ||

[libvirt] [PATCH v2 2/6] qemu: use VIR_AUTOUNREF in qemuDomainDeviceDefValidate()

[Jonathon Jongsma]

This allows us to simplify the function and avoid jumping to 'cleanup'.

Signed-off-by: Jonathon Jongsma 
---
 src/qemu/qemu_domain.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index dc7568fe18..516ae7e444 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7197,8 +7197,8 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
 {
 int ret = 0;
 virQEMUDriverPtr driver = opaque;
-virQEMUCapsPtr qemuCaps = NULL;
-virDomainCapsPtr domCaps = NULL;
+VIR_AUTOUNREF(virQEMUCapsPtr) qemuCaps = NULL;
+VIR_AUTOUNREF(virDomainCapsPtr) domCaps = NULL;
 
 if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache,
 def->emulator)))
@@ -7208,13 +7208,13 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef 
*dev,
def->os.machine,
def->os.arch,
def->virtType)))
-goto cleanup;
+return -1;
 
 if ((ret = qemuDomainDeviceDefValidateAddress(dev, qemuCaps)) < 0)
-goto cleanup;
+return ret;
 
 if (virDomainCapsDeviceDefValidate(domCaps, dev, def) < 0)
-goto cleanup;
+return ret;
 
 switch ((virDomainDeviceType)dev->type) {
 case VIR_DOMAIN_DEVICE_NET:
@@ -7300,9 +7300,6 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
 break;
 }
 
- cleanup:
-virObjectUnref(qemuCaps);
-virObjectUnref(domCaps);
 return ret;
 }
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PULL 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 113d99bf520..b3398a13c20 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -737,8 +737,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 6dfc0835485..ebc1afccd3d 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PULL 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 257f0f67046..958e7474fb5 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 6065db80949..4bbb251b2c9 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 99812b418b8..6dfc0835485 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PULL 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 37c9de7446d..f5099435136 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 5b5b0337acc..aaf5d796284 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 

[libvirt] [PULL 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 aaf5d796284..c548885608d 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PULL 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
Signed-off-by: John Snow 
---
 include/block/dirty-bitmap.h   | 14 +-
 block/backup.c | 14 ++
 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 ++--
 8 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 973056778aa..2f9b088e11e 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 763f0d7ff6d..acb67da3a7b 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -352,7 +352,6 @@ static int coroutine_fn backup_before_write_notify(
 static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret)
 {
 BdrvDirtyBitmap *bm;
-BlockDriverState *bs = blk_bs(job->common.blk);
 bool sync = (((ret == 0) || (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS)) 
\
  && (job->bitmap_mode != BITMAP_SYNC_MODE_NEVER));
 
@@ -361,13 +360,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(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(bs, job->sync_bitmap, NULL);
+bm = bdrv_reclaim_dirty_bitmap(job->sync_bitmap, NULL);
 }
 
 assert(bm);
@@ -398,10 +397,9 @@ static void backup_abort(Job *job)
 static void backup_clean(Job *job)
 {
 BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-BlockDriverState *bs = blk_bs(s->common.blk);
 
 if (s->copy_bitmap) {
-bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
+bdrv_release_dirty_bitmap(s->copy_bitmap);
 s->copy_bitmap = NULL;
 }
 
@@ -679,7 +677,7 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 }
 
 /* Create a new bitmap, and freeze/disable this one. */
-if 

[libvirt] [PULL 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 5650dc7c874..951ea011a27 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 ae1213e6f86..fbc63e62f88 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PULL 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 2f9b088e11e..257f0f67046 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 bea03cfcc92..5b5b0337acc 100644
--- a/block.c
+++ b/block.c
@@ -5363,9 +5363,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 4e5c87a907f..6065db80949 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 687087d2bc2..99812b418b8 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 793f249aa5b..7eafface614 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PULL 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 07503b03b53..973056778aa 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 03e0872b972..4ecf18d5df7 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 

[libvirt] [PULL 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 44453ff8241..4e5c87a907f 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,
  

[libvirt] [PULL 00/19] Bitmaps patches

[John Snow]

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



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 |  14 +-
 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 +
 24 files changed, 624 insertions(+), 370 deletions(-)
 create mode 100755 tests/qemu-iotests/260
 create mode 100644 tests/qemu-iotests/260.out

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PULL 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 59441248451..bea03cfcc92 100644
--- a/block.c
+++ b/block.c
@@ -6555,25 +6555,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 134e0c9a0c8..8f42015db95 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PULL 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 a488d761ff0..2ed54821635 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -747,9 +747,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 0422acdf1c4..503ac9e3cd2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -556,9 +556,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 4b4b731b469..07503b03b53 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 8f42015db95..d1ae2e19229 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 b2487101ede..9821c1628f5 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

[libvirt] [PULL 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 2ed54821635..113d99bf520 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -743,12 +743,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 503ac9e3cd2..1e54486ad14 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -552,13 +552,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 d1ae2e19229..03e0872b972 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 

[libvirt] [PULL 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]
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 4afbe6292e3..1bf944ca3d1 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 eed5d288cbc..e1f867085f4 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 fd44c897ab0..757d39e360a 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(!(start & (gran - 1)));
+assert(!(count & (gran - 1)) || (start + count == hb->orig_size));
 
 trace_hbitmap_reset(hb, start, count,
 start >> hb->granularity, last >> hb->granularity);
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/1] IDE: deprecate ide-drive

[John Snow]

On 10/11/19 5:12 AM, Peter Krempa wrote:
> On Thu, Oct 10, 2019 at 14:08:12 -0400, John Snow wrote:
>> On 10/10/19 7:54 AM, Peter Krempa wrote:
>>> On Thu, Oct 10, 2019 at 13:42:26 +0200, Philippe Mathieu-Daudé wrote:
 On 10/10/19 1:26 PM, Peter Krempa wrote:
> 
> [...]
> 
>>> To be honest, I didn't really review the code nor the documentation.
>>> I actually reviewed only the idea itself in the context of integration
>>> with libvirt and that's why I didn't go for 'Reviewed-by:'.
>>>
>>> The gist of the citation above is that we should stick to well known
>>> tags with their well known meanings and I think that considering this a
>>> 'review' would be a stretch of the definiton.
>>>
>>
>> I wasn't aware that PMM wanted to avoid non-standard tags; I consider
>> them to be for human use, but I can change that behavior.
>>
>> Peter, I'll change it to an ACK (as suggested by Kevin) is that's OK by you.
> 
> Sure! I'll spell it out even for compliance:
> 
> ACKed-by: Peter Krempa 
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/3] qemu: use a bigger unplug timeout for PPC64 guests

[Daniel Henrique Barboza]

On 10/9/19 5:15 PM, Cole Robinson wrote:

On 9/11/19 5:05 PM, Daniel Henrique Barboza wrote:

For some architectures and setups, device removal can take
longer than the default 5 seconds. This results in commands
such as 'virsh setvcpus' to fire timeout messages even if
the operation were successful in the guest, confusing the
user.

This patch sets a new 10 seconds unplug timeout for PPC64
guests. All other archs will keep the default 5 seconds
timeout.

Instead of putting 'if PPC64' conditionals inside qemu_hotplug.c
to set the new timeout value, a new QEMU driver attribute
'unplugTimeout' was added. The timeout value is set during
qemuStateInitialize only once. All qemu_hotplug.c functions
that uses the timeout have easy access to a qemu_driver object,
thus the change to use unplugTimeout is straightforward.

The now unused 'qemuDomainRemoveDeviceWaitTime' global can't
be simply erased from qemu_hotplug.c though. Next patch will
remove it properly.



Sorry for the wrong review delay. I see this implements danpb's 
suggestion from the previous thread. The implementation seems a little 
odd to me though because it is differentiating on host arch, but this 
is about guest arch right? And probably an arbitrary number of 
options, like I imagine TCG would want a longer timeout too (though 
that's not anything you need to deal with)


I think it's sensible to say that a TCG guest would always required a
greater unplug timeout than a hardware accelerated one. However, never
thought about considering TCG guests in this patch series though. A shame.

So, considering that TCG guest exists, we can parametrize this unplug 
timeout
calculation by considering guest (not host) architecture and if it's a 
TCG or

a KVM guest. Speaking about the problem I'm trying to fix with ppc64 and
what we already have, we can roll out this unplug timeout logic like:

- pseries guest on KVM: 10 seconds
- everyone else on KVM: 5 seconds (untouched, like it is today)

For TCG guests, perhaps double the KVM timeout? This would need
experimentation, but double the timeout seems ok at first glance. Then we
can do:

- TCG pseries guest: 10 * 2 = 20 seconds
- TCG with every other arch guest = 5 * 2 = 10 seconds


This TCG calculation can be left alone for now as well - I can create 
the API
considering that TCG guest exists, but do not infer the timeout for it. 
Either

way works for me.



So I think this should be a function that lives in qemu_hotplug.c and 
acts on a DomainDef at least. The test suite will have to mock that in 
a qemuhotplugmock.c file, tests/qemucpumock.c is a good example to 
follow.


Just to check if we're on the same page, by 'I think this should be a 
function'

you mean the calculation of qemuDomainRemoveDeviceWaitTime that I just
mentioned above, right?




If you do that as an upfront patch, it can go in first. Then add the 
ppc changes in an add on patch.


Works for me.



Thanks,


DHB



You can CC me on the next version and I will review it

Thanks,
Cole


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/4] PCI multifunction partial assignment support

[Daniel Henrique Barboza]

On 10/10/19 9:27 PM, Abdulla Bubshait wrote:

On Mon,  7 Oct 2019 18:11:32 -0300
Daniel Henrique Barboza  wrote:
> These series tries to solve the partial assignment of multifunction
> hostdev PCI devices by introducing a new hostdev attribute called
> 'assigned'. This is how it works:
> 
> - it is a boolean value that will be efffective just for

> multifunction hostdev PCI devices, since there's no other
> occurrence for this kind of use in Libvirt. Trying to
> declare assign='yes|no' in any other PCI hostdev device
> will cause parse errors;
I think this functionality should be available to all hostdev PCI
devices and not just multifunction ones. You might have several
devices that are part of the same IOMMU group that you don't mind
giving up from the host, but don't want to supply all of them to the
guest. This would be an alternative to using ACS patch in these
situations.

One example is having two nvme drives in a single IOMMU group, you
will be able to pass a single one to the guest rather than being
forced to pass both.


You're right. This can be extended to all PCI hostdev devices. The
conditions that I'm handling here (detaching/reattaching all the
devices that belongs in the same IOMMU) isn't exclusive to PCI
multifunction devices.

Thanks for the input. I'll re-spin this series considering all PCI hostdev
devices this time.


DHB



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/30] storagefile: qcow1: Check for BACKING_STORE_OK

[Cole Robinson]

On 10/11/19 9:05 AM, Michal Privoznik wrote:

On 10/7/19 11:49 PM, Cole Robinson wrote:

Check explicitly for BACKING_STORE_OK and not its 0 value

Signed-off-by: Cole Robinson 
---
  src/util/virstoragefile.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 51726006e7..1549067c48 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -578,7 +578,7 @@ qcow1GetBackingStore(char **res,
   * used to store backing format */
  *format = VIR_STORAGE_FILE_AUTO;
  ret = qcowXGetBackingStore(res, NULL, buf, buf_size, false);
-    if (ret == 0 && *buf == '\0')
+    if (ret == BACKING_STORE_OK && *buf == '\0')
  *format = VIR_STORAGE_FILE_NONE;
  return ret;
  }



We can make qcowXGetBackingStore() return the enum type instead of plain 
int. But that can be done in a follow up (trivial) patch. When doing 
that, both qcow1GetBackingStore() and qcow2GetBackingStore() and also 
getBackingStore() callback can use the same tretement then.


// after seeing future patches

Ah, you're removing some functions, but you get the idea.



FYI I've pushed the series now. I think after everything is applied 
there isn't anything left to do here regarding your suggestion? But 
correct me if I'm wrong


Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] security: apparmor: Label externalDataStore

[Cole Robinson]

Teach virt-aa-helper how to label a qcow2 data_file, tracked internally
as externalDataStore. It should be treated the same as its sibling
disk image

Signed-off-by: Cole Robinson 
---
Compiled but not runtime tested, I don't have an apparmor setup

 src/security/virt-aa-helper.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 509187ac36..fe6fa12550 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -949,6 +949,10 @@ storage_source_add_files(virStorageSourcePtr src,
 if (add_file_path(tmp, depth, buf) < 0)
 return -1;
 
+if (src->externalDataStore &&
+storage_source_add_files(src->externalDataStore, buf, depth) < 0)
+return -1;
+
 depth++;
 }
 
-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 12/30] storagefile: Fix backing format \0 check

[Cole Robinson]

On 10/11/19 9:04 AM, Michal Privoznik wrote:

On 10/7/19 11:49 PM, Cole Robinson wrote:

From qemu.git docs/interop/qcow2.txt


   == String header extensions ==

   Some header extensions (such as the backing file format name and
   the external data file name) are just a single string. In this case,
   the header extension length is the string length and the string is
   not '\0' terminated. (The header extension padding can make it look
   like a string is '\0' terminated, but neither is padding always
   necessary nor is there a guarantee that zero bytes are used
   for padding.)

So we shouldn't be checking for a \0 byte at the end of the backing
format section. I think in practice there always is a \0 but we
shouldn't depend on that.

Signed-off-by: Cole Robinson 
---
  src/util/virstoragefile.c | 13 +
  1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 8cd576a463..5a4e4b24ae 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -506,13 +506,18 @@ qcow2GetExtensions(const char *buf,
  case QCOW2_HDR_EXTENSION_END:
  goto done;
-    case QCOW2_HDR_EXTENSION_BACKING_FORMAT:
-    if (buf[offset+len] != '\0')
-    break;
-    *backingFormat = 
virStorageFileFormatTypeFromString(buf+offset);

+    case QCOW2_HDR_EXTENSION_BACKING_FORMAT: {
+    VIR_AUTOFREE(char *) tmp = NULL;
+    if (VIR_ALLOC_N(tmp, len + 1) < 0)
+    return -1;
+    memcpy(tmp, buf + offset, len);
+    tmp[len] = '\0';
+
+    *backingFormat = virStorageFileFormatTypeFromString(tmp);
  if (*backingFormat <= VIR_STORAGE_FILE_NONE)
  return -1;
  }
+    }


Pre-existing, but there's missing 'break;' here. Since you're touching 
these lines, might be worth putting it here.




Thanks, will fix before pushing

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 12/30] storagefile: Fix backing format \0 check

[Cole Robinson]

On 10/9/19 5:34 PM, Daniel Henrique Barboza wrote:



On 10/7/19 6:49 PM, Cole Robinson wrote:

>From qemu.git docs/interop/qcow2.txt


Here's the '>' again. I think this is something you're using to cite an
external source in the commit message. Is that it?




   == String header extensions ==

   Some header extensions (such as the backing file format name and
   the external data file name) are just a single string. In this case,
   the header extension length is the string length and the string is
   not '\0' terminated. (The header extension padding can make it look
   like a string is '\0' terminated, but neither is padding always
   necessary nor is there a guarantee that zero bytes are used
   for padding.)

So we shouldn't be checking for a \0 byte at the end of the backing
format section. I think in practice there always is a \0 but we
shouldn't depend on that.

Signed-off-by: Cole Robinson 
---
  src/util/virstoragefile.c | 13 +
  1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 8cd576a463..5a4e4b24ae 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -506,13 +506,18 @@ qcow2GetExtensions(const char *buf,
  case QCOW2_HDR_EXTENSION_END:
  goto done;
-    case QCOW2_HDR_EXTENSION_BACKING_FORMAT:
-    if (buf[offset+len] != '\0')
-    break;
-    *backingFormat = 
virStorageFileFormatTypeFromString(buf+offset);

+    case QCOW2_HDR_EXTENSION_BACKING_FORMAT: {
+    VIR_AUTOFREE(char *) tmp = NULL;
+    if (VIR_ALLOC_N(tmp, len + 1) < 0)
+    return -1;
+    memcpy(tmp, buf + offset, len);
+    tmp[len] = '\0';
+
+    *backingFormat = virStorageFileFormatTypeFromString(tmp);
  if (*backingFormat <= VIR_STORAGE_FILE_NONE)
  return -1;
  }
+    }


I suppose this extra brace is due to the new brace right after label that
you added up there. I'm probably being a purist here, but I'd rather make
an extra indent level in each 'case' statement of this switch just to avoid
this  braces right below each other. We have this style of switch 
statement in

the code, so I think it would be ok.


That's my personal inclination as well, but a 'git grep -A1 "switch ("' 
shows that the vast majority of switch statements don't indent the case. 
The weirdness with the bracket here is just a side effect of that.


I swapped the case blocks around so at least the brackets are on 
adjacent lines :)


Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/30] storagefile: Rename qcow2GetExtensions 'format' argument

[Cole Robinson]

On 10/9/19 5:14 PM, Daniel Henrique Barboza wrote:



On 10/7/19 6:49 PM, Cole Robinson wrote:

To backingFormat, which makes it more clear. Move it to the end of
the argument list which will scale nicer with future patches

Signed-off-by: Cole Robinson 
---


This really makes it clearer. I was getting confused about whether
'format' was referring to the image or its backing file in these
patches.




  src/util/virstoragefile.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index d6bd38777b..8cd576a463 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -427,9 +427,9 @@ cowGetBackingStore(char **res,
  static int
-qcow2GetExtensions(int *format,
-   const char *buf,
-   size_t buf_size)
+qcow2GetExtensions(const char *buf,
+   size_t buf_size,
+   int *backingFormat)
  {
  size_t offset;
  size_t extension_start;
@@ -509,8 +509,8 @@ qcow2GetExtensions(int *format,
  case QCOW2_HDR_EXTENSION_BACKING_FORMAT:
  if (buf[offset+len] != '\0')
  break;
-    *format = virStorageFileFormatTypeFromString(buf+offset);
-    if (*format <= VIR_STORAGE_FILE_NONE)
+    *backingFormat = 
virStorageFileFormatTypeFromString(buf+offset);

+    if (*backingFormat <= VIR_STORAGE_FILE_NONE)
  return -1;
  }
@@ -561,7 +561,7 @@ qcowXGetBackingStore(char **res,
  memcpy(*res, buf + offset, size);
  (*res)[size] = '\0';
-    if (qcow2GetExtensions(format, buf, buf_size) < 0)
+    if (qcow2GetExtensions(buf, buf_size, format) < 0)
  return BACKING_STORE_INVALID;


Not sure if the next patches are changing it or making it obsolete, but 
you can

change 'format' to 'backingFormat' inside qcowXGetBackingStore too.



Later patches don't change it, as you've seen. We could change it, but 
context wise it's less necessary there IMO, and also we should then 
change it for the other GetBackingStore functions as well.


Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/30] storagefile, security: qcow2 data_file support

[Cole Robinson]

On 10/11/19 9:45 AM, Ján Tomko wrote:

On Mon, Oct 07, 2019 at 05:49:14PM -0400, Cole Robinson wrote:

This series is the first steps to teaching libvirt about qcow2
data_file support, aka external data files or qcow2 external metadata.

A bit about the feature: it was added in qemu 4.0. It essentially
creates a two part image file: a qcow2 layer that just tracks the
image metadata, and a separate data file which is stores the VM
disk contents. AFAICT the driving use case is to keep a fully coherent
raw disk image on disk, and only use qcow2 as an intermediate metadata
layer when necessary, for things like incremental backup support.

The original qemu patch posting is here:
https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07496.html

For testing, you can create a new qcow2+raw data_file image from an
existing image, like:

   qemu-img convert -O qcow2 \
   -o data_file=NEW.raw,data_file_raw=yes
   EXISTING.raw NEW.qcow2

The goal of this series is to teach libvirt enough about this case
so that we can correctly relabel the data_file on VM startup/shutdown.
The main functional changes are

 * Teach storagefile how to parse out data_file from the qcow2 header
 * Store the raw string as virStorageSource->externalDataStoreRaw
 * Track that as its out virStorageSource in externalDataStore
 * dac/selinux relabel externalDataStore as needed


From libvirt's perspective, externalDataStore is conceptually pretty

close to a backingStore, but the main difference is its read/write
permissions should match its parent image, rather than being readonly
like backingStore.

This series has only been tested on top of the -blockdev enablement
series, but I don't think it actually interacts with that work at
the moment.


Future work:
 * Exposing this in the runtime XML. We need to figure out an XML


This also belongs in the persistent XML.



Agreed


   schema. It will reuse virStorageSource obviously, but the main
   thing to figure out is probably 1) what the top element name
   should be ('dataFile' maybe?), 2) where it sits in the XML
   hierarchy (under  or under  I guess)



 maybe?


The way this code is structured, we have

src->path = FOO.qcow2
src->externalDataStore-> FOO.raw

FOO.raw contains the disk/OS contents, FOO.qcow2 just the qcow2 
metadata. If we reflect that layout in the XML, we have



  

  

  


If we called it metadataStore it sounds like the layout is inverted.


 >>  * Exposing this on the qemu -blockdev command line. Similar to how

   in the blockdev world we are explicitly putting the disk backing
   chain on the command line, we can do that for data_file too.


Historically, not being explicit on the command line and letting QEMU
do the right thing has bitten us, so yes, we have to do it for data_file
too.


Then
   like persistent  XML the user will have the power
   to overwrite the data_file location for an individual VM run.



If the point of the thin qcow2 layer is to contain the dirty bitmaps for
incremental backup then running this then you might as well use a
different metadata_file? Otherwise the metadata won't match the actual
data.



I'm not sure I follow this part, but maybe that's due to data_file 
naming mixup



OTOH, I can imagine throwing away the metadata file and starting over.



Yes this is one of the main drivers I think. That the qcow2 layer gives 
qcow2 native features like dirty bitmaps, but if it ever comes to it, 
the data is still in raw format which simplifies processing the image 
with other tools. Plus raw is less of a boogieman than qcow2 for some 
people, so I think there's some marketing opportunity behind it to say 
'see your data is still there in FOO.raw'.


There's probably cases where the user would want to ditch the top level 
layer and use that data raw layer directly, but similar to writing to a 
backing image, it invalidates the top layer, and there's no rebase 
operation for data_file AFAICT. But the persistent XML will allow 
configuring that if someone wanted it



 * Figure out how we expect ovirt/rhev to be using this at runtime.
   Possibly taking a running VM using a raw image, doing blockdev-*
   magic to pivot it to qcow2+raw data_file, so it can initiate
   incremental backup on top of a previously raw only VM?


Known issues:
 * In the qemu driver, the qcow2 image metadata is only parsed
   in -blockdev world if no  is specified in the
   persistent XML. So basically if there's a  listed,
   we never parse the qcow2 header and detect the presence of
   data_file. Fixable I'm sure but I didn't look into it much yet.


This will be fixed by introducing an XML element for it.



It's part of the fix I think. We will still need to change qemu_block.c 
logic to accomodate this in some way. Right now, whether we probe the 
qcow2 file metadata is only dependent on  in the 
persistent XML or not. But now the probing provides info on both 
backingStore and externalDataStore, so tying probing only to prescence 
of 

Re: [libvirt] [PATCH v5 1/1] tests: add a test for driver.c:virConnectValidateURIPath()

[Cole Robinson]

On 10/9/19 3:11 PM, Daniel Henrique Barboza wrote:

Signed-off-by: Daniel Henrique Barboza 
---
  tests/Makefile.am |  7 ++-
  tests/virdriverconnvalidatetest.c | 90 +++
  2 files changed, 96 insertions(+), 1 deletion(-)
  create mode 100644 tests/virdriverconnvalidatetest.c


Reviewed-by: Cole Robinson 

and pushed

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/30] storagefile, security: qcow2 data_file support

[Cole Robinson]

On 10/10/19 11:25 PM, Han Han wrote:

Hi Cole,
I merged crobinso/qcow2-data_file branch to 37b565c00. Reserved new 
capabilities introduced by these to branches to resolve conflicts.

Then build and test as following:
# ./autogen.sh&& ./configure --without-libssh 
--build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu 
--program-prefix= --disable-dependency-tracking --prefix=/usr 
--exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin 
--sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include 
--libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var 
--sharedstatedir=/var/lib --mandir=/usr/share/man 
--infodir=/usr/share/info --with-qemu --without-openvz --without-lxc 
--without-vbox --without-libxl --with-sasl --with-polkit --with-libvirtd 
--without-phyp --with-esx --without-hyperv --without-vmware 
--without-xenapi --without-vz --without-bhyve --with-interface 
--with-network --with-storage-fs --with-storage-lvm --with-storage-iscsi 
--with-storage-iscsi-direct --with-storage-scsi --with-storage-disk 
--with-storage-mpath --with-storage-rbd --without-storage-sheepdog 
--with-storage-gluster --without-storage-zfs --without-storage-vstorage 
--with-numactl --with-numad --with-capng --without-fuse --with-netcf 
--with-selinux --with-selinux-mount=/sys/fs/selinux --without-apparmor 
--without-hal --with-udev --with-yajl --with-sanlock --with-libpcap 
--with-macvtap --with-audit --with-dtrace --with-driver-modules 
--with-firewalld --with-firewalld-zone --without-wireshark-dissector 
--without-pm-utils --with-nss-plugin '--with-packager=Unknown, 
2019-08-19-12:13:01, lab.rhel8.me ' 
--with-packager-version=1.el8 --with-qemu-user=qemu 
--with-qemu-group=qemu --with-tls-priority=@LIBVIRT,SYSTEM 
--enable-werror --enable-expensive-tests --with-init-script=systemd 
--without-login-shell && make


Start libvirtd and virtlogd
# LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')" src/.libs/libvirtd
# LD_PRELOAD="$(find src -name '*.so.*'|tr '\n' ' ')" ./src/virtlogd

Then try to list all domains:
# virsh list --all

Libvirtd exits with segment fault:
[1]    30104 segmentation fault (core dumped)  LD_PRELOAD="$(find src 
-name '*.so.*'|tr '\n' ' ')" src/.libs/libvirtd


Version:
qemu-4.1

Backtrace:
(gdb) bt
#0  0x7fbe57a0d1b9 in virDomainVirtioSerialAddrSetAddControllers 
(def=, def=, addrs=) at 
conf/domain_addr.c:1656
#1  virDomainVirtioSerialAddrSetCreateFromDomain 
(def=def@entry=0x7fbde81cc3f0) at conf/domain_addr.c:1753
#2  0x7fbe0179897e in qemuDomainAssignVirtioSerialAddresses 
(def=0x7fbde81cc3f0) at qemu/qemu_domain_address.c:3174
#3  qemuDomainAssignAddresses (def=0x7fbde81cc3f0, 
qemuCaps=0x7fbde81d2210, driver=0x7fbde8126850, obj=0x0, 
newDomain=) at qemu/qemu_domain_address.c:3174
#4  0x7fbe57a39e0d in virDomainDefPostParse 
(def=def@entry=0x7fbde81cc3f0, caps=caps@entry=0x7fbde8154d20, 
parseFlags=parseFlags@entry=4610, xmlopt=xmlopt@entry=0x7fbde83ce070,

     parseOpaque=parseOpaque@entry=0x0) at conf/domain_conf.c:5858
#5  0x7fbe57a525c5 in virDomainDefParseNode (xml=, 
root=0x7fbde83c5ff0, caps=0x7fbde8154d20, xmlopt=0x7fbde83ce070, 
parseOpaque=0x0, flags=4610) at conf/domain_conf.c:21677
#6  0x7fbe57a526c8 in virDomainDefParse (xmlStr=xmlStr@entry=0x0, 
filename=, caps=caps@entry=0x7fbde8154d20, 
xmlopt=xmlopt@entry=0x7fbde83ce070, parseOpaque=parseOpaque@entry=0x0,

     flags=flags@entry=4610) at conf/domain_conf.c:21628
#7  0x7fbe57a528f6 in virDomainDefParseFile (filename=out>, caps=caps@entry=0x7fbde8154d20, 
xmlopt=xmlopt@entry=0x7fbde83ce070, parseOpaque=parseOpaque@entry=0x0, 
flags=flags@entry=4610)

     at conf/domain_conf.c:21653
#8  0x7fbe57a5e16a in virDomainObjListLoadConfig (opaque=0x0, 
notify=0x0, name=0x7fbde81d7ff3 "pc", autostartDir=0x7fbde8124070 
"/etc/libvirt/qemu/autostart", configDir=0x7fbde8124050 
"/etc/libvirt/qemu",
     xmlopt=0x7fbde83ce070, caps=0x7fbde8154d20, doms=0x7fbde8126940) at 
conf/virdomainobjlist.c:503
#9  virDomainObjListLoadAllConfigs (doms=0x7fbde8126940, 
configDir=0x7fbde8124050 "/etc/libvirt/qemu", 
autostartDir=0x7fbde8124070 "/etc/libvirt/qemu/autostart", 
liveStatus=liveStatus@entry=false,
     caps=0x7fbde8154d20, xmlopt=0x7fbde83ce070, notify=0x0, opaque=0x0) 
at conf/virdomainobjlist.c:625
#10 0x7fbe017f57e2 in qemuStateInitialize (privileged=true, 
callback=, opaque=) at 
qemu/qemu_driver.c:1007
#11 0x7fbe57b8033d in virStateInitialize (privileged=true, 
mandatory=mandatory@entry=false, callback=callback@entry=0x55dfb702ecc0 
, opaque=opaque@entry=0x55dfb8869d60)

     at libvirt.c:666
#12 0x55dfb702ed1d in daemonRunStateInit (opaque=0x55dfb8869d60) at 
remote/remote_daemon.c:846
#13 0x7fbe579f4be2 in virThreadHelper (data=) at 
util/virthread.c:196
#14 0x7fbe55a322de in start_thread (arg=) at 
pthread_create.c:486
#15 0x7fbe55763133 in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:95


Could you please check this issue?
The full 

Re: [libvirt] [PATCH 3/3] security: Don't remember labels for TPM

[Cole Robinson]

On 10/11/19 11:08 AM, Michal Privoznik wrote:

On 10/10/19 10:15 PM, Cole Robinson wrote:

On 10/1/19 11:00 AM, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1755803

The /dev/tpmN file can be opened only once, as implemented in
drivers/char/tpm/tpm-dev.c:tpm_open() from the kernel's tree. Any
other attempt to open the file fails. And since we're opening the
file ourselves and passing the FD to qemu we will not succeed
opening the file again when locking it for seclabel remembering.

Signed-off-by: Michal Privoznik 
---
  src/security/security_dac.c | 18 +-
  src/security/security_selinux.c | 10 +-
  2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 2733fa664f..347a7a5f63 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1653,14 +1653,14 @@ 
virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr,

  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-    ret = virSecurityDACSetChardevLabel(mgr, def,
- >data.passthrough.source,
-    false);
+    ret = virSecurityDACSetChardevLabelHelper(mgr, def,
+ >data.passthrough.source,
+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
-    ret = virSecurityDACSetChardevLabel(mgr, def,
-    >data.emulator.source,
-    false);
+    ret = virSecurityDACSetChardevLabelHelper(mgr, def,
+ >data.emulator.source,
+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_LAST:
  break;
@@ -1679,9 +1679,9 @@ 
virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr,

  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-    ret = virSecurityDACRestoreChardevLabel(mgr, def,
- >data.passthrough.source,
-    false);
+    ret = virSecurityDACRestoreChardevLabelHelper(mgr, def,
+ >data.passthrough.source,
+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
  /* swtpm will have removed the Unix socket upon termination */
diff --git a/src/security/security_selinux.c 
b/src/security/security_selinux.c

index e3be724a2b..0486bdd6b6 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1682,14 +1682,14 @@ 
virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr,

  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
  tpmdev = tpm->data.passthrough.source.data.file.path;
-    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, true);
+    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, false);

  if (rc < 0)
  return -1;
  if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) {
  rc = virSecuritySELinuxSetFilecon(mgr,
    cancel_path,
-  seclabel->imagelabel, 
true);
+  seclabel->imagelabel, 
false);

  VIR_FREE(cancel_path);
  if (rc < 0) {
  virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, 
tpm);
@@ -1701,7 +1701,7 @@ 
virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr,

  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
  tpmdev = tpm->data.emulator.source.data.nix.path;
-    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, true);
+    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, false);

  if (rc < 0)
  return -1;
  break;


Are the EMULATOR changes actually required? I think it just uses a 
unix socket and doesn't touch the host kernel tpm subsystem


Well, not per se, because the socket is created on domain startup and 
then unlink()-ed on domain shutdown. That's why it doesn't make sense to 
try remember anything about the socket.


Okay so it's basically a separate issue from the patch series. In that 
case use of virSecurityDACRestoreChardevLabelHelper for EMULATOR is a 
bit misleading, and instead virSecurityDACRestoreChardevLabel should 
probably grow some smarts to set remember=false for any unix socket path 
it sees. But it's a minor distinction because I don't think it matters 
in practice given that sockets are unlinked like you say


Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v4 03/26] build-aux: rewrite po file minimizer in Python

[Ján Tomko]

On Wed, Oct 09, 2019 at 12:37:18PM +0100, Daniel P. Berrangé wrote:

As part of an goal to eliminate Perl from libvirt build tools,
rewrite the minimize-po.pl tool in Python.

This was a straight conversion, manually going line-by-line to
change the syntax from Perl to Python. Thus the overall structure
of the file and approach is the same.

Signed-off-by: Daniel P. Berrangé 
---
Makefile.am  |  2 +-
build-aux/minimize-po.pl | 37 -
po/Makefile.am   |  2 +-
scripts/minimize-po.py   | 58 
4 files changed, 60 insertions(+), 39 deletions(-)
delete mode 100755 build-aux/minimize-po.pl
create mode 100755 scripts/minimize-po.py

diff --git a/Makefile.am b/Makefile.am
index 9c16fa72ec..02506f2183 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -48,7 +48,7 @@ EXTRA_DIST = \
  scripts/augeas-gentest.py \
  build-aux/check-spacing.pl \
  build-aux/header-ifdef.pl \
-  build-aux/minimize-po.pl \
+  scripts/minimize-po.py \
  build-aux/mock-noinline.pl \
  build-aux/prohibit-duplicate-header.pl \
  build-aux/useless-if-before-free \
diff --git a/po/Makefile.am b/po/Makefile.am
index da117edbd5..b0e2c15d44 100644
--- a/po/Makefile.am
+++ b/po/Makefile.am
@@ -58,7 +58,7 @@ update-mini-po: $(POTFILE)
  $(MSGMERGE) --no-location --no-fuzzy-matching --sort-output \
$$lang.po $(POTFILE) | \
  $(SED) $(SED_PO_FIXUP_ARGS) | \
- $(PERL) $(top_srcdir)/build-aux/minimize-po.pl > \
+ $(RUNUTF8) $(PYTHON) $(top_srcdir)/scripts/minimize-po.py > \
$(srcdir)/$$lang.mini.po ; \
done

diff --git a/scripts/minimize-po.py b/scripts/minimize-po.py
new file mode 100755
index 00..61454a5eea
--- /dev/null
+++ b/scripts/minimize-po.py
@@ -0,0 +1,58 @@
+#!/usr/bin/env python
+#
+# Copyright (C) 2018-2019 Red Hat, Inc.
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2.1 of the License, or (at your option) any later version.
+#
+# This library 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
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library.  If not, see
+# .
+
+from __future__ import print_function
+
+import re
+import sys
+
+block = []
+msgstr = False
+empty = False
+unused = False
+fuzzy = False
+
+for line in sys.stdin:
+line = line.rstrip("\n")
+


You can get rid of the stripping (and creating an extra copy)
by using


+if line == "":


line.isspace()


+if not empty and not unused and not fuzzy:
+for b in block:
+print(b)


print(b, end='')


+
+block = []
+msgstr = False
+fuzzy = False
+block.append(line)
+else:
+if line.startswith("msgstr"):
+msgstr = True
+empty = True
+
+if line[0] == '#' and line.find("fuzzy") != -1:


and "fuzzy" in line

If you don't care about the exact position, it's faster and more
readable.


+fuzzy = True
+if line.startswith("#~ msgstr"):
+unused = True
+if msgstr and re.search(r'''".+"''', line):


Why the triple quotes for a single-line string?


+empty = False
+
+block.append(line)
+
+if not empty and not unused and not fuzzy:
+for b in block:
+print(b)


print(b, end='')
if you go with storing unstripped lines

Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] security: Don't remember labels for TPM

[Michal Privoznik]

On 10/10/19 10:15 PM, Cole Robinson wrote:

On 10/1/19 11:00 AM, Michal Privoznik wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1755803

The /dev/tpmN file can be opened only once, as implemented in
drivers/char/tpm/tpm-dev.c:tpm_open() from the kernel's tree. Any
other attempt to open the file fails. And since we're opening the
file ourselves and passing the FD to qemu we will not succeed
opening the file again when locking it for seclabel remembering.

Signed-off-by: Michal Privoznik 
---
  src/security/security_dac.c | 18 +-
  src/security/security_selinux.c | 10 +-
  2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 2733fa664f..347a7a5f63 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1653,14 +1653,14 @@ 
virSecurityDACSetTPMFileLabel(virSecurityManagerPtr mgr,

  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-    ret = virSecurityDACSetChardevLabel(mgr, def,
-
>data.passthrough.source,

-    false);
+    ret = virSecurityDACSetChardevLabelHelper(mgr, def,
+  
>data.passthrough.source,

+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
-    ret = virSecurityDACSetChardevLabel(mgr, def,
-    >data.emulator.source,
-    false);
+    ret = virSecurityDACSetChardevLabelHelper(mgr, def,
+  
>data.emulator.source,

+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_LAST:
  break;
@@ -1679,9 +1679,9 @@ 
virSecurityDACRestoreTPMFileLabel(virSecurityManagerPtr mgr,

  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
-    ret = virSecurityDACRestoreChardevLabel(mgr, def,
-
>data.passthrough.source,

-    false);
+    ret = virSecurityDACRestoreChardevLabelHelper(mgr, def,
+  
>data.passthrough.source,

+  false, false);
  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
  /* swtpm will have removed the Unix socket upon termination */
diff --git a/src/security/security_selinux.c 
b/src/security/security_selinux.c

index e3be724a2b..0486bdd6b6 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1682,14 +1682,14 @@ 
virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr,

  switch (tpm->type) {
  case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH:
  tpmdev = tpm->data.passthrough.source.data.file.path;
-    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, true);
+    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, false);

  if (rc < 0)
  return -1;
  if ((cancel_path = virTPMCreateCancelPath(tpmdev)) != NULL) {
  rc = virSecuritySELinuxSetFilecon(mgr,
    cancel_path,
-  seclabel->imagelabel, 
true);
+  seclabel->imagelabel, 
false);

  VIR_FREE(cancel_path);
  if (rc < 0) {
  virSecuritySELinuxRestoreTPMFileLabelInt(mgr, def, 
tpm);
@@ -1701,7 +1701,7 @@ 
virSecuritySELinuxSetTPMFileLabel(virSecurityManagerPtr mgr,

  break;
  case VIR_DOMAIN_TPM_TYPE_EMULATOR:
  tpmdev = tpm->data.emulator.source.data.nix.path;
-    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, true);
+    rc = virSecuritySELinuxSetFilecon(mgr, tpmdev, 
seclabel->imagelabel, false);

  if (rc < 0)
  return -1;
  break;


Are the EMULATOR changes actually required? I think it just uses a unix 
socket and doesn't touch the host kernel tpm subsystem


Well, not per se, because the socket is created on domain startup and 
then unlink()-ed on domain shutdown. That's why it doesn't make sense to 
try remember anything about the socket.


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 18/30] storagefile: Add externalDataStore member

[Daniel Henrique Barboza]

On 10/11/19 10:04 AM, Michal Privoznik wrote:

On 10/10/19 5:09 PM, Daniel Henrique Barboza wrote:



On 10/7/19 6:49 PM, Cole Robinson wrote:

Add the plumbing to track a externalDataStoreRaw as a virStorageSource

Signed-off-by: Cole Robinson 
---
  src/util/virstoragefile.c | 9 +
  src/util/virstoragefile.h | 3 +++
  2 files changed, 12 insertions(+)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7ae6719dd6..ce669b6e0b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2339,6 +2339,12 @@ virStorageSourceCopy(const virStorageSource 
*src,

  return NULL;
  }
+    if (src->externalDataStore) {
+    if (!(def->externalDataStore = 
virStorageSourceCopy(src->externalDataStore,

+ true)))
+    return NULL;
+    }
+
  VIR_STEAL_PTR(ret, def);
  return ret;
  }
@@ -2560,6 +2566,9 @@ virStorageSourceClear(virStorageSourcePtr def)
  VIR_FREE(def->timestamps);
  VIR_FREE(def->externalDataStoreRaw);
+    virObjectUnref(def->externalDataStore);
+    def->externalDataStore = NULL;


Is this assign to NULL necessary? virObjectUnref will call VIR_FREE() in
def->externalDataStore if there is no more references to it (which will
assign it to NULL in the end).


It is. Point of virXXXClear() functions is to clear passed struct. 
That is, in theory (I'm not saying it's the case of 
virStorageSourceDef and also I'm not saying it isn't), it should be 
possible to re-use a structure after it's been cleared. But for that 
to happen, all poitners must be reset to NULL regardless of 
refcounters and stuff.




Got it. I didn't get across an example of this re-use yet, didn't know 
it is a

valid possibility.

However, there's a memset() called at the end of this function which 
sets all members of this struct to 0, so in the end I agree that the 
line is redundant, but for a different reason :-D





I was right in the end! That all I'm going to remember ;)


DHB



Michal


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] libvirt doc - missing entry in input devices section

[Ján Tomko]

On Mon, Oct 07, 2019 at 11:23:07AM +0300, Ruth Netser wrote:

The option   is missing from code box under
https://libvirt.org/formatdomain.html#elementsInput section.
(Exists for the other devices).


The boxes with examples are not intended to be exhaustive - the 'xen'
bus and 'ps2' bus examples are also missing.

Would labeling the code box as 'Example' help?

Jano



--
Ruty Netser



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list




signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/30] storagefile, security: qcow2 data_file support

[Ján Tomko]

On Mon, Oct 07, 2019 at 05:49:14PM -0400, Cole Robinson wrote:

This series is the first steps to teaching libvirt about qcow2
data_file support, aka external data files or qcow2 external metadata.

A bit about the feature: it was added in qemu 4.0. It essentially
creates a two part image file: a qcow2 layer that just tracks the
image metadata, and a separate data file which is stores the VM
disk contents. AFAICT the driving use case is to keep a fully coherent
raw disk image on disk, and only use qcow2 as an intermediate metadata
layer when necessary, for things like incremental backup support.

The original qemu patch posting is here:
https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07496.html

For testing, you can create a new qcow2+raw data_file image from an
existing image, like:

   qemu-img convert -O qcow2 \
   -o data_file=NEW.raw,data_file_raw=yes
   EXISTING.raw NEW.qcow2

The goal of this series is to teach libvirt enough about this case
so that we can correctly relabel the data_file on VM startup/shutdown.
The main functional changes are

 * Teach storagefile how to parse out data_file from the qcow2 header
 * Store the raw string as virStorageSource->externalDataStoreRaw
 * Track that as its out virStorageSource in externalDataStore
 * dac/selinux relabel externalDataStore as needed


From libvirt's perspective, externalDataStore is conceptually pretty

close to a backingStore, but the main difference is its read/write
permissions should match its parent image, rather than being readonly
like backingStore.

This series has only been tested on top of the -blockdev enablement
series, but I don't think it actually interacts with that work at
the moment.


Future work:
 * Exposing this in the runtime XML. We need to figure out an XML


This also belongs in the persistent XML.


   schema. It will reuse virStorageSource obviously, but the main
   thing to figure out is probably 1) what the top element name
   should be ('dataFile' maybe?), 2) where it sits in the XML
   hierarchy (under  or under  I guess)



 maybe?


 * Exposing this on the qemu -blockdev command line. Similar to how
   in the blockdev world we are explicitly putting the disk backing
   chain on the command line, we can do that for data_file too.


Historically, not being explicit on the command line and letting QEMU
do the right thing has bitten us, so yes, we have to do it for data_file
too.


Then
   like persistent  XML the user will have the power
   to overwrite the data_file location for an individual VM run.



If the point of the thin qcow2 layer is to contain the dirty bitmaps for
incremental backup then running this then you might as well use a
different metadata_file? Otherwise the metadata won't match the actual
data.

OTOH, I can imagine throwing away the metadata file and starting over.


 * Figure out how we expect ovirt/rhev to be using this at runtime.
   Possibly taking a running VM using a raw image, doing blockdev-*
   magic to pivot it to qcow2+raw data_file, so it can initiate
   incremental backup on top of a previously raw only VM?


Known issues:
 * In the qemu driver, the qcow2 image metadata is only parsed
   in -blockdev world if no  is specified in the
   persistent XML. So basically if there's a  listed,
   we never parse the qcow2 header and detect the presence of
   data_file. Fixable I'm sure but I didn't look into it much yet.


This will be fixed by introducing an XML element for it.

Jano



Most of this is cleanups and refactorings to simplify the actual
functional changes.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/30] storagefile: qcow1: Fix check for empty backing file

[Michal Privoznik]

On 10/9/19 9:26 PM, Daniel Henrique Barboza wrote:



On 10/7/19 6:49 PM, Cole Robinson wrote:

>From f772b3d91fd the intention of this code seems to be to set


Is this leading '>' a bogus?


That's a feature of git-send-email so that this line is not mistaken for 
e-mail header line "From: Michal Privoznik ". When Cole pushes these 
patches, it won't appear in git log, nor it shows in his local branch.





format=NONE when the image does not have a backing file. However
'buf' here is the whole qcow1 file header. What we want to be
checking is 'res' which is the parsed backing file path.
qcowXGetBackingStore sets this to NULL when there's no backing file.

Signed-off-by: Cole Robinson 
---


This is unusual. Either the qcow file header 'buf' is being set to \0 when
there is no backing file (which would make the current code work even
when checking the wrong thing) or this is a bug that flew under the radar
from a long time.



Since this is for qcow1 I wouldn't be surprised if it is the latter one. 
I mean, does anybody even use qcow1 these days?


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/30] storagefile: qcow1: Check for BACKING_STORE_OK

[Michal Privoznik]

On 10/7/19 11:49 PM, Cole Robinson wrote:

Check explicitly for BACKING_STORE_OK and not its 0 value

Signed-off-by: Cole Robinson 
---
  src/util/virstoragefile.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 51726006e7..1549067c48 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -578,7 +578,7 @@ qcow1GetBackingStore(char **res,
   * used to store backing format */
  *format = VIR_STORAGE_FILE_AUTO;
  ret = qcowXGetBackingStore(res, NULL, buf, buf_size, false);
-if (ret == 0 && *buf == '\0')
+if (ret == BACKING_STORE_OK && *buf == '\0')
  *format = VIR_STORAGE_FILE_NONE;
  return ret;
  }



We can make qcowXGetBackingStore() return the enum type instead of plain 
int. But that can be done in a follow up (trivial) patch. When doing 
that, both qcow1GetBackingStore() and qcow2GetBackingStore() and also 
getBackingStore() callback can use the same tretement then.


// after seeing future patches

Ah, you're removing some functions, but you get the idea.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/30] storagefile, security: qcow2 data_file support

[Michal Privoznik]

On 10/7/19 11:49 PM, Cole Robinson wrote:

This series is the first steps to teaching libvirt about qcow2
data_file support, aka external data files or qcow2 external metadata.

A bit about the feature: it was added in qemu 4.0. It essentially
creates a two part image file: a qcow2 layer that just tracks the
image metadata, and a separate data file which is stores the VM
disk contents. AFAICT the driving use case is to keep a fully coherent
raw disk image on disk, and only use qcow2 as an intermediate metadata
layer when necessary, for things like incremental backup support.

The original qemu patch posting is here:
https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07496.html

For testing, you can create a new qcow2+raw data_file image from an
existing image, like:

 qemu-img convert -O qcow2 \
 -o data_file=NEW.raw,data_file_raw=yes
 EXISTING.raw NEW.qcow2

The goal of this series is to teach libvirt enough about this case
so that we can correctly relabel the data_file on VM startup/shutdown.
The main functional changes are

   * Teach storagefile how to parse out data_file from the qcow2 header
   * Store the raw string as virStorageSource->externalDataStoreRaw
   * Track that as its out virStorageSource in externalDataStore
   * dac/selinux relabel externalDataStore as needed


From libvirt's perspective, externalDataStore is conceptually pretty

close to a backingStore, but the main difference is its read/write
permissions should match its parent image, rather than being readonly
like backingStore.

This series has only been tested on top of the -blockdev enablement
series, but I don't think it actually interacts with that work at
the moment.


Future work:
   * Exposing this in the runtime XML. We need to figure out an XML
 schema. It will reuse virStorageSource obviously, but the main
 thing to figure out is probably 1) what the top element name
 should be ('dataFile' maybe?), 2) where it sits in the XML
 hierarchy (under  or under  I guess)

   * Exposing this on the qemu -blockdev command line. Similar to how
 in the blockdev world we are explicitly putting the disk backing
 chain on the command line, we can do that for data_file too. Then
 like persistent  XML the user will have the power
 to overwrite the data_file location for an individual VM run.

   * Figure out how we expect ovirt/rhev to be using this at runtime.
 Possibly taking a running VM using a raw image, doing blockdev-*
 magic to pivot it to qcow2+raw data_file, so it can initiate
 incremental backup on top of a previously raw only VM?


Known issues:
   * In the qemu driver, the qcow2 image metadata is only parsed
 in -blockdev world if no  is specified in the
 persistent XML. So basically if there's a  listed,
 we never parse the qcow2 header and detect the presence of
 data_file. Fixable I'm sure but I didn't look into it much yet.

Most of this is cleanups and refactorings to simplify the actual
functional changes.

Cole Robinson (30):
   storagefile: Make GetMetadataInternal static
   storagefile: qcow1: Check for BACKING_STORE_OK
   storagefile: qcow1: Fix check for empty backing file
   storagefile: qcow1: Let qcowXGetBackingStore fill in format
   storagefile: Check version to determine if qcow2 or not
   storagefile: Drop now unused isQCow2 argument
   storagefile: Use qcowXGetBackingStore directly
   storagefile: Push 'start' into qcow2GetBackingStoreFormat
   storagefile: Push extension_end calc to qcow2GetBackingStoreFormat
   storagefile: Rename qcow2GetBackingStoreFormat
   storagefile: Rename qcow2GetExtensions 'format' argument
   storagefile: Fix backing format \0 check
   storagefile: Add externalDataStoreRaw member
   storagefile: Parse qcow2 external data file
   storagefile: Fill in meta->externalDataStoreRaw
   storagefile: Don't access backingStoreRaw directly in
 FromBackingRelative
   storagefile: Split out virStorageSourceNewFromChild
   storagefile: Add externalDataStore member
   storagefile: Fill in meta->externalDataStore
   security: dac: Drop !parent handling in SetImageLabelInternal
   security: dac: Add is_toplevel to SetImageLabelInternal
   security: dac: Restore image label for externalDataStore
   security: dac: break out SetImageLabelRelative
   security: dac: Label externalDataStore
   security: selinux: Simplify SetImageLabelInternal
   security: selinux: Drop !parent handling in SetImageLabelInternal
   security: selinux: Add is_toplevel to SetImageLabelInternal
   security: selinux: Restore image label for externalDataStore
   security: selinux: break out SetImageLabelRelative
   security: selinux: Label externalDataStore

  src/libvirt_private.syms|   1 -
  src/security/security_dac.c |  63 +--
  src/security/security_selinux.c |  97 +++
  src/util/virstoragefile.c   | 281 
  src/util/virstoragefile.h   |  11 +-
  5 files changed, 290 

Re: [libvirt] [PATCH 25/30] security: selinux: Simplify SetImageLabelInternal

[Michal Privoznik]

On 10/7/19 11:49 PM, Cole Robinson wrote:

All the SetFileCon calls only differ by the label they pass in.
Rework the conditionals to track what label we need, and use a
single SetFileCon call

Signed-off-by: Cole Robinson 
---
  src/security/security_selinux.c | 31 ++-
  1 file changed, 10 insertions(+), 21 deletions(-)


Huh, I posted the same patch a while ago:

https://www.redhat.com/archives/libvir-list/2019-September/msg01240.html

Great minds think alike :-D

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 18/30] storagefile: Add externalDataStore member

[Michal Privoznik]

On 10/10/19 5:09 PM, Daniel Henrique Barboza wrote:



On 10/7/19 6:49 PM, Cole Robinson wrote:

Add the plumbing to track a externalDataStoreRaw as a virStorageSource

Signed-off-by: Cole Robinson 
---
  src/util/virstoragefile.c | 9 +
  src/util/virstoragefile.h | 3 +++
  2 files changed, 12 insertions(+)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7ae6719dd6..ce669b6e0b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2339,6 +2339,12 @@ virStorageSourceCopy(const virStorageSource *src,
  return NULL;
  }
+    if (src->externalDataStore) {
+    if (!(def->externalDataStore = 
virStorageSourceCopy(src->externalDataStore,

+    true)))
+    return NULL;
+    }
+
  VIR_STEAL_PTR(ret, def);
  return ret;
  }
@@ -2560,6 +2566,9 @@ virStorageSourceClear(virStorageSourcePtr def)
  VIR_FREE(def->timestamps);
  VIR_FREE(def->externalDataStoreRaw);
+    virObjectUnref(def->externalDataStore);
+    def->externalDataStore = NULL;


Is this assign to NULL necessary? virObjectUnref will call VIR_FREE() in
def->externalDataStore if there is no more references to it (which will
assign it to NULL in the end).


It is. Point of virXXXClear() functions is to clear passed struct. That 
is, in theory (I'm not saying it's the case of virStorageSourceDef and 
also I'm not saying it isn't), it should be possible to re-use a 
structure after it's been cleared. But for that to happen, all poitners 
must be reset to NULL regardless of refcounters and stuff.


However, there's a memset() called at the end of this function which 
sets all members of this struct to 0, so in the end I agree that the 
line is redundant, but for a different reason :-D


Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 12/30] storagefile: Fix backing format \0 check

[Michal Privoznik]

On 10/7/19 11:49 PM, Cole Robinson wrote:

From qemu.git docs/interop/qcow2.txt


   == String header extensions ==

   Some header extensions (such as the backing file format name and
   the external data file name) are just a single string. In this case,
   the header extension length is the string length and the string is
   not '\0' terminated. (The header extension padding can make it look
   like a string is '\0' terminated, but neither is padding always
   necessary nor is there a guarantee that zero bytes are used
   for padding.)

So we shouldn't be checking for a \0 byte at the end of the backing
format section. I think in practice there always is a \0 but we
shouldn't depend on that.

Signed-off-by: Cole Robinson 
---
  src/util/virstoragefile.c | 13 +
  1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 8cd576a463..5a4e4b24ae 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -506,13 +506,18 @@ qcow2GetExtensions(const char *buf,
  case QCOW2_HDR_EXTENSION_END:
  goto done;
  
-case QCOW2_HDR_EXTENSION_BACKING_FORMAT:

-if (buf[offset+len] != '\0')
-break;
-*backingFormat = virStorageFileFormatTypeFromString(buf+offset);
+case QCOW2_HDR_EXTENSION_BACKING_FORMAT: {
+VIR_AUTOFREE(char *) tmp = NULL;
+if (VIR_ALLOC_N(tmp, len + 1) < 0)
+return -1;
+memcpy(tmp, buf + offset, len);
+tmp[len] = '\0';
+
+*backingFormat = virStorageFileFormatTypeFromString(tmp);
  if (*backingFormat <= VIR_STORAGE_FILE_NONE)
  return -1;
  }
+}
  


Pre-existing, but there's missing 'break;' here. Since you're touching 
these lines, might be worth putting it here.



  offset += len;
  }



Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] security: apparmor: Allow RO /usr/share/edk2/

[Michal Privoznik]

On 10/9/19 8:24 PM, Cole Robinson wrote:

On Fedora, already whitelisted paths to AAVMF and OVMF binaries
are symlinks to binaries under /usr/share/edk2/. Add that directory
to the RO whitelist so virt-aa-helper-test passes

Signed-off-by: Cole Robinson 
---
I don't know if anyone is actually using apparmor on Fedora, but
I have the libs installed now for testing. I think the better thing
to do would be to adjust virt-aa-helper-test to not touch host
state


Oh yeah, definitely. But since majority of libvirt contributors come 
from distros that don't use AppArmor, it doesn't get as many attention.




  src/security/virt-aa-helper.c | 1 +
  1 file changed, 1 insertion(+)


Reviewed-by: Michal Privoznik 

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/2] tests: Add capabilities for QEMU 4.2.0 on ppc64 and aarch64

[Andrea Bolognani]

On Fri, 2019-10-11 at 10:59 +0200, Bjoern Walk wrote:
> Andrea Bolognani  [2019-10-10, 04:56PM +0200]:
> > This will be useful to test Jirka's changes to how we handle default
> > CPU models on more architectures.
> > 
> > The version posted to the list is heavily snipped, but you can grab
> > the full one with
> > 
> >   $ git fetch https://gitlab.com/abologna/libvirt caps-4.2.0
> > 
> > Andrea Bolognani (2):
> >   tests: Add capabilitie for QEMU 4.2.0 on ppc64
> >   tests: Add capabilitie for QEMU 4.2.0 on aarch64
> 
> Although it actually is a pre-4.2.0 version for QEMU, right? I am
> confused, because for s390x we once where told that we do _NOT_ want
> pre-versions in the capability files? How do we handle those now?

We routinely introduce capabilities for pre-release versions of QEMU,
because we want to implement support for new QEMU features as soon as
possible rather than waiting for the corresponding QEMU release to be
out.

Of course doing so causes churn, because we then have to go back
after the QEMU release has happened and update those capabilities to
reflect the actual release, but the benefits outweight the drawbacks
so we're okay with it.

What we usually avoid is adding capabilities "just because": if
adding the pre-release capabilities doesn't allow us to cover more
useful scenarios in the test suite, then it makes sense to avoid the
churn and introduce the capabilities after the QEMU release is out.

In this specific case, as I mention above Jirka is working on a
series that will exercise a QEMU 4.2.0-only feature, and having the
capabilities for more architectures available will allow him to write
tests that ensure the feature doesn't only work on x86; it just so
happens that it's more convenient for me to generate these files than
it is for him, so that's why these patches are their own series and
not part of his upcoming one.

> At least, please update the commit messages to reflect the actual
> changes.

I'm not sure I understand what you're suggesting I should change.

Either way, the patches have already been pushed, but if you expand
on your concerns I'll certainly keep them in mind next time.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/1] IDE: deprecate ide-drive

[Peter Krempa]

On Thu, Oct 10, 2019 at 14:08:12 -0400, John Snow wrote:
> On 10/10/19 7:54 AM, Peter Krempa wrote:
> > On Thu, Oct 10, 2019 at 13:42:26 +0200, Philippe Mathieu-Daudé wrote:
> >> On 10/10/19 1:26 PM, Peter Krempa wrote:

[...]

> > To be honest, I didn't really review the code nor the documentation.
> > I actually reviewed only the idea itself in the context of integration
> > with libvirt and that's why I didn't go for 'Reviewed-by:'.
> > 
> > The gist of the citation above is that we should stick to well known
> > tags with their well known meanings and I think that considering this a
> > 'review' would be a stretch of the definiton.
> > 
> 
> I wasn't aware that PMM wanted to avoid non-standard tags; I consider
> them to be for human use, but I can change that behavior.
> 
> Peter, I'll change it to an ACK (as suggested by Kevin) is that's OK by you.

Sure! I'll spell it out even for compliance:

ACKed-by: Peter Krempa 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/2] tests: Add capabilities for QEMU 4.2.0 on ppc64 and aarch64

[Bjoern Walk]

Andrea Bolognani  [2019-10-10, 04:56PM +0200]:
> This will be useful to test Jirka's changes to how we handle default
> CPU models on more architectures.
> 
> The version posted to the list is heavily snipped, but you can grab
> the full one with
> 
>   $ git fetch https://gitlab.com/abologna/libvirt caps-4.2.0
> 
> Andrea Bolognani (2):
>   tests: Add capabilitie for QEMU 4.2.0 on ppc64
>   tests: Add capabilitie for QEMU 4.2.0 on aarch64

Although it actually is a pre-4.2.0 version for QEMU, right? I am
confused, because for s390x we once where told that we do _NOT_ want
pre-versions in the capability files? How do we handle those now?

At least, please update the commit messages to reflect the actual
changes.

-- 
IBM Systems
Linux on Z & Virtualization Development
--
IBM Deutschland Research & Development GmbH
Schönaicher Str. 220, 71032 Böblingen
Phone: +49 7031 16 1819
--
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [jenkins-ci PATCH] lcitool: Force LANG=en_US.UTF-8 for the containers

[Martin Kletzander]

On Thu, Oct 10, 2019 at 06:15:35PM +0200, Andrea Bolognani wrote:

On Thu, 2019-10-10 at 17:13 +0200, Fabiano Fidêncio wrote:

As we cannot and should not rely on how the containers were generated,
let's force the container LANG to be en_US.UTF-8 otherwise some
containers (Debian 9, Ubuntu 16, and Ubuntu 18) would simply bail when
dealing with environment variables inherited from Gitlab CI which
contains non-POSIX characteres, such as "Fidêncio".

Unfortunately, there's no standard way to do this accross different


s/accross/across/ if you haven't pushed it yet ;)


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [jenkins-ci PATCH] lcitool: Force LANG=en_US.UTF-8 for the containers

[Martin Kletzander]

On Fri, Oct 11, 2019 at 09:50:18AM +0200, Martin Kletzander wrote:

On Thu, Oct 10, 2019 at 06:15:35PM +0200, Andrea Bolognani wrote:

On Thu, 2019-10-10 at 17:13 +0200, Fabiano Fidêncio wrote:

As we cannot and should not rely on how the containers were generated,
let's force the container LANG to be en_US.UTF-8 otherwise some
containers (Debian 9, Ubuntu 16, and Ubuntu 18) would simply bail when
dealing with environment variables inherited from Gitlab CI which
contains non-POSIX characteres, such as "Fidêncio".

Unfortunately, there's no standard way to do this accross different


s/accross/across/ if you haven't pushed it yet ;)


Of course I'm way too late.  Just disregard this and the previous message from 
me...


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/2] tests: Add capabilities for QEMU 4.2.0 on ppc64 and aarch64

[Andrea Bolognani]

On Fri, 2019-10-11 at 08:51 +0200, Jiri Denemark wrote:
> On Thu, Oct 10, 2019 at 16:56:29 +0200, Andrea Bolognani wrote:
> > This will be useful to test Jirka's changes to how we handle default
> > CPU models on more architectures.
> > 
> > The version posted to the list is heavily snipped, but you can grab
> > the full one with
> > 
> >   $ git fetch https://gitlab.com/abologna/libvirt caps-4.2.0
> > 
> > Andrea Bolognani (2):
> >   tests: Add capabilitie for QEMU 4.2.0 on ppc64
> >   tests: Add capabilitie for QEMU 4.2.0 on aarch64
> > 
> >  .../caps_4.2.0.aarch64.replies| 20684 +
> >  .../caps_4.2.0.aarch64.xml|   321 +
> >  .../caps_4.2.0.ppc64.replies  | 24406 
> >  .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml |  1087 +
> >  4 files changed, 46498 insertions(+)
> >  create mode 100644 tests/qemucapabilitiesdata/caps_4.2.0.aarch64.replies
> >  create mode 100644 tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml
> >  create mode 100644 tests/qemucapabilitiesdata/caps_4.2.0.ppc64.replies
> >  create mode 100644 tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml
> 
> Could you also add corresponding test cases to domaincapstest.c?

Sure, I'll squash that in.

Is there any chance we can convert that test case to automatically
pick up new .replies files, same as qemucapabilitiestest and
qemucaps2xmltest? It's so much more convenient, and reduces the
chance of missing the necessary changes to zero :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/2] tests: Add capabilities for QEMU 4.2.0 on ppc64 and aarch64

[Jiri Denemark]

On Thu, Oct 10, 2019 at 16:56:29 +0200, Andrea Bolognani wrote:
> This will be useful to test Jirka's changes to how we handle default
> CPU models on more architectures.
> 
> The version posted to the list is heavily snipped, but you can grab
> the full one with
> 
>   $ git fetch https://gitlab.com/abologna/libvirt caps-4.2.0
> 
> Andrea Bolognani (2):
>   tests: Add capabilitie for QEMU 4.2.0 on ppc64
>   tests: Add capabilitie for QEMU 4.2.0 on aarch64
> 
>  .../caps_4.2.0.aarch64.replies| 20684 +
>  .../caps_4.2.0.aarch64.xml|   321 +
>  .../caps_4.2.0.ppc64.replies  | 24406 
>  .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml |  1087 +
>  4 files changed, 46498 insertions(+)
>  create mode 100644 tests/qemucapabilitiesdata/caps_4.2.0.aarch64.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_4.2.0.aarch64.xml
>  create mode 100644 tests/qemucapabilitiesdata/caps_4.2.0.ppc64.replies
>  create mode 100644 tests/qemucapabilitiesdata/caps_4.2.0.ppc64.xml

Could you also add corresponding test cases to domaincapstest.c?

Reviewed-by: Jiri Denemark 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list