3:55 a.m.
Adds handling of the VIR_DOMAIN_BLOCK_COMMIT_DELETE flag to the qemu driver
which previously just ignored it. This should fix a problem with the virsh
blockcommit command where --delete did nothing.
https://bugzilla.redhat.com/show_bug.cgi?id=1008350
Pavel Mores (4):
qemu: block: propagate the delete flag to where it can actually be
used
qemu: block: use the delete flag to delete snapshot images if
requested
qemu: block: store the delete flag in libvirtd's status XML
qemu: block: add unit test of storing the delete flag in status XML
src/qemu/qemu_blockjob.c | 35 ++++++++++++++++++-
src/qemu/qemu_blockjob.h | 4 ++-
src/qemu/qemu_domain.c | 4 +++
src/qemu/qemu_driver.c | 5 +--
.../blockjob-blockdev-in.xml | 1 +
5 files changed, 45 insertions(+), 4 deletions(-)
--
2.21.0
3:55 a.m.
New subject: [libvirt] [PATCH 1/4] qemu: block: propagate the delete flag to where it can actually be used
Since the blockcommit operation is asynchronous, this has conceptually two
parts. First, we have to propagate the flag from qemuDomainBlockCommit()
(which was just ignoring it until now) to qemuBlockJobDiskNewCommit(). Then
it can be stored in the qemuBlockJobCommitData structure which holds
information necessary to finish the job asynchronously.
Signed-off-by: Pavel Mores <pmores(a)redhat.com>
---
src/qemu/qemu_blockjob.c | 4 +++-
src/qemu/qemu_blockjob.h | 4 +++-
src/qemu/qemu_driver.c | 5 +++--
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 5c294f8024..7d94a6ce38 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -250,7 +250,8 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm,
virDomainDiskDefPtr disk,
virStorageSourcePtr topparent,
virStorageSourcePtr top,
- virStorageSourcePtr base)
+ virStorageSourcePtr base,
+ bool delete_imgs)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
g_autoptr(qemuBlockJobData) job = NULL;
@@ -273,6 +274,7 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm,
job->data.commit.topparent = topparent;
job->data.commit.top = top;
job->data.commit.base = base;
+ job->data.commit.deleteCommittedImages = delete_imgs;
if (qemuBlockJobRegister(job, vm, disk, true) < 0)
return NULL;
diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h
index d8da918f2f..d77f1dcdb3 100644
--- a/src/qemu/qemu_blockjob.h
+++ b/src/qemu/qemu_blockjob.h
@@ -85,6 +85,7 @@ struct _qemuBlockJobCommitData {
virStorageSourcePtr topparent;
virStorageSourcePtr top;
virStorageSourcePtr base;
+ bool deleteCommittedImages;
};
@@ -165,7 +166,8 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm,
virDomainDiskDefPtr disk,
virStorageSourcePtr topparent,
virStorageSourcePtr top,
- virStorageSourcePtr base);
+ virStorageSourcePtr base,
+ bool delete_imgs);
qemuBlockJobDataPtr
qemuBlockJobNewCreate(virDomainObjPtr vm,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dc14ec86a3..75458f5c8a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18496,10 +18496,10 @@ qemuDomainBlockCommit(virDomainPtr dom,
bool persistjob = false;
bool blockdev = false;
- /* XXX Add support for COMMIT_DELETE */
virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW |
VIR_DOMAIN_BLOCK_COMMIT_ACTIVE |
VIR_DOMAIN_BLOCK_COMMIT_RELATIVE |
+ VIR_DOMAIN_BLOCK_COMMIT_DELETE |
VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1);
if (!(vm = qemuDomainObjFromDomain(dom)))
@@ -18638,7 +18638,8 @@ qemuDomainBlockCommit(virDomainPtr dom,
goto endjob;
if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource,
- baseSource)))
+ baseSource,
+ flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE)))
goto endjob;
disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
--
2.21.0
3:55 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: block: use the delete flag to delete snapshot images if requested
When blockcommit finishes successfully, one of the
qemuBlockJobProcessEventCompletedCommit() and
qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called.
This is where the delete flag (stored in qemuBlockJobCommitData since the
previous commit) can actually be used to delete the committed snapshot
images if requested.
Signed-off-by: Pavel Mores <pmores(a)redhat.com>
---
src/qemu/qemu_blockjob.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 7d94a6ce38..dae1fa44c8 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -965,6 +965,31 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver,
}
+/**
+ * Helper for qemuBlockJobProcessEventCompletedCommit() and
+ * qemuBlockJobProcessEventCompletedActiveCommit(). Relies on adjustments
+ * these functions perform on the 'backingStore' chain to function correctly.
+ *
+ * TODO look into removing backing store for non-local snapshots too
+ */
+static void
+qemuBlockJobUnlinkCommittedStorage(virStorageSourcePtr top)
+{
+ virStorageSourcePtr p = top;
+ const size_t errmsg_len = 128;
+ char errmsg[errmsg_len];
+
+ for (; p != NULL; p = p->backingStore) {
+ if (virStorageSourceIsLocalStorage(p))
+ if (unlink(p->path) < 0) {
+ char *dummy = strerror_r(errno, errmsg, errmsg_len);
+ (void)dummy;
+ VIR_WARN("Unable to remove snapshot image file %s (%s)",
+ p->path, errmsg);
+ }
+ }
+}
+
/**
* qemuBlockJobProcessEventCompletedCommit:
* @driver: qemu driver object
@@ -1031,6 +1056,9 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriverPtr driver,
job->data.commit.topparent->backingStore = job->data.commit.base;
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob,
job->data.commit.top);
+
+ if (job->data.commit.deleteCommittedImages)
+ qemuBlockJobUnlinkCommittedStorage(job->data.commit.top);
virObjectUnref(job->data.commit.top);
job->data.commit.top = NULL;
@@ -1121,6 +1149,9 @@ qemuBlockJobProcessEventCompletedActiveCommit(virQEMUDriverPtr
driver,
job->disk->src->readonly = job->data.commit.top->readonly;
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob,
job->data.commit.top);
+
+ if (job->data.commit.deleteCommittedImages)
+ qemuBlockJobUnlinkCommittedStorage(job->data.commit.top);
virObjectUnref(job->data.commit.top);
job->data.commit.top = NULL;
/* the mirror element does not serve functional purpose for the commit job */
--
2.21.0
4:12 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: block: use the delete flag to delete snapshot images if requested
On Wed, Nov 20, 2019 at 10:55:25AM +0100, Pavel Mores wrote:
When blockcommit finishes successfully, one of the
qemuBlockJobProcessEventCompletedCommit() and
qemuBlockJobProcessEventCompletedActiveCommit() event handlers is called.
This is where the delete flag (stored in qemuBlockJobCommitData since the
previous commit) can actually be used to delete the committed snapshot
images if requested.
Signed-off-by: Pavel Mores <pmores(a)redhat.com>
---
src/qemu/qemu_blockjob.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 7d94a6ce38..dae1fa44c8 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -965,6 +965,31 @@ qemuBlockJobProcessEventCompletedPull(virQEMUDriverPtr driver,
}
+/**
+ * Helper for qemuBlockJobProcessEventCompletedCommit() and
+ * qemuBlockJobProcessEventCompletedActiveCommit(). Relies on adjustments
+ * these functions perform on the 'backingStore' chain to function correctly.
+ *
+ * TODO look into removing backing store for non-local snapshots too
+ */
+static void
+qemuBlockJobUnlinkCommittedStorage(virStorageSourcePtr top)
+{
+ virStorageSourcePtr p = top;
+ const size_t errmsg_len = 128;
+ char errmsg[errmsg_len];
+
+ for (; p != NULL; p = p->backingStore) {
+ if (virStorageSourceIsLocalStorage(p))
+ if (unlink(p->path) < 0) {
+ char *dummy = strerror_r(errno, errmsg, errmsg_len);
+ (void)dummy;
Sorry, just realised that a non-standard version of strerror_r() is used here
where the return value is actually useful. I'll fix this is in v2.
+ VIR_WARN("Unable to remove snapshot image file
%s (%s)",
+ p->path, errmsg);
+ }
+ }
+}
+
/**
* qemuBlockJobProcessEventCompletedCommit:
* @driver: qemu driver object
@@ -1031,6 +1056,9 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriverPtr driver,
job->data.commit.topparent->backingStore = job->data.commit.base;
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob,
job->data.commit.top);
+
+ if (job->data.commit.deleteCommittedImages)
+ qemuBlockJobUnlinkCommittedStorage(job->data.commit.top);
virObjectUnref(job->data.commit.top);
job->data.commit.top = NULL;
@@ -1121,6 +1149,9 @@ qemuBlockJobProcessEventCompletedActiveCommit(virQEMUDriverPtr
driver,
job->disk->src->readonly = job->data.commit.top->readonly;
qemuBlockJobEventProcessConcludedRemoveChain(driver, vm, asyncJob,
job->data.commit.top);
+
+ if (job->data.commit.deleteCommittedImages)
+ qemuBlockJobUnlinkCommittedStorage(job->data.commit.top);
virObjectUnref(job->data.commit.top);
job->data.commit.top = NULL;
/* the mirror element does not serve functional purpose for the commit job */
--
2.21.0
3:55 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: block: store the delete flag in libvirtd's status XML
Since blockcommit is asynchronous, libvirtd can be restarted while the
operation runs. To ensure the information necessary to finish up the job
is not lost, serialisation to and deserialisation from the status XML is
added.
Signed-off-by: Pavel Mores <pmores(a)redhat.com>
---
src/qemu/qemu_domain.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 262b74d1ab..dba287973c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2512,6 +2512,8 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void *payload,
virBufferAsprintf(&childBuf, "<top
node='%s'/>\n", job->data.commit.top->nodeformat);
if (job->data.commit.topparent)
virBufferAsprintf(&childBuf, "<topparent
node='%s'/>\n", job->data.commit.topparent->nodeformat);
+ if (job->data.commit.deleteCommittedImages)
+ virBufferAddLit(&childBuf,
"<deleteCommittedImages/>\n");
break;
case QEMU_BLOCKJOB_TYPE_CREATE:
@@ -3069,6 +3071,8 @@ qemuDomainObjPrivateXMLParseBlockjobDataSpecific(qemuBlockJobDataPtr
job,
"string(./base/@node)",
&job->data.commit.base,
ctxt);
+ if (virXPathNode("./deleteCommittedImages", ctxt))
+ job->data.commit.deleteCommittedImages = true;
if (!job->data.commit.top ||
!job->data.commit.base)
goto broken;
--
2.21.0
3:55 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: block: add unit test of storing the delete flag in status XML
The new element was only added to the active commit test, the non-active
commit test doesn't have the new element so as to test its absence.
Signed-off-by: Pavel Mores <pmores(a)redhat.com>
---
tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
index 4f6930001e..5fd2ed1d78 100644
--- a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
+++ b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml
@@ -242,6 +242,7 @@
<disk dst='vde'/>
<base node='libvirt-19-format'/>
<top node='libvirt-17-format'/>
+ <deleteCommittedImages/>
</blockjob>
<blockjob name='create-libvirt-1337-storage' type='create'
state='running'>
<create mode='storage'/>
--
2.21.0
1865
days inactive
1865
days old
5 comments
1 participants
participants (1)
-
Pavel Mores