The qemu implementation requires that the VM associated with the checkpoint is running when checking the size. Mention this possibility with the flag. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/libvirt-domain-checkpoint.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libvirt-domain-checkpoint.c b/src/libvirt-domain-checkpoint.c index 8a7b55dcd2..e9af8e4cf0 100644 --- a/src/libvirt-domain-checkpoint.c +++ b/src/libvirt-domain-checkpoint.c @@ -192,7 +192,9 @@ virDomainCheckpointCreateXML(virDomainPtr domain, * attribute that shows an estimate of the current size in bytes that * have been dirtied between the time the checkpoint was created and the * current point in time. Note that updating the size may be expensive and - * data will be inaccurate once guest OS writes to the disk. + * data will be inaccurate once guest OS writes to the disk. Also note that + * hypervisors may require that the domain associated with @checkpoint is + * running when VIR_DOMAIN_CHECKPOINT_XML_SIZE is used. * * Returns a 0 terminated UTF-8 encoded XML instance or NULL in case * of error. The caller must free() the returned value. -- 2.26.2

Separate the docs for the '--size' flag into it's own paragraph and add a mention that the domain may be required to be running. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- docs/manpages/virsh.rst | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index d34a1c8684..40b7dce093 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -7307,11 +7307,15 @@ checkpoint-dumpxml Output the checkpoint XML for the domain's checkpoint named *checkpoint*. Using *--security-info* will also include security sensitive information. + Using *--size* will add XML indicating the current size in bytes of guest data that has changed since the checkpoint was created (although remember that guest activity between a size check and actually creating a backup can result in the backup needing slightly more -space). Using *--no-domain* will omit the <domain> element from the +space). Note that some hypervisors may require that *domain* is running when +*--size* is used. + +Using *--no-domain* will omit the <domain> element from the output for a more compact view. -- 2.26.2

This code will be used to signal cases when the checkpoint is broken either during backup or other operations where a user might want to make decision based on the presence of the checkpoint, such as do a full backup instead of an incremental one. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- include/libvirt/virterror.h | 1 + src/util/virerror.c | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h index 0f1c32283d..b96fe250aa 100644 --- a/include/libvirt/virterror.h +++ b/include/libvirt/virterror.h @@ -332,6 +332,7 @@ typedef enum { VIR_ERR_NETWORK_PORT_EXIST = 106, /* the network port already exist */ VIR_ERR_NO_NETWORK_PORT = 107, /* network port not found */ VIR_ERR_NO_HOSTNAME = 108, /* no domain's hostname found */ + VIR_ERR_CHECKPOINT_INCONSISTENT = 109, /* checkpoint can't be used */ # ifdef VIR_ENUM_SENTINELS VIR_ERR_NUMBER_LAST diff --git a/src/util/virerror.c b/src/util/virerror.c index 80a7cfe0ed..9e3bad97eb 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1227,6 +1227,10 @@ static const virErrorMsgTuple virErrorMsgStrings[] = { [VIR_ERR_NO_HOSTNAME] = { N_("no hostname found"), N_("no hostname found: %s") }, + [VIR_ERR_CHECKPOINT_INCONSISTENT] = { + N_("checkpoint inconsistent"), + N_("checkpoint inconsistent: %s") + }, }; G_STATIC_ASSERT(G_N_ELEMENTS(virErrorMsgStrings) == VIR_ERR_NUMBER_LAST); -- 2.26.2

If we don't have a consistent chain of bitmaps for the backup to proceed we'd report VIR_ERR_INVALID_ARG error code, which makes it hard to decide whether an incremental backup makes even sense. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_backup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 5376d9485d..d0c852cf80 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -206,7 +206,7 @@ qemuBackupDiskPrepareOneBitmaps(struct qemuBackupDiskData *dd, if (!qemuBlockBitmapChainIsValid(dd->domdisk->src, dd->backupdisk->incremental, blockNamedNodeData)) { - virReportError(VIR_ERR_INVALID_ARG, + virReportError(VIR_ERR_CHECKPOINT_INCONSISTENT, _("missing or broken bitmap '%s' for disk '%s'"), dd->backupdisk->incremental, dd->domdisk->dst); return -1; -- 2.26.2

Introduce a flag which will allow users to perform hypervisor-specific valdiation when redefining the checkpoint metadata. This will allow to check metadata which is stored e.g. in disk images when populating the libvirt metadata. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- include/libvirt/libvirt-domain-checkpoint.h | 2 ++ src/libvirt-domain-checkpoint.c | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/include/libvirt/libvirt-domain-checkpoint.h b/include/libvirt/libvirt-domain-checkpoint.h index f4bd92e81c..58932c8a6a 100644 --- a/include/libvirt/libvirt-domain-checkpoint.h +++ b/include/libvirt/libvirt-domain-checkpoint.h @@ -57,6 +57,8 @@ typedef enum { quiesce all mounted file systems within the domain */ + VIR_DOMAIN_CHECKPOINT_REDEFINE_VALIDATE = (1 << 2), /* validate disk data state + when redefining a checkpoint */ } virDomainCheckpointCreateFlags; /* Create a checkpoint using the current VM state. */ diff --git a/src/libvirt-domain-checkpoint.c b/src/libvirt-domain-checkpoint.c index e9af8e4cf0..e0c2527ccb 100644 --- a/src/libvirt-domain-checkpoint.c +++ b/src/libvirt-domain-checkpoint.c @@ -125,6 +125,11 @@ virDomainCheckpointGetConnect(virDomainCheckpointPtr checkpoint) * has a way to resupply correct defaults). Not all hypervisors support * this flag. * + * If @flags includes VIR_DOMAIN_CHECKPOINT_REDEFINE_VALIDATE along with + * VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE the state of the metadata related + * to the disk state of the redefined checkpoint is validated. Note that + * hypervisors may require that the @domain is running to perform validation. + * * If @flags includes VIR_DOMAIN_CHECKPOINT_CREATE_QUIESCE, then the * libvirt will attempt to use guest agent to freeze and thaw all file * systems in use within domain OS. However, if the guest agent is not @@ -155,6 +160,10 @@ virDomainCheckpointCreateXML(virDomainPtr domain, VIR_DOMAIN_CHECKPOINT_CREATE_QUIESCE, error); + VIR_REQUIRE_FLAG_GOTO(VIR_DOMAIN_CHECKPOINT_REDEFINE_VALIDATE, + VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE, + error); + if (conn->driver->domainCheckpointCreateXML) { virDomainCheckpointPtr ret; ret = conn->driver->domainCheckpointCreateXML(domain, xmlDesc, flags); -- 2.26.2

Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- docs/manpages/virsh.rst | 7 ++++++- tools/virsh-checkpoint.c | 8 ++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 40b7dce093..6fc17e38cd 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -7141,7 +7141,7 @@ checkpoint-create :: - checkpoint-create domain [xmlfile] { --redefine | [--quiesce]} + checkpoint-create domain [xmlfile] { --redefine [--redefine-validate] | [--quiesce]} Create a checkpoint for domain *domain* with the properties specified in *xmlfile* describing a <domaincheckpoint> top-level element. The @@ -7159,6 +7159,11 @@ alterations in the checkpoint metadata (such as host-specific aspects of the domain XML embedded in the checkpoint). When this flag is supplied, the *xmlfile* argument is mandatory. +If *--redefine-validate* is specified along with *--redefine* the hypervisor +performs validation of metadata associated with the checkpoint stored in places +besides the checkpoint XML. Note that some hypervisors may require that the +domain is running to perform validation. + If *--quiesce* is specified, libvirt will try to use guest agent to freeze and unfreeze domain's mounted file systems. However, if domain has no guest agent, checkpoint creation will fail. diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c index ac9d5bd348..cc2bbdae8a 100644 --- a/tools/virsh-checkpoint.c +++ b/tools/virsh-checkpoint.c @@ -99,6 +99,10 @@ static const vshCmdOptDef opts_checkpoint_create[] = { .type = VSH_OT_BOOL, .help = N_("redefine metadata for existing checkpoint") }, + {.name = "redefine-validate", + .type = VSH_OT_BOOL, + .help = N_("validate the redefined checkpoint") + }, {.name = "quiesce", .type = VSH_OT_BOOL, .help = N_("quiesce guest's file systems") @@ -116,8 +120,12 @@ cmdCheckpointCreate(vshControl *ctl, char *buffer = NULL; unsigned int flags = 0; + VSH_REQUIRE_OPTION("redefine-validate", "redefine"); + if (vshCommandOptBool(cmd, "redefine")) flags |= VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE; + if (vshCommandOptBool(cmd, "redefine-validate")) + flags |= VIR_DOMAIN_CHECKPOINT_REDEFINE_VALIDATE; if (vshCommandOptBool(cmd, "quiesce")) flags |= VIR_DOMAIN_CHECKPOINT_CREATE_QUIESCE; -- 2.26.2

First one prepares and validates the definition, the second one actually either updates an existing checkpoint or assigns definition for the new one. This will allow driver code to add extra validation between those steps. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/checkpoint_conf.c | 30 +++++++++++++++++++++--------- src/conf/checkpoint_conf.h | 14 +++++++++----- src/libvirt_private.syms | 1 + src/qemu/qemu_checkpoint.c | 12 ++---------- src/test/test_driver.c | 9 ++++----- 5 files changed, 37 insertions(+), 29 deletions(-) diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index e175917eae..a8d18928de 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -513,16 +513,11 @@ virDomainCheckpointDefFormat(virDomainCheckpointDefPtr def, int virDomainCheckpointRedefinePrep(virDomainObjPtr vm, - virDomainCheckpointDefPtr *defptr, - virDomainMomentObjPtr *chk, - virDomainXMLOptionPtr xmlopt, + virDomainCheckpointDefPtr def, bool *update_current) { - virDomainCheckpointDefPtr def = *defptr; char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainMomentObjPtr parent = NULL; - virDomainMomentObjPtr other = NULL; - virDomainCheckpointDefPtr otherdef = NULL; virUUIDFormat(vm->def->uuid, uuidstr); @@ -550,12 +545,26 @@ virDomainCheckpointRedefinePrep(virDomainObjPtr vm, if (virDomainCheckpointGetCurrent(vm->checkpoints) == NULL) *update_current = true; + return 0; +} + + +virDomainMomentObjPtr +virDomainCheckpointRedefineCommit(virDomainObjPtr vm, + virDomainCheckpointDefPtr *defptr, + virDomainXMLOptionPtr xmlopt) +{ + virDomainCheckpointDefPtr def = *defptr; + virDomainMomentObjPtr other = NULL; + virDomainCheckpointDefPtr otherdef = NULL; + virDomainMomentObjPtr chk = NULL; + other = virDomainCheckpointFindByName(vm->checkpoints, def->parent.name); if (other) { otherdef = virDomainCheckpointObjGetDef(other); if (!virDomainDefCheckABIStability(otherdef->parent.dom, def->parent.dom, xmlopt)) - return -1; + return NULL; /* Drop and rebuild the parent relationship, but keep all * child relations by reusing chk. */ @@ -563,8 +572,11 @@ virDomainCheckpointRedefinePrep(virDomainObjPtr vm, virObjectUnref(otherdef); other->def = &(*defptr)->parent; *defptr = NULL; - *chk = other; + chk = other; + } else { + chk = virDomainCheckpointAssignDef(vm->checkpoints, def); + *defptr = NULL; } - return 0; + return chk; } diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h index f115b98c2b..631f863151 100644 --- a/src/conf/checkpoint_conf.h +++ b/src/conf/checkpoint_conf.h @@ -90,10 +90,14 @@ virDomainCheckpointDefFormat(virDomainCheckpointDefPtr def, int virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr checkpoint); -int virDomainCheckpointRedefinePrep(virDomainObjPtr vm, - virDomainCheckpointDefPtr *def, - virDomainMomentObjPtr *checkpoint, - virDomainXMLOptionPtr xmlopt, - bool *update_current); +int +virDomainCheckpointRedefinePrep(virDomainObjPtr vm, + virDomainCheckpointDefPtr def, + bool *update_current); + +virDomainMomentObjPtr +virDomainCheckpointRedefineCommit(virDomainObjPtr vm, + virDomainCheckpointDefPtr *defptr, + virDomainXMLOptionPtr xmlopt); VIR_ENUM_DECL(virDomainCheckpoint); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 95e50835ad..fb6fbd1f9f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -86,6 +86,7 @@ virDomainCheckpointDefFormat; virDomainCheckpointDefNew; virDomainCheckpointDefParseString; virDomainCheckpointFormatConvertXMLFlags; +virDomainCheckpointRedefineCommit; virDomainCheckpointRedefinePrep; virDomainCheckpointTypeFromString; virDomainCheckpointTypeToString; diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index fb76c211f8..caedfe1c55 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -377,22 +377,14 @@ qemuCheckpointRedefine(virQEMUDriverPtr driver, virDomainCheckpointDefPtr *def, bool *update_current) { - virDomainMomentObjPtr chk = NULL; - - if (virDomainCheckpointRedefinePrep(vm, def, &chk, driver->xmlopt, - update_current) < 0) + if (virDomainCheckpointRedefinePrep(vm, *def, update_current) < 0) return NULL; /* XXX Should we validate that the redefined checkpoint even * makes sense, such as checking that qemu-img recognizes the * checkpoint bitmap name in at least one of the domain's disks? */ - if (chk) - return chk; - - chk = virDomainCheckpointAssignDef(vm->checkpoints, *def); - *def = NULL; - return chk; + return virDomainCheckpointRedefineCommit(vm, def, driver->xmlopt); } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5c02a8ebb0..cd502781e1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -8989,9 +8989,10 @@ testDomainCheckpointCreateXML(virDomainPtr domain, goto cleanup; if (redefine) { - if (virDomainCheckpointRedefinePrep(vm, &def, &chk, - privconn->xmlopt, - &update_current) < 0) + if (virDomainCheckpointRedefinePrep(vm, def, &update_current) < 0) + goto cleanup; + + if (!(chk = virDomainCheckpointRedefineCommit(vm, &def, privconn->xmlopt))) goto cleanup; } else { if (!(def->parent.dom = virDomainDefCopy(vm->def, @@ -9002,9 +9003,7 @@ testDomainCheckpointCreateXML(virDomainPtr domain, if (virDomainCheckpointAlignDisks(def) < 0) goto cleanup; - } - if (!chk) { if (!(chk = virDomainCheckpointAssignDef(vm->checkpoints, def))) goto cleanup; -- 2.26.2

Validate that the bitmaps are present when redefining a checkpoint. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_checkpoint.c | 52 +++++++++++++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index caedfe1c55..83cad5eab1 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -371,18 +371,56 @@ qemuCheckpointAddActions(virDomainObjPtr vm, } +static int +qemuCheckpointRedefineValidateBitmaps(virDomainObjPtr vm, + virDomainCheckpointDefPtr chkdef) +{ + g_autoptr(virHashTable) blockNamedNodeData = NULL; + size_t i; + + if (virDomainObjCheckActive(vm) < 0) + return -1; + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, QEMU_ASYNC_JOB_NONE))) + return -1; + + for (i = 0; i < chkdef->ndisks; i++) { + virDomainCheckpointDiskDefPtr chkdisk = chkdef->disks + i; + virDomainDiskDefPtr domdisk; + + if (chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) + continue; + + /* we tolerate missing disks due to possible detach */ + if (!(domdisk = virDomainDiskByTarget(vm->def, chkdisk->name))) + continue; + + if (!qemuBlockBitmapChainIsValid(domdisk->src, chkdef->parent.name, + blockNamedNodeData)) { + virReportError(VIR_ERR_CHECKPOINT_INCONSISTENT, + _("missing or broken bitmap '%s' for disk '%s'"), + chkdef->parent.name, domdisk->dst); + return -1; + } + } + + return 0; +} + + static virDomainMomentObjPtr qemuCheckpointRedefine(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainCheckpointDefPtr *def, - bool *update_current) + bool *update_current, + bool validate_bitmaps) { if (virDomainCheckpointRedefinePrep(vm, *def, update_current) < 0) return NULL; - /* XXX Should we validate that the redefined checkpoint even - * makes sense, such as checking that qemu-img recognizes the - * checkpoint bitmap name in at least one of the domain's disks? */ + if (validate_bitmaps && + qemuCheckpointRedefineValidateBitmaps(vm, *def) < 0) + return NULL; return virDomainCheckpointRedefineCommit(vm, def, driver->xmlopt); } @@ -500,11 +538,13 @@ qemuCheckpointCreateXML(virDomainPtr domain, virDomainCheckpointPtr checkpoint = NULL; bool update_current = true; bool redefine = flags & VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE; + bool validate_bitmaps = flags & VIR_DOMAIN_CHECKPOINT_REDEFINE_VALIDATE; unsigned int parse_flags = 0; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virDomainCheckpointDef) def = NULL; - virCheckFlags(VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE, NULL); + virCheckFlags(VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE | + VIR_DOMAIN_CHECKPOINT_REDEFINE_VALIDATE, NULL); if (redefine) { parse_flags |= VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE; @@ -535,7 +575,7 @@ qemuCheckpointCreateXML(virDomainPtr domain, return NULL; if (redefine) { - chk = qemuCheckpointRedefine(driver, vm, &def, &update_current); + chk = qemuCheckpointRedefine(driver, vm, &def, &update_current, validate_bitmaps); } else { chk = qemuCheckpointCreate(driver, vm, &def); } -- 2.26.2

Verify that the checkpoint requested by an incremental backup exists. Unfortunately validating whether the checkpoint configuration actually matches the disk may not be reasonably feasible as the disk may have been renamed/snapshotted/etc. We still rely on bitmap presence. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_backup.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index d0c852cf80..92cdf34c46 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -277,6 +277,17 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm, return -1; if (dd->backupdisk->incremental) { + /* We deliberately don't check the config of the disk in the checkpoint + * definition as it's not guaranteed that the disks still correspond. + * We just verify that a checkpoint exists and later on that the disk + * has corresponding bitmap. */ + if (!virDomainCheckpointFindByName(vm->checkpoints, dd->backupdisk->incremental)) { + virReportError(VIR_ERR_NO_DOMAIN_CHECKPOINT, + _("Checkpoint '%s' for incremental backup of disk '%s' not found"), + dd->backupdisk->incremental, dd->backupdisk->name); + return -1; + } + if (dd->backupdisk->exportbitmap) dd->incrementalBitmap = g_strdup(dd->backupdisk->exportbitmap); else -- 2.26.2