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?