[libvirt] [RFC PATCHv2 4/n] snapshot: actually compute branch definition from XML

[Peter Krempa]

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