[libvirt] [PATCH v7 3/4] vbox_tmpl.c: Patch for redefining snapshots

[Daniel P. Berrange]

On Fri, Apr 18, 2014 at 11:51:33AM +0200, Yohan BELLEGUIC wrote:
> The machine is unregistered and its vbox XML file is changed in order to
> add snapshot information. The machine is then registered with the
> snapshot to redefine.
> ---
>  src/vbox/vbox_tmpl.c |  949 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 942 insertions(+), 7 deletions(-)
> 
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index ac000cf..f8667f6 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -38,12 +38,12 @@
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <fcntl.h>
> -#include <libxml/xmlwriter.h>

Ah, removing the bogus include I mentioned in patch 1. Can just squash
this change into the first patch instead.


> +static int
> +vboxSnapshotRedefine(virDomainPtr dom,
> +                     virDomainSnapshotDefPtr def,
> +                     bool isCurrent)
> +{

> +        /*
> +         * Now we have done this swap, we remove the snapshot xml file from the
> +         * current machine location.
> +         */
> +        if (remove(currentSnapshotXmlFilePath) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Unable to delete file %s"), currentSnapshotXmlFilePath);
> +            goto cleanup;
> +        }

Small preference to use  'unlink' instead of remove, since we know
this is a plain file. Also use virReportSystemError(errno, _("....")...)
since we have an errno we can usefully provide here.


> +        /*
> +         * We save the snapshot xml file to retrieve the real read-write disk during the
> +         * next define. This file is saved as "'machineLocation'/snapshot-'uuid'.xml"
> +         */
> +        VIR_FREE(currentSnapshotXmlFilePath);
> +        if (virAsprintf(&currentSnapshotXmlFilePath, "%s%s.xml", machineLocationPath, snapshotMachineDesc->currentSnapshot) < 0)
> +            goto cleanup;
> +        char *snapshotContent = virDomainSnapshotDefFormat(NULL, def, VIR_DOMAIN_XML_SECURE, 0);

Need to check for snapshotContent == NULL here.

> +        xmlDocPtr newXml = virXMLParse(NULL, snapshotContent, NULL);

Or this will cause a crash on OOM

> +        VIR_FREE(snapshotContent);
> +        if (newXml && xmlSaveFile(currentSnapshotXmlFilePath, newXml) < 0) {

This whole sequence of functions is wierd.

You have 'snapshotContent' which is a string containing an XML document.
You then parse this to get an xmlDocPtr.
You then call xmlSaveFile which turns xmlDocPtr back into a string and
writes it to a file.

IMHO you should just use  virFileWriteStr(snapshotContent) and remove
this parsing 


> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                           _("Unable to save new snapshot xml file"));
> +            goto cleanup;
> +        }
> +    }


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|