> On 11/25/2013 07:23 PM, Manuel VIVES wrote:
> > The snapshots are saved in xml files, and then can be redefined
> > ---
> >
> > src/vbox/vbox_tmpl.c | 852
> > +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 844
> > insertions(+), 8 deletions(-)
> >
> > diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> > index e05ed97..23f8aab 100644
> > --- a/src/vbox/vbox_tmpl.c
> > +++ b/src/vbox/vbox_tmpl.c
> > @@ -61,6 +61,7 @@
> >
> > #include "virstring.h"
> > #include "virtime.h"
> > #include "virutil.h"
> >
> > +#include "dirname.h"
> >
> > /* This one changes from version to version. */
> > #if VBOX_API_VERSION == 2002
> >
> > @@ -276,6 +277,12 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL;
> >
> > #endif /* VBOX_API_VERSION >= 4000 */
> >
> > +/*This error is a bit specific
> > + *In the VBOX API it is named E_ACCESSDENIED
> > + *It is returned when the called object is not ready. In
> > + *particular when we do any call on a disk which has been closed
> > +*/
> > +#define VBOX_E_ACCESSDENIED 0x80070005
>
> Is this code not defined anywhere in vbox API include files?
>
I did not find this code anywhere in the API, so I defined it here.
> > #define reportInternalErrorIfNS_FAILED(message) \
> >
> > if (NS_FAILED(rc)) { \
> >
> > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(message)); \
> >
> > @@ -286,6 +293,8 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL;
> >
> > static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char
> > *xml); static int vboxDomainCreate(virDomainPtr dom);
> > static int vboxDomainUndefineFlags(virDomainPtr dom, unsigned int
> > flags);
> >
> > +static virStorageVolPtr vboxStorageVolLookupByPath(virConnectPtr conn,
> > const char *path); +static int vboxStorageDeleteOrClose(virStorageVolPtr
> > vol, unsigned int flags, unsigned int flagDeleteOrClose);
>
> The above line looks to be > 80 columns.
>
> > static void vboxDriverLock(vboxGlobalData *data) {
> >
> > virMutexLock(&data->lock);
> >
> > }
> >
> > @@ -5946,6 +5955,827 @@ cleanup:
> > return snapshot;
> >
> > }
> >
> > +#if VBOX_API_VERSION >=4002
> > +static void
> > +vboxSnapshotXmlRetrieveSnapshotNodeByName(xmlNodePtr a_node,
> > + const char *name,
> > + xmlNodePtr *snap_node)
> > +{
> > + xmlNodePtr cur_node = NULL;
> > +
> > + for (cur_node = a_node; cur_node; cur_node = cur_node->next) {
> > + if (cur_node->type == XML_ELEMENT_NODE) {
> > + if (!xmlStrcmp(cur_node->name, (const xmlChar *) "Snapshot")
> > && + STREQ(virXMLPropString(cur_node, "name"), name)) {
>
> Two problems here:
>
> 1) virXMLPropString() could return NULL in case it doesn't find the
> property you're looking for, and you haven't allowed for that.
>
> 2) virXMLPropString returns a *copy* of the string, and you need to
> VIR_FREE() it after you're finished with it. But you haven't even saved
> a copy of the pointer, so you have no way of doing that.
>
> So every time you call virXMLPropString(), you're either going to leak a
> string, or segv libvirtd.
>
> > + *snap_node = cur_node;
> > + return;
> > + }
> > + }
> > + if (cur_node->children)
> > +
> > vboxSnapshotXmlRetrieveSnapshotNodeByName(cur_node->children, name,
> > snap_node); + }
> > +}
> > +
> > +
> > +
> > +
> > +static int
> > +vboxDetachAndCloseDisks(virDomainPtr dom,
> > + IMedium *disk)
> > +{
> > + VBOX_OBJECT_CHECK(dom->conn, int, -1);
> > + nsresult rc;
> > + PRUnichar *location = NULL;
> > + vboxArray childrenDiskArray = VBOX_ARRAY_INITIALIZER;
> > + virStorageVolPtr volPtr = NULL;
> > + char *location_utf8 = NULL;
> > + PRUint32 dummyState = 0;
> > + size_t i = 0;
> > + if (disk == NULL) {
> > + VIR_DEBUG("Null pointer to disk");
> > + return -1;
>
> If something is an error, and will end up failing an operation, then you
> need to log an error message rather than just outputting a debug
> message. This is true even if the error can only be caused by an
> internal problem in libvirt's own code.
>
> > + }
> > + rc = disk->vtbl->GetLocation(disk, &location);
> > + if (rc == VBOX_E_ACCESSDENIED) {
> > + VIR_DEBUG("Disk already closed");
>
> Not sure if this is an actual error, or a valid occurrence (I'm guessing
> the former, since you skip the rest of the function on failure). If it
> shouldn't happen, then you should be logging an error.
>
This is a valid occurence, it just means that the disk has already
been closed (so it is not useful to continue in the function).
> > + goto cleanup;
> > + }
> > + reportInternalErrorIfNS_FAILED("cannot get disk location");
> > + rc = vboxArrayGet(&childrenDiskArray, disk,
> > disk->vtbl->GetChildren); + reportInternalErrorIfNS_FAILED("cannot
> > get children disks"); + for (i = 0; i < childrenDiskArray.count; ++i)
> > {
> > + IMedium *childDisk = childrenDiskArray.items[i];
> > + if (childDisk) {
> > + vboxDetachAndCloseDisks(dom, childDisk);
> > + }
> > + }
> > + rc = disk->vtbl->RefreshState(disk, &dummyState);
> > + reportInternalErrorIfNS_FAILED("cannot refresh state");
> > + VBOX_UTF16_TO_UTF8(location, &location_utf8);
> > + volPtr = vboxStorageVolLookupByPath(dom->conn, location_utf8);
> > +
> > + if (volPtr) {
> > + VIR_DEBUG("Closing %s", location_utf8);
> > + if (vboxStorageDeleteOrClose(volPtr, 0, VBOX_STORAGE_CLOSE_FLAG)
> > != 0) { + VIR_DEBUG("Error while closing disk");
>
> Same problem here.
>
> > + }
> > + }
> > + VBOX_UTF8_FREE(location_utf8);
> > +cleanup:
> > + VBOX_UTF16_FREE(location);
> > + vboxArrayRelease(&childrenDiskArray);
> > + return ret;
> > +}
> > +
> > +static void
> > +vboxSnapshotXmlAddChild(xmlNodePtr parent,
> > + xmlNodePtr child)
> > +{
> > + /*Used in order to add child without writing the stuff concerning
> > xml namespaces*/ + xmlBufferPtr tmpBuf = xmlBufferCreate();
> > + char *tmpString = NULL;
> > + xmlNodePtr tmpNode = NULL;
> > + xmlNodeDump(tmpBuf, parent->doc, child, 0, 0);
> > + ignore_value(VIR_STRDUP(tmpString, (char
> > *)xmlBufferContent(tmpBuf)));
>
> I don't think it's safe to ignore the return of VIR_STRDUP here.
>
> > + xmlParseInNodeContext(parent, tmpString, (int)strlen(tmpString), 0,
> > &tmpNode); + if (tmpNode) {
> > + if (xmlAddChild(parent, xmlCopyNode(tmpNode, 1)) == NULL) {
> > + VIR_DEBUG("Error while adding %s to %s", (char
> > *)tmpNode->name, (char *)parent->name); + }
> > + }
> > + xmlFree(tmpNode);
> > + xmlBufferFree(tmpBuf);
> > +}
> > +
> > +static void
> > +vboxSnapshotXmlRetrieveMachineNode(xmlNodePtr root,
> > + xmlNodePtr *machineNode)
> > +{
> > + xmlNodePtr cur = root->xmlChildrenNode;
> > + while (cur && xmlIsBlankNode(cur)) {
> > + cur = cur -> next;
> > + }
> > + if (xmlStrcmp(cur->name, (const xmlChar *) "Machine")) {
> > + VIR_DEBUG("Problem, found %s, Machine expected", (char
> > *)cur->name); + return;
> > + }
> > + *machineNode = cur;
> > +}
> > +
> > +static void
> > +vboxSnapshotXmlRetrieveSnapshotsNode(xmlNodePtr snapshotNode,
> > + xmlNodePtr *snapshotsNode)
> > +{
> > + xmlNodePtr cur_node = NULL;
> > +
> > + for (cur_node = snapshotNode; cur_node; cur_node = cur_node->next) {
> > + if (cur_node->type == XML_ELEMENT_NODE) {
> > + if (!xmlStrcmp(cur_node->name, (const xmlChar *)
> > "Snapshots")) { + *snapshotsNode = cur_node;
> > + return;
> > + }
> > + }
> > + if (cur_node->children)
> > + vboxSnapshotXmlRetrieveSnapshotsNode(cur_node->children,
> > snapshotsNode); + }
> > +}
> > +
> > +static void
> > +vboxSnapshotXmlRetrieveRootNodeByName(xmlNodePtr root,
> > + const char *name,
> > + xmlNodePtr *returnNode)
> > +{
> > + xmlNodePtr cur = root->xmlChildrenNode;
> > + while (cur && xmlIsBlankNode(cur)) {
> > + cur = cur -> next;
> > + }
> > + if (xmlStrcmp(cur->name, (const xmlChar *) "Machine")) {
> > + VIR_DEBUG("Problem, found %s, Machine expected", (char
> > *)cur->name); + }
> > + cur=cur->xmlChildrenNode;
> > + while (cur != NULL) {
> > + if (!xmlStrcmp(cur->name, (const xmlChar*) name)) {
> > + *returnNode = cur;
> > + return;
> > + }
> > + cur = cur -> next;
> > + }
> > +}
> > +
> > +static void
> > +vboxSnapshotXmlAppendDiskToMediaRegistry(xmlNodePtr *inMediaRegistry,
> > + char *parentId,
> > + char *childId,
> > + char *childLocation,
> > + char *childFormat)
> > +{
> > + /*This function will modify the inMediaregistry node to append all
> > the informations about the child disk + */
> > + xmlNodePtr cur_node = NULL;
> > + for (cur_node = *inMediaRegistry; cur_node; cur_node =
> > cur_node->next) { + if (cur_node) {
> > + if (cur_node->type == XML_ELEMENT_NODE
> > + && !xmlStrcmp(cur_node->name, (const xmlChar *)
> > "HardDisk") + && xmlHasProp(cur_node, BAD_CAST "uuid") !=
> > NULL + && STREQ((char *)xmlHasProp(cur_node, BAD_CAST
> > "uuid")->children->content, parentId)) { +
> > + xmlNodePtr childDiskNode = xmlNewTextChild(cur_node,
> > NULL, BAD_CAST "HardDisk", NULL); +
> > xmlNewProp(childDiskNode, BAD_CAST "uuid", BAD_CAST childId); +
> > xmlNewProp(childDiskNode, BAD_CAST "location", BAD_CAST
> > childLocation); + xmlNewProp(childDiskNode, BAD_CAST
> > "format", BAD_CAST childFormat);
>
> xmlNewProp can return an error on failure. But of course this function
> only returns void, so there's no way to report it to the caller anyway :-(
>
> > + return;
> > + }
> > + }
> > + if (&(cur_node->children))
> > +
> > vboxSnapshotXmlAppendDiskToMediaRegistry(&(cur_node->children),
> > parentId, childId, childLocation, childFormat); +
> > + }
> > +}
> > +
> > +static int
> > +vboxSnapshotGenerateVboxXML(xmlNodePtr rootElementVboxXML,
> > + char *storageControllerString,
> > + char *parent,
> > + char *defName,
> > + char *timeStamp,
> > + char *uuid,
> > + xmlNodePtr *snapshotNodeReturned)
> > +{
> > + xmlDocPtr doc;
> > + xmlNodePtr snapshotNode;
> > + xmlNodePtr snapshotsNode;
> > + xmlNodePtr machineHardwareNode = NULL;
> > + char *uuidstrWithBrackets = NULL;
> > + char *timeVboxFormat = NULL;
> > + int ret = -1;
> > + /*We change the date format from "yyyy-MM-dd hh:mm:ss.msec+timeZone"
> > to "yyyy-MM-ddThh:mm:ssZ"*/ + char *date =
> > virStringSplit(virStringJoin((const
> > char**)virStringSplit(virStringSplit(timeStamp, "+", 0)[0], " ", 0),
> > "T"), ".", 0)[0];
>
> This is compact, but doesn't protect against OOM errors (I know some
> people argue that once there is an OOM error, you're dead anyway so
> there isn't any point, but that isn't the philosophy that libvirt tries
> to follow). This is a statement about the vbox driver in general, btw,
> not just this particular occurrence in this patch...
>
> > +
> > + /*Retrieve hardware*/
> > + vboxSnapshotXmlRetrieveRootNodeByName(rootElementVboxXML,
> > "Hardware", &machineHardwareNode); + /* Create a new XML DOM tree, to
> > which the XML document will be + * written */
> > + doc = xmlNewDoc(BAD_CAST XML_DEFAULT_VERSION);
> > + if (doc == NULL) {
> > + VIR_DEBUG("Error creating the xml document tree\n");
> > + ret = -1;
> > + goto cleanup;
> > + }
> > + /* Create a new XML node, to which the XML document will be
> > + * appended */
> > + snapshotNode = xmlNewDocNode(doc, NULL, BAD_CAST "Snapshot", NULL);
> > + if (snapshotNode == NULL) {
> > + VIR_DEBUG("Error creating the xml node Snapshot\n");
> > + ret = -1;
> > + goto cleanup;
> > + }
> > + if (virAsprintf(&uuidstrWithBrackets, "{%s}", uuid) != -1) {
> > + xmlNewProp(snapshotNode, BAD_CAST "uuid", BAD_CAST
> > uuidstrWithBrackets); + VIR_FREE(uuidstrWithBrackets);
> > + }
> > + xmlNewProp(snapshotNode, BAD_CAST "name", BAD_CAST defName);
> > + if (virAsprintf(&timeVboxFormat, "%sZ", date) != -1) {
> > + xmlNewProp(snapshotNode, BAD_CAST "timeStamp", BAD_CAST
> > timeVboxFormat); + VIR_FREE(timeVboxFormat);
> > + }
> > + xmlDocSetRootElement(doc, snapshotNode);
> > + if (machineHardwareNode) {
> > + vboxSnapshotXmlAddChild(snapshotNode, machineHardwareNode);
> > + }
> > + xmlNodePtr listStorageControllerNodes;
> > + xmlParseInNodeContext(snapshotNode, storageControllerString,
> > (int)strlen(storageControllerString), 0, &listStorageControllerNodes); +
> > if (listStorageControllerNodes) {
> > + if (xmlAddChild(snapshotNode,
> > xmlCopyNode(listStorageControllerNodes, 1)) == NULL) { +
> > VIR_DEBUG("Could not add listStorageControllerNodes node to snapshot
> > node"); + ret = -1;
> > + goto cleanup;
> > + }
> > + }
> > + snapshotsNode = xmlNewDocNode(doc, NULL, BAD_CAST "Snapshots",
> > NULL); + if (snapshotsNode == NULL) {
> > + VIR_DEBUG("Error creating the xml node Snapshots\n");
> > + ret = -1;
> > + goto cleanup;
> > + }
> > + if (snapshotsNode) {
> > + if (xmlAddChild(snapshotNode, xmlCopyNode(snapshotsNode, 1)) ==
> > NULL) { + VIR_DEBUG("Could not add snapshotsNode node to
> > snapshot node"); + ret = -1;
> > + goto cleanup;
> > + }
> > + }
> > + if (parent) {
> > + xmlNodePtr rootSnapshotNode = NULL;
> > + xmlNodePtr parentNode = NULL;
> > + xmlNodePtr parentSnapshotsNode = NULL;
> > + vboxSnapshotXmlRetrieveRootNodeByName(rootElementVboxXML,
> > "Snapshot", &rootSnapshotNode); + if (!rootSnapshotNode) {
> > + VIR_DEBUG("Could not retrieve Snapshot node");
> > + ret = -1;
> > + goto cleanup;
> > + }
> > + vboxSnapshotXmlRetrieveSnapshotNodeByName(rootSnapshotNode,
> > parent, &parentNode); + if (!parentNode) {
> > + VIR_DEBUG("Could not retrieve Snapshot node of %s", parent);
> > + ret = -1;
> > + goto cleanup;
> > + }
> > + vboxSnapshotXmlRetrieveSnapshotsNode(parentNode->children,
> > &parentSnapshotsNode); + if (!parentSnapshotsNode) {
> > + VIR_DEBUG("Could not retrieve Snapshots node");
> > + ret = -1;
> > + goto cleanup;
> > + }
> > + if (xmlAddChild(parentSnapshotsNode, xmlCopyNode(snapshotNode,
> > 1)) == NULL) { + VIR_DEBUG("Could not add snapshotsNode node
> > to its parent"); + ret = -1;
> > + goto cleanup;
> > + } else {
> > + *snapshotNodeReturned = xmlCopyNode(rootSnapshotNode, 1);
> > + }
> > + } else {
> > + *snapshotNodeReturned = xmlCopyNode(snapshotNode, 1);
> > + }
> > + ret = 0;
> > +cleanup:
> > + return ret;
> > +}
> > +
> > +static xmlBufferPtr
> > +vboxSnapshotGenerateXmlSkeleton(char *domainUuid,
> > + char *machineName,
> > + char *snapshotUuid,
> > + char *snapshotFolder,
> > + PRInt64 lastStateChange,
> > + bool isCurrent,
> > + char *savedCurrentSnapshotUuid)
> > +{
> > + char *snapshotUuidWithBrackets = NULL;
> > + char *lastStateChangeStr = NULL;
> > + /*Let's write a new xml
> > + */
> > + xmlTextWriterPtr writer;
> > + xmlBufferPtr buf = xmlBufferCreate();
> > + xmlBufferPtr ret = NULL;
> > + int resultXml;
> > + char *uuidWithBracket = NULL;
> > +
> > + /* Create a new XmlWriter with no compression. */
> > + writer = xmlNewTextWriterMemory(buf, 0);
> > + if (writer == NULL) {
> > + VIR_DEBUG("Error creating the xml writer\n");
> > + goto cleanup;
> > + }
> > +
> > + /* Start the document with the xml default for the version,
> > + * encoding ISO 8859-1 and the default for the standalone
> > + * declaration. */
> > + resultXml = xmlTextWriterStartDocument(writer, NULL, "ISO-8859-1",
> > NULL); + if (resultXml < 0) {
> > + VIR_DEBUG("Error at xmlTextWriterStartDocument\n");
> > + goto cleanup;
> > + }
> > + /* Write a comment as child of root.
> > + * Please observe, that the input to the xmlTextWriter functions
> > + * HAS to be in UTF-8, even if the output XML is encoded
> > + * in iso-8859-1 */
> > + resultXml = xmlTextWriterWriteFormatComment(writer, "WARNING: THIS
> > IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE \n" +
> > "OVERWRITTEN AND LOST.\n" +
> > "Changes to this xml
> > configuration should be made using Virtualbox \n" +
> > "or other application using the libvirt API");
> > + if (resultXml < 0) {
> > + VIR_DEBUG("Error at xmlTextWriterWriteComment\n");
> > + goto cleanup;
> > + }
> > + xmlTextWriterWriteRaw(writer, BAD_CAST "\n"); /*Break line after
> > comment*/ +
> > + /* Start an element named "VirtualBox". Since thist is the first
> > + * element, this will be the root element of the document. */
> > + resultXml = xmlTextWriterStartElement(writer, BAD_CAST
> > "VirtualBox"); + if (resultXml < 0) {
> > + VIR_DEBUG("Error creating the xml node\n");
> > + goto cleanup;
> > + }
> > + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "version",
> > BAD_CAST "1.12-linux"); + if (resultXml < 0) {
> > + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n");
> > + goto cleanup;
> > + }
> > + /* Start an element named "Machine" as child of VirtualBox. */
> > + resultXml = xmlTextWriterStartElement(writer, BAD_CAST "Machine");
> > + if (resultXml < 0) {
> > + VIR_DEBUG("Error at xmlTextWriterStartElement\n");
> > + goto cleanup;
> > + }
> > + /* Add an attribute with name "uuid" and value "{uuid}" to Machine.
> > */ + if (virAsprintf(&uuidWithBracket, "{%s}", domainUuid) != -1) { +
> > resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "uuid",
> > BAD_CAST uuidWithBracket); + if (resultXml < 0) {
> > + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n");
> > + goto cleanup;
> > + }
> > + VIR_FREE(uuidWithBracket);
> > + }
> > +
> > + if (!machineName) {
> > + VIR_DEBUG("No machine name");
> > + goto cleanup;
> > + }
> > + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "name",
> > BAD_CAST machineName); + if (resultXml < 0) {
> > + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n");
> > + goto cleanup;
> > + }
> > + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST "OSType",
> > BAD_CAST "Other"); + if (resultXml < 0) {
> > + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n");
> > + goto cleanup;
> > + }
> > +
> > + if (!snapshotFolder) {
> > + VIR_DEBUG("No machine snapshotFolder");
> > + goto cleanup;
> > + }
> > + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST
> > "snapshotFolder", BAD_CAST snapshotFolder); + if (resultXml < 0) {
> > + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n");
> > + goto cleanup;
> > + }
> > + if (virAsprintf(&lastStateChangeStr, "%ld", lastStateChange) < 0) {
> > + VIR_DEBUG("Could not convert lastStateChange from long to
> > string"); + goto cleanup;
> > + }
> > + resultXml = xmlTextWriterWriteAttribute(writer, BAD_CAST
> > "lastStateChange", BAD_CAST lastStateChangeStr); + if (resultXml < 0)
> > {
> > + VIR_DEBUG("Error at xmlTextWriterWriteAttribute\n");
> > + goto cleanup;
> > + }
> > + /* Current Snapshot
> > + *
> > https://www.virtualbox.org/sdkref/interface_i_machine.html#ac785dbe04ecc
> > c0793d949d6940202767 + * There is always a current snapshot, except
> > when there is no snapshot (obvious) or when they have all been deleted
> > (obvious) + * or when the current snapshot is deleted and it does
> > not have a parent (not so obvious). + * This can happen only when
> > the last snapshot is deleted because there can only be one snapshot with
> > no parent, the first one. + * For the moment we always put the
> > snapshot we are defining as current snapshot. When all the snapshots are
> > redefined, you can always set + * the current snapshot to one you
> > have previously saved.
> > + */
>
> Reformat to fit in 80 columns.
>
> I'm going to have to stop here, as there is just too much vbox-centric
> code that I don't understand, and already a lot of re-occurring problems
> I've pointed out that should be fixed everywhere in all the patches
> before a rebase/resubmit.
>
> If there is someone else who understands the vbox driver (and vbox
> itself) better than me, I would encourage them to jump in when revised
> patches are posted to do a lower level review - I can still look for
> problems related to the way that libvirt internal APIs (and do some
> small extent libxml APIs) are called, but can't even *run* the code to
> test for correctness.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list