Pull out the portion of virDomainSnapshotRefinePrep() that deals
with definition sanity into a separate helper routine that can
be reused with bulk redefine, leaving behind only the code
specific to loop checking and in-place updates that are only
needed in single-definition handling.
Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
---
src/conf/snapshot_conf.c | 186 ++++++++++++++++++++-------------------
1 file changed, 96 insertions(+), 90 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 386ec82d15..a5b05eadf4 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -426,6 +426,87 @@ virDomainSnapshotDefParseString(const char *xmlStr,
}
+/* Perform sanity checking on a redefined snapshot definition. If
+ * @other is non-NULL, this may include swapping def->dom from other
+ * into def. */
+static int
+virDomainSnapshotRedefineValidate(virDomainSnapshotDefPtr def,
+ const unsigned char *domain_uuid,
+ virDomainSnapshotObjPtr other,
+ virDomainXMLOptionPtr xmlopt,
+ unsigned int flags)
+{
+ int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
+ bool align_match = true;
+ bool external = def->state == VIR_SNAP_STATE_DISK_SNAPSHOT ||
+ virDomainSnapshotDefIsExternal(def);
+
+ if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && !external) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("disk-only flag for snapshot %s requires "
+ "disk-snapshot state"),
+ def->name);
+ return -1;
+ }
+ if (def->dom && memcmp(def->dom->uuid, domain_uuid, VIR_UUID_BUFLEN)) {
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+ virUUIDFormat(domain_uuid, uuidstr);
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("definition for snapshot %s must use uuid %s"),
+ def->name, uuidstr);
+ return -1;
+ }
+
+ if (other) {
+ if ((other->def->state == VIR_SNAP_STATE_RUNNING ||
+ other->def->state == VIR_SNAP_STATE_PAUSED) !=
+ (def->state == VIR_SNAP_STATE_RUNNING ||
+ def->state == VIR_SNAP_STATE_PAUSED)) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("cannot change between online and offline "
+ "snapshot state in snapshot %s"),
+ def->name);
+ return -1;
+ }
+
+ if ((other->def->state == VIR_SNAP_STATE_DISK_SNAPSHOT) !=
+ (def->state == VIR_SNAP_STATE_DISK_SNAPSHOT)) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("cannot change between disk only and "
+ "full system in snapshot %s"),
+ def->name);
+ return -1;
+ }
+
+ if (other->def->dom) {
+ if (def->dom) {
+ if (!virDomainDefCheckABIStability(other->def->dom,
+ def->dom, xmlopt))
+ return -1;
+ } else {
+ /* Transfer the domain def */
+ def->dom = other->def->dom;
+ other->def->dom = NULL;
+ }
+ }
+ }
+
+ if (def->dom) {
+ if (external) {
+ align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
+ align_match = false;
+ }
+ if (virDomainSnapshotAlignDisks(def, align_location,
+ align_match) < 0)
+ return -1;
+ }
+
+
+ return 0;
+}
+
+
/**
* virDomainSnapshotDefAssignExternalNames:
* @def: snapshot def object
@@ -1325,12 +1406,8 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
unsigned int flags)
{
virDomainSnapshotDefPtr def = *defptr;
- int ret = -1;
- int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
- bool align_match = true;
virDomainSnapshotObjPtr other;
- bool external = def->state == VIR_SNAP_STATE_DISK_SNAPSHOT ||
- virDomainSnapshotDefIsExternal(def);
+ bool check_stolen;
/* Prevent circular chains */
if (def->parent) {
@@ -1338,21 +1415,21 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
virReportError(VIR_ERR_INVALID_ARG,
_("cannot set snapshot %s as its own parent"),
def->name);
- goto cleanup;
+ return -1;
}
other = virDomainSnapshotFindByName(vm->snapshots, def->parent);
if (!other) {
virReportError(VIR_ERR_INVALID_ARG,
_("parent %s for snapshot %s not found"),
def->parent, def->name);
- goto cleanup;
+ return -1;
}
while (other->def->parent) {
if (STREQ(other->def->parent, def->name)) {
virReportError(VIR_ERR_INVALID_ARG,
_("parent %s would create cycle to %s"),
other->def->name, def->name);
- goto cleanup;
+ return -1;
}
other = virDomainSnapshotFindByName(vm->snapshots,
other->def->parent);
@@ -1364,77 +1441,18 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
}
}
- /* Check that any replacement is compatible */
- if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && !external) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("disk-only flag for snapshot %s requires "
- "disk-snapshot state"),
- def->name);
- goto cleanup;
- }
-
- if (def->dom &&
- memcmp(def->dom->uuid, domain->uuid, VIR_UUID_BUFLEN)) {
- char uuidstr[VIR_UUID_STRING_BUFLEN];
-
- virUUIDFormat(domain->uuid, uuidstr);
- virReportError(VIR_ERR_INVALID_ARG,
- _("definition for snapshot %s must use uuid %s"),
- def->name, uuidstr);
- goto cleanup;
- }
-
other = virDomainSnapshotFindByName(vm->snapshots, def->name);
+ check_stolen = other && other->def->dom;
+ if (virDomainSnapshotRedefineValidate(def, domain->uuid, other, xmlopt,
+ flags) < 0) {
+ /* revert stealing of the snapshot domain definition */
+ if (check_stolen && def->dom && !other->def->dom) {
+ other->def->dom = def->dom;
+ def->dom = NULL;
+ }
+ return -1;
+ }
if (other) {
- if ((other->def->state == VIR_SNAP_STATE_RUNNING ||
- other->def->state == VIR_SNAP_STATE_PAUSED) !=
- (def->state == VIR_SNAP_STATE_RUNNING ||
- def->state == VIR_SNAP_STATE_PAUSED)) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("cannot change between online and offline "
- "snapshot state in snapshot %s"),
- def->name);
- goto cleanup;
- }
-
- if ((other->def->state == VIR_SNAP_STATE_DISK_SNAPSHOT) !=
- (def->state == VIR_SNAP_STATE_DISK_SNAPSHOT)) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("cannot change between disk only and "
- "full system in snapshot %s"),
- def->name);
- goto cleanup;
- }
-
- if (other->def->dom) {
- if (def->dom) {
- if (!virDomainDefCheckABIStability(other->def->dom,
- def->dom, xmlopt))
- goto cleanup;
- } else {
- /* Transfer the domain def */
- def->dom = other->def->dom;
- other->def->dom = NULL;
- }
- }
-
- if (def->dom) {
- if (external) {
- align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
- align_match = false;
- }
-
- if (virDomainSnapshotAlignDisks(def, align_location,
- align_match) < 0) {
- /* revert stealing of the snapshot domain definition */
- if (def->dom && !other->def->dom) {
- other->def->dom = def->dom;
- def->dom = NULL;
- }
- goto cleanup;
- }
- }
-
if (other == vm->current_snapshot) {
*update_current = true;
vm->current_snapshot = NULL;
@@ -1447,19 +1465,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
other->def = def;
*defptr = NULL;
*snap = other;
- } else {
- if (def->dom) {
- if (external) {
- align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
- align_match = false;
- }
- if (virDomainSnapshotAlignDisks(def, align_location,
- align_match) < 0)
- goto cleanup;
- }
}
- ret = 0;
- cleanup:
- return ret;
+ return 0;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
