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(¤tSnapshotXmlFilePath, "%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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list