On 06/15/12 06:18, Eric Blake wrote:
This idea was first suggested by Daniel Veillard here:
https://www.redhat.com/archives/libvir-list/2011-October/msg00353.html
Now that I am about to add more complexity to snapshot listing, it
makes sense to avoid code duplication and special casing for domain
listing (all snapshots) vs. snapshot listing (descendants); adding
a metaroot reduces the number of code lines by having the domain
listing turn into a descendant listing of the metaroot.
Note that this has one minor pessimization - if we are going to list
ALL snapshots without filtering, then virHashForeach is more efficient
than recursing through the child relationships; restoring that minor
optimization will occur in the next patch.
* src/conf/domain_conf.h (_virDomainSnapshotObj)
(_virDomainSnapshotObjList): Repurpose some fields.
(virDomainSnapshotDropParent): Drop unused parameter.
* src/conf/domain_conf.c (virDomainSnapshotObjListGetNames)
(virDomainSnapshotObjListCount): Simplify.
(virDomainSnapshotFindByName, virDomainSnapshotSetRelations)
(virDomainSnapshotDropParent): Match new field semantics.
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML)
(qemuDomainSnapshotReparentChildren, qemuDomainSnapshotDelete):
Adjust clients.
---
v2: fix bugs in virDomainSnapshotSetRelations, rebase earlier in series
src/conf/domain_conf.c | 116 ++++++++++++------------------------------------
src/conf/domain_conf.h | 12 ++---
src/qemu/qemu_driver.c | 36 +++++----------
3 files changed, 46 insertions(+), 118 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8d80f3b..c7437af 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14289,32 +14289,9 @@ int
virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots,
char **const names, int maxnames,
unsigned int flags)
{
- struct virDomainSnapshotNameData data = { 0, 0, maxnames, names, 0 };
- int i;
-
- data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
-
- if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) {
- virHashForEach(snapshots->objs, virDomainSnapshotObjListCopyNames,
- &data);
- } else {
- virDomainSnapshotObjPtr root = snapshots->first_root;
- while (root) {
- virDomainSnapshotObjListCopyNames(root, root->def->name, &data);
- root = root->sibling;
- }
- }
- if (data.oom) {
- virReportOOMError();
- goto cleanup;
- }
-
- return data.numnames;
-
-cleanup:
- for (i = 0; i < data.numnames; i++)
- VIR_FREE(data.names[i]);
- return -1;
+ flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
I'm not quite sure what's the meaning of this statement. It efectively
negates the VIR_DOMAIN_SNAPSHOT_LIST_ROOTS flag, but the original code
filtered it out.
+ return virDomainSnapshotObjListGetNamesFrom(&snapshots->metaroot, names,
+ maxnames, flags);
This function calls virDomainSnapshotObjListCopyNames on each of
children of the object but the VIR_DOMAIN_SNAPSHOT_LIST_ROOTS is never
checked. In fact virDomainSnapshotObjListCopyNames states that:
"/* LIST_ROOTS/LIST_DESCENDANTS was handled by caller, ..."
I assume that it has to be filtered out:
flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
}
int virDomainSnapshotObjListGetNamesFrom(virDomainSnapshotObjPtr snapshot,
@@ -14369,23 +14346,8 @@ static void virDomainSnapshotObjListCount(void
*payload,
int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots,
unsigned int flags)
{
- struct virDomainSnapshotNumData data = { 0, 0 };
-
- data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
-
- if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) {
- virHashForEach(snapshots->objs, virDomainSnapshotObjListCount, &data);
- } else if (data.flags) {
- virDomainSnapshotObjPtr root = snapshots->first_root;
- while (root) {
- virDomainSnapshotObjListCount(root, root->def->name, &data);
- root = root->sibling;
- }
- } else {
- data.count = snapshots->nroots;
- }
-
- return data.count;
+ flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
+ return virDomainSnapshotObjListNumFrom(&snapshots->metaroot, flags);
Same comment as above.
}
int
ACK if the flag masking issue gets cleared. The metaroot approach is
nice and consistent.
Peter
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
