[PATCH 00/14] Snapshot definition cleanups
Cleanup patches extracted from my upcoming series for handling snapshots with outsourced storage. Peter Krempa (14): virDomainDiskDefFormat: Refactor to virXMLFormatElement qemuSnapshotDiskPrepareActiveExternal: Handle only external snapshots qemuSnapshotCreateAlignDisks: Rewrite logic for selecting default memory snapshot mode virDomainSnapshotDiskDefParseXML: Automatically free temporary variables and remove cleanup virStorageSource: Convert 'type' to proper enum conf: Move definition of 'virDomainSnapshotLocation' Rename VIR_DOMAIN_SNAPSHOT_LOCATION_NONE to VIR_DOMAIN_SNAPSHOT_LOCATION_NO qemuDomainSnapshotForEachQcow2Raw: Act only on internal snapshots conf: snapshot: Remove VIR_DOMAIN_SNAPSHOT_PARSE_DISKS flag virDomainSnapshotDefParse: Refactor cleanup virDomainSnapshotDefParse: Avoid 'memoryfile' temporary variable virDomainSnapshotDefParse: Decouple parsing of memory snapshot config conf: snapshot: Use proper types for snapshot location qemuSnapshotCreateActiveExternal: Remove duplicit assignment src/ch/ch_monitor.c | 1 + src/conf/domain_conf.c | 122 ++++++------ src/conf/domain_conf.h | 14 +- src/conf/domain_validate.c | 2 +- src/conf/snapshot_conf.c | 255 ++++++++++++-------------- src/conf/snapshot_conf.h | 23 +-- src/conf/storage_source_conf.h | 2 +- src/libvirt_private.syms | 4 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 1 - src/qemu/qemu_snapshot.c | 23 ++- src/test/test_driver.c | 7 +- src/vbox/vbox_common.c | 4 +- src/vz/vz_driver.c | 2 +- src/vz/vz_sdk.c | 2 +- tests/qemudomainsnapshotxml2xmltest.c | 2 +- 16 files changed, 221 insertions(+), 245 deletions(-) -- 2.35.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 79 ++++++++++++++++++++---------------------- 1 file changed, 38 insertions(+), 41 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 34fec887a3..69f61aadc1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -23568,6 +23568,8 @@ virDomainDiskDefFormat(virBuffer *buf, const char *device = virDomainDiskDeviceTypeToString(def->device); const char *bus = virDomainDiskBusTypeToString(def->bus); const char *sgio = virDomainDeviceSGIOTypeToString(def->sgio); + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); if (!type || !def->src->type) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -23590,39 +23592,35 @@ virDomainDiskDefFormat(virBuffer *buf, return -1; } - virBufferAsprintf(buf, - "<disk type='%s' device='%s'", - type, device); + virBufferAsprintf(&attrBuf, " type='%s' device='%s'", type, device); if (def->model) { - virBufferAsprintf(buf, " model='%s'", + virBufferAsprintf(&attrBuf, " model='%s'", virDomainDiskModelTypeToString(def->model)); } if (def->rawio) { - virBufferAsprintf(buf, " rawio='%s'", + virBufferAsprintf(&attrBuf, " rawio='%s'", virTristateBoolTypeToString(def->rawio)); } if (def->sgio) - virBufferAsprintf(buf, " sgio='%s'", sgio); + virBufferAsprintf(&attrBuf, " sgio='%s'", sgio); if (def->snapshot && !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && def->src->readonly)) - virBufferAsprintf(buf, " snapshot='%s'", + virBufferAsprintf(&attrBuf, " snapshot='%s'", virDomainSnapshotLocationTypeToString(def->snapshot)); - virBufferAddLit(buf, ">\n"); - virBufferAdjustIndent(buf, 2); - virDomainDiskDefFormatDriver(buf, def); + virDomainDiskDefFormatDriver(&childBuf, def); /* Format as child of <disk> if defined there; otherwise, * if defined as child of <source>, then format later */ if (def->src->auth && def->diskElementAuth) - virStorageAuthDefFormat(buf, def->src->auth); + virStorageAuthDefFormat(&childBuf, def->src->auth); - if (virDomainDiskSourceFormat(buf, def->src, "source", def->startupPolicy, + if (virDomainDiskSourceFormat(&childBuf, def->src, "source", def->startupPolicy, true, flags, def->diskElementAuth, def->diskElementEnc, xmlopt) < 0) @@ -23630,75 +23628,74 @@ virDomainDiskDefFormat(virBuffer *buf, /* Don't format backingStore to inactive XMLs until the code for * persistent storage of backing chains is ready. */ - if (virDomainDiskBackingStoreFormat(buf, def->src, xmlopt, flags) < 0) + if (virDomainDiskBackingStoreFormat(&childBuf, def->src, xmlopt, flags) < 0) return -1; - virBufferEscapeString(buf, "<backenddomain name='%s'/>\n", def->domain_name); + virBufferEscapeString(&childBuf, "<backenddomain name='%s'/>\n", def->domain_name); - virDomainDiskGeometryDefFormat(buf, def); - virDomainDiskBlockIoDefFormat(buf, def); + virDomainDiskGeometryDefFormat(&childBuf, def); + virDomainDiskBlockIoDefFormat(&childBuf, def); - if (virDomainDiskDefFormatMirror(buf, def, flags, xmlopt) < 0) + if (virDomainDiskDefFormatMirror(&childBuf, def, flags, xmlopt) < 0) return -1; - virBufferAsprintf(buf, "<target dev='%s' bus='%s'", + virBufferAsprintf(&childBuf, "<target dev='%s' bus='%s'", def->dst, bus); if ((def->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY || def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) && def->tray_status != VIR_DOMAIN_DISK_TRAY_CLOSED) - virBufferAsprintf(buf, " tray='%s'", + virBufferAsprintf(&childBuf, " tray='%s'", virDomainDiskTrayTypeToString(def->tray_status)); if (def->bus == VIR_DOMAIN_DISK_BUS_USB && def->removable != VIR_TRISTATE_SWITCH_ABSENT) { - virBufferAsprintf(buf, " removable='%s'", + virBufferAsprintf(&childBuf, " removable='%s'", virTristateSwitchTypeToString(def->removable)); } if (def->rotation_rate) - virBufferAsprintf(buf, " rotation_rate='%u'", def->rotation_rate); - virBufferAddLit(buf, "/>\n"); + virBufferAsprintf(&childBuf, " rotation_rate='%u'", def->rotation_rate); + virBufferAddLit(&childBuf, "/>\n"); - virDomainDiskDefFormatIotune(buf, def); + virDomainDiskDefFormatIotune(&childBuf, def); if (def->src->readonly) - virBufferAddLit(buf, "<readonly/>\n"); + virBufferAddLit(&childBuf, "<readonly/>\n"); if (def->src->shared) - virBufferAddLit(buf, "<shareable/>\n"); + virBufferAddLit(&childBuf, "<shareable/>\n"); if (def->transient) { - virBufferAddLit(buf, "<transient"); + virBufferAddLit(&childBuf, "<transient"); if (def->transientShareBacking == VIR_TRISTATE_BOOL_YES) - virBufferAddLit(buf, " shareBacking='yes'"); - virBufferAddLit(buf, "/>\n"); + virBufferAddLit(&childBuf, " shareBacking='yes'"); + virBufferAddLit(&childBuf, "/>\n"); } - virBufferEscapeString(buf, "<serial>%s</serial>\n", def->serial); - virBufferEscapeString(buf, "<wwn>%s</wwn>\n", def->wwn); - virBufferEscapeString(buf, "<vendor>%s</vendor>\n", def->vendor); - virBufferEscapeString(buf, "<product>%s</product>\n", def->product); + virBufferEscapeString(&childBuf, "<serial>%s</serial>\n", def->serial); + virBufferEscapeString(&childBuf, "<wwn>%s</wwn>\n", def->wwn); + virBufferEscapeString(&childBuf, "<vendor>%s</vendor>\n", def->vendor); + virBufferEscapeString(&childBuf, "<product>%s</product>\n", def->product); /* If originally found as a child of <disk>, then format thusly; * otherwise, will be formatted as child of <source> */ if (def->src->encryption && def->diskElementEnc && - virStorageEncryptionFormat(buf, def->src->encryption) < 0) + virStorageEncryptionFormat(&childBuf, def->src->encryption) < 0) return -1; - virDomainDeviceInfoFormat(buf, &def->info, flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT); + virDomainDeviceInfoFormat(&childBuf, &def->info, flags | VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT); - if (virDomainDiskDefFormatPrivateData(buf, def, flags, xmlopt) < 0) + if (virDomainDiskDefFormatPrivateData(&childBuf, def, flags, xmlopt) < 0) return -1; /* format diskElementAuth and diskElementEnc into status XML to preserve * formatting */ if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS) { - g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) secretPlacementAttrBuf = VIR_BUFFER_INITIALIZER; if (def->diskElementAuth) - virBufferAddLit(&attrBuf, " auth='true'"); + virBufferAddLit(&secretPlacementAttrBuf, " auth='true'"); if (def->diskElementEnc) - virBufferAddLit(&attrBuf, " enc='true'"); + virBufferAddLit(&secretPlacementAttrBuf, " enc='true'"); - virXMLFormatElement(buf, "diskSecretsPlacement", &attrBuf, NULL); + virXMLFormatElement(&childBuf, "diskSecretsPlacement", &secretPlacementAttrBuf, NULL); } - virBufferAdjustIndent(buf, -2); - virBufferAddLit(buf, "</disk>\n"); + virXMLFormatElement(buf, "disk", &attrBuf, &childBuf); return 0; } -- 2.35.1
Preparation steps ensure that the 'snapshot' field can only be 'VIR_DOMAIN_SNAPSHOT_LOCATION_NONE' or VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL' at this point, but upcoming patches will change that. Handle only external snapshots. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index cb2a7eb739..f92b571b06 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1165,7 +1165,7 @@ qemuSnapshotDiskPrepareActiveExternal(virDomainObj *vm, snapctxt = qemuSnapshotDiskContextNew(snapdef->ndisks, vm, asyncJob); for (i = 0; i < snapdef->ndisks; i++) { - if (snapdef->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) + if (snapdef->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue; if (qemuSnapshotDiskPrepareOne(snapctxt, -- 2.35.1
Use an if/else branch rather than a expression with a ternary operator. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index f92b571b06..355a2fd782 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1672,9 +1672,10 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, return -1; } - def->memory = (def->state == VIR_DOMAIN_SNAPSHOT_SHUTOFF ? - VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : - VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL); + if (def->state == VIR_DOMAIN_SNAPSHOT_SHUTOFF) + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; + else + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; } if (virDomainSnapshotAlignDisks(def, NULL, align_location, true) < 0) return -1; -- 2.35.1
Refactor the function to avoid the cleanup section used to just free memory associated with the parsed object. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 58 ++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 32 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 79a7b891a2..a2e45632bf 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -138,21 +138,20 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, unsigned int flags, virDomainXMLOption *xmlopt) { - int ret = -1; - char *snapshot = NULL; - char *type = NULL; - char *driver = NULL; + g_autofree char *snapshot = NULL; + g_autofree char *type = NULL; + g_autofree char *driver = NULL; + g_autofree char *name = NULL; + g_autoptr(virStorageSource) src = virStorageSourceNew(); xmlNodePtr cur; VIR_XPATH_NODE_AUTORESTORE(ctxt) ctxt->node = node; - def->src = virStorageSourceNew(); - def->name = virXMLPropString(node, "name"); - if (!def->name) { + if (!(name = virXMLPropString(node, "name"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing name from disk snapshot element")); - goto cleanup; + return -1; } snapshot = virXMLPropString(node, "snapshot"); @@ -162,59 +161,54 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk snapshot setting '%s'"), snapshot); - goto cleanup; + return -1; } } if ((type = virXMLPropString(node, "type"))) { - if ((def->src->type = virStorageTypeFromString(type)) <= 0 || - def->src->type == VIR_STORAGE_TYPE_VOLUME || - def->src->type == VIR_STORAGE_TYPE_DIR) { + if ((src->type = virStorageTypeFromString(type)) <= 0 || + src->type == VIR_STORAGE_TYPE_VOLUME || + src->type == VIR_STORAGE_TYPE_DIR) { virReportError(VIR_ERR_XML_ERROR, _("unknown disk snapshot type '%s'"), type); - goto cleanup; + return -1; } } else { - def->src->type = VIR_STORAGE_TYPE_FILE; + src->type = VIR_STORAGE_TYPE_FILE; } if ((cur = virXPathNode("./source", ctxt)) && - virDomainStorageSourceParse(cur, ctxt, def->src, flags, xmlopt) < 0) - goto cleanup; + virDomainStorageSourceParse(cur, ctxt, src, flags, xmlopt) < 0) + return -1; if ((driver = virXPathString("string(./driver/@type)", ctxt)) && - (def->src->format = virStorageFileFormatTypeFromString(driver)) <= 0) { + (src->format = virStorageFileFormatTypeFromString(driver)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown disk snapshot driver '%s'"), driver); - goto cleanup; + return -1; } if (virParseScaledValue("./driver/metadata_cache/max_size", NULL, ctxt, - &def->src->metadataCacheMaxSize, + &src->metadataCacheMaxSize, 1, ULLONG_MAX, false) < 0) - goto cleanup; + return -1; /* validate that the passed path is absolute */ - if (virStorageSourceIsRelative(def->src)) { + if (virStorageSourceIsRelative(src)) { virReportError(VIR_ERR_XML_ERROR, _("disk snapshot image path '%s' must be absolute"), - def->src->path); - goto cleanup; + src->path); + return -1; } - if (!def->snapshot && (def->src->path || def->src->format)) + if (!def->snapshot && (src->path || src->format)) def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - ret = 0; - cleanup: + def->name = g_steal_pointer(&name); + def->src = g_steal_pointer(&src); - VIR_FREE(driver); - VIR_FREE(snapshot); - VIR_FREE(type); - if (ret < 0) - virDomainSnapshotDiskDefClear(def); - return ret; + return 0; } /* flags is bitwise-or of virDomainSnapshotParseFlags. -- 2.35.1
Use 'virStorageType' as type for the 'type' member and convert the code to work properly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/ch/ch_monitor.c | 1 + src/conf/domain_conf.c | 27 ++++++++++++++------------- src/conf/snapshot_conf.c | 24 +++++++++++++----------- src/conf/storage_source_conf.h | 2 +- 4 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/ch/ch_monitor.c b/src/ch/ch_monitor.c index 60905e36c2..84085b7991 100644 --- a/src/ch/ch_monitor.c +++ b/src/ch/ch_monitor.c @@ -198,6 +198,7 @@ virCHMonitorBuildDiskJson(virJSONValue *disks, virDomainDiskDef *diskdef) case VIR_STORAGE_TYPE_VOLUME: case VIR_STORAGE_TYPE_NVME: case VIR_STORAGE_TYPE_VHOST_USER: + case VIR_STORAGE_TYPE_LAST: default: virReportEnumRangeError(virStorageType, diskdef->src->type); return -1; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 69f61aadc1..40ff71d7db 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8502,11 +8502,15 @@ virDomainStorageSourceParseBase(const char *type, src = virStorageSourceNew(); src->type = VIR_STORAGE_TYPE_FILE; - if (type && - (src->type = virStorageTypeFromString(type)) <= 0) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown storage source type '%s'"), type); - return NULL; + if (type) { + int tmp; + if ((tmp = virStorageTypeFromString(type)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("unknown storage source type '%s'"), type); + return NULL; + } + + src->type = tmp; } if (format && @@ -9055,19 +9059,16 @@ virDomainDiskDefParseSourceXML(virDomainXMLOption *xmlopt, { g_autoptr(virStorageSource) src = virStorageSourceNew(); VIR_XPATH_NODE_AUTORESTORE(ctxt) - g_autofree char *type = NULL; xmlNodePtr tmp; ctxt->node = node; - src->type = VIR_STORAGE_TYPE_FILE; - - if ((type = virXMLPropString(node, "type")) && - (src->type = virStorageTypeFromString(type)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk type '%s'"), type); + if (virXMLPropEnumDefault(node, "type", + virStorageTypeFromString, + VIR_XML_PROP_NONZERO, + &src->type, + VIR_STORAGE_TYPE_FILE) < 0) return NULL; - } if ((tmp = virXPathNode("./source[1]", ctxt))) { if (virDomainStorageSourceParse(tmp, ctxt, src, flags, xmlopt) < 0) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index a2e45632bf..812cca2ed1 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -139,7 +139,6 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, virDomainXMLOption *xmlopt) { g_autofree char *snapshot = NULL; - g_autofree char *type = NULL; g_autofree char *driver = NULL; g_autofree char *name = NULL; g_autoptr(virStorageSource) src = virStorageSourceNew(); @@ -165,16 +164,19 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, } } - if ((type = virXMLPropString(node, "type"))) { - if ((src->type = virStorageTypeFromString(type)) <= 0 || - src->type == VIR_STORAGE_TYPE_VOLUME || - src->type == VIR_STORAGE_TYPE_DIR) { - virReportError(VIR_ERR_XML_ERROR, - _("unknown disk snapshot type '%s'"), type); - return -1; - } - } else { - src->type = VIR_STORAGE_TYPE_FILE; + if (virXMLPropEnumDefault(node, "type", + virStorageTypeFromString, + VIR_XML_PROP_NONZERO, + &src->type, + VIR_STORAGE_TYPE_FILE) < 0) + return -1; + + if (src->type == VIR_STORAGE_TYPE_VOLUME || + src->type == VIR_STORAGE_TYPE_DIR) { + virReportError(VIR_ERR_XML_ERROR, + _("unsupported disk snapshot type '%s'"), + virStorageTypeToString(src->type)); + return -1; } if ((cur = virXPathNode("./source", ctxt)) && diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h index c720d093be..e984421e3d 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -269,7 +269,7 @@ struct _virStorageSource { virObject parent; unsigned int id; /* backing chain identifier, 0 is unset */ - int type; /* virStorageType */ + virStorageType type; char *path; int protocol; /* virStorageNetProtocol */ char *volume; /* volume name for remote storage */ -- 2.35.1
The snapshot location enum is also needed for the disk definition so if we house it inside domain_conf we can use the proper type for it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 10 +++++++++- src/conf/domain_conf.h | 14 +++++++++++++- src/conf/snapshot_conf.c | 8 -------- src/conf/snapshot_conf.h | 12 ------------ src/libvirt_private.syms | 4 ++-- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 40ff71d7db..721623129c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1402,6 +1402,14 @@ VIR_ENUM_IMPL(virDomainIBS, "fixed-na", ); +VIR_ENUM_IMPL(virDomainSnapshotLocation, + VIR_DOMAIN_SNAPSHOT_LOCATION_LAST, + "default", + "no", + "internal", + "external", +); + /* Internal mapping: subset of block job types that can be present in * <mirror> XML (remaining types are not two-phase). */ VIR_ENUM_DECL(virDomainBlockJob); @@ -23608,7 +23616,7 @@ virDomainDiskDefFormat(virBuffer *buf, if (def->sgio) virBufferAsprintf(&attrBuf, " sgio='%s'", sgio); - if (def->snapshot && + if (def->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT && !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && def->src->readonly)) virBufferAsprintf(&attrBuf, " snapshot='%s'", diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9fcf842ee7..5f6b508e89 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -537,6 +537,18 @@ typedef enum { } virDomainMemoryAllocation; +typedef enum { + VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT = 0, + VIR_DOMAIN_SNAPSHOT_LOCATION_NONE, + VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, + + VIR_DOMAIN_SNAPSHOT_LOCATION_LAST +} virDomainSnapshotLocation; + +VIR_ENUM_DECL(virDomainSnapshotLocation); + + /* Stores the virtual disk configuration */ struct _virDomainDiskDef { virStorageSource *src; /* non-NULL. XXX Allow NULL for empty cdrom? */ @@ -581,7 +593,7 @@ struct _virDomainDiskDef { virTristateSwitch ioeventfd; virTristateSwitch event_idx; virTristateSwitch copy_on_read; - unsigned int snapshot; /* virDomainSnapshotLocation, snapshot_conf.h */ + virDomainSnapshotLocation snapshot; virDomainStartupPolicy startupPolicy; bool transient; virTristateBool transientShareBacking; diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 812cca2ed1..a4b3cd8c2b 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -67,14 +67,6 @@ virDomainSnapshotOnceInit(void) VIR_ONCE_GLOBAL_INIT(virDomainSnapshot); -VIR_ENUM_IMPL(virDomainSnapshotLocation, - VIR_DOMAIN_SNAPSHOT_LOCATION_LAST, - "default", - "no", - "internal", - "external", -); - /* virDomainSnapshotState is really virDomainState plus one extra state */ VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_SNAPSHOT_LAST, diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index c8997c710c..b7e0d441ff 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -26,17 +26,6 @@ #include "moment_conf.h" #include "virenum.h" -/* Items related to snapshot state */ - -typedef enum { - VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT = 0, - VIR_DOMAIN_SNAPSHOT_LOCATION_NONE, - VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL, - VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, - - VIR_DOMAIN_SNAPSHOT_LOCATION_LAST -} virDomainSnapshotLocation; - /** * This enum has to map all known domain states from the public enum * virDomainState, before adding one additional state possible only @@ -139,5 +128,4 @@ int virDomainSnapshotRedefinePrep(virDomainObj *vm, virDomainXMLOption *xmlopt, unsigned int flags); -VIR_ENUM_DECL(virDomainSnapshotLocation); VIR_ENUM_DECL(virDomainSnapshotState); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6f0d72ca38..01503083ef 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -637,6 +637,8 @@ virDomainSmartcardTypeFromString; virDomainSmartcardTypeToString; virDomainSmbiosModeTypeFromString; virDomainSmbiosModeTypeToString; +virDomainSnapshotLocationTypeFromString; +virDomainSnapshotLocationTypeToString; virDomainSoundDefFind; virDomainSoundDefFree; virDomainSoundDefRemove; @@ -1024,8 +1026,6 @@ virDomainSnapshotDiskDefFree; virDomainSnapshotDiskDefParseXML; virDomainSnapshotFormatConvertXMLFlags; virDomainSnapshotIsExternal; -virDomainSnapshotLocationTypeFromString; -virDomainSnapshotLocationTypeToString; virDomainSnapshotRedefinePrep; virDomainSnapshotStateTypeFromString; virDomainSnapshotStateTypeToString; -- 2.35.1
The string value associated to the enum is "no". Rename the enum accordingly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 6 +++--- src/conf/domain_conf.h | 2 +- src/conf/domain_validate.c | 2 +- src/conf/snapshot_conf.c | 12 ++++++------ src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_snapshot.c | 12 ++++++------ src/test/test_driver.c | 4 ++-- src/vz/vz_sdk.c | 2 +- 8 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 721623129c..58fe24a8c1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5521,7 +5521,7 @@ virDomainDiskDefPostParse(virDomainDiskDef *disk, if (disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT && disk->src->readonly) - disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; + disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NO; if (disk->src->type == VIR_STORAGE_TYPE_NETWORK && disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) { @@ -5536,7 +5536,7 @@ virDomainDiskDefPostParse(virDomainDiskDef *disk, /* vhost-user doesn't allow us to snapshot, disable snapshots by default */ if (disk->src->type == VIR_STORAGE_TYPE_VHOST_USER && disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT) { - disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; + disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NO; } if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && @@ -23617,7 +23617,7 @@ virDomainDiskDefFormat(virBuffer *buf, virBufferAsprintf(&attrBuf, " sgio='%s'", sgio); if (def->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT && - !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && + !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO && def->src->readonly)) virBufferAsprintf(&attrBuf, " snapshot='%s'", virDomainSnapshotLocationTypeToString(def->snapshot)); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5f6b508e89..a4de46773c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -539,7 +539,7 @@ typedef enum { typedef enum { VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT = 0, - VIR_DOMAIN_SNAPSHOT_LOCATION_NONE, + VIR_DOMAIN_SNAPSHOT_LOCATION_NO, VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL, VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index f0b8aa2655..d6869e8fd8 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -317,7 +317,7 @@ virDomainDiskVhostUserValidate(const virDomainDiskDef *disk) return -1; } - if (disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) { + if (disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_NO) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("only snapshot=no is supported with vhostuser disk")); return -1; diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index a4b3cd8c2b..f5ae2ee212 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -337,11 +337,11 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; } else if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { def->memory = (offline ? - VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : + VIR_DOMAIN_SNAPSHOT_LOCATION_NO : VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL); } if (offline && def->memory && - def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) { + def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_NO) { virReportError(VIR_ERR_XML_ERROR, "%s", _("memory state cannot be saved with offline or " "disk-only snapshot")); @@ -681,15 +681,15 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, if (snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT) { if (domdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT && (!require_match || - domdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) { + domdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO)) { snapdisk->snapshot = domdisk->snapshot; } else { snapdisk->snapshot = default_snapshot; } } else if (require_match && snapdisk->snapshot != default_snapshot && - !(snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && - domdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) { + !(snapdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO && + domdisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' must use snapshot mode '%s'"), snapdisk->name, @@ -728,7 +728,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, /* Don't snapshot empty drives */ if (virStorageSourceIsEmpty(domdef->disks[i]->src)) - snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; + snapdisk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NO; else snapdisk->snapshot = domdef->disks[i]->snapshot; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6b61fefb8f..936a0c8a35 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6886,7 +6886,7 @@ qemuDomainSnapshotForEachQcow2Raw(virQEMUDriver *driver, /* FIXME: we also need to handle LVM here */ if (def->disks[i]->device != VIR_DOMAIN_DISK_DEVICE_DISK || - snapdef->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) + snapdef->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO) continue; if (!virStorageSourceIsLocalStorage(def->disks[i]->src)) { diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 355a2fd782..c5fc7dcf67 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -692,7 +692,7 @@ qemuSnapshotPrepare(virDomainObj *vm, virDomainSnapshotDiskDef *disk = &def->disks[i]; virDomainDiskDef *dom_disk = vm->def->disks[i]; - if (disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && + if (disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_NO && qemuDomainDiskBlockJobIsActive(dom_disk)) return -1; @@ -757,7 +757,7 @@ qemuSnapshotPrepare(virDomainObj *vm, external++; break; - case VIR_DOMAIN_SNAPSHOT_LOCATION_NONE: + case VIR_DOMAIN_SNAPSHOT_LOCATION_NO: /* Remember seeing a disk that has snapshot disabled */ if (!virStorageSourceIsEmpty(dom_disk->src) && !dom_disk->src->readonly) @@ -773,7 +773,7 @@ qemuSnapshotPrepare(virDomainObj *vm, } if (!found_internal && !external && - def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) { + def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_NO) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("nothing selected for snapshot")); return -1; @@ -1657,7 +1657,7 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, def->state = VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT; else def->state = VIR_DOMAIN_SNAPSHOT_SHUTOFF; - def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NO; } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { def->state = virDomainObjGetState(vm, NULL); align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; @@ -1665,7 +1665,7 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, def->state = virDomainObjGetState(vm, NULL); if (virDomainObjIsActive(vm) && - def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) { + def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_NO) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("internal snapshot of a running VM " "must include the memory state")); @@ -1673,7 +1673,7 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, } if (def->state == VIR_DOMAIN_SNAPSHOT_SHUTOFF) - def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NO; else def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4eca5c4a65..da1c6c8f6e 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -8723,14 +8723,14 @@ testDomainSnapshotAlignDisks(virDomainObj *vm, def->state = VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT; else def->state = VIR_DOMAIN_SNAPSHOT_SHUTOFF; - def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NO; } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { def->state = virDomainObjGetState(vm, NULL); align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; } else { def->state = virDomainObjGetState(vm, NULL); def->memory = def->state == VIR_DOMAIN_SNAPSHOT_SHUTOFF ? - VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : + VIR_DOMAIN_SNAPSHOT_LOCATION_NO : VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; } diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index f149360c03..43d528820e 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -4640,7 +4640,7 @@ prlsdkParseSnapshotTree(const char *treexml) def->parent.description = virXPathString("string(./Description)", ctxt); - def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NO; xmlstr = virXPathString("string(./@state)", ctxt); if (!xmlstr) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", -- 2.35.1
Similarly to the external snapshot code the internal inactive snapshot creation helper should act only when an internal snapshot of the disk is required. For now the callers ensure that it's either _INTERNAL or _NO when control reaches this function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 936a0c8a35..f61509d00b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6886,7 +6886,7 @@ qemuDomainSnapshotForEachQcow2Raw(virQEMUDriver *driver, /* FIXME: we also need to handle LVM here */ if (def->disks[i]->device != VIR_DOMAIN_DISK_DEVICE_DISK || - snapdef->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO) + snapdef->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL) continue; if (!virStorageSourceIsLocalStorage(def->disks[i]->src)) { -- 2.35.1
All callers except the one in the 'esx' driver pass the flag. The 'esx' driver has a check that 'def->ndisks' is zero after parsing the definition. This means that we can simply always parse the disks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 22 ++++++++-------------- src/conf/snapshot_conf.h | 7 +++---- src/qemu/qemu_driver.c | 1 - src/qemu/qemu_snapshot.c | 2 +- src/test/test_driver.c | 3 +-- src/vbox/vbox_common.c | 4 +--- src/vz/vz_driver.c | 2 +- tests/qemudomainsnapshotxml2xmltest.c | 2 +- 8 files changed, 16 insertions(+), 27 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index f5ae2ee212..f477b67785 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -359,21 +359,15 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) goto cleanup; - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS) { - if (n) - def->disks = g_new0(virDomainSnapshotDiskDef, n); - def->ndisks = n; - for (i = 0; i < def->ndisks; i++) { - if (virDomainSnapshotDiskDefParseXML(nodes[i], ctxt, &def->disks[i], - flags, xmlopt) < 0) - goto cleanup; - } - VIR_FREE(nodes); - } else if (n) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("unable to handle disk requests in snapshot")); - goto cleanup; + if (n) + def->disks = g_new0(virDomainSnapshotDiskDef, n); + def->ndisks = n; + for (i = 0; i < def->ndisks; i++) { + if (virDomainSnapshotDiskDefParseXML(nodes[i], ctxt, &def->disks[i], + flags, xmlopt) < 0) + goto cleanup; } + VIR_FREE(nodes); if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { if (!current) { diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index b7e0d441ff..8823af1ac1 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -84,10 +84,9 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSnapshotDef, virObjectUnref); typedef enum { VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0, - VIR_DOMAIN_SNAPSHOT_PARSE_DISKS = 1 << 1, - VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 2, - VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE = 1 << 3, - VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE = 1 << 4, + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 1, + VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE = 1 << 2, + VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE = 1 << 3, } virDomainSnapshotParseFlags; typedef enum { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c1b3bd8536..de7d2b93b9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -347,7 +347,6 @@ qemuDomainSnapshotLoad(virDomainObj *vm, virDomainMomentObj *current = NULL; bool cur; unsigned int flags = (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE | - VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL); int ret = -1; int direrr; diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index c5fc7dcf67..3fb1d3abe0 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1530,7 +1530,7 @@ qemuSnapshotCreateXMLParse(virDomainObj *vm, unsigned int flags) { qemuDomainObjPrivate *priv = vm->privateData; - unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + unsigned int parse_flags = 0; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index da1c6c8f6e..9ceb0b45c8 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -978,7 +978,6 @@ testParseDomainSnapshots(testDriver *privconn, privconn->xmlopt, NULL, &cur, - VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL | VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE); if (!def) @@ -8777,7 +8776,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, virObjectEvent *event = NULL; bool update_current = true; bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE; - unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + unsigned int parse_flags = 0; g_autoptr(virDomainSnapshotDef) def = NULL; /* diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 36db6e06be..acd18494d3 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -5364,8 +5364,7 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, nsresult rc; resultCodeUnion result; virDomainSnapshotPtr ret = NULL; - unsigned int parse_flags = (VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | - VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE); + unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; g_autoptr(virDomainSnapshotDef) def = NULL; if (!data->vboxObj) @@ -6783,7 +6782,6 @@ vboxDomainSnapshotDeleteMetadataOnly(virDomainSnapshotPtr snapshot) } def = virDomainSnapshotDefParseString(defXml, data->xmlopt, NULL, NULL, - VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE); if (!def) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index fdd776282e..2107785ab2 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -2580,7 +2580,7 @@ vzDomainSnapshotCreateXML(virDomainPtr domain, virDomainObj *dom; struct _vzConn *privconn = domain->conn->privateData; struct _vzDriver *driver = privconn->driver; - unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + unsigned int parse_flags = 0; virDomainSnapshotObjList *snapshots = NULL; virDomainMomentObj *current; bool job = false; diff --git a/tests/qemudomainsnapshotxml2xmltest.c b/tests/qemudomainsnapshotxml2xmltest.c index 735d0e21a1..3fa932e755 100644 --- a/tests/qemudomainsnapshotxml2xmltest.c +++ b/tests/qemudomainsnapshotxml2xmltest.c @@ -34,7 +34,7 @@ testCompareXMLToXMLFiles(const char *inxml, g_autofree char *inXmlData = NULL; g_autofree char *outXmlData = NULL; g_autofree char *actual = NULL; - unsigned int parseflags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; + unsigned int parseflags = 0; unsigned int formatflags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE; bool cur = false; g_autoptr(virDomainSnapshotDef) def = NULL; -- 2.35.1
Use automatic memory cleanup, decrease scope of variables and remove the 'cleanup' label. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 74 +++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 42 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index f477b67785..f3b264d2e1 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -216,17 +216,12 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, bool *current, unsigned int flags) { - virDomainSnapshotDef *def = NULL; - virDomainSnapshotDef *ret = NULL; - xmlNodePtr *nodes = NULL; - xmlNodePtr inactiveDomNode = NULL; + g_autoptr(virDomainSnapshotDef) def = NULL; + g_autofree xmlNodePtr *diskNodes = NULL; size_t i; int n; - char *state = NULL; - int active; - char *tmp; - char *memorySnapshot = NULL; - char *memoryFile = NULL; + g_autofree char *memorySnapshot = NULL; + g_autofree char *memoryFile = NULL; bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE); virSaveCookieCallbacks *saveCookie = virDomainXMLOptionGetSaveCookie(xmlopt); int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE | @@ -240,18 +235,22 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { virReportError(VIR_ERR_XML_ERROR, "%s", _("a redefined snapshot must have a name")); - goto cleanup; + return NULL; } } def->parent.description = virXPathString("string(./description)", ctxt); if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + g_autofree char *state = NULL; + g_autofree char *domtype = NULL; + xmlNodePtr inactiveDomNode = NULL; + if (virXPathLongLong("string(./creationTime)", ctxt, &def->parent.creationTime) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing creationTime from existing snapshot")); - goto cleanup; + return NULL; } def->parent.parent_name = virXPathString("string(./parent/name)", ctxt); @@ -263,14 +262,14 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, */ virReportError(VIR_ERR_XML_ERROR, "%s", _("missing state from existing snapshot")); - goto cleanup; + return NULL; } def->state = virDomainSnapshotStateTypeFromString(state); if (def->state <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Invalid state '%s' in domain snapshot XML"), state); - goto cleanup; + return NULL; } offline = (def->state == VIR_DOMAIN_SNAPSHOT_SHUTOFF || def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT); @@ -279,20 +278,19 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, * lack domain/@type. In that case, leave dom NULL, and * clients will have to decide between best effort * initialization or outright failure. */ - if ((tmp = virXPathString("string(./domain/@type)", ctxt))) { + if ((domtype = virXPathString("string(./domain/@type)", ctxt))) { xmlNodePtr domainNode = virXPathNode("./domain", ctxt); - VIR_FREE(tmp); if (!domainNode) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing domain in snapshot")); - goto cleanup; + return NULL; } def->parent.dom = virDomainDefParseNode(ctxt->node->doc, domainNode, xmlopt, parseOpaque, domainflags); if (!def->parent.dom) - goto cleanup; + return NULL; } else { VIR_WARN("parsing older snapshot that lacks domain"); } @@ -304,10 +302,10 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, def->parent.inactiveDom = virDomainDefParseNode(ctxt->node->doc, inactiveDomNode, xmlopt, NULL, domainflags); if (!def->parent.inactiveDom) - goto cleanup; + return NULL; } } else if (virDomainXMLOptionRunMomentPostParse(xmlopt, &def->parent) < 0) { - goto cleanup; + return NULL; } memorySnapshot = virXPathString("string(./memory/@snapshot)", ctxt); @@ -318,20 +316,20 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown memory snapshot setting '%s'"), memorySnapshot); - goto cleanup; + return NULL; } if (memoryFile && def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_XML_ERROR, _("memory filename '%s' requires external snapshot"), memoryFile); - goto cleanup; + return NULL; } if (!memoryFile && def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("external memory snapshots require a filename")); - goto cleanup; + return NULL; } } else if (memoryFile) { def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; @@ -345,7 +343,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, virReportError(VIR_ERR_XML_ERROR, "%s", _("memory state cannot be saved with offline or " "disk-only snapshot")); - goto cleanup; + return NULL; } def->memorysnapshotfile = g_steal_pointer(&memoryFile); @@ -354,48 +352,40 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, virReportError(VIR_ERR_XML_ERROR, _("memory snapshot file path (%s) must be absolute"), def->memorysnapshotfile); - goto cleanup; + return NULL; } - if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) - goto cleanup; + if ((n = virXPathNodeSet("./disks/*", ctxt, &diskNodes)) < 0) + return NULL; if (n) def->disks = g_new0(virDomainSnapshotDiskDef, n); def->ndisks = n; for (i = 0; i < def->ndisks; i++) { - if (virDomainSnapshotDiskDefParseXML(nodes[i], ctxt, &def->disks[i], + if (virDomainSnapshotDiskDefParseXML(diskNodes[i], ctxt, &def->disks[i], flags, xmlopt) < 0) - goto cleanup; + return NULL; } - VIR_FREE(nodes); if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { + int active; + if (!current) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("internal parse requested with NULL current")); - goto cleanup; + return NULL; } if (virXPathInt("string(./active)", ctxt, &active) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not find 'active' element")); - goto cleanup; + return NULL; } *current = active != 0; } if (!offline && virSaveCookieParse(ctxt, &def->cookie, saveCookie) < 0) - goto cleanup; - - ret = g_steal_pointer(&def); - - cleanup: - VIR_FREE(state); - VIR_FREE(nodes); - VIR_FREE(memorySnapshot); - VIR_FREE(memoryFile); - virObjectUnref(def); + return NULL; - return ret; + return g_steal_pointer(&def); } virDomainSnapshotDef * -- 2.35.1
Assign directly into the definition. The cleanup code can deal with that. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index f3b264d2e1..7f98ae6aec 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -221,7 +221,6 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, size_t i; int n; g_autofree char *memorySnapshot = NULL; - g_autofree char *memoryFile = NULL; bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE); virSaveCookieCallbacks *saveCookie = virDomainXMLOptionGetSaveCookie(xmlopt); int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE | @@ -309,7 +308,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, } memorySnapshot = virXPathString("string(./memory/@snapshot)", ctxt); - memoryFile = virXPathString("string(./memory/@file)", ctxt); + def->memorysnapshotfile = virXPathString("string(./memory/@file)", ctxt); if (memorySnapshot) { def->memory = virDomainSnapshotLocationTypeFromString(memorySnapshot); if (def->memory <= 0) { @@ -318,20 +317,20 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, memorySnapshot); return NULL; } - if (memoryFile && + if (def->memorysnapshotfile && def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_XML_ERROR, _("memory filename '%s' requires external snapshot"), - memoryFile); + def->memorysnapshotfile); return NULL; } - if (!memoryFile && + if (!def->memorysnapshotfile && def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("external memory snapshots require a filename")); return NULL; } - } else if (memoryFile) { + } else if (def->memorysnapshotfile) { def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; } else if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { def->memory = (offline ? @@ -345,7 +344,6 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, "disk-only snapshot")); return NULL; } - def->memorysnapshotfile = g_steal_pointer(&memoryFile); /* verify that memory path is absolute */ if (def->memorysnapshotfile && !g_path_is_absolute(def->memorysnapshotfile)) { -- 2.35.1
Separate the steps of parsing the memory snapshot config from the post-processing and validation code. The upcoming patch refactoring the parsing will be simpler. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 50 ++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 7f98ae6aec..594492ccd2 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -317,27 +317,37 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, memorySnapshot); return NULL; } - if (def->memorysnapshotfile && - def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { - virReportError(VIR_ERR_XML_ERROR, - _("memory filename '%s' requires external snapshot"), - def->memorysnapshotfile); - return NULL; - } - if (!def->memorysnapshotfile && - def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("external memory snapshots require a filename")); - return NULL; + } + + if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT) { + if (def->memorysnapshotfile) { + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + } else if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + if (offline) { + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NO; + } else { + def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; + } } - } else if (def->memorysnapshotfile) { - def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - } else if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { - def->memory = (offline ? - VIR_DOMAIN_SNAPSHOT_LOCATION_NO : - VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL); - } - if (offline && def->memory && + } + + if (def->memorysnapshotfile && + def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + virReportError(VIR_ERR_XML_ERROR, + _("memory filename '%s' requires external snapshot"), + def->memorysnapshotfile); + return NULL; + } + + if (!def->memorysnapshotfile && + def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("external memory snapshots require a filename")); + return NULL; + } + + if (offline && + def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT && def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_NO) { virReportError(VIR_ERR_XML_ERROR, "%s", _("memory state cannot be saved with offline or " -- 2.35.1
Refactor the code to use proper types for the memory and disk snapshot location and fix the parsing code to be compatible with an unsigned type. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 39 +++++++++++++++++---------------------- src/conf/snapshot_conf.h | 4 ++-- 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 594492ccd2..e2442441d0 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -130,7 +130,6 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, unsigned int flags, virDomainXMLOption *xmlopt) { - g_autofree char *snapshot = NULL; g_autofree char *driver = NULL; g_autofree char *name = NULL; g_autoptr(virStorageSource) src = virStorageSourceNew(); @@ -145,16 +144,12 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, return -1; } - snapshot = virXMLPropString(node, "snapshot"); - if (snapshot) { - def->snapshot = virDomainSnapshotLocationTypeFromString(snapshot); - if (def->snapshot <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk snapshot setting '%s'"), - snapshot); - return -1; - } - } + if (virXMLPropEnumDefault(node, "snapshot", + virDomainSnapshotLocationTypeFromString, + VIR_XML_PROP_NONZERO, + &def->snapshot, + VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT) < 0) + return -1; if (virXMLPropEnumDefault(node, "type", virStorageTypeFromString, @@ -196,7 +191,8 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, return -1; } - if (!def->snapshot && (src->path || src->format)) + if (def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT && + (src->path || src->format)) def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; def->name = g_steal_pointer(&name); @@ -220,7 +216,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, g_autofree xmlNodePtr *diskNodes = NULL; size_t i; int n; - g_autofree char *memorySnapshot = NULL; + xmlNodePtr memoryNode = NULL; bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE); virSaveCookieCallbacks *saveCookie = virDomainXMLOptionGetSaveCookie(xmlopt); int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE | @@ -307,16 +303,15 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, return NULL; } - memorySnapshot = virXPathString("string(./memory/@snapshot)", ctxt); - def->memorysnapshotfile = virXPathString("string(./memory/@file)", ctxt); - if (memorySnapshot) { - def->memory = virDomainSnapshotLocationTypeFromString(memorySnapshot); - if (def->memory <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown memory snapshot setting '%s'"), - memorySnapshot); + if ((memoryNode = virXPathNode("./memory", ctxt))) { + def->memorysnapshotfile = virXMLPropString(memoryNode, "file"); + + if (virXMLPropEnumDefault(memoryNode, "snapshot", + virDomainSnapshotLocationTypeFromString, + VIR_XML_PROP_NONZERO, + &def->memory, + VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT) < 0) return NULL; - } } if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT) { diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 8823af1ac1..1f787f1a94 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -51,7 +51,7 @@ G_STATIC_ASSERT((int)VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT == VIR_DOMAIN_LAST); typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef; struct _virDomainSnapshotDiskDef { char *name; /* name matching the <target dev='...' of the domain */ - int snapshot; /* virDomainSnapshotLocation */ + virDomainSnapshotLocation snapshot; /* details of wrapper external file. src is always non-NULL. * XXX optimize this to allow NULL for internal snapshots? */ @@ -70,7 +70,7 @@ struct _virDomainSnapshotDef { /* Additional public XML. */ int state; /* virDomainSnapshotState */ - int memory; /* virDomainMemorySnapshot */ + virDomainSnapshotLocation memory; char *memorysnapshotfile; /* memory state file when snapshot is external */ size_t ndisks; /* should not exceed dom->ndisks */ -- 2.35.1
The block of code pausing the VM assigns 'resume' to true but it's already true because of the previous condition. The code is deliberately kept in two blocks as upcoming changes will modify both conditions. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 3fb1d3abe0..a7901779fc 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1400,8 +1400,6 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, _("guest unexpectedly quit")); goto cleanup; } - - resume = true; } } -- 2.35.1
On a Thursday in 2022, Peter Krempa wrote:
Cleanup patches extracted from my upcoming series for handling snapshots with outsourced storage.
Peter Krempa (14): virDomainDiskDefFormat: Refactor to virXMLFormatElement qemuSnapshotDiskPrepareActiveExternal: Handle only external snapshots qemuSnapshotCreateAlignDisks: Rewrite logic for selecting default memory snapshot mode virDomainSnapshotDiskDefParseXML: Automatically free temporary variables and remove cleanup virStorageSource: Convert 'type' to proper enum conf: Move definition of 'virDomainSnapshotLocation' Rename VIR_DOMAIN_SNAPSHOT_LOCATION_NONE to VIR_DOMAIN_SNAPSHOT_LOCATION_NO qemuDomainSnapshotForEachQcow2Raw: Act only on internal snapshots conf: snapshot: Remove VIR_DOMAIN_SNAPSHOT_PARSE_DISKS flag virDomainSnapshotDefParse: Refactor cleanup virDomainSnapshotDefParse: Avoid 'memoryfile' temporary variable virDomainSnapshotDefParse: Decouple parsing of memory snapshot config conf: snapshot: Use proper types for snapshot location qemuSnapshotCreateActiveExternal: Remove duplicit assignment
src/ch/ch_monitor.c | 1 + [..] src/vz/vz_driver.c | 2 +- src/vz/vz_sdk.c | 2 +- tests/qemudomainsnapshotxml2xmltest.c | 2 +- 16 files changed, 221 insertions(+), 245 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa