Re: [libvirt] [PATCHv4 4/5] vbox_tmpl.c: Patch for redefining snapshots

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?

> #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.

> + 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.