[libvirt] [PATCH 0/8] qemu: Lock-out checkpoints until backups are ready
See patch 6/8 for the reasoning. This series applies on top of the v2 of splitting checkpoints into a separate file. Peter Krempa (8): qemu: checkpoint: Refactor cleanup in qemuCheckpointCreateXML qemu: checkpoint: Remove open-ended TODOs qemu: Simplify argument list of qemuDomainBlockPullCommon qemu: Don't repeat virDomainObjEndAPI in qemuDomainBlockPull qemu: caps: Add capability for incremental backup support qemu: checkpoint: Forbid creating checkpoints until we support backups qemu: Aggregate interlocking of blockjobs by checkpoints in one place qemu: domain: Base block job interlocking on QEMU_CAPS_INCREMENTAL_BACKUP src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 3 +++ src/qemu/qemu_checkpoint.c | 42 +++++++++++++----------------- src/qemu/qemu_domain.c | 24 +++++++++++++++++ src/qemu/qemu_domain.h | 4 +++ src/qemu/qemu_driver.c | 50 +++++++++++++----------------------- 6 files changed, 70 insertions(+), 56 deletions(-) -- 2.21.0
Use VIR_AUTO* helpers and get rid of the 'cleanup' label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 30d1b6ae3a..4430f69911 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -363,9 +363,9 @@ qemuCheckpointCreateXML(virDomainPtr domain, bool redefine = flags & VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE; unsigned int parse_flags = 0; virDomainMomentObjPtr other = NULL; - virQEMUDriverConfigPtr cfg = NULL; - virCapsPtr caps = NULL; - virJSONValuePtr actions = NULL; + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = NULL; + VIR_AUTOUNREF(virCapsPtr) caps = NULL; + VIR_AUTOPTR(virJSONValue) actions = NULL; int ret; VIR_AUTOUNREF(virDomainCheckpointDefPtr) def = NULL; @@ -380,32 +380,32 @@ qemuCheckpointCreateXML(virDomainPtr domain, if (virDomainSnapshotObjListNum(vm->snapshots, NULL, 0) > 0) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot create checkpoint while snapshot exists")); - goto cleanup; + return NULL; } if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BITMAP_MERGE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("qemu binary lacks persistent bitmaps support")); - goto cleanup; + return NULL; } if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - goto cleanup; + return NULL; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot create checkpoint for inactive domain")); - goto cleanup; + return NULL; } if (!(def = virDomainCheckpointDefParseString(xmlDesc, caps, driver->xmlopt, priv->qemuCaps, parse_flags))) - goto cleanup; + return NULL; /* Unlike snapshots, the RNG schema already ensured a sane filename. */ /* We are going to modify the domain below. */ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; + return NULL; if (redefine) { if (virDomainCheckpointRedefinePrep(vm, &def, &chk, @@ -484,10 +484,6 @@ qemuCheckpointCreateXML(virDomainPtr domain, qemuDomainObjEndJob(driver, vm); - cleanup: - virJSONValueFree(actions); - virObjectUnref(caps); - virObjectUnref(cfg); return checkpoint; } -- 2.21.0
Once somebody is motivated enough to add the support for the quiesce flag or offline checkpoint deletion they are welcome to do so but we don't need to have a reminder. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 4430f69911..59f700e1d3 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -370,7 +370,6 @@ qemuCheckpointCreateXML(virDomainPtr domain, VIR_AUTOUNREF(virDomainCheckpointDefPtr) def = NULL; virCheckFlags(VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE, NULL); - /* TODO: VIR_DOMAIN_CHECKPOINT_CREATE_QUIESCE */ if (redefine) { parse_flags |= VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE; @@ -570,8 +569,6 @@ qemuCheckpointDelete(virDomainObjPtr vm, return -1; if (!metadata_only) { - /* Until qemu-img supports offline bitmap deletion, we are stuck - * with requiring a running guest */ if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot delete checkpoint for inactive domain")); -- 2.21.0
Drop the 'driver' argument since it can be extracted from private data to shorten the argument list. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5852df2a53..71503898e4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17534,14 +17534,14 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, /* bandwidth in MiB/s per public API. Caller must lock vm beforehand, * and not access it afterwards. */ static int -qemuDomainBlockPullCommon(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuDomainBlockPullCommon(virDomainObjPtr vm, const char *path, const char *base, unsigned long bandwidth, unsigned int flags) { qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver; const char *device = NULL; const char *jobname = NULL; virDomainDiskDefPtr disk; @@ -18355,7 +18355,6 @@ static int qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, unsigned long bandwidth, unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; unsigned long long speed = bandwidth; @@ -18384,7 +18383,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, /* For normal rebase (enhanced blockpull), the common code handles * everything, including vm cleanup. */ if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY)) - return qemuDomainBlockPullCommon(driver, vm, path, base, bandwidth, flags); + return qemuDomainBlockPullCommon(vm, path, base, bandwidth, flags); /* If we got here, we are doing a block copy rebase. */ if (!(dest = virStorageSourceNew())) @@ -18540,8 +18539,7 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, return -1; } - return qemuDomainBlockPullCommon(dom->conn->privateData, - vm, path, NULL, bandwidth, flags); + return qemuDomainBlockPullCommon(vm, path, NULL, bandwidth, flags); } -- 2.21.0
Add a 'cleanup' label and use jumps as we do in other places. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 71503898e4..3ad1699eb1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18522,24 +18522,27 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags) { virDomainObjPtr vm; + int ret = -1; + virCheckFlags(VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES, -1); if (!(vm = qemuDomainObjFromDomain(dom))) return -1; - if (virDomainBlockPullEnsureACL(dom->conn, vm->def) < 0) { - virDomainObjEndAPI(&vm); - return -1; - } + if (virDomainBlockPullEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) > 0) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot perform block pull while checkpoint exists")); - virDomainObjEndAPI(&vm); - return -1; + goto cleanup; } - return qemuDomainBlockPullCommon(vm, path, NULL, bandwidth, flags); + ret = qemuDomainBlockPullCommon(vm, path, NULL, bandwidth, flags); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; } -- 2.21.0
On 9/26/19 10:51 AM, Peter Krempa wrote:
Add a 'cleanup' label and use jumps as we do in other places.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
+ cleanup: + virDomainObjEndAPI(&vm); + return ret; }
Is there any way to make virDomainObjEndAPI() replaceable by AUTO framework? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Thu, Sep 26, 2019 at 17:02:49 -0500, Eric Blake wrote:
On 9/26/19 10:51 AM, Peter Krempa wrote:
Add a 'cleanup' label and use jumps as we do in other places.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
+ cleanup: + virDomainObjEndAPI(&vm); + return ret; }
Is there any way to make virDomainObjEndAPI() replaceable by AUTO framework?
It will probably require a new macro or a new type to avoid breaking semantics we've established. VIR_AUTOPTR is unuitable because the semantics say that the object will be freed. VIR_AUTOCLEAN is similarly unsuitable because we don't clean anything inside, but AUTOCLEAN can be used internally in some cases as e.g. with the autmatic XPath context cleanup code. VIR_AUTOUNREF is not enough in this case because we are dealing with a lockable object. We could introduce something like VIR_AUTORELEASE and VIR_AUTOUNLOCK for use with virOjbectLockable-based objects. The former would then do the required virObjectUnlock and then virObjectUnref. The latter is for parity in cases when the lock should be dropped. I'll propose these and see how it will go.
On 9/27/19 1:33 AM, Peter Krempa wrote:
On Thu, Sep 26, 2019 at 17:02:49 -0500, Eric Blake wrote:
On 9/26/19 10:51 AM, Peter Krempa wrote:
Add a 'cleanup' label and use jumps as we do in other places.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
+ cleanup: + virDomainObjEndAPI(&vm); + return ret; }
Is there any way to make virDomainObjEndAPI() replaceable by AUTO framework?
It will probably require a new macro or a new type to avoid breaking semantics we've established.
We could introduce something like VIR_AUTORELEASE and VIR_AUTOUNLOCK for use with virOjbectLockable-based objects. The former would then do the required virObjectUnlock and then virObjectUnref. The latter is for parity in cases when the lock should be dropped.
I'll propose these and see how it will go.
glibc has GMutexLocker which is a way to release a lock on all exit paths after its use (or to release the lock early). https://developer.gnome.org/glib/stable/glib-Threads.html#g-mutex-locker-new You can use that for inspiration. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Sep 27, 2019 at 07:48:55 -0500, Eric Blake wrote:
On 9/27/19 1:33 AM, Peter Krempa wrote:
On Thu, Sep 26, 2019 at 17:02:49 -0500, Eric Blake wrote:
On 9/26/19 10:51 AM, Peter Krempa wrote:
Add a 'cleanup' label and use jumps as we do in other places.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
+ cleanup: + virDomainObjEndAPI(&vm); + return ret; }
Is there any way to make virDomainObjEndAPI() replaceable by AUTO framework?
It will probably require a new macro or a new type to avoid breaking semantics we've established.
We could introduce something like VIR_AUTORELEASE and VIR_AUTOUNLOCK for use with virOjbectLockable-based objects. The former would then do the required virObjectUnlock and then virObjectUnref. The latter is for parity in cases when the lock should be dropped.
I'll propose these and see how it will go.
glibc has GMutexLocker which is a way to release a lock on all exit paths after its use (or to release the lock early). https://developer.gnome.org/glib/stable/glib-Threads.html#g-mutex-locker-new You can use that for inspiration.
I opted to do something similar to VIR_AUTOUNREF in this iteration, but we already use a very similar approach to what you've suggested in VIR_XPATH_NODE_AUTORESTORE so that might also be a way forward.
Add a new all-covering capability which will be used to interlock incremental backup support until all bits are ready. --- src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 3 +++ 2 files changed, 6 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 72e070e8ab..2edbb3fdfe 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -540,6 +540,9 @@ VIR_ENUM_IMPL(virQEMUCaps, "dbus-vmstate", "vhost-user-gpu", "vhost-user-vga", + + /* 340 */ + "incremental-backup", ); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 137b7161a5..5e990ce01e 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -522,6 +522,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_DEVICE_VHOST_USER_GPU, /* -device vhost-user-gpu */ QEMU_CAPS_DEVICE_VHOST_USER_VGA, /* -device vhost-user-vga */ + /* 340 */ + QEMU_CAPS_INCREMENTAL_BACKUP, /* incremental backup is supported */ + QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; -- 2.21.0
Checkpoints by themselves are not very useful for anything else than testing the few bitmap interactions that are currently implemented. It's very unlikely that anybody used this feature and thus we can disable it until we have a more complete implementation ready. Additionally the code for deleting checkpoints has many broken failure scenarios which should be fixed first. This will require support of deleting a bitmap in a qemu 'transaction' which was not released yet. Curious users obviously can use the qemu namespace in the XML to enable this for experiments: <domain type='kvm' xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'> ... <qemu:capabilities> <qemu:add capability='incremental-backup'/> </qemu:capabilities> </domain> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 59f700e1d3..46a10cbeba 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -382,9 +382,9 @@ qemuCheckpointCreateXML(virDomainPtr domain, return NULL; } - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BITMAP_MERGE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("qemu binary lacks persistent bitmaps support")); + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_INCREMENTAL_BACKUP)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("incremental backup is not supported yet")); return NULL; } @@ -569,14 +569,15 @@ qemuCheckpointDelete(virDomainObjPtr vm, return -1; if (!metadata_only) { - if (!virDomainObjIsActive(vm)) { + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_INCREMENTAL_BACKUP)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot delete checkpoint for inactive domain")); + _("incremental backup is not supported yet")); goto endjob; } - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BITMAP_MERGE)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("qemu binary lacks persistent bitmaps support")); + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cannot delete checkpoint for inactive domain")); goto endjob; } } -- 2.21.0
On 9/26/19 10:51 AM, Peter Krempa wrote:
Checkpoints by themselves are not very useful for anything else than testing the few bitmap interactions that are currently implemented.
It's very unlikely that anybody used this feature and thus we can disable it until we have a more complete implementation ready.
Additionally the code for deleting checkpoints has many broken failure scenarios which should be fixed first.
I suspect you are probably right.
This will require support of deleting a bitmap in a qemu 'transaction' which was not released yet.
However, I'm not yet convinced on this point. Deleting a bitmap does not have to be atomic with other actions from the standpoint of correct contents of remaining bitmaps. I _do_ see that an atomic delete makes it easier to delete multiple bitmaps at once, without having to worry about failure states if one delete fails after another succeeded; but it may still be possible to make things work without transactioned delete. On the flip side, waiting to support checkpoints until qemu makes it nice, and not worrying about older qemu, makes for less code to maintain.
Curious users obviously can use the qemu namespace in the XML to enable this for experiments:
<domain type='kvm' xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'> ... <qemu:capabilities> <qemu:add capability='incremental-backup'/> </qemu:capabilities> </domain>
Useful to know, for testing things in the meantime. Does this need to go on any of our published documentation, perhaps in docs/formatcheckpoint.html.in?
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Rather than having to fix 5 places once we support the combination add a function called by all the blockjob/snapshot APIs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 21 +++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_driver.c | 25 +++++-------------------- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 19fa5420e7..e8a82fee0b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -15513,3 +15513,24 @@ qemuDomainDefHasManagedPR(virDomainObjPtr vm) return jobPR; } + + +/** + * qemuDomainSupportsCheckpointsBlockjobs: + * @vm: domain object + * + * Checks whether a block job is supported in possible combination with + * checkpoints (qcow2 bitmaps). Returns -1 if unsupported and reports an error + * 0 in case everything is supported. + */ +int +qemuDomainSupportsCheckpointsBlockjobs(virDomainObjPtr vm) +{ + if (virDomainListCheckpoints(vm->checkpoints, NULL, NULL, NULL, 0) > 0) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cannot perform block operations while checkpoint exists")); + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f45da882a8..01a54d4265 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1217,3 +1217,7 @@ qemuDomainPausedReasonToSuspendedEvent(virDomainPausedReason reason); int qemuDomainValidateActualNetDef(const virDomainNetDef *net, virQEMUCapsPtr qemuCaps); + +int +qemuDomainSupportsCheckpointsBlockjobs(virDomainObjPtr vm) + ATTRIBUTE_RETURN_CHECK; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3ad1699eb1..3847c727cf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15846,11 +15846,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (virDomainSnapshotCreateXMLEnsureACL(domain->conn, vm->def, flags) < 0) goto cleanup; - if (virDomainListCheckpoints(vm->checkpoints, NULL, domain, NULL, 0) > 0) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot create snapshot while checkpoint exists")); + if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) goto cleanup; - } if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; @@ -18374,11 +18371,8 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, if (virDomainBlockRebaseEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) > 0) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot perform block rebase while checkpoint exists")); + if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) goto cleanup; - } /* For normal rebase (enhanced blockpull), the common code handles * everything, including vm cleanup. */ @@ -18464,11 +18458,8 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml, if (virDomainBlockCopyEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) > 0) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot perform block copy while checkpoint exists")); + if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) goto cleanup; - } for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; @@ -18532,11 +18523,8 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, if (virDomainBlockPullEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) > 0) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot perform block pull while checkpoint exists")); + if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) goto cleanup; - } ret = qemuDomainBlockPullCommon(vm, path, NULL, bandwidth, flags); @@ -18591,11 +18579,8 @@ qemuDomainBlockCommit(virDomainPtr dom, if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) > 0) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot perform block commit while checkpoint exists")); + if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) goto cleanup; - } if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; -- 2.21.0
On 9/26/19 10:51 AM, Peter Krempa wrote:
Rather than having to fix 5 places once we support the combination add a
s/ add/, add/
function called by all the blockjob/snapshot APIs.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
The QEMU_CAPS_INCREMENTAL_BACKUP will be enabled once all bits of the incremental backup feature work as expected which means also properly interacting with blockjobs and snapshots. Thus we can allow blockjobs and snapshots if QEMU_CAPS_INCREMENTAL_BACKUP is present even when checkpoints exist. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e8a82fee0b..6d42530db0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -15526,7 +15526,10 @@ qemuDomainDefHasManagedPR(virDomainObjPtr vm) int qemuDomainSupportsCheckpointsBlockjobs(virDomainObjPtr vm) { - if (virDomainListCheckpoints(vm->checkpoints, NULL, NULL, NULL, 0) > 0) { + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_INCREMENTAL_BACKUP) && + virDomainListCheckpoints(vm->checkpoints, NULL, NULL, NULL, 0) > 0) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot perform block operations while checkpoint exists")); return -1; -- 2.21.0
On 9/26/19 10:51 AM, Peter Krempa wrote:
See patch 6/8 for the reasoning.
This series applies on top of the v2 of splitting checkpoints into a separate file.
Peter Krempa (8): qemu: checkpoint: Refactor cleanup in qemuCheckpointCreateXML qemu: checkpoint: Remove open-ended TODOs qemu: Simplify argument list of qemuDomainBlockPullCommon qemu: Don't repeat virDomainObjEndAPI in qemuDomainBlockPull qemu: caps: Add capability for incremental backup support qemu: checkpoint: Forbid creating checkpoints until we support backups qemu: Aggregate interlocking of blockjobs by checkpoints in one place qemu: domain: Base block job interlocking on QEMU_CAPS_INCREMENTAL_BACKUP
Couple of minor comments, but overall the series makes sense (even if it's rough to see a regression in support, I agree with the comment that you can at least override it with custom xml namespace, and we ARE working towards re-enabling things as soon as possible). Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa