Re: [libvirt] [PATCHv2 1/3] snapshot: make virDomainSnapshotObjList opaque

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