On 2/6/19 2:18 PM, Eric Blake wrote: > Work in progress - the checkpoint code is not quite passing > tests (part of that is figuring out the minimal XML that is > still valid as a <domain> element, or just use --no-domain flag). > > Signed-off-by: Eric Blake <eblake@xxxxxxxxxx> > --- > src/conf/checkpoint_conf.h | 150 ++++ > src/conf/domain_conf.h | 11 +- > po/POTFILES | 1 + > src/conf/Makefile.inc.am | 2 + > src/conf/checkpoint_conf.c | 1030 +++++++++++++++++++++++++++ > src/conf/domain_conf.c | 7 +- > src/libvirt_private.syms | 21 + > tests/Makefile.am | 9 +- > tests/domaincheckpointxml2xmltest.c | 231 ++++++ > 9 files changed, 1458 insertions(+), 4 deletions(-) > create mode 100644 src/conf/checkpoint_conf.h > create mode 100644 src/conf/checkpoint_conf.c > create mode 100644 tests/domaincheckpointxml2xmltest.c > 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... > diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h > new file mode 100644 > index 0000000000..994a8bd083 > --- /dev/null > +++ b/src/conf/checkpoint_conf.h > @@ -0,0 +1,150 @@ > +/* > + * checkpoint_conf.h: domain checkpoint XML processing > + * > + * Copyright (C) 2006-2019 Red Hat, Inc. > + * Copyright (C) 2006-2008 Daniel P. Berrange This is new, right? Not a copy of... > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>. > + */ > + > +#ifndef LIBVIRT_CHECKPOINT_CONF_H > +# define LIBVIRT_CHECKPOINT_CONF_H > + > +# include "internal.h" > +# include "domain_conf.h" > + > +/* Items related to checkpoint state */ > + > +typedef enum { > + VIR_DOMAIN_CHECKPOINT_TYPE_DEFAULT = 0, > + VIR_DOMAIN_CHECKPOINT_TYPE_NONE, > + VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP, > + > + VIR_DOMAIN_CHECKPOINT_TYPE_LAST > +} virDomainCheckpointType; > + > +/* Stores disk-checkpoint information */ > +typedef struct _virDomainCheckpointDiskDef virDomainCheckpointDiskDef; > +typedef virDomainCheckpointDiskDef *virDomainCheckpointDiskDefPtr; > +struct _virDomainCheckpointDiskDef { > + char *name; /* name matching the <target dev='...' of the domain */ > + int idx; /* index within checkpoint->dom->disks that matches name */ > + int type; /* virDomainCheckpointType */ > + char *bitmap; /* bitmap name, if type is bitmap */ > + unsigned long long size; /* current checkpoint size in bytes */ Recall earlier query in RNG file about unsigned long... > +}; > + > +/* Stores the complete checkpoint metadata */ > +typedef struct _virDomainCheckpointDef virDomainCheckpointDef; > +typedef virDomainCheckpointDef *virDomainCheckpointDefPtr; > +struct _virDomainCheckpointDef { > + /* Public XML. */ > + char *name; > + char *description; > + char *parent; > + long long creationTime; /* in seconds */ > + > + size_t ndisks; /* should not exceed dom->ndisks */ > + virDomainCheckpointDiskDef *disks; > + > + virDomainDefPtr dom; > + > + /* Internal use. */ > + bool current; /* At most one checkpoint in the list should have this set */ Typically these are then in the *Obj... > +}; > + > +struct _virDomainCheckpointObj { > + virDomainCheckpointDefPtr def; /* non-NULL except for metaroot */ > + > + virDomainCheckpointObjPtr parent; /* non-NULL except for metaroot, before > + virDomainCheckpointUpdateRelations, or > + after virDomainCheckpointDropParent */ > + virDomainCheckpointObjPtr sibling; /* NULL if last child of parent */ > + size_t nchildren; > + virDomainCheckpointObjPtr first_child; /* NULL if no children */ 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 ;-) > + > +virDomainCheckpointObjListPtr virDomainCheckpointObjListNew(void); > +void virDomainCheckpointObjListFree(virDomainCheckpointObjListPtr checkpoints); > + > +typedef enum { > + VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE = 1 << 0, > + VIR_DOMAIN_CHECKPOINT_PARSE_DISKS = 1 << 1, > + VIR_DOMAIN_CHECKPOINT_PARSE_INTERNAL = 1 << 2, > +} virDomainCheckpointParseFlags; > + > +virDomainCheckpointDefPtr virDomainCheckpointDefParseString(const char *xmlStr, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags); > +virDomainCheckpointDefPtr virDomainCheckpointDefParseNode(xmlDocPtr xml, > + xmlNodePtr root, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags); > +void virDomainCheckpointDefFree(virDomainCheckpointDefPtr def); > +char *virDomainCheckpointDefFormat(virDomainCheckpointDefPtr def, > + virCapsPtr caps, > + virDomainXMLOptionPtr xmlopt, > + unsigned int flags, > + bool internal); > +int virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr checkpoint); > +virDomainCheckpointObjPtr virDomainCheckpointAssignDef(virDomainCheckpointObjListPtr checkpoints, > + virDomainCheckpointDefPtr def); > + > +virDomainCheckpointObjPtr virDomainCheckpointFindByName(virDomainCheckpointObjListPtr checkpoints, > + const char *name); > +void virDomainCheckpointObjListRemove(virDomainCheckpointObjListPtr checkpoints, > + virDomainCheckpointObjPtr checkpoint); > +int virDomainCheckpointForEach(virDomainCheckpointObjListPtr checkpoints, > + virHashIterator iter, > + void *data); > +int virDomainCheckpointForEachChild(virDomainCheckpointObjPtr checkpoint, > + virHashIterator iter, > + void *data); > +int virDomainCheckpointForEachDescendant(virDomainCheckpointObjPtr checkpoint, > + virHashIterator iter, > + void *data); > +int virDomainCheckpointUpdateRelations(virDomainCheckpointObjListPtr checkpoints); > +void virDomainCheckpointDropParent(virDomainCheckpointObjPtr checkpoint); 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... > + > +# define VIR_DOMAIN_CHECKPOINT_FILTERS_METADATA \ > + (VIR_DOMAIN_CHECKPOINT_LIST_METADATA | \ > + VIR_DOMAIN_CHECKPOINT_LIST_NO_METADATA) > + > +# define VIR_DOMAIN_CHECKPOINT_FILTERS_LEAVES \ > + (VIR_DOMAIN_CHECKPOINT_LIST_LEAVES | \ > + VIR_DOMAIN_CHECKPOINT_LIST_NO_LEAVES) > + > +# define VIR_DOMAIN_CHECKPOINT_FILTERS_ALL \ > + (VIR_DOMAIN_CHECKPOINT_FILTERS_METADATA | \ > + VIR_DOMAIN_CHECKPOINT_FILTERS_LEAVES) > + > +int virDomainListAllCheckpoints(virDomainCheckpointObjListPtr checkpoints, > + virDomainCheckpointObjPtr from, > + virDomainPtr dom, > + virDomainCheckpointPtr **objs, > + unsigned int flags); > + > +int virDomainCheckpointRedefinePrep(virDomainPtr domain, > + virDomainObjPtr vm, > + virDomainCheckpointDefPtr *def, > + virDomainCheckpointObjPtr *checkpoint, > + virDomainXMLOptionPtr xmlopt, > + bool *update_current); > + Add a: VIR_DEFINE_AUTOPTR_FUNC(virDomainCheckpointDef, virDomainCheckpointDefFree); details coming... > +VIR_ENUM_DECL(virDomainCheckpoint); > + > +#endif /* LIBVIRT_CHECKPOINT_CONF_H */ > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 5db4396fd5..d31c45427e 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -1,7 +1,7 @@ > /* > * domain_conf.h: 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. > * > @@ -119,6 +119,12 @@ typedef virDomainMemballoonDef *virDomainMemballoonDefPtr; > typedef struct _virDomainNVRAMDef virDomainNVRAMDef; > typedef virDomainNVRAMDef *virDomainNVRAMDefPtr; > > +typedef struct _virDomainCheckpointObj virDomainCheckpointObj; > +typedef virDomainCheckpointObj *virDomainCheckpointObjPtr; > + > +typedef struct _virDomainCheckpointObjList virDomainCheckpointObjList; > +typedef virDomainCheckpointObjList *virDomainCheckpointObjListPtr; > + > typedef struct _virDomainSnapshotObj virDomainSnapshotObj; > typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; > > @@ -2631,6 +2637,9 @@ struct _virDomainObj { > > bool hasManagedSave; > > + virDomainCheckpointObjListPtr checkpoints; > + virDomainCheckpointObjPtr current_checkpoint; > + 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. > void *privateData; > void (*privateDataFreeFunc)(void *); > > diff --git a/po/POTFILES b/po/POTFILES > index 88af551664..57c55fb35f 100644 > --- a/po/POTFILES > +++ b/po/POTFILES > @@ -15,6 +15,7 @@ src/bhyve/bhyve_monitor.c > src/bhyve/bhyve_parse_command.c > src/bhyve/bhyve_process.c > src/conf/capabilities.c > +src/conf/checkpoint_conf.c > src/conf/cpu_conf.c > src/conf/device_conf.c > src/conf/domain_addr.c > diff --git a/src/conf/Makefile.inc.am b/src/conf/Makefile.inc.am > index 219ff350d7..c425363bde 100644 > --- a/src/conf/Makefile.inc.am > +++ b/src/conf/Makefile.inc.am > @@ -10,6 +10,8 @@ NETDEV_CONF_SOURCES = \ > DOMAIN_CONF_SOURCES = \ > conf/capabilities.c \ > conf/capabilities.h \ > + conf/checkpoint_conf.c \ > + conf/checkpoint_conf.h \ > conf/domain_addr.c \ > conf/domain_addr.h \ > conf/domain_capabilities.c \ > diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c > new file mode 100644 > index 0000000000..c0840a96b2 > --- /dev/null > +++ b/src/conf/checkpoint_conf.c > @@ -0,0 +1,1030 @@ > +/* > + * checkpoint_conf.c: domain checkpoint XML processing > + * > + * Copyright (C) 2006-2019 Red Hat, Inc. > + * Copyright (C) 2006-2008 Daniel P. Berrange Similar w/r/t copyright > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>. > + */ > + > +#include <config.h> > + > +#include <fcntl.h> > +#include <sys/stat.h> > +#include <sys/time.h> > +#include <unistd.h> > + > +#include "internal.h" > +#include "virbitmap.h" > +#include "virbuffer.h" > +#include "count-one-bits.h" > +#include "datatypes.h" > +#include "domain_conf.h" > +#include "virlog.h" > +#include "viralloc.h" > +#include "netdev_bandwidth_conf.h" > +#include "netdev_vport_profile_conf.h" > +#include "nwfilter_conf.h" > +#include "secret_conf.h" > +#include "checkpoint_conf.h" > +#include "virstoragefile.h" > +#include "viruuid.h" > +#include "virfile.h" > +#include "virerror.h" > +#include "virxml.h" > +#include "virstring.h" Are all of these really needed? > + > +#define VIR_FROM_THIS VIR_FROM_DOMAIN_CHECKPOINT > + > +VIR_LOG_INIT("conf.checkpoint_conf"); > + > +VIR_ENUM_IMPL(virDomainCheckpoint, VIR_DOMAIN_CHECKPOINT_TYPE_LAST, > + "default", "no", "bitmap"); > + > +struct _virDomainCheckpointObjList { > + /* name string -> virDomainCheckpointObj mapping > + * for O(1), lockless lookup-by-name */ > + virHashTable *objs; > + > + virDomainCheckpointObj metaroot; /* Special parent of all root checkpoints */' 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) > +void virDomainCheckpointDefFree(virDomainCheckpointDefPtr def) 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 > + xmlNodePtr saved = ctxt->node; > + > + ctxt->node = node; > + > + def->name = virXMLPropString(node, "name"); > + if (!def->name) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("missing name from disk checkpoint element")); > + goto cleanup; > + } > + > + checkpoint = virXMLPropString(node, "checkpoint"); > + if (checkpoint) { > + def->type = virDomainCheckpointTypeFromString(checkpoint); > + if (def->type <= 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unknown disk checkpoint setting '%s'"), > + checkpoint); > + goto cleanup; > + } > + } else { > + def->type = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP; > + } > + > + bitmap = virXMLPropString(node, "bitmap"); > + if (bitmap) { > + if (def->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("disk checkpoint bitmap '%s' requires " > + "type='bitmap'"), > + bitmap); > + goto cleanup; > + } > + VIR_STEAL_PTR(def->bitmap, bitmap); > + } Size is not parsed. Restore from restart will lose it. > + > + ret = 0; > + cleanup: > + ctxt->node = saved; > + > + VIR_FREE(checkpoint); > + VIR_FREE(bitmap); With VIR_AUTOFREE, these 2 are unnecessary > + if (ret < 0) > + virDomainCheckpointDiskDefClear(def); 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. > + struct timeval tv; > + int active; > + char *tmp; 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. > + > + if (VIR_ALLOC(def) < 0) > + goto cleanup; > + > + gettimeofday(&tv, NULL); > + > + def->name = virXPathString("string(./name)", ctxt); > + if (def->name == NULL) { > + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("a redefined checkpoint must have a name")); > + goto cleanup; > + } > + if (virAsprintf(&def->name, "%lld", (long long)tv.tv_sec) < 0) > + goto cleanup; > + } > + > + def->description = virXPathString("string(./description)", ctxt); > + > + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) { > + if (virXPathLongLong("string(./creationTime)", ctxt, > + &def->creationTime) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("missing creationTime from existing checkpoint")); > + goto cleanup; > + } > + > + def->parent = virXPathString("string(./parent/name)", ctxt); > + > + if ((tmp = virXPathString("string(./domain/@type)", ctxt))) { > + int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE | > + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; > + xmlNodePtr domainNode = virXPathNode("./domain", ctxt); > + > + VIR_FREE(tmp); Unnecessary w/ autofree > + if (!domainNode) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("missing domain in checkpoint")); > + goto cleanup; > + } > + def->dom = virDomainDefParseNode(ctxt->node->doc, domainNode, > + caps, xmlopt, NULL, domainflags); > + if (!def->dom) > + goto cleanup; > + } else { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("missing domain in checkpoint redefine")); > + goto cleanup; > + } > + } else { > + def->creationTime = tv.tv_sec; > + } > + > + if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) > + goto cleanup; > + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_DISKS) { > + if (n && VIR_ALLOC_N(def->disks, n) < 0) > + goto cleanup; > + def->ndisks = n; > + for (i = 0; i < def->ndisks; i++) { > + if (virDomainCheckpointDiskDefParseXML(nodes[i], ctxt, > + &def->disks[i]) < 0) > + goto cleanup; > + } > + VIR_FREE(nodes); Unnecessary w/ autofree > + } else if (n) { > + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", > + _("unable to handle disk requests in checkpoint")); > + goto cleanup; > + } > + > + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_INTERNAL) { > + if (virXPathInt("string(./active)", ctxt, &active) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Could not find 'active' element")); > + goto cleanup; > + } > + def->current = active != 0; > + } > + > + VIR_STEAL_PTR(ret, def); > + > + cleanup: > + VIR_FREE(creation); > + VIR_FREE(nodes); > + virDomainCheckpointDefFree(def); 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 > + } > + > + ctxt = xmlXPathNewContext(xml); > + if (ctxt == NULL) { > + virReportOOMError(); > + goto cleanup; 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 > +{ > + const virDomainCheckpointDiskDef *diska = a; > + const virDomainCheckpointDiskDef *diskb = b; > + > + /* Integer overflow shouldn't be a problem here. */ 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. > +int > +virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def) > +{ > + int ret = -1; > + virBitmapPtr map = NULL; > + size_t i; > + int ndisks; > + int checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_NONE; > + I assume we'd enter here with sort of lock, right? to prevent some other unexpected action from undoing us. > + if (!def->dom) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("missing domain in checkpoint")); > + goto cleanup; > + } > + > + if (def->ndisks > def->dom->ndisks) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("too many disk checkpoint requests for domain")); > + goto cleanup; > + } > + > + /* Unlikely to have a guest without disks but technically possible. */ > + if (!def->dom->ndisks) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("domain must have at least one disk to perform " > + "checkpoints")); > + goto cleanup; > + } I'm still trying to come to grips with how def->ndisks != def->dom->ndisks. > + > + /* If <disks> omitted, do bitmap on all disks; otherwise, do nothing > + * for omitted disks */ > + if (!def->ndisks) > + checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP; > + > + if (!(map = virBitmapNew(def->dom->ndisks))) > + goto cleanup; > + > + /* Double check requested disks. */ > + for (i = 0; i < def->ndisks; i++) { > + virDomainCheckpointDiskDefPtr disk = &def->disks[i]; > + int idx = virDomainDiskIndexByName(def->dom, disk->name, false); > + > + if (idx < 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("no disk named '%s'"), disk->name); > + goto cleanup; > + } > + > + if (virBitmapIsBitSet(map, idx)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("disk '%s' specified twice"), > + disk->name); > + goto cleanup; > + } > + ignore_value(virBitmapSetBit(map, idx)); > + disk->idx = idx; > + > + if (STRNEQ(disk->name, def->dom->disks[idx]->dst)) { > + VIR_FREE(disk->name); > + if (VIR_STRDUP(disk->name, def->dom->disks[idx]->dst) < 0) > + goto cleanup; > + } > + } 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. > + } > + > + virBufferAsprintf(&buf, "<creationTime>%lld</creationTime>\n", > + def->creationTime); > + > + if (def->ndisks) { > + virBufferAddLit(&buf, "<disks>\n"); > + virBufferAdjustIndent(&buf, 2); > + for (i = 0; i < def->ndisks; i++) { > + if (virDomainCheckpointDiskDefFormat(&buf, &def->disks[i], > + flags) < 0) > + goto error; > + } > + virBufferAdjustIndent(&buf, -2); > + virBufferAddLit(&buf, "</disks>\n"); > + } > + > + if (!(flags & VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN) && > + virDomainDefFormatInternal(def->dom, caps, domflags, &buf, xmlopt) < 0) > + goto error; > + > + if (internal) > + virBufferAsprintf(&buf, "<active>%d</active>\n", def->current); 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. > +/* Checkpoint Obj functions */ > +static virDomainCheckpointObjPtr virDomainCheckpointObjNew(void) 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(virDomainCheckpointObjListPtr checkpoints, > + virDomainCheckpointDefPtr def) virDomainCheckpointObjPtr virDomainCheckpointAssignDef(...) I think "virDomainCheckpointObjListAdd" would be what's happening here. > +{ > + virDomainCheckpointObjPtr chk; > + > + if (virHashLookup(checkpoints->objs, def->name) != NULL) { s/ != NULL// > + virReportError(VIR_ERR_OPERATION_INVALID, > + _("domain checkpoint %s already exists"), > + def->name); > + return NULL; > + } > + > + if (!(chk = virDomainCheckpointObjNew())) > + return NULL; > + chk->def = def; > + > + if (virHashAddEntry(checkpoints->objs, chk->def->name, chk) < 0) { 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. > +static int > +virDomainCheckpointObjListGetNames(virDomainCheckpointObjListPtr checkpoints, > + virDomainCheckpointObjPtr from, > + char **const names, int maxnames, args... > + unsigned int flags) > +{ > + struct virDomainCheckpointNameData data = { names, maxnames, flags, 0, > + false }; > + size_t i; > + > + if (!from) { > + /* LIST_ROOTS and LIST_DESCENDANTS have the same bit value, > + * but opposite semantics. Toggle here to get the correct > + * traversal on the metaroot. */ > + flags ^= VIR_DOMAIN_CHECKPOINT_LIST_ROOTS; > + from = &checkpoints->metaroot; I don't see checkpoints->metaroot being set anywhere yet. I'd expect a Set/Get type API for external callers. > + } > + > + /* We handle LIST_ROOT/LIST_DESCENDANTS directly, mask that bit > + * out to determine when we must use the filter callback. */ > + data.flags &= ~VIR_DOMAIN_CHECKPOINT_LIST_DESCENDANTS; > + > + /* If this common code is being used, we assume that all checkpoints > + * have metadata, and thus can handle METADATA up front as an > + * all-or-none filter. XXX This might not always be true, if we ^^^ Need to address the XXX... > + * add the ability to track qcow2 bitmaps without the > + * use of metadata. */ > + if ((data.flags & VIR_DOMAIN_CHECKPOINT_FILTERS_METADATA) == > + VIR_DOMAIN_CHECKPOINT_LIST_NO_METADATA) > + return 0; > + data.flags &= ~VIR_DOMAIN_CHECKPOINT_FILTERS_METADATA; > + > + /* For ease of coding the visitor, it is easier to zero each group > + * where all of the bits are set. */ > + if ((data.flags & VIR_DOMAIN_CHECKPOINT_FILTERS_LEAVES) == > + VIR_DOMAIN_CHECKPOINT_FILTERS_LEAVES) > + data.flags &= ~VIR_DOMAIN_CHECKPOINT_FILTERS_LEAVES; > + 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. > + if (flags & VIR_DOMAIN_CHECKPOINT_LIST_DESCENDANTS) { > + if (from->def) > + virDomainCheckpointForEachDescendant(from, > + virDomainCheckpointObjListCopyNames, > + &data); > + else if (names || data.flags) > + virHashForEach(checkpoints->objs, > + virDomainCheckpointObjListCopyNames, > + &data); > + else > + data.count = virHashSize(checkpoints->objs); > + } else if (names || data.flags) { > + virDomainCheckpointForEachChild(from, > + virDomainCheckpointObjListCopyNames, > + &data); > + } else { > + data.count = from->nchildren; 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... > +{ > + return name ? virHashLookup(checkpoints->objs, name) : > + &checkpoints->metaroot; 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... > +static int > +virDomainCheckpointSetRelations(void *payload, > + const void *name ATTRIBUTE_UNUSED, > + void *data)> +{ > + virDomainCheckpointObjPtr obj = payload; > + struct checkpoint_set_relation *curr = data; > + virDomainCheckpointObjPtr tmp; > + > + obj->parent = virDomainCheckpointFindByName(curr->checkpoints, > + obj->def->parent); > + if (!obj->parent) { > + curr->err = -1; > + obj->parent = &curr->checkpoints->metaroot; > + VIR_WARN("checkpoint %s lacks parent", obj->def->name); > + } else { > + tmp = obj->parent; > + while (tmp && tmp->def) { > + if (tmp == obj) { > + curr->err = -1; > + obj->parent = &curr->checkpoints->metaroot; > + VIR_WARN("checkpoint %s in circular chain", obj->def->name); > + break; > + } > + tmp = tmp->parent; > + } > + } > + obj->parent->nchildren++; > + obj->sibling = obj->parent->first_child; > + obj->parent->first_child = obj; 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. > + int count = virDomainCheckpointObjListNum(checkpoints, from, flags); Go direct virHashSize > + virDomainCheckpointPtr *list = NULL; > + char **names; > + int ret = -1; > + size_t i; > + > + if (!chks || count < 0) <= 0? > + return count; > + if (VIR_ALLOC_N(names, count) < 0 || > + VIR_ALLOC_N(list, count + 1) < 0) > + goto cleanup; > + > + if (virDomainCheckpointObjListGetNames(checkpoints, from, names, count, > + flags) < 0) indent is off > + goto cleanup; > + for (i = 0; i < count; i++) > + if ((list[i] = virGetDomainCheckpoint(dom, names[i])) == NULL) 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 */ >
