[libvirt] [RFC PATCHv2 0/n] snapshot: add revert-and-create branching

v1 was here: https://www.redhat.com/archives/libvir-list/2012-November/msg00484.html Since then, I've folded in review comments on 1; 2-4 are new. Documentation is still pending (split out of 1 based on the v1 review). Also, I'm still writing and testing the actual qemu use of the new snapshot_conf.c code, so this series is not quite ready for committing without that being done. In my review of Peter's series, I promised to post my patch for making it easier to clone the in-memory view of a domain when creating or reverting to a snapshot; that's patch 3. Eric Blake (4): snapshot: add revert-and-create branching of external snapshots snapshot: prepare to parse new XML snapshot: make cloning of domain definition easier snapshot: actually compute branch definition from XML docs/formatsnapshot.html.in | 16 +++++-- docs/schemas/domainsnapshot.rng | 5 +++ include/libvirt/libvirt.h.in | 3 ++ src/conf/domain_conf.c | 37 ++++++++++------ src/conf/domain_conf.h | 2 + src/conf/snapshot_conf.c | 93 ++++++++++++++++++++++++++++++++++++++- src/conf/snapshot_conf.h | 2 + src/esx/esx_driver.c | 2 +- src/libvirt.c | 27 +++++++++--- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 17 ++----- src/vbox/vbox_tmpl.c | 2 +- tests/domainsnapshotxml2xmltest.c | 2 +- tools/virsh-snapshot.c | 40 +++++++++++++---- tools/virsh.pod | 16 ++++++- 15 files changed, 215 insertions(+), 50 deletions(-) -- 1.7.11.7

Right now, libvirt refuses to revert to the state at the time of an external snapshot, because doing so would reset things so that the next time the domain boots, we are using the backing file; but modifying the backing file invalidates all qcow2 files that are based on top of it. There are three possibilities for lifting this restriction: 1. delete all snapshot metadata and qcow2 files that are invalidated by the revert (losing changes since the snapshot) 2. perform a block commit (such as with qemu-img commit) to merge the qcow2 file back into the backing file (keeping the changes since the snapshot, but losing the qcow2 files) 3. create NEW qcow2 files that wrap the same base file as the original snapshot (keeping the changes since the original snapshot) This patch documents the API for option 3, by adding a new flag to virDomainSnapshotCreateXML (the only snapshot-related function that takes XML, which is required to pass in new file names during the branch), and wires up virsh to expose it. Later patches will enhance virDomainRevertToSnapshot to cover options 1 and 2. API wise, an application wishing to do the revert-and-create operation must add a <branch>name</branch> element to their <domainsnapshot> XML, and pass VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH in the flags argument. In virsh, snapshot-create gains a new boolean flag --branch that must match the XML passed in, and snapshot-create-as gains a new --branch=name argument along with a --current boolean for creating such XML. * include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH): New flag. * src/libvirt.c (virDomainSnapshotCreateXML): Document it, and enforce some mutual exclusion. * docs/formatsnapshot.html.in: Document the new <branch> element. * docs/schemas/domainsnapshot.rng: Allow new element. * tools/virsh-snapshot.c (cmdSnapshotCreate): Add --branch option. (cmdSnapshotCreateAs): Likewise, also add --current. * tools/virsh.pod (snapshot-create, snapshot-create-as): Document new usage. --- docs/formatsnapshot.html.in | 16 +++++++++++++--- docs/schemas/domainsnapshot.rng | 5 +++++ include/libvirt/libvirt.h.in | 3 +++ src/libvirt.c | 27 ++++++++++++++++++++++----- tools/virsh-snapshot.c | 40 ++++++++++++++++++++++++++++++++-------- tools/virsh.pod | 16 ++++++++++++++-- 6 files changed, 89 insertions(+), 18 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 8fcc04c..b92295f 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -188,9 +188,19 @@ </dd> <dt><code>parent</code></dt> <dd>The parent of this snapshot. If present, this element - contains exactly one child element, name. This specifies the - name of the parent snapshot of this snapshot, and is used to - represent trees of snapshots. Readonly. + contains exactly one child element, <code>name</code>. This + specifies the name of the parent snapshot of this snapshot, + and is used to represent trees of snapshots. Readonly. + </dd> + <dt><code>branch</code></dt> + <dd>The name of an existing external snapshot that forms the + branch point for this snapshot. Required when creating a + snapshot with + the <code>VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH</code> flag, and + not present on output. When creating a branch snapshot, the + same set of <code>disks</code> must be external, + and <code>memory</code> is copied from the branch + point. <span class="since">since 1.0.1</span>. </dd> <dt><code>domain</code></dt> <dd>The domain that this snapshot was taken against. Older diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 45d55b5..7ec01e1 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -85,6 +85,11 @@ </element> </element> </optional> + <optional> + <element name='branch'> + <text/> + </element> + </optional> </interleave> </element> </define> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 84dcde1..be16629 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3777,6 +3777,9 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_CREATE_LIVE = (1 << 8), /* create the snapshot while the guest is running */ + VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH = (1 << 9), /* specify an existing + external snapshot as + the branching point */ } virDomainSnapshotCreateFlags; /* Take a snapshot of the current VM state */ diff --git a/src/libvirt.c b/src/libvirt.c index 4af6089..3229133 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17746,9 +17746,7 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * normally fails if snapshot metadata still remains on the source * machine). When redefining snapshot metadata, the current snapshot * will not be altered unless the VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT - * flag is also present. It is an error to request the - * VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT flag without - * VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE. On some hypervisors, + * flag is also present. On some hypervisors, * redefining an existing snapshot can be used to alter host-specific * portions of the domain XML to be used during revert (such as * backing filenames associated with disk devices), but must not alter @@ -17787,6 +17785,15 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot) * is not present, an error is thrown. Moreover, this flag requires * VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY to be passed as well. * + * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH, then @xmlDesc + * must describe an existing external snapshot as well as actions to + * create a new snapshot from the point where the existing snapshot was + * created rather than the current state of the guest. In this mode, + * the new snapshot branch is created but not activated unless the + * VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT flag is also present. It is an + * error to request VIR_DOMAIN_SNAPSHOT_CREATE_LIVE or + * VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE in this mode. + * * By default, if the snapshot involves external files, and any of the * destination files already exist as a non-empty regular file, the * snapshot is rejected to avoid losing contents of those files. @@ -17843,9 +17850,11 @@ virDomainSnapshotCreateXML(virDomainPtr domain, } if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) && - !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { + !(flags & (VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH))) { virReportInvalidArg(flags, - _("use of 'current' flag in %s requires 'redefine' flag"), + _("use of 'current' flag in %s requires 'redefine' " + "or 'branch' flag"), __FUNCTION__); goto error; } @@ -17863,6 +17872,14 @@ virDomainSnapshotCreateXML(virDomainPtr domain, __FUNCTION__); goto error; } + if (!!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) + + !!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE) + + !!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH) > 1) { + virReportInvalidArg(flags, + _("'redefine', 'live', and 'branch' flags in %s are mutually exclusive"), + __FUNCTION__); + goto error; + } if (conn->driver->domainSnapshotCreateXML) { virDomainSnapshotPtr ret; diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 398730c..6232ec2 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -121,7 +121,8 @@ static const vshCmdOptDef opts_snapshot_create[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"xmlfile", VSH_OT_DATA, 0, N_("domain snapshot XML")}, {"redefine", VSH_OT_BOOL, 0, N_("redefine metadata for existing snapshot")}, - {"current", VSH_OT_BOOL, 0, N_("with redefine, set current snapshot")}, + {"current", VSH_OT_BOOL, 0, + N_("with redefine or branch, set current snapshot")}, {"no-metadata", VSH_OT_BOOL, 0, N_("take snapshot but create no metadata")}, {"halt", VSH_OT_BOOL, 0, N_("halt domain after snapshot is created")}, {"disk-only", VSH_OT_BOOL, 0, N_("capture disk state but not vm state")}, @@ -129,6 +130,7 @@ static const vshCmdOptDef opts_snapshot_create[] = { {"quiesce", VSH_OT_BOOL, 0, N_("quiesce guest's file systems")}, {"atomic", VSH_OT_BOOL, 0, N_("require atomic operation")}, {"live", VSH_OT_BOOL, 0, N_("take a live snapshot")}, + {"branch", VSH_OT_BOOL, 0, N_("create a branch snapshot")}, {NULL, 0, 0, NULL} }; @@ -159,6 +161,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; if (vshCommandOptBool(cmd, "live")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE; + if (vshCommandOptBool(cmd, "branch")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) @@ -307,6 +311,10 @@ static const vshCmdOptDef opts_snapshot_create_as[] = { {"quiesce", VSH_OT_BOOL, 0, N_("quiesce guest's file systems")}, {"atomic", VSH_OT_BOOL, 0, N_("require atomic operation")}, {"live", VSH_OT_BOOL, 0, N_("take a live snapshot")}, + {"branch", VSH_OT_DATA, VSH_OFLAG_REQ_OPT, + N_("name of external snapshot to use as a branch point")}, + {"current", VSH_OT_BOOL, 0, + N_("with branch, make the new snapshot current")}, {"memspec", VSH_OT_DATA, VSH_OFLAG_REQ_OPT, N_("memory attributes: [file=]name[,snapshot=type]")}, {"diskspec", VSH_OT_ARGV, 0, @@ -323,6 +331,7 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) const char *name = NULL; const char *desc = NULL; const char *memspec = NULL; + const char *branch = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; unsigned int flags = 0; const vshCmdOpt *opt = NULL; @@ -341,22 +350,37 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC; if (vshCommandOptBool(cmd, "live")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_LIVE; + if (vshCommandOptBool(cmd, "branch")) { + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH; + if (vshCommandOptString(cmd, "branch", &branch) < 0) { + vshError(ctl, _("branch argument must not be empty")); + goto cleanup; + } + if (vshCommandOptBool(cmd, "memspec")) { + vshError(ctl, _("--branch and --memspec are mutually exclusive")); + goto cleanup; + } + } + if (vshCommandOptBool(cmd, "current")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) goto cleanup; - if (vshCommandOptString(cmd, "name", &name) < 0 || - vshCommandOptString(cmd, "description", &desc) < 0) { - vshError(ctl, _("argument must not be empty")); + if (vshCommandOptString(cmd, "name", &name) < 0) { + vshError(ctl, _("name argument must not be empty")); + goto cleanup; + } + if (vshCommandOptString(cmd, "description", &desc) < 0) { + vshError(ctl, _("description argument must not be empty")); goto cleanup; } virBufferAddLit(&buf, "<domainsnapshot>\n"); - if (name) - virBufferEscapeString(&buf, " <name>%s</name>\n", name); - if (desc) - virBufferEscapeString(&buf, " <description>%s</description>\n", desc); + virBufferEscapeString(&buf, " <name>%s</name>\n", name); + virBufferEscapeString(&buf, " <description>%s</description>\n", desc); + virBufferEscapeString(&buf, " <branch>%s</branch>\n", branch); if (vshCommandOptString(cmd, "memspec", &memspec) < 0 || vshParseSnapshotMemspec(ctl, &buf, memspec) < 0) { diff --git a/tools/virsh.pod b/tools/virsh.pod index c4d505f..38c691e 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2594,7 +2594,7 @@ used to represent properties of snapshots. =item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]] | [I<--no-metadata>] [I<--halt>] [I<--disk-only>] [I<--reuse-external>] -[I<--quiesce>] [I<--atomic>] [I<--live>]} +[I<--quiesce>] [I<--atomic>] [I<--live>] [I<--branch>]} Create a snapshot for domain I<domain> with the properties specified in I<xmlfile>. Normally, the only properties settable for a domain snapshot @@ -2651,6 +2651,12 @@ If I<--live> is specified, libvirt takes the snapshot while the guest is running. This increases the size of the memory image of the external checkpoint. This is currently supported only for external checkpoints. +If I<--branch> is specified, then I<xmlfile> must contain a B<branch> +element naming an existing external snapshot which the new snapshot +will branch from. In this usage, the new snapshot must also be +external, and will not be made current unless I<--current> is also +requested. + Existence of snapshot metadata will prevent attempts to B<undefine> a persistent domain. However, for transient domains, snapshot metadata is silently lost when the domain quits running (whether @@ -2659,7 +2665,8 @@ by command such as B<destroy> or by internal guest action). =item B<snapshot-create-as> I<domain> {[I<--print-xml>] | [I<--no-metadata>] [I<--halt>] [I<--reuse-external>]} [I<name>] [I<description>] [I<--disk-only> [I<--quiesce>]] [I<--atomic>] -[[I<--live>] [I<--memspec> B<memspec>]] [I<--diskspec>] B<diskspec>]... +[[I<--live>] [I<--memspec> B<memspec>]] [I<--branch> B<branch> [I<--current>]] +[I<--diskspec>] B<diskspec>]... Create a snapshot for domain I<domain> with the given <name> and <description>; if either value is omitted, libvirt will choose a @@ -2675,6 +2682,11 @@ by a B<memspec> of the form B<[file=]name[,snapshot=type]>, where type can be B<none>, B<internal>, or B<external>. To include a literal comma in B<file=name>, escape it with a second comma. +The I<--branch> option can be used to create an external snapshot that +branches from an existing snapshot named B<branch>. In this usage, the +new snapshot is not made current unless I<--current> is also present. +Specifying a branch point is incompatible with using I<--memspec>. + The I<--diskspec> option can be used to control how I<--disk-only> and external checkpoints create external files. This option can occur multiple times, according to the number of <disk> elements in the domain -- 1.7.11.7

On 11/20/12 01:09, Eric Blake wrote:
Right now, libvirt refuses to revert to the state at the time of an external snapshot, because doing so would reset things so that the next time the domain boots, we are using the backing file; but modifying the backing file invalidates all qcow2 files that are based on top of it. There are three possibilities for lifting this restriction: 1. delete all snapshot metadata and qcow2 files that are invalidated by the revert (losing changes since the snapshot) 2. perform a block commit (such as with qemu-img commit) to merge the qcow2 file back into the backing file (keeping the changes since the snapshot, but losing the qcow2 files) 3. create NEW qcow2 files that wrap the same base file as the original snapshot (keeping the changes since the original snapshot)
This patch documents the API for option 3, by adding a new flag to virDomainSnapshotCreateXML (the only snapshot-related function that takes XML, which is required to pass in new file names during the branch), and wires up virsh to expose it. Later patches will enhance virDomainRevertToSnapshot to cover options 1 and 2.
API wise, an application wishing to do the revert-and-create operation must add a <branch>name</branch> element to their <domainsnapshot> XML, and pass VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH in the flags argument. In virsh, snapshot-create gains a new boolean flag --branch that must match the XML passed in, and snapshot-create-as gains a new --branch=name argument along with a --current boolean for creating such XML.
* include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_CREATE_BRANCH): New flag. * src/libvirt.c (virDomainSnapshotCreateXML): Document it, and enforce some mutual exclusion. * docs/formatsnapshot.html.in: Document the new <branch> element. * docs/schemas/domainsnapshot.rng: Allow new element. * tools/virsh-snapshot.c (cmdSnapshotCreate): Add --branch option. (cmdSnapshotCreateAs): Likewise, also add --current. * tools/virsh.pod (snapshot-create, snapshot-create-as): Document new usage. --- docs/formatsnapshot.html.in | 16 +++++++++++++--- docs/schemas/domainsnapshot.rng | 5 +++++ include/libvirt/libvirt.h.in | 3 +++ src/libvirt.c | 27 ++++++++++++++++++++++----- tools/virsh-snapshot.c | 40 ++++++++++++++++++++++++++++++++-------- tools/virsh.pod | 16 ++++++++++++++-- 6 files changed, 89 insertions(+), 18 deletions(-)
Looks well designed. ACK. Hopefully no changes will be needed :) Peter

No semantic change, but prepare for a new mode of parsing where a new _BRANCH flag requests that the parse look up the existing snapshot to branch from. * src/conf/snapshot_conf.h (VIR_DOMAIN_SNAPSHOT_PARSE_BRANCH): New flag, unused for now. (virDomainSnapshotDefParseString): Add parameter. * src/conf/snapshot_conf.c (virDomainSnapshotDefParseString): Update signature. * src/esx/esx_driver.c (esxDomainSnapshotCreateXML): Update caller. * src/qemu/qemu_driver.c (qemuDomainSnapshotLoad): Likewise. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotCreateXML): Likewise. * tests/domainsnapshotxml2xmltest.c (testCompareXMLToXMLFiles): Likewise. --- src/conf/snapshot_conf.c | 4 +++- src/conf/snapshot_conf.h | 2 ++ src/esx/esx_driver.c | 2 +- src/qemu/qemu_driver.c | 3 ++- src/vbox/vbox_tmpl.c | 2 +- tests/domainsnapshotxml2xmltest.c | 2 +- 6 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 72bdd30..10aa5e5 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -164,12 +164,14 @@ cleanup: /* flags is bitwise-or of virDomainSnapshotParseFlags. * If flags does not include VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE, then - * caps and expectedVirtTypes are ignored. + * caps and expectedVirtTypes are ignored. If flags does not include + * VIR_DOMAIN_SNAPSHOT_PARSE_BRANCH, then snapshots is ignored. */ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virCapsPtr caps, unsigned int expectedVirtTypes, + virDomainSnapshotObjListPtr snapshots ATTRIBUTE_UNUSED, unsigned int flags) { xmlXPathContextPtr ctxt = NULL; diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index b0f8760..c06f3b3 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -97,11 +97,13 @@ typedef enum { 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_BRANCH = 1 << 4, } virDomainSnapshotParseFlags; virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virCapsPtr caps, unsigned int expectedVirtTypes, + virDomainSnapshotObjListPtr snapshots, unsigned int flags); void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); char *virDomainSnapshotDefFormat(const char *domain_uuid, diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 56f31bb..3c869d7 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4267,7 +4267,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, return NULL; } - def = virDomainSnapshotDefParseString(xmlDesc, NULL, 0, 0); + def = virDomainSnapshotDefParseString(xmlDesc, NULL, 0, NULL, 0); if (def == NULL) { return NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 595c452..039cbf3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -509,7 +509,7 @@ qemuDomainSnapshotLoad(void *payload, def = virDomainSnapshotDefParseString(xmlStr, qemu_driver->caps, QEMU_EXPECTED_VIRT_TYPES, - flags); + NULL, flags); if (def == NULL) { /* Nothing we can do here, skip this one */ VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"), @@ -11511,6 +11511,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!(def = virDomainSnapshotDefParseString(xmlDesc, driver->caps, QEMU_EXPECTED_VIRT_TYPES, + vm->snapshots, parse_flags))) goto cleanup; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index bcffb2f..4fd0505 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5929,7 +5929,7 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, /* VBox has no snapshot metadata, so this flag is trivial. */ virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); - if (!(def = virDomainSnapshotDefParseString(xmlDesc, NULL, 0, 0))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, NULL, 0, NULL, 0))) goto cleanup; if (def->ndisks) { diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index 84278d6..2c631c3 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -35,7 +35,7 @@ testCompareXMLToXMLFiles(const char *inxml, const char *uuid, int internal) flags |= VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL; if (!(def = virDomainSnapshotDefParseString(inXmlData, driver.caps, QEMU_EXPECTED_VIRT_TYPES, - flags))) + NULL, flags))) goto fail; if (!(actual = virDomainSnapshotDefFormat(uuid, def, -- 1.7.11.7

On 11/20/12 01:09, Eric Blake wrote:
No semantic change, but prepare for a new mode of parsing where a new _BRANCH flag requests that the parse look up the existing snapshot to branch from.
* src/conf/snapshot_conf.h (VIR_DOMAIN_SNAPSHOT_PARSE_BRANCH): New flag, unused for now. (virDomainSnapshotDefParseString): Add parameter. * src/conf/snapshot_conf.c (virDomainSnapshotDefParseString): Update signature. * src/esx/esx_driver.c (esxDomainSnapshotCreateXML): Update caller. * src/qemu/qemu_driver.c (qemuDomainSnapshotLoad): Likewise. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotCreateXML): Likewise. * tests/domainsnapshotxml2xmltest.c (testCompareXMLToXMLFiles): Likewise. --- src/conf/snapshot_conf.c | 4 +++- src/conf/snapshot_conf.h | 2 ++ src/esx/esx_driver.c | 2 +- src/qemu/qemu_driver.c | 3 ++- src/vbox/vbox_tmpl.c | 2 +- tests/domainsnapshotxml2xmltest.c | 2 +- 6 files changed, 10 insertions(+), 5 deletions(-)
Sane incremental change. Good for reviewing :) ACK Peter

Upcoming patches for revert-and-clone branching of snapshots need to be able to copy a domain definition; make this step reusable. * src/conf/domain_conf.h (virDomainDefCopy): New prototype. * src/conf/domain_conf.c (virDomainObjCopyPersistentDef): Split... (virDomainDefCopy): ...into new function. (virDomainObjSetDefTransient): Use it. * src/libvirt_private.syms (domain_conf.h): Export it. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use it. --- src/conf/domain_conf.c | 37 +++++++++++++++++++++++-------------- src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 14 ++------------ 4 files changed, 28 insertions(+), 26 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ed21f0f..be76c06 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1861,8 +1861,6 @@ virDomainObjSetDefTransient(virCapsPtr caps, bool live) { int ret = -1; - char *xml = NULL; - virDomainDefPtr newDef = NULL; if (!virDomainObjIsActive(domain) && !live) return 0; @@ -1873,17 +1871,11 @@ virDomainObjSetDefTransient(virCapsPtr caps, if (domain->newDef) return 0; - if (!(xml = virDomainDefFormat(domain->def, VIR_DOMAIN_XML_WRITE_FLAGS))) + if (!(domain->newDef = virDomainDefCopy(caps, domain->def, false))) goto out; - if (!(newDef = virDomainDefParseString(caps, xml, -1, - VIR_DOMAIN_XML_READ_FLAGS))) - goto out; - - domain->newDef = newDef; ret = 0; out: - VIR_FREE(xml); return ret; } @@ -14917,24 +14909,41 @@ cleanup: } +/* Copy src into a new definition; with the quality of the copy + * depending on the migratable flag (false for transitions between + * persistent and active, true for transitions across save files or + * snapshots). */ virDomainDefPtr -virDomainObjCopyPersistentDef(virCapsPtr caps, virDomainObjPtr dom) +virDomainDefCopy(virCapsPtr caps, virDomainDefPtr src, bool migratable) { char *xml; - virDomainDefPtr cur, ret; + virDomainDefPtr ret; + unsigned int write_flags = VIR_DOMAIN_XML_WRITE_FLAGS; + unsigned int read_flags = VIR_DOMAIN_XML_READ_FLAGS; - cur = virDomainObjGetPersistentDef(caps, dom); + if (migratable) + write_flags |= VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_MIGRATABLE; - xml = virDomainDefFormat(cur, VIR_DOMAIN_XML_WRITE_FLAGS); + /* Easiest to clone via a round-trip through XML. */ + xml = virDomainDefFormat(src, write_flags); if (!xml) return NULL; - ret = virDomainDefParseString(caps, xml, -1, VIR_DOMAIN_XML_READ_FLAGS); + ret = virDomainDefParseString(caps, xml, -1, read_flags); VIR_FREE(xml); return ret; } +virDomainDefPtr +virDomainObjCopyPersistentDef(virCapsPtr caps, virDomainObjPtr dom) +{ + virDomainDefPtr cur; + + cur = virDomainObjGetPersistentDef(caps, dom); + return virDomainDefCopy(caps, cur, false); +} + virDomainState virDomainObjGetState(virDomainObjPtr dom, int *reason) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 091879e..c3e8c16 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1943,6 +1943,8 @@ virDomainLiveConfigHelperMethod(virCapsPtr caps, unsigned int *flags, virDomainDefPtr *persistentDef); +virDomainDefPtr virDomainDefCopy(virCapsPtr caps, virDomainDefPtr src, + bool migratable); virDomainDefPtr virDomainObjCopyPersistentDef(virCapsPtr caps, virDomainObjPtr dom); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5a07139..756d7bd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -312,6 +312,7 @@ virDomainDefCheckABIStability; virDomainDefClearDeviceAliases; virDomainDefClearPCIAddresses; virDomainDefCompatibleDevice; +virDomainDefCopy; virDomainDefFormat; virDomainDefFormatInternal; virDomainDefFree; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 039cbf3..f5bbc52 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12190,23 +12190,13 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } /* Prepare to copy the snapshot inactive xml as the config of this - * domain. Easiest way is by a round trip through xml. + * domain. * * XXX Should domain snapshots track live xml rather * than inactive xml? */ snap->def->current = true; if (snap->def->dom) { - char *xml; - if (!(xml = qemuDomainDefFormatXML(driver, - snap->def->dom, - VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_SECURE | - VIR_DOMAIN_XML_MIGRATABLE))) - goto cleanup; - config = virDomainDefParseString(driver->caps, xml, - QEMU_EXPECTED_VIRT_TYPES, - VIR_DOMAIN_XML_INACTIVE); - VIR_FREE(xml); + config = virDomainDefCopy(driver->caps, snap->def->dom, true); if (!config) goto cleanup; } -- 1.7.11.7

On 11/20/12 01:09, Eric Blake wrote:
Upcoming patches for revert-and-clone branching of snapshots need to be able to copy a domain definition; make this step reusable.
* src/conf/domain_conf.h (virDomainDefCopy): New prototype. * src/conf/domain_conf.c (virDomainObjCopyPersistentDef): Split... (virDomainDefCopy): ...into new function. (virDomainObjSetDefTransient): Use it. * src/libvirt_private.syms (domain_conf.h): Export it. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use it. --- src/conf/domain_conf.c | 37 +++++++++++++++++++++++-------------- src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 14 ++------------ 4 files changed, 28 insertions(+), 26 deletions(-)
ACK and you can push this out of order. I'll fix my series to take this improvement into account. Peter

On 11/20/2012 08:05 AM, Peter Krempa wrote:
On 11/20/12 01:09, Eric Blake wrote:
Upcoming patches for revert-and-clone branching of snapshots need to be able to copy a domain definition; make this step reusable.
* src/conf/domain_conf.h (virDomainDefCopy): New prototype. * src/conf/domain_conf.c (virDomainObjCopyPersistentDef): Split... (virDomainDefCopy): ...into new function. (virDomainObjSetDefTransient): Use it. * src/libvirt_private.syms (domain_conf.h): Export it. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use it. --- src/conf/domain_conf.c | 37 +++++++++++++++++++++++-------------- src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 14 ++------------ 4 files changed, 28 insertions(+), 26 deletions(-)
ACK and you can push this out of order. I'll fix my series to take this improvement into account.
Done. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

When asked to parse a branch snapshot XML definition, we have to piece together the definition of the new snapshot from parts of the branch point, as well as run some sanity checks that the branches are compatible. This patch is rather restrictive in what it allows; depending on effort and future needs, we may be able to relax some of those restrictions and allow more cases. * src/conf/snapshot_conf.c (virDomainSnapshotDefParseString): Honor new flag. --- src/conf/snapshot_conf.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 89 insertions(+), 2 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 10aa5e5..901705e 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -171,7 +171,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virCapsPtr caps, unsigned int expectedVirtTypes, - virDomainSnapshotObjListPtr snapshots ATTRIBUTE_UNUSED, + virDomainSnapshotObjListPtr snapshots, unsigned int flags) { xmlXPathContextPtr ctxt = NULL; @@ -188,6 +188,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, char *memorySnapshot = NULL; char *memoryFile = NULL; bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE); + virDomainSnapshotObjPtr other = NULL; xml = virXMLParseCtxt(NULL, xmlStr, _("(domain_snapshot)"), &ctxt); if (!xml) { @@ -223,7 +224,61 @@ virDomainSnapshotDefParseString(const char *xmlStr, def->description = virXPathString("string(./description)", ctxt); - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_BRANCH) { + if ((flags & (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL)) || + !snapshots) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected parse request")); + goto cleanup; + } + + /* In order to form a branch, we must find the existing + * snapshot, and it must be external. */ + tmp = virXPathString("string(./branch)", ctxt); + if (!tmp) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("use of branch flag requires <branch> element")); + goto cleanup; + } + other = virDomainSnapshotFindByName(snapshots, tmp); + if (!other) { + virReportError(VIR_ERR_XML_ERROR, _("could not find branch '%s'"), + tmp); + VIR_FREE(tmp); + goto cleanup; + } + + if (!virDomainSnapshotIsExternal(other)) { + virReportError(VIR_ERR_XML_ERROR, + _("branch '%s' is not an external snapshot"), tmp); + VIR_FREE(tmp); + goto cleanup; + } + if (!other->def->dom) { + virReportError(VIR_ERR_XML_ERROR, + _("branch '%s' lacks corresponding domain details"), + tmp); + VIR_FREE(tmp); + goto cleanup; + } + VIR_FREE(tmp); + + /* The new definition shares the same <parent>, <state>, and + * <domain> as what it is branching from. */ + def->creationTime = tv.tv_sec; + if (other->def->parent && + !(def->parent = strdup(other->def->parent))) { + virReportOOMError(); + goto cleanup; + } + def->state = other->def->state; + + /* Any <memory> or <disk> in the XML must be consistent with + * the branch point, and any <disk> not in the XML will be + * populated to match the branch; checked below. */ + + } else if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { if (virXPathLongLong("string(./creationTime)", ctxt, &def->creationTime) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -314,6 +369,24 @@ virDomainSnapshotDefParseString(const char *xmlStr, _("memory state cannot be saved with offline snapshot")); goto cleanup; } + if (other) { + /* XXX It would be nice to allow automatic reuse of existing + * memory files, with bookkeeping that prevents deleting a + * file if some other branch is still using it. But for a + * first implementation, it is easier to enforce that the user + * always matches (disk-only branching to disk-only; + * checkpoint branching to checkpoint and giving us a new name + * which we set up as a copy). There is also a question of + * whether attempting a hard link rather than always copying + * to a new inode makes sense, if the original is a regular + * file and not a block device. */ + if (other->def->memory != def->memory) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot convert between disk-only and checkpoint; " + "<memory> element must match across branch")); + goto cleanup; + } + } def->file = memoryFile; memoryFile = NULL; @@ -335,6 +408,20 @@ virDomainSnapshotDefParseString(const char *xmlStr, _("unable to handle disk requests in snapshot")); goto cleanup; } + if (other) { + /* For now, we only allow branch snapshots of external snapshots. */ + if (virDomainSnapshotAlignDisks(def, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, + true) < 0) + goto cleanup; + for (i = 0; i < def->ndisks; def++) + if (def->disks[i].snapshot != other->def->disks[i].snapshot) { + virReportError(VIR_ERR_XML_ERROR, + _("mismatch in snapshot mode for disk '%s'"), + def->disks[i].name); + goto cleanup; + } + } if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { if (virXPathInt("string(./active)", ctxt, &active) < 0) { -- 1.7.11.7

On 11/20/12 01:09, Eric Blake wrote:
When asked to parse a branch snapshot XML definition, we have to piece together the definition of the new snapshot from parts of the branch point, as well as run some sanity checks that the branches are compatible. This patch is rather restrictive in what it allows; depending on effort and future needs, we may be able to relax some of those restrictions and allow more cases.
* src/conf/snapshot_conf.c (virDomainSnapshotDefParseString): Honor new flag.
Now when 3/n is pushed this and the prepare one will be right together. Although the changes in 2/n are trivial I'd probably rather keep them separate for now.
--- src/conf/snapshot_conf.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 89 insertions(+), 2 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 10aa5e5..901705e 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -171,7 +171,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virCapsPtr caps, unsigned int expectedVirtTypes, - virDomainSnapshotObjListPtr snapshots ATTRIBUTE_UNUSED, + virDomainSnapshotObjListPtr snapshots, unsigned int flags) { xmlXPathContextPtr ctxt = NULL; @@ -188,6 +188,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, char *memorySnapshot = NULL; char *memoryFile = NULL; bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE); + virDomainSnapshotObjPtr other = NULL;
I'd also set "tmp" to NULL here and ...
xml = virXMLParseCtxt(NULL, xmlStr, _("(domain_snapshot)"), &ctxt); if (!xml) { @@ -223,7 +224,61 @@ virDomainSnapshotDefParseString(const char *xmlStr,
def->description = virXPathString("string(./description)", ctxt);
- if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_BRANCH) { + if ((flags & (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL)) || + !snapshots) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected parse request")); + goto cleanup; + } + + /* In order to form a branch, we must find the existing + * snapshot, and it must be external. */ + tmp = virXPathString("string(./branch)", ctxt); + if (!tmp) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("use of branch flag requires <branch> element")); + goto cleanup; + } + other = virDomainSnapshotFindByName(snapshots, tmp); + if (!other) { + virReportError(VIR_ERR_XML_ERROR, _("could not find branch '%s'"), + tmp); + VIR_FREE(tmp);
move this free statement right to the cleanup section.
+ goto cleanup; + } + + if (!virDomainSnapshotIsExternal(other)) { + virReportError(VIR_ERR_XML_ERROR, + _("branch '%s' is not an external snapshot"), tmp); + VIR_FREE(tmp); + goto cleanup; + } + if (!other->def->dom) { + virReportError(VIR_ERR_XML_ERROR, + _("branch '%s' lacks corresponding domain details"), + tmp); + VIR_FREE(tmp); + goto cleanup; + } + VIR_FREE(tmp); + + /* The new definition shares the same <parent>, <state>, and + * <domain> as what it is branching from. */ + def->creationTime = tv.tv_sec; + if (other->def->parent && + !(def->parent = strdup(other->def->parent))) { + virReportOOMError(); + goto cleanup; + } + def->state = other->def->state; + + /* Any <memory> or <disk> in the XML must be consistent with + * the branch point, and any <disk> not in the XML will be + * populated to match the branch; checked below. */ + + } else if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { if (virXPathLongLong("string(./creationTime)", ctxt, &def->creationTime) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -314,6 +369,24 @@ virDomainSnapshotDefParseString(const char *xmlStr, _("memory state cannot be saved with offline snapshot")); goto cleanup; } + if (other) { + /* XXX It would be nice to allow automatic reuse of existing + * memory files, with bookkeeping that prevents deleting a + * file if some other branch is still using it. But for a + * first implementation, it is easier to enforce that the user + * always matches (disk-only branching to disk-only; + * checkpoint branching to checkpoint and giving us a new name + * which we set up as a copy). There is also a question of + * whether attempting a hard link rather than always copying + * to a new inode makes sense, if the original is a regular + * file and not a block device. */
Hm, despite this was my idea it's looking more strange the more I think about it. If we're going to have the users to specify a new image file now we will need to support that after we come up with a better version. I'm going to think about this a bit more. But for now it seems to be the laziest solution.
+ if (other->def->memory != def->memory) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot convert between disk-only and checkpoint; " + "<memory> element must match across branch")); + goto cleanup; + } + } def->file = memoryFile; memoryFile = NULL;
@@ -335,6 +408,20 @@ virDomainSnapshotDefParseString(const char *xmlStr, _("unable to handle disk requests in snapshot")); goto cleanup; } + if (other) { + /* For now, we only allow branch snapshots of external snapshots. */ + if (virDomainSnapshotAlignDisks(def, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, + true) < 0) + goto cleanup; + for (i = 0; i < def->ndisks; def++) + if (def->disks[i].snapshot != other->def->disks[i].snapshot) { + virReportError(VIR_ERR_XML_ERROR, + _("mismatch in snapshot mode for disk '%s'"), + def->disks[i].name); + goto cleanup; + } + }
if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { if (virXPathInt("string(./active)", ctxt, &active) < 0) {
Looks good as a starting point. Peter

On 11/20/2012 09:41 AM, Peter Krempa wrote:
@@ -188,6 +188,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, char *memorySnapshot = NULL; char *memoryFile = NULL; bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE); + virDomainSnapshotObjPtr other = NULL;
I'd also set "tmp" to NULL here and ...
xml = virXMLParseCtxt(NULL, xmlStr, _("(domain_snapshot)"), &ctxt); if (!xml) { @@ -223,7 +224,61 @@ virDomainSnapshotDefParseString(const char *xmlStr,
+ tmp = virXPathString("string(./branch)", ctxt); + if (!tmp) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("use of branch flag requires <branch> element")); + goto cleanup; + } + other = virDomainSnapshotFindByName(snapshots, tmp); + if (!other) { + virReportError(VIR_ERR_XML_ERROR, _("could not find branch '%s'"), + tmp); + VIR_FREE(tmp);
move this free statement right to the cleanup section.
Good idea. Also, it will let me raise an error if the user passed <branch> in the xml but did not pass the right flag.
+ if (other) { + /* XXX It would be nice to allow automatic reuse of existing + * memory files, with bookkeeping that prevents deleting a + * file if some other branch is still using it. But for a + * first implementation, it is easier to enforce that the user + * always matches (disk-only branching to disk-only; + * checkpoint branching to checkpoint and giving us a new name + * which we set up as a copy). There is also a question of + * whether attempting a hard link rather than always copying + * to a new inode makes sense, if the original is a regular + * file and not a block device. */
Hm, despite this was my idea it's looking more strange the more I think about it. If we're going to have the users to specify a new image file now we will need to support that after we come up with a better version. I'm going to think about this a bit more. But for now it seems to be the laziest solution.
Actually, now that I'm diving into qemu implementation, it is actually harder to allow (or even require) the user to pass the external file name; in order to link() or copy the original file into the new name, the qemu driver has to know what the old name was. So I either have to alter this function signature to return one more piece of information, or else explicitly plan that any code we do for deleting a snapshot is careful to not delete the external memory file if the snapshot has siblings. Also, while link() is fast, copying is potentially slow (and since state files can be several hundred megabytes, doing a copy can also potentially wreak havoc on the file system cache unless we use O_DIRECT, and I really don't want to go there). So in my next revision of the series, I'm going with the shared file approach, as it just seems cleaner all around. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 11/19/2012 05:09 PM, Eric Blake wrote:
When asked to parse a branch snapshot XML definition, we have to piece together the definition of the new snapshot from parts of the branch point, as well as run some sanity checks that the branches are compatible. This patch is rather restrictive in what it allows; depending on effort and future needs, we may be able to relax some of those restrictions and allow more cases.
+ if (other) { + /* For now, we only allow branch snapshots of external snapshots. */ + if (virDomainSnapshotAlignDisks(def, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, + true) < 0) + goto cleanup; + for (i = 0; i < def->ndisks; def++)
You can tell I was sending this late yesterday, with blatant bugs like iterating the wrong variable. Now that I've actually tested the patch, and had valgrind point out the obvious, my v3 should be something that actually works. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Peter Krempa