[libvirt] [PATCH v2 0/4] qemu: block: implement optional removal of committed snapshot images
v2 fixes a mess-up in handling of strerror_r() return value. 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
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@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
On Wed, Nov 20, 2019 at 11:44:52 +0100, Pavel Mores wrote:
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@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;
I'd prefer if these last two hunks which enable the feature are in a separate commit at the end of the series, so that they enable it only once all the plumbing is in place. The rest looks good.
On Wed, Nov 20, 2019 at 12:14:11PM +0100, Peter Krempa wrote:
On Wed, Nov 20, 2019 at 11:44:52 +0100, Pavel Mores wrote:
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@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;
I'd prefer if these last two hunks which enable the feature are in a separate commit at the end of the series, so that they enable it only once all the plumbing is in place.
I tried to figure out how to do this but I'm afraid I don't see a very good way. Here's my thinking: since code in patch 2 reads and uses qemuBlockJobCommitData.deleteCommittedImages, patch 1 should make sure it's there and it's initialised. So patch 1 should add deleteCommittedImages to qemuBlockJobCommitData and qemuBlockJobDiskNewCommit() should set it to a value. To get that value, it needs to take the extra argument. However, if it does take the extra argument, qemuDomainBlockCommit() has to pass it. So (without resorting to an initialisation to a dummy value), I can't really see a natural way how to split this patch. Also, the actual enabling of the feature doesn't happen anyway until patch 2 where deleteCommittedImages is read and acted upon - until then, patch 1 - even as it is now - is just setting a value that no-one reads. I'm not sure if it's worth the trouble - am I overlooking something?
On Wed, Nov 20, 2019 at 18:12:18 +0100, Pavel Mores wrote:
On Wed, Nov 20, 2019 at 12:14:11PM +0100, Peter Krempa wrote:
On Wed, Nov 20, 2019 at 11:44:52 +0100, Pavel Mores wrote:
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@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;
I'd prefer if these last two hunks which enable the feature are in a separate commit at the end of the series, so that they enable it only once all the plumbing is in place.
I tried to figure out how to do this but I'm afraid I don't see a very good way. Here's my thinking: since code in patch 2 reads and uses qemuBlockJobCommitData.deleteCommittedImages, patch 1 should make sure it's there and it's initialised. So patch 1 should add deleteCommittedImages to qemuBlockJobCommitData and qemuBlockJobDiskNewCommit() should set it to a value. To get that value, it needs to take the extra argument. However, if it does take the extra argument, qemuDomainBlockCommit() has to pass it.
Just pass 'false' as the last argument of qemuBlockJobDiskNewCommit which will equal to the same behaviour as it did until now. Then pass the correct value along with enabling it in virCheckFlags after the code which actually uses the value is in place.
So (without resorting to an initialisation to a dummy value), I can't really see a natural way how to split this patch. Also, the actual enabling of the feature doesn't happen anyway until patch 2 where deleteCommittedImages is read and acted upon - until then, patch 1 - even as it is now - is just
Well, the problem is that after the first patch the virCheckFlags macro will no longer reject VIR_DOMAIN_BLOCK_COMMIT_DELETE despite the code doing nothing (as in nothing besides storing the value) with it until later patches.
setting a value that no-one reads. I'm not sure if it's worth the trouble - am I overlooking something?
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Nov 21, 2019 at 08:38:13AM +0100, Peter Krempa wrote:
On Wed, Nov 20, 2019 at 18:12:18 +0100, Pavel Mores wrote:
On Wed, Nov 20, 2019 at 12:14:11PM +0100, Peter Krempa wrote:
On Wed, Nov 20, 2019 at 11:44:52 +0100, Pavel Mores wrote:
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@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;
I'd prefer if these last two hunks which enable the feature are in a separate commit at the end of the series, so that they enable it only once all the plumbing is in place.
I tried to figure out how to do this but I'm afraid I don't see a very good way. Here's my thinking: since code in patch 2 reads and uses qemuBlockJobCommitData.deleteCommittedImages, patch 1 should make sure it's there and it's initialised. So patch 1 should add deleteCommittedImages to qemuBlockJobCommitData and qemuBlockJobDiskNewCommit() should set it to a value. To get that value, it needs to take the extra argument. However, if it does take the extra argument, qemuDomainBlockCommit() has to pass it.
Just pass 'false' as the last argument of qemuBlockJobDiskNewCommit which will equal to the same behaviour as it did until now.
Then pass the correct value along with enabling it in virCheckFlags after the code which actually uses the value is in place.
So (without resorting to an initialisation to a dummy value), I can't really see a natural way how to split this patch. Also, the actual enabling of the feature doesn't happen anyway until patch 2 where deleteCommittedImages is read and acted upon - until then, patch 1 - even as it is now - is just
Well, the problem is that after the first patch the virCheckFlags macro will no longer reject VIR_DOMAIN_BLOCK_COMMIT_DELETE despite the code doing nothing (as in nothing besides storing the value) with it until later patches.
Ah okay then, makes sense.
setting a value that no-one reads. I'm not sure if it's worth the trouble - am I overlooking something?
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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..cf221a2839 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_buf[errmsg_len]; + char *errmsg; + + for (; p != NULL; p = p->backingStore) { + if (virStorageSourceIsLocalStorage(p)) + if (unlink(p->path) < 0) { + errmsg = strerror_r(errno, errmsg_buf, errmsg_len); + 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
On Wed, Nov 20, 2019 at 11:44:53 +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@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..cf221a2839 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_buf[errmsg_len]; + char *errmsg; + + for (; p != NULL; p = p->backingStore) { + if (virStorageSourceIsLocalStorage(p))
The above condition has a multiline body, so it must be enclosed in a block.
+ if (unlink(p->path) < 0) {
I was considering whether we should use virFileRemove which will also work properly on root-squashed NFS. I'm not sure though how easy it will be to figure out the correct uid and gid inside this helper. If you are interrested into investigating if it's possible, there should be some prior art in the qemu driver where we try to get the uid/gid frm virStorageSource if it's configured, then fall back to the domain uid/gid of the process.
+ errmsg = strerror_r(errno, errmsg_buf, errmsg_len);
Please use g_strerror(); It does not require any of the buffers and stuff.
+ VIR_WARN("Unable to remove snapshot image file %s (%s)", + p->path, errmsg); + } + }
The rest looks good.
On Wed, Nov 20, 2019 at 12:11:32PM +0100, Peter Krempa wrote:
On Wed, Nov 20, 2019 at 11:44:53 +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@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..cf221a2839 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_buf[errmsg_len]; + char *errmsg; + + for (; p != NULL; p = p->backingStore) { + if (virStorageSourceIsLocalStorage(p))
The above condition has a multiline body, so it must be enclosed in a block.
+ if (unlink(p->path) < 0) {
I was considering whether we should use virFileRemove which will also work properly on root-squashed NFS. I'm not sure though how easy it will be to figure out the correct uid and gid inside this helper.
If you are interrested into investigating if it's possible, there should be some prior art in the qemu driver where we try to get the uid/gid frm virStorageSource if it's configured, then fall back to the domain uid/gid of the process.
I can see qemuDomainGetImageIds() in qemu_domain.c which seems somewhat close to what you describe. It tries to get uid/gid from the virStorageSourcePtr itself, then its parent virStorageSourcePtr, then virDomainObjPtr and finally virQEMUDriverConfigPtr. It's static in its file though, and I don't immediately see an easy way to pass it the virQEMUDriverConfigPtr. However, if it's OK to make it visible from blockdev code, I could still call it along the lines of qemuDomainGetImageIds(NULL, vm, p, NULL, &uid, &gid); as I can apparently get a virDomainObjPtr into the helper quite easily.
+ errmsg = strerror_r(errno, errmsg_buf, errmsg_len);
Please use g_strerror(); It does not require any of the buffers and stuff.
+ VIR_WARN("Unable to remove snapshot image file %s (%s)", + p->path, errmsg); + } + }
The rest looks good.
On Wed, Nov 20, 2019 at 14:56:18 +0100, Pavel Mores wrote:
On Wed, Nov 20, 2019 at 12:11:32PM +0100, Peter Krempa wrote:
On Wed, Nov 20, 2019 at 11:44:53 +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@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..cf221a2839 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_buf[errmsg_len]; + char *errmsg; + + for (; p != NULL; p = p->backingStore) { + if (virStorageSourceIsLocalStorage(p))
The above condition has a multiline body, so it must be enclosed in a block.
+ if (unlink(p->path) < 0) {
I was considering whether we should use virFileRemove which will also work properly on root-squashed NFS. I'm not sure though how easy it will be to figure out the correct uid and gid inside this helper.
If you are interrested into investigating if it's possible, there should be some prior art in the qemu driver where we try to get the uid/gid frm virStorageSource if it's configured, then fall back to the domain uid/gid of the process.
I can see qemuDomainGetImageIds() in qemu_domain.c which seems somewhat close
yes, this is the one I meant! I couldn't remember :D
to what you describe. It tries to get uid/gid from the virStorageSourcePtr itself, then its parent virStorageSourcePtr, then virDomainObjPtr and finally virQEMUDriverConfigPtr.
That is the correct hierarchy. An image file can either have it's own security label, or the security label of the disk applies to all of the chain members, if the disk doesn't have a label there is a default label for the whole VM and in case that isn't available in case of the DAC driver we apply the configured uid/gid.
It's static in its file though, and I don't immediately see an easy way to pass it the virQEMUDriverConfigPtr. However, if it's OK to make it visible from
I think it's okay to export it.
blockdev code, I could still call it along the lines of
You can get the config via: virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); and driver via: virQEMUDriverPtr driver = priv->driver; and finally: qemuDomainObjPrivatePtr priv = vm->privateData; Since qemuDomainGetImageIds already takes vm, I'd suggest you refactor it or add a wrapper which will not require passing of 'cfg', but will acquire it via the above mentioned method from vm, so that we don't have to do it in the blockjob code.
qemuDomainGetImageIds(NULL, vm, p, NULL, &uid, &gid);
as I can apparently get a virDomainObjPtr into the helper quite easily.
+ errmsg = strerror_r(errno, errmsg_buf, errmsg_len);
Please use g_strerror(); It does not require any of the buffers and stuff.
+ VIR_WARN("Unable to remove snapshot image file %s (%s)", + p->path, errmsg); + } + }
The rest looks good.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Nov 20, 2019 at 12:11:32PM +0100, Peter Krempa wrote:
On Wed, Nov 20, 2019 at 11:44:53 +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@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..cf221a2839 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_buf[errmsg_len]; + char *errmsg; + + for (; p != NULL; p = p->backingStore) { + if (virStorageSourceIsLocalStorage(p))
The above condition has a multiline body, so it must be enclosed in a block.
+ if (unlink(p->path) < 0) {
I was considering whether we should use virFileRemove which will also work properly on root-squashed NFS. I'm not sure though how easy it will be to figure out the correct uid and gid inside this helper.
If you are interrested into investigating if it's possible, there should be some prior art in the qemu driver where we try to get the uid/gid frm virStorageSource if it's configured, then fall back to the domain uid/gid of the process.
+ errmsg = strerror_r(errno, errmsg_buf, errmsg_len);
Please use g_strerror(); It does not require any of the buffers and stuff.
+ VIR_WARN("Unable to remove snapshot image file %s (%s)", + p->path, errmsg); + } + }
The rest looks good.
One more thing comes to my mind though - is VIR_WARN() enough as far as reporting the error goes? Would it make sense to replace it with e.g. virReportError()?
On Thu, Nov 21, 2019 at 09:31:41 +0100, Pavel Mores wrote:
On Wed, Nov 20, 2019 at 12:11:32PM +0100, Peter Krempa wrote:
On Wed, Nov 20, 2019 at 11:44:53 +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@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..cf221a2839 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_buf[errmsg_len]; + char *errmsg; + + for (; p != NULL; p = p->backingStore) { + if (virStorageSourceIsLocalStorage(p))
The above condition has a multiline body, so it must be enclosed in a block.
+ if (unlink(p->path) < 0) {
I was considering whether we should use virFileRemove which will also work properly on root-squashed NFS. I'm not sure though how easy it will be to figure out the correct uid and gid inside this helper.
If you are interrested into investigating if it's possible, there should be some prior art in the qemu driver where we try to get the uid/gid frm virStorageSource if it's configured, then fall back to the domain uid/gid of the process.
+ errmsg = strerror_r(errno, errmsg_buf, errmsg_len);
Please use g_strerror(); It does not require any of the buffers and stuff.
+ VIR_WARN("Unable to remove snapshot image file %s (%s)", + p->path, errmsg); + } + }
The rest looks good.
One more thing comes to my mind though - is VIR_WARN() enough as far as reporting the error goes? Would it make sense to replace it with e.g. virReportError()?
Usually we'd want a virReportError. This case is special. The block job handlers are executed from the qemu event handler thread which is not connected to any API which executes. That means there is nobody to report the error to. Additionally the error of deleting the file itself is only minor. The block job still completed successfully. Even if we could propagate the error to the user it would make the user think the job failed, but in fact the data was already copied and we can't undo that. You can see that we also don't report errors from unplugging of the file itself. There isn't anything we can do about that if it fails.
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@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
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@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
On Wed, Nov 20, 2019 at 11:44:55 +0100, Pavel Mores wrote:
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@redhat.com> --- tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml | 1 + 1 file changed, 1 insertion(+)
Squash this into the previous commit since it's small enough. We usually split test changes only when they are too complex.
participants (2)
-
Pavel Mores -
Peter Krempa