[libvirt] [RFC PATCHv2 4/n] snapshot: actually compute branch definition from XML
Peter Krempa
pkrempa at redhat.com
Tue Nov 20 16:41:29 UTC 2012
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
More information about the libvir-list
mailing list