[libvirt] [PATCHv2 0/3] split snapshot conf out of domain conf

Rebased version from v1 [1], only changes are catering to other changes in domain_conf in the meantime, and (temporarily) dropping the XML changes of the original patch 4 until I have code that can actually use the XML in 0.10.1. I'm hoping these three patches still qualify for 0.10.0. [1] https://www.redhat.com/archives/libvir-list/2012-August/msg01250.html Eric Blake (3): snapshot: make virDomainSnapshotObjList opaque snapshot: split snapshot conf code into own file snapshot: rename an enum po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/domain_conf.c | 933 +------------------------------------------- src/conf/domain_conf.h | 143 +------ src/conf/snapshot_conf.c | 972 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/snapshot_conf.h | 157 ++++++++ src/esx/esx_driver.c | 1 + src/libvirt_private.syms | 5 +- src/qemu/qemu_command.c | 1 + src/qemu/qemu_domain.c | 7 +- src/qemu/qemu_domain.h | 3 +- src/qemu/qemu_driver.c | 69 ++-- src/qemu/qemu_migration.c | 2 +- src/vbox/vbox_tmpl.c | 1 + 14 files changed, 1209 insertions(+), 1089 deletions(-) create mode 100644 src/conf/snapshot_conf.c create mode 100644 src/conf/snapshot_conf.h -- 1.7.11.4

We were failing to react to allocation failure when initializing a snapshot object list. Changing things to store a pointer instead of a complete object adds one more possible point of allocation failure, but at the same time, will make it easier to react to failure now, as well as making it easier for a future patch to split all virDomainSnapshotPtr handling into a separate file, as I continue to add even more snapshot code. Luckily, there was only one client outside of domain_conf.c that was actually peeking inside the object, and a new wrapper function was easy. * src/conf/domain_conf.h (_virDomainObj): Use a pointer. (virDomainSnapshotObjListInit): Rename. (virDomainSnapshotObjListFree, virDomainSnapshotForEach): New declarations. (_virDomainSnapshotObjList): Move definitions... * src/conf/domain_conf.c: ...here. (virDomainSnapshotObjListInit, virDomainSnapshotObjListDeinit): Rename... (virDomainSnapshotObjListNew, virDomainSnapshotObjListFree): ...to these. (virDomainSnapshotForEach): New function. (virDomainObjDispose, virDomainListPopulate): Adjust callers. * src/qemu/qemu_domain.c (qemuDomainSnapshotDiscard) (qemuDomainSnapshotDiscardAllMetadata): Likewise. * src/qemu/qemu_migration.c (qemuMigrationIsAllowed): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotLoad) (qemuDomainUndefineFlags, qemuDomainSnapshotCreateXML) (qemuDomainSnapshotListNames, qemuDomainSnapshotNum) (qemuDomainListAllSnapshots) (qemuDomainSnapshotListChildrenNames) (qemuDomainSnapshotNumChildren) (qemuDomainSnapshotListAllChildren) (qemuDomainSnapshotLookupByName, qemuDomainSnapshotGetParent) (qemuDomainSnapshotGetXMLDesc, qemuDomainSnapshotIsCurrent) (qemuDomainSnapshotHasMetadata, qemuDomainRevertToSnapshot) (qemuDomainSnapshotDelete): Likewise. * src/libvirt_private.syms (domain_conf.h): Export new function. --- src/conf/domain_conf.c | 72 ++++++++++++++++++++++++++++++++--------------- src/conf/domain_conf.h | 14 ++++----- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 7 +++-- src/qemu/qemu_driver.c | 50 ++++++++++++++++---------------- src/qemu/qemu_migration.c | 2 +- 6 files changed, 87 insertions(+), 59 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 419088c..e5711d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -661,6 +661,15 @@ VIR_ENUM_IMPL(virDomainNumatuneMemPlacementMode, #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE +struct _virDomainSnapshotObjList { + /* name string -> virDomainSnapshotObj mapping + * for O(1), lockless lookup-by-name */ + virHashTable *objs; + + virDomainSnapshotObj metaroot; /* Special parent of all root snapshots */ +}; + + static virClassPtr virDomainObjClass; static void virDomainObjDispose(void *obj); @@ -1692,8 +1701,6 @@ void virDomainDefFree(virDomainDefPtr def) VIR_FREE(def); } -static void virDomainSnapshotObjListDeinit(virDomainSnapshotObjListPtr snapshots); - static void virDomainObjDispose(void *obj) { virDomainObjPtr dom = obj; @@ -1707,7 +1714,7 @@ static void virDomainObjDispose(void *obj) virMutexDestroy(&dom->lock); - virDomainSnapshotObjListDeinit(&dom->snapshots); + virDomainSnapshotObjListFree(dom->snapshots); } @@ -1721,31 +1728,33 @@ virDomainObjPtr virDomainObjNew(virCapsPtr caps) if (!(domain = virObjectNew(virDomainObjClass))) return NULL; - if (caps->privateDataAllocFunc && - !(domain->privateData = (caps->privateDataAllocFunc)())) { - virReportOOMError(); - VIR_FREE(domain); - return NULL; - } - domain->privateDataFreeFunc = caps->privateDataFreeFunc; - if (virMutexInit(&domain->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); - if (domain->privateDataFreeFunc) - (domain->privateDataFreeFunc)(domain->privateData); VIR_FREE(domain); return NULL; } + if (caps->privateDataAllocFunc && + !(domain->privateData = (caps->privateDataAllocFunc)())) { + virReportOOMError(); + goto error; + } + domain->privateDataFreeFunc = caps->privateDataFreeFunc; + + if (!(domain->snapshots = virDomainSnapshotObjListNew())) + goto error; + virDomainObjLock(domain); virDomainObjSetState(domain, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_UNKNOWN); - virDomainSnapshotObjListInit(&domain->snapshots); - VIR_DEBUG("obj=%p", domain); return domain; + +error: + virObjectUnref(domain); + return NULL; } void virDomainObjAssignDef(virDomainObjPtr domain, @@ -14685,18 +14694,29 @@ virDomainSnapshotObjListDataFree(void *payload, virDomainSnapshotObjFree(obj); } -int virDomainSnapshotObjListInit(virDomainSnapshotObjListPtr snapshots) +virDomainSnapshotObjListPtr +virDomainSnapshotObjListNew(void) { + virDomainSnapshotObjListPtr snapshots; + if (VIR_ALLOC(snapshots) < 0) { + virReportOOMError(); + return NULL; + } snapshots->objs = virHashCreate(50, virDomainSnapshotObjListDataFree); - if (!snapshots->objs) - return -1; - return 0; + if (!snapshots->objs) { + VIR_FREE(snapshots); + return NULL; + } + return snapshots; } -static void -virDomainSnapshotObjListDeinit(virDomainSnapshotObjListPtr snapshots) +void +virDomainSnapshotObjListFree(virDomainSnapshotObjListPtr snapshots) { + if (!snapshots) + return; virHashFree(snapshots->objs); + VIR_FREE(snapshots); } struct virDomainSnapshotNameData { @@ -14817,6 +14837,14 @@ void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, virHashRemoveEntry(snapshots->objs, snapshot->def->name); } +int +virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots, + virHashIterator iter, + void *data) +{ + return virHashForEach(snapshots->objs, iter, data); +} + /* Run iter(data) on all direct children of snapshot, while ignoring all * other entries in snapshots. Return the number of children * visited. No particular ordering is guaranteed. */ @@ -15738,7 +15766,7 @@ virDomainListPopulate(void *payload, /* filter by snapshot existence */ if (MATCH(VIR_CONNECT_LIST_DOMAINS_FILTERS_SNAPSHOT)) { - int nsnap = virDomainSnapshotObjListNum(&vm->snapshots, NULL, 0); + int nsnap = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0); if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT) && nsnap > 0) || (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT) && nsnap <= 0))) goto cleanup; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0c3824e..6033641 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1761,13 +1761,9 @@ struct _virDomainSnapshotObj { typedef struct _virDomainSnapshotObjList virDomainSnapshotObjList; typedef virDomainSnapshotObjList *virDomainSnapshotObjListPtr; -struct _virDomainSnapshotObjList { - /* name string -> virDomainSnapshotObj mapping - * for O(1), lockless lookup-by-name */ - virHashTable *objs; - virDomainSnapshotObj metaroot; /* Special parent of all root snapshots */ -}; +virDomainSnapshotObjListPtr virDomainSnapshotObjListNew(void); +void virDomainSnapshotObjListFree(virDomainSnapshotObjListPtr snapshots); typedef enum { VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0, @@ -1790,7 +1786,6 @@ int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot, virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, const virDomainSnapshotDefPtr def); -int virDomainSnapshotObjListInit(virDomainSnapshotObjListPtr objs); int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr from, char **const names, int maxnames, @@ -1802,6 +1797,9 @@ virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjLi const char *name); void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr snapshot); +int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots, + virHashIterator iter, + void *data); int virDomainSnapshotForEachChild(virDomainSnapshotObjPtr snapshot, virHashIterator iter, void *data); @@ -1835,7 +1833,7 @@ struct _virDomainObj { virDomainDefPtr def; /* The current definition */ virDomainDefPtr newDef; /* New definition to activate at shutdown */ - virDomainSnapshotObjList snapshots; + virDomainSnapshotObjListPtr snapshots; virDomainSnapshotObjPtr current_snapshot; bool hasManagedSave; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d91f492..c339664 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -482,6 +482,7 @@ virDomainSnapshotDefFree; virDomainSnapshotDefParseString; virDomainSnapshotDropParent; virDomainSnapshotFindByName; +virDomainSnapshotForEach; virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; virDomainSnapshotObjListGetNames; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c47890b..0ae30b7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1750,7 +1750,7 @@ qemuDomainSnapshotDiscard(struct qemud_driver *driver, if (snap == vm->current_snapshot) { if (update_current && snap->def->parent) { - parentsnap = virDomainSnapshotFindByName(&vm->snapshots, + parentsnap = virDomainSnapshotFindByName(vm->snapshots, snap->def->parent); if (!parentsnap) { VIR_WARN("missing parent snapshot matching name '%s'", @@ -1771,7 +1771,7 @@ qemuDomainSnapshotDiscard(struct qemud_driver *driver, if (unlink(snapFile) < 0) VIR_WARN("Failed to unlink %s", snapFile); - virDomainSnapshotObjListRemove(&vm->snapshots, snap); + virDomainSnapshotObjListRemove(vm->snapshots, snap); ret = 0; @@ -1808,7 +1808,8 @@ qemuDomainSnapshotDiscardAllMetadata(struct qemud_driver *driver, rem.vm = vm; rem.metadata_only = true; rem.err = 0; - virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardAll, &rem); + virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll, + &rem); return rem.err; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 773a989..c1cfa5a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -483,7 +483,7 @@ static void qemuDomainSnapshotLoad(void *payload, continue; } - snap = virDomainSnapshotAssignDef(&vm->snapshots, def); + snap = virDomainSnapshotAssignDef(vm->snapshots, def); if (snap == NULL) { virDomainSnapshotDefFree(def); } else if (snap->def->current) { @@ -502,7 +502,7 @@ static void qemuDomainSnapshotLoad(void *payload, vm->current_snapshot = NULL; } - if (virDomainSnapshotUpdateRelations(&vm->snapshots) < 0) + if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0) VIR_ERROR(_("Snapshots have inconsistent relations for domain %s"), vm->def->name); @@ -5629,7 +5629,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, } if (!virDomainObjIsActive(vm) && - (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, NULL, 0))) { + (nsnapshots = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0))) { if (!(flags & VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA)) { virReportError(VIR_ERR_OPERATION_INVALID, _("cannot delete inactive domain with %d " @@ -11102,7 +11102,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, def->name); goto cleanup; } - other = virDomainSnapshotFindByName(&vm->snapshots, def->parent); + other = virDomainSnapshotFindByName(vm->snapshots, def->parent); if (!other) { virReportError(VIR_ERR_INVALID_ARG, _("parent %s for snapshot %s not found"), @@ -11116,7 +11116,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, other->def->name, def->name); goto cleanup; } - other = virDomainSnapshotFindByName(&vm->snapshots, + other = virDomainSnapshotFindByName(vm->snapshots, other->def->parent); if (!other) { VIR_WARN("snapshots are inconsistent for %s", @@ -11134,7 +11134,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, def->name, uuidstr); goto cleanup; } - other = virDomainSnapshotFindByName(&vm->snapshots, def->name); + other = virDomainSnapshotFindByName(vm->snapshots, def->name); if (other) { if ((other->def->state == VIR_DOMAIN_RUNNING || other->def->state == VIR_DOMAIN_PAUSED) != @@ -11223,7 +11223,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (snap) snap->def = def; - else if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) + else if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) goto cleanup; def = NULL; @@ -11280,7 +11280,7 @@ cleanup: } else { if (update_current) vm->current_snapshot = snap; - other = virDomainSnapshotFindByName(&vm->snapshots, + other = virDomainSnapshotFindByName(vm->snapshots, snap->def->parent); snap->parent = other; other->nchildren++; @@ -11288,7 +11288,7 @@ cleanup: other->first_child = snap; } } else if (snap) { - virDomainSnapshotObjListRemove(&vm->snapshots, snap); + virDomainSnapshotObjListRemove(vm->snapshots, snap); } virDomainObjUnlock(vm); } @@ -11319,7 +11319,7 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, goto cleanup; } - n = virDomainSnapshotObjListGetNames(&vm->snapshots, NULL, names, nameslen, + n = virDomainSnapshotObjListGetNames(vm->snapshots, NULL, names, nameslen, flags); cleanup: @@ -11349,7 +11349,7 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, goto cleanup; } - n = virDomainSnapshotObjListNum(&vm->snapshots, NULL, flags); + n = virDomainSnapshotObjListNum(vm->snapshots, NULL, flags); cleanup: if (vm) @@ -11379,7 +11379,7 @@ qemuDomainListAllSnapshots(virDomainPtr domain, virDomainSnapshotPtr **snaps, goto cleanup; } - n = virDomainListSnapshots(&vm->snapshots, NULL, domain, snaps, flags); + n = virDomainListSnapshots(vm->snapshots, NULL, domain, snaps, flags); cleanup: if (vm) @@ -11412,7 +11412,7 @@ qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, goto cleanup; } - snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11420,7 +11420,7 @@ qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, goto cleanup; } - n = virDomainSnapshotObjListGetNames(&vm->snapshots, snap, names, nameslen, + n = virDomainSnapshotObjListGetNames(vm->snapshots, snap, names, nameslen, flags); cleanup: @@ -11452,7 +11452,7 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, goto cleanup; } - snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11460,7 +11460,7 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, goto cleanup; } - n = virDomainSnapshotObjListNum(&vm->snapshots, snap, flags); + n = virDomainSnapshotObjListNum(vm->snapshots, snap, flags); cleanup: if (vm) @@ -11492,7 +11492,7 @@ qemuDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, goto cleanup; } - snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11500,7 +11500,7 @@ qemuDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, goto cleanup; } - n = virDomainListSnapshots(&vm->snapshots, snap, snapshot->domain, snaps, + n = virDomainListSnapshots(vm->snapshots, snap, snapshot->domain, snaps, flags); cleanup: @@ -11531,7 +11531,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotLookupByName(virDomainPtr domain, goto cleanup; } - snap = virDomainSnapshotFindByName(&vm->snapshots, name); + snap = virDomainSnapshotFindByName(vm->snapshots, name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no snapshot with matching name '%s'"), name); @@ -11596,7 +11596,7 @@ qemuDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, goto cleanup; } - snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11674,7 +11674,7 @@ static char *qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, goto cleanup; } - snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11712,7 +11712,7 @@ qemuDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot, goto cleanup; } - snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11752,7 +11752,7 @@ qemuDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, goto cleanup; } - snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11825,7 +11825,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } - snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -12193,7 +12193,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto cleanup; } - snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f65c81a..1b21ef6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -807,7 +807,7 @@ qemuMigrationIsAllowed(struct qemud_driver *driver, virDomainObjPtr vm, "%s", _("domain is marked for auto destroy")); return false; } - if ((nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, NULL, + if ((nsnapshots = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0))) { virReportError(VIR_ERR_OPERATION_INVALID, _("cannot migrate domain with %d snapshots"), -- 1.7.11.4

On Thu, Aug 23, 2012 at 02:54:14PM -0600, Eric Blake wrote:
We were failing to react to allocation failure when initializing a snapshot object list. Changing things to store a pointer instead of a complete object adds one more possible point of allocation failure, but at the same time, will make it easier to react to failure now, as well as making it easier for a future patch to split all virDomainSnapshotPtr handling into a separate file, as I continue to add even more snapshot code.
Luckily, there was only one client outside of domain_conf.c that was actually peeking inside the object, and a new wrapper function was easy.
* src/conf/domain_conf.h (_virDomainObj): Use a pointer. (virDomainSnapshotObjListInit): Rename. (virDomainSnapshotObjListFree, virDomainSnapshotForEach): New declarations. (_virDomainSnapshotObjList): Move definitions... * src/conf/domain_conf.c: ...here. (virDomainSnapshotObjListInit, virDomainSnapshotObjListDeinit): Rename... (virDomainSnapshotObjListNew, virDomainSnapshotObjListFree): ...to these. (virDomainSnapshotForEach): New function. (virDomainObjDispose, virDomainListPopulate): Adjust callers. * src/qemu/qemu_domain.c (qemuDomainSnapshotDiscard) (qemuDomainSnapshotDiscardAllMetadata): Likewise. * src/qemu/qemu_migration.c (qemuMigrationIsAllowed): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotLoad) (qemuDomainUndefineFlags, qemuDomainSnapshotCreateXML) (qemuDomainSnapshotListNames, qemuDomainSnapshotNum) (qemuDomainListAllSnapshots) (qemuDomainSnapshotListChildrenNames) (qemuDomainSnapshotNumChildren) (qemuDomainSnapshotListAllChildren) (qemuDomainSnapshotLookupByName, qemuDomainSnapshotGetParent) (qemuDomainSnapshotGetXMLDesc, qemuDomainSnapshotIsCurrent) (qemuDomainSnapshotHasMetadata, qemuDomainRevertToSnapshot) (qemuDomainSnapshotDelete): Likewise. * src/libvirt_private.syms (domain_conf.h): Export new function. --- src/conf/domain_conf.c | 72 ++++++++++++++++++++++++++++++++--------------- src/conf/domain_conf.h | 14 ++++----- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 7 +++-- src/qemu/qemu_driver.c | 50 ++++++++++++++++---------------- src/qemu/qemu_migration.c | 2 +- 6 files changed, 87 insertions(+), 59 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 419088c..e5711d4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -661,6 +661,15 @@ VIR_ENUM_IMPL(virDomainNumatuneMemPlacementMode, #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE
+struct _virDomainSnapshotObjList { + /* name string -> virDomainSnapshotObj mapping + * for O(1), lockless lookup-by-name */ + virHashTable *objs; + + virDomainSnapshotObj metaroot; /* Special parent of all root snapshots */ +}; + + static virClassPtr virDomainObjClass; static void virDomainObjDispose(void *obj);
@@ -1692,8 +1701,6 @@ void virDomainDefFree(virDomainDefPtr def) VIR_FREE(def); }
-static void virDomainSnapshotObjListDeinit(virDomainSnapshotObjListPtr snapshots); - static void virDomainObjDispose(void *obj) { virDomainObjPtr dom = obj; @@ -1707,7 +1714,7 @@ static void virDomainObjDispose(void *obj)
virMutexDestroy(&dom->lock);
- virDomainSnapshotObjListDeinit(&dom->snapshots); + virDomainSnapshotObjListFree(dom->snapshots); }
@@ -1721,31 +1728,33 @@ virDomainObjPtr virDomainObjNew(virCapsPtr caps) if (!(domain = virObjectNew(virDomainObjClass))) return NULL;
- if (caps->privateDataAllocFunc && - !(domain->privateData = (caps->privateDataAllocFunc)())) { - virReportOOMError(); - VIR_FREE(domain); - return NULL; - } - domain->privateDataFreeFunc = caps->privateDataFreeFunc; - if (virMutexInit(&domain->lock) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize mutex")); - if (domain->privateDataFreeFunc) - (domain->privateDataFreeFunc)(domain->privateData); VIR_FREE(domain); return NULL; }
+ if (caps->privateDataAllocFunc && + !(domain->privateData = (caps->privateDataAllocFunc)())) { + virReportOOMError(); + goto error; + } + domain->privateDataFreeFunc = caps->privateDataFreeFunc; + + if (!(domain->snapshots = virDomainSnapshotObjListNew())) + goto error; + virDomainObjLock(domain); virDomainObjSetState(domain, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_UNKNOWN);
- virDomainSnapshotObjListInit(&domain->snapshots); - VIR_DEBUG("obj=%p", domain); return domain; + +error: + virObjectUnref(domain); + return NULL; }
void virDomainObjAssignDef(virDomainObjPtr domain, @@ -14685,18 +14694,29 @@ virDomainSnapshotObjListDataFree(void *payload, virDomainSnapshotObjFree(obj); }
-int virDomainSnapshotObjListInit(virDomainSnapshotObjListPtr snapshots) +virDomainSnapshotObjListPtr +virDomainSnapshotObjListNew(void) { + virDomainSnapshotObjListPtr snapshots; + if (VIR_ALLOC(snapshots) < 0) { + virReportOOMError(); + return NULL; + } snapshots->objs = virHashCreate(50, virDomainSnapshotObjListDataFree); - if (!snapshots->objs) - return -1; - return 0; + if (!snapshots->objs) { + VIR_FREE(snapshots); + return NULL; + } + return snapshots; }
-static void -virDomainSnapshotObjListDeinit(virDomainSnapshotObjListPtr snapshots) +void +virDomainSnapshotObjListFree(virDomainSnapshotObjListPtr snapshots) { + if (!snapshots) + return; virHashFree(snapshots->objs); + VIR_FREE(snapshots); }
struct virDomainSnapshotNameData { @@ -14817,6 +14837,14 @@ void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, virHashRemoveEntry(snapshots->objs, snapshot->def->name); }
+int +virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots, + virHashIterator iter, + void *data) +{ + return virHashForEach(snapshots->objs, iter, data); +} + /* Run iter(data) on all direct children of snapshot, while ignoring all * other entries in snapshots. Return the number of children * visited. No particular ordering is guaranteed. */ @@ -15738,7 +15766,7 @@ virDomainListPopulate(void *payload,
/* filter by snapshot existence */ if (MATCH(VIR_CONNECT_LIST_DOMAINS_FILTERS_SNAPSHOT)) { - int nsnap = virDomainSnapshotObjListNum(&vm->snapshots, NULL, 0); + int nsnap = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0); if (!((MATCH(VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT) && nsnap > 0) || (MATCH(VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT) && nsnap <= 0))) goto cleanup; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0c3824e..6033641 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1761,13 +1761,9 @@ struct _virDomainSnapshotObj {
typedef struct _virDomainSnapshotObjList virDomainSnapshotObjList; typedef virDomainSnapshotObjList *virDomainSnapshotObjListPtr; -struct _virDomainSnapshotObjList { - /* name string -> virDomainSnapshotObj mapping - * for O(1), lockless lookup-by-name */ - virHashTable *objs;
- virDomainSnapshotObj metaroot; /* Special parent of all root snapshots */ -}; +virDomainSnapshotObjListPtr virDomainSnapshotObjListNew(void); +void virDomainSnapshotObjListFree(virDomainSnapshotObjListPtr snapshots);
typedef enum { VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0, @@ -1790,7 +1786,6 @@ int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot, virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, const virDomainSnapshotDefPtr def);
-int virDomainSnapshotObjListInit(virDomainSnapshotObjListPtr objs); int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr from, char **const names, int maxnames, @@ -1802,6 +1797,9 @@ virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjLi const char *name); void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr snapshot); +int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots, + virHashIterator iter, + void *data); int virDomainSnapshotForEachChild(virDomainSnapshotObjPtr snapshot, virHashIterator iter, void *data); @@ -1835,7 +1833,7 @@ struct _virDomainObj { virDomainDefPtr def; /* The current definition */ virDomainDefPtr newDef; /* New definition to activate at shutdown */
- virDomainSnapshotObjList snapshots; + virDomainSnapshotObjListPtr snapshots; virDomainSnapshotObjPtr current_snapshot;
bool hasManagedSave; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d91f492..c339664 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -482,6 +482,7 @@ virDomainSnapshotDefFree; virDomainSnapshotDefParseString; virDomainSnapshotDropParent; virDomainSnapshotFindByName; +virDomainSnapshotForEach; virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; virDomainSnapshotObjListGetNames; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c47890b..0ae30b7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1750,7 +1750,7 @@ qemuDomainSnapshotDiscard(struct qemud_driver *driver,
if (snap == vm->current_snapshot) { if (update_current && snap->def->parent) { - parentsnap = virDomainSnapshotFindByName(&vm->snapshots, + parentsnap = virDomainSnapshotFindByName(vm->snapshots, snap->def->parent); if (!parentsnap) { VIR_WARN("missing parent snapshot matching name '%s'", @@ -1771,7 +1771,7 @@ qemuDomainSnapshotDiscard(struct qemud_driver *driver,
if (unlink(snapFile) < 0) VIR_WARN("Failed to unlink %s", snapFile); - virDomainSnapshotObjListRemove(&vm->snapshots, snap); + virDomainSnapshotObjListRemove(vm->snapshots, snap);
ret = 0;
@@ -1808,7 +1808,8 @@ qemuDomainSnapshotDiscardAllMetadata(struct qemud_driver *driver, rem.vm = vm; rem.metadata_only = true; rem.err = 0; - virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardAll, &rem); + virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll, + &rem);
return rem.err; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 773a989..c1cfa5a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -483,7 +483,7 @@ static void qemuDomainSnapshotLoad(void *payload, continue; }
- snap = virDomainSnapshotAssignDef(&vm->snapshots, def); + snap = virDomainSnapshotAssignDef(vm->snapshots, def); if (snap == NULL) { virDomainSnapshotDefFree(def); } else if (snap->def->current) { @@ -502,7 +502,7 @@ static void qemuDomainSnapshotLoad(void *payload, vm->current_snapshot = NULL; }
- if (virDomainSnapshotUpdateRelations(&vm->snapshots) < 0) + if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0) VIR_ERROR(_("Snapshots have inconsistent relations for domain %s"), vm->def->name);
@@ -5629,7 +5629,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, }
if (!virDomainObjIsActive(vm) && - (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, NULL, 0))) { + (nsnapshots = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0))) { if (!(flags & VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA)) { virReportError(VIR_ERR_OPERATION_INVALID, _("cannot delete inactive domain with %d " @@ -11102,7 +11102,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, def->name); goto cleanup; } - other = virDomainSnapshotFindByName(&vm->snapshots, def->parent); + other = virDomainSnapshotFindByName(vm->snapshots, def->parent); if (!other) { virReportError(VIR_ERR_INVALID_ARG, _("parent %s for snapshot %s not found"), @@ -11116,7 +11116,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, other->def->name, def->name); goto cleanup; } - other = virDomainSnapshotFindByName(&vm->snapshots, + other = virDomainSnapshotFindByName(vm->snapshots, other->def->parent); if (!other) { VIR_WARN("snapshots are inconsistent for %s", @@ -11134,7 +11134,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, def->name, uuidstr); goto cleanup; } - other = virDomainSnapshotFindByName(&vm->snapshots, def->name); + other = virDomainSnapshotFindByName(vm->snapshots, def->name); if (other) { if ((other->def->state == VIR_DOMAIN_RUNNING || other->def->state == VIR_DOMAIN_PAUSED) != @@ -11223,7 +11223,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
if (snap) snap->def = def; - else if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) + else if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) goto cleanup; def = NULL;
@@ -11280,7 +11280,7 @@ cleanup: } else { if (update_current) vm->current_snapshot = snap; - other = virDomainSnapshotFindByName(&vm->snapshots, + other = virDomainSnapshotFindByName(vm->snapshots, snap->def->parent); snap->parent = other; other->nchildren++; @@ -11288,7 +11288,7 @@ cleanup: other->first_child = snap; } } else if (snap) { - virDomainSnapshotObjListRemove(&vm->snapshots, snap); + virDomainSnapshotObjListRemove(vm->snapshots, snap); } virDomainObjUnlock(vm); } @@ -11319,7 +11319,7 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, goto cleanup; }
- n = virDomainSnapshotObjListGetNames(&vm->snapshots, NULL, names, nameslen, + n = virDomainSnapshotObjListGetNames(vm->snapshots, NULL, names, nameslen, flags);
cleanup: @@ -11349,7 +11349,7 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, goto cleanup; }
- n = virDomainSnapshotObjListNum(&vm->snapshots, NULL, flags); + n = virDomainSnapshotObjListNum(vm->snapshots, NULL, flags);
cleanup: if (vm) @@ -11379,7 +11379,7 @@ qemuDomainListAllSnapshots(virDomainPtr domain, virDomainSnapshotPtr **snaps, goto cleanup; }
- n = virDomainListSnapshots(&vm->snapshots, NULL, domain, snaps, flags); + n = virDomainListSnapshots(vm->snapshots, NULL, domain, snaps, flags);
cleanup: if (vm) @@ -11412,7 +11412,7 @@ qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, goto cleanup; }
- snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11420,7 +11420,7 @@ qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, goto cleanup; }
- n = virDomainSnapshotObjListGetNames(&vm->snapshots, snap, names, nameslen, + n = virDomainSnapshotObjListGetNames(vm->snapshots, snap, names, nameslen, flags);
cleanup: @@ -11452,7 +11452,7 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, goto cleanup; }
- snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11460,7 +11460,7 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, goto cleanup; }
- n = virDomainSnapshotObjListNum(&vm->snapshots, snap, flags); + n = virDomainSnapshotObjListNum(vm->snapshots, snap, flags);
cleanup: if (vm) @@ -11492,7 +11492,7 @@ qemuDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, goto cleanup; }
- snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11500,7 +11500,7 @@ qemuDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, goto cleanup; }
- n = virDomainListSnapshots(&vm->snapshots, snap, snapshot->domain, snaps, + n = virDomainListSnapshots(vm->snapshots, snap, snapshot->domain, snaps, flags);
cleanup: @@ -11531,7 +11531,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotLookupByName(virDomainPtr domain, goto cleanup; }
- snap = virDomainSnapshotFindByName(&vm->snapshots, name); + snap = virDomainSnapshotFindByName(vm->snapshots, name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no snapshot with matching name '%s'"), name); @@ -11596,7 +11596,7 @@ qemuDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, goto cleanup; }
- snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11674,7 +11674,7 @@ static char *qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, goto cleanup; }
- snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11712,7 +11712,7 @@ qemuDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot, goto cleanup; }
- snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11752,7 +11752,7 @@ qemuDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, goto cleanup; }
- snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -11825,7 +11825,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; }
- snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), @@ -12193,7 +12193,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto cleanup; }
- snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + snap = virDomainSnapshotFindByName(vm->snapshots, snapshot->name); if (!snap) { virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, _("no domain snapshot with matching name '%s'"), diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f65c81a..1b21ef6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -807,7 +807,7 @@ qemuMigrationIsAllowed(struct qemud_driver *driver, virDomainObjPtr vm, "%s", _("domain is marked for auto destroy")); return false; } - if ((nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, NULL, + if ((nsnapshots = virDomainSnapshotObjListNum(vm->snapshots, NULL, 0))) { virReportError(VIR_ERR_OPERATION_INVALID, _("cannot migrate domain with %d snapshots"), -- 1.7.11.4
ACK, that extra level of indirection makes things a bit cleaner IMHO Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

This has several benefits: 1. Future snapshot-related code has a definite place to go (and I _will_ be adding some) 2. Snapshot errors now use the VIR_FROM_DOMAIN_SNAPSHOT error classification, which has been underutilized (previously only in libvirt.c) * src/conf/domain_conf.h, domain_conf.c: Split... * src/conf/snapshot_conf.h, snapshot_conf.c: ...into new files. * src/Makefile.am (DOMAIN_CONF_SOURCES): Build new files. * po/POTFILES.in: Mark new file for translation. * src/vbox/vbox_tmpl.c: Update caller. * src/esx/esx_driver.c: Likewise. * src/qemu/qemu_command.c: Likewise. * src/qemu/qemu_domain.h: Likewise. --- po/POTFILES.in | 1 + src/Makefile.am | 3 +- src/conf/domain_conf.c | 921 +------------------------------------------- src/conf/domain_conf.h | 139 +------ src/conf/snapshot_conf.c | 970 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/snapshot_conf.h | 157 ++++++++ src/esx/esx_driver.c | 1 + src/qemu/qemu_command.c | 1 + src/qemu/qemu_domain.h | 3 +- src/vbox/vbox_tmpl.c | 1 + 10 files changed, 1143 insertions(+), 1054 deletions(-) create mode 100644 src/conf/snapshot_conf.c create mode 100644 src/conf/snapshot_conf.h diff --git a/po/POTFILES.in b/po/POTFILES.in index 6c30116..7a91eb4 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -19,6 +19,7 @@ src/conf/node_device_conf.c src/conf/nwfilter_conf.c src/conf/nwfilter_params.c src/conf/secret_conf.c +src/conf/snapshot_conf.c src/conf/storage_conf.c src/conf/storage_encryption_conf.c src/conf/virconsole.c diff --git a/src/Makefile.am b/src/Makefile.am index 2074571..995e032 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -153,7 +153,8 @@ DOMAIN_CONF_SOURCES = \ conf/capabilities.c conf/capabilities.h \ conf/domain_conf.c conf/domain_conf.h \ conf/domain_audit.c conf/domain_audit.h \ - conf/domain_nwfilter.c conf/domain_nwfilter.h + conf/domain_nwfilter.c conf/domain_nwfilter.h \ + conf/snapshot_conf.c conf/snapshot_conf.h DOMAIN_EVENT_SOURCES = \ conf/domain_event.c conf/domain_event.h diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e5711d4..e3d04a7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -35,6 +35,7 @@ #include "virterror_internal.h" #include "datatypes.h" #include "domain_conf.h" +#include "snapshot_conf.h" #include "memory.h" #include "verify.h" #include "xml.h" @@ -226,12 +227,6 @@ VIR_ENUM_IMPL(virDomainDiskCopyOnRead, VIR_DOMAIN_DISK_COPY_ON_READ_LAST, "on", "off") -VIR_ENUM_IMPL(virDomainDiskSnapshot, VIR_DOMAIN_DISK_SNAPSHOT_LAST, - "default", - "no", - "internal", - "external") - VIR_ENUM_IMPL(virDomainController, VIR_DOMAIN_CONTROLLER_TYPE_LAST, "ide", "fdc", @@ -528,18 +523,6 @@ VIR_ENUM_IMPL(virDomainState, VIR_DOMAIN_LAST, "crashed", "pmsuspended") -/* virDomainSnapshotState is really virDomainState plus one extra state */ -VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_SNAPSHOT_STATE_LAST, - "nostate", - "running", - "blocked", - "paused", - "shutdown", - "shutoff", - "crashed", - "pmsuspended", - "disk-snapshot") - #define VIR_DOMAIN_NOSTATE_LAST (VIR_DOMAIN_NOSTATE_UNKNOWN + 1) VIR_ENUM_IMPL(virDomainNostateReason, VIR_DOMAIN_NOSTATE_LAST, "unknown") @@ -661,15 +644,6 @@ VIR_ENUM_IMPL(virDomainNumatuneMemPlacementMode, #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE -struct _virDomainSnapshotObjList { - /* name string -> virDomainSnapshotObj mapping - * for O(1), lockless lookup-by-name */ - virHashTable *objs; - - virDomainSnapshotObj metaroot; /* Special parent of all root snapshots */ -}; - - static virClassPtr virDomainObjClass; static void virDomainObjDispose(void *obj); @@ -14136,856 +14110,6 @@ cleanup: return -1; } -/* Snapshot Def functions */ -static void -virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) -{ - VIR_FREE(disk->name); - VIR_FREE(disk->file); - VIR_FREE(disk->driverType); -} - -void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) -{ - int i; - - if (!def) - return; - - VIR_FREE(def->name); - VIR_FREE(def->description); - VIR_FREE(def->parent); - for (i = 0; i < def->ndisks; i++) - virDomainSnapshotDiskDefClear(&def->disks[i]); - VIR_FREE(def->disks); - virDomainDefFree(def->dom); - VIR_FREE(def); -} - -static int -virDomainSnapshotDiskDefParseXML(xmlNodePtr node, - virDomainSnapshotDiskDefPtr def) -{ - int ret = -1; - char *snapshot = NULL; - xmlNodePtr cur; - - def->name = virXMLPropString(node, "name"); - if (!def->name) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("missing name from disk snapshot element")); - goto cleanup; - } - - snapshot = virXMLPropString(node, "snapshot"); - if (snapshot) { - def->snapshot = virDomainDiskSnapshotTypeFromString(snapshot); - if (def->snapshot <= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unknown disk snapshot setting '%s'"), - snapshot); - goto cleanup; - } - } - - cur = node->children; - while (cur) { - if (cur->type == XML_ELEMENT_NODE) { - if (!def->file && - xmlStrEqual(cur->name, BAD_CAST "source")) { - def->file = virXMLPropString(cur, "file"); - } else if (!def->driverType && - xmlStrEqual(cur->name, BAD_CAST "driver")) { - def->driverType = virXMLPropString(cur, "type"); - } - } - cur = cur->next; - } - - if (!def->snapshot && (def->file || def->driverType)) - def->snapshot = VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL; - - ret = 0; -cleanup: - VIR_FREE(snapshot); - if (ret < 0) - virDomainSnapshotDiskDefClear(def); - return ret; -} - -/* flags is bitwise-or of virDomainSnapshotParseFlags. - * If flags does not include VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE, then - * caps and expectedVirtTypes are ignored. - */ -virDomainSnapshotDefPtr -virDomainSnapshotDefParseString(const char *xmlStr, - virCapsPtr caps, - unsigned int expectedVirtTypes, - unsigned int flags) -{ - xmlXPathContextPtr ctxt = NULL; - xmlDocPtr xml = NULL; - virDomainSnapshotDefPtr def = NULL; - virDomainSnapshotDefPtr ret = NULL; - xmlNodePtr *nodes = NULL; - int i; - char *creation = NULL, *state = NULL; - struct timeval tv; - int active; - char *tmp; - int keepBlanksDefault = xmlKeepBlanksDefault(0); - - xml = virXMLParseCtxt(NULL, xmlStr, _("(domain_snapshot)"), &ctxt); - if (!xml) { - xmlKeepBlanksDefault(keepBlanksDefault); - return NULL; - } - xmlKeepBlanksDefault(keepBlanksDefault); - - if (VIR_ALLOC(def) < 0) { - virReportOOMError(); - goto cleanup; - } - - if (!xmlStrEqual(ctxt->node->name, BAD_CAST "domainsnapshot")) { - virReportError(VIR_ERR_XML_ERROR, "%s", _("domainsnapshot")); - goto cleanup; - } - - gettimeofday(&tv, NULL); - - def->name = virXPathString("string(./name)", ctxt); - if (def->name == NULL) { - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("a redefined snapshot must have a name")); - goto cleanup; - } else { - ignore_value(virAsprintf(&def->name, "%lld", - (long long)tv.tv_sec)); - } - } - - if (def->name == NULL) { - virReportOOMError(); - goto cleanup; - } - - def->description = virXPathString("string(./description)", ctxt); - - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { - if (virXPathLongLong("string(./creationTime)", ctxt, - &def->creationTime) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("missing creationTime from existing snapshot")); - goto cleanup; - } - - def->parent = virXPathString("string(./parent/name)", ctxt); - - state = virXPathString("string(./state)", ctxt); - if (state == NULL) { - /* there was no state in an existing snapshot; this - * should never happen - */ - virReportError(VIR_ERR_XML_ERROR, "%s", - _("missing state from existing snapshot")); - goto cleanup; - } - def->state = virDomainSnapshotStateTypeFromString(state); - if (def->state < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Invalid state '%s' in domain snapshot XML"), - state); - goto cleanup; - } - - /* Older snapshots were created with just <domain>/<uuid>, and - * lack domain/@type. In that case, leave dom NULL, and - * clients will have to decide between best effort - * initialization or outright failure. */ - if ((tmp = virXPathString("string(./domain/@type)", ctxt))) { - xmlNodePtr domainNode = virXPathNode("./domain", ctxt); - - VIR_FREE(tmp); - if (!domainNode) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("missing domain in snapshot")); - goto cleanup; - } - def->dom = virDomainDefParseNode(caps, xml, domainNode, - expectedVirtTypes, - (VIR_DOMAIN_XML_INACTIVE | - VIR_DOMAIN_XML_SECURE)); - if (!def->dom) - goto cleanup; - } else { - VIR_WARN("parsing older snapshot that lacks domain"); - } - } else { - def->creationTime = tv.tv_sec; - } - - if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) - goto cleanup; - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS || - (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE && - def->state == VIR_DOMAIN_DISK_SNAPSHOT)) { - def->ndisks = i; - if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) { - virReportOOMError(); - goto cleanup; - } - for (i = 0; i < def->ndisks; i++) { - if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0) - goto cleanup; - } - VIR_FREE(nodes); - } else if (i) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("unable to handle disk requests in snapshot")); - goto cleanup; - } - - if (flags & VIR_DOMAIN_SNAPSHOT_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; - } - - ret = def; - -cleanup: - VIR_FREE(creation); - VIR_FREE(state); - VIR_FREE(nodes); - xmlXPathFreeContext(ctxt); - if (ret == NULL) - virDomainSnapshotDefFree(def); - xmlFreeDoc(xml); - - return ret; -} - -static int -disksorter(const void *a, const void *b) -{ - const virDomainSnapshotDiskDef *diska = a; - const virDomainSnapshotDiskDef *diskb = b; - - /* Integer overflow shouldn't be a problem here. */ - return diska->index - diskb->index; -} - -/* Align def->disks to def->domain. Sort the list of def->disks, - * filling in any missing disks or snapshot state defaults given by - * the domain, with a fallback to a passed in 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. If require_match, also require that existing - * def->disks snapshot states do not override explicit def->dom - * settings. */ -int -virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, - int default_snapshot, - bool require_match) -{ - int ret = -1; - virBitmapPtr map = NULL; - int i; - int ndisks; - bool inuse; - - if (!def->dom) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("missing domain in snapshot")); - goto cleanup; - } - - if (def->ndisks > def->dom->ndisks) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("too many disk snapshot requests for domain")); - goto cleanup; - } - - /* Unlikely to have a guest without disks but technically possible. */ - if (!def->dom->ndisks) { - ret = 0; - goto cleanup; - } - - if (!(map = virBitmapAlloc(def->dom->ndisks))) { - virReportOOMError(); - goto cleanup; - } - - /* Double check requested disks. */ - for (i = 0; i < def->ndisks; i++) { - virDomainSnapshotDiskDefPtr disk = &def->disks[i]; - int idx = virDomainDiskIndexByName(def->dom, disk->name, false); - int disk_snapshot; - - if (idx < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("no disk named '%s'"), disk->name); - goto cleanup; - } - disk_snapshot = def->dom->disks[idx]->snapshot; - - if (virBitmapGetBit(map, idx, &inuse) < 0 || inuse) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("disk '%s' specified twice"), - disk->name); - goto cleanup; - } - ignore_value(virBitmapSetBit(map, idx)); - disk->index = idx; - if (!disk_snapshot) - disk_snapshot = default_snapshot; - if (!disk->snapshot) { - disk->snapshot = disk_snapshot; - } else if (disk_snapshot && require_match && - disk->snapshot != disk_snapshot) { - const char *tmp = virDomainDiskSnapshotTypeToString(disk_snapshot); - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("disk '%s' must use snapshot mode '%s'"), - disk->name, tmp); - goto cleanup; - } - if (disk->file && - disk->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("file '%s' for disk '%s' requires " - "use of external snapshot mode"), - disk->file, disk->name); - goto cleanup; - } - if (STRNEQ(disk->name, def->dom->disks[idx]->dst)) { - VIR_FREE(disk->name); - if (!(disk->name = strdup(def->dom->disks[idx]->dst))) { - virReportOOMError(); - goto cleanup; - } - } - } - - /* Provide defaults for all remaining disks. */ - ndisks = def->ndisks; - if (VIR_EXPAND_N(def->disks, def->ndisks, - def->dom->ndisks - def->ndisks) < 0) { - virReportOOMError(); - goto cleanup; - } - - for (i = 0; i < def->dom->ndisks; i++) { - virDomainSnapshotDiskDefPtr disk; - - ignore_value(virBitmapGetBit(map, i, &inuse)); - if (inuse) - continue; - disk = &def->disks[ndisks++]; - if (!(disk->name = strdup(def->dom->disks[i]->dst))) { - virReportOOMError(); - goto cleanup; - } - disk->index = i; - disk->snapshot = def->dom->disks[i]->snapshot; - if (!disk->snapshot) - disk->snapshot = default_snapshot; - } - - qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), disksorter); - - /* Generate any default external file names, but only if the - * backing file is a regular file. */ - for (i = 0; i < def->ndisks; i++) { - virDomainSnapshotDiskDefPtr disk = &def->disks[i]; - - if (disk->snapshot == VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL && - !disk->file) { - const char *original = def->dom->disks[i]->src; - const char *tmp; - struct stat sb; - - if (!original) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("cannot generate external snapshot name " - "for disk '%s' without source"), - disk->name); - goto cleanup; - } - if (stat(original, &sb) < 0 || !S_ISREG(sb.st_mode)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("source for disk '%s' is not a regular " - "file; refusing to generate external " - "snapshot name"), - disk->name); - goto cleanup; - } - - tmp = strrchr(original, '.'); - if (!tmp || strchr(tmp, '/')) { - ignore_value(virAsprintf(&disk->file, "%s.%s", - original, def->name)); - } else { - if ((tmp - original) > INT_MAX) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("integer overflow")); - goto cleanup; - } - ignore_value(virAsprintf(&disk->file, "%.*s.%s", - (int) (tmp - original), original, - def->name)); - } - if (!disk->file) { - virReportOOMError(); - goto cleanup; - } - } - } - - ret = 0; - -cleanup: - virBitmapFree(map); - return ret; -} - -char *virDomainSnapshotDefFormat(const char *domain_uuid, - virDomainSnapshotDefPtr def, - unsigned int flags, - int internal) -{ - virBuffer buf = VIR_BUFFER_INITIALIZER; - int i; - - virCheckFlags(VIR_DOMAIN_XML_SECURE | - VIR_DOMAIN_XML_UPDATE_CPU, NULL); - - flags |= VIR_DOMAIN_XML_INACTIVE; - - virBufferAddLit(&buf, "<domainsnapshot>\n"); - virBufferEscapeString(&buf, " <name>%s</name>\n", def->name); - if (def->description) - virBufferEscapeString(&buf, " <description>%s</description>\n", - def->description); - virBufferAsprintf(&buf, " <state>%s</state>\n", - virDomainSnapshotStateTypeToString(def->state)); - if (def->parent) { - virBufferAddLit(&buf, " <parent>\n"); - virBufferEscapeString(&buf, " <name>%s</name>\n", def->parent); - virBufferAddLit(&buf, " </parent>\n"); - } - virBufferAsprintf(&buf, " <creationTime>%lld</creationTime>\n", - def->creationTime); - /* For now, only output <disks> on disk-snapshot */ - if (def->state == VIR_DOMAIN_DISK_SNAPSHOT) { - virBufferAddLit(&buf, " <disks>\n"); - for (i = 0; i < def->ndisks; i++) { - virDomainSnapshotDiskDefPtr disk = &def->disks[i]; - - if (!disk->name) - continue; - - virBufferEscapeString(&buf, " <disk name='%s'", disk->name); - if (disk->snapshot) - virBufferAsprintf(&buf, " snapshot='%s'", - virDomainDiskSnapshotTypeToString(disk->snapshot)); - if (disk->file || disk->driverType) { - virBufferAddLit(&buf, ">\n"); - if (disk->driverType) - virBufferEscapeString(&buf, " <driver type='%s'/>\n", - disk->driverType); - if (disk->file) - virBufferEscapeString(&buf, " <source file='%s'/>\n", - disk->file); - virBufferAddLit(&buf, " </disk>\n"); - } else { - virBufferAddLit(&buf, "/>\n"); - } - } - virBufferAddLit(&buf, " </disks>\n"); - } - if (def->dom) { - virBufferAdjustIndent(&buf, 2); - if (virDomainDefFormatInternal(def->dom, flags, &buf) < 0) { - virBufferFreeAndReset(&buf); - return NULL; - } - virBufferAdjustIndent(&buf, -2); - } else if (domain_uuid) { - virBufferAddLit(&buf, " <domain>\n"); - virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); - virBufferAddLit(&buf, " </domain>\n"); - } - if (internal) - virBufferAsprintf(&buf, " <active>%d</active>\n", def->current); - virBufferAddLit(&buf, "</domainsnapshot>\n"); - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); - return NULL; - } - - return virBufferContentAndReset(&buf); -} - -/* Snapshot Obj functions */ -static virDomainSnapshotObjPtr virDomainSnapshotObjNew(void) -{ - virDomainSnapshotObjPtr snapshot; - - if (VIR_ALLOC(snapshot) < 0) { - virReportOOMError(); - return NULL; - } - - VIR_DEBUG("obj=%p", snapshot); - - return snapshot; -} - -static void virDomainSnapshotObjFree(virDomainSnapshotObjPtr snapshot) -{ - if (!snapshot) - return; - - VIR_DEBUG("obj=%p", snapshot); - - virDomainSnapshotDefFree(snapshot->def); - VIR_FREE(snapshot); -} - -virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, - const virDomainSnapshotDefPtr def) -{ - virDomainSnapshotObjPtr snap; - - if (virHashLookup(snapshots->objs, def->name) != NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected domain snapshot %s already exists"), - def->name); - return NULL; - } - - if (!(snap = virDomainSnapshotObjNew())) - return NULL; - snap->def = def; - - if (virHashAddEntry(snapshots->objs, snap->def->name, snap) < 0) { - VIR_FREE(snap); - return NULL; - } - - return snap; -} - -/* Snapshot Obj List functions */ -static void -virDomainSnapshotObjListDataFree(void *payload, - const void *name ATTRIBUTE_UNUSED) -{ - virDomainSnapshotObjPtr obj = payload; - - virDomainSnapshotObjFree(obj); -} - -virDomainSnapshotObjListPtr -virDomainSnapshotObjListNew(void) -{ - virDomainSnapshotObjListPtr snapshots; - if (VIR_ALLOC(snapshots) < 0) { - virReportOOMError(); - return NULL; - } - snapshots->objs = virHashCreate(50, virDomainSnapshotObjListDataFree); - if (!snapshots->objs) { - VIR_FREE(snapshots); - return NULL; - } - return snapshots; -} - -void -virDomainSnapshotObjListFree(virDomainSnapshotObjListPtr snapshots) -{ - if (!snapshots) - return; - virHashFree(snapshots->objs); - VIR_FREE(snapshots); -} - -struct virDomainSnapshotNameData { - char **const names; - int maxnames; - unsigned int flags; - int count; - bool error; -}; - -static void virDomainSnapshotObjListCopyNames(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *opaque) -{ - virDomainSnapshotObjPtr obj = payload; - struct virDomainSnapshotNameData *data = opaque; - - if (data->error) - return; - /* Caller already sanitized flags. Filtering on DESCENDANTS was - * done by choice of iteration in the caller. */ - if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) && obj->nchildren) - return; - if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES) && !obj->nchildren) - return; - - if (data->names && data->count < data->maxnames && - !(data->names[data->count] = strdup(obj->def->name))) { - data->error = true; - virReportOOMError(); - return; - } - data->count++; -} - -int -virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr from, - char **const names, int maxnames, - unsigned int flags) -{ - struct virDomainSnapshotNameData data = { names, maxnames, flags, 0, - false }; - int 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_SNAPSHOT_LIST_ROOTS; - from = &snapshots->metaroot; - } - - /* We handle LIST_ROOT/LIST_DESCENDANTS directly, mask that bit - * out to determine when we must use the filter callback. */ - data.flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; - - /* If this common code is being used, we assume that all snapshots - * have metadata, and thus can handle METADATA up front as an - * all-or-none filter. XXX This might not always be true, if we - * add the ability to track qcow2 internal snapshots without the - * use of metadata. */ - if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA) == - VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA) - return 0; - data.flags &= ~VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA; - - /* For ease of coding the visitor, it is easier to zero the LEAVES - * group if both bits are set. */ - if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES) == - VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES) - data.flags &= ~VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES; - - if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) { - if (from->def) - virDomainSnapshotForEachDescendant(from, - virDomainSnapshotObjListCopyNames, - &data); - else if (names || data.flags) - virHashForEach(snapshots->objs, virDomainSnapshotObjListCopyNames, - &data); - else - data.count = virHashSize(snapshots->objs); - } else if (names || data.flags) { - virDomainSnapshotForEachChild(from, - virDomainSnapshotObjListCopyNames, &data); - } else { - data.count = from->nchildren; - } - - if (data.error) { - for (i = 0; i < data.count; i++) - VIR_FREE(names[i]); - return -1; - } - - return data.count; -} - -int -virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr from, - unsigned int flags) -{ - return virDomainSnapshotObjListGetNames(snapshots, from, NULL, 0, flags); -} - -virDomainSnapshotObjPtr -virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, - const char *name) -{ - return name ? virHashLookup(snapshots->objs, name) : &snapshots->metaroot; -} - -void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr snapshot) -{ - virHashRemoveEntry(snapshots->objs, snapshot->def->name); -} - -int -virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots, - virHashIterator iter, - void *data) -{ - return virHashForEach(snapshots->objs, iter, data); -} - -/* Run iter(data) on all direct children of snapshot, while ignoring all - * other entries in snapshots. Return the number of children - * visited. No particular ordering is guaranteed. */ -int -virDomainSnapshotForEachChild(virDomainSnapshotObjPtr snapshot, - virHashIterator iter, - void *data) -{ - virDomainSnapshotObjPtr child = snapshot->first_child; - - while (child) { - virDomainSnapshotObjPtr next = child->sibling; - (iter)(child, child->def->name, data); - child = next; - } - - return snapshot->nchildren; -} - -struct snapshot_act_on_descendant { - int number; - virHashIterator iter; - void *data; -}; - -static void -virDomainSnapshotActOnDescendant(void *payload, - const void *name, - void *data) -{ - virDomainSnapshotObjPtr obj = payload; - struct snapshot_act_on_descendant *curr = data; - - curr->number += 1 + virDomainSnapshotForEachDescendant(obj, - curr->iter, - curr->data); - (curr->iter)(payload, name, curr->data); -} - -/* Run iter(data) on all descendants of snapshot, while ignoring all - * other entries in snapshots. Return the number of descendants - * visited. No particular ordering is guaranteed. */ -int -virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr snapshot, - virHashIterator iter, - void *data) -{ - struct snapshot_act_on_descendant act; - - act.number = 0; - act.iter = iter; - act.data = data; - virDomainSnapshotForEachChild(snapshot, - virDomainSnapshotActOnDescendant, &act); - - return act.number; -} - -/* Struct and callback function used as a hash table callback; each call - * inspects the pre-existing snapshot->def->parent field, and adjusts - * the snapshot->parent field as well as the parent's child fields to - * wire up the hierarchical relations for the given snapshot. The error - * indicator gets set if a parent is missing or a requested parent would - * cause a circular parent chain. */ -struct snapshot_set_relation { - virDomainSnapshotObjListPtr snapshots; - int err; -}; -static void -virDomainSnapshotSetRelations(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *data) -{ - virDomainSnapshotObjPtr obj = payload; - struct snapshot_set_relation *curr = data; - virDomainSnapshotObjPtr tmp; - - obj->parent = virDomainSnapshotFindByName(curr->snapshots, - obj->def->parent); - if (!obj->parent) { - curr->err = -1; - obj->parent = &curr->snapshots->metaroot; - VIR_WARN("snapshot %s lacks parent", obj->def->name); - } else { - tmp = obj->parent; - while (tmp && tmp->def) { - if (tmp == obj) { - curr->err = -1; - obj->parent = &curr->snapshots->metaroot; - VIR_WARN("snapshot %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; -} - -/* Populate parent link and child count of all snapshots, 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 -virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots) -{ - struct snapshot_set_relation act = { snapshots, 0 }; - - virHashForEach(snapshots->objs, virDomainSnapshotSetRelations, &act); - return act.err; -} - -/* Prepare to reparent or delete snapshot, 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 -virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot) -{ - virDomainSnapshotObjPtr prev = NULL; - virDomainSnapshotObjPtr curr = NULL; - - snapshot->parent->nchildren--; - curr = snapshot->parent->first_child; - while (curr != snapshot) { - if (!curr) { - VIR_WARN("inconsistent snapshot relations"); - return; - } - prev = curr; - curr = curr->sibling; - } - if (prev) - prev->sibling = snapshot->sibling; - else - snapshot->parent->first_child = snapshot->sibling; - snapshot->parent = NULL; - snapshot->sibling = NULL; -} - - int virDomainChrDefForeach(virDomainDefPtr def, bool abortOnError, virDomainChrDefIterator iter, @@ -15836,49 +14960,6 @@ cleanup: return ret; } -int -virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr from, - virDomainPtr dom, - virDomainSnapshotPtr **snaps, - unsigned int flags) -{ - int count = virDomainSnapshotObjListNum(snapshots, from, flags); - virDomainSnapshotPtr *list; - char **names; - int ret = -1; - int i; - - if (!snaps) - return count; - if (VIR_ALLOC_N(names, count) < 0 || - VIR_ALLOC_N(list, count + 1) < 0) { - virReportOOMError(); - goto cleanup; - } - - if (virDomainSnapshotObjListGetNames(snapshots, from, names, count, - flags) < 0) - goto cleanup; - for (i = 0; i < count; i++) - if ((list[i] = virGetDomainSnapshot(dom, names[i])) == NULL) - goto cleanup; - - ret = count; - *snaps = 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; -} - virSecurityLabelDefPtr virDomainDefGetSecurityLabelDef(virDomainDefPtr def, const char *model) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6033641..ef4feaa 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -101,6 +101,12 @@ typedef virDomainChrDef *virDomainChrDefPtr; typedef struct _virDomainMemballoonDef virDomainMemballoonDef; typedef virDomainMemballoonDef *virDomainMemballoonDefPtr; +typedef struct _virDomainSnapshotObj virDomainSnapshotObj; +typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; + +typedef struct _virDomainSnapshotObjList virDomainSnapshotObjList; +typedef virDomainSnapshotObjList *virDomainSnapshotObjListPtr; + /* Flags for the 'type' field in virDomainDeviceDef */ typedef enum { VIR_DOMAIN_DEVICE_NONE = 0, @@ -501,21 +507,6 @@ enum virDomainDiskCopyOnRead { VIR_DOMAIN_DISK_COPY_ON_READ_LAST }; -enum virDomainDiskSnapshot { - VIR_DOMAIN_DISK_SNAPSHOT_DEFAULT = 0, - VIR_DOMAIN_DISK_SNAPSHOT_NO, - VIR_DOMAIN_DISK_SNAPSHOT_INTERNAL, - VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, - - VIR_DOMAIN_DISK_SNAPSHOT_LAST -}; - -enum virDomainSnapshotState { - /* Inherit the VIR_DOMAIN_* states from virDomainState. */ - VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST, - VIR_DOMAIN_SNAPSHOT_STATE_LAST -}; - enum virDomainStartupPolicy { VIR_DOMAIN_STARTUP_POLICY_DEFAULT = 0, VIR_DOMAIN_STARTUP_POLICY_MANDATORY, @@ -587,7 +578,7 @@ struct _virDomainDiskDef { int ioeventfd; int event_idx; int copy_on_read; - int snapshot; /* enum virDomainDiskSnapshot */ + int snapshot; /* enum virDomainDiskSnapshot, snapshot_conf.h */ int startupPolicy; /* enum virDomainStartupPolicy */ unsigned int readonly : 1; unsigned int shared : 1; @@ -1713,102 +1704,6 @@ enum virDomainTaintFlags { VIR_DOMAIN_TAINT_LAST }; -/* Items related to snapshot state */ - -/* Stores disk-snapshot information */ -typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef; -typedef virDomainSnapshotDiskDef *virDomainSnapshotDiskDefPtr; -struct _virDomainSnapshotDiskDef { - char *name; /* name matching the <target dev='...' of the domain */ - int index; /* index within snapshot->dom->disks that matches name */ - int snapshot; /* enum virDomainDiskSnapshot */ - char *file; /* new source file when snapshot is external */ - char *driverType; /* file format type of new file */ -}; - -/* Stores the complete snapshot metadata */ -typedef struct _virDomainSnapshotDef virDomainSnapshotDef; -typedef virDomainSnapshotDef *virDomainSnapshotDefPtr; -struct _virDomainSnapshotDef { - /* Public XML. */ - char *name; - char *description; - char *parent; - long long creationTime; /* in seconds */ - int state; /* enum virDomainSnapshotState */ - - size_t ndisks; /* should not exceed dom->ndisks */ - virDomainSnapshotDiskDef *disks; - - virDomainDefPtr dom; - - /* Internal use. */ - bool current; /* At most one snapshot in the list should have this set */ -}; - -typedef struct _virDomainSnapshotObj virDomainSnapshotObj; -typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; -struct _virDomainSnapshotObj { - virDomainSnapshotDefPtr def; /* non-NULL except for metaroot */ - - virDomainSnapshotObjPtr parent; /* non-NULL except for metaroot, before - virDomainSnapshotUpdateRelations, or - after virDomainSnapshotDropParent */ - virDomainSnapshotObjPtr sibling; /* NULL if last child of parent */ - size_t nchildren; - virDomainSnapshotObjPtr first_child; /* NULL if no children */ -}; - -typedef struct _virDomainSnapshotObjList virDomainSnapshotObjList; -typedef virDomainSnapshotObjList *virDomainSnapshotObjListPtr; - -virDomainSnapshotObjListPtr virDomainSnapshotObjListNew(void); -void virDomainSnapshotObjListFree(virDomainSnapshotObjListPtr snapshots); - -typedef enum { - VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0, - VIR_DOMAIN_SNAPSHOT_PARSE_DISKS = 1 << 1, - VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 2, -} virDomainSnapshotParseFlags; - -virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - virCapsPtr caps, - unsigned int expectedVirtTypes, - unsigned int flags); -void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); -char *virDomainSnapshotDefFormat(const char *domain_uuid, - virDomainSnapshotDefPtr def, - unsigned int flags, - int internal); -int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot, - int default_snapshot, - bool require_match); -virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, - const virDomainSnapshotDefPtr def); - -int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr from, - char **const names, int maxnames, - unsigned int flags); -int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr from, - unsigned int flags); -virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, - const char *name); -void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr snapshot); -int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots, - virHashIterator iter, - void *data); -int virDomainSnapshotForEachChild(virDomainSnapshotObjPtr snapshot, - virHashIterator iter, - void *data); -int virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr snapshot, - virHashIterator iter, - void *data); -int virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots); -void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot); - /* Guest VM runtime state */ typedef struct _virDomainStateReason virDomainStateReason; struct _virDomainStateReason { @@ -2202,7 +2097,6 @@ VIR_ENUM_DECL(virDomainDiskErrorPolicy) VIR_ENUM_DECL(virDomainDiskProtocol) VIR_ENUM_DECL(virDomainDiskIo) VIR_ENUM_DECL(virDomainDiskSecretType) -VIR_ENUM_DECL(virDomainDiskSnapshot) VIR_ENUM_DECL(virDomainDiskTray) VIR_ENUM_DECL(virDomainIoEventFd) VIR_ENUM_DECL(virDomainVirtioEventIdx) @@ -2253,7 +2147,6 @@ VIR_ENUM_DECL(virDomainGraphicsSpiceClipboardCopypaste) VIR_ENUM_DECL(virDomainGraphicsSpiceMouseMode) VIR_ENUM_DECL(virDomainNumatuneMemMode) VIR_ENUM_DECL(virDomainNumatuneMemPlacementMode) -VIR_ENUM_DECL(virDomainSnapshotState) /* from libvirt.h */ VIR_ENUM_DECL(virDomainState) VIR_ENUM_DECL(virDomainNostateReason) @@ -2316,25 +2209,7 @@ virDomainNetDefPtr virDomainNetFind(virDomainDefPtr def, VIR_CONNECT_LIST_DOMAINS_FILTERS_AUTOSTART | \ VIR_CONNECT_LIST_DOMAINS_FILTERS_SNAPSHOT) -# define VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA \ - (VIR_DOMAIN_SNAPSHOT_LIST_METADATA | \ - VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA) - -# define VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES \ - (VIR_DOMAIN_SNAPSHOT_LIST_LEAVES | \ - VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES) - -# define VIR_DOMAIN_SNAPSHOT_FILTERS_ALL \ - (VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA | \ - VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES) - int virDomainList(virConnectPtr conn, virHashTablePtr domobjs, virDomainPtr **domains, unsigned int flags); -int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr from, - virDomainPtr dom, - virDomainSnapshotPtr **snaps, - unsigned int flags); - #endif /* __DOMAIN_CONF_H */ diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c new file mode 100644 index 0000000..894a74c --- /dev/null +++ b/src/conf/snapshot_conf.c @@ -0,0 +1,970 @@ +/* + * snapshot_conf.c: domain snapshot XML processing + * + * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2008 Daniel P. Berrange + * + * 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/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#include <config.h> + +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> +#include <fcntl.h> +#include <sys/time.h> + +#include "internal.h" +#include "virterror_internal.h" +#include "datatypes.h" +#include "domain_conf.h" +#include "snapshot_conf.h" +#include "memory.h" +#include "xml.h" +#include "uuid.h" +#include "util.h" +#include "buf.h" +#include "logging.h" +#include "nwfilter_conf.h" +#include "storage_file.h" +#include "virfile.h" +#include "bitmap.h" +#include "count-one-bits.h" +#include "secret_conf.h" +#include "netdev_vport_profile_conf.h" +#include "netdev_bandwidth_conf.h" + +#define VIR_FROM_THIS VIR_FROM_DOMAIN_SNAPSHOT + +VIR_ENUM_IMPL(virDomainDiskSnapshot, VIR_DOMAIN_DISK_SNAPSHOT_LAST, + "default", + "no", + "internal", + "external") + +/* virDomainSnapshotState is really virDomainState plus one extra state */ +VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_SNAPSHOT_STATE_LAST, + "nostate", + "running", + "blocked", + "paused", + "shutdown", + "shutoff", + "crashed", + "pmsuspended", + "disk-snapshot") + +struct _virDomainSnapshotObjList { + /* name string -> virDomainSnapshotObj mapping + * for O(1), lockless lookup-by-name */ + virHashTable *objs; + + virDomainSnapshotObj metaroot; /* Special parent of all root snapshots */ +}; + +/* Snapshot Def functions */ +static void +virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) +{ + VIR_FREE(disk->name); + VIR_FREE(disk->file); + VIR_FREE(disk->driverType); +} + +void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) +{ + int i; + + if (!def) + return; + + VIR_FREE(def->name); + VIR_FREE(def->description); + VIR_FREE(def->parent); + for (i = 0; i < def->ndisks; i++) + virDomainSnapshotDiskDefClear(&def->disks[i]); + VIR_FREE(def->disks); + virDomainDefFree(def->dom); + VIR_FREE(def); +} + +static int +virDomainSnapshotDiskDefParseXML(xmlNodePtr node, + virDomainSnapshotDiskDefPtr def) +{ + int ret = -1; + char *snapshot = NULL; + xmlNodePtr cur; + + def->name = virXMLPropString(node, "name"); + if (!def->name) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing name from disk snapshot element")); + goto cleanup; + } + + snapshot = virXMLPropString(node, "snapshot"); + if (snapshot) { + def->snapshot = virDomainDiskSnapshotTypeFromString(snapshot); + if (def->snapshot <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown disk snapshot setting '%s'"), + snapshot); + goto cleanup; + } + } + + cur = node->children; + while (cur) { + if (cur->type == XML_ELEMENT_NODE) { + if (!def->file && + xmlStrEqual(cur->name, BAD_CAST "source")) { + def->file = virXMLPropString(cur, "file"); + } else if (!def->driverType && + xmlStrEqual(cur->name, BAD_CAST "driver")) { + def->driverType = virXMLPropString(cur, "type"); + } + } + cur = cur->next; + } + + if (!def->snapshot && (def->file || def->driverType)) + def->snapshot = VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL; + + ret = 0; +cleanup: + VIR_FREE(snapshot); + if (ret < 0) + virDomainSnapshotDiskDefClear(def); + return ret; +} + +/* flags is bitwise-or of virDomainSnapshotParseFlags. + * If flags does not include VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE, then + * caps and expectedVirtTypes are ignored. + */ +virDomainSnapshotDefPtr +virDomainSnapshotDefParseString(const char *xmlStr, + virCapsPtr caps, + unsigned int expectedVirtTypes, + unsigned int flags) +{ + xmlXPathContextPtr ctxt = NULL; + xmlDocPtr xml = NULL; + virDomainSnapshotDefPtr def = NULL; + virDomainSnapshotDefPtr ret = NULL; + xmlNodePtr *nodes = NULL; + int i; + char *creation = NULL, *state = NULL; + struct timeval tv; + int active; + char *tmp; + int keepBlanksDefault = xmlKeepBlanksDefault(0); + + xml = virXMLParseCtxt(NULL, xmlStr, _("(domain_snapshot)"), &ctxt); + if (!xml) { + xmlKeepBlanksDefault(keepBlanksDefault); + return NULL; + } + xmlKeepBlanksDefault(keepBlanksDefault); + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (!xmlStrEqual(ctxt->node->name, BAD_CAST "domainsnapshot")) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("domainsnapshot")); + goto cleanup; + } + + gettimeofday(&tv, NULL); + + def->name = virXPathString("string(./name)", ctxt); + if (def->name == NULL) { + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("a redefined snapshot must have a name")); + goto cleanup; + } else { + ignore_value(virAsprintf(&def->name, "%lld", + (long long)tv.tv_sec)); + } + } + + if (def->name == NULL) { + virReportOOMError(); + goto cleanup; + } + + def->description = virXPathString("string(./description)", ctxt); + + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + if (virXPathLongLong("string(./creationTime)", ctxt, + &def->creationTime) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing creationTime from existing snapshot")); + goto cleanup; + } + + def->parent = virXPathString("string(./parent/name)", ctxt); + + state = virXPathString("string(./state)", ctxt); + if (state == NULL) { + /* there was no state in an existing snapshot; this + * should never happen + */ + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing state from existing snapshot")); + goto cleanup; + } + def->state = virDomainSnapshotStateTypeFromString(state); + if (def->state < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid state '%s' in domain snapshot XML"), + state); + goto cleanup; + } + + /* Older snapshots were created with just <domain>/<uuid>, and + * lack domain/@type. In that case, leave dom NULL, and + * clients will have to decide between best effort + * initialization or outright failure. */ + if ((tmp = virXPathString("string(./domain/@type)", ctxt))) { + xmlNodePtr domainNode = virXPathNode("./domain", ctxt); + + VIR_FREE(tmp); + if (!domainNode) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing domain in snapshot")); + goto cleanup; + } + def->dom = virDomainDefParseNode(caps, xml, domainNode, + expectedVirtTypes, + (VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE)); + if (!def->dom) + goto cleanup; + } else { + VIR_WARN("parsing older snapshot that lacks domain"); + } + } else { + def->creationTime = tv.tv_sec; + } + + if ((i = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0) + goto cleanup; + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_DISKS || + (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE && + def->state == VIR_DOMAIN_DISK_SNAPSHOT)) { + def->ndisks = i; + if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) { + virReportOOMError(); + goto cleanup; + } + for (i = 0; i < def->ndisks; i++) { + if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0) + goto cleanup; + } + VIR_FREE(nodes); + } else if (i) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("unable to handle disk requests in snapshot")); + goto cleanup; + } + + if (flags & VIR_DOMAIN_SNAPSHOT_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; + } + + ret = def; + +cleanup: + VIR_FREE(creation); + VIR_FREE(state); + VIR_FREE(nodes); + xmlXPathFreeContext(ctxt); + if (ret == NULL) + virDomainSnapshotDefFree(def); + xmlFreeDoc(xml); + + return ret; +} + +static int +disksorter(const void *a, const void *b) +{ + const virDomainSnapshotDiskDef *diska = a; + const virDomainSnapshotDiskDef *diskb = b; + + /* Integer overflow shouldn't be a problem here. */ + return diska->index - diskb->index; +} + +/* Align def->disks to def->domain. Sort the list of def->disks, + * filling in any missing disks or snapshot state defaults given by + * the domain, with a fallback to a passed in 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. If require_match, also require that existing + * def->disks snapshot states do not override explicit def->dom + * settings. */ +int +virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, + int default_snapshot, + bool require_match) +{ + int ret = -1; + virBitmapPtr map = NULL; + int i; + int ndisks; + bool inuse; + + if (!def->dom) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing domain in snapshot")); + goto cleanup; + } + + if (def->ndisks > def->dom->ndisks) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("too many disk snapshot requests for domain")); + goto cleanup; + } + + /* Unlikely to have a guest without disks but technically possible. */ + if (!def->dom->ndisks) { + ret = 0; + goto cleanup; + } + + if (!(map = virBitmapAlloc(def->dom->ndisks))) { + virReportOOMError(); + goto cleanup; + } + + /* Double check requested disks. */ + for (i = 0; i < def->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk = &def->disks[i]; + int idx = virDomainDiskIndexByName(def->dom, disk->name, false); + int disk_snapshot; + + if (idx < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("no disk named '%s'"), disk->name); + goto cleanup; + } + disk_snapshot = def->dom->disks[idx]->snapshot; + + if (virBitmapGetBit(map, idx, &inuse) < 0 || inuse) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk '%s' specified twice"), + disk->name); + goto cleanup; + } + ignore_value(virBitmapSetBit(map, idx)); + disk->index = idx; + if (!disk_snapshot) + disk_snapshot = default_snapshot; + if (!disk->snapshot) { + disk->snapshot = disk_snapshot; + } else if (disk_snapshot && require_match && + disk->snapshot != disk_snapshot) { + const char *tmp = virDomainDiskSnapshotTypeToString(disk_snapshot); + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk '%s' must use snapshot mode '%s'"), + disk->name, tmp); + goto cleanup; + } + if (disk->file && + disk->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("file '%s' for disk '%s' requires " + "use of external snapshot mode"), + disk->file, disk->name); + goto cleanup; + } + if (STRNEQ(disk->name, def->dom->disks[idx]->dst)) { + VIR_FREE(disk->name); + if (!(disk->name = strdup(def->dom->disks[idx]->dst))) { + virReportOOMError(); + goto cleanup; + } + } + } + + /* Provide defaults for all remaining disks. */ + ndisks = def->ndisks; + if (VIR_EXPAND_N(def->disks, def->ndisks, + def->dom->ndisks - def->ndisks) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0; i < def->dom->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk; + + ignore_value(virBitmapGetBit(map, i, &inuse)); + if (inuse) + continue; + disk = &def->disks[ndisks++]; + if (!(disk->name = strdup(def->dom->disks[i]->dst))) { + virReportOOMError(); + goto cleanup; + } + disk->index = i; + disk->snapshot = def->dom->disks[i]->snapshot; + if (!disk->snapshot) + disk->snapshot = default_snapshot; + } + + qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), disksorter); + + /* Generate any default external file names, but only if the + * backing file is a regular file. */ + for (i = 0; i < def->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk = &def->disks[i]; + + if (disk->snapshot == VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL && + !disk->file) { + const char *original = def->dom->disks[i]->src; + const char *tmp; + struct stat sb; + + if (!original) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("cannot generate external snapshot name " + "for disk '%s' without source"), + disk->name); + goto cleanup; + } + if (stat(original, &sb) < 0 || !S_ISREG(sb.st_mode)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("source for disk '%s' is not a regular " + "file; refusing to generate external " + "snapshot name"), + disk->name); + goto cleanup; + } + + tmp = strrchr(original, '.'); + if (!tmp || strchr(tmp, '/')) { + ignore_value(virAsprintf(&disk->file, "%s.%s", + original, def->name)); + } else { + if ((tmp - original) > INT_MAX) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("integer overflow")); + goto cleanup; + } + ignore_value(virAsprintf(&disk->file, "%.*s.%s", + (int) (tmp - original), original, + def->name)); + } + if (!disk->file) { + virReportOOMError(); + goto cleanup; + } + } + } + + ret = 0; + +cleanup: + virBitmapFree(map); + return ret; +} + +char *virDomainSnapshotDefFormat(const char *domain_uuid, + virDomainSnapshotDefPtr def, + unsigned int flags, + int internal) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int i; + + virCheckFlags(VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_UPDATE_CPU, NULL); + + flags |= VIR_DOMAIN_XML_INACTIVE; + + virBufferAddLit(&buf, "<domainsnapshot>\n"); + virBufferEscapeString(&buf, " <name>%s</name>\n", def->name); + if (def->description) + virBufferEscapeString(&buf, " <description>%s</description>\n", + def->description); + virBufferAsprintf(&buf, " <state>%s</state>\n", + virDomainSnapshotStateTypeToString(def->state)); + if (def->parent) { + virBufferAddLit(&buf, " <parent>\n"); + virBufferEscapeString(&buf, " <name>%s</name>\n", def->parent); + virBufferAddLit(&buf, " </parent>\n"); + } + virBufferAsprintf(&buf, " <creationTime>%lld</creationTime>\n", + def->creationTime); + /* For now, only output <disks> on disk-snapshot */ + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT) { + virBufferAddLit(&buf, " <disks>\n"); + for (i = 0; i < def->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk = &def->disks[i]; + + if (!disk->name) + continue; + + virBufferEscapeString(&buf, " <disk name='%s'", disk->name); + if (disk->snapshot) + virBufferAsprintf(&buf, " snapshot='%s'", + virDomainDiskSnapshotTypeToString(disk->snapshot)); + if (disk->file || disk->driverType) { + virBufferAddLit(&buf, ">\n"); + if (disk->driverType) + virBufferEscapeString(&buf, " <driver type='%s'/>\n", + disk->driverType); + if (disk->file) + virBufferEscapeString(&buf, " <source file='%s'/>\n", + disk->file); + virBufferAddLit(&buf, " </disk>\n"); + } else { + virBufferAddLit(&buf, "/>\n"); + } + } + virBufferAddLit(&buf, " </disks>\n"); + } + if (def->dom) { + virBufferAdjustIndent(&buf, 2); + if (virDomainDefFormatInternal(def->dom, flags, &buf) < 0) { + virBufferFreeAndReset(&buf); + return NULL; + } + virBufferAdjustIndent(&buf, -2); + } else if (domain_uuid) { + virBufferAddLit(&buf, " <domain>\n"); + virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); + virBufferAddLit(&buf, " </domain>\n"); + } + if (internal) + virBufferAsprintf(&buf, " <active>%d</active>\n", def->current); + virBufferAddLit(&buf, "</domainsnapshot>\n"); + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + +/* Snapshot Obj functions */ +static virDomainSnapshotObjPtr virDomainSnapshotObjNew(void) +{ + virDomainSnapshotObjPtr snapshot; + + if (VIR_ALLOC(snapshot) < 0) { + virReportOOMError(); + return NULL; + } + + VIR_DEBUG("obj=%p", snapshot); + + return snapshot; +} + +static void virDomainSnapshotObjFree(virDomainSnapshotObjPtr snapshot) +{ + if (!snapshot) + return; + + VIR_DEBUG("obj=%p", snapshot); + + virDomainSnapshotDefFree(snapshot->def); + VIR_FREE(snapshot); +} + +virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, + const virDomainSnapshotDefPtr def) +{ + virDomainSnapshotObjPtr snap; + + if (virHashLookup(snapshots->objs, def->name) != NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected domain snapshot %s already exists"), + def->name); + return NULL; + } + + if (!(snap = virDomainSnapshotObjNew())) + return NULL; + snap->def = def; + + if (virHashAddEntry(snapshots->objs, snap->def->name, snap) < 0) { + VIR_FREE(snap); + return NULL; + } + + return snap; +} + +/* Snapshot Obj List functions */ +static void +virDomainSnapshotObjListDataFree(void *payload, + const void *name ATTRIBUTE_UNUSED) +{ + virDomainSnapshotObjPtr obj = payload; + + virDomainSnapshotObjFree(obj); +} + +virDomainSnapshotObjListPtr +virDomainSnapshotObjListNew(void) +{ + virDomainSnapshotObjListPtr snapshots; + if (VIR_ALLOC(snapshots) < 0) { + virReportOOMError(); + return NULL; + } + snapshots->objs = virHashCreate(50, virDomainSnapshotObjListDataFree); + if (!snapshots->objs) { + VIR_FREE(snapshots); + return NULL; + } + return snapshots; +} + +void +virDomainSnapshotObjListFree(virDomainSnapshotObjListPtr snapshots) +{ + if (!snapshots) + return; + virHashFree(snapshots->objs); + VIR_FREE(snapshots); +} + +struct virDomainSnapshotNameData { + char **const names; + int maxnames; + unsigned int flags; + int count; + bool error; +}; + +static void virDomainSnapshotObjListCopyNames(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *opaque) +{ + virDomainSnapshotObjPtr obj = payload; + struct virDomainSnapshotNameData *data = opaque; + + if (data->error) + return; + /* Caller already sanitized flags. Filtering on DESCENDANTS was + * done by choice of iteration in the caller. */ + if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) && obj->nchildren) + return; + if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES) && !obj->nchildren) + return; + + if (data->names && data->count < data->maxnames && + !(data->names[data->count] = strdup(obj->def->name))) { + data->error = true; + virReportOOMError(); + return; + } + data->count++; +} + +int +virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + char **const names, int maxnames, + unsigned int flags) +{ + struct virDomainSnapshotNameData data = { names, maxnames, flags, 0, + false }; + int 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_SNAPSHOT_LIST_ROOTS; + from = &snapshots->metaroot; + } + + /* We handle LIST_ROOT/LIST_DESCENDANTS directly, mask that bit + * out to determine when we must use the filter callback. */ + data.flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + + /* If this common code is being used, we assume that all snapshots + * have metadata, and thus can handle METADATA up front as an + * all-or-none filter. XXX This might not always be true, if we + * add the ability to track qcow2 internal snapshots without the + * use of metadata. */ + if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA) == + VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA) + return 0; + data.flags &= ~VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA; + + /* For ease of coding the visitor, it is easier to zero the LEAVES + * group if both bits are set. */ + if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES) == + VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES) + data.flags &= ~VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES; + + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) { + if (from->def) + virDomainSnapshotForEachDescendant(from, + virDomainSnapshotObjListCopyNames, + &data); + else if (names || data.flags) + virHashForEach(snapshots->objs, virDomainSnapshotObjListCopyNames, + &data); + else + data.count = virHashSize(snapshots->objs); + } else if (names || data.flags) { + virDomainSnapshotForEachChild(from, + virDomainSnapshotObjListCopyNames, &data); + } else { + data.count = from->nchildren; + } + + if (data.error) { + for (i = 0; i < data.count; i++) + VIR_FREE(names[i]); + return -1; + } + + return data.count; +} + +int +virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + unsigned int flags) +{ + return virDomainSnapshotObjListGetNames(snapshots, from, NULL, 0, flags); +} + +virDomainSnapshotObjPtr +virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, + const char *name) +{ + return name ? virHashLookup(snapshots->objs, name) : &snapshots->metaroot; +} + +void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr snapshot) +{ + virHashRemoveEntry(snapshots->objs, snapshot->def->name); +} + +int +virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots, + virHashIterator iter, + void *data) +{ + return virHashForEach(snapshots->objs, iter, data); +} + +/* Run iter(data) on all direct children of snapshot, while ignoring all + * other entries in snapshots. Return the number of children + * visited. No particular ordering is guaranteed. */ +int +virDomainSnapshotForEachChild(virDomainSnapshotObjPtr snapshot, + virHashIterator iter, + void *data) +{ + virDomainSnapshotObjPtr child = snapshot->first_child; + + while (child) { + virDomainSnapshotObjPtr next = child->sibling; + (iter)(child, child->def->name, data); + child = next; + } + + return snapshot->nchildren; +} + +struct snapshot_act_on_descendant { + int number; + virHashIterator iter; + void *data; +}; + +static void +virDomainSnapshotActOnDescendant(void *payload, + const void *name, + void *data) +{ + virDomainSnapshotObjPtr obj = payload; + struct snapshot_act_on_descendant *curr = data; + + curr->number += 1 + virDomainSnapshotForEachDescendant(obj, + curr->iter, + curr->data); + (curr->iter)(payload, name, curr->data); +} + +/* Run iter(data) on all descendants of snapshot, while ignoring all + * other entries in snapshots. Return the number of descendants + * visited. No particular ordering is guaranteed. */ +int +virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr snapshot, + virHashIterator iter, + void *data) +{ + struct snapshot_act_on_descendant act; + + act.number = 0; + act.iter = iter; + act.data = data; + virDomainSnapshotForEachChild(snapshot, + virDomainSnapshotActOnDescendant, &act); + + return act.number; +} + +/* Struct and callback function used as a hash table callback; each call + * inspects the pre-existing snapshot->def->parent field, and adjusts + * the snapshot->parent field as well as the parent's child fields to + * wire up the hierarchical relations for the given snapshot. The error + * indicator gets set if a parent is missing or a requested parent would + * cause a circular parent chain. */ +struct snapshot_set_relation { + virDomainSnapshotObjListPtr snapshots; + int err; +}; +static void +virDomainSnapshotSetRelations(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data) +{ + virDomainSnapshotObjPtr obj = payload; + struct snapshot_set_relation *curr = data; + virDomainSnapshotObjPtr tmp; + + obj->parent = virDomainSnapshotFindByName(curr->snapshots, + obj->def->parent); + if (!obj->parent) { + curr->err = -1; + obj->parent = &curr->snapshots->metaroot; + VIR_WARN("snapshot %s lacks parent", obj->def->name); + } else { + tmp = obj->parent; + while (tmp && tmp->def) { + if (tmp == obj) { + curr->err = -1; + obj->parent = &curr->snapshots->metaroot; + VIR_WARN("snapshot %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; +} + +/* Populate parent link and child count of all snapshots, 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 +virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots) +{ + struct snapshot_set_relation act = { snapshots, 0 }; + + virHashForEach(snapshots->objs, virDomainSnapshotSetRelations, &act); + return act.err; +} + +/* Prepare to reparent or delete snapshot, 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 +virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot) +{ + virDomainSnapshotObjPtr prev = NULL; + virDomainSnapshotObjPtr curr = NULL; + + snapshot->parent->nchildren--; + curr = snapshot->parent->first_child; + while (curr != snapshot) { + if (!curr) { + VIR_WARN("inconsistent snapshot relations"); + return; + } + prev = curr; + curr = curr->sibling; + } + if (prev) + prev->sibling = snapshot->sibling; + else + snapshot->parent->first_child = snapshot->sibling; + snapshot->parent = NULL; + snapshot->sibling = NULL; +} + +int +virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + virDomainPtr dom, + virDomainSnapshotPtr **snaps, + unsigned int flags) +{ + int count = virDomainSnapshotObjListNum(snapshots, from, flags); + virDomainSnapshotPtr *list; + char **names; + int ret = -1; + int i; + + if (!snaps) + return count; + if (VIR_ALLOC_N(names, count) < 0 || + VIR_ALLOC_N(list, count + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virDomainSnapshotObjListGetNames(snapshots, from, names, count, + flags) < 0) + goto cleanup; + for (i = 0; i < count; i++) + if ((list[i] = virGetDomainSnapshot(dom, names[i])) == NULL) + goto cleanup; + + ret = count; + *snaps = 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; +} diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h new file mode 100644 index 0000000..314c4d1 --- /dev/null +++ b/src/conf/snapshot_conf.h @@ -0,0 +1,157 @@ +/* + * snapshot_conf.h: domain snapshot XML processing + * + * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2008 Daniel P. Berrange + * + * 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/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com> + */ + +#ifndef __SNAPSHOT_CONF_H +# define __SNAPSHOT_CONF_H + +# include "internal.h" +# include "domain_conf.h" + +/* Items related to snapshot state */ + +enum virDomainDiskSnapshot { + VIR_DOMAIN_DISK_SNAPSHOT_DEFAULT = 0, + VIR_DOMAIN_DISK_SNAPSHOT_NO, + VIR_DOMAIN_DISK_SNAPSHOT_INTERNAL, + VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, + + VIR_DOMAIN_DISK_SNAPSHOT_LAST +}; + +enum virDomainSnapshotState { + /* Inherit the VIR_DOMAIN_* states from virDomainState. */ + VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST, + VIR_DOMAIN_SNAPSHOT_STATE_LAST +}; + +/* Stores disk-snapshot information */ +typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef; +typedef virDomainSnapshotDiskDef *virDomainSnapshotDiskDefPtr; +struct _virDomainSnapshotDiskDef { + char *name; /* name matching the <target dev='...' of the domain */ + int index; /* index within snapshot->dom->disks that matches name */ + int snapshot; /* enum virDomainDiskSnapshot */ + char *file; /* new source file when snapshot is external */ + char *driverType; /* file format type of new file */ +}; + +/* Stores the complete snapshot metadata */ +typedef struct _virDomainSnapshotDef virDomainSnapshotDef; +typedef virDomainSnapshotDef *virDomainSnapshotDefPtr; +struct _virDomainSnapshotDef { + /* Public XML. */ + char *name; + char *description; + char *parent; + long long creationTime; /* in seconds */ + int state; /* enum virDomainSnapshotState */ + + size_t ndisks; /* should not exceed dom->ndisks */ + virDomainSnapshotDiskDef *disks; + + virDomainDefPtr dom; + + /* Internal use. */ + bool current; /* At most one snapshot in the list should have this set */ +}; + +struct _virDomainSnapshotObj { + virDomainSnapshotDefPtr def; /* non-NULL except for metaroot */ + + virDomainSnapshotObjPtr parent; /* non-NULL except for metaroot, before + virDomainSnapshotUpdateRelations, or + after virDomainSnapshotDropParent */ + virDomainSnapshotObjPtr sibling; /* NULL if last child of parent */ + size_t nchildren; + virDomainSnapshotObjPtr first_child; /* NULL if no children */ +}; + +virDomainSnapshotObjListPtr virDomainSnapshotObjListNew(void); +void virDomainSnapshotObjListFree(virDomainSnapshotObjListPtr snapshots); + +typedef enum { + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0, + VIR_DOMAIN_SNAPSHOT_PARSE_DISKS = 1 << 1, + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 2, +} virDomainSnapshotParseFlags; + +virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, + virCapsPtr caps, + unsigned int expectedVirtTypes, + unsigned int flags); +void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); +char *virDomainSnapshotDefFormat(const char *domain_uuid, + virDomainSnapshotDefPtr def, + unsigned int flags, + int internal); +int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot, + int default_snapshot, + bool require_match); +virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, + const virDomainSnapshotDefPtr def); + +int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + char **const names, int maxnames, + unsigned int flags); +int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + unsigned int flags); +virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, + const char *name); +void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr snapshot); +int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots, + virHashIterator iter, + void *data); +int virDomainSnapshotForEachChild(virDomainSnapshotObjPtr snapshot, + virHashIterator iter, + void *data); +int virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr snapshot, + virHashIterator iter, + void *data); +int virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots); +void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot); + +# define VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA \ + (VIR_DOMAIN_SNAPSHOT_LIST_METADATA | \ + VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA) + +# define VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES \ + (VIR_DOMAIN_SNAPSHOT_LIST_LEAVES | \ + VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES) + +# define VIR_DOMAIN_SNAPSHOT_FILTERS_ALL \ + (VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA | \ + VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES) + +int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + virDomainPtr dom, + virDomainSnapshotPtr **snaps, + unsigned int flags); + +VIR_ENUM_DECL(virDomainDiskSnapshot) +VIR_ENUM_DECL(virDomainSnapshotState) + +#endif /* __SNAPSHOT_CONF_H */ diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 72a7acc..e57296a 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -26,6 +26,7 @@ #include "internal.h" #include "domain_conf.h" +#include "snapshot_conf.h" #include "virauth.h" #include "util.h" #include "memory.h" diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ca62f0c..8c32a4d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -38,6 +38,7 @@ #include "domain_nwfilter.h" #include "domain_audit.h" #include "domain_conf.h" +#include "snapshot_conf.h" #include "network/bridge_driver.h" #include "virnetdevtap.h" #include "base64.h" diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b96087e..dff53cf 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1,7 +1,7 @@ /* * qemu_domain.h: QEMU domain private state * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -26,6 +26,7 @@ # include "threads.h" # include "domain_conf.h" +# include "snapshot_conf.h" # include "qemu_monitor.h" # include "qemu_agent.h" # include "qemu_conf.h" diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 4cdb11c..48f371f 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -43,6 +43,7 @@ #include "internal.h" #include "datatypes.h" #include "domain_conf.h" +#include "snapshot_conf.h" #include "network_conf.h" #include "virterror_internal.h" #include "domain_event.h" -- 1.7.11.4

On Thu, Aug 23, 2012 at 02:54:15PM -0600, Eric Blake wrote:
This has several benefits: 1. Future snapshot-related code has a definite place to go (and I _will_ be adding some) 2. Snapshot errors now use the VIR_FROM_DOMAIN_SNAPSHOT error classification, which has been underutilized (previously only in libvirt.c)
* src/conf/domain_conf.h, domain_conf.c: Split... * src/conf/snapshot_conf.h, snapshot_conf.c: ...into new files. * src/Makefile.am (DOMAIN_CONF_SOURCES): Build new files. * po/POTFILES.in: Mark new file for translation. * src/vbox/vbox_tmpl.c: Update caller. * src/esx/esx_driver.c: Likewise. * src/qemu/qemu_command.c: Likewise. * src/qemu/qemu_domain.h: Likewise. [...] diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c new file mode 100644 index 0000000..894a74c --- /dev/null +++ b/src/conf/snapshot_conf.c @@ -0,0 +1,970 @@ +/* + * snapshot_conf.c: domain snapshot XML processing + * + * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2008 Daniel P. Berrange + * + * 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/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com>
Hum ... I would have guessed you're the author of most of that snapshot related code, not Dan, so i would probably adjust this accordingly
+ */ + +#include <config.h> + +#include <sys/types.h> +#include <sys/stat.h> +#include <unistd.h> +#include <fcntl.h> +#include <sys/time.h> + +#include "internal.h" +#include "virterror_internal.h" +#include "datatypes.h" +#include "domain_conf.h" +#include "snapshot_conf.h" +#include "memory.h" +#include "xml.h" +#include "uuid.h" +#include "util.h" +#include "buf.h" +#include "logging.h" +#include "nwfilter_conf.h" +#include "storage_file.h" +#include "virfile.h" +#include "bitmap.h" +#include "count-one-bits.h" +#include "secret_conf.h" +#include "netdev_vport_profile_conf.h" +#include "netdev_bandwidth_conf.h"
I assume from there everything is just moved around [...]
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h new file mode 100644 index 0000000..314c4d1 --- /dev/null +++ b/src/conf/snapshot_conf.h @@ -0,0 +1,157 @@ +/* + * snapshot_conf.h: domain snapshot XML processing + * + * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2008 Daniel P. Berrange + * + * 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/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com>
idem
+ */ + +#ifndef __SNAPSHOT_CONF_H +# define __SNAPSHOT_CONF_H + +# include "internal.h" +# include "domain_conf.h" + +/* Items related to snapshot state */ + +enum virDomainDiskSnapshot { + VIR_DOMAIN_DISK_SNAPSHOT_DEFAULT = 0, + VIR_DOMAIN_DISK_SNAPSHOT_NO, + VIR_DOMAIN_DISK_SNAPSHOT_INTERNAL, + VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, + + VIR_DOMAIN_DISK_SNAPSHOT_LAST +}; + +enum virDomainSnapshotState { + /* Inherit the VIR_DOMAIN_* states from virDomainState. */ + VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST, + VIR_DOMAIN_SNAPSHOT_STATE_LAST +}; + +/* Stores disk-snapshot information */ +typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef; +typedef virDomainSnapshotDiskDef *virDomainSnapshotDiskDefPtr; +struct _virDomainSnapshotDiskDef { + char *name; /* name matching the <target dev='...' of the domain */ + int index; /* index within snapshot->dom->disks that matches name */ + int snapshot; /* enum virDomainDiskSnapshot */ + char *file; /* new source file when snapshot is external */ + char *driverType; /* file format type of new file */ +}; + +/* Stores the complete snapshot metadata */ +typedef struct _virDomainSnapshotDef virDomainSnapshotDef; +typedef virDomainSnapshotDef *virDomainSnapshotDefPtr; +struct _virDomainSnapshotDef { + /* Public XML. */ + char *name; + char *description; + char *parent; + long long creationTime; /* in seconds */ + int state; /* enum virDomainSnapshotState */ + + size_t ndisks; /* should not exceed dom->ndisks */ + virDomainSnapshotDiskDef *disks; + + virDomainDefPtr dom; + + /* Internal use. */ + bool current; /* At most one snapshot in the list should have this set */ +}; + +struct _virDomainSnapshotObj { + virDomainSnapshotDefPtr def; /* non-NULL except for metaroot */ + + virDomainSnapshotObjPtr parent; /* non-NULL except for metaroot, before + virDomainSnapshotUpdateRelations, or + after virDomainSnapshotDropParent */ + virDomainSnapshotObjPtr sibling; /* NULL if last child of parent */ + size_t nchildren; + virDomainSnapshotObjPtr first_child; /* NULL if no children */ +}; + +virDomainSnapshotObjListPtr virDomainSnapshotObjListNew(void); +void virDomainSnapshotObjListFree(virDomainSnapshotObjListPtr snapshots); + +typedef enum { + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0, + VIR_DOMAIN_SNAPSHOT_PARSE_DISKS = 1 << 1, + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 2, +} virDomainSnapshotParseFlags; + +virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, + virCapsPtr caps, + unsigned int expectedVirtTypes, + unsigned int flags); +void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); +char *virDomainSnapshotDefFormat(const char *domain_uuid, + virDomainSnapshotDefPtr def, + unsigned int flags, + int internal); +int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot, + int default_snapshot, + bool require_match); +virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, + const virDomainSnapshotDefPtr def); + +int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + char **const names, int maxnames, + unsigned int flags); +int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + unsigned int flags); +virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, + const char *name); +void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr snapshot); +int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots, + virHashIterator iter, + void *data); +int virDomainSnapshotForEachChild(virDomainSnapshotObjPtr snapshot, + virHashIterator iter, + void *data); +int virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr snapshot, + virHashIterator iter, + void *data); +int virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots); +void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot); + +# define VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA \ + (VIR_DOMAIN_SNAPSHOT_LIST_METADATA | \ + VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA) + +# define VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES \ + (VIR_DOMAIN_SNAPSHOT_LIST_LEAVES | \ + VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES) + +# define VIR_DOMAIN_SNAPSHOT_FILTERS_ALL \ + (VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA | \ + VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES) + +int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + virDomainPtr dom, + virDomainSnapshotPtr **snaps, + unsigned int flags); + +VIR_ENUM_DECL(virDomainDiskSnapshot) +VIR_ENUM_DECL(virDomainSnapshotState) + +#endif /* __SNAPSHOT_CONF_H */
okay
index 72a7acc..e57296a 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -26,6 +26,7 @@
#include "internal.h" #include "domain_conf.h" +#include "snapshot_conf.h" #include "virauth.h" #include "util.h" #include "memory.h" diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ca62f0c..8c32a4d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -38,6 +38,7 @@ #include "domain_nwfilter.h" #include "domain_audit.h" #include "domain_conf.h" +#include "snapshot_conf.h" #include "network/bridge_driver.h" #include "virnetdevtap.h" #include "base64.h" diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b96087e..dff53cf 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1,7 +1,7 @@ /* * qemu_domain.h: QEMU domain private state * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -26,6 +26,7 @@
# include "threads.h" # include "domain_conf.h" +# include "snapshot_conf.h" # include "qemu_monitor.h" # include "qemu_agent.h" # include "qemu_conf.h" diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 4cdb11c..48f371f 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -43,6 +43,7 @@ #include "internal.h" #include "datatypes.h" #include "domain_conf.h" +#include "snapshot_conf.h" #include "network_conf.h" #include "virterror_internal.h" #include "domain_event.h"
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On 08/24/2012 12:58 AM, Daniel Veillard wrote:
On Thu, Aug 23, 2012 at 02:54:15PM -0600, Eric Blake wrote:
This has several benefits: 1. Future snapshot-related code has a definite place to go (and I _will_ be adding some) 2. Snapshot errors now use the VIR_FROM_DOMAIN_SNAPSHOT error classification, which has been underutilized (previously only in libvirt.c)
+ * 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/>. + * + * Author: Daniel P. Berrange <berrange@redhat.com>
Hum ... I would have guessed you're the author of most of that snapshot related code, not Dan, so i would probably adjust this accordingly
I wasn't the original author of domainsnapshot objects (kudos to Chris Lalancette for first introducing them), but indeed I have rewritten most of the logic in this file to the point that the original design hardly remains. I'll make the change as suggested.
ACK,
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

The name 'virDomainDiskSnapshot' didn't fit in with our normal conventions of using a prefix hinting that it is related to a virDomainSnapshotPtr. Also, a future patch will reuse the enum for declaring where the VM memory is stored. * src/conf/snapshot_conf.h (virDomainDiskSnapshot): Rename... (virDomainSnapshotLocation): ...to this. (_virDomainSnapshotDiskDef): Update clients. * src/conf/domain_conf.h (_virDomainDiskDef): Likewise. * src/libvirt_private.syms (domain_conf.h): Likewise. * src/conf/domain_conf.c (virDomainDiskDefParseXML) (virDomainDiskDefFormat): Likewise. * src/conf/snapshot_conf.c: (virDomainSnapshotDiskDefParseXML) (virDomainSnapshotAlignDisks, virDomainSnapshotDefFormat): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotDiskPrepare) (qemuDomainSnapshotCreateSingleDiskActive) (qemuDomainSnapshotCreateDiskActive, qemuDomainSnapshotCreateXML): Likewise. --- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 2 +- src/conf/snapshot_conf.c | 16 +++++++++------- src/conf/snapshot_conf.h | 16 ++++++++-------- src/libvirt_private.syms | 4 ++-- src/qemu/qemu_driver.c | 19 ++++++++++--------- 6 files changed, 34 insertions(+), 31 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e3d04a7..bcaa9b1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3766,7 +3766,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, } if (snapshot) { - def->snapshot = virDomainDiskSnapshotTypeFromString(snapshot); + def->snapshot = virDomainSnapshotLocationTypeFromString(snapshot); if (def->snapshot <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown disk snapshot setting '%s'"), @@ -3774,7 +3774,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, goto error; } } else if (def->readonly) { - def->snapshot = VIR_DOMAIN_DISK_SNAPSHOT_NO; + def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; } if (rawio) { @@ -11362,9 +11362,9 @@ virDomainDiskDefFormat(virBufferPtr buf, } } if (def->snapshot && - !(def->snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO && def->readonly)) + !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && def->readonly)) virBufferAsprintf(buf, " snapshot='%s'", - virDomainDiskSnapshotTypeToString(def->snapshot)); + virDomainSnapshotLocationTypeToString(def->snapshot)); virBufferAddLit(buf, ">\n"); if (def->driverName || def->driverType || def->cachemode || diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ef4feaa..9ee57e1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -578,7 +578,7 @@ struct _virDomainDiskDef { int ioeventfd; int event_idx; int copy_on_read; - int snapshot; /* enum virDomainDiskSnapshot, snapshot_conf.h */ + int snapshot; /* enum virDomainSnapshotLocation, snapshot_conf.h */ int startupPolicy; /* enum virDomainStartupPolicy */ unsigned int readonly : 1; unsigned int shared : 1; diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 894a74c..b9eb74c 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -51,7 +51,7 @@ #define VIR_FROM_THIS VIR_FROM_DOMAIN_SNAPSHOT -VIR_ENUM_IMPL(virDomainDiskSnapshot, VIR_DOMAIN_DISK_SNAPSHOT_LAST, +VIR_ENUM_IMPL(virDomainSnapshotLocation, VIR_DOMAIN_SNAPSHOT_LOCATION_LAST, "default", "no", "internal", @@ -120,7 +120,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, snapshot = virXMLPropString(node, "snapshot"); if (snapshot) { - def->snapshot = virDomainDiskSnapshotTypeFromString(snapshot); + def->snapshot = virDomainSnapshotLocationTypeFromString(snapshot); if (def->snapshot <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown disk snapshot setting '%s'"), @@ -144,7 +144,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, } if (!def->snapshot && (def->file || def->driverType)) - def->snapshot = VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL; + def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; ret = 0; cleanup: @@ -390,14 +390,16 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, disk->snapshot = disk_snapshot; } else if (disk_snapshot && require_match && disk->snapshot != disk_snapshot) { - const char *tmp = virDomainDiskSnapshotTypeToString(disk_snapshot); + const char *tmp; + + tmp = virDomainSnapshotLocationTypeToString(disk_snapshot); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' must use snapshot mode '%s'"), disk->name, tmp); goto cleanup; } if (disk->file && - disk->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) { + disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("file '%s' for disk '%s' requires " "use of external snapshot mode"), @@ -445,7 +447,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk = &def->disks[i]; - if (disk->snapshot == VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL && + if (disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL && !disk->file) { const char *original = def->dom->disks[i]->src; const char *tmp; @@ -534,7 +536,7 @@ char *virDomainSnapshotDefFormat(const char *domain_uuid, virBufferEscapeString(&buf, " <disk name='%s'", disk->name); if (disk->snapshot) virBufferAsprintf(&buf, " snapshot='%s'", - virDomainDiskSnapshotTypeToString(disk->snapshot)); + virDomainSnapshotLocationTypeToString(disk->snapshot)); if (disk->file || disk->driverType) { virBufferAddLit(&buf, ">\n"); if (disk->driverType) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 314c4d1..7d46a82 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -29,13 +29,13 @@ /* Items related to snapshot state */ -enum virDomainDiskSnapshot { - VIR_DOMAIN_DISK_SNAPSHOT_DEFAULT = 0, - VIR_DOMAIN_DISK_SNAPSHOT_NO, - VIR_DOMAIN_DISK_SNAPSHOT_INTERNAL, - VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, +enum virDomainSnapshotLocation { + VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT = 0, + VIR_DOMAIN_SNAPSHOT_LOCATION_NONE, + VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, - VIR_DOMAIN_DISK_SNAPSHOT_LAST + VIR_DOMAIN_SNAPSHOT_LOCATION_LAST }; enum virDomainSnapshotState { @@ -50,7 +50,7 @@ typedef virDomainSnapshotDiskDef *virDomainSnapshotDiskDefPtr; struct _virDomainSnapshotDiskDef { char *name; /* name matching the <target dev='...' of the domain */ int index; /* index within snapshot->dom->disks that matches name */ - int snapshot; /* enum virDomainDiskSnapshot */ + int snapshot; /* enum virDomainSnapshotLocation */ char *file; /* new source file when snapshot is external */ char *driverType; /* file format type of new file */ }; @@ -151,7 +151,7 @@ int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotPtr **snaps, unsigned int flags); -VIR_ENUM_DECL(virDomainDiskSnapshot) +VIR_ENUM_DECL(virDomainSnapshotLocation) VIR_ENUM_DECL(virDomainSnapshotState) #endif /* __SNAPSHOT_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c339664..27eb43e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -341,8 +341,6 @@ virDomainDiskIoTypeToString; virDomainDiskPathByName; virDomainDiskRemove; virDomainDiskRemoveByName; -virDomainDiskSnapshotTypeFromString; -virDomainDiskSnapshotTypeToString; virDomainDiskTypeFromString; virDomainDiskTypeToString; virDomainEmulatorPinAdd; @@ -485,6 +483,8 @@ virDomainSnapshotFindByName; virDomainSnapshotForEach; virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; +virDomainSnapshotLocationTypeFromString; +virDomainSnapshotLocationTypeToString; virDomainSnapshotObjListGetNames; virDomainSnapshotObjListNum; virDomainSnapshotObjListRemove; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c1cfa5a..64c407d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10558,7 +10558,7 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, virDomainSnapshotDiskDefPtr disk = &def->disks[i]; switch (disk->snapshot) { - case VIR_DOMAIN_DISK_SNAPSHOT_INTERNAL: + case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL: if (active) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("active qemu domains require external disk " @@ -10578,7 +10578,7 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, found = true; break; - case VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL: + case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: if (!disk->driverType) { if (!(disk->driverType = strdup("qcow2"))) { virReportOOMError(); @@ -10610,10 +10610,10 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, external++; break; - case VIR_DOMAIN_DISK_SNAPSHOT_NO: + case VIR_DOMAIN_SNAPSHOT_LOCATION_NONE: break; - case VIR_DOMAIN_DISK_SNAPSHOT_DEFAULT: + case VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT: default: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unexpected code path")); @@ -10668,7 +10668,7 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, char *origdriver = NULL; bool need_unlink = false; - if (snap->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) { + if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unexpected code path")); return -1; @@ -10919,7 +10919,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, for (i = 0; i < snap->def->ndisks; i++) { virDomainDiskDefPtr persistDisk = NULL; - if (snap->def->disks[i].snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO) + if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) continue; if (vm->newDef) { int indx = virDomainDiskIndexByName(vm->newDef, @@ -10949,7 +10949,8 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, while (--i >= 0) { virDomainDiskDefPtr persistDisk = NULL; - if (snap->def->disks[i].snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO) + if (snap->def->disks[i].snapshot == + VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) continue; if (vm->newDef) { int indx = virDomainDiskIndexByName(vm->newDef, @@ -11178,7 +11179,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && def->dom) { if (virDomainSnapshotAlignDisks(def, - VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, false) < 0) goto cleanup; } @@ -11199,7 +11200,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } if (virDomainSnapshotAlignDisks(def, - VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, false) < 0) goto cleanup; if (qemuDomainSnapshotDiskPrepare(vm, def, &flags) < 0) -- 1.7.11.4

On Thu, Aug 23, 2012 at 02:54:16PM -0600, Eric Blake wrote:
The name 'virDomainDiskSnapshot' didn't fit in with our normal conventions of using a prefix hinting that it is related to a virDomainSnapshotPtr. Also, a future patch will reuse the enum for declaring where the VM memory is stored.
* src/conf/snapshot_conf.h (virDomainDiskSnapshot): Rename... (virDomainSnapshotLocation): ...to this. (_virDomainSnapshotDiskDef): Update clients. * src/conf/domain_conf.h (_virDomainDiskDef): Likewise. * src/libvirt_private.syms (domain_conf.h): Likewise. * src/conf/domain_conf.c (virDomainDiskDefParseXML) (virDomainDiskDefFormat): Likewise. * src/conf/snapshot_conf.c: (virDomainSnapshotDiskDefParseXML) (virDomainSnapshotAlignDisks, virDomainSnapshotDefFormat): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotDiskPrepare) (qemuDomainSnapshotCreateSingleDiskActive) (qemuDomainSnapshotCreateDiskActive, qemuDomainSnapshotCreateXML): Likewise. --- src/conf/domain_conf.c | 8 ++++---- src/conf/domain_conf.h | 2 +- src/conf/snapshot_conf.c | 16 +++++++++------- src/conf/snapshot_conf.h | 16 ++++++++-------- src/libvirt_private.syms | 4 ++-- src/qemu/qemu_driver.c | 19 ++++++++++--------- 6 files changed, 34 insertions(+), 31 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e3d04a7..bcaa9b1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3766,7 +3766,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, }
if (snapshot) { - def->snapshot = virDomainDiskSnapshotTypeFromString(snapshot); + def->snapshot = virDomainSnapshotLocationTypeFromString(snapshot); if (def->snapshot <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown disk snapshot setting '%s'"), @@ -3774,7 +3774,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, goto error; } } else if (def->readonly) { - def->snapshot = VIR_DOMAIN_DISK_SNAPSHOT_NO; + def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; }
if (rawio) { @@ -11362,9 +11362,9 @@ virDomainDiskDefFormat(virBufferPtr buf, } } if (def->snapshot && - !(def->snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO && def->readonly)) + !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && def->readonly)) virBufferAsprintf(buf, " snapshot='%s'", - virDomainDiskSnapshotTypeToString(def->snapshot)); + virDomainSnapshotLocationTypeToString(def->snapshot)); virBufferAddLit(buf, ">\n");
if (def->driverName || def->driverType || def->cachemode || diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ef4feaa..9ee57e1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -578,7 +578,7 @@ struct _virDomainDiskDef { int ioeventfd; int event_idx; int copy_on_read; - int snapshot; /* enum virDomainDiskSnapshot, snapshot_conf.h */ + int snapshot; /* enum virDomainSnapshotLocation, snapshot_conf.h */ int startupPolicy; /* enum virDomainStartupPolicy */ unsigned int readonly : 1; unsigned int shared : 1; diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 894a74c..b9eb74c 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -51,7 +51,7 @@
#define VIR_FROM_THIS VIR_FROM_DOMAIN_SNAPSHOT
-VIR_ENUM_IMPL(virDomainDiskSnapshot, VIR_DOMAIN_DISK_SNAPSHOT_LAST, +VIR_ENUM_IMPL(virDomainSnapshotLocation, VIR_DOMAIN_SNAPSHOT_LOCATION_LAST, "default", "no", "internal", @@ -120,7 +120,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
snapshot = virXMLPropString(node, "snapshot"); if (snapshot) { - def->snapshot = virDomainDiskSnapshotTypeFromString(snapshot); + def->snapshot = virDomainSnapshotLocationTypeFromString(snapshot); if (def->snapshot <= 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("unknown disk snapshot setting '%s'"), @@ -144,7 +144,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, }
if (!def->snapshot && (def->file || def->driverType)) - def->snapshot = VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL; + def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
ret = 0; cleanup: @@ -390,14 +390,16 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, disk->snapshot = disk_snapshot; } else if (disk_snapshot && require_match && disk->snapshot != disk_snapshot) { - const char *tmp = virDomainDiskSnapshotTypeToString(disk_snapshot); + const char *tmp; + + tmp = virDomainSnapshotLocationTypeToString(disk_snapshot); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' must use snapshot mode '%s'"), disk->name, tmp); goto cleanup; } if (disk->file && - disk->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) { + disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("file '%s' for disk '%s' requires " "use of external snapshot mode"), @@ -445,7 +447,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk = &def->disks[i];
- if (disk->snapshot == VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL && + if (disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL && !disk->file) { const char *original = def->dom->disks[i]->src; const char *tmp; @@ -534,7 +536,7 @@ char *virDomainSnapshotDefFormat(const char *domain_uuid, virBufferEscapeString(&buf, " <disk name='%s'", disk->name); if (disk->snapshot) virBufferAsprintf(&buf, " snapshot='%s'", - virDomainDiskSnapshotTypeToString(disk->snapshot)); + virDomainSnapshotLocationTypeToString(disk->snapshot)); if (disk->file || disk->driverType) { virBufferAddLit(&buf, ">\n"); if (disk->driverType) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 314c4d1..7d46a82 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -29,13 +29,13 @@
/* Items related to snapshot state */
-enum virDomainDiskSnapshot { - VIR_DOMAIN_DISK_SNAPSHOT_DEFAULT = 0, - VIR_DOMAIN_DISK_SNAPSHOT_NO, - VIR_DOMAIN_DISK_SNAPSHOT_INTERNAL, - VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, +enum virDomainSnapshotLocation { + VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT = 0, + VIR_DOMAIN_SNAPSHOT_LOCATION_NONE, + VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL,
- VIR_DOMAIN_DISK_SNAPSHOT_LAST + VIR_DOMAIN_SNAPSHOT_LOCATION_LAST };
enum virDomainSnapshotState { @@ -50,7 +50,7 @@ typedef virDomainSnapshotDiskDef *virDomainSnapshotDiskDefPtr; struct _virDomainSnapshotDiskDef { char *name; /* name matching the <target dev='...' of the domain */ int index; /* index within snapshot->dom->disks that matches name */ - int snapshot; /* enum virDomainDiskSnapshot */ + int snapshot; /* enum virDomainSnapshotLocation */ char *file; /* new source file when snapshot is external */ char *driverType; /* file format type of new file */ }; @@ -151,7 +151,7 @@ int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotPtr **snaps, unsigned int flags);
-VIR_ENUM_DECL(virDomainDiskSnapshot) +VIR_ENUM_DECL(virDomainSnapshotLocation) VIR_ENUM_DECL(virDomainSnapshotState)
#endif /* __SNAPSHOT_CONF_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c339664..27eb43e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -341,8 +341,6 @@ virDomainDiskIoTypeToString; virDomainDiskPathByName; virDomainDiskRemove; virDomainDiskRemoveByName; -virDomainDiskSnapshotTypeFromString; -virDomainDiskSnapshotTypeToString; virDomainDiskTypeFromString; virDomainDiskTypeToString; virDomainEmulatorPinAdd; @@ -485,6 +483,8 @@ virDomainSnapshotFindByName; virDomainSnapshotForEach; virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; +virDomainSnapshotLocationTypeFromString; +virDomainSnapshotLocationTypeToString; virDomainSnapshotObjListGetNames; virDomainSnapshotObjListNum; virDomainSnapshotObjListRemove; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c1cfa5a..64c407d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10558,7 +10558,7 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, virDomainSnapshotDiskDefPtr disk = &def->disks[i];
switch (disk->snapshot) { - case VIR_DOMAIN_DISK_SNAPSHOT_INTERNAL: + case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL: if (active) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("active qemu domains require external disk " @@ -10578,7 +10578,7 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, found = true; break;
- case VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL: + case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: if (!disk->driverType) { if (!(disk->driverType = strdup("qcow2"))) { virReportOOMError(); @@ -10610,10 +10610,10 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, external++; break;
- case VIR_DOMAIN_DISK_SNAPSHOT_NO: + case VIR_DOMAIN_SNAPSHOT_LOCATION_NONE: break;
- case VIR_DOMAIN_DISK_SNAPSHOT_DEFAULT: + case VIR_DOMAIN_SNAPSHOT_LOCATION_DEFAULT: default: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unexpected code path")); @@ -10668,7 +10668,7 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, char *origdriver = NULL; bool need_unlink = false;
- if (snap->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) { + if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unexpected code path")); return -1; @@ -10919,7 +10919,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, for (i = 0; i < snap->def->ndisks; i++) { virDomainDiskDefPtr persistDisk = NULL;
- if (snap->def->disks[i].snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO) + if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) continue; if (vm->newDef) { int indx = virDomainDiskIndexByName(vm->newDef, @@ -10949,7 +10949,8 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, while (--i >= 0) { virDomainDiskDefPtr persistDisk = NULL;
- if (snap->def->disks[i].snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO) + if (snap->def->disks[i].snapshot == + VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) continue; if (vm->newDef) { int indx = virDomainDiskIndexByName(vm->newDef, @@ -11178,7 +11179,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && def->dom) { if (virDomainSnapshotAlignDisks(def, - VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, false) < 0) goto cleanup; } @@ -11199,7 +11200,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } if (virDomainSnapshotAlignDisks(def, - VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, false) < 0) goto cleanup; if (qemuDomainSnapshotDiskPrepare(vm, def, &flags) < 0)
ACK, the only annoyance is that the name being longuer the simple substitution leads to lines over 80 characters, I would be a proponent of dropping the indentation to the opening ( for the sake of keeping everything on one 80 char line Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On 08/24/2012 01:00 AM, Daniel Veillard wrote:
On Thu, Aug 23, 2012 at 02:54:16PM -0600, Eric Blake wrote:
The name 'virDomainDiskSnapshot' didn't fit in with our normal conventions of using a prefix hinting that it is related to a virDomainSnapshotPtr. Also, a future patch will reuse the enum for declaring where the VM memory is stored.
@@ -11199,7 +11200,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } if (virDomainSnapshotAlignDisks(def, - VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, false) < 0) goto cleanup; if (qemuDomainSnapshotDiskPrepare(vm, def, &flags) < 0)
ACK, the only annoyance is that the name being longuer the simple substitution leads to lines over 80 characters, I would be a proponent of dropping the indentation to the opening ( for the sake of keeping everything on one 80 char line
Compared to the original: VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL virDomainDiskSnapshotTypeFromString I thought of various other possibilities for shorter names, such as: VIR_DOMAIN_SNAPSHOT_STYLE_EXTERNAL virDomainSnapshotStyleTypeFromString VIR_DOMAIN_SNAPSHOT_LOC_EXTERNAL virDomainSnapshotLocTypeFromString VIR_DOMAIN_SNAPSHOT_LOCATION_EXT virDomainSnapshotLocationTypeFromString but I'm not sure that I like the abbreviation 'loc'. Should I go ahead and push the patch as-is, or go with one of the shorter names? After all, long lines are a cosmetic issue, and I'd rather code with long names that are legible than short names that leave me scratching my head. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Fri, Aug 24, 2012 at 09:49:38AM -0600, Eric Blake wrote:
On 08/24/2012 01:00 AM, Daniel Veillard wrote:
On Thu, Aug 23, 2012 at 02:54:16PM -0600, Eric Blake wrote:
The name 'virDomainDiskSnapshot' didn't fit in with our normal conventions of using a prefix hinting that it is related to a virDomainSnapshotPtr. Also, a future patch will reuse the enum for declaring where the VM memory is stored.
@@ -11199,7 +11200,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } if (virDomainSnapshotAlignDisks(def, - VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, false) < 0) goto cleanup; if (qemuDomainSnapshotDiskPrepare(vm, def, &flags) < 0)
ACK, the only annoyance is that the name being longuer the simple substitution leads to lines over 80 characters, I would be a proponent of dropping the indentation to the opening ( for the sake of keeping everything on one 80 char line
Compared to the original:
VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL virDomainDiskSnapshotTypeFromString
I thought of various other possibilities for shorter names, such as:
VIR_DOMAIN_SNAPSHOT_STYLE_EXTERNAL virDomainSnapshotStyleTypeFromString
VIR_DOMAIN_SNAPSHOT_LOC_EXTERNAL virDomainSnapshotLocTypeFromString
VIR_DOMAIN_SNAPSHOT_LOCATION_EXT virDomainSnapshotLocationTypeFromString
but I'm not sure that I like the abbreviation 'loc'.
Should I go ahead and push the patch as-is, or go with one of the shorter names? After all, long lines are a cosmetic issue, and I'd rather code with long names that are legible than short names that leave me scratching my head.
Oh, sure push :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On 08/24/2012 09:57 AM, Daniel Veillard wrote:
ACK, the only annoyance is that the name being longuer the simple substitution leads to lines over 80 characters, I would be a proponent of dropping the indentation to the opening ( for the sake of keeping everything on one 80 char line
Should I go ahead and push the patch as-is, or go with one of the shorter names? After all, long lines are a cosmetic issue, and I'd rather code with long names that are legible than short names that leave me scratching my head.
Oh, sure push :-)
Done. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel Veillard
-
Eric Blake