Re: [libvirt] [PATCH v4 11/20] wip: backup: Parse and output checkpoint XML

On 2/6/19 2:18 PM, Eric Blake wrote: Starting to lose some steam - seeing wip means I don't want to spend too much time on some algorithms for fear they'll change, but then again I know you're looking for feedback... This is new, right? Not a copy of... Recall earlier query in RNG file about unsigned long... Typically these are then in the *Obj... The whole relationship thing is I think overly complex and a bit difficult to follow. Having @sibling and @first_child seems to make the code even more complex. I would think you have a parent and maybe a child. If this is the "first" checkpoint, then there is no parent. I would think that means it's the root. There's just much more to be see when it comes to how these relationships play out as checkpoints are made and removed. I guess I just have a very linear model in mind, but reading the code seems to go beyond linearality. The use of hash tables just makes it easier to ensure no def->name is reused. Not having locks in the object means some parent lock is managing to make sure two checkpoint operations cannot occur at the same time from different threads (e.g. a domain object lock), but that is not 100% clear. When objectifying module to to avoid leaking *obj entries into other modules, this struct would go inside .c file w/ accessors to whatever is needed externally. I purposefully avoided domain & domain snapshot as there was just too much rework. Whether or not that can be done here - not sure yet, still reading, but figured I'd point it out at least ;-) More recently in the .h prototypes, been trying to follow the .c entry as well... where it's type function(args...); makes copy-pasta easier... Add a: VIR_DEFINE_AUTOPTR_FUNC(virDomainCheckpointDef, virDomainCheckpointDefFree); details coming... Note to self or you since I could forget by the time I find this usage... wouldn't update to current_checkpoint need a lock? What's really not clear is whether the domain object lock is in place or not when we get here. I think the domainobj's are just plain write locks and not the domainobjlist fancy rwlocks. Similar w/r/t copyright Are all of these really needed? So not a pointer, but copied object from somewhere? > + > +/* Checkpoint Def functions */ > +static void > +virDomainCheckpointDiskDefClear(virDomainCheckpointDiskDefPtr disk) > +{ > + VIR_FREE(disk->name); > + VIR_FREE(disk->bitmap); Should we clear idx, size, and type too? Depends on consumer usage which could clear and reuse, thus using something old. ... Two blank lines between functions (repeats) Two lines void virDomain* > +{ > + size_t i; > + > + if (!def) > + return; > + > + VIR_FREE(def->name); > + VIR_FREE(def->description); > + VIR_FREE(def->parent); > + for (i = 0; i < def->ndisks; i++) > + virDomainCheckpointDiskDefClear(&def->disks[i]); > + VIR_FREE(def->disks); > + virDomainDefFree(def->dom); > + VIR_FREE(def); > +static int > +virDomainCheckpointDiskDefParseXML(xmlNodePtr node, > + xmlXPathContextPtr ctxt, > + virDomainCheckpointDiskDefPtr def) > +{ > + int ret = -1; > + char *checkpoint = NULL; > + char *bitmap = NULL; Use VIR_AUTOFREE(char *) for both Size is not parsed. Restore from restart will lose it. With VIR_AUTOFREE, these 2 are unnecessary The caller does this on error anyway so it's unnecessary (since ndisks == n from parse). > + return ret; > +/* flags is bitwise-or of virDomainCheckpointParseFlags. > + * If flags does not include VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE, then > + * caps are ignored. > + */ > +static virDomainCheckpointDefPtr > +virDomainCheckpointDefParse(xmlXPathContextPtr ctxt, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags) > +{ > + virDomainCheckpointDefPtr def = NULL; > + virDomainCheckpointDefPtr ret = NULL; > + xmlNodePtr *nodes = NULL; > + size_t i; > + int n; > + char *creation = NULL; Never used. VIR_AUTOPTR(virDomainCheckpointDef) def = NULL; VIR_AUTOFREE(xmlNodePtr *) nodes = NULL; VIR_AUTOFREE(char *) tmp = NULL; Erik likes 'em at the bottom too. With @ret autofree'd, the goto cleanup is unnecessary and replaced by return NULL Use of the AUTOFREE stuff really cleans up a lot. Unnecessary w/ autofree Unnecessary w/ autofree Cleanup unnecessary, just return ret; > + > + return ret; > +virDomainCheckpointDefPtr > +virDomainCheckpointDefParseNode(xmlDocPtr xml, > + xmlNodePtr root, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags) > +{ > + xmlXPathContextPtr ctxt = NULL; > + virDomainCheckpointDefPtr def = NULL; > + > + if (!virXMLNodeNameEqual(root, "domaincheckpoint")) { > + virReportError(VIR_ERR_XML_ERROR, "%s", _("domaincheckpoint")); > + goto cleanup; Could just return NULL return NULL; > + } > + > + ctxt->node = root; > + def = virDomainCheckpointDefParse(ctxt, caps, xmlopt, flags); > + cleanup: > + xmlXPathFreeContext(ctxt); > + return def; > +virDomainCheckpointDefPtr > +virDomainCheckpointDefParseString(const char *xmlStr, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags) > +{ > + virDomainCheckpointDefPtr ret = NULL; > + xmlDocPtr xml; > + int keepBlanksDefault = xmlKeepBlanksDefault(0); > + > + if ((xml = virXMLParse(NULL, xmlStr, _("(domain_checkpoint)")))) { > + xmlKeepBlanksDefault(keepBlanksDefault); > + ret = virDomainCheckpointDefParseNode(xml, xmlDocGetRootElement(xml), > + caps, xmlopt, flags); > + xmlFreeDoc(xml); > + } > + xmlKeepBlanksDefault(keepBlanksDefault); > + > + return ret; > +/** > + * virDomainCheckpointDefAssignBitmapNames: > + * @def: checkpoint def object > + * > + * Generate default bitmap names for checkpoint targets. Returns 0 on > + * success, -1 on error. > + */ > +static int > +virDomainCheckpointDefAssignBitmapNames(virDomainCheckpointDefPtr def) > +{ > + size_t i; > + > + for (i = 0; i < def->ndisks; i++) { > + virDomainCheckpointDiskDefPtr disk = &def->disks[i]; > + > + if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP || > + disk->bitmap) > + continue; > + > + if (VIR_STRDUP(disk->bitmap, def->name) < 0) > + return -1; > + } > + > + return 0; > + > +static int > +virDomainCheckpointCompareDiskIndex(const void *a, const void *b) One line for each argument s/. /. / (one space before */) > + return diska->idx - diskb->idx; > +/* Align def->disks to def->domain. Sort the list of def->disks, > + * filling in any missing disks with appropriate default. Convert > + * paths to disk targets for uniformity. Issue an error and return -1 > + * if any def->disks[n]->name appears more than once or does not map > + * to dom->disks. */ Things start to get more complex and confusing. What doesn't make sense here is why/how things/order could be different. If this is a checkpoint in time, then wouldn't the order saved (somewhere) be the same? So what causes something to get out of order? I guess this is part of the "problem" with posting code that's been in development for a long time. I'm sure this solves some issue, but what that is, I'm not sure. I'm beginning to think splitting things up to get the basic add objects, list objects, etc. and then adding in the more complex well we need to output a list in this manner or that manner or we have discovered an issue with order as a result of some operation and this fixes it would be more prudent. Means quite a bit of patch reordering to not expose the external API's, but I think will make things easier to review and I would think easier to debug and finish up the work. I assume we'd enter here with sort of lock, right? to prevent some other unexpected action from undoing us. I'm still trying to come to grips with how def->ndisks != def->dom->ndisks. There's something about this remapping of disk->name that doesn't feel right or is ripe for something going wrong. I would think we'd want to correllate where we are/were in a previous list with now, but I guess I'm not 100% of how this is/will be used. > + > + /* Provide defaults for all remaining disks. */ > + ndisks = def->ndisks; > + if (VIR_EXPAND_N(def->disks, def->ndisks, > + def->dom->ndisks - def->ndisks) < 0) > + goto cleanup; > + > + for (i = 0; i < def->dom->ndisks; i++) { > + virDomainCheckpointDiskDefPtr disk; > + > + if (virBitmapIsBitSet(map, i)) > + continue; > + disk = &def->disks[ndisks++]; > + if (VIR_STRDUP(disk->name, def->dom->disks[i]->dst) < 0) > + goto cleanup; > + disk->idx = i; > + > + /* Don't checkpoint empty drives */ > + if (virStorageSourceIsEmpty(def->dom->disks[i]->src)) > + disk->type = VIR_DOMAIN_CHECKPOINT_TYPE_NONE; > + else > + disk->type = checkpoint_default; > + } > + > + qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), > + virDomainCheckpointCompareDiskIndex); > + > + /* Generate default bitmap names for checkpoint */ > + if (virDomainCheckpointDefAssignBitmapNames(def) < 0) > + goto cleanup; > + > + ret = 0; > + > + cleanup: > + virBitmapFree(map); > + return ret; > +static int > +virDomainCheckpointDiskDefFormat(virBufferPtr buf, > + virDomainCheckpointDiskDefPtr disk, > + unsigned int flags) > +{ > + if (!disk->name) > + return 0; > + > + virBufferEscapeString(buf, "<disk name='%s'", disk->name); > + if (disk->type) > + virBufferAsprintf(buf, " checkpoint='%s'", > + virDomainCheckpointTypeToString(disk->type)); > + if (disk->bitmap) { > + virBufferEscapeString(buf, " bitmap='%s'", disk->bitmap); > + if (flags & VIR_DOMAIN_CHECKPOINT_XML_SIZE) > + virBufferAsprintf(buf, " size='%llu'", disk->size); Recall my earlier point in RNG format w/ unsignedLong > + } > + virBufferAddLit(buf, "/>\n"); > + return 0; > + > +char * > +virDomainCheckpointDefFormat(virDomainCheckpointDefPtr def, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags, > + bool internal) > +{ > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + size_t i; > + unsigned int domflags = VIR_DOMAIN_DEF_FORMAT_INACTIVE; > + > + virCheckFlags(VIR_DOMAIN_CHECKPOINT_XML_SECURE | > + VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN | > + VIR_DOMAIN_CHECKPOINT_XML_SIZE, NULL); > + if (flags & VIR_DOMAIN_CHECKPOINT_XML_SECURE) > + domflags |= VIR_DOMAIN_DEF_FORMAT_SECURE; > + > + virBufferAddLit(&buf, "<domaincheckpoint>\n"); > + virBufferAdjustIndent(&buf, 2); > + > + virBufferEscapeString(&buf, "<name>%s</name>\n", def->name); > + if (def->description) > + virBufferEscapeString(&buf, "<description>%s</description>\n", > + def->description); > + > + if (def->parent) { > + virBufferAddLit(&buf, "<parent>\n"); > + virBufferAdjustIndent(&buf, 2); > + virBufferEscapeString(&buf, "<name>%s</name>\n", def->parent); > + virBufferAdjustIndent(&buf, -2); > + virBufferAddLit(&buf, "</parent>\n"); Interesting any reason why it's not <parent>%s</parent> - are you planning to add to parent. Guess I should have asked earlier, but it only dawns on me now seeing this. So this is only valid for ACTIVE guest and not INACTIVE parse/format? Perhaps no need for bool, but use @flags... > + > + virBufferAdjustIndent(&buf, -2); > + virBufferAddLit(&buf, "</domaincheckpoint>\n"); > + > + if (virBufferCheckError(&buf) < 0) > + return NULL; > + > + return virBufferContentAndReset(&buf); > + > + error: > + virBufferFreeAndReset(&buf); > + return NULL; Keeping the *Def logic separate from the *Obj logic is the new norm. The snapshot_conf was left alone by me when I went through the others... Anything object related should be virdomaincheckpointobj.{c,h}. That means stuff above when separated out could be added sooner in order to make sure review cycles shorter and the pile of code smaller. Then to start the object code should only need a subset of what's here. Adding in the more complex stuff later. There's probably a couple of patches worth of changes and functionality all being rolled into one here. I think it'll be easier overall to review and get accepted if a more logical (and slow) progression to the desired functionality is posted. Two lines > +{ > + virDomainCheckpointObjPtr checkpoint; > + > + if (VIR_ALLOC(checkpoint) < 0) > + return NULL; > + > + VIR_DEBUG("obj=%p", checkpoint); > + > + return checkpoint; > +static void virDomainCheckpointObjFree(virDomainCheckpointObjPtr checkpoint) > +{ > + if (!checkpoint) > + return; > + > + VIR_DEBUG("obj=%p", checkpoint); > + > + virDomainCheckpointDefFree(checkpoint->def); > + VIR_FREE(checkpoint); > +} Hmm, so not a real virObject... The domain and domain snapshot code was just too inter-related for me to attempt to modify back when I objectified other driver -> object -> def code. I think if you follow examples in virstorageobj, virsecretobj, virnodedevobj, virinterfaceobj, etc. you'll get a better mechanism than what exists in the domain and domain snapshot code for for how vir*Object can be used with hash tables. There's no comments here... most important perhaps is on success @def is consume in the object and thus cannot be free'd by the caller. virDomainCheckpointObjPtr virDomainCheckpointAssignDef(...) I think "virDomainCheckpointObjListAdd" would be what's happening here. s/ != NULL// Hopefully we can never generate two checkpoints by name in the same clocktick ;-) > + VIR_FREE(chk); > + return NULL; > + } > + > + return chk; > +/* Checkpoint Obj List functions */ > +static void > +virDomainCheckpointObjListDataFree(void *payload, > + const void *name ATTRIBUTE_UNUSED) > +{ > + virDomainCheckpointObjPtr obj = payload; > + > + virDomainCheckpointObjFree(obj); > +virDomainCheckpointObjListPtr > +virDomainCheckpointObjListNew(void) > +{ > + virDomainCheckpointObjListPtr checkpoints; > + if (VIR_ALLOC(checkpoints) < 0) > + return NULL; > + checkpoints->objs = virHashCreate(50, virDomainCheckpointObjListDataFree); > + if (!checkpoints->objs) { > + VIR_FREE(checkpoints); > + return NULL; > + } How is metaroot handled? At this point it's just an empty struct. Guess my first inclination was that it was going to be the first checkpoint added. That would mean some amount of management when it comes to that checkpoint being free'd on us. > + return checkpoints; > +void > +virDomainCheckpointObjListFree(virDomainCheckpointObjListPtr checkpoints) > +{ > + if (!checkpoints) > + return; > + virHashFree(checkpoints->objs); > + VIR_FREE(checkpoints); > +struct virDomainCheckpointNameData { > + char **const names; > + int maxnames; > + unsigned int flags; > + int count; > + bool error; > + > +static int > +virDomainCheckpointObjListCopyNames(void *payload, > + const void *name ATTRIBUTE_UNUSED, > + void *opaque) > +{ > + virDomainCheckpointObjPtr obj = payload; > + struct virDomainCheckpointNameData *data = opaque; > + > + if (data->error) > + return 0; > + /* Caller already sanitized flags. Filtering on DESCENDANTS was > + * done by choice of iteration in the caller. */ > + if ((data->flags & VIR_DOMAIN_CHECKPOINT_LIST_LEAVES) && obj->nchildren) > + return 0; > + if ((data->flags & VIR_DOMAIN_CHECKPOINT_LIST_NO_LEAVES) && !obj->nchildren) > + return 0; Filters in these callback routines have been ACL Filters (see src/conf/vir*obj*.c other than snapshot of course). > + > + if (data->names && data->count < data->maxnames && > + VIR_STRDUP(data->names[data->count], obj->def->name) < 0) { > + data->error = true; > + return 0; > + } > + data->count++; > + return 0; Maybe comments here would help answer questions below. args... I don't see checkpoints->metaroot being set anywhere yet. I'd expect a Set/Get type API for external callers. ^^^ Need to address the XXX... oh, my head, my eyes, my favorite deity. It seems we're building a mostrously complex pile here. I have to wonder whether leaves and filters are really worth all this trouble and whether/how this will be used. Well this seems to be some sort of magic... > + }> + > + if (data.error) { > + for (i = 0; i < data.count; i++) > + VIR_FREE(names[i]); > + return -1; > + } > + > + return data.count; > +static int > +virDomainCheckpointObjListNum(virDomainCheckpointObjListPtr checkpoints, > + virDomainCheckpointObjPtr from, > + unsigned int flags) > +{ FWIW: virHashSize get's nb_elems in hash table. Do we even need this anymore? Functionality is above, abeit quite hidden in the pile of if - then - else of VIR_DOMAIN_CHECKPOINT_FILTERS_LEAVES > + return virDomainCheckpointObjListGetNames(checkpoints, from, NULL, 0, > + flags); > +virDomainCheckpointObjPtr > +virDomainCheckpointFindByName(virDomainCheckpointObjListPtr checkpoints, > + const char *name) Usually have this closer to or above the *Add function and not in the midst of these functions that use Hash callbacks... Would a caller really expect this? That is obj->def == NULL? > +void virDomainCheckpointObjListRemove(virDomainCheckpointObjListPtr checkpoints, > + virDomainCheckpointObjPtr checkpoint) > +{ I think when I first read things, it felt like metaroot was the first checkpoint ever created... If that does come to pass this is where the well we're about to remove metaroot would occur so we have to replace it with it's direct child/descendant. > + virHashRemoveEntry(checkpoints->objs, checkpoint->def->name); > +int > +virDomainCheckpointForEach(virDomainCheckpointObjListPtr checkpoints, > + virHashIterator iter, > + void *data) > +{ > + return virHashForEach(checkpoints->objs, iter, data); > +/* Run iter(data) on all direct children of checkpoint, while ignoring all > + * other entries in checkpoints. Return the number of children > + * visited. No particular ordering is guaranteed. */ > +int > +virDomainCheckpointForEachChild(virDomainCheckpointObjPtr checkpoint, > + virHashIterator iter, > + void *data) > +{ > + virDomainCheckpointObjPtr child = checkpoint->first_child; > + > + while (child) { > + virDomainCheckpointObjPtr next = child->sibling; > + (iter)(child, child->def->name, data); > + child = next; > + } > + > + return checkpoint->nchildren; > +struct checkpoint_act_on_descendant { > + int number; > + virHashIterator iter; > + void *data; > + > +static int > +virDomainCheckpointActOnDescendant(void *payload, > + const void *name, > + void *data) > +{ > + virDomainCheckpointObjPtr obj = payload; > + struct checkpoint_act_on_descendant *curr = data; > + > + curr->number += 1 + virDomainCheckpointForEachDescendant(obj, > + curr->iter, > + curr->data); Does this double count with the 1 +. Having a really hard time understanding what's being done ... What's really the difference between a descendant and a child? > + (curr->iter)(payload, name, curr->data); > + return 0; > +/* Run iter(data) on all descendants of checkpoint, while ignoring all > + * other entries in checkpoints. Return the number of descendants > + * visited. No particular ordering is guaranteed. */ > +int > +virDomainCheckpointForEachDescendant(virDomainCheckpointObjPtr checkpoint, > + virHashIterator iter, > + void *data) > +{ > + struct checkpoint_act_on_descendant act; > + > + act.number = 0; > + act.iter = iter; > + act.data = data; > + virDomainCheckpointForEachChild(checkpoint, > + virDomainCheckpointActOnDescendant, &act); So many levels of indirection in multiple directions. We call ForEachChild, which calls an @iter function to act on the descendant which calls ForEachDescendant (IOW, this function) again. ForEachDescendant and ForEachChild use ActOnDescendant, but have this relationship with each other which is hard to think about without some sort of picture. Maybe I'm thinking of a much simpler model where parent <- child1 <- child2 <- child3 [etc.] One takes a checkpoint and it is the parent. Then at some point in the future another checkpoint is taken. So it's now the child1 with it's parent. When the next checkpoint is taken, it's grandparent is parent and child1 is it's parent. Getting a desendant of some "child#" I thought was a linear operation. If it's more of a tree operation then the model used by the hash table to bucket and link same hashed values into a bucket would then be more useful. > + > + return act.number; > +/* Struct and callback function used as a hash table callback; each call > + * inspects the pre-existing checkpoint->def->parent field, and adjusts > + * the checkpoint->parent field as well as the parent's child fields to > + * wire up the hierarchical relations for the given checkpoint. The error > + * indicator gets set if a parent is missing or a requested parent would > + * cause a circular parent chain. */ > +struct checkpoint_set_relation { > + virDomainCheckpointObjListPtr checkpoints; > + int err; Having a hard time picturing the use... Ah so I see where nchildren comes from... Not sure I'm clear yet on the need for sibling/first_child. This would appear to be inserting something into the middle of a list. > + return 0; > +/* Populate parent link and child count of all checkpoints, with all > + * relations starting as 0/NULL. Return 0 on success, -1 if a parent > + * is missing or if a circular relationship was requested. */ > +int > +virDomainCheckpointUpdateRelations(virDomainCheckpointObjListPtr checkpoints) > +{ > + struct checkpoint_set_relation act = { checkpoints, 0 }; > + Updating the relations for everyone? Having a hard time thinking about the usage model that would need this. If an objet is removed, the update is simple - alter the parent/child. If for some reason we're rebuilding the tree - what is that reason? What causes this to be necessary. I know, you've been at this for a long time. > + virHashForEach(checkpoints->objs, virDomainCheckpointSetRelations, &act); > + return act.err; > +/* Prepare to reparent or delete checkpoint, by removing it from its > + * current listed parent. Note that when bulk removing all children > + * of a parent, it is faster to just 0 the count rather than calling > + * this function on each child. */ > +void > +virDomainCheckpointDropParent(virDomainCheckpointObjPtr checkpoint) > +{ > + virDomainCheckpointObjPtr prev = NULL; > + virDomainCheckpointObjPtr curr = NULL; > + > + checkpoint->parent->nchildren--; Decrementing the child count before actually dropping the child? > + curr = checkpoint->parent->first_child; > + while (curr != checkpoint) { > + if (!curr) { > + VIR_WARN("inconsistent checkpoint relations"); > + return; > + } > + prev = curr; > + curr = curr->sibling; > + } > + if (prev) > + prev->sibling = checkpoint->sibling; > + else > + checkpoint->parent->first_child = checkpoint->sibling; > + checkpoint->parent = NULL; > + checkpoint->sibling = NULL; > +int > +virDomainListAllCheckpoints(virDomainCheckpointObjListPtr checkpoints, > + virDomainCheckpointObjPtr from, > + virDomainPtr dom, > + virDomainCheckpointPtr **chks, > + unsigned int flags) > +{ There's better examples than what *snapshots do in order to build/export a list of virDomainCheckpointPtr's - usually in *Export type functions. Go direct virHashSize <= 0? indent is off Other examples will show that this is done in the callback function. I know you've found it easier to use the same callback function for several different kinds of operations. I tried that too when I did the common object changes and got told/reminded during review that one function should serve 3 or 4 purposes as it makes each callback function more unnecessarily complicated especially w/r/t knowing what each caller may expect. So take that advice as something to follow for the round. > + goto cleanup; > + > + ret = count; > + *chks = list; > + > + cleanup: > + for (i = 0; i < count; i++) > + VIR_FREE(names[i]); > + VIR_FREE(names); > + if (ret < 0 && list) { > + for (i = 0; i < count; i++) > + virObjectUnref(list[i]); > + VIR_FREE(list); > + } > + return ret; > + This is like the vir*ObjList*Export* function from other code. Only a very sparse look/scan of anything below this point. John > +int> +virDomainCheckpointRedefinePrep(virDomainPtr domain, > + virDomainObjPtr vm, > + virDomainCheckpointDefPtr *defptr, > + virDomainCheckpointObjPtr *chk, > + virDomainXMLOptionPtr xmlopt, > + bool *update_current) > +{ > + virDomainCheckpointDefPtr def = *defptr; > + int ret = -1; > + char uuidstr[VIR_UUID_STRING_BUFLEN]; > + virDomainCheckpointObjPtr other; > + > + virUUIDFormat(domain->uuid, uuidstr); > + > + /* Prevent circular chains */ > + if (def->parent) { > + if (STREQ(def->name, def->parent)) { > + virReportError(VIR_ERR_INVALID_ARG, > + _("cannot set checkpoint %s as its own parent"), > + def->name); > + goto cleanup; > + } > + other = virDomainCheckpointFindByName(vm->checkpoints, def->parent); > + if (!other) { > + virReportError(VIR_ERR_INVALID_ARG, > + _("parent %s for checkpoint %s not found"), > + def->parent, def->name); > + goto cleanup; > + } > + while (other->def->parent) { > + if (STREQ(other->def->parent, def->name)) { > + virReportError(VIR_ERR_INVALID_ARG, > + _("parent %s would create cycle to %s"), > + other->def->name, def->name); > + goto cleanup; > + } > + other = virDomainCheckpointFindByName(vm->checkpoints, > + other->def->parent); > + if (!other) { > + VIR_WARN("checkpoints are inconsistent for %s", > + vm->def->name); > + break; > + } > + } > + } > + > + if (def->dom && > + memcmp(def->dom->uuid, domain->uuid, VIR_UUID_BUFLEN)) { > + virReportError(VIR_ERR_INVALID_ARG, > + _("definition for checkpoint %s must use uuid %s"), > + def->name, uuidstr); > + goto cleanup; > + } > + > + other = virDomainCheckpointFindByName(vm->checkpoints, def->name); > + if (other) { > + if (other->def->dom) { > + if (def->dom) { > + if (!virDomainDefCheckABIStability(other->def->dom, > + def->dom, xmlopt)) > + goto cleanup; > + } else { > + /* Transfer the domain def */ > + def->dom = other->def->dom; > + other->def->dom = NULL; > + } > + } > + > + if (def->dom) { > + if (virDomainCheckpointAlignDisks(def) < 0) { > + /* revert stealing of the checkpoint domain definition */ > + if (def->dom && !other->def->dom) { > + other->def->dom = def->dom; > + def->dom = NULL; > + } > + goto cleanup; > + } > + } > + > + if (other == vm->current_checkpoint) { > + *update_current = true; > + vm->current_checkpoint = NULL; > + } > + > + /* Drop and rebuild the parent relationship, but keep all > + * child relations by reusing chk. */ > + virDomainCheckpointDropParent(other); > + virDomainCheckpointDefFree(other->def); > + other->def = def; > + *defptr = NULL; > + *chk = other; > + } else if (def->dom && virDomainCheckpointAlignDisks(def) < 0) { > + goto cleanup; > + } > + > + ret = 0; > + cleanup: > + return ret; > +} > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index fa3db9266f..25fc4af450 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -1,7 +1,7 @@ > /* > * domain_conf.c: domain XML processing > * > - * Copyright (C) 2006-2016 Red Hat, Inc. > + * Copyright (C) 2006-2018 Red Hat, Inc. > * Copyright (C) 2006-2008 Daniel P. Berrange > * Copyright (c) 2015 SUSE LINUX Products GmbH, Nuernberg, Germany. > * > @@ -29,6 +29,7 @@ > #include "configmake.h" > #include "internal.h" > #include "virerror.h" > +#include "checkpoint_conf.h" > #include "datatypes.h" > #include "domain_addr.h" > #include "domain_conf.h" > @@ -3313,6 +3314,7 @@ static void virDomainObjDispose(void *obj) > (dom->privateDataFreeFunc)(dom->privateData); > > virDomainSnapshotObjListFree(dom->snapshots); > + virDomainCheckpointObjListFree(dom->checkpoints); > } > > virDomainObjPtr > @@ -3342,6 +3344,9 @@ virDomainObjNew(virDomainXMLOptionPtr xmlopt) > if (!(domain->snapshots = virDomainSnapshotObjListNew())) > goto error; > > + if (!(domain->checkpoints = virDomainCheckpointObjListNew())) > + goto error; > + > virObjectLock(domain); > virDomainObjSetState(domain, VIR_DOMAIN_SHUTOFF, > VIR_DOMAIN_SHUTOFF_UNKNOWN); > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 5e22acb059..fbe7ba2d40 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -68,6 +68,27 @@ virCapabilitiesSetHostCPU; > virCapabilitiesSetNetPrefix; > > > +# conf/checkpoint_conf.h > +virDomainCheckpointAlignDisks; > +virDomainCheckpointAssignDef; > +virDomainCheckpointDefFormat; > +virDomainCheckpointDefFree; > +virDomainCheckpointDefParseString; > +virDomainCheckpointDropParent; > +virDomainCheckpointFindByName; > +virDomainCheckpointForEach; > +virDomainCheckpointForEachChild; > +virDomainCheckpointForEachDescendant; > +virDomainCheckpointObjListFree; > +virDomainCheckpointObjListNew; > +virDomainCheckpointObjListRemove; > +virDomainCheckpointRedefinePrep; > +virDomainCheckpointTypeFromString; > +virDomainCheckpointTypeToString; > +virDomainCheckpointUpdateRelations; > +virDomainListAllCheckpoints; > + > + > # conf/cpu_conf.h > virCPUCacheModeTypeFromString; > virCPUCacheModeTypeToString; > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 9b835fa369..87a4b3632b 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -288,7 +288,7 @@ endif WITH_LIBXL > > if WITH_QEMU > test_programs += qemuxml2argvtest qemuxml2xmltest \ > - qemuargv2xmltest domainsnapshotxml2xmltest \ > + qemuargv2xmltest domaincheckpointxml2xmltest domainsnapshotxml2xmltest \ > qemumonitorjsontest qemuhotplugtest \ > qemuagenttest qemucapabilitiestest qemucaps2xmltest \ > qemumemlocktest \ > @@ -673,6 +673,11 @@ qemublocktest_LDADD = \ > $(LDADDS) \ > $(NULL) > > +domaincheckpointxml2xmltest_SOURCES = \ > + domaincheckpointxml2xmltest.c testutilsqemu.c testutilsqemu.h \ > + testutils.c testutils.h > +domaincheckpointxml2xmltest_LDADD = $(qemu_LDADDS) $(LDADDS) > + > domainsnapshotxml2xmltest_SOURCES = \ > domainsnapshotxml2xmltest.c testutilsqemu.c testutilsqemu.h \ > testutils.c testutils.h > @@ -701,7 +706,7 @@ qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS) > > else ! WITH_QEMU > EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \ > - domainsnapshotxml2xmltest.c \ > + domaincheckpointxml2xmltest.c domainsnapshotxml2xmltest.c \ > testutilsqemu.c testutilsqemu.h \ > testutilsqemuschema.c testutilsqemuschema.h \ > qemumonitorjsontest.c qemuhotplugtest.c \ > diff --git a/tests/domaincheckpointxml2xmltest.c b/tests/domaincheckpointxml2xmltest.c > new file mode 100644 > index 0000000000..5381b6352b > --- /dev/null > +++ b/tests/domaincheckpointxml2xmltest.c > @@ -0,0 +1,231 @@ > +#include <config.h> > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <unistd.h> > +#include <string.h> > + > +#include <sys/types.h> > +#include <fcntl.h> > + > +#include <regex.h> > + > +#include "testutils.h" > + > +#ifdef WITH_QEMU > + > +# include "internal.h" > +# include "qemu/qemu_conf.h" > +# include "qemu/qemu_domain.h" > +# include "checkpoint_conf.h" > +# include "testutilsqemu.h" > +# include "virstring.h" > + > +# define VIR_FROM_THIS VIR_FROM_NONE > + > +static virQEMUDriver driver; > + > +/* This regex will skip the following XML constructs in test files > + * that are dynamically generated and thus problematic to test: > + * <name>1234352345</name> if the checkpoint has no name, > + * <creationTime>23523452345</creationTime>. > + */ > +static const char *testCheckpointXMLVariableLineRegexStr = > + "<(name|creationTime)>[0-9]+</(name|creationTime)>"; > + > +regex_t *testCheckpointXMLVariableLineRegex = NULL; > + > +static char * > +testFilterXML(char *xml) > +{ > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + char **xmlLines = NULL; > + char **xmlLine; > + char *ret = NULL; > + > + if (!(xmlLines = virStringSplit(xml, "\n", 0))) { > + VIR_FREE(xml); > + goto cleanup; > + } > + VIR_FREE(xml); > + > + for (xmlLine = xmlLines; *xmlLine; xmlLine++) { > + if (regexec(testCheckpointXMLVariableLineRegex, > + *xmlLine, 0, NULL, 0) == 0) > + continue; > + > + virBufferStrcat(&buf, *xmlLine, "\n", NULL); > + } > + > + if (virBufferCheckError(&buf) < 0) > + goto cleanup; > + > + ret = virBufferContentAndReset(&buf); > + > + cleanup: > + virBufferFreeAndReset(&buf); > + virStringListFree(xmlLines); > + return ret; > +static int > +testCompareXMLToXMLFiles(const char *inxml, > + const char *outxml, > + bool internal, > + bool redefine) > +{ > + char *inXmlData = NULL; > + char *outXmlData = NULL; > + char *actual = NULL; > + int ret = -1; > + virDomainCheckpointDefPtr def = NULL; > + unsigned int flags = VIR_DOMAIN_CHECKPOINT_PARSE_DISKS; > + > + if (internal) > + flags |= VIR_DOMAIN_CHECKPOINT_PARSE_INTERNAL; > + > + if (redefine) > + flags |= VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE; > + > + if (virTestLoadFile(inxml, &inXmlData) < 0) > + goto cleanup; > + > + if (virTestLoadFile(outxml, &outXmlData) < 0) > + goto cleanup; > + > + if (!(def = virDomainCheckpointDefParseString(inXmlData, driver.caps, > + driver.xmlopt, > + flags))) > + goto cleanup; > + > + /* Parsing XML does not populate the domain definition, so add a > + * canned bare-bones fallback */ > + if (!def->dom) { > + // HACK yep. > + ret = 77; > + goto cleanup; > + const char *def_dom = "" > + "<domain type='qemu'>" > + " <name>fedora</name>" > + " <uuid>93a5c045-6457-2c09-e56c-927cdf34e178</uuid>" > +/* arch='x86_64' machine='pc'*/ > + " <os><type>hvm</type></os>" > + "</domain>"; > + int dom_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; > + if (!(def->dom = virDomainDefParseString(def_dom, driver.caps, > + driver.xmlopt, NULL, > + dom_flags))) > + goto cleanup; > + } > + > + if (!(actual = virDomainCheckpointDefFormat(def, driver.caps, > + driver.xmlopt, > + VIR_DOMAIN_DEF_FORMAT_SECURE, > + internal))) > + goto cleanup; > + > + if (!redefine) { > + if (!(actual = testFilterXML(actual))) > + goto cleanup; > + > + if (!(outXmlData = testFilterXML(outXmlData))) > + goto cleanup; > + } > + > + if (STRNEQ(outXmlData, actual)) { > + virTestDifferenceFull(stderr, outXmlData, outxml, actual, inxml); > + goto cleanup; > + } > + > + ret = 0; > + > + cleanup: > + VIR_FREE(inXmlData); > + VIR_FREE(outXmlData); > + VIR_FREE(actual); > + virDomainCheckpointDefFree(def); > + return ret; > +struct testInfo { > + const char *inxml; > + const char *outxml; > + bool internal; > + bool redefine; > + > + > +static int > +testCompareXMLToXMLHelper(const void *data) > +{ > + const struct testInfo *info = data; > + > + return testCompareXMLToXMLFiles(info->inxml, info->outxml, > + info->internal, info->redefine); > + > +static int > +mymain(void) > +{ > + int ret = 0; > + > + if (qemuTestDriverInit(&driver) < 0) > + return EXIT_FAILURE; > + > + if (VIR_ALLOC(testCheckpointXMLVariableLineRegex) < 0) > + goto cleanup; > + > + if (regcomp(testCheckpointXMLVariableLineRegex, > + testCheckpointXMLVariableLineRegexStr, > + REG_EXTENDED | REG_NOSUB) != 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + "failed to compile test regex"); > + goto cleanup; > + } > + > + > +# define DO_TEST(prefix, name, inpath, outpath, internal, redefine) \ > + do { \ > + const struct testInfo info = {abs_srcdir "/" inpath "/" name ".xml", \ > + abs_srcdir "/" outpath "/" name ".xml", \ > + internal, redefine}; \ > + if (virTestRun("CHECKPOINT XML-2-XML " prefix " " name, \ > + testCompareXMLToXMLHelper, &info) < 0) \ > + ret = -1; \ > + } while (0) > + > +# define DO_TEST_INOUT(name, internal, redefine) \ > + DO_TEST("in->out", name,\ > + "domaincheckpointxml2xmlin",\ > + "domaincheckpointxml2xmlout",\ > + internal, redefine) > + > + /* Unset or set all envvars here that are copied in qemudBuildCommandLine > + * using ADD_ENV_COPY, otherwise these tests may fail due to unexpected > + * values for these envvars */ > + setenv("PATH", "/bin", 1); > + > + DO_TEST_INOUT("empty", false, false); > + DO_TEST_INOUT("sample", false, false); > + > + cleanup: > + if (testCheckpointXMLVariableLineRegex) > + regfree(testCheckpointXMLVariableLineRegex); > + VIR_FREE(testCheckpointXMLVariableLineRegex); > + qemuTestDriverFree(&driver); > + > + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; > +VIR_TEST_MAIN(mymain) > + > +#else > + > +int > +main(void) > +{ > + return EXIT_AM_SKIP; > +#endif /* WITH_QEMU */ >