5:16 a.m.
Pavel Hrdina (11):
qemu_snapshot: create: move XML parsing to separate function
qemu_snapshot: create: move XML def validation to separate function
qemu_snapshot: create: move disk align to separate function
qemu_snapshot: create: move virDomainSnapshotAssignDef to both code
paths
qemu_snapshot: create: virDomainSnapshotGetCurrent is not used with
redefine
qemu_snapshot: create: drop update_current variable
qemu_snapshot: create: refactor endjob in qemuSnapshotCreateXML
qemu_snapshot: create: move saving metadata to separate function
qemu_snapshot: create: move snapshot redefine to separate function
qemu_snapshot: create: move snapshot create to separate function
qemu_snapshot: create: drop redefine boolean
src/qemu/qemu_snapshot.c | 409 ++++++++++++++++++++++++---------------
1 file changed, 251 insertions(+), 158 deletions(-)
--
2.31.1
5:16 a.m.
New subject: [libvirt PATCH 01/11] qemu_snapshot: create: move XML parsing to separate function
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 36 +++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 661aeeb8aa..6ba3215ee3 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1531,6 +1531,30 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
}
+static virDomainSnapshotDef*
+qemuSnapshotCreateXMLParse(virDomainObj *vm,
+ virQEMUDriver *driver,
+ const char *xmlDesc,
+ unsigned int flags)
+{
+ qemuDomainObjPrivate *priv = vm->privateData;
+ unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS;
+
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)
+ parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE;
+
+ if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) ||
+ !virDomainObjIsActive(vm))
+ parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE;
+
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE)
+ parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE;
+
+ return virDomainSnapshotDefParseString(xmlDesc, driver->xmlopt,
+ priv->qemuCaps, NULL, parse_flags);
+}
+
+
virDomainSnapshotPtr
qemuSnapshotCreateXML(virDomainPtr domain,
virDomainObj *vm,
@@ -1544,7 +1568,6 @@ qemuSnapshotCreateXML(virDomainPtr domain,
virDomainMomentObj *current = NULL;
bool update_current = true;
bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE;
- unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS;
int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
bool align_match = true;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
@@ -1573,8 +1596,6 @@ qemuSnapshotCreateXML(virDomainPtr domain,
if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) ||
(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA))
update_current = false;
- if (redefine)
- parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE;
if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0)
return NULL;
@@ -1584,15 +1605,8 @@ qemuSnapshotCreateXML(virDomainPtr domain,
_("cannot halt after transient domain snapshot"));
return NULL;
}
- if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) ||
- !virDomainObjIsActive(vm))
- parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE;
- if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE)
- parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_VALIDATE;
-
- if (!(def = virDomainSnapshotDefParseString(xmlDesc, driver->xmlopt,
- priv->qemuCaps, NULL, parse_flags)))
+ if (!(def = qemuSnapshotCreateXMLParse(vm, driver, xmlDesc, flags)))
return NULL;
/* reject snapshot names containing slashes or starting with dot as
--
2.31.1
5:16 a.m.
New subject: [libvirt PATCH 02/11] qemu_snapshot: create: move XML def validation to separate function
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 140 +++++++++++++++++++++------------------
1 file changed, 77 insertions(+), 63 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 6ba3215ee3..5ebe959151 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1555,6 +1555,82 @@ qemuSnapshotCreateXMLParse(virDomainObj *vm,
}
+static int
+qemuSnapshotCreateXMLValidateDef(virDomainObj *vm,
+ virDomainSnapshotDef *def,
+ unsigned int flags)
+{
+ bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE;
+ virDomainSnapshotState state;
+
+ /* reject snapshot names containing slashes or starting with dot as
+ * snapshot definitions are saved in files named by the snapshot name */
+ if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
+ if (strchr(def->parent.name, '/')) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("invalid snapshot name '%s': "
+ "name can't contain '/'"),
+ def->parent.name);
+ return -1;
+ }
+
+ if (def->parent.name[0] == '.') {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("invalid snapshot name '%s': "
+ "name can't start with '.'"),
+ def->parent.name);
+ return -1;
+ }
+ }
+
+ /* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE &&
+ (!virDomainObjIsActive(vm) ||
+ def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("live snapshot creation is supported only "
+ "during full system snapshots"));
+ return -1;
+ }
+
+ /* allow snapshots only in certain states */
+ state = redefine ? def->state : vm->state.state;
+ switch (state) {
+ /* valid states */
+ case VIR_DOMAIN_SNAPSHOT_RUNNING:
+ case VIR_DOMAIN_SNAPSHOT_PAUSED:
+ case VIR_DOMAIN_SNAPSHOT_SHUTDOWN:
+ case VIR_DOMAIN_SNAPSHOT_SHUTOFF:
+ case VIR_DOMAIN_SNAPSHOT_CRASHED:
+ break;
+
+ case VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT:
+ if (!redefine) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state
%s"),
+ virDomainSnapshotStateTypeToString(state));
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_SNAPSHOT_PMSUSPENDED:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("qemu doesn't support taking snapshots of "
+ "PMSUSPENDED guests"));
+ return -1;
+
+ /* invalid states */
+ case VIR_DOMAIN_SNAPSHOT_NOSTATE:
+ case VIR_DOMAIN_SNAPSHOT_BLOCKED: /* invalid state, unused in qemu */
+ case VIR_DOMAIN_SNAPSHOT_LAST:
+ virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"),
+ virDomainSnapshotStateTypeToString(state));
+ return -1;
+ }
+
+ return 0;
+}
+
+
virDomainSnapshotPtr
qemuSnapshotCreateXML(virDomainPtr domain,
virDomainObj *vm,
@@ -1572,7 +1648,6 @@ qemuSnapshotCreateXML(virDomainPtr domain,
bool align_match = true;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
qemuDomainObjPrivate *priv = vm->privateData;
- virDomainSnapshotState state;
g_autoptr(virDomainSnapshotDef) def = NULL;
virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE |
@@ -1609,69 +1684,8 @@ qemuSnapshotCreateXML(virDomainPtr domain,
if (!(def = qemuSnapshotCreateXMLParse(vm, driver, xmlDesc, flags)))
return NULL;
- /* reject snapshot names containing slashes or starting with dot as
- * snapshot definitions are saved in files named by the snapshot name */
- if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
- if (strchr(def->parent.name, '/')) {
- virReportError(VIR_ERR_XML_DETAIL,
- _("invalid snapshot name '%s': "
- "name can't contain '/'"),
- def->parent.name);
- return NULL;
- }
-
- if (def->parent.name[0] == '.') {
- virReportError(VIR_ERR_XML_DETAIL,
- _("invalid snapshot name '%s': "
- "name can't start with '.'"),
- def->parent.name);
- return NULL;
- }
- }
-
- /* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */
- if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE &&
- (!virDomainObjIsActive(vm) ||
- def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)) {
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("live snapshot creation is supported only "
- "during full system snapshots"));
+ if (qemuSnapshotCreateXMLValidateDef(vm, def, flags) < 0)
return NULL;
- }
-
- /* allow snapshots only in certain states */
- state = redefine ? def->state : vm->state.state;
- switch (state) {
- /* valid states */
- case VIR_DOMAIN_SNAPSHOT_RUNNING:
- case VIR_DOMAIN_SNAPSHOT_PAUSED:
- case VIR_DOMAIN_SNAPSHOT_SHUTDOWN:
- case VIR_DOMAIN_SNAPSHOT_SHUTOFF:
- case VIR_DOMAIN_SNAPSHOT_CRASHED:
- break;
-
- case VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT:
- if (!redefine) {
- virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state
%s"),
- virDomainSnapshotStateTypeToString(state));
- return NULL;
- }
- break;
-
- case VIR_DOMAIN_SNAPSHOT_PMSUSPENDED:
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("qemu doesn't support taking snapshots of "
- "PMSUSPENDED guests"));
- return NULL;
-
- /* invalid states */
- case VIR_DOMAIN_SNAPSHOT_NOSTATE:
- case VIR_DOMAIN_SNAPSHOT_BLOCKED: /* invalid state, unused in qemu */
- case VIR_DOMAIN_SNAPSHOT_LAST:
- virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"),
- virDomainSnapshotStateTypeToString(state));
- return NULL;
- }
/* We are going to modify the domain below. Internal snapshots would use
* a regular job, so we need to set the job mask to disallow query as
--
2.31.1
5:16 a.m.
New subject: [libvirt PATCH 03/11] qemu_snapshot: create: move disk align to separate function
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 115 ++++++++++++++++++++++-----------------
1 file changed, 65 insertions(+), 50 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 5ebe959151..7198a2aad3 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1631,6 +1631,69 @@ qemuSnapshotCreateXMLValidateDef(virDomainObj *vm,
}
+static int
+qemuSnapshotAlignDisks(virDomainObj *vm,
+ virDomainSnapshotDef *def,
+ virQEMUDriver *driver,
+ unsigned int flags)
+{
+ g_autofree char *xml = NULL;
+ qemuDomainObjPrivate *priv = vm->privateData;
+ int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
+ bool align_match = true;
+
+ /* Easiest way to clone inactive portion of vm->def is via
+ * conversion in and back out of xml. */
+ if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps,
+ vm->def, priv->origCPU,
+ true, true)) ||
+ !(def->parent.dom = virDomainDefParseString(xml, driver->xmlopt,
+ priv->qemuCaps,
+ VIR_DOMAIN_DEF_PARSE_INACTIVE |
+
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
+ return -1;
+
+ if (vm->newDef) {
+ def->parent.inactiveDom = virDomainDefCopy(vm->newDef,
+ driver->xmlopt, priv->qemuCaps,
true);
+ if (!def->parent.inactiveDom)
+ return -1;
+ }
+
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
+ align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
+ align_match = false;
+ if (virDomainObjIsActive(vm))
+ def->state = VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT;
+ else
+ def->state = VIR_DOMAIN_SNAPSHOT_SHUTOFF;
+ def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
+ } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
+ def->state = virDomainObjGetState(vm, NULL);
+ align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
+ align_match = false;
+ } else {
+ def->state = virDomainObjGetState(vm, NULL);
+
+ if (virDomainObjIsActive(vm) &&
+ def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) {
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("internal snapshot of a running VM "
+ "must include the memory state"));
+ return -1;
+ }
+
+ def->memory = (def->state == VIR_DOMAIN_SNAPSHOT_SHUTOFF ?
+ VIR_DOMAIN_SNAPSHOT_LOCATION_NONE :
+ VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL);
+ }
+ if (virDomainSnapshotAlignDisks(def, align_location, align_match) < 0)
+ return -1;
+
+ return 0;
+}
+
+
virDomainSnapshotPtr
qemuSnapshotCreateXML(virDomainPtr domain,
virDomainObj *vm,
@@ -1638,16 +1701,12 @@ qemuSnapshotCreateXML(virDomainPtr domain,
unsigned int flags)
{
virQEMUDriver *driver = domain->conn->privateData;
- g_autofree char *xml = NULL;
virDomainMomentObj *snap = NULL;
virDomainSnapshotPtr snapshot = NULL;
virDomainMomentObj *current = NULL;
bool update_current = true;
bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE;
- int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
- bool align_match = true;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
- qemuDomainObjPrivate *priv = vm->privateData;
g_autoptr(virDomainSnapshotDef) def = NULL;
virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE |
@@ -1703,54 +1762,10 @@ qemuSnapshotCreateXML(virDomainPtr domain,
flags) < 0)
goto endjob;
} else {
- /* Easiest way to clone inactive portion of vm->def is via
- * conversion in and back out of xml. */
- if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps,
- vm->def, priv->origCPU,
- true, true)) ||
- !(def->parent.dom = virDomainDefParseString(xml, driver->xmlopt,
- priv->qemuCaps,
- VIR_DOMAIN_DEF_PARSE_INACTIVE |
-
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
+ if (qemuSnapshotAlignDisks(vm, def, driver, flags) < 0)
goto endjob;
- if (vm->newDef) {
- def->parent.inactiveDom = virDomainDefCopy(vm->newDef,
- driver->xmlopt,
priv->qemuCaps, true);
- if (!def->parent.inactiveDom)
- goto endjob;
- }
-
- if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
- align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
- align_match = false;
- if (virDomainObjIsActive(vm))
- def->state = VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT;
- else
- def->state = VIR_DOMAIN_SNAPSHOT_SHUTOFF;
- def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
- } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
- def->state = virDomainObjGetState(vm, NULL);
- align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
- align_match = false;
- } else {
- def->state = virDomainObjGetState(vm, NULL);
-
- if (virDomainObjIsActive(vm) &&
- def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) {
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("internal snapshot of a running VM "
- "must include the memory state"));
- goto endjob;
- }
-
- def->memory = (def->state == VIR_DOMAIN_SNAPSHOT_SHUTOFF ?
- VIR_DOMAIN_SNAPSHOT_LOCATION_NONE :
- VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL);
- }
- if (virDomainSnapshotAlignDisks(def, align_location,
- align_match) < 0 ||
- qemuSnapshotPrepare(vm, def, &flags) < 0)
+ if (qemuSnapshotPrepare(vm, def, &flags) < 0)
goto endjob;
}
--
2.31.1
7:45 a.m.
New subject: [libvirt PATCH 03/11] qemu_snapshot: create: move disk align to separate function
On a Thursday in 2021, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 115 ++++++++++++++++++++++-----------------
1 file changed, 65 insertions(+), 50 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 5ebe959151..7198a2aad3 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1631,6 +1631,69 @@ qemuSnapshotCreateXMLValidateDef(virDomainObj *vm,
}
+static int
+qemuSnapshotAlignDisks(virDomainObj *vm,
+ virDomainSnapshotDef *def,
+ virQEMUDriver *driver,
+ unsigned int flags)
All the other functions you separated from qemuSnapshotCreateXML
use the qemuSnapshotCreate prefix.
I think it makes sense here too, since 'flags' are
VIR_DOMAIN_SNAPSHOT_CREATE* flags.
Jano
+{
+ g_autofree char *xml = NULL;
+ qemuDomainObjPrivate *priv = vm->privateData;
+ int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
+ bool align_match = true;
+
5:16 a.m.
New subject: [libvirt PATCH 04/11] qemu_snapshot: create: move virDomainSnapshotAssignDef to both code paths
This makes it obvious that the function is called for creating new
snapshot and redefining old snapshot as well.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 7198a2aad3..6860133bbb 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1761,15 +1761,18 @@ qemuSnapshotCreateXML(virDomainPtr domain,
driver->xmlopt,
flags) < 0)
goto endjob;
+
+ if (!snap) {
+ if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def)))
+ goto endjob;
+ }
} else {
if (qemuSnapshotAlignDisks(vm, def, driver, flags) < 0)
goto endjob;
if (qemuSnapshotPrepare(vm, def, &flags) < 0)
goto endjob;
- }
- if (!snap) {
if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def)))
goto endjob;
--
2.31.1
5:16 a.m.
New subject: [libvirt PATCH 05/11] qemu_snapshot: create: virDomainSnapshotGetCurrent is not used with redefine
Move it to code path for creating new snapshot.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 6860133bbb..9a3f90e346 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1703,7 +1703,6 @@ qemuSnapshotCreateXML(virDomainPtr domain,
virQEMUDriver *driver = domain->conn->privateData;
virDomainMomentObj *snap = NULL;
virDomainSnapshotPtr snapshot = NULL;
- virDomainMomentObj *current = NULL;
bool update_current = true;
bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
@@ -1767,6 +1766,8 @@ qemuSnapshotCreateXML(virDomainPtr domain,
goto endjob;
}
} else {
+ virDomainMomentObj *current = NULL;
+
if (qemuSnapshotAlignDisks(vm, def, driver, flags) < 0)
goto endjob;
@@ -1777,12 +1778,11 @@ qemuSnapshotCreateXML(virDomainPtr domain,
goto endjob;
def = NULL;
- }
- current = virDomainSnapshotGetCurrent(vm->snapshots);
- if (current) {
- if (!redefine)
+ current = virDomainSnapshotGetCurrent(vm->snapshots);
+ if (current) {
snap->def->parent_name = g_strdup(current->def->name);
+ }
}
/* actually do the snapshot */
--
2.31.1
5:16 a.m.
New subject: [libvirt PATCH 06/11] qemu_snapshot: create: drop update_current variable
There is no need for the extra variable as it is used only at one place.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 9a3f90e346..ac57d4e90e 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1703,7 +1703,6 @@ qemuSnapshotCreateXML(virDomainPtr domain,
virQEMUDriver *driver = domain->conn->privateData;
virDomainMomentObj *snap = NULL;
virDomainSnapshotPtr snapshot = NULL;
- bool update_current = true;
bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
g_autoptr(virDomainSnapshotDef) def = NULL;
@@ -1726,10 +1725,6 @@ qemuSnapshotCreateXML(virDomainPtr domain,
VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE,
NULL);
- if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) ||
- (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA))
- update_current = false;
-
if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0)
return NULL;
@@ -1824,7 +1819,7 @@ qemuSnapshotCreateXML(virDomainPtr domain,
endjob:
if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
- if (update_current)
+ if (!redefine || (flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT))
qemuSnapshotSetCurrent(vm, snap);
if (qemuDomainSnapshotWriteMetadata(vm, snap,
--
2.31.1
5:16 a.m.
New subject: [libvirt PATCH 07/11] qemu_snapshot: create: refactor endjob in qemuSnapshotCreateXML
The logic of saving metadata doesn't have to be in endjob section as
it will only happen if we have successfully created snapshot.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index ac57d4e90e..46f65b8144 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1815,10 +1815,10 @@ qemuSnapshotCreateXML(virDomainPtr domain,
* do; we've successfully taken the snapshot, and we are now running
* on it, so we have to go forward the best we can
*/
- snapshot = virGetDomainSnapshot(domain, snap->def->name);
+ if (!(snapshot = virGetDomainSnapshot(domain, snap->def->name)))
+ goto endjob;
- endjob:
- if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
+ if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
if (!redefine || (flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT))
qemuSnapshotSetCurrent(vm, snap);
@@ -1832,13 +1832,17 @@ qemuSnapshotCreateXML(virDomainPtr domain,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unable to save metadata for snapshot %s"),
snap->def->name);
- virDomainSnapshotObjListRemove(vm->snapshots, snap);
- } else {
- virDomainSnapshotLinkParent(vm->snapshots, snap);
+ goto endjob;
}
- } else if (snap) {
+
+ virDomainSnapshotLinkParent(vm->snapshots, snap);
+ snap = NULL;
+ }
+
+ endjob:
+
+ if (snap)
virDomainSnapshotObjListRemove(vm->snapshots, snap);
- }
qemuDomainObjEndAsyncJob(driver, vm);
--
2.31.1
5:16 a.m.
New subject: [libvirt PATCH 08/11] qemu_snapshot: create: move saving metadata to separate function
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 46f65b8144..ce16236224 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1694,6 +1694,27 @@ qemuSnapshotAlignDisks(virDomainObj *vm,
}
+static int
+qemuSnapshotCreateWriteMetadata(virDomainObj *vm,
+ virDomainMomentObj *snap,
+ virQEMUDriver *driver,
+ virQEMUDriverConfig *cfg)
+{
+ if (qemuDomainSnapshotWriteMetadata(vm, snap,
+ driver->xmlopt,
+ cfg->snapshotDir) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to save metadata for snapshot %s"),
+ snap->def->name);
+ return -1;
+ }
+
+ virDomainSnapshotLinkParent(vm->snapshots, snap);
+
+ return 0;
+}
+
+
virDomainSnapshotPtr
qemuSnapshotCreateXML(virDomainPtr domain,
virDomainObj *vm,
@@ -1822,20 +1843,14 @@ qemuSnapshotCreateXML(virDomainPtr domain,
if (!redefine || (flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT))
qemuSnapshotSetCurrent(vm, snap);
- if (qemuDomainSnapshotWriteMetadata(vm, snap,
- driver->xmlopt,
- cfg->snapshotDir) < 0) {
+ if (qemuSnapshotCreateWriteMetadata(vm, snap, driver, cfg) < 0) {
/* if writing of metadata fails, error out rather than trying
* to silently carry on without completing the snapshot */
virObjectUnref(snapshot);
snapshot = NULL;
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unable to save metadata for snapshot %s"),
- snap->def->name);
goto endjob;
}
- virDomainSnapshotLinkParent(vm->snapshots, snap);
snap = NULL;
}
--
2.31.1
5:16 a.m.
New subject: [libvirt PATCH 09/11] qemu_snapshot: create: move snapshot redefine to separate function
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 61 ++++++++++++++++++++++++++++++----------
1 file changed, 46 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index ce16236224..d22f0522ed 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1715,6 +1715,48 @@ qemuSnapshotCreateWriteMetadata(virDomainObj *vm,
}
+static virDomainSnapshotPtr
+qemuSnapshotRedefine(virDomainObj *vm,
+ virDomainPtr domain,
+ virDomainSnapshotDef *def,
+ virQEMUDriver *driver,
+ virQEMUDriverConfig *cfg,
+ unsigned int flags)
+{
+ virDomainMomentObj *snap = NULL;
+ virDomainSnapshotPtr ret = NULL;
+
+ if (virDomainSnapshotRedefinePrep(vm, &def, &snap,
+ driver->xmlopt,
+ flags) < 0)
+ return NULL;
+
+ if (!snap) {
+ if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def)))
+ return NULL;
+ }
+ /* XXX Should we validate that the redefined snapshot even
+ * makes sense, such as checking that qemu-img recognizes the
+ * snapshot name in at least one of the domain's disks? */
+
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)
+ qemuSnapshotSetCurrent(vm, snap);
+
+ if (qemuSnapshotCreateWriteMetadata(vm, snap, driver, cfg) < 0)
+ goto error;
+
+ ret = virGetDomainSnapshot(domain, snap->def->name);
+ if (!ret)
+ goto error;
+
+ return ret;
+
+ error:
+ virDomainSnapshotObjListRemove(vm->snapshots, snap);
+ return NULL;
+}
+
+
virDomainSnapshotPtr
qemuSnapshotCreateXML(virDomainPtr domain,
virDomainObj *vm,
@@ -1772,15 +1814,8 @@ qemuSnapshotCreateXML(virDomainPtr domain,
qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE);
if (redefine) {
- if (virDomainSnapshotRedefinePrep(vm, &def, &snap,
- driver->xmlopt,
- flags) < 0)
- goto endjob;
-
- if (!snap) {
- if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def)))
- goto endjob;
- }
+ snapshot = qemuSnapshotRedefine(vm, domain, def, driver, cfg, flags);
+ goto endjob;
} else {
virDomainMomentObj *current = NULL;
@@ -1802,11 +1837,7 @@ qemuSnapshotCreateXML(virDomainPtr domain,
}
/* actually do the snapshot */
- if (redefine) {
- /* XXX Should we validate that the redefined snapshot even
- * makes sense, such as checking that qemu-img recognizes the
- * snapshot name in at least one of the domain's disks? */
- } else if (virDomainObjIsActive(vm)) {
+ if (virDomainObjIsActive(vm)) {
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY ||
virDomainSnapshotObjGetDef(snap)->memory ==
VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
/* external full system or disk snapshot */
@@ -1840,7 +1871,7 @@ qemuSnapshotCreateXML(virDomainPtr domain,
goto endjob;
if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
- if (!redefine || (flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT))
+ if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT))
qemuSnapshotSetCurrent(vm, snap);
if (qemuSnapshotCreateWriteMetadata(vm, snap, driver, cfg) < 0) {
--
2.31.1
5:16 a.m.
New subject: [libvirt PATCH 10/11] qemu_snapshot: create: move snapshot create to separate function
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 149 ++++++++++++++++++++-------------------
1 file changed, 76 insertions(+), 73 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index d22f0522ed..c5f0a5afe8 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1757,6 +1757,81 @@ qemuSnapshotRedefine(virDomainObj *vm,
}
+static virDomainSnapshotPtr
+qemuSnapshotCreate(virDomainObj *vm,
+ virDomainPtr domain,
+ virDomainSnapshotDef *def,
+ virQEMUDriver *driver,
+ virQEMUDriverConfig *cfg,
+ unsigned int flags)
+{
+
+ virDomainMomentObj *snap = NULL;
+ virDomainMomentObj *current = NULL;
+ virDomainSnapshotPtr ret = NULL;
+
+ if (qemuSnapshotAlignDisks(vm, def, driver, flags) < 0)
+ return NULL;
+
+ if (qemuSnapshotPrepare(vm, def, &flags) < 0)
+ return NULL;
+
+ if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def)))
+ return NULL;
+
+ virObjectRef(def);
+
+ current = virDomainSnapshotGetCurrent(vm->snapshots);
+ if (current) {
+ snap->def->parent_name = g_strdup(current->def->name);
+ }
+
+ /* actually do the snapshot */
+ if (virDomainObjIsActive(vm)) {
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY ||
+ virDomainSnapshotObjGetDef(snap)->memory ==
VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
+ /* external full system or disk snapshot */
+ if (qemuSnapshotCreateActiveExternal(driver, vm, snap, cfg, flags) < 0)
+ goto error;
+ } else {
+ /* internal full system */
+ if (qemuSnapshotCreateActiveInternal(driver, vm, snap, flags) < 0)
+ goto error;
+ }
+ } else {
+ /* inactive; qemuSnapshotPrepare guaranteed that we
+ * aren't mixing internal and external, and altered flags to
+ * contain DISK_ONLY if there is an external disk. */
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
+ bool reuse = !!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT);
+
+ if (qemuSnapshotCreateInactiveExternal(driver, vm, snap, reuse) < 0)
+ goto error;
+ } else {
+ if (qemuSnapshotCreateInactiveInternal(driver, vm, snap) < 0)
+ goto error;
+ }
+ }
+
+ if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
+ qemuSnapshotSetCurrent(vm, snap);
+
+ if (qemuSnapshotCreateWriteMetadata(vm, snap, driver, cfg) < 0)
+ goto error;
+ }
+
+ ret = virGetDomainSnapshot(domain, snap->def->name);
+ if (!ret)
+ goto error;
+
+ return ret;
+
+ error:
+ virDomainSnapshotObjListRemove(vm->snapshots, snap);
+ return NULL;
+}
+
+
virDomainSnapshotPtr
qemuSnapshotCreateXML(virDomainPtr domain,
virDomainObj *vm,
@@ -1764,7 +1839,6 @@ qemuSnapshotCreateXML(virDomainPtr domain,
unsigned int flags)
{
virQEMUDriver *driver = domain->conn->privateData;
- virDomainMomentObj *snap = NULL;
virDomainSnapshotPtr snapshot = NULL;
bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
@@ -1815,81 +1889,10 @@ qemuSnapshotCreateXML(virDomainPtr domain,
if (redefine) {
snapshot = qemuSnapshotRedefine(vm, domain, def, driver, cfg, flags);
- goto endjob;
} else {
- virDomainMomentObj *current = NULL;
-
- if (qemuSnapshotAlignDisks(vm, def, driver, flags) < 0)
- goto endjob;
-
- if (qemuSnapshotPrepare(vm, def, &flags) < 0)
- goto endjob;
-
- if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def)))
- goto endjob;
-
- def = NULL;
-
- current = virDomainSnapshotGetCurrent(vm->snapshots);
- if (current) {
- snap->def->parent_name = g_strdup(current->def->name);
- }
+ snapshot = qemuSnapshotCreate(vm, domain, def, driver, cfg, flags);
}
- /* actually do the snapshot */
- if (virDomainObjIsActive(vm)) {
- if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY ||
- virDomainSnapshotObjGetDef(snap)->memory ==
VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
- /* external full system or disk snapshot */
- if (qemuSnapshotCreateActiveExternal(driver, vm, snap, cfg, flags) < 0)
- goto endjob;
- } else {
- /* internal full system */
- if (qemuSnapshotCreateActiveInternal(driver, vm, snap, flags) < 0)
- goto endjob;
- }
- } else {
- /* inactive; qemuSnapshotPrepare guaranteed that we
- * aren't mixing internal and external, and altered flags to
- * contain DISK_ONLY if there is an external disk. */
- if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
- bool reuse = !!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT);
-
- if (qemuSnapshotCreateInactiveExternal(driver, vm, snap, reuse) < 0)
- goto endjob;
- } else {
- if (qemuSnapshotCreateInactiveInternal(driver, vm, snap) < 0)
- goto endjob;
- }
- }
-
- /* If we fail after this point, there's not a whole lot we can
- * do; we've successfully taken the snapshot, and we are now running
- * on it, so we have to go forward the best we can
- */
- if (!(snapshot = virGetDomainSnapshot(domain, snap->def->name)))
- goto endjob;
-
- if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
- if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT))
- qemuSnapshotSetCurrent(vm, snap);
-
- if (qemuSnapshotCreateWriteMetadata(vm, snap, driver, cfg) < 0) {
- /* if writing of metadata fails, error out rather than trying
- * to silently carry on without completing the snapshot */
- virObjectUnref(snapshot);
- snapshot = NULL;
- goto endjob;
- }
-
- snap = NULL;
- }
-
- endjob:
-
- if (snap)
- virDomainSnapshotObjListRemove(vm->snapshots, snap);
-
qemuDomainObjEndAsyncJob(driver, vm);
return snapshot;
--
2.31.1
5:16 a.m.
New subject: [libvirt PATCH 11/11] qemu_snapshot: create: drop redefine boolean
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index c5f0a5afe8..2b4d8e801b 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1840,7 +1840,6 @@ qemuSnapshotCreateXML(virDomainPtr domain,
{
virQEMUDriver *driver = domain->conn->privateData;
virDomainSnapshotPtr snapshot = NULL;
- bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
g_autoptr(virDomainSnapshotDef) def = NULL;
@@ -1887,7 +1886,7 @@ qemuSnapshotCreateXML(virDomainPtr domain,
qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE);
- if (redefine) {
+ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) {
snapshot = qemuSnapshotRedefine(vm, domain, def, driver, cfg, flags);
} else {
snapshot = qemuSnapshotCreate(vm, domain, def, driver, cfg, flags);
--
2.31.1
7:48 a.m.
On a Thursday in 2021, Pavel Hrdina wrote:
Pavel Hrdina (11):
qemu_snapshot: create: move XML parsing to separate function
qemu_snapshot: create: move XML def validation to separate function
qemu_snapshot: create: move disk align to separate function
qemu_snapshot: create: move virDomainSnapshotAssignDef to both code
paths
qemu_snapshot: create: virDomainSnapshotGetCurrent is not used with
redefine
qemu_snapshot: create: drop update_current variable
qemu_snapshot: create: refactor endjob in qemuSnapshotCreateXML
qemu_snapshot: create: move saving metadata to separate function
qemu_snapshot: create: move snapshot redefine to separate function
qemu_snapshot: create: move snapshot create to separate function
qemu_snapshot: create: drop redefine boolean
src/qemu/qemu_snapshot.c | 409 ++++++++++++++++++++++++---------------
1 file changed, 251 insertions(+), 158 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
1128
days inactive
1128
days old
13 comments
2 participants
participants (2)
-
Ján Tomko -
Pavel Hrdina