[libvirt] [PATCHv2 00/13] list all snapshots
This is a v2 of the remaining patches in: https://www.redhat.com/archives/libvir-list/2012-June/msg00273.html https://www.redhat.com/archives/libvir-list/2012-June/msg00284.html It assumes that Peter's list all domain patches have been applied already, at least through patch 6/12: https://www.redhat.com/archives/libvir-list/2012-June/msg00316.html In other words, this series will not apply to current libvirt.git at the time of this mail. And since I developed against v3 of Peter's patches and he has some tweaks to make, I will probably have to make minor tweaks to rebase on top of his eventual v4. I will follow up with a link to a git repo with Peter's v3 and my series, and/or my rebase on top of his v4. Overall changes since v1: actually tested this time address review comments lots of bug fixes due to corner case testing rearrange the patch series so that metaroot comes before filtering pick up some good ideas from Peter's v3 More details in some of the individual patches, if I can remember everything I did. In particular, I'm quite pleased that the overall diffstat for domain_conf.c reduced in size in spite of adding features :) Eric Blake (13): snapshot: new virsh function factored from snapshot-list snapshot: use new virsh function for snapshot-list snapshot: use metaroot node to simplify management snapshot: merge domain and snapshot computation snapshot: merge count and name collectoin snapshot: add additional filters when getting lists snapshot: expose new flags in virsh list: add virDomainListAllSnapshots API list: use the new snapshot API in virsh when possible list: provide python bindings for snapshots list: provide RPC call for snapshots list: new helper function to collect snapshots list: add qemu snapshot list support daemon/remote.c | 126 ++++++ include/libvirt/libvirt.h.in | 35 +- python/Makefile.am | 2 + python/generator.py | 2 + python/libvirt-override-api.xml | 16 +- python/libvirt-override-virDomain.py | 11 + python/libvirt-override-virDomainSnapshot.py | 11 + python/libvirt-override.c | 92 +++++ src/conf/domain_conf.c | 250 ++++------- src/conf/domain_conf.h | 19 +- src/conf/virdomainlist.c | 44 +- src/conf/virdomainlist.h | 18 + src/driver.h | 12 + src/libvirt.c | 275 +++++++++++-- src/libvirt_private.syms | 3 +- src/libvirt_public.syms | 2 + src/qemu/qemu_driver.c | 137 +++++-- src/qemu/qemu_migration.c | 3 +- src/remote/remote_driver.c | 126 ++++++ src/remote/remote_protocol.x | 26 +- src/remote_protocol-structs | 26 ++ tools/virsh.c | 570 +++++++++++++++++--------- tools/virsh.pod | 14 +- 23 files changed, 1371 insertions(+), 449 deletions(-) create mode 100644 python/libvirt-override-virDomain.py create mode 100644 python/libvirt-override-virDomainSnapshot.py -- 1.7.10.2
This patch is based on the fallback code out of cmdSnapshotList, with tweaks to keep the snapshot objects around rather than just their name, and to remove unwanted elements before returning. It looks forward to a future patch when we add a way to list all snapshot objects at once, and the next patch will simplify cmdSnapshotList to take advantage of this factorization. * tools/virsh.c (vshSnapshotList, vshSnapshotListFree): New functions. --- v2: fix lots of stupid bugs, such as a qsort callback that returned 1 where -1 was meant and led to a segv. This no longer resembles the code from cmdSnapshotList quite as nicely, but I'm more confident that I have tested it across multiple server versions and all possible combinations of command line options, and across several snapshot hierarchies (linear list, all roots, random tree). tools/virsh.c | 278 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 278 insertions(+) diff --git a/tools/virsh.c b/tools/virsh.c index e3cf159..ac748d9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -16943,6 +16943,284 @@ cleanup: return ret; } +/* Helpers for collecting a list of snapshots. */ +struct vshSnap { + virDomainSnapshotPtr snap; + char *parent; +}; +struct vshSnapshotList { + struct vshSnap *snaps; + int nsnaps; +}; +typedef struct vshSnapshotList *vshSnapshotListPtr; + +static void +vshSnapshotListFree(vshSnapshotListPtr snaplist) +{ + int i; + + if (!snaplist) + return; + if (snaplist->snaps) { + for (i = 0; i < snaplist->nsnaps; i++) { + if (snaplist->snaps[i].snap) + virDomainSnapshotFree(snaplist->snaps[i].snap); + VIR_FREE(snaplist->snaps[i].parent); + } + VIR_FREE(snaplist->snaps); + } + VIR_FREE(snaplist); +} + +static int +vshSnapSorter(const void *a, const void *b) +{ + const struct vshSnap *sa = a; + const struct vshSnap *sb = b; + + if (sa->snap && !sb->snap) + return -1; + if (!sa->snap) + return sb->snap != NULL; + + /* User visible sort, so we want locale-specific case comparison. */ + return strcasecmp(virDomainSnapshotGetName(sa->snap), + virDomainSnapshotGetName(sb->snap)); +} + +/* Compute a list of snapshots from DOM. If FROM is provided, the + * list is limited to descendants of the given snapshot. If FLAGS is + * given, the list is filtered. If TREE is specified, then all but + * FROM or the roots will also have parent information. */ +static vshSnapshotListPtr ATTRIBUTE_UNUSED +vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, + virDomainSnapshotPtr from, + unsigned int flags, bool tree) +{ + int i; + char **names = NULL; + int count = -1; + bool descendants = false; + bool roots = false; + vshSnapshotListPtr snaplist = vshMalloc(ctl, sizeof(*snaplist)); + vshSnapshotListPtr ret = NULL; + const char *fromname = NULL; + int start_index = -1; + int deleted = 0; + + /* 0.9.13 will be adding a new listing API. */ + + /* This uses the interfaces available in 0.8.0-0.9.6 + * (virDomainSnapshotListNames, global list only) and in + * 0.9.7-0.9.12 (addition of virDomainSnapshotListChildrenNames + * for child listing, and new flags), as follows, with [*] by the + * combinations that need parent info (either for filtering + * purposes or for the resulting tree listing): + * old new + * list global as-is global as-is + * list --roots *global + filter global + flags + * list --from *global + filter child + flags + * list --from --descendants *global + filter child + flags + * list --tree *global as-is *global as-is + * list --tree --from *global + filter *child + flags + * + * Additionally, when --tree and --from are both used, from is + * added to the final list as the only element without a parent. + * Otherwise, --from does not appear in the final list. + */ + if (from) { + fromname = virDomainSnapshotGetName(from); + if (!fromname) { + vshError(ctl, "%s", _("Could not get snapshot name")); + goto cleanup; + } + descendants = (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) || tree; + if (tree) + flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + + /* First, determine if we can use the new child listing API. */ + if (ctl->useSnapshotOld || + ((count = virDomainSnapshotNumChildren(from, flags)) < 0 && + last_error->code == VIR_ERR_NO_SUPPORT)) { + /* We can emulate --from. */ + /* XXX can we also emulate --leaves? */ + virFreeError(last_error); + last_error = NULL; + ctl->useSnapshotOld = true; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + goto global; + } + if (tree && count >= 0) + count++; + } else { + global: + /* Global listing (including fallback when --from failed with + * child listing). */ + count = virDomainSnapshotNum(dom, flags); + + /* Fall back to simulation if --roots was unsupported. */ + /* XXX can we also emulate --leaves? */ + if (!from && count < 0 && last_error->code == VIR_ERR_INVALID_ARG && + (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { + virFreeError(last_error); + last_error = NULL; + roots = true; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + count = virDomainSnapshotNum(dom, flags); + } + } + + if (count < 0) { + if (!last_error) + vshError(ctl, _("failed to collect snapshot list")); + goto cleanup; + } + + if (!count) + goto success; + + names = vshCalloc(ctl, sizeof(*names), count); + + /* Now that we have a count, collect the list. */ + if (from && !ctl->useSnapshotOld) { + if (tree) { + if (count) + count = virDomainSnapshotListChildrenNames(from, names + 1, + count - 1, flags); + if (count >= 0) { + count++; + names[0] = vshStrdup(ctl, fromname); + } + } else { + count = virDomainSnapshotListChildrenNames(from, names, + count, flags); + } + } else { + count = virDomainSnapshotListNames(dom, names, count, flags); + } + if (count < 0) + goto cleanup; + + snaplist->snaps = vshCalloc(ctl, sizeof(*snaplist->snaps), count); + snaplist->nsnaps = count; + for (i = 0; i < count; i++) { + snaplist->snaps[i].snap = virDomainSnapshotLookupByName(dom, + names[i], 0); + if (!snaplist->snaps[i].snap) + goto cleanup; + } + + /* Collect parents when needed. With the new API, --tree and + * --from together put from as the first element without a parent; + * with the old API we still need to do a post-process filtering + * based on all parent information. */ + if (tree || (from && ctl->useSnapshotOld) || roots) { + for (i = (from && !ctl->useSnapshotOld); i < count; i++) { + if (from && ctl->useSnapshotOld && STREQ(names[i], fromname)) { + start_index = i; + if (tree) + continue; + } + if (vshGetSnapshotParent(ctl, snaplist->snaps[i].snap, + &snaplist->snaps[i].parent) < 0) + goto cleanup; + if ((from && ((tree && !snaplist->snaps[i].parent) || + (!descendants && + STRNEQ_NULLABLE(fromname, + snaplist->snaps[i].parent)))) || + (roots && snaplist->snaps[i].parent)) { + virDomainSnapshotFree(snaplist->snaps[i].snap); + snaplist->snaps[i].snap = NULL; + VIR_FREE(snaplist->snaps[i].parent); + deleted++; + } + } + } + if (tree) + goto success; + + if (ctl->useSnapshotOld && descendants) { + bool changed = false; + bool remaining = false; + + /* Make multiple passes over the list - first pass finds + * direct children and NULLs out all roots and from, remaining + * passes NULL out any undecided entry whose parent is not + * still in list. We mark known descendants by clearing + * snaps[i].parents. Sorry, this is O(n^3) - hope your + * hierarchy isn't huge. XXX Is it worth making O(n^2 log n) + * by using qsort and bsearch? */ + if (start_index < 0) { + vshError(ctl, _("snapshot %s disappeared from list"), fromname); + goto cleanup; + } + for (i = 0; i < count; i++) { + if (i == start_index || !snaplist->snaps[i].parent) { + VIR_FREE(names[i]); + virDomainSnapshotFree(snaplist->snaps[i].snap); + snaplist->snaps[i].snap = NULL; + VIR_FREE(snaplist->snaps[i].parent); + deleted++; + } else if (STREQ(snaplist->snaps[i].parent, fromname)) { + VIR_FREE(snaplist->snaps[i].parent); + changed = true; + } else { + remaining = true; + } + } + if (!changed) { + ret = vshMalloc(ctl, sizeof(*snaplist)); + goto cleanup; + } + while (changed && remaining) { + changed = remaining = false; + for (i = 0; i < count; i++) { + bool found_parent = false; + int j; + + if (!names[i] || !snaplist->snaps[i].parent) + continue; + for (j = 0; j < count; j++) { + if (!names[j] || i == j) + continue; + if (STREQ(snaplist->snaps[i].parent, names[j])) { + found_parent = true; + if (!snaplist->snaps[j].parent) + VIR_FREE(snaplist->snaps[i].parent); + else + remaining = true; + break; + } + } + if (!found_parent) { + changed = true; + VIR_FREE(names[i]); + virDomainSnapshotFree(snaplist->snaps[i].snap); + snaplist->snaps[i].snap = NULL; + VIR_FREE(snaplist->snaps[i].parent); + deleted++; + } + } + } + } + +success: + qsort(snaplist->snaps, snaplist->nsnaps, sizeof(*snaplist->snaps), + vshSnapSorter); + snaplist->nsnaps -= deleted; + + ret = snaplist; + snaplist = NULL; + +cleanup: + vshSnapshotListFree(snaplist); + if (names) + for (i = 0; i < count; i++) + VIR_FREE(names[i]); + VIR_FREE(names); + return ret; +} + /* * "snapshot-list" command */ -- 1.7.10.2
On 06/15/12 06:18, Eric Blake wrote:
This patch is based on the fallback code out of cmdSnapshotList, with tweaks to keep the snapshot objects around rather than just their name, and to remove unwanted elements before returning. It looks forward to a future patch when we add a way to list all snapshot objects at once, and the next patch will simplify cmdSnapshotList to take advantage of this factorization.
* tools/virsh.c (vshSnapshotList, vshSnapshotListFree): New functions. ---
v2: fix lots of stupid bugs, such as a qsort callback that returned 1 where -1 was meant and led to a segv. This no longer resembles the code from cmdSnapshotList quite as nicely, but I'm more confident that I have tested it across multiple server versions and all possible combinations of command line options, and across several snapshot hierarchies (linear list, all roots, random tree).
tools/virsh.c | 278 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 278 insertions(+)
diff --git a/tools/virsh.c b/tools/virsh.c index e3cf159..ac748d9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c
+ /* Collect parents when needed. With the new API, --tree and + * --from together put from as the first element without a parent; + * with the old API we still need to do a post-process filtering + * based on all parent information. */ + if (tree || (from && ctl->useSnapshotOld) || roots) { + for (i = (from && !ctl->useSnapshotOld); i < count; i++) {
That initialization of "i" isn't intuitive on the first read, but It's a correct optimalisation. ACK. Peter
Operating on a list of snapshot objects looks so much simpler. In particular, since the helper function already trimmed out irrelevant entries, we no longer have quite so many special cases on finding the first snapshot to operate on. Also, vshTreePrint no longer has a generic callback struct; both clients now pass something different according to their own needs. * tools/virsh.c (cmdSnapshotList): Use previous patches. (vshTreeArrayLookup): Rename... (vshNodeListLookup): ...now that it only has one client. (cmdNodeListDevices): Adjust caller. --- v2: rename vshNodeListLookup tools/virsh.c | 225 +++++++++++---------------------------------------------- 1 file changed, 41 insertions(+), 184 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index ac748d9..4f841fd 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -13489,15 +13489,15 @@ vshTreePrint(vshControl *ctl, vshTreeLookup lookup, void *opaque, return ret; } -struct vshTreeArray { +struct vshNodeList { char **names; char **parents; }; static const char * -vshTreeArrayLookup(int devid, bool parent, void *opaque) +vshNodeListLookup(int devid, bool parent, void *opaque) { - struct vshTreeArray *arrays = opaque; + struct vshNodeList *arrays = opaque; if (parent) return arrays->parents[devid]; return arrays->names[devid]; @@ -13552,7 +13552,7 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) qsort(&devices[0], num_devices, sizeof(char*), namesorter); if (tree) { char **parents = vshMalloc(ctl, sizeof(char *) * num_devices); - struct vshTreeArray arrays = { devices, parents }; + struct vshNodeList arrays = { devices, parents }; for (i = 0; i < num_devices; i++) { virNodeDevicePtr dev = virNodeDeviceLookupByName(ctl->conn, devices[i]); @@ -13566,7 +13566,7 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } for (i = 0 ; i < num_devices ; i++) { if (parents[i] == NULL && - vshTreePrint(ctl, vshTreeArrayLookup, &arrays, num_devices, + vshTreePrint(ctl, vshNodeListLookup, &arrays, num_devices, i) < 0) ret = false; } @@ -16992,7 +16992,7 @@ vshSnapSorter(const void *a, const void *b) * list is limited to descendants of the given snapshot. If FLAGS is * given, the list is filtered. If TREE is specified, then all but * FROM or the roots will also have parent information. */ -static vshSnapshotListPtr ATTRIBUTE_UNUSED +static vshSnapshotListPtr vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, virDomainSnapshotPtr from, unsigned int flags, bool tree) @@ -17221,6 +17221,15 @@ cleanup: return ret; } +static const char * +vshSnapshotListLookup(int id, bool parent, void *opaque) +{ + vshSnapshotListPtr snaplist = opaque; + if (parent) + return snaplist->snaps[id].parent; + return virDomainSnapshotGetName(snaplist->snaps[id].snap); +} + /* * "snapshot-list" command */ @@ -17251,11 +17260,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; bool ret = false; unsigned int flags = 0; - int parent_filter = 0; /* -1 for roots filtering, 0 for no parent - information needed, 1 for parent column */ - char **names = NULL; - char **parents = NULL; - int count = 0; + bool show_parent = false; int i; xmlDocPtr xml = NULL; xmlXPathContextPtr ctxt = NULL; @@ -17271,8 +17276,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) bool leaves = vshCommandOptBool(cmd, "leaves"); const char *from = NULL; virDomainSnapshotPtr start = NULL; - int start_index = -1; - bool descendants = false; + vshSnapshotListPtr snaplist = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -17297,7 +17301,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) _("--parent and --tree are mutually exclusive")); goto cleanup; } - parent_filter = 1; + show_parent = true; } else if (vshCommandOptBool(cmd, "roots")) { if (tree) { vshError(ctl, "%s", @@ -17324,50 +17328,21 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_LIST_METADATA; } - if (from) { - descendants = vshCommandOptBool(cmd, "descendants"); - if (descendants || tree) { - flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; - } - count = ctl->useSnapshotOld ? -1 : - virDomainSnapshotNumChildren(start, flags); - if (count < 0) { - if (ctl->useSnapshotOld || - last_error->code == VIR_ERR_NO_SUPPORT) { - /* We can emulate --from. */ - /* XXX can we also emulate --leaves? */ - virFreeError(last_error); - last_error = NULL; - ctl->useSnapshotOld = true; - flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; - count = virDomainSnapshotNum(dom, flags); - } - } else if (tree) { - count++; - } - } else { - count = virDomainSnapshotNum(dom, flags); - - /* Fall back to simulation if --roots was unsupported. */ - /* XXX can we also emulate --leaves? */ - if (count < 0 && last_error->code == VIR_ERR_INVALID_ARG && - (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { - virFreeError(last_error); - last_error = NULL; - parent_filter = -1; - flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; - count = virDomainSnapshotNum(dom, flags); + if (vshCommandOptBool(cmd, "descendants")) { + if (!from) { + vshError(ctl, "%s", + _("--descendants requires either --from or --current")); + goto cleanup; } + flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; } - if (count < 0) { - if (!last_error) - vshError(ctl, _("missing support")); + if ((snaplist = vshSnapshotListCollect(ctl, dom, start, flags, + tree)) == NULL) goto cleanup; - } if (!tree) { - if (parent_filter > 0) + if (show_parent) vshPrintExtra(ctl, " %-20s %-25s %-15s %s", _("Name"), _("Creation Time"), _("State"), _("Parent")); @@ -17378,142 +17353,35 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) "------------------------------------------------------------\n"); } - if (!count) { + if (!snaplist->nsnaps) { ret = true; goto cleanup; } - if (VIR_ALLOC_N(names, count) < 0) - goto cleanup; - - if (from && !ctl->useSnapshotOld) { - /* When mixing --from and --tree, we want to start the tree at the - * given snapshot. Without --tree, only list the children. */ - if (tree) { - if (count) - count = virDomainSnapshotListChildrenNames(start, names + 1, - count - 1, flags); - if (count >= 0) { - count++; - names[0] = vshStrdup(ctl, from); - } - } else { - count = virDomainSnapshotListChildrenNames(start, names, - count, flags); - } - } else { - count = virDomainSnapshotListNames(dom, names, count, flags); - } - if (count < 0) - goto cleanup; - - qsort(&names[0], count, sizeof(char*), namesorter); - - if (tree || (from && ctl->useSnapshotOld)) { - parents = vshCalloc(ctl, sizeof(char *), count); - for (i = (from && !ctl->useSnapshotOld); i < count; i++) { - if (from && ctl->useSnapshotOld && STREQ(names[i], from)) { - start_index = i; - continue; - } - - /* free up memory from previous iterations of the loop */ - if (snapshot) - virDomainSnapshotFree(snapshot); - snapshot = virDomainSnapshotLookupByName(dom, names[i], 0); - if (!snapshot || - vshGetSnapshotParent(ctl, snapshot, &parents[i]) < 0) { - while (--i >= 0) - VIR_FREE(parents[i]); - VIR_FREE(parents); - goto cleanup; - } - } - } if (tree) { - struct vshTreeArray arrays = { names, parents }; - - for (i = 0 ; i < count ; i++) { - if ((from && ctl->useSnapshotOld) ? STREQ(names[i], from) : - !parents[i] && - vshTreePrint(ctl, vshTreeArrayLookup, &arrays, count, i) < 0) + for (i = 0; i < snaplist->nsnaps; i++) { + if (!snaplist->snaps[i].parent && + vshTreePrint(ctl, vshSnapshotListLookup, snaplist, + snaplist->nsnaps, i) < 0) goto cleanup; } - ret = true; goto cleanup; } - if (ctl->useSnapshotOld && descendants) { - bool changed = false; - - /* Make multiple passes over the list - first pass NULLs out - * all roots except start, remaining passes NULL out any entry - * whose parent is not still in list. Also, we NULL out - * parent when name is known to be in list. Sorry, this is - * O(n^3) - hope your hierarchy isn't huge. */ - if (start_index < 0) { - vshError(ctl, _("snapshot %s disappeared from list"), from); - goto cleanup; - } - for (i = 0; i < count; i++) { - if (i == start_index) - continue; - if (!parents[i]) { - VIR_FREE(names[i]); - } else if (STREQ(parents[i], from)) { - VIR_FREE(parents[i]); - changed = true; - } - } - if (!changed) { - ret = true; - goto cleanup; - } - while (changed) { - changed = false; - for (i = 0; i < count; i++) { - bool found = false; - int j; - - if (!names[i] || !parents[i]) - continue; - for (j = 0; j < count; j++) { - if (!names[j] || i == j) - continue; - if (STREQ(parents[i], names[j])) { - found = true; - if (!parents[j]) - VIR_FREE(parents[i]); - break; - } - } - if (!found) { - changed = true; - VIR_FREE(names[i]); - } - } - } - VIR_FREE(names[start_index]); - } - - for (i = 0; i < count; i++) { - if (from && ctl->useSnapshotOld && - (descendants ? !names[i] : STRNEQ_NULLABLE(parents[i], from))) - continue; + for (i = 0; i < snaplist->nsnaps; i++) { + const char *name; /* free up memory from previous iterations of the loop */ VIR_FREE(parent); VIR_FREE(state); - if (snapshot) - virDomainSnapshotFree(snapshot); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); VIR_FREE(doc); - snapshot = virDomainSnapshotLookupByName(dom, names[i], 0); - if (snapshot == NULL) - continue; + snapshot = snaplist->snaps[i].snap; + name = virDomainSnapshotGetName(snapshot); + assert(name); doc = virDomainSnapshotGetXMLDesc(snapshot, 0); if (!doc) @@ -17523,12 +17391,9 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (!xml) continue; - if (parent_filter) { + if (show_parent) parent = virXPathString("string(/domainsnapshot/parent/name)", ctxt); - if (!parent && parent_filter < 0) - continue; - } state = virXPathString("string(/domainsnapshot/state)", ctxt); if (state == NULL) @@ -17547,31 +17412,23 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (parent) vshPrint(ctl, " %-20s %-25s %-15s %s\n", - names[i], timestr, state, parent); + name, timestr, state, parent); else - vshPrint(ctl, " %-20s %-25s %s\n", names[i], timestr, state); + vshPrint(ctl, " %-20s %-25s %s\n", name, timestr, state); } ret = true; cleanup: /* this frees up memory from the last iteration of the loop */ + vshSnapshotListFree(snaplist); VIR_FREE(parent); VIR_FREE(state); - if (snapshot) - virDomainSnapshotFree(snapshot); if (start) virDomainSnapshotFree(start); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); VIR_FREE(doc); - for (i = 0; i < count; i++) { - VIR_FREE(names[i]); - if (parents) - VIR_FREE(parents[i]); - } - VIR_FREE(names); - VIR_FREE(parents); if (dom) virDomainFree(dom); -- 1.7.10.2
On 06/15/12 06:18, Eric Blake wrote:
Operating on a list of snapshot objects looks so much simpler. In particular, since the helper function already trimmed out irrelevant entries, we no longer have quite so many special cases on finding the first snapshot to operate on. Also, vshTreePrint no longer has a generic callback struct; both clients now pass something different according to their own needs.
* tools/virsh.c (cmdSnapshotList): Use previous patches. (vshTreeArrayLookup): Rename... (vshNodeListLookup): ...now that it only has one client. (cmdNodeListDevices): Adjust caller. ---
v2: rename vshNodeListLookup
tools/virsh.c | 225 +++++++++++---------------------------------------------- 1 file changed, 41 insertions(+), 184 deletions(-)
ACK; the listing functions look much better after splitting out getter code. Peter
On 06/15/2012 03:38 AM, Peter Krempa wrote:
On 06/15/12 06:18, Eric Blake wrote:
Operating on a list of snapshot objects looks so much simpler. In particular, since the helper function already trimmed out irrelevant entries, we no longer have quite so many special cases on finding the first snapshot to operate on. Also, vshTreePrint no longer has a generic callback struct; both clients now pass something different according to their own needs.
* tools/virsh.c (cmdSnapshotList): Use previous patches. (vshTreeArrayLookup): Rename... (vshNodeListLookup): ...now that it only has one client. (cmdNodeListDevices): Adjust caller. ---
v2: rename vshNodeListLookup
tools/virsh.c | 225 +++++++++++---------------------------------------------- 1 file changed, 41 insertions(+), 184 deletions(-)
ACK; the listing functions look much better after splitting out getter code.
Patches 1 and 2 were independent from your series, so I've pushed them now. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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; + return virDomainSnapshotObjListGetNamesFrom(&snapshots->metaroot, names, + maxnames, flags); } 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); } int @@ -14413,7 +14375,7 @@ virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, const char *name) { - return virHashLookup(snapshots->objs, name); + return name ? virHashLookup(snapshots->objs, name) : &snapshots->metaroot; } void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, @@ -14499,34 +14461,27 @@ virDomainSnapshotSetRelations(void *payload, struct snapshot_set_relation *curr = data; virDomainSnapshotObjPtr tmp; - if (obj->def->parent) { - obj->parent = virDomainSnapshotFindByName(curr->snapshots, - obj->def->parent); - if (!obj->parent) { - curr->err = -1; - VIR_WARN("snapshot %s lacks parent", obj->def->name); - } else { - tmp = obj->parent; - while (tmp) { - if (tmp == obj) { - curr->err = -1; - obj->parent = NULL; - VIR_WARN("snapshot %s in circular chain", obj->def->name); - break; - } - tmp = tmp->parent; - } - if (!tmp) { - obj->parent->nchildren++; - obj->sibling = obj->parent->first_child; - obj->parent->first_child = obj; + 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; } - } else { - curr->snapshots->nroots++; - obj->sibling = curr->snapshots->first_root; - curr->snapshots->first_root = obj; } + 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 @@ -14546,28 +14501,13 @@ virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots) * of a parent, it is faster to just 0 the count rather than calling * this function on each child. */ void -virDomainSnapshotDropParent(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr snapshot) +virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot) { virDomainSnapshotObjPtr prev = NULL; virDomainSnapshotObjPtr curr = NULL; - size_t *count; - virDomainSnapshotObjPtr *first; - if (snapshot->parent) { - count = &snapshot->parent->nchildren; - first = &snapshot->parent->first_child; - } else { - count = &snapshots->nroots; - first = &snapshots->first_root; - } - - if (!*count || !*first) { - VIR_WARN("inconsistent snapshot relations"); - return; - } - (*count)--; - curr = *first; + snapshot->parent->nchildren--; + curr = snapshot->parent->first_child; while (curr != snapshot) { if (!curr) { VIR_WARN("inconsistent snapshot relations"); @@ -14579,7 +14519,7 @@ virDomainSnapshotDropParent(virDomainSnapshotObjListPtr snapshots, if (prev) prev->sibling = snapshot->sibling; else - *first = snapshot->sibling; + snapshot->parent->first_child = snapshot->sibling; snapshot->parent = NULL; snapshot->sibling = NULL; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 88674eb..e3a3679 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1726,9 +1726,11 @@ struct _virDomainSnapshotDef { typedef struct _virDomainSnapshotObj virDomainSnapshotObj; typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; struct _virDomainSnapshotObj { - virDomainSnapshotDefPtr def; + virDomainSnapshotDefPtr def; /* non-NULL except for metaroot */ - virDomainSnapshotObjPtr parent; /* NULL if root */ + 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 */ @@ -1741,8 +1743,7 @@ struct _virDomainSnapshotObjList { * for O(1), lockless lookup-by-name */ virHashTable *objs; - size_t nroots; - virDomainSnapshotObjPtr first_root; + virDomainSnapshotObj metaroot; /* Special parent of all root snapshots */ }; typedef enum { @@ -1788,8 +1789,7 @@ int virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr snapshot, virHashIterator iter, void *data); int virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots); -void virDomainSnapshotDropParent(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr snapshot); +void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot); /* Guest VM runtime state */ typedef struct _virDomainStateReason virDomainStateReason; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a1b76c2..7038a4c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10480,7 +10480,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } /* Drop and rebuild the parent relationship, but keep all * child relations by reusing snap. */ - virDomainSnapshotDropParent(&vm->snapshots, other); + virDomainSnapshotDropParent(other); virDomainSnapshotDefFree(other->def); other->def = NULL; snap = other; @@ -10589,18 +10589,12 @@ cleanup: } else { if (update_current) vm->current_snapshot = snap; - if (snap->def->parent) { - other = virDomainSnapshotFindByName(&vm->snapshots, - snap->def->parent); - snap->parent = other; - other->nchildren++; - snap->sibling = other->first_child; - other->first_child = snap; - } else { - vm->snapshots.nroots++; - snap->sibling = vm->snapshots.first_root; - vm->snapshots.first_root = snap; - } + other = virDomainSnapshotFindByName(&vm->snapshots, + snap->def->parent); + snap->parent = other; + other->nchildren++; + snap->sibling = other->first_child; + other->first_child = snap; } } else if (snap) { virDomainSnapshotObjListRemove(&vm->snapshots, snap); @@ -11405,7 +11399,7 @@ qemuDomainSnapshotReparentChildren(void *payload, VIR_FREE(snap->def->parent); snap->parent = rep->parent; - if (rep->parent) { + if (rep->parent->def) { snap->def->parent = strdup(rep->parent->def->name); if (snap->def->parent == NULL) { @@ -11513,15 +11507,9 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, if (rep.err < 0) goto endjob; /* Can't modify siblings during ForEachChild, so do it now. */ - if (snap->parent) { - snap->parent->nchildren += snap->nchildren; - rep.last->sibling = snap->parent->first_child; - snap->parent->first_child = snap->first_child; - } else { - vm->snapshots.nroots += snap->nchildren; - rep.last->sibling = vm->snapshots.first_root; - vm->snapshots.first_root = snap->first_child; - } + snap->parent->nchildren += snap->nchildren; + rep.last->sibling = snap->parent->first_child; + snap->parent->first_child = snap->first_child; } if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { @@ -11529,7 +11517,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, snap->first_child = NULL; ret = 0; } else { - virDomainSnapshotDropParent(&vm->snapshots, snap); + virDomainSnapshotDropParent(snap); ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); } -- 1.7.10.2
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
On 06/15/2012 04:23 AM, Peter Krempa wrote:
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.
+ 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.
^= toggles, not clears. Due to (in hind-sight, a rather poor) choice on my part in 0.9.7 when adding the ability to list children, we are stuck with the following two bits with opposite meanings: Bit 1 is named either VIR_DOMAIN_SNAPSHOT_LIST_ROOTS or VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS. When listing a domain, you use _ROOTS, with the following effect: virDomainSnapshotNum(,0) - list all snapshots (effectively, all descendants of the metaroot) virDomainSnapshotNum(,_ROOTS) - list all roots (effectively, the direct children of the metaroot) When listing a snapshot, you use _DESCENDANTS, with the following effect: virDomainSnapshotNumChildren(,0) - list direct children virDomainSnapshotNumChildren(,_DESCENDANTS) - list all descendants So starting from a domain, we want to toggle the bit into something where we can then start from the metaroot snapshot. I'll add a comment.
+ 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, ..."
And this is precisely the case. Since _ROOTS/_DESCENDANTS is bit one in either case, in the caller we have: _DESCENDANTS was specified (perhaps because _ROOTS was omitted): call ForEachDescendant else 0 (direct children, perhaps because _ROOTS was supplied) call ForEachChild
I assume that it has to be filtered out: flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
In other words, our choice of _which_ iterator to call determined whether we have filtered the list according to _ROOTS/_DESCENDANTS, and the callback doesn't have to care about the state of the bit.
int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, unsigned int flags)
+ flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + return virDomainSnapshotObjListNumFrom(&snapshots->metaroot, flags);
Same comment as above.
Same answer as above. :)
ACK if the flag masking issue gets cleared. The metaroot approach is nice and consistent.
Then, assuming my explanation is okay, I will resolve this by adding more comments to the code. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/15/2012 10:21 AM, Eric Blake wrote:
+ 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.
^= toggles, not clears. Due to (in hind-sight, a rather poor) choice on my part in 0.9.7 when adding the ability to list children, we are stuck with the following two bits with opposite meanings:
ACK if the flag masking issue gets cleared. The metaroot approach is nice and consistent.
It turns out that 3-5 are also independent of Peter's series, so I will push those shortly as well.
Then, assuming my explanation is okay, I will resolve this by adding more comments to the code.
Here's what I'm squashing in. diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index c7437af..43872d1 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -14272,8 +14272,9 @@ static void virDomainSnapshotObjListCopyNames(void *payload, if (data->oom) return; - /* LIST_ROOTS/LIST_DESCENDANTS was handled by caller, - * LIST_METADATA is a no-op if we get this far. */ + /* LIST_ROOTS/LIST_DESCENDANTS was handled by the choice of + * iteration made in the caller, and LIST_METADATA is a no-op if + * we get this far. */ if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) && obj->nchildren) return; @@ -14289,6 +14290,9 @@ int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, char **const names, int maxnames, unsigned int flags) { + /* 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; return virDomainSnapshotObjListGetNamesFrom(&snapshots->metaroot, names, maxnames, flags); @@ -14336,8 +14340,9 @@ static void virDomainSnapshotObjListCount(void *payload, virDomainSnapshotObjPtr obj = payload; struct virDomainSnapshotNumData *data = opaque; - /* LIST_ROOTS/LIST_DESCENDANTS was handled by caller, - * LIST_METADATA is a no-op if we get this far. */ + /* LIST_ROOTS/LIST_DESCENDANTS was handled by the choice of + * iteration made in the caller, and LIST_METADATA is a no-op if + * we get this far. */ if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) && obj->nchildren) return; data->count++; @@ -14346,6 +14351,9 @@ static void virDomainSnapshotObjListCount(void *payload, int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, unsigned int flags) { + /* 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; return virDomainSnapshotObjListNumFrom(&snapshots->metaroot, flags); } -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Now that domain listing is a thin wrapper around child listing, it's easier to have a common entry point. This restores the hashForEach optimization lost in the previous patch when there are no snapshots being filtered out of the entire list. * src/conf/domain_conf.h (virDomainSnapshotObjListGetNames) (virDomainSnapshotObjListNum): Add parameter. (virDomainSnapshotObjListGetNamesFrom) (virDomainSnapshotObjListNumFrom): Delete. * src/libvirt_private.syms (domain_conf.h): Drop deleted functions. * src/conf/domain_conf.c (virDomainSnapshotObjListGetNames): Merge, and (re)add an optimization. * src/qemu/qemu_driver.c (qemuDomainUndefineFlags) (qemuDomainSnapshotListNames, qemuDomainSnapshotNum) (qemuDomainSnapshotListChildrenNames) (qemuDomainSnapshotNumChildren): Update callers. * src/qemu/qemu_migration.c (qemuMigrationIsAllowed): Likewise. * src/conf/virdomainlist.c (virDomainListPopulate): Likewise. --- v2: fix logic on when to optimize, rebase earlier in series src/conf/domain_conf.c | 76 ++++++++++++++++++++++++--------------------- src/conf/domain_conf.h | 7 ++--- src/conf/virdomainlist.c | 2 +- src/libvirt_private.syms | 2 -- src/qemu/qemu_driver.c | 11 ++++--- src/qemu/qemu_migration.c | 3 +- 6 files changed, 52 insertions(+), 49 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c7437af..adc3b3c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14285,31 +14285,34 @@ static void virDomainSnapshotObjListCopyNames(void *payload, } } -int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, - char **const names, int maxnames, - unsigned int flags) -{ - flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; - return virDomainSnapshotObjListGetNamesFrom(&snapshots->metaroot, names, - maxnames, flags); -} - -int virDomainSnapshotObjListGetNamesFrom(virDomainSnapshotObjPtr snapshot, - char **const names, int maxnames, - unsigned int flags) +int +virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + char **const names, int maxnames, + unsigned int flags) { struct virDomainSnapshotNameData data = { 0, 0, maxnames, names, 0 }; int i; + if (!from) { + flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + from = &snapshots->metaroot; + } + data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; - if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) - virDomainSnapshotForEachDescendant(snapshot, - virDomainSnapshotObjListCopyNames, - &data); - else - virDomainSnapshotForEachChild(snapshot, + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) { + if (from->def) + virDomainSnapshotForEachDescendant(from, + virDomainSnapshotObjListCopyNames, + &data); + else + virHashForEach(snapshots->objs, virDomainSnapshotObjListCopyNames, + &data); + } else { + virDomainSnapshotForEachChild(from, virDomainSnapshotObjListCopyNames, &data); + } if (data.oom) { virReportOOMError(); @@ -14343,30 +14346,33 @@ static void virDomainSnapshotObjListCount(void *payload, data->count++; } -int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, - unsigned int flags) -{ - flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; - return virDomainSnapshotObjListNumFrom(&snapshots->metaroot, flags); -} - int -virDomainSnapshotObjListNumFrom(virDomainSnapshotObjPtr snapshot, - unsigned int flags) +virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + unsigned int flags) { struct virDomainSnapshotNumData data = { 0, 0 }; + if (!from) { + flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + from = &snapshots->metaroot; + } + data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; - if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) - virDomainSnapshotForEachDescendant(snapshot, - virDomainSnapshotObjListCount, - &data); - else if (data.flags) - virDomainSnapshotForEachChild(snapshot, + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) { + if (data.flags || from->def) + virDomainSnapshotForEachDescendant(from, + virDomainSnapshotObjListCount, + &data); + else + data.count = virHashSize(snapshots->objs); + } else if (data.flags) { + virDomainSnapshotForEachChild(from, virDomainSnapshotObjListCount, &data); - else - data.count = snapshot->nchildren; + } else { + data.count = from->nchildren; + } return data.count; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index e3a3679..86c1e63 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1769,15 +1769,12 @@ virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr s int virDomainSnapshotObjListInit(virDomainSnapshotObjListPtr objs); int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, char **const names, int maxnames, unsigned int flags); int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, unsigned int flags); -int virDomainSnapshotObjListGetNamesFrom(virDomainSnapshotObjPtr snapshot, - char **const names, int maxnames, - unsigned int flags); -int virDomainSnapshotObjListNumFrom(virDomainSnapshotObjPtr snapshot, - unsigned int flags); virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, const char *name); void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, diff --git a/src/conf/virdomainlist.c b/src/conf/virdomainlist.c index 8889fee..246b838 100644 --- a/src/conf/virdomainlist.c +++ b/src/conf/virdomainlist.c @@ -106,7 +106,7 @@ virDomainListPopulate(void *payload, /* filter by snapshot existence */ if (MATCH(VIR_CONNECT_LIST_FILTERS_SNAPSHOT)) { - int nsnap = virDomainSnapshotObjListNum(&vm->snapshots, 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/libvirt_private.syms b/src/libvirt_private.syms index 5a94b8b..b37fe68 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -457,9 +457,7 @@ virDomainSnapshotFindByName; virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; virDomainSnapshotObjListGetNames; -virDomainSnapshotObjListGetNamesFrom; virDomainSnapshotObjListNum; -virDomainSnapshotObjListNumFrom; virDomainSnapshotObjListRemove; virDomainSnapshotStateTypeFromString; virDomainSnapshotStateTypeToString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7038a4c..ce90ddf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5105,7 +5105,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, } if (!virDomainObjIsActive(vm) && - (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { + (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, NULL, 0))) { if (!(flags & VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA)) { qemuReportError(VIR_ERR_OPERATION_INVALID, _("cannot delete inactive domain with %d " @@ -10629,7 +10629,7 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, goto cleanup; } - n = virDomainSnapshotObjListGetNames(&vm->snapshots, names, nameslen, + n = virDomainSnapshotObjListGetNames(&vm->snapshots, NULL, names, nameslen, flags); cleanup: @@ -10664,7 +10664,7 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, * VIR_DOMAIN_SNAPSHOT_LIST_METADATA makes no difference to our * answer. */ - n = virDomainSnapshotObjListNum(&vm->snapshots, flags); + n = virDomainSnapshotObjListNum(&vm->snapshots, NULL, flags); cleanup: if (vm) @@ -10706,7 +10706,8 @@ qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, goto cleanup; } - n = virDomainSnapshotObjListGetNamesFrom(snap, names, nameslen, flags); + n = virDomainSnapshotObjListGetNames(&vm->snapshots, snap, names, nameslen, + flags); cleanup: if (vm) @@ -10750,7 +10751,7 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, * VIR_DOMAIN_SNAPSHOT_LIST_METADATA makes no difference to our * answer. */ - n = virDomainSnapshotObjListNumFrom(snap, flags); + n = virDomainSnapshotObjListNum(&vm->snapshots, snap, flags); cleanup: if (vm) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b893fd5..48369d6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -807,7 +807,8 @@ qemuMigrationIsAllowed(struct qemud_driver *driver, virDomainObjPtr vm, "%s", _("domain is marked for auto destroy")); return false; } - if ((nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { + if ((nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, NULL, + 0))) { qemuReportError(VIR_ERR_OPERATION_INVALID, _("cannot migrate domain with %d snapshots"), nsnapshots); -- 1.7.10.2
On 06/15/12 06:18, Eric Blake wrote:
Now that domain listing is a thin wrapper around child listing, it's easier to have a common entry point. This restores the hashForEach optimization lost in the previous patch when there are no snapshots being filtered out of the entire list.
* src/conf/domain_conf.h (virDomainSnapshotObjListGetNames) (virDomainSnapshotObjListNum): Add parameter. (virDomainSnapshotObjListGetNamesFrom) (virDomainSnapshotObjListNumFrom): Delete. * src/libvirt_private.syms (domain_conf.h): Drop deleted functions. * src/conf/domain_conf.c (virDomainSnapshotObjListGetNames): Merge, and (re)add an optimization. * src/qemu/qemu_driver.c (qemuDomainUndefineFlags) (qemuDomainSnapshotListNames, qemuDomainSnapshotNum) (qemuDomainSnapshotListChildrenNames) (qemuDomainSnapshotNumChildren): Update callers. * src/qemu/qemu_migration.c (qemuMigrationIsAllowed): Likewise. * src/conf/virdomainlist.c (virDomainListPopulate): Likewise. ---
ACK. Peter
Another case where we can do the same amount of work with fewer lines of redundant code, which will make adding new filters easier. * src/conf/domain_conf.c (virDomainSnapshotNameData): Adjust struct. (virDomainSnapshotObjListCount): Delete, now taken care of... (virDomainSnapshotObjListCopyNames): ...here. (virDomainSnapshotObjListGetNames): Adjust caller to handle counting. (virDomainSnapshotObjListNum): Simplify. --- v2: new patch src/conf/domain_conf.c | 90 ++++++++++++++---------------------------------- 1 file changed, 25 insertions(+), 65 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index adc3b3c..6967557 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14256,11 +14256,11 @@ virDomainSnapshotObjListDeinit(virDomainSnapshotObjListPtr snapshots) } struct virDomainSnapshotNameData { - int oom; - int numnames; - int maxnames; char **const names; + int maxnames; unsigned int flags; + int count; + bool error; }; static void virDomainSnapshotObjListCopyNames(void *payload, @@ -14270,19 +14270,20 @@ static void virDomainSnapshotObjListCopyNames(void *payload, virDomainSnapshotObjPtr obj = payload; struct virDomainSnapshotNameData *data = opaque; - if (data->oom) + if (data->error) return; /* LIST_ROOTS/LIST_DESCENDANTS was handled by caller, * LIST_METADATA is a no-op if we get this far. */ if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) && obj->nchildren) return; - if (data->numnames < data->maxnames) { - if (!(data->names[data->numnames] = strdup(obj->def->name))) - data->oom = 1; - else - data->numnames++; + if (data->names && data->count < data->maxnames && + !(data->names[data->count] = strdup(obj->def->name))) { + data->error = true; + virReportOOMError(); + return; } + data->count++; } int @@ -14291,7 +14292,8 @@ virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, char **const names, int maxnames, unsigned int flags) { - struct virDomainSnapshotNameData data = { 0, 0, maxnames, names, 0 }; + struct virDomainSnapshotNameData data = { names, maxnames, flags, 0, + false }; int i; if (!from) { @@ -14299,51 +14301,32 @@ virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, from = &snapshots->metaroot; } - data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + data.flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) { if (from->def) virDomainSnapshotForEachDescendant(from, virDomainSnapshotObjListCopyNames, &data); - else + else if (names || data.flags) virHashForEach(snapshots->objs, virDomainSnapshotObjListCopyNames, &data); - } else { + else + data.count = virHashSize(snapshots->objs); + } else if (names || data.flags) { virDomainSnapshotForEachChild(from, virDomainSnapshotObjListCopyNames, &data); + } else { + data.count = from->nchildren; } - if (data.oom) { - virReportOOMError(); - goto cleanup; + if (data.error) { + for (i = 0; i < data.count; i++) + VIR_FREE(names[i]); + return -1; } - return data.numnames; - -cleanup: - for (i = 0; i < data.numnames; i++) - VIR_FREE(data.names[i]); - return -1; -} - -struct virDomainSnapshotNumData { - int count; - unsigned int flags; -}; - -static void virDomainSnapshotObjListCount(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *opaque) -{ - virDomainSnapshotObjPtr obj = payload; - struct virDomainSnapshotNumData *data = opaque; - - /* LIST_ROOTS/LIST_DESCENDANTS was handled by caller, - * LIST_METADATA is a no-op if we get this far. */ - if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) && obj->nchildren) - return; - data->count++; + return data.count; } int @@ -14351,30 +14334,7 @@ virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr from, unsigned int flags) { - struct virDomainSnapshotNumData data = { 0, 0 }; - - if (!from) { - flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; - from = &snapshots->metaroot; - } - - data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; - - if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) { - if (data.flags || from->def) - virDomainSnapshotForEachDescendant(from, - virDomainSnapshotObjListCount, - &data); - else - data.count = virHashSize(snapshots->objs); - } else if (data.flags) { - virDomainSnapshotForEachChild(from, - virDomainSnapshotObjListCount, &data); - } else { - data.count = from->nchildren; - } - - return data.count; + return virDomainSnapshotObjListGetNames(snapshots, from, NULL, 0, flags); } virDomainSnapshotObjPtr -- 1.7.10.2
On 06/15/12 06:18, Eric Blake wrote:
Another case where we can do the same amount of work with fewer lines of redundant code, which will make adding new filters easier.
* src/conf/domain_conf.c (virDomainSnapshotNameData): Adjust struct. (virDomainSnapshotObjListCount): Delete, now taken care of... (virDomainSnapshotObjListCopyNames): ...here. (virDomainSnapshotObjListGetNames): Adjust caller to handle counting. (virDomainSnapshotObjListNum): Simplify. ---
ACK. Peter
It turns out that one-bit filtering makes it hard to select the inverse set, so it is easier to provide filtering groups. For back-compat, omitting all bits within a group means the group is not used for filtering, and by definition of a group (each snapshot matches exactly one bit within the group, and the set of bits in the group covers all snapshots), selecting all bits also makes the group useless. Unfortunately, virDomainSnapshotListChildren defined the bit VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS as an expansion rather than a filter, so we cannot make it part of a filter group, so that bit (and its counterpart VIR_DOMAIN_SNAPSHOT_LIST_ROOTS for virDomainSnapshotList) remains a single control bit. * include/libvirt/libvirt.h.in (virDomainSnapshotListFlags): Add a couple more flags. * src/libvirt.c (virDomainSnapshotNum) (virDomainSnapshotNumChildren): Document them. (virDomainSnapshotListNames, virDomainSnapshotListChildrenNames): Likewise, and add thread-safety caveats. * src/conf/virdomainlist.h (VIR_DOMAIN_SNAPSHOT_FILTERS_*): New convenience macros. * src/conf/domain_conf.c (virDomainSnapshotObjListCopyNames) (virDomainSnapshotObjListCount): Support the new flags. --- v2: add constants to virdomainlist.h, rebase on top of metaroot for only one place to add new filters include/libvirt/libvirt.h.in | 22 ++++++-- src/conf/domain_conf.c | 24 +++++++- src/conf/virdomainlist.h | 12 ++++ src/libvirt.c | 126 ++++++++++++++++++++++++++++++------------ 4 files changed, 144 insertions(+), 40 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index ed0ca2b..e1e8cbb 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3384,10 +3384,16 @@ virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain, char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags); -/* Flags valid for virDomainSnapshotNum(), +/** + * virDomainSnapshotListFlags: + * + * Flags valid for virDomainSnapshotNum(), * virDomainSnapshotListNames(), virDomainSnapshotNumChildren(), and * virDomainSnapshotListChildrenNames(). Note that the interpretation - * of flag (1<<0) depends on which function it is passed to. */ + * of flag (1<<0) depends on which function it is passed to; but serves + * to toggle the per-call default of whether the listing is shallow or + * recursive. Remaining bits come in groups; if all bits from a group are + * 0, then that group is not used to filter results. */ typedef enum { VIR_DOMAIN_SNAPSHOT_LIST_ROOTS = (1 << 0), /* Filter by snapshots with no parents, when @@ -3395,10 +3401,18 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS = (1 << 0), /* List all descendants, not just children, when listing a snapshot */ - VIR_DOMAIN_SNAPSHOT_LIST_METADATA = (1 << 1), /* Filter by snapshots - which have metadata */ + + /* For historical reasons, groups do not use contiguous bits. */ + VIR_DOMAIN_SNAPSHOT_LIST_LEAVES = (1 << 2), /* Filter by snapshots with no children */ + VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES = (1 << 3), /* Filter by snapshots + that have children */ + + VIR_DOMAIN_SNAPSHOT_LIST_METADATA = (1 << 1), /* Filter by snapshots + which have metadata */ + VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA = (1 << 4), /* Filter by snapshots + with no metadata */ } virDomainSnapshotListFlags; /* Return the number of snapshots for this domain */ diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6967557..d629b67 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -52,6 +52,7 @@ #include "secret_conf.h" #include "netdev_vport_profile_conf.h" #include "netdev_bandwidth_conf.h" +#include "virdomainlist.h" #define VIR_FROM_THIS VIR_FROM_DOMAIN @@ -14272,10 +14273,11 @@ static void virDomainSnapshotObjListCopyNames(void *payload, if (data->error) return; - /* LIST_ROOTS/LIST_DESCENDANTS was handled by caller, - * LIST_METADATA is a no-op if we get this far. */ + /* Caller already sanitized flags. */ 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))) { @@ -14301,8 +14303,26 @@ virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, 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, diff --git a/src/conf/virdomainlist.h b/src/conf/virdomainlist.h index caee592..7a066d2 100644 --- a/src/conf/virdomainlist.h +++ b/src/conf/virdomainlist.h @@ -60,6 +60,18 @@ VIR_CONNECT_LIST_FILTERS_AUTOSTART | \ VIR_CONNECT_LIST_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); diff --git a/src/libvirt.c b/src/libvirt.c index 23af5dd..1eb7bd0 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17059,16 +17059,27 @@ error: * * Provides the number of domain snapshots for this domain. * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_ROOTS, then the result is - * filtered to the number of snapshots that have no parents. Likewise, - * if @flags includes VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, then the result is - * filtered to the number of snapshots that have no children. Both flags - * can be used together to find unrelated snapshots. + * By default, this command covers all snapshots; it is also possible to + * limit things to just snapshots with no parents, when @flags includes + * VIR_DOMAIN_SNAPSHOT_LIST_ROOTS. Additional filters are provided in + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is - * the number of snapshots that also include metadata that would prevent - * the removal of the last reference to a domain; this value will either - * be 0 or the same value as if the flag were not given. + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. * * Returns the number of domain snapshots found or -1 in case of error. */ @@ -17113,18 +17124,36 @@ error: * of the array. The value to use for @nameslen can be determined by * virDomainSnapshotNum() with the same @flags. * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_ROOTS, then the result is - * filtered to the number of snapshots that have no parents. Likewise, - * if @flags includes VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, then the result is - * filtered to the number of snapshots that have no children. Both flags - * can be used together to find unrelated snapshots. + * By default, this command covers all snapshots; it is also possible to + * limit things to just snapshots with no parents, when @flags includes + * VIR_DOMAIN_SNAPSHOT_LIST_ROOTS. Additional filters are provided in + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is - * the number of snapshots that also include metadata that would prevent - * the removal of the last reference to a domain; this value will either - * be 0 or the same value as if the flag were not given. + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. * * Returns the number of domain snapshots found or -1 in case of error. + * Note that this command is inherently racy: another connection can + * define a new snapshot between a call to virDomainSnapshotNum() and + * this call. You are only guaranteed that all currently defined + * snapshots were listed if the return is less than @nameslen. Likewise, + * you should be prepared for virDomainSnapshotLookupByName() to fail when + * converting a name from this call into a snapshot object, if another + * connection deletes the snapshot in the meantime. */ int virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, @@ -17169,16 +17198,27 @@ error: * * Provides the number of child snapshots for this domain snapshot. * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS, then the result - * includes all descendants, otherwise it is limited to direct children. + * By default, this command covers only direct children; it is also possible + * to expand things to cover all descendants, when @flags includes + * VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS. Also, some filters are provided in + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, then the result is - * filtered to the number of snapshots that have no children. + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is - * the number of snapshots that also include metadata that would prevent - * the removal of the last reference to a domain; this value will either - * be 0 or the same value as if the flag were not given. + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. * * Returns the number of domain snapshots found or -1 in case of error. */ @@ -17224,18 +17264,36 @@ error: * freeing each member of the array. The value to use for @nameslen can * be determined by virDomainSnapshotNumChildren() with the same @flags. * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS, then the result - * includes all descendants, otherwise it is limited to direct children. + * By default, this command covers only direct children; it is also possible + * to expand things to cover all descendants, when @flags includes + * VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS. Also, some filters are provided in + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, then the result is - * filtered to the number of snapshots that have no children. + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is - * the number of snapshots that also include metadata that would prevent - * the removal of the last reference to a domain; this value will either - * be 0 or the same value as if the flag were not given. + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. * * Returns the number of domain snapshots found or -1 in case of error. + * Note that this command is inherently racy: another connection can + * define a new snapshot between a call to virDomainSnapshotNumChildren() + * and this call. You are only guaranteed that all currently defined + * snapshots were listed if the return is less than @nameslen. Likewise, + * you should be prepared for virDomainSnapshotLookupByName() to fail when + * converting a name from this call into a snapshot object, if another + * connection deletes the snapshot in the meantime. */ int virDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, -- 1.7.10.2
On 06/15/12 06:18, Eric Blake wrote:
It turns out that one-bit filtering makes it hard to select the inverse set, so it is easier to provide filtering groups. For back-compat, omitting all bits within a group means the group is not used for filtering, and by definition of a group (each snapshot matches exactly one bit within the group, and the set of bits in the group covers all snapshots), selecting all bits also makes the group useless.
Unfortunately, virDomainSnapshotListChildren defined the bit VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS as an expansion rather than a filter, so we cannot make it part of a filter group, so that bit (and its counterpart VIR_DOMAIN_SNAPSHOT_LIST_ROOTS for virDomainSnapshotList) remains a single control bit.
* include/libvirt/libvirt.h.in (virDomainSnapshotListFlags): Add a couple more flags. * src/libvirt.c (virDomainSnapshotNum) (virDomainSnapshotNumChildren): Document them. (virDomainSnapshotListNames, virDomainSnapshotListChildrenNames): Likewise, and add thread-safety caveats. * src/conf/virdomainlist.h (VIR_DOMAIN_SNAPSHOT_FILTERS_*): New convenience macros. * src/conf/domain_conf.c (virDomainSnapshotObjListCopyNames) (virDomainSnapshotObjListCount): Support the new flags. ---
ACK. Peter
On 06/18/12 13:58, Peter Krempa wrote:
On 06/15/12 06:18, Eric Blake wrote:
It turns out that one-bit filtering makes it hard to select the inverse set, so it is easier to provide filtering groups. For back-compat, omitting all bits within a group means the group is not used for filtering, and by definition of a group (each snapshot matches exactly one bit within the group, and the set of bits in the group covers all snapshots), selecting all bits also makes the group useless.
Unfortunately, virDomainSnapshotListChildren defined the bit VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS as an expansion rather than a filter, so we cannot make it part of a filter group, so that bit (and its counterpart VIR_DOMAIN_SNAPSHOT_LIST_ROOTS for virDomainSnapshotList) remains a single control bit.
* include/libvirt/libvirt.h.in (virDomainSnapshotListFlags): Add a couple more flags. * src/libvirt.c (virDomainSnapshotNum) (virDomainSnapshotNumChildren): Document them. (virDomainSnapshotListNames, virDomainSnapshotListChildrenNames): Likewise, and add thread-safety caveats. * src/conf/virdomainlist.h (VIR_DOMAIN_SNAPSHOT_FILTERS_*): New convenience macros. * src/conf/domain_conf.c (virDomainSnapshotObjListCopyNames) (virDomainSnapshotObjListCount): Support the new flags. ---
ACK.
Peter
I'll try to polish and repost the virDomainListAll series ASAP so that the dependencies are satisfied. Peter
Previously, to get the name of all snapshots with children, it was necessary to get the name of all snapshots and then remove the name of leaf snapshots. This is racy, and somewhat inefficient compared to planned API additions. We can emulate --no-metadata on 0.9.5-0.9.12, but for now, there is no emulation of --no-leaves. * tools/virsh.c (cmdSnapshotList): Add new options --no-leaves and --no-metadata. (vshSnapshotList): Emulate where possible. * tools/virsh.pod (snapshot-list): Document them. --- v2: fix typo in flag name tools/virsh.c | 40 ++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 14 ++++++++++---- 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 4f841fd..3e1018e 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -17010,6 +17010,30 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, /* 0.9.13 will be adding a new listing API. */ + /* Assume that if we got this far, then the --no-leaves and + * --no-metadata flags were not supported. Disable groups that + * have no impact. */ + /* XXX should we emulate --no-leaves? */ + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES && + flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) + flags &= ~(VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES | + VIR_DOMAIN_SNAPSHOT_LIST_LEAVES); + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA && + flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA) + flags &= ~(VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA | + VIR_DOMAIN_SNAPSHOT_LIST_METADATA); + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA) { + /* We can emulate --no-metadata if --metadata was supported, + * since it was an all-or-none attribute on old servers. */ + count = virDomainSnapshotNum(dom, + VIR_DOMAIN_SNAPSHOT_LIST_METADATA); + if (count < 0) + goto cleanup; + if (count > 0) + return snaplist; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA; + } + /* This uses the interfaces available in 0.8.0-0.9.6 * (virDomainSnapshotListNames, global list only) and in * 0.9.7-0.9.12 (addition of virDomainSnapshotListChildrenNames @@ -17244,8 +17268,12 @@ static const vshCmdOptDef opts_snapshot_list[] = { {"parent", VSH_OT_BOOL, 0, N_("add a column showing parent snapshot")}, {"roots", VSH_OT_BOOL, 0, N_("list only snapshots without parents")}, {"leaves", VSH_OT_BOOL, 0, N_("list only snapshots without children")}, + {"no-leaves", VSH_OT_BOOL, 0, + N_("list only snapshots that are not leaves (with children)")}, {"metadata", VSH_OT_BOOL, 0, N_("list only snapshots that have metadata that would prevent undefine")}, + {"no-metadata", VSH_OT_BOOL, 0, + N_("list only snapshots that have no metadata managed by libvirt")}, {"tree", VSH_OT_BOOL, 0, N_("list snapshots in a tree")}, {"from", VSH_OT_DATA, 0, N_("limit list to children of given snapshot")}, {"current", VSH_OT_BOOL, 0, @@ -17274,6 +17302,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) struct tm time_info; bool tree = vshCommandOptBool(cmd, "tree"); bool leaves = vshCommandOptBool(cmd, "leaves"); + bool no_leaves = vshCommandOptBool(cmd, "no-leaves"); const char *from = NULL; virDomainSnapshotPtr start = NULL; vshSnapshotListPtr snaplist = NULL; @@ -17323,10 +17352,21 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) } flags |= VIR_DOMAIN_SNAPSHOT_LIST_LEAVES; } + if (no_leaves) { + if (tree) { + vshError(ctl, "%s", + _("--no-leaves and --tree are mutually exclusive")); + goto cleanup; + } + flags |= VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES; + } if (vshCommandOptBool(cmd, "metadata")) { flags |= VIR_DOMAIN_SNAPSHOT_LIST_METADATA; } + if (vshCommandOptBool(cmd, "no-metadata")) { + flags |= VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA; + } if (vshCommandOptBool(cmd, "descendants")) { if (!from) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 6408881..7cb4adc 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2552,7 +2552,7 @@ with I<--current>. =item B<snapshot-list> I<domain> [{I<--parent> | I<--roots> | I<--tree>}] [{[I<--from>] B<snapshot> | I<--current>} [I<--descendants>]] -[I<--metadata>] [I<--leaves>] +[I<--metadata>] [I<--no-metadata>] [I<--leaves>] [I<--no-leaves>] List all of the available snapshots for the given domain, defaulting to show columns for the snapshot name, creation time, and domain state. @@ -2572,13 +2572,19 @@ use of I<--descendants> is implied. This option is not compatible with I<--roots>. If I<--leaves> is specified, the list will be filtered to just -snapshots that have no children. This option is not compatible -with I<--tree>. +snapshots that have no children. Likewise, if I<--no-leaves> is +specified, the list will be filtered to just snapshots with +children. (Note that omitting both options does no filtering, +while providing both options will either produce the same list +or error out depending on whether the server recognizes the flags). +These options are not compatible with I<--tree>. If I<--metadata> is specified, the list will be filtered to just snapshots that involve libvirt metadata, and thus would prevent B<undefine> of a persistent domain, or be lost on B<destroy> of -a transient domain. +a transient domain. Likewise, if I<--no-metadata> is specified, +the list will be filtered to just snapshots that exist without +the need for libvirt metadata. =item B<snapshot-dumpxml> I<domain> I<snapshot> [I<--security-info>] -- 1.7.10.2
On 06/15/12 06:18, Eric Blake wrote:
Previously, to get the name of all snapshots with children, it was necessary to get the name of all snapshots and then remove the name of leaf snapshots. This is racy, and somewhat inefficient compared to planned API additions. We can emulate --no-metadata on 0.9.5-0.9.12, but for now, there is no emulation of --no-leaves.
* tools/virsh.c (cmdSnapshotList): Add new options --no-leaves and --no-metadata. (vshSnapshotList): Emulate where possible. * tools/virsh.pod (snapshot-list): Document them. ---
ACK. Peter
There was an inherent race between virDomainSnapshotNum() and virDomainSnapshotListNames(), where an additional snapshot could be created in the meantime, or where a snapshot could be deleted before converting the name back to a virDomainSnapshotPtr. It was also an awkward name: the function operates on domains, not domain snapshots. virDomainSnapshotListChildrenNames() suffered from the same inherent race, although its naming was nicer. This patch makes things nicer by grabbing a snapshot list atomically, in the format most useful to the user. * include/libvirt/libvirt.h.in (virDomainListAllSnapshots) (virDomainSnapshotListAllChildren): New declarations. * src/libvirt.c (virDomainListAllSnapshots) (virDomainSnapshotListAllChildren): New functions. * src/libvirt_public.syms (LIBVIRT_0.9.13): Export them. * src/driver.h (virDrvDomainListAllSnapshots) (virDrvDomainSnapshotListAllChildren): New callbacks. * python/generator.py (skip_function): Prepare for later hand-written versions. --- v2: guarantee NULL on error include/libvirt/libvirt.h.in | 13 +++- python/generator.py | 2 + src/driver.h | 12 ++++ src/libvirt.c | 149 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + 5 files changed, 177 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e1e8cbb..04244bc 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3389,7 +3389,8 @@ char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, * * Flags valid for virDomainSnapshotNum(), * virDomainSnapshotListNames(), virDomainSnapshotNumChildren(), and - * virDomainSnapshotListChildrenNames(). Note that the interpretation + * virDomainSnapshotListChildrenNames(), virDomainListAllSnapshots(), + * and virDomainSnapshotListAllChildren(). Note that the interpretation * of flag (1<<0) depends on which function it is passed to; but serves * to toggle the per-call default of whether the listing is shallow or * recursive. Remaining bits come in groups; if all bits from a group are @@ -3422,6 +3423,11 @@ int virDomainSnapshotNum(virDomainPtr domain, unsigned int flags); int virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, unsigned int flags); +/* Get all snapshot objects for this domain */ +int virDomainListAllSnapshots(virDomainPtr domain, + virDomainSnapshotPtr **snaps, + unsigned int flags); + /* Return the number of child snapshots for this snapshot */ int virDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, unsigned int flags); @@ -3431,6 +3437,11 @@ int virDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, char **names, int nameslen, unsigned int flags); +/* Get all snapshot object children for this snapshot */ +int virDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, + virDomainSnapshotPtr **snaps, + unsigned int flags); + /* Get a handle to a named snapshot */ virDomainSnapshotPtr virDomainSnapshotLookupByName(virDomainPtr domain, const char *name, diff --git a/python/generator.py b/python/generator.py index 0b6ac7c..2dada6e 100755 --- a/python/generator.py +++ b/python/generator.py @@ -454,6 +454,8 @@ skip_function = ( 'virSaveLastError', # We have our own python error wrapper 'virFreeError', # Only needed if we use virSaveLastError 'virConnectListAllDomains', #overridden in virConnect.py + 'virDomainListAllSnapshots', # overridden in virDomain.py + 'virDomainSnapshotListAllChildren', # overridden in virDomainSnapshot.py 'virStreamRecvAll', # Pure python libvirt-override-virStream.py 'virStreamSendAll', # Pure python libvirt-override-virStream.py diff --git a/src/driver.h b/src/driver.h index bbeca06..4a4b60f 100644 --- a/src/driver.h +++ b/src/driver.h @@ -628,6 +628,11 @@ typedef int unsigned int flags); typedef int + (*virDrvDomainListAllSnapshots)(virDomainPtr domain, + virDomainSnapshotPtr **snaps, + unsigned int flags); + +typedef int (*virDrvDomainSnapshotNumChildren)(virDomainSnapshotPtr snapshot, unsigned int flags); @@ -637,6 +642,11 @@ typedef int int nameslen, unsigned int flags); +typedef int + (*virDrvDomainSnapshotListAllChildren)(virDomainSnapshotPtr snapshot, + virDomainSnapshotPtr **snaps, + unsigned int flags); + typedef virDomainSnapshotPtr (*virDrvDomainSnapshotLookupByName)(virDomainPtr domain, const char *name, @@ -993,8 +1003,10 @@ struct _virDriver { virDrvDomainSnapshotGetXMLDesc domainSnapshotGetXMLDesc; virDrvDomainSnapshotNum domainSnapshotNum; virDrvDomainSnapshotListNames domainSnapshotListNames; + virDrvDomainListAllSnapshots domainListAllSnapshots; virDrvDomainSnapshotNumChildren domainSnapshotNumChildren; virDrvDomainSnapshotListChildrenNames domainSnapshotListChildrenNames; + virDrvDomainSnapshotListAllChildren domainSnapshotListAllChildren; virDrvDomainSnapshotLookupByName domainSnapshotLookupByName; virDrvDomainHasCurrentSnapshot domainHasCurrentSnapshot; virDrvDomainSnapshotGetParent domainSnapshotGetParent; diff --git a/src/libvirt.c b/src/libvirt.c index 1eb7bd0..c44bf0a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17192,6 +17192,79 @@ error: } /** + * virDomainListAllSnapshots: + * @domain: a domain object + * @snaps: pointer to variable to store the array containing snapshot objects, + * or NULL if the list is not required (just returns number of + * snapshots) + * @flags: bitwise-OR of supported virDomainSnapshotListFlags + * + * Collect the list of domain snapshots for the given domain, and allocate + * an array to store those objects. Caller is responsible for calling + * virDomainSnapshotFree() on each member of the array, then free() on the + * array itself. + * + * By default, this command covers all snapshots; it is also possible to + * limit things to just snapshots with no parents, when @flags includes + * VIR_DOMAIN_SNAPSHOT_LIST_ROOTS. Additional filters are provided in + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. + * + * Returns the number of domain snapshots found or -1 in case of error. + * On success, the array stored into @snaps is guaranteed to have an + * extra allocated element set to NULL, to make iteration easier. + */ +int +virDomainListAllSnapshots(virDomainPtr domain, virDomainSnapshotPtr **snaps, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "snaps=%p, flags=%x", snaps, flags); + + virResetLastError(); + + if (snaps) + *snaps = NULL; + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = domain->conn; + + if (conn->driver->domainListAllSnapshots) { + int ret = conn->driver->domainListAllSnapshots(domain, snaps, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); +error: + virDispatchError(conn); + return -1; +} + +/** * virDomainSnapshotNumChildren: * @snapshot: a domain snapshot object * @flags: bitwise-OR of supported virDomainSnapshotListFlags @@ -17336,6 +17409,82 @@ error: } /** + * virDomainSnapshotListAllChildren: + * @snapshot: a domain snapshot object + * @snaps: pointer to variable to store the array containing snapshot objects, + * or NULL if the list is not required (just returns number of + * snapshots) + * @flags: bitwise-OR of supported virDomainSnapshotListFlags + * + * Collect the list of domain snapshots that are children of the given + * snapshot, and allocate an array to store those objects. Caller is + * responsible for calling virDomainSnapshotFree() on each member of the + * array, then free() on the array itself. + * + * By default, this command covers only direct children; it is also possible + * to expand things to cover all descendants, when @flags includes + * VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS. Also, some filters are provided in + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. + * + * Returns the number of domain snapshots found or -1 in case of error. + * On success, the array stored into @snaps is guaranteed to have an + * extra allocated element set to NULL, to make iteration easier. + */ +int +virDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, + virDomainSnapshotPtr **snaps, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DEBUG("snapshot=%p, snaps=%p, flags=%x", snapshot, snaps, flags); + + virResetLastError(); + + if (snaps) + *snaps = NULL; + + if (!VIR_IS_DOMAIN_SNAPSHOT(snapshot)) { + virLibDomainSnapshotError(VIR_ERR_INVALID_DOMAIN_SNAPSHOT, + __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = snapshot->domain->conn; + + if (conn->driver->domainSnapshotListAllChildren) { + int ret = conn->driver->domainSnapshotListAllChildren(snapshot, snaps, + flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); +error: + virDispatchError(conn); + return -1; +} + +/** * virDomainSnapshotLookupByName: * @domain: a domain object * @name: name for the domain snapshot diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index ea49a68..2913a81 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -537,8 +537,10 @@ LIBVIRT_0.9.11 { LIBVIRT_0.9.13 { global: virConnectListAllDomains; + virDomainListAllSnapshots; virDomainSnapshotHasMetadata; virDomainSnapshotIsCurrent; + virDomainSnapshotListAllChildren; virDomainSnapshotRef; } LIBVIRT_0.9.11; -- 1.7.10.2
On 06/15/12 06:18, Eric Blake wrote:
There was an inherent race between virDomainSnapshotNum() and virDomainSnapshotListNames(), where an additional snapshot could be created in the meantime, or where a snapshot could be deleted before converting the name back to a virDomainSnapshotPtr. It was also an awkward name: the function operates on domains, not domain snapshots. virDomainSnapshotListChildrenNames() suffered from the same inherent race, although its naming was nicer.
This patch makes things nicer by grabbing a snapshot list atomically, in the format most useful to the user.
* include/libvirt/libvirt.h.in (virDomainListAllSnapshots) (virDomainSnapshotListAllChildren): New declarations. * src/libvirt.c (virDomainListAllSnapshots) (virDomainSnapshotListAllChildren): New functions. * src/libvirt_public.syms (LIBVIRT_0.9.13): Export them. * src/driver.h (virDrvDomainListAllSnapshots) (virDrvDomainSnapshotListAllChildren): New callbacks. * python/generator.py (skip_function): Prepare for later hand-written versions. ---
/** + * virDomainListAllSnapshots: + * @domain: a domain object + * @snaps: pointer to variable to store the array containing snapshot objects, + * or NULL if the list is not required (just returns number of + * snapshots) + * @flags: bitwise-OR of supported virDomainSnapshotListFlags + * + * Collect the list of domain snapshots for the given domain, and allocate + * an array to store those objects. Caller is responsible for calling + * virDomainSnapshotFree() on each member of the array, then free() on the + * array itself. + * + * By default, this command covers all snapshots; it is also possible to + * limit things to just snapshots with no parents, when @flags includes + * VIR_DOMAIN_SNAPSHOT_LIST_ROOTS. Additional filters are provided in + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. + * + * Returns the number of domain snapshots found or -1 in case of error.
See below ...
+ * On success, the array stored into @snaps is guaranteed to have an
/** + * virDomainSnapshotListAllChildren: + * @snapshot: a domain snapshot object + * @snaps: pointer to variable to store the array containing snapshot objects, + * or NULL if the list is not required (just returns number of + * snapshots) + * @flags: bitwise-OR of supported virDomainSnapshotListFlags + * + * Collect the list of domain snapshots that are children of the given + * snapshot, and allocate an array to store those objects. Caller is + * responsible for calling virDomainSnapshotFree() on each member of the + * array, then free() on the array itself. + * + * By default, this command covers only direct children; it is also possible + * to expand things to cover all descendants, when @flags includes + * VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS. Also, some filters are provided in + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. + * + * Returns the number of domain snapshots found or -1 in case of error.
As the new API guarantees to NULL the list on failure I'd state it here as I did in the domain list series: Returns the number of domain snapshots found or -1 and sets snaps to NULL in case of error.
+ * On success, the array stored into @snaps is guaranteed to have an + * extra allocated element set to NULL, to make iteration easier. + */ +int
ACK with the docs fixed. Peter
On 06/18/2012 06:22 AM, Peter Krempa wrote:
On 06/15/12 06:18, Eric Blake wrote:
There was an inherent race between virDomainSnapshotNum() and virDomainSnapshotListNames(), where an additional snapshot could be created in the meantime, or where a snapshot could be deleted before converting the name back to a virDomainSnapshotPtr. It was also an awkward name: the function operates on domains, not domain snapshots. virDomainSnapshotListChildrenNames() suffered from the same inherent race, although its naming was nicer.
+ * + * Returns the number of domain snapshots found or -1 in case of error.
As the new API guarantees to NULL the list on failure I'd state it here as I did in the domain list series:
Returns the number of domain snapshots found or -1 and sets snaps to NULL in case of error.
ACK with the docs fixed.
Sure, here's what I squashed in before pushing 6-8: diff --git i/src/libvirt.c w/src/libvirt.c index 79ed324..0aa50cb 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -17123,9 +17123,8 @@ error: * @flags: bitwise-OR of supported virDomainSnapshotListFlags * * Collect the list of domain snapshots for the given domain, and store - * their names in @names. Caller is responsible for freeing each member - * of the array. The value to use for @nameslen can be determined by - * virDomainSnapshotNum() with the same @flags. + * their names in @names. The value to use for @nameslen can be determined + * by virDomainSnapshotNum() with the same @flags. * * By default, this command covers all snapshots; it is also possible to * limit things to just snapshots with no parents, when @flags includes @@ -17149,14 +17148,17 @@ error: * whether they have metadata that would prevent the removal of the last * reference to a domain. * - * Returns the number of domain snapshots found or -1 in case of error. * Note that this command is inherently racy: another connection can * define a new snapshot between a call to virDomainSnapshotNum() and * this call. You are only guaranteed that all currently defined * snapshots were listed if the return is less than @nameslen. Likewise, * you should be prepared for virDomainSnapshotLookupByName() to fail when * converting a name from this call into a snapshot object, if another - * connection deletes the snapshot in the meantime. + * connection deletes the snapshot in the meantime. For more control over + * the results, see virDomainListAllSnapshots(). + * + * Returns the number of domain snapshots found or -1 in case of error. + * The caller is responsible for freeing each member of the array. */ int virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, @@ -17203,9 +17205,8 @@ error: * @flags: bitwise-OR of supported virDomainSnapshotListFlags * * Collect the list of domain snapshots for the given domain, and allocate - * an array to store those objects. Caller is responsible for calling - * virDomainSnapshotFree() on each member of the array, then free() on the - * array itself. + * an array to store those objects. This API solves the race inherent in + * virDomainSnapshotListNames(). * * By default, this command covers all snapshots; it is also possible to * limit things to just snapshots with no parents, when @flags includes @@ -17229,9 +17230,12 @@ error: * whether they have metadata that would prevent the removal of the last * reference to a domain. * - * Returns the number of domain snapshots found or -1 in case of error. - * On success, the array stored into @snaps is guaranteed to have an - * extra allocated element set to NULL, to make iteration easier. + * Returns the number of domain snapshots found or -1 and sets @snaps to + * NULL in case of error. On success, the array stored into @snaps is + * guaranteed to have an extra allocated element set to NULL but not included + * in the return count, to make iteration easier. The caller is responsible + * for calling virDomainSnapshotFree() on each array element, then calling + * free() on @snaps. */ int virDomainListAllSnapshots(virDomainPtr domain, virDomainSnapshotPtr **snaps, @@ -17336,9 +17340,9 @@ error: * @flags: bitwise-OR of supported virDomainSnapshotListFlags * * Collect the list of domain snapshots that are children of the given - * snapshot, and store their names in @names. Caller is responsible for - * freeing each member of the array. The value to use for @nameslen can - * be determined by virDomainSnapshotNumChildren() with the same @flags. + * snapshot, and store their names in @names. The value to use for + * @nameslen can be determined by virDomainSnapshotNumChildren() with + * the same @flags. * * By default, this command covers only direct children; it is also possible * to expand things to cover all descendants, when @flags includes @@ -17369,7 +17373,11 @@ error: * snapshots were listed if the return is less than @nameslen. Likewise, * you should be prepared for virDomainSnapshotLookupByName() to fail when * converting a name from this call into a snapshot object, if another - * connection deletes the snapshot in the meantime. + * connection deletes the snapshot in the meantime. For more control over + * the results, see virDomainSnapshotListAllChildren(). + * + * Returns the number of domain snapshots found or -1 in case of error. + * The caller is responsible for freeing each member of the array. */ int virDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, @@ -17420,9 +17428,8 @@ error: * @flags: bitwise-OR of supported virDomainSnapshotListFlags * * Collect the list of domain snapshots that are children of the given - * snapshot, and allocate an array to store those objects. Caller is - * responsible for calling virDomainSnapshotFree() on each member of the - * array, then free() on the array itself. + * snapshot, and allocate an array to store those objects. This API solves + * the race inherent in virDomainSnapshotListChildrenNames(). * * By default, this command covers only direct children; it is also possible * to expand things to cover all descendants, when @flags includes @@ -17446,9 +17453,12 @@ error: * whether they have metadata that would prevent the removal of the last * reference to a domain. * - * Returns the number of domain snapshots found or -1 in case of error. - * On success, the array stored into @snaps is guaranteed to have an - * extra allocated element set to NULL, to make iteration easier. + * Returns the number of domain snapshots found or -1 and sets @snaps to + * NULL in case of error. On success, the array stored into @snaps is + * guaranteed to have an extra allocated element set to NULL but not included + * in the return count, to make iteration easier. The caller is responsible + * for calling virDomainSnapshotFree() on each array element, then calling + * free() on @snaps. */ int virDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Using the new API is so much shorter than the rest of the remainder of the function. * tools/virsh.c (vshSnapshotList): Use the new API. --- v2: fixes needed to pass testing tools/virsh.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/tools/virsh.c b/tools/virsh.c index 3e1018e..13b1c01 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -17002,13 +17002,42 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, int count = -1; bool descendants = false; bool roots = false; + virDomainSnapshotPtr *snaps; vshSnapshotListPtr snaplist = vshMalloc(ctl, sizeof(*snaplist)); vshSnapshotListPtr ret = NULL; const char *fromname = NULL; int start_index = -1; int deleted = 0; - /* 0.9.13 will be adding a new listing API. */ + /* Try the interface available in 0.9.13 and newer. */ + if (!ctl->useSnapshotOld) { + if (from) + count = virDomainSnapshotListAllChildren(from, &snaps, flags); + else + count = virDomainListAllSnapshots(dom, &snaps, flags); + } + if (count >= 0) { + /* When mixing --from and --tree, we also want a copy of from + * in the list, but with no parent for that one entry. */ + snaplist->snaps = vshCalloc(ctl, sizeof(*snaplist->snaps), + count + (tree && from)); + snaplist->nsnaps = count; + for (i = 0; i < count; i++) + snaplist->snaps[i].snap = snaps[i]; + VIR_FREE(snaps); + if (tree) { + for (i = 0; i < count; i++) { + if (vshGetSnapshotParent(ctl, snaplist->snaps[i].snap, + &snaplist->snaps[i].parent) < 0) + goto cleanup; + } + if (from) { + snaps[snaplist->nsnaps++] = from; + virDomainSnapshotRef(from); + } + } + goto success; + } /* Assume that if we got this far, then the --no-leaves and * --no-metadata flags were not supported. Disable groups that -- 1.7.10.2
On 06/15/12 06:18, Eric Blake wrote:
Using the new API is so much shorter than the rest of the remainder of the function.
* tools/virsh.c (vshSnapshotList): Use the new API. ---
v2: fixes needed to pass testing
tools/virsh.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 3e1018e..13b1c01 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -17002,13 +17002,42 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, + if (count >= 0) { + /* When mixing --from and --tree, we also want a copy of from + * in the list, but with no parent for that one entry. */ + snaplist->snaps = vshCalloc(ctl, sizeof(*snaplist->snaps), + count + (tree && from));
A little bit awkward on the first read, but correct.
+ snaplist->nsnaps = count; + for (i = 0; i < count; i++) + snaplist->snaps[i].snap = snaps[i]; + VIR_FREE(snaps);
ACK. Peter
This adds support for the new virDomainListAllSnapshots (a domain function) and virDomainSnapshotListAllChildren (a snapshot function) to the libvirt-python bindings. The implementation is done manually as the generator does not support wrapping lists of C pointers into python objects. * python/libvirt-override.c (libvirt_virDomainListAllSnapshots) (libvirt_virDomainSnapshotListAllChildren): New functions. * python/libvirt-override-api.xml: Document them. * python/libvirt-override-virDomain.py (listAllSnapshots): New file. * python/libvirt-override-virDomainSnapshot.py (listAllChildren): Likewise. * python/Makefile.am (CLASSES_EXTRA): Ship them. --- v2: no real change python/Makefile.am | 2 + python/libvirt-override-api.xml | 16 ++++- python/libvirt-override-virDomain.py | 11 +++ python/libvirt-override-virDomainSnapshot.py | 11 +++ python/libvirt-override.c | 92 ++++++++++++++++++++++++++ 5 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 python/libvirt-override-virDomain.py create mode 100644 python/libvirt-override-virDomainSnapshot.py diff --git a/python/Makefile.am b/python/Makefile.am index 02b59eb..97f21c3 100644 --- a/python/Makefile.am +++ b/python/Makefile.am @@ -24,6 +24,8 @@ DOCS = ${srcdir}/TODO CLASSES_EXTRA = \ libvirt-override-virConnect.py \ + libvirt-override-virDomain.py \ + libvirt-override-virDomainSnapshot.py \ libvirt-override-virStream.py EXTRA_DIST = \ diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index 2fd6dec..67ef36e 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -406,17 +406,29 @@ <arg name='flags' type='unsigned int' info='fine-tuning flags, currently unused, pass 0.'/> </function> <function name='virDomainSnapshotListNames' file='python'> - <info>collect the list of snapshots for the given domain</info> + <info>collect the list of snapshot names for the given domain</info> <arg name='dom' type='virDomainPtr' info='pointer to the domain'/> <arg name='flags' type='unsigned int' info='flags'/> <return type='str *' info='the list of Names or None in case of error'/> </function> + <function name='virDomainListAllSnapshots' file='python'> + <info>returns the list of snapshots for the given domain</info> + <arg name='dom' type='virDomainPtr' info='pointer to the domain'/> + <arg name='flags' type='unsigned int' info='flags'/> + <return type='snapshot *' info='the list of snapshots or None in case of error'/> + </function> <function name='virDomainSnapshotListChildrenNames' file='python'> - <info>collect the list of child snapshots for the given snapshot</info> + <info>collect the list of child snapshot names for the given snapshot</info> <arg name='snapshot' type='virDomainSnapshotPtr' info='pointer to the snapshot'/> <arg name='flags' type='unsigned int' info='flags'/> <return type='str *' info='the list of Names or None in case of error'/> </function> + <function name='virDomainSnapshotListAllChildren' file='python'> + <info>returns the list of child snapshots for the given snapshot</info> + <arg name='snapshot' type='virDomainSnapshotPtr' info='pointer to the snapshot'/> + <arg name='flags' type='unsigned int' info='flags'/> + <return type='snapshot *' info='the list of snapshots or None in case of error'/> + </function> <function name='virDomainRevertToSnapshot' file='python'> <info>revert the domain to the given snapshot</info> <arg name='dom' type='virDomainPtr' info='dummy domain pointer'/> diff --git a/python/libvirt-override-virDomain.py b/python/libvirt-override-virDomain.py new file mode 100644 index 0000000..ccc4d5f --- /dev/null +++ b/python/libvirt-override-virDomain.py @@ -0,0 +1,11 @@ + def listAllSnapshots(self, flags): + """List all snapshots and returns a list of snapshot objects""" + ret = libvirtmod.virDomainListAllSnapshots(self._o, flags) + if ret is None: + raise libvirtError("virDomainListAllSnapshots() failed", conn=self) + + retlist = list() + for snapptr in ret: + retlist.append(virDomainSnapshot(self, _obj=snapptr)) + + return retlist diff --git a/python/libvirt-override-virDomainSnapshot.py b/python/libvirt-override-virDomainSnapshot.py new file mode 100644 index 0000000..3da7bfd --- /dev/null +++ b/python/libvirt-override-virDomainSnapshot.py @@ -0,0 +1,11 @@ + def listAllChildren(self, flags): + """List all child snapshots and returns a list of snapshot objects""" + ret = libvirtmod.virDomainSnapshotListAllChildren(self._o, flags) + if ret is None: + raise libvirtError("virDomainSnapshotListAllChildren() failed", conn=self) + + retlist = list() + for snapptr in ret: + retlist.append(virDomainSnapshot(self, _obj=snapptr)) + + return retlist diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 8d1ede1..4370045 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -2127,6 +2127,51 @@ cleanup: } static PyObject * +libvirt_virDomainListAllSnapshots(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + PyObject *py_retval = NULL; + virDomainSnapshotPtr *snaps = NULL; + int c_retval, i; + virDomainPtr dom; + PyObject *pyobj_dom; + unsigned int flags; + PyObject *pyobj_snap; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainListAllSnapshots", + &pyobj_dom, &flags)) + return NULL; + dom = (virDomainPtr) PyvirDomain_Get(pyobj_dom); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainListAllSnapshots(dom, &snaps, flags); + LIBVIRT_END_ALLOW_THREADS; + if (c_retval < 0) + return VIR_PY_NONE; + + if (!(py_retval = PyList_New(c_retval))) + goto cleanup; + + for (i = 0; i < c_retval; i++) { + if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL || + PyList_SetItem(py_retval, i, pyobj_snap) < 0) { + Py_XDECREF(pyobj_snap); + Py_DECREF(py_retval); + py_retval = NULL; + goto cleanup; + } + snaps[i] = NULL; + } + +cleanup: + for (i = 0; i < c_retval; i++) + if (snaps[i]) + virDomainSnapshotFree(snaps[i]); + VIR_FREE(snaps); + return py_retval; +} + +static PyObject * libvirt_virDomainSnapshotListChildrenNames(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { @@ -2181,6 +2226,51 @@ cleanup: } static PyObject * +libvirt_virDomainSnapshotListAllChildren(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + PyObject *py_retval = NULL; + virDomainSnapshotPtr *snaps = NULL; + int c_retval, i; + virDomainSnapshotPtr parent; + PyObject *pyobj_parent; + unsigned int flags; + PyObject *pyobj_snap; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainSnapshotListAllChildren", + &pyobj_parent, &flags)) + return NULL; + parent = (virDomainSnapshotPtr) PyvirDomainSnapshot_Get(pyobj_parent); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainSnapshotListAllChildren(parent, &snaps, flags); + LIBVIRT_END_ALLOW_THREADS; + if (c_retval < 0) + return VIR_PY_NONE; + + if (!(py_retval = PyList_New(c_retval))) + goto cleanup; + + for (i = 0; i < c_retval; i++) { + if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL || + PyList_SetItem(py_retval, i, pyobj_snap) < 0) { + Py_XDECREF(pyobj_snap); + Py_DECREF(py_retval); + py_retval = NULL; + goto cleanup; + } + snaps[i] = NULL; + } + +cleanup: + for (i = 0; i < c_retval; i++) + if (snaps[i]) + virDomainSnapshotFree(snaps[i]); + VIR_FREE(snaps); + return py_retval; +} + +static PyObject * libvirt_virDomainRevertToSnapshot(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { int c_retval; @@ -5763,7 +5853,9 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virConnectBaselineCPU", libvirt_virConnectBaselineCPU, METH_VARARGS, NULL}, {(char *) "virDomainGetJobInfo", libvirt_virDomainGetJobInfo, METH_VARARGS, NULL}, {(char *) "virDomainSnapshotListNames", libvirt_virDomainSnapshotListNames, METH_VARARGS, NULL}, + {(char *) "virDomainListAllSnapshots", libvirt_virDomainListAllSnapshots, METH_VARARGS, NULL}, {(char *) "virDomainSnapshotListChildrenNames", libvirt_virDomainSnapshotListChildrenNames, METH_VARARGS, NULL}, + {(char *) "virDomainSnapshotListAllChildren", libvirt_virDomainSnapshotListAllChildren, METH_VARARGS, NULL}, {(char *) "virDomainRevertToSnapshot", libvirt_virDomainRevertToSnapshot, METH_VARARGS, NULL}, {(char *) "virDomainGetBlockJobInfo", libvirt_virDomainGetBlockJobInfo, METH_VARARGS, NULL}, {(char *) "virDomainSetBlockIoTune", libvirt_virDomainSetBlockIoTune, METH_VARARGS, NULL}, -- 1.7.10.2
On 06/15/12 06:18, Eric Blake wrote:
This adds support for the new virDomainListAllSnapshots (a domain function) and virDomainSnapshotListAllChildren (a snapshot function) to the libvirt-python bindings. The implementation is done manually as the generator does not support wrapping lists of C pointers into python objects.
* python/libvirt-override.c (libvirt_virDomainListAllSnapshots) (libvirt_virDomainSnapshotListAllChildren): New functions. * python/libvirt-override-api.xml: Document them. * python/libvirt-override-virDomain.py (listAllSnapshots): New file. * python/libvirt-override-virDomainSnapshot.py (listAllChildren): Likewise. * python/Makefile.am (CLASSES_EXTRA): Ship them. ---
diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 8d1ede1..4370045 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -2127,6 +2127,51 @@ cleanup: }
static PyObject * +libvirt_virDomainListAllSnapshots(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + PyObject *py_retval = NULL; + virDomainSnapshotPtr *snaps = NULL; + int c_retval, i; + virDomainPtr dom; + PyObject *pyobj_dom; + unsigned int flags; + PyObject *pyobj_snap; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainListAllSnapshots", + &pyobj_dom, &flags)) + return NULL; + dom = (virDomainPtr) PyvirDomain_Get(pyobj_dom); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainListAllSnapshots(dom, &snaps, flags); + LIBVIRT_END_ALLOW_THREADS; + if (c_retval < 0) + return VIR_PY_NONE; + + if (!(py_retval = PyList_New(c_retval))) + goto cleanup; + + for (i = 0; i < c_retval; i++) { + if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL || + PyList_SetItem(py_retval, i, pyobj_snap) < 0) { + Py_XDECREF(pyobj_snap); + Py_DECREF(py_retval); + py_retval = NULL; + goto cleanup; + } + snaps[i] = NULL; + } + +cleanup: + for (i = 0; i < c_retval; i++) + if (snaps[i]) + virDomainSnapshotFree(snaps[i]); + VIR_FREE(snaps); + return py_retval; +} + +static PyObject * libvirt_virDomainSnapshotListChildrenNames(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { @@ -2181,6 +2226,51 @@ cleanup: }
static PyObject * +libvirt_virDomainSnapshotListAllChildren(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + PyObject *py_retval = NULL; + virDomainSnapshotPtr *snaps = NULL; + int c_retval, i; + virDomainSnapshotPtr parent; + PyObject *pyobj_parent; + unsigned int flags; + PyObject *pyobj_snap; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainSnapshotListAllChildren", + &pyobj_parent, &flags)) + return NULL; + parent = (virDomainSnapshotPtr) PyvirDomainSnapshot_Get(pyobj_parent); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainSnapshotListAllChildren(parent, &snaps, flags); + LIBVIRT_END_ALLOW_THREADS; + if (c_retval < 0) + return VIR_PY_NONE; + + if (!(py_retval = PyList_New(c_retval))) + goto cleanup; + + for (i = 0; i < c_retval; i++) { + if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL || + PyList_SetItem(py_retval, i, pyobj_snap) < 0) {
Sadly, this code pattern doesn't save us from leaking memory: The PyCapsule objects, that are used to wrap libvirt pointers lack a destructor, so if this whole function fails, calling Py_DECREF() on the resulting output list doesn't save us from leaking all the processed references that were stored in the List. Later on, when the list of PyCapsules is converted into actual python-libvirt objects, those objects contain destructors that dispose the pointers properly. A workaround would be not to NULL members of the snap array and on successful end of the loop just set c_retval to 0 so that the cleanup loop is not called. But this still is not perfect. To fix all possibilities of leaking these pointers, we will need to provide destructor callbacks for PyCapsules that wrap libvirt pointers and increase the reference count, when the Capsule object is created. This will allow to call free on all pointers and those that are stored in python structures will remain in memory thanks to the reference count. This will probably require to drop destructors on the libvirt-python wrapping objects, so when they get deleted, they use the destructor of the PyCapsule holding the actual pointer instead of the wrapping object's destructor that cleans the reference to domains/snapshots/... This does not apply to libvirt_virDomainSnapshotListNames and others as strings are wrapped by python wrappers that (I assume) have destructors, but does apply on my domain listing bindings.
+ Py_XDECREF(pyobj_snap); + Py_DECREF(py_retval); + py_retval = NULL; + goto cleanup; + } + snaps[i] = NULL; + } + +cleanup: + for (i = 0; i < c_retval; i++) + if (snaps[i]) + virDomainSnapshotFree(snaps[i]); + VIR_FREE(snaps); + return py_retval; +} + +static PyObject * libvirt_virDomainRevertToSnapshot(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { int c_retval; @@ -5763,7 +5853,9 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virConnectBaselineCPU", libvirt_virConnectBaselineCPU, METH_VARARGS, NULL}, {(char *) "virDomainGetJobInfo", libvirt_virDomainGetJobInfo, METH_VARARGS, NULL}, {(char *) "virDomainSnapshotListNames", libvirt_virDomainSnapshotListNames, METH_VARARGS, NULL}, + {(char *) "virDomainListAllSnapshots", libvirt_virDomainListAllSnapshots, METH_VARARGS, NULL}, {(char *) "virDomainSnapshotListChildrenNames", libvirt_virDomainSnapshotListChildrenNames, METH_VARARGS, NULL}, + {(char *) "virDomainSnapshotListAllChildren", libvirt_virDomainSnapshotListAllChildren, METH_VARARGS, NULL}, {(char *) "virDomainRevertToSnapshot", libvirt_virDomainRevertToSnapshot, METH_VARARGS, NULL}, {(char *) "virDomainGetBlockJobInfo", libvirt_virDomainGetBlockJobInfo, METH_VARARGS, NULL}, {(char *) "virDomainSetBlockIoTune", libvirt_virDomainSetBlockIoTune, METH_VARARGS, NULL},
ACK, as the code follows a pattern, but memory leaks are possible yet very unlikely. Peter
On 06/18/2012 08:22 AM, Peter Krempa wrote:
On 06/15/12 06:18, Eric Blake wrote:
This adds support for the new virDomainListAllSnapshots (a domain function) and virDomainSnapshotListAllChildren (a snapshot function) to the libvirt-python bindings. The implementation is done manually as the generator does not support wrapping lists of C pointers into python objects.
+ for (i = 0; i < c_retval; i++) { + if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL || + PyList_SetItem(py_retval, i, pyobj_snap) < 0) {
Sadly, this code pattern doesn't save us from leaking memory: The PyCapsule objects, that are used to wrap libvirt pointers lack a destructor, so if this whole function fails, calling Py_DECREF() on the resulting output list doesn't save us from leaking all the processed references that were stored in the List. Later on, when the list of PyCapsules is converted into actual python-libvirt objects, those objects contain destructors that dispose the pointers properly.
A workaround would be not to NULL members of the snap array and on successful end of the loop just set c_retval to 0 so that the cleanup loop is not called. But this still is not perfect.
To fix all possibilities of leaking these pointers, we will need to provide destructor callbacks for PyCapsules that wrap libvirt pointers and increase the reference count, when the Capsule object is created.
This does not apply to libvirt_virDomainSnapshotListNames and others as strings are wrapped by python wrappers that (I assume) have destructors, but does apply on my domain listing bindings.
Sounds like a global cleanup worth doing over multiple affected functions at once...
ACK, as the code follows a pattern, but memory leaks are possible yet very unlikely.
so I pushed it as-is, leaving the PyCapsule fix for later. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The generator doesn't handle lists of virDomainSnapshotPtr, so this commit requires a bit more work than some RPC additions. * src/remote/remote_protocol.x (REMOTE_PROC_DOMAIN_LIST_ALL_SNAPSHOTS) (REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_ALL_CHILDREN): New RPC calls, with corresponding structs. * daemon/remote.c (remoteDispatchDomainListAllSnapshots) (remoteDispatchDomainSnapshotListAllChildren): New functions. * src/remote/remote_driver.c (remoteDomainListAllSnapshots) (remoteDomainSnapshotListAllChildren): Likewise. * src/remote_protocol-structs: Regenerate. --- v2: no real change daemon/remote.c | 126 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 126 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 26 ++++++++- src/remote_protocol-structs | 26 +++++++++ 4 files changed, 303 insertions(+), 1 deletion(-) diff --git a/daemon/remote.c b/daemon/remote.c index 8dd5a4f..21bc3f6 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3797,6 +3797,132 @@ cleanup: return rv; } +static int +remoteDispatchDomainListAllSnapshots(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_list_all_snapshots_args *args, + remote_domain_list_all_snapshots_ret *ret) +{ + virDomainSnapshotPtr *snaps = NULL; + int nsnaps = 0; + int i; + int rv = -1; + struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + virDomainPtr dom = NULL; + + if (!priv->conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = get_nonnull_domain(priv->conn, args->dom))) + goto cleanup; + + if ((nsnaps = virDomainListAllSnapshots(dom, + args->need_results ? &snaps : NULL, + args->flags)) < 0) + goto cleanup; + + if (snaps && nsnaps) { + if (VIR_ALLOC_N(ret->snapshots.snapshots_val, nsnaps) < 0) { + virReportOOMError(); + goto cleanup; + } + + ret->snapshots.snapshots_len = nsnaps; + + for (i = 0; i < nsnaps; i++) + make_nonnull_domain_snapshot(ret->snapshots.snapshots_val + i, + snaps[i]); + } else { + ret->snapshots.snapshots_len = 0; + ret->snapshots.snapshots_val = NULL; + } + + ret->ret = nsnaps; + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (dom) + virDomainFree(dom); + if (snaps) { + for (i = 0; i < nsnaps; i++) + virDomainSnapshotFree(snaps[i]); + VIR_FREE(snaps); + } + return rv; +} + +static int +remoteDispatchDomainSnapshotListAllChildren(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_snapshot_list_all_children_args *args, + remote_domain_snapshot_list_all_children_ret *ret) +{ + virDomainSnapshotPtr *snaps = NULL; + int nsnaps = 0; + int i; + int rv = -1; + struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); + virDomainPtr dom = NULL; + virDomainSnapshotPtr snapshot = NULL; + + if (!priv->conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (!(dom = get_nonnull_domain(priv->conn, args->snapshot.dom))) + goto cleanup; + + if (!(snapshot = get_nonnull_domain_snapshot(dom, args->snapshot))) + goto cleanup; + + if ((nsnaps = virDomainSnapshotListAllChildren(snapshot, + args->need_results ? &snaps : NULL, + args->flags)) < 0) + goto cleanup; + + if (snaps && nsnaps) { + if (VIR_ALLOC_N(ret->snapshots.snapshots_val, nsnaps) < 0) { + virReportOOMError(); + goto cleanup; + } + + ret->snapshots.snapshots_len = nsnaps; + + for (i = 0; i < nsnaps; i++) + make_nonnull_domain_snapshot(ret->snapshots.snapshots_val + i, + snaps[i]); + } else { + ret->snapshots.snapshots_len = 0; + ret->snapshots.snapshots_val = NULL; + } + + ret->ret = nsnaps; + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + if (snapshot) + virDomainSnapshotFree(snapshot); + if (dom) + virDomainFree(dom); + if (snaps) { + for (i = 0; i < nsnaps; i++) + virDomainSnapshotFree(snaps[i]); + VIR_FREE(snaps); + } + return rv; +} + /*----- Helpers. -----*/ /* get_nonnull_domain and get_nonnull_network turn an on-wire diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 28f12a4..7c1f0cc 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -4873,6 +4873,130 @@ done: return rv; } +static int +remoteDomainListAllSnapshots(virDomainPtr dom, + virDomainSnapshotPtr **snapshots, + unsigned int flags) +{ + int rv = -1; + int i; + virDomainSnapshotPtr *snaps = NULL; + remote_domain_list_all_snapshots_args args; + remote_domain_list_all_snapshots_ret ret; + + struct private_data *priv = dom->conn->privateData; + + remoteDriverLock(priv); + + args.need_results = !!snapshots; + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + if (call (dom->conn, + priv, + 0, + REMOTE_PROC_DOMAIN_LIST_ALL_SNAPSHOTS, + (xdrproc_t) xdr_remote_domain_list_all_snapshots_args, + (char *) &args, + (xdrproc_t) xdr_remote_domain_list_all_snapshots_ret, + (char *) &ret) == -1) + goto done; + + if (snapshots) { + if (VIR_ALLOC_N(snaps, ret.snapshots.snapshots_len + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + for (i = 0; i < ret.snapshots.snapshots_len; i++) { + snaps[i] = get_nonnull_domain_snapshot(dom, ret.snapshots.snapshots_val[i]); + if (!snaps[i]) { + virReportOOMError(); + goto cleanup; + } + } + *snapshots = snaps; + snaps = NULL; + } + + rv = ret.ret; + +cleanup: + if (snaps) { + for (i = 0; i < ret.snapshots.snapshots_len; i++) + if (snaps[i]) + virDomainSnapshotFree(snaps[i]); + VIR_FREE(snaps); + } + + xdr_free((xdrproc_t) xdr_remote_domain_list_all_snapshots_ret, (char *) &ret); + +done: + remoteDriverUnlock(priv); + return rv; +} + +static int +remoteDomainSnapshotListAllChildren(virDomainSnapshotPtr parent, + virDomainSnapshotPtr **snapshots, + unsigned int flags) +{ + int rv = -1; + int i; + virDomainSnapshotPtr *snaps = NULL; + remote_domain_snapshot_list_all_children_args args; + remote_domain_snapshot_list_all_children_ret ret; + + struct private_data *priv = parent->domain->conn->privateData; + + remoteDriverLock(priv); + + args.need_results = !!snapshots; + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + if (call (parent->domain->conn, + priv, + 0, + REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_ALL_CHILDREN, + (xdrproc_t) xdr_remote_domain_snapshot_list_all_children_args, + (char *) &args, + (xdrproc_t) xdr_remote_domain_snapshot_list_all_children_ret, + (char *) &ret) == -1) + goto done; + + if (snapshots) { + if (VIR_ALLOC_N(snaps, ret.snapshots.snapshots_len + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + for (i = 0; i < ret.snapshots.snapshots_len; i++) { + snaps[i] = get_nonnull_domain_snapshot(parent->domain, ret.snapshots.snapshots_val[i]); + if (!snaps[i]) { + virReportOOMError(); + goto cleanup; + } + } + *snapshots = snaps; + snaps = NULL; + } + + rv = ret.ret; + +cleanup: + if (snaps) { + for (i = 0; i < ret.snapshots.snapshots_len; i++) + if (snaps[i]) + virDomainSnapshotFree(snaps[i]); + VIR_FREE(snaps); + } + + xdr_free((xdrproc_t) xdr_remote_domain_snapshot_list_all_children_ret, (char *) &ret); + +done: + remoteDriverUnlock(priv); + return rv; +} + static void remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event) { @@ -5138,7 +5262,9 @@ static virDriver remote_driver = { .domainSnapshotGetXMLDesc = remoteDomainSnapshotGetXMLDesc, /* 0.8.0 */ .domainSnapshotNum = remoteDomainSnapshotNum, /* 0.8.0 */ .domainSnapshotListNames = remoteDomainSnapshotListNames, /* 0.8.0 */ + .domainListAllSnapshots = remoteDomainListAllSnapshots, /* 0.9.13 */ .domainSnapshotNumChildren = remoteDomainSnapshotNumChildren, /* 0.9.7 */ + .domainSnapshotListAllChildren = remoteDomainSnapshotListAllChildren, /* 0.9.13 */ .domainSnapshotListChildrenNames = remoteDomainSnapshotListChildrenNames, /* 0.9.7 */ .domainSnapshotLookupByName = remoteDomainSnapshotLookupByName, /* 0.8.0 */ .domainHasCurrentSnapshot = remoteDomainHasCurrentSnapshot, /* 0.8.0 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 865cfe6..1da9f3e 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2249,6 +2249,17 @@ struct remote_domain_snapshot_list_names_ret { remote_nonnull_string names<REMOTE_DOMAIN_SNAPSHOT_LIST_NAMES_MAX>; /* insert@1 */ }; +struct remote_domain_list_all_snapshots_args { + remote_nonnull_domain dom; + int need_results; + unsigned int flags; +}; + +struct remote_domain_list_all_snapshots_ret { + remote_nonnull_domain_snapshot snapshots<>; + int ret; +}; + struct remote_domain_snapshot_num_children_args { remote_nonnull_domain_snapshot snap; unsigned int flags; @@ -2268,6 +2279,17 @@ struct remote_domain_snapshot_list_children_names_ret { remote_nonnull_string names<REMOTE_DOMAIN_SNAPSHOT_LIST_NAMES_MAX>; /* insert@1 */ }; +struct remote_domain_snapshot_list_all_children_args { + remote_nonnull_domain_snapshot snapshot; + int need_results; + unsigned int flags; +}; + +struct remote_domain_snapshot_list_all_children_ret { + remote_nonnull_domain_snapshot snapshots<>; + int ret; +}; + struct remote_domain_snapshot_lookup_by_name_args { remote_nonnull_domain dom; remote_nonnull_string name; @@ -2814,7 +2836,9 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SNAPSHOT_IS_CURRENT = 271, /* autogen autogen */ REMOTE_PROC_DOMAIN_SNAPSHOT_HAS_METADATA = 272, /* autogen autogen */ - REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS = 273 /* skipgen skipgen priority:high */ + REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS = 273, /* skipgen skipgen priority:high */ + REMOTE_PROC_DOMAIN_LIST_ALL_SNAPSHOTS = 274, /* skipgen skipgen priority:high */ + REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_ALL_CHILDREN = 275 /* skipgen skipgen priority:high */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 63d70e0..b667527 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -1709,6 +1709,18 @@ struct remote_domain_snapshot_list_names_ret { remote_nonnull_string * names_val; } names; }; +struct remote_domain_list_all_snapshots_args { + remote_nonnull_domain dom; + int need_results; + u_int flags; +}; +struct remote_domain_list_all_snapshots_ret { + struct { + u_int snapshots_len; + remote_nonnull_domain_snapshot * snapshots_val; + } snapshots; + int ret; +}; struct remote_domain_snapshot_num_children_args { remote_nonnull_domain_snapshot snap; u_int flags; @@ -1727,6 +1739,18 @@ struct remote_domain_snapshot_list_children_names_ret { remote_nonnull_string * names_val; } names; }; +struct remote_domain_snapshot_list_all_children_args { + remote_nonnull_domain_snapshot snapshot; + int need_results; + u_int flags; +}; +struct remote_domain_snapshot_list_all_children_ret { + struct { + u_int snapshots_len; + remote_nonnull_domain_snapshot * snapshots_val; + } snapshots; + int ret; +}; struct remote_domain_snapshot_lookup_by_name_args { remote_nonnull_domain dom; remote_nonnull_string name; @@ -2220,4 +2244,6 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SNAPSHOT_IS_CURRENT = 271, REMOTE_PROC_DOMAIN_SNAPSHOT_HAS_METADATA = 272, REMOTE_PROC_CONNECT_LIST_ALL_DOMAINS = 273, + REMOTE_PROC_DOMAIN_LIST_ALL_SNAPSHOTS = 274, + REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_ALL_CHILDREN = 275, }; -- 1.7.10.2
On 06/15/12 06:18, Eric Blake wrote:
The generator doesn't handle lists of virDomainSnapshotPtr, so this commit requires a bit more work than some RPC additions.
* src/remote/remote_protocol.x (REMOTE_PROC_DOMAIN_LIST_ALL_SNAPSHOTS) (REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_ALL_CHILDREN): New RPC calls, with corresponding structs. * daemon/remote.c (remoteDispatchDomainListAllSnapshots) (remoteDispatchDomainSnapshotListAllChildren): New functions. * src/remote/remote_driver.c (remoteDomainListAllSnapshots) (remoteDomainSnapshotListAllChildren): Likewise. * src/remote_protocol-structs: Regenerate. --- --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -2249,6 +2249,17 @@ struct remote_domain_snapshot_list_names_ret { remote_nonnull_string names<REMOTE_DOMAIN_SNAPSHOT_LIST_NAMES_MAX>; /* insert@1 */ };
+struct remote_domain_list_all_snapshots_args { + remote_nonnull_domain dom; + int need_results; + unsigned int flags; +}; + +struct remote_domain_list_all_snapshots_ret { + remote_nonnull_domain_snapshot snapshots<>; + int ret; +}; + struct remote_domain_snapshot_num_children_args { remote_nonnull_domain_snapshot snap; unsigned int flags; @@ -2268,6 +2279,17 @@ struct remote_domain_snapshot_list_children_names_ret { remote_nonnull_string names<REMOTE_DOMAIN_SNAPSHOT_LIST_NAMES_MAX>; /* insert@1 */ };
+struct remote_domain_snapshot_list_all_children_args { + remote_nonnull_domain_snapshot snapshot; + int need_results; + unsigned int flags; +}; + +struct remote_domain_snapshot_list_all_children_ret { + remote_nonnull_domain_snapshot snapshots<>;
Hopefully we'll go with unbounded arrays in the future, but as with my listing function: Shouldn't we use the limit that was imposed on name list length also on this list? (REMOTE_DOMAIN_SNAPSHOT_LIST_NAMES_MAX).
+ int ret; +}; + struct remote_domain_snapshot_lookup_by_name_args { remote_nonnull_domain dom; remote_nonnull_string name;
ACK with the limit added. I'm a fan of unbounded arrays, but I think we should be consistent. Peter
On 06/18/2012 08:48 AM, Peter Krempa wrote:
On 06/15/12 06:18, Eric Blake wrote:
The generator doesn't handle lists of virDomainSnapshotPtr, so this commit requires a bit more work than some RPC additions.
+struct remote_domain_snapshot_list_all_children_ret { + remote_nonnull_domain_snapshot snapshots<>;
Hopefully we'll go with unbounded arrays in the future, but as with my listing function: Shouldn't we use the limit that was imposed on name list length also on this list? (REMOTE_DOMAIN_SNAPSHOT_LIST_NAMES_MAX).
Here's a case where I was copying from your series :) Whichever way you go in your commit, I will copy.
+ int ret; +}; + struct remote_domain_snapshot_lookup_by_name_args { remote_nonnull_domain dom; remote_nonnull_string name;
ACK with the limit added. I'm a fan of unbounded arrays, but I think we should be consistent.
I'm starting to agree that unbounded intermediates are okay because we still have the overall bound on the max RPC; but I'm also thinking that a global swap to unbounded would be better than introducing one instance of an unbounded use. So being consistent by having this commit preserve a bound is fine by me. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/18/2012 09:00 AM, Eric Blake wrote:
On 06/18/2012 08:48 AM, Peter Krempa wrote:
On 06/15/12 06:18, Eric Blake wrote:
The generator doesn't handle lists of virDomainSnapshotPtr, so this commit requires a bit more work than some RPC additions.
+struct remote_domain_snapshot_list_all_children_ret { + remote_nonnull_domain_snapshot snapshots<>;
Hopefully we'll go with unbounded arrays in the future, but as with my listing function: Shouldn't we use the limit that was imposed on name list length also on this list? (REMOTE_DOMAIN_SNAPSHOT_LIST_NAMES_MAX).
Here's a case where I was copying from your series :)
Whichever way you go in your commit, I will copy.
Looks like you used unbounded <>, so I copied.
ACK with the limit added. I'm a fan of unbounded arrays, but I think we should be consistent.
I will push this one as-is. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Wraps the conversion from 'char *name' to virDomainSnapshotPtr in a reusable manner. * src/conf/virdomainlist.h (virDomainListSnapshots): New declaration. * src/conf/virdomainlist.c (virDomainListSnapshots): Implement it. * src/libvirt_private.syms (virdomainlist.h): Export it. --- v2: no real change src/conf/virdomainlist.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/conf/virdomainlist.h | 6 ++++++ src/libvirt_private.syms | 1 + 3 files changed, 49 insertions(+) diff --git a/src/conf/virdomainlist.c b/src/conf/virdomainlist.c index 246b838..c680f18 100644 --- a/src/conf/virdomainlist.c +++ b/src/conf/virdomainlist.c @@ -180,3 +180,45 @@ cleanup: VIR_FREE(data.domains); 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; + } + + virDomainSnapshotObjListGetNames(snapshots, from, names, count, flags); + 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++) + if (list[i]) + virDomainSnapshotFree(list[i]); + VIR_FREE(list); + } + return ret; +} diff --git a/src/conf/virdomainlist.h b/src/conf/virdomainlist.h index 7a066d2..e623129 100644 --- a/src/conf/virdomainlist.h +++ b/src/conf/virdomainlist.h @@ -75,4 +75,10 @@ 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 /* __VIR_DOMAIN_LIST_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b37fe68..2fe5068 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1239,6 +1239,7 @@ virDBusGetSystemBus; # virdomainlist.h virDomainList; +virDomainListSnapshots; # virfile.h -- 1.7.10.2
On 06/15/12 06:18, Eric Blake wrote:
Wraps the conversion from 'char *name' to virDomainSnapshotPtr in a reusable manner.
* src/conf/virdomainlist.h (virDomainListSnapshots): New declaration. * src/conf/virdomainlist.c (virDomainListSnapshots): Implement it. * src/libvirt_private.syms (virdomainlist.h): Export it. ---
ACK. Peter
On 06/14/2012 10:18 PM, Eric Blake wrote:
Wraps the conversion from 'char *name' to virDomainSnapshotPtr in a reusable manner.
* src/conf/virdomainlist.h (virDomainListSnapshots): New declaration. * src/conf/virdomainlist.c (virDomainListSnapshots): Implement it. * src/libvirt_private.syms (virdomainlist.h): Export it. ---
v2: no real change
Turns out I needed to squash this in after all, to avoid calling virGetDomainSnapshot with a NULL argument.
+ + virDomainSnapshotObjListGetNames(snapshots, from, names, count, flags); + for (i = 0; i < count; i++) + if ((list[i] = virGetDomainSnapshot(dom, names[i])) == NULL) + goto cleanup;
diff --git i/src/conf/virdomainlist.c w/src/conf/virdomainlist.c index 663591a..6ac2af1 100644 --- i/src/conf/virdomainlist.c +++ w/src/conf/virdomainlist.c @@ -202,7 +202,9 @@ virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, goto cleanup; } - virDomainSnapshotObjListGetNames(snapshots, from, names, count, flags); + 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; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The two new functions are very similar to the existing functions; just a matter of different arguments and a call to a different helper function. * src/qemu/qemu_driver.c (qemuDomainSnapshotListNames) (qemuDomainSnapshotNum, qemuDomainSnapshotListChildrenNames) (qemuDomainSnapshotNumChildren): Support new flags. (qemuDomainListAllSnapshots): New functions. --- v2: use new convenience macros instead of open-coding filter bits src/qemu/qemu_driver.c | 94 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 78 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ce90ddf..ca8f0e7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -92,6 +92,7 @@ #include "virnodesuspend.h" #include "virtime.h" #include "virtypedparam.h" +#include "virdomainlist.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -10616,8 +10617,7 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | - VIR_DOMAIN_SNAPSHOT_LIST_METADATA | - VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, -1); + VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); @@ -10647,8 +10647,7 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | - VIR_DOMAIN_SNAPSHOT_LIST_METADATA | - VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, -1); + VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); @@ -10660,10 +10659,6 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, goto cleanup; } - /* All qemu snapshots have libvirt metadata, so - * VIR_DOMAIN_SNAPSHOT_LIST_METADATA makes no difference to our - * answer. */ - n = virDomainSnapshotObjListNum(&vm->snapshots, NULL, flags); cleanup: @@ -10674,6 +10669,36 @@ cleanup: } static int +qemuDomainListAllSnapshots(virDomainPtr domain, virDomainSnapshotPtr **snaps, + unsigned int flags) +{ + struct qemud_driver *driver = domain->conn->privateData; + virDomainObjPtr vm = NULL; + int n = -1; + + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_ROOTS | + VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, domain->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(domain->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + n = virDomainListSnapshots(&vm->snapshots, NULL, domain, snaps, flags); + +cleanup: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return n; +} + +static int qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, char **names, int nameslen, @@ -10685,8 +10710,7 @@ qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | - VIR_DOMAIN_SNAPSHOT_LIST_METADATA | - VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, -1); + VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, snapshot->domain->uuid); @@ -10726,8 +10750,7 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, int n = -1; virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | - VIR_DOMAIN_SNAPSHOT_LIST_METADATA | - VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, -1); + VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, snapshot->domain->uuid); @@ -10747,10 +10770,6 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, goto cleanup; } - /* All qemu snapshots have libvirt metadata, so - * VIR_DOMAIN_SNAPSHOT_LIST_METADATA makes no difference to our - * answer. */ - n = virDomainSnapshotObjListNum(&vm->snapshots, snap, flags); cleanup: @@ -10760,6 +10779,47 @@ cleanup: return n; } +static int +qemuDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, + virDomainSnapshotPtr **snaps, + unsigned int flags) +{ + struct qemud_driver *driver = snapshot->domain->conn->privateData; + virDomainObjPtr vm = NULL; + virDomainSnapshotObjPtr snap = NULL; + int n = -1; + + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS | + VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); + + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, snapshot->domain->uuid); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(snapshot->domain->uuid, uuidstr); + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + snap = virDomainSnapshotFindByName(&vm->snapshots, snapshot->name); + if (!snap) { + qemuReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, + _("no domain snapshot with matching name '%s'"), + snapshot->name); + goto cleanup; + } + + n = virDomainListSnapshots(&vm->snapshots, snap, snapshot->domain, snaps, + flags); + +cleanup: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return n; +} + static virDomainSnapshotPtr qemuDomainSnapshotLookupByName(virDomainPtr domain, const char *name, unsigned int flags) @@ -13167,8 +13227,10 @@ static virDriver qemuDriver = { .domainSnapshotGetXMLDesc = qemuDomainSnapshotGetXMLDesc, /* 0.8.0 */ .domainSnapshotNum = qemuDomainSnapshotNum, /* 0.8.0 */ .domainSnapshotListNames = qemuDomainSnapshotListNames, /* 0.8.0 */ + .domainListAllSnapshots = qemuDomainListAllSnapshots, /* 0.9.13 */ .domainSnapshotNumChildren = qemuDomainSnapshotNumChildren, /* 0.9.7 */ .domainSnapshotListChildrenNames = qemuDomainSnapshotListChildrenNames, /* 0.9.7 */ + .domainSnapshotListAllChildren = qemuDomainSnapshotListAllChildren, /* 0.9.13 */ .domainSnapshotLookupByName = qemuDomainSnapshotLookupByName, /* 0.8.0 */ .domainHasCurrentSnapshot = qemuDomainHasCurrentSnapshot, /* 0.8.0 */ .domainSnapshotGetParent = qemuDomainSnapshotGetParent, /* 0.9.7 */ -- 1.7.10.2
On 06/15/12 06:18, Eric Blake wrote:
The two new functions are very similar to the existing functions; just a matter of different arguments and a call to a different helper function.
* src/qemu/qemu_driver.c (qemuDomainSnapshotListNames) (qemuDomainSnapshotNum, qemuDomainSnapshotListChildrenNames) (qemuDomainSnapshotNumChildren): Support new flags. (qemuDomainListAllSnapshots): New functions. ---
ACK. Peter
On 06/18/2012 08:58 AM, Peter Krempa wrote:
On 06/15/12 06:18, Eric Blake wrote:
The two new functions are very similar to the existing functions; just a matter of different arguments and a call to a different helper function.
* src/qemu/qemu_driver.c (qemuDomainSnapshotListNames) (qemuDomainSnapshotNum, qemuDomainSnapshotListChildrenNames) (qemuDomainSnapshotNumChildren): Support new flags. (qemuDomainListAllSnapshots): New functions. ---
ACK.
Thanks again for the review; series pushed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/14/2012 10:18 PM, Eric Blake wrote:
This is a v2 of the remaining patches in: https://www.redhat.com/archives/libvir-list/2012-June/msg00273.html https://www.redhat.com/archives/libvir-list/2012-June/msg00284.html
It assumes that Peter's list all domain patches have been applied already, at least through patch 6/12: https://www.redhat.com/archives/libvir-list/2012-June/msg00316.html
In other words, this series will not apply to current libvirt.git at the time of this mail. And since I developed against v3 of Peter's patches and he has some tweaks to make, I will probably have to make minor tweaks to rebase on top of his eventual v4. I will follow up with a link to a git repo with Peter's v3 and my series, and/or my rebase on top of his v4.
http://repo.or.cz/w/libvirt/ericb.git/shortlog/refs/heads/snapshot git fetch git://repo.or.cz/libvirt/ericb.git snapshot -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa